On Fri, Aug 11, 2017 at 10:52:48AM +0200, René Scharfe wrote:
> > 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.
> > 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.
> 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
