Re: [PATCH] apply: fix adding new files on i-t-a entries

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 24, 2015, 04:48 UTC
Message-ID: <CAPc5daXgEt==wpWH7CBXQFFZVSMuTR1PUMZqwQDmRG+QTJyt0g@mail.gmail.com>
In-Reply-To: <xmqqoak6e5dx.fsf@gitster.dls.corp.google.com>

On Tue, Jun 23, 2015 at 9:50 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 36 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> -     pos = cache_name_pos(name, strlen(name));
>> +     pos = exists_in_cache(name, strlen(name));
>
> Something that is named as if it would return yes/no that returns a
> real value is not a very welcome idea.
>
>> +/* This is the same as index_name_pos, except that i-t-a entries are invisible */
>> +int exists_in_index(const struct index_state *istate, const char *name, int namelen)
>> +{
>> +     int pos = index_name_stage_pos(istate, name, namelen, 0);
>> +
>> +     if (pos < 0)
>> +             return pos;
>> +     if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD)
>> +             return -pos-1;
>
> This is a useless complexity.  Your callers cannot use the returned
> value like this:
>
>         pos = exists_in_cache(...);
>         if (pos < 0) {
>                 if (active_cache[-pos-1]->ce_flags & CE_INTENT_TO_ADD)
>                         ; /* ah it actually exists but it is i-t-a */
>                 else
>                         ; /* no it does not really exist */
>         } else {
>                 ; /* yes it is really there at pos */
>         }
>
> because they cannot tell two cases apart: (1) you do have i-t-a with
> the given name, (2) you do not have the entry but the location you
> would insert an entry with such a name is occupied by an unrelated
> entry (i.e. with a name that sorts adjacent) that happens to be
> i-t-a.

Also, the callers cannot even use that return value in the usual way they would use the return value from index_name_pos(), either.

    pos = exists_in_cache(...);
    if (pos < 0) {
        /* ah, it does not exist, so... */
        pos = -1 - pos;
        /*
         * ... it is OK to shift active_cache[pos..] by one and add our
         * entry at active_cache[pos]
         */
   } else {
        /* it exists, so update in place */
        ;
   }

So, returning pos that smells like a return value from index_name_pos() only has an effect of confusing callers into buggy code, I am afraid. The callers that care need to be updated to check for ce_flags after finding the entry with index_name_pos() the usual way if you want to avoid search in the index_state->cache[] twice, and the callers that are only interested in knowing if an entry "exists" are better off with an exists_in_cache() that returns Yes/No and not a confusing and useless "pos", I think.