Re: [PATCH] setup.c: move statement under condition

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 27, 2017, 18:57 UTC
Message-ID: <xmqqvagsm227.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.21.1.1712232332000.406@MININT-6BKU6QN.europe.corp.microsoft.com>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 16 quoted lines

>> I suppose that if the condition is fulfilled then the previously
>> obtained value will not be necessary.
>
> I have to be honest: this commit message (including the subject) left me
> quite puzzled as to the intent of this patch.
>
> Maybe something like this would have spared me that puzzlement:
>
> 	Avoid unnecessary offset_1st_component() when prefixing filenames
>
> 	In the abspath_part_inside_repo() function that is called
> 	somewhere deep in the call-chain when prefixing paths, we
> 	calculate the offset of the first component, but under certain
> 	circumstances, the result is not even used.
>
> 	This patch changes the code to avoid that.

Sensible.

Show 22 quoted lines

>> diff --git a/setup.c b/setup.c
>> index 8cc34186c..1ce0189fa 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -35,7 +35,6 @@ static int abspath_part_inside_repo(char *path)
>>  		return -1;
>>  	wtlen = strlen(work_tree);
>>  	len = strlen(path);
>> -	off = offset_1st_component(path);
>>  
>>  	/* check if work tree is already the prefix */
>>  	if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
>> @@ -49,6 +48,8 @@ static int abspath_part_inside_repo(char *path)
>>  		}
>>  		/* work tree might match beginning of a symlink to work tree */
>>  		off = wtlen;
>> +	} else {
>> +		off = offset_1st_component(path);
>>  	}
>
> Up until recently, we encouraged dropping the curly brackets from
> single-line statements, but apparently that changed.

That is not quite correct.  We do encourage

	if (...)
		single statement;

with or without

	else
		another single statement;

IOW, when both sides do not need curlies, we save vertical space. On the other hand, when one side needs curlies, we tend to add to both to make it easier to spot the correspondence, i.e.

	if (...) {
		compond statement;
		compond statement;
	} else {
		single statement;
	}

or the other way around.

	if (...) {
		single statement;
	} else {
		compond statement;
		compond statement;
	}

Show 25 quoted lines

> However, we still encourage to put shorter alternative code paths
> (i.e. the blocks after `if` and `else`) first, in your case:
>
> @@ -35,18 +35,19 @@ static int abspath_part_inside_repo(char *path)
>  		return -1;
>  	wtlen = strlen(work_tree);
>  	len = strlen(path);
> -	off = offset_1st_component(path);
>  
>  	/* check if work tree is already the prefix */
> - 	if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
> +	if (wtlen > len || strncmp(path, work_tree, wtlen))
> +		off = offset_1st_component(path);
> +	else {
>  		if (path[wtlen] == '/') {
>  			memmove(path, path + wtlen + 1, len - wtlen);
>  			return 0;
> 		} else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
> 			/* work tree is the root, or the whole path */
> 			memmove(path, path + wtlen, len - wtlen + 1);
> 			return 0;
> 		}
> 		/* work tree might match beginning of a symlink to work * tree */
> 		off = wtlen;
> 	}

This also may allow you to further dedent the if/else chain and make the result easier to follow. I dunno.

Thanks for a review.