git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: newren@gmail.com
To: git@vger.kernel.org
Cc: gitster@pobox.com, spearce@spearce.org, agladysh@gmail.com,
	Elijah Newren <newren@gmail.com>
Subject: [PATCHv3 6/6] fast-import: Improve robustness when D->F changes provided in wrong order
Date: Tue,  6 Jul 2010 23:20:34 -0600	[thread overview]
Message-ID: <1278480034-22939-7-git-send-email-newren@gmail.com> (raw)
In-Reply-To: <1278480034-22939-1-git-send-email-newren@gmail.com>

From: Elijah Newren <newren@gmail.com>

When older versions of fast-export came across a directory changing to a
symlink (or regular file), it would output the changes in the form
  M 120000 :239821 dir-changing-to-symlink
  D dir-changing-to-symlink/filename1
When fast-import sees the first line, it deletes the directory named
dir-changing-to-symlink (and any files below it) and creates a symlink in
its place.  When fast-import came across the second line, it was previously
trying to remove the file and relevant leading directories in
tree_content_remove(), and as a side effect it would delete the symlink
that was just created.  This resulted in the symlink silently missing from
the resulting repository.

To improve robustness, we ignore file deletions underneath directory names
that correspond to non-directories.  This can also be viewed as a minor
optimization: since there cannot be a file and a directory with the same
name in the same directory, the file clearly can't exist so nothing needs
to be done to delete it.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
Quoting Shawn's comments on the previous version of this patch:

  I'm not against making the input parser more robust...

  It probably makes sense to still do this in fast-import, deleting
  something that doesn't exist is probably OK, its still going to
  be deleted in the end anyway.

I'm guessing that probably qualifies as an Acked-by, but I'm not quite
sure.  However, I made a minor change to the patch: the previous
version only handled directory->symlink changes (S_ISLNK), whereas
this one also handles directory->(normal)file changes and
directory->gitlink changes (!S_ISDIR).

 fast-import.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 309f2c5..c8c717a 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1528,6 +1528,13 @@ static int tree_content_remove(
 	for (i = 0; i < t->entry_count; i++) {
 		e = t->entries[i];
 		if (e->name->str_len == n && !strncmp(p, e->name->str_dat, n)) {
+			if (slash1 && !S_ISDIR(e->versions[1].mode))
+				/* If p names a file in some subdirectory, and a
+				 * file or symlink matching the name of the
+				 * parent directory of p exists, then p cannot
+				 * exist and need not be deleted.
+				 */
+				return 1;
 			if (!slash1 || !S_ISDIR(e->versions[1].mode))
 				goto del_entry;
 			if (!e->tree)
-- 
1.7.1.1.10.g2e807

      parent reply	other threads:[~2010-07-07  5:13 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-07-07  5:20 [PATCHv3 0/6] D/F conflict fixes newren
2010-07-07  5:20 ` [PATCHv3 1/6] Add additional testcases for D/F conflicts newren
2010-07-07 21:25   ` Junio C Hamano
2010-07-08  3:07     ` Elijah Newren
2010-07-08 16:03       ` Junio C Hamano
2010-07-07  5:20 ` [PATCHv3 2/6] Add a rename + D/F conflict testcase newren
2010-07-07 21:26   ` Junio C Hamano
2010-07-08  5:04     ` Elijah Newren
2010-07-07  5:20 ` [PATCHv3 3/6] merge-recursive: Fix D/F conflicts newren
2010-07-07 21:40   ` Junio C Hamano
2010-07-08  4:34     ` Elijah Newren
2010-07-07  5:20 ` [PATCHv3 4/6] merge_recursive: Fix renames across paths below " newren
2010-07-07  5:20 ` [PATCHv3 5/6] fast-export: Fix output order of D/F changes newren
2010-07-07 21:40   ` Junio C Hamano
2010-07-08  4:36     ` Elijah Newren
2010-07-08  6:04       ` Junio C Hamano
2010-07-07  5:20 ` newren [this message]

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=1278480034-22939-7-git-send-email-newren@gmail.com \
    --to=newren@gmail.com \
    --cc=agladysh@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=spearce@spearce.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).