Re: [PATCH v3] Add push --set-upstream
- From
Tay Ray Chuan <rctay89@gmail.com>
- Date
- Jan 16, 2010, 18:43 UTC
- Message-ID
- <be6fef0d1001161043q2ace931fyb17c2b2b03fe2cf6@mail.gmail.com>
- In-Reply-To
- <20100116134656.GA4504@Knoppix>
Hi,
yet again, this patch has gone through another iteration, so I'm adding people from the 'git push --track' thread here.
--
Cheers,
Ray Chuan
On Sat, Jan 16, 2010 at 9:46 PM, Ilari Liusvaara
<ilari.liusvaara@elisanet.fi> wrote:
> On Sat, Jan 16, 2010 at 08:35:57PM +0800, Tay Ray Chuan wrote:
>> Hi,
>>
>> On Sat, 16 Jan 2010 11:23:47 +0200
>> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
>> > + OPT_BIT('u', "set-upstream", &flags, "Set upstream for git pull", TRANSPORT_PUSH_SET_UPSTREAM),
>>
>> This line exceeds 80 characters.
>
> Broken into two (without breaking the message), it is still over 80.
>
>> Also, I think you should follow the case-style of the other option
>> descriptions and s/Set(?= upstream)/set/.
>
> Fixed.
>
>> > [snip]
>> > +static void set_upstreams(struct transport *trans, struct ref *refs,
>>
>> I would prefer if you could follow the naming convention and say
>> "transport" instead of truncating to "trans".
>
> Fixed.
>
>> > + struct ref *i;
>> > + for (i = refs; i; i = i->next) {
>>
>> In most places, "ref" instead of "i" is used; ie.
>
> Fixed.
>
>> > + * alreay up-to-date ref create/modify (not delete).
>>
>> s/alreay/already/.
>
> Fixed.
>
>> > + /* Chase symbolic refs (mainly for HEAD). */
>>
>> Did you mean "Change" here?
>
> No. Changed to 'Follow'.
>
>> Regarding the checking of ref->status here:
>>
>> Is it possible to delegate this to push_had_errors(remote_refs)
>> instead? We skip setting up upstream tracking when there are errors
>> from pushing, so we don't have to check ref->status anymore.
>
> No. As documetnation says, the update or no update is done on per-branch
> basis.
>
> <snip patch>
>
>> (As a side note, if you apply this on top of 'tr/http-push-ref-status'
>> in 'pu', the result of push_had_errors() is saved in a variable
>> ('err'), so you could just use it and not have to call push_had_errors
>> () again.)
>
> See above.
>
> I'll sit on v4 for some more time to wait for more comments.
>
> -Ilari
>