Re: [PATCH 2/2] restore: default to HEAD when combining --worktree and --staged
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 1, 2020, 15:32 UTC
- Message-ID
- <xmqqimhfoelf.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20200501082746.23943-3-sunshine@sunshineco.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
Show 18 quoted lines
> 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.
Show 8 quoted lines
> @@ -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.
Show 23 quoted lines
> `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.
Show 21 quoted lines
> 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.