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

On Mon, Mar 25, 2013 at 12:50:54PM -0700, Junio C Hamano wrote:

quoted

quoted

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.

Ah, if you didn't explicitly run "gcc-4.6", then it was probably the
default version in 12.04 (as it was for a while in Debian testing, but
they never actually made a release with it, so everybody is now on 4.7
by default).

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?

Ugh, that is horrible. At first I thought it was even wrong, as we pass
&tail and not &dummy.next to read_loose_refs. But two wrongs _do_ make a
right, because read_loose_refs, rather than do:

  *tail = new;
  tail = &new->next;

does:

  (*tail)->next = new;
  *tail = new;

   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.

Actually, it is the head of the loose list (though it is hard to
realize, because it is called tail!).

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-<.

Yeah, it's quite ugly. I really wonder if it is time to drop rsync
support. I'd be really surprised if anybody is actively using it.

I wonder, though, what made you look at this. It did not come up in my
list of -Wuninitialized warnings. Did it get triggered by one of the
other gcc versions?

I think that's fine.

-Peff