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

[Jeff King <peff@peff.net>]

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