Re: [PATCH/RFC 1/3] Teach mv to move submodules together with their work trees
Jens Lehmann wrote:
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 16, 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.
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.
+ } 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,
Re: [PATCH/RFC 1/3] Teach mv to move submodules together with their work trees
Ramkumar Ramachandra artag...@gmail.com writes:
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.
man 2 rename; it is called from a generic part of builtin/mv.c to
rename one path to another and can move both files and directories.
+ 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?
The path is not in the middle of the conflict, but the index records
something that is not a gitlink. E.g. you start from A (regular file)
in the index, rm A mkdir A and make it the top of a working
tree of a separate project. git mv A elsewhere will say src_is_dir
but the index still thinks A should be a regular file blob.
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.
It wants to move all the paths in the directory to a new
destination, e.g. git mv srcdir dstdir, and update
+ } 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?
cache_name_pos() either returns the position for a path at stage #0
that exists in the index, or the position the given path _would_ be
inserted if you were to add it to the index for a path that does not
exist in the index at stage #0 (I think this part of the code does
not consider that the given path is unmerged, either by being sloppy
or detecting that case much earlier---I didn't check), and when
doing the latter, it encodes the position by negating it and
offsetting it by 1 (otherwise you cannot tell it would come at the
very beginning and it is at the very beginning, because negated
zero is still zero). The -1 - first is an idiom used everywhere
by callers of cache_name_pos() to recover the latter from the
returned value.
If you start at a position src/ would have been inserted, and
iterate over the index while the entry's path prefix-matches with
src/, you will find where the entries in the src/ directory
ends.
+ 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?
I think the above covers it.
Asking questions to learn the basic part of Git internals on this
list, e.g. I found this existing code, and I do not understand what
it is doing. Can somebody shed a light on it?, is perfectly fine,
but can you do so outside the review discussion?
It clutters the review discussions when such a request for education
is labeled as if it were a review or mixed with a message with
genuine review comments.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 1/3] Teach mv to move submodules together with their work trees
Ramkumar Ramachandra artag...@gmail.com writes: 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 16, gitlink. What happens if I attempt to git mv oldpath/ newpath/ (with the literal slashes, probably because I'm using a stupid shell completion)? I think it should work. mkdir a a/f git add a/f git mv a/ b/ 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. For moving an entire directory's contents, because the index is flat, you would look for name/, because you know all of the paths contained in it will share that prefix. But when dealing with a submodule, you need to see if the path that found to be a directory in the working tree is a gitlink in the index. And the way to do so is to look for it without trailing slash, because there is nothing hanging under it in the index. So the right implementation of handle appropriately is to search without slash. They are saying the same thing, and the latter is a more specific way to point out in what way the existing code that wanted to delegate moving to submodule mv and not having to worry about gitlinks was unprepared for it, and what change is needed to make it handle appropriately. So I think there is no need to rephrase here, but your comment makes me wonder if the patch covers the case where you have a submodule at a/x and the user does git mv a/ b/. The src_is_dir thing will notice a/ is a directory, finds all the paths inside a/ including a/x (and we won't see any paths inside the submodule's working tree) to b/, and update the cache entries and the working tree. Does the adjusting of the path for that moved submodule, which is a theme for [PATCH 3/3], cover that case, too? Another thing to wonder is if we correctly reject git mv a/x/f here in the same example where a/x is a directory. The path is beyond the lower boundary of our working tree and should not be touched. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC 1/3] Teach mv to move submodules together with their work trees
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.
Fix that by searching for the name without a trailing slash and continue
if it is a submodule. Then rename() will move the submodule work tree just
like it moves a file.
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
builtin/mv.c | 99 +++
t/t7001-mv.sh | 34
2 files changed, 86 insertions(+), 47 deletions(-)
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);
+ } 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;
}
-
- 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
