On Tue, Feb 13, 2024 at 11:33 PM Linus Arver <linusa@xxxxxxxxxx> wrote:
>
> Christian Couder <christian.couder@xxxxxxxxx> writes:
> > We could introduce a new option to make it work like this, but most
> > users are likely to prefer the command to have this behavior as the
> > default one. Introducing a new option would require another dumb loop
> > to look for that options early, which isn't nice.
>
> s/options/option
Fixed in the V3 I just sent.
> > Also we made `git rev-list` work with missing commits very recently
> > and the command is most often passed commits as arguments. So let's
> > consider this as a bug fix related to these previous change.
>
> s/previous change/recent changes
Fixed in V3, thanks.
> > 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. Anyway I think that topic
is independent from this small series.
> 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, 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.
> 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.)
> > --exclude-promisor-objects::
> > (For internal use only.) Prefilter object traversal at
> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > index b3f4783858..ec9556f135 100644
> > --- a/builtin/rev-list.c
> > +++ b/builtin/rev-list.c
> > @@ -545,6 +545,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> > *
> > * Let "--missing" to conditionally set fetch_if_missing.
> > */
> > + /*
> > + * NEEDSWORK: These dump loops to look for some options early
> > + * are ugly.
>
> I agree with Junio's suggestion to use more objective language.
>
> > We really need setup_revisions() to have a
> > + * mechanism to allow and disallow some sets of options for
> > + * different commands (like rev-list, replay, etc). Such
>
> s/Such/Such a
Fixed in V3
> > + * mechanism should do an early parsing of option and be able
>
> s/option/options
Fixed in V3, thanks.
> > + * 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.
> > + * ... 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(...))`. So I prefer to avoid some back and forth
here.
> > +++ 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.
