| Message ID | bed2a8a07696371e07c0b2d1282ed51c0b1b9fee.1698314128.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | show-ref: introduce mode to check for ref existence | expand |
On Thu, Oct 26, 2023 at 11:56:37AM +0200, Patrick Steinhardt wrote: > It's not immediately obvious options which options are applicable to > what subcommand in 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. Consequently, 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> All makes sense, but... > @@ -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; ...do we need an .enabled here? I think checking whether or not .pattern is NULL is sufficient, but perhaps there is another use of .enabled later on in the series... Thanks, Taylor
On Thu, Oct 26, 2023 at 11:56:37AM +0200, Patrick Steinhardt wrote: > @@ -95,6 +94,11 @@ static int add_existing(const char *refname, > return 0; > } > > +struct exclude_existing_options { > + int enabled; > + const char *pattern; > +}; > + Thinking on my earlier suggestion more, I wondered if using the OPT_SUBCOMMAND() function might make things easier to organize and eliminate the need for things like .enabled or having to define structs for each of the sub-commands. But I don't think that this is (easily) possible to do, since `--exclude-existing` is behind a command-line option, not a separate mode (e.g. "commit-graph verify", not "commit-graph --verify"). So I think you *could* make it work with some combination of OPT_SUBCOMMAND and callbacks to set the function pointer yourself when given the `--exclude-existing` option. But I think that's sufficiently gross as to not be worth it. Thanks, Taylor
On Mon, Oct 30, 2023 at 02:37:14PM -0400, Taylor Blau wrote: > On Thu, Oct 26, 2023 at 11:56:37AM +0200, Patrick Steinhardt wrote: > > It's not immediately obvious options which options are applicable to > > what subcommand in 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. Consequently, 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> > > All makes sense, but... > > > @@ -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; > > ...do we need an .enabled here? I think checking whether or not .pattern > is NULL is sufficient, but perhaps there is another use of .enabled > later on in the series... This is the second time that this question comes up, which is likely not all that surprising. Quoting my first reply: On Wed, Oct 25, 2023 at 01:50:44PM +0200, Patrick Steinhardt wrote: > 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. Anyway, the fact that this question comes up again indicates that I need to comment this better. Patrick
On Mon, Oct 30, 2023 at 02:55:21PM -0400, Taylor Blau wrote: > On Thu, Oct 26, 2023 at 11:56:37AM +0200, Patrick Steinhardt wrote: > > @@ -95,6 +94,11 @@ static int add_existing(const char *refname, > > return 0; > > } > > > > +struct exclude_existing_options { > > + int enabled; > > + const char *pattern; > > +}; > > + > > Thinking on my earlier suggestion more, I wondered if using the > OPT_SUBCOMMAND() function might make things easier to organize and > eliminate the need for things like .enabled or having to define structs > for each of the sub-commands. > > But I don't think that this is (easily) possible to do, since > `--exclude-existing` is behind a command-line option, not a separate > mode (e.g. "commit-graph verify", not "commit-graph --verify"). So I > think you *could* make it work with some combination of OPT_SUBCOMMAND > and callbacks to set the function pointer yourself when given the > `--exclude-existing` option. But I think that's sufficiently gross as to > not be worth it. Yeah, agreed. Honestly, while working on this series I had the dream of just discarding git-show-ref(1) in favor of a new command with proper subcommands as we tend to use them nowadays: - `git references list <patterns>...` replaces `git show-ref <pattern>`. - `git references filter <pattern>` replaces `git show-ref --exclude-existing` and filters references from stdin. - `git references exists <ref>` checks whether a reference exists or not and replaces `git show-ref --exists`. This would make for a much more enjoyable UX. It'd also be a more natural home for potential future additions: - `git references show <ref>` allows you to show the contents of the reference without resolving it, regardless of whether it's a direct or a symbolic reference. - `git references count <patterns>...` allows you to count refs patching the pattern. I shied away though because it would be a much more controversial topic that would potentially result in lots of bikeshedding. Now if everyone was enthusiastic about this idea I'd still be happy to do it, even though it derails the topic even further from its original intent to just fix a bunch of tests. But unless that happens, I'll continue to stick with the mediocre UI we have in git-show-ref(1). Patrick
diff --git a/builtin/show-ref.c b/builtin/show-ref.c index f95418d3d16..90481c58492 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 patternlen = 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) + if (reflen < patternlen) continue; - if (strncmp(ref, match, matchlen)) + if (strncmp(ref, opts->pattern, patternlen)) continue; } if (check_refname_format(ref, 0)) { @@ -201,44 +205,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 in 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. Consequently, 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 | 74 +++++++++++++++++++++++++--------------------- 1 file changed, 40 insertions(+), 34 deletions(-)