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

From: Jeff King <peff@peff.net>
Date: Aug 10, 2013, 07:21 UTC
Message-ID: <20130810072114.GD30185@sigill.intra.peff.net>
In-Reply-To: <1375986704-11441-1-git-send-email-stefanbeller@googlemail.com>

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