"Hugo Sales via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> Subject: Re: [PATCH] Add `restore.defaultLocation` option
As to the form, perhaps
Subject: [PATCH] restore: make it configurable what gets updated
cf. Documentation/SubmittingPatches[[describe-changes]]
Also on substance,
* "option" usually refers to the --option given on the command
line; when you mean "configuration variable", it is a misleading
word to use.
* "restore" command deals with two quite conceptually different
locations. Where it gets the contents from (i.e. the source
location), and where the contents are used to update (i.e. the
destination location). The ".defaultLocation" is a poor name for
a configuration variable because it does not convey which one is
meant.
> From: Hugo Sales <hugo@xxxxxxx>
>
> This options allows control over which of `--worktree` or `--staged` is
> applied when `git restore` is invoked with neither
>
> This patch is intended to reduce lost work to accidental `git restore .`
> when `git restore --staged .` was intended.
We usually describe the status quo (what the current system does),
what bad things can happen with the current system, and then what
change is proposed to improve the situation, in this order. So the
above is backwards. Perhaps like
"git restore" takes "--worktree" and/or "--staged" options to
specify which one of the working tree files and/or the index
entries are updated. With neither options, the command by
default updates the working tree files.
A user who wanted to reset the index entries from HEAD may by
mistake run "git restore" without the "--staged" option. When
such a mistake happens, the work made in the working tree files
that are not yet added to the index will be forever lost.
Introduce the restore.defaultDestination configuration variable,
which can be set to one of "both", "index", or "worktree", to
help those users who want to set it to "index" to avoid touching
the working tree files by mistake. They now force themselves to
use the "--worktree" option explicitly when they want to restore
the working tree files.
or something along that line would make it more like our log messages.
Note that even though I wrote the above by guessing what your
motivation behind this change is, I do not very much agree with the
third paragraph myself. I think a better solution would be to teach
these users to use "git reset -- <path>" when they want to reset the
index entries, and not run the "git restore" command.
> + This
> + option can be used to prevent accidentally losing work by running `git
> + restore .` when `git restore --staged .` was intended.
This is better left out, as it cuts both ways. With it set to
'index', this option will clobber the index entries the user
carefully prepared so far with "git add -p" and friends, when the
user wanted to update the working tree files (either from the index
or from an existing commit) by running "git restore", which will
lose work.
If we wanted to advertise it as a feature, then the sentence should
say something like
This variable can be set to 'index' to prevent accidentally
losing work ...
"can be used" is too vague when you are talking about setting it to
a particular value. IOW, setting it to any other value would *NOT*
prevent "git restore" from behaving differently from "git resetore --staged".
Again, I do not think it is a good idea to sell it as a feature in
the first place, as it cuts both ways.
> diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
> index 5964810caa4..28165861f55 100644
> --- a/Documentation/git-restore.txt
> +++ b/Documentation/git-restore.txt
> @@ -14,14 +14,18 @@ SYNOPSIS
>
> DESCRIPTION
> -----------
> -Restore specified paths in the working tree with some contents from a
> +Restore specified paths in the working tree or index with some contents from a
Shouldn't it be "and/or"?
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index a5155cf55c1..5067753030b 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1942,6 +1966,7 @@ int cmd_restore(int argc, const char **argv, const char *prefix)
> struct branch_info new_branch_info = { 0 };
>
> memset(&opts, 0, sizeof(opts));
> +
> opts.accept_ref = 0;
> opts.accept_pathspec = 1;
> opts.empty_pathspec_ok = 0;
Why?
> diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
> index 7c43ddf1d99..6e9b06e0bf4 100755
> --- a/t/t2070-restore.sh
> +++ b/t/t2070-restore.sh
> @@ -137,4 +137,128 @@ test_expect_success 'restore --staged invalidates cache tree for deletions' '
> test_must_fail git rev-parse HEAD:new1
> '
>
> +test_expect_success 'restore with restore.defaultLocation unset works as if --worktree given' '
> + test_when_finished git reset --hard HEAD^ &&
> + test_commit root-unset-restore.defaultLocation &&
> + test_commit unset-restore.defaultLocation one one &&
> + > one &&
Lose SP between ">" and "one", its redirection target.
cf. Documentation/CodingGuidelines, look for "Redirection operators
should be written with space before, but no space after them." and
study the entire paragraph.
> + git restore one &&
> + test -z $(git status --porcelain --untracked-files=no) &&
This (together with many other uses of "git status" in the new
tests) will not catch a segfaulting "git status".
Thanks.
