[PATCH v9 0/6] transport-helper: fixes

[PATCH v9 0/6] transport-helper: fixes

[Felipe Contreras]

Hi,

These patches add support for remote helpers --force, --dry-run, and reporting
forced update.

Changes since v8:

--- a/transport-helper.c
+++ b/transport-helper.c
@@ -734,7 +734,7 @@ static int push_update_ref_status(struct strbuf *buf,
        }
 
        (*ref)->status = status;
-       (*ref)->forced_update = forced;
+       (*ref)->forced_update |= forced;
        (*ref)->remote_status = msg;
        return !(status == REF_STATUS_OK);
 }

Felipe Contreras (4):
  transport-helper: mismerge fix
  transport-helper: don't update refs in dry-run
  transport-helper: add 'force' to 'export' helpers
  transport-helper: check for 'forced update' message

Richard Hansen (2):
  test-hg.sh: tests are now expected to pass
  remote-bzr: support the new 'force' option

 Documentation/gitremote-helpers.txt   |  4 ++++
 contrib/remote-helpers/git-remote-bzr | 31 ++++++++++++++++++++++++++++++-
 contrib/remote-helpers/test-bzr.sh    | 22 +++++++++++++++++++++-
 contrib/remote-helpers/test-hg.sh     |  4 ++--
 git-remote-testgit.sh                 | 18 ++++++++++++++++++
 t/t5801-remote-helpers.sh             | 13 +++++++++++++
 transport-helper.c                    | 25 +++++++++++++++++--------
 7 files changed, 105 insertions(+), 12 deletions(-)

-- 
1.9.1+fc3.9.gc73078e

[PATCH v9 1/6] transport-helper: mismerge fix

[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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 transport-helper.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index ad72fbd..ea34d0c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -881,9 +881,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.9.1+fc3.9.gc73078e

[PATCH v9 2/6] 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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 transport-helper.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index ea34d0c..4b3e38e 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -734,7 +734,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;
@@ -748,7 +749,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 */
@@ -839,7 +840,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;
 }
 
@@ -893,7 +894,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.9.1+fc3.9.gc73078e

[PATCH v9 3/6] 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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/gitremote-helpers.txt |  4 ++++
 git-remote-testgit.sh               | 18 ++++++++++++++++++
 t/t5801-remote-helpers.sh           | 13 +++++++++++++
 transport-helper.c                  |  5 +++++
 4 files changed, 40 insertions(+)

diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index c2908db..c24c0c2 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -444,6 +444,10 @@ set by Git if the remote helper has the 'option' capability.
 'option update-shallow \{'true'|'false'\}::
 	Allow to extend .git/shallow if the new refs require it.
 
+'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..1c006a0 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"
@@ -39,6 +41,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 +96,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 +119,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 4b3e38e..f50e84f 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -861,6 +861,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)
+			warning("helper %s does not support 'force'", data->name);
+	}
+
 	helper = get_helper(transport);
 
 	write_constant(helper->in, "export\n");
-- 
1.9.1+fc3.9.gc73078e

[PATCH v9 4/6] transport-helper: check for 'forced update' message

[Felipe Contreras]

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

Helped-by: Max Horn <max@quendi.de>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 transport-helper.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index f50e84f..86e1679 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -650,7 +650,7 @@ static int push_update_ref_status(struct strbuf *buf,
 				   struct ref *remote_refs)
 {
 	char *refname, *msg;
-	int status;
+	int status, forced = 0;
 
 	if (starts_with(buf->buf, "ok ")) {
 		status = REF_STATUS_OK;
@@ -708,6 +708,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)
@@ -729,6 +734,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.9.1+fc3.9.gc73078e

[PATCH v9 5/6] test-hg.sh: tests are now expected to pass

[Felipe Contreras]

From: Richard Hansen <rhansen@bbn.com>

Signed-off-by: Richard Hansen <rhansen@bbn.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.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 5d128a5..a933b1e 100755
--- a/contrib/remote-helpers/test-hg.sh
+++ b/contrib/remote-helpers/test-hg.sh
@@ -680,7 +680,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
@@ -710,7 +710,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.9.1+fc3.9.gc73078e

[PATCH v9 6/6] remote-bzr: support the new 'force' option

[Felipe Contreras]

From: Richard Hansen <rhansen@bbn.com>

Signed-off-by: Richard Hansen <rhansen@bbn.com>
Acked-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-bzr | 31 ++++++++++++++++++++++++++++++-
 contrib/remote-helpers/test-bzr.sh    | 22 +++++++++++++++++++++-
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index 332aba7..5f4b2e3 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -684,7 +684,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 +719,32 @@ def do_capabilities(parser):
         print "*import-marks %s" % path
     print "*export-marks %s" % path
 
+    print "option"
     print
 
+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
+    opt, val = parser[1:3]
+    try:
+        if opt == 'force':
+            force = get_bool_option(val)
+            print 'ok'
+        else:
+            print 'unsupported'
+    except InvalidOptionValue:
+        print "error '%s' is not a valid value for option '%s'" % (val, opt)
+
 def ref_is_valid(name):
     return not True in [c in name for c in '~^: \\']
 
@@ -882,6 +907,7 @@ def main(args):
     global is_tmp
     global branches, peers
     global transports
+    global force
 
     marks = None
     is_tmp = False
@@ -904,6 +930,7 @@ def main(args):
     branches = {}
     peers = {}
     transports = []
+    force = False
 
     if alias[5:] == url:
         is_tmp = True
@@ -936,6 +963,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 1e53ff9..4f379c2 100755
--- a/contrib/remote-helpers/test-bzr.sh
+++ b/contrib/remote-helpers/test-bzr.sh
@@ -66,13 +66,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.9.1+fc3.9.gc73078e

Re: [PATCH v9 0/6] transport-helper: fixes

[Junio C Hamano]

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

> These patches add support for remote helpers --force, --dry-run, and reporting
> forced update.
>
> Changes since v8:
>
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -734,7 +734,7 @@ static int push_update_ref_status(struct strbuf *buf,
>         }
>  
>         (*ref)->status = status;
> -       (*ref)->forced_update = forced;
> +       (*ref)->forced_update |= forced;
>         (*ref)->remote_status = msg;
>         return !(status == REF_STATUS_OK);
>  }

Hmph, isn't v8 already in 'master' as of 90e6255a (Merge branch
'fc/transport-helper-fixes', 2014-03-18)?


> Felipe Contreras (4):
>   transport-helper: mismerge fix
>   transport-helper: don't update refs in dry-run
>   transport-helper: add 'force' to 'export' helpers
>   transport-helper: check for 'forced update' message
>
> Richard Hansen (2):
>   test-hg.sh: tests are now expected to pass
>   remote-bzr: support the new 'force' option
>
>  Documentation/gitremote-helpers.txt   |  4 ++++
>  contrib/remote-helpers/git-remote-bzr | 31 ++++++++++++++++++++++++++++++-
>  contrib/remote-helpers/test-bzr.sh    | 22 +++++++++++++++++++++-
>  contrib/remote-helpers/test-hg.sh     |  4 ++--
>  git-remote-testgit.sh                 | 18 ++++++++++++++++++
>  t/t5801-remote-helpers.sh             | 13 +++++++++++++
>  transport-helper.c                    | 25 +++++++++++++++++--------
>  7 files changed, 105 insertions(+), 12 deletions(-)

Re: [PATCH v9 0/6] transport-helper: fixes

[Junio C Hamano]

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

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> These patches add support for remote helpers --force, --dry-run, and reporting
>> forced update.
>>
>> Changes since v8:
>>
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -734,7 +734,7 @@ static int push_update_ref_status(struct strbuf *buf,
>>         }
>>  
>>         (*ref)->status = status;
>> -       (*ref)->forced_update = forced;
>> +       (*ref)->forced_update |= forced;
>>         (*ref)->remote_status = msg;
>>         return !(status == REF_STATUS_OK);
>>  }
>
> Hmph, isn't v8 already in 'master' as of 90e6255a (Merge branch
> 'fc/transport-helper-fixes', 2014-03-18)?

I checked and it seems to be that way.  If there are new development
that are missing in my tree, please send in incrementals.

Thanks.

Re: [PATCH v9 0/6] transport-helper: fixes

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > These patches add support for remote helpers --force, --dry-run, and reporting
> > forced update.
> >
> > Changes since v8:
> >
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -734,7 +734,7 @@ static int push_update_ref_status(struct strbuf *buf,
> >         }
> >  
> >         (*ref)->status = status;
> > -       (*ref)->forced_update = forced;
> > +       (*ref)->forced_update |= forced;
> >         (*ref)->remote_status = msg;
> >         return !(status == REF_STATUS_OK);
> >  }
> 
> Hmph, isn't v8 already in 'master' as of 90e6255a (Merge branch
> 'fc/transport-helper-fixes', 2014-03-18)?

I think I saw gitk report "Branches: remotes/origin/pu", but OK.

-- 
Felipe Contreras

Re: [PATCH v9 0/6] transport-helper: fixes

[Junio C Hamano]

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

> Junio C Hamano wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> 
>> > These patches add support for remote helpers --force, --dry-run, and reporting
>> > forced update.
>> >
>> > Changes since v8:
>> >
>> > --- a/transport-helper.c
>> > +++ b/transport-helper.c
>> > @@ -734,7 +734,7 @@ static int push_update_ref_status(struct strbuf *buf,
>> >         }
>> >  
>> >         (*ref)->status = status;
>> > -       (*ref)->forced_update = forced;
>> > +       (*ref)->forced_update |= forced;
>> >         (*ref)->remote_status = msg;
>> >         return !(status == REF_STATUS_OK);
>> >  }
>> 
>> Hmph, isn't v8 already in 'master' as of 90e6255a (Merge branch
>> 'fc/transport-helper-fixes', 2014-03-18)?
>
> I think I saw gitk report "Branches: remotes/origin/pu", but OK.

I don't get that "but" part, but as I said, if what you wanted to
apply has further updates relative to what we have, please send in
incremental.  The patches did not apply cleanly near the tip of
'master', so I didn't check what could be missing myself.

Thanks.