* [PATCH v2 00/10] "git fetch" should not clobber existing tags without --force
2018-04-29 20:20 ` [PATCH 0/8] "git fetch" should not clobber existing tags without --force Ævar Arnfjörð Bjarmason
@ 2018-07-31 13:07 ` Ævar Arnfjörð Bjarmason
2018-08-13 19:22 ` [PATCH v3 0/7] Prep for " Ævar Arnfjörð Bjarmason
` (7 more replies)
2018-07-31 13:07 ` [PATCH v2 01/10] fetch tests: change "Tag" test tag to "testTag" Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
10 siblings, 8 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-31 13:07 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply [flat|nested] 101+ messages in thread* [PATCH v3 0/7] Prep for "git fetch" should not clobber existing tags without --force
2018-07-31 13:07 ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
@ 2018-08-13 19:22 ` Ævar Arnfjörð Bjarmason
2018-08-13 20:29 ` Junio C Hamano
` (7 more replies)
2018-08-13 19:22 ` [PATCH v3 1/7] fetch tests: change "Tag" test tag to "testTag" Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
7 siblings, 8 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-13 19:22 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply [flat|nested] 101+ messages in thread* Re: [PATCH v3 0/7] Prep for "git fetch" should not clobber existing tags without --force
2018-08-13 19:22 ` [PATCH v3 0/7] Prep for " Ævar Arnfjörð Bjarmason
@ 2018-08-13 20:29 ` Junio C Hamano
2018-08-13 20:37 ` Ævar Arnfjörð Bjarmason
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
7 siblings, 1 reply; 101+ messages in thread
From: Junio C Hamano @ 2018-08-13 20:29 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam
Æ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(-)
^ permalink raw reply [flat|nested] 101+ messages in thread* Re: [PATCH v3 0/7] Prep for "git fetch" should not clobber existing tags without --force
2018-08-13 20:29 ` Junio C Hamano
@ 2018-08-13 20:37 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-13 20:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam
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!
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v4 0/6] "git fetch" should not clobber existing tags without --force
2018-08-13 19:22 ` [PATCH v3 0/7] Prep for " Ævar Arnfjörð Bjarmason
2018-08-13 20:29 ` Junio C Hamano
@ 2018-08-30 20:12 ` Ævar Arnfjörð Bjarmason
2018-08-31 20:09 ` [PATCH v5 0/9] git " Ævar Arnfjörð Bjarmason
` (9 more replies)
2018-08-30 20:12 ` [PATCH v4 1/6] fetch: change "branch" to "reference" in --force -h output Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
7 siblings, 10 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-30 20:12 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg,
Æ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
^ permalink raw reply [flat|nested] 101+ messages in thread* [PATCH v5 0/9] git fetch" should not clobber existing tags without --force
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
@ 2018-08-31 20:09 ` Ævar Arnfjörð Bjarmason
2018-08-31 20:09 ` [PATCH v5 1/9] fetch: change "branch" to "reference" in --force -h output Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 20:09 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud,
Æ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
^ permalink raw reply [flat|nested] 101+ messages in thread* [PATCH v5 1/9] fetch: change "branch" to "reference" in --force -h output
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
2018-08-31 20:09 ` [PATCH v5 0/9] git " Ævar Arnfjörð Bjarmason
@ 2018-08-31 20:09 ` Ævar Arnfjörð Bjarmason
2018-08-31 20:09 ` [PATCH v5 2/9] push tests: make use of unused $1 in test description Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 20:09 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v5 2/9] push tests: make use of unused $1 in test description
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
2018-08-31 20:09 ` [PATCH v5 0/9] git " Ævar Arnfjörð Bjarmason
2018-08-31 20:09 ` [PATCH v5 1/9] fetch: change "branch" to "reference" in --force -h output Ævar Arnfjörð Bjarmason
@ 2018-08-31 20:09 ` Ævar Arnfjörð Bjarmason
2018-08-31 21:07 ` Junio C Hamano
2018-08-31 20:09 ` [PATCH v5 3/9] push tests: use spaces in interpolated string Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
9 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 20:09 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* Re: [PATCH v5 2/9] push tests: make use of unused $1 in test description
2018-08-31 20:09 ` [PATCH v5 2/9] push tests: make use of unused $1 in test description Ævar Arnfjörð Bjarmason
@ 2018-08-31 21:07 ` Junio C Hamano
2018-08-31 22:02 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 101+ messages in thread
From: Junio C Hamano @ 2018-08-31 21:07 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud
Æ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 &&
^ permalink raw reply [flat|nested] 101+ messages in thread* Re: [PATCH v5 2/9] push tests: make use of unused $1 in test description
2018-08-31 21:07 ` Junio C Hamano
@ 2018-08-31 22:02 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 22:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud
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!
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v5 3/9] push tests: use spaces in interpolated string
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2018-08-31 20:09 ` [PATCH v5 2/9] push tests: make use of unused $1 in test description Ævar Arnfjörð Bjarmason
@ 2018-08-31 20:09 ` Ævar Arnfjörð Bjarmason
2018-08-31 20:09 ` [PATCH v5 4/9] fetch tests: add a test for clobbering tag behavior Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 20:09 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v5 4/9] fetch tests: add a test for clobbering tag behavior
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2018-08-31 20:09 ` [PATCH v5 3/9] push tests: use spaces in interpolated string Ævar Arnfjörð Bjarmason
@ 2018-08-31 20:09 ` Ævar Arnfjörð Bjarmason
2018-08-31 20:10 ` [PATCH v5 5/9] push doc: remove confusing mention of remote merger Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 20:09 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v5 5/9] push doc: remove confusing mention of remote merger
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2018-08-31 20:09 ` [PATCH v5 4/9] fetch tests: add a test for clobbering tag behavior Ævar Arnfjörð Bjarmason
@ 2018-08-31 20:10 ` Ævar Arnfjörð Bjarmason
2018-08-31 20:10 ` [PATCH v5 6/9] push doc: move mention of "tag <tag>" later in the prose Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 20:10 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v5 6/9] push doc: move mention of "tag <tag>" later in the prose
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2018-08-31 20:10 ` [PATCH v5 5/9] push doc: remove confusing mention of remote merger Ævar Arnfjörð Bjarmason
@ 2018-08-31 20:10 ` Ævar Arnfjörð Bjarmason
2018-08-31 20:10 ` [PATCH v5 7/9] push doc: correct lies about how push refspecs work Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 20:10 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v5 7/9] push doc: correct lies about how push refspecs work
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2018-08-31 20:10 ` [PATCH v5 6/9] push doc: move mention of "tag <tag>" later in the prose Ævar Arnfjörð Bjarmason
@ 2018-08-31 20:10 ` Ævar Arnfjörð Bjarmason
2018-08-31 20:10 ` [PATCH v5 8/9] fetch: document local ref updates with/without --force Ævar Arnfjörð Bjarmason
2018-08-31 20:10 ` [PATCH v5 9/9] fetch: stop clobbering existing tags without --force Ævar Arnfjörð Bjarmason
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 20:10 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v5 8/9] fetch: document local ref updates with/without --force
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2018-08-31 20:10 ` [PATCH v5 7/9] push doc: correct lies about how push refspecs work Ævar Arnfjörð Bjarmason
@ 2018-08-31 20:10 ` Ævar Arnfjörð Bjarmason
2018-08-31 20:10 ` [PATCH v5 9/9] fetch: stop clobbering existing tags without --force Ævar Arnfjörð Bjarmason
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 20:10 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v5 9/9] fetch: stop clobbering existing tags without --force
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2018-08-31 20:10 ` [PATCH v5 8/9] fetch: document local ref updates with/without --force Ævar Arnfjörð Bjarmason
@ 2018-08-31 20:10 ` Ævar Arnfjörð Bjarmason
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 20:10 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Marc Branchaud,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v4 1/6] fetch: change "branch" to "reference" in --force -h output
2018-08-13 19:22 ` [PATCH v3 0/7] Prep for " Ævar Arnfjörð Bjarmason
2018-08-13 20:29 ` Junio C Hamano
2018-08-30 20:12 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
@ 2018-08-30 20:12 ` Ævar Arnfjörð Bjarmason
2018-08-30 20:12 ` [PATCH v4 2/6] push tests: correct quoting in interpolated string Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
7 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-30 20:12 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v4 2/6] push tests: correct quoting in interpolated string
2018-08-13 19:22 ` [PATCH v3 0/7] Prep for " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2018-08-30 20:12 ` [PATCH v4 1/6] fetch: change "branch" to "reference" in --force -h output Ævar Arnfjörð Bjarmason
@ 2018-08-30 20:12 ` Ævar Arnfjörð Bjarmason
2018-08-30 21:20 ` Junio C Hamano
2018-08-30 20:12 ` [PATCH v4 3/6] fetch tests: add a test for clobbering tag behavior Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
7 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-30 20:12 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* Re: [PATCH v4 2/6] push tests: correct quoting in interpolated string
2018-08-30 20:12 ` [PATCH v4 2/6] push tests: correct quoting in interpolated string Ævar Arnfjörð Bjarmason
@ 2018-08-30 21:20 ` Junio C Hamano
0 siblings, 0 replies; 101+ messages in thread
From: Junio C Hamano @ 2018-08-30 21:20 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg
Æ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.
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v4 3/6] fetch tests: add a test for clobbering tag behavior
2018-08-13 19:22 ` [PATCH v3 0/7] Prep for " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2018-08-30 20:12 ` [PATCH v4 2/6] push tests: correct quoting in interpolated string Ævar Arnfjörð Bjarmason
@ 2018-08-30 20:12 ` Ævar Arnfjörð Bjarmason
2018-08-30 21:22 ` Junio C Hamano
2018-08-30 20:12 ` [PATCH v4 4/6] push doc: correct lies about how push refspecs work Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
7 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-30 20:12 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* Re: [PATCH v4 3/6] fetch tests: add a test for clobbering tag behavior
2018-08-30 20:12 ` [PATCH v4 3/6] fetch tests: add a test for clobbering tag behavior Ævar Arnfjörð Bjarmason
@ 2018-08-30 21:22 ` Junio C Hamano
0 siblings, 0 replies; 101+ messages in thread
From: Junio C Hamano @ 2018-08-30 21:22 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg
Æ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" &&
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v4 4/6] push doc: correct lies about how push refspecs work
2018-08-13 19:22 ` [PATCH v3 0/7] Prep for " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2018-08-30 20:12 ` [PATCH v4 3/6] fetch tests: add a test for clobbering tag behavior Ævar Arnfjörð Bjarmason
@ 2018-08-30 20:12 ` Ævar Arnfjörð Bjarmason
2018-08-30 21:31 ` Junio C Hamano
2018-08-30 22:34 ` Ævar Arnfjörð Bjarmason
2018-08-30 20:12 ` [PATCH v4 5/6] fetch: document local ref updates with/without --force Ævar Arnfjörð Bjarmason
2018-08-30 20:12 ` [PATCH v4 6/6] fetch: stop clobbering existing tags without --force Ævar Arnfjörð Bjarmason
7 siblings, 2 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-30 20:12 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work
2018-08-30 20:12 ` [PATCH v4 4/6] push doc: correct lies about how push refspecs work Ævar Arnfjörð Bjarmason
@ 2018-08-30 21:31 ` Junio C Hamano
2018-08-30 22:34 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 101+ messages in thread
From: Junio C Hamano @ 2018-08-30 21:31 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg
Æ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.
^ permalink raw reply [flat|nested] 101+ messages in thread* Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work
2018-08-30 20:12 ` [PATCH v4 4/6] push doc: correct lies about how push refspecs work Ævar Arnfjörð Bjarmason
2018-08-30 21:31 ` Junio C Hamano
@ 2018-08-30 22:34 ` Ævar Arnfjörð Bjarmason
2018-08-31 16:24 ` Junio C Hamano
1 sibling, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-30 22:34 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg
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?
^ permalink raw reply [flat|nested] 101+ messages in thread* Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work
2018-08-30 22:34 ` Ævar Arnfjörð Bjarmason
@ 2018-08-31 16:24 ` Junio C Hamano
2018-08-31 16:35 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 101+ messages in thread
From: Junio C Hamano @ 2018-08-31 16:24 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg
Æ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.
^ permalink raw reply [flat|nested] 101+ messages in thread
* Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work
2018-08-31 16:24 ` Junio C Hamano
@ 2018-08-31 16:35 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-31 16:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg
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.
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v4 5/6] fetch: document local ref updates with/without --force
2018-08-13 19:22 ` [PATCH v3 0/7] Prep for " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2018-08-30 20:12 ` [PATCH v4 4/6] push doc: correct lies about how push refspecs work Ævar Arnfjörð Bjarmason
@ 2018-08-30 20:12 ` Ævar Arnfjörð Bjarmason
2018-08-30 20:12 ` [PATCH v4 6/6] fetch: stop clobbering existing tags without --force Ævar Arnfjörð Bjarmason
7 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-30 20:12 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v4 6/6] fetch: stop clobbering existing tags without --force
2018-08-13 19:22 ` [PATCH v3 0/7] Prep for " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2018-08-30 20:12 ` [PATCH v4 5/6] fetch: document local ref updates with/without --force Ævar Arnfjörð Bjarmason
@ 2018-08-30 20:12 ` Ævar Arnfjörð Bjarmason
2018-08-30 21:43 ` Junio C Hamano
7 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-30 20:12 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg,
Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* Re: [PATCH v4 6/6] fetch: stop clobbering existing tags without --force
2018-08-30 20:12 ` [PATCH v4 6/6] fetch: stop clobbering existing tags without --force Ævar Arnfjörð Bjarmason
@ 2018-08-30 21:43 ` Junio C Hamano
0 siblings, 0 replies; 101+ messages in thread
From: Junio C Hamano @ 2018-08-30 21:43 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Kristian Høgsberg
Æ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.
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v3 1/7] fetch tests: change "Tag" test tag to "testTag"
2018-07-31 13:07 ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
2018-08-13 19:22 ` [PATCH v3 0/7] Prep for " Ævar Arnfjörð Bjarmason
@ 2018-08-13 19:22 ` Ævar Arnfjörð Bjarmason
2018-08-13 19:22 ` [PATCH v3 2/7] push tests: remove redundant 'git push' invocation Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
7 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-13 19:22 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v3 2/7] push tests: remove redundant 'git push' invocation
2018-07-31 13:07 ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
2018-08-13 19:22 ` [PATCH v3 0/7] Prep for " Ævar Arnfjörð Bjarmason
2018-08-13 19:22 ` [PATCH v3 1/7] fetch tests: change "Tag" test tag to "testTag" Ævar Arnfjörð Bjarmason
@ 2018-08-13 19:22 ` Ævar Arnfjörð Bjarmason
2018-08-13 19:22 ` [PATCH v3 3/7] push tests: fix logic error in "push" test assertion Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
7 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-13 19:22 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v3 3/7] push tests: fix logic error in "push" test assertion
2018-07-31 13:07 ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2018-08-13 19:22 ` [PATCH v3 2/7] push tests: remove redundant 'git push' invocation Ævar Arnfjörð Bjarmason
@ 2018-08-13 19:22 ` Ævar Arnfjörð Bjarmason
2018-08-13 19:22 ` [PATCH v3 4/7] push tests: add more testing for forced tag pushing Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
7 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-13 19:22 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v3 4/7] push tests: add more testing for forced tag pushing
2018-07-31 13:07 ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2018-08-13 19:22 ` [PATCH v3 3/7] push tests: fix logic error in "push" test assertion Ævar Arnfjörð Bjarmason
@ 2018-08-13 19:22 ` Ævar Arnfjörð Bjarmason
2018-08-13 19:22 ` [PATCH v3 5/7] push tests: assert re-pushing annotated tags Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
7 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-13 19:22 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v3 5/7] push tests: assert re-pushing annotated tags
2018-07-31 13:07 ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2018-08-13 19:22 ` [PATCH v3 4/7] push tests: add more testing for forced tag pushing Ævar Arnfjörð Bjarmason
@ 2018-08-13 19:22 ` Ævar Arnfjörð Bjarmason
2018-08-13 19:22 ` [PATCH v3 6/7] fetch tests: correct a comment "remove it" -> "remove them" Ævar Arnfjörð Bjarmason
2018-08-13 19:22 ` [PATCH v3 7/7] pull doc: fix a long-standing grammar error Ævar Arnfjörð Bjarmason
7 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-13 19:22 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v3 6/7] fetch tests: correct a comment "remove it" -> "remove them"
2018-07-31 13:07 ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2018-08-13 19:22 ` [PATCH v3 5/7] push tests: assert re-pushing annotated tags Ævar Arnfjörð Bjarmason
@ 2018-08-13 19:22 ` Ævar Arnfjörð Bjarmason
2018-08-13 19:22 ` [PATCH v3 7/7] pull doc: fix a long-standing grammar error Ævar Arnfjörð Bjarmason
7 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-13 19:22 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v3 7/7] pull doc: fix a long-standing grammar error
2018-07-31 13:07 ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2018-08-13 19:22 ` [PATCH v3 6/7] fetch tests: correct a comment "remove it" -> "remove them" Ævar Arnfjörð Bjarmason
@ 2018-08-13 19:22 ` Ævar Arnfjörð Bjarmason
7 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-13 19:22 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v2 01/10] fetch tests: change "Tag" test tag to "testTag"
2018-04-29 20:20 ` [PATCH 0/8] "git fetch" should not clobber existing tags without --force Ævar Arnfjörð Bjarmason
2018-07-31 13:07 ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
@ 2018-07-31 13:07 ` Ævar Arnfjörð Bjarmason
2018-07-31 13:07 ` [PATCH v2 02/10] push tests: remove redundant 'git push' invocation Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-31 13:07 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v2 02/10] push tests: remove redundant 'git push' invocation
2018-04-29 20:20 ` [PATCH 0/8] "git fetch" should not clobber existing tags without --force Ævar Arnfjörð Bjarmason
2018-07-31 13:07 ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
2018-07-31 13:07 ` [PATCH v2 01/10] fetch tests: change "Tag" test tag to "testTag" Ævar Arnfjörð Bjarmason
@ 2018-07-31 13:07 ` Ævar Arnfjörð Bjarmason
2018-07-31 13:07 ` [PATCH v2 03/10] push tests: fix logic error in "push" test assertion Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-31 13:07 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v2 03/10] push tests: fix logic error in "push" test assertion
2018-04-29 20:20 ` [PATCH 0/8] "git fetch" should not clobber existing tags without --force Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2018-07-31 13:07 ` [PATCH v2 02/10] push tests: remove redundant 'git push' invocation Ævar Arnfjörð Bjarmason
@ 2018-07-31 13:07 ` Ævar Arnfjörð Bjarmason
2018-07-31 13:07 ` [PATCH v2 04/10] push tests: add more testing for forced tag pushing Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-31 13:07 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v2 04/10] push tests: add more testing for forced tag pushing
2018-04-29 20:20 ` [PATCH 0/8] "git fetch" should not clobber existing tags without --force Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2018-07-31 13:07 ` [PATCH v2 03/10] push tests: fix logic error in "push" test assertion Ævar Arnfjörð Bjarmason
@ 2018-07-31 13:07 ` Ævar Arnfjörð Bjarmason
2018-07-31 13:07 ` [PATCH v2 05/10] push tests: assert re-pushing annotated tags Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-31 13:07 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v2 05/10] push tests: assert re-pushing annotated tags
2018-04-29 20:20 ` [PATCH 0/8] "git fetch" should not clobber existing tags without --force Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2018-07-31 13:07 ` [PATCH v2 04/10] push tests: add more testing for forced tag pushing Ævar Arnfjörð Bjarmason
@ 2018-07-31 13:07 ` Ævar Arnfjörð Bjarmason
2018-07-31 13:07 ` [PATCH v2 06/10] push doc: correct lies about how push refspecs work Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-31 13:07 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v2 06/10] push doc: correct lies about how push refspecs work
2018-04-29 20:20 ` [PATCH 0/8] "git fetch" should not clobber existing tags without --force Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2018-07-31 13:07 ` [PATCH v2 05/10] push tests: assert re-pushing annotated tags Ævar Arnfjörð Bjarmason
@ 2018-07-31 13:07 ` Ævar Arnfjörð Bjarmason
2018-07-31 17:40 ` Junio C Hamano
2018-07-31 13:07 ` [PATCH v2 07/10] fetch tests: correct a comment "remove it" -> "remove them" Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
10 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-31 13:07 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work
2018-07-31 13:07 ` [PATCH v2 06/10] push doc: correct lies about how push refspecs work Ævar Arnfjörð Bjarmason
@ 2018-07-31 17:40 ` Junio C Hamano
2018-08-30 14:52 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 101+ messages in thread
From: Junio C Hamano @ 2018-07-31 17:40 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam
Æ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.
^ permalink raw reply [flat|nested] 101+ messages in thread* Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work
2018-07-31 17:40 ` Junio C Hamano
@ 2018-08-30 14:52 ` Ævar Arnfjörð Bjarmason
2018-08-30 15:23 ` Junio C Hamano
0 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-30 14:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam
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.
^ permalink raw reply [flat|nested] 101+ messages in thread* Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work
2018-08-30 14:52 ` Ævar Arnfjörð Bjarmason
@ 2018-08-30 15:23 ` Junio C Hamano
2018-08-30 16:59 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 101+ messages in thread
From: Junio C Hamano @ 2018-08-30 15:23 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam
Æ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.
^ permalink raw reply [flat|nested] 101+ messages in thread* Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work
2018-08-30 15:23 ` Junio C Hamano
@ 2018-08-30 16:59 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-30 16:59 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam
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.
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v2 07/10] fetch tests: correct a comment "remove it" -> "remove them"
2018-04-29 20:20 ` [PATCH 0/8] "git fetch" should not clobber existing tags without --force Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2018-07-31 13:07 ` [PATCH v2 06/10] push doc: correct lies about how push refspecs work Ævar Arnfjörð Bjarmason
@ 2018-07-31 13:07 ` Ævar Arnfjörð Bjarmason
2018-07-31 13:07 ` [PATCH v2 08/10] fetch tests: add a test clobbering tag behavior Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-31 13:07 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v2 08/10] fetch tests: add a test clobbering tag behavior
2018-04-29 20:20 ` [PATCH 0/8] "git fetch" should not clobber existing tags without --force Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2018-07-31 13:07 ` [PATCH v2 07/10] fetch tests: correct a comment "remove it" -> "remove them" Ævar Arnfjörð Bjarmason
@ 2018-07-31 13:07 ` Ævar Arnfjörð Bjarmason
2018-07-31 17:48 ` Junio C Hamano
2018-07-31 13:07 ` [PATCH v2 09/10] pull doc: fix a long-standing grammar error Ævar Arnfjörð Bjarmason
2018-07-31 13:07 ` [PATCH v2 10/10] fetch: stop clobbering existing tags without --force Ævar Arnfjörð Bjarmason
10 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-31 13:07 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* Re: [PATCH v2 08/10] fetch tests: add a test clobbering tag behavior
2018-07-31 13:07 ` [PATCH v2 08/10] fetch tests: add a test clobbering tag behavior Ævar Arnfjörð Bjarmason
@ 2018-07-31 17:48 ` Junio C Hamano
0 siblings, 0 replies; 101+ messages in thread
From: Junio C Hamano @ 2018-07-31 17:48 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam
Æ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" &&
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v2 09/10] pull doc: fix a long-standing grammar error
2018-04-29 20:20 ` [PATCH 0/8] "git fetch" should not clobber existing tags without --force Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2018-07-31 13:07 ` [PATCH v2 08/10] fetch tests: add a test clobbering tag behavior Ævar Arnfjörð Bjarmason
@ 2018-07-31 13:07 ` Ævar Arnfjörð Bjarmason
2018-07-31 13:07 ` [PATCH v2 10/10] fetch: stop clobbering existing tags without --force Ævar Arnfjörð Bjarmason
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-31 13:07 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* [PATCH v2 10/10] fetch: stop clobbering existing tags without --force
2018-04-29 20:20 ` [PATCH 0/8] "git fetch" should not clobber existing tags without --force Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2018-07-31 13:07 ` [PATCH v2 09/10] pull doc: fix a long-standing grammar error Ævar Arnfjörð Bjarmason
@ 2018-07-31 13:07 ` Ævar Arnfjörð Bjarmason
2018-07-31 18:03 ` Junio C Hamano
10 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-31 13:07 UTC (permalink / raw)
To: git
Cc: Wink Saville, Jacob Keller, Bryan Turner, Junio C Hamano,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam, Æ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
^ permalink raw reply related [flat|nested] 101+ messages in thread* Re: [PATCH v2 10/10] fetch: stop clobbering existing tags without --force
2018-07-31 13:07 ` [PATCH v2 10/10] fetch: stop clobbering existing tags without --force Ævar Arnfjörð Bjarmason
@ 2018-07-31 18:03 ` Junio C Hamano
0 siblings, 0 replies; 101+ messages in thread
From: Junio C Hamano @ 2018-07-31 18:03 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Wink Saville, Jacob Keller, Bryan Turner,
Uwe Kleine-König, Jeff King, SZEDER Gábor,
Kaartic Sivaraam
Æ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?
^ permalink raw reply [flat|nested] 101+ messages in thread