On 7/21/2022 10:13 AM, Shaoxuan Yuan wrote:
> On 7/20/2022 1:59 AM, Derrick Stolee wrote:
>> On 7/19/2022 9:28 AM, Shaoxuan Yuan wrote:
>>> Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
>>> was everywhere and can be simplified.
>>
>>> else if (!lstat(dest_path[0], &st) &&
>>> S_ISDIR(st.st_mode)) {
>>> - dest_path[0] = add_slash(dest_path[0]);
>>> - destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
>>> + destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
>>
>> Hm. I find it interesting how this works even if the directory is _not_
>> present in the index. Is there a test that checks this kind of setup?
>>
>> git init &&
>> >file &&
>> git add file &&
>> git commit -m init &&
>> mkdir dir &&
>> git mv file dir/
>>
>> Locally, this indeed works with the following 'git status' detail:
>>
>> renamed: file -> dir/file
>
> In my impression, 'git-mv' does not seem to care about whether the
> <destination> directory is in index? Given that `rename()` works so
> long as the directory is in the working tree, and `rename_index_entry_at()`
> cares even less about the <destination> dir?
Right. Instead of changing the behavior, I'm asking you to double-check that
this behavior is tested. If not, then please add a test.
>>> + if (dst_w_slash != dest_path[0])
>>> + free((char *)dst_w_slash);
>>
>> Good that you are freeing this here. You might also want to set it to NULL
>> just in case.
>
> I was using the `FREE_AND_NULL()` macro, but I wasn't sure since other
> places in 'git-mv' only use `free()`. Though I think it is better to
> `FREE_AND_NULL()`.
free() is generally the way to go if it is clear that the variable
is about to go out-of-scope and could not possibly be referenced
again. Since there is a lot more of the current code block to go,
nulling the variable is good defensive programming.
Thanks,
-Stolee
