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

[Taylor Blau <me@ttaylorr.com>]

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