mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: "René Scharfe" <>
Cc: Sergii Shkarnikov <>,
	Eric Sunshine <>,
	Git List <>
Subject: Re: Possible bug with git restore
Date: Mon, 24 Aug 2020 16:25:56 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Sat, Aug 22, 2020 at 12:29:23PM +0200, René Scharfe wrote:

> > So the fundamental issue is treating the pathspec in two different ways,
> > and then correlating the results. We need to either do a recursive match
> > for the tree match (as your patch does), or do non-recursive for this
> > index match (which I don't think is trivial, because of the way the
> > recursive flag works).
> If using the same pathspec with both tree_entry_interesting and
> match_pathspec gives inconsistent results and can even lead to data loss
> as we've seen here, then we better prevent it.

Yeah, it would be nice to do away with this inconsistency entirely. I
don't know if that even causes user-visible behavior changes at this
point, though.

> The easiest way to do that would be to BUG out in match_pathspec if
> recursive is unset, to indicate that it doesn't support non-recursive
> matching.  Finding all the places that didn't bothered to set this flag
> since it doesn't affect match_pathspec anyway would be quite tedious,
> though.

Yeah. I think it's easier to approach it from the tree-entry side, and
say: which spots are not recursive, and could/should they be. Which I
think is where your patch below is going.

> At least the test suite still completes with the following evil patch
> and the fix I sent earlier (evil because it ignores const), so we
> currently don't have any other mismatches in covered code.

That's kind-of good news, in that it lends support to the notion that
basically everything wants to be recursive. But I'd be worried there's a
lingering code path that is not covered in the test suite. Certainly
shipping your BUG() patch would flush it out, but it's not very friendly
to users who do run into it.

I wonder if we can drop it to a warning() and see if anybody reports it.

Or the more-work but more-responsible version: manually trace all of the
pathspec calls to see which code paths might rely on leaving "recursive"


  reply	other threads:[~2020-08-24 20:25 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-12 18:51 Possible bug with git restore Sergii Shkarnikov
2020-08-14 22:41 ` Eric Sunshine
2020-08-20 12:59   ` Sergii Shkarnikov
2020-08-20 13:40     ` Jeff King
2020-08-20 17:48       ` René Scharfe
2020-08-20 18:27         ` Jeff King
2020-08-22  8:57           ` [PATCH] checkout, restore: make pathspec recursive René Scharfe
2020-08-24 20:21             ` Jeff King
2020-08-22 10:29           ` Possible bug with git restore René Scharfe
2020-08-24 20:25             ` Jeff King [this message]
2020-08-22 19:36           ` Junio C Hamano

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 \ \ \ \ \ \ \

* 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).