[PATCH] diff: remove ternary operator evaluating always to true

[PATCH] diff: remove ternary operator evaluating always to true

[Stefan Beller]

The line being changed is deep inside the function builtin_diff.
The variable name_b, which is used to evaluate the ternary expression
must evaluate to true at that position, hence the replacement with
just name_b.

The name_b variable only occurs a few times in that lengthy function:
As a parameter to the function itself:
	static void builtin_diff(const char *name_a,
				 const char *name_b,
				...
The next occurrences are at:
	/* Never use a non-valid filename anywhere if at all possible */
	name_a = DIFF_FILE_VALID(one) ? name_a : name_b;
	name_b = DIFF_FILE_VALID(two) ? name_b : name_a;

	a_one = quote_two(a_prefix, name_a + (*name_a == '/'));
	b_two = quote_two(b_prefix, name_b + (*name_b == '/'));

In the last line of this block 'name_b' is dereferenced and compared
to '/'. This would crash if name_b was NULL. Hence in the following code
we can assume name_b being non-null.

The next occurrence is just as a function argument, which doesn't change
the memory, which name_b points to, so the assumption name_b being not
null still holds:
	emit_rewrite_diff(name_a, name_b, one, two,
				textconv_one, textconv_two, o);

The next occurrence would be the line of this patch. As name_b still must
be not null, we can remove the ternary operator.


Inside the emit_rewrite_diff function there is a also a line
	ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
which was also simplified as there is also a dereference before the
ternary operator.

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 266112c..80f8439 100644
--- a/diff.c
+++ b/diff.c
@@ -669,7 +669,7 @@ static void emit_rewrite_diff(const char *name_a,
 	memset(&ecbdata, 0, sizeof(ecbdata));
 	ecbdata.color_diff = want_color(o->use_color);
 	ecbdata.found_changesp = &o->found_changes;
-	ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
+	ecbdata.ws_rule = whitespace_rule(name_b);
 	ecbdata.opt = o;
 	if (ecbdata.ws_rule & WS_BLANK_AT_EOF) {
 		mmfile_t mf1, mf2;
@@ -2372,7 +2372,7 @@ static void builtin_diff(const char *name_a,
 		ecbdata.label_path = lbl;
 		ecbdata.color_diff = want_color(o->use_color);
 		ecbdata.found_changesp = &o->found_changes;
-		ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
+		ecbdata.ws_rule = whitespace_rule(name_b);
 		if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
 			check_blank_at_eof(&mf1, &mf2, &ecbdata);
 		ecbdata.opt = o;
-- 
1.8.4.rc1.25.gd121ba2

Re: [PATCH] diff: remove ternary operator evaluating always to true

[Jeff King]

On Thu, Aug 08, 2013 at 08:31:44PM +0200, Stefan Beller wrote:

> The next occurrences are at:
> 	/* Never use a non-valid filename anywhere if at all possible */
> 	name_a = DIFF_FILE_VALID(one) ? name_a : name_b;
> 	name_b = DIFF_FILE_VALID(two) ? name_b : name_a;
> 
> 	a_one = quote_two(a_prefix, name_a + (*name_a == '/'));
> 	b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
> 
> In the last line of this block 'name_b' is dereferenced and compared
> to '/'. This would crash if name_b was NULL. Hence in the following code
> we can assume name_b being non-null.

I think your change is correct, but I find the reasoning above a little
suspect. It assumes that the second chunk of code (accessing name_a and
name_b) is correct, and pins the correctness of the code you are
changing to it. If the second chunk is buggy, then you are actually
making the code worse.

The interesting part is the top chunk, which aliases the names if one of
them is NULL. And there is an implicit assumption there that we will
never get _two_ NULL names in this function. And that is why the second
chunk does not ever crash (and why your change is the right thing to
do).

Sorry if this seems nit-picky. It is just that seemingly trivial
cleanups can sometimes end up revealing larger bugs, and I think it is
worth making sure we understand the root cause to be sure that our
cleanup really is trivial.

I wonder if the implicit expectation of the function to take at least
one non-NULL name would be more obvious if the first few lines were
written as:

  if (DIFF_FILE_VALID(one)) {
          if (!DIFF_FILE_VALID(two))
                  name_b = name_a;
  } else if (DIFF_FILE_VALID(two))
          name_a = name_b;
  else
          die("BUG: two invalid files to diff");

That covers all of the cases explicitly, though it is IMHO uglier to
read (and there is still an implicit assumption that the name is
non-NULL if DIFF_FILE_VALID() is true).

-Peff

Re: [PATCH] diff: remove ternary operator evaluating always to true

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 08, 2013 at 08:31:44PM +0200, Stefan Beller wrote:
>
>> The next occurrences are at:
>> 	/* Never use a non-valid filename anywhere if at all possible */
>> 	name_a = DIFF_FILE_VALID(one) ? name_a : name_b;
>> 	name_b = DIFF_FILE_VALID(two) ? name_b : name_a;
>> 
>> 	a_one = quote_two(a_prefix, name_a + (*name_a == '/'));
>> 	b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
>> 
>> In the last line of this block 'name_b' is dereferenced and compared
>> to '/'. This would crash if name_b was NULL. Hence in the following code
>> we can assume name_b being non-null.
>
> I think your change is correct, but I find the reasoning above a little
> suspect. It assumes that the second chunk of code (accessing name_a and
> name_b) is correct, and pins the correctness of the code you are
> changing to it. If the second chunk is buggy, then you are actually
> making the code worse.

True.  I think the original code structure design is name_a should
always exist but name_b may not (the caller of run_diff_cmd() that
eventually calls this call these "name" and "other", and the intent
is renaming filepair is what needs "other").

> I wonder if the implicit expectation of the function to take at least
> one non-NULL name would be more obvious if the first few lines were
> written as:
>
>   if (DIFF_FILE_VALID(one)) {
>           if (!DIFF_FILE_VALID(two))
>                   name_b = name_a;
>   } else if (DIFF_FILE_VALID(two))
>           name_a = name_b;
>   else
>           die("BUG: two invalid files to diff");
>
> That covers all of the cases explicitly, though it is IMHO uglier to
> read (and there is still an implicit assumption that the name is
> non-NULL if DIFF_FILE_VALID() is true).

I think that is an overall improvement, especially if we also update
the checks of {one,two}->mode made for the block that deals with
submodules to use DIFF_FILE_VALID().

Thanks.

Re: [PATCH] diff: remove ternary operator evaluating always to true

[Stefan Beller]

[-- Attachment #1: Type: text/plain, Size: 4270 bytes --]

On 08/12/2013 07:46 AM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Thu, Aug 08, 2013 at 08:31:44PM +0200, Stefan Beller wrote:
>>
>>> The next occurrences are at:
>>> 	/* Never use a non-valid filename anywhere if at all possible */
>>> 	name_a = DIFF_FILE_VALID(one) ? name_a : name_b;
>>> 	name_b = DIFF_FILE_VALID(two) ? name_b : name_a;
>>>
>>> 	a_one = quote_two(a_prefix, name_a + (*name_a == '/'));
>>> 	b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
>>>
>>> In the last line of this block 'name_b' is dereferenced and compared
>>> to '/'. This would crash if name_b was NULL. Hence in the following code
>>> we can assume name_b being non-null.
>>
>> I think your change is correct, but I find the reasoning above a little
>> suspect. It assumes that the second chunk of code (accessing name_a and
>> name_b) is correct, and pins the correctness of the code you are
>> changing to it. If the second chunk is buggy, then you are actually
>> making the code worse.
> 
> True.  I think the original code structure design is name_a should
> always exist but name_b may not (the caller of run_diff_cmd() that
> eventually calls this call these "name" and "other", and the intent
> is renaming filepair is what needs "other").
> 
>> I wonder if the implicit expectation of the function to take at least
>> one non-NULL name would be more obvious if the first few lines were
>> written as:
>>
>>   if (DIFF_FILE_VALID(one)) {
>>           if (!DIFF_FILE_VALID(two))
>>                   name_b = name_a;
>>   } else if (DIFF_FILE_VALID(two))
>>           name_a = name_b;
>>   else
>>           die("BUG: two invalid files to diff");
>>
>> That covers all of the cases explicitly, though it is IMHO uglier to
>> read (and there is still an implicit assumption that the name is
>> non-NULL if DIFF_FILE_VALID() is true).
> 
> I think that is an overall improvement, especially if we also update
> the checks of {one,two}->mode made for the block that deals with
> submodules to use DIFF_FILE_VALID().
> 
> Thanks.
> 

So, do I understand your reasoning, when proposing this patch?
(This may break whitespaces as it's copied into my MUA, will resend with
git send-mail if you think this is the right thing.)

This patch just covers your discussion and not the previous patches.

Stefan

--8<--
From 701bab4f15598ba230552af7f1d5719187f1b2e8 Mon Sep 17 00:00:00 2001
From: Stefan Beller <stefanbeller@googlemail.com>
Date: Mon, 12 Aug 2013 10:29:07 +0200
Subject: [PATCH] diff: Additional error checking for input parameters

This makes the diff function error out instead of segfaulting if the
parameters are bad.

Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 diff.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index e53ddad..de21971 100644
--- a/diff.c
+++ b/diff.c
@@ -2254,8 +2254,11 @@ static void builtin_diff(const char *name_a,
 			(!two->mode || S_ISGITLINK(two->mode))) {
 		const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
 		const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
-		show_submodule_summary(o->file, one ? one->path : two->path,
-				line_prefix,
+		struct diff_filespec *spec = one && DIFF_FILE_VALID(one) ? one : two;
+		if (!spec && !DIFF_FILE_VALID(spec))
+			die("BUG: two invalid diff_filespec structs in diff");
+
+		show_submodule_summary(o->file, spec->path, line_prefix,
 				one->sha1, two->sha1, two->dirty_submodule,
 				meta, del, add, reset);
 		return;
@@ -2276,8 +2279,13 @@ static void builtin_diff(const char *name_a,
 	}
 
 	/* Never use a non-valid filename anywhere if at all possible */
-	name_a = DIFF_FILE_VALID(one) ? name_a : name_b;
-	name_b = DIFF_FILE_VALID(two) ? name_b : name_a;
+	if (DIFF_FILE_VALID(one)) {
+		if (!DIFF_FILE_VALID(two))
+			name_b = name_a;
+	} else if (DIFF_FILE_VALID(two))
+		name_a = name_b;
+	else
+		die("BUG: two invalid files to diff");
 
 	a_one = quote_two(a_prefix, name_a + (*name_a == '/'));
 	b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
-- 
1.8.4.rc2



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: [PATCH] diff: remove ternary operator evaluating always to true

[Stefan Beller]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

On 08/12/2013 10:32 AM, Stefan Beller wrote:
> 
> diff --git a/diff.c b/diff.c
> index e53ddad..de21971 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2254,8 +2254,11 @@ static void builtin_diff(const char *name_a,
>  			(!two->mode || S_ISGITLINK(two->mode))) {
>  		const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
>  		const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
> -		show_submodule_summary(o->file, one ? one->path : two->path,
> -				line_prefix,
> +		struct diff_filespec *spec = one && DIFF_FILE_VALID(one) ? one : two;
> +		if (!spec && !DIFF_FILE_VALID(spec))
> +			die("BUG: two invalid diff_filespec structs in diff");
> +
> +		show_submodule_summary(o->file, spec->path, line_prefix,
>  				one->sha1, two->sha1, two->dirty_submodule,
>  				meta, del, add, reset);
>  		return;

This doesn't make sense, as we're definitely dereferencing both 
one and two with ->sha1, so we do not need the "one &&" and the 
"!spec &&". 
I think the originally sent patches are correct, but the commit 
message may need rewriting to comply with Jeffs nitpicking.




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: [PATCH] diff: remove ternary operator evaluating always to true

[Junio C Hamano]

Stefan Beller <stefanbeller@googlemail.com> writes:

> On 08/12/2013 07:46 AM, Junio C Hamano wrote:
>> I think that is an overall improvement, especially if we also update
>> the checks of {one,two}->mode made for the block that deals with
>> submodules to use DIFF_FILE_VALID().
>> 
>> Thanks.
>> 
>
> So, do I understand your reasoning, when proposing this patch?

I meant the condition in the "if" statement, not the parameters to
show_submodule_summary().  There are checks on ->mode fields that
bypass the DIFF_FILE_VALID() macro, hurting readability.