{"thread":{"id":"60945","subject":"[PATCH] builtin/stash: configs keepIndex, includeUntracked","startedAt":"2024-02-18T03:33:14Z","lastAt":"2024-02-21T23:54:04Z","messageCount":16,"participants":["MithicSpirit","Phillip Wood","Ricardo C","Jean-Noël Avila","Junio C Hamano","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"488860","messageId":"20240218033146.372727-2-rpc01234@gmail.com","threadId":"60945","inReplyTo":null,"subject":"[PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"MithicSpirit","fromEmail":"rpc01234@gmail.com","sentAt":"2024-02-18T03:30:46Z","receivedAt":"2024-02-18T03:33:14Z","isPatch":true,"sender":{"key":"rpc01234@gmail.com","avatar":null},"body":"Adds options `stash.keepIndex` and `stash.includeUntracked`, which\nenable `--keep-index` and `--include-untracked` by default (unless it\nwould conflict with another option). This is done to facilitate the\nworkflow where a user:\n\n. Makes modifications;\n. Selectively adds them with `git add -p`; and\n. Stashes the unadded changes to run tests with only the current\n  modifications.\n\n`stash.keepIndex` is especially important for this, as otherwise the\nwork done in `git add -p` is lost since applying the stash does not\nrestore the index. This problem could also be solved by adding\nfunctionality to the stash to restore the index specifically, but this\nwould create much more complexity and still wouldn't be as convenient\nfor that workflow.\n\nSigned-off-by: Ricardo Prado Cunha (MithicSpirit) <rpc01234@gmail.com>\n---\nThis is my first patch to git and my first time using mailing lists for this\nkind of stuff. Please do let me know of any mistakes or gaffes I've made.\n\n Documentation/config/stash.txt |  13 ++++\n builtin/stash.c                |  65 ++++++++++++------\n t/t3903-stash.sh               | 119 +++++++++++++++++++++++++++++++++\n 3 files changed, 178 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/config/stash.txt b/Documentation/config/stash.txt\nindex ec1edaeba6..d6d9ea7daa 100644\n--- a/Documentation/config/stash.txt\n+++ b/Documentation/config/stash.txt\n@@ -1,6 +1,19 @@\n+stash.includeUntracked::\n+\tBoolean option that configures whether the `git stash push` and `git\n+\tstash save` commands also stash untracked files by default. This option\n+\tis ignored if `--include-untracked`, `--no-include-untracked`, `--all`,\n+\t`--patch`, or `--staged` are used. Defaults to `false`. See\n+\tlinkgit:git-stash[1].\n+\n+stash.keepIndex::\n+\tBoolean option that configures whether the `git stash push` and `git\n+\tstash save` commands also stash changes that have been added to the\n+\tindex. This option is ignored if `--keep-index`, `--no-keep-index`, or\n+\t`--patch` are used. Defaults to `false`. See linkgit:git-stash[1].\n+\n stash.showIncludeUntracked::\n \tIf this is set to true, the `git stash show` command will show\n \tthe untracked files of a stash entry.  Defaults to false. See\n \tthe description of the 'show' command in linkgit:git-stash[1].\n\n stash.showPatch::\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 7fb355bff0..c591a2bf4b 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -836,12 +836,14 @@ static int list_stash(int argc, const char **argv, const char *prefix)\n \treturn run_command(&cp);\n }\n\n static int show_stat = 1;\n static int show_patch;\n static int show_include_untracked;\n+static int default_keep_index;\n+static int default_include_untracked;\n\n static int git_stash_config(const char *var, const char *value,\n \t\t\t    const struct config_context *ctx, void *cb)\n {\n \tif (!strcmp(var, \"stash.showstat\")) {\n \t\tshow_stat = git_config_bool(var, value);\n@@ -852,12 +854,20 @@ static int git_stash_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"stash.showincludeuntracked\")) {\n \t\tshow_include_untracked = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"stash.keepindex\")) {\n+\t\tdefault_keep_index = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(var, \"stash.includeuntracked\")) {\n+\t\tdefault_include_untracked = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \treturn git_diff_basic_config(var, value, ctx, cb);\n }\n\n static void diff_include_untracked(const struct stash_info *info, struct diff_options *diff_opt)\n {\n \tconst struct object_id *oid[] = { &info->w_commit, &info->u_tree };\n@@ -1509,33 +1519,50 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \tint ret = 0;\n \tstruct stash_info info = STASH_INFO_INIT;\n \tstruct strbuf patch = STRBUF_INIT;\n \tstruct strbuf stash_msg_buf = STRBUF_INIT;\n \tstruct strbuf untracked_files = STRBUF_INIT;\n\n-\tif (patch_mode && keep_index == -1)\n-\t\tkeep_index = 1;\n-\n-\tif (patch_mode && include_untracked) {\n-\t\tfprintf_ln(stderr, _(\"Can't use --patch and --include-untracked\"\n-\t\t\t\t     \" or --all at the same time\"));\n-\t\tret = -1;\n-\t\tgoto done;\n+\tif (keep_index == -1) {\n+\t\tif (patch_mode)\n+\t\t\tkeep_index = 1;\n+\t\telse\n+\t\t\tkeep_index = default_keep_index;\n \t}\n\n-\t/* --patch overrides --staged */\n-\tif (patch_mode)\n+\tif (patch_mode) {\n+\t\tif (include_untracked == -1)\n+\t\t\tinclude_untracked = 0;\n+\t\telse if (include_untracked) {\n+\t\t\tfprintf_ln(stderr,\n+\t\t\t\t   _(\"Can't use --patch and --include-untracked\"\n+\t\t\t\t     \" or --all at the same time\"));\n+\t\t\tret = -1;\n+\t\t\tgoto done;\n+\t\t}\n+\n+\t\t/* --patch overrides --staged */\n \t\tonly_staged = 0;\n-\n-\tif (only_staged && include_untracked) {\n-\t\tfprintf_ln(stderr, _(\"Can't use --staged and --include-untracked\"\n-\t\t\t\t     \" or --all at the same time\"));\n-\t\tret = -1;\n-\t\tgoto done;\n \t}\n\n+\tif (only_staged) {\n+\t\tif (include_untracked == -1)\n+\t\t\tinclude_untracked = 0;\n+\t\telse if (include_untracked) {\n+\t\t\tfprintf_ln(\n+\t\t\t\tstderr,\n+\t\t\t\t_(\"Can't use --staged and --include-untracked\"\n+\t\t\t\t  \" or --all at the same time\"));\n+\t\t\tret = -1;\n+\t\t\tgoto done;\n+\t\t}\n+\t}\n+\n+\tif (include_untracked == -1)\n+\t\tinclude_untracked = default_include_untracked;\n+\n \trepo_read_index_preload(the_repository, NULL, 0);\n \tif (!include_untracked && ps->nr) {\n \t\tint i;\n \t\tchar *ps_matched = xcalloc(ps->nr, 1);\n\n \t\t/* TODO: audit for interaction with sparse-index. */\n@@ -1688,13 +1715,13 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\t\t\tfprintf_ln(stderr, _(\"Cannot remove \"\n \t\t\t\t\t\t     \"worktree changes\"));\n \t\t\tret = -1;\n \t\t\tgoto done;\n \t\t}\n\n-\t\tif (keep_index < 1) {\n+\t\tif (!keep_index) {\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n\n \t\t\tcp.git_cmd = 1;\n \t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--refresh\", \"--\",\n \t\t\t\t     NULL);\n \t\t\tadd_pathspecs(&cp.args, ps);\n@@ -1718,13 +1745,13 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \t\t      int push_assumed)\n {\n \tint force_assume = 0;\n \tint keep_index = -1;\n \tint only_staged = 0;\n \tint patch_mode = 0;\n-\tint include_untracked = 0;\n+\tint include_untracked = -1;\n \tint quiet = 0;\n \tint pathspec_file_nul = 0;\n \tconst char *stash_msg = NULL;\n \tconst char *pathspec_from_file = NULL;\n \tstruct pathspec ps;\n \tstruct option options[] = {\n@@ -1798,13 +1825,13 @@ static int push_stash_unassumed(int argc, const char **argv, const char *prefix)\n\n static int save_stash(int argc, const char **argv, const char *prefix)\n {\n \tint keep_index = -1;\n \tint only_staged = 0;\n \tint patch_mode = 0;\n-\tint include_untracked = 0;\n+\tint include_untracked = -1;\n \tint quiet = 0;\n \tint ret = 0;\n \tconst char *stash_msg = NULL;\n \tstruct pathspec ps;\n \tstruct strbuf stash_msg_buf = STRBUF_INIT;\n \tstruct option options[] = {\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 00db82fb24..4ffcca742c 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1565,7 +1565,126 @@ test_expect_success 'stash apply reports a locked index' '\n \t\tEOF\n \t\ttest_must_fail git stash apply 2>err &&\n \t\ttest_cmp expect err\n \t)\n '\n\n+setup_dirty() {\n+\techo a >tracked &&\n+\techo b >added-modified &&\n+\tgit add tracked added-modified &&\n+\tgit commit -m initial &&\n+\techo 1 >>tracked &&\n+\techo 2 >>added-modified &&\n+\techo c >added-new &&\n+\techo d >untracked &&\n+\tgit add added-modified added-new\n+}\n+\n+test_expect_success 'stash.includeuntracked equivalent to --include-untracked' '\n+\tgit init using_opt &&\n+\ttest_when_finished rm -rf using_opt &&\n+\t(\n+\t\tcd using_opt &&\n+\t\tsetup_dirty &&\n+\t\tgit stash push &&\n+\t\tgit stash show -u --patch >../using-opt\n+\t) &&\n+\n+\ttest_config stash.includeuntracked true &&\n+\tgit init using_config &&\n+\ttest_when_finished rm -rf using_config &&\n+\t(\n+\t\tcd using_config &&\n+\t\tsetup_dirty &&\n+\t\tgit stash push &&\n+\t\tgit stash show -u --patch >../using-config\n+\t) &&\n+\n+\ttest_cmp using-opt using-config\n+'\n+\n+test_expect_success 'stash.includeuntracked yields to --no-include-untracked' '\n+\tgit init no_config &&\n+\ttest_when_finished rm -rf no_config &&\n+\t(\n+\t\tcd no_config &&\n+\t\tsetup_dirty &&\n+\t\tgit stash push --no-include-untracked &&\n+\t\tgit stash show -u --patch >../no-config\n+\t) &&\n+\n+\ttest_config stash.includeuntracked true &&\n+\tgit init using_config &&\n+\ttest_when_finished rm -rf using_config &&\n+\t(\n+\t\tcd using_config &&\n+\t\tsetup_dirty &&\n+\t\tgit stash push --no-include-untracked &&\n+\t\tgit stash show -u --patch >../using-config\n+\t) &&\n+\n+\ttest_cmp no-config using-config\n+'\n+\n+test_expect_success 'stash.includeuntracked succeeds with --patch' '\n+\ttest_config stash.includeuntracked true &&\n+\tgit stash --patch\n+'\n+\n+test_expect_success 'stash.includeuntracked succeeds with --staged' '\n+\ttest_config stash.includeuntracked true &&\n+\tgit stash --staged\n+'\n+\n+test_expect_success 'stash.keepindex equivalent to --keep-index' '\n+\tgit init using_opt &&\n+\ttest_when_finished rm -rf using_opt &&\n+\t(\n+\t\tcd using_opt &&\n+\t\tsetup_dirty &&\n+\t\tgit stash push &&\n+\t\tgit stash show -u --patch >../using-opt\n+\t) &&\n+\n+\ttest_config stash.keepindex true &&\n+\tgit init using_config &&\n+\ttest_when_finished rm -rf using_config &&\n+\t(\n+\t\tcd using_config &&\n+\t\tsetup_dirty &&\n+\t\tgit stash push &&\n+\t\tgit stash show -u --patch >../using-config\n+\t) &&\n+\n+\ttest_cmp using-opt using-config\n+'\n+\n+test_expect_success 'stash.keepindex yields to --no-keep-index' '\n+\tgit init no_config &&\n+\ttest_when_finished rm -rf no_config &&\n+\t(\n+\t\tcd no_config &&\n+\t\tsetup_dirty &&\n+\t\tgit stash push --no-keep-index &&\n+\t\tgit stash show -u --patch >../no-config\n+\t) &&\n+\n+\ttest_config stash.keepindex true &&\n+\tgit init using_config &&\n+\ttest_when_finished rm -rf using_config &&\n+\t(\n+\t\tcd using_config &&\n+\t\tsetup_dirty &&\n+\t\tgit stash push --no-keep-index &&\n+\t\tgit stash show -u --patch >../using-config\n+\t) &&\n+\n+\ttest_cmp no-config using-config\n+'\n+\n+test_expect_success 'stash.keepindex succeeds with --patch' '\n+\ttest_config stash.keepindex true &&\n+\tgit stash --patch\n+'\n+\n test_done\n--\n2.43.2\n"},{"id":"488870","messageId":"99346639-5a36-4c2e-a5d7-035c3c1fda8b@gmail.com","threadId":"60945","inReplyTo":"20240218033146.372727-2-rpc01234@gmail.com","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-18T10:32:22Z","receivedAt":"2024-02-18T10:32:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ricardo\n\nOn 18/02/2024 03:30, MithicSpirit wrote:\n> Adds options `stash.keepIndex` and `stash.includeUntracked`, which\n> enable `--keep-index` and `--include-untracked` by default (unless it\n> would conflict with another option). This is done to facilitate the\n> workflow where a user:\n> \n> . Makes modifications;\n> . Selectively adds them with `git add -p`; and\n> . Stashes the unadded changes to run tests with only the current\n>    modifications.\n> \n> `stash.keepIndex` is especially important for this, as otherwise the\n> work done in `git add -p` is lost since applying the stash does not\n> restore the index.\n\nHow does \"stash.keepIndex\" interact with \"git rebase --autostash\" and \n\"git merge --autostash\"? I think both those commands expect a clean \nindex after running \"git stash\". They could just override the config \nsetting but it might get a bit confusing if some commands respect the \nconfig and others don't.\n\n> This problem could also be solved by adding\n> functionality to the stash to restore the index specifically, but this\n> would create much more complexity and still wouldn't be as convenient\n> for that workflow.\n> \n> Signed-off-by: Ricardo Prado Cunha (MithicSpirit) <rpc01234@gmail.com>\n> ---\n> This is my first patch to git and my first time using mailing lists for this\n> kind of stuff. Please do let me know of any mistakes or gaffes I've made.\n\nI've only given the patch a very quick scan, but it looked sensible. The \nonly thing that jumped out at me was that quite a few tests seem to do\n\n\tgit init repo &&\n\t(\n\t\tcd repo &&\n\t\t# test things\n\t) &&\n\nOur normal practice is to run all the tests in the same file in the same \nrepository rather than setting up a new one each time.\n\nBest Wishes\n\nPhillip\n\n\n>   Documentation/config/stash.txt |  13 ++++\n>   builtin/stash.c                |  65 ++++++++++++------\n>   t/t3903-stash.sh               | 119 +++++++++++++++++++++++++++++++++\n>   3 files changed, 178 insertions(+), 19 deletions(-)\n> \n> diff --git a/Documentation/config/stash.txt b/Documentation/config/stash.txt\n> index ec1edaeba6..d6d9ea7daa 100644\n> --- a/Documentation/config/stash.txt\n> +++ b/Documentation/config/stash.txt\n> @@ -1,6 +1,19 @@\n> +stash.includeUntracked::\n> +\tBoolean option that configures whether the `git stash push` and `git\n> +\tstash save` commands also stash untracked files by default. This option\n> +\tis ignored if `--include-untracked`, `--no-include-untracked`, `--all`,\n> +\t`--patch`, or `--staged` are used. Defaults to `false`. See\n> +\tlinkgit:git-stash[1].\n> +\n> +stash.keepIndex::\n> +\tBoolean option that configures whether the `git stash push` and `git\n> +\tstash save` commands also stash changes that have been added to the\n> +\tindex. This option is ignored if `--keep-index`, `--no-keep-index`, or\n> +\t`--patch` are used. Defaults to `false`. See linkgit:git-stash[1].\n> +\n>   stash.showIncludeUntracked::\n>   \tIf this is set to true, the `git stash show` command will show\n>   \tthe untracked files of a stash entry.  Defaults to false. See\n>   \tthe description of the 'show' command in linkgit:git-stash[1].\n> \n>   stash.showPatch::\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 7fb355bff0..c591a2bf4b 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -836,12 +836,14 @@ static int list_stash(int argc, const char **argv, const char *prefix)\n>   \treturn run_command(&cp);\n>   }\n> \n>   static int show_stat = 1;\n>   static int show_patch;\n>   static int show_include_untracked;\n> +static int default_keep_index;\n> +static int default_include_untracked;\n> \n>   static int git_stash_config(const char *var, const char *value,\n>   \t\t\t    const struct config_context *ctx, void *cb)\n>   {\n>   \tif (!strcmp(var, \"stash.showstat\")) {\n>   \t\tshow_stat = git_config_bool(var, value);\n> @@ -852,12 +854,20 @@ static int git_stash_config(const char *var, const char *value,\n>   \t\treturn 0;\n>   \t}\n>   \tif (!strcmp(var, \"stash.showincludeuntracked\")) {\n>   \t\tshow_include_untracked = git_config_bool(var, value);\n>   \t\treturn 0;\n>   \t}\n> +\tif (!strcmp(var, \"stash.keepindex\")) {\n> +\t\tdefault_keep_index = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\tif (!strcmp(var, \"stash.includeuntracked\")) {\n> +\t\tdefault_include_untracked = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n>   \treturn git_diff_basic_config(var, value, ctx, cb);\n>   }\n> \n>   static void diff_include_untracked(const struct stash_info *info, struct diff_options *diff_opt)\n>   {\n>   \tconst struct object_id *oid[] = { &info->w_commit, &info->u_tree };\n> @@ -1509,33 +1519,50 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>   \tint ret = 0;\n>   \tstruct stash_info info = STASH_INFO_INIT;\n>   \tstruct strbuf patch = STRBUF_INIT;\n>   \tstruct strbuf stash_msg_buf = STRBUF_INIT;\n>   \tstruct strbuf untracked_files = STRBUF_INIT;\n> \n> -\tif (patch_mode && keep_index == -1)\n> -\t\tkeep_index = 1;\n> -\n> -\tif (patch_mode && include_untracked) {\n> -\t\tfprintf_ln(stderr, _(\"Can't use --patch and --include-untracked\"\n> -\t\t\t\t     \" or --all at the same time\"));\n> -\t\tret = -1;\n> -\t\tgoto done;\n> +\tif (keep_index == -1) {\n> +\t\tif (patch_mode)\n> +\t\t\tkeep_index = 1;\n> +\t\telse\n> +\t\t\tkeep_index = default_keep_index;\n>   \t}\n> \n> -\t/* --patch overrides --staged */\n> -\tif (patch_mode)\n> +\tif (patch_mode) {\n> +\t\tif (include_untracked == -1)\n> +\t\t\tinclude_untracked = 0;\n> +\t\telse if (include_untracked) {\n> +\t\t\tfprintf_ln(stderr,\n> +\t\t\t\t   _(\"Can't use --patch and --include-untracked\"\n> +\t\t\t\t     \" or --all at the same time\"));\n> +\t\t\tret = -1;\n> +\t\t\tgoto done;\n> +\t\t}\n> +\n> +\t\t/* --patch overrides --staged */\n>   \t\tonly_staged = 0;\n> -\n> -\tif (only_staged && include_untracked) {\n> -\t\tfprintf_ln(stderr, _(\"Can't use --staged and --include-untracked\"\n> -\t\t\t\t     \" or --all at the same time\"));\n> -\t\tret = -1;\n> -\t\tgoto done;\n>   \t}\n> \n> +\tif (only_staged) {\n> +\t\tif (include_untracked == -1)\n> +\t\t\tinclude_untracked = 0;\n> +\t\telse if (include_untracked) {\n> +\t\t\tfprintf_ln(\n> +\t\t\t\tstderr,\n> +\t\t\t\t_(\"Can't use --staged and --include-untracked\"\n> +\t\t\t\t  \" or --all at the same time\"));\n> +\t\t\tret = -1;\n> +\t\t\tgoto done;\n> +\t\t}\n> +\t}\n> +\n> +\tif (include_untracked == -1)\n> +\t\tinclude_untracked = default_include_untracked;\n> +\n>   \trepo_read_index_preload(the_repository, NULL, 0);\n>   \tif (!include_untracked && ps->nr) {\n>   \t\tint i;\n>   \t\tchar *ps_matched = xcalloc(ps->nr, 1);\n> \n>   \t\t/* TODO: audit for interaction with sparse-index. */\n> @@ -1688,13 +1715,13 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>   \t\t\t\tfprintf_ln(stderr, _(\"Cannot remove \"\n>   \t\t\t\t\t\t     \"worktree changes\"));\n>   \t\t\tret = -1;\n>   \t\t\tgoto done;\n>   \t\t}\n> \n> -\t\tif (keep_index < 1) {\n> +\t\tif (!keep_index) {\n>   \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> \n>   \t\t\tcp.git_cmd = 1;\n>   \t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--refresh\", \"--\",\n>   \t\t\t\t     NULL);\n>   \t\t\tadd_pathspecs(&cp.args, ps);\n> @@ -1718,13 +1745,13 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n>   \t\t      int push_assumed)\n>   {\n>   \tint force_assume = 0;\n>   \tint keep_index = -1;\n>   \tint only_staged = 0;\n>   \tint patch_mode = 0;\n> -\tint include_untracked = 0;\n> +\tint include_untracked = -1;\n>   \tint quiet = 0;\n>   \tint pathspec_file_nul = 0;\n>   \tconst char *stash_msg = NULL;\n>   \tconst char *pathspec_from_file = NULL;\n>   \tstruct pathspec ps;\n>   \tstruct option options[] = {\n> @@ -1798,13 +1825,13 @@ static int push_stash_unassumed(int argc, const char **argv, const char *prefix)\n> \n>   static int save_stash(int argc, const char **argv, const char *prefix)\n>   {\n>   \tint keep_index = -1;\n>   \tint only_staged = 0;\n>   \tint patch_mode = 0;\n> -\tint include_untracked = 0;\n> +\tint include_untracked = -1;\n>   \tint quiet = 0;\n>   \tint ret = 0;\n>   \tconst char *stash_msg = NULL;\n>   \tstruct pathspec ps;\n>   \tstruct strbuf stash_msg_buf = STRBUF_INIT;\n>   \tstruct option options[] = {\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 00db82fb24..4ffcca742c 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1565,7 +1565,126 @@ test_expect_success 'stash apply reports a locked index' '\n>   \t\tEOF\n>   \t\ttest_must_fail git stash apply 2>err &&\n>   \t\ttest_cmp expect err\n>   \t)\n>   '\n> \n> +setup_dirty() {\n> +\techo a >tracked &&\n> +\techo b >added-modified &&\n> +\tgit add tracked added-modified &&\n> +\tgit commit -m initial &&\n> +\techo 1 >>tracked &&\n> +\techo 2 >>added-modified &&\n> +\techo c >added-new &&\n> +\techo d >untracked &&\n> +\tgit add added-modified added-new\n> +}\n> +\n> +test_expect_success 'stash.includeuntracked equivalent to --include-untracked' '\n> +\tgit init using_opt &&\n> +\ttest_when_finished rm -rf using_opt &&\n> +\t(\n> +\t\tcd using_opt &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push &&\n> +\t\tgit stash show -u --patch >../using-opt\n> +\t) &&\n> +\n> +\ttest_config stash.includeuntracked true &&\n> +\tgit init using_config &&\n> +\ttest_when_finished rm -rf using_config &&\n> +\t(\n> +\t\tcd using_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push &&\n> +\t\tgit stash show -u --patch >../using-config\n> +\t) &&\n> +\n> +\ttest_cmp using-opt using-config\n> +'\n> +\n> +test_expect_success 'stash.includeuntracked yields to --no-include-untracked' '\n> +\tgit init no_config &&\n> +\ttest_when_finished rm -rf no_config &&\n> +\t(\n> +\t\tcd no_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push --no-include-untracked &&\n> +\t\tgit stash show -u --patch >../no-config\n> +\t) &&\n> +\n> +\ttest_config stash.includeuntracked true &&\n> +\tgit init using_config &&\n> +\ttest_when_finished rm -rf using_config &&\n> +\t(\n> +\t\tcd using_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push --no-include-untracked &&\n> +\t\tgit stash show -u --patch >../using-config\n> +\t) &&\n> +\n> +\ttest_cmp no-config using-config\n> +'\n> +\n> +test_expect_success 'stash.includeuntracked succeeds with --patch' '\n> +\ttest_config stash.includeuntracked true &&\n> +\tgit stash --patch\n> +'\n> +\n> +test_expect_success 'stash.includeuntracked succeeds with --staged' '\n> +\ttest_config stash.includeuntracked true &&\n> +\tgit stash --staged\n> +'\n> +\n> +test_expect_success 'stash.keepindex equivalent to --keep-index' '\n> +\tgit init using_opt &&\n> +\ttest_when_finished rm -rf using_opt &&\n> +\t(\n> +\t\tcd using_opt &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push &&\n> +\t\tgit stash show -u --patch >../using-opt\n> +\t) &&\n> +\n> +\ttest_config stash.keepindex true &&\n> +\tgit init using_config &&\n> +\ttest_when_finished rm -rf using_config &&\n> +\t(\n> +\t\tcd using_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push &&\n> +\t\tgit stash show -u --patch >../using-config\n> +\t) &&\n> +\n> +\ttest_cmp using-opt using-config\n> +'\n> +\n> +test_expect_success 'stash.keepindex yields to --no-keep-index' '\n> +\tgit init no_config &&\n> +\ttest_when_finished rm -rf no_config &&\n> +\t(\n> +\t\tcd no_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push --no-keep-index &&\n> +\t\tgit stash show -u --patch >../no-config\n> +\t) &&\n> +\n> +\ttest_config stash.keepindex true &&\n> +\tgit init using_config &&\n> +\ttest_when_finished rm -rf using_config &&\n> +\t(\n> +\t\tcd using_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push --no-keep-index &&\n> +\t\tgit stash show -u --patch >../using-config\n> +\t) &&\n> +\n> +\ttest_cmp no-config using-config\n> +'\n> +\n> +test_expect_success 'stash.keepindex succeeds with --patch' '\n> +\ttest_config stash.keepindex true &&\n> +\tgit stash --patch\n> +'\n> +\n>   test_done\n> --\n> 2.43.2\n> \n"},{"id":"488877","messageId":"df020dcc-afe8-4dd8-b476-4ca032206214@gmail.com","threadId":"60945","inReplyTo":"99346639-5a36-4c2e-a5d7-035c3c1fda8b@gmail.com","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Ricardo C","fromEmail":"rpc01234@gmail.com","sentAt":"2024-02-18T17:54:48Z","receivedAt":"2024-02-18T17:54:50Z","isPatch":true,"sender":{"key":"rpc01234@gmail.com","avatar":null},"body":"Hi Phillip,\n\nOn 2/18/24 05:32, Phillip Wood wrote:\n> How does \"stash.keepIndex\" interact with \"git rebase --autostash\" and \"git \n> merge --autostash\"? I think both those commands expect a clean index after \n> running \"git stash\". They could just override the config setting but it might \n> get a bit confusing if some commands respect the config and others don't.\n\nBoth `git rebase --autostash` and `git merge --autostash` seem to be hardcoded \nto clean the index, regardless of the configuration or CLI flags. They do not \nuse regular `git stash` to do so, but rather `git stash create`. This is \nunaffected by my changes, since it follows a different code path and does not \naccept `--keep-index` nor `--include-untracked`. I'll add some tests for `git \nrebase --autostash` and `git merge --autostash`, just in case this behavior is \nchanged in the future and causes breakage.\n\n> I've only given the patch a very quick scan, but it looked sensible. The only \n> thing that jumped out at me was that quite a few tests seem to do\n> \n>      git init repo &&\n>      (\n>          cd repo &&\n>          # test things\n>      ) &&\n> \n> Our normal practice is to run all the tests in the same file in the same \n> repository rather than setting up a new one each time.\n\nI was doing this because it makes comparing different commands easier, but \nlooking through other tests again, it seems like I should be comparing the \noutputs to hardcoded files anyway.\n\nThank you,\n\nRicardo\n"},{"id":"488906","messageId":"fc7a8c46-61e4-4b5d-b625-cbc845b81590@gmail.com","threadId":"60945","inReplyTo":"20240218033146.372727-2-rpc01234@gmail.com","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Jean-Noël Avila","fromEmail":"avila.jn@gmail.com","sentAt":"2024-02-19T08:04:28Z","receivedAt":"2024-02-19T08:04:32Z","isPatch":true,"sender":{"key":"jn.avila@free.fr","avatar":"https://avatars.githubusercontent.com/u/156172?v=4"},"body":"Hello,\n\nLe 18/02/2024 à 04:30, MithicSpirit a écrit :\n> Adds options `stash.keepIndex` and `stash.includeUntracked`, which\n> enable `--keep-index` and `--include-untracked` by default (unless it\n> would conflict with another option). This is done to facilitate the\n> workflow where a user:\n> \n> . Makes modifications;\n> . Selectively adds them with `git add -p`; and\n> . Stashes the unadded changes to run tests with only the current\n>    modifications.\n> \n> `stash.keepIndex` is especially important for this, as otherwise the\n> work done in `git add -p` is lost since applying the stash does not\n> restore the index. This problem could also be solved by adding\n> functionality to the stash to restore the index specifically, but this\n> would create much more complexity and still wouldn't be as convenient\n> for that workflow.\n> \n> Signed-off-by: Ricardo Prado Cunha (MithicSpirit) <rpc01234@gmail.com>\n> ---\n> This is my first patch to git and my first time using mailing lists for this\n> kind of stuff. Please do let me know of any mistakes or gaffes I've made.\n> \n>   Documentation/config/stash.txt |  13 ++++\n>   builtin/stash.c                |  65 ++++++++++++------\n>   t/t3903-stash.sh               | 119 +++++++++++++++++++++++++++++++++\n>   3 files changed, 178 insertions(+), 19 deletions(-)\n> \n> diff --git a/Documentation/config/stash.txt b/Documentation/config/stash.txt\n> index ec1edaeba6..d6d9ea7daa 100644\n> --- a/Documentation/config/stash.txt\n> +++ b/Documentation/config/stash.txt\n> @@ -1,6 +1,19 @@\n> +stash.includeUntracked::\n> +\tBoolean option that configures whether the `git stash push` and `git\n> +\tstash save` commands also stash untracked files by default. This option\n> +\tis ignored if `--include-untracked`, `--no-include-untracked`, `--all`,\n> +\t`--patch`, or `--staged` are used. Defaults to `false`. See\n> +\tlinkgit:git-stash[1].\n> +\n> +stash.keepIndex::\n> +\tBoolean option that configures whether the `git stash push` and `git\n> +\tstash save` commands also stash changes that have been added to the\n> +\tindex. This option is ignored if `--keep-index`, `--no-keep-index`, or\n> +\t`--patch` are used. Defaults to `false`. See linkgit:git-stash[1].\n> +\n>   stash.showIncludeUntracked::\n>   \tIf this is set to true, the `git stash show` command will show\n>   \tthe untracked files of a stash entry.  Defaults to false. See\n>   \tthe description of the 'show' command in linkgit:git-stash[1].\n> \n>   stash.showPatch::\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 7fb355bff0..c591a2bf4b 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -836,12 +836,14 @@ static int list_stash(int argc, const char **argv, const char *prefix)\n>   \treturn run_command(&cp);\n>   }\n> \n>   static int show_stat = 1;\n>   static int show_patch;\n>   static int show_include_untracked;\n> +static int default_keep_index;\n> +static int default_include_untracked;\n> \n>   static int git_stash_config(const char *var, const char *value,\n>   \t\t\t    const struct config_context *ctx, void *cb)\n>   {\n>   \tif (!strcmp(var, \"stash.showstat\")) {\n>   \t\tshow_stat = git_config_bool(var, value);\n> @@ -852,12 +854,20 @@ static int git_stash_config(const char *var, const char *value,\n>   \t\treturn 0;\n>   \t}\n>   \tif (!strcmp(var, \"stash.showincludeuntracked\")) {\n>   \t\tshow_include_untracked = git_config_bool(var, value);\n>   \t\treturn 0;\n>   \t}\n> +\tif (!strcmp(var, \"stash.keepindex\")) {\n> +\t\tdefault_keep_index = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\tif (!strcmp(var, \"stash.includeuntracked\")) {\n> +\t\tdefault_include_untracked = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n>   \treturn git_diff_basic_config(var, value, ctx, cb);\n>   }\n> \n>   static void diff_include_untracked(const struct stash_info *info, struct diff_options *diff_opt)\n>   {\n>   \tconst struct object_id *oid[] = { &info->w_commit, &info->u_tree };\n> @@ -1509,33 +1519,50 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>   \tint ret = 0;\n>   \tstruct stash_info info = STASH_INFO_INIT;\n>   \tstruct strbuf patch = STRBUF_INIT;\n>   \tstruct strbuf stash_msg_buf = STRBUF_INIT;\n>   \tstruct strbuf untracked_files = STRBUF_INIT;\n> \n> -\tif (patch_mode && keep_index == -1)\n> -\t\tkeep_index = 1;\n> -\n> -\tif (patch_mode && include_untracked) {\n> -\t\tfprintf_ln(stderr, _(\"Can't use --patch and --include-untracked\"\n> -\t\t\t\t     \" or --all at the same time\"));\n> -\t\tret = -1;\n> -\t\tgoto done;\n> +\tif (keep_index == -1) {\n> +\t\tif (patch_mode)\n> +\t\t\tkeep_index = 1;\n> +\t\telse\n> +\t\t\tkeep_index = default_keep_index;\n>   \t}\n> \n> -\t/* --patch overrides --staged */\n> -\tif (patch_mode)\n> +\tif (patch_mode) {\n> +\t\tif (include_untracked == -1)\n> +\t\t\tinclude_untracked = 0;\n> +\t\telse if (include_untracked) {\n> +\t\t\tfprintf_ln(stderr,\n> +\t\t\t\t   _(\"Can't use --patch and --include-untracked\"\n> +\t\t\t\t     \" or --all at the same time\"));\n> +\t\t\tret = -1;\n> +\t\t\tgoto done;\n> +\t\t}\n> +\n> +\t\t/* --patch overrides --staged */\n>   \t\tonly_staged = 0;\n> -\n> -\tif (only_staged && include_untracked) {\n> -\t\tfprintf_ln(stderr, _(\"Can't use --staged and --include-untracked\"\n> -\t\t\t\t     \" or --all at the same time\"));\n> -\t\tret = -1;\n> -\t\tgoto done;\n>   \t}\n> \n> +\tif (only_staged) {\n> +\t\tif (include_untracked == -1)\n> +\t\t\tinclude_untracked = 0;\n> +\t\telse if (include_untracked) {\n> +\t\t\tfprintf_ln(\n> +\t\t\t\tstderr,\n> +\t\t\t\t_(\"Can't use --staged and --include-untracked\"\n> +\t\t\t\t  \" or --all at the same time\"));\n> +\t\t\tret = -1;\n> +\t\t\tgoto done;\n> +\t\t}\n> +\t}\n> +\n> +\tif (include_untracked == -1)\n> +\t\tinclude_untracked = default_include_untracked;\n> +\n\nI'm not sure this would be better, but instead of mixing option \ncompatibility and actually building your logic, why not use a series of\ndie_for_incompatible_opt3 and the like in order to clear the option \nlists, then build your action logic without resorting to special values?\n\n\n>   \trepo_read_index_preload(the_repository, NULL, 0);\n>   \tif (!include_untracked && ps->nr) {\n>   \t\tint i;\n>   \t\tchar *ps_matched = xcalloc(ps->nr, 1);\n> \n>   \t\t/* TODO: audit for interaction with sparse-index. */\n> @@ -1688,13 +1715,13 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>   \t\t\t\tfprintf_ln(stderr, _(\"Cannot remove \"\n>   \t\t\t\t\t\t     \"worktree changes\"));\n>   \t\t\tret = -1;\n>   \t\t\tgoto done;\n>   \t\t}\n> \n> -\t\tif (keep_index < 1) {\n> +\t\tif (!keep_index) {\n>   \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> \n>   \t\t\tcp.git_cmd = 1;\n>   \t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--refresh\", \"--\",\n>   \t\t\t\t     NULL);\n>   \t\t\tadd_pathspecs(&cp.args, ps);\n> @@ -1718,13 +1745,13 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n>   \t\t      int push_assumed)\n>   {\n>   \tint force_assume = 0;\n>   \tint keep_index = -1;\n>   \tint only_staged = 0;\n>   \tint patch_mode = 0;\n> -\tint include_untracked = 0;\n> +\tint include_untracked = -1;\n>   \tint quiet = 0;\n>   \tint pathspec_file_nul = 0;\n>   \tconst char *stash_msg = NULL;\n>   \tconst char *pathspec_from_file = NULL;\n>   \tstruct pathspec ps;\n>   \tstruct option options[] = {\n> @@ -1798,13 +1825,13 @@ static int push_stash_unassumed(int argc, const char **argv, const char *prefix)\n> \n>   static int save_stash(int argc, const char **argv, const char *prefix)\n>   {\n>   \tint keep_index = -1;\n>   \tint only_staged = 0;\n>   \tint patch_mode = 0;\n> -\tint include_untracked = 0;\n> +\tint include_untracked = -1;\n>   \tint quiet = 0;\n>   \tint ret = 0;\n>   \tconst char *stash_msg = NULL;\n>   \tstruct pathspec ps;\n>   \tstruct strbuf stash_msg_buf = STRBUF_INIT;\n>   \tstruct option options[] = {\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 00db82fb24..4ffcca742c 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1565,7 +1565,126 @@ test_expect_success 'stash apply reports a locked index' '\n>   \t\tEOF\n>   \t\ttest_must_fail git stash apply 2>err &&\n>   \t\ttest_cmp expect err\n>   \t)\n>   '\n> \n> +setup_dirty() {\n> +\techo a >tracked &&\n> +\techo b >added-modified &&\n> +\tgit add tracked added-modified &&\n> +\tgit commit -m initial &&\n> +\techo 1 >>tracked &&\n> +\techo 2 >>added-modified &&\n> +\techo c >added-new &&\n> +\techo d >untracked &&\n> +\tgit add added-modified added-new\n> +}\n> +\n> +test_expect_success 'stash.includeuntracked equivalent to --include-untracked' '\n> +\tgit init using_opt &&\n> +\ttest_when_finished rm -rf using_opt &&\n> +\t(\n> +\t\tcd using_opt &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push &&\n> +\t\tgit stash show -u --patch >../using-opt\n> +\t) &&\n> +\n> +\ttest_config stash.includeuntracked true &&\n> +\tgit init using_config &&\n> +\ttest_when_finished rm -rf using_config &&\n> +\t(\n> +\t\tcd using_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push &&\n> +\t\tgit stash show -u --patch >../using-config\n> +\t) &&\n> +\n> +\ttest_cmp using-opt using-config\n> +'\n> +\n> +test_expect_success 'stash.includeuntracked yields to --no-include-untracked' '\n> +\tgit init no_config &&\n> +\ttest_when_finished rm -rf no_config &&\n> +\t(\n> +\t\tcd no_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push --no-include-untracked &&\n> +\t\tgit stash show -u --patch >../no-config\n> +\t) &&\n> +\n> +\ttest_config stash.includeuntracked true &&\n> +\tgit init using_config &&\n> +\ttest_when_finished rm -rf using_config &&\n> +\t(\n> +\t\tcd using_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push --no-include-untracked &&\n> +\t\tgit stash show -u --patch >../using-config\n> +\t) &&\n> +\n> +\ttest_cmp no-config using-config\n> +'\n> +\n> +test_expect_success 'stash.includeuntracked succeeds with --patch' '\n> +\ttest_config stash.includeuntracked true &&\n> +\tgit stash --patch\n> +'\n> +\n> +test_expect_success 'stash.includeuntracked succeeds with --staged' '\n> +\ttest_config stash.includeuntracked true &&\n> +\tgit stash --staged\n> +'\n> +\n> +test_expect_success 'stash.keepindex equivalent to --keep-index' '\n> +\tgit init using_opt &&\n> +\ttest_when_finished rm -rf using_opt &&\n> +\t(\n> +\t\tcd using_opt &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push &&\n> +\t\tgit stash show -u --patch >../using-opt\n> +\t) &&\n> +\n> +\ttest_config stash.keepindex true &&\n> +\tgit init using_config &&\n> +\ttest_when_finished rm -rf using_config &&\n> +\t(\n> +\t\tcd using_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push &&\n> +\t\tgit stash show -u --patch >../using-config\n> +\t) &&\n> +\n> +\ttest_cmp using-opt using-config\n> +'\n> +\n> +test_expect_success 'stash.keepindex yields to --no-keep-index' '\n> +\tgit init no_config &&\n> +\ttest_when_finished rm -rf no_config &&\n> +\t(\n> +\t\tcd no_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push --no-keep-index &&\n> +\t\tgit stash show -u --patch >../no-config\n> +\t) &&\n> +\n> +\ttest_config stash.keepindex true &&\n> +\tgit init using_config &&\n> +\ttest_when_finished rm -rf using_config &&\n> +\t(\n> +\t\tcd using_config &&\n> +\t\tsetup_dirty &&\n> +\t\tgit stash push --no-keep-index &&\n> +\t\tgit stash show -u --patch >../using-config\n> +\t) &&\n> +\n> +\ttest_cmp no-config using-config\n> +'\n> +\n> +test_expect_success 'stash.keepindex succeeds with --patch' '\n> +\ttest_config stash.keepindex true &&\n> +\tgit stash --patch\n> +'\n> +\n>   test_done\n> --\n> 2.43.2\n> \n\n"},{"id":"488934","messageId":"cdc377ea-d810-49c6-9124-c450aff34118@gmail.com","threadId":"60945","inReplyTo":"fc7a8c46-61e4-4b5d-b625-cbc845b81590@gmail.com","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Ricardo C","fromEmail":"rpc01234@gmail.com","sentAt":"2024-02-19T21:41:25Z","receivedAt":"2024-02-19T21:41:28Z","isPatch":true,"sender":{"key":"rpc01234@gmail.com","avatar":null},"body":"Hello,\n\nOn 2/19/24 03:04, Jean-Noël Avila wrote:\n> I'm not sure this would be better, but instead of mixing option compatibility \n> and actually building your logic, why not use a series of\n> die_for_incompatible_opt3 and the like in order to clear the option lists, \n> then build your action logic without resorting to special values?\n\nI'm not sure dying would be appropriate here, since the original behavior was \nalready to clean up buffers and such and then return an errorful value (-1), \nand I'd rather keep maximum backwards compatibility. Also, the special value \nof -1 for `keep_index` and `include_untracked` is necessary to detect whether \nthe variable was set by the CLI flags or if it is fine to override with the \nconfig value.\n\nThis could be addressed by moving more logic up to `push_stash` and \n`save_stash` (where the arguments are parsed), but this would need much more \nrewriting and would lead to some code duplication, for what I think is minimal \ngain.\n\nThank you,\n\nRicardo\n"},{"id":"488954","messageId":"xmqq34tnyhhf.fsf@gitster.g","threadId":"60945","inReplyTo":"99346639-5a36-4c2e-a5d7-035c3c1fda8b@gmail.com","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T02:52:44Z","receivedAt":"2024-02-20T02:52:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> How does \"stash.keepIndex\" interact with \"git rebase --autostash\" and\n> \"git merge --autostash\"? I think both those commands expect a clean\n> index after running \"git stash\". They could just override the config\n> setting but it might get a bit confusing if some commands respect the\n> config and others don't.\n\nThe \"autostash\" feature fundamentally should not respect such\nconfiguration variables, as the reason why they exist is not to\ncreate a stash of any unusual kind the user expresses preference to\nby the configuration variable(s), but to clear the slate and bring\nboth the working tree and the index pristine with respect to HEAD.\nAlso, when able, they would automatically unstash after the main\noperation to rebase or merge is done, right?  IOW, normally the use\nof the stash ought to be invisible to the end users.  I would not\nworry too much about the \"confusion\" factor for these reasons.\n\nYou are however right that this will confuse the toolchain.  These\ntwo commands we provide may not be affected (or we can make them not\naffected by changing their implementation if needed, while we add\nsuch configuration variables at the same time), but third-party\ntools and end-user scripts that has trusted that when they write\n\"git stash\", the command will give them a clean index and working\ntree will be broken big time.\n\nSo, I am somewhat negative on the patch in the current form, until I\nsee how the plan to help third-party tools and end-user scripts that\nrely on the promise we have given them looks like.\n\nThanks.\n"},{"id":"488958","messageId":"1d66eb0f-077a-4a63-8acf-f383538a41c7@gmail.com","threadId":"60945","inReplyTo":"xmqq34tnyhhf.fsf@gitster.g","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Ricardo C","fromEmail":"rpc01234@gmail.com","sentAt":"2024-02-20T03:30:44Z","receivedAt":"2024-02-20T03:30:49Z","isPatch":true,"sender":{"key":"rpc01234@gmail.com","avatar":null},"body":"Hello Junio,\n\nOn 2/19/24 21:52, Junio C Hamano wrote:\n> You are however right that this will confuse the toolchain.  These\n> two commands we provide may not be affected (or we can make them not\n> affected by changing their implementation if needed, while we add\n> such configuration variables at the same time), but third-party\n> tools and end-user scripts that has trusted that when they write\n> \"git stash\", the command will give them a clean index and working\n> tree will be broken big time.\n> \n> So, I am somewhat negative on the patch in the current form, until I\n> see how the plan to help third-party tools and end-user scripts that\n> rely on the promise we have given them looks like.\n\nThis is an issue I hadn't considered, and I'm not sure whether it can even be \nfixed. In some sense, the entire point of this patch is to allow the user to \nbreak that promise in their configuration. However, I'm not sure how big of a \nproblem this is, as it is entirely opt-in (default behavior should be the same \nas current behavior), and tools can be altered to pass `--no-keep-index \n--no-include-untracked` if they wish to force the current behavior. Either \nway, I would like to address your concern if possible, and I'd appreciate any \nideas on how to do so.\n\nThank you,\n\nRicardo\n"},{"id":"488959","messageId":"xmqq5xyjx0jc.fsf@gitster.g","threadId":"60945","inReplyTo":"1d66eb0f-077a-4a63-8acf-f383538a41c7@gmail.com","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T03:44:07Z","receivedAt":"2024-02-20T03:44:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ricardo C <rpc01234@gmail.com> writes:\n\n> This is an issue I hadn't considered, and I'm not sure whether it can\n> even be fixed. In some sense, the entire point of this patch is to\n> allow the user to break that promise in their configuration. However,\n> I'm not sure how big of a problem this is, as it is entirely opt-in\n> (default behavior should be the same as current behavior),\n\nCorrect.\n\n> and tools\n> can be altered to pass `--no-keep-index --no-include-untracked` if\n> they wish to force the current behavior.\n\nThis is not.\n\nPeople expect a bit better from Git, and such a callous disregard to\nbackward compatibility that breaks other people's tools and scripts\nis a non-starter.  \n\nUsers of such tools, whether they were written by themselves or\nother people, do *not* want them to break only because they want to\nuse a shiny new feature that is advertised in a new version of Git.\n\nThe point of packaging a solution, the reason why they wroute such a\ntool or script that happens to use \"git stash\" as an ingredient and\ndepends on the current behaviour of \"git stash\", is so that they do\nnot need to remember they even used \"git stash\" as a small part of\ntheir solution.  And of course they do not want to remember that\nthey rely on how \"git stash\" behaves in such a solution.  They do\nnot even want to bother complaining loudly when such a change is\nproposed before it hits a release and hurt them.  Saying \"nobody\ncomplained when these configuration variables were proposed\" does\nnot help anybody later, after we already hurt them.\n"},{"id":"488960","messageId":"31135b8a-af7a-4d73-b3b3-d91ea8d3d5fd@gmail.com","threadId":"60945","inReplyTo":"xmqq5xyjx0jc.fsf@gitster.g","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Ricardo C","fromEmail":"rpc01234@gmail.com","sentAt":"2024-02-20T03:59:53Z","receivedAt":"2024-02-20T03:59:57Z","isPatch":true,"sender":{"key":"rpc01234@gmail.com","avatar":null},"body":"\n\nOn 2/19/24 22:44, Junio C Hamano wrote:\n> Ricardo C <rpc01234@gmail.com> writes:\n> \n>> This is an issue I hadn't considered, and I'm not sure whether it can\n>> even be fixed. In some sense, the entire point of this patch is to\n>> allow the user to break that promise in their configuration. However,\n>> I'm not sure how big of a problem this is, as it is entirely opt-in\n>> (default behavior should be the same as current behavior),\n> \n> Correct.\n> \n>> and tools\n>> can be altered to pass `--no-keep-index --no-include-untracked` if\n>> they wish to force the current behavior.\n> \n> This is not.\n> \n> People expect a bit better from Git, and such a callous disregard to\n> backward compatibility that breaks other people's tools and scripts\n> is a non-starter.\n> \n> Users of such tools, whether they were written by themselves or\n> other people, do *not* want them to break only because they want to\n> use a shiny new feature that is advertised in a new version of Git.\n> \n> The point of packaging a solution, the reason why they wroute such a\n> tool or script that happens to use \"git stash\" as an ingredient and\n> depends on the current behaviour of \"git stash\", is so that they do\n> not need to remember they even used \"git stash\" as a small part of\n> their solution.  And of course they do not want to remember that\n> they rely on how \"git stash\" behaves in such a solution.  They do\n> not even want to bother complaining loudly when such a change is\n> proposed before it hits a release and hurt them.  Saying \"nobody\n> complained when these configuration variables were proposed\" does\n> not help anybody later, after we already hurt them.\n\nThat makes sense. Do you have any ideas on how to address this? It feels to me \nlike providing this config option is fundamentally incompatible with requiring \nbackwards-compatible behavior regardless of configuration.\n\nRicardo\n"},{"id":"488984","messageId":"78a8733b-c74a-4017-8905-d29b2e05adb1@gmail.com","threadId":"60945","inReplyTo":"df020dcc-afe8-4dd8-b476-4ca032206214@gmail.com","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-20T11:01:30Z","receivedAt":"2024-02-20T11:01:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ricardo\n\nOn 18/02/2024 17:54, Ricardo C wrote:\n> Hi Phillip,\n> \n> On 2/18/24 05:32, Phillip Wood wrote:\n>> How does \"stash.keepIndex\" interact with \"git rebase --autostash\" and \n>> \"git merge --autostash\"? I think both those commands expect a clean \n>> index after running \"git stash\". They could just override the config \n>> setting but it might get a bit confusing if some commands respect the \n>> config and others don't.\n> \n> Both `git rebase --autostash` and `git merge --autostash` seem to be \n> hardcoded to clean the index, regardless of the configuration or CLI \n> flags. They do not use regular `git stash` to do so, but rather `git \n> stash create`. This is unaffected by my changes, since it follows a \n> different code path and does not accept `--keep-index` nor \n> `--include-untracked`.\n\nAh, I'd forgotten that we hand \"git stash create\" as well as \"git stash \npush\". It's good that these changes do not affect \"git stash create\" but \nI think Junio is right to be concerned about the effect on tools  that \nare using \"git stash push\" where they arguably ought to be using \"git \nstash create\".\n\n> I'll add some tests for `git rebase --autostash` \n> and `git merge --autostash`, just in case this behavior is changed in \n> the future and causes breakage.\n\nThat's a good idea but I think it would be better to test that \"git \nstash create\" is not affected by the config as we don't want to change \nthe behavior of its behavior.\n\n>> I've only given the patch a very quick scan, but it looked sensible. \n>> The only thing that jumped out at me was that quite a few tests seem \n>> to do\n>>\n>>      git init repo &&\n>>      (\n>>          cd repo &&\n>>          # test things\n>>      ) &&\n>>\n>> Our normal practice is to run all the tests in the same file in the \n>> same repository rather than setting up a new one each time.\n> \n> I was doing this because it makes comparing different commands easier, \n> but looking through other tests again, it seems like I should be \n> comparing the outputs to hardcoded files anyway.\n\nYes, that is our usual practice.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"489001","messageId":"xmqqjzmzqb85.fsf@gitster.g","threadId":"60945","inReplyTo":"78a8733b-c74a-4017-8905-d29b2e05adb1@gmail.com","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T17:47:22Z","receivedAt":"2024-02-20T17:47:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> That's a good idea but I think it would be better to test that \"git\n> stash create\" is not affected by the config as we don't want to change\n> the behavior of its behavior.\n\nYup.  Making sure \"stash create\" stays the same is a very good\nthing.  Thanks for suggesting it.\n\nRegardling the need to have an escape hatch that is well publicized\nlong before a potentially harmful switch can safely happen, one way\nout might be to\n\n - Declare \"git stash create\" is a plumbing, but the rest is\n   considered Porcelain.  Publicize it well and wait for the word to\n   spread out.  Folks who are in favor of adding these configuration\n   variables to the system may need to do some legwork, spreading\n   the word around at stackoverflow and various other venues,\n   scouring public repositories at GitHub and other hosting sites\n   and studying third-party uses of \"git stash\" that should be\n   replaced with \"git stash create\" followed by \"git update-ref\",\n   and raising issues to notify the owners of such projects, etc.\n\n - Add breaking configuration variables after a few years.\n\nWe cannot do the usual \"start warning when we detect problematic use\nwithout changing the current behaviour, wait, and then switch\" dance\nin this case, unfortunately, because we lack a good way to detect a\n\"problematic use\".  We cannot tell a direct use of \"git stash\" via\nthe command prompt (which may want to honor the configuration) from\nan IDE running \"git stash\" reacting to the \"[Stash Now]\" UI element\n(which might want to honor the configuration) and from a third-party\ntool that acts like \"rebase --autostash\" that wants to save away all\nlocal changes to clear the slate to do its thing (which we should\ndefinitely not interfare).  If we could do so reliably, then we\nwouldn't be having this discussion---we'd be using the same logic as\nwe would use to tell when to warn and instead of warning, just silently\nrefraining to honor the configuration variables.\n\nAn eve easier way out of course is not to do these variables, of course.\n\nFWIW, I can see how permanently enabling \"include untracked\" may be\nOK in a workflow, but I cannot see the utility of permanently\nenabling \"keepindex\" at all.  Sometimes I'd want to clear the slate\nso that I can try building and testing what I added to the index and\nthat is why I want to use the option.  But a lot of other times, I\nwant to clear the slate to go back to the very pristine state that\nis equal to the HEAD.  As the need to switch between the two modes\nhappens quite often, the way to defeat configured stash.keepindex\ntakes quite many keystrokes, and in general I think the regular\nstashing happens far more often than keepindex stashing, I'd find\nthat using shorthand \"-k\" on the command line occasionally without\nhaving this configuration variable would be what people would end up\nusing anyway.\n\nAnd there is always \"[alias] stk = stash -k\" available ;-)\n"},{"id":"489016","messageId":"14a10788-37fb-46ad-91f4-303703dbd312@app.fastmail.com","threadId":"60945","inReplyTo":"31135b8a-af7a-4d73-b3b3-d91ea8d3d5fd@gmail.com","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-20T19:30:03Z","receivedAt":"2024-02-20T19:31:01Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Feb 20, 2024, at 04:59, Ricardo C wrote:\n> That makes sense. Do you have any ideas on how to address this? It feels to me\n> like providing this config option is fundamentally incompatible with requiring\n> backwards-compatible behavior regardless of configuration.\n\nIf my armchair also had a time machine: one could have used an `-s`\noption to `git` (`git -s stash`) that only reads a few configuration\nvariables, like `user.name` and `user.email`. It would read none of the\nvariables meant for making interactive/custom use convenient. This would\nbe meant for scripts. That way new user-convenience config variables\nwouldn’t interfere with some `git -s` invocation deep in some script\nsomewhere.\n\nAs for the status quo though one seems stuck.\n"},{"id":"489105","messageId":"b84e77a1-93e4-4de0-a0cd-4d76a5691a9b@gmail.com","threadId":"60945","inReplyTo":"xmqqjzmzqb85.fsf@gitster.g","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Ricardo C","fromEmail":"rpc01234@gmail.com","sentAt":"2024-02-21T19:13:27Z","receivedAt":"2024-02-21T19:13:29Z","isPatch":true,"sender":{"key":"rpc01234@gmail.com","avatar":null},"body":"On 2/20/24 12:47, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> That's a good idea but I think it would be better to test that \"git\n>> stash create\" is not affected by the config as we don't want to change\n>> the behavior of its behavior.\n> \n> Yup.  Making sure \"stash create\" stays the same is a very good\n> thing.  Thanks for suggesting it.\n\nI agree. I'll make sure to add testing for it if/when we decide to go through \nwith this change.\n\n> Regardling the need to have an escape hatch that is well publicized\n> long before a potentially harmful switch can safely happen, one way\n> out might be to\n> \n>   - Declare \"git stash create\" is a plumbing, but the rest is\n>     considered Porcelain.  Publicize it well and wait for the word to\n>     spread out.  Folks who are in favor of adding these configuration\n>     variables to the system may need to do some legwork, spreading\n>     the word around at stackoverflow and various other venues,\n>     scouring public repositories at GitHub and other hosting sites\n>     and studying third-party uses of \"git stash\" that should be\n>     replaced with \"git stash create\" followed by \"git update-ref\",\n>     and raising issues to notify the owners of such projects, etc.\n> \n>   - Add breaking configuration variables after a few year\nThis feels like quite a lot of work. I guess that the main question now is \nwhether this change is worth the effort. What would publicizing this look \nlike? Updating documentation? Sending out an announcement?\n\nI can also imagine some tools would dislike having to switch to the more \nmanual stash+update-ref, so maybe easier plumbing should be added?\n\n> FWIW, I can see how permanently enabling \"include untracked\" may be\n> OK in a workflow, but I cannot see the utility of permanently\n> enabling \"keepindex\" at all.  Sometimes I'd want to clear the slate\n> so that I can try building and testing what I added to the index and\n> that is why I want to use the option.  But a lot of other times, I\n> want to clear the slate to go back to the very pristine state that\n> is equal to the HEAD.  As the need to switch between the two modes\n> happens quite often, the way to defeat configured stash.keepindex\n> takes quite many keystrokes, and in general I think the regular\n> stashing happens far more often than keepindex stashing, I'd find\n> that using shorthand \"-k\" on the command line occasionally without\n> having this configuration variable would be what people would end up\n> using anyway.\n\nPermanently enabling keepIndex is mainly intended for people that like to \nstash their unstaged changes before committing (e.g., for testing them \nindependently of other changes). The main issue with what you recommend is \nthat, if they forget to use `-k`, then the entire state of the index is lost, \nwhich is especially problematic if the changes were interactively staged. A \nreport of someone having issues with this workflow is here[1].\n\nPerhaps a better solution would be to provide some mechanism by which to also \nsave the state of the index when stashing changes, and also restore it when \napplying the stash. I figure this change in behavior would be much less \nproblematic.\n\nAnother option would be to go closer to the route of `git switch` and `git \nrestore`, where a separate command with a more user-friendly interface is \nadded. Then all of `git stash` could be kept as plumbing, and this new command \nwould be entirely porcelain. I don't think that this change would be worth \nintroducing a new command for, but if there are other changes that could also \nbe included, then it might make sense.\n\n> And there is always \"[alias] stk = stash -k\" available ;-)\n\nYeah, currently I use a shell alias for `git stash -ku`, but that doesn't help \nwhen I'm using other git frontends.\n\n\n[1] https://bsd.network/@ed1conf/111783574839749798\n"},{"id":"489106","messageId":"xmqq4je1o9yu.fsf@gitster.g","threadId":"60945","inReplyTo":"b84e77a1-93e4-4de0-a0cd-4d76a5691a9b@gmail.com","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-21T20:09:45Z","receivedAt":"2024-02-21T20:09:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ricardo C <rpc01234@gmail.com> writes:\n\n> Permanently enabling keepIndex is mainly intended for people that like\n> to stash their unstaged changes before committing (e.g., for testing\n> them independently of other changes). The main issue with what you\n> recommend is that, if they forget to use `-k`, then the entire state\n> of the index is lost, which is especially problematic if the changes\n> were interactively staged.\n\nDoesn't \"git stash pop --index\" meant to recover from such a\nmistake, though?  If you stash, because your \"git commit\" notices\nthere is no change after you did \"git stash\" without \"-k\", your\nrecovery to \"pop --index\" would apply the changes to the index and\nto the working tree on top of exactly the same commit, so there is\nno risk of losing any changes by doing so, right?  IOW, such a pop\nwill always round-trip.\n\n"},{"id":"489115","messageId":"ec72c1f0-bcac-4982-bb67-cdbc8c47eafe@gmail.com","threadId":"60945","inReplyTo":"xmqq4je1o9yu.fsf@gitster.g","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Ricardo C","fromEmail":"rpc01234@gmail.com","sentAt":"2024-02-21T23:14:18Z","receivedAt":"2024-02-21T23:14:20Z","isPatch":true,"sender":{"key":"rpc01234@gmail.com","avatar":null},"body":"On 2/21/24 15:09, Junio C Hamano wrote:\n> Ricardo C <rpc01234@gmail.com> writes:\n> \n>> Permanently enabling keepIndex is mainly intended for people that like\n>> to stash their unstaged changes before committing (e.g., for testing\n>> them independently of other changes). The main issue with what you\n>> recommend is that, if they forget to use `-k`, then the entire state\n>> of the index is lost, which is especially problematic if the changes\n>> were interactively staged.\n> \n> Doesn't \"git stash pop --index\" meant to recover from such a\n> mistake, though?  If you stash, because your \"git commit\" notices\n> there is no change after you did \"git stash\" without \"-k\", your\n> recovery to \"pop --index\" would apply the changes to the index and\n> to the working tree on top of exactly the same commit, so there is\n> no risk of losing any changes by doing so, right?  IOW, such a pop\n> will always round-trip.\n\nOh, wow, I didn't realize that was a thing (I mistakenly interpreted the \ndocumentation to mean that it added the entire stash to the index). This does \nhelp a lot, thanks! I guess keepIndex is a lot less important, so this should \nmainly be about includeUntracked... but now I'm also longing for a config to \nhave --index on pop/apply by default :P\n\nInterestingly, it seems like frontend support for --index is very variable. \nVScode is probably the most popular frontend and doesn't seem to have any \nsupport for it, while more niche frontends like Emacs Magit and vim-fugitive \ndo support it and moreover have it as the default. Maybe the real issue here \nis lack of awareness of that flag, and the solution is just to spread the word \nabout it.\n\nThanks again,\n\nRicardo\n"},{"id":"489117","messageId":"xmqqzfvtjrvs.fsf@gitster.g","threadId":"60945","inReplyTo":"xmqq4je1o9yu.fsf@gitster.g","subject":"Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-21T23:53:59Z","receivedAt":"2024-02-21T23:54:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Ricardo C <rpc01234@gmail.com> writes:\n>\n>> Permanently enabling keepIndex is mainly intended for people that like\n>> to stash their unstaged changes before committing (e.g., for testing\n>> them independently of other changes). The main issue with what you\n>> recommend is that, if they forget to use `-k`, then the entire state\n>> of the index is lost, which is especially problematic if the changes\n>> were interactively staged.\n>\n> Doesn't \"git stash pop --index\" meant to recover from such a\n> mistake, though?  If you stash, because your \"git commit\" notices\n> there is no change after you did \"git stash\" without \"-k\", your\n> recovery to \"pop --index\" would apply the changes to the index and\n> to the working tree on top of exactly the same commit, so there is\n> no risk of losing any changes by doing so, right?  IOW, such a pop\n> will always round-trip.\n\nActually, \"git commit\" gets into the picture of making and\nrecovering from such a mistake a bit more costly than I made it\nsound in the above.  My bad.\n\nThe common sequence is\n\n    $ edit edit edit\n    $ git add -p\n    $ git stash -k\n    $ build what is in the index and test\n\nand then when you are happy, conclude it with\n\n    $ git commit [NO -o/-i/pathspec]\n\nfollowed by\n\n    $ git stash pop\n\nto continue for the next commit.  So \"git commit\" should notice if\nyour earlier \"stash -k\" forgot to say \"-k\", but by that time, you\nwould have wasted the whole build and test cycle.  The HEAD wouldn't\nhave moved, so the conclusion that \"pop --index\" would be a good way\nto recover from \"stash\" without \"--keep\" does not change, though.\n\n"}]}