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@xxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
