Re: [PATCH] git push --track

[Rudolf Polzer <divVerent@alientrap.org>]

On Thu, Jan 14, 2010 at 01:21:17PM +0800, Tay Ray Chuan wrote:
> Hi,
> 
> generally, it would be better if you could add some tests for this.
> 
> If I'm not wrong, the place to put it would be t5516-fetch-push.sh.

Can add that, but it seems like it won't go in anyway from the discussion here,
so it's probably not worth working on it. Sad.

> On Wed, Jan 13, 2010 at 11:55 PM, Rudolf Polzer <divVerent@alientrap.org> wrote:
> > On Wed, 13 Jan 2010 16:43:10 +0100, Ilari Liusvaara
> > <ilari.liusvaara@elisanet.fi> wrote:
> 
> please don't drop people from the Cc list - especially when you're
> replying to somebody!

I did not drop anyone, but simply replied from my newsreader. I really don't
want to subscribe to a mailing list and then get hundreds of emails a day.

> > From 123598516c7d4e1f83591e8dae64e2c76dc87c90 Mon Sep 17 00:00:00 2001
> > From: Rudolf Polzer <divVerent@alientrap.org>
> > Date: Wed, 13 Jan 2010 16:42:04 +0100
> > Subject: [PATCH 1/2] Add a feature "git push --track" to automatically make
> > the pushed branches tracking
> 
> Each patch should be sent out in its own mail. (As Matthieu has
> recommended, you should check out Documentation/SubmittingPatches.)

So, using a newsreader is not accepted practice? Why is the mailing list on a
newsgroup then?

> >  static const char * const push_usage[] = {
> > @@ -115,6 +116,36 @@ static int push_with_options(struct transport
> > *transport, int flags)
> >                fprintf(stderr, "Pushing to %s\n", transport->url);
> >        err = transport_push(transport, refspec_nr, refspec, flags,
> >                             &nonfastforward);
> > +       if (err == 0 && flags & TRANSPORT_PUSH_TRACK) {
> > +               struct ref *remote_refs =
> > +                       transport->get_refs_list(transport, 1);
> > +               struct ref *local_refs = get_local_heads();
> > +               int match_flags = 0;
> > +               if (flags & TRANSPORT_PUSH_ALL)
> > +                       match_flags |= MATCH_REFS_ALL;
> > +               if (flags & TRANSPORT_PUSH_MIRROR)
> > +                       match_flags |= MATCH_REFS_MIRROR;
> > +               if(!(flags & TRANSPORT_PUSH_DRY_RUN))
> > +               if(!match_refs(local_refs, &remote_refs, refspec_nr,
> > refspec,
> > +                                       match_flags)) {
> 
> It would be better if you can move this to
> transport.c::transport_push(). It repeats what's already there, so you
> don't have to configure match_flags, nor call match_refs, etc.

Then I have to duplicate it in the rsync specific push code too. Otherwise,
agreed.

> > +                       struct ref *next = remote_refs;
> > +                       while(next) {
> > [snip]
> > +                               next = next->next;
> 
> In most places, this is done like this:
> 
>   struct ref* ref;
>   for (ref = remote_refs; ref; ref = ref->next) {
>     ...
>   }

Sure, could do that too, I got this loop from the loop that frees a ref list.

Best regards,

Rudolf