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!