Re: [PATCH v2 5/5] list-objects-filter: implement filter tree:none
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Aug 13, 2018, 18:29 UTC
- Message-ID
- <20180813182950.15723-1-jonathantanmy@google.com>
- In-Reply-To
- <0af4ef64aa52d76a597b4e97c0626c70fb2a883b.1533941986.git.matvore@google.com>
Show 22 quoted lines
> - 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?
Show 11 quoted lines
> +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
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.
Show 9 quoted lines
> +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.
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.)