threads / patch / 34644

patchdiff: remove ternary operator evaluating always to true

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

## tl;dr

6 messages between Aug 8, 2013 and Aug 12, 2013. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Stefan Beller· Aug 8, 2013, 18:31 UTC · lore

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(-)
Show changes to diff.c +2 −2
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
Jeff King· Aug 10, 2013, 07:21 UTC · re: Stefan Beller · lore

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

On Thu, Aug 08, 2013 at 08:31:44PM +0200, Stefan Beller wrote:
Show 11 quoted lines
> 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
Junio C Hamano· Aug 12, 2013, 05:46 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> 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").

Show 15 quoted lines
> 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.
Stefan Beller· Aug 12, 2013, 08:32 UTC · re: Junio C Hamano · lore

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

On 08/12/2013 07:46 AM, Junio C Hamano wrote:
Show 49 quoted lines
> 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(-)
Show changes to diff.c +12 −4
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
Stefan Beller· Aug 12, 2013, 08:38 UTC · re: Stefan Beller · lore

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

On 08/12/2013 10:32 AM, Stefan Beller wrote:
Show 19 quoted lines
> 
> 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.

Junio C Hamano· Aug 12, 2013, 17:15 UTC · re: Stefan Beller · lore

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

Stefan Beller <stefanbeller@googlemail.com> writes:
Show 9 quoted lines
> 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.

← back to recent threads