Re: [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)

On Tue, May 26, 2009 at 10:09 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Greg Stark <greg(dot)stark(at)enterprisedb(dot)com> writes:
>> I'll repeat my suggestion that everyone poo-pooed: we can have the
>> mail list filters recognize patches, run filterdiff on them with our
>> prefered options, and attach the result as an additional attachment
>> (or link to some web directory).
>
> The argument that was made at the developer meeting is that the
> preferred way of working will be to apply the submitted patch in one's
> local git repository, and then do any needed editorialization as a
> second patch on top of it.  So the critical need as I see it is to be
> able to see a -c version of a patch-in-progress (ie, diff current
> working state versus some previous committed state).  Readability of the
> patch as-submitted is useful for quick eyeball checks, but I think all
> serious reviewing is going to be done on local copies.
>
>> I think this is actually all a red herring since it's pretty easy for
>> the reviewer to run filterdiff anyways.
>
> I don't trust filterdiff one bit :-(

For any particular reason, or just natural skepticism?

I believe there have been some wild-eyed claims tossed around in this
space previously that unified diffs don't provide all the same
information as context diffs, which is flatly false. AIUI, the reason
for the name "unified diff" is that it combines, or unifies, the
"before" and "after" versions of the code into a single chunk. The
nice thing about this is that when you have a bunch of small changes
in a file, you don't end up with all of the surrounding lines repeated
in both the "before" and "after" sections. If you change four
consecutive lines and run a unified diff, you end up with 4 +s, 4 -s,
and 6 lines of context (3 before and 3 after), for a total of 14
lines. If you run a context diff, you end up with 4 !s and 6 lines of
context in the before section and the same in the after section, for a
total of 20 lines, 6 of which are duplicated. This means that in many
cases you can see what's changed without having to page up and down in
the diff.

The not-so-nice thing about unified diffs is that when there is a huge
hunk of code that's changed, there are probably by chance a few
identical lines buried in there, like " }", so the + and - lines
end up mixed together in a way that wouldn't happen in a context diff
(which would turn the whole thing into two big "!" sections). It's no
problem for a machine to understand this, but it's hard to read for a
human being.

I haven't personally verified the filterdiff code, but the
transformation is pretty mechanical so I'm not sure why we should
believe that it hasn't been implemented correctly without some
evidence along those lines.

I don't think there's any way to make anyone 100% happy here. I
personally prefer unified diffs, so when I'm reviewing a complex patch
formatted as a context diff I typically apply it and then run a
unified diff using git. When I'm submitting a patch I use a unified
diff to check my work and then convert it to a context diff for
submission. On the other hand, I assume that, if you were presented
with a complex unified diff, would just apply it and then run a
context-diff to review it. Since, as you say, serious reviewing will
be done on local copies anyway, I really don't see the point of
worrying too much about how they're submitted to the mailing list.
Let's just tell everyone to keep using context diffs as the have been
doing, and if anyone doesn't then let's THROW THEIR PATCH ON THE
DUST-HEAP OF HISTORY AND HAUL THEM OUT TO BE DRAWN AND QUARTERED...
er, um, I mean, ask them not to do it that way the next time.

If there's an issue here that's worth getting worked up about, I'm not
seeing it.

...Robert