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(-)
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
