Re: [PATCH 0/3] Re: [PATCH] unpack-tree.c: remove dead code
- From
Stefan Beller <stefanbeller@gmail.com>
- Date
- Aug 13, 2014, 06:41 UTC
- Message-ID
- <53EB0896.7060106@gmail.com>
- In-Reply-To
- <20140812235731.GD24621@google.com>
On 13.08.2014 01:57, Jonathan Nieder wrote:
Show 8 quoted lines
> 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!
Show 5 quoted lines
> > 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.
Show 13 quoted lines
>
> [...]
>> --- 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!
Show 34 quoted lines
>
> 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.
Show 30 quoted lines
> > 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
Show 13 quoted lines
> > 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(-) >