| Message ID | pull.1232.v3.git.1687219414844.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] Introduced force flag to the git stash clear subcommand. | expand |
"Nadav Goldstein via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Nadav Goldstein <nadav.goldstein96@gmail.com> > > stash clean subcommand now support the force flag, along > with the configuration var stash.requireforce, that if > set to true, will make git stash clear fail unless supplied > with force flag. Documentation/SubmittingPatches gives many helpful hints on how to write log messages for the project. > @@ -208,6 +208,14 @@ to learn how to operate the `--patch` mode. > The `--patch` option implies `--keep-index`. You can use > `--no-keep-index` to override this. > > +-f:: > +--force:: > + This option is only valid for `clear` command Missing full-stop? > ++ > +If the Git configuration variable stash.requireForce is set Drop "Git" perhaps? I haven't seen any other place that says "Git configuration variable X" when talking about a single variable (it probably is OK to call those defined in a Git configuration file collectively as "Git configuration variables", though). > +to true, 'git stash clear' will refuse to remove all the stash > +entries unless given -f. I am not sure how much value users would get by requiring "--force", though. I know this was (partly) modeled after "git clean", but over there, when the required "--force" is not given, the user would give "--dry-run" (or "-n"), and the user will see what would be removed if the user gave "--force". If missing "--force" made "git stash clear" show the stash entries that would be lost, then after seeing an error message, it would be easier for the user to decide if their next move should be to re-run the command with "--force", or there are some precious entries and the user is not ready to do "stash clear". But just refusing to run without giving any other information will just train the user to give "git stash clear --force" without thinking, because getting "because you did not give the required --force option, I am not doing anything" is only annoying without giving any useful information. > diff --git a/builtin/stash.c b/builtin/stash.c > index a7e17ffe384..d037bc4f69c 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -53,7 +53,7 @@ > #define BUILTIN_STASH_CREATE_USAGE \ > N_("git stash create [<message>]") > #define BUILTIN_STASH_CLEAR_USAGE \ > - "git stash clear" > + "git stash clear [-f | --force]" > > static const char * const git_stash_usage[] = { > BUILTIN_STASH_LIST_USAGE, > @@ -122,6 +122,7 @@ static const char * const git_stash_save_usage[] = { > > static const char ref_stash[] = "refs/stash"; > static struct strbuf stash_index_path = STRBUF_INIT; > +static int clear_require_force = 0; Do not explicitly initialize globals to 0 or NULL; let BSS take care of the zero initialization, instead. > @@ -246,7 +247,9 @@ static int do_clear_stash(void) > > static int clear_stash(int argc, const char **argv, const char *prefix) > { > + int force = 0; > struct option options[] = { > + OPT__FORCE(&force, N_("force"), PARSE_OPT_NOCOMPLETE), > OPT_END() > }; As this topic focuses on "git stash clear", this is OK (and the description in the documentation that says that "force" is currently supported only by "clear" is also fine), but is "clear" the only destructive subcommand and no other subcommand will want to learn the "--force" for similar safety in the future? The answer to this question matters because ... > @@ -258,6 +261,9 @@ static int clear_stash(int argc, const char **argv, const char *prefix) > return error(_("git stash clear with arguments is " > "unimplemented")); > > + if (!force && clear_require_force) > + return error(_("fatal: stash.requireForce set to true and -f was not given; refusing to clear stash")); > + > return do_clear_stash(); > } > > @@ -851,6 +857,10 @@ static int git_stash_config(const char *var, const char *value, void *cb) > show_include_untracked = git_config_bool(var, value); > return 0; > } > + if (!strcmp(var, "stash.requireforce")) { ... the naming of this variable, facing the end users, does not limit itself to "clear" at all. It gives an impression that setting this will require "--force" for all other subcommands that would support it. However ... > + clear_require_force = git_config_bool(var, value); ... inside the code, the variable is named in such a way that it is only about the "clear" subcommand and nothing else. I suspect that the end-user facing "stash.requireforce" should be renamed to make it clear that it is about "stash clear" subcommand, and not everywhere in "stash" requires "--force". Nobody wants to keep saying "git stash save --force" ;-) > + return 0; > + } Thanks.
> I am not sure how much value users would get by requiring "--force", > though. I know this was (partly) modeled after "git clean", but > over there, when the required "--force" is not given, the user would > give "--dry-run" (or "-n"), and the user will see what would be > removed if the user gave "--force". If missing "--force" made "git > stash clear" show the stash entries that would be lost, then after > seeing an error message, it would be easier for the user to decide > if their next move should be to re-run the command with "--force", > or there are some precious entries and the user is not ready to do > "stash clear". > > But just refusing to run without giving any other information will > just train the user to give "git stash clear --force" without > thinking, because getting "because you did not give the required > --force option, I am not doing anything" is only annoying without > giving any useful information. I see, but isn't the same argument apply for git clean? if not adding the force flag, the same message as I wrote appear in git clean (I copied it from there), and it will exit without any other information, hence given your argument, running git clean is also not very useful. One can argue that git clean --dry-run == git stash list I suggested in the beginning of this thread to ask the user if he is sure he want to proceed (default to no), and only if he wrote y/yes proceed with the action (and force will just do it, or requireforce=false). The reason I suggested it is because when running git stash clear, it will remain in the user recent commands, and when the user will navigate through the commands history in the terminal, he might accidentally fire git stash clear, and this confirmation will be another safeguard against this mistake. Maybe it will be useful for git clean as well for the same reasons. Also when the user types git clean, I argue he wanted to clean or he did it by mistake, and In both scenarios I don't see why making git clean just fail will be useful. So what do you think? Maybe we should present in both clean and clear a confirmation message? (only if requireforce=true) What do you think?
Nadav Goldstein <nadav.goldstein96@gmail.com> writes: > I see, but isn't the same argument apply for git clean? if not adding > the force flag, the same message as I wrote appear in git clean (I > copied it from there), and it will exit without any other information, > hence given your argument, running git clean is also not very useful. The thing is that "git clean" by default forces people to choose between "-f" and "-n" to force people to understand the issue. And once they understand the issue, they'd learn to run "clean -n" first, which lets them see what would be removed, before they run "clean -f". Does your "stash clear" work the same way? I do not think so. If there is "stash clear --dry-run" that runs "stash list", it might be similar, but not similar enough. I wonder if "stash clear", when stashClear.requireForce is set to true and unless "--force" is given, should do "stash list" and then error out. I dunno.
On Tue, Jun 20, 2023 at 4:05 PM Nadav Goldstein <nadav.goldstein96@gmail.com> wrote: > > I am not sure how much value users would get by requiring "--force", > > though. I know this was (partly) modeled after "git clean", but > > over there, when the required "--force" is not given, the user would > > give "--dry-run" (or "-n"), and the user will see what would be > > removed if the user gave "--force". If missing "--force" made "git > > stash clear" show the stash entries that would be lost, then after > > seeing an error message, it would be easier for the user to decide > > if their next move should be to re-run the command with "--force", > > or there are some precious entries and the user is not ready to do > > "stash clear". > > > > But just refusing to run without giving any other information will > > just train the user to give "git stash clear --force" without > > thinking, because getting "because you did not give the required > > --force option, I am not doing anything" is only annoying without > > giving any useful information. > > I see, but isn't the same argument apply for git clean? if not adding > the force flag, the same message as I wrote appear in git clean (I > copied it from there), and it will exit without any other information, > hence given your argument, running git clean is also not very useful. For what it's worth, I had the same reaction as Junio upon reading this patch; specifically, that it will train users to type "git stash clear --force" mechanically without thinking, thus won't be much of a safeguard. > I suggested in the beginning of this thread to ask the user if he is > sure he want to proceed (default to no), and only if he wrote y/yes > proceed with the action (and force will just do it, or requireforce=false). > > The reason I suggested it is because when running git stash clear, it > will remain in the user recent commands, and when the user will navigate > through the commands history in the terminal, he might accidentally fire > git stash clear, and this confirmation will be another safeguard against > this mistake. > > Maybe it will be useful for git clean as well for the same reasons. > Also when the user types git clean, I argue he wanted to clean or he did > it by mistake, and In both scenarios I don't see why making git clean > just fail will be useful. "git clean" is in a rather different (and more severe) boat since file deletion is irrevocable, whereas a stash thrown away by "git stash clear" (or "git stash drop") can be recovered (at least until it gets garbage-collected). So, rather than adding a --force option or an interactive "yes/no" prompt, perhaps a better approach would be to have "git stash clear" (and "git stash drop") print out advice explaining to the user how to recover the dropped stash(es), much like "git switch" or "git checkout" prints advice explaining how to recover commits left dangling on a detached head.
I see both of your points, and I agree adding the flag will only make users type the flag without thinking. But I still don't understand why do we need git clean without any flags. The only users that will run git clean are new users that don't know you need to run it with -f or experienced users that set clean.requireforce = false. Moreover, we can also assume that every user that have clean.requireforce = true (the default), and ran git clean, did so by mistake/intended to clean, So why don't we offer him interactive way to understand the consequences and confirm his action? (and explain about clean.requireforce like we currently do). By doing it this way, the change will not effect experienced users because they either run git clean -f when they need to clean or they set clean.requireforce = false, and then the change will not apply to them. This argument is also for stash clear. Thanks. On 21/06/2023 0:01, Eric Sunshine wrote: > On Tue, Jun 20, 2023 at 4:05 PM Nadav Goldstein > <nadav.goldstein96@gmail.com> wrote: >>> I am not sure how much value users would get by requiring "--force", >>> though. I know this was (partly) modeled after "git clean", but >>> over there, when the required "--force" is not given, the user would >>> give "--dry-run" (or "-n"), and the user will see what would be >>> removed if the user gave "--force". If missing "--force" made "git >>> stash clear" show the stash entries that would be lost, then after >>> seeing an error message, it would be easier for the user to decide >>> if their next move should be to re-run the command with "--force", >>> or there are some precious entries and the user is not ready to do >>> "stash clear". >>> >>> But just refusing to run without giving any other information will >>> just train the user to give "git stash clear --force" without >>> thinking, because getting "because you did not give the required >>> --force option, I am not doing anything" is only annoying without >>> giving any useful information. >> I see, but isn't the same argument apply for git clean? if not adding >> the force flag, the same message as I wrote appear in git clean (I >> copied it from there), and it will exit without any other information, >> hence given your argument, running git clean is also not very useful. > For what it's worth, I had the same reaction as Junio upon reading > this patch; specifically, that it will train users to type "git stash > clear --force" mechanically without thinking, thus won't be much of a > safeguard. > >> I suggested in the beginning of this thread to ask the user if he is >> sure he want to proceed (default to no), and only if he wrote y/yes >> proceed with the action (and force will just do it, or requireforce=false). >> >> The reason I suggested it is because when running git stash clear, it >> will remain in the user recent commands, and when the user will navigate >> through the commands history in the terminal, he might accidentally fire >> git stash clear, and this confirmation will be another safeguard against >> this mistake. >> >> Maybe it will be useful for git clean as well for the same reasons. >> Also when the user types git clean, I argue he wanted to clean or he did >> it by mistake, and In both scenarios I don't see why making git clean >> just fail will be useful. > "git clean" is in a rather different (and more severe) boat since file > deletion is irrevocable, whereas a stash thrown away by "git stash > clear" (or "git stash drop") can be recovered (at least until it gets > garbage-collected). So, rather than adding a --force option or an > interactive "yes/no" prompt, perhaps a better approach would be to > have "git stash clear" (and "git stash drop") print out advice > explaining to the user how to recover the dropped stash(es), much like > "git switch" or "git checkout" prints advice explaining how to recover > commits left dangling on a detached head. > >
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt index f4bb6114d91..e95410d507e 100644 --- a/Documentation/git-stash.txt +++ b/Documentation/git-stash.txt @@ -20,7 +20,7 @@ SYNOPSIS [--] [<pathspec>...]] 'git stash' save [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet] [-u | --include-untracked] [-a | --all] [<message>] -'git stash' clear +'git stash' clear [-f | --force] 'git stash' create [<message>] 'git stash' store [(-m | --message) <message>] [-q | --quiet] <commit> @@ -130,7 +130,7 @@ the stash entry is applied on top of the commit that was HEAD at the time `git stash` was run, it restores the originally stashed state with no conflicts. -clear:: +clear [-f|--force]:: Remove all the stash entries. Note that those entries will then be subject to pruning, and may be impossible to recover (see 'Examples' below for a possible strategy). @@ -208,6 +208,14 @@ to learn how to operate the `--patch` mode. The `--patch` option implies `--keep-index`. You can use `--no-keep-index` to override this. +-f:: +--force:: + This option is only valid for `clear` command ++ +If the Git configuration variable stash.requireForce is set +to true, 'git stash clear' will refuse to remove all the stash +entries unless given -f. + -S:: --staged:: This option is only valid for `push` and `save` commands. diff --git a/builtin/stash.c b/builtin/stash.c index a7e17ffe384..d037bc4f69c 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -53,7 +53,7 @@ #define BUILTIN_STASH_CREATE_USAGE \ N_("git stash create [<message>]") #define BUILTIN_STASH_CLEAR_USAGE \ - "git stash clear" + "git stash clear [-f | --force]" static const char * const git_stash_usage[] = { BUILTIN_STASH_LIST_USAGE, @@ -122,6 +122,7 @@ static const char * const git_stash_save_usage[] = { static const char ref_stash[] = "refs/stash"; static struct strbuf stash_index_path = STRBUF_INIT; +static int clear_require_force = 0; /* * w_commit is set to the commit containing the working tree @@ -246,7 +247,9 @@ static int do_clear_stash(void) static int clear_stash(int argc, const char **argv, const char *prefix) { + int force = 0; struct option options[] = { + OPT__FORCE(&force, N_("force"), PARSE_OPT_NOCOMPLETE), OPT_END() }; @@ -258,6 +261,9 @@ static int clear_stash(int argc, const char **argv, const char *prefix) return error(_("git stash clear with arguments is " "unimplemented")); + if (!force && clear_require_force) + return error(_("fatal: stash.requireForce set to true and -f was not given; refusing to clear stash")); + return do_clear_stash(); } @@ -851,6 +857,10 @@ static int git_stash_config(const char *var, const char *value, void *cb) show_include_untracked = git_config_bool(var, value); return 0; } + if (!strcmp(var, "stash.requireforce")) { + clear_require_force = git_config_bool(var, value); + return 0; + } return git_diff_basic_config(var, value, cb); }