Re: [PATCH] apply: remove prefix_length member from apply_state

From: Jeff King <peff@peff.net>
Date: Aug 11, 2017, 09:04 UTC
Message-ID: <20170811090442.lyamw5euek4wwbfl@sigill.intra.peff.net>
In-Reply-To: <746a8339-a490-0a10-a4af-ead78b7b7a6e@web.de>

On Fri, Aug 11, 2017 at 10:52:48AM +0200, René Scharfe wrote:

Show 15 quoted lines

> > I wondered at first whether it's actually necessary. Wouldn't
> > an empty prefix always match?
> > 
> > But I think we're looking for the pathname to be a proper superset of
> > the prefix (hence the "!*rest" check). So I guess an empty path would
> > not be a proper superset of an empty prefix, even though with the
> > current code it doesn't hit that block at all.
> > 
> > I'm still not sure it's possible to have an empty pathname, so that
> > corner case may be moot and we could simplify the condition a little.
> > But certainly as you've written it, it could not be a regression.
> 
> So you mean that removing the prefix length check would just cause
> empty paths to be rejected if we have an empty prefix, which shouldn't
> bother anyone because empty paths can't be used anyway, right?

Right.

> Actually I'm not even sure it's possible to have an empty, non-NULL
> prefix.

I'm not sure either. I had assumed this came from a --prefix argument to git-apply, but it looks like it only ever comes from setup.c's prefix. We seem to avoid empty prefixes there, but there are a lot of different code paths and I didn't check them all.

Show 9 quoted lines

> > The use of skip_prefix also took me a minute. I wonder if it's worth a
> > comment with the words "proper superset" or some other indicator (it
> > also surprised me that we're ok with matching "foobar" in the prefix
> > "foo", and not demanding "foo/bar". But I didn't look at the context to
> > know whether that's right or not. This may be a case where the prefix is
> > supposed to have "/" on it already).
> 
> As the literal translation it is intended to be it should have been a
> no-brainer. :)

Yeah. All of this is mostly me thinking out loud about whether we can improve the existing code further. Don't feel like you need to spend time on it.

Show 6 quoted lines

> Applying a patch to foobar when we're in foo/ is not intended ("Paths
> outside are not touched").
> 
> I don't know if prefixes are guaranteed to end with a slash; the code
> in setup.c seems to ensure that, but has it been spelled out
> explicitly somewhere?  apply.c::use_patch() certainly relies on that.

I don't know that it's been spelled out. But if you do this:

diff --git a/setup.c b/setup.c
index 860507e1fd..48af25cac1 100644
--- a/setup.c
+++ b/setup.c
@@ -765,7 +765,6 @@ static const char *setup_discovered_git_dir(const char *gitdir,
 	if (offset != offset_1st_component(cwd->buf))
 		offset++;
 	/* Add a '/' at the end */
-	strbuf_addch(cwd, '/');
 	return cwd->buf + offset;
 }
 

lots of tests break horribly. So I'm content that we'd probably catch a
regression there, even if it's not spelled out explicitly.

-Peff