git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Elijah Newren <newren@gmail.com>
To: git@vger.kernel.org
Cc: Elijah Newren <newren@gmail.com>
Subject: [WIP PATCH 01/10] merge-recursive: Remove redundant check for removing rename source
Date: Mon, 21 Mar 2011 12:30:55 -0600	[thread overview]
Message-ID: <1300732264-9638-2-git-send-email-newren@gmail.com> (raw)
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

  reply	other threads:[~2011-03-21 18:24 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-03-21 18:30 [WIP PATCH 00/10] Work-in-progress merge-recursive work Elijah Newren
2011-03-21 18:30 ` Elijah Newren [this message]
2011-03-21 18:30 ` [WIP PATCH 02/10] Reminder to fix o->call_depth handling in conflict_rename_rename_1to2 Elijah Newren
2011-03-21 18:30 ` [WIP PATCH 03/10] A bunch of fixes and FIXMEs Elijah Newren
2011-03-21 18:30 ` [WIP PATCH 04/10] Correct a comment Elijah Newren
2011-03-21 18:30 ` [WIP PATCH 05/10] merge-recursive: Fix sorting order and directory change assumptions Elijah Newren
2011-03-21 18:31 ` [WIP PATCH 06/10] Add a comment pointing out a bug Elijah Newren
2011-03-21 18:31 ` [WIP PATCH 07/10] Good testcases Elijah Newren
2011-03-21 18:31 ` [WIP PATCH 08/10] More test scripts Elijah Newren
2011-03-21 18:31 ` [WIP PATCH 09/10] Tests and fixes associated with rename/rename conflicts Elijah Newren
2011-03-21 18:31 ` [WIP PATCH 10/10] Add new testcase (temp14) showing how undetected renames can cause or spuriously avoid merge conflicts Elijah Newren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1300732264-9638-2-git-send-email-newren@gmail.com \
    --to=newren@gmail.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).