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