Re: [PATCH] pathspec: always honor `PATHSPEC_PREFIX_ORIGIN` flag

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3375 bytes --]

On Mon, Apr 03, 2017 at 09:26:48AM -0700, Brandon Williams wrote:
> On 04/03, Patrick Steinhardt wrote:
> > Previous to commit 5d8f084a5 (pathspec: simpler logic to prefix original
> > pathspec elements, 2017-01-04), we were always using the computed
> > `match` variable to perform pathspec matching whenever
> > `PATHSPEC_PREFIX_ORIGIN` is set. This is for example useful when passing
> > the parsed pathspecs to other commands, as the computed `match` may
> > contain a pathspec relative to the repository root. The commit changed
> > this logic to only do so when we do have an actual prefix and when
> > literal pathspecs are deactivated.
> > 
> > But this change may actually break some commands which expect passed
> > pathspecs to be relative to the repository root. One such case is `git
> > add --patch`, which now fails when using relative paths from a
> > subdirectory. For example if executing "git add -p ../foo.c" in a
> > subdirectory, the `git-add--interactive` command will directly pass
> > "../foo.c" to `git-ls-files`. As ls-files is executed at the
> > repository's root, the command will notice that "../foo.c" is outside
> > the repository and fail.
> > 
> > Fix the issue by again using the computed `match` variable whenever
> > `PATHSPEC_PREFIX_ORIGIN` is set. This restores behavior previous to
> > 5d8f084a5 and fixes interactive add.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  pathspec.c                 |  6 +++---
> >  t/t3701-add-interactive.sh | 20 ++++++++++++++++++++
> >  2 files changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/pathspec.c b/pathspec.c
> > index 303efda83..3193e45a6 100644
> > --- a/pathspec.c
> > +++ b/pathspec.c
> > @@ -504,12 +504,12 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
> >  	 * Prefix the pathspec (keep all magic) and assign to
> >  	 * original. Useful for passing to another command.
> >  	 */
> > -	if ((flags & PATHSPEC_PREFIX_ORIGIN) &&
> > -	    prefixlen && !get_literal_global()) {
> > +	if (flags & PATHSPEC_PREFIX_ORIGIN) {
> >  		struct strbuf sb = STRBUF_INIT;
> >  
> >  		/* Preserve the actual prefix length of each pattern */
> > -		prefix_magic(&sb, prefixlen, element_magic);
> > +		if (prefixlen && !get_literal_global())
> > +			prefix_magic(&sb, prefixlen, element_magic);
> >  
> >  		strbuf_addstr(&sb, match);
> >  		item->original = strbuf_detach(&sb, NULL);
> 
> Would it just make sense to drop the requirement that prefixlen be
> non-zero?  My problem with this change currently is the ability to get
> an original string with is empty (ie "\0") which would cause git to
> throw some warnings about not allowing empty strings as pathspecs if
> they were then passed on to other processes.

You're right. My patch results in:

$ git init repo
$ cd repo
$ touch file
$ git add file
$ git commit -mfile
$ echo foo>file
$ mkdir subdir
$ cd subdir
$ git add -p ..
warning: empty strings as pathspecs will be made invalid...
warning: empty strings as pathspecs will be made invalid...

Dropping only the `prefixlen` condition and then unconditionally
calling `prefix_magic` suffices to fix the actual bug. I've
improved the test to use a pathspec which resolves to "" and
check stderr.

Thanks for catching this!

Regards
Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]