Re: [RFC PATCH] Support gitlinks in fast-import/export.

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Fri, 18 Jul 2008, Alexander Gavrilov wrote:

> 	What I'm unsure of is, should fast-export try to reuse commit
> 	marks for gitlinks where it happened to recognize the object,
> 	or always output the SHA as it is stored in the tree?

Are they commit marks?  No.  So they should be handled as marks, just as 
those for blobs and trees.

(They are commit marks in the _submodule_, but that does not matter here.)

> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 395c055..80c591a 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -481,6 +481,9 @@ in octal.  Git only supports the following modes:
>    what you want.
>  * `100755` or `755`: A normal, but executable, file.
>  * `120000`: A symlink, the content of the file will be the link target.
> +* `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 implements submodules.

s/\(implement\)s/\1/

> diff --git a/builtin-fast-export.c b/builtin-fast-export.c
> index d0a462f..14b1549 100644
> --- a/builtin-fast-export.c
> +++ b/builtin-fast-export.c
> @@ -123,8 +123,19 @@ static void show_filemodify(struct diff_queue_struct *q,
>  			printf("D %s\n", spec->path);
>  		else {
>  			struct object *object = lookup_object(spec->sha1);
> -			printf("M %06o :%d %s\n", spec->mode,
> -			       get_object_mark(object), spec->path);
> +			int mark = object ? get_object_mark(object) : 0;

As I said, that looks wrong.  Maybe we have to fake objects for the 
gitlinks.

> @@ -183,7 +194,8 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)
>  				    "", &rev->diffopt);
>  
>  	for (i = 0; i < diff_queued_diff.nr; i++)
> -		handle_object(diff_queued_diff.queue[i]->two->sha1);
> +		if (!S_ISGITLINK(diff_queued_diff.queue[i]->two->mode))
> +			handle_object(diff_queued_diff.queue[i]->two->sha1);

Why?  You do not want to export changes in the submodules?

> diff --git a/fast-import.c b/fast-import.c
> index e72b286..e7977c1 100644
> --- a/fast-import.c
> +++ b/fast-import.c

I'll let Shawn comment on that.  Oh, wait, it's his last day in work 
today.  Maybe I have something useful to say about this part of the patch, 
then, to save Shawn some work.

> @@ -1900,7 +1901,16 @@ static void file_change_m(struct branch *b)
>  		p = uq.buf;
>  	}
>  
> -	if (inline_data) {
> +	if (S_ISGITLINK(mode)) {
> +		if (inline_data)
> +			die("Git links cannot be specified 'inline': %s",
> +				command_buf.buf);
> +		else if (oe) {
> +			if (oe->type != OBJ_COMMIT)
> +				die("Not a commit (actually a %s): %s",
> +					typename(oe->type), command_buf.buf);

How is that supposed to work?  Do I understand correctly that you require 
the user to construct a commit object for the gitlink?  That would be 
actively wrong.

Oh, and your patch lacks test cases that demonstrate how you envisage the 
whole thing to work.

Ciao,
Dscho