Thread (128 messages) 128 messages, 6 authors, 2018-10-12

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

flat view

From: Jonathan Tan <hidden>
Date: 2018-08-10 19:04:00

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help