Re: [PATCH 02/12] builtin/show-ref: split up different subcommands
From: Eric Sunshine <hidden>
Date: 2023-10-24 17:55:51
On Tue, Oct 24, 2023 at 9:10 AM Patrick Steinhardt [off-list ref] wrote:
quoted hunk ↗ jump to hunk
While not immediately obvious, git-show-ref(1) actually implements three different subcommands: - `git show-ref <patterns>` can be used to list references that match a specific pattern. - `git show-ref --verify <refs>` can be used to list references. These are _not_ patterns. - `git show-ref --exclude-existing` can be used as a filter that reads references from standard input, performing some conversions on each of them. Let's make this more explicit in the code by splitting up the three subcommands into separate functions. This also allows us to address the confusingly named `patterns` variable, which may hold either patterns or reference names. Signed-off-by: Patrick Steinhardt <redacted> ---@@ -142,6 +142,53 @@ static int exclude_existing(const char *match) +static int cmd_show_ref__verify(const char **refs) +{ + if (!refs || !*refs) + die("--verify requires a reference"); + + while (*refs) { + struct object_id oid; + + if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) && + !read_ref(*refs, &oid)) { + show_one(*refs, &oid); + } + else if (!quiet) + die("'%s' - not a valid ref", *refs); + else + return 1; + refs++; + }
A couple style-nits here caught my attention... - "}" and "else" should be cuddled: `} else if` - coding guidelines these days want braces on all branches if any branch needs them However, since this code is merely being relocated from elsewhere in this file and since these style-nits were already present, moving the code verbatim without correcting the style problems is more reviewer-friendly. Okay.
+ return 0;
+}
+
+static int cmd_show_ref__patterns(const char **patterns)
+{
+ struct show_ref_data show_ref_data = {
+ .patterns = (patterns && *patterns) ? patterns : NULL,
+ };Are we allowing non-constant initializers in the codebase? If not, this should probably initialize .patterns to NULL and then conditionally assign `patterns` separately in code below the initializer.
+ if (show_head)
+ head_ref(show_ref, &show_ref_data);
+ if (heads_only || tags_only) {
+ if (heads_only)
+ for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
+ if (tags_only)
+ for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
+ } else {
+ for_each_ref(show_ref, &show_ref_data);
+ }
+ if (!found_match) {
+ if (verify && !quiet)
+ die("No match");
+ return 1;
+ }
+
+ return 0;
+}