| Message ID | SA1PR14MB469144B8903909F65493323A8D2B2@SA1PR14MB4691.namprd14.prod.outlook.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Optionally support push options on up-to-date branches | expand |
Christopher Lindee <christopher.lindee@webpros.com> writes: > Subject: Re: [PATCH 1/2] Teach send-pack & push to --send-up-to-date refs cf. Documentation/SubmittingPatches:describe-changes. Perhaps [PATCH 1/2] push: allow pushing a no-op update to refs or something? > Provide an option that forces the local Git transport to transmit a local > ref even when the receiving ref would not change (i.e. the local & remote > refs point to the same object). This is not done normally, as no changes > would take place on the server in a vanilla Git setup. However, some Git > servers support push options and those push options decide which branches > to operate on based on the refs that are transmitted alongside the option. > This option ensures the refs listed on the command line are always sent. Flip the order of description to give your observation of the current behaviour first, your perceived problem in it next, and then finally your solution. That way, those who are totally unfamiliar with the problem area can just read from start to end straight, and those who know what we do currently can skip and start from your problem description. Also, it would be nice if you throw in the issue about missing ref in the push certificate, which I mentioned in my response to the cover letter. > Documentation/git-push.txt | 8 +++++++- > Documentation/git-send-pack.txt | 7 +++++++ > builtin/push.c | 1 + > builtin/send-pack.c | 4 ++++ > send-pack.c | 2 +- > send-pack.h | 3 ++- > transport-helper.c | 7 ++++++- > transport.c | 1 + > transport.h | 1 + > 9 files changed, 30 insertions(+), 4 deletions(-) No tests? > 'git push' [--all | --branches | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>] > [--repo=<repository>] [-f | --force] [-d | --delete] [--prune] [-q | --quiet] [-v | --verbose] > - [-u | --set-upstream] [-o <string> | --push-option=<string>] > + [-u | --set-upstream] [-o <string> | --push-option=<string>] [--send-up-to-date] I do not know if the new option is a name that is easy to understand. I am not great at naming, either but how does "--force-no-op" sound? > + Usually, the command will not send a local ref when the remote ref > + already matches, as no change would occur if it did. This flag In the context of "push", 'match' is a verb that is used in different contexts, like "'git push master' finds a ref that matches 'master' and updates refs/heads/master". You would want to avoid it when you can. Usually the command omits instructing the receiving end to update a ref to point at an object, if the target ref points at the exact object already, as no change ... > + disables that check. This allows push options to be sent alongside > + up-to-date references on the remote. Aside from options and push certificates, there may be other side effects from this change. I am not sure if we want to make sure we enumerate the benefit all like this. Perhaps drop "This allows ..." altogether? > diff --git a/send-pack.c b/send-pack.c > index 37f59d4f66..30b0f2f276 100644 > --- a/send-pack.c > +++ b/send-pack.c > @@ -313,7 +313,7 @@ static int check_to_send_update(const struct ref *ref, const struct send_pack_ar > case REF_STATUS_REJECT_NODELETE: > return CHECK_REF_STATUS_REJECTED; > case REF_STATUS_UPTODATE: > - return CHECK_REF_UPTODATE; > + return args->send_uptodate ? 0 : CHECK_REF_UPTODATE; > > default: > case REF_STATUS_EXPECTING_REPORT: Given the existing flow of this code, I would prefer to see it written more like so: diff --git i/send-pack.c w/send-pack.c index 37f59d4f66..97d01726bb 100644 --- i/send-pack.c +++ w/send-pack.c @@ -313,8 +313,9 @@ static int check_to_send_update(const struct ref *ref, const struct send_pack_ar case REF_STATUS_REJECT_NODELETE: return CHECK_REF_STATUS_REJECTED; case REF_STATUS_UPTODATE: - return CHECK_REF_UPTODATE; - + if (!args->send_uptodate) + return CHECK_REF_UPTODATE; + /* fallthru */ default: case REF_STATUS_EXPECTING_REPORT: /* already passed checks on the local side */ to make it clear that the caller gets 0 ("go ahead and do it") unless it is one of the cases explicitly listed abouve the "default" label. > + int do_nop = flags & TRANSPORT_PUSH_SEND_UPTODATE; Here we do call it "do_nop", showing that at least to you, "nop" is a much more fitting word than "uptodate" for what we are trying to achieve in this topic. It would give us one piece of good input to decide what the end-user facing name should be. In fact it is where I took inspiration from for "force-noop" I mentioned earlier. > @@ -1010,7 +1011,11 @@ static int push_refs_with_push(struct transport *transport, > } else > continue; > case REF_STATUS_UPTODATE: > - continue; > + if (do_nop) { > + ; /* do nothing */ > + } > + else > + continue; Drop needless braces around a single-statement block. Alternatively, we could write it like so: if (!do_nop) continue; /* fallthru */ but I think what you wrote, modulo the unnecessary braces, makes the intent a bit more clear. > default: > ; /* do nothing */ > }
Junio C Hamano <gitster@pobox.com> writes: > Christopher Lindee <christopher.lindee@webpros.com> writes: > > > Subject: Re: [PATCH 1/2] Teach send-pack & push to --send-up-to-date refs > > cf. Documentation/SubmittingPatches:describe-changes. > > Perhaps > > [PATCH 1/2] push: allow pushing a no-op update to refs > > or something? Thanks! I will change the commit message. > > Provide an option that forces the local Git transport to transmit a local > > ref even when the receiving ref would not change (i.e. the local & remote > > refs point to the same object). This is not done normally, as no changes > > would take place on the server in a vanilla Git setup. However, some Git > > servers support push options and those push options decide which branches > > to operate on based on the refs that are transmitted alongside the option. > > This option ensures the refs listed on the command line are always sent. > > Flip the order of description to give your observation of the > current behaviour first, your perceived problem in it next, and then > finally your solution. That way, those who are totally unfamiliar > with the problem area can just read from start to end straight, and > those who know what we do currently can skip and start from your > problem description. Can do. I wonder if the new message will come out justified too. > Also, it would be nice if you throw in the issue about missing ref > in the push certificate, which I mentioned in my response to the > cover letter. I'd like to get a better understanding of the feature before I comment on push certificates. I'll read up and see if I can include it. > > Documentation/git-push.txt | 8 +++++++- > > Documentation/git-send-pack.txt | 7 +++++++ > > builtin/push.c | 1 + > > builtin/send-pack.c | 4 ++++ > > send-pack.c | 2 +- > > send-pack.h | 3 ++- > > transport-helper.c | 7 ++++++- > > transport.c | 1 + > > transport.h | 1 + > > 9 files changed, 30 insertions(+), 4 deletions(-) > > No tests? Only manual. I'm happy to add automated tests; can you recommend a t####? I was also hesitant since I did not know what to expect and thought a wholesale redesign might be required; v2 will have less uncertainty. > > 'git push' [--all | --branches | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>] > > [--repo=<repository>] [-f | --force] [-d | --delete] [--prune] [-q | --quiet] [-v | --verbose] > > - [-u | --set-upstream] [-o <string> | --push-option=<string>] > > + [-u | --set-upstream] [-o <string> | --push-option=<string>] [--send-up-to-date] > > I do not know if the new option is a name that is easy to > understand. I am not great at naming, either but how does > "--force-no-op" sound? I already gave a response in a reply to the cover letter, but I also have some new thoughts: Is this something that can be easy to understand? It seems to require some knowledge of transports and server behavior. Moreover, with the server-side implications mentioned by brian, we should encourage a deeper understanding before people use this option. In that case, perhaps a simple name is not as valuable. How do people feel about --aggressive? ;) To discourage unconsidered use, we could go with a scary option name, like --disable-noop-optimizations, though that particular name has the unfortunate American vs. British spelling issue. It does exactly what it says on the tin® though, which is nice. See the cover letter reply for other suggestions. > > + Usually, the command will not send a local ref when the remote ref > > + already matches, as no change would occur if it did. This flag > > In the context of "push", 'match' is a verb that is used in > different contexts, like "'git push master' finds a ref that matches > 'master' and updates refs/heads/master". You would want to avoid it > when you can. > > Usually the command omits instructing the receiving end to > update a ref to point at an object, if the target ref points at > the exact object already, as no change ... Good to know. I will avoid overloading the term. > > + disables that check. This allows push options to be sent alongside > > + up-to-date references on the remote. > > Aside from options and push certificates, there may be other side > effects from this change. I am not sure if we want to make sure we > enumerate the benefit all like this. Perhaps drop "This allows ..." > altogether? I think we both agree this is a option that is difficult to convey, so much so that I think we need examples. How obvious would it be that I wanted to use this for push options if I didn't explicitly mention it? But I also think you're right. There are a decent number of things to consider before using this. Perhaps a fuller discussion on the option - with examples - is warranted in a more deep-dive document and we can refer the user to that document here. If so, would any existing pages be appropriate? > > diff --git a/send-pack.c b/send-pack.c > > index 37f59d4f66..30b0f2f276 100644 > > --- a/send-pack.c > > +++ b/send-pack.c > > @@ -313,7 +313,7 @@ static int check_to_send_update(const struct ref *ref, const struct send_pack_ar > > case REF_STATUS_REJECT_NODELETE: > > return CHECK_REF_STATUS_REJECTED; > > case REF_STATUS_UPTODATE: > > - return CHECK_REF_UPTODATE; > > + return args->send_uptodate ? 0 : CHECK_REF_UPTODATE; > > > > default: > > case REF_STATUS_EXPECTING_REPORT: > > Given the existing flow of this code, I would prefer to see it > written more like so: > > diff --git i/send-pack.c w/send-pack.c > index 37f59d4f66..97d01726bb 100644 > --- i/send-pack.c > +++ w/send-pack.c > @@ -313,8 +313,9 @@ static int check_to_send_update(const struct ref *ref, const struct send_pack_ar > case REF_STATUS_REJECT_NODELETE: > return CHECK_REF_STATUS_REJECTED; > case REF_STATUS_UPTODATE: > - return CHECK_REF_UPTODATE; > - > + if (!args->send_uptodate) > + return CHECK_REF_UPTODATE; > + /* fallthru */ > default: > case REF_STATUS_EXPECTING_REPORT: > /* already passed checks on the local side */ > > to make it clear that the caller gets 0 ("go ahead and do it") > unless it is one of the cases explicitly listed abouve the "default" > label. Nice; will do. > > + int do_nop = flags & TRANSPORT_PUSH_SEND_UPTODATE; > > Here we do call it "do_nop", showing that at least to you, "nop" is > a much more fitting word than "uptodate" for what we are trying to > achieve in this topic. It would give us one piece of good input to > decide what the end-user facing name should be. In fact it is where > I took inspiration from for "force-noop" I mentioned earlier. > > > @@ -1010,7 +1011,11 @@ static int push_refs_with_push(struct transport *transport, > > } else > > continue; > > case REF_STATUS_UPTODATE: > > - continue; > > + if (do_nop) { > > + ; /* do nothing */ > > + } > > + else > > + continue; > > Drop needless braces around a single-statement block. > Alternatively, we could write it like so: > > if (!do_nop) > continue; > /* fallthru */ > > but I think what you wrote, modulo the unnecessary braces, makes the > intent a bit more clear. > > > default: > > ; /* do nothing */ > > } I was having trouble coming up with a good name when developing this, so I went with something that was 6 characters long. I'm open to identifier suggestions that might be easier to read. Thanks, Chris.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 9b7cfbc5c1..44d1e04931 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -11,7 +11,7 @@ SYNOPSIS [verse] 'git push' [--all | --branches | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-d | --delete] [--prune] [-q | --quiet] [-v | --verbose] - [-u | --set-upstream] [-o <string> | --push-option=<string>] + [-u | --set-upstream] [-o <string> | --push-option=<string>] [--send-up-to-date] [--[no-]signed|--signed=(true|false|if-asked)] [--force-with-lease[=<refname>[:<expect>]] [--force-if-includes]] [--no-verify] [<repository> [<refspec>...]] @@ -225,6 +225,12 @@ already exists on the remote side. line, the values of configuration variable `push.pushOption` are used instead. +--send-up-to-date:: + Usually, the command will not send a local ref when the remote ref + already matches, as no change would occur if it did. This flag + disables that check. This allows push options to be sent alongside + up-to-date references on the remote. + --receive-pack=<git-receive-pack>:: --exec=<git-receive-pack>:: Path to the 'git-receive-pack' program on the remote diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt index b9e73f2e77..2517d226d0 100644 --- a/Documentation/git-send-pack.txt +++ b/Documentation/git-send-pack.txt @@ -13,6 +13,7 @@ SYNOPSIS [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic] [--[no-]signed | --signed=(true|false|if-asked)] + [--send-up-to-date] [<host>:]<directory> (--all | <ref>...) DESCRIPTION @@ -88,6 +89,12 @@ be in a separate packet, and the list must end with a flush packet. options, error out. See linkgit:git-push[1] and linkgit:githooks[5] for details. +--send-up-to-date:: + Usually, the command will not send a local ref when the remote ref + already matches, as no change would occur if it did. This flag + disables that check. This allows push options to be sent alongside + up-to-date references on the remote. + <host>:: A remote host to house the repository. When this part is specified, 'git-receive-pack' is invoked via diff --git a/builtin/push.c b/builtin/push.c index 2fbb31c3ad..555e8b53fe 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -617,6 +617,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK), OPT_BIT(0, "follow-tags", &flags, N_("push missing but relevant tags"), TRANSPORT_PUSH_FOLLOW_TAGS), + OPT_BIT(0, "send-up-to-date", &flags, N_("send refs when there is no change"), TRANSPORT_PUSH_SEND_UPTODATE), OPT_CALLBACK_F(0, "signed", &push_cert, "(yes|no|if-asked)", N_("GPG sign the push"), PARSE_OPT_OPTARG, option_parse_push_signed), OPT_BIT(0, "atomic", &flags, N_("request atomic transaction on remote side"), TRANSPORT_PUSH_ATOMIC), diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 3df9eaad09..da6630fa12 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -19,6 +19,7 @@ static const char * const send_pack_usage[] = { " [--receive-pack=<git-receive-pack>]\n" " [--verbose] [--thin] [--atomic]\n" " [--[no-]signed | --signed=(true|false|if-asked)]\n" + " [--send-up-to-date]\n" " [<host>:]<directory> (--all | <ref>...)"), NULL, }; @@ -165,6 +166,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) const char *receivepack = "git-receive-pack"; unsigned dry_run = 0; unsigned send_mirror = 0; + unsigned send_uptodate = 0; unsigned force_update = 0; unsigned quiet = 0; int push_cert = 0; @@ -200,6 +202,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "stateless-rpc", &stateless_rpc, N_("use stateless RPC protocol")), OPT_BOOL(0, "stdin", &from_stdin, N_("read refs from stdin")), OPT_BOOL(0, "helper-status", &helper_status, N_("print status from remote helper")), + OPT_BOOL(0, "send-up-to-date", &send_uptodate, N_("send refs when there is no change")), OPT_CALLBACK_F(0, "force-with-lease", &cas, N_("<refname>:<expect>"), N_("require old value of ref to be at this value"), PARSE_OPT_OPTARG, parseopt_push_cas_option), @@ -221,6 +224,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) args.verbose = verbose; args.dry_run = dry_run; args.send_mirror = send_mirror; + args.send_uptodate = send_uptodate; args.force_update = force_update; args.quiet = quiet; args.push_cert = push_cert; diff --git a/send-pack.c b/send-pack.c index 37f59d4f66..30b0f2f276 100644 --- a/send-pack.c +++ b/send-pack.c @@ -313,7 +313,7 @@ static int check_to_send_update(const struct ref *ref, const struct send_pack_ar case REF_STATUS_REJECT_NODELETE: return CHECK_REF_STATUS_REJECTED; case REF_STATUS_UPTODATE: - return CHECK_REF_UPTODATE; + return args->send_uptodate ? 0 : CHECK_REF_UPTODATE; default: case REF_STATUS_EXPECTING_REPORT: diff --git a/send-pack.h b/send-pack.h index 7edb80596c..241b4278bc 100644 --- a/send-pack.h +++ b/send-pack.h @@ -27,7 +27,8 @@ struct send_pack_args { push_cert:2, stateless_rpc:1, atomic:1, - disable_bitmaps:1; + disable_bitmaps:1, + send_uptodate:1; const struct string_list *push_options; }; diff --git a/transport-helper.c b/transport-helper.c index dd6002b393..115f46f6b8 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -983,6 +983,7 @@ static int push_refs_with_push(struct transport *transport, int force_all = flags & TRANSPORT_PUSH_FORCE; int mirror = flags & TRANSPORT_PUSH_MIRROR; int atomic = flags & TRANSPORT_PUSH_ATOMIC; + int do_nop = flags & TRANSPORT_PUSH_SEND_UPTODATE; struct helper_data *data = transport->data; struct strbuf buf = STRBUF_INIT; struct ref *ref; @@ -1010,7 +1011,11 @@ static int push_refs_with_push(struct transport *transport, } else continue; case REF_STATUS_UPTODATE: - continue; + if (do_nop) { + ; /* do nothing */ + } + else + continue; default: ; /* do nothing */ } diff --git a/transport.c b/transport.c index df518ead70..84deadd2b6 100644 --- a/transport.c +++ b/transport.c @@ -867,6 +867,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re memset(&args, 0, sizeof(args)); args.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR); + args.send_uptodate = !!(flags & TRANSPORT_PUSH_SEND_UPTODATE); args.force_update = !!(flags & TRANSPORT_PUSH_FORCE); args.use_thin_pack = data->options.thin; args.verbose = (transport->verbose > 0); diff --git a/transport.h b/transport.h index 6393cd9823..61a20e84f0 100644 --- a/transport.h +++ b/transport.h @@ -158,6 +158,7 @@ struct transport { #define TRANSPORT_RECURSE_SUBMODULES_ONLY (1<<15) #define TRANSPORT_PUSH_FORCE_IF_INCLUDES (1<<16) #define TRANSPORT_PUSH_AUTO_UPSTREAM (1<<17) +#define TRANSPORT_PUSH_SEND_UPTODATE (1<<18) int transport_summary_width(const struct ref *refs);
Provide an option that forces the local Git transport to transmit a local ref even when the receiving ref would not change (i.e. the local & remote refs point to the same object). This is not done normally, as no changes would take place on the server in a vanilla Git setup. However, some Git servers support push options and those push options decide which branches to operate on based on the refs that are transmitted alongside the option. This option ensures the refs listed on the command line are always sent. Signed-off-by: Christopher Lindee <christopher.lindee@webpros.com> --- Documentation/git-push.txt | 8 +++++++- Documentation/git-send-pack.txt | 7 +++++++ builtin/push.c | 1 + builtin/send-pack.c | 4 ++++ send-pack.c | 2 +- send-pack.h | 3 ++- transport-helper.c | 7 ++++++- transport.c | 1 + transport.h | 1 + 9 files changed, 30 insertions(+), 4 deletions(-)