[PATCH 0/2] enhance "git restore --worktree --staged" behavior

[PATCH 0/2] enhance "git restore --worktree --staged" behavior

[Eric Sunshine]

This series addresses a shortcoming or two when git-restore's --worktree
and --staged options are combined. The first patch tightens the
implementation to match the documentation. The second patch loosens the
documented restriction by adding a bit of DWIMing to make it more
convenient to combine the two options.

Although the second patch effectively throws away the changes of the
first patch, I kept them separate in case someone comes up with a good
objection to the new DWIMing (which escaped me), in which case the first
patch can be kept and the second thrown away.

Eric Sunshine (2):
  restore: require --source when combining --worktree and --staged
  restore: default to HEAD when combining --worktree and --staged

 Documentation/git-restore.txt | 13 +++++++------
 builtin/checkout.c            |  6 +++---
 t/t2070-restore.sh            | 11 +++++++++++
 3 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.26.2.526.g744177e7f7

[PATCH 1/2] restore: require --source when combining --worktree and --staged

[Eric Sunshine]

The default restore source for --worktree is the index, and the default
source for --staged is HEAD. When combining --worktree and --staged in
the same invocation, the restore source is ambiguous ("should it restore
from the index or from HEAD?"). To avoid such ambiguity, the git-restore
documentation has always stated that --source must be used when
combining --worktree and --staged. However, this restriction is not
actually enforced. Address this deficiency by making the implementation
match the documented behavior (to wit, error out if --source is not
specified when combining --worktree and --staged).

While at it, enhance the documentation to mention the --source
requirement prominently in the "Description" section (rather than only
in the description of the --source option itself).

Reported-by: Harri Mehtälä <harri.mehtala@finago.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-restore.txt | 3 ++-
 builtin/checkout.c            | 3 +++
 t/t2070-restore.sh            | 5 +++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
index 8e3b339802..8906499637 100644
--- a/Documentation/git-restore.txt
+++ b/Documentation/git-restore.txt
@@ -24,7 +24,8 @@ The command can also be used to restore the content in the index with
 
 By default, the restore sources for working tree and the index are the
 index and `HEAD` respectively. `--source` could be used to specify a
-commit as the restore source.
+commit as the restore source; it is required when combining `--staged`
+and `--worktree`.
 
 See "Reset, restore and revert" in linkgit:git[1] for the differences
 between the three commands.
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8bc94d392b..7a01d00f53 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1604,6 +1604,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 	}
 	if (opts->checkout_index < 0 || opts->checkout_worktree < 0)
 		BUG("these flags should be non-negative by now");
+	if (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&
+	    !opts->from_treeish)
+		die(_("--source required when using --worktree and --staged"));
 	/*
 	 * convenient shortcut: "git restore --staged" equals
 	 * "git restore --staged --source HEAD"
diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
index 076d0df7fc..19efa21fdb 100755
--- a/t/t2070-restore.sh
+++ b/t/t2070-restore.sh
@@ -69,6 +69,11 @@ test_expect_success 'restore --staged uses HEAD as source' '
 	test_cmp expected actual
 '
 
+test_expect_success 'restore --worktree --staged requires --source' '
+	test_must_fail git restore --worktree --staged first.t 2>err &&
+	test_i18ngrep "source required when using --worktree and --staged" err
+'
+
 test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '
 	git init unmerged &&
 	(
-- 
2.26.2.526.g744177e7f7

Re: [PATCH 1/2] restore: require --source when combining --worktree and --staged

[Eric Sunshine]

On Fri, May 1, 2020 at 4:28 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> @@ -1604,6 +1604,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>         }
>         if (opts->checkout_index < 0 || opts->checkout_worktree < 0)
>                 BUG("these flags should be non-negative by now");
> +       if (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&
> +           !opts->from_treeish)

If I re-roll the series for some reason, I'll drop the unnecessary ">
0". (But this code goes away in patch 2/2 anyhow, so I'm not too
worried about it.)

Re: [PATCH 1/2] restore: require --source when combining --worktree and --staged

[Taylor Blau]

On Fri, May 01, 2020 at 04:27:45AM -0400, Eric Sunshine wrote:
> The default restore source for --worktree is the index, and the default
> source for --staged is HEAD. When combining --worktree and --staged in
> the same invocation, the restore source is ambiguous ("should it restore
> from the index or from HEAD?"). To avoid such ambiguity, the git-restore
> documentation has always stated that --source must be used when
> combining --worktree and --staged. However, this restriction is not
> actually enforced. Address this deficiency by making the implementation
> match the documented behavior (to wit, error out if --source is not
> specified when combining --worktree and --staged).

This explanation is very helpful, and makes the fix below very natural.
Thanks for a helpful explanation.

> While at it, enhance the documentation to mention the --source
> requirement prominently in the "Description" section (rather than only
> in the description of the --source option itself).
>
> Reported-by: Harri Mehtälä <harri.mehtala@finago.com>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  Documentation/git-restore.txt | 3 ++-
>  builtin/checkout.c            | 3 +++
>  t/t2070-restore.sh            | 5 +++++
>  3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
> index 8e3b339802..8906499637 100644
> --- a/Documentation/git-restore.txt
> +++ b/Documentation/git-restore.txt
> @@ -24,7 +24,8 @@ The command can also be used to restore the content in the index with
>
>  By default, the restore sources for working tree and the index are the
>  index and `HEAD` respectively. `--source` could be used to specify a
> -commit as the restore source.
> +commit as the restore source; it is required when combining `--staged`
> +and `--worktree`.
>
>  See "Reset, restore and revert" in linkgit:git[1] for the differences
>  between the three commands.
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 8bc94d392b..7a01d00f53 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1604,6 +1604,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  	}
>  	if (opts->checkout_index < 0 || opts->checkout_worktree < 0)
>  		BUG("these flags should be non-negative by now");
> +	if (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&
> +	    !opts->from_treeish)
> +		die(_("--source required when using --worktree and --staged"));
>  	/*
>  	 * convenient shortcut: "git restore --staged" equals
>  	 * "git restore --staged --source HEAD"
> diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
> index 076d0df7fc..19efa21fdb 100755
> --- a/t/t2070-restore.sh
> +++ b/t/t2070-restore.sh
> @@ -69,6 +69,11 @@ test_expect_success 'restore --staged uses HEAD as source' '
>  	test_cmp expected actual
>  '
>
> +test_expect_success 'restore --worktree --staged requires --source' '
> +	test_must_fail git restore --worktree --staged first.t 2>err &&
> +	test_i18ngrep "source required when using --worktree and --staged" err
> +'
> +
>  test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '
>  	git init unmerged &&
>  	(
> --
> 2.26.2.526.g744177e7f7

Very sane.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

[PATCH 2/2] restore: default to HEAD when combining --worktree and --staged

[Eric Sunshine]

The default restore source for --worktree is the index, and the default
source for --staged is HEAD. However, when combining --worktree and
--staged in the same invocation, git-restore requires the source to be
specified explicitly via --source since it would otherwise be ambiguous
("should it restore from the index or from HEAD?"). This requirement
makes it cumbersome to restore a file in both the worktree and the
index.

However, HEAD is also a reasonably intuitive default restore source when
--worktree and --staged are combined. After all, if a user is asking to
throw away all local changes to a file (on disk and in the index)
without specifying a restore source explicitly -- and the user expects
the file to be restored from _somewhere_ -- then it is likely that the
user expects them to be restored from HEAD, which is an intuitive and
logical place to find a recent unadulterated copy of the file.

Therefore, make HEAD the default restore source when --worktree and
--staged are combined.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-restore.txt | 14 +++++++-------
 builtin/checkout.c            |  9 +++------
 t/t2070-restore.sh            | 12 +++++++++---
 3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
index 8906499637..5b61812e17 100644
--- a/Documentation/git-restore.txt
+++ b/Documentation/git-restore.txt
@@ -22,10 +22,10 @@ The command can also be used to restore the content in the index with
 `--staged`, or restore both the working tree and the index with
 `--staged --worktree`.
 
-By default, the restore sources for working tree and the index are the
-index and `HEAD` respectively. `--source` could be used to specify a
-commit as the restore source; it is required when combining `--staged`
-and `--worktree`.
+By default, the restore source for `--worktree` is the index, and the
+restore source for `--staged` is `HEAD`. When combining `--worktree` and
+`--staged`, the restore source is `HEAD`. `--source` can be used to specify
+a different commit as the restore source.
 
 See "Reset, restore and revert" in linkgit:git[1] for the differences
 between the three commands.
@@ -40,10 +40,10 @@ OPTIONS
 	tree. It is common to specify the source tree by naming a
 	commit, branch or tag associated with it.
 +
-If not specified, the default restore source for the working tree is
-the index, and the default restore source for the index is
+If not specified, the default restore source for `--worktree` is
+the index, and the default restore source for `--staged` is
 `HEAD`. When both `--staged` and `--worktree` are specified,
-`--source` must also be specified.
+the default restore source is `HEAD`.
 
 -p::
 --patch::
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7a01d00f53..500c3e23ff 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1604,14 +1604,11 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 	}
 	if (opts->checkout_index < 0 || opts->checkout_worktree < 0)
 		BUG("these flags should be non-negative by now");
-	if (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&
-	    !opts->from_treeish)
-		die(_("--source required when using --worktree and --staged"));
 	/*
-	 * convenient shortcut: "git restore --staged" equals
-	 * "git restore --staged --source HEAD"
+	 * convenient shortcut: "git restore --staged [--worktree]" equals
+	 * "git restore --staged [--worktree] --source HEAD"
 	 */
-	if (!opts->from_treeish && opts->checkout_index && !opts->checkout_worktree)
+	if (!opts->from_treeish && opts->checkout_index)
 		opts->from_treeish = "HEAD";
 
 	/*
diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
index 19efa21fdb..89e5a142c9 100755
--- a/t/t2070-restore.sh
+++ b/t/t2070-restore.sh
@@ -69,9 +69,15 @@ test_expect_success 'restore --staged uses HEAD as source' '
 	test_cmp expected actual
 '
 
-test_expect_success 'restore --worktree --staged requires --source' '
-	test_must_fail git restore --worktree --staged first.t 2>err &&
-	test_i18ngrep "source required when using --worktree and --staged" err
+test_expect_success 'restore --worktree --staged uses HEAD as source' '
+	test_when_finished git reset --hard &&
+	git show HEAD:./first.t >expected &&
+	echo dirty >>first.t &&
+	git add first.t &&
+	git restore --worktree --staged first.t &&
+	git show :./first.t >actual &&
+	test_cmp expected actual &&
+	test_cmp expected first.t
 '
 
 test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '
-- 
2.26.2.526.g744177e7f7

Re: [PATCH 2/2] restore: default to HEAD when combining --worktree and --staged

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> The default restore source for --worktree is the index, and the default
> source for --staged is HEAD. However, when combining --worktree and
> --staged in the same invocation, git-restore requires the source to be
> specified explicitly via --source since it would otherwise be ambiguous
> ("should it restore from the index or from HEAD?"). This requirement
> makes it cumbersome to restore a file in both the worktree and the
> index.
>
> However, HEAD is also a reasonably intuitive default restore source when
> --worktree and --staged are combined. After all, if a user is asking to
> throw away all local changes to a file (on disk and in the index)
> without specifying a restore source explicitly -- and the user expects
> the file to be restored from _somewhere_ -- then it is likely that the
> user expects them to be restored from HEAD, which is an intuitive and
> logical place to find a recent unadulterated copy of the file.
>
> Therefore, make HEAD the default restore source when --worktree and
> --staged are combined.

The mention in the second paragraph that you are dealing with the
case where you are updating both the index and the working tree is
acceptable.  It explains why HEAD is a reasonable default in that
case.  But the third paragraph is totally redundant.  

I also found that these two paragraphs a bit too long, and by the
time I finished reading them I forgot that you mentioned that HEAD
is the default when --staged is given.  It ended up giving me "ok,
you made the default to HEAD when both are given; but what about the
case when only --staged is given?" reaction X-<.

    ... This requirement makes it cumbersome to restore a file in
    both the worktree and the index.

    As we are *not* going to restore the index and the working tree
    from two different sources, we need to pick a single default
    when both options are given, and the default used for restoring
    the index, HEAD, is a reasonable one in this case, too.  Another
    plausible source might be the index, but that does not make any
    sense to the user who explicitly gave the `--staged` option.

    So, make HEAD the default source when --staged is given, whether
    --worktree is used at the same time.

perhaps?

> +By default, the restore source for `--worktree` is the index, and the
> +restore source for `--staged` is `HEAD`. When combining `--worktree` and
> +`--staged`, the restore source is `HEAD`. `--source` can be used to specify
> +a different commit as the restore source.

Clear enough, but I wonder if we can simplify it even further.

    By default, if `--staged` is given, the contents are restored
    from `HEAD`.  Otherwise, the contents are restored from the
    index.

because `--worktree` is the default for the command when neither
`--staged` or `--worktree` is given.

> @@ -40,10 +40,10 @@ OPTIONS
>  	tree. It is common to specify the source tree by naming a
>  	commit, branch or tag associated with it.
>  +
> -If not specified, the default restore source for the working tree is
> -the index, and the default restore source for the index is
> +If not specified, the default restore source for `--worktree` is
> +the index, and the default restore source for `--staged` is

Likewise.

>  `HEAD`. When both `--staged` and `--worktree` are specified,
> -`--source` must also be specified.
> +the default restore source is `HEAD`.
>  
>  -p::
>  --patch::
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 7a01d00f53..500c3e23ff 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1604,14 +1604,11 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  	}
>  	if (opts->checkout_index < 0 || opts->checkout_worktree < 0)
>  		BUG("these flags should be non-negative by now");
> -	if (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&
> -	    !opts->from_treeish)
> -		die(_("--source required when using --worktree and --staged"));
>  	/*
> -	 * convenient shortcut: "git restore --staged" equals
> -	 * "git restore --staged --source HEAD"
> +	 * convenient shortcut: "git restore --staged [--worktree]" equals
> +	 * "git restore --staged [--worktree] --source HEAD"
>  	 */

Good.


> -	if (!opts->from_treeish && opts->checkout_index && !opts->checkout_worktree)
> +	if (!opts->from_treeish && opts->checkout_index)
>  		opts->from_treeish = "HEAD";

This succinctly tells the gist of this change.  

When source is not given, the default is HEAD when we are updating
the index, regardless of any other condition.

Good.

> diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
> index 19efa21fdb..89e5a142c9 100755
> --- a/t/t2070-restore.sh
> +++ b/t/t2070-restore.sh
> @@ -69,9 +69,15 @@ test_expect_success 'restore --staged uses HEAD as source' '
>  	test_cmp expected actual
>  '
>  
> -test_expect_success 'restore --worktree --staged requires --source' '
> -	test_must_fail git restore --worktree --staged first.t 2>err &&
> -	test_i18ngrep "source required when using --worktree and --staged" err
> +test_expect_success 'restore --worktree --staged uses HEAD as source' '
> +	test_when_finished git reset --hard &&
> +	git show HEAD:./first.t >expected &&
> +	echo dirty >>first.t &&
> +	git add first.t &&
> +	git restore --worktree --staged first.t &&
> +	git show :./first.t >actual &&
> +	test_cmp expected actual &&
> +	test_cmp expected first.t
>  '

Quite straight-forward and makes sense.

Re: [PATCH 2/2] restore: default to HEAD when combining --worktree and --staged

[Eric Sunshine]

On Fri, May 1, 2020 at 11:33 AM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > [...] This requirement
> > makes it cumbersome to restore a file in both the worktree and the
> > index.
> >
> > However, HEAD is also a reasonably intuitive default restore source when
> > --worktree and --staged are combined. After all, if a user is asking to
> > throw away all local changes to a file (on disk and in the index)
> > without specifying a restore source explicitly -- and the user expects
> > the file to be restored from _somewhere_ -- then it is likely that the
> > user expects them to be restored from HEAD, which is an intuitive and
> > logical place to find a recent unadulterated copy of the file.
>
> I also found that these two paragraphs a bit too long, and by the
> time I finished reading them I forgot that you mentioned that HEAD
> is the default when --staged is given. [...]
>
>   ... This requirement makes it cumbersome to restore a file in
>   both the worktree and the index.
>
>   As we are *not* going to restore the index and the working tree
>   from two different sources, we need to pick a single default
>   when both options are given, and the default used for restoring
>   the index, HEAD, is a reasonable one in this case, too. Another
>   plausible source might be the index, but that does not make any
>   sense to the user who explicitly gave the `--staged` option.
>
>   So, make HEAD the default source when --staged is given, whether
>   --worktree is used at the same time.

Thanks. I was having trouble writing it without being overly
subjective (and using words like "intuitive" and "logical"). I'll
probably drop the "Another plausible..." bit from the rewrite, though.

> > +By default, the restore source for `--worktree` is the index, and the
> > +restore source for `--staged` is `HEAD`. When combining `--worktree` and
> > +`--staged`, the restore source is `HEAD`. `--source` can be used to specify
> > +a different commit as the restore source.
>
> Clear enough, but I wonder if we can simplify it even further.
>
>   By default, if `--staged` is given, the contents are restored
>   from `HEAD`. Otherwise, the contents are restored from the
>   index.
>
> because `--worktree` is the default for the command when neither
> `--staged` or `--worktree` is given.

Makes sense. I wasn't terribly happy with it either.

Re: [PATCH 2/2] restore: default to HEAD when combining --worktree and --staged

[Taylor Blau]

On Fri, May 01, 2020 at 04:27:46AM -0400, Eric Sunshine wrote:
> The default restore source for --worktree is the index, and the default
> source for --staged is HEAD. However, when combining --worktree and

I think that you could very reasonably drop the first sentence here,
especially because it is repeated verbatim from the previous commit.

In fact... this whole paragraph looks similar to me. Maybe just:

  When invoking 'git restore' with both '--worktree' and '--staged', it
  is required that the ambiguity of which source to restore from be
  resolved by also passing '--source'.

> --staged in the same invocation, git-restore requires the source to be
> specified explicitly via --source since it would otherwise be ambiguous
> ("should it restore from the index or from HEAD?"). This requirement
> makes it cumbersome to restore a file in both the worktree and the
> index.
>
> However, HEAD is also a reasonably intuitive default restore source when
> --worktree and --staged are combined. After all, if a user is asking to
> throw away all local changes to a file (on disk and in the index)
> without specifying a restore source explicitly -- and the user expects
> the file to be restored from _somewhere_ -- then it is likely that the
> user expects them to be restored from HEAD, which is an intuitive and
> logical place to find a recent unadulterated copy of the file.
>
> Therefore, make HEAD the default restore source when --worktree and
> --staged are combined.

I think that this is a very sensible default choice here, so I am OK
with this second patch, too.

> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  Documentation/git-restore.txt | 14 +++++++-------
>  builtin/checkout.c            |  9 +++------
>  t/t2070-restore.sh            | 12 +++++++++---
>  3 files changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
> index 8906499637..5b61812e17 100644
> --- a/Documentation/git-restore.txt
> +++ b/Documentation/git-restore.txt
> @@ -22,10 +22,10 @@ The command can also be used to restore the content in the index with
>  `--staged`, or restore both the working tree and the index with
>  `--staged --worktree`.
>
> -By default, the restore sources for working tree and the index are the
> -index and `HEAD` respectively. `--source` could be used to specify a
> -commit as the restore source; it is required when combining `--staged`
> -and `--worktree`.
> +By default, the restore source for `--worktree` is the index, and the
> +restore source for `--staged` is `HEAD`. When combining `--worktree` and
> +`--staged`, the restore source is `HEAD`. `--source` can be used to specify

This is extremely nit-pick-y, but is this line a little over-long? My
memory is that Documentation should be wrapped at 72 characters instead
of 80. I culd be totally wrong.

> +a different commit as the restore source.
>
>  See "Reset, restore and revert" in linkgit:git[1] for the differences
>  between the three commands.
> @@ -40,10 +40,10 @@ OPTIONS
>  	tree. It is common to specify the source tree by naming a
>  	commit, branch or tag associated with it.
>  +
> -If not specified, the default restore source for the working tree is
> -the index, and the default restore source for the index is
> +If not specified, the default restore source for `--worktree` is
> +the index, and the default restore source for `--staged` is
>  `HEAD`. When both `--staged` and `--worktree` are specified,
> -`--source` must also be specified.
> +the default restore source is `HEAD`.
>
>  -p::
>  --patch::
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 7a01d00f53..500c3e23ff 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1604,14 +1604,11 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  	}
>  	if (opts->checkout_index < 0 || opts->checkout_worktree < 0)
>  		BUG("these flags should be non-negative by now");
> -	if (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&
> -	    !opts->from_treeish)
> -		die(_("--source required when using --worktree and --staged"));
>  	/*
> -	 * convenient shortcut: "git restore --staged" equals
> -	 * "git restore --staged --source HEAD"
> +	 * convenient shortcut: "git restore --staged [--worktree]" equals
> +	 * "git restore --staged [--worktree] --source HEAD"
>  	 */
> -	if (!opts->from_treeish && opts->checkout_index && !opts->checkout_worktree)
> +	if (!opts->from_treeish && opts->checkout_index)
>  		opts->from_treeish = "HEAD";
>
>  	/*
> diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
> index 19efa21fdb..89e5a142c9 100755
> --- a/t/t2070-restore.sh
> +++ b/t/t2070-restore.sh
> @@ -69,9 +69,15 @@ test_expect_success 'restore --staged uses HEAD as source' '
>  	test_cmp expected actual
>  '
>
> -test_expect_success 'restore --worktree --staged requires --source' '
> -	test_must_fail git restore --worktree --staged first.t 2>err &&
> -	test_i18ngrep "source required when using --worktree and --staged" err
> +test_expect_success 'restore --worktree --staged uses HEAD as source' '
> +	test_when_finished git reset --hard &&
> +	git show HEAD:./first.t >expected &&
> +	echo dirty >>first.t &&
> +	git add first.t &&
> +	git restore --worktree --staged first.t &&
> +	git show :./first.t >actual &&
> +	test_cmp expected actual &&
> +	test_cmp expected first.t
>  '
>
>  test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '
> --
> 2.26.2.526.g744177e7f7

All looks good to me, here, too.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

Re: [PATCH 2/2] restore: default to HEAD when combining --worktree and --staged

[Eric Sunshine]

On Fri, May 1, 2020 at 6:20 PM Taylor Blau <me@ttaylorr.com> wrote:
> On Fri, May 01, 2020 at 04:27:46AM -0400, Eric Sunshine wrote:
> > The default restore source for --worktree is the index, and the default
> > source for --staged is HEAD. However, when combining --worktree and
>
> I think that you could very reasonably drop the first sentence here,
> especially because it is repeated verbatim from the previous commit.

The repetition is intentional so that each commit can be understood
stand-alone (without having to know what came before it).

> In fact... this whole paragraph looks similar to me. Maybe just:
>
>  When invoking 'git restore' with both '--worktree' and '--staged', it
>  is required that the ambiguity of which source to restore from be
>  resolved by also passing '--source'.

I'll see if I can trim it down a bit -- Junio also found it too long.

> > -By default, the restore sources for working tree and the index are the
> > -index and `HEAD` respectively. `--source` could be used to specify a
> > -commit as the restore source; it is required when combining `--staged`
> > -and `--worktree`.
> > +By default, the restore source for `--worktree` is the index, and the
> > +restore source for `--staged` is `HEAD`. When combining `--worktree` and
> > +`--staged`, the restore source is `HEAD`. `--source` can be used to specify
>
> This is extremely nit-pick-y, but is this line a little over-long? My
> memory is that Documentation should be wrapped at 72 characters instead
> of 80. I culd be totally wrong.

Column 72 for commit messages, certainly, but I don't think there is
any such guideline about documentation also being wrapped at 72. As an
old-schooler who still uses 80-column terminal and editor windows, I'm
quite sensitive to line length -- these lines are wrapped at 79.

Re: [PATCH 2/2] restore: default to HEAD when combining --worktree and --staged

[Taylor Blau]

On Tue, May 05, 2020 at 12:00:08AM -0400, Eric Sunshine wrote:
> On Fri, May 1, 2020 at 6:20 PM Taylor Blau <me@ttaylorr.com> wrote:
> > On Fri, May 01, 2020 at 04:27:46AM -0400, Eric Sunshine wrote:
> > > The default restore source for --worktree is the index, and the default
> > > source for --staged is HEAD. However, when combining --worktree and
> >
> > I think that you could very reasonably drop the first sentence here,
> > especially because it is repeated verbatim from the previous commit.
>
> The repetition is intentional so that each commit can be understood
> stand-alone (without having to know what came before it).

Fair enough; I figure that the commits will probably be read most often
in conjunction with one another, but a little bit of extra commentary
doesn't hurt, either.

> > In fact... this whole paragraph looks similar to me. Maybe just:
> >
> >  When invoking 'git restore' with both '--worktree' and '--staged', it
> >  is required that the ambiguity of which source to restore from be
> >  resolved by also passing '--source'.
>
> I'll see if I can trim it down a bit -- Junio also found it too long.

Thanks. I'm happy to read whatever you have once you're ready.

> > > -By default, the restore sources for working tree and the index are the
> > > -index and `HEAD` respectively. `--source` could be used to specify a
> > > -commit as the restore source; it is required when combining `--staged`
> > > -and `--worktree`.
> > > +By default, the restore source for `--worktree` is the index, and the
> > > +restore source for `--staged` is `HEAD`. When combining `--worktree` and
> > > +`--staged`, the restore source is `HEAD`. `--source` can be used to specify
> >
> > This is extremely nit-pick-y, but is this line a little over-long? My
> > memory is that Documentation should be wrapped at 72 characters instead
> > of 80. I culd be totally wrong.
>
> Column 72 for commit messages, certainly, but I don't think there is
> any such guideline about documentation also being wrapped at 72. As an
> old-schooler who still uses 80-column terminal and editor windows, I'm
> quite sensitive to line length -- these lines are wrapped at 79.

Hmm... I've always wrapped changes in the Documentation tree at 72
characters, but I could very easily be in the wrong there ;). I tried to
find something in Documentation about wrapping at 72 characters, but I
failed. So, please disregard my original suggestion, and sorry for the
trouble there.


Thanks,
Taylor