| Message ID | 8d0b0b5700c7ffed6b3a74760d0d9155b404bb4f.1698152926.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | show-ref: introduce mode to check for ref existence | expand |
On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote: > It's not immediately obvious options which options are applicable to > what subcommand ni git-show-ref(1) because all options exist as global s/ni/in/ > state. This can easily cause confusion for the reader. > > Refactor options for the `--exclude-existing` subcommand to be contained > in a separate structure. This structure is stored on the stack and > passed down as required. Consequentially, it clearly delimits the scope s/Consequentially/Consequently/ > of those options and requires the reader to worry less about global > state. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > diff --git a/builtin/show-ref.c b/builtin/show-ref.c > @@ -95,6 +94,11 @@ static int add_existing(const char *refname, > +struct exclude_existing_options { > + int enabled; > + const char *pattern; > +}; Do we need this `enabled` flag? Can't the same be achieved by checking whether `pattern` is NULL or not (see below)? > @@ -104,11 +108,11 @@ static int add_existing(const char *refname, > -static int cmd_show_ref__exclude_existing(const char *match) > +static int cmd_show_ref__exclude_existing(const struct exclude_existing_options *opts) Since you're renaming `match` to `opts->pattern`... > { > - int matchlen = match ? strlen(match) : 0; > + int matchlen = opts->pattern ? strlen(opts->pattern) : 0; ... and since you're touching this line anyway, maybe it makes sense to rename `matchlen` to `patternlen`? > @@ -124,11 +128,11 @@ static int cmd_show_ref__exclude_existing(const char *match) > - if (strncmp(ref, match, matchlen)) > + if (strncmp(ref, opts->pattern, matchlen)) Especially since, as shown in this context, `matchlen` is really the length of the _pattern_, not the length of the resulting _match_. > @@ -200,44 +204,46 @@ static int hash_callback(const struct option *opt, const char *arg, int unset) > int cmd_show_ref(int argc, const char **argv, const char *prefix) > { > [...] > - if (exclude_arg) > - return cmd_show_ref__exclude_existing(exclude_existing_arg); > + if (exclude_existing_opts.enabled) > + return cmd_show_ref__exclude_existing(&exclude_existing_opts); (continued from above) Can't this be handled without a separate `enabled` flag? if (exclude_existing_opts.pattern) ...
On Tue, Oct 24, 2023 at 02:48:14PM -0400, Eric Sunshine wrote: > On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote: > > It's not immediately obvious options which options are applicable to > > what subcommand ni git-show-ref(1) because all options exist as global > > s/ni/in/ > > > state. This can easily cause confusion for the reader. > > > > Refactor options for the `--exclude-existing` subcommand to be contained > > in a separate structure. This structure is stored on the stack and > > passed down as required. Consequentially, it clearly delimits the scope > > s/Consequentially/Consequently/ > > > of those options and requires the reader to worry less about global > > state. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > diff --git a/builtin/show-ref.c b/builtin/show-ref.c > > @@ -95,6 +94,11 @@ static int add_existing(const char *refname, > > +struct exclude_existing_options { > > + int enabled; > > + const char *pattern; > > +}; > > Do we need this `enabled` flag? Can't the same be achieved by checking > whether `pattern` is NULL or not (see below)? Yeah, we do. It's perfectly valid to pass `--exclude-existing` without the optional pattern argument. We still want to use this mode in that case, but don't populate the pattern. An alternative would be to assign something like a sentinel value in here. But I'd think that it's clearer to instead have an explicit separate field for this. > > @@ -104,11 +108,11 @@ static int add_existing(const char *refname, > > -static int cmd_show_ref__exclude_existing(const char *match) > > +static int cmd_show_ref__exclude_existing(const struct exclude_existing_options *opts) > > Since you're renaming `match` to `opts->pattern`... > > > { > > - int matchlen = match ? strlen(match) : 0; > > + int matchlen = opts->pattern ? strlen(opts->pattern) : 0; > > ... and since you're touching this line anyway, maybe it makes sense > to rename `matchlen` to `patternlen`? Yes, let's do it. It's been more of an oversight rather than intentional to keep the previous name. > > @@ -124,11 +128,11 @@ static int cmd_show_ref__exclude_existing(const char *match) > > - if (strncmp(ref, match, matchlen)) > > + if (strncmp(ref, opts->pattern, matchlen)) > > Especially since, as shown in this context, `matchlen` is really the > length of the _pattern_, not the length of the resulting _match_. > > > @@ -200,44 +204,46 @@ static int hash_callback(const struct option *opt, const char *arg, int unset) > > int cmd_show_ref(int argc, const char **argv, const char *prefix) > > { > > [...] > > - if (exclude_arg) > > - return cmd_show_ref__exclude_existing(exclude_existing_arg); > > + if (exclude_existing_opts.enabled) > > + return cmd_show_ref__exclude_existing(&exclude_existing_opts); > > (continued from above) Can't this be handled without a separate `enabled` flag? > > if (exclude_existing_opts.pattern) > ... See the explanation above. Patrick
diff --git a/builtin/show-ref.c b/builtin/show-ref.c index eb60f940a3c..e130b999c0b 100644 --- a/builtin/show-ref.c +++ b/builtin/show-ref.c @@ -19,8 +19,7 @@ static const char * const show_ref_usage[] = { }; static int deref_tags, show_head, tags_only, heads_only, found_match, verify, - quiet, hash_only, abbrev, exclude_arg; -static const char *exclude_existing_arg; + quiet, hash_only, abbrev; static void show_one(const char *refname, const struct object_id *oid) { @@ -95,6 +94,11 @@ static int add_existing(const char *refname, return 0; } +struct exclude_existing_options { + int enabled; + const char *pattern; +}; + /* * read "^(?:<anything>\s)?<refname>(?:\^\{\})?$" from the standard input, * and @@ -104,11 +108,11 @@ static int add_existing(const char *refname, * (4) ignore if refname is a ref that exists in the local repository; * (5) otherwise output the line. */ -static int cmd_show_ref__exclude_existing(const char *match) +static int cmd_show_ref__exclude_existing(const struct exclude_existing_options *opts) { struct string_list existing_refs = STRING_LIST_INIT_DUP; char buf[1024]; - int matchlen = match ? strlen(match) : 0; + int matchlen = opts->pattern ? strlen(opts->pattern) : 0; for_each_ref(add_existing, &existing_refs); while (fgets(buf, sizeof(buf), stdin)) { @@ -124,11 +128,11 @@ static int cmd_show_ref__exclude_existing(const char *match) for (ref = buf + len; buf < ref; ref--) if (isspace(ref[-1])) break; - if (match) { + if (opts->pattern) { int reflen = buf + len - ref; if (reflen < matchlen) continue; - if (strncmp(ref, match, matchlen)) + if (strncmp(ref, opts->pattern, matchlen)) continue; } if (check_refname_format(ref, 0)) { @@ -200,44 +204,46 @@ static int hash_callback(const struct option *opt, const char *arg, int unset) static int exclude_existing_callback(const struct option *opt, const char *arg, int unset) { + struct exclude_existing_options *opts = opt->value; BUG_ON_OPT_NEG(unset); - exclude_arg = 1; - *(const char **)opt->value = arg; + opts->enabled = 1; + opts->pattern = arg; return 0; } -static const struct option show_ref_options[] = { - OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")), - OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")), - OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, " - "requires exact ref path")), - OPT_HIDDEN_BOOL('h', NULL, &show_head, - N_("show the HEAD reference, even if it would be filtered out")), - OPT_BOOL(0, "head", &show_head, - N_("show the HEAD reference, even if it would be filtered out")), - OPT_BOOL('d', "dereference", &deref_tags, - N_("dereference tags into object IDs")), - OPT_CALLBACK_F('s', "hash", &abbrev, N_("n"), - N_("only show SHA1 hash using <n> digits"), - PARSE_OPT_OPTARG, &hash_callback), - OPT__ABBREV(&abbrev), - OPT__QUIET(&quiet, - N_("do not print results to stdout (useful with --verify)")), - OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_arg, - N_("pattern"), N_("show refs from stdin that aren't in local repository"), - PARSE_OPT_OPTARG | PARSE_OPT_NONEG, exclude_existing_callback), - OPT_END() -}; - int cmd_show_ref(int argc, const char **argv, const char *prefix) { + struct exclude_existing_options exclude_existing_opts = {0}; + const struct option show_ref_options[] = { + OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")), + OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")), + OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, " + "requires exact ref path")), + OPT_HIDDEN_BOOL('h', NULL, &show_head, + N_("show the HEAD reference, even if it would be filtered out")), + OPT_BOOL(0, "head", &show_head, + N_("show the HEAD reference, even if it would be filtered out")), + OPT_BOOL('d', "dereference", &deref_tags, + N_("dereference tags into object IDs")), + OPT_CALLBACK_F('s', "hash", &abbrev, N_("n"), + N_("only show SHA1 hash using <n> digits"), + PARSE_OPT_OPTARG, &hash_callback), + OPT__ABBREV(&abbrev), + OPT__QUIET(&quiet, + N_("do not print results to stdout (useful with --verify)")), + OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_opts, + N_("pattern"), N_("show refs from stdin that aren't in local repository"), + PARSE_OPT_OPTARG | PARSE_OPT_NONEG, exclude_existing_callback), + OPT_END() + }; + git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix, show_ref_options, show_ref_usage, 0); - if (exclude_arg) - return cmd_show_ref__exclude_existing(exclude_existing_arg); + if (exclude_existing_opts.enabled) + return cmd_show_ref__exclude_existing(&exclude_existing_opts); else if (verify) return cmd_show_ref__verify(argv); else
It's not immediately obvious options which options are applicable to what subcommand ni git-show-ref(1) because all options exist as global state. This can easily cause confusion for the reader. Refactor options for the `--exclude-existing` subcommand to be contained in a separate structure. This structure is stored on the stack and passed down as required. Consequentially, it clearly delimits the scope of those options and requires the reader to worry less about global state. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/show-ref.c | 72 +++++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 33 deletions(-)