Re: [PATCH/RFC] Teach fast-import to import subtrees named by tree id

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi Jonathan,

Jonathan Nieder writes:
> To simulate the svn cp command, it would be very useful to be
> replace an arbitrary file in the current revision by an
> arbitrary directory from a previous one.  Modify the filemodify
> command to allow that:
> 
>  M 040000 <tree id> pathname
> 
> This would be most useful in combination with a facility to
> print the commit ids for new revisions as they are written.

Thanks for this patch! I applied and tested it: works as
expected. It's ready for inclusion, yes?

Reviewed-by: Ramkumar Ramachandra <artagnon@gmail.com>

> ---
> I actually thought fast-import already did this until David
> mentioned that no, it does not.  Well, live and learn.
> 
> This and Sverre’s --print-marks command would allow svn-fe
> to be simplified a great deal.
> 
> I was not sure whether to add a "feature" specification for
> this, so I’ll try that as a separate patch.
> 
> Thoughts?
> Jonathan
> 
>  Documentation/git-fast-import.txt |    8 ++++-
>  fast-import.c                     |   24 ++++++++++------
>  t/t9300-fast-import.sh            |   54 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 75 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 19082b0..f4d9aeb 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -482,9 +482,11 @@ External data format::
>  	'M' SP <mode> SP <dataref> SP <path> LF
>  ....
>  +
> -Here `<dataref>` can be either a mark reference (`:<idnum>`)
> +Here usually `<dataref>` must be either a mark reference (`:<idnum>`)
>  set by a prior `blob` command, or a full 40-byte SHA-1 of an
> -existing Git blob object.
> +existing Git blob object.  If `<mode>` is `040000`` then
> +`<dataref>` must be the full 40-byte SHA-1 of an existing
> +Git tree object or a mark reference set with `--import-marks`.
>  
>  Inline data format::
>  	The data content for the file has not been supplied yet.
> @@ -509,6 +511,8 @@ in octal.  Git only supports the following modes:
>  * `160000`: A gitlink, SHA-1 of the object refers to a commit in
>    another repository. Git links can only be specified by SHA or through
>    a commit mark. They are used to implement submodules.
> +* `040000`: A subdirectory.  Subdirectories can only be specified by
> +  SHA or through a tree mark set with `--import-marks`.
>  
>  In both formats `<path>` is the complete path of the file to be added
>  (if not already existing) or modified (if already existing).
> diff --git a/fast-import.c b/fast-import.c
> index 1e5d66e..ad6843a 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2131,6 +2131,7 @@ static void file_change_m(struct branch *b)
>  	case S_IFREG | 0644:
>  	case S_IFREG | 0755:
>  	case S_IFLNK:
> +	case S_IFDIR:
>  	case S_IFGITLINK:
>  		/* ok */
>  		break;
> @@ -2176,23 +2177,28 @@ static void file_change_m(struct branch *b)
>  		 * another repository.
>  		 */
>  	} else if (inline_data) {
> +		if (S_ISDIR(mode))
> +			die("Directories cannot be specified 'inline': %s",
> +				command_buf.buf);

Okay. Since you've passed S_IFDIR in the earlier switch-case, you've
made sure that directories aren't specified inline here.

>  		if (p != uq.buf) {
>  			strbuf_addstr(&uq, p);
>  			p = uq.buf;
>  		}
>  		read_next_command();
>  		parse_and_store_blob(&last_blob, sha1, 0);
> -	} else if (oe) {
> -		if (oe->type != OBJ_BLOB)
> -			die("Not a blob (actually a %s): %s",
> -				typename(oe->type), command_buf.buf);
>  	} else {
> -		enum object_type type = sha1_object_info(sha1, NULL);
> +		enum object_type expected = S_ISDIR(mode) ?
> +						OBJ_TREE: OBJ_BLOB;
> +		enum object_type type = oe ? oe->type :
> +					sha1_object_info(sha1, NULL);

Instead allowing just blobs, you've allowed both blob and tree objects
to be specified here. I don't see any tree writing code in your
change, so I'm assuming blob and tree writing is just handled
transparently in Git.

>  		if (type < 0)
> -			die("Blob not found: %s", command_buf.buf);
> -		if (type != OBJ_BLOB)
> -			die("Not a blob (actually a %s): %s",
> -			    typename(type), command_buf.buf);
> +			die("%s not found: %s",
> +					S_ISDIR(mode) ?  "Tree" : "Blob",
> +					command_buf.buf);
> +		if (type != expected)
> +			die("Not a %s (actually a %s): %s",
> +				typename(expected), typename(type),
> +				command_buf.buf);
>  	}
>  	tree_content_set(&b->branch_tree, p, sha1, mode, NULL);

Conditionally printing either "blob" or "tree" in the error message. Okay.

> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index 131f032..50d5913 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh

Okay. Test passes.

-- Ram