Re: [PATCH 1/1] fast-import: show a warning for non-existent files.

From: Shawn O. Pearce <spearce@spearce.org>
Date: Sep 1, 2008, 19:25 UTC
Message-ID: <20080901192506.GE7482@spearce.org>
In-Reply-To: <1220275214-6178-1-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 5 quoted lines

> 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.

Show 19 quoted lines

> 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.

Show 12 quoted lines

> @@ -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.