Re: [PATCH] fast-import: Add ability to copy a path from an arbitrary commit

From: Shawn O. Pearce <spearce@spearce.org>
Date: Oct 26, 2007, 07:01 UTC
Message-ID: <20071026070157.GM14735@spearce.org>
In-Reply-To: <20071020155749.29528.9258.julian@quantumfyre.co.uk>

Julian Phillips <julian@quantumfyre.co.uk> wrote:

> This adds the ability to copy a path from an arbitrary commit instead
> of just from the current commit.  This is done using the 'K' version
> of the filecopy sub-command.

So the reason I didn't apply this earlier when I was playing the role of "interim maintainer" was four fold:

- There's no test cases for this new 'K' command.  Everything else
  in fast-import has at least one test case for it, this should
  as well.

- There's a huge memory leak every time you use the 'K' command.
  See below in the code for where.

- The documentation doesn't talk about the special "/" source path.

- I was really busy and just forgot to reply to this email.

Other than that I think this is good.  I've just been too busy
to do it myself, and I'm glad that you took it upon yourself to
remedy that.

Show 19 quoted lines

> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index d511967..89aefea 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -534,6 +534,17 @@ location has been copied to the destination any future commands
>  applied to the source location will not impact the destination of
>  the copy.
>  
> +It is also possible to copy a file or directory from a commit other
> +than the current one.
> +
> +....
> +	'K' <dataref> SP <path> SP <path> LF
> +....
> +
> +where `<dataref>` is a reference (see `filemodify` above) to the
> +commit that the first (source) `<path>` is located in, the second
> +`<path>` is the destination.
> +

This should talk about the special case of the first (src) <path> being able to be "/" to copy the entire tree. If I follow your implementation right a src_path of "/" also causes the destination path to be ignored entirely. Which means you cannot copy another branch into a subdirectory of this branch (aka one direction of the subtree merge strategy).

> diff --git a/fast-import.c b/fast-import.c
> index e9c80be..9537c63 100644
> --- a/fast-import.c
> +++ b/fast-import.c

...

> +static void copy_from_commit(
> +	unsigned char *src_commit, const char *src_path,
> +        struct branch *dest_br, const char *dest_path)

...

> +	load_tree(&src_br.branch_tree);

This is a huge memory leak. You never unload this tree and return its data onto the freelists so we'll leak everything that wasn't moved into the destination tree.

-- 
Shawn.