Jeff King [off-list ref] 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.
On Tue, Feb 17, 2015 at 09:45:07AM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
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
Jeff King [off-list ref] writes:
quoted
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 <redacted>
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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
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
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