Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic

From: Jeff King <peff@peff.net>
Date: Apr 19, 2016, 05:03 UTC
Message-ID: <20160419050342.GA19439@sigill.intra.peff.net>
In-Reply-To: <1461013950-12503-3-git-send-email-sbeller@google.com>

On Mon, Apr 18, 2016 at 02:12:30PM -0700, Stefan Beller wrote:

Show 5 quoted lines

> +
> +		/*
> +		 * If a group can be moved back and forth, see if there is an
> +		 * blank line in the moving space. If there is a blank line,
> +		 * make sure the last blank line is the end of the group.

s/an/a/ on the first line

> +		 * As we shifted the group forward as far as possible, we only
> +		 * need to shift it back if at all.

Maybe because I'm reading it as a diff that only contains this hunk and not the whole rest of the function, but the "we" here confused me. You mean the earlier, existing loop in xdl_change_compact, right?

Maybe something like:

  As we already shifted the group forward as far as possible in the
  earlier loop...

would help.

Show 8 quoted lines

> +		if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
> +			while (ixs > 0 &&
> +			       !is_blank_line(recs, ix - 1, flags) &&
> +			       recs_match(recs, ixs - 1, ix - 1, flags)) {
> +				rchg[--ixs] = 1;
> +				rchg[--ix] = 0;
> +			}
> +		}

This turned out to be delightfully simple (especially compared to the perl monstrosity).

I tried comparing the output to the perl one, but it's not quite the same. In that one we had to work with the existing hunks and context lines, so any hunk that got shifted ended up with extra context on one side, and too little on the other. But here, we can actually bump the context lines to give the correct amount on both sides, which is good.

I guess this will invalidate old patch-ids, but there's not much to be done about that.

-Peff