Re: [PATCH 2/6] Split unpack_trees 'reset' flag into two for untracked handling

[Elijah Newren <newren@gmail.com>]

On Mon, Sep 20, 2021 at 11:11 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 20/09/2021 17:05, Elijah Newren wrote:
> > On Mon, Sep 20, 2021 at 3:19 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >>
> >> On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:
> >>> From: Elijah Newren <newren@gmail.com>
> >>>
> >>> Traditionally, unpack_trees_options->reset was used to signal that it
> >>> was okay to delete any untracked files in the way.  This was used by
> >>> `git read-tree --reset`, but then started appearing in other places as
> >>> well.  However, many of the other uses should not be deleting untracked
> >>> files in the way.  Split this into two separate fields:
> >>>      reset_nuke_untracked
> >>>      reset_keep_untracked
> >>> and, since many code paths in unpack_trees need to be followed for both
> >>> of these flags, introduce a third one for convenience:
> >>>      reset_either
> >>> which is simply an or-ing of the other two.
> >>
> >> See [1] for an alternative approach that used an enum instead of adding
> >> mutually exclusive flags.
> >
> > Oh, interesting.  Any reason you didn't pursue that old series further?
>
> Mainly lack of time/distracted by other things. I was also not that
> confident about modifying the unpack_trees() code. Duy was very helpful
> but then moved on quite soon after I posted that series I think and
> there didn't seem to be much interest from others.
>
> >>> Modify existing callers so that
> >>>      read-tree --reset
> >>
> >> it would be nice if read-tree callers could choose whether they want to
> >> remove untracked files or not - that could always be added later. This
> >> patch changes the behavior of 'git read-tree -m -u' (and other commands)
> >> so that they will overwrite ignored files - I'm in favor of that change
> >> but it would be good to spell out the change in the commit message.
> >
> > Those commands made no distinction between untracked and ignored files
> > previously, and overwrote all of them.
>
> Are you sure, I thought 'read-tree -m -u' unlike 'read-tree --reset -u'
> refused to overwrite untracked and ignored files currently.

Doh, I was thinking of read-tree --reset -u rather than read-tree -m
-u, despite the fact that you explicitly called out (and I even quoted
you on) the latter.  You are right.

> >  This patch changes those
> > commands so that they stop overwriting untracked files, unless those
> > files are ignored.  So, there's no change in behavior for ignored
> > files, only for non-ignored untracked files.
> >
> > Your suggestion to point out the behavior relative to ignored files in
> > the commit message, though, is probably a good idea.  I should mention
> > that ignored files will continue to be removed by these commands.
> >
> >>>      reset --hard
> >>>      checkout --force
> >>
> >> I often use checkout --force to clear unwanted changes when I'm
> >> switching branches, I'd prefer it if it did not remove untracked files.
> >
> > I originally started down that path to see what it looked like, but
> > Junio weighed in and explicitly called out checkout --force as being a
> > command that should remove untracked files in the way.  See
> > https://lore.kernel.org/git/xmqqr1e2ejs9.fsf@gitster.g/.  Seems you
> > also felt that way previously, at
> > https://lore.kernel.org/git/d4c36a24-b40c-a6ca-7a05-572ab93a0101@gmail.com/
> > -- any reason for your change of opinion?
>
> I've no recollection of writing that email! When I was writing today I
> thought that 'checkout -f' and 'switch --discard-changes' behaved the
> same way but it appears from that other message that they do not so
> maybe it is OK for 'checkout -f' to nuke everything if there is a safe
> alternative available in the form of 'switch --discard-changes'
>
> >>> continue using reset_nuke_untracked, but so that other callers,
> >>> including
> >>>      am
> >>>      checkout without --force
> >>>      stash  (though currently dead code; reset always had a value of 0)
> >>>      numerous callers from rebase/sequencer to reset_head()
> >>> will use the new reset_keep_untracked field.
> >>
> >> This is great. In the discussion around [1] there is a mention of 'git
> >> checkout <pathspec>' which also overwrites untracked files. It does not
> >> use unpack_trees() so is arguably outside the scope of what you're doing
> >> here but it might be worth mentioning.
> >
> > Oh, that's interesting.  Yeah, that's worth mentioning and perhaps digging into.
>
> It'd be fantastic to fix that if you have the time and inclination to
> dig into it.

I won't include it in this series, but I'll throw it on my (long) pile
of things to perhaps look at later.


Thanks for the suggestions and pointers in your reviews!