Re: [PATCH/RFC 1/3] Teach mv to move submodules together with their work trees
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 11, 2013, 16:27 UTC
- Message-ID
- <7vk3o9jan8.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <CALkWK0mWHwTV8dc9F3tyq9HYOnC2S56x_efr1+eRsqJqTFutXA@mail.gmail.com>
Ramkumar Ramachandra <artagnon@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.
Show 6 quoted lines
>> + 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
Show 22 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?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.
Show 5 quoted lines
>> + 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.