v2, 4 partsrev-list: allow missing tips with --missing

> In a following commit, we will need to add all the oids from a set into
> another set. In "list-objects-filter.c", there is already a static
> function called add_all() to do that.
>
> Let's rename this function oidset_insert_from_set() and move it into
> oidset.{c,h} to make it generally available.
>
> While at it, let's remove a useless `!= NULL`.

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  list-objects-filter.c | 11 +----------
>  oidset.c              | 10 ++++++++++
>  oidset.h              |  6 ++++++
>  3 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/list-objects-filter.c b/list-objects-filter.c
> index da287cc8e0..4346f8da45 100644
> --- a/list-objects-filter.c
> +++ b/list-objects-filter.c
> @@ -711,15 +711,6 @@ static void filter_combine__free(void *filter_data)
>  	free(d);
>  }
>  
> -static void add_all(struct oidset *dest, struct oidset *src) {
> -	struct oidset_iter iter;
> -	struct object_id *src_oid;
> -
> -	oidset_iter_init(src, &iter);
> -	while ((src_oid = oidset_iter_next(&iter)) != NULL)
> -		oidset_insert(dest, src_oid);
> -}
> -
>  static void filter_combine__finalize_omits(
>  	struct oidset *omits,
>  	void *filter_data)
> @@ -728,7 +719,7 @@ static void filter_combine__finalize_omits(
>  	size_t sub;
>  
>  	for (sub = 0; sub < d->nr; sub++) {
> -		add_all(omits, &d->sub[sub].omits);
> +		oidset_insert_from_set(omits, &d->sub[sub].omits);
>  		oidset_clear(&d->sub[sub].omits);
>  	}
>  }
> diff --git a/oidset.c b/oidset.c
> index d1e5376316..91d1385910 100644
> --- a/oidset.c
> +++ b/oidset.c
> @@ -23,6 +23,16 @@ int oidset_insert(struct oidset *set, const struct object_id *oid)
>  	return !added;
>  }
>  
> +void oidset_insert_from_set(struct oidset *dest, struct oidset *src)
> +{
> +	struct oidset_iter iter;
> +	struct object_id *src_oid;
> +
> +	oidset_iter_init(src, &iter);
> +	while ((src_oid = oidset_iter_next(&iter)))

> +		oidset_insert(dest, src_oid);
> +}
> +
>  int oidset_remove(struct oidset *set, const struct object_id *oid)
>  {
>  	khiter_t pos = kh_get_oid_set(&set->set, *oid);
> diff --git a/oidset.h b/oidset.h
> index ba4a5a2cd3..262f4256d6 100644
> --- a/oidset.h
> +++ b/oidset.h
> @@ -47,6 +47,12 @@ int oidset_contains(const struct oidset *set, const struct object_id *oid);
>   */
>  int oidset_insert(struct oidset *set, const struct object_id *oid);
>  
> +/**
> + * Insert all the oids that are in set 'src' into set 'dest'; a copy
> + * is made of each oid inserted into set 'dest'.
> + */

> +
>  /**
>   * Remove the oid from the set.
>   *
> -- 
> 2.43.0.565.g97b5fd12a3.dirty

>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > In a following commit, we will need to add all the oids from a set into
> > another set. In "list-objects-filter.c", there is already a static
> > function called add_all() to do that.
>
> Nice find.
>
> > Let's rename this function oidset_insert_from_set() and move it into
> > oidset.{c,h} to make it generally available.
>
> At some point (I don't ask for it in this series) we should add unit
> tests for this newly-exposed function. Presumably the stuff around
> object/oid handling is stable enough to receive unit tests.

> > +void oidset_insert_from_set(struct oidset *dest, struct oidset *src)
> > +{
> > +     struct oidset_iter iter;
> > +     struct object_id *src_oid;
> > +
> > +     oidset_iter_init(src, &iter);
> > +     while ((src_oid = oidset_iter_next(&iter)))
>
> Are the extra parentheses necessary?

> > +/**
> > + * Insert all the oids that are in set 'src' into set 'dest'; a copy
> > + * is made of each oid inserted into set 'dest'.
> > + */
>
> Just above in oid_insert() there is already a comment about needing to
> copy each oid.

>     /**
>      * Insert the oid into the set; a copy is made, so "oid" does not need
>      * to persist after this function is called.
>      *
>      * Returns 1 if the oid was already in the set, 0 otherwise. This can be used
>      * to perform an efficient check-and-add.
>      */
>
> so perhaps the following wording is simpler?
>
>     Like oid_insert(), but insert all oids found in 'src'. Calls
>     oid_insert() internally.

> > +void oidset_insert_from_set(struct oidset *dest, struct oidset *src);
>
> Perhaps "oidset_insert_all" would be a simpler name? I generally prefer
> to reuse any descriptors in comments to guide the names. Plus this
> function used to be called "add_all()" so keeping the "all" naming style
> feels right.

> On Tue, Feb 13, 2024 at 10:02 PM Linus Arver <linusa@google.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>> > In a following commit, we will need to add all the oids from a set into
>> > another set. In "list-objects-filter.c", there is already a static
>> > function called add_all() to do that.
>>
>> Nice find.
>>
>> > Let's rename this function oidset_insert_from_set() and move it into
>> > oidset.{c,h} to make it generally available.
>>
>> At some point (I don't ask for it in this series) we should add unit
>> tests for this newly-exposed function. Presumably the stuff around
>> object/oid handling is stable enough to receive unit tests.
>
> Yeah, ideally there should be unit tests for oidset and all its
> features, but it seems to me that there aren't any. Also oidset is
> based on khash.h which was originally imported from
> https://github.com/attractivechaos/klib/ without tests. So I think
> it's a different topic to add tests from scratch to oidset, khash.h or
> both.
>
> Actually after taking another look, it looks like khash.h or some of
> its features are tested through "helper/test-oidmap.c" and
> "t0016-oidmap.sh". I still think it's another topic to test oidset.

>> > +void oidset_insert_from_set(struct oidset *dest, struct oidset *src)
>> > +{
>> > +     struct oidset_iter iter;
>> > +     struct object_id *src_oid;
>> > +
>> > +     oidset_iter_init(src, &iter);
>> > +     while ((src_oid = oidset_iter_next(&iter)))
>>
>> Are the extra parentheses necessary?
>
> Yes. Without them gcc errors out with:
>
> oidset.c: In function ‘oidset_insert_from_set’:
> oidset.c:32:16: error: suggest parentheses around assignment used as
> truth value [-Werror=parentheses]
>   32 |         while (src_oid = oidset_iter_next(&iter))
>      |                ^~~~~~~
>
> Having extra parentheses is a way to tell the compiler that we do want
> to use '=' and not '=='. This helps avoid the very common mistake of
> using '=' where '==' was intended.

>> > +/**
>> > + * Insert all the oids that are in set 'src' into set 'dest'; a copy
>> > + * is made of each oid inserted into set 'dest'.
>> > + */
>>
>> Just above in oid_insert() there is already a comment about needing to
>> copy each oid.
>
> (It's "oidset_insert()" not "oid_insert()".)

>>     /**
>>      * Insert the oid into the set; a copy is made, so "oid" does not need
>>      * to persist after this function is called.
>>      *
>>      * Returns 1 if the oid was already in the set, 0 otherwise. This can be used
>>      * to perform an efficient check-and-add.
>>      */
>>
>> so perhaps the following wording is simpler?
>>
>>     Like oid_insert(), but insert all oids found in 'src'. Calls
>>     oid_insert() internally.
>
> (What you suggest would need s/oid_insert/oidset_insert/)
>
> Yeah, it's a bit simpler and shorter, but on the other hand a reader
> might have to read both this and the oidset_insert() doc, so in the
> end I am not sure it's a big win for readability. And if they don't
> read the oidset_insert() doc, they might miss the fact that we are
> copying the oids we insert, which might result in a bug.

> Also your wording ties the implementation with oidset_insert(), which
> we might not want if we could find something more performant. See
> Junio's comment on this patch saying his initial reaction was that
> copying underlying bits may even be more efficient.
>
> So I prefer not to change this.

>> > +void oidset_insert_from_set(struct oidset *dest, struct oidset *src);
>>
>> Perhaps "oidset_insert_all" would be a simpler name? I generally prefer
>> to reuse any descriptors in comments to guide the names. Plus this
>> function used to be called "add_all()" so keeping the "all" naming style
>> feels right.
>
> We already have other related types like 'struct oid-array' and
> 'struct oidmap' to store oids, as well as code that inserts many oids
> into an oidset from a 'struct ref *' linked list or array in a tight
> loop.

> So if we want to add functions inserting all the oids from
> instances of such types, how should we call them?
>
> I would say we should use suffixes like: "_from_set", "_from_map",
> "from_array", "_from_ref_list", "_from_ref_array", etc.

> If we start using just "_all" for oidset, then what should we use for
> the other types? I don't see a good answer to that, so I prefer to
> stick with "_from_set" for oidset.
>
> Thanks.

> >> Are the extra parentheses necessary?
> >
> > Yes. Without them gcc errors out with:
> >
> > oidset.c: In function ‘oidset_insert_from_set’:
> > oidset.c:32:16: error: suggest parentheses around assignment used as
> > truth value [-Werror=parentheses]
> >   32 |         while (src_oid = oidset_iter_next(&iter))
> >      |                ^~~~~~~
> >
> > Having extra parentheses is a way to tell the compiler that we do want
> > to use '=' and not '=='. This helps avoid the very common mistake of
> > using '=' where '==' was intended.
>
> Ah, so it is a "please trust me gcc, I know what I am doing" thing and
> not a "this is required in C" thing. Makes sense, thanks for clarifying.
>
> Sorry for the noise.

> >> so perhaps the following wording is simpler?
> >>
> >>     Like oid_insert(), but insert all oids found in 'src'. Calls
> >>     oid_insert() internally.
> >
> > (What you suggest would need s/oid_insert/oidset_insert/)
> >
> > Yeah, it's a bit simpler and shorter, but on the other hand a reader
> > might have to read both this and the oidset_insert() doc, so in the
> > end I am not sure it's a big win for readability. And if they don't
> > read the oidset_insert() doc, they might miss the fact that we are
> > copying the oids we insert, which might result in a bug.
>
> When functions are built on top of other functions, I think it is good
> practice to point readers to those underlying functions. In this case
> the new function is a wrapper around oidset_insert() which does all the
> real work. Plus the helper function already has some documentation about
> copying behavior that we already thought was important enough to call
> out explicitly.
>
> So, tying this definition to that (foundational) helper function sounds
> like a good idea to me in terms of readability. IOW we can inform
> readers "hey, we're just a wrapper around this other important function
> --- go there if you're curious about internals" and emphasizing that
> sort of relationship which may not be immediately obvious to those not
> familiar with this area would be nice.
>
> Alternatively, we could repeat the same comment WRT copying here but
> that seems redundant and prone to maintenance burdens down the road (if
> we ever change this behavior we have to change the comment in multiple
> functions, possibly).
>
> > Also your wording ties the implementation with oidset_insert(), which
> > we might not want if we could find something more performant. See
> > Junio's comment on this patch saying his initial reaction was that
> > copying underlying bits may even be more efficient.
> >
> > So I prefer not to change this.
>
> OK.

> >> > +void oidset_insert_from_set(struct oidset *dest, struct oidset *src);
> >>
> >> Perhaps "oidset_insert_all" would be a simpler name? I generally prefer
> >> to reuse any descriptors in comments to guide the names. Plus this
> >> function used to be called "add_all()" so keeping the "all" naming style
> >> feels right.
> >
> > We already have other related types like 'struct oid-array' and
> > 'struct oidmap' to store oids, as well as code that inserts many oids
> > into an oidset from a 'struct ref *' linked list or array in a tight
> > loop.
>
> Thank you for the additional context I was not aware of.
>
> > So if we want to add functions inserting all the oids from
> > instances of such types, how should we call them?
> >
> > I would say we should use suffixes like: "_from_set", "_from_map",
> > "from_array", "_from_ref_list", "_from_ref_array", etc.
>
> I agree.
>
> However, I would like to point out that the function being added in this
> patch is a bit special: it is inserting from one "oidset" into another
> "oidset". IOW the both the dest and src types are the same.
>
> For the cases where the types are different, I totally agree that using
> the suffixes (to encode the type information of the src into the
> function name itself) is a good idea.
>
> So I think it's still fine to use "oidset_insert_all" because the only
> type in the parameter list is an oidset.

> On Fri, Feb 16, 2024 at 2:10 AM Linus Arver <linusa@google.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>> >> so perhaps the following wording is simpler?
>> >>
>> >>     Like oid_insert(), but insert all oids found in 'src'. Calls
>> >>     oid_insert() internally.
>> >
>> > (What you suggest would need s/oid_insert/oidset_insert/)
>> >
>> > Yeah, it's a bit simpler and shorter, but on the other hand a reader
>> > might have to read both this and the oidset_insert() doc, so in the
>> > end I am not sure it's a big win for readability. And if they don't
>> > read the oidset_insert() doc, they might miss the fact that we are
>> > copying the oids we insert, which might result in a bug.
>>
>> When functions are built on top of other functions, I think it is good
>> practice to point readers to those underlying functions. In this case
>> the new function is a wrapper around oidset_insert() which does all the
>> real work. Plus the helper function already has some documentation about
>> copying behavior that we already thought was important enough to call
>> out explicitly.
>>
>> So, tying this definition to that (foundational) helper function sounds
>> like a good idea to me in terms of readability. IOW we can inform
>> readers "hey, we're just a wrapper around this other important function
>> --- go there if you're curious about internals" and emphasizing that
>> sort of relationship which may not be immediately obvious to those not
>> familiar with this area would be nice.
>>
>> Alternatively, we could repeat the same comment WRT copying here but
>> that seems redundant and prone to maintenance burdens down the road (if
>> we ever change this behavior we have to change the comment in multiple
>> functions, possibly).
>>
>> > Also your wording ties the implementation with oidset_insert(), which
>> > we might not want if we could find something more performant. See
>> > Junio's comment on this patch saying his initial reaction was that
>> > copying underlying bits may even be more efficient.
>> >
>> > So I prefer not to change this.
>>
>> OK.
>
> I must say that in cases like this, it's difficult to be right for
> sure because it's mostly with enough hindsight that we can tell what
> turned out to be a good decision. Here for example, it might be that
> someone will find something more performant soon or it might turn out
> that the function will never change. We just can't know.
>
> So as long as the wording is clear and good enough, I think there is
> no point in trying to improve it as much as possible. Here both your
> wording and my wording seem clear and good enough to me. Junio might
> change his mind but so far it seems that he found my wording good
> enough too. So in cases like this, it's just simpler to keep current
> wording.

>> >> > +void oidset_insert_from_set(struct oidset *dest, struct oidset *src);
>> >>
>> >> Perhaps "oidset_insert_all" would be a simpler name? I generally prefer
>> >> to reuse any descriptors in comments to guide the names. Plus this
>> >> function used to be called "add_all()" so keeping the "all" naming style
>> >> feels right.
>> >
>> > We already have other related types like 'struct oid-array' and
>> > 'struct oidmap' to store oids, as well as code that inserts many oids
>> > into an oidset from a 'struct ref *' linked list or array in a tight
>> > loop.
>>
>> Thank you for the additional context I was not aware of.
>>
>> > So if we want to add functions inserting all the oids from
>> > instances of such types, how should we call them?
>> >
>> > I would say we should use suffixes like: "_from_set", "_from_map",
>> > "from_array", "_from_ref_list", "_from_ref_array", etc.
>>
>> I agree.
>>
>> However, I would like to point out that the function being added in this
>> patch is a bit special: it is inserting from one "oidset" into another
>> "oidset". IOW the both the dest and src types are the same.
>>
>> For the cases where the types are different, I totally agree that using
>> the suffixes (to encode the type information of the src into the
>> function name itself) is a good idea.
>>
>> So I think it's still fine to use "oidset_insert_all" because the only
>> type in the parameter list is an oidset.
>
> Yeah, here also I think both "oidset_insert_from_set" and
> "oidset_insert_all" are clear and good enough.
>
>> BUT, maybe in our codebase we already use suffixes like this even for
>> cases where the types are the same? I don't know the answer to this
>> question.
>
> I agree that it could be a good thing to be consistent with similar
> structs, but so far for oidmap there is only oidmap_put(), and, for
> oid-array, only oid_array_append(). (And no, I didn't look further
> than this.)
>
>> However if we really wanted to be consistent then maybe we
>> should be using the name oidset_insert_from_oidset() and not
>> oidset_insert_from_set().
>
> Yeah, "oidset_insert_from_oidset" and perhaps
> "oidset_insert_all_from_oidset" would probably be fine too. Junio
> found my wording good enough though, so I think it's just simpler not
> to change it.
>
> Also it's not like it can't be improved later if there is a good
> reason like consistency with other oid related structs that might get
> oidmap_put_all() or oid_array_append_all(). But again we can't predict
> what will happen, so...

>  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.

> 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. 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
> +	 * mechanism should do an early parsing of option and be able
> +	 * 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")) {
> @@ -753,8 +762,12 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  
>  	if (arg_print_omitted)
>  		oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);
> -	if (arg_missing_action == MA_PRINT)
> +	if (arg_missing_action == MA_PRINT) {
>  		oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
> +		/* Already add missing tips */
> +		oidset_insert_from_set(&missing_objects, &revs.missing_commits);
> +		oidset_clear(&revs.missing_commits);
> +	}

> Christian Couder <christian.couder@gmail.com> writes:
>
>> +	 * mechanism to allow and disallow some sets of options for
>> +	 * different commands (like rev-list, replay, etc). Such
>> +	 * mechanism should do an early parsing of option and be able
>> +	 * 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")) {
>> @@ -753,8 +762,12 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>>  
>>  	if (arg_print_omitted)
>>  		oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);
>> -	if (arg_missing_action == MA_PRINT)
>> +	if (arg_missing_action == MA_PRINT) {
>>  		oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
>> +		/* Already add missing tips */
>> +		oidset_insert_from_set(&missing_objects, &revs.missing_commits);
>> +		oidset_clear(&revs.missing_commits);
>> +	}
>
> It is unclear what "already" here refers to, at least to me.

>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Christian Couder <christian.couder@gmail.com> writes:
> >
> >> +     * mechanism to allow and disallow some sets of options for
> >> +     * different commands (like rev-list, replay, etc). Such
> >> +     * mechanism should do an early parsing of option and be able
> >> +     * 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")) {
> >> @@ -753,8 +762,12 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> >>
> >>      if (arg_print_omitted)
> >>              oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);
> >> -    if (arg_missing_action == MA_PRINT)
> >> +    if (arg_missing_action == MA_PRINT) {
> >>              oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
> >> +            /* Already add missing tips */
> >> +            oidset_insert_from_set(&missing_objects, &revs.missing_commits);
> >> +            oidset_clear(&revs.missing_commits);
> >> +    }
> >
> > It is unclear what "already" here refers to, at least to me.

> It's grammatically correct but perhaps a bit "over eager" (gives the
> impression that we get these missing tips all the time and is a common
> "happy" path). I do still think my earlier v1 comments
>
>     Did you mean "Add already-missing commits"? Perhaps even more explicit
>     would be "Add missing tips"?

>> >> +            /* Already add missing tips */
>> >> +            oidset_insert_from_set(&missing_objects, &revs.missing_commits);
>> >> +            oidset_clear(&revs.missing_commits);
>> >> +    }
>> >
>> > It is unclear what "already" here refers to, at least to me.
>
> I wanted to hint that we already have some missing objects that we can
> add to the set. But it's not an important detail and I agree it can be
> confusing.

> > 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. We really need setup_revisions() to have a
>
> I would drop the "dump" and "ugly" from here.  It OK to make value
> judgement with such words in casual conversations on a proposed
> patch, but when we make a request to future developers to fix our
> mess, we should be more objective and stick to the techincal facts,
> so that they have better understanding on why we think this area
> needs more work.
>
> Perhaps something like:
>
>     These loops that attempt to find presence of options without
>     understanding what the options they are skipping are broken
>     (e.g., it would not know "--grep --exclude-promisor-objects" is
>     not triggering "--exclude-promisor-objects" option).

> Everything after "We really need" is good (modulo possible grammos),
> I think.  Thanks for writing it.
>
> > +      * mechanism to allow and disallow some sets of options for
> > +      * different commands (like rev-list, replay, etc). Such
> > +      * mechanism should do an early parsing of option and be able
> > +      * 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")) {
> > @@ -753,8 +762,12 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> >
> >       if (arg_print_omitted)
> >               oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);
> > -     if (arg_missing_action == MA_PRINT)
> > +     if (arg_missing_action == MA_PRINT) {
> >               oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
> > +             /* Already add missing tips */
> > +             oidset_insert_from_set(&missing_objects, &revs.missing_commits);
> > +             oidset_clear(&revs.missing_commits);
> > +     }
>
> It is unclear what "already" here refers to, at least to me.

> 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 works with with missing commits, not just blobs/trees.
>
> 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 (before the rev walking even begins).
>
> 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.

> 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.
>
> Helped-by: Linus Arver <linusa@google.com>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/rev-list-options.txt |  4 +++
>  builtin/rev-list.c                 | 15 +++++++-
>  revision.c                         | 14 ++++++--
>  t/t6022-rev-list-missing.sh        | 56 ++++++++++++++++++++++++++++++
>  4 files changed, 85 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index a583b52c61..bb753b6292 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -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.

>  --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.

> +	 * 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, ...
> +	 */

>  	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_objects) ? 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);
> @@ -3830,8 +3840,6 @@ int prepare_revision_walk(struct rev_info *revs)
>  				       FOR_EACH_OBJECT_PROMISOR_ONLY);
>  	}
>  
> -	oidset_init(&revs->missing_commits, 0);
> -
>  	if (!revs->reflog_info)
>  		prepare_to_use_bloom_filter(revs);
>  	if (!revs->unsorted_input)
> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> index 5f1be7abb5..78387eebb3 100755
> --- a/t/t6022-rev-list-missing.sh
> +++ 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

> > 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

> > 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

> > @@ -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).

> 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.

> >  --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

> > +      * 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.

> > +      * ... 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.

> > +++ 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.

> On Tue, Feb 13, 2024 at 11:33 PM Linus Arver <linusa@google.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> 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.

>> 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

>> 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.)

>> > +      * 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(...))`.

>> > +++ 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.

> # Patch overview
>
> Patches 1/4 (revision: clarify a 'return NULL' in get_reference()),
> 2/4 (oidset: refactor oidset_insert_from_set()) and 
> 3/4 (t6022: fix 'test' style and 'even though' typo) are very small
> preparatory cleanups.
>
> Patch 4/4 (rev-list: allow missing tips with --missing=[print|allow*])
> allows git-rev-list(1) with `--missing=<arg>` when <arg> is not
> 'error' to not fail if some tips it is passed are missing.

>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > # Patch overview
> >
> > Patches 1/4 (revision: clarify a 'return NULL' in get_reference()),
> > 2/4 (oidset: refactor oidset_insert_from_set()) and
> > 3/4 (t6022: fix 'test' style and 'even though' typo) are very small
> > preparatory cleanups.
> >
> > Patch 4/4 (rev-list: allow missing tips with --missing=[print|allow*])
> > allows git-rev-list(1) with `--missing=<arg>` when <arg> is not
> > 'error' to not fail if some tips it is passed are missing.
>
> Thanks.  There is an existing test that makes a bad assumption and
> fails with these patches.  We may need something like this patch as
> a preliminary fix before these four patches.

> diff --git a/revision.c b/revision.c
> index 0c7266b1eb..8f0d638af1 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -419,15 +419,21 @@ static struct commit *handle_commit(struct rev_info *revs,
>  	 */
>  	while (object->type == OBJ_TAG) {
>  		struct tag *tag = (struct tag *) object;
> +		struct object_id *oid;
>  		if (revs->tag_objects && !(flags & UNINTERESTING))
>  			add_pending_object(revs, object, tag->tag);
> -		object = parse_object(revs->repo, get_tagged_oid(tag));
> +		oid = get_tagged_oid(tag);
> +		object = parse_object(revs->repo, oid);

>  		if (!object) {
>  			if (revs->ignore_missing_links || (flags & UNINTERESTING))
>  				return NULL;
>  			if (revs->exclude_promisor_objects &&
>  			    is_promisor_object(&tag->tagged->oid))
>  				return NULL;
> +			if (revs->do_not_die_on_missing_objects && oid) {
> +				oidset_insert(&revs->missing_commits, oid);
> +				return NULL;
> +			}

> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> index 78387eebb3..127180e1c9 100755
> --- a/t/t6022-rev-list-missing.sh
> +++ b/t/t6022-rev-list-missing.sh
> @@ -10,7 +10,10 @@ TEST_PASSES_SANITIZE_LEAK=true
>  test_expect_success 'create repository and alternate directory' '
>  	test_commit 1 &&
>  	test_commit 2 &&
> -	test_commit 3
> +	test_commit 3 &&
> +	git tag -m "tag message" annot_tag HEAD~1 &&
> +	git tag regul_tag HEAD~1 &&
> +	git branch a_branch HEAD~1
>  '
>  
>  # We manually corrupt the repository, which means that the commit-graph may
> @@ -78,7 +81,7 @@ do
>  	done
>  done
>  
> -for missing_tip in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> +for missing_tip in "annot_tag" "regul_tag" "a_branch" "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
> @@ -88,9 +91,6 @@ do
>  		for action in "allow-any" "print"
>  		do
>  			test_expect_success "--missing=$action with tip '$missing_tip' missing and tip '$existing_tip'" '
> -				oid="$(git rev-parse $missing_tip)" &&
> -				path=".git/objects/$(test_oid_to_path $oid)" &&
> -
>  				# Before the object is made missing, we use rev-list to
>  				# get the expected oids.
>  				if test "$existing_tip" = "HEAD"
> @@ -109,11 +109,23 @@ do
>  					echo $(git rev-parse HEAD:2.t) >>expect.raw
>  				fi &&
>  
> +				missing_oid="$(git rev-parse $missing_tip)" &&
> +
> +				if test "$missing_tip" = "annot_tag"
> +				then
> +					oid="$(git rev-parse $missing_tip^{commit})" &&
> +					echo "$missing_oid" >>expect.raw
> +				else
> +					oid="$missing_oid"
> +				fi &&
> +
> +				path=".git/objects/$(test_oid_to_path $oid)" &&
> +
>  				mv "$path" "$path.hidden" &&
>  				test_when_finished "mv $path.hidden $path" &&

>  				git rev-list --missing=$action --objects --no-object-names \
> -				     $oid $existing_tip >actual.raw &&
> +				     $missing_oid $existing_tip >actual.raw &&
>  
>  				# When the action is to print, we should also add the missing
>  				# oid to the expect list.