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

From: Patrick Steinhardt <ps@pks.im>
Date: Jul 29, 2024, 12:02 UTC
Message-ID: <ZqeE2IaIhiWmpVM4@tanuki>
In-Reply-To: <20240621231826.3280338-4-gitster@pobox.com>

On Fri, Jun 21, 2024 at 04:18:24PM -0700, Junio C Hamano wrote:

Show 24 quoted lines

> 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.

Show 10 quoted lines

> @@ -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