Re: [PATCH 0/3] Re: [PATCH] unpack-tree.c: remove dead code
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
[PATCH 0/3] Re: [PATCH] unpack-tree.c: remove dead code
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*.
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?
[...]
> --- 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':
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.
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?
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
Re: [PATCH] unpack-tree.c: remove dead code
Stefan Beller writes:
> In line 1763 of unpack-tree.c we have a condition on the current tree
> if (current) {
> ...
> Within this block of code we can assume current to be non NULL, hence
> the code after the statement in line 1796:
> if (current)
> return ...
>
> cannot be reached.
>
> current/newtree/oldtree are used in the
> call to reject_merge() *only* for their path aka ce->name, and they
> all point at the same name (there is no rename funkies here); hence
> "all other failures" code path should just rely on current always
> being present.
>
> All referenced lines have been introduced in the same commit
> 076b0adc (2006-07-30, read-tree: move merge functions to the library),
> which was just moving the code around.
> The outer condition on the current tree (now in line 1763) was introduced
> in c859600954df4c292e, June 2005, [PATCH] read-tree: save more user hassles
> during fast-forward.
> The inner condition on the current tree was introduced in
> ee6566e8d70da682ac4926d, Sept. 2005, [PATCH] Rewrite read-tree
>
> This issue was found by coverity, Id:290002
>
> Signed-off-by: Stefan Beller
> Helped-by: Junio C Hamano
> ---
> unpack-trees.c | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> Did I understand you right, when changing to this one?
Something like that. I've already pushed out the original with a
tentative "SQUASH???" on top for today's integration; I'll try to
remember replacing them with this version.
Thanks.
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index c6aa8fb..42ee84e 100644
> --- 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 ;
> + }
> 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;
> + reject_merge(current, o);
> }
> }
> else if (newtree) {
--
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
[PATCH] unpack-tree.c: remove dead code
In line 1763 of unpack-tree.c we have a condition on the current tree
if (current) {
...
Within this block of code we can assume current to be non NULL, hence
the code after the statement in line 1796:
if (current)
return ...
cannot be reached.
current/newtree/oldtree are used in the
call to reject_merge() *only* for their path aka ce->name, and they
all point at the same name (there is no rename funkies here); hence
"all other failures" code path should just rely on current always
being present.
All referenced lines have been introduced in the same commit
076b0adc (2006-07-30, read-tree: move merge functions to the library),
which was just moving the code around.
The outer condition on the current tree (now in line 1763) was introduced
in c859600954df4c292e, June 2005, [PATCH] read-tree: save more user hassles
during fast-forward.
The inner condition on the current tree was introduced in
ee6566e8d70da682ac4926d, Sept. 2005, [PATCH] Rewrite read-tree
This issue was found by coverity, Id:290002
Signed-off-by: Stefan Beller
Helped-by: Junio C Hamano
---
unpack-trees.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
Did I understand you right, when changing to this one?
diff --git a/unpack-trees.c b/unpack-trees.c
index c6aa8fb..42ee84e 100644
--- 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 ;
+ }
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;
+ reject_merge(current, o);
}
}
else if (newtree) {
--
2.1.0.rc2
--
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
Re: [PATCH] unpack-tree.c: remove dead code
Stefan Beller writes:
> In line 1763 of unpack-tree.c we have a condition on the current tree
> if (current) {
> ...
> Within this block of code we can assume current to be non NULL, hence
> the code after the statement in line 1796:
> if (current)
> return ...
>
> cannot be reached.
>
> The proposed patch here changes the order of the current tree and the
> newtree part. I'm not sure if that's the right way to handle it.
If the existing code decides to reject the merge and falls into that
code path, src[0] aka current is not NULL at that point as you
noticed, and we would call reject_merge(current, o); we would keep
doing so after this "remove dead code" patch is applied.
If you remove the dead code, which are the inner check for current,
reject_merge() call with newtree and the final fallback of returning
-1, then it would be a faithful "remove dead code without changing
anything else" update.
Having said that, I think current/newtree/oldtree are used in the
call to reject_merge() *only* for their path aka ce->name, and they
all point at the same name (there is no rename funkies here); hence
"all other failures" code path should just rely on current always
being present and become something like this instead:
/* 20 or 21 */
...
} else if (o->gently) {
return -1;
} else {
return reject_merge(current, o);
}
Thanks.
--
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
[PATCH] unpack-tree.c: remove dead code
In line 1763 of unpack-tree.c we have a condition on the current tree
if (current) {
...
Within this block of code we can assume current to be non NULL, hence
the code after the statement in line 1796:
if (current)
return ...
cannot be reached.
The proposed patch here changes the order of the current tree and the
newtree part. I'm not sure if that's the right way to handle it.
All referenced lines have been introduced in the same commit
076b0adc (2006-07-30, read-tree: move merge functions to the library),
which was just moving the code around.
The outer condition on the current tree (now in line 1763) was introduced
in c859600954df4c292e, June 2005, [PATCH] read-tree: save more user hassles
during fast-forward.
The inner condition on the current tree was introduced in
ee6566e8d70da682ac4926d, Sept. 2005, [PATCH] Rewrite read-tree
This issue was found by coverity, Id:290002
Signed-off-by: Stefan Beller
---
unpack-trees.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index c6aa8fb..e6d37ff 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1793,11 +1793,10 @@ int twoway_merge(const struct cache_entry * const *src,
/* 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;
+ /* current is definitely exists here */
+ return o->gently ? -1 : reject_merge(current, o);
}
}
else if (newtree) {
--
2.1.0.rc2
--
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
