From: Elijah Newren Date: Mon, 02 Feb 2026 20:31:07 GMT Subject: Re: [PATCH v2] stash: honor --no-overwrite-ignore with --all Message-ID: In-Reply-To: <20260202162225.35206-3-pushkarkumarsingh1970@gmail.com> On Mon, Feb 2, 2026 at 8:37 AM 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. > > Signed-off-by: Pushkar Singh > --- > 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