[PATCH] push: allow atomic flag via configuration

[PATCH] push: allow atomic flag via configuration

[Romuald Brunet]

Added a "push.atomic" option to git-config to allow site-specific
configuration of the atomic flag of git push

Signed-off-by: Romuald Brunet <romuald@chivil.com>
---
 Documentation/config.txt               |  5 +++++
 builtin/push.c                         |  6 ++++++
 contrib/completion/git-completion.bash |  1 +
 t/t5543-atomic-push.sh                 | 31 +++++++++++++++++++++++++++++++
 4 files changed, 43 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0d8df5a9f..c826c86ae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2555,6 +2555,11 @@ new default).
 
 --
 
+push.atomic::
+	If set to true enable `--atomic` option by default.  You
+	may override this configuration at time of push by specifying
+	`--no-atomic`.
+
 push.followTags::
 	If set to true enable `--follow-tags` option by default.  You
 	may override this configuration at time of push by specifying
diff --git a/builtin/push.c b/builtin/push.c
index 5c22e9f2e..03066f3f7 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -498,6 +498,12 @@ static int git_push_config(const char *k, const char *v, void *cb)
 		const char *value;
 		if (!git_config_get_value("push.recursesubmodules", &value))
 			recurse_submodules = parse_push_recurse_submodules_arg(k, value);
+	} else if (!strcmp(k, "push.atomic")) {
+		if (git_config_bool(k, v))
+			*flags |= TRANSPORT_PUSH_ATOMIC;
+		else
+			*flags &= ~TRANSPORT_PUSH_ATOMIC;
+		return 0;
 	}
 
 	return git_default_config(k, v, NULL);
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index fc32286a4..8d38f5f8f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2396,6 +2396,7 @@ _git_config ()
 		pull.octopus
 		pull.twohead
 		push.default
+		push.atomic
 		push.followTags
 		rebase.autosquash
 		rebase.stat
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 3480b3300..7c573b85b 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -191,4 +191,35 @@ test_expect_success 'atomic push is not advertised if configured' '
 	test_refs master HEAD@{1}
 '
 
+test_expect_success 'atomic option possible via git-config' '
+	# prepare the repo
+	mk_repo_pair &&
+	(
+		cd workbench &&
+		test_commit one &&
+		git checkout -b second master &&
+		test_commit two &&
+		git push --mirror up
+	) &&
+	# a third party modifies the server side:
+	(
+		cd upstream &&
+		git checkout second &&
+		git tag test_tag second
+	) &&
+	# see if we can now push both branches.
+	(
+		cd workbench &&
+		git config push.atomic true &&
+		git checkout master &&
+		test_commit three &&
+		git checkout second &&
+		test_commit four &&
+		git tag test_tag &&
+		test_must_fail git push --tags up master second
+	) &&
+	test_refs master HEAD@{3} &&
+	test_refs second HEAD@{1}
+'
+
 test_done
-- 
2.12.0.dirty

Re: [PATCH] push: allow atomic flag via configuration

[Jeff King]

On Fri, Mar 24, 2017 at 06:17:54PM +0100, Romuald Brunet wrote:

> Added a "push.atomic" option to git-config to allow site-specific
> configuration of the atomic flag of git push

I don't really use --atomic myself, but this seems like a reasonable
thing to want, and the implementation looks cleanly done.

My one question would be whether people would want this to actually be
specific to a particular remote, and not just on for a given repository
(your "site-specific" in the description made me think of that). In that
case it would be better as part of the remote.* config.

-Peff

Re: [PATCH] push: allow atomic flag via configuration

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Fri, Mar 24, 2017 at 06:17:54PM +0100, Romuald Brunet wrote:

>> Added a "push.atomic" option to git-config to allow site-specific
>> configuration of the atomic flag of git push
>
> I don't really use --atomic myself, but this seems like a reasonable
> thing to want, and the implementation looks cleanly done.
>
> My one question would be whether people would want this to actually be
> specific to a particular remote, and not just on for a given repository
> (your "site-specific" in the description made me think of that). In that
> case it would be better as part of the remote.* config.

I didn't receive the original patch (maybe mailing delay?) so
commenting here.

I use --atomic sometimes, but it's tied to the specific context of
what I'm trying to push.

I'd be interested to hear more about the use case.  Do you want pushes
to be atomic opportunistically when the server supports that
(analogous to push.gpgSign=if-asked)?

This also seems likely to break scripts that rely on the current
non-atomic behavior, so without knowing more about the use case it's
hard to see how to proceed.

Thanks and hope that helps,
Jonathan

Re: [PATCH] push: allow atomic flag via configuration

[Jeff King]

On Fri, Mar 24, 2017 at 11:53:54AM -0700, Jonathan Nieder wrote:

> I didn't receive the original patch (maybe mailing delay?) so
> commenting here.

Vger seems a bit slow lately. The list copy did eventually get delivered
to me and public-inbox:

  http://public-inbox.org/git/1490375874.745.227.camel@locke.gandi.net/

I don't think it answers any of your questions, though. ;)

-Peff

Re: [PATCH] push: allow atomic flag via configuration

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> My one question would be whether people would want this to actually be
> specific to a particular remote, and not just on for a given repository
> (your "site-specific" in the description made me think of that). In that
> case it would be better as part of the remote.* config.

Yeah, I had the same reaction.  

Conceptually, this sits next to remote.*.push that defines which set
of refs are sent by default, and remote.<name>.pushAtomic does make
sense.  If (and only if) it turns out to be cumbersome for somebody
to set the configuration for each and every remote, it is OK to also
add push.atomic to serve as a fallback for remote.*.pushAtomic, I
would think, but adding only push.atomic feels somewhat backwards.

Re: [PATCH] push: allow atomic flag via configuration

[Romuald Brunet]

On ven., 2017-03-24 at 12:29 -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > My one question would be whether people would want this to actually be
> > specific to a particular remote, and not just on for a given repository
> > (your "site-specific" in the description made me think of that). In that
> > case it would be better as part of the remote.* config.
> 
> Yeah, I had the same reaction.  
> 
> Conceptually, this sits next to remote.*.push that defines which set
> of refs are sent by default, and remote.<name>.pushAtomic does make
> sense.  If (and only if) it turns out to be cumbersome for somebody
> to set the configuration for each and every remote, it is OK to also
> add push.atomic to serve as a fallback for remote.*.pushAtomic, I
> would think, but adding only push.atomic feels somewhat backwards.

Thanks for your feedback

I'm mostly using single remotes that's why I didn't even think of making
it configurable per remote. But you're right that makes more sense.

I'll try to make that modification to the patch.

As for my use case: I'd like to use default atomic pushes when pushing a
new tag among our stable branch, but inevitably forgetting to rebase
beforehand. Therefore pushing a "dangling" commit/tag


-- 
Romuald Brunet

Re: [PATCH] push: allow atomic flag via configuration

[Jonathan Nieder]

Hi,

Romuald Brunet wrote:
> On ven., 2017-03-24 at 12:29 -0700, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:

>>> My one question would be whether people would want this to actually be
>>> specific to a particular remote, and not just on for a given repository
>>> (your "site-specific" in the description made me think of that). In that
>>> case it would be better as part of the remote.* config.
>>
>> Yeah, I had the same reaction.
>>
>> Conceptually, this sits next to remote.*.push that defines which set
>> of refs are sent by default, and remote.<name>.pushAtomic does make
>> sense.  If (and only if) it turns out to be cumbersome for somebody
>> to set the configuration for each and every remote, it is OK to also
>> add push.atomic to serve as a fallback for remote.*.pushAtomic, I
>> would think, but adding only push.atomic feels somewhat backwards.
>
> Thanks for your feedback
>
> I'm mostly using single remotes that's why I didn't even think of making
> it configurable per remote. But you're right that makes more sense.
>
> I'll try to make that modification to the patch.
>
> As for my use case: I'd like to use default atomic pushes when pushing a
> new tag among our stable branch, but inevitably forgetting to rebase
> beforehand. Therefore pushing a "dangling" commit/tag

Making it per-remote would also address my concern about scripts.
Existing scripts may be using

	git push $url $refs

and relying on the push to partially succeed even when some refs
cannot be fast-forwarded.  (For example, I had such a script that did

	git push my-mirror refs/remotes/origin/*:refs/heads/*
	git push my-mirror +origin/pu:pu

because the first command would fail to update pu, and then the second
command would clean up after it.)  A configuration that globally
changes the effect of "git push" to mean "git push --atomic" would
break such scripts.  A per-remote configuration is tightly enough
scoped to not be likely to cause unrelated breakage for users that run
scripts written by others.

Thanks and hope that helps,
Jonathan

Re: [PATCH] push: allow atomic flag via configuration

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 24, 2017 at 6:17 PM, Romuald Brunet <romuald@chivil.com> wrote:
> +push.atomic::
> +       If set to true enable `--atomic` option by default.  You
> +       may override this configuration at time of push by specifying
> +       `--no-atomic`.
> +

This should also be mentioned in the --atomic documentation in
git-push.txt itself. See e.g. the documentation for pull --rebase for
an example of this.

Re: [PATCH] push: allow atomic flag via configuration

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 24, 2017 at 6:17 PM, Romuald Brunet <romuald@chivil.com> wrote:
> +test_expect_success 'atomic option possible via git-config' '
> +       # prepare the repo
> +       mk_repo_pair &&
> +       (
> +               cd workbench &&
> +               test_commit one &&
> +               git checkout -b second master &&
> +               test_commit two &&
> +               git push --mirror up
> +       ) &&
> +       # a third party modifies the server side:
> +       (
> +               cd upstream &&
> +               git checkout second &&
> +               git tag test_tag second
> +       ) &&
> +       # see if we can now push both branches.
> +       (
> +               cd workbench &&
> +               git config push.atomic true &&
> +               git checkout master &&
> +               test_commit three &&
> +               git checkout second &&
> +               test_commit four &&
> +               git tag test_tag &&
> +               test_must_fail git push --tags up master second
> +       ) &&
> +       test_refs master HEAD@{3} &&
> +       test_refs second HEAD@{1}
> +'
> +

Sent my earlier E-Mail too soon, so I missed this, there's no test
here for what "git config push.atomic false" && "git push --atomic"
does, is that atomic or not? I.e. what does "git -c push.atomic=false
push --atomic" do? Does the CLI option override the config as it
should?

Re: [PATCH] push: allow atomic flag via configuration

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, Mar 24, 2017 at 6:17 PM, Romuald Brunet <romuald@chivil.com> wrote:
>> +test_expect_success 'atomic option possible via git-config' '
>> +       # prepare the repo
>> +       mk_repo_pair &&
>> +       (
>> +               cd workbench &&
>> +               test_commit one &&
>> +               git checkout -b second master &&
>> +               test_commit two &&
>> +               git push --mirror up
>> +       ) &&
>> +       # a third party modifies the server side:
>> +       (
>> +               cd upstream &&
>> +               git checkout second &&
>> +               git tag test_tag second
>> +       ) &&
>> +       # see if we can now push both branches.
>> +       (
>> +               cd workbench &&
>> +               git config push.atomic true &&
>> +               git checkout master &&
>> +               test_commit three &&
>> +               git checkout second &&
>> +               test_commit four &&
>> +               git tag test_tag &&
>> +               test_must_fail git push --tags up master second
>> +       ) &&
>> +       test_refs master HEAD@{3} &&
>> +       test_refs second HEAD@{1}
>> +'
>> +
>
> Sent my earlier E-Mail too soon, so I missed this, there's no test
> here for what "git config push.atomic false" && "git push --atomic"
> does, is that atomic or not? I.e. what does "git -c push.atomic=false
> push --atomic" do? Does the CLI option override the config as it
> should?

Good points.  Thanks for reading and reviewing the tests carefully.