Re: [PATCH v3 5/7] builtin: patch-id: add --include-whitespace as a command mode

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 17, 2022, 15:38 UTC
Message-ID: <xmqq7d0y1mw7.fsf@gitster.g>
In-Reply-To: <xmqqbkqe6qv4.fsf@gitster.g>

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

Show 12 quoted lines

> "Jerry Zhang via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +--include-whitespace::
>> +	Use the "stable" algorithm described below and also don't strip whitespace
>> +	from lines when calculating the patch-id.
>> +
>> +	This is the default if patchid.includeWhitespace is true and implies
>> +	patchid.stable.
>
> This seems very much orthogonal to "--stable/--unstable.  
>
> Because the "--stable" variant is more expensive than "--unstable",

Sorry, I misspoke. The way we make the result stable is *not* by enforcing a fixed order to the input of the hash (which would have been more expensive), but by hashing each file separately and summing up the hashes, and it shouldn't be noticeably more expensive than the unstable variant.

So, I do not think I mind if we introduced --include-whitespace as a third option in addition to --stable and --unstable, instead of allowing it to be combined with both --stable and --unstable.

But I wonder:

 * (minor) Would "--verbatim" work as a better option name?  The
   name "--include-whitespace" can apply even to an implementation
   that squashes multiple consecutive spaces and tabs into a single
   space, i.e. we keep words on a single line as separate words,
   instead of squishing them together, when hashing.

 * Do users even care about the internal reliance on the "stable"
   algorithm?  Wouldn't it be better to leave such an implementation
   detail unsaid?  After all, "--verbatim --unstable" would not work
   as they expect.

So I would suggest dropping "and implies patchid.stable" from the above description.