From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 20 Apr 2026 11:44:44 GMT
Subject: Re: [PATCH v1 1/1] rev-list: add --missing=print-only mode
Message-ID: <8a3e3a38-3f8e-4363-9e45-b5610c3efbc5@gmail.com>
In-Reply-To: <9e2ca91d-9091-4d4d-9427-ec8a23ee8909@gmail.com>

On 4/20/2026 6:24 AM, Siddharth Asthana wrote:
> On 20/04/26 04:06, Derrick Stolee wrote:
>> On 4/19/26 4:48 AM, Siddharth Asthana wrote:

>>> -    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.

>>> +    } else {
>>>           printf("%s%cmissing=yes", oid_to_hex(&entry->entry.oid),
>>>                  info_term);

I was just looking at this line in the patch context as
comparison, but if there are other patterns that prefer
"printf(); putc();" then that's enough for me.

>>> -    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.

That's a good idea. '--missing=' formats the output while
your mode _filters_ the output. They are different things
and worth different options.

>> 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.

You're right. I thought I had looked for it in the patch
but missed it somehow.

Thanks,
-Stolee