Re: [PATCH v2 2/2] receive-pack: detect aliased updates which can occur with symrefs

From: Jay Soffian <jaysoffian@gmail.com>
Date: Apr 19, 2010, 20:57 UTC
Message-ID: <z2s76718491004191357gf4783770i84308e930d9adb22@mail.gmail.com>
In-Reply-To: <7veiibqirf.fsf@alter.siamese.dyndns.org>

On Mon, Apr 19, 2010 at 4:39 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

>> -     for (cmd = commands; cmd; cmd = cmd->next)
>> +     for (cmd = commands; cmd && !cmd->skip_update; cmd = cmd->next)
>>               cmd->error_string = update(cmd);
>>  }
>
> Do you really mean to have "skip-update" check as the loop termination
> condition like this (i.e. "upon seeing any skip-update, abandon the rest
> of the update queue"), or is this a typo of "skip this entry but keep
> going, processing the rest" that should have been a separate "if" in the
> body of the loop?

Oh wow, the perils of coding before coffee. I wish I could call it a typo, it's a thinko which is much worse.

Show 11 quoted lines

>> @@ -545,6 +606,7 @@ static struct command *read_head_info(void)
>>               hashcpy(cmd->old_sha1, old_sha1);
>>               hashcpy(cmd->new_sha1, new_sha1);
>>               memcpy(cmd->ref_name, line + 82, len - 81);
>> +             cmd->skip_update = 0;
>>               cmd->error_string = NULL;
>>               cmd->next = NULL;
>
> It would make sense to do xcalloc(nmemb, size) of one member of that
> length to allocate cmd at this point, instead of adding yet another
> assignment like this.

Okay.

Show 5 quoted lines

> It also would help me a slight bit if you compared what has been queued
> with what you sent to catch minor differences between my expectation from
> this series and what you have (e.g. I'd like to keep this as a fix that is
> back-mergeable to 'maint' and also I have already done some style fixes to
> the test).

I didn't realize you'd queued it before I sent out v2. But I'm not sure what you're asking for beyond what I already said in the email:

- Reformatted commit message; minor rewording.
- Detect situation where there is an inconsistent aliased update and
 give a better diagnostic than "failed to lock"
- Add additional test case for inconsistent update situation

j.