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

On Mon, Sep 20, 2021 at 11:11 AM Phillip Wood [off-list ref] wrote:

On 20/09/2021 17:05, Elijah Newren wrote:

quoted

On Mon, Sep 20, 2021 at 3:19 AM Phillip Wood [off-list ref] wrote:

quoted

On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

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.

quoted

quoted

quoted

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.

quoted

 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.

quoted

quoted

     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/ (local).  Seems you
also felt that way previously, at
https://lore.kernel.org/git/d4c36a24-b40c-a6ca-7a05-572ab93a0101@gmail.com/ (local)
-- 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'

quoted

quoted

quoted

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!