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

From: Christopher Lindee <christopher.lindee@webpros.com>
Date: Mar 14, 2024, 23:08 UTC
Message-ID: <SA1PR14MB46916648249AFA5D723369188D292@SA1PR14MB4691.namprd14.prod.outlook.com>
In-Reply-To: <xmqqa5n168nh.fsf@gitster.g>

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

Show 19 quoted lines

> 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?

Show 6 quoted lines

> 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!

Show 19 quoted lines

> > 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).

Show 5 quoted lines

>     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.

Show 18 quoted lines

> >
> > 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