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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

On Fri, Jun 21, 2024 at 04:18:24PM -0700, Junio C Hamano wrote:
> We pass two independent Boolean flags (i.e. do we want the stable
> variant of patch-id?  do we want to hash the stuff verbatim?) into
> the function as two separate parameters.  Before adding the third
> one and make the interface even wider, let's consolidate them into
> a single flag word.
> 
> No changes in behaviour.  Just a trivial interface change.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/patch-id.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/patch-id.c b/builtin/patch-id.c
> index 0f262e7a03..128e0997d8 100644
> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -58,9 +58,14 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
>  	return 1;
>  }
>  
> +#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.

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.

> @@ -237,7 +243,11 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, builtin_patch_id_options,
>  			     patch_id_usage, 0);
>  
> -	generate_id_list(opts ? opts > 1 : config.stable,
> -			 opts ? opts == 3 : config.verbatim);
> +	if (opts ? opts > 1 : config.stable)
> +		flags |= GOPID_STABLE;
> +	if (opts ? opts == 3 : config.verbatim)
> +		flags |= GOPID_VERBATIM;

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.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]