* Re: [PATCH v2] stash: honor --no-overwrite-ignore with --all
2026-02-02 16:22 ` [PATCH v2] stash: honor --no-overwrite-ignore with --all Pushkar Singh
@ 2026-02-02 16:48 ` Kristoffer Haugsbakk
2026-02-02 17:09 ` Pushkar Singh
2026-02-02 20:00 ` D. Ben Knoble
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Kristoffer Haugsbakk @ 2026-02-02 16:48 UTC (permalink / raw)
To: Pushkar Singh, git
Cc: Junio C Hamano, Jeff King, karthiknayak, Patrick Steinhardt, kh
On Mon, Feb 2, 2026, at 17:22, Pushkar Singh wrote:
> Teach stash push/save to avoid -a cleanup when --no-overwrite-ignore
> is given by downgrading INCLUDE_ALL_FILES to include-untracked.
>
> This fixes ignored files being incorrectly removed despite
> --no-overwrite-ignore, and removes the stash FIXME by plumbing
> overwrite_ignore into unpack_trees().
>
> Add regression tests covering both overwrite and no-overwrite cases.
>
> Changes since v1:
> - Use OPT_BOOL correctly for overwrite-ignore.
> - Fix stash -a cleanup when --no-overwrite-ignore is given by downgrading
> INCLUDE_ALL_FILES to include-untracked.
> - Add regression test for --overwrite-ignore.
> - Adjust no-overwrite-ignore test to explicitly use -a.
> - Add Signed-off-by.
These patch version changes are supposed to go after the `---` (after
the `Signed-off-by`). I guess people who are comfortable editing patches
write them manually in that place (unless something like b4 or
gigitgadget does it for them). I prefer to use `--notes` and let
git-format-patch(1) inject it for me. :)
>
> Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
> ---
> builtin/stash.c | 14 ++++++++------
> t/t3905-stash-include-untracked.sh | 16 ++++++++++++++--
> 2 files changed, 22 insertions(+), 8 deletions(-)
>[snip]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2] stash: honor --no-overwrite-ignore with --all
2026-02-02 16:22 ` [PATCH v2] stash: honor --no-overwrite-ignore with --all Pushkar Singh
2026-02-02 16:48 ` Kristoffer Haugsbakk
@ 2026-02-02 20:00 ` D. Ben Knoble
2026-02-02 20:31 ` Elijah Newren
2026-02-03 18:04 ` [PATCH v3] " Pushkar Singh
3 siblings, 0 replies; 15+ messages in thread
From: D. Ben Knoble @ 2026-02-02 20:00 UTC (permalink / raw)
To: Pushkar Singh; +Cc: git, gitster, peff, karthiknayak, ps, kh
On Mon, Feb 2, 2026 at 11:37 AM Pushkar Singh
<pushkarkumarsingh1970@gmail.com> wrote:
>
> Teach stash push/save to avoid -a cleanup when --no-overwrite-ignore
> is given by downgrading INCLUDE_ALL_FILES to include-untracked.
>
> This fixes ignored files being incorrectly removed despite
> --no-overwrite-ignore, and removes the stash FIXME by plumbing
> overwrite_ignore into unpack_trees().
>
> Add regression tests covering both overwrite and no-overwrite cases.
>
> Changes since v1:
> - Use OPT_BOOL correctly for overwrite-ignore.
> - Fix stash -a cleanup when --no-overwrite-ignore is given by downgrading
> INCLUDE_ALL_FILES to include-untracked.
> - Add regression test for --overwrite-ignore.
> - Adjust no-overwrite-ignore test to explicitly use -a.
> - Add Signed-off-by.
>
> Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
> ---
> builtin/stash.c | 14 ++++++++------
> t/t3905-stash-include-untracked.sh | 16 ++++++++++++++--
> 2 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 82d10520fe..c3ee33cce1 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1858,9 +1858,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
> OPT_SET_INT('a', "all", &include_untracked,
> N_("include ignore files"), 2),
> OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore,
> - N_("update ignored files (default)")),
> - OPT_BOOL(0, "no-overwrite-ignore", &overwrite_ignore,
> - N_("do not update ignored files")),
> + N_("update ignored files")),
> OPT_STRING('m', "message", &stash_msg, N_("message"),
> N_("stash message")),
> OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
This doesn't apply on top of, say, the master branch; it looks like
you generated this patch on top of the previous version?
> diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
> index 9c5421cd76..a979831a64 100755
> --- a/t/t3905-stash-include-untracked.sh
> +++ b/t/t3905-stash-include-untracked.sh
> @@ -427,17 +427,29 @@ test_expect_success 'stash -u ignores sub-repository' '
> git stash -u
> '
>
> -test_expect_success 'stash push --no-overwrite-ignore preserves ignored files' '
> +test_expect_success 'stash push -a --no-overwrite-ignore preserves ignored files' '
> echo ignored.txt >>.gitignore &&
> echo before >ignored.txt &&
> git add .gitignore &&
> git commit -m "add ignore" &&
>
> echo after >ignored.txt &&
> - git stash push --no-overwrite-ignore &&
> + git stash push -a --no-overwrite-ignore &&
>
> test_path_is_file ignored.txt &&
> grep after ignored.txt
> '
>
> +test_expect_success 'stash push -a --overwrite-ignore overwrites ignored files' '
> + echo ignored.txt >>.gitignore &&
> + echo before >ignored.txt &&
> + git add .gitignore &&
> + git commit -m "add ignore" &&
> +
> + echo after >ignored.txt &&
> + git stash push -a --overwrite-ignore &&
> +
> + ! grep after ignored.txt
> +'
After removing --overwrite-ignore from these 2 tests to run them on
unmodified Git, the first one fails (good: exercising new feature and
showing improvement) and the 2nd one succeeds (probably good:
exercising existing behavior, but strange: see below).
Use test_grep instead of plain grep. For example, it reveals that in
the first test, ignored.txt doesn't exist in current Git! (Which is
expected with -a, although should be changed by this series).
For the second test, I think we're hitting a similar issue (test_grep
complains ignored.txt doesn't exist, so while inverted grep would
succeed, we actually want to see that the file doesn't exist, right?).
Anyway, the current "! grep …" passes on current Git because it's not
really the right test, plus "--overwrite-ignore" is the current
behavior of "-a", so a modified version of this test _should_ pass on
current Git. I think we want "test_path_is_missing" here?
--
D. Ben Knoble
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2] stash: honor --no-overwrite-ignore with --all
2026-02-02 16:22 ` [PATCH v2] stash: honor --no-overwrite-ignore with --all Pushkar Singh
2026-02-02 16:48 ` Kristoffer Haugsbakk
2026-02-02 20:00 ` D. Ben Knoble
@ 2026-02-02 20:31 ` Elijah Newren
2026-02-03 18:18 ` Pushkar Singh
2026-02-03 18:04 ` [PATCH v3] " Pushkar Singh
3 siblings, 1 reply; 15+ messages in thread
From: Elijah Newren @ 2026-02-02 20:31 UTC (permalink / raw)
To: Pushkar Singh; +Cc: git, gitster, peff, karthiknayak, ps, kh
On Mon, Feb 2, 2026 at 8:37 AM Pushkar Singh
<pushkarkumarsingh1970@gmail.com> wrote:
>
> Teach stash push/save to avoid -a cleanup when --no-overwrite-ignore
> is given by downgrading INCLUDE_ALL_FILES to include-untracked.
>
> This fixes ignored files being incorrectly removed despite
> --no-overwrite-ignore, and removes the stash FIXME by plumbing
> overwrite_ignore into unpack_trees().
>
> Add regression tests covering both overwrite and no-overwrite cases.
>
> Changes since v1:
> - Use OPT_BOOL correctly for overwrite-ignore.
> - Fix stash -a cleanup when --no-overwrite-ignore is given by downgrading
> INCLUDE_ALL_FILES to include-untracked.
> - Add regression test for --overwrite-ignore.
> - Adjust no-overwrite-ignore test to explicitly use -a.
> - Add Signed-off-by.
>
> Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
> ---
> builtin/stash.c | 14 ++++++++------
> t/t3905-stash-include-untracked.sh | 16 ++++++++++++++--
> 2 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 82d10520fe..c3ee33cce1 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1858,9 +1858,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
> OPT_SET_INT('a', "all", &include_untracked,
> N_("include ignore files"), 2),
> OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore,
> - N_("update ignored files (default)")),
> - OPT_BOOL(0, "no-overwrite-ignore", &overwrite_ignore,
> - N_("do not update ignored files")),
What's the basis for this patch? I don't see any "overwrite-ignore"
anywhere in builtin/stash.c .
> + N_("update ignored files")),
> OPT_STRING('m', "message", &stash_msg, N_("message"),
> N_("stash message")),
> OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
> @@ -1894,6 +1892,9 @@ static int push_stash(int argc, const char **argv, const char *prefix,
> parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
> prefix, argv);
>
> + if (!overwrite_ignore && include_untracked == INCLUDE_ALL_FILES)
> + include_untracked = 1;
This suggests that --all and --no-overwrite-ignore are incompatible,
yes? Shouldn't they be reported as such rather than having one
silently override the other?
> +
> if (pathspec_from_file) {
> if (patch_mode)
> die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--patch");
> @@ -1965,9 +1966,7 @@ static int save_stash(int argc, const char **argv, const char *prefix,
> OPT_SET_INT('a', "all", &include_untracked,
> N_("include ignore files"), 2),
> OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore,
> - N_("update ignored files (default)")),
> - OPT_BOOL(0, "no-overwrite-ignore", &overwrite_ignore,
> - N_("do not update ignored files")),
> + N_("update ignored files")),
> OPT_STRING('m', "message", &stash_msg, "message",
> N_("stash message")),
> OPT_END()
> @@ -1994,6 +1993,9 @@ static int save_stash(int argc, const char **argv, const char *prefix,
> die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
> }
>
> + if (!overwrite_ignore && include_untracked == INCLUDE_ALL_FILES)
> + include_untracked = 1;
> +
Same comments as above.
Also, the commit message claims you are removing a FIXME comment, but
no such removal is found in this patch. Is this simply a patch
against v1? If so, don't do that; please send a corrected patch
I took a look at v1 as well, and I'll note here that if that FIXME was
the only line that needed fixing, I would have just fixed it at the
time. I left the FIXME there because I knew it was *one* of the
places that would need fixing and didn't have the time or energy
(already being a few levels deep in the rabbit hole) to track down all
the stash related issues in this area. Perhaps the other sites have
since been fixed by someone else, but if so, that should really be
documented in the commit message. I'd personally be pretty surprised
if the other locations have been fixed; see
https://lore.kernel.org/git/CABPp-BFyR19ch71W10oJDFuRX1OHzQ3si971pMn6dPtHKxJDXQ@mail.gmail.com/
and perhaps the references to stash in
https://lore.kernel.org/git/pull.1627.git.1703643931314.gitgitgadget@gmail.com/
; there may also be other issues within stash, those were just the
ones I was aware of that looked fishy at the time.
> ret = do_push_stash(&ps, stash_msg, quiet, keep_index,
> patch_mode, &add_p_opt, include_untracked,
> only_staged);
> diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
> index 9c5421cd76..a979831a64 100755
> --- a/t/t3905-stash-include-untracked.sh
> +++ b/t/t3905-stash-include-untracked.sh
> @@ -427,17 +427,29 @@ test_expect_success 'stash -u ignores sub-repository' '
> git stash -u
> '
>
> -test_expect_success 'stash push --no-overwrite-ignore preserves ignored files' '
> +test_expect_success 'stash push -a --no-overwrite-ignore preserves ignored files' '
> echo ignored.txt >>.gitignore &&
> echo before >ignored.txt &&
> git add .gitignore &&
> git commit -m "add ignore" &&
>
> echo after >ignored.txt &&
> - git stash push --no-overwrite-ignore &&
> + git stash push -a --no-overwrite-ignore &&
Isn't this a non-sensical combination of command line options?
--no-overwrite-ignore is explicitly setting include_untracked to 1
while --all sets include_untracked to 2, and I believe those are the
_only_ things each of those flags do, which means these ought to be
incompatible flags.
>
> test_path_is_file ignored.txt &&
> grep after ignored.txt
> '
>
> +test_expect_success 'stash push -a --overwrite-ignore overwrites ignored files' '
> + echo ignored.txt >>.gitignore &&
> + echo before >ignored.txt &&
> + git add .gitignore &&
> + git commit -m "add ignore" &&
> +
> + echo after >ignored.txt &&
> + git stash push -a --overwrite-ignore &&
> +
> + ! grep after ignored.txt
> +'
> +
> test_done
> --
> 2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2] stash: honor --no-overwrite-ignore with --all
2026-02-02 20:31 ` Elijah Newren
@ 2026-02-03 18:18 ` Pushkar Singh
2026-02-03 19:22 ` Elijah Newren
0 siblings, 1 reply; 15+ messages in thread
From: Pushkar Singh @ 2026-02-03 18:18 UTC (permalink / raw)
To: newren; +Cc: git, gitster, karthiknayak, kh, peff, ps, pushkarkumarsingh1970
Hi Elijah,
Thanks for the detailed feedback. Much appreciated.
> What's the basis for this patch? I don't see any "overwrite-ignore"
> anywhere in builtin/stash.c .
The basis was the existing behavior where git stash push -a would remove
ignored files even when --no-overwrite-ignore is provided. The intent was
to make stash honor --no-overwrite-ignore consistently with other callers
of unpack_trees, limited specifically to the stash -a cleanup path.
In v3 I rebased onto current master and also removed the commit message
claim about removing the stash FIXME, since this series only addresses
the concrete stash behavior and does not attempt to solve the broader
unpack_trees issues.
> This suggests that --all and --no-overwrite-ignore are incompatible,
> yes? Shouldn't they be reported as such rather than having one silently
> override the other?
I agree they are philosophically contradictory. I chose to downgrade
INCLUDE_ALL_FILES to include-untracked when --no-overwrite-ignore is given
so that users explicitly requesting preservation of ignored files are not
surprised by their removal.
I am open to changing this to an explicit error instead if that is
preferred. I went with downgrading to preserve backwards compatibility
and to honor the more conservative option.
> Also, the commit message claims you are removing a FIXME comment, but
> no such removal is found in this patch.
Yes, that was an error in v2. In v3 the commit message no longer claims
to remove the FIXME and is scoped only to fixing stash -a behavior plus
adding regression tests.
Regarding tests, v3 now explicitly covers both:
- stash push -a --no-overwrite-ignore preserving ignored files
- stash push -a --overwrite-ignore removing them
using test_grep and test_path_is_missing as suggested.
Thanks also for the references to the broader unpack_trees and stash
history. I understand this patch only addresses a small part of a much
larger and messier area.
Please let me know if you would prefer the -a plus --no-overwrite-ignore
combination to error out instead of downgrading.
Thanks,
Pushkar
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] stash: honor --no-overwrite-ignore with --all
2026-02-03 18:18 ` Pushkar Singh
@ 2026-02-03 19:22 ` Elijah Newren
0 siblings, 0 replies; 15+ messages in thread
From: Elijah Newren @ 2026-02-03 19:22 UTC (permalink / raw)
To: Pushkar Singh; +Cc: git, gitster, karthiknayak, kh, peff, ps
Hi Pushkar,
On Tue, Feb 3, 2026 at 10:18 AM Pushkar Singh
<pushkarkumarsingh1970@gmail.com> wrote:
>
> Hi Elijah,
>
> Thanks for the detailed feedback. Much appreciated.
>
> > What's the basis for this patch? I don't see any "overwrite-ignore"
> > anywhere in builtin/stash.c .
>
> The basis was the existing behavior where git stash push -a would remove
> ignored files even when --no-overwrite-ignore is provided.
By basis, I meant what commit was it based on. The patch(es) you send
need to be applied by others, and if they are based on commits only
you have locally, others can't apply or try them and have to guess the
details of all your intermediate patches. v2 should be what you would
have sent to the list if you had gotten everything right the first
time. Same with v3, v4, etc.
You appear to have thought in terms of the purpose of the patch, but
your stated purpose doesn't make sense either. There is no
--no-overwrite-ignore option, so complaining about how the command
behaved when that non-existent option is given doesn't help me
understand the purpose of the new option.
> The intent was
> to make stash honor --no-overwrite-ignore consistently with other callers
> of unpack_trees, limited specifically to the stash -a cleanup path.
Oh, so the point of the patch is an attempt to make command line
options more consistent along some axis? If so, I think you picked a
place where it doesn't actually make sense. We need to back up and
figure out what the user-side desired behavior is and what they cannot
achieve today, or what is confusing today, and find ways to improve
that and implement it. Starting from the low-level details can work,
but only if at the end we can explain to users why our changes make
sense.
> In v3 I rebased onto current master and also removed the commit message
> claim about removing the stash FIXME, since this series only addresses
> the concrete stash behavior and does not attempt to solve the broader
> unpack_trees issues.
>
> > This suggests that --all and --no-overwrite-ignore are incompatible,
> > yes? Shouldn't they be reported as such rather than having one silently
> > override the other?
>
> I agree they are philosophically contradictory. I chose to downgrade
> INCLUDE_ALL_FILES to include-untracked when --no-overwrite-ignore is given
> so that users explicitly requesting preservation of ignored files are not
> surprised by their removal.
You don't want people who pass "--no-overwrite-ignore" (a new option)
to be surprised when ignores are overwritten, but don't care about
people who pass "-a" ("--all") getting surprised that all files aren't
included in the stash? I don't quite understand the logic.
> I am open to changing this to an explicit error instead if that is
> preferred. I went with downgrading to preserve backwards compatibility
> and to honor the more conservative option.
You added a new option and only changed behavior relative to when that
new option is invoked, so I don't understand the claim about
preserving backwards compatibility; how can backwards compatibility
even be relevant in this situation?
I'm not sure I understand the "honor the more conservative option"
either. Is that a cyclical argument (you're introducing a new option
and deciding to honor it in order to honor it), or am I
misunderstanding?
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] stash: honor --no-overwrite-ignore with --all
2026-02-02 16:22 ` [PATCH v2] stash: honor --no-overwrite-ignore with --all Pushkar Singh
` (2 preceding siblings ...)
2026-02-02 20:31 ` Elijah Newren
@ 2026-02-03 18:04 ` Pushkar Singh
2026-02-03 19:22 ` Elijah Newren
3 siblings, 1 reply; 15+ messages in thread
From: Pushkar Singh @ 2026-02-03 18:04 UTC (permalink / raw)
To: git; +Cc: gitster, karthiknayak, kh, peff, ps, Pushkar Singh
Teach stash push/save to avoid -a cleanup when --no-overwrite-ignore
is given by downgrading INCLUDE_ALL_FILES to include-untracked.
This fixes ignored files being incorrectly removed despite
--no-overwrite-ignore.
Add regression tests covering both overwrite and no-overwrite cases.
Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
---
Changes since v2:
- Use test_grep instead of grep
- Use test_path_is_missing for overwrite-ignore test
- Rebase onto current master so patch applies cleanly
builtin/stash.c | 14 ++++++++------
t/t3905-stash-include-untracked.sh | 18 +++++++++++++++---
2 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index 82d10520fe..c3ee33cce1 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1858,9 +1858,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
OPT_SET_INT('a', "all", &include_untracked,
N_("include ignore files"), 2),
OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore,
- N_("update ignored files (default)")),
- OPT_BOOL(0, "no-overwrite-ignore", &overwrite_ignore,
- N_("do not update ignored files")),
+ N_("update ignored files")),
OPT_STRING('m', "message", &stash_msg, N_("message"),
N_("stash message")),
OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
@@ -1894,6 +1892,9 @@ static int push_stash(int argc, const char **argv, const char *prefix,
parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
prefix, argv);
+ if (!overwrite_ignore && include_untracked == INCLUDE_ALL_FILES)
+ include_untracked = 1;
+
if (pathspec_from_file) {
if (patch_mode)
die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--patch");
@@ -1965,9 +1966,7 @@ static int save_stash(int argc, const char **argv, const char *prefix,
OPT_SET_INT('a', "all", &include_untracked,
N_("include ignore files"), 2),
OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore,
- N_("update ignored files (default)")),
- OPT_BOOL(0, "no-overwrite-ignore", &overwrite_ignore,
- N_("do not update ignored files")),
+ N_("update ignored files")),
OPT_STRING('m', "message", &stash_msg, "message",
N_("stash message")),
OPT_END()
@@ -1994,6 +1993,9 @@ static int save_stash(int argc, const char **argv, const char *prefix,
die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
}
+ if (!overwrite_ignore && include_untracked == INCLUDE_ALL_FILES)
+ include_untracked = 1;
+
ret = do_push_stash(&ps, stash_msg, quiet, keep_index,
patch_mode, &add_p_opt, include_untracked,
only_staged);
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index 9c5421cd76..63b59de47b 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -427,17 +427,29 @@ test_expect_success 'stash -u ignores sub-repository' '
git stash -u
'
-test_expect_success 'stash push --no-overwrite-ignore preserves ignored files' '
+test_expect_success 'stash push -a --no-overwrite-ignore preserves ignored files' '
echo ignored.txt >>.gitignore &&
echo before >ignored.txt &&
git add .gitignore &&
git commit -m "add ignore" &&
echo after >ignored.txt &&
- git stash push --no-overwrite-ignore &&
+ git stash push -a --no-overwrite-ignore &&
test_path_is_file ignored.txt &&
- grep after ignored.txt
+ test_grep after ignored.txt
+'
+
+test_expect_success 'stash push -a --overwrite-ignore overwrites ignored files' '
+ echo ignored.txt >>.gitignore &&
+ echo before >ignored.txt &&
+ git add .gitignore &&
+ git commit -m "add ignore" &&
+
+ echo after >ignored.txt &&
+ git stash push -a --overwrite-ignore &&
+
+ test_path_is_missing ignored.txt
'
test_done
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3] stash: honor --no-overwrite-ignore with --all
2026-02-03 18:04 ` [PATCH v3] " Pushkar Singh
@ 2026-02-03 19:22 ` Elijah Newren
2026-02-03 20:07 ` Pushkar Singh
0 siblings, 1 reply; 15+ messages in thread
From: Elijah Newren @ 2026-02-03 19:22 UTC (permalink / raw)
To: Pushkar Singh; +Cc: git, gitster, karthiknayak, kh, peff, ps
On Tue, Feb 3, 2026 at 10:09 AM Pushkar Singh
<pushkarkumarsingh1970@gmail.com> wrote:
>
> Teach stash push/save to avoid -a cleanup when --no-overwrite-ignore
> is given by downgrading INCLUDE_ALL_FILES to include-untracked.
This feels like you're regurgitating the patch with low-enough level
of details ("-a cleanup", INCLUDE_ALL_FILES, include-untracked) that
it'll only be intelligible to someone who has builtin/stash.c code
fresh on their mind. It doesn't explain the high-level purpose behind
your patch, and, in fact, will likely lead readers to try to read the
patch in order to understand the commit message, when usually we hope
for the opposite.
> This fixes ignored files being incorrectly removed despite
> --no-overwrite-ignore.
This claim makes no sense; --no-overwrite-ignore doesn't exist in git
yet, and this is the first (and only) patch in your series, so at best
you're claiming to fix something you introduced? Very confusing.
> Add regression tests covering both overwrite and no-overwrite cases.
>
> Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
> ---
> Changes since v2:
> - Use test_grep instead of grep
> - Use test_path_is_missing for overwrite-ignore test
> - Rebase onto current master so patch applies cleanly
Great...but you still seem to be submitting a patch that is based on
your previous (rebased?) patches, without submitting the previous
patches, leaving us to guess how you got to your current state, as
noted below. You should have been editing your previous patch and
then submitting the edited patch. After v2, you should have squashed
and resent.
> builtin/stash.c | 14 ++++++++------
> t/t3905-stash-include-untracked.sh | 18 +++++++++++++++---
> 2 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 82d10520fe..c3ee33cce1 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1858,9 +1858,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
> OPT_SET_INT('a', "all", &include_untracked,
> N_("include ignore files"), 2),
> OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore,
> - N_("update ignored files (default)")),
> - OPT_BOOL(0, "no-overwrite-ignore", &overwrite_ignore,
> - N_("do not update ignored files")),
> + N_("update ignored files")),
And here's where it's clear that this patch was broken in the same way
as v2: "no-overwrite-ignore" has never appeared in any version of
builtin/stash.c upstream (same with "overwrite-ignore"), so this patch
is clearly against some local state you have. The base of your series
(or the base of your patch, since you only have one patch in this
series) needs to be an upstream commit, not some other commit that
only you have access to. Might I interest you in using gitgitgadget,
which would make it easier to submit patches?
> OPT_STRING('m', "message", &stash_msg, N_("message"),
> N_("stash message")),
> OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
> @@ -1894,6 +1892,9 @@ static int push_stash(int argc, const char **argv, const char *prefix,
> parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
> prefix, argv);
>
> + if (!overwrite_ignore && include_untracked == INCLUDE_ALL_FILES)
> + include_untracked = 1;
> +
> if (pathspec_from_file) {
> if (patch_mode)
> die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--patch");
> @@ -1965,9 +1966,7 @@ static int save_stash(int argc, const char **argv, const char *prefix,
> OPT_SET_INT('a', "all", &include_untracked,
> N_("include ignore files"), 2),
> OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore,
> - N_("update ignored files (default)")),
> - OPT_BOOL(0, "no-overwrite-ignore", &overwrite_ignore,
> - N_("do not update ignored files")),
> + N_("update ignored files")),
> OPT_STRING('m', "message", &stash_msg, "message",
> N_("stash message")),
> OPT_END()
> @@ -1994,6 +1993,9 @@ static int save_stash(int argc, const char **argv, const char *prefix,
> die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
> }
>
> + if (!overwrite_ignore && include_untracked == INCLUDE_ALL_FILES)
> + include_untracked = 1;
> +
>
> ret = do_push_stash(&ps, stash_msg, quiet, keep_index,
> patch_mode, &add_p_opt, include_untracked,
> only_staged);
> diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
> index 9c5421cd76..63b59de47b 100755
> --- a/t/t3905-stash-include-untracked.sh
> +++ b/t/t3905-stash-include-untracked.sh
> @@ -427,17 +427,29 @@ test_expect_success 'stash -u ignores sub-repository' '
> git stash -u
> '
>
> -test_expect_success 'stash push --no-overwrite-ignore preserves ignored files' '
> +test_expect_success 'stash push -a --no-overwrite-ignore preserves ignored files' '
> echo ignored.txt >>.gitignore &&
> echo before >ignored.txt &&
> git add .gitignore &&
> git commit -m "add ignore" &&
>
> echo after >ignored.txt &&
> - git stash push --no-overwrite-ignore &&
> + git stash push -a --no-overwrite-ignore &&
Not only is the patch broken ("no-overwrite-ignore" has never appeared
in any version of git; so this patch is clearly against your local
state), but the command line makes no sense:
-a : stash ignored files too
--no-overwrite-ignore: wait, we don't want to mess with ignored
files, so nevermind, don't stash them
Why wouldn't the user just leave off "-a" if they don't want them stashed?
> test_path_is_file ignored.txt &&
> - grep after ignored.txt
> + test_grep after ignored.txt
> +'
> +
> +test_expect_success 'stash push -a --overwrite-ignore overwrites ignored files' '
> + echo ignored.txt >>.gitignore &&
> + echo before >ignored.txt &&
> + git add .gitignore &&
> + git commit -m "add ignore" &&
> +
> + echo after >ignored.txt &&
> + git stash push -a --overwrite-ignore &&
And this command line makes no sense either:
-a: stash ignored files too
--overwrite-ignore: yes, I'm explicitly giving you permission to pay
attention to the fact that I already passed you the "-a" parameter.
Please do what that other parameter says.
Why would the user need an extra flag instead of just using "-a"?
Additionally, if there is some user problem you're trying to solve
here, then these tests look rather incomplete; they only test the push
side and not the pop side. What if someone runs "git stash push -a"
followed by "git stash pop --no-overwrite-ignore"? Or is that flag
not going to be added to pop? Do we only care about protecting
ignored files at push/save time and not at pop time? Why? (And if we
do care about pop time, won't we need to worry about both former
untracked and former ignored files both having the possibility of
overwriting files that are now ignored? And if we do allow users to
not overwrite ignored files at pop time, do we have a similar special
flag to avoid overwriting untracked files at pop time? If not, are
ignored files thus more important or special than untracked files?)
I don't understand the user-driven problem this patch is attempting to solve.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3] stash: honor --no-overwrite-ignore with --all
2026-02-03 19:22 ` Elijah Newren
@ 2026-02-03 20:07 ` Pushkar Singh
0 siblings, 0 replies; 15+ messages in thread
From: Pushkar Singh @ 2026-02-03 20:07 UTC (permalink / raw)
To: Elijah Newren; +Cc: git, gitster, karthiknayak, kh, peff, ps
Hi Elijah,
Thank you for taking the time to explain this so clearly.
You are absolutely right. I misunderstood what you meant by “basis”
and also approached this patch from the implementation side instead of
starting from a concrete user problem.
I also realize now that I incorrectly stacked this on top of my local
changes instead of rebasing and editing the previous version, which
made the patch impossible to apply upstream. Sorry about that.
Given your feedback, I agree that I need to step back and rethink this
from a user perspective (what real workflow is broken today, how -a
should behave, and whether any new flags even make sense here), rather
than trying to force consistency at a low level.
I will drop this series for now, spend time understanding stash
behavior and the broader context you pointed out, and only resend if I
can clearly articulate a user-driven problem with a clean patch based
directly on upstream.
Thanks again for your patience and guidance.
Pushkar
^ permalink raw reply [flat|nested] 15+ messages in thread