git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Cc: Elijah Newren <newren@gmail.com>
Subject: [PATCH] merge-recursive: tweak magic band-aid
Date: Thu, 17 Mar 2011 23:07:20 -0700	[thread overview]
Message-ID: <7v1v250xnr.fsf_-_@alter.siamese.dyndns.org> (raw)
In-Reply-To: <7vpqpp1kww.fsf@alter.siamese.dyndns.org> (Junio C. Hamano's message of "Thu, 17 Mar 2011 14:45:03 -0700")

Running checks against working tree (e.g. lstat()) and causing
changes to working tree (e.g. unlink()) while building a virtual
ancestor merge does not make any sense. Avoid doing so.

This is not a real fix; it is another magic band-aid on top of
another band-aid we placed earlier.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This does not fix the "even though we have local change in the working
   tree, we do have a clean index entry for that path that happens to be
   the one we renamed, and they didn't" case this message responds to, it
   does seem to fix the real-life breakage I saw when I merged 'maint' to
   'master' yesterday, admittedly in a clean working tree.

   merge-recursive is riddled with places that touch/inspect working tree
   when it shouldn't, and it is beyond salvage without a major refactoring
   in its current shape, so this magic band-aid should do for now.

   Generally speaking, the only valid kinds of accesses a merge strategy
   is allowed are:

   (1) to compare the working tree file with our original index entry to
       see if it has local changes. This should be done only for paths
       that the index entry of the merge result is different from the
       current one and the working tree file needs updating.  We need this
       check because the merge must not lose such local changes when we
       checkout the merge result. This check could use lstat(2) as part of
       ce_match_stat() from read-cache.c; or

   (2) to make sure there is no file that was not tracked in our original
       index in the working tree at the path that the result of the merge
       needs to create a file or a directory. We need this check because
       the merge must not lose such an untracked file when we checkout the
       merge result. This check would use lstat(2), as part of
       checkout_entry() from entry.c; or

   (3) when the result of the merge needs to create a file at a path and
       the working tree has a directory at the path, to make sure that the
       contents of the directory does not have any locally modified files
       relative to our original index, or untracked-and-unignored files.
       We need this check because the merge must be able to "rm -r" such a
       directory safely in order to checkout the merge result.

   As a special case, a file missing from the working tree is considered
   unmodified for the purpose of (1). IOW, removal of a path from the
   working tree without touching the index is considered a local change
   that we are willing to lose during a merge. This is to support a
   workflow (or a Porcelain script) that merges commits in a fresh
   temporary directory, e.g. this sequence

	cd_to_toplevel
	mkdir tmp_merge
        cd tmp_merge
        export GIT_DIR=../.git
        git read-tree pu ;# the branch may not be related to the current one
        git merge-resolve $(git merge-base pu topic) -- pu topic

   would start from an empty temporary merge directory, check out only the
   paths that were involved in the merge, potentially leaving conflict
   markers in them, and allows the user to resolve them, without touching
   the real working tree or the current branch. The resulting index can be
   written to a tree object to record the result of the merge.

 merge-recursive.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 847bc84..59482ff 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -370,6 +370,13 @@ static void make_room_for_directories_of_df_conflicts(struct merge_options *o,
 	struct stage_data *last_e;
 	int i;
 
+	/*
+	 * Do not do any of this crazyness during the recursive; we don't
+	 * even write anything to the working tree!
+	 */
+	if (o->call_depth)
+		return;
+
 	for (i = 0; i < entries->nr; i++) {
 		const char *path = entries->items[i].string;
 		int len = strlen(path);
@@ -1274,7 +1281,7 @@ static int merge_content(struct merge_options *o,
 
 	if (mfi.clean && !df_conflict_remains &&
 	    sha_eq(mfi.sha, a_sha) && mfi.mode == a.mode &&
-	    lstat(path, &st) == 0) {
+	    !o->call_depth && !lstat(path, &st)) {
 		output(o, 3, "Skipped %s (merged same as existing)", path);
 		add_cacheinfo(mfi.mode, mfi.sha, path,
 			      0 /*stage*/, 1 /*refresh*/, 0 /*options*/);
-- 
1.7.4.1.494.g5ddab

  reply	other threads:[~2011-03-18  6:07 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-03-17  0:39 [BUG] merge-recursive triggered "BUG" Junio C Hamano
2011-03-17 21:45 ` Junio C Hamano
2011-03-18  6:07   ` Junio C Hamano [this message]
2011-03-21 18:24     ` [PATCH] merge-recursive: tweak magic band-aid Elijah Newren
2011-05-20  1:14 ` [BUG] merge-recursive triggered "BUG" Jay Soffian
2011-05-20  1:17   ` Jay Soffian
2011-05-20  3:21   ` Junio C Hamano
2011-05-20 12:29     ` Jay Soffian
2011-05-20 13:00       ` Jay Soffian

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=7v1v250xnr.fsf_-_@alter.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    /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).