Re: [PATCH v2 16/21] Make sha1_array_append take a struct object_id *

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 28, 2017, 17:27 UTC
Message-ID: <xmqq7f39e2gi.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170326160143.769630-17-sandals@crustytoothpaste.net>

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Show 14 quoted lines

> Convert the callers to pass struct object_id by changing the function
> declaration and definition and applying the following semantic patch:
>
> @@
> expression E1, E2, E3;
> @@
> - sha1_array_append(E1, E2[E3].hash)
> + sha1_array_append(E1, E2 + E3)
>
> @@
> expression E1, E2;
> @@
> - sha1_array_append(E1, E2.hash)
> + sha1_array_append(E1, &E2)

I noticed something similar in the change to bisect.c while reading the previous step, and I suspect that the above two rules leave somewhat inconsistent and harder-to-read result. Wouldn't it make the result more readable if the former rule were

    -sha1_array_append(E1, E2[E3].hash)
    +sha1_array_append(E1, &E2[E3])

FWIW, the bit that made me read it twice in the previous step was this change

- strbuf_addstr(&joined_hexs, sha1_to_hex(array->sha1[i])); + strbuf_addstr(&joined_hexs, oid_to_hex(array->oid + i));

which I would have written &(array->oid[i]) instead.

After all, the original written by a human said E2[E3].hash (or array->sha1[i]) because to the human's mind, E2 is a series of things that can be indexed with an int E3, and even though

    *(E2 + E3)
    E2[E3]
    E3[E2]

all mean the same thing, the human decided that E2[E3] is the most natural way to express this particular reference to an item in the array. &E2[E3] would keep that intention by the original author better than E2 + E3.

The above comment does not affect the correctness of the conversion, but I think it would affect the readability of the resulting code.