{"thread":{"id":"21938","subject":"[PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir","startedAt":"2009-12-12T04:32:51Z","lastAt":"2009-12-30T05:47:24Z","messageCount":18,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":7},"messages":[{"id":"129779","messageId":"20091212042042.3930.54783.chriscool@tuxfamily.org","threadId":"21938","inReplyTo":null,"subject":"[PATCH v5 0/7] \"git reset --merge\" related improvements","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-12-12T04:32:51Z","receivedAt":"2009-12-12T04:32:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Another reroll with the following changes:\n\n- the name of the new option is now \"--keep\" instead of \"--keep-local-changes\",\n- the fill_tree_descriptor() function is used instead of adding a new\nparse_and_init_tree_desc() function (thanks to Stephen Boyd),\n- patch 1/7 was added to only accept soft reset when not in a working tree;\nthis makes the test suite pass,\n- the commit message of patch 4/7 that adds the --keep option has been\nimproved; it talks more about the use case of this new option.\n\nChristian Couder (6):\n  reset: do not accept a mixed reset in a .git dir\n  reset: add a few tests for \"git reset --merge\"\n  reset: add option \"--keep\" to \"git reset\"\n  reset: add test cases for \"--keep\" option\n  Documentation: reset: describe new \"--keep\" option\n  Documentation: reset: add some tables to describe the different\n    options\n\nStephan Beyer (1):\n  reset: use \"unpack_trees()\" directly instead of \"git read-tree\"\n\n Documentation/git-reset.txt |   79 +++++++++++++++++++++-\n builtin-reset.c             |   74 +++++++++++++++-----\n t/t7103-reset-bare.sh       |    4 +-\n t/t7110-reset-merge.sh      |  156 +++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 292 insertions(+), 21 deletions(-)\n create mode 100755 t/t7110-reset-merge.sh\n"},{"id":"129778","messageId":"20091212043259.3930.98779.chriscool@tuxfamily.org","threadId":"21938","inReplyTo":"20091212042042.3930.54783.chriscool@tuxfamily.org","subject":"[PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-12-12T04:32:52Z","receivedAt":"2009-12-12T04:32:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"It is strange and fragile that a mixed reset is disallowed in a bare\nrepo but is allowed in a .git dir. So this patch simplifies things\nby only allowing soft resets when not in a working tree.\n\nThis patch is also needed to speed up \"git reset\" by using\nunpack_tree() directly (instead of execing \"git read-tree\"). A\nfollowing patch will do just that.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin-reset.c       |    3 +--\n t/t7103-reset-bare.sh |    4 ++--\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex 11d1c6e..14bdb03 100644\n--- a/builtin-reset.c\n+++ b/builtin-reset.c\n@@ -286,8 +286,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tif (reset_type == NONE)\n \t\treset_type = MIXED; /* by default */\n \n-\tif ((reset_type == HARD || reset_type == MERGE)\n-\t    && !is_inside_work_tree())\n+\tif (reset_type != SOFT && !is_inside_work_tree())\n \t\tdie(\"%s reset requires a work tree\",\n \t\t    reset_type_names[reset_type]);\n \ndiff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh\nindex 68041df..13344b2 100755\n--- a/t/t7103-reset-bare.sh\n+++ b/t/t7103-reset-bare.sh\n@@ -21,8 +21,8 @@ test_expect_success 'merge reset requires a worktree' '\n \t test_must_fail git reset --merge)\n '\n \n-test_expect_success 'mixed reset is ok' '\n-\t(cd .git && git reset)\n+test_expect_success 'mixed reset requires a worktree' '\n+\t(cd .git && test_must_fail git reset)\n '\n \n test_expect_success 'soft reset is ok' '\n-- \n1.6.6.rc1.8.gd33ec\n"},{"id":"129784","messageId":"20091212043259.3930.19134.chriscool@tuxfamily.org","threadId":"21938","inReplyTo":"20091212042042.3930.54783.chriscool@tuxfamily.org","subject":"[PATCH v5 2/7] reset: add a few tests for \"git reset --merge\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-12-12T04:32:53Z","receivedAt":"2009-12-12T04:32:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Commit 9e8eceab (\"Add 'merge' mode to 'git reset'\", 2008-12-01),\nadded the --merge option to git reset, but there were no test cases\nfor it.\n\nThis was not a big problem because \"git reset\" was just forking and\nexecing \"git read-tree\", but this will change in a following patch.\n\nSo let's add a few test cases to make sure that there will be no\nregression.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/t7110-reset-merge.sh |   94 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 94 insertions(+), 0 deletions(-)\n create mode 100755 t/t7110-reset-merge.sh\n\ndiff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh\nnew file mode 100755\nindex 0000000..8190da1\n--- /dev/null\n+++ b/t/t7110-reset-merge.sh\n@@ -0,0 +1,94 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2009 Christian Couder\n+#\n+\n+test_description='Tests for \"git reset --merge\"'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'creating initial files' '\n+     echo \"line 1\" >> file1 &&\n+     echo \"line 2\" >> file1 &&\n+     echo \"line 3\" >> file1 &&\n+     cp file1 file2 &&\n+     git add file1 file2 &&\n+     test_tick &&\n+     git commit -m \"Initial commit\"\n+'\n+\n+test_expect_success 'reset --merge is ok with changes in file it does not touch' '\n+     echo \"line 4\" >> file1 &&\n+     echo \"line 4\" >> file2 &&\n+     test_tick &&\n+     git commit -m \"add line 4\" file1 &&\n+     git reset --merge HEAD^ &&\n+     ! grep 4 file1 &&\n+     grep 4 file2 &&\n+     git reset --merge HEAD@{1} &&\n+     grep 4 file1 &&\n+     grep 4 file2\n+'\n+\n+test_expect_success 'reset --merge discards changes added to index (1)' '\n+     echo \"line 5\" >> file1 &&\n+     git add file1 &&\n+     git reset --merge HEAD^ &&\n+     ! grep 4 file1 &&\n+     ! grep 5 file1 &&\n+     grep 4 file2 &&\n+     echo \"line 5\" >> file2 &&\n+     git add file2 &&\n+     git reset --merge HEAD@{1} &&\n+     ! grep 4 file2 &&\n+     ! grep 5 file1 &&\n+     grep 4 file1\n+'\n+\n+test_expect_success 'reset --merge discards changes added to index (2)' '\n+     echo \"line 4\" >> file2 &&\n+     git add file2 &&\n+     git reset --merge HEAD^ &&\n+     ! grep 4 file2 &&\n+     git reset --merge HEAD@{1} &&\n+     ! grep 4 file2 &&\n+     grep 4 file1\n+'\n+\n+test_expect_success 'reset --merge fails with changes in file it touches' '\n+     echo \"line 5\" >> file1 &&\n+     test_tick &&\n+     git commit -m \"add line 5\" file1 &&\n+     sed -e \"s/line 1/changed line 1/\" <file1 >file3 &&\n+     mv file3 file1 &&\n+     test_must_fail git reset --merge HEAD^ 2>err.log &&\n+     grep file1 err.log | grep \"not uptodate\" &&\n+     git reset --hard HEAD^\n+'\n+\n+test_expect_success 'setup 2 different branches' '\n+     git branch branch1 &&\n+     git branch branch2 &&\n+     git checkout branch1 &&\n+     echo \"line 5 in branch1\" >> file1 &&\n+     test_tick &&\n+     git commit -a -m \"change in branch1\" &&\n+     git checkout branch2 &&\n+     echo \"line 5 in branch2\" >> file1 &&\n+     test_tick &&\n+     git commit -a -m \"change in branch2\"\n+'\n+\n+test_expect_success '\"reset --merge HEAD^\" fails with pending merge' '\n+     test_must_fail git merge branch1 &&\n+     test_must_fail git reset --merge HEAD^ &&\n+     git reset --hard HEAD\n+'\n+\n+test_expect_success '\"reset --merge HEAD\" fails with pending merge' '\n+     test_must_fail git merge branch1 &&\n+     test_must_fail git reset --merge HEAD &&\n+     git reset --hard HEAD\n+'\n+\n+test_done\n-- \n1.6.6.rc1.8.gd33ec\n"},{"id":"129782","messageId":"20091212043259.3930.82570.chriscool@tuxfamily.org","threadId":"21938","inReplyTo":"20091212042042.3930.54783.chriscool@tuxfamily.org","subject":"[PATCH v5 3/7] reset: use \"unpack_trees()\" directly instead of \"git read-tree\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-12-12T04:32:54Z","receivedAt":"2009-12-12T04:32:54Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Stephan Beyer <s-beyer@gmx.net>\n\nThis patch makes \"reset_index_file()\" call \"unpack_trees()\" directly\ninstead of forking and execing \"git read-tree\". So the code is more\nefficient.\n\nAnd it's also easier to see which unpack_tree() options will be used,\nas we don't need to follow \"git read-tree\"'s command line parsing\nwhich is quite complex.\n\nAs Daniel Barkalow found, there is a difference between this new\nversion and the old one. The old version gives an error for\n\"git reset --merge\" with unmerged entries and the new version does\nnot. But this can be seen as a bug fix, because \"--merge\" was the\nonly \"git reset\" option with this behavior and this behavior was\nnot documented.\n\nIn fact there is still an error with unmerge entries if we reset\nthe unmerge entries to the same state as HEAD. So the bug is not\ncompletely fixed.\n\nThe code comes from the sequencer GSoC project:\n\ngit://repo.or.cz/git/sbeyer.git\n\n(at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)\n\nMentored-by: Daniel Barkalow <barkalow@iabervon.org>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Stephan Beyer <s-beyer@gmx.net>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin-reset.c        |   41 ++++++++++++++++++++++++++++++-----------\n t/t7110-reset-merge.sh |    8 +++++---\n 2 files changed, 35 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex 14bdb03..ac38aaa 100644\n--- a/builtin-reset.c\n+++ b/builtin-reset.c\n@@ -18,6 +18,8 @@\n #include \"tree.h\"\n #include \"branch.h\"\n #include \"parse-options.h\"\n+#include \"unpack-trees.h\"\n+#include \"cache-tree.h\"\n \n static const char * const git_reset_usage[] = {\n \t\"git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]\",\n@@ -54,27 +56,44 @@ static inline int is_merge(void)\n \n static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet)\n {\n-\tint i = 0;\n-\tconst char *args[6];\n+\tint nr = 1;\n+\tint newfd;\n+\tstruct tree_desc desc[2];\n+\tstruct unpack_trees_options opts;\n+\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n \n-\targs[i++] = \"read-tree\";\n+\tmemset(&opts, 0, sizeof(opts));\n+\topts.head_idx = 1;\n+\topts.src_index = &the_index;\n+\topts.dst_index = &the_index;\n+\topts.fn = oneway_merge;\n+\topts.merge = 1;\n \tif (!quiet)\n-\t\targs[i++] = \"-v\";\n+\t\topts.verbose_update = 1;\n \tswitch (reset_type) {\n \tcase MERGE:\n-\t\targs[i++] = \"-u\";\n-\t\targs[i++] = \"-m\";\n+\t\topts.update = 1;\n \t\tbreak;\n \tcase HARD:\n-\t\targs[i++] = \"-u\";\n+\t\topts.update = 1;\n \t\t/* fallthrough */\n \tdefault:\n-\t\targs[i++] = \"--reset\";\n+\t\topts.reset = 1;\n \t}\n-\targs[i++] = sha1_to_hex(sha1);\n-\targs[i] = NULL;\n \n-\treturn run_command_v_opt(args, RUN_GIT_CMD);\n+\tnewfd = hold_locked_index(lock, 1);\n+\n+\tread_cache_unmerged();\n+\n+\tif (!fill_tree_descriptor(desc + nr - 1, sha1))\n+\t\treturn error(\"Failed to find tree of %s.\", sha1_to_hex(sha1));\n+\tif (unpack_trees(nr, desc, &opts))\n+\t\treturn -1;\n+\tif (write_cache(newfd, active_cache, active_nr) ||\n+\t    commit_locked_index(lock))\n+\t\treturn error(\"Could not write new index file.\");\n+\n+\treturn 0;\n }\n \n static void print_new_head_line(struct commit *commit)\ndiff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh\nindex 8190da1..6afaf73 100755\n--- a/t/t7110-reset-merge.sh\n+++ b/t/t7110-reset-merge.sh\n@@ -79,10 +79,12 @@ test_expect_success 'setup 2 different branches' '\n      git commit -a -m \"change in branch2\"\n '\n \n-test_expect_success '\"reset --merge HEAD^\" fails with pending merge' '\n+test_expect_success '\"reset --merge HEAD^\" is ok with pending merge' '\n      test_must_fail git merge branch1 &&\n-     test_must_fail git reset --merge HEAD^ &&\n-     git reset --hard HEAD\n+     git reset --merge HEAD^ &&\n+     test -z \"$(git diff --cached)\" &&\n+     test -n \"$(git diff)\" &&\n+     git reset --hard HEAD@{1}\n '\n \n test_expect_success '\"reset --merge HEAD\" fails with pending merge' '\n-- \n1.6.6.rc1.8.gd33ec\n"},{"id":"129780","messageId":"20091212043259.3930.21831.chriscool@tuxfamily.org","threadId":"21938","inReplyTo":"20091212042042.3930.54783.chriscool@tuxfamily.org","subject":"[PATCH v5 4/7] reset: add option \"--keep\" to \"git reset\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-12-12T04:32:55Z","receivedAt":"2009-12-12T04:32:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"The purpose of this new option is to discard some of the\nlast commits but to keep current changes in the work tree.\n\nThe use case is when you work on something and commit\nthat work. And then you work on something else that touches\nother files, but you don't commit it yet. Then you realize\nthat what you commited when you worked on the first thing\nis not good or belongs to another branch.\n\nSo you want to get rid of the previous commits (at least in\nthe current branch) but you want to make sure that you keep\nthe changes you have in the work tree. And you are pretty\nsure that your changes are independent from what you\npreviously commited, so you don't want the reset to succeed\nif the previous commits changed a file that you also\nchanged in your work tree.\n\nThe \"--keep\" option will do what you want.\n\nThe table below shows what happens when running\n\"git reset --option target\" to reset the HEAD to another\ncommit (as a special case \"target\" could be the same as\nHEAD) in the cases where \"--merge\" and \"--keep\" behave\ndifferently.\n\nworking index HEAD target         working index HEAD\n----------------------------------------------------\n  A      B     C     D   --keep    (disallowed)\n                         --merge   (disallowed)\n  A      B     C     C   --keep     A      C     C\n                         --merge   (disallowed)\n  B      B     C     D   --keep    (disallowed)\n                         --merge    C      C     C\n  B      B     C     C   --keep     B      C     C\n                         --merge    C      C     C\n\nIn this table, A, B and C are some different states of\na file. For example the last line of the table means\nthat if a file is in state B in the working tree and\nthe index, and in a different state C in HEAD and in\nthe target, then \"git reset --merge target\" will put\nthe file in state C in the working tree, in the index\nand in HEAD.\n\nSo as can be seen in the table, \"--merge\" can discard\nchanges in the working tree, while \"--keep\" does not.\nSo if one wants to avoid using \"git stash\" before and\nafter using \"git reset\" to save current changes, it is\nbetter to use \"--keep\" rather than \"--merge\".\n\nThe following table shows what happens on unmerged entries:\n\nworking index HEAD target         working index HEAD\n----------------------------------------------------\n X       U     A    B     --keep   X       B     B\n                          --merge  X       B     B\n X       U     A    A     --keep   X       A     A\n                          --merge (disallowed)\n\nIn this table X can be any state and U means an unmerged\nentry.\n\nA following patch will add some test cases for\n\"--keep\", where the differences between \"--merge\" and\n\"--keep\" can also be seen.\n\nThe \"--keep\" option is implemented by doing a 2 way merge\nbetween HEAD and the reset target, and if this succeeds\nby doing a mixed reset to the target.\n\nThe code comes from the sequencer GSoC project, where\nsuch an option was developed by Stephan Beyer:\n\ngit://repo.or.cz/git/sbeyer.git\n\n(at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)\n\nBut in the sequencer project the \"reset\" flag was set\nin the \"struct unpack_trees_options\" passed to\n\"unpack_trees()\". With this flag the changes in the\nworking tree were discarded if the file was different\nbetween HEAD and the reset target.\n\nMentored-by: Daniel Barkalow <barkalow@iabervon.org>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Stephan Beyer <s-beyer@gmx.net>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin-reset.c |   30 +++++++++++++++++++++++++-----\n 1 files changed, 25 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex ac38aaa..859e0d1 100644\n--- a/builtin-reset.c\n+++ b/builtin-reset.c\n@@ -22,13 +22,15 @@\n #include \"cache-tree.h\"\n \n static const char * const git_reset_usage[] = {\n-\t\"git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]\",\n+\t\"git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]\",\n \t\"git reset [--mixed] <commit> [--] <paths>...\",\n \tNULL\n };\n \n-enum reset_type { MIXED, SOFT, HARD, MERGE, NONE };\n-static const char *reset_type_names[] = { \"mixed\", \"soft\", \"hard\", \"merge\", NULL };\n+enum reset_type { MIXED, SOFT, HARD, MERGE, KEEP, NONE };\n+static const char *reset_type_names[] = {\n+\t\"mixed\", \"soft\", \"hard\", \"merge\", \"keep\", NULL\n+};\n \n static char *args_to_str(const char **argv)\n {\n@@ -71,6 +73,7 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet\n \tif (!quiet)\n \t\topts.verbose_update = 1;\n \tswitch (reset_type) {\n+\tcase KEEP:\n \tcase MERGE:\n \t\topts.update = 1;\n \t\tbreak;\n@@ -85,6 +88,16 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet\n \n \tread_cache_unmerged();\n \n+\tif (reset_type == KEEP) {\n+\t\tunsigned char head_sha1[20];\n+\t\tif (get_sha1(\"HEAD\", head_sha1))\n+\t\t\treturn error(\"You do not have a valid HEAD.\");\n+\t\tif (!fill_tree_descriptor(desc, head_sha1))\n+\t\t\treturn error(\"Failed to find tree of HEAD.\");\n+\t\tnr++;\n+\t\topts.fn = twoway_merge;\n+\t}\n+\n \tif (!fill_tree_descriptor(desc + nr - 1, sha1))\n \t\treturn error(\"Failed to find tree of %s.\", sha1_to_hex(sha1));\n \tif (unpack_trees(nr, desc, &opts))\n@@ -228,6 +241,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\t\"reset HEAD, index and working tree\", HARD),\n \t\tOPT_SET_INT(0, \"merge\", &reset_type,\n \t\t\t\t\"reset HEAD, index and working tree\", MERGE),\n+\t\tOPT_SET_INT(0, \"keep\", &reset_type,\n+\t\t\t\t\"reset HEAD but keep local changes\",\n+\t\t\t\tKEEP),\n \t\tOPT_BOOLEAN('q', NULL, &quiet,\n \t\t\t\t\"disable showing new HEAD in hard reset and progress message\"),\n \t\tOPT_BOOLEAN('p', \"patch\", &patch_mode, \"select hunks interactively\"),\n@@ -315,9 +331,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tif (reset_type == SOFT) {\n \t\tif (is_merge() || read_cache() < 0 || unmerged_cache())\n \t\t\tdie(\"Cannot do a soft reset in the middle of a merge.\");\n+\t} else {\n+\t\tint err = reset_index_file(sha1, reset_type, quiet);\n+\t\tif (reset_type == KEEP)\n+\t\t\terr = err || reset_index_file(sha1, MIXED, quiet);\n+\t\tif (err)\n+\t\t\tdie(\"Could not reset index file to revision '%s'.\", rev);\n \t}\n-\telse if (reset_index_file(sha1, reset_type, quiet))\n-\t\tdie(\"Could not reset index file to revision '%s'.\", rev);\n \n \t/* Any resets update HEAD to the head being switched to,\n \t * saving the previous head in ORIG_HEAD before. */\n-- \n1.6.6.rc1.8.gd33ec\n"},{"id":"129785","messageId":"20091212043259.3930.62225.chriscool@tuxfamily.org","threadId":"21938","inReplyTo":"20091212042042.3930.54783.chriscool@tuxfamily.org","subject":"[PATCH v5 5/7] reset: add test cases for \"--keep\" option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-12-12T04:32:56Z","receivedAt":"2009-12-12T04:32:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This shows that with the \"--keep\" option, changes that are both in\nthe work tree and the index are kept in the work tree after the\nreset (but discarded in the index). As with the \"--merge\" option,\nchanges that are in both the work tree and the index are discarded\nafter the reset.\n\nIn the case of unmerged entries, we can see that\n\"git reset --merge HEAD\" is disallowed while it works using\n\"--keep\" instead.\n\nAnd this shows that otherwise \"--merge\" and \"--keep\" have the same\nbehavior.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/t7110-reset-merge.sh |   62 +++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 61 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh\nindex 6afaf73..bd2ef94 100755\n--- a/t/t7110-reset-merge.sh\n+++ b/t/t7110-reset-merge.sh\n@@ -3,7 +3,7 @@\n # Copyright (c) 2009 Christian Couder\n #\n \n-test_description='Tests for \"git reset --merge\"'\n+test_description='Tests for \"git reset\" with --merge and --keep'\n \n . ./test-lib.sh\n \n@@ -30,6 +30,20 @@ test_expect_success 'reset --merge is ok with changes in file it does not touch'\n      grep 4 file2\n '\n \n+test_expect_success 'reset --keep is ok with changes in file it does not touch' '\n+     git reset --hard HEAD^ &&\n+     echo \"line 4\" >> file1 &&\n+     echo \"line 4\" >> file2 &&\n+     test_tick &&\n+     git commit -m \"add line 4\" file1 &&\n+     git reset --keep HEAD^ &&\n+     ! grep 4 file1 &&\n+     grep 4 file2 &&\n+     git reset --keep HEAD@{1} &&\n+     grep 4 file1 &&\n+     grep 4 file2\n+'\n+\n test_expect_success 'reset --merge discards changes added to index (1)' '\n      echo \"line 5\" >> file1 &&\n      git add file1 &&\n@@ -55,6 +69,25 @@ test_expect_success 'reset --merge discards changes added to index (2)' '\n      grep 4 file1\n '\n \n+test_expect_success 'reset --keep fails with changes in index in files it touches' '\n+     echo \"line 4\" >> file2 &&\n+     echo \"line 5\" >> file1 &&\n+     git add file1 &&\n+     test_must_fail git reset --keep HEAD^ &&\n+     git reset --hard HEAD\n+'\n+\n+test_expect_success 'reset --keep keeps changes it does not touch' '\n+     echo \"line 4\" >> file2 &&\n+     git add file2 &&\n+     git reset --keep HEAD^ &&\n+     grep 4 file2 &&\n+     git reset --keep HEAD@{1} &&\n+     grep 4 file2 &&\n+     grep 4 file1 &&\n+     git reset --hard HEAD\n+'\n+\n test_expect_success 'reset --merge fails with changes in file it touches' '\n      echo \"line 5\" >> file1 &&\n      test_tick &&\n@@ -66,6 +99,17 @@ test_expect_success 'reset --merge fails with changes in file it touches' '\n      git reset --hard HEAD^\n '\n \n+test_expect_success 'reset --keep fails with changes in file it touches' '\n+     echo \"line 5\" >> file1 &&\n+     test_tick &&\n+     git commit -m \"add line 5\" file1 &&\n+     sed -e \"s/line 1/changed line 1/\" <file1 >file3 &&\n+     mv file3 file1 &&\n+     test_must_fail git reset --keep HEAD^ 2>err.log &&\n+     grep file1 err.log | grep \"not uptodate\" &&\n+     git reset --hard HEAD^\n+'\n+\n test_expect_success 'setup 2 different branches' '\n      git branch branch1 &&\n      git branch branch2 &&\n@@ -87,10 +131,26 @@ test_expect_success '\"reset --merge HEAD^\" is ok with pending merge' '\n      git reset --hard HEAD@{1}\n '\n \n+test_expect_success '\"reset --keep HEAD^\" is ok with pending merge' '\n+     test_must_fail git merge branch1 &&\n+     git reset --keep HEAD^ &&\n+     test -z \"$(git diff --cached)\" &&\n+     test -n \"$(git diff)\" &&\n+     git reset --hard HEAD@{1}\n+'\n+\n test_expect_success '\"reset --merge HEAD\" fails with pending merge' '\n      test_must_fail git merge branch1 &&\n      test_must_fail git reset --merge HEAD &&\n      git reset --hard HEAD\n '\n \n+test_expect_success '\"reset --keep HEAD\" is ok with pending merge' '\n+     test_must_fail git merge branch1 &&\n+     git reset --keep HEAD &&\n+     test -z \"$(git diff --cached)\" &&\n+     test -n \"$(git diff)\" &&\n+     git reset --hard HEAD\n+'\n+\n test_done\n-- \n1.6.6.rc1.8.gd33ec\n"},{"id":"129781","messageId":"20091212043259.3930.41408.chriscool@tuxfamily.org","threadId":"21938","inReplyTo":"20091212042042.3930.54783.chriscool@tuxfamily.org","subject":"[PATCH v5 6/7] Documentation: reset: describe new \"--keep\" option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-12-12T04:32:57Z","receivedAt":"2009-12-12T04:32:57Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Documentation/git-reset.txt |    7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt\nindex 2d27e40..f8724d0 100644\n--- a/Documentation/git-reset.txt\n+++ b/Documentation/git-reset.txt\n@@ -8,7 +8,7 @@ git-reset - Reset current HEAD to the specified state\n SYNOPSIS\n --------\n [verse]\n-'git reset' [--mixed | --soft | --hard | --merge] [-q] [<commit>]\n+'git reset' [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]\n 'git reset' [-q] [<commit>] [--] <paths>...\n 'git reset' --patch [<commit>] [--] [<paths>...]\n \n@@ -52,6 +52,11 @@ OPTIONS\n \tand updates the files that are different between the named commit\n \tand the current commit in the working tree.\n \n+--keep::\n+\tThis does the same thing as --merge, but it keeps changes in\n+\tthe working tree and the index or it aborts if it is not\n+\tpossible.\n+\n -p::\n --patch::\n \tInteractively select hunks in the difference between the index\n-- \n1.6.6.rc1.8.gd33ec\n"},{"id":"129783","messageId":"20091212043259.3930.16467.chriscool@tuxfamily.org","threadId":"21938","inReplyTo":"20091212042042.3930.54783.chriscool@tuxfamily.org","subject":"[PATCH v5 7/7] Documentation: reset: add some tables to describe the different options","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-12-12T04:32:58Z","receivedAt":"2009-12-12T04:32:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch adds a DISCUSSION section that contains some tables to\nshow how the different \"git reset\" options work depending on the\nstates of the files in the working tree, the index, HEAD and the\ntarget commit.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Documentation/git-reset.txt |   72 +++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 72 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt\nindex f8724d0..4ded9e3 100644\n--- a/Documentation/git-reset.txt\n+++ b/Documentation/git-reset.txt\n@@ -72,6 +72,78 @@ linkgit:git-add[1]).\n <commit>::\n \tCommit to make the current HEAD. If not given defaults to HEAD.\n \n+DISCUSSION\n+----------\n+\n+The tables below show what happens when running:\n+\n+----------\n+git reset --option target\n+----------\n+\n+to reset the HEAD to another commit (`target`) with the different\n+reset options depending on the state of the files.\n+\n+      working index HEAD target         working index HEAD\n+      ----------------------------------------------------\n+       A       B     C    D     --soft   A       B     D\n+                                --mixed  A       D     D\n+                                --hard   D       D     D\n+                                --merge (disallowed)\n+                                --keep  (disallowed)\n+\n+      working index HEAD target         working index HEAD\n+      ----------------------------------------------------\n+       A       B     C    C     --soft   A       B     C\n+                                --mixed  A       C     C\n+                                --hard   C       C     C\n+                                --merge (disallowed)\n+                                --keep   A       C     C\n+\n+      working index HEAD target         working index HEAD\n+      ----------------------------------------------------\n+       B       B     C    D     --soft   B       B     D\n+                                --mixed  B       D     D\n+                                --hard   D       D     D\n+                                --merge  D       D     D\n+                                --keep  (disallowed)\n+\n+      working index HEAD target         working index HEAD\n+      ----------------------------------------------------\n+       B       B     C    C     --soft   B       B     C\n+                                --mixed  B       C     C\n+                                --hard   C       C     C\n+                                --merge  C       C     C\n+                                --keep   B       C     C\n+\n+In these tables, A, B, C and D are some different states of a\n+file. For example, the last line of the last table means that if a\n+file is in state B in the working tree and the index, and in a\n+different state C in HEAD and in the target, then \"git reset\n+--keep target\" will put the file in state B in the working tree\n+and in state C in the index and HEAD.\n+\n+The following tables show what happens when there are unmerged\n+entries:\n+\n+      working index HEAD target         working index HEAD\n+      ----------------------------------------------------\n+       X       U     A    B     --soft  (disallowed)\n+                                --mixed  X       B     B\n+                                --hard   B       B     B\n+                                --merge  X       B     B\n+                                --keep   X       B     B\n+\n+      working index HEAD target         working index HEAD\n+      ----------------------------------------------------\n+       X       U     A    A     --soft  (disallowed)\n+                                --mixed  X       A     A\n+                                --hard   A       A     A\n+                                --merge (disallowed)\n+                                --keep   X       A     A\n+\n+X means any state and U means an unmerged index.\n+\n Examples\n --------\n \n-- \n1.6.6.rc1.8.gd33ec\n"},{"id":"129803","messageId":"7vtyvvn9wx.fsf@alter.siamese.dyndns.org","threadId":"21938","inReplyTo":"20091212043259.3930.98779.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-12T21:45:50Z","receivedAt":"2009-12-12T21:45:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> It is strange and fragile that a mixed reset is disallowed in a bare\n> repo but is allowed in a .git dir. So this patch simplifies things\n> by only allowing soft resets when not in a working tree.\n\nI would not mind what you said were \"I find the difference strange\", as it\nis a fairly subjective word.  But if you say \"fragile\", you would need to\ndefend the use of the word better.  What kind of misuse does it invite,\nand what grave consequences such misuses would cause?  I do not see any\nfragility and I would want to understand it better so that I can write\nabout it in the Release Note to warn people and encourage them to upgrade.\n\nMore importantly, I think the difference between the two actually makes\nsense.  A bare repository by definition does _not_ have any work tree so\nthere is no point in having the index file in there.  On the other hand,\neven though it is not necessary to \"cd .git && git reset HEAD^\", I don't\nsee a strong reason why it needs to be disallowed.\n\nOn the other hand, I don't see a strong reason why such a use needs to be\nsupported, other than \"we've allowed it for a long time, and people may\nhave hooks (they typically start in $GIT_DIR) that rely on it\", which by\nitself is not something strong enough to veto the change.  It is only a\nminor incompatibility and I can be persuaded to listen to a pros-and-cons\nargument.\n\nAn honest justification might have been \"This change to disallow a mixed\nreset in $GIT_DIR of a repository with a work tree will break existing\nscripts, but I think it is not widely used _for such and such reasons_,\nand can easily be worked around.  On the other hand, this change vastly\nsimplifies the reimplementation of 'reset' _because X and Y and Z_\".\n\n> This patch is also needed to speed up \"git reset\" by using\n> unpack_tree() directly (instead of execing \"git read-tree\").\n\nIt is very unclear _why_ it is \"needed\" from this description.\n"},{"id":"129805","messageId":"7vocm3n9wj.fsf@alter.siamese.dyndns.org","threadId":"21938","inReplyTo":"20091212043259.3930.82570.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 3/7] reset: use \"unpack_trees()\" directly instead of \"git read-tree\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-12T21:46:04Z","receivedAt":"2009-12-12T21:46:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> As Daniel Barkalow found, there is a difference between this new\n> version and the old one. The old version gives an error for\n> \"git reset --merge\" with unmerged entries and the new version does\n> not. But this can be seen as a bug fix,...\n\nI sense that there is one crucial sentence missing here for a reader to\njudge the \"can be seen as a bug fix\" claim.  Instead of giving an error\nand stopping, what _does_ this version do?  If it ran \"rm -rf\" on the\nentire work tree instead of giving an error, it wouldn't be a bugfix, for\nexample ;-)\n\n> In fact there is still an error with unmerge entries if we reset\n> the unmerge entries to the same state as HEAD. So the bug is not\n> completely fixed.\n\n\"If we reset to HEAD then it is a bug\"---and what the patch actually does\nis...?\n\nI agree with the analysis in the log message of 9e8ecea (Add 'merge' mode\nto 'git reset', 2008-12-01):\n\n    If the index has unmerged entries, \"--merge\" will currently simply\n    refuse to reset (\"you need to resolve your current index first\").\n    You'll need to use \"--hard\" or similar in this case.\n\n    This is sad, because normally a unmerged index means that the working\n    tree file should have matched the source tree, so the correct action\n    is likely to make --merge reset such a path to the target (like --hard),\n    regardless of dirty state in-tree or in-index.\n\nIf I have a local change that I know is unrelated to a merge I am\ngoing to do, I can imagine I'd work like this:\n\n    $ git pull some-where\n    ... Some paths conflict, others merge cleanly and update\n    ... the index, but the overall change looks horrible, and\n    ... I decide to discard the whole thing.\n    $ git reset --merge HEAD\n\nIn this case HEAD is the target.  Similarly, if I have a local change that\nI know is unrelated to a series of patches I am applying:\n\n    $ git am -3 topic.mbox\n    ... A few patches apply cleanly, and then the series fail\n    ... with conflict.  The overall change looks horrible, and\n    ... I decide to discard the whole thing.\n    $ git reset --merge @{3.minutes.ago}\n\nIn this case, the target is the commit that I was at before starting to\napply the series---i.e. different from HEAD.\n\nIn either case, because \"merge\" (triggered by \"pull\") and \"am -3\" honor\nthe promise with the user that:\n\n (1) no mergy (aka \"integration\") command stuffs an unmerged entry to an\n     index that is dirty with respect to HEAD (this should also apply to\n     \"cherry-pick\" and \"rebase -m/-i\" even though the last one is often\n     stricter than it is absolutely necessary); and\n\n (2) every mergy command verifies that the index entry and the path in the\n     work tree do not have any local change before they are updated by it;\n\nresetting can safely overwrite both the index entry and the path in the\nwork tree with contents taken from the commit we are switching to.\n\nThe updated test seems to be testing that \"reset --merge HEAD^\" does make\nthe index match the target, but it only checks \"is there _any_ change\",\nand does not even test \"which path kept the change and which path got\ncloberred\".\n\nIdeally it should test \"Is the change what we expect to have?  Did we keep\nwhat we should have kept, instead of clobbering? Did we discard the\nchanges to the path that the failed merge cloberred?\", all of the three.\n\n> diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh\n> index 8190da1..6afaf73 100755\n> --- a/t/t7110-reset-merge.sh\n> +++ b/t/t7110-reset-merge.sh\n> @@ -79,10 +79,12 @@ test_expect_success 'setup 2 different branches' '\n>       git commit -a -m \"change in branch2\"\n>  '\n>  \n> -test_expect_success '\"reset --merge HEAD^\" fails with pending merge' '\n> +test_expect_success '\"reset --merge HEAD^\" is ok with pending merge' '\n>       test_must_fail git merge branch1 &&\n> -     test_must_fail git reset --merge HEAD^ &&\n> -     git reset --hard HEAD\n> +     git reset --merge HEAD^ &&\n> +     test -z \"$(git diff --cached)\" &&\n> +     test -n \"$(git diff)\" &&\n> +     git reset --hard HEAD@{1}\n>  '\n>  \n>  test_expect_success '\"reset --merge HEAD\" fails with pending merge' '\n> -- \n> 1.6.6.rc1.8.gd33ec\n"},{"id":"129806","messageId":"7viqcbn9w3.fsf@alter.siamese.dyndns.org","threadId":"21938","inReplyTo":"20091212043259.3930.21831.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 4/7] reset: add option \"--keep\" to \"git reset\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-12T21:46:20Z","receivedAt":"2009-12-12T21:46:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> The purpose of this new option is to discard some of the last commits\n> but to keep current changes in the work tree.\n>\n> The use case is when you work on something and commit that work. And\n> then you work on something else that touches other files, but you don't\n> commit it yet. Then you realize that what you commited when you worked\n> on the first thing is not good or belongs to another branch.\n>\n> So you want to get rid of the previous commits (at least in the current\n> branch) but you want to make sure that you keep the changes you have in\n> the work tree. And you are pretty sure that your changes are independent\n> from what you previously commited, so you don't want the reset to\n> succeed if the previous commits changed a file that you also changed in\n> your work tree.\n>\n> The \"--keep\" option will do what you want.\n\nHaving two paragraphs that describe the use case near the top is a big\nimprovement from older rounds, but this one line is more irritating than\nuseful, as it tries to be convincing only by being repetitive.  The only\nnew information it adds relative to the first two lines is its name.  The\ntrue convincing argument immediately follows in the form of the table\nbelow anyway, so I'd drop this line if I were you.\n\n> The table below shows what happens when running \"git reset --option\n> target\" to reset the HEAD to another commit (as a special case \"target\"\n> could be the same as HEAD) in the cases where \"--merge\" and \"--keep\"\n> behave differently.\n>\n> working index HEAD target         working index HEAD\n> ----------------------------------------------------\n>   A      B     C     D   --keep    (disallowed)\n>                          --merge   (disallowed)\n\nOk. the user has partially updated the index and has further changes; we\ndon't want to lose either of them.  During a failed merge, this cannot\nhappen, so it is a good safety measure on \"--merge\" side as well.\n\n>   A      B     C     C   --keep     A      C     C\n>                          --merge   (disallowed)\n\nThe user started working based on C, and has partially updated the index.\nThis cannot have come from a failed automerge because A != B, so using\n\"reset --merge\" is likely to be a mistake and we should disallow it as a\nsafety measure.\n\n>   B      B     C     D   --keep    (disallowed)\n>                          --merge    C      C     C\n\nFor --keep this is the same as the first case (except that the \"partially\nupdated the index\" happened to be \"100% pertially\") and it makes sense to\ndisallow it.\n\nHowever, I think --merge _should_ have D (target) in all of the three in\nthe result in this case, as I mentioned in my response to [PATCH 3/7].  Is\nthat \"the bug\" you talked about there?\n\n>   B      B     C     C   --keep     B      C     C\n>                          --merge    C      C     C\n\nA special case of the second one for --keep.  For --merge, B can only have\ncome from the previous merge operation we are resetting away, and matching\nall three result to the \"C\", which is the target, is the right thing to do.\n\n> So as can be seen in the table, \"--merge\" can discard changes in the\n> working tree, while \"--keep\" does not.  So if one wants to avoid using\n> \"git stash\" before and after using \"git reset\" to save current changes,\n> it is better to use \"--keep\" rather than \"--merge\".\n\nThis is a very flawed and misleading description.  These two options serve\ntwo entirely different purposes and use cases, but your \"if one wants to\navoid using ... to save current changes\" is too broad and does not\ndistinguish between the two. \"--keep\" option is valid (or \"better\") for\nonly one of them, while it is absolutely the wrong option to use for the\nother one.  So it may be \"better\" only half the time and it is wrong in\nthe other half.\n\nThe precondition of using a --merge is the scenario Linus described in\n9e8ecea (Add 'merge' mode to 'git reset', 2008-12-01).  You started some\nmergy operation and ended up in a state where all the differences, either\nmerged or unmerged, between the HEAD and the index came from the mergy\noperation.  Also the paths in the work tree that correspond to the index\nentries with differences from HEAD are modified by the mergy operation\n(either updated with a cleanly resolved merge, or with conflict markers)\nand do not match HEAD when \"reset --merge\" is run, but it is guaranteed\nthat you didn't have any local modification to them before the mergy\noperation.  You do want to reset these changes to the work tree away,\nwhile keeping the changes you had before the mergy operation.\n\n\"--merge\" not only _can_ (as in \"has the risk to\") discard changes, but in\nthe \"failed merge\" scenario, it is absolutely the right thing to do to\ndiscard the changes in the work tree for the paths that have differences\nbetween HEAD and index and to update them to that of the target.  Not\nupdating like \"--keep\" does is not \"better\" but is simply _wrong_.\n\nThe use case \"--keep\" wants to support is very different (and you have a\ngood description near the beginning of the commit log message for people\nto judge if the intended use case makes sense).  For it, discarding the\nchanges to the work tree is a wrong thing to do.\n\n> The following table shows what happens on unmerged entries:\n>\n> working index HEAD target         working index HEAD\n> ----------------------------------------------------\n>  X       U     A    B     --keep   X       B     B\n>                           --merge  X       B     B\n>  X       U     A    A     --keep   X       A     A\n>                           --merge (disallowed)\n>\n> In this table X can be any state and U means an unmerged entry.\n\nI am wondering if we should disallow \"--keep\" if we see unmerged entries\nin the index as a minimum safety measure.  Failing \"reset --keep\" when an\nunmerged entry exists in the index will save people who are trying to\ndiscard a failed merge (i.e. who should have used --merge) but was somehow\nfooled into thinking that \"--keep\" is a better \"--merge\".\n\nIt also is tempting to say that we should forbid \"reset --merge\" without\nan unmerged entry in the index, but that wouldn't work.  A mergy operation\nwould have left unmerged entries in the index initially before giving the\ncontrol back to the user, but the user may have used \"edit && git add\" to\nresolve them, and then decided that it is not worth committing.  By the\ntime \"reset --merge\" is run, there may not be any unmerged path left in\nthe index.\n\nI am suggesting extra safety measures primarily because I am worried that\npeople will get confused by these two similar looking options that are\nmeant for entirely different use cases.  An obvious alternative solution\nto avoid the confusion is not to add \"--keep\" in the first place.  While I\nthink that is rather a cop-out than a solution, it might make more sense.\nIt is hopeless to educate users to pick the right one, if even the author\nof the new option mistakenly thinks that \"--keep is merely a better\nversion of --merge\".\n\nMy preference is at this point to first have patches [1/7] to [3/7] to\nupdate \"reset --merge\" (I am not sure about the \"--mixed in $GIT_DIR\"\nchange, though), with a new follow-up patch [3.5/7] to fix \"reset --merge\"\nto reset paths that were cloberred by the merge to the target (not HEAD),\nand start cooking these changes in 'pu' and then 'next'.\n\nThat will lay groundwork of using unpack_trees() in \"reset\" and after they\nstabilize, build new modes like \"--keep\" on top of it.\n"},{"id":"129799","messageId":"7v638blqhy.fsf@alter.siamese.dyndns.org","threadId":"21938","inReplyTo":"20091212043259.3930.19134.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 2/7] reset: add a few tests for \"git reset --merge\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-12T23:30:33Z","receivedAt":"2009-12-12T23:30:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> +test_description='Tests for \"git reset --merge\"'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'creating initial files' '\n> +     echo \"line 1\" >> file1 &&\n> +     echo \"line 2\" >> file1 &&\n> +     echo \"line 3\" >> file1 &&\n> +     cp file1 file2 &&\n> +     git add file1 file2 &&\n> +     test_tick &&\n> +     git commit -m \"Initial commit\"\n> +'\n\n5-char indent is somewhat unusual; please fix it.  Also you may want to\ntag the initial one and other key commits for later use.\n\n> +test_expect_success 'reset --merge is ok with changes in file it does not touch' '\n> +     echo \"line 4\" >> file1 &&\n> +     echo \"line 4\" >> file2 &&\n> +     test_tick &&\n> +     git commit -m \"add line 4\" file1 &&\n\nIOW, move the above four lines to the end of the first \"setup\", and tag as\nnecessary like this:\n\n    test_expect_success setup '\n        for i in 1 2 3; do echo line $i; done >file1 &&\n        cat file1 >file2 &&\n        git add file1 file2 &&\n        test_tick &&\n        git commit -m \"Initial commit\" &&\n        git tag initial &&\n        echo line 4 >>file1 &&\n        cat file1 >file2 &&\n        test_tick &&\n        git commit -m \"add line 4 to file1\" file1 &&\n        git tag second\n    '\n\nAnd the rest should become its own test, starting from a known state.\n\n> +     git reset --merge HEAD^ &&\n> +     ! grep 4 file1 &&\n> +     grep 4 file2 &&\n\nSwitching from the second commit to the initial one should succeed because\nthe only local change is in file2 that is unrelated.  This should also check\nif HEAD points at the \"initial\" commit after \"reset\", and the index\nmatches the HEAD commit.\n\n> +     git reset --merge HEAD@{1} &&\n> +     grep 4 file1 &&\n> +     grep 4 file2\n> +'\n\nAnd switching back to the second commit should recover.  This should be a\nseparate test, and should make sure it begins in a known state even if the\n\"reset --merge HEAD^\" in the previous test failed.  This should also\ncheck:\n\n\ttest $(git rev-parse HEAD) = $(git rev-parse second) &&\n\ttest -z \"$(git diff --cached)\"\n\nIf any of these two \"reset --merge\" is broken, the next test will start at\nan unknown commit, hence you should add\n\n\tgit reset --hard second &&\n\nat the beginning of the next test.  You may also need to copy file1 to\nfile2 to recreate the local change as well.\n\nThe idea is not to assume that tests will succeed and 'recovery' step at\nthe end will run; instead, make sure each test starts itself in a known\nstate.  The way you wrote \"reset --hard\" at the end of some tests for\nrecovery is fragile.  There is no guarantee that each test run to the end,\nexecuting these recovery parts.  Instead, have a corresponding set-up at\nthe beginning of each test as necessary.\n\n> +test_expect_success 'reset --merge discards changes added to index (1)' '\n> +     echo \"line 5\" >> file1 &&\n> +     git add file1 &&\n\nSo at this point, file2 has an extra \"line 4\", and file1 has a change to\nbe committed.  And we reset to the initial commit.\n\n> +     git reset --merge HEAD^ &&\n> +     ! grep 4 file1 &&\n> +     ! grep 5 file1 &&\n> +     grep 4 file2 &&\n\nAnd that will make file1 match that of initial, while keeping the change\nto file2 intact.  The same comment about missing checks applies.  The\nremainder of this test should be a separate test.\n\n> +     echo \"line 5\" >> file2 &&\n> +     git add file2 &&\n> +     git reset --merge HEAD@{1} &&\n> +     ! grep 4 file2 &&\n> +     ! grep 5 file1 &&\n> +     grep 4 file1\n> +'\n\nStarting at 'initial' but with two lines added to file2 and the index\nupdated to it, we reset to 'second' (spell these out with tags, instead of\nrelying on reflog, so that you do not assume all the previous tests have\npassed).  Both files should match those of 'second'.  The same comment\nabout missing checks applies.\n\n> +test_expect_success 'reset --merge discards changes added to index (2)' '\n> +     echo \"line 4\" >> file2 &&\n> +     git add file2 &&\n> +     git reset --merge HEAD^ &&\n> +     ! grep 4 file2 &&\n> +     git reset --merge HEAD@{1} &&\n> +     ! grep 4 file2 &&\n> +     grep 4 file1\n> +'\n\nThe same comment about the missing 'make sure it begins in a known state'\napplies, but I don't see the point of this (2).  Is there any fundamental\ndifference of the set-up this one tests, compared to the earlier test?\n\n> +test_expect_success 'reset --merge fails with changes in file it touches' '\n> +     echo \"line 5\" >> file1 &&\n> +     test_tick &&\n> +     git commit -m \"add line 5\" file1 &&\n> +     sed -e \"s/line 1/changed line 1/\" <file1 >file3 &&\n> +     mv file3 file1 &&\n> +     test_must_fail git reset --merge HEAD^ 2>err.log &&\n> +     grep file1 err.log | grep \"not uptodate\" &&\n\nHmm, this is something we had a patch for to give different messages from\nplumbing and Porcelain.\n\n> +     git reset --hard HEAD^\n\nLose this 'recover at the end'.  There is no guarantee the each test run\nto the end.   Instead, have a corresponding set-up at the beginning of the\nnext one.\n\nI'd re-review the rest after this is rerolled.\n"},{"id":"129955","messageId":"200912152041.36194.chriscool@tuxfamily.org","threadId":"21938","inReplyTo":"7vtyvvn9wx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-12-15T19:41:35Z","receivedAt":"2009-12-15T19:41:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On samedi 12 décembre 2009, Junio C Hamano wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> > It is strange and fragile that a mixed reset is disallowed in a bare\n> > repo but is allowed in a .git dir. So this patch simplifies things\n> > by only allowing soft resets when not in a working tree.\n>\n> I would not mind what you said were \"I find the difference strange\", as\n> it is a fairly subjective word.  But if you say \"fragile\", you would need\n> to defend the use of the word better.  What kind of misuse does it\n> invite, and what grave consequences such misuses would cause?  I do not\n> see any fragility and I would want to understand it better so that I can\n> write about it in the Release Note to warn people and encourage them to\n> upgrade.\n>\n> More importantly, I think the difference between the two actually makes\n> sense.  A bare repository by definition does _not_ have any work tree so\n> there is no point in having the index file in there.  On the other hand,\n> even though it is not necessary to \"cd .git && git reset HEAD^\", I don't\n> see a strong reason why it needs to be disallowed.\n\nOk. I have the following patch now:\n\n---------- 8< ---------------\ncommit c20f969db6e565f2fe854b95202c3ef95ad0ff42\nAuthor: Christian Couder <chriscool@tuxfamily.org>\nDate:   Thu Dec 10 22:10:07 2009 +0100\n\n    reset: improve mixed reset error message when in a bare repo\n\n    When running a \"git reset --mixed\" in a bare repository, the\n    message displayed is:\n\n    fatal: This operation must be run in a work tree\n    fatal: Could not reset index file to revision 'HEAD^'.\n\n    This message is a little bit misleading as a mixed reset is ok in\n    a git directory, so it is not absolutely needed to run it in a\n    work tree.\n\n    So this patch improves upon the above by changing the message to:\n\n    fatal: mixed reset is not allowed in a bare repository\n\n    This patch is also needed to speed up \"git reset\" by using\n    unpack_tree() directly (instead of execing \"git read-tree\"). A\n    following patch will do just that.\n\n    While at it, instead of disallowing \"git reset --option\" outside\n    a work tree only when option is \"hard\" or \"merge\", we now disallow\n    it except when option is \"soft\" or \"mixed\", as it is safer if we\n    ever add options to \"git reset\".\n\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex 11d1c6e..ac3505b 100644\n--- a/builtin-reset.c\n+++ b/builtin-reset.c\n@@ -286,11 +286,15 @@ int cmd_reset(int argc, const char **argv, const char \n*pre\n        if (reset_type == NONE)\n                reset_type = MIXED; /* by default */\n\n-       if ((reset_type == HARD || reset_type == MERGE)\n-           && !is_inside_work_tree())\n+       if (reset_type != SOFT && reset_type != MIXED\n+            && !is_inside_work_tree())\n                die(\"%s reset requires a work tree\",\n                    reset_type_names[reset_type]);\n\n+       if (reset_type == MIXED && is_bare_repository())\n+               die(\"%s reset is not allowed in a bare repository\",\n+                   reset_type_names[reset_type]);\n+\n        /* Soft reset does not touch the index file nor the working tree\n         * at all, but requires them in a good order.  Other resets reset\n         * the index file to the tree object we are switching to. */\n---------- 8< ---------------\n\n> On the other hand, I don't see a strong reason why such a use needs to be\n> supported, other than \"we've allowed it for a long time, and people may\n> have hooks (they typically start in $GIT_DIR) that rely on it\", which by\n> itself is not something strong enough to veto the change.  It is only a\n> minor incompatibility and I can be persuaded to listen to a pros-and-cons\n> argument.\n>\n> An honest justification might have been \"This change to disallow a mixed\n> reset in $GIT_DIR of a repository with a work tree will break existing\n> scripts, but I think it is not widely used _for such and such reasons_,\n> and can easily be worked around.  On the other hand, this change vastly\n> simplifies the reimplementation of 'reset' _because X and Y and Z_\".\n\nMy opinion is that it works this way just by accident not by design (that's \nwhy I said \"fragile\"). But if we don't want to take any risk of regression, \nthen let's use the new patch above.\n\n> > This patch is also needed to speed up \"git reset\" by using\n> > unpack_tree() directly (instead of execing \"git read-tree\").\n>\n> It is very unclear _why_ it is \"needed\" from this description.\n\nThe reason is that after the next patch, it will not fail in a bare \nrepository, so if we don't change anything, the test that checks that it \nfails in a bare repo will fail. I will add this explanation to the new \npatch.\n\nBest regards,\nChristian.\n"},{"id":"129958","messageId":"7vskbcyot5.fsf@alter.siamese.dyndns.org","threadId":"21938","inReplyTo":"200912152041.36194.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-15T20:17:42Z","receivedAt":"2009-12-15T20:17:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n>     This patch is also needed to speed up \"git reset\" by using\n>     unpack_tree() directly (instead of execing \"git read-tree\"). A\n>     following patch will do just that.\n\nThis still doesn't seem to explain anything that the part you added after\nyour patch.\n\n>     While at it, instead of disallowing \"git reset --option\" outside\n>     a work tree only when option is \"hard\" or \"merge\", we now disallow\n>     it except when option is \"soft\" or \"mixed\", as it is safer if we\n>     ever add options to \"git reset\".\n\nI fail to see any sane logic behind this reasoning; you cannot decide if\nyou need to allow or disallow the new --option with unspecified semantics\nuntil you have that --option, and you are saying \n\nHmm, \"reset --option\" that does not work when it should work is a bug,\njust like \"reset --option\" that does not refuse to work when it should\nrefuse is, and you cannot decide if you should allow a new --option until\nyou have it.  Your \"disallowing everything the code does not know about by\ndefault\" doesn't particularly sound safer to me.  I'd suggest dropping it\nfrom this patch.\n\nIt is perfectly fine to have a change like that, if it makes the logic\neasier to follow with the updated repertoire when a new --option is added,\nbut not before.\n\n>> An honest justification might have been \"This change to disallow a mixed\n>> reset in $GIT_DIR of a repository with a work tree will break existing\n>> scripts, but I think it is not widely used _for such and such reasons_,\n>> and can easily be worked around.  On the other hand, this change vastly\n>> simplifies the reimplementation of 'reset' _because X and Y and Z_\".\n>\n> My opinion is that it works this way just by accident not by design (that's \n> why I said \"fragile\").\n\nAnd do you still think it is accident after I explained the difference\nbetween the two for you (or perhaps you didn't read it)?\n\n>> > This patch is also needed to speed up \"git reset\" by using\n>> > unpack_tree() directly (instead of execing \"git read-tree\").\n>>\n>> It is very unclear _why_ it is \"needed\" from this description.\n>\n> The reason is that after the next patch, it will not fail in a bare \n> repository,...\n\nThat sounds as if you want to change the definition of what the expected\nbehaviour is early, because you want to claim a regression you will later\nintroduce is not a regression.  I hope that is not the case.\n"},{"id":"129959","messageId":"7vocm0yop9.fsf@alter.siamese.dyndns.org","threadId":"21938","inReplyTo":"200912152041.36194.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-15T20:20:02Z","receivedAt":"2009-12-15T20:20:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> diff --git a/builtin-reset.c b/builtin-reset.c\n> index 11d1c6e..ac3505b 100644\n> --- a/builtin-reset.c\n> +++ b/builtin-reset.c\n> @@ -286,11 +286,15 @@ int cmd_reset(int argc, const char **argv, const char \n> *pre\n>         if (reset_type == NONE)\n>                 reset_type = MIXED; /* by default */\n>\n> -       if ((reset_type == HARD || reset_type == MERGE)\n> -           && !is_inside_work_tree())\n> +       if (reset_type != SOFT && reset_type != MIXED\n> +            && !is_inside_work_tree())\n>                 die(\"%s reset requires a work tree\",\n>                     reset_type_names[reset_type]);\n>\n> +       if (reset_type == MIXED && is_bare_repository())\n> +               die(\"%s reset is not allowed in a bare repository\",\n> +                   reset_type_names[reset_type]);\n\nThis patch text itself makes sense, I think, except the first part.\n"},{"id":"129960","messageId":"7vk4woyogb.fsf@alter.siamese.dyndns.org","threadId":"21938","inReplyTo":"7vskbcyot5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-15T20:25:24Z","receivedAt":"2009-12-15T20:25:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> The reason is that after the next patch, it will not fail in a bare \n>> repository,...\n>\n> That sounds as if you want to change the definition of what the expected\n> behaviour is early, because you want to claim a regression you will later\n> introduce is not a regression.  I hope that is not the case.\n\nHmm...\n\nBy \"after the next patch, it will not fail in a bare repository\",\ndid you mean \"if the next patch blindly replaced an external call to\nread-tree with an internal call to unpack_trees(), it will change the\nbehaviour, and we will end up allowing '--mixed in bare'.  To prevent it\nfrom happening, cmd_reset() should check that condition upfront\"?\n\nThen you were not trying to hide regressions (which makes me happier).\nBut then doesn't the change belong to the next patch, not this one?\n"},{"id":"129984","messageId":"200912160736.46826.chriscool@tuxfamily.org","threadId":"21938","inReplyTo":"7vskbcyot5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-12-16T06:36:46Z","receivedAt":"2009-12-16T06:36:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On mardi 15 décembre 2009, Junio C Hamano wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> >     While at it, instead of disallowing \"git reset --option\" outside\n> >     a work tree only when option is \"hard\" or \"merge\", we now disallow\n> >     it except when option is \"soft\" or \"mixed\", as it is safer if we\n> >     ever add options to \"git reset\".\n>\n> I fail to see any sane logic behind this reasoning; you cannot decide if\n> you need to allow or disallow the new --option with unspecified semantics\n> until you have that --option, and you are saying\n>\n> Hmm, \"reset --option\" that does not work when it should work is a bug,\n> just like \"reset --option\" that does not refuse to work when it should\n> refuse is, and you cannot decide if you should allow a new --option until\n> you have it.  Your \"disallowing everything the code does not know about\n> by default\" doesn't particularly sound safer to me.  I'd suggest dropping\n> it from this patch.\n\nOk, I will drop it.\n\n> It is perfectly fine to have a change like that, if it makes the logic\n> easier to follow with the updated repertoire when a new --option is\n> added, but not before.\n\nOk.\n\n[...]\n\n> By \"after the next patch, it will not fail in a bare repository\",\n> did you mean \"if the next patch blindly replaced an external call to\n> read-tree with an internal call to unpack_trees(), it will change the\n> behaviour, and we will end up allowing '--mixed in bare'.  To prevent it\n> from happening, cmd_reset() should check that condition upfront\"?\n\nYes, that's what I meant.\n\n> Then you were not trying to hide regressions (which makes me happier).\n> But then doesn't the change belong to the next patch, not this one?\n\nI can put it in the patch that calls unpack_trees() directly, but on the \nother hand it can also be seen as an improvement that could be applied \nto \"maint\" as it improves the error message.\n\nBest regards,\nChristian.\n"},{"id":"130513","messageId":"200912300647.25184.chriscool@tuxfamily.org","threadId":"21938","inReplyTo":"7viqcbn9w3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 4/7] reset: add option \"--keep\" to \"git reset\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-12-30T05:47:24Z","receivedAt":"2009-12-30T05:47:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On samedi 12 décembre 2009, Junio C Hamano wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n>\n> >   B      B     C     D   --keep    (disallowed)\n> >                          --merge    C      C     C\n>\n> For --keep this is the same as the first case (except that the \"partially\n> updated the index\" happened to be \"100% pertially\") and it makes sense to\n> disallow it.\n>\n> However, I think --merge _should_ have D (target) in all of the three in\n> the result in this case, as I mentioned in my response to [PATCH 3/7]. \n> Is that \"the bug\" you talked about there?\n\nNo it is not the bug I talked about. It was a bug in the documentation \npatch. Thanks for finding it! I left some \"C\" on the merge line from an \nearlier documentation patch but I should have changed them to \"D\" like \nthat:\n\n   B      B     C     D   --keep    (disallowed)\n                          --merge    D     D     D\n\n[...]\n\n> It also is tempting to say that we should forbid \"reset --merge\" without\n> an unmerged entry in the index, but that wouldn't work.  A mergy\n> operation would have left unmerged entries in the index initially before\n> giving the control back to the user, but the user may have used \"edit &&\n> git add\" to resolve them, and then decided that it is not worth\n> committing.  By the time \"reset --merge\" is run, there may not be any\n> unmerged path left in the index.\n>\n> I am suggesting extra safety measures primarily because I am worried that\n> people will get confused by these two similar looking options that are\n> meant for entirely different use cases.  An obvious alternative solution\n> to avoid the confusion is not to add \"--keep\" in the first place.  While\n> I think that is rather a cop-out than a solution, it might make more\n> sense. It is hopeless to educate users to pick the right one, if even the\n> author of the new option mistakenly thinks that \"--keep is merely a\n> better version of --merge\".\n\nI just think it is better in some cases not always.\n\n> My preference is at this point to first have patches [1/7] to [3/7] to\n> update \"reset --merge\" (I am not sure about the \"--mixed in $GIT_DIR\"\n> change, though), with a new follow-up patch [3.5/7] to fix \"reset\n> --merge\" to reset paths that were cloberred by the merge to the target\n> (not HEAD), and start cooking these changes in 'pu' and then 'next'.\n\nOk, I will send a patch series to do that except that I I don't understand \nexactly what the follow up patch [3.5/7] shoud do. Perhaps you asked for \nthis additional patch because of the documentation bug above?\n\n> That will lay groundwork of using unpack_trees() in \"reset\" and after\n> they stabilize, build new modes like \"--keep\" on top of it.\n\nOk.\n\nThanks,\nChristian.\n"}]}