From: "Shawn O. Pearce" <spearce@spearce•org>
To: Felipe Contreras <felipe.contreras@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 1/1] fast-import: show a warning for non-existent files.
Date: Mon, 1 Sep 2008 12:25:06 -0700 [thread overview]
Message-ID: <20080901192506.GE7482@spearce.org> (raw)
In-Reply-To: <1220275214-6178-1-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail•com> wrote:
> This is useful in certain SCMs like monotone, where each 'merge revision' has
> the changes of all the micro-branches merged. So it appears as duplicated commands.
>
> The delete command was ignoring the issue completely. The rename/copy commands
> where throwing a fatal exception.
Signed-off-by line? See Documentation/SubmittingPatches.
> diff --git a/fast-import.c b/fast-import.c
> index 7089e6f..3dd2ab6 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1952,7 +1953,13 @@ static void file_change_d(struct branch *b)
> die("Garbage after path in: %s", command_buf.buf);
> p = uq.buf;
> }
> - tree_content_remove(&b->branch_tree, p, NULL);
> + memset(&leaf, 0, sizeof(leaf));
> + tree_content_remove(&b->branch_tree, p, &leaf);
> + if (!leaf.versions[1].mode)
> + {
> + warning("Path %s not in branch", p);
> + return;
> + }
> }
>
> static void file_change_cr(struct branch *b, int rename)
This is going to leak memory unless you add this before the
if (..mode) condition:
if (leaf->tree)
release_tree_content_recursive(e->tree);
We didn't worry about deleting a path that doesn't exist because
the importer clearly wants it gone. If it wants it gone and it is
already gone then it should be fine to ignore the delete command.
But as I point out below some import front-ends should be accurate
enough that they should not send a 'D' command unless the path is
already in the tree. Thus this can be an error condition for some
types of frontends, but can be ignored for others.
> @@ -1994,7 +2001,10 @@ static void file_change_cr(struct branch *b, int rename)
> else
> tree_content_get(&b->branch_tree, s, &leaf);
> if (!leaf.versions[1].mode)
> - die("Path %s not in branch", s);
> + {
> + warning("Path %s not in branch", s);
> + return;
> + }
> tree_content_set(&b->branch_tree, d,
> leaf.versions[1].sha1,
> leaf.versions[1].mode,
Normally we consider invalid paths to be an error. I wonder if this
should still be an error, unless the front-end passes an option on
the command line. Then monotone based importers can make these
warnings, but other importers that don't have this problem can
still treat them what they are, which is a fatal error.
Did you run the test suite (t/t9300-fast-import.sh) after your patch?
I would have thought a few of the bad path errors should be caught
there.
--
Shawn.
next prev parent reply other threads:[~2008-09-01 19:26 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-09-01 13:20 [PATCH 1/1] fast-import: show a warning for non-existent files Felipe Contreras
2008-09-01 19:04 ` Junio C Hamano
2008-09-01 21:58 ` Felipe Contreras
2008-09-01 19:25 ` Shawn O. Pearce [this message]
2008-09-01 22:01 ` Felipe Contreras
2008-09-01 22:30 ` [PATCH] fast-import: add ignore non-existent files option Felipe Contreras
2008-09-01 22:38 ` Shawn O. Pearce
2008-09-01 22:52 ` Felipe Contreras
2008-09-02 4:39 ` Shawn O. Pearce
2008-09-02 4:53 ` Junio C Hamano
2008-09-02 5:35 ` Shawn O. Pearce
2008-09-02 7:36 ` Junio C Hamano
2008-09-02 7:48 ` Felipe Contreras
2008-09-01 23:04 ` Junio C Hamano
2008-09-01 23:25 ` Felipe Contreras
2008-09-02 2:07 ` Junio C Hamano
2008-09-02 7:57 ` Felipe Contreras
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=20080901192506.GE7482@spearce.org \
--to=spearce@spearce$(echo .)org \
--cc=felipe.contreras@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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