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.

In line 1763 of unpack-tree.c we have a condition on the current tree

Match the predominant style in git by following K&R style for if/else cascades. Documentation/CodingStyle from linux.git explains:

Note that the closing brace is empty on a line of its own, _except_ in the cases where it is followed by a continuation of the same statement, ie a "while" in a do-statement or an "else" in an if-statement, like this:

        if (x == y) {
                ..
        } else if (x > y) {
                ...
        } else {
                ....
        }

Rationale: K&R.

Also, note that this brace-placement also minimizes the number of empty (or almost empty) lines, without any loss of readability.

Signed-off-by Jonathan Nieder <[email protected]>

---
 unpack-trees.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

unpack-trees.c +4 -10

index f4a9aa9..187b15b 100644
@@ -1771,8 +1771,7 @@ int twoway_merge(const struct cache_entry * const *src,
                                        return merged_entry(newtree, current, o);
                        }
                        return o->gently ? -1 : reject_merge(current, o);
-               }
-               else if ((!oldtree && !newtree) || /* 4 and 5 */
+               } else if ((!oldtree && !newtree) || /* 4 and 5 */
                         (!oldtree && newtree &&
                          same(current, newtree)) || /* 6 and 7 */
                         (oldtree && newtree &&

@@ -1781,17 +1780,14 @@ int twoway_merge(const struct cache_entry * const *src,
                          !same(oldtree, newtree) && /* 18 and 19 */
                          same(current, newtree))) {
                        return keep_entry(current, o);
-               }
-               else if (oldtree && !newtree && same(current, oldtree)) {
+               } else if (oldtree && !newtree && same(current, oldtree)) {
                        /* 10 or 11 */
                        return deleted_entry(oldtree, current, o);
-               }
-               else if (oldtree && newtree &&
+               } else if (oldtree && newtree &&
                         same(current, oldtree) && !same(current, newtree)) {
                        /* 20 or 21 */
                        return merged_entry(newtree, current, o);
-               }
-               else
+               } else
                        return o->gently ? -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

twoway_merge() is missing an o->gently check in the case where a file that needs to be modified is missing from the index but present in the old and new trees. As a result, in this case 'git checkout -m' errors out instead of trying to perform a merge.

Fix it by checking o->gently. While at it, inline the o->gently check into reject_merge to prevent future call sites from making the same mistake.

Noticed by code inspection. The motivating case hasn't been tested.

Signed-off-by Jonathan Nieder <[email protected]>

---
This is the most iffy of the three patches, mostly because I was too lazy to write a test. I believe it's safe as-is nonetheless.

Thanks for reading.

 unpack-trees.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

unpack-trees.c +6 -6

index 187b15b..6c45af7 100644
@@ -1178,7 +1178,8 @@ return_failed:
 static int reject_merge(const struct cache_entry *ce,
                        struct unpack_trees_options *o)
 {
-       return add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);
+       return o->gently ? -1 :
+               add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);
 }
 
 static int same(const struct cache_entry *a, const struct cache_entry *b)

@@ -1633,7 +1634,7 @@ int threeway_merge(const struct cache_entry * const *stages,
        /* #14, #14ALT, #2ALT */
        if (remote && !df_conflict_head && head_match && !remote_match) {
                if (index && !same(index, remote) && !same(index, head))
-                       return o->gently ? -1 : reject_merge(index, o);
+                       return reject_merge(index, o);
                return merged_entry(remote, index, o);
        }
        /*

@@ -1641,7 +1642,7 @@ int threeway_merge(const struct cache_entry * const *stages,
         * make sure that it matches head.
         */
        if (index && !same(index, head))
-               return o->gently ? -1 : reject_merge(index, o);
+               return reject_merge(index, o);
 
        if (head) {
                /* #5ALT, #15 */

@@ -1770,7 +1771,7 @@ int twoway_merge(const struct cache_entry * const *src,
                                else
                                        return merged_entry(newtree, current, o);
                        }
-                       return o->gently ? -1 : reject_merge(current, o);
+                       return reject_merge(current, o);
                } else if ((!oldtree && !newtree) || /* 4 and 5 */
                         (!oldtree && newtree &&
                          same(current, newtree)) || /* 6 and 7 */

@@ -1788,7 +1789,7 @@ int twoway_merge(const struct cache_entry * const *src,
                        /* 20 or 21 */
                        return merged_entry(newtree, current, o);
                } else
-                       return o->gently ? -1 : reject_merge(current, o);
+                       return reject_merge(current, o);
        }
        else if (newtree) {
                if (oldtree && !o->initial_checkout) {
--

twoway_merge() is missing an o->gently check in the case where a file that needs to be modified is missing from the index but present in the old and new trees. As a result, in this case 'git checkout -m' errors out instead of trying to perform a merge.

This is the most iffy of the three patches, mostly because I was too lazy to write a test.

Quoted 4 lines · Jonathan Nieder

twoway_merge() is missing an o->gently check in the case where a file that needs to be modified is missing from the index but present in the old and new trees. As a result, in this case 'git checkout -m' errors out instead of trying to perform a merge.

I see two hunks in threeway_merge(), so two existing callers there will not change their behaviour. Two hunks in twoway_merge() means that among three existing callers in that function, this one at the end (not shown in your patch) changes its behaviour:

	else if (newtree) {
		if (oldtree && !o->initial_checkout) {
			/*
			 * deletion of the path was staged;
			 */
			if (same(oldtree, newtree))
				return 1;
			return reject_merge(oldtree, o);
		}
		return merged_entry(newtree, current, o);
	}
	return deleted_entry(oldtree, current, o);

Quoted 2 lines

This is the most iffy of the three patches, mostly because I was too lazy to write a test.

You would trigger this codepath by jumping from an old revision to a new revision after "git rm $path" any path that has been modified between the two. The only behaviour difference is that it will stop issuing an error message---the "checkout -m" will successfully switch between the revs and leave the index in a "we modified, they removed" conflicting state with or without your patch.

t/t7201-co.sh +0 -0

index 0c9ec0a..cedbb6a 100755
@@ -223,6 +223,23 @@ test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
 	test_cmp two expect
 '
 

Am 13.08.2014 20:59, schrieb Junio C Hamano:

Quoted 8 lines

t/t7201-co.sh +0 -0

index 0c9ec0a..cedbb6a 100755
@@ -223,6 +223,23 @@ test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
 	test_cmp two expect
 '
 

t/t7201-co.sh test_expect_success 'checkout --merge --conflict=diff3 <branch>' ' · line 235

Quoted 10 lines

t/t7201-co.sh +10 -0

+test_expect_success 'switch to another branch while carrying a deletion' '
+
+	git checkout -f master && git reset --hard && git clean -f &&
+	git rm two &&
+
+	test_must_fail git checkout simple 2>errs &&
+	test_i18ngrep overwritten errs &&
+
+	git checkout --merge simple 2>errs &&
+	! test_i18ngrep overwritten errs &&

This must be written as

	test_i18ngrep ! overwritten errs &&

I see a few wrong usages in the current code base. Here's a fix.

--- >8 ---
Subject: [PATCH] tests: fix negated test_i18ngrep calls

The helper function test_i18ngrep pretends that it found the expected results when it is running under GETTEXT_POISON. For this reason, it must not be used negated like so

! test_i18ngrep foo bar

because the test case would fail under GETTEXT_POISON. The function offers a special syntax to test that a pattern is *not* found:

test_i18ngrep ! foo bar

Convert incorrect uses to this syntax.

Signed-off-by Johannes Sixt <[email protected]>

---
 t/t4018-diff-funcname.sh | 8 ++++----
 t/t9800-git-p4-basic.sh  | 2 +-
 t/t9807-git-p4-submit.sh | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

t/t4018-diff-funcname.sh +4 -4

index 34591c2..1dbaa38 100755
@@ -52,15 +52,15 @@ do
 		echo "*.java diff=$p" >.gitattributes &&
 		test_expect_code 1 git diff --no-index \
 			A.java B.java 2>msg &&
-		! test_i18ngrep fatal msg &&
-		! test_i18ngrep error msg
+		test_i18ngrep ! fatal msg &&
+		test_i18ngrep ! error msg
 	'
 	test_expect_success "builtin $p wordRegex pattern compiles" '
 		echo "*.java diff=$p" >.gitattributes &&
 		test_expect_code 1 git diff --no-index --word-diff \
 			A.java B.java 2>msg &&
-		! test_i18ngrep fatal msg &&
-		! test_i18ngrep error msg
+		test_i18ngrep ! fatal msg &&
+		test_i18ngrep ! error msg
 	'
 done
 

t/t9800-git-p4-basic.sh +1 -1

index 665607c..5b56212 100755
@@ -145,7 +145,7 @@ test_expect_success 'exit when p4 fails to produce marshaled output' '
 		test_expect_code 1 git p4 clone --dest="$git" //depot >errs 2>&1
 	) &&
 	cat errs &&
-	! test_i18ngrep Traceback errs
+	test_i18ngrep ! Traceback errs
 '
 
 # Hide a file from p4d, make sure we catch its complaint.  This won't fail in

t/t9807-git-p4-submit.sh +1 -1

index 7fab2ed..1f74a88 100755
@@ -404,7 +404,7 @@ test_expect_success 'submit --prepare-p4-only' '
 		git p4 submit --prepare-p4-only >out &&
 		test_i18ngrep "prepared for submission" out &&
 		test_i18ngrep "must be deleted" out &&
-		! test_i18ngrep "everything below this line is just the diff" out
+		test_i18ngrep ! "everything below this line is just the diff" out
 	) &&
 	(
 		cd "$cli" &&

Quoted 1 line · Stefan Beller

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?

[...]

unpack-trees.c int twoway_merge(const struct cache_entry * const *src, · line 1794

Quoted 9 lines

unpack-trees.c +3 -0

@@ -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.

unpack-trees.c int twoway_merge(const struct cache_entry * const *src, · line 1800

Quoted 9 lines

unpack-trees.c +0 -8

 		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(-)

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 <[email protected]>

Helped-by Junio C Hamano <[email protected]>

---
 unpack-trees.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Did I understand you right, when changing to this one?

unpack-trees.c +4 -8

index c6aa8fb..42ee84e 100644
@@ -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) {