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

From: Junio C Hamano <gitster@pobox.com>
Date: May 8, 2018, 05:37 UTC
Message-ID: <xmqqvabylnbi.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180429202100.32353-9-avarab@gmail.com>

Æ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.

Show 5 quoted lines

> 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.

Show 14 quoted lines

> 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

Show 22 quoted lines

> 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.