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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 17, 2013, 16:18 UTC
Message-ID: <7vzjuog175.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1371301305-30160-1-git-send-email-apelisse@gmail.com>

Antoine Pelisse <apelisse@gmail.com> writes:

> So here is a more thorough description of the option:

> - real changes are interesting

OK, I think I can understand it.

> - blank lines that are close enough (less than context size) to
>   interesting changes are considered interesting (recursive definition)

OK.

> - "context" lines are used around each hunk of interesting changes

OK.

> - If two hunks are separated by less than "inter-hunk-context", they
>   will be merged into one.

Makes sense.

> The current implementation does the "interesting changes selection" in a
> single pass.

"current" meaning "the code after this patch is applied"? Is there a possible future enhancement hinted here?

Show 15 quoted lines

> +xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg)
> +{
> +	xdchange_t *xch, *xchp, *lxch;
>  	long max_common = 2 * xecfg->ctxlen + xecfg->interhunkctxlen;
> +	long max_ignorable = xecfg->ctxlen;
> +	unsigned long changes = ULONG_MAX;
> +
> +	/* remove ignorable changes that are too far before other changes */
> +	for (xchp = *xscr; xchp && xchp->ignore; xchp = xchp->next) {
> +		xch = xchp->next;
> +
> +		if (xch == NULL ||
> +		    xch->i1 - (xchp->i1 + xchp->chg1) >= max_ignorable)
> +			*xscr = xch;
> +	}

This strips leading ignorable ones away until we see an unignorable one. Looks sane.

> +	if (*xscr == NULL)
> +		return NULL;
> +
> +	lxch = *xscr;

"lxch" remembers the last one that is "interesting".

> +	for (xchp = *xscr, xch = xchp->next; xch; xchp = xch, xch = xch->next) {
> +		long distance = xch->i1 - (xchp->i1 + xchp->chg1);
> +		if (distance > max_common)
>  			break;

If we see large-enough gap, the one we processed last (in xchp) is the end of the current hunk. Looks sane.

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

The current one is made into the "last interesting one we have seen" and the hunk continues, if either (1) the current one is interesting by itself, or (2) the last one we saw does not match some unexplainable criteria to cause changes set to not ULONG_MAX.

Puzzling.

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

If the last one we saw does not match some unexplainable criteria to cause changes set to not ULONG_MAX, and the distance between this one and the last "intersting" one is further than the context, this one will not be a part of the current hunk.

Puzzling.

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

> +		} 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.

> +		} 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.