{"thread":{"id":"28946","subject":"[PATCH] apply: squash consecutive slashes with p_value > 0","startedAt":"2011-11-16T12:04:03Z","lastAt":"2011-11-17T18:57:19Z","messageCount":5,"participants":["Robie Basak","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"179582","messageId":"20111116120403.GA10342@mal.justgohome.co.uk","threadId":"28946","inReplyTo":null,"subject":"[PATCH] apply: squash consecutive slashes with p_value > 0","fromName":"Robie Basak","fromEmail":"robie.basak@canonical.com","sentAt":"2011-11-16T12:04:03Z","receivedAt":"2011-11-16T12:04:03Z","isPatch":true,"sender":{"key":"robie.basak@canonical.com","avatar":null},"body":"\"patch\" works with -p1 and diffs in the following form:\n    --- foo.orig//bar\n    +++ foo//bar\n    ...\n\npatch(1) says that \"A sequence of one or more adjacent slashes is\ncounted as a single slash.\"\n\nThis patch amends git-apply's filename parsing to behave in the same\nway, eliminating both '/'s with -p1 in this example.\n\nTest case included.\n\nSigned-off-by: Robie Basak <robie.basak@canonical.com>\n---\n builtin/apply.c              |    8 ++++++--\n t/t4153-apply-doubleslash.sh |   29 +++++++++++++++++++++++++++++\n 2 files changed, 35 insertions(+), 2 deletions(-)\n create mode 100755 t/t4153-apply-doubleslash.sh\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 84a8a0b..78e25fa 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -627,9 +627,13 @@ static char *find_name_common(const char *line, char *def, int p_value,\n \t\t\tif (name_terminate(start, line-start, c, terminate))\n \t\t\t\tbreak;\n \t\t}\n-\t\tline++;\n-\t\tif (c == '/' && !--p_value)\n+\t\tif (c == '/' && !--p_value) {\n+\t\t\twhile (*line == '/')\n+\t\t\t    line++;\n \t\t\tstart = line;\n+\t\t} else {\n+\t\t\tline++;\n+\t\t}\n \t}\n \tif (!start)\n \t\treturn squash_slash(def);\ndiff --git a/t/t4153-apply-doubleslash.sh b/t/t4153-apply-doubleslash.sh\nnew file mode 100755\nindex 0000000..1ea76b5\n--- /dev/null\n+++ b/t/t4153-apply-doubleslash.sh\n@@ -0,0 +1,29 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2011 Canonical Ltd.\n+#\n+\n+test_description='git apply filenames with double slashes\n+\n+Try to apply a patch with git-apply where the filename specified has a double\n+slash after the first (to-be-stripped) component'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_commit 1 f &&\n+\tcat > good.patch <<EOF\n+diff -u a//f b//f\n+--- a//f\n++++ b//f\n+@@ -1 +1 @@\n+-1\n++2\n+EOF\n+'\n+\n+test_expect_success 'apply diff with double slashes in filenames' '\n+\tgit apply good.patch 2>err\n+'\n+\n+test_done\n-- \n1.7.5.4\n"},{"id":"179595","messageId":"7v62ikq89h.fsf@alter.siamese.dyndns.org","threadId":"28946","inReplyTo":"20111116120403.GA10342@mal.justgohome.co.uk","subject":"Re: [PATCH] apply: squash consecutive slashes with p_value > 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-16T17:36:42Z","receivedAt":"2011-11-16T17:36:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robie Basak <robie.basak@canonical.com> writes:\n\n> \"patch\" works with -p1 and diffs in the following form:\n>     --- foo.orig//bar\n>     +++ foo//bar\n>     ...\n>\n> patch(1) says that \"A sequence of one or more adjacent slashes is\n> counted as a single slash.\"\n\nIt merely says patch(1) treats duplicate slashes that way; it does not\nmean it is a useful thing to do in the real life.\n\nCould you justify why such a change is a good thing in your proposed\ncommit log message?\n\n>  builtin/apply.c              |    8 ++++++--\n>  t/t4153-apply-doubleslash.sh |   29 +++++++++++++++++++++++++++++\n>  2 files changed, 35 insertions(+), 2 deletions(-)\n>  create mode 100755 t/t4153-apply-doubleslash.sh\n\nCan we avoid wasting a new test number for just a single trivial test by\nfindign appropriate places in existing tests to add new ones to? I think\n4133 may be one of the good places (not just checkint between a/f vs b/f,\nyou would check a//f vs b/f and somesuch).\n\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 84a8a0b..78e25fa 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -627,9 +627,13 @@ static char *find_name_common(const char *line, char *def, int p_value,\n>  \t\t\tif (name_terminate(start, line-start, c, terminate))\n>  \t\t\t\tbreak;\n>  \t\t}\n> -\t\tline++;\n> -\t\tif (c == '/' && !--p_value)\n> +\t\tif (c == '/' && !--p_value) {\n> +\t\t\twhile (*line == '/')\n> +\t\t\t    line++;\n>  \t\t\tstart = line;\n\nI am not sure if this is sufficient or necessarily correct.\n\nYou are de-dupling slashes only when p_value hits 0. When working on an\ninput \"a///b//c\" with -p3, your loop strips one slash per decrement of\np_value between \"a\" and \"b\" and then you notice slashes after \"b\" are\nduplicated and clean them up, which would mean you normalized the input to\n\"a///b/c\", not \"a/b/c\", no?\n"},{"id":"179642","messageId":"20111117175544.GC17472@mal.justgohome.co.uk","threadId":"28946","inReplyTo":"7vr516myqh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] apply: squash consecutive slashes with p_value > 0","fromName":"Robie Basak","fromEmail":"robie.basak@canonical.com","sentAt":"2011-11-17T17:55:44Z","receivedAt":"2011-11-17T17:55:44Z","isPatch":true,"sender":{"key":"robie.basak@canonical.com","avatar":null},"body":"On Thu, Nov 17, 2011 at 09:43:02AM -0800, Junio C Hamano wrote:\n> I didn't mention this, but I suspect the code with your patch would not\n> touch redundant slashes after it finds \"start\", either.\n\nWhat is happening to me is that b//foo with -p1 gets split at the first\nslash and then git apply cannot find /foo when it should be looking for\nfoo. But in the case of b/foo//bar and -p1, without squashing extra\nslashes it would look for foo//bar which I presume that it would still\nbe able to find.\n\nSo in my case, I only need duplicate slashes around the -p boundary\npoint to be removed. I assumed that the squash_slash() later on would\neliminate the rest, but I didn't look into this; if it does it'd be a\ndifferent issue to the one that I'm seeing.\n\nI'm now confused about what it will do (which is why I need to look at\nit again to make sure), but if it turns out to be easier to just handle\nthat one boundary point, would you accept a patch that eliminates just\nthat duplicate, on the basis that in Unix-land duplicate slashes are\nperfectly acceptable to be left lying around?\n\nThanks,\n\nRobie\n"},{"id":"179643","messageId":"7vmxbumxls.fsf@alter.siamese.dyndns.org","threadId":"28946","inReplyTo":"20111117175544.GC17472@mal.justgohome.co.uk","subject":"Re: [PATCH] apply: squash consecutive slashes with p_value > 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-17T18:07:27Z","receivedAt":"2011-11-17T18:07:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robie Basak <robie.basak@canonical.com> writes:\n\n> I'm now confused about what it will do (which is why I need to look at\n> it again to make sure), but if it turns out to be easier to just handle\n> that one boundary point, would you accept a patch that eliminates just\n> that duplicate, on the basis that in Unix-land duplicate slashes are\n> perfectly acceptable to be left lying around?\n\nIf that is the case, I would rather say we should even shoot for\nsimpler. Just tell the patch generator not to include unneeded double\nslashes.\n"},{"id":"179644","messageId":"20111117185718.GE17472@mal.justgohome.co.uk","threadId":"28946","inReplyTo":"7vmxbumxls.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] apply: squash consecutive slashes with p_value > 0","fromName":"Robie Basak","fromEmail":"robie.basak@canonical.com","sentAt":"2011-11-17T18:57:19Z","receivedAt":"2011-11-17T18:57:19Z","isPatch":true,"sender":{"key":"robie.basak@canonical.com","avatar":null},"body":"On Thu, Nov 17, 2011 at 10:07:27AM -0800, Junio C Hamano wrote:\n> If that is the case, I would rather say we should even shoot for\n> simpler. Just tell the patch generator not to include unneeded double\n> slashes.\n\nI don't have a choice about the patch generator. These are humans typing\n\"diff -ur foo.orig/ foo/\" which I think is reasonable.\n\nIMHO, this is a bug in git-apply's -p behaviour. It's not so much about\nstripping double slashes; more about doing the -p split in the middle of\nthe doubled slashes rather than the end. patch does the right thing.\n"}]}