Re: [PATCH] push: allow --follow-tags to be set by config push.followTags

From: Jeff King <peff@peff.net>
Date: Feb 16, 2015, 05:20 UTC
Message-ID: <20150216052049.GA5031@peff.net>
In-Reply-To: <1424055690-32631-1-git-send-email-cxreg@pobox.com>

On Sun, Feb 15, 2015 at 07:01:30PM -0800, Dave Olszewski wrote:

> +push.followTags::
> +	If set to true enable '--follow-tags' option by default.  You
> +	may override this configuration at time of push by specifying
> +	'--no-follow-tags'.

Thanks, this is something I've considered implementing myself, as I have one repo that is frequently migrating tags from one remote to another, and I often forget to specify the option.

Show 12 quoted lines

> diff --git a/builtin/push.c b/builtin/push.c
> index fc771a9..47f0119 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -525,6 +525,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>  
>  	packet_trace_identity("push");
>  	git_config(git_push_config, NULL);
> +
> +	/* set TRANSPORT_PUSH_FOLLOW_TAGS in flags so that --no-follow-tags may unset it */
> +	if (push_follow_tags)
> +		flags |= TRANSPORT_PUSH_FOLLOW_TAGS;

You can see above that we use git_push_config to load our config...

Show 10 quoted lines

> --- a/config.c
> +++ b/config.c
> @@ -977,6 +977,11 @@ static int git_default_push_config(const char *var, const char *value)
>  		return 0;
>  	}
>  
> +	if (!strcmp(var, "push.followtags")) {
> +		push_follow_tags = git_config_bool(var, value);
> +		return 0;
> +	}

But here you are adding to git_default_push_config, which is in another file.

I'm trying to figure out why git_default_push_config exists at all. The major difference from git_push_config is that the "default" variant will get loaded for _all_ commands, not just "push". So if it affected variables that were used by other commands, it would be needed. But all it sets is push_default, which seems to be specific to builtin/push.c.

So I suspect it can be removed entirely, and folded into git_config_push. But that's outside the scope of your patch.

What _is_ in the scope of your patch is that I think the new option you are adding could go into git_push_config; it is definitely only about the push command itself. And then you could declare it as:

  static int push_follow_tags;

without having to worry about making it an extern that is available everywhere.

Show 11 quoted lines

> diff --git a/transport.c b/transport.c
> index 0694a7c..ff5f63d 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1148,7 +1148,7 @@ int transport_push(struct transport *transport,
>  			match_flags |= MATCH_REFS_MIRROR;
>  		if (flags & TRANSPORT_PUSH_PRUNE)
>  			match_flags |= MATCH_REFS_PRUNE;
> -		if (flags & TRANSPORT_PUSH_FOLLOW_TAGS)
> +		if ((flags & TRANSPORT_PUSH_FOLLOW_TAGS))
>  			match_flags |= MATCH_REFS_FOLLOW_TAGS;

This looks like just noise in the diff (I guess leftover from some debugging you were doing). Is that correct?

-Peff