Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...

>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > In 9830926c7d (rev-list: add commit object support in `--missing`
> > option, 2023-10-27) we fixed the `--missing` option in `git rev-list`
> > so that it now works with commits too.
> >
> > Unfortunately, such a command would still fail with a "fatal: bad
> > object <oid>" if it is passed a missing commit, blob or tree as an
> > argument.
> >
> > When such a command is used to find the dependencies of some objects,
> > for example the dependencies of quarantined objects, it would be
> > better if the command would instead consider such missing objects,
> > especially commits, in the same way as other missing objects.
> >
> > If, for example `--missing=print` is used, it would be nice for some
> > use cases if the missing tips passed as arguments were reported in
> > the same way as other missing objects instead of the command just
> > failing.
> >
> > Let's introduce a new `--allow-missing-tips` option to make it work
> > like this.
>
> OK.  Unlike a missing object referenced by a tree, a commit, or a
> tag, where the expected type of the missing object is known to Git,
> I would expect that nobody knowsn what type these missing objects at
> the tip have.  So do we now require "object X missing" instead of
> "commit X missing" in the output?  If we are not giving any type
> information even when we know what the type we expect is, then we do
> not have to worry about this change introducing a new output logic,
> I guess.

> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > index b3f4783858..ae7bb15478 100644
> > --- a/builtin/rev-list.c
> > +++ b/builtin/rev-list.c
> > @@ -562,6 +562,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> >                               break;
> >               }
> >       }
> > +     for (i = 1; i < argc; i++) {
> > +             const char *arg = argv[i];
> > +             if (!strcmp(arg, "--allow-missing-tips")) {
> > +                     if (arg_missing_action == MA_ERROR)
> > +                             die(_("option '%s' only makes sense with '%s' set to '%s' or '%s'"),
> > +                                   "--allow-missing-tips", "--missing=", "allow-*", "print");
> > +                     revs.do_not_die_on_missing_tips = 1;
> > +                     break;
> > +             }
> > +     }
>
> It is unfortunate that we need to add yet another dumb loop that
> does not even try to understand there may be an option whose value
> happens to be the string strcmp() looks for (we already have two
> such loops above this hunk).  I have to wonder if we can do better.

> An idle piece of idea.  Perhaps we can instruct setup_revisions()
> not to die immediately upon seeing a problematic argument, marking
> the revs as "broken" instead, and keep going and interpreting as
> much as it could, so that it may record the presence of "--missing",
> "--exclude-promisor-objects", and "--allow-missing-tips".  Then upon
> seeing setup_revisions() return with such an error, we can redo the
> call with these bits already on.

> Anyway, I digress.
>
> > diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> > index 527aa94f07..283e8fc2c2 100755
> > --- a/t/t6022-rev-list-missing.sh
> > +++ b/t/t6022-rev-list-missing.sh
> > @@ -77,4 +77,55 @@ do
> >       done
> >  done
> >
> > +for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> > +do
> > +     for tip in "" "HEAD"
> > +     do
> > +             for action in "allow-any" "print"
> > +             do
> > +                     test_expect_success "--missing=$action --allow-missing-tips with tip '$obj' missing and tip '$tip'" '
> > +                             oid="$(git rev-parse $obj)" &&
> > +                             path=".git/objects/$(test_oid_to_path $oid)" &&
> > +
> > +                             # Before the object is made missing, we use rev-list to
> > +                             # get the expected oids.
> > +                             if [ "$tip" = "HEAD" ]; then
>
> Style:
>
>                                 if test "$tip" = "HEAD"
>                                 then

> > +                                     git rev-list --objects --no-object-names \
> > +                                             HEAD ^$obj >expect.raw
> > +                             else
> > +                                     >expect.raw
> > +                             fi &&
> > +
> > +                             # Blobs are shared by all commits, so even though a commit/tree
> > +                             # might be skipped, its blob must be accounted for.
> > +                             if [ "$tip" = "HEAD" ] && [ $obj != "HEAD:1.t" ]; then
>
> Ditto.

> > +                                     echo $(git rev-parse HEAD:1.t) >>expect.raw &&
> > +                                     echo $(git rev-parse HEAD:2.t) >>expect.raw
> > +                             fi &&
> > +
> > +                             mv "$path" "$path.hidden" &&
> > +                             test_when_finished "mv $path.hidden $path" &&
> > +
> > +                             git rev-list --missing=$action --allow-missing-tips \
> > +                                  --objects --no-object-names $oid $tip >actual.raw &&
> > +
> > +                             # When the action is to print, we should also add the missing
> > +                             # oid to the expect list.
> > +                             case $action in
> > +                             allow-any)
> > +                                     ;;
> > +                             print)
> > +                                     grep ?$oid actual.raw &&
> > +                                     echo ?$oid >>expect.raw
> > +                                     ;;
>
> OK.  We do not say anything more than the object name (and the fact
> that it is missing with a single byte '?'), so my earlier worry was
> unfounded.  Good.
>
> > +                             esac &&
> > +
> > +                             sort actual.raw >actual &&
> > +                             sort expect.raw >expect &&
> > +                             test_cmp expect actual
> > +                     '
> > +             done
> > +     done
> > +done
> > +
> >  test_done
>
> THanks.