{"thread":{"id":"45595","subject":"[PATCH] pathspec: always honor `PATHSPEC_PREFIX_ORIGIN` flag","startedAt":"2017-04-03T08:04:06Z","lastAt":"2017-04-05T13:41:08Z","messageCount":4,"participants":["Patrick Steinhardt","Brandon Williams","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316086","messageId":"1556910880cfce391bdca2d8f0cbcb8c71371691.1491206540.git.ps@pks.im","threadId":"45595","inReplyTo":null,"subject":"[PATCH] pathspec: always honor `PATHSPEC_PREFIX_ORIGIN` flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2017-04-03T08:03:02Z","receivedAt":"2017-04-03T08:04:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Previous to commit 5d8f084a5 (pathspec: simpler logic to prefix original\npathspec elements, 2017-01-04), we were always using the computed\n`match` variable to perform pathspec matching whenever\n`PATHSPEC_PREFIX_ORIGIN` is set. This is for example useful when passing\nthe parsed pathspecs to other commands, as the computed `match` may\ncontain a pathspec relative to the repository root. The commit changed\nthis logic to only do so when we do have an actual prefix and when\nliteral pathspecs are deactivated.\n\nBut this change may actually break some commands which expect passed\npathspecs to be relative to the repository root. One such case is `git\nadd --patch`, which now fails when using relative paths from a\nsubdirectory. For example if executing \"git add -p ../foo.c\" in a\nsubdirectory, the `git-add--interactive` command will directly pass\n\"../foo.c\" to `git-ls-files`. As ls-files is executed at the\nrepository's root, the command will notice that \"../foo.c\" is outside\nthe repository and fail.\n\nFix the issue by again using the computed `match` variable whenever\n`PATHSPEC_PREFIX_ORIGIN` is set. This restores behavior previous to\n5d8f084a5 and fixes interactive add.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n pathspec.c                 |  6 +++---\n t/t3701-add-interactive.sh | 20 ++++++++++++++++++++\n 2 files changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 303efda83..3193e45a6 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -504,12 +504,12 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t * Prefix the pathspec (keep all magic) and assign to\n \t * original. Useful for passing to another command.\n \t */\n-\tif ((flags & PATHSPEC_PREFIX_ORIGIN) &&\n-\t    prefixlen && !get_literal_global()) {\n+\tif (flags & PATHSPEC_PREFIX_ORIGIN) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\t/* Preserve the actual prefix length of each pattern */\n-\t\tprefix_magic(&sb, prefixlen, element_magic);\n+\t\tif (prefixlen && !get_literal_global())\n+\t\t\tprefix_magic(&sb, prefixlen, element_magic);\n \n \t\tstrbuf_addstr(&sb, match);\n \t\titem->original = strbuf_detach(&sb, NULL);\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex f9528fa00..640088dd6 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -436,6 +436,26 @@ test_expect_success 'add -p handles globs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'add -p handles relative paths' '\n+\tgit reset --hard &&\n+\n+\techo base >root.c &&\n+\tgit add \"*.c\" &&\n+\tgit commit -m base &&\n+\n+\techo change >root.c &&\n+\tmkdir -p subdir &&\n+\tgit -C subdir add -p \"../root.c\" <<-\\EOF &&\n+\ty\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\troot.c\n+\tEOF\n+\tgit diff --cached --name-only >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'add -p does not expand argument lists' '\n \tgit reset --hard &&\n \n-- \n2.12.2\n\n"},{"id":"316095","messageId":"20170403162648.GA166794@google.com","threadId":"45595","inReplyTo":"1556910880cfce391bdca2d8f0cbcb8c71371691.1491206540.git.ps@pks.im","subject":"Re: [PATCH] pathspec: always honor `PATHSPEC_PREFIX_ORIGIN` flag","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-03T16:26:48Z","receivedAt":"2017-04-03T16:26:57Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/03, Patrick Steinhardt wrote:\n> Previous to commit 5d8f084a5 (pathspec: simpler logic to prefix original\n> pathspec elements, 2017-01-04), we were always using the computed\n> `match` variable to perform pathspec matching whenever\n> `PATHSPEC_PREFIX_ORIGIN` is set. This is for example useful when passing\n> the parsed pathspecs to other commands, as the computed `match` may\n> contain a pathspec relative to the repository root. The commit changed\n> this logic to only do so when we do have an actual prefix and when\n> literal pathspecs are deactivated.\n> \n> But this change may actually break some commands which expect passed\n> pathspecs to be relative to the repository root. One such case is `git\n> add --patch`, which now fails when using relative paths from a\n> subdirectory. For example if executing \"git add -p ../foo.c\" in a\n> subdirectory, the `git-add--interactive` command will directly pass\n> \"../foo.c\" to `git-ls-files`. As ls-files is executed at the\n> repository's root, the command will notice that \"../foo.c\" is outside\n> the repository and fail.\n> \n> Fix the issue by again using the computed `match` variable whenever\n> `PATHSPEC_PREFIX_ORIGIN` is set. This restores behavior previous to\n> 5d8f084a5 and fixes interactive add.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  pathspec.c                 |  6 +++---\n>  t/t3701-add-interactive.sh | 20 ++++++++++++++++++++\n>  2 files changed, 23 insertions(+), 3 deletions(-)\n> \n> diff --git a/pathspec.c b/pathspec.c\n> index 303efda83..3193e45a6 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -504,12 +504,12 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n>  \t * Prefix the pathspec (keep all magic) and assign to\n>  \t * original. Useful for passing to another command.\n>  \t */\n> -\tif ((flags & PATHSPEC_PREFIX_ORIGIN) &&\n> -\t    prefixlen && !get_literal_global()) {\n> +\tif (flags & PATHSPEC_PREFIX_ORIGIN) {\n>  \t\tstruct strbuf sb = STRBUF_INIT;\n>  \n>  \t\t/* Preserve the actual prefix length of each pattern */\n> -\t\tprefix_magic(&sb, prefixlen, element_magic);\n> +\t\tif (prefixlen && !get_literal_global())\n> +\t\t\tprefix_magic(&sb, prefixlen, element_magic);\n>  \n>  \t\tstrbuf_addstr(&sb, match);\n>  \t\titem->original = strbuf_detach(&sb, NULL);\n\nWould it just make sense to drop the requirement that prefixlen be\nnon-zero?  My problem with this change currently is the ability to get\nan original string with is empty (ie \"\\0\") which would cause git to\nthrow some warnings about not allowing empty strings as pathspecs if\nthey were then passed on to other processes.\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index f9528fa00..640088dd6 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -436,6 +436,26 @@ test_expect_success 'add -p handles globs' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'add -p handles relative paths' '\n> +\tgit reset --hard &&\n> +\n> +\techo base >root.c &&\n> +\tgit add \"*.c\" &&\n> +\tgit commit -m base &&\n> +\n> +\techo change >root.c &&\n> +\tmkdir -p subdir &&\n> +\tgit -C subdir add -p \"../root.c\" <<-\\EOF &&\n> +\ty\n> +\tEOF\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\troot.c\n> +\tEOF\n> +\tgit diff --cached --name-only >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'add -p does not expand argument lists' '\n>  \tgit reset --hard &&\n>  \n> -- \n> 2.12.2\n> \n\n-- \nBrandon Williams\n"},{"id":"316142","messageId":"20170404091757.GA4584@pks-xps","threadId":"45595","inReplyTo":"20170403162648.GA166794@google.com","subject":"Re: [PATCH] pathspec: always honor `PATHSPEC_PREFIX_ORIGIN` flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2017-04-04T09:17:57Z","receivedAt":"2017-04-04T09:17:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 03, 2017 at 09:26:48AM -0700, Brandon Williams wrote:\n> On 04/03, Patrick Steinhardt wrote:\n> > Previous to commit 5d8f084a5 (pathspec: simpler logic to prefix original\n> > pathspec elements, 2017-01-04), we were always using the computed\n> > `match` variable to perform pathspec matching whenever\n> > `PATHSPEC_PREFIX_ORIGIN` is set. This is for example useful when passing\n> > the parsed pathspecs to other commands, as the computed `match` may\n> > contain a pathspec relative to the repository root. The commit changed\n> > this logic to only do so when we do have an actual prefix and when\n> > literal pathspecs are deactivated.\n> > \n> > But this change may actually break some commands which expect passed\n> > pathspecs to be relative to the repository root. One such case is `git\n> > add --patch`, which now fails when using relative paths from a\n> > subdirectory. For example if executing \"git add -p ../foo.c\" in a\n> > subdirectory, the `git-add--interactive` command will directly pass\n> > \"../foo.c\" to `git-ls-files`. As ls-files is executed at the\n> > repository's root, the command will notice that \"../foo.c\" is outside\n> > the repository and fail.\n> > \n> > Fix the issue by again using the computed `match` variable whenever\n> > `PATHSPEC_PREFIX_ORIGIN` is set. This restores behavior previous to\n> > 5d8f084a5 and fixes interactive add.\n> > \n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  pathspec.c                 |  6 +++---\n> >  t/t3701-add-interactive.sh | 20 ++++++++++++++++++++\n> >  2 files changed, 23 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/pathspec.c b/pathspec.c\n> > index 303efda83..3193e45a6 100644\n> > --- a/pathspec.c\n> > +++ b/pathspec.c\n> > @@ -504,12 +504,12 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n> >  \t * Prefix the pathspec (keep all magic) and assign to\n> >  \t * original. Useful for passing to another command.\n> >  \t */\n> > -\tif ((flags & PATHSPEC_PREFIX_ORIGIN) &&\n> > -\t    prefixlen && !get_literal_global()) {\n> > +\tif (flags & PATHSPEC_PREFIX_ORIGIN) {\n> >  \t\tstruct strbuf sb = STRBUF_INIT;\n> >  \n> >  \t\t/* Preserve the actual prefix length of each pattern */\n> > -\t\tprefix_magic(&sb, prefixlen, element_magic);\n> > +\t\tif (prefixlen && !get_literal_global())\n> > +\t\t\tprefix_magic(&sb, prefixlen, element_magic);\n> >  \n> >  \t\tstrbuf_addstr(&sb, match);\n> >  \t\titem->original = strbuf_detach(&sb, NULL);\n> \n> Would it just make sense to drop the requirement that prefixlen be\n> non-zero?  My problem with this change currently is the ability to get\n> an original string with is empty (ie \"\\0\") which would cause git to\n> throw some warnings about not allowing empty strings as pathspecs if\n> they were then passed on to other processes.\n\nYou're right. My patch results in:\n\n$ git init repo\n$ cd repo\n$ touch file\n$ git add file\n$ git commit -mfile\n$ echo foo>file\n$ mkdir subdir\n$ cd subdir\n$ git add -p ..\nwarning: empty strings as pathspecs will be made invalid...\nwarning: empty strings as pathspecs will be made invalid...\n\nDropping only the `prefixlen` condition and then unconditionally\ncalling `prefix_magic` suffices to fix the actual bug. I've\nimproved the test to use a pathspec which resolves to \"\" and\ncheck stderr.\n\nThanks for catching this!\n\nRegards\nPatrick\n"},{"id":"316199","messageId":"CACsJy8CYzJv1RfC9-Z5gW_4a2cCX+0MbDtpgRD5kgoBDaQt67w@mail.gmail.com","threadId":"45595","inReplyTo":"20170403162648.GA166794@google.com","subject":"Re: [PATCH] pathspec: always honor `PATHSPEC_PREFIX_ORIGIN` flag","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-05T13:40:28Z","receivedAt":"2017-04-05T13:41:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Apr 3, 2017 at 11:26 PM, Brandon Williams <bmwill@google.com> wrote:\n> On 04/03, Patrick Steinhardt wrote:\n>> Previous to commit 5d8f084a5 (pathspec: simpler logic to prefix original\n>> pathspec elements, 2017-01-04), we were always using the computed\n>> `match` variable to perform pathspec matching whenever\n>> `PATHSPEC_PREFIX_ORIGIN` is set. This is for example useful when passing\n>> the parsed pathspecs to other commands, as the computed `match` may\n>> contain a pathspec relative to the repository root. The commit changed\n>> this logic to only do so when we do have an actual prefix and when\n>> literal pathspecs are deactivated.\n>>\n>> But this change may actually break some commands which expect passed\n>> pathspecs to be relative to the repository root. One such case is `git\n>> add --patch`, which now fails when using relative paths from a\n>> subdirectory. For example if executing \"git add -p ../foo.c\" in a\n>> subdirectory, the `git-add--interactive` command will directly pass\n>> \"../foo.c\" to `git-ls-files`. As ls-files is executed at the\n>> repository's root, the command will notice that \"../foo.c\" is outside\n>> the repository and fail.\n\nOops. Sorry I missed this.\n\n>> @@ -504,12 +504,12 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n>>        * Prefix the pathspec (keep all magic) and assign to\n>>        * original. Useful for passing to another command.\n>>        */\n>> -     if ((flags & PATHSPEC_PREFIX_ORIGIN) &&\n>> -         prefixlen && !get_literal_global()) {\n>> +     if (flags & PATHSPEC_PREFIX_ORIGIN) {\n>>               struct strbuf sb = STRBUF_INIT;\n>>\n>>               /* Preserve the actual prefix length of each pattern */\n>> -             prefix_magic(&sb, prefixlen, element_magic);\n>> +             if (prefixlen && !get_literal_global())\n>> +                     prefix_magic(&sb, prefixlen, element_magic);\n>>\n>>               strbuf_addstr(&sb, match);\n>>               item->original = strbuf_detach(&sb, NULL);\n>\n> Would it just make sense to drop the requirement that prefixlen be\n> non-zero?  My problem with this change currently is the ability to get\n> an original string with is empty (ie \"\\0\") which would cause git to\n> throw some warnings about not allowing empty strings as pathspecs if\n> they were then passed on to other processes.\n\nGood catch.\n\nI did wonder if it's a right thing, because the result pathspec is\n':(prefix:0)'. After leaving out the magic path, you get an empty\n\"path\" of the pathspec, which probably should be warned because we\nwouldn't be able to handle it. For example, \"git add .\" at root will\ngive you the path \".\", not empty one. Maybe we can't handle the empty\n\"path\" part.\n\nBut we have an exception for this already. In \"git add :/\", the \"path\"\nof the pathspec is still empty ('/' is a magic, not \"path\" even if it\nlooks like so) and we handle it just fine. So everything should be\ngood here.\n\nPatrick, please add a line or two about why you drop prefixlen when\nyou re-roll (or even better, make it a separate patch; this sounds\nlike an issue even before the code reorganization changes).\n-- \nDuy\n"}]}