Re: [PATCH 02/15] read-cache: Improve readability

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 20, 2015 at 9:19 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
>> ---
>>  read-cache.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/read-cache.c b/read-cache.c
>> index f72ea9f..769897e 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -703,9 +703,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>>                   !hashcmp(alias->sha1, ce->sha1) &&
>>                   ce->ce_mode == alias->ce_mode);
>>
>> -     if (pretend)
>> -             ;
>> -     else if (add_index_entry(istate, ce, add_option))
>> +     if (!pretend && add_index_entry(istate, ce, add_option))
>>               return error("unable to add %s to index",path);
>>       if (verbose && !was_same)
>>               printf("add '%s'\n", path);
>
> I have a moderately strong feeling against this change, as the code
> was done this way quite deliberately to keep it readable, namely, to
> avoid using && to concatenate two boolean expressions that are in
> totally different class inside condition part of if/while, where A
> is a precondition guard for B (i.e. you cannot evaluate B unless A
> holds) and B is called primarily for its side effect.  The problem
> is that, once you start liberally doing
>
>         if (A && B && C && D ...)
>
> with booleans with mixed semantics (guards and actions), it will
> quickly get harder to tell which one is which.
>
> I could have written it as
>
>         if (!pretend) {
>                 if (add_index_entry(...))
>                         return error(...);
>         }

This makes sense to point out the different semantics to me.
Maybe I have read too much of the refs code lately as there we
have these long chains which combine precondition with error
checking. :/ That's why I thought it would be global to git to not
care much about this semantics distinction.

>
> and that would have been just as readable as the original; it
> clearly separates the guard (i.e. only do the add-index thing when
> we are not pretending) and the operation that is done for the side
> effect.
>
> But I find the original tells you "if pretend mode, do *nothing*"
> and "otherwise, try add_index_entry() and act on its error" very
> clearly.  Of course, I am biased as the original is my code from
> 38ed1d89 ("git-add -n -u" should not add but just report,
> 2008-05-21).
>
> FYI, between preference and taste, I'd say this one is much closer
> to the latter than the former.
>
> By the way, aren't we leaking ce when we are merely pretending?

Yes we are, that's how I found this spot. (coverity pointed out ce was
leaking, so I was refactoring to actually make it easier to fix it, and then
heavily reordered the patch series afterwards. That spot was forgotten
apparently.

>
>
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]