[2/2] send-pack: downgrade push options error to warning
diff mbox series

Message ID 20200217144432.43920-2-sir@cmpwn.com
State New
Headers show
Series
  • [1/2] git-receive-pack: document push options
Related show

Commit Message

Drew DeVault Feb. 17, 2020, 2:44 p.m. UTC
Because the receiving end has to explicitly enable
receive.advertisePushOptions, and many servers don't, it doesn't make
sense to set push options globally when half of your pushes are just
going to die.

Signed-off-by: Drew DeVault <sir@cmpwn.com>
---
 send-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jeff King Feb. 18, 2020, 5:40 a.m. UTC | #1
On Mon, Feb 17, 2020 at 09:44:32AM -0500, Drew DeVault wrote:

> Because the receiving end has to explicitly enable
> receive.advertisePushOptions, and many servers don't, it doesn't make
> sense to set push options globally when half of your pushes are just
> going to die.

This makes me a little nervous, because we don't know what those push
options were supposed to do. Yet we'll proceed with the push minus the
options, which might perform an action that it's hard for the user to
undo. Imagine something as harmless as an option to suppress
notifications to your teammates about a push, or something as dangerous
as one that changes how the push will do an auto-deploy to a production
service.

That latter is probably unlikely, but it feels like we ought to be
erring on the conservative side here, especially since we've had the old
behavior for so many versions.

I do agree that setting push.pushOptions in your global gitconfig is
probably going to be annoying. Even in the repo .git/config, you might
push to multiple remotes, only some of which support the options.

So perhaps it would make sense to do one or both of:

 - allow remote.*.pushOptions for specific remotes

 - add a push.pushOptionIfAble key which behaves similarly to
   push.pushOption, but is quietly ignored if options aren't supported.
   Then you could put options there that you know are safe to be
   ignored.

I'm not sure exactly what kinds of options you want be setting globally,
so I'm not sure which of those would be more useful.

-Peff
Junio C Hamano Feb. 18, 2020, 5:44 p.m. UTC | #2
Jeff King <peff@peff.net> writes:

> This makes me a little nervous, because we don't know what those push
> options were supposed to do.

Thanks for stopping this early.  As you said, this die() is very
much deliberate way for us to make sure that we do not damage an
receiving end that is not prepared.
          
> So perhaps it would make sense to do one or both of:
>
>  - allow remote.*.pushOptions for specific remotes
>
>  - add a push.pushOptionIfAble key which behaves similarly to
>    push.pushOption, but is quietly ignored if options aren't supported.
>    Then you could put options there that you know are safe to be
>    ignored.

Sensible suggestions.

Patch
diff mbox series

diff --git a/send-pack.c b/send-pack.c
index 0407841ae8..8c81825e7d 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -439,7 +439,7 @@  int send_pack(struct send_pack_args *args,
 	use_atomic = atomic_supported && args->atomic;
 
 	if (args->push_options && !push_options_supported)
-		die(_("the receiving end does not support push options"));
+		warning(_("the receiving end does not support push options"));
 
 	use_push_options = push_options_supported && args->push_options;