Re: [PATCH v3 8/8] sparse-checkout: clear tracked sparse dirs

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 20, 2021, 15:49 UTC
Message-ID: <643d67a9-054f-bc22-cf30-763fc082e9d3@gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2108191015260.55@tvgsbejvaqbjf.bet>

On 8/19/2021 4:48 AM, Johannes Schindelin wrote:

Show 6 quoted lines

> On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote:
> This description makes sense, and is easy to explain.
> 
> It does not cover the case where untracked files are found and the
> directory is _not_ removed as a consequence, though. I would like to ask
> to add this to the commit message, because it is kind of important.

Right. I should have modified the message from my earlier version when
the issues with untracked files came up.

Show 6 quoted lines

> The implementation of this behavior looks fine to me.
> 
> About this behavior itself: in my experience, the more tricky a feature is
> to explain, the more likely the design should have been adjusted in the
> first place. And I find myself struggling a little bit to explain what
> files `git switch` touches in cone mode in addition to tracked files.

Keep in mind that 'git switch' does not change the sparse-checkout cone, and the activity being described is something that would happen within 'git sparse-checkout set' or 'git sparse-checkout reapply'.

Show 11 quoted lines

> So I wonder whether an easier-to-explain behavior would be the following:
> ignored files in directories that fell out of the sparse-checkout cone are
> deleted. (Even if there are untracked, unignored files in the same
> directory tree.)
> 
> This is different than what this patch implements: we would now have to
> delete the ignored and out-of-cone files _also_ when there are untracked
> files in the same directory, i.e. we could no longer use the sweet
> `remove_dir_recursively()` call. Therefore, the implementation of what I
> suggested would be much more complicated: you would have to enumerate the
> ignored files and remove them individually.

Outside of "it's harder to write that feature", perhaps I could convince you that it is better to do nothing in the presence of untracked files.

If a user has an untracked file within a directory that is leaving the sparse cone, then that means they were doing something in that space and perhaps has unfinished work. By leaving the files on-disk, they have an opportunity to revert the change to the sparse-checkout cone and continue their work interrupted. This includes keeping things like build artifacts (that are ignored) so they can incrementally build from that position.

The general thought I have here is: having untracked, not-ignored files in a directory that is leaving the sparse-checkout cone is an unexpected behavior, so we should do as little as possible in that scenario.

It also makes it more clear to the user what happened: "You had untracked files here, so we left them alone" is easier to understand than "You had untracked files here, so we deleted the ones that were ignored and kept the rest."

> Having said that, even after mulling over this behavior and sleeping over
> it, I am unsure what the best way forward would be. Just because it is
> easy to explain does not make it right.

Of course, you already have a retort to my claim that "simpler is better", but I'll just focus on the point that "simpler for the user to understand" is a different point than "simpler to implement".

> Which leaves me to wonder whether we need at least a flag to turn this
> behavior on and off? Something like
> `core.ignoredFilesInSparseConesArePrecious = true` (obviously with a
> better, shorter name).

You are right that there are a lot of users out there, and they have very different habits. Those habits are shaped by the way Git has worked for a long time, so adding a way to go back to the previous behavior would be good to have.

Thanks, -Stolee