From: Jacob Keller <jacob.keller@gmail.com>
Date: Fri, 15 Apr 2016 19:09:46 GMT
Subject: Re: [RFC PATCH, WAS: "weird diff output?" 1/2] xdiff: add xdl_hash_and_recmatch helper function
Message-ID: <CA+P7+xquEuu0aLb9vFaFhzun4YYW4PmiXycAt01HUxxUXqUuyQ@mail.gmail.com>
In-Reply-To: <xmqq60vidbsu.fsf@gitster.mtv.corp.google.com>

On Fri, Apr 15, 2016 at 10:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.
>

That makes sense. I like the sound of recs_match, and it's shorter too
which is nice.

Regards,
Jake