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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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