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

2018-07-31 Thread Junio C Hamano
Ævar Arnfjörð Bjarmason   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 `:`
> - 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 may
> + 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.

Ah, that's tricky.  I could not locate "the `` 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 
> -is fetched, and if  is not an 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.
> +is fetched, and if  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 v2 10/10] fetch: stop clobbering existing tags without --force

2018-07-31 Thread Æ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/2023221658.ga22...@sigill.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 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 `:`
-   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 may
+   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 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 
-is fetched, and if  is not an 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.
+is fetched, and if  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[] =