Am 04.10.19 um 01:15 schrieb Junio C Hamano: > René Scharfe <l.s.r@xxxxxx> writes: > >> Found with "git grep '^#include ' '*.c' | sort | uniq -d". >> >> Signed-off-by: René Scharfe <l.s.r@xxxxxx> >> --- >> Patch formatted with --function-context for easier review. > > I have a mixed feelings about that. > > The only audience benefitted by --function-context patch are those > who read the patch only in MUA without looking at anything outside > and declare they are done with reviewing the patch. For something > tricky, wider context does help to see what is going on, but then > I'd feel uncomfortable to hear "looks good to me" from those who did > not even apply the patch to their trees and looked at the changes > after "reviewing" tricky stuff that requires wider context to > properly review. Shallow reviews can happen with any form of patch. The intent of --function-context is to provide meaningful context along with the patch, as the basis for a discussion on the mailing list. It works best for changes whose effects are constrained to within the affected functions, but have crucial information located outside the three default lines of context. An example would be a change at the end of a function for which a reviewer might need to know the type of some variables declared at the top. The price for that is that patches get longer, which eats up more precious reviewer bandwidth. That shouldn't affect those who apply the patch before review, though -- they can ignore any extra lines and have git am deal with them. > If there were topics in flight that touch any of these include > blocks, the patch would not apply and a reviewer who is interested > in these fixes ends up needing to wiggle them in somehow. Instructing git am or apply to ignore extra context lines using -C3 or similar would help in such a case. > Having said all that, for _this_ particular case, I do not see a > reason why a review needs to look at anywhere outside the context > presented in this patch, so I'd say it is a narrow case that -W is > an appropriate thing to use. Right, sometimes the context in a patch is sufficient to understand the contained change completely. This one here requires one more piece of information, though, namely our convention of wrapping header files in guard defines to make repeated includes of them no-ops. We do that for those removed by the patch, but we have a few exceptions to that rule in our repo (at least command-list.h, kwset.h, sha1dc_git.h, tar.h, unicode-width.h). So in that sense it's not such a good example of a self-sufficient patch. :) > I just do not want to see contributors > less experienced than you (hence cannot make good judgement on when > to and not to use the option) get in the habit of using -W all the > time. Providing full context (all the code, all dependencies) is impractical. Three-line context is an arbitrary amount that happens to work well surprisingly often. No context (as in the original diff format) can suffice sometimes, e.g. for typo fixes. Function context is a different point on the spectrum that has its own use cases. Patches of long functions would become tedious with -W, not sure if I'd be ready for that. A MUA with syntax highlighting for diffs would help a bit, I guess. René
