Re: [PATCH 0/3] Re: [PATCH] unpack-tree.c: remove dead code

[Stefan Beller <stefanbeller@gmail.com>]

On 13.08.2014 01:57, Jonathan Nieder wrote:
> Stefan Beller wrote:
> 
>> In line 1763 of unpack-tree.c we have a condition on the current tree
> [...]
> 
> The description is describing why the patch is *correct* (i.e., not
> going to introduce a bug), while what the reader wants to know is why
> the change is *desirable*.

Indeed. Thanks for the reminder!

> 
> Is this about making the code more readable, or robust, or suppressing
> a static analysis error, or something else?  What did the user or
> reader want to do that they couldn't do before and now can after this
> patch?

In my opinion it's making the code easier to read as there are less
lines of code with less conditionals.
The supression of a static code analysis warning is rather a desired
side effect, but not the main reason for the patch.


> 
> [...]
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -1789,15 +1789,11 @@ int twoway_merge(const struct cache_entry * const *src,
>>  			/* 20 or 21 */
>>  			return merged_entry(newtree, current, o);
>>  		}
>> +		else if (o->gently) {
>> +			return  -1 ;
>> +		}
> 
> (not about this patch) Elsewhere git uses the 'cuddled else':

Yes, I intentionally used this style, as the surrounding code was
using this style. You already added the reformatting follow up patch,
thanks!

> 
> 		if (foo) {
> 			...
> 		} else if (bar) {
> 			...
> 		} else {
> 			...
> 		}
> 
> That stylefix would be a topic for a different patch, though.
> 
>>  		else {
>> -			/* all other failures */
>> -			if (oldtree)
>> -				return o->gently ? -1 : reject_merge(oldtree, o);
>> -			if (current)
>> -				return o->gently ? -1 : reject_merge(current, o);
>> -			if (newtree)
>> -				return o->gently ? -1 : reject_merge(newtree, o);
>> -			return -1;
> 
> Does the static analysis tool support comments like
> 
> 			if (oldtree)
> 				...
> 			if (current)
> 				...
> 			...
> 
> 			/* not reached */
> 			return -1;
> 
> ?  That might be the simplest minimally invasive fix for what coverity
> pointed out.

I was looking for things like that, but either the
extensive documentation is well hidden or there is only short
tutorial-like documentation, which doesn't cover this case.


> 
> Now that we're looking there, though, it's worth understanding why we
> do the 'if oldtree exists, use it, else fall back to, etc' thing.  Was
> this meant as futureproofing in case commands like 'git checkout' want
> to do rename detection some day?
> 
> Everywhere else in the file that reject_merge is used, it is as
> 
> 	return o->gently ? -1 : reject_merge(..., o);
> 
> The one exception is
> 
> 	!current &&
> 	oldtree &&
> 	newtree &&
> 	oldtree != newtree &&
> 	!initial_checkout
> 
> (#17), which seems like a bug (it should have the same check).  Would
> it make sense to inline the o->gently check into reject_merge so callers
> don't have to care?
> 
> In that spirit, I suspect the simplest fix would be
> 
> 		else
> 			return o->gently ? -1 : reject_merge(current, o);
> 
> and then all calls could be replaced in a followup patch.
> 
> Sensible?

I need to read more code to follow.

Thanks for picking up my inital patch and improving. :)
Stefan

> 
> Thanks,
> 
> Jonathan Nieder (2):
>   unpack-trees: use 'cuddled' style for if-else cascade
>   checkout -m: attempt merge when deletion of path was staged
> 
> Stefan Beller (1):
>   unpack-trees: simplify 'all other failures' case
> 
>  unpack-trees.c | 31 ++++++++++---------------------
>  1 file changed, 10 insertions(+), 21 deletions(-)
>