mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <>
To: Junio C Hamano <>
Cc: Derrick Stolee <>,
	Tim Renouf <>,
	Derrick Stolee <>,
	Git Mailing List <>
Subject: Re: [PATCH] unpack-trees: add core.sparseCheckoutRmFiles config
Date: Tue, 1 Jun 2021 22:53:58 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <xmqqmts9q9m2.fsf@gitster.g>

Derrick and Junio already made some really good points, I'll just try
to add a few comments.

On Tue, Jun 1, 2021 at 7:32 PM Junio C Hamano <> wrote:
> Derrick Stolee <> writes:
> >> This commit adds the above config item, defaulting to "true" to get the
> >> old behavior. Set it to "false" to avoid removing such a file from the
> >> worktree.
> >
> > I don't think config is necessary here. This behavior is niche
> > enough to create a behavior-breaking change. However, we do want
> > to ensure that the full flow of eventually clearing the file when
> > clean is handled.
> I didn't have a chance to get around to commenting on the patch
> (instead I was preparing for -rc3), but you covered pretty much
> everything I wanted to say.  It is unusual for those who are using
> sparse checkout to have a modified (=tracked and dirty) file that
> shouldn't be there, and making sure the user notices these unusual
> files, instead of silently losing the changes to them, is probably a
> "bugfix".
> An explicit request to destructively overwrite the path ("git
> restore -- path") or remove the working tree file along with
> modification ("git reset --hard") is a good thing to have, but
> the branch switching "git switch" is supposed to preserve local
> modification (or fail to switch), whether the dirty path is inside
> or outside the sparse checkout area.

Yes, I think this is the best phrasing of the bug.  Since
SKIP_WORKTREE was originally implemented as "assume file contents
match HEAD" (which is sometimes a reasonable first approximation to
the intended behavior, but which keeps leading us astray into bad
behavior), it's pretty easy to see what's likely happening here:
unpack-trees.c is almost certainly just assuming that the file
contents match HEAD and not even bothering to verify that assumption
before deleting the file.  We should make it verify when a file is

> > If we _are_ going to go with a config option, then I'm not a big
> > fan of this name. I've also been thinking that the sparse-checkout
> > config has been squatting in the "core.*" space too long. Perhaps
> > it is time to create its own section?
> >
> > What about something like sparseCheckout.keepDirtyFiles, which
> > defaults to false?
> What about not having a configuration?

I agree; we all concur that the reported behavior is a bug; adding a
config option to turn a bug off for some people just doesn't make any
sense.  Let's fix the bug instead.

I'm also worried that making a config option may have masked subtle
bugs in the patch that the rest of the testsuite would have turned up.

> > 1. How does a user with a dirty, tracked, sparse file get back
> >    into a state where that file is deleted? What commands do
> >    they need to run? Can that be tested and added to the sparse-
> >    checkout documentation?

Yeah, I've wondered if 'git sparse-checkout reapply' should do
something special here...or whether additional subcommands might be
needed.  I'm still not quite sure.

> > 2. What does 'git status' look like when a user is in this state?
> >    Is that helpful?

I'm worried that trying to make 'git status' report these files would
be expensive and defeat some of the advantages of a sparse-checkout.
Even in cone mode, and even if we could just stat the leading
directories to see they aren't there, would we still end up iterating
over all the index entries just to verify that their leading directory
is missing so we don't have to stat it?

Whereas checkout, if it might end up deleting something, it makes
perfect sense to pay the cost of a stat at that point and throw an
error message and abort if things aren't clean.

  reply	other threads:[~2021-06-02  5:54 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-28 20:14 bug report: git checkout deletes worktree file even though it is excluded by sparse-checkout Tim Renouf (open source)
2021-05-28 22:44 ` Elijah Newren
2021-06-01 18:31   ` [PATCH] unpack-trees: add core.sparseCheckoutRmFiles config Tim Renouf
2021-06-02  2:00     ` Derrick Stolee
2021-06-02  2:32       ` Junio C Hamano
2021-06-02  5:53         ` Elijah Newren [this message]
2021-06-02  6:13           ` Tim Renouf (open source)
2021-06-02 23:41             ` Elijah Newren
2021-06-05 15:33               ` Tim Renouf (open source)
2021-06-02  1:37 ` bug report: git checkout deletes worktree file even though it is excluded by sparse-checkout Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).