Re: [PATCH v1 1/1] rev-list: add --missing=print-only mode

[Siddharth Asthana <siddharthasthana31@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