From: Jens Lehmann <Jens.Lehmann@web.de>
To: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Cc: git <git@vger.kernel.org>, Jonathan Nieder <jrnieder@gmail.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules
Date: Sun, 13 Oct 2013 20:37:28 +0200 [thread overview]
Message-ID: <525AE868.9050207@web.de> (raw)
In-Reply-To: <vpqr4bp6wkh.fsf@anie.imag.fr>
Am 13.10.2013 17:05, schrieb Matthieu Moy:
> 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!
next prev parent reply other threads:[~2013-10-13 18:37 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-11 14:29 Spurious warning when moving a file in presence of submodules Matthieu Moy
2013-10-11 17:53 ` Jens Lehmann
2013-10-13 11:52 ` [PATCH] mv: Fix spurious " Jens Lehmann
2013-10-13 15:05 ` Matthieu Moy
2013-10-13 18:37 ` Jens Lehmann [this message]
2013-10-14 5:40 ` Jonathan Nieder
2013-10-14 12:33 ` Matthieu Moy
2013-10-17 18:24 ` Jonathan Nieder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=525AE868.9050207@web.de \
--to=jens.lehmann@web.de \
--cc=Matthieu.Moy@grenoble-inp.fr \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).