Re: [PATCH v1 4/7] mv: check if <destination> is a SKIP_WORKTREE_DIR

From: Derrick Stolee <derrickstolee@github.com>
To: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
Cc: vdye@github.com, gitster@pobox.com, git@vger.kernel.org
Subject: Re: [PATCH v1 4/7] mv: check if <destination> is a SKIP_WORKTREE_DIR
Date: Fri, 22 Jul 2022 08:48:08 -0400	[thread overview]
Message-ID: <25ca0c62-e3b4-e96a-4c44-285bcfef2143@github.com> (raw)
In-Reply-To: <ab96cbe0-f256-ea5e-8356-db11d2b773f4@gmail.com>

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