{"thread":{"id":"37896","subject":"[PATCH 0/2] Support updating working trees when pushing into non-bare repos","startedAt":"2014-11-07T13:58:12Z","lastAt":"2014-12-02T17:23:13Z","messageCount":71,"participants":["Johannes Schindelin","Junio C Hamano","Jeff King","brian m. carlson","Jens Lehmann","Michael J Gruber"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"251455","messageId":"cover.1415368490.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":null,"subject":"[PATCH 0/2] Support updating working trees when pushing into non-bare repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-07T13:58:12Z","receivedAt":"2014-11-07T13:58:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"A few years ago, this developer was convinced that it was a bad idea to\nauto-update working directories when pushing into the current branch, and\nthat an excellent way to prove this was to implement that feature. To his\nsurprise, it turned out to be the one thing he misses most in upstream Git.\n\nSo here goes: this patch series adds support for two new\nreceive.denyCurrentBranch settings: one to update the working directory\n(which must be clean, i.e. there must not be any uncommitted changes) when\npushing into the current branch, the other setting detaches the HEAD\ninstead.\n\nThe scenario in which in particular the 'updateInstead' setting became a\nboon in this developer's daily work is a multi-laptop one, where working\ndirectories need to be updated between computers without a hassle.\n\nJohannes Schindelin (2):\n Add a few more values for receive.denyCurrentBranch\n Let deny.currentBranch=updateInstead ignore submodules\n\n Documentation/config.txt | 5 +++++\n builtin/receive-pack.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t5516-fetch-push.sh | 36 ++++++++++++++++++++++++++++++\n 3 files changed, 97 insertions(+), 2 deletions(-)\n\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"251456","messageId":"f82aedcb632571d0b756d62c58479c0aab35b026.1415368490.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":"cover.1415368490.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/2] Add a few more values for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-07T13:58:17Z","receivedAt":"2014-11-07T13:58:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Under certain circumstances, it makes a *lot* of sense to allow pushing\ninto the current branch. For example, when two machines with different\nOperating Systems are required for testing, it makes much more sense to\nsynchronize between working directories than having to go via a third\nserver.\n\nUnder different circumstances, the working directory needs to be left\nuntouched, for example when a bunch of VMs need to be shut down to save\nRAM and one needs to push everything out into the host's non-bare\nrepositories quickly.\n\nThis change supports both workflows by offering two new values for the\ndenyCurrentBranch setting:\n\n'updateInstead':\n\tUpdate the working tree accordingly, but refuse to do so if there\n\tare any uncommitted changes.\n\n'detachInstead':\n\tDetach the HEAD, thereby keeping currently checked-out revision,\n\tindex and working directory unchanged.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/config.txt | 5 +++++\n builtin/receive-pack.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t5516-fetch-push.sh | 36 ++++++++++++++++++++++++++++++\n 3 files changed, 97 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e8dd76d..6fc0d6d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2129,6 +2129,11 @@ receive.denyCurrentBranch::\n \tprint a warning of such a push to stderr, but allow the push to\n \tproceed. If set to false or \"ignore\", allow such pushes with no\n \tmessage. Defaults to \"refuse\".\n++\n+There are two more options: \"updateInstead\" which will update the working\n+directory (must be clean) if pushing into the current branch, and\n+\"detachInstead\" which will leave the working directory untouched, detaching\n+the HEAD so it does not need to change.\n \n receive.denyNonFastForwards::\n \tIf set to true, git-receive-pack will deny a ref update which is\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 32fc540..be4172f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -26,7 +26,9 @@ enum deny_action {\n \tDENY_UNCONFIGURED,\n \tDENY_IGNORE,\n \tDENY_WARN,\n-\tDENY_REFUSE\n+\tDENY_REFUSE,\n+\tDENY_UPDATE_INSTEAD,\n+\tDENY_DETACH_INSTEAD,\n };\n \n static int deny_deletes;\n@@ -120,7 +122,12 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(var, \"receive.denycurrentbranch\")) {\n-\t\tdeny_current_branch = parse_deny_action(var, value);\n+\t\tif (value && !strcasecmp(value, \"updateinstead\"))\n+\t\t\tdeny_current_branch = DENY_UPDATE_INSTEAD;\n+\t\telse if (value && !strcasecmp(value, \"detachinstead\"))\n+\t\t\tdeny_current_branch = DENY_DETACH_INSTEAD;\n+\t\telse\n+\t\t\tdeny_current_branch = parse_deny_action(var, value);\n \t\treturn 0;\n \t}\n \n@@ -730,6 +737,44 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \treturn 0;\n }\n \n+static void merge_worktree(unsigned char *sha1)\n+{\n+\tconst char *update_refresh[] = {\n+\t\t\"update-index\", \"--refresh\", NULL\n+\t};\n+\tconst char *read_tree[] = {\n+\t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n+\t};\n+\tstruct child_process child;\n+\tstruct strbuf git_env = STRBUF_INIT;\n+\tconst char *env[2];\n+\n+\tif (is_bare_repository())\n+\t\tdie (\"denyCurrentBranch = updateInstead needs a worktree\");\n+\n+\tstrbuf_addf(&git_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\tenv[0] = git_env.buf;\n+\tenv[1] = NULL;\n+\n+\tmemset(&child, 0, sizeof(child));\n+\tchild.argv = update_refresh;\n+\tchild.env = env;\n+\tchild.dir = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n+\tchild.stdout_to_stderr = 1;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child))\n+\t\tdie (\"Could not refresh the index\");\n+\n+\tchild.argv = read_tree;\n+\tchild.no_stdin = 1;\n+\tchild.no_stdout = 1;\n+\tchild.stdout_to_stderr = 0;\n+\tif (run_command(&child))\n+\t\tdie (\"Could not merge working tree with new HEAD. Good luck.\");\n+\n+\tstrbuf_release(&git_env);\n+}\n+\n static const char *update(struct command *cmd, struct shallow_info *si)\n {\n \tconst char *name = cmd->ref_name;\n@@ -760,6 +805,13 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\tif (deny_current_branch == DENY_UNCONFIGURED)\n \t\t\t\trefuse_unconfigured_deny();\n \t\t\treturn \"branch is currently checked out\";\n+\t\tcase DENY_UPDATE_INSTEAD:\n+\t\t\tmerge_worktree(new_sha1);\n+\t\t\tbreak;\n+\t\tcase DENY_DETACH_INSTEAD:\n+\t\t\tupdate_ref(\"push into current branch (detach)\", \"HEAD\",\n+\t\t\t\told_sha1, NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n+\t\t\tbreak;\n \t\t}\n \t}\n \n@@ -788,6 +840,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\t\trefuse_unconfigured_deny_delete_current();\n \t\t\t\trp_error(\"refusing to delete the current branch: %s\", name);\n \t\t\t\treturn \"deletion of the current branch prohibited\";\n+\t\t\tdefault:\n+\t\t\t\tdie (\"Invalid denyDeleteCurrent setting\");\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f4da20a..3981d1b 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1330,4 +1330,40 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '\n \t)\n '\n \n+test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n+\tgit push testrepo master &&\n+\t(cd testrepo &&\n+\t\tgit reset --hard &&\n+\t\tgit config receive.denyCurrentBranch updateInstead\n+\t) &&\n+\ttest_commit third path2 &&\n+\tgit push testrepo master &&\n+\ttest $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&\n+\ttest third = \"$(cat testrepo/path2)\" &&\n+\t(cd testrepo &&\n+\t\tgit update-index --refresh &&\n+\t\tgit diff-files --quiet &&\n+\t\tgit diff-index --cached HEAD --\n+\t)\n+'\n+\n+test_expect_success 'receive.denyCurrentBranch = detachInstead' '\n+\t(cd testrepo &&\n+\t\tgit reset --hard &&\n+\t\tgit config receive.denyCurrentBranch detachInstead\n+\t) &&\n+\tOLDHEAD=$(cd testrepo && git rev-parse HEAD) &&\n+\ttest_commit fourth path2 &&\n+\ttest fourth = \"$(cat path2)\" &&\n+\tgit push testrepo master &&\n+\ttest $OLDHEAD = $(cd testrepo && git rev-parse HEAD) &&\n+\ttest fourth != \"$(cat testrepo/path2)\" &&\n+\t(cd testrepo &&\n+\t\ttest_must_fail git symbolic-ref HEAD &&\n+\t\tgit update-index --refresh &&\n+\t\tgit diff-files --quiet &&\n+\t\tgit diff-index --cached HEAD --\n+\t)\n+'\n+\n test_done\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"251457","messageId":"84dba8872922da96e99953eea0ccff5f5af9dd4a.1415368490.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":"cover.1415368490.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-07T13:58:22Z","receivedAt":"2014-11-07T13:58:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"They are not affected by the update anyway.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/receive-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex be4172f..4ba51df 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n static void merge_worktree(unsigned char *sha1)\n {\n \tconst char *update_refresh[] = {\n-\t\t\"update-index\", \"--refresh\", NULL\n+\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n \t};\n \tconst char *read_tree[] = {\n \t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"251471","messageId":"xmqqvbmqg81g.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"f82aedcb632571d0b756d62c58479c0aab35b026.1415368490.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-07T18:49:47Z","receivedAt":"2014-11-07T18:49:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Under certain circumstances, it makes a *lot* of sense to allow pushing\n> into the current branch. For example, when two machines with different\n> Operating Systems are required for testing, it makes much more sense to\n> synchronize between working directories than having to go via a third\n> server.\n\nEven if you do not have a third server, each time you decide to do\nsuch a push, you have a single source of truth, i.e. the repository\nyou are pushing from, so instead of doing that push, all the others\ncould instead pull from that single source of truth. In that sense,\neven though I wouldn't say it is wrong to use \"push\" in the opposite\ndirection for this synchronization, I have to say that the above is\nnot a very strong argument. It certainly does not deserve \"*lot*\"\nin bold font ;-)\n\n> Under different circumstances, the working directory needs to be left\n> untouched, for example when a bunch of VMs need to be shut down to save\n> RAM and one needs to push everything out into the host's non-bare\n> repositories quickly.\n\nFor this use case, if you assume that the tips of branches in the\nrepositories on \"a bunch of VMs\" could be pointing at different\ncommits (i.e. each of them has acquired more work without\ncoordination), you are risking lossage by pushing into refs/heads/,\nnot refs/remotes/vm$n/, aren't you? When you want to save things\naway \"quickly\", you do not want to be worried about a push from VM1\nstomping on what was stored from VM0, and using separate remotes,\ni.e. refs/remotes/vm$n/, has been the recommended way to do so. So\nthis is not a very strong argument, either.\n\nDoing things only within refs/heads/ without using any refs/remotes/\nhowever is a handy option, especially for those who know what they\nare doing, and I do not have problem with an effort to help\nimproving such a workflow. It just feels unnecessary, and inviting\nmistakes, to try to mix that with checked out branches.\n\nFor example, if these VMs are mostly for building and making fix-up\ncommits, I would imagine another possible flow might be:\n\n * Each repository on these bochs would have refs/heads/* but not\n refs/remotes/* and is on detached HEAD.\n\n * Pushing into these repositories will trigger a hook that performs\n a checkout of the commit at the tip of the updated branch to the\n detached HEAD, and use of \"git checkout\" in that hook will\n prevent lossage of local changes automatically.\n\n * After making fix-up commits on one VMs repository, you would\n first locally push HEAD:$branch to update the refs/heads/$branch\n and propagate that to others by pushing $branch.\n\nNow, with \"updateInstead\", you can keep the target branch checked\nout, instead of detaching HEAD at the tip of it and having a hook to\nrun \"git checkout\", and automatically update the working tree. The\nworkflow would become:\n\n * Each repository on these bochs would have refs/heads/* but not\n refs/remotes/*. Each have a branch checked out and some or all\n may be on the same branch.\n\n * Pushing into these repositories will trigger updateInstead to\n update the working tree in a safe way.\n\n * After making fix-up commits on one VMs repository, propagate that\n change to other VMs by pushing the branch out.\n\nwhich is a definite improvement.\n\nExplained in that way, I would say \"updateInstead\" is a welcome\naddition. The documentation would probably need to explain an\nexpected use (something like what I gave above), so that this can be\nuseful not only by those who know what they are doing, though.\n\nI do not think of a good justification of detachInstead offhand, but\nyou must have thought things through a lot more than I did, so you\ncan come up with a work flow description that is more usable by mere\nmortals to justify that mode. Without such a description to justify\nwhy detachInstead makes sense, for example, I cannot even answer\nthis simple question:\n\n Would it make sense to have a third mode, \"check out if the\n working tree is clean, detach otherwise\"?\n\n> This change supports both workflows by offering two new values for the\n> denyCurrentBranch setting:\n>\n> 'updateInstead':\n> \tUpdate the working tree accordingly, but refuse to do so if there\n> \tare any uncommitted changes.\n>\n> 'detachInstead':\n> \tDetach the HEAD, thereby keeping currently checked-out revision,\n> \tindex and working directory unchanged.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> Documentation/config.txt | 5 +++++\n> builtin/receive-pack.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++--\n> t/t5516-fetch-push.sh | 36 ++++++++++++++++++++++++++++++\n> 3 files changed, 97 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index e8dd76d..6fc0d6d 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2129,6 +2129,11 @@ receive.denyCurrentBranch::\n> \tprint a warning of such a push to stderr, but allow the push to\n> \tproceed. If set to false or \"ignore\", allow such pushes with no\n> \tmessage. Defaults to \"refuse\".\n> ++\n> +There are two more options: \"updateInstead\" which will update the working\n> +directory (must be clean) if pushing into the current branch, and\n> +\"detachInstead\" which will leave the working directory untouched, detaching\n> +the HEAD so it does not need to change.\n> \n> receive.denyNonFastForwards::\n> \tIf set to true, git-receive-pack will deny a ref update which is\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 32fc540..be4172f 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -26,7 +26,9 @@ enum deny_action {\n> \tDENY_UNCONFIGURED,\n> \tDENY_IGNORE,\n> \tDENY_WARN,\n> -\tDENY_REFUSE\n> +\tDENY_REFUSE,\n> +\tDENY_UPDATE_INSTEAD,\n> +\tDENY_DETACH_INSTEAD,\n\nDon't add trailing comma after the last element of enum.\n\n> @@ -730,6 +737,44 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n> \treturn 0;\n> }\n> \n> +static void merge_worktree(unsigned char *sha1)\n> +{\n> +\tconst char *update_refresh[] = {\n> +\t\t\"update-index\", \"--refresh\", NULL\n> +\t};\n> +\tconst char *read_tree[] = {\n> +\t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n> +\t};\n> +\tstruct child_process child;\n> +\tstruct strbuf git_env = STRBUF_INIT;\n> +\tconst char *env[2];\n> +\n> +\tif (is_bare_repository())\n> +\t\tdie (\"denyCurrentBranch = updateInstead needs a worktree\");\n\nWhy have extra SP only after \"die\" but not other function names?\n\n> +\tstrbuf_addf(&git_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n> +\tenv[0] = git_env.buf;\n> +\tenv[1] = NULL;\n\nDoesn't child.env have managed argv_array these days?\n\n> +\tmemset(&child, 0, sizeof(child));\n> +\tchild.argv = update_refresh;\n> +\tchild.env = env;\n> +\tchild.dir = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n> +\tchild.stdout_to_stderr = 1;\n> +\tchild.git_cmd = 1;\n> +\tif (run_command(&child))\n> +\t\tdie (\"Could not refresh the index\");\n> +\n> +\tchild.argv = read_tree;\n> +\tchild.no_stdin = 1;\n> +\tchild.no_stdout = 1;\n> +\tchild.stdout_to_stderr = 0;\n> +\tif (run_command(&child))\n> +\t\tdie (\"Could not merge working tree with new HEAD. Good luck.\");\n\nDrop \"Good luck.\" and replace it with a more useful info. At least,\ntell the user what state you left her repository in by this botched\nattempt, so that she can manually recover.\n\nI am guessing that you have already updated HEAD, you are not\ntelling her what commit the index and the working tree is originally\nbased on (which would be very useful to know for manual recovery\nfrom this state), but you haven't touched either the index or the\nworking tree at this point in the code.\n\nOther than that, I like what this \"updateInstead\" code path does.\n\nThanks.\n"},{"id":"251474","messageId":"alpine.DEB.1.00.1411071958020.990@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqvbmqg81g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-07T18:58:23Z","receivedAt":"2014-11-07T18:58:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 7 Nov 2014, Junio C Hamano wrote:\n\n> [...]\n\nI will address your concerns after the weekend.\n\nCiao,\nJohannes\n"},{"id":"251477","messageId":"xmqqh9yag6mt.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"84dba8872922da96e99953eea0ccff5f5af9dd4a.1415368490.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-07T19:20:10Z","receivedAt":"2014-11-07T19:20:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> They are not affected by the update anyway.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/receive-pack.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index be4172f..4ba51df 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n> static void merge_worktree(unsigned char *sha1)\n> {\n> \tconst char *update_refresh[] = {\n> -\t\t\"update-index\", \"--refresh\", NULL\n> +\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n> \t};\n> \tconst char *read_tree[] = {\n> \t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n\nI suspect that you did not squash this into 1/2 on purpose, and I am\nguessing the reason is because you were unsure what should happen\nwhen there were differences in submodules' working trees (otherwise,\nyou would have simply squashed without \"oops it was a thinko to\nforget passing this option\" as a separate patch). I am not sure\neither.\n\nBy the way, if the expected use case of updateInstead is what I\noutlined in the previous message, would it make more sense not to\nfail with \"update-index --refresh\" failure (i.e. the working tree\nfiles have no changes since the index)?\n\nThinking about it a bit more, checking with \"update-index --refresh\"\nfeels doubly wrong. You not just want the working tree files to be\npristine with respect to the index, but also you do not want to see\nany change between the index and the original HEAD, i.e.\n\n\t$ git reset --hard && echo >>Makefile ; git add Makefile\n $ git update-index --refresh ; echo $?\n 0\n\nthis is not a good state from which you would want to update the\nworking tree.\n\nWouldn't the two-tree form \"read-tree -u -m\" that is the equivalent\nto branch switching do a sufficient check?\n\nAlso, regarding the new calls to die() in the main patch, shouldn't\nthey just be returning the error reason in string form, just like\nDENY_REFUSE returns \"branch is currently checked out\" to signal a\npush failure to the caller?\n"},{"id":"251556","messageId":"20141108111855.GA21620@peff.net","threadId":"37896","inReplyTo":"f82aedcb632571d0b756d62c58479c0aab35b026.1415368490.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-08T11:18:55Z","receivedAt":"2014-11-08T11:18:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 07, 2014 at 02:58:17PM +0100, Johannes Schindelin wrote:\n\n> Under certain circumstances, it makes a *lot* of sense to allow pushing\n> into the current branch. For example, when two machines with different\n> Operating Systems are required for testing, it makes much more sense to\n> synchronize between working directories than having to go via a third\n> server.\n\nFWIW, I do this without a third server (and without resorting to pull),\nwith:\n\n host1$ git push host2 master:refs/remotes/host1/master\n host2$ git merge host1/master\n\nYou can even set up a push refspec to make \"git push host2\" do the right\nthing.\n\nThat being said, I do like the premise of your patch, as it eliminates\nthe extra step on the remote side (which is not that big a deal in\nitself, but when you realize that host2 _did_ have some changes on it,\nthen you end up doing the merge there, when in general I'd prefer to do\nall the work on host1 via \"git pull\").\n\n-Peff\n"},{"id":"251566","messageId":"20141108184832.GA167774@vauxhall.crustytoothpaste.net","threadId":"37896","inReplyTo":"20141108111855.GA21620@peff.net","subject":"Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-11-08T18:48:32Z","receivedAt":"2014-11-08T18:48:32Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Nov 08, 2014 at 06:18:55AM -0500, Jeff King wrote:\n> On Fri, Nov 07, 2014 at 02:58:17PM +0100, Johannes Schindelin wrote:\n> \n> > Under certain circumstances, it makes a *lot* of sense to allow pushing\n> > into the current branch. For example, when two machines with different\n> > Operating Systems are required for testing, it makes much more sense to\n> > synchronize between working directories than having to go via a third\n> > server.\n> \n> FWIW, I do this without a third server (and without resorting to pull),\n> with:\n> \n> host1$ git push host2 master:refs/remotes/host1/master\n> host2$ git merge host1/master\n> \n> You can even set up a push refspec to make \"git push host2\" do the right\n> thing.\n\nI do something similar, but it's inconvenient when the repo you're\npushing into is $HOME, since you have to type something like \"exec zsh\n-l\" in order to fix things up.\n\n> That being said, I do like the premise of your patch, as it eliminates\n> the extra step on the remote side (which is not that big a deal in\n> itself, but when you realize that host2 _did_ have some changes on it,\n> then you end up doing the merge there, when in general I'd prefer to do\n> all the work on host1 via \"git pull\").\n\nI agree. This is very useful.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"251618","messageId":"545F9990.9050505@web.de","threadId":"37896","inReplyTo":"xmqqh9yag6mt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2014-11-09T16:42:56Z","receivedAt":"2014-11-09T16:42:56Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 07.11.2014 um 20:20 schrieb Junio C Hamano:\n> Johannes Schindelin writes:\n>\n>> They are not affected by the update anyway.\n>>\n>> Signed-off-by: Johannes Schindelin \n>> ---\n>> builtin/receive-pack.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index be4172f..4ba51df 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n>> static void merge_worktree(unsigned char *sha1)\n>> {\n>> \tconst char *update_refresh[] = {\n>> -\t\t\"update-index\", \"--refresh\", NULL\n>> +\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n>> \t};\n>> \tconst char *read_tree[] = {\n>> \t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n>\n> I suspect that you did not squash this into 1/2 on purpose, and I am\n> guessing the reason is because you were unsure what should happen\n> when there were differences in submodules' working trees (otherwise,\n> you would have simply squashed without \"oops it was a thinko to\n> forget passing this option\" as a separate patch). I am not sure\n> either.\n\nI think --ignore-submodules is currently the right thing to do here\nand would rather squash this into the first commit.\n\n> By the way, if the expected use case of updateInstead is what I\n> outlined in the previous message, would it make more sense not to\n> fail with \"update-index --refresh\" failure (i.e. the working tree\n> files have no changes since the index)?\n>\n> Thinking about it a bit more, checking with \"update-index --refresh\"\n> feels doubly wrong. You not just want the working tree files to be\n> pristine with respect to the index, but also you do not want to see\n> any change between the index and the original HEAD, i.e.\n>\n> \t$ git reset --hard && echo >>Makefile ; git add Makefile\n> $ git update-index --refresh ; echo $?\n> 0\n>\n> this is not a good state from which you would want to update the\n> working tree.\n>\n> Wouldn't the two-tree form \"read-tree -u -m\" that is the equivalent\n> to branch switching do a sufficient check?\n>\n> Also, regarding the new calls to die() in the main patch, shouldn't\n> they just be returning the error reason in string form, just like\n> DENY_REFUSE returns \"branch is currently checked out\" to signal a\n> push failure to the caller?\n>\n>\n"},{"id":"251669","messageId":"alpine.DEB.1.00.1411101305510.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqvbmqg81g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-10T12:54:34Z","receivedAt":"2014-11-10T12:54:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 7 Nov 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Under certain circumstances, it makes a *lot* of sense to allow pushing\n> > into the current branch. For example, when two machines with different\n> > Operating Systems are required for testing, it makes much more sense to\n> > synchronize between working directories than having to go via a third\n> > server.\n> \n> Even if you do not have a third server, each time you decide to do\n> such a push, you have a single source of truth, i.e. the repository\n> you are pushing from, so instead of doing that push, all the others\n> could instead pull from that single source of truth. In that sense,\n> even though I wouldn't say it is wrong to use \"push\" in the opposite\n> direction for this synchronization, I have to say that the above is\n> not a very strong argument. It certainly does not deserve \"*lot*\"\n> in bold font ;-)\n\nI am sorry, I should have been more explicit about the fact that other\n\"Operating System\" includes also Windows, where it is a major hassle to\nset up an ssh daemon, hence the asymmetry of ease to connect from one to\nanother machine. I really thought that was obvious, my apologies. My next\npatch iteration will have the Windows scenario as motivation.\n\nNote that I did not even dive into the problem of many loaner laptops\nwhere you are not allowed to set up an ssh daemon, or even know the\nuser's password. I did not mention those problems because I again assumed\n(again, apologies) that this was obvious because it occurred to me\nautomatically when thinking about the multi-laptop problem.\n\nAnd I must apologize yet again, for I also failed to specify explicitly\nanother reason why pushing makes a *lot* more sense than pulling: at least\nfor me, personally, having to switch keyboards just to synchronize Git\ncheckouts from one to another computer *is* a hassle.\n\n> > Under different circumstances, the working directory needs to be left\n> > untouched, for example when a bunch of VMs need to be shut down to save\n> > RAM and one needs to push everything out into the host's non-bare\n> > repositories quickly.\n> \n> For this use case, if you assume that the tips of branches in the\n> repositories on \"a bunch of VMs\" could be pointing at different\n> commits (i.e. each of them has acquired more work without\n> coordination), you are risking lossage by pushing into refs/heads/,\n> not refs/remotes/vm$n/, aren't you? When you want to save things\n> away \"quickly\", you do not want to be worried about a push from VM1\n> stomping on what was stored from VM0, and using separate remotes,\n> i.e. refs/remotes/vm$n/, has been the recommended way to do so. So\n> this is not a very strong argument, either.\n\nI thank you for your assessment of my personal working style.\n\n:-)\n\nAnd yet again I have to apologize because I find that relying on Git's\nfast-forward default (you need to force non-fast-forward ref updates,\nwhich I don't, at least not by default) saves me from that lossage, and\nwill therefore not change my personal working style that served me so well\nfor years.\n\n> I do not think of a good justification of detachInstead offhand, but\n> you must have thought things through a lot more than I did, so you\n> can come up with a work flow description that is more usable by mere\n> mortals to justify that mode.\n\nThe main justification is that it is safer than updateInstead because it\nis guaranteed not to modify the working directory. I intended it for use\nby developers who are uncomfortable with updating the working directory\nthrough a push, and as uncomfortable with forgetting about temporary\nbranches pushed, say, via \"git push other-computer HEAD:refs/heads/tmp\".\n\nHowever, I have to admit that I never used this myself after the first few\nweeks of playing with this patch series.\n\n> Without such a description to justify why detachInstead makes sense, for\n> example, I cannot even answer this simple question:\n> \n> Would it make sense to have a third mode, \"check out if the\n> working tree is clean, detach otherwise\"?\n\nI imagine that some developer might find that useful. If you insist, I\nwill implement it, even if I personally deem this mode way too complicated\na concept for myself to be used safely.\n\n> > [... snip a lot of text that made me almost miss the comments below ...]\n> >\n> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> > index 32fc540..be4172f 100644\n> > --- a/builtin/receive-pack.c\n> > +++ b/builtin/receive-pack.c\n> > @@ -26,7 +26,9 @@ enum deny_action {\n> > \tDENY_UNCONFIGURED,\n> > \tDENY_IGNORE,\n> > \tDENY_WARN,\n> > -\tDENY_REFUSE\n> > +\tDENY_REFUSE,\n> > +\tDENY_UPDATE_INSTEAD,\n> > +\tDENY_DETACH_INSTEAD,\n> \n> Don't add trailing comma after the last element of enum.\n\nWill fix in the next iteration.\n\n> > @@ -730,6 +737,44 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n> > \treturn 0;\n> > }\n> > \n> > +static void merge_worktree(unsigned char *sha1)\n> > +{\n> > +\tconst char *update_refresh[] = {\n> > +\t\t\"update-index\", \"--refresh\", NULL\n> > +\t};\n> > +\tconst char *read_tree[] = {\n> > +\t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n> > +\t};\n> > +\tstruct child_process child;\n> > +\tstruct strbuf git_env = STRBUF_INIT;\n> > +\tconst char *env[2];\n> > +\n> > +\tif (is_bare_repository())\n> > +\t\tdie (\"denyCurrentBranch = updateInstead needs a worktree\");\n> \n> Why have extra SP only after \"die\" but not other function names?\n\nWill remove the space in the next iteration.\n\n> > +\tstrbuf_addf(&git_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n> > +\tenv[0] = git_env.buf;\n> > +\tenv[1] = NULL;\n> \n> Doesn't child.env have managed argv_array these days?\n\nI missed those developments, likewise the introduction of\nCHILD_PROCESS_INIT. Fixed in the next iteration.\n\n> > +\tmemset(&child, 0, sizeof(child));\n> > +\tchild.argv = update_refresh;\n> > +\tchild.env = env;\n> > +\tchild.dir = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n> > +\tchild.stdout_to_stderr = 1;\n> > +\tchild.git_cmd = 1;\n> > +\tif (run_command(&child))\n> > +\t\tdie (\"Could not refresh the index\");\n> > +\n> > +\tchild.argv = read_tree;\n> > +\tchild.no_stdin = 1;\n> > +\tchild.no_stdout = 1;\n> > +\tchild.stdout_to_stderr = 0;\n> > +\tif (run_command(&child))\n> > +\t\tdie (\"Could not merge working tree with new HEAD. Good luck.\");\n> \n> Drop \"Good luck.\" and replace it with a more useful info. At least,\n> tell the user what state you left her repository in by this botched\n> attempt, so that she can manually recover.\n\nThe best information Git can give at that point is that the working tree\ncould not be merged with the new HEAD. I stripped \"Good luck.\" in the next\niteration, although I mourn the loss of comedy in Git.\n\nCiao,\nJohannes\n"},{"id":"251670","messageId":"alpine.DEB.1.00.1411101400050.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqh9yag6mt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-10T13:01:20Z","receivedAt":"2014-11-10T13:01:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 7 Nov 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > builtin/receive-pack.c | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> > index be4172f..4ba51df 100644\n> > --- a/builtin/receive-pack.c\n> > +++ b/builtin/receive-pack.c\n> > @@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n> > static void merge_worktree(unsigned char *sha1)\n> > {\n> > \tconst char *update_refresh[] = {\n> > -\t\t\"update-index\", \"--refresh\", NULL\n> > +\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n> > \t};\n> > \tconst char *read_tree[] = {\n> > \t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n> \n> I suspect that you did not squash this into 1/2 on purpose, and I am\n> guessing the reason is because you were unsure what should happen\n> when there were differences in submodules' working trees (otherwise,\n> you would have simply squashed without \"oops it was a thinko to\n> forget passing this option\" as a separate patch). I am not sure\n> either.\n\nThis change is squashed into the first patch in the next iteration.\n\n> By the way, if the expected use case of updateInstead is what I\n> outlined in the previous message, would it make more sense not to\n> fail with \"update-index --refresh\" failure (i.e. the working tree\n> files have no changes since the index)?\n> \n> Thinking about it a bit more, checking with \"update-index --refresh\"\n> feels doubly wrong. You not just want the working tree files to be\n> pristine with respect to the index, but also you do not want to see\n> any change between the index and the original HEAD, i.e.\n> \n> \t$ git reset --hard && echo >>Makefile ; git add Makefile\n> $ git update-index --refresh ; echo $?\n> 0\n> \n> this is not a good state from which you would want to update the\n> working tree.\n> \n> Wouldn't the two-tree form \"read-tree -u -m\" that is the equivalent\n> to branch switching do a sufficient check?\n\nThat is indeed what the patched code calls.\n\n> Also, regarding the new calls to die() in the main patch, shouldn't\n> they just be returning the error reason in string form, just like\n> DENY_REFUSE returns \"branch is currently checked out\" to signal a\n> push failure to the caller?\n\nChanged in the next iteration.\n\nCiao,\nJohannes\n"},{"id":"251671","messageId":"alpine.DEB.1.00.1411101401510.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"20141108111855.GA21620@peff.net","subject":"Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-10T13:03:29Z","receivedAt":"2014-11-10T13:03:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sat, 8 Nov 2014, Jeff King wrote:\n\n> On Fri, Nov 07, 2014 at 02:58:17PM +0100, Johannes Schindelin wrote:\n> \n> > Under certain circumstances, it makes a *lot* of sense to allow pushing\n> > into the current branch. For example, when two machines with different\n> > Operating Systems are required for testing, it makes much more sense to\n> > synchronize between working directories than having to go via a third\n> > server.\n> \n> FWIW, I do this without a third server (and without resorting to pull),\n> with:\n> \n> host1$ git push host2 master:refs/remotes/host1/master\n> host2$ git merge host1/master\n> \n> You can even set up a push refspec to make \"git push host2\" do the right\n> thing.\n> \n> That being said, I do like the premise of your patch, as it eliminates\n> the extra step on the remote side (which is not that big a deal in\n> itself, but when you realize that host2 _did_ have some changes on it,\n> then you end up doing the merge there, when in general I'd prefer to do\n> all the work on host1 via \"git pull\").\n\nPlus: you have the luxury of working on an OS that makes ssh'ing from\nanother machine relatively easy. At least if you have the root password on\nyour machine. Which, I hate to point it out, is not too common a\ncommodity.\n\nCiao,\nDscho\n"},{"id":"251672","messageId":"alpine.DEB.1.00.1411101403380.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"545F9990.9050505@web.de","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-10T13:04:04Z","receivedAt":"2014-11-10T13:04:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jens,\n\nOn Sun, 9 Nov 2014, Jens Lehmann wrote:\n\n> Am 07.11.2014 um 20:20 schrieb Junio C Hamano:\n> > Johannes Schindelin writes:\n> >\n> > > Signed-off-by: Johannes Schindelin \n> > > ---\n> > > builtin/receive-pack.c | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> > > index be4172f..4ba51df 100644\n> > > --- a/builtin/receive-pack.c\n> > > +++ b/builtin/receive-pack.c\n> > > @@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd,\n> > > struct shallow_info *si)\n> > > static void merge_worktree(unsigned char *sha1)\n> > > {\n> > > \tconst char *update_refresh[] = {\n> > > -\t\t\"update-index\", \"--refresh\", NULL\n> > > +\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n> > > };\n> > > const char *read_tree[] = {\n> > > \"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n> >\n> > I suspect that you did not squash this into 1/2 on purpose, and I am\n> > guessing the reason is because you were unsure what should happen\n> > when there were differences in submodules' working trees (otherwise,\n> > you would have simply squashed without \"oops it was a thinko to\n> > forget passing this option\" as a separate patch). I am not sure\n> > either.\n> \n> I think --ignore-submodules is currently the right thing to do here\n> and would rather squash this into the first commit.\n\nDone.\n\nThanks,\nDscho\n"},{"id":"251675","messageId":"cover.1415629053.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":"cover.1415368490.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 0/2] Support updating working trees when pushing into non-bare repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-10T14:38:28Z","receivedAt":"2014-11-10T14:38:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series adds support for two new receive.denyCurrentBranch settings:\none to update the working directory (which must be clean, i.e. there must not\nbe any uncommitted changes) when pushing into the current branch, the other\nsetting detaches the HEAD instead.\n\nThe scenario in which in particular the 'updateInstead' setting became a\nboon in this developer's daily work is when trying to get a bug fix from a\nWindows computer, a virtual machine or a user's machine onto his main\nmachine (in all of those cases it is only possible to connect via ssh in one\ndirection, but not in the reverse direction).\n\nInterdiff vs v1 below the diffstat.\n\nJohannes Schindelin (2):\n Clean stale environment pointer in finish_command()\n Add a few more options for receive.denyCurrentBranch\n\n Documentation/config.txt | 9 ++++++++\n builtin/receive-pack.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++--\n run-command.c | 3 +++\n t/t5516-fetch-push.sh | 36 +++++++++++++++++++++++++++++\n 4 files changed, 106 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 6fc0d6d..fc9b8db 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2130,10 +2130,14 @@ receive.denyCurrentBranch::\n \tproceed. If set to false or \"ignore\", allow such pushes with no\n \tmessage. Defaults to \"refuse\".\n +\n-There are two more options: \"updateInstead\" which will update the working\n-directory (must be clean) if pushing into the current branch, and\n-\"detachInstead\" which will leave the working directory untouched, detaching\n-the HEAD so it does not need to change.\n+Another option is \"updateInstead\" which will update the working\n+directory (must be clean) if pushing into the current branch. This option is\n+intended for synchronizing working directories when one side is not easily\n+accessible via ssh (e.g. inside a VM).\n++\n+Yet another option is \"detachInstead\" which will detach the HEAD if updates\n+are pushed into the current branch; That way, the current revision, the\n+index and the working directory are always left untouched by pushes.\n \n receive.denyNonFastForwards::\n \tIf set to true, git-receive-pack will deny a ref update which is\ndiff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\nindex 2b2d797..7db860f 100755\n--- a/GIT-VERSION-GEN\n+++ b/GIT-VERSION-GEN\n@@ -1,7 +1,7 @@\n #!/bin/sh\n \n GVF=GIT-VERSION-FILE\n-DEF_VER=v2.2.0-rc0\n+DEF_VER=v2.2.0-rc1\n \n LF='\n '\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 4ba51df..4534e88 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -28,7 +28,7 @@ enum deny_action {\n \tDENY_WARN,\n \tDENY_REFUSE,\n \tDENY_UPDATE_INSTEAD,\n-\tDENY_DETACH_INSTEAD,\n+\tDENY_DETACH_INSTEAD\n };\n \n static int deny_deletes;\n@@ -737,7 +737,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \treturn 0;\n }\n \n-static void merge_worktree(unsigned char *sha1)\n+static const char *merge_worktree(unsigned char *sha1)\n {\n \tconst char *update_refresh[] = {\n \t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n@@ -745,41 +745,36 @@ static void merge_worktree(unsigned char *sha1)\n \tconst char *read_tree[] = {\n \t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n \t};\n-\tstruct child_process child;\n-\tstruct strbuf git_env = STRBUF_INIT;\n-\tconst char *env[2];\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n \n \tif (is_bare_repository())\n-\t\tdie (\"denyCurrentBranch = updateInstead needs a worktree\");\n-\n-\tstrbuf_addf(&git_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n-\tenv[0] = git_env.buf;\n-\tenv[1] = NULL;\n+\t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n \n-\tmemset(&child, 0, sizeof(child));\n+\targv_array_pushf(&child.env_array, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n \tchild.argv = update_refresh;\n-\tchild.env = env;\n \tchild.dir = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n \tchild.stdout_to_stderr = 1;\n \tchild.git_cmd = 1;\n \tif (run_command(&child))\n-\t\tdie (\"Could not refresh the index\");\n+\t\tdie(\"Could not refresh the index\");\n \n+\t/* finish_command cleared the environment; reinitialize */\n+\targv_array_pushf(&child.env_array, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n \tchild.argv = read_tree;\n \tchild.no_stdin = 1;\n \tchild.no_stdout = 1;\n \tchild.stdout_to_stderr = 0;\n \tif (run_command(&child))\n-\t\tdie (\"Could not merge working tree with new HEAD. Good luck.\");\n+\t\tdie(\"Could not merge working tree with new HEAD.\");\n \n-\tstrbuf_release(&git_env);\n+\treturn NULL;\n }\n \n static const char *update(struct command *cmd, struct shallow_info *si)\n {\n \tconst char *name = cmd->ref_name;\n \tstruct strbuf namespaced_name_buf = STRBUF_INIT;\n-\tconst char *namespaced_name;\n+\tconst char *namespaced_name, *ret;\n \tunsigned char *old_sha1 = cmd->old_sha1;\n \tunsigned char *new_sha1 = cmd->new_sha1;\n \n@@ -806,11 +801,17 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\trefuse_unconfigured_deny();\n \t\t\treturn \"branch is currently checked out\";\n \t\tcase DENY_UPDATE_INSTEAD:\n-\t\t\tmerge_worktree(new_sha1);\n+\t\t\tret = merge_worktree(new_sha1);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n \t\t\tbreak;\n \t\tcase DENY_DETACH_INSTEAD:\n-\t\t\tupdate_ref(\"push into current branch (detach)\", \"HEAD\",\n-\t\t\t\told_sha1, NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n+\t\t\tret = update_ref(\"push into current branch (detach)\",\n+\t\t\t\t\"HEAD\", old_sha1, NULL, REF_NODEREF,\n+\t\t\t\tUPDATE_REFS_DIE_ON_ERR) ?\n+\t\t\t\t\"Could not detach HEAD\" : NULL;\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n \t\t\tbreak;\n \t\t}\n \t}\ndiff --git a/run-command.c b/run-command.c\nindex 79a0a76..85578da 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -555,6 +555,9 @@ int finish_command(struct child_process *cmd)\n {\n \tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n \targv_array_clear(&cmd->args);\n+\t/* Avoid pointing to a stale environment */\n+\tif (cmd->env == cmd->env_array.argv)\n+\t\tcmd->env = NULL;\n \targv_array_clear(&cmd->env_array);\n \treturn ret;\n }\n\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"251674","messageId":"df1b16208e443416f7185c8c8c110e637ea97ac4.1415630072.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":"cover.1415630072.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/2] Clean stale environment pointer in finish_command()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-10T14:38:36Z","receivedAt":"2014-11-10T14:38:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In start_command(), unset \"env\" fields are initialized via \"env_array\". In\nfinish_command(), the \"env_array\" is cleared, therefore the \"env\" field\nwill point to free()d data.\n\nHowever, start_command() will set \"env\" to env_array.argv only if \"env\"\nwas unset to begin with, and if it was already set, the caller will need\nthe original value. Therefore, we need to be very careful only to reset\n\"env\" in finish_command() when it has been initialized in start_command().\n\nSigned-off-by: Johannes Schindelin \n---\n run-command.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 79a0a76..85578da 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -555,6 +555,9 @@ int finish_command(struct child_process *cmd)\n {\n \tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n \targv_array_clear(&cmd->args);\n+\t/* Avoid pointing to a stale environment */\n+\tif (cmd->env == cmd->env_array.argv)\n+\t\tcmd->env = NULL;\n \targv_array_clear(&cmd->env_array);\n \treturn ret;\n }\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"251676","messageId":"99cc2455d46c3188fb32356ab326aeb809cb863a.1415630072.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":"cover.1415630072.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/2] Add a few more options for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-10T14:38:44Z","receivedAt":"2014-11-10T14:38:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When synchronizing between working directories, it can be handy to update\nthe current branch via 'push' rather than 'pull', e.g. when pushing a fix\nfrom inside a VM, or when pushing a fix made on a user's machine (where\nthe developer is not at liberty to install an ssh daemon let alone know\nthe user's password).\n\nThe common workaround – pushing into a temporary branch and then merging\non the other machine – is no longer necessary with this patch.\n\nFor developers who are uncomfortable with letting pushes update the\nworking directory, but who are equally uncomfortable with the idea of\npushing into a temporary ref that will be readily forgotten, there is now\nalso an option to detach the HEAD if a push wants to update the current\nbranch (no working directory update is required in such a case because the\nbranch is no longer current after detaching the HEAD).\n\nThe new options are:\n\n'updateInstead':\n\tUpdate the working tree accordingly, but refuse to do so if there\n\tare any uncommitted changes.\n\n'detachInstead':\n\tDetach the HEAD, thereby keeping currently checked-out revision,\n\tindex and working directory unchanged.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/config.txt | 9 +++++++\n builtin/receive-pack.c | 61 +++++++++++++++++++++++++++++++++++++++++++++---\n t/t5516-fetch-push.sh | 36 ++++++++++++++++++++++++++++\n 3 files changed, 103 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e8dd76d..fc9b8db 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2129,6 +2129,15 @@ receive.denyCurrentBranch::\n \tprint a warning of such a push to stderr, but allow the push to\n \tproceed. If set to false or \"ignore\", allow such pushes with no\n \tmessage. Defaults to \"refuse\".\n++\n+Another option is \"updateInstead\" which will update the working\n+directory (must be clean) if pushing into the current branch. This option is\n+intended for synchronizing working directories when one side is not easily\n+accessible via ssh (e.g. inside a VM).\n++\n+Yet another option is \"detachInstead\" which will detach the HEAD if updates\n+are pushed into the current branch; That way, the current revision, the\n+index and the working directory are always left untouched by pushes.\n \n receive.denyNonFastForwards::\n \tIf set to true, git-receive-pack will deny a ref update which is\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 32fc540..4534e88 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -26,7 +26,9 @@ enum deny_action {\n \tDENY_UNCONFIGURED,\n \tDENY_IGNORE,\n \tDENY_WARN,\n-\tDENY_REFUSE\n+\tDENY_REFUSE,\n+\tDENY_UPDATE_INSTEAD,\n+\tDENY_DETACH_INSTEAD\n };\n \n static int deny_deletes;\n@@ -120,7 +122,12 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(var, \"receive.denycurrentbranch\")) {\n-\t\tdeny_current_branch = parse_deny_action(var, value);\n+\t\tif (value && !strcasecmp(value, \"updateinstead\"))\n+\t\t\tdeny_current_branch = DENY_UPDATE_INSTEAD;\n+\t\telse if (value && !strcasecmp(value, \"detachinstead\"))\n+\t\t\tdeny_current_branch = DENY_DETACH_INSTEAD;\n+\t\telse\n+\t\t\tdeny_current_branch = parse_deny_action(var, value);\n \t\treturn 0;\n \t}\n \n@@ -730,11 +737,44 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \treturn 0;\n }\n \n+static const char *merge_worktree(unsigned char *sha1)\n+{\n+\tconst char *update_refresh[] = {\n+\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n+\t};\n+\tconst char *read_tree[] = {\n+\t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n+\t};\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\n+\tif (is_bare_repository())\n+\t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n+\n+\targv_array_pushf(&child.env_array, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\tchild.argv = update_refresh;\n+\tchild.dir = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n+\tchild.stdout_to_stderr = 1;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child))\n+\t\tdie(\"Could not refresh the index\");\n+\n+\t/* finish_command cleared the environment; reinitialize */\n+\targv_array_pushf(&child.env_array, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\tchild.argv = read_tree;\n+\tchild.no_stdin = 1;\n+\tchild.no_stdout = 1;\n+\tchild.stdout_to_stderr = 0;\n+\tif (run_command(&child))\n+\t\tdie(\"Could not merge working tree with new HEAD.\");\n+\n+\treturn NULL;\n+}\n+\n static const char *update(struct command *cmd, struct shallow_info *si)\n {\n \tconst char *name = cmd->ref_name;\n \tstruct strbuf namespaced_name_buf = STRBUF_INIT;\n-\tconst char *namespaced_name;\n+\tconst char *namespaced_name, *ret;\n \tunsigned char *old_sha1 = cmd->old_sha1;\n \tunsigned char *new_sha1 = cmd->new_sha1;\n \n@@ -760,6 +800,19 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\tif (deny_current_branch == DENY_UNCONFIGURED)\n \t\t\t\trefuse_unconfigured_deny();\n \t\t\treturn \"branch is currently checked out\";\n+\t\tcase DENY_UPDATE_INSTEAD:\n+\t\t\tret = merge_worktree(new_sha1);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n+\t\t\tbreak;\n+\t\tcase DENY_DETACH_INSTEAD:\n+\t\t\tret = update_ref(\"push into current branch (detach)\",\n+\t\t\t\t\"HEAD\", old_sha1, NULL, REF_NODEREF,\n+\t\t\t\tUPDATE_REFS_DIE_ON_ERR) ?\n+\t\t\t\t\"Could not detach HEAD\" : NULL;\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n+\t\t\tbreak;\n \t\t}\n \t}\n \n@@ -788,6 +841,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\t\trefuse_unconfigured_deny_delete_current();\n \t\t\t\trp_error(\"refusing to delete the current branch: %s\", name);\n \t\t\t\treturn \"deletion of the current branch prohibited\";\n+\t\t\tdefault:\n+\t\t\t\tdie (\"Invalid denyDeleteCurrent setting\");\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f4da20a..3981d1b 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1330,4 +1330,40 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '\n \t)\n '\n \n+test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n+\tgit push testrepo master &&\n+\t(cd testrepo &&\n+\t\tgit reset --hard &&\n+\t\tgit config receive.denyCurrentBranch updateInstead\n+\t) &&\n+\ttest_commit third path2 &&\n+\tgit push testrepo master &&\n+\ttest $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&\n+\ttest third = \"$(cat testrepo/path2)\" &&\n+\t(cd testrepo &&\n+\t\tgit update-index --refresh &&\n+\t\tgit diff-files --quiet &&\n+\t\tgit diff-index --cached HEAD --\n+\t)\n+'\n+\n+test_expect_success 'receive.denyCurrentBranch = detachInstead' '\n+\t(cd testrepo &&\n+\t\tgit reset --hard &&\n+\t\tgit config receive.denyCurrentBranch detachInstead\n+\t) &&\n+\tOLDHEAD=$(cd testrepo && git rev-parse HEAD) &&\n+\ttest_commit fourth path2 &&\n+\ttest fourth = \"$(cat path2)\" &&\n+\tgit push testrepo master &&\n+\ttest $OLDHEAD = $(cd testrepo && git rev-parse HEAD) &&\n+\ttest fourth != \"$(cat testrepo/path2)\" &&\n+\t(cd testrepo &&\n+\t\ttest_must_fail git symbolic-ref HEAD &&\n+\t\tgit update-index --refresh &&\n+\t\tgit diff-files --quiet &&\n+\t\tgit diff-index --cached HEAD --\n+\t)\n+'\n+\n test_done\n-- \n2.0.0.rc3.9669.g840d1f9"},{"id":"251677","messageId":"alpine.DEB.1.00.1411101539091.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"df1b16208e443416f7185c8c8c110e637ea97ac4.1415630072.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-10T14:41:09Z","receivedAt":"2014-11-10T14:41:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 10 Nov 2014, Johannes Schindelin wrote:\n\n> In start_command(), unset \"env\" fields are initialized via \"env_array\". In\n> finish_command(), the \"env_array\" is cleared, therefore the \"env\" field\n> will point to free()d data.\n> \n> However, start_command() will set \"env\" to env_array.argv only if \"env\"\n> was unset to begin with, and if it was already set, the caller will need\n> the original value. Therefore, we need to be very careful only to reset\n> \"env\" in finish_command() when it has been initialized in start_command().\n\nIn case it was unclear: this is needed for the the suggested switch from the\nprevious method to construct the environment to the new env_array method\nto work.\n\n(The env_array method unfortunately requires the code to initialize the\nenvironment twice because finish_command() insists on always releasing the\nenv_array, even if the caller may want to reuse the generated array).\n\nCiao,\nJohannes\n"},{"id":"251678","messageId":"xmqq1tpbawqe.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1411101400050.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-10T15:42:01Z","receivedAt":"2014-11-10T15:42:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> By the way, if the expected use case of updateInstead is what I\n>> outlined in the previous message, would it make more sense not to\n>> fail with \"update-index --refresh\" failure (i.e. the working tree\n>> files have no changes since the index)?\n>> \n>> Thinking about it a bit more, checking with \"update-index --refresh\"\n>> feels doubly wrong. You not just want the working tree files to be\n>> pristine with respect to the index, but also you do not want to see\n>> any change between the index and the original HEAD, i.e.\n>> \n>> \t$ git reset --hard && echo >>Makefile ; git add Makefile\n>> $ git update-index --refresh ; echo $?\n>> 0\n>> \n>> this is not a good state from which you would want to update the\n>> working tree.\n>> \n>> Wouldn't the two-tree form \"read-tree -u -m\" that is the equivalent\n>> to branch switching do a sufficient check?\n>\n> That is indeed what the patched code calls.\n\nI know that ;-), but I think I wasn't clear enough.\n\nI am not saying you are not using two-tree read-tree. I am saying\nthe check with update-index --refresh before read-tree seems\ndubious.\n\nThere are three \"cleanliness\" we require in various occasions:\n\n (1) rebase asks you to have your index and the working tree match\n HEAD exactly, as if just after \"reset --hard HEAD\".\n\n (2) merge asks you to have your index match HEAD exactly (i.e. no\n output from \"diff --cached HEAD\"), and have no changes in the\n working tree at paths that are involved in the operation. It\n is OK to have local changes in the working tree (but not in the\n index) at paths that are not involved in a mergy operation.\n\n (3) checkout asks you to have your index and the working tree match\n either HEAD or the commit you are switching to exactly at paths\n that are involved in the operation. It is OK to have local\n changes in the working tree and in the index at paths that are\n not different between the commits you are moving between, and\n it is also OK to have the contents from the \"new\" commit in the\n working tree and the index at paths that are different between\n the two.\n\nDying when \"update-index --refresh\" signals a difference is an\nattempt to mimic #1, but it is in line with the spirit of the reason\nwhy a user would want to use updateInstead, I think. The situation\nis more like the person who pushed into your repository from\nsideline did a \"checkout -B $current_branch $new_commit\" to update\nthe HEAD, the index and the working tree, to let you pretend as if\nyou based your work on the commit he pushed to you.\n\nWhile you still need to error out when your local work does not\nsatisfy the cleanliness criteria #3 above, I do not think you would\nwant to stop the operation when \"checkout\" would not fail, e.g. you\nhave a local change that does not interfere with the update between\nthe two commits, with this one:\n\n+\tif (run_command(&child))\n+\t\tdie (\"Could not refresh the index\");\n\nWhen refreshed the index successfully, we signal that there were\ndifferences between the index and the working tree with a non-zero\nreturn value, so \"Could not refresh\" is not quite right, either.\n\nBut this one that checks the exit status from two-tree read-tree\n\n+\tif (run_command(&child))\n+\t\tdie (\"Could not merge working tree with new HEAD. Good luck.\");\n\nis checking the right condition, i.e. cleanliness #3. The\ndisposition should not be \"die\", but an error return to tell the\ncaller to abort the push as we discussed earlier.\n"},{"id":"251679","messageId":"xmqqwq739hau.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1411101305510.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-10T16:00:41Z","receivedAt":"2014-11-10T16:00:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> I do not think of a good justification of detachInstead offhand, but\n>> you must have thought things through a lot more than I did, so you\n>> can come up with a work flow description that is more usable by mere\n>> mortals to justify that mode.\n>\n> The main justification is that it is safer than updateInstead because it\n> is guaranteed not to modify the working directory. I intended it for use\n> by developers who are uncomfortable with updating the working directory\n> through a push, and as uncomfortable with forgetting about temporary\n> branches pushed, say, via \"git push other-computer HEAD:refs/heads/tmp\".\n>\n> However, I have to admit that I never used this myself after the first few\n> weeks of playing with this patch series.\n>\n>> Without such a description to justify why detachInstead makes sense, for\n>> example, I cannot even answer this simple question:\n>> \n>> Would it make sense to have a third mode, \"check out if the\n>> working tree is clean, detach otherwise\"?\n>\n> I imagine that some developer might find that useful. If you insist, I\n> will implement it, even if I personally deem this mode way too complicated\n> a concept for myself to be used safely.\n\nYou have been around long enough to know that adding a feature of an\nunknown value is the last thing I would ask, don't you?\n\nI am essentially saying this:\n\n I do not see why and the patch and its documentation do not\n explain why it is useful, but Dscho wouldn't have added the\n feature without a reason better than \"just because we can do\n so\", so let's assume the mode is useful for some unknown reason.\n Would people find \"updateInstead if able otherwise\n detachInstead\" even more useful for that same unknown reason?\n\nSo a good answer would have been one of these:\n\n * detachInstead is not backed by a use case as useful as\n updateInstead, and was done more from \"because we can while at\n it\". Let's drop it for now.\n\n * detachInstead is a good thing and here is an updated patch to\n better explain the readers of our documentation the workflow to\n use to the feature well. updateIfAbleOrDetachInstead would be\n useful for the same reason stated in the updated documentation,\n but let's not make the scope of the topic too wide and stop at\n mentioning the possibility in the log message for now.\n \n\n * detachInstead is a good thing and here is an updated patch to\n better explain the readers of our documentation the workflow to\n use to the feature well. Once a reader understands this, she\n would realize why updateIfAbleOrDetachInstead would not be\n useful, so it is not even worth mentioning the possibility.\n\n>> > +\tif (run_command(&child))\n>> > +\t\tdie (\"Could not merge working tree with new HEAD. Good luck.\");\n>> \n>> Drop \"Good luck.\" and replace it with a more useful info. At least,\n>> tell the user what state you left her repository in by this botched\n>> attempt, so that she can manually recover.\n>\n> The best information Git can give at that point is that the working tree\n> could not be merged with the new HEAD. I stripped \"Good luck.\" in the next\n> iteration, although I mourn the loss of comedy in Git.\n\nBeing humourous is good, but \"Good luck\" while refusing to be\nhelpful is not humourous; it is just being hostile to the user.\n\nWe would be showing the string as the reason for push failure to\n\"git push\" in the new code that does not die but signal the failure\nto our caller, so \"could not merge working tree with new HEAD\" is\ngood (we shouldn't be doing the \"advice\" thing here).\n\nThanks.\n"},{"id":"251687","messageId":"xmqq389qam25.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"xmqq1tpbawqe.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-10T19:32:34Z","receivedAt":"2014-11-10T19:32:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Dying when \"update-index --refresh\" signals a difference is an\n> attempt to mimic #1, but it is in line with the spirit of the reason\n> why a user would want to use updateInstead, I think. The situation\n> is more like the person who pushed into your repository from\n> sideline did a \"checkout -B $current_branch $new_commit\" to update\n> the HEAD, the index and the working tree, to let you pretend as if\n> you based your work on the commit he pushed to you.\n>\n> While you still need to error out when your local work does not\n> satisfy the cleanliness criteria #3 above, I do not think you would\n> want to stop the operation when \"checkout\" would not fail, e.g. you\n> have a local change that does not interfere with the update between\n> the two commits, with this one:\n>\n> +\tif (run_command(&child))\n> +\t\tdie (\"Could not refresh the index\");\n>\n> When refreshed the index successfully, we signal that there were\n> differences between the index and the working tree with a non-zero\n> return value, so \"Could not refresh\" is not quite right, either.\n\nJust to make sure. I am *not* saying that you do not need to run\n\"update-index --refresh\". It is necessary before running read-tree\nto avoid false dirtyness, so you do need to run it.\n\nI am only saying that it is too strict to fail the operation when\nthe command reports that you have a local modification in the\nworking tree.\n\n> But this one that checks the exit status from two-tree read-tree\n>\n> +\tif (run_command(&child))\n> +\t\tdie (\"Could not merge working tree with new HEAD. Good luck.\");\n>\n> is checking the right condition, i.e. cleanliness #3. The\n> disposition should not be \"die\", but an error return to tell the\n> caller to abort the push as we discussed earlier.\n"},{"id":"251694","messageId":"xmqqbnoe91dz.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"df1b16208e443416f7185c8c8c110e637ea97ac4.1415630072.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-10T21:44:24Z","receivedAt":"2014-11-10T21:44:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> In start_command(), unset \"env\" fields are initialized via \"env_array\". In\n> finish_command(), the \"env_array\" is cleared, therefore the \"env\" field\n> will point to free()d data.\n>\n> However, start_command() will set \"env\" to env_array.argv only if \"env\"\n> was unset to begin with, and if it was already set, the caller will need\n> the original value. Therefore, we need to be very careful only to reset\n> \"env\" in finish_command() when it has been initialized in start_command().\n\nHmph. Does the same observation apply to cmd->argv that is\ninitialied to point to cmd->args.argv only when it is unset?\n\nThese managed argv/env arrays originate from c460c0ec (run-command:\nstore an optional argv_array, 2014-05-15), so I am CC'ing Peff. I\nthink this change makes sense but I suspect we should do the same\nfor args.\n\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> run-command.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 79a0a76..85578da 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -555,6 +555,9 @@ int finish_command(struct child_process *cmd)\n> {\n> \tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n> \targv_array_clear(&cmd->args);\n> +\t/* Avoid pointing to a stale environment */\n> +\tif (cmd->env == cmd->env_array.argv)\n> +\t\tcmd->env = NULL;\n> \targv_array_clear(&cmd->env_array);\n> \treturn ret;\n> }\n"},{"id":"251711","messageId":"20141111031109.GD21328@peff.net","threadId":"37896","inReplyTo":"xmqqbnoe91dz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-11T03:11:09Z","receivedAt":"2014-11-11T03:11:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 10, 2014 at 01:44:24PM -0800, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > In start_command(), unset \"env\" fields are initialized via \"env_array\". In\n> > finish_command(), the \"env_array\" is cleared, therefore the \"env\" field\n> > will point to free()d data.\n> >\n> > However, start_command() will set \"env\" to env_array.argv only if \"env\"\n> > was unset to begin with, and if it was already set, the caller will need\n> > the original value. Therefore, we need to be very careful only to reset\n> > \"env\" in finish_command() when it has been initialized in start_command().\n> \n> Hmph. Does the same observation apply to cmd->argv that is\n> initialied to point to cmd->args.argv only when it is unset?\n\nYes, they behave exactly the same (I think Dscho just doesn't hit it in\nhis patch because he assigns argv manually).\n\nI don't have a real problem with going in this direction as a safety\nmeasure, but I am not sure that it is safe to reuse a child_process\nafter finish_command in general, without an intervening\nchild_process_init. For instance, calling start_command will convert a\n\"child_process.in\" value of \"-1\" instead a pipe, and overwrite that \"-1\"\nwith the descriptor of the pipe. A subsequent use of the same\nchild_process struct will ask the second child to use that pipe (the\nwrite-half of the pipe, mind you) as its stdin, which is nonsensical.\n\nSo I think you are much better off just using two child_process structs\n(or a single one and reinitializing in between calls).\n\n-Peff\n"},{"id":"251712","messageId":"20141111031636.GE21328@peff.net","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1411101539091.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-11T03:16:36Z","receivedAt":"2014-11-11T03:16:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 10, 2014 at 03:41:09PM +0100, Johannes Schindelin wrote:\n\n> > However, start_command() will set \"env\" to env_array.argv only if \"env\"\n> > was unset to begin with, and if it was already set, the caller will need\n> > the original value. Therefore, we need to be very careful only to reset\n> > \"env\" in finish_command() when it has been initialized in start_command().\n> \n> In case it was unclear: this is needed for the the suggested switch from the\n> previous method to construct the environment to the new env_array method\n> to work.\n> \n> (The env_array method unfortunately requires the code to initialize the\n> environment twice because finish_command() insists on always releasing the\n> env_array, even if the caller may want to reuse the generated array).\n\nI don't think this is \"unfortunately\"; freeing the memory was the entire\npurpose in adding env_array. If you want to easily reuse the same\nenvironment in multiple commands, it is still perfectly fine to use\n\"env\" directly, like:\n\n struct argv_array env = ARGV_ARRAY_INIT;\n struct child_process one = CHILD_PROCESS_INIT;\n struct child_process two = CHILD_PROCESS_INIT;\n\n ... setup env with argv_array_push ...\n\n one.argv = foo;\n one.env = env.argv;\n run_command(&one);\n\n two.argv = bar;\n two.env = env.argv;\n run_command(&two);\n\n argv_array_clear(&env);\n\nYou do not get the benefit of the auto-cleanup (you have to call\nargv_array_clear yourself), but that is less bad than repeating the\nsetup of \"env\" twice.\n\nIt may be that Documentation/technical/api-run-command.txt needs to\nmake this more explicit (I just read over it and it looks OK to me. But\nsince I am the one who designed the feature, I am not the best test of\nwhether it is sufficiently clear).\n\n-Peff\n"},{"id":"251718","messageId":"xmqqr3x97mv4.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"20141111031636.GE21328@peff.net","subject":"Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-11T15:55:43Z","receivedAt":"2014-11-11T15:55:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't think this is \"unfortunately\"; freeing the memory was the entire\n> purpose in adding env_array. If you want to easily reuse the same\n> environment in multiple commands, it is still perfectly fine to use\n> \"env\" directly, like:\n>\n> struct argv_array env = ARGV_ARRAY_INIT;\n> struct child_process one = CHILD_PROCESS_INIT;\n> struct child_process two = CHILD_PROCESS_INIT;\n>\n> ... setup env with argv_array_push ...\n>\n> one.argv = foo;\n> one.env = env.argv;\n> run_command(&one);\n>\n> two.argv = bar;\n> two.env = env.argv;\n> run_command(&two);\n>\n> argv_array_clear(&env);\n>\n> You do not get the benefit of the auto-cleanup (you have to call\n> argv_array_clear yourself), but that is less bad than repeating the\n> setup of \"env\" twice.\n\nYeah, the above looks like the best option, better than using a\nsingle child_process and having to re-initialize fds and envs.\n\nThanks for helping.\n"},{"id":"251750","messageId":"alpine.DEB.1.00.1411121142480.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqr3x97mv4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-12T10:45:19Z","receivedAt":"2014-11-12T10:45:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 11 Nov 2014, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I don't think this is \"unfortunately\"; freeing the memory was the entire\n> > purpose in adding env_array. If you want to easily reuse the same\n> > environment in multiple commands, it is still perfectly fine to use\n> > \"env\" directly, like:\n> >\n> > struct argv_array env = ARGV_ARRAY_INIT;\n> > struct child_process one = CHILD_PROCESS_INIT;\n> > struct child_process two = CHILD_PROCESS_INIT;\n> >\n> > ... setup env with argv_array_push ...\n> >\n> > one.argv = foo;\n> > one.env = env.argv;\n> > run_command(&one);\n> >\n> > two.argv = bar;\n> > two.env = env.argv;\n> > run_command(&two);\n> >\n> > argv_array_clear(&env);\n> >\n> > You do not get the benefit of the auto-cleanup (you have to call\n> > argv_array_clear yourself), but that is less bad than repeating the\n> > setup of \"env\" twice.\n> \n> Yeah, the above looks like the best option, better than using a\n> single child_process and having to re-initialize fds and envs.\n\nOkay, I have to say that I was led to believe that reusing the\nchild_process struct is okay because argv_array_clear() explicitly\nreinitializes the env_array field, something that is useless churn unless\nyou plan to reuse the memory.\n\nHowever, my personal taste says that reusing the same memory is more\nelegant than to waste extra memory unnecessarily, so I will go with the\nchild_process_init() solution.\n\nCiao,\nDscho\n"},{"id":"251751","messageId":"20141112105229.GA29101@peff.net","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1411121142480.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-12T10:52:29Z","receivedAt":"2014-11-12T10:52:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 12, 2014 at 11:45:19AM +0100, Johannes Schindelin wrote:\n\n> Okay, I have to say that I was led to believe that reusing the\n> child_process struct is okay because argv_array_clear() explicitly\n> reinitializes the env_array field, something that is useless churn unless\n> you plan to reuse the memory.\n\nThe argv_array code prepares its data structure for reuse after freeing.\nBut child_process which uses it does not make any such promises. If\nthere were an argv_array_free(), child_process could use it. But there\nis only argv_array_clear().\n\nI don't have a problem with finish_command leaving its child_process in\na known usable state (e.g., by calling child_process_init). But I also\ndon't see much benefit.\n\n> However, my personal taste says that reusing the same memory is more\n> elegant than to waste extra memory unnecessarily, so I will go with the\n> child_process_init() solution.\n\nI do not mind much either way. But I doubt that a single extra struct on\nthe stack will break the bank, compared to the fact that we are forking\nand execing a new program. I'd also not be surprised if a smart compiler\ncould notice that the variables are used exclusively in non-overlapping\nbits of the code, and just reuse the stack space.\n\n-Peff\n"},{"id":"251752","messageId":"20141112105911.GA31608@peff.net","threadId":"37896","inReplyTo":"20141112105229.GA29101@peff.net","subject":"Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-12T10:59:11Z","receivedAt":"2014-11-12T10:59:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 12, 2014 at 05:52:29AM -0500, Jeff King wrote:\n\n> > However, my personal taste says that reusing the same memory is more\n> > elegant than to waste extra memory unnecessarily, so I will go with the\n> > child_process_init() solution.\n> \n> I do not mind much either way. But I doubt that a single extra struct on\n> the stack will break the bank, compared to the fact that we are forking\n> and execing a new program. I'd also not be surprised if a smart compiler\n> could notice that the variables are used exclusively in non-overlapping\n> bits of the code, and just reuse the stack space.\n\nActually, I take that back. We are passing a pointer to a struct, rather\nthan by-value, so the compiler cannot know that the sub-function does\nnot store that pointer in a static variable, and reference it in the\nnext call. It must use two variables if it cannot see the definition of\nrun_command.\n\nI still think it's pointless optimization to worry about, and you should\nwrite whichever is the most readable and maintainable.\n\n-Peff\n"},{"id":"251753","messageId":"alpine.DEB.1.00.1411121205360.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqq1tpbawqe.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-12T11:06:37Z","receivedAt":"2014-11-12T11:06:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 10 Nov 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> By the way, if the expected use case of updateInstead is what I\n> >> outlined in the previous message, would it make more sense not to\n> >> fail with \"update-index --refresh\" failure (i.e. the working tree\n> >> files have no changes since the index)?\n> >> \n> >> Thinking about it a bit more, checking with \"update-index --refresh\"\n> >> feels doubly wrong. You not just want the working tree files to be\n> >> pristine with respect to the index, but also you do not want to see\n> >> any change between the index and the original HEAD, i.e.\n> >> \n> >> \t$ git reset --hard && echo >>Makefile ; git add Makefile\n> >> $ git update-index --refresh ; echo $?\n> >> 0\n> >> \n> >> this is not a good state from which you would want to update the\n> >> working tree.\n> >> \n> >> Wouldn't the two-tree form \"read-tree -u -m\" that is the equivalent\n> >> to branch switching do a sufficient check?\n> >\n> > That is indeed what the patched code calls.\n> \n> I know that ;-), but I think I wasn't clear enough.\n> \n> I am not saying you are not using two-tree read-tree. I am saying\n> the check with update-index --refresh before read-tree seems\n> dubious.\n> \n> There are three \"cleanliness\" we require in various occasions:\n> \n> (1) rebase asks you to have your index and the working tree match\n> HEAD exactly, as if just after \"reset --hard HEAD\".\n> \n> (2) merge asks you to have your index match HEAD exactly (i.e. no\n> output from \"diff --cached HEAD\"), and have no changes in the\n> working tree at paths that are involved in the operation. It\n> is OK to have local changes in the working tree (but not in the\n> index) at paths that are not involved in a mergy operation.\n> \n> (3) checkout asks you to have your index and the working tree match\n> either HEAD or the commit you are switching to exactly at paths\n> that are involved in the operation. It is OK to have local\n> changes in the working tree and in the index at paths that are\n> not different between the commits you are moving between, and\n> it is also OK to have the contents from the \"new\" commit in the\n> working tree and the index at paths that are different between\n> the two.\n> \n> Dying when \"update-index --refresh\" signals a difference is an\n> attempt to mimic #1, but it is in line with the spirit of the reason\n> why a user would want to use updateInstead, I think. The situation\n> is more like the person who pushed into your repository from\n> sideline did a \"checkout -B $current_branch $new_commit\" to update\n> the HEAD, the index and the working tree, to let you pretend as if\n> you based your work on the commit he pushed to you.\n> \n> While you still need to error out when your local work does not\n> satisfy the cleanliness criteria #3 above, I do not think you would\n> want to stop the operation when \"checkout\" would not fail, e.g. you\n> have a local change that does not interfere with the update between\n> the two commits, with this one:\n> \n> +\tif (run_command(&child))\n> +\t\tdie (\"Could not refresh the index\");\n> \n> When refreshed the index successfully, we signal that there were\n> differences between the index and the working tree with a non-zero\n> return value, so \"Could not refresh\" is not quite right, either.\n> \n> But this one that checks the exit status from two-tree read-tree\n> \n> +\tif (run_command(&child))\n> +\t\tdie (\"Could not merge working tree with new HEAD. Good luck.\");\n> \n> is checking the right condition, i.e. cleanliness #3. The\n> disposition should not be \"die\", but an error return to tell the\n> caller to abort the push as we discussed earlier.\n\nI have to say that I am quite puzzled now: I guess that you want me to\ndrop the update-index --refresh call?\n\nCiao,\nJohannes\n"},{"id":"251754","messageId":"alpine.DEB.1.00.1411121208250.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqq389qam25.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-12T11:09:14Z","receivedAt":"2014-11-12T11:09:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 10 Nov 2014, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Dying when \"update-index --refresh\" signals a difference is an\n> > attempt to mimic #1, but it is in line with the spirit of the reason\n> > why a user would want to use updateInstead, I think. The situation\n> > is more like the person who pushed into your repository from\n> > sideline did a \"checkout -B $current_branch $new_commit\" to update\n> > the HEAD, the index and the working tree, to let you pretend as if\n> > you based your work on the commit he pushed to you.\n> >\n> > While you still need to error out when your local work does not\n> > satisfy the cleanliness criteria #3 above, I do not think you would\n> > want to stop the operation when \"checkout\" would not fail, e.g. you\n> > have a local change that does not interfere with the update between\n> > the two commits, with this one:\n> >\n> > +\tif (run_command(&child))\n> > +\t\tdie (\"Could not refresh the index\");\n> >\n> > When refreshed the index successfully, we signal that there were\n> > differences between the index and the working tree with a non-zero\n> > return value, so \"Could not refresh\" is not quite right, either.\n> \n> Just to make sure. I am *not* saying that you do not need to run\n> \"update-index --refresh\". It is necessary before running read-tree\n> to avoid false dirtyness, so you do need to run it.\n> \n> I am only saying that it is too strict to fail the operation when\n> the command reports that you have a local modification in the\n> working tree.\n\nOkay, now I am even more puzzled. I guess you actually meant to say that I\nneed to convert the die() into a return? If so, I agree fully.\n\nCiao,\nJohannes\n"},{"id":"251755","messageId":"alpine.DEB.1.00.1411121209330.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqwq739hau.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-12T11:13:21Z","receivedAt":"2014-11-12T11:13:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 10 Nov 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> I do not think of a good justification of detachInstead offhand, but\n> >> you must have thought things through a lot more than I did, so you\n> >> can come up with a work flow description that is more usable by mere\n> >> mortals to justify that mode.\n> >\n> > The main justification is that it is safer than updateInstead because\n> > it is guaranteed not to modify the working directory. I intended it\n> > for use by developers who are uncomfortable with updating the working\n> > directory through a push, and as uncomfortable with forgetting about\n> > temporary branches pushed, say, via \"git push other-computer\n> > HEAD:refs/heads/tmp\".\n> >\n> > However, I have to admit that I never used this myself after the first\n> > few weeks of playing with this patch series.\n> >\n> >> Without such a description to justify why detachInstead makes sense,\n> >> for example, I cannot even answer this simple question:\n> >> \n> >> Would it make sense to have a third mode, \"check out if the\n> >> working tree is clean, detach otherwise\"?\n> >\n> > I imagine that some developer might find that useful. If you insist, I\n> > will implement it, even if I personally deem this mode way too\n> > complicated a concept for myself to be used safely.\n> \n> You have been around long enough to know that adding a feature of an\n> unknown value is the last thing I would ask, don't you?\n\nGiven that you actually did ask me to add such a feature when I simply\nwanted to get a bug fix for fast-export into Git to support Sverre's\nremote-hg (that he abandoned because of my failure to get the bug fix in),\nI have to respectfully declare that I do not know that, no, sorry!\n\n> I am essentially saying this:\n> \n> I do not see why and the patch and its documentation do not\n> explain why it is useful, but Dscho wouldn't have added the\n> feature without a reason better than \"just because we can do\n> so\", so let's assume the mode is useful for some unknown reason.\n> Would people find \"updateInstead if able otherwise\n> detachInstead\" even more useful for that same unknown reason?\n\nOkay, here is my explanation: at the time I wanted to disprove that\nupdateInstead could make sense, I wanted to offer a milder version of\nupdating the current branch that left the working directory alone:\ndetachInstead.\n\nNow, I never used it myself, but I use updateInstead extensively.\n\nSo now I offer you two choices:\n\n- help me come up with a good scenario where it makes sense to\n detachInstead, or\n\n- tell me to drop it.\n\nI have no preference.\n\nCiao,\nJohannes\n"},{"id":"251758","messageId":"CAPc5daXRUFqjrZbTE968uP5tY9hF3EG0pMKwGW+YPRLBo5=ghQ@mail.gmail.com","threadId":"37896","inReplyTo":"20141112105911.GA31608@peff.net","subject":"Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-12T16:17:12Z","receivedAt":"2014-11-12T16:17:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Wed, Nov 12, 2014 at 2:59 AM, Jeff King wrote:\n>>\n>> I do not mind much either way. But I doubt that a single extra struct on\n>> the stack will break the bank, compared to the fact that we are forking\n>> and execing a new program. I'd also not be surprised if a smart compiler\n>> could notice that the variables are used exclusively in non-overlapping\n>> bits of the code, and just reuse the stack space.\n>\n> Actually, I take that back. We are passing a pointer to a struct, rather\n> than by-value, so the compiler cannot know that the sub-function does\n> not store that pointer in a static variable, and reference it in the\n> next call. It must use two variables if it cannot see the definition of\n> run_command.\n>\n> I still think it's pointless optimization to worry about, and you should\n> write whichever is the most readable and maintainable.\n\nAmen. I do not have strong preference either way as long as the result\nis readable, correct and maintainlable ;-).\n"},{"id":"251761","messageId":"xmqqzjbw47vr.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1411121208250.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-12T17:59:52Z","receivedAt":"2014-11-12T17:59:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Mon, 10 Nov 2014, Junio C Hamano wrote:\n>\n>> Junio C Hamano writes:\n>> \n>> > Dying when \"update-index --refresh\" signals a difference is an\n>> > attempt to mimic #1, but it is in line with the spirit of the reason\n>> > why a user would want to use updateInstead, I think. The situation\n>> > is more like the person who pushed into your repository from\n>> > sideline did a \"checkout -B $current_branch $new_commit\" to update\n>> > the HEAD, the index and the working tree, to let you pretend as if\n>> > you based your work on the commit he pushed to you.\n>> >\n>> > While you still need to error out when your local work does not\n>> > satisfy the cleanliness criteria #3 above, I do not think you would\n>> > want to stop the operation when \"checkout\" would not fail, e.g. you\n>> > have a local change that does not interfere with the update between\n>> > the two commits, with this one:\n>> >\n>> > +\tif (run_command(&child))\n>> > +\t\tdie (\"Could not refresh the index\");\n>> >\n>> > When refreshed the index successfully, we signal that there were\n>> > differences between the index and the working tree with a non-zero\n>> > return value, so \"Could not refresh\" is not quite right, either.\n>> \n>> Just to make sure. I am *not* saying that you do not need to run\n>> \"update-index --refresh\". It is necessary before running read-tree\n>> to avoid false dirtyness, so you do need to run it.\n>> \n>> I am only saying that it is too strict to fail the operation when\n>> the command reports that you have a local modification in the\n>> working tree.\n>\n> Okay, now I am even more puzzled. I guess you actually meant to say that I\n> need to convert the die() into a return? If so, I agree fully.\n\nNo.\n\n\"update-index --refresh\" does two things.\n\n (a) For performance reasons, plumbing commands such as diff-files\n and read-tree do not refresh the stat bits in the index. They\n expect to be run from scripted Porcelain commands, and expect\n that the caller would refresh the stat bits before they are\n called to prevent them from mistakingly seeing that an\n unmodified existing file, after \"touch existing-file\", as\n modified.\n\n And \"update-index --refresh\" is the way for the caller to do so.\n\n (b) \"update-index --refresh\" indicates with its exit status if the\n working tree files match what is recorded in the index. This\n can be used to see if \"diff-files\" would report difference.\n\nAs you are going to run \"read-tree -m -u\", you need to refresh the\nstat bits for purpose (a) above, i.e. to avoid \"read-tree\" from\nfailing due to a difference that does not exist. Because I am\nassuming that your \"cleanliness\" requirement to update the working\ntree is criterion #3 in the previous message, I do not think you\nwould want to abort the update only because there are some\ndifference between the index and the working tree. That means that\nchecking the exit status of \"update-index --refresh\" and dying (or\nsignaling the failure to the caller by returning a non-NULL string,\nin the context of this call path) is not what you want. You may\nhave a local change to Makefile in the working tree of the\nrepository that you are pushing into, and there may not be any\nchange to the Makefile between the original HEAD the working tree is\nbased on and the updated HEAD you are pushing into the repository.\n\"update-index --refresh\" will say \"You have a local change.\" and\nexit with non-zero status, but just like \"git checkout another\" to\nswitch to another branch while you have some local change that does\nnot overlap with the difference between branches does not fail, you\nwould want to allow the update.\n\nYou may be trying to use a cleanliness requirement that is different\nfrom criterion #3 in the previous message, but checking the exit\nstatus from \"update-index --refresh\" does not make much sense in\nthat case either. I do not think you want to have:\n\n * pushing into a repository that did \"edit Makefile; git add Makefile\"\n succeeds.\n\n * pushing into a repository that did \"edit Makefile\" without \"git\n add Makefile\" fails.\n\nbut that is what you will get, because \"update-index --refresh\"\nwould say \"Your working tree matches the index\" by exiting with 0 in\nthe former case, and you will end up running \"read-tree -m -u\".\n\nHaving said all that.\n\nInstead of running \"update-index --refresh; read-tree -m -u\", using\n\"reset --keep\" may be a better implementation of what you are trying\nto do here.\n\nI think a \"checkout -B \" is what you\nwant to run when a push attempts to update the current branch from\nsideways with updateInstead, and \"reset --keep \" lets\nyou run an equivalent of the \"checkout -B \" but you\ndo not have to know the name of the . Also by using\n\"reset --keep\", you do not have to worry about refreshing the index.\n\nThanks.\n"},{"id":"251762","messageId":"xmqqsiho47uq.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1411121209330.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-12T18:00:29Z","receivedAt":"2014-11-12T18:00:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Okay, here is my explanation: at the time I wanted to disprove that\n> updateInstead could make sense, I wanted to offer a milder version of\n> updating the current branch that left the working directory alone:\n> detachInstead.\n>\n> Now, I never used it myself, but I use updateInstead extensively.\n\nSounds like updateInstead turned out to be useful enough to make\na \"possibly more cautious\" detachInstead unnecessary? It probably\nmakes sense not to add it in that case, I would think.\n"},{"id":"251795","messageId":"alpine.DEB.1.00.1411131123330.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqzjbw47vr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-13T10:29:03Z","receivedAt":"2014-11-13T10:29:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 12 Nov 2014, Junio C Hamano wrote:\n\n> Instead of running \"update-index --refresh; read-tree -m -u\", using\n> \"reset --keep\" may be a better implementation of what you are trying to\n> do here.\n\nI do not think that `reset --keep` is what I want. I really want to update\nonly if the working directory is clean. So I guess I will have to bite the\nbullet and test the output of `update-index --refresh`, `diff-index\n--quiet --cached HEAD --`.\n\nIn my case, the lacking test whether there are staged changes did not\nmatter, just because I pretty much never leave staged changes around.\n\nWhat did matter, however, was to make sure that I did not update the\nworking directory carelessly. In one case, that `update-index --refresh`\ntest really helped me out because I was about to push into a working\ndirectory with uncommitted changes inside some web space. That push would\nhave broken the web application because of the local changes, so I was\nreally, really happy that I decided to be quite strict in the\nimplementation of `updateInstead`.\n\nDue to that experience, the documentation also states pretty clearly that\n`updateInstead` succeeds only in updating the current branch if the\nworking directory is clean.\n\nCiao,\nJohannes\n"},{"id":"251796","messageId":"alpine.DEB.1.00.1411131136200.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1411131123330.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-13T10:38:25Z","receivedAt":"2014-11-13T10:38:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 13 Nov 2014, Johannes Schindelin wrote:\n\n> Due to that experience, the documentation also states pretty clearly that\n> `updateInstead` succeeds only in updating the current branch if the\n> working directory is clean.\n\nTo clarify why `updateInstead` is stricter than the `merge` scenario: when\npushing into a remote repository with attached working directory, we\ncannot simply clean up merge conflicts or other problems introduced by a\nmerge. Indeed, it is even possible to push without having any option to\nfix files in the working directory afterwards. Therefore, the\n`updateInstead` feature really needs to be adamant about the working\ndirectory being clean.\n\nCiao,\nJohannes\n"},{"id":"251800","messageId":"cover.1415876330.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":"cover.1415629053.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 0/1] Support updating working trees when pushing into non-bare repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-13T11:03:41Z","receivedAt":"2014-11-13T11:03:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series adds support for a new receive.denyCurrentBranch setting\nto update the working directory (which must be clean, i.e. there must not be\nany uncommitted changes) when pushing into the current branch.\n\nThe scenario in which the 'updateInstead' setting became a boon in this\ndeveloper's daily work is when trying to get a bug fix from a Windows\ncomputer, a virtual machine or a user's machine onto his main machine (in\nall of those cases it is only possible to connect via ssh in one direction,\nbut not in the reverse direction).\n\nInterdiff vs v2 below the diffstat.\n\nJohannes Schindelin (1):\n Add another option for receive.denyCurrentBranch\n\n Documentation/config.txt | 5 ++++\n builtin/receive-pack.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t5516-fetch-push.sh | 17 +++++++++++\n 3 files changed, 98 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4f9fe81..c384515 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2134,10 +2134,6 @@ Another option is \"updateInstead\" which will update the working\n directory (must be clean) if pushing into the current branch. This option is\n intended for synchronizing working directories when one side is not easily\n accessible via ssh (e.g. inside a VM).\n-+\n-Yet another option is \"detachInstead\" which will detach the HEAD if updates\n-are pushed into the current branch; That way, the current revision, the\n-index and the working directory are always left untouched by pushes.\n \n receive.denyNonFastForwards::\n \tIf set to true, git-receive-pack will deny a ref update which is\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 4534e88..836720f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -27,8 +27,7 @@ enum deny_action {\n \tDENY_IGNORE,\n \tDENY_WARN,\n \tDENY_REFUSE,\n-\tDENY_UPDATE_INSTEAD,\n-\tDENY_DETACH_INSTEAD\n+\tDENY_UPDATE_INSTEAD\n };\n \n static int deny_deletes;\n@@ -78,6 +77,8 @@ static enum deny_action parse_deny_action(const char *var, const char *value)\n \t\t\treturn DENY_WARN;\n \t\tif (!strcasecmp(value, \"refuse\"))\n \t\t\treturn DENY_REFUSE;\n+\t\tif (!strcasecmp(value, \"updateinstead\"))\n+\t\t\treturn DENY_UPDATE_INSTEAD;\n \t}\n \tif (git_config_bool(var, value))\n \t\treturn DENY_REFUSE;\n@@ -122,12 +123,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(var, \"receive.denycurrentbranch\")) {\n-\t\tif (value && !strcasecmp(value, \"updateinstead\"))\n-\t\t\tdeny_current_branch = DENY_UPDATE_INSTEAD;\n-\t\telse if (value && !strcasecmp(value, \"detachinstead\"))\n-\t\t\tdeny_current_branch = DENY_DETACH_INSTEAD;\n-\t\telse\n-\t\t\tdeny_current_branch = parse_deny_action(var, value);\n+\t\tdeny_current_branch = parse_deny_action(var, value);\n \t\treturn 0;\n \t}\n \n@@ -737,36 +733,66 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \treturn 0;\n }\n \n-static const char *merge_worktree(unsigned char *sha1)\n+static const char *update_worktree(unsigned char *sha1)\n {\n \tconst char *update_refresh[] = {\n \t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n \t};\n+\tconst char *diff_index[] = {\n+\t\t\"diff-index\", \"--quiet\", \"--cached\", \"--ignore-submodules\",\n+\t\t\"HEAD\", \"--\", NULL\n+\t};\n \tconst char *read_tree[] = {\n \t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n \t};\n+\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n+\tstruct argv_array env = ARGV_ARRAY_INIT;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \n \tif (is_bare_repository())\n \t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n \n-\targv_array_pushf(&child.env_array, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\n \tchild.argv = update_refresh;\n-\tchild.dir = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n+\tchild.env = env.argv;\n+\tchild.dir = work_tree;\n+\tchild.no_stdin = 1;\n \tchild.stdout_to_stderr = 1;\n \tchild.git_cmd = 1;\n-\tif (run_command(&child))\n-\t\tdie(\"Could not refresh the index\");\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Up-to-date check failed\";\n+\t}\n \n-\t/* finish_command cleared the environment; reinitialize */\n-\targv_array_pushf(&child.env_array, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\t/* run_command() does not clean up completely; reinitialize */\n+\tchild_process_init(&child);\n+\tchild.argv = diff_index;\n+\tchild.env = env.argv;\n+\tchild.no_stdin = 1;\n+\tchild.no_stdout = 1;\n+\tchild.stdout_to_stderr = 0;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Working directory not clean\";\n+\t}\n+\n+\t/* run_command() does not clean up completely; reinitialize */\n+\tchild_process_init(&child);\n \tchild.argv = read_tree;\n+\tchild.env = env.argv;\n+\tchild.dir = work_tree;\n \tchild.no_stdin = 1;\n \tchild.no_stdout = 1;\n \tchild.stdout_to_stderr = 0;\n-\tif (run_command(&child))\n-\t\tdie(\"Could not merge working tree with new HEAD.\");\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Could not update working tree to new HEAD\";\n+\t}\n \n+\targv_array_clear(&env);\n \treturn NULL;\n }\n \n@@ -801,15 +827,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\trefuse_unconfigured_deny();\n \t\t\treturn \"branch is currently checked out\";\n \t\tcase DENY_UPDATE_INSTEAD:\n-\t\t\tret = merge_worktree(new_sha1);\n-\t\t\tif (ret)\n-\t\t\t\treturn ret;\n-\t\t\tbreak;\n-\t\tcase DENY_DETACH_INSTEAD:\n-\t\t\tret = update_ref(\"push into current branch (detach)\",\n-\t\t\t\t\"HEAD\", old_sha1, NULL, REF_NODEREF,\n-\t\t\t\tUPDATE_REFS_DIE_ON_ERR) ?\n-\t\t\t\t\"Could not detach HEAD\" : NULL;\n+\t\t\tret = update_worktree(new_sha1);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t\tbreak;\n@@ -837,12 +855,13 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\tbreak;\n \t\t\tcase DENY_REFUSE:\n \t\t\tcase DENY_UNCONFIGURED:\n+\t\t\tcase DENY_UPDATE_INSTEAD:\n \t\t\t\tif (deny_delete_current == DENY_UNCONFIGURED)\n \t\t\t\t\trefuse_unconfigured_deny_delete_current();\n \t\t\t\trp_error(\"refusing to delete the current branch: %s\", name);\n \t\t\t\treturn \"deletion of the current branch prohibited\";\n \t\t\tdefault:\n-\t\t\t\tdie (\"Invalid denyDeleteCurrent setting\");\n+\t\t\t\treturn \"Invalid denyDeleteCurrent setting\";\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/run-command.c b/run-command.c\nindex 551c6c2..a476999 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -555,9 +555,6 @@ int finish_command(struct child_process *cmd)\n {\n \tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n \targv_array_clear(&cmd->args);\n-\t/* Avoid pointing to a stale environment */\n-\tif (cmd->env == cmd->env_array.argv)\n-\t\tcmd->env = NULL;\n \targv_array_clear(&cmd->env_array);\n \treturn ret;\n }\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 3981d1b..ba002a9 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1347,23 +1347,4 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n \t)\n '\n \n-test_expect_success 'receive.denyCurrentBranch = detachInstead' '\n-\t(cd testrepo &&\n-\t\tgit reset --hard &&\n-\t\tgit config receive.denyCurrentBranch detachInstead\n-\t) &&\n-\tOLDHEAD=$(cd testrepo && git rev-parse HEAD) &&\n-\ttest_commit fourth path2 &&\n-\ttest fourth = \"$(cat path2)\" &&\n-\tgit push testrepo master &&\n-\ttest $OLDHEAD = $(cd testrepo && git rev-parse HEAD) &&\n-\ttest fourth != \"$(cat testrepo/path2)\" &&\n-\t(cd testrepo &&\n-\t\ttest_must_fail git symbolic-ref HEAD &&\n-\t\tgit update-index --refresh &&\n-\t\tgit diff-files --quiet &&\n-\t\tgit diff-index --cached HEAD --\n-\t)\n-'\n-\n test_done\n\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"251801","messageId":"7bd583cb3c7097811cff81cada9720eb30c3585e.1415876330.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":"cover.1415876330.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 1/1] Add another option for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-13T11:03:46Z","receivedAt":"2014-11-13T11:03:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When synchronizing between working directories, it can be handy to update\nthe current branch via 'push' rather than 'pull', e.g. when pushing a fix\nfrom inside a VM, or when pushing a fix made on a user's machine (where\nthe developer is not at liberty to install an ssh daemon let alone know\nthe user's password).\n\nThe common workaround – pushing into a temporary branch and then merging\non the other machine – is no longer necessary with this patch.\n\nThe new option is:\n\n'updateInstead':\n\tUpdate the working tree accordingly, but refuse to do so if there\n\tare any uncommitted changes.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/config.txt | 5 ++++\n builtin/receive-pack.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t5516-fetch-push.sh | 17 +++++++++++\n 3 files changed, 98 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 9220725..c384515 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2129,6 +2129,11 @@ receive.denyCurrentBranch::\n \tprint a warning of such a push to stderr, but allow the push to\n \tproceed. If set to false or \"ignore\", allow such pushes with no\n \tmessage. Defaults to \"refuse\".\n++\n+Another option is \"updateInstead\" which will update the working\n+directory (must be clean) if pushing into the current branch. This option is\n+intended for synchronizing working directories when one side is not easily\n+accessible via ssh (e.g. inside a VM).\n \n receive.denyNonFastForwards::\n \tIf set to true, git-receive-pack will deny a ref update which is\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 32fc540..836720f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -26,7 +26,8 @@ enum deny_action {\n \tDENY_UNCONFIGURED,\n \tDENY_IGNORE,\n \tDENY_WARN,\n-\tDENY_REFUSE\n+\tDENY_REFUSE,\n+\tDENY_UPDATE_INSTEAD\n };\n \n static int deny_deletes;\n@@ -76,6 +77,8 @@ static enum deny_action parse_deny_action(const char *var, const char *value)\n \t\t\treturn DENY_WARN;\n \t\tif (!strcasecmp(value, \"refuse\"))\n \t\t\treturn DENY_REFUSE;\n+\t\tif (!strcasecmp(value, \"updateinstead\"))\n+\t\t\treturn DENY_UPDATE_INSTEAD;\n \t}\n \tif (git_config_bool(var, value))\n \t\treturn DENY_REFUSE;\n@@ -730,11 +733,74 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \treturn 0;\n }\n \n+static const char *update_worktree(unsigned char *sha1)\n+{\n+\tconst char *update_refresh[] = {\n+\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n+\t};\n+\tconst char *diff_index[] = {\n+\t\t\"diff-index\", \"--quiet\", \"--cached\", \"--ignore-submodules\",\n+\t\t\"HEAD\", \"--\", NULL\n+\t};\n+\tconst char *read_tree[] = {\n+\t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n+\t};\n+\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n+\tstruct argv_array env = ARGV_ARRAY_INIT;\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\n+\tif (is_bare_repository())\n+\t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n+\n+\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\n+\tchild.argv = update_refresh;\n+\tchild.env = env.argv;\n+\tchild.dir = work_tree;\n+\tchild.no_stdin = 1;\n+\tchild.stdout_to_stderr = 1;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Up-to-date check failed\";\n+\t}\n+\n+\t/* run_command() does not clean up completely; reinitialize */\n+\tchild_process_init(&child);\n+\tchild.argv = diff_index;\n+\tchild.env = env.argv;\n+\tchild.no_stdin = 1;\n+\tchild.no_stdout = 1;\n+\tchild.stdout_to_stderr = 0;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Working directory not clean\";\n+\t}\n+\n+\t/* run_command() does not clean up completely; reinitialize */\n+\tchild_process_init(&child);\n+\tchild.argv = read_tree;\n+\tchild.env = env.argv;\n+\tchild.dir = work_tree;\n+\tchild.no_stdin = 1;\n+\tchild.no_stdout = 1;\n+\tchild.stdout_to_stderr = 0;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Could not update working tree to new HEAD\";\n+\t}\n+\n+\targv_array_clear(&env);\n+\treturn NULL;\n+}\n+\n static const char *update(struct command *cmd, struct shallow_info *si)\n {\n \tconst char *name = cmd->ref_name;\n \tstruct strbuf namespaced_name_buf = STRBUF_INIT;\n-\tconst char *namespaced_name;\n+\tconst char *namespaced_name, *ret;\n \tunsigned char *old_sha1 = cmd->old_sha1;\n \tunsigned char *new_sha1 = cmd->new_sha1;\n \n@@ -760,6 +826,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\tif (deny_current_branch == DENY_UNCONFIGURED)\n \t\t\t\trefuse_unconfigured_deny();\n \t\t\treturn \"branch is currently checked out\";\n+\t\tcase DENY_UPDATE_INSTEAD:\n+\t\t\tret = update_worktree(new_sha1);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n+\t\t\tbreak;\n \t\t}\n \t}\n \n@@ -784,10 +855,13 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\tbreak;\n \t\t\tcase DENY_REFUSE:\n \t\t\tcase DENY_UNCONFIGURED:\n+\t\t\tcase DENY_UPDATE_INSTEAD:\n \t\t\t\tif (deny_delete_current == DENY_UNCONFIGURED)\n \t\t\t\t\trefuse_unconfigured_deny_delete_current();\n \t\t\t\trp_error(\"refusing to delete the current branch: %s\", name);\n \t\t\t\treturn \"deletion of the current branch prohibited\";\n+\t\t\tdefault:\n+\t\t\t\treturn \"Invalid denyDeleteCurrent setting\";\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f4da20a..ba002a9 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1330,4 +1330,21 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '\n \t)\n '\n \n+test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n+\tgit push testrepo master &&\n+\t(cd testrepo &&\n+\t\tgit reset --hard &&\n+\t\tgit config receive.denyCurrentBranch updateInstead\n+\t) &&\n+\ttest_commit third path2 &&\n+\tgit push testrepo master &&\n+\ttest $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&\n+\ttest third = \"$(cat testrepo/path2)\" &&\n+\t(cd testrepo &&\n+\t\tgit update-index --refresh &&\n+\t\tgit diff-files --quiet &&\n+\t\tgit diff-index --cached HEAD --\n+\t)\n+'\n+\n test_done\n-- \n2.0.0.rc3.9669.g840d1f9"},{"id":"251827","messageId":"xmqqh9y32e36.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1411131136200.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-13T17:41:01Z","receivedAt":"2014-11-13T17:41:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 13 Nov 2014, Johannes Schindelin wrote:\n>\n>> Due to that experience, the documentation also states pretty clearly that\n>> `updateInstead` succeeds only in updating the current branch if the\n>> working directory is clean.\n>\n> To clarify why `updateInstead` is stricter than the `merge` scenario: when\n> pushing into a remote repository with attached working directory, we\n> cannot simply clean up merge conflicts or other problems introduced by a\n> merge. Indeed, it is even possible to push without having any option to\n> fix files in the working directory afterwards. Therefore, the\n> `updateInstead` feature really needs to be adamant about the working\n> directory being clean.\n\nAs the stricter check (which I think is unnecessarily strict and\nwhich you don't) can be loosened later without making something the\nuser used to be able to do in an early version unable to do later,\nI am OK to accept the design as-is.\n\nBut after reading this addendum, I feel the need to double check.\n\nA \"reset --keep \", which is the same as \"checkout -B\n \" (note the lack of \"-m\"), does not have\nany \"merge conflict\" issues.\n\nTo see what I mean, follow along this simple experiment.\n\n # Just make sure we start clean\n $ git reset --hard\n\n # Create a playpen\n $ git checkout -b throwaway master\n\n # Pick one random path that is different. We pretend that\n # somebody pushed the commit at the tip of 'next' from the side\n $ git diff --name-only next | head -n 1\n Documentation/git-clone.txt\n\n # Make sure another random path used in the experiment is unchanged\n $ git diff next -- COPYING | wc -l\n 0\n\n # Smudge a path not involved in the branch/HEAD switching\n $ echo >>COPYING\n\n # attempt to updateInstead to the other version succeeds.\n $ git reset --keep next; echo $?\n 0\n $ git status -suno\n M COPYING\n # Notice that we did not get into a funny state.\n # You can verify it with \"git diff\".\n\n # Go back and try smudging what would need a real merge\n $ git reset --hard master\n $ echo >>Documentation/git-clone.txt\n\n # attempt to updateInstead to the other version\n $ git reset --keep next; echo $?\n error: Entry 'Documentation/git-clone.txt' not uptodate. Cannot merge.\n fatal: Could not reset index file to revision 'next'.\n 128\n\n # See that HEAD did not move\n $ git log HEAD...master | wc -l\n 0\n\n # See that the working tree change is intact\n $ git status -suno\n M Documentation/git-clone.txt\n # Notice that we did not get into a funny state.\n # You can verify it with \"git diff\".\n\nThe semantics is just as safe as \"checkout \" without\nthe \"-m\" option; if a local change may be clobbered or needs real\nfile-level merge, the operation fails without touching anything to\npreserve the local state.\n\nIt is OK for us to agree to disagree, and I am willing to accept the\nstricter design for an initial version because loosening it later is\npossible without harming users.\n\nBut I wanted to first make sure that your disagreement is an\ninformed one. Do you still feel the need for \"absolutely clean\",\neven if you take the safety built into \"reset --keep\" shown in the\nabove experiment into account?\n"},{"id":"251828","messageId":"xmqqfvdn2e2u.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1411131136200.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-13T17:41:13Z","receivedAt":"2014-11-13T17:41:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 13 Nov 2014, Johannes Schindelin wrote:\n>\n>> Due to that experience, the documentation also states pretty clearly that\n>> `updateInstead` succeeds only in updating the current branch if the\n>> working directory is clean.\n>\n> To clarify why `updateInstead` is stricter than the `merge` scenario: when\n> pushing into a remote repository with attached working directory, we\n> cannot simply clean up merge conflicts or other problems introduced by a\n> merge. Indeed, it is even possible to push without having any option to\n> fix files in the working directory afterwards. Therefore, the\n> `updateInstead` feature really needs to be adamant about the working\n> directory being clean.\n\nAs the stricter check (which I think is unnecessarily strict and\nwhich you don't) can be loosened later without making something the\nuser used to be able to do in an early version unable to do later,\nI am OK to accept the design as-is.\n\nBut after reading this addendum, I feel the need to double check.\n\nA \"reset --keep \", which is the same as \"checkout -B\n \" (note the lack of \"-m\"), does not have\nany \"merge conflict\" issues.\n\nTo see what I mean, follow along this simple experiment.\n\n # Just make sure we start clean\n $ git reset --hard\n\n # Create a playpen\n $ git checkout -b throwaway master\n\n # Pick one random path that is different. We pretend that\n # somebody pushed the commit at the tip of 'next' from the side\n $ git diff --name-only next | head -n 1\n Documentation/git-clone.txt\n\n # Make sure another random path used in the experiment is unchanged\n $ git diff next -- COPYING | wc -l\n 0\n\n # Smudge a path not involved in the branch/HEAD switching\n $ echo >>COPYING\n\n # attempt to updateInstead to the other version succeeds.\n $ git reset --keep next; echo $?\n 0\n $ git status -suno\n M COPYING\n # Notice that we did not get into a funny state.\n # You can verify it with \"git diff\".\n\n # Go back and try smudging what would need a real merge\n $ git reset --hard master\n $ echo >>Documentation/git-clone.txt\n\n # attempt to updateInstead to the other version\n $ git reset --keep next; echo $?\n error: Entry 'Documentation/git-clone.txt' not uptodate. Cannot merge.\n fatal: Could not reset index file to revision 'next'.\n 128\n\n # See that HEAD did not move\n $ git log HEAD...master | wc -l\n 0\n\n # See that the working tree change is intact\n $ git status -suno\n M Documentation/git-clone.txt\n # Notice that we did not get into a funny state.\n # You can verify it with \"git diff\".\n\nThe semantics is just as safe as \"checkout \" without\nthe \"-m\" option; if a local change may be clobbered or needs real\nfile-level merge, the operation fails without touching anything to\npreserve the local state.\n\nIt is OK for us to agree to disagree, and I am willing to accept the\nstricter design for an initial version because loosening it later is\npossible without harming users.\n\nBut I wanted to first make sure that your disagreement is an\ninformed one. Do you still feel the need for \"absolutely clean\",\neven if you take the safety built into \"reset --keep\" shown in the\nabove demonstration into account?\n"},{"id":"251829","messageId":"xmqqbnob2ds7.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"cover.1415876330.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 0/1] Support updating working trees when pushing into non-bare repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-13T17:47:36Z","receivedAt":"2014-11-13T17:47:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> This patch series adds support for a new receive.denyCurrentBranch setting\n> to update the working directory (which must be clean, i.e. there must not be\n> any uncommitted changes) when pushing into the current branch.\n>\n> The scenario in which the 'updateInstead' setting became a boon in this\n> developer's daily work is when trying to get a bug fix from a Windows\n> computer, a virtual machine or a user's machine onto his main machine (in\n> all of those cases it is only possible to connect via ssh in one direction,\n> but not in the reverse direction).\n>\n> Interdiff vs v2 below the diffstat.\n>\n> Johannes Schindelin (1):\n> Add another option for receive.denyCurrentBranch\n>\n> Documentation/config.txt | 5 ++++\n> builtin/receive-pack.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++--\n> t/t5516-fetch-push.sh | 17 +++++++++++\n> 3 files changed, 98 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 4f9fe81..c384515 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2134,10 +2134,6 @@ Another option is \"updateInstead\" which will update the working\n> directory (must be clean) if pushing into the current branch. This option is\n> intended for synchronizing working directories when one side is not easily\n> accessible via ssh (e.g. inside a VM).\n> -+\n> -Yet another option is \"detachInstead\" which will detach the HEAD if updates\n> -are pushed into the current branch; That way, the current revision, the\n> -index and the working directory are always left untouched by pushes.\n\nI think we had an exchange to clarify the workflow in which\nupdateInstead is useful and how to help readers, but I do not see\nany change on that in this part of documentation. Forgot to revise?\n\n> @@ -737,36 +733,66 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n> \treturn 0;\n> }\n> \n> -static const char *merge_worktree(unsigned char *sha1)\n> +static const char *update_worktree(unsigned char *sha1)\n> {\n> \tconst char *update_refresh[] = {\n> \t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n> \t};\n> +\tconst char *diff_index[] = {\n> +\t\t\"diff-index\", \"--quiet\", \"--cached\", \"--ignore-submodules\",\n> +\t\t\"HEAD\", \"--\", NULL\n> +\t};\n> \tconst char *read_tree[] = {\n> \t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n> \t};\n\nOK.\n\n\"update-index --refresh && diff-files && diff-index --cached\" is how\nwe traditionally ensure the working tree is absolutely clean (see\nrequire_clean_work_tree in git-sh-setup.sh), but I do not think of a\nreason why diff-files step is not redundant. As a totally separate\ntopic outside this series, we may want to visit that shell function.\n"},{"id":"251830","messageId":"xmqq7fyz2dlj.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"7bd583cb3c7097811cff81cada9720eb30c3585e.1415876330.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 1/1] Add another option for receive.denyCurrentBranch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-13T17:51:36Z","receivedAt":"2014-11-13T17:51:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index f4da20a..ba002a9 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1330,4 +1330,21 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '\n> \t)\n> '\n> \n> +test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n> +\tgit push testrepo master &&\n> +\t(cd testrepo &&\n> +\t\tgit reset --hard &&\n> +\t\tgit config receive.denyCurrentBranch updateInstead\n> +\t) &&\n> +\ttest_commit third path2 &&\n> +\tgit push testrepo master &&\n> +\ttest $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&\n> +\ttest third = \"$(cat testrepo/path2)\" &&\n> +\t(cd testrepo &&\n> +\t\tgit update-index --refresh &&\n> +\t\tgit diff-files --quiet &&\n> +\t\tgit diff-index --cached HEAD --\n> +\t)\n> +'\n> +\n\nThis new feature has two sides. Update when we can and more\nimportantly fail the update safely. This tests the \"success\" case,\nbut not the \"safely fail\" one.\n\nFor the latter \"test_must_fail git push\" on the sending side, and\n\"original HEAD stays the same and the working tree changes are\npreserved when there are local changes before the push\" on the\nreceiving side needs to be tested.\n\nThanks.\n"},{"id":"251837","messageId":"alpine.DEB.1.00.1411131850510.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqh9y32e36.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-13T18:55:49Z","receivedAt":"2014-11-13T18:55:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 13 Nov 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Thu, 13 Nov 2014, Johannes Schindelin wrote:\n> >\n> >> Due to that experience, the documentation also states pretty clearly that\n> >> `updateInstead` succeeds only in updating the current branch if the\n> >> working directory is clean.\n> >\n> > To clarify why `updateInstead` is stricter than the `merge` scenario: when\n> > pushing into a remote repository with attached working directory, we\n> > cannot simply clean up merge conflicts or other problems introduced by a\n> > merge. Indeed, it is even possible to push without having any option to\n> > fix files in the working directory afterwards. Therefore, the\n> > `updateInstead` feature really needs to be adamant about the working\n> > directory being clean.\n> \n> As the stricter check (which I think is unnecessarily strict and\n> which you don't) can be loosened later without making something the\n> user used to be able to do in an early version unable to do later,\n> I am OK to accept the design as-is.\n\nThanks for mentioning this. I would like to ask not to loosen this later.\nLet me try to explain in more detail than before why I think it would make\n*my* life hard if it ever were loosened.\n\nLet's start with a hypothetical change to notes.c, a change, say, that\nuses part of the diff machinery. So I am doing that in my working\ndirectory, happily testing stuff, but not quite happy with the result just\nyet, staging a couple of things after a debug run so I can \"git stash -k\"\naway the debugging statements.\n\nPlease note that while the scenario I just illustrated is purely\nhypothetical, made up from thin air, the workflow and the technique is\nvery much my daily bread, though. I do use \"git stash -k\" quite frequently\nto stash away stuff that I do not want, after staging what I want to keep\nfirst.\n\nNow let's continue by saying that I get interrupted with that notes.c work\nand only come back to it much, much later, say, a couple of months,\nbecause I have been working on a different computer in the meantime,\nsay, fixing Git for Windows bugs like mad.\n\nOkay, so now I am back and I \"git pull\" your 'master'. Now, for the sake\nof the argument, let's say that the pull does not introduce any changes to\nnotes.c, but instead that there has been an improvement in the diff\nmachinery, one that required a change of the diff machinery's API and all\nof a sudden, my code does not compile anymore. It merged cleanly,\nGit is completely groovy, but the code just does not compile any more. Not\na big deal: I can inspect the changes via \"git diff\njunio/master@{1}..junio/master\" and find out very easily what went wrong\nand fix my code, compile, stage, and maybe even commit because the code\nnow works.\n\nSplendid.\n\nBut it would be different if the working directory was on another computer\nthan the one I am operating from. I could not easily access the working\ndirectory, let alone the Git history, if all I did was a \"git push\".\n\nNow let's modify the scenario a little bit further, still. Let's not talk\nabout git.git, but about a project implementing a web application. And\nlet's say that I did not dabble with notes.c but I had to work on the\nproduction setup for a really, really urgent hotfix where the speed of a\nfix was more important than not touching the production setup at all.\n\nPlease note that this is *not* a hypothetical scenario, but a true account\nof what I had to go through a couple of times (don't try this at home, it\ncauses a lot of gray hair).\n\nNow let's assume that I forgot to commit and push, and that nobody noticed\n– and let's just draw the curtain of charity over the reasons why. Also,\nlet's assume that a couple of months later, I am asked to implement a new\nfeature for the very same web application, this time without frantic \"You.\nHave. To. Fix. This. ASAP!\" stuff going on. So I develop this on a test\nmachine, test thoroughly, everything's groovy, and then I push, using the\n'updateInstead' feature introduced by the patch in question (and which was\napplied to that machine's Git at that time, before some joker updated it\nwith the official Git).\n\nAnd now when I try to push, Git complains that the working directory is\nnot clean, which is *just* fine by me, because after further inspection it\nturns out that the uncommitted changes – which are in a different file\nthan the changes introducing my new feature – would have borked the\nproduction system rather thoroughly.\n\nPlease note again that I am talking about something that really happened.\n\nSo I would really like to have 'updateInstead', but only in the safe\nversion, i.e. refusing to update anything but a clean working directory,\nand it would be a major regression for me, affecting my workflow rather\nnegatively, if those strict checks were loosened.\n\nIn my mind, when (and if!) a less strict version is desired, it should be\nadded as another denyCurrentBranch value and 'updateInstead' should be\nunaffected, otherwise, speaking for me personally, all my work in this\npatch series would be for naught.\n\nCiao,\nJohannes"},{"id":"251838","messageId":"xmqqd28q29w2.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"xmqqbnob2ds7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 0/1] Support updating working trees when pushing into non-bare repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-13T19:11:41Z","receivedAt":"2014-11-13T19:11:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> -static const char *merge_worktree(unsigned char *sha1)\n>> +static const char *update_worktree(unsigned char *sha1)\n>> {\n>> \tconst char *update_refresh[] = {\n>> \t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n>> \t};\n>> +\tconst char *diff_index[] = {\n>> +\t\t\"diff-index\", \"--quiet\", \"--cached\", \"--ignore-submodules\",\n>> +\t\t\"HEAD\", \"--\", NULL\n>> +\t};\n>> \tconst char *read_tree[] = {\n>> \t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n>> \t};\n>\n> OK.\n>\n> \"update-index --refresh && diff-files && diff-index --cached\" is how\n> we traditionally ensure the working tree is absolutely clean (see\n> require_clean_work_tree in git-sh-setup.sh), but I do not think of a\n> reason why diff-files step is not redundant. As a totally separate\n> topic outside this series, we may want to visit that shell function.\n\nAhh, scratch that. Over there we use 'update-index -q --refresh',\nso the difference between the index and the working tree is not\nnoticed and we do need diff-files. Here, the code runs update-index\nwithout -q, so we do catch such a difference without diff-files.\n\nSorry for the noise ;-)\n"},{"id":"251841","messageId":"alpine.DEB.1.00.1411132001290.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqbnob2ds7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 0/1] Support updating working trees when pushing into non-bare repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-13T19:18:09Z","receivedAt":"2014-11-13T19:18:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 13 Nov 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > This patch series adds support for a new receive.denyCurrentBranch setting\n> > to update the working directory (which must be clean, i.e. there must not be\n> > any uncommitted changes) when pushing into the current branch.\n> >\n> > The scenario in which the 'updateInstead' setting became a boon in this\n> > developer's daily work is when trying to get a bug fix from a Windows\n> > computer, a virtual machine or a user's machine onto his main machine (in\n> > all of those cases it is only possible to connect via ssh in one direction,\n> > but not in the reverse direction).\n> >\n> > Interdiff vs v2 below the diffstat.\n> >\n> > Johannes Schindelin (1):\n> > Add another option for receive.denyCurrentBranch\n> >\n> > Documentation/config.txt | 5 ++++\n> > builtin/receive-pack.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++--\n> > t/t5516-fetch-push.sh | 17 +++++++++++\n> > 3 files changed, 98 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/Documentation/config.txt b/Documentation/config.txt\n> > index 4f9fe81..c384515 100644\n> > --- a/Documentation/config.txt\n> > +++ b/Documentation/config.txt\n> > @@ -2134,10 +2134,6 @@ Another option is \"updateInstead\" which will update the working\n> > directory (must be clean) if pushing into the current branch. This option is\n> > intended for synchronizing working directories when one side is not easily\n> > accessible via ssh (e.g. inside a VM).\n> > -+\n> > -Yet another option is \"detachInstead\" which will detach the HEAD if updates\n> > -are pushed into the current branch; That way, the current revision, the\n> > -index and the working directory are always left untouched by pushes.\n> \n> I think we had an exchange to clarify the workflow in which\n> updateInstead is useful and how to help readers, but I do not see\n> any change on that in this part of documentation. Forgot to revise?\n\nI had revised it for v2 already. It now reads:\n\n-- snip --\nAnother option is \"updateInstead\" which will update the working directory\n(must be clean) if pushing into the current branch. This option is\nintended for synchronizing working directories when one side is not easily\naccessible via ssh (e.g. inside a VM).\n-- snap --\n\nIn my mind, this strikes the balance between sketching a scenario where\nthe setting makes sense on the one hand and abducting config.txt to tell\nmy life's story on the other.\n\n> > @@ -737,36 +733,66 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n> > \treturn 0;\n> > }\n> > \n> > -static const char *merge_worktree(unsigned char *sha1)\n> > +static const char *update_worktree(unsigned char *sha1)\n> > {\n> > \tconst char *update_refresh[] = {\n> > \t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n> > \t};\n> > +\tconst char *diff_index[] = {\n> > +\t\t\"diff-index\", \"--quiet\", \"--cached\", \"--ignore-submodules\",\n> > +\t\t\"HEAD\", \"--\", NULL\n> > +\t};\n> > \tconst char *read_tree[] = {\n> > \t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n> > \t};\n> \n> OK.\n> \n> \"update-index --refresh && diff-files && diff-index --cached\" is how\n> we traditionally ensure the working tree is absolutely clean (see\n> require_clean_work_tree in git-sh-setup.sh), but I do not think of a\n> reason why diff-files step is not redundant.\n\nI fear that my double-negation-fu is still stuck somewhere in dreamland.\nDo you mean to say \"I could imagine that the diff-files step is\nredundant\"? If that is what you are telling me, then your explanation of\nthe exit code of update-index --refresh would suggest so, and so would\nhttps://github.com/git/git/blob/f5709437/read-cache.c#L1201-L1230 *except*\nin the case where refresh_cache_ent() returns an updated cache entry:\nhttps://github.com/git/git/blob/f5709437/read-cache.c#L1116-L1131 – but I\ncould not figure out quickly when this code path is hit.\n\nCiao,\nJohannes"},{"id":"251842","messageId":"alpine.DEB.1.00.1411132020570.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqq7fyz2dlj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/1] Add another option for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-13T19:21:52Z","receivedAt":"2014-11-13T19:21:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 13 Nov 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> > index f4da20a..ba002a9 100755\n> > --- a/t/t5516-fetch-push.sh\n> > +++ b/t/t5516-fetch-push.sh\n> > @@ -1330,4 +1330,21 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '\n> > \t)\n> > '\n> > \n> > +test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n> > +\tgit push testrepo master &&\n> > +\t(cd testrepo &&\n> > +\t\tgit reset --hard &&\n> > +\t\tgit config receive.denyCurrentBranch updateInstead\n> > +\t) &&\n> > +\ttest_commit third path2 &&\n> > +\tgit push testrepo master &&\n> > +\ttest $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&\n> > +\ttest third = \"$(cat testrepo/path2)\" &&\n> > +\t(cd testrepo &&\n> > +\t\tgit update-index --refresh &&\n> > +\t\tgit diff-files --quiet &&\n> > +\t\tgit diff-index --cached HEAD --\n> > +\t)\n> > +'\n> > +\n> \n> This new feature has two sides. Update when we can and more\n> importantly fail the update safely. This tests the \"success\" case,\n> but not the \"safely fail\" one.\n> \n> For the latter \"test_must_fail git push\" on the sending side, and\n> \"original HEAD stays the same and the working tree changes are\n> preserved when there are local changes before the push\" on the\n> receiving side needs to be tested.\n\nRight.\n\nI have amended this for the upcoming v4 (but I'll wait whether there are\nother things I need to change before submitting that):\n\n-- snipsnap --\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex ba002a9..b8df39c 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1343,8 +1343,12 @@ test_expect_success 'receive.denyCurrentBranch =\nupdateInstead' '\n \t(cd testrepo &&\n \t\tgit update-index --refresh &&\n \t\tgit diff-files --quiet &&\n-\t\tgit diff-index --cached HEAD --\n-\t)\n+\t\tgit diff-index --cached HEAD -- &&\n+\t\techo changed > path2 &&\n+\t\tgit add path2\n+\t) &&\n+\ttest_commit fourth path2 &&\n+\ttest_must_fail git push testrepo master\n '\n \n test_done\n"},{"id":"251844","messageId":"xmqq4mu2285w.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1411131850510.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-13T19:48:59Z","receivedAt":"2014-11-13T19:48:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Thanks for mentioning this. I would like to ask not to loosen this later.\n> Let me try to explain in more detail than before why I think it would make\n> *my* life hard if it ever were loosened.\n> ...\n> And now when I try to push, Git complains that the working directory is\n> not clean, which is *just* fine by me, because after further inspection it\n> turns out that the uncommitted changes – which are in a different file\n> than the changes introducing my new feature – would have borked the\n> production system rather thoroughly.\n> ...\n> In my mind, when (and if!) a less strict version is desired, it should be\n> added as another denyCurrentBranch value and 'updateInstead' should be\n> unaffected, otherwise, speaking for me personally, all my work in this\n> patch series would be for naught.\n\nThanks for an explanation why the updateInstead mode must require a\npristine working tree (and the index). I *now* agree why such a\nmode would be useful, *after* reading it. I did not understand why\n*after* reading only the patch, the documentation updates and the\nlog message.\n\nThat tells us something, doesn't it? ;-)\n\nAlso the failure case test must protect us from making a change you\nfear in the future. The interdiff you sent in a separate message\nwas to smudge path2 that is modified in the 'fourth' commit, which\nshould fail with either your \"OK only if really clean\" requirement\nor the other \"OK as long as it does not interfere with the switch\"\ncriterion. Checking that is a good step, but you would want to have\na separate \"Smudge a path that is unchanged with the switch and see\nthe push updateInstead fail\" to protect the current semantics.\n\nI agree that a new value \"mergeInstead\" or something should be\ninvented when/if different workflows want a looser semantics.\nPeople would rely not only on \"being able to push when clean\" but\nalso on \"being safely prevented from pushing when not\" (and that is\nwhere my earlier comment to test both sides comes from). Loosening\nthe check to an existing \"updateInstead\" would break users who have\nbeen using updateInstead.\n\nAlso I suspect that people would want to send a patch to add \"-q\" to\nyour \"update-index --refresh\" invocation, at which time you would\nneed to add a call to \"diff-files\" to check that the working tree\nand the index match. You may want to add that \"diff-files\" now, or\nat least have a test that is designed to break when \"-q\" is added to\n\"update-index --refresh\" without adding the necessary \"diff-files\".\n\nThanks.\n"},{"id":"251849","messageId":"xmqqzjbuzu6t.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"xmqq4mu2285w.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-13T21:06:50Z","receivedAt":"2014-11-13T21:06:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Thanks for an explanation why the updateInstead mode must require a\n> pristine working tree (and the index). I *now* agree why such a\n> mode would be useful, *after* reading it. I did not understand why\n> *after* reading only the patch, the documentation updates and the\n> log message.\n>\n> That tells us something, doesn't it? ;-)\n\nJust to be less cryptic, I meant that the documentation since v2 is\nprobably insufficient.\n\n> I agree that a new value \"mergeInstead\" or something should be\n> invented when/if different workflows want a looser semantics.\n> People would rely not only on \"being able to push when clean\" but\n> also on \"being safely prevented from pushing when not\" (and that is\n> where my earlier comment to test both sides comes from). Loosening\n> the check to an existing \"updateInstead\" would break users who have\n> been using updateInstead.\n\nAnd thinking about the names again, I have a feeling that\nupdateInstead and mergeInstead are both probably misnomer. The\n\"make sure there is no modification and then do an equivalent of\nreset --hard there\" option makes sense only in push-to-deploy\nscenario (perhaps \"resetInstead\"?) Compared to that, \"do an\nequivalent of checkout as long as it can be done without involving\nreal merges\" sounds more deserving the \"updateInstead\" name.\n\nBut I am not very good at names (and I suspect you feel you yourself\naren't, either), so perhaps somebody listening from the sideline can\nchime in.\n"},{"id":"251875","messageId":"xmqq1tp643yb.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"xmqqzjbuzu6t.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-14T07:49:16Z","receivedAt":"2014-11-14T07:49:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> I agree that a new value \"mergeInstead\" or something should be\n>> invented when/if different workflows want a looser semantics.\n>> People would rely not only on \"being able to push when clean\" but\n>> also on \"being safely prevented from pushing when not\" (and that is\n>> where my earlier comment to test both sides comes from). Loosening\n>> the check to an existing \"updateInstead\" would break users who have\n>> been using updateInstead.\n>\n> And thinking about the names again, I have a feeling that\n> updateInstead and mergeInstead are both probably misnomer.\n\nLet me take this part back. After all, I do not think I would\ndesign the mechanism to implement an alternative logic that decides\nwhen it is safe to allow the update of the ref and to reflect the\nchanges to the working tree, and that actually does the checkout to\nthe working tree by using a new value like mergeInstead. So we\nwould only need a single name, and updateInstead is not too bad.\n\nThe word \"update\" is so heavily loaded in the context of accepting a\npush (i.e. it is unclear what update we are talking about---updating\nthe ref that we normally refuse to update? updating the index?\nupdating the working tree? Some combination of them?), so as a\nsingle word, \"checkoutInstead\" may probably be a better one, though.\nUpon hearing \"checkout\", by definition anybody would know that we\nare involving the working tree.\n\nThe mechanism I would employ when doing an alternative logic,\npossibly looser one but does not necessarily so, would be to have a\nhook script \"push-to-checkout\". When denyCurrentBranch is set to\nupdateInstead, if there is no such hook, the \"working tree has to be\nabsolutely clean and we would do a 'read-tree -m -u $old $new'\n(which is the same as 'reset --hard $new' under the precondition)\"\nyou implemented will be used as the \"logic that decides when it is\nsafe, and that does the checkout to the working tree\". When the\n\"push-to-checkout\" hook exists, however, we just invoke that hook\nwith $new as argument, and it can decide when it is safe in whatever\nway it chooses to, and it can checkout the $new to the working tree\nin whatever way it wants. The users of \"mergeInstead\" (now a dead\nand unnecessary name) mode would just have a single-liner\n\n\t#!/bin/sh\n\tgit reset --keep \"$1\" --\n\nin there, as this single command would both decide when it is safe\nand does the safe (according to its own definition) updating of the\nworking tree.\n\nIn your other example (not the \"deploy to live website\" one) of\nunidirectional SSH connection, you would be able to connect from\nmachine A to machine B but not the other way, so while sitting on\nmachine A you would typically have one SSH session to have an\ninteractive shell session running on machine B. You may have local\nmodification on machine B but your changes to history on machine A\ncannot be pulled, so you would emulate it by pushing from A into B.\nIn such a case, unlike the \"live website\" example, it would be\nuseful to loosen the condition even more than \"reset --keep\" (which\nis an equivalent of \"checkout -B $current_branch $new_commit\").\n\nIn such a case, what you want to do is to simulate \"git pull\" that\ncould conflict and give you a chance to resolve with a push in the\nreverse direction. You want to run an equivalent of the same\n\"checkout -B\" command but with the \"-m\" option when accepting such a\npush.\n\nThere are other definitions of what is safe and how update should\nhappen depending on the user, and such a logic can be placed in the\npush-to-checkout hook without harming other users.\n"},{"id":"252598","messageId":"cover.1417033080.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":"cover.1415876330.git.johannes.schindelin@gmx.de","subject":"[PATCH v4] Support updating working trees when pushing into non-bare repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-26T20:21:32Z","receivedAt":"2014-11-26T20:21:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch \"series\" adds support for a new receive.denyCurrentBranch setting\nto update the working directory (which must be clean, i.e. there must not be\nany uncommitted changes) when pushing into the current branch.\n\nThe scenario in which the 'updateInstead' setting became a boon in this\ndeveloper's daily work is when trying to get a bug fix from a Windows\ncomputer, a virtual machine, or when getting a bug fix from a user's machine\nonto his main machine (in all of those cases it is only possible to connect\nvia ssh in one direction, but not in the reverse direction). It also comes\nin handy when updating a live web site via push (in which case a clean\nworking directory is an absolute must).\n\nAs to the name 'updateInstead': since I do not want the option to perform\nthe equivalent of a checkout (where staged changes would be okay), I stuck\nto the name I use in all of my $HOME/.gitconfigs. Hopefully you don't\nmind.\n\nInterdiff vs v3 below the diffstat\n\nJohannes Schindelin (1):\n Add another option for receive.denyCurrentBranch\n\n Documentation/config.txt | 7 ++++\n builtin/receive-pack.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t5516-fetch-push.sh | 21 +++++++++++\n 3 files changed, 120 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex c384515..0519073 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2133,7 +2133,9 @@ receive.denyCurrentBranch::\n Another option is \"updateInstead\" which will update the working\n directory (must be clean) if pushing into the current branch. This option is\n intended for synchronizing working directories when one side is not easily\n-accessible via ssh (e.g. inside a VM).\n+accessible via interactive ssh (e.g. a live web site, hence the requirement\n+that the working directory be clean). This mode also comes in handy when\n+developing inside a VM to test and fix code on different Operating Systems.\n \n receive.denyNonFastForwards::\n \tIf set to true, git-receive-pack will deny a ref update which is\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 836720f..1b05e4e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -736,7 +736,10 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n static const char *update_worktree(unsigned char *sha1)\n {\n \tconst char *update_refresh[] = {\n-\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", NULL\n+\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", \"-q\", NULL\n+\t};\n+\tconst char *diff_files[] = {\n+\t\t\"diff-files\", \"--quiet\", \"--ignore-submodules\", \"--\", NULL\n \t};\n \tconst char *diff_index[] = {\n \t\t\"diff-index\", \"--quiet\", \"--cached\", \"--ignore-submodules\",\n@@ -767,6 +770,19 @@ static const char *update_worktree(unsigned char *sha1)\n \n \t/* run_command() does not clean up completely; reinitialize */\n \tchild_process_init(&child);\n+\tchild.argv = diff_files;\n+\tchild.env = env.argv;\n+\tchild.dir = work_tree;\n+\tchild.no_stdin = 1;\n+\tchild.stdout_to_stderr = 1;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Working directory not clean\";\n+\t}\n+\n+\t/* run_command() does not clean up completely; reinitialize */\n+\tchild_process_init(&child);\n \tchild.argv = diff_index;\n \tchild.env = env.argv;\n \tchild.no_stdin = 1;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex ba002a9..b8df39c 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1343,8 +1343,12 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n \t(cd testrepo &&\n \t\tgit update-index --refresh &&\n \t\tgit diff-files --quiet &&\n-\t\tgit diff-index --cached HEAD --\n-\t)\n+\t\tgit diff-index --cached HEAD -- &&\n+\t\techo changed > path2 &&\n+\t\tgit add path2\n+\t) &&\n+\ttest_commit fourth path2 &&\n+\ttest_must_fail git push testrepo master\n '\n \n test_done\n\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"252599","messageId":"2b44016c4a233348ef0c7600ca4aaf8aa50948e8.1417033080.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":"cover.1417033080.git.johannes.schindelin@gmx.de","subject":"[PATCH v4] Add another option for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-26T20:21:36Z","receivedAt":"2014-11-26T20:21:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When synchronizing between working directories, it can be handy to update\nthe current branch via 'push' rather than 'pull', e.g. when pushing a fix\nfrom inside a VM, or when pushing a fix made on a user's machine (where\nthe developer is not at liberty to install an ssh daemon let alone know\nthe user's password).\n\nThe common workaround – pushing into a temporary branch and then merging\non the other machine – is no longer necessary with this patch.\n\nThe new option is:\n\n'updateInstead':\n\tUpdate the working tree accordingly, but refuse to do so if there\n\tare any uncommitted changes.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/config.txt | 7 ++++\n builtin/receive-pack.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t5516-fetch-push.sh | 21 +++++++++++\n 3 files changed, 120 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 9220725..0519073 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2129,6 +2129,13 @@ receive.denyCurrentBranch::\n \tprint a warning of such a push to stderr, but allow the push to\n \tproceed. If set to false or \"ignore\", allow such pushes with no\n \tmessage. Defaults to \"refuse\".\n++\n+Another option is \"updateInstead\" which will update the working\n+directory (must be clean) if pushing into the current branch. This option is\n+intended for synchronizing working directories when one side is not easily\n+accessible via interactive ssh (e.g. a live web site, hence the requirement\n+that the working directory be clean). This mode also comes in handy when\n+developing inside a VM to test and fix code on different Operating Systems.\n \n receive.denyNonFastForwards::\n \tIf set to true, git-receive-pack will deny a ref update which is\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e908d07..7ff8be5 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -26,7 +26,8 @@ enum deny_action {\n \tDENY_UNCONFIGURED,\n \tDENY_IGNORE,\n \tDENY_WARN,\n-\tDENY_REFUSE\n+\tDENY_REFUSE,\n+\tDENY_UPDATE_INSTEAD\n };\n \n static int deny_deletes;\n@@ -76,6 +77,8 @@ static enum deny_action parse_deny_action(const char *var, const char *value)\n \t\t\treturn DENY_WARN;\n \t\tif (!strcasecmp(value, \"refuse\"))\n \t\t\treturn DENY_REFUSE;\n+\t\tif (!strcasecmp(value, \"updateinstead\"))\n+\t\t\treturn DENY_UPDATE_INSTEAD;\n \t}\n \tif (git_config_bool(var, value))\n \t\treturn DENY_REFUSE;\n@@ -730,11 +733,90 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \treturn 0;\n }\n \n+static const char *update_worktree(unsigned char *sha1)\n+{\n+\tconst char *update_refresh[] = {\n+\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", \"-q\", NULL\n+\t};\n+\tconst char *diff_files[] = {\n+\t\t\"diff-files\", \"--quiet\", \"--ignore-submodules\", \"--\", NULL\n+\t};\n+\tconst char *diff_index[] = {\n+\t\t\"diff-index\", \"--quiet\", \"--cached\", \"--ignore-submodules\",\n+\t\t\"HEAD\", \"--\", NULL\n+\t};\n+\tconst char *read_tree[] = {\n+\t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n+\t};\n+\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n+\tstruct argv_array env = ARGV_ARRAY_INIT;\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\n+\tif (is_bare_repository())\n+\t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n+\n+\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\n+\tchild.argv = update_refresh;\n+\tchild.env = env.argv;\n+\tchild.dir = work_tree;\n+\tchild.no_stdin = 1;\n+\tchild.stdout_to_stderr = 1;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Up-to-date check failed\";\n+\t}\n+\n+\t/* run_command() does not clean up completely; reinitialize */\n+\tchild_process_init(&child);\n+\tchild.argv = diff_files;\n+\tchild.env = env.argv;\n+\tchild.dir = work_tree;\n+\tchild.no_stdin = 1;\n+\tchild.stdout_to_stderr = 1;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Working directory not clean\";\n+\t}\n+\n+\t/* run_command() does not clean up completely; reinitialize */\n+\tchild_process_init(&child);\n+\tchild.argv = diff_index;\n+\tchild.env = env.argv;\n+\tchild.no_stdin = 1;\n+\tchild.no_stdout = 1;\n+\tchild.stdout_to_stderr = 0;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Working directory not clean\";\n+\t}\n+\n+\t/* run_command() does not clean up completely; reinitialize */\n+\tchild_process_init(&child);\n+\tchild.argv = read_tree;\n+\tchild.env = env.argv;\n+\tchild.dir = work_tree;\n+\tchild.no_stdin = 1;\n+\tchild.no_stdout = 1;\n+\tchild.stdout_to_stderr = 0;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Could not update working tree to new HEAD\";\n+\t}\n+\n+\targv_array_clear(&env);\n+\treturn NULL;\n+}\n+\n static const char *update(struct command *cmd, struct shallow_info *si)\n {\n \tconst char *name = cmd->ref_name;\n \tstruct strbuf namespaced_name_buf = STRBUF_INIT;\n-\tconst char *namespaced_name;\n+\tconst char *namespaced_name, *ret;\n \tunsigned char *old_sha1 = cmd->old_sha1;\n \tunsigned char *new_sha1 = cmd->new_sha1;\n \n@@ -760,6 +842,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\tif (deny_current_branch == DENY_UNCONFIGURED)\n \t\t\t\trefuse_unconfigured_deny();\n \t\t\treturn \"branch is currently checked out\";\n+\t\tcase DENY_UPDATE_INSTEAD:\n+\t\t\tret = update_worktree(new_sha1);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n+\t\t\tbreak;\n \t\t}\n \t}\n \n@@ -784,10 +871,13 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\tbreak;\n \t\t\tcase DENY_REFUSE:\n \t\t\tcase DENY_UNCONFIGURED:\n+\t\t\tcase DENY_UPDATE_INSTEAD:\n \t\t\t\tif (deny_delete_current == DENY_UNCONFIGURED)\n \t\t\t\t\trefuse_unconfigured_deny_delete_current();\n \t\t\t\trp_error(\"refusing to delete the current branch: %s\", name);\n \t\t\t\treturn \"deletion of the current branch prohibited\";\n+\t\t\tdefault:\n+\t\t\t\treturn \"Invalid denyDeleteCurrent setting\";\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f4da20a..b8df39c 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1330,4 +1330,25 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '\n \t)\n '\n \n+test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n+\tgit push testrepo master &&\n+\t(cd testrepo &&\n+\t\tgit reset --hard &&\n+\t\tgit config receive.denyCurrentBranch updateInstead\n+\t) &&\n+\ttest_commit third path2 &&\n+\tgit push testrepo master &&\n+\ttest $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&\n+\ttest third = \"$(cat testrepo/path2)\" &&\n+\t(cd testrepo &&\n+\t\tgit update-index --refresh &&\n+\t\tgit diff-files --quiet &&\n+\t\tgit diff-index --cached HEAD -- &&\n+\t\techo changed > path2 &&\n+\t\tgit add path2\n+\t) &&\n+\ttest_commit fourth path2 &&\n+\ttest_must_fail git push testrepo master\n+'\n+\n test_done\n-- \n2.0.0.rc3.9669.g840d1f9"},{"id":"252602","messageId":"xmqqppc9ptf3.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"2b44016c4a233348ef0c7600ca4aaf8aa50948e8.1417033080.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v4] Add another option for receive.denyCurrentBranch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-26T21:02:08Z","receivedAt":"2014-11-26T21:02:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> +Another option is \"updateInstead\" which will update the working\n> +directory (must be clean) if pushing into the current branch. This option is\n> +intended for synchronizing working directories when one side is not easily\n> +accessible via interactive ssh (e.g. a live web site, hence the requirement\n> +that the working directory be clean). This mode also comes in handy when\n> +developing inside a VM to test and fix code on different Operating Systems.\n\nThanks; this explains the intent very clearly.\n\n> @@ -730,11 +733,90 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n> \treturn 0;\n> }\n> \n> +static const char *update_worktree(unsigned char *sha1)\n> +{\n> +\tconst char *update_refresh[] = {\n> +\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", \"-q\", NULL\n> +\t};\n\nPlease have \"-q\" as the first parameter.\n\n $ git reset --hard\n $ echo >Makefile\n $ git update-index -q --refresh ; echo $?\n 0\n $ git update-index --refresh -q ; echo $?\n Makefile: needs update\n 1\n\nYes, I understand and agree that this is a \"WAT???\".\n\nBut update-index has processed its options from left to right from\nthe beginning of time, which we may want to change someday, but this\ntopic is more important than updating that \"WAT???\".\n\n> +...\n> +\tconst char *read_tree[] = {\n> +\t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n> +\t};\n\nIs everybody's compiler OK with this initialization with computed\nvalue? Just checking to see if somebody else says \"that would not\nwork for my setting\".\n\n> +\t/* run_command() does not clean up completely; reinitialize */\n> +\tchild_process_init(&child);\n\nI do not think you need that comment. The name run_command() does\nnot imply \"run and clean up for reuse\" at all, at least to me.\n\n> +\tchild.argv = diff_files;\n> +\t...\n> +\tif (run_command(&child)) {\n> +\t\targv_array_clear(&env);\n> +\t\treturn \"Working directory not clean\";\n> +\t}\n> +\n> +\t/* run_command() does not clean up completely; reinitialize */\n> +\tchild_process_init(&child);\n> +\tchild.argv = diff_index;\n> +\t...\n> +\tif (run_command(&child)) {\n> +\t\targv_array_clear(&env);\n> +\t\treturn \"Working directory not clean\";\n> +\t}\n\nDo we want to give the same message for these two cases?\n\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index f4da20a..b8df39c 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1330,4 +1330,25 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '\n> \t)\n> '\n> \n> +test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n> +\tgit push testrepo master &&\n> +\t(cd testrepo &&\n> +\t\tgit reset --hard &&\n> +\t\tgit config receive.denyCurrentBranch updateInstead\n> +\t) &&\n> +\ttest_commit third path2 &&\n> +\tgit push testrepo master &&\n> +\ttest $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&\n> +\ttest third = \"$(cat testrepo/path2)\" &&\n> +\t(cd testrepo &&\n> +\t\tgit update-index --refresh &&\n> +\t\tgit diff-files --quiet &&\n> +\t\tgit diff-index --cached HEAD -- &&\n\nThis \"diff-index\", without \"--quiet\" would not signal if the index\nmatches HEAD with its exit status.\n\n> +\t\techo changed > path2 &&\n\ns/> />/;\n\n> +\t\tgit add path2\n> +\t) &&\n\nThis made sure that the update happens in a clean state.\n\n> +\ttest_commit fourth path2 &&\n> +\ttest_must_fail git push testrepo master\n\nAnd this made sure that the push fails, but does not check what\nhappened on the receiving repository; minimally something like this\nperhaps?\n\n\ttest_must_fail git push testrepo master &&\n test $(git rev-parse HEAD^) = $(git -C testrepo rev-parse HEAD) &&\n\t(\n\t\tcd testrepo &&\n\t\tgit diff --quiet &&\n\t\ttest changed = \"$(cat path2)\"\n\t)\n\nThat is, we expect that \"third\" is still the HEAD in testrepo, there\nis no difference between the working tree and the index (because you\ndid \"git add path2\" over there previously), and path2 still has the\nlocally updated string in the working tree.\n\n> +'\n> +\n> test_done\n\nOther than that, this looks pretty well done.\n\nThanks.\n\nP.S. I'll be doing 2.2 final today, so I won't have time to apply\nthis with local tweaking to address the issues above.\n"},{"id":"252616","messageId":"cover.1417041787.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":"cover.1417033080.git.johannes.schindelin@gmx.de","subject":"[PATCH v5] Support updating working trees when pushing into non-bare repos","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-26T22:44:13Z","receivedAt":"2014-11-26T22:44:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series adds support for a new receive.denyCurrentBranch setting\nto update the working directory (which must be clean, i.e. there must not be\nany uncommitted changes) when pushing into the current branch.\n\nThe scenario in which the 'updateInstead' setting became a boon in this\ndeveloper's daily work is when trying to get a bug fix from a Windows\ncomputer, a virtual machine, or when getting a bug fix from a user's machine\nonto his main machine (in all of those cases it is only possible to connect\nvia ssh in one direction, but not in the reverse direction). It also comes\nin handy when updating a live web site via push (in which case a clean\nworking directory is an absolute must).\n\nInterdiff vs v4 below diffstat.\n\nJohannes Schindelin (1):\n Add another option for receive.denyCurrentBranch\n\n Documentation/config.txt | 7 ++++\n builtin/receive-pack.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t5516-fetch-push.sh | 26 ++++++++++++++\n 3 files changed, 124 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 7ff8be5..bbd9ba3 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -736,7 +736,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n static const char *update_worktree(unsigned char *sha1)\n {\n \tconst char *update_refresh[] = {\n-\t\t\"update-index\", \"--ignore-submodules\", \"--refresh\", \"-q\", NULL\n+\t\t\"update-index\", \"-q\", \"--ignore-submodules\", \"--refresh\", NULL\n \t};\n \tconst char *diff_files[] = {\n \t\t\"diff-files\", \"--quiet\", \"--ignore-submodules\", \"--\", NULL\n@@ -746,7 +746,7 @@ static const char *update_worktree(unsigned char *sha1)\n \t\t\"HEAD\", \"--\", NULL\n \t};\n \tconst char *read_tree[] = {\n-\t\t\"read-tree\", \"-u\", \"-m\", sha1_to_hex(sha1), NULL\n+\t\t\"read-tree\", \"-u\", \"-m\", NULL, NULL\n \t};\n \tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n \tstruct argv_array env = ARGV_ARRAY_INIT;\n@@ -778,10 +778,9 @@ static const char *update_worktree(unsigned char *sha1)\n \tchild.git_cmd = 1;\n \tif (run_command(&child)) {\n \t\targv_array_clear(&env);\n-\t\treturn \"Working directory not clean\";\n+\t\treturn \"Working directory has unstaged changes\";\n \t}\n \n-\t/* run_command() does not clean up completely; reinitialize */\n \tchild_process_init(&child);\n \tchild.argv = diff_index;\n \tchild.env = env.argv;\n@@ -791,10 +790,10 @@ static const char *update_worktree(unsigned char *sha1)\n \tchild.git_cmd = 1;\n \tif (run_command(&child)) {\n \t\targv_array_clear(&env);\n-\t\treturn \"Working directory not clean\";\n+\t\treturn \"Working directory has staged changes\";\n \t}\n \n-\t/* run_command() does not clean up completely; reinitialize */\n+\tread_tree[3] = sha1_to_hex(sha1);\n \tchild_process_init(&child);\n \tchild.argv = read_tree;\n \tchild.env = env.argv;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex b8df39c..7b353d0 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1341,14 +1341,19 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n \ttest $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&\n \ttest third = \"$(cat testrepo/path2)\" &&\n \t(cd testrepo &&\n-\t\tgit update-index --refresh &&\n-\t\tgit diff-files --quiet &&\n-\t\tgit diff-index --cached HEAD -- &&\n-\t\techo changed > path2 &&\n+\t\tgit update-index -q --refresh &&\n+\t\tgit diff-files --quiet -- &&\n+\t\tgit diff-index --quiet --cached HEAD -- &&\n+\t\techo changed >path2 &&\n \t\tgit add path2\n \t) &&\n \ttest_commit fourth path2 &&\n-\ttest_must_fail git push testrepo master\n+\ttest_must_fail git push testrepo master &&\n+\ttest $(git rev-parse HEAD^) = $(git -C testrepo rev-parse HEAD) &&\n+\t(cd testrepo &&\n+\t\tgit diff --quiet &&\n+\t\ttest changed = \"$(cat path2)\"\n+\t)\n '\n \n test_done\n\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"252617","messageId":"ff9c17b14b4418ae9743a094989d5478dab19701.1417041788.git.johannes.schindelin@gmx.de","threadId":"37896","inReplyTo":"cover.1417041787.git.johannes.schindelin@gmx.de","subject":"[PATCH v5] Add another option for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-11-26T22:44:16Z","receivedAt":"2014-11-26T22:44:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When synchronizing between working directories, it can be handy to update\nthe current branch via 'push' rather than 'pull', e.g. when pushing a fix\nfrom inside a VM, or when pushing a fix made on a user's machine (where\nthe developer is not at liberty to install an ssh daemon let alone know\nthe user's password).\n\nThe common workaround – pushing into a temporary branch and then merging\non the other machine – is no longer necessary with this patch.\n\nThe new option is:\n\n'updateInstead':\n\tUpdate the working tree accordingly, but refuse to do so if there\n\tare any uncommitted changes.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/config.txt | 7 ++++\n builtin/receive-pack.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t5516-fetch-push.sh | 26 ++++++++++++++\n 3 files changed, 124 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 9220725..0519073 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2129,6 +2129,13 @@ receive.denyCurrentBranch::\n \tprint a warning of such a push to stderr, but allow the push to\n \tproceed. If set to false or \"ignore\", allow such pushes with no\n \tmessage. Defaults to \"refuse\".\n++\n+Another option is \"updateInstead\" which will update the working\n+directory (must be clean) if pushing into the current branch. This option is\n+intended for synchronizing working directories when one side is not easily\n+accessible via interactive ssh (e.g. a live web site, hence the requirement\n+that the working directory be clean). This mode also comes in handy when\n+developing inside a VM to test and fix code on different Operating Systems.\n \n receive.denyNonFastForwards::\n \tIf set to true, git-receive-pack will deny a ref update which is\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e908d07..bbd9ba3 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -26,7 +26,8 @@ enum deny_action {\n \tDENY_UNCONFIGURED,\n \tDENY_IGNORE,\n \tDENY_WARN,\n-\tDENY_REFUSE\n+\tDENY_REFUSE,\n+\tDENY_UPDATE_INSTEAD\n };\n \n static int deny_deletes;\n@@ -76,6 +77,8 @@ static enum deny_action parse_deny_action(const char *var, const char *value)\n \t\t\treturn DENY_WARN;\n \t\tif (!strcasecmp(value, \"refuse\"))\n \t\t\treturn DENY_REFUSE;\n+\t\tif (!strcasecmp(value, \"updateinstead\"))\n+\t\t\treturn DENY_UPDATE_INSTEAD;\n \t}\n \tif (git_config_bool(var, value))\n \t\treturn DENY_REFUSE;\n@@ -730,11 +733,89 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \treturn 0;\n }\n \n+static const char *update_worktree(unsigned char *sha1)\n+{\n+\tconst char *update_refresh[] = {\n+\t\t\"update-index\", \"-q\", \"--ignore-submodules\", \"--refresh\", NULL\n+\t};\n+\tconst char *diff_files[] = {\n+\t\t\"diff-files\", \"--quiet\", \"--ignore-submodules\", \"--\", NULL\n+\t};\n+\tconst char *diff_index[] = {\n+\t\t\"diff-index\", \"--quiet\", \"--cached\", \"--ignore-submodules\",\n+\t\t\"HEAD\", \"--\", NULL\n+\t};\n+\tconst char *read_tree[] = {\n+\t\t\"read-tree\", \"-u\", \"-m\", NULL, NULL\n+\t};\n+\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n+\tstruct argv_array env = ARGV_ARRAY_INIT;\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\n+\tif (is_bare_repository())\n+\t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n+\n+\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\n+\tchild.argv = update_refresh;\n+\tchild.env = env.argv;\n+\tchild.dir = work_tree;\n+\tchild.no_stdin = 1;\n+\tchild.stdout_to_stderr = 1;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Up-to-date check failed\";\n+\t}\n+\n+\t/* run_command() does not clean up completely; reinitialize */\n+\tchild_process_init(&child);\n+\tchild.argv = diff_files;\n+\tchild.env = env.argv;\n+\tchild.dir = work_tree;\n+\tchild.no_stdin = 1;\n+\tchild.stdout_to_stderr = 1;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Working directory has unstaged changes\";\n+\t}\n+\n+\tchild_process_init(&child);\n+\tchild.argv = diff_index;\n+\tchild.env = env.argv;\n+\tchild.no_stdin = 1;\n+\tchild.no_stdout = 1;\n+\tchild.stdout_to_stderr = 0;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Working directory has staged changes\";\n+\t}\n+\n+\tread_tree[3] = sha1_to_hex(sha1);\n+\tchild_process_init(&child);\n+\tchild.argv = read_tree;\n+\tchild.env = env.argv;\n+\tchild.dir = work_tree;\n+\tchild.no_stdin = 1;\n+\tchild.no_stdout = 1;\n+\tchild.stdout_to_stderr = 0;\n+\tchild.git_cmd = 1;\n+\tif (run_command(&child)) {\n+\t\targv_array_clear(&env);\n+\t\treturn \"Could not update working tree to new HEAD\";\n+\t}\n+\n+\targv_array_clear(&env);\n+\treturn NULL;\n+}\n+\n static const char *update(struct command *cmd, struct shallow_info *si)\n {\n \tconst char *name = cmd->ref_name;\n \tstruct strbuf namespaced_name_buf = STRBUF_INIT;\n-\tconst char *namespaced_name;\n+\tconst char *namespaced_name, *ret;\n \tunsigned char *old_sha1 = cmd->old_sha1;\n \tunsigned char *new_sha1 = cmd->new_sha1;\n \n@@ -760,6 +841,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\tif (deny_current_branch == DENY_UNCONFIGURED)\n \t\t\t\trefuse_unconfigured_deny();\n \t\t\treturn \"branch is currently checked out\";\n+\t\tcase DENY_UPDATE_INSTEAD:\n+\t\t\tret = update_worktree(new_sha1);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n+\t\t\tbreak;\n \t\t}\n \t}\n \n@@ -784,10 +870,13 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\tbreak;\n \t\t\tcase DENY_REFUSE:\n \t\t\tcase DENY_UNCONFIGURED:\n+\t\t\tcase DENY_UPDATE_INSTEAD:\n \t\t\t\tif (deny_delete_current == DENY_UNCONFIGURED)\n \t\t\t\t\trefuse_unconfigured_deny_delete_current();\n \t\t\t\trp_error(\"refusing to delete the current branch: %s\", name);\n \t\t\t\treturn \"deletion of the current branch prohibited\";\n+\t\t\tdefault:\n+\t\t\t\treturn \"Invalid denyDeleteCurrent setting\";\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f4da20a..7b353d0 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1330,4 +1330,30 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '\n \t)\n '\n \n+test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n+\tgit push testrepo master &&\n+\t(cd testrepo &&\n+\t\tgit reset --hard &&\n+\t\tgit config receive.denyCurrentBranch updateInstead\n+\t) &&\n+\ttest_commit third path2 &&\n+\tgit push testrepo master &&\n+\ttest $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&\n+\ttest third = \"$(cat testrepo/path2)\" &&\n+\t(cd testrepo &&\n+\t\tgit update-index -q --refresh &&\n+\t\tgit diff-files --quiet -- &&\n+\t\tgit diff-index --quiet --cached HEAD -- &&\n+\t\techo changed >path2 &&\n+\t\tgit add path2\n+\t) &&\n+\ttest_commit fourth path2 &&\n+\ttest_must_fail git push testrepo master &&\n+\ttest $(git rev-parse HEAD^) = $(git -C testrepo rev-parse HEAD) &&\n+\t(cd testrepo &&\n+\t\tgit diff --quiet &&\n+\t\ttest changed = \"$(cat path2)\"\n+\t)\n+'\n+\n test_done\n-- \n2.0.0.rc3.9669.g840d1f9"},{"id":"252762","messageId":"xmqq1tokrrad.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"ff9c17b14b4418ae9743a094989d5478dab19701.1417041788.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v5] Add another option for receive.denyCurrentBranch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-01T03:18:50Z","receivedAt":"2014-12-01T03:18:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, will queue.\n\nI think we would need a bit more tests to protect the feature from\nfuture changes, if you care about the cleanliness requirement of\nthis feature which is a lot stricter than that of \"git checkout\".\n\nPerhaps like this one on top.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Sun, 30 Nov 2014 17:54:30 -0800\nSubject: [PATCH] t5516: more tests for receive.denyCurrentBranch=updateInstead\n\nThe previous one tests only the case where a path to be updated by\nthe push-to-deploy has an incompatible change in the target's\nworking tree that has already been added to the index, but the\nfeature itself wants to require the working tree to be a lot cleaner\nthan what is tested. Add a handful more tests to protect the\nfeature from future changes that mistakenly (from the viewpoint of\nthe inventor of the feature) loosens the cleanliness requirement,\nnamely:\n\n - A change only to the working tree but not to the index is still a\n change to be protected;\n\n - An untracked file in the working tree that would be overwritten\n by a push-to-deploy needs to be protected;\n\n - A change that happens to make a file identical to what is being\n pushed is still a change to be protected (i.e. the feature's\n cleanliness requirement is more strict than that of checkout).\n\nAlso, test that a stat-only change to the working tree is not a\nreason to reject a push-to-deploy.\n\nSigned-off-by: Junio C Hamano \n---\n t/t5516-fetch-push.sh | 96 ++++++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 87 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 7b353d0..85c7fec 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1332,28 +1332,106 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '\n \n test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n \tgit push testrepo master &&\n-\t(cd testrepo &&\n+\t(\n+\t\tcd testrepo &&\n \t\tgit reset --hard &&\n \t\tgit config receive.denyCurrentBranch updateInstead\n \t) &&\n \ttest_commit third path2 &&\n+\n+\t# Try pushing into a repository with pristine working tree\n \tgit push testrepo master &&\n-\ttest $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&\n-\ttest third = \"$(cat testrepo/path2)\" &&\n-\t(cd testrepo &&\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit update-index -q --refresh &&\n+\t\tgit diff-files --quiet -- &&\n+\t\tgit diff-index --quiet --cached HEAD -- &&\n+\t\ttest third = \"$(cat path2)\" &&\n+\t\ttest $(git -C .. rev-parse HEAD) = $(git rev-parse HEAD)\n+\t) &&\n+\n+\t# Try pushing into a repository with working tree needing a refresh\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit reset --hard HEAD^ &&\n+\t\ttest $(git -C .. rev-parse HEAD^) = $(git rev-parse HEAD) &&\n+\t\ttest-chmtime +100 path1\n+\t) &&\n+\tgit push testrepo master &&\n+\t(\n+\t\tcd testrepo &&\n \t\tgit update-index -q --refresh &&\n \t\tgit diff-files --quiet -- &&\n \t\tgit diff-index --quiet --cached HEAD -- &&\n-\t\techo changed >path2 &&\n-\t\tgit add path2\n+\t\ttest_cmp ../path1 path1 &&\n+\t\ttest third = \"$(cat path2)\" &&\n+\t\ttest $(git -C .. rev-parse HEAD) = $(git rev-parse HEAD)\n \t) &&\n+\n+\t# Update what is to be pushed\n \ttest_commit fourth path2 &&\n+\n+\t# Try pushing into a repository with a dirty working tree\n+\t# (1) the working tree updated\n+\t(\n+\t\tcd testrepo &&\n+\t\techo changed >path1\n+\t) &&\n \ttest_must_fail git push testrepo master &&\n-\ttest $(git rev-parse HEAD^) = $(git -C testrepo rev-parse HEAD) &&\n-\t(cd testrepo &&\n+\t(\n+\t\tcd testrepo &&\n+\t\ttest $(git -C .. rev-parse HEAD^) = $(git rev-parse HEAD) &&\n+\t\tgit diff --quiet --cached &&\n+\t\ttest changed = \"$(cat path1)\"\n+\t) &&\n+\n+\t# (2) the index updated\n+\t(\n+\t\tcd testrepo &&\n+\t\techo changed >path1 &&\n+\t\tgit add path1\n+\t) &&\n+\ttest_must_fail git push testrepo master &&\n+\t(\n+\t\tcd testrepo &&\n+\t\ttest $(git -C .. rev-parse HEAD^) = $(git rev-parse HEAD) &&\n+\t\tgit diff --quiet &&\n+\t\ttest changed = \"$(cat path1)\"\n+\t) &&\n+\n+\t# Introduce a new file in the update\n+\ttest_commit fifth path3 &&\n+\n+\t# (3) the working tree has an untracked file that would interfere\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit reset --hard &&\n+\t\techo changed >path3\n+\t) &&\n+\ttest_must_fail git push testrepo master &&\n+\t(\n+\t\tcd testrepo &&\n+\t\ttest $(git -C .. rev-parse HEAD^^) = $(git rev-parse HEAD) &&\n+\t\tgit diff --quiet &&\n+\t\tgit diff --quiet --cached &&\n+\t\ttest changed = \"$(cat path3)\"\n+\t) &&\n+\n+\t# (4) the target changes to what gets pushed but it still is a change\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit reset --hard &&\n+\t\techo fifth >path3 &&\n+\t\tgit add path3\n+\t) &&\n+\ttest_must_fail git push testrepo master &&\n+\t(\n+\t\tcd testrepo &&\n+\t\ttest $(git -C .. rev-parse HEAD^^) = $(git rev-parse HEAD) &&\n \t\tgit diff --quiet &&\n-\t\ttest changed = \"$(cat path2)\"\n+\t\ttest fifth = \"$(cat path3)\"\n \t)\n+\n '\n \n test_done\n-- \n2.2.0-117-g2c5ea17\n"},{"id":"252770","messageId":"alpine.DEB.1.00.1412010843570.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqq1tokrrad.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5] Add another option for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-01T07:44:48Z","receivedAt":"2014-12-01T07:44:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 30 Nov 2014, Junio C Hamano wrote:\n\n> Thanks, will queue.\n\nThanks!\n\n> I think we would need a bit more tests to protect the feature from\n> future changes, if you care about the cleanliness requirement of\n> this feature which is a lot stricter than that of \"git checkout\".\n> \n> Perhaps like this one on top.\n\nThanks again!\nDscho\n"},{"id":"252812","messageId":"xmqqr3wjnd65.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"ff9c17b14b4418ae9743a094989d5478dab19701.1417041788.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v5] Add another option for receive.denyCurrentBranch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-01T23:49:38Z","receivedAt":"2014-12-01T23:49:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> +static const char *update_worktree(unsigned char *sha1)\n> +{\n> +...\n> +\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n\nI overlooked this one, but is there a reason why this has to look at\nan internal implementatino detail which is git_work_tree_cfg,\ninstead of asking get_git_work_tree()?\n\nI am wondering if that reason is a valid rationale to fix whatever\nmakes get_git_work_tree() unsuitable for this purpose.\n\nCc'ing Duy who has been working on the part of the system to\ndetermine and set-up work-tree and git-dir.\n"},{"id":"252813","messageId":"xmqqk32aoowa.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"xmqqr3wjnd65.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5] Add another option for receive.denyCurrentBranch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-02T00:51:01Z","receivedAt":"2014-12-02T00:51:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> +static const char *update_worktree(unsigned char *sha1)\n>> +{\n>> +...\n>> +\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n>\n> I overlooked this one, but is there a reason why this has to look at\n> an internal implementatino detail which is git_work_tree_cfg,\n> instead of asking get_git_work_tree()?\n>\n> I am wondering if that reason is a valid rationale to fix whatever\n> makes get_git_work_tree() unsuitable for this purpose.\n>\n> Cc'ing Duy who has been working on the part of the system to\n> determine and set-up work-tree and git-dir.\n\nAnswering myself...\n\nThis is because you know receive-pack runs inside the $GIT_DIR,\nwhether it is a bare or non-bare repository, so either core.worktree\npoints at a directory that is otherwise unrelated to the $GIT_DIR\n(but is the correct $GIT_WORK_TREE), or the top of the working tree\nmust be \"..\" for a non-bare repository. I haven't checked the code,\nbut we probably are not even doing the repository/working-tree\ndiscovery to set up the data necessary for get_git_work_tree() to\ngive us a correct answer.\n\nDoing an optional set-up to make get_git_work_tree() work would\ninvolve more damage to the codebase than necessary, I would suspect,\nso let's keep this part of the code as-is.\n\nThat is, unless I am over-estimating the \"damage\" necessary.\n\nThanks.\n"},{"id":"252822","messageId":"xmqqa936ohs3.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"xmqq1tp643yb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-02T03:24:44Z","receivedAt":"2014-12-02T03:24:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> And thinking about the names again, I have a feeling that\n>> updateInstead and mergeInstead are both probably misnomer.\n>\n> Let me take this part back. After all, I do not think I would\n> design the mechanism to implement an alternative logic that decides\n> when it is safe to allow the update of the ref and to reflect the\n> changes to the working tree, and that actually does the checkout to\n> the working tree by using a new value like mergeInstead. So we\n> would only need a single name, and updateInstead is not too bad.\n> ...\n> The mechanism I would employ when doing an alternative logic,\n> possibly looser one but does not necessarily so, would be to have a\n> hook script \"push-to-checkout\". When denyCurrentBranch is set to\n> updateInstead, if there is no such hook, the \"working tree has to be\n> absolutely clean and we would do a 'read-tree -m -u $old $new'\n> (which is the same as 'reset --hard $new' under the precondition)\"\n> you implemented will be used as the \"logic that decides when it is\n> safe, and that does the checkout to the working tree\". When the\n> \"push-to-checkout\" hook exists, however, we just invoke that hook\n> with $new as argument, and it can decide when it is safe in whatever\n> way it chooses to, and it can checkout the $new to the working tree\n> in whatever way it wants.\n\nSo here comes a two-patch series on top of your series (with the\ntest update I sent earlier). As I never do \"push to deploy\" that\nrequires no changes to the working tree or to the index, while I\nhave seem myself in a situation where I have to emulate a \"git pull\"\nwith a \"git push\" in the opposite direction (and work it around if\nthe target of the 'git pull' I wanted to do were the current branch,\nby first pushing into a throw-away branch, because of denyCurrent),\nI could imagine myself using this variant. Having said that, this\nis primarily so that I do not want to forget and discard the brain\ncycles we spent discussing this to the waste, more than that I\ncannot wait to use this feature myself ;-)\n\nThe first one here is a pure refactoring. The second one is the\nreal fun.\n\n-- >8 --\nSubject: [PATCH 1/2] receive-pack: refactor updateInstead codepath\n\nKeep the \"there is nothing to update in a bare repository\", \"when\nthe check and update process runs, here are the GIT_DIR and\nGIT_WORK_TREE\" logic, which will be common regardless of how the\ndecision to update and the actual update are done, in the original\nupdate_worktree() function, and split out the \"working tree and\nthe index must match the original HEAD exactly\" and \"use two-way\nread-tree to update the working tree\" into a new push_to_deploy()\nhelper function. This will allow customizing the logic more cleanly\nand easily.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/receive-pack.c | 53 ++++++++++++++++++++++++++------------------------\n 1 file changed, 28 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c047418..11800cd 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -733,7 +733,9 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \treturn 0;\n }\n \n-static const char *update_worktree(unsigned char *sha1)\n+static const char *push_to_deploy(unsigned char *sha1,\n+\t\t\t\t struct argv_array *env,\n+\t\t\t\t const char *work_tree)\n {\n \tconst char *update_refresh[] = {\n \t\t\"update-index\", \"-q\", \"--ignore-submodules\", \"--refresh\", NULL\n@@ -748,69 +750,70 @@ static const char *update_worktree(unsigned char *sha1)\n \tconst char *read_tree[] = {\n \t\t\"read-tree\", \"-u\", \"-m\", NULL, NULL\n \t};\n-\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n-\tstruct argv_array env = ARGV_ARRAY_INIT;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \n-\tif (is_bare_repository())\n-\t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n-\n-\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n-\n \tchild.argv = update_refresh;\n-\tchild.env = env.argv;\n+\tchild.env = env->argv;\n \tchild.dir = work_tree;\n \tchild.no_stdin = 1;\n \tchild.stdout_to_stderr = 1;\n \tchild.git_cmd = 1;\n-\tif (run_command(&child)) {\n-\t\targv_array_clear(&env);\n+\tif (run_command(&child))\n \t\treturn \"Up-to-date check failed\";\n-\t}\n \n \t/* run_command() does not clean up completely; reinitialize */\n \tchild_process_init(&child);\n \tchild.argv = diff_files;\n-\tchild.env = env.argv;\n+\tchild.env = env->argv;\n \tchild.dir = work_tree;\n \tchild.no_stdin = 1;\n \tchild.stdout_to_stderr = 1;\n \tchild.git_cmd = 1;\n-\tif (run_command(&child)) {\n-\t\targv_array_clear(&env);\n+\tif (run_command(&child))\n \t\treturn \"Working directory has unstaged changes\";\n-\t}\n \n \tchild_process_init(&child);\n \tchild.argv = diff_index;\n-\tchild.env = env.argv;\n+\tchild.env = env->argv;\n \tchild.no_stdin = 1;\n \tchild.no_stdout = 1;\n \tchild.stdout_to_stderr = 0;\n \tchild.git_cmd = 1;\n-\tif (run_command(&child)) {\n-\t\targv_array_clear(&env);\n+\tif (run_command(&child))\n \t\treturn \"Working directory has staged changes\";\n-\t}\n \n \tread_tree[3] = sha1_to_hex(sha1);\n \tchild_process_init(&child);\n \tchild.argv = read_tree;\n-\tchild.env = env.argv;\n+\tchild.env = env->argv;\n \tchild.dir = work_tree;\n \tchild.no_stdin = 1;\n \tchild.no_stdout = 1;\n \tchild.stdout_to_stderr = 0;\n \tchild.git_cmd = 1;\n-\tif (run_command(&child)) {\n-\t\targv_array_clear(&env);\n+\tif (run_command(&child))\n \t\treturn \"Could not update working tree to new HEAD\";\n-\t}\n \n-\targv_array_clear(&env);\n \treturn NULL;\n }\n \n+static const char *update_worktree(unsigned char *sha1)\n+{\n+\tconst char *retval;\n+\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n+\tstruct argv_array env = ARGV_ARRAY_INIT;\n+\n+\tif (is_bare_repository())\n+\t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n+\n+\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\n+\tretval = push_to_deploy(sha1, &env, work_tree);\n+\n+\targv_array_clear(&env);\n+\treturn retval;\n+}\n+\n static const char *update(struct command *cmd, struct shallow_info *si)\n {\n \tconst char *name = cmd->ref_name;\n-- \n2.2.0-141-gd3f4719\n"},{"id":"252823","messageId":"xmqq61duohq7.fsf_-_@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"xmqqa936ohs3.fsf@gitster.dls.corp.google.com","subject":"[PATCH 2/2] receive-pack: support push-to-checkout hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-02T03:25:52Z","receivedAt":"2014-12-02T03:25:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When receive.denyCurrentBranch is set to updateInstead, this hook\ncan be used to override the built-in \"push-to-deploy\" logic, which\ninsists that the working tree and the index must be unchanged\nrelative to HEAD. The hook receives the commit with which the\ntip of the current is going to be updated, and is responsible to\nmake any necessary changes to the working tree and to the index to\nbring them to the desired state when the tip of the current branch\nis updated to the new commit.\n\nFor example, the hook can simply run \"git read-tree -u -m HEAD $1\"\nto the workflow to emulate \"'git fetch' going in the reverse\ndirection with 'git push'\" better than the push-to-deploy logic, as\nthe two-tree form of \"read-tree -u -m\" is essentially the same as\n\"git checkout\" that switches branches while keeping the local\nchanges in the working tree that do not interfere with the\ndifference between the branches.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/receive-pack.c | 19 ++++++++++++++-\n t/t5516-fetch-push.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 81 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 11800cd..fc8ec9c 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -797,6 +797,20 @@ static const char *push_to_deploy(unsigned char *sha1,\n \treturn NULL;\n }\n \n+static const char *push_to_checkout_hook = \"push-to-checkout\";\n+\n+static const char *push_to_checkout(unsigned char *sha1,\n+\t\t\t\t struct argv_array *env,\n+\t\t\t\t const char *work_tree)\n+{\n+\targv_array_pushf(env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n+\tif (run_hook_le(env->argv, push_to_checkout_hook,\n+\t\t\tsha1_to_hex(sha1), NULL))\n+\t\treturn \"push-to-checkout hook declined\";\n+\telse\n+\t\treturn NULL;\n+}\n+\n static const char *update_worktree(unsigned char *sha1)\n {\n \tconst char *retval;\n@@ -808,7 +822,10 @@ static const char *update_worktree(unsigned char *sha1)\n \n \targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n \n-\tretval = push_to_deploy(sha1, &env, work_tree);\n+\tif (!find_hook(push_to_checkout_hook))\n+\t\tretval = push_to_deploy(sha1, &env, work_tree);\n+\telse\n+\t\tretval = push_to_checkout(sha1, &env, work_tree);\n \n \targv_array_clear(&env);\n \treturn retval;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 85c7fec..e4436c1 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1434,4 +1434,67 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n \n '\n \n+test_expect_success 'updateInstead with push-to-checkout hook' '\n+\trm -fr testrepo &&\n+\tgit init testrepo &&\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit pull .. master &&\n+\t\tgit reset --hard HEAD^^ &&\n+\t\tgit tag initial &&\n+\t\tgit config receive.denyCurrentBranch updateInstead &&\n+\t\twrite_script .git/hooks/push-to-checkout <<-\\EOF\n+\t\techo >&2 updating from $(git rev-parse HEAD)\n+\t\techo >&2 updating to \"$1\"\n+\n+\t\tgit update-index -q --refresh &&\n+\t\tgit read-tree -u -m HEAD \"$1\" || {\n+\t\t\tstatus=$?\n+\t\t\techo >&2 read-tree failed\n+\t\t\texit $status\n+\t\t}\n+\t\tEOF\n+\t) &&\n+\n+\t# Try pushing into a pristine\n+\tgit push testrepo master &&\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit diff --quiet &&\n+\t\tgit diff HEAD --quiet &&\n+\t\ttest $(git -C .. rev-parse HEAD) = $(git rev-parse HEAD)\n+\t) &&\n+\n+\t# Try pushing into a repository with conflicting change\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit reset --hard initial &&\n+\t\techo conflicting >path2\n+\t) &&\n+\ttest_must_fail git push testrepo master &&\n+\t(\n+\t\tcd testrepo &&\n+\t\ttest $(git rev-parse initial) = $(git rev-parse HEAD) &&\n+\t\ttest conflicting = \"$(cat path2)\" &&\n+\t\tgit diff-index --quiet --cached HEAD\n+\t) &&\n+\n+\t# Try pushing into a repository with unrelated change\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit reset --hard initial &&\n+\t\techo unrelated >path1 &&\n+\t\techo irrelevant >path5 &&\n+\t\tgit add path5\n+\t) &&\n+\tgit push testrepo master &&\n+\t(\n+\t\tcd testrepo &&\n+\t\ttest \"$(cat path1)\" = unrelated &&\n+\t\ttest \"$(cat path5)\" = irrelevant &&\n+\t\ttest \"$(git diff --name-only --cached HEAD)\" = path5 &&\n+\t\ttest $(git -C .. rev-parse HEAD) = $(git rev-parse HEAD)\n+\t)\n+'\n+\n test_done\n-- \n2.2.0-141-gd3f4719\n"},{"id":"252845","messageId":"alpine.DEB.1.00.1412020909020.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqk32aoowa.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5] Add another option for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-02T08:21:01Z","receivedAt":"2014-12-02T08:21:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 1 Dec 2014, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Johannes Schindelin writes:\n> >\n> >> +static const char *update_worktree(unsigned char *sha1)\n> >> +{\n> >> +...\n> >> +\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n> >\n> > I overlooked this one, but is there a reason why this has to look at\n> > an internal implementatino detail which is git_work_tree_cfg,\n> > instead of asking get_git_work_tree()?\n> >\n> > I am wondering if that reason is a valid rationale to fix whatever\n> > makes get_git_work_tree() unsuitable for this purpose.\n> >\n> > Cc'ing Duy who has been working on the part of the system to\n> > determine and set-up work-tree and git-dir.\n> \n> Answering myself...\n> \n> This is because you know receive-pack runs inside the $GIT_DIR,\n> whether it is a bare or non-bare repository, so either core.worktree\n> points at a directory that is otherwise unrelated to the $GIT_DIR\n> (but is the correct $GIT_WORK_TREE), or the top of the working tree\n> must be \"..\" for a non-bare repository. I haven't checked the code,\n> but we probably are not even doing the repository/working-tree\n> discovery to set up the data necessary for get_git_work_tree() to\n> give us a correct answer.\n\nExcellent analysis. Indeed, we do not enter the\nsetup_git_directory_gently() path that would interpret core.worktree and\ncall set_git_work_tree() explicitly. Instead, we are using the\nenter_repo() code path in cmd_receive_pack() because only minimal setup is\nrequired for the default operation of git receive-pack.\n\n> Doing an optional set-up to make get_git_work_tree() work would\n> involve more damage to the codebase than necessary, I would suspect,\n> so let's keep this part of the code as-is.\n\nIndeed, that is the exact reason why I did not want to go down that rabbit\nhole. There are so many things that are skipped when using enter_repo()\ninstead of setup_git_directory() that the performance impact of switching\nalone would probably pose a major regression for big hosters like GitHub\nor Atlassian.\n\nI have to admit also that I never used the work tree feature myself,\ntherefore the support for it is pretty much untested (I *think* that I\nbriefly tested it years ago, when I came up with the original\nupdateInstead patch, but that is *long* ago). We could of course simply go\nthe safe route and error out if we detect that git_work_tree_cfg is set,\nleaving the work to support it and to add a proper unit test to somebody\nwho actually needs this. Hmm?\n\nCiao,\nJohannes\n"},{"id":"252846","messageId":"alpine.DEB.1.00.1412020929420.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqq61duohq7.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] receive-pack: support push-to-checkout hook","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-02T08:47:46Z","receivedAt":"2014-12-02T08:47:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 1 Dec 2014, Junio C Hamano wrote:\n\n> When receive.denyCurrentBranch is set to updateInstead, this hook\n> can be used to override the built-in \"push-to-deploy\" logic, which\n> insists that the working tree and the index must be unchanged\n> relative to HEAD. The hook receives the commit with which the\n> tip of the current is going to be updated, and is responsible to\n> make any necessary changes to the working tree and to the index to\n> bring them to the desired state when the tip of the current branch\n> is updated to the new commit.\n> \n> For example, the hook can simply run \"git read-tree -u -m HEAD $1\"\n> to the workflow to emulate \"'git fetch' going in the reverse\n> direction with 'git push'\" better than the push-to-deploy logic, as\n> the two-tree form of \"read-tree -u -m\" is essentially the same as\n> \"git checkout\" that switches branches while keeping the local\n> changes in the working tree that do not interfere with the\n> difference between the branches.\n\nI like it.\n\nThe only sad part is that the already huge test suite is enlarged by yet\nanother extensive set of test cases (and those tests might not really\nneed to be that extensive because they essentially only need to make sure\nthat the hook is run successfully *instead* of trying to update the\nworking directory, i.e. a simple 'touch yep' hook would have been enough).\nIt starts to be painful to run the complete test suite, not only on\nWindows (where this has been a multi-hour endeavor for me for ages\nalready). BuildHive (CloudBees' very kind offer of Jenkins CI for Open\nSource, integrated conveniently with GitHub) already takes over an hour to\nrun the Git test suite – and BuildHive runs on Linux, not Windows!\n\nThat means that everytime I push into a GitHub Pull Request, I have to\nwait for a full hour to know whether everything is groovy.\n\nWorse: when Git for Windows contributors (yes! they exist!) push into\ntheir Pull Requests, I have to wait for a full hour before I can merge,\nunless I want to merge something that the test suite did not validate!\n\nSo maybe it is time to start thinking about conciser tests that verify the\nbare minimum, especially for rarely exercised functionality? I mean,\ntesting is always a balance between too much and too little. And at this\npoint, I am afraid that several well-intended, but overly extensive tests\nincrease the overall runtime of \"make test\" to a point where developers\n*avoid* running it, costing more time in the long run than necessary.\n\nIn this particular case, I think that we really, really *just* need to\nverify that the presence of the hook switches off the default behavior of\nupdateInstead. *Nothing* else is needed to verify that this particular\nfunctionality hasn't regressed. I.e. something like:\n\n+test_expect_success 'updateInstead with push-to-checkout hook' '\n+\trm -fr testrepo &&\n+\tgit clone . testrepo &&\n+\t(\n+\t\tcd testrepo &&\n+\t\techo unclean > path1 &&\n+\t\tgit config receive.denyCurrentBranch updateInstead &&\n+\t\techo 'touch yep' | write_script .git/hooks/push-to-checkout\n+\t) &&\n+\tgit push testrepo HEAD^:refs/heads/master &&\n+\ttest unclean = $(cat testrepo/path1) &&\n+\ttest -f testrepo/yep\n+'\n\nwould be more appropriate (although it probably has one or three bugs,\ngiven that I wrote this in the mailer).\n\nCiao,\nJohannes"},{"id":"252863","messageId":"547DB894.4040609@drmicha.warpmail.net","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1412020929420.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/2] receive-pack: support push-to-checkout hook","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-12-02T13:03:16Z","receivedAt":"2014-12-02T13:03:16Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Johannes Schindelin schrieb am 02.12.2014 um 09:47:\n> Hi Junio,\n> \n> On Mon, 1 Dec 2014, Junio C Hamano wrote:\n> \n>> When receive.denyCurrentBranch is set to updateInstead, this hook\n>> can be used to override the built-in \"push-to-deploy\" logic, which\n>> insists that the working tree and the index must be unchanged\n>> relative to HEAD. The hook receives the commit with which the\n>> tip of the current is going to be updated, and is responsible to\n>> make any necessary changes to the working tree and to the index to\n>> bring them to the desired state when the tip of the current branch\n>> is updated to the new commit.\n>>\n>> For example, the hook can simply run \"git read-tree -u -m HEAD $1\"\n>> to the workflow to emulate \"'git fetch' going in the reverse\n>> direction with 'git push'\" better than the push-to-deploy logic, as\n>> the two-tree form of \"read-tree -u -m\" is essentially the same as\n>> \"git checkout\" that switches branches while keeping the local\n>> changes in the working tree that do not interfere with the\n>> difference between the branches.\n> \n> I like it.\n> \n> The only sad part is that the already huge test suite is enlarged by yet\n> another extensive set of test cases (and those tests might not really\n> need to be that extensive because they essentially only need to make sure\n> that the hook is run successfully *instead* of trying to update the\n> working directory, i.e. a simple 'touch yep' hook would have been enough).\n> It starts to be painful to run the complete test suite, not only on\n> Windows (where this has been a multi-hour endeavor for me for ages\n> already). BuildHive (CloudBees' very kind offer of Jenkins CI for Open\n> Source, integrated conveniently with GitHub) already takes over an hour to\n> run the Git test suite – and BuildHive runs on Linux, not Windows!\n> \n> That means that everytime I push into a GitHub Pull Request, I have to\n> wait for a full hour to know whether everything is groovy.\n> \n> Worse: when Git for Windows contributors (yes! they exist!) push into\n> their Pull Requests, I have to wait for a full hour before I can merge,\n> unless I want to merge something that the test suite did not validate!\n> \n> So maybe it is time to start thinking about conciser tests that verify the\n> bare minimum, especially for rarely exercised functionality? I mean,\n> testing is always a balance between too much and too little. And at this\n> point, I am afraid that several well-intended, but overly extensive tests\n> increase the overall runtime of \"make test\" to a point where developers\n> *avoid* running it, costing more time in the long run than necessary.\n> \n> In this particular case, I think that we really, really *just* need to\n> verify that the presence of the hook switches off the default behavior of\n> updateInstead. *Nothing* else is needed to verify that this particular\n> functionality hasn't regressed. I.e. something like:\n> \n> +test_expect_success 'updateInstead with push-to-checkout hook' '\n> +\trm -fr testrepo &&\n> +\tgit clone . testrepo &&\n> +\t(\n> +\t\tcd testrepo &&\n> +\t\techo unclean > path1 &&\n> +\t\tgit config receive.denyCurrentBranch updateInstead &&\n> +\t\techo 'touch yep' | write_script .git/hooks/push-to-checkout\n> +\t) &&\n> +\tgit push testrepo HEAD^:refs/heads/master &&\n> +\ttest unclean = $(cat testrepo/path1) &&\n> +\ttest -f testrepo/yep\n> +'\n> \n> would be more appropriate (although it probably has one or three bugs,\n> given that I wrote this in the mailer).\n> \n> Ciao,\n> Johannes\n> \n\nHow about reusing the prerequisites feature for that? We could either\nmark the minimal tests, or mark the others similar to how we do with the\n(extra) expensive tests. Your config.mk would then determine which tests\nare executed.\n\nMichael\n"},{"id":"252864","messageId":"alpine.DEB.1.00.1412021421000.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"547DB894.4040609@drmicha.warpmail.net","subject":"Re: [PATCH 2/2] receive-pack: support push-to-checkout hook","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-02T13:25:04Z","receivedAt":"2014-12-02T13:25:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn Tue, 2 Dec 2014, Michael J Gruber wrote:\n\n> Johannes Schindelin schrieb am 02.12.2014 um 09:47:\n> \n> > The only sad part is that the already huge test suite is enlarged by\n> > yet another extensive set of test cases (and those tests might not\n> > really need to be that extensive because they essentially only need to\n> > make sure that the hook is run successfully *instead* of trying to\n> > update the working directory, i.e. a simple 'touch yep' hook would\n> > have been enough). It starts to be painful to run the complete test\n> > suite, not only on Windows (where this has been a multi-hour endeavor\n> > for me for ages already). BuildHive (CloudBees' very kind offer of\n> > Jenkins CI for Open Source, integrated conveniently with GitHub)\n> > already takes over an hour to run the Git test suite – and BuildHive\n> > runs on Linux, not Windows!\n> \n> How about reusing the prerequisites feature for that? We could either\n> mark the minimal tests, or mark the others similar to how we do with the\n> (extra) expensive tests. Your config.mk would then determine which tests\n> are executed.\n\nIn general, you are correct. And we already have the test_have_prereq\nEXPENSIVE precedent.\n\nIn this particular case, I question the value of the extent of the tests:\nthe only thing we really need to test is that the new hook really\noverrides the default behavior, not all kinds of real-world simulations\nthat *use* that behavior.\n\nIn other words, it is my opinion that the difference between the \"touch\nyep\" test I demonstrated and the test originally suggested is the amount\nof time it takes to run, not the extent to which the new code ist actually\nverified.\n\nCiao,\nJohannes"},{"id":"252867","messageId":"xmqqzjb6m3a2.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1412020909020.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH v5] Add another option for receive.denyCurrentBranch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-02T16:20:53Z","receivedAt":"2014-12-02T16:20:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> This is because you know receive-pack runs inside the $GIT_DIR,\n>> whether it is a bare or non-bare repository, so either core.worktree\n>> points at a directory that is otherwise unrelated to the $GIT_DIR\n>> (but is the correct $GIT_WORK_TREE), or the top of the working tree\n>> must be \"..\" for a non-bare repository....\n\nAnd this reasoning may be broken, unfortunately.\n\nIn a repository with separate-git-dir, we enter $GIT_DIR that is\npointed by the \"gitdir: $over_there\" thing, and once we go there,\nwe have no linkage back to find where the working tree is unless\nthere is core.worktree set, do we?\n\nThis feature (with or without the push-to-checkout hook, as that\nshares exactly the same logic that discovers, or fails to do so,\nwhere the working tree is) needs to be documented with an entry in\nthe BUGS section, saying that it will not work in a repository that\nis tied to its working tree via the \"gitdir:\" mechanism.\n\nIt actually is a lot worse than merely \"it will not work\", when this\nproblem ever manifests itself. The use of this mechanism in such a\nrepository will destroy the contents of a wrong directory that\nhappens to be the parent directory of a repository pointed by the\n\"gitdir:\" mechanism, unless core.worktree is set. Fortunately, real\nsubmodule directories found in the \".git/modules/\" directory of the\nsuperproject, even though they are bound to their checkout locations\nin the working tree of the superproject using \"gitdir:\" mechanism,\ndo maintain their core.worktree when \"git submodule\" manages them,\nso the use of the mechanism in submodule setting may be safe.\n"},{"id":"252869","messageId":"xmqqtx1em2fu.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1412020929420.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/2] receive-pack: support push-to-checkout hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-02T16:39:01Z","receivedAt":"2014-12-02T16:39:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> In this particular case, I think that we really, really *just* need to\n> verify that the presence of the hook switches off the default behavior of\n> updateInstead. *Nothing* else is needed to verify that this particular\n> functionality hasn't regressed. I.e. something like:\n>\n> +test_expect_success 'updateInstead with push-to-checkout hook' '\n> +\trm -fr testrepo &&\n> +\tgit clone . testrepo &&\n> +\t(\n> +\t\tcd testrepo &&\n> +\t\techo unclean > path1 &&\n> +\t\tgit config receive.denyCurrentBranch updateInstead &&\n> +\t\techo 'touch yep' | write_script .git/hooks/push-to-checkout\n> +\t) &&\n> +\tgit push testrepo HEAD^:refs/heads/master &&\n> +\ttest unclean = $(cat testrepo/path1) &&\n> +\ttest -f testrepo/yep\n> +'\n>\n> would be more appropriate (although it probably has one or three bugs,\n> given that I wrote this in the mailer).\n\nNot really. You need to remember that we write tests not to show\noff the new shiny, but to protect essential invariants from being\nbroken by careless others attempting to rewrite the implementation\nin the future.\n\nWhat needs to be tested for the feature are at the minimum:\n\n * The hook is not triggered when denycurrent is set to\n updateInstead; the posted version does not even test this, but it\n should.\n\n * The hook is run with the right settings, so that git commands it\n runs will operate on the right repository and its working tree,\n but without overspecifying how that \"right settings\" happens [*1*].\n\n * Non-zero exit from the hook will fail \"git push\", and zero exit\n from the hook will allow \"git push\" to pass.\n\n * Whether the hook returns a success or a failure, the working tree\n and the index is in the state the hook expects to give the user\n (i.e. nobody else further munges the working tree and the index\n after hook returns) [*2*].\n\nThe patch I posted is minimum to do so. Compared to that, the \"yep\"\ntest is not checking anything of the importance, and only insists on\na single immaterial detail (i.e. hook must be run after cd'ing to\nthe top of the working tree).\n\nI fail to see why it could be more appropriate.\n\n\n[Footnote]\n\n*1* Your version above assumes that the hook must run in the working\n tree, but it does not have to, if GIT_DIR and GIT_WORK_TREE are\n both exported; your test is overspecifying what should happen,\n and will reject a legitimate implementation of the feature.\n\n*2* A hook may muck with the index or with the working tree and then\n return a failure. I do not offhand see why a failing push\n should be allowed to contaminate the working tree that way, but\n whatever the hook does is what the user wishes, and we should\n not lose data coming from that wish. The hook the test uses\n refrains from touching the working tree or the index when it\n fails, so the test checks that the working tree and the index\n are left as-is when it happens.\n"},{"id":"252870","messageId":"alpine.DEB.1.00.1412021744490.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqtx1em2fu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] receive-pack: support push-to-checkout hook","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-02T16:45:49Z","receivedAt":"2014-12-02T16:45:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 2 Dec 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > In this particular case, I think that we really, really *just* need to\n> > verify that the presence of the hook switches off the default behavior of\n> > updateInstead. *Nothing* else is needed to verify that this particular\n> > functionality hasn't regressed. I.e. something like:\n> >\n> > +test_expect_success 'updateInstead with push-to-checkout hook' '\n> > +\trm -fr testrepo &&\n> > +\tgit clone . testrepo &&\n> > +\t(\n> > +\t\tcd testrepo &&\n> > +\t\techo unclean > path1 &&\n> > +\t\tgit config receive.denyCurrentBranch updateInstead &&\n> > +\t\techo 'touch yep' | write_script .git/hooks/push-to-checkout\n> > +\t) &&\n> > +\tgit push testrepo HEAD^:refs/heads/master &&\n> > +\ttest unclean = $(cat testrepo/path1) &&\n> > +\ttest -f testrepo/yep\n> > +'\n> >\n> > would be more appropriate (although it probably has one or three bugs,\n> > given that I wrote this in the mailer).\n> \n> Not really. You need to remember that we write tests not to show\n> off the new shiny, but to protect essential invariants from being\n> broken by careless others attempting to rewrite the implementation\n> in the future.\n\nFair enough. You are the boss.\n\nI am not, therefore it does not matter what I think,\nJohannes\n"},{"id":"252871","messageId":"alpine.DEB.1.00.1412021750340.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqzjb6m3a2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5] Add another option for receive.denyCurrentBranch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-02T16:51:57Z","receivedAt":"2014-12-02T16:51:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 2 Dec 2014, Junio C Hamano wrote:\n\n> This feature [...] needs to be documented with an entry in the BUGS\n> section, saying that it will not work in a repository that is tied to\n> its working tree via the \"gitdir:\" mechanism.\n\nFair enough. But which BUGS section? Should I add one to `git-push.txt` or\n`git-receive-pack.txt`? Technically, it should be the latter, but nobody's\ngonna find it there...\n\nCiao,\nJohannes\n"},{"id":"252872","messageId":"xmqqppc2m1fz.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1412021744490.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/2] receive-pack: support push-to-checkout hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-02T17:00:32Z","receivedAt":"2014-12-02T17:00:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Not really. You need to remember that we write tests not to show\n>> off the new shiny, but to protect essential invariants from being\n>> broken by careless others attempting to rewrite the implementation\n>> in the future.\n>\n> Fair enough. You are the boss.\n>\n> I am not, therefore it does not matter what I think,\n\nIt is not that it does not matter because you are not the boss; it\nis just that when you are wrong, you are wrong.\n\nYou said in your response to Michael:\n\n the difference between the \"touch yep\" ... and\n the test originally suggested is ...\n not the extent to which the new code is actually verified.\n\nIf your \"verification\" is based on \"faith\", you are correct. You\nmay be \"verifying\" the code to the right extent, i.e. \"Yes, what I\nwrote is actually run, and I write perfect code every time, so what\nit does must be correct, as long as it gets run\".\n\nBut I do not have faith in people who will be touching the relevant\ncode in the future; \"Yes, it is triggered\" is far from satisfactory\nwithout faith like yours in the code being tested.\n\nI actually do not have much faith in what I write myself ;-). That\nis why we have tests.\n"},{"id":"252874","messageId":"alpine.DEB.1.00.1412021804210.13845@s15462909.onlinehome-server.info","threadId":"37896","inReplyTo":"xmqqppc2m1fz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] receive-pack: support push-to-checkout hook","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-02T17:12:50Z","receivedAt":"2014-12-02T17:12:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 2 Dec 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> Not really. You need to remember that we write tests not to show\n> >> off the new shiny, but to protect essential invariants from being\n> >> broken by careless others attempting to rewrite the implementation\n> >> in the future.\n> >\n> > Fair enough. You are the boss.\n> >\n> > I am not, therefore it does not matter what I think,\n> \n> It is not that it does not matter because you are not the boss; it\n> is just that when you are wrong, you are wrong.\n\nPlease, there is no need to get emotional, let alone personal.\n\nI am not really interested in challenging your policy regarding the test\nsuite, even if it does hurt my development style where I want to run the\ntest suite frequently but its tests just take too long because their focus\nis more on thoroughness rather than trying to save time in the manner I\nsuggested (i.e. by only lightly testing obscure code paths that will be\nexecuted rarely, risking bugs in favor of adding tests when fixing said\nbugs when – and if – they arise). There is nothing inherently wrong in the\nway you want to have the test suite, it is a matter of preference, that is\nall. I would like a more light-weight test suite that runs much faster,\nyou want a thorough one, even if it takes more time to run.\n\nSo: you are the boss, you do the things you do, and my opinion does not\nmatter. I say this most pragmatically, to save more time by ending this\ndiscussion now. There are no hard feelings on my side.\n\nCiao,\nJohannes"},{"id":"252875","messageId":"xmqqlhmqm0k2.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"alpine.DEB.1.00.1412021804210.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/2] receive-pack: support push-to-checkout hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-02T17:19:41Z","receivedAt":"2014-12-02T17:19:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> ... (i.e. by only lightly testing obscure code paths that will be\n> executed rarely, risking bugs in favor of adding tests when fixing said\n> bugs when – and if – they arise).\n\nI'd like to learn a bit more about this part, not because I want to\nsay you are wrong, but because I want to find out what you say is\npractically useful and can be adopted by the project.\n\nThe part I find most troublesome is this. Without tests covering\n\"obscure\" cases, how would you expect to notice when the codebase\nregresses, at which point you plan to fix and add tests for the fix?\n\nNot that I think the \"minimally these need to be tested\" list I sent\nearlier are anything obscure (they are all essential part of the\nfeature; without them working, the feature would not be useful).\n"},{"id":"252876","messageId":"xmqqh9xem0e6.fsf@gitster.dls.corp.google.com","threadId":"37896","inReplyTo":"xmqqzjb6m3a2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5] Add another option for receive.denyCurrentBranch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-02T17:23:13Z","receivedAt":"2014-12-02T17:23:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>>> This is because you know receive-pack runs inside the $GIT_DIR,\n>>> whether it is a bare or non-bare repository, so either core.worktree\n>>> points at a directory that is otherwise unrelated to the $GIT_DIR\n>>> (but is the correct $GIT_WORK_TREE), or the top of the working tree\n>>> must be \"..\" for a non-bare repository....\n>\n> And this reasoning may be broken, unfortunately.\n>\n> In a repository with separate-git-dir, we enter $GIT_DIR that is\n> pointed by the \"gitdir: $over_there\" thing, and once we go there,\n> we have no linkage back to find where the working tree is unless\n> there is core.worktree set, do we?\n>\n> This feature (with or without the push-to-checkout hook, as that\n> shares exactly the same logic that discovers, or fails to do so,\n> where the working tree is) needs to be documented with an entry in\n> the BUGS section, saying that it will not work in a repository that\n> is tied to its working tree via the \"gitdir:\" mechanism.\n>\n> It actually is a lot worse than merely \"it will not work\", when this\n> problem ever manifests itself. The use of this mechanism in such a\n> repository will destroy the contents of a wrong directory that\n> happens to be the parent directory of a repository pointed by the\n> \"gitdir:\" mechanism, unless core.worktree is set. Fortunately, real\n> submodule directories found in the \".git/modules/\" directory of the\n> superproject, even though they are bound to their checkout locations\n> in the working tree of the superproject using \"gitdir:\" mechanism,\n> do maintain their core.worktree when \"git submodule\" manages them,\n> so the use of the mechanism in submodule setting may be safe.\n\nActually the other part of the system that uses \"gitdir:\" mechanism,\ni.e. \"git init --separate-git-dir $real $worktree\", does add the\ncore.worktree configuration in $real/config pointing at $worktree,\nso the above is simply me being overly worried. If somebody uses\nthe \"gitdir:\" mechanism without setting core.worktree to point back,\nthat is a misconfigured repository/worktree pair, so there is no\nneed for any BUGS entry.\n\nSorry about the noise.\n"}]}