Re: Simple git push --tags deleted all branches
On Thu, 29 Nov 2018 at 16:39, Ævar Arnfjörð Bjarmason wrote: > On Thu, Nov 29 2018, Mateusz Loskot wrote: > > On Thu, 29 Nov 2018 at 16:03, Ævar Arnfjörð Bjarmason > > wrote: > >> On Wed, Nov 28 2018, Mateusz Loskot wrote: > >> > > >> > (using git version 2.19.2.windows.1) > >> > > >> > I've just encountered one of those WTH moments. > >> > > >> > I have a bare repository > >> > > >> > core.git (BARE:master) $ git branch > >> > 1.0 > >> > 2.0 > >> > * master > >> > > >> > core.git (BARE:master) $ git tag > >> > 1.0.1651 > >> > 1.0.766 > >> > 2.0.1103 > >> > 2.0.1200 > >> > > >> > I published the repo using: git push --all --follow-tags > >> > > >> > This succeeded, but there seem to be no tags pushed, just branches. > >> > So, I followed with > >> > > >> > core.git (BARE:master) $ git push --tags > >> > To XXX > >> > - [deleted] 1.0 > >> > - [deleted] 2.0 > >> > ! [remote rejected] master (refusing to delete the current > >> > branch: refs/heads/master) > >> > error: failed to push some refs to 'XXX' > >> > > >> > And, I've found out that all branches and tags have been > >> > wiped in both, local repo and remote :) > >> > > >> > I restored the repo and tried out > >> > > >> > git push origin 1.0 > >> > git push origin --tags > >> > > >> > and this time both succeeded, without wiping out any refs. > >> > > >> > Could anyone help me to understand why remote-less > >> > > >> > git push --tags > >> > > >> > is/was so dangerous and unforgiving?! > >> > >> Since nobody's replied yet, I can't see what's going on here from the > >> info you've provided. My guess is that you have something "mirror" set > >> on the remote. > > > > Thank you for responding. > > > > The git push --tags hugely surprised me, and here is genuine screenshot > > https://twitter.com/mloskot/status/1068072285846859776 > > > >> It seems you can't share the repo or its URL, but could you share the > >> scrubbed output of 'git config -l --show-origin' when run inside this > >> repository? > > > > Here is complete output. I have not stripped the basics like aliases, > > just in case. > > Right, it's because you used --mirror, the important bit: > > > file:config remote.origin.url=https://xxx.com/core-external-metadata.git > > file:config remote.origin.fetch=+refs/*:refs/* > > file:config remote.origin.mirror=true > > file:config > > I.e. you have cloned with the --mirror flag, this is what it's supposed > to do: https://git-scm.com/docs/git-clone#git-clone---mirror > https://git-scm.com/docs/git-fetch#git-fetch---prune > > I.e. you push and git tries to mirror the refs you have locally to the > remote, including pruning stuff in the remote. Thank you very much for diagnosing my issue. I was not aware about how --mirror affects the workflow. It all makes perfect sense now. Best regards, -- Mateusz Loskot, http://mateusz.loskot.net
Re: Simple git push --tags deleted all branches
On Thu, Nov 29 2018, Mateusz Loskot wrote: > On Thu, 29 Nov 2018 at 16:03, Ævar Arnfjörð Bjarmason > wrote: >> On Wed, Nov 28 2018, Mateusz Loskot wrote: >> > >> > (using git version 2.19.2.windows.1) >> > >> > I've just encountered one of those WTH moments. >> > >> > I have a bare repository >> > >> > core.git (BARE:master) $ git branch >> > 1.0 >> > 2.0 >> > * master >> > >> > core.git (BARE:master) $ git tag >> > 1.0.1651 >> > 1.0.766 >> > 2.0.1103 >> > 2.0.1200 >> > >> > I published the repo using: git push --all --follow-tags >> > >> > This succeeded, but there seem to be no tags pushed, just branches. >> > So, I followed with >> > >> > core.git (BARE:master) $ git push --tags >> > To XXX >> > - [deleted] 1.0 >> > - [deleted] 2.0 >> > ! [remote rejected] master (refusing to delete the current >> > branch: refs/heads/master) >> > error: failed to push some refs to 'XXX' >> > >> > And, I've found out that all branches and tags have been >> > wiped in both, local repo and remote :) >> > >> > I restored the repo and tried out >> > >> > git push origin 1.0 >> > git push origin --tags >> > >> > and this time both succeeded, without wiping out any refs. >> > >> > Could anyone help me to understand why remote-less >> > >> > git push --tags >> > >> > is/was so dangerous and unforgiving?! >> >> Since nobody's replied yet, I can't see what's going on here from the >> info you've provided. My guess is that you have something "mirror" set >> on the remote. > > Thank you for responding. > > The git push --tags hugely surprised me, and here is genuine screenshot > https://twitter.com/mloskot/status/1068072285846859776 > >> It seems you can't share the repo or its URL, but could you share the >> scrubbed output of 'git config -l --show-origin' when run inside this >> repository? > > Here is complete output. I have not stripped the basics like aliases, > just in case. Right, it's because you used --mirror, the important bit: > file:config remote.origin.url=https://xxx.com/core-external-metadata.git > file:config remote.origin.fetch=+refs/*:refs/* > file:config remote.origin.mirror=true > file:config I.e. you have cloned with the --mirror flag, this is what it's supposed to do: https://git-scm.com/docs/git-clone#git-clone---mirror https://git-scm.com/docs/git-fetch#git-fetch---prune I.e. you push and git tries to mirror the refs you have locally to the remote, including pruning stuff in the remote. This is useful, but not what you wanted here. It's used for e.g. making an up-to-date copy of a repo from server A to server B in HA setups where you'd like to fail over to server B and get the same refs you had on A.
Re: Simple git push --tags deleted all branches
On Thu, 29 Nov 2018 at 16:03, Ævar Arnfjörð Bjarmason wrote: > On Wed, Nov 28 2018, Mateusz Loskot wrote: > > > > (using git version 2.19.2.windows.1) > > > > I've just encountered one of those WTH moments. > > > > I have a bare repository > > > > core.git (BARE:master) $ git branch > > 1.0 > > 2.0 > > * master > > > > core.git (BARE:master) $ git tag > > 1.0.1651 > > 1.0.766 > > 2.0.1103 > > 2.0.1200 > > > > I published the repo using: git push --all --follow-tags > > > > This succeeded, but there seem to be no tags pushed, just branches. > > So, I followed with > > > > core.git (BARE:master) $ git push --tags > > To XXX > > - [deleted] 1.0 > > - [deleted] 2.0 > > ! [remote rejected] master (refusing to delete the current > > branch: refs/heads/master) > > error: failed to push some refs to 'XXX' > > > > And, I've found out that all branches and tags have been > > wiped in both, local repo and remote :) > > > > I restored the repo and tried out > > > > git push origin 1.0 > > git push origin --tags > > > > and this time both succeeded, without wiping out any refs. > > > > Could anyone help me to understand why remote-less > > > > git push --tags > > > > is/was so dangerous and unforgiving?! > > Since nobody's replied yet, I can't see what's going on here from the > info you've provided. My guess is that you have something "mirror" set > on the remote. Thank you for responding. The git push --tags hugely surprised me, and here is genuine screenshot https://twitter.com/mloskot/status/1068072285846859776 > It seems you can't share the repo or its URL, but could you share the > scrubbed output of 'git config -l --show-origin' when run inside this > repository? Here is complete output. I have not stripped the basics like aliases, just in case. ``` core-external-metadata.git (BARE:master) $ git config -l --show-origin file:"C:\\ProgramData/Git/config" core.symlinks=false file:"C:\\ProgramData/Git/config" core.autocrlf=true file:"C:\\ProgramData/Git/config" color.diff=auto file:"C:\\ProgramData/Git/config" color.status=auto file:"C:\\ProgramData/Git/config" color.branch=auto file:"C:\\ProgramData/Git/config" color.interactive=true file:"C:\\ProgramData/Git/config" help.format=html file:"C:\\ProgramData/Git/config" http.sslcainfo=C:/Program Files/Git/mingw64/ssl/certs/ca-bundle.crt file:"C:\\ProgramData/Git/config" diff.astextplain.textconv=astextplain file:"C:\\ProgramData/Git/config" rebase.autosquash=true file:C:/Program Files/Git/mingw64/etc/gitconfig http.sslcainfo=C:/Program Files/Git/mingw64/ssl/certs/ca-bundle.crt file:C:/Program Files/Git/mingw64/etc/gitconfig http.sslbackend=openssl file:C:/Program Files/Git/mingw64/etc/gitconfig diff.astextplain.textconv=astextplain file:C:/Program Files/Git/mingw64/etc/gitconfig filter.lfs.clean=git-lfs clean -- %f file:C:/Program Files/Git/mingw64/etc/gitconfig filter.lfs.smudge=git-lfs smudge -- %f file:C:/Program Files/Git/mingw64/etc/gitconfig filter.lfs.process=git-lfs filter-process file:C:/Program Files/Git/mingw64/etc/gitconfig filter.lfs.required=true file:C:/Program Files/Git/mingw64/etc/gitconfig credential.helper=manager file:C:/Users/mateuszl/.gitconfig user.name=Mateusz Łoskot file:C:/Users/mateuszl/.gitconfig user.email=mate...@loskot.net file:C:/Users/mateuszl/.gitconfig github.user=mloskot file:C:/Users/mateuszl/.gitconfig core.editor=vim file:C:/Users/mateuszl/.gitconfig color.branch=auto file:C:/Users/mateuszl/.gitconfig color.diff=auto file:C:/Users/mateuszl/.gitconfig color.status=auto file:C:/Users/mateuszl/.gitconfig color.ui=auto file:C:/Users/mateuszl/.gitconfig alias.br=branch file:C:/Users/mateuszl/.gitconfig alias.bv=branch -vv file:C:/Users/mateuszl/.gitconfig alias.brv=branch -vv file:C:/Users/mateuszl/.gitconfig alias.bra=branch -a file:C:/Users/mateuszl/.gitconfig alias.ci=commit --verbose file:C:/Users/mateuszl/.gitconfig alias.cia=commit --verbose --amend file:C:/Users/mateuszl/.gitconfig alias.ciae=commit --verbose --amend --no-edit file:C:/Users/mateuszl/.gitconfig alias.co=checkout file:C:/Users/mateuszl/.gitconfig alias.dc=diff --cached file:C:/Users/mateuszl/.gitconfig alias.df=diff file:C:/Users/mateuszl/.gitconfig alias.gf=flow file:C:/Users/mateuszl/.gitconfig alias.lg=log --color --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr) %C(bold blue)<%an>%Creset' --abbre
Re: Git Tags
On Thu, Nov 29 2018, Stefanie Leisestreichler wrote: > Hi. > > I have done this (on box A): > > git commit -m "Message" > git tag -a 0.9.0 > git push origin master > > In my local repository, when I run "git tag" it is showing me "0.9.0". > > Then I did (on box B) > git clone ssh://user@host:/path/project.git > cd project > git tag > > Now git tag is showing nothing. > > Why is the tag only available in my local repository? > > Also when I try to > git clone --branch 0.9.0 ssh://user@host:/path/project.git > it tells me: fatal:remote branch not found in upstream repository origin Because --branch means get refs/heads/, tags are not branches. However, because we're apparently quite loose about this in the clone/fetch code this does give you the tag if it exists, but probably not in the way you expect. We interpret the argument as a branch, and will get not only this tag but "follow" (see --no-tags in git-fetch(1)) the tag as though it were a branch and give you all tags leading up to that one. This would give you a single tag: git clone --no-tags --branch v2.19.0 --single-branch https://github.com/git/git.git But this is a more direct way to do it: git init git; git -C git fetch --no-tags https://github.com/git/git.git tag v2.19.0 Which'll since you said it failed that's because you haven't pushed the tag. Try 'git ls-remote ' to see if it's there (it's not).
Re: Simple git push --tags deleted all branches
On Wed, Nov 28 2018, Mateusz Loskot wrote: > Hi, > > (using git version 2.19.2.windows.1) > > I've just encountered one of those WTH moments. > > I have a bare repository > > core.git (BARE:master) $ git branch > 1.0 > 2.0 > * master > > core.git (BARE:master) $ git tag > 1.0.1651 > 1.0.766 > 2.0.1103 > 2.0.1200 > > I published the repo using: git push --all --follow-tags > > This succeeded, but there seem to be no tags pushed, just branches. > So, I followed with > > core.git (BARE:master) $ git push --tags > To XXX > - [deleted] 1.0 > - [deleted] 2.0 > ! [remote rejected] master (refusing to delete the current > branch: refs/heads/master) > error: failed to push some refs to 'XXX' > > And, I've found out that all branches and tags have been > wiped in both, local repo and remote :) > > I restored the repo and tried out > > git push origin 1.0 > git push origin --tags > > and this time both succeeded, without wiping out any refs. > > Could anyone help me to understand why remote-less > > git push --tags > > is/was so dangerous and unforgiving?! Since nobody's replied yet, I can't see what's going on here from the info you've provided. My guess is that you have something "mirror" set on the remote. It seems you can't share the repo or its URL, but could you share the scrubbed output of 'git config -l --show-origin' when run inside this repository?
Re: Git Tags
On Thu, 29 Nov 2018 at 14:40, Randall S. Becker wrote: > On November 29, 2018 6:56, Mateusz Loskot wrote: > > On Thu, 29 Nov 2018 at 12:50, Stefanie Leisestreichler > > wrote: > > > > > > git tag -a 0.9.0 > > > git push origin master > > > > > > In my local repository, when I run "git tag" it is showing me "0.9.0". > > > > > > Then I did (on box B) > > > git clone ssh://user@host:/path/project.git cd project git tag > > > > > > Now git tag is showing nothing. > > > > > > Why is the tag only available in my local repository? > > > > >From https://git-scm.com/book/en/v2/Git-Basics-Tagging > > "By default, the git push command doesn’t transfer tags to remote servers. > > You will have to explicitly push tags to a shared server after you have > > created > > them." > > git push --tags > After my yesterday experience [1], I'd be careful with git push --tags :) [1] genuine screenshot and link to related thread at https://twitter.com/mloskot/status/1068072285846859776 Best regards, -- Mateusz Loskot, http://mateusz.loskot.net
RE: Git Tags
On November 29, 2018 6:56, Mateusz Loskot wrote: > On Thu, 29 Nov 2018 at 12:50, Stefanie Leisestreichler > wrote: > > > > git tag -a 0.9.0 > > git push origin master > > > > In my local repository, when I run "git tag" it is showing me "0.9.0". > > > > Then I did (on box B) > > git clone ssh://user@host:/path/project.git cd project git tag > > > > Now git tag is showing nothing. > > > > Why is the tag only available in my local repository? > > >From https://git-scm.com/book/en/v2/Git-Basics-Tagging > "By default, the git push command doesn’t transfer tags to remote servers. > You will have to explicitly push tags to a shared server after you have > created > them." git push --tags and git fetch --tags to be symmetrical Cheers, Randall -- Brief whoami: NonStop developer since approximately 2112884442 UNIX developer since approximately 421664400 -- In my real life, I talk too much.
Re: Git Tags
On Thu, 29 Nov 2018 at 12:50, Stefanie Leisestreichler wrote: > > git tag -a 0.9.0 > git push origin master > > In my local repository, when I run "git tag" it is showing me "0.9.0". > > Then I did (on box B) > git clone ssh://user@host:/path/project.git > cd project > git tag > > Now git tag is showing nothing. > > Why is the tag only available in my local repository? >From https://git-scm.com/book/en/v2/Git-Basics-Tagging "By default, the git push command doesn’t transfer tags to remote servers. You will have to explicitly push tags to a shared server after you have created them." Best regards, -- Mateusz Loskot, http://mateusz.loskot.net
Git Tags
Hi. I have done this (on box A): git commit -m "Message" git tag -a 0.9.0 git push origin master In my local repository, when I run "git tag" it is showing me "0.9.0". Then I did (on box B) git clone ssh://user@host:/path/project.git cd project git tag Now git tag is showing nothing. Why is the tag only available in my local repository? Also when I try to git clone --branch 0.9.0 ssh://user@host:/path/project.git it tells me: fatal:remote branch not found in upstream repository origin Why is the tag not available in origin? Thanks, Stefanie
Re: Simple git push --tags deleted all branches
On Wed, 28 Nov 2018 at 17:50, Mateusz Loskot wrote: > > (using git version 2.19.2.windows.1) > [...] > I restored the repo and tried out > > git push origin 1.0 > git push origin --tags > > and this time both succeeded, without wiping out any refs. And, to my surprise, this pushed all branches and all tags: git push --all origin So, I did not have to run follow with tags push only using git push --tags origin Has anything changes in [1] that now --all Push all branches ...AND tags? [1] https://git-scm.com/docs/git-push#git-push---all Best regards, -- Mateusz Loskot, http://mateusz.loskot.net
Simple git push --tags deleted all branches
Hi, (using git version 2.19.2.windows.1) I've just encountered one of those WTH moments. I have a bare repository core.git (BARE:master) $ git branch 1.0 2.0 * master core.git (BARE:master) $ git tag 1.0.1651 1.0.766 2.0.1103 2.0.1200 I published the repo using: git push --all --follow-tags This succeeded, but there seem to be no tags pushed, just branches. So, I followed with core.git (BARE:master) $ git push --tags To XXX - [deleted] 1.0 - [deleted] 2.0 ! [remote rejected] master (refusing to delete the current branch: refs/heads/master) error: failed to push some refs to 'XXX' And, I've found out that all branches and tags have been wiped in both, local repo and remote :) I restored the repo and tried out git push origin 1.0 git push origin --tags and this time both succeeded, without wiping out any refs. Could anyone help me to understand why remote-less git push --tags is/was so dangerous and unforgiving?! Best regards, -- Mateusz Loskot, http://mateusz.loskot.net
[PATCH v3 05/11] fast-export: avoid dying when filtering by paths and old tags exist
If --tag-of-filtered-object=rewrite is specified along with a set of
paths to limit what is exported, then any tags pointing to old commits
that do not contain any of those specified paths cause problems. Since
the old tagged commit is not exported, fast-export attempts to rewrite
such tags to an ancestor commit which was exported. If no such commit
exists, then fast-export currently die()s. Five years after the tag
rewriting logic was added to fast-export (see commit 2d8ad4691921,
"fast-export: Add a --tag-of-filtered-object option for newly dangling
tags", 2009-06-25), fast-import gained the ability to delete refs (see
commit 4ee1b225b99f, "fast-import: add support to delete refs",
2014-04-20), so now we do have a valid option to rewrite the tag to.
Delete these tags instead of dying.
Signed-off-by: Elijah Newren
---
builtin/fast-export.c | 9 ++---
t/t9350-fast-export.sh | 16
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e2be35f41e..7d50f5414e 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -775,9 +775,12 @@ static void handle_tag(const char *name, struct tag *tag)
break;
if (!(p->object.flags & TREESAME))
break;
- if (!p->parents)
- die("can't find replacement commit for
tag %s",
-oid_to_hex(>object.oid));
+ if (!p->parents) {
+ printf("reset %s\nfrom %s\n\n",
+ name, oid_to_hex(_oid));
+ free(buf);
+ return;
+ }
p = p->parents->item;
}
tagged_mark = get_object_mark(>object);
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 6a392e87bc..3400ebeb51 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -325,6 +325,22 @@ test_expect_success 'rewriting tag of filtered out object'
'
)
'
+test_expect_success 'rewrite tag predating pathspecs to nothing' '
+ test_create_repo rewrite_tag_predating_pathspecs &&
+ (
+ cd rewrite_tag_predating_pathspecs &&
+
+ test_commit initial &&
+
+ git tag -a -m "Some old tag" v0.0.0.0.0.0.1 &&
+
+ test_commit bar &&
+
+ git fast-export --tag-of-filtered-object=rewrite --all -- bar.t
>output &&
+ grep from.$ZERO_OID output
+ )
+'
+
cat > limit-by-paths/expected << EOF
blob
mark :1
--
2.19.1.1063.g1796373474.dirty
Re: [PATCH v2 04/11] fast-export: avoid dying when filtering by paths and old tags exist
On Wed, Nov 14, 2018 at 11:17 AM SZEDER Gábor wrote:
> On Tue, Nov 13, 2018 at 04:25:53PM -0800, Elijah Newren wrote:
> > diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> > index af724e9937..b984a44224 100644
> > --- a/builtin/fast-export.c
> > +++ b/builtin/fast-export.c
> > @@ -774,9 +774,12 @@ static void handle_tag(const char *name, struct tag
> > *tag)
> > break;
> > if (!(p->object.flags & TREESAME))
> > break;
> > - if (!p->parents)
> > - die("can't find replacement commit
> > for tag %s",
> > - oid_to_hex(>object.oid));
> > + if (!p->parents) {
> > + printf("reset %s\nfrom %s\n\n",
> > +name, sha1_to_hex(null_sha1));
>
> Please use oid_to_hex(_oid) instead.
Will do. Looks like origin/master:builtin/fast-export.c already had
two sha1_to_hex() calls, so I'll add a cleanup patch fixing those too.
Re: [PATCH v2 04/11] fast-export: avoid dying when filtering by paths and old tags exist
On Tue, Nov 13, 2018 at 04:25:53PM -0800, Elijah Newren wrote:
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index af724e9937..b984a44224 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -774,9 +774,12 @@ static void handle_tag(const char *name, struct tag *tag)
> break;
> if (!(p->object.flags & TREESAME))
> break;
> - if (!p->parents)
> - die("can't find replacement commit for
> tag %s",
> - oid_to_hex(>object.oid));
> + if (!p->parents) {
> + printf("reset %s\nfrom %s\n\n",
> +name, sha1_to_hex(null_sha1));
Please use oid_to_hex(_oid) instead.
> + free(buf);
> + return;
> + }
> p = p->parents->item;
> }
> tagged_mark = get_object_mark(>object);
[PATCH v2 04/11] fast-export: avoid dying when filtering by paths and old tags exist
If --tag-of-filtered-object=rewrite is specified along with a set of
paths to limit what is exported, then any tags pointing to old commits
that do not contain any of those specified paths cause problems. Since
the old tagged commit is not exported, fast-export attempts to rewrite
such tags to an ancestor commit which was exported. If no such commit
exists, then fast-export currently die()s. Five years after the tag
rewriting logic was added to fast-export (see commit 2d8ad4691921,
"fast-export: Add a --tag-of-filtered-object option for newly dangling
tags", 2009-06-25), fast-import gained the ability to delete refs (see
commit 4ee1b225b99f, "fast-import: add support to delete refs",
2014-04-20), so now we do have a valid option to rewrite the tag to.
Delete these tags instead of dying.
Signed-off-by: Elijah Newren
---
builtin/fast-export.c | 9 ++---
t/t9350-fast-export.sh | 16
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index af724e9937..b984a44224 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -774,9 +774,12 @@ static void handle_tag(const char *name, struct tag *tag)
break;
if (!(p->object.flags & TREESAME))
break;
- if (!p->parents)
- die("can't find replacement commit for
tag %s",
-oid_to_hex(>object.oid));
+ if (!p->parents) {
+ printf("reset %s\nfrom %s\n\n",
+ name, sha1_to_hex(null_sha1));
+ free(buf);
+ return;
+ }
p = p->parents->item;
}
tagged_mark = get_object_mark(>object);
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 6a392e87bc..3400ebeb51 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -325,6 +325,22 @@ test_expect_success 'rewriting tag of filtered out object'
'
)
'
+test_expect_success 'rewrite tag predating pathspecs to nothing' '
+ test_create_repo rewrite_tag_predating_pathspecs &&
+ (
+ cd rewrite_tag_predating_pathspecs &&
+
+ test_commit initial &&
+
+ git tag -a -m "Some old tag" v0.0.0.0.0.0.1 &&
+
+ test_commit bar &&
+
+ git fast-export --tag-of-filtered-object=rewrite --all -- bar.t
>output &&
+ grep from.$ZERO_OID output
+ )
+'
+
cat > limit-by-paths/expected << EOF
blob
mark :1
--
2.19.1.1063.g2b8e4a4f82.dirty
Re: [PATCH 04/10] fast-export: avoid dying when filtering by paths and old tags exist
On Mon, Nov 12, 2018 at 10:50:43PM +, brian m. carlson wrote:
> On Sun, Nov 11, 2018 at 01:44:43AM -0500, Jeff King wrote:
> > > + git fast-export --tag-of-filtered-object=rewrite --all -- bar
> > > >output &&
> > > + grep -A 1 refs/tags/v0.0.0.0.0.0.1 output | grep -E ^from.0{40}
> >
> > I don't think "grep -A" is portable (and we don't seem to otherwise use
> > it). You can probably do something similar with sed.
> >
> > Use $ZERO_OID instead of hard-coding 40, which future-proofs for the
> > hash transition (though I suppose the hash is not likely to get
> > _shorter_ ;) ).
>
> It would indeed be nice if we used $ZERO_OID. Also, we prefer to write
> "egrep", since some less capable systems don't have a grep with -E.
I thought that, too, but it is only "grep -F" that has been a problem
for us in the past, and we have many "grep -E" calls already. c.f.
https://public-inbox.org/git/20180910154453.ga15...@sigill.intra.peff.net/
-Peff
Re: [PATCH 1/2] refs: show --exclude failure with --branches/tags/remotes=glob
Hi Junio, On Tue, 13 Nov 2018, Junio C Hamano wrote: > Rafael Ascensão writes: > > > The documentation of `--exclude=` option from rev-list and rev-parse > > explicitly states that exclude patterns *should not* start with 'refs/' > > when used with `--branches`, `--tags` or `--remotes`. > > > > However, following this advice results in refereces not being excluded > > if the next `--branches`, `--tags`, `--remotes` use the optional > > inclusive glob. > > > > Demonstrate this failure. > > > > Signed-off-by: Rafael Ascensão > > --- > > t/t6018-rev-list-glob.sh | 60 ++-- > > 1 file changed, 57 insertions(+), 3 deletions(-) > > For a trivially small change/fix like this (i.e. the real fix in 2/2 > is just 4 lines), it is OK and even preferrable to make 1+2 a single > step, as applying t/ part only to try to see the breakage (or > "am"ing everything and then "diff | apply -R" the part outside t/ > for the same purpose) is easy enough. I wish you were not so adamant about this. I really consider it poor style to smoosh those together, and there is nothing easy about disentangling changes that have been thrown into the same commit. Please stop saying that this is easy. It is as easy as maintaining Linux kernel development using .tar files and patches. It is possible, yes, and Linus Torvalds did it for years. It is also error-prone and the entire reason we have Git. And nobody wants to go back anymore to .tar files and patches. Likewise, I do not want to read anybody recommending some semi-understandable diff|apply-R dance when the alternative would be a simple cherry-pick. I do not even want to read such a recommendation from you. I respect you a lot for what you do, and for your knowledge, but this is simply bad advice and I would wish you stopped giving it. Besides, we spent a decade trying to come up with clear-cut rules how to organize commits, and we ended up pretty quickly with recommending logically-separate changes belonging to separate commits. A typo fix should not be thrown in with a regression fix, they are two different things. Likewise, demonstrating a bug is a different thing from fixing it. If you need more arguments to make the case, here is another one: it is reflecting the reality a lot better if the regression test comes first, and then the fix. This is how Rafael did it, too, according to what he said on IRC. And reflecting this in the commit history is a good thing, not a bad thing. It goes further: obviously, Rafael had really good success with this strategy, even figuring out part of the bug while trying to write the regression test. I, myself wrote a regression test yesterday that completely short-circuited the bug hunt: originally, I thought the left-over MERGE_HEAD files in the rebase -r stemmed from mere conflicts during a `merge` command, and somehow `git commit` not cleaning it properly. But when I wrote that regression test and ran it, it failed to show a regression. So then I took my (rather lengthy: >200 todo commands) real-world example, and condensed it into the regression test that you saw yesterday. I would estimate that this saved me about 1-3 hours of debugging in vain. So it is a very, very good idea to start with the regression test, and only then analyze the bug. Reading the commit history this way makes therefore not only sense, but also sets a good example for new contributors to follow. For these reasons, and many more, I implore you to stop suggesting to conflate the demonstration of a bug with the fix. Instead, we should be happy to see good practices in action and encourage more of the same. Thank you, Dscho > Often the patch 2 with your method ends up showing only the test > set-up part in the context by changing _failure to _success, without > showing what end-user visible breakage the step fixed, which usually > comes near the end of the added test piece. For this particular > test, s/_failure/_success/ shows everything in the verification > phase, but the entire set-up for these tests cannot be seen while > reviewing 2/2. Unlike that, a single patch that gives tests that > ought to succeed would not force the readers to switch between > patches 1 and 2 while reading the fix. > > Of course, the above would not apply for a more involved case where > the actual fix to the code needs to span multiple patches. > > > diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh > > index 0bf10d0686..8e2b136356 100755 > > --- a/t/t6018-rev-list-glob.sh > > +++ b/t/t6018-rev-list-glob.sh > > @@ -36,7 +36,13 @@ test_expect_success 'setup' ' > > git tag foo/bar master && > > commit master3 && > > git update-ref refs/remotes/foo/baz master && > > - com
Re: [PATCH 1/2] refs: show --exclude failure with --branches/tags/remotes=glob
Rafael Ascensão writes:
> The documentation of `--exclude=` option from rev-list and rev-parse
> explicitly states that exclude patterns *should not* start with 'refs/'
> when used with `--branches`, `--tags` or `--remotes`.
>
> However, following this advice results in refereces not being excluded
> if the next `--branches`, `--tags`, `--remotes` use the optional
> inclusive glob.
>
> Demonstrate this failure.
>
> Signed-off-by: Rafael Ascensão
> ---
> t/t6018-rev-list-glob.sh | 60 ++--
> 1 file changed, 57 insertions(+), 3 deletions(-)
For a trivially small change/fix like this (i.e. the real fix in 2/2
is just 4 lines), it is OK and even preferrable to make 1+2 a single
step, as applying t/ part only to try to see the breakage (or
"am"ing everything and then "diff | apply -R" the part outside t/
for the same purpose) is easy enough.
Often the patch 2 with your method ends up showing only the test
set-up part in the context by changing _failure to _success, without
showing what end-user visible breakage the step fixed, which usually
comes near the end of the added test piece. For this particular
test, s/_failure/_success/ shows everything in the verification
phase, but the entire set-up for these tests cannot be seen while
reviewing 2/2. Unlike that, a single patch that gives tests that
ought to succeed would not force the readers to switch between
patches 1 and 2 while reading the fix.
Of course, the above would not apply for a more involved case where
the actual fix to the code needs to span multiple patches.
> diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
> index 0bf10d0686..8e2b136356 100755
> --- a/t/t6018-rev-list-glob.sh
> +++ b/t/t6018-rev-list-glob.sh
> @@ -36,7 +36,13 @@ test_expect_success 'setup' '
> git tag foo/bar master &&
> commit master3 &&
> git update-ref refs/remotes/foo/baz master &&
> - commit master4
> + commit master4 &&
> + git update-ref refs/remotes/upstream/one subspace/one &&
> + git update-ref refs/remotes/upstream/two subspace/two &&
> + git update-ref refs/remotes/upstream/x subspace-x &&
> + git tag qux/one subspace/one &&
> + git tag qux/two subspace/two &&
> + git tag qux/x subspace-x
> '
Let me follow along.
We add three remote-tracking looking branches for 'upstream', and
three tags under refs/tags/qux/.
> +test_expect_failure 'rev-parse --exclude=glob with --branches=glob' '
> + compare rev-parse "--exclude=subspace-* --branches=sub*" "subspace/one
> subspace/two"
> +'
We want to list all branches that begin with "sub", but we do not
want ones that begin with "subspace-". subspace/{one,two} should
pass that criteria, while subspace-x, other/three, someref, and
master should not. Makes sense.
> +
> +test_expect_failure 'rev-parse --exclude=glob with --tags=glob' '
> + compare rev-parse "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
> +'
We want all tags that begin with "qux/" but we do not want qux/
followed by just a single letter. qux/{one,two} are in, qux/x is
out. Makes sense.
> +test_expect_failure 'rev-parse --exclude=glob with --remotes=glob' '
> + compare rev-parse "--exclude=upstream/? --remotes=upstream/*"
> "upstream/one upstream/two"
> +'
Similarly for refs/remotes/upstream/ hierarchy.
> +test_expect_failure 'rev-parse --exclude=ref with --branches=glob' '
> + compare rev-parse "--exclude=subspace-x --branches=sub*" "subspace/one
> subspace/two
> +'
This is almost a repeat of the first new one. As subspace-* in
branches only match subspace-x, this should give the same result as
that one.
> +test_expect_failure 'rev-parse --exclude=ref with --tags=glob' '
> + compare rev-parse "--exclude=qux/x --tags=qux/*" "qux/one qux/two"
> +'
Likewise.
> +test_expect_failure 'rev-parse --exclude=ref with --remotes=glob' '
> + compare rev-parse "--exclude=upstream/x --remotes=upstream/*"
> "upstream/one upstream/two"
> +'
Likewise.
> +test_expect_failure 'rev-list --exclude=glob with --branches=glob' '
> + compare rev-list "--exclude=subspace-* --branches=sub*" "subspace/one
> subspace/two"
> +'
And then the same pattern continues with rev-list.
> +test_expect_failure 'rev-list --exclude=glob with --tags=glob' '
> + compare rev-list "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
> +'
> +
> +test_expect_failure 'rev-list --exclude=glob with --remotes=glob' '
> + compare rev-list "--exclude=upstream/? --remotes=upstream
Re: [PATCH 04/10] fast-export: avoid dying when filtering by paths and old tags exist
On Sun, Nov 11, 2018 at 01:44:43AM -0500, Jeff King wrote:
> > + git fast-export --tag-of-filtered-object=rewrite --all -- bar
> > >output &&
> > + grep -A 1 refs/tags/v0.0.0.0.0.0.1 output | grep -E ^from.0{40}
>
> I don't think "grep -A" is portable (and we don't seem to otherwise use
> it). You can probably do something similar with sed.
>
> Use $ZERO_OID instead of hard-coding 40, which future-proofs for the
> hash transition (though I suppose the hash is not likely to get
> _shorter_ ;) ).
It would indeed be nice if we used $ZERO_OID. Also, we prefer to write
"egrep", since some less capable systems don't have a grep with -E.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
[PATCH 1/2] refs: show --exclude failure with --branches/tags/remotes=glob
The documentation of `--exclude=` option from rev-list and rev-parse explicitly states that exclude patterns *should not* start with 'refs/' when used with `--branches`, `--tags` or `--remotes`. However, following this advice results in refereces not being excluded if the next `--branches`, `--tags`, `--remotes` use the optional inclusive glob. Demonstrate this failure. Signed-off-by: Rafael Ascensão --- t/t6018-rev-list-glob.sh | 60 ++-- 1 file changed, 57 insertions(+), 3 deletions(-) diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh index 0bf10d0686..8e2b136356 100755 --- a/t/t6018-rev-list-glob.sh +++ b/t/t6018-rev-list-glob.sh @@ -36,7 +36,13 @@ test_expect_success 'setup' ' git tag foo/bar master && commit master3 && git update-ref refs/remotes/foo/baz master && - commit master4 + commit master4 && + git update-ref refs/remotes/upstream/one subspace/one && + git update-ref refs/remotes/upstream/two subspace/two && + git update-ref refs/remotes/upstream/x subspace-x && + git tag qux/one subspace/one && + git tag qux/two subspace/two && + git tag qux/x subspace-x ' test_expect_success 'rev-parse --glob=refs/heads/subspace/*' ' @@ -141,6 +147,54 @@ test_expect_success 'rev-parse accumulates multiple --exclude' ' compare rev-parse "--exclude=refs/remotes/* --exclude=refs/tags/* --all" --branches ' +test_expect_failure 'rev-parse --exclude=glob with --branches=glob' ' + compare rev-parse "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two" +' + +test_expect_failure 'rev-parse --exclude=glob with --tags=glob' ' + compare rev-parse "--exclude=qux/? --tags=qux/*" "qux/one qux/two" +' + +test_expect_failure 'rev-parse --exclude=glob with --remotes=glob' ' + compare rev-parse "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two" +' + +test_expect_failure 'rev-parse --exclude=ref with --branches=glob' ' + compare rev-parse "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two" +' + +test_expect_failure 'rev-parse --exclude=ref with --tags=glob' ' + compare rev-parse "--exclude=qux/x --tags=qux/*" "qux/one qux/two" +' + +test_expect_failure 'rev-parse --exclude=ref with --remotes=glob' ' + compare rev-parse "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two" +' + +test_expect_failure 'rev-list --exclude=glob with --branches=glob' ' + compare rev-list "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two" +' + +test_expect_failure 'rev-list --exclude=glob with --tags=glob' ' + compare rev-list "--exclude=qux/? --tags=qux/*" "qux/one qux/two" +' + +test_expect_failure 'rev-list --exclude=glob with --remotes=glob' ' + compare rev-list "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two" +' + +test_expect_failure 'rev-list --exclude=ref with --branches=glob' ' + compare rev-list "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two" +' + +test_expect_failure 'rev-list --exclude=ref with --tags=glob' ' + compare rev-list "--exclude=qux/x --tags=qux/*" "qux/one qux/two" +' + +test_expect_failure 'rev-list --exclude=ref with --remotes=glob' ' + compare rev-list "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two" +' + test_expect_success 'rev-list --glob=refs/heads/subspace/*' ' compare rev-list "subspace/one subspace/two" "--glob=refs/heads/subspace/*" @@ -233,7 +287,7 @@ test_expect_success 'rev-list --tags=foo' ' test_expect_success 'rev-list --tags' ' - compare rev-list "foo/bar" "--tags" + compare rev-list "foo/bar qux/x qux/two qux/one" "--tags" ' @@ -292,7 +346,7 @@ test_expect_success 'shortlog accepts --glob/--tags/--remotes' ' "master other/three someref subspace-x subspace/one subspace/two" \ "--glob=heads/*" && compare shortlog foo/bar --tags=foo && - compare shortlog foo/bar --tags && + compare shortlog "foo/bar qux/one qux/two qux/x" --tags && compare shortlog foo/baz --remotes=foo ' -- 2.19.1
Re: [PATCH 04/10] fast-export: avoid dying when filtering by paths and old tags exist
On Sat, Nov 10, 2018 at 11:38:45PM -0800, Elijah Newren wrote: > > Hmm. That's the right thing to do if we're considering the export to be > > an independent unit. But what if I'm just rewriting a portion of history > > like: > > > > git fast-export HEAD~5..HEAD | some_filter | git fast-import > > > > ? If I have a tag pointing to HEAD~10, will this delete that? Ideally I > > think it would be left alone. > > A couple things: > * This code path only triggers in a very specific case: If a tag is > requested for export but points to a commit which is filtered out by > something else (e.g. path limiters and the commit in question didn't > modify any of the relevant paths), AND the user explicitly specified > --tag-of-filtered-object=rewrite (so that the tag in question can be > rewritten to the nearest non-filtered ancestor). Right, I think this is the bit I was missing: somebody has to have explicitly asked to export the tag. At which point the only sensible thing to do is drop it. -Peff
Re: [PATCH 04/10] fast-export: avoid dying when filtering by paths and old tags exist
On Sat, Nov 10, 2018 at 10:44 PM Jeff King wrote:
>
> On Sat, Nov 10, 2018 at 10:23:06PM -0800, Elijah Newren wrote:
>
> > If --tag-of-filtered-object=rewrite is specified along with a set of
> > paths to limit what is exported, then any tags pointing to old commits
> > that do not contain any of those specified paths cause problems. Since
> > the old tagged commit is not exported, fast-export attempts to rewrite
> > such tags to an ancestor commit which was exported. If no such commit
> > exists, then fast-export currently die()s. Five years after the tag
> > rewriting logic was added to fast-export (see commit 2d8ad4691921,
> > "fast-export: Add a --tag-of-filtered-object option for newly dangling
> > tags", 2009-06-25), fast-import gained the ability to delete refs (see
> > commit 4ee1b225b99f, "fast-import: add support to delete refs",
> > 2014-04-20), so now we do have a valid option to rewrite the tag to.
> > Delete these tags instead of dying.
>
> Hmm. That's the right thing to do if we're considering the export to be
> an independent unit. But what if I'm just rewriting a portion of history
> like:
>
> git fast-export HEAD~5..HEAD | some_filter | git fast-import
>
> ? If I have a tag pointing to HEAD~10, will this delete that? Ideally I
> think it would be left alone.
A couple things:
* This code path only triggers in a very specific case: If a tag is
requested for export but points to a commit which is filtered out by
something else (e.g. path limiters and the commit in question didn't
modify any of the relevant paths), AND the user explicitly specified
--tag-of-filtered-object=rewrite (so that the tag in question can be
rewritten to the nearest non-filtered ancestor).
* You didn't specify to export any tags, only HEAD, so this
situation isn't relevant (the tag wouldn't be exported or deleted).
* You didn't specify --tag-of-filtered-object=rewrite, so this
situation isn't relevant (even if you had specified a tag to filter,
you'd get an abort instead)
But let's say you do modify the example some:
git fast-export --tag-of-filtered-object=rewrite
--signed-tags=strip --tags master -- relatively_recent_subdirectory/ |
some_filter | git fast-import
The user asked that all tags and master be exported but only for the
history that touched relatively_recent_subdirectory/, and if any tags
point at commits that are pruned by only asking for commits touching
relatively_recent_subdirectory/, then rewrite what those tags point to
so that they instead point to the nearest non-filtered ancestor. What
about a commit like v0.1.0 that likely pre-dated the introduction of
relatively_recent_subdirectory/? It has no nearest ancestor to
rewrite to. The previous answer was to abort, which is really bad,
especially since the user was clearly asking us to do whatever smart
rewriting we can (--signed-tags=strip and
--tag-of-filtered-object=rewrite).
Perhaps there's a different answer that's workable as well, but this
one, in these circumstances, seemed the most reasonable to me.
> > +test_expect_success 'rewrite tag predating pathspecs to nothing' '
> > + test_create_repo rewrite_tag_predating_pathspecs &&
> > + (
> > + cd rewrite_tag_predating_pathspecs &&
> > +
> > + touch ignored &&
>
> We usually prefer ">ignored" to create an empty file rather than
> "touch".
Will fix.
>
> > + git add ignored &&
> > + test_commit initial &&
>
> What do we need this "ignored" for? test_commit should create a file
> "initial.t".
I think I original had plain "git commit", then switched to
test_commit, then didn't recheck. Thanks, will fix.
> > + echo foo >bar &&
> > + git add bar &&
> > + test_commit add-bar &&
>
> Likewise, "test_commit bar" should work by itself (though note the
> filename is "bar.t" in your fast-export command).
>
> > + git fast-export --tag-of-filtered-object=rewrite --all -- bar
> > >output &&
> > + grep -A 1 refs/tags/v0.0.0.0.0.0.1 output | grep -E
> > ^from.0{40}
>
> I don't think "grep -A" is portable (and we don't seem to otherwise use
> it). You can probably do something similar with sed.
>
> Use $ZERO_OID instead of hard-coding 40, which future-proofs for the
> hash transition (though I suppose the hash is not likely to get
> _shorter_ ;) ).
Will fix these up as well...after waiting for more feedback on
possible alternate suggestions.
Re: [PATCH 04/10] fast-export: avoid dying when filtering by paths and old tags exist
On Sat, Nov 10, 2018 at 10:23:06PM -0800, Elijah Newren wrote:
> If --tag-of-filtered-object=rewrite is specified along with a set of
> paths to limit what is exported, then any tags pointing to old commits
> that do not contain any of those specified paths cause problems. Since
> the old tagged commit is not exported, fast-export attempts to rewrite
> such tags to an ancestor commit which was exported. If no such commit
> exists, then fast-export currently die()s. Five years after the tag
> rewriting logic was added to fast-export (see commit 2d8ad4691921,
> "fast-export: Add a --tag-of-filtered-object option for newly dangling
> tags", 2009-06-25), fast-import gained the ability to delete refs (see
> commit 4ee1b225b99f, "fast-import: add support to delete refs",
> 2014-04-20), so now we do have a valid option to rewrite the tag to.
> Delete these tags instead of dying.
Hmm. That's the right thing to do if we're considering the export to be
an independent unit. But what if I'm just rewriting a portion of history
like:
git fast-export HEAD~5..HEAD | some_filter | git fast-import
? If I have a tag pointing to HEAD~10, will this delete that? Ideally I
think it would be left alone.
> +test_expect_success 'rewrite tag predating pathspecs to nothing' '
> + test_create_repo rewrite_tag_predating_pathspecs &&
> + (
> + cd rewrite_tag_predating_pathspecs &&
> +
> + touch ignored &&
We usually prefer ">ignored" to create an empty file rather than
"touch".
> + git add ignored &&
> + test_commit initial &&
What do we need this "ignored" for? test_commit should create a file
"initial.t".
> + echo foo >bar &&
> + git add bar &&
> + test_commit add-bar &&
Likewise, "test_commit bar" should work by itself (though note the
filename is "bar.t" in your fast-export command).
> + git fast-export --tag-of-filtered-object=rewrite --all -- bar
> >output &&
> + grep -A 1 refs/tags/v0.0.0.0.0.0.1 output | grep -E ^from.0{40}
I don't think "grep -A" is portable (and we don't seem to otherwise use
it). You can probably do something similar with sed.
Use $ZERO_OID instead of hard-coding 40, which future-proofs for the
hash transition (though I suppose the hash is not likely to get
_shorter_ ;) ).
-Peff
[PATCH 04/10] fast-export: avoid dying when filtering by paths and old tags exist
If --tag-of-filtered-object=rewrite is specified along with a set of
paths to limit what is exported, then any tags pointing to old commits
that do not contain any of those specified paths cause problems. Since
the old tagged commit is not exported, fast-export attempts to rewrite
such tags to an ancestor commit which was exported. If no such commit
exists, then fast-export currently die()s. Five years after the tag
rewriting logic was added to fast-export (see commit 2d8ad4691921,
"fast-export: Add a --tag-of-filtered-object option for newly dangling
tags", 2009-06-25), fast-import gained the ability to delete refs (see
commit 4ee1b225b99f, "fast-import: add support to delete refs",
2014-04-20), so now we do have a valid option to rewrite the tag to.
Delete these tags instead of dying.
Signed-off-by: Elijah Newren
---
builtin/fast-export.c | 9 ++---
t/t9350-fast-export.sh | 20
2 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 1a299c2a21..89de9d6400 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -774,9 +774,12 @@ static void handle_tag(const char *name, struct tag *tag)
break;
if (!(p->object.flags & TREESAME))
break;
- if (!p->parents)
- die("can't find replacement commit for
tag %s",
-oid_to_hex(>object.oid));
+ if (!p->parents) {
+ printf("reset %s\nfrom %s\n\n",
+ name, sha1_to_hex(null_sha1));
+ free(buf);
+ return;
+ }
p = p->parents->item;
}
tagged_mark = get_object_mark(>object);
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 6a392e87bc..5bf21b4908 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -325,6 +325,26 @@ test_expect_success 'rewriting tag of filtered out object'
'
)
'
+test_expect_success 'rewrite tag predating pathspecs to nothing' '
+ test_create_repo rewrite_tag_predating_pathspecs &&
+ (
+ cd rewrite_tag_predating_pathspecs &&
+
+ touch ignored &&
+ git add ignored &&
+ test_commit initial &&
+
+ git tag -a -m "Some old tag" v0.0.0.0.0.0.1 &&
+
+ echo foo >bar &&
+ git add bar &&
+ test_commit add-bar &&
+
+ git fast-export --tag-of-filtered-object=rewrite --all -- bar
>output &&
+ grep -A 1 refs/tags/v0.0.0.0.0.0.1 output | grep -E ^from.0{40}
+ )
+'
+
cat > limit-by-paths/expected << EOF
blob
mark :1
--
2.19.1.866.g82735bcbde
[PATCH 2/2] ls-remote: pass heads/tags prefixes to transport
Unlike its arbitrary text patterns, the --heads and --tags
options to ls-remote are true prefixes. We can pass this
information to the transport code. If the v2 protocol is in
use, that will reduce the size of the ref advertisement.
Note that the test added here succeeds both before and after
the patch. This is an optimization, not a bug-fix; it's just
making sure we didn't break anything.
Signed-off-by: Jeff King
---
builtin/ls-remote.c | 5 +
t/t5512-ls-remote.sh | 9 +
2 files changed, 14 insertions(+)
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 5faa8459d9..1d7f1f5ce2 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -92,6 +92,11 @@ int cmd_ls_remote(int argc, const char **argv, const char
*prefix)
}
}
+ if (flags & REF_TAGS)
+ argv_array_push(_prefixes, "refs/tags/");
+ if (flags & REF_HEADS)
+ argv_array_push(_prefixes, "refs/heads/");
+
remote = remote_get(dest);
if (!remote) {
if (dest)
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index ca1b7e5bc1..91ee6841c1 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -311,4 +311,13 @@ test_expect_success 'ls-remote patterns work with all
protocol versions' '
test_cmp expect actual.v2
'
+test_expect_success 'ls-remote prefixes work with all protocol versions' '
+ git for-each-ref --format="%(objectname)%(refname)" \
+ refs/heads/ refs/tags/ >expect &&
+ git -c protocol.version=1 ls-remote --heads --tags . >actual.v1 &&
+ test_cmp expect actual.v1 &&
+ git -c protocol.version=2 ls-remote --heads --tags . >actual.v2 &&
+ test_cmp expect actual.v2
+'
+
test_done
--
2.19.1.1298.g19f18f2a22
[PATCH 1/1] fast-import: checkpoint: only write out refs and tags if we changed them
At a checkpoint, fast-import resets all branches and tags on disk to the
OID that we have in memory. If --force is not given, only branch resets
that result in fast forwards with respect to the state on disk are
allowed.
With this approach, even for refs that fast-import has not been
commanded to change for a long time (or ever), at each checkpoint, we
will systematically reset them to the last state this process knows
about, a state which may have been set before the previous checkpoint.
Any changes made to these refs by a different process since the last
checkpoint will be overwritten.
1> is one process, 2> is another:
1> $ git fast-import --force
1> reset refs/heads/master
1> from $A
1> checkpoint
2> $ git branch -f refs/heads/master $B
1> reset refs/heads/tip
1> from $C
1> checkpoint
At this point master points again to $A.
This problem is mitigated somewhat for branches when --force is not
specified (the default), as requiring a fast forward means in practice
that concurrent external changes are likely to be preserved. But it's
not foolproof either:
1> $ git fast-import
1> reset refs/heads/master
1> from $A
1> checkpoint
2> $ git branch -f refs/heads/master refs/heads/master~1
1> reset refs/heads/tip
1> from $C
1> checkpoint
At this point, master points again to $A, not $A~1 as requested by the
second process.
We now keep track of whether branches and tags have been changed by this
fast-import process since our last checkpoint (or startup). At the next
checkpoint, only refs and tags that new commands have changed are
written to disk.
---
fast-import.c | 14 +++
t/t9300-fast-import.sh | 55 ++
2 files changed, 69 insertions(+)
Changed since v1:
The testcases failed to enforce the new behavior.
- t9300: Fixed tests to properly interleave operations between the
fast-import in the background and the concurrent "git branch" and "git
tag" command.
I misremembered how background_import_then_checkpoint() works: it
return only after the whole set of commands provided in input_file
(and now input_file2) are run by fast-import.
In v2, the competing git commands are started first in the background
and are run after a delay of 1 second. input_file is sent right away
to fast-import, followed 2 seconds later by input_file2. That is:
second 0: input_file
second 1: git branch or git tag command
second 2: input_file2
- t9300: Use --force for the branch testcase: without it, earlier
versions of Git don't fail this testcase (as resetting V3 to U is not
a FF, so the second checkpoint skips updating it in earlier versions
of Git, failing to demonstrate the problematic behavior).
Checked that the new testcases do fail with earlier versions of Git.
TODO: The testcases still rely on sleep(1).
diff --git a/fast-import.c b/fast-import.c
index 95600c78e048..d25be5c83110 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -250,6 +250,7 @@ struct branch {
uintmax_t num_notes;
unsigned active : 1;
unsigned delete : 1;
+ unsigned changed : 1;
unsigned pack_id : PACK_ID_BITS;
struct object_id oid;
};
@@ -257,6 +258,7 @@ struct branch {
struct tag {
struct tag *next_tag;
const char *name;
+ unsigned changed : 1;
unsigned int pack_id;
struct object_id oid;
};
@@ -728,6 +730,7 @@ static struct branch *new_branch(const char *name)
b->branch_tree.versions[1].mode = S_IFDIR;
b->num_notes = 0;
b->active = 0;
+ b->changed = 0;
b->pack_id = MAX_PACK_ID;
branch_table[hc] = b;
branch_count++;
@@ -1715,6 +1718,10 @@ static int update_branch(struct branch *b)
struct object_id old_oid;
struct strbuf err = STRBUF_INIT;
+ if (!b->changed)
+ return 0;
+ b->changed = 0;
+
if (is_null_oid(>oid)) {
if (b->delete)
delete_ref(NULL, b->name, NULL, 0);
@@ -1780,6 +1787,10 @@ static void dump_tags(void)
goto cleanup;
}
for (t = first_tag; t; t = t->next_tag) {
+ if (!t->changed)
+ continue;
+ t->changed = 0;
+
strbuf_reset(_name);
strbuf_addf(_name, "refs/tags/%s", t->name);
@@ -2813,6 +2824,7 @@ static void parse_new_commit(const char *arg)
if (!store_object(OBJ_COMMIT, _data, NULL, >oid, next_mark))
b->pack_id = pack_id;
+ b->changed = 1;
b->last_commit = object_count_by_type[OBJ_COMMIT];
}
@@ -2894,6 +2906,7 @@ static void parse_new_tag(const char *arg)
t->pack_id = MAX_PACK_ID
[PATCH 1/1] fast-import: checkpoint: only write out refs and tags if we changed them
At a checkpoint, fast-import resets all branches and tags on disk to the
OID that we have in memory. If --force is not given, only branch resets
that result in fast forwards with respect to the state on disk are
allowed.
With this approach, even for refs that fast-import has not been
commanded to change for a long time (or ever), at each checkpoint, we
will systematically reset them to the last state this process knows
about, a state which may have been set before the previous checkpoint.
Any changes made to these refs by a different process since the last
checkpoint will be overwritten.
1> is one process, 2> is another:
1> $ git fast-import --force
1> reset refs/heads/master
1> from $A
1> checkpoint
2> $ git branch -f refs/heads/master $B
1> reset refs/heads/tip
1> from $C
1> checkpoint
At this point master points again to $A.
This problem is mitigated somewhat for branches when --force is not
specified (the default), as requiring a fast forward means in practice
that concurrent external changes are likely to be preserved. But it's
not foolproof either:
1> $ git fast-import
1> reset refs/heads/master
1> from $A
1> checkpoint
2> $ git branch -f refs/heads/master refs/heads/master~1
1> reset refs/heads/tip
1> from $C
1> checkpoint
At this point, master points again to $A, not $A~1 as requested by the
second process.
We now keep track of whether branches and tags have been changed by this
fast-import process since our last checkpoint (or startup). At the next
checkpoint, only refs and tags that new commands have changed are
written to disk.
---
fast-import.c | 14 +++
t/t9300-fast-import.sh | 53 ++
2 files changed, 67 insertions(+)
Please let me know what you think of the change itself, before I spend
some quality time bashing out a way to avoid using sleep(1) in the
tests.
Thanks.
diff --git a/fast-import.c b/fast-import.c
index 95600c78e048..d25be5c83110 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -250,6 +250,7 @@ struct branch {
uintmax_t num_notes;
unsigned active : 1;
unsigned delete : 1;
+ unsigned changed : 1;
unsigned pack_id : PACK_ID_BITS;
struct object_id oid;
};
@@ -257,6 +258,7 @@ struct branch {
struct tag {
struct tag *next_tag;
const char *name;
+ unsigned changed : 1;
unsigned int pack_id;
struct object_id oid;
};
@@ -728,6 +730,7 @@ static struct branch *new_branch(const char *name)
b->branch_tree.versions[1].mode = S_IFDIR;
b->num_notes = 0;
b->active = 0;
+ b->changed = 0;
b->pack_id = MAX_PACK_ID;
branch_table[hc] = b;
branch_count++;
@@ -1715,6 +1718,10 @@ static int update_branch(struct branch *b)
struct object_id old_oid;
struct strbuf err = STRBUF_INIT;
+ if (!b->changed)
+ return 0;
+ b->changed = 0;
+
if (is_null_oid(>oid)) {
if (b->delete)
delete_ref(NULL, b->name, NULL, 0);
@@ -1780,6 +1787,10 @@ static void dump_tags(void)
goto cleanup;
}
for (t = first_tag; t; t = t->next_tag) {
+ if (!t->changed)
+ continue;
+ t->changed = 0;
+
strbuf_reset(_name);
strbuf_addf(_name, "refs/tags/%s", t->name);
@@ -2813,6 +2824,7 @@ static void parse_new_commit(const char *arg)
if (!store_object(OBJ_COMMIT, _data, NULL, >oid, next_mark))
b->pack_id = pack_id;
+ b->changed = 1;
b->last_commit = object_count_by_type[OBJ_COMMIT];
}
@@ -2894,6 +2906,7 @@ static void parse_new_tag(const char *arg)
t->pack_id = MAX_PACK_ID;
else
t->pack_id = pack_id;
+ t->changed = 1;
}
static void parse_reset_branch(const char *arg)
@@ -2914,6 +2927,7 @@ static void parse_reset_branch(const char *arg)
b = new_branch(arg);
read_next_command();
parse_from(b);
+ b->changed = 1;
if (command_buf.len > 0)
unread_command_buf = 1;
}
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 40fe7e49767a..548994dfbeb3 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3123,6 +3123,9 @@ test_expect_success 'U: validate root delete result' '
# The commands in input_file should not produce any output on the file
# descriptor set with --cat-blob-fd (or stdout if unspecified).
+#
+# If input_file2 is specified, sleep 2 seconds before writing it to fast-import
+# stdin.
#
# To make sure you're observing the side effects of checkpoint *before*
# fast-import
Re: Git wire protocol v2 fails fetching shallow changes with `pack has XXX unresolved deltas` on large repos with lots of tags Inbox x
Hello, Unfortunately I do not have for such step. On one of our internal repositories it is failing but unreliably when fetching from remote. -- Arturas Moskvinas On Wed, Oct 17, 2018 at 11:53 PM Jonathan Tan wrote: > > > No changes are needed in mirrored repository. Crash happens both with > > 2.18.0 and 2.19.1 git versions. Having repository locally is not > > required but reduces test runtime, you can quite reliably reproduce > > issue when fetching over net directly from chromium.orgbypassing > > mirroring step. > > Do you have the reproduction steps for this? If I run > > git -c protocol.version=2 fetch --tags \ > https://chromium.googlesource.com/chromium/src \ > +refs/heads/*:refs/remotes/origin/* --depth=1 > > repeatedly in the same repository, it succeeds. (And I've checked with > GIT_TRACE_PACKET that it uses protocol v2.)
Re: Git wire protocol v2 fails fetching shallow changes with `pack has XXX unresolved deltas` on large repos with lots of tags Inbox x
> No changes are needed in mirrored repository. Crash happens both with > 2.18.0 and 2.19.1 git versions. Having repository locally is not > required but reduces test runtime, you can quite reliably reproduce > issue when fetching over net directly from chromium.orgbypassing > mirroring step. Do you have the reproduction steps for this? If I run git -c protocol.version=2 fetch --tags \ https://chromium.googlesource.com/chromium/src \ +refs/heads/*:refs/remotes/origin/* --depth=1 repeatedly in the same repository, it succeeds. (And I've checked with GIT_TRACE_PACKET that it uses protocol v2.)
Re: Git wire protocol v2 fails fetching shallow changes with `pack has XXX unresolved deltas` on large repos with lots of tags Inbox x
> On large repositories with lots of tags - git wire protocol v2 fails > to fetch shallow changes, it fails with error `pack has XXX unresolved > deltas`. Unfortunately I didn't find easy way to reproduce it except > cloning+fetching chromium repository, the way jenkins does. > Reproduction steps: [snip] Thanks for your bug report and reproduction steps. I managed to reproduce your issue and took a look. The main issue seems to be that with v2, upload-pack doesn't pass "--shallow X" to pack-objects (the write_one_shallow() callback is never called, even if I change the "if (shallow_nr)" to "if (1)"), so pack-objects probably doesn't know that some objects cannot be used as delta bases. (With v0, write_one_shallow() is indeed called.) The issue probably lies in how v0 and v2 handle client-provided shallows differently. There also seems to be another issue in that negotiation occurs differently in these 2 protocols - I see the full list of "have" lines being sent in the final request to the server in v0, but a very limited list in v2. This might be because of the ref prefix limiting in v2, but I haven't fully investigated it. I'll look some more into this.
Git wire protocol v2 fails fetching shallow changes with `pack has XXX unresolved deltas` on large repos with lots of tags Inbox x
Hello, On large repositories with lots of tags - git wire protocol v2 fails to fetch shallow changes, it fails with error `pack has XXX unresolved deltas`. Unfortunately I didn't find easy way to reproduce it except cloning+fetching chromium repository, the way jenkins does. Reproduction steps: ``` $ git clone --mirror https://chromium.googlesource.com/chromium/src chromium Cloning into bare repository 'chromium'... remote: Sending approximately 12.66 GiB ... remote: Counting objects: 8523271, done remote: Finding sources: 100% (8399287/8399287) remote: Total 19829104 (delta 11897257), reused 19828780 (delta 11897257) Receiving objects: 100% (19829104/19829104), 20.45 GiB | 29.17 MiB/s, done. Resolving deltas: 100% (11897257/11897257), done $ mkdir chromium-test $ cd chromium-test $ git init Initialized empty Git repository in /home/arturas/.git/ $ git -c protocol.version=2 fetch --tags file://$(pwd)/../chromium/ +refs/heads/*:refs/remotes/origin/* --depth=1 remote: Enumerating objects: 4614683, done. remote: Counting objects: 100% (4614683/4614683), done. remote: Compressing objects: 100% (1556691/1556691), done. Receiving objects: 100% (4614683/4614683), 8.29 GiB | 24.71 MiB/s, done. remote: Total 4614683 (delta 3470249), reused 3989191 (delta 2923800) Resolving deltas: 100% (3470249/3470249), done. >From file:///home/arturas/chromium-test/../chromium * [new branch]ignore/bar -> origin/ignore/bar * [new branch]ignore/foo -> origin/ignore/foo * [new branch]infra/config-> origin/infra/config * [new branch]lkgr-> origin/lkgr * [new branch]master -> origin/master * [new tag] 10.0.601.0 -> 10.0.601.0 ... $ git -c protocol.version=2 fetch --tags file://$(pwd)/../chromium/ +refs/heads/*:refs/remotes/origin/* --depth=1 remote: Enumerating objects: 642969, done. remote: Counting objects: 100% (588314/588314), done. remote: Compressing objects: 100% (240564/240564), done. Receiving objects: 100% (571353/571353), 967.83 MiB | 10.52 MiB/s, done. remote: Total 571353 (delta 372206), reused 497895 (delta 310113) Resolving deltas: 99% (371726/372206), completed with 5582 local objects. fatal: pack has 480 unresolved deltas fatal: index-pack failed $ git --version git version 2.19.1 ``` No changes are needed in mirrored repository. Crash happens both with 2.18.0 and 2.19.1 git versions. Having repository locally is not required but reduces test runtime, you can quite reliably reproduce issue when fetching over net directly from chromium.orgbypassing mirroring step. -- Arturas Moskvinas
Re: [PATCH v4 1/1] commit-reach: properly peel tags and clear flags
On Mon, Sep 24, 2018 at 4:58 PM Derrick Stolee via GitGitGadget
wrote:
> diff --git a/commit-reach.c b/commit-reach.c
> @@ -544,20 +544,42 @@ int can_all_from_reach_with_flag(struct object_array
> *from,
> {
> + from_one = deref_tag(the_repository, from_one,
> +"a from object", 0);
> + if (!from_one || from_one->type != OBJ_COMMIT) {
> + /* no way to tell if this is reachable by
> +* looking at the ancestry chain alone, so
> +* leave a note to ourselves not to worry about
> +* this object anymore.
> +*/
Style nit:
/*
* Multi-line comment
* formatting.
*/
Re: [PATCH v4 1/1] commit-reach: properly peel tags and clear flags
On Mon, Sep 24, 2018 at 01:57:52PM -0700, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee > > The can_all_from_reach_with_flag() algorithm was refactored in 4fbcca4e > "commit-reach: make can_all_from_reach... linear" but incorrectly > assumed that all objects provided were commits. During a fetch > negotiation, ok_to_give_up() in upload-pack.c may provide unpeeled tags > to the 'from' array. The current code creates a segfault. > [...] Thanks, this version looks good to me. -Peff
[PATCH v4 0/1] Properly peel tags in can_all_from_reach_with_flags()
As Peff reported [1], the refactored can_all_from_reach_with_flags() method
does not properly peel tags. Since the helper method can_all_from_reach()
and code in t/helper/test-reach.c all peel tags before getting to this
method, it is not super-simple to create a test that demonstrates this.
I modified t/helper/test-reach.c to allow calling
can_all_from_reach_with_flags() directly, and added a test in
t6600-test-reach.sh that demonstrates the segfault without the fix.
For V2, I compared the loop that inspects the 'from' commits in commit
ba3ca1edce "commit-reach: move can_all_from_reach_with_flags" to the version
here and got the following diff:
3c3
< if (from_one->flags & assign_flag)
---
> if (!from_one || from_one->flags & assign_flag)
5c5,7
< from_one = deref_tag(the_repository, from_one, "a from
object", 0);
---
>
> from_one = deref_tag(the_repository, from_one,
> "a from object", 0);
14a17,22
>
> list[nr_commits] = (struct commit *)from_one;
> if (parse_commit(list[nr_commits]) ||
> list[nr_commits]->generation < min_generation)
> return 0; /* is this a leak? */
> nr_commits++;
This diff includes the early termination we had before 'deref_tag' and the
comment for why we can ignore non-commit objects.
[1]
https://public-inbox.org/git/0bf9103c-9377-506b-7ad7-e5273d8e9...@gmail.com/T/#u
Derrick Stolee (1):
commit-reach: properly peel tags and clear flags
commit-reach.c| 44 +--
t/helper/test-reach.c | 22 +-
t/t6600-test-reach.sh | 30 +++--
3 files changed, 79 insertions(+), 17 deletions(-)
base-commit: 6621c838743812aaba96e55cfec8524ea1144c2d
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-39%2Fderrickstolee%2Ftag-fix-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-39/derrickstolee/tag-fix-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/39
Range-diff vs v3:
1: 0a1e661271 ! 1: a0a3cf0134 commit-reach: properly peel tags
@@ -1,6 +1,6 @@
Author: Derrick Stolee
-commit-reach: properly peel tags
+commit-reach: properly peel tags and clear flags
The can_all_from_reach_with_flag() algorithm was refactored in
4fbcca4e
"commit-reach: make can_all_from_reach... linear" but incorrectly
@@ -14,6 +14,19 @@
Correct the issue by peeling tags when investigating the initial list
of objects in the 'from' array.
+The can_all_from_reach_with_flag() method uses 'assign_flag' as a
+value we can use to mark objects temporarily during our commit walk.
+The intent is that these flags are removed from all objects before
+returning. However, this is not the case.
+
+The 'from' array could also contain objects that are not commits, and
+we mark those objects with 'assign_flag'. Add a loop to the 'cleanup'
+section that removes these markers.
+
+Also, we forgot to free() the memory for 'list', so add that to the
+'cleanup' section. Also, use a cleaner mechanism for clearing those
+flags.
+
Signed-off-by: Jeff King
Signed-off-by: Derrick Stolee
@@ -74,10 +87,18 @@
cleanup:
- for (i = 0; i < from->nr; i++) {
-+ for (i = 0; i < nr_commits; i++) {
- clear_commit_marks(list[i], RESULT);
- clear_commit_marks(list[i], assign_flag);
- }
+- clear_commit_marks(list[i], RESULT);
+- clear_commit_marks(list[i], assign_flag);
+- }
++ clear_commit_marks_many(nr_commits, list, RESULT | assign_flag);
++ free(list);
++
++ for (i = 0; i < from->nr; i++)
++ from->objects[i].item->flags &= ~assign_flag;
++
+ return result;
+ }
+
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
--- a/t/helper/test-reach.c
2: b2e0ee4978 < -: -- commit-reach: fix memory and flag leaks
--
gitgitgadget
[PATCH v4 1/1] commit-reach: properly peel tags and clear flags
From: Derrick Stolee
The can_all_from_reach_with_flag() algorithm was refactored in 4fbcca4e
"commit-reach: make can_all_from_reach... linear" but incorrectly
assumed that all objects provided were commits. During a fetch
negotiation, ok_to_give_up() in upload-pack.c may provide unpeeled tags
to the 'from' array. The current code creates a segfault.
Add a direct call to can_all_from_reach_with_flag() in 'test-tool reach'
and add a test in t6600-test-reach.sh that demonstrates this segfault.
Correct the issue by peeling tags when investigating the initial list
of objects in the 'from' array.
The can_all_from_reach_with_flag() method uses 'assign_flag' as a
value we can use to mark objects temporarily during our commit walk.
The intent is that these flags are removed from all objects before
returning. However, this is not the case.
The 'from' array could also contain objects that are not commits, and
we mark those objects with 'assign_flag'. Add a loop to the 'cleanup'
section that removes these markers.
Also, we forgot to free() the memory for 'list', so add that to the
'cleanup' section. Also, use a cleaner mechanism for clearing those
flags.
Signed-off-by: Jeff King
Signed-off-by: Derrick Stolee
---
commit-reach.c| 44 +--
t/helper/test-reach.c | 22 +-
t/t6600-test-reach.sh | 30 +++--
3 files changed, 79 insertions(+), 17 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 86715c103c..db84f85986 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -544,20 +544,42 @@ int can_all_from_reach_with_flag(struct object_array
*from,
{
struct commit **list = NULL;
int i;
+ int nr_commits;
int result = 1;
ALLOC_ARRAY(list, from->nr);
+ nr_commits = 0;
for (i = 0; i < from->nr; i++) {
- list[i] = (struct commit *)from->objects[i].item;
+ struct object *from_one = from->objects[i].item;
- if (parse_commit(list[i]) ||
- list[i]->generation < min_generation)
- return 0;
+ if (!from_one || from_one->flags & assign_flag)
+ continue;
+
+ from_one = deref_tag(the_repository, from_one,
+"a from object", 0);
+ if (!from_one || from_one->type != OBJ_COMMIT) {
+ /* no way to tell if this is reachable by
+* looking at the ancestry chain alone, so
+* leave a note to ourselves not to worry about
+* this object anymore.
+*/
+ from->objects[i].item->flags |= assign_flag;
+ continue;
+ }
+
+ list[nr_commits] = (struct commit *)from_one;
+ if (parse_commit(list[nr_commits]) ||
+ list[nr_commits]->generation < min_generation) {
+ result = 0;
+ goto cleanup;
+ }
+
+ nr_commits++;
}
- QSORT(list, from->nr, compare_commits_by_gen);
+ QSORT(list, nr_commits, compare_commits_by_gen);
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
/* DFS from list[i] */
struct commit_list *stack = NULL;
@@ -600,10 +622,12 @@ int can_all_from_reach_with_flag(struct object_array
*from,
}
cleanup:
- for (i = 0; i < from->nr; i++) {
- clear_commit_marks(list[i], RESULT);
- clear_commit_marks(list[i], assign_flag);
- }
+ clear_commit_marks_many(nr_commits, list, RESULT | assign_flag);
+ free(list);
+
+ for (i = 0; i < from->nr; i++)
+ from->objects[i].item->flags &= ~assign_flag;
+
return result;
}
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index eb21103998..08d2ea68e8 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -31,6 +31,7 @@ int cmd__reach(int ac, const char **av)
struct object_id oid_A, oid_B;
struct commit *A, *B;
struct commit_list *X, *Y;
+ struct object_array X_obj = OBJECT_ARRAY_INIT;
struct commit **X_array;
int X_nr, X_alloc;
struct strbuf buf = STRBUF_INIT;
@@ -49,7 +50,8 @@ int cmd__reach(int ac, const char **av)
while (strbuf_getline(, stdin) != EOF) {
struct object_id oid;
- struct object *o;
+ struct object *orig;
+ struct object *peeled;
struct commit *c;
if (buf.len < 3)
continue;
@@ -57,14 +59,14 @@ int cmd__reach(int ac, const char **av)
if (get_oid_committish(buf.buf + 2, ))
Re: [PATCH v3 1/2] commit-reach: properly peel tags
On 9/21/2018 7:56 PM, Jeff King wrote:
On Fri, Sep 21, 2018 at 08:05:26AM -0700, Derrick Stolee via GitGitGadget wrote:
+ if (!from_one || from_one->type != OBJ_COMMIT) {
+ /* no way to tell if this is reachable by
+* looking at the ancestry chain alone, so
+* leave a note to ourselves not to worry about
+* this object anymore.
+*/
A minor nit, but the original comment you restored here has a style
violation. Might be worth fixing up (but certainly not worth a re-roll
on its own).
@@ -600,7 +622,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
}
cleanup:
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
clear_commit_marks(list[i], RESULT);
clear_commit_marks(list[i], assign_flag);
Also, a minor aside, but I think it would be slightly more efficient for
those final two lines to do:
clear_commit_marks(list[i], RESULT | assign_flag);
Of course, that's totally orthogonal to this patch, but it may make you
feel better to offset the other round of clearing I'm suggesting. ;)
This is definitely a better thing to do, and I'll make the change in v4,
along with the comment style cleanup above.
Thanks,
-Stolee
Re: [PATCH v3 1/2] commit-reach: properly peel tags
On Fri, Sep 21, 2018 at 08:05:26AM -0700, Derrick Stolee via GitGitGadget wrote:
> diff --git a/commit-reach.c b/commit-reach.c
> index 86715c103c..e748414d04 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -544,20 +544,42 @@ int can_all_from_reach_with_flag(struct object_array
> *from,
> {
> struct commit **list = NULL;
> int i;
> + int nr_commits;
> int result = 1;
>
> ALLOC_ARRAY(list, from->nr);
> + nr_commits = 0;
> for (i = 0; i < from->nr; i++) {
> - list[i] = (struct commit *)from->objects[i].item;
> + struct object *from_one = from->objects[i].item;
>
> - if (parse_commit(list[i]) ||
> - list[i]->generation < min_generation)
> - return 0;
> + if (!from_one || from_one->flags & assign_flag)
> + continue;
> +
> + from_one = deref_tag(the_repository, from_one,
> + "a from object", 0);
> + if (!from_one || from_one->type != OBJ_COMMIT) {
> + /* no way to tell if this is reachable by
> + * looking at the ancestry chain alone, so
> + * leave a note to ourselves not to worry about
> + * this object anymore.
> + */
A minor nit, but the original comment you restored here has a style
violation. Might be worth fixing up (but certainly not worth a re-roll
on its own).
> + from->objects[i].item->flags |= assign_flag;
OK, so here we mark the original tag with a flag...
> +
> + list[nr_commits] = (struct commit *)from_one;
> + if (parse_commit(list[nr_commits]) ||
> + list[nr_commits]->generation < min_generation) {
> + result = 0;
> + goto cleanup;
> + }
And we jump to the cleanup here, but...
> @@ -600,7 +622,7 @@ int can_all_from_reach_with_flag(struct object_array
> *from,
> }
>
> cleanup:
> - for (i = 0; i < from->nr; i++) {
> + for (i = 0; i < nr_commits; i++) {
> clear_commit_marks(list[i], RESULT);
> clear_commit_marks(list[i], assign_flag);
This walks over the items in the list array, which don't include the tag
we marked. Did we decide that's OK? I think it's actually how the
original code behaved, too, but it seems funny. At the least we might
want to call it out in the commit message.
Should we also be walking from->objects and clearing the flags from
there (maybe not RESULT, but assign_flag)? It's not clear to me if it's
unintentional for those to be marked after the function leaves or not.
Also, a minor aside, but I think it would be slightly more efficient for
those final two lines to do:
clear_commit_marks(list[i], RESULT | assign_flag);
Of course, that's totally orthogonal to this patch, but it may make you
feel better to offset the other round of clearing I'm suggesting. ;)
> diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
> index eb21103998..08d2ea68e8 100644
> --- a/t/helper/test-reach.c
> +++ b/t/helper/test-reach.c
These bits all looked good to me.
-Peff
[PATCH v3 0/2] Properly peel tags in can_all_from_reach_with_flags()
As Peff reported [1], the refactored can_all_from_reach_with_flags() method
does not properly peel tags. Since the helper method can_all_from_reach()
and code in t/helper/test-reach.c all peel tags before getting to this
method, it is not super-simple to create a test that demonstrates this.
I modified t/helper/test-reach.c to allow calling
can_all_from_reach_with_flags() directly, and added a test in
t6600-test-reach.sh that demonstrates the segfault without the fix.
For V2, I compared the loop that inspects the 'from' commits in commit
ba3ca1edce "commit-reach: move can_all_from_reach_with_flags" to the version
here and got the following diff:
3c3
< if (from_one->flags & assign_flag)
---
> if (!from_one || from_one->flags & assign_flag)
5c5,7
< from_one = deref_tag(the_repository, from_one, "a from
object", 0);
---
>
> from_one = deref_tag(the_repository, from_one,
> "a from object", 0);
14a17,22
>
> list[nr_commits] = (struct commit *)from_one;
> if (parse_commit(list[nr_commits]) ||
> list[nr_commits]->generation < min_generation)
> return 0; /* is this a leak? */
> nr_commits++;
This diff includes the early termination we had before 'deref_tag' and the
comment for why we can ignore non-commit objects.
[1]
https://public-inbox.org/git/0bf9103c-9377-506b-7ad7-e5273d8e9...@gmail.com/T/#u
Derrick Stolee (2):
commit-reach: properly peel tags
commit-reach: fix memory and flag leaks
commit-reach.c| 41 ++---
t/helper/test-reach.c | 22 +-
t/t6600-test-reach.sh | 30 --
3 files changed, 79 insertions(+), 14 deletions(-)
base-commit: 6621c838743812aaba96e55cfec8524ea1144c2d
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-39%2Fderrickstolee%2Ftag-fix-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-39/derrickstolee/tag-fix-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/39
Range-diff vs v2:
1: 4bf21204dd ! 1: 0a1e661271 commit-reach: properly peel tags
@@ -53,8 +53,11 @@
+
+ list[nr_commits] = (struct commit *)from_one;
+ if (parse_commit(list[nr_commits]) ||
-+ list[nr_commits]->generation < min_generation)
-+ return 0; /* is this a leak? */
++ list[nr_commits]->generation < min_generation) {
++ result = 0;
++ goto cleanup;
++ }
++
+ nr_commits++;
}
-: -- > 2: b2e0ee4978 commit-reach: fix memory and flag leaks
--
gitgitgadget
[PATCH v3 1/2] commit-reach: properly peel tags
From: Derrick Stolee
The can_all_from_reach_with_flag() algorithm was refactored in 4fbcca4e
"commit-reach: make can_all_from_reach... linear" but incorrectly
assumed that all objects provided were commits. During a fetch
negotiation, ok_to_give_up() in upload-pack.c may provide unpeeled tags
to the 'from' array. The current code creates a segfault.
Add a direct call to can_all_from_reach_with_flag() in 'test-tool reach'
and add a test in t6600-test-reach.sh that demonstrates this segfault.
Correct the issue by peeling tags when investigating the initial list
of objects in the 'from' array.
Signed-off-by: Jeff King
Signed-off-by: Derrick Stolee
---
commit-reach.c| 36 +---
t/helper/test-reach.c | 22 +-
t/t6600-test-reach.sh | 30 --
3 files changed, 74 insertions(+), 14 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 86715c103c..e748414d04 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -544,20 +544,42 @@ int can_all_from_reach_with_flag(struct object_array
*from,
{
struct commit **list = NULL;
int i;
+ int nr_commits;
int result = 1;
ALLOC_ARRAY(list, from->nr);
+ nr_commits = 0;
for (i = 0; i < from->nr; i++) {
- list[i] = (struct commit *)from->objects[i].item;
+ struct object *from_one = from->objects[i].item;
- if (parse_commit(list[i]) ||
- list[i]->generation < min_generation)
- return 0;
+ if (!from_one || from_one->flags & assign_flag)
+ continue;
+
+ from_one = deref_tag(the_repository, from_one,
+"a from object", 0);
+ if (!from_one || from_one->type != OBJ_COMMIT) {
+ /* no way to tell if this is reachable by
+* looking at the ancestry chain alone, so
+* leave a note to ourselves not to worry about
+* this object anymore.
+*/
+ from->objects[i].item->flags |= assign_flag;
+ continue;
+ }
+
+ list[nr_commits] = (struct commit *)from_one;
+ if (parse_commit(list[nr_commits]) ||
+ list[nr_commits]->generation < min_generation) {
+ result = 0;
+ goto cleanup;
+ }
+
+ nr_commits++;
}
- QSORT(list, from->nr, compare_commits_by_gen);
+ QSORT(list, nr_commits, compare_commits_by_gen);
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
/* DFS from list[i] */
struct commit_list *stack = NULL;
@@ -600,7 +622,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
}
cleanup:
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
clear_commit_marks(list[i], RESULT);
clear_commit_marks(list[i], assign_flag);
}
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index eb21103998..08d2ea68e8 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -31,6 +31,7 @@ int cmd__reach(int ac, const char **av)
struct object_id oid_A, oid_B;
struct commit *A, *B;
struct commit_list *X, *Y;
+ struct object_array X_obj = OBJECT_ARRAY_INIT;
struct commit **X_array;
int X_nr, X_alloc;
struct strbuf buf = STRBUF_INIT;
@@ -49,7 +50,8 @@ int cmd__reach(int ac, const char **av)
while (strbuf_getline(, stdin) != EOF) {
struct object_id oid;
- struct object *o;
+ struct object *orig;
+ struct object *peeled;
struct commit *c;
if (buf.len < 3)
continue;
@@ -57,14 +59,14 @@ int cmd__reach(int ac, const char **av)
if (get_oid_committish(buf.buf + 2, ))
die("failed to resolve %s", buf.buf + 2);
- o = parse_object(r, );
- o = deref_tag_noverify(o);
+ orig = parse_object(r, );
+ peeled = deref_tag_noverify(orig);
- if (!o)
+ if (!peeled)
die("failed to load commit for input %s resulting in
oid %s\n",
buf.buf, oid_to_hex());
- c = object_as_type(r, o, OBJ_COMMIT, 0);
+ c = object_as_type(r, peeled, OBJ_COMMIT, 0);
if (!c)
die("failed to load commit for input %s resulting in
oid %s\n",
@@ -85,6 +87,7 @@ int cmd__reach(int ac, const char **av)
[PATCH 0/3] doc: fixups for ab/fetch-tags-noclobber
Of course I only noticed these after the series had landed in master... Ævar Arnfjörð Bjarmason (3): push doc: add spacing between two words fetch doc: correct grammar in --force docs fetch doc: correct grammar in --force docs Documentation/git-push.txt | 2 +- Documentation/pull-fetch-param.txt | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) -- 2.19.0.rc2.392.g5ba43deb5a
Re: with git 1.8.3.1 get only merged tags
On Thu, Sep 13, 2018 at 1:27 PM Ævar Arnfjörð Bjarmason wrote: > > > On Tue, Sep 11 2018, Michal Novotny wrote: > > > I need to emulate git tag --merged with very old git 1.8.3.1. Is that > > somehow possible? > > I am looking for a bash function that would take what git 1.8.3.1 > > offers and return only the tags accessible from the current branch > > Jeff answer the question you had, but I just have one of my own: Is > RedHat stuck on 1.8-era git in some release it's still maintaining, does > this mean that e.g. you're still backporting security fixes to this > 2012-era release? Yes, that's exactly the case with RHEL-7. clime
Re: [PATCH v2 1/1] commit-reach: properly peel tags
Derrick Stolee writes:
>> +if (!from_one || from_one->type != OBJ_COMMIT) {
>> +/* no way to tell if this is reachable by
>> + * looking at the ancestry chain alone, so
>> + * leave a note to ourselves not to worry about
>> + * this object anymore.
>> + */
>> +from->objects[i].item->flags |= assign_flag;
>> +continue;
>> +}
>> +
>> +list[nr_commits] = (struct commit *)from_one;
>> +if (parse_commit(list[nr_commits]) ||
>> +list[nr_commits]->generation < min_generation)
>> +return 0; /* is this a leak? */
>
> Of course, after sending v2, I see this comment. This is a leak of
> 'list' and should be fixed.
>
> Not only is it a leak here, it is also a leak in the 'cleanup'
> section. I'll squash the following into v3, but I'll let v2 simmer for
> review before rerolling.
>
> diff --git a/commit-reach.c b/commit-reach.c
> index 4048a2132a..c457d8d85f 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -569,8 +569,11 @@ int can_all_from_reach_with_flag(struct
> object_array *from,
>
> list[nr_commits] = (struct commit *)from_one;
> if (parse_commit(list[nr_commits]) ||
> - list[nr_commits]->generation < min_generation)
> - return 0; /* is this a leak? */
> + list[nr_commits]->generation < min_generation) {
> + result = 0;
> + goto cleanup;
> + }
> +
> nr_commits++;
> }
>
> @@ -623,6 +626,7 @@ int can_all_from_reach_with_flag(struct
> object_array *from,
> clear_commit_marks(list[i], RESULT);
> clear_commit_marks(list[i], assign_flag);
> }
> + free(list);
> return result;
> }
With this, commit marks are cleared even when we do the "early
return", but only for the objects that appear in the resulting
list[]. Because the for() loop in the last hunk interates over
list[], those non-commit objects that are smudged with assign_flag
but never made to list[] will be left smudged when this function
returns (this is true when there is no early return).
Is that intended?
Re: [PATCH v2 1/1] commit-reach: properly peel tags
On 9/13/2018 12:10 PM, Derrick Stolee via GitGitGadget wrote:
From: Derrick Stolee
The can_all_from_reach_with_flag() algorithm was refactored in 4fbcca4e
"commit-reach: make can_all_from_reach... linear" but incorrectly
assumed that all objects provided were commits. During a fetch
negotiation, ok_to_give_up() in upload-pack.c may provide unpeeled tags
to the 'from' array. The current code creates a segfault.
Add a direct call to can_all_from_reach_with_flag() in 'test-tool reach'
and add a test in t6600-test-reach.sh that demonstrates this segfault.
Correct the issue by peeling tags when investigating the initial list
of objects in the 'from' array.
Signed-off-by: Jeff King
Signed-off-by: Derrick Stolee
---
commit-reach.c| 33 ++---
t/helper/test-reach.c | 22 +-
t/t6600-test-reach.sh | 30 --
3 files changed, 71 insertions(+), 14 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 86715c103c..4048a2132a 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -544,20 +544,39 @@ int can_all_from_reach_with_flag(struct object_array
*from,
{
struct commit **list = NULL;
int i;
+ int nr_commits;
int result = 1;
ALLOC_ARRAY(list, from->nr);
+ nr_commits = 0;
for (i = 0; i < from->nr; i++) {
- list[i] = (struct commit *)from->objects[i].item;
+ struct object *from_one = from->objects[i].item;
- if (parse_commit(list[i]) ||
- list[i]->generation < min_generation)
- return 0;
+ if (!from_one || from_one->flags & assign_flag)
+ continue;
+
+ from_one = deref_tag(the_repository, from_one,
+"a from object", 0);
+ if (!from_one || from_one->type != OBJ_COMMIT) {
+ /* no way to tell if this is reachable by
+* looking at the ancestry chain alone, so
+* leave a note to ourselves not to worry about
+* this object anymore.
+*/
+ from->objects[i].item->flags |= assign_flag;
+ continue;
+ }
+
+ list[nr_commits] = (struct commit *)from_one;
+ if (parse_commit(list[nr_commits]) ||
+ list[nr_commits]->generation < min_generation)
+ return 0; /* is this a leak? */
Of course, after sending v2, I see this comment. This is a leak of
'list' and should be fixed.
Not only is it a leak here, it is also a leak in the 'cleanup' section.
I'll squash the following into v3, but I'll let v2 simmer for review
before rerolling.
diff --git a/commit-reach.c b/commit-reach.c
index 4048a2132a..c457d8d85f 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -569,8 +569,11 @@ int can_all_from_reach_with_flag(struct
object_array *from,
list[nr_commits] = (struct commit *)from_one;
if (parse_commit(list[nr_commits]) ||
- list[nr_commits]->generation < min_generation)
- return 0; /* is this a leak? */
+ list[nr_commits]->generation < min_generation) {
+ result = 0;
+ goto cleanup;
+ }
+
nr_commits++;
}
@@ -623,6 +626,7 @@ int can_all_from_reach_with_flag(struct object_array
*from,
clear_commit_marks(list[i], RESULT);
clear_commit_marks(list[i], assign_flag);
}
+ free(list);
return result;
}
[PATCH v2 1/1] commit-reach: properly peel tags
From: Derrick Stolee
The can_all_from_reach_with_flag() algorithm was refactored in 4fbcca4e
"commit-reach: make can_all_from_reach... linear" but incorrectly
assumed that all objects provided were commits. During a fetch
negotiation, ok_to_give_up() in upload-pack.c may provide unpeeled tags
to the 'from' array. The current code creates a segfault.
Add a direct call to can_all_from_reach_with_flag() in 'test-tool reach'
and add a test in t6600-test-reach.sh that demonstrates this segfault.
Correct the issue by peeling tags when investigating the initial list
of objects in the 'from' array.
Signed-off-by: Jeff King
Signed-off-by: Derrick Stolee
---
commit-reach.c| 33 ++---
t/helper/test-reach.c | 22 +-
t/t6600-test-reach.sh | 30 --
3 files changed, 71 insertions(+), 14 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 86715c103c..4048a2132a 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -544,20 +544,39 @@ int can_all_from_reach_with_flag(struct object_array
*from,
{
struct commit **list = NULL;
int i;
+ int nr_commits;
int result = 1;
ALLOC_ARRAY(list, from->nr);
+ nr_commits = 0;
for (i = 0; i < from->nr; i++) {
- list[i] = (struct commit *)from->objects[i].item;
+ struct object *from_one = from->objects[i].item;
- if (parse_commit(list[i]) ||
- list[i]->generation < min_generation)
- return 0;
+ if (!from_one || from_one->flags & assign_flag)
+ continue;
+
+ from_one = deref_tag(the_repository, from_one,
+"a from object", 0);
+ if (!from_one || from_one->type != OBJ_COMMIT) {
+ /* no way to tell if this is reachable by
+* looking at the ancestry chain alone, so
+* leave a note to ourselves not to worry about
+* this object anymore.
+*/
+ from->objects[i].item->flags |= assign_flag;
+ continue;
+ }
+
+ list[nr_commits] = (struct commit *)from_one;
+ if (parse_commit(list[nr_commits]) ||
+ list[nr_commits]->generation < min_generation)
+ return 0; /* is this a leak? */
+ nr_commits++;
}
- QSORT(list, from->nr, compare_commits_by_gen);
+ QSORT(list, nr_commits, compare_commits_by_gen);
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
/* DFS from list[i] */
struct commit_list *stack = NULL;
@@ -600,7 +619,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
}
cleanup:
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
clear_commit_marks(list[i], RESULT);
clear_commit_marks(list[i], assign_flag);
}
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index eb21103998..08d2ea68e8 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -31,6 +31,7 @@ int cmd__reach(int ac, const char **av)
struct object_id oid_A, oid_B;
struct commit *A, *B;
struct commit_list *X, *Y;
+ struct object_array X_obj = OBJECT_ARRAY_INIT;
struct commit **X_array;
int X_nr, X_alloc;
struct strbuf buf = STRBUF_INIT;
@@ -49,7 +50,8 @@ int cmd__reach(int ac, const char **av)
while (strbuf_getline(, stdin) != EOF) {
struct object_id oid;
- struct object *o;
+ struct object *orig;
+ struct object *peeled;
struct commit *c;
if (buf.len < 3)
continue;
@@ -57,14 +59,14 @@ int cmd__reach(int ac, const char **av)
if (get_oid_committish(buf.buf + 2, ))
die("failed to resolve %s", buf.buf + 2);
- o = parse_object(r, );
- o = deref_tag_noverify(o);
+ orig = parse_object(r, );
+ peeled = deref_tag_noverify(orig);
- if (!o)
+ if (!peeled)
die("failed to load commit for input %s resulting in
oid %s\n",
buf.buf, oid_to_hex());
- c = object_as_type(r, o, OBJ_COMMIT, 0);
+ c = object_as_type(r, peeled, OBJ_COMMIT, 0);
if (!c)
die("failed to load commit for input %s resulting in
oid %s\n",
@@ -85,6 +87,7 @@ int cmd__reach(int ac, const char **av)
commit_list_insert(c, );
[PATCH v2 0/1] Properly peel tags in can_all_from_reach_with_flags()
As Peff reported [1], the refactored can_all_from_reach_with_flags() method
does not properly peel tags. Since the helper method can_all_from_reach()
and code in t/helper/test-reach.c all peel tags before getting to this
method, it is not super-simple to create a test that demonstrates this.
I modified t/helper/test-reach.c to allow calling
can_all_from_reach_with_flags() directly, and added a test in
t6600-test-reach.sh that demonstrates the segfault without the fix.
For V2, I compared the loop that inspects the 'from' commits in commit
ba3ca1edce "commit-reach: move can_all_from_reach_with_flags" to the version
here and got the following diff:
3c3
< if (from_one->flags & assign_flag)
---
> if (!from_one || from_one->flags & assign_flag)
5c5,7
< from_one = deref_tag(the_repository, from_one, "a from
object", 0);
---
>
> from_one = deref_tag(the_repository, from_one,
> "a from object", 0);
14a17,22
>
> list[nr_commits] = (struct commit *)from_one;
> if (parse_commit(list[nr_commits]) ||
> list[nr_commits]->generation < min_generation)
> return 0; /* is this a leak? */
> nr_commits++;
This diff includes the early termination we had before 'deref_tag' and the
comment for why we can ignore non-commit objects.
[1]
https://public-inbox.org/git/0bf9103c-9377-506b-7ad7-e5273d8e9...@gmail.com/T/#u
Derrick Stolee (1):
commit-reach: properly peel tags
commit-reach.c| 33 ++---
t/helper/test-reach.c | 22 +-
t/t6600-test-reach.sh | 30 --
3 files changed, 71 insertions(+), 14 deletions(-)
base-commit: 6621c838743812aaba96e55cfec8524ea1144c2d
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-39%2Fderrickstolee%2Ftag-fix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-39/derrickstolee/tag-fix-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/39
Range-diff vs v1:
1: 948e28 ! 1: 4bf21204dd commit-reach: properly peel tags
@@ -36,9 +36,17 @@
- if (parse_commit(list[i]) ||
- list[i]->generation < min_generation)
- return 0;
++ if (!from_one || from_one->flags & assign_flag)
++ continue;
++
+ from_one = deref_tag(the_repository, from_one,
+ "a from object", 0);
+ if (!from_one || from_one->type != OBJ_COMMIT) {
++ /* no way to tell if this is reachable by
++ * looking at the ancestry chain alone, so
++ * leave a note to ourselves not to worry about
++ * this object anymore.
++ */
+ from->objects[i].item->flags |= assign_flag;
+ continue;
+ }
@@ -187,7 +195,7 @@
+ echo "can_all_from_reach_with_flag(X,_,_,0,0):1" >expect &&
+ test_three_modes can_all_from_reach_with_flag
+'
-+
++
test_expect_success 'commit_contains:hit' '
cat >input <<-\EOF &&
A:commit-7-7
--
gitgitgadget
Re: with git 1.8.3.1 get only merged tags
On Tue, Sep 11 2018, Michal Novotny wrote: > I need to emulate git tag --merged with very old git 1.8.3.1. Is that > somehow possible? > I am looking for a bash function that would take what git 1.8.3.1 > offers and return only the tags accessible from the current branch Jeff answer the question you had, but I just have one of my own: Is RedHat stuck on 1.8-era git in some release it's still maintaining, does this mean that e.g. you're still backporting security fixes to this 2012-era release?
Re: with git 1.8.3.1 get only merged tags
On Tue, Sep 11, 2018 at 9:05 PM Jeff King wrote:
>
> On Tue, Sep 11, 2018 at 12:43:15PM +0200, Michal Novotny wrote:
>
> > I need to emulate git tag --merged with very old git 1.8.3.1. Is that
> > somehow possible?
> > I am looking for a bash function that would take what git 1.8.3.1
> > offers and return only the tags accessible from the current branch
> > tip. Could you, please, give me at least a hint how this could be
> > done?
>
> This is not particularly fast, but it should work:
>
> git for-each-ref refs/tags |
> cut -f2 |
> while read tag; do
> test "$(git merge-base $tag HEAD)" = \
> "$(git rev-parse $tag^{commit})" && echo $tag
> done
That works for me. Thank you a lot for help!
clime
>
> -Peff
Re: [PATCH 1/1] commit-reach: properly peel tags
On Wed, Sep 12, 2018 at 02:23:42PM -0700, Junio C Hamano wrote:
> > diff --git a/commit-reach.c b/commit-reach.c
> > index 86715c103c..6de72c6e03 100644
> > --- a/commit-reach.c
> > +++ b/commit-reach.c
> > @@ -544,20 +544,31 @@ int can_all_from_reach_with_flag(struct object_array
> > *from,
> > {
> > struct commit **list = NULL;
> > int i;
> > + int nr_commits;
> > int result = 1;
> >
> > ALLOC_ARRAY(list, from->nr);
> > + nr_commits = 0;
> > for (i = 0; i < from->nr; i++) {
> > - list[i] = (struct commit *)from->objects[i].item;
> > + struct object *from_one = from->objects[i].item;
> >
> > - if (parse_commit(list[i]) ||
> > - list[i]->generation < min_generation)
> > - return 0;
> > + from_one = deref_tag(the_repository, from_one,
> > +"a from object", 0);
> > + if (!from_one || from_one->type != OBJ_COMMIT) {
> > + from->objects[i].item->flags |= assign_flag;
>
> I wondered why this is not futzing with "from_one->flags"; by going
> back to the original from->objects[] array, the code is setting the
> flags on the original tag object and not the non-commit object that
> was pointed at by the tag.
Note that from_one may even be NULL.
> > + continue;
> > + }
> > +
> > + list[nr_commits] = (struct commit *)from_one;
> > + if (parse_commit(list[nr_commits]) ||
> > + list[nr_commits]->generation < min_generation)
> > + return 0; /* is this a leak? */
> > + nr_commits++;
> > }
>
> In the original code, the flags bits were left unchanged if the loop
> terminated by hitting a commit whose generation is too young (or
> parse_commit() returns non-zero). With this updated code, flags bit
> can be modified before the code notices the situation and leave the
> function, bypassing the "cleanup" we see below that clears the
> "assign_flag" bits.
>
> Would it be a problem that we return early without cleaning up?
>
> Even if we do not call this early return, the assign_flag bits added
> to the original tag in from->objects[i].item won't be cleaned in
> this new code, as "cleanup:" section now loops over the list[] that
> omits the object whose flags was smudged above before the "continue".
>
> Would it be a problem that we leave the assign_flags without
> cleaning up?
Yeah, I hadn't thought about the bit cleanup when making my original
suggestion. In the original code (before 4fbcca4eff), I think we did set
flags as we iterated through the loop, and we could still do an early
return when we hit "!reachable(...)". But I don't see any cleanup of
assign_flag there at all.
So I guess I'm pretty confused about what the semantics are supposed to
be.
-Peff
Re: [PATCH 1/1] commit-reach: properly peel tags
"Derrick Stolee via GitGitGadget" writes:
> diff --git a/commit-reach.c b/commit-reach.c
> index 86715c103c..6de72c6e03 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -544,20 +544,31 @@ int can_all_from_reach_with_flag(struct object_array
> *from,
> {
> struct commit **list = NULL;
> int i;
> + int nr_commits;
> int result = 1;
>
> ALLOC_ARRAY(list, from->nr);
> + nr_commits = 0;
> for (i = 0; i < from->nr; i++) {
> - list[i] = (struct commit *)from->objects[i].item;
> + struct object *from_one = from->objects[i].item;
>
> - if (parse_commit(list[i]) ||
> - list[i]->generation < min_generation)
> - return 0;
> + from_one = deref_tag(the_repository, from_one,
> + "a from object", 0);
> + if (!from_one || from_one->type != OBJ_COMMIT) {
> + from->objects[i].item->flags |= assign_flag;
I wondered why this is not futzing with "from_one->flags"; by going
back to the original from->objects[] array, the code is setting the
flags on the original tag object and not the non-commit object that
was pointed at by the tag.
> + continue;
> + }
> +
> + list[nr_commits] = (struct commit *)from_one;
> + if (parse_commit(list[nr_commits]) ||
> + list[nr_commits]->generation < min_generation)
> + return 0; /* is this a leak? */
> + nr_commits++;
> }
In the original code, the flags bits were left unchanged if the loop
terminated by hitting a commit whose generation is too young (or
parse_commit() returns non-zero). With this updated code, flags bit
can be modified before the code notices the situation and leave the
function, bypassing the "cleanup" we see below that clears the
"assign_flag" bits.
Would it be a problem that we return early without cleaning up?
Even if we do not call this early return, the assign_flag bits added
to the original tag in from->objects[i].item won't be cleaned in
this new code, as "cleanup:" section now loops over the list[] that
omits the object whose flags was smudged above before the "continue".
Would it be a problem that we leave the assign_flags without
cleaning up?
> - QSORT(list, from->nr, compare_commits_by_gen);
> + QSORT(list, nr_commits, compare_commits_by_gen);
>
> - for (i = 0; i < from->nr; i++) {
> + for (i = 0; i < nr_commits; i++) {
> /* DFS from list[i] */
> struct commit_list *stack = NULL;
>
> @@ -600,7 +611,7 @@ int can_all_from_reach_with_flag(struct object_array
> *from,
> }
>
> cleanup:
> - for (i = 0; i < from->nr; i++) {
> + for (i = 0; i < nr_commits; i++) {
> clear_commit_marks(list[i], RESULT);
> clear_commit_marks(list[i], assign_flag);
> }
Re: [PATCH 1/1] commit-reach: properly peel tags
On Wed, Sep 12, 2018 at 07:22:56AM -0700, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee
>
> The can_all_from_reach_with_flag() algorithm was refactored in 4fbcca4e
> "commit-reach: make can_all_from_reach... linear" but incorrectly
> assumed that all objects provided were commits. During a fetch
> negotiation, ok_to_give_up() in upload-pack.c may provide unpeeled tags
> to the 'from' array. The current code creates a segfault.
>
> Add a direct call to can_all_from_reach_with_flag() in 'test-tool reach'
> and add a test in t6600-test-reach.sh that demonstrates this segfault.
Thanks, that makes a lot of sense for reproducing. I almost wonder if
the whole X_array of commits in test-reach could just go away, and we'd
feed whatever list of objects the caller happens to send. That may make
it simpler to include non-commit objects in a variety of tests.
That said, I didn't look closely at other fallout in the program from
that, so I'll defer to your judgement.
> Correct the issue by peeling tags when investigating the initial list
> of objects in the 'from' array.
>
> Signed-off-by: Jeff King
I'm not sure if this should just be Reported-by, since I don't know that
I actually contributed any code. ;) But for anything I might have
contributed, certainly you have my signoff.
> for (i = 0; i < from->nr; i++) {
> - list[i] = (struct commit *)from->objects[i].item;
> + struct object *from_one = from->objects[i].item;
>
> - if (parse_commit(list[i]) ||
> - list[i]->generation < min_generation)
> - return 0;
> + from_one = deref_tag(the_repository, from_one,
> + "a from object", 0);
> + if (!from_one || from_one->type != OBJ_COMMIT) {
> + from->objects[i].item->flags |= assign_flag;
> + continue;
> + }
I didn't resurrect the comment from this conditional that was in the
original code (mostly because I wasn't sure if the reasoning was still
entirely valid, or what setting the flag here actually means). But it's
probably worth saying something here to explain why it's OK to punt on
this case, and what it means to just set the flag.
> [...]
The rest of the patch looks sane to me. I didn't go through the trouble
to reproduce the segfault with the test, but it sounds like you did.
-Peff
Starting subshells via tags
Hello - This is not a git bug or issue. It's just something I stumbled across and didn't see any google results for. I thought others would benefit from being aware of it. I saw a makefile which included of "git describe --tags --dirty" to define version information for a binary's --version command line parameter. The commit/tag information was passed to g++ in a Makefile via: CXXFLAGS += -DBUILD_COMMIT="\"$(shell git describe --tags --dirty)\"" For fun (on Linux) I made simple c++ program and Makefile with the above CXXFLAGS, and a tag (backticks work too): git tag '$(echo>/tmp/test.txt)' Then built. Make executes the git command via a shell and the shell executes the subshell. /tmp/test.txt was created. The tags themselves don't allow spaces so the complexity of the command is limited, though I didn't explore what I could do with chaining shells or escape characters. It's easy enough to add a script to the repository where the tag is located and execute that script from the tag's subshell with a tag, such as $(./test.sh). Again, this is not at all a git issue, git is just used as the transport. As with every other shell attack, it comes down to "always sanitize what you pass through to a shell". Or don't pass it to the shell at all, use another mechanism.
[PATCH 1/1] commit-reach: properly peel tags
From: Derrick Stolee
The can_all_from_reach_with_flag() algorithm was refactored in 4fbcca4e
"commit-reach: make can_all_from_reach... linear" but incorrectly
assumed that all objects provided were commits. During a fetch
negotiation, ok_to_give_up() in upload-pack.c may provide unpeeled tags
to the 'from' array. The current code creates a segfault.
Add a direct call to can_all_from_reach_with_flag() in 'test-tool reach'
and add a test in t6600-test-reach.sh that demonstrates this segfault.
Correct the issue by peeling tags when investigating the initial list
of objects in the 'from' array.
Signed-off-by: Jeff King
Signed-off-by: Derrick Stolee
---
commit-reach.c| 25 ++---
t/helper/test-reach.c | 22 +-
t/t6600-test-reach.sh | 30 --
3 files changed, 63 insertions(+), 14 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 86715c103c..6de72c6e03 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -544,20 +544,31 @@ int can_all_from_reach_with_flag(struct object_array
*from,
{
struct commit **list = NULL;
int i;
+ int nr_commits;
int result = 1;
ALLOC_ARRAY(list, from->nr);
+ nr_commits = 0;
for (i = 0; i < from->nr; i++) {
- list[i] = (struct commit *)from->objects[i].item;
+ struct object *from_one = from->objects[i].item;
- if (parse_commit(list[i]) ||
- list[i]->generation < min_generation)
- return 0;
+ from_one = deref_tag(the_repository, from_one,
+"a from object", 0);
+ if (!from_one || from_one->type != OBJ_COMMIT) {
+ from->objects[i].item->flags |= assign_flag;
+ continue;
+ }
+
+ list[nr_commits] = (struct commit *)from_one;
+ if (parse_commit(list[nr_commits]) ||
+ list[nr_commits]->generation < min_generation)
+ return 0; /* is this a leak? */
+ nr_commits++;
}
- QSORT(list, from->nr, compare_commits_by_gen);
+ QSORT(list, nr_commits, compare_commits_by_gen);
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
/* DFS from list[i] */
struct commit_list *stack = NULL;
@@ -600,7 +611,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
}
cleanup:
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
clear_commit_marks(list[i], RESULT);
clear_commit_marks(list[i], assign_flag);
}
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index eb21103998..08d2ea68e8 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -31,6 +31,7 @@ int cmd__reach(int ac, const char **av)
struct object_id oid_A, oid_B;
struct commit *A, *B;
struct commit_list *X, *Y;
+ struct object_array X_obj = OBJECT_ARRAY_INIT;
struct commit **X_array;
int X_nr, X_alloc;
struct strbuf buf = STRBUF_INIT;
@@ -49,7 +50,8 @@ int cmd__reach(int ac, const char **av)
while (strbuf_getline(, stdin) != EOF) {
struct object_id oid;
- struct object *o;
+ struct object *orig;
+ struct object *peeled;
struct commit *c;
if (buf.len < 3)
continue;
@@ -57,14 +59,14 @@ int cmd__reach(int ac, const char **av)
if (get_oid_committish(buf.buf + 2, ))
die("failed to resolve %s", buf.buf + 2);
- o = parse_object(r, );
- o = deref_tag_noverify(o);
+ orig = parse_object(r, );
+ peeled = deref_tag_noverify(orig);
- if (!o)
+ if (!peeled)
die("failed to load commit for input %s resulting in
oid %s\n",
buf.buf, oid_to_hex());
- c = object_as_type(r, o, OBJ_COMMIT, 0);
+ c = object_as_type(r, peeled, OBJ_COMMIT, 0);
if (!c)
die("failed to load commit for input %s resulting in
oid %s\n",
@@ -85,6 +87,7 @@ int cmd__reach(int ac, const char **av)
commit_list_insert(c, );
ALLOC_GROW(X_array, X_nr + 1, X_alloc);
X_array[X_nr++] = c;
+ add_object_array(orig, NULL, _obj);
break;
case 'Y':
@@ -113,6 +116,15 @@ int cmd__reach(int ac, const char **av)
print_sorted_commit_ids(list);
} else if (!s
[PATCH 0/1] Properly peel tags in can_all_from_reach_with_flags()
As Peff reported [1], the refactored can_all_from_reach_with_flags() method does not properly peel tags. Since the helper method can_all_from_reach() and code in t/helper/test-reach.c all peel tags before getting to this method, it is not super-simple to create a test that demonstrates this. I modified t/helper/test-reach.c to allow calling can_all_from_reach_with_flags() directly, and added a test in t6600-test-reach.sh that demonstrates the segfault without the fix. The fix in commit-reach.c is Peff's fix verbatim. [1] https://public-inbox.org/git/0bf9103c-9377-506b-7ad7-e5273d8e9...@gmail.com/T/#u Derrick Stolee (1): commit-reach: properly peel tags commit-reach.c| 25 ++--- t/helper/test-reach.c | 22 +- t/t6600-test-reach.sh | 30 -- 3 files changed, 63 insertions(+), 14 deletions(-) base-commit: 6621c838743812aaba96e55cfec8524ea1144c2d Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-39%2Fderrickstolee%2Ftag-fix-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-39/derrickstolee/tag-fix-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/39 -- gitgitgadget
Re: with git 1.8.3.1 get only merged tags
On Tue, Sep 11, 2018 at 12:43:15PM +0200, Michal Novotny wrote:
> I need to emulate git tag --merged with very old git 1.8.3.1. Is that
> somehow possible?
> I am looking for a bash function that would take what git 1.8.3.1
> offers and return only the tags accessible from the current branch
> tip. Could you, please, give me at least a hint how this could be
> done?
This is not particularly fast, but it should work:
git for-each-ref refs/tags |
cut -f2 |
while read tag; do
test "$(git merge-base $tag HEAD)" = \
"$(git rev-parse $tag^{commit})" && echo $tag
done
-Peff
with git 1.8.3.1 get only merged tags
Hello, I need to emulate git tag --merged with very old git 1.8.3.1. Is that somehow possible? I am looking for a bash function that would take what git 1.8.3.1 offers and return only the tags accessible from the current branch tip. Could you, please, give me at least a hint how this could be done? Thank you clime
[PATCH v5 9/9] fetch: stop clobbering existing tags without --force
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/2023221658.ga22...@sigill.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
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
`...`
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(>old_oid) &&
starts_with(ref->name, "refs/tags/")) {
-
[PATCH v5 0/9] git fetch" should not clobber existing tags without --force
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
-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@gitster-ct.c.googlers.com/
-2.
https://public-inbox.org/git/20180813192249.27585-1-ava...@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
@@ -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 " 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
-. By having the optional leading `+`, you can tell Git to update
-the ref even if it is not allowed by default (e.g., it is not a
--fast-forward.) This does *not* attempt to merge into . See
--EXAMPLES below for details.
+-fast-forward.).
+-+
+-Pushing an empty allows you to delete the ref from
+-the remote repository.
+on the remote side. Whether this is allowed depends on where in
-+`refs/*` the reference lives as described in detail below. Any
-+such update does *not* attempt to merge into . See EXAMPLES
-+below for details.
++`refs/*` the 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
Re: [PATCH v4 6/6] fetch: stop clobbering existing tags without --force
Ævar Arnfjörð Bjarmason 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(>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.
[PATCH v4 6/6] fetch: stop clobbering existing tags without --force
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/2023221658.ga22...@sigill.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
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
`...`
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(>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]"),
+
[PATCH v4 0/6] "git fetch" should not clobber existing tags without --force
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
Re: [PATCH] chainlint: match "quoted" here-doc tags
Eric Sunshine writes: > This is an extract from v3 of es/chain-lint-more[1] which never got > picked up. Instead, v2 of that series[2] got merged to 'next' and later > 'master'. The only change between v2 and v3 was to make 'chainlint' also > recognize double-quoted here-doc tags. This solo patch makes that change > incrementally atop v2. Thanks.
[PATCH] chainlint: match "quoted" here-doc tags
A here-doc tag can be quoted ('EOF'/"EOF") or escaped (\EOF) to suppress
interpolation within the body. chainlint recognizes single-quoted and
escaped tags, but does not know about double-quoted tags. For
completeness, teach it to recognize double-quoted tags, as well.
Signed-off-by: Eric Sunshine
---
This is an extract from v3 of es/chain-lint-more[1] which never got
picked up. Instead, v2 of that series[2] got merged to 'next' and later
'master'. The only change between v2 and v3 was to make 'chainlint' also
recognize double-quoted here-doc tags. This solo patch makes that change
incrementally atop v2.
This is built on 'master'. With the default "recursive" strategy, it
merges cleanly with Ævar's ab/portable[3] changes to 'chainlint' for AIX
compatibility, currently in 'next'.
[1]: https://public-inbox.org/git/20180815184552.8418-1-sunsh...@sunshineco.com/
[2]:
https://public-inbox.org/git/20180813084739.16134-1-sunsh...@sunshineco.com/
[3]: https://public-inbox.org/git/20180824152016.20286-1-ava...@gmail.com/
t/chainlint.sed | 8
t/chainlint/here-doc.expect | 2 ++
t/chainlint/here-doc.test| 7 +++
t/chainlint/subshell-here-doc.expect | 1 +
t/chainlint/subshell-here-doc.test | 4
5 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/t/chainlint.sed b/t/chainlint.sed
index 8544df38df..1da58b554b 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -94,8 +94,8 @@
# here-doc -- swallow it to avoid false hits within its body (but keep the
# command to which it was attached)
-/<<[ ]*[-\\']*[A-Za-z0-9_]/ {
- s/^\(.*\)<<[]*[-\\']*\([A-Za-z0-9_][A-Za-z0-9_]*\)'*/<\2>\1<\1<\1<\1<foo &&
cat >bar &&
+cat >boo &&
+
horticulture
diff --git a/t/chainlint/here-doc.test b/t/chainlint/here-doc.test
index f2bb14b693..ad4ce8afd9 100644
--- a/t/chainlint/here-doc.test
+++ b/t/chainlint/here-doc.test
@@ -21,6 +21,13 @@ boz
woz
FUMP
+# LINT: swallow "quoted" here-doc
+cat <<"zump" >boo &&
+snoz
+boz
+woz
+zump
+
# LINT: swallow here-doc (EOF is last line of test)
horticulture <<\EOF
gomez
diff --git a/t/chainlint/subshell-here-doc.expect
b/t/chainlint/subshell-here-doc.expect
index 7663ea7fc4..74723e7340 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -6,5 +6,6 @@
(
cat >bup &&
cat >bup2 &&
+ cat >bup3 &&
meep
>)
diff --git a/t/chainlint/subshell-here-doc.test
b/t/chainlint/subshell-here-doc.test
index b6b5a9b33a..f6b3ba4214 100644
--- a/t/chainlint/subshell-here-doc.test
+++ b/t/chainlint/subshell-here-doc.test
@@ -31,5 +31,9 @@
glink
FIZZ
ARBITRARY2
+ cat <<-"ARBITRARY3" >bup3 &&
+ glink
+ FIZZ
+ ARBITRARY3
meep
)
--
2.19.0.rc1.352.gb1634b371d
[PATCH v3 2/6] chainlint: match quoted here-doc tags
A here-doc tag can be quoted ('EOF'/"EOF") or escaped (\EOF) to suppress
interpolation within the body. Although, chainlint recognizes escaped
tags, it does not know about quoted tags. For completeness, teach it to
recognize quoted tags, as well.
Signed-off-by: Eric Sunshine
---
t/chainlint.sed | 8
t/chainlint/here-doc.expect | 4
t/chainlint/here-doc.test| 14 ++
t/chainlint/subshell-here-doc.expect | 2 ++
t/chainlint/subshell-here-doc.test | 8
5 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/t/chainlint.sed b/t/chainlint.sed
index 2af1a687f8..07c624fe09 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -94,8 +94,8 @@
# here-doc -- swallow it to avoid false hits within its body (but keep the
# command to which it was attached)
-/<<[ ]*[-\\]*[A-Za-z0-9_]/ {
- s/^\(.*\)<<[]*[-\\]*\([A-Za-z0-9_][A-Za-z0-9_]*\)/<\2>\1<\1<\1<\1<foo &&
+cat >bar &&
+
+cat >boo &&
+
horticulture
diff --git a/t/chainlint/here-doc.test b/t/chainlint/here-doc.test
index 8986eefe74..ad4ce8afd9 100644
--- a/t/chainlint/here-doc.test
+++ b/t/chainlint/here-doc.test
@@ -14,6 +14,20 @@ boz
woz
Arbitrary_Tag_42
+# LINT: swallow 'quoted' here-doc
+cat <<'FUMP' >bar &&
+snoz
+boz
+woz
+FUMP
+
+# LINT: swallow "quoted" here-doc
+cat <<"zump" >boo &&
+snoz
+boz
+woz
+zump
+
# LINT: swallow here-doc (EOF is last line of test)
horticulture <<\EOF
gomez
diff --git a/t/chainlint/subshell-here-doc.expect
b/t/chainlint/subshell-here-doc.expect
index 7c2da63bc7..74723e7340 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -5,5 +5,7 @@
>) &&
(
cat >bup &&
+ cat >bup2 &&
+ cat >bup3 &&
meep
>)
diff --git a/t/chainlint/subshell-here-doc.test
b/t/chainlint/subshell-here-doc.test
index 05139af0b5..f6b3ba4214 100644
--- a/t/chainlint/subshell-here-doc.test
+++ b/t/chainlint/subshell-here-doc.test
@@ -27,5 +27,13 @@
glink
FIZZ
ARBITRARY
+ cat <<-'ARBITRARY2' >bup2 &&
+ glink
+ FIZZ
+ ARBITRARY2
+ cat <<-"ARBITRARY3" >bup3 &&
+ glink
+ FIZZ
+ ARBITRARY3
meep
)
--
2.18.0.267.gbc8be36ecb
[PATCH v3 1/6] chainlint: match arbitrary here-docs tags rather than hard-coded names
chainlint.sed swallows top-level here-docs to avoid being fooled by
content which might look like start-of-subshell. It likewise swallows
here-docs in subshells to avoid marking content lines as breaking the
&&-chain, and to avoid being fooled by content which might look like
end-of-subshell, start-of-nested-subshell, or other specially-recognized
constructs.
At the time of implementation, it was believed that it was not possible
to support arbitrary here-doc tag names since 'sed' provides no way to
stash the opening tag name in a variable for later comparison against a
line signaling end-of-here-doc. Consequently, tag names are hard-coded,
with "EOF" being the only tag recognized at the top-level, and only
"EOF", "EOT", and "INPUT_END" being recognized within subshells. Also,
special care was taken to avoid being confused by here-docs nested
within other here-docs.
In practice, this limited number of hard-coded tag names has been "good
enough" for the 13000+ existing Git test, despite many of those tests
using tags other than the recognized ones, since the bodies of those
here-docs do not contain content which would fool the linter.
Nevertheless, the situation is not ideal since someone writing new
tests, and choosing a name not in the "blessed" set could potentially
trigger a false-positive.
To address this shortcoming, upgrade chainlint.sed to handle arbitrary
here-doc tag names, both at the top-level and within subshells.
Signed-off-by: Eric Sunshine
---
t/chainlint.sed | 57 +---
t/chainlint/here-doc.expect | 2 +
t/chainlint/here-doc.test| 7
t/chainlint/nested-here-doc.expect | 2 +
t/chainlint/nested-here-doc.test | 10 +
t/chainlint/subshell-here-doc.expect | 4 ++
t/chainlint/subshell-here-doc.test | 8
7 files changed, 67 insertions(+), 23 deletions(-)
diff --git a/t/chainlint.sed b/t/chainlint.sed
index 5f0882cb38..2af1a687f8 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -61,6 +61,22 @@
# "else", and "fi" in if-then-else likewise must not end with "&&", thus
# receives similar treatment.
#
+# Swallowing here-docs with arbitrary tags requires a bit of finesse. When a
+# line such as "cat <out" is seen, the here-doc tag is moved to the front
+# of the line enclosed in angle brackets as a sentinel, giving "cat >out".
+# As each subsequent line is read, it is appended to the target line and a
+# (whitespace-loose) back-reference match /^<(.*)>\n\1$/ is attempted to see if
+# the content inside "<...>" matches the entirety of the newly-read line. For
+# instance, if the next line read is "some data", when concatenated with the
+# target line, it becomes "cat >out\nsome data", and a match is attempted
+# to see if "EOF" matches "some data". Since it doesn't, the next line is
+# attempted. When a line consisting of only "EOF" (and possible whitespace) is
+# encountered, it is appended to the target line giving "cat >out\nEOF",
+# in which case the "EOF" inside "<...>" does match the text following the
+# newline, thus the closing here-doc tag has been found. The closing tag line
+# and the "<...>" prefix on the target line are then discarded, leaving just
+# the target line "cat >out".
+#
# To facilitate regression testing (and manual debugging), a ">" annotation is
# applied to the line containing ")" which closes a subshell, ">>" to a line
# closing a nested subshell, and ">>>" to a line closing both at once. This
@@ -78,14 +94,17 @@
# here-doc -- swallow it to avoid false hits within its body (but keep the
# command to which it was attached)
-/<<[ ]*[-\\]*EOF[]*/ {
- s/[ ]*<<[ ]*[-\\]*EOF//
- h
+/<<[ ]*[-\\]*[A-Za-z0-9_]/ {
+ s/^\(.*\)<<[]*[-\\]*\([A-Za-z0-9_][A-Za-z0-9_]*\)/<\2>\1<]*\)>.*\n[ ]*\1[ ]*$/!{
+ s/\n.*$//
+ bhereslurp
+ }
+ s/^<[^>]*>//
+ s/\n.*$//
}
# one-liner "(...) &&"
@@ -139,9 +158,7 @@ s/.*\n//
/"[^'"]*'[^'"]*"/!bsqstring
}
# here-doc -- swallow it
-/<<[ ]*[-\\]*EOF/bheredoc
-/<<[ ]*[-\\]*EOT/bheredoc
-/<<[ ]*[-\\]*INPUT_END/bheredoc
+/<<[ ]*[-\\]*[A-Za-z0-9_]/bheredoc
# comment or empty line -- discard since final non-comment, non-empty line
# before closing ")", "done", "elsif", "else", or "fi" will need to be
# re-visited to drop "suspect" marking since final line of those constructs
@@ -249,23 +266,17 @@ s/\n//
bcheckchain
# found here-doc -- swallow
Re: [PATCH v3 0/7] Prep for "git fetch" should not clobber existing tags without --force
On Mon, Aug 13 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason 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-39ohtch4xwsa6f...@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!
Re: [PATCH v3 0/7] Prep for "git fetch" should not clobber existing tags without --force
Ævar Arnfjörð Bjarmason 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(-)
Re: [PATCH v2 2/6] chainlint: match 'quoted' here-doc tags
On Mon, Aug 13, 2018 at 3:27 PM Junio C Hamano wrote:
> Eric Sunshine writes:
>
> > A here-doc tag can be quoted ('EOF') or escaped (\EOF) to suppress
> > interpolation within the body. Although, chainlint recognizes escaped
> > tags, it does not know about quoted tags. For completeness, teach it to
> > recognize quoted tags, as well.
>
> Is this step merely completeness and future-proofing, or do we have
> existing instances that the linter would be confused without this
> patch?
We do have quite a few instances of single-quoted 'EOF' in the tests
already, though none of them confuse the linter, so this change is for
completeness and future-proofing.
> I am primarily wondering what the reason is that <<"EOF" is
> not looked for while we are at it (the answer could be that we
> have tests that use <<'EOF' but not <<"EOF").
It's an oversight on my part. I've gotten so used to \EOF (and before
working on Git, 'EOF'), that "EOF" just slipped my mind[1].
I do think it's worth a re-roll. Thanks for noticing.
[1]: What did not slip my mind is that quoting _any_ part of the tag
suppresses interpolation, so 'E'OF, E'O'F, and even E''OF are all
valid, however, that's so ugly that I really didn't want to support
it.
Re: [PATCH v2 2/6] chainlint: match 'quoted' here-doc tags
Eric Sunshine writes:
> A here-doc tag can be quoted ('EOF') or escaped (\EOF) to suppress
> interpolation within the body. Although, chainlint recognizes escaped
> tags, it does not know about quoted tags. For completeness, teach it to
> recognize quoted tags, as well.
Is this step merely completeness and future-proofing, or do we have
existing instances that the linter would be confused without this
patch? I am primarily wondering what the reason is that <<"EOF" is
not looked for while we are at it (the answer could be that we
have tests that use <<'EOF' but not <<"EOF").
> Signed-off-by: Eric Sunshine
> ---
> t/chainlint.sed | 8
> t/chainlint/here-doc.expect | 2 ++
> t/chainlint/here-doc.test| 7 +++
> t/chainlint/subshell-here-doc.expect | 1 +
> t/chainlint/subshell-here-doc.test | 4
> 5 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/t/chainlint.sed b/t/chainlint.sed
> index 2af1a687f8..2901494e8a 100644
> --- a/t/chainlint.sed
> +++ b/t/chainlint.sed
> @@ -94,8 +94,8 @@
>
> # here-doc -- swallow it to avoid false hits within its body (but keep the
> # command to which it was attached)
> -/<<[ ]*[-\\]*[A-Za-z0-9_]/ {
> - s/^\(.*\)<<[]*[-\\]*\([A-Za-z0-9_][A-Za-z0-9_]*\)/<\2>\1< +/<<[ ]*[-\\']*[A-Za-z0-9_]/ {
> + s/^\(.*\)<<[]*[-\\']*\([A-Za-z0-9_][A-Za-z0-9_]*\)'*/<\2>\1< s/[ ]*< :hereslurp
> N
> @@ -158,7 +158,7 @@ s/.*\n//
> /"[^'"]*'[^'"]*"/!bsqstring
> }
> # here-doc -- swallow it
> -/<<[ ]*[-\\]*[A-Za-z0-9_]/bheredoc
> +/<<[ ]*[-\\']*[A-Za-z0-9_]/bheredoc
> # comment or empty line -- discard since final non-comment, non-empty line
> # before closing ")", "done", "elsif", "else", or "fi" will need to be
> # re-visited to drop "suspect" marking since final line of those constructs
> @@ -268,7 +268,7 @@ bcheckchain
> # found here-doc -- swallow it to avoid false hits within its body (but keep
> # the command to which it was attached)
> :heredoc
> -s/^\(.*\)<<[ ]*[-\\]*\([A-Za-z0-9_][A-Za-z0-9_]*\)/<\2>\1< +s/^\(.*\)<<[ ]*[-\\']*\([A-Za-z0-9_][A-Za-z0-9_]*\)'*/<\2>\1< s/[ ]*< :hereslurpsub
> N
> diff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect
> index 33bc3cc0b4..aff6568716 100644
> --- a/t/chainlint/here-doc.expect
> +++ b/t/chainlint/here-doc.expect
> @@ -2,4 +2,6 @@ boodle wobbagorgo snootwafta snurb &&
>
> cat >foo &&
>
> +cat >bar &&
> +
> horticulture
> diff --git a/t/chainlint/here-doc.test b/t/chainlint/here-doc.test
> index 8986eefe74..f2bb14b693 100644
> --- a/t/chainlint/here-doc.test
> +++ b/t/chainlint/here-doc.test
> @@ -14,6 +14,13 @@ boz
> woz
> Arbitrary_Tag_42
>
> +# LINT: swallow 'quoted' here-doc
> +cat <<'FUMP' >bar &&
> +snoz
> +boz
> +woz
> +FUMP
> +
> # LINT: swallow here-doc (EOF is last line of test)
> horticulture <<\EOF
> gomez
> diff --git a/t/chainlint/subshell-here-doc.expect
> b/t/chainlint/subshell-here-doc.expect
> index 7c2da63bc7..7663ea7fc4 100644
> --- a/t/chainlint/subshell-here-doc.expect
> +++ b/t/chainlint/subshell-here-doc.expect
> @@ -5,5 +5,6 @@
> >) &&
> (
> cat >bup &&
> + cat >bup2 &&
> meep
> >)
> diff --git a/t/chainlint/subshell-here-doc.test
> b/t/chainlint/subshell-here-doc.test
> index 05139af0b5..b6b5a9b33a 100644
> --- a/t/chainlint/subshell-here-doc.test
> +++ b/t/chainlint/subshell-here-doc.test
> @@ -27,5 +27,9 @@
> glink
> FIZZ
> ARBITRARY
> + cat <<-'ARBITRARY2' >bup2 &&
> + glink
> + FIZZ
> + ARBITRARY2
> meep
> )
[PATCH v3 5/7] push tests: assert re-pushing annotated tags
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
---
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 " 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 '
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
[PATCH v3 0/7] Prep for "git fetch" should not clobber existing tags without --force
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-ava...@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
[PATCH v2 2/6] chainlint: match 'quoted' here-doc tags
A here-doc tag can be quoted ('EOF') or escaped (\EOF) to suppress
interpolation within the body. Although, chainlint recognizes escaped
tags, it does not know about quoted tags. For completeness, teach it to
recognize quoted tags, as well.
Signed-off-by: Eric Sunshine
---
t/chainlint.sed | 8
t/chainlint/here-doc.expect | 2 ++
t/chainlint/here-doc.test| 7 +++
t/chainlint/subshell-here-doc.expect | 1 +
t/chainlint/subshell-here-doc.test | 4
5 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/t/chainlint.sed b/t/chainlint.sed
index 2af1a687f8..2901494e8a 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -94,8 +94,8 @@
# here-doc -- swallow it to avoid false hits within its body (but keep the
# command to which it was attached)
-/<<[ ]*[-\\]*[A-Za-z0-9_]/ {
- s/^\(.*\)<<[]*[-\\]*\([A-Za-z0-9_][A-Za-z0-9_]*\)/<\2>\1<\1<\1<\1<foo &&
+cat >bar &&
+
horticulture
diff --git a/t/chainlint/here-doc.test b/t/chainlint/here-doc.test
index 8986eefe74..f2bb14b693 100644
--- a/t/chainlint/here-doc.test
+++ b/t/chainlint/here-doc.test
@@ -14,6 +14,13 @@ boz
woz
Arbitrary_Tag_42
+# LINT: swallow 'quoted' here-doc
+cat <<'FUMP' >bar &&
+snoz
+boz
+woz
+FUMP
+
# LINT: swallow here-doc (EOF is last line of test)
horticulture <<\EOF
gomez
diff --git a/t/chainlint/subshell-here-doc.expect
b/t/chainlint/subshell-here-doc.expect
index 7c2da63bc7..7663ea7fc4 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -5,5 +5,6 @@
>) &&
(
cat >bup &&
+ cat >bup2 &&
meep
>)
diff --git a/t/chainlint/subshell-here-doc.test
b/t/chainlint/subshell-here-doc.test
index 05139af0b5..b6b5a9b33a 100644
--- a/t/chainlint/subshell-here-doc.test
+++ b/t/chainlint/subshell-here-doc.test
@@ -27,5 +27,9 @@
glink
FIZZ
ARBITRARY
+ cat <<-'ARBITRARY2' >bup2 &&
+ glink
+ FIZZ
+ ARBITRARY2
meep
)
--
2.18.0.267.gbc8be36ecb
[PATCH v2 1/6] chainlint: match arbitrary here-docs tags rather than hard-coded names
chainlint.sed swallows top-level here-docs to avoid being fooled by
content which might look like start-of-subshell. It likewise swallows
here-docs in subshells to avoid marking content lines as breaking the
&&-chain, and to avoid being fooled by content which might look like
end-of-subshell, start-of-nested-subshell, or other specially-recognized
constructs.
At the time of implementation, it was believed that it was not possible
to support arbitrary here-doc tag names since 'sed' provides no way to
stash the opening tag name in a variable for later comparison against a
line signaling end-of-here-doc. Consequently, tag names are hard-coded,
with "EOF" being the only tag recognized at the top-level, and only
"EOF", "EOT", and "INPUT_END" being recognized within subshells. Also,
special care was taken to avoid being confused by here-docs nested
within other here-docs.
In practice, this limited number of hard-coded tag names has been "good
enough" for the 13000+ existing Git test, despite many of those tests
using tags other than the recognized ones, since the bodies of those
here-docs do not contain content which would fool the linter.
Nevertheless, the situation is not ideal since someone writing new
tests, and choosing a name not in the "blessed" set could potentially
trigger a false-positive.
To address this shortcoming, upgrade chainlint.sed to handle arbitrary
here-doc tag names, both at the top-level and within subshells.
Signed-off-by: Eric Sunshine
---
t/chainlint.sed | 57 +---
t/chainlint/here-doc.expect | 2 +
t/chainlint/here-doc.test| 7
t/chainlint/nested-here-doc.expect | 2 +
t/chainlint/nested-here-doc.test | 10 +
t/chainlint/subshell-here-doc.expect | 4 ++
t/chainlint/subshell-here-doc.test | 8
7 files changed, 67 insertions(+), 23 deletions(-)
diff --git a/t/chainlint.sed b/t/chainlint.sed
index 5f0882cb38..2af1a687f8 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -61,6 +61,22 @@
# "else", and "fi" in if-then-else likewise must not end with "&&", thus
# receives similar treatment.
#
+# Swallowing here-docs with arbitrary tags requires a bit of finesse. When a
+# line such as "cat <out" is seen, the here-doc tag is moved to the front
+# of the line enclosed in angle brackets as a sentinel, giving "cat >out".
+# As each subsequent line is read, it is appended to the target line and a
+# (whitespace-loose) back-reference match /^<(.*)>\n\1$/ is attempted to see if
+# the content inside "<...>" matches the entirety of the newly-read line. For
+# instance, if the next line read is "some data", when concatenated with the
+# target line, it becomes "cat >out\nsome data", and a match is attempted
+# to see if "EOF" matches "some data". Since it doesn't, the next line is
+# attempted. When a line consisting of only "EOF" (and possible whitespace) is
+# encountered, it is appended to the target line giving "cat >out\nEOF",
+# in which case the "EOF" inside "<...>" does match the text following the
+# newline, thus the closing here-doc tag has been found. The closing tag line
+# and the "<...>" prefix on the target line are then discarded, leaving just
+# the target line "cat >out".
+#
# To facilitate regression testing (and manual debugging), a ">" annotation is
# applied to the line containing ")" which closes a subshell, ">>" to a line
# closing a nested subshell, and ">>>" to a line closing both at once. This
@@ -78,14 +94,17 @@
# here-doc -- swallow it to avoid false hits within its body (but keep the
# command to which it was attached)
-/<<[ ]*[-\\]*EOF[]*/ {
- s/[ ]*<<[ ]*[-\\]*EOF//
- h
+/<<[ ]*[-\\]*[A-Za-z0-9_]/ {
+ s/^\(.*\)<<[]*[-\\]*\([A-Za-z0-9_][A-Za-z0-9_]*\)/<\2>\1<]*\)>.*\n[ ]*\1[ ]*$/!{
+ s/\n.*$//
+ bhereslurp
+ }
+ s/^<[^>]*>//
+ s/\n.*$//
}
# one-liner "(...) &&"
@@ -139,9 +158,7 @@ s/.*\n//
/"[^'"]*'[^'"]*"/!bsqstring
}
# here-doc -- swallow it
-/<<[ ]*[-\\]*EOF/bheredoc
-/<<[ ]*[-\\]*EOT/bheredoc
-/<<[ ]*[-\\]*INPUT_END/bheredoc
+/<<[ ]*[-\\]*[A-Za-z0-9_]/bheredoc
# comment or empty line -- discard since final non-comment, non-empty line
# before closing ")", "done", "elsif", "else", or "fi" will need to be
# re-visited to drop "suspect" marking since final line of those constructs
@@ -249,23 +266,17 @@ s/\n//
bcheckchain
# found here-doc -- swallow
Re: [PATCH 1/5] chainlint: match arbitrary here-docs tags rather than hard-coded names
On Thu, Aug 09, 2018 at 01:58:05AM -0400, Eric Sunshine wrote: > On Wed, Aug 8, 2018 at 6:50 PM Jeff King wrote: > > On Tue, Aug 07, 2018 at 04:21:31AM -0400, Eric Sunshine wrote: > > > +# Swallowing here-docs with arbitrary tags requires a bit of finesse. > > > When a > > > +# line such as "cat <out" is seen, the here-doc tag is moved to > > > the front > > > +# of the line enclosed in angle brackets as a sentinel, giving "cat > > > >out". > > > > Gross, but OK, as long as we would not get confused by a line that > > actually started with at the start. > > It can't get confused by such a line. There here-doc swallower > prepends that when it starts the swallowing process and removes it add > the end. Even if a line actually started with that, it would become > "cmd" while swallowing the here-doc, and be restored to > "cmd" at the end. Stripping the "" is done non-greedily, so > it wouldn't remove both of them. Likewise, non-greedy matching is used > for pulling the "EOF" out of the "<...>" when trying to match against > the terminating "EOF" line, so there can be no confusion. Thanks. I figured you probably had thought of that, but it seemed easier to ask than to wade through the sed code (I do feel like a bad person to give that answer, because IMHO one of the key things that makes open source work is a willingness to dig in yourself rather than asking; but I am making an exception for this sed code). > Yeah, I was going with the tighter uppercase-only which Jonathan > suggested[1], but I guess it wouldn't hurt to re-roll to allow > lowercase too. > > [...] > > No. I've gotten so used to \EOF in this codebase that it didn't occur > to me to even think about 'EOF', but a re-roll could add that, as > well. Thanks. I could take or leave such fixes, since I think our style discourages both, so I'll leave it up to you whether you want to pursue them. -Peff
Re: [PATCH 1/5] chainlint: match arbitrary here-docs tags rather than hard-coded names
On Wed, Aug 8, 2018 at 6:50 PM Jeff King wrote:
> On Tue, Aug 07, 2018 at 04:21:31AM -0400, Eric Sunshine wrote:
> > +# Swallowing here-docs with arbitrary tags requires a bit of finesse. When
> > a
> > +# line such as "cat <out" is seen, the here-doc tag is moved to the
> > front
> > +# of the line enclosed in angle brackets as a sentinel, giving "cat
> > >out".
>
> Gross, but OK, as long as we would not get confused by a line that
> actually started with at the start.
It can't get confused by such a line. There here-doc swallower
prepends that when it starts the swallowing process and removes it add
the end. Even if a line actually started with that, it would become
"cmd" while swallowing the here-doc, and be restored to
"cmd" at the end. Stripping the "" is done non-greedily, so
it wouldn't remove both of them. Likewise, non-greedy matching is used
for pulling the "EOF" out of the "<...>" when trying to match against
the terminating "EOF" line, so there can be no confusion.
> > +/<<[ ]*[-\\]*[A-Z0-9_][A-Z0-9_]*/ {
> > + s/^\(.*\)<<[]*[-\\]*\([A-Z0-9_][A-Z0-9_]*\)/<\2>\1< > + s/[ ]*<
> Here-docs can use lowercase, too, though I'd personally frown on that
> from a style perspective.
Yeah, I was going with the tighter uppercase-only which Jonathan
suggested[1], but I guess it wouldn't hurt to re-roll to allow
lowercase too.
[1]:
https://public-inbox.org/git/20180730205914.ge156...@aiede.svl.corp.google.com/
> It looks like this doesn't catch:
>
> cat <<'EOF'
> EOF
>
> either. I think we prefer the backslash style, but there are quite a few
> <<-'EOF' hits. Is it covered somewhere else?
No. I've gotten so used to \EOF in this codebase that it didn't occur
to me to even think about 'EOF', but a re-roll could add that, as
well.
Re: [PATCH 1/5] chainlint: match arbitrary here-docs tags rather than hard-coded names
On Tue, Aug 07, 2018 at 04:21:31AM -0400, Eric Sunshine wrote:
> diff --git a/t/chainlint.sed b/t/chainlint.sed
> index 5f0882cb38..bd76c5d181 100644
> --- a/t/chainlint.sed
> +++ b/t/chainlint.sed
> @@ -61,6 +61,22 @@
> # "else", and "fi" in if-then-else likewise must not end with "&&", thus
> # receives similar treatment.
> #
> +# Swallowing here-docs with arbitrary tags requires a bit of finesse. When a
> +# line such as "cat <out" is seen, the here-doc tag is moved to the
> front
> +# of the line enclosed in angle brackets as a sentinel, giving "cat
> >out".
> +# As each subsequent line is read, it is appended to the target line and a
> +# (whitespace-loose) back-reference match /^<(.*)>\n\1$/ is attempted to see
> if
> +# the content inside "<...>" matches the entirety of the newly-read line. For
> +# instance, if the next line read is "some data", when concatenated with the
> +# target line, it becomes "cat >out\nsome data", and a match is
> attempted
> +# to see if "EOF" matches "some data". Since it doesn't, the next line is
> +# attempted. When a line consisting of only "EOF" (and possible whitespace)
> is
> +# encountered, it is appended to the target line giving "cat >out\nEOF",
> +# in which case the "EOF" inside "<...>" does match the text following the
> +# newline, thus the closing here-doc tag has been found. The closing tag line
> +# and the "<...>" prefix on the target line are then discarded, leaving just
> +# the target line "cat >out".
Gross, but OK, as long as we would not get confused by a line that
actually started with at the start.
> +/<<[ ]*[-\\]*[A-Z0-9_][A-Z0-9_]*/ {
> + s/^\(.*\)<<[]*[-\\]*\([A-Z0-9_][A-Z0-9_]*\)/<\2>\1< + s/[ ]*<
[PATCH 1/5] chainlint: match arbitrary here-docs tags rather than hard-coded names
chainlint.sed swallows top-level here-docs to avoid being fooled by
content which might look like start-of-subshell. It likewise swallows
here-docs in subshells to avoid marking content lines as breaking the
&&-chain, and to avoid being fooled by content which might look like
end-of-subshell, start-of-nested-subshell, or other specially-recognized
constructs.
At the time of implementation, it was believed that it was not possible
to support arbitrary here-doc tag names since 'sed' provides no way to
stash the opening tag name in a variable for later comparison against a
line signaling end-of-here-doc. Consequently, tag names are hard-coded,
with "EOF" being the only tag recognized at the top-level, and only
"EOF", "EOT", and "INPUT_END" being recognized within subshells. Also,
special care was taken to avoid being confused by here-docs nested
within other here-docs.
In practice, this limited number of hard-coded tag names has been "good
enough" for the 13000+ existing Git test, despite many of those tests
using tags other than the recognized ones, since the bodies of those
here-docs do not contain content which would fool the linter.
Nevertheless, the situation is not ideal since someone writing new
tests, and choosing a name not in the "blessed" set could potentially
trigger a false-positive.
To address this shortcoming, upgrade chainlint.sed to handle arbitrary
here-doc tag names, both at the top-level and within subshells.
Signed-off-by: Eric Sunshine
---
t/chainlint.sed | 57 +---
t/chainlint/here-doc.expect | 2 +
t/chainlint/here-doc.test| 7
t/chainlint/nested-here-doc.expect | 2 +
t/chainlint/nested-here-doc.test | 10 +
t/chainlint/subshell-here-doc.expect | 4 ++
t/chainlint/subshell-here-doc.test | 8
7 files changed, 67 insertions(+), 23 deletions(-)
diff --git a/t/chainlint.sed b/t/chainlint.sed
index 5f0882cb38..bd76c5d181 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -61,6 +61,22 @@
# "else", and "fi" in if-then-else likewise must not end with "&&", thus
# receives similar treatment.
#
+# Swallowing here-docs with arbitrary tags requires a bit of finesse. When a
+# line such as "cat <out" is seen, the here-doc tag is moved to the front
+# of the line enclosed in angle brackets as a sentinel, giving "cat >out".
+# As each subsequent line is read, it is appended to the target line and a
+# (whitespace-loose) back-reference match /^<(.*)>\n\1$/ is attempted to see if
+# the content inside "<...>" matches the entirety of the newly-read line. For
+# instance, if the next line read is "some data", when concatenated with the
+# target line, it becomes "cat >out\nsome data", and a match is attempted
+# to see if "EOF" matches "some data". Since it doesn't, the next line is
+# attempted. When a line consisting of only "EOF" (and possible whitespace) is
+# encountered, it is appended to the target line giving "cat >out\nEOF",
+# in which case the "EOF" inside "<...>" does match the text following the
+# newline, thus the closing here-doc tag has been found. The closing tag line
+# and the "<...>" prefix on the target line are then discarded, leaving just
+# the target line "cat >out".
+#
# To facilitate regression testing (and manual debugging), a ">" annotation is
# applied to the line containing ")" which closes a subshell, ">>" to a line
# closing a nested subshell, and ">>>" to a line closing both at once. This
@@ -78,14 +94,17 @@
# here-doc -- swallow it to avoid false hits within its body (but keep the
# command to which it was attached)
-/<<[ ]*[-\\]*EOF[]*/ {
- s/[ ]*<<[ ]*[-\\]*EOF//
- h
+/<<[ ]*[-\\]*[A-Z0-9_][A-Z0-9_]*/ {
+ s/^\(.*\)<<[]*[-\\]*\([A-Z0-9_][A-Z0-9_]*\)/<\2>\1<]*\)>.*\n[ ]*\1[ ]*$/!{
+ s/\n.*$//
+ bhereslurp
+ }
+ s/^<[^>]*>//
+ s/\n.*$//
}
# one-liner "(...) &&"
@@ -139,9 +158,7 @@ s/.*\n//
/"[^'"]*'[^'"]*"/!bsqstring
}
# here-doc -- swallow it
-/<<[ ]*[-\\]*EOF/bheredoc
-/<<[ ]*[-\\]*EOT/bheredoc
-/<<[ ]*[-\\]*INPUT_END/bheredoc
+/<<[ ]*[-\\]*[A-Z0-9_][A-Z0-9_]*/bheredoc
# comment or empty line -- discard since final non-comment, non-empty line
# before closing ")", "done", "elsif", "else", or "fi" will need to be
# re-visited to drop "suspect" marking since final line of those constructs
@@ -249,23 +266,17 @@ s/\n//
bcheckchain
# found here-doc -- sw
Re: [PATCH v2 10/10] fetch: stop clobbering existing tags without --force
Ævar Arnfjörð Bjarmason writes: > diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt > index 97d3217df9..5b624caf58 100644 > --- a/Documentation/fetch-options.txt > +++ b/Documentation/fetch-options.txt > @@ -49,11 +49,16 @@ endif::git-pull[] > > -f:: > --force:: > - When 'git fetch' is used with `:` > - refspec, it refuses to update the local branch > - `` unless the remote branch `` it > - fetches is a descendant of ``. This option > - overrides that check. > + When 'git fetch' is used with `:` refspec it may > + refuse to update the local branch as discussed > +ifdef::git-pull[] > + in the `` part of the linkgit:git-fetch[1] > + documentation. > +endif::git-pull[] > +ifndef::git-pull[] > + in the `` part below. > +endif::git-pull[] > + This option overrides that check. Ah, that's tricky. I could not locate "the `` part" in Documentation/git-fetch.txt and was scratching my head, but it comes from pull-fetch-param.txt by inclusion ;-) > diff --git a/Documentation/pull-fetch-param.txt > b/Documentation/pull-fetch-param.txt > index f1fb08dc68..acb8e1a4f0 100644 > --- a/Documentation/pull-fetch-param.txt > +++ b/Documentation/pull-fetch-param.txt > @@ -33,11 +33,21 @@ name. > it requests fetching everything up to the given tag. > + > The remote ref that matches > -is fetched, and if is not an empty string, the local > -ref that matches it is fast-forwarded using . > -If the optional plus `+` is used, the local ref > -is updated even if it does not result in a fast-forward > -update. > +is fetched, and if is not an empty string, an attempt > +is made to update the local ref that matches it. > ++ > +Whether that update is allowed without `--force` depends on the ref > +namespace it's being fetched to, and the type of object being > +fetched. If it's a commit under `refs/heads/*` only fast-forwards are > +allowed. > ++ > +By having the optional leading `+` to a refspec (or using `--force` > +command line option) you can tell Git to update the local ref even if > +it is not allowed by its respective namespace clobbering rules. The above two paragraphs imply that I can "fetch +blob:refs/heads/master" to cause havoc locally? > +Before Git version 2.19 tag objects under `refs/tags/*` would not be > +protected from updates, but since then the `+` (or `--force`) syntax > +is required to clobber them. I think that is a good change; it belongs more to the b/c notes in the release notes; while I do not think it is wrong describe "it used to be that way" just after a drastic change in the immediate past, we shouldn't carry that forever, so perhaps we can leave a "NEEDSWORK: remove the 'it used to be this way' in 2020" comment around here?
[PATCH v2 00/10] "git fetch" should not clobber existing tags without --force
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-ava...@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
-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 "
+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
@@ -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
[PATCH v2 10/10] fetch: stop clobbering existing tags without --force
Change "fetch" to treat "+" in refspecs (aka --force) to mean we
should clobber a local tag of the same name.
This changes the long-standing behavior of "fetch" added in
853a3697dc ("[PATCH] Multi-head fetch.", 2005-08-20), before this
change all tag fetches effectively had --force enabled. See the
git-fetch-script code in fast_forward_local() with the comment:
> Tags need not be pointing at commits so there is no way to
> guarantee "fast-forward" anyway.
That commit and the rest of the history of "fetch" shows that the
"+" (--force) part of refpecs was only conceived for branch updates,
while tags have accepted any changes from upstream unconditionally and
clobbered the local tag object. Changing this behavior has been
discussed as early as 2011[1].
The current behavior doesn't make sense to me, it easily results in
local tags accidentally being clobbered. We could namespace our tags
per-remote and not locally populate refs/tags/*, but as with my
97716d217c ("fetch: add a --prune-tags option and fetch.pruneTags
config", 2018-02-09) it's easier to work around the current
implementation than to fix the root cause.
So this change implements suggestion #1 from Jeff's 2011 E-Mail[1],
"fetch" now only clobbers the tag if either "+" is provided as part of
the refspec, or if "--force" is provided on the command-line.
This also makes it nicely symmetrical with how "tag" itself works when
creating tags. I.e. we refuse to clobber any existing tags unless
"--force" is supplied. Now we can refuse all such clobbering, whether
it would happen by clobbering a local tag with "tag", or by fetching
it from the remote with "fetch".
It's still not at all nicely symmetrical with how "git push" works, as
discussed in the updated pull-fetch-param.txt documentation, but this
change brings them more into line with one another. I don't think
there's any reason "fetch" couldn't fully converge with the behavior
used by "push", but that's a topic for another change.
One of the tests added in 31b808a032 ("clone --single: limit the fetch
refspec to fetched branch", 2012-09-20) is being changed to use
--force where a clone would clobber a tag. This changes nothing about
the existing behavior of the test.
1. https://public-inbox.org/git/2023221658.ga22...@sigill.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/fetch-options.txt| 15 ++-
Documentation/pull-fetch-param.txt | 20 +++-
builtin/fetch.c| 20 +---
t/t5516-fetch-push.sh | 5 +++--
t/t5612-clone-refspec.sh | 4 ++--
5 files changed, 43 insertions(+), 21 deletions(-)
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 97d3217df9..5b624caf58 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -49,11 +49,16 @@ endif::git-pull[]
-f::
--force::
- When 'git fetch' is used with `:`
- refspec, it refuses to update the local branch
- `` unless the remote branch `` it
- fetches is a descendant of ``. This option
- overrides that check.
+ When 'git fetch' is used with `:` refspec it may
+ refuse to update the local branch as discussed
+ifdef::git-pull[]
+ in the `` part of the linkgit:git-fetch[1]
+ documentation.
+endif::git-pull[]
+ifndef::git-pull[]
+ in the `` part below.
+endif::git-pull[]
+ This option overrides that check.
-k::
--keep::
diff --git a/Documentation/pull-fetch-param.txt
b/Documentation/pull-fetch-param.txt
index f1fb08dc68..acb8e1a4f0 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -33,11 +33,21 @@ name.
it requests fetching everything up to the given tag.
+
The remote ref that matches
-is fetched, and if is not an empty string, the local
-ref that matches it is fast-forwarded using .
-If the optional plus `+` is used, the local ref
-is updated even if it does not result in a fast-forward
-update.
+is fetched, and if is not an empty string, an attempt
+is made to update the local ref that matches it.
++
+Whether that update is allowed without `--force` depends on the ref
+namespace it's being fetched to, and the type of object being
+fetched. If it's a commit under `refs/heads/*` only fast-forwards are
+allowed.
++
+By having the optional leading `+` to a refspec (or using `--force`
+command line option) you can tell Git to update the local ref even if
+it is not allowed by its respective namespace clobbering rules.
++
+Before Git version 2.19 tag objects under `refs/tags/*` would not be
+protected from updates, but since then the `+` (or `--force`) syntax
+is required to clobber them.
+
[NOTE]
When the remote branch you want to fetch is kn
[PATCH v2 05/10] push tests: assert re-pushing annotated tags
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
---
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 " 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 '
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
[PATCH 1/2] revision: tolerate promised targets of tags
In handle_commit(), it is fatal for an annotated tag to point to a
non-existent object. --exclude-promisor-objects should relax this rule
and allow non-existent objects that are promisor objects, but this is
not the case. Update handle_commit() to tolerate this situation.
This was observed when cloning from a repository with an annotated tag
pointing to a blob. The test included in this patch demonstrates this
case.
Signed-off-by: Jonathan Tan
---
---
revision.c | 3 +++
t/t5616-partial-clone.sh | 39 +++
2 files changed, 42 insertions(+)
diff --git a/revision.c b/revision.c
index 1b37da988..95546e6d4 100644
--- a/revision.c
+++ b/revision.c
@@ -248,6 +248,9 @@ static struct commit *handle_commit(struct rev_info *revs,
if (!object) {
if (revs->ignore_missing_links || (flags &
UNINTERESTING))
return NULL;
+ if (revs->exclude_promisor_objects &&
+ is_promisor_object(>tagged->oid))
+ return NULL;
die("bad object %s", oid_to_hex(>tagged->oid));
}
object->flags |= flags;
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 44d8e8017..e8dfeafe7 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -216,6 +216,45 @@ test_expect_success 'upon cloning, check that all refs
point to objects' '
! test -e "$HTTPD_ROOT_PATH/one-time-sed"
'
+test_expect_success 'when partial cloning, tolerate server not sending target
of tag' '
+ SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
+ rm -rf "$SERVER" repo &&
+ test_create_repo "$SERVER" &&
+ test_commit -C "$SERVER" foo &&
+ test_config -C "$SERVER" uploadpack.allowfilter 1 &&
+ test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
+
+ # Create an annotated tag pointing to a blob.
+ BLOB=$(echo blob-contents | git -C "$SERVER" hash-object --stdin -w) &&
+ git -C "$SERVER" tag -m message -a myblob "$BLOB" &&
+
+ # Craft a packfile including the tag, but not the blob it points to.
+ printf "%s\n%s\n--not\n%s\n" \
+ $(git -C "$SERVER" rev-parse HEAD) \
+ $(git -C "$SERVER" rev-parse myblob) \
+ $(git -C "$SERVER" rev-parse myblob^{blob}) |
+ git -C "$SERVER" pack-objects --thin --stdout >incomplete.pack
&&
+
+ # Replace the existing packfile with the crafted one. The protocol
+ # requires that the packfile be sent in sideband 1, hence the extra
+ # \x01 byte at the beginning.
+ printf "1,/packfile/!c %04xx01%s" \
+ "$(($(wc -c "$HTTPD_ROOT_PATH/one-time-sed" &&
+
+ # Use protocol v2 because the sed command looks for the "packfile"
+ # section header.
+ test_config -C "$SERVER" protocol.version 2 &&
+
+ # Exercise to make sure it works.
+ git -c protocol.version=2 clone \
+ --filter=blob:none $HTTPD_URL/one_time_sed/server repo &&
+
+ # Ensure that the one-time-sed script was used.
+ ! test -e "$HTTPD_ROOT_PATH/one-time-sed"
+'
+
stop_httpd
test_done
--
2.18.0.203.gfac676dfb9-goog
[PATCH 0/2] Annotated tags pointing to missing but promised blobs
These are based on jt/partial-clone-fsck-connectivity. The patches in jt/partial-clone-fsck-connectivity were motivated by bugs I discovered in partial clones when refs pointed to blobs directly. While continuing to work on this, I noticed issues with annotated tags - that is, refs pointing to tags pointing to blobs. Here are some fixes. Jonathan Tan (2): revision: tolerate promised targets of tags tag: don't warn if target is missing but promised revision.c | 3 +++ t/t5616-partial-clone.sh | 44 tag.c| 13 +--- 3 files changed, 57 insertions(+), 3 deletions(-) -- 2.18.0.203.gfac676dfb9-goog
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
Brandon Williams writes: > On 07/09, Junio C Hamano wrote: >> Brandon Williams writes: >> >> > I agree with this observation, though I'm a bit sad about it. I think >> > that having tag auto-following the default is a little confusing (and >> > hurts perf[1] when using proto v2) but since thats the way its always been >> > we'll have to live with it for now. I think exploring changing the >> > defaults might be a good thing to do in the future. But for now we've >> > had enough people comment on this lacking functionality that we should >> > fix it. >> > >> > [1] Thankfully it doesn't completely undo what protocol v2 did, as we >> > still are able to eliminate refs/changes or refs/pull or other various >> > refs which significantly add to the number of refs advertised during >> > fetch. >> >> I thought JTan's <20180618231642.174650-1-jonathanta...@google.com> >> showed us a way forward to reduce the overhead even further without >> having to be sad ;-). Am I mistaken? > > That's true, what Jonathan mentioned there would avoid having to send > "refs/tags/*" when requesting the refs. The question is do we wait on > implementing that functionality (as another feature to fetch) or do we > fix this now? It's not like the earlier v2 protocol used to be super efficient and correct, whose performance benefit is destroyed with this "fix" irreversibly. It was a fast but sometimes incorrect implementation, and we'd protect users of the still-under-development feature with these two patches while updating the protocol further in order to become truly efficient and correct, so I do not see it a wrong move to take a hit like this patch does in the meantime. What I would see a wrong move would be to leave it very long as-is after we apply this fix, and declare to flip the default not to follow tags, using the performance hit as an excuse. So I do not care too deeply either way, whether we wait to regain efficiency or taking this safe fix as the first step, as long as it is fixed in the longer term. I had an impression that the sentiment in the thread was that it was OK to accept the inefficiency for now and fix it later, and I am personally fine with that approach.
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On 07/09, Junio C Hamano wrote: > Brandon Williams writes: > > > I agree with this observation, though I'm a bit sad about it. I think > > that having tag auto-following the default is a little confusing (and > > hurts perf[1] when using proto v2) but since thats the way its always been > > we'll have to live with it for now. I think exploring changing the > > defaults might be a good thing to do in the future. But for now we've > > had enough people comment on this lacking functionality that we should > > fix it. > > > > [1] Thankfully it doesn't completely undo what protocol v2 did, as we > > still are able to eliminate refs/changes or refs/pull or other various > > refs which significantly add to the number of refs advertised during > > fetch. > > I thought JTan's <20180618231642.174650-1-jonathanta...@google.com> > showed us a way forward to reduce the overhead even further without > having to be sad ;-). Am I mistaken? That's true, what Jonathan mentioned there would avoid having to send "refs/tags/*" when requesting the refs. The question is do we wait on implementing that functionality (as another feature to fetch) or do we fix this now? -- Brandon Williams
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
Brandon Williams writes: > I agree with this observation, though I'm a bit sad about it. I think > that having tag auto-following the default is a little confusing (and > hurts perf[1] when using proto v2) but since thats the way its always been > we'll have to live with it for now. I think exploring changing the > defaults might be a good thing to do in the future. But for now we've > had enough people comment on this lacking functionality that we should > fix it. > > [1] Thankfully it doesn't completely undo what protocol v2 did, as we > still are able to eliminate refs/changes or refs/pull or other various > refs which significantly add to the number of refs advertised during > fetch. I thought JTan's <20180618231642.174650-1-jonathanta...@google.com> showed us a way forward to reduce the overhead even further without having to be sad ;-). Am I mistaken?
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On 07/09, Jonathan Nieder wrote:
> Hi,
>
> Jonathan Tan wrote:
>
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> > *transport,
> > refspec_ref_prefixes(>remote->fetch, _prefixes);
> >
> > if (ref_prefixes.argc &&
> > - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> > + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
>
> Makes a lot of sense.
>
> This means that if I run
>
> git fetch origin master
>
> then the ls-refs step will now include refs/tags/* in addition to
> refs/heads/master, erasing some of the gain that protocol v2 brought.
> But since the user didn't pass --no-tags, that is what the user asked
> for.
>
> One could argue that the user didn't *mean* to ask for that. In other
> words, I wonder if the following would better match the user intent:
>
> - introduce a new tagopt value, --tags=auto or something, that means
>that tags are only followed when no refspec is supplied. In other
>words,
>
> git fetch --tags=auto
>
>would perform tag auto-following, while
>
> git fetch --tags=auto
>
>would act like fetch --no-tags.
>
> - make --tags=auto the default for new clones.
>
> What do you think?
>
> Some related thoughts on tag auto-following:
>
> It would be tempting to not use tag auto-following at all in the
> default configuration and just include refs/tags/*:refs/tags/* in the
> default clone refspec. Unfortunately, that would be a pretty
> significant behavior change from the present behavior, since
>
> - it would fetch tags pointing to objects unrelated to the fetched
>history
> - if we have ref-in-want *with* pattern support, it would mean we
>try to fetch all tags on every fetch. As discussed in the
>thread [1], this is expensive and difficult to get right.
> - if an unannotated tag is fast-forwarded, it would allow existing
>tags to be updated
> - it interacts poorly with --prune
> - ...
>
> I actually suspect it's a good direction in the long term, but until
> we address those downsides (see also the occasional discussions on tag
> namespaces), we're not ready for it. The --tags=auto described above
> is a sort of half approximation.
>
> Anyway, this is all a long way of saying that despite the performance
> regression I believe this patch heads in the right direction. The
> performance regression is inevitable in what the user is
> (unintentionally) requesting, and we can address it separately by
> changing the defaults to better match what the user intends to
> request.
I agree with this observation, though I'm a bit sad about it. I think
that having tag auto-following the default is a little confusing (and
hurts perf[1] when using proto v2) but since thats the way its always been
we'll have to live with it for now. I think exploring changing the
defaults might be a good thing to do in the future. But for now we've
had enough people comment on this lacking functionality that we should
fix it.
[1] Thankfully it doesn't completely undo what protocol v2 did, as we
still are able to eliminate refs/changes or refs/pull or other various
refs which significantly add to the number of refs advertised during
fetch.
--
Brandon Williams
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
Hi,
Jonathan Tan wrote:
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> *transport,
> refspec_ref_prefixes(>remote->fetch, _prefixes);
>
> if (ref_prefixes.argc &&
> - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
Makes a lot of sense.
This means that if I run
git fetch origin master
then the ls-refs step will now include refs/tags/* in addition to
refs/heads/master, erasing some of the gain that protocol v2 brought.
But since the user didn't pass --no-tags, that is what the user asked
for.
One could argue that the user didn't *mean* to ask for that. In other
words, I wonder if the following would better match the user intent:
- introduce a new tagopt value, --tags=auto or something, that means
that tags are only followed when no refspec is supplied. In other
words,
git fetch --tags=auto
would perform tag auto-following, while
git fetch --tags=auto
would act like fetch --no-tags.
- make --tags=auto the default for new clones.
What do you think?
Some related thoughts on tag auto-following:
It would be tempting to not use tag auto-following at all in the
default configuration and just include refs/tags/*:refs/tags/* in the
default clone refspec. Unfortunately, that would be a pretty
significant behavior change from the present behavior, since
- it would fetch tags pointing to objects unrelated to the fetched
history
- if we have ref-in-want *with* pattern support, it would mean we
try to fetch all tags on every fetch. As discussed in the
thread [1], this is expensive and difficult to get right.
- if an unannotated tag is fast-forwarded, it would allow existing
tags to be updated
- it interacts poorly with --prune
- ...
I actually suspect it's a good direction in the long term, but until
we address those downsides (see also the occasional discussions on tag
namespaces), we're not ready for it. The --tags=auto described above
is a sort of half approximation.
Anyway, this is all a long way of saying that despite the performance
regression I believe this patch heads in the right direction. The
performance regression is inevitable in what the user is
(unintentionally) requesting, and we can address it separately by
changing the defaults to better match what the user intends to
request.
Thanks,
Jonathan
[1] https://public-inbox.org/git/20180605175144.4225-1-bmw...@google.com/
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
> Jonathan Tan writes: > > >> Wouldn't that allow us not having to advertise the whole tags > >> namespace only to implement the tag following? > > > > Yes, it would, but as far as I can tell, it would add an extra burden on > > the server to walk all refs requested in the ls-refs call (in order to > > determine which tags to send back in the response). Also, this walk must > > be done before any negotiation (since this is a ls-refs call),... > > My comment was that I doubt the "must be done" part of the above. > How would refs-in-want be responded where client-supplied "I want > 'master' branch---I am not asking for the exact object the first > server I contacted said where the 'master' is at" gets turned into > "So the final value of 'master' among these servers that are not > quite in sync is this" by the one that gives you the pack, not > necessarily the one that responds to ls-refs upon initial contact? > Can't we do something similar, i.e. let the client say "I want tags > that refer to new objects you are going to send me, I do not know > what they are offhand" and the server that actually gives you the > pack to say "here are the tags I ended up including"? The > "include-tag" process to generate pack with extra objects (i.e. the > tags that point at packed objects) has to involve walking for > reachabliity anyway, so as long as the feature is supported, > somebody has to do the work, and if you want to cut down the > transfer cost of the refs/tags/* enumeration, it needs to happen on > the server end, no? Ah, I think I see. There are these possible worlds: (1) the current world (2) no ref-in-want, and upload-pack sends tag information as part of its response to ls-refs (3) no ref-in-want, but upload-pack can send ref information right before the packfile (4) ref-in-want, and upload-pack will send ref information right before the packfile I was only thinking about (2) and (4), but I think you are talking about (3). Yes, that would work, although I don't think it's worth the protocol churn to do (3) then (4), especially since we already have ref-in-want patches sent to the mailing list - but I should have discussed this option in my previous e-mails too. > Or perhaps v2 fetch should implement the automated tag following > without using include-tag and instead as an extended feature of > ref-in-want. I think that is merely giving a different name to the > same idea outlined above, though ;-) Instead of not using include-tag, I would define include-tag in the presence of want-refs to also include the refs, but I agree with this solution.
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
Jonathan Tan writes: >> Wouldn't that allow us not having to advertise the whole tags >> namespace only to implement the tag following? > > Yes, it would, but as far as I can tell, it would add an extra burden on > the server to walk all refs requested in the ls-refs call (in order to > determine which tags to send back in the response). Also, this walk must > be done before any negotiation (since this is a ls-refs call),... My comment was that I doubt the "must be done" part of the above. How would refs-in-want be responded where client-supplied "I want 'master' branch---I am not asking for the exact object the first server I contacted said where the 'master' is at" gets turned into "So the final value of 'master' among these servers that are not quite in sync is this" by the one that gives you the pack, not necessarily the one that responds to ls-refs upon initial contact? Can't we do something similar, i.e. let the client say "I want tags that refer to new objects you are going to send me, I do not know what they are offhand" and the server that actually gives you the pack to say "here are the tags I ended up including"? The "include-tag" process to generate pack with extra objects (i.e. the tags that point at packed objects) has to involve walking for reachabliity anyway, so as long as the feature is supported, somebody has to do the work, and if you want to cut down the transfer cost of the refs/tags/* enumeration, it needs to happen on the server end, no? Or perhaps v2 fetch should implement the automated tag following without using include-tag and instead as an extended feature of ref-in-want. I think that is merely giving a different name to the same idea outlined above, though ;-)
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
> Jonathan Tan writes: > > > When performing tag following, in addition to using the server's > > "include-tag" capability to send tag objects (and emulating it if the > > server does not support that capability), "git fetch" relies upon the > > presence of refs/tags/* entries in the initial ref advertisement to > > locally create refs pointing to the aforementioned tag objects. When > > using protocol v2, refs/tags/* entries in the initial ref advertisement > > may be suppressed by a ref-prefix argument, leading to the tag object > > being downloaded, but the ref not being created. > > I wonder if it is reasonable to define the protocol v2 semantics of > "include-tag" request a bit differently. > > Unlike v0 protocol where the client iterates though the advertised > refs and see if objects at the tip of them, even if they weren't > what the client initially wanted to fetch, to find these unsolicited > followed tag objects, allow the server to say, "I've included some > objects you did not explicitly ask for, and here are the refs/tags/* > names you should place on them", somewhat similar to the way how the > ref-in-want thing would work (i.e. the client does not really ask > for just objects and decides what name to place on them---instead it > lets the server to make part of the decision). > > Wouldn't that allow us not having to advertise the whole tags > namespace only to implement the tag following? Yes, it would, but as far as I can tell, it would add an extra burden on the server to walk all refs requested in the ls-refs call (in order to determine which tags to send back in the response). Also, this walk must be done before any negotiation (since this is a ls-refs call), so the walk is done all the way to the commits without any parents (and so an overestimate of tags might be sent). It might be better to have this functionality with ref-in-want, not only because of its conceptual similarity, but because ref-in-want provides a way for us to send refs at packfile generation time. So it is more efficient (since the server has to iterate through all the refs anyway to include objects for include-tag, it can probably just remember which refs caused objects to be added and return them), and also, the list of tags will not include any tags that point to non-sent objects. So my current inclination is to let v2 "include-tag" have the same semantics as v0 "include-tag", and only provide automatic sending of the ref itself when we have ref-in-want.
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
Jonathan Tan writes: > When performing tag following, in addition to using the server's > "include-tag" capability to send tag objects (and emulating it if the > server does not support that capability), "git fetch" relies upon the > presence of refs/tags/* entries in the initial ref advertisement to > locally create refs pointing to the aforementioned tag objects. When > using protocol v2, refs/tags/* entries in the initial ref advertisement > may be suppressed by a ref-prefix argument, leading to the tag object > being downloaded, but the ref not being created. I wonder if it is reasonable to define the protocol v2 semantics of "include-tag" request a bit differently. Unlike v0 protocol where the client iterates though the advertised refs and see if objects at the tip of them, even if they weren't what the client initially wanted to fetch, to find these unsolicited followed tag objects, allow the server to say, "I've included some objects you did not explicitly ask for, and here are the refs/tags/* names you should place on them", somewhat similar to the way how the ref-in-want thing would work (i.e. the client does not really ask for just objects and decides what name to place on them---instead it lets the server to make part of the decision). Wouldn't that allow us not having to advertise the whole tags namespace only to implement the tag following?
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On 06/18, Jonathan Tan wrote:
>
> This would cause different behavior. For example, if I run "git fetch
> refs/heads/master refs/tags/foo", I would expect tag following to work
> even if the tag's name does not start with refs/tags/foo.
I understand that some people *may* want tag following, but really its
confusing from an end user's standpoint. You can fetch a particular ref
and end up with a bunch of tags that you didn't want. Aside from that
my biggest issue is with performance. The ref filtering was added to
cut down on the number of refs sent from the server, now we're basically
adding them all back no matter what (unless a user goes and flips the
default to not fetch tags).
I think we're probably going to need some people other than us to
comment on how this should be handled.
>
> > > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> > > *transport,
> > > refspec_ref_prefixes(>remote->fetch,
> > > _prefixes);
> > >
> > > if (ref_prefixes.argc &&
> > > - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> > > + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
> >
> > Oh, I see. This always asks for refs/tags/ whereas before we only
> > asked for them if there were *no* refspec given. Maybe we can
> > change this to
> >
> > refspec_any_item_contains("refs/tags/")
> >
> > instead of always asking for the tags?
> > (that function would need to be written; I guess for a short term bugfix
> > this is good enough)
>
> Same answer as above.
>
> > How is the tag following documented (i.e. when is the user least
> > surprised that we do not tag-follow all and unconditionally)?
>
> It's documented under the --no-tags option in the man page for git
> fetch. I'm not sure what you mean by the user being surprised.
--
Brandon Williams
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
> okay. Thinking long term, we may want to introduce a capability that
> can filter the tag space, e.g. "want-refs-since refs/tags/*"
> as a client directive as then the client only asks for new (newly
> created/appearing) tags instead of all tags.
I don't think refs normally have a created/modified time.
> > since tag refs are sent by
> > default now, the test has been switched to using branch refs instead.
>
> That mismatches what I would expect to read below.
> I would expect the client to always ask for refs/tags/* now and
> instead of the server just giving them.
I don't understand why there is a mismatch, so I'll just answer all your
questions. Yes, the client indeed always asks for refs/tags/* now.
By the server just giving them, do you mean (1) giving them based on
what is being fetched, or (2) giving them as if refs/tags/* was sent? If
(1), this is possible, but requires the server to precompute all the
commits that would be sent (and this would be an overestimate, since
negotiation has not yet taken place). If (2), I think it's better to let
the client control this (since the client could just as easily want
--no-tags, then sending tags would be wasted).
> Oh, the problem is in that other test to restrict the refs/tags
> to *not* be sent?
The problem is that refs/tags/* is sent by default. Maybe adding
--no-tags would work (I haven't tried it, though), but I think it's
clearer for the test to just operate on branches, than to have --no-tags
and let the reader wonder what --no-tags has to do with what's being
tested.
> Maybe we can only ask for refs/tags/* if we do not have any
> "refs/tags/" on the CLI: if I invoke "git fetch refs/tags/v1"
> I would not get an advertisement for refs/tags/v2 but if I omit
> all tags from the refspec, I'd get all its advertisements (v1+v2)
This would cause different behavior. For example, if I run "git fetch
refs/heads/master refs/tags/foo", I would expect tag following to work
even if the tag's name does not start with refs/tags/foo.
> > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> > *transport,
> > refspec_ref_prefixes(>remote->fetch,
> > _prefixes);
> >
> > if (ref_prefixes.argc &&
> > - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> > + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
>
> Oh, I see. This always asks for refs/tags/ whereas before we only
> asked for them if there were *no* refspec given. Maybe we can
> change this to
>
> refspec_any_item_contains("refs/tags/")
>
> instead of always asking for the tags?
> (that function would need to be written; I guess for a short term bugfix
> this is good enough)
Same answer as above.
> How is the tag following documented (i.e. when is the user least
> surprised that we do not tag-follow all and unconditionally)?
It's documented under the --no-tags option in the man page for git
fetch. I'm not sure what you mean by the user being surprised.
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On Tue, Jun 5, 2018 at 2:41 PM Jonathan Tan wrote:
>
> When performing tag following, in addition to using the server's
> "include-tag" capability to send tag objects (and emulating it if the
> server does not support that capability), "git fetch" relies upon the
> presence of refs/tags/* entries in the initial ref advertisement to
> locally create refs pointing to the aforementioned tag objects. When
> using protocol v2, refs/tags/* entries in the initial ref advertisement
> may be suppressed by a ref-prefix argument, leading to the tag object
> being downloaded, but the ref not being created.
>
> Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
> refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
> prefix when "git fetch" is invoked with no refspecs, but not when "git
> fetch" is invoked with refspecs. Extend that functionality to make it
> work in both situations.
okay. Thinking long term, we may want to introduce a capability that
can filter the tag space, e.g. "want-refs-since refs/tags/*"
as a client directive as then the client only asks for new (newly
created/appearing) tags instead of all tags.
> This also necessitates a change another test which tested ref
> advertisement filtering using tag refs -
sounds plausible.
> since tag refs are sent by
> default now, the test has been switched to using branch refs instead.
That mismatches what I would expect to read below.
I would expect the client to always ask for refs/tags/* now and
instead of the server just giving them.
Oh, the problem is in that other test to restrict the refs/tags
to *not* be sent?
Maybe we can only ask for refs/tags/* if we do not have any
"refs/tags/" on the CLI: if I invoke "git fetch refs/tags/v1"
I would not get an advertisement for refs/tags/v2 but if I omit
all tags from the refspec, I'd get all its advertisements (v1+v2)
> Signed-off-by: Jonathan Tan
> ---
> builtin/fetch.c| 2 +-
> t/t5702-protocol-v2.sh | 24 +---
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ea5b9669a..1f447f1e8 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> *transport,
> refspec_ref_prefixes(>remote->fetch,
> _prefixes);
>
> if (ref_prefixes.argc &&
> - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
Oh, I see. This always asks for refs/tags/ whereas before we only
asked for them if there were *no* refspec given. Maybe we can
change this to
refspec_any_item_contains("refs/tags/")
instead of always asking for the tags?
(that function would need to be written; I guess for a short term bugfix
this is good enough)
How is the tag following documented (i.e. when is the user least
surprised that we do not tag-follow all and unconditionally)?
> argv_array_push(_prefixes, "refs/tags/");
> }
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 261e82b0f..b31b6d8d3 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -204,6 +204,7 @@ test_expect_success 'ref advertisment is filtered during
> fetch using protocol v2
> test_when_finished "rm -f log" &&
>
> test_commit -C file_parent three &&
> + git -C file_parent branch unwanted-branch three &&
>
> GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2
> \
> fetch origin master &&
> @@ -212,9 +213,8 @@ test_expect_success 'ref advertisment is filtered during
> fetch using protocol v2
> git -C file_parent log -1 --format=%s >expect &&
> test_cmp expect actual &&
>
> - ! grep "refs/tags/one" log &&
> - ! grep "refs/tags/two" log &&
> - ! grep "refs/tags/three" log
> + grep "refs/heads/master" log &&
> + ! grep "refs/heads/unwanted-branch" log
> '
That makes sense so far.
>
> test_expect_success 'server-options are sent when fetching' '
> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have
> lines' '
> $(git -C server rev-parse completely-unrelated)
> '
>
> +test_expect_success 'fetch supports include-tag and tag following' '
> + rm -rf server client trace &&
test_when_finished ;)
> + git init server &&
> +
> + test
Re: [PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects
On Sun, Jun 10, 2018 at 02:32:57PM +, Kirill Smelkov wrote:
> Added test shows remote with two tag objects pointing to a blob and a
> tree. The tag objects themselves are referenced from under regular
> refs/tags/* namespace. If test_expect_failure is changed to
> test_expect_success the test fails:
Interesting case. The problem is actually that upload-pack complains:
> fatal: git upload-pack: not our ref
> 038f48ad0beaffbea71d186a05084b79e3870cbf
And that sha1 is the tagged blob:
> bc4e9e1fa80662b449805b1ac29fc9b1e4c49187refs/tags/tag-to-blob
> # <-- NOTE
> 038f48ad0beaffbea71d186a05084b79e3870cbfrefs/tags/tag-to-blob^{}
> 520db1f5e1afeaa12b1a8d73ce82db72ca036ee1refs/tags/tag-to-tree
> # <-- NOTE
> 7395c100223b7cd760f58ccfa0d3f3d2dd539bb6refs/tags/tag-to-tree^{}
So it seems like upload-pack is at fault for not marking the object as a
tip when it peels the tag.
> For the reference, porcelain fetch 'refs/*:refs/origin/*' works:
That's because it doesn't actually issue a "want" for the peeled blob
(it doesn't need to, because it's fetching the tag itself). So it
happens to work, but I still think upload-pack is at fault for not
accepting the "want" on the blob it advertised.
Doubly interesting, it looks like this case _used_ to work, but was
broken by 5f0fc64513 (fetch-pack: eliminate spurious error messages,
2012-09-09). Which only changed the fetch-pack side. It moved the
handling of --all so that it was no longer in the "else" for
check_refname_format(). I guess the original code was rejecting those
peeled bits as "not a ref" (which makes sense).
So that seems like a bug in fetch-pack. But I'm still not convinced that
upload-pack doesn't also have a bug.
> +test_expect_failure 'test --all wrt tag to non-commits' '
> + blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) &&
> + git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 &&
> + tree_sha1=$(echo -e "100644 blob $blob_sha1\tfile" | git mktree) &&
I had to switch this "echo -e" to:
printf "100644 blob $blob_sha1\tfile\n"
since "-e" is a bash-ism (and my /bin/sh is dash).
-Peff
[PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects
Added test shows remote with two tag objects pointing to a blob and a
tree. The tag objects themselves are referenced from under regular
refs/tags/* namespace. If test_expect_failure is changed to
test_expect_success the test fails:
Initialized empty Git repository in
/home/kirr/src/tools/git/git/t/trash directory.t5500-fetch-pack/fetchall/.git/
fatal: git upload-pack: not our ref
038f48ad0beaffbea71d186a05084b79e3870cbf
fatal: The remote end hung up unexpectedly
not ok 56 - test --all wrt tag to non-commits
#
# blob_sha1=$(echo "hello blob" | git hash-object -t blob
-w --stdin) &&
# git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 &&
# tree_sha1=$(echo -e "100644 blob $blob_sha1\tfile" |
git mktree) &&
# git tag -a -m "tag -> tree" tag-to-tree $tree_sha1 &&
# mkdir fetchall &&
# (
# cd fetchall &&
# git init &&
# git fetch-pack --all .. &&
# git cat-file blob $blob_sha1 >/dev/null &&
# git cat-file tree $tree_sha1 >/dev/null
# )
and manual investigation from under "trash
directory.t5500-fetch-pack/fetchall/" shows:
.../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote ..
440858748ae905d48259d4fb67a12a7aa1520cf7HEAD
f85e353c1b377970afbb804118d9135948598eearefs/heads/A
440858748ae905d48259d4fb67a12a7aa1520cf7refs/heads/B
7f3cb539fbce926dd99233cfc9b6966f1d69747erefs/heads/C
b3401427a9637a35f6a203d635e5677e76ad409drefs/heads/D
4928b093c801d36be5cdb3ed3ab572fa0d4c93bfrefs/heads/E
c1375be492d3716839043d7f7e9a593f8e80c668refs/heads/F
f85e353c1b377970afbb804118d9135948598eearefs/tags/A
440858748ae905d48259d4fb67a12a7aa1520cf7refs/tags/B
7f3cb539fbce926dd99233cfc9b6966f1d69747erefs/tags/C
b3401427a9637a35f6a203d635e5677e76ad409drefs/tags/D
4928b093c801d36be5cdb3ed3ab572fa0d4c93bfrefs/tags/E
c1375be492d3716839043d7f7e9a593f8e80c668refs/tags/F
27f494dfb7e67d2f9cd2282404adf1d97581aa34refs/tags/OLDTAG
10e1d7b51cacf2f0478498681177f0e6f1e8392drefs/tags/TAGA1
f85e353c1b377970afbb804118d9135948598eearefs/tags/TAGA1^{}
f85e353c1b377970afbb804118d9135948598eearefs/tags/TAGA2
a540a4ddd2b16a9fe66e9539d5ec103c68052eaarefs/tags/TAGB1
9ca64d8fd8038b086badca1d11ccd8bbcfdeace1refs/tags/TAGB1^{}
9ca64d8fd8038b086badca1d11ccd8bbcfdeace1refs/tags/TAGB2
bc4e9e1fa80662b449805b1ac29fc9b1e4c49187refs/tags/tag-to-blob
# <-- NOTE
038f48ad0beaffbea71d186a05084b79e3870cbfrefs/tags/tag-to-blob^{}
520db1f5e1afeaa12b1a8d73ce82db72ca036ee1refs/tags/tag-to-tree
# <-- NOTE
7395c100223b7cd760f58ccfa0d3f3d2dd539bb6refs/tags/tag-to-tree^{}
.../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack
--all ..
fatal: A git upload-pack: not our ref
038f48ad0beaffbea71d186a05084b79e3870cbf
fatal: The remote end hung up unexpectedly
however the remote has all referenced objects and is generally ok:
.../git/t/trash directory.t5500-fetch-pack/fetchall$ cd ..
.../git/t/trash directory.t5500-fetch-pack$ git show tag-to-blob
tag tag-to-blob
Tagger: C O Mitter
Date: Thu Apr 7 16:36:13 2005 -0700
tag -> blob
hello blob
.../git/t/trash directory.t5500-fetch-pack$ git show tag-to-tree
tag tag-to-tree
Tagger: C O Mitter
Date: Thu Apr 7 16:36:13 2005 -0700
tag -> tree
tree tag-to-tree
file
.../git/t/trash directory.t5500-fetch-pack$ git fsck
Checking object directories: 100% (256/256), done.
For the reference, porcelain fetch 'refs/*:refs/origin/*' works:
.../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch ..
'refs/*:refs/origin/*'
remote: Enumerating objects: 259, done.
remote: Counting objects: 100% (259/259), done.
remote: Compressing objects: 100% (89/89), done.
remote: Total 259 (delta 1), reused 0 (delta 0)
Receiving objects: 100% (259/259), 21.64 KiB | 1.97 MiB/s, done.
Resolving deltas: 100% (1/1), done.
From ..
* [new branch] A -> refs/origin/heads/A
* [new branch] B -> refs/origin/heads/B
* [new branch] C
[PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
When performing tag following, in addition to using the server's
"include-tag" capability to send tag objects (and emulating it if the
server does not support that capability), "git fetch" relies upon the
presence of refs/tags/* entries in the initial ref advertisement to
locally create refs pointing to the aforementioned tag objects. When
using protocol v2, refs/tags/* entries in the initial ref advertisement
may be suppressed by a ref-prefix argument, leading to the tag object
being downloaded, but the ref not being created.
Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
prefix when "git fetch" is invoked with no refspecs, but not when "git
fetch" is invoked with refspecs. Extend that functionality to make it
work in both situations.
This also necessitates a change another test which tested ref
advertisement filtering using tag refs - since tag refs are sent by
default now, the test has been switched to using branch refs instead.
Signed-off-by: Jonathan Tan
---
builtin/fetch.c| 2 +-
t/t5702-protocol-v2.sh | 24 +---
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..1f447f1e8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,
refspec_ref_prefixes(>remote->fetch, _prefixes);
if (ref_prefixes.argc &&
- (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
+ (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
argv_array_push(_prefixes, "refs/tags/");
}
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 261e82b0f..b31b6d8d3 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -204,6 +204,7 @@ test_expect_success 'ref advertisment is filtered during
fetch using protocol v2
test_when_finished "rm -f log" &&
test_commit -C file_parent three &&
+ git -C file_parent branch unwanted-branch three &&
GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
fetch origin master &&
@@ -212,9 +213,8 @@ test_expect_success 'ref advertisment is filtered during
fetch using protocol v2
git -C file_parent log -1 --format=%s >expect &&
test_cmp expect actual &&
- ! grep "refs/tags/one" log &&
- ! grep "refs/tags/two" log &&
- ! grep "refs/tags/three" log
+ grep "refs/heads/master" log &&
+ ! grep "refs/heads/unwanted-branch" log
'
test_expect_success 'server-options are sent when fetching' '
@@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have
lines' '
$(git -C server rev-parse completely-unrelated)
'
+test_expect_success 'fetch supports include-tag and tag following' '
+ rm -rf server client trace &&
+ git init server &&
+
+ test_commit -C server to_fetch &&
+ git -C server tag -a annotated_tag -m message &&
+
+ git init client &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
+ fetch "$(pwd)/server" to_fetch:to_fetch &&
+
+ grep "fetch> ref-prefix to_fetch" trace &&
+ grep "fetch> ref-prefix refs/tags/" trace &&
+ grep "fetch> include-tag" trace &&
+
+ git -C client cat-file -e $(git -C client rev-parse annotated_tag)
+'
+
# Test protocol v2 with 'http://' transport
#
. "$TEST_DIRECTORY"/lib-httpd.sh
--
2.17.0.768.g1526ddbba1.dirty
Re: [PATCH 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On 06/05, Jonathan Tan wrote:
> When performing tag following, in addition to using the server's
> "include-tag" capability to send tag objects (and emulating it if the
> server does not support that capability), "git fetch" relies upon the
> presence of refs/tags/* entries in the initial ref advertisement to
> locally create refs pointing to the aforementioned tag objects. When
> using protocol v2, refs/tags/* entries in the initial ref advertisement
> may be suppressed by a ref-prefix argument, leading to the tag object
> being downloaded, but the ref not being created.
>
> Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
> refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
> prefix when "git fetch" is invoked with no refspecs, but not when "git
> fetch" is invoked with refspecs. Extend that functionality to make it
> work in both situations.
>
> Signed-off-by: Jonathan Tan
> ---
> builtin/fetch.c| 2 +-
> t/t5702-protocol-v2.sh | 18 ++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ea5b9669a..1f447f1e8 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> *transport,
> refspec_ref_prefixes(>remote->fetch, _prefixes);
>
> if (ref_prefixes.argc &&
> - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
> argv_array_push(_prefixes, "refs/tags/");
> }
This is difficult...Really I don't think the default should be to follow
tags. Mostly because this defeats the purpose of ref filtering when a
user only requests the master branch. Now instead of the server only
sending the master branch, you get the whole tags namespace as well.
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 261e82b0f..6733579c1 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have
> lines' '
> $(git -C server rev-parse completely-unrelated)
> '
>
> +test_expect_success 'fetch supports include-tag and tag following' '
> + rm -rf server client trace &&
> + git init server &&
> +
> + test_commit -C server to_fetch &&
> + git -C server tag -a annotated_tag -m message &&
> +
> + git init client &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> + fetch "$(pwd)/server" to_fetch:to_fetch &&
> +
> + grep "fetch> ref-prefix to_fetch" trace &&
> + grep "fetch> ref-prefix refs/tags/" trace &&
> + grep "fetch> include-tag" trace &&
> +
> + git -C client cat-file -e $(git -C client rev-parse annotated_tag)
> +'
> +
> # Test protocol v2 with 'http://' transport
> #
> . "$TEST_DIRECTORY"/lib-httpd.sh
> --
> 2.17.0.768.g1526ddbba1.dirty
>
--
Brandon Williams
Re: [PATCH 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On 06/05, Jonathan Tan wrote:
> When performing tag following, in addition to using the server's
> "include-tag" capability to send tag objects (and emulating it if the
> server does not support that capability), "git fetch" relies upon the
> presence of refs/tags/* entries in the initial ref advertisement to
> locally create refs pointing to the aforementioned tag objects. When
> using protocol v2, refs/tags/* entries in the initial ref advertisement
> may be suppressed by a ref-prefix argument, leading to the tag object
> being downloaded, but the ref not being created.
>
> Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
> refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
> prefix when "git fetch" is invoked with no refspecs, but not when "git
> fetch" is invoked with refspecs. Extend that functionality to make it
> work in both situations.
>
> Signed-off-by: Jonathan Tan
Test t5702-protocol-v2.sh doesn't pass with this patch.
> ---
> builtin/fetch.c| 2 +-
> t/t5702-protocol-v2.sh | 18 ++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ea5b9669a..1f447f1e8 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> *transport,
> refspec_ref_prefixes(>remote->fetch, _prefixes);
>
> if (ref_prefixes.argc &&
> - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
> argv_array_push(_prefixes, "refs/tags/");
> }
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 261e82b0f..6733579c1 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have
> lines' '
> $(git -C server rev-parse completely-unrelated)
> '
>
> +test_expect_success 'fetch supports include-tag and tag following' '
> + rm -rf server client trace &&
> + git init server &&
> +
> + test_commit -C server to_fetch &&
> + git -C server tag -a annotated_tag -m message &&
> +
> + git init client &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> + fetch "$(pwd)/server" to_fetch:to_fetch &&
> +
> + grep "fetch> ref-prefix to_fetch" trace &&
> + grep "fetch> ref-prefix refs/tags/" trace &&
> + grep "fetch> include-tag" trace &&
> +
> + git -C client cat-file -e $(git -C client rev-parse annotated_tag)
> +'
> +
> # Test protocol v2 with 'http://' transport
> #
> . "$TEST_DIRECTORY"/lib-httpd.sh
> --
> 2.17.0.768.g1526ddbba1.dirty
>
--
Brandon Williams
[PATCH 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
When performing tag following, in addition to using the server's
"include-tag" capability to send tag objects (and emulating it if the
server does not support that capability), "git fetch" relies upon the
presence of refs/tags/* entries in the initial ref advertisement to
locally create refs pointing to the aforementioned tag objects. When
using protocol v2, refs/tags/* entries in the initial ref advertisement
may be suppressed by a ref-prefix argument, leading to the tag object
being downloaded, but the ref not being created.
Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
prefix when "git fetch" is invoked with no refspecs, but not when "git
fetch" is invoked with refspecs. Extend that functionality to make it
work in both situations.
Signed-off-by: Jonathan Tan
---
builtin/fetch.c| 2 +-
t/t5702-protocol-v2.sh | 18 ++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..1f447f1e8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,
refspec_ref_prefixes(>remote->fetch, _prefixes);
if (ref_prefixes.argc &&
- (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
+ (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
argv_array_push(_prefixes, "refs/tags/");
}
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 261e82b0f..6733579c1 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have
lines' '
$(git -C server rev-parse completely-unrelated)
'
+test_expect_success 'fetch supports include-tag and tag following' '
+ rm -rf server client trace &&
+ git init server &&
+
+ test_commit -C server to_fetch &&
+ git -C server tag -a annotated_tag -m message &&
+
+ git init client &&
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
+ fetch "$(pwd)/server" to_fetch:to_fetch &&
+
+ grep "fetch> ref-prefix to_fetch" trace &&
+ grep "fetch> ref-prefix refs/tags/" trace &&
+ grep "fetch> include-tag" trace &&
+
+ git -C client cat-file -e $(git -C client rev-parse annotated_tag)
+'
+
# Test protocol v2 with 'http://' transport
#
. "$TEST_DIRECTORY"/lib-httpd.sh
--
2.17.0.768.g1526ddbba1.dirty
