BUG: For first push to a bare repo, using "--tags" prevents creation of master branch

BUG: For first push to a bare repo, using "--tags" prevents creation of master branch

[Michael Darling]

Using git version 2.4.1.171.ga34f239, and gitolite v3.6.3 (b05c94ff).
I apologize I don't know enough about the client vs server functions,
during a git push, to know whether this is a git or gitolite issue.

After cloning the public glibc repo, and creating a new bare repo on
my local gitserver by adding it to gitolite.conf and pushing it:

$ git push --set-upstream origin
Counting objects: 339319, done.
Delta compression using up to 10 threads.
Compressing objects: 100% (55496/55496), done.
Writing objects: 100% (339319/339319), 99.28 MiB | 22.67 MiB/s, done.
Total 339319 (delta 283044), reused 331722 (delta 276505)
To gitserver:glibctest
 * [new branch]      master -> master
Branch master set up to track remote branch master from origin.

**After resetting to before the last push**: (removing and re-cloning
my clone of the public glibc repo, and resetting the repo on my local
gitserver back to a new bare repo (removing the repo from
gitolite.conf, pushing the conf, removing the repo directory in the
gitolite user, re-adding the repo to gitolite.conf, and pushing the
conf):

$ git push --tags --set-upstream origin
Counting objects: 382283, done.
Delta compression using up to 10 threads.
Compressing objects: 100% (62075/62075), done.
Writing objects: 100% (382283/382283), 101.16 MiB | 19.64 MiB/s, done.
Total 382283 (delta 316774), reused 376933 (delta 312407)
To gitserver:glibctest
 * [new tag]         cvs/ChangeLog -> cvs/ChangeLog
 * [new tag]         ... <a bunch more> ...
 * [new tag]         glibc-2.9 -> glibc-2.9

Note this time there is NO:

 * [new branch]      master -> master
Branch master set up to track remote branch master from origin.

Hence, my local gitserver has no master branch.  If cloning in another
location, I get a "warning: remote HEAD refers to nonexistent ref,
unable to checkout." that I don't get if I do a "git push
--set-upstream origin && git push --tags" instead.

Re: BUG: For first push to a bare repo, using "--tags" prevents creation of master branch

[Matthieu Moy]

Michael Darling <darlingm@gmail.com> writes:

> $ git push --tags --set-upstream origin

[...]

> Note this time there is NO:
>
>  * [new branch]      master -> master
> Branch master set up to track remote branch master from origin.

man git-push says:

SYNOPSIS
       git push [...] [<repository> [<refspec>...]]
[...]
       --tags
           All refs under refs/tags are pushed, in addition to refspecs explicitly listed on the command
           line.

You did not provide any refspec (you provided <repository> but not
<refspec>), hence --tags pushes only tags. So, this is the expected
behavior.

That said, we may want to add an option like --tags-also that would push
tags _in addition_ to what would normally be pushed. --follow-tags does
more or less this, though, but only for annotated tags pointing at a ref
being pushed.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: BUG: For first push to a bare repo, using "--tags" prevents creation of master branch

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> You did not provide any refspec (you provided <repository> but not
> <refspec>), hence --tags pushes only tags. So, this is the expected
> behavior.

Right.

> That said, we may want to add an option like --tags-also that would push
> tags _in addition_ to what would normally be pushed. --follow-tags does
> more or less this, though, but only for annotated tags pointing at a ref
> being pushed.

Yeah.  I am not sure --tags-also is necessarily a good thing (I do
not think "fetch --tags" is a good thing, either).  If --follow-tags
does not touch lightweight ones, that may be a bug we would want to
fix.  I did not mean to implement --follow-tags that way, and I do
not offhand remember if there was a reason to omit lightweight ones.

Thanks.

Re: BUG: For first push to a bare repo, using "--tags" prevents creation of master branch

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Yeah.  I am not sure --tags-also is necessarily a good thing (I do
> not think "fetch --tags" is a good thing, either).  If --follow-tags
> does not touch lightweight ones, that may be a bug we would want to
> fix.  I did not mean to implement --follow-tags that way, and I do
> not offhand remember if there was a reason to omit lightweight ones.

It's documented to push only annotated tags both in the commit message
of c2aba15 (push: --follow-tags, 2013-03-04) and in the man page, so it
was done on purpose, but I don't know if it was a good thing.

Perhaps a justification is that annotated tags are clearly meant to be
public while lightweight tags can also be used as private bookmarks.

In any case, the fact that it's been this way and documented this way
since 2013 makes it a bit hard to change now.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: BUG: For first push to a bare repo, using "--tags" prevents creation of master branch

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Perhaps a justification is that annotated tags are clearly meant to be
> public while lightweight tags can also be used as private bookmarks.

Ah, thanks.  That makes perfect sense.

> In any case, the fact that it's been this way and documented this way
> since 2013 makes it a bit hard to change now.

That too.

I recall "fetch --tags" was updated to become your "--tags-also" by
e66ef7ae (Merge branch 'mh/fetch-tags-in-addition-to-normal-refs',
2013-12-12).  Perhaps we should do the same on the push side.

Re: BUG: For first push to a bare repo, using "--tags" prevents creation of master branch

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I recall "fetch --tags" was updated to become your "--tags-also" by
> e66ef7ae (Merge branch 'mh/fetch-tags-in-addition-to-normal-refs',
> 2013-12-12).  Perhaps we should do the same on the push side.




 builtin/push.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 57c138b..ec215d7 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -382,7 +382,7 @@ static int push_with_options(struct transport *transport, int flags)
 	return 1;
 }
 
-static int do_push(const char *repo, int flags)
+static int do_push(const char *repo, int flags, int tags_too)
 {
 	int i, errs;
 	struct remote *remote = pushremote_get(repo);
@@ -429,6 +429,10 @@ static int do_push(const char *repo, int flags)
 		} else if (!(flags & TRANSPORT_PUSH_MIRROR))
 			setup_default_push_refspecs(remote);
 	}
+
+	if (tags_too)
+		add_refspec("refs/tags/*");
+
 	errs = 0;
 	url_nr = push_url_of_remote(remote, &url);
 	if (url_nr) {
@@ -494,7 +498,7 @@ static int git_push_config(const char *k, const char *v, void *cb)
 int cmd_push(int argc, const char **argv, const char *prefix)
 {
 	int flags = 0;
-	int tags = 0;
+	int tags_too = 0;
 	int rc;
 	const char *repo = NULL;	/* default repository */
 	struct option options[] = {
@@ -504,7 +508,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT( 0 , "mirror", &flags, N_("mirror all refs"),
 			    (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),
 		OPT_BOOL( 0, "delete", &deleterefs, N_("delete refs")),
-		OPT_BOOL( 0 , "tags", &tags, N_("push tags (can't be used with --all or --mirror)")),
+		OPT_BOOL( 0 , "tags", &tags_too, N_("push tags (can't be used with --all or --mirror)")),
 		OPT_BIT('n' , "dry-run", &flags, N_("dry run"), TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
 		OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
@@ -535,20 +539,18 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	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))))
+	if (deleterefs &&
+	    (tags_too || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
 		die(_("--delete is incompatible with --all, --mirror and --tags"));
 	if (deleterefs && argc < 2)
 		die(_("--delete doesn't make sense without any refs"));
 
-	if (tags)
-		add_refspec("refs/tags/*");
-
 	if (argc > 0) {
 		repo = argv[0];
 		set_refspecs(argv + 1, argc - 1, repo);
 	}
 
-	rc = do_push(repo, flags);
+	rc = do_push(repo, flags, tags_too);
 	if (rc == -1)
 		usage_with_options(push_usage, options);
 	else

Re: BUG: For first push to a bare repo, using "--tags" prevents creation of master branch

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> I recall "fetch --tags" was updated to become your "--tags-also" by
> e66ef7ae (Merge branch 'mh/fetch-tags-in-addition-to-normal-refs',
> 2013-12-12).  Perhaps we should do the same on the push side.

Would we still have a way to push tags only with this?

fetch and push are a bit different: it's almost never a bad idea to run
fetch (I see remote-tracking as a kind of cache, and fetch is the way to
refresh this cache), but pushing too much can be harmful. Not a strong
objection, but I'm not sure that this change is 100% safe for everyone.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: BUG: For first push to a bare repo, using "--tags" prevents creation of master branch

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I recall "fetch --tags" was updated to become your "--tags-also" by
>> e66ef7ae (Merge branch 'mh/fetch-tags-in-addition-to-normal-refs',
>> 2013-12-12).  Perhaps we should do the same on the push side.
>
> Would we still have a way to push tags only with this?
>
> fetch and push are a bit different: it's almost never a bad idea to run
> fetch (I see remote-tracking as a kind of cache, and fetch is the way to
> refresh this cache), but pushing too much can be harmful. Not a strong
> objection, but I'm not sure that this change is 100% safe for everyone.

Yeah, a possible fallout can be seen in the updates to the tests in
this patch.  I am undecided if "pushing can be harmful" should trump
"--tags should behave in a similar way for fetch and push that are
the reverse of each other".

If we are to introduce --tags-also (or --tags-too), we should add it
to fetch, too.


-- >8 --
Subject: [PATCH] push --tags: push tags *in addition to* other stuff

Earlier, we changed "git fetch --tags" from "fetch all tags instead
of what we would fetch without --tags argument" to "fetch all tags
in addition to what we would fetch without --tags argument" with
c5a84e92 (fetch --tags: fetch tags *in addition to* other stuff,
2013-10-30), which was merged as a part of a series at e66ef7ae.

This does the same to "git push --tags"; instead of pushing all tags
(and nothing else, as the command line does not have any other
refspec), the command now pushes all tags in addition to what "git
push" would have pushed.

The changes to the tests shows a possible fallout.  Both of them do
not set up the default push refspec, causing "git push" without any
refspecs on the command line to fail, which was OK because they do
not intend to do "git push" without any explicit refspec on the
command line.  But now "--tags" no longer is considered an explicit
refspec, the logic to find the refs to be pushed by default is
triggered to find what "other stuff" needs to be pushed, and causes
the "git push --tags" to fail.  As it is to push tags and nothing
else what these pushes want to do, replace these "git push --tags"
invocations with "git push" with an explicit "refs/tags/\*" refspec.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/push.c                   | 18 ++++++++++--------
 t/t5509-fetch-push-namespaces.sh |  2 +-
 t/t5516-fetch-push.sh            |  2 +-
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 57c138b..ec215d7 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -382,7 +382,7 @@ static int push_with_options(struct transport *transport, int flags)
 	return 1;
 }
 
-static int do_push(const char *repo, int flags)
+static int do_push(const char *repo, int flags, int tags_too)
 {
 	int i, errs;
 	struct remote *remote = pushremote_get(repo);
@@ -429,6 +429,10 @@ static int do_push(const char *repo, int flags)
 		} else if (!(flags & TRANSPORT_PUSH_MIRROR))
 			setup_default_push_refspecs(remote);
 	}
+
+	if (tags_too)
+		add_refspec("refs/tags/*");
+
 	errs = 0;
 	url_nr = push_url_of_remote(remote, &url);
 	if (url_nr) {
@@ -494,7 +498,7 @@ static int git_push_config(const char *k, const char *v, void *cb)
 int cmd_push(int argc, const char **argv, const char *prefix)
 {
 	int flags = 0;
-	int tags = 0;
+	int tags_too = 0;
 	int rc;
 	const char *repo = NULL;	/* default repository */
 	struct option options[] = {
@@ -504,7 +508,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT( 0 , "mirror", &flags, N_("mirror all refs"),
 			    (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),
 		OPT_BOOL( 0, "delete", &deleterefs, N_("delete refs")),
-		OPT_BOOL( 0 , "tags", &tags, N_("push tags (can't be used with --all or --mirror)")),
+		OPT_BOOL( 0 , "tags", &tags_too, N_("push tags (can't be used with --all or --mirror)")),
 		OPT_BIT('n' , "dry-run", &flags, N_("dry run"), TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
 		OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
@@ -535,20 +539,18 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	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))))
+	if (deleterefs &&
+	    (tags_too || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
 		die(_("--delete is incompatible with --all, --mirror and --tags"));
 	if (deleterefs && argc < 2)
 		die(_("--delete doesn't make sense without any refs"));
 
-	if (tags)
-		add_refspec("refs/tags/*");
-
 	if (argc > 0) {
 		repo = argv[0];
 		set_refspecs(argv + 1, argc - 1, repo);
 	}
 
-	rc = do_push(repo, flags);
+	rc = do_push(repo, flags, tags_too);
 	if (rc == -1)
 		usage_with_options(push_usage, options);
 	else
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index cc0b31f..2f9b0f0 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -30,7 +30,7 @@ test_expect_success 'pushing into a repository using a ref namespace' '
 		git ls-remote pushee-namespaced >actual &&
 		printf "$commit1\trefs/heads/master\n" >expected &&
 		test_cmp expected actual &&
-		git push pushee-namespaced --tags &&
+		git push pushee-namespaced "refs/tags/*" &&
 		git ls-remote pushee-namespaced >actual &&
 		printf "$commit0\trefs/tags/0\n" >>expected &&
 		printf "$commit1\trefs/tags/1\n" >>expected &&
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8a5f236..9d54186 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -802,7 +802,7 @@ test_expect_success 'allow deleting a ref using --delete' '
 test_expect_success 'allow deleting a tag using --delete' '
 	mk_test testrepo heads/master &&
 	git tag -a -m dummy_message deltag heads/master &&
-	git push testrepo --tags &&
+	git push testrepo "refs/tags/*" &&
 	(cd testrepo && git rev-parse --verify -q refs/tags/deltag) &&
 	git push testrepo --delete tag deltag &&
 	(cd testrepo && test_must_fail git rev-parse --verify refs/tags/deltag)
-- 
2.4.2-540-gc413a76

Re: Not a BUG: For first push to a bare repo, using "--tags" prevents creation of master branch

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I recall "fetch --tags" was updated to become your "--tags-also" by
>> e66ef7ae (Merge branch 'mh/fetch-tags-in-addition-to-normal-refs',
>> 2013-12-12).  Perhaps we should do the same on the push side.
>
> Would we still have a way to push tags only with this?
>
> fetch and push are a bit different: it's almost never a bad idea to run
> fetch (I see remote-tracking as a kind of cache, and fetch is the way to
> refresh this cache), but pushing too much can be harmful. Not a strong
> objection, but I'm not sure that this change is 100% safe for everyone.

Whether the user is familiar with "git fetch --tags" or not, "git
push --tags" that is not spelled as "git push 'tags/*'" does look
like an option that augments what the command normally does, and the
"mistaken" (non-)bug report was very understandable. 

After having thought about this, I would say that it would not add
much value to the system if we add it as "git push --tags-also".  We
should either keep the current semantics as-is, or we replace "git
push --tags" to make it similar to what "git fetch --tags" does, I
think.