Re: [PATCH v2 5/5] list-objects-filter: implement filter tree:none

[Matthew DeVore <matvore@google.com>]

On Mon, Aug 13, 2018 at 11:29 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > -     case LOFS_BEGIN_TREE:
> > -             assert(obj->type == OBJ_TREE);
> > -             /* always include all tree objects */
> > -             return LOFR_MARK_SEEN | LOFR_DO_SHOW;
> > -
> >       case LOFS_END_TREE:
> >               assert(obj->type == OBJ_TREE);
> >               return LOFR_ZERO;
> >
> > +     case LOFS_BEGIN_TREE:
> > +             assert(obj->type == OBJ_TREE);
> > +             if (!filter_data->omit_trees)
> > +                     return LOFR_MARK_SEEN | LOFR_DO_SHOW;
> > +
> > +             /*
> > +              * Fallthrough to insert into omitted list for trees as well as
> > +              * blobs.
> > +              */
> > +             /* fallthrough */
> >       case LOFS_BLOB:
> > -             assert(obj->type == OBJ_BLOB);
> >               assert((obj->flags & SEEN) == 0);
>
> After looking at the resulting file, I don't think saving a few lines of
> code (to add the OID, then return LOFR_MARK_SEEN) is worth rearranging
> the cases and falling through. Can you just add the OID-adding code to
> the LOFS_BEGIN_TREE case?

I've followed Jeff's suggestion of splitting up the functions, so
though the code is now redundant, it is ready for some future
improvements that are planned, and it's more clear and consistent with
the other filter functions.

>
> > +test_expect_success 'can use tree:none to filter partial clone' '
> > +     rm -rf dst &&
> > +     git clone --no-checkout --filter=tree:none "file://$(pwd)/srv.bare" dst &&
> > +     git -C dst rev-list master --missing=allow-any --objects >fetched_objects &&
> > +     cat fetched_objects \
> > +             | awk -f print_1.awk \
> > +             | xargs -n1 git -C dst cat-file -t >fetched_types &&
> > +     sort fetched_types -u >unique_types.observed &&
> > +     echo commit > unique_types.expected &&
> > +     test_cmp unique_types.observed unique_types.expected
> > +'
>
> We also need to verify that the resulting partial clone works - after
> all relevant tests, can you also ensure that:
>  - fsck works
>  - a cat-file on an indirectly missing tree works (i.e. if you have
>    commit -> A -> B and both A and B are missing, cat-file the B)
>  - fsck still works after the cat-file
Done - added it to t5616 in the last commit of the patchset.

>
> There is another potential issue about expanding the documentation of
> the pack protocol because we now support a new type of filter, but that
> is fine because the protocol currently points us to the rev-list
> documentation (which is updated). We probably need a way for clients to
> query servers about which filters they support, but that is definitely
> beyond the scope of this patch set.
>
> > +test_expect_success 'show missing tree objects with --missing=print' '
> > +     git -C dst rev-list master --missing=print --quiet --objects >missing_objs &&
> > +     sed "s/?//" missing_objs \
> > +             | xargs -n1 git -C srv.bare cat-file -t \
> > +             >missing_types &&
> > +     sort -u missing_types >missing_types.uniq &&
> > +     echo tree >expected &&
> > +     test_cmp missing_types.uniq expected
> > +'
>
> As stated in my review of patch 3, also test the other --missing
> arguments.
Done, mostly in t0410 in patch 3.

>
> Patches 1, 2, and 4 look good to me. (Writing this here so that I don't
> need to send one e-mail for each.)