Hi Samo, On Sat, Jun 01, 2024 at 10:01:53AM +0200, Samo Pogačnik wrote: > I prepared a new 0.4.6-1 release with patched upstream regarding: > - bash-completition integration with git > - executable permissions on sourced-only files > - hashbangs inside sourced-only files > > I've also removed old lintian-override from debian/, since it is not > required anymore.
Thanks, commit review inline below. In general you're missing the Debian patch metadata, see [DEP-3]. I hate this archaic stuff I just mention it for completness. If you use gbp-pq for generating the patches you can mostly ignore. I do like to use the Forwarded field to note the upstream PR for the patch tho. [DEP-3]: https://dep-team.pages.debian.net/deps/dep3/ Next time I see the patches I want to see a Forwarded field for each -- one PR for all of them pleas, not spam upstream :) > Dne 28.05.2024 (tor) ob 19:23 +0200 je Daniel Gröber napisal(a): > > I'm not super happy with the approach of putting git-subrepo.d inside > > /usr/share/git-subrepo tbh. I might be able to let it pass but it seems > > lintian found another issue that needs patching anyway so you may as well > > fix this too. I'm still not seeing a change to remove git-subrepo.d. My comments don't just go away by ignoring them :P I noticed another thing, we disable the test suite for what appear to be trivial reasons. I really don't like maintaining packages without a test suite so this is a no-go. Please look into why it's not working and fix it with more upstreamable patches if necessary. From a quick look it seems removing the `git config core.autocrlf input` call in test/setup already gets us quite far but the way git subrepo finds its libs needs adjustment too. Commit review below: > From: Samo Pogačnik <samo_pogac...@t-2.net> > > Default 'git-core' location of git-subrepo executables currently +The default 'git-core' ... ? > does not automatically integrate git-subrepo into git's own bash- > completion. This change moves git-subrepo executables into > /usr/share/git-subrepo and adds a symlink of its main executable > script into /usr/bin to achieve recognition of the 'git subrepo' > sub-command under the git bash-completion (through git's: > --list-cmds=...,other,...). > > Gbp-Pq: Name 0001-Fixed-bash-completion-integration-with-git.patch > --- > Makefile | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 79898f5..3d84454 100644 > --- a/Makefile > +++ b/Makefile > @@ -17,7 +17,7 @@ SHARE = share > > # Install variables: > PREFIX ?= /usr/local > -INSTALL_LIB ?= $(DESTDIR)$(shell git --exec-path) > +INSTALL_LIB ?= $(DESTDIR)$(PREFIX)/share/$(NAME) While we're fixing upstream's mess $(DESTDIR) should be interpolated in the install target only so overriding the directory structure is easier. The install target should look something like this, vars listed for clarity: ``` PREFIX ?= /usr/local INSTALL_LIB ?= $(PREFIX)/share/$(NAME) install: $(INSTALL) -d -m 0755 $(DESTDIR)$(INSTALL_LIB)/ $(INSTALL) -C -m 0755 $(LIB) $(DESTDIR)$(INSTALL_LIB)/ ``` Notice the canonical use of $(INSTALL) instead of the plain command, doesn't make a difference in this case but it's good Makefile hygiene. With that setup we can just export the INSTALL_* vars in debian/rules to override them: export INSTALL_LIB=/usr/share/git-subrepo export INSTALL_EXT=$(INSTALL_LIB) Setting INSTALL_EXT equal to INSTALL_LIB gets rid of the git-subrepo.d as I've been requesting. I'm sending you the full patch "Drop unecessary subdir in usr/share" I used to test this seperately, lets see if you can figure out how to apply it to your repo ;) You still have to add a seperate commit to make the $(DESTDIR) adjustment. > INSTALL_EXT ?= $(INSTALL_LIB)/$(NAME).d > INSTALL_MAN1 ?= $(DESTDIR)$(PREFIX)/share/man/man1 > > @@ -67,6 +67,8 @@ install: > install -C -m 0755 $(EXTS) $(INSTALL_EXT)/ > install -d -m 0755 $(INSTALL_MAN1)/ > install -C -m 0644 $(MAN1)/$(NAME).1 $(INSTALL_MAN1)/ > + install -d -m 0755 $(DESTDIR)$(PREFIX)/bin/ > + ln -s ../share/$(NAME)/$(NAME) $(DESTDIR)$(PREFIX)/bin/$(NAME) Creating symlinks like this we'd usually do with dh_link(1). This would be OK if you're intending to send this patch upstream? > > # Uninstall support: > uninstall: > -- > 2.39.2 > > From: Samo Pogačnik <samo_pogac...@t-2.net> > > Bash scripts under git-suberpo.d are not to be executed standalone > therefore should not have executable permissions. > > Gbp-Pq: Name 0002-Removed-executable-permission-on-sourced-only-files.patch > --- > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 3d84454..818cd7d 100644 > --- a/Makefile > +++ b/Makefile > @@ -64,7 +64,7 @@ install: > install -C -m 0755 $(LIB) $(INSTALL_LIB)/ > sed -i 's!^SUBREPO_EXT_DIR=.*!SUBREPO_EXT_DIR=$(INSTALL_EXT)!' > $(INSTALL_LIB)/$(NAME) > install -d -m 0755 $(INSTALL_EXT)/ > - install -C -m 0755 $(EXTS) $(INSTALL_EXT)/ > + install -C -m 0644 $(EXTS) $(INSTALL_EXT)/ This sort of thing would usually be done in debian/rules to keep things maintainable. Again if the patch is intended to go upstream it's acceptable. > install -d -m 0755 $(INSTALL_MAN1)/ > install -C -m 0644 $(MAN1)/$(NAME).1 $(INSTALL_MAN1)/ > install -d -m 0755 $(DESTDIR)$(PREFIX)/bin/ > -- > 2.39.2 > > From: Samo Pogačnik <samo_pogac...@t-2.net> > > Sourced-only scripts should not start with hashbangs. > > Gbp-Pq: Name 0003-Removed-hashbangs-from-sourced-only-files.patch > --- > lib/git-subrepo.d/help-functions.bash | 2 -- > pkg/bin/generate-completion.pl | 2 -- > pkg/bin/generate-help-functions.pl | 2 -- > share/completion.bash | 2 -- > 4 files changed, 8 deletions(-) > > diff --git a/lib/git-subrepo.d/help-functions.bash > b/lib/git-subrepo.d/help-functions.bash > index 123bb54..6dd5c17 100644 > --- a/lib/git-subrepo.d/help-functions.bash > +++ b/lib/git-subrepo.d/help-functions.bash > @@ -1,5 +1,3 @@ > -#!/usr/bin/env bash > - > # DO NOT EDIT. This file generated by pkg/bin/generate-help-functions.pl. Should we not fix pkg/bin/generate-help-functions.pl too/instead ? The header does say not to edit this ;) > > set -e > diff --git a/pkg/bin/generate-completion.pl b/pkg/bin/generate-completion.pl > index 7ba4165..c4aedb2 100644 > --- a/pkg/bin/generate-completion.pl > +++ b/pkg/bin/generate-completion.pl > @@ -156,8 +156,6 @@ sub generate_bash { > } > > print <<"..."; > -#!bash > - > # DO NOT EDIT. This file generated by pkg/bin/generate-completion.pl. > > _git_subrepo() { > diff --git a/pkg/bin/generate-help-functions.pl > b/pkg/bin/generate-help-functions.pl > index 40b6af8..dcd3b27 100644 > --- a/pkg/bin/generate-help-functions.pl > +++ b/pkg/bin/generate-help-functions.pl > @@ -29,8 +29,6 @@ sub main { > > sub write_start { > print <<'...'; > -#!/usr/bin/env bash > - > # DO NOT EDIT. This file generated by pkg/bin/generate-help-functions.pl. > > set -e > diff --git a/share/completion.bash b/share/completion.bash > index adaa9c6..8a89a72 100644 > --- a/share/completion.bash > +++ b/share/completion.bash > @@ -1,5 +1,3 @@ > -#!bash > - > # DO NOT EDIT. This file generated by pkg/bin/generate-completion.pl. > > _git_subrepo() { > -- > 2.39.2 > Thanks, --Daniel
signature.asc
Description: PGP signature
