Junio C Hamano <gitster@xxxxxxxxx> writes:
> Christopher Lindee <christopher.lindee@xxxxxxxxxxx> 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.
