| Message ID | 20240218033146.372727-2-rpc01234@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | builtin/stash: configs keepIndex, includeUntracked | expand |
Hi Ricardo On 18/02/2024 03:30, MithicSpirit wrote: > Adds options `stash.keepIndex` and `stash.includeUntracked`, which > enable `--keep-index` and `--include-untracked` by default (unless it > would conflict with another option). This is done to facilitate the > workflow where a user: > > . Makes modifications; > . Selectively adds them with `git add -p`; and > . Stashes the unadded changes to run tests with only the current > modifications. > > `stash.keepIndex` is especially important for this, as otherwise the > work done in `git add -p` is lost since applying the stash does not > restore the index. How does "stash.keepIndex" interact with "git rebase --autostash" and "git merge --autostash"? I think both those commands expect a clean index after running "git stash". They could just override the config setting but it might get a bit confusing if some commands respect the config and others don't. > This problem could also be solved by adding > functionality to the stash to restore the index specifically, but this > would create much more complexity and still wouldn't be as convenient > for that workflow. > > Signed-off-by: Ricardo Prado Cunha (MithicSpirit) <rpc01234@gmail.com> > --- > This is my first patch to git and my first time using mailing lists for this > kind of stuff. Please do let me know of any mistakes or gaffes I've made. I've only given the patch a very quick scan, but it looked sensible. The only thing that jumped out at me was that quite a few tests seem to do git init repo && ( cd repo && # test things ) && Our normal practice is to run all the tests in the same file in the same repository rather than setting up a new one each time. Best Wishes Phillip > Documentation/config/stash.txt | 13 ++++ > builtin/stash.c | 65 ++++++++++++------ > t/t3903-stash.sh | 119 +++++++++++++++++++++++++++++++++ > 3 files changed, 178 insertions(+), 19 deletions(-) > > diff --git a/Documentation/config/stash.txt b/Documentation/config/stash.txt > index ec1edaeba6..d6d9ea7daa 100644 > --- a/Documentation/config/stash.txt > +++ b/Documentation/config/stash.txt > @@ -1,6 +1,19 @@ > +stash.includeUntracked:: > + Boolean option that configures whether the `git stash push` and `git > + stash save` commands also stash untracked files by default. This option > + is ignored if `--include-untracked`, `--no-include-untracked`, `--all`, > + `--patch`, or `--staged` are used. Defaults to `false`. See > + linkgit:git-stash[1]. > + > +stash.keepIndex:: > + Boolean option that configures whether the `git stash push` and `git > + stash save` commands also stash changes that have been added to the > + index. This option is ignored if `--keep-index`, `--no-keep-index`, or > + `--patch` are used. Defaults to `false`. See linkgit:git-stash[1]. > + > stash.showIncludeUntracked:: > If this is set to true, the `git stash show` command will show > the untracked files of a stash entry. Defaults to false. See > the description of the 'show' command in linkgit:git-stash[1]. > > stash.showPatch:: > diff --git a/builtin/stash.c b/builtin/stash.c > index 7fb355bff0..c591a2bf4b 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -836,12 +836,14 @@ static int list_stash(int argc, const char **argv, const char *prefix) > return run_command(&cp); > } > > static int show_stat = 1; > static int show_patch; > static int show_include_untracked; > +static int default_keep_index; > +static int default_include_untracked; > > static int git_stash_config(const char *var, const char *value, > const struct config_context *ctx, void *cb) > { > if (!strcmp(var, "stash.showstat")) { > show_stat = git_config_bool(var, value); > @@ -852,12 +854,20 @@ static int git_stash_config(const char *var, const char *value, > return 0; > } > if (!strcmp(var, "stash.showincludeuntracked")) { > show_include_untracked = git_config_bool(var, value); > return 0; > } > + if (!strcmp(var, "stash.keepindex")) { > + default_keep_index = git_config_bool(var, value); > + return 0; > + } > + if (!strcmp(var, "stash.includeuntracked")) { > + default_include_untracked = git_config_bool(var, value); > + return 0; > + } > return git_diff_basic_config(var, value, ctx, cb); > } > > static void diff_include_untracked(const struct stash_info *info, struct diff_options *diff_opt) > { > const struct object_id *oid[] = { &info->w_commit, &info->u_tree }; > @@ -1509,33 +1519,50 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q > int ret = 0; > struct stash_info info = STASH_INFO_INIT; > struct strbuf patch = STRBUF_INIT; > struct strbuf stash_msg_buf = STRBUF_INIT; > struct strbuf untracked_files = STRBUF_INIT; > > - if (patch_mode && keep_index == -1) > - keep_index = 1; > - > - if (patch_mode && include_untracked) { > - fprintf_ln(stderr, _("Can't use --patch and --include-untracked" > - " or --all at the same time")); > - ret = -1; > - goto done; > + if (keep_index == -1) { > + if (patch_mode) > + keep_index = 1; > + else > + keep_index = default_keep_index; > } > > - /* --patch overrides --staged */ > - if (patch_mode) > + if (patch_mode) { > + if (include_untracked == -1) > + include_untracked = 0; > + else if (include_untracked) { > + fprintf_ln(stderr, > + _("Can't use --patch and --include-untracked" > + " or --all at the same time")); > + ret = -1; > + goto done; > + } > + > + /* --patch overrides --staged */ > only_staged = 0; > - > - if (only_staged && include_untracked) { > - fprintf_ln(stderr, _("Can't use --staged and --include-untracked" > - " or --all at the same time")); > - ret = -1; > - goto done; > } > > + if (only_staged) { > + if (include_untracked == -1) > + include_untracked = 0; > + else if (include_untracked) { > + fprintf_ln( > + stderr, > + _("Can't use --staged and --include-untracked" > + " or --all at the same time")); > + ret = -1; > + goto done; > + } > + } > + > + if (include_untracked == -1) > + include_untracked = default_include_untracked; > + > repo_read_index_preload(the_repository, NULL, 0); > if (!include_untracked && ps->nr) { > int i; > char *ps_matched = xcalloc(ps->nr, 1); > > /* TODO: audit for interaction with sparse-index. */ > @@ -1688,13 +1715,13 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q > fprintf_ln(stderr, _("Cannot remove " > "worktree changes")); > ret = -1; > goto done; > } > > - if (keep_index < 1) { > + if (!keep_index) { > struct child_process cp = CHILD_PROCESS_INIT; > > cp.git_cmd = 1; > strvec_pushl(&cp.args, "reset", "-q", "--refresh", "--", > NULL); > add_pathspecs(&cp.args, ps); > @@ -1718,13 +1745,13 @@ static int push_stash(int argc, const char **argv, const char *prefix, > int push_assumed) > { > int force_assume = 0; > int keep_index = -1; > int only_staged = 0; > int patch_mode = 0; > - int include_untracked = 0; > + int include_untracked = -1; > int quiet = 0; > int pathspec_file_nul = 0; > const char *stash_msg = NULL; > const char *pathspec_from_file = NULL; > struct pathspec ps; > struct option options[] = { > @@ -1798,13 +1825,13 @@ static int push_stash_unassumed(int argc, const char **argv, const char *prefix) > > static int save_stash(int argc, const char **argv, const char *prefix) > { > int keep_index = -1; > int only_staged = 0; > int patch_mode = 0; > - int include_untracked = 0; > + int include_untracked = -1; > int quiet = 0; > int ret = 0; > const char *stash_msg = NULL; > struct pathspec ps; > struct strbuf stash_msg_buf = STRBUF_INIT; > struct option options[] = { > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 00db82fb24..4ffcca742c 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -1565,7 +1565,126 @@ test_expect_success 'stash apply reports a locked index' ' > EOF > test_must_fail git stash apply 2>err && > test_cmp expect err > ) > ' > > +setup_dirty() { > + echo a >tracked && > + echo b >added-modified && > + git add tracked added-modified && > + git commit -m initial && > + echo 1 >>tracked && > + echo 2 >>added-modified && > + echo c >added-new && > + echo d >untracked && > + git add added-modified added-new > +} > + > +test_expect_success 'stash.includeuntracked equivalent to --include-untracked' ' > + git init using_opt && > + test_when_finished rm -rf using_opt && > + ( > + cd using_opt && > + setup_dirty && > + git stash push && > + git stash show -u --patch >../using-opt > + ) && > + > + test_config stash.includeuntracked true && > + git init using_config && > + test_when_finished rm -rf using_config && > + ( > + cd using_config && > + setup_dirty && > + git stash push && > + git stash show -u --patch >../using-config > + ) && > + > + test_cmp using-opt using-config > +' > + > +test_expect_success 'stash.includeuntracked yields to --no-include-untracked' ' > + git init no_config && > + test_when_finished rm -rf no_config && > + ( > + cd no_config && > + setup_dirty && > + git stash push --no-include-untracked && > + git stash show -u --patch >../no-config > + ) && > + > + test_config stash.includeuntracked true && > + git init using_config && > + test_when_finished rm -rf using_config && > + ( > + cd using_config && > + setup_dirty && > + git stash push --no-include-untracked && > + git stash show -u --patch >../using-config > + ) && > + > + test_cmp no-config using-config > +' > + > +test_expect_success 'stash.includeuntracked succeeds with --patch' ' > + test_config stash.includeuntracked true && > + git stash --patch > +' > + > +test_expect_success 'stash.includeuntracked succeeds with --staged' ' > + test_config stash.includeuntracked true && > + git stash --staged > +' > + > +test_expect_success 'stash.keepindex equivalent to --keep-index' ' > + git init using_opt && > + test_when_finished rm -rf using_opt && > + ( > + cd using_opt && > + setup_dirty && > + git stash push && > + git stash show -u --patch >../using-opt > + ) && > + > + test_config stash.keepindex true && > + git init using_config && > + test_when_finished rm -rf using_config && > + ( > + cd using_config && > + setup_dirty && > + git stash push && > + git stash show -u --patch >../using-config > + ) && > + > + test_cmp using-opt using-config > +' > + > +test_expect_success 'stash.keepindex yields to --no-keep-index' ' > + git init no_config && > + test_when_finished rm -rf no_config && > + ( > + cd no_config && > + setup_dirty && > + git stash push --no-keep-index && > + git stash show -u --patch >../no-config > + ) && > + > + test_config stash.keepindex true && > + git init using_config && > + test_when_finished rm -rf using_config && > + ( > + cd using_config && > + setup_dirty && > + git stash push --no-keep-index && > + git stash show -u --patch >../using-config > + ) && > + > + test_cmp no-config using-config > +' > + > +test_expect_success 'stash.keepindex succeeds with --patch' ' > + test_config stash.keepindex true && > + git stash --patch > +' > + > test_done > -- > 2.43.2 >
Hi Phillip, On 2/18/24 05:32, Phillip Wood wrote: > How does "stash.keepIndex" interact with "git rebase --autostash" and "git > merge --autostash"? I think both those commands expect a clean index after > running "git stash". They could just override the config setting but it might > get a bit confusing if some commands respect the config and others don't. Both `git rebase --autostash` and `git merge --autostash` seem to be hardcoded to clean the index, regardless of the configuration or CLI flags. They do not use regular `git stash` to do so, but rather `git stash create`. This is unaffected by my changes, since it follows a different code path and does not accept `--keep-index` nor `--include-untracked`. I'll add some tests for `git rebase --autostash` and `git merge --autostash`, just in case this behavior is changed in the future and causes breakage. > I've only given the patch a very quick scan, but it looked sensible. The only > thing that jumped out at me was that quite a few tests seem to do > > git init repo && > ( > cd repo && > # test things > ) && > > Our normal practice is to run all the tests in the same file in the same > repository rather than setting up a new one each time. I was doing this because it makes comparing different commands easier, but looking through other tests again, it seems like I should be comparing the outputs to hardcoded files anyway. Thank you, Ricardo
Hello, Le 18/02/2024 à 04:30, MithicSpirit a écrit : > Adds options `stash.keepIndex` and `stash.includeUntracked`, which > enable `--keep-index` and `--include-untracked` by default (unless it > would conflict with another option). This is done to facilitate the > workflow where a user: > > . Makes modifications; > . Selectively adds them with `git add -p`; and > . Stashes the unadded changes to run tests with only the current > modifications. > > `stash.keepIndex` is especially important for this, as otherwise the > work done in `git add -p` is lost since applying the stash does not > restore the index. This problem could also be solved by adding > functionality to the stash to restore the index specifically, but this > would create much more complexity and still wouldn't be as convenient > for that workflow. > > Signed-off-by: Ricardo Prado Cunha (MithicSpirit) <rpc01234@gmail.com> > --- > This is my first patch to git and my first time using mailing lists for this > kind of stuff. Please do let me know of any mistakes or gaffes I've made. > > Documentation/config/stash.txt | 13 ++++ > builtin/stash.c | 65 ++++++++++++------ > t/t3903-stash.sh | 119 +++++++++++++++++++++++++++++++++ > 3 files changed, 178 insertions(+), 19 deletions(-) > > diff --git a/Documentation/config/stash.txt b/Documentation/config/stash.txt > index ec1edaeba6..d6d9ea7daa 100644 > --- a/Documentation/config/stash.txt > +++ b/Documentation/config/stash.txt > @@ -1,6 +1,19 @@ > +stash.includeUntracked:: > + Boolean option that configures whether the `git stash push` and `git > + stash save` commands also stash untracked files by default. This option > + is ignored if `--include-untracked`, `--no-include-untracked`, `--all`, > + `--patch`, or `--staged` are used. Defaults to `false`. See > + linkgit:git-stash[1]. > + > +stash.keepIndex:: > + Boolean option that configures whether the `git stash push` and `git > + stash save` commands also stash changes that have been added to the > + index. This option is ignored if `--keep-index`, `--no-keep-index`, or > + `--patch` are used. Defaults to `false`. See linkgit:git-stash[1]. > + > stash.showIncludeUntracked:: > If this is set to true, the `git stash show` command will show > the untracked files of a stash entry. Defaults to false. See > the description of the 'show' command in linkgit:git-stash[1]. > > stash.showPatch:: > diff --git a/builtin/stash.c b/builtin/stash.c > index 7fb355bff0..c591a2bf4b 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -836,12 +836,14 @@ static int list_stash(int argc, const char **argv, const char *prefix) > return run_command(&cp); > } > > static int show_stat = 1; > static int show_patch; > static int show_include_untracked; > +static int default_keep_index; > +static int default_include_untracked; > > static int git_stash_config(const char *var, const char *value, > const struct config_context *ctx, void *cb) > { > if (!strcmp(var, "stash.showstat")) { > show_stat = git_config_bool(var, value); > @@ -852,12 +854,20 @@ static int git_stash_config(const char *var, const char *value, > return 0; > } > if (!strcmp(var, "stash.showincludeuntracked")) { > show_include_untracked = git_config_bool(var, value); > return 0; > } > + if (!strcmp(var, "stash.keepindex")) { > + default_keep_index = git_config_bool(var, value); > + return 0; > + } > + if (!strcmp(var, "stash.includeuntracked")) { > + default_include_untracked = git_config_bool(var, value); > + return 0; > + } > return git_diff_basic_config(var, value, ctx, cb); > } > > static void diff_include_untracked(const struct stash_info *info, struct diff_options *diff_opt) > { > const struct object_id *oid[] = { &info->w_commit, &info->u_tree }; > @@ -1509,33 +1519,50 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q > int ret = 0; > struct stash_info info = STASH_INFO_INIT; > struct strbuf patch = STRBUF_INIT; > struct strbuf stash_msg_buf = STRBUF_INIT; > struct strbuf untracked_files = STRBUF_INIT; > > - if (patch_mode && keep_index == -1) > - keep_index = 1; > - > - if (patch_mode && include_untracked) { > - fprintf_ln(stderr, _("Can't use --patch and --include-untracked" > - " or --all at the same time")); > - ret = -1; > - goto done; > + if (keep_index == -1) { > + if (patch_mode) > + keep_index = 1; > + else > + keep_index = default_keep_index; > } > > - /* --patch overrides --staged */ > - if (patch_mode) > + if (patch_mode) { > + if (include_untracked == -1) > + include_untracked = 0; > + else if (include_untracked) { > + fprintf_ln(stderr, > + _("Can't use --patch and --include-untracked" > + " or --all at the same time")); > + ret = -1; > + goto done; > + } > + > + /* --patch overrides --staged */ > only_staged = 0; > - > - if (only_staged && include_untracked) { > - fprintf_ln(stderr, _("Can't use --staged and --include-untracked" > - " or --all at the same time")); > - ret = -1; > - goto done; > } > > + if (only_staged) { > + if (include_untracked == -1) > + include_untracked = 0; > + else if (include_untracked) { > + fprintf_ln( > + stderr, > + _("Can't use --staged and --include-untracked" > + " or --all at the same time")); > + ret = -1; > + goto done; > + } > + } > + > + if (include_untracked == -1) > + include_untracked = default_include_untracked; > + I'm not sure this would be better, but instead of mixing option compatibility and actually building your logic, why not use a series of die_for_incompatible_opt3 and the like in order to clear the option lists, then build your action logic without resorting to special values? > repo_read_index_preload(the_repository, NULL, 0); > if (!include_untracked && ps->nr) { > int i; > char *ps_matched = xcalloc(ps->nr, 1); > > /* TODO: audit for interaction with sparse-index. */ > @@ -1688,13 +1715,13 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q > fprintf_ln(stderr, _("Cannot remove " > "worktree changes")); > ret = -1; > goto done; > } > > - if (keep_index < 1) { > + if (!keep_index) { > struct child_process cp = CHILD_PROCESS_INIT; > > cp.git_cmd = 1; > strvec_pushl(&cp.args, "reset", "-q", "--refresh", "--", > NULL); > add_pathspecs(&cp.args, ps); > @@ -1718,13 +1745,13 @@ static int push_stash(int argc, const char **argv, const char *prefix, > int push_assumed) > { > int force_assume = 0; > int keep_index = -1; > int only_staged = 0; > int patch_mode = 0; > - int include_untracked = 0; > + int include_untracked = -1; > int quiet = 0; > int pathspec_file_nul = 0; > const char *stash_msg = NULL; > const char *pathspec_from_file = NULL; > struct pathspec ps; > struct option options[] = { > @@ -1798,13 +1825,13 @@ static int push_stash_unassumed(int argc, const char **argv, const char *prefix) > > static int save_stash(int argc, const char **argv, const char *prefix) > { > int keep_index = -1; > int only_staged = 0; > int patch_mode = 0; > - int include_untracked = 0; > + int include_untracked = -1; > int quiet = 0; > int ret = 0; > const char *stash_msg = NULL; > struct pathspec ps; > struct strbuf stash_msg_buf = STRBUF_INIT; > struct option options[] = { > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 00db82fb24..4ffcca742c 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -1565,7 +1565,126 @@ test_expect_success 'stash apply reports a locked index' ' > EOF > test_must_fail git stash apply 2>err && > test_cmp expect err > ) > ' > > +setup_dirty() { > + echo a >tracked && > + echo b >added-modified && > + git add tracked added-modified && > + git commit -m initial && > + echo 1 >>tracked && > + echo 2 >>added-modified && > + echo c >added-new && > + echo d >untracked && > + git add added-modified added-new > +} > + > +test_expect_success 'stash.includeuntracked equivalent to --include-untracked' ' > + git init using_opt && > + test_when_finished rm -rf using_opt && > + ( > + cd using_opt && > + setup_dirty && > + git stash push && > + git stash show -u --patch >../using-opt > + ) && > + > + test_config stash.includeuntracked true && > + git init using_config && > + test_when_finished rm -rf using_config && > + ( > + cd using_config && > + setup_dirty && > + git stash push && > + git stash show -u --patch >../using-config > + ) && > + > + test_cmp using-opt using-config > +' > + > +test_expect_success 'stash.includeuntracked yields to --no-include-untracked' ' > + git init no_config && > + test_when_finished rm -rf no_config && > + ( > + cd no_config && > + setup_dirty && > + git stash push --no-include-untracked && > + git stash show -u --patch >../no-config > + ) && > + > + test_config stash.includeuntracked true && > + git init using_config && > + test_when_finished rm -rf using_config && > + ( > + cd using_config && > + setup_dirty && > + git stash push --no-include-untracked && > + git stash show -u --patch >../using-config > + ) && > + > + test_cmp no-config using-config > +' > + > +test_expect_success 'stash.includeuntracked succeeds with --patch' ' > + test_config stash.includeuntracked true && > + git stash --patch > +' > + > +test_expect_success 'stash.includeuntracked succeeds with --staged' ' > + test_config stash.includeuntracked true && > + git stash --staged > +' > + > +test_expect_success 'stash.keepindex equivalent to --keep-index' ' > + git init using_opt && > + test_when_finished rm -rf using_opt && > + ( > + cd using_opt && > + setup_dirty && > + git stash push && > + git stash show -u --patch >../using-opt > + ) && > + > + test_config stash.keepindex true && > + git init using_config && > + test_when_finished rm -rf using_config && > + ( > + cd using_config && > + setup_dirty && > + git stash push && > + git stash show -u --patch >../using-config > + ) && > + > + test_cmp using-opt using-config > +' > + > +test_expect_success 'stash.keepindex yields to --no-keep-index' ' > + git init no_config && > + test_when_finished rm -rf no_config && > + ( > + cd no_config && > + setup_dirty && > + git stash push --no-keep-index && > + git stash show -u --patch >../no-config > + ) && > + > + test_config stash.keepindex true && > + git init using_config && > + test_when_finished rm -rf using_config && > + ( > + cd using_config && > + setup_dirty && > + git stash push --no-keep-index && > + git stash show -u --patch >../using-config > + ) && > + > + test_cmp no-config using-config > +' > + > +test_expect_success 'stash.keepindex succeeds with --patch' ' > + test_config stash.keepindex true && > + git stash --patch > +' > + > test_done > -- > 2.43.2 >
Hello, On 2/19/24 03:04, Jean-Noël Avila wrote: > I'm not sure this would be better, but instead of mixing option compatibility > and actually building your logic, why not use a series of > die_for_incompatible_opt3 and the like in order to clear the option lists, > then build your action logic without resorting to special values? I'm not sure dying would be appropriate here, since the original behavior was already to clean up buffers and such and then return an errorful value (-1), and I'd rather keep maximum backwards compatibility. Also, the special value of -1 for `keep_index` and `include_untracked` is necessary to detect whether the variable was set by the CLI flags or if it is fine to override with the config value. This could be addressed by moving more logic up to `push_stash` and `save_stash` (where the arguments are parsed), but this would need much more rewriting and would lead to some code duplication, for what I think is minimal gain. Thank you, Ricardo
Phillip Wood <phillip.wood123@gmail.com> writes: > How does "stash.keepIndex" interact with "git rebase --autostash" and > "git merge --autostash"? I think both those commands expect a clean > index after running "git stash". They could just override the config > setting but it might get a bit confusing if some commands respect the > config and others don't. The "autostash" feature fundamentally should not respect such configuration variables, as the reason why they exist is not to create a stash of any unusual kind the user expresses preference to by the configuration variable(s), but to clear the slate and bring both the working tree and the index pristine with respect to HEAD. Also, when able, they would automatically unstash after the main operation to rebase or merge is done, right? IOW, normally the use of the stash ought to be invisible to the end users. I would not worry too much about the "confusion" factor for these reasons. You are however right that this will confuse the toolchain. These two commands we provide may not be affected (or we can make them not affected by changing their implementation if needed, while we add such configuration variables at the same time), but third-party tools and end-user scripts that has trusted that when they write "git stash", the command will give them a clean index and working tree will be broken big time. So, I am somewhat negative on the patch in the current form, until I see how the plan to help third-party tools and end-user scripts that rely on the promise we have given them looks like. Thanks.
Hello Junio, On 2/19/24 21:52, Junio C Hamano wrote: > You are however right that this will confuse the toolchain. These > two commands we provide may not be affected (or we can make them not > affected by changing their implementation if needed, while we add > such configuration variables at the same time), but third-party > tools and end-user scripts that has trusted that when they write > "git stash", the command will give them a clean index and working > tree will be broken big time. > > So, I am somewhat negative on the patch in the current form, until I > see how the plan to help third-party tools and end-user scripts that > rely on the promise we have given them looks like. This is an issue I hadn't considered, and I'm not sure whether it can even be fixed. In some sense, the entire point of this patch is to allow the user to break that promise in their configuration. However, I'm not sure how big of a problem this is, as it is entirely opt-in (default behavior should be the same as current behavior), and tools can be altered to pass `--no-keep-index --no-include-untracked` if they wish to force the current behavior. Either way, I would like to address your concern if possible, and I'd appreciate any ideas on how to do so. Thank you, Ricardo
Ricardo C <rpc01234@gmail.com> writes: > This is an issue I hadn't considered, and I'm not sure whether it can > even be fixed. In some sense, the entire point of this patch is to > allow the user to break that promise in their configuration. However, > I'm not sure how big of a problem this is, as it is entirely opt-in > (default behavior should be the same as current behavior), Correct. > and tools > can be altered to pass `--no-keep-index --no-include-untracked` if > they wish to force the current behavior. This is not. People expect a bit better from Git, and such a callous disregard to backward compatibility that breaks other people's tools and scripts is a non-starter. Users of such tools, whether they were written by themselves or other people, do *not* want them to break only because they want to use a shiny new feature that is advertised in a new version of Git. The point of packaging a solution, the reason why they wroute such a tool or script that happens to use "git stash" as an ingredient and depends on the current behaviour of "git stash", is so that they do not need to remember they even used "git stash" as a small part of their solution. And of course they do not want to remember that they rely on how "git stash" behaves in such a solution. They do not even want to bother complaining loudly when such a change is proposed before it hits a release and hurt them. Saying "nobody complained when these configuration variables were proposed" does not help anybody later, after we already hurt them.
On 2/19/24 22:44, Junio C Hamano wrote: > Ricardo C <rpc01234@gmail.com> writes: > >> This is an issue I hadn't considered, and I'm not sure whether it can >> even be fixed. In some sense, the entire point of this patch is to >> allow the user to break that promise in their configuration. However, >> I'm not sure how big of a problem this is, as it is entirely opt-in >> (default behavior should be the same as current behavior), > > Correct. > >> and tools >> can be altered to pass `--no-keep-index --no-include-untracked` if >> they wish to force the current behavior. > > This is not. > > People expect a bit better from Git, and such a callous disregard to > backward compatibility that breaks other people's tools and scripts > is a non-starter. > > Users of such tools, whether they were written by themselves or > other people, do *not* want them to break only because they want to > use a shiny new feature that is advertised in a new version of Git. > > The point of packaging a solution, the reason why they wroute such a > tool or script that happens to use "git stash" as an ingredient and > depends on the current behaviour of "git stash", is so that they do > not need to remember they even used "git stash" as a small part of > their solution. And of course they do not want to remember that > they rely on how "git stash" behaves in such a solution. They do > not even want to bother complaining loudly when such a change is > proposed before it hits a release and hurt them. Saying "nobody > complained when these configuration variables were proposed" does > not help anybody later, after we already hurt them. That makes sense. Do you have any ideas on how to address this? It feels to me like providing this config option is fundamentally incompatible with requiring backwards-compatible behavior regardless of configuration. Ricardo
Hi Ricardo On 18/02/2024 17:54, Ricardo C wrote: > Hi Phillip, > > On 2/18/24 05:32, Phillip Wood wrote: >> How does "stash.keepIndex" interact with "git rebase --autostash" and >> "git merge --autostash"? I think both those commands expect a clean >> index after running "git stash". They could just override the config >> setting but it might get a bit confusing if some commands respect the >> config and others don't. > > Both `git rebase --autostash` and `git merge --autostash` seem to be > hardcoded to clean the index, regardless of the configuration or CLI > flags. They do not use regular `git stash` to do so, but rather `git > stash create`. This is unaffected by my changes, since it follows a > different code path and does not accept `--keep-index` nor > `--include-untracked`. Ah, I'd forgotten that we hand "git stash create" as well as "git stash push". It's good that these changes do not affect "git stash create" but I think Junio is right to be concerned about the effect on tools that are using "git stash push" where they arguably ought to be using "git stash create". > I'll add some tests for `git rebase --autostash` > and `git merge --autostash`, just in case this behavior is changed in > the future and causes breakage. That's a good idea but I think it would be better to test that "git stash create" is not affected by the config as we don't want to change the behavior of its behavior. >> I've only given the patch a very quick scan, but it looked sensible. >> The only thing that jumped out at me was that quite a few tests seem >> to do >> >> git init repo && >> ( >> cd repo && >> # test things >> ) && >> >> Our normal practice is to run all the tests in the same file in the >> same repository rather than setting up a new one each time. > > I was doing this because it makes comparing different commands easier, > but looking through other tests again, it seems like I should be > comparing the outputs to hardcoded files anyway. Yes, that is our usual practice. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > That's a good idea but I think it would be better to test that "git > stash create" is not affected by the config as we don't want to change > the behavior of its behavior. Yup. Making sure "stash create" stays the same is a very good thing. Thanks for suggesting it. Regardling the need to have an escape hatch that is well publicized long before a potentially harmful switch can safely happen, one way out might be to - Declare "git stash create" is a plumbing, but the rest is considered Porcelain. Publicize it well and wait for the word to spread out. Folks who are in favor of adding these configuration variables to the system may need to do some legwork, spreading the word around at stackoverflow and various other venues, scouring public repositories at GitHub and other hosting sites and studying third-party uses of "git stash" that should be replaced with "git stash create" followed by "git update-ref", and raising issues to notify the owners of such projects, etc. - Add breaking configuration variables after a few years. We cannot do the usual "start warning when we detect problematic use without changing the current behaviour, wait, and then switch" dance in this case, unfortunately, because we lack a good way to detect a "problematic use". We cannot tell a direct use of "git stash" via the command prompt (which may want to honor the configuration) from an IDE running "git stash" reacting to the "[Stash Now]" UI element (which might want to honor the configuration) and from a third-party tool that acts like "rebase --autostash" that wants to save away all local changes to clear the slate to do its thing (which we should definitely not interfare). If we could do so reliably, then we wouldn't be having this discussion---we'd be using the same logic as we would use to tell when to warn and instead of warning, just silently refraining to honor the configuration variables. An eve easier way out of course is not to do these variables, of course. FWIW, I can see how permanently enabling "include untracked" may be OK in a workflow, but I cannot see the utility of permanently enabling "keepindex" at all. Sometimes I'd want to clear the slate so that I can try building and testing what I added to the index and that is why I want to use the option. But a lot of other times, I want to clear the slate to go back to the very pristine state that is equal to the HEAD. As the need to switch between the two modes happens quite often, the way to defeat configured stash.keepindex takes quite many keystrokes, and in general I think the regular stashing happens far more often than keepindex stashing, I'd find that using shorthand "-k" on the command line occasionally without having this configuration variable would be what people would end up using anyway. And there is always "[alias] stk = stash -k" available ;-)
On Tue, Feb 20, 2024, at 04:59, Ricardo C wrote: > That makes sense. Do you have any ideas on how to address this? It feels to me > like providing this config option is fundamentally incompatible with requiring > backwards-compatible behavior regardless of configuration. If my armchair also had a time machine: one could have used an `-s` option to `git` (`git -s stash`) that only reads a few configuration variables, like `user.name` and `user.email`. It would read none of the variables meant for making interactive/custom use convenient. This would be meant for scripts. That way new user-convenience config variables wouldn’t interfere with some `git -s` invocation deep in some script somewhere. As for the status quo though one seems stuck.
On 2/20/24 12:47, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> That's a good idea but I think it would be better to test that "git >> stash create" is not affected by the config as we don't want to change >> the behavior of its behavior. > > Yup. Making sure "stash create" stays the same is a very good > thing. Thanks for suggesting it. I agree. I'll make sure to add testing for it if/when we decide to go through with this change. > Regardling the need to have an escape hatch that is well publicized > long before a potentially harmful switch can safely happen, one way > out might be to > > - Declare "git stash create" is a plumbing, but the rest is > considered Porcelain. Publicize it well and wait for the word to > spread out. Folks who are in favor of adding these configuration > variables to the system may need to do some legwork, spreading > the word around at stackoverflow and various other venues, > scouring public repositories at GitHub and other hosting sites > and studying third-party uses of "git stash" that should be > replaced with "git stash create" followed by "git update-ref", > and raising issues to notify the owners of such projects, etc. > > - Add breaking configuration variables after a few year This feels like quite a lot of work. I guess that the main question now is whether this change is worth the effort. What would publicizing this look like? Updating documentation? Sending out an announcement? I can also imagine some tools would dislike having to switch to the more manual stash+update-ref, so maybe easier plumbing should be added? > FWIW, I can see how permanently enabling "include untracked" may be > OK in a workflow, but I cannot see the utility of permanently > enabling "keepindex" at all. Sometimes I'd want to clear the slate > so that I can try building and testing what I added to the index and > that is why I want to use the option. But a lot of other times, I > want to clear the slate to go back to the very pristine state that > is equal to the HEAD. As the need to switch between the two modes > happens quite often, the way to defeat configured stash.keepindex > takes quite many keystrokes, and in general I think the regular > stashing happens far more often than keepindex stashing, I'd find > that using shorthand "-k" on the command line occasionally without > having this configuration variable would be what people would end up > using anyway. Permanently enabling keepIndex is mainly intended for people that like to stash their unstaged changes before committing (e.g., for testing them independently of other changes). The main issue with what you recommend is that, if they forget to use `-k`, then the entire state of the index is lost, which is especially problematic if the changes were interactively staged. A report of someone having issues with this workflow is here[1]. Perhaps a better solution would be to provide some mechanism by which to also save the state of the index when stashing changes, and also restore it when applying the stash. I figure this change in behavior would be much less problematic. Another option would be to go closer to the route of `git switch` and `git restore`, where a separate command with a more user-friendly interface is added. Then all of `git stash` could be kept as plumbing, and this new command would be entirely porcelain. I don't think that this change would be worth introducing a new command for, but if there are other changes that could also be included, then it might make sense. > And there is always "[alias] stk = stash -k" available ;-) Yeah, currently I use a shell alias for `git stash -ku`, but that doesn't help when I'm using other git frontends. [1] https://bsd.network/@ed1conf/111783574839749798
Ricardo C <rpc01234@gmail.com> writes: > Permanently enabling keepIndex is mainly intended for people that like > to stash their unstaged changes before committing (e.g., for testing > them independently of other changes). The main issue with what you > recommend is that, if they forget to use `-k`, then the entire state > of the index is lost, which is especially problematic if the changes > were interactively staged. Doesn't "git stash pop --index" meant to recover from such a mistake, though? If you stash, because your "git commit" notices there is no change after you did "git stash" without "-k", your recovery to "pop --index" would apply the changes to the index and to the working tree on top of exactly the same commit, so there is no risk of losing any changes by doing so, right? IOW, such a pop will always round-trip.
On 2/21/24 15:09, Junio C Hamano wrote: > Ricardo C <rpc01234@gmail.com> writes: > >> Permanently enabling keepIndex is mainly intended for people that like >> to stash their unstaged changes before committing (e.g., for testing >> them independently of other changes). The main issue with what you >> recommend is that, if they forget to use `-k`, then the entire state >> of the index is lost, which is especially problematic if the changes >> were interactively staged. > > Doesn't "git stash pop --index" meant to recover from such a > mistake, though? If you stash, because your "git commit" notices > there is no change after you did "git stash" without "-k", your > recovery to "pop --index" would apply the changes to the index and > to the working tree on top of exactly the same commit, so there is > no risk of losing any changes by doing so, right? IOW, such a pop > will always round-trip. Oh, wow, I didn't realize that was a thing (I mistakenly interpreted the documentation to mean that it added the entire stash to the index). This does help a lot, thanks! I guess keepIndex is a lot less important, so this should mainly be about includeUntracked... but now I'm also longing for a config to have --index on pop/apply by default :P Interestingly, it seems like frontend support for --index is very variable. VScode is probably the most popular frontend and doesn't seem to have any support for it, while more niche frontends like Emacs Magit and vim-fugitive do support it and moreover have it as the default. Maybe the real issue here is lack of awareness of that flag, and the solution is just to spread the word about it. Thanks again, Ricardo
Junio C Hamano <gitster@pobox.com> writes: > Ricardo C <rpc01234@gmail.com> writes: > >> Permanently enabling keepIndex is mainly intended for people that like >> to stash their unstaged changes before committing (e.g., for testing >> them independently of other changes). The main issue with what you >> recommend is that, if they forget to use `-k`, then the entire state >> of the index is lost, which is especially problematic if the changes >> were interactively staged. > > Doesn't "git stash pop --index" meant to recover from such a > mistake, though? If you stash, because your "git commit" notices > there is no change after you did "git stash" without "-k", your > recovery to "pop --index" would apply the changes to the index and > to the working tree on top of exactly the same commit, so there is > no risk of losing any changes by doing so, right? IOW, such a pop > will always round-trip. Actually, "git commit" gets into the picture of making and recovering from such a mistake a bit more costly than I made it sound in the above. My bad. The common sequence is $ edit edit edit $ git add -p $ git stash -k $ build what is in the index and test and then when you are happy, conclude it with $ git commit [NO -o/-i/pathspec] followed by $ git stash pop to continue for the next commit. So "git commit" should notice if your earlier "stash -k" forgot to say "-k", but by that time, you would have wasted the whole build and test cycle. The HEAD wouldn't have moved, so the conclusion that "pop --index" would be a good way to recover from "stash" without "--keep" does not change, though.
diff --git a/Documentation/config/stash.txt b/Documentation/config/stash.txt index ec1edaeba6..d6d9ea7daa 100644 --- a/Documentation/config/stash.txt +++ b/Documentation/config/stash.txt @@ -1,6 +1,19 @@ +stash.includeUntracked:: + Boolean option that configures whether the `git stash push` and `git + stash save` commands also stash untracked files by default. This option + is ignored if `--include-untracked`, `--no-include-untracked`, `--all`, + `--patch`, or `--staged` are used. Defaults to `false`. See + linkgit:git-stash[1]. + +stash.keepIndex:: + Boolean option that configures whether the `git stash push` and `git + stash save` commands also stash changes that have been added to the + index. This option is ignored if `--keep-index`, `--no-keep-index`, or + `--patch` are used. Defaults to `false`. See linkgit:git-stash[1]. + stash.showIncludeUntracked:: If this is set to true, the `git stash show` command will show the untracked files of a stash entry. Defaults to false. See the description of the 'show' command in linkgit:git-stash[1]. stash.showPatch:: diff --git a/builtin/stash.c b/builtin/stash.c index 7fb355bff0..c591a2bf4b 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -836,12 +836,14 @@ static int list_stash(int argc, const char **argv, const char *prefix) return run_command(&cp); } static int show_stat = 1; static int show_patch; static int show_include_untracked; +static int default_keep_index; +static int default_include_untracked; static int git_stash_config(const char *var, const char *value, const struct config_context *ctx, void *cb) { if (!strcmp(var, "stash.showstat")) { show_stat = git_config_bool(var, value); @@ -852,12 +854,20 @@ static int git_stash_config(const char *var, const char *value, return 0; } if (!strcmp(var, "stash.showincludeuntracked")) { show_include_untracked = git_config_bool(var, value); return 0; } + if (!strcmp(var, "stash.keepindex")) { + default_keep_index = git_config_bool(var, value); + return 0; + } + if (!strcmp(var, "stash.includeuntracked")) { + default_include_untracked = git_config_bool(var, value); + return 0; + } return git_diff_basic_config(var, value, ctx, cb); } static void diff_include_untracked(const struct stash_info *info, struct diff_options *diff_opt) { const struct object_id *oid[] = { &info->w_commit, &info->u_tree }; @@ -1509,33 +1519,50 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q int ret = 0; struct stash_info info = STASH_INFO_INIT; struct strbuf patch = STRBUF_INIT; struct strbuf stash_msg_buf = STRBUF_INIT; struct strbuf untracked_files = STRBUF_INIT; - if (patch_mode && keep_index == -1) - keep_index = 1; - - if (patch_mode && include_untracked) { - fprintf_ln(stderr, _("Can't use --patch and --include-untracked" - " or --all at the same time")); - ret = -1; - goto done; + if (keep_index == -1) { + if (patch_mode) + keep_index = 1; + else + keep_index = default_keep_index; } - /* --patch overrides --staged */ - if (patch_mode) + if (patch_mode) { + if (include_untracked == -1) + include_untracked = 0; + else if (include_untracked) { + fprintf_ln(stderr, + _("Can't use --patch and --include-untracked" + " or --all at the same time")); + ret = -1; + goto done; + } + + /* --patch overrides --staged */ only_staged = 0; - - if (only_staged && include_untracked) { - fprintf_ln(stderr, _("Can't use --staged and --include-untracked" - " or --all at the same time")); - ret = -1; - goto done; } + if (only_staged) { + if (include_untracked == -1) + include_untracked = 0; + else if (include_untracked) { + fprintf_ln( + stderr, + _("Can't use --staged and --include-untracked" + " or --all at the same time")); + ret = -1; + goto done; + } + } + + if (include_untracked == -1) + include_untracked = default_include_untracked; + repo_read_index_preload(the_repository, NULL, 0); if (!include_untracked && ps->nr) { int i; char *ps_matched = xcalloc(ps->nr, 1); /* TODO: audit for interaction with sparse-index. */ @@ -1688,13 +1715,13 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q fprintf_ln(stderr, _("Cannot remove " "worktree changes")); ret = -1; goto done; } - if (keep_index < 1) { + if (!keep_index) { struct child_process cp = CHILD_PROCESS_INIT; cp.git_cmd = 1; strvec_pushl(&cp.args, "reset", "-q", "--refresh", "--", NULL); add_pathspecs(&cp.args, ps); @@ -1718,13 +1745,13 @@ static int push_stash(int argc, const char **argv, const char *prefix, int push_assumed) { int force_assume = 0; int keep_index = -1; int only_staged = 0; int patch_mode = 0; - int include_untracked = 0; + int include_untracked = -1; int quiet = 0; int pathspec_file_nul = 0; const char *stash_msg = NULL; const char *pathspec_from_file = NULL; struct pathspec ps; struct option options[] = { @@ -1798,13 +1825,13 @@ static int push_stash_unassumed(int argc, const char **argv, const char *prefix) static int save_stash(int argc, const char **argv, const char *prefix) { int keep_index = -1; int only_staged = 0; int patch_mode = 0; - int include_untracked = 0; + int include_untracked = -1; int quiet = 0; int ret = 0; const char *stash_msg = NULL; struct pathspec ps; struct strbuf stash_msg_buf = STRBUF_INIT; struct option options[] = { diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 00db82fb24..4ffcca742c 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1565,7 +1565,126 @@ test_expect_success 'stash apply reports a locked index' ' EOF test_must_fail git stash apply 2>err && test_cmp expect err ) ' +setup_dirty() { + echo a >tracked && + echo b >added-modified && + git add tracked added-modified && + git commit -m initial && + echo 1 >>tracked && + echo 2 >>added-modified && + echo c >added-new && + echo d >untracked && + git add added-modified added-new +} + +test_expect_success 'stash.includeuntracked equivalent to --include-untracked' ' + git init using_opt && + test_when_finished rm -rf using_opt && + ( + cd using_opt && + setup_dirty && + git stash push && + git stash show -u --patch >../using-opt + ) && + + test_config stash.includeuntracked true && + git init using_config && + test_when_finished rm -rf using_config && + ( + cd using_config && + setup_dirty && + git stash push && + git stash show -u --patch >../using-config + ) && + + test_cmp using-opt using-config +' + +test_expect_success 'stash.includeuntracked yields to --no-include-untracked' ' + git init no_config && + test_when_finished rm -rf no_config && + ( + cd no_config && + setup_dirty && + git stash push --no-include-untracked && + git stash show -u --patch >../no-config + ) && + + test_config stash.includeuntracked true && + git init using_config && + test_when_finished rm -rf using_config && + ( + cd using_config && + setup_dirty && + git stash push --no-include-untracked && + git stash show -u --patch >../using-config + ) && + + test_cmp no-config using-config +' + +test_expect_success 'stash.includeuntracked succeeds with --patch' ' + test_config stash.includeuntracked true && + git stash --patch +' + +test_expect_success 'stash.includeuntracked succeeds with --staged' ' + test_config stash.includeuntracked true && + git stash --staged +' + +test_expect_success 'stash.keepindex equivalent to --keep-index' ' + git init using_opt && + test_when_finished rm -rf using_opt && + ( + cd using_opt && + setup_dirty && + git stash push && + git stash show -u --patch >../using-opt + ) && + + test_config stash.keepindex true && + git init using_config && + test_when_finished rm -rf using_config && + ( + cd using_config && + setup_dirty && + git stash push && + git stash show -u --patch >../using-config + ) && + + test_cmp using-opt using-config +' + +test_expect_success 'stash.keepindex yields to --no-keep-index' ' + git init no_config && + test_when_finished rm -rf no_config && + ( + cd no_config && + setup_dirty && + git stash push --no-keep-index && + git stash show -u --patch >../no-config + ) && + + test_config stash.keepindex true && + git init using_config && + test_when_finished rm -rf using_config && + ( + cd using_config && + setup_dirty && + git stash push --no-keep-index && + git stash show -u --patch >../using-config + ) && + + test_cmp no-config using-config +' + +test_expect_success 'stash.keepindex succeeds with --patch' ' + test_config stash.keepindex true && + git stash --patch +' + test_done
Adds options `stash.keepIndex` and `stash.includeUntracked`, which enable `--keep-index` and `--include-untracked` by default (unless it would conflict with another option). This is done to facilitate the workflow where a user: . Makes modifications; . Selectively adds them with `git add -p`; and . Stashes the unadded changes to run tests with only the current modifications. `stash.keepIndex` is especially important for this, as otherwise the work done in `git add -p` is lost since applying the stash does not restore the index. This problem could also be solved by adding functionality to the stash to restore the index specifically, but this would create much more complexity and still wouldn't be as convenient for that workflow. Signed-off-by: Ricardo Prado Cunha (MithicSpirit) <rpc01234@gmail.com> --- This is my first patch to git and my first time using mailing lists for this kind of stuff. Please do let me know of any mistakes or gaffes I've made. Documentation/config/stash.txt | 13 ++++ builtin/stash.c | 65 ++++++++++++------ t/t3903-stash.sh | 119 +++++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 19 deletions(-) -- 2.43.2