[PATCH 0/3] Make Git compile warning-free with Clang

[PATCH 0/3] Make Git compile warning-free with Clang

[John Keeping]

The first two patches here were sent to the list before but seem to have
got lost in the noise [1][2].  The final one is new but was prompted by
discussion in the same thread.

After applying all of these patches, I don't see any warnings compiling
Git with Clang 3.2.

[1] http://article.gmane.org/gmane.comp.version-control.git/213817
[2] http://article.gmane.org/gmane.comp.version-control.git/213849

Antoine Pelisse (1):
  fix clang -Wtautological-compare with unsigned enum

John Keeping (2):
  combine-diff: suppress a clang warning
  builtin/apply: tighten (dis)similarity index parsing

 builtin/apply.c | 10 ++++++----
 combine-diff.c  |  2 +-
 grep.c          |  3 ++-
 grep.h          |  3 ++-
 4 files changed, 11 insertions(+), 7 deletions(-)

-- 
1.8.1.2

[PATCH 1/3] fix clang -Wtautological-compare with unsigned enum

[John Keeping]

From: Antoine Pelisse <apelisse@gmail.com>

Create a GREP_HEADER_FIELD_MIN so we can check that the field value is
sane and silent the clang warning.

Clang warning happens because the enum is unsigned (this is
implementation-defined, and there is no negative fields) and the check
is then tautological.

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 grep.c | 3 ++-
 grep.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/grep.c b/grep.c
index 4bd1b8b..bb548ca 100644
--- a/grep.c
+++ b/grep.c
@@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 	for (p = opt->header_list; p; p = p->next) {
 		if (p->token != GREP_PATTERN_HEAD)
 			die("bug: a non-header pattern in grep header list.");
-		if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
+		if (p->field < GREP_HEADER_FIELD_MIN ||
+		    GREP_HEADER_FIELD_MAX <= p->field)
 			die("bug: unknown header field %d", p->field);
 		compile_regexp(p, opt);
 	}
diff --git a/grep.h b/grep.h
index 8fc854f..e4a1df5 100644
--- a/grep.h
+++ b/grep.h
@@ -28,7 +28,8 @@ enum grep_context {
 };
 
 enum grep_header_field {
-	GREP_HEADER_AUTHOR = 0,
+	GREP_HEADER_FIELD_MIN = 0,
+	GREP_HEADER_AUTHOR = GREP_HEADER_FIELD_MIN,
 	GREP_HEADER_COMMITTER,
 	GREP_HEADER_REFLOG,
 
-- 
1.8.1.2

[PATCH 2/3] combine-diff: suppress a clang warning

[John Keeping]

When compiling combine-diff.c, clang 3.2 says:

    combine-diff.c:1006:19: warning: adding 'int' to a string does not
	    append to the string [-Wstring-plus-int]
		prefix = COLONS + offset;
			 ~~~~~~~^~~~~~~~
    combine-diff.c:1006:19: note: use array indexing to silence this warning
		prefix = COLONS + offset;
				^
			 &      [       ]

Suppress this by making the suggested change.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 combine-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/combine-diff.c b/combine-diff.c
index bb1cc96..dba4748 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 		offset = strlen(COLONS) - num_parent;
 		if (offset < 0)
 			offset = 0;
-		prefix = COLONS + offset;
+		prefix = &COLONS[offset];
 
 		/* Show the modes */
 		for (i = 0; i < num_parent; i++) {
-- 
1.8.1.2

[PATCH 3/3] builtin/apply: tighten (dis)similarity index parsing

[John Keeping]

This was prompted by an incorrect warning issued by clang [1], and a
suggestion by Linus to restrict the range to check for values greater
than INT_MAX since these will give bogus output after casting to int.

In fact the (dis)similarity index is a percentage, so reject values
greater than 100.

[1] http://article.gmane.org/gmane.comp.version-control.git/213857

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 builtin/apply.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 6c11e8b..4745e75 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1041,15 +1041,17 @@ static int gitdiff_renamedst(const char *line, struct patch *patch)
 
 static int gitdiff_similarity(const char *line, struct patch *patch)
 {
-	if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
-		patch->score = 0;
+	unsigned long val = strtoul(line, NULL, 10);
+	if (val <= 100)
+		patch->score = val;
 	return 0;
 }
 
 static int gitdiff_dissimilarity(const char *line, struct patch *patch)
 {
-	if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
-		patch->score = 0;
+	unsigned long val = strtoul(line, NULL, 10);
+	if (val <= 100)
+		patch->score = val;
 	return 0;
 }
 
-- 
1.8.1.2

Re: [PATCH 0/3] Make Git compile warning-free with Clang

[Antoine Pelisse]

Thanks John,
I couldn't find any time to send that "sum-up" series.

On Sun, Feb 3, 2013 at 3:37 PM, John Keeping <john@keeping.me.uk> wrote:
> The first two patches here were sent to the list before but seem to have
> got lost in the noise [1][2].  The final one is new but was prompted by
> discussion in the same thread.

Re: [PATCH 2/3] combine-diff: suppress a clang warning

[Tay Ray Chuan]

On Sun, Feb 3, 2013 at 10:37 PM, John Keeping <john@keeping.me.uk> wrote:
> When compiling combine-diff.c, clang 3.2 says:
>
>     combine-diff.c:1006:19: warning: adding 'int' to a string does not
>             append to the string [-Wstring-plus-int]
>                 prefix = COLONS + offset;
>                          ~~~~~~~^~~~~~~~
>     combine-diff.c:1006:19: note: use array indexing to silence this warning
>                 prefix = COLONS + offset;
>                                 ^
>                          &      [       ]
>
> Suppress this by making the suggested change.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  combine-diff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/combine-diff.c b/combine-diff.c
> index bb1cc96..dba4748 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
>                 offset = strlen(COLONS) - num_parent;
>                 if (offset < 0)
>                         offset = 0;
> -               prefix = COLONS + offset;
> +               prefix = &COLONS[offset];
>
>                 /* Show the modes */
>                 for (i = 0; i < num_parent; i++) {
> --

Hmm, does

               prefix = (const char *) COLONS + offset;

suppress the warning?

--
Cheers,
Ray Chuan

Re: [PATCH 2/3] combine-diff: suppress a clang warning

[John Keeping]

On Mon, Feb 04, 2013 at 02:20:06AM +0800, Tay Ray Chuan wrote:
> On Sun, Feb 3, 2013 at 10:37 PM, John Keeping <john@keeping.me.uk> wrote:
> > When compiling combine-diff.c, clang 3.2 says:
> >
> >     combine-diff.c:1006:19: warning: adding 'int' to a string does not
> >             append to the string [-Wstring-plus-int]
> >                 prefix = COLONS + offset;
> >                          ~~~~~~~^~~~~~~~
> >     combine-diff.c:1006:19: note: use array indexing to silence this warning
> >                 prefix = COLONS + offset;
> >                                 ^
> >                          &      [       ]
> >
> > Suppress this by making the suggested change.
> >
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> >  combine-diff.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/combine-diff.c b/combine-diff.c
> > index bb1cc96..dba4748 100644
> > --- a/combine-diff.c
> > +++ b/combine-diff.c
> > @@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
> >                 offset = strlen(COLONS) - num_parent;
> >                 if (offset < 0)
> >                         offset = 0;
> > -               prefix = COLONS + offset;
> > +               prefix = &COLONS[offset];
> >
> >                 /* Show the modes */
> >                 for (i = 0; i < num_parent; i++) {
> 
> Hmm, does
> 
>                prefix = (const char *) COLONS + offset;
> 
> suppress the warning?

It does, but it turns out that the following also suppresses the
warning:

-- >8 --

diff --git a/combine-diff.c b/combine-diff.c
index bb1cc96..a07d329 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -982,7 +982,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	free(sline);
 }
 
-#define COLONS "::::::::::::::::::::::::::::::::"
+static const char COLONS[] = "::::::::::::::::::::::::::::::::";
 
 static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)
 {

I think that's a nicer change than the original suggestion.


John

Re: [PATCH 1/3] fix clang -Wtautological-compare with unsigned enum

[Jonathan Nieder]

John Keeping wrote:

> From: Antoine Pelisse <apelisse@gmail.com>
>
> Create a GREP_HEADER_FIELD_MIN so we can check that the field value is
> sane and silent the clang warning.

Thanks.  Looks good to me.

[...]
> --- a/grep.c
> +++ b/grep.c
> @@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
>  	for (p = opt->header_list; p; p = p->next) {
>  		if (p->token != GREP_PATTERN_HEAD)
>  			die("bug: a non-header pattern in grep header list.");
> -		if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
> +		if (p->field < GREP_HEADER_FIELD_MIN ||
> +		    GREP_HEADER_FIELD_MAX <= p->field)
>  			die("bug: unknown header field %d", p->field);

I also think it would be fine to drop this test or replace it with an

	assert((unsigned) p->field < ARRAY_SIZE(header_field));

because we know the test never trips.

Re: [PATCH 2/3] combine-diff: suppress a clang warning

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> When compiling combine-diff.c, clang 3.2 says:
>
>     combine-diff.c:1006:19: warning: adding 'int' to a string does not
> 	    append to the string [-Wstring-plus-int]
> 		prefix = COLONS + offset;
> 			 ~~~~~~~^~~~~~~~
>     combine-diff.c:1006:19: note: use array indexing to silence this warning
> 		prefix = COLONS + offset;
> 				^
> 			 &      [       ]
>
> Suppress this by making the suggested change.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---

This was not lost in the noise.

I thought that this wasn't a serious patch, but your attempt to
demonstrate to others why patches trying to squelch clang warnings
are not necessarily a good thing to do.

Who is that compiler trying to help with such a warning message?
After all, we are writing in C, and clang is supposed to be a C
compiler.  And adding integer to a pointer to (const) char is a
straight-forward way to look at the trailing part of a given string.

> -		prefix = COLONS + offset;
> +		prefix = &COLONS[offset];

In other words, both are perfectly valid C.  Why should we make it
less readable to avoid a stupid compiler warning?

Re: [PATCH 2/3] combine-diff: suppress a clang warning

[John Keeping]

On Sun, Feb 03, 2013 at 11:58:15AM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > When compiling combine-diff.c, clang 3.2 says:
> >
> >     combine-diff.c:1006:19: warning: adding 'int' to a string does not
> > 	    append to the string [-Wstring-plus-int]
> > 		prefix = COLONS + offset;
> > 			 ~~~~~~~^~~~~~~~
> >     combine-diff.c:1006:19: note: use array indexing to silence this warning
> > 		prefix = COLONS + offset;
> > 				^
> > 			 &      [       ]
> >
> > Suppress this by making the suggested change.
> >
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> 
> This was not lost in the noise.
> 
> I thought that this wasn't a serious patch, but your attempt to
> demonstrate to others why patches trying to squelch clang warnings
> are not necessarily a good thing to do.
>
> Who is that compiler trying to help with such a warning message?
> After all, we are writing in C, and clang is supposed to be a C
> compiler.  And adding integer to a pointer to (const) char is a
> straight-forward way to look at the trailing part of a given string.

A quick search turned up the original thread where this feature was
added to Clang [1].  It seems that it does find genuine bugs where
people try to log values by doing:

    log("failed to handle error: " + errno);

[1] http://thread.gmane.org/gmane.comp.compilers.clang.scm/47203

> > -		prefix = COLONS + offset;
> > +		prefix = &COLONS[offset];
> 
> In other words, both are perfectly valid C.  Why should we make it
> less readable to avoid a stupid compiler warning?

Are you happy to change COLONS to a const char[] instead of a #define?
That also suppresses the warning.

Since Git is warning-free on GCC and so close to being warning-free on
recent Clang I think it is worthwhile to fix the remaining two issues
which do seem to be intentional diagnostics rather than Clang bugs.


John

Re: [PATCH 3/3] builtin/apply: tighten (dis)similarity index parsing

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> diff --git a/builtin/apply.c b/builtin/apply.c
> index 6c11e8b..4745e75 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -1041,15 +1041,17 @@ static int gitdiff_renamedst(const char *line, struct patch *patch)
>  
>  static int gitdiff_similarity(const char *line, struct patch *patch)
>  {
> -	if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
> -		patch->score = 0;
> +	unsigned long val = strtoul(line, NULL, 10);
> +	if (val <= 100)
> +		patch->score = val;
>  	return 0;
>  }
>  
>  static int gitdiff_dissimilarity(const char *line, struct patch *patch)
>  {
> -	if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
> -		patch->score = 0;
> +	unsigned long val = strtoul(line, NULL, 10);
> +	if (val <= 100)
> +		patch->score = val;
>  	return 0;
>  }

This makes sort of sense; .score is used only for display and not
for making any decision, so as long as you know it is initialized to
zero when the call to this function is made, it should be OK.

Thanks.

Re: [PATCH 2/3] combine-diff: suppress a clang warning

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> A quick search turned up the original thread where this feature was
> added to Clang [1].  It seems that it does find genuine bugs where
> people try to log values by doing:
>
>     log("failed to handle error: " + errno);

To be perfectly honest, anybody who writes such a code should be
sent back to school before trying to touch out code ever again ;-).
It is not even valid Python, Perl nor Java, I would think.

> Are you happy to change COLONS to a const char[] instead of a #define?

Happy?  Not really.

It could be a good change for entirely different reason. We will
save space if we ever need to use it in multiple places.  But the
entire "COLONS + offset" thing was a hack we did, knowing that it
will break when we end up showing a muiti-way diff for more than 32
blobs.

If we were to be touching that area of code, I'd rather see a change
to make it more robust against such a corner case.  If it results in
squelching misguided clang warnings against programmers who should
not be writing in C, that is a nice side effect, but I loathe to see
any change whose primary purpose is to squelch pointless warnings.

 combine-diff.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index bb1cc96..7f6187f 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -982,14 +982,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	free(sline);
 }
 
-#define COLONS "::::::::::::::::::::::::::::::::"
-
 static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)
 {
 	struct diff_options *opt = &rev->diffopt;
-	int i, offset;
-	const char *prefix;
-	int line_termination, inter_name_termination;
+	int line_termination, inter_name_termination, i;
 
 	line_termination = opt->line_termination;
 	inter_name_termination = '\t';
@@ -1000,17 +996,14 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 		show_log(rev);
 
 	if (opt->output_format & DIFF_FORMAT_RAW) {
-		offset = strlen(COLONS) - num_parent;
-		if (offset < 0)
-			offset = 0;
-		prefix = COLONS + offset;
+		/* As many colons as there are parents */
+		for (i = 0; i < num_parent; i++)
+			putchar(':');
 
 		/* Show the modes */
-		for (i = 0; i < num_parent; i++) {
-			printf("%s%06o", prefix, p->parent[i].mode);
-			prefix = " ";
-		}
-		printf("%s%06o", prefix, p->mode);
+		for (i = 0; i < num_parent; i++)
+			printf("%06o ", p->parent[i].mode);
+		printf("%06o", p->mode);
 
 		/* Show sha1's */
 		for (i = 0; i < num_parent; i++)

Re: [PATCH 2/3] combine-diff: suppress a clang warning

[John Keeping]

On Sun, Feb 03, 2013 at 01:07:48PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > A quick search turned up the original thread where this feature was
> > added to Clang [1].  It seems that it does find genuine bugs where
> > people try to log values by doing:
> >
> >     log("failed to handle error: " + errno);
> 
> To be perfectly honest, anybody who writes such a code should be
> sent back to school before trying to touch out code ever again ;-).

Yeah, I can't see that getting through review here :-).

> It is not even valid Python, Perl nor Java, I would think.

It is valid Java, although I can't think of any other languages that let
you do that.

> > Are you happy to change COLONS to a const char[] instead of a #define?
> 
> Happy?  Not really.
> 
> It could be a good change for entirely different reason. We will
> save space if we ever need to use it in multiple places.  But the
> entire "COLONS + offset" thing was a hack we did, knowing that it
> will break when we end up showing a muiti-way diff for more than 32
> blobs.
> 
> If we were to be touching that area of code, I'd rather see a change
> to make it more robust against such a corner case.  If it results in
> squelching misguided clang warnings against programmers who should
> not be writing in C, that is a nice side effect, but I loathe to see
> any change whose primary purpose is to squelch pointless warnings.

This seems like a sensible change.

I generally like to get rid of the pointless warnings so that the useful
ones can't hide in the noise.  Perhaps "CFLAGS += -Wno-string-plus-int"
would be better for this particular warning, but when there's only one
bit of code that triggers it, tweaking that seemed simpler.

>  combine-diff.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/combine-diff.c b/combine-diff.c
> index bb1cc96..7f6187f 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -982,14 +982,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
>  	free(sline);
>  }
>  
> -#define COLONS "::::::::::::::::::::::::::::::::"
> -
>  static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)
>  {
>  	struct diff_options *opt = &rev->diffopt;
> -	int i, offset;
> -	const char *prefix;
> -	int line_termination, inter_name_termination;
> +	int line_termination, inter_name_termination, i;
>  
>  	line_termination = opt->line_termination;
>  	inter_name_termination = '\t';
> @@ -1000,17 +996,14 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
>  		show_log(rev);
>  
>  	if (opt->output_format & DIFF_FORMAT_RAW) {
> -		offset = strlen(COLONS) - num_parent;
> -		if (offset < 0)
> -			offset = 0;
> -		prefix = COLONS + offset;
> +		/* As many colons as there are parents */
> +		for (i = 0; i < num_parent; i++)
> +			putchar(':');
>  
>  		/* Show the modes */
> -		for (i = 0; i < num_parent; i++) {
> -			printf("%s%06o", prefix, p->parent[i].mode);
> -			prefix = " ";
> -		}
> -		printf("%s%06o", prefix, p->mode);
> +		for (i = 0; i < num_parent; i++)
> +			printf("%06o ", p->parent[i].mode);
> +		printf("%06o", p->mode);
>  
>  		/* Show sha1's */
>  		for (i = 0; i < num_parent; i++)

Re: [PATCH 2/3] combine-diff: suppress a clang warning

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

>> If we were to be touching that area of code, I'd rather see a change
>> to make it more robust against such a corner case.  If it results in
>> squelching misguided clang warnings against programmers who should
>> not be writing in C, that is a nice side effect, but I loathe to see
>> any change whose primary purpose is to squelch pointless warnings.
>
> This seems like a sensible change.
>
> I generally like to get rid of the pointless warnings so that the useful
> ones can't hide in the noise.  Perhaps "CFLAGS += -Wno-string-plus-int"
> would be better for this particular warning, but when there's only one
> bit of code that triggers it, tweaking that seemed simpler.

Thanks for a sanity check.  Ideally it should also have test cases
to show "git diff --cc --raw blob1 blob2...blob$n" for n=4 and n=40
(or any two values clearly below and above the old hardcoded limit)
behave sensibly, exposing the old breakage, which I'll leave as a
LHF (low-hanging-fruit).  Hint, hint...

-- >8 --
Subject: [PATCH] combine-diff: lift 32-way limit of combined diff

The "raw" format of combine-diff output is supposed to have as many
colons as there are parents at the beginning, then blob modes for
these parents, and then object names for these parents.

We weren't however prepared to handle a more than 32-way merge and
did not show the correct number of colons in such a case.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 combine-diff.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index bb1cc96..7f6187f 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -982,14 +982,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	free(sline);
 }
 
-#define COLONS "::::::::::::::::::::::::::::::::"
-
 static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)
 {
 	struct diff_options *opt = &rev->diffopt;
-	int i, offset;
-	const char *prefix;
-	int line_termination, inter_name_termination;
+	int line_termination, inter_name_termination, i;
 
 	line_termination = opt->line_termination;
 	inter_name_termination = '\t';
@@ -1000,17 +996,14 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 		show_log(rev);
 
 	if (opt->output_format & DIFF_FORMAT_RAW) {
-		offset = strlen(COLONS) - num_parent;
-		if (offset < 0)
-			offset = 0;
-		prefix = COLONS + offset;
+		/* As many colons as there are parents */
+		for (i = 0; i < num_parent; i++)
+			putchar(':');
 
 		/* Show the modes */
-		for (i = 0; i < num_parent; i++) {
-			printf("%s%06o", prefix, p->parent[i].mode);
-			prefix = " ";
-		}
-		printf("%s%06o", prefix, p->mode);
+		for (i = 0; i < num_parent; i++)
+			printf("%06o ", p->parent[i].mode);
+		printf("%06o", p->mode);
 
 		/* Show sha1's */
 		for (i = 0; i < num_parent; i++)
-- 
1.8.1.2.628.geb8a6d5

[PATCH] t4038: add tests for "diff --cc --raw <trees>"

[John Keeping]

Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Sun, Feb 03, 2013 at 04:24:52PM -0800, Junio C Hamano wrote:
>                             Ideally it should also have test cases
> to show "git diff --cc --raw blob1 blob2...blob$n" for n=4 and n=40
> (or any two values clearly below and above the old hardcoded limit)
> behave sensibly, exposing the old breakage, which I'll leave as a
> LHF (low-hanging-fruit).  Hint, hint...

Hint taken ;-)

git-diff uses a different code path for blobs, so I've had to use trees
to trigger this.  The last test fails without
jc/combine-diff-many-parents and passes with it.

 t/t4038-diff-combined.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 40277c7..a0701bc 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -89,4 +89,33 @@ test_expect_success 'diagnose truncated file' '
 	grep "diff --cc file" out
 '
 
+test_expect_success 'setup for --cc --raw' '
+	blob=$(echo file |git hash-object --stdin -w) &&
+	base_tree=$(echo "100644 blob $blob	file" | git mktree) &&
+	trees= &&
+	for i in `test_seq 1 40`
+	do
+		blob=$(echo file$i |git hash-object --stdin -w) &&
+		trees="$trees $(echo "100644 blob $blob	file" |git mktree)"
+	done
+'
+
+test_expect_success 'check --cc --raw with four trees' '
+	four_trees=$(echo "$trees" |awk -e "{
+		print \$1
+		print \$2
+		print \$3
+		print \$4
+	}") &&
+	git diff --cc --raw $four_trees $base_tree >out &&
+	# Check for four leading colons in the output:
+	grep "^::::[^:]" out
+'
+
+test_expect_success 'check --cc --raw with forty trees' '
+	git diff --cc --raw $trees $base_tree >out &&
+	# Check for forty leading colons in the output:
+	grep "^::::::::::::::::::::::::::::::::::::::::[^:]" out
+'
+
 test_done
-- 
1.8.1.2

Re: [PATCH] t4038: add tests for "diff --cc --raw <trees>"

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> ...
> diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
> index 40277c7..a0701bc 100755
> --- a/t/t4038-diff-combined.sh
> +++ b/t/t4038-diff-combined.sh
> @@ -89,4 +89,33 @@ test_expect_success 'diagnose truncated file' '
>  	grep "diff --cc file" out
>  '
>  
> +test_expect_success 'setup for --cc --raw' '
> +	blob=$(echo file |git hash-object --stdin -w) &&
> +	base_tree=$(echo "100644 blob $blob	file" | git mktree) &&
> +	trees= &&
> +	for i in `test_seq 1 40`
> +	do
> +		blob=$(echo file$i |git hash-object --stdin -w) &&
> +		trees="$trees $(echo "100644 blob $blob	file" |git mktree)"

Please have a SP after each of these '|' pipes.

If you collect trees this way:

	trees="$trees$(echo ... | git mktree)$LF"

then ...

> +	done
> +'
> +
> +test_expect_success 'check --cc --raw with four trees' '
> +	four_trees=$(echo "$trees" |awk -e "{
> +		print \$1
> +		print \$2
> +		print \$3
> +		print \$4
> +	}") &&

(What's "awk -e"?)

... you can do

	echo "$trees" | sed -e 4q

which is less repetitive.

Thanks.

[PATCH v2] t4038: add tests for "diff --cc --raw <trees>"

[John Keeping]

Signed-off-by: John Keeping <john@keeping.me.uk>

---
Changes since v1:

- more spaces around '|'
- create trees with line feeds and use 'sed -e 4q'
---
 t/t4038-diff-combined.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 40277c7..614425a 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -89,4 +89,28 @@ test_expect_success 'diagnose truncated file' '
 	grep "diff --cc file" out
 '
 
+test_expect_success 'setup for --cc --raw' '
+	blob=$(echo file | git hash-object --stdin -w) &&
+	base_tree=$(echo "100644 blob $blob	file" | git mktree) &&
+	trees= &&
+	for i in `test_seq 1 40`
+	do
+		blob=$(echo file$i | git hash-object --stdin -w) &&
+		trees="$trees$(echo "100644 blob $blob	file" | git mktree)$LF"
+	done
+'
+
+test_expect_success 'check --cc --raw with four trees' '
+	four_trees=$(echo "$trees" | sed -e 4q) &&
+	git diff --cc --raw $four_trees $base_tree >out &&
+	# Check for four leading colons in the output:
+	grep "^::::[^:]" out
+'
+
+test_expect_success 'check --cc --raw with forty trees' '
+	git diff --cc --raw $trees $base_tree >out &&
+	# Check for forty leading colons in the output:
+	grep "^::::::::::::::::::::::::::::::::::::::::[^:]" out
+'
+
 test_done
-- 
1.8.1.2.689.g36c777b

Re: [PATCH v2] t4038: add tests for "diff --cc --raw <trees>"

[Junio C Hamano]

Thanks.

Re: [PATCH 2/3] combine-diff: suppress a clang warning

[Miles Bader]

John Keeping <john@keeping.me.uk> writes:
> I generally like to get rid of the pointless warnings so that the useful
> ones can't hide in the noise.  Perhaps "CFLAGS += -Wno-string-plus-int"
> would be better for this particular warning, but when there's only one
> bit of code that triggers it, tweaking that seemed simpler.

An even better approach would be to file a bug against clang ... it
really is a very ill-considered warning -- PTR + OFFS is not just
valid C, it's _idiomatic_ in C for getting interior pointers into
arrays -- and such a warning should never be enabled by default, or by
any standard warning options.

-miles 

-- 
永日の　澄んだ紺から　永遠へ

Re: [PATCH 2/3] combine-diff: suppress a clang warning

[John Keeping]

On Thu, Feb 07, 2013 at 01:12:59PM +0900, Miles Bader wrote:
> John Keeping <john@keeping.me.uk> writes:
> > I generally like to get rid of the pointless warnings so that the useful
> > ones can't hide in the noise.  Perhaps "CFLAGS += -Wno-string-plus-int"
> > would be better for this particular warning, but when there's only one
> > bit of code that triggers it, tweaking that seemed simpler.
> 
> An even better approach would be to file a bug against clang ... it
> really is a very ill-considered warning -- PTR + OFFS is not just
> valid C, it's _idiomatic_ in C for getting interior pointers into
> arrays -- and such a warning should never be enabled by default, or by
> any standard warning options.

It doesn't warn of PTR + OFFS, only STRING_LITERAL + OFFS.  I agree that
it's not a particularly useful warning but it was clearly introduced
intentionally and appears to find real bugs [1] so I don't intend to
argue about it with the Clang developers.

[1] http://article.gmane.org/gmane.comp.compilers.clang.scm/47203


John