Re: [RFC PATCH, WAS: "weird diff output?" 1/2] xdiff: add xdl_hash_and_recmatch helper function

[Junio C Hamano <gitster@pobox.com>]

Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> It is a common pattern in xdl_change_compact to check that hashes and
> strings match. The resulting code to perform this change causes very
> long lines and makes it hard to follow the intention. Introduce a helper
> function xdl_hash_and_recmatch which performs both checks to increase
> code readability.

Think _why_ it is common to check hash and then do recmatch().  What
is the combination of two trying to compute?

How about calling it after "what" it computes, not after "how" it
computes it?  E.g.

    static int recs_match(xrecord_t **recs, long x, long y, long flags)

if we answer the above question "they try to see if two records match".
We could also go s/recs/lines/.

The xdl_recmatch() function appears in xutils.c, and over there the
functions do not use arrays of (xrecord_t *), so I think we are
better off without xdl_ prefix to avoid confusion.

> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  xdiff/xdiffi.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index 2358a2d6326e..fff97ac3e3c7 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -400,6 +400,14 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,
>  }
>  
>  
> +static int xdl_hash_and_recmatch(xrecord_t **recs, long ixs, long ix, long flags)
> +{
> +	return (recs[ixs]->ha == recs[ix]->ha &&
> +		xdl_recmatch(recs[ixs]->ptr, recs[ixs]->size,
> +			     recs[ix]->ptr, recs[ix]->size,
> +			     flags));
> +}
> +
>  int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>  	long ix, ixo, ixs, ixref, grpsiz, nrec = xdf->nrec;
>  	char *rchg = xdf->rchg, *rchgo = xdfo->rchg;
> @@ -442,8 +450,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>  			 * the last line of the current change group, shift backward
>  			 * the group.
>  			 */
> -			while (ixs > 0 && recs[ixs - 1]->ha == recs[ix - 1]->ha &&
> -			       xdl_recmatch(recs[ixs - 1]->ptr, recs[ixs - 1]->size, recs[ix - 1]->ptr, recs[ix - 1]->size, flags)) {
> +			while (ixs > 0 && xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {
>  				rchg[--ixs] = 1;
>  				rchg[--ix] = 0;
>  
> @@ -470,8 +477,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>  			 * the line next of the current change group, shift forward
>  			 * the group.
>  			 */
> -			while (ix < nrec && recs[ixs]->ha == recs[ix]->ha &&
> -			       xdl_recmatch(recs[ixs]->ptr, recs[ixs]->size, recs[ix]->ptr, recs[ix]->size, flags)) {
> +			while (ix < nrec && xdl_hash_and_recmatch(recs, ixs, ix, flags)) {
> +				emptylines += is_emptyline(recs[ix]->ptr);
> +
>  				rchg[ixs++] = 0;
>  				rchg[ix++] = 1;