Re: [PATCH] pull: fix 'git pull --all' when current branch is tracking remote that is not last in the list of remotes

Michael Lukashov [off-list ref] writes:

On Wed, Feb 24, 2010 at 2:02 AM, Junio C Hamano [off-list ref] wrote:

quoted

Michael Lukashov [off-list ref] writes:

quoted

diff --git a/git-pull.sh b/git-pull.sh
index 38331a8..fcde096 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -214,7 +214,11 @@ test true = "$rebase" && {

      done
 }
 orig_head=$(git rev-parse -q --verify HEAD)
-git fetch $verbosity --update-head-ok "$@" || exit 1
+if test -e "$GIT_DIR"/FETCH_HEAD
+then
+     rm "$GIT_DIR"/FETCH_HEAD 2>/dev/null
+fi

When is it sane to ignore an error from this "rm", especially after you
made sure that it exists?

The file "$GIT_DIR"/FETCH_HEAD is rewritten
in subsequent call to 'git fetch', thus it is safe to ignore all errors.

You are not answering my question.

You found out that the thing exists, and you want to overwrite it later.
You _need_ that file to either not exist, or at least be empty, because
you will be _appending_ to it, unlike the earlier code.

Now, you expected you would be able to remove it, and that is why you
called "rm".  Suppose that removal has failed for some reason.  The file
stays.  It is not emptied, either.

Why is it sane to ignore that error and let fetch --append to run, as if
it is starting from either non-existing file or an empty one?  You already
diagnosed that the file is in some _funny_ state.  It is not sensible to
continue further at that point, knowing that there is something wrong.

If the new code you introduced were

	rm -f "$GIT_DIR/FETCH_HEAD" || exit

then I would understand it.  But your patch doesn't make sense to me;
neither your "thus it is safe".

Steps to reproduce the bug:

	1. Create repository and add more than one remote
	2. Make sure current branch is tracking branch from the remote AND this remote
	   is not last in the list of remotes
	3. 'git pull --all' exits with error message:

		You asked to pull from the remote '--all', but did not specify
		a branch. Because this is not the default configured remote
		for your current branch, you must specify a branch on the command line.

After 'git pull --all' you need to run 'git pull' to update current branch.
This is annoying.

After this patch, 'git pull --all' does what it should do - fetches all changes
from all remotes and then updates current branch, if there were changes.

A minimal test case is added that reproduces the problem.
Tested under Windows and Debian GNU/Linux.

Signed-off-by: Michael Lukashov <redacted>
---
 builtin-fetch.c         |    6 +++++-
 git-pull.sh             |    6 +++++-
 t/t5521-pull-options.sh |   39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index d3b9d8a..8e54c5a 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c

@@ -784,13 +784,17 @@ static int add_remote_or_group(const char *name, struct string_list *list)
 static int fetch_multiple(struct string_list *list)
 {
 	int i, result = 0;
-	const char *argv[] = { "fetch", NULL, NULL, NULL, NULL, NULL, NULL };
+	const char *argv[] = { "fetch", NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL };
 	int argc = 1;
 
 	if (dry_run)
 		argv[argc++] = "--dry-run";
 	if (prune)
 		argv[argc++] = "--prune";
+	if (append)
+		argv[argc++] = "--append";
+	if (update_head_ok)
+		argv[argc++] = "--update-head-ok";
 	if (verbosity >= 2)
 		argv[argc++] = "-v";
 	if (verbosity >= 1)

diff --git a/git-pull.sh b/git-pull.sh
index 38331a8..2fbee42 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -214,7 +214,11 @@ test true = "$rebase" && {
 	done
 }
 orig_head=$(git rev-parse -q --verify HEAD)
-git fetch $verbosity --update-head-ok "$@" || exit 1
+if test -e "$GIT_DIR"/FETCH_HEAD
+then
+	rm -f "$GIT_DIR"/FETCH_HEAD || exit
+fi
+git fetch $verbosity --update-head-ok --append "$@" || exit 1
 
 curr_head=$(git rev-parse -q --verify HEAD)
 if test -n "$orig_head" && test "$curr_head" != "$orig_head"

diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 83e2e8a..2665caa 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh

@@ -4,6 +4,17 @@ test_description='pull options'
 
 . ./test-lib.sh
 
+setup_repository () {
+	mkdir "$1" && (
+	cd "$1" &&
+	git init &&
+	>file &&
+	git add file &&
+	test_tick &&
+	git commit -m "Initial"
+	)
+}
+
 D=`pwd`
 
 test_expect_success 'setup' '

@@ -57,4 +68,32 @@ test_expect_success 'git pull -q -v' '
 	test -s out
 '
 
+cd "$D"
+
+test_expect_success 'git pull --all' '
+	mkdir pullall &&
+	cd pullall &&
+	setup_repository remote1 &&
+	setup_repository remote2 &&
+	mkdir test &&
+	cd test &&
+	git init &&
+	git remote add remote1 "$D/pullall/remote1" &&
+	git remote add remote2 "$D/pullall/remote2" &&
+	(
+		# "git pull remote1" should print error message
+		# because there is no local branch that is tracking remote repo
+		git pull remote1
+		test $? = 1
+	) &&
+	(
+		# "git pull --all" should not print error message
+		# when current branch is tracking remote repo and that remote
+		# is not last in the list of remotes
+		git checkout -b remote1master remote1/master
+		git pull --all
+		test $? = 0
+	)
+'
+
 test_done

-- 
1.7.0.1706.g00cdbe

On 02/24/2010 02:07 PM, Michael Lukashov wrote:

+if test -e "$GIT_DIR"/FETCH_HEAD
+then
+	rm -f "$GIT_DIR"/FETCH_HEAD || exit
+fi

You do not need the if.

Paolo