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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   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 `:`
> - refspec, it refuses to update the local branch
> - `` unless the remote branch `` it
> - fetches is a descendant of ``.  This option
> - overrides that check.
> + When 'git fetch' is used with `:` 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 ` means the same as `refs/tags/:refs/tags/`;
>  it requests fetching everything up to the given tag.
>  +
> -The remote ref that matches 
> -is fetched, and if  is not empty string, the local
> -ref that matches it is fast-forwarded using .
> -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  is fetched, and if  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 `...`
> +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(, N_("force overwrite of local branch"), 0),
> + OPT__FORCE(, N_("force overwrite of local reference"), 0),

Good.  This is long overdue.

[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/2023221658.ga22...@sigill.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 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 `:`
-   refspec, it refuses to update the local branch
-   `` unless the remote branch `` it
-   fetches is a descendant of ``.  This option
-   overrides that check.
+   When 'git fetch' is used with `:` refspec it might
+   refuse to update the local branch as discussed
+ifdef::git-pull[]
+   in the `` part of the linkgit:git-fetch[1]
+   documentation.
+endif::git-pull[]
+ifndef::git-pull[]
+   in the `` 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 ` means the same as `refs/tags/:refs/tags/`;
 it requests fetching everything up to the given tag.
 +
-The remote ref that matches 
-is fetched, and if  is not empty string, the local
-ref that matches it is fast-forwarded using .
-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  is fetched, and if  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 `...`
+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 @@