Hi Samo,
On Fri, Jun 14, 2024 at 09:55:31PM +0200, Samo Pogačnik wrote:
> Hi Daniel,
>
> Dne 14.06.2024 (pet) ob 20:50 +0200 je Daniel Gröber napisal(a):
> > Below you're not ACK'ing some of my comments again. With email review you
> > really kind of have to say something about each comment otherwise it's
> > really hard to tell if you just missed one or not and it's easier to
> > discuss any disagreements sooner (when I have forgotten less of the context
> > :x)
> > rather than later when you send the next version.
>
> I just realized that i sent the unfinished mail instead of saving the
> draft. I am sorry. I'll add the missing replies here and thanks for the
> valuable response.
No worries, that happens :)
> > 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_pogacnik@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.
> >
> ACK
>
> > 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 ;)
> >
> ACK
>
> > 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?
> >
> ACK
>
> > > --- 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.
> >
> ACK
>
> > > 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 ;)
>
> While, the released code contains both the generated code and the
> generators, and it seem's that there is no need to call generators
> downstream, i corrected both (see below).
Oh, it seems I missed that bit. That's fine then.
> > > 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
--Daniel
Attachment:
signature.asc
Description: PGP signature