Re: [PATCH 6/6] diff.c: detect blocks despite whitespace changes

From: Stefan Beller <sbeller@google.com>
Date: Jun 28, 2017, 20:36 UTC
Message-ID: <CAGZ79kbsAi95+ZgjMem+MViKiXby7gZnGyg4Hbua5xw6TaPz_w@mail.gmail.com>
In-Reply-To: <xmqq37akya3j.fsf@gitster.mtv.corp.google.com>

On Tue, Jun 27, 2017 at 10:06 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Looking at the implementation of get_ws_cleaned_string() that is the
>> workhorse of emitted_symbol_cmp_no_ws(), it seems to be doing wrong
>> things for various "ignore whitespace" options (i.e. there is only
>> one implementation, while "git diff" family takes things like
>> "ignore space change", "ignore all whitespace", etc.), though.
>
> This probably deserves a bit more illustration of how I envision the
> code should evolve.
>
> In the longer term, I would prefer to see emitted_symbol_cmp_no_ws()
> to go and instead emitted_symbol_cmp() to take the diff options
> so
> that it can change the behaviour of the comparison function based on
> the -w/-b/--ignore-space-at-eol/etc. settings.  And compare two strings
> in place.

ok, in-place is no problem. But passing down the diff options into the compare function is a bit hard.

Originally I wanted to do that, see prep work in [1], but Jeff explained that the additional pointer in the compare function is **not** supposed to be a additional payload (such as the diff options specifying the white space options.)

[1] https://public-inbox.org/git/20170512200244.25245-1-sbeller@google.com/

However as we no settled on the struct emitted_diff_symbol, that has a 'flags' field in there, which ought to contain everything we know about whitespace settings, we should be able to do that from there.

>         emitted_symbol_eqv(struct emitted_diff_symbol *a,
>                            struct emitted_diff_symbol *b,
>                            const void *keydata) {
>                 struct diff_options *diffopt = keydata;

The prep work mentioned, would allow for this, as keydata would be passed through as-is in all calls of the hashmap API, such that the user can decide if they use it as the actual 'keydata' or rather as an additional payload.

Thanks for outlining the idea in code, but maybe we need to reconsider the hashmap API before that.

By not considering the change in the hashmap API, the current implementation tried to get away by having different compare functions.

Thanks, Stefan