Re: [PATCH 1/2] submodule--helper: initial clone learns retry logic

From: Stefan Beller <sbeller@google.com>
Date: Jun 9, 2016, 19:47 UTC
Message-ID: <CAGZ79kbpurbwAB4H5_JPMsaGHmbeZA=EWjMOfQGB6apLhEXVeA@mail.gmail.com>
In-Reply-To: <xmqqy46egofw.fsf@gitster.mtv.corp.google.com>

On Thu, Jun 9, 2016 at 12:19 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> +static int compare_ce(const void *one, const void *two, void *cb_data)
>> +{
>> +     const struct cache_entry *ce_one = one, *ce_two = two;
>> +     return ce_two - ce_one;
>> +}
>
> This would work in practice, but I suspect that this is not ANSI-C
> kosher; as address comparison for ordering (not equality) is
> undefined if two pointers are not pointing into the same array or
> into the same struct's fields.

Uh :(

I thought about that for a while as I was about to send a 'simplification'
patch for commit.c:compare_commits_by_{author,commit}_date
There it made sense to keep it the way it is because
    sizeof(date) > sizeof(return value)

At the time of writing it made sense for this to be a subtractions, but I think we want to make it

    if (ce_one < ce_two)
        return 1;
    else if (ce_one > ce_two)
        return -1;
    else
        return 0;

instead. But that is still unspecified, so we rather go with

static int compare_ce(const void *one, const void *two, void *cb_data)
{
    const struct cache_entry *ce_one = one, *ce_two = two;
    return strcmp(ce_one->name, ce_two->name);
}

When reading the prio queue code correctly, we can be sure there is no NULL passed to either of one or two.

Show 5 quoted lines

>
> I think we have one or two other instances of such fishy pointer
> comparison already in the codebase, so it is not a show-stopper, but
> it would be better if this can be avoided and be replaced with
> something that I do not have to raise eyebrows at ;-)