Re: [PATCH 2/2] format-patch: add an option to suppress commit hash

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 7, 2015, 19:47 UTC
Message-ID: <xmqqmvtmm46w.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqr3iym4rg.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

>> +--no-hash::
>> +  Output an all-zero hash in each patch's From header instead
>> +  of the hash of the commit.
>> +
>
> Two (big) problems with the option name.
>
>  - "--no-something" would mislead people to think you are removing
>    something, not replacing it with something else.  This option
>    does the latter (i.e. the first line of your output still has
>    40-hex; it's just it no longer has a useful 40-hex).
>
>  - There are many places we use hexadecimal strings in format-patch
>    output and you are not removing or replacing all of them, only
>    the commit object name on the fake "From " line.  Saying "hash"
>    would mislead readers.

I am not good at bikeshedding, but you used 'zero_commit' elsewhere in the code. I think that would be a much better name--perhaps use that consistently throughout, as the local variable in options[] array and end-user facing option name?

Show 16 quoted lines

>
>> +test_expect_success 'format-patch --no-hash' '
>> +	git format-patch --no-hash --stdout v2..v1 >patch2 &&
>> +	cnt=$(egrep "^From 0+ Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
>
> Don't test "any number of '0'"; test 40 '0's.  This is because the
> line format was designed to be usable by things like /etc/magic to
> detect format-patch output, and we want to notice if/when we break
> that aspect of our output format.
>
>> +	test $cnt = 3
>> +'
>> +
>>  test_done
>
> Thanks.