* [PATCH 0/9] make stash apply with --index by default
@ 2025-05-10 18:33 D. Ben Knoble
2025-05-10 18:33 ` [PATCH 1/9] t3903: reduce dependencies on previous tests D. Ben Knoble
` (12 more replies)
0 siblings, 13 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-10 18:33 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/9] t3903: reduce dependencies on previous tests
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
@ 2025-05-10 18:33 ` D. Ben Knoble
2025-05-10 18:33 ` [PATCH 2/9] t3905: remove unneeded blank line D. Ben Knoble
` (11 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-10 18:33 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano
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 <ben.knoble+github@gmail.com>
---
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 -- <subdir> 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 -- <subdir> 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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 2/9] t3905: remove unneeded blank line
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
2025-05-10 18:33 ` [PATCH 1/9] t3903: reduce dependencies on previous tests D. Ben Knoble
@ 2025-05-10 18:33 ` D. Ben Knoble
2025-05-10 18:33 ` [PATCH 3/9] BreakingChanges: announce stash {apply,pop} will imply --index D. Ben Knoble
` (10 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-10 18:33 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Denton Liu, Junio C Hamano
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 <ben.knoble+github@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 3/9] BreakingChanges: announce stash {apply,pop} will imply --index
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
2025-05-10 18:33 ` [PATCH 1/9] t3903: reduce dependencies on previous tests D. Ben Knoble
2025-05-10 18:33 ` [PATCH 2/9] t3905: remove unneeded blank line D. Ben Knoble
@ 2025-05-10 18:33 ` D. Ben Knoble
2025-05-10 18:33 ` [PATCH 4/9] stash: restore the index by default when breaking changes are enabled D. Ben Knoble
` (9 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-10 18:33 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Patrick Steinhardt, Junio C Hamano
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 <ben.knoble+github@gmail.com>
---
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>,
<CA+EOSBncr=4a4d8n9xS4FNehyebpmX8JiUwCsXD47EQDE+DiUQ@mail.gmail.com>.
+* 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. <CAPx1GvcxyDDQmCssMjEnt6JoV6qPc5ZUpgPLX3mpUC_4PNYA1w@mail.gmail.com>,
+<c5a811ac-8cd3-c389-ac6d-29020a648c87@gmail.com>.
+
=== Removals
* Support for grafting commits has long been superseded by git-replace(1).
--
2.48.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 4/9] stash: restore the index by default when breaking changes are enabled
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
` (2 preceding siblings ...)
2025-05-10 18:33 ` [PATCH 3/9] BreakingChanges: announce stash {apply,pop} will imply --index D. Ben Knoble
@ 2025-05-10 18:33 ` D. Ben Knoble
2025-05-10 18:33 ` [PATCH 5/9] t0450: mark stash documentation as a known discrepancy D. Ben Knoble
` (8 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-10 18:33 UTC (permalink / raw)
To: git
Cc: D. Ben Knoble, Junio C Hamano, Patrick Steinhardt, Karthik Nayak,
Denton Liu, Kyle Meyer, Ævar Arnfjörð Bjarmason
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 <ben.knoble+github@gmail.com>
---
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 [<log-options>]
'git stash' show [-u | --include-untracked | --only-untracked] [<diff-options>] [<stash>]
'git stash' drop [-q | --quiet] [<stash>]
+ifndef::with-breaking-changes[]
'git stash' pop [--index] [-q | --quiet] [<stash>]
'git stash' apply [--index] [-q | --quiet] [<stash>]
+endif::with-breaking-changes[]
+ifdef::with-breaking-changes[]
+'git stash' pop [--no-index] [-q | --quiet] [<stash>]
+'git stash' apply [--no-index] [-q | --quiet] [<stash>]
+endif::with-breaking-changes[]
'git stash' branch <branchname> [<stash>]
'git stash' [push [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]
[-u | --include-untracked] [-a | --all] [(-m | --message) <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] [<diff-options>] [<stash>]")
#define BUILTIN_STASH_DROP_USAGE \
N_("git stash drop [-q | --quiet] [<stash>]")
+#ifdef WITH_BREAKING_CHANGES
+#define BUILTIN_STASH_POP_USAGE \
+ N_("git stash pop [--no-index] [-q | --quiet] [<stash>]")
+#define BUILTIN_STASH_APPLY_USAGE \
+ N_("git stash apply [--no-index] [-q | --quiet] [<stash>]")
+#else
#define BUILTIN_STASH_POP_USAGE \
N_("git stash pop [--index] [-q | --quiet] [<stash>]")
#define BUILTIN_STASH_APPLY_USAGE \
N_("git stash apply [--index] [-q | --quiet] [<stash>]")
+#endif /* WITH_BREAKING_CHANGES */
#define BUILTIN_STASH_BRANCH_USAGE \
N_("git stash branch <branchname> [<stash>]")
#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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 5/9] t0450: mark stash documentation as a known discrepancy
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
` (3 preceding siblings ...)
2025-05-10 18:33 ` [PATCH 4/9] stash: restore the index by default when breaking changes are enabled D. Ben Knoble
@ 2025-05-10 18:33 ` D. Ben Knoble
2025-05-10 18:33 ` [PATCH 6/9] t3903: adjust stash test to account for --[no-]index with breaking changes D. Ben Knoble
` (7 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-10 18:33 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano,
Ævar Arnfjörð Bjarmason
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 <ben.knoble+github@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 6/9] t3903: adjust stash test to account for --[no-]index with breaking changes
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
` (4 preceding siblings ...)
2025-05-10 18:33 ` [PATCH 5/9] t0450: mark stash documentation as a known discrepancy D. Ben Knoble
@ 2025-05-10 18:33 ` D. Ben Knoble
2025-05-10 18:33 ` [PATCH 7/9] t3904: adjust stash -p test to account for index states " D. Ben Knoble
` (6 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-10 18:33 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano, John Cai
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 <pathspec>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 <ben.knoble+github@gmail.com>
---
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 <<EOF
diff --git a/file2 b/file2
new file mode 100644
@@ -320,7 +375,7 @@ 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 &&
@@ -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 -- <pathspec> stashes and restores the file' '
+test_expect_success !WITH_BREAKING_CHANGES 'stash -- <pathspec> 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 -- <pathspec> stashes in subdirectory' '
+test_expect_success WITH_BREAKING_CHANGES 'stash -- <pathspec> 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 -- <pathspec> 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 -- <pathspec> 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 -- <pathspec> leaves unstaged files intact' '
git reset &&
>foo &&
--
2.48.1
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 7/9] t3904: adjust stash -p test to account for index states with breaking changes
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
` (5 preceding siblings ...)
2025-05-10 18:33 ` [PATCH 6/9] t3903: adjust stash test to account for --[no-]index with breaking changes D. Ben Knoble
@ 2025-05-10 18:33 ` D. Ben Knoble
2025-05-10 18:33 ` [PATCH 8/9] t3905: adjust stash -u tests for " D. Ben Knoble
` (5 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-10 18:33 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 8/9] t3905: adjust stash -u tests for breaking changes
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
` (6 preceding siblings ...)
2025-05-10 18:33 ` [PATCH 7/9] t3904: adjust stash -p test to account for index states " D. Ben Knoble
@ 2025-05-10 18:33 ` D. Ben Knoble
2025-05-10 18:33 ` [PATCH 9/9] t3906: adjust stash submodule tests to account " D. Ben Knoble
` (4 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-10 18:33 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Denton Liu, Junio C Hamano
Like previous commits, adjust the expected results of application of
stashes with --no-index as needed, with <pathspec> 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 <ben.knoble+github@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 9/9] t3906: adjust stash submodule tests to account for breaking changes
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
` (7 preceding siblings ...)
2025-05-10 18:33 ` [PATCH 8/9] t3905: adjust stash -u tests for " D. Ben Knoble
@ 2025-05-10 18:33 ` D. Ben Knoble
2025-05-12 12:52 ` [PATCH 0/9] make stash apply with --index by default Junio C Hamano
` (3 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-10 18:33 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano
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 <ben.knoble+github@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 0/9] make stash apply with --index by default
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
` (8 preceding siblings ...)
2025-05-10 18:33 ` [PATCH 9/9] t3906: adjust stash submodule tests to account " D. Ben Knoble
@ 2025-05-12 12:52 ` Junio C Hamano
2025-05-20 14:36 ` D. Ben Knoble
` (2 subsequent siblings)
12 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2025-05-12 12:52 UTC (permalink / raw)
To: D. Ben Knoble; +Cc: git
"D. Ben Knoble" <ben.knoble+github@gmail.com> 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).
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/9] make stash apply with --index by default
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
` (9 preceding siblings ...)
2025-05-12 12:52 ` [PATCH 0/9] make stash apply with --index by default Junio C Hamano
@ 2025-05-20 14:36 ` D. Ben Knoble
2025-05-20 14:39 ` D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 0/4] Teach git-stash to use --index from config D. Ben Knoble
12 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-20 14:36 UTC (permalink / raw)
To: git
On Sat, May 10, 2025 at 2:34 PM D. Ben Knoble
<ben.knoble+github@gmail.com> 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
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/9] make stash apply with --index by default
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
` (10 preceding siblings ...)
2025-05-20 14:36 ` D. Ben Knoble
@ 2025-05-20 14:39 ` D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 0/4] Teach git-stash to use --index from config D. Ben Knoble
12 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-05-20 14:39 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Ævar Arnfjörð Bjarmason,
Denton Liu, Junio C Hamano, John Cai
[Apologies for re-send; mis-clicked]
On Sat, May 10, 2025 at 2:34 PM D. Ben Knoble
<ben.knoble+github@gmail.com> 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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 0/4] Teach git-stash to use --index from config
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
` (11 preceding siblings ...)
2025-05-20 14:39 ` D. Ben Knoble
@ 2025-09-16 0:37 ` D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 1/4] t3903: reduce dependencies on previous tests D. Ben Knoble
` (6 more replies)
12 siblings, 7 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-16 0:37 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano
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. <CAPx1GvcxyDDQmCssMjEnt6JoV6qPc5ZUpgPLX3mpUC_4PNYA1w@mail.gmail.com>,
<c5a811ac-8cd3-c389-ac6d-29020a648c87@gmail.com>.
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 <ben.knoble+github@gmail.com>
## 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 <ben.knoble+github@gmail.com>
## 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 <pathspec>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 <ben.knoble+github@gmail.com>
+ ## 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 -- <pathspec> stashes and restores the file' '
-+test_expect_success !WITH_BREAKING_CHANGES 'stash -- <pathspec> 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 -- <pathspec> stashes in subdirectory' '
-+test_expect_success WITH_BREAKING_CHANGES 'stash -- <pathspec> stashes and restores the file' '
++test_expect_success 'stash -- <pathspec> 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 -- <pathspec> stashes in subdirectory' '
+ test_expect_success 'stash -- <pathspec> 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 -- <pathspec> stashes in subdirectory' '
++test_expect_success 'stash -- <pathspec> 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 -- <pathspec> 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
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 1/4] t3903: reduce dependencies on previous tests
2025-09-16 0:37 ` [PATCH v2 0/4] Teach git-stash to use --index from config D. Ben Knoble
@ 2025-09-16 0:37 ` D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 2/4] t3905: remove unneeded blank line D. Ben Knoble
` (5 subsequent siblings)
6 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-16 0:37 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano
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 <ben.knoble+github@gmail.com>
---
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 -- <subdir> 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 -- <subdir> 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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 2/4] t3905: remove unneeded blank line
2025-09-16 0:37 ` [PATCH v2 0/4] Teach git-stash to use --index from config D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 1/4] t3903: reduce dependencies on previous tests D. Ben Knoble
@ 2025-09-16 0:37 ` D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 3/4] stash: refactor private config globals D. Ben Knoble
` (4 subsequent siblings)
6 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-16 0:37 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano, Denton Liu
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 <ben.knoble+github@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 3/4] stash: refactor private config globals
2025-09-16 0:37 ` [PATCH v2 0/4] Teach git-stash to use --index from config D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 1/4] t3903: reduce dependencies on previous tests D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 2/4] t3905: remove unneeded blank line D. Ben Knoble
@ 2025-09-16 0:37 ` D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 4/4] stash: honor stash.index in apply, pop modes D. Ben Knoble
` (3 subsequent siblings)
6 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-16 0:37 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano, Denton Liu, Glen Choo
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 <ben.knoble+github@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 4/4] stash: honor stash.index in apply, pop modes
2025-09-16 0:37 ` [PATCH v2 0/4] Teach git-stash to use --index from config D. Ben Knoble
` (2 preceding siblings ...)
2025-09-16 0:37 ` [PATCH v2 3/4] stash: refactor private config globals D. Ben Knoble
@ 2025-09-16 0:37 ` D. Ben Knoble
2025-09-16 9:18 ` Phillip Wood
2025-09-16 9:24 ` [PATCH v2 0/4] Teach git-stash to use --index from config Phillip Wood
` (2 subsequent siblings)
6 siblings, 1 reply; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-16 0:37 UTC (permalink / raw)
To: git
Cc: D. Ben Knoble, Junio C Hamano, Glen Choo, Karthik Nayak, John Cai,
Denton Liu, Ævar Arnfjörð Bjarmason
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 <ben.knoble+github@gmail.com>
---
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 <<EOF
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 -- <pathspec> 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 -- <pathspec> stashes in subdirectory' '
mkdir sub &&
>foo &&
@@ -1194,6 +1283,24 @@ setup_stash()
test_path_is_file bar
'
+test_expect_success 'stash -- <pathspec> 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 -- <pathspec> 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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 4/4] stash: honor stash.index in apply, pop modes
2025-09-16 0:37 ` [PATCH v2 4/4] stash: honor stash.index in apply, pop modes D. Ben Knoble
@ 2025-09-16 9:18 ` Phillip Wood
2025-09-16 17:07 ` D. Ben Knoble
0 siblings, 1 reply; 35+ messages in thread
From: Phillip Wood @ 2025-09-16 9:18 UTC (permalink / raw)
To: D. Ben Knoble, git
Cc: Junio C Hamano, Glen Choo, Karthik Nayak, John Cai, Denton Liu,
Ævar Arnfjörð Bjarmason
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 <ben.knoble+github@gmail.com>
> ---
> 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 <<EOF
> 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 -- <pathspec> 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 -- <pathspec> stashes in subdirectory' '
> mkdir sub &&
> >foo &&
> @@ -1194,6 +1283,24 @@ setup_stash()
> test_path_is_file bar
> '
>
> +test_expect_success 'stash -- <pathspec> 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 -- <pathspec> 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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 0/4] Teach git-stash to use --index from config
2025-09-16 0:37 ` [PATCH v2 0/4] Teach git-stash to use --index from config D. Ben Knoble
` (3 preceding siblings ...)
2025-09-16 0:37 ` [PATCH v2 4/4] stash: honor stash.index in apply, pop modes D. Ben Knoble
@ 2025-09-16 9:24 ` Phillip Wood
2025-09-16 17:06 ` D. Ben Knoble
2025-09-16 16:49 ` Junio C Hamano
2025-09-22 1:39 ` [PATCH v3 " D. Ben Knoble
6 siblings, 1 reply; 35+ messages in thread
From: Phillip Wood @ 2025-09-16 9:24 UTC (permalink / raw)
To: D. Ben Knoble, git; +Cc: Junio C Hamano
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 <ben.knoble+github@gmail.com>
>
> ## 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 <ben.knoble+github@gmail.com>
>
> ## 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 <pathspec>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 <ben.knoble+github@gmail.com>
> + ## 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 -- <pathspec> stashes and restores the file' '
> -+test_expect_success !WITH_BREAKING_CHANGES 'stash -- <pathspec> 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 -- <pathspec> stashes in subdirectory' '
> -+test_expect_success WITH_BREAKING_CHANGES 'stash -- <pathspec> stashes and restores the file' '
> ++test_expect_success 'stash -- <pathspec> 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 -- <pathspec> stashes in subdirectory' '
> + test_expect_success 'stash -- <pathspec> 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 -- <pathspec> stashes in subdirectory' '
> ++test_expect_success 'stash -- <pathspec> 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 -- <pathspec> 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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 0/4] Teach git-stash to use --index from config
2025-09-16 0:37 ` [PATCH v2 0/4] Teach git-stash to use --index from config D. Ben Knoble
` (4 preceding siblings ...)
2025-09-16 9:24 ` [PATCH v2 0/4] Teach git-stash to use --index from config Phillip Wood
@ 2025-09-16 16:49 ` Junio C Hamano
2025-09-22 1:39 ` [PATCH v3 " D. Ben Knoble
6 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2025-09-16 16:49 UTC (permalink / raw)
To: D. Ben Knoble; +Cc: git
"D. Ben Knoble" <ben.knoble+github@gmail.com> 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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 0/4] Teach git-stash to use --index from config
2025-09-16 9:24 ` [PATCH v2 0/4] Teach git-stash to use --index from config Phillip Wood
@ 2025-09-16 17:06 ` D. Ben Knoble
0 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-16 17:06 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Junio C Hamano
On Tue, Sep 16, 2025 at 5:25 AM Phillip Wood <phillip.wood123@gmail.com> 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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 4/4] stash: honor stash.index in apply, pop modes
2025-09-16 9:18 ` Phillip Wood
@ 2025-09-16 17:07 ` D. Ben Knoble
0 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-16 17:07 UTC (permalink / raw)
To: phillip.wood
Cc: git, Junio C Hamano, Glen Choo, Karthik Nayak, John Cai,
Denton Liu, Ævar Arnfjörð Bjarmason
On Tue, Sep 16, 2025 at 5:18 AM Phillip Wood <phillip.wood123@gmail.com> 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!
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 0/4] Teach git-stash to use --index from config
2025-09-16 0:37 ` [PATCH v2 0/4] Teach git-stash to use --index from config D. Ben Knoble
` (5 preceding siblings ...)
2025-09-16 16:49 ` Junio C Hamano
@ 2025-09-22 1:39 ` D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 1/4] t3903: reduce dependencies on previous tests D. Ben Knoble
` (3 more replies)
6 siblings, 4 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-22 1:39 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano, Phillip Wood
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. <CAPx1GvcxyDDQmCssMjEnt6JoV6qPc5ZUpgPLX3mpUC_4PNYA1w@mail.gmail.com>,
<c5a811ac-8cd3-c389-ac6d-29020a648c87@gmail.com>.
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 <<EOF
- diff --git a/file2 b/file2
- new file mode 100644
-@@ t/t3903-stash.sh: 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 &&
++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 -- <pathspec> 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 -- <pathspec> stashes in subdirectory' '
- mkdir sub &&
- >foo &&
-@@ t/t3903-stash.sh: setup_stash()
- test_path_is_file bar
- '
-
-+test_expect_success 'stash -- <pathspec> 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 -- <pathspec> 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
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 1/4] t3903: reduce dependencies on previous tests
2025-09-22 1:39 ` [PATCH v3 " D. Ben Knoble
@ 2025-09-22 1:39 ` D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 2/4] t3905: remove unneeded blank line D. Ben Knoble
` (2 subsequent siblings)
3 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-22 1:39 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano, Phillip Wood
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 <ben.knoble+github@gmail.com>
---
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 -- <subdir> 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 -- <subdir> 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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 2/4] t3905: remove unneeded blank line
2025-09-22 1:39 ` [PATCH v3 " D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 1/4] t3903: reduce dependencies on previous tests D. Ben Knoble
@ 2025-09-22 1:39 ` D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 3/4] stash: refactor private config globals D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 4/4] stash: honor stash.index in apply, pop modes D. Ben Knoble
3 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-22 1:39 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano, Phillip Wood, Denton Liu
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 <ben.knoble+github@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 3/4] stash: refactor private config globals
2025-09-22 1:39 ` [PATCH v3 " D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 1/4] t3903: reduce dependencies on previous tests D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 2/4] t3905: remove unneeded blank line D. Ben Knoble
@ 2025-09-22 1:39 ` D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 4/4] stash: honor stash.index in apply, pop modes D. Ben Knoble
3 siblings, 0 replies; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-22 1:39 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Junio C Hamano, Phillip Wood, Denton Liu,
Glen Choo
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 <ben.knoble+github@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 4/4] stash: honor stash.index in apply, pop modes
2025-09-22 1:39 ` [PATCH v3 " D. Ben Knoble
` (2 preceding siblings ...)
2025-09-22 1:39 ` [PATCH v3 3/4] stash: refactor private config globals D. Ben Knoble
@ 2025-09-22 1:39 ` D. Ben Knoble
2025-09-22 14:11 ` Phillip Wood
3 siblings, 1 reply; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-22 1:39 UTC (permalink / raw)
To: git
Cc: D. Ben Knoble, Junio C Hamano, Phillip Wood, moti sd, Denton Liu,
Patrick Steinhardt, Karthik Nayak,
Ævar Arnfjörð Bjarmason, Glen Choo
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 <ben.knoble+github@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/4] stash: honor stash.index in apply, pop modes
2025-09-22 1:39 ` [PATCH v3 4/4] stash: honor stash.index in apply, pop modes D. Ben Knoble
@ 2025-09-22 14:11 ` Phillip Wood
2025-09-24 20:40 ` D. Ben Knoble
0 siblings, 1 reply; 35+ messages in thread
From: Phillip Wood @ 2025-09-22 14:11 UTC (permalink / raw)
To: D. Ben Knoble, git
Cc: Junio C Hamano, moti sd, Denton Liu, Patrick Steinhardt,
Karthik Nayak, Ævar Arnfjörð Bjarmason, Glen Choo
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 <ben.knoble+github@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/4] stash: honor stash.index in apply, pop modes
2025-09-22 14:11 ` Phillip Wood
@ 2025-09-24 20:40 ` D. Ben Knoble
2025-09-29 10:01 ` Phillip Wood
0 siblings, 1 reply; 35+ messages in thread
From: D. Ben Knoble @ 2025-09-24 20:40 UTC (permalink / raw)
To: phillip.wood
Cc: git, Junio C Hamano, moti sd, Denton Liu, Patrick Steinhardt,
Karthik Nayak, Ævar Arnfjörð Bjarmason, Glen Choo
On Mon, Sep 22, 2025 at 10:11 AM Phillip Wood <phillip.wood123@gmail.com> 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?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/4] stash: honor stash.index in apply, pop modes
2025-09-24 20:40 ` D. Ben Knoble
@ 2025-09-29 10:01 ` Phillip Wood
2025-10-06 12:59 ` [PATCH] doc: explain the impact of stash.index on --autostash options D. Ben Knoble
0 siblings, 1 reply; 35+ messages in thread
From: Phillip Wood @ 2025-09-29 10:01 UTC (permalink / raw)
To: D. Ben Knoble, phillip.wood
Cc: git, Junio C Hamano, moti sd, Denton Liu, Patrick Steinhardt,
Karthik Nayak, Ævar Arnfjörð Bjarmason, Glen Choo
On 24/09/2025 21:40, D. Ben Knoble wrote:
> On Mon, Sep 22, 2025 at 10:11 AM Phillip Wood <phillip.wood123@gmail.com> 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
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH] doc: explain the impact of stash.index on --autostash options
2025-09-29 10:01 ` Phillip Wood
@ 2025-10-06 12:59 ` D. Ben Knoble
2025-10-09 22:54 ` Kristoffer Haugsbakk
0 siblings, 1 reply; 35+ messages in thread
From: D. Ben Knoble @ 2025-10-06 12:59 UTC (permalink / raw)
To: git, phillip.wood123
Cc: D. Ben Knoble, avarab, gitster, glencbz, karthik.188, liu.denton,
motisd8, phillip.wood, ps
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 <ben.knoble+github@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] doc: explain the impact of stash.index on --autostash options
2025-10-06 12:59 ` [PATCH] doc: explain the impact of stash.index on --autostash options D. Ben Knoble
@ 2025-10-09 22:54 ` Kristoffer Haugsbakk
2025-10-11 14:44 ` D. Ben Knoble
0 siblings, 1 reply; 35+ messages in thread
From: Kristoffer Haugsbakk @ 2025-10-09 22:54 UTC (permalink / raw)
To: D. Ben Knoble, git, Phillip Wood
Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano, Glen Choo,
Karthik Nayak, Denton Liu, motisd8, Phillip Wood,
Patrick Steinhardt
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 <ben.knoble+github@gmail.com>
> ---
>
> 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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] doc: explain the impact of stash.index on --autostash options
2025-10-09 22:54 ` Kristoffer Haugsbakk
@ 2025-10-11 14:44 ` D. Ben Knoble
2025-10-11 17:28 ` Junio C Hamano
0 siblings, 1 reply; 35+ messages in thread
From: D. Ben Knoble @ 2025-10-11 14:44 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: git, Phillip Wood, Ævar Arnfjörð Bjarmason,
Junio C Hamano, Glen Choo, Karthik Nayak, Denton Liu, motisd8,
Phillip Wood, Patrick Steinhardt
On Thu, Oct 9, 2025 at 6:55 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> 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).
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] doc: explain the impact of stash.index on --autostash options
2025-10-11 14:44 ` D. Ben Knoble
@ 2025-10-11 17:28 ` Junio C Hamano
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2025-10-11 17:28 UTC (permalink / raw)
To: D. Ben Knoble
Cc: Kristoffer Haugsbakk, git, Phillip Wood,
Ævar Arnfjörð Bjarmason, Glen Choo, Karthik Nayak,
Denton Liu, motisd8, Phillip Wood, Patrick Steinhardt
"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
> On Thu, Oct 9, 2025 at 6:55 PM Kristoffer Haugsbakk
> <code@khaugsbakk.name> 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.
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2025-10-11 17:29 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-10 18:33 [PATCH 0/9] make stash apply with --index by default D. Ben Knoble
2025-05-10 18:33 ` [PATCH 1/9] t3903: reduce dependencies on previous tests D. Ben Knoble
2025-05-10 18:33 ` [PATCH 2/9] t3905: remove unneeded blank line D. Ben Knoble
2025-05-10 18:33 ` [PATCH 3/9] BreakingChanges: announce stash {apply,pop} will imply --index D. Ben Knoble
2025-05-10 18:33 ` [PATCH 4/9] stash: restore the index by default when breaking changes are enabled D. Ben Knoble
2025-05-10 18:33 ` [PATCH 5/9] t0450: mark stash documentation as a known discrepancy D. Ben Knoble
2025-05-10 18:33 ` [PATCH 6/9] t3903: adjust stash test to account for --[no-]index with breaking changes D. Ben Knoble
2025-05-10 18:33 ` [PATCH 7/9] t3904: adjust stash -p test to account for index states " D. Ben Knoble
2025-05-10 18:33 ` [PATCH 8/9] t3905: adjust stash -u tests for " D. Ben Knoble
2025-05-10 18:33 ` [PATCH 9/9] t3906: adjust stash submodule tests to account " D. Ben Knoble
2025-05-12 12:52 ` [PATCH 0/9] make stash apply with --index by default Junio C Hamano
2025-05-20 14:36 ` D. Ben Knoble
2025-05-20 14:39 ` D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 0/4] Teach git-stash to use --index from config D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 1/4] t3903: reduce dependencies on previous tests D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 2/4] t3905: remove unneeded blank line D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 3/4] stash: refactor private config globals D. Ben Knoble
2025-09-16 0:37 ` [PATCH v2 4/4] stash: honor stash.index in apply, pop modes D. Ben Knoble
2025-09-16 9:18 ` Phillip Wood
2025-09-16 17:07 ` D. Ben Knoble
2025-09-16 9:24 ` [PATCH v2 0/4] Teach git-stash to use --index from config Phillip Wood
2025-09-16 17:06 ` D. Ben Knoble
2025-09-16 16:49 ` Junio C Hamano
2025-09-22 1:39 ` [PATCH v3 " D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 1/4] t3903: reduce dependencies on previous tests D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 2/4] t3905: remove unneeded blank line D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 3/4] stash: refactor private config globals D. Ben Knoble
2025-09-22 1:39 ` [PATCH v3 4/4] stash: honor stash.index in apply, pop modes D. Ben Knoble
2025-09-22 14:11 ` Phillip Wood
2025-09-24 20:40 ` D. Ben Knoble
2025-09-29 10:01 ` Phillip Wood
2025-10-06 12:59 ` [PATCH] doc: explain the impact of stash.index on --autostash options D. Ben Knoble
2025-10-09 22:54 ` Kristoffer Haugsbakk
2025-10-11 14:44 ` D. Ben Knoble
2025-10-11 17:28 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).