Hi Daniel, Thanks for the review. I'll do my best to include as much as i know how to into another 'release candidate', however it may take me a while. I also have some additional concerns/questions below...
Dne 10.06.2024 (pon) ob 22:26 +0200 je Daniel Gröber napisal(a): > 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 :) > I am a bit confused. I'm not sure i want to present patches upstream while you (at least) have not yet accepted them. Otherwise i'd have to revise my upstream proposal (and spam upstream) every time you find another thing i missed about the debian policy (which i probably will for some time). How do you see this? > > 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 am sorry that you felt ignored. I simply thought i have a choice here and tbh i still do not see what could be wrong by having sourced only scripts in a separate subdirectory. I'd be happy to understand your git-subrepo.d concerns better to be able to fully support this decision. > I noticed another thing, w > e 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' ... ? thanks > > > 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
