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

On Mon, Aug 13, 2018 at 11:29 AM Jonathan Tan [off-list ref] wrote:

quoted

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

quoted

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

quoted

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