Re: [PATCH/RFC 1/3] Teach mv to move submodules together with their work trees
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- Apr 11, 2013, 09:12 UTC
- Message-ID
- <CALkWK0mWHwTV8dc9F3tyq9HYOnC2S56x_efr1+eRsqJqTFutXA@mail.gmail.com>
- In-Reply-To
- <515C8958.4080704@web.de>
Jens Lehmann wrote:
Show 6 quoted lines
> Currently the attempt to use "git mv" on a submodule errors out with: > fatal: source directory is empty, source=<src>, destination=<dest> > The reason is that mv searches for the submodule with a trailing slash in > the index, which it doesn't find (because it is stored without a trailing > slash). As it doesn't find any index entries inside the submodule it > claims the directory would be empty even though it isn't.
Why does it search for a submodule with a trailing slash in the index? You make it sound like it's doing something unnatural; in reality, it does this because it executes lstat() on the filesystem path specified, and the stat mode matches S_ISDIR (because it _is_ a directory on the filesystem). It is stored in the index as an entry (without a trailing slash) with the mode 160000, gitlink.
What happens if I attempt to git mv oldpath/ newpath/ (with the literal slashes, probably because I'm using a stupid shell completion)?
> Fix that by searching for the name without a trailing slash and continue > if it is a submodule.
So, the correct solution is not to "search for a name without a trailing slash", but rather to handle the gitlink entry in the index appropriately.
> Then rename() will move the submodule work tree just > like it moves a file.
What is this rename() function you're talking about? I don't see it anywhere.
Show 46 quoted lines
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 034fec9..361028d 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -117,55 +117,60 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> && lstat(dst, &st) == 0)
> bad = _("cannot move directory over file");
> else if (src_is_dir) {
> - const char *src_w_slash = add_slash(src);
> - int len_w_slash = length + 1;
> - int first, last;
> -
> - modes[i] = WORKING_DIRECTORY;
> -
> - first = cache_name_pos(src_w_slash, len_w_slash);
> - if (first >= 0)
> - die (_("Huh? %.*s is in index?"),
> - len_w_slash, src_w_slash);
> -
> - first = -1 - first;
> - for (last = first; last < active_nr; last++) {
> - const char *path = active_cache[last]->name;
> - if (strncmp(path, src_w_slash, len_w_slash))
> - break;
> - }
> - free((char *)src_w_slash);
> -
> - if (last - first < 1)
> - bad = _("source directory is empty");
> - else {
> - int j, dst_len;
> -
> - if (last - first > 0) {
> - source = xrealloc(source,
> - (argc + last - first)
> - * sizeof(char *));
> - destination = xrealloc(destination,
> - (argc + last - first)
> - * sizeof(char *));
> - modes = xrealloc(modes,
> - (argc + last - first)
> - * sizeof(enum update_mode));
> + int first = cache_name_pos(src, length);
> + if (first >= 0) {
> + if (!S_ISGITLINK(active_cache[first]->ce_mode))
> + die (_("Huh? Directory %s is in index and no submodule?"), src);I didn't understand this. Why does it have to be a gitlink if it is stored at index position >= 0? I'm assuming the else-case has nothing to do with the actual moving but rather something specific to directories (enumerating entries in it?), which is why it doesn't get executed when we find a gitlink.
Show 17 quoted lines
> + } else {
> + const char *src_w_slash = add_slash(src);
> + int last, len_w_slash = length + 1;
> +
> + modes[i] = WORKING_DIRECTORY;
> +
> + first = cache_name_pos(src_w_slash, len_w_slash);
> + if (first >= 0)
> + die (_("Huh? %.*s is in index?"),
> + len_w_slash, src_w_slash);
> +
> + first = -1 - first;
> + for (last = first; last < active_nr; last++) {
> + const char *path = active_cache[last]->name;
> + if (strncmp(path, src_w_slash, len_w_slash))
> + break;
> }Mostly unchanged, but I didn't understand the line first = -1 - first in the original. What is it doing? So, I'm guessing first is the cache position of the directory itself, and last stores the index of the "last" entry in the cache? What does that even mean?
Show 16 quoted lines
> -
> - dst = add_slash(dst);
> - dst_len = strlen(dst);
> -
> - for (j = 0; j < last - first; j++) {
> - const char *path =
> - active_cache[first + j]->name;
> - source[argc + j] = path;
> - destination[argc + j] =
> - prefix_path(dst, dst_len,
> - path + length + 1);
> - modes[argc + j] = INDEX;
> + free((char *)src_w_slash);
> +
> + if (last - first < 1)
> + bad = _("source directory is empty");This is exactly what was tripping us up earlier. Can you explain what last - first < 1 means?
Show 29 quoted lines
> + else {
> + int j, dst_len;
> +
> + if (last - first > 0) {
> + source = xrealloc(source,
> + (argc + last - first)
> + * sizeof(char *));
> + destination = xrealloc(destination,
> + (argc + last - first)
> + * sizeof(char *));
> + modes = xrealloc(modes,
> + (argc + last - first)
> + * sizeof(enum update_mode));
> + }
> +
> + dst = add_slash(dst);
> + dst_len = strlen(dst);
> +
> + for (j = 0; j < last - first; j++) {
> + const char *path =
> + active_cache[first + j]->name;
> + source[argc + j] = path;
> + destination[argc + j] =
> + prefix_path(dst, dst_len,
> + path + length + 1);
> + modes[argc + j] = INDEX;
> + }
> + argc += last - first;
> }Mostly unchanged, but hard to review because I can't easily see what changed and what didn't.
> - argc += last - first;
Why did you remove this line?