Re: [RFC PATCH 0/5] filter: support for excluding all trees and blobs

From: Jonathan Tan <jonathantanmy@google.com>
Date: Aug 10, 2018, 19:03 UTC
Message-ID: <20180810190352.62705-1-jonathantanmy@google.com>
In-Reply-To: <cover.1533854545.git.matvore@google.com>

Show 6 quoted lines

> Matthew DeVore (5):
>   revision: invert meaning of the USER_GIVEN flag
>   list-objects-filter: implement filter only:commits
>   list-objects: store common func args in struct
>   list-objects: refactor to process_tree_contents
>   rev-list: handle missing tree objects properly

Firstly, run every patch with "make DEVELOPER=1" - there is at least one "mixed declarations and code", which the Git coding style does not allow.

I've already replied to patches 1, 2, and 5. Patches 3 and 4 look OK to me and seem like good changes (patch 4, in addition to reducing indentation, also reduces the scope of the local variables - so it is a good change).

One last thing is that I'm not sure that this order of patches is the best order - in particular, if I run the tests at the 5th patch using a binary compiled at the 4th patch, I notice that cloning with "--filter=only:commits" fails with a cryptic error "fatal: bad tree object e891efadd67ca0c01b1c518a2fd91130d40f5904". This makes bisecting for errors difficult, but perhaps with this problem manifesting in only a few commits, it is not so bad.

The ideal order is to put patches 3-5 before 1-2. I've tried the rearrangement myself and found many instances where I had to rewrite code because one patch introduces "ctx" and the other, NOT_USER_GIVEN. So as a reviewer, I'm on the fence about suggesting that the patches be reordered.