Re: [PATCH 4/4] transport: drop "int cmp = cmp" hack

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2013, 19:50 UTC
Message-ID: <7vfvzjxnq9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130324093212.GA28234@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 22 quoted lines

> On Sat, Mar 23, 2013 at 09:00:05PM -0700, Junio C Hamano wrote:
>
>> On Thu, Mar 21, 2013 at 4:13 AM, Jeff King <peff@peff.net> wrote:
>> >
>> > According to 47ec794, this initialization is meant to
>> > squelch an erroneous uninitialized variable warning from gcc
>> > 4.0.1.  That version is quite old at this point, and gcc 4.1
>> > and up handle it fine, with one exception. There seems to be
>> > a regression in gcc 4.6.3, which produces the warning;
>> > however, gcc versions 4.4.7 and 4.7.2 do not.
>> >
>> 
>> transport.c: In function 'get_refs_via_rsync':
>> transport.c:127:29: error: 'cmp' may be used uninitialized in this
>> function [-Werror=uninitialized]
>> transport.c:109:7: note: 'cmp' was declared here
>> 
>> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
>
> Right, that's the same version I noted above. Is 4.6.3 the default
> compiler under a particular release of Ubuntu, or did you use their
> gcc-4.6 package?

I'll check later with one of my VMs. The copy of U 12.04 I happened to have handy has that version installed.

By the way, I find this piece of code less than pleasant:

 * It uses "struct ref dummy = { NULL }, *tail = &dummy", and then
   accumulates things by appending to "&tail" and then returns
   dummy.next.  Why doesn't it do

	struct ref *retval = NULL, **tail = &retval;

   and pass tail around to append things, like everybody else?  Is
   this another instance of "People do not understand linked list"
   problem?  Perhaps fixing that may unconfuse the compiler?

 * Its read_loose_refs() is a recursive function that sorts the
   results from readdir(3) and iterates over them, expecting its
   recursive call to fail _only_ when the entry it read is not a
   directory that it needs to recurse into.

   It is not obvious if the resulting list is sorted correctly with
   this loop structure when you have branches "foo.bar", "foo/bar",
   and "foo=bar".  I think the loop first reads "foo", "foo.bar" and
   "foo=bar", sorts them in that order, and starts reading
   recursively, ending up with "foo/bar" first and then "foo.bar"
   and finally "foo=bar".  Later, the tail of the same list is
   passed to insert_packed_refs(), which does in-place merging of
   this list and the contents of the packed_refs file.  These two
   data sources have to be sorted the same way for this merge to
   work correctly, but there is no validating the order of the
   entries it reads from the packed-refs file.  At least, it should
   barf when the file is not sorted.  It could be lenient and accept
   a mal-sorted input, but I do not think that is advisable.

I'll apply the attached on 'maint' for now, as rsync is not worth spending too many cycles on worrying about; I need to go to the bathroom to wash my eyes after staring this code for 20 minutes X-<.

 transport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/transport.c b/transport.c
index 87b8f14..e6f9346 100644
--- a/transport.c
+++ b/transport.c
@@ -106,7 +106,8 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
 		return;
 
 	for (;;) {
-		int cmp, len;
+		int cmp = 0; /* assigned before used */
+		int len;
 
 		if (!fgets(buffer, sizeof(buffer), f)) {
 			fclose(f);