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

> > It actually kind of "works" when you pass blobs or trees. It looks
> > like the command just doesn't use them (except for checking that they
> > actually exist) unless options like --objects, --missing=print and
> > perhaps others are used. So yeah, the doc might need an update, but I
> > think it could be a separate issue, as it's not new and doesn't depend
> > on this small series.
>
> SG. But also, if there's a way to put invisible (developer-facing, not
> user facing) comments inside the relevant doc file it might be easy
> enough to add a to this series.

> > "quarantined objects" refers to the following doc:
> >
> > https://www.git-scm.com/docs/git-receive-pack#_quarantine_environment
> >
> > So to clarify things, the above paragraph looks like the following now:
> >
> > "When such a command is used to find the dependencies of some objects,
> > for example the dependencies of quarantined objects (see the
> > "QUARANTINE ENVIRONMENT" section in the git-receive-pack(1)
> > documentation), it would be better if the command would instead
> > consider such missing objects, especially commits, in the same way as
> > other missing objects."
>
> This reads much better, and is a good motivation to keep in the log
> message.

> > Yeah, I was surprised too. I only found the following similar code in
> > list-objects-filter.c:
> >
> >     oidset_iter_init(src, &iter);
> >     while ((src_oid = oidset_iter_next(&iter)) != NULL)
> >         oidset_insert(dest, src_oid);
> >
> > So yeah perhaps we could create such a helper function.
>
> Perhaps a NEEDSWORK comment is appropriate?

> >> With a few more context lines, the diff looks like
> >>
> >> --8<---------------cut here---------------start------------->8---
> >>         if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))
> >>                 return revs->ignore_missing ? 0 : -1;
> >>         if (!cant_be_filename)
> >>                 verify_non_filename(revs->prefix, arg);
> >>         object = get_reference(revs, arg, &oid, flags ^ local_flags);
> >>         if (!object)
> >> -               return revs->ignore_missing ? 0 : -1;
> >> +               return (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;
> >>         add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);
> >>         add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);
> >>         free(oc.path);
> >>         return 0;
> >> --8<---------------cut here---------------end--------------->8---
> >>
> >> and it's not obvious to me why you only touched the "if (!object)" case
> >> but not the "if (get_oid_with_context(...))" case. Are there any
> >> subtleties here that would not be obvious to reviewers?
> >
> > I thought that if we can't get an oid, we cannot put anything in the
> > 'missing_commit' oidset anyway, so it might be better to error out.
>
> Ah, makes sense. This is a subtle detail, and perhaps worth keeping
> either as a comment (just above the "if (get_oid_with_context(...))"
> case) or in the log message.

> Ah, I see. I think you could add a comment like
>
>     We want to check that things work as expected both:
>
>       - when we pass only one missing tip (when $tip is ""), and:
>       - when we pass one missing tip and a tip that is not missing (when
>         $tip is "HEAD").
>
> at the top of the test case, and probably rename $obj to $missing_tip,
> and rename $tip to $existing_tip.

> Aside: it's a bit unfortunate that the meaning of "missing" becomes
> overloaded slightly here because one could say '$tip == ""' is a
> "missing" tip becauuse the name is not provided. Subtle. Plus there's
> some interplay with the "if (get_oid_with_context(...))" case above
> because we will (?) hit that path if we do pass in "" (empty string arg)
> as a tip to rev-list. It might be worth saying in the comments in the
> implementation, something like
>
>     The term "missing" here means the case where we could not find the object
>     given the object_id. For example, given HEAD~1, its object id (oid)
>     could be 82335b23aa7234320d6f8055243c852e4b6a5ca3. If no real object
>     with this oid exists, it is considered missing. Providing an empty
>     string to rev-list does not touch the "missing tip" codepath.
>
> I wrote the above hastily so it may need further edits to make it
> succinct. But the point is that this definition isn't spelled out in the
> test case, which makes the "" argument for $tip that much more subtle.

> >> OK so you are using this empty string to clear the expect.raw file. If
> >> that's true then what stops us from doing this at the start of every
> >> iteration?
> >
> > I am not sure what you mean. Both:
> >
> >     git rev-list --objects --no-object-names HEAD ^$obj >expect.raw
> >
> > and:
> >
> >     >expect.raw #2
> >
> > are clearing "expect.raw" as ">expect.raw" is used in both cases.
> >
> > If we did the latter at the start of every iteration, then when we do
> > the former afterwards, we would clear "expect.raw" raw again, while
> > there is no point in clearing it twice.
>
> Yes but doing it that way would separate "set up a clean environment for
> this test case" from "find the oid of the non-missing tip" from each
> other in the same if/else stanza, which I believe helps readability.

> >> Also, given how most of this is identical from the loop already in t6022
> >> just above it, it would help to add a comment at the top of this one
> >> explaining how it's different than the one above it.
> >
> > The one I added has an extra `for tip in "" "HEAD"` loop. Anyway I
> > think I will just modify the existing test in the next version I will
> > send, as I plan to implement Junio's suggestion and there will be no
> > new option.

> Now that I have your attention (was this my plan all along? ;) ), I will
> take this opportunity to ping you directly for a review of my own patch
> series for the trailers subsystem:
> https://lore.kernel.org/git/pull.1632.v4.git.1707196348.gitgitgadget@gmail.com/
> which is in its 4th iteration after many helpful comments from Junio.
> Please don't let the patch count (28) raise any alarms --- they used to
> be 10 but I broke them down into smaller steps to ease the review
> process.