threads / patch / 32182

v6, 9 partstransport-helper and fast-export fixes

Subject: [PATCH v6 p2 0/9] transport-helper and fast-export fixes

## tl;dr

16 messages between Nov 24, 2012 and Nov 28, 2012. Diffs are folded; open one to read it.

replies: 15people: 2as markdown or json

Felipe Contreras· Nov 24, 2012, 03:25 UTC · lore
Hi,

Trying to fix the remaining issues with transport-helper I stumbled upon a problem with the first patch attached. Now that the namespaced refs of the remote helper are properly tracked, there's a problem when pushing more than one ref at the same time *and* the last patch on the v5 patch series is applied.

The second patch here tries to solve that problem.
The rest are cleanups and trivial fixes.
Felipe Contreras (9):
  transport-helper: update remote helper namespace
  fast-export: don't handle uninteresting refs
  transport-helper: trivial code shuffle
  transport-helper: fix push without refspec
  transport-helper: fix pushing with straight refspec
  transport-helper: fix push without marks
  fast-export: make extra_refs global
  fast-export: refactor get_tags_and_duplicates()
  fast-export: trivial cleanups
 builtin/fast-export.c     | 99 +++++++++++++++++++++++++----------------------
 t/t5801-remote-helpers.sh | 14 +++++--
 t/t9350-fast-export.sh    | 30 ++++++++++++++
 transport-helper.c        | 39 ++++++++++++-------
 4 files changed, 120 insertions(+), 62 deletions(-)
-- 
1.8.0
Felipe Contreras· Nov 24, 2012, 03:25 UTC · re: Felipe Contreras · lore

[PATCH v6 p2 1/9] transport-helper: update remote helper namespace

When pushing, the remote namespace is updated correctly (e.g. refs/origin/master), but not the remote helper's (e.g. refs/testgit/origin/master).

This alone should not cause any regressions, but combined with other patches to handle negative refs correctly (upcoming patch), it might. However, that's a good thing; otherwise those issues would go unnoticed.

For the moment though, this patch alone shouldn't cause any issues, in fact the rest of the code seems to rely on this happening.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 transport-helper.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)
Show changes to transport-helper.c +18 −4
diff --git a/transport-helper.c b/transport-helper.c
index cfe0988..32ad877 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -10,6 +10,7 @@
 #include "string-list.h"
 #include "thread-utils.h"
 #include "sigchain.h"
+#include "refs.h"
 
 static int debug;
 
@@ -600,7 +601,7 @@ static int fetch(struct transport *transport,
 	return -1;
 }
 
-static void push_update_ref_status(struct strbuf *buf,
+static int push_update_ref_status(struct strbuf *buf,
 				   struct ref **ref,
 				   struct ref *remote_refs)
 {
@@ -651,7 +652,7 @@ static void push_update_ref_status(struct strbuf *buf,
 		*ref = find_ref_by_name(remote_refs, refname);
 	if (!*ref) {
 		warning("helper reported unexpected status of %s", refname);
-		return;
+		return 1;
 	}
 
 	if ((*ref)->status != REF_STATUS_NONE) {
@@ -660,11 +661,12 @@ static void push_update_ref_status(struct strbuf *buf,
 		 * status reported by the remote helper if the latter is 'no match'.
 		 */
 		if (status == REF_STATUS_NONE)
-			return;
+			return 1;
 	}
 
 	(*ref)->status = status;
 	(*ref)->remote_status = msg;
+	return 0;
 }
 
 static void push_update_refs_status(struct helper_data *data,
@@ -673,11 +675,23 @@ static void push_update_refs_status(struct helper_data *data,
 	struct strbuf buf = STRBUF_INIT;
 	struct ref *ref = remote_refs;
 	for (;;) {
+		char *private;
+
 		recvline(data, &buf);
 		if (!buf.len)
 			break;
 
-		push_update_ref_status(&buf, &ref, remote_refs);
+		if (push_update_ref_status(&buf, &ref, remote_refs))
+			continue;
+
+		if (!data->refspecs)
+			continue;
+
+		/* propagate back the update to the remote namespace */
+		private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
+		if (!private)
+			continue;
+		update_ref("update by helper", private, ref->new_sha1, NULL, 0, 0);
 	}
 	strbuf_release(&buf);
 }
-- 
1.8.0
Felipe Contreras· Nov 24, 2012, 03:28 UTC · re: Felipe Contreras · lore

Re: [PATCH v6 p2 1/9] transport-helper: update remote helper namespace

On Sat, Nov 24, 2012 at 4:25 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 22 quoted lines
> @@ -673,11 +675,23 @@ static void push_update_refs_status(struct helper_data *data,
>         struct strbuf buf = STRBUF_INIT;
>         struct ref *ref = remote_refs;
>         for (;;) {
> +               char *private;
> +
>                 recvline(data, &buf);
>                 if (!buf.len)
>                         break;
>
> -               push_update_ref_status(&buf, &ref, remote_refs);
> +               if (push_update_ref_status(&buf, &ref, remote_refs))
> +                       continue;
> +
> +               if (!data->refspecs)
> +                       continue;
> +
> +               /* propagate back the update to the remote namespace */
> +               private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
> +               if (!private)
> +                       continue;
> +               update_ref("update by helper", private, ref->new_sha1, NULL, 0, 0);
free(private); I guess
-- 
Felipe Contreras
Felipe Contreras· Nov 24, 2012, 03:25 UTC · re: Felipe Contreras · lore

[PATCH v6 p2 2/9] fast-export: don't handle uninteresting refs

They have been marked as UNINTERESTING for a reason, lets respect that.
Currently the first ref is handled properly, but not the rest:
  % git fast-export master ^uninteresting ^foo ^bar
  reset refs/heads/bar
  from :0
  reset refs/heads/foo
  from :0
  reset refs/heads/uninteresting
  from :0
  % git fast-export ^uninteresting ^foo ^bar master
  reset refs/heads/master
  from :0
  reset refs/heads/bar
  from :0
  reset refs/heads/foo
  from :0
Clearly this is wrong; the negative refs should be ignored.
After this patch:
  % git fast-export ^uninteresting ^foo ^bar master
  # nothing
  % git fast-export master ^uninteresting ^foo ^bar
  # nothing

And even more, it would only happen if the ref is pointing to exactly the same commit, but not otherwise:

 % git fast-export ^next next
 reset refs/heads/next
 from :0
 % git fast-export ^next next^{commit}
 # nothing
 % git fast-export ^next next~0
 # nothing
 % git fast-export ^next next~1
 # nothing
 % git fast-export ^next next~2
 # nothing

The reason this happens is that before traversing the commits, fast-export checks if any of the refs point to the same object, and any duplicated ref gets added to a list in order to issue 'reset' commands after the traversing. Unfortunately, it's not even checking if the commit is flagged as UNINTERESTING. The fix of course, is to do precisely that.

However, in order to do it properly we need to get the UNINTERESTING flag from the command line ref, not from the commit object. Fortunately we can simply use revs.pending, which contains all the information we need for get_tags_and_duplicates(), plus the ref flag. This way the rest of the positive refs will remain untouched; it's only the negative ones that change in behavior.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

The difference with the previous version is that now '^uninteresting ^bad master new' would handle 'master' and 'new', while the previous one would not do anything. The current behavior would handle 'bad', 'master', and 'new'.

 builtin/fast-export.c     | 11 +++++++----
 t/t5801-remote-helpers.sh |  8 ++++++++
 t/t9350-fast-export.sh    | 30 ++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 4 deletions(-)
Show changes to 3 files +45 −4

builtin/fast-export.c, t/t5801-remote-helpers.sh, t/t9350-fast-export.sh

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 31bfbee..77dffd1 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -474,18 +474,21 @@ static void handle_tag(const char *name, struct tag *tag)
 	       (int)message_size, (int)message_size, message ? message : "");
 }
 
-static void get_tags_and_duplicates(struct object_array *pending,
+static void get_tags_and_duplicates(struct rev_cmdline_info *info,
 				    struct string_list *extra_refs)
 {
 	struct tag *tag;
 	int i;
 
-	for (i = 0; i < pending->nr; i++) {
-		struct object_array_entry *e = pending->objects + i;
+	for (i = 0; i < info->nr; i++) {
+		struct rev_cmdline_entry *e = info->rev + i;
 		unsigned char sha1[20];
 		struct commit *commit;
 		char *full_name;
 
+		if (e->flags & UNINTERESTING)
+			continue;
+
 		if (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)
 			continue;
 
@@ -685,7 +688,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	if (import_filename && revs.prune_data.nr)
 		full_tree = 1;
 
-	get_tags_and_duplicates(&revs.pending, &extra_refs);
+	get_tags_and_duplicates(&revs.cmdline, &extra_refs);
 
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index b2782a2..2e027c8 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -158,4 +158,12 @@ test_expect_success 'push ref with existing object' '
 	compare_refs local dup server dup
 '
 
+test_expect_success 'push all with existing object' '
+	(cd local &&
+	git branch dup2 master &&
+	git push origin --all
+	) &&
+	compare_refs local dup2 server dup2
+'
+
 test_done
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 237d2e5..2e7187e 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -469,4 +469,34 @@ test_expect_success 'refs are updated even if no commits need to be exported' '
 	test_cmp expected actual
 '
 
+cat > expected << EOF
+blob
+mark :13
+data 5
+bump
+
+commit refs/heads/master
+mark :14
+author A U Thor <author@example.com> 1112912773 -0700
+committer C O Mitter <committer@example.com> 1112912773 -0700
+data 5
+bump
+from :12
+M 100644 :13 file
+
+EOF
+
+test_expect_success 'refs are updated even if no commits need to be exported' '
+	> tmp-marks &&
+	git fast-export --import-marks=tmp-marks \
+		--export-marks=tmp-marks master > /dev/null &&
+	git tag v1.0 &&
+	git branch uninteresting &&
+	echo bump > file &&
+	git commit -a -m bump &&
+	git fast-export --import-marks=tmp-marks \
+		--export-marks=tmp-marks ^uninteresting ^v1.0 master > actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.0
Felipe Contreras· Nov 24, 2012, 03:27 UTC · re: Felipe Contreras · lore

Re: [PATCH v6 p2 2/9] fast-export: don't handle uninteresting refs

On Sat, Nov 24, 2012 at 4:25 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 18 quoted lines
> +cat > expected << EOF
> +blob
> +mark :13
> +data 5
> +bump
> +
> +commit refs/heads/master
> +mark :14
> +author A U Thor <author@example.com> 1112912773 -0700
> +committer C O Mitter <committer@example.com> 1112912773 -0700
> +data 5
> +bump
> +from :12
> +M 100644 :13 file
> +
> +EOF
> +
> +test_expect_success 'refs are updated even if no commits need to be exported' '
The title should be updated: 'avoid uninteresting refs'
-- 
Felipe Contreras
Felipe Contreras· Nov 24, 2012, 03:25 UTC · re: Felipe Contreras · lore

[PATCH v6 p2 3/9] transport-helper: trivial code shuffle

Just shuffle the die() part to make it more explicit, and cleanup the code-style.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 transport-helper.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Show changes to transport-helper.c +3 −4
diff --git a/transport-helper.c b/transport-helper.c
index 32ad877..0c95101 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -775,6 +775,9 @@ static int push_refs_with_export(struct transport *transport,
 		char *private;
 		unsigned char sha1[20];
 
+		if (ref->deletion)
+			die("remote-helpers do not support ref deletion");
+
 		if (!data->refspecs)
 			continue;
 		private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
@@ -784,10 +787,6 @@ static int push_refs_with_export(struct transport *transport,
 		}
 		free(private);
 
-		if (ref->deletion) {
-			die("remote-helpers do not support ref deletion");
-		}
-
 		if (ref->peer_ref)
 			string_list_append(&revlist_args, ref->peer_ref->name);
 
-- 
1.8.0
Junio C Hamano· Nov 27, 2012, 17:00 UTC · re: Felipe Contreras · lore

Re: [PATCH v6 p2 3/9] transport-helper: trivial code shuffle

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 21 quoted lines
> Just shuffle the die() part to make it more explicit, and cleanup the
> code-style.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  transport-helper.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 32ad877..0c95101 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -775,6 +775,9 @@ static int push_refs_with_export(struct transport *transport,
>  		char *private;
>  		unsigned char sha1[20];
>  
> +		if (ref->deletion)
> +			die("remote-helpers do not support ref deletion");
> +
>  		if (!data->refspecs)
>  			continue;

This is not just "just shuffle the die to make it explicit" but it does change the semantics; earlier ref->deletion was perfectly fine as long as data->refspecs is not given, but the new code always dies.

If this semantic change is a good thing, please explain why it is so in the log message. If the change is "it does not matter because when data->refspecs is not given and ref->deletion is set, we die later elsewhere in the code anyway", then it needs to be described.

Thanks.
Show 10 quoted lines
> @@ -784,10 +787,6 @@ static int push_refs_with_export(struct transport *transport,
>  		}
>  		free(private);
>  
> -		if (ref->deletion) {
> -			die("remote-helpers do not support ref deletion");
> -		}
> -
>  		if (ref->peer_ref)
>  			string_list_append(&revlist_args, ref->peer_ref->name);
Felipe Contreras· Nov 28, 2012, 00:05 UTC · re: Junio C Hamano · lore

Re: [PATCH v6 p2 3/9] transport-helper: trivial code shuffle

On Tue, Nov 27, 2012 at 6:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 33 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Just shuffle the die() part to make it more explicit, and cleanup the
>> code-style.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  transport-helper.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/transport-helper.c b/transport-helper.c
>> index 32ad877..0c95101 100644
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -775,6 +775,9 @@ static int push_refs_with_export(struct transport *transport,
>>               char *private;
>>               unsigned char sha1[20];
>>
>> +             if (ref->deletion)
>> +                     die("remote-helpers do not support ref deletion");
>> +
>>               if (!data->refspecs)
>>                       continue;
>
> This is not just "just shuffle the die to make it explicit" but it
> does change the semantics; earlier ref->deletion was perfectly fine
> as long as data->refspecs is not given, but the new code always
> dies.
>
> If this semantic change is a good thing, please explain why it is so
> in the log message.  If the change is "it does not matter because
> when data->refspecs is not given and ref->deletion is set, we die
> later elsewhere in the code anyway", then it needs to be described.

refspecs are optional, but when they are not present the code doesn't work at all. This patch changes the behavior that was totally broken anyway.

-- 
Felipe Contreras
Junio C Hamano· Nov 28, 2012, 01:54 UTC · re: Felipe Contreras · lore

Re: [PATCH v6 p2 3/9] transport-helper: trivial code shuffle

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 13 quoted lines
>> This is not just "just shuffle the die to make it explicit" but it
>> does change the semantics; earlier ref->deletion was perfectly fine
>> as long as data->refspecs is not given, but the new code always
>> dies.
>>
>> If this semantic change is a good thing, please explain why it is so
>> in the log message.  If the change is "it does not matter because
>> when data->refspecs is not given and ref->deletion is set, we die
>> later elsewhere in the code anyway", then it needs to be described.
>
> refspecs are optional, but when they are not present the code doesn't
> work at all. This patch changes the behavior that was totally broken
> anyway.

In case it was not clear, I did not request/expect responses in the discussion thread, but a rerolled series with updated description.

Thanks.
Felipe Contreras· Nov 28, 2012, 02:15 UTC · re: Junio C Hamano · lore

Re: [PATCH v6 p2 3/9] transport-helper: trivial code shuffle

On Wed, Nov 28, 2012 at 2:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> This is not just "just shuffle the die to make it explicit" but it
>>> does change the semantics; earlier ref->deletion was perfectly fine
>>> as long as data->refspecs is not given, but the new code always
>>> dies.
>>>
>>> If this semantic change is a good thing, please explain why it is so
>>> in the log message.  If the change is "it does not matter because
>>> when data->refspecs is not given and ref->deletion is set, we die
>>> later elsewhere in the code anyway", then it needs to be described.
>>
>> refspecs are optional, but when they are not present the code doesn't
>> work at all. This patch changes the behavior that was totally broken
>> anyway.
>
> In case it was not clear, I did not request/expect responses in the
> discussion thread, but a rerolled series with updated description.

An updated description that is irrelevant; the stuff is totally broken, that's my point. But I'm tired of explaining it, and showing it with test cases, and patches that fix those test cases, it's not my itch, and "the other camp" doesn't even bother to acknowledge that indeed remote helpers without marks just don't work, or even utter a single word replying to one of these patches, nothing.

Not blaming you, just saying that I don't think anybody cares, clearly nobody is exercising this code.

Cheers.
-- 
Felipe Contreras
Felipe Contreras· Nov 24, 2012, 03:25 UTC · re: Felipe Contreras · lore

[PATCH v6 p2 4/9] transport-helper: fix push without refspec

The refspec feature is not mandatory.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5801-remote-helpers.sh | 2 +-
 transport-helper.c        | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)
Show changes to 2 files +4 −4

t/t5801-remote-helpers.sh, transport-helper.c

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 2e027c8..b268cd2 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -111,7 +111,7 @@ test_expect_success 'pulling without refspecs' '
 	compare_refs local2 HEAD server HEAD
 '
 
-test_expect_failure 'pushing without refspecs' '
+test_expect_success 'pushing without refspecs' '
 	test_when_finished "(cd local2 && git reset --hard origin)" &&
 	(cd local2 &&
 	echo content >>file &&
diff --git a/transport-helper.c b/transport-helper.c
index 0c95101..899eb36 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -778,6 +778,9 @@ static int push_refs_with_export(struct transport *transport,
 		if (ref->deletion)
 			die("remote-helpers do not support ref deletion");
 
+		if (ref->peer_ref)
+			string_list_append(&revlist_args, ref->peer_ref->name);
+
 		if (!data->refspecs)
 			continue;
 		private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
@@ -787,9 +790,6 @@ static int push_refs_with_export(struct transport *transport,
 		}
 		free(private);
 
-		if (ref->peer_ref)
-			string_list_append(&revlist_args, ref->peer_ref->name);
-
 	}
 
 	if (get_exporter(transport, &exporter, &revlist_args))
-- 
1.8.0
Felipe Contreras· Nov 24, 2012, 03:25 UTC · re: Felipe Contreras · lore

[PATCH v6 p2 5/9] transport-helper: fix pushing with straight refspec

For example '*:*'.

Obviously '^refs/heads/master refs/heads/master' is not going to work, so lets check that the ref we are negating is not the same we are pushing.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5801-remote-helpers.sh | 2 +-
 transport-helper.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

t/t5801-remote-helpers.sh, transport-helper.c

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index b268cd2..9c7871b 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -126,7 +126,7 @@ test_expect_success 'pulling with straight refspec' '
 	compare_refs local2 HEAD server HEAD
 '
 
-test_expect_failure 'pushing with straight refspec' '
+test_expect_success 'pushing with straight refspec' '
 	test_when_finished "(cd local2 && git reset --hard origin)" &&
 	(cd local2 &&
 	echo content >>file &&
diff --git a/transport-helper.c b/transport-helper.c
index 899eb36..6dbb72e 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -784,7 +784,7 @@ static int push_refs_with_export(struct transport *transport,
 		if (!data->refspecs)
 			continue;
 		private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
-		if (private && !get_sha1(private, sha1)) {
+		if (private && strcmp(private, ref->name) && !get_sha1(private, sha1)) {
 			strbuf_addf(&buf, "^%s", private);
 			string_list_append(&revlist_args, strbuf_detach(&buf, NULL));
 		}
-- 
1.8.0
Felipe Contreras· Nov 24, 2012, 03:25 UTC · re: Felipe Contreras · lore

[PATCH v6 p2 6/9] transport-helper: fix push without marks

There's not much to do when marks are not available, except pushing everything, so let's do so by avoiding the negative refs (e.g. ^refs/testgit/origin/master).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5801-remote-helpers.sh | 2 +-
 transport-helper.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

t/t5801-remote-helpers.sh, transport-helper.c

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 9c7871b..456303b 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -141,7 +141,7 @@ test_expect_success 'pulling without marks' '
 	compare_refs local2 HEAD server HEAD
 '
 
-test_expect_failure 'pushing without marks' '
+test_expect_success 'pushing without marks' '
 	test_when_finished "(cd local2 && git reset --hard origin)" &&
 	(cd local2 &&
 	echo content >>file &&
diff --git a/transport-helper.c b/transport-helper.c
index 6dbb72e..78e4e82 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -781,7 +781,7 @@ static int push_refs_with_export(struct transport *transport,
 		if (ref->peer_ref)
 			string_list_append(&revlist_args, ref->peer_ref->name);
 
-		if (!data->refspecs)
+		if (!data->refspecs || !data->import_marks)
 			continue;
 		private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
 		if (private && strcmp(private, ref->name) && !get_sha1(private, sha1)) {
-- 
1.8.0
Felipe Contreras· Nov 24, 2012, 03:25 UTC · re: Felipe Contreras · lore

[PATCH v6 p2 7/9] fast-export: make extra_refs global

There's no need to pass it around everywhere. This would make easier further refactoring that makes use of this variable.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
Show changes to builtin/fast-export.c +10 −11
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 77dffd1..5035382 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -30,6 +30,7 @@ static int fake_missing_tagger;
 static int use_done_feature;
 static int no_data;
 static int full_tree;
+static struct string_list extra_refs = STRING_LIST_INIT_NODUP;
 
 static int parse_opt_signed_tag_mode(const struct option *opt,
 				     const char *arg, int unset)
@@ -474,8 +475,7 @@ static void handle_tag(const char *name, struct tag *tag)
 	       (int)message_size, (int)message_size, message ? message : "");
 }
 
-static void get_tags_and_duplicates(struct rev_cmdline_info *info,
-				    struct string_list *extra_refs)
+static void get_tags_and_duplicates(struct rev_cmdline_info *info)
 {
 	struct tag *tag;
 	int i;
@@ -502,7 +502,7 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info,
 			/* handle nested tags */
 			while (tag && tag->object.type == OBJ_TAG) {
 				parse_object(tag->object.sha1);
-				string_list_append(extra_refs, full_name)->util = tag;
+				string_list_append(&extra_refs, full_name)->util = tag;
 				tag = (struct tag *)tag->tagged;
 			}
 			if (!tag)
@@ -532,20 +532,20 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info,
 		 * sure it gets properly updated eventually.
 		 */
 		if (commit->util || commit->object.flags & SHOWN)
-			string_list_append(extra_refs, full_name)->util = commit;
+			string_list_append(&extra_refs, full_name)->util = commit;
 		if (!commit->util)
 			commit->util = full_name;
 	}
 }
 
-static void handle_tags_and_duplicates(struct string_list *extra_refs)
+static void handle_tags_and_duplicates(void)
 {
 	struct commit *commit;
 	int i;
 
-	for (i = extra_refs->nr - 1; i >= 0; i--) {
-		const char *name = extra_refs->items[i].string;
-		struct object *object = extra_refs->items[i].util;
+	for (i = extra_refs.nr - 1; i >= 0; i--) {
+		const char *name = extra_refs.items[i].string;
+		struct object *object = extra_refs.items[i].util;
 		switch (object->type) {
 		case OBJ_TAG:
 			handle_tag(name, (struct tag *)object);
@@ -638,7 +638,6 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
 	struct object_array commits = OBJECT_ARRAY_INIT;
-	struct string_list extra_refs = STRING_LIST_INIT_NODUP;
 	struct commit *commit;
 	char *export_filename = NULL, *import_filename = NULL;
 	struct option options[] = {
@@ -688,7 +687,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	if (import_filename && revs.prune_data.nr)
 		full_tree = 1;
 
-	get_tags_and_duplicates(&revs.cmdline, &extra_refs);
+	get_tags_and_duplicates(&revs.cmdline);
 
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
@@ -704,7 +703,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	handle_tags_and_duplicates(&extra_refs);
+	handle_tags_and_duplicates();
 
 	if (export_filename)
 		export_marks(export_filename);
-- 
1.8.0
Felipe Contreras· Nov 24, 2012, 03:25 UTC · re: Felipe Contreras · lore

[PATCH v6 p2 8/9] fast-export: refactor get_tags_and_duplicates()

Split into a separate helper function get_commit() so that the part that finds the relevant commit, and the part that does something with it (handle tag object, etc.) are in different places.

No functional changes.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c | 68 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 30 deletions(-)
Show changes to builtin/fast-export.c +38 −30
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 5035382..d88fa10 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -475,9 +475,32 @@ static void handle_tag(const char *name, struct tag *tag)
 	       (int)message_size, (int)message_size, message ? message : "");
 }
 
+static struct commit *get_commit(struct rev_cmdline_entry *e, char *full_name)
+{
+	switch (e->item->type) {
+	case OBJ_COMMIT:
+		return (struct commit *)e->item;
+	case OBJ_TAG: {
+		struct tag *tag = (struct tag *)e->item;
+
+		/* handle nested tags */
+		while (tag && tag->object.type == OBJ_TAG) {
+			parse_object(tag->object.sha1);
+			string_list_append(&extra_refs, full_name)->util = tag;
+			tag = (struct tag *)tag->tagged;
+		}
+		if (!tag)
+			die("Tag %s points nowhere?", e->name);
+		return (struct commit *)tag;
+		break;
+	}
+	default:
+		return NULL;
+	}
+}
+
 static void get_tags_and_duplicates(struct rev_cmdline_info *info)
 {
-	struct tag *tag;
 	int i;
 
 	for (i = 0; i < info->nr; i++) {
@@ -492,41 +515,26 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info)
 		if (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)
 			continue;
 
-		switch (e->item->type) {
-		case OBJ_COMMIT:
-			commit = (struct commit *)e->item;
-			break;
-		case OBJ_TAG:
-			tag = (struct tag *)e->item;
-
-			/* handle nested tags */
-			while (tag && tag->object.type == OBJ_TAG) {
-				parse_object(tag->object.sha1);
-				string_list_append(&extra_refs, full_name)->util = tag;
-				tag = (struct tag *)tag->tagged;
-			}
-			if (!tag)
-				die ("Tag %s points nowhere?", e->name);
-			switch(tag->object.type) {
-			case OBJ_COMMIT:
-				commit = (struct commit *)tag;
-				break;
-			case OBJ_BLOB:
-				handle_object(tag->object.sha1);
-				continue;
-			default: /* OBJ_TAG (nested tags) is already handled */
-				warning("Tag points to object of unexpected type %s, skipping.",
-					typename(tag->object.type));
-				continue;
-			}
-			break;
-		default:
+		commit = get_commit(e, full_name);
+		if (!commit) {
 			warning("%s: Unexpected object of type %s, skipping.",
 				e->name,
 				typename(e->item->type));
 			continue;
 		}
 
+		switch(commit->object.type) {
+		case OBJ_COMMIT:
+			break;
+		case OBJ_BLOB:
+			handle_object(commit->object.sha1);
+			continue;
+		default: /* OBJ_TAG (nested tags) is already handled */
+			warning("Tag points to object of unexpected type %s, skipping.",
+				typename(commit->object.type));
+			continue;
+		}
+
 		/*
 		 * This ref will not be updated through a commit, lets make
 		 * sure it gets properly updated eventually.
-- 
1.8.0
Felipe Contreras· Nov 24, 2012, 03:25 UTC · re: Felipe Contreras · lore

[PATCH v6 p2 9/9] fast-export: trivial cleanups

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
Show changes to builtin/fast-export.c +1 −4
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d88fa10..bf5c822 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -548,7 +548,6 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info)
 
 static void handle_tags_and_duplicates(void)
 {
-	struct commit *commit;
 	int i;
 
 	for (i = extra_refs.nr - 1; i >= 0; i--) {
@@ -560,9 +559,7 @@ static void handle_tags_and_duplicates(void)
 			break;
 		case OBJ_COMMIT:
 			/* create refs pointing to already seen commits */
-			commit = (struct commit *)object;
-			printf("reset %s\nfrom :%d\n\n", name,
-			       get_object_mark(&commit->object));
+			printf("reset %s\nfrom :%d\n\n", name, get_object_mark(object));
 			show_progress();
 			break;
 		}
-- 
1.8.0

← back to recent threads