Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules

From: Jens Lehmann <jens.lehmann@web.de>
Date: Oct 13, 2013, 18:37 UTC
Message-ID: <525AE868.9050207@web.de>
In-Reply-To: <vpqr4bp6wkh.fsf@anie.imag.fr>

Am 13.10.2013 17:05, schrieb Matthieu Moy:

Show 9 quoted lines

> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>>  static struct lock_file lock_file;
>> +#define SUBMODULE_WITH_GITDIR ((const char *)1)
> 
> I don't like very much hardcoded addresses like this. Are you 100% sure
> address 1 will never be returned by xstrdup on any platform? The risk is
> small if not negligible, but I'm unconfortable with this. Isn't there an
> alternative (NULL, or empty string) that is guaranteed to never happen?

All alternatives I could think of would require an extra array storing that information, which I dismissed for performance and memory footprint reasons (NULL is already taken for not being a submodule). I think 1 is one of the safest hard coded addresses as on sane systems accessing the zeropage will trigger a segfault. And if someday someone wants to free the memory I expect the special casing of 1 to be rather obvious. But I'm open to alternatives and would change that if people disagree.

>> +test_expect_success 'git mv moves a submodule with a .git directory and .gitmodules' '
> 
> This doesn't seem to test the problem I was having (move a file, not a
> submodule). Is this intentional?

Yes. The first idea was to simply move the update_path_in_gitmodules() into the "if (submodule_gitfile[i])"-block, but that would have resulted in not updating .gitmodules for submodules with a .git directory, which I would consider a bug. So I thought this was worth an extra test case, while I wasn't sure testing mv of a regular file to not issue a warning is a very useful test case in submodule context.

> In any case, this fixes my problem, thanks!

Sure, glad to help and thanks for testing!