{"thread":{"id":"33888","subject":"[PATCH 0/3] Fixing volatile HEAD in push.default = current","startedAt":"2013-05-21T18:23:17Z","lastAt":"2013-05-23T19:27:25Z","messageCount":17,"participants":["Ramkumar Ramachandra","Junio C Hamano","Andreas Krey"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"218077","messageId":"1369160600-22337-1-git-send-email-artagnon@gmail.com","threadId":"33888","inReplyTo":null,"subject":"[PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-21T18:23:17Z","receivedAt":"2013-05-21T18:23:17Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"There's still a lot to think about.\n\n[3/3] is the big itch: [1/2] and [2/2] are just setup patches.\n\nRamkumar Ramachandra (3):\n  push: factor out the detached HEAD error message\n  push: fail early with detached HEAD and current\n  push: don't push the volatile HEAD with current\n\n builtin/push.c | 20 +++++++++++++-------\n 1 file changed, 13 insertions(+), 7 deletions(-)\n\n-- \n1.8.3.rc3.7.gc1ff30b\n"},{"id":"218079","messageId":"1369160600-22337-2-git-send-email-artagnon@gmail.com","threadId":"33888","inReplyTo":"1369160600-22337-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/3] push: factor out the detached HEAD error message","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-21T18:23:18Z","receivedAt":"2013-05-21T18:23:18Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"With push.default set to upstream or simple, and a detached HEAD, git\npush prints the following error:\n\n  $ git push\n  fatal: 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 ram HEAD:<name-of-remote-branch>\n\nThis error is not unique to upstream or simple: current cannot push with\na detached HEAD either.  So, factor out the error string in preparation\nfor using it in current.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/push.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 909c34d..ef3aa97 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -113,17 +113,19 @@ static NORETURN int die_push_simple(struct branch *branch, struct remote *remote\n \t    remote->name, branch->name, advice_maybe);\n }\n \n+static const char message_detached_head_die[] =\n+\tN_(\"You are not currently on a branch.\\n\"\n+\t   \"To push the history leading to the current (detached HEAD)\\n\"\n+\t   \"state now, use\\n\"\n+\t   \"\\n\"\n+\t   \"    git push %s HEAD:<name-of-remote-branch>\\n\");\n+\n static void setup_push_upstream(struct remote *remote, int simple)\n {\n \tstruct strbuf refspec = STRBUF_INIT;\n \tstruct branch *branch = branch_get(NULL);\n \tif (!branch)\n-\t\tdie(_(\"You are not currently on a branch.\\n\"\n-\t\t    \"To push the history leading to the current (detached HEAD)\\n\"\n-\t\t    \"state now, use\\n\"\n-\t\t    \"\\n\"\n-\t\t    \"    git push %s HEAD:<name-of-remote-branch>\\n\"),\n-\t\t    remote->name);\n+\t\tdie(_(message_detached_head_die), remote->name);\n \tif (!branch->merge_nr || !branch->merge || !branch->remote_name)\n \t\tdie(_(\"The current branch %s has no upstream branch.\\n\"\n \t\t    \"To push the current branch and set the remote as upstream, use\\n\"\n-- \n1.8.3.rc3.7.gc1ff30b\n"},{"id":"218078","messageId":"1369160600-22337-3-git-send-email-artagnon@gmail.com","threadId":"33888","inReplyTo":"1369160600-22337-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/3] push: fail early with detached HEAD and current","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-21T18:23:19Z","receivedAt":"2013-05-21T18:23:19Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Setting push.default to current adds the refspec \"HEAD\" for the\ntransport layer to handle.  If \"HEAD\" doesn't resolve to a branch (and\nsince no refspec rhs is specified), the push fails after some time with\na cryptic error message:\n\n  $ git push\n  error: unable to push to unqualified destination: HEAD\n  The destination refspec neither matches an existing ref on the remote nor\n  begins with refs/, and we are unable to guess a prefix based on the source ref.\n  error: failed to push some refs to 'git@github.com:artagnon/git'\n\nFail early with a nicer error message:\n\n  $ git push\n  fatal: 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 ram HEAD:<name-of-remote-branch>\n\nJust like in the upstream and simple cases.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/push.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex ef3aa97..a79038c 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -175,6 +175,8 @@ static void warn_unspecified_push_default_configuration(void)\n \n static void setup_default_push_refspecs(struct remote *remote)\n {\n+\tstruct branch *branch = branch_get(NULL);\n+\n \tswitch (push_default) {\n \tdefault:\n \tcase PUSH_DEFAULT_UNSPECIFIED:\n@@ -194,6 +196,8 @@ static void setup_default_push_refspecs(struct remote *remote)\n \t\tbreak;\n \n \tcase PUSH_DEFAULT_CURRENT:\n+\t\tif (!branch)\n+\t\t\tdie(_(message_detached_head_die), remote->name);\n \t\tadd_refspec(\"HEAD\");\n \t\tbreak;\n \n-- \n1.8.3.rc3.7.gc1ff30b\n"},{"id":"218080","messageId":"1369160600-22337-4-git-send-email-artagnon@gmail.com","threadId":"33888","inReplyTo":"1369160600-22337-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/3] push: don't push the volatile HEAD with current","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-21T18:23:20Z","receivedAt":"2013-05-21T18:23:20Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Since a push does not lock the \"HEAD\" ref, there is no guarantee that it\nwill remain the same for the entire operation of the push.  Practically,\nthis means that with push.default set to current:\n\n  # on branch push-current-head\n  $ git push\n  # on another terminal\n  $ git checkout master\n  # return to the first terminal\n  # the push tried to push master!\n\nAvoid this confusion by adding the branch that HEAD resolves to as the\nrefspec to push (as opposed to the literal \"HEAD\").  With this change,\nthe output of the push changes subtly from:\n\n  $ git push\n  ...\n   * [new branch]      HEAD -> push-current-head\n\nto:\n\n  $ git push\n  ...\n   * [new branch]      push-current-head -> push-current-head\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/push.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex a79038c..d819487 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -198,7 +198,7 @@ static void setup_default_push_refspecs(struct remote *remote)\n \tcase PUSH_DEFAULT_CURRENT:\n \t\tif (!branch)\n \t\t\tdie(_(message_detached_head_die), remote->name);\n-\t\tadd_refspec(\"HEAD\");\n+\t\tadd_refspec(branch->name);\n \t\tbreak;\n \n \tcase PUSH_DEFAULT_NOTHING:\n-- \n1.8.3.rc3.7.gc1ff30b\n"},{"id":"218082","messageId":"7v38tgno2k.fsf@alter.siamese.dyndns.org","threadId":"33888","inReplyTo":"1369160600-22337-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-21T19:13:39Z","receivedAt":"2013-05-21T19:13:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> There's still a lot to think about.\n\nIs there?  I do not think \"volatile\" is particularly a good\ndescription for this, but showing what is pushed as a concrete\nbranch name feels like a good improvement to me, at least in\nprinciple.\n\nI haven't picked them up, and I won't be picking them up today, as I\nsuspect this series may conflict with the pre-2.0 preparation and\n2.0 transition patches and I may end up having to fix conflicts\nunnecessarily (resolving is eventually needed before 2.0 happens,\nbut resolving them, or even having to worry about the possibility\nthat I may have to do so, do not have to steal time from me today).\n\nThanks.\n\n> [3/3] is the big itch: [1/2] and [2/2] are just setup patches.\n>\n> Ramkumar Ramachandra (3):\n>   push: factor out the detached HEAD error message\n>   push: fail early with detached HEAD and current\n>   push: don't push the volatile HEAD with current\n>\n>  builtin/push.c | 20 +++++++++++++-------\n>  1 file changed, 13 insertions(+), 7 deletions(-)\n"},{"id":"218086","messageId":"CALkWK0m3QP_eE14y8UoJ+HeFzkKM=1nVKAnYJcTrwQMp+Atphg@mail.gmail.com","threadId":"33888","inReplyTo":"7v38tgno2k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-21T19:52:45Z","receivedAt":"2013-05-21T19:52:45Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Is there?  I do not think \"volatile\" is particularly a good\n> description for this, but showing what is pushed as a concrete\n> branch name feels like a good improvement to me, at least in\n> principle.\n\nOkay.  I used \"volatile\", because push does not lock HEAD when the\noperation begins, even though it performs a super-late resolution (in\nthe transport-layer); HEAD is not guaranteed to remain invariant in\nthat time.  Suggest nicer wording?\n\n> I haven't picked them up, and I won't be picking them up today, as I\n> suspect this series may conflict with the pre-2.0 preparation and\n> 2.0 transition patches and I may end up having to fix conflicts\n> unnecessarily (resolving is eventually needed before 2.0 happens,\n> but resolving them, or even having to worry about the possibility\n> that I may have to do so, do not have to steal time from me today).\n\nWe're at 1.8.3; isn't it a little early to be thinking of 2.0?  Is it\nconflicting with jc/push-2.0-default-to-simple in pu?  I should\nre-send after this topic graduates to master in 2.0?\n\nI have no problems re-sending at a convenient time (provided you tell\nme how to determine that convenient time), but reviews don't have to\nwait: it's fresh in my memory now.\n"},{"id":"218087","messageId":"7vy5b8m75x.fsf@alter.siamese.dyndns.org","threadId":"33888","inReplyTo":"CALkWK0m3QP_eE14y8UoJ+HeFzkKM=1nVKAnYJcTrwQMp+Atphg@mail.gmail.com","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-21T20:04:10Z","receivedAt":"2013-05-21T20:04:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> We're at 1.8.3; isn't it a little early to be thinking of 2.0?  Is it\n> conflicting with jc/push-2.0-default-to-simple in pu?  I should\n> re-send after this topic graduates to master in 2.0?\n\nI implicitly asked you not to ask as I do not even have to worry\nabout conflicts ;-)  That does not mean I'll discard.  Please ping\nme back later after 1.8.3 if you do not see it in my tree anywhere;\nI may have found time to splice these _before_ the 2.0 patch (which\nis where it should come, I think) by then.\n"},{"id":"218090","messageId":"7vppwkm682.fsf@alter.siamese.dyndns.org","threadId":"33888","inReplyTo":"CALkWK0m3QP_eE14y8UoJ+HeFzkKM=1nVKAnYJcTrwQMp+Atphg@mail.gmail.com","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-21T20:24:29Z","receivedAt":"2013-05-21T20:24:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> Is there?  I do not think \"volatile\" is particularly a good\n>> description for this, but showing what is pushed as a concrete\n>> branch name feels like a good improvement to me, at least in\n>> principle.\n>\n> Okay.  I used \"volatile\", because push does not lock HEAD when the\n> operation begins, even though it performs a super-late resolution (in\n> the transport-layer); HEAD is not guaranteed to remain invariant in\n> that time.  Suggest nicer wording?\n\nIn general, when a command is working in your repository with a\nworking tree, we do not make any such promise that it keeps\noperationg normally when you pull the rug under its feet from\nanother terminal (\"git checkout maint\" running at the same time \"git\npull\" would not have a chance to work correctly).  Some are safe\n(like \"git push\" racing with \"git checkout maint\" that would not\nhave to look at what the current branch is) and some are not (like\n\"git push github\" racing with \"git checkout maint && git push\norigin HEAD:preview\").\n\nI view the value of this topic purely as \"showing a real branch name\nwhen that is what we actually pushed is a lot more preferrable than\nshowing HEAD, especially because the user may see it in the terminal\nscrollback buffer hours after it happened\".  Explaining this patch\nas \"we avoid issues from simultaneously flipping HEAD by resolving\nearly\" gives a false sense of security to the reader, as \"early\" has\nto happen early enough for the patch to really avoid the issue, but\nyou are not in control of when the user does that flipping in the\nother terminal.\n"},{"id":"218094","messageId":"CALkWK0np7o0eH8ZsWQSwk1Cdwnnpj5B==gS8kAE+OkTskASOsQ@mail.gmail.com","threadId":"33888","inReplyTo":"7vppwkm682.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-21T21:09:44Z","receivedAt":"2013-05-21T21:09:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> In general, when a command is working in your repository with a\n> working tree, we do not make any such promise that it keeps\n> operationg normally when you pull the rug under its feet from\n> another terminal (\"git checkout maint\" running at the same time \"git\n> pull\" would not have a chance to work correctly).  Some are safe\n> (like \"git push\" racing with \"git checkout maint\" that would not\n> have to look at what the current branch is) and some are not (like\n> \"git push github\" racing with \"git checkout maint && git push\n> origin HEAD:preview\").\n\nMy current set of expectations look like this:\n\n1. Commands that operate on a worktree (merge family) do not lock the\nworktree before operating on it.  These are fast synchronous commands\n(operating on recent hot-cached stuff), and there is no _utility_ in a\nperfectly atomic worktree operation.  Might be an interesting\ntheoretical exercise to merge-trees in memory, but I have absolutely\nno interest in such a problem.\n\n2. Commands that operate on refs, the-index, and intermediate states\n(update-ref, update-index, rebase-state family) do so atomically using\nthe lockfile API.  The atomicity is important here, because we don't\nwant a higher-level command to half-fail.  And it's trivial to\nimplement.\n\n3. Commands that operate only on the object store are guaranteed not\nto race as the object store itself is read-only (hash-object,\ncommit-tree family; gc being the exception).  This is very important\nfor concurrent access in a server implementation.\n\nIn 3 out of the 4 push.default states, push is category (3).\npush.default = current is a special case, and should try not to break\nend-user expectation even if it involves resolving HEAD.\n\n> I view the value of this topic purely as \"showing a real branch name\n> when that is what we actually pushed is a lot more preferrable than\n> showing HEAD, especially because the user may see it in the terminal\n> scrollback buffer hours after it happened\".  Explaining this patch\n> as \"we avoid issues from simultaneously flipping HEAD by resolving\n> early\" gives a false sense of security to the reader, as \"early\" has\n> to happen early enough for the patch to really avoid the issue, but\n> you are not in control of when the user does that flipping in the\n> other terminal.\n\nWhy should I lie in the patch?  The terminal flipping was a very big\nitch I had, and the patch fixes exactly that issue.  Showing the real\nbranch name was an unintended side-effect.\n\nI just said \"early\" and showed a nice end-user example in which it\nworks, not \"theoretically impossible to race with\".  Better wording\n(while not lying about the motivation behind the patch)?\n"},{"id":"218180","messageId":"7v7girks4s.fsf@alter.siamese.dyndns.org","threadId":"33888","inReplyTo":"CALkWK0np7o0eH8ZsWQSwk1Cdwnnpj5B==gS8kAE+OkTskASOsQ@mail.gmail.com","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-22T14:26:27Z","receivedAt":"2013-05-22T14:26:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Why should I lie in the patch?  The terminal flipping was a very big\n> itch I had, and the patch fixes exactly that issue.  Showing the real\n> branch name was an unintended side-effect.\n>\n> I just said \"early\" and showed a nice end-user example in which it\n> works, not \"theoretically impossible to race with\".  Better wording\n> (while not lying about the motivation behind the patch)?\n\nThe patch may have been done by a wrong motivation, in that it does\nnot fundamentally \"fix\" the itch.  The particular \"itch\" is not\nsomething we are going to promise to the end users, ever, anyway.\n\nThe only remaining justification for the change is, even though the\nuser cannot _safely_ flip the branches with this patch, it improves\nthe output.\n\nThat does not make the patch wrong, but the original motivation is\nan irrelevant, lost cause.  \"Even though this started to address an\nitch, the patch does not fundamentally fix that itch at all.\" may be\na honest statement to make, but that alone is not a justification to\nhave this change.\n\nThe \"side effect\" is the only improvement this patch gives us, and\nthat happens to be a good enough justification.  At that point, is\nthe original itch the patch does not correctly address even worth\nmentioning?  I answered \"no\" to that question.\n\nSo I do not think you are lying anything.\n"},{"id":"218181","messageId":"CALkWK0kXonM+J92KHs+dW31A_kZ8h9jNqwYn8AqT_V+GJhe_vg@mail.gmail.com","threadId":"33888","inReplyTo":"7v7girks4s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-22T14:43:37Z","receivedAt":"2013-05-22T14:43:37Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> The patch may have been done by a wrong motivation, in that it does\n> not fundamentally \"fix\" the itch.  The particular \"itch\" is not\n> something we are going to promise to the end users, ever, anyway.\n\nJust out of curiosity, is it possible to write a correct fix at all?\nEven if the first statement in do_push() locks the HEAD ref, it's not\nenough: the program may be wading around in git.c/setup.c when I\nswitch terminals and change HEAD, right?\n\nSo, our position on the matter is: no git command makes any guarantees\nwith respect to races, correct?\n"},{"id":"218193","messageId":"7vppwjj5ws.fsf@alter.siamese.dyndns.org","threadId":"33888","inReplyTo":"CALkWK0kXonM+J92KHs+dW31A_kZ8h9jNqwYn8AqT_V+GJhe_vg@mail.gmail.com","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-22T17:11:47Z","receivedAt":"2013-05-22T17:11:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> The patch may have been done by a wrong motivation, in that it does\n>> not fundamentally \"fix\" the itch.  The particular \"itch\" is not\n>> something we are going to promise to the end users, ever, anyway.\n>\n> Just out of curiosity, is it possible to write a correct fix at all?\n\nIs there anything to \"fix\" in the first place, you have to wonder.\n\nYour \"git push there HEAD:master\" would roughly do the following:\n\n    (1) read HEAD to learn what commit you are pushing;\n\n    (2) contact the other side and find where there tips are;\n\n    (3) send a packfile that should be enough for the other side to\n        have complete history leading to the commit you read in (1);\n\n    (4) tell the other side to update its 'master' to the commit you\n        read in (1).\n\nIf you drop step (1) and replace \"the commit you read in (1)\" in\nsteps (3) and (4) with \"the commit you see in HEAD at this point by\nre-reading HEAD\", then such a \"git push\" that races with something\nelse you do in your other terminal may break---you can cause it to\nsee different commits at steps (3) and (4), potentially getting the\nother side out of sync (but the receiving end does an independent\nconnectivity check so your push will likely to be rejected).\n\nAnd the fix to such a breakage is to structure the code like the\nabove four steps to make it race-free.\n\nIf I understand your example correctly, you are talking about a\nquite different thing.  \"git push there HEAD:master\" racing with\nyour other terminal that changes the HEAD sees different HEAD\ndepending on the order:\n\n    (a) if the other terminal changes the HEAD first, step (1) will\n        see that updated HEAD; or\n\n    (b) if the step (1) reads HEAD before you change it in the other\n        terminal, it will see the original HEAD.\n\nBut that is very much to be expected, isn't it?  It sounds similar\nto\n\n    I have \"largedir\" I want to get rid of, but there is a directory\n    I want to save, \"largedir/precious\", in it, so I do\n\n        cp -R largedir/precious precious\n\n    and then run 'rm -rf largedir' in another terminal in parallel.\n\nIs there anything to fix?\n"},{"id":"218238","messageId":"CALkWK0mfGWA3aBntQQU5fkCJDPx+MUOScvMRJjS+JzjNucFzPw@mail.gmail.com","threadId":"33888","inReplyTo":"7vppwjj5ws.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-23T07:55:55Z","receivedAt":"2013-05-23T07:55:55Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n>     I have \"largedir\" I want to get rid of, but there is a directory\n>     I want to save, \"largedir/precious\", in it, so I do\n>\n>         cp -R largedir/precious precious\n>\n>     and then run 'rm -rf largedir' in another terminal in parallel.\n\nI would argue that there is something to \"fix\", but that fix involves\nmaking the cp a purely atomic operation which is super-complicated,\nand totally not worth it.  Would you _not_ like the above example to\nwork?  Then how can you say that there's nothing to be fixed?\n\nConsider a slightly different example: I rename a file while having an\nactive file handle open in a process that's reading the file.  Will\nthe rename fail or will the fread() in the process fail?  Nope, both\nwork fine.  Replace \"rename\" with \"remove\", and we still have the same\nanswer.  Ofcourse there are no guarantees: I can start up another\nprocess to overwrite the sectors corresponding to that file's data\nwith zeros; unless the complete file is there in the kernel buffer, a\nread() will eventually end up slurping in the zeros (or fail?), right?\n It's just that it works in practice.\n\nYet another example: I have a terminal pointing to a directory, and I\nremove that directory in another terminal.  When I switch back to the\noriginal terminal, I can't cd .., because getcwd() fails.  This has\nannoyed me endlessly in practice, and I would really like to fix this\nif I can.\n\nDon't accept the way things are, and assume that there's nothing to be\n\"fixed\".  In my opinion, if something about a piece of software annoys\nyou, there is always something to fix.  It just depends on what _can_\nbe fixed in a reasonable amount of time with a good engineering\nsolution.  There's no need to go to the other extreme: I'm not\ninterested in rewriting the whole operating system in Haskell and\nproviding theoretical guarantees for everything.\n\nComing back to our push example, I don't see why you think HEAD is\nspecial: I could even say git push master and expect it to race with\nan update-ref.  But nobody is complaining about that: if someone does\ncomplain, I would seriously consider copying master to PUSH_HEAD early\n(and push that).  With HEAD, however, someone is complaining (namely,\nme): pushing usually means that I've finished working on that branch,\nand want to switch to another branch and continue working.  Why should\nI have to wait for the push to complete?  I've hit this bug several\ntimes (from terminal as well as Magit), and this patch fixes the\nproblem for me in practice.\n\nThat said, I agree that my patch does not guarantee anything (and I\nwill modify my commit message to clarify this).  I'm just expressing\nmy opinion on the issue of \"fixing problems\".\n"},{"id":"218241","messageId":"20130523085359.GA23933@inner.h.apk.li","threadId":"33888","inReplyTo":"CALkWK0mfGWA3aBntQQU5fkCJDPx+MUOScvMRJjS+JzjNucFzPw@mail.gmail.com","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Andreas Krey","fromEmail":"a.krey@gmx.de","sentAt":"2013-05-23T08:53:59Z","receivedAt":"2013-05-23T08:53:59Z","isPatch":true,"sender":{"key":"a.krey@gmx.de","avatar":"https://avatars.githubusercontent.com/u/37810?v=4"},"body":"On Thu, 23 May 2013 13:25:55 +0000, Ramkumar Ramachandra wrote:\n> Junio C Hamano wrote:\n> >     I have \"largedir\" I want to get rid of, but there is a directory\n> >     I want to save, \"largedir/precious\", in it, so I do\n> >\n> >         cp -R largedir/precious precious\n> >\n> >     and then run 'rm -rf largedir' in another terminal in parallel.\n\n'mv largedir/precious precious; rm -rf largedir'? No race here.\n\n...\n> Consider a slightly different example: I rename a file while having an\n> active file handle open in a process that's reading the file.  Will\n> the rename fail or will the fread() in the process fail?  Nope, both\n> work fine.  Replace \"rename\" with \"remove\", and we still have the same\n> answer.  Ofcourse there are no guarantees: I can start up another\n> process to overwrite the sectors corresponding to that file's data\n> with zeros; unless the complete file is there in the kernel buffer, a\n> read() will eventually end up slurping in the zeros (or fail?), right?\n\nOh, there are guarantees, they just don't include the case where you\ntake a shotgun to the disk. (Or do it on an nfs mount and delete the\nfile from another machine.)\n\n...\n\nAndreas\n\n-- \n\"Totally trivial. Famous last words.\"\nFrom: Linus Torvalds <torvalds@*.org>\nDate: Fri, 22 Jan 2010 07:29:21 -0800\n"},{"id":"218267","messageId":"7vmwrlhi0h.fsf@alter.siamese.dyndns.org","threadId":"33888","inReplyTo":"20130523085359.GA23933@inner.h.apk.li","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-23T14:45:34Z","receivedAt":"2013-05-23T14:45:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Krey <a.krey@gmx.de> writes:\n\n> On Thu, 23 May 2013 13:25:55 +0000, Ramkumar Ramachandra wrote:\n>> Junio C Hamano wrote:\n>> >     I have \"largedir\" I want to get rid of, but there is a directory\n>> >     I want to save, \"largedir/precious\", in it, so I do\n>> >\n>> >         cp -R largedir/precious precious\n>> >\n>> >     and then run 'rm -rf largedir' in another terminal in parallel.\n>\n> 'mv largedir/precious precious; rm -rf largedir'? No race here.\n\nYeah, but 'cp -R largedir/precious precious; rm -fr largedir' is the\nsame thing.  Ram's original issue was \"I do something else in\nanother terminal without waiting for the first command to finish\".\n\nEven with 'mv', between the time the main in mv starts and the\nprocess finally issues rename(2) on the directory, you can start\nrunning what competes and interferes with it in another terminal,\nso it does not fundamentally change anything, I would have to say.\n"},{"id":"218268","messageId":"7vip29hhuj.fsf@alter.siamese.dyndns.org","threadId":"33888","inReplyTo":"CALkWK0mfGWA3aBntQQU5fkCJDPx+MUOScvMRJjS+JzjNucFzPw@mail.gmail.com","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-23T14:49:08Z","receivedAt":"2013-05-23T14:49:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> I would argue that there is something to \"fix\", but that fix involves\n> making the cp a purely atomic operation which is super-complicated,\n> and totally not worth it.  Would you _not_ like the above example to\n> work?\n\nNo.  I do not think I like the above example to \"work\", at all.\n\nI know we live in the real world, and I do not want to see such\nserializing implementations of cp or rm that try to \"fix\", as it\nwill interfere my everyday use where such a race does not matter.\n"},{"id":"218301","messageId":"20130523192725.GS9448@inner.h.apk.li","threadId":"33888","inReplyTo":"7vmwrlhi0h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] Fixing volatile HEAD in push.default = current","fromName":"Andreas Krey","fromEmail":"a.krey@gmx.de","sentAt":"2013-05-23T19:27:25Z","receivedAt":"2013-05-23T19:27:25Z","isPatch":true,"sender":{"key":"a.krey@gmx.de","avatar":"https://avatars.githubusercontent.com/u/37810?v=4"},"body":"On Thu, 23 May 2013 07:45:34 +0000, Junio C Hamano wrote:\n...\n> Even with 'mv', between the time the main in mv starts and the\n> process finally issues rename(2) on the directory, you can start\n> running what competes and interferes with it in another terminal,\n> so it does not fundamentally change anything, I would have to say.\n\nNot fundamentally, it's just that the mv is fast enough you wouldn't\neven think of switch to another term/backgrounding it.\n\nAndreas\n\n-- \n\"Totally trivial. Famous last words.\"\nFrom: Linus Torvalds <torvalds@*.org>\nDate: Fri, 22 Jan 2010 07:29:21 -0800\n"}]}