Re: [PATCH 3/5] patch-id: make get_one_patchid() more extensible

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 29, 2024, 20:03 UTC
Message-ID: <xmqq34nshso8.fsf@gitster.g>
In-Reply-To: <ZqeE2IaIhiWmpVM4@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

Show 19 quoted lines

>> No changes in behaviour.  Just a trivial interface change.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ...
>> +#define GOPID_STABLE   01
>> +#define GOPID_VERBATIM 02
>> +
>
> This certainly is a worthwhile change. I have to wonder about code style
> though:
>
>   - Using 01 and 02 as constants feels somewhat weird to me. Don't we
>     typically use `(1 << 0)` and `(1 << 1)` for such binary flags?
>
>   - What is our preferred style nowadays? Do we prefer defines over
>     enums? I rather had the feeling that enums are the go-to style for
>     things like this nowadays.
>
> It would also be nice to have documentation for the flags.

For an internal implementation detail that does not even cross file boundaries with descriptive _STABLE and _VERBATIM that corresponds to the member names of config structure? I doubt it.

Show 9 quoted lines

> In any case, all of these are really just smallish nits and I think that
> this is a strict improvement regardless of whether we massage the style
> or not.
> ...
> I was wondering whether we could use `OPT_BIT()` here to set those as
> flags directly. I guess that would require a bit more refactoring, but
> if we also converted `struct patch_id_opts` to have a `flags` field then
> this might overall be easier to read than the weird massaging of opts
> that we did before and after your change.

As a longer direction, I envision that most of the implementation we see in this file and what diff.c:diff_get_patch_id() does should be refactored and one of them should just go. So until that happens, I am inclined to keep the changes to this file to an absolute minimum.