Re: PATCH] remove duplicate #include directives

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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é




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux