* [PATCH] push: allow --follow-tags to be set by config push.followTags @ 2015-02-16 3:01 Dave Olszewski 2015-02-16 5:20 ` Jeff King 0 siblings, 1 reply; 29+ messages in thread From: Dave Olszewski @ 2015-02-16 3:01 UTC (permalink / raw) To: git, gitster; +Cc: 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] push: allow --follow-tags to be set by config push.followTags 2015-02-16 3:01 [PATCH] push: allow --follow-tags to be set by config push.followTags Dave Olszewski @ 2015-02-16 5:20 ` Jeff King 2015-02-16 5:45 ` [PATCH 0/2] clean up push config callbacks Jeff King 0 siblings, 1 reply; 29+ messages in thread From: Jeff King @ 2015-02-16 5:20 UTC (permalink / raw) To: Dave Olszewski; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 0/2] clean up push config callbacks 2015-02-16 5:20 ` Jeff King @ 2015-02-16 5:45 ` Jeff King 2015-02-16 5:46 ` [PATCH 1/2] git_push_config: drop cargo-culted wt_status pointer Jeff King ` (2 more replies) 0 siblings, 3 replies; 29+ messages in thread From: Jeff King @ 2015-02-16 5:45 UTC (permalink / raw) To: Dave Olszewski; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 1/2] git_push_config: drop cargo-culted wt_status pointer 2015-02-16 5:45 ` [PATCH 0/2] clean up push config callbacks Jeff King @ 2015-02-16 5:46 ` Jeff King 2015-02-16 5:47 ` [PATCH 2/2] builtin/push.c: make push_default a static variable Jeff King 2015-02-16 5:54 ` [PATCH 3/2] push: allow --follow-tags to be set by config push.followTags Jeff King 2 siblings, 0 replies; 29+ messages in thread From: Jeff King @ 2015-02-16 5:46 UTC (permalink / raw) To: Dave Olszewski; +Cc: git, gitster 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 2/2] builtin/push.c: make push_default a static variable 2015-02-16 5:45 ` [PATCH 0/2] clean up push config callbacks Jeff King 2015-02-16 5:46 ` [PATCH 1/2] git_push_config: drop cargo-culted wt_status pointer Jeff King @ 2015-02-16 5:47 ` Jeff King 2015-02-16 5:57 ` Junio C Hamano 2015-02-17 10:46 ` Jeff King 2015-02-16 5:54 ` [PATCH 3/2] push: allow --follow-tags to be set by config push.followTags Jeff King 2 siblings, 2 replies; 29+ messages in thread From: Jeff King @ 2015-02-16 5:47 UTC (permalink / raw) To: Dave Olszewski; +Cc: git, gitster 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] builtin/push.c: make push_default a static variable 2015-02-16 5:47 ` [PATCH 2/2] builtin/push.c: make push_default a static variable Jeff King @ 2015-02-16 5:57 ` Junio C Hamano 2015-02-17 10:46 ` Jeff King 1 sibling, 0 replies; 29+ messages in thread From: Junio C Hamano @ 2015-02-16 5:57 UTC (permalink / raw) To: Jeff King; +Cc: Dave Olszewski, Git Mailing List 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] builtin/push.c: make push_default a static variable 2015-02-16 5:47 ` [PATCH 2/2] builtin/push.c: make push_default a static variable Jeff King 2015-02-16 5:57 ` Junio C Hamano @ 2015-02-17 10:46 ` Jeff King 2015-02-17 17:45 ` Junio C Hamano 1 sibling, 1 reply; 29+ messages in thread From: Jeff King @ 2015-02-17 10:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, git 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] builtin/push.c: make push_default a static variable 2015-02-17 10:46 ` Jeff King @ 2015-02-17 17:45 ` Junio C Hamano 2015-02-17 18:23 ` Jeff King 0 siblings, 1 reply; 29+ messages in thread From: Junio C Hamano @ 2015-02-17 17:45 UTC (permalink / raw) To: Jeff King; +Cc: Dave Olszewski, git 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] builtin/push.c: make push_default a static variable 2015-02-17 17:45 ` Junio C Hamano @ 2015-02-17 18:23 ` Jeff King 2015-02-17 22:16 ` Junio C Hamano 0 siblings, 1 reply; 29+ messages in thread From: Jeff King @ 2015-02-17 18:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, git 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] builtin/push.c: make push_default a static variable 2015-02-17 18:23 ` Jeff King @ 2015-02-17 22:16 ` Junio C Hamano 2015-02-18 18:50 ` Jeff King 0 siblings, 1 reply; 29+ messages in thread From: Junio C Hamano @ 2015-02-17 22:16 UTC (permalink / raw) To: Jeff King; +Cc: Dave Olszewski, git 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] builtin/push.c: make push_default a static variable 2015-02-17 22:16 ` Junio C Hamano @ 2015-02-18 18:50 ` Jeff King 2015-02-18 19:08 ` Junio C Hamano 0 siblings, 1 reply; 29+ messages in thread From: Jeff King @ 2015-02-18 18:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, git 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] builtin/push.c: make push_default a static variable 2015-02-18 18:50 ` Jeff King @ 2015-02-18 19:08 ` Junio C Hamano 2015-02-18 19:25 ` Jeff King 0 siblings, 1 reply; 29+ messages in thread From: Junio C Hamano @ 2015-02-18 19:08 UTC (permalink / raw) To: Jeff King; +Cc: Dave Olszewski, git 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] builtin/push.c: make push_default a static variable 2015-02-18 19:08 ` Junio C Hamano @ 2015-02-18 19:25 ` Jeff King 2015-02-18 19:50 ` Junio C Hamano 0 siblings, 1 reply; 29+ messages in thread From: Jeff King @ 2015-02-18 19:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, git 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] builtin/push.c: make push_default a static variable 2015-02-18 19:25 ` Jeff King @ 2015-02-18 19:50 ` Junio C Hamano 2015-02-18 20:03 ` Jeff King 0 siblings, 1 reply; 29+ messages in thread From: Junio C Hamano @ 2015-02-18 19:50 UTC (permalink / raw) To: Jeff King; +Cc: Dave Olszewski, git 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] builtin/push.c: make push_default a static variable 2015-02-18 19:50 ` Junio C Hamano @ 2015-02-18 20:03 ` Jeff King 0 siblings, 0 replies; 29+ messages in thread From: Jeff King @ 2015-02-18 20:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, git 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) { ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 3/2] push: allow --follow-tags to be set by config push.followTags 2015-02-16 5:45 ` [PATCH 0/2] clean up push config callbacks Jeff King 2015-02-16 5:46 ` [PATCH 1/2] git_push_config: drop cargo-culted wt_status pointer Jeff King 2015-02-16 5:47 ` [PATCH 2/2] builtin/push.c: make push_default a static variable Jeff King @ 2015-02-16 5:54 ` Jeff King 2015-02-16 6:02 ` Junio C Hamano 2 siblings, 1 reply; 29+ messages in thread From: Jeff King @ 2015-02-16 5:54 UTC (permalink / raw) To: Dave Olszewski; +Cc: git, gitster 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 3/2] push: allow --follow-tags to be set by config push.followTags 2015-02-16 5:54 ` [PATCH 3/2] push: allow --follow-tags to be set by config push.followTags Jeff King @ 2015-02-16 6:02 ` Junio C Hamano 2015-02-16 6:10 ` [PATCH 0/3] cleaner bit-setting in cmd_push Jeff King 2015-02-16 6:11 ` [PATCH 3/2] " Junio C Hamano 0 siblings, 2 replies; 29+ messages in thread From: Junio C Hamano @ 2015-02-16 6:02 UTC (permalink / raw) To: Jeff King; +Cc: Dave Olszewski, Git Mailing List 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 0/3] cleaner bit-setting in cmd_push 2015-02-16 6:02 ` Junio C Hamano @ 2015-02-16 6:10 ` Jeff King 2015-02-16 6:12 ` [PATCH 1/3] cmd_push: set "atomic" bit directly Jeff King ` (2 more replies) 2015-02-16 6:11 ` [PATCH 3/2] " Junio C Hamano 1 sibling, 3 replies; 29+ messages in thread From: Jeff King @ 2015-02-16 6:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, Git Mailing List 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 1/3] cmd_push: set "atomic" bit directly 2015-02-16 6:10 ` [PATCH 0/3] cleaner bit-setting in cmd_push Jeff King @ 2015-02-16 6:12 ` Jeff King 2015-02-16 6:13 ` [PATCH 2/3] cmd_push: pass "flags" pointer to config callback Jeff King 2015-02-16 6:16 ` [PATCH 3/3] push: allow --follow-tags to be set by config push.followTags Jeff King 2 siblings, 0 replies; 29+ messages in thread From: Jeff King @ 2015-02-16 6:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, Git Mailing List 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 2/3] cmd_push: pass "flags" pointer to config callback 2015-02-16 6:10 ` [PATCH 0/3] cleaner bit-setting in cmd_push Jeff King 2015-02-16 6:12 ` [PATCH 1/3] cmd_push: set "atomic" bit directly Jeff King @ 2015-02-16 6:13 ` Jeff King 2015-02-16 7:05 ` Junio C Hamano 2015-02-16 6:16 ` [PATCH 3/3] push: allow --follow-tags to be set by config push.followTags Jeff King 2 siblings, 1 reply; 29+ messages in thread From: Jeff King @ 2015-02-16 6:13 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, Git Mailing List 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 2/3] cmd_push: pass "flags" pointer to config callback 2015-02-16 6:13 ` [PATCH 2/3] cmd_push: pass "flags" pointer to config callback Jeff King @ 2015-02-16 7:05 ` Junio C Hamano 2015-02-16 7:16 ` Jeff King 0 siblings, 1 reply; 29+ messages in thread From: Junio C Hamano @ 2015-02-16 7:05 UTC (permalink / raw) To: Jeff King; +Cc: Dave Olszewski, Git Mailing List 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)))) ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/3] cmd_push: pass "flags" pointer to config callback 2015-02-16 7:05 ` Junio C Hamano @ 2015-02-16 7:16 ` Jeff King 0 siblings, 0 replies; 29+ messages in thread From: Jeff King @ 2015-02-16 7:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, Git Mailing List 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 3/3] push: allow --follow-tags to be set by config push.followTags 2015-02-16 6:10 ` [PATCH 0/3] cleaner bit-setting in cmd_push Jeff King 2015-02-16 6:12 ` [PATCH 1/3] cmd_push: set "atomic" bit directly Jeff King 2015-02-16 6:13 ` [PATCH 2/3] cmd_push: pass "flags" pointer to config callback Jeff King @ 2015-02-16 6:16 ` Jeff King 2015-03-14 6:06 ` Junio C Hamano 2 siblings, 1 reply; 29+ messages in thread From: Jeff King @ 2015-02-16 6:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, Git Mailing List 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] push: allow --follow-tags to be set by config push.followTags 2015-02-16 6:16 ` [PATCH 3/3] push: allow --follow-tags to be set by config push.followTags Jeff King @ 2015-03-14 6:06 ` Junio C Hamano 2015-03-14 17:34 ` Jeff King 2015-03-14 17:50 ` Dave Olszewski 0 siblings, 2 replies; 29+ messages in thread From: Junio C Hamano @ 2015-03-14 6:06 UTC (permalink / raw) To: Dave Olszewski, Jeff King; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] push: allow --follow-tags to be set by config push.followTags 2015-03-14 6:06 ` Junio C Hamano @ 2015-03-14 17:34 ` Jeff King 2015-03-14 17:50 ` Dave Olszewski 1 sibling, 0 replies; 29+ messages in thread From: Jeff King @ 2015-03-14 17:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, Git Mailing List 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] push: allow --follow-tags to be set by config push.followTags 2015-03-14 6:06 ` Junio C Hamano 2015-03-14 17:34 ` Jeff King @ 2015-03-14 17:50 ` Dave Olszewski 2015-03-14 22:08 ` Junio C Hamano 1 sibling, 1 reply; 29+ messages in thread From: Dave Olszewski @ 2015-03-14 17:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, Jeff King, Git Mailing List 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] push: allow --follow-tags to be set by config push.followTags 2015-03-14 17:50 ` Dave Olszewski @ 2015-03-14 22:08 ` Junio C Hamano 0 siblings, 0 replies; 29+ messages in thread From: Junio C Hamano @ 2015-03-14 22:08 UTC (permalink / raw) To: Dave Olszewski; +Cc: Jeff King, Git Mailing List 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/2] push: allow --follow-tags to be set by config push.followTags 2015-02-16 6:02 ` Junio C Hamano 2015-02-16 6:10 ` [PATCH 0/3] cleaner bit-setting in cmd_push Jeff King @ 2015-02-16 6:11 ` Junio C Hamano 2015-02-16 6:17 ` Jeff King 1 sibling, 1 reply; 29+ messages in thread From: Junio C Hamano @ 2015-02-16 6:11 UTC (permalink / raw) To: Jeff King; +Cc: Dave Olszewski, Git Mailing List 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/2] push: allow --follow-tags to be set by config push.followTags 2015-02-16 6:11 ` [PATCH 3/2] " Junio C Hamano @ 2015-02-16 6:17 ` Jeff King 0 siblings, 0 replies; 29+ messages in thread From: Jeff King @ 2015-02-16 6:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dave Olszewski, Git Mailing List 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2015-03-14 22:08 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-02-16 3:01 [PATCH] push: allow --follow-tags to be set by config push.followTags Dave Olszewski 2015-02-16 5:20 ` Jeff King 2015-02-16 5:45 ` [PATCH 0/2] clean up push config callbacks Jeff King 2015-02-16 5:46 ` [PATCH 1/2] git_push_config: drop cargo-culted wt_status pointer Jeff King 2015-02-16 5:47 ` [PATCH 2/2] builtin/push.c: make push_default a static variable Jeff King 2015-02-16 5:57 ` Junio C Hamano 2015-02-17 10:46 ` Jeff King 2015-02-17 17:45 ` Junio C Hamano 2015-02-17 18:23 ` Jeff King 2015-02-17 22:16 ` Junio C Hamano 2015-02-18 18:50 ` Jeff King 2015-02-18 19:08 ` Junio C Hamano 2015-02-18 19:25 ` Jeff King 2015-02-18 19:50 ` Junio C Hamano 2015-02-18 20:03 ` Jeff King 2015-02-16 5:54 ` [PATCH 3/2] push: allow --follow-tags to be set by config push.followTags Jeff King 2015-02-16 6:02 ` Junio C Hamano 2015-02-16 6:10 ` [PATCH 0/3] cleaner bit-setting in cmd_push Jeff King 2015-02-16 6:12 ` [PATCH 1/3] cmd_push: set "atomic" bit directly Jeff King 2015-02-16 6:13 ` [PATCH 2/3] cmd_push: pass "flags" pointer to config callback Jeff King 2015-02-16 7:05 ` Junio C Hamano 2015-02-16 7:16 ` Jeff King 2015-02-16 6:16 ` [PATCH 3/3] push: allow --follow-tags to be set by config push.followTags Jeff King 2015-03-14 6:06 ` Junio C Hamano 2015-03-14 17:34 ` Jeff King 2015-03-14 17:50 ` Dave Olszewski 2015-03-14 22:08 ` Junio C Hamano 2015-02-16 6:11 ` [PATCH 3/2] " Junio C Hamano 2015-02-16 6:17 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.