On Tue, Jun 23, 2015 at 9:50 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html