[PATCH] push: allow pushing new branches with --force-with-lease

[PATCH] push: allow pushing new branches with --force-with-lease

[John Keeping]

If there is no upstream information for a branch, it is likely that it
is newly created and can safely be pushed under the normal fast-forward
rules.  Relax the --force-with-lease check so that we do not reject
these branches immediately but rather attempt to push them as new
branches, using the null SHA-1 as the expected value.

In fact, it is already possible to push new branches using the explicit
--force-with-lease=<branch>:<expect> syntax, so all we do here is make
this behaviour the default if no explicit "expect" value is specified.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 remote.c            |  7 +++----
 remote.h            |  1 -
 t/t5533-push-cas.sh | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index a326e4e..cd2ee52 100644
--- a/remote.c
+++ b/remote.c
@@ -1544,8 +1544,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 * branch.
 		 */
 		if (ref->expect_old_sha1) {
-			if (ref->expect_old_no_trackback ||
-			    oidcmp(&ref->old_oid, &ref->old_oid_expect))
+			if (oidcmp(&ref->old_oid, &ref->old_oid_expect))
 				reject_reason = REF_STATUS_REJECT_STALE;
 			else
 				/* If the ref isn't stale then force the update. */
@@ -2343,7 +2342,7 @@ static void apply_cas(struct push_cas_option *cas,
 		if (!entry->use_tracking)
 			hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
 		else if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
-			ref->expect_old_no_trackback = 1;
+			memset(&ref->old_oid_expect, 0, sizeof(ref->old_oid_expect));
 		return;
 	}
 
@@ -2353,7 +2352,7 @@ static void apply_cas(struct push_cas_option *cas,
 
 	ref->expect_old_sha1 = 1;
 	if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
-		ref->expect_old_no_trackback = 1;
+		memset(&ref->old_oid_expect, 0, sizeof(ref->old_oid_expect));
 }
 
 void apply_push_cas(struct push_cas_option *cas,
diff --git a/remote.h b/remote.h
index c21fd37..9248811 100644
--- a/remote.h
+++ b/remote.h
@@ -89,7 +89,6 @@ struct ref {
 		force:1,
 		forced_update:1,
 		expect_old_sha1:1,
-		expect_old_no_trackback:1,
 		deletion:1,
 		matched:1;
 
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index c732012..4276b1b 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,4 +191,42 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		git branch branch master &&
+		git push --force-with-lease=branch origin branch
+	) &&
+	git ls-remote dst refs/heads/branch >expect &&
+	git ls-remote src refs/heads/branch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'new branch covered by force-with-lease (explicit)' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		git branch branch master &&
+		git push --force-with-lease=branch:0000000000000000000000000000000000000000 origin branch
+	) &&
+	git ls-remote dst refs/heads/branch >expect &&
+	git ls-remote src refs/heads/branch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'new branch already exists' '
+	setup_srcdst_basic &&
+	(
+		cd src &&
+		git checkout -b branch master &&
+		test_commit c
+	) &&
+	(
+		cd dst &&
+		git branch branch master &&
+		test_must_fail git push --force-with-lease=branch origin branch
+	)
+'
+
 test_done
-- 
2.9.2.637.g8b832fc

Re: [PATCH] push: allow pushing new branches with --force-with-lease

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> If there is no upstream information for a branch, it is likely that it
> is newly created and can safely be pushed under the normal fast-forward
> rules.  Relax the --force-with-lease check so that we do not reject
> these branches immediately but rather attempt to push them as new
> branches, using the null SHA-1 as the expected value.
>
> In fact, it is already possible to push new branches using the explicit
> --force-with-lease=<branch>:<expect> syntax, so all we do here is make
> this behaviour the default if no explicit "expect" value is specified.

I like the loss of an extra field from "struct ref".

I suspect that the if/else cascade in the loop in apply_cas() can
also be taught that ':' followed by an empty string asks to check
that the target ref does not exist, in order to make it a bit more
useful for folks who do not rely on the "use the last observed
status of the tracking branch".

That would make the "explicit" test much less cumbersome to read.


> +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> +	setup_srcdst_basic &&
> +	(
> +		cd dst &&
> +		git branch branch master &&
> +		git push --force-with-lease=branch:0000000000000000000000000000000000000000 origin branch
> +	) &&

[PATCH v2 0/3] push: allow pushing new branches with --force-with-lease

[John Keeping]

On Mon, Jul 25, 2016 at 10:28:01AM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > If there is no upstream information for a branch, it is likely that it
> > is newly created and can safely be pushed under the normal fast-forward
> > rules.  Relax the --force-with-lease check so that we do not reject
> > these branches immediately but rather attempt to push them as new
> > branches, using the null SHA-1 as the expected value.
> >
> > In fact, it is already possible to push new branches using the explicit
> > --force-with-lease=<branch>:<expect> syntax, so all we do here is make
> > this behaviour the default if no explicit "expect" value is specified.
> 
> I like the loss of an extra field from "struct ref".
> 
> I suspect that the if/else cascade in the loop in apply_cas() can
> also be taught that ':' followed by an empty string asks to check
> that the target ref does not exist, in order to make it a bit more
> useful for folks who do not rely on the "use the last observed
> status of the tracking branch".
> 
> That would make the "explicit" test much less cumbersome to read.

Yes, that's nicer and it mirrors the syntax for deleting a remote
branch.

I've pulled it out as a preparatory step because I like the fact that
the "explicit" test passes even before the patch that is the main point
of the series.

> > +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> > +   setup_srcdst_basic &&
> > +   (
> > +           cd dst &&
> > +           git branch branch master &&
> > +           git push --force-with-lease=branch:0000000000000000000000000000000000000000 origin branch
> > +   ) &&

John Keeping (3):
  Documentation/git-push: fix placeholder formatting
  push: add shorthand for --force-with-lease branch creation
  push: allow pushing new branches with --force-with-lease

 Documentation/git-push.txt |  5 +++--
 remote.c                   |  9 +++++----
 remote.h                   |  1 -
 t/t5533-push-cas.sh        | 38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 7 deletions(-)

-- 
2.9.2.639.g855ae9f

[PATCH v2 1/3] Documentation/git-push: fix placeholder formatting

[John Keeping]

Format the placeholder as monospace to match other occurrences in this
file and obey CodingGuidelines.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
New in v2.

 Documentation/git-push.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 93c3527..bf7c9a2 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -198,7 +198,7 @@ branch we have for it.
 +
 `--force-with-lease=<refname>:<expect>` will protect the named ref (alone),
 if it is going to be updated, by requiring its current value to be
-the same as the specified value <expect> (which is allowed to be
+the same as the specified value `<expect>` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
 this form is used).
-- 
2.9.2.639.g855ae9f

[PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[John Keeping]

Allow the empty string to stand in for the null SHA-1 when pushing a new
branch, like we do when deleting branches.

This means that the following command ensures that `new-branch` is
created on the remote (that is, is must not already exist):

	git push --force-with-lease=new-branch: origin new-branch

Signed-off-by: John Keeping <john@keeping.me.uk>
---
New in v2.

 Documentation/git-push.txt |  3 ++-
 remote.c                   |  2 ++
 t/t5533-push-cas.sh        | 12 ++++++++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index bf7c9a2..927a034 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -201,7 +201,8 @@ if it is going to be updated, by requiring its current value to be
 the same as the specified value `<expect>` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
-this form is used).
+this form is used).  If `<expect>` is the empty string, then the named ref
+must not already exist.
 +
 Note that all forms other than `--force-with-lease=<refname>:<expect>`
 that specifies the expected current value of the ref explicitly are
diff --git a/remote.c b/remote.c
index a326e4e..af94892 100644
--- a/remote.c
+++ b/remote.c
@@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse
 	entry = add_cas_entry(cas, arg, colon - arg);
 	if (!*colon)
 		entry->use_tracking = 1;
+	else if (!colon[1])
+		memset(entry->expect, 0, sizeof(entry->expect));
 	else if (get_sha1(colon + 1, entry->expect))
 		return error("cannot parse expected object name '%s'", colon + 1);
 	return 0;
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index c732012..5e7f6e9 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,4 +191,16 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease (explicit)' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		git branch branch master &&
+		git push --force-with-lease=branch: origin branch
+	) &&
+	git ls-remote dst refs/heads/branch >expect &&
+	git ls-remote src refs/heads/branch >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.9.2.639.g855ae9f

[PATCH v2 3/3] push: allow pushing new branches with --force-with-lease

[John Keeping]

If there is no upstream information for a branch, it is likely that it
is newly created and can safely be pushed under the normal fast-forward
rules.  Relax the --force-with-lease check so that we do not reject
these branches immediately but rather attempt to push them as new
branches, using the null SHA-1 as the expected value.

In fact, it is already possible to push new branches using the explicit
--force-with-lease=<branch>:<expect> syntax, so all we do here is make
this behaviour the default if no explicit "expect" value is specified.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
Changes in v2:
- The "explicit" test was previously in this patch but is now added in
  patch 2/3.

 remote.c            |  7 +++----
 remote.h            |  1 -
 t/t5533-push-cas.sh | 26 ++++++++++++++++++++++++++
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index af94892..20e174d 100644
--- a/remote.c
+++ b/remote.c
@@ -1544,8 +1544,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 * branch.
 		 */
 		if (ref->expect_old_sha1) {
-			if (ref->expect_old_no_trackback ||
-			    oidcmp(&ref->old_oid, &ref->old_oid_expect))
+			if (oidcmp(&ref->old_oid, &ref->old_oid_expect))
 				reject_reason = REF_STATUS_REJECT_STALE;
 			else
 				/* If the ref isn't stale then force the update. */
@@ -2345,7 +2344,7 @@ static void apply_cas(struct push_cas_option *cas,
 		if (!entry->use_tracking)
 			hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
 		else if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
-			ref->expect_old_no_trackback = 1;
+			memset(&ref->old_oid_expect, 0, sizeof(ref->old_oid_expect));
 		return;
 	}
 
@@ -2355,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas,
 
 	ref->expect_old_sha1 = 1;
 	if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
-		ref->expect_old_no_trackback = 1;
+		memset(&ref->old_oid_expect, 0, sizeof(ref->old_oid_expect));
 }
 
 void apply_push_cas(struct push_cas_option *cas,
diff --git a/remote.h b/remote.h
index c21fd37..9248811 100644
--- a/remote.h
+++ b/remote.h
@@ -89,7 +89,6 @@ struct ref {
 		force:1,
 		forced_update:1,
 		expect_old_sha1:1,
-		expect_old_no_trackback:1,
 		deletion:1,
 		matched:1;
 
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index 5e7f6e9..5f29664 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,6 +191,18 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		git branch branch master &&
+		git push --force-with-lease=branch origin branch
+	) &&
+	git ls-remote dst refs/heads/branch >expect &&
+	git ls-remote src refs/heads/branch >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'new branch covered by force-with-lease (explicit)' '
 	setup_srcdst_basic &&
 	(
@@ -203,4 +215,18 @@ test_expect_success 'new branch covered by force-with-lease (explicit)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'new branch already exists' '
+	setup_srcdst_basic &&
+	(
+		cd src &&
+		git checkout -b branch master &&
+		test_commit c
+	) &&
+	(
+		cd dst &&
+		git branch branch master &&
+		test_must_fail git push --force-with-lease=branch origin branch
+	)
+'
+
 test_done
-- 
2.9.2.639.g855ae9f

Re: [PATCH] push: allow pushing new branches with --force-with-lease

[Junio C Hamano]

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

> I suspect that the if/else cascade in the loop in apply_cas() can
> also be taught that ':' followed by an empty string asks to check
> that the target ref does not exist, in order to make it a bit more
> useful for folks who do not rely on the "use the last observed
> status of the tracking branch".
>
> That would make the "explicit" test much less cumbersome to read.

In other words, something like this, perhaps?

 remote.c            |  2 ++
 t/t5533-push-cas.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/remote.c b/remote.c
index b35ffd9..55812d8 100644
--- a/remote.c
+++ b/remote.c
@@ -2303,6 +2303,8 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse
 	entry = add_cas_entry(cas, arg, colon - arg);
 	if (!*colon)
 		entry->use_tracking = 1;
+	else if (!colon[1])
+		hashclr(entry->expect);
 	else if (get_sha1(colon + 1, entry->expect))
 		return error("cannot parse expected object name '%s'", colon + 1);
 	return 0;
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index 4276b1b..04f4636 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -215,6 +215,18 @@ test_expect_success 'new branch covered by force-with-lease (explicit)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease (less cumbersome)' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		git branch another master &&
+		git push --force-with-lease=another: origin another
+	) &&
+	git ls-remote dst refs/heads/another >expect &&
+	git ls-remote src refs/heads/another >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'new branch already exists' '
 	setup_srcdst_basic &&
 	(
-- 
2.9.2-629-gdd92683

Re: [PATCH v2 0/3] push: allow pushing new branches with --force-with-lease

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Mon, Jul 25, 2016 at 10:28:01AM -0700, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>> 
>> > If there is no upstream information for a branch, it is likely that it
>> > is newly created and can safely be pushed under the normal fast-forward
>> > rules.  Relax the --force-with-lease check so that we do not reject
>> > these branches immediately but rather attempt to push them as new
>> > branches, using the null SHA-1 as the expected value.
>> >
>> > In fact, it is already possible to push new branches using the explicit
>> > --force-with-lease=<branch>:<expect> syntax, so all we do here is make
>> > this behaviour the default if no explicit "expect" value is specified.
>> 
>> I like the loss of an extra field from "struct ref".
>> 
>> I suspect that the if/else cascade in the loop in apply_cas() can
>> also be taught that ':' followed by an empty string asks to check
>> that the target ref does not exist, in order to make it a bit more
>> useful for folks who do not rely on the "use the last observed
>> status of the tracking branch".
>> 
>> That would make the "explicit" test much less cumbersome to read.
>
> Yes, that's nicer and it mirrors the syntax for deleting a remote
> branch.
>
> I've pulled it out as a preparatory step because I like the fact that
> the "explicit" test passes even before the patch that is the main point
> of the series.

Ah, our mails crossed ;-)

Thanks, I'll read these three patches.

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> Allow the empty string to stand in for the null SHA-1 when pushing a new
> branch, like we do when deleting branches.
>
> This means that the following command ensures that `new-branch` is
> created on the remote (that is, is must not already exist):
>
> 	git push --force-with-lease=new-branch: origin new-branch
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> New in v2.
>
>  Documentation/git-push.txt |  3 ++-
>  remote.c                   |  2 ++
>  t/t5533-push-cas.sh        | 12 ++++++++++++
>  3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index bf7c9a2..927a034 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -201,7 +201,8 @@ if it is going to be updated, by requiring its current value to be
>  the same as the specified value `<expect>` (which is allowed to be
>  different from the remote-tracking branch we have for the refname,
>  or we do not even have to have such a remote-tracking branch when
> -this form is used).
> +this form is used).  If `<expect>` is the empty string, then the named ref
> +must not already exist.
>  +
>  Note that all forms other than `--force-with-lease=<refname>:<expect>`
>  that specifies the expected current value of the ref explicitly are
> diff --git a/remote.c b/remote.c
> index a326e4e..af94892 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse
>  	entry = add_cas_entry(cas, arg, colon - arg);
>  	if (!*colon)
>  		entry->use_tracking = 1;
> +	else if (!colon[1])
> +		memset(entry->expect, 0, sizeof(entry->expect));

hashclr()?

>  	else if (get_sha1(colon + 1, entry->expect))
>  		return error("cannot parse expected object name '%s'", colon + 1);
>  	return 0;
> diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
> index c732012..5e7f6e9 100755
> --- a/t/t5533-push-cas.sh
> +++ b/t/t5533-push-cas.sh
> @@ -191,4 +191,16 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> +	setup_srcdst_basic &&
> +	(
> +		cd dst &&
> +		git branch branch master &&
> +		git push --force-with-lease=branch: origin branch
> +	) &&
> +	git ls-remote dst refs/heads/branch >expect &&
> +	git ls-remote src refs/heads/branch >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[John Keeping]

On Mon, Jul 25, 2016 at 03:22:48PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > Allow the empty string to stand in for the null SHA-1 when pushing a new
> > branch, like we do when deleting branches.
> >
> > This means that the following command ensures that `new-branch` is
> > created on the remote (that is, is must not already exist):
> >
> > 	git push --force-with-lease=new-branch: origin new-branch
> >
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> > New in v2.
> >
> >  Documentation/git-push.txt |  3 ++-
> >  remote.c                   |  2 ++
> >  t/t5533-push-cas.sh        | 12 ++++++++++++
> >  3 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> > index bf7c9a2..927a034 100644
> > --- a/Documentation/git-push.txt
> > +++ b/Documentation/git-push.txt
> > @@ -201,7 +201,8 @@ if it is going to be updated, by requiring its current value to be
> >  the same as the specified value `<expect>` (which is allowed to be
> >  different from the remote-tracking branch we have for the refname,
> >  or we do not even have to have such a remote-tracking branch when
> > -this form is used).
> > +this form is used).  If `<expect>` is the empty string, then the named ref
> > +must not already exist.
> >  +
> >  Note that all forms other than `--force-with-lease=<refname>:<expect>`
> >  that specifies the expected current value of the ref explicitly are
> > diff --git a/remote.c b/remote.c
> > index a326e4e..af94892 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse
> >  	entry = add_cas_entry(cas, arg, colon - arg);
> >  	if (!*colon)
> >  		entry->use_tracking = 1;
> > +	else if (!colon[1])
> > +		memset(entry->expect, 0, sizeof(entry->expect));
> 
> hashclr()?

Yes (and in the following patch as well).  I hadn't realised that
function exists.

> >  	else if (get_sha1(colon + 1, entry->expect))
> >  		return error("cannot parse expected object name '%s'", colon + 1);
> >  	return 0;
> > diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
> > index c732012..5e7f6e9 100755
> > --- a/t/t5533-push-cas.sh
> > +++ b/t/t5533-push-cas.sh
> > @@ -191,4 +191,16 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' '
> >  	test_cmp expect actual
> >  '
> >  
> > +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> > +	setup_srcdst_basic &&
> > +	(
> > +		cd dst &&
> > +		git branch branch master &&
> > +		git push --force-with-lease=branch: origin branch
> > +	) &&
> > +	git ls-remote dst refs/heads/branch >expect &&
> > +	git ls-remote src refs/heads/branch >actual &&
> > +	test_cmp expect actual
> > +'
> > +
> >  test_done

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[Jakub Narębski]

W dniu 2016-07-25 o 23:59, John Keeping pisze:

> +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> +	setup_srcdst_basic &&
> +	(
> +		cd dst &&
> +		git branch branch master &&
> +		git push --force-with-lease=branch: origin branch
> +	) &&
> +	git ls-remote dst refs/heads/branch >expect &&
> +	git ls-remote src refs/heads/branch >actual &&
> +	test_cmp expect actual
> +'

Do we need to test the negative, that is that if branch is not
new it prevents push (e.g. when <branch> is HEAD), or is it
covered by other tests?

-- 
Jakub Narębski

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[John Keeping]

On Tue, Jul 26, 2016 at 12:30:05PM +0200, Jakub Narębski wrote:
> W dniu 2016-07-25 o 23:59, John Keeping pisze:
> 
> > +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> > +	setup_srcdst_basic &&
> > +	(
> > +		cd dst &&
> > +		git branch branch master &&
> > +		git push --force-with-lease=branch: origin branch
> > +	) &&
> > +	git ls-remote dst refs/heads/branch >expect &&
> > +	git ls-remote src refs/heads/branch >actual &&
> > +	test_cmp expect actual
> > +'
> 
> Do we need to test the negative, that is that if branch is not
> new it prevents push (e.g. when <branch> is HEAD), or is it
> covered by other tests?

It's covered by a test in patch 3 (at least for the implicit case added
there), but I could pull that forwards.  In fact, converting that test
to the explicit syntax will make it simpler since we won't need to set
up a non-fast-forward push.

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

>> > @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse
>> >  	entry = add_cas_entry(cas, arg, colon - arg);
>> >  	if (!*colon)
>> >  		entry->use_tracking = 1;
>> > +	else if (!colon[1])
>> > +		memset(entry->expect, 0, sizeof(entry->expect));
>> 
>> hashclr()?
>
> Yes (and in the following patch as well).  I hadn't realised that
> function exists.

Thanks; I've locally tweaked these two patches; the interdiff looks
like this.

 remote.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index e8b7bac..7eaf3c8 100644
--- a/remote.c
+++ b/remote.c
@@ -2304,7 +2304,7 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse
 	if (!*colon)
 		entry->use_tracking = 1;
 	else if (!colon[1])
-		memset(entry->expect, 0, sizeof(entry->expect));
+		hashclr(entry->expect);
 	else if (get_sha1(colon + 1, entry->expect))
 		return error("cannot parse expected object name '%s'", colon + 1);
 	return 0;
@@ -2354,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas,
 		if (!entry->use_tracking)
 			hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
 		else if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
-			memset(&ref->old_oid_expect, 0, sizeof(ref->old_oid_expect));
+			hashclr(ref->old_oid_expect.hash);
 		return;
 	}
 
@@ -2364,7 +2364,7 @@ static void apply_cas(struct push_cas_option *cas,
 
 	ref->expect_old_sha1 = 1;
 	if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
-		memset(&ref->old_oid_expect, 0, sizeof(ref->old_oid_expect));
+		hashclr(ref->old_oid_expect.hash);
 }
 
 void apply_push_cas(struct push_cas_option *cas,

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[John Keeping]

On Tue, Jul 26, 2016 at 12:59:04PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> >> > @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse
> >> >  	entry = add_cas_entry(cas, arg, colon - arg);
> >> >  	if (!*colon)
> >> >  		entry->use_tracking = 1;
> >> > +	else if (!colon[1])
> >> > +		memset(entry->expect, 0, sizeof(entry->expect));
> >> 
> >> hashclr()?
> >
> > Yes (and in the following patch as well).  I hadn't realised that
> > function exists.
> 
> Thanks; I've locally tweaked these two patches; the interdiff looks
> like this.

Thanks.  I'm about to send v3 anyway to pull a test forward to address
Jakub's comment.  I also used oidclr() for the last two changes below.

>  remote.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/remote.c b/remote.c
> index e8b7bac..7eaf3c8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2304,7 +2304,7 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse
>  	if (!*colon)
>  		entry->use_tracking = 1;
>  	else if (!colon[1])
> -		memset(entry->expect, 0, sizeof(entry->expect));
> +		hashclr(entry->expect);
>  	else if (get_sha1(colon + 1, entry->expect))
>  		return error("cannot parse expected object name '%s'", colon + 1);
>  	return 0;
> @@ -2354,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas,
>  		if (!entry->use_tracking)
>  			hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
>  		else if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
> -			memset(&ref->old_oid_expect, 0, sizeof(ref->old_oid_expect));
> +			hashclr(ref->old_oid_expect.hash);
>  		return;
>  	}
>  
> @@ -2364,7 +2364,7 @@ static void apply_cas(struct push_cas_option *cas,
>  
>  	ref->expect_old_sha1 = 1;
>  	if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
> -		memset(&ref->old_oid_expect, 0, sizeof(ref->old_oid_expect));
> +		hashclr(ref->old_oid_expect.hash);
>  }
>  
>  void apply_push_cas(struct push_cas_option *cas,

[PATCH v3 0/3] push: allow pushing new branches with --force-with-lease

[John Keeping]

Changes in v3:
- Use hashclr() and oidclr() where appropriate instead of memset()
- Pull a test forward from patch 3 to patch 2 

John Keeping (3):
  Documentation/git-push: fix placeholder formatting
  push: add shorthand for --force-with-lease branch creation
  push: allow pushing new branches with --force-with-lease

 Documentation/git-push.txt |  5 +++--
 remote.c                   |  9 +++++----
 remote.h                   |  1 -
 t/t5533-push-cas.sh        | 38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 7 deletions(-)

-- 
2.9.2.639.g855ae9f

[PATCH v3 1/3] Documentation/git-push: fix placeholder formatting

[John Keeping]

Format the placeholder as monospace to match other occurrences in this
file and obey CodingGuidelines.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
No changes in v3.

 Documentation/git-push.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 93c3527..bf7c9a2 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -198,7 +198,7 @@ branch we have for it.
 +
 `--force-with-lease=<refname>:<expect>` will protect the named ref (alone),
 if it is going to be updated, by requiring its current value to be
-the same as the specified value <expect> (which is allowed to be
+the same as the specified value `<expect>` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
 this form is used).
-- 
2.9.2.639.g855ae9f

[PATCH v3 2/3] push: add shorthand for --force-with-lease branch creation

[John Keeping]

Allow the empty string to stand in for the null SHA-1 when pushing a new
branch, like we do when deleting branches.

This means that the following command ensures that `new-branch` is
created on the remote (that is, is must not already exist):

	git push --force-with-lease=new-branch: origin new-branch

Signed-off-by: John Keeping <john@keeping.me.uk>
---
Changes in v3:
- use hashclr()
- pull 'new branch already exists' test forward from patch 3 and use
  explicit --force-with-lease syntax

 Documentation/git-push.txt |  3 ++-
 remote.c                   |  2 ++
 t/t5533-push-cas.sh        | 26 ++++++++++++++++++++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index bf7c9a2..927a034 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -201,7 +201,8 @@ if it is going to be updated, by requiring its current value to be
 the same as the specified value `<expect>` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
-this form is used).
+this form is used).  If `<expect>` is the empty string, then the named ref
+must not already exist.
 +
 Note that all forms other than `--force-with-lease=<refname>:<expect>`
 that specifies the expected current value of the ref explicitly are
diff --git a/remote.c b/remote.c
index a326e4e..42c4a34 100644
--- a/remote.c
+++ b/remote.c
@@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse
 	entry = add_cas_entry(cas, arg, colon - arg);
 	if (!*colon)
 		entry->use_tracking = 1;
+	else if (!colon[1])
+		hashclr(entry->expect);
 	else if (get_sha1(colon + 1, entry->expect))
 		return error("cannot parse expected object name '%s'", colon + 1);
 	return 0;
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index c732012..ed631c3 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,4 +191,30 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease (explicit)' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		git branch branch master &&
+		git push --force-with-lease=branch: origin branch
+	) &&
+	git ls-remote dst refs/heads/branch >expect &&
+	git ls-remote src refs/heads/branch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'new branch already exists' '
+	setup_srcdst_basic &&
+	(
+		cd src &&
+		git checkout -b branch master &&
+		test_commit c
+	) &&
+	(
+		cd dst &&
+		git branch branch master &&
+		test_must_fail git push --force-with-lease=branch: origin branch
+	)
+'
+
 test_done
-- 
2.9.2.639.g855ae9f

[PATCH v3 3/3] push: allow pushing new branches with --force-with-lease

[John Keeping]

If there is no upstream information for a branch, it is likely that it
is newly created and can safely be pushed under the normal fast-forward
rules.  Relax the --force-with-lease check so that we do not reject
these branches immediately but rather attempt to push them as new
branches, using the null SHA-1 as the expected value.

In fact, it is already possible to push new branches using the explicit
--force-with-lease=<branch>:<expect> syntax, so all we do here is make
this behaviour the default if no explicit "expect" value is specified.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
Changes in v3:
- use oidclr()
- final test is now added in the previous patch and now uses the
  explicit --force-with-lease syntax

 remote.c            |  7 +++----
 remote.h            |  1 -
 t/t5533-push-cas.sh | 12 ++++++++++++
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 42c4a34..d29850a 100644
--- a/remote.c
+++ b/remote.c
@@ -1544,8 +1544,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 * branch.
 		 */
 		if (ref->expect_old_sha1) {
-			if (ref->expect_old_no_trackback ||
-			    oidcmp(&ref->old_oid, &ref->old_oid_expect))
+			if (oidcmp(&ref->old_oid, &ref->old_oid_expect))
 				reject_reason = REF_STATUS_REJECT_STALE;
 			else
 				/* If the ref isn't stale then force the update. */
@@ -2345,7 +2344,7 @@ static void apply_cas(struct push_cas_option *cas,
 		if (!entry->use_tracking)
 			hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
 		else if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
-			ref->expect_old_no_trackback = 1;
+			oidclr(&ref->old_oid_expect);
 		return;
 	}
 
@@ -2355,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas,
 
 	ref->expect_old_sha1 = 1;
 	if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
-		ref->expect_old_no_trackback = 1;
+		oidclr(&ref->old_oid_expect);
 }
 
 void apply_push_cas(struct push_cas_option *cas,
diff --git a/remote.h b/remote.h
index c21fd37..9248811 100644
--- a/remote.h
+++ b/remote.h
@@ -89,7 +89,6 @@ struct ref {
 		force:1,
 		forced_update:1,
 		expect_old_sha1:1,
-		expect_old_no_trackback:1,
 		deletion:1,
 		matched:1;
 
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index ed631c3..09899af 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,6 +191,18 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		git branch branch master &&
+		git push --force-with-lease=branch origin branch
+	) &&
+	git ls-remote dst refs/heads/branch >expect &&
+	git ls-remote src refs/heads/branch >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'new branch covered by force-with-lease (explicit)' '
 	setup_srcdst_basic &&
 	(
-- 
2.9.2.639.g855ae9f

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> Thanks.  I'm about to send v3 anyway to pull a test forward to address
> Jakub's comment.  I also used oidclr() for the last two changes below.

Will replace with v3.

I think v3 is ready to advance to 'next'.  Let's see if we get
further comments from others for a few days.

Thanks.