Re: [PATCH 3/5] patch-id: make get_one_patchid() more extensible
From: Patrick Steinhardt <hidden>
Date: 2024-07-29 12:02:50
On Fri, Jun 21, 2024 at 04:18:24PM -0700, Junio C Hamano wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.
quoted hunk ↗ jump to hunk
@@ -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
Attachments
- signature.asc [application/pgp-signature] 833 bytes