Re: [PATCH v3] revision: add `--ignore-missing-links` user option

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 18, 2023, 15:56 UTC
Message-ID: <xmqqv8c7mq4o.fsf@gitster.g>
In-Reply-To: <CAOLa=ZQ9ZRmYe5b6R9ZTTpDCzb2L0UmxkeBujb2kOMSeFuwJGA@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 30 quoted lines

>> Given the above "we merely surface a feature that already exists and
>> supported to be used by the end users from the command line" claim ...
>>
>> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
>> > index ff715d6918..5239d83c76 100644
>> > --- a/builtin/rev-list.c
>> > +++ b/builtin/rev-list.c
>> > @@ -266,7 +266,8 @@ static int finish_object(struct object *obj, const char *name UNUSED,
>> >  {
>> >       struct rev_list_info *info = cb_data;
>> >       if (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {
>> > -             finish_object__ma(obj);
>> > +             if (!info->revs->ignore_missing_links)
>> > +                     finish_object__ma(obj);
>> >               return 1;
>> >       }
>>
>> ... this hunk is a bit unexpected.  As a low-level plumbing command,
>> shouldn't it be left to the user who gives --ignore-missing-links
>> from their command line to specify how the missing "obj" here should
>> be dealt with by giving the "--missing=<foo>" option?  While giving
>> "allow-promisor" may not make much sense, "--missing=allow-any" may
>> of course make sense (it is the same as hardcoding the decision not
>> to call finish_object__ma() at all), and so may "--missing=print".
>>
>
> This is to be expected, in my opinion. In terms of revision.c and
> setting the `revs->ignore_missing_links` bit, the traversal will
> go throw all objects (commits and otherwise) and call
> `show_commit` or `show_object` on them.

Yes. And the user can choose how to handle such an object here by telling finish_object__ma() with the --missing=<how> option, so letting them do so, instead of robbing the choice from them, would be a more flexible design here, right?

> if a commit is
> missing, git-rev-list(1) will still barf an error, but this error

OK, yeah, I do see the need for setting the ignore-missing-links bit for what you are doing, and --missing and --ignore-missing-links are orthogonal options. Getting rid of the hardcoded skipping of finish_object__ma() would make sense from this angle, too.

Thanks.