| Message ID | 20230414121841.373980-7-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/18] version-gen: reorganize | expand |
Felipe Contreras wrote: > If we are not in a git repository `git describe` will fail anyway. The parent directory may be a git repository though. The current code ensures that we're running `git describe` in the proper repository. If we drop this, aren't we breaking things for someone building a git tarball which is in a subdirectory of a git repository? > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > GIT-VERSION-GEN | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN > index 34f561752b..cd94a7902e 100755 > --- a/GIT-VERSION-GEN > +++ b/GIT-VERSION-GEN > @@ -26,7 +26,7 @@ describe () { > if test -f version > then > VN=$(cat version) > -elif test -d "${GIT_DIR:-.git}" -o -f .git && describe > +elif describe > then > VN=$(echo "$VN" | sed -e 's/-/./g') > fi
Todd Zullinger wrote: > Felipe Contreras wrote: > > If we are not in a git repository `git describe` will fail anyway. > > The parent directory may be a git repository though. The > current code ensures that we're running `git describe` in > the proper repository. How exactly does it do that? The current code expects the cwd to be the git repo, run it in any other directory and it will generate GIT-VERSION-FILE in that directory, which is clearly not intended. > If we drop this, aren't we breaking things for someone > building a git tarball which is in a subdirectory of a git > repository? How exactly would this hypothetical person build such a tarball? git init /tmp/foo mkdir -p /tmp/foo/bar cd /tmp/foo/bar make -C ~/dev/git dist Works fine.
Felipe Contreras wrote: > Todd Zullinger wrote: >> Felipe Contreras wrote: >>> If we are not in a git repository `git describe` will fail anyway. >> >> The parent directory may be a git repository though. The >> current code ensures that we're running `git describe` in >> the proper repository. > > How exactly does it do that? > > The current code expects the cwd to be the git repo, run it in any other > directory and it will generate GIT-VERSION-FILE in that directory, which is > clearly not intended. Whether it's fool proof isn't really my point. It did attempt to check that .git was a file or directory. Not checking at all isn't necessarily an improvement, was my concern. >> If we drop this, aren't we breaking things for someone >> building a git tarball which is in a subdirectory of a git >> repository? > > How exactly would this hypothetical person build such a tarball? > > git init /tmp/foo > mkdir -p /tmp/foo/bar > cd /tmp/foo/bar > make -C ~/dev/git dist If I have a git repo, say ~/fedora/git which contains the fedora packaging (spec file, etc.) and extract a git archive in this directory, the describe will now pick up the data from the parent git directory, won't it? $ git -C ~/fedora clone https://src.fedoraproject.org/rpms/git.git $ cd ~/fedora/git $ git -C ~/upstream/git archive --format=tar --prefix=git/ HEAD | tar xf - $ cd git $ make GIT-VERSION-GEN $ cat GIT-VERSION-FILE GIT_VERSION = The version file in the tarballs prevents this from happening in the most common case, but it still feels like this is loosening things a little more than it should.
Todd Zullinger wrote: > Felipe Contreras wrote: > > Todd Zullinger wrote: > >> Felipe Contreras wrote: > >>> If we are not in a git repository `git describe` will fail anyway. > >> > >> The parent directory may be a git repository though. The > >> current code ensures that we're running `git describe` in > >> the proper repository. > > > > How exactly does it do that? > > > > The current code expects the cwd to be the git repo, run it in any other > > directory and it will generate GIT-VERSION-FILE in that directory, which is > > clearly not intended. > > Whether it's fool proof isn't really my point. It did > attempt to check that .git was a file or directory. Not > checking at all isn't necessarily an improvement, was my > concern. > > >> If we drop this, aren't we breaking things for someone > >> building a git tarball which is in a subdirectory of a git > >> repository? > > > > How exactly would this hypothetical person build such a tarball? > > > > git init /tmp/foo > > mkdir -p /tmp/foo/bar > > cd /tmp/foo/bar > > make -C ~/dev/git dist > > If I have a git repo, say ~/fedora/git which contains the > fedora packaging (spec file, etc.) and extract a git archive > in this directory, the describe will now pick up the data > from the parent git directory, won't it? > > $ git -C ~/fedora clone https://src.fedoraproject.org/rpms/git.git > $ cd ~/fedora/git > $ git -C ~/upstream/git archive --format=tar --prefix=git/ HEAD | tar xf - > $ cd git > $ make GIT-VERSION-GEN > $ cat GIT-VERSION-FILE > GIT_VERSION = I don't think this is a realistic use-case, but supposing it is, what would be the desired outcome in this case? GIT_VERSION = 2.40.GIT ? > The version file in the tarballs prevents this from > happening in the most common case, but it still feels like > this is loosening things a little more than it should. If we care about this, the same behavior can be achieved with GIT_CEILING_DIRECTORIES: GIT_CEILING_DIRECTORIES=$(cd .. && pwd) git describe ...
Felipe Contreras wrote: > Todd Zullinger wrote: >> If I have a git repo, say ~/fedora/git which contains the >> fedora packaging (spec file, etc.) and extract a git archive >> in this directory, the describe will now pick up the data >> from the parent git directory, won't it? >> >> $ git -C ~/fedora clone https://src.fedoraproject.org/rpms/git.git >> $ cd ~/fedora/git >> $ git -C ~/upstream/git archive --format=tar --prefix=git/ HEAD | tar xf - >> $ cd git >> $ make GIT-VERSION-GEN >> $ cat GIT-VERSION-FILE >> GIT_VERSION = > > I don't think this is a realistic use-case, but supposing it is, what would be > the desired outcome in this case? > > GIT_VERSION = 2.40.GIT > > ? That's what we get before this change. I don't have an opinion on whether that could/should change, but if it does, it should be deliberate I think. >> The version file in the tarballs prevents this from >> happening in the most common case, but it still feels like >> this is loosening things a little more than it should. > > If we care about this, the same behavior can be achieved with GIT_CEILING_DIRECTORIES: > > GIT_CEILING_DIRECTORIES=$(cd .. && pwd) git describe ... I only mention it because it seemed like a change in behavior when the series aimed to not change any behavior. I'm sure there are plenty of things which could be changed while making imrpveoments here. I think we'll be better off if those changes are all noted and deliberate. Thanks for working on this stuff, it is good to see improvements to this sort of plumbing.
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 34f561752b..cd94a7902e 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -26,7 +26,7 @@ describe () { if test -f version then VN=$(cat version) -elif test -d "${GIT_DIR:-.git}" -o -f .git && describe +elif describe then VN=$(echo "$VN" | sed -e 's/-/./g') fi
If we are not in a git repository `git describe` will fail anyway. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- GIT-VERSION-GEN | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)