On 1/18/14, 9:38 PM, Andrew Dunstan wrote:
> On 01/18/2014 12:34 PM, Marko Tiikkaja wrote:
>> Is it possible for you to generate a diff that doesn't have all these
>> unrelated changes in it (from a pgindent run, I presume)? I just read
>> through three pagefuls and I didn't see any relevant changes, but I'm
>> not sure I want to keep doing that for the rest of the patch.
>>
>
> This seems to be quite overstated. The chunks in the version 3 patch
> that only contain pgindent effects are those at lines 751,771,866 and
> 1808 of json.c, by my reckoning. All the other changes are more than
> formatting.
Oh I see, there's a version 3 which improves things by a lot. I just
took the latest patch from the CF app and that was the v2 patch. Now
looking at it again, I see that it actually added new lines around
json.c:68, which I believe proves my point that reviewing such a patch
is hard.
> And undoing the pgindent changes and then individually applying all but
> those mentioned above would take me a lot of time.
v3 looks "ok". I would have preferred a patch with no unrelated
changes, but I can live with what we have there.
Something like the first three pagefuls of v2, however, would take *me*
a lot of time, which I believe is not acceptable. I don't care why a
patch has lots of unrelated stuff in it, I'm not going to waste my time
trying to figure out which parts are relevant and which aren't. That's
a lot of time wasted just to end up with a review possibly full of
missed problems and misunderstood code.
But I'll continue with my review now that this has been sorted out.
Regards,
Marko Tiikkaja