| Message ID | 20240621231826.3280338-4-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tighten patch header parsing in patch-id | expand |
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
Patrick Steinhardt <ps@pks.im> writes: >> 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. > 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.
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 + static int get_one_patchid(struct object_id *next_oid, struct object_id *result, - struct strbuf *line_buf, int stable, int verbatim) + struct strbuf *line_buf, unsigned flags) { + int stable = flags & GOPID_STABLE; + int verbatim = flags & GOPID_VERBATIM; int patchlen = 0, found_next = 0; int before = -1, after = -1; int diff_is_binary = 0; @@ -171,7 +176,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result, return patchlen; } -static void generate_id_list(int stable, int verbatim) +static void generate_id_list(unsigned flags) { struct object_id oid, n, result; int patchlen; @@ -179,7 +184,7 @@ static void generate_id_list(int stable, int verbatim) oidclr(&oid); while (!feof(stdin)) { - patchlen = get_one_patchid(&n, &result, &line_buf, stable, verbatim); + patchlen = get_one_patchid(&n, &result, &line_buf, flags); if (patchlen) flush_current_id(&oid, &result); oidcpy(&oid, &n); @@ -218,6 +223,7 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix) /* if nothing is set, default to unstable */ struct patch_id_opts config = {0, 0}; int opts = 0; + unsigned flags = 0; struct option builtin_patch_id_options[] = { OPT_CMDMODE(0, "unstable", &opts, N_("use the unstable patch-id algorithm"), 1), @@ -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; + generate_id_list(flags); + return 0; }
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(-)