{"thread":{"id":"20985","subject":"[PATCH v3 0/4] \"git reset --merge\" related improvements","startedAt":"2009-09-17T04:14:35Z","lastAt":"2009-09-17T21:04:42Z","messageCount":15,"participants":["Christian Couder","Junio C Hamano","Johannes Sixt","Daniel Barkalow"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"123401","messageId":"20090917040835.4048.51057.chriscool@tuxfamily.org","threadId":"20985","inReplyTo":null,"subject":"[PATCH v3 0/4] \"git reset --merge\" related improvements","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-09-17T04:14:35Z","receivedAt":"2009-09-17T04:14:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In this new version, some test cases have been added to show that the\nbehavior of \"git reset --merge\" when there is a pending merge is changed\nby patch 2/4.\n\nAnd some commit messages have been improved, thanks to Junio.\n\nThere is no documentation yet but I am working on it. \n\nChristian Couder (2):\n reset: add a few tests for \"git reset --merge\"\n reset: add test cases for \"--merge-safe\" option\n\nStephan Beyer (2):\n reset: use \"unpack_trees()\" directly instead of \"git read-tree\"\n reset: add option \"--merge-safe\" to \"git reset\"\n\n builtin-reset.c | 81 ++++++++++++++++++++++------\n t/t7110-reset-merge.sh | 142 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 207 insertions(+), 16 deletions(-)\n create mode 100755 t/t7110-reset-merge.sh\n"},{"id":"123404","messageId":"20090917041440.4048.96587.chriscool@tuxfamily.org","threadId":"20985","inReplyTo":"20090917040835.4048.51057.chriscool@tuxfamily.org","subject":"[PATCH v3 1/4] reset: add a few tests for \"git reset --merge\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-09-17T04:14:36Z","receivedAt":"2009-09-17T04:14:36Z","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 \n---\n t/t7110-reset-merge.sh | 87 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 87 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..8a28553\n--- /dev/null\n+++ b/t/t7110-reset-merge.sh\n@@ -0,0 +1,87 @@\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/\" 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 fails with pending merge' '\n+ test_must_fail git merge branch1 &&\n+ test_must_fail git reset --merge HEAD^\n+'\n+\n+test_done\n-- \n1.6.5.rc0.150.g38fe6\n"},{"id":"123403","messageId":"20090917041440.4048.80022.chriscool@tuxfamily.org","threadId":"20985","inReplyTo":"20090917040835.4048.51057.chriscool@tuxfamily.org","subject":"[PATCH v3 2/4] reset: use \"unpack_trees()\" directly instead of \"git read-tree\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-09-17T04:14:37Z","receivedAt":"2009-09-17T04:14:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Stephan Beyer \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\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 \nMentored-by: Christian Couder \nSigned-off-by: Stephan Beyer \nSigned-off-by: Christian Couder \n---\n builtin-reset.c | 51 +++++++++++++++++++++++++++++++++++++----------\n t/t7110-reset-merge.sh | 7 ++++-\n 2 files changed, 45 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex 73e6022..ddb81f3 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] []\",\n@@ -52,29 +54,56 @@ static inline int is_merge(void)\n \treturn !access(git_path(\"MERGE_HEAD\"), F_OK);\n }\n \n+static int parse_and_init_tree_desc(const unsigned char *sha1,\n+\t\t\t\t\t struct tree_desc *desc)\n+{\n+\tstruct tree *tree = parse_tree_indirect(sha1);\n+\tif (!tree)\n+\t\treturn 1;\n+\tinit_tree_desc(desc, tree->buffer, tree->size);\n+\treturn 0;\n+}\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 (parse_and_init_tree_desc(sha1, desc + nr - 1))\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 8a28553..6211096 100755\n--- a/t/t7110-reset-merge.sh\n+++ b/t/t7110-reset-merge.sh\n@@ -79,9 +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 fails with pending merge' '\n+test_expect_success 'reset --merge is ok with pending merge' '\n test_must_fail git merge branch1 &&\n- test_must_fail git reset --merge 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_done\n-- \n1.6.5.rc0.150.g38fe6\n"},{"id":"123405","messageId":"20090917041440.4048.16353.chriscool@tuxfamily.org","threadId":"20985","inReplyTo":"20090917040835.4048.51057.chriscool@tuxfamily.org","subject":"[PATCH v3 3/4] reset: add option \"--merge-safe\" to \"git reset\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-09-17T04:14:38Z","receivedAt":"2009-09-17T04:14:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Stephan Beyer \n\nThis option is nearly like \"--merge\" except that it is\nsafer.\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 \"--merge-safe\"\n(abreviated --m-s) behave differently.\n\nworking index HEAD target working index HEAD\n B B A A --m-s B A A\n --merge A A A\n B B A C --m-s (disallowed)\n --merge C C C\n\nIn this table, A, B and C are some different states of\na file. For example the first 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 A in HEAD and in\nthe target, then \"git reset --merge-safe target\" will\nput the file in state B in the working tree and in\nstate A in the index and HEAD.\n\nSo as can be seen in the table, \"--merge\" discards changes\nin the index, while \"--merge-safe\" does not.\n\nA following patch will add some test cases for\n\"--merge-safe\", where the differences between \"--merge\"\nand \"--merge-safe\" can also be seen.\n\nThe \"--merge-safe\" option is implemented by doing a 2 way\nmerge between HEAD and the reset target, and if this\nsucceeds by doing a mixed reset to the target.\n\nThe code comes from the sequencer GSoC project:\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 \nMentored-by: Christian Couder \nSigned-off-by: Stephan Beyer \nSigned-off-by: Christian Couder \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 ddb81f3..78d42e6 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] []\",\n+\t\"git reset [--mixed | --soft | --hard | --merge | --merge-safe] [-q] []\",\n \t\"git reset [--mixed] [--] ...\",\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, MERGE_SAFE, NONE };\n+static const char *reset_type_names[] = {\n+\t\"mixed\", \"soft\", \"hard\", \"merge\", \"merge_safe\", NULL\n+};\n \n static char *args_to_str(const char **argv)\n {\n@@ -81,6 +83,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 MERGE_SAFE:\n \tcase MERGE:\n \t\topts.update = 1;\n \t\tbreak;\n@@ -95,6 +98,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 == MERGE_SAFE) {\n+\t\tunsigned char *head_sha1;\n+\t\tif (get_sha1(\"HEAD\", head_sha1))\n+\t\t\treturn error(\"You do not have a valid HEAD.\");\n+\t\tif (parse_and_init_tree_desc(head_sha1, desc))\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 (parse_and_init_tree_desc(sha1, desc + nr - 1))\n \t\treturn error(\"Failed to find tree of %s.\", sha1_to_hex(sha1));\n \tif (unpack_trees(nr, desc, &opts))\n@@ -238,6 +251,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, \"merge-safe\", &reset_type,\n+\t\t\t\t\"reset HEAD, index and working tree\",\n+\t\t\t\tMERGE_SAFE),\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@@ -324,9 +340,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 == MERGE_SAFE)\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.5.rc0.150.g38fe6\n"},{"id":"123402","messageId":"20090917041440.4048.10260.chriscool@tuxfamily.org","threadId":"20985","inReplyTo":"20090917040835.4048.51057.chriscool@tuxfamily.org","subject":"[PATCH v3 4/4] reset: add test cases for \"--merge-safe\" option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-09-17T04:14:39Z","receivedAt":"2009-09-17T04:14:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This shows that with the \"--merge-safe\" option, changes that are\nboth in the work tree and the index are kept in the work tree after\nthe reset (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\nAnd this shows that otherwise \"--merge\" and \"--merge-safe\" have the\nsame behavior.\n\nSigned-off-by: Christian Couder \n---\n t/t7110-reset-merge.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 53 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh\nindex 6211096..794a506 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 --merge-safe'\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 --merge-safe 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 --merge-safe HEAD^ &&\n+ ! grep 4 file1 &&\n+ grep 4 file2 &&\n+ git reset --merge-safe 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 --merge-safe 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 --merge-safe HEAD^ &&\n+ git reset --hard HEAD\n+'\n+\n+test_expect_success 'reset --merge-safe keeps changes it does not touch' '\n+ echo \"line 4\" >> file2 &&\n+ git add file2 &&\n+ git reset --merge-safe HEAD^ &&\n+ grep 4 file2 &&\n+ git reset --merge-safe 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 --merge-safe 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/\" file3 &&\n+ mv file3 file1 &&\n+ test_must_fail git reset --merge-safe 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,4 +131,12 @@ test_expect_success 'reset --merge is ok with pending merge' '\n git reset --hard HEAD@{1}\n '\n \n+test_expect_success 'reset --merge-safe is ok with pending merge' '\n+ test_must_fail git merge branch1 &&\n+ git reset --merge-safe HEAD^ &&\n+ test -z \"$(git diff --cached)\" &&\n+ test -n \"$(git diff)\" &&\n+ git reset --hard HEAD@{1}\n+'\n+\n test_done\n-- \n1.6.5.rc0.150.g38fe6\n"},{"id":"123406","messageId":"7vk4zykv7o.fsf@alter.siamese.dyndns.org","threadId":"20985","inReplyTo":"20090917041440.4048.16353.chriscool@tuxfamily.org","subject":"Re: [PATCH v3 3/4] reset: add option \"--merge-safe\" to \"git reset\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-17T05:15:39Z","receivedAt":"2009-09-17T05:15:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> From: Stephan Beyer \n>\n> This option is nearly like \"--merge\" except that it is\n> safer.\n\nDo you still want to have this description after the last round?\n\n> The table below shows what happens when running\n> \"git reset --option target\" to reset the HEAD to another\n> commit (as a special case \"target\" could be the same as\n> HEAD) in the cases where \"--merge\" and \"--merge-safe\"\n> (abreviated --m-s) behave differently.\n>\n> working index HEAD target working index HEAD\n> B B A A --m-s B A A\n> --merge A A A\n> B B A C --m-s (disallowed)\n> --merge C C C\n\nI'd like to see at least the following rows filled as well.\n\n X U A A --m-s ??? ??? ???\n --merge ??? ??? ???\n X U B A --m-s ??? ??? ???\n --merge ??? ??? ???\n\n> In this table, A, B and C are some different states of a file.\n\n... and X is \"don't care\", and U is \"unmerged index\".\n\n> The code comes from the sequencer GSoC project:\n>\n> git://repo.or.cz/git/sbeyer.git\n>\n> (at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)\n>\n> But in the sequencer project the \"reset\" flag was set in the \"struct\n> unpack_trees_options\" passed to \"unpack_trees()\". With this flag the\n> changes in the working tree were discarded if the file was different\n> between HEAD and the reset target.\n\nIf you need to have four lines worth of description here, is this still\nStephan's patch, or would it be more appropriate to say \"This is based on\nan earlier GSoC work by Stephan in git://repo.or.cz/git/sbeyer.git\nrepository.\" and you take all the credit and blame?\n\n> static const char * const git_reset_usage[] = {\n> -\t\"git reset [--mixed | --soft | --hard | --merge] [-q] []\",\n> +\t\"git reset [--mixed | --soft | --hard | --merge | --merge-safe] [-q] []\",\n> \t\"git reset [--mixed] [--] ...\",\n> \tNULL\n> };\n\nAs we established in the previous round, this is _different_ from --merge,\nbut *not* in the sense that --merge is more dangerous and users should be\nusing this new option instead, but in the sense that --merge perfectly\nworks well for its intended use case, and this new option triggers a mode\nof operation that is meant to be used in a completely different use case,\nwhich is unspecified in this series without documentation.\n\nIn that light, is --merge-safe still a good name for the option, or merely\na misleading one?\n\nAs I said in the previous round, --merge discards the modified index state\nwhen switching, and it is absolutely _the right thing to do_ in the use\ncase it was intended for. It is _not_ dangerous, and using --merge-safe\nin that scenario is not being _safe_ but is actively a _wrong_ thing to do.\n\n> @@ -95,6 +98,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 == MERGE_SAFE) {\n> +\t\tunsigned char *head_sha1;\n> +\t\tif (get_sha1(\"HEAD\", head_sha1))\n> +\t\t\treturn error(\"You do not have a valid HEAD.\");\n> +\t\tif (parse_and_init_tree_desc(head_sha1, desc))\n> +\t\t\treturn error(\"Failed to find tree of HEAD.\");\n> +\t\tnr++;\n> +\t\topts.fn = twoway_merge;\n> +\t}\n\nget_sha1() takes an allocated buffer, does not allocate space on its own.\nI think you meant \"unsigned char head_sha1[20];\" here.\n"},{"id":"123411","messageId":"4AB1D957.20902@viscovery.net","threadId":"20985","inReplyTo":"7vk4zykv7o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 3/4] reset: add option \"--merge-safe\" to \"git reset\"","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-09-17T06:38:15Z","receivedAt":"2009-09-17T06:38:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> As we established in the previous round, this is _different_ from --merge,\n> but *not* in the sense that --merge is more dangerous and users should be\n> using this new option instead, but in the sense that --merge perfectly\n> works well for its intended use case, and this new option triggers a mode\n> of operation that is meant to be used in a completely different use case,\n> which is unspecified in this series without documentation.\n> \n> In that light, is --merge-safe still a good name for the option, or merely\n> a misleading one?\n\nDo I understand this correctly?\n\n(1) The intended use-case of --merge is to \"reset _a_ merge\".\n\n(2) The intended use-case of --merge-safe is to point the branch head to a\ndifferent commit, but to carry the changes that currently are in the index\nand wd over to the new commit, similar to checkout --merge.\n\nI had mistaken that --merge actually performs (2) because of the striking\nsimilarity of the option's name to checkout's --merge. So, IMHO, whatever\nthe new option is named that performs (2) - it introduces an\ninconsistency, because --merge is already taken.\n\n-- Hannes\n"},{"id":"123413","messageId":"7vr5u6jbgk.fsf@alter.siamese.dyndns.org","threadId":"20985","inReplyTo":"4AB1D957.20902@viscovery.net","subject":"Re: [PATCH v3 3/4] reset: add option \"--merge-safe\" to \"git reset\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-17T07:07:39Z","receivedAt":"2009-09-17T07:07:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Junio C Hamano schrieb:\n>> As we established in the previous round, this is _different_ from --merge,\n>> but *not* in the sense that --merge is more dangerous and users should be\n>> using this new option instead, but in the sense that --merge perfectly\n>> works well for its intended use case, and this new option triggers a mode\n>> of operation that is meant to be used in a completely different use case,\n>> which is unspecified in this series without documentation.\n>> \n>> In that light, is --merge-safe still a good name for the option, or merely\n>> a misleading one?\n>\n> Do I understand this correctly?\n>\n> (1) The intended use-case of --merge is to \"reset _a_ merge\".\n\nSee my review comment for the previous round where I described the\nintended use case of \"reset --merge\" and explained why discarding the\nchanges to the index is _the right thing_. It is to throw away the\nchanges that was done to your index by an either completed or conflicted\nmerge.\n\n> (2) The intended use-case of --merge-safe is to point the branch head to a\n> different commit, but to carry the changes that currently are in the index\n> and wd over to the new commit, similar to checkout --merge.\n\nI have _no_ idea what the intended use-case of --merge-safe is, and that\nwas why I asked Christian for clarification in the previous round. The\nanswer was still not clear enough so I pointed out --merge-safe could be\nstill doing a wrong thing even in _his_ use-case.\n"},{"id":"123417","messageId":"4AB1E41F.6050304@viscovery.net","threadId":"20985","inReplyTo":"7vr5u6jbgk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 3/4] reset: add option \"--merge-safe\" to \"git reset\"","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-09-17T07:24:15Z","receivedAt":"2009-09-17T07:24:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Sixt writes:\n>> Junio C Hamano schrieb:\n>> (2) The intended use-case of --merge-safe is to point the branch head to a\n>> different commit, but to carry the changes that currently are in the index\n>> and wd over to the new commit, similar to checkout --merge.\n\nThis is actually an operation that I need quite often, but I can do it\nonly by way of git stash.\n\nClarification: I did not say that I actually meant to carry *only* the\nindex and wd changes to the new commit. That is, the operation I have in\nmind can roughly be done in terms of\n\n $ git stash\n $ git reset --hard $target\n $ git stash pop\n\n> I have _no_ idea what the intended use-case of --merge-safe is, and that\n> was why I asked Christian for clarification in the previous round. The\n> answer was still not clear enough so I pointed out --merge-safe could be\n> still doing a wrong thing even in _his_ use-case.\n\nReading Christian in 200909170554.49416.chriscool@tuxfamily.org, I think\nthis *is* his use-case? Christian?\n\n-- Hannes\n"},{"id":"123439","messageId":"c07716ae0909170512k3c2ec7m21e69e9acfdfd81f@mail.gmail.com","threadId":"20985","inReplyTo":"4AB1E41F.6050304@viscovery.net","subject":"Re: [PATCH v3 3/4] reset: add option \"--merge-safe\" to \"git reset\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2009-09-17T12:12:50Z","receivedAt":"2009-09-17T12:12:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Sep 17, 2009 at 9:24 AM, Johannes Sixt wrote:\n> Junio C Hamano schrieb:\n>> Johannes Sixt writes:\n>>> Junio C Hamano schrieb:\n>>> (2) The intended use-case of --merge-safe is to point the branch head to a\n>>> different commit, but to carry the changes that currently are in the index\n>>> and wd over to the new commit, similar to checkout --merge.\n>\n> This is actually an operation that I need quite often, but I can do it\n> only by way of git stash.\n>\n> Clarification: I did not say that I actually meant to carry *only* the\n> index and wd changes to the new commit. That is, the operation I have in\n> mind can roughly be done in terms of\n>\n>  $ git stash\n>  $ git reset --hard $target\n>  $ git stash pop\n>\n>> I have _no_ idea what the intended use-case of --merge-safe is, and that\n>> was why I asked Christian for clarification in the previous round.  The\n>> answer was still not clear enough so I pointed out --merge-safe could be\n>> still doing a wrong thing even in _his_ use-case.\n>\n> Reading Christian in 200909170554.49416.chriscool@tuxfamily.org, I think\n> this *is* his use-case? Christian?\n\nYes, I agree, it can be used instead of git stash. By the way Linus, in his\npatch that added the --merge option, said that --merge could be used like\nthat.\n\nSo for this use case --merge-safe is safer than --merge, and that's why I\nsuggested that name. But I know I suck at naming things, so there is\nprobably a better name.\n\nThanks,\nChristian.\n"},{"id":"123441","messageId":"c07716ae0909170525h21ab26f5y84b0fbce2192c69@mail.gmail.com","threadId":"20985","inReplyTo":"7vk4zykv7o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 3/4] reset: add option \"--merge-safe\" to \"git reset\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2009-09-17T12:25:40Z","receivedAt":"2009-09-17T12:25:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Sep 17, 2009 at 7:15 AM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> From: Stephan Beyer \n>>\n>> This option is nearly like \"--merge\" except that it is\n>> safer.\n>\n> Do you still want to have this description after the last round?\n>\n>> The table below shows what happens when running\n>> \"git reset --option target\" to reset the HEAD to another\n>> commit (as a special case \"target\" could be the same as\n>> HEAD) in the cases where \"--merge\" and \"--merge-safe\"\n>> (abreviated --m-s) behave differently.\n>>\n>> working index HEAD target         working index HEAD\n>>   B      B     A     A   --m-s      B      A     A\n>>                          --merge    A      A     A\n>>   B      B     A     C   --m-s       (disallowed)\n>>                          --merge    C      C     C\n>\n> I'd like to see at least the following rows filled as well.\n>\n>    X      U     A     A   --m-s      ???    ???   ???\n>                           --merge    ???    ???   ???\n>    X      U     B     A   --m-s      ???    ???   ???\n>                           --merge    ???    ???   ???\n>\n>> In this table, A, B and C are some different states of a file.\n>\n> ... and X is \"don't care\", and U is \"unmerged index\".\n\nI will have a look, but it seems to me that --m-s and --merge\nbehave the same in these cases.\n\n>> The code comes from the sequencer GSoC project:\n>>\n>> git://repo.or.cz/git/sbeyer.git\n>>\n>> (at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)\n>>\n>> But in the sequencer project the \"reset\" flag was set in the \"struct\n>> unpack_trees_options\" passed to \"unpack_trees()\". With this flag the\n>> changes in the working tree were discarded if the file was different\n>> between HEAD and the reset target.\n>\n> If you need to have four lines worth of description here, is this still\n> Stephan's patch, or would it be more appropriate to say \"This is based on\n> an earlier GSoC work by Stephan in git://repo.or.cz/git/sbeyer.git\n> repository.\" and you take all the credit and blame?\n\nAs you insist, I will take all the credit and blame.\n\n>>  static const char * const git_reset_usage[] = {\n>> -     \"git reset [--mixed | --soft | --hard | --merge] [-q] []\",\n>> +     \"git reset [--mixed | --soft | --hard | --merge | --merge-safe] [-q] []\",\n>>       \"git reset [--mixed] [--] ...\",\n>>       NULL\n>>  };\n>\n> As we established in the previous round, this is _different_ from --merge,\n> but *not* in the sense that --merge is more dangerous and users should be\n> using this new option instead, but in the sense that --merge perfectly\n> works well for its intended use case, and this new option triggers a mode\n> of operation that is meant to be used in a completely different use case,\n> which is unspecified in this series without documentation.\n>\n> In that light, is --merge-safe still a good name for the option, or merely\n> a misleading one?\n\nI agree that it could be misleading. What about \"--stash\" ?\n\n> As I said in the previous round, --merge discards the modified index state\n> when switching, and it is absolutely _the right thing to do_ in the use\n> case it was intended for.  It is _not_ dangerous, and using --merge-safe\n> in that scenario is not being _safe_ but is actively a _wrong_ thing to do.\n\nOk.\n\n>> @@ -95,6 +98,16 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet\n>>\n>>       read_cache_unmerged();\n>>\n>> +     if (reset_type == MERGE_SAFE) {\n>> +             unsigned char *head_sha1;\n>> +             if (get_sha1(\"HEAD\", head_sha1))\n>> +                     return error(\"You do not have a valid HEAD.\");\n>> +             if (parse_and_init_tree_desc(head_sha1, desc))\n>> +                     return error(\"Failed to find tree of HEAD.\");\n>> +             nr++;\n>> +             opts.fn = twoway_merge;\n>> +     }\n>\n> get_sha1() takes an allocated buffer, does not allocate space on its own.\n> I think you meant \"unsigned char head_sha1[20];\" here.\n\nYes.\n\nThanks,\nChristian.\n"},{"id":"123445","messageId":"4AB23410.6080508@viscovery.net","threadId":"20985","inReplyTo":"c07716ae0909170512k3c2ec7m21e69e9acfdfd81f@mail.gmail.com","subject":"Re: [PATCH v3 3/4] reset: add option \"--merge-safe\" to \"git reset\"","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-09-17T13:05:20Z","receivedAt":"2009-09-17T13:05:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Christian Couder schrieb:\n> On Thu, Sep 17, 2009 at 9:24 AM, Johannes Sixt wrote:\n>> Junio C Hamano schrieb:\n>>> Johannes Sixt writes:\n>> $ git stash\n>> $ git reset --hard $target\n>> $ git stash pop\n>>\n>>> I have _no_ idea what the intended use-case of --merge-safe is, and that\n>>> was why I asked Christian for clarification in the previous round. The\n>>> answer was still not clear enough so I pointed out --merge-safe could be\n>>> still doing a wrong thing even in _his_ use-case.\n>> Reading Christian in 200909170554.49416.chriscool@tuxfamily.org, I think\n>> this *is* his use-case? Christian?\n> \n> Yes, I agree, it can be used instead of git stash.\n\nIt \"can\"? Do you say that you intend --merge-safe for something else in\naddition to the above stash + reset --hard + stash pop sequence? What?\n\n> By the way Linus, in his\n> patch that added the --merge option, said that --merge could be used like\n> that.\n\nBut that use-case has one important difference: You can't use stash right\nbefore the reset:\n\n # work tree is dirty\n $ git pull $there $topic # assume we have conflicts\n\n # investigate result ...\n # oh no, that's crap, scratch it\n\n $ git stash what? conflicted changes?\n $ git reset what? --hard would remove my dirty state, too\n\nYou are screwed. 'git reset --merge' comes to rescue.\n\nI'm pretty sure you don't mean --merge-safe to provide extra safety in\n*this* use-case, but that you have a very different use-case in mind.\n\n-- Hannes\n"},{"id":"123448","messageId":"c07716ae0909170625s7d7e93a5n500db9e4d341335f@mail.gmail.com","threadId":"20985","inReplyTo":"4AB23410.6080508@viscovery.net","subject":"Re: [PATCH v3 3/4] reset: add option \"--merge-safe\" to \"git reset\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2009-09-17T13:25:23Z","receivedAt":"2009-09-17T13:25:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Sep 17, 2009 at 3:05 PM, Johannes Sixt wrote:\n> Christian Couder schrieb:\n>> On Thu, Sep 17, 2009 at 9:24 AM, Johannes Sixt wrote:\n>>> Junio C Hamano schrieb:\n>>>> Johannes Sixt writes:\n>>>  $ git stash\n>>>  $ git reset --hard $target\n>>>  $ git stash pop\n>>>\n>>>> I have _no_ idea what the intended use-case of --merge-safe is, and that\n>>>> was why I asked Christian for clarification in the previous round.  The\n>>>> answer was still not clear enough so I pointed out --merge-safe could be\n>>>> still doing a wrong thing even in _his_ use-case.\n>>> Reading Christian in 200909170554.49416.chriscool@tuxfamily.org, I think\n>>> this *is* his use-case? Christian?\n>>\n>> Yes, I agree, it can be used instead of git stash.\n>\n> It \"can\"? Do you say that you intend --merge-safe for something else in\n> addition to the above stash + reset --hard + stash pop sequence? What?\n\nAs I said to Junio, another \"use case\" is to enable more commands (like\nan improved cherry-pick) to be used with a dirty work tree or index.\n\n>> By the way Linus, in his\n>> patch that added the --merge option, said that --merge could be used like\n>> that.\n>\n> But that use-case has one important difference: You can't use stash right\n> before the reset:\n>\n>   # work tree is dirty\n>   $ git pull $there $topic  # assume we have conflicts\n>\n>   # investigate result ...\n>   # oh no, that's crap, scratch it\n>\n>   $ git stash what? conflicted changes?\n>   $ git reset what? --hard would remove my dirty state, too\n>\n> You are screwed. 'git reset --merge' comes to rescue.\n>\n> I'm pretty sure you don't mean --merge-safe to provide extra safety in\n> *this* use-case,\n\nYou are right, I don't think it is usefull in this use case.\n\n> but that you have a very different use-case in mind.\n\nI don't have other use cases I didn't already talked about in mind.\n\nBest regards,\nChristian.\n"},{"id":"123464","messageId":"7v1vm5i9o6.fsf@alter.siamese.dyndns.org","threadId":"20985","inReplyTo":"4AB23410.6080508@viscovery.net","subject":"Re: [PATCH v3 3/4] reset: add option \"--merge-safe\" to \"git reset\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-17T20:43:53Z","receivedAt":"2009-09-17T20:43:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Christian Couder schrieb:\n>> On Thu, Sep 17, 2009 at 9:24 AM, Johannes Sixt wrote:\n>>> Junio C Hamano schrieb:\n>>>> Johannes Sixt writes:\n>>> $ git stash\n>>> $ git reset --hard $target\n>>> $ git stash pop\n>>\n> It \"can\"? Do you say that you intend --merge-safe for something else in\n> addition to the above stash + reset --hard + stash pop sequence? What?\n\nI think I am starting to understand. The use case in a larger picture\nwould look like\n\n $ (edit/add/commit)+ to work towards something\n ... And you _finished_ that something.\n $ (edit/add)+ to work towards something else that is wonderful\n ... Now you notice that all the commits for that earlier something\n ... are crap and you would want to discard them, while still keeping\n ... changes for \"something else that is wonderful\".\n ... Luckily, you haven't committed anything you would want to keep,\n ... and you can afford to reset the tip commits away.\n $ git reset --keep-local-changes HEAD~7\n ... or howmanyever commits you want to discard.\n\nThe reset in this case is done purely to discard the crap, not to redo\nthem (you have something else you want to work on in your work tree and\nindex, and they are not fixups---if they were you wouldn't have resetted\nbut used \"commit --amend\" or \"rebase -i\").\n\nIt is more like switching branches but in this case you are switching to\nyour own branch immediately after rewinding that same branch.\n\nIn other words, as far as the index, the work tree and where HEAD will\npoint at are concerned, the new mode of reset works exactly like this:\n\n $ (edit/add)+ to work towards something wonderful\n ... notice that the work does not belong to the current branch\n ... and you would want to switch to another branch while carrying\n ... your local changes.\n $ git checkout another-branch\n\nThe only difference being that reset will stay on the same branch and\nremove some commits from the tip of it, while checkout will leave the\noriginal branch intact.\n\nIt makes sense that it has the same \"safety\" as \"checkout\" has when\nswitching branches; when you have modification in the index for a path,\nand the path is different between switched-to commit and the current\ncommit, the command errors out with \"cannot merge\" (or a better message).\n\nOne drawback is that you can use this new mode of resetting only until you\nmake a commit that is part of the new \"something else that is wonderful\"\ntopic. After that \"git reset\" with this new option is not useful for this\nworkflow, and you would need to stash then rebase -i then unstash.\n"},{"id":"123465","messageId":"alpine.LNX.2.00.0909171654500.14907@iabervon.org","threadId":"20985","inReplyTo":"c07716ae0909170525h21ab26f5y84b0fbce2192c69@mail.gmail.com","subject":"Re: [PATCH v3 3/4] reset: add option \"--merge-safe\" to \"git reset\"","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-09-17T21:04:42Z","receivedAt":"2009-09-17T21:04:42Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 17 Sep 2009, Christian Couder wrote:\n\n> On Thu, Sep 17, 2009 at 7:15 AM, Junio C Hamano wrote:\n>\n> > I'd like to see at least the following rows filled as well.\n> >\n> >    X      U     A     A   --m-s      ???    ???   ???\n> >                           --merge    ???    ???   ???\n> >    X      U     B     A   --m-s      ???    ???   ???\n> >                           --merge    ???    ???   ???\n> >\n> >> In this table, A, B and C are some different states of a file.\n> >\n> > ... and X is \"don't care\", and U is \"unmerged index\".\n> \n> I will have a look, but it seems to me that --m-s and --merge\n> behave the same in these cases.\n\nIt's still worth documenting, since it used to be entirely undocumented \n(and probably not what it should have been), and also because you're \nmaking it something reliable for the first time in this series.\n\n\t-Daniel\n*This .sig left intentionally blank*"}]}