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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 21, 2015, 04:19 UTC
Message-ID: <xmqqbnjnaso7.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1426897692-18322-3-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 20 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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(...);
	}

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?