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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 18, 2023, 10:12 UTC
Message-ID: <CAOLa=ZQ9ZRmYe5b6R9ZTTpDCzb2L0UmxkeBujb2kOMSeFuwJGA@mail.gmail.com>
In-Reply-To: <xmqqfs3fe08e.fsf@gitster.g>

On Fri, Sep 15, 2023 at 8:54 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 36 quoted lines

>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > From: Karthik Nayak <karthik.188@gmail.com>
> >
> > The revision backend is used by multiple porcelain commands such as
> > git-rev-list(1) and git-log(1). The backend currently supports ignoring
> > missing links by setting the `ignore_missing_links` bit. This allows the
> > revision walk to skip any objects links which are missing. Expose this
> > bit via an `--ignore-missing-links` user option.
>
> 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.

Here there is a difference for commits and non-commit objects.
1. Commit objects: commits are parsed in revision.c and after that the
`show_commit`
function is called only when the object is available.
2. Non-commit objects: while trees are parsed in revision.c, blobs are
never parsed and
hence, ` show_object` can be called on missing blobs. This is left to
the user to handle. In
our case, we error out in `rev-list.c`, which is not what we want when using the
`--ignore-missing-links` option. Hence, this addition.

There is an argument to be made around compatibility between the `--missing` option and `--ignore-missing-links` option, but since the former only works with non-commit objects I think the latter should be independent, and also the latter is about ignoring all missing links. I also don't think the user should again specify what to do with missing links by adding `--missing=allow-any` as `--ignore-missing-links` is a superset of it.

> Stepping back a bit, with "--missing=print", is this change still
> needed?  The missing objects discovered will be shown at the end,
> with the setting, no?
>

The main difference is that the `--missing` options works entirely with non-commit objects (I'm assuming this was built with promisor notes in mind). So if a commit is missing, git-rev-list(1) will still barf an error, but this error handling is not in `builtin/rev-list.c` rather is in a layer above in `revision.c`. So it wouldn't be trivial for the `--missing` option to support missing commit links. So that's why we expose `--ignore-missing-links` which ensures any kind of object (commits included) if missing, is ignored.

Thanks for the review!