On Mon, Sep 20, 2021 at 11:11 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > > On 20/09/2021 17:05, Elijah Newren wrote: > > On Mon, Sep 20, 2021 at 3:19 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > >> > >> On 19/09/2021 00:15, Elijah Newren via GitGitGadget wrote: > >>> From: Elijah Newren <newren@xxxxxxxxx> > >>> > >>> 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@xxxxxxxxx/ > > -- 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!
