threads / patch / 58128

patchrev-parse: respect push.autosetupremote when evaluating @{push}

Subject: [PATCH] rev-parse: respect push.autosetupremote when evaluating @{push}

## tl;dr

4 messages between Jul 10, 2022 and May 28, 2023. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Tao Klerks via GitGitGadget· Jul 10, 2022, 19:16 UTC · lore
From: Tao Klerks <tao@klerks.biz>

In a previous release, the push.autosetupremote config was introduced to ease new branch management in "simple" single-remote workflows. This makes "git push" work on new branches (without configured upstream) with push.default set to "simple" or "upstream" and it implies "--set-upstream" regardless of whether the same-name remote branch exists or not.

The "@{push}" suffix logic was not adjusted to account for this new option, however, and sometimes returns an error when "git push" would successfully push to an existing remote branch.

This is an edge-case, as the main context where push.autosetupremote will apply is for *new* branches, with no corresponding remote branch yet, and so even if the defaulting is handled correctly, the rev-parse will still fail with "unknown revision or path not in the working tree".

Fix this edge-case so "git rev-parse @{push}" works, if there is no upstream tracking relationship set up but the remote tracking branch that will be defaulted to does exist and can be resolved.

Also add corresponding test cases.
Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    rev-parse: respect push.autosetupremote when evaluating @{push}
    
    Minor consistency fix for previously-introduced "push.autosetupremote"
    option.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1279%2FTaoK%2Ftao-rev-parse-autosetupremote-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1279/TaoK/tao-rev-parse-autosetupremote-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1279
 remote.c                  | 30 ++++++++++++++++++++++++++++--
 t/t1514-rev-parse-push.sh | 20 ++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)
Show changes to 2 files +48 −2

remote.c, t/t1514-rev-parse-push.sh

diff --git a/remote.c b/remote.c
index b19e3a2f015..59f5bf5b5f5 100644
--- a/remote.c
+++ b/remote.c
@@ -1919,6 +1919,23 @@ static const char *tracking_for_push_dest(struct remote *remote,
 	return ret;
 }
 
+static const char *default_missing_upstream(struct remote *remote,
+				    struct branch *branch,
+				    struct strbuf *err)
+{
+	int autosetupremote = 0;
+
+	if (branch && (!branch->merge || !branch->merge[0])) {
+		repo_config_get_bool(the_repository,
+				     "push.autosetupremote",
+				     &autosetupremote);
+		if (autosetupremote)
+			return tracking_for_push_dest(remote, branch->refname, err);
+	}
+
+	return NULL;
+}
+
 static const char *branch_get_push_1(struct remote_state *remote_state,
 				     struct branch *branch, struct strbuf *err)
 {
@@ -1959,13 +1976,22 @@ static const char *branch_get_push_1(struct remote_state *remote_state,
 		return tracking_for_push_dest(remote, branch->refname, err);
 
 	case PUSH_DEFAULT_UPSTREAM:
-		return branch_get_upstream(branch, err);
-
+		{
+			const char *up;
+			up = default_missing_upstream(remote, branch, err);
+			if (up)
+				return up;
+			return branch_get_upstream(branch, err);
+		}
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
 		{
 			const char *up, *cur;
 
+			up = default_missing_upstream(remote, branch, err);
+			if (up)
+				return up;
+
 			up = branch_get_upstream(branch, err);
 			if (!up)
 				return NULL;
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index d868a081105..ffa0db14585 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -21,7 +21,10 @@ test_expect_success 'setup' '
 	git push origin HEAD &&
 	git branch --set-upstream-to=origin/main main &&
 	git branch --track topic origin/main &&
+	git branch --no-track indie_topic origin/main &&
+	git branch --no-track new_topic origin/main &&
 	git push origin topic &&
+	git push origin indie_topic &&
 	git push other topic
 '
 
@@ -73,4 +76,21 @@ test_expect_success 'resolving @{push} fails with a detached HEAD' '
 	test_must_fail git rev-parse @{push}
 '
 
+test_expect_success '@{push} with default=simple without tracking' '
+	test_config push.default simple &&
+	test_must_fail git rev-parse indie_topic@{push}
+'
+
+test_expect_success '@{push} with default=simple with autosetupremote' '
+	test_config push.default simple &&
+	test_config push.autosetupremote true &&
+	resolve indie_topic@{push} refs/remotes/origin/indie_topic
+'
+
+test_expect_success '@{push} with default=simple with autosetupremote, new branch' '
+	test_config push.default simple &&
+	test_config push.autosetupremote true &&
+	test_must_fail git rev-parse new_topic@{push}
+'
+
 test_done

base-commit: 39c15e485575089eb77c769f6da02f98a55905e0
-- 
gitgitgadget
Junio C Hamano· Jul 10, 2022, 20:42 UTC · re: Tao Klerks via GitGitGadget · lore

Re: [PATCH] rev-parse: respect push.autosetupremote when evaluating @{push}

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> +	if (branch && (!branch->merge || !branch->merge[0])) {
> +		repo_config_get_bool(the_repository,
> +				     "push.autosetupremote",
> +				     &autosetupremote);
> +		if (autosetupremote)
> +			return tracking_for_push_dest(remote, branch->refname, err);

Before the first push of the branch X where we are asking for X@{push}, i.e. there is not the corresponding branch over there yet and we do not have the remote-tracking branch for it yet, what does this function return? If it continues to error out, then I think this patch may make sense, but ...

Show 6 quoted lines
> +		{
> +			const char *up;
> +			up = default_missing_upstream(remote, branch, err);
> +			if (up)
> +				return up;
> +			return branch_get_upstream(branch, err);
... shouldn't the precedence order the other way around here ...
Show 13 quoted lines
> +		}
>  	case PUSH_DEFAULT_UNSPECIFIED:
>  	case PUSH_DEFAULT_SIMPLE:
>  		{
>  			const char *up, *cur;
>  
> +			up = default_missing_upstream(remote, branch, err);
> +			if (up)
> +				return up;
> +
>  			up = branch_get_upstream(branch, err);
>  			if (!up)
>  				return NULL;

... and here? That is, if branch_get_upstream() finds an explicitly configured one, shouldn't we use that and fall back to the new "missing" code path only when there isn't an explicitly configured one?

Tao Klerks· Jul 16, 2022, 17:40 UTC · re: Junio C Hamano · lore

Re: [PATCH] rev-parse: respect push.autosetupremote when evaluating @{push}

On Sun, Jul 10, 2022 at 10:42 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
Thanks so much for looking at this, my apologies for the delayed response.
Show 13 quoted lines
>
> > +     if (branch && (!branch->merge || !branch->merge[0])) {
> > +             repo_config_get_bool(the_repository,
> > +                                  "push.autosetupremote",
> > +                                  &autosetupremote);
> > +             if (autosetupremote)
> > +                     return tracking_for_push_dest(remote, branch->refname, err);
>
> Before the first push of the branch X where we are asking for
> X@{push}, i.e. there is not the corresponding branch over there yet
> and we do not have the remote-tracking branch for it yet, what does
> this function return?  If it continues to error out, then I think
> this patch may make sense, but ...

It does indeed still error out, because even though "git push" may succeed (eg if "push.autosetupremote=true"), there simply isn't a remote-tracking branch for it yet.

The only "point" of the patch is to address the edge-case where the remote-tracking branch *does* exist, and git push *would* push to it, on the basis of the new "push.autosetupremote=true" behavior.

In that very specific case, it's "wrong" to return an error just because the tracking relationship doesn't exist yet.

Show 9 quoted lines
>
> > +             {
> > +                     const char *up;
> > +                     up = default_missing_upstream(remote, branch, err);
> > +                     if (up)
> > +                             return up;
> > +                     return branch_get_upstream(branch, err);
>
> ... shouldn't the precedence order the other way around here ...
This is a bit convoluted, but I don't see how to make it more obvious.

The new "default_missing_upstream" function checks the *same conditions* - it only "kicks in" if "branch_get_upstream()" would return an error.

The reasons I can't add the new logic *after* or *in*
"branch_get_upstream()", and avoid this non-obviousness, are that:
* "branch_get_upstream()" raises an error, so recovering afterwards
seems non-trivial (to me at least - I haven't got my head around the
"idiom" yet)
* "branch_get_upstream()" is used in a a few other places for other
purposes, so I'm not comfortable modifying it for my purposes
* "branch_get_upstream()" is non-trivial, so I'm not comfortable
duplicating it / creating a new variant
I think I see one other approach that I could try and haven't attempted yet:
* Adding an extra parameter to "branch_get_upstream()" for new
behavior in these contexts

This might be easier to follow, even if it makes "branch_get_upstream()" itself more complex.

There's probably some clean refactor that I'm failing to see.
Show 20 quoted lines
>
> > +             }
> >       case PUSH_DEFAULT_UNSPECIFIED:
> >       case PUSH_DEFAULT_SIMPLE:
> >               {
> >                       const char *up, *cur;
> >
> > +                     up = default_missing_upstream(remote, branch, err);
> > +                     if (up)
> > +                             return up;
> > +
> >                       up = branch_get_upstream(branch, err);
> >                       if (!up)
> >                               return NULL;
>
> ... and here?  That is, if branch_get_upstream() finds an explicitly
> configured one, shouldn't we use that and fall back to the new
> "missing" code path only when there isn't an explicitly configured
> one?
>

Yep, same deal - by the time "branch_get_upstream()" has *not* found one, it's returned an error, and I haven't understood the side-effects of that in this project, if any.

Any advice on whether an already-prepared error can simply be discarded would be welcome!

Tao Klerks via GitGitGadget· May 28, 2023, 09:58 UTC · re: Tao Klerks via GitGitGadget · lore

[PATCH v2] rev-parse: respect push.autosetupremote when evaluating @{push}

From: Tao Klerks <tao@klerks.biz>

In a previous release, the push.autosetupremote config was introduced to ease new branch management in "simple" single-remote workflows. This makes "git push" work on new branches (without configured upstream) with push.default set to "simple" or "upstream" and it implies "--set-upstream" regardless of whether the same-name remote branch exists or not.

The "@{push}" suffix logic was not adjusted to account for this new option, however, and sometimes returns an error when "git push" would successfully push to an existing remote branch.

This is an edge-case, as the main context where push.autosetupremote will apply is for *new* branches, with no corresponding remote branch yet, and so even if the defaulting is handled correctly, the rev-parse will still fail with "unknown revision or path not in the working tree".

Fix this edge-case so "git rev-parse @{push}" works, if there is no upstream tracking relationship set up but the remote tracking branch that will be defaulted to does already exist and can be resolved.

Also add corresponding test cases.
Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    rev-parse: respect push.autosetupremote when evaluating @{push}
    
    Minor consistency fix for previously-introduced "push.autosetupremote"
    option.
    
    V2:
    
     * Rebased onto recent main, 10 months later.
    
    On the original thread, Junio expressed some concerns over the clarity
    of the code / the obviousness of the change, but I did not find any
    better approach.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1279%2FTaoK%2Ftao-rev-parse-autosetupremote-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1279/TaoK/tao-rev-parse-autosetupremote-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1279
Range-diff vs v1:
 1:  147e40ce932 ! 1:  d895e380b98 rev-parse: respect push.autosetupremote when evaluating @{push}
     @@ Commit message
      
          Fix this edge-case so "git rev-parse @{push}" works, if there is no
          upstream tracking relationship set up but the remote tracking branch that
     -    will be defaulted to does exist and can be resolved.
     +    will be defaulted to does already exist and can be resolved.
      
          Also add corresponding test cases.
      
 remote.c                  | 30 ++++++++++++++++++++++++++++--
 t/t1514-rev-parse-push.sh | 20 ++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)
Show changes to 2 files +48 −2

remote.c, t/t1514-rev-parse-push.sh

diff --git a/remote.c b/remote.c
index 0764fca0db9..07194c616da 100644
--- a/remote.c
+++ b/remote.c
@@ -1906,6 +1906,23 @@ static const char *tracking_for_push_dest(struct remote *remote,
 	return ret;
 }
 
+static const char *default_missing_upstream(struct remote *remote,
+				    struct branch *branch,
+				    struct strbuf *err)
+{
+	int autosetupremote = 0;
+
+	if (branch && (!branch->merge || !branch->merge[0])) {
+		repo_config_get_bool(the_repository,
+				     "push.autosetupremote",
+				     &autosetupremote);
+		if (autosetupremote)
+			return tracking_for_push_dest(remote, branch->refname, err);
+	}
+
+	return NULL;
+}
+
 static const char *branch_get_push_1(struct remote_state *remote_state,
 				     struct branch *branch, struct strbuf *err)
 {
@@ -1946,13 +1963,22 @@ static const char *branch_get_push_1(struct remote_state *remote_state,
 		return tracking_for_push_dest(remote, branch->refname, err);
 
 	case PUSH_DEFAULT_UPSTREAM:
-		return branch_get_upstream(branch, err);
-
+		{
+			const char *up;
+			up = default_missing_upstream(remote, branch, err);
+			if (up)
+				return up;
+			return branch_get_upstream(branch, err);
+		}
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
 		{
 			const char *up, *cur;
 
+			up = default_missing_upstream(remote, branch, err);
+			if (up)
+				return up;
+
 			up = branch_get_upstream(branch, err);
 			if (!up)
 				return NULL;
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index d868a081105..ffa0db14585 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -21,7 +21,10 @@ test_expect_success 'setup' '
 	git push origin HEAD &&
 	git branch --set-upstream-to=origin/main main &&
 	git branch --track topic origin/main &&
+	git branch --no-track indie_topic origin/main &&
+	git branch --no-track new_topic origin/main &&
 	git push origin topic &&
+	git push origin indie_topic &&
 	git push other topic
 '
 
@@ -73,4 +76,21 @@ test_expect_success 'resolving @{push} fails with a detached HEAD' '
 	test_must_fail git rev-parse @{push}
 '
 
+test_expect_success '@{push} with default=simple without tracking' '
+	test_config push.default simple &&
+	test_must_fail git rev-parse indie_topic@{push}
+'
+
+test_expect_success '@{push} with default=simple with autosetupremote' '
+	test_config push.default simple &&
+	test_config push.autosetupremote true &&
+	resolve indie_topic@{push} refs/remotes/origin/indie_topic
+'
+
+test_expect_success '@{push} with default=simple with autosetupremote, new branch' '
+	test_config push.default simple &&
+	test_config push.autosetupremote true &&
+	test_must_fail git rev-parse new_topic@{push}
+'
+
 test_done

base-commit: 4a714b37029a4b63dbd22f7d7ed81f7a0d693680
-- 
gitgitgadget

← back to recent threads