{"thread":{"id":"47546","subject":"[PATCH 0/3] Some small fixes to merge-recursive and tests","startedAt":"2018-01-05T20:21:28Z","lastAt":"2018-01-05T21:26:57Z","messageCount":7,"participants":["Elijah Newren","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"335961","messageId":"20180105202001.24218-1-newren@gmail.com","threadId":"47546","inReplyTo":null,"subject":"[PATCH 0/3] Some small fixes to merge-recursive and tests","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-01-05T20:19:58Z","receivedAt":"2018-01-05T20:21:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"These three patches were the first three from\nen/rename-directory-detection[1], but as Stefan suggested[2], I'm submitting\nthem separately as they are independent fixes.\n\nChange from PATCHv5-rename-directory-detection:\n  * Split from the rest of the series.\n  * Added Stefan's Reviewed-By.\n\n[1] https://public-inbox.org/git/20171228041352.27880-1-newren@gmail.com/\n[2] https://public-inbox.org/git/CAGZ79kapuEKLO4RUUPVS6_-aeBERDhjpBAtmK=gycT8GaK2bFg@mail.gmail.com/\n\nElijah Newren (3):\n  Tighten and correct a few testcases for merging and cherry-picking\n  merge-recursive: fix logic ordering issue\n  merge-recursive: add explanation for src_entry and dst_entry\n\n merge-recursive.c             | 21 ++++++++++++++++++++-\n t/t3501-revert-cherry-pick.sh |  7 +++++--\n t/t7607-merge-overwrite.sh    |  5 ++++-\n 3 files changed, 29 insertions(+), 4 deletions(-)\n\n-- \n2.14.2\n"},{"id":"335962","messageId":"20180105202001.24218-2-newren@gmail.com","threadId":"47546","inReplyTo":"20180105202001.24218-1-newren@gmail.com","subject":"[PATCH 1/3] Tighten and correct a few testcases for merging and cherry-picking","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-01-05T20:19:59Z","receivedAt":"2018-01-05T20:21:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"t3501 had a testcase originally added in 05f2dfb965 (cherry-pick:\ndemonstrate a segmentation fault, 2016-11-26) to ensure cherry-pick\nwouldn't segfault when working with a dirty file involved in a rename.\nWhile the segfault was fixed, there was another problem this test\ndemonstrated: namely, that git would overwrite a dirty file involved in a\nrename.  Further, the test encoded a \"successful merge\" and overwriting of\nthis file as correct behavior.  Modify the test so that it would still\ncatch the segfault, but to require the correct behavior.  Mark it as\ntest_expect_failure for now too, since this second bug is not yet fixed.\n\nt7607 had a test added in 30fd3a5425 (merge overwrites unstaged changes in\nrenamed file, 2012-04-15) specific to looking for a merge overwriting a\ndirty file involved in a rename, but it too actually encoded what I would\nterm incorrect behavior: it expected the merge to succeed.  Fix that, and\nadd a few more checks to make sure that the merge really does produce the\nexpected results.\n\nReviewed-By: Stefan Beller <sbeller@google.com>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t3501-revert-cherry-pick.sh | 7 +++++--\n t/t7607-merge-overwrite.sh    | 5 ++++-\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 4f2a263b6..783bdbf59 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -141,7 +141,7 @@ test_expect_success 'cherry-pick \"-\" works with arguments' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'cherry-pick works with dirty renamed file' '\n+test_expect_failure 'cherry-pick works with dirty renamed file' '\n \ttest_commit to-rename &&\n \tgit checkout -b unrelated &&\n \ttest_commit unrelated &&\n@@ -150,7 +150,10 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n \ttest_tick &&\n \tgit commit -m renamed &&\n \techo modified >renamed &&\n-\tgit cherry-pick refs/heads/unrelated\n+\ttest_must_fail git cherry-pick refs/heads/unrelated >out &&\n+\ttest_i18ngrep \"Refusing to lose dirty file at renamed\" out &&\n+\ttest $(git rev-parse :0:renamed) = $(git rev-parse HEAD^:to-rename.t) &&\n+\tgrep -q \"^modified$\" renamed\n '\n \n test_done\ndiff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh\nindex 9444d6a9b..9c422bcd7 100755\n--- a/t/t7607-merge-overwrite.sh\n+++ b/t/t7607-merge-overwrite.sh\n@@ -97,7 +97,10 @@ test_expect_failure 'will not overwrite unstaged changes in renamed file' '\n \tgit mv c1.c other.c &&\n \tgit commit -m rename &&\n \tcp important other.c &&\n-\tgit merge c1a &&\n+\ttest_must_fail git merge c1a >out &&\n+\ttest_i18ngrep \"Refusing to lose dirty file at other.c\" out &&\n+\ttest_path_is_file other.c~HEAD &&\n+\ttest $(git hash-object other.c~HEAD) = $(git rev-parse c1a:c1.c) &&\n \ttest_cmp important other.c\n '\n \n-- \n2.14.2\n\n"},{"id":"335963","messageId":"20180105202001.24218-3-newren@gmail.com","threadId":"47546","inReplyTo":"20180105202001.24218-1-newren@gmail.com","subject":"[PATCH 2/3] merge-recursive: fix logic ordering issue","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-01-05T20:20:00Z","receivedAt":"2018-01-05T20:21:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"merge_trees() did a variety of work, including:\n  * Calling get_unmerged() to get unmerged entries\n  * Calling record_df_conflict_files() with all unmerged entries to\n    do some work to ensure we could handle D/F conflicts correctly\n  * Calling get_renames() to check for renames.\n\nAn easily overlooked issue is that get_renames() can create more\nunmerged entries and add them to the list, which have the possibility of\nbeing involved in D/F conflicts.  So the call to\nrecord_df_conflict_files() should really be moved after all the rename\ndetection.  I didn't come up with any testcases demonstrating any bugs\nwith the old ordering, but I suspect there were some for both normal\nrenames and for directory renames.  Fix the ordering.\n\nReviewed-By: Stefan Beller <sbeller@google.com>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d00b27438..98c84e73d 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1982,10 +1982,10 @@ int merge_trees(struct merge_options *o,\n \t\tget_files_dirs(o, merge);\n \n \t\tentries = get_unmerged();\n-\t\trecord_df_conflict_files(o, entries);\n \t\tre_head  = get_renames(o, head, common, head, merge, entries);\n \t\tre_merge = get_renames(o, merge, common, head, merge, entries);\n \t\tclean = process_renames(o, re_head, re_merge);\n+\t\trecord_df_conflict_files(o, entries);\n \t\tif (clean < 0)\n \t\t\tgoto cleanup;\n \t\tfor (i = entries->nr-1; 0 <= i; i--) {\n-- \n2.14.2\n\n"},{"id":"335964","messageId":"20180105202001.24218-4-newren@gmail.com","threadId":"47546","inReplyTo":"20180105202001.24218-1-newren@gmail.com","subject":"[PATCH 3/3] merge-recursive: add explanation for src_entry and dst_entry","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-01-05T20:20:01Z","receivedAt":"2018-01-05T20:21:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"If I have to walk through the debugger and inspect the values found in\nhere in order to figure out their meaning, despite having known these\nthings inside and out some years back, then they probably need a comment\nfor the casual reader to explain their purpose.\n\nReviewed-By: Stefan Beller <sbeller@google.com>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 98c84e73d..d78853d5e 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -513,6 +513,25 @@ static void record_df_conflict_files(struct merge_options *o,\n \n struct rename {\n \tstruct diff_filepair *pair;\n+\t/*\n+\t * Purpose of src_entry and dst_entry:\n+\t *\n+\t * If 'before' is renamed to 'after' then src_entry will contain\n+\t * the versions of 'before' from the merge_base, HEAD, and MERGE in\n+\t * stages 1, 2, and 3; dst_entry will contain the respective\n+\t * versions of 'after' in corresponding locations.  Thus, we have a\n+\t * total of six modes and oids, though some will be null.  (Stage 0\n+\t * is ignored; we're interested in handling conflicts.)\n+\t *\n+\t * Since we don't turn on break-rewrites by default, neither\n+\t * src_entry nor dst_entry can have all three of their stages have\n+\t * non-null oids, meaning at most four of the six will be non-null.\n+\t * Also, since this is a rename, both src_entry and dst_entry will\n+\t * have at least one non-null oid, meaning at least two will be\n+\t * non-null.  Of the six oids, a typical rename will have three be\n+\t * non-null.  Only two implies a rename/delete, and four implies a\n+\t * rename/add.\n+\t */\n \tstruct stage_data *src_entry;\n \tstruct stage_data *dst_entry;\n \tunsigned processed:1;\n-- \n2.14.2\n\n"},{"id":"336014","messageId":"nycvar.QRO.7.76.6.1801052221210.1337@wbunaarf-fpuvaqryva.tvgsbejvaqbjf.bet","threadId":"47546","inReplyTo":"20180105202001.24218-3-newren@gmail.com","subject":"Re: [PATCH 2/3] merge-recursive: fix logic ordering issue","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-01-05T21:22:52Z","receivedAt":"2018-01-05T21:23:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Fri, 5 Jan 2018, Elijah Newren wrote:\n\n> merge_trees() did a variety of work, including:\n>   * Calling get_unmerged() to get unmerged entries\n>   * Calling record_df_conflict_files() with all unmerged entries to\n>     do some work to ensure we could handle D/F conflicts correctly\n>   * Calling get_renames() to check for renames.\n> \n> An easily overlooked issue is that get_renames() can create more\n> unmerged entries and add them to the list, which have the possibility of\n> being involved in D/F conflicts.  So the call to\n> record_df_conflict_files() should really be moved after all the rename\n> detection.\n\nI cannot actually see how the *current* rename detection would result in\nD/F conflicts, as it is really only about detecting the renames, the\nconflicts should have happened earlier.\n\nBut I could imagine that your \"detect directory renames\" patch series\nchanges this behavior.\n\nIn any case, the reordering does not hurt.\n\nCiao,\nDscho\n"},{"id":"336015","messageId":"nycvar.QRO.7.76.6.1801052223130.1337@wbunaarf-fpuvaqryva.tvgsbejvaqbjf.bet","threadId":"47546","inReplyTo":"20180105202001.24218-2-newren@gmail.com","subject":"Re: [PATCH 1/3] Tighten and correct a few testcases for merging and cherry-picking","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-01-05T21:24:21Z","receivedAt":"2018-01-05T21:24:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Fri, 5 Jan 2018, Elijah Newren wrote:\n\n> t3501 had a testcase originally added in 05f2dfb965 (cherry-pick:\n> demonstrate a segmentation fault, 2016-11-26) to ensure cherry-pick\n> wouldn't segfault when working with a dirty file involved in a rename.\n>\n> While the segfault was fixed, there was another problem this test\n> demonstrated: namely, that git would overwrite a dirty file involved in a\n> rename.  Further, the test encoded a \"successful merge\" and overwriting of\n> this file as correct behavior.\n\nWhoops. Sorry about that. Thanks for the patch, looks obviously good (I\nhaven't read 3/3 yet, but figure that the error message stems from the\nchanges introduced in that patch).\n\nCiao,\nDscho\n"},{"id":"336016","messageId":"nycvar.QRO.7.76.6.1801052226200.1337@wbunaarf-fpuvaqryva.tvgsbejvaqbjf.bet","threadId":"47546","inReplyTo":"20180105202001.24218-4-newren@gmail.com","subject":"Re: [PATCH 3/3] merge-recursive: add explanation for src_entry and dst_entry","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-01-05T21:26:47Z","receivedAt":"2018-01-05T21:26:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Fri, 5 Jan 2018, Elijah Newren wrote:\n\n> If I have to walk through the debugger and inspect the values found in\n> here in order to figure out their meaning, despite having known these\n> things inside and out some years back, then they probably need a comment\n> for the casual reader to explain their purpose.\n\nMakes sense.\n\nThanks,\nDscho\n"}]}