Ævar Arnfjörð Bjarmason <ava...@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.
