[PATCH 0/3] Fixing volatile HEAD in push.default = current

[PATCH 0/3] Fixing volatile HEAD in push.default = current

[Ramkumar Ramachandra]

There's still a lot to think about.

[3/3] is the big itch: [1/2] and [2/2] are just setup patches.

Ramkumar Ramachandra (3):
  push: factor out the detached HEAD error message
  push: fail early with detached HEAD and current
  push: don't push the volatile HEAD with current

 builtin/push.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

-- 
1.8.3.rc3.7.gc1ff30b

[PATCH 1/3] push: factor out the detached HEAD error message

[Ramkumar Ramachandra]

With push.default set to upstream or simple, and a detached HEAD, git
push prints the following error:

  $ git push
  fatal: You are not currently on a branch.
  To push the history leading to the current (detached HEAD)
  state now, use

    git push ram HEAD:<name-of-remote-branch>

This error is not unique to upstream or simple: current cannot push with
a detached HEAD either.  So, factor out the error string in preparation
for using it in current.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 909c34d..ef3aa97 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -113,17 +113,19 @@ static NORETURN int die_push_simple(struct branch *branch, struct remote *remote
 	    remote->name, branch->name, advice_maybe);
 }
 
+static const char message_detached_head_die[] =
+	N_("You are not currently on a branch.\n"
+	   "To push the history leading to the current (detached HEAD)\n"
+	   "state now, use\n"
+	   "\n"
+	   "    git push %s HEAD:<name-of-remote-branch>\n");
+
 static void setup_push_upstream(struct remote *remote, int simple)
 {
 	struct strbuf refspec = STRBUF_INIT;
 	struct branch *branch = branch_get(NULL);
 	if (!branch)
-		die(_("You are not currently on a branch.\n"
-		    "To push the history leading to the current (detached HEAD)\n"
-		    "state now, use\n"
-		    "\n"
-		    "    git push %s HEAD:<name-of-remote-branch>\n"),
-		    remote->name);
+		die(_(message_detached_head_die), remote->name);
 	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
 		die(_("The current branch %s has no upstream branch.\n"
 		    "To push the current branch and set the remote as upstream, use\n"
-- 
1.8.3.rc3.7.gc1ff30b

[PATCH 2/3] push: fail early with detached HEAD and current

[Ramkumar Ramachandra]

Setting push.default to current adds the refspec "HEAD" for the
transport layer to handle.  If "HEAD" doesn't resolve to a branch (and
since no refspec rhs is specified), the push fails after some time with
a cryptic error message:

  $ git push
  error: unable to push to unqualified destination: HEAD
  The destination refspec neither matches an existing ref on the remote nor
  begins with refs/, and we are unable to guess a prefix based on the source ref.
  error: failed to push some refs to 'git@github.com:artagnon/git'

Fail early with a nicer error message:

  $ git push
  fatal: You are not currently on a branch.
  To push the history leading to the current (detached HEAD)
  state now, use

    git push ram HEAD:<name-of-remote-branch>

Just like in the upstream and simple cases.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/push.c b/builtin/push.c
index ef3aa97..a79038c 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -175,6 +175,8 @@ static void warn_unspecified_push_default_configuration(void)
 
 static void setup_default_push_refspecs(struct remote *remote)
 {
+	struct branch *branch = branch_get(NULL);
+
 	switch (push_default) {
 	default:
 	case PUSH_DEFAULT_UNSPECIFIED:
@@ -194,6 +196,8 @@ static void setup_default_push_refspecs(struct remote *remote)
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
+		if (!branch)
+			die(_(message_detached_head_die), remote->name);
 		add_refspec("HEAD");
 		break;
 
-- 
1.8.3.rc3.7.gc1ff30b

[PATCH 3/3] push: don't push the volatile HEAD with current

[Ramkumar Ramachandra]

Since a push does not lock the "HEAD" ref, there is no guarantee that it
will remain the same for the entire operation of the push.  Practically,
this means that with push.default set to current:

  # on branch push-current-head
  $ git push
  # on another terminal
  $ git checkout master
  # return to the first terminal
  # the push tried to push master!

Avoid this confusion by adding the branch that HEAD resolves to as the
refspec to push (as opposed to the literal "HEAD").  With this change,
the output of the push changes subtly from:

  $ git push
  ...
   * [new branch]      HEAD -> push-current-head

to:

  $ git push
  ...
   * [new branch]      push-current-head -> push-current-head

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index a79038c..d819487 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -198,7 +198,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 	case PUSH_DEFAULT_CURRENT:
 		if (!branch)
 			die(_(message_detached_head_die), remote->name);
-		add_refspec("HEAD");
+		add_refspec(branch->name);
 		break;
 
 	case PUSH_DEFAULT_NOTHING:
-- 
1.8.3.rc3.7.gc1ff30b

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> There's still a lot to think about.

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.

I haven't picked them up, and I won't be picking them up today, as I
suspect this series may conflict with the pre-2.0 preparation and
2.0 transition patches and I may end up having to fix conflicts
unnecessarily (resolving is eventually needed before 2.0 happens,
but resolving them, or even having to worry about the possibility
that I may have to do so, do not have to steal time from me today).

Thanks.

> [3/3] is the big itch: [1/2] and [2/2] are just setup patches.
>
> Ramkumar Ramachandra (3):
>   push: factor out the detached HEAD error message
>   push: fail early with detached HEAD and current
>   push: don't push the volatile HEAD with current
>
>  builtin/push.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> 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?

> I haven't picked them up, and I won't be picking them up today, as I
> suspect this series may conflict with the pre-2.0 preparation and
> 2.0 transition patches and I may end up having to fix conflicts
> unnecessarily (resolving is eventually needed before 2.0 happens,
> but resolving them, or even having to worry about the possibility
> that I may have to do so, do not have to steal time from me today).

We're at 1.8.3; isn't it a little early to be thinking of 2.0?  Is it
conflicting with jc/push-2.0-default-to-simple in pu?  I should
re-send after this topic graduates to master in 2.0?

I have no problems re-sending at a convenient time (provided you tell
me how to determine that convenient time), but reviews don't have to
wait: it's fresh in my memory now.

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> We're at 1.8.3; isn't it a little early to be thinking of 2.0?  Is it
> conflicting with jc/push-2.0-default-to-simple in pu?  I should
> re-send after this topic graduates to master in 2.0?

I implicitly asked you not to ask as I do not even have to worry
about conflicts ;-)  That does not mean I'll discard.  Please ping
me back later after 1.8.3 if you do not see it in my tree anywhere;
I may have found time to splice these _before_ the 2.0 patch (which
is where it should come, I think) by then.

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> 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.

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Ramkumar Ramachandra]

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

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> 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.

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Ramkumar Ramachandra]

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?

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

Is there anything to "fix" in the first place, you have to wonder.

Your "git push there HEAD:master" would roughly do the following:

    (1) read HEAD to learn what commit you are pushing;

    (2) contact the other side and find where there tips are;

    (3) send a packfile that should be enough for the other side to
        have complete history leading to the commit you read in (1);

    (4) tell the other side to update its 'master' to the commit you
        read in (1).

If you drop step (1) and replace "the commit you read in (1)" in
steps (3) and (4) with "the commit you see in HEAD at this point by
re-reading HEAD", then such a "git push" that races with something
else you do in your other terminal may break---you can cause it to
see different commits at steps (3) and (4), potentially getting the
other side out of sync (but the receiving end does an independent
connectivity check so your push will likely to be rejected).

And the fix to such a breakage is to structure the code like the
above four steps to make it race-free.

If I understand your example correctly, you are talking about a
quite different thing.  "git push there HEAD:master" racing with
your other terminal that changes the HEAD sees different HEAD
depending on the order:

    (a) if the other terminal changes the HEAD first, step (1) will
        see that updated HEAD; or

    (b) if the step (1) reads HEAD before you change it in the other
        terminal, it will see the original HEAD.

But that is very much to be expected, isn't it?  It sounds similar
to

    I have "largedir" I want to get rid of, but there is a directory
    I want to save, "largedir/precious", in it, so I do

        cp -R largedir/precious precious

    and then run 'rm -rf largedir' in another terminal in parallel.

Is there anything to fix?

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>     I have "largedir" I want to get rid of, but there is a directory
>     I want to save, "largedir/precious", in it, so I do
>
>         cp -R largedir/precious precious
>
>     and then run 'rm -rf largedir' in another terminal in parallel.

I would argue that there is something to "fix", but that fix involves
making the cp a purely atomic operation which is super-complicated,
and totally not worth it.  Would you _not_ like the above example to
work?  Then how can you say that there's nothing to be fixed?

Consider a slightly different example: I rename a file while having an
active file handle open in a process that's reading the file.  Will
the rename fail or will the fread() in the process fail?  Nope, both
work fine.  Replace "rename" with "remove", and we still have the same
answer.  Ofcourse there are no guarantees: I can start up another
process to overwrite the sectors corresponding to that file's data
with zeros; unless the complete file is there in the kernel buffer, a
read() will eventually end up slurping in the zeros (or fail?), right?
 It's just that it works in practice.

Yet another example: I have a terminal pointing to a directory, and I
remove that directory in another terminal.  When I switch back to the
original terminal, I can't cd .., because getcwd() fails.  This has
annoyed me endlessly in practice, and I would really like to fix this
if I can.

Don't accept the way things are, and assume that there's nothing to be
"fixed".  In my opinion, if something about a piece of software annoys
you, there is always something to fix.  It just depends on what _can_
be fixed in a reasonable amount of time with a good engineering
solution.  There's no need to go to the other extreme: I'm not
interested in rewriting the whole operating system in Haskell and
providing theoretical guarantees for everything.

Coming back to our push example, I don't see why you think HEAD is
special: I could even say git push master and expect it to race with
an update-ref.  But nobody is complaining about that: if someone does
complain, I would seriously consider copying master to PUSH_HEAD early
(and push that).  With HEAD, however, someone is complaining (namely,
me): pushing usually means that I've finished working on that branch,
and want to switch to another branch and continue working.  Why should
I have to wait for the push to complete?  I've hit this bug several
times (from terminal as well as Magit), and this patch fixes the
problem for me in practice.

That said, I agree that my patch does not guarantee anything (and I
will modify my commit message to clarify this).  I'm just expressing
my opinion on the issue of "fixing problems".

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Andreas Krey]

On Thu, 23 May 2013 13:25:55 +0000, Ramkumar Ramachandra wrote:
> Junio C Hamano wrote:
> >     I have "largedir" I want to get rid of, but there is a directory
> >     I want to save, "largedir/precious", in it, so I do
> >
> >         cp -R largedir/precious precious
> >
> >     and then run 'rm -rf largedir' in another terminal in parallel.

'mv largedir/precious precious; rm -rf largedir'? No race here.

...
> Consider a slightly different example: I rename a file while having an
> active file handle open in a process that's reading the file.  Will
> the rename fail or will the fread() in the process fail?  Nope, both
> work fine.  Replace "rename" with "remove", and we still have the same
> answer.  Ofcourse there are no guarantees: I can start up another
> process to overwrite the sectors corresponding to that file's data
> with zeros; unless the complete file is there in the kernel buffer, a
> read() will eventually end up slurping in the zeros (or fail?), right?

Oh, there are guarantees, they just don't include the case where you
take a shotgun to the disk. (Or do it on an nfs mount and delete the
file from another machine.)

...

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Junio C Hamano]

Andreas Krey <a.krey@gmx.de> writes:

> On Thu, 23 May 2013 13:25:55 +0000, Ramkumar Ramachandra wrote:
>> Junio C Hamano wrote:
>> >     I have "largedir" I want to get rid of, but there is a directory
>> >     I want to save, "largedir/precious", in it, so I do
>> >
>> >         cp -R largedir/precious precious
>> >
>> >     and then run 'rm -rf largedir' in another terminal in parallel.
>
> 'mv largedir/precious precious; rm -rf largedir'? No race here.

Yeah, but 'cp -R largedir/precious precious; rm -fr largedir' is the
same thing.  Ram's original issue was "I do something else in
another terminal without waiting for the first command to finish".

Even with 'mv', between the time the main in mv starts and the
process finally issues rename(2) on the directory, you can start
running what competes and interferes with it in another terminal,
so it does not fundamentally change anything, I would have to say.

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> I would argue that there is something to "fix", but that fix involves
> making the cp a purely atomic operation which is super-complicated,
> and totally not worth it.  Would you _not_ like the above example to
> work?

No.  I do not think I like the above example to "work", at all.

I know we live in the real world, and I do not want to see such
serializing implementations of cp or rm that try to "fix", as it
will interfere my everyday use where such a race does not matter.

Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current

[Andreas Krey]

On Thu, 23 May 2013 07:45:34 +0000, Junio C Hamano wrote:
...
> Even with 'mv', between the time the main in mv starts and the
> process finally issues rename(2) on the directory, you can start
> running what competes and interferes with it in another terminal,
> so it does not fundamentally change anything, I would have to say.

Not fundamentally, it's just that the mv is fast enough you wouldn't
even think of switch to another term/backgrounding it.

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

[PATCH 1/3] push: factor out the detached HEAD error message

[Ramkumar Ramachandra]

With push.default set to upstream or simple, and a detached HEAD, git
push prints the following error:

  $ git push
  fatal: You are not currently on a branch.
  To push the history leading to the current (detached HEAD)
  state now, use

    git push ram HEAD:<name-of-remote-branch>

This error is not unique to upstream or simple: current cannot push with
a detached HEAD either.  So, factor out the error string in preparation
for using it in current.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 909c34d..ef3aa97 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -113,17 +113,19 @@ static NORETURN int die_push_simple(struct branch *branch, struct remote *remote
 	    remote->name, branch->name, advice_maybe);
 }
 
+static const char message_detached_head_die[] =
+	N_("You are not currently on a branch.\n"
+	   "To push the history leading to the current (detached HEAD)\n"
+	   "state now, use\n"
+	   "\n"
+	   "    git push %s HEAD:<name-of-remote-branch>\n");
+
 static void setup_push_upstream(struct remote *remote, int simple)
 {
 	struct strbuf refspec = STRBUF_INIT;
 	struct branch *branch = branch_get(NULL);
 	if (!branch)
-		die(_("You are not currently on a branch.\n"
-		    "To push the history leading to the current (detached HEAD)\n"
-		    "state now, use\n"
-		    "\n"
-		    "    git push %s HEAD:<name-of-remote-branch>\n"),
-		    remote->name);
+		die(_(message_detached_head_die), remote->name);
 	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
 		die(_("The current branch %s has no upstream branch.\n"
 		    "To push the current branch and set the remote as upstream, use\n"
-- 
1.8.3.12.gbd56588