* [BUG] git-cherry false-positive on upstream new-line change at end-of-file
@ 2020-07-23 14:42 Tilman Vogel
2020-08-10 11:01 ` Tilman Vogel
2020-08-18 22:08 ` [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id() René Scharfe
0 siblings, 2 replies; 6+ messages in thread
From: Tilman Vogel @ 2020-07-23 14:42 UTC (permalink / raw)
To: git
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
I was working on two branches "master" and "feature". On "master" I
"git cherry-pick feature" in order to pick the latest commit from
"feature".
What did you expect to happen? (Expected behavior)
When I run "git cherry master feature" the picked commit is
marked with "-".
What happened instead? (Actual behavior)
When I run "git cherry master feature" the picked commit is
marked with "+".
What's different between what you expected and what actually happened?
Because the commit has just been picked, it should be marked as "-".
The actual behavior claims the commit had not been picked.
Anything else you want to add:
It is important to note that the only change, I did on master after
branching "feature", was to add a missing newline at the end of the
file that I changed on "feature".
If I replay the same scenario with a file that has a newline at the end
and I am only adding a newline at the beginning of it on master, cherry
gives the expected "-" after cherry-picking.
Please see this simple example:
> git clone https://github.com/tvogel/cherry-broken.git
> cd cherry-broken
# see broken behaviour:
> git cherry origin/master origin/feature
+ 26f77def8f1380cc3fd08ebc3f70652da79f1a2e
# check that patch-id is actually the same:
> git show --patch origin/master | git patch-id
ca5d666a572ec32bff9cec17b97d611316f89236
0725b090dd82f2e6f9949ce853778391e60b08d1
> git show --patch origin/feature | git patch-id
ca5d666a572ec32bff9cec17b97d611316f89236
26f77def8f1380cc3fd08ebc3f70652da79f1a2e
# see OK behaviour:
> git cherry origin/master-ok origin/feature-ok
- 7ce2c0ecd3024c04bcf7b78d332841b2e791e06f
In the "*-ok" branches, the file has newline at the end from the beginning
and "master" only inserts a newline at the beginning of the file. "feature-ok"
has the same change as "feature" (content-wise).
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.27.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
uname: Linux 5.7.5-1-default #1 SMP Tue Jun 23 06:00:46 UTC 2020 (a1775d0)
x86_64
compiler info: gnuc: 10.1
libc info: glibc: 2.31
[Enabled Hooks]
not run from a git repository - no hooks to show
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] git-cherry false-positive on upstream new-line change at end-of-file
2020-07-23 14:42 [BUG] git-cherry false-positive on upstream new-line change at end-of-file Tilman Vogel
@ 2020-08-10 11:01 ` Tilman Vogel
2020-08-18 22:08 ` [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id() René Scharfe
1 sibling, 0 replies; 6+ messages in thread
From: Tilman Vogel @ 2020-08-10 11:01 UTC (permalink / raw)
To: git
Hi!
Because there was not a single reply to my report, I wonder whether
anybody read it. May I ping for attention, please?
Best regards,
Tilman
Am Do., 23. Juli 2020 um 16:42 Uhr schrieb Tilman Vogel <tilman.vogel@web.de>:
>
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
>
> I was working on two branches "master" and "feature". On "master" I
> "git cherry-pick feature" in order to pick the latest commit from
> "feature".
>
> What did you expect to happen? (Expected behavior)
>
> When I run "git cherry master feature" the picked commit is
> marked with "-".
>
> What happened instead? (Actual behavior)
>
> When I run "git cherry master feature" the picked commit is
> marked with "+".
>
> What's different between what you expected and what actually happened?
>
> Because the commit has just been picked, it should be marked as "-".
> The actual behavior claims the commit had not been picked.
>
> Anything else you want to add:
>
> It is important to note that the only change, I did on master after
> branching "feature", was to add a missing newline at the end of the
> file that I changed on "feature".
>
> If I replay the same scenario with a file that has a newline at the end
> and I am only adding a newline at the beginning of it on master, cherry
> gives the expected "-" after cherry-picking.
>
> Please see this simple example:
>
> > git clone https://github.com/tvogel/cherry-broken.git
> > cd cherry-broken
>
> # see broken behaviour:
> > git cherry origin/master origin/feature
> + 26f77def8f1380cc3fd08ebc3f70652da79f1a2e
>
> # check that patch-id is actually the same:
> > git show --patch origin/master | git patch-id
> ca5d666a572ec32bff9cec17b97d611316f89236
> 0725b090dd82f2e6f9949ce853778391e60b08d1
> > git show --patch origin/feature | git patch-id
> ca5d666a572ec32bff9cec17b97d611316f89236
> 26f77def8f1380cc3fd08ebc3f70652da79f1a2e
>
> # see OK behaviour:
> > git cherry origin/master-ok origin/feature-ok
> - 7ce2c0ecd3024c04bcf7b78d332841b2e791e06f
>
> In the "*-ok" branches, the file has newline at the end from the beginning
> and "master" only inserts a newline at the beginning of the file. "feature-ok"
> has the same change as "feature" (content-wise).
>
>
> Please review the rest of the bug report below.
> You can delete any lines you don't wish to share.
>
>
> [System Info]
> git version:
> git version 2.27.0
> cpu: x86_64
> no commit associated with this build
> sizeof-long: 8
> sizeof-size_t: 8
> uname: Linux 5.7.5-1-default #1 SMP Tue Jun 23 06:00:46 UTC 2020 (a1775d0)
> x86_64
> compiler info: gnuc: 10.1
> libc info: glibc: 2.31
>
>
> [Enabled Hooks]
> not run from a git repository - no hooks to show
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id()
2020-07-23 14:42 [BUG] git-cherry false-positive on upstream new-line change at end-of-file Tilman Vogel
2020-08-10 11:01 ` Tilman Vogel
@ 2020-08-18 22:08 ` René Scharfe
2020-08-18 22:52 ` Junio C Hamano
2020-08-27 9:05 ` Tilman Vogel
1 sibling, 2 replies; 6+ messages in thread
From: René Scharfe @ 2020-08-18 22:08 UTC (permalink / raw)
To: Tilman Vogel, git; +Cc: Junio C Hamano, Johannes Schindelin
Whitespace is ignored when calculating patch IDs. This is done by
removing all whitespace from diff lines before hashing them, including
a newline at the end of a file. If that newline is missing, however,
diff reports that fact in a separate line containing "\ No newline at
end of file\n", and this marker is hashed like a context line.
This goes against our goal of making patch IDs independent of
whitespace. Use the same heuristic that 2485eab55cc (git-patch-id: do
not trip over "no newline" markers, 2011-02-17) added to git patch-id
instead and skip diff lines that start with a backslash and a space
and are longer than twelve characters.
Reported-by: Tilman Vogel <tilman.vogel@web.de>
Initial-test-by: Tilman Vogel <tilman.vogel@web.de>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
diff.c | 2 ++
t/t3500-cherry.sh | 23 +++++++++++++++++++++++
2 files changed, 25 insertions(+)
diff --git a/diff.c b/diff.c
index f9709de7b45..f175019eb7a 100644
--- a/diff.c
+++ b/diff.c
@@ -6044,6 +6044,8 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)
struct patch_id_t *data = priv;
int new_len;
+ if (len > 12 && starts_with(line, "\\ "))
+ return;
new_len = remove_space(line, len);
the_hash_algo->update_fn(data->ctx, line, new_len);
diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh
index f038f34b7c0..2b8d9cb38ed 100755
--- a/t/t3500-cherry.sh
+++ b/t/t3500-cherry.sh
@@ -55,4 +55,27 @@ test_expect_success \
expr "$(echo $(git cherry master my-topic-branch) )" : "+ [^ ]* - .*"
'
+test_expect_success 'cherry ignores whitespace' '
+ git switch --orphan=upstream-with-space &&
+ test_commit initial file &&
+ >expect &&
+ git switch --create=feature-without-space &&
+
+ # A spaceless file on the feature branch. Expect a match upstream.
+ printf space >file &&
+ git add file &&
+ git commit -m"file without space" &&
+ git log --format="- %H" -1 >>expect &&
+
+ # A further change. Should not match upstream.
+ test_commit change file &&
+ git log --format="+ %H" -1 >>expect &&
+
+ git switch upstream-with-space &&
+ # Same as the spaceless file, just with spaces and on upstream.
+ test_commit "file with space" file "s p a c e" file-with-space &&
+ git cherry upstream-with-space feature-without-space >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id()
2020-08-18 22:08 ` [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id() René Scharfe
@ 2020-08-18 22:52 ` Junio C Hamano
2020-08-24 12:42 ` Johannes Schindelin
2020-08-27 9:05 ` Tilman Vogel
1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2020-08-18 22:52 UTC (permalink / raw)
To: René Scharfe; +Cc: Tilman Vogel, git, Johannes Schindelin
René Scharfe <l.s.r@web.de> writes:
> Whitespace is ignored when calculating patch IDs. This is done by
> removing all whitespace from diff lines before hashing them, including
> a newline at the end of a file. If that newline is missing, however,
> diff reports that fact in a separate line containing "\ No newline at
> end of file\n", and this marker is hashed like a context line.
Ah, ouch.
> This goes against our goal of making patch IDs independent of
> whitespace. Use the same heuristic that 2485eab55cc (git-patch-id: do
> not trip over "no newline" markers, 2011-02-17) added to git patch-id
> instead and skip diff lines that start with a backslash and a space
> and are longer than twelve characters.
Good find of previous example. Excellent.
> Reported-by: Tilman Vogel <tilman.vogel@web.de>
> Initial-test-by: Tilman Vogel <tilman.vogel@web.de>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> diff.c | 2 ++
> t/t3500-cherry.sh | 23 +++++++++++++++++++++++
> 2 files changed, 25 insertions(+)
Thanks.
> diff --git a/diff.c b/diff.c
> index f9709de7b45..f175019eb7a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6044,6 +6044,8 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)
> struct patch_id_t *data = priv;
> int new_len;
>
> + if (len > 12 && starts_with(line, "\\ "))
> + return;
> new_len = remove_space(line, len);
>
> the_hash_algo->update_fn(data->ctx, line, new_len);
> diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh
> index f038f34b7c0..2b8d9cb38ed 100755
> --- a/t/t3500-cherry.sh
> +++ b/t/t3500-cherry.sh
> @@ -55,4 +55,27 @@ test_expect_success \
> expr "$(echo $(git cherry master my-topic-branch) )" : "+ [^ ]* - .*"
> '
>
> +test_expect_success 'cherry ignores whitespace' '
> + git switch --orphan=upstream-with-space &&
> + test_commit initial file &&
> + >expect &&
> + git switch --create=feature-without-space &&
> +
> + # A spaceless file on the feature branch. Expect a match upstream.
> + printf space >file &&
> + git add file &&
> + git commit -m"file without space" &&
> + git log --format="- %H" -1 >>expect &&
> +
> + # A further change. Should not match upstream.
> + test_commit change file &&
> + git log --format="+ %H" -1 >>expect &&
> +
> + git switch upstream-with-space &&
> + # Same as the spaceless file, just with spaces and on upstream.
> + test_commit "file with space" file "s p a c e" file-with-space &&
> + git cherry upstream-with-space feature-without-space >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
> --
> 2.28.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id()
2020-08-18 22:52 ` Junio C Hamano
@ 2020-08-24 12:42 ` Johannes Schindelin
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Schindelin @ 2020-08-24 12:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Tilman Vogel, git
[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]
Hi,
On Tue, 18 Aug 2020, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
> > Whitespace is ignored when calculating patch IDs. This is done by
> > removing all whitespace from diff lines before hashing them, including
> > a newline at the end of a file. If that newline is missing, however,
> > diff reports that fact in a separate line containing "\ No newline at
> > end of file\n", and this marker is hashed like a context line.
>
> Ah, ouch.
>
> > This goes against our goal of making patch IDs independent of
> > whitespace. Use the same heuristic that 2485eab55cc (git-patch-id: do
> > not trip over "no newline" markers, 2011-02-17) added to git patch-id
> > instead and skip diff lines that start with a backslash and a space
> > and are longer than twelve characters.
>
> Good find of previous example. Excellent.
Yup. Looks good to me, too. Thank you!
Dscho
>
> > Reported-by: Tilman Vogel <tilman.vogel@web.de>
> > Initial-test-by: Tilman Vogel <tilman.vogel@web.de>
> > Signed-off-by: René Scharfe <l.s.r@web.de>
> > ---
> > diff.c | 2 ++
> > t/t3500-cherry.sh | 23 +++++++++++++++++++++++
> > 2 files changed, 25 insertions(+)
>
> Thanks.
>
> > diff --git a/diff.c b/diff.c
> > index f9709de7b45..f175019eb7a 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -6044,6 +6044,8 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)
> > struct patch_id_t *data = priv;
> > int new_len;
> >
> > + if (len > 12 && starts_with(line, "\\ "))
> > + return;
> > new_len = remove_space(line, len);
> >
> > the_hash_algo->update_fn(data->ctx, line, new_len);
> > diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh
> > index f038f34b7c0..2b8d9cb38ed 100755
> > --- a/t/t3500-cherry.sh
> > +++ b/t/t3500-cherry.sh
> > @@ -55,4 +55,27 @@ test_expect_success \
> > expr "$(echo $(git cherry master my-topic-branch) )" : "+ [^ ]* - .*"
> > '
> >
> > +test_expect_success 'cherry ignores whitespace' '
> > + git switch --orphan=upstream-with-space &&
> > + test_commit initial file &&
> > + >expect &&
> > + git switch --create=feature-without-space &&
> > +
> > + # A spaceless file on the feature branch. Expect a match upstream.
> > + printf space >file &&
> > + git add file &&
> > + git commit -m"file without space" &&
> > + git log --format="- %H" -1 >>expect &&
> > +
> > + # A further change. Should not match upstream.
> > + test_commit change file &&
> > + git log --format="+ %H" -1 >>expect &&
> > +
> > + git switch upstream-with-space &&
> > + # Same as the spaceless file, just with spaces and on upstream.
> > + test_commit "file with space" file "s p a c e" file-with-space &&
> > + git cherry upstream-with-space feature-without-space >actual &&
> > + test_cmp expect actual
> > +'
> > +
> > test_done
> > --
> > 2.28.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id()
2020-08-18 22:08 ` [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id() René Scharfe
2020-08-18 22:52 ` Junio C Hamano
@ 2020-08-27 9:05 ` Tilman Vogel
1 sibling, 0 replies; 6+ messages in thread
From: Tilman Vogel @ 2020-08-27 9:05 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano, Johannes Schindelin
That's great, thanks René! Looking forward to try that out!
Tilman
Am Mi., 19. Aug. 2020 um 00:09 Uhr schrieb René Scharfe <l.s.r@web.de>:
>
> Whitespace is ignored when calculating patch IDs. This is done by
> removing all whitespace from diff lines before hashing them, including
> a newline at the end of a file. If that newline is missing, however,
> diff reports that fact in a separate line containing "\ No newline at
> end of file\n", and this marker is hashed like a context line.
>
> This goes against our goal of making patch IDs independent of
> whitespace. Use the same heuristic that 2485eab55cc (git-patch-id: do
> not trip over "no newline" markers, 2011-02-17) added to git patch-id
> instead and skip diff lines that start with a backslash and a space
> and are longer than twelve characters.
>
> Reported-by: Tilman Vogel <tilman.vogel@web.de>
> Initial-test-by: Tilman Vogel <tilman.vogel@web.de>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> diff.c | 2 ++
> t/t3500-cherry.sh | 23 +++++++++++++++++++++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/diff.c b/diff.c
> index f9709de7b45..f175019eb7a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6044,6 +6044,8 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)
> struct patch_id_t *data = priv;
> int new_len;
>
> + if (len > 12 && starts_with(line, "\\ "))
> + return;
> new_len = remove_space(line, len);
>
> the_hash_algo->update_fn(data->ctx, line, new_len);
> diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh
> index f038f34b7c0..2b8d9cb38ed 100755
> --- a/t/t3500-cherry.sh
> +++ b/t/t3500-cherry.sh
> @@ -55,4 +55,27 @@ test_expect_success \
> expr "$(echo $(git cherry master my-topic-branch) )" : "+ [^ ]* - .*"
> '
>
> +test_expect_success 'cherry ignores whitespace' '
> + git switch --orphan=upstream-with-space &&
> + test_commit initial file &&
> + >expect &&
> + git switch --create=feature-without-space &&
> +
> + # A spaceless file on the feature branch. Expect a match upstream.
> + printf space >file &&
> + git add file &&
> + git commit -m"file without space" &&
> + git log --format="- %H" -1 >>expect &&
> +
> + # A further change. Should not match upstream.
> + test_commit change file &&
> + git log --format="+ %H" -1 >>expect &&
> +
> + git switch upstream-with-space &&
> + # Same as the spaceless file, just with spaces and on upstream.
> + test_commit "file with space" file "s p a c e" file-with-space &&
> + git cherry upstream-with-space feature-without-space >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
> --
> 2.28.0
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-27 9:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-23 14:42 [BUG] git-cherry false-positive on upstream new-line change at end-of-file Tilman Vogel
2020-08-10 11:01 ` Tilman Vogel
2020-08-18 22:08 ` [PATCH] patch-id: ignore newline at end of file in diff_flush_patch_id() René Scharfe
2020-08-18 22:52 ` Junio C Hamano
2020-08-24 12:42 ` Johannes Schindelin
2020-08-27 9:05 ` Tilman Vogel
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).