Re: [PATCH v3] stash: honor --no-overwrite-ignore with --all
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.
Show 8 quoted lines
> 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.
Show 16 quoted lines
> 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?
Show 53 quoted lines
> 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 themWhy wouldn't the user just leave off "-a" if they don't want them stashed?
Show 13 quoted lines
> 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.
Pushkar Singh
