On 25/01/08 11:08AM, Christian Couder wrote:
> On Wed, Jan 8, 2025 at 4:43 AM Justin Tobler <jltobler@xxxxxxxxx> 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?
In [1], Junio suggested path information as another option.
>
> > 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@xxxxxxxxx>
> > ---
> > 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.
Since there is some other information that we may also want to print, I
think it makes sense to follow a more generic approach now. I'm
currently thinking could continue to use `--missing=print`, but add a
`--missing-attr` option to specify additional information we want to
print. Something like:
$ git rev-list --objects --missing=print \
--missing-attr=type --missing-attr=type
> > @@ -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.
Good idea, I'll factor this out in the next version.
>
> > 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.
I agree with this suggestion and will also factor this out in the next
version. Thanks!
-Justin
