Re: [PATCH v3 5/7] builtin: patch-id: add --include-whitespace as a command mode
On Mon, Oct 17, 2022 at 8:38 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 41 quoted lines
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "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.
Sure I can spin a new series with these changes
>
>