{"thread":{"id":"53911","subject":"[BUG] git-cherry false-positive on upstream new-line change at end-of-file","startedAt":"2020-07-23T14:42:14Z","lastAt":"2020-08-27T09:05:41Z","messageCount":6,"participants":["Tilman Vogel","René Scharfe","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"401928","messageId":"2639321.dTF8K4C05n@alien.lan","threadId":"53911","inReplyTo":null,"subject":"[BUG] git-cherry false-positive on upstream new-line change at end-of-file","fromName":"Tilman Vogel","fromEmail":"tilman.vogel@web.de","sentAt":"2020-07-23T14:42:08Z","receivedAt":"2020-07-23T14:42:14Z","isPatch":false,"sender":{"key":"tilman.vogel@web.de","avatar":"https://avatars.githubusercontent.com/u/349303?v=4"},"body":"Thank you for filling out a Git bug report! \nPlease answer the following questions to help us understand your issue. \n\nWhat did you do before the bug happened? (Steps to reproduce your issue) \n\nI was working on two branches \"master\" and \"feature\". On \"master\" I  \n\"git cherry-pick feature\" in order to pick the latest commit from \n\"feature\".  \n\nWhat did you expect to happen? (Expected behavior) \n\nWhen I run \"git cherry master feature\" the picked commit is \nmarked with \"-\". \n\nWhat happened instead? (Actual behavior) \n\nWhen I run \"git cherry master feature\" the picked commit is \nmarked with \"+\". \n\nWhat's different between what you expected and what actually happened? \n\nBecause the commit has just been picked, it should be marked as \"-\". \nThe actual behavior claims the commit had not been picked. \n\nAnything else you want to add: \n\nIt is important to note that the only change, I did on master after \nbranching \"feature\", was to add a missing newline at the end of the \nfile that I changed on \"feature\".  \n\nIf I replay the same scenario with a file that has a newline at the end \nand I am only adding a newline at the beginning of it on master, cherry \ngives the expected \"-\" after cherry-picking. \n\nPlease see this simple example: \n\n> git clone https://github.com/tvogel/cherry-broken.git \n> cd cherry-broken \n\n# see broken behaviour: \n> git cherry origin/master origin/feature \n+ 26f77def8f1380cc3fd08ebc3f70652da79f1a2e \n\n# check that patch-id is actually the same: \n> git show --patch origin/master | git patch-id \nca5d666a572ec32bff9cec17b97d611316f89236 \n0725b090dd82f2e6f9949ce853778391e60b08d1 \n> git show --patch origin/feature | git patch-id \nca5d666a572ec32bff9cec17b97d611316f89236 \n26f77def8f1380cc3fd08ebc3f70652da79f1a2e \n\n# see OK behaviour: \n> git cherry origin/master-ok origin/feature-ok \n- 7ce2c0ecd3024c04bcf7b78d332841b2e791e06f \n\nIn the \"*-ok\" branches, the file has newline at the end from the beginning \nand \"master\" only inserts a newline at the beginning of the file. \"feature-ok\"\nhas the same change as \"feature\" (content-wise).\n\n\nPlease review the rest of the bug report below. \nYou can delete any lines you don't wish to share. \n\n\n[System Info] \ngit version: \ngit version 2.27.0 \ncpu: x86_64 \nno commit associated with this build \nsizeof-long: 8 \nsizeof-size_t: 8 \nuname: Linux 5.7.5-1-default #1 SMP Tue Jun 23 06:00:46 UTC 2020 (a1775d0) \nx86_64 \ncompiler info: gnuc: 10.1 \nlibc info: glibc: 2.31 \n\n\n[Enabled Hooks] \nnot run from a git repository - no hooks to show\n\n\n"},{"id":"403256","messageId":"CAAbQbbBAjhgGLCJ4C_jxf3Byk+pL3Bh8CjUPaBngM25p38OkuA@mail.gmail.com","threadId":"53911","inReplyTo":"2639321.dTF8K4C05n@alien.lan","subject":"Re: [BUG] git-cherry false-positive on upstream new-line change at end-of-file","fromName":"Tilman Vogel","fromEmail":"tilman.vogel@web.de","sentAt":"2020-08-10T11:01:39Z","receivedAt":"2020-08-10T11:01:55Z","isPatch":false,"sender":{"key":"tilman.vogel@web.de","avatar":"https://avatars.githubusercontent.com/u/349303?v=4"},"body":"Hi!\n\nBecause there was not a single reply to my report, I wonder whether\nanybody read it. May I ping for attention, please?\n\nBest regards,\n\nTilman\n\n\nAm Do., 23. Juli 2020 um 16:42 Uhr schrieb Tilman Vogel <tilman.vogel@web.de>:\n>\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n>\n> What did you do before the bug happened? (Steps to reproduce your issue)\n>\n> I was working on two branches \"master\" and \"feature\". On \"master\" I\n> \"git cherry-pick feature\" in order to pick the latest commit from\n> \"feature\".\n>\n> What did you expect to happen? (Expected behavior)\n>\n> When I run \"git cherry master feature\" the picked commit is\n> marked with \"-\".\n>\n> What happened instead? (Actual behavior)\n>\n> When I run \"git cherry master feature\" the picked commit is\n> marked with \"+\".\n>\n> What's different between what you expected and what actually happened?\n>\n> Because the commit has just been picked, it should be marked as \"-\".\n> The actual behavior claims the commit had not been picked.\n>\n> Anything else you want to add:\n>\n> It is important to note that the only change, I did on master after\n> branching \"feature\", was to add a missing newline at the end of the\n> file that I changed on \"feature\".\n>\n> If I replay the same scenario with a file that has a newline at the end\n> and I am only adding a newline at the beginning of it on master, cherry\n> gives the expected \"-\" after cherry-picking.\n>\n> Please see this simple example:\n>\n> > git clone https://github.com/tvogel/cherry-broken.git\n> > cd cherry-broken\n>\n> # see broken behaviour:\n> > git cherry origin/master origin/feature\n> + 26f77def8f1380cc3fd08ebc3f70652da79f1a2e\n>\n> # check that patch-id is actually the same:\n> > git show --patch origin/master | git patch-id\n> ca5d666a572ec32bff9cec17b97d611316f89236\n> 0725b090dd82f2e6f9949ce853778391e60b08d1\n> > git show --patch origin/feature | git patch-id\n> ca5d666a572ec32bff9cec17b97d611316f89236\n> 26f77def8f1380cc3fd08ebc3f70652da79f1a2e\n>\n> # see OK behaviour:\n> > git cherry origin/master-ok origin/feature-ok\n> - 7ce2c0ecd3024c04bcf7b78d332841b2e791e06f\n>\n> In the \"*-ok\" branches, the file has newline at the end from the beginning\n> and \"master\" only inserts a newline at the beginning of the file. \"feature-ok\"\n> has the same change as \"feature\" (content-wise).\n>\n>\n> Please review the rest of the bug report below.\n> You can delete any lines you don't wish to share.\n>\n>\n> [System Info]\n> git version:\n> git version 2.27.0\n> cpu: x86_64\n> no commit associated with this build\n> sizeof-long: 8\n> sizeof-size_t: 8\n> uname: Linux 5.7.5-1-default #1 SMP Tue Jun 23 06:00:46 UTC 2020 (a1775d0)\n> x86_64\n> compiler info: gnuc: 10.1\n> libc info: glibc: 2.31\n>\n>\n> [Enabled Hooks]\n> not run from a git repository - no hooks to show\n>\n>\n"},{"id":"403976","messageId":"b67eb51d-75e8-62c5-d1c4-fc3015e13fc6@web.de","threadId":"53911","inReplyTo":"2639321.dTF8K4C05n@alien.lan","subject":"[PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-08-18T22:08:54Z","receivedAt":"2020-08-18T22:09:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Whitespace is ignored when calculating patch IDs.  This is done by\nremoving all whitespace from diff lines before hashing them, including\na newline at the end of a file.  If that newline is missing, however,\ndiff reports that fact in a separate line containing \"\\ No newline at\nend of file\\n\", and this marker is hashed like a context line.\n\nThis goes against our goal of making patch IDs independent of\nwhitespace.  Use the same heuristic that 2485eab55cc (git-patch-id: do\nnot trip over \"no newline\" markers, 2011-02-17) added to git patch-id\ninstead and skip diff lines that start with a backslash and a space\nand are longer than twelve characters.\n\nReported-by: Tilman Vogel <tilman.vogel@web.de>\nInitial-test-by: Tilman Vogel <tilman.vogel@web.de>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n diff.c            |  2 ++\n t/t3500-cherry.sh | 23 +++++++++++++++++++++++\n 2 files changed, 25 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex f9709de7b45..f175019eb7a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6044,6 +6044,8 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)\n \tstruct patch_id_t *data = priv;\n \tint new_len;\n\n+\tif (len > 12 && starts_with(line, \"\\\\ \"))\n+\t\treturn;\n \tnew_len = remove_space(line, len);\n\n \tthe_hash_algo->update_fn(data->ctx, line, new_len);\ndiff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh\nindex f038f34b7c0..2b8d9cb38ed 100755\n--- a/t/t3500-cherry.sh\n+++ b/t/t3500-cherry.sh\n@@ -55,4 +55,27 @@ test_expect_success \\\n      expr \"$(echo $(git cherry master my-topic-branch) )\" : \"+ [^ ]* - .*\"\n '\n\n+test_expect_success 'cherry ignores whitespace' '\n+\tgit switch --orphan=upstream-with-space &&\n+\ttest_commit initial file &&\n+\t>expect &&\n+\tgit switch --create=feature-without-space &&\n+\n+\t# A spaceless file on the feature branch.  Expect a match upstream.\n+\tprintf space >file &&\n+\tgit add file &&\n+\tgit commit -m\"file without space\" &&\n+\tgit log --format=\"- %H\" -1 >>expect &&\n+\n+\t# A further change.  Should not match upstream.\n+\ttest_commit change file &&\n+\tgit log --format=\"+ %H\" -1 >>expect &&\n+\n+\tgit switch upstream-with-space &&\n+\t# Same as the spaceless file, just with spaces and on upstream.\n+\ttest_commit \"file with space\" file \"s p a c e\" file-with-space &&\n+\tgit cherry upstream-with-space feature-without-space >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n--\n2.28.0\n"},{"id":"403980","messageId":"xmqqblj7worr.fsf@gitster.c.googlers.com","threadId":"53911","inReplyTo":"b67eb51d-75e8-62c5-d1c4-fc3015e13fc6@web.de","subject":"Re: [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-18T22:52:08Z","receivedAt":"2020-08-18T22:52:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Whitespace is ignored when calculating patch IDs.  This is done by\n> removing all whitespace from diff lines before hashing them, including\n> a newline at the end of a file.  If that newline is missing, however,\n> diff reports that fact in a separate line containing \"\\ No newline at\n> end of file\\n\", and this marker is hashed like a context line.\n\nAh, ouch.\n\n> This goes against our goal of making patch IDs independent of\n> whitespace.  Use the same heuristic that 2485eab55cc (git-patch-id: do\n> not trip over \"no newline\" markers, 2011-02-17) added to git patch-id\n> instead and skip diff lines that start with a backslash and a space\n> and are longer than twelve characters.\n\nGood find of previous example.  Excellent.\n\n> Reported-by: Tilman Vogel <tilman.vogel@web.de>\n> Initial-test-by: Tilman Vogel <tilman.vogel@web.de>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  diff.c            |  2 ++\n>  t/t3500-cherry.sh | 23 +++++++++++++++++++++++\n>  2 files changed, 25 insertions(+)\n\nThanks.\n\n> diff --git a/diff.c b/diff.c\n> index f9709de7b45..f175019eb7a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6044,6 +6044,8 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)\n>  \tstruct patch_id_t *data = priv;\n>  \tint new_len;\n>\n> +\tif (len > 12 && starts_with(line, \"\\\\ \"))\n> +\t\treturn;\n>  \tnew_len = remove_space(line, len);\n>\n>  \tthe_hash_algo->update_fn(data->ctx, line, new_len);\n> diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh\n> index f038f34b7c0..2b8d9cb38ed 100755\n> --- a/t/t3500-cherry.sh\n> +++ b/t/t3500-cherry.sh\n> @@ -55,4 +55,27 @@ test_expect_success \\\n>       expr \"$(echo $(git cherry master my-topic-branch) )\" : \"+ [^ ]* - .*\"\n>  '\n>\n> +test_expect_success 'cherry ignores whitespace' '\n> +\tgit switch --orphan=upstream-with-space &&\n> +\ttest_commit initial file &&\n> +\t>expect &&\n> +\tgit switch --create=feature-without-space &&\n> +\n> +\t# A spaceless file on the feature branch.  Expect a match upstream.\n> +\tprintf space >file &&\n> +\tgit add file &&\n> +\tgit commit -m\"file without space\" &&\n> +\tgit log --format=\"- %H\" -1 >>expect &&\n> +\n> +\t# A further change.  Should not match upstream.\n> +\ttest_commit change file &&\n> +\tgit log --format=\"+ %H\" -1 >>expect &&\n> +\n> +\tgit switch upstream-with-space &&\n> +\t# Same as the spaceless file, just with spaces and on upstream.\n> +\ttest_commit \"file with space\" file \"s p a c e\" file-with-space &&\n> +\tgit cherry upstream-with-space feature-without-space >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> --\n> 2.28.0\n"},{"id":"404313","messageId":"nycvar.QRO.7.76.6.2008241441410.56@tvgsbejvaqbjf.bet","threadId":"53911","inReplyTo":"xmqqblj7worr.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-08-24T12:42:05Z","receivedAt":"2020-08-24T12:42:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 18 Aug 2020, Junio C Hamano wrote:\n\n> René Scharfe <l.s.r@web.de> writes:\n>\n> > Whitespace is ignored when calculating patch IDs.  This is done by\n> > removing all whitespace from diff lines before hashing them, including\n> > a newline at the end of a file.  If that newline is missing, however,\n> > diff reports that fact in a separate line containing \"\\ No newline at\n> > end of file\\n\", and this marker is hashed like a context line.\n>\n> Ah, ouch.\n>\n> > This goes against our goal of making patch IDs independent of\n> > whitespace.  Use the same heuristic that 2485eab55cc (git-patch-id: do\n> > not trip over \"no newline\" markers, 2011-02-17) added to git patch-id\n> > instead and skip diff lines that start with a backslash and a space\n> > and are longer than twelve characters.\n>\n> Good find of previous example.  Excellent.\n\nYup. Looks good to me, too. Thank you!\nDscho\n\n>\n> > Reported-by: Tilman Vogel <tilman.vogel@web.de>\n> > Initial-test-by: Tilman Vogel <tilman.vogel@web.de>\n> > Signed-off-by: René Scharfe <l.s.r@web.de>\n> > ---\n> >  diff.c            |  2 ++\n> >  t/t3500-cherry.sh | 23 +++++++++++++++++++++++\n> >  2 files changed, 25 insertions(+)\n>\n> Thanks.\n>\n> > diff --git a/diff.c b/diff.c\n> > index f9709de7b45..f175019eb7a 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -6044,6 +6044,8 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)\n> >  \tstruct patch_id_t *data = priv;\n> >  \tint new_len;\n> >\n> > +\tif (len > 12 && starts_with(line, \"\\\\ \"))\n> > +\t\treturn;\n> >  \tnew_len = remove_space(line, len);\n> >\n> >  \tthe_hash_algo->update_fn(data->ctx, line, new_len);\n> > diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh\n> > index f038f34b7c0..2b8d9cb38ed 100755\n> > --- a/t/t3500-cherry.sh\n> > +++ b/t/t3500-cherry.sh\n> > @@ -55,4 +55,27 @@ test_expect_success \\\n> >       expr \"$(echo $(git cherry master my-topic-branch) )\" : \"+ [^ ]* - .*\"\n> >  '\n> >\n> > +test_expect_success 'cherry ignores whitespace' '\n> > +\tgit switch --orphan=upstream-with-space &&\n> > +\ttest_commit initial file &&\n> > +\t>expect &&\n> > +\tgit switch --create=feature-without-space &&\n> > +\n> > +\t# A spaceless file on the feature branch.  Expect a match upstream.\n> > +\tprintf space >file &&\n> > +\tgit add file &&\n> > +\tgit commit -m\"file without space\" &&\n> > +\tgit log --format=\"- %H\" -1 >>expect &&\n> > +\n> > +\t# A further change.  Should not match upstream.\n> > +\ttest_commit change file &&\n> > +\tgit log --format=\"+ %H\" -1 >>expect &&\n> > +\n> > +\tgit switch upstream-with-space &&\n> > +\t# Same as the spaceless file, just with spaces and on upstream.\n> > +\ttest_commit \"file with space\" file \"s p a c e\" file-with-space &&\n> > +\tgit cherry upstream-with-space feature-without-space >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> > +\n> >  test_done\n> > --\n> > 2.28.0\n>\n"},{"id":"404596","messageId":"CAAbQbbDM-C3UzQVqm6tdUV+wztKVu-g=gLDKEp_oJQiaXVkbNQ@mail.gmail.com","threadId":"53911","inReplyTo":"b67eb51d-75e8-62c5-d1c4-fc3015e13fc6@web.de","subject":"Re: [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id()","fromName":"Tilman Vogel","fromEmail":"tilman.vogel@web.de","sentAt":"2020-08-27T09:05:26Z","receivedAt":"2020-08-27T09:05:41Z","isPatch":true,"sender":{"key":"tilman.vogel@web.de","avatar":"https://avatars.githubusercontent.com/u/349303?v=4"},"body":"That's great, thanks René! Looking forward to try that out!\n\nTilman\n\nAm Mi., 19. Aug. 2020 um 00:09 Uhr schrieb René Scharfe <l.s.r@web.de>:\n>\n> Whitespace is ignored when calculating patch IDs.  This is done by\n> removing all whitespace from diff lines before hashing them, including\n> a newline at the end of a file.  If that newline is missing, however,\n> diff reports that fact in a separate line containing \"\\ No newline at\n> end of file\\n\", and this marker is hashed like a context line.\n>\n> This goes against our goal of making patch IDs independent of\n> whitespace.  Use the same heuristic that 2485eab55cc (git-patch-id: do\n> not trip over \"no newline\" markers, 2011-02-17) added to git patch-id\n> instead and skip diff lines that start with a backslash and a space\n> and are longer than twelve characters.\n>\n> Reported-by: Tilman Vogel <tilman.vogel@web.de>\n> Initial-test-by: Tilman Vogel <tilman.vogel@web.de>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  diff.c            |  2 ++\n>  t/t3500-cherry.sh | 23 +++++++++++++++++++++++\n>  2 files changed, 25 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index f9709de7b45..f175019eb7a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6044,6 +6044,8 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)\n>         struct patch_id_t *data = priv;\n>         int new_len;\n>\n> +       if (len > 12 && starts_with(line, \"\\\\ \"))\n> +               return;\n>         new_len = remove_space(line, len);\n>\n>         the_hash_algo->update_fn(data->ctx, line, new_len);\n> diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh\n> index f038f34b7c0..2b8d9cb38ed 100755\n> --- a/t/t3500-cherry.sh\n> +++ b/t/t3500-cherry.sh\n> @@ -55,4 +55,27 @@ test_expect_success \\\n>       expr \"$(echo $(git cherry master my-topic-branch) )\" : \"+ [^ ]* - .*\"\n>  '\n>\n> +test_expect_success 'cherry ignores whitespace' '\n> +       git switch --orphan=upstream-with-space &&\n> +       test_commit initial file &&\n> +       >expect &&\n> +       git switch --create=feature-without-space &&\n> +\n> +       # A spaceless file on the feature branch.  Expect a match upstream.\n> +       printf space >file &&\n> +       git add file &&\n> +       git commit -m\"file without space\" &&\n> +       git log --format=\"- %H\" -1 >>expect &&\n> +\n> +       # A further change.  Should not match upstream.\n> +       test_commit change file &&\n> +       git log --format=\"+ %H\" -1 >>expect &&\n> +\n> +       git switch upstream-with-space &&\n> +       # Same as the spaceless file, just with spaces and on upstream.\n> +       test_commit \"file with space\" file \"s p a c e\" file-with-space &&\n> +       git cherry upstream-with-space feature-without-space >actual &&\n> +       test_cmp expect actual\n> +'\n> +\n>  test_done\n> --\n> 2.28.0\n"}]}