git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[WIP PATCH 01/10] merge-recursive: Remove redundant check for removing rename source

From
Elijah Newren <newren@gmail.com>
Date
Mar 21, 2011, 18:30 UTC
Message-ID
<1300732264-9638-2-git-send-email-newren@gmail.com>
In-Reply-To
<1300732264-9638-1-git-send-email-newren@gmail.com>

If a file is renamed on the side of history being merged into head, the filename serving as the source of that rename needs to be removed from the working directory. (This statement is about a real merge rather than a merge of merge bases to come up with a virtual ancestor; in the latter case, no working directory changes of any type should be made).

The path to getting the above statement implemented in merge-recursive took
several steps.  The relevant bits of code may be instructive to keep in
mind for the explanation, especially since an English-only description
involves double negatives that are hard to follow.  These bits of code are:
  int remove_file(..., const char *path, int no_wd)
  {
    ...
    int update_working_directory = !o->call_depth && !no_wd;
and
  remove_file(o, 1, ren1_src, <expression>);
Where the choice for <expression> has morphed over time:

65ac6e9 (merge-recursive: adjust to loosened "working file clobbered" check 2006-10-27), introduced the "no_wd" parameter to remove_file() and used "1" for <expression>. This meant ren1_src was never deleted, leaving it around in the working copy.

In 8371234 (Remove uncontested renamed files during merge. 2006-12-13), <expression> was changed to "index_only" (where index_only == !!o->call_depth; see b7fa51da). This was equivalent to using "0" for <expression> (due to the early logic in remove_file), and is orthogonal to the condition we actually want to check at this point; it resulted in the source file being removed except when index_only was false. This was problematic because the file could have been renamed on the side of history including head, in which case ren1_src could correspond to an untracked file that should not be deleted.

In 183d797 (Keep untracked files not involved in a merge. 2007-02-04), <expression> was changed to "index_only || stage == 3". While this gives correct behavior, the "index_only ||" portion of <expression> is unnecessary and makes the code slightly harder to follow.

There were also two further changes to this expression, though without
any change in behavior.  First in b7fa51d (merge-recursive: get rid of the
index_only global variable 2008-09-02), it was changed to "o->call_depth
|| stage == 3".  (index_only == !!o->call_depth).  Earlier in this series
(merge-recursive: Small code clarification -- variable name and comments),
this was changed to "o->call_depth || renamed_stage == 2" (where stage
was renamed to other_stage and renamed_stage == other_stage ^ 1).
So we ended with <expression> being "o->call_depth || renamed_stage == 2".
But the "o->call_depth ||" piece was unnecessary.  Remove it.
---
 merge-recursive.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 42d52cb..6dc74dc 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1055,7 +1055,7 @@ static int process_renames(struct merge_options *o,
 			int renamed_stage = a_renames == renames1 ? 2 : 3;
 			int other_stage =   a_renames == renames1 ? 3 : 2;
 
-			remove_file(o, 1, ren1_src, o->call_depth || renamed_stage == 2);
+			remove_file(o, 1, ren1_src, renamed_stage == 2);
 
 			hashcpy(src_other.sha1, ren1->src_entry->stages[other_stage].sha);
 			src_other.mode = ren1->src_entry->stages[other_stage].mode;
-- 
1.7.4
Previous: Elijah NewrenNext: Elijah Newren
Message 2 of 11 in “Work-in-progress merge-recursive work”
  1. 00/10 Work-in-progress merge-recursive workElijah Newren, Mar 21, 2011
  2. 01/10 merge-recursive: Remove redundant check for removing rename sourceElijah Newren, Mar 21, 2011
  3. 02/10 Reminder to fix o->call_depth handling in conflict_rename_rename_1to2Elijah Newren, Mar 21, 2011
  4. 03/10 A bunch of fixes and FIXMEsElijah Newren, Mar 21, 2011
  5. 04/10 Correct a commentElijah Newren, Mar 21, 2011
  6. 05/10 merge-recursive: Fix sorting order and directory change assumptionsElijah Newren, Mar 21, 2011
  7. 06/10 Add a comment pointing out a bugElijah Newren, Mar 21, 2011
  8. 07/10 Good testcasesElijah Newren, Mar 21, 2011
  9. 08/10 More test scriptsElijah Newren, Mar 21, 2011
  10. 09/10 Tests and fixes associated with rename/rename conflictsElijah Newren, Mar 21, 2011
  11. 10/10 Add new testcase (temp14) showing how undetected renames can cause or spuriously avoid merge conflictsElijah Newren, Mar 21, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.