Re: [PATCH] git mv: Support moving submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 12, 2008, 22:19 UTC
Message-ID: <7vprn9m2hw.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080912214129.14829.53058.stgit@localhost>

Petr Baudis <pasky@suse.cz> writes:

Show 20 quoted lines

> diff --git a/builtin-mv.c b/builtin-mv.c
> index 4f65b5a..2970acc 100644
> --- a/builtin-mv.c
> +++ b/builtin-mv.c
> @@ -9,6 +9,7 @@
>  #include "cache-tree.h"
>  #include "string-list.h"
>  #include "parse-options.h"
> +#include "submodule.h"
>  
>  static const char * const builtin_mv_usage[] = {
>  	"git mv [options] <source>... <destination>",
> @@ -49,6 +50,24 @@ static const char *add_slash(const char *path)
>  	return path;
>  }
>  
> +static int ce_is_gitlink(int i)
> +{
> +	return i < 0 ? 0 : S_ISGITLINK(active_cache[i]->ce_mode);
> +}

This interface itself is ugly (why should a caller pass "it is unmerged or does not exist" without checking?), and it also makes the hunk that begins at 84/105 ugly. Why not "path_is_gitlink(const char*)" and run cache_name_pos() here instead?

The interface, even if it is internal, should be done with a better taste than that, even though I understand that you wanted to reuse the cache_pos for the source one while you check.

Oh, by the way, what should happen when you have an unmerged path in the index and say "git mv path elsewhere" (this question is *not* limited to submodules). Compared to that, what _does_ happen with the current code, and with your patch?