git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jens Lehmann <Jens.Lehmann@web.de>
To: Roy Liu <carsomyr@gmail.com>
Cc: Andrew Ardill <andrew.ardill@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH] git-submodule: a small fix
Date: Tue, 27 Sep 2011 09:24:13 +0200	[thread overview]
Message-ID: <4E817A1D.8050902@web.de> (raw)
In-Reply-To: <CAH5451k-6HHx2xxddJauE8=P1umjG=TnrcOKmQfeh=4GOzpCKQ@mail.gmail.com>

Am 27.09.2011 05:22, schrieb Andrew Ardill:
> On 27 September 2011 08:00, Roy Liu <carsomyr@gmail.com> wrote:
>> In git-submodule.sh, the "url" variable may contain a stale value from
>> the previous loop iteration, so clear it.
>>
>> --- git-submodule.sh.orig   2011-09-26 17:50:45.000000000 -0400
>> +++ git-submodule.sh    2011-09-26 17:51:18.000000000 -0400
>> @@ -370,6 +370,8 @@
>>            esac
>>            git config submodule."$name".url "$url" ||
>>            die "Failed to register url for submodule path '$path'"
>> +        else
>> +            url=""
>>        fi
>>
>>        # Copy "update" setting when it is not set yet
> 
> Perhaps a better commit description would be:
> 
> git-submodule: clear the url variable when not set to avoid using stale values

Yes, the commit description needs to describe what was changed (and you can
also drop the "git-", to start with "submodule:" is sufficient here). Also
it would be nice if the message would describe under what circumstances this
happens (how did you notice this problem?).

But I wonder if the patch does it the right way. While it fixes the basic
issue that "url" might not be set, I doubt it does what the user expects.
The place where the - sometimes uninitialized - variable "url" is used is
a few lines down:

		say "$(eval_gettext "Submodule '\$name' (\$url) registered for path '\$path'")"

I doesn't make much sense to say "Submodule 'foo' () registered for path 'foo'"
here. Shouldn't "url" be set to "$(git config "submodule.$name.url")"? And
when looking at the if you added the else to it might make sense to set it
unconditionally before the if and then test "$url" there instead of adding an
extra else.

      reply	other threads:[~2011-09-27  7:24 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-09-26 22:00 [PATCH] git-submodule: a small fix Roy Liu
2011-09-27  3:22 ` Andrew Ardill
2011-09-27  7:24   ` Jens Lehmann [this message]

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=4E817A1D.8050902@web.de \
    --to=jens.lehmann@web.de \
    --cc=andrew.ardill@gmail.com \
    --cc=carsomyr@gmail.com \
    --cc=git@vger.kernel.org \
    /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).