Re: [PATCH v2] stash: honor --no-overwrite-ignore with --all

From: Elijah Newren <newren@gmail.com>
Date: Feb 3, 2026, 19:22 UTC
Message-ID: <CABPp-BEP=KBKdF-hgMgF0ngJDsBHMehCJoRc=ww=-W=F3s5rcQ@mail.gmail.com>
In-Reply-To: <20260203181845.602979-1-pushkarkumarsingh1970@gmail.com>

Hi Pushkar,

On Tue, Feb 3, 2026 at 10:18 AM Pushkar Singh <pushkarkumarsingh1970@gmail.com> wrote:

Show 10 quoted lines

>
> 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.

Show 13 quoted lines

> 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?