On Fri, Sep 15, 2023 at 8:54 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
> > From: Karthik Nayak <karthik.188@xxxxxxxxx>
> >
> > 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!
