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

From: Jerry Zhang <jerry@skydio.com>
Date: Oct 14, 2022, 22:55 UTC
Message-ID: <CAMKO5CuCbyFt739GOzcvFn92i8vNqK6vgJqvT8E5zs=kJ1+H=A@mail.gmail.com>
In-Reply-To: <xmqqbkqe6qv4.fsf@gitster.g>

On Fri, Oct 14, 2022 at 2:24 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 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",

I didn't realize it was more expensive, I'm assuming you mean in terms of time, maybe it does slightly more hashing operations under the hood? I tried timing some runs locally and they were a wash:

time /bin/sh -c "git show | git patch-id --stable" dddea79ee68d62a32cf8c0d7bb6691bcd0445628 4677fe858366a51ff3c5a0c0893418e32e934262

real 0m0.011s user 0m0.003s sys 0m0.012s time /bin/sh -c "git show | git patch-id" 6602a3b2fe8b17d5bc295c2703901ad3e18eee18 4677fe858366a51ff3c5a0c0893418e32e934262

real 0m0.012s user 0m0.009s sys 0m0.007s

The operation is probably bound by process / disk overhead quite a bit and a small amount of cpu use wouldn't really be user-visible. Based on these results I don't think a user would choose --unstable just for the speed gain (if any).

Show 7 quoted lines

> I am not sure why such an implication is a good thing to have.  Why
> can we not have
>
>     --include-whitespace --stable
>     --include-whitespace --unstable
>
> both combinations valid?

If you accept my point above, then a user would only choose "--unstable" if they actually had a need for backwards compatibility, such as for a persistent database. Trying to include whitespace on top of that would break the compatibility they're relying on. So my conclusion was that there isn't any usecase for the combination "--include-whitespace --unstable", and it's better for usability and not needing to always maintain compatibility if we don't expose it to users at all.