[PATCH v5 10/10] transport-helper: add support to delete branches

[PATCH v5 10/10] transport-helper: add support to delete branches

[Felipe Contreras]

For remote-helpers that use 'export' to push.

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

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 454337e..c667965 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -94,6 +94,14 @@ test_expect_success 'push new branch with old:new refspec' '
 	compare_refs local HEAD server refs/heads/new-refspec
 '
 
+test_expect_success 'push delete branch' '
+	(cd local &&
+	 git push origin :new-name
+	) &&
+	test_must_fail git --git-dir="server/.git" \
+	 rev-parse --verify refs/heads/new-name
+'
+
 test_expect_success 'forced push' '
 	(cd local &&
 	git checkout -b force-test &&
diff --git a/transport-helper.c b/transport-helper.c
index 84ff578..ef91882 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -875,9 +875,6 @@ 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");
-
 		private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
 		if (private && !get_sha1(private, sha1)) {
 			strbuf_addf(&buf, "^%s", private);
@@ -889,12 +886,16 @@ static int push_refs_with_export(struct transport *transport,
 		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);
+				if (!ref->deletion)
+					strbuf_addf(&buf, "%s:%s", ref->peer_ref->name, ref->name);
+				else
+					strbuf_addf(&buf, ":%s", 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 (!ref->deletion)
+				string_list_append(&revlist_args, ref->peer_ref->name);
 		}
 	}
 
-- 
1.8.4.2+fc1

[PATCH v5 00/10] transport-helper: updates

[Felipe Contreras]

Hi,

Here are the patches that allow transport helpers to be completely transparent;
renaming branches, deleting them, custom refspecs, --force, --dry-run,
reporting forced update, everything works.

Some of these were were sent before and rejected without a reason, but here
they are again in case anybody is interested.

Diff from v3:

diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index f1f4ca9..e75699c 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -437,6 +437,10 @@ set by Git if the remote helper has the 'option' capability.
 'option check-connectivity' \{'true'|'false'\}::
 	Request the helper to check connectivity of a clone.
 
+'option force' \{'true'|'false'\}::
+	Request the helper to perform a force update.  Defaults to
+	'false'.
+
 SEE ALSO
 --------
 linkgit:git-remote[1]
diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh
index 6d2f282..716aa4c 100755
--- a/git-remote-testgit.sh
+++ b/git-remote-testgit.sh
@@ -39,6 +39,7 @@ do
 		fi
 		test -n "$GIT_REMOTE_TESTGIT_SIGNED_TAGS" && echo "signed-tags"
 		test -n "$GIT_REMOTE_TESTGIT_NO_PRIVATE_UPDATE" && echo "no-private-update"
+		echo 'option'
 		echo
 		;;
 	list)
@@ -93,6 +94,7 @@ do
 		before=$(git for-each-ref --format=' %(refname) %(objectname) ')
 
 		git fast-import \
+			${force:+--force} \
 			${testgitmarks:+"--import-marks=$testgitmarks"} \
 			${testgitmarks:+"--export-marks=$testgitmarks"} \
 			--quiet
@@ -115,6 +117,20 @@ do
 
 		echo
 		;;
+	option\ *)
+		read cmd opt val <<-EOF
+		$line
+		EOF
+		case $opt in
+		force)
+			test $val = "true" && force="true" || force=
+			echo "ok"
+			;;
+		*)
+			echo "unsupported"
+			;;
+		esac
+		;;
 	'')
 		exit
 		;;
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index be543c0..c667965 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -102,6 +102,19 @@ test_expect_success 'push delete branch' '
 	 rev-parse --verify refs/heads/new-name
 '
 
+test_expect_success 'forced push' '
+	(cd local &&
+	git checkout -b force-test &&
+	echo content >> file &&
+	git commit -a -m eight &&
+	git push origin force-test &&
+	echo content >> file &&
+	git commit -a --amend -m eight-modified &&
+	git push --force origin force-test
+	) &&
+	compare_refs local refs/heads/force-test server refs/heads/force-test
+'
+
 test_expect_success 'cloning without refspec' '
 	GIT_REMOTE_TESTGIT_REFSPEC="" \
 	git clone "testgit::${PWD}/server" local2 2>error &&

Felipe Contreras (10):
  transport-helper: fix extra lines
  transport-helper: don't update refs in dry-run
  transport-helper: add 'force' to 'export' helpers
  transport-helper: check for 'forced update' message
  fast-export: improve argument parsing
  fast-export: add new --refspec option
  transport-helper: add support for old:new refspec
  fast-import: add support to delete refs
  fast-export: add support to delete refs
  transport-helper: add support to delete branches

 Documentation/git-fast-export.txt   |  4 ++++
 Documentation/git-fast-import.txt   |  3 +++
 Documentation/gitremote-helpers.txt |  4 ++++
 builtin/fast-export.c               | 47 ++++++++++++++++++++++++++++++++++++-
 fast-import.c                       | 13 +++++++---
 git-remote-testgit.sh               | 16 +++++++++++++
 t/t5801-remote-helpers.sh           | 23 +++++++++++++++++-
 t/t9300-fast-import.sh              | 18 ++++++++++++++
 t/t9350-fast-export.sh              | 18 ++++++++++++++
 transport-helper.c                  | 47 +++++++++++++++++++++++++------------
 10 files changed, 173 insertions(+), 20 deletions(-)

-- 
1.8.4.2+fc1

[PATCH v5 01/10] transport-helper: fix extra lines

[Felipe Contreras]

Commit 9c51558 (transport-helper: trivial code shuffle) moved these
lines above, but 99d9ec0 (Merge branch 'fc/transport-helper-no-refspec')
had a wrong merge conflict and readded them.

Reported-by: Richard Hansen <rhansen@bbn.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 transport-helper.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index b32e2d6..985eeea 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -874,9 +874,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) {
 			if (strcmp(ref->peer_ref->name, ref->name))
 				die("remote-helpers do not support old:new syntax");
-- 
1.8.4.2+fc1

[PATCH v5 08/10] fast-import: add support to delete refs

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-fast-import.txt |  3 +++
 fast-import.c                     | 13 ++++++++++---
 t/t9300-fast-import.sh            | 18 ++++++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 73f9806..c49ede4 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -483,6 +483,9 @@ Marks must be declared (via `mark`) before they can be used.
 * Any valid Git SHA-1 expression that resolves to a commit.  See
   ``SPECIFYING REVISIONS'' in linkgit:gitrevisions[7] for details.
 
+* The special null SHA-1 (40 zeros) specifices that the branch is to be
+  removed.
+
 The special case of restarting an incremental import from the
 current branch value should be written as:
 ----
diff --git a/fast-import.c b/fast-import.c
index f4d9969..fdce0b7 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -248,6 +248,7 @@ struct branch {
 	uintmax_t last_commit;
 	uintmax_t num_notes;
 	unsigned active : 1;
+	unsigned delete : 1;
 	unsigned pack_id : PACK_ID_BITS;
 	unsigned char sha1[20];
 };
@@ -1690,10 +1691,13 @@ static int update_branch(struct branch *b)
 	struct ref_lock *lock;
 	unsigned char old_sha1[20];
 
-	if (is_null_sha1(b->sha1))
-		return 0;
 	if (read_ref(b->name, old_sha1))
 		hashclr(old_sha1);
+	if (is_null_sha1(b->sha1)) {
+		if (b->delete)
+			delete_ref(b->name, old_sha1, 0);
+		return 0;
+	}
 	lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);
 	if (!lock)
 		return error("Unable to lock %s", b->name);
@@ -2620,8 +2624,11 @@ static int parse_from(struct branch *b)
 			free(buf);
 		} else
 			parse_from_existing(b);
-	} else if (!get_sha1(from, b->sha1))
+	} else if (!get_sha1(from, b->sha1)) {
 		parse_from_existing(b);
+		if (is_null_sha1(b->sha1))
+			b->delete = 1;
+	}
 	else
 		die("Invalid ref name or SHA1 expression: %s", from);
 
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 88fc407..f453388 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2999,4 +2999,22 @@ test_expect_success 'T: ls root tree' '
 	test_cmp expect actual
 '
 
+test_expect_success 'T: delete branch' '
+	git branch to-delete &&
+	git fast-import <<-EOF &&
+	reset refs/heads/to-delete
+	from 0000000000000000000000000000000000000000
+	EOF
+	test_must_fail git rev-parse --verify refs/heads/to-delete
+'
+
+test_expect_success 'T: empty reset doesnt delete branch' '
+	git branch not-to-delete &&
+	git fast-import <<-EOF &&
+	reset refs/heads/not-to-delete
+	EOF
+	git show-ref &&
+	git rev-parse --verify refs/heads/not-to-delete
+'
+
 test_done
-- 
1.8.4.2+fc1

[PATCH v5 05/10] 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 78250ea..eea5b8c 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -701,8 +701,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.4.2+fc1

[PATCH v5 06/10] fast-export: add new --refspec option

[Felipe Contreras]

So that we can convert 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 85f1f30..221506b 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 eea5b8c..b6f623e 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]"),
@@ -31,6 +32,8 @@ static int use_done_feature;
 static int no_data;
 static int full_tree;
 static struct string_list extra_refs = STRING_LIST_INIT_NODUP;
+static struct refspec *refspecs;
+static int refspecs_nr;
 
 static int parse_opt_signed_tag_mode(const struct option *opt,
 				     const char *arg, int unset)
@@ -525,6 +528,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;
+			}
+		}
+
 		commit = get_commit(e, full_name);
 		if (!commit) {
 			warning("%s: Unexpected object of type %s, skipping.",
@@ -668,6 +680,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 = STRING_LIST_INIT_NODUP;
 	struct option options[] = {
 		OPT_INTEGER(0, "progress", &progress,
 			    N_("show progress after <n> objects")),
@@ -688,6 +701,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(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()
 	};
 
@@ -707,6 +722,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");
 
@@ -741,5 +769,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 34c2d8f..dc6666f 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -504,4 +504,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.4.2+fc1

[PATCH v5 03/10] transport-helper: add 'force' to 'export' helpers

[Felipe Contreras]

Otherwise they cannot know when to force the push or not (other than
hacks).

Tests-by: Richard Hansen <rhansen@bbn.com>
Documentation-by: Richard Hansen <rhansen@bbn.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/gitremote-helpers.txt |  4 ++++
 git-remote-testgit.sh               | 16 ++++++++++++++++
 t/t5801-remote-helpers.sh           | 13 +++++++++++++
 transport-helper.c                  |  5 +++++
 4 files changed, 38 insertions(+)

diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index f1f4ca9..e75699c 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -437,6 +437,10 @@ set by Git if the remote helper has the 'option' capability.
 'option check-connectivity' \{'true'|'false'\}::
 	Request the helper to check connectivity of a clone.
 
+'option force' \{'true'|'false'\}::
+	Request the helper to perform a force update.  Defaults to
+	'false'.
+
 SEE ALSO
 --------
 linkgit:git-remote[1]
diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh
index 6d2f282..716aa4c 100755
--- a/git-remote-testgit.sh
+++ b/git-remote-testgit.sh
@@ -39,6 +39,7 @@ do
 		fi
 		test -n "$GIT_REMOTE_TESTGIT_SIGNED_TAGS" && echo "signed-tags"
 		test -n "$GIT_REMOTE_TESTGIT_NO_PRIVATE_UPDATE" && echo "no-private-update"
+		echo 'option'
 		echo
 		;;
 	list)
@@ -93,6 +94,7 @@ do
 		before=$(git for-each-ref --format=' %(refname) %(objectname) ')
 
 		git fast-import \
+			${force:+--force} \
 			${testgitmarks:+"--import-marks=$testgitmarks"} \
 			${testgitmarks:+"--export-marks=$testgitmarks"} \
 			--quiet
@@ -115,6 +117,20 @@ do
 
 		echo
 		;;
+	option\ *)
+		read cmd opt val <<-EOF
+		$line
+		EOF
+		case $opt in
+		force)
+			test $val = "true" && force="true" || force=
+			echo "ok"
+			;;
+		*)
+			echo "unsupported"
+			;;
+		esac
+		;;
 	'')
 		exit
 		;;
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 613f69a..c33cc25 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -94,6 +94,19 @@ test_expect_failure 'push new branch with old:new refspec' '
 	compare_refs local HEAD server refs/heads/new-refspec
 '
 
+test_expect_success 'forced push' '
+	(cd local &&
+	git checkout -b force-test &&
+	echo content >> file &&
+	git commit -a -m eight &&
+	git push origin force-test &&
+	echo content >> file &&
+	git commit -a --amend -m eight-modified &&
+	git push --force origin force-test
+	) &&
+	compare_refs local refs/heads/force-test server refs/heads/force-test
+'
+
 test_expect_success 'cloning without refspec' '
 	GIT_REMOTE_TESTGIT_REFSPEC="" \
 	git clone "testgit::${PWD}/server" local2 2>error &&
diff --git a/transport-helper.c b/transport-helper.c
index d05fc7c..ed238e5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -854,6 +854,11 @@ static int push_refs_with_export(struct transport *transport,
 			die("helper %s does not support dry-run", data->name);
 	}
 
+	if (flags & TRANSPORT_PUSH_FORCE) {
+		if (set_helper_option(transport, "force", "true") != 0)
+			die("helper %s does not support 'force'", data->name);
+	}
+
 	helper = get_helper(transport);
 
 	write_constant(helper->in, "export\n");
-- 
1.8.4.2+fc1

[PATCH v5 09/10] fast-export: add support to delete refs

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c  | 14 ++++++++++++++
 t/t9350-fast-export.sh | 11 +++++++++++
 2 files changed, 25 insertions(+)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index b6f623e..8ed41b4 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -673,6 +673,19 @@ static void import_marks(char *input_file)
 	fclose(f);
 }
 
+static void handle_deletes(void)
+{
+	int i;
+	for (i = 0; i < refspecs_nr; i++) {
+		struct refspec *refspec = &refspecs[i];
+		if (*refspec->src)
+			continue;
+
+		printf("reset %s\nfrom %s\n\n",
+				refspec->dst, sha1_to_hex(null_sha1));
+	}
+}
+
 int cmd_fast_export(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
@@ -762,6 +775,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	}
 
 	handle_tags_and_duplicates();
+	handle_deletes();
 
 	if (export_filename && lastimportid != last_idnum)
 		export_marks(export_filename);
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index dc6666f..ea6c96c 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -511,4 +511,15 @@ test_expect_success 'use refspec' '
 	test_cmp expected actual
 '
 
+test_expect_success 'delete refspec' '
+	git branch to-delete &&
+	git fast-export --refspec :refs/heads/to-delete to-delete ^to-delete > actual &&
+	cat > expected <<-EOF &&
+	reset refs/heads/to-delete
+	from 0000000000000000000000000000000000000000
+
+	EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.4.2+fc1

[PATCH v5 07/10] 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        | 13 ++++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index c33cc25..454337e 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -87,7 +87,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 5aba15c..84ff578 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -849,7 +849,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;
 
 	if (!data->refspecs)
@@ -887,8 +887,13 @@ static int push_refs_with_export(struct transport *transport,
 		free(private);
 
 		if (ref->peer_ref) {
-			if (strcmp(ref->peer_ref->name, ref->name))
-				die("remote-helpers do not support old:new syntax");
+			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);
 		}
 	}
@@ -896,6 +901,8 @@ static int push_refs_with_export(struct transport *transport,
 	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, flags);
-- 
1.8.4.2+fc1

[PATCH v5 04/10] transport-helper: check for 'forced update' message

[Felipe Contreras]

So the remote-helpers can tell us when a forced push was needed.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 transport-helper.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index ed238e5..5aba15c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -643,7 +643,7 @@ static int push_update_ref_status(struct strbuf *buf,
 				   struct ref *remote_refs)
 {
 	char *refname, *msg;
-	int status;
+	int status, forced = 0;
 
 	if (!prefixcmp(buf->buf, "ok ")) {
 		status = REF_STATUS_OK;
@@ -701,6 +701,11 @@ static int push_update_ref_status(struct strbuf *buf,
 			free(msg);
 			msg = NULL;
 		}
+		else if (!strcmp(msg, "forced update")) {
+			forced = 1;
+			free(msg);
+			msg = NULL;
+		}
 	}
 
 	if (*ref)
@@ -722,6 +727,7 @@ static int push_update_ref_status(struct strbuf *buf,
 	}
 
 	(*ref)->status = status;
+	(*ref)->forced_update = forced;
 	(*ref)->remote_status = msg;
 	return !(status == REF_STATUS_OK);
 }
-- 
1.8.4.2+fc1

[PATCH v5 02/10] transport-helper: don't update refs in dry-run

[Felipe Contreras]

The remote helper namespace should not be updated.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 transport-helper.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 985eeea..d05fc7c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -727,7 +727,8 @@ static int push_update_ref_status(struct strbuf *buf,
 }
 
 static void push_update_refs_status(struct helper_data *data,
-				    struct ref *remote_refs)
+				    struct ref *remote_refs,
+				    int flags)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct ref *ref = remote_refs;
@@ -741,7 +742,7 @@ static void push_update_refs_status(struct helper_data *data,
 		if (push_update_ref_status(&buf, &ref, remote_refs))
 			continue;
 
-		if (!data->refspecs || data->no_private_update)
+		if (flags & TRANSPORT_PUSH_DRY_RUN || !data->refspecs || data->no_private_update)
 			continue;
 
 		/* propagate back the update to the remote namespace */
@@ -832,7 +833,7 @@ static int push_refs_with_push(struct transport *transport,
 	sendline(data, &buf);
 	strbuf_release(&buf);
 
-	push_update_refs_status(data, remote_refs);
+	push_update_refs_status(data, remote_refs, flags);
 	return 0;
 }
 
@@ -886,7 +887,7 @@ static int push_refs_with_export(struct transport *transport,
 
 	if (finish_command(&exporter))
 		die("Error while running fast-export");
-	push_update_refs_status(data, remote_refs);
+	push_update_refs_status(data, remote_refs, flags);
 	return 0;
 }
 
-- 
1.8.4.2+fc1

Re: [PATCH v5 00/10] transport-helper: updates

[Max Horn]

[-- Attachment #1: Type: text/plain, Size: 549 bytes --]


On 31.10.2013, at 10:36, Felipe Contreras <felipe.contreras@gmail.com> wrote:

> Hi,
> 
> Here are the patches that allow transport helpers to be completely transparent;
> renaming branches, deleting them, custom refspecs, --force, --dry-run,
> reporting forced update, everything works.

I looked through this patch series in detail, and it looks fine to me. Indeed, it fixes several nuisances when using remote-helpers, and as such would be a definite win. In other words: Would be really nice to see these applied!


Cheers,
Max


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: [PATCH v5 00/10] transport-helper: updates

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

> On 31.10.2013, at 10:36, Felipe Contreras <felipe.contreras@gmail.com>
> wrote:
>
>> Hi,
>> 
>> Here are the patches that allow transport helpers to be completely
> transparent;
>> renaming branches, deleting them, custom refspecs, --force,
> --dry-run,
>> reporting forced update, everything works.
>
> I looked through this patch series in detail, and it looks fine to
> me. Indeed, it fixes several nuisances when using remote-helpers, and
> as such would be a definite win.

Nice.

> In other words: Would be really nice to see these applied!

The series is sitting on the 'pu' branch, and I think there were
some "fixup" suggestions during the review, so it may need to be
rerolled before advancing to 'next'.

Re: [PATCH v5 01/10] transport-helper: fix extra lines

[Junio C Hamano]

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

> Commit 9c51558 (transport-helper: trivial code shuffle) moved these
> lines above, but 99d9ec0 (Merge branch 'fc/transport-helper-no-refspec')
> had a wrong merge conflict and readded them.
>
> Reported-by: Richard Hansen <rhansen@bbn.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---

Ahh, sorry about that.  Will apply with a retitle.

>  transport-helper.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index b32e2d6..985eeea 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -874,9 +874,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) {
>  			if (strcmp(ref->peer_ref->name, ref->name))
>  				die("remote-helpers do not support old:new syntax");

Re: [PATCH v5 03/10] transport-helper: add 'force' to 'export' helpers

[Junio C Hamano]

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

> Otherwise they cannot know when to force the push or not (other than
> hacks).
> ...
> diff --git a/transport-helper.c b/transport-helper.c
> index d05fc7c..ed238e5 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -854,6 +854,11 @@ static int push_refs_with_export(struct transport *transport,
>  			die("helper %s does not support dry-run", data->name);
>  	}
>  
> +	if (flags & TRANSPORT_PUSH_FORCE) {
> +		if (set_helper_option(transport, "force", "true") != 0)
> +			die("helper %s does not support 'force'", data->name);
> +	}
> +

Does this cause a "git push --force $there A:B" to fail when $there
is a destination that goes via an existing helper does not suport
"force" option?

Should it fail even when the current value of B is an ancestor of A
(i.e. when an unforced push would succeed)?

Re: [PATCH v5 06/10] 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 &&

It feels somewhat redundant that you have to twice say that you are
pushing your "master", once with --refspec and then the branch
name.  Is this the best we can do?

If the answer is "yes", I guess it is OK to leave the external
interface to fast-export of this patch as-is, but if we can do
better, then we'd be better off without redundancy.

There are quite a few shell script style violations in this test, by
the way.  You do not want the trailing " \" after a pipe (the shell,
when seeing a line that ends with "|", knows you haven't finished
giving it the pipeline).  Also we do not put extra space between a
redirection operator and the target filename.



> +	echo "commit refs/heads/foobar" > expected &&
> +	test_cmp expected actual
> +'
> +
>  test_done

Re: [PATCH v5 00/10] transport-helper: updates

[Felipe Contreras]

On Thu, Oct 31, 2013 at 12:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Max Horn <max@quendi.de> writes:

>> In other words: Would be really nice to see these applied!
>
> The series is sitting on the 'pu' branch, and I think there were
> some "fixup" suggestions during the review, so it may need to be
> rerolled before advancing to 'next'.

The suggestions are applied, as you can see in the diff.

-- 
Felipe Contreras

Re: [PATCH v5 00/10] transport-helper: updates

[Junio C Hamano]

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

> Max Horn <max@quendi.de> writes:
>
>> On 31.10.2013, at 10:36, Felipe Contreras <felipe.contreras@gmail.com>
>> wrote:
>>
>>> Hi,
>>> 
>>> Here are the patches that allow transport helpers to be completely
>> transparent;
>>> renaming branches, deleting them, custom refspecs, --force,
>> --dry-run,
>>> reporting forced update, everything works.
>>
>> I looked through this patch series in detail, and it looks fine to
>> me. Indeed, it fixes several nuisances when using remote-helpers, and
>> as such would be a definite win.
>
> Nice.
>
>> In other words: Would be really nice to see these applied!
>
> The series is sitting on the 'pu' branch, and I think there were
> some "fixup" suggestions during the review, so it may need to be
> rerolled before advancing to 'next'.

Ah, I spoke too early; this is v5, updated from v4.

Re: [PATCH v5 00/10] transport-helper: updates

[Max Horn]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]


On 31.10.2013, at 19:10, Junio C Hamano <gitster@pobox.com> wrote:

> Max Horn <max@quendi.de> writes:
> 
>> On 31.10.2013, at 10:36, Felipe Contreras <felipe.contreras@gmail.com>
>> wrote:
>> 
>>> Hi,
>>> 
>>> Here are the patches that allow transport helpers to be completely
>> transparent;
>>> renaming branches, deleting them, custom refspecs, --force,
>> --dry-run,
>>> reporting forced update, everything works.
>> 
>> I looked through this patch series in detail, and it looks fine to
>> me. Indeed, it fixes several nuisances when using remote-helpers, and
>> as such would be a definite win.
> 
> Nice.
> 
>> In other words: Would be really nice to see these applied!
> 
> The series is sitting on the 'pu' branch, and I think there were
> some "fixup" suggestions during the review, so it may need to be
> rerolled before advancing to 'next'.

You are of course right. Next time I comment, I'll make sure to check whether previous review suggestions have been picked up.


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: [PATCH v5 06/10] fast-export: add new --refspec option

[Felipe Contreras]

On Thu, Oct 31, 2013 at 12:26 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 &&
>
> It feels somewhat redundant that you have to twice say that you are
> pushing your "master", once with --refspec and then the branch
> name.  Is this the best we can do?

As this has been discussed before and no other solution came forward, yes.

-- 
Felipe Contreras

Re: [PATCH v5 08/10] fast-import: add support to delete refs

[Max Horn]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]


On 31.10.2013, at 10:36, Felipe Contreras <felipe.contreras@gmail.com> wrote:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> Documentation/git-fast-import.txt |  3 +++
> fast-import.c                     | 13 ++++++++++---
> t/t9300-fast-import.sh            | 18 ++++++++++++++++++
> 3 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 73f9806..c49ede4 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -483,6 +483,9 @@ Marks must be declared (via `mark`) before they can be used.
> * Any valid Git SHA-1 expression that resolves to a commit.  See
>   ``SPECIFYING REVISIONS'' in linkgit:gitrevisions[7] for details.
> 
> +* The special null SHA-1 (40 zeros) specifices that the branch is to be

s/specifices/specifies/

(this was previously pointed out by Eric Sunshine for patch 7 of v4 of this series).


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: [PATCH v5 03/10] transport-helper: add 'force' to 'export' helpers

[Felipe Contreras]

On Thu, Oct 31, 2013 at 12:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Otherwise they cannot know when to force the push or not (other than
>> hacks).
>> ...
>> diff --git a/transport-helper.c b/transport-helper.c
>> index d05fc7c..ed238e5 100644
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -854,6 +854,11 @@ static int push_refs_with_export(struct transport *transport,
>>                       die("helper %s does not support dry-run", data->name);
>>       }
>>
>> +     if (flags & TRANSPORT_PUSH_FORCE) {
>> +             if (set_helper_option(transport, "force", "true") != 0)
>> +                     die("helper %s does not support 'force'", data->name);
>> +     }
>> +
>
> Does this cause a "git push --force $there A:B" to fail when $there
> is a destination that goes via an existing helper does not suport
> "force" option?

Yes.

> Should it fail even when the current value of B is an ancestor of A
> (i.e. when an unforced push would succeed)?

It might make sense to fail only when the push is non-fast-forward,
but it's not so straight-forward to implement.

-- 
Felipe Contreras

Re: [PATCH v5 03/10] transport-helper: add 'force' to 'export' helpers

[Junio C Hamano]

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

> On Thu, Oct 31, 2013 at 12:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Otherwise they cannot know when to force the push or not (other than
>>> hacks).
>>> ...
>>> diff --git a/transport-helper.c b/transport-helper.c
>>> index d05fc7c..ed238e5 100644
>>> --- a/transport-helper.c
>>> +++ b/transport-helper.c
>>> @@ -854,6 +854,11 @@ static int push_refs_with_export(struct transport *transport,
>>>                       die("helper %s does not support dry-run", data->name);
>>>       }
>>>
>>> +     if (flags & TRANSPORT_PUSH_FORCE) {
>>> +             if (set_helper_option(transport, "force", "true") != 0)
>>> +                     die("helper %s does not support 'force'", data->name);
>>> +     }
>>> +
>>
>> Does this cause a "git push --force $there A:B" to fail when $there
>> is a destination that goes via an existing helper does not suport
>> "force" option?
>
> Yes.
>
>> Should it fail even when the current value of B is an ancestor of A
>> (i.e. when an unforced push would succeed)?
>
> It might make sense to fail only when the push is non-fast-forward,
> but it's not so straight-forward to implement.

OK; let's see if anybody screams by cooking the series in 'next'
(that is, when other issues people may discover in the series are
addressed).

Thanks.

Re: [PATCH v5 09/10] fast-export: add support to delete refs

[Max Horn]

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

Actually, I just noticed one thing that I *do* have a question about:

On 31.10.2013, at 10:36, Felipe Contreras <felipe.contreras@gmail.com> wrote:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> builtin/fast-export.c  | 14 ++++++++++++++
> t/t9350-fast-export.sh | 11 +++++++++++
> 2 files changed, 25 insertions(+)
> 
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index b6f623e..8ed41b4 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -673,6 +673,19 @@ static void import_marks(char *input_file)
> 	fclose(f);
> }
> 
> +static void handle_deletes(void)
> +{
> +	int i;
> +	for (i = 0; i < refspecs_nr; i++) {
> +		struct refspec *refspec = &refspecs[i];
> +		if (*refspec->src)
> +			continue;
> +
> +		printf("reset %s\nfrom %s\n\n",
> +				refspec->dst, sha1_to_hex(null_sha1));

If I understand it right, this issues a "reset" command in the fast-import stream, resetting a ref to an all-zero SHA1. I had a look at the git-fast-import documentation, but I found that it does not explicitly cover this case. In particular, the "reset" command does not specify that an all-zero SHA1 should be treated as "delete this ref". On the other hand, the docs for "reset" seem to indicate that one can omit the "from" part, although I couldn't tell for sure what that would mean, either.

I have not yet tried to dive into the code to figure out what actually happens, but I assume Felipe did that resp. tested it, and so supposedly doing this works, at least for git resp. existing transport helpers. But I wonder if other implementers of the fast-import format would handle it properly?

In any case, it seems to me that this is a gap in the documentation, isn't it? Or am I just being dense?


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: [PATCH v5 09/10] fast-export: add support to delete refs

[Felipe Contreras]

On Thu, Oct 31, 2013 at 1:29 PM, Max Horn <max@quendi.de> wrote:
> Actually, I just noticed one thing that I *do* have a question about:
>
> On 31.10.2013, at 10:36, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>> builtin/fast-export.c  | 14 ++++++++++++++
>> t/t9350-fast-export.sh | 11 +++++++++++
>> 2 files changed, 25 insertions(+)
>>
>> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
>> index b6f623e..8ed41b4 100644
>> --- a/builtin/fast-export.c
>> +++ b/builtin/fast-export.c
>> @@ -673,6 +673,19 @@ static void import_marks(char *input_file)
>>       fclose(f);
>> }
>>
>> +static void handle_deletes(void)
>> +{
>> +     int i;
>> +     for (i = 0; i < refspecs_nr; i++) {
>> +             struct refspec *refspec = &refspecs[i];
>> +             if (*refspec->src)
>> +                     continue;
>> +
>> +             printf("reset %s\nfrom %s\n\n",
>> +                             refspec->dst, sha1_to_hex(null_sha1));
>
> If I understand it right, this issues a "reset" command in the fast-import stream, resetting a ref to an all-zero SHA1. I had a look at the git-fast-import documentation, but I found that it does not explicitly cover this case. In particular, the "reset" command does not specify that an all-zero SHA1 should be treated as "delete this ref".

That's what the previous patch does.

> On the other hand, the docs for "reset" seem to indicate that one can omit the "from" part, although I couldn't tell for sure what that would mean, either.

It means something different.

> I have not yet tried to dive into the code to figure out what actually happens, but I assume Felipe did that resp. tested it, and so supposedly doing this works, at least for git resp. existing transport helpers. But I wonder if other implementers of the fast-import format would handle it properly?

Try this:

--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -519,7 +519,9 @@ test_expect_success 'delete refspec' '
        from 0000000000000000000000000000000000000000

        EOF
-       test_cmp expected actual
+       test_cmp expected actual &&
+       git fast-import < actual &&
+       test_must_fail git rev-parse -q --verify refs/heads/to-delete
 '

 test_done

-- 
Felipe Contreras

Re: [PATCH v5 09/10] fast-export: add support to delete refs

[Max Horn]

[-- Attachment #1: Type: text/plain, Size: 2068 bytes --]


On 31.10.2013, at 20:41, Felipe Contreras <felipe.contreras@gmail.com> wrote:

> On Thu, Oct 31, 2013 at 1:29 PM, Max Horn <max@quendi.de> wrote:
>> Actually, I just noticed one thing that I *do* have a question about:
>> 
>> On 31.10.2013, at 10:36, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> 
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> ---
>>> builtin/fast-export.c  | 14 ++++++++++++++
>>> t/t9350-fast-export.sh | 11 +++++++++++
>>> 2 files changed, 25 insertions(+)
>>> 
>>> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
>>> index b6f623e..8ed41b4 100644
>>> --- a/builtin/fast-export.c
>>> +++ b/builtin/fast-export.c
>>> @@ -673,6 +673,19 @@ static void import_marks(char *input_file)
>>>      fclose(f);
>>> }
>>> 
>>> +static void handle_deletes(void)
>>> +{
>>> +     int i;
>>> +     for (i = 0; i < refspecs_nr; i++) {
>>> +             struct refspec *refspec = &refspecs[i];
>>> +             if (*refspec->src)
>>> +                     continue;
>>> +
>>> +             printf("reset %s\nfrom %s\n\n",
>>> +                             refspec->dst, sha1_to_hex(null_sha1));
>> 
>> If I understand it right, this issues a "reset" command in the fast-import stream, resetting a ref to an all-zero SHA1. I had a look at the git-fast-import documentation, but I found that it does not explicitly cover this case. In particular, the "reset" command does not specify that an all-zero SHA1 should be treated as "delete this ref".
> 
> That's what the previous patch does.

Right *facepalm*.

But then this should be documented in git-fast-import.txt, shouldn't it?

> 
>> On the other hand, the docs for "reset" seem to indicate that one can omit the "from" part, although I couldn't tell for sure what that would mean, either.
> 
> It means something different.

Yeah, I figured that -- just wanted to point out that this, too, is not very clear in the documentation and should be improved (not saying that I expect you to do that, just pointing it out).


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: [PATCH v5 09/10] fast-export: add support to delete refs

[Felipe Contreras]

On Thu, Oct 31, 2013 at 1:47 PM, Max Horn <max@quendi.de> wrote:
>
> On 31.10.2013, at 20:41, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
>> On Thu, Oct 31, 2013 at 1:29 PM, Max Horn <max@quendi.de> wrote:
>>> Actually, I just noticed one thing that I *do* have a question about:
>>>
>>> On 31.10.2013, at 10:36, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>>>
>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>> ---
>>>> builtin/fast-export.c  | 14 ++++++++++++++
>>>> t/t9350-fast-export.sh | 11 +++++++++++
>>>> 2 files changed, 25 insertions(+)
>>>>
>>>> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
>>>> index b6f623e..8ed41b4 100644
>>>> --- a/builtin/fast-export.c
>>>> +++ b/builtin/fast-export.c
>>>> @@ -673,6 +673,19 @@ static void import_marks(char *input_file)
>>>>      fclose(f);
>>>> }
>>>>
>>>> +static void handle_deletes(void)
>>>> +{
>>>> +     int i;
>>>> +     for (i = 0; i < refspecs_nr; i++) {
>>>> +             struct refspec *refspec = &refspecs[i];
>>>> +             if (*refspec->src)
>>>> +                     continue;
>>>> +
>>>> +             printf("reset %s\nfrom %s\n\n",
>>>> +                             refspec->dst, sha1_to_hex(null_sha1));
>>>
>>> If I understand it right, this issues a "reset" command in the fast-import stream, resetting a ref to an all-zero SHA1. I had a look at the git-fast-import documentation, but I found that it does not explicitly cover this case. In particular, the "reset" command does not specify that an all-zero SHA1 should be treated as "delete this ref".
>>
>> That's what the previous patch does.
>
> Right *facepalm*.
>
> But then this should be documented in git-fast-import.txt, shouldn't it?

It is... in the previous patch.

-- 
Felipe Contreras

Re: [PATCH v5 09/10] fast-export: add support to delete refs

[Max Horn]

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]


On 31.10.2013, at 20:53, Felipe Contreras <felipe.contreras@gmail.com> wrote:

> On Thu, Oct 31, 2013 at 1:47 PM, Max Horn <max@quendi.de> wrote:
>> 
>> On 31.10.2013, at 20:41, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> 
>>> On Thu, Oct 31, 2013 at 1:29 PM, Max Horn <max@quendi.de> wrote:
[...]
>>> 
>>> That's what the previous patch does.
>> 
>> Right *facepalm*.
>> 
>> But then this should be documented in git-fast-import.txt, shouldn't it?
> 
> It is... in the previous patch.

Indeed, there it is. So that answer to my initial question is: Yes, I am being dense :-).

So, I am (still) happy with the patch series :-)


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: [PATCH v5 03/10] transport-helper: add 'force' to 'export' helpers

[Junio C Hamano]

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

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Oct 31, 2013 at 12:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> Otherwise they cannot know when to force the push or not (other than
>>>> hacks).
>>>> ...
>>>> diff --git a/transport-helper.c b/transport-helper.c
>>>> index d05fc7c..ed238e5 100644
>>>> --- a/transport-helper.c
>>>> +++ b/transport-helper.c
>>>> @@ -854,6 +854,11 @@ static int push_refs_with_export(struct transport *transport,
>>>>                       die("helper %s does not support dry-run", data->name);
>>>>       }
>>>>
>>>> +     if (flags & TRANSPORT_PUSH_FORCE) {
>>>> +             if (set_helper_option(transport, "force", "true") != 0)
>>>> +                     die("helper %s does not support 'force'", data->name);
>>>> +     }
>>>> +
>>>
>>> Does this cause a "git push --force $there A:B" to fail when $there
>>> is a destination that goes via an existing helper does not suport
>>> "force" option?
>>
>> Yes.
>>
>>> Should it fail even when the current value of B is an ancestor of A
>>> (i.e. when an unforced push would succeed)?
>>
>> It might make sense to fail only when the push is non-fast-forward,
>> but it's not so straight-forward to implement.
>
> OK; let's see if anybody screams by cooking the series in 'next'
> (that is, when other issues people may discover in the series are
> addressed).
>
> Thanks.

Actually, thinking about it again, would it be a better alternative
to issue an error message and continue, instead of dying here
(i.e. replace the above die() with error() or even warning())?

That way, if the helper has not been updated to support 'force', but
the user has been happily accepting the result he gets from "git
push $there +master" (perhaps because he has never pushed a non-ff
history so far), he now gets a warning that essentially says that
his push has been working by accident, even when the particular
ff-push that showed the error message goes through successfully.

If his push through the helper that has not been updated does need
'force', he gets the same old behaviour, possibly a broken one, out
of the helper, and again he does get the warning.

Because updated helpers know 'force' option, such a s/die/warning/
change would not affect them at all.

Am I missing something?

Re: [PATCH v5 03/10] transport-helper: add 'force' to 'export' helpers

[Felipe Contreras]

On Fri, Nov 1, 2013 at 8:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> On Thu, Oct 31, 2013 at 12:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>>
>>>>> Otherwise they cannot know when to force the push or not (other than
>>>>> hacks).
>>>>> ...
>>>>> diff --git a/transport-helper.c b/transport-helper.c
>>>>> index d05fc7c..ed238e5 100644
>>>>> --- a/transport-helper.c
>>>>> +++ b/transport-helper.c
>>>>> @@ -854,6 +854,11 @@ static int push_refs_with_export(struct transport *transport,
>>>>>                       die("helper %s does not support dry-run", data->name);
>>>>>       }
>>>>>
>>>>> +     if (flags & TRANSPORT_PUSH_FORCE) {
>>>>> +             if (set_helper_option(transport, "force", "true") != 0)
>>>>> +                     die("helper %s does not support 'force'", data->name);
>>>>> +     }
>>>>> +
>>>>
>>>> Does this cause a "git push --force $there A:B" to fail when $there
>>>> is a destination that goes via an existing helper does not suport
>>>> "force" option?
>>>
>>> Yes.
>>>
>>>> Should it fail even when the current value of B is an ancestor of A
>>>> (i.e. when an unforced push would succeed)?
>>>
>>> It might make sense to fail only when the push is non-fast-forward,
>>> but it's not so straight-forward to implement.
>>
>> OK; let's see if anybody screams by cooking the series in 'next'
>> (that is, when other issues people may discover in the series are
>> addressed).
>>
>> Thanks.
>
> Actually, thinking about it again, would it be a better alternative
> to issue an error message and continue, instead of dying here
> (i.e. replace the above die() with error() or even warning())?
>
> That way, if the helper has not been updated to support 'force', but
> the user has been happily accepting the result he gets from "git
> push $there +master" (perhaps because he has never pushed a non-ff
> history so far), he now gets a warning that essentially says that
> his push has been working by accident, even when the particular
> ff-push that showed the error message goes through successfully.
>
> If his push through the helper that has not been updated does need
> 'force', he gets the same old behaviour, possibly a broken one, out
> of the helper, and again he does get the warning.
>
> Because updated helpers know 'force' option, such a s/die/warning/
> change would not affect them at all.

Yeah, that makes sense.

-- 
Felipe Contreras

Re: [PATCH v5 03/10] transport-helper: add 'force' to 'export' helpers

[Junio C Hamano]

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

> On Fri, Nov 1, 2013 at 8:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Because updated helpers know 'force' option, such a s/die/warning/
>> change would not affect them at all.
>
> Yeah, that makes sense.

OK, then let's queue this separately on top, so that we can revert
it more easily when our "s/die/warning/ should be safe and more
friendly to existing users" reasoning we discussed is found to be
faulty later.

-- >8 --
Subject: [PATCH] transport-helper: demote lack of "force" option to a warning

It would have been a good conservative position to take, if there
were no existing helpers, to die when the user asked to force a push
through a transport helper mechanism and the helper script hasn't
been updated to handle the "force" option.

There however are existing helpers in the field and none of them
obviously has been taught about the option yet.  If a helper has not
been updated to understand "force", but the user has happily been
accepting the result of "git push $there +master" (perhaps because
he has never pushed a non-ff history so far), the change made
previously in this series will fail the push, which would be a minor
regression for such a user.

Demote lack of "force" option from a "die()" to a "warning()".  By
doing so, such a user now gets a warning that notifies him that his
push has been working by accident, even when the particular ff-push
that showed the error message goes through successfully.

If his push does need "force", he gets the same old behaviour,
possibly a broken one, out of the helper that has not been update,
but again he does see the warning.

Helped-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 transport-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index ef91882..6b167ea 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -862,7 +862,7 @@ static int push_refs_with_export(struct transport *transport,
 
 	if (flags & TRANSPORT_PUSH_FORCE) {
 		if (set_helper_option(transport, "force", "true") != 0)
-			die("helper %s does not support 'force'", data->name);
+			warning("helper %s does not support 'force'", data->name);
 	}
 
 	helper = get_helper(transport);
-- 
1.8.5-rc0-205-g5b7460b

Re: [PATCH v5 03/10] transport-helper: add 'force' to 'export' helpers

[Eric Sunshine]

On Fri, Nov 1, 2013 at 12:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Subject: [PATCH] transport-helper: demote lack of "force" option to a warning
>
> It would have been a good conservative position to take, if there
> were no existing helpers, to die when the user asked to force a push
> through a transport helper mechanism and the helper script hasn't
> been updated to handle the "force" option.
>
> There however are existing helpers in the field and none of them
> obviously has been taught about the option yet.  If a helper has not
> been updated to understand "force", but the user has happily been
> accepting the result of "git push $there +master" (perhaps because
> he has never pushed a non-ff history so far), the change made
> previously in this series will fail the push, which would be a minor
> regression for such a user.
>
> Demote lack of "force" option from a "die()" to a "warning()".  By
> doing so, such a user now gets a warning that notifies him that his
> push has been working by accident, even when the particular ff-push
> that showed the error message goes through successfully.
>
> If his push does need "force", he gets the same old behaviour,
> possibly a broken one, out of the helper that has not been update,

s/update/updated/

> but again he does see the warning.
>
> Helped-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH v5 06/10] fast-export: add new --refspec option

[Junio C Hamano]

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

> On Thu, Oct 31, 2013 at 12:26 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 &&
>>
>> It feels somewhat redundant that you have to twice say that you are
>> pushing your "master", once with --refspec and then the branch
>> name.  Is this the best we can do?
>
> As this has been discussed before and no other solution came forward, yes.

We need to take that "no other solution came forward" with a grain
of salt.  After all, this is your itch, and if nobody was interested
in helping you (which I think that we both understand entirely
plausible, given the recent history), it only means you didn't think
of any other solution.

I didn't think things through, but at the external UI level, I see a
possibility of a nicer way to express the above.

In our "push" refspec (and export is about pushing what we have), a
colonless refspec A is a short-hand for A:A, so the traditional

	git fast-export master

can be thought of, in a new world order with a patch that lets you
do a ref mapping, a short-hand for an identical mapping:

	git fast-export master:master

It follows that the syntax naturally support

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

I would think.

That approach lets you express ref mapping without a new option
--refspec, which goes contrary to existing UI for any commands in
Git (I think nobody takes refspec as a value to a dashed-option in
the transport; both in fetch and push, they are proper operands,
i.e. command line arguments to the command), no?

Re: [PATCH v5 06/10] fast-export: add new --refspec option

[Jeff King]

On Wed, Nov 06, 2013 at 01:00:42PM -0800, Junio C Hamano wrote:

> I didn't think things through, but at the external UI level, I see a
> possibility of a nicer way to express the above.
> 
> In our "push" refspec (and export is about pushing what we have), a
> colonless refspec A is a short-hand for A:A, so the traditional
> 
> 	git fast-export master
> 
> can be thought of, in a new world order with a patch that lets you
> do a ref mapping, a short-hand for an identical mapping:
> 
> 	git fast-export master:master
> 
> It follows that the syntax naturally support
> 
> 	git fast-export refs/heads/master:refs/heads/foobar
> 
> I would think.
> 
> That approach lets you express ref mapping without a new option
> --refspec, which goes contrary to existing UI for any commands in
> Git (I think nobody takes refspec as a value to a dashed-option in
> the transport; both in fetch and push, they are proper operands,
> i.e. command line arguments to the command), no?

I think that is much nicer for the simple cases, but how do we handle
more complex rev expressions? One can say:

  git fast-export master ^origin

or even:

  git fast-export origin..master

The "^origin" is not a refspec, and finding the refspec in the
dot-expression would involve parsing it into two components. I think you
can come up with a workable system by parsing the arguments as revision
specifiers and then applying some rules. E.g., a positive ref "foo" is a
refspec "foo:foo", but negative "^bar" does not impact refspecs at all,
and the same rules are applied for "bar..foo". There is a syntactic
conflict where "foo:bar" would be interpreted as a tree:path by the
revision code, though, whereas in this context it means a refspec.

So I think it is possible to go that route (and perhaps preferable,
even, because it keeps the simple and common cases easy for the user),
but the implementation is not as simple as just treating the arguments
as refspecs.

-Peff

Re: [PATCH v5 06/10] fast-export: add new --refspec option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think that is much nicer for the simple cases, but how do we handle
> more complex rev expressions? One can say:
>
>   git fast-export master ^origin
>
> or even:
>
>   git fast-export origin..master
>
> The "^origin" is not a refspec, and finding the refspec in the
> dot-expression would involve parsing it into two components. I think you
> can come up with a workable system by parsing the arguments as revision
> specifiers and then applying some rules. E.g., a positive ref "foo" is a
> refspec "foo:foo", but negative "^bar" does not impact refspecs at all,
> and the same rules are applied for "bar..foo". There is a syntactic
> conflict where "foo:bar" would be interpreted as a tree:path by the
> revision code, though, whereas in this context it means a refspec.
>
> So I think it is possible to go that route (and perhaps preferable,
> even, because it keeps the simple and common cases easy for the user),
> but the implementation is not as simple as just treating the arguments
> as refspecs.

I forgot about the ranges, but thinking about it further, I think
what we really need to worry about is the positive end.

	git fast-export origin..master

is (by the usual definition of A..B range) the same as

	git fast-export ^origin master

If we realize that the bottom ends of ranges do not participate in
the "update the ref at the receiving end" phase of object/history
transfer at all, that command line can still be thought of as a
short-hand for

	git fast-export ^origin refs/heads/master:refs/heads/master

That is, anything negative is used only to specify the bottom of the
range, and positive ones (e.g. 'master' in both of your examples) tell
us what ref to update in addition to where the history ends.

Re: [PATCH v5 06/10] fast-export: add new --refspec option

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> I think that is much nicer for the simple cases, but how do we handle
>> more complex rev expressions? One can say:
>>
>>   git fast-export master ^origin
>>
>> or even:
>>
>>   git fast-export origin..master
>>
>> The "^origin" is not a refspec, and finding the refspec in the
>> dot-expression would involve parsing it into two components. I think you
>> can come up with a workable system by parsing the arguments as revision
>> specifiers and then applying some rules. E.g., a positive ref "foo" is a
>> refspec "foo:foo", but negative "^bar" does not impact refspecs at all,
>> and the same rules are applied for "bar..foo". There is a syntactic
>> conflict where "foo:bar" would be interpreted as a tree:path by the
>> revision code, though, whereas in this context it means a refspec.
>>
>> So I think it is possible to go that route (and perhaps preferable,
>> even, because it keeps the simple and common cases easy for the user),
>> but the implementation is not as simple as just treating the arguments
>> as refspecs.
>
> I forgot about the ranges, but thinking about it further, I think ...

And I am an idiot; you've spelled out everything I wanted to say, so
I should have just dropped this ;-)

Re: [PATCH v5 06/10] fast-export: add new --refspec option

[Felipe Contreras]

On Wed, Nov 6, 2013 at 3:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Oct 31, 2013 at 12:26 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 &&
>>>
>>> It feels somewhat redundant that you have to twice say that you are
>>> pushing your "master", once with --refspec and then the branch
>>> name.  Is this the best we can do?
>>
>> As this has been discussed before and no other solution came forward, yes.
>
> We need to take that "no other solution came forward" with a grain
> of salt.  After all, this is your itch, and if nobody was interested
> in helping you (which I think that we both understand entirely
> plausible, given the recent history), it only means you didn't think
> of any other solution.

Thinking is easy, the problem is not thinking of a solution, the
problem is actually doing it. I think we should end world hunger. What
a great idea!

> I didn't think things through, but at the external UI level, I see a
> possibility of a nicer way to express the above.

You already said that before.

Talk is cheap. Show me the code.

-- 
Felipe Contreras

Re: [PATCH v5 06/10] fast-export: add new --refspec option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Nov 06, 2013 at 01:00:42PM -0800, Junio C Hamano wrote:
>
>> It follows that the syntax naturally support
>> 
>> 	git fast-export refs/heads/master:refs/heads/foobar
>> 
>> I would think.
>> 
>> That approach lets you express ref mapping without a new option
>> --refspec, which goes contrary to existing UI for any commands in
>> Git (I think nobody takes refspec as a value to a dashed-option in
>> the transport; both in fetch and push, they are proper operands,
>> i.e. command line arguments to the command), no?
>
> I think that is much nicer for the simple cases, but how do we handle
> more complex rev expressions? ...
>
> The "^origin" is not a refspec, and finding the refspec in the
> dot-expression would involve parsing it into two components. I think you
> can come up with a workable system by parsing the arguments as revision
> specifiers and then applying some rules. E.g....

I was thinking about this a bit more today.  It is more or less
trivial to actually teach the setup_revisions() infrastructure to
optionally allow A:B to mean "We want a revision A, but with an
extra twist", and leave that "extra twist" information in the
rev_cmdline machinery.  After all, rev_cmdline was introduced for
doing exactly this kind of thing.

Earlier I said that all the existing transport commands take refspec
as proper operands, not a value to a dashed-option, but I can imagine
that we may in the future want to update "git push" in a way similar
to what Felipe did to "git fast-export" so that it allows something
like this:

    $ git push mothership \
    > --refspec refs/heads/*:refs/remotes/satellite/* master

which would mean "I am pushing out 'master', but anything I push out
to the mothership from my refs/heads/ hierarchy should be used to
update the refs/remotes/satellite/ hierarchy over there".  The same
thing can be done in the reverse direction for "git fetch".

But such a wildcard refspec cannot be supported naturally by
extending the setup_revisions(); what the wildcarded refspec expands
to will depend on what other things are on the command line (in this
case, 'master').  So I stopped there (I'll attach a toy patch at the
end, but I'll discard it once I send this message out).

If you set remote.*.fetch (or remote.*.push) refspec with the
current system, it tells us two logically separate/separable things:

 (1) what is the set of refs fetched (or pushed); and

 (2) what refs at the receiving end are updated.

"git push" and "git fetch" could borrow the independent ref-mapping
UI from "git fast-export" to allow us to dissociate these two
concepts.  In the above "mothership-satelite" example, the "master"
specifies what is pushed out, while the value of "--refspec" option
specifies the mapping.  It would open a door to even make the
mapping a configuration variable.  In short, "nobody uses an
independent refspec mapping parameter" does not necessarily mean "we
should not have such an independent refspec mapping parameter".

If we were to go that route, however, I would be strongly against
calling that option --refspec; perhaps calling it --refmap would
avoid confusion.

 remote.c   |  5 +++++
 remote.h   |  2 ++
 revision.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 revision.h | 10 +++++++++-
 4 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 9f1a8aa..26b86a0 100644
--- a/remote.c
+++ b/remote.c
@@ -653,6 +653,11 @@ static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
 	return parse_refspec_internal(nr_refspec, refspec, 0, 0);
 }
 
+struct refspec *parse_push_refspec_verify(int nr_refspec, const char **refspec)
+{
+	return parse_refspec_internal(nr_refspec, refspec, 0, 1);
+}
+
 void free_refspec(int nr_refspec, struct refspec *refspec)
 {
 	int i;
diff --git a/remote.h b/remote.h
index 131130a..2bc0a7e 100644
--- a/remote.h
+++ b/remote.h
@@ -156,6 +156,8 @@ void ref_remove_duplicates(struct ref *ref_map);
 int valid_fetch_refspec(const char *refspec);
 struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
 
+struct refspec *parse_push_refspec_verify(int nr_refspec, const char **refspec);
+
 void free_refspec(int nr_refspec, struct refspec *refspec);
 
 char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
diff --git a/revision.c b/revision.c
index 956040c..17e7b3d 100644
--- a/revision.c
+++ b/revision.c
@@ -16,6 +16,7 @@
 #include "line-log.h"
 #include "mailmap.h"
 #include "commit-slab.h"
+#include "remote.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -1377,6 +1378,58 @@ static void prepare_show_merge(struct rev_info *revs)
 	revs->limited = 1;
 }
 
+static int handle_revision_refspec(const char *arg, struct rev_info *revs, unsigned revarg_opt)
+{
+	int cant_be_filename = revarg_opt & REVARG_CANNOT_BE_FILENAME;
+	struct refspec *pair;
+	static struct ref *local_refs;
+	struct ref *remote_refs = NULL;
+	struct object *object;
+	int retval = 0;
+
+	pair = parse_push_refspec_verify(1, &arg);
+	if (!pair)
+		return -1;
+
+	if (pair->matching || pair->pattern) {
+		/* ":" or "refs/heads/<star>:refs/heads/<star>" are not revs */
+		retval = -1;
+		goto cleanup_return;
+	}
+
+	/* The source side is what we are pushing out */
+	if (!local_refs)
+		local_refs = get_local_heads();
+	if (match_push_refs(local_refs, &remote_refs, 1, &arg, 0)) {
+		retval = -1;
+		goto cleanup_return;
+	}
+
+	/*
+	 * Now, remote_refs should have a single element that tells
+	 * us what object we are pushing.
+	 */
+	if (!remote_refs || remote_refs->next || !remote_refs->peer_ref ||
+	    !(object = parse_object(remote_refs->peer_ref->new_sha1))) {
+		retval = -1;
+		goto cleanup_return;
+	}
+
+	if (!cant_be_filename)
+		verify_non_filename(revs->prefix, arg);
+
+	retval = 0;
+	add_rev_cmdline(revs, object, arg, REV_CMD_REFSPEC, 0);
+	add_pending_object(revs, object, arg);
+
+cleanup_return:
+	free_refs(remote_refs);
+	free(pair->src);
+	free(pair->dst);
+	free(pair);
+	return retval;
+}
+
 int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
 {
 	struct object_context oc;
@@ -1500,8 +1553,20 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
 	if (revarg_opt & REVARG_COMMITTISH)
 		get_sha1_flags = GET_SHA1_COMMITTISH;
 
-	if (get_sha1_with_context(arg, get_sha1_flags, sha1, &oc))
+	/*
+	 * Are we allowed to interpret A:B refspec as a revision
+	 * specifying A?  ^A:B nor --not A:B would make any sense, so
+	 * do not route such cases to handle_revision_refspec().
+	 */
+	if (!local_flags && !flags &&
+	    (revarg_opt & REVARG_ALLOW_REFSPEC) &&
+	    !handle_revision_refspec(arg, revs, revarg_opt))
+		return 0;
+
+	if (get_sha1_with_context(arg, get_sha1_flags, sha1, &oc)) {
 		return revs->ignore_missing ? 0 : -1;
+	}
+
 	if (!cant_be_filename)
 		verify_non_filename(revs->prefix, arg);
 	object = get_reference(revs, arg, sha1, flags ^ local_flags);
diff --git a/revision.h b/revision.h
index 89132df..c97805e 100644
--- a/revision.h
+++ b/revision.h
@@ -40,7 +40,8 @@ struct rev_cmdline_info {
 			REV_CMD_LEFT,
 			REV_CMD_RIGHT,
 			REV_CMD_MERGE_BASE,
-			REV_CMD_REV
+			REV_CMD_REV,
+			REV_CMD_REFSPEC
 		} whence;
 		unsigned flags;
 	} *rev;
@@ -205,7 +206,13 @@ extern volatile show_early_output_fn_t show_early_output;
 
 struct setup_revision_opt {
 	const char *def;
+	/* hook to call after parsing the command line */
 	void (*tweak)(struct rev_info *, struct setup_revision_opt *);
+	/*
+	 * hook to call for a command line argument that is not a rev
+	 */
+	int (*parse_extended_rev)(struct rev_info *, struct setup_revision_opt *, const char *);
+
 	const char *submodule;
 	int assume_dashdash;
 	unsigned revarg_opt;
@@ -219,6 +226,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct
 			       const char * const usagestr[]);
 #define REVARG_CANNOT_BE_FILENAME 01
 #define REVARG_COMMITTISH 02
+#define REVARG_ALLOW_REFSPEC 04
 extern int handle_revision_arg(const char *arg, struct rev_info *revs,
 			       int flags, unsigned revarg_opt);
 

Re: [PATCH v5 06/10] fast-export: add new --refspec option

[Jeff King]

On Fri, Nov 08, 2013 at 03:44:43PM -0800, Junio C Hamano wrote:

> > The "^origin" is not a refspec, and finding the refspec in the
> > dot-expression would involve parsing it into two components. I think you
> > can come up with a workable system by parsing the arguments as revision
> > specifiers and then applying some rules. E.g....
> 
> I was thinking about this a bit more today.  It is more or less
> trivial to actually teach the setup_revisions() infrastructure to
> optionally allow A:B to mean "We want a revision A, but with an
> extra twist", and leave that "extra twist" information in the
> rev_cmdline machinery.  After all, rev_cmdline was introduced for
> doing exactly this kind of thing.

I had wondered at first whether it would be a problem to be
syntactically ambiguous with <ref>:<path>. But I don't think so. Doing:

   git fast-export HEAD:Documentation

does not produce any output currently. And the fast-import stream cannot
actually represent trees outside of commits. Something like:

  git fast-export HEAD:Makefile

could produce a blob entry, but it currently does nothing. So I don't
think it's a big deal, as nobody is clamoring for the feature. And
certainly "git push" suffers from the same problem, and you can work
around it with:

  git push $(git rev-parse HEAD:Makefile):refs/tag/makefile-blob

> Earlier I said that all the existing transport commands take refspec
> as proper operands, not a value to a dashed-option, but I can imagine
> that we may in the future want to update "git push" in a way similar
> to what Felipe did to "git fast-export" so that it allows something
> like this:
> 
>     $ git push mothership \
>     > --refspec refs/heads/*:refs/remotes/satellite/* master
> 
> which would mean "I am pushing out 'master', but anything I push out
> to the mothership from my refs/heads/ hierarchy should be used to
> update the refs/remotes/satellite/ hierarchy over there".  The same
> thing can be done in the reverse direction for "git fetch".

Yes, though I would expect the "--refspec" bit would end up in config
(else why would you not just apply it directly to master). And you would
want to be able to override it, like:

  git push mothership \
    --refspec refs/heads/*:refs/remotes/satellite/* master:refs/other/master

So I think even with such wildcard magic, you'd still want refspecs,
both for "push" and for "fast-export".

> But such a wildcard refspec cannot be supported naturally by
> extending the setup_revisions(); what the wildcarded refspec expands
> to will depend on what other things are on the command line (in this
> case, 'master').  So I stopped there (I'll attach a toy patch at the
> end, but I'll discard it once I send this message out).

Yes, I think applying such a refspec would be the duty of whoever
expands "master" into "master:master". And that is not a job for
setup_revisions, which should be dealing mostly with the syntax and
telling us "I got just 'master'" or "I got 'master:foo'". The
interpretation is up to the caller.

> If we were to go that route, however, I would be strongly against
> calling that option --refspec; perhaps calling it --refmap would
> avoid confusion.

Agreed.

-Peff

[PATCH] fixup! transport-helper: add 'force' to 'export' helpers

[Richard Hansen]

defend against force=foo in the user's environment

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
 git-remote-testgit.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh
index 6d2f282..1cfdea2 100755
--- a/git-remote-testgit.sh
+++ b/git-remote-testgit.sh
@@ -15,6 +15,8 @@ test -z "$refspec" && prefix="refs"
 
 export GIT_DIR="$url/.git"
 
+force=
+
 mkdir -p "$dir"
 
 if test -z "$GIT_REMOTE_TESTGIT_NO_MARKS"
-- 
1.8.5.rc1.207.gc17dd22

[PATCH v5 11/10] test-hg.sh: tests are now expected to pass

[Richard Hansen]

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
 contrib/remote-helpers/test-hg.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh
index 9f5066b..e6e3fed 100755
--- a/contrib/remote-helpers/test-hg.sh
+++ b/contrib/remote-helpers/test-hg.sh
@@ -604,7 +604,7 @@ test_expect_success 'remote big push fetch first' '
 	)
 '
 
-test_expect_failure 'remote big push force' '
+test_expect_success 'remote big push force' '
 	test_when_finished "rm -rf hgrepo gitrepo*" &&
 
 	setup_big_push
@@ -634,7 +634,7 @@ test_expect_failure 'remote big push force' '
 	check_bookmark hgrepo new_bmark six
 '
 
-test_expect_failure 'remote big push dry-run' '
+test_expect_success 'remote big push dry-run' '
 	test_when_finished "rm -rf hgrepo gitrepo*" &&
 
 	setup_big_push
-- 
1.8.5.rc1.207.gc17dd22

[PATCH v5 12/10] remote-bzr: support the new 'force' option

[Richard Hansen]

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
 contrib/remote-helpers/git-remote-bzr | 34 +++++++++++++++++++++++++++++++++-
 contrib/remote-helpers/test-bzr.sh    | 22 +++++++++++++++++++++-
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index 7e34532..ba693d1 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -42,6 +42,7 @@ import json
 import re
 import StringIO
 import atexit, shutil, hashlib, urlparse, subprocess
+import types
 
 NAME_RE = re.compile('^([^<>]+)')
 AUTHOR_RE = re.compile('^([^<>]+?)? ?[<>]([^<>]*)(?:$|>)')
@@ -684,7 +685,8 @@ def do_export(parser):
                 peer = bzrlib.branch.Branch.open(peers[name],
                                                  possible_transports=transports)
                 try:
-                    peer.bzrdir.push_branch(branch, revision_id=revid)
+                    peer.bzrdir.push_branch(branch, revision_id=revid,
+                                            overwrite=force)
                 except bzrlib.errors.DivergedBranches:
                     print "error %s non-fast forward" % ref
                     continue
@@ -718,8 +720,34 @@ def do_capabilities(parser):
         print "*import-marks %s" % path
     print "*export-marks %s" % path
 
+    print "option"
     print
 
+class InvalidOptionValue(Exception):
+    pass
+
+def do_option(parser):
+    (opt, val) = parser[1:3]
+    handler = globals().get('do_option_' + opt)
+    if handler and type(handler) == types.FunctionType:
+        try:
+            handler(val)
+        except InvalidOptionValue:
+            print "error '%s' is not a valid value for option '%s'" % (val, opt)
+    else:
+        print "unsupported"
+
+def do_bool_option(val):
+    if val == 'true': ret = True
+    elif val == 'false': ret = False
+    else: raise InvalidOptionValue()
+    print "ok"
+    return ret
+
+def do_option_force(val):
+    global force
+    force = do_bool_option(val)
+
 def ref_is_valid(name):
     return not True in [c in name for c in '~^: \\']
 
@@ -882,6 +910,7 @@ def main(args):
     global is_tmp
     global branches, peers
     global transports
+    global force
 
     alias = args[1]
     url = args[2]
@@ -895,6 +924,7 @@ def main(args):
     branches = {}
     peers = {}
     transports = []
+    force = False
 
     if alias[5:] == url:
         is_tmp = True
@@ -930,6 +960,8 @@ def main(args):
             do_import(parser)
         elif parser.check('export'):
             do_export(parser)
+        elif parser.check('option'):
+            do_option(parser)
         else:
             die('unhandled command: %s' % line)
         sys.stdout.flush()
diff --git a/contrib/remote-helpers/test-bzr.sh b/contrib/remote-helpers/test-bzr.sh
index ea597b0..7d7778f 100755
--- a/contrib/remote-helpers/test-bzr.sh
+++ b/contrib/remote-helpers/test-bzr.sh
@@ -69,13 +69,33 @@ test_expect_success 'pushing' '
 	test_cmp expected actual
 '
 
+test_expect_success 'forced pushing' '
+	(
+	cd gitrepo &&
+	echo three-new >content &&
+	git commit -a --amend -m three-new &&
+	git push -f
+	) &&
+
+	(
+	cd bzrrepo &&
+	# the forced update overwrites the bzr branch but not the bzr
+	# working directory (it tries to merge instead)
+	bzr revert
+	) &&
+
+	echo three-new >expected &&
+	cat bzrrepo/content >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'roundtrip' '
 	(
 	cd gitrepo &&
 	git pull &&
 	git log --format="%s" -1 origin/master >actual
 	) &&
-	echo three >expected &&
+	echo three-new >expected &&
 	test_cmp expected actual &&
 
 	(cd gitrepo && git push && git pull) &&
-- 
1.8.5.rc1.207.gc17dd22

RE: [PATCH v5 12/10] remote-bzr: support the new 'force' option

[Felipe Contreras]

Richard Hansen wrote:
> Signed-off-by: Richard Hansen <rhansen@bbn.com>
> ---
>  contrib/remote-helpers/git-remote-bzr | 34 +++++++++++++++++++++++++++++++++-
>  contrib/remote-helpers/test-bzr.sh    | 22 +++++++++++++++++++++-
>  2 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
> index 7e34532..ba693d1 100755
> --- a/contrib/remote-helpers/git-remote-bzr
> +++ b/contrib/remote-helpers/git-remote-bzr
> @@ -42,6 +42,7 @@ import json
>  import re
>  import StringIO
>  import atexit, shutil, hashlib, urlparse, subprocess
> +import types
>  
>  NAME_RE = re.compile('^([^<>]+)')
>  AUTHOR_RE = re.compile('^([^<>]+?)? ?[<>]([^<>]*)(?:$|>)')
> @@ -684,7 +685,8 @@ def do_export(parser):
>                  peer = bzrlib.branch.Branch.open(peers[name],
>                                                   possible_transports=transports)
>                  try:
> -                    peer.bzrdir.push_branch(branch, revision_id=revid)
> +                    peer.bzrdir.push_branch(branch, revision_id=revid,
> +                                            overwrite=force)
>                  except bzrlib.errors.DivergedBranches:
>                      print "error %s non-fast forward" % ref
>                      continue
> @@ -718,8 +720,34 @@ def do_capabilities(parser):
>          print "*import-marks %s" % path
>      print "*export-marks %s" % path
>  
> +    print "option"
>      print
>  
> +class InvalidOptionValue(Exception):
> +    pass
> +
> +def do_option(parser):
> +    (opt, val) = parser[1:3]
> +    handler = globals().get('do_option_' + opt)
> +    if handler and type(handler) == types.FunctionType:
> +        try:
> +            handler(val)
> +        except InvalidOptionValue:
> +            print "error '%s' is not a valid value for option '%s'" % (val, opt)
> +    else:
> +        print "unsupported"
> +
> +def do_bool_option(val):
> +    if val == 'true': ret = True
> +    elif val == 'false': ret = False
> +    else: raise InvalidOptionValue()
> +    print "ok"
> +    return ret
> +
> +def do_option_force(val):
> +    global force
> +    force = do_bool_option(val)
> +

While this organization has merit, I think it's overkill for a single option,
or just a couple of them. If in the future we add more, we might revisit this,
for the moment something like this would suffice:

    class InvalidOptionValue(Exception):
	pass

    def get_bool_option(val):
	if val == 'true':
	    return True
	elif val == 'false':
	    return False
	else:
	    raise InvalidOptionValue()

    def do_option(parser):
	global force
	_, key, value = parser.line.split(' ')
	try:
	    if key == 'force':
		force = get_bool_option(value)
		print 'ok'
	    else:
		print 'unsupported'
	except InvalidOptionValue:
	    print "error '%s' is not a valid value for option '%s'" % (value, key)

Cheers.

-- 
Felipe Contreras

Re: [PATCH v5 12/10] remote-bzr: support the new 'force' option

[Richard Hansen]

On 2013-11-11 06:51, Felipe Contreras wrote:
> Richard Hansen wrote:
>> Signed-off-by: Richard Hansen <rhansen@bbn.com>
>> ---
>>  contrib/remote-helpers/git-remote-bzr | 34 +++++++++++++++++++++++++++++++++-
>>  contrib/remote-helpers/test-bzr.sh    | 22 +++++++++++++++++++++-
>>  2 files changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
>> index 7e34532..ba693d1 100755
>> --- a/contrib/remote-helpers/git-remote-bzr
>> +++ b/contrib/remote-helpers/git-remote-bzr
>> @@ -42,6 +42,7 @@ import json
>>  import re
>>  import StringIO
>>  import atexit, shutil, hashlib, urlparse, subprocess
>> +import types
>>  
>>  NAME_RE = re.compile('^([^<>]+)')
>>  AUTHOR_RE = re.compile('^([^<>]+?)? ?[<>]([^<>]*)(?:$|>)')
>> @@ -684,7 +685,8 @@ def do_export(parser):
>>                  peer = bzrlib.branch.Branch.open(peers[name],
>>                                                   possible_transports=transports)
>>                  try:
>> -                    peer.bzrdir.push_branch(branch, revision_id=revid)
>> +                    peer.bzrdir.push_branch(branch, revision_id=revid,
>> +                                            overwrite=force)
>>                  except bzrlib.errors.DivergedBranches:
>>                      print "error %s non-fast forward" % ref
>>                      continue
>> @@ -718,8 +720,34 @@ def do_capabilities(parser):
>>          print "*import-marks %s" % path
>>      print "*export-marks %s" % path
>>  
>> +    print "option"
>>      print
>>  
>> +class InvalidOptionValue(Exception):
>> +    pass
>> +
>> +def do_option(parser):
>> +    (opt, val) = parser[1:3]
>> +    handler = globals().get('do_option_' + opt)
>> +    if handler and type(handler) == types.FunctionType:
>> +        try:
>> +            handler(val)
>> +        except InvalidOptionValue:
>> +            print "error '%s' is not a valid value for option '%s'" % (val, opt)
>> +    else:
>> +        print "unsupported"
>> +
>> +def do_bool_option(val):
>> +    if val == 'true': ret = True
>> +    elif val == 'false': ret = False
>> +    else: raise InvalidOptionValue()
>> +    print "ok"
>> +    return ret
>> +
>> +def do_option_force(val):
>> +    global force
>> +    force = do_bool_option(val)
>> +
> 
> While this organization has merit, I think it's overkill for a single option,
> or just a couple of them. If in the future we add more, we might revisit this,
> for the moment something like this would suffice:

OK, I'll reroll.

> 
>     class InvalidOptionValue(Exception):
> 	pass
> 
>     def get_bool_option(val):
> 	if val == 'true':
> 	    return True
> 	elif val == 'false':
> 	    return False
> 	else:
> 	    raise InvalidOptionValue()
> 
>     def do_option(parser):
> 	global force
> 	_, key, value = parser.line.split(' ')

I'm surprised you prefer this over 'key, val = parser[1:3]' or even
'_, key, val = parser[:]'.  Are you intending to eventually remove
Parser.__getitem__()?

Thanks,
Richard


> 	try:
> 	    if key == 'force':
> 		force = get_bool_option(value)
> 		print 'ok'
> 	    else:
> 		print 'unsupported'
> 	except InvalidOptionValue:
> 	    print "error '%s' is not a valid value for option '%s'" % (value, key)
> 
> Cheers.

Re: [PATCH v5 12/10] remote-bzr: support the new 'force' option

[Felipe Contreras]

On Mon, Nov 11, 2013 at 12:12 PM, Richard Hansen <rhansen@bbn.com> wrote:
> On 2013-11-11 06:51, Felipe Contreras wrote:

>>     def do_option(parser):
>>       global force
>>       _, key, value = parser.line.split(' ')
>
> I'm surprised you prefer this over 'key, val = parser[1:3]' or even
> '_, key, val = parser[:]'.  Are you intending to eventually remove
> Parser.__getitem__()?

I don't, actually. I'm fine with either way.

Cheers.

-- 
Felipe Contreras