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.
> 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.
>
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).
> >
> > 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
> >
>
regards, Samo
