Re: [PATCH 0/2] Optionally support push options on up-to-date branches

[Christopher Lindee <christopher.lindee@webpros.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Christopher Lindee <christopher.lindee@webpros.com> writes:
> 
> > Some Git servers can take actions on a branch using push options; to do this,
> > the branch ref must be passed along with the push options.  However, if the
> > branch is up-to-date, then Git submits push options without any refs, so the
> > server is not given a branch to act upon and the push options become no-ops.
> 
> Yeah, the send-pack/receive-pack were written in the simpler days
> back when "pushing the same object again to the ref was an absolute
> no-op", and push options that breaks the expectation did not exist.
> 
> It makes sense to allow a (seemingly no-op) push to go through with
> an option.
> 
> And even without custom push option, recording this seemingly no-op
> push as a ref "update" does make sense--push certificate records
> what object the pusher wanted each target ref to have, and omitting
> a ref from the record only because the ref was already pointing at
> the desired object is losing information.

Hmm... does this mean that push certificate records should imply this new
option?

> So I doubly agree with the reasoning beind this change.
> 
> > This changeset proposes to address this issue by adding an option to `push` and
> > `send-pack` that, when specified, will send refs where the old-oid and new-oid
> 
> "where" -> "even if"

Excellent clarification!

> > are identical - instead of silently skipping these refs.  The first commit
> > introduces the `--send-up-to-date` option to toggle this behavior, while the
> > second commit updates the commands to output an `(up-to-date)` notice for each
> > branch with an identical old-oid and new-oid.
> >
> > Notably, the `--force` option will not send a ref when the remote is up-to-date.
> 
> And it makes sense *not* to update `--force` to do the no-op push,
> becaues you may not want to (accidentally) force push a ref that
> does not fast-forward.  As I already said, tying this with use of
> the "-o" option is not sufficient.  So I agree we may want a new
> option to trigger this behaviour.
> 
> A radical counter-proposal for the design is to update the client
> side to do this unconditionally, without needing any new option.
> For an already up-to-date ref, its only contribution to the cost of
> "git push" is from its "Finally, tell the other end!" instruction,
> which is in the order of 100 bytes per ref, and it should not add to
> the pack generation cost at all [*].

About 100 bytes per ref can grow when using a ref glob; for example,
`git push <remote> refs/tags/*:refs/tags/*` would push up ~1200 refs
to the Git repository, or about 120 KiB across ~80 Ethernet frames.
That's not much compared to the amount of data fetched during a push,
but it seems like a lot of unnecessary data in most instances and it
would add a tiny bit of latency (more on that in a second).

>     Side note: But we have to be careful---if the receiving end is
>     totally up-to-date and there is absolutely no ref, I think the
>     current code will not even call pack_objects(), and you'd
>     probably want a similar optimization to avoid the cost to spawn
>     pack_objects().

Good point.  This got me thinking about other potential side-effects.

What would happen if two actors push to the repository at the same
time with a lot of no-op branch changes?  For example,

Actors 1 & 2 (either actor could be first):
push< aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/heads/main\0report-status ...
push< bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb refs/heads/branch
push< ffffffffffffffffffffffffffffffffffffffff .have
push< ...
push< 0000 

Actor 1:
push> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa 5555555555555555555555555555555555555555 refs/heads/main\0 report-status-v2 ...
push> bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb refs/heads/branch
push> 0000

Actor 2:
push> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/heads/main\0 report-status-v2 ...
push> bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb 4444444444444444444444444444444444444444 refs/heads/branch
push> 0000

If repository locking makes `push<` and `push>` a combined atomic operation,
then this could never happen.  Otherwise, I suspect Actor 2 would receive an
error (or at least a warning) since refs/heads/main is now 555... instead of
aaa....  Moreover, if an error and `--atomic` is specified, then everything
fails to update.

And, as mentioned before, with a large number of refs there will be
additional latency, which increases the chance of collisions.

> I do not know if "send-up-to-date" is a great name for the option,
> though.

Agreed; names are difficult.

I considered --always-send-refs, but thought it a bit vague since it
implies refs are sometimes not sent, but requires you to look at the
documentation to understand under what conditions refs would not be
sent.  I also feared there might be other conditions where refs are
not sent that I was unaware of.

I think something with "noop" makes sense; thing is, which do we use?
1) --force-nop
2) --force-noop
3) --force-no-op

I've seen all 3 ways of writing "no operation".  Everyone has their
preference (e.g. I did 1; Junio suggested 3 and later used 2) and if
the one we choose doesn't match up to a user's, typos seem inevitable.
Though I suppose we already have this issue with color/colour.

I also want to consider the audience.  To me, "no-op" feels low-level.
Meanwhile, "send-up-to-date" uses the same nomenclature as the UI
displays and feels more porcelain.  Frankly, this options seems more
low-level to me, since users probably want to consider the possible
side-effects on server-side before using it.  However, I could see
GitLab telling average users to use the option on their Push Options
(https://docs.gitlab.com/ee/user/project/push_options.html) docs.
I'm curious what others think.

> >
> > Chris Lindee (2):
> >   Teach send-pack & push to --send-up-to-date refs
> >   Add transport message for up-to-date references
> >
> >  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                     | 3 +++
> >  transport.h                     | 1 +
> >  9 files changed, 32 insertions(+), 4 deletions(-)
> >
> >
> > base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0