{"thread":{"id":"58128","subject":"[PATCH] rev-parse: respect push.autosetupremote when evaluating @{push}","startedAt":"2022-07-10T19:16:42Z","lastAt":"2023-05-28T09:58:51Z","messageCount":4,"participants":["Tao Klerks via GitGitGadget","Junio C Hamano","Tao Klerks"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"458725","messageId":"pull.1279.git.1657480594123.gitgitgadget@gmail.com","threadId":"58128","inReplyTo":null,"subject":"[PATCH] rev-parse: respect push.autosetupremote when evaluating @{push}","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-10T19:16:33Z","receivedAt":"2022-07-10T19:16:42Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nIn a previous release, the push.autosetupremote config was introduced to\nease new branch management in \"simple\" single-remote workflows. This makes\n\"git push\" work on new branches (without configured upstream) with\npush.default set to \"simple\" or \"upstream\" and it implies\n\"--set-upstream\" regardless of whether the same-name remote branch exists\nor not.\n\nThe \"@{push}\" suffix logic was not adjusted to account for this new option,\nhowever, and sometimes returns an error when \"git push\" would successfully\npush to an existing remote branch.\n\nThis is an edge-case, as the main context where push.autosetupremote will\napply is for *new* branches, with no corresponding remote branch yet, and\nso even if the defaulting is handled correctly, the rev-parse will still\nfail with \"unknown revision or path not in the working tree\".\n\nFix this edge-case so \"git rev-parse @{push}\" works, if there is no\nupstream tracking relationship set up but the remote tracking branch that\nwill be defaulted to does exist and can be resolved.\n\nAlso add corresponding test cases.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    rev-parse: respect push.autosetupremote when evaluating @{push}\n    \n    Minor consistency fix for previously-introduced \"push.autosetupremote\"\n    option.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1279%2FTaoK%2Ftao-rev-parse-autosetupremote-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1279/TaoK/tao-rev-parse-autosetupremote-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1279\n\n remote.c                  | 30 ++++++++++++++++++++++++++++--\n t/t1514-rev-parse-push.sh | 20 ++++++++++++++++++++\n 2 files changed, 48 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex b19e3a2f015..59f5bf5b5f5 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1919,6 +1919,23 @@ static const char *tracking_for_push_dest(struct remote *remote,\n \treturn ret;\n }\n \n+static const char *default_missing_upstream(struct remote *remote,\n+\t\t\t\t    struct branch *branch,\n+\t\t\t\t    struct strbuf *err)\n+{\n+\tint autosetupremote = 0;\n+\n+\tif (branch && (!branch->merge || !branch->merge[0])) {\n+\t\trepo_config_get_bool(the_repository,\n+\t\t\t\t     \"push.autosetupremote\",\n+\t\t\t\t     &autosetupremote);\n+\t\tif (autosetupremote)\n+\t\t\treturn tracking_for_push_dest(remote, branch->refname, err);\n+\t}\n+\n+\treturn NULL;\n+}\n+\n static const char *branch_get_push_1(struct remote_state *remote_state,\n \t\t\t\t     struct branch *branch, struct strbuf *err)\n {\n@@ -1959,13 +1976,22 @@ static const char *branch_get_push_1(struct remote_state *remote_state,\n \t\treturn tracking_for_push_dest(remote, branch->refname, err);\n \n \tcase PUSH_DEFAULT_UPSTREAM:\n-\t\treturn branch_get_upstream(branch, err);\n-\n+\t\t{\n+\t\t\tconst char *up;\n+\t\t\tup = default_missing_upstream(remote, branch, err);\n+\t\t\tif (up)\n+\t\t\t\treturn up;\n+\t\t\treturn branch_get_upstream(branch, err);\n+\t\t}\n \tcase PUSH_DEFAULT_UNSPECIFIED:\n \tcase PUSH_DEFAULT_SIMPLE:\n \t\t{\n \t\t\tconst char *up, *cur;\n \n+\t\t\tup = default_missing_upstream(remote, branch, err);\n+\t\t\tif (up)\n+\t\t\t\treturn up;\n+\n \t\t\tup = branch_get_upstream(branch, err);\n \t\t\tif (!up)\n \t\t\t\treturn NULL;\ndiff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh\nindex d868a081105..ffa0db14585 100755\n--- a/t/t1514-rev-parse-push.sh\n+++ b/t/t1514-rev-parse-push.sh\n@@ -21,7 +21,10 @@ test_expect_success 'setup' '\n \tgit push origin HEAD &&\n \tgit branch --set-upstream-to=origin/main main &&\n \tgit branch --track topic origin/main &&\n+\tgit branch --no-track indie_topic origin/main &&\n+\tgit branch --no-track new_topic origin/main &&\n \tgit push origin topic &&\n+\tgit push origin indie_topic &&\n \tgit push other topic\n '\n \n@@ -73,4 +76,21 @@ test_expect_success 'resolving @{push} fails with a detached HEAD' '\n \ttest_must_fail git rev-parse @{push}\n '\n \n+test_expect_success '@{push} with default=simple without tracking' '\n+\ttest_config push.default simple &&\n+\ttest_must_fail git rev-parse indie_topic@{push}\n+'\n+\n+test_expect_success '@{push} with default=simple with autosetupremote' '\n+\ttest_config push.default simple &&\n+\ttest_config push.autosetupremote true &&\n+\tresolve indie_topic@{push} refs/remotes/origin/indie_topic\n+'\n+\n+test_expect_success '@{push} with default=simple with autosetupremote, new branch' '\n+\ttest_config push.default simple &&\n+\ttest_config push.autosetupremote true &&\n+\ttest_must_fail git rev-parse new_topic@{push}\n+'\n+\n test_done\n\nbase-commit: 39c15e485575089eb77c769f6da02f98a55905e0\n-- \ngitgitgadget\n"},{"id":"458729","messageId":"xmqq5yk4r96l.fsf@gitster.g","threadId":"58128","inReplyTo":"pull.1279.git.1657480594123.gitgitgadget@gmail.com","subject":"Re: [PATCH] rev-parse: respect push.autosetupremote when evaluating @{push}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-10T20:42:42Z","receivedAt":"2022-07-10T20:42:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> +\tif (branch && (!branch->merge || !branch->merge[0])) {\n> +\t\trepo_config_get_bool(the_repository,\n> +\t\t\t\t     \"push.autosetupremote\",\n> +\t\t\t\t     &autosetupremote);\n> +\t\tif (autosetupremote)\n> +\t\t\treturn tracking_for_push_dest(remote, branch->refname, err);\n\nBefore the first push of the branch X where we are asking for\nX@{push}, i.e. there is not the corresponding branch over there yet\nand we do not have the remote-tracking branch for it yet, what does\nthis function return?  If it continues to error out, then I think\nthis patch may make sense, but ...\n\n> +\t\t{\n> +\t\t\tconst char *up;\n> +\t\t\tup = default_missing_upstream(remote, branch, err);\n> +\t\t\tif (up)\n> +\t\t\t\treturn up;\n> +\t\t\treturn branch_get_upstream(branch, err);\n\n... shouldn't the precedence order the other way around here ...\n\n> +\t\t}\n>  \tcase PUSH_DEFAULT_UNSPECIFIED:\n>  \tcase PUSH_DEFAULT_SIMPLE:\n>  \t\t{\n>  \t\t\tconst char *up, *cur;\n>  \n> +\t\t\tup = default_missing_upstream(remote, branch, err);\n> +\t\t\tif (up)\n> +\t\t\t\treturn up;\n> +\n>  \t\t\tup = branch_get_upstream(branch, err);\n>  \t\t\tif (!up)\n>  \t\t\t\treturn NULL;\n\n... and here?  That is, if branch_get_upstream() finds an explicitly\nconfigured one, shouldn't we use that and fall back to the new\n\"missing\" code path only when there isn't an explicitly configured\none?\n\n"},{"id":"459207","messageId":"CAPMMpoirWDBJn45dkWo8CRDU23G1mnyPDHcKq1eH1AtACEW9Rg@mail.gmail.com","threadId":"58128","inReplyTo":"xmqq5yk4r96l.fsf@gitster.g","subject":"Re: [PATCH] rev-parse: respect push.autosetupremote when evaluating @{push}","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-07-16T17:40:21Z","receivedAt":"2022-07-16T17:40:37Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Sun, Jul 10, 2022 at 10:42 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\nThanks so much for looking at this, my apologies for the delayed response.\n\n>\n> > +     if (branch && (!branch->merge || !branch->merge[0])) {\n> > +             repo_config_get_bool(the_repository,\n> > +                                  \"push.autosetupremote\",\n> > +                                  &autosetupremote);\n> > +             if (autosetupremote)\n> > +                     return tracking_for_push_dest(remote, branch->refname, err);\n>\n> Before the first push of the branch X where we are asking for\n> X@{push}, i.e. there is not the corresponding branch over there yet\n> and we do not have the remote-tracking branch for it yet, what does\n> this function return?  If it continues to error out, then I think\n> this patch may make sense, but ...\n\nIt does indeed still error out, because even though \"git push\" may\nsucceed (eg if \"push.autosetupremote=true\"), there simply isn't a\nremote-tracking branch for it yet.\n\nThe only \"point\" of the patch is to address the edge-case where the\nremote-tracking branch *does* exist, and git push *would* push to it,\non the basis of the new \"push.autosetupremote=true\" behavior.\n\nIn that very specific case, it's \"wrong\" to return an error just\nbecause the tracking relationship doesn't exist yet.\n\n>\n> > +             {\n> > +                     const char *up;\n> > +                     up = default_missing_upstream(remote, branch, err);\n> > +                     if (up)\n> > +                             return up;\n> > +                     return branch_get_upstream(branch, err);\n>\n> ... shouldn't the precedence order the other way around here ...\n\nThis is a bit convoluted, but I don't see how to make it more obvious.\n\nThe new \"default_missing_upstream\" function checks the *same\nconditions* - it only \"kicks in\" if \"branch_get_upstream()\" would\nreturn an error.\n\nThe reasons I can't add the new logic *after* or *in*\n\"branch_get_upstream()\", and avoid this non-obviousness, are that:\n* \"branch_get_upstream()\" raises an error, so recovering afterwards\nseems non-trivial (to me at least - I haven't got my head around the\n\"idiom\" yet)\n* \"branch_get_upstream()\" is used in a a few other places for other\npurposes, so I'm not comfortable modifying it for my purposes\n* \"branch_get_upstream()\" is non-trivial, so I'm not comfortable\nduplicating it / creating a new variant\n\nI think I see one other approach that I could try and haven't attempted yet:\n* Adding an extra parameter to \"branch_get_upstream()\" for new\nbehavior in these contexts\n\nThis might be easier to follow, even if it makes\n\"branch_get_upstream()\" itself more complex.\n\nThere's probably some clean refactor that I'm failing to see.\n\n>\n> > +             }\n> >       case PUSH_DEFAULT_UNSPECIFIED:\n> >       case PUSH_DEFAULT_SIMPLE:\n> >               {\n> >                       const char *up, *cur;\n> >\n> > +                     up = default_missing_upstream(remote, branch, err);\n> > +                     if (up)\n> > +                             return up;\n> > +\n> >                       up = branch_get_upstream(branch, err);\n> >                       if (!up)\n> >                               return NULL;\n>\n> ... and here?  That is, if branch_get_upstream() finds an explicitly\n> configured one, shouldn't we use that and fall back to the new\n> \"missing\" code path only when there isn't an explicitly configured\n> one?\n>\n\nYep, same deal - by the time \"branch_get_upstream()\" has *not* found\none, it's returned an error, and I haven't understood the side-effects\nof that in this project, if any.\n\nAny advice on whether an already-prepared error can simply be\ndiscarded would be welcome!\n"},{"id":"477764","messageId":"pull.1279.v2.git.1685267922901.gitgitgadget@gmail.com","threadId":"58128","inReplyTo":"pull.1279.git.1657480594123.gitgitgadget@gmail.com","subject":"[PATCH v2] rev-parse: respect push.autosetupremote when evaluating @{push}","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-28T09:58:42Z","receivedAt":"2023-05-28T09:58:51Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nIn a previous release, the push.autosetupremote config was introduced to\nease new branch management in \"simple\" single-remote workflows. This makes\n\"git push\" work on new branches (without configured upstream) with\npush.default set to \"simple\" or \"upstream\" and it implies\n\"--set-upstream\" regardless of whether the same-name remote branch exists\nor not.\n\nThe \"@{push}\" suffix logic was not adjusted to account for this new option,\nhowever, and sometimes returns an error when \"git push\" would successfully\npush to an existing remote branch.\n\nThis is an edge-case, as the main context where push.autosetupremote will\napply is for *new* branches, with no corresponding remote branch yet, and\nso even if the defaulting is handled correctly, the rev-parse will still\nfail with \"unknown revision or path not in the working tree\".\n\nFix this edge-case so \"git rev-parse @{push}\" works, if there is no\nupstream tracking relationship set up but the remote tracking branch that\nwill be defaulted to does already exist and can be resolved.\n\nAlso add corresponding test cases.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    rev-parse: respect push.autosetupremote when evaluating @{push}\n    \n    Minor consistency fix for previously-introduced \"push.autosetupremote\"\n    option.\n    \n    V2:\n    \n     * Rebased onto recent main, 10 months later.\n    \n    On the original thread, Junio expressed some concerns over the clarity\n    of the code / the obviousness of the change, but I did not find any\n    better approach.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1279%2FTaoK%2Ftao-rev-parse-autosetupremote-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1279/TaoK/tao-rev-parse-autosetupremote-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1279\n\nRange-diff vs v1:\n\n 1:  147e40ce932 ! 1:  d895e380b98 rev-parse: respect push.autosetupremote when evaluating @{push}\n     @@ Commit message\n      \n          Fix this edge-case so \"git rev-parse @{push}\" works, if there is no\n          upstream tracking relationship set up but the remote tracking branch that\n     -    will be defaulted to does exist and can be resolved.\n     +    will be defaulted to does already exist and can be resolved.\n      \n          Also add corresponding test cases.\n      \n\n\n remote.c                  | 30 ++++++++++++++++++++++++++++--\n t/t1514-rev-parse-push.sh | 20 ++++++++++++++++++++\n 2 files changed, 48 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 0764fca0db9..07194c616da 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1906,6 +1906,23 @@ static const char *tracking_for_push_dest(struct remote *remote,\n \treturn ret;\n }\n \n+static const char *default_missing_upstream(struct remote *remote,\n+\t\t\t\t    struct branch *branch,\n+\t\t\t\t    struct strbuf *err)\n+{\n+\tint autosetupremote = 0;\n+\n+\tif (branch && (!branch->merge || !branch->merge[0])) {\n+\t\trepo_config_get_bool(the_repository,\n+\t\t\t\t     \"push.autosetupremote\",\n+\t\t\t\t     &autosetupremote);\n+\t\tif (autosetupremote)\n+\t\t\treturn tracking_for_push_dest(remote, branch->refname, err);\n+\t}\n+\n+\treturn NULL;\n+}\n+\n static const char *branch_get_push_1(struct remote_state *remote_state,\n \t\t\t\t     struct branch *branch, struct strbuf *err)\n {\n@@ -1946,13 +1963,22 @@ static const char *branch_get_push_1(struct remote_state *remote_state,\n \t\treturn tracking_for_push_dest(remote, branch->refname, err);\n \n \tcase PUSH_DEFAULT_UPSTREAM:\n-\t\treturn branch_get_upstream(branch, err);\n-\n+\t\t{\n+\t\t\tconst char *up;\n+\t\t\tup = default_missing_upstream(remote, branch, err);\n+\t\t\tif (up)\n+\t\t\t\treturn up;\n+\t\t\treturn branch_get_upstream(branch, err);\n+\t\t}\n \tcase PUSH_DEFAULT_UNSPECIFIED:\n \tcase PUSH_DEFAULT_SIMPLE:\n \t\t{\n \t\t\tconst char *up, *cur;\n \n+\t\t\tup = default_missing_upstream(remote, branch, err);\n+\t\t\tif (up)\n+\t\t\t\treturn up;\n+\n \t\t\tup = branch_get_upstream(branch, err);\n \t\t\tif (!up)\n \t\t\t\treturn NULL;\ndiff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh\nindex d868a081105..ffa0db14585 100755\n--- a/t/t1514-rev-parse-push.sh\n+++ b/t/t1514-rev-parse-push.sh\n@@ -21,7 +21,10 @@ test_expect_success 'setup' '\n \tgit push origin HEAD &&\n \tgit branch --set-upstream-to=origin/main main &&\n \tgit branch --track topic origin/main &&\n+\tgit branch --no-track indie_topic origin/main &&\n+\tgit branch --no-track new_topic origin/main &&\n \tgit push origin topic &&\n+\tgit push origin indie_topic &&\n \tgit push other topic\n '\n \n@@ -73,4 +76,21 @@ test_expect_success 'resolving @{push} fails with a detached HEAD' '\n \ttest_must_fail git rev-parse @{push}\n '\n \n+test_expect_success '@{push} with default=simple without tracking' '\n+\ttest_config push.default simple &&\n+\ttest_must_fail git rev-parse indie_topic@{push}\n+'\n+\n+test_expect_success '@{push} with default=simple with autosetupremote' '\n+\ttest_config push.default simple &&\n+\ttest_config push.autosetupremote true &&\n+\tresolve indie_topic@{push} refs/remotes/origin/indie_topic\n+'\n+\n+test_expect_success '@{push} with default=simple with autosetupremote, new branch' '\n+\ttest_config push.default simple &&\n+\ttest_config push.autosetupremote true &&\n+\ttest_must_fail git rev-parse new_topic@{push}\n+'\n+\n test_done\n\nbase-commit: 4a714b37029a4b63dbd22f7d7ed81f7a0d693680\n-- \ngitgitgadget\n"}]}