threads / patch / 22798

patch, 2 partst5521: fix and modernize

Subject: [PATCH 1/2] t5521: fix and modernize

## tl;dr

7 messages between Feb 24, 2010 and Feb 24, 2010. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Junio C Hamano· Feb 24, 2010, 18:22 UTC · lore

All of these tests were bogus, as they created new directory and tried to run "git pull" without even running "git init" in there. They were mucking with the repository in $TEST_DIRECTORY.

While fixing it, modernize the style not to chdir around outside of subshell. Otherwise a failed test will take us to an unexpected directory and we need to chdir back to the test directory in each test, which is ugly and error prone.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5521-pull-options.sh |   46 ++++++++++++++++++++--------------------------
 1 files changed, 20 insertions(+), 26 deletions(-)
Show changes to t/t5521-pull-options.sh +20 −26
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 83e2e8a..c18d829 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -4,8 +4,6 @@ test_description='pull options'
 
 . ./test-lib.sh
 
-D=`pwd`
-
 test_expect_success 'setup' '
 	mkdir parent &&
 	(cd parent && git init &&
@@ -13,48 +11,44 @@ test_expect_success 'setup' '
 	 git commit -m one)
 '
 
-cd "$D"
-
 test_expect_success 'git pull -q' '
 	mkdir clonedq &&
-	cd clonedq &&
-	git pull -q "$D/parent" >out 2>err &&
-	test ! -s out
+	(cd clonedq && git init &&
+	git pull -q "../parent" >out 2>err &&
+	test ! -s err &&
+	test ! -s out)
 '
 
-cd "$D"
-
 test_expect_success 'git pull' '
 	mkdir cloned &&
-	cd cloned &&
-	git pull "$D/parent" >out 2>err &&
-	test -s out
+	(cd cloned && git init &&
+	git pull "../parent" >out 2>err &&
+	test -s err &&
+	test ! -s out)
 '
-cd "$D"
 
 test_expect_success 'git pull -v' '
 	mkdir clonedv &&
-	cd clonedv &&
-	git pull -v "$D/parent" >out 2>err &&
-	test -s out
+	(cd clonedv && git init &&
+	git pull -v "../parent" >out 2>err &&
+	test -s err &&
+	test ! -s out)
 '
 
-cd "$D"
-
 test_expect_success 'git pull -v -q' '
 	mkdir clonedvq &&
-	cd clonedvq &&
-	git pull -v -q "$D/parent" >out 2>err &&
-	test ! -s out
+	(cd clonedvq && git init &&
+	git pull -v -q "../parent" >out 2>err &&
+	test ! -s out &&
+	test ! -s err)
 '
 
-cd "$D"
-
 test_expect_success 'git pull -q -v' '
 	mkdir clonedqv &&
-	cd clonedqv &&
-	git pull -q -v "$D/parent" >out 2>err &&
-	test -s out
+	(cd clonedqv && git init &&
+	git pull -q -v "../parent" >out 2>err &&
+	test ! -s out &&
+	test -s err)
 '
 
 test_done

-- 
1.7.0.207.gac4ec
Junio C Hamano· Feb 24, 2010, 18:22 UTC · re: Junio C Hamano · lore

[PATCH 2/2] builtin-fetch --all/--multi: propagate options correctly

When running a subfetch, the code propagated some options but not others. Propagate --force, --update-head-ok and --keep options as well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-fetch.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
Show changes to builtin-fetch.c +8 −1
diff --git a/builtin-fetch.c b/builtin-fetch.c
index 8654fa7..61b2e40 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -784,13 +784,19 @@ 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[10] = { "fetch" };
 	int argc = 1;
 
 	if (dry_run)
 		argv[argc++] = "--dry-run";
 	if (prune)
 		argv[argc++] = "--prune";
+	if (update_head_ok)
+		argv[argc++] = "--update-head-ok";
+	if (force)
+		argv[argc++] = "--force";
+	if (keep)
+		argv[argc++] = "--keep";
 	if (verbosity >= 2)
 		argv[argc++] = "-v";
 	if (verbosity >= 1)
@@ -801,6 +807,7 @@ static int fetch_multiple(struct string_list *list)
 	for (i = 0; i < list->nr; i++) {
 		const char *name = list->items[i].string;
 		argv[argc] = name;
+		argv[argc + 1] = NULL;
 		if (verbosity >= 0)
 			printf("Fetching %s\n", name);
 		if (run_command_v_opt(argv, RUN_GIT_CMD)) {

-- 
1.7.0.207.gac4ec
Junio C Hamano· Feb 24, 2010, 19:02 UTC · re: Junio C Hamano · lore

[PATCH 3/3] fetch --all/--multiple: keep all the fetched branch information

Since "git fetch" learned "--all" and "--multiple" options, it has become tempting for users to say "git pull --all". Even though it may fetch from remotes that do not need to be fetched from for merging with the current branch, it is handy.

"git fetch" however clears the list of fetched branches every time it contacts a different remote. Unless the current branch is configured to merge with a branch from a remote that happens to be the last in the list of remotes that are contacted, "git pull" that fetches from multiple remotes will not be able to find the branch it should be merging with.

Make "fetch" clear FETCH_HEAD (unless --append is given) and then append the list of branches fetched to it (even when --apend is not given). That way, "pull" will be able to find the data for the branch being merged in FETCH_HEAD no matter where the remote appears in the list of remotes to be contacted by "git fetch".

Reported-by: Michael Lukashov
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This obviously builds on top of the earlier two clean-up patches.
 builtin-fetch.c         |   29 ++++++++++++++++++++++-------
 t/t5521-pull-options.sh |   18 ++++++++++++++++++
 2 files changed, 40 insertions(+), 7 deletions(-)
Show changes to 2 files +40 −7

builtin-fetch.c, t/t5521-pull-options.sh

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 61b2e40..b059d65 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -651,6 +651,17 @@ static void check_not_current_branch(struct ref *ref_map)
 			    "of non-bare repository", current_branch->refname);
 }
 
+static int truncate_fetch_head(void)
+{
+	char *filename = git_path("FETCH_HEAD");
+	FILE *fp = fopen(filename, "w");
+
+	if (!fp)
+		return error("cannot open %s: %s\n", filename, strerror(errno));
+	fclose(fp);
+	return 0;
+}
+
 static int do_fetch(struct transport *transport,
 		    struct refspec *refs, int ref_count)
 {
@@ -672,11 +683,9 @@ static int do_fetch(struct transport *transport,
 
 	/* if not appending, truncate FETCH_HEAD */
 	if (!append && !dry_run) {
-		char *filename = git_path("FETCH_HEAD");
-		FILE *fp = fopen(filename, "w");
-		if (!fp)
-			return error("cannot open %s: %s\n", filename, strerror(errno));
-		fclose(fp);
+		int errcode = truncate_fetch_head();
+		if (errcode)
+			return errcode;
 	}
 
 	ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);
@@ -784,8 +793,8 @@ 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[10] = { "fetch" };
-	int argc = 1;
+	const char *argv[11] = { "fetch", "--append" };
+	int argc = 2;
 
 	if (dry_run)
 		argv[argc++] = "--dry-run";
@@ -804,6 +813,12 @@ static int fetch_multiple(struct string_list *list)
 	else if (verbosity < 0)
 		argv[argc++] = "-q";
 
+	if (!append && !dry_run) {
+		int errcode = truncate_fetch_head();
+		if (errcode)
+			return errcode;
+	}
+
 	for (i = 0; i < list->nr; i++) {
 		const char *name = list->items[i].string;
 		argv[argc] = name;
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 84059d8..1b06691 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -72,4 +72,22 @@ test_expect_success 'git pull --force' '
 	)
 '
 
+test_expect_success 'git pull --all' '
+	mkdir clonedmulti &&
+	(cd clonedmulti && git init &&
+	cat >>.git/config <<-\EOF &&
+	[remote "one"]
+		url = ../parent
+		fetch = refs/heads/*:refs/remotes/one/*
+	[remote "two"]
+		url = ../parent
+		fetch = refs/heads/*:refs/remotes/two/*
+	[branch "master"]
+		remote = one
+		merge = refs/heads/master
+	EOF
+	git pull --all
+	)
+'
+
 test_done

-- 
1.7.0.207.gac4ec
Teemu Likonen· Feb 24, 2010, 19:07 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] fetch --all/--multiple: keep all the fetched branch information

* 2010-02-24 11:02 (-0800), Junio C. Hamano wrote:
Show 13 quoted lines
> Since "git fetch" learned "--all" and "--multiple" options, it has become
> tempting for users to say "git pull --all".  Even though it may fetch from
> remotes that do not need to be fetched from for merging with the current
> branch, it is handy.
>
> "git fetch" however clears the list of fetched branches every time it
> contacts a different remote.  Unless the current branch is configured to
> merge with a branch from a remote that happens to be the last in the list
> of remotes that are contacted, "git pull" that fetches from multiple
> remotes will not be able to find the branch it should be merging with.
>
> Make "fetch" clear FETCH_HEAD (unless --append is given) and then append
> the list of branches fetched to it (even when --apend is not given).  That
                                                ^^^^^^^
Not very important but there's a typo: "apend".
Show 6 quoted lines
> way, "pull" will be able to find the data for the branch being merged in
> FETCH_HEAD no matter where the remote appears in the list of remotes to be
> contacted by "git fetch".
>
> Reported-by: Michael Lukashov
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Michael Lukashov· Feb 24, 2010, 19:54 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] fetch --all/--multiple: keep all the fetched branch information

Patch 3 does not apply correctly:
Applying: fetch --all/--multiple: keep all the fetched branch information
error: patch failed: t/t5521-pull-options.sh:72
error: t/t5521-pull-options.sh: patch does not apply
Patch failed at 0001 fetch --all/--multiple: keep all the fetched
branch information
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
On Wed, Feb 24, 2010 at 10:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> Since "git fetch" learned "--all" and "--multiple" options, it has become
> tempting for users to say "git pull --all".  Even though it may fetch from
> remotes that do not need to be fetched from for merging with the current
> branch, it is handy.
>
Junio C Hamano· Feb 24, 2010, 19:59 UTC · re: Michael Lukashov · lore

Re: [PATCH 3/3] fetch --all/--multiple: keep all the fetched branch information

Michael Lukashov <michael.lukashov@gmail.com> writes:
> Patch 3 does not apply correctly:

It probably has a trivial confict, because there is an added test to the second patch since I sent it out. Please add this after applying [2/3].

Show changes to t/t5521-pull-options.sh +21 −0
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index c18d829..84059d8 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -51,4 +51,25 @@ test_expect_success 'git pull -q -v' '
 	test -s err)
 '
 
+test_expect_success 'git pull --force' '
+	mkdir clonedoldstyle &&
+	(cd clonedoldstyle && git init &&
+	cat >>.git/config <<-\EOF &&
+	[remote "one"]
+		url = ../parent
+		fetch = refs/heads/master:refs/heads/mirror
+	[remote "two"]
+		url = ../parent
+		fetch = refs/heads/master:refs/heads/origin
+	[branch "master"]
+		remote = two
+		merge = refs/heads/master
+	EOF
+	git pull two &&
+	test_commit A &&
+	git branch -f origin &&
+	git pull --all --force
+	)
+'
+
 test_done
Michael Lukashov· Feb 24, 2010, 20:28 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] fetch --all/--multiple: keep all the fetched branch information

Well, these patches solve the problem. Thanks for your work!

On Wed, Feb 24, 2010 at 10:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 38 quoted lines
> Michael Lukashov <michael.lukashov@gmail.com> writes:
>
>> Patch 3 does not apply correctly:
>
> It probably has a trivial confict, because there is an added test to the
> second patch since I sent it out.  Please add this after applying [2/3].
>
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index c18d829..84059d8 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -51,4 +51,25 @@ test_expect_success 'git pull -q -v' '
>        test -s err)
>  '
>
> +test_expect_success 'git pull --force' '
> +       mkdir clonedoldstyle &&
> +       (cd clonedoldstyle && git init &&
> +       cat >>.git/config <<-\EOF &&
> +       [remote "one"]
> +               url = ../parent
> +               fetch = refs/heads/master:refs/heads/mirror
> +       [remote "two"]
> +               url = ../parent
> +               fetch = refs/heads/master:refs/heads/origin
> +       [branch "master"]
> +               remote = two
> +               merge = refs/heads/master
> +       EOF
> +       git pull two &&
> +       test_commit A &&
> +       git branch -f origin &&
> +       git pull --all --force
> +       )
> +'
> +
>  test_done
>

← back to recent threads