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

["Shawn O. Pearce" <spearce@spearce.org>]

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