From: Siddharth Asthana Date: Mon, 20 Apr 2026 10:24:34 GMT Subject: Re: [PATCH v1 1/1] rev-list: add --missing=print-only mode Message-ID: <9e2ca91d-9091-4d4d-9427-ec8a23ee8909@gmail.com> In-Reply-To: <4c9fee0b-99bb-4e41-9227-f09c63df9f9d@gmail.com> On 20/04/26 04:06, Derrick Stolee wrote: > On 4/19/26 4:48 AM, Siddharth Asthana wrote: >> When working with partial clones, it's common to want just the list of >> missing objects. The current --missing=print mode does this but mixes >> present and missing objects together, with missing ones prefixed by '?'. >> Getting only the missing OIDs requires an extra pipe: >> >>    git rev-list --objects --all --missing=print | perl -ne 'print if >> s/^[?]//' >> >> Add --missing=print-only which outputs only the missing object OIDs, one >> per line, without any prefix. This makes the above one-liner unnecessary >> and the output directly usable by downstream tools. > > I'm a fan of this mode. It saves time dealing with the I/O. Thanks! > >>   static struct oidmap missing_objects; >>   enum missing_action { >> -    MA_ERROR = 0,    /* fail if any missing objects are encountered */ >> -    MA_ALLOW_ANY,    /* silently allow ALL missing objects */ >> -    MA_PRINT,        /* print ALL missing objects in special section */ >> -    MA_PRINT_INFO,   /* same as MA_PRINT but also prints missing >> object info */ >> +    MA_ERROR = 0, /* fail if any missing objects are encountered */ >> +    MA_ALLOW_ANY, /* silently allow ALL missing objects */ >> +    MA_PRINT, /* print ALL missing objects in special section */ >> +    MA_PRINT_INFO, /* same as MA_PRINT but also prints missing object >> info */ >> +    MA_PRINT_ONLY, /* print ONLY missing objects, without the "?" >> prefix */ >>       MA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR >> objects */ > > I'm not a fan of this adjustment of spacing on the comments, You are right, unnecessary churn - will restore! > though I do see that MA_ALLOW_PROMISOR had already broken the > pattern. MA_PRINT_ONLY would still work with the earlier > comment alignment, though. > >> -    if (line_term) >> +    if (arg_missing_action == MA_PRINT_ONLY) { >> +        printf("%s", oid_to_hex(&entry->entry.oid)); >> +        putchar(line_term); > > Is there a reason you didn't use a printf("%s%c") here to > put the oid and line_term together? I just followed the existing pattern in the same function where every other path uses a separate putchar(line_term). Happy to combine if you prefer. > >> +        return; >> +    } else if (line_term) { >>           printf("?%s", oid_to_hex(&entry->entry.oid)); >> -    else >> +    } else { >>           printf("%s%cmissing=yes", oid_to_hex(&entry->entry.oid), >>                  info_term); >> +    } > > > >>       if (!print_missing_info) { >>           putchar(line_term); >> @@ -209,6 +222,7 @@ static inline void finish_object__ma(struct object >> *obj, const char *name) >>       case MA_PRINT: >>       case MA_PRINT_INFO: >> +    case MA_PRINT_ONLY: >>           add_missing_object_entry(&obj->oid, name, obj->type); >>           return; >> @@ -246,6 +260,11 @@ static void show_commit(struct commit *commit, >> void *data) >>           return; >>       } >> +    if (arg_missing_action == MA_PRINT_ONLY) { >> +        finish_commit(commit); >> +        return; >> +    } >> + >>       if (show_disk_usage) >>           total_disk_usage += get_object_disk_usage(&commit->object); >> @@ -384,6 +403,8 @@ static void show_object(struct object *obj, const >> char *name, void *cb_data) >>       if (finish_object(obj, name, cb_data)) >>           return; >>       display_progress(progress, ++progress_counter); >> +    if (arg_missing_action == MA_PRINT_ONLY) >> +        return; >>       if (show_disk_usage) >>           total_disk_usage += get_object_disk_usage(obj); >>       if (info->flags & REV_LIST_QUIET) >> @@ -525,6 +546,12 @@ static inline int >> parse_missing_action_value(const char *value) >>           return 1; >>       } >> +    if (!strcmp(value, "print-only")) { >> +        arg_missing_action = MA_PRINT_ONLY; >> +        fetch_if_missing = 0; >> +        return 1; >> +    } >> + >>       if (!strcmp(value, "allow-promisor")) { >>           arg_missing_action = MA_ALLOW_PROMISOR; >>           fetch_if_missing = 0; >> @@ -967,8 +994,7 @@ int cmd_rev_list(int argc, >>       if (arg_print_omitted) >>           oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE); >> -    if (arg_missing_action == MA_PRINT || >> -        arg_missing_action == MA_PRINT_INFO) { >> +    if (missing_action_prints()) { >>           struct oidset_iter iter; >>           struct object_id *oid; >> @@ -994,8 +1020,7 @@ int cmd_rev_list(int argc, >>               printf("~%s\n", oid_to_hex(oid)); >>           oidset_clear(&omitted_objects); >>       } >> -    if (arg_missing_action == MA_PRINT || >> -        arg_missing_action == MA_PRINT_INFO) { >> +    if (missing_action_prints()) { >>           struct missing_objects_map_entry *entry; >>           struct oidmap_iter iter; >> @@ -1011,7 +1036,7 @@ int cmd_rev_list(int argc, >>       stop_progress(&progress); >> -    if (revs.count) { >> +    if (revs.count && arg_missing_action != MA_PRINT_ONLY) { >>           if (revs.left_right && revs.cherry_mark) >>               printf("%d\t%d\t%d\n", revs.count_left, >> revs.count_right, revs.count_same); >>           else if (revs.left_right) >> @@ -1022,7 +1047,7 @@ int cmd_rev_list(int argc, >>               printf("%d\n", revs.count_left + revs.count_right); >>       } >> -    if (show_disk_usage) >> +    if (show_disk_usage && arg_missing_action != MA_PRINT_ONLY) >>           print_disk_usage(total_disk_usage); > > I'm a little worried about all of these checks that need > special-casing. These seem like options that are enabled Phillip suggested making this a separate --missing-only flag that composes with existing --missing= modes - I think that's a better design. will work v2 around it. > by other options and could easily be grouped with the > print-only setting. > > If there is something wrong here where these need to be > disabled, then we should update the documentation to say Agreed - I will make --count and --disk-usage incompatible with --missing-only and die() with a clear message instead of silently suppressing them. > how this interacts with those options. And perhaps some > warnings to say "these options are not compatible". > > On that note: this patch is missing a document update. There is a doc update in rev-list-options.doc, but it doesn't cover interactions with --count/--disk-usage. Will fix in v2. > > Thanks, > -Stolee > > Thanks, Asthana