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

[brian m. carlson]

On Wed, Mar 29, 2017 at 08:14:19AM -0700, Junio C Hamano wrote:
> This change is about dropping the need for ".hash", and I think a
> faithful, boring and mechanical conversion that tries to preserve
> the intent of the original author would be more appropriate.  It is
> entirely possible that some places where the original said E2[E3]
> were easier to understand if it were *(E2 + E3), thus we may want to
> further rewrite such a place to (E2 + E3) instead of [E3] after
> the mechanical conversion.

You've convinced me that [E3] is a better choice in this situation,
so I'll reroll and fix the other issues mentioned.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

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

[Junio C Hamano]

"brian m. carlson"  writes:

> On Tue, Mar 28, 2017 at 10:27:41AM -0700, Junio C Hamano wrote:
>> ... 
>> 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.  [E3] would keep that intention by the original author
>> better than E2 + E3.
>
> I'm happy to make that change.  I'm an experienced C programmer, so a
> pointer addition seems very readable and natural to me, but if you think
> it's better or more readable as [E3], I can certainly reroll.

The obvious three possible variants I listed above are equivalent to
the language lawyers and the compiler ;-).

This change is about dropping the need for ".hash", and I think a
faithful, boring and mechanical conversion that tries to preserve
the intent of the original author would be more appropriate.  It is
entirely possible that some places where the original said E2[E3]
were easier to understand if it were *(E2 + E3), thus we may want to
further rewrite such a place to (E2 + E3) instead of [E3] after
the mechanical conversion.

Thanks.

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

[brian m. carlson]

On Tue, Mar 28, 2017 at 10:27:41AM -0700, Junio C Hamano wrote:
> "brian m. carlson"  writes:
> 
> > 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, )
> 
> 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, [E3])
> 
> 
> FWIW, the bit that made me read it twice in the previous step was
> this change
> 
> - strbuf_addstr(_hexs, sha1_to_hex(array->sha1[i]));
> + strbuf_addstr(_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.  [E3] would keep that intention by the original author
> better than E2 + E3.

I'm happy to make that change.  I'm an experienced C programmer, so a
pointer addition seems very readable and natural to me, but if you think
it's better or more readable as [E3], I can certainly reroll.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

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

[Junio C Hamano]

"brian m. carlson"  writes:

> 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, )

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, [E3])


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

-   strbuf_addstr(_hexs, sha1_to_hex(array->sha1[i]));
+   strbuf_addstr(_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.  [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.

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

[Jeff King]

On Sun, Mar 26, 2017 at 04:01:38PM +, brian m. carlson wrote:

> diff --git a/transport.c b/transport.c
> index 8a90b0c29b..e492757726 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1027,7 +1027,8 @@ int transport_push(struct transport *transport,
>  
>   for (; ref; ref = ref->next)
>   if (!is_null_oid(>new_oid))
> - sha1_array_append(, 
> ref->new_oid.hash);
> + sha1_array_append(,
> +   >new_oid);

Funny that this line wrapped when it got shorter. :)

I think wrapping is the right thing, though (it is longer than 80).

-Peff