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@xxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature