Fetching tags overwrites existing tags

Fetching tags overwrites existing tags

[Wink Saville]

If have a repository with a tag "v1.0.0" and I add a remote repository
which also has a tag "v1.0.0" tag is overwritten.

Google found [1] from 2011 and option 3 is what I'd like to see. Has it been
implemented and I just don't see it?

[1]: https://groups.google.com/forum/#!topic/git-version-control/0l_rJFyTE60


Here is an example demonstrating what I see:

$ echo abc > abc.txt
$ git init .
Initialized empty Git repository in
/home/wink/prgs/git/investigate-fetch-tags/.git/
$ git add *
$ git commit -m "Initial commit"
[master (root-commit) 1116fdc] Initial commit
 1 file changed, 1 insertion(+)
 create mode 100644 abc.txt
$ git tag v1.0.0
$ git remote add gbenchmark git@github.com:google/benchmark
$ git log --graph --format="%h %s %d"
* 1116fdc Initial commit  (HEAD -> master, tag: v1.0.0)
$ git fetch --tags gbenchmark
warning: no common commits
remote: Counting objects: 4400, done.
remote: Compressing objects: 100% (15/15), done.
remote: Total 4400 (delta 5), reused 5 (delta 3), pack-reused 4382
Receiving objects: 100% (4400/4400), 1.33 MiB | 2.81 MiB/s, done.
Resolving deltas: 100% (2863/2863), done.
From github.com:google/benchmark
 * [new branch]      clangtidy       -> gbenchmark/clangtidy
 * [new branch]      iter_report     -> gbenchmark/iter_report
 * [new branch]      master          -> gbenchmark/master
 * [new branch]      releasing       -> gbenchmark/releasing
 * [new branch]      reportercleanup -> gbenchmark/reportercleanup
 * [new branch]      rmheaders       -> gbenchmark/rmheaders
 * [new branch]      v2              -> gbenchmark/v2
 * [new tag]         v0.0.9          -> v0.0.9
 * [new tag]         v0.1.0          -> v0.1.0
 t [tag update]      v1.0.0          -> v1.0.0
 * [new tag]         v1.1.0          -> v1.1.0
 * [new tag]         v1.2.0          -> v1.2.0
 * [new tag]         v1.3.0          -> v1.3.0
 * [new tag]         v1.4.0          -> v1.4.0
$ git log --graph --format="%h %s %d"
* 1116fdc Initial commit  (HEAD -> master)

As you can see the tag on 1116fdc is gone, v1.0.0 tag has been updated
and now its pointing to the tag in gbenchmark:

$ git log -5 --graph --format="%h %s %d" v1.0.0
*   cd525ae Merge pull request #171 from eliben/update-doc-userealtime
 (tag: v1.0.0)
|\
| * c7ab1b9 Update README to mention UseRealTime for wallclock time
measurements.
|/
* f662e8b Rename OS_MACOSX macro to new name BENCHMARK_OS_MACOSX. Fix #169
*   0a1f484 Merge pull request #166 from disconnect3d/master
|\
| * d2917bc Fixes #165: CustomArguments ret type in README
|/

Ideally I would have liked the tags fetched from gbenchmark to have a prefix
of gbenchmark/, like the branches have, maybe something like:

$ git fetch --tags gbenchmark
...
 * [new branch]      v2              -> gbenchmark/v2
 * [new tag]         v0.0.9          -> gbenchmark/v0.0.9
 * [new tag]         v0.1.0          -> gbenchmark/v0.1.0
 * [new tag]         v1.0.0          -> gbenchmark/v1.0.0
 * [new tag]         v1.1.0          -> gbenchmark/v1.1.0
 * [new tag]         v1.2.0          -> gbenchmark/v1.2.0
 * [new tag]         v1.3.0          -> gbenchmark/v1.3.0
 * [new tag]         v1.4.0          -> gbenchmark/v1.4.0


-- Wink

Re: Fetching tags overwrites existing tags

[Jacob Keller]

On Tue, Apr 24, 2018 at 12:57 PM, Wink Saville <wink@saville.com> wrote:
> If have a repository with a tag "v1.0.0" and I add a remote repository
> which also has a tag "v1.0.0" tag is overwritten.
>
> Google found [1] from 2011 and option 3 is what I'd like to see. Has it been
> implemented and I just don't see it?
>
> [1]: https://groups.google.com/forum/#!topic/git-version-control/0l_rJFyTE60
>
>
> Here is an example demonstrating what I see:
>
> $ echo abc > abc.txt
> $ git init .
> Initialized empty Git repository in
> /home/wink/prgs/git/investigate-fetch-tags/.git/
> $ git add *
> $ git commit -m "Initial commit"
> [master (root-commit) 1116fdc] Initial commit
>  1 file changed, 1 insertion(+)
>  create mode 100644 abc.txt
> $ git tag v1.0.0
> $ git remote add gbenchmark git@github.com:google/benchmark
> $ git log --graph --format="%h %s %d"
> * 1116fdc Initial commit  (HEAD -> master, tag: v1.0.0)
> $ git fetch --tags gbenchmark
> warning: no common commits
> remote: Counting objects: 4400, done.
> remote: Compressing objects: 100% (15/15), done.
> remote: Total 4400 (delta 5), reused 5 (delta 3), pack-reused 4382
> Receiving objects: 100% (4400/4400), 1.33 MiB | 2.81 MiB/s, done.
> Resolving deltas: 100% (2863/2863), done.
> From github.com:google/benchmark
>  * [new branch]      clangtidy       -> gbenchmark/clangtidy
>  * [new branch]      iter_report     -> gbenchmark/iter_report
>  * [new branch]      master          -> gbenchmark/master
>  * [new branch]      releasing       -> gbenchmark/releasing
>  * [new branch]      reportercleanup -> gbenchmark/reportercleanup
>  * [new branch]      rmheaders       -> gbenchmark/rmheaders
>  * [new branch]      v2              -> gbenchmark/v2
>  * [new tag]         v0.0.9          -> v0.0.9
>  * [new tag]         v0.1.0          -> v0.1.0
>  t [tag update]      v1.0.0          -> v1.0.0
>  * [new tag]         v1.1.0          -> v1.1.0
>  * [new tag]         v1.2.0          -> v1.2.0
>  * [new tag]         v1.3.0          -> v1.3.0
>  * [new tag]         v1.4.0          -> v1.4.0
> $ git log --graph --format="%h %s %d"
> * 1116fdc Initial commit  (HEAD -> master)
>
> As you can see the tag on 1116fdc is gone, v1.0.0 tag has been updated
> and now its pointing to the tag in gbenchmark:
>
> $ git log -5 --graph --format="%h %s %d" v1.0.0
> *   cd525ae Merge pull request #171 from eliben/update-doc-userealtime
>  (tag: v1.0.0)
> |\
> | * c7ab1b9 Update README to mention UseRealTime for wallclock time
> measurements.
> |/
> * f662e8b Rename OS_MACOSX macro to new name BENCHMARK_OS_MACOSX. Fix #169
> *   0a1f484 Merge pull request #166 from disconnect3d/master
> |\
> | * d2917bc Fixes #165: CustomArguments ret type in README
> |/
>
> Ideally I would have liked the tags fetched from gbenchmark to have a prefix
> of gbenchmark/, like the branches have, maybe something like:
>

That would require a complete redesign of how we handle remotes. I've
proposed ideas in the past but never had time and they didn't gain
much traction.

It's a known limitation that the tags namespace can only hold a single
tag name (even if remotes differ in what that tag is). I *thought*
that the tags should not be updated after you fetch it once, but it
seems this is not the behavior we get now?

My basic idea was to fetch *all* remote refs into a
refs/<remotes-bikeshed>/<remote-name>/* such that *every* ref in a
remote can be determined by something like
"refs/tracking/origin/tags/name" instead of
"refs/remotes/origin/name", and then tags would have to be updated to
check for tags in each remote as well as locally. Additionally, you
could update the tool to warn when two remotes have the same tag at
different refs, and allow disambiguation.

Ideally, "origin/branch" should still DWIM, same for "tag" should work
unless there are conflicts.

Unfortunately, it's a pretty big change in how remotes are handled,
and I never had time to actually work towards a POC or implementation.
Mostly, we ended up on bikeshedding what the name should be now that
we can't use "refs/remotes" due to backwards compatibility. I don't
really like "tracking" as a name, but it was the best I could come up
with.

(Note, the impetus for this proposal was actually to allow easy
sharing of notes and other specialized refs).

Thanks,
Jake

> $ git fetch --tags gbenchmark
> ...
>  * [new branch]      v2              -> gbenchmark/v2
>  * [new tag]         v0.0.9          -> gbenchmark/v0.0.9
>  * [new tag]         v0.1.0          -> gbenchmark/v0.1.0
>  * [new tag]         v1.0.0          -> gbenchmark/v1.0.0
>  * [new tag]         v1.1.0          -> gbenchmark/v1.1.0
>  * [new tag]         v1.2.0          -> gbenchmark/v1.2.0
>  * [new tag]         v1.3.0          -> gbenchmark/v1.3.0
>  * [new tag]         v1.4.0          -> gbenchmark/v1.4.0
>
>
> -- Wink

Re: Fetching tags overwrites existing tags

[Junio C Hamano]

Wink Saville <wink@saville.com> writes:

> Ideally I would have liked the tags fetched from gbenchmark to have a prefix
> of gbenchmark/, like the branches have, maybe something like:
>
> $ git fetch --tags gbenchmark
> ...
>  * [new branch]      v2              -> gbenchmark/v2
>  * [new tag]         v0.0.9          -> gbenchmark/v0.0.9
>  * [new tag]         v0.1.0          -> gbenchmark/v0.1.0
>  * [new tag]         v1.0.0          -> gbenchmark/v1.0.0
>  * [new tag]         v1.1.0          -> gbenchmark/v1.1.0
>  * [new tag]         v1.2.0          -> gbenchmark/v1.2.0
>  * [new tag]         v1.3.0          -> gbenchmark/v1.3.0
>  * [new tag]         v1.4.0          -> gbenchmark/v1.4.0

The tag namespace (refs/tags/) is considered a shared resource (I am
not saying that that is the only valid world model---I am merely
explaining why things are like they are), hence the auto-following
tags will bring them to refs/tags/ (and I do not think there is no
way to configure auto-following to place them elsewhere).

But you could configure things yourself.

    $ git init victim && cd victim
    $ git remote add origin ../git.git
    $ git config --add remote.origin.fetch \
      "+refs/tags/*:refs/remote-tags/origin/*"
    $ tail -n 4 .git/config
    [remote "origin"]
	url = ../git.git/
	fetch = +refs/heads/*:refs/remotes/origin/*
	fetch = +refs/tags/*:refs/remote-tags/origin/*
    $ git fetch --no-tags

The "--no-tags" option serves to decline the auto-following tags to
refs/tags/ hierarchy; once your repository is configured this way,
your initial and subsequent "git fetch" will copy refs it finds in
refs/tags/ hierarchy over there to your refs/remote-tags/origin/
hierarchy locally.

Re: Fetching tags overwrites existing tags

[Jacob Keller]

On Tue, Apr 24, 2018 at 5:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Wink Saville <wink@saville.com> writes:
>
>> Ideally I would have liked the tags fetched from gbenchmark to have a prefix
>> of gbenchmark/, like the branches have, maybe something like:
>>
>> $ git fetch --tags gbenchmark
>> ...
>>  * [new branch]      v2              -> gbenchmark/v2
>>  * [new tag]         v0.0.9          -> gbenchmark/v0.0.9
>>  * [new tag]         v0.1.0          -> gbenchmark/v0.1.0
>>  * [new tag]         v1.0.0          -> gbenchmark/v1.0.0
>>  * [new tag]         v1.1.0          -> gbenchmark/v1.1.0
>>  * [new tag]         v1.2.0          -> gbenchmark/v1.2.0
>>  * [new tag]         v1.3.0          -> gbenchmark/v1.3.0
>>  * [new tag]         v1.4.0          -> gbenchmark/v1.4.0
>
> The tag namespace (refs/tags/) is considered a shared resource (I am
> not saying that that is the only valid world model---I am merely
> explaining why things are like they are), hence the auto-following
> tags will bring them to refs/tags/ (and I do not think there is no
> way to configure auto-following to place them elsewhere).
>
> But you could configure things yourself.
>
>     $ git init victim && cd victim
>     $ git remote add origin ../git.git
>     $ git config --add remote.origin.fetch \
>       "+refs/tags/*:refs/remote-tags/origin/*"
>     $ tail -n 4 .git/config
>     [remote "origin"]
>         url = ../git.git/
>         fetch = +refs/heads/*:refs/remotes/origin/*
>         fetch = +refs/tags/*:refs/remote-tags/origin/*
>     $ git fetch --no-tags
>
> The "--no-tags" option serves to decline the auto-following tags to
> refs/tags/ hierarchy; once your repository is configured this way,
> your initial and subsequent "git fetch" will copy refs it finds in
> refs/tags/ hierarchy over there to your refs/remote-tags/origin/
> hierarchy locally.

It should be noted, that remote-tags would not be integrated into "git
tag" or many other places in git commands, so it may be significantly
less visible.

Thanks,
Jake

Re: Fetching tags overwrites existing tags

[Wink Saville]

On Tue, Apr 24, 2018 at 5:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Wink Saville <wink@saville.com> writes:
>
>> Ideally I would have liked the tags fetched from gbenchmark to have a prefix
>> of gbenchmark/, like the branches have, maybe something like:
>>
>> $ git fetch --tags gbenchmark
>> ...
>>  * [new branch]      v2              -> gbenchmark/v2
>>  * [new tag]         v0.0.9          -> gbenchmark/v0.0.9
>>  * [new tag]         v0.1.0          -> gbenchmark/v0.1.0
>>  * [new tag]         v1.0.0          -> gbenchmark/v1.0.0
>>  * [new tag]         v1.1.0          -> gbenchmark/v1.1.0
>>  * [new tag]         v1.2.0          -> gbenchmark/v1.2.0
>>  * [new tag]         v1.3.0          -> gbenchmark/v1.3.0
>>  * [new tag]         v1.4.0          -> gbenchmark/v1.4.0
>
> The tag namespace (refs/tags/) is considered a shared resource (I am
> not saying that that is the only valid world model---I am merely
> explaining why things are like they are), hence the auto-following
> tags will bring them to refs/tags/ (and I do not think there is no
> way to configure auto-following to place them elsewhere).
>
> But you could configure things yourself.
>
>     $ git init victim && cd victim
>     $ git remote add origin ../git.git
>     $ git config --add remote.origin.fetch \
>       "+refs/tags/*:refs/remote-tags/origin/*"
>     $ tail -n 4 .git/config
>     [remote "origin"]
>         url = ../git.git/
>         fetch = +refs/heads/*:refs/remotes/origin/*
>         fetch = +refs/tags/*:refs/remote-tags/origin/*
>     $ git fetch --no-tags
>
> The "--no-tags" option serves to decline the auto-following tags to
> refs/tags/ hierarchy; once your repository is configured this way,
> your initial and subsequent "git fetch" will copy refs it finds in
> refs/tags/ hierarchy over there to your refs/remote-tags/origin/
> hierarchy locally.

Interesting that kinda works, what about teaching git-remote to
understand a "--prefix-tags" option which would create the
"fetch = refs/tags/*:refs/remote-tags/origin" entry. And teach
git-fetch to use that entry if it exists and not require the "--no-tags"?

Of course I'm sure there are "lots" of other things to change, doing
a search for "remotes/" gives the following:

$ find . -type f -name '*.c' | xargs grep '\bremotes/' | sort -uk1,1
./builtin/branch.c: fmt = "refs/remotes/%s";
./builtin/checkout.c: skip_prefix(argv0, "remotes/", &argv0);
./builtin/clone.c: strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin);
./builtin/describe.c:     !skip_prefix(path, "refs/remotes/", &path_to_match)) {
./builtin/fast-export.c: "refs/remotes/",
./builtin/fetch.c: else if (starts_with(rm->name, "refs/remotes/")) {
./builtin/merge.c: if (starts_with(found_ref, "refs/remotes/")) {
./builtin/pull.c: * refs/heads/<branch_name> to
refs/remotes/<remote_name>/<branch_name>.
./builtin/remote.c: strbuf_addf(tmp, "refs/heads/%s:refs/remotes/%s/%s",
./builtin/rev-parse.c: handle_ref_opt(arg, "refs/remotes/");
./builtin/show-branch.c: if (!starts_with(refname, "refs/remotes/"))
./contrib/examples/builtin-fetch--tool.c: else if
(!strncmp(remote_name, "refs/remotes/", 13)) {
./help.c: if (skip_prefix(refname, "refs/remotes/", &remote) &&
./log-tree.c: else if (starts_with(refname, "refs/remotes/"))
./ref-filter.c:        skip_prefix(refname, "refs/remotes/", &refname) ||
./refs.c: return refs_for_each_ref_in(refs, "refs/remotes/", fn, cb_data);
./remote.c: FILE *f = fopen_or_warn(git_path("remotes/%s", remote->name), "r");
./revision.c: for_each_glob_ref_in(handle_one_ref, optarg,
"refs/remotes/", &cb);
./sha1_name.c:     starts_with(refname, "refs/remotes/"))
./wt-status.c: skip_prefix(from, "refs/remotes/", &from);

-- wink

Re: Fetching tags overwrites existing tags

[Wink Saville]

I've tried to teach 'git remote add' the --prefix-tags option using the
technique Junio provided. At moment it is PR #486 on github [1]
and I'd love some comments on whether or not this the right direction
for fetching tags and putting them in the branches namespace.

-- Wink

[1] https://github.com/git/git/pull/486

Re: Fetching tags overwrites existing tags

[Junio C Hamano]

Wink Saville <wink@saville.com> writes:

> I've tried to teach 'git remote add' the --prefix-tags option using the
> technique Junio provided. At moment it is PR #486 on github [1]
> and I'd love some comments on whether or not this the right direction
> for fetching tags and putting them in the branches namespace.
>
> -- Wink
>
> [1] https://github.com/git/git/pull/486

FWIW, here is how that pull/486/head looks like.

-- >8 --

From: Wink Saville <wink@saville.com>
Date: Thu, 26 Apr 2018 09:56:11 -0700
Subject: [PATCH] Teach remote add the --prefix-tags option

When --prefix-tags is passed to `git remote add` the tagopt is set to
--prefix-tags and a second fetch line is added so tags are placed in
the branches namespace.

For example:
  $ git remote add -f --prefix-tags gbenchmark git@github.com:google/benchmark
  Updating gbenchmark
  warning: no common commits
  remote: Counting objects: 4406, done.
  remote: Compressing objects: 100% (18/18), done.
  remote: Total 4406 (delta 7), reused 13 (delta 6), pack-reused 4382
  Receiving objects: 100% (4406/4406), 1.34 MiB | 7.46 MiB/s, done.
  Resolving deltas: 100% (2865/2865), done.
  From github.com:google/benchmark
   * [new branch]      clangtidy       -> gbenchmark/clangtidy
   * [new branch]      iter_report     -> gbenchmark/iter_report
   * [new branch]      master          -> gbenchmark/master
   * [new branch]      releasing       -> gbenchmark/releasing
   * [new branch]      reportercleanup -> gbenchmark/reportercleanup
   * [new branch]      rmheaders       -> gbenchmark/rmheaders
   * [new branch]      v2              -> gbenchmark/v2
   * [new tag]         v0.0.9          -> refs/remote-tags/gbenchmark/v0.0.9
   * [new tag]         v0.1.0          -> refs/remote-tags/gbenchmark/v0.1.0
   * [new tag]         v1.0.0          -> refs/remote-tags/gbenchmark/v1.0.0
   * [new tag]         v1.1.0          -> refs/remote-tags/gbenchmark/v1.1.0
   * [new tag]         v1.2.0          -> refs/remote-tags/gbenchmark/v1.2.0
   * [new tag]         v1.3.0          -> refs/remote-tags/gbenchmark/v1.3.0
   * [new tag]         v1.4.0          -> refs/remote-tags/gbenchmark/v1.4.0

And the .git/config remote "gbenchmark" section looks like:
  [remote "gbenchmark"]
    url = git@github.com:google/benchmark
    fetch = +refs/heads/*:refs/remotes/gbenchmark/*
    fetch = +refs/tags/*:refs/remote-tags/gbenchmark/*
    tagopt = --prefix-tags
---
 Documentation/git-remote.txt |  8 ++++--
 builtin/remote.c             | 47 +++++++++++++++++++++++++++++++++---
 remote.c                     |  2 ++
 3 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4feddc0293..cdfd24e2ea 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git remote' [-v | --verbose]
-'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=<fetch|push>] <name> <url>
+'git remote add' [-t <branch>] [-m <master>] [-f] [--prefix-tags | --tags | --no-tags] [--mirror=<fetch|push>] <name> <url>
 'git remote rename' <old> <new>
 'git remote remove' <name>
 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)
@@ -54,7 +54,11 @@ With `-f` option, `git fetch <name>` is run immediately after
 the remote information is set up.
 +
 With `--tags` option, `git fetch <name>` imports every tag from the
-remote repository.
+remote repository to refs/tags, use --prefix-tags to import them
+to refs/remote-tags/<name>/<tag>.
++
+With `--prefix-tags` option, `git fetch <name>` imports every tag from the
+remote repository to refs/remote-tags/<name>/<tag>.
 +
 With `--no-tags` option, `git fetch <name>` does not import tags from
 the remote repository.
diff --git a/builtin/remote.c b/builtin/remote.c
index 805ffc05cd..75813eeaa3 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -11,7 +11,7 @@
 
 static const char * const builtin_remote_usage[] = {
 	N_("git remote [-v | --verbose]"),
-	N_("git remote add [-t <branch>] [-m <master>] [-f] [--tags | --no-tags] [--mirror=<fetch|push>] <name> <url>"),
+	N_("git remote add [-t <branch>] [-m <master>] [-f] [--prefix-tags | --tags | --no-tags] [--mirror=<fetch|push>] <name> <url>"),
 	N_("git remote rename <old> <new>"),
 	N_("git remote remove <name>"),
 	N_("git remote set-head <name> (-a | --auto | -d | --delete | <branch>)"),
@@ -101,7 +101,8 @@ static int fetch_remote(const char *name)
 enum {
 	TAGS_UNSET = 0,
 	TAGS_DEFAULT = 1,
-	TAGS_SET = 2
+	TAGS_SET = 2,
+	TAGS_SET_PREFIX = 3
 };
 
 #define MIRROR_NONE 0
@@ -123,6 +124,16 @@ static void add_branch(const char *key, const char *branchname,
 	git_config_set_multivar(key, tmp->buf, "^$", 0);
 }
 
+static void add_remote_tags(const char *key, const char *branchname,
+		       const char *remotename, struct strbuf *tmp)
+{
+	strbuf_reset(tmp);
+	strbuf_addch(tmp, '+');
+	strbuf_addf(tmp, "refs/tags/%s:refs/remote-tags/%s/%s",
+				branchname, remotename, branchname);
+	git_config_set_multivar(key, tmp->buf, "^$", 0);
+}
+
 static const char mirror_advice[] =
 N_("--mirror is dangerous and deprecated; please\n"
    "\t use --mirror=fetch or --mirror=push instead");
@@ -161,6 +172,9 @@ static int add(int argc, const char **argv)
 		OPT_SET_INT(0, "tags", &fetch_tags,
 			    N_("import all tags and associated objects when fetching"),
 			    TAGS_SET),
+		OPT_SET_INT(0, "prefix-tags", &fetch_tags,
+			    N_("import all tags and associated objects when fetching and prefix with <name>"),
+          TAGS_SET_PREFIX),
 		OPT_SET_INT(0, NULL, &fetch_tags,
 			    N_("or do not fetch any tag at all (--no-tags)"), TAGS_UNSET),
 		OPT_STRING_LIST('t', "track", &track, N_("branch"),
@@ -215,10 +229,35 @@ static int add(int argc, const char **argv)
 	}
 
 	if (fetch_tags != TAGS_DEFAULT) {
+		if (fetch_tags == TAGS_SET_PREFIX) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "remote.%s.fetch", name);
+			if (track.nr == 0)
+				string_list_append(&track, "*");
+			for (i = 0; i < track.nr; i++) {
+				add_remote_tags(buf.buf, track.items[i].string,
+						name, &buf2);
+			}
+		}
+
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "remote.%s.tagopt", name);
-		git_config_set(buf.buf,
-			       fetch_tags == TAGS_SET ? "--tags" : "--no-tags");
+		char* config_val = NULL;
+		switch (fetch_tags) {
+		case TAGS_UNSET:
+			config_val = "--no-tags";
+			break;
+		case TAGS_SET:
+			config_val = "--tags";
+			break;
+		case TAGS_SET_PREFIX:
+			config_val = "--prefix-tags";
+			break;
+		default:
+			die(_("Unexpected TAGS enum %d"), fetch_tags);
+			break;
+		}
+		git_config_set(buf.buf, config_val);
 	}
 
 	if (fetch && fetch_remote(name))
diff --git a/remote.c b/remote.c
index 91eb010ca9..f383ce3cdf 100644
--- a/remote.c
+++ b/remote.c
@@ -447,6 +447,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 			remote->fetch_tags = -1;
 		else if (!strcmp(value, "--tags"))
 			remote->fetch_tags = 2;
+		else if (!strcmp(value, "--prefix-tags"))
+			remote->fetch_tags = -1; // A fetch for refs/tags is present so tags are retrieved
 	} else if (!strcmp(subkey, "proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
-- 
2.17.0-391-g1f1cddd558

Re: Fetching tags overwrites existing tags

[Junio C Hamano]

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

> Wink Saville <wink@saville.com> writes:
>
>> I've tried to teach 'git remote add' the --prefix-tags option using the
>> technique Junio provided. At moment it is PR #486 on github [1]
>> and I'd love some comments on whether or not this the right direction
>> for fetching tags and putting them in the branches namespace.
>>
>> -- Wink
>>
>> [1] https://github.com/git/git/pull/486
>
> FWIW, here is how that pull/486/head looks like.
>
> -- >8 --
>
> From: Wink Saville <wink@saville.com>
> Date: Thu, 26 Apr 2018 09:56:11 -0700
> Subject: [PATCH] Teach remote add the --prefix-tags option
>
> When --prefix-tags is passed to `git remote add` the tagopt is set to
> --prefix-tags and a second fetch line is added so tags are placed in
> the branches namespace.

When I hear "branches namespace", what comes to my mind is refs/heads/
or perhaps refs/remotes/*/.  "... are placed in a separate hierarchy
per remote" or something, perhaps?

>
> ...
> And the .git/config remote "gbenchmark" section looks like:
>   [remote "gbenchmark"]
>     url = git@github.com:google/benchmark
>     fetch = +refs/heads/*:refs/remotes/gbenchmark/*
>     fetch = +refs/tags/*:refs/remote-tags/gbenchmark/*
>     tagopt = --prefix-tags
> ---

Missing sign-off ;-)

> +static void add_remote_tags(const char *key, const char *branchname,
> +		       const char *remotename, struct strbuf *tmp)
> +{
> +	strbuf_reset(tmp);
> +	strbuf_addch(tmp, '+');
> +	strbuf_addf(tmp, "refs/tags/%s:refs/remote-tags/%s/%s",
> +				branchname, remotename, branchname);

With "+refs/tags/%s:refs/remote-tags/%s/%s", combine addch/addf into
one, perhaps?

> +	git_config_set_multivar(key, tmp->buf, "^$", 0);
> +}

Calling the second parameter "branchname" makes little sense, I
would think.  Practically, you would call this at most once with its
second parameter set to '*', and even if the second parameter is not
a wildcard/asterisk, it would be a tagname.


>  static const char mirror_advice[] =
>  N_("--mirror is dangerous and deprecated; please\n"
>     "\t use --mirror=fetch or --mirror=push instead");
> @@ -161,6 +172,9 @@ static int add(int argc, const char **argv)
>  		OPT_SET_INT(0, "tags", &fetch_tags,
>  			    N_("import all tags and associated objects when fetching"),
>  			    TAGS_SET),
> +		OPT_SET_INT(0, "prefix-tags", &fetch_tags,
> +			    N_("import all tags and associated objects when fetching and prefix with <name>"),
> +          TAGS_SET_PREFIX),

Funny indent.  Use monospaced font in your editor, set tab width to
8 and align, imitating how the above OPT_SET_INT() item does for
TAGS_SET.

> @@ -215,10 +229,35 @@ static int add(int argc, const char **argv)
>  	}
>  
>  	if (fetch_tags != TAGS_DEFAULT) {
> +		if (fetch_tags == TAGS_SET_PREFIX) {
> +			strbuf_reset(&buf);
> +			strbuf_addf(&buf, "remote.%s.fetch", name);
> +			if (track.nr == 0)
> +				string_list_append(&track, "*");
> +			for (i = 0; i < track.nr; i++) {
> +				add_remote_tags(buf.buf, track.items[i].string,
> +						name, &buf2);
> +			}

The "track" thing is made incompatible with anything but mirror in
early part of this function (outside the precontext).  I highly
suspect that --prefix-tags does *not* make sense when mirroring.

Hence (1) we should detect and error out when --prefix-tags is used
with mirror fetch near where we do the same for track used without
mirror fetch already, (2) detect and error out when --prefix-tags is
used with track, and (3) add "+refs/tags/*:refs/remote-tags/$name/*"
just once without paying attention to track here.  We may not even
want add_remote_tags() helper function if we go that route.

> +		}
> +
>  		strbuf_reset(&buf);
>  		strbuf_addf(&buf, "remote.%s.tagopt", name);
> -		git_config_set(buf.buf,
> -			       fetch_tags == TAGS_SET ? "--tags" : "--no-tags");
> +		char* config_val = NULL;

decl-after-statement.  Also "char *var", not "char* var".

> +		switch (fetch_tags) {
> +		case TAGS_UNSET:
> +			config_val = "--no-tags";
> +			break;
> +		case TAGS_SET:
> +			config_val = "--tags";
> +			break;
> +		case TAGS_SET_PREFIX:
> +			config_val = "--prefix-tags";
> +			break;
> +		default:
> +			die(_("Unexpected TAGS enum %d"), fetch_tags);
> +			break;
> +		}
> +		git_config_set(buf.buf, config_val);
>  	}
>  
>  	if (fetch && fetch_remote(name))
> diff --git a/remote.c b/remote.c
> index 91eb010ca9..f383ce3cdf 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -447,6 +447,8 @@ static int handle_config(const char *key, const char *value, void *cb)
>  			remote->fetch_tags = -1;
>  		else if (!strcmp(value, "--tags"))
>  			remote->fetch_tags = 2;
> +		else if (!strcmp(value, "--prefix-tags"))
> +			remote->fetch_tags = -1; // A fetch for refs/tags is present so tags are retrieved

We are old fashioned and do not use // comments, but more
importantly it is not clear what this comment is trying to
say, at least to me.

>  	} else if (!strcmp(subkey, "proxy")) {
>  		return git_config_string((const char **)&remote->http_proxy,
>  					 key, value);

[RFC PATCH v2] Teach remote add the --prefix-tags option

[Wink Saville]

When --prefix-tags is passed to `git remote add` the tagopt is set to
--prefix-tags and a second fetch line is added so tags are placed in
a separate hierarchy per remote.

For example:
  $ git remote add -f --prefix-tags gbenchmark git@github.com:google/benchmark
  Updating gbenchmark
  warning: no common commits
  remote: Counting objects: 4406, done.
  remote: Compressing objects: 100% (18/18), done.
  remote: Total 4406 (delta 7), reused 13 (delta 6), pack-reused 4382
  Receiving objects: 100% (4406/4406), 1.34 MiB | 7.58 MiB/s, done.
  Resolving deltas: 100% (2865/2865), done.
  From github.com:google/benchmark
   * [new branch]      clangtidy       -> gbenchmark/clangtidy
   * [new branch]      iter_report     -> gbenchmark/iter_report
   * [new branch]      master          -> gbenchmark/master
   * [new branch]      releasing       -> gbenchmark/releasing
   * [new branch]      reportercleanup -> gbenchmark/reportercleanup
   * [new branch]      rmheaders       -> gbenchmark/rmheaders
   * [new branch]      v2              -> gbenchmark/v2
   * [new tag]         v0.0.9          -> tags/gbenchmark/v0.0.9
   * [new tag]         v0.1.0          -> tags/gbenchmark/v0.1.0
   * [new tag]         v1.0.0          -> tags/gbenchmark/v1.0.0
   * [new tag]         v1.1.0          -> tags/gbenchmark/v1.1.0
   * [new tag]         v1.2.0          -> tags/gbenchmark/v1.2.0
   * [new tag]         v1.3.0          -> tags/gbenchmark/v1.3.0
   * [new tag]         v1.4.0          -> tags/gbenchmark/v1.4.0

And the .git/config remote "gbenchmark" section looks like:
  [remote "gbenchmark"]
    url = git@github.com:google/benchmark
    fetch = +refs/heads/*:refs/remotes/gbenchmark/*
    fetch = +refs/tags/*:refs/remotes/tags/gbenchmark/*
    tagopt = --prefix-tags

Based on a solution proposed by Junio on the email list [1]

[1]: https://public-inbox.org/git/xmqqbme51rgn.fsf@gitster-ct.c.googlers.com/T/#me7f7f153b8ba742c0dc48d8ec79c280c9682d32e

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jacob Keller <jacob.keller@gmail.com>
Signed-off-by: Wink Saville <wink@saville.com>
---
 Documentation/git-remote.txt |  8 +++++--
 builtin/remote.c             | 42 ++++++++++++++++++++++++++++++++----
 remote.c                     |  2 ++
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4feddc0293..c97bf29d46 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git remote' [-v | --verbose]
-'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=<fetch|push>] <name> <url>
+'git remote add' [-t <branch>] [-m <master>] [-f] [--prefix-tags | --tags | --no-tags] [--mirror=<fetch|push>] <name> <url>
 'git remote rename' <old> <new>
 'git remote remove' <name>
 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)
@@ -54,7 +54,11 @@ With `-f` option, `git fetch <name>` is run immediately after
 the remote information is set up.
 +
 With `--tags` option, `git fetch <name>` imports every tag from the
-remote repository.
+remote repository to refs/tags, use --prefix-tags to import them
+to refs/remotes/tags/<name>/<tag>.
++
+With `--prefix-tags` option, `git fetch <name>` imports every tag from the
+remote repository to refs/remotes/tags/<name>/<tag>.
 +
 With `--no-tags` option, `git fetch <name>` does not import tags from
 the remote repository.
diff --git a/builtin/remote.c b/builtin/remote.c
index 805ffc05cd..39de50bdd6 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -11,7 +11,7 @@
 
 static const char * const builtin_remote_usage[] = {
 	N_("git remote [-v | --verbose]"),
-	N_("git remote add [-t <branch>] [-m <master>] [-f] [--tags | --no-tags] [--mirror=<fetch|push>] <name> <url>"),
+	N_("git remote add [-t <branch>] [-m <master>] [-f] [--prefix-tags | --tags | --no-tags] [--mirror=<fetch|push>] <name> <url>"),
 	N_("git remote rename <old> <new>"),
 	N_("git remote remove <name>"),
 	N_("git remote set-head <name> (-a | --auto | -d | --delete | <branch>)"),
@@ -101,7 +101,8 @@ static int fetch_remote(const char *name)
 enum {
 	TAGS_UNSET = 0,
 	TAGS_DEFAULT = 1,
-	TAGS_SET = 2
+	TAGS_SET = 2,
+	TAGS_SET_PREFIX = 3
 };
 
 #define MIRROR_NONE 0
@@ -123,6 +124,14 @@ static void add_branch(const char *key, const char *branchname,
 	git_config_set_multivar(key, tmp->buf, "^$", 0);
 }
 
+static void add_remote_tags(const char *key, const char *remotename,
+			    struct strbuf *tmp)
+{
+	strbuf_reset(tmp);
+	strbuf_addf(tmp, "+refs/tags/*:refs/remotes/tags/%s/*", remotename);
+	git_config_set_multivar(key, tmp->buf, "^$", 0);
+}
+
 static const char mirror_advice[] =
 N_("--mirror is dangerous and deprecated; please\n"
    "\t use --mirror=fetch or --mirror=push instead");
@@ -161,6 +170,9 @@ static int add(int argc, const char **argv)
 		OPT_SET_INT(0, "tags", &fetch_tags,
 			    N_("import all tags and associated objects when fetching"),
 			    TAGS_SET),
+		OPT_SET_INT(0, "prefix-tags", &fetch_tags,
+			    N_("import all tags and associated objects when fetching and prefix with <name>"),
+			    TAGS_SET_PREFIX),
 		OPT_SET_INT(0, NULL, &fetch_tags,
 			    N_("or do not fetch any tag at all (--no-tags)"), TAGS_UNSET),
 		OPT_STRING_LIST('t', "track", &track, N_("branch"),
@@ -182,6 +194,8 @@ static int add(int argc, const char **argv)
 		die(_("specifying a master branch makes no sense with --mirror"));
 	if (mirror && !(mirror & MIRROR_FETCH) && track.nr)
 		die(_("specifying branches to track makes sense only with fetch mirrors"));
+	if (mirror && (fetch_tags == TAGS_SET_PREFIX))
+		die(_("specifying a --prefix-tags makes no sense with --mirror"));
 
 	name = argv[0];
 	url = argv[1];
@@ -215,10 +229,30 @@ static int add(int argc, const char **argv)
 	}
 
 	if (fetch_tags != TAGS_DEFAULT) {
+		if (fetch_tags == TAGS_SET_PREFIX) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "remote.%s.fetch", name);
+			add_remote_tags(buf.buf, name, &buf2);
+		}
+
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "remote.%s.tagopt", name);
-		git_config_set(buf.buf,
-			       fetch_tags == TAGS_SET ? "--tags" : "--no-tags");
+		char *config_val = NULL;
+		switch (fetch_tags) {
+		case TAGS_UNSET:
+			config_val = "--no-tags";
+			break;
+		case TAGS_SET:
+			config_val = "--tags";
+			break;
+		case TAGS_SET_PREFIX:
+			config_val = "--prefix-tags";
+			break;
+		default:
+			die(_("Unexpected TAGS enum %d"), fetch_tags);
+			break;
+		}
+		git_config_set(buf.buf, config_val);
 	}
 
 	if (fetch && fetch_remote(name))
diff --git a/remote.c b/remote.c
index 91eb010ca9..65dd974868 100644
--- a/remote.c
+++ b/remote.c
@@ -447,6 +447,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 			remote->fetch_tags = -1;
 		else if (!strcmp(value, "--tags"))
 			remote->fetch_tags = 2;
+		else if (!strcmp(value, "--prefix-tags"))
+			remote->fetch_tags = -1;
 	} else if (!strcmp(subkey, "proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
-- 
2.17.0.392.g28baa87d09.dirty

Re: Fetching tags overwrites existing tags

[Wink Saville]

On Thu, Apr 26, 2018 at 4:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>
> Hence (1) we should detect and error out when --prefix-tags is used
> with mirror fetch near where we do the same for track used without
> mirror fetch already, (2) detect and error out when --prefix-tags is
> used with track, and (3) add "+refs/tags/*:refs/remote-tags/$name/*"
> just once without paying attention to track here.  We may not even
> want add_remote_tags() helper function if we go that route.
>

I've replied to the thread using format-email/send-email with the
subject: "[RFC PATCH v2] Teach remote add the --prefix-tags option",
but I misspelled Junio's email address :(

I've tried to address the issues pointed out by Junio. But I've choosen
not to do "(2) detect and error out when --prefix-tags is used with track".
My thinking is tags are independent of tracking and it seems reasonable
that they sould be included if requested. If I'm wrong I'll certainly fix it.

The other change was rather than using ""+refs/tags/*:refs/remote-tags/$name/*"
I've changed it to "+refs/tags/*:refs/remote/tags/$name/*" which seems cleaner.
Again, if remote-tags is preferred I'll change it back.

One other question, I'm not sure "--prefix-tags" is the best name for
the option,
maybe "--sub-tags" or "--nested-tags" or ...

-- Wink

Re: Fetching tags overwrites existing tags

[Bryan Turner]

On Fri, Apr 27, 2018 at 12:08 PM, Wink Saville <wink@saville.com> wrote:
>
> The other change was rather than using ""+refs/tags/*:refs/remote-tags/$name/*"
> I've changed it to "+refs/tags/*:refs/remote/tags/$name/*" which seems cleaner.
> Again, if remote-tags is preferred I'll change it back.


From looking at the code, it looks like you mean
"+refs/tags/*:refs/remotes/tags/$name/*".

The issue with that approach is that it collides with a remote named
"tags". "refs/remote-tags", on the other hand, represents a new-to-Git
path, one that won't already be in use by any other standard
functionality. That seems like a better approach than hoping no one
out there will call one of their remotes "tags".

Bryan

Re: Fetching tags overwrites existing tags

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 24, 2018 at 9:57 PM, Wink Saville <wink@saville.com> wrote:
> If have a repository with a tag "v1.0.0" and I add a remote repository
> which also has a tag "v1.0.0" tag is overwritten.

I feel like this thread has gotten somewhat side-tracked by the valid
discussion about whether we should have remote tracking tags, but the
much easier thing to fix is that the "+" prefix for refs/tags/* means
nothing.

I noticed this when working on fetch.pruneTags in the last release,
but didn't dig further.

I.e. if you clone git.git and update "master" and a tag:

$ git fetch origin 'refs/heads/*:refs/heads/*' --dry-run
From github.com:git/git
 ! [rejected]              master     -> master  (non-fast-forward)
$ git fetch origin '+refs/heads/*:refs/heads/*' --dry-run
From github.com:git/git
 + 969e05fae2...1f1cddd558 master     -> master  (forced update)

Here "+" does the right thing, but then:

$ git fetch origin 'refs/tags/*:refs/tags/*' --dry-run
From github.com:git/git
 t [tag update]            v2.17.0    -> v2.17.0
$ git fetch origin '+refs/tags/*:refs/tags/*' --dry-run
From github.com:git/git
 t [tag update]            v2.17.0    -> v2.17.0

Here the former shouldn't be clobbering the existing tag.

Re: Fetching tags overwrites existing tags

[Jacob Keller]

On Fri, Apr 27, 2018 at 12:08 PM, Wink Saville <wink@saville.com> wrote:
> On Thu, Apr 26, 2018 at 4:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>
>> Hence (1) we should detect and error out when --prefix-tags is used
>> with mirror fetch near where we do the same for track used without
>> mirror fetch already, (2) detect and error out when --prefix-tags is
>> used with track, and (3) add "+refs/tags/*:refs/remote-tags/$name/*"
>> just once without paying attention to track here.  We may not even
>> want add_remote_tags() helper function if we go that route.
>>
>
> I've replied to the thread using format-email/send-email with the
> subject: "[RFC PATCH v2] Teach remote add the --prefix-tags option",
> but I misspelled Junio's email address :(
>
> I've tried to address the issues pointed out by Junio. But I've choosen
> not to do "(2) detect and error out when --prefix-tags is used with track".
> My thinking is tags are independent of tracking and it seems reasonable
> that they sould be included if requested. If I'm wrong I'll certainly fix it.
>
> The other change was rather than using ""+refs/tags/*:refs/remote-tags/$name/*"
> I've changed it to "+refs/tags/*:refs/remote/tags/$name/*" which seems cleaner.
> Again, if remote-tags is preferred I'll change it back.

The only main concern I have with "remote" is that it is very similar
but not exactly the same as "remotes". Unfortunately, it is not
possible in *every* circumstance to use remotes.

Personally, I'd prefer we used "refs/remote/<remote-name>/tags" rather
than "refs/remote/tags/<remote-name>", and possibly plan to migrate
from refs/remotes/<remote-name>/<branches> to
refs/remote/<remote-name>/heads/<branches>

This is mostly so that future additions of things like notes,
replaces, or really *any* refs would automatically drop into
"refs/remotes/<name>/<full remote ref name with refs prefix removed>",
which is a longer term goal I've had for a while (though i haven't
been able to put much time to it at present). Historically, I proposed
using "tracking" instead of "remote", but I am ok with any name we
choose as long as it doesn't create conflicts.

Thanks,
Jake

>
> One other question, I'm not sure "--prefix-tags" is the best name for
> the option,
> maybe "--sub-tags" or "--nested-tags" or ...
>
> -- Wink

[RFC PATCH v3] Teach remote add the --remote-tags option

[Wink Saville]

When --remote-tags is passed to `git remote add` the tagopt is set to
--remote-tags and a second fetch line is added so tags are placed in
a separate hierarchy per remote.

For example:
  $ git remote add -f --remote-tags gbenchmark git@github.com:google/benchmark
  Updating gbenchmark
  warning: no common commits
  remote: Counting objects: 4406, done.
  remote: Compressing objects: 100% (18/18), done.
  remote: Total 4406 (delta 7), reused 13 (delta 6), pack-reused 4382
  Receiving objects: 100% (4406/4406), 1.34 MiB | 7.58 MiB/s, done.
  Resolving deltas: 100% (2865/2865), done.
  From github.com:google/benchmark
   * [new branch]      clangtidy       -> gbenchmark/clangtidy
   * [new branch]      iter_report     -> gbenchmark/iter_report
   * [new branch]      master          -> gbenchmark/master
   * [new branch]      releasing       -> gbenchmark/releasing
   * [new branch]      reportercleanup -> gbenchmark/reportercleanup
   * [new branch]      rmheaders       -> gbenchmark/rmheaders
   * [new branch]      v2              -> gbenchmark/v2
   * [new tag]         v0.0.9          -> refs/remote-tags/gbenchmark/v0.0.9
   * [new tag]         v0.1.0          -> refs/remote-tags/gbenchmark/v0.1.0
   * [new tag]         v1.0.0          -> refs/remote-tags/gbenchmark/v1.0.0
   * [new tag]         v1.1.0          -> refs/remote-tags/gbenchmark/v1.1.0
   * [new tag]         v1.2.0          -> refs/remote-tags/gbenchmark/v1.2.0
   * [new tag]         v1.3.0          -> refs/remote-tags/gbenchmark/v1.3.0
   * [new tag]         v1.4.0          -> refs/remote-tags/gbenchmark/v1.4.0

And the .git/config remote "gbenchmark" section looks like:
  [remote "gbenchmark"]
    url = git@github.com:google/benchmark
    fetch = +refs/heads/*:refs/remotes/gbenchmark/*
    fetch = +refs/tags/*:refs/remote-tags/gbenchmark/*
    tagopt = --remote-tags

Based on a solution proposed by Junio on the email list [1]

[1]: https://public-inbox.org/git/xmqqbme51rgn.fsf@gitster-ct.c.googlers.com/T/#me7f7f153b8ba742c0dc48d8ec79c280c9682d32e

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jacob Keller <jacob.keller@gmail.com>
Signed-off-by: Wink Saville <wink@saville.com>
---
 Documentation/git-remote.txt |  8 +++++--
 builtin/remote.c             | 44 ++++++++++++++++++++++++++++++++----
 remote.c                     |  2 ++
 3 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4feddc0293..fc983c2ff1 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git remote' [-v | --verbose]
-'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=<fetch|push>] <name> <url>
+'git remote add' [-t <branch>] [-m <master>] [-f] [--tags | --remote-tags | --no-tags] [--mirror=<fetch|push>] <name> <url>
 'git remote rename' <old> <new>
 'git remote remove' <name>
 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)
@@ -54,7 +54,11 @@ With `-f` option, `git fetch <name>` is run immediately after
 the remote information is set up.
 +
 With `--tags` option, `git fetch <name>` imports every tag from the
-remote repository.
+remote repository to refs/tags, use --remote-tags to import them
+to refs/remote-tags/<name>/<tag>.
++
+With `--remote-tags` option, `git fetch <name>` imports every tag from the
+remote repository to refs/remote-tags/<name>/<tag>.
 +
 With `--no-tags` option, `git fetch <name>` does not import tags from
 the remote repository.
diff --git a/builtin/remote.c b/builtin/remote.c
index 805ffc05cd..07832113e9 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -11,7 +11,7 @@
 
 static const char * const builtin_remote_usage[] = {
 	N_("git remote [-v | --verbose]"),
-	N_("git remote add [-t <branch>] [-m <master>] [-f] [--tags | --no-tags] [--mirror=<fetch|push>] <name> <url>"),
+	N_("git remote add [-t <branch>] [-m <master>] [-f] [--tags | --remote-tags | --no-tags] [--mirror=<fetch|push>] <name> <url>"),
 	N_("git remote rename <old> <new>"),
 	N_("git remote remove <name>"),
 	N_("git remote set-head <name> (-a | --auto | -d | --delete | <branch>)"),
@@ -101,7 +101,8 @@ static int fetch_remote(const char *name)
 enum {
 	TAGS_UNSET = 0,
 	TAGS_DEFAULT = 1,
-	TAGS_SET = 2
+	TAGS_SET = 2,
+	TAGS_SET_REMOTE = 3
 };
 
 #define MIRROR_NONE 0
@@ -123,6 +124,14 @@ static void add_branch(const char *key, const char *branchname,
 	git_config_set_multivar(key, tmp->buf, "^$", 0);
 }
 
+static void add_remote_tags(const char *key, const char *remotename,
+			    struct strbuf *tmp)
+{
+	strbuf_reset(tmp);
+	strbuf_addf(tmp, "+refs/tags/*:refs/remote-tags/%s/*", remotename);
+	git_config_set_multivar(key, tmp->buf, "^$", 0);
+}
+
 static const char mirror_advice[] =
 N_("--mirror is dangerous and deprecated; please\n"
    "\t use --mirror=fetch or --mirror=push instead");
@@ -161,6 +170,9 @@ static int add(int argc, const char **argv)
 		OPT_SET_INT(0, "tags", &fetch_tags,
 			    N_("import all tags and associated objects when fetching"),
 			    TAGS_SET),
+		OPT_SET_INT(0, "remote-tags", &fetch_tags,
+			    N_("import all tags and associated objects when fetching to refs/remote-tags/<name>/<tag>"),
+			    TAGS_SET_REMOTE),
 		OPT_SET_INT(0, NULL, &fetch_tags,
 			    N_("or do not fetch any tag at all (--no-tags)"), TAGS_UNSET),
 		OPT_STRING_LIST('t', "track", &track, N_("branch"),
@@ -182,6 +194,10 @@ static int add(int argc, const char **argv)
 		die(_("specifying a master branch makes no sense with --mirror"));
 	if (mirror && !(mirror & MIRROR_FETCH) && track.nr)
 		die(_("specifying branches to track makes sense only with fetch mirrors"));
+	if ((fetch_tags == TAGS_SET_REMOTE) && mirror)
+		die(_("specifying --remote-tags makes no sense with --mirror"));
+	if ((fetch_tags == TAGS_SET_REMOTE) && track.nr)
+		die(_("specifying --remote-tags makes no sense with -t or --track"));
 
 	name = argv[0];
 	url = argv[1];
@@ -215,10 +231,30 @@ static int add(int argc, const char **argv)
 	}
 
 	if (fetch_tags != TAGS_DEFAULT) {
+		if (fetch_tags == TAGS_SET_REMOTE) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "remote.%s.fetch", name);
+			add_remote_tags(buf.buf, name, &buf2);
+		}
+
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "remote.%s.tagopt", name);
-		git_config_set(buf.buf,
-			       fetch_tags == TAGS_SET ? "--tags" : "--no-tags");
+		char *config_val = NULL;
+		switch (fetch_tags) {
+		case TAGS_UNSET:
+			config_val = "--no-tags";
+			break;
+		case TAGS_SET:
+			config_val = "--tags";
+			break;
+		case TAGS_SET_REMOTE:
+			config_val = "--remote-tags";
+			break;
+		default:
+			die(_("Unexpected TAGS enum %d"), fetch_tags);
+			break;
+		}
+		git_config_set(buf.buf, config_val);
 	}
 
 	if (fetch && fetch_remote(name))
diff --git a/remote.c b/remote.c
index 91eb010ca9..86cccc0b6e 100644
--- a/remote.c
+++ b/remote.c
@@ -447,6 +447,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 			remote->fetch_tags = -1;
 		else if (!strcmp(value, "--tags"))
 			remote->fetch_tags = 2;
+		else if (!strcmp(value, "--remote-tags"))
+			remote->fetch_tags = -1;
 	} else if (!strcmp(subkey, "proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
-- 
2.17.0.392.ga7de9262c6

Re: [RFC PATCH v3] Teach remote add the --remote-tags option

[Wink Saville]

In this version I changed "+refs/tags/*:refs/remotes/tags/%s/*"
back to "+refs/tags/*:refs/remote-tags/%s/*" as Junio originally
suggested. This way don't have remotename collisions as Bryan
pointed out.

Since I don't like "--prefix-tags" I changed the option to
"--remote-tags" but obviously open to anything else.

I think jacob.keller (+ demerphq) suggestion is probably good,
but for the moment just using remote-tags as seems simplest.

Re: [RFC PATCH v3] Teach remote add the --remote-tags option

[Wink Saville]

I forgot to mention that I added the code to error out
if --remote-tags is used with "tracking" (-t | --track) or
mirror:
  +       if ((fetch_tags == TAGS_SET_REMOTE) && mirror)
  +               die(_("specifying --remote-tags makes no sense with
--mirror"));
  +       if ((fetch_tags == TAGS_SET_REMOTE) && track.nr)
  +               die(_("specifying --remote-tags makes no sense with
-t or --track"));

[PATCH 0/8] "git fetch" should not clobber existing tags without --force

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 27 2018, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Apr 24, 2018 at 9:57 PM, Wink Saville <wink@saville.com> wrote:
>> If have a repository with a tag "v1.0.0" and I add a remote repository
>> which also has a tag "v1.0.0" tag is overwritten.
>
> I feel like this thread has gotten somewhat side-tracked by the valid
> discussion about whether we should have remote tracking tags, but the
> much easier thing to fix is that the "+" prefix for refs/tags/* means
> nothing.
> [...]

This patch series implements that simpler way of digging ourselves out
of the immediate hole that we're clobbering tags by default without
the --force option.

I'm not 100% happy about this, but I think sans stuff that comes up in
review it's in principle ready for inclusion, stuff I wished I'd done
but have left for later:

 * Write a gitrefspec(5) man page, now we have how they work, and how
   they work on push/pull scattered over two docs, which before this
   are in conflict with one another.

 * Have much more exhaustive tests, I started trying to integrate this
   with the much more exhaustive tag pruning tests in my
   https://github.com/avar/git/tree/refspec-support-+-in-tags but gave
   up because the various interaction with those tests is messy,
   e.g. if we fail a tag update we don't prune as the existing tests
   assert, because the whole ref transaction fails.

 * 05/08 notes how the semantics of whether something needs a --force
   are really confusing because the rules are different depending on
   the ref namespace you're pushing into. We should probably build on
   top of this and e.g. refuse to clobber tags outside of refs/tags/*.

 * Should we do better to mitigate this breaking stuff for existing
   users who really are expecting their tags to be clobbered? Maybe by
   adding a --force-tags option (which wouldn't clobber branches), or
   have some config option either enable the old behavior, or make
   this opt-in?

Ævar Arnfjörð Bjarmason (8):
  push tests: remove redundant 'git push' invocation
  push tests: fix logic error in "push" test assertion
  push tests: add more testing for forced tag pushing
  push tests: assert re-pushing annotated tags
  push doc: correct lies about how push refspecs work
  fetch tests: correct a comment "remove it" -> "remove them"
  fetch tests: add a test clobbering tag behavior
  fetch: stop clobbering existing tags without --force

 Documentation/fetch-options.txt    | 15 ++++--
 Documentation/git-push.txt         | 30 ++++++++---
 Documentation/gitrevisions.txt     |  7 +--
 Documentation/pull-fetch-param.txt | 22 +++++---
 builtin/fetch.c                    | 20 +++++---
 t/t5510-fetch.sh                   |  2 +-
 t/t5516-fetch-push.sh              | 82 ++++++++++++++++++++++--------
 t/t5612-clone-refspec.sh           |  4 +-
 8 files changed, 130 insertions(+), 52 deletions(-)

-- 
2.17.0.290.gded63e768a

[PATCH 1/8] push tests: remove redundant 'git push' invocation

[Ævar Arnfjörð Bjarmason]

Remove an invocation of 'git push' that's exactly the same as the one
on the preceding line. This was seemingly added by mistake in
dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29) and doesn't affect the result of the test, the second
"push" was a no-op as there was nothing new to push.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 82239138d5..7b5a553398 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -973,7 +973,6 @@ test_expect_success 'push requires --force to update lightweight tag' '
 		cd child1 &&
 		git tag Tag &&
 		git push ../child2 Tag &&
-		git push ../child2 Tag &&
 		>file1 &&
 		git add file1 &&
 		git commit -m "file1" &&
-- 
2.17.0.290.gded63e768a

[PATCH 2/8] push tests: fix logic error in "push" test assertion

[Ævar Arnfjörð Bjarmason]

Fix a logic error that's been here since this test was added in
dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29).

The intent of this test is to force-create a new tag pointing to
HEAD~, and then assert that pushing it doesn't work without --force.

Instead, the code was not creating a new tag at all, and then failing
to push the previous tag for the unrelated reason of providing a
refspec that doesn't make any sense.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 7b5a553398..15c8d5a734 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -979,8 +979,8 @@ test_expect_success 'push requires --force to update lightweight tag' '
 		git tag -f Tag &&
 		test_must_fail git push ../child2 Tag &&
 		git push --force ../child2 Tag &&
-		git tag -f Tag &&
-		test_must_fail git push ../child2 Tag HEAD~ &&
+		git tag -f Tag HEAD~ &&
+		test_must_fail git push ../child2 Tag &&
 		git push --force ../child2 Tag
 	)
 '
-- 
2.17.0.290.gded63e768a

[PATCH 3/8] push tests: add more testing for forced tag pushing

[Ævar Arnfjörð Bjarmason]

Improve the tests added in dbfeddb12e ("push: require force for refs
under refs/tags/", 2012-11-29) to assert that the same behavior
applies various forms other refspecs, and that "+" in a refspec will
override the "--no-force" option (but not the other way around).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 15c8d5a734..c9a2011915 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -981,7 +981,17 @@ test_expect_success 'push requires --force to update lightweight tag' '
 		git push --force ../child2 Tag &&
 		git tag -f Tag HEAD~ &&
 		test_must_fail git push ../child2 Tag &&
-		git push --force ../child2 Tag
+		git push --force ../child2 Tag &&
+		git tag -f Tag &&
+		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
+		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
+		git tag -f Tag HEAD~ &&
+		git push ../child2 "+refs/tags/*:refs/tags/*" &&
+		git tag -f Tag &&
+		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
+		git tag -f Tag HEAD~ &&
+		test_must_fail git push ../child2 tag Tag &&
+		git push --force ../child2 tag Tag
 	)
 '
 
-- 
2.17.0.290.gded63e768a

[PATCH 4/8] push tests: assert re-pushing annotated tags

[Ævar Arnfjörð Bjarmason]

Change the test that asserts that lightweight tags can only be
clobbered by a force-push to check do the same tests for annotated
tags.

There used to be less exhaustive tests for this with the code added in
40eff17999 ("push: require force for annotated tags", 2012-11-29), but
Junio removed them in 256b9d70a4 ("push: fix "refs/tags/ hierarchy
cannot be updated without --force"", 2013-01-16) while fixing some of
the behavior around tag pushing.

That change left us without any coverage asserting that pushing and
clobbering annotated tags worked as intended.  There was no reason to
suspect that the receive machinery wouldn't behave the same way with
annotated tags, but now we know for sure.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 66 ++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 29 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c9a2011915..71fc902062 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -965,35 +965,43 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 	)
 '
 
-test_expect_success 'push requires --force to update lightweight tag' '
-	mk_test testrepo heads/master &&
-	mk_child testrepo child1 &&
-	mk_child testrepo child2 &&
-	(
-		cd child1 &&
-		git tag Tag &&
-		git push ../child2 Tag &&
-		>file1 &&
-		git add file1 &&
-		git commit -m "file1" &&
-		git tag -f Tag &&
-		test_must_fail git push ../child2 Tag &&
-		git push --force ../child2 Tag &&
-		git tag -f Tag HEAD~ &&
-		test_must_fail git push ../child2 Tag &&
-		git push --force ../child2 Tag &&
-		git tag -f Tag &&
-		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
-		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
-		git tag -f Tag HEAD~ &&
-		git push ../child2 "+refs/tags/*:refs/tags/*" &&
-		git tag -f Tag &&
-		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
-		git tag -f Tag HEAD~ &&
-		test_must_fail git push ../child2 tag Tag &&
-		git push --force ../child2 tag Tag
-	)
-'
+test_force_push_tag () {
+	tag_type_description=$1
+	tag_args=$2
+
+	test_expect_success "push requires --force to update $tag_type_description" "
+		mk_test testrepo heads/master &&
+		mk_child testrepo child1 &&
+		mk_child testrepo child2 &&
+		(
+			cd child1 &&
+			git tag Tag &&
+			git push ../child2 Tag &&
+			>file1 &&
+			git add file1 &&
+			git commit -m 'file1' &&
+			git tag $tag_args Tag &&
+			test_must_fail git push ../child2 Tag &&
+			git push --force ../child2 Tag &&
+			git tag $tag_args Tag HEAD~ &&
+			test_must_fail git push ../child2 Tag &&
+			git push --force ../child2 Tag &&
+			git tag $tag_args Tag &&
+			test_must_fail git push ../child2 'refs/tags/*:refs/tags/*' &&
+			git push --force ../child2 'refs/tags/*:refs/tags/*' &&
+			git tag $tag_args Tag HEAD~ &&
+			git push ../child2 '+refs/tags/*:refs/tags/*' &&
+			git tag $tag_args Tag &&function
+			git push --no-force ../child2 '+refs/tags/*:refs/tags/*' &&
+			git tag $tag_args Tag HEAD~ &&
+			test_must_fail git push ../child2 tag Tag &&
+			git push --force ../child2 tag Tag
+		)
+	"
+}
+
+test_force_push_tag "lightweight tag" "-f"
+test_force_push_tag "annotated tag" "-f -a -m'msg'"
 
 test_expect_success 'push --porcelain' '
 	mk_empty testrepo &&
-- 
2.17.0.290.gded63e768a

[PATCH 5/8] push doc: correct lies about how push refspecs work

[Ævar Arnfjörð Bjarmason]

There's complex rules governing whether a push is allowed to take
place depending on whether we're pushing to refs/heads/*, refs/tags/*
or refs/not-that/*. See is_branch() in refs.c, and the various
assertions in refs/files-backend.c. (e.g. "trying to write non-commit
object %s to branch '%s'").

This documentation has never been quite correct, but went downhill
after dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29) when we started claiming that <dst> couldn't be a tag
object, which is incorrect. After some of the logic in that patch was
changed in 256b9d70a4 ("push: fix "refs/tags/ hierarchy cannot be
updated without --force"", 2013-01-16) the docs weren't updated, and
we've had some version of documentation that confused whether <src>
was a tag or not with whether <dst> would accept either an annotated
tag object or the commit it points to.

This makes the intro somewhat more verbose & complex, perhaps we
should have a shorter description here and split the full complexity
into a dedicated section. Very few users will find themselves needing
to e.g. push blobs or trees to refs/custom-namespace/* (or blobs or
trees at all), and that could be covered separately as an advanced
topic.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-push.txt     | 30 ++++++++++++++++++++++--------
 Documentation/gitrevisions.txt |  7 ++++---
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 5b08302fc2..806c3d8c65 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -60,8 +60,10 @@ OPTIONS[[OPTIONS]]
 	by a colon `:`, followed by the destination ref <dst>.
 +
 The <src> is often the name of the branch you would want to push, but
-it can be any arbitrary "SHA-1 expression", such as `master~4` or
-`HEAD` (see linkgit:gitrevisions[7]).
+it can be any arbitrary "SHA-1 expression" referring to a branch, such
+as `master~4` or `HEAD` (see linkgit:gitrevisions[7]). It can also
+refer to tag objects, trees or blobs if the <dst> is outside of
+`refs/heads/*`.
 +
 The <dst> tells which ref on the remote side is updated with this
 push. Arbitrary expressions cannot be used here, an actual ref must
@@ -74,12 +76,24 @@ without any `<refspec>` on the command line.  Otherwise, missing
 `:<dst>` means to update the same ref as the `<src>`.
 +
 The object referenced by <src> is used to update the <dst> reference
-on the remote side.  By default this is only allowed if <dst> is not
-a tag (annotated or lightweight), and then only if it can fast-forward
-<dst>.  By having the optional leading `+`, you can tell Git to update
-the <dst> ref even if it is not allowed by default (e.g., it is not a
-fast-forward.)  This does *not* attempt to merge <src> into <dst>.  See
-EXAMPLES below for details.
+on the remote side. Whether this is allowed depends on what where in
+`refs/*` the <dst> reference lives. The `refs/heads/*` namespace will
+only accept commit objects, and then only they can be
+fast-forwarded. The `refs/tags/*` namespace will accept any kind of
+object, but there commit objects are known as lightweight tags, and
+any changes to them and others types of objects will be
+rejected. Finally and most confusingly, it's possible to push any type
+of object to any namespace outside of `refs/{tags,heads}/*`, but these
+will be treated as branches, even in the case where a tag object is
+pushed. That tag object will be overwritten by another tag object (or
+commit!) without `--force` if the new tag happens to point to a commit
+that's a fast-forward of the commit it replaces.
++
+By having the optional leading `+`, you can tell Git to update the
+<dst> ref even if it is not allowed by its respective namespace
+clobbering rules (e.g., it is not a fast-forward. in the case of
+`refs/heads/*` updates) This does *not* attempt to merge <src> into
+<dst>.  See EXAMPLES below for details.
 +
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
 +
diff --git a/Documentation/gitrevisions.txt b/Documentation/gitrevisions.txt
index 27dec5b91d..1b79cf1634 100644
--- a/Documentation/gitrevisions.txt
+++ b/Documentation/gitrevisions.txt
@@ -19,9 +19,10 @@ walk the revision graph (such as linkgit:git-log[1]), all commits which are
 reachable from that commit. For commands that walk the revision graph one can
 also specify a range of revisions explicitly.
 
-In addition, some Git commands (such as linkgit:git-show[1]) also take
-revision parameters which denote other objects than commits, e.g. blobs
-("files") or trees ("directories of files").
+In addition, some Git commands (such as linkgit:git-show[1] and
+linkgit:git-push[1]) can also take revision parameters which denote
+other objects than commits, e.g. blobs ("files") or trees
+("directories of files").
 
 include::revisions.txt[]
 
-- 
2.17.0.290.gded63e768a

[PATCH 6/8] fetch tests: correct a comment "remove it" -> "remove them"

[Ævar Arnfjörð Bjarmason]

Correct a comment referring to the removal of just the branch to also
refer to the tag. This should have been changed in my
ca3065e7e7 ("fetch tests: add a tag to be deleted to the pruning
tests", 2018-02-09) when the tag deletion was added, but I missed it
at the time.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5510-fetch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ae5a530a2d..9bd2783521 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -613,7 +613,7 @@ test_configured_prune_type () {
 			git rev-parse --verify refs/tags/newtag
 		) &&
 
-		# now remove it
+		# now remove them
 		git branch -d newbranch &&
 		git tag -d newtag &&
 
-- 
2.17.0.290.gded63e768a

[PATCH 7/8] fetch tests: add a test clobbering tag behavior

[Ævar Arnfjörð Bjarmason]

The test suite only incidentally (and unintentionally) tested for the
current behavior of eager tag clobbering on "fetch". This follow-up to
the previous "push tests: assert re-pushing annotated tags" change
tests for it explicitly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 71fc902062..9cf14c5cc1 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1003,6 +1003,30 @@ test_force_push_tag () {
 test_force_push_tag "lightweight tag" "-f"
 test_force_push_tag "annotated tag" "-f -a -m'msg'"
 
+test_force_fetch_tag () {
+	tag_type_description=$1
+	tag_args=$2
+
+	test_expect_success "fetch will clobber an existing $tag_type_description" "
+		mk_test testrepo heads/master &&
+		mk_child testrepo child1 &&
+		mk_child testrepo child2 &&
+		(
+			cd testrepo &&
+			git tag Tag &&
+			git -C ../child1 fetch origin tag Tag &&
+			>file1 &&
+			git add file1 &&
+			git commit -m 'file1' &&
+			git tag $tag_args Tag &&
+			git -C ../child1 fetch origin tag Tag
+		)
+	"
+}
+
+test_force_fetch_tag "lightweight tag" "-f"
+test_force_fetch_tag "annotated tag" "-f -a -m'msg'"
+
 test_expect_success 'push --porcelain' '
 	mk_empty testrepo &&
 	echo >.git/foo  "To testrepo" &&
-- 
2.17.0.290.gded63e768a

[PATCH 8/8] fetch: stop clobbering existing tags without --force

[Ævar Arnfjörð Bjarmason]

Change "fetch" to treat "+" in refspecs (aka --force) to mean we
should clobber a local tag of the same name.

This changes the long-standing behavior of "fetch" added in
853a3697dc ("[PATCH] Multi-head fetch.", 2005-08-20), before this
change all tag fetches effectively had --force enabled. The original
rationale in that change was:

    > Tags need not be pointing at commits so there is no way to
    > guarantee "fast-forward" anyway.

That comment and the rest of the history of "fetch" shows that the
"+" (--force) part of refpecs was only conceived for branch updates,
while tags have accepted any changes from upstream unconditionally and
clobbered the local tag object. Changing this behavior has been
discussed as early as 2011[1].

I the current behavior doesn't make sense, it easily results in local
tags accidentally being clobbered. Ideally we'd namespace our tags
per-remote, but as with my 97716d217c ("fetch: add a --prune-tags
option and fetch.pruneTags config", 2018-02-09) it's easier to work
around the current implementation than to fix the root cause, so this
implements suggestion #1 from [1], "fetch" now only clobbers the tag
if either "+" is provided as part of the refspec, or if "--force" is
provided on the command-line.

This also makes it nicely symmetrical with how "tag" itself
works. We'll now refuse to clobber any existing tags unless "--force"
is supplied, whether that clobbering would happen by clobbering a
local tag with "tag", or by fetching it from the remote with "fetch".

It's still not at all nicely symmetrical with how "git push" works, as
discussed in the updated pull-fetch-param.txt documentation, but this
change brings them more into line with one another. I don't think
there's any reason "fetch" couldn't fully converge with the behavior
used by "push", but that's a topic for another change.

One of the tests added in 31b808a032 ("clone --single: limit the fetch
refspec to fetched branch", 2012-09-20) is being changed to use
--force where a clone would clobber a tag. This changes nothing about
the existing behavior of the test.

1. https://public-inbox.org/git/20111123221658.GA22313@sigill.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/fetch-options.txt    | 15 ++++++++++-----
 Documentation/pull-fetch-param.txt | 22 ++++++++++++++++------
 builtin/fetch.c                    | 20 +++++++++++++-------
 t/t5516-fetch-push.sh              |  5 +++--
 t/t5612-clone-refspec.sh           |  4 ++--
 5 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 8631e365f4..5b4fc36866 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -49,11 +49,16 @@ endif::git-pull[]
 
 -f::
 --force::
-	When 'git fetch' is used with `<rbranch>:<lbranch>`
-	refspec, it refuses to update the local branch
-	`<lbranch>` unless the remote branch `<rbranch>` it
-	fetches is a descendant of `<lbranch>`.  This option
-	overrides that check.
+	When 'git fetch' is used with `<src>:<dst>` refspec it might
+	refuse to update the local branch as discussed
+ifdef::git-pull[]
+	in the `<refspec>` part of the linkgit:git-fetch[1]
+	documentation.
+endif::git-pull[]
+ifndef::git-pull[]
+	in the `<refspec>` part below.
+endif::git-pull[]
+	This option overrides that check.
 
 -k::
 --keep::
diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
index c579793af5..672e8bc1c0 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -32,12 +32,22 @@ name.
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`;
 it requests fetching everything up to the given tag.
 +
-The remote ref that matches <src>
-is fetched, and if <dst> is not empty string, the local
-ref that matches it is fast-forwarded using <src>.
-If the optional plus `+` is used, the local ref
-is updated even if it does not result in a fast-forward
-update.
+The remote ref that matches <src> is fetched, and if <dst> is not
+empty string, an attempt is made to update the local ref that matches
+it.
++
+Whether that update is allowed is confusingly not the inverse of
+whether a server will accept a push as described in the `<refspec>...`
+section of linkgit:git-push[1]. If it's a commit under `refs/heads/*`
+only fast-forwards are allowed, but unlike what linkgit:git-push[1]
+will accept clobbering any ref pointing to blobs, trees etc. in any
+other namespace will be accepted, but commits in any ref
+namespace. Those apply the same fast-forward rule. An exception to
+this is that as of Git version 2.18 any object under `refs/tags/*` is
+protected from updates.
++
+If the optional plus `+` is used, the local ref is updated if the
+update would have otherwise been rejected.
 +
 [NOTE]
 When the remote branch you want to fetch is known to
diff --git a/builtin/fetch.c b/builtin/fetch.c
index dcdfc66f09..e3a44b582a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -126,7 +126,7 @@ static struct option builtin_fetch_options[] = {
 		 N_("append to .git/FETCH_HEAD instead of overwriting")),
 	OPT_STRING(0, "upload-pack", &upload_pack, N_("path"),
 		   N_("path to upload pack on remote end")),
-	OPT__FORCE(&force, N_("force overwrite of local branch"), 0),
+	OPT__FORCE(&force, N_("force overwrite of local reference"), 0),
 	OPT_BOOL('m', "multiple", &multiple,
 		 N_("fetch from multiple remotes")),
 	OPT_SET_INT('t', "tags", &tags,
@@ -664,12 +664,18 @@ static int update_local_ref(struct ref *ref,
 
 	if (!is_null_oid(&ref->old_oid) &&
 	    starts_with(ref->name, "refs/tags/")) {
-		int r;
-		r = s_update_ref("updating tag", ref, 0);
-		format_display(display, r ? '!' : 't', _("[tag update]"),
-			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref, summary_width);
-		return r;
+		if (force || ref->force) {
+			int r;
+			r = s_update_ref("updating tag", ref, 0);
+			format_display(display, r ? '!' : 't', _("[tag update]"),
+				       r ? _("unable to update local ref") : NULL,
+				       remote, pretty_ref, summary_width);
+			return r;
+		} else {
+			format_display(display, '!', _("[rejected]"), _("would clobber existing tag"),
+				       remote, pretty_ref, summary_width);
+			return 1;
+		}
 	}
 
 	current = lookup_commit_reference_gently(&ref->old_oid, 1);
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 9cf14c5cc1..327737ecaf 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1007,7 +1007,7 @@ test_force_fetch_tag () {
 	tag_type_description=$1
 	tag_args=$2
 
-	test_expect_success "fetch will clobber an existing $tag_type_description" "
+	test_expect_success "fetch will not clobber an existing $tag_type_description without --force" "
 		mk_test testrepo heads/master &&
 		mk_child testrepo child1 &&
 		mk_child testrepo child2 &&
@@ -1019,7 +1019,8 @@ test_force_fetch_tag () {
 			git add file1 &&
 			git commit -m 'file1' &&
 			git tag $tag_args Tag &&
-			git -C ../child1 fetch origin tag Tag
+			test_must_fail git -C ../child1 fetch origin tag Tag &&
+			git -C ../child1 fetch origin '+refs/tags/*:refs/tags/*'
 		)
 	"
 }
diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
index fac5a73851..6ea8f50dae 100755
--- a/t/t5612-clone-refspec.sh
+++ b/t/t5612-clone-refspec.sh
@@ -104,7 +104,7 @@ test_expect_success 'clone with --no-tags' '
 test_expect_success '--single-branch while HEAD pointing at master' '
 	(
 		cd dir_master &&
-		git fetch &&
+		git fetch --force &&
 		git for-each-ref refs/remotes/origin |
 		sed -e "/HEAD$/d" \
 		    -e "s|/remotes/origin/|/heads/|" >../actual
@@ -115,7 +115,7 @@ test_expect_success '--single-branch while HEAD pointing at master' '
 	test_cmp expect actual &&
 	(
 		cd dir_master &&
-		git fetch --tags &&
+		git fetch --tags --force &&
 		git for-each-ref refs/tags >../actual
 	) &&
 	git for-each-ref refs/tags >expect &&
-- 
2.17.0.290.gded63e768a

[RFC PATCH v4 0/3] Optional sub hierarchy for remote tags

[Wink Saville]

As discussed on the git email list [1] with the subject
"Fetching tags overwrites existing tags" there is a bug
where fetching tags can overwrite existing tags.

Ævar Arnfjörð Bjarmasono, has created a patch series [2]
which requires a '--force' flag when fetching if the user
actually wants to overwrite any existing flags.

This patch series allows tags in remotes to be placed in
a sub hierarchy, refs/remote-tags when "git remote add" is
passed the --remote-tags option.

The patch series also modifies tag so "git tag -l" will list
tags in refs/remote-tags properly. It also test for
"git remote add" which uses "git tag -l".

[1]: https://public-inbox.org/git/xmqqbme51rgn.fsf@gitster-ct.c.googlers.com/T/#m4928340a9f49c6ffa9db5b94243e46df6958eaa5
[2]: https://public-inbox.org/git/xmqqbme51rgn.fsf@gitster-ct.c.googlers.com/T/#mb68d54667a9ad8eee60a22ebdf2264f8022d07a8

Wink Saville (3):
  Teach remote add the --remote-tags option
  Teach tag to list remote-tags
  Test git remote add -f --remote-tags

 Documentation/git-remote.txt |  8 +++++--
 builtin/remote.c             | 44 ++++++++++++++++++++++++++++++++----
 builtin/tag.c                |  2 +-
 ref-filter.c                 |  9 ++++++--
 ref-filter.h                 | 11 +++++----
 refs.c                       |  1 +
 remote.c                     |  2 ++
 t/t5505-remote.sh            | 20 ++++++++++++++++
 8 files changed, 83 insertions(+), 14 deletions(-)

-- 
2.17.0.393.g4573f5e134

[RFC PATCH v4 1/3] Teach remote add the --remote-tags option

[Wink Saville]

When --remote-tags is passed to `git remote add` the tagopt is set to
--remote-tags and a second fetch line is added so tags are placed in
a separate hierarchy per remote.

For example:
  $ git remote add -f --remote-tags gbenchmark git@github.com:google/benchmark
  Updating gbenchmark
  warning: no common commits
  remote: Counting objects: 4406, done.
  remote: Compressing objects: 100% (18/18), done.
  remote: Total 4406 (delta 7), reused 13 (delta 6), pack-reused 4382
  Receiving objects: 100% (4406/4406), 1.34 MiB | 7.58 MiB/s, done.
  Resolving deltas: 100% (2865/2865), done.
  From github.com:google/benchmark
   * [new branch]      clangtidy       -> gbenchmark/clangtidy
   * [new branch]      iter_report     -> gbenchmark/iter_report
   * [new branch]      master          -> gbenchmark/master
   * [new branch]      releasing       -> gbenchmark/releasing
   * [new branch]      reportercleanup -> gbenchmark/reportercleanup
   * [new branch]      rmheaders       -> gbenchmark/rmheaders
   * [new branch]      v2              -> gbenchmark/v2
   * [new tag]         v0.0.9          -> gbenchmark/v0.0.9
   * [new tag]         v0.1.0          -> gbenchmark/v0.1.0
   * [new tag]         v1.0.0          -> gbenchmark/v1.0.0
   * [new tag]         v1.1.0          -> gbenchmark/v1.1.0
   * [new tag]         v1.2.0          -> gbenchmark/v1.2.0
   * [new tag]         v1.3.0          -> gbenchmark/v1.3.0
   * [new tag]         v1.4.0          -> gbenchmark/v1.4.0

And the .git/config remote "gbenchmark" section looks like:
  [remote "gbenchmark"]
    url = git@github.com:google/benchmark
    fetch = +refs/heads/*:refs/remotes/gbenchmark/*
    fetch = +refs/tags/*:refs/remote-tags/gbenchmark/*
    tagopt = --remote-tags

Based on a solution proposed by Junio on the email list [1]

[1]: https://public-inbox.org/git/xmqqbme51rgn.fsf@gitster-ct.c.googlers.com/T/#me7f7f153b8ba742c0dc48d8ec79c280c9682d32e

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jacob Keller <jacob.keller@gmail.com>
Helped-by: Bryan Turner <bturner@atlassian.com>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Wink Saville <wink@saville.com>
---
 Documentation/git-remote.txt |  8 +++++--
 builtin/remote.c             | 44 ++++++++++++++++++++++++++++++++----
 refs.c                       |  1 +
 remote.c                     |  2 ++
 4 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4feddc0293..fc983c2ff1 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git remote' [-v | --verbose]
-'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=<fetch|push>] <name> <url>
+'git remote add' [-t <branch>] [-m <master>] [-f] [--tags | --remote-tags | --no-tags] [--mirror=<fetch|push>] <name> <url>
 'git remote rename' <old> <new>
 'git remote remove' <name>
 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)
@@ -54,7 +54,11 @@ With `-f` option, `git fetch <name>` is run immediately after
 the remote information is set up.
 +
 With `--tags` option, `git fetch <name>` imports every tag from the
-remote repository.
+remote repository to refs/tags, use --remote-tags to import them
+to refs/remote-tags/<name>/<tag>.
++
+With `--remote-tags` option, `git fetch <name>` imports every tag from the
+remote repository to refs/remote-tags/<name>/<tag>.
 +
 With `--no-tags` option, `git fetch <name>` does not import tags from
 the remote repository.
diff --git a/builtin/remote.c b/builtin/remote.c
index 805ffc05cd..07832113e9 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -11,7 +11,7 @@
 
 static const char * const builtin_remote_usage[] = {
 	N_("git remote [-v | --verbose]"),
-	N_("git remote add [-t <branch>] [-m <master>] [-f] [--tags | --no-tags] [--mirror=<fetch|push>] <name> <url>"),
+	N_("git remote add [-t <branch>] [-m <master>] [-f] [--tags | --remote-tags | --no-tags] [--mirror=<fetch|push>] <name> <url>"),
 	N_("git remote rename <old> <new>"),
 	N_("git remote remove <name>"),
 	N_("git remote set-head <name> (-a | --auto | -d | --delete | <branch>)"),
@@ -101,7 +101,8 @@ static int fetch_remote(const char *name)
 enum {
 	TAGS_UNSET = 0,
 	TAGS_DEFAULT = 1,
-	TAGS_SET = 2
+	TAGS_SET = 2,
+	TAGS_SET_REMOTE = 3
 };
 
 #define MIRROR_NONE 0
@@ -123,6 +124,14 @@ static void add_branch(const char *key, const char *branchname,
 	git_config_set_multivar(key, tmp->buf, "^$", 0);
 }
 
+static void add_remote_tags(const char *key, const char *remotename,
+			    struct strbuf *tmp)
+{
+	strbuf_reset(tmp);
+	strbuf_addf(tmp, "+refs/tags/*:refs/remote-tags/%s/*", remotename);
+	git_config_set_multivar(key, tmp->buf, "^$", 0);
+}
+
 static const char mirror_advice[] =
 N_("--mirror is dangerous and deprecated; please\n"
    "\t use --mirror=fetch or --mirror=push instead");
@@ -161,6 +170,9 @@ static int add(int argc, const char **argv)
 		OPT_SET_INT(0, "tags", &fetch_tags,
 			    N_("import all tags and associated objects when fetching"),
 			    TAGS_SET),
+		OPT_SET_INT(0, "remote-tags", &fetch_tags,
+			    N_("import all tags and associated objects when fetching to refs/remote-tags/<name>/<tag>"),
+			    TAGS_SET_REMOTE),
 		OPT_SET_INT(0, NULL, &fetch_tags,
 			    N_("or do not fetch any tag at all (--no-tags)"), TAGS_UNSET),
 		OPT_STRING_LIST('t', "track", &track, N_("branch"),
@@ -182,6 +194,10 @@ static int add(int argc, const char **argv)
 		die(_("specifying a master branch makes no sense with --mirror"));
 	if (mirror && !(mirror & MIRROR_FETCH) && track.nr)
 		die(_("specifying branches to track makes sense only with fetch mirrors"));
+	if ((fetch_tags == TAGS_SET_REMOTE) && mirror)
+		die(_("specifying --remote-tags makes no sense with --mirror"));
+	if ((fetch_tags == TAGS_SET_REMOTE) && track.nr)
+		die(_("specifying --remote-tags makes no sense with -t or --track"));
 
 	name = argv[0];
 	url = argv[1];
@@ -215,10 +231,30 @@ static int add(int argc, const char **argv)
 	}
 
 	if (fetch_tags != TAGS_DEFAULT) {
+		if (fetch_tags == TAGS_SET_REMOTE) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "remote.%s.fetch", name);
+			add_remote_tags(buf.buf, name, &buf2);
+		}
+
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "remote.%s.tagopt", name);
-		git_config_set(buf.buf,
-			       fetch_tags == TAGS_SET ? "--tags" : "--no-tags");
+		char *config_val = NULL;
+		switch (fetch_tags) {
+		case TAGS_UNSET:
+			config_val = "--no-tags";
+			break;
+		case TAGS_SET:
+			config_val = "--tags";
+			break;
+		case TAGS_SET_REMOTE:
+			config_val = "--remote-tags";
+			break;
+		default:
+			die(_("Unexpected TAGS enum %d"), fetch_tags);
+			break;
+		}
+		git_config_set(buf.buf, config_val);
 	}
 
 	if (fetch && fetch_remote(name))
diff --git a/refs.c b/refs.c
index 8b7a77fe5e..4075150e26 100644
--- a/refs.c
+++ b/refs.c
@@ -472,6 +472,7 @@ const char *prettify_refname(const char *name)
 {
 	if (skip_prefix(name, "refs/heads/", &name) ||
 	    skip_prefix(name, "refs/tags/", &name) ||
+	    skip_prefix(name, "refs/remote-tags/", &name) ||
 	    skip_prefix(name, "refs/remotes/", &name))
 		; /* nothing */
 	return name;
diff --git a/remote.c b/remote.c
index 91eb010ca9..86cccc0b6e 100644
--- a/remote.c
+++ b/remote.c
@@ -447,6 +447,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 			remote->fetch_tags = -1;
 		else if (!strcmp(value, "--tags"))
 			remote->fetch_tags = 2;
+		else if (!strcmp(value, "--remote-tags"))
+			remote->fetch_tags = -1;
 	} else if (!strcmp(subkey, "proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
-- 
2.17.0.393.g4573f5e134

[RFC PATCH v4 2/3] Teach tag to list remote-tags

[Wink Saville]

Add FILTER_REFS_REMOTE_TAGS to allow tags to be to identified as a
remote-tags and then list them without displaying refs/remote-tags.

Signed-off-by: Wink Saville <wink@saville.com>
---
 builtin/tag.c |  2 +-
 ref-filter.c  |  9 +++++++--
 ref-filter.h  | 11 ++++++-----
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 46a5c6a1da..9ac85f0ff8 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -57,7 +57,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 	if (verify_ref_format(format))
 		die(_("unable to parse format string"));
 	filter->with_commit_tag_algo = 1;
-	filter_refs(&array, filter, FILTER_REFS_TAGS);
+	filter_refs(&array, filter, (FILTER_REFS_TAGS | FILTER_REFS_REMOTE_TAGS));
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++)
diff --git a/ref-filter.c b/ref-filter.c
index ac82f9f21e..1fcaa2ea33 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1682,6 +1682,7 @@ static int match_pattern(const struct ref_filter *filter, const char *refname)
 	 * for matching refs of tags and branches.
 	 */
 	(void)(skip_prefix(refname, "refs/tags/", &refname) ||
+	       skip_prefix(refname, "refs/remote-tags/", &refname) ||
 	       skip_prefix(refname, "refs/heads/", &refname) ||
 	       skip_prefix(refname, "refs/remotes/", &refname) ||
 	       skip_prefix(refname, "refs/", &refname));
@@ -1866,7 +1867,8 @@ static int ref_kind_from_refname(const char *refname)
 	} ref_kind[] = {
 		{ "refs/heads/" , FILTER_REFS_BRANCHES },
 		{ "refs/remotes/" , FILTER_REFS_REMOTES },
-		{ "refs/tags/", FILTER_REFS_TAGS}
+		{ "refs/tags/", FILTER_REFS_TAGS},
+		{ "refs/remote-tags/", FILTER_REFS_REMOTE_TAGS}
 	};
 
 	if (!strcmp(refname, "HEAD"))
@@ -1884,7 +1886,8 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
 {
 	if (filter->kind == FILTER_REFS_BRANCHES ||
 	    filter->kind == FILTER_REFS_REMOTES ||
-	    filter->kind == FILTER_REFS_TAGS)
+	    filter->kind == FILTER_REFS_TAGS ||
+	    filter->kind == FILTER_REFS_REMOTE_TAGS)
 		return filter->kind;
 	return ref_kind_from_refname(refname);
 }
@@ -2053,6 +2056,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 			ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata, broken);
 		else if (filter->kind == FILTER_REFS_TAGS)
 			ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata, broken);
+		else if (filter->kind == FILTER_REFS_REMOTE_TAGS)
+			ret = for_each_fullref_in("refs/remote-tags/", ref_filter_handler, &ref_cbdata, broken);
 		else if (filter->kind & FILTER_REFS_ALL)
 			ret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata, broken);
 		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
diff --git a/ref-filter.h b/ref-filter.h
index 76cf87cb6c..8b8d725b37 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -15,12 +15,13 @@
 
 #define FILTER_REFS_INCLUDE_BROKEN 0x0001
 #define FILTER_REFS_TAGS           0x0002
-#define FILTER_REFS_BRANCHES       0x0004
-#define FILTER_REFS_REMOTES        0x0008
-#define FILTER_REFS_OTHERS         0x0010
+#define FILTER_REFS_REMOTE_TAGS    0x0004
+#define FILTER_REFS_BRANCHES       0x0008
+#define FILTER_REFS_REMOTES        0x0010
+#define FILTER_REFS_OTHERS         0x0020
 #define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
-				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
-#define FILTER_REFS_DETACHED_HEAD  0x0020
+				    FILTER_REFS_REMOTES | FILTER_REFS_REMOTE_TAGS | FILTER_REFS_OTHERS)
+#define FILTER_REFS_DETACHED_HEAD  0x0040
 #define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
 
 struct atom_value;
-- 
2.17.0.393.g4573f5e134

[RFC PATCH v4 3/3] Test git remote add -f --remote-tags

[Wink Saville]

The test adds and fetches a remote repository and then lists the
imported remote-tags verifying the correct values.

Signed-off-by: Wink Saville <wink@saville.com>
---
 t/t5505-remote.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index a6c0178f3a..cb30ed5cf2 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -573,6 +573,26 @@ test_expect_success 'add --tags' '
 	test_cmp test/expect test/output
 '
 
+cat >test/expect <<\EOF
+origin/some-tag
+origin/foobar-tag
+--remote-tags
+EOF
+
+test_expect_success 'add --remote-tags' '
+	rm -rf add-tags &&
+	(
+		mkdir add-tags &&
+		cd add-tags &&
+		git init &&
+		git remote add -f --remote-tags origin ../one &&
+		git tag -l origin/some-tag >../test/output &&
+		git tag -l origin/foobar-tag >>../test/output &&
+		git config remote.origin.tagopt >>../test/output
+	) &&
+	test_cmp test/expect test/output
+'
+
 cat >test/expect <<\EOF
 --no-tags
 EOF
-- 
2.17.0.393.g4573f5e134

Re: Fetching tags overwrites existing tags

[Wink Saville]

I'm going on vacation until May 17 and will have limited connectivity.
I've created a new patch series, "Optional sub hierarchy for remote tags" [1]
that I'd appreciate comments and if someone wants to take this over
in my absence that would be great. I'd sure like to see this continue to move
forward.

In v4 I've tweaked git tag -l and added a test, so for me it has the minimal
functionality I'd need to make use of it.

-- Wink

https://public-inbox.org/git/xmqqbme51rgn.fsf@gitster-ct.c.googlers.com/T/#m89df9b973ab55d85eae72dd749e038354ea3a250

Re: [RFC PATCH v4 1/3] Teach remote add the --remote-tags option

[Ævar Arnfjörð Bjarmason]

On Tue, May 01 2018, Wink Saville wrote:

> [1]: https://public-inbox.org/git/xmqqbme51rgn.fsf@gitster-ct.c.googlers.com/T/#me7f7f153b8ba742c0dc48d8ec79c280c9682d32e

Better to link to https://public-inbox.org/git/xmqqzi1s3y5h.fsf@gitster-ct.c.googlers.com/

Re: [RFC PATCH v4 0/3] Optional sub hierarchy for remote tags

[Ævar Arnfjörð Bjarmason]

On Tue, May 01 2018, Wink Saville wrote:

> As discussed on the git email list [1] with the subject
> "Fetching tags overwrites existing tags" there is a bug
> where fetching tags can overwrite existing tags.
>
> Ævar Arnfjörð Bjarmasono, has created a patch series [2]
> which requires a '--force' flag when fetching if the user
> actually wants to overwrite any existing flags.
>
> This patch series allows tags in remotes to be placed in
> a sub hierarchy, refs/remote-tags when "git remote add" is
> passed the --remote-tags option.
>
> The patch series also modifies tag so "git tag -l" will list
> tags in refs/remote-tags properly. It also test for
> "git remote add" which uses "git tag -l".

Thanks a lot for working on this, it's great to have something the
direction of getting rid of this discrepancy between the 1=1 mapping for
branches, but the 1=many mapping for tags. My recent patch series and
the pruneTags feature I added in 2.17.0 are really just hacks around not
having that.

My concern with your patches is not that they're not implementing some
mythical future where we're mapping each <remote>'s refs to
remotes/<fully qualified ref> and local refs to local/<fully qualified
ref>, which is what we should really be aiming for and would forever get
us out of this rut of 1=many and local & remote refs existing in the
same namespace, but that they might make it harder to get there.

So specifically, your 1/3 writes this to the config:

  [remote "gbenchmark"]
    url = git@github.com:google/benchmark
    fetch = +refs/heads/*:refs/remotes/gbenchmark/*
    fetch = +refs/tags/*:refs/remote-tags/gbenchmark/*
    tagopt = --remote-tags

If the user clones with your patch, and then uses an older git version
on the same repo (a use case we need to support) that older version
doesn't know about --remote-tags, and will fetch them all.

As a workaround for that maybe we'll need something like:

  [remote "gbenchmark"]
    url = git@github.com:google/benchmark
    fetch = +refs/heads/*:refs/remotes/gbenchmark/*
    fetch = +refs/tags/*:refs/remote-tags/gbenchmark/*
	tagStyle = remote
    tagopt = --no-tags

Or whatever, i.e. something where only the new version will fetch the
tags and ignore the tagopt option (which I never liked anyway). It's a
hack, but at least you don't end up with crap it your ref namespace by
flip-flopping between versions.

Then as I alluded to in my
https://public-inbox.org/git/20180429202100.32353-6-avarab@gmail.com/ we
have a lot of stuff that hardcodes special behaviors for
refs/{tags,heads}/, including but not limited to:

    git grep -C2 -e TAG_REFSPEC -e tag_refspec -e '"refs/tags/' -- '*.[ch]'

So maybe we need to start this series with some set of patches where we
make the currently hardcoded behavior for refs/{heads,tags}/
configurable.

Sorry about this "nice shed you built, how about you make a cathedral
instead?" E-Mail. I really don't think we should make perfect the enemy
of the good, but at the same time it would be unfortunate if we can't
get perfect because we settled for good.

Re: [RFC PATCH v4 0/3] Optional sub hierarchy for remote tags

[Jacob Keller]

On Tue, May 1, 2018 at 12:24 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Thanks a lot for working on this, it's great to have something the
> direction of getting rid of this discrepancy between the 1=1 mapping for
> branches, but the 1=many mapping for tags. My recent patch series and
> the pruneTags feature I added in 2.17.0 are really just hacks around not
> having that.
>
> My concern with your patches is not that they're not implementing some
> mythical future where we're mapping each <remote>'s refs to
> remotes/<fully qualified ref> and local refs to local/<fully qualified
> ref>, which is what we should really be aiming for and would forever get
> us out of this rut of 1=many and local & remote refs existing in the
> same namespace, but that they might make it harder to get there.
>

I also agree, I'd prefer if we aim for the mapping to be something
which works for all refs in the future, even if such support isn't
added now, which is why i've proposed using "refs/remote/<name>/" so
that a tag would go from

refs/tags/v1.7

to

refs/remote/<name>/tags/v1.7

Ideally, we could work to update "refs/remotes/<name>" to go to
"refs/remote/<name>/heads" as well. This allows obtaining any ref and
mapping it safely per remote. I don't really like the fact that we
can't simply re-use "remotes", nor do I like the fact that "remote" is
very close to "remotes" increasing the chance of typos... historically
I suggested using "tracking" but I don't really like that either..
because honestly there *is* no good name left for this purpose once
"remotes" became only about branches.

The reason I care about this is that I do want to be able to share the
notes refs in a way that allows easy merging, which currently is not
really doable without a lot of work from all users of notes knowing
exactly how you plan to work on them.

Thanks,
Jake

> So specifically, your 1/3 writes this to the config:
>
>   [remote "gbenchmark"]
>     url = git@github.com:google/benchmark
>     fetch = +refs/heads/*:refs/remotes/gbenchmark/*
>     fetch = +refs/tags/*:refs/remote-tags/gbenchmark/*
>     tagopt = --remote-tags
>
> If the user clones with your patch, and then uses an older git version
> on the same repo (a use case we need to support) that older version
> doesn't know about --remote-tags, and will fetch them all.
>
> As a workaround for that maybe we'll need something like:
>
>   [remote "gbenchmark"]
>     url = git@github.com:google/benchmark
>     fetch = +refs/heads/*:refs/remotes/gbenchmark/*
>     fetch = +refs/tags/*:refs/remote-tags/gbenchmark/*
>         tagStyle = remote
>     tagopt = --no-tags
>
> Or whatever, i.e. something where only the new version will fetch the
> tags and ignore the tagopt option (which I never liked anyway). It's a
> hack, but at least you don't end up with crap it your ref namespace by
> flip-flopping between versions.
>
> Then as I alluded to in my
> https://public-inbox.org/git/20180429202100.32353-6-avarab@gmail.com/ we
> have a lot of stuff that hardcodes special behaviors for
> refs/{tags,heads}/, including but not limited to:
>
>     git grep -C2 -e TAG_REFSPEC -e tag_refspec -e '"refs/tags/' -- '*.[ch]'
>
> So maybe we need to start this series with some set of patches where we
> make the currently hardcoded behavior for refs/{heads,tags}/
> configurable.
>
> Sorry about this "nice shed you built, how about you make a cathedral
> instead?" E-Mail. I really don't think we should make perfect the enemy
> of the good, but at the same time it would be unfortunate if we can't
> get perfect because we settled for good.

Re: [RFC PATCH v4 0/3] Optional sub hierarchy for remote tags

[Wink Saville]

Jake & Ævar,

Thanks for the great feed back. As I mentioned previously,
I'm going to be on vacation until May 17th, but will be following
along on email as connedtivity allows.

I'm all for moving in the "perfect" direction so hopefully you guys can
decide on a solution and start moving in that direction. When I
get back I'll do whatever I can to help.

My only criteria is that when a "remote" is added and fetched its tags
are put in a different namespace and can be referenced easily.

Here are some suggestions for naming:

  refs/subs
  refs/sub-repos
  refs/repos
  refs/externals
  refs/imports


Arrivederci,

Wink

Re: [RFC PATCH v4 0/3] Optional sub hierarchy for remote tags

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> I also agree, I'd prefer if we aim for the mapping to be something
> which works for all refs in the future, even if such support isn't
> added now, which is why i've proposed using "refs/remote/<name>/" so
> that a tag would go from
>
> refs/tags/v1.7
>
> to
>
> refs/remote/<name>/tags/v1.7
>
> Ideally, we could work to update "refs/remotes/<name>" to go to
> "refs/remote/<name>/heads" as well.

This is *not* imcompatible with having refs/remote-tags/* as an
interim solution.  

We'll have to support refs/remotes/<name>/<branch> anyway long after
we start using refs/remote/<name>/heads/<branch> by (1) switching
the fetch refspecs newer "git clone" writes to the latter format,
and (2) extending the dwim table to try both formats.  Having Wink's
solution as an interim step adds one more entry to (2) but the
machinery is already there.  And it does not change (1), either.

Re: [RFC PATCH v4 0/3] Optional sub hierarchy for remote tags

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> As a workaround for that maybe we'll need something like:
>
>   [remote "gbenchmark"]
>     url = git@github.com:google/benchmark
>     fetch = +refs/heads/*:refs/remotes/gbenchmark/*
>     fetch = +refs/tags/*:refs/remote-tags/gbenchmark/*
> 	tagStyle = remote
>     tagopt = --no-tags

Good thinking.  In the longer term we would probably want to
deprecate tagopt that was invented in a very lazy way (it was
originally meant to hold any random string that we can insert on the
shell command that invokes "git fetch", which obviously is not a
good idea in the production code) and replace it with something more
"controlled", and the above looks like a good improvement to Wink's
proposed change.

> Or whatever, i.e. something where only the new version will fetch the
> tags and ignore the tagopt option (which I never liked anyway). It's a
> hack, but at least you don't end up with crap it your ref namespace by
> flip-flopping between versions.

Re: [RFC PATCH v4 0/3] Optional sub hierarchy for remote tags

[Jacob Keller]

On Tue, May 1, 2018 at 4:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> I also agree, I'd prefer if we aim for the mapping to be something
>> which works for all refs in the future, even if such support isn't
>> added now, which is why i've proposed using "refs/remote/<name>/" so
>> that a tag would go from
>>
>> refs/tags/v1.7
>>
>> to
>>
>> refs/remote/<name>/tags/v1.7
>>
>> Ideally, we could work to update "refs/remotes/<name>" to go to
>> "refs/remote/<name>/heads" as well.
>
> This is *not* imcompatible with having refs/remote-tags/* as an
> interim solution.

Sure. I'm just proposing that we pick a name that all the refs can move to now.

>
> We'll have to support refs/remotes/<name>/<branch> anyway long after
> we start using refs/remote/<name>/heads/<branch> by (1) switching
> the fetch refspecs newer "git clone" writes to the latter format,

Ofcourse we'll have to support this, and i didn't mean to imply we wouldn't.

I was just hoping to avoid having even more places to check in the future.

> and (2) extending the dwim table to try both formats.  Having Wink's
> solution as an interim step adds one more entry to (2) but the
> machinery is already there.  And it does not change (1), either.
>

Sure, we could. And yes, we have to do (1), which means we have to do
(2) anyways. But we can still pick something which is more easily
expandable than refs/remotes/<name> was.

Thanks,
Jake

Re: Fetching tags overwrites existing tags

[Jacob Keller]

On Fri, Apr 27, 2018 at 12:13 PM, Bryan Turner <bturner@atlassian.com> wrote:
> On Fri, Apr 27, 2018 at 12:08 PM, Wink Saville <wink@saville.com> wrote:
>>
>> The other change was rather than using ""+refs/tags/*:refs/remote-tags/$name/*"
>> I've changed it to "+refs/tags/*:refs/remote/tags/$name/*" which seems cleaner.
>> Again, if remote-tags is preferred I'll change it back.
>
>
> From looking at the code, it looks like you mean
> "+refs/tags/*:refs/remotes/tags/$name/*".
>
> The issue with that approach is that it collides with a remote named
> "tags". "refs/remote-tags", on the other hand, represents a new-to-Git
> path, one that won't already be in use by any other standard
> functionality. That seems like a better approach than hoping no one
> out there will call one of their remotes "tags".
>
> Bryan

Note that my suggestion was very specific "remote" not pluralized,
which is obviously a bit confusing, since there's remote and
"remotes".

The goal being that you put "remote/<name>/" followed by the full
remote ref minus the refs prefix.

It specifically is attempting to avoid the problem of expanding
"remotes". Unfortunately, I don't have a better alternative format,
and i very much want to avoid having to do "remote-tags",
"remote-notes", "remote-replaces", "remote-meta" etc...

In that spirit, I'm working to hopefully propose something today.

Thanks,
Jake

Re: [PATCH 3/8] push tests: add more testing for forced tag pushing

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 2528 bytes --]

Hi,

On Monday 30 April 2018 01:50 AM, Ævar Arnfjörð Bjarmason wrote:

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 15c8d5a734..c9a2011915 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -981,7 +981,17 @@ test_expect_success 'push requires --force to update lightweight tag' '

I think the test description has become misleading now that it's testing
for 'force pushing' in general and not just the '--force' option. So, a
better description is needed. Probably, "force pushing required to
update lightweight tag".


>  		git push --force ../child2 Tag &&
>  		git tag -f Tag HEAD~ &&
>  		test_must_fail git push ../child2 Tag &&
> -		git push --force ../child2 Tag
> +		git push --force ../child2 Tag &&
> +		git tag -f Tag &&
> +		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
> +		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
> +		git tag -f Tag HEAD~ &&
> +		git push ../child2 "+refs/tags/*:refs/tags/*" &&

> +		git tag -f Tag &&
> +		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&

This test shouldn't hiding within the tests about force pushing. It
seems to warrant a separate test case to clearly note the behavior that
the "+" in refspec overrides "--no-force". This would help in easily
identifying if this particular behavior is broken or not.


-- 
Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

      - Joel Spolsky


Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/8] push tests: add more testing for forced tag pushing

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Improve the tests added in dbfeddb12e ("push: require force for refs
> under refs/tags/", 2012-11-29) to assert that the same behavior
> applies various forms other refspecs, and that "+" in a refspec will
> override the "--no-force" option (but not the other way around).

For some reason this fell out of my radar; sorry about that.

I like the general idea to ensure non-ff pushes are rejected, unless
forced, to update a light-weight tag with another.  I am unsure what
should happen when trying to update a light-weight tag with an
object with different type (or vice versa), and haven't read in this
series what your opinion is yet.  Let's read on and see how it goes.

I have a moderately strong preference that

	$ git push --no-force child2

with a configured refspec

	[remote "child2"]
		url = ../child2
		push = +refs/tags/*:refs/tags/*

should behave as a non-forced push (regardless of the refs hierarchy
involved, not limited to tags/).  I have a mild preference against

	$ git push --no-force ../child2 +refs/tags/*:refs/tags/*

that forces, just because command line options look a lot more
explicit than the prefix '+', and choosing it not to force would
make it consistent with the desired behaviour for configured forcing
refspec.

I couldn't quite get what you meant by "(but not the other way
around)".  Did you mean

	$ git push --force ../child2 refs/tags/*:refs/tags/*

should not become non-forcing version because of the (lack of)
prefix on the refspec does not trump the --force command line
option?  If so, making

	$ git push --no-force ../child2 +refs/tags/*:refs/tags/*

not to force would make things more consistent, I suspect, i.e. we
can simply declare that presence or absense of '+' prefix in the
refspec determines the forced-ness of the push/fetch when there is
no command line option to decide it, but an explicit command line
option will always override it.  

Am I missing something obvious?

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t5516-fetch-push.sh | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 15c8d5a734..c9a2011915 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -981,7 +981,17 @@ test_expect_success 'push requires --force to update lightweight tag' '
>  		git push --force ../child2 Tag &&
>  		git tag -f Tag HEAD~ &&
>  		test_must_fail git push ../child2 Tag &&
> -		git push --force ../child2 Tag
> +		git push --force ../child2 Tag &&
> +		git tag -f Tag &&
> +		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
> +		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
> +		git tag -f Tag HEAD~ &&
> +		git push ../child2 "+refs/tags/*:refs/tags/*" &&
> +		git tag -f Tag &&
> +		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
> +		git tag -f Tag HEAD~ &&
> +		test_must_fail git push ../child2 tag Tag &&
> +		git push --force ../child2 tag Tag
>  	)
>  '

Re: [PATCH 3/8] push tests: add more testing for forced tag pushing

[Junio C Hamano]

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

> I couldn't quite get what you meant by "(but not the other way
> around)".  Did you mean
>
> 	$ git push --force ../child2 refs/tags/*:refs/tags/*
>
> should not become non-forcing version because of the (lack of)
> prefix on the refspec does not trump the --force command line
> option?  If so, making
>
> 	$ git push --no-force ../child2 +refs/tags/*:refs/tags/*
>
> not to force would make things more consistent, I suspect, i.e. we
> can simply declare that presence or absense of '+' prefix in the
> refspec determines the forced-ness of the push/fetch when there is
> no command line option to decide it, but an explicit command line
> option will always override it.  
>
> Am I missing something obvious?

And of course I am missing the fact that --force and --no-force
controls a single boolean.  If it controled a tristate (unspecified,
false, true), then what I wrote above makes tons of sense, but that
is not the reality.  "git push --no-force" is saying the the same as
"git push", and its primarily reason for being there is to countermand
a "--force" that appears earlier on the command line for whatever
reason, e.g.

	$ alias push='git push --force'
	$ push --no-force ../child2 ...

So what you said in this patch 100%  makes sense.

Re: [PATCH 4/8] push tests: assert re-pushing annotated tags

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the test that asserts that lightweight tags can only be
> clobbered by a force-push to check do the same tests for annotated
> tags.
>
> There used to be less exhaustive tests for this with the code added in
> 40eff17999 ("push: require force for annotated tags", 2012-11-29), but
> Junio removed them in 256b9d70a4 ("push: fix "refs/tags/ hierarchy
> cannot be updated without --force"", 2013-01-16) while fixing some of
> the behavior around tag pushing.
>
> That change left us without any coverage asserting that pushing and
> clobbering annotated tags worked as intended.  There was no reason to
> suspect that the receive machinery wouldn't behave the same way with
> annotated tags, but now we know for sure.

Hmm, I am not sure if annotated tag T1 on commit C1 should be called
to "fast-forward to annotated tag T2 on commit C2" when C2 is a
descendant of C1.  Tag is meant to be a non-moving anchor point, so
it may make sense not to allow "fast-forwarding hence it is OK to
replace" that is typical for branch heads, which are meant to move
"forward".

But let's move on and keep reading, at least temporarily assuming
that "fast-forwarding" annotated tags makes sense.  Under that
assumption, this patch makes perfect sense to ensure lightweight and
annotated tags behave the same.

Thanks.

Re: [PATCH 5/8] push doc: correct lies about how push refspecs work

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  +
>  The <src> is often the name of the branch you would want to push, but
> -it can be any arbitrary "SHA-1 expression", such as `master~4` or
> -`HEAD` (see linkgit:gitrevisions[7]).
> +it can be any arbitrary "SHA-1 expression" referring to a branch, such
> +as `master~4` or `HEAD` (see linkgit:gitrevisions[7]). It can also
> +refer to tag objects, trees or blobs if the <dst> is outside of
> +`refs/heads/*`.

I think the addition of "referring to a branch" here is an opposite
of an improvement.  The fact <src> can name any object (if outside
the refs/heads/) or any commit (otherwise) is stressed with the
added "It can also ...", which is great, but neither "master~4" nor
"HEAD" refer to a branch (they refer to a commit in terms relative
to a branch and a (psuedo)ref, respectively).  And a "SHA-1 expression"
that uses branch tips as a starting point (e.g. master~4 is "start
at the tip of master and go backwards by 4 steps) is *not* special
here.  You can spell your <src> side as "v2.17.0^0" for example, and
it does not refer to any branch.

> @@ -74,12 +76,24 @@ without any `<refspec>` on the command line.  Otherwise, missing
>  `:<dst>` means to update the same ref as the `<src>`.
>  +
>  The object referenced by <src> is used to update the <dst> reference
> -on the remote side.  By default this is only allowed if <dst> is not
> -a tag (annotated or lightweight), and then only if it can fast-forward
> -<dst>.  By having the optional leading `+`, you can tell Git to update
> -the <dst> ref even if it is not allowed by default (e.g., it is not a
> -fast-forward.)  This does *not* attempt to merge <src> into <dst>.  See
> -EXAMPLES below for details.
> +on the remote side. Whether this is allowed depends on what where in

s/what where/where/, I think.

> +`refs/*` the <dst> reference lives. The `refs/heads/*` namespace will
> +only accept commit objects, and then only they can be
> +fast-forwarded. ...

Nicely clarified.  Excellent.

> +.... The `refs/tags/*` namespace will accept any kind of
> +object, but there commit objects are known as lightweight tags, and
> +any changes to them and others types of objects will be
> +rejected. ...

with s/, but there commit objects are known as lightweght tags/ the
sentence does not change any meaning?  An early part of the paragraph
made readers anticipate that they hear rules for what can go where,
and "refs/tags/ ref that point at a commit is called lightweight tag",
while it is not an incorrect statement per-se, does not belong to
these "rules".  Unless the discussing of the rules immediately
follows involves (or becomes easier to read if we use the term)
"lightweight tags", it probably is better to drop it.

> +... Finally and most confusingly, it's possible to push any type
> +of object to any namespace outside of `refs/{tags,heads}/*`, but these
> +will be treated as branches, even in the case where a tag object is
> +pushed.

I sense a confused writer, not a confusing behaviour being described
here.  If refs/poo/* is "treated as branches", because of what you
earlier said, you shouldn't be able to push a tag object in the
first place.

If refs/poo/* is meant to be lawless land where anything goes, then
saying "will be treated as branches" does not help readers.

> +... That tag object will be overwritten by another tag object (or
> +commit!) without `--force` if the new tag happens to point to a commit
> +that's a fast-forward of the commit it replaces.

If I pretend that I didn't see the "treated as branches", I fully
agree with the above description and refs/poo/* being a world
governed by random rules, and I do not think I'd be too opposed to
change it to "anything goes".  I do not think I'd be too opposed to
change it to "nothing is allowed unless forced", either, though.

> +By having the optional leading `+`, you can tell Git to update the
> +<dst> ref even if it is not allowed by its respective namespace
> +clobbering rules (e.g., it is not a fast-forward. in the case of
> +`refs/heads/*` updates) This does *not* attempt to merge <src> into
> +<dst>.  See EXAMPLES below for details.

Excellent.  

s/leading `+`/& to a refspec (or using "--force" command line option)/

Thanks.

Re: [PATCH 8/8] fetch: stop clobbering existing tags without --force

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>     > Tags need not be pointing at commits so there is no way to
>     > guarantee "fast-forward" anyway.

The observation the above statement makes is not incorrect per-se,
but it does not justify "anything goes".  "nothing is allowed unless
forced" is equally a logical consequence of the observation.

> That comment and the rest of the history of "fetch" shows that the
> "+" (--force) part of refpecs was only conceived for branch updates,
> while tags have accepted any changes from upstream unconditionally and
> clobbered the local tag object. Changing this behavior has been
> discussed as early as 2011[1].

Thanks for a pointer.  We didn't keep reflog on tags as we wanted
tags to be fixed points and made --tags a refspec without leading
'+' because we didn't want this local clobbering.  I'd say it is
just a buggy implementation, and we should just implement a simple
rule "refs/tags/* is never updated unless forced".

> I the current behavior doesn't make sense, it easily results in local

s/I the/To me, the/, or s/I the/The/.

> tags accidentally being clobbered. Ideally we'd namespace our tags
> per-remote, but as with my 97716d217c ("fetch: add a --prune-tags
> option and fetch.pruneTags config", 2018-02-09) it's easier to work
> around the current implementation than to fix the root cause,

I do not think they are the same problem.

You can have refs/remote/$name/v1.0 and have look-up rules to peek
at various places in refs/* hierarchy for v1.0, and you may have
*solved* the "oops I overwrote and the meaning of v1.0 suddenly
changed" issue, but if you fetched to a location in refs/* that has
higher precedence, then "oops, the meaning of v1.0 suddenly changed"
issue itself is *not* solved at all.

> so this
> implements suggestion #1 from [1], "fetch" now only clobbers the tag
> if either "+" is provided as part of the refspec, or if "--force" is
> provided on the command-line.

Good.  Regardless of the issue of separate namespace that is
overlayed at the look-up time, this makes tons of sense.

> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 8631e365f4..5b4fc36866 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -49,11 +49,16 @@ endif::git-pull[]
>  
>  -f::
>  --force::
> -	When 'git fetch' is used with `<rbranch>:<lbranch>`
> -	refspec, it refuses to update the local branch
> -	`<lbranch>` unless the remote branch `<rbranch>` it
> -	fetches is a descendant of `<lbranch>`.  This option
> -	overrides that check.
> +	When 'git fetch' is used with `<src>:<dst>` refspec it might

Nice to see attention to the detail here.  s/might/may/, I would
say, though.

> +	refuse to update the local branch as discussed

> diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
> index c579793af5..672e8bc1c0 100644
> --- a/Documentation/pull-fetch-param.txt
> +++ b/Documentation/pull-fetch-param.txt
> @@ -32,12 +32,22 @@ name.
>  `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`;
>  it requests fetching everything up to the given tag.
>  +
> -The remote ref that matches <src>
> -is fetched, and if <dst> is not empty string, the local
> -ref that matches it is fast-forwarded using <src>.
> -If the optional plus `+` is used, the local ref
> -is updated even if it does not result in a fast-forward
> -update.
> +The remote ref that matches <src> is fetched, and if <dst> is not
> +empty string, an attempt is made to update the local ref that matches
> +it.
> ++
> +Whether that update is allowed is confusingly not the inverse of
> +whether a server will accept a push as described in the `<refspec>...`
> +section of linkgit:git-push[1]. If it's a commit under `refs/heads/*`
> +only fast-forwards are allowed,

Perhaps correct.  It is unclear what happens when it is fetching
non-commit to refs/heads/* in the above sentence.

> but unlike what linkgit:git-push[1]
> +will accept clobbering any ref pointing to blobs, trees etc. in any
> +other namespace will be accepted, but commits in any ref
> +namespace. ...

I cannot quite parse this.

> +... Those apply the same fast-forward rule.

Who are "Those"?  refs/poo/*?

> +... An exception to
> +this is that as of Git version 2.18 any object under `refs/tags/*` is
> +protected from updates.

OK.

> +If the optional plus `+` is used, the local ref is updated if the

Tighten "is used" to claify that you are talking about the '+'
prefix that signals a forced push/fetch.  We do not want to hear
from people who complain their "git fetch origin master+" does not
work.

> -	OPT__FORCE(&force, N_("force overwrite of local branch"), 0),
> +	OPT__FORCE(&force, N_("force overwrite of local reference"), 0),

Good.  This is long overdue.

Re: [PATCH 3/8] push tests: add more testing for forced tag pushing

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 1850 bytes --]

On Tuesday 08 May 2018 08:49 AM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> I couldn't quite get what you meant by "(but not the other way
>> around)".  Did you mean
>>
>> 	$ git push --force ../child2 refs/tags/*:refs/tags/*
>>
>> should not become non-forcing version because of the (lack of)
>> prefix on the refspec does not trump the --force command line
>> option?

When I was reading the commit message, I had the same doubt about what
"(but not the other way around)" actually meant but I assumed it meant
that "the `--no-force` in the command line does not override the '+' in
the refspec". Maybe the commit message could be updated to clarify this?


-- 
Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

      - Joel Spolsky


Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/8] push tests: add more testing for forced tag pushing

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 2529 bytes --]

On Monday 30 April 2018 01:50 AM, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 15c8d5a734..c9a2011915 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -981,7 +981,17 @@ test_expect_success 'push requires --force to update lightweight tag' '
>  		git push --force ../child2 Tag &&
>  		git tag -f Tag HEAD~ &&
>  		test_must_fail git push ../child2 Tag &&
> -		git push --force ../child2 Tag
> +		git push --force ../child2 Tag &&
> +		git tag -f Tag &&
> +		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
> +		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
> +		git tag -f Tag HEAD~ &&
> +		git push ../child2 "+refs/tags/*:refs/tags/*" &&
> +		git tag -f Tag &&
> +		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
> +		git tag -f Tag HEAD~ &&
> +		test_must_fail git push ../child2 tag Tag &&
> +		git push --force ../child2 tag Tag

As a person who came to know about the "tag <tag_name>" refspec for the
first time while seeing this patch, I found it a little hard to parse
the following two lines of the test:

		test_must_fail git push ../child2 tag Tag &&
		git push --force ../child2 tag Tag

Maybe some other name than "Tag" for the example would have made it
easier for the person reading it. Something like "foo"/"bar" etc.


-- 
Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

      - Joel Spolsky


Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v4 1/3] Teach remote add the --remote-tags option

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 1608 bytes --]

On Tuesday 01 May 2018 10:29 PM, Wink Saville wrote:
> When --remote-tags is passed to `git remote add` the tagopt is set to
> --remote-tags and a second fetch line is added so tags are placed in
> a separate hierarchy per remote.
> 

I find '--remote' in the option name to be redundant given that it is an
option to `git remote add`. I guess '--namespace-tags' would be a better
alternative as it seems to convey the meaning more directly to the user.


-- 
Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

      - Joel Spolsky


Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/8] push tests: assert re-pushing annotated tags

[SZEDER Gábor]

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index c9a2011915..71fc902062 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -965,35 +965,43 @@ test_expect_success 'push into aliased refs (inconsistent)' '
>  	)
>  '
>  
> -test_expect_success 'push requires --force to update lightweight tag' '
> -	mk_test testrepo heads/master &&
> -	mk_child testrepo child1 &&
> -	mk_child testrepo child2 &&
> -	(
> -		cd child1 &&
> -		git tag Tag &&
> -		git push ../child2 Tag &&
> -		>file1 &&
> -		git add file1 &&
> -		git commit -m "file1" &&
> -		git tag -f Tag &&
> -		test_must_fail git push ../child2 Tag &&
> -		git push --force ../child2 Tag &&
> -		git tag -f Tag HEAD~ &&
> -		test_must_fail git push ../child2 Tag &&
> -		git push --force ../child2 Tag &&
> -		git tag -f Tag &&
> -		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
> -		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
> -		git tag -f Tag HEAD~ &&
> -		git push ../child2 "+refs/tags/*:refs/tags/*" &&
> -		git tag -f Tag &&
> -		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
> -		git tag -f Tag HEAD~ &&
> -		test_must_fail git push ../child2 tag Tag &&
> -		git push --force ../child2 tag Tag
> -	)
> -'
> +test_force_push_tag () {
> +	tag_type_description=$1
> +	tag_args=$2
> +
> +	test_expect_success "push requires --force to update $tag_type_description" "
> +		mk_test testrepo heads/master &&
> +		mk_child testrepo child1 &&
> +		mk_child testrepo child2 &&
> +		(
> +			cd child1 &&
> +			git tag Tag &&
> +			git push ../child2 Tag &&
> +			>file1 &&
> +			git add file1 &&
> +			git commit -m 'file1' &&
> +			git tag $tag_args Tag &&
> +			test_must_fail git push ../child2 Tag &&
> +			git push --force ../child2 Tag &&
> +			git tag $tag_args Tag HEAD~ &&
> +			test_must_fail git push ../child2 Tag &&
> +			git push --force ../child2 Tag &&
> +			git tag $tag_args Tag &&
> +			test_must_fail git push ../child2 'refs/tags/*:refs/tags/*' &&
> +			git push --force ../child2 'refs/tags/*:refs/tags/*' &&
> +			git tag $tag_args Tag HEAD~ &&
> +			git push ../child2 '+refs/tags/*:refs/tags/*' &&
> +			git tag $tag_args Tag &&function

There is that unwanted "function" at the end of the line.

Interstingly, the test does pass when run with dash, but fails the
chain-lint tests when run with Bash, even though it's in a subshell.

> +			git push --no-force ../child2 '+refs/tags/*:refs/tags/*' &&
> +			git tag $tag_args Tag HEAD~ &&
> +			test_must_fail git push ../child2 tag Tag &&
> +			git push --force ../child2 tag Tag
> +		)
> +	"
> +}
> +
> +test_force_push_tag "lightweight tag" "-f"
> +test_force_push_tag "annotated tag" "-f -a -m'msg'"
>  
>  test_expect_success 'push --porcelain' '
>  	mk_empty testrepo &&

[PATCH v2 00/10] "git fetch" should not clobber existing tags without --force

[Ævar Arnfjörð Bjarmason]

It took me a long time to submit a re-roll for this, but this should
solve all issues noted with v1, see
https://public-inbox.org/git/20180429202100.32353-1-avarab@gmail.com/
for the notes on that.

A range-diff with v1 follows below.

 2:  a47d861704 !  1:  77a612e89c push tests: fix logic error in "push" test assertion
    @@ -1,17 +1,13 @@
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    -    push tests: fix logic error in "push" test assertion
    +    fetch tests: change "Tag" test tag to "testTag"
     
    -    Fix a logic error that's been here since this test was added in
    -    dbfeddb12e ("push: require force for refs under refs/tags/",
    -    2012-11-29).
    +    Calling the test tag "Tag" will make for confusing reading later in
    +    this series when making use of the "git push tag <name>"
    +    feature. Let's call the tag testTag instead.
     
    -    The intent of this test is to force-create a new tag pointing to
    -    HEAD~, and then assert that pushing it doesn't work without --force.
    -
    -    Instead, the code was not creating a new tag at all, and then failing
    -    to push the previous tag for the unrelated reason of providing a
    -    refspec that doesn't make any sense.
    +    Changes code initially added in dbfeddb12e ("push: require force for
    +    refs under refs/tags/", 2012-11-29).
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ -19,13 +15,30 @@
     --- a/t/t5516-fetch-push.sh
     +++ b/t/t5516-fetch-push.sh
     @@
    - 		git tag -f Tag &&
    - 		test_must_fail git push ../child2 Tag &&
    - 		git push --force ../child2 Tag &&
    + 	mk_child testrepo child2 &&
    + 	(
    + 		cd child1 &&
    +-		git tag Tag &&
    +-		git push ../child2 Tag &&
    +-		git push ../child2 Tag &&
    ++		git tag testTag &&
    ++		git push ../child2 testTag &&
    ++		git push ../child2 testTag &&
    + 		>file1 &&
    + 		git add file1 &&
    + 		git commit -m "file1" &&
    +-		git tag -f Tag &&
    +-		test_must_fail git push ../child2 Tag &&
    +-		git push --force ../child2 Tag &&
     -		git tag -f Tag &&
     -		test_must_fail git push ../child2 Tag HEAD~ &&
    -+		git tag -f Tag HEAD~ &&
    -+		test_must_fail git push ../child2 Tag &&
    - 		git push --force ../child2 Tag
    +-		git push --force ../child2 Tag
    ++		git tag -f testTag &&
    ++		test_must_fail git push ../child2 testTag &&
    ++		git push --force ../child2 testTag &&
    ++		git tag -f testTag &&
    ++		test_must_fail git push ../child2 testTag HEAD~ &&
    ++		git push --force ../child2 testTag
      	)
      '
    + 
 1:  4a3c29b593 !  2:  2386f0c6c6 push tests: remove redundant 'git push' invocation
    @@ -15,9 +15,9 @@
     +++ b/t/t5516-fetch-push.sh
     @@
      		cd child1 &&
    - 		git tag Tag &&
    - 		git push ../child2 Tag &&
    --		git push ../child2 Tag &&
    + 		git tag testTag &&
    + 		git push ../child2 testTag &&
    +-		git push ../child2 testTag &&
      		>file1 &&
      		git add file1 &&
      		git commit -m "file1" &&
 -:  ---------- >  3:  3eaea7c262 push tests: fix logic error in "push" test assertion
 3:  6c54d51a0e !  4:  9dbfb0c058 push tests: add more testing for forced tag pushing
    @@ -4,8 +4,17 @@
     
         Improve the tests added in dbfeddb12e ("push: require force for refs
         under refs/tags/", 2012-11-29) to assert that the same behavior
    -    applies various forms other refspecs, and that "+" in a refspec will
    -    override the "--no-force" option (but not the other way around).
    +    applies various other combinations of command-line option and
    +    refspecs.
    +
    +    Supplying either "+" in refspec or "--force" is sufficient to clobber
    +    the reference. With --no-force we still pay attention to "+" in the
    +    refspec, and vice-versa with clobbering kicking in if there's no "+"
    +    in the refspec but "+" is given.
    +
    +    This is consistent with how refspecs work for branches, where either
    +    "+" or "--force" will enable clobbering, with neither taking priority
    +    over the other.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ -13,21 +22,38 @@
     --- a/t/t5516-fetch-push.sh
     +++ b/t/t5516-fetch-push.sh
     @@
    - 		git push --force ../child2 Tag &&
    - 		git tag -f Tag HEAD~ &&
    - 		test_must_fail git push ../child2 Tag &&
    --		git push --force ../child2 Tag
    -+		git push --force ../child2 Tag &&
    -+		git tag -f Tag &&
    + 	)
    + '
    + 
    +-test_expect_success 'push requires --force to update lightweight tag' '
    ++test_expect_success 'force pushing required to update lightweight tag' '
    + 	mk_test testrepo heads/master &&
    + 	mk_child testrepo child1 &&
    + 	mk_child testrepo child2 &&
    +@@
    + 		git push --force ../child2 testTag &&
    + 		git tag -f testTag HEAD~ &&
    + 		test_must_fail git push ../child2 testTag &&
    +-		git push --force ../child2 testTag
    ++		git push --force ../child2 testTag &&
    ++
    ++		# Clobbering without + in refspec needs --force
    ++		git tag -f testTag &&
     +		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
     +		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
    -+		git tag -f Tag HEAD~ &&
    ++
    ++		# Clobbering with + in refspec does not need --force
    ++		git tag -f testTag HEAD~ &&
     +		git push ../child2 "+refs/tags/*:refs/tags/*" &&
    -+		git tag -f Tag &&
    ++
    ++		# Clobbering with --no-force still obeys + in refspec
    ++		git tag -f testTag &&
     +		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
    -+		git tag -f Tag HEAD~ &&
    -+		test_must_fail git push ../child2 tag Tag &&
    -+		git push --force ../child2 tag Tag
    ++
    ++		# Clobbering with/without --force and "tag <name>" format
    ++		git tag -f testTag HEAD~ &&
    ++		test_must_fail git push ../child2 tag testTag &&
    ++		git push --force ../child2 tag testTag
      	)
      '
      
 4:  0d6b780cb3 !  5:  64bae445e5 push tests: assert re-pushing annotated tags
    @@ -26,66 +26,82 @@
      	)
      '
      
    --test_expect_success 'push requires --force to update lightweight tag' '
    +-test_expect_success 'force pushing required to update lightweight tag' '
     -	mk_test testrepo heads/master &&
     -	mk_child testrepo child1 &&
     -	mk_child testrepo child2 &&
     -	(
     -		cd child1 &&
    --		git tag Tag &&
    --		git push ../child2 Tag &&
    +-		git tag testTag &&
    +-		git push ../child2 testTag &&
     -		>file1 &&
     -		git add file1 &&
     -		git commit -m "file1" &&
    --		git tag -f Tag &&
    --		test_must_fail git push ../child2 Tag &&
    --		git push --force ../child2 Tag &&
    --		git tag -f Tag HEAD~ &&
    --		test_must_fail git push ../child2 Tag &&
    --		git push --force ../child2 Tag &&
    --		git tag -f Tag &&
    +-		git tag -f testTag &&
    +-		test_must_fail git push ../child2 testTag &&
    +-		git push --force ../child2 testTag &&
    +-		git tag -f testTag HEAD~ &&
    +-		test_must_fail git push ../child2 testTag &&
    +-		git push --force ../child2 testTag &&
    +-
    +-		# Clobbering without + in refspec needs --force
    +-		git tag -f testTag &&
     -		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
     -		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
    --		git tag -f Tag HEAD~ &&
    +-
    +-		# Clobbering with + in refspec does not need --force
    +-		git tag -f testTag HEAD~ &&
     -		git push ../child2 "+refs/tags/*:refs/tags/*" &&
    --		git tag -f Tag &&
    +-
    +-		# Clobbering with --no-force still obeys + in refspec
    +-		git tag -f testTag &&
     -		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
    --		git tag -f Tag HEAD~ &&
    --		test_must_fail git push ../child2 tag Tag &&
    --		git push --force ../child2 tag Tag
    +-
    +-		# Clobbering with/without --force and "tag <name>" format
    +-		git tag -f testTag HEAD~ &&
    +-		test_must_fail git push ../child2 tag testTag &&
    +-		git push --force ../child2 tag testTag
     -	)
     -'
     +test_force_push_tag () {
     +	tag_type_description=$1
     +	tag_args=$2
     +
    -+	test_expect_success "push requires --force to update $tag_type_description" "
    ++	test_expect_success 'force pushing required to update lightweight tag' "
     +		mk_test testrepo heads/master &&
     +		mk_child testrepo child1 &&
     +		mk_child testrepo child2 &&
     +		(
     +			cd child1 &&
    -+			git tag Tag &&
    -+			git push ../child2 Tag &&
    ++			git tag testTag &&
    ++			git push ../child2 testTag &&
     +			>file1 &&
     +			git add file1 &&
     +			git commit -m 'file1' &&
    -+			git tag $tag_args Tag &&
    -+			test_must_fail git push ../child2 Tag &&
    -+			git push --force ../child2 Tag &&
    -+			git tag $tag_args Tag HEAD~ &&
    -+			test_must_fail git push ../child2 Tag &&
    -+			git push --force ../child2 Tag &&
    -+			git tag $tag_args Tag &&
    ++			git tag $tag_args testTag &&
    ++			test_must_fail git push ../child2 testTag &&
    ++			git push --force ../child2 testTag &&
    ++			git tag $tag_args testTag HEAD~ &&
    ++			test_must_fail git push ../child2 testTag &&
    ++			git push --force ../child2 testTag &&
    ++
    ++			# Clobbering without + in refspec needs --force
    ++			git tag -f testTag &&
     +			test_must_fail git push ../child2 'refs/tags/*:refs/tags/*' &&
     +			git push --force ../child2 'refs/tags/*:refs/tags/*' &&
    -+			git tag $tag_args Tag HEAD~ &&
    ++
    ++			# Clobbering with + in refspec does not need --force
    ++			git tag -f testTag HEAD~ &&
     +			git push ../child2 '+refs/tags/*:refs/tags/*' &&
    -+			git tag $tag_args Tag &&function
    ++
    ++			# Clobbering with --no-force still obeys + in refspec
    ++			git tag -f testTag &&
     +			git push --no-force ../child2 '+refs/tags/*:refs/tags/*' &&
    -+			git tag $tag_args Tag HEAD~ &&
    -+			test_must_fail git push ../child2 tag Tag &&
    -+			git push --force ../child2 tag Tag
    ++
    ++			# Clobbering with/without --force and 'tag <name>' format
    ++			git tag -f testTag HEAD~ &&
    ++			test_must_fail git push ../child2 tag testTag &&
    ++			git push --force ../child2 tag testTag
     +		)
     +	"
     +}
 5:  277fa440a7 !  6:  2209f03463 push doc: correct lies about how push refspecs work
    @@ -36,10 +36,9 @@
      The <src> is often the name of the branch you would want to push, but
     -it can be any arbitrary "SHA-1 expression", such as `master~4` or
     -`HEAD` (see linkgit:gitrevisions[7]).
    -+it can be any arbitrary "SHA-1 expression" referring to a branch, such
    -+as `master~4` or `HEAD` (see linkgit:gitrevisions[7]). It can also
    -+refer to tag objects, trees or blobs if the <dst> is outside of
    -+`refs/heads/*`.
    ++it can be any arbitrary expression to a commit, such as `master~4` or
    ++`HEAD` (see linkgit:gitrevisions[7]). It can also refer to tag
    ++objects, trees or blobs if the <dst> is outside of `refs/heads/*`.
      +
      The <dst> tells which ref on the remote side is updated with this
      push. Arbitrary expressions cannot be used here, an actual ref must
    @@ -53,24 +52,25 @@
     -the <dst> ref even if it is not allowed by default (e.g., it is not a
     -fast-forward.)  This does *not* attempt to merge <src> into <dst>.  See
     -EXAMPLES below for details.
    -+on the remote side. Whether this is allowed depends on what where in
    ++on the remote side. Whether this is allowed depends on where in
     +`refs/*` the <dst> reference lives. The `refs/heads/*` namespace will
     +only accept commit objects, and then only they can be
     +fast-forwarded. The `refs/tags/*` namespace will accept any kind of
    -+object, but there commit objects are known as lightweight tags, and
    -+any changes to them and others types of objects will be
    -+rejected. Finally and most confusingly, it's possible to push any type
    -+of object to any namespace outside of `refs/{tags,heads}/*`, but these
    -+will be treated as branches, even in the case where a tag object is
    -+pushed. That tag object will be overwritten by another tag object (or
    -+commit!) without `--force` if the new tag happens to point to a commit
    -+that's a fast-forward of the commit it replaces.
    ++object, and any changes to them and others types of objects will be
    ++rejected. Finally, it's possible to push any type of object to any
    ++namespace outside of `refs/{tags,heads}/*`, but these will be treated
    ++as branches for the purposes of whether `--force` is required, even in
    ++the case where a tag object is pushed. That tag object will be
    ++overwritten by another tag object (or commit!) without `--force` if
    ++the new tag happens to point to a commit that's a fast-forward of the
    ++commit it replaces.
     ++
    -+By having the optional leading `+`, you can tell Git to update the
    -+<dst> ref even if it is not allowed by its respective namespace
    -+clobbering rules (e.g., it is not a fast-forward. in the case of
    -+`refs/heads/*` updates) This does *not* attempt to merge <src> into
    -+<dst>.  See EXAMPLES below for details.
    ++By having the optional leading `+` to a refspec (or using `--force`
    ++command line option) you can tell Git to update the <dst> ref even if
    ++it is not allowed by its respective namespace clobbering rules (e.g.,
    ++it is not a fast-forward. in the case of `refs/heads/*` updates) This
    ++does *not* attempt to merge <src> into <dst>.  See EXAMPLES below for
    ++details.
      +
      `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
      +
 6:  158fcc08d2 =  7:  844f4fb491 fetch tests: correct a comment "remove it" -> "remove them"
 7:  0134b9353f =  8:  cf9abd7c69 fetch tests: add a test clobbering tag behavior
 -:  ---------- >  9:  af1e1482a2 pull doc: fix a long-standing grammar error
 8:  57b7bc325d ! 10:  9c508fb926 fetch: stop clobbering existing tags without --force
    @@ -7,31 +7,34 @@
     
         This changes the long-standing behavior of "fetch" added in
         853a3697dc ("[PATCH] Multi-head fetch.", 2005-08-20), before this
    -    change all tag fetches effectively had --force enabled. The original
    -    rationale in that change was:
    +    change all tag fetches effectively had --force enabled. See the
    +    git-fetch-script code in fast_forward_local() with the comment:
     
             > Tags need not be pointing at commits so there is no way to
             > guarantee "fast-forward" anyway.
     
    -    That comment and the rest of the history of "fetch" shows that the
    +    That commit and the rest of the history of "fetch" shows that the
         "+" (--force) part of refpecs was only conceived for branch updates,
         while tags have accepted any changes from upstream unconditionally and
         clobbered the local tag object. Changing this behavior has been
         discussed as early as 2011[1].
     
    -    I the current behavior doesn't make sense, it easily results in local
    -    tags accidentally being clobbered. Ideally we'd namespace our tags
    -    per-remote, but as with my 97716d217c ("fetch: add a --prune-tags
    -    option and fetch.pruneTags config", 2018-02-09) it's easier to work
    -    around the current implementation than to fix the root cause, so this
    -    implements suggestion #1 from [1], "fetch" now only clobbers the tag
    -    if either "+" is provided as part of the refspec, or if "--force" is
    -    provided on the command-line.
    +    The current behavior doesn't make sense to me, it easily results in
    +    local tags accidentally being clobbered.  We could namespace our tags
    +    per-remote and not locally populate refs/tags/*, but as with my
    +    97716d217c ("fetch: add a --prune-tags option and fetch.pruneTags
    +    config", 2018-02-09) it's easier to work around the current
    +    implementation than to fix the root cause.
     
    -    This also makes it nicely symmetrical with how "tag" itself
    -    works. We'll now refuse to clobber any existing tags unless "--force"
    -    is supplied, whether that clobbering would happen by clobbering a
    -    local tag with "tag", or by fetching it from the remote with "fetch".
    +    So this change implements suggestion #1 from Jeff's 2011 E-Mail[1],
    +    "fetch" now only clobbers the tag if either "+" is provided as part of
    +    the refspec, or if "--force" is provided on the command-line.
    +
    +    This also makes it nicely symmetrical with how "tag" itself works when
    +    creating tags. I.e. we refuse to clobber any existing tags unless
    +    "--force" is supplied. Now we can refuse all such clobbering, whether
    +    it would happen by clobbering a local tag with "tag", or by fetching
    +    it from the remote with "fetch".
     
         It's still not at all nicely symmetrical with how "git push" works, as
         discussed in the updated pull-fetch-param.txt documentation, but this
    @@ -60,7 +63,7 @@
     -	`<lbranch>` unless the remote branch `<rbranch>` it
     -	fetches is a descendant of `<lbranch>`.  This option
     -	overrides that check.
    -+	When 'git fetch' is used with `<src>:<dst>` refspec it might
    ++	When 'git fetch' is used with `<src>:<dst>` refspec it may
     +	refuse to update the local branch as discussed
     +ifdef::git-pull[]
     +	in the `<refspec>` part of the linkgit:git-fetch[1]
    @@ -78,31 +81,29 @@
     --- a/Documentation/pull-fetch-param.txt
     +++ b/Documentation/pull-fetch-param.txt
     @@
    - `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`;
      it requests fetching everything up to the given tag.
      +
    --The remote ref that matches <src>
    --is fetched, and if <dst> is not empty string, the local
    + The remote ref that matches <src>
    +-is fetched, and if <dst> is not an empty string, the local
     -ref that matches it is fast-forwarded using <src>.
     -If the optional plus `+` is used, the local ref
     -is updated even if it does not result in a fast-forward
     -update.
    -+The remote ref that matches <src> is fetched, and if <dst> is not
    -+empty string, an attempt is made to update the local ref that matches
    -+it.
    ++is fetched, and if <dst> is not an empty string, an attempt
    ++is made to update the local ref that matches it.
    +++
    ++Whether that update is allowed without `--force` depends on the ref
    ++namespace it's being fetched to, and the type of object being
    ++fetched. If it's a commit under `refs/heads/*` only fast-forwards are
    ++allowed.
     ++
    -+Whether that update is allowed is confusingly not the inverse of
    -+whether a server will accept a push as described in the `<refspec>...`
    -+section of linkgit:git-push[1]. If it's a commit under `refs/heads/*`
    -+only fast-forwards are allowed, but unlike what linkgit:git-push[1]
    -+will accept clobbering any ref pointing to blobs, trees etc. in any
    -+other namespace will be accepted, but commits in any ref
    -+namespace. Those apply the same fast-forward rule. An exception to
    -+this is that as of Git version 2.18 any object under `refs/tags/*` is
    -+protected from updates.
    ++By having the optional leading `+` to a refspec (or using `--force`
    ++command line option) you can tell Git to update the local ref even if
    ++it is not allowed by its respective namespace clobbering rules.
     ++
    -+If the optional plus `+` is used, the local ref is updated if the
    -+update would have otherwise been rejected.
    ++Before Git version 2.19 tag objects under `refs/tags/*` would not be
    ++protected from updates, but since then the `+` (or `--force`) syntax
    ++is required to clobber them.
      +
      [NOTE]
      When the remote branch you want to fetch is known to

Ævar Arnfjörð Bjarmason (10):
  fetch tests: change "Tag" test tag to "testTag"
  push tests: remove redundant 'git push' invocation
  push tests: fix logic error in "push" test assertion
  push tests: add more testing for forced tag pushing
  push tests: assert re-pushing annotated tags
  push doc: correct lies about how push refspecs work
  fetch tests: correct a comment "remove it" -> "remove them"
  fetch tests: add a test clobbering tag behavior
  pull doc: fix a long-standing grammar error
  fetch: stop clobbering existing tags without --force

 Documentation/fetch-options.txt    | 15 +++--
 Documentation/git-push.txt         | 30 +++++++---
 Documentation/gitrevisions.txt     |  7 ++-
 Documentation/pull-fetch-param.txt | 20 +++++--
 builtin/fetch.c                    | 20 ++++---
 t/t5510-fetch.sh                   |  2 +-
 t/t5516-fetch-push.sh              | 90 +++++++++++++++++++++++-------
 t/t5612-clone-refspec.sh           |  4 +-
 8 files changed, 137 insertions(+), 51 deletions(-)

-- 
2.18.0.345.g5c9ce644c3

[PATCH v2 01/10] fetch tests: change "Tag" test tag to "testTag"

[Ævar Arnfjörð Bjarmason]

Calling the test tag "Tag" will make for confusing reading later in
this series when making use of the "git push tag <name>"
feature. Let's call the tag testTag instead.

Changes code initially added in dbfeddb12e ("push: require force for
refs under refs/tags/", 2012-11-29).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index a5077d8b7c..08b9cf581d 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -971,18 +971,18 @@ test_expect_success 'push requires --force to update lightweight tag' '
 	mk_child testrepo child2 &&
 	(
 		cd child1 &&
-		git tag Tag &&
-		git push ../child2 Tag &&
-		git push ../child2 Tag &&
+		git tag testTag &&
+		git push ../child2 testTag &&
+		git push ../child2 testTag &&
 		>file1 &&
 		git add file1 &&
 		git commit -m "file1" &&
-		git tag -f Tag &&
-		test_must_fail git push ../child2 Tag &&
-		git push --force ../child2 Tag &&
-		git tag -f Tag &&
-		test_must_fail git push ../child2 Tag HEAD~ &&
-		git push --force ../child2 Tag
+		git tag -f testTag &&
+		test_must_fail git push ../child2 testTag &&
+		git push --force ../child2 testTag &&
+		git tag -f testTag &&
+		test_must_fail git push ../child2 testTag HEAD~ &&
+		git push --force ../child2 testTag
 	)
 '
 
-- 
2.18.0.345.g5c9ce644c3

[PATCH v2 02/10] push tests: remove redundant 'git push' invocation

[Ævar Arnfjörð Bjarmason]

Remove an invocation of 'git push' that's exactly the same as the one
on the preceding line. This was seemingly added by mistake in
dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29) and doesn't affect the result of the test, the second
"push" was a no-op as there was nothing new to push.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 08b9cf581d..4d487d6875 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -973,7 +973,6 @@ test_expect_success 'push requires --force to update lightweight tag' '
 		cd child1 &&
 		git tag testTag &&
 		git push ../child2 testTag &&
-		git push ../child2 testTag &&
 		>file1 &&
 		git add file1 &&
 		git commit -m "file1" &&
-- 
2.18.0.345.g5c9ce644c3

[PATCH v2 03/10] push tests: fix logic error in "push" test assertion

[Ævar Arnfjörð Bjarmason]

Fix a logic error that's been here since this test was added in
dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29).

The intent of this test is to force-create a new tag pointing to
HEAD~, and then assert that pushing it doesn't work without --force.

Instead, the code was not creating a new tag at all, and then failing
to push the previous tag for the unrelated reason of providing a
refspec that doesn't make any sense.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4d487d6875..82af990ab3 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -979,8 +979,8 @@ test_expect_success 'push requires --force to update lightweight tag' '
 		git tag -f testTag &&
 		test_must_fail git push ../child2 testTag &&
 		git push --force ../child2 testTag &&
-		git tag -f testTag &&
-		test_must_fail git push ../child2 testTag HEAD~ &&
+		git tag -f testTag HEAD~ &&
+		test_must_fail git push ../child2 testTag &&
 		git push --force ../child2 testTag
 	)
 '
-- 
2.18.0.345.g5c9ce644c3

[PATCH v2 04/10] push tests: add more testing for forced tag pushing

[Ævar Arnfjörð Bjarmason]

Improve the tests added in dbfeddb12e ("push: require force for refs
under refs/tags/", 2012-11-29) to assert that the same behavior
applies various other combinations of command-line option and
refspecs.

Supplying either "+" in refspec or "--force" is sufficient to clobber
the reference. With --no-force we still pay attention to "+" in the
refspec, and vice-versa with clobbering kicking in if there's no "+"
in the refspec but "+" is given.

This is consistent with how refspecs work for branches, where either
"+" or "--force" will enable clobbering, with neither taking priority
over the other.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 82af990ab3..4bd533dd48 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -965,7 +965,7 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 	)
 '
 
-test_expect_success 'push requires --force to update lightweight tag' '
+test_expect_success 'force pushing required to update lightweight tag' '
 	mk_test testrepo heads/master &&
 	mk_child testrepo child1 &&
 	mk_child testrepo child2 &&
@@ -981,7 +981,25 @@ test_expect_success 'push requires --force to update lightweight tag' '
 		git push --force ../child2 testTag &&
 		git tag -f testTag HEAD~ &&
 		test_must_fail git push ../child2 testTag &&
-		git push --force ../child2 testTag
+		git push --force ../child2 testTag &&
+
+		# Clobbering without + in refspec needs --force
+		git tag -f testTag &&
+		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
+		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
+
+		# Clobbering with + in refspec does not need --force
+		git tag -f testTag HEAD~ &&
+		git push ../child2 "+refs/tags/*:refs/tags/*" &&
+
+		# Clobbering with --no-force still obeys + in refspec
+		git tag -f testTag &&
+		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
+
+		# Clobbering with/without --force and "tag <name>" format
+		git tag -f testTag HEAD~ &&
+		test_must_fail git push ../child2 tag testTag &&
+		git push --force ../child2 tag testTag
 	)
 '
 
-- 
2.18.0.345.g5c9ce644c3

[PATCH v2 05/10] push tests: assert re-pushing annotated tags

[Ævar Arnfjörð Bjarmason]

Change the test that asserts that lightweight tags can only be
clobbered by a force-push to check do the same tests for annotated
tags.

There used to be less exhaustive tests for this with the code added in
40eff17999 ("push: require force for annotated tags", 2012-11-29), but
Junio removed them in 256b9d70a4 ("push: fix "refs/tags/ hierarchy
cannot be updated without --force"", 2013-01-16) while fixing some of
the behavior around tag pushing.

That change left us without any coverage asserting that pushing and
clobbering annotated tags worked as intended.  There was no reason to
suspect that the receive machinery wouldn't behave the same way with
annotated tags, but now we know for sure.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 82 ++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 37 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4bd533dd48..1331a8de08 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -965,43 +965,51 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 	)
 '
 
-test_expect_success 'force pushing required to update lightweight tag' '
-	mk_test testrepo heads/master &&
-	mk_child testrepo child1 &&
-	mk_child testrepo child2 &&
-	(
-		cd child1 &&
-		git tag testTag &&
-		git push ../child2 testTag &&
-		>file1 &&
-		git add file1 &&
-		git commit -m "file1" &&
-		git tag -f testTag &&
-		test_must_fail git push ../child2 testTag &&
-		git push --force ../child2 testTag &&
-		git tag -f testTag HEAD~ &&
-		test_must_fail git push ../child2 testTag &&
-		git push --force ../child2 testTag &&
-
-		# Clobbering without + in refspec needs --force
-		git tag -f testTag &&
-		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
-		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
-
-		# Clobbering with + in refspec does not need --force
-		git tag -f testTag HEAD~ &&
-		git push ../child2 "+refs/tags/*:refs/tags/*" &&
-
-		# Clobbering with --no-force still obeys + in refspec
-		git tag -f testTag &&
-		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
-
-		# Clobbering with/without --force and "tag <name>" format
-		git tag -f testTag HEAD~ &&
-		test_must_fail git push ../child2 tag testTag &&
-		git push --force ../child2 tag testTag
-	)
-'
+test_force_push_tag () {
+	tag_type_description=$1
+	tag_args=$2
+
+	test_expect_success 'force pushing required to update lightweight tag' "
+		mk_test testrepo heads/master &&
+		mk_child testrepo child1 &&
+		mk_child testrepo child2 &&
+		(
+			cd child1 &&
+			git tag testTag &&
+			git push ../child2 testTag &&
+			>file1 &&
+			git add file1 &&
+			git commit -m 'file1' &&
+			git tag $tag_args testTag &&
+			test_must_fail git push ../child2 testTag &&
+			git push --force ../child2 testTag &&
+			git tag $tag_args testTag HEAD~ &&
+			test_must_fail git push ../child2 testTag &&
+			git push --force ../child2 testTag &&
+
+			# Clobbering without + in refspec needs --force
+			git tag -f testTag &&
+			test_must_fail git push ../child2 'refs/tags/*:refs/tags/*' &&
+			git push --force ../child2 'refs/tags/*:refs/tags/*' &&
+
+			# Clobbering with + in refspec does not need --force
+			git tag -f testTag HEAD~ &&
+			git push ../child2 '+refs/tags/*:refs/tags/*' &&
+
+			# Clobbering with --no-force still obeys + in refspec
+			git tag -f testTag &&
+			git push --no-force ../child2 '+refs/tags/*:refs/tags/*' &&
+
+			# Clobbering with/without --force and 'tag <name>' format
+			git tag -f testTag HEAD~ &&
+			test_must_fail git push ../child2 tag testTag &&
+			git push --force ../child2 tag testTag
+		)
+	"
+}
+
+test_force_push_tag "lightweight tag" "-f"
+test_force_push_tag "annotated tag" "-f -a -m'msg'"
 
 test_expect_success 'push --porcelain' '
 	mk_empty testrepo &&
-- 
2.18.0.345.g5c9ce644c3

[PATCH v2 06/10] push doc: correct lies about how push refspecs work

[Ævar Arnfjörð Bjarmason]

There's complex rules governing whether a push is allowed to take
place depending on whether we're pushing to refs/heads/*, refs/tags/*
or refs/not-that/*. See is_branch() in refs.c, and the various
assertions in refs/files-backend.c. (e.g. "trying to write non-commit
object %s to branch '%s'").

This documentation has never been quite correct, but went downhill
after dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29) when we started claiming that <dst> couldn't be a tag
object, which is incorrect. After some of the logic in that patch was
changed in 256b9d70a4 ("push: fix "refs/tags/ hierarchy cannot be
updated without --force"", 2013-01-16) the docs weren't updated, and
we've had some version of documentation that confused whether <src>
was a tag or not with whether <dst> would accept either an annotated
tag object or the commit it points to.

This makes the intro somewhat more verbose & complex, perhaps we
should have a shorter description here and split the full complexity
into a dedicated section. Very few users will find themselves needing
to e.g. push blobs or trees to refs/custom-namespace/* (or blobs or
trees at all), and that could be covered separately as an advanced
topic.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-push.txt     | 30 ++++++++++++++++++++++--------
 Documentation/gitrevisions.txt |  7 ++++---
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 55277a9781..fe654482dc 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -60,8 +60,9 @@ OPTIONS[[OPTIONS]]
 	by a colon `:`, followed by the destination ref <dst>.
 +
 The <src> is often the name of the branch you would want to push, but
-it can be any arbitrary "SHA-1 expression", such as `master~4` or
-`HEAD` (see linkgit:gitrevisions[7]).
+it can be any arbitrary expression to a commit, such as `master~4` or
+`HEAD` (see linkgit:gitrevisions[7]). It can also refer to tag
+objects, trees or blobs if the <dst> is outside of `refs/heads/*`.
 +
 The <dst> tells which ref on the remote side is updated with this
 push. Arbitrary expressions cannot be used here, an actual ref must
@@ -74,12 +75,25 @@ without any `<refspec>` on the command line.  Otherwise, missing
 `:<dst>` means to update the same ref as the `<src>`.
 +
 The object referenced by <src> is used to update the <dst> reference
-on the remote side.  By default this is only allowed if <dst> is not
-a tag (annotated or lightweight), and then only if it can fast-forward
-<dst>.  By having the optional leading `+`, you can tell Git to update
-the <dst> ref even if it is not allowed by default (e.g., it is not a
-fast-forward.)  This does *not* attempt to merge <src> into <dst>.  See
-EXAMPLES below for details.
+on the remote side. Whether this is allowed depends on where in
+`refs/*` the <dst> reference lives. The `refs/heads/*` namespace will
+only accept commit objects, and then only they can be
+fast-forwarded. The `refs/tags/*` namespace will accept any kind of
+object, and any changes to them and others types of objects will be
+rejected. Finally, it's possible to push any type of object to any
+namespace outside of `refs/{tags,heads}/*`, but these will be treated
+as branches for the purposes of whether `--force` is required, even in
+the case where a tag object is pushed. That tag object will be
+overwritten by another tag object (or commit!) without `--force` if
+the new tag happens to point to a commit that's a fast-forward of the
+commit it replaces.
++
+By having the optional leading `+` to a refspec (or using `--force`
+command line option) you can tell Git to update the <dst> ref even if
+it is not allowed by its respective namespace clobbering rules (e.g.,
+it is not a fast-forward. in the case of `refs/heads/*` updates) This
+does *not* attempt to merge <src> into <dst>.  See EXAMPLES below for
+details.
 +
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
 +
diff --git a/Documentation/gitrevisions.txt b/Documentation/gitrevisions.txt
index 1f6cceaefb..d407b7dee1 100644
--- a/Documentation/gitrevisions.txt
+++ b/Documentation/gitrevisions.txt
@@ -19,9 +19,10 @@ walk the revision graph (such as linkgit:git-log[1]), all commits which are
 reachable from that commit. For commands that walk the revision graph one can
 also specify a range of revisions explicitly.
 
-In addition, some Git commands (such as linkgit:git-show[1]) also take
-revision parameters which denote other objects than commits, e.g. blobs
-("files") or trees ("directories of files").
+In addition, some Git commands (such as linkgit:git-show[1] and
+linkgit:git-push[1]) can also take revision parameters which denote
+other objects than commits, e.g. blobs ("files") or trees
+("directories of files").
 
 include::revisions.txt[]
 
-- 
2.18.0.345.g5c9ce644c3

[PATCH v2 07/10] fetch tests: correct a comment "remove it" -> "remove them"

[Ævar Arnfjörð Bjarmason]

Correct a comment referring to the removal of just the branch to also
refer to the tag. This should have been changed in my
ca3065e7e7 ("fetch tests: add a tag to be deleted to the pruning
tests", 2018-02-09) when the tag deletion was added, but I missed it
at the time.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5510-fetch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index e402aee6a2..6ab093207a 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -613,7 +613,7 @@ test_configured_prune_type () {
 			git rev-parse --verify refs/tags/newtag
 		) &&
 
-		# now remove it
+		# now remove them
 		git branch -d newbranch &&
 		git tag -d newtag &&
 
-- 
2.18.0.345.g5c9ce644c3

[PATCH v2 08/10] fetch tests: add a test clobbering tag behavior

[Ævar Arnfjörð Bjarmason]

The test suite only incidentally (and unintentionally) tested for the
current behavior of eager tag clobbering on "fetch". This follow-up to
the previous "push tests: assert re-pushing annotated tags" change
tests for it explicitly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 1331a8de08..8912312be7 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1011,6 +1011,30 @@ test_force_push_tag () {
 test_force_push_tag "lightweight tag" "-f"
 test_force_push_tag "annotated tag" "-f -a -m'msg'"
 
+test_force_fetch_tag () {
+	tag_type_description=$1
+	tag_args=$2
+
+	test_expect_success "fetch will clobber an existing $tag_type_description" "
+		mk_test testrepo heads/master &&
+		mk_child testrepo child1 &&
+		mk_child testrepo child2 &&
+		(
+			cd testrepo &&
+			git tag Tag &&
+			git -C ../child1 fetch origin tag Tag &&
+			>file1 &&
+			git add file1 &&
+			git commit -m 'file1' &&
+			git tag $tag_args Tag &&
+			git -C ../child1 fetch origin tag Tag
+		)
+	"
+}
+
+test_force_fetch_tag "lightweight tag" "-f"
+test_force_fetch_tag "annotated tag" "-f -a -m'msg'"
+
 test_expect_success 'push --porcelain' '
 	mk_empty testrepo &&
 	echo >.git/foo  "To testrepo" &&
-- 
2.18.0.345.g5c9ce644c3

[PATCH v2 09/10] pull doc: fix a long-standing grammar error

[Ævar Arnfjörð Bjarmason]

It should be "is not an empty string" not "is not empty string". This
fixes wording originally introduced in ab9b31386b ("Documentation:
multi-head fetch.", 2005-08-24).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pull-fetch-param.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
index c579793af5..f1fb08dc68 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -33,7 +33,7 @@ name.
 it requests fetching everything up to the given tag.
 +
 The remote ref that matches <src>
-is fetched, and if <dst> is not empty string, the local
+is fetched, and if <dst> is not an empty string, the local
 ref that matches it is fast-forwarded using <src>.
 If the optional plus `+` is used, the local ref
 is updated even if it does not result in a fast-forward
-- 
2.18.0.345.g5c9ce644c3

[PATCH v2 10/10] fetch: stop clobbering existing tags without --force

[Ævar Arnfjörð Bjarmason]

Change "fetch" to treat "+" in refspecs (aka --force) to mean we
should clobber a local tag of the same name.

This changes the long-standing behavior of "fetch" added in
853a3697dc ("[PATCH] Multi-head fetch.", 2005-08-20), before this
change all tag fetches effectively had --force enabled. See the
git-fetch-script code in fast_forward_local() with the comment:

    > Tags need not be pointing at commits so there is no way to
    > guarantee "fast-forward" anyway.

That commit and the rest of the history of "fetch" shows that the
"+" (--force) part of refpecs was only conceived for branch updates,
while tags have accepted any changes from upstream unconditionally and
clobbered the local tag object. Changing this behavior has been
discussed as early as 2011[1].

The current behavior doesn't make sense to me, it easily results in
local tags accidentally being clobbered.  We could namespace our tags
per-remote and not locally populate refs/tags/*, but as with my
97716d217c ("fetch: add a --prune-tags option and fetch.pruneTags
config", 2018-02-09) it's easier to work around the current
implementation than to fix the root cause.

So this change implements suggestion #1 from Jeff's 2011 E-Mail[1],
"fetch" now only clobbers the tag if either "+" is provided as part of
the refspec, or if "--force" is provided on the command-line.

This also makes it nicely symmetrical with how "tag" itself works when
creating tags. I.e. we refuse to clobber any existing tags unless
"--force" is supplied. Now we can refuse all such clobbering, whether
it would happen by clobbering a local tag with "tag", or by fetching
it from the remote with "fetch".

It's still not at all nicely symmetrical with how "git push" works, as
discussed in the updated pull-fetch-param.txt documentation, but this
change brings them more into line with one another. I don't think
there's any reason "fetch" couldn't fully converge with the behavior
used by "push", but that's a topic for another change.

One of the tests added in 31b808a032 ("clone --single: limit the fetch
refspec to fetched branch", 2012-09-20) is being changed to use
--force where a clone would clobber a tag. This changes nothing about
the existing behavior of the test.

1. https://public-inbox.org/git/20111123221658.GA22313@sigill.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/fetch-options.txt    | 15 ++++++++++-----
 Documentation/pull-fetch-param.txt | 20 +++++++++++++++-----
 builtin/fetch.c                    | 20 +++++++++++++-------
 t/t5516-fetch-push.sh              |  5 +++--
 t/t5612-clone-refspec.sh           |  4 ++--
 5 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 97d3217df9..5b624caf58 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -49,11 +49,16 @@ endif::git-pull[]
 
 -f::
 --force::
-	When 'git fetch' is used with `<rbranch>:<lbranch>`
-	refspec, it refuses to update the local branch
-	`<lbranch>` unless the remote branch `<rbranch>` it
-	fetches is a descendant of `<lbranch>`.  This option
-	overrides that check.
+	When 'git fetch' is used with `<src>:<dst>` refspec it may
+	refuse to update the local branch as discussed
+ifdef::git-pull[]
+	in the `<refspec>` part of the linkgit:git-fetch[1]
+	documentation.
+endif::git-pull[]
+ifndef::git-pull[]
+	in the `<refspec>` part below.
+endif::git-pull[]
+	This option overrides that check.
 
 -k::
 --keep::
diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
index f1fb08dc68..acb8e1a4f0 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -33,11 +33,21 @@ name.
 it requests fetching everything up to the given tag.
 +
 The remote ref that matches <src>
-is fetched, and if <dst> is not an empty string, the local
-ref that matches it is fast-forwarded using <src>.
-If the optional plus `+` is used, the local ref
-is updated even if it does not result in a fast-forward
-update.
+is fetched, and if <dst> is not an empty string, an attempt
+is made to update the local ref that matches it.
++
+Whether that update is allowed without `--force` depends on the ref
+namespace it's being fetched to, and the type of object being
+fetched. If it's a commit under `refs/heads/*` only fast-forwards are
+allowed.
++
+By having the optional leading `+` to a refspec (or using `--force`
+command line option) you can tell Git to update the local ref even if
+it is not allowed by its respective namespace clobbering rules.
++
+Before Git version 2.19 tag objects under `refs/tags/*` would not be
+protected from updates, but since then the `+` (or `--force`) syntax
+is required to clobber them.
 +
 [NOTE]
 When the remote branch you want to fetch is known to
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ac06f6a576..683f70d71e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -113,7 +113,7 @@ static struct option builtin_fetch_options[] = {
 		 N_("append to .git/FETCH_HEAD instead of overwriting")),
 	OPT_STRING(0, "upload-pack", &upload_pack, N_("path"),
 		   N_("path to upload pack on remote end")),
-	OPT__FORCE(&force, N_("force overwrite of local branch"), 0),
+	OPT__FORCE(&force, N_("force overwrite of local reference"), 0),
 	OPT_BOOL('m', "multiple", &multiple,
 		 N_("fetch from multiple remotes")),
 	OPT_SET_INT('t', "tags", &tags,
@@ -664,12 +664,18 @@ static int update_local_ref(struct ref *ref,
 
 	if (!is_null_oid(&ref->old_oid) &&
 	    starts_with(ref->name, "refs/tags/")) {
-		int r;
-		r = s_update_ref("updating tag", ref, 0);
-		format_display(display, r ? '!' : 't', _("[tag update]"),
-			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref, summary_width);
-		return r;
+		if (force || ref->force) {
+			int r;
+			r = s_update_ref("updating tag", ref, 0);
+			format_display(display, r ? '!' : 't', _("[tag update]"),
+				       r ? _("unable to update local ref") : NULL,
+				       remote, pretty_ref, summary_width);
+			return r;
+		} else {
+			format_display(display, '!', _("[rejected]"), _("would clobber existing tag"),
+				       remote, pretty_ref, summary_width);
+			return 1;
+		}
 	}
 
 	current = lookup_commit_reference_gently(&ref->old_oid, 1);
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8912312be7..8647f9cc9e 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1015,7 +1015,7 @@ test_force_fetch_tag () {
 	tag_type_description=$1
 	tag_args=$2
 
-	test_expect_success "fetch will clobber an existing $tag_type_description" "
+	test_expect_success "fetch will not clobber an existing $tag_type_description without --force" "
 		mk_test testrepo heads/master &&
 		mk_child testrepo child1 &&
 		mk_child testrepo child2 &&
@@ -1027,7 +1027,8 @@ test_force_fetch_tag () {
 			git add file1 &&
 			git commit -m 'file1' &&
 			git tag $tag_args Tag &&
-			git -C ../child1 fetch origin tag Tag
+			test_must_fail git -C ../child1 fetch origin tag Tag &&
+			git -C ../child1 fetch origin '+refs/tags/*:refs/tags/*'
 		)
 	"
 }
diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
index fac5a73851..6ea8f50dae 100755
--- a/t/t5612-clone-refspec.sh
+++ b/t/t5612-clone-refspec.sh
@@ -104,7 +104,7 @@ test_expect_success 'clone with --no-tags' '
 test_expect_success '--single-branch while HEAD pointing at master' '
 	(
 		cd dir_master &&
-		git fetch &&
+		git fetch --force &&
 		git for-each-ref refs/remotes/origin |
 		sed -e "/HEAD$/d" \
 		    -e "s|/remotes/origin/|/heads/|" >../actual
@@ -115,7 +115,7 @@ test_expect_success '--single-branch while HEAD pointing at master' '
 	test_cmp expect actual &&
 	(
 		cd dir_master &&
-		git fetch --tags &&
+		git fetch --tags --force &&
 		git for-each-ref refs/tags >../actual
 	) &&
 	git for-each-ref refs/tags >expect &&
-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  The <src> is often the name of the branch you would want to push, but
> -it can be any arbitrary "SHA-1 expression", such as `master~4` or
> -`HEAD` (see linkgit:gitrevisions[7]).
> +it can be any arbitrary expression to a commit, such as `master~4` or
> +`HEAD` (see linkgit:gitrevisions[7]). It can also refer to tag
> +objects, trees or blobs if the <dst> is outside of `refs/heads/*`.

"It can also refer to..." is a good addition, but do you really want
to make it part of this series to change/deprecate "SHA-1 expression"
(which would certainly involve discussion on "then what to call them
instead, now we are trying to refrain from saying SHA-1?")?

> +on the remote side. Whether this is allowed depends on where in
> +`refs/*` the <dst> reference lives. The `refs/heads/*` namespace will
> +only accept commit objects, and then only they can be
> +fast-forwarded. The `refs/tags/*` namespace will accept any kind of
> +object, and any changes to them and others types of objects will be
> +rejected. Finally, it's possible to push any type of object to any
> +namespace outside of `refs/{tags,heads}/*`,

All sound correct.

> but these will be treated
> +as branches for the purposes of whether `--force` is required, even in
> +the case where a tag object is pushed.

I am not sure what "will be treated as branches" exactly means.
Does it mean "as if they were in refs/heads/* hierarchy?"  Or
something else?

> That tag object will be
> +overwritten by another tag object (or commit!) without `--force` if
> +the new tag happens to point to a commit that's a fast-forward of the
> +commit it replaces.

Yup, and that is something we want to fix with a later part of this
series.

> +By having the optional leading `+` to a refspec (or using `--force`
> +command line option) you can tell Git to update the <dst> ref even if
> +it is not allowed by its respective namespace clobbering rules (e.g.,
> +it is not a fast-forward. in the case of `refs/heads/*` updates).

This gives an impression that with "--force" you can put non-commit
inside refs/heads/* hierarchy.  Is that correct (if so we probably
would want to fix that behaviour)?

> +This
> +does *not* attempt to merge <src> into <dst>.  See EXAMPLES below for
> +details.

That is not wrong per-se, but would normal people expect a merge to
happen upon pushing on the other side, I wonder?

Thanks for cleaning up our longstanding mess.

Re: [PATCH v2 08/10] fetch tests: add a test clobbering tag behavior

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The test suite only incidentally (and unintentionally) tested for the
> current behavior of eager tag clobbering on "fetch". This follow-up to
> the previous "push tests: assert re-pushing annotated tags" change
> tests for it explicitly.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t5516-fetch-push.sh | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 1331a8de08..8912312be7 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1011,6 +1011,30 @@ test_force_push_tag () {
>  test_force_push_tag "lightweight tag" "-f"
>  test_force_push_tag "annotated tag" "-f -a -m'msg'"
>  
> +test_force_fetch_tag () {
> +	tag_type_description=$1
> +	tag_args=$2
> +
> +	test_expect_success "fetch will clobber an existing $tag_type_description" "
> +		mk_test testrepo heads/master &&
> +		mk_child testrepo child1 &&
> +		mk_child testrepo child2 &&
> +		(
> +			cd testrepo &&
> +			git tag Tag &&
> +			git -C ../child1 fetch origin tag Tag &&
> +			>file1 &&
> +			git add file1 &&
> +			git commit -m 'file1' &&
> +			git tag $tag_args Tag &&
> +			git -C ../child1 fetch origin tag Tag
> +		)
> +	"
> +}
> +
> +test_force_fetch_tag "lightweight tag" "-f"
> +test_force_fetch_tag "annotated tag" "-f -a -m'msg'"

I do not think that the single quotes around msg on the second one
does what you want them to.  In "git tag $tag_args Tag" there is no
eval.  You have the same for the push side, which can be seen in the
precontext of this hunk.

I somehow thought that you switched to using testTag for some
reason in an earlier step.  Shouldn't we be calling this Tag
also testTag?


> +
>  test_expect_success 'push --porcelain' '
>  	mk_empty testrepo &&
>  	echo >.git/foo  "To testrepo" &&

Re: [PATCH v2 10/10] fetch: stop clobbering existing tags without --force

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 97d3217df9..5b624caf58 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -49,11 +49,16 @@ endif::git-pull[]
>  
>  -f::
>  --force::
> -	When 'git fetch' is used with `<rbranch>:<lbranch>`
> -	refspec, it refuses to update the local branch
> -	`<lbranch>` unless the remote branch `<rbranch>` it
> -	fetches is a descendant of `<lbranch>`.  This option
> -	overrides that check.
> +	When 'git fetch' is used with `<src>:<dst>` refspec it may
> +	refuse to update the local branch as discussed
> +ifdef::git-pull[]
> +	in the `<refspec>` part of the linkgit:git-fetch[1]
> +	documentation.
> +endif::git-pull[]
> +ifndef::git-pull[]
> +	in the `<refspec>` part below.
> +endif::git-pull[]
> +	This option overrides that check.

Ah, that's tricky.  I could not locate "the `<refspec>` part" in
Documentation/git-fetch.txt and was scratching my head, but it
comes from pull-fetch-param.txt by inclusion ;-)

> diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
> index f1fb08dc68..acb8e1a4f0 100644
> --- a/Documentation/pull-fetch-param.txt
> +++ b/Documentation/pull-fetch-param.txt
> @@ -33,11 +33,21 @@ name.
>  it requests fetching everything up to the given tag.
>  +
>  The remote ref that matches <src>
> -is fetched, and if <dst> is not an empty string, the local
> -ref that matches it is fast-forwarded using <src>.
> -If the optional plus `+` is used, the local ref
> -is updated even if it does not result in a fast-forward
> -update.
> +is fetched, and if <dst> is not an empty string, an attempt
> +is made to update the local ref that matches it.
> ++
> +Whether that update is allowed without `--force` depends on the ref
> +namespace it's being fetched to, and the type of object being
> +fetched. If it's a commit under `refs/heads/*` only fast-forwards are
> +allowed.
> ++
> +By having the optional leading `+` to a refspec (or using `--force`
> +command line option) you can tell Git to update the local ref even if
> +it is not allowed by its respective namespace clobbering rules.

The above two paragraphs imply that I can "fetch +blob:refs/heads/master"
to cause havoc locally?

> +Before Git version 2.19 tag objects under `refs/tags/*` would not be
> +protected from updates, but since then the `+` (or `--force`) syntax
> +is required to clobber them.

I think that is a good change; it belongs more to the b/c notes in
the release notes; while I do not think it is wrong describe "it
used to be that way" just after a drastic change in the immediate
past, we shouldn't carry that forever, so perhaps we can leave a
"NEEDSWORK: remove the 'it used to be this way' in 2020" comment
around here?

[PATCH v3 0/7] Prep for "git fetch" should not clobber existing tags without --force

[Ævar Arnfjörð Bjarmason]

I have not had time to re-submit a v2 of my patch series to make "+"
meaningful in refspecs when it comes to tags, see v2 here:
https://public-inbox.org/git/20180731130718.25222-1-avarab@gmail.com/

Given where we're at with the 2.19 release I'd like to propose this
shortened version for inclusion in 2.19 for now. It's 7/10 patches in
that series, that purely deal with fixing some test issues and a
trivial grammar error in the tests.

This is unchanged from what's been cooking in pu for months now, so
hopefully it can be merged down faster than most, and then I can later
submit the actual meat of this series once I fix the (mostly doc)
issues with it.

Ævar Arnfjörð Bjarmason (7):
  fetch tests: change "Tag" test tag to "testTag"
  push tests: remove redundant 'git push' invocation
  push tests: fix logic error in "push" test assertion
  push tests: add more testing for forced tag pushing
  push tests: assert re-pushing annotated tags
  fetch tests: correct a comment "remove it" -> "remove them"
  pull doc: fix a long-standing grammar error

 Documentation/pull-fetch-param.txt |  2 +-
 t/t5510-fetch.sh                   |  2 +-
 t/t5516-fetch-push.sh              | 65 +++++++++++++++++++++---------
 3 files changed, 47 insertions(+), 22 deletions(-)

-- 
2.18.0.345.g5c9ce644c3

[PATCH v3 1/7] fetch tests: change "Tag" test tag to "testTag"

[Ævar Arnfjörð Bjarmason]

Calling the test tag "Tag" will make for confusing reading later in
this series when making use of the "git push tag <name>"
feature. Let's call the tag testTag instead.

Changes code initially added in dbfeddb12e ("push: require force for
refs under refs/tags/", 2012-11-29).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index bd8f23e430..2cbe459ee6 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -971,18 +971,18 @@ test_expect_success 'push requires --force to update lightweight tag' '
 	mk_child testrepo child2 &&
 	(
 		cd child1 &&
-		git tag Tag &&
-		git push ../child2 Tag &&
-		git push ../child2 Tag &&
+		git tag testTag &&
+		git push ../child2 testTag &&
+		git push ../child2 testTag &&
 		>file1 &&
 		git add file1 &&
 		git commit -m "file1" &&
-		git tag -f Tag &&
-		test_must_fail git push ../child2 Tag &&
-		git push --force ../child2 Tag &&
-		git tag -f Tag &&
-		test_must_fail git push ../child2 Tag HEAD~ &&
-		git push --force ../child2 Tag
+		git tag -f testTag &&
+		test_must_fail git push ../child2 testTag &&
+		git push --force ../child2 testTag &&
+		git tag -f testTag &&
+		test_must_fail git push ../child2 testTag HEAD~ &&
+		git push --force ../child2 testTag
 	)
 '
 
-- 
2.18.0.345.g5c9ce644c3

[PATCH v3 2/7] push tests: remove redundant 'git push' invocation

[Ævar Arnfjörð Bjarmason]

Remove an invocation of 'git push' that's exactly the same as the one
on the preceding line. This was seemingly added by mistake in
dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29) and doesn't affect the result of the test, the second
"push" was a no-op as there was nothing new to push.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2cbe459ee6..59d7ea689a 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -973,7 +973,6 @@ test_expect_success 'push requires --force to update lightweight tag' '
 		cd child1 &&
 		git tag testTag &&
 		git push ../child2 testTag &&
-		git push ../child2 testTag &&
 		>file1 &&
 		git add file1 &&
 		git commit -m "file1" &&
-- 
2.18.0.345.g5c9ce644c3

[PATCH v3 3/7] push tests: fix logic error in "push" test assertion

[Ævar Arnfjörð Bjarmason]

Fix a logic error that's been here since this test was added in
dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29).

The intent of this test is to force-create a new tag pointing to
HEAD~, and then assert that pushing it doesn't work without --force.

Instead, the code was not creating a new tag at all, and then failing
to push the previous tag for the unrelated reason of providing a
refspec that doesn't make any sense.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 59d7ea689a..fbc44273d9 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -979,8 +979,8 @@ test_expect_success 'push requires --force to update lightweight tag' '
 		git tag -f testTag &&
 		test_must_fail git push ../child2 testTag &&
 		git push --force ../child2 testTag &&
-		git tag -f testTag &&
-		test_must_fail git push ../child2 testTag HEAD~ &&
+		git tag -f testTag HEAD~ &&
+		test_must_fail git push ../child2 testTag &&
 		git push --force ../child2 testTag
 	)
 '
-- 
2.18.0.345.g5c9ce644c3

[PATCH v3 4/7] push tests: add more testing for forced tag pushing

[Ævar Arnfjörð Bjarmason]

Improve the tests added in dbfeddb12e ("push: require force for refs
under refs/tags/", 2012-11-29) to assert that the same behavior
applies various other combinations of command-line option and
refspecs.

Supplying either "+" in refspec or "--force" is sufficient to clobber
the reference. With --no-force we still pay attention to "+" in the
refspec, and vice-versa with clobbering kicking in if there's no "+"
in the refspec but "+" is given.

This is consistent with how refspecs work for branches, where either
"+" or "--force" will enable clobbering, with neither taking priority
over the other.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index fbc44273d9..c7b0d2ba00 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -965,7 +965,7 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 	)
 '
 
-test_expect_success 'push requires --force to update lightweight tag' '
+test_expect_success 'force pushing required to update lightweight tag' '
 	mk_test testrepo heads/master &&
 	mk_child testrepo child1 &&
 	mk_child testrepo child2 &&
@@ -981,7 +981,25 @@ test_expect_success 'push requires --force to update lightweight tag' '
 		git push --force ../child2 testTag &&
 		git tag -f testTag HEAD~ &&
 		test_must_fail git push ../child2 testTag &&
-		git push --force ../child2 testTag
+		git push --force ../child2 testTag &&
+
+		# Clobbering without + in refspec needs --force
+		git tag -f testTag &&
+		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
+		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
+
+		# Clobbering with + in refspec does not need --force
+		git tag -f testTag HEAD~ &&
+		git push ../child2 "+refs/tags/*:refs/tags/*" &&
+
+		# Clobbering with --no-force still obeys + in refspec
+		git tag -f testTag &&
+		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
+
+		# Clobbering with/without --force and "tag <name>" format
+		git tag -f testTag HEAD~ &&
+		test_must_fail git push ../child2 tag testTag &&
+		git push --force ../child2 tag testTag
 	)
 '
 
-- 
2.18.0.345.g5c9ce644c3

[PATCH v3 5/7] push tests: assert re-pushing annotated tags

[Ævar Arnfjörð Bjarmason]

Change the test that asserts that lightweight tags can only be
clobbered by a force-push to check do the same tests for annotated
tags.

There used to be less exhaustive tests for this with the code added in
40eff17999 ("push: require force for annotated tags", 2012-11-29), but
Junio removed them in 256b9d70a4 ("push: fix "refs/tags/ hierarchy
cannot be updated without --force"", 2013-01-16) while fixing some of
the behavior around tag pushing.

That change left us without any coverage asserting that pushing and
clobbering annotated tags worked as intended.  There was no reason to
suspect that the receive machinery wouldn't behave the same way with
annotated tags, but now we know for sure.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 82 ++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 37 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c7b0d2ba00..539c25aada 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -965,43 +965,51 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 	)
 '
 
-test_expect_success 'force pushing required to update lightweight tag' '
-	mk_test testrepo heads/master &&
-	mk_child testrepo child1 &&
-	mk_child testrepo child2 &&
-	(
-		cd child1 &&
-		git tag testTag &&
-		git push ../child2 testTag &&
-		>file1 &&
-		git add file1 &&
-		git commit -m "file1" &&
-		git tag -f testTag &&
-		test_must_fail git push ../child2 testTag &&
-		git push --force ../child2 testTag &&
-		git tag -f testTag HEAD~ &&
-		test_must_fail git push ../child2 testTag &&
-		git push --force ../child2 testTag &&
-
-		# Clobbering without + in refspec needs --force
-		git tag -f testTag &&
-		test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
-		git push --force ../child2 "refs/tags/*:refs/tags/*" &&
-
-		# Clobbering with + in refspec does not need --force
-		git tag -f testTag HEAD~ &&
-		git push ../child2 "+refs/tags/*:refs/tags/*" &&
-
-		# Clobbering with --no-force still obeys + in refspec
-		git tag -f testTag &&
-		git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
-
-		# Clobbering with/without --force and "tag <name>" format
-		git tag -f testTag HEAD~ &&
-		test_must_fail git push ../child2 tag testTag &&
-		git push --force ../child2 tag testTag
-	)
-'
+test_force_push_tag () {
+	tag_type_description=$1
+	tag_args=$2
+
+	test_expect_success 'force pushing required to update lightweight tag' "
+		mk_test testrepo heads/master &&
+		mk_child testrepo child1 &&
+		mk_child testrepo child2 &&
+		(
+			cd child1 &&
+			git tag testTag &&
+			git push ../child2 testTag &&
+			>file1 &&
+			git add file1 &&
+			git commit -m 'file1' &&
+			git tag $tag_args testTag &&
+			test_must_fail git push ../child2 testTag &&
+			git push --force ../child2 testTag &&
+			git tag $tag_args testTag HEAD~ &&
+			test_must_fail git push ../child2 testTag &&
+			git push --force ../child2 testTag &&
+
+			# Clobbering without + in refspec needs --force
+			git tag -f testTag &&
+			test_must_fail git push ../child2 'refs/tags/*:refs/tags/*' &&
+			git push --force ../child2 'refs/tags/*:refs/tags/*' &&
+
+			# Clobbering with + in refspec does not need --force
+			git tag -f testTag HEAD~ &&
+			git push ../child2 '+refs/tags/*:refs/tags/*' &&
+
+			# Clobbering with --no-force still obeys + in refspec
+			git tag -f testTag &&
+			git push --no-force ../child2 '+refs/tags/*:refs/tags/*' &&
+
+			# Clobbering with/without --force and 'tag <name>' format
+			git tag -f testTag HEAD~ &&
+			test_must_fail git push ../child2 tag testTag &&
+			git push --force ../child2 tag testTag
+		)
+	"
+}
+
+test_force_push_tag "lightweight tag" "-f"
+test_force_push_tag "annotated tag" "-f -a -m'msg'"
 
 test_expect_success 'push --porcelain' '
 	mk_empty testrepo &&
-- 
2.18.0.345.g5c9ce644c3

[PATCH v3 6/7] fetch tests: correct a comment "remove it" -> "remove them"

[Ævar Arnfjörð Bjarmason]

Correct a comment referring to the removal of just the branch to also
refer to the tag. This should have been changed in my
ca3065e7e7 ("fetch tests: add a tag to be deleted to the pruning
tests", 2018-02-09) when the tag deletion was added, but I missed it
at the time.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5510-fetch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 62308be499..fdb73b3971 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -613,7 +613,7 @@ test_configured_prune_type () {
 			git rev-parse --verify refs/tags/newtag
 		) &&
 
-		# now remove it
+		# now remove them
 		git branch -d newbranch &&
 		git tag -d newtag &&
 
-- 
2.18.0.345.g5c9ce644c3

[PATCH v3 7/7] pull doc: fix a long-standing grammar error

[Ævar Arnfjörð Bjarmason]

It should be "is not an empty string" not "is not empty string". This
fixes wording originally introduced in ab9b31386b ("Documentation:
multi-head fetch.", 2005-08-24).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pull-fetch-param.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
index c579793af5..f1fb08dc68 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -33,7 +33,7 @@ name.
 it requests fetching everything up to the given tag.
 +
 The remote ref that matches <src>
-is fetched, and if <dst> is not empty string, the local
+is fetched, and if <dst> is not an empty string, the local
 ref that matches it is fast-forwarded using <src>.
 If the optional plus `+` is used, the local ref
 is updated even if it does not result in a fast-forward
-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH v3 0/7] Prep for "git fetch" should not clobber existing tags without --force

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This is unchanged from what's been cooking in pu for months now, so
> hopefully it can be merged down faster than most, and then I can later
> submit the actual meat of this series once I fix the (mostly doc)
> issues with it.

They have been held in 'pu' only because you said they were not
ready, I think ;-)

I can confirm that the first 5 do look the same, and you dropped the
old 6, 8 and 10.  The remainder look the same.

I quickly re-scanned them and all of them looked obviously good.
Will discard the remainder and requeue.


>
> Ævar Arnfjörð Bjarmason (7):
>   fetch tests: change "Tag" test tag to "testTag"
>   push tests: remove redundant 'git push' invocation
>   push tests: fix logic error in "push" test assertion
>   push tests: add more testing for forced tag pushing
>   push tests: assert re-pushing annotated tags
>   fetch tests: correct a comment "remove it" -> "remove them"
>   pull doc: fix a long-standing grammar error
>
>  Documentation/pull-fetch-param.txt |  2 +-
>  t/t5510-fetch.sh                   |  2 +-
>  t/t5516-fetch-push.sh              | 65 +++++++++++++++++++++---------
>  3 files changed, 47 insertions(+), 22 deletions(-)

Re: [PATCH v3 0/7] Prep for "git fetch" should not clobber existing tags without --force

[Ævar Arnfjörð Bjarmason]

On Mon, Aug 13 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> This is unchanged from what's been cooking in pu for months now, so
>> hopefully it can be merged down faster than most, and then I can later
>> submit the actual meat of this series once I fix the (mostly doc)
>> issues with it.
>
> They have been held in 'pu' only because you said they were not
> ready, I think ;-)

You had some feedback to 6, 8 & 10 in the last round which I haven't
addressed yet.

I think the "not ready" comment you're remembering is this for v1:
https://public-inbox.org/git/CACBZZX4yG5h5kk4NFQz_NzAweMa+Nh3H-39OHtcH4XWsA6FGpg@mail.gmail.com/

> I can confirm that the first 5 do look the same, and you dropped the
> old 6, 8 and 10.  The remainder look the same.

Yup!

> I quickly re-scanned them and all of them looked obviously good.
> Will discard the remainder and requeue.

Thanks!

Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 31 2018, Junio C Hamano wrote:

I'm finally getting to re-rolling this. Just some inline comments.

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>>  The <src> is often the name of the branch you would want to push, but
>> -it can be any arbitrary "SHA-1 expression", such as `master~4` or
>> -`HEAD` (see linkgit:gitrevisions[7]).
>> +it can be any arbitrary expression to a commit, such as `master~4` or
>> +`HEAD` (see linkgit:gitrevisions[7]). It can also refer to tag
>> +objects, trees or blobs if the <dst> is outside of `refs/heads/*`.
>
> "It can also refer to..." is a good addition, but do you really want
> to make it part of this series to change/deprecate "SHA-1 expression"
> (which would certainly involve discussion on "then what to call them
> instead, now we are trying to refrain from saying SHA-1?")?

I won't change that.

>> +on the remote side. Whether this is allowed depends on where in
>> +`refs/*` the <dst> reference lives. The `refs/heads/*` namespace will
>> +only accept commit objects, and then only they can be
>> +fast-forwarded. The `refs/tags/*` namespace will accept any kind of
>> +object, and any changes to them and others types of objects will be
>> +rejected. Finally, it's possible to push any type of object to any
>> +namespace outside of `refs/{tags,heads}/*`,
>
> All sound correct.
>
>> but these will be treated
>> +as branches for the purposes of whether `--force` is required, even in
>> +the case where a tag object is pushed.
>
> I am not sure what "will be treated as branches" exactly means.
> Does it mean "as if they were in refs/heads/* hierarchy?"  Or
> something else?

I'll clarify this. Have rewritten most of this.

>> That tag object will be
>> +overwritten by another tag object (or commit!) without `--force` if
>> +the new tag happens to point to a commit that's a fast-forward of the
>> +commit it replaces.
>
> Yup, and that is something we want to fix with a later part of this
> series.
>

For what it's worth this is not at all what I'm fixing. The new docs
describe this better, but what I'm talking about here is that you can
push a tag like git.git's v2.18.0 to refs/blah/my-tag, then you can push
v2.19.0-rc0^{} to refs/blah/my-tag and it'll be allowed as a
fast-forward, and then v2.19.0-rc1 etc.

I.e. the non-refs/{tags,heads}/* update logic treats all updates to
tags/commits as branch updates. We just look at the tag v2.18.0, see you
want to replace it with the commit v2.19.0-rc0^{} and see "oh, that's a
fast-forward".

Arguably that should be changed, but I won't do that in this series.

>> +By having the optional leading `+` to a refspec (or using `--force`
>> +command line option) you can tell Git to update the <dst> ref even if
>> +it is not allowed by its respective namespace clobbering rules (e.g.,
>> +it is not a fast-forward. in the case of `refs/heads/*` updates).
>
> This gives an impression that with "--force" you can put non-commit
> inside refs/heads/* hierarchy.  Is that correct (if so we probably
> would want to fix that behaviour)?

I'll fix the wording, but nope, luckily you can't do that.

>> +This
>> +does *not* attempt to merge <src> into <dst>.  See EXAMPLES below for
>> +details.
>
> That is not wrong per-se, but would normal people expect a merge to
> happen upon pushing on the other side, I wonder?
>
> Thanks for cleaning up our longstanding mess.

Will fix/reword.

Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I.e. the non-refs/{tags,heads}/* update logic treats all updates to
> tags/commits as branch updates. We just look at the tag v2.18.0, see you
> want to replace it with the commit v2.19.0-rc0^{} and see "oh, that's a
> fast-forward".

In my old message you are responding to, I asked what you meant by
"will be treated as branches", and after seeing "as branch updates"
above, I think I know what you want the phrase to mean, namely, that
old-to-new transition requires new to be a descendant of old.  But I
think that is weaker than what other people (including me) thinks of
rules to update refs/heads/* hierarchy (i.e. "branch update").

You are allowing to store an object that is not a commit in
refs/blah/my-tag in your example, so it clearly does not protect the
ref with an extra rule that applies to "branches", namely, "it has
to be a commit."

> Arguably that should be changed, but I won't do that in this series.

OK.

Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 30, 2018 at 5:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > I.e. the non-refs/{tags,heads}/* update logic treats all updates to
> > tags/commits as branch updates. We just look at the tag v2.18.0, see you
> > want to replace it with the commit v2.19.0-rc0^{} and see "oh, that's a
> > fast-forward".
>
> In my old message you are responding to, I asked what you meant by
> "will be treated as branches", and after seeing "as branch updates"
> above, I think I know what you want the phrase to mean, namely, that
> old-to-new transition requires new to be a descendant of old.  But I
> think that is weaker than what other people (including me) thinks of
> rules to update refs/heads/* hierarchy (i.e. "branch update").
>
> You are allowing to store an object that is not a commit in
> refs/blah/my-tag in your example, so it clearly does not protect the
> ref with an extra rule that applies to "branches", namely, "it has
> to be a commit."

Indeed. This was all confusing. I've reworded in something I'll send
shortly, which should address this confusion.

> > Arguably that should be changed, but I won't do that in this series.
>
> OK.

[PATCH v4 0/6] "git fetch" should not clobber existing tags without --force

[Ævar Arnfjörð Bjarmason]

Now that the tests for this have landed in master (in v3), and because
I needed to rebase these for rolling out my own version based on
v2.19.0-rc1, here's a re-roll which should address the (mostly doc)
comments on the previous (v2) round.

Ævar Arnfjörð Bjarmason (6):
  fetch: change "branch" to "reference" in --force -h output
  push tests: correct quoting in interpolated string
  fetch tests: add a test for clobbering tag behavior
  push doc: correct lies about how push refspecs work
  fetch: document local ref updates with/without --force
  fetch: stop clobbering existing tags without --force

 Documentation/fetch-options.txt    | 15 +++++++----
 Documentation/git-push.txt         | 41 +++++++++++++++++++++++++-----
 Documentation/gitrevisions.txt     |  7 ++---
 Documentation/pull-fetch-param.txt | 35 +++++++++++++++++++++----
 builtin/fetch.c                    | 20 ++++++++++-----
 t/t5516-fetch-push.sh              | 27 +++++++++++++++++++-
 t/t5612-clone-refspec.sh           |  4 +--
 7 files changed, 120 insertions(+), 29 deletions(-)

-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v4 1/6] fetch: change "branch" to "reference" in --force -h output

[Ævar Arnfjörð Bjarmason]

The -h output has been referring to the --force command as forcing the
overwriting of local branches, but since "fetch" more generally
fetches all sorts of references in all refs/ namespaces, let's talk
about forcing the update of a a "reference" instead.

This wording was initially introduced in 8320199873 ("Rewrite
builtin-fetch option parsing to use parse_options().", 2007-12-04).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fetch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61bec5d213..b0706b3803 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -114,7 +114,7 @@ static struct option builtin_fetch_options[] = {
 		 N_("append to .git/FETCH_HEAD instead of overwriting")),
 	OPT_STRING(0, "upload-pack", &upload_pack, N_("path"),
 		   N_("path to upload pack on remote end")),
-	OPT__FORCE(&force, N_("force overwrite of local branch"), 0),
+	OPT__FORCE(&force, N_("force overwrite of local reference"), 0),
 	OPT_BOOL('m', "multiple", &multiple,
 		 N_("fetch from multiple remotes")),
 	OPT_SET_INT('t', "tags", &tags,
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v4 2/6] push tests: correct quoting in interpolated string

[Ævar Arnfjörð Bjarmason]

The quoted -m'msg' option is passed as a string to another function,
where due to interpolation it'll end up meaning the same as if we did
just did -m'msg' here.

In [1] this was pointed out to me, but in submitting [2] the patches I
missed this (since it was feedback on another patch I was holding
off), so this logic error landed in 380efb65df ("push tests: assert
re-pushing annotated tags", 2018-07-31).

Let's just remove the quotes, and use a string that doesn't need to be
quoted (-mtag.message is a bit less confusing than -mmsg). I could try
to chase after getting the quoting right here with multiple
backslashes, but I don't think it's worth it, and it makes things much
less readable.

1. https://public-inbox.org/git/xmqq4lgfcn5a.fsf@gitster-ct.c.googlers.com/
2. https://public-inbox.org/git/20180813192249.27585-1-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 539c25aada..69f7c9bfe6 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1009,7 +1009,7 @@ test_force_push_tag () {
 }
 
 test_force_push_tag "lightweight tag" "-f"
-test_force_push_tag "annotated tag" "-f -a -m'msg'"
+test_force_push_tag "annotated tag" "-f -a -mtag.message"
 
 test_expect_success 'push --porcelain' '
 	mk_empty testrepo &&
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v4 3/6] fetch tests: add a test for clobbering tag behavior

[Ævar Arnfjörð Bjarmason]

The test suite only incidentally (and unintentionally) tested for the
current behavior of eager tag clobbering on "fetch". This is a
followup to 380efb65df ("push tests: assert re-pushing annotated
tags", 2018-07-31) which tests for it explicitly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 69f7c9bfe6..3cde72ae47 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1011,6 +1011,30 @@ test_force_push_tag () {
 test_force_push_tag "lightweight tag" "-f"
 test_force_push_tag "annotated tag" "-f -a -mtag.message"
 
+test_force_fetch_tag () {
+	tag_type_description=$1
+	tag_args=$2
+
+	test_expect_success "fetch will clobber an existing $tag_type_description" "
+		mk_test testrepo heads/master &&
+		mk_child testrepo child1 &&
+		mk_child testrepo child2 &&
+		(
+			cd testrepo &&
+			git tag testTag &&
+			git -C ../child1 fetch origin tag testTag &&
+			>file1 &&
+			git add file1 &&
+			git commit -m 'file1' &&
+			git tag $tag_args testTag &&
+			git -C ../child1 fetch origin tag testTag
+		)
+	"
+}
+
+test_force_fetch_tag "lightweight tag" "-f"
+test_force_fetch_tag "annotated tag" "-f -a -mtag.message"
+
 test_expect_success 'push --porcelain' '
 	mk_empty testrepo &&
 	echo >.git/foo  "To testrepo" &&
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v4 4/6] push doc: correct lies about how push refspecs work

[Ævar Arnfjörð Bjarmason]

There's complex rules governing whether a push is allowed to take
place depending on whether we're pushing to refs/heads/*, refs/tags/*
or refs/not-that/*. See is_branch() in refs.c, and the various
assertions in refs/files-backend.c. (e.g. "trying to write non-commit
object %s to branch '%s'").

This documentation has never been quite correct, but went downhill
after dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29) when we started claiming that <dst> couldn't be a tag
object, which is incorrect. After some of the logic in that patch was
changed in 256b9d70a4 ("push: fix "refs/tags/ hierarchy cannot be
updated without --force"", 2013-01-16) the docs weren't updated, and
we've had some version of documentation that confused whether <src>
was a tag or not with whether <dst> would accept either an annotated
tag object or the commit it points to.

This makes the intro somewhat more verbose & complex, perhaps we
should have a shorter description here and split the full complexity
into a dedicated section. Very few users will find themselves needing
to e.g. push blobs or trees to refs/custom-namespace/* (or blobs or
trees at all), and that could be covered separately as an advanced
topic.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-push.txt     | 41 +++++++++++++++++++++++++++++-----
 Documentation/gitrevisions.txt |  7 +++---
 2 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 55277a9781..0f03d36f1e 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -74,12 +74,41 @@ without any `<refspec>` on the command line.  Otherwise, missing
 `:<dst>` means to update the same ref as the `<src>`.
 +
 The object referenced by <src> is used to update the <dst> reference
-on the remote side.  By default this is only allowed if <dst> is not
-a tag (annotated or lightweight), and then only if it can fast-forward
-<dst>.  By having the optional leading `+`, you can tell Git to update
-the <dst> ref even if it is not allowed by default (e.g., it is not a
-fast-forward.)  This does *not* attempt to merge <src> into <dst>.  See
-EXAMPLES below for details.
+on the remote side. Whether this is allowed depends on where in
+`refs/*` the <dst> reference lives as described in detail below. Any
+such update does *not* attempt to merge <src> into <dst>. See EXAMPLES
+below for details.
++
+The `refs/heads/*` namespace will only accept commit objects, and only
+if they can be fast-forwarded.
++
+The `refs/tags/*` namespace will accept any kind of object (as
+commits, trees and blobs can be tagged), and any changes to them will
+be rejected.
++
+It's possible to push any type of object to any namespace outside of
+`refs/{tags,heads}/*`. In the case of tags and commits, these will be
+treated as if they were the commits inside `refs/heads/*` for the
+purposes of whether the update is allowed.
++
+I.e. a fast-forward of commits and tags outside `refs/{tags,heads}/*`
+is allowed, even in cases where what's being fast-forwarded is not a
+commit, but a tag object which happens to point to a new commit which
+is a fast-forward of the commit the last tag (or commit) it's
+replacing. Replacing a tag with an entirely different tag is also
+allowed, if it points to the same commit, as well as pushing a peeled
+tag, i.e. pushing the commit that existing tag object points to, or a
+new tag object which an existing commit points to.
++
+Tree and blob objects outside of `refs/{tags,heads}/*` will be treated
+the same way as if they were inside `refs/tags/*`, any modification of
+them will be rejected.
++
+All of the rules described above about what's not allowed as an update
+can be overridden by adding an the optional leading `+` to a refspec
+(or using `--force` command line option). The only exception to this
+is that no amount of forcing will make the `refs/heads/*` namespace
+accept a non-commit object.
 +
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
 +
diff --git a/Documentation/gitrevisions.txt b/Documentation/gitrevisions.txt
index 1f6cceaefb..d407b7dee1 100644
--- a/Documentation/gitrevisions.txt
+++ b/Documentation/gitrevisions.txt
@@ -19,9 +19,10 @@ walk the revision graph (such as linkgit:git-log[1]), all commits which are
 reachable from that commit. For commands that walk the revision graph one can
 also specify a range of revisions explicitly.
 
-In addition, some Git commands (such as linkgit:git-show[1]) also take
-revision parameters which denote other objects than commits, e.g. blobs
-("files") or trees ("directories of files").
+In addition, some Git commands (such as linkgit:git-show[1] and
+linkgit:git-push[1]) can also take revision parameters which denote
+other objects than commits, e.g. blobs ("files") or trees
+("directories of files").
 
 include::revisions.txt[]
 
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v4 5/6] fetch: document local ref updates with/without --force

[Ævar Arnfjörð Bjarmason]

Refer to the new git-push(1) documentation about when ref updates are
and aren't allowed with and without --force, noting how "git-fetch"
differs from the behavior of "git-push".

Perhaps it would be better to split this all out into a new
gitrefspecs(7) man page, or present this information using tables.

In lieu of that, this is accurate, and fixes a big omission in the
existing refspec docs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/fetch-options.txt    | 15 +++++++++-----
 Documentation/pull-fetch-param.txt | 32 +++++++++++++++++++++++++-----
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 8bc36af4b1..fa0a3151b3 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -68,11 +68,16 @@ endif::git-pull[]
 
 -f::
 --force::
-	When 'git fetch' is used with `<rbranch>:<lbranch>`
-	refspec, it refuses to update the local branch
-	`<lbranch>` unless the remote branch `<rbranch>` it
-	fetches is a descendant of `<lbranch>`.  This option
-	overrides that check.
+	When 'git fetch' is used with `<src>:<dst>` refspec it may
+	refuse to update the local branch as discussed
+ifdef::git-pull[]
+	in the `<refspec>` part of the linkgit:git-fetch[1]
+	documentation.
+endif::git-pull[]
+ifndef::git-pull[]
+	in the `<refspec>` part below.
+endif::git-pull[]
+	This option overrides that check.
 
 -k::
 --keep::
diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
index f1fb08dc68..ab9617ad01 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -33,11 +33,33 @@ name.
 it requests fetching everything up to the given tag.
 +
 The remote ref that matches <src>
-is fetched, and if <dst> is not an empty string, the local
-ref that matches it is fast-forwarded using <src>.
-If the optional plus `+` is used, the local ref
-is updated even if it does not result in a fast-forward
-update.
+is fetched, and if <dst> is not an empty string, an attempt
+is made to update the local ref that matches it.
++
+Whether that update is allowed without `--force` depends on the ref
+namespace it's being fetched to, the type of object being fetched, and
+whether the update is considered to be a fast-forward. Generally, the
+same rules apply for fetching as when pushing, see the `<refspec>...`
+section of linkgit:git-push[1] for what those are. Exceptions to those
+rules particular to 'git fetch' are noted below.
++
+Unlike when pushing with linkgit:git-push[1], any updates to
+`refs/tags/*` will be accepted without `+` in the refspec (or
+`--force`). The receiving promiscuously considers all tag updates from
+a remote to be forced fetches.
++
+Unlike when pushing with linkgit:git-push[1], any updates outside of
+`refs/{tags,heads}/*` will be accepted without `+` in the refspec (or
+`--force`), whether that's swapping e.g. a tree object for a blob, or
+a commit for another commit that's doesn't have the previous commit as
+an ancestor etc.
++
+As with pushing with linkgit:git-push[1], all of the rules described
+above about what's not allowed as an update can be overridden by
+adding an the optional leading `+` to a refspec (or using `--force`
+command line option). The only exception to this is that no amount of
+forcing will make the `refs/heads/*` namespace accept a non-commit
+object.
 +
 [NOTE]
 When the remote branch you want to fetch is known to
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v4 6/6] fetch: stop clobbering existing tags without --force

[Ævar Arnfjörð Bjarmason]

Change "fetch" to treat "+" in refspecs (aka --force) to mean we
should clobber a local tag of the same name.

This changes the long-standing behavior of "fetch" added in
853a3697dc ("[PATCH] Multi-head fetch.", 2005-08-20). Before this
change, all tag fetches effectively had --force enabled. See the
git-fetch-script code in fast_forward_local() with the comment:

    > Tags need not be pointing at commits so there is no way to
    > guarantee "fast-forward" anyway.

That commit and the rest of the history of "fetch" shows that the
"+" (--force) part of refpecs was only conceived for branch updates,
while tags have accepted any changes from upstream unconditionally and
clobbered the local tag object. Changing this behavior has been
discussed as early as 2011[1].

The current behavior doesn't make sense to me, it easily results in
local tags accidentally being clobbered. We could namespace our tags
per-remote and not locally populate refs/tags/*, but as with my
97716d217c ("fetch: add a --prune-tags option and fetch.pruneTags
config", 2018-02-09) it's easier to work around the current
implementation than to fix the root cause.

So this change implements suggestion #1 from Jeff's 2011 E-Mail[1],
"fetch" now only clobbers the tag if either "+" is provided as part of
the refspec, or if "--force" is provided on the command-line.

This also makes it nicely symmetrical with how "tag" itself works when
creating tags. I.e. we refuse to clobber any existing tags unless
"--force" is supplied. Now we can refuse all such clobbering, whether
it would happen by clobbering a local tag with "tag", or by fetching
it from the remote with "fetch".

Ref updates outside refs/{tags,heads/* are still still not symmetrical
with how "git push" works, as discussed in the recently changed
pull-fetch-param.txt documentation. This change brings the two
divergent behaviors more into line with one another. I don't think
there's any reason "fetch" couldn't fully converge with the behavior
used by "push", but that's a topic for another change.

One of the tests added in 31b808a032 ("clone --single: limit the fetch
refspec to fetched branch", 2012-09-20) is being changed to use
--force where a clone would clobber a tag. This changes nothing about
the existing behavior of the test.

1. https://public-inbox.org/git/20111123221658.GA22313@sigill.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pull-fetch-param.txt | 11 +++++++----
 builtin/fetch.c                    | 18 ++++++++++++------
 t/t5516-fetch-push.sh              |  5 +++--
 t/t5612-clone-refspec.sh           |  4 ++--
 4 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
index ab9617ad01..47c832b17c 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -43,10 +43,13 @@ same rules apply for fetching as when pushing, see the `<refspec>...`
 section of linkgit:git-push[1] for what those are. Exceptions to those
 rules particular to 'git fetch' are noted below.
 +
-Unlike when pushing with linkgit:git-push[1], any updates to
-`refs/tags/*` will be accepted without `+` in the refspec (or
-`--force`). The receiving promiscuously considers all tag updates from
-a remote to be forced fetches.
+Until Git version 2.20, and unlike when pushing with
+linkgit:git-push[1], any updates to `refs/tags/*` would be accepted
+without `+` in the refspec (or `--force`). The receiving promiscuously
+considered all tag updates from a remote to be forced fetches. Since
+Git version 2.20 updates to `refs/tags/*` work the same way as when
+pushing. I.e. any updates will be rejected without `+` in the refspec
+(or `--force`).
 +
 Unlike when pushing with linkgit:git-push[1], any updates outside of
 `refs/{tags,heads}/*` will be accepted without `+` in the refspec (or
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b0706b3803..ed4ed9d8c4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -667,12 +667,18 @@ static int update_local_ref(struct ref *ref,
 
 	if (!is_null_oid(&ref->old_oid) &&
 	    starts_with(ref->name, "refs/tags/")) {
-		int r;
-		r = s_update_ref("updating tag", ref, 0);
-		format_display(display, r ? '!' : 't', _("[tag update]"),
-			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref, summary_width);
-		return r;
+		if (force || ref->force) {
+			int r;
+			r = s_update_ref("updating tag", ref, 0);
+			format_display(display, r ? '!' : 't', _("[tag update]"),
+				       r ? _("unable to update local ref") : NULL,
+				       remote, pretty_ref, summary_width);
+			return r;
+		} else {
+			format_display(display, '!', _("[rejected]"), _("would clobber existing tag"),
+				       remote, pretty_ref, summary_width);
+			return 1;
+		}
 	}
 
 	current = lookup_commit_reference_gently(the_repository,
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 3cde72ae47..6c5aa967ee 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1015,7 +1015,7 @@ test_force_fetch_tag () {
 	tag_type_description=$1
 	tag_args=$2
 
-	test_expect_success "fetch will clobber an existing $tag_type_description" "
+	test_expect_success "fetch will not clobber an existing $tag_type_description without --force" "
 		mk_test testrepo heads/master &&
 		mk_child testrepo child1 &&
 		mk_child testrepo child2 &&
@@ -1027,7 +1027,8 @@ test_force_fetch_tag () {
 			git add file1 &&
 			git commit -m 'file1' &&
 			git tag $tag_args testTag &&
-			git -C ../child1 fetch origin tag testTag
+			test_must_fail git -C ../child1 fetch origin tag testTag &&
+			git -C ../child1 fetch origin '+refs/tags/*:refs/tags/*'
 		)
 	"
 }
diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
index 5582b3d5fd..e36ac01661 100755
--- a/t/t5612-clone-refspec.sh
+++ b/t/t5612-clone-refspec.sh
@@ -103,7 +103,7 @@ test_expect_success 'clone with --no-tags' '
 test_expect_success '--single-branch while HEAD pointing at master' '
 	(
 		cd dir_master &&
-		git fetch &&
+		git fetch --force &&
 		git for-each-ref refs/remotes/origin |
 		sed -e "/HEAD$/d" \
 		    -e "s|/remotes/origin/|/heads/|" >../actual
@@ -114,7 +114,7 @@ test_expect_success '--single-branch while HEAD pointing at master' '
 	test_cmp expect actual &&
 	(
 		cd dir_master &&
-		git fetch --tags &&
+		git fetch --tags --force &&
 		git for-each-ref refs/tags >../actual
 	) &&
 	git for-each-ref refs/tags >expect &&
-- 
2.19.0.rc1.350.ge57e33dbd1

Re: [PATCH v4 2/6] push tests: correct quoting in interpolated string

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The quoted -m'msg' option is passed as a string to another function,
> where due to interpolation it'll end up meaning the same as if we did
> just did -m'msg' here.

"as if we did just did"?  Also the sentence says -m'msg' is treated
as if we gave -m'msg' that is tautology.  Perhaps

	... as if we just did -mmsg here.

is what you meant?

But I think the pointing out I did in the old thread is wrong.  If
you change your "-mtag.message" to "-m'tag  message'" (notice that
I have two spaces between the words) and then insert an invocation
of "git show -s testTag" immediately after "git tag" is run to
create testTag with $tag_args in test_force_push_tag, I can
observe in "cd t && sh t5516-fetch-push.sh -v" output that
the single quote is taking effect just fine.  IOW, I do not see
anything wrong in the original "-m'msg'", which probably anticipated
that we may want to change it to -m'tag message' or something to
clarify.

However,...

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 539c25aada..69f7c9bfe6 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1009,7 +1009,7 @@ test_force_push_tag () {
>  }
>  
>  test_force_push_tag "lightweight tag" "-f"
> -test_force_push_tag "annotated tag" "-f -a -m'msg'"
> +test_force_push_tag "annotated tag" "-f -a -mtag.message"

Comparing test_force_push_tag and test_force_fetch_tag which is
added in the next step, I notice that the former ignores $1 so
passing "annotated tag" here has no effect.  That may be worth
fixing in a follow-up patch like this.

Re: [PATCH v4 3/6] fetch tests: add a test for clobbering tag behavior

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The test suite only incidentally (and unintentionally) tested for the
> current behavior of eager tag clobbering on "fetch". This is a
> followup to 380efb65df ("push tests: assert re-pushing annotated
> tags", 2018-07-31) which tests for it explicitly.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---

Good addition, and correctly uses $tag_type_description unlike the
one that is left unfixed in 2/6 for the push side.

>  t/t5516-fetch-push.sh | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 69f7c9bfe6..3cde72ae47 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1011,6 +1011,30 @@ test_force_push_tag () {
>  test_force_push_tag "lightweight tag" "-f"
>  test_force_push_tag "annotated tag" "-f -a -mtag.message"
>  
> +test_force_fetch_tag () {
> +	tag_type_description=$1
> +	tag_args=$2
> +
> +	test_expect_success "fetch will clobber an existing $tag_type_description" "
> +		mk_test testrepo heads/master &&
> +		mk_child testrepo child1 &&
> +		mk_child testrepo child2 &&
> +		(
> +			cd testrepo &&
> +			git tag testTag &&
> +			git -C ../child1 fetch origin tag testTag &&
> +			>file1 &&
> +			git add file1 &&
> +			git commit -m 'file1' &&
> +			git tag $tag_args testTag &&
> +			git -C ../child1 fetch origin tag testTag
> +		)
> +	"
> +}
> +
> +test_force_fetch_tag "lightweight tag" "-f"
> +test_force_fetch_tag "annotated tag" "-f -a -mtag.message"
> +
>  test_expect_success 'push --porcelain' '
>  	mk_empty testrepo &&
>  	echo >.git/foo  "To testrepo" &&

Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +on the remote side. Whether this is allowed depends on where in
> +`refs/*` the <dst> reference lives as described in detail below. Any
> +such update does *not* attempt to merge <src> into <dst>. See EXAMPLES
> +below for details.
> ++
> +The `refs/heads/*` namespace will only accept commit objects, and only
> +if they can be fast-forwarded.
> ++
> +The `refs/tags/*` namespace will accept any kind of object (as
> +commits, trees and blobs can be tagged), and any changes to them will
> +be rejected.
> ++
> +It's possible to push any type of object to any namespace outside of
> +`refs/{tags,heads}/*`. In the case of tags and commits, these will be
> +treated as if they were the commits inside `refs/heads/*` for the
> +purposes of whether the update is allowed.
> ++
> +I.e. a fast-forward of commits and tags outside `refs/{tags,heads}/*`
> +is allowed, even in cases where what's being fast-forwarded is not a
> +commit, but a tag object which happens to point to a new commit which
> +is a fast-forward of the commit the last tag (or commit) it's
> +replacing. Replacing a tag with an entirely different tag is also
> +allowed, if it points to the same commit, as well as pushing a peeled
> +tag, i.e. pushing the commit that existing tag object points to, or a
> +new tag object which an existing commit points to.
> ++
> +Tree and blob objects outside of `refs/{tags,heads}/*` will be treated
> +the same way as if they were inside `refs/tags/*`, any modification of
> +them will be rejected.
> ++
> +All of the rules described above about what's not allowed as an update
> +can be overridden by adding an the optional leading `+` to a refspec
> +(or using `--force` command line option). The only exception to this
> +is that no amount of forcing will make the `refs/heads/*` namespace
> +accept a non-commit object.

This, while some may find it overly long, is quite clear, compared
to the current text and to the previous rounds of this patch, and I
found it very much readable.

Re: [PATCH v4 6/6] fetch: stop clobbering existing tags without --force

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  +
> -Unlike when pushing with linkgit:git-push[1], any updates to
> -`refs/tags/*` will be accepted without `+` in the refspec (or
> -`--force`). The receiving promiscuously considers all tag updates from
> -a remote to be forced fetches.
> +Until Git version 2.20, and unlike when pushing with
> +linkgit:git-push[1], any updates to `refs/tags/*` would be accepted
> +without `+` in the refspec (or `--force`). The receiving promiscuously
> +considered all tag updates from a remote to be forced fetches. Since
> +Git version 2.20 updates to `refs/tags/*` work the same way as when
> +pushing. I.e. any updates will be rejected without `+` in the refspec
> +(or `--force`).

Have a comma after 2.20; otherwise it was unreadable, at least to
me, who took three attempts before realizing that the "updates" is
not a verb whose subject is "Git version 2.20".  Or

	Since Git version 2.20, fetching to update `refs/tags/*`
	work the same way as pushing into it

perhaps.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b0706b3803..ed4ed9d8c4 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -667,12 +667,18 @@ static int update_local_ref(struct ref *ref,
>  
>  	if (!is_null_oid(&ref->old_oid) &&
>  	    starts_with(ref->name, "refs/tags/")) {
> -		int r;
> -		r = s_update_ref("updating tag", ref, 0);
> -		format_display(display, r ? '!' : 't', _("[tag update]"),
> -			       r ? _("unable to update local ref") : NULL,
> -			       remote, pretty_ref, summary_width);
> -		return r;
> +		if (force || ref->force) {
> +			int r;
> +			r = s_update_ref("updating tag", ref, 0);
> +			format_display(display, r ? '!' : 't', _("[tag update]"),
> +				       r ? _("unable to update local ref") : NULL,
> +				       remote, pretty_ref, summary_width);
> +			return r;
> +		} else {
> +			format_display(display, '!', _("[rejected]"), _("would clobber existing tag"),
> +				       remote, pretty_ref, summary_width);
> +			return 1;
> +		}
>  	}

A straight-forward change to turn an unconditional update to either
an unconditonal rejection (when force is not given) or an
unconditional acceptance (when forced), which makes sense and has
near-zero chance of being wrong ;-)

It is a huge change in behaviour, but in a very good way.  I'd
imagine that users will welcome it very much.

Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 30 2018, Ævar Arnfjörð Bjarmason wrote:

[Notes to self]

> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 55277a9781..0f03d36f1e 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -74,12 +74,41 @@ without any `<refspec>` on the command line.  Otherwise, missing
>  `:<dst>` means to update the same ref as the `<src>`.
>  +
>  The object referenced by <src> is used to update the <dst> reference
> -on the remote side.  By default this is only allowed if <dst> is not
> -a tag (annotated or lightweight), and then only if it can fast-forward
> -<dst>.  By having the optional leading `+`, you can tell Git to update
> -the <dst> ref even if it is not allowed by default (e.g., it is not a
> -fast-forward.)  This does *not* attempt to merge <src> into <dst>.  See
> -EXAMPLES below for details.
> +on the remote side. Whether this is allowed depends on where in
> +`refs/*` the <dst> reference lives as described in detail below. Any
> +such update does *not* attempt to merge <src> into <dst>. See EXAMPLES
> +below for details.
> ++
> +The `refs/heads/*` namespace will only accept commit objects, and only
> +if they can be fast-forwarded.
> ++
> +The `refs/tags/*` namespace will accept any kind of object (as
> +commits, trees and blobs can be tagged), and any changes to them will
> +be rejected.
> ++

Both of these should carve out some mention for the "deletion" aspect of
"updates". I.e. you don't need --force to delete.

> +It's possible to push any type of object to any namespace outside of
> +`refs/{tags,heads}/*`. In the case of tags and commits, these will be
> +treated as if they were the commits inside `refs/heads/*` for the
> +purposes of whether the update is allowed.
> ++
> +I.e. a fast-forward of commits and tags outside `refs/{tags,heads}/*`
> +is allowed, even in cases where what's being fast-forwarded is not a
> +commit, but a tag object which happens to point to a new commit which
> +is a fast-forward of the commit the last tag (or commit) it's
> +replacing. Replacing a tag with an entirely different tag is also
> +allowed, if it points to the same commit, as well as pushing a peeled
> +tag, i.e. pushing the commit that existing tag object points to, or a
> +new tag object which an existing commit points to.
> ++
> +Tree and blob objects outside of `refs/{tags,heads}/*` will be treated
> +the same way as if they were inside `refs/tags/*`, any modification of
> +them will be rejected.
> ++
> +All of the rules described above about what's not allowed as an update
> +can be overridden by adding an the optional leading `+` to a refspec
> +(or using `--force` command line option). The only exception to this
> +is that no amount of forcing will make the `refs/heads/*` namespace
> +accept a non-commit object.
>  +
>  `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
>  +

Later below this we say:

    Pushing an empty <src> allows you to delete the <dst> ref from the
    remote repository.

Which, perhaps given the discussion of deletions as updates, should be
mentioned earlier in some way, i.e. should we just say above all these
rules that by "update" we mean non-deletions?

Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Aug 30 2018, Ævar Arnfjörð Bjarmason wrote:
>
> [Notes to self]
> ...
>
> Later below this we say:
>
>     Pushing an empty <src> allows you to delete the <dst> ref from the
>     remote repository.
>
> Which, perhaps given the discussion of deletions as updates, should be
> mentioned earlier in some way, i.e. should we just say above all these
> rules that by "update" we mean non-deletions?

You raised good points.  The rule that applies to deletion is quite
different from the one for update, we want to make sure readers know
updates and deletions are different.  As the rule for deletion is a
lot simpler (i.e. you can always delete unless a configuration or
pre-receive says otherwise), perhaps it would be sufficient to give
the rules for deletion upfront in one section, and then start the
section(s) for update with a phrase like "rules for accepting
updates are follows" after that.

Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work

[Ævar Arnfjörð Bjarmason]

On Fri, Aug 31, 2018 at 6:24 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > On Thu, Aug 30 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> > [Notes to self]
> > ...
> >
> > Later below this we say:
> >
> >     Pushing an empty <src> allows you to delete the <dst> ref from the
> >     remote repository.
> >
> > Which, perhaps given the discussion of deletions as updates, should be
> > mentioned earlier in some way, i.e. should we just say above all these
> > rules that by "update" we mean non-deletions?
>
> You raised good points.  The rule that applies to deletion is quite
> different from the one for update, we want to make sure readers know
> updates and deletions are different.  As the rule for deletion is a
> lot simpler (i.e. you can always delete unless a configuration or
> pre-receive says otherwise), perhaps it would be sufficient to give
> the rules for deletion upfront in one section, and then start the
> section(s) for update with a phrase like "rules for accepting
> updates are follows" after that.

Yeah, that was the plan. I'll do that.

[PATCH v5 0/9] git fetch" should not clobber existing tags without --force

[Ævar Arnfjörð Bjarmason]

Addresses Junio's comments to v4, and I had a few fixes of my own. I
don't know if this range-diff is more or less readble than just
re-reading it, but here goes:

 1:  d05fd561f3 =  1:  d05fd561f3 fetch: change "branch" to "reference" in --force -h output
 -:  ---------- >  2:  28275baca2 push tests: make use of unused $1 in test description
 2:  013ecd83b3 !  3:  834501afdc push tests: correct quoting in interpolated string
    @@ -1,24 +1,11 @@
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    -    push tests: correct quoting in interpolated string
    +    push tests: use spaces in interpolated string
     
    -    The quoted -m'msg' option is passed as a string to another function,
    -    where due to interpolation it'll end up meaning the same as if we did
    -    just did -m'msg' here.
    -
    -    In [1] this was pointed out to me, but in submitting [2] the patches I
    -    missed this (since it was feedback on another patch I was holding
    -    off), so this logic error landed in 380efb65df ("push tests: assert
    -    re-pushing annotated tags", 2018-07-31).
    -
    -    Let's just remove the quotes, and use a string that doesn't need to be
    -    quoted (-mtag.message is a bit less confusing than -mmsg). I could try
    -    to chase after getting the quoting right here with multiple
    -    backslashes, but I don't think it's worth it, and it makes things much
    -    less readable.
    -
    -    1. https://public-inbox.org/git/xmqq4lgfcn5a.fsf@gitster-ct.c.googlers.com/
    -    2. https://public-inbox.org/git/20180813192249.27585-1-avarab@gmail.com/
    +    The quoted -m'msg' option would mean the same as -mmsg when passed
    +    through the test_force_push_tag helper. Let's instead use a string
    +    with spaces in it, to have a working example in case we need to pass
    +    other whitespace-delimited arguments to git-tag.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ -30,7 +17,7 @@
      
      test_force_push_tag "lightweight tag" "-f"
     -test_force_push_tag "annotated tag" "-f -a -m'msg'"
    -+test_force_push_tag "annotated tag" "-f -a -mtag.message"
    ++test_force_push_tag "annotated tag" "-f -a -m'tag message'"
      
      test_expect_success 'push --porcelain' '
      	mk_empty testrepo &&
 3:  2d216a7ef6 !  4:  5f85542bb2 fetch tests: add a test for clobbering tag behavior
    @@ -14,7 +14,7 @@
      +++ b/t/t5516-fetch-push.sh
     @@
      test_force_push_tag "lightweight tag" "-f"
    - test_force_push_tag "annotated tag" "-f -a -mtag.message"
    + test_force_push_tag "annotated tag" "-f -a -m'tag message'"
      
     +test_force_fetch_tag () {
     +	tag_type_description=$1
    @@ -38,7 +38,7 @@
     +}
     +
     +test_force_fetch_tag "lightweight tag" "-f"
    -+test_force_fetch_tag "annotated tag" "-f -a -mtag.message"
    ++test_force_fetch_tag "annotated tag" "-f -a -m'tag message'"
     +
      test_expect_success 'push --porcelain' '
      	mk_empty testrepo &&
 -:  ---------- >  5:  6906d5a84d push doc: remove confusing mention of remote merger
 -:  ---------- >  6:  a16a9c2d7f push doc: move mention of "tag <tag>" later in the prose
 4:  b751e80b00 !  7:  9f8785e01a push doc: correct lies about how push refspecs work
    @@ -38,18 +38,20 @@
     -a tag (annotated or lightweight), and then only if it can fast-forward
     -<dst>.  By having the optional leading `+`, you can tell Git to update
     -the <dst> ref even if it is not allowed by default (e.g., it is not a
    --fast-forward.)  This does *not* attempt to merge <src> into <dst>.  See
    --EXAMPLES below for details.
    +-fast-forward.).
    +-+
    +-Pushing an empty <src> allows you to delete the <dst> ref from
    +-the remote repository.
     +on the remote side. Whether this is allowed depends on where in
    -+`refs/*` the <dst> reference lives as described in detail below. Any
    -+such update does *not* attempt to merge <src> into <dst>. See EXAMPLES
    -+below for details.
    ++`refs/*` the <dst> reference lives as described in detail below, in
    ++those sections "update" means any modifications except deletes, which
    ++as noted after the next few sections are treated differently.
     ++
    -+The `refs/heads/*` namespace will only accept commit objects, and only
    -+if they can be fast-forwarded.
    ++The `refs/heads/*` namespace will only accept commit objects, and
    ++updates only if they can be fast-forwarded.
     ++
     +The `refs/tags/*` namespace will accept any kind of object (as
    -+commits, trees and blobs can be tagged), and any changes to them will
    ++commits, trees and blobs can be tagged), and any updates to them will
     +be rejected.
     ++
     +It's possible to push any type of object to any namespace outside of
    @@ -67,17 +69,26 @@
     +new tag object which an existing commit points to.
     ++
     +Tree and blob objects outside of `refs/{tags,heads}/*` will be treated
    -+the same way as if they were inside `refs/tags/*`, any modification of
    -+them will be rejected.
    ++the same way as if they were inside `refs/tags/*`, any update of them
    ++will be rejected.
     ++
     +All of the rules described above about what's not allowed as an update
     +can be overridden by adding an the optional leading `+` to a refspec
     +(or using `--force` command line option). The only exception to this
     +is that no amount of forcing will make the `refs/heads/*` namespace
    -+accept a non-commit object.
    - +
    - `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
    ++accept a non-commit object. Hooks and configuration can also override
    ++or amend these rules, see e.g. `receive.denyNonFastForwards` in
    ++linkgit:git-config[1] and`pre-receive` and `update` in
    ++linkgit:githooks[5].
    +++
    ++Pushing an empty <src> allows you to delete the <dst> ref from the
    ++remote repository. Deletions are always accepted without a leading `+`
    ++in the refspec (or `--force`), except when forbidden by configuration
    ++or hooks. See `receive.denyDeletes` in linkgit:git-config[1] and
    ++`pre-receive` and `update` in linkgit:githooks[5].
      +
    + The special refspec `:` (or `+:` to allow non-fast-forward updates)
    + directs Git to push "matching" branches: for every branch that exists on
     
      diff --git a/Documentation/gitrevisions.txt b/Documentation/gitrevisions.txt
      --- a/Documentation/gitrevisions.txt
 5:  b120051957 =  8:  3e90699b9f fetch: document local ref updates with/without --force
 6:  25df331fce !  9:  0e183b6f23 fetch: stop clobbering existing tags without --force
    @@ -66,13 +66,24 @@
     +Until Git version 2.20, and unlike when pushing with
     +linkgit:git-push[1], any updates to `refs/tags/*` would be accepted
     +without `+` in the refspec (or `--force`). The receiving promiscuously
    -+considered all tag updates from a remote to be forced fetches. Since
    -+Git version 2.20 updates to `refs/tags/*` work the same way as when
    -+pushing. I.e. any updates will be rejected without `+` in the refspec
    -+(or `--force`).
    ++considered all tag updates from a remote to be forced fetches.  Since
    ++Git version 2.20, fetching to update `refs/tags/*` work the same way
    ++as when pushing. I.e. any updates will be rejected without `+` in the
    ++refspec (or `--force`).
      +
      Unlike when pushing with linkgit:git-push[1], any updates outside of
      `refs/{tags,heads}/*` will be accepted without `+` in the refspec (or
    +@@
    + a commit for another commit that's doesn't have the previous commit as
    + an ancestor etc.
    + +
    ++Unlike when pushing with linkgit:git-push[1], there is no
    ++configuration which'll amend these rules, and nothing like a
    ++`pre-fetch` hook analogous to the `pre-receive` hook.
    +++
    + As with pushing with linkgit:git-push[1], all of the rules described
    + above about what's not allowed as an update can be overridden by
    + adding an the optional leading `+` to a refspec (or using `--force`
     
      diff --git a/builtin/fetch.c b/builtin/fetch.c
      --- a/builtin/fetch.c

Ævar Arnfjörð Bjarmason (9):
  fetch: change "branch" to "reference" in --force -h output
  push tests: make use of unused $1 in test description
  push tests: use spaces in interpolated string
  fetch tests: add a test for clobbering tag behavior
  push doc: remove confusing mention of remote merger
  push doc: move mention of "tag <tag>" later in the prose
  push doc: correct lies about how push refspecs work
  fetch: document local ref updates with/without --force
  fetch: stop clobbering existing tags without --force

 Documentation/fetch-options.txt    | 15 +++++---
 Documentation/git-push.txt         | 57 ++++++++++++++++++++++++------
 Documentation/gitrevisions.txt     |  7 ++--
 Documentation/pull-fetch-param.txt | 39 +++++++++++++++++---
 builtin/fetch.c                    | 20 +++++++----
 t/t5516-fetch-push.sh              | 29 +++++++++++++--
 t/t5612-clone-refspec.sh           |  4 +--
 7 files changed, 136 insertions(+), 35 deletions(-)

-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v5 1/9] fetch: change "branch" to "reference" in --force -h output

[Ævar Arnfjörð Bjarmason]

The -h output has been referring to the --force command as forcing the
overwriting of local branches, but since "fetch" more generally
fetches all sorts of references in all refs/ namespaces, let's talk
about forcing the update of a a "reference" instead.

This wording was initially introduced in 8320199873 ("Rewrite
builtin-fetch option parsing to use parse_options().", 2007-12-04).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fetch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61bec5d213..b0706b3803 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -114,7 +114,7 @@ static struct option builtin_fetch_options[] = {
 		 N_("append to .git/FETCH_HEAD instead of overwriting")),
 	OPT_STRING(0, "upload-pack", &upload_pack, N_("path"),
 		   N_("path to upload pack on remote end")),
-	OPT__FORCE(&force, N_("force overwrite of local branch"), 0),
+	OPT__FORCE(&force, N_("force overwrite of local reference"), 0),
 	OPT_BOOL('m', "multiple", &multiple,
 		 N_("fetch from multiple remotes")),
 	OPT_SET_INT('t', "tags", &tags,
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v5 2/9] push tests: make use of unused $1 in test description

[Ævar Arnfjörð Bjarmason]

Fix up a logic error in 380efb65df ("push tests: assert re-pushing
annotated tags", 2018-07-31), where the $tag_type_description variable
was assigned to but never used, unlike in the subsequently added
companion test for fetches in 2d216a7ef6 ("fetch tests: add a test for
clobbering tag behavior", 2018-04-29).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 539c25aada..62d5059f92 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -969,7 +969,7 @@ test_force_push_tag () {
 	tag_type_description=$1
 	tag_args=$2
 
-	test_expect_success 'force pushing required to update lightweight tag' "
+	test_expect_success 'force pushing required to update $tag_type_description' "
 		mk_test testrepo heads/master &&
 		mk_child testrepo child1 &&
 		mk_child testrepo child2 &&
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v5 3/9] push tests: use spaces in interpolated string

[Ævar Arnfjörð Bjarmason]

The quoted -m'msg' option would mean the same as -mmsg when passed
through the test_force_push_tag helper. Let's instead use a string
with spaces in it, to have a working example in case we need to pass
other whitespace-delimited arguments to git-tag.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 62d5059f92..8b67f08265 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1009,7 +1009,7 @@ test_force_push_tag () {
 }
 
 test_force_push_tag "lightweight tag" "-f"
-test_force_push_tag "annotated tag" "-f -a -m'msg'"
+test_force_push_tag "annotated tag" "-f -a -m'tag message'"
 
 test_expect_success 'push --porcelain' '
 	mk_empty testrepo &&
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v5 4/9] fetch tests: add a test for clobbering tag behavior

[Ævar Arnfjörð Bjarmason]

The test suite only incidentally (and unintentionally) tested for the
current behavior of eager tag clobbering on "fetch". This is a
followup to 380efb65df ("push tests: assert re-pushing annotated
tags", 2018-07-31) which tests for it explicitly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8b67f08265..7f3d4c4965 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1011,6 +1011,30 @@ test_force_push_tag () {
 test_force_push_tag "lightweight tag" "-f"
 test_force_push_tag "annotated tag" "-f -a -m'tag message'"
 
+test_force_fetch_tag () {
+	tag_type_description=$1
+	tag_args=$2
+
+	test_expect_success "fetch will clobber an existing $tag_type_description" "
+		mk_test testrepo heads/master &&
+		mk_child testrepo child1 &&
+		mk_child testrepo child2 &&
+		(
+			cd testrepo &&
+			git tag testTag &&
+			git -C ../child1 fetch origin tag testTag &&
+			>file1 &&
+			git add file1 &&
+			git commit -m 'file1' &&
+			git tag $tag_args testTag &&
+			git -C ../child1 fetch origin tag testTag
+		)
+	"
+}
+
+test_force_fetch_tag "lightweight tag" "-f"
+test_force_fetch_tag "annotated tag" "-f -a -m'tag message'"
+
 test_expect_success 'push --porcelain' '
 	mk_empty testrepo &&
 	echo >.git/foo  "To testrepo" &&
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v5 5/9] push doc: remove confusing mention of remote merger

[Ævar Arnfjörð Bjarmason]

Saying that "git push <remote> <src>:<dst>" won't push a merger of
<src> and <dst> to <dst> is clear from the rest of the context here,
so mentioning it is redundant, furthermore the mention of "EXAMPLES
below" isn't specific or useful.

This phrase was originally added in 149f6ddfb3 ("Docs: Expand
explanation of the use of + in git push refspecs.", 2009-02-19), as
can be seen in that change the point of the example being cited was to
show that force pushing can leave unreferenced commits on the
remote. It's enough that we explain that in its own section, it
doesn't need to be mentioned here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-push.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 55277a9781..83e499ee97 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -78,8 +78,7 @@ on the remote side.  By default this is only allowed if <dst> is not
 a tag (annotated or lightweight), and then only if it can fast-forward
 <dst>.  By having the optional leading `+`, you can tell Git to update
 the <dst> ref even if it is not allowed by default (e.g., it is not a
-fast-forward.)  This does *not* attempt to merge <src> into <dst>.  See
-EXAMPLES below for details.
+fast-forward.).
 +
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
 +
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v5 6/9] push doc: move mention of "tag <tag>" later in the prose

[Ævar Arnfjörð Bjarmason]

This change will be followed-up with a subsequent change where I'll
change both sides of this mention of "tag <tag>" to be something
that's best read without interruption.

To make that change smaller, let's move this mention of "tag <tag>" to
the end of the "<refspec>..." section, it's now somewhere in the
middle.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-push.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 83e499ee97..71c78ac1a4 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -80,8 +80,6 @@ a tag (annotated or lightweight), and then only if it can fast-forward
 the <dst> ref even if it is not allowed by default (e.g., it is not a
 fast-forward.).
 +
-`tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
-+
 Pushing an empty <src> allows you to delete the <dst> ref from
 the remote repository.
 +
@@ -89,6 +87,8 @@ The special refspec `:` (or `+:` to allow non-fast-forward updates)
 directs Git to push "matching" branches: for every branch that exists on
 the local side, the remote side is updated if a branch of the same name
 already exists on the remote side.
++
+`tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
 
 --all::
 	Push all branches (i.e. refs under `refs/heads/`); cannot be
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v5 7/9] push doc: correct lies about how push refspecs work

[Ævar Arnfjörð Bjarmason]

There's complex rules governing whether a push is allowed to take
place depending on whether we're pushing to refs/heads/*, refs/tags/*
or refs/not-that/*. See is_branch() in refs.c, and the various
assertions in refs/files-backend.c. (e.g. "trying to write non-commit
object %s to branch '%s'").

This documentation has never been quite correct, but went downhill
after dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29) when we started claiming that <dst> couldn't be a tag
object, which is incorrect. After some of the logic in that patch was
changed in 256b9d70a4 ("push: fix "refs/tags/ hierarchy cannot be
updated without --force"", 2013-01-16) the docs weren't updated, and
we've had some version of documentation that confused whether <src>
was a tag or not with whether <dst> would accept either an annotated
tag object or the commit it points to.

This makes the intro somewhat more verbose & complex, perhaps we
should have a shorter description here and split the full complexity
into a dedicated section. Very few users will find themselves needing
to e.g. push blobs or trees to refs/custom-namespace/* (or blobs or
trees at all), and that could be covered separately as an advanced
topic.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-push.txt     | 52 ++++++++++++++++++++++++++++------
 Documentation/gitrevisions.txt |  7 +++--
 2 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 71c78ac1a4..f345bd30fc 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -74,14 +74,50 @@ without any `<refspec>` on the command line.  Otherwise, missing
 `:<dst>` means to update the same ref as the `<src>`.
 +
 The object referenced by <src> is used to update the <dst> reference
-on the remote side.  By default this is only allowed if <dst> is not
-a tag (annotated or lightweight), and then only if it can fast-forward
-<dst>.  By having the optional leading `+`, you can tell Git to update
-the <dst> ref even if it is not allowed by default (e.g., it is not a
-fast-forward.).
-+
-Pushing an empty <src> allows you to delete the <dst> ref from
-the remote repository.
+on the remote side. Whether this is allowed depends on where in
+`refs/*` the <dst> reference lives as described in detail below, in
+those sections "update" means any modifications except deletes, which
+as noted after the next few sections are treated differently.
++
+The `refs/heads/*` namespace will only accept commit objects, and
+updates only if they can be fast-forwarded.
++
+The `refs/tags/*` namespace will accept any kind of object (as
+commits, trees and blobs can be tagged), and any updates to them will
+be rejected.
++
+It's possible to push any type of object to any namespace outside of
+`refs/{tags,heads}/*`. In the case of tags and commits, these will be
+treated as if they were the commits inside `refs/heads/*` for the
+purposes of whether the update is allowed.
++
+I.e. a fast-forward of commits and tags outside `refs/{tags,heads}/*`
+is allowed, even in cases where what's being fast-forwarded is not a
+commit, but a tag object which happens to point to a new commit which
+is a fast-forward of the commit the last tag (or commit) it's
+replacing. Replacing a tag with an entirely different tag is also
+allowed, if it points to the same commit, as well as pushing a peeled
+tag, i.e. pushing the commit that existing tag object points to, or a
+new tag object which an existing commit points to.
++
+Tree and blob objects outside of `refs/{tags,heads}/*` will be treated
+the same way as if they were inside `refs/tags/*`, any update of them
+will be rejected.
++
+All of the rules described above about what's not allowed as an update
+can be overridden by adding an the optional leading `+` to a refspec
+(or using `--force` command line option). The only exception to this
+is that no amount of forcing will make the `refs/heads/*` namespace
+accept a non-commit object. Hooks and configuration can also override
+or amend these rules, see e.g. `receive.denyNonFastForwards` in
+linkgit:git-config[1] and`pre-receive` and `update` in
+linkgit:githooks[5].
++
+Pushing an empty <src> allows you to delete the <dst> ref from the
+remote repository. Deletions are always accepted without a leading `+`
+in the refspec (or `--force`), except when forbidden by configuration
+or hooks. See `receive.denyDeletes` in linkgit:git-config[1] and
+`pre-receive` and `update` in linkgit:githooks[5].
 +
 The special refspec `:` (or `+:` to allow non-fast-forward updates)
 directs Git to push "matching" branches: for every branch that exists on
diff --git a/Documentation/gitrevisions.txt b/Documentation/gitrevisions.txt
index 1f6cceaefb..d407b7dee1 100644
--- a/Documentation/gitrevisions.txt
+++ b/Documentation/gitrevisions.txt
@@ -19,9 +19,10 @@ walk the revision graph (such as linkgit:git-log[1]), all commits which are
 reachable from that commit. For commands that walk the revision graph one can
 also specify a range of revisions explicitly.
 
-In addition, some Git commands (such as linkgit:git-show[1]) also take
-revision parameters which denote other objects than commits, e.g. blobs
-("files") or trees ("directories of files").
+In addition, some Git commands (such as linkgit:git-show[1] and
+linkgit:git-push[1]) can also take revision parameters which denote
+other objects than commits, e.g. blobs ("files") or trees
+("directories of files").
 
 include::revisions.txt[]
 
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v5 8/9] fetch: document local ref updates with/without --force

[Ævar Arnfjörð Bjarmason]

Refer to the new git-push(1) documentation about when ref updates are
and aren't allowed with and without --force, noting how "git-fetch"
differs from the behavior of "git-push".

Perhaps it would be better to split this all out into a new
gitrefspecs(7) man page, or present this information using tables.

In lieu of that, this is accurate, and fixes a big omission in the
existing refspec docs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/fetch-options.txt    | 15 +++++++++-----
 Documentation/pull-fetch-param.txt | 32 +++++++++++++++++++++++++-----
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 8bc36af4b1..fa0a3151b3 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -68,11 +68,16 @@ endif::git-pull[]
 
 -f::
 --force::
-	When 'git fetch' is used with `<rbranch>:<lbranch>`
-	refspec, it refuses to update the local branch
-	`<lbranch>` unless the remote branch `<rbranch>` it
-	fetches is a descendant of `<lbranch>`.  This option
-	overrides that check.
+	When 'git fetch' is used with `<src>:<dst>` refspec it may
+	refuse to update the local branch as discussed
+ifdef::git-pull[]
+	in the `<refspec>` part of the linkgit:git-fetch[1]
+	documentation.
+endif::git-pull[]
+ifndef::git-pull[]
+	in the `<refspec>` part below.
+endif::git-pull[]
+	This option overrides that check.
 
 -k::
 --keep::
diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
index f1fb08dc68..ab9617ad01 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -33,11 +33,33 @@ name.
 it requests fetching everything up to the given tag.
 +
 The remote ref that matches <src>
-is fetched, and if <dst> is not an empty string, the local
-ref that matches it is fast-forwarded using <src>.
-If the optional plus `+` is used, the local ref
-is updated even if it does not result in a fast-forward
-update.
+is fetched, and if <dst> is not an empty string, an attempt
+is made to update the local ref that matches it.
++
+Whether that update is allowed without `--force` depends on the ref
+namespace it's being fetched to, the type of object being fetched, and
+whether the update is considered to be a fast-forward. Generally, the
+same rules apply for fetching as when pushing, see the `<refspec>...`
+section of linkgit:git-push[1] for what those are. Exceptions to those
+rules particular to 'git fetch' are noted below.
++
+Unlike when pushing with linkgit:git-push[1], any updates to
+`refs/tags/*` will be accepted without `+` in the refspec (or
+`--force`). The receiving promiscuously considers all tag updates from
+a remote to be forced fetches.
++
+Unlike when pushing with linkgit:git-push[1], any updates outside of
+`refs/{tags,heads}/*` will be accepted without `+` in the refspec (or
+`--force`), whether that's swapping e.g. a tree object for a blob, or
+a commit for another commit that's doesn't have the previous commit as
+an ancestor etc.
++
+As with pushing with linkgit:git-push[1], all of the rules described
+above about what's not allowed as an update can be overridden by
+adding an the optional leading `+` to a refspec (or using `--force`
+command line option). The only exception to this is that no amount of
+forcing will make the `refs/heads/*` namespace accept a non-commit
+object.
 +
 [NOTE]
 When the remote branch you want to fetch is known to
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH v5 9/9] fetch: stop clobbering existing tags without --force

[Ævar Arnfjörð Bjarmason]

Change "fetch" to treat "+" in refspecs (aka --force) to mean we
should clobber a local tag of the same name.

This changes the long-standing behavior of "fetch" added in
853a3697dc ("[PATCH] Multi-head fetch.", 2005-08-20). Before this
change, all tag fetches effectively had --force enabled. See the
git-fetch-script code in fast_forward_local() with the comment:

    > Tags need not be pointing at commits so there is no way to
    > guarantee "fast-forward" anyway.

That commit and the rest of the history of "fetch" shows that the
"+" (--force) part of refpecs was only conceived for branch updates,
while tags have accepted any changes from upstream unconditionally and
clobbered the local tag object. Changing this behavior has been
discussed as early as 2011[1].

The current behavior doesn't make sense to me, it easily results in
local tags accidentally being clobbered. We could namespace our tags
per-remote and not locally populate refs/tags/*, but as with my
97716d217c ("fetch: add a --prune-tags option and fetch.pruneTags
config", 2018-02-09) it's easier to work around the current
implementation than to fix the root cause.

So this change implements suggestion #1 from Jeff's 2011 E-Mail[1],
"fetch" now only clobbers the tag if either "+" is provided as part of
the refspec, or if "--force" is provided on the command-line.

This also makes it nicely symmetrical with how "tag" itself works when
creating tags. I.e. we refuse to clobber any existing tags unless
"--force" is supplied. Now we can refuse all such clobbering, whether
it would happen by clobbering a local tag with "tag", or by fetching
it from the remote with "fetch".

Ref updates outside refs/{tags,heads/* are still still not symmetrical
with how "git push" works, as discussed in the recently changed
pull-fetch-param.txt documentation. This change brings the two
divergent behaviors more into line with one another. I don't think
there's any reason "fetch" couldn't fully converge with the behavior
used by "push", but that's a topic for another change.

One of the tests added in 31b808a032 ("clone --single: limit the fetch
refspec to fetched branch", 2012-09-20) is being changed to use
--force where a clone would clobber a tag. This changes nothing about
the existing behavior of the test.

1. https://public-inbox.org/git/20111123221658.GA22313@sigill.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pull-fetch-param.txt | 15 +++++++++++----
 builtin/fetch.c                    | 18 ++++++++++++------
 t/t5516-fetch-push.sh              |  5 +++--
 t/t5612-clone-refspec.sh           |  4 ++--
 4 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
index ab9617ad01..293c6b967d 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -43,10 +43,13 @@ same rules apply for fetching as when pushing, see the `<refspec>...`
 section of linkgit:git-push[1] for what those are. Exceptions to those
 rules particular to 'git fetch' are noted below.
 +
-Unlike when pushing with linkgit:git-push[1], any updates to
-`refs/tags/*` will be accepted without `+` in the refspec (or
-`--force`). The receiving promiscuously considers all tag updates from
-a remote to be forced fetches.
+Until Git version 2.20, and unlike when pushing with
+linkgit:git-push[1], any updates to `refs/tags/*` would be accepted
+without `+` in the refspec (or `--force`). The receiving promiscuously
+considered all tag updates from a remote to be forced fetches.  Since
+Git version 2.20, fetching to update `refs/tags/*` work the same way
+as when pushing. I.e. any updates will be rejected without `+` in the
+refspec (or `--force`).
 +
 Unlike when pushing with linkgit:git-push[1], any updates outside of
 `refs/{tags,heads}/*` will be accepted without `+` in the refspec (or
@@ -54,6 +57,10 @@ Unlike when pushing with linkgit:git-push[1], any updates outside of
 a commit for another commit that's doesn't have the previous commit as
 an ancestor etc.
 +
+Unlike when pushing with linkgit:git-push[1], there is no
+configuration which'll amend these rules, and nothing like a
+`pre-fetch` hook analogous to the `pre-receive` hook.
++
 As with pushing with linkgit:git-push[1], all of the rules described
 above about what's not allowed as an update can be overridden by
 adding an the optional leading `+` to a refspec (or using `--force`
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b0706b3803..ed4ed9d8c4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -667,12 +667,18 @@ static int update_local_ref(struct ref *ref,
 
 	if (!is_null_oid(&ref->old_oid) &&
 	    starts_with(ref->name, "refs/tags/")) {
-		int r;
-		r = s_update_ref("updating tag", ref, 0);
-		format_display(display, r ? '!' : 't', _("[tag update]"),
-			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref, summary_width);
-		return r;
+		if (force || ref->force) {
+			int r;
+			r = s_update_ref("updating tag", ref, 0);
+			format_display(display, r ? '!' : 't', _("[tag update]"),
+				       r ? _("unable to update local ref") : NULL,
+				       remote, pretty_ref, summary_width);
+			return r;
+		} else {
+			format_display(display, '!', _("[rejected]"), _("would clobber existing tag"),
+				       remote, pretty_ref, summary_width);
+			return 1;
+		}
 	}
 
 	current = lookup_commit_reference_gently(the_repository,
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 7f3d4c4965..0e758e2a43 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1015,7 +1015,7 @@ test_force_fetch_tag () {
 	tag_type_description=$1
 	tag_args=$2
 
-	test_expect_success "fetch will clobber an existing $tag_type_description" "
+	test_expect_success "fetch will not clobber an existing $tag_type_description without --force" "
 		mk_test testrepo heads/master &&
 		mk_child testrepo child1 &&
 		mk_child testrepo child2 &&
@@ -1027,7 +1027,8 @@ test_force_fetch_tag () {
 			git add file1 &&
 			git commit -m 'file1' &&
 			git tag $tag_args testTag &&
-			git -C ../child1 fetch origin tag testTag
+			test_must_fail git -C ../child1 fetch origin tag testTag &&
+			git -C ../child1 fetch origin '+refs/tags/*:refs/tags/*'
 		)
 	"
 }
diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
index 5582b3d5fd..e36ac01661 100755
--- a/t/t5612-clone-refspec.sh
+++ b/t/t5612-clone-refspec.sh
@@ -103,7 +103,7 @@ test_expect_success 'clone with --no-tags' '
 test_expect_success '--single-branch while HEAD pointing at master' '
 	(
 		cd dir_master &&
-		git fetch &&
+		git fetch --force &&
 		git for-each-ref refs/remotes/origin |
 		sed -e "/HEAD$/d" \
 		    -e "s|/remotes/origin/|/heads/|" >../actual
@@ -114,7 +114,7 @@ test_expect_success '--single-branch while HEAD pointing at master' '
 	test_cmp expect actual &&
 	(
 		cd dir_master &&
-		git fetch --tags &&
+		git fetch --tags --force &&
 		git for-each-ref refs/tags >../actual
 	) &&
 	git for-each-ref refs/tags >expect &&
-- 
2.19.0.rc1.350.ge57e33dbd1

Re: [PATCH v5 2/9] push tests: make use of unused $1 in test description

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix up a logic error in 380efb65df ("push tests: assert re-pushing
> annotated tags", 2018-07-31), where the $tag_type_description variable
> was assigned to but never used, unlike in the subsequently added
> companion test for fetches in 2d216a7ef6 ("fetch tests: add a test for
> clobbering tag behavior", 2018-04-29).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t5516-fetch-push.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 539c25aada..62d5059f92 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -969,7 +969,7 @@ test_force_push_tag () {
>  	tag_type_description=$1
>  	tag_args=$2
>  
> -	test_expect_success 'force pushing required to update lightweight tag' "
> +	test_expect_success 'force pushing required to update $tag_type_description' "

Of course, $1 needs to be inside "dq-pair" for $tag_type_description
to be substituted ;-)  So I'll tweak it while queuing.

All the other ones in this series looked sensible to me.  Will
replace.

Thanks.

>  		mk_test testrepo heads/master &&
>  		mk_child testrepo child1 &&
>  		mk_child testrepo child2 &&

Re: [PATCH v5 2/9] push tests: make use of unused $1 in test description

[Ævar Arnfjörð Bjarmason]

On Fri, Aug 31 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Fix up a logic error in 380efb65df ("push tests: assert re-pushing
>> annotated tags", 2018-07-31), where the $tag_type_description variable
>> was assigned to but never used, unlike in the subsequently added
>> companion test for fetches in 2d216a7ef6 ("fetch tests: add a test for
>> clobbering tag behavior", 2018-04-29).
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/t5516-fetch-push.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> index 539c25aada..62d5059f92 100755
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -969,7 +969,7 @@ test_force_push_tag () {
>>  	tag_type_description=$1
>>  	tag_args=$2
>>
>> -	test_expect_success 'force pushing required to update lightweight tag' "
>> +	test_expect_success 'force pushing required to update $tag_type_description' "
>
> Of course, $1 needs to be inside "dq-pair" for $tag_type_description
> to be substituted ;-)  So I'll tweak it while queuing.

D'oh! I knew I'd miss something. Hopefully this was the only thing.

> All the other ones in this series looked sensible to me.  Will
> replace.

Thanks!