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