[PATCH 0/3] Support for old:new remote-helper push

[PATCH 0/3] Support for old:new remote-helper push

[Felipe Contreras]

Hi,

In order to support pushing old:new refspecs in remote-helpers, the best way to
do what is to add an option to fast-export to rename the refs in the stream, so
here it is:

Felipe Contreras (3):
  fast-export: improve argument parsing
  fast-export: add new --refspec option
  transport-helper: add support for old:new refspec

 Documentation/git-fast-export.txt |  4 ++++
 builtin/fast-export.c             | 33 ++++++++++++++++++++++++++++++++-
 t/t5801-remote-helpers.sh         |  2 +-
 t/t9350-fast-export.sh            |  7 +++++++
 transport-helper.c                | 14 ++++++++++++--
 5 files changed, 56 insertions(+), 4 deletions(-)

-- 
1.8.3.rc1.553.gac13664

[PATCH 1/3] fast-export: improve argument parsing

[Felipe Contreras]

We don't want to pass arguments specific to fast-export to
setup_revisions.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d60d675..6e46057 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -686,8 +686,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	revs.topo_order = 1;
 	revs.show_source = 1;
 	revs.rewrite_parents = 1;
+	argc = parse_options(argc, argv, prefix, options, fast_export_usage,
+			PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);
 	argc = setup_revisions(argc, argv, &revs, NULL);
-	argc = parse_options(argc, argv, prefix, options, fast_export_usage, 0);
 	if (argc > 1)
 		usage_with_options (fast_export_usage, options);
 
-- 
1.8.3.rc1.553.gac13664

[PATCH 2/3] fast-export: add new --refspec option

[Felipe Contreras]

So that we can covert the exported ref names.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-fast-export.txt |  4 ++++
 builtin/fast-export.c             | 30 ++++++++++++++++++++++++++++++
 t/t9350-fast-export.sh            |  7 +++++++
 3 files changed, 41 insertions(+)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 03fc8c3..d1985d3 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -105,6 +105,10 @@ marks the same across runs.
 	in the commit (as opposed to just listing the files which are
 	different from the commit's first parent).
 
+--refspec::
+	Apply the specified refspec to each ref exported. Multiple of them can
+	be specified.
+
 [<git-rev-list-args>...]::
        A list of arguments, acceptable to 'git rev-parse' and
        'git rev-list', that specifies the specific objects and references
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 6e46057..0097103 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -17,6 +17,7 @@
 #include "utf8.h"
 #include "parse-options.h"
 #include "quote.h"
+#include "remote.h"
 
 static const char *fast_export_usage[] = {
 	N_("git fast-export [rev-list-opts]"),
@@ -30,6 +31,8 @@ static int fake_missing_tagger;
 static int use_done_feature;
 static int no_data;
 static int full_tree;
+static struct refspec *refspecs;
+static int refspecs_nr;
 
 static int parse_opt_signed_tag_mode(const struct option *opt,
 				     const char *arg, int unset)
@@ -502,6 +505,15 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info,
 		if (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)
 			continue;
 
+		if (refspecs) {
+			char *private;
+			private = apply_refspecs(refspecs, refspecs_nr, full_name);
+			if (private) {
+				free(full_name);
+				full_name = private;
+			}
+		}
+
 		switch (e->item->type) {
 		case OBJ_COMMIT:
 			commit = (struct commit *)e->item;
@@ -653,6 +665,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	struct commit *commit;
 	char *export_filename = NULL, *import_filename = NULL;
 	uint32_t lastimportid;
+	struct string_list refspecs_list;
 	struct option options[] = {
 		OPT_INTEGER(0, "progress", &progress,
 			    N_("show progress after <n> objects")),
@@ -673,6 +686,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN(0, "use-done-feature", &use_done_feature,
 			     N_("Use the done feature to terminate the stream")),
 		OPT_BOOL(0, "no-data", &no_data, N_("Skip output of blob data")),
+		OPT_STRING_LIST(0, "refspec", &refspecs_list, N_("refspec"),
+			     N_("Apply refspec to exported refs")),
 		OPT_END()
 	};
 
@@ -692,6 +707,19 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	if (argc > 1)
 		usage_with_options (fast_export_usage, options);
 
+	if (refspecs_list.nr) {
+		const char *refspecs_str[refspecs_list.nr];
+		int i;
+
+		for (i = 0; i < refspecs_list.nr; i++)
+			refspecs_str[i] = refspecs_list.items[i].string;
+
+		refspecs_nr = refspecs_list.nr;
+		refspecs = parse_fetch_refspec(refspecs_nr, refspecs_str);
+
+		string_list_clear(&refspecs_list, 1);
+	}
+
 	if (use_done_feature)
 		printf("feature done\n");
 
@@ -726,5 +754,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	if (use_done_feature)
 		printf("done\n");
 
+	free_refspec(refspecs_nr, refspecs);
+
 	return 0;
 }
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 2471bc6..ef2d76e 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -505,4 +505,11 @@ test_expect_success 'refs are updated even if no commits need to be exported' '
 	test_cmp expected actual
 '
 
+test_expect_success 'use refspec' '
+	git fast-export --refspec refs/heads/master:refs/heads/foobar master | \
+		grep "^commit " | sort | uniq > actual &&
+	echo "commit refs/heads/foobar" > expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.3.rc1.553.gac13664

[PATCH 3/3] transport-helper: add support for old:new refspec

[Felipe Contreras]

By using fast-export's new --refspec option.

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

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index dbb02e2..d15f794 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -92,7 +92,7 @@ test_expect_success 'push new branch by name' '
 	compare_refs local HEAD server refs/heads/new-name
 '
 
-test_expect_failure 'push new branch with old:new refspec' '
+test_expect_success 'push new branch with old:new refspec' '
 	(cd local &&
 	 git push origin new-name:new-refspec
 	) &&
diff --git a/transport-helper.c b/transport-helper.c
index 835815f..b1fdd39 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -787,7 +787,7 @@ static int push_refs_with_export(struct transport *transport,
 	struct ref *ref;
 	struct child_process *helper, exporter;
 	struct helper_data *data = transport->data;
-	struct string_list revlist_args = STRING_LIST_INIT_NODUP;
+	struct string_list revlist_args = STRING_LIST_INIT_DUP;
 	struct strbuf buf = STRBUF_INIT;
 
 	helper = get_helper(transport);
@@ -814,14 +814,24 @@ static int push_refs_with_export(struct transport *transport,
 			die("remote-helpers do not support ref deletion");
 		}
 
-		if (ref->peer_ref)
+		if (ref->peer_ref) {
+			if (strcmp(ref->name, ref->peer_ref->name)) {
+				struct strbuf buf = STRBUF_INIT;
+				strbuf_addf(&buf, "%s:%s", ref->peer_ref->name, ref->name);
+				string_list_append(&revlist_args, "--refspec");
+				string_list_append(&revlist_args, buf.buf);
+				strbuf_release(&buf);
+			}
 			string_list_append(&revlist_args, ref->peer_ref->name);
+		}
 
 	}
 
 	if (get_exporter(transport, &exporter, &revlist_args))
 		die("Couldn't run fast-export");
 
+	string_list_clear(&revlist_args, 1);
+
 	if (finish_command(&exporter))
 		die("Error while running fast-export");
 	push_update_refs_status(data, remote_refs);
-- 
1.8.3.rc1.553.gac13664

Re: [PATCH 1/3] fast-export: improve argument parsing

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> We don't want to pass arguments specific to fast-export to
> setup_revisions.

Interesting.  What bad things happen with the current order?

Does "fast-export --export-marks=foo" causes setup_revisions() to
mistakenly eat --export-marks=foo and barf?

>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/fast-export.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index d60d675..6e46057 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -686,8 +686,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
>  	revs.topo_order = 1;
>  	revs.show_source = 1;
>  	revs.rewrite_parents = 1;
> +	argc = parse_options(argc, argv, prefix, options, fast_export_usage,
> +			PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);
>  	argc = setup_revisions(argc, argv, &revs, NULL);
> -	argc = parse_options(argc, argv, prefix, options, fast_export_usage, 0);
>  	if (argc > 1)
>  		usage_with_options (fast_export_usage, options);

There is a SP between the function name and its arguments here ;-)

Re: [PATCH 2/3] fast-export: add new --refspec option

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> +test_expect_success 'use refspec' '
> +	git fast-export --refspec refs/heads/master:refs/heads/foobar master | \
> +		grep "^commit " | sort | uniq > actual &&

You do not need backslash after the pipe symbol at the end of line;
the shell knows you haven't finished speaking at that point.

The usual "pipe hides the error status of upstream commands" applies
here.  The command may die after writing enough to fill the pipe
buffer and showing the lines that begin with "commit".

Also it makes it harder to debug the test when something goes wrong.

By the way, don't you find that something does not feel quite right
with this command line?

    git fast-export --refspec=refs/heads/master:refs/heads/foobar master 

Why do we even have to say "master" at the end, when the other
option makes it clear that we are shipping "master" out?

Without thinking ramifications through, my gut feeling is that it
would feel more natural if we took:

    git fast-export master:foobar

to mean the same thing (which is what happens to the users of "git
push").  Is there a case where you have some ref on the left hand
side of the --refspec but you do not push out the history leading to
it?

With such an update, this part of the test would of course look
like:

	git fast-export master:foobar >actual.dump &&
        grep "^commit " actual.dump |
        sort -u >actual &&
        ...

and we do not need a new option.  Just a new extension to express
what gets pushed out.

But of course I may be missing some cases why there need to be a
separate option.

Re: [PATCH 2/3] fast-export: add new --refspec option

[Felipe Contreras]

On Thu, May 9, 2013 at 5:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> +test_expect_success 'use refspec' '
>> +     git fast-export --refspec refs/heads/master:refs/heads/foobar master | \
>> +             grep "^commit " | sort | uniq > actual &&
>
> You do not need backslash after the pipe symbol at the end of line;
> the shell knows you haven't finished speaking at that point.
>
> The usual "pipe hides the error status of upstream commands" applies
> here.  The command may die after writing enough to fill the pipe
> buffer and showing the lines that begin with "commit".
>
> Also it makes it harder to debug the test when something goes wrong.
>
> By the way, don't you find that something does not feel quite right
> with this command line?
>
>     git fast-export --refspec=refs/heads/master:refs/heads/foobar master
>
> Why do we even have to say "master" at the end, when the other
> option makes it clear that we are shipping "master" out?
>
> Without thinking ramifications through, my gut feeling is that it
> would feel more natural if we took:
>
>     git fast-export master:foobar
>
> to mean the same thing (which is what happens to the users of "git
> push").  Is there a case where you have some ref on the left hand
> side of the --refspec but you do not push out the history leading to
> it?
>
> With such an update, this part of the test would of course look
> like:
>
>         git fast-export master:foobar >actual.dump &&
>         grep "^commit " actual.dump |
>         sort -u >actual &&
>         ...
>
> and we do not need a new option.  Just a new extension to express
> what gets pushed out.
>
> But of course I may be missing some cases why there need to be a
> separate option.

Of course, but how do you implement that? That's mixing refspecs and
revlist arguments, which AFAIK don't mix:

% git fast-export ^next:new-next master:new-master --not
refs/tags/*:refs/tags/backup/*

-- 
Felipe Contreras

Re: [PATCH 1/3] fast-export: improve argument parsing

[Felipe Contreras]

On Thu, May 9, 2013 at 5:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> We don't want to pass arguments specific to fast-export to
>> setup_revisions.
>
> Interesting.  What bad things happen with the current order?
>
> Does "fast-export --export-marks=foo" causes setup_revisions() to
> mistakenly eat --export-marks=foo and barf?

No, apparently it skips them. But try 'git fast-export --export-marks
marks HEAD'.

>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  builtin/fast-export.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
>> index d60d675..6e46057 100644
>> --- a/builtin/fast-export.c
>> +++ b/builtin/fast-export.c
>> @@ -686,8 +686,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
>>       revs.topo_order = 1;
>>       revs.show_source = 1;
>>       revs.rewrite_parents = 1;
>> +     argc = parse_options(argc, argv, prefix, options, fast_export_usage,
>> +                     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);
>>       argc = setup_revisions(argc, argv, &revs, NULL);
>> -     argc = parse_options(argc, argv, prefix, options, fast_export_usage, 0);
>>       if (argc > 1)
>>               usage_with_options (fast_export_usage, options);
>
> There is a SP between the function name and its arguments here ;-)

Yeah, and I already did my part: I sent a patch to fix this style. Not
that it has anything to do with this patch.

-- 
Felipe Contreras

Re: [PATCH 2/3] fast-export: add new --refspec option

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Of course, but how do you implement that? That's mixing refspecs and
> revlist arguments, which AFAIK don't mix:

Simple.  You treat everything as refspecs and form revision ranges
out of them.  Note that that is exactly the reason why "git push"
can take "master" as a short-hand for "master:master" [*1*].

> % git fast-export ^next:new-next master:new-master --not
> refs/tags/*:refs/tags/backup/*

I thought you stopped mentioning the bottom of the range
(e.g. ^next) in the output from export stream at around 49266e8a11cf
(fast-export: don't handle uninteresting refs, 2012-11-28).

What does ^next:new-next (or mapping after "--not" in general) even
mean?  They would not make sense, would they?

So I would imagine you would be spelling that as:

    git fast-export master:new-master --not next refs/tags/*

or something, no?


[Footnote]

*1* Of course, unlike "git push", but similar to "git bundle",
    export does not know who the "receiving side" is and what they
    have, so in addition to the positives, you would need to tell
    the command where the bottoms of the range you are exporting
    are, so there needs to be some difference between the way "git
    push" and export/bundle specify the ranges.

    But that does not affect what should happen on the positive end
    of the ranges, which both "git push" and export/bundle need to
    specify anyway.

Re: [PATCH 1/3] fast-export: improve argument parsing

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, May 9, 2013 at 5:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> We don't want to pass arguments specific to fast-export to
>>> setup_revisions.
>>
>> Interesting.  What bad things happen with the current order?
>>
>> Does "fast-export --export-marks=foo" causes setup_revisions() to
>> mistakenly eat --export-marks=foo and barf?
>
> No, apparently it skips them. But try 'git fast-export --export-marks
> marks HEAD'.

That is the kind of thing that needs to be said, not in the
discussion but in the history, either in the log or in a new test,
or both.

Re: [PATCH 2/3] fast-export: add new --refspec option

[Felipe Contreras]

On Thu, May 9, 2013 at 6:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Of course, but how do you implement that? That's mixing refspecs and
>> revlist arguments, which AFAIK don't mix:
>
> Simple.  You treat everything as refspecs and form revision ranges
> out of them.  Note that that is exactly the reason why "git push"
> can take "master" as a short-hand for "master:master" [*1*].

And how do you implement that?

>> % git fast-export ^next:new-next master:new-master --not
>> refs/tags/*:refs/tags/backup/*
>
> I thought you stopped mentioning the bottom of the range
> (e.g. ^next) in the output from export stream at around 49266e8a11cf
> (fast-export: don't handle uninteresting refs, 2012-11-28).

That doesn't prevent the rev-list parsing from working.

> What does ^next:new-next (or mapping after "--not" in general) even
> mean?  They would not make sense, would they?

They don't, which is precisely my point.

> So I would imagine you would be spelling that as:
>
>     git fast-export master:new-master --not next refs/tags/*
>
> or something, no?

rev-list doesn't accept 'refs/tags/*'.

-- 
Felipe Contreras

Re: [PATCH 1/3] fast-export: improve argument parsing

[Felipe Contreras]

On Thu, May 9, 2013 at 6:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, May 9, 2013 at 5:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> We don't want to pass arguments specific to fast-export to
>>>> setup_revisions.
>>>
>>> Interesting.  What bad things happen with the current order?
>>>
>>> Does "fast-export --export-marks=foo" causes setup_revisions() to
>>> mistakenly eat --export-marks=foo and barf?
>>
>> No, apparently it skips them. But try 'git fast-export --export-marks
>> marks HEAD'.
>
> That is the kind of thing that needs to be said, not in the
> discussion but in the history, either in the log or in a new test,
> or both.

If only I had known that when I wrote the commit message.

-- 
Felipe Contreras

Re: [PATCH 2/3] fast-export: add new --refspec option

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, May 9, 2013 at 6:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Simple.  You treat everything as refspecs and form revision ranges
>> out of them.  Note that that is exactly the reason why "git push"
>> can take "master" as a short-hand for "master:master" [*1*].
> ...
>> So I would imagine you would be spelling that as:
>>
>>     git fast-export master:new-master --not next refs/tags/*
>>
>> or something, no?
>
> rev-list doesn't accept 'refs/tags/*'.

I think you misunderstood.  That is meant to illustrate what your
end users feed "fast-export". "fast-export" in turn expands that
into a revision range, which needs to happen anyway when it strips
:new-master from the positive end of the range to make the range
into

	master ^next ^refs/tags/v1.0 ^refs/tags/v1.1 ...

Re: [PATCH 2/3] fast-export: add new --refspec option

[Felipe Contreras]

On Thu, May 9, 2013 at 7:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, May 9, 2013 at 6:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Simple.  You treat everything as refspecs and form revision ranges
>>> out of them.  Note that that is exactly the reason why "git push"
>>> can take "master" as a short-hand for "master:master" [*1*].
>> ...
>>> So I would imagine you would be spelling that as:
>>>
>>>     git fast-export master:new-master --not next refs/tags/*
>>>
>>> or something, no?
>>
>> rev-list doesn't accept 'refs/tags/*'.
>
> I think you misunderstood.  That is meant to illustrate what your
> end users feed "fast-export". "fast-export" in turn expands that
> into a revision range, which needs to happen anyway when it strips
> :new-master from the positive end of the range to make the range
> into
>
>         master ^next ^refs/tags/v1.0 ^refs/tags/v1.1 ...

'git fast-export' accepts rev-list arguments, to make it do something
else would not only break existing users, but would require massive
work. I'm not interested in doing that.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 2/3] fast-export: add new --refspec option

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, May 9, 2013 at 6:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Simple.  You treat everything as refspecs and form revision ranges
>>> out of them.  Note that that is exactly the reason why "git push"
>>> can take "master" as a short-hand for "master:master" [*1*].
>> ...
>>> So I would imagine you would be spelling that as:
>>>
>>>     git fast-export master:new-master --not next refs/tags/*
>>>
>>> or something, no?
>>
>> rev-list doesn't accept 'refs/tags/*'.
>
> I think you misunderstood.  That is meant to illustrate what your
> end users feed "fast-export". "fast-export" in turn expands that
> into a revision range, which needs to happen anyway when it strips
> :new-master from the positive end of the range to make the range
> into
>
> 	master ^next ^refs/tags/v1.0 ^refs/tags/v1.1 ...

A few corrections and a related random thought.

 * The original --not "refs/tags/*:refs/tags/backup/*" fooled me. As
   my primary point was that an earlier change to fast-export made
   the uninteresting bottom not appear as reset in the output
   stream, I omitted everything after colon without much thought.

   If the current fast-export does not let you say "refs/tags/*" to
   export all the tags, we do not have to extend it to allow it to
   do so.

 * On the other hand, "git log 'fc/*'" might be a handy thing for
   any command that wants to have multiple starting points for
   revision traversal, so in principle I would not mind such an
   enhancement to rev-list machinery.

   But that does not have to be part of this series.

 * I alluded to the similarity between fast-export and bundle,
   meaning to hint that "git bundle" might also benefit from being
   able to say "record these refs under different name", but after
   thinking about it a bit more, I think it is unnecessary.

   The user of the bundle's output, either fetch or clone, can use
   the usual refspec mapping to use the data in any way it wants.
   If the bundle says refs/heads/master is at one commit, the _user_
   of that bundle can decide to map it to refs/remotes/origin/master
   (or refs/heads/origin, for that matter).

   It is a very powerful concept that we can generate data once,
   cast it in stone, and delay the decision on _how_ it is used
   until the last minute, much better than mapping at export time.
   So bundle does not need a similar refspec mechanism to map what
   it exports.  I haven't thought things through to see if the same
   logic applies to fast-export output (if so, it would mean it is
   better to allow the consumer of the stream take the refspec
   parameter and map the tips it finds in its input), though.

Re: [PATCH 1/3] fast-export: improve argument parsing

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, May 9, 2013 at 6:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> That is the kind of thing that needs to be said, not in the
>> discussion but in the history, either in the log or in a new test,
>> or both.
>
> If only I had known that when I wrote the commit message.

This is exactly what we review patches on the list for.  There is no
need to feel bad.  Nobody expects anybody to be perfect on the first
try.

Other people who may not know what the thought process of the author
was when the patch was written (or those who may not be as familiar
with the area in question as the author of the patch) can sometimes
spot a gap in the thought that is recorded in the patch.

Regarding the text of the patch in question, I am of two minds.  It
may solve the "--import-marks foo" to swap the orders, but I suspect
that may merely be shifting the problem and break rev-list options
(e.g. "--since 3.days") for the same reason.

In the longer term, setup_revisions() may need to allow its callers
to tell it what to do when it sees a parameter that it does not
understand.

There already is a similar cascading mechanism in place from
revision argument parsing to diff argument parsing (look for the
place where diff_opt_parse() is called in revision.c).  It may be
just the matter of adding a "struct option *" to rev_info and
calling parse_options_step() after diff_opt_parse() says "I dunno
about this option".

For the mechanism to be more flexible, we may want to give the
custom option table the caller of setup_revisions() gives us
precedence over revision/diff options, though.

Re: [PATCH 2/3] fast-export: add new --refspec option

[Johannes Sixt]

Am 5/10/2013 3:13, schrieb Junio C Hamano:
>  * On the other hand, "git log 'fc/*'" might be a handy thing for
>    any command that wants to have multiple starting points for
>    revision traversal, so in principle I would not mind such an
>    enhancement to rev-list machinery.

Currently, we spell this as "git log --branches=fc" or
"git log --branches='*export*'". But I do not mean to say that
"git log fc/\*" would be a bad thing to have.

-- Hannes

Re: [PATCH 2/3] fast-export: add new --refspec option

[Felipe Contreras]

On Thu, May 9, 2013 at 8:13 PM, Junio C Hamano <gitster@pobox.com> wrote:

>    It is a very powerful concept that we can generate data once,
>    cast it in stone, and delay the decision on _how_ it is used
>    until the last minute, much better than mapping at export time.
>    So bundle does not need a similar refspec mechanism to map what
>    it exports.  I haven't thought things through to see if the same
>    logic applies to fast-export output (if so, it would mean it is
>    better to allow the consumer of the stream take the refspec
>    parameter and map the tips it finds in its input), though.

It's possible to delay the renaming of refspecs to leave that duty to
the remote-helper, however, each and every remote-helper would need
advertise a new capability, and then implement the renaming. I think
it makes much more sense to implement something that just works for
free in all remote-helpers.

But I wasn't even that interested in this patch, I implemented it
because while I was helping the emacs folks I had to explain to them
that old:new didn't work, and I thought it would be trivial to make it
work.

I'm dropping this series. Somebody should at least make it so a proper
error is displayed so that the users are not left in the dark
wondering what the hell is going on.

-- 
Felipe Contreras

Re: [PATCH 0/3] Support for old:new remote-helper push

[Felipe Contreras]

On Wed, May 8, 2013 at 8:31 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:

> In order to support pushing old:new refspecs in remote-helpers, the best way to
> do what is to add an option to fast-export to rename the refs in the stream, so
> here it is:

I'm not going to work on this series any more.

-- 
Felipe Contreras