Christian Couder <christian.couder@xxxxxxxxx> writes:
> On Tue, Feb 13, 2024 at 11:33 PM Linus Arver <linusa@xxxxxxxxxx> wrote:
>>
>> Christian Couder <christian.couder@xxxxxxxxx> writes:
>> > While at it let's add a NEEDSWORK comment to say that we should get
>> > rid of the existing ugly dumb loops that parse the
>> > `--exclude-promisor-objects` and `--missing=...` options early.
>
>> > @@ -1019,6 +1019,10 @@ Unexpected missing objects will raise an error.
>> > +
>> > The form '--missing=print' is like 'allow-any', but will also print a
>> > list of the missing objects. Object IDs are prefixed with a ``?'' character.
>> > ++
>> > +If some tips passed to the traversal are missing, they will be
>> > +considered as missing too, and the traversal will ignore them. In case
>> > +we cannot get their Object ID though, an error will be raised.
>>
>> The only other mention of the term "tips" is for the "--alternate-refs"
>> flag on line 215 which uses "ref tips". Perhaps this is obvious to
>> veteran Git developers but I do wonder if we need to somehow define it
>> (however briefly) the first time we mention it (either in the document
>> as a whole, or just within this newly added paragraph).
>
> I did a quick grep in Documentation/git*.txt and found more than 130
> instances of the 'tip' word. So I think it is quite common in the
> whole Git documentation and our glossary would likely be the right
> place to document it if we decide to do so.
SGTM.
> Anyway I think that topic
> is independent from this small series.
Agreed.
>> Here's an alternate wording
>>
>> Ref tips given on the command line are a special case.
>
> `git rev-list` has a `--stdin` mode which makes it accept tips from
> stdin
Ah, thanks for the context.
> , so talking about the command line is not enough. Also maybe one
> day some config option could be added that makes the command include
> additional tips.
>> They are
>> first dereferenced to Object IDs (if this is not possible, an error
>> will be raised). Then these IDs are checked to see if the objects
>> they refer to exist. If so, the traversal happens starting with
>> these tips. Otherwise, then such tips will not be used for
>> traversal.
>>
>> Even though such missing tips won't be included for traversal, for
>> purposes of the `--missing` flag they will be treated the same way
>> as those objects that did get traversed (and were determined to be
>> missing). For example, if `--missing=print` is given then the Object
>> IDs of these tips will be printed just like all other missing
>> objects encountered during traversal.
>
> Your wording describes what happens correctly, but I don't see much
> added value for this specific patch compared to my wording which is
> shorter.
>
> Here I think, we only need to describe the result of the command in
> the special case that the patch is fixing. We don't need to go into
> details of how the command or even --missing works. It could be
> interesting to go into details of how things work, but I think it's a
> separate topic. Or perhaps it's even actually counter productive to go
> into too much detail as it could prevent us from finding other ways to
> make it work better. Anyway it seems to me to be a separate topic to
> discuss.
Fair enough.
>> But also, I realize that these documentation touch-ups might be better
>> served by an overall pass over the whole document, so it's fine if we
>> decide not to take this suggestion at this time.
>
> Right, I agree. Thanks for telling this.
>
>> Aside: unfortunately we don't seem to define the relationship between
>> ref tips (e.g., "HEAD"), object IDs (40-char hex string), and the actual
>> objects (the real data that we traverse over). It's probably another
>> thing that could be fixed up in the docs in the future.
>
> Yeah, and for rev-list a tip could also be a tree or a blob. It
> doesn't need to be a "ref tip". (Even though a ref can point to a tree
> or a blog, it's very rare in practice.)
Interesting, thanks for the info.
BTW I appreciate you (and others on the list too) taking the time to
explain such subtleties. Although I've been using Git since 2008 a lot
of the terms used around in the codebase can feel quite foreign to me.
So, thanks again.
>> > + * to manage the `--exclude-promisor-objects` and `--missing=...`
>> > + * options below.
>> > + */
>> > for (i = 1; i < argc; i++) {
>> > const char *arg = argv[i];
>> > if (!strcmp(arg, "--exclude-promisor-objects")) {
>> >
>> > [...]
>> >
>> > @@ -2178,13 +2183,18 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
>> > if (revarg_opt & REVARG_COMMITTISH)
>> > get_sha1_flags |= GET_OID_COMMITTISH;
>> >
>> > + /*
>> > + * Even if revs->do_not_die_on_missing_objects is set, we
>> > + * should error out if we can't even get an oid, ...
>> > + */
>>
>> Perhaps this wording is more precise?
>>
>> If we can't even get an oid, we are forced to error out (regardless
>> of revs->do_not_die_on_missing_objects) because a valid traversal
>> must start from *some* valid oid. OTOH we ignore the ref tip
>> altogether with revs->ignore_missing.
>
> This uses "valid oid" and "valid traversal", but I am not sure it's
> easy to understand what "valid" means in both of these expressions.
>
> Also if all the tips passed to the command are missing, the traversal
> doesn't need to actually start. The command, assuming
> `--missing=print` is passed, just needs to output the oids of the tips
> as missing oids.
>
> I also think that "ref tip" might be misleading as trees and blos can
> be passed as tips.
>
> So I prefer to keep the wording I used.
Makes sense, SGTM.
>> > + * ... as
>> > + * `--missing=print` should be able to report missing oids.
>>
>> I think this comment would be better placed ...
>>
>> > 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);
>>
>> ... around here.
>
> In a previous round, I was asked to put such a comment before `if
> (get_oid_with_context(...))`.
Sorry, I missed that.
> So I prefer to avoid some back and forth
> here.
SGTM.
>> > +++ b/t/t6022-rev-list-missing.sh
>> > @@ -78,4 +78,60 @@ do
>> > done
>> > done
>> >
>> > +for missing_tip in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
>> > +do
>> > + # We want to check that things work when both
>> > + # - all the tips passed are missing (case existing_tip = ""), and
>> > + # - there is one missing tip and one existing tip (case existing_tip = "HEAD")
>> > + for existing_tip in "" "HEAD"
>> > + do
>>
>> Though I am biased, these new variable names do make this test that much
>> easier to read. Thanks.
>
> Thanks for suggesting them and for your reviews.
You're welcome!
