threads / patch / 42912

patchpush: allow pushing new branches with --force-with-lease

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

## tl;dr

19 messages between Jul 23, 2016 and Jul 26, 2016. Diffs are folded; open one to read it.

replies: 18people: 3as markdown or json

John Keeping· Jul 23, 2016, 15:31 UTC · lore

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(-)
Show changes to 3 files +41 −5

remote.c, remote.h, t/t5533-push-cas.sh

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
Junio C Hamano· Jul 25, 2016, 17:28 UTC · re: John Keeping · lore

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

John Keeping <john@keeping.me.uk> writes:
Show 9 quoted lines
> 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.
Show 7 quoted lines
> +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· Jul 25, 2016, 21:59 UTC · re: Junio C Hamano · lore

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

On Mon, Jul 25, 2016 at 10:28:01AM -0700, Junio C Hamano wrote:
Show 21 quoted lines
> 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.

Show 7 quoted lines
> > +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
John Keeping· Jul 25, 2016, 21:59 UTC · re: John Keeping · lore

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

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(-)
Show changes to Documentation/git-push.txt +1 −1
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
John Keeping· Jul 25, 2016, 21:59 UTC · re: John Keeping · lore

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

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(-)
Show changes to 3 files +16 −1

Documentation/git-push.txt, remote.c, t/t5533-push-cas.sh

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
Junio C Hamano· Jul 25, 2016, 22:22 UTC · re: John Keeping · lore

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

John Keeping <john@keeping.me.uk> writes:
Show 41 quoted lines
> 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()?
Show 24 quoted lines
>  	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
John Keeping· Jul 26, 2016, 08:03 UTC · re: Junio C Hamano · lore

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

On Mon, Jul 25, 2016 at 03:22:48PM -0700, Junio C Hamano wrote:
Show 45 quoted lines
> 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.

Show 24 quoted lines
> >  	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
Junio C Hamano· Jul 26, 2016, 19:59 UTC · re: John Keeping · lore

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

John Keeping <john@keeping.me.uk> writes:
Show 11 quoted lines
>> > @@ -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(-)
Show changes to remote.c +3 −3
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,
John Keeping· Jul 26, 2016, 20:42 UTC · re: Junio C Hamano · lore

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

On Tue, Jul 26, 2016 at 12:59:04PM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> 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.

Show 34 quoted lines
>  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,
Junio C Hamano· Jul 26, 2016, 21:19 UTC · re: John Keeping · lore

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

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.
Jakub Narębski· Jul 26, 2016, 10:30 UTC · re: John Keeping · lore

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

W dniu 2016-07-25 o 23:59, John Keeping pisze:
Show 11 quoted lines
> +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
John Keeping· Jul 26, 2016, 11:19 UTC · re: Jakub Narębski · lore

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

On Tue, Jul 26, 2016 at 12:30:05PM +0200, Jakub Narębski wrote:
Show 17 quoted lines
> 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.

John Keeping· Jul 25, 2016, 21:59 UTC · re: John Keeping · lore

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

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(-)
Show changes to 3 files +29 −5

remote.c, remote.h, t/t5533-push-cas.sh

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
Junio C Hamano· Jul 25, 2016, 22:21 UTC · re: John Keeping · lore

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

John Keeping <john@keeping.me.uk> writes:
Show 29 quoted lines
> 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.
John Keeping· Jul 26, 2016, 20:44 UTC · re: John Keeping · lore

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

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
John Keeping· Jul 26, 2016, 20:44 UTC · re: John Keeping · lore

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

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(-)
Show changes to Documentation/git-push.txt +1 −1
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
John Keeping· Jul 26, 2016, 20:44 UTC · re: John Keeping · lore

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

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(-)
Show changes to 3 files +30 −1

Documentation/git-push.txt, remote.c, t/t5533-push-cas.sh

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
John Keeping· Jul 26, 2016, 20:44 UTC · re: John Keeping · lore

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

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(-)
Show changes to 3 files +15 −5

remote.c, remote.h, t/t5533-push-cas.sh

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
Junio C Hamano· Jul 25, 2016, 22:11 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> 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(+)
Show changes to 2 files +14 −0

remote.c, t/t5533-push-cas.sh

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

← back to recent threads