{"thread":{"id":"45603","subject":"[PATCH v2] pathspec: honor `PATHSPEC_PREFIX_ORIGIN` with empty prefix","startedAt":"2017-04-04T09:16:10Z","lastAt":"2017-04-12T08:47:28Z","messageCount":5,"participants":["Patrick Steinhardt","Brandon Williams","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"316141","messageId":"4650e1396a5ad006b2b4f355685e3c59efd554d6.1491297393.git.ps@pks.im","threadId":"45603","inReplyTo":null,"subject":"[PATCH v2] pathspec: honor `PATHSPEC_PREFIX_ORIGIN` with empty prefix","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2017-04-04T09:16:56Z","receivedAt":"2017-04-04T09:16:10Z","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 when\n`PATHSPEC_PREFIX_ORIGIN` is set and global literal pathspecs are\ndeactivated. Note that in contrast to previous behavior, we will now\nalways call `prefix_magic` regardless of whether a prefix is actually\nset. But this is the right thing to do: when the `match` variable has\nbeen resolved to the repository's root, it will be set to an empty\nstring. When passing the empty string directly to other commands, it\nwill result in a warning regarding deprecated empty pathspecs. By always\nadding the prefix magic, we will end up with at least the string\n\":(prefix:0)\" and thus avoid the warning.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n\nThis is the second version of [1]. It fixes a bug catched by\nBrandon when the pathspec is resolved to the empty string and\nimproves the test a bit to actually catch this issue.\n\n[1]: http://public-inbox.org/git/1556910880cfce391bdca2d8f0cbcb8c71371691.1491206540.git.ps@pks.im/T/#u\n\n\n pathspec.c                 |  2 +-\n t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 303efda83..3079a817a 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -505,7 +505,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t * original. Useful for passing to another command.\n \t */\n \tif ((flags & PATHSPEC_PREFIX_ORIGIN) &&\n-\t    prefixlen && !get_literal_global()) {\n+\t    !get_literal_global()) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\t/* Preserve the actual prefix length of each pattern */\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex f9528fa00..2ecb43a61 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -436,6 +436,28 @@ 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 >relpath.c &&\n+\tgit add \"*.c\" &&\n+\tgit commit -m relpath &&\n+\n+\techo change >relpath.c &&\n+\tmkdir -p subdir &&\n+\tgit -C subdir add -p .. 2>error <<-\\EOF &&\n+\ty\n+\tEOF\n+\n+\ttest_must_be_empty error &&\n+\n+\tcat >expect <<-\\EOF &&\n+\trelpath.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":"316161","messageId":"20170404163909.GA189807@google.com","threadId":"45603","inReplyTo":"4650e1396a5ad006b2b4f355685e3c59efd554d6.1491297393.git.ps@pks.im","subject":"Re: [PATCH v2] pathspec: honor `PATHSPEC_PREFIX_ORIGIN` with empty prefix","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-04T16:39:09Z","receivedAt":"2017-04-04T16:39:16Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/04, 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 when\n> `PATHSPEC_PREFIX_ORIGIN` is set and global literal pathspecs are\n> deactivated. Note that in contrast to previous behavior, we will now\n> always call `prefix_magic` regardless of whether a prefix is actually\n> set. But this is the right thing to do: when the `match` variable has\n> been resolved to the repository's root, it will be set to an empty\n> string. When passing the empty string directly to other commands, it\n> will result in a warning regarding deprecated empty pathspecs. By always\n> adding the prefix magic, we will end up with at least the string\n> \":(prefix:0)\" and thus avoid the warning.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> \n> This is the second version of [1]. It fixes a bug catched by\n> Brandon when the pathspec is resolved to the empty string and\n> improves the test a bit to actually catch this issue.\n\nThis version looks good to me.  Thanks for fixing that small issue!\n\n-- \nBrandon Williams\n"},{"id":"316202","messageId":"CACsJy8AeZWmGuvEqoA4y6rM-kiz6_7r31Q4kqor7+sHSwgZ1sQ@mail.gmail.com","threadId":"45603","inReplyTo":"20170404163909.GA189807@google.com","subject":"Re: [PATCH v2] pathspec: honor `PATHSPEC_PREFIX_ORIGIN` with empty prefix","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-05T13:46:19Z","receivedAt":"2017-04-05T13:47:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Apr 4, 2017 at 11:39 PM, Brandon Williams <bmwill@google.com> wrote:\n>> This is the second version of [1]. It fixes a bug catched by\n>> Brandon when the pathspec is resolved to the empty string and\n>> improves the test a bit to actually catch this issue.\n>\n> This version looks good to me.  Thanks for fixing that small issue!\n\nLooks good to me too (I didn't see this when I read v1, too much mails)\n-- \nDuy\n"},{"id":"316671","messageId":"20170412082205.GA448@pks-xps","threadId":"45603","inReplyTo":"4650e1396a5ad006b2b4f355685e3c59efd554d6.1491297393.git.ps@pks.im","subject":"Re: [PATCH v2] pathspec: honor `PATHSPEC_PREFIX_ORIGIN` with empty prefix","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2017-04-12T08:22:05Z","receivedAt":"2017-04-12T08:21:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 04, 2017 at 11:16:56AM +0200, 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 when\n> `PATHSPEC_PREFIX_ORIGIN` is set and global literal pathspecs are\n> deactivated. Note that in contrast to previous behavior, we will now\n> always call `prefix_magic` regardless of whether a prefix is actually\n> set. But this is the right thing to do: when the `match` variable has\n> been resolved to the repository's root, it will be set to an empty\n> string. When passing the empty string directly to other commands, it\n> will result in a warning regarding deprecated empty pathspecs. By always\n> adding the prefix magic, we will end up with at least the string\n> \":(prefix:0)\" and thus avoid the warning.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n\nJust a short reminder on this patch, as I haven't seen it or\nv1 being picked up by the What's Cooking reports. Am I simply\nbeing too eager or was this an oversight?\n\nThanks\nPatrick\n\n> \n> This is the second version of [1]. It fixes a bug catched by\n> Brandon when the pathspec is resolved to the empty string and\n> improves the test a bit to actually catch this issue.\n> \n> [1]: http://public-inbox.org/git/1556910880cfce391bdca2d8f0cbcb8c71371691.1491206540.git.ps@pks.im/T/#u\n> \n> \n>  pathspec.c                 |  2 +-\n>  t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++\n>  2 files changed, 23 insertions(+), 1 deletion(-)\n> \n> diff --git a/pathspec.c b/pathspec.c\n> index 303efda83..3079a817a 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -505,7 +505,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n>  \t * original. Useful for passing to another command.\n>  \t */\n>  \tif ((flags & PATHSPEC_PREFIX_ORIGIN) &&\n> -\t    prefixlen && !get_literal_global()) {\n> +\t    !get_literal_global()) {\n>  \t\tstruct strbuf sb = STRBUF_INIT;\n>  \n>  \t\t/* Preserve the actual prefix length of each pattern */\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index f9528fa00..2ecb43a61 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -436,6 +436,28 @@ 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 >relpath.c &&\n> +\tgit add \"*.c\" &&\n> +\tgit commit -m relpath &&\n> +\n> +\techo change >relpath.c &&\n> +\tmkdir -p subdir &&\n> +\tgit -C subdir add -p .. 2>error <<-\\EOF &&\n> +\ty\n> +\tEOF\n> +\n> +\ttest_must_be_empty error &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\trelpath.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"},{"id":"316673","messageId":"xmqqr30yovvs.fsf@gitster.mtv.corp.google.com","threadId":"45603","inReplyTo":"20170412082205.GA448@pks-xps","subject":"Re: [PATCH v2] pathspec: honor `PATHSPEC_PREFIX_ORIGIN` with empty prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-12T08:47:19Z","receivedAt":"2017-04-12T08:47:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Just a short reminder on this patch, as I haven't seen it or\n> v1 being picked up by the What's Cooking reports. Am I simply\n> being too eager or was this an oversight?\n\nI have been offline and will be for a few more days; I may\noccasionally pop in to give design level guidance on topics, but\nwon't be reading patches carefully enough to queue a new topic to my\ntree.  New topics won't be added to my tree until next week the\nearliest.\n"}]}