Ramkumar Ramachandra [off-list ref] writes:
Junio C Hamano wrote:
quoted
Is there? I do not think "volatile" is particularly a good
description for this, but showing what is pushed as a concrete
branch name feels like a good improvement to me, at least in
principle.
Okay. I used "volatile", because push does not lock HEAD when the
operation begins, even though it performs a super-late resolution (in
the transport-layer); HEAD is not guaranteed to remain invariant in
that time. Suggest nicer wording?
In general, when a command is working in your repository with a
working tree, we do not make any such promise that it keeps
operationg normally when you pull the rug under its feet from
another terminal ("git checkout maint" running at the same time "git
pull" would not have a chance to work correctly). Some are safe
(like "git push" racing with "git checkout maint" that would not
have to look at what the current branch is) and some are not (like
"git push github" racing with "git checkout maint && git push
origin HEAD:preview").
I view the value of this topic purely as "showing a real branch name
when that is what we actually pushed is a lot more preferrable than
showing HEAD, especially because the user may see it in the terminal
scrollback buffer hours after it happened". Explaining this patch
as "we avoid issues from simultaneously flipping HEAD by resolving
early" gives a false sense of security to the reader, as "early" has
to happen early enough for the patch to really avoid the issue, but
you are not in control of when the user does that flipping in the
other terminal.
Junio C Hamano wrote:
In general, when a command is working in your repository with a
working tree, we do not make any such promise that it keeps
operationg normally when you pull the rug under its feet from
another terminal ("git checkout maint" running at the same time "git
pull" would not have a chance to work correctly). Some are safe
(like "git push" racing with "git checkout maint" that would not
have to look at what the current branch is) and some are not (like
"git push github" racing with "git checkout maint && git push
origin HEAD:preview").
My current set of expectations look like this:
1. Commands that operate on a worktree (merge family) do not lock the
worktree before operating on it. These are fast synchronous commands
(operating on recent hot-cached stuff), and there is no _utility_ in a
perfectly atomic worktree operation. Might be an interesting
theoretical exercise to merge-trees in memory, but I have absolutely
no interest in such a problem.
2. Commands that operate on refs, the-index, and intermediate states
(update-ref, update-index, rebase-state family) do so atomically using
the lockfile API. The atomicity is important here, because we don't
want a higher-level command to half-fail. And it's trivial to
implement.
3. Commands that operate only on the object store are guaranteed not
to race as the object store itself is read-only (hash-object,
commit-tree family; gc being the exception). This is very important
for concurrent access in a server implementation.
In 3 out of the 4 push.default states, push is category (3).
push.default = current is a special case, and should try not to break
end-user expectation even if it involves resolving HEAD.
I view the value of this topic purely as "showing a real branch name
when that is what we actually pushed is a lot more preferrable than
showing HEAD, especially because the user may see it in the terminal
scrollback buffer hours after it happened". Explaining this patch
as "we avoid issues from simultaneously flipping HEAD by resolving
early" gives a false sense of security to the reader, as "early" has
to happen early enough for the patch to really avoid the issue, but
you are not in control of when the user does that flipping in the
other terminal.
Why should I lie in the patch? The terminal flipping was a very big
itch I had, and the patch fixes exactly that issue. Showing the real
branch name was an unintended side-effect.
I just said "early" and showed a nice end-user example in which it
works, not "theoretically impossible to race with". Better wording
(while not lying about the motivation behind the patch)?
Ramkumar Ramachandra [off-list ref] writes:
Why should I lie in the patch? The terminal flipping was a very big
itch I had, and the patch fixes exactly that issue. Showing the real
branch name was an unintended side-effect.
I just said "early" and showed a nice end-user example in which it
works, not "theoretically impossible to race with". Better wording
(while not lying about the motivation behind the patch)?
The patch may have been done by a wrong motivation, in that it does
not fundamentally "fix" the itch. The particular "itch" is not
something we are going to promise to the end users, ever, anyway.
The only remaining justification for the change is, even though the
user cannot _safely_ flip the branches with this patch, it improves
the output.
That does not make the patch wrong, but the original motivation is
an irrelevant, lost cause. "Even though this started to address an
itch, the patch does not fundamentally fix that itch at all." may be
a honest statement to make, but that alone is not a justification to
have this change.
The "side effect" is the only improvement this patch gives us, and
that happens to be a good enough justification. At that point, is
the original itch the patch does not correctly address even worth
mentioning? I answered "no" to that question.
So I do not think you are lying anything.
Junio C Hamano wrote:
The patch may have been done by a wrong motivation, in that it does
not fundamentally "fix" the itch. The particular "itch" is not
something we are going to promise to the end users, ever, anyway.
Just out of curiosity, is it possible to write a correct fix at all?
Even if the first statement in do_push() locks the HEAD ref, it's not
enough: the program may be wading around in git.c/setup.c when I
switch terminals and change HEAD, right?
So, our position on the matter is: no git command makes any guarantees
with respect to races, correct?