git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* Why does git-patch-id(1) sometimes print two lines, one of which has commit = 0000000000000000000000000000000000000000
@ 2011-02-16 14:56 Ævar Arnfjörð Bjarmason
  2011-02-16 16:11 ` Michael J Gruber
  0 siblings, 1 reply; 18+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2011-02-16 14:56 UTC (permalink / raw)
  To: Git Mailing List

This behaves as I'd expect on git.git, i.e. shows a patch id and the commit id:

    $ git show 24231e063f0f003f8ffd7b64c7ba6a0baaaa5283 | git patch-id
    f10c69e0e5b33da206f37bd93639875555ac9b79
24231e063f0f003f8ffd7b64c7ba6a0baaaa5283

But what does this mean, also on git.git:

    $ git show 7d48e9e6f77d336376c1a554eeff0590f77e1ee1 | git patch-id
    4ba8a248731c5fcbd09cacb248d3128e742d1c90
7d48e9e6f77d336376c1a554eeff0590f77e1ee1
    d019b35e0b859cdd6907ee170927de1124c0ed6e
0000000000000000000000000000000000000000

7d48e9e6f77d336376c1a554eeff0590f77e1ee1 is just one of the commits
that results in this output:

    $ git log --pretty=format:%H -p | git patch-id | grep
0000000000000000000000000000000000000000
    d019b35e0b859cdd6907ee170927de1124c0ed6e
0000000000000000000000000000000000000000
    3b23a2a11055aef557369971e825010879a8c4d7
0000000000000000000000000000000000000000
    d498fbbad6f1374d952925df699da237c3e8f2df
0000000000000000000000000000000000000000
    b0c930dc1926ffae9cca022797856762fa908be6
0000000000000000000000000000000000000000

And on another repository where I'm dealing with this I have a bunch
more of them.

Why are they there and what do they mean? Maybe it's a bug, or
something I can explain in the manual page.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Why does git-patch-id(1) sometimes print two lines, one of which has commit = 0000000000000000000000000000000000000000
  2011-02-16 14:56 Why does git-patch-id(1) sometimes print two lines, one of which has commit = 0000000000000000000000000000000000000000 Ævar Arnfjörð Bjarmason
@ 2011-02-16 16:11 ` Michael J Gruber
  2011-02-16 16:55   ` [PATCH 1/2] git-patch-id: test for "no newline" markers Michael J Gruber
  0 siblings, 1 reply; 18+ messages in thread
From: Michael J Gruber @ 2011-02-16 16:11 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List

Ævar Arnfjörð Bjarmason venit, vidit, dixit 16.02.2011 15:56:
> This behaves as I'd expect on git.git, i.e. shows a patch id and the commit id:
> 
>     $ git show 24231e063f0f003f8ffd7b64c7ba6a0baaaa5283 | git patch-id
>     f10c69e0e5b33da206f37bd93639875555ac9b79
> 24231e063f0f003f8ffd7b64c7ba6a0baaaa5283
> 
> But what does this mean, also on git.git:
> 
>     $ git show 7d48e9e6f77d336376c1a554eeff0590f77e1ee1 | git patch-id
>     4ba8a248731c5fcbd09cacb248d3128e742d1c90
> 7d48e9e6f77d336376c1a554eeff0590f77e1ee1
>     d019b35e0b859cdd6907ee170927de1124c0ed6e
> 0000000000000000000000000000000000000000
> 
> 7d48e9e6f77d336376c1a554eeff0590f77e1ee1 is just one of the commits
> that results in this output:
> 
>     $ git log --pretty=format:%H -p | git patch-id | grep
> 0000000000000000000000000000000000000000
>     d019b35e0b859cdd6907ee170927de1124c0ed6e
> 0000000000000000000000000000000000000000
>     3b23a2a11055aef557369971e825010879a8c4d7
> 0000000000000000000000000000000000000000
>     d498fbbad6f1374d952925df699da237c3e8f2df
> 0000000000000000000000000000000000000000
>     b0c930dc1926ffae9cca022797856762fa908be6
> 0000000000000000000000000000000000000000

You're telling us that patch-ids, not the sha1's of affected commits here.

> 
> And on another repository where I'm dealing with this I have a bunch
> more of them.
> 
> Why are they there and what do they mean? Maybe it's a bug, or
> something I can explain in the manual page.

You would have to explain that git-patch-id trips over "\ Now
newline..." lines in our diffs. Alternatively, wait a few minutes for my
patch (done) with a test (the boring part...).

Cheers,
Michael

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/2] git-patch-id: test for "no newline" markers
  2011-02-16 16:11 ` Michael J Gruber
@ 2011-02-16 16:55   ` Michael J Gruber
  2011-02-16 16:55     ` [PATCH 2/2] git-patch-id: do not trip over " Michael J Gruber
  0 siblings, 1 reply; 18+ messages in thread
From: Michael J Gruber @ 2011-02-16 16:55 UTC (permalink / raw)
  To: git; +Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano

Currently, patch-id trips over our very own diff extension for marking
the absence of newline at EOF.

Expose this in a test.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t4204-patch-id.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index 68e2652..db96064 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -63,4 +63,40 @@ test_expect_success 'patch-id supports git-format-patch MIME output' '
 	test_cmp patch-id_master patch-id_same
 '
 
+cat >nonl <<\EOF
+diff --git i/a w/a
+index e69de29..2e65efe 100644
+--- i/a
++++ w/a
+@@ -0,0 +1 @@
++a
+\ No newline at end of file
+diff --git i/b w/b
+index e69de29..6178079 100644
+--- i/b
++++ w/b
+@@ -0,0 +1 @@
++b
+EOF
+
+cat >withnl <<\EOF
+diff --git i/a w/a
+index e69de29..7898192 100644
+--- i/a
++++ w/a
+@@ -0,0 +1 @@
++a
+diff --git i/b w/b
+index e69de29..6178079 100644
+--- i/b
++++ w/b
+@@ -0,0 +1 @@
++b
+EOF
+
+test_expect_failure 'patch-id handles no-nl-at-eof markers' '
+	cat nonl | calc_patch_id nonl &&
+	cat withnl | calc_patch_id withnl &&
+	test_cmp patch-id_nonl patch-id_withnl
+'
 test_done
-- 
1.7.4.1.74.gf39475.dirty

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/2] git-patch-id: do not trip over "no newline" markers
  2011-02-16 16:55   ` [PATCH 1/2] git-patch-id: test for "no newline" markers Michael J Gruber
@ 2011-02-16 16:55     ` Michael J Gruber
  2011-02-16 20:05       ` Junio C Hamano
  0 siblings, 1 reply; 18+ messages in thread
From: Michael J Gruber @ 2011-02-16 16:55 UTC (permalink / raw)
  To: git; +Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano

Currently, patch-id trips over our very own diff extension for marking
the absence of newline at EOF.

Fix it. (Ignore it, it's whitespace.)

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/patch-id.c  |    2 ++
 t/t4204-patch-id.sh |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 5125300..653d958 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -73,6 +73,8 @@ int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)
 			p += 7;
 		else if (!memcmp(line, "From ", 5))
 			p += 5;
+		else if (!memcmp(line, "\\ No newline at end of file", 27))
+			continue;
 
 		if (!get_sha1_hex(p, next_sha1)) {
 			found_next = 1;
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index db96064..d2c930d 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -94,7 +94,7 @@ index e69de29..6178079 100644
 +b
 EOF
 
-test_expect_failure 'patch-id handles no-nl-at-eof markers' '
+test_expect_success 'patch-id handles no-nl-at-eof markers' '
 	cat nonl | calc_patch_id nonl &&
 	cat withnl | calc_patch_id withnl &&
 	test_cmp patch-id_nonl patch-id_withnl
-- 
1.7.4.1.74.gf39475.dirty

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] git-patch-id: do not trip over "no newline" markers
  2011-02-16 16:55     ` [PATCH 2/2] git-patch-id: do not trip over " Michael J Gruber
@ 2011-02-16 20:05       ` Junio C Hamano
  2011-02-17  7:44         ` [PATCHv2 1/2] git-patch-id: test for " Michael J Gruber
  0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2011-02-16 20:05 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Ævar Arnfjörð Bjarmason

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Currently, patch-id trips over our very own diff extension for marking
> the absence of newline at EOF.
>
> Fix it. (Ignore it, it's whitespace.)
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  builtin/patch-id.c  |    2 ++
>  t/t4204-patch-id.sh |    2 +-
>  2 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/patch-id.c b/builtin/patch-id.c
> index 5125300..653d958 100644
> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -73,6 +73,8 @@ int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)
>  			p += 7;
>  		else if (!memcmp(line, "From ", 5))
>  			p += 5;
> +		else if (!memcmp(line, "\\ No newline at end of file", 27))
> +			continue;

Good spotting, except that this string is a bit too specific to git and
GNU diff run in the C or ??_en locales.  An early draft of "git apply"
used to have the same string but the string was removed before the command
was released to the wild with a lot weaker condition (grep for "l10n" in
builtin/apply.c).

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCHv2 1/2] git-patch-id: test for "no newline" markers
  2011-02-16 20:05       ` Junio C Hamano
@ 2011-02-17  7:44         ` Michael J Gruber
  2011-02-17  7:44           ` [PATCHv2 2/2] git-patch-id: do not trip over " Michael J Gruber
  2011-02-17 11:55           ` [PATCHv2 1/2] git-patch-id: test for " Jakub Narebski
  0 siblings, 2 replies; 18+ messages in thread
From: Michael J Gruber @ 2011-02-17  7:44 UTC (permalink / raw)
  To: git; +Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano

Currently, patch-id trips over our very own diff extension for marking
the absence of newline at EOF.

Expose this in a test.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t4204-patch-id.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index 68e2652..db96064 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -63,4 +63,40 @@ test_expect_success 'patch-id supports git-format-patch MIME output' '
 	test_cmp patch-id_master patch-id_same
 '
 
+cat >nonl <<\EOF
+diff --git i/a w/a
+index e69de29..2e65efe 100644
+--- i/a
++++ w/a
+@@ -0,0 +1 @@
++a
+\ No newline at end of file
+diff --git i/b w/b
+index e69de29..6178079 100644
+--- i/b
++++ w/b
+@@ -0,0 +1 @@
++b
+EOF
+
+cat >withnl <<\EOF
+diff --git i/a w/a
+index e69de29..7898192 100644
+--- i/a
++++ w/a
+@@ -0,0 +1 @@
++a
+diff --git i/b w/b
+index e69de29..6178079 100644
+--- i/b
++++ w/b
+@@ -0,0 +1 @@
++b
+EOF
+
+test_expect_failure 'patch-id handles no-nl-at-eof markers' '
+	cat nonl | calc_patch_id nonl &&
+	cat withnl | calc_patch_id withnl &&
+	test_cmp patch-id_nonl patch-id_withnl
+'
 test_done
-- 
1.7.4.1.74.gf39475.dirty

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCHv2 2/2] git-patch-id: do not trip over "no newline" markers
  2011-02-17  7:44         ` [PATCHv2 1/2] git-patch-id: test for " Michael J Gruber
@ 2011-02-17  7:44           ` Michael J Gruber
  2011-02-18  4:16             ` Jeff King
  2011-02-17 11:55           ` [PATCHv2 1/2] git-patch-id: test for " Jakub Narebski
  1 sibling, 1 reply; 18+ messages in thread
From: Michael J Gruber @ 2011-02-17  7:44 UTC (permalink / raw)
  To: git; +Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano

Currently, patch-id trips over our very own diff extension for marking
the absence of newline at EOF.

Fix it. (Ignore it, it's whitespace.)

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
v2 has the more fuzzy marker detection from "apply" as suggested by Junio.

 builtin/patch-id.c  |    2 ++
 t/t4204-patch-id.sh |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 5125300..e1c3cb9 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -73,6 +73,8 @@ int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)
 			p += 7;
 		else if (!memcmp(line, "From ", 5))
 			p += 5;
+		else if (!memcmp(line, "\\ ", 2) && strlen(line)>=12)
+			continue;
 
 		if (!get_sha1_hex(p, next_sha1)) {
 			found_next = 1;
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index db96064..d2c930d 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -94,7 +94,7 @@ index e69de29..6178079 100644
 +b
 EOF
 
-test_expect_failure 'patch-id handles no-nl-at-eof markers' '
+test_expect_success 'patch-id handles no-nl-at-eof markers' '
 	cat nonl | calc_patch_id nonl &&
 	cat withnl | calc_patch_id withnl &&
 	test_cmp patch-id_nonl patch-id_withnl
-- 
1.7.4.1.74.gf39475.dirty

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 1/2] git-patch-id: test for "no newline" markers
  2011-02-17  7:44         ` [PATCHv2 1/2] git-patch-id: test for " Michael J Gruber
  2011-02-17  7:44           ` [PATCHv2 2/2] git-patch-id: do not trip over " Michael J Gruber
@ 2011-02-17 11:55           ` Jakub Narebski
  2011-02-17 12:11             ` Michael J Gruber
  1 sibling, 1 reply; 18+ messages in thread
From: Jakub Narebski @ 2011-02-17 11:55 UTC (permalink / raw)
  To: Michael J Gruber
  Cc: git, Ævar Arnfjörð Bjarmason, Junio C Hamano

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Currently, patch-id trips over our very own diff extension for marking
> the absence of newline at EOF.

This is not *our* diff extension; it is either GNU diff extension, or
it is defined in diff/patch format standard.

diff.info states in chapter "Incomplete Lines":

     When an input file ends in a non-newline character, its last line is
  called an "incomplete line" because its last character is not a
  newline.  All other lines are called "full lines" and end in a newline
  character.  Incomplete lines do not match full lines unless differences
  in white space are ignored (*note White Space::).

     An incomplete line is normally distinguished on output from a full
  line by a following line that starts with `\'. [...]

     For example, suppose `F' and `G' are one-byte files that contain
  just `f' and `g', respectively.  Then `diff F G' outputs

       1c1
       < f
       \ No newline at end of file
       ---
       > g
       \ No newline at end of file

  (The exact message may differ in non-English locales.)

-- 
Jakub Narebski
Poland
ShadeHawk on #git

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 1/2] git-patch-id: test for "no newline" markers
  2011-02-17 11:55           ` [PATCHv2 1/2] git-patch-id: test for " Jakub Narebski
@ 2011-02-17 12:11             ` Michael J Gruber
  0 siblings, 0 replies; 18+ messages in thread
From: Michael J Gruber @ 2011-02-17 12:11 UTC (permalink / raw)
  To: Jakub Narebski
  Cc: git, Ævar Arnfjörð Bjarmason, Junio C Hamano

Jakub Narebski venit, vidit, dixit 17.02.2011 12:55:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Currently, patch-id trips over our very own diff extension for marking
>> the absence of newline at EOF.
> 
> This is not *our* diff extension; it is either GNU diff extension, or
> it is defined in diff/patch format standard.

So, which one is it?

The point is that our very own code produces that extension, so we'd
better support what our "diff" and "log -p" produce.

Michael

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 2/2] git-patch-id: do not trip over "no newline" markers
  2011-02-17  7:44           ` [PATCHv2 2/2] git-patch-id: do not trip over " Michael J Gruber
@ 2011-02-18  4:16             ` Jeff King
  2011-02-18  8:02               ` Michael J Gruber
  0 siblings, 1 reply; 18+ messages in thread
From: Jeff King @ 2011-02-18  4:16 UTC (permalink / raw)
  To: Michael J Gruber
  Cc: git, Ævar Arnfjörð Bjarmason, Junio C Hamano

On Thu, Feb 17, 2011 at 08:44:42AM +0100, Michael J Gruber wrote:

> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -73,6 +73,8 @@ int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)
>  			p += 7;
>  		else if (!memcmp(line, "From ", 5))
>  			p += 5;
> +		else if (!memcmp(line, "\\ ", 2) && strlen(line)>=12)
> +			continue;

Wow, that's pretty obscure. I wonder if the test should be factored out
into line_is_no_newline_at_end_of_file() (or surely there is some more
sensible name), and used by both apply and patch-id. Along with a nice
comment (which I see apply already has) describing what in the world the
magic number 12 means.

-Peff

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 2/2] git-patch-id: do not trip over "no newline" markers
  2011-02-18  4:16             ` Jeff King
@ 2011-02-18  8:02               ` Michael J Gruber
  2011-02-18 10:12                 ` [PATCHv3 1/2] git-patch-id: test for " Michael J Gruber
  2011-02-18 10:40                 ` [PATCHv2 " Jeff King
  0 siblings, 2 replies; 18+ messages in thread
From: Michael J Gruber @ 2011-02-18  8:02 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Ævar Arnfjörð Bjarmason, Junio C Hamano

Jeff King venit, vidit, dixit 18.02.2011 05:16:
> On Thu, Feb 17, 2011 at 08:44:42AM +0100, Michael J Gruber wrote:
> 
>> --- a/builtin/patch-id.c
>> +++ b/builtin/patch-id.c
>> @@ -73,6 +73,8 @@ int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)
>>  			p += 7;
>>  		else if (!memcmp(line, "From ", 5))
>>  			p += 5;
>> +		else if (!memcmp(line, "\\ ", 2) && strlen(line)>=12)
>> +			continue;
> 
> Wow, that's pretty obscure. I wonder if the test should be factored out
> into line_is_no_newline_at_end_of_file() (or surely there is some more
> sensible name), and used by both apply and patch-id. Along with a nice
> comment (which I see apply already has) describing what in the world the
> magic number 12 means.

So, where do you suggest it should go? As far as I can see, we have two
places reading these markers (the above builtins) and two places writing
them (diff.c, xdiff/xutils.c), at least for git-core. (git-gui's
diff.tcl has its own strict check etc.)

I would opt for putting a more detailed explanation in the commit
message (reasoning + ref. to apply.c) rather then trying to factor this.

After all, our code is largely undocumented as far as inline
documentation goes (it has frustrated me a few times), and the more
comprehensive documentation are the commit messages which git blame
points you to.

Michael

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCHv3 1/2] git-patch-id: test for "no newline" markers
  2011-02-18  8:02               ` Michael J Gruber
@ 2011-02-18 10:12                 ` Michael J Gruber
  2011-02-18 10:12                   ` [PATCHv3 2/2] git-patch-id: do not trip over " Michael J Gruber
  2011-02-18 10:40                 ` [PATCHv2 " Jeff King
  1 sibling, 1 reply; 18+ messages in thread
From: Michael J Gruber @ 2011-02-18 10:12 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Jakub Narebski

Currently, patch-id trips over the diff extension for marking
the absence of newline at EOF.

Expose this in a test.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t4204-patch-id.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index 68e2652..db96064 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -63,4 +63,40 @@ test_expect_success 'patch-id supports git-format-patch MIME output' '
 	test_cmp patch-id_master patch-id_same
 '
 
+cat >nonl <<\EOF
+diff --git i/a w/a
+index e69de29..2e65efe 100644
+--- i/a
++++ w/a
+@@ -0,0 +1 @@
++a
+\ No newline at end of file
+diff --git i/b w/b
+index e69de29..6178079 100644
+--- i/b
++++ w/b
+@@ -0,0 +1 @@
++b
+EOF
+
+cat >withnl <<\EOF
+diff --git i/a w/a
+index e69de29..7898192 100644
+--- i/a
++++ w/a
+@@ -0,0 +1 @@
++a
+diff --git i/b w/b
+index e69de29..6178079 100644
+--- i/b
++++ w/b
+@@ -0,0 +1 @@
++b
+EOF
+
+test_expect_failure 'patch-id handles no-nl-at-eof markers' '
+	cat nonl | calc_patch_id nonl &&
+	cat withnl | calc_patch_id withnl &&
+	test_cmp patch-id_nonl patch-id_withnl
+'
 test_done
-- 
1.7.4.1.74.gf39475.dirty

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCHv3 2/2] git-patch-id: do not trip over "no newline" markers
  2011-02-18 10:12                 ` [PATCHv3 1/2] git-patch-id: test for " Michael J Gruber
@ 2011-02-18 10:12                   ` Michael J Gruber
  2011-02-18 10:41                     ` Jeff King
  2011-02-18 14:29                     ` Jakub Narebski
  0 siblings, 2 replies; 18+ messages in thread
From: Michael J Gruber @ 2011-02-18 10:12 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Jakub Narebski

Currently, patch-id trips over the diff extension for marking
the absence of newline at EOF.

Fix it. (Ignore it, it's whitespace.)

This uses the same detection rationale as in buitlin/apply.c, which was
introduced in

433ef8a ([PATCH] Make git-apply understand incomplete lines in non-C locales, 2005-09-04)

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/patch-id.c  |    2 ++
 t/t4204-patch-id.sh |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 5125300..e1c3cb9 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -73,6 +73,8 @@ int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)
 			p += 7;
 		else if (!memcmp(line, "From ", 5))
 			p += 5;
+		else if (!memcmp(line, "\\ ", 2) && strlen(line)>=12)
+			continue;
 
 		if (!get_sha1_hex(p, next_sha1)) {
 			found_next = 1;
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index db96064..d2c930d 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -94,7 +94,7 @@ index e69de29..6178079 100644
 +b
 EOF
 
-test_expect_failure 'patch-id handles no-nl-at-eof markers' '
+test_expect_success 'patch-id handles no-nl-at-eof markers' '
 	cat nonl | calc_patch_id nonl &&
 	cat withnl | calc_patch_id withnl &&
 	test_cmp patch-id_nonl patch-id_withnl
-- 
1.7.4.1.74.gf39475.dirty

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCHv2 2/2] git-patch-id: do not trip over "no newline" markers
  2011-02-18  8:02               ` Michael J Gruber
  2011-02-18 10:12                 ` [PATCHv3 1/2] git-patch-id: test for " Michael J Gruber
@ 2011-02-18 10:40                 ` Jeff King
  1 sibling, 0 replies; 18+ messages in thread
From: Jeff King @ 2011-02-18 10:40 UTC (permalink / raw)
  To: Michael J Gruber
  Cc: git, Ævar Arnfjörð Bjarmason, Junio C Hamano

On Fri, Feb 18, 2011 at 09:02:23AM +0100, Michael J Gruber wrote:

> > Wow, that's pretty obscure. I wonder if the test should be factored out
> > into line_is_no_newline_at_end_of_file() (or surely there is some more
> > sensible name), and used by both apply and patch-id. Along with a nice
> > comment (which I see apply already has) describing what in the world the
> > magic number 12 means.
> 
> So, where do you suggest it should go? As far as I can see, we have two
> places reading these markers (the above builtins) and two places writing
> them (diff.c, xdiff/xutils.c), at least for git-core.

I think you could ignore the writers, as they are much more
straightforward.  But...

> (git-gui's diff.tcl has its own strict check etc.)

Ugh. Factoring that out with the C code would not be easy. :)

> I would opt for putting a more detailed explanation in the commit
> message (reasoning + ref. to apply.c) rather then trying to factor this.

I think that's OK. It's not a lot of code, it's just very non-obvious.
So a comment is probably as good as a refactor.

> After all, our code is largely undocumented as far as inline
> documentation goes (it has frustrated me a few times), and the more
> comprehensive documentation are the commit messages which git blame
> points you to.

Yeah, it is sometimes annoying. On the other hand, you know that a
comment in the commit log cannot be stale, because you are viewing it in
context (well, it can be, but theoretically you have the tools to
determine that).

I'm not sure what the right balance is. I tend to write inline comments
for small non-obvious things (e.g., why a variable is being strdup'd),
and describe the bigger picture in the commit message (e.g., why a
particular algorithm was chosen).

But it has only been a few years that we've had version control systems
where history browsing wasn't absolutely horrendous. Maybe in another
decade or so there will be best practices for this sort of thing. :)

-Peff

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 2/2] git-patch-id: do not trip over "no newline" markers
  2011-02-18 10:12                   ` [PATCHv3 2/2] git-patch-id: do not trip over " Michael J Gruber
@ 2011-02-18 10:41                     ` Jeff King
  2011-02-18 10:43                       ` Michael J Gruber
  2011-02-18 14:29                     ` Jakub Narebski
  1 sibling, 1 reply; 18+ messages in thread
From: Jeff King @ 2011-02-18 10:41 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Junio C Hamano, Jakub Narebski

On Fri, Feb 18, 2011 at 11:12:42AM +0100, Michael J Gruber wrote:

> Currently, patch-id trips over the diff extension for marking
> the absence of newline at EOF.
> 
> Fix it. (Ignore it, it's whitespace.)
> 
> This uses the same detection rationale as in buitlin/apply.c, which was
> introduced in
> 
> 433ef8a ([PATCH] Make git-apply understand incomplete lines in non-C locales, 2005-09-04)

Thanks, this looks good to me, with one style nit:

> +		else if (!memcmp(line, "\\ ", 2) && strlen(line)>=12)
> +			continue;

Whitespace around relational operators.

-Peff

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 2/2] git-patch-id: do not trip over "no newline" markers
  2011-02-18 10:41                     ` Jeff King
@ 2011-02-18 10:43                       ` Michael J Gruber
  2011-02-18 10:47                         ` Jeff King
  0 siblings, 1 reply; 18+ messages in thread
From: Michael J Gruber @ 2011-02-18 10:43 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Jakub Narebski

Jeff King venit, vidit, dixit 18.02.2011 11:41:
> On Fri, Feb 18, 2011 at 11:12:42AM +0100, Michael J Gruber wrote:
> 
>> Currently, patch-id trips over the diff extension for marking
>> the absence of newline at EOF.
>>
>> Fix it. (Ignore it, it's whitespace.)
>>
>> This uses the same detection rationale as in buitlin/apply.c, which was
>> introduced in
>>
>> 433ef8a ([PATCH] Make git-apply understand incomplete lines in non-C locales, 2005-09-04)
> 
> Thanks, this looks good to me, with one style nit:
> 
>> +		else if (!memcmp(line, "\\ ", 2) && strlen(line)>=12)
>> +			continue;
> 
> Whitespace around relational operators.

You think

+		else if (!memcmp(line, "\\ ", 2)&&strlen(line)>=12)

is that much better?

Nitpickers need to brace for smartass responses ;)

Junio, should I resend with two extra spaces?

Michael

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 2/2] git-patch-id: do not trip over "no newline" markers
  2011-02-18 10:43                       ` Michael J Gruber
@ 2011-02-18 10:47                         ` Jeff King
  0 siblings, 0 replies; 18+ messages in thread
From: Jeff King @ 2011-02-18 10:47 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Junio C Hamano, Jakub Narebski

On Fri, Feb 18, 2011 at 11:43:16AM +0100, Michael J Gruber wrote:

> >> +		else if (!memcmp(line, "\\ ", 2) && strlen(line)>=12)
> >> +			continue;
> > 
> > Whitespace around relational operators.
> 
> You think
> 
> +		else if (!memcmp(line, "\\ ", 2)&&strlen(line)>=12)
> 
> is that much better?

Yeah, it's starting to look like perl. :)

-Peff

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCHv3 2/2] git-patch-id: do not trip over "no newline" markers
  2011-02-18 10:12                   ` [PATCHv3 2/2] git-patch-id: do not trip over " Michael J Gruber
  2011-02-18 10:41                     ` Jeff King
@ 2011-02-18 14:29                     ` Jakub Narebski
  1 sibling, 0 replies; 18+ messages in thread
From: Jakub Narebski @ 2011-02-18 14:29 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Junio C Hamano, Jeff King

Michael J Gruber wrote:

> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -73,6 +73,8 @@ int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)
>  			p += 7;
>  		else if (!memcmp(line, "From ", 5))
>  			p += 5;
> +		else if (!memcmp(line, "\\ ", 2) && strlen(line)>=12)
> +			continue;
>  
>  		if (!get_sha1_hex(p, next_sha1)) {
>  			found_next = 1;

All of those are minor issues:

1. Whitespace (coding style) - it should be

   +		else if (!memcmp(line, "\\ ", 2) && strlen(line) >= 12)

2. Is "\\ " needed, or "\\" would be enough (line[0] == '\')?

3. What is this 12 in "strlen(line) >= 12" about?  Note that 
   strlen(<const>) is optimized out during compiling.

-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2011-02-18 14:29 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-16 14:56 Why does git-patch-id(1) sometimes print two lines, one of which has commit = 0000000000000000000000000000000000000000 Ævar Arnfjörð Bjarmason
2011-02-16 16:11 ` Michael J Gruber
2011-02-16 16:55   ` [PATCH 1/2] git-patch-id: test for "no newline" markers Michael J Gruber
2011-02-16 16:55     ` [PATCH 2/2] git-patch-id: do not trip over " Michael J Gruber
2011-02-16 20:05       ` Junio C Hamano
2011-02-17  7:44         ` [PATCHv2 1/2] git-patch-id: test for " Michael J Gruber
2011-02-17  7:44           ` [PATCHv2 2/2] git-patch-id: do not trip over " Michael J Gruber
2011-02-18  4:16             ` Jeff King
2011-02-18  8:02               ` Michael J Gruber
2011-02-18 10:12                 ` [PATCHv3 1/2] git-patch-id: test for " Michael J Gruber
2011-02-18 10:12                   ` [PATCHv3 2/2] git-patch-id: do not trip over " Michael J Gruber
2011-02-18 10:41                     ` Jeff King
2011-02-18 10:43                       ` Michael J Gruber
2011-02-18 10:47                         ` Jeff King
2011-02-18 14:29                     ` Jakub Narebski
2011-02-18 10:40                 ` [PATCHv2 " Jeff King
2011-02-17 11:55           ` [PATCHv2 1/2] git-patch-id: test for " Jakub Narebski
2011-02-17 12:11             ` Michael J Gruber

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).