Re: [PATCHv2 22/25] diff.c: color moved lines differently

On Fri, Jun 30, 2017 at 2:11 PM, Junio C Hamano [off-list ref] wrote:

quoted

+             return (int)' ';

Do we need a cast here?

No, I figured it is good to have it here explicitly, though.
We can drop that if you have strong preferences one way or another.

quoted

+static unsigned get_string_hash(struct emitted_diff_symbol *es, struct diff_options *o)
+{
+     if (o->xdl_opts & XDF_WHITESPACE_FLAGS) {
+             static struct strbuf sb = STRBUF_INIT;
+             const char *ap = es->line, *ae = es->line + es->len;
+             int c;
+
+             strbuf_reset(&sb);
+             while (ae > ap && isspace(*ae))
+                     ae--;

Not testing for the AT_EOL option here?

No, because the other options are stronger than the AT_EOL,
such that as you note it is still correct.

If in the future, we'd have another new option e.g.
IGNORE_TAB_BLANK_CONVERSION_BUT_WARN_ON_LENGTH_DIFF
(useful for python programmers ;)
this would break.

By the way, this is an unrelated tangent because I think you
inherited this pattern by copying and pasting from elsewhere, but I
think it would be better if we avoid casting the function pointer
type like this:

quoted

+             if (o->color_moved) {
+                     struct hashmap add_lines, del_lines;
+
+                     hashmap_init(&del_lines,
+                                  (hashmap_cmp_fn)moved_entry_cmp, o, 0);
+                     hashmap_init(&add_lines,
+                                  (hashmap_cmp_fn)moved_entry_cmp, o, 0);

When hashmap_cmp_fn's definition changes, these two calling sites
won't be caught as passing a incorrectly typed callback function by
the compiler.

Instead, we can match the actual implementation of the callback
function, e.g.

quoted

+static int moved_entry_cmp(const struct diff_options *diffopt,
+                        const struct moved_entry *a,
+                        const struct moved_entry *b,
+                        const void *keydata)
+{

to the expected function type, i.e.

static int moved_entry_cmp(const void *fndata,
                           const void *entry, const void *entry_or_key,
                           const void *keydata)
{
        const struct diff_options *diffopt = fndata;
        const struct moved_entry *a = entry;
        const struct moved_entry *b = entry_or_key;
        ...

by casting the parameters.

I agree.  I can make a cleanup throughout the whole code base,
but I would prefer if that is done in a separate series, as this
is already slightly lengthy.

Thanks,
Stefan