Re: [PATCH] diff: add --ignore-blank-lines option

Antoine Pelisse [off-list ref] writes:

quoted

quoted

+     unsigned long changes = ULONG_MAX;

Let me explain what "changes" means, as I know it will help the rest
of the message:
It counts the number of *added* blank lines we have ignored since
"lxch" (needed to calculate the distance between lxch and xch)
It also has the meaning of what was called "interesting" before.
If changes == ULONG_MAX, we are still in interesting zone, otherwise
it means we have ignored "changes" *added* blank lines (0 being a
valid value).

OK.  That deserves a comment next to this variable.

(Actually, After rereading this part, it looks like I could check that
lxch == xchp rather than setting changes to ULONG_MAX).

Yeah, I think so.

quoted

quoted

+             if (distance < max_ignorable &&
+                 (!xch->ignore || changes == ULONG_MAX)) {
+                     lxch = xch;
+                     changes = ULONG_MAX;

- If we are still in interesting zone, we take it, even if it's
ignorable change. Because it's close enough.
- Otherwise, only take real changes. We are close to another change,
and we are still in the loop, so it must be interesting.

OK.

quoted

quoted

+             } else if (changes != ULONG_MAX &&
+                        xch->i1 + changes - (lxch->i1 + lxch->chg1) > max_common) {
+                     break;

If we are no longer in "interesting zone" (changes != ULONG_MAX), it
means we will stop if the distance is too big.
"changes" is used in the calculation to consider the changes we have
already ignored (xch->i1 - (lxch->i1 + lxch->chg1) will only work if
xch and lxch are consecutive, we need to add the blank lines we
ignored).

And this uses max_common that is much larger than max_ignorable
because...?

The last interesting change, with its post context and inter hunk
gap, together with precontext for this one, is close enough to the
beginning of this one.  So it is understandable if xch by itself is
intereseting to use max_common.  Even an interesting one, if that is
so far from the last interesting one, should not be part of this
hunk.

However, if the current one is by itself uninteresting, should we
still use the max_common, or should this be compared with
max_ignorable?
    

quoted

Could you add comment to the "changes" variable and explain what the
variable means?

quoted

+             } else if (!xch->ignore) {
+                     lxch = xch;
+                     changes = ULONG_MAX;

When this change by itself is interesting, it becomes the "last
interesting one" and the hunk continues.

Exactly, and changes goes back to "interesting".

quoted

quoted

+             } else {
+                     if (changes == ULONG_MAX)
+                             changes = 0;
+                     changes += xch->chg2;

Puzzled beyond guessing.  Also it is curious why here and only here
we look at chg2 side of the things, not i1/chg1 in this whole thing.

chg2 being the number of blank line *additions*.

This is on the else side of if (!xch->ignore), so we are looking at
ignored hunk, which means there is only blank line change.  Can chg2
be 0 while chg1 is not zero, i.e. xch being a blank line removal?

What should happen in that case?  Don't we want to show it, for the
same reason we want to keep removal, as long as it is close enough
to the interesting zone?

Hope that makes things clearer,

Yes, it helped quite a bit.

quoted

quoted

quoted

+             } else if (changes != ULONG_MAX &&
+                        xch->i1 + changes - (lxch->i1 + lxch->chg1) > max_common) {
+                     break;

If we are no longer in "interesting zone" (changes != ULONG_MAX), it
means we will stop if the distance is too big.
"changes" is used in the calculation to consider the changes we have
already ignored (xch->i1 - (lxch->i1 + lxch->chg1) will only work if
xch and lxch are consecutive, we need to add the blank lines we
ignored).

And this uses max_common that is much larger than max_ignorable
because...?

The last interesting change, with its post context and inter hunk
gap, together with precontext for this one, is close enough to the
beginning of this one.  So it is understandable if xch by itself is
intereseting to use max_common.  Even an interesting one, if that is
so far from the last interesting one, should not be part of this
hunk.

However, if the current one is by itself uninteresting, should we
still use the max_common, or should this be compared with
max_ignorable?

Because of the "recursive definition", we don't know yet if an
ignorable change will be interesting or not.
We need to make sure it will be close to another interesting change first.
If it is, it will fall in the first if part, and lxch will catch-up.
If not, we will eventually be too far and break.

Re-reading note: OK, This last sentence ("If not we will eventually be
too far and break") is actually a bug. We might break before we find
something interesting while we should keep going. For example in such
a case, we should display like this, but won't:

@@ -x,x +x,x @@
+change   <--- That is lxch
 1
 2
 3
+       <--- Here we leave "interesting"
 4
 5
+       <--- We are too far and quit searching
 6
 7
+
 8
 9
+
 10
 11
+change

quoted

quoted

quoted

+             } else {
+                     if (changes == ULONG_MAX)
+                             changes = 0;
+                     changes += xch->chg2;

Puzzled beyond guessing.  Also it is curious why here and only here
we look at chg2 side of the things, not i1/chg1 in this whole thing.

chg2 being the number of blank line *additions*.

This is on the else side of if (!xch->ignore), so we are looking at
ignored hunk, which means there is only blank line change.  Can chg2
be 0 while chg1 is not zero, i.e. xch being a blank line removal?

Exactly. It can be a blank line removal. But I don't want to consider
it in the calculation.
Here's why:
We have:
1
2
3




4
5
6

and change it to:
change
1
2
3
4
5
6
change

What should be the output of diff --ignore-blank-lines ?

I chose this alternative:

@@ -1,3 +1,4 @@
+change
 1
 2
 3

@@ -7,3 +5,4 @@
 4
 5
 6
+change

While one could have chosen:

@@ -1,10 +1,8 @@
+change
 1
 2
 3
-
-
-
-
 4
 5
 6
+change

What should happen in that case?  Don't we want to show it, for the
same reason we want to keep removal, as long as it is close enough
to the interesting zone?

Nothing is interesting here, we just leave the interesting zone (if
not already left) because everything else failed.