{"thread":{"id":"45497","subject":"[PATCH] push: allow atomic flag via configuration","startedAt":"2017-03-24T17:18:02Z","lastAt":"2017-03-28T00:53:47Z","messageCount":10,"participants":["Romuald Brunet","Jeff King","Jonathan Nieder","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"315277","messageId":"1490375874.745.227.camel@locke.gandi.net","threadId":"45497","inReplyTo":null,"subject":"[PATCH] push: allow atomic flag via configuration","fromName":"Romuald Brunet","fromEmail":"romuald@chivil.com","sentAt":"2017-03-24T17:17:54Z","receivedAt":"2017-03-24T17:18:02Z","isPatch":true,"sender":{"key":"romuald@chivil.com","avatar":null},"body":"Added a \"push.atomic\" option to git-config to allow site-specific\nconfiguration of the atomic flag of git push\n\nSigned-off-by: Romuald Brunet <romuald@chivil.com>\n---\n Documentation/config.txt               |  5 +++++\n builtin/push.c                         |  6 ++++++\n contrib/completion/git-completion.bash |  1 +\n t/t5543-atomic-push.sh                 | 31 +++++++++++++++++++++++++++++++\n 4 files changed, 43 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0d8df5a9f..c826c86ae 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2555,6 +2555,11 @@ new default).\n \n --\n \n+push.atomic::\n+\tIf set to true enable `--atomic` option by default.  You\n+\tmay override this configuration at time of push by specifying\n+\t`--no-atomic`.\n+\n push.followTags::\n \tIf set to true enable `--follow-tags` option by default.  You\n \tmay override this configuration at time of push by specifying\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 5c22e9f2e..03066f3f7 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -498,6 +498,12 @@ static int git_push_config(const char *k, const char *v, void *cb)\n \t\tconst char *value;\n \t\tif (!git_config_get_value(\"push.recursesubmodules\", &value))\n \t\t\trecurse_submodules = parse_push_recurse_submodules_arg(k, value);\n+\t} else if (!strcmp(k, \"push.atomic\")) {\n+\t\tif (git_config_bool(k, v))\n+\t\t\t*flags |= TRANSPORT_PUSH_ATOMIC;\n+\t\telse\n+\t\t\t*flags &= ~TRANSPORT_PUSH_ATOMIC;\n+\t\treturn 0;\n \t}\n \n \treturn git_default_config(k, v, NULL);\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex fc32286a4..8d38f5f8f 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2396,6 +2396,7 @@ _git_config ()\n \t\tpull.octopus\n \t\tpull.twohead\n \t\tpush.default\n+\t\tpush.atomic\n \t\tpush.followTags\n \t\trebase.autosquash\n \t\trebase.stat\ndiff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh\nindex 3480b3300..7c573b85b 100755\n--- a/t/t5543-atomic-push.sh\n+++ b/t/t5543-atomic-push.sh\n@@ -191,4 +191,35 @@ test_expect_success 'atomic push is not advertised if configured' '\n \ttest_refs master HEAD@{1}\n '\n \n+test_expect_success 'atomic option possible via git-config' '\n+\t# prepare the repo\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit checkout -b second master &&\n+\t\ttest_commit two &&\n+\t\tgit push --mirror up\n+\t) &&\n+\t# a third party modifies the server side:\n+\t(\n+\t\tcd upstream &&\n+\t\tgit checkout second &&\n+\t\tgit tag test_tag second\n+\t) &&\n+\t# see if we can now push both branches.\n+\t(\n+\t\tcd workbench &&\n+\t\tgit config push.atomic true &&\n+\t\tgit checkout master &&\n+\t\ttest_commit three &&\n+\t\tgit checkout second &&\n+\t\ttest_commit four &&\n+\t\tgit tag test_tag &&\n+\t\ttest_must_fail git push --tags up master second\n+\t) &&\n+\ttest_refs master HEAD@{3} &&\n+\ttest_refs second HEAD@{1}\n+'\n+\n test_done\n-- \n2.12.0.dirty\n\n\n\n"},{"id":"315319","messageId":"20170324184550.3l6mw4ybynizblsm@sigill.intra.peff.net","threadId":"45497","inReplyTo":"1490375874.745.227.camel@locke.gandi.net","subject":"Re: [PATCH] push: allow atomic flag via configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T18:45:51Z","receivedAt":"2017-03-24T18:45:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2017 at 06:17:54PM +0100, Romuald Brunet wrote:\n\n> Added a \"push.atomic\" option to git-config to allow site-specific\n> configuration of the atomic flag of git push\n\nI don't really use --atomic myself, but this seems like a reasonable\nthing to want, and the implementation looks cleanly done.\n\nMy one question would be whether people would want this to actually be\nspecific to a particular remote, and not just on for a given repository\n(your \"site-specific\" in the description made me think of that). In that\ncase it would be better as part of the remote.* config.\n\n-Peff\n"},{"id":"315320","messageId":"20170324185354.GE31294@aiede.mtv.corp.google.com","threadId":"45497","inReplyTo":"20170324184550.3l6mw4ybynizblsm@sigill.intra.peff.net","subject":"Re: [PATCH] push: allow atomic flag via configuration","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-24T18:53:54Z","receivedAt":"2017-03-24T18:54:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Fri, Mar 24, 2017 at 06:17:54PM +0100, Romuald Brunet wrote:\n\n>> Added a \"push.atomic\" option to git-config to allow site-specific\n>> configuration of the atomic flag of git push\n>\n> I don't really use --atomic myself, but this seems like a reasonable\n> thing to want, and the implementation looks cleanly done.\n>\n> My one question would be whether people would want this to actually be\n> specific to a particular remote, and not just on for a given repository\n> (your \"site-specific\" in the description made me think of that). In that\n> case it would be better as part of the remote.* config.\n\nI didn't receive the original patch (maybe mailing delay?) so\ncommenting here.\n\nI use --atomic sometimes, but it's tied to the specific context of\nwhat I'm trying to push.\n\nI'd be interested to hear more about the use case.  Do you want pushes\nto be atomic opportunistically when the server supports that\n(analogous to push.gpgSign=if-asked)?\n\nThis also seems likely to break scripts that rely on the current\nnon-atomic behavior, so without knowing more about the use case it's\nhard to see how to proceed.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"315323","messageId":"20170324190123.b3ln56x5s6uztvil@sigill.intra.peff.net","threadId":"45497","inReplyTo":"20170324185354.GE31294@aiede.mtv.corp.google.com","subject":"Re: [PATCH] push: allow atomic flag via configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T19:01:23Z","receivedAt":"2017-03-24T19:01:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2017 at 11:53:54AM -0700, Jonathan Nieder wrote:\n\n> I didn't receive the original patch (maybe mailing delay?) so\n> commenting here.\n\nVger seems a bit slow lately. The list copy did eventually get delivered\nto me and public-inbox:\n\n  http://public-inbox.org/git/1490375874.745.227.camel@locke.gandi.net/\n\nI don't think it answers any of your questions, though. ;)\n\n-Peff\n"},{"id":"315325","messageId":"xmqqmvca79sh.fsf@gitster.mtv.corp.google.com","threadId":"45497","inReplyTo":"20170324184550.3l6mw4ybynizblsm@sigill.intra.peff.net","subject":"Re: [PATCH] push: allow atomic flag via configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-24T19:29:50Z","receivedAt":"2017-03-24T19:31:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> My one question would be whether people would want this to actually be\n> specific to a particular remote, and not just on for a given repository\n> (your \"site-specific\" in the description made me think of that). In that\n> case it would be better as part of the remote.* config.\n\nYeah, I had the same reaction.  \n\nConceptually, this sits next to remote.*.push that defines which set\nof refs are sent by default, and remote.<name>.pushAtomic does make\nsense.  If (and only if) it turns out to be cumbersome for somebody\nto set the configuration for each and every remote, it is OK to also\nadd push.atomic to serve as a fallback for remote.*.pushAtomic, I\nwould think, but adding only push.atomic feels somewhat backwards.\n\n\n\n"},{"id":"315508","messageId":"1490603272.9788.7.camel@locke.gandi.net","threadId":"45497","inReplyTo":"xmqqmvca79sh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] push: allow atomic flag via configuration","fromName":"Romuald Brunet","fromEmail":"romuald@chivil.com","sentAt":"2017-03-27T08:27:52Z","receivedAt":"2017-03-27T08:39:10Z","isPatch":true,"sender":{"key":"romuald@chivil.com","avatar":null},"body":"On ven., 2017-03-24 at 12:29 -0700, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > My one question would be whether people would want this to actually be\n> > specific to a particular remote, and not just on for a given repository\n> > (your \"site-specific\" in the description made me think of that). In that\n> > case it would be better as part of the remote.* config.\n> \n> Yeah, I had the same reaction.  \n> \n> Conceptually, this sits next to remote.*.push that defines which set\n> of refs are sent by default, and remote.<name>.pushAtomic does make\n> sense.  If (and only if) it turns out to be cumbersome for somebody\n> to set the configuration for each and every remote, it is OK to also\n> add push.atomic to serve as a fallback for remote.*.pushAtomic, I\n> would think, but adding only push.atomic feels somewhat backwards.\n\nThanks for your feedback\n\nI'm mostly using single remotes that's why I didn't even think of making\nit configurable per remote. But you're right that makes more sense.\n\nI'll try to make that modification to the patch.\n\nAs for my use case: I'd like to use default atomic pushes when pushing a\nnew tag among our stable branch, but inevitably forgetting to rebase\nbeforehand. Therefore pushing a \"dangling\" commit/tag\n\n\n-- \nRomuald Brunet\n\n"},{"id":"315509","messageId":"CACBZZX5buDHBy+aHWYVugdg=jyPgHOmgjuRx6NQTQ67k4_WU5w@mail.gmail.com","threadId":"45497","inReplyTo":"1490375874.745.227.camel@locke.gandi.net","subject":"Re: [PATCH] push: allow atomic flag via configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-27T09:30:21Z","receivedAt":"2017-03-27T09:31:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Mar 24, 2017 at 6:17 PM, Romuald Brunet <romuald@chivil.com> wrote:\n> +push.atomic::\n> +       If set to true enable `--atomic` option by default.  You\n> +       may override this configuration at time of push by specifying\n> +       `--no-atomic`.\n> +\n\nThis should also be mentioned in the --atomic documentation in\ngit-push.txt itself. See e.g. the documentation for pull --rebase for\nan example of this.\n"},{"id":"315510","messageId":"CACBZZX7HAg89htqPgkFd+ks-E7fcgAWfQ9n9qRj-YM88tj3t-A@mail.gmail.com","threadId":"45497","inReplyTo":"1490375874.745.227.camel@locke.gandi.net","subject":"Re: [PATCH] push: allow atomic flag via configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-27T09:33:05Z","receivedAt":"2017-03-27T09:35:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Mar 24, 2017 at 6:17 PM, Romuald Brunet <romuald@chivil.com> wrote:\n> +test_expect_success 'atomic option possible via git-config' '\n> +       # prepare the repo\n> +       mk_repo_pair &&\n> +       (\n> +               cd workbench &&\n> +               test_commit one &&\n> +               git checkout -b second master &&\n> +               test_commit two &&\n> +               git push --mirror up\n> +       ) &&\n> +       # a third party modifies the server side:\n> +       (\n> +               cd upstream &&\n> +               git checkout second &&\n> +               git tag test_tag second\n> +       ) &&\n> +       # see if we can now push both branches.\n> +       (\n> +               cd workbench &&\n> +               git config push.atomic true &&\n> +               git checkout master &&\n> +               test_commit three &&\n> +               git checkout second &&\n> +               test_commit four &&\n> +               git tag test_tag &&\n> +               test_must_fail git push --tags up master second\n> +       ) &&\n> +       test_refs master HEAD@{3} &&\n> +       test_refs second HEAD@{1}\n> +'\n> +\n\nSent my earlier E-Mail too soon, so I missed this, there's no test\nhere for what \"git config push.atomic false\" && \"git push --atomic\"\ndoes, is that atomic or not? I.e. what does \"git -c push.atomic=false\npush --atomic\" do? Does the CLI option override the config as it\nshould?\n"},{"id":"315518","messageId":"xmqq7f3a3bbg.fsf@gitster.mtv.corp.google.com","threadId":"45497","inReplyTo":"CACBZZX7HAg89htqPgkFd+ks-E7fcgAWfQ9n9qRj-YM88tj3t-A@mail.gmail.com","subject":"Re: [PATCH] push: allow atomic flag via configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-27T16:59:31Z","receivedAt":"2017-03-27T17:00:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Fri, Mar 24, 2017 at 6:17 PM, Romuald Brunet <romuald@chivil.com> wrote:\n>> +test_expect_success 'atomic option possible via git-config' '\n>> +       # prepare the repo\n>> +       mk_repo_pair &&\n>> +       (\n>> +               cd workbench &&\n>> +               test_commit one &&\n>> +               git checkout -b second master &&\n>> +               test_commit two &&\n>> +               git push --mirror up\n>> +       ) &&\n>> +       # a third party modifies the server side:\n>> +       (\n>> +               cd upstream &&\n>> +               git checkout second &&\n>> +               git tag test_tag second\n>> +       ) &&\n>> +       # see if we can now push both branches.\n>> +       (\n>> +               cd workbench &&\n>> +               git config push.atomic true &&\n>> +               git checkout master &&\n>> +               test_commit three &&\n>> +               git checkout second &&\n>> +               test_commit four &&\n>> +               git tag test_tag &&\n>> +               test_must_fail git push --tags up master second\n>> +       ) &&\n>> +       test_refs master HEAD@{3} &&\n>> +       test_refs second HEAD@{1}\n>> +'\n>> +\n>\n> Sent my earlier E-Mail too soon, so I missed this, there's no test\n> here for what \"git config push.atomic false\" && \"git push --atomic\"\n> does, is that atomic or not? I.e. what does \"git -c push.atomic=false\n> push --atomic\" do? Does the CLI option override the config as it\n> should?\n\nGood points.  Thanks for reading and reviewing the tests carefully.\n\n"},{"id":"315548","messageId":"20170328005126.GP31294@aiede.mtv.corp.google.com","threadId":"45497","inReplyTo":"1490603272.9788.7.camel@locke.gandi.net","subject":"Re: [PATCH] push: allow atomic flag via configuration","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-28T00:51:26Z","receivedAt":"2017-03-28T00:53:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRomuald Brunet wrote:\n> On ven., 2017-03-24 at 12:29 -0700, Junio C Hamano wrote:\n>> Jeff King <peff@peff.net> writes:\n\n>>> My one question would be whether people would want this to actually be\n>>> specific to a particular remote, and not just on for a given repository\n>>> (your \"site-specific\" in the description made me think of that). In that\n>>> case it would be better as part of the remote.* config.\n>>\n>> Yeah, I had the same reaction.\n>>\n>> Conceptually, this sits next to remote.*.push that defines which set\n>> of refs are sent by default, and remote.<name>.pushAtomic does make\n>> sense.  If (and only if) it turns out to be cumbersome for somebody\n>> to set the configuration for each and every remote, it is OK to also\n>> add push.atomic to serve as a fallback for remote.*.pushAtomic, I\n>> would think, but adding only push.atomic feels somewhat backwards.\n>\n> Thanks for your feedback\n>\n> I'm mostly using single remotes that's why I didn't even think of making\n> it configurable per remote. But you're right that makes more sense.\n>\n> I'll try to make that modification to the patch.\n>\n> As for my use case: I'd like to use default atomic pushes when pushing a\n> new tag among our stable branch, but inevitably forgetting to rebase\n> beforehand. Therefore pushing a \"dangling\" commit/tag\n\nMaking it per-remote would also address my concern about scripts.\nExisting scripts may be using\n\n\tgit push $url $refs\n\nand relying on the push to partially succeed even when some refs\ncannot be fast-forwarded.  (For example, I had such a script that did\n\n\tgit push my-mirror refs/remotes/origin/*:refs/heads/*\n\tgit push my-mirror +origin/pu:pu\n\nbecause the first command would fail to update pu, and then the second\ncommand would clean up after it.)  A configuration that globally\nchanges the effect of \"git push\" to mean \"git push --atomic\" would\nbreak such scripts.  A per-remote configuration is tightly enough\nscoped to not be likely to cause unrelated breakage for users that run\nscripts written by others.\n\nThanks and hope that helps,\nJonathan\n"}]}