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?
> 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 */
> }
