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

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

[Dave Olszewski]

Signed-off-by: Dave Olszewski <cxreg@pobox.com>
---
 Documentation/config.txt               | 6 ++++++
 Documentation/git-push.txt             | 5 ++++-
 builtin/push.c                         | 5 +++++
 cache.h                                | 1 +
 config.c                               | 5 +++++
 contrib/completion/git-completion.bash | 1 +
 environment.c                          | 1 +
 transport.c                            | 2 +-
 8 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ae6791d..e01d21c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2079,6 +2079,12 @@ new default).
 
 --
 
+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'.
+
+
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
 	rebase. False by default.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index ea97576..caa187b 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -128,7 +128,10 @@ already exists on the remote side.
 	Push all the refs that would be pushed without this option,
 	and also push annotated tags in `refs/tags` that are missing
 	from the remote but are pointing at commit-ish that are
-	reachable from the refs being pushed.
+	reachable from the refs being pushed.  This can also be specified
+	with configuration variable 'push.followTags'.  For more
+	information, see 'push.followTags' in linkgit:git-config[1].
+
 
 --signed::
 	GPG-sign the push request to update refs on the receiving
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;
+
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
 	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
diff --git a/cache.h b/cache.h
index f704af5..9318189 100644
--- a/cache.h
+++ b/cache.h
@@ -648,6 +648,7 @@ enum push_default_type {
 extern enum branch_track git_branch_track;
 extern enum rebase_setup_type autorebase;
 extern enum push_default_type push_default;
+extern int push_follow_tags;
 
 enum object_creation_mode {
 	OBJECT_CREATION_USES_HARDLINKS = 0,
diff --git a/config.c b/config.c
index e5e64dc..cb237cd 100644
--- 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;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c21190d..cffb2b8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2188,6 +2188,7 @@ _git_config ()
 		pull.octopus
 		pull.twohead
 		push.default
+		push.followTags
 		rebase.autosquash
 		rebase.stat
 		receive.autogc
diff --git a/environment.c b/environment.c
index 1ade5c9..aef9587 100644
--- a/environment.c
+++ b/environment.c
@@ -52,6 +52,7 @@ unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
 enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
+int push_follow_tags = 0;
 #ifndef OBJECT_CREATION_MODE
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
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;
 
 		if (match_push_refs(local_refs, &remote_refs,
-- 
2.1.4

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

[Jeff King]

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.

> 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...

> --- 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.

> 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

[PATCH 0/2] clean up push config callbacks

[Jeff King]

On Mon, Feb 16, 2015 at 12:20:49AM -0500, Jeff King wrote:

> 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.

Here's that cleanup, plus another one I noticed while doing it.

  [1/2]: git_push_config: drop cargo-culted wt_status pointer
  [2/2]: builtin/push.c: make push_default a static variable

-Peff

[PATCH 1/2] git_push_config: drop cargo-culted wt_status pointer

[Jeff King]

The push config callback does not expect any incoming data
via the void pointer. And if it did, it would certainly not
be a "struct wt_status". This probably got picked up
accidentally in b945901 (push: heed user.signingkey for
signed pushes, 2014-10-22), which copied the template for
the config callback from builtin/commit.c.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/push.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index fc771a9..aa9334c 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -473,13 +473,12 @@ static int option_parse_recurse_submodules(const struct option *opt,
 
 static int git_push_config(const char *k, const char *v, void *cb)
 {
-	struct wt_status *s = cb;
 	int status;
 
 	status = git_gpg_config(k, v, NULL);
 	if (status)
 		return status;
-	return git_default_config(k, v, s);
+	return git_default_config(k, v, NULL);
 }
 
 int cmd_push(int argc, const char **argv, const char *prefix)
-- 
2.3.0.rc1.287.g761fd19

[PATCH 2/2] builtin/push.c: make push_default a static variable

[Jeff King]

When the "push_default" flag was originally added, it was
made globally visible to all code. This might have been
useful if other commands or library calls ended up depending
on it, but as it turns out, only builtin/push.c cares.

Let's make it a static variable in builtin/push.c. Since it
is no longer globally visible, it only needs to be set
inside that function. That means we can drop the
git_push_default_config function (which is called from
git_default_config for all commands) and just set it as part
of git_push_config.

That in turn makes it easier for people adding new config to
git-push to know which callback function to add to (since
there is only one now).

Signed-off-by: Jeff King <peff@peff.net>
---
We know this is safe because no other callers needed tweaked when the
variable went out of scope. :) It would only be a bad idea if we
were planning on having other code in the future depend on push_default
(e.g., the code in remote.c to find the push destination). But it does
not seem to have needed that in the intervening years, so it's probably
fine to do this cleanup now.

 builtin/push.c | 33 +++++++++++++++++++++++++++++++++
 cache.h        | 10 ----------
 config.c       | 32 --------------------------------
 environment.c  |  1 -
 4 files changed, 33 insertions(+), 43 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index aa9334c..ab99f4c 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -23,6 +23,15 @@ static int progress = -1;
 
 static struct push_cas_option cas;
 
+static enum push_default_type {
+	PUSH_DEFAULT_NOTHING = 0,
+	PUSH_DEFAULT_MATCHING,
+	PUSH_DEFAULT_SIMPLE,
+	PUSH_DEFAULT_UPSTREAM,
+	PUSH_DEFAULT_CURRENT,
+	PUSH_DEFAULT_UNSPECIFIED
+} push_default = PUSH_DEFAULT_UNSPECIFIED;
+
 static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
@@ -478,6 +487,30 @@ static int git_push_config(const char *k, const char *v, void *cb)
 	status = git_gpg_config(k, v, NULL);
 	if (status)
 		return status;
+
+	if (!strcmp(k, "push.default")) {
+		if (!v)
+			return config_error_nonbool(k);
+		else if (!strcmp(v, "nothing"))
+			push_default = PUSH_DEFAULT_NOTHING;
+		else if (!strcmp(v, "matching"))
+			push_default = PUSH_DEFAULT_MATCHING;
+		else if (!strcmp(v, "simple"))
+			push_default = PUSH_DEFAULT_SIMPLE;
+		else if (!strcmp(v, "upstream"))
+			push_default = PUSH_DEFAULT_UPSTREAM;
+		else if (!strcmp(v, "tracking")) /* deprecated */
+			push_default = PUSH_DEFAULT_UPSTREAM;
+		else if (!strcmp(v, "current"))
+			push_default = PUSH_DEFAULT_CURRENT;
+		else {
+			error("Malformed value for %s: %s", k, v);
+			return error("Must be one of nothing, matching, simple, "
+				     "upstream or current.");
+		}
+		return 0;
+	}
+
 	return git_default_config(k, v, NULL);
 }
 
diff --git a/cache.h b/cache.h
index f704af5..5394546 100644
--- a/cache.h
+++ b/cache.h
@@ -636,18 +636,8 @@ enum rebase_setup_type {
 	AUTOREBASE_ALWAYS
 };
 
-enum push_default_type {
-	PUSH_DEFAULT_NOTHING = 0,
-	PUSH_DEFAULT_MATCHING,
-	PUSH_DEFAULT_SIMPLE,
-	PUSH_DEFAULT_UPSTREAM,
-	PUSH_DEFAULT_CURRENT,
-	PUSH_DEFAULT_UNSPECIFIED
-};
-
 extern enum branch_track git_branch_track;
 extern enum rebase_setup_type autorebase;
-extern enum push_default_type push_default;
 
 enum object_creation_mode {
 	OBJECT_CREATION_USES_HARDLINKS = 0,
diff --git a/config.c b/config.c
index e5e64dc..5782442 100644
--- a/config.c
+++ b/config.c
@@ -952,35 +952,6 @@ static int git_default_branch_config(const char *var, const char *value)
 	return 0;
 }
 
-static int git_default_push_config(const char *var, const char *value)
-{
-	if (!strcmp(var, "push.default")) {
-		if (!value)
-			return config_error_nonbool(var);
-		else if (!strcmp(value, "nothing"))
-			push_default = PUSH_DEFAULT_NOTHING;
-		else if (!strcmp(value, "matching"))
-			push_default = PUSH_DEFAULT_MATCHING;
-		else if (!strcmp(value, "simple"))
-			push_default = PUSH_DEFAULT_SIMPLE;
-		else if (!strcmp(value, "upstream"))
-			push_default = PUSH_DEFAULT_UPSTREAM;
-		else if (!strcmp(value, "tracking")) /* deprecated */
-			push_default = PUSH_DEFAULT_UPSTREAM;
-		else if (!strcmp(value, "current"))
-			push_default = PUSH_DEFAULT_CURRENT;
-		else {
-			error("Malformed value for %s: %s", var, value);
-			return error("Must be one of nothing, matching, simple, "
-				     "upstream or current.");
-		}
-		return 0;
-	}
-
-	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
-}
-
 static int git_default_mailmap_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "mailmap.file"))
@@ -1006,9 +977,6 @@ int git_default_config(const char *var, const char *value, void *dummy)
 	if (starts_with(var, "branch."))
 		return git_default_branch_config(var, value);
 
-	if (starts_with(var, "push."))
-		return git_default_push_config(var, value);
-
 	if (starts_with(var, "mailmap."))
 		return git_default_mailmap_config(var, value);
 
diff --git a/environment.c b/environment.c
index 1ade5c9..bea09b6 100644
--- a/environment.c
+++ b/environment.c
@@ -51,7 +51,6 @@ enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
-enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
 #ifndef OBJECT_CREATION_MODE
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
-- 
2.3.0.rc1.287.g761fd19

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

[Jeff King]

On Mon, Feb 16, 2015 at 12:45:50AM -0500, Jeff King wrote:

> On Mon, Feb 16, 2015 at 12:20:49AM -0500, Jeff King wrote:
> 
> > 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.
> 
> Here's that cleanup, plus another one I noticed while doing it.
> 
>   [1/2]: git_push_config: drop cargo-culted wt_status pointer
>   [2/2]: builtin/push.c: make push_default a static variable

And here's what your patch would look like rebased on top. Two nits,
though. One, it could probably use a few basic tests.

And two, the way that the config and --follow-tags interact is a little
non-obvious (as evidenced by the fact that you needed a comment to
explain what was going on).

One way to do it would be similar to how "atomic" is implemented: use
OPT_BOOL to set an int, and then pick up the final value of that int
after config and command-line parsing is done. Then a reader does not
have to wonder why the "follow_tags" variable is not set by
"--follow-tags".

Or alternatively, we could pull the "flags" field from cmd_push out into
a static global "transport_flags", and manipulate it directly from the
config (or if we don't like a global, pass it via the config-callback
void pointer; but certainly a global is more common in git for code like
this). Then we do not have to worry about propagating values from
integers into flag bits at all.

-- >8 --
From: Dave Olszewski <cxreg@pobox.com>
Subject: push: allow --follow-tags to be set by config push.followTags

Signed-off-by: Dave Olszewski <cxreg@pobox.com>
---
 Documentation/config.txt               |  6 ++++++
 Documentation/git-push.txt             |  5 ++++-
 builtin/push.c                         | 11 +++++++++++
 contrib/completion/git-completion.bash |  1 +
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ae6791d..e01d21c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2079,6 +2079,12 @@ new default).
 
 --
 
+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'.
+
+
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
 	rebase. False by default.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index ea97576..caa187b 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -128,7 +128,10 @@ already exists on the remote side.
 	Push all the refs that would be pushed without this option,
 	and also push annotated tags in `refs/tags` that are missing
 	from the remote but are pointing at commit-ish that are
-	reachable from the refs being pushed.
+	reachable from the refs being pushed.  This can also be specified
+	with configuration variable 'push.followTags'.  For more
+	information, see 'push.followTags' in linkgit:git-config[1].
+
 
 --signed::
 	GPG-sign the push request to update refs on the receiving
diff --git a/builtin/push.c b/builtin/push.c
index ab99f4c..7ddf4dd 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -20,6 +20,7 @@ static int deleterefs;
 static const char *receivepack;
 static int verbosity;
 static int progress = -1;
+static int follow_tags;
 
 static struct push_cas_option cas;
 
@@ -511,6 +512,11 @@ static int git_push_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(k, "push.followtags")) {
+		follow_tags = git_config_bool(k, v);
+		return 0;
+	}
+
 	return git_default_config(k, v, NULL);
 }
 
@@ -557,6 +563,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 (follow_tags)
+		flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
+
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
 	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c21190d..cffb2b8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2188,6 +2188,7 @@ _git_config ()
 		pull.octopus
 		pull.twohead
 		push.default
+		push.followTags
 		rebase.autosquash
 		rebase.stat
 		receive.autogc
-- 
2.3.0.rc1.287.g761fd19

Re: [PATCH 2/2] builtin/push.c: make push_default a static variable

[Junio C Hamano]

On Sun, Feb 15, 2015 at 9:47 PM, Jeff King <peff@peff.net> wrote:
> When the "push_default" flag was originally added, it was
> made globally visible to all code. This might have been
> useful if other commands or library calls ended up depending
> on it, but as it turns out, only builtin/push.c cares.
> ...
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> We know this is safe because no other callers needed tweaked when the
> variable went out of scope. :) It would only be a bad idea if we
> were planning on having other code in the future depend on push_default
> (e.g., the code in remote.c to find the push destination). But it does
> not seem to have needed that in the intervening years, so it's probably
> fine to do this cleanup now.

Yay. Great minds think alike ;-)

"It definitely smells wrong to touch environment.c and cache.h" was my
first reaction to the "follow-tags config" patch, and I really think this shows
the right way forward.

Thanks.

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

[Junio C Hamano]

On Sun, Feb 15, 2015 at 9:54 PM, Jeff King <peff@peff.net> wrote:
>
> Or alternatively, we could pull the "flags" field from cmd_push out into
> a static global "transport_flags", and manipulate it directly from the
> config (or if we don't like a global, pass it via the config-callback
> void pointer; but certainly a global is more common in git for code like
> this). Then we do not have to worry about propagating values from
> integers into flag bits at all.

Yup, that would be my preference. The largest problem I had with the
original change was how to ensure that future new code would not
mistakenly set the global follow_tags _without_ letting the command
line option parser to override it. If the config parser flips the bit in the
same flags, it would become much less likely for future code to make
such a mistake.

Thanks.

[PATCH 0/3] cleaner bit-setting in cmd_push

[Jeff King]

On Sun, Feb 15, 2015 at 10:02:08PM -0800, Junio C Hamano wrote:

> On Sun, Feb 15, 2015 at 9:54 PM, Jeff King <peff@peff.net> wrote:
> >
> > Or alternatively, we could pull the "flags" field from cmd_push out into
> > a static global "transport_flags", and manipulate it directly from the
> > config (or if we don't like a global, pass it via the config-callback
> > void pointer; but certainly a global is more common in git for code like
> > this). Then we do not have to worry about propagating values from
> > integers into flag bits at all.
> 
> Yup, that would be my preference. The largest problem I had with the
> original change was how to ensure that future new code would not
> mistakenly set the global follow_tags _without_ letting the command
> line option parser to override it. If the config parser flips the bit in the
> same flags, it would become much less likely for future code to make
> such a mistake.

So here's my take on it (on top of the two-patch series I just sent).
Dave's patch is 3rd here, just to show its rebased form, but do not take
that as a final endorsement. I still think it could use tests, but I
will let him write them. I am just doing the cleanup in the area, none
of which needs to be his problem. :)

  [1/3]: cmd_push: set "atomic" bit directly
  [2/3]: cmd_push: pass "flags" pointer to config callback
  [3/3]: push: allow --follow-tags to be set by config push.followTags

-Peff

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

[Junio C Hamano]

On Sun, Feb 15, 2015 at 10:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> On Sun, Feb 15, 2015 at 9:54 PM, Jeff King <peff@peff.net> wrote:
>>
>> Or alternatively, we could pull the "flags" field from cmd_push out into
>> a static global "transport_flags", and manipulate it directly from the
>> config (or if we don't like a global, pass it via the config-callback
>> void pointer; but certainly a global is more common in git for code like
>> this). Then we do not have to worry about propagating values from
>> integers into flag bits at all.
>
> Yup, that would be my preference. The largest problem I had with the
> original change was how to ensure that future new code would not
> mistakenly set the global follow_tags _without_ letting the command
> line option parser to override it. If the config parser flips the bit in the
> same flags, it would become much less likely for future code to make
> such a mistake.

Having said that, I think this version is good enough.

Unlike a global in environment.c (that is named not-so-specifically that
anybody can set by reading the configuration file) that can be overriden
only by command line parser used only for "git push", the global int and
the flags are both localized to "git push" in this version, and there is
much less chance to introduce new buggy code that forgets the command
line override.

Thanks, again.

[PATCH 1/3] cmd_push: set "atomic" bit directly

[Jeff King]

This makes the code shorter and more obvious by removing an
unnecessary interim variable.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/push.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index ab99f4c..f558c2e 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -519,7 +519,6 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	int flags = 0;
 	int tags = 0;
 	int rc;
-	int atomic = 0;
 	const char *repo = NULL;	/* default repository */
 	struct option options[] = {
 		OPT__VERBOSITY(&verbosity),
@@ -551,7 +550,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "follow-tags", &flags, N_("push missing but relevant tags"),
 			TRANSPORT_PUSH_FOLLOW_TAGS),
 		OPT_BIT(0, "signed", &flags, N_("GPG sign the push"), TRANSPORT_PUSH_CERT),
-		OPT_BOOL(0, "atomic", &atomic, N_("request atomic transaction on remote side")),
+		OPT_BIT(0, "atomic", &flags, N_("request atomic transaction on remote side"), TRANSPORT_PUSH_ATOMIC),
 		OPT_END()
 	};
 
@@ -567,9 +566,6 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	if (tags)
 		add_refspec("refs/tags/*");
 
-	if (atomic)
-		flags |= TRANSPORT_PUSH_ATOMIC;
-
 	if (argc > 0) {
 		repo = argv[0];
 		set_refspecs(argv + 1, argc - 1, repo);
-- 
2.3.0.rc1.287.g761fd19

[PATCH 2/3] cmd_push: pass "flags" pointer to config callback

[Jeff King]

This will let us manipulate any transport flags which have matching
config options (there are none yet, but we will add one in
the next patch).

We could also just make "flags" a static file-scope global,
but the result is a little confusing. We end up passing it
along through do_push and push_with_options, each of which
further munge it. Having slightly-differing versions of the
flags variable available to those functions would probably
cause more confusion than it is worth. Let's just keep the
original local to cmd_push, and it can continue to pass it
through the call-stack.

Signed-off-by: Jeff King <peff@peff.net>
---
I was also tempted to just remove the passing of "flags" through the
call stack entirely, and just have everybody touch a global
transport_flags. That is a much bigger change, and less obviously
correct (after a callee munges their local version, do we ever care
about seeing the original in the caller?). I don't think so.

To be honest, the whole do_push is confusing to me. It seems like that
should just be part of cmd_push.

 builtin/push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index f558c2e..c25108f 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -555,7 +555,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	};
 
 	packet_trace_identity("push");
-	git_config(git_push_config, NULL);
+	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
 	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
-- 
2.3.0.rc1.287.g761fd19

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

[Jeff King]

From: Dave Olszewski <cxreg@pobox.com>

Signed-off-by: Dave Olszewski <cxreg@pobox.com>
---
Again, this is just a preview. Dave should send the final when he thinks
it is good.

The if/else I added to the config callback is kind of ugly. I wonder if
we should have git_config_bit, or even just a function to set/clear a
bit. Then the OPT_BIT code could use it, too. Something like:

  munge_bit(flags, TRANSPORT_PUSH_FOLLOW_TAGS, git_config_bool(k, v));

Or maybe that is getting too fancy and obfuscated for a simple bit
set/clear. I dunno.

 Documentation/config.txt               | 6 ++++++
 Documentation/git-push.txt             | 5 ++++-
 builtin/push.c                         | 9 +++++++++
 contrib/completion/git-completion.bash | 1 +
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ae6791d..e01d21c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2079,6 +2079,12 @@ new default).
 
 --
 
+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'.
+
+
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
 	rebase. False by default.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index ea97576..caa187b 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -128,7 +128,10 @@ already exists on the remote side.
 	Push all the refs that would be pushed without this option,
 	and also push annotated tags in `refs/tags` that are missing
 	from the remote but are pointing at commit-ish that are
-	reachable from the refs being pushed.
+	reachable from the refs being pushed.  This can also be specified
+	with configuration variable 'push.followTags'.  For more
+	information, see 'push.followTags' in linkgit:git-config[1].
+
 
 --signed::
 	GPG-sign the push request to update refs on the receiving
diff --git a/builtin/push.c b/builtin/push.c
index c25108f..6831c2d 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -482,6 +482,7 @@ static int option_parse_recurse_submodules(const struct option *opt,
 
 static int git_push_config(const char *k, const char *v, void *cb)
 {
+	int *flags = cb;
 	int status;
 
 	status = git_gpg_config(k, v, NULL);
@@ -511,6 +512,14 @@ static int git_push_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(k, "push.followtags")) {
+		if (git_config_bool(k, v))
+			*flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
+		else
+			*flags &= ~TRANSPORT_PUSH_FOLLOW_TAGS;
+		return 0;
+	}
+
 	return git_default_config(k, v, NULL);
 }
 
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c21190d..cffb2b8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2188,6 +2188,7 @@ _git_config ()
 		pull.octopus
 		pull.twohead
 		push.default
+		push.followTags
 		rebase.autosquash
 		rebase.stat
 		receive.autogc
-- 
2.3.0.rc1.287.g761fd19

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

[Jeff King]

On Sun, Feb 15, 2015 at 10:11:21PM -0800, Junio C Hamano wrote:

> On Sun, Feb 15, 2015 at 10:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > On Sun, Feb 15, 2015 at 9:54 PM, Jeff King <peff@peff.net> wrote:
> >>
> >> Or alternatively, we could pull the "flags" field from cmd_push out into
> >> a static global "transport_flags", and manipulate it directly from the
> >> config (or if we don't like a global, pass it via the config-callback
> >> void pointer; but certainly a global is more common in git for code like
> >> this). Then we do not have to worry about propagating values from
> >> integers into flag bits at all.
> >
> > Yup, that would be my preference. The largest problem I had with the
> > original change was how to ensure that future new code would not
> > mistakenly set the global follow_tags _without_ letting the command
> > line option parser to override it. If the config parser flips the bit in the
> > same flags, it would become much less likely for future code to make
> > such a mistake.
> 
> Having said that, I think this version is good enough.

Too late. :)

I am OK if we leave it here, though, and drop the extra two patches I
just sent.

-Peff

Re: [PATCH 2/3] cmd_push: pass "flags" pointer to config callback

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This will let us manipulate any transport flags which have matching
> config options (there are none yet, but we will add one in
> the next patch).

Nice---this will later lets us do push.atomic if we really wanted
to, right?

> To be honest, the whole do_push is confusing to me. It seems like that
> should just be part of cmd_push.

Yeah, that part of the push callchain always confuses me every time
I look at it.  I think it was a consequence of how transport layer
was wedged into the existing codepath that only handled push that
called send-pack to unify the codepaths that push calls into
different transport backends, and we may have done it differently
and more cleanly if we were designing the push to transport to
backends from scratch.

>  builtin/push.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index f558c2e..c25108f 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -555,7 +555,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>  	};
>  
>  	packet_trace_identity("push");
> -	git_config(git_push_config, NULL);
> +	git_config(git_push_config, &flags);
>  	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
>  
>  	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))

Re: [PATCH 2/3] cmd_push: pass "flags" pointer to config callback

[Jeff King]

On Sun, Feb 15, 2015 at 11:05:57PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This will let us manipulate any transport flags which have matching
> > config options (there are none yet, but we will add one in
> > the next patch).
> 
> Nice---this will later lets us do push.atomic if we really wanted
> to, right?

Yes, exactly. Or push.signed, or whatever.

> > To be honest, the whole do_push is confusing to me. It seems like that
> > should just be part of cmd_push.
> 
> Yeah, that part of the push callchain always confuses me every time
> I look at it.  I think it was a consequence of how transport layer
> was wedged into the existing codepath that only handled push that
> called send-pack to unify the codepaths that push calls into
> different transport backends, and we may have done it differently
> and more cleanly if we were designing the push to transport to
> backends from scratch.

I took a very cursory look at folding do_push into cmd_push. It's not
_too_ bad. You wouldn't want to fold push_with_options in, as that gets
called from a loop (you could make it the loop body, but I think it is
more clear as-is).

However, it is really do_push which continues to manipulate the flags
and set up the push, so that is the bit that should be folded in. And
then it would be fine to make transport_flags a global, and
push_with_options could just use it directly, I think.

-Peff

Re: [PATCH 2/2] builtin/push.c: make push_default a static variable

[Jeff King]

On Mon, Feb 16, 2015 at 12:47:54AM -0500, Jeff King wrote:

> When the "push_default" flag was originally added, it was
> made globally visible to all code. This might have been
> useful if other commands or library calls ended up depending
> on it, but as it turns out, only builtin/push.c cares.
> 
> Let's make it a static variable in builtin/push.c.
>
> [...]
> 
> ---
> We know this is safe because no other callers needed tweaked when the
> variable went out of scope. :) It would only be a bad idea if we
> were planning on having other code in the future depend on push_default
> (e.g., the code in remote.c to find the push destination). But it does
> not seem to have needed that in the intervening years, so it's probably
> fine to do this cleanup now.

I had a nagging feeling that there was some code which wanted to use
this elsewhere, and I did finally find it, when I merged this topic with
my other personal topics.

If we wanted to implement "@{push}" (or "@{publish}") to mean "the
tracking ref of the remote ref you would push to if you ran git-push",
then this is a step in the wrong direction.

The patches I posted last January (and which you carried as
jk/branch-at-publish for a while) do work, and I've used the feature
once or twice since then. From the discussion, it looks like they were
meant to be a building block for more triangular-flow work, but I don't
remember what else was needed. I'm tempted to resurrect them, but it's
not a high priority for me.

Anyway, food for thought on whether we want to do this cleanup or not,
then. We can always leave this here as part of git_default_config, and
still move Dave's new option into git_push_config.

-Peff

Re: [PATCH 2/2] builtin/push.c: make push_default a static variable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If we wanted to implement "@{push}" (or "@{publish}") to mean "the
> tracking ref of the remote ref you would push to if you ran git-push",
> then this is a step in the wrong direction.

Is that because push_default variable needs to be looked at from
sha1_name.c when resolving "@{push}", optionally prefixed with the
name of the branch?  I wonder if that codepath should know the gory
details of which ref at the remote the branch is pushed to and which
remote-tracking ref we use in the local repository to mirror that
remote ref in the first place?

What do we do for the @{upstream} side of the things---it calls
branch_get() and when the branch structure is returned, the details
have been computed for us so get_upstream_branch() only needs to use
the information already computed.  The interesting parts of the
computation all happen inside remote.c, it seems.

So we probably would do something similar to @{push} side, which
would mean that push_default variable and the logic needs to be
visible to remote.c if we want to have the helper that is similar to
set_merge() that is used from branch_get() to support @{upstream}.

Hmmm, I have a feeling that "with default configuration, where does
'git push' send this branch to?" logic should be contained within
the source file whose name has "push" in it and exposed as a helper
function, instead of exposing just one of the lowest level knob
push_default to outside callers and have them figure things out.

Viewed from that angle, it might be the case that remote.c knows too
much about what happens during fetch and pull, but I dunno.

Re: [PATCH 2/2] builtin/push.c: make push_default a static variable

[Jeff King]

On Tue, Feb 17, 2015 at 09:45:07AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > If we wanted to implement "@{push}" (or "@{publish}") to mean "the
> > tracking ref of the remote ref you would push to if you ran git-push",
> > then this is a step in the wrong direction.
> 
> Is that because push_default variable needs to be looked at from
> sha1_name.c when resolving "@{push}", optionally prefixed with the
> name of the branch?

Yes, exactly.

> I wonder if that codepath should know the gory details of which ref at
> the remote the branch is pushed to and which remote-tracking ref we
> use in the local repository to mirror that remote ref in the first
> place?

I think that was one of the ugly bits from the series; that we had to
reimplement "where would we push" and "what would it be called if we
pushed and then fetched"? The former cares about push_default, and the
latter has to apply push and then fetch refspecs.

If you want to peek at it again, it's at:

  https://github.com/peff/git/commit/8859afb1af63cb3cb0bc4cc8c1719c2011f406c9

(but note that it should not be called @{publish}, as per earlier
discussions).

> What do we do for the @{upstream} side of the things---it calls
> branch_get() and when the branch structure is returned, the details
> have been computed for us so get_upstream_branch() only needs to use
> the information already computed.  The interesting parts of the
> computation all happen inside remote.c, it seems.
> 
> So we probably would do something similar to @{push} side, which
> would mean that push_default variable and the logic needs to be
> visible to remote.c if we want to have the helper that is similar to
> set_merge() that is used from branch_get() to support @{upstream}.

Sure, we could go that way. But I don't think it changes the issue for
_this_ patch series, which is that the variable needs visibility outside
of builtin/push.c (and we need to load the config for programs besides
git-push).

> Hmmm, I have a feeling that "with default configuration, where does
> 'git push' send this branch to?" logic should be contained within
> the source file whose name has "push" in it and exposed as a helper
> function, instead of exposing just one of the lowest level knob
> push_default to outside callers and have them figure things out.
> 
> Viewed from that angle, it might be the case that remote.c knows too
> much about what happens during fetch and pull, but I dunno.

Yeah, it would be nice if there were a convenient lib-ified set of
functions for getting this information, and "fetch" and "push" commands
were built on top of it. I don't know how painful that would be, though.
The existing code has grown somewhat organically.

But even with that change, the lib-ified code needs to hook into
git_default_config (or do its own config lookup) so that we get the
proper value no matter who the caller is.

-Peff

Re: [PATCH 2/2] builtin/push.c: make push_default a static variable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> So we probably would do something similar to @{push} side, which
>> would mean that push_default variable and the logic needs to be
>> visible to remote.c if we want to have the helper that is similar to
>> set_merge() that is used from branch_get() to support @{upstream}.
>
> Sure, we could go that way. But I don't think it changes the issue for
> _this_ patch series, which is that the variable needs visibility outside
> of builtin/push.c (and we need to load the config for programs besides
> git-push).

I do not disagree.  push_default and other things that affect the
computation needs to be visible to the code that implements the
logic.

Do you want to resurrect that @{publish} stuff?  I think it had
sensible semantics, and I do not think we mind keeping the
push_default configuration to be read from the default_config
codepath.

If we decide to go that route, then the series would become
something like this:

$gmane/263871 [1/4] git_push_config: drop cargo-culted wt_status pointer
$gmane/263878 [2/4] cmd_push: set "atomic" bit directly
$gmane/263879 [3/4] cmd_push: pass "flags" pointer to config callback
$gmane/263880 [4/4] push: allow --follow-tags to be set by config push.followTags

omitting the original 2/2 patch we are discussing.  I am inclined to
replace what I queued with the above four.

The last one needs a bit of tweaking and should look like the
attached.  Again, as you wrote in $gmane/263880, this is just a
preview. Dave should send the final when he thinks it is good,
possibly with some tests.

-- >8 --
From: Dave Olszewski <cxreg@pobox.com>
Date: Mon, 16 Feb 2015 01:16:19 -0500
Subject: [PATCH] [NEEDSACK] push: allow --follow-tags to be set by config push.followTags

Signed-off-by: Dave Olszewski <cxreg@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt               |  6 ++++++
 Documentation/git-push.txt             |  5 ++++-
 builtin/push.c                         | 10 ++++++++++
 contrib/completion/git-completion.bash |  1 +
 4 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ae6791d..e01d21c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2079,6 +2079,12 @@ new default).
 
 --
 
+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'.
+
+
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
 	rebase. False by default.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index ea97576..caa187b 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -128,7 +128,10 @@ already exists on the remote side.
 	Push all the refs that would be pushed without this option,
 	and also push annotated tags in `refs/tags` that are missing
 	from the remote but are pointing at commit-ish that are
-	reachable from the refs being pushed.
+	reachable from the refs being pushed.  This can also be specified
+	with configuration variable 'push.followTags'.  For more
+	information, see 'push.followTags' in linkgit:git-config[1].
+
 
 --signed::
 	GPG-sign the push request to update refs on the receiving
diff --git a/builtin/push.c b/builtin/push.c
index bba22b8..57c138b 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -473,11 +473,21 @@ static int option_parse_recurse_submodules(const struct option *opt,
 
 static int git_push_config(const char *k, const char *v, void *cb)
 {
+	int *flags = cb;
 	int status;
 
 	status = git_gpg_config(k, v, NULL);
 	if (status)
 		return status;
+
+	if (!strcmp(k, "push.followtags")) {
+		if (git_config_bool(k, v))
+			*flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
+		else
+			*flags &= ~TRANSPORT_PUSH_FOLLOW_TAGS;
+		return 0;
+	}
+
 	return git_default_config(k, v, NULL);
 }
 
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c21190d..cffb2b8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2188,6 +2188,7 @@ _git_config ()
 		pull.octopus
 		pull.twohead
 		push.default
+		push.followTags
 		rebase.autosquash
 		rebase.stat
 		receive.autogc
-- 
2.3.0-283-g21bf3f5

Re: [PATCH 2/2] builtin/push.c: make push_default a static variable

[Jeff King]

On Tue, Feb 17, 2015 at 02:16:30PM -0800, Junio C Hamano wrote:

> Do you want to resurrect that @{publish} stuff?  I think it had
> sensible semantics, and I do not think we mind keeping the
> push_default configuration to be read from the default_config
> codepath.

I'll take a look at it and see if it's in good enough shape to apply
as-is, or with minor tweaking. But regardless, let's...

> If we decide to go that route, then the series would become
> something like this:
> 
> $gmane/263871 [1/4] git_push_config: drop cargo-culted wt_status pointer
> $gmane/263878 [2/4] cmd_push: set "atomic" bit directly
> $gmane/263879 [3/4] cmd_push: pass "flags" pointer to config callback
> $gmane/263880 [4/4] push: allow --follow-tags to be set by config push.followTags
> 
> omitting the original 2/2 patch we are discussing.  I am inclined to
> replace what I queued with the above four.

...do this. Even if we don't apply other patches to make use of
push_default immediately, it's a plausible area for us to touch later,
and the cleanup from the dropped patch was not so important.

> +	if (!strcmp(k, "push.followtags")) {
> +		if (git_config_bool(k, v))
> +			*flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
> +		else
> +			*flags &= ~TRANSPORT_PUSH_FOLLOW_TAGS;
> +		return 0;
> +	}

Did you have an opinion on sticking this behind a helper function?

It feels like a lot of repeating of the same variables and flags, but I
worried that "munge_bit" ends up being even more confusing.

-Peff

Re: [PATCH 2/2] builtin/push.c: make push_default a static variable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +	if (!strcmp(k, "push.followtags")) {
>> +		if (git_config_bool(k, v))
>> +			*flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
>> +		else
>> +			*flags &= ~TRANSPORT_PUSH_FOLLOW_TAGS;
>> +		return 0;
>> +	}
>
> Did you have an opinion on sticking this behind a helper function?

Not very strongly either way.  Seeing the above does not bother me
too much, but I do not know how I would feel when I start seeing

	val = git_config_book(k, v);
	flip_bool(val, &flags, TRANSPORT_PUSH_FOLLOW_TAGS);

often.  Not having to make sure that the bit constant whose name
tends to get long is not misspelled is certainly a plus.

Re: [PATCH 2/2] builtin/push.c: make push_default a static variable

[Jeff King]

On Wed, Feb 18, 2015 at 11:08:34AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> +	if (!strcmp(k, "push.followtags")) {
> >> +		if (git_config_bool(k, v))
> >> +			*flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
> >> +		else
> >> +			*flags &= ~TRANSPORT_PUSH_FOLLOW_TAGS;
> >> +		return 0;
> >> +	}
> >
> > Did you have an opinion on sticking this behind a helper function?
> 
> Not very strongly either way.  Seeing the above does not bother me
> too much, but I do not know how I would feel when I start seeing
> 
> 	val = git_config_book(k, v);
> 	flip_bool(val, &flags, TRANSPORT_PUSH_FOLLOW_TAGS);
> 
> often.  Not having to make sure that the bit constant whose name
> tends to get long is not misspelled is certainly a plus.

I think it would be even nicer as:

  git_config_bits(k, v, &flags, TRANSPORT_PUSH_FOLLOW_TAGS);

There is a similar spot in the tar.*.remote config. And that could of
course build on a "flip_bool" or similar, which itself has many other
uses. But after taking a quick peek, I noticed that one call around
diff.c:3600 would look like:

  flip_bool(!negate, &opt->filter, bit);

IOW, it is the same pattern of conditional, but it flips the AND and OR,
because its flag is flipped. Reading that line makes me head hurt,
because we've really introduced an extra double-negative into the flow.

That "negate" flag is local to the loop we are in, and we could flip it
for clarity. But it makes me second-guess the technique.

-Peff

Re: [PATCH 2/2] builtin/push.c: make push_default a static variable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Feb 18, 2015 at 11:08:34AM -0800, Junio C Hamano wrote:
> ...
>> Not very strongly either way.  Seeing the above does not bother me
>> too much, but I do not know how I would feel when I start seeing
>> 
>> 	val = git_config_book(k, v);
>> 	flip_bool(val, &flags, TRANSPORT_PUSH_FOLLOW_TAGS);
>> 
>> often.  Not having to make sure that the bit constant whose name
>> tends to get long is not misspelled is certainly a plus.
>
> I think it would be even nicer as:
>
>   git_config_bits(k, v, &flags, TRANSPORT_PUSH_FOLLOW_TAGS);

Maybe.  I do not feel very strongly either way.

Re: [PATCH 2/2] builtin/push.c: make push_default a static variable

[Jeff King]

On Wed, Feb 18, 2015 at 11:50:20AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Feb 18, 2015 at 11:08:34AM -0800, Junio C Hamano wrote:
> > ...
> >> Not very strongly either way.  Seeing the above does not bother me
> >> too much, but I do not know how I would feel when I start seeing
> >> 
> >> 	val = git_config_book(k, v);
> >> 	flip_bool(val, &flags, TRANSPORT_PUSH_FOLLOW_TAGS);
> >> 
> >> often.  Not having to make sure that the bit constant whose name
> >> tends to get long is not misspelled is certainly a plus.
> >
> > I think it would be even nicer as:
> >
> >   git_config_bits(k, v, &flags, TRANSPORT_PUSH_FOLLOW_TAGS);
> 
> Maybe.  I do not feel very strongly either way.

So I started to prepare a patch for this, because I wanted to see how it
would look. It's below in case you are curious, but note that it doesn't
compile, and that is does something a bit dangerous. So I think I am
giving up on this line of thought. Read on if you're curious.

The sticking point is the type of the bit-field. If it is:

    void flip_bits(int set_or_clear, int *field, int bits);

then we cannot pass a pointer to "unsigned field". We can pass unsigned
bits, but note that we might lose the 32nd (or 64th) bit.

We can do this instead:

    void flip_bits(int set_or_clear, void *field, unsigned bits);

But of course we will end up dereferencing "void *field" as "unsigned *".
I suspect if field is originally an "int" it works fine on most
platforms, but isn't legal according to the standard. Much worse,
though: if your original field is a different size, it's even less
likely to work. Passing a "char *" may set bits in random adjacent
memory (depending on your endianness), and an "unsigned long *" may set
bits in the wrong part of the variable. And because of the "void *", we
get no compiler warnings. :)

Add on top that we may want to use this for C bit-fields, whose address
cannot legally be taken (and this is why it does not compile).

So besides adding type-specific bit-flippers (yuck), I think the only
way to do it universally would be with a macro.  Which I think tips it
over the "too gross, just write it out" line.

---
diff --git a/archive-tar.c b/archive-tar.c
index 0d1e6bd..3c794e2 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -352,10 +352,7 @@ static int tar_filter_config(const char *var, const char *value, void *data)
 		return 0;
 	}
 	if (!strcmp(type, "remote")) {
-		if (git_config_bool(var, value))
-			ar->flags |= ARCHIVER_REMOTE;
-		else
-			ar->flags &= ~ARCHIVER_REMOTE;
+		git_config_bits(var, value, &ar->flags, ARCHIVER_REMOTE);
 		return 0;
 	}
 
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d816587..073445e 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2216,10 +2216,8 @@ static int git_pack_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 	if (!strcmp(k, "pack.writebitmaphashcache")) {
-		if (git_config_bool(k, v))
-			write_bitmap_options |= BITMAP_OPT_HASH_CACHE;
-		else
-			write_bitmap_options &= ~BITMAP_OPT_HASH_CACHE;
+		git_config_bits(k, v, &write_bitmap_options,
+				BITMAP_OPT_HASH_CACHE);
 	}
 	if (!strcmp(k, "pack.usebitmaps")) {
 		use_bitmap_index = git_config_bool(k, v);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 5878986..9d4eb24 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -53,10 +53,7 @@ static int mark_ce_flags(const char *path, int flag, int mark)
 	int namelen = strlen(path);
 	int pos = cache_name_pos(path, namelen);
 	if (0 <= pos) {
-		if (mark)
-			active_cache[pos]->ce_flags |= flag;
-		else
-			active_cache[pos]->ce_flags &= ~flag;
+		flip_bits(mark, &active_cache[pos]->ce_flags, flag);
 		active_cache[pos]->ce_flags |= CE_UPDATE_IN_BASE;
 		cache_tree_invalidate_path(&the_index, path);
 		active_cache_changed |= CE_ENTRY_CHANGED;
diff --git a/cache.h b/cache.h
index f704af5..957f150 100644
--- a/cache.h
+++ b/cache.h
@@ -1316,6 +1316,18 @@ extern int sha1_object_info_extended(const unsigned char *, struct object_info *
 /* Dumb servers support */
 extern int update_server_info(int);
 
+/*
+ * Either set or clear "bits" from "field", based on
+ * whether or not "set_or_clear" is set.
+ */
+static inline void flip_bits(int set_or_clear, void *field, unsigned bits)
+{
+	if (set_or_clear)
+		*(unsigned *)field |= bits;
+	else
+		*(unsigned *)field &= ~bits;
+}
+
 /* git_config_parse_key() returns these negated: */
 #define CONFIG_INVALID_KEY 1
 #define CONFIG_NO_SECTION_OR_NAME 2
@@ -1385,6 +1397,12 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
+static inline int git_config_bits(const char *name, const char *value, void *field, unsigned bits)
+{
+	flip_bits(git_config_bool(name, value), field, bits);
+	return 0;
+}
+
 /*
  * Match and parse a config key of the form:
  *
diff --git a/column.c b/column.c
index 786abe6..21c9765 100644
--- a/column.c
+++ b/column.c
@@ -281,12 +281,8 @@ static int parse_option(const char *arg, int len, unsigned int *colopts,
 
 		if (opts[i].mask)
 			*colopts = (*colopts & ~opts[i].mask) | opts[i].value;
-		else {
-			if (set)
-				*colopts |= opts[i].value;
-			else
-				*colopts &= ~opts[i].value;
-		}
+		else
+			flip_bits(set, colopts, opts[i].value);
 		return 0;
 	}
 
diff --git a/combine-diff.c b/combine-diff.c
index 91edce5..ad52b77 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -581,12 +581,9 @@ static int make_hunks(struct sline *sline, unsigned long cnt,
 	unsigned long i;
 	int has_interesting = 0;
 
-	for (i = 0; i <= cnt; i++) {
-		if (interesting(&sline[i], all_mask))
-			sline[i].flag |= mark;
-		else
-			sline[i].flag &= ~mark;
-	}
+	for (i = 0; i <= cnt; i++)
+		flip_bits(interesting(&sline[i], all_mask),
+			  &sline[i].flag, mark);
 	if (!dense)
 		return give_context(sline, cnt, num_parent);
 
diff --git a/diff.c b/diff.c
index d1bd534..53b9481 100644
--- a/diff.c
+++ b/diff.c
@@ -3585,22 +3585,17 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)
 
 	for (i = 0; (optch = optarg[i]) != '\0'; i++) {
 		unsigned int bit;
-		int negate;
+		int positive = 1;
 
 		if ('a' <= optch && optch <= 'z') {
-			negate = 1;
+			positive = 0;
 			optch = toupper(optch);
-		} else {
-			negate = 0;
 		}
 
 		bit = (0 <= optch && optch <= 'Z') ? filter_bit[optch] : 0;
 		if (!bit)
 			return optarg[i];
-		if (negate)
-			opt->filter &= ~bit;
-		else
-			opt->filter |= bit;
+		flip_bits(positive, &opt->filter, bit);
 	}
 	return 0;
 }
diff --git a/parse-options.c b/parse-options.c
index 80106c0..47c169c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -100,17 +100,11 @@ static int get_value(struct parse_opt_ctx_t *p,
 		return (*(parse_opt_ll_cb *)opt->callback)(p, opt, unset);
 
 	case OPTION_BIT:
-		if (unset)
-			*(int *)opt->value &= ~opt->defval;
-		else
-			*(int *)opt->value |= opt->defval;
+		flip_bits(!unset, opt->value, opt->defval);
 		return 0;
 
 	case OPTION_NEGBIT:
-		if (unset)
-			*(int *)opt->value |= opt->defval;
-		else
-			*(int *)opt->value &= ~opt->defval;
+		flip_bits(unset, opt->value, opt->defval);
 		return 0;
 
 	case OPTION_COUNTUP:
diff --git a/revision.c b/revision.c
index 66520c6..85cb245 100644
--- a/revision.c
+++ b/revision.c
@@ -554,10 +554,8 @@ static int compact_treesame(struct rev_info *revs, struct commit *commit, unsign
 		if (nth_parent != 0)
 			die("compact_treesame %u", nth_parent);
 		old_same = !!(commit->object.flags & TREESAME);
-		if (rev_same_tree_as_empty(revs, commit))
-			commit->object.flags |= TREESAME;
-		else
-			commit->object.flags &= ~TREESAME;
+		flip_bits(rev_same_tree_as_empty(revs, commit),
+			  &commit->object.flags, TREESAME);
 		return old_same;
 	}
 
@@ -578,10 +576,8 @@ static int compact_treesame(struct rev_info *revs, struct commit *commit, unsign
 	if (--st->nparents == 1) {
 		if (commit->parents->next)
 			die("compact_treesame parents mismatch");
-		if (st->treesame[0] && revs->dense)
-			commit->object.flags |= TREESAME;
-		else
-			commit->object.flags &= ~TREESAME;
+		flip_bits(st->treesame[0] && revs->dense,
+			  &commit->object.flags, TREESAME);
 		free(add_decoration(&revs->treesame, &commit->object, NULL));
 	}
 
@@ -609,10 +605,8 @@ static unsigned update_treesame(struct rev_info *revs, struct commit *commit)
 			} else
 				irrelevant_change |= !st->treesame[n];
 		}
-		if (relevant_parents ? relevant_change : irrelevant_change)
-			commit->object.flags &= ~TREESAME;
-		else
-			commit->object.flags |= TREESAME;
+		flip_bits(!(relevant_parents ? relevant_change : irrelevant_change),
+			  &commit->object.flags, TREESAME);
 	}
 
 	return commit->object.flags & TREESAME;
diff --git a/unpack-trees.c b/unpack-trees.c
index be84ba2..9089c5b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -248,10 +248,8 @@ static int apply_sparse_checkout(struct index_state *istate,
 {
 	int was_skip_worktree = ce_skip_worktree(ce);
 
-	if (ce->ce_flags & CE_NEW_SKIP_WORKTREE)
-		ce->ce_flags |= CE_SKIP_WORKTREE;
-	else
-		ce->ce_flags &= ~CE_SKIP_WORKTREE;
+	flip_bits(ce->ce_flags & CE_NEW_SKIP_WORKTREE,
+		  &ce->ce_flags, CE_SKIP_WORKTREE);
 	if (was_skip_worktree != ce_skip_worktree(ce)) {
 		ce->ce_flags |= CE_UPDATE_IN_BASE;
 		istate->cache_changed |= CE_ENTRY_CHANGED;
@@ -982,10 +980,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,
 		if (select_flag && !(ce->ce_flags & select_flag))
 			continue;
 
-		if (!ce_stage(ce))
-			ce->ce_flags |= skip_wt_flag;
-		else
-			ce->ce_flags &= ~skip_wt_flag;
+		flip_bits(!ce_stage(ce), &ce->ce_flags, skip_wt_flag);
 	}
 
 	/*
diff --git a/ws.c b/ws.c
index ea4b2b1..6c5f4bd 100644
--- a/ws.c
+++ b/ws.c
@@ -30,14 +30,14 @@ unsigned parse_whitespace_rule(const char *string)
 		int i;
 		size_t len;
 		const char *ep;
-		int negated = 0;
+		int positive = 1;
 
 		string = string + strspn(string, ", \t\n\r");
 		ep = strchrnul(string, ',');
 		len = ep - string;
 
 		if (*string == '-') {
-			negated = 1;
+			positive = 0;
 			string++;
 			len--;
 		}
@@ -47,10 +47,8 @@ unsigned parse_whitespace_rule(const char *string)
 			if (strncmp(whitespace_rule_names[i].rule_name,
 				    string, len))
 				continue;
-			if (negated)
-				rule &= ~whitespace_rule_names[i].rule_bits;
-			else
-				rule |= whitespace_rule_names[i].rule_bits;
+			flip_bits(positive, &rule,
+				  whitespace_rule_names[i].rule_bits);
 			break;
 		}
 		if (strncmp(string, "tabwidth=", 9) == 0) {

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> From: Dave Olszewski <cxreg@pobox.com>
>
> Signed-off-by: Dave Olszewski <cxreg@pobox.com>
> ---
> Again, this is just a preview. Dave should send the final when he thinks
> it is good.

Dave?

I do not see anything wrong with this version that builds on top of
the previous 2 clean-up.  Personally I find that these clean-up
changes more valuable than I care about this particular feature, and
it is unfortunate that waiting an Ack or reroll of this one kept
them stalled.

I am tempted to throw "Helped-by: Peff" into the log message and
merge the result to 'next', unless I hear otherwise in a few days.

> The if/else I added to the config callback is kind of ugly. I wonder if
> we should have git_config_bit, or even just a function to set/clear a
> bit. Then the OPT_BIT code could use it, too. Something like:
>
>   munge_bit(flags, TRANSPORT_PUSH_FOLLOW_TAGS, git_config_bool(k, v));
>
> Or maybe that is getting too fancy and obfuscated for a simple bit
> set/clear. I dunno.

I think we agreed that the code we have in this series is good.

>  Documentation/config.txt               | 6 ++++++
>  Documentation/git-push.txt             | 5 ++++-
>  builtin/push.c                         | 9 +++++++++
>  contrib/completion/git-completion.bash | 1 +
>  4 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ae6791d..e01d21c 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2079,6 +2079,12 @@ new default).
>  
>  --
>  
> +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'.
> +
> +
>  rebase.stat::
>  	Whether to show a diffstat of what changed upstream since the last
>  	rebase. False by default.
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index ea97576..caa187b 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -128,7 +128,10 @@ already exists on the remote side.
>  	Push all the refs that would be pushed without this option,
>  	and also push annotated tags in `refs/tags` that are missing
>  	from the remote but are pointing at commit-ish that are
> -	reachable from the refs being pushed.
> +	reachable from the refs being pushed.  This can also be specified
> +	with configuration variable 'push.followTags'.  For more
> +	information, see 'push.followTags' in linkgit:git-config[1].
> +
>  
>  --signed::
>  	GPG-sign the push request to update refs on the receiving
> diff --git a/builtin/push.c b/builtin/push.c
> index c25108f..6831c2d 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -482,6 +482,7 @@ static int option_parse_recurse_submodules(const struct option *opt,
>  
>  static int git_push_config(const char *k, const char *v, void *cb)
>  {
> +	int *flags = cb;
>  	int status;
>  
>  	status = git_gpg_config(k, v, NULL);
> @@ -511,6 +512,14 @@ static int git_push_config(const char *k, const char *v, void *cb)
>  		return 0;
>  	}
>  
> +	if (!strcmp(k, "push.followtags")) {
> +		if (git_config_bool(k, v))
> +			*flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
> +		else
> +			*flags &= ~TRANSPORT_PUSH_FOLLOW_TAGS;
> +		return 0;
> +	}
> +
>  	return git_default_config(k, v, NULL);
>  }
>  
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c21190d..cffb2b8 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2188,6 +2188,7 @@ _git_config ()
>  		pull.octopus
>  		pull.twohead
>  		push.default
> +		push.followTags
>  		rebase.autosquash
>  		rebase.stat
>  		receive.autogc

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

[Jeff King]

On Fri, Mar 13, 2015 at 11:06:22PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > From: Dave Olszewski <cxreg@pobox.com>
> >
> > Signed-off-by: Dave Olszewski <cxreg@pobox.com>
> > ---
> > Again, this is just a preview. Dave should send the final when he thinks
> > it is good.
> 
> Dave?
> 
> I do not see anything wrong with this version that builds on top of
> the previous 2 clean-up.  Personally I find that these clean-up
> changes more valuable than I care about this particular feature, and
> it is unfortunate that waiting an Ack or reroll of this one kept
> them stalled.
> 
> I am tempted to throw "Helped-by: Peff" into the log message and
> merge the result to 'next', unless I hear otherwise in a few days.

FWIW, as the author of the leadup patches, that would be fine with me. I
think the end patch is in fine shape.

-Peff

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

[Dave Olszewski]

On Fri, 13 Mar 2015, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > From: Dave Olszewski <cxreg@pobox.com>
> >
> > Signed-off-by: Dave Olszewski <cxreg@pobox.com>
> > ---
> > Again, this is just a preview. Dave should send the final when he thinks
> > it is good.
> 
> Dave?
> 
> I do not see anything wrong with this version that builds on top of
> the previous 2 clean-up.  Personally I find that these clean-up
> changes more valuable than I care about this particular feature, and
> it is unfortunate that waiting an Ack or reroll of this one kept
> them stalled.
> 
> I am tempted to throw "Helped-by: Peff" into the log message and
> merge the result to 'next', unless I hear otherwise in a few days.

Sorry, work has kept me very busy lately, I haven't had time to re-visit
this.  Jeff's version looks great to me, please go ahead with it.
Thanks everyone.

    Dave

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

[Junio C Hamano]

Dave Olszewski <cxreg@pobox.com> writes:

> On Fri, 13 Mar 2015, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > From: Dave Olszewski <cxreg@pobox.com>
>> >
>> > Signed-off-by: Dave Olszewski <cxreg@pobox.com>
>> > ---
>> > Again, this is just a preview. Dave should send the final when he thinks
>> > it is good.
>> 
>> Dave?
>> 
>> I do not see anything wrong with this version that builds on top of
>> the previous 2 clean-up.  Personally I find that these clean-up
>> changes more valuable than I care about this particular feature, and
>> it is unfortunate that waiting an Ack or reroll of this one kept
>> them stalled.
>> 
>> I am tempted to throw "Helped-by: Peff" into the log message and
>> merge the result to 'next', unless I hear otherwise in a few days.
>
> Sorry, work has kept me very busy lately, I haven't had time to re-visit
> this.  Jeff's version looks great to me, please go ahead with it.
> Thanks everyone.
>
>     Dave

Thanks.