# [PATCH 0/9] make stash apply with --index by default 38 messages from 2025-05-10 to 2025-10-14. Participants: D. Ben Knoble, Junio C Hamano, Phillip Wood, Kristoffer Haugsbakk, Ben Knoble. Thread: https://gitlist.dev/t/63436 ## D. Ben Knoble, 2025-05-10 18:33 Subject: [PATCH 0/9] make stash apply with --index by default Message-ID: <20250510183358.36806-1-ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/20250510183358.36806-1-ben.knoble%2Bgithub%40gmail.com ``` Since git-stash's inception, it has defaulted to stashing the index but not restoring it. This has caused some confusion: change the default as part of Git 3.0 to unstash the index, too. I ran into this myself a while back and did some digging; it appears that other have been bit, too (see references in patch 3). Moreover, when git-stash was originally written, defaulting with --index was suggested but not implemented before the script merged. So this RFC should bring us back towards "less confusing," hopefully. The series is structured as follows: Patches 1-2: unrelated prep/style commits noticed while working on tests. Patch 3: update Documentation/BreakingChanges.adoc. This seems like a natural place to discuss the proposal, so it contains no other changes. Patch 4: make it so in builtin/stash.c. Patches 5-9: update the impacted tests. Separated out for ease of review. I used a style more like "split the test into 2: one with, one without breaking changes." In retrospect, the diff might be smaller (and the 2 versions of the test easier to compare) if I used "test_has_prereq" in the tests… but at the cost of making the tests harder to follow. Thoughts? D. Ben Knoble (9): t3903: reduce dependencies on previous tests t3905: remove unneeded blank line BreakingChanges: announce stash {apply,pop} will imply --index stash: restore the index by default when breaking changes are enabled t0450: mark stash documentation as a known discrepancy t3903: adjust stash test to account for --[no-]index with breaking changes t3904: adjust stash -p test to account for index states with breaking changes t3905: adjust stash -u tests for breaking changes t3906: adjust stash submodule tests to account for breaking changes Documentation/BreakingChanges.adoc | 11 ++ Documentation/git-stash.adoc | 6 ++ builtin/stash.c | 38 +++++++ t/lib-submodule-update.sh | 24 ++++- t/t0450/adoc-help-mismatches | 1 + t/t3903-stash.sh | 161 +++++++++++++++++++++++++++-- t/t3904-stash-patch.sh | 14 ++- t/t3905-stash-include-untracked.sh | 40 ++++++- 8 files changed, 276 insertions(+), 19 deletions(-) base-commit: 1ee85f0e215f22b0878d0ad4b2445d12bbb63887 -- 2.48.1 ``` ## D. Ben Knoble, 2025-05-10 18:33 Subject: [PATCH 1/9] t3903: reduce dependencies on previous tests Message-ID: <20250510183358.36806-2-ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/20250510183358.36806-2-ben.knoble%2Bgithub%40gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` Skipping previous tests to work through only failing tests with arguments like --run=4,122- causes some tests to fail because subdir doesn't exist yet (it is created by a previous test; typically "unstashing in a subdirectory"). Create it on demand for tests that need it, but don't fail (-p) if the directory already exists. Signed-off-by: D. Ben Knoble --- t/t3903-stash.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 74666ff3e4..b8936a653b 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -895,6 +895,7 @@ setup_stash() test_expect_success 'apply: show same status as git status (relative to ./)' ' git stash clear && + mkdir -p subdir && echo 1 >subdir/subfile1 && echo 2 >subdir/subfile2 && git add subdir/subfile1 && @@ -1327,6 +1328,7 @@ setup_stash() test_expect_success 'stash -- leaves untracked files in subdir intact' ' git reset && + mkdir -p subdir && >subdir/untracked && >subdir/tracked1 && >subdir/tracked2 && @@ -1343,6 +1345,7 @@ setup_stash() test_expect_success 'stash -- works with binary files' ' git reset && + mkdir -p subdir && >subdir/untracked && >subdir/tracked && cp "$TEST_DIRECTORY"/test-binary-1.png subdir/tracked-binary && -- 2.48.1 ``` ## D. Ben Knoble, 2025-05-10 18:33 Subject: [PATCH 2/9] t3905: remove unneeded blank line Message-ID: <20250510183358.36806-3-ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/20250510183358.36806-3-ben.knoble%2Bgithub%40gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` This is leftover from 787513027a (stash: Add --include-untracked option to stash and remove all untracked files, 2011-06-24) when it was converted in bbaa45c3aa (t3905: move all commands into test cases, 2021-02-08). Signed-off-by: D. Ben Knoble --- t/t3905-stash-include-untracked.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh index 1289ae3e07..7704709054 100755 --- a/t/t3905-stash-include-untracked.sh +++ b/t/t3905-stash-include-untracked.sh @@ -87,7 +87,6 @@ test_expect_success 'clean up untracked/untracked file to prepare for next tests' ' git clean --force --quiet - ' test_expect_success 'stash pop after save --include-untracked leaves files untracked again' ' -- 2.48.1 ``` ## D. Ben Knoble, 2025-05-10 18:33 Subject: [PATCH 3/9] BreakingChanges: announce stash {apply,pop} will imply --index Message-ID: <20250510183358.36806-4-ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/20250510183358.36806-4-ben.knoble%2Bgithub%40gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` Some uses may now require --no-index, but remove a footgun that has bit users over the years where stash {apply,pop} are not the opposite of stash push because they drop the (saved) index. Signed-off-by: D. Ben Knoble --- Notes: Dscho/Junio suggested it in the original thread [1], but it wasn't considered for the release I believe [2]. [1]: https://lore.kernel.org/git/Pine.LNX.4.64.0707021213350.4438@racer.site/ [2]: https://lore.kernel.org/git/7vzm20q1l7.fsf_-_@assigned-by-dhcp.cox.net/ Documentation/BreakingChanges.adoc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc index 61bdd586b9..798e742267 100644 --- a/Documentation/BreakingChanges.adoc +++ b/Documentation/BreakingChanges.adoc @@ -118,6 +118,17 @@ Cf. <2f5de416-04ba-c23d-1e0b-83bb655829a7@zombino.com>, <20170223155046.e7nxivfwqqoprsqj@LykOS.localdomain>, . +* The git-stash(1) command now tries to reinstate the index by default in + the "apply" and "pop" modes. Not doing so creates a common trap: "git stash + apply" is not the reverse of "git stash push" because carefully staged indices + are lost and have to be manually recreated. ++ +Now git-stash(1) will behave like "--index" was given in the "apply" and "pop" +modes. Use "--no-index" to disable this behavior. ++ +Cf. , +. + === Removals * Support for grafting commits has long been superseded by git-replace(1). -- 2.48.1 ``` ## D. Ben Knoble, 2025-05-10 18:33 Subject: [PATCH 4/9] stash: restore the index by default when breaking changes are enabled Message-ID: <20250510183358.36806-5-ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/20250510183358.36806-5-ben.knoble%2Bgithub%40gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` This footgun is described in the previous commit to Documentation/BreakingChanges.adoc; without --index, stash apply and stash pop confusingly drop stashed index changes. When compiling with breaking changes, instead restore the index unless asked not to. Adjust both argument parsing and handling, as well as help synopses. Don't forget to adjust the error message when application fails to point to --no-index instead. Signed-off-by: D. Ben Knoble --- Notes: Tests will follow in separate patches, as some changes are rather complicated. Documentation/git-stash.adoc | 6 ++++++ builtin/stash.c | 38 ++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/Documentation/git-stash.adoc b/Documentation/git-stash.adoc index 1a5177f498..3b01f494a3 100644 --- a/Documentation/git-stash.adoc +++ b/Documentation/git-stash.adoc @@ -11,8 +11,14 @@ SYNOPSIS 'git stash' list [] 'git stash' show [-u | --include-untracked | --only-untracked] [] [] 'git stash' drop [-q | --quiet] [] +ifndef::with-breaking-changes[] 'git stash' pop [--index] [-q | --quiet] [] 'git stash' apply [--index] [-q | --quiet] [] +endif::with-breaking-changes[] +ifdef::with-breaking-changes[] +'git stash' pop [--no-index] [-q | --quiet] [] +'git stash' apply [--no-index] [-q | --quiet] [] +endif::with-breaking-changes[] 'git stash' branch [] 'git stash' [push [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet] [-u | --include-untracked] [-a | --all] [(-m | --message) ] diff --git a/builtin/stash.c b/builtin/stash.c index cfbd92852a..4ffa586d07 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -38,10 +38,17 @@ N_("git stash show [-u | --include-untracked | --only-untracked] [] []") #define BUILTIN_STASH_DROP_USAGE \ N_("git stash drop [-q | --quiet] []") +#ifdef WITH_BREAKING_CHANGES +#define BUILTIN_STASH_POP_USAGE \ + N_("git stash pop [--no-index] [-q | --quiet] []") +#define BUILTIN_STASH_APPLY_USAGE \ + N_("git stash apply [--no-index] [-q | --quiet] []") +#else #define BUILTIN_STASH_POP_USAGE \ N_("git stash pop [--index] [-q | --quiet] []") #define BUILTIN_STASH_APPLY_USAGE \ N_("git stash apply [--index] [-q | --quiet] []") +#endif /* WITH_BREAKING_CHANGES */ #define BUILTIN_STASH_BRANCH_USAGE \ N_("git stash branch []") #define BUILTIN_STASH_STORE_USAGE \ @@ -562,8 +569,13 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, ret = apply_cached(&out); strbuf_release(&out); if (ret) +#ifdef WITH_BREAKING_CHANGES + return error(_("conflicts in index. " + "Try with --no-index.")); +#else return error(_("conflicts in index. " "Try without --index.")); +#endif /* WITH_BREAKING_CHANGES */ discard_index(the_repository->index); repo_read_index(the_repository); @@ -658,12 +670,21 @@ static int apply_stash(int argc, const char **argv, const char *prefix, { int ret = -1; int quiet = 0; +#ifdef WITH_BREAKING_CHANGES + int no_index = 0; +#else int index = 0; +#endif /* WITH_BREAKING_CHANGES */ struct stash_info info = STASH_INFO_INIT; struct option options[] = { OPT__QUIET(&quiet, N_("be quiet, only report errors")), +#ifdef WITH_BREAKING_CHANGES + OPT_BOOL(0, "no-index", &no_index, + N_("do not attempt to recreate the index")), +#else OPT_BOOL(0, "index", &index, N_("attempt to recreate the index")), +#endif /* WITH_BREAKING_CHANGES */ OPT_END() }; @@ -673,7 +694,11 @@ static int apply_stash(int argc, const char **argv, const char *prefix, if (get_stash_info(&info, argc, argv)) goto cleanup; +#ifdef WITH_BREAKING_CHANGES + ret = do_apply_stash(prefix, &info, !no_index, quiet); +#else ret = do_apply_stash(prefix, &info, index, quiet); +#endif /* WITH_BREAKING_CHANGES */ cleanup: free_stash_info(&info); return ret; @@ -755,13 +780,22 @@ static int pop_stash(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) { int ret = -1; +#ifdef WITH_BREAKING_CHANGES + int no_index = 0; +#else int index = 0; +#endif /* WITH_BREAKING_CHANGES */ int quiet = 0; struct stash_info info = STASH_INFO_INIT; struct option options[] = { OPT__QUIET(&quiet, N_("be quiet, only report errors")), +#ifdef WITH_BREAKING_CHANGES + OPT_BOOL(0, "no-index", &no_index, + N_("do not attempt to recreate the index")), +#else OPT_BOOL(0, "index", &index, N_("attempt to recreate the index")), +#endif /* WITH_BREAKING_CHANGES */ OPT_END() }; @@ -771,7 +805,11 @@ static int pop_stash(int argc, const char **argv, const char *prefix, if (get_stash_info_assert(&info, argc, argv)) goto cleanup; +#ifdef WITH_BREAKING_CHANGES + if ((ret = do_apply_stash(prefix, &info, !no_index, quiet))) +#else if ((ret = do_apply_stash(prefix, &info, index, quiet))) +#endif /* WITH_BREAKING_CHANGES */ printf_ln(_("The stash entry is kept in case " "you need it again.")); else -- 2.48.1 ``` ## D. Ben Knoble, 2025-05-10 18:33 Subject: [PATCH 5/9] t0450: mark stash documentation as a known discrepancy Message-ID: <20250510183358.36806-6-ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/20250510183358.36806-6-ben.knoble%2Bgithub%40gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` The stash documentation now differs when built with breaking changes, and t0450 is not smart enough to understand the results. Expect failure by adding to the known list of mismatches. Signed-off-by: D. Ben Knoble --- t/t0450/adoc-help-mismatches | 1 + 1 file changed, 1 insertion(+) diff --git a/t/t0450/adoc-help-mismatches b/t/t0450/adoc-help-mismatches index c4a15fd0cb..379da30191 100644 --- a/t/t0450/adoc-help-mismatches +++ b/t/t0450/adoc-help-mismatches @@ -50,6 +50,7 @@ restore rev-parse show stage +stash switch update-index update-ref -- 2.48.1 ``` ## D. Ben Knoble, 2025-05-10 18:33 Subject: [PATCH 6/9] t3903: adjust stash test to account for --[no-]index with breaking changes Message-ID: <20250510183358.36806-7-ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/20250510183358.36806-7-ben.knoble%2Bgithub%40gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` A few tests check the results of the index after applying a stash; with breaking changes from previous commits that automatically restore the stashed index, the expected values are wrong. A few of the relevant tests check the restoration of s; with the aforementioned breaking changes, things get more interesting. In particular, if we "git stash push -- foo" but have "bar" in the index, then when applying the stash we get a conflict: "bar" was not removed from the index by the stash, but it was included in the recorded index in the stash. In those cases, apply the stash with "--no-index" (which would be the required user behavior). Signed-off-by: D. Ben Knoble --- Notes: It looks like the pathspec filtering is not applied to the stashed index; should it be? t/t3903-stash.sh | 158 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 147 insertions(+), 11 deletions(-) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index b8936a653b..36e1d3ec08 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -99,7 +99,7 @@ setup_stash() test_cmp expect file ' -test_expect_success 'apply stashed changes' ' +test_expect_success !WITH_BREAKING_CHANGES 'apply stashed changes' ' git reset --hard && echo 5 >other-file && git add other-file && @@ -111,6 +111,18 @@ setup_stash() test 1 = $(git show HEAD:file) ' +test_expect_success WITH_BREAKING_CHANGES 'apply stashed changes' ' + git reset --hard && + echo 5 >other-file && + git add other-file && + test_tick && + git commit -m other-file && + git stash apply && + test 3 = $(cat file) && + test 2 = $(git show :file) && + test 1 = $(git show HEAD:file) +' + test_expect_success 'apply stashed changes (including index)' ' git reset --hard HEAD^ && echo 6 >other-file && @@ -136,7 +148,7 @@ setup_stash() test_must_fail git stash drop --foo ' -test_expect_success 'drop top stash' ' +test_expect_success !WITH_BREAKING_CHANGES 'drop top stash' ' git reset --hard && git stash list >expected && echo 7 >file && @@ -150,7 +162,21 @@ setup_stash() test 1 = $(git show HEAD:file) ' -test_expect_success 'drop middle stash' ' +test_expect_success WITH_BREAKING_CHANGES 'drop top stash' ' + git reset --hard && + git stash list >expected && + echo 7 >file && + git stash && + git stash drop && + git stash list >actual && + test_cmp expected actual && + git stash apply && + test 3 = $(cat file) && + test 2 = $(git show :file) && + test 1 = $(git show HEAD:file) +' + +test_expect_success !WITH_BREAKING_CHANGES 'drop middle stash' ' git reset --hard && echo 8 >file && git stash && @@ -170,7 +196,27 @@ setup_stash() test 1 = $(git show HEAD:file) ' -test_expect_success 'drop middle stash by index' ' +test_expect_success WITH_BREAKING_CHANGES 'drop middle stash' ' + git reset --hard && + echo 8 >file && + git stash && + echo 9 >file && + git stash && + git stash drop stash@{1} && + test 2 = $(git stash list | wc -l) && + git stash apply && + test 9 = $(cat file) && + test 1 = $(git show :file) && + test 1 = $(git show HEAD:file) && + git reset --hard && + git stash drop && + git stash apply && + test 3 = $(cat file) && + test 2 = $(git show :file) && + test 1 = $(git show HEAD:file) +' + +test_expect_success !WITH_BREAKING_CHANGES 'drop middle stash by index' ' git reset --hard && echo 8 >file && git stash && @@ -227,7 +273,7 @@ setup_stash() test_cmp expect actual ' -test_expect_success 'stash pop' ' +test_expect_success !WITH_BREAKING_CHANGES 'stash pop' ' git reset --hard && git stash pop && test 3 = $(cat file) && @@ -236,6 +282,15 @@ setup_stash() test 0 = $(git stash list | wc -l) ' +test_expect_success WITH_BREAKING_CHANGES 'stash pop' ' + git reset --hard && + git stash pop && + test 3 = $(cat file) && + test 2 = $(git show :file) && + test 1 = $(git show HEAD:file) && + test 0 = $(git stash list | wc -l) +' + cat >expect <output.out 2>&1 && echo bar >expect && git show :file >actual && @@ -328,6 +383,14 @@ setup_stash() test_must_be_empty output.out ' +test_expect_success WITH_BREAKING_CHANGES 'pop -q works and is quiet' ' + git stash pop -q >output.out 2>&1 && + echo test >expect && + git show :file >actual && + test_cmp expect actual && + test_must_be_empty output.out +' + test_expect_success 'pop -q --index works and is quiet' ' echo foo >file && git add file && @@ -1166,7 +1229,7 @@ setup_stash() test_cmp expect actual ' -test_expect_success 'stash -- stashes and restores the file' ' +test_expect_success !WITH_BREAKING_CHANGES 'stash -- stashes and restores the file' ' >foo && >bar && git add foo bar && @@ -1178,7 +1241,19 @@ setup_stash() test_path_is_file bar ' -test_expect_success 'stash -- stashes in subdirectory' ' +test_expect_success WITH_BREAKING_CHANGES 'stash -- stashes and restores the file' ' + >foo && + >bar && + git add foo bar && + git stash push -- foo && + test_path_is_file bar && + test_path_is_missing foo && + git stash pop --no-index && + test_path_is_file foo && + test_path_is_file bar +' + +test_expect_success !WITH_BREAKING_CHANGES 'stash -- stashes in subdirectory' ' mkdir sub && >foo && >bar && @@ -1194,7 +1269,23 @@ setup_stash() test_path_is_file bar ' -test_expect_success 'stash with multiple pathspec arguments' ' +test_expect_success WITH_BREAKING_CHANGES 'stash -- stashes in subdirectory' ' + mkdir sub && + >foo && + >bar && + git add foo bar && + ( + cd sub && + git stash push -- ../foo + ) && + test_path_is_file bar && + test_path_is_missing foo && + git stash pop --no-index && + test_path_is_file foo && + test_path_is_file bar +' + +test_expect_success !WITH_BREAKING_CHANGES 'stash with multiple pathspec arguments' ' >foo && >bar && >extra && @@ -1209,7 +1300,22 @@ setup_stash() test_path_is_file extra ' -test_expect_success 'stash with file including $IFS character' ' +test_expect_success WITH_BREAKING_CHANGES 'stash with multiple pathspec arguments' ' + >foo && + >bar && + >extra && + git add foo bar extra && + git stash push -- foo bar && + test_path_is_missing bar && + test_path_is_missing foo && + test_path_is_file extra && + git stash pop --no-index && + test_path_is_file foo && + test_path_is_file bar && + test_path_is_file extra +' + +test_expect_success !WITH_BREAKING_CHANGES 'stash with file including $IFS character' ' >"foo bar" && >foo && >bar && @@ -1224,6 +1330,21 @@ setup_stash() test_path_is_file bar ' +test_expect_success WITH_BREAKING_CHANGES 'stash with file including $IFS character' ' + >"foo bar" && + >foo && + >bar && + git add foo* && + git stash push -- "foo b*" && + test_path_is_missing "foo bar" && + test_path_is_file foo && + test_path_is_file bar && + git stash pop --no-index && + test_path_is_file "foo bar" && + test_path_is_file foo && + test_path_is_file bar +' + test_expect_success 'stash with pathspec matching multiple paths' ' echo original >file && echo original >other-file && @@ -1297,7 +1418,7 @@ setup_stash() test_path_is_file untracked ' -test_expect_success 'stash without verb with pathspec' ' +test_expect_success !WITH_BREAKING_CHANGES 'stash without verb with pathspec' ' >"foo bar" && >foo && >bar && @@ -1312,6 +1433,21 @@ setup_stash() test_path_is_file bar ' +test_expect_success WITH_BREAKING_CHANGES 'stash without verb with pathspec' ' + >"foo bar" && + >foo && + >bar && + git add foo* && + git stash -- "foo b*" && + test_path_is_missing "foo bar" && + test_path_is_file foo && + test_path_is_file bar && + git stash pop --no-index && + test_path_is_file "foo bar" && + test_path_is_file foo && + test_path_is_file bar +' + test_expect_success 'stash -k -- leaves unstaged files intact' ' git reset && >foo && -- 2.48.1 ``` ## D. Ben Knoble, 2025-05-10 18:33 Subject: [PATCH 7/9] t3904: adjust stash -p test to account for index states with breaking changes Message-ID: <20250510183358.36806-8-ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/20250510183358.36806-8-ben.knoble%2Bgithub%40gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` Signed-off-by: D. Ben Knoble --- t/t3904-stash-patch.sh | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh index ae313e3c70..90f6b6bc90 100755 --- a/t/t3904-stash-patch.sh +++ b/t/t3904-stash-patch.sh @@ -30,7 +30,7 @@ verify_state dir/foo work index ' -test_expect_success 'git stash -p' ' +test_expect_success !WITH_BREAKING_CHANGES 'git stash -p' ' test_write_lines y n y | git stash save -p && verify_state HEAD committed HEADfile_index && verify_saved_state bar && @@ -42,6 +42,18 @@ verify_state dir/foo work head ' +test_expect_success WITH_BREAKING_CHANGES 'git stash -p' ' + test_write_lines y n y | git stash save -p && + verify_state HEAD committed HEADfile_index && + verify_saved_state bar && + verify_state dir/foo head index && + git reset --hard && + git stash apply && + verify_state HEAD HEADfile_work HEADfile_index && + verify_state bar dummy bar_index && + verify_state dir/foo work index +' + test_expect_success 'git stash -p --no-keep-index' ' set_state HEAD HEADfile_work HEADfile_index && set_state bar bar_work bar_index && -- 2.48.1 ``` ## D. Ben Knoble, 2025-05-10 18:33 Subject: [PATCH 8/9] t3905: adjust stash -u tests for breaking changes Message-ID: <20250510183358.36806-9-ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/20250510183358.36806-9-ben.knoble%2Bgithub%40gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` Like previous commits, adjust the expected results of application of stashes with --no-index as needed, with tests typically requiring --no-index. One test (stash pop after save --include-untracked leaves files untracked again) requires an extra cleanup step: subsequent tests ("stash save -u dirty index" and company) are not expecting "file" to be dirty in the index, but after "stash pop" it will be. Clean up after ourselves rather than adjusting later tests. Signed-off-by: D. Ben Knoble --- t/t3905-stash-include-untracked.sh | 39 ++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh index 7704709054..ee6cea49c8 100755 --- a/t/t3905-stash-include-untracked.sh +++ b/t/t3905-stash-include-untracked.sh @@ -89,7 +89,7 @@ git clean --force --quiet ' -test_expect_success 'stash pop after save --include-untracked leaves files untracked again' ' +test_expect_success !WITH_BREAKING_CHANGES 'stash pop after save --include-untracked leaves files untracked again' ' cat >expect <<-EOF && M file ?? HEAD @@ -108,6 +108,26 @@ test_cmp untracked_expect untracked/untracked ' +test_expect_success WITH_BREAKING_CHANGES 'stash pop after save --include-untracked leaves files untracked again' ' + cat >expect <<-EOF && + MM file + ?? HEAD + ?? actual + ?? expect + ?? file2 + ?? untracked/ + EOF + + git stash pop && + test_when_finished "git restore --staged file" && + git status --porcelain >actual && + test_cmp expect actual && + echo 1 >expect_file2 && + test_cmp expect_file2 file2 && + echo untracked >untracked_expect && + test_cmp untracked_expect untracked/untracked +' + test_expect_success 'clean up untracked/ directory to prepare for next tests' ' git clean --force --quiet -d ' @@ -206,7 +226,7 @@ test_path_is_file foo ' -test_expect_success 'stash push with $IFS character' ' +test_expect_success !WITH_BREAKING_CHANGES 'stash push with $IFS character' ' >"foo bar" && >foo && >bar && @@ -221,6 +241,21 @@ test_path_is_file bar ' +test_expect_success WITH_BREAKING_CHANGES 'stash push with $IFS character' ' + >"foo bar" && + >foo && + >bar && + git add foo* && + git stash push --include-untracked -- "foo b*" && + test_path_is_missing "foo bar" && + test_path_is_file foo && + test_path_is_file bar && + git stash pop --no-index && + test_path_is_file "foo bar" && + test_path_is_file foo && + test_path_is_file bar +' + test_expect_success 'stash previously ignored file' ' cat >.gitignore <<-EOF && ignored -- 2.48.1 ``` ## D. Ben Knoble, 2025-05-10 18:33 Subject: [PATCH 9/9] t3906: adjust stash submodule tests to account for breaking changes Message-ID: <20250510183358.36806-10-ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/20250510183358.36806-10-ben.knoble%2Bgithub%40gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` I cannot explain _why_ this occurs, but it seems that automatically unstashing the index from previous commits resolves some known failures in t3906 (which are captured by t/lib-submodule-updates.sh). In particular: - 'replace tracked file with submodule creates empty directory' succeeds with breaking changes; - all KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR tests succeed with breaking changes; - all KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES suceed with breaking changes. Signed-off-by: D. Ben Knoble --- t/lib-submodule-update.sh | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 36f767cb74..4ae909c432 100644 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -353,7 +353,21 @@ test_submodule_switch_common () ' # Replacing a tracked file with a submodule produces an empty # directory ... - test_expect_$RESULT "$command: replace tracked file with submodule creates empty directory" ' + test_expect_$RESULT !WITH_BREAKING_CHANGES "$command: replace tracked file with submodule creates empty directory" ' + prolog && + reset_work_tree_to replace_sub1_with_file && + ( + cd submodule_update && + git branch -t replace_file_with_sub1 origin/replace_file_with_sub1 && + $command replace_file_with_sub1 && + test_superproject_content origin/replace_file_with_sub1 && + test_dir_is_empty sub1 && + git submodule update --init --recursive && + test_submodule_content sub1 origin/replace_file_with_sub1 + ) + ' + # (unless we automatically unstash the index!) + test_expect_success WITH_BREAKING_CHANGES "$command: replace tracked file with submodule creates empty directory" ' prolog && reset_work_tree_to replace_sub1_with_file && ( @@ -368,7 +382,8 @@ test_submodule_switch_common () ' # ... as does removing a directory with tracked files with a # submodule. - if test "$KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR" = 1 + if ! test_have_prereq WITH_BREAKING_CHANGES && + test "$KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR" = 1 then # Non fast-forward merges fail with "Directory sub1 doesn't # exist. sub1" because the empty submodule directory is not @@ -392,8 +407,9 @@ test_submodule_switch_common () ' ######################## Disappearing submodule ####################### - # Removing a submodule doesn't remove its work tree ... - if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1 + # Removing a submodule doesn't remove its work tree (unless stash applies the index!) ... + if ! test_have_prereq WITH_BREAKING_CHANGES && + test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1 then RESULT="failure" else -- 2.48.1 ``` ## Junio C Hamano, 2025-05-12 12:52 Subject: Re: [PATCH 0/9] make stash apply with --index by default Message-ID: URL: https://gitlist.dev/e/xmqqzffivvnr.fsf%40gitster.g In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` "D. Ben Knoble" writes: > Since git-stash's inception, it has defaulted to stashing the index but not > restoring it. This has caused some confusion: change the default as part of Git > 3.0 to unstash the index, too. I am very tempted to veto this, as I very much recall trying to see how I liked it and used in my workflow (by using an alias to do the "pop" always used with "--index"), got quite frustrated seeing that a lot of times the command would have worked perfectly well if the "--index" option weren't given, and gave up after a few weeks even though I tried very hard to like it as the default. As "--index" is a Boolean option, I can see that we probably already have "--no-index" for free, so there is an escape hatch already, but I do not think it would give a good end-user experience to force them to keep saying "--no-index" (that is 3 keystrokes longer than what those who want "--index" need to type). ``` ## D. Ben Knoble, 2025-05-20 14:36 Subject: Re: [PATCH 0/9] make stash apply with --index by default Message-ID: URL: https://gitlist.dev/e/CALnO6CA0S_54HjyNDWyTAhfmRFcTsFEMF67yWjMeL526O14aQg%40mail.gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` On Sat, May 10, 2025 at 2:34 PM D. Ben Knoble wrote: > > Since git-stash's inception, it has defaulted to stashing the index but not > restoring it. This has caused some confusion: change the default as part of Git > 3.0 to unstash the index, too. > > I ran into this myself a while back and did some digging; it appears that other > have been bit, too (see references in patch 3). Moreover, when git-stash was > originally written, defaulting with --index was suggested but not implemented > before the script merged. So this RFC should bring us back towards "less > confusing," hopefully. > > The series is structured as follows: > > Patches 1-2: unrelated prep/style commits noticed while working on tests. > Patch 3: update Documentation/BreakingChanges.adoc. This seems like a natural > place to discuss the proposal, so it contains no other changes. > Patch 4: make it so in builtin/stash.c. > Patches 5-9: update the impacted tests. Separated out for ease of review. I used > a style more like "split the test into 2: one with, one without breaking > changes." In retrospect, the diff might be smaller (and the 2 versions of > the test easier to compare) if I used "test_has_prereq" in the tests… but at > the cost of making the tests harder to follow. Thoughts? > > D. Ben Knoble (9): > t3903: reduce dependencies on previous tests > t3905: remove unneeded blank line > BreakingChanges: announce stash {apply,pop} will imply --index > stash: restore the index by default when breaking changes are enabled > t0450: mark stash documentation as a known discrepancy > t3903: adjust stash test to account for --[no-]index with breaking > changes > t3904: adjust stash -p test to account for index states with breaking > changes > t3905: adjust stash -u tests for breaking changes > t3906: adjust stash submodule tests to account for breaking changes > > Documentation/BreakingChanges.adoc | 11 ++ > Documentation/git-stash.adoc | 6 ++ > builtin/stash.c | 38 +++++++ > t/lib-submodule-update.sh | 24 ++++- > t/t0450/adoc-help-mismatches | 1 + > t/t3903-stash.sh | 161 +++++++++++++++++++++++++++-- > t/t3904-stash-patch.sh | 14 ++- > t/t3905-stash-include-untracked.sh | 40 ++++++- > 8 files changed, 276 insertions(+), 19 deletions(-) > > > base-commit: 1ee85f0e215f22b0878d0ad4b2445d12bbb63887 > -- > 2.48.1 > ``` ## D. Ben Knoble, 2025-05-20 14:39 Subject: Re: [PATCH 0/9] make stash apply with --index by default Message-ID: URL: https://gitlist.dev/e/CALnO6CB%3D2ih-_%3DVUth%3DPJZxGkz_GA_mUfWd6gQOFcd36eUpD4A%40mail.gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` [Apologies for re-send; mis-clicked] On Sat, May 10, 2025 at 2:34 PM D. Ben Knoble wrote: > > Since git-stash's inception, it has defaulted to stashing the index but not > restoring it. This has caused some confusion: change the default as part of Git > 3.0 to unstash the index, too. > > I ran into this myself a while back and did some digging; it appears that other > have been bit, too (see references in patch 3). Moreover, when git-stash was > originally written, defaulting with --index was suggested but not implemented > before the script merged. So this RFC should bring us back towards "less > confusing," hopefully. > > The series is structured as follows: > > Patches 1-2: unrelated prep/style commits noticed while working on tests. > Patch 3: update Documentation/BreakingChanges.adoc. This seems like a natural > place to discuss the proposal, so it contains no other changes. > Patch 4: make it so in builtin/stash.c. > Patches 5-9: update the impacted tests. Separated out for ease of review. I used > a style more like "split the test into 2: one with, one without breaking > changes." In retrospect, the diff might be smaller (and the 2 versions of > the test easier to compare) if I used "test_has_prereq" in the tests… but at > the cost of making the tests harder to follow. Thoughts? > > D. Ben Knoble (9): > t3903: reduce dependencies on previous tests > t3905: remove unneeded blank line > BreakingChanges: announce stash {apply,pop} will imply --index > stash: restore the index by default when breaking changes are enabled > t0450: mark stash documentation as a known discrepancy > t3903: adjust stash test to account for --[no-]index with breaking > changes > t3904: adjust stash -p test to account for index states with breaking > changes > t3905: adjust stash -u tests for breaking changes > t3906: adjust stash submodule tests to account for breaking changes > > Documentation/BreakingChanges.adoc | 11 ++ > Documentation/git-stash.adoc | 6 ++ > builtin/stash.c | 38 +++++++ > t/lib-submodule-update.sh | 24 ++++- > t/t0450/adoc-help-mismatches | 1 + > t/t3903-stash.sh | 161 +++++++++++++++++++++++++++-- > t/t3904-stash-patch.sh | 14 ++- > t/t3905-stash-include-untracked.sh | 40 ++++++- > 8 files changed, 276 insertions(+), 19 deletions(-) > > > base-commit: 1ee85f0e215f22b0878d0ad4b2445d12bbb63887 > -- > 2.48.1 > Any comments from others? CC'd folks from individual patches on cover letter. ``` ## D. Ben Knoble, 2025-09-16 00:37 Subject: [PATCH v2 0/4] Teach git-stash to use --index from config Message-ID: URL: https://gitlist.dev/e/cover.1757982870.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: <20250510183358.36806-1-ben.knoble+github@gmail.com> ``` Changes from v1: - configure --index via config - drop BreakingChanges related work With stash.index=true, git-stash(1) command now tries to reinstate the index by default in the "apply" and "pop" modes. Not doing so creates a common trap: "git stash apply" is not the reverse of "git stash push" because carefully staged indices are lost and have to be manually recreated. OTOH, this mode is not always desirable and may create more conflicts when applying stashes. Use "--no-index" to disable this behavior. Cf. , . PS I've left some new t3903 tests as copy-pasta for now to get feedback on the rest of the series; there are bits of that file that could use an update to the modern style (e.g., not using "test 1 = $(cat file)"). Since some new tests are substantially similar to old tests that use this style, such cleanup is /probably/ warranted but will delay eyeballs on the core of this series. Published-as: https://github.com/benknoble/git/tree/stash-apply-index v1: https://lore.kernel.org/git/20250510183358.36806-1-ben.knoble+github@gmail.com/ D. Ben Knoble (4): t3903: reduce dependencies on previous tests t3905: remove unneeded blank line stash: refactor private config globals stash: honor stash.index in apply, pop modes Documentation/config/stash.adoc | 5 + builtin/stash.c | 17 ++-- t/t3903-stash.sh | 158 +++++++++++++++++++++++++++++ t/t3904-stash-patch.sh | 11 ++ t/t3905-stash-include-untracked.sh | 49 ++++++++- 5 files changed, 232 insertions(+), 8 deletions(-) Diff-intervalle contre v1 : 1: 1328eb8eac = 1: 1328eb8eac t3903: reduce dependencies on previous tests 2: 8ac06ad62d = 2: 8ac06ad62d t3905: remove unneeded blank line 3: c068f1dc0b < -: ---------- BreakingChanges: announce stash {apply,pop} will imply --index 4: 8caff91c0e < -: ---------- stash: restore the index by default when breaking changes are enabled 5: 387427bb8c < -: ---------- t0450: mark stash documentation as a known discrepancy 9: c72a1fe6ea ! 3: bf0a561ce3 t3906: adjust stash submodule tests to account for breaking changes @@ Metadata Author: D. Ben Knoble ## Commit message ## - t3906: adjust stash submodule tests to account for breaking changes + stash: refactor private config globals - I cannot explain _why_ this occurs, but it seems that automatically - unstashing the index from previous commits resolves some known failures - in t3906 (which are captured by t/lib-submodule-updates.sh). + A subsequent commit will access a new config variable in the stash + subcommand implementations, which requires the variables to be declared + before the relevant functions. Prep with a pure refactoring change to + consolidate config-related globals with the rest of the globals. - In particular: - - 'replace tracked file with submodule creates empty directory' succeeds - with breaking changes; - - all KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR tests - succeed with breaking changes; - - all KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES suceed with - breaking changes. + Best-viewed-with: --color-moved - ## t/lib-submodule-update.sh ## -@@ t/lib-submodule-update.sh: test_submodule_switch_common () - ' - # Replacing a tracked file with a submodule produces an empty - # directory ... -- test_expect_$RESULT "$command: replace tracked file with submodule creates empty directory" ' -+ test_expect_$RESULT !WITH_BREAKING_CHANGES "$command: replace tracked file with submodule creates empty directory" ' -+ prolog && -+ reset_work_tree_to replace_sub1_with_file && -+ ( -+ cd submodule_update && -+ git branch -t replace_file_with_sub1 origin/replace_file_with_sub1 && -+ $command replace_file_with_sub1 && -+ test_superproject_content origin/replace_file_with_sub1 && -+ test_dir_is_empty sub1 && -+ git submodule update --init --recursive && -+ test_submodule_content sub1 origin/replace_file_with_sub1 -+ ) -+ ' -+ # (unless we automatically unstash the index!) -+ test_expect_success WITH_BREAKING_CHANGES "$command: replace tracked file with submodule creates empty directory" ' - prolog && - reset_work_tree_to replace_sub1_with_file && - ( -@@ t/lib-submodule-update.sh: test_submodule_switch_common () - ' - # ... as does removing a directory with tracked files with a - # submodule. -- if test "$KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR" = 1 -+ if ! test_have_prereq WITH_BREAKING_CHANGES && -+ test "$KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR" = 1 - then - # Non fast-forward merges fail with "Directory sub1 doesn't - # exist. sub1" because the empty submodule directory is not -@@ t/lib-submodule-update.sh: test_submodule_switch_common () - ' + ## builtin/stash.c ## +@@ builtin/stash.c: static const char * const git_stash_save_usage[] = { + static const char ref_stash[] = "refs/stash"; + static struct strbuf stash_index_path = STRBUF_INIT; - ######################## Disappearing submodule ####################### -- # Removing a submodule doesn't remove its work tree ... -- if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1 -+ # Removing a submodule doesn't remove its work tree (unless stash applies the index!) ... -+ if ! test_have_prereq WITH_BREAKING_CHANGES && -+ test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1 - then - RESULT="failure" - else ++static int show_stat = 1; ++static int show_patch; ++static int show_include_untracked; ++ + /* + * w_commit is set to the commit containing the working tree + * b_commit is set to the base commit +@@ builtin/stash.c: 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 git_stash_config(const char *var, const char *value, + const struct config_context *ctx, void *cb) + { 6: 0a12983c00 ! 4: 585e124467 t3903: adjust stash test to account for --[no-]index with breaking changes @@ Metadata Author: D. Ben Knoble ## Commit message ## - t3903: adjust stash test to account for --[no-]index with breaking changes + stash: honor stash.index in apply, pop modes - A few tests check the results of the index after applying a stash; with - breaking changes from previous commits that automatically restore the - stashed index, the expected values are wrong. + With stash.index=true, git-stash(1) command now tries to reinstate the + index by default in the "apply" and "pop" modes. Not doing so creates a + common trap [1], [2]: "git stash apply" is not the reverse of "git stash + push" because carefully staged indices are lost and have to be manually + recreated. OTOH, this mode is not always desirable and may create more + conflicts when applying stashes. As usual, "--no-index" will disable + this behavior if you set "stash.index". - A few of the relevant tests check the restoration of s; with - the aforementioned breaking changes, things get more interesting. In - particular, if we "git stash push -- foo" but have "bar" in the index, - then when applying the stash we get a conflict: "bar" was not removed - from the index by the stash, but it was included in the recorded index - in the stash. In those cases, apply the stash with "--no-index" (which - would be the required user behavior). + [1]: https://lore.kernel.org/git/CAPx1GvcxyDDQmCssMjEnt6JoV6qPc5ZUpgPLX3mpUC_4PNYA1w@mail.gmail.com/ + [2]: https://lore.kernel.org/git/c5a811ac-8cd3-c389-ac6d-29020a648c87@gmail.com/ - Signed-off-by: D. Ben Knoble + ## Documentation/config/stash.adoc ## +@@ ++stash.index:: ++ If this is set to true, `git stash apply` and `git stash pop` will ++ behave as if `--index` was supplied. Defaults to false. See the ++ descriptions in 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 - - ## Notes ## - It looks like the pathspec filtering is not applied to the stashed - index; should it be? + ## builtin/stash.c ## +@@ builtin/stash.c: static struct strbuf stash_index_path = STRBUF_INIT; + static int show_stat = 1; + static int show_patch; + static int show_include_untracked; ++static int use_index; + + /* + * w_commit is set to the commit containing the working tree +@@ builtin/stash.c: static int apply_stash(int argc, const char **argv, const char *prefix, + { + int ret = -1; + int quiet = 0; +- int index = 0; ++ int index = use_index; + struct stash_info info = STASH_INFO_INIT; + struct option options[] = { + OPT__QUIET(&quiet, N_("be quiet, only report errors")), +@@ builtin/stash.c: static int pop_stash(int argc, const char **argv, const char *prefix, + struct repository *repo UNUSED) + { + int ret = -1; +- int index = 0; ++ int index = use_index; + int quiet = 0; + struct stash_info info = STASH_INFO_INIT; + struct option options[] = { +@@ builtin/stash.c: static int git_stash_config(const char *var, const char *value, + show_include_untracked = git_config_bool(var, value); + return 0; + } ++ if (!strcmp(var, "stash.index")) { ++ use_index = git_config_bool(var, value); ++ return 0; ++ } + return git_diff_basic_config(var, value, ctx, cb); + } + ## t/t3903-stash.sh ## -@@ t/t3903-stash.sh: setup_stash() - test_cmp expect file - ' - --test_expect_success 'apply stashed changes' ' -+test_expect_success !WITH_BREAKING_CHANGES 'apply stashed changes' ' - git reset --hard && - echo 5 >other-file && - git add other-file && @@ t/t3903-stash.sh: setup_stash() test 1 = $(git show HEAD:file) ' -+test_expect_success WITH_BREAKING_CHANGES 'apply stashed changes' ' -+ git reset --hard && ++test_expect_success 'apply stashed changes with stash.index' ' ++ test_config stash.index true && ++ git reset --hard HEAD^ && + echo 5 >other-file && + git add other-file && + test_tick && @@ t/t3903-stash.sh: setup_stash() test_expect_success 'apply stashed changes (including index)' ' git reset --hard HEAD^ && echo 6 >other-file && -@@ t/t3903-stash.sh: setup_stash() - test_must_fail git stash drop --foo - ' - --test_expect_success 'drop top stash' ' -+test_expect_success !WITH_BREAKING_CHANGES 'drop top stash' ' - git reset --hard && - git stash list >expected && - echo 7 >file && @@ t/t3903-stash.sh: setup_stash() test 1 = $(git show HEAD:file) ' --test_expect_success 'drop middle stash' ' -+test_expect_success WITH_BREAKING_CHANGES 'drop top stash' ' ++test_expect_success 'drop top stash with stash.index' ' ++ test_config stash.index true && + git reset --hard && + git stash list >expected && + echo 7 >file && @@ t/t3903-stash.sh: setup_stash() + test 1 = $(git show HEAD:file) +' + -+test_expect_success !WITH_BREAKING_CHANGES 'drop middle stash' ' + test_expect_success 'drop middle stash' ' git reset --hard && echo 8 >file && - git stash && @@ t/t3903-stash.sh: setup_stash() test 1 = $(git show HEAD:file) ' --test_expect_success 'drop middle stash by index' ' -+test_expect_success WITH_BREAKING_CHANGES 'drop middle stash' ' ++test_expect_success 'drop middle stash with stash.index' ' ++ test_config stash.index true && + git reset --hard && + echo 8 >file && + git stash && @@ t/t3903-stash.sh: setup_stash() + test 1 = $(git show HEAD:file) +' + -+test_expect_success !WITH_BREAKING_CHANGES 'drop middle stash by index' ' + test_expect_success 'drop middle stash by index' ' git reset --hard && echo 8 >file && - git stash && -@@ t/t3903-stash.sh: setup_stash() - test_cmp expect actual - ' - --test_expect_success 'stash pop' ' -+test_expect_success !WITH_BREAKING_CHANGES 'stash pop' ' - git reset --hard && - git stash pop && - test 3 = $(cat file) && @@ t/t3903-stash.sh: setup_stash() test 0 = $(git stash list | wc -l) ' -+test_expect_success WITH_BREAKING_CHANGES 'stash pop' ' ++test_expect_success 'stash pop with stash.index' ' ++ test_config stash.index true && + git reset --hard && ++ setup_stash && + git stash pop && + test 3 = $(cat file) && + test 2 = $(git show :file) && @@ t/t3903-stash.sh: setup_stash() test_must_be_empty output.out ' --test_expect_success 'pop -q works and is quiet' ' -+test_expect_success !WITH_BREAKING_CHANGES 'pop -q works and is quiet' ' - git stash pop -q >output.out 2>&1 && - echo bar >expect && - git show :file >actual && -@@ t/t3903-stash.sh: setup_stash() - test_must_be_empty output.out - ' - -+test_expect_success WITH_BREAKING_CHANGES 'pop -q works and is quiet' ' ++test_expect_success 'pop -q works and is quiet with stash.index' ' ++ # Added file, deleted file, modified file all staged for commit ++ echo foo >new-file && ++ echo test >file && ++ git add new-file file && ++ git rm other-file && ++ git stash && ++ ++ test_config stash.index true && + git stash pop -q >output.out 2>&1 && + echo test >expect && + git show :file >actual && @@ t/t3903-stash.sh: setup_stash() test_expect_success 'pop -q --index works and is quiet' ' echo foo >file && git add file && -@@ t/t3903-stash.sh: setup_stash() - test_cmp expect actual - ' - --test_expect_success 'stash -- stashes and restores the file' ' -+test_expect_success !WITH_BREAKING_CHANGES 'stash -- stashes and restores the file' ' - >foo && - >bar && - git add foo bar && @@ t/t3903-stash.sh: setup_stash() test_path_is_file bar ' --test_expect_success 'stash -- stashes in subdirectory' ' -+test_expect_success WITH_BREAKING_CHANGES 'stash -- stashes and restores the file' ' ++test_expect_success 'stash -- stashes and restores the file with stash.index' ' ++ test_config stash.index true && + >foo && + >bar && + git add foo bar && @@ t/t3903-stash.sh: setup_stash() + test_path_is_file bar +' + -+test_expect_success !WITH_BREAKING_CHANGES 'stash -- stashes in subdirectory' ' + test_expect_success 'stash -- stashes in subdirectory' ' mkdir sub && >foo && - >bar && @@ t/t3903-stash.sh: setup_stash() test_path_is_file bar ' --test_expect_success 'stash with multiple pathspec arguments' ' -+test_expect_success WITH_BREAKING_CHANGES 'stash -- stashes in subdirectory' ' ++test_expect_success 'stash -- stashes in subdirectory with stash.index' ' ++ test_config stash.index true && ++ rm -r sub && + mkdir sub && + >foo && + >bar && @@ t/t3903-stash.sh: setup_stash() + test_path_is_file bar +' + -+test_expect_success !WITH_BREAKING_CHANGES 'stash with multiple pathspec arguments' ' + test_expect_success 'stash with multiple pathspec arguments' ' >foo && >bar && - >extra && @@ t/t3903-stash.sh: setup_stash() test_path_is_file extra ' --test_expect_success 'stash with file including $IFS character' ' -+test_expect_success WITH_BREAKING_CHANGES 'stash with multiple pathspec arguments' ' ++test_expect_success 'stash with multiple pathspec arguments with stash.index' ' ++ test_config stash.index true && + >foo && + >bar && + >extra && @@ t/t3903-stash.sh: setup_stash() + test_path_is_file extra +' + -+test_expect_success !WITH_BREAKING_CHANGES 'stash with file including $IFS character' ' + test_expect_success 'stash with file including $IFS character' ' >"foo bar" && >foo && - >bar && @@ t/t3903-stash.sh: setup_stash() test_path_is_file bar ' -+test_expect_success WITH_BREAKING_CHANGES 'stash with file including $IFS character' ' ++test_expect_success 'stash with file including $IFS character with stash.index' ' ++ test_config stash.index true && + >"foo bar" && + >foo && + >bar && @@ t/t3903-stash.sh: setup_stash() test_expect_success 'stash with pathspec matching multiple paths' ' echo original >file && echo original >other-file && -@@ t/t3903-stash.sh: setup_stash() - test_path_is_file untracked - ' - --test_expect_success 'stash without verb with pathspec' ' -+test_expect_success !WITH_BREAKING_CHANGES 'stash without verb with pathspec' ' - >"foo bar" && - >foo && - >bar && @@ t/t3903-stash.sh: setup_stash() test_path_is_file bar ' -+test_expect_success WITH_BREAKING_CHANGES 'stash without verb with pathspec' ' ++test_expect_success 'stash without verb with pathspec with stash.index' ' ++ test_config stash.index true && + >"foo bar" && + >foo && + >bar && @@ t/t3903-stash.sh: setup_stash() test_expect_success 'stash -k -- leaves unstaged files intact' ' git reset && >foo && + + ## t/t3904-stash-patch.sh ## +@@ + verify_state dir/foo work head + ' + ++test_expect_success 'git stash -p with stash.index' ' ++ test_config stash.index true && ++ set_state HEAD HEADfile_work HEADfile_index && ++ set_state dir/foo work index && ++ test_write_lines y n y | git stash save -p && ++ git reset --hard && ++ git stash apply && ++ verify_state HEAD HEADfile_work HEADfile_index && ++ verify_state dir/foo head index ++' ++ + test_expect_success 'git stash -p --no-keep-index' ' + set_state HEAD HEADfile_work HEADfile_index && + set_state bar bar_work bar_index && + + ## t/t3905-stash-include-untracked.sh ## +@@ + + . ./test-lib.sh + +-test_expect_success 'stash save --include-untracked some dirty working directory' ' ++setup() { + echo 1 >file && + git add file && + test_tick && +@@ + git stash --include-untracked && + git diff-files --quiet && + git diff-index --cached --quiet HEAD ++} ++ ++test_expect_success 'stash save --include-untracked some dirty working directory' ' ++ setup + ' + + test_expect_success 'stash save --include-untracked cleaned the untracked files' ' +@@ + test_cmp untracked_expect untracked/untracked + ' + ++test_expect_success 'stash pop after save --include-untracked leaves files untracked again with stash.index' ' ++ git init repo && ++ test_when_finished rm -r repo && ++ ( ++ cd repo && ++ git config stash.index true && ++ setup && ++ cat >expect <<-EOF && ++ MM file ++ ?? HEAD ++ ?? actual ++ ?? expect ++ ?? file2 ++ ?? untracked/ ++ EOF ++ ++ git stash pop && ++ git status --porcelain >actual && ++ test_cmp expect actual && ++ echo 1 >expect_file2 && ++ test_cmp expect_file2 file2 && ++ echo untracked >untracked_expect && ++ test_cmp untracked_expect untracked/untracked ++ ) ++' ++ + test_expect_success 'clean up untracked/ directory to prepare for next tests' ' + git clean --force --quiet -d + ' +@@ + test_path_is_file bar + ' + ++test_expect_success 'stash push with $IFS character with stash.index' ' ++ test_config stash.index true && ++ >"foo bar" && ++ >foo && ++ >bar && ++ git add foo* && ++ git stash push --include-untracked -- "foo b*" && ++ test_path_is_missing "foo bar" && ++ test_path_is_file foo && ++ test_path_is_file bar && ++ git stash pop --no-index && ++ test_path_is_file "foo bar" && ++ test_path_is_file foo && ++ test_path_is_file bar ++' ++ + test_expect_success 'stash previously ignored file' ' + cat >.gitignore <<-EOF && + ignored 7: 4b92d47e16 < -: ---------- t3904: adjust stash -p test to account for index states with breaking changes 8: af14dee1be < -: ---------- t3905: adjust stash -u tests for breaking changes base-commit: 1ee85f0e215f22b0878d0ad4b2445d12bbb63887 -- 2.48.1 ``` ## D. Ben Knoble, 2025-09-16 00:37 Subject: [PATCH v2 1/4] t3903: reduce dependencies on previous tests Message-ID: <1328eb8eac65ec7bbf1f3cb257d7221a372c45c3.1757982870.git.ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/1328eb8eac65ec7bbf1f3cb257d7221a372c45c3.1757982870.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` Skipping previous tests to work through only failing tests with arguments like --run=4,122- causes some tests to fail because subdir doesn't exist yet (it is created by a previous test; typically "unstashing in a subdirectory"). Create it on demand for tests that need it, but don't fail (-p) if the directory already exists. Signed-off-by: D. Ben Knoble --- t/t3903-stash.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 74666ff3e4..b8936a653b 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -895,6 +895,7 @@ setup_stash() test_expect_success 'apply: show same status as git status (relative to ./)' ' git stash clear && + mkdir -p subdir && echo 1 >subdir/subfile1 && echo 2 >subdir/subfile2 && git add subdir/subfile1 && @@ -1327,6 +1328,7 @@ setup_stash() test_expect_success 'stash -- leaves untracked files in subdir intact' ' git reset && + mkdir -p subdir && >subdir/untracked && >subdir/tracked1 && >subdir/tracked2 && @@ -1343,6 +1345,7 @@ setup_stash() test_expect_success 'stash -- works with binary files' ' git reset && + mkdir -p subdir && >subdir/untracked && >subdir/tracked && cp "$TEST_DIRECTORY"/test-binary-1.png subdir/tracked-binary && -- 2.48.1 ``` ## D. Ben Knoble, 2025-09-16 00:37 Subject: [PATCH v2 2/4] t3905: remove unneeded blank line Message-ID: <8ac06ad62df581bb84ecdca0b0144c218daa0733.1757982870.git.ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/8ac06ad62df581bb84ecdca0b0144c218daa0733.1757982870.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` This is leftover from 787513027a (stash: Add --include-untracked option to stash and remove all untracked files, 2011-06-24) when it was converted in bbaa45c3aa (t3905: move all commands into test cases, 2021-02-08). Signed-off-by: D. Ben Knoble --- t/t3905-stash-include-untracked.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh index 1289ae3e07..7704709054 100755 --- a/t/t3905-stash-include-untracked.sh +++ b/t/t3905-stash-include-untracked.sh @@ -87,7 +87,6 @@ test_expect_success 'clean up untracked/untracked file to prepare for next tests' ' git clean --force --quiet - ' test_expect_success 'stash pop after save --include-untracked leaves files untracked again' ' -- 2.48.1 ``` ## D. Ben Knoble, 2025-09-16 00:37 Subject: [PATCH v2 3/4] stash: refactor private config globals Message-ID: URL: https://gitlist.dev/e/bf0a561ce36afe14b6bc03bf851d19b30ef491ad.1757982870.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` A subsequent commit will access a new config variable in the stash subcommand implementations, which requires the variables to be declared before the relevant functions. Prep with a pure refactoring change to consolidate config-related globals with the rest of the globals. Best-viewed-with: --color-moved Signed-off-by: D. Ben Knoble --- builtin/stash.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index cfbd92852a..d9b478d1d1 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -127,6 +127,10 @@ static const char * const git_stash_save_usage[] = { static const char ref_stash[] = "refs/stash"; static struct strbuf stash_index_path = STRBUF_INIT; +static int show_stat = 1; +static int show_patch; +static int show_include_untracked; + /* * w_commit is set to the commit containing the working tree * b_commit is set to the base commit @@ -845,10 +849,6 @@ 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 git_stash_config(const char *var, const char *value, const struct config_context *ctx, void *cb) { -- 2.48.1 ``` ## D. Ben Knoble, 2025-09-16 00:37 Subject: [PATCH v2 4/4] stash: honor stash.index in apply, pop modes Message-ID: <585e124467dcb1ec1de71fa72e749140d44dc367.1757982870.git.ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/585e124467dcb1ec1de71fa72e749140d44dc367.1757982870.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` With stash.index=true, git-stash(1) command now tries to reinstate the index by default in the "apply" and "pop" modes. Not doing so creates a common trap [1], [2]: "git stash apply" is not the reverse of "git stash push" because carefully staged indices are lost and have to be manually recreated. OTOH, this mode is not always desirable and may create more conflicts when applying stashes. As usual, "--no-index" will disable this behavior if you set "stash.index". [1]: https://lore.kernel.org/git/CAPx1GvcxyDDQmCssMjEnt6JoV6qPc5ZUpgPLX3mpUC_4PNYA1w@mail.gmail.com/ [2]: https://lore.kernel.org/git/c5a811ac-8cd3-c389-ac6d-29020a648c87@gmail.com/ Signed-off-by: D. Ben Knoble --- Documentation/config/stash.adoc | 5 + builtin/stash.c | 9 +- t/t3903-stash.sh | 155 +++++++++++++++++++++++++++++ t/t3904-stash-patch.sh | 11 ++ t/t3905-stash-include-untracked.sh | 48 ++++++++- 5 files changed, 225 insertions(+), 3 deletions(-) diff --git a/Documentation/config/stash.adoc b/Documentation/config/stash.adoc index ec1edaeba6..e556105a15 100644 --- a/Documentation/config/stash.adoc +++ b/Documentation/config/stash.adoc @@ -1,3 +1,8 @@ +stash.index:: + If this is set to true, `git stash apply` and `git stash pop` will + behave as if `--index` was supplied. Defaults to false. See the + descriptions in 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 diff --git a/builtin/stash.c b/builtin/stash.c index d9b478d1d1..8a0eef3c70 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -130,6 +130,7 @@ static struct strbuf stash_index_path = STRBUF_INIT; static int show_stat = 1; static int show_patch; static int show_include_untracked; +static int use_index; /* * w_commit is set to the commit containing the working tree @@ -662,7 +663,7 @@ static int apply_stash(int argc, const char **argv, const char *prefix, { int ret = -1; int quiet = 0; - int index = 0; + int index = use_index; struct stash_info info = STASH_INFO_INIT; struct option options[] = { OPT__QUIET(&quiet, N_("be quiet, only report errors")), @@ -759,7 +760,7 @@ static int pop_stash(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) { int ret = -1; - int index = 0; + int index = use_index; int quiet = 0; struct stash_info info = STASH_INFO_INIT; struct option options[] = { @@ -864,6 +865,10 @@ static int git_stash_config(const char *var, const char *value, show_include_untracked = git_config_bool(var, value); return 0; } + if (!strcmp(var, "stash.index")) { + use_index = git_config_bool(var, value); + return 0; + } return git_diff_basic_config(var, value, ctx, cb); } diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index b8936a653b..1d53a94165 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -111,6 +111,19 @@ setup_stash() test 1 = $(git show HEAD:file) ' +test_expect_success 'apply stashed changes with stash.index' ' + test_config stash.index true && + git reset --hard HEAD^ && + echo 5 >other-file && + git add other-file && + test_tick && + git commit -m other-file && + git stash apply && + test 3 = $(cat file) && + test 2 = $(git show :file) && + test 1 = $(git show HEAD:file) +' + test_expect_success 'apply stashed changes (including index)' ' git reset --hard HEAD^ && echo 6 >other-file && @@ -150,6 +163,21 @@ setup_stash() test 1 = $(git show HEAD:file) ' +test_expect_success 'drop top stash with stash.index' ' + test_config stash.index true && + git reset --hard && + git stash list >expected && + echo 7 >file && + git stash && + git stash drop && + git stash list >actual && + test_cmp expected actual && + git stash apply && + test 3 = $(cat file) && + test 2 = $(git show :file) && + test 1 = $(git show HEAD:file) +' + test_expect_success 'drop middle stash' ' git reset --hard && echo 8 >file && @@ -170,6 +198,27 @@ setup_stash() test 1 = $(git show HEAD:file) ' +test_expect_success 'drop middle stash with stash.index' ' + test_config stash.index true && + git reset --hard && + echo 8 >file && + git stash && + echo 9 >file && + git stash && + git stash drop stash@{1} && + test 2 = $(git stash list | wc -l) && + git stash apply && + test 9 = $(cat file) && + test 1 = $(git show :file) && + test 1 = $(git show HEAD:file) && + git reset --hard && + git stash drop && + git stash apply && + test 3 = $(cat file) && + test 2 = $(git show :file) && + test 1 = $(git show HEAD:file) +' + test_expect_success 'drop middle stash by index' ' git reset --hard && echo 8 >file && @@ -236,6 +285,17 @@ setup_stash() test 0 = $(git stash list | wc -l) ' +test_expect_success 'stash pop with stash.index' ' + test_config stash.index true && + git reset --hard && + setup_stash && + git stash pop && + test 3 = $(cat file) && + test 2 = $(git show :file) && + test 1 = $(git show HEAD:file) && + test 0 = $(git stash list | wc -l) +' + cat >expect <new-file && + echo test >file && + git add new-file file && + git rm other-file && + git stash && + + test_config stash.index true && + git stash pop -q >output.out 2>&1 && + echo test >expect && + git show :file >actual && + test_cmp expect actual && + test_must_be_empty output.out +' + test_expect_success 'pop -q --index works and is quiet' ' echo foo >file && git add file && @@ -1178,6 +1254,19 @@ setup_stash() test_path_is_file bar ' +test_expect_success 'stash -- stashes and restores the file with stash.index' ' + test_config stash.index true && + >foo && + >bar && + git add foo bar && + git stash push -- foo && + test_path_is_file bar && + test_path_is_missing foo && + git stash pop --no-index && + test_path_is_file foo && + test_path_is_file bar +' + test_expect_success 'stash -- stashes in subdirectory' ' mkdir sub && >foo && @@ -1194,6 +1283,24 @@ setup_stash() test_path_is_file bar ' +test_expect_success 'stash -- stashes in subdirectory with stash.index' ' + test_config stash.index true && + rm -r sub && + mkdir sub && + >foo && + >bar && + git add foo bar && + ( + cd sub && + git stash push -- ../foo + ) && + test_path_is_file bar && + test_path_is_missing foo && + git stash pop --no-index && + test_path_is_file foo && + test_path_is_file bar +' + test_expect_success 'stash with multiple pathspec arguments' ' >foo && >bar && @@ -1209,6 +1316,22 @@ setup_stash() test_path_is_file extra ' +test_expect_success 'stash with multiple pathspec arguments with stash.index' ' + test_config stash.index true && + >foo && + >bar && + >extra && + git add foo bar extra && + git stash push -- foo bar && + test_path_is_missing bar && + test_path_is_missing foo && + test_path_is_file extra && + git stash pop --no-index && + test_path_is_file foo && + test_path_is_file bar && + test_path_is_file extra +' + test_expect_success 'stash with file including $IFS character' ' >"foo bar" && >foo && @@ -1224,6 +1347,22 @@ setup_stash() test_path_is_file bar ' +test_expect_success 'stash with file including $IFS character with stash.index' ' + test_config stash.index true && + >"foo bar" && + >foo && + >bar && + git add foo* && + git stash push -- "foo b*" && + test_path_is_missing "foo bar" && + test_path_is_file foo && + test_path_is_file bar && + git stash pop --no-index && + test_path_is_file "foo bar" && + test_path_is_file foo && + test_path_is_file bar +' + test_expect_success 'stash with pathspec matching multiple paths' ' echo original >file && echo original >other-file && @@ -1312,6 +1451,22 @@ setup_stash() test_path_is_file bar ' +test_expect_success 'stash without verb with pathspec with stash.index' ' + test_config stash.index true && + >"foo bar" && + >foo && + >bar && + git add foo* && + git stash -- "foo b*" && + test_path_is_missing "foo bar" && + test_path_is_file foo && + test_path_is_file bar && + git stash pop --no-index && + test_path_is_file "foo bar" && + test_path_is_file foo && + test_path_is_file bar +' + test_expect_success 'stash -k -- leaves unstaged files intact' ' git reset && >foo && diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh index ae313e3c70..fe402f6ab5 100755 --- a/t/t3904-stash-patch.sh +++ b/t/t3904-stash-patch.sh @@ -42,6 +42,17 @@ verify_state dir/foo work head ' +test_expect_success 'git stash -p with stash.index' ' + test_config stash.index true && + set_state HEAD HEADfile_work HEADfile_index && + set_state dir/foo work index && + test_write_lines y n y | git stash save -p && + git reset --hard && + git stash apply && + verify_state HEAD HEADfile_work HEADfile_index && + verify_state dir/foo head index +' + test_expect_success 'git stash -p --no-keep-index' ' set_state HEAD HEADfile_work HEADfile_index && set_state bar bar_work bar_index && diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh index 7704709054..5407f11030 100755 --- a/t/t3905-stash-include-untracked.sh +++ b/t/t3905-stash-include-untracked.sh @@ -7,7 +7,7 @@ . ./test-lib.sh -test_expect_success 'stash save --include-untracked some dirty working directory' ' +setup() { echo 1 >file && git add file && test_tick && @@ -23,6 +23,10 @@ git stash --include-untracked && git diff-files --quiet && git diff-index --cached --quiet HEAD +} + +test_expect_success 'stash save --include-untracked some dirty working directory' ' + setup ' test_expect_success 'stash save --include-untracked cleaned the untracked files' ' @@ -108,6 +112,32 @@ test_cmp untracked_expect untracked/untracked ' +test_expect_success 'stash pop after save --include-untracked leaves files untracked again with stash.index' ' + git init repo && + test_when_finished rm -r repo && + ( + cd repo && + git config stash.index true && + setup && + cat >expect <<-EOF && + MM file + ?? HEAD + ?? actual + ?? expect + ?? file2 + ?? untracked/ + EOF + + git stash pop && + git status --porcelain >actual && + test_cmp expect actual && + echo 1 >expect_file2 && + test_cmp expect_file2 file2 && + echo untracked >untracked_expect && + test_cmp untracked_expect untracked/untracked + ) +' + test_expect_success 'clean up untracked/ directory to prepare for next tests' ' git clean --force --quiet -d ' @@ -221,6 +251,22 @@ test_path_is_file bar ' +test_expect_success 'stash push with $IFS character with stash.index' ' + test_config stash.index true && + >"foo bar" && + >foo && + >bar && + git add foo* && + git stash push --include-untracked -- "foo b*" && + test_path_is_missing "foo bar" && + test_path_is_file foo && + test_path_is_file bar && + git stash pop --no-index && + test_path_is_file "foo bar" && + test_path_is_file foo && + test_path_is_file bar +' + test_expect_success 'stash previously ignored file' ' cat >.gitignore <<-EOF && ignored -- 2.48.1 ``` ## Phillip Wood, 2025-09-16 09:18 Subject: Re: [PATCH v2 4/4] stash: honor stash.index in apply, pop modes Message-ID: <25836bc2-db3a-4761-b13d-c587728f4c3c@gmail.com> URL: https://gitlist.dev/e/25836bc2-db3a-4761-b13d-c587728f4c3c%40gmail.com In-Reply-To: <585e124467dcb1ec1de71fa72e749140d44dc367.1757982870.git.ben.knoble+github@gmail.com> ``` Hi Ben On 16/09/2025 01:37, D. Ben Knoble wrote: > With stash.index=true, git-stash(1) command now tries to reinstate the > index by default in the "apply" and "pop" modes. Not doing so creates a > common trap [1], [2]: "git stash apply" is not the reverse of "git stash > push" because carefully staged indices are lost and have to be manually > recreated. OTOH, this mode is not always desirable and may create more > conflicts when applying stashes. As usual, "--no-index" will disable > this behavior if you set "stash.index". I don't have a strong opinion either way on the new config setting but I do think we should rationalize the new tests. Assuming we already have good coverage for "git stash pop --index" then all we need to do is check that "git -c stash.index=true stash pop", "git -c stash.index=true stash pop --no-index" and "git -c stash.index=false stash pop --index". We don't need an exhaustive list of tests that check the config setting in scenarios like "create twos stashes, drop the second one and apply the first". Tests like that add no new coverage for the changes in this patch and slow the test suite down. Thanks Phillip > [1]: https://lore.kernel.org/git/CAPx1GvcxyDDQmCssMjEnt6JoV6qPc5ZUpgPLX3mpUC_4PNYA1w@mail.gmail.com/ > [2]: https://lore.kernel.org/git/c5a811ac-8cd3-c389-ac6d-29020a648c87@gmail.com/ > > Signed-off-by: D. Ben Knoble > --- > Documentation/config/stash.adoc | 5 + > builtin/stash.c | 9 +- > t/t3903-stash.sh | 155 +++++++++++++++++++++++++++++ > t/t3904-stash-patch.sh | 11 ++ > t/t3905-stash-include-untracked.sh | 48 ++++++++- > 5 files changed, 225 insertions(+), 3 deletions(-) > > diff --git a/Documentation/config/stash.adoc b/Documentation/config/stash.adoc > index ec1edaeba6..e556105a15 100644 > --- a/Documentation/config/stash.adoc > +++ b/Documentation/config/stash.adoc > @@ -1,3 +1,8 @@ > +stash.index:: > + If this is set to true, `git stash apply` and `git stash pop` will > + behave as if `--index` was supplied. Defaults to false. See the > + descriptions in 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 > diff --git a/builtin/stash.c b/builtin/stash.c > index d9b478d1d1..8a0eef3c70 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -130,6 +130,7 @@ static struct strbuf stash_index_path = STRBUF_INIT; > static int show_stat = 1; > static int show_patch; > static int show_include_untracked; > +static int use_index; > > /* > * w_commit is set to the commit containing the working tree > @@ -662,7 +663,7 @@ static int apply_stash(int argc, const char **argv, const char *prefix, > { > int ret = -1; > int quiet = 0; > - int index = 0; > + int index = use_index; > struct stash_info info = STASH_INFO_INIT; > struct option options[] = { > OPT__QUIET(&quiet, N_("be quiet, only report errors")), > @@ -759,7 +760,7 @@ static int pop_stash(int argc, const char **argv, const char *prefix, > struct repository *repo UNUSED) > { > int ret = -1; > - int index = 0; > + int index = use_index; > int quiet = 0; > struct stash_info info = STASH_INFO_INIT; > struct option options[] = { > @@ -864,6 +865,10 @@ static int git_stash_config(const char *var, const char *value, > show_include_untracked = git_config_bool(var, value); > return 0; > } > + if (!strcmp(var, "stash.index")) { > + use_index = git_config_bool(var, value); > + return 0; > + } > return git_diff_basic_config(var, value, ctx, cb); > } > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index b8936a653b..1d53a94165 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -111,6 +111,19 @@ setup_stash() > test 1 = $(git show HEAD:file) > ' > > +test_expect_success 'apply stashed changes with stash.index' ' > + test_config stash.index true && > + git reset --hard HEAD^ && > + echo 5 >other-file && > + git add other-file && > + test_tick && > + git commit -m other-file && > + git stash apply && > + test 3 = $(cat file) && > + test 2 = $(git show :file) && > + test 1 = $(git show HEAD:file) > +' > + > test_expect_success 'apply stashed changes (including index)' ' > git reset --hard HEAD^ && > echo 6 >other-file && > @@ -150,6 +163,21 @@ setup_stash() > test 1 = $(git show HEAD:file) > ' > > +test_expect_success 'drop top stash with stash.index' ' > + test_config stash.index true && > + git reset --hard && > + git stash list >expected && > + echo 7 >file && > + git stash && > + git stash drop && > + git stash list >actual && > + test_cmp expected actual && > + git stash apply && > + test 3 = $(cat file) && > + test 2 = $(git show :file) && > + test 1 = $(git show HEAD:file) > +' > + > test_expect_success 'drop middle stash' ' > git reset --hard && > echo 8 >file && > @@ -170,6 +198,27 @@ setup_stash() > test 1 = $(git show HEAD:file) > ' > > +test_expect_success 'drop middle stash with stash.index' ' > + test_config stash.index true && > + git reset --hard && > + echo 8 >file && > + git stash && > + echo 9 >file && > + git stash && > + git stash drop stash@{1} && > + test 2 = $(git stash list | wc -l) && > + git stash apply && > + test 9 = $(cat file) && > + test 1 = $(git show :file) && > + test 1 = $(git show HEAD:file) && > + git reset --hard && > + git stash drop && > + git stash apply && > + test 3 = $(cat file) && > + test 2 = $(git show :file) && > + test 1 = $(git show HEAD:file) > +' > + > test_expect_success 'drop middle stash by index' ' > git reset --hard && > echo 8 >file && > @@ -236,6 +285,17 @@ setup_stash() > test 0 = $(git stash list | wc -l) > ' > > +test_expect_success 'stash pop with stash.index' ' > + test_config stash.index true && > + git reset --hard && > + setup_stash && > + git stash pop && > + test 3 = $(cat file) && > + test 2 = $(git show :file) && > + test 1 = $(git show HEAD:file) && > + test 0 = $(git stash list | wc -l) > +' > + > cat >expect < diff --git a/file2 b/file2 > new file mode 100644 > @@ -328,6 +388,22 @@ setup_stash() > test_must_be_empty output.out > ' > > +test_expect_success 'pop -q works and is quiet with stash.index' ' > + # Added file, deleted file, modified file all staged for commit > + echo foo >new-file && > + echo test >file && > + git add new-file file && > + git rm other-file && > + git stash && > + > + test_config stash.index true && > + git stash pop -q >output.out 2>&1 && > + echo test >expect && > + git show :file >actual && > + test_cmp expect actual && > + test_must_be_empty output.out > +' > + > test_expect_success 'pop -q --index works and is quiet' ' > echo foo >file && > git add file && > @@ -1178,6 +1254,19 @@ setup_stash() > test_path_is_file bar > ' > > +test_expect_success 'stash -- stashes and restores the file with stash.index' ' > + test_config stash.index true && > + >foo && > + >bar && > + git add foo bar && > + git stash push -- foo && > + test_path_is_file bar && > + test_path_is_missing foo && > + git stash pop --no-index && > + test_path_is_file foo && > + test_path_is_file bar > +' > + > test_expect_success 'stash -- stashes in subdirectory' ' > mkdir sub && > >foo && > @@ -1194,6 +1283,24 @@ setup_stash() > test_path_is_file bar > ' > > +test_expect_success 'stash -- stashes in subdirectory with stash.index' ' > + test_config stash.index true && > + rm -r sub && > + mkdir sub && > + >foo && > + >bar && > + git add foo bar && > + ( > + cd sub && > + git stash push -- ../foo > + ) && > + test_path_is_file bar && > + test_path_is_missing foo && > + git stash pop --no-index && > + test_path_is_file foo && > + test_path_is_file bar > +' > + > test_expect_success 'stash with multiple pathspec arguments' ' > >foo && > >bar && > @@ -1209,6 +1316,22 @@ setup_stash() > test_path_is_file extra > ' > > +test_expect_success 'stash with multiple pathspec arguments with stash.index' ' > + test_config stash.index true && > + >foo && > + >bar && > + >extra && > + git add foo bar extra && > + git stash push -- foo bar && > + test_path_is_missing bar && > + test_path_is_missing foo && > + test_path_is_file extra && > + git stash pop --no-index && > + test_path_is_file foo && > + test_path_is_file bar && > + test_path_is_file extra > +' > + > test_expect_success 'stash with file including $IFS character' ' > >"foo bar" && > >foo && > @@ -1224,6 +1347,22 @@ setup_stash() > test_path_is_file bar > ' > > +test_expect_success 'stash with file including $IFS character with stash.index' ' > + test_config stash.index true && > + >"foo bar" && > + >foo && > + >bar && > + git add foo* && > + git stash push -- "foo b*" && > + test_path_is_missing "foo bar" && > + test_path_is_file foo && > + test_path_is_file bar && > + git stash pop --no-index && > + test_path_is_file "foo bar" && > + test_path_is_file foo && > + test_path_is_file bar > +' > + > test_expect_success 'stash with pathspec matching multiple paths' ' > echo original >file && > echo original >other-file && > @@ -1312,6 +1451,22 @@ setup_stash() > test_path_is_file bar > ' > > +test_expect_success 'stash without verb with pathspec with stash.index' ' > + test_config stash.index true && > + >"foo bar" && > + >foo && > + >bar && > + git add foo* && > + git stash -- "foo b*" && > + test_path_is_missing "foo bar" && > + test_path_is_file foo && > + test_path_is_file bar && > + git stash pop --no-index && > + test_path_is_file "foo bar" && > + test_path_is_file foo && > + test_path_is_file bar > +' > + > test_expect_success 'stash -k -- leaves unstaged files intact' ' > git reset && > >foo && > diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh > index ae313e3c70..fe402f6ab5 100755 > --- a/t/t3904-stash-patch.sh > +++ b/t/t3904-stash-patch.sh > @@ -42,6 +42,17 @@ > verify_state dir/foo work head > ' > > +test_expect_success 'git stash -p with stash.index' ' > + test_config stash.index true && > + set_state HEAD HEADfile_work HEADfile_index && > + set_state dir/foo work index && > + test_write_lines y n y | git stash save -p && > + git reset --hard && > + git stash apply && > + verify_state HEAD HEADfile_work HEADfile_index && > + verify_state dir/foo head index > +' > + > test_expect_success 'git stash -p --no-keep-index' ' > set_state HEAD HEADfile_work HEADfile_index && > set_state bar bar_work bar_index && > diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh > index 7704709054..5407f11030 100755 > --- a/t/t3905-stash-include-untracked.sh > +++ b/t/t3905-stash-include-untracked.sh > @@ -7,7 +7,7 @@ > > . ./test-lib.sh > > -test_expect_success 'stash save --include-untracked some dirty working directory' ' > +setup() { > echo 1 >file && > git add file && > test_tick && > @@ -23,6 +23,10 @@ > git stash --include-untracked && > git diff-files --quiet && > git diff-index --cached --quiet HEAD > +} > + > +test_expect_success 'stash save --include-untracked some dirty working directory' ' > + setup > ' > > test_expect_success 'stash save --include-untracked cleaned the untracked files' ' > @@ -108,6 +112,32 @@ > test_cmp untracked_expect untracked/untracked > ' > > +test_expect_success 'stash pop after save --include-untracked leaves files untracked again with stash.index' ' > + git init repo && > + test_when_finished rm -r repo && > + ( > + cd repo && > + git config stash.index true && > + setup && > + cat >expect <<-EOF && > + MM file > + ?? HEAD > + ?? actual > + ?? expect > + ?? file2 > + ?? untracked/ > + EOF > + > + git stash pop && > + git status --porcelain >actual && > + test_cmp expect actual && > + echo 1 >expect_file2 && > + test_cmp expect_file2 file2 && > + echo untracked >untracked_expect && > + test_cmp untracked_expect untracked/untracked > + ) > +' > + > test_expect_success 'clean up untracked/ directory to prepare for next tests' ' > git clean --force --quiet -d > ' > @@ -221,6 +251,22 @@ > test_path_is_file bar > ' > > +test_expect_success 'stash push with $IFS character with stash.index' ' > + test_config stash.index true && > + >"foo bar" && > + >foo && > + >bar && > + git add foo* && > + git stash push --include-untracked -- "foo b*" && > + test_path_is_missing "foo bar" && > + test_path_is_file foo && > + test_path_is_file bar && > + git stash pop --no-index && > + test_path_is_file "foo bar" && > + test_path_is_file foo && > + test_path_is_file bar > +' > + > test_expect_success 'stash previously ignored file' ' > cat >.gitignore <<-EOF && > ignored ``` ## Phillip Wood, 2025-09-16 09:24 Subject: Re: [PATCH v2 0/4] Teach git-stash to use --index from config Message-ID: <4736597d-1144-43ce-8c09-e78e4563765b@gmail.com> URL: https://gitlist.dev/e/4736597d-1144-43ce-8c09-e78e4563765b%40gmail.com In-Reply-To: ``` Hi Ben On 16/09/2025 01:37, D. Ben Knoble wrote: > > PS I've left some new t3903 tests as copy-pasta for now to get feedback > on the rest of the series; there are bits of that file that could use an > update to the modern style (e.g., not using "test 1 = $(cat file)"). > Since some new tests are substantially similar to old tests that use > this style, such cleanup is /probably/ warranted but will delay eyeballs > on the core of this series. In situations like this even if we don't convert the old tests, I think it is worth using a modern style for the new ones. Some people argue that the style within a file should be consistent but in practice that means if someone decides to clean them up in the future they have even more work to do and in the meantime we have a harder time debugging test failures. The cleanups in patches 1 and 2 look very welcome. I've left a comment about the tests on patch 4. Thanks Phillip > Published-as: https://github.com/benknoble/git/tree/stash-apply-index > v1: https://lore.kernel.org/git/20250510183358.36806-1-ben.knoble+github@gmail.com/ > > D. Ben Knoble (4): > t3903: reduce dependencies on previous tests > t3905: remove unneeded blank line > stash: refactor private config globals > stash: honor stash.index in apply, pop modes > > Documentation/config/stash.adoc | 5 + > builtin/stash.c | 17 ++-- > t/t3903-stash.sh | 158 +++++++++++++++++++++++++++++ > t/t3904-stash-patch.sh | 11 ++ > t/t3905-stash-include-untracked.sh | 49 ++++++++- > 5 files changed, 232 insertions(+), 8 deletions(-) > > Diff-intervalle contre v1 : > 1: 1328eb8eac = 1: 1328eb8eac t3903: reduce dependencies on previous tests > 2: 8ac06ad62d = 2: 8ac06ad62d t3905: remove unneeded blank line > 3: c068f1dc0b < -: ---------- BreakingChanges: announce stash {apply,pop} will imply --index > 4: 8caff91c0e < -: ---------- stash: restore the index by default when breaking changes are enabled > 5: 387427bb8c < -: ---------- t0450: mark stash documentation as a known discrepancy > 9: c72a1fe6ea ! 3: bf0a561ce3 t3906: adjust stash submodule tests to account for breaking changes > @@ Metadata > Author: D. Ben Knoble > > ## Commit message ## > - t3906: adjust stash submodule tests to account for breaking changes > + stash: refactor private config globals > > - I cannot explain _why_ this occurs, but it seems that automatically > - unstashing the index from previous commits resolves some known failures > - in t3906 (which are captured by t/lib-submodule-updates.sh). > + A subsequent commit will access a new config variable in the stash > + subcommand implementations, which requires the variables to be declared > + before the relevant functions. Prep with a pure refactoring change to > + consolidate config-related globals with the rest of the globals. > > - In particular: > - - 'replace tracked file with submodule creates empty directory' succeeds > - with breaking changes; > - - all KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR tests > - succeed with breaking changes; > - - all KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES suceed with > - breaking changes. > + Best-viewed-with: --color-moved > > - ## t/lib-submodule-update.sh ## > -@@ t/lib-submodule-update.sh: test_submodule_switch_common () > - ' > - # Replacing a tracked file with a submodule produces an empty > - # directory ... > -- test_expect_$RESULT "$command: replace tracked file with submodule creates empty directory" ' > -+ test_expect_$RESULT !WITH_BREAKING_CHANGES "$command: replace tracked file with submodule creates empty directory" ' > -+ prolog && > -+ reset_work_tree_to replace_sub1_with_file && > -+ ( > -+ cd submodule_update && > -+ git branch -t replace_file_with_sub1 origin/replace_file_with_sub1 && > -+ $command replace_file_with_sub1 && > -+ test_superproject_content origin/replace_file_with_sub1 && > -+ test_dir_is_empty sub1 && > -+ git submodule update --init --recursive && > -+ test_submodule_content sub1 origin/replace_file_with_sub1 > -+ ) > -+ ' > -+ # (unless we automatically unstash the index!) > -+ test_expect_success WITH_BREAKING_CHANGES "$command: replace tracked file with submodule creates empty directory" ' > - prolog && > - reset_work_tree_to replace_sub1_with_file && > - ( > -@@ t/lib-submodule-update.sh: test_submodule_switch_common () > - ' > - # ... as does removing a directory with tracked files with a > - # submodule. > -- if test "$KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR" = 1 > -+ if ! test_have_prereq WITH_BREAKING_CHANGES && > -+ test "$KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR" = 1 > - then > - # Non fast-forward merges fail with "Directory sub1 doesn't > - # exist. sub1" because the empty submodule directory is not > -@@ t/lib-submodule-update.sh: test_submodule_switch_common () > - ' > + ## builtin/stash.c ## > +@@ builtin/stash.c: static const char * const git_stash_save_usage[] = { > + static const char ref_stash[] = "refs/stash"; > + static struct strbuf stash_index_path = STRBUF_INIT; > > - ######################## Disappearing submodule ####################### > -- # Removing a submodule doesn't remove its work tree ... > -- if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1 > -+ # Removing a submodule doesn't remove its work tree (unless stash applies the index!) ... > -+ if ! test_have_prereq WITH_BREAKING_CHANGES && > -+ test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1 > - then > - RESULT="failure" > - else > ++static int show_stat = 1; > ++static int show_patch; > ++static int show_include_untracked; > ++ > + /* > + * w_commit is set to the commit containing the working tree > + * b_commit is set to the base commit > +@@ builtin/stash.c: 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 git_stash_config(const char *var, const char *value, > + const struct config_context *ctx, void *cb) > + { > 6: 0a12983c00 ! 4: 585e124467 t3903: adjust stash test to account for --[no-]index with breaking changes > @@ Metadata > Author: D. Ben Knoble > > ## Commit message ## > - t3903: adjust stash test to account for --[no-]index with breaking changes > + stash: honor stash.index in apply, pop modes > > - A few tests check the results of the index after applying a stash; with > - breaking changes from previous commits that automatically restore the > - stashed index, the expected values are wrong. > + With stash.index=true, git-stash(1) command now tries to reinstate the > + index by default in the "apply" and "pop" modes. Not doing so creates a > + common trap [1], [2]: "git stash apply" is not the reverse of "git stash > + push" because carefully staged indices are lost and have to be manually > + recreated. OTOH, this mode is not always desirable and may create more > + conflicts when applying stashes. As usual, "--no-index" will disable > + this behavior if you set "stash.index". > > - A few of the relevant tests check the restoration of s; with > - the aforementioned breaking changes, things get more interesting. In > - particular, if we "git stash push -- foo" but have "bar" in the index, > - then when applying the stash we get a conflict: "bar" was not removed > - from the index by the stash, but it was included in the recorded index > - in the stash. In those cases, apply the stash with "--no-index" (which > - would be the required user behavior). > + [1]: https://lore.kernel.org/git/CAPx1GvcxyDDQmCssMjEnt6JoV6qPc5ZUpgPLX3mpUC_4PNYA1w@mail.gmail.com/ > + [2]: https://lore.kernel.org/git/c5a811ac-8cd3-c389-ac6d-29020a648c87@gmail.com/ > > - Signed-off-by: D. Ben Knoble > + ## Documentation/config/stash.adoc ## > +@@ > ++stash.index:: > ++ If this is set to true, `git stash apply` and `git stash pop` will > ++ behave as if `--index` was supplied. Defaults to false. See the > ++ descriptions in 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 > > - > - ## Notes ## > - It looks like the pathspec filtering is not applied to the stashed > - index; should it be? > + ## builtin/stash.c ## > +@@ builtin/stash.c: static struct strbuf stash_index_path = STRBUF_INIT; > + static int show_stat = 1; > + static int show_patch; > + static int show_include_untracked; > ++static int use_index; > + > + /* > + * w_commit is set to the commit containing the working tree > +@@ builtin/stash.c: static int apply_stash(int argc, const char **argv, const char *prefix, > + { > + int ret = -1; > + int quiet = 0; > +- int index = 0; > ++ int index = use_index; > + struct stash_info info = STASH_INFO_INIT; > + struct option options[] = { > + OPT__QUIET(&quiet, N_("be quiet, only report errors")), > +@@ builtin/stash.c: static int pop_stash(int argc, const char **argv, const char *prefix, > + struct repository *repo UNUSED) > + { > + int ret = -1; > +- int index = 0; > ++ int index = use_index; > + int quiet = 0; > + struct stash_info info = STASH_INFO_INIT; > + struct option options[] = { > +@@ builtin/stash.c: static int git_stash_config(const char *var, const char *value, > + show_include_untracked = git_config_bool(var, value); > + return 0; > + } > ++ if (!strcmp(var, "stash.index")) { > ++ use_index = git_config_bool(var, value); > ++ return 0; > ++ } > + return git_diff_basic_config(var, value, ctx, cb); > + } > + > > ## t/t3903-stash.sh ## > -@@ t/t3903-stash.sh: setup_stash() > - test_cmp expect file > - ' > - > --test_expect_success 'apply stashed changes' ' > -+test_expect_success !WITH_BREAKING_CHANGES 'apply stashed changes' ' > - git reset --hard && > - echo 5 >other-file && > - git add other-file && > @@ t/t3903-stash.sh: setup_stash() > test 1 = $(git show HEAD:file) > ' > > -+test_expect_success WITH_BREAKING_CHANGES 'apply stashed changes' ' > -+ git reset --hard && > ++test_expect_success 'apply stashed changes with stash.index' ' > ++ test_config stash.index true && > ++ git reset --hard HEAD^ && > + echo 5 >other-file && > + git add other-file && > + test_tick && > @@ t/t3903-stash.sh: setup_stash() > test_expect_success 'apply stashed changes (including index)' ' > git reset --hard HEAD^ && > echo 6 >other-file && > -@@ t/t3903-stash.sh: setup_stash() > - test_must_fail git stash drop --foo > - ' > - > --test_expect_success 'drop top stash' ' > -+test_expect_success !WITH_BREAKING_CHANGES 'drop top stash' ' > - git reset --hard && > - git stash list >expected && > - echo 7 >file && > @@ t/t3903-stash.sh: setup_stash() > test 1 = $(git show HEAD:file) > ' > > --test_expect_success 'drop middle stash' ' > -+test_expect_success WITH_BREAKING_CHANGES 'drop top stash' ' > ++test_expect_success 'drop top stash with stash.index' ' > ++ test_config stash.index true && > + git reset --hard && > + git stash list >expected && > + echo 7 >file && > @@ t/t3903-stash.sh: setup_stash() > + test 1 = $(git show HEAD:file) > +' > + > -+test_expect_success !WITH_BREAKING_CHANGES 'drop middle stash' ' > + test_expect_success 'drop middle stash' ' > git reset --hard && > echo 8 >file && > - git stash && > @@ t/t3903-stash.sh: setup_stash() > test 1 = $(git show HEAD:file) > ' > > --test_expect_success 'drop middle stash by index' ' > -+test_expect_success WITH_BREAKING_CHANGES 'drop middle stash' ' > ++test_expect_success 'drop middle stash with stash.index' ' > ++ test_config stash.index true && > + git reset --hard && > + echo 8 >file && > + git stash && > @@ t/t3903-stash.sh: setup_stash() > + test 1 = $(git show HEAD:file) > +' > + > -+test_expect_success !WITH_BREAKING_CHANGES 'drop middle stash by index' ' > + test_expect_success 'drop middle stash by index' ' > git reset --hard && > echo 8 >file && > - git stash && > -@@ t/t3903-stash.sh: setup_stash() > - test_cmp expect actual > - ' > - > --test_expect_success 'stash pop' ' > -+test_expect_success !WITH_BREAKING_CHANGES 'stash pop' ' > - git reset --hard && > - git stash pop && > - test 3 = $(cat file) && > @@ t/t3903-stash.sh: setup_stash() > test 0 = $(git stash list | wc -l) > ' > > -+test_expect_success WITH_BREAKING_CHANGES 'stash pop' ' > ++test_expect_success 'stash pop with stash.index' ' > ++ test_config stash.index true && > + git reset --hard && > ++ setup_stash && > + git stash pop && > + test 3 = $(cat file) && > + test 2 = $(git show :file) && > @@ t/t3903-stash.sh: setup_stash() > test_must_be_empty output.out > ' > > --test_expect_success 'pop -q works and is quiet' ' > -+test_expect_success !WITH_BREAKING_CHANGES 'pop -q works and is quiet' ' > - git stash pop -q >output.out 2>&1 && > - echo bar >expect && > - git show :file >actual && > -@@ t/t3903-stash.sh: setup_stash() > - test_must_be_empty output.out > - ' > - > -+test_expect_success WITH_BREAKING_CHANGES 'pop -q works and is quiet' ' > ++test_expect_success 'pop -q works and is quiet with stash.index' ' > ++ # Added file, deleted file, modified file all staged for commit > ++ echo foo >new-file && > ++ echo test >file && > ++ git add new-file file && > ++ git rm other-file && > ++ git stash && > ++ > ++ test_config stash.index true && > + git stash pop -q >output.out 2>&1 && > + echo test >expect && > + git show :file >actual && > @@ t/t3903-stash.sh: setup_stash() > test_expect_success 'pop -q --index works and is quiet' ' > echo foo >file && > git add file && > -@@ t/t3903-stash.sh: setup_stash() > - test_cmp expect actual > - ' > - > --test_expect_success 'stash -- stashes and restores the file' ' > -+test_expect_success !WITH_BREAKING_CHANGES 'stash -- stashes and restores the file' ' > - >foo && > - >bar && > - git add foo bar && > @@ t/t3903-stash.sh: setup_stash() > test_path_is_file bar > ' > > --test_expect_success 'stash -- stashes in subdirectory' ' > -+test_expect_success WITH_BREAKING_CHANGES 'stash -- stashes and restores the file' ' > ++test_expect_success 'stash -- stashes and restores the file with stash.index' ' > ++ test_config stash.index true && > + >foo && > + >bar && > + git add foo bar && > @@ t/t3903-stash.sh: setup_stash() > + test_path_is_file bar > +' > + > -+test_expect_success !WITH_BREAKING_CHANGES 'stash -- stashes in subdirectory' ' > + test_expect_success 'stash -- stashes in subdirectory' ' > mkdir sub && > >foo && > - >bar && > @@ t/t3903-stash.sh: setup_stash() > test_path_is_file bar > ' > > --test_expect_success 'stash with multiple pathspec arguments' ' > -+test_expect_success WITH_BREAKING_CHANGES 'stash -- stashes in subdirectory' ' > ++test_expect_success 'stash -- stashes in subdirectory with stash.index' ' > ++ test_config stash.index true && > ++ rm -r sub && > + mkdir sub && > + >foo && > + >bar && > @@ t/t3903-stash.sh: setup_stash() > + test_path_is_file bar > +' > + > -+test_expect_success !WITH_BREAKING_CHANGES 'stash with multiple pathspec arguments' ' > + test_expect_success 'stash with multiple pathspec arguments' ' > >foo && > >bar && > - >extra && > @@ t/t3903-stash.sh: setup_stash() > test_path_is_file extra > ' > > --test_expect_success 'stash with file including $IFS character' ' > -+test_expect_success WITH_BREAKING_CHANGES 'stash with multiple pathspec arguments' ' > ++test_expect_success 'stash with multiple pathspec arguments with stash.index' ' > ++ test_config stash.index true && > + >foo && > + >bar && > + >extra && > @@ t/t3903-stash.sh: setup_stash() > + test_path_is_file extra > +' > + > -+test_expect_success !WITH_BREAKING_CHANGES 'stash with file including $IFS character' ' > + test_expect_success 'stash with file including $IFS character' ' > >"foo bar" && > >foo && > - >bar && > @@ t/t3903-stash.sh: setup_stash() > test_path_is_file bar > ' > > -+test_expect_success WITH_BREAKING_CHANGES 'stash with file including $IFS character' ' > ++test_expect_success 'stash with file including $IFS character with stash.index' ' > ++ test_config stash.index true && > + >"foo bar" && > + >foo && > + >bar && > @@ t/t3903-stash.sh: setup_stash() > test_expect_success 'stash with pathspec matching multiple paths' ' > echo original >file && > echo original >other-file && > -@@ t/t3903-stash.sh: setup_stash() > - test_path_is_file untracked > - ' > - > --test_expect_success 'stash without verb with pathspec' ' > -+test_expect_success !WITH_BREAKING_CHANGES 'stash without verb with pathspec' ' > - >"foo bar" && > - >foo && > - >bar && > @@ t/t3903-stash.sh: setup_stash() > test_path_is_file bar > ' > > -+test_expect_success WITH_BREAKING_CHANGES 'stash without verb with pathspec' ' > ++test_expect_success 'stash without verb with pathspec with stash.index' ' > ++ test_config stash.index true && > + >"foo bar" && > + >foo && > + >bar && > @@ t/t3903-stash.sh: setup_stash() > test_expect_success 'stash -k -- leaves unstaged files intact' ' > git reset && > >foo && > + > + ## t/t3904-stash-patch.sh ## > +@@ > + verify_state dir/foo work head > + ' > + > ++test_expect_success 'git stash -p with stash.index' ' > ++ test_config stash.index true && > ++ set_state HEAD HEADfile_work HEADfile_index && > ++ set_state dir/foo work index && > ++ test_write_lines y n y | git stash save -p && > ++ git reset --hard && > ++ git stash apply && > ++ verify_state HEAD HEADfile_work HEADfile_index && > ++ verify_state dir/foo head index > ++' > ++ > + test_expect_success 'git stash -p --no-keep-index' ' > + set_state HEAD HEADfile_work HEADfile_index && > + set_state bar bar_work bar_index && > + > + ## t/t3905-stash-include-untracked.sh ## > +@@ > + > + . ./test-lib.sh > + > +-test_expect_success 'stash save --include-untracked some dirty working directory' ' > ++setup() { > + echo 1 >file && > + git add file && > + test_tick && > +@@ > + git stash --include-untracked && > + git diff-files --quiet && > + git diff-index --cached --quiet HEAD > ++} > ++ > ++test_expect_success 'stash save --include-untracked some dirty working directory' ' > ++ setup > + ' > + > + test_expect_success 'stash save --include-untracked cleaned the untracked files' ' > +@@ > + test_cmp untracked_expect untracked/untracked > + ' > + > ++test_expect_success 'stash pop after save --include-untracked leaves files untracked again with stash.index' ' > ++ git init repo && > ++ test_when_finished rm -r repo && > ++ ( > ++ cd repo && > ++ git config stash.index true && > ++ setup && > ++ cat >expect <<-EOF && > ++ MM file > ++ ?? HEAD > ++ ?? actual > ++ ?? expect > ++ ?? file2 > ++ ?? untracked/ > ++ EOF > ++ > ++ git stash pop && > ++ git status --porcelain >actual && > ++ test_cmp expect actual && > ++ echo 1 >expect_file2 && > ++ test_cmp expect_file2 file2 && > ++ echo untracked >untracked_expect && > ++ test_cmp untracked_expect untracked/untracked > ++ ) > ++' > ++ > + test_expect_success 'clean up untracked/ directory to prepare for next tests' ' > + git clean --force --quiet -d > + ' > +@@ > + test_path_is_file bar > + ' > + > ++test_expect_success 'stash push with $IFS character with stash.index' ' > ++ test_config stash.index true && > ++ >"foo bar" && > ++ >foo && > ++ >bar && > ++ git add foo* && > ++ git stash push --include-untracked -- "foo b*" && > ++ test_path_is_missing "foo bar" && > ++ test_path_is_file foo && > ++ test_path_is_file bar && > ++ git stash pop --no-index && > ++ test_path_is_file "foo bar" && > ++ test_path_is_file foo && > ++ test_path_is_file bar > ++' > ++ > + test_expect_success 'stash previously ignored file' ' > + cat >.gitignore <<-EOF && > + ignored > 7: 4b92d47e16 < -: ---------- t3904: adjust stash -p test to account for index states with breaking changes > 8: af14dee1be < -: ---------- t3905: adjust stash -u tests for breaking changes > > base-commit: 1ee85f0e215f22b0878d0ad4b2445d12bbb63887 ``` ## Junio C Hamano, 2025-09-16 16:49 Subject: Re: [PATCH v2 0/4] Teach git-stash to use --index from config Message-ID: URL: https://gitlist.dev/e/xmqq8qieny0o.fsf%40gitster.g In-Reply-To: ``` "D. Ben Knoble" writes: > With stash.index=true, git-stash(1) command now tries to reinstate the > index by default in the "apply" and "pop" modes. Not doing so creates a > common trap: "git stash apply" is not the reverse of "git stash push" > because carefully staged indices are lost and have to be manually > recreated. OTOH, this mode is not always desirable and may create more > conflicts when applying stashes. Use "--no-index" to disable this behavior. I've read the patches and I think I can agree with all the changes proposed. We might eventually flip the default, but we do not truly know until we unleash the version with choices to end users. I agree with Phillip's comment on minimum tests to ensure that the interaction between configuration variables and the command option is sane. Thanks. ``` ## D. Ben Knoble, 2025-09-16 17:06 Subject: Re: [PATCH v2 0/4] Teach git-stash to use --index from config Message-ID: URL: https://gitlist.dev/e/CALnO6CCpcy2E4Q4u3%2BfVTtb3iNOCPUx2fdL%2BLgd-%2BvWhptK2EQ%40mail.gmail.com In-Reply-To: <4736597d-1144-43ce-8c09-e78e4563765b@gmail.com> ``` On Tue, Sep 16, 2025 at 5:25 AM Phillip Wood wrote: > > Hi Ben > > On 16/09/2025 01:37, D. Ben Knoble wrote: > > > > PS I've left some new t3903 tests as copy-pasta for now to get feedback > > on the rest of the series; there are bits of that file that could use an > > update to the modern style (e.g., not using "test 1 = $(cat file)"). > > Since some new tests are substantially similar to old tests that use > > this style, such cleanup is /probably/ warranted but will delay eyeballs > > on the core of this series. > > In situations like this even if we don't convert the old tests, I think > it is worth using a modern style for the new ones. Some people argue > that the style within a file should be consistent but in practice that > means if someone decides to clean them up in the future they have even > more work to do and in the meantime we have a harder time debugging test > failures. Agreed—with Junio's "mostly good" downthread, I'll work on a v3 which _at least_ does the right thing for new tests. No promises on the old ones… > > The cleanups in patches 1 and 2 look very welcome. I've left a comment > about the tests on patch 4. Thanks, will look. ``` ## D. Ben Knoble, 2025-09-16 17:07 Subject: Re: [PATCH v2 4/4] stash: honor stash.index in apply, pop modes Message-ID: URL: https://gitlist.dev/e/CALnO6CByUNHWFRYBSOpP-uD8moBrm48UW7k0MaGTUtL%3DbDL6GQ%40mail.gmail.com In-Reply-To: <25836bc2-db3a-4761-b13d-c587728f4c3c@gmail.com> ``` On Tue, Sep 16, 2025 at 5:18 AM Phillip Wood wrote: > > Hi Ben > > On 16/09/2025 01:37, D. Ben Knoble wrote: > > With stash.index=true, git-stash(1) command now tries to reinstate the > > index by default in the "apply" and "pop" modes. Not doing so creates a > > common trap [1], [2]: "git stash apply" is not the reverse of "git stash > > push" because carefully staged indices are lost and have to be manually > > recreated. OTOH, this mode is not always desirable and may create more > > conflicts when applying stashes. As usual, "--no-index" will disable > > this behavior if you set "stash.index". > > I don't have a strong opinion either way on the new config setting but I > do think we should rationalize the new tests. Assuming we already have > good coverage for "git stash pop --index" then all we need to do is > check that "git -c stash.index=true stash pop", "git -c stash.index=true > stash pop --no-index" and "git -c stash.index=false stash pop --index". > We don't need an exhaustive list of tests that check the config setting > in scenarios like "create twos stashes, drop the second one and apply > the first". Tests like that add no new coverage for the changes in this > patch and slow the test suite down. Ah, yep. That's much saner. I started from a "git reset @{u}" of the original series, so it was "cheaper" to keep that copy-pasta. But I much prefer your idea. Thanks! ``` ## D. Ben Knoble, 2025-09-22 01:39 Subject: [PATCH v3 0/4] Teach git-stash to use --index from config Message-ID: URL: https://gitlist.dev/e/cover.1758505011.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` Changes from v2: - use "rational" tests (check interactions between CLI, config; drop duplicate tests) thanks to Phillip's review Changes from v1: - configure --index via config - drop BreakingChanges related work With stash.index=true, git-stash(1) command now tries to reinstate the index by default in the "apply" and "pop" modes. Not doing so creates a common trap: "git stash apply" is not the reverse of "git stash push" because carefully staged indices are lost and have to be manually recreated. OTOH, this mode is not always desirable and may create more conflicts when applying stashes. Use "--no-index" to disable this behavior. Cf. , . PS I've left some new t3903 tests as copy-pasta for now to get feedback on the rest of the series; there are bits of that file that could use an update to the modern style (e.g., not using "test 1 = $(cat file)"). Since some new tests are substantially similar to old tests that use this style, such cleanup is /probably/ warranted but will delay eyeballs on the core of this series. Published-as: https://github.com/benknoble/git/tree/stash-apply-index v1: https://lore.kernel.org/git/20250510183358.36806-1-ben.knoble+github@gmail.com/ v2: https://lore.kernel.org/git/cover.1757982870.git.ben.knoble+github@gmail.com/ D. Ben Knoble (4): t3903: reduce dependencies on previous tests t3905: remove unneeded blank line stash: refactor private config globals stash: honor stash.index in apply, pop modes Documentation/config/stash.adoc | 5 ++++ builtin/stash.c | 17 ++++++++----- t/t3903-stash.sh | 40 ++++++++++++++++++++++++++++++ t/t3905-stash-include-untracked.sh | 1 - 4 files changed, 56 insertions(+), 7 deletions(-) Diff-intervalle contre v2 : 1: 1328eb8eac = 1: 1328eb8eac t3903: reduce dependencies on previous tests 2: 8ac06ad62d = 2: 8ac06ad62d t3905: remove unneeded blank line 3: bf0a561ce3 = 3: bf0a561ce3 stash: refactor private config globals 4: 585e124467 ! 4: 8e6cafbf3a stash: honor stash.index in apply, pop modes @@ builtin/stash.c: static int git_stash_config(const char *var, const char *value, ## t/t3903-stash.sh ## @@ t/t3903-stash.sh: setup_stash() - test 1 = $(git show HEAD:file) + ) ' -+test_expect_success 'apply stashed changes with stash.index' ' -+ test_config stash.index true && -+ git reset --hard HEAD^ && -+ echo 5 >other-file && -+ git add other-file && -+ test_tick && -+ git commit -m other-file && -+ git stash apply && -+ test 3 = $(cat file) && -+ test 2 = $(git show :file) && -+ test 1 = $(git show HEAD:file) -+' -+ - test_expect_success 'apply stashed changes (including index)' ' - git reset --hard HEAD^ && - echo 6 >other-file && -@@ t/t3903-stash.sh: setup_stash() - test 1 = $(git show HEAD:file) - ' - -+test_expect_success 'drop top stash with stash.index' ' -+ test_config stash.index true && -+ git reset --hard && -+ git stash list >expected && -+ echo 7 >file && -+ git stash && -+ git stash drop && -+ git stash list >actual && -+ test_cmp expected actual && -+ git stash apply && -+ test 3 = $(cat file) && -+ test 2 = $(git show :file) && -+ test 1 = $(git show HEAD:file) -+' -+ - test_expect_success 'drop middle stash' ' - git reset --hard && - echo 8 >file && -@@ t/t3903-stash.sh: setup_stash() - test 1 = $(git show HEAD:file) - ' - -+test_expect_success 'drop middle stash with stash.index' ' -+ test_config stash.index true && -+ git reset --hard && -+ echo 8 >file && -+ git stash && -+ echo 9 >file && -+ git stash && -+ git stash drop stash@{1} && -+ test 2 = $(git stash list | wc -l) && -+ git stash apply && -+ test 9 = $(cat file) && -+ test 1 = $(git show :file) && -+ test 1 = $(git show HEAD:file) && -+ git reset --hard && -+ git stash drop && -+ git stash apply && -+ test 3 = $(cat file) && -+ test 2 = $(git show :file) && -+ test 1 = $(git show HEAD:file) -+' -+ - test_expect_success 'drop middle stash by index' ' - git reset --hard && - echo 8 >file && -@@ t/t3903-stash.sh: setup_stash() - test 0 = $(git stash list | wc -l) - ' - -+test_expect_success 'stash pop with stash.index' ' -+ test_config stash.index true && -+ git reset --hard && -+ setup_stash && -+ git stash pop && -+ test 3 = $(cat file) && -+ test 2 = $(git show :file) && -+ test 1 = $(git show HEAD:file) && -+ test 0 = $(git stash list | wc -l) -+' -+ - cat >expect <new-file && -+ echo test >file && -+ git add new-file file && -+ git rm other-file && ++test_expect_success 'stash.index=true implies --index' ' ++ # setup for a few related tests ++ test_commit file base && ++ echo index >file && ++ git add file && ++ echo working >file && + git stash && + -+ test_config stash.index true && -+ git stash pop -q >output.out 2>&1 && -+ echo test >expect && -+ git show :file >actual && ++ test_when_finished "git reset --hard" && ++ git -c stash.index=true stash apply && ++ echo index >expect && ++ git show :0:file >actual && + test_cmp expect actual && -+ test_must_be_empty output.out ++ echo working >expect && ++ test_cmp expect file +' + - test_expect_success 'pop -q --index works and is quiet' ' - echo foo >file && - git add file && -@@ t/t3903-stash.sh: setup_stash() - test_path_is_file bar - ' - -+test_expect_success 'stash -- stashes and restores the file with stash.index' ' -+ test_config stash.index true && -+ >foo && -+ >bar && -+ git add foo bar && -+ git stash push -- foo && -+ test_path_is_file bar && -+ test_path_is_missing foo && -+ git stash pop --no-index && -+ test_path_is_file foo && -+ test_path_is_file bar ++test_expect_success 'stash.index=true overridden by --no-index' ' ++ test_when_finished "git reset --hard" && ++ git -c stash.index=true stash apply --no-index && ++ echo base >expect && ++ git show :0:file >actual && ++ test_cmp expect actual && ++ echo working >expect && ++ test_cmp expect file +' + - test_expect_success 'stash -- stashes in subdirectory' ' - mkdir sub && - >foo && -@@ t/t3903-stash.sh: setup_stash() - test_path_is_file bar - ' - -+test_expect_success 'stash -- stashes in subdirectory with stash.index' ' -+ test_config stash.index true && -+ rm -r sub && -+ mkdir sub && -+ >foo && -+ >bar && -+ git add foo bar && -+ ( -+ cd sub && -+ git stash push -- ../foo -+ ) && -+ test_path_is_file bar && -+ test_path_is_missing foo && -+ git stash pop --no-index && -+ test_path_is_file foo && -+ test_path_is_file bar ++test_expect_success 'stash.index=false overridden by --index' ' ++ test_when_finished "git reset --hard" && ++ git -c stash.index=false stash apply --index && ++ echo index >expect && ++ git show :0:file >actual && ++ test_cmp expect actual && ++ echo working >expect && ++ test_cmp expect file +' + - test_expect_success 'stash with multiple pathspec arguments' ' - >foo && - >bar && -@@ t/t3903-stash.sh: setup_stash() - test_path_is_file extra - ' - -+test_expect_success 'stash with multiple pathspec arguments with stash.index' ' -+ test_config stash.index true && -+ >foo && -+ >bar && -+ >extra && -+ git add foo bar extra && -+ git stash push -- foo bar && -+ test_path_is_missing bar && -+ test_path_is_missing foo && -+ test_path_is_file extra && -+ git stash pop --no-index && -+ test_path_is_file foo && -+ test_path_is_file bar && -+ test_path_is_file extra -+' -+ - test_expect_success 'stash with file including $IFS character' ' - >"foo bar" && - >foo && -@@ t/t3903-stash.sh: setup_stash() - test_path_is_file bar - ' - -+test_expect_success 'stash with file including $IFS character with stash.index' ' -+ test_config stash.index true && -+ >"foo bar" && -+ >foo && -+ >bar && -+ git add foo* && -+ git stash push -- "foo b*" && -+ test_path_is_missing "foo bar" && -+ test_path_is_file foo && -+ test_path_is_file bar && -+ git stash pop --no-index && -+ test_path_is_file "foo bar" && -+ test_path_is_file foo && -+ test_path_is_file bar -+' -+ - test_expect_success 'stash with pathspec matching multiple paths' ' - echo original >file && - echo original >other-file && -@@ t/t3903-stash.sh: setup_stash() - test_path_is_file bar - ' - -+test_expect_success 'stash without verb with pathspec with stash.index' ' -+ test_config stash.index true && -+ >"foo bar" && -+ >foo && -+ >bar && -+ git add foo* && -+ git stash -- "foo b*" && -+ test_path_is_missing "foo bar" && -+ test_path_is_file foo && -+ test_path_is_file bar && -+ git stash pop --no-index && -+ test_path_is_file "foo bar" && -+ test_path_is_file foo && -+ test_path_is_file bar -+' -+ - test_expect_success 'stash -k -- leaves unstaged files intact' ' - git reset && - >foo && - - ## t/t3904-stash-patch.sh ## -@@ - verify_state dir/foo work head - ' - -+test_expect_success 'git stash -p with stash.index' ' -+ test_config stash.index true && -+ set_state HEAD HEADfile_work HEADfile_index && -+ set_state dir/foo work index && -+ test_write_lines y n y | git stash save -p && -+ git reset --hard && -+ git stash apply && -+ verify_state HEAD HEADfile_work HEADfile_index && -+ verify_state dir/foo head index -+' -+ - test_expect_success 'git stash -p --no-keep-index' ' - set_state HEAD HEADfile_work HEADfile_index && - set_state bar bar_work bar_index && - - ## t/t3905-stash-include-untracked.sh ## -@@ - - . ./test-lib.sh - --test_expect_success 'stash save --include-untracked some dirty working directory' ' -+setup() { - echo 1 >file && - git add file && - test_tick && -@@ - git stash --include-untracked && - git diff-files --quiet && - git diff-index --cached --quiet HEAD -+} -+ -+test_expect_success 'stash save --include-untracked some dirty working directory' ' -+ setup - ' - - test_expect_success 'stash save --include-untracked cleaned the untracked files' ' -@@ - test_cmp untracked_expect untracked/untracked - ' - -+test_expect_success 'stash pop after save --include-untracked leaves files untracked again with stash.index' ' -+ git init repo && -+ test_when_finished rm -r repo && -+ ( -+ cd repo && -+ git config stash.index true && -+ setup && -+ cat >expect <<-EOF && -+ MM file -+ ?? HEAD -+ ?? actual -+ ?? expect -+ ?? file2 -+ ?? untracked/ -+ EOF -+ -+ git stash pop && -+ git status --porcelain >actual && -+ test_cmp expect actual && -+ echo 1 >expect_file2 && -+ test_cmp expect_file2 file2 && -+ echo untracked >untracked_expect && -+ test_cmp untracked_expect untracked/untracked -+ ) -+' -+ - test_expect_success 'clean up untracked/ directory to prepare for next tests' ' - git clean --force --quiet -d - ' -@@ - test_path_is_file bar - ' - -+test_expect_success 'stash push with $IFS character with stash.index' ' -+ test_config stash.index true && -+ >"foo bar" && -+ >foo && -+ >bar && -+ git add foo* && -+ git stash push --include-untracked -- "foo b*" && -+ test_path_is_missing "foo bar" && -+ test_path_is_file foo && -+ test_path_is_file bar && -+ git stash pop --no-index && -+ test_path_is_file "foo bar" && -+ test_path_is_file foo && -+ test_path_is_file bar -+' -+ - test_expect_success 'stash previously ignored file' ' - cat >.gitignore <<-EOF && - ignored + test_done base-commit: 1ee85f0e215f22b0878d0ad4b2445d12bbb63887 -- 2.48.1 ``` ## D. Ben Knoble, 2025-09-22 01:39 Subject: [PATCH v3 1/4] t3903: reduce dependencies on previous tests Message-ID: <1328eb8eac65ec7bbf1f3cb257d7221a372c45c3.1758505011.git.ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/1328eb8eac65ec7bbf1f3cb257d7221a372c45c3.1758505011.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` Skipping previous tests to work through only failing tests with arguments like --run=4,122- causes some tests to fail because subdir doesn't exist yet (it is created by a previous test; typically "unstashing in a subdirectory"). Create it on demand for tests that need it, but don't fail (-p) if the directory already exists. Signed-off-by: D. Ben Knoble --- t/t3903-stash.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 74666ff3e4..b8936a653b 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -895,6 +895,7 @@ setup_stash() test_expect_success 'apply: show same status as git status (relative to ./)' ' git stash clear && + mkdir -p subdir && echo 1 >subdir/subfile1 && echo 2 >subdir/subfile2 && git add subdir/subfile1 && @@ -1327,6 +1328,7 @@ setup_stash() test_expect_success 'stash -- leaves untracked files in subdir intact' ' git reset && + mkdir -p subdir && >subdir/untracked && >subdir/tracked1 && >subdir/tracked2 && @@ -1343,6 +1345,7 @@ setup_stash() test_expect_success 'stash -- works with binary files' ' git reset && + mkdir -p subdir && >subdir/untracked && >subdir/tracked && cp "$TEST_DIRECTORY"/test-binary-1.png subdir/tracked-binary && -- 2.48.1 ``` ## D. Ben Knoble, 2025-09-22 01:39 Subject: [PATCH v3 2/4] t3905: remove unneeded blank line Message-ID: <8ac06ad62df581bb84ecdca0b0144c218daa0733.1758505011.git.ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/8ac06ad62df581bb84ecdca0b0144c218daa0733.1758505011.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` This is leftover from 787513027a (stash: Add --include-untracked option to stash and remove all untracked files, 2011-06-24) when it was converted in bbaa45c3aa (t3905: move all commands into test cases, 2021-02-08). Signed-off-by: D. Ben Knoble --- t/t3905-stash-include-untracked.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh index 1289ae3e07..7704709054 100755 --- a/t/t3905-stash-include-untracked.sh +++ b/t/t3905-stash-include-untracked.sh @@ -87,7 +87,6 @@ test_expect_success 'clean up untracked/untracked file to prepare for next tests' ' git clean --force --quiet - ' test_expect_success 'stash pop after save --include-untracked leaves files untracked again' ' -- 2.48.1 ``` ## D. Ben Knoble, 2025-09-22 01:39 Subject: [PATCH v3 3/4] stash: refactor private config globals Message-ID: URL: https://gitlist.dev/e/bf0a561ce36afe14b6bc03bf851d19b30ef491ad.1758505011.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` A subsequent commit will access a new config variable in the stash subcommand implementations, which requires the variables to be declared before the relevant functions. Prep with a pure refactoring change to consolidate config-related globals with the rest of the globals. Best-viewed-with: --color-moved Signed-off-by: D. Ben Knoble --- builtin/stash.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index cfbd92852a..d9b478d1d1 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -127,6 +127,10 @@ static const char * const git_stash_save_usage[] = { static const char ref_stash[] = "refs/stash"; static struct strbuf stash_index_path = STRBUF_INIT; +static int show_stat = 1; +static int show_patch; +static int show_include_untracked; + /* * w_commit is set to the commit containing the working tree * b_commit is set to the base commit @@ -845,10 +849,6 @@ 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 git_stash_config(const char *var, const char *value, const struct config_context *ctx, void *cb) { -- 2.48.1 ``` ## D. Ben Knoble, 2025-09-22 01:39 Subject: [PATCH v3 4/4] stash: honor stash.index in apply, pop modes Message-ID: <8e6cafbf3a01b968663b65559acf3df615eecbad.1758505011.git.ben.knoble+github@gmail.com> URL: https://gitlist.dev/e/8e6cafbf3a01b968663b65559acf3df615eecbad.1758505011.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` With stash.index=true, git-stash(1) command now tries to reinstate the index by default in the "apply" and "pop" modes. Not doing so creates a common trap [1], [2]: "git stash apply" is not the reverse of "git stash push" because carefully staged indices are lost and have to be manually recreated. OTOH, this mode is not always desirable and may create more conflicts when applying stashes. As usual, "--no-index" will disable this behavior if you set "stash.index". [1]: https://lore.kernel.org/git/CAPx1GvcxyDDQmCssMjEnt6JoV6qPc5ZUpgPLX3mpUC_4PNYA1w@mail.gmail.com/ [2]: https://lore.kernel.org/git/c5a811ac-8cd3-c389-ac6d-29020a648c87@gmail.com/ Signed-off-by: D. Ben Knoble --- Documentation/config/stash.adoc | 5 +++++ builtin/stash.c | 9 ++++++-- t/t3903-stash.sh | 37 +++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/Documentation/config/stash.adoc b/Documentation/config/stash.adoc index ec1edaeba6..e556105a15 100644 --- a/Documentation/config/stash.adoc +++ b/Documentation/config/stash.adoc @@ -1,3 +1,8 @@ +stash.index:: + If this is set to true, `git stash apply` and `git stash pop` will + behave as if `--index` was supplied. Defaults to false. See the + descriptions in 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 diff --git a/builtin/stash.c b/builtin/stash.c index d9b478d1d1..8a0eef3c70 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -130,6 +130,7 @@ static struct strbuf stash_index_path = STRBUF_INIT; static int show_stat = 1; static int show_patch; static int show_include_untracked; +static int use_index; /* * w_commit is set to the commit containing the working tree @@ -662,7 +663,7 @@ static int apply_stash(int argc, const char **argv, const char *prefix, { int ret = -1; int quiet = 0; - int index = 0; + int index = use_index; struct stash_info info = STASH_INFO_INIT; struct option options[] = { OPT__QUIET(&quiet, N_("be quiet, only report errors")), @@ -759,7 +760,7 @@ static int pop_stash(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) { int ret = -1; - int index = 0; + int index = use_index; int quiet = 0; struct stash_info info = STASH_INFO_INIT; struct option options[] = { @@ -864,6 +865,10 @@ static int git_stash_config(const char *var, const char *value, show_include_untracked = git_config_bool(var, value); return 0; } + if (!strcmp(var, "stash.index")) { + use_index = git_config_bool(var, value); + return 0; + } return git_diff_basic_config(var, value, ctx, cb); } diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index b8936a653b..d6127173b1 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1595,4 +1595,41 @@ setup_stash() ) ' +test_expect_success 'stash.index=true implies --index' ' + # setup for a few related tests + test_commit file base && + echo index >file && + git add file && + echo working >file && + git stash && + + test_when_finished "git reset --hard" && + git -c stash.index=true stash apply && + echo index >expect && + git show :0:file >actual && + test_cmp expect actual && + echo working >expect && + test_cmp expect file +' + +test_expect_success 'stash.index=true overridden by --no-index' ' + test_when_finished "git reset --hard" && + git -c stash.index=true stash apply --no-index && + echo base >expect && + git show :0:file >actual && + test_cmp expect actual && + echo working >expect && + test_cmp expect file +' + +test_expect_success 'stash.index=false overridden by --index' ' + test_when_finished "git reset --hard" && + git -c stash.index=false stash apply --index && + echo index >expect && + git show :0:file >actual && + test_cmp expect actual && + echo working >expect && + test_cmp expect file +' + test_done -- 2.48.1 ``` ## Phillip Wood, 2025-09-22 14:11 Subject: Re: [PATCH v3 4/4] stash: honor stash.index in apply, pop modes Message-ID: URL: https://gitlist.dev/e/d7721018-3b2f-4764-b33d-4fc6bdc034a1%40gmail.com In-Reply-To: <8e6cafbf3a01b968663b65559acf3df615eecbad.1758505011.git.ben.knoble+github@gmail.com> ``` Hi Ben On 22/09/2025 02:39, D. Ben Knoble wrote: > With stash.index=true, git-stash(1) command now tries to reinstate the > index by default in the "apply" and "pop" modes. Not doing so creates a > common trap [1], [2]: "git stash apply" is not the reverse of "git stash > push" because carefully staged indices are lost and have to be manually > recreated. OTOH, this mode is not always desirable and may create more > conflicts when applying stashes. As usual, "--no-index" will disable > this behavior if you set "stash.index". Thanks for updating the tests, they look good. As I said before I don't have a strong opinion about this change but I certainly don't object to it. I think this change will also affect the behavior of "git merge/pull/rebase --autostash" which we should maybe call out in the commit message. I don't think that change in behavior is a problem as it is probably what the user would expect when they set this config. Thanks Phillip > [1]: https://lore.kernel.org/git/CAPx1GvcxyDDQmCssMjEnt6JoV6qPc5ZUpgPLX3mpUC_4PNYA1w@mail.gmail.com/ > [2]: https://lore.kernel.org/git/c5a811ac-8cd3-c389-ac6d-29020a648c87@gmail.com/ > > Signed-off-by: D. Ben Knoble > --- > Documentation/config/stash.adoc | 5 +++++ > builtin/stash.c | 9 ++++++-- > t/t3903-stash.sh | 37 +++++++++++++++++++++++++++++++++ > 3 files changed, 49 insertions(+), 2 deletions(-) > > diff --git a/Documentation/config/stash.adoc b/Documentation/config/stash.adoc > index ec1edaeba6..e556105a15 100644 > --- a/Documentation/config/stash.adoc > +++ b/Documentation/config/stash.adoc > @@ -1,3 +1,8 @@ > +stash.index:: > + If this is set to true, `git stash apply` and `git stash pop` will > + behave as if `--index` was supplied. Defaults to false. See the > + descriptions in 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 > diff --git a/builtin/stash.c b/builtin/stash.c > index d9b478d1d1..8a0eef3c70 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -130,6 +130,7 @@ static struct strbuf stash_index_path = STRBUF_INIT; > static int show_stat = 1; > static int show_patch; > static int show_include_untracked; > +static int use_index; > > /* > * w_commit is set to the commit containing the working tree > @@ -662,7 +663,7 @@ static int apply_stash(int argc, const char **argv, const char *prefix, > { > int ret = -1; > int quiet = 0; > - int index = 0; > + int index = use_index; > struct stash_info info = STASH_INFO_INIT; > struct option options[] = { > OPT__QUIET(&quiet, N_("be quiet, only report errors")), > @@ -759,7 +760,7 @@ static int pop_stash(int argc, const char **argv, const char *prefix, > struct repository *repo UNUSED) > { > int ret = -1; > - int index = 0; > + int index = use_index; > int quiet = 0; > struct stash_info info = STASH_INFO_INIT; > struct option options[] = { > @@ -864,6 +865,10 @@ static int git_stash_config(const char *var, const char *value, > show_include_untracked = git_config_bool(var, value); > return 0; > } > + if (!strcmp(var, "stash.index")) { > + use_index = git_config_bool(var, value); > + return 0; > + } > return git_diff_basic_config(var, value, ctx, cb); > } > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index b8936a653b..d6127173b1 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -1595,4 +1595,41 @@ setup_stash() > ) > ' > > +test_expect_success 'stash.index=true implies --index' ' > + # setup for a few related tests > + test_commit file base && > + echo index >file && > + git add file && > + echo working >file && > + git stash && > + > + test_when_finished "git reset --hard" && > + git -c stash.index=true stash apply && > + echo index >expect && > + git show :0:file >actual && > + test_cmp expect actual && > + echo working >expect && > + test_cmp expect file > +' > + > +test_expect_success 'stash.index=true overridden by --no-index' ' > + test_when_finished "git reset --hard" && > + git -c stash.index=true stash apply --no-index && > + echo base >expect && > + git show :0:file >actual && > + test_cmp expect actual && > + echo working >expect && > + test_cmp expect file > +' > + > +test_expect_success 'stash.index=false overridden by --index' ' > + test_when_finished "git reset --hard" && > + git -c stash.index=false stash apply --index && > + echo index >expect && > + git show :0:file >actual && > + test_cmp expect actual && > + echo working >expect && > + test_cmp expect file > +' > + > test_done ``` ## D. Ben Knoble, 2025-09-24 20:40 Subject: Re: [PATCH v3 4/4] stash: honor stash.index in apply, pop modes Message-ID: URL: https://gitlist.dev/e/CALnO6CA9z39rgGd673p-ZMzMOBM_G37YPOs4ysNtPHGGCDVM-A%40mail.gmail.com In-Reply-To: ``` On Mon, Sep 22, 2025 at 10:11 AM Phillip Wood wrote: > > Hi Ben > > On 22/09/2025 02:39, D. Ben Knoble wrote: > > With stash.index=true, git-stash(1) command now tries to reinstate the > > index by default in the "apply" and "pop" modes. Not doing so creates a > > common trap [1], [2]: "git stash apply" is not the reverse of "git stash > > push" because carefully staged indices are lost and have to be manually > > recreated. OTOH, this mode is not always desirable and may create more > > conflicts when applying stashes. As usual, "--no-index" will disable > > this behavior if you set "stash.index". > > Thanks for updating the tests, they look good. As I said before I don't > have a strong opinion about this change but I certainly don't object to > it. I think this change will also affect the behavior of "git > merge/pull/rebase --autostash" which we should maybe call out in the > commit message. I don't think that change in behavior is a problem as it > is probably what the user would expect when they set this config. Agreed, I hadn't considered that here. Should we also update the docs, do you think? ``` ## Phillip Wood, 2025-09-29 10:01 Subject: Re: [PATCH v3 4/4] stash: honor stash.index in apply, pop modes Message-ID: URL: https://gitlist.dev/e/b36d2dbf-7cf4-46cd-81bb-b502e0eed380%40gmail.com In-Reply-To: ``` On 24/09/2025 21:40, D. Ben Knoble wrote: > On Mon, Sep 22, 2025 at 10:11 AM Phillip Wood wrote: >> >> Hi Ben >> >> On 22/09/2025 02:39, D. Ben Knoble wrote: >>> With stash.index=true, git-stash(1) command now tries to reinstate the >>> index by default in the "apply" and "pop" modes. Not doing so creates a >>> common trap [1], [2]: "git stash apply" is not the reverse of "git stash >>> push" because carefully staged indices are lost and have to be manually >>> recreated. OTOH, this mode is not always desirable and may create more >>> conflicts when applying stashes. As usual, "--no-index" will disable >>> this behavior if you set "stash.index". >> >> Thanks for updating the tests, they look good. As I said before I don't >> have a strong opinion about this change but I certainly don't object to >> it. I think this change will also affect the behavior of "git >> merge/pull/rebase --autostash" which we should maybe call out in the >> commit message. I don't think that change in behavior is a problem as it >> is probably what the user would expect when they set this config. > > Agreed, I hadn't considered that here. Should we also update the docs, > do you think? Good idea, we should certainly mention it in the documentation for the new config setting, I'm not sure whether it is worth mentioning it in the "--autostash" documentation for the individual commands. Thanks Phillip ``` ## D. Ben Knoble, 2025-10-06 12:59 Subject: [PATCH] doc: explain the impact of stash.index on --autostash options Message-ID: URL: https://gitlist.dev/e/d4a277b6b0695d86636562f4c07efae17f9249f9.1759755379.git.ben.knoble%2Bgithub%40gmail.com In-Reply-To: ``` With 9842c0c749 (stash: honor stash.index in apply, pop modes, 2025-09-21) merged in a5d4779e6e (Merge branch 'dk/stash-apply-index', 2025-09-29), we did not advertise the connection between the new config option stash.index and the implicit use of git-stash via --autostash (which may also be configured). Do so. Signed-off-by: D. Ben Knoble --- This builds on dk/stash-apply-index from gitster/git and is published at https://github.com/benknoble/git/tree/stash-apply-index-doc Documentation/config/stash.adoc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/config/stash.adoc b/Documentation/config/stash.adoc index e556105a15..fcb9a4a7a0 100644 --- a/Documentation/config/stash.adoc +++ b/Documentation/config/stash.adoc @@ -2,6 +2,10 @@ stash.index:: If this is set to true, `git stash apply` and `git stash pop` will behave as if `--index` was supplied. Defaults to false. See the descriptions in linkgit:git-stash[1]. ++ +This also affects invocations of linkgit:git-stash[1] via `--autostash` from +commands like linkgit:git-merge[1], linkgit:git-rebase[1], and +linkgit:git-pull[1]. stash.showIncludeUntracked:: If this is set to true, the `git stash show` command will show base-commit: 9842c0c7492d2858d64ef81128f7b1f0b38e326b -- 2.48.1 ``` ## Kristoffer Haugsbakk, 2025-10-09 22:54 Subject: Re: [PATCH] doc: explain the impact of stash.index on --autostash options Message-ID: URL: https://gitlist.dev/e/e77d07d0-c26e-4d91-9777-ebaffffd764c%40app.fastmail.com In-Reply-To: ``` This follow-up patch makes sense. • It reads like a logical continuation of the previous commit 9842c0c749 • The log message is clear (and with no spelling mistakes) • The markup is correct (list continuation, links) • `make lint-docs` passes • `./ci/check-whitespace.sh @^` passes On Mon, Oct 6, 2025, at 14:59, D. Ben Knoble wrote: > With 9842c0c749 (stash: honor stash.index in apply, pop modes, > 2025-09-21) Curiously, since this is also the base commit, referring to “the previous commit” would also work if this patch is indeed applied on top of that one. But maybe that contextual reference is a bad idea? > merged in a5d4779e6e (Merge branch 'dk/stash-apply-index', > 2025-09-29), This is over-specified IMO. Like mentioned this patch could be applied on top of commit 9842c0c749. Then that merge commit will not be reachable from this resulting commit. I also don’t see the point of mentioning when things were merged in in the commit message. > we did not advertise the connection between the new config > option stash.index and the implicit use of git-stash via --autostash > (which may also be configured). Do so. > > Signed-off-by: D. Ben Knoble > --- > > This builds on dk/stash-apply-index from gitster/git and is published at > https://github.com/benknoble/git/tree/stash-apply-index-doc > > Documentation/config/stash.adoc | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/config/stash.adoc b/Documentation/config/stash.adoc > index e556105a15..fcb9a4a7a0 100644 > --- a/Documentation/config/stash.adoc > +++ b/Documentation/config/stash.adoc > @@ -2,6 +2,10 @@ stash.index:: > If this is set to true, `git stash apply` and `git stash pop` will > behave as if `--index` was supplied. Defaults to false. See the > descriptions in linkgit:git-stash[1]. > ++ > +This also affects invocations of linkgit:git-stash[1] via `--autostash` from > +commands like linkgit:git-merge[1], linkgit:git-rebase[1], and > +linkgit:git-pull[1]. According to these • `git grep -- --autostash` • `git grep merge-options.adoc` This text exhaustively covers all commands which have this option. ... which might mean that “like” is an unneeded hedge? (it’s probably not intended to be a hedge) > > stash.showIncludeUntracked:: > If this is set to true, the `git stash show` command will show > > base-commit: 9842c0c7492d2858d64ef81128f7b1f0b38e326b > -- > 2.48.1 ``` ## D. Ben Knoble, 2025-10-11 14:44 Subject: Re: [PATCH] doc: explain the impact of stash.index on --autostash options Message-ID: URL: https://gitlist.dev/e/CALnO6CA0SEGrzsrS_yqZ3Ztep0Gi%3DzPFjjGzSW94EvDW8RZ2Bg%40mail.gmail.com In-Reply-To: ``` On Thu, Oct 9, 2025 at 6:55 PM Kristoffer Haugsbakk wrote: > > This follow-up patch makes sense. > > • It reads like a logical continuation of the previous commit 9842c0c749 > • The log message is clear (and with no spelling mistakes) > • The markup is correct (list continuation, links) > • `make lint-docs` passes > • `./ci/check-whitespace.sh @^` passes > > On Mon, Oct 6, 2025, at 14:59, D. Ben Knoble wrote: > > With 9842c0c749 (stash: honor stash.index in apply, pop modes, > > 2025-09-21) > > Curiously, since this is also the base commit, referring to “the > previous commit” would also work if this patch is indeed applied on top > of that one. But maybe that contextual reference is a bad idea? Generally I think so, but that's just my preference. Once commits have stable reference points, I'd rather use that. But I'm not attached to this one, so if we end up re-rolling, I can adjust either way. > > merged in a5d4779e6e (Merge branch 'dk/stash-apply-index', > > 2025-09-29), > > This is over-specified IMO. Like mentioned this patch could be applied > on top of commit 9842c0c749. Then that merge commit will not be > reachable from this resulting commit. > > I also don’t see the point of mentioning when things were merged in in > the commit message. Indeed. I think I wanted to call out the topic branch this was part of, especially since my understanding of the process of queueing patches on top of in-flight topics is shaky from parts of Documentation/SubmittingPatches and …/howto/maintain-git: * A topic already in 'next' can get fixes while still in 'next'. Such a topic will have many merges to 'next' (in other words, "git log --first-parent next" will show many "Merge branch 'ai/topic' to next" for the same topic. So, idk. If the eventual merge to master won't have the prior "topic merge," it's probably important to omit (since the final topology of master won't contain the referenced commit). In this case, since that merge _is_ part of master, it seemed worth explaining what topic we were improving. > > diff --git a/Documentation/config/stash.adoc b/Documentation/config/stash.adoc > > index e556105a15..fcb9a4a7a0 100644 > > --- a/Documentation/config/stash.adoc > > +++ b/Documentation/config/stash.adoc > > @@ -2,6 +2,10 @@ stash.index:: > > If this is set to true, `git stash apply` and `git stash pop` will > > behave as if `--index` was supplied. Defaults to false. See the > > descriptions in linkgit:git-stash[1]. > > ++ > > +This also affects invocations of linkgit:git-stash[1] via `--autostash` from > > +commands like linkgit:git-merge[1], linkgit:git-rebase[1], and > > +linkgit:git-pull[1]. > > According to these > > • `git grep -- --autostash` > • `git grep merge-options.adoc` > > This text exhaustively covers all commands which have this option. > > ... which might mean that “like” is an unneeded hedge? (it’s probably > not intended to be a hedge) Indeed. I'm not sure where autostash might be introduced in the future (git-history?), so it might be more "hedge" by way of "listing examples" than necessary. I could go either way on all of this, so will defer to guidance from others (but don't have the impetus to rewrite without a strong opinion at the moment). ``` ## Junio C Hamano, 2025-10-11 17:28 Subject: Re: [PATCH] doc: explain the impact of stash.index on --autostash options Message-ID: URL: https://gitlist.dev/e/xmqqy0ph5ov8.fsf%40gitster.g In-Reply-To: ``` "D. Ben Knoble" writes: > On Thu, Oct 9, 2025 at 6:55 PM Kristoffer Haugsbakk > wrote: >> >> This follow-up patch makes sense. >> >> • It reads like a logical continuation of the previous commit 9842c0c749 >> • The log message is clear (and with no spelling mistakes) >> • The markup is correct (list continuation, links) >> • `make lint-docs` passes >> • `./ci/check-whitespace.sh @^` passes >> >> On Mon, Oct 6, 2025, at 14:59, D. Ben Knoble wrote: >> > With 9842c0c749 (stash: honor stash.index in apply, pop modes, >> > 2025-09-21) >> >> Curiously, since this is also the base commit, referring to “the >> previous commit” would also work if this patch is indeed applied on top >> of that one. But maybe that contextual reference is a bad idea? > Generally I think so, but that's just my preference. Once commits have > stable reference points, I'd rather use that. But I'm not attached to > this one, so if we end up re-rolling, I can adjust either way. That matches my preference, too. Use of relative "previous" or "next", unless they are in the same series, can make things confusing. For example, you could say something silly like "The test added in the previous commit revealed age-old bug. Here is a fix and more test", and the fix may be important enough that it wants to be forked from a maintenance track that is far older than the "previous" commit. >> This is over-specified IMO. Like mentioned this patch could be applied >> on top of commit 9842c0c749. Then that merge commit will not be >> reachable from this resulting commit. >> >> I also don’t see the point of mentioning when things were merged in in >> the commit message. Yeah, that is less useful to me (there is a tool, given a commit object, to figure out at which merge it got merged to the mainline); I didn't think of a way the information can be useful to general readers. If the mainline merge was a release or more ago, then it may make sense to say "commit X, which appeared in version Y, was broken in such and such way, and here is to fix its breakage". >> > If this is set to true, `git stash apply` and `git stash pop` will >> > behave as if `--index` was supplied. Defaults to false. See the >> > descriptions in linkgit:git-stash[1]. >> > ++ >> > +This also affects invocations of linkgit:git-stash[1] via `--autostash` from >> > +commands like linkgit:git-merge[1], linkgit:git-rebase[1], and >> > +linkgit:git-pull[1]. >> >> According to these >> >> • `git grep -- --autostash` >> • `git grep merge-options.adoc` >> >> This text exhaustively covers all commands which have this option. >> >> ... which might mean that “like” is an unneeded hedge? (it’s probably >> not intended to be a hedge) You have to devise a way to somehow ensure that a list, which happens to be exhaustive right now, stays exhaustive, if you present it as authoritative exhaustive list to your readers. But as long as the list covers the use cases majority of readers would encounter every day, it does not make the list any less useful even if it were not exhausitive (and does not pretend to be). So I prefer to keep the presented text than making it sound an authoritative exhaustive list and add maintenance cost. Thanks. ``` ## Ben Knoble, 2025-10-12 18:04 Subject: Re: [PATCH] doc: explain the impact of stash.index on --autostash options Message-ID: URL: https://gitlist.dev/e/B78CD1B1-67C1-4EA1-94A7-13AD40C43585%40gmail.com In-Reply-To: ``` > > Le 11 oct. 2025 à 13:29, Junio C Hamano a écrit : > > "D. Ben Knoble" writes: > >>> On Thu, Oct 9, 2025 at 6:55 PM Kristoffer Haugsbakk >>> wrote: >>> >>> This is over-specified IMO. Like mentioned this patch could be applied >>> on top of commit 9842c0c749. Then that merge commit will not be >>> reachable from this resulting commit. >>> >>> I also don’t see the point of mentioning when things were merged in in >>> the commit message. > > Yeah, that is less useful to me (there is a tool, given a commit > object, to figure out at which merge it got merged to the mainline); > I didn't think of a way the information can be useful to general > readers. If the mainline merge was a release or more ago, then it > may make sense to say "commit X, which appeared in version Y, was > broken in such and such way, and here is to fix its breakage" It’s not clear to me if you’d like to see a v2 or not, but given you were already happy to merge in the latest what’s cooking, I don’t mind not sending one ;) ``` ## Kristoffer Haugsbakk, 2025-10-13 12:17 Subject: Re: [PATCH] doc: explain the impact of stash.index on --autostash options Message-ID: URL: https://gitlist.dev/e/d4dc8042-b996-4889-88cb-f0862a12d09c%40app.fastmail.com In-Reply-To: ``` On Sun, Oct 12, 2025, at 20:04, Ben Knoble wrote: >>>[snip] >> >> Yeah, that is less useful to me (there is a tool, given a commit >> object, to figure out at which merge it got merged to the mainline); >> I didn't think of a way the information can be useful to general >> readers. If the mainline merge was a release or more ago, then it >> may make sense to say "commit X, which appeared in version Y, was >> broken in such and such way, and here is to fix its breakage" > > It’s not clear to me if you’d like to see a v2 or not, but given you > were already happy to merge in the latest what’s cooking, I don’t mind > not sending one ;) Sorry. I misread a What’s Cooking email[1] as asking why a topic hadn’t been picked up. But you were asking about sending another patch on top of something which was already in `next`. I wouldn’t have sent that email if I knew it was already in `next` (if I had checked better). ``` ## D. Ben Knoble, 2025-10-14 02:18 Subject: Re: [PATCH] doc: explain the impact of stash.index on --autostash options Message-ID: URL: https://gitlist.dev/e/CALnO6CDULtw1UmqHfiqLCH4v6U%3DMuiECHhYwz1-Omkh8x%3D9yFg%40mail.gmail.com In-Reply-To: ``` On Mon, Oct 13, 2025 at 8:18 AM Kristoffer Haugsbakk wrote: > > On Sun, Oct 12, 2025, at 20:04, Ben Knoble wrote: > >>>[snip] > >> > >> Yeah, that is less useful to me (there is a tool, given a commit > >> object, to figure out at which merge it got merged to the mainline); > >> I didn't think of a way the information can be useful to general > >> readers. If the mainline merge was a release or more ago, then it > >> may make sense to say "commit X, which appeared in version Y, was > >> broken in such and such way, and here is to fix its breakage" > > > > It’s not clear to me if you’d like to see a v2 or not, but given you > > were already happy to merge in the latest what’s cooking, I don’t mind > > not sending one ;) > > Sorry. I misread a What’s Cooking email[1] as asking why a topic hadn’t > been picked up. But you were asking about sending another patch on top > of something which was already in `next`. Apologies. That's probably my poor wording. > I wouldn’t have sent that email if I knew it was already in `next` (if I > had checked better). Anything I can do to make it easier next time? Appreciate the review, btw :) -- D. Ben Knoble ```