Re: [PATCH] rev-list: print missing object type with --missing=print-type

> On Wed, Jan 8, 2025 at 4:43 AM Justin Tobler <jltobler@gmail.com> wrote:
> >
> > Handling of missing objects encounted by git-rev-list(1) can be
> > configured with the `--missing=<action>` option and specifying the
> > desired action. Of the available missing actions, none provide a way to
> > print additional information about the missing object such as its type.
> 
> What kind of additional information could we also print except for the type?

> 
> > Add a new missing action called `print-type`. Similar to `print`, this
> > action prints a list of missing objects but also includes the object
> > type if available in the form: `?<oid> [type]`.
> >
> > Signed-off-by: Justin Tobler <jltobler@gmail.com>
> > ---
> > Instead of adding an additional missing action type for the explicit
> > purpose of printing type info, I also considered a separate option
> > (something like `--object-types`, or maybe a `--missing-format`) that
> > could be used in combination with the `--missing=print` option to
> > achieve the same result. The main intent is for the missing object type
> > and I wasn't sure if type info would be useful in the general case so I
> > opted to choose the former approach.
> 
> If there are many other kinds of information we could also print, then
> maybe a `--missing-format=<format>` option would make more sense
> rather than adding `print-X`, `print-Y`, `print-X-Y`, etc. Otherwise,
> yeah, I would say that `print-type` makes sense.

> > @@ -103,6 +109,8 @@ static off_t get_object_disk_usage(struct object *obj)
> >
> >  static inline void finish_object__ma(struct object *obj)
> >  {
> > +       struct missing_objects_map_entry *entry, *old;
> > +
> >         /*
> >          * Whether or not we try to dynamically fetch missing objects
> >          * from the server, we currently DO NOT have the object.  We
> > @@ -119,7 +127,12 @@ static inline void finish_object__ma(struct object *obj)
> >                 return;
> >
> >         case MA_PRINT:
> > -               oidset_insert(&missing_objects, &obj->oid);
> > +       case MA_PRINT_TYPE:
> > +               CALLOC_ARRAY(entry, 1);
> > +               entry->entry.oid = obj->oid;
> > +               entry->type = obj->type;
> > +               old = oidmap_put(&missing_objects, entry);
> > +               free(old);
> >                 return;
> 
> Maybe a function like:
> 
> static void add_missing_object_entry(struct object_id *oid, unsigned int type)
> {
>     struct missing_objects_map_entry *entry, *old;
> 
>     CALLOC_ARRAY(entry, 1);
>     entry->entry.oid = *oid;
>     entry->type = type;
>     old = oidmap_put(&missing_objects, entry);
>     free(old);
> }
> 
> and then:
> 
>     case MA_PRINT:
>     case MA_PRINT_TYPE:
>         add_missing_object_entry(&obj->oid, obj->type);
>         return;
> 
> could help keep finish_object__ma() (which is inlined) short and also
> avoid some code duplication in the code below.

> 
> >         case MA_ALLOW_PROMISOR:
> > @@ -414,6 +427,12 @@ static inline int parse_missing_action_value(const char *value)
> >                 return 1;
> >         }
> >
> > +       if (!strcmp(value, "print-type")) {
> > +               arg_missing_action = MA_PRINT_TYPE;
> > +               fetch_if_missing = 0;
> > +               return 1;
> > +       }
> > +
> >         if (!strcmp(value, "allow-promisor")) {
> >                 arg_missing_action = MA_ALLOW_PROMISOR;
> >                 fetch_if_missing = 0;
> > @@ -781,10 +800,26 @@ int cmd_rev_list(int argc,
> >
> >         if (arg_print_omitted)
> >                 oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);
> > -       if (arg_missing_action == MA_PRINT) {
> > -               oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
> > -               /* Add missing tips */
> > -               oidset_insert_from_set(&missing_objects, &revs.missing_commits);
> > +       if (arg_missing_action == MA_PRINT ||
> > +           arg_missing_action == MA_PRINT_TYPE) {
> > +               struct oidset_iter iter;
> > +               struct object_id *oid;
> > +
> > +               oidmap_init(&missing_objects, DEFAULT_OIDSET_SIZE);
> > +               oidset_iter_init(&revs.missing_commits, &iter);
> > +
> > +               /*
> > +                * Revisions pointing to missing objects lack the context
> > +                * required to determine object type.
> > +                */
> > +               while ((oid = oidset_iter_next(&iter))) {
> > +                       struct missing_objects_map_entry *entry;
> > +
> > +                       CALLOC_ARRAY(entry, 1);
> > +                       oidcpy(&entry->entry.oid, oid);
> > +                       oidmap_put(&missing_objects, entry);
> > +               }
> 
> Using the function suggested above this could be:
> 
>         /*
>          * Revisions pointing to missing objects lack the context
>          * required to determine object type.
>          */
>         while ((oid = oidset_iter_next(&iter)))
>             add_missing_object_entry(oid, 0);
> 
> > +
> >                 oidset_clear(&revs.missing_commits);
> >         }
> >
> > @@ -801,12 +836,27 @@ int cmd_rev_list(int argc,
> >                 oidset_clear(&omitted_objects);
> >         }
> >         if (arg_missing_action == MA_PRINT) {
> > -               struct oidset_iter iter;
> > -               struct object_id *oid;
> > -               oidset_iter_init(&missing_objects, &iter);
> > -               while ((oid = oidset_iter_next(&iter)))
> > -                       printf("?%s\n", oid_to_hex(oid));
> > -               oidset_clear(&missing_objects);
> > +               struct missing_objects_map_entry *entry;
> > +               struct oidmap_iter iter;
> > +
> > +               oidmap_iter_init(&missing_objects, &iter);
> > +
> > +               while ((entry = oidmap_iter_next(&iter)))
> > +                       printf("?%s\n", oid_to_hex(&entry->entry.oid));
> > +
> > +               oidmap_free(&missing_objects, true);
> > +       }
> > +       if (arg_missing_action == MA_PRINT_TYPE) {
> > +               struct missing_objects_map_entry *entry;
> > +               struct oidmap_iter iter;
> > +
> > +               oidmap_iter_init(&missing_objects, &iter);
> > +
> > +               while ((entry = oidmap_iter_next(&iter)))
> > +                       printf("?%s %s\n", oid_to_hex(&entry->entry.oid),
> > +                              entry->type ? type_name(entry->type) : "");
> > +
> > +               oidmap_free(&missing_objects, true);
> >         }
> 
> Maybe a function like:
> 
> static void print_missing_objects(unsigned int with_type)
> {
>     struct missing_objects_map_entry *entry;
>     struct oidmap_iter iter;
> 
>     oidmap_iter_init(&missing_objects, &iter);
> 
>     while ((entry = oidmap_iter_next(&iter)))
>         if (with_type && entry->type)
>             printf("?%s %s\n", oid_to_hex(&entry->entry.oid),
>                    type_name(entry->type));
>         else
>             printf("?%s\n", oid_to_hex(&entry->entry.oid));
> 
>     oidmap_free(&missing_objects, true);
> }
> 
> could avoid some code duplication. It could also make the output a bit
> cleaner as when there is no type, there would be no space after the
> oid in the output.