Re: [PATCH 6/6] push: honor branch.*.push

From: Johan Herland <johan@herland.net>
Date: Jun 24, 2013, 06:58 UTC
Message-ID: <CALKQrgebusbcDOTR+bFccoT4qzDJ8E=2LULutL0egEN9YY1v_g@mail.gmail.com>
In-Reply-To: <1372048388-16742-7-git-send-email-gitster@pobox.com>

On Mon, Jun 24, 2013 at 6:33 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 27 quoted lines

> When branch.*.push configuration variable is defined for the current
> branch, a lazy-typing "git push" (and "git push there") will push
> the commit at the tip of the current branch to the destination and
> update the branch named by that variable.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/push.c | 18 +++++++++++++++++-
>  remote.c       |  5 +++++
>  remote.h       |  2 ++
>  3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index f6c8047..a140b8e 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -185,6 +185,15 @@ static void warn_unspecified_push_default_configuration(void)
>         warning("%s\n", _(warn_unspecified_push_default_msg));
>  }
>
> +static void setup_per_branch_push(struct branch *branch)
> +{
> +       struct strbuf refspec = STRBUF_INIT;
> +
> +       strbuf_addf(&refspec, "%s:%s",
> +                   branch->name, branch->push_name);
> +       add_refspec(refspec.buf);

This goes back to the question I raised in 3/6: If this code path adds refspec "foo:bar", and - say - setup_push_current() has already added refspec "foo:foo" (or simply "foo"), then do we end up pushing into "foo" or "bar"? To me, "branch.*.push" feels more specific than "push.default = current", so it would make sense that "foo:bar" overrides "foo:foo", but that is not obvious from the refspec alone. IMHO, this definitely needs some tests.

Show 15 quoted lines

> +}
> +
>  static int is_workflow_triagular(struct remote *remote)
>  {
>         struct remote *fetch_remote = remote_get(NULL);
> @@ -194,7 +203,14 @@ static int is_workflow_triagular(struct remote *remote)
>  static void setup_default_push_refspecs(struct remote *remote)
>  {
>         struct branch *branch = branch_get(NULL);
> -       int triangular = is_workflow_triagular(remote);
> +       int triangular;
> +
> +       if (branch->push_name) {
> +               setup_per_branch_push(branch);
> +               return;

I guess this return ensures that branch.*.push overrides push.default, but might there be other sources of add_refspec() that would complicate things?

Show 35 quoted lines

> +       }
> +
> +       triangular = is_workflow_triagular(remote);
>
>         switch (push_default) {
>         default:
> diff --git a/remote.c b/remote.c
> index e71f66d..e033fef 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -372,6 +372,11 @@ static int handle_config(const char *key, const char *value, void *cb)
>                         if (!value)
>                                 return config_error_nonbool(key);
>                         add_merge(branch, xstrdup(value));
> +               } else if (!strcmp(subkey, ".push")) {
> +                       if (!value)
> +                               return config_error_nonbool(key);
> +                       free(branch->push_name);
> +                       branch->push_name = xstrdup(value);
>                 }
>                 return 0;
>         }
> diff --git a/remote.h b/remote.h
> index cf56724..84e0f72 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -138,6 +138,8 @@ struct branch {
>         struct refspec **merge;
>         int merge_nr;
>         int merge_alloc;
> +
> +       char *push_name;
>  };
>
>  struct branch *branch_get(const char *name);

Otherwise, this patch, and the entire series looks good to me.

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net