Hi Samo,

On Sat, Apr 27, 2024 at 10:48:01AM +0200, Samo Pogačnik wrote:
> > So you should be doing something like this:
> > 
> >     $ git remote add upstream https://github.com/ingydotnet/git-subrepo.git
> >     $ git fetch upstream
> >     $ gbp import-ref -u 0.4.6 # this will do the upstream tag/branch
> >                               # changes and create the merge
> >     $ gbp dch
> >     <make sure the changelog entries makes sense>
> >     $ gbp buildpackage [...sbuild runes...]
> >     <test>
> >     $ git push --tags salsa debian/sid upstream
> > 
> > There's also `gbp push` to make the git-push easier but it only works after
> > doing a `gbp tag` to create the debian/* tag. This is however inappropriate
> > for you as DM to do as the convention is to have the debian tag correspond
> > to what I upload not what you propose to me :)
> > 
> > On my side I'll do:
> > 
> >     $ gbp pull samo
> >     <review>
> >     $ gbp buildpackage [...sbuild runes...]
> >     <test>
> >     $ gbp tag    # creates the debian/* tag
> >     $ debrelease   # upload to ftp-master
> >     $ gbp push salsa
> > 
> > Hope that gives you something more actionable than my previous mails.
>
> Thanks for this explanation. I suppose we shall apply this workflow upon
> the git-repo, that you are going to push into the debian/* namespace.

Yup, just want to make sure you have the ins and outs of the workflow down
first.

> As i understand, we are going to push and pull our changes from the same
> branches of the sam git-repo, however i got a bit confused by your 'gbp
> pull samo' command which indicates another git-repo involved. If your
> initial command should've been 'gbp pull salsa', then it is clear to me.

There's not much difference really. I'm just a stickler for clean git
history and also want to make sure new contributors un-learn the really bad
force-pushing habbit Git-hub/lab/tea teach people.

To be clear force-push should never ever be done when collaborating on the
branche(s) with multiple people, except in the most dire of circumstances
and only if everyone involved is notified appropriately. I'll be happy to
give you commit rights on the repo as soon as you show you've internalised
this :)

On Wed, May 01, 2024 at 11:09:48PM +0200, Samo Pogačnik wrote:
> i noticed, that bash-completion integration with git does not work.

Good catch, I don't use completions at that level much myself so I probably
wouldn't have noticed :)

> However i noticed, that the --list-cmds group 'other' scans the /usr/bin/
> directory, where our old friend 'git-debrebase' and friends already
> reside and are being successfully recognized by git. Thus i prepared a
> change (in my salsa repo: in
> https://salsa.debian.org/spog/git-subrepo/-/tree/debian/sid) to change
> the target installation directory, which seems to work.

Sounds like a reasonable plan.

On Sat, May 04, 2024 at 01:48:56PM +0200, Samo Pogačnik wrote:
> Unfortunately, lintian is not happy with my solution above (does not
> allow subdirs in /usr/bin and however git-subrepo script searches its
> helper functions in git-subrepo.d subdir in the location of main
> git-subrepo script).

Right, that doesn't comply with policy at all. My first instinct would be
to patch git-subrepo to parameterise the path to git-subrepo.d and send
that patch upstream.

> I managed to prepare another solution, without changing upstream sources
> in a way to move 'git-subrepo' executable scripts into
> /usr/libexec/git-subrepo and by adding a symlink to main git-subrepo
> executable into /usr/bin. That way bash- completion integration works as
> in the initial solution and lintian is also happy.

Hmm. I'm not sure I like the idea of abusing libexec in this
way. Technically speaking it's for "internal binaries that are not intended
to be executed directly by users or shell scripts" this is clearly not the
case here.

Looking over [git-* packages] to see what other packages do I see most git
addons are in fact installed into /usr/bin.

[git-* packages]: 
https://packages.debian.org/search?searchon=contents&keywords=git-&mode=filename&suite=stable&arch=any

Notable exception: git-absorb is still in lib/git-core and has [Bug#985775]
which seems like it may be relevant to our problem. Have a read of it when
you get the chance. Some snippets "Git changed the gitexecdir directory"
maybe that change is what broke completion from git-core?  "... git-absorb
works but git's bash completion doesn't suggest absorb" So they are aware
of the completion issue already.

[Bug#985775]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=985775

Anyway. I think the right solution here is to patch git-subrepo. IMO what's
in git-subrepo.d really belongs in /usr/share/git-subrepo OR could just be
concatenated into the main git-subrepo script, hmm. Bonus points for
forwarding this bug and patch upstream.


Git Review
----------

Now, let's get into the review. Here's what I see -- if you're not reading
this in a monospace font this is the time to reconsider your ~life~
eer. config choices :D

    84c24 * debian/sid origin/debian/sid Release 0.4.6-2     Samo Pogačnik  2d
    ac9b0 * d/*: Fixed bash-completion integration with gi$  Samo Pogačnik  2d
    ce3fb *   git-debrebase import: declare upstream         Samo Pogačnik  2w
          |\                                                                  
    c9552 * | git-debrebase convert-from-gbp: drop patches$  Samo Pogačnik  2w
    873da * | Release 0.4.6-1                                Samo Pogačnik  3w
    51d5b * | d/control: Set myself as Maintainer            Samo Pogačnik  3w
    43a8c * | d/control: Point Vcs to new location (salsa/$  Samo Pogačnik  3w
    bf7e8 * | Merge tag '0.4.6' into debian/sid              Samo Pogačnik  4w
          |\|                                                                 
    5a1ed * | Revert "Update changelog for 0.4.3-2 release$  Daniel Gröber  3Y
    4e559 * | origin/ci/salsa Add salsa-ci config            Daniel Gröber  3Y
    181d5 * | debian/0.4.3-2 Update changelog for 0.4.3-2 $  Daniel Gröber  3Y
    b6c79 * | Fix Vcs URLs, s/guest-dxld/dxld-guest/         Daniel Gröber  3Y
    f180e * | debian/0.4.3-1 Initial release. (Closes: #91$  Daniel Gröber  3Y
    73a01 | | * upstream origin/upstream docs: Replace 404$  Edwin Kofler   5M
          | |/                                                                
    110b9 | * 0.4.6 Release 0.4.6                            Austin Morgan  1Y
    3994d | * Add test for empty push                        andreasxp      1Y
    89f56 | * Remove unneeded worktree on push               Daniel Bauten  4Y
    c14bf | * Remove worktree in push                        Daniel Bauten  4Y
    dbb99 | * Remove branch creation from GitHub Action      Matijs van Z$  2Y
    84854 | * Do not depend on main repo for status tests    Matijs van Z$  4Y
    aa416 | * 0.4.5 Release 0.4.5                            Austin Morgan  2Y
    1b06c | * Add --file option                              Austin Morgan  2Y
    b4ae8 | * Fix git subrepo status command for subrepos $  Catalin Cioco  3Y
    be9f0 | * Don't allow -b and --all                       Austin Morgan  3Y
    df975 | * Fix documentation links                        Austin Morgan  3Y
    4d3db | * fix tests to support use of a default branch$  Michael Tedde  3Y
    87ee3 | * pass --force to git add so a user's global .$  Michael Tedde  3Y
    94ac5 | * Fix .rc and enable-completion.sh for zsh bef$  Ingy döt Net   3Y
    2f685 | * Better format for options                      Ingy döt Net   3Y
          |/                                                                  
    b562f * 0.4.3 Release 0.4.3                              Ingy döt Net   3Y


Drilling in: 

    c9552 * | git-debrebase convert-from-gbp: drop patches$  Samo Pogačnik  2w

I thought we agreed on using plain gbp for now?


    73a01 | | * upstream origin/upstream docs: Replace 404$  Edwin Kofler   5M
          | |/                                                                
    110b9 | * 0.4.6 Release 0.4.6                            Austin Morgan  1Y

The upstream branch is ahead of the 0.4.6 tag. Why? Seems to me you meddled
with the upstream branch by hand instead of letting the tooling take care
of it. Technicaly not a problem just makes me wonder what you're doing.


    ac9b0 * d/*: Fixed bash-completion integration with gi$  Samo Pogačnik  2d

Don't use d/*. If many files are involved just leave off the context. I
hope I haven't given you the impression you *have* to put some context
there, that's not the case.

The convention is to mention the file/package when the added context helps
the rest of the commit subject read better of be shorter. It is a pretty
soft convention however I'm not very consistent with it myself ;)

My main use case for files is stuff like "d/changelog: Fix typo" or
"d/copyright: Update for new upstream". As source packages get bigger which
binary package you're talking about starts to be important, say
"some-binpkg: Remove dep on flubnub".

Technically all of these could be reworded as something like "DoThing in
$context for this and that reason", but see what's actually happening is
split in two that way. It just reads better to get the $context first and
then the $whats_happening.

Something to keep in mind here too: if you do use the prefix convention it
is prudent to edit the gbp autogenerated d/changelog entries as end-users
don't (and shouldn't) really know what any of this Debian stuff is.

Until you're a DM/DD there's always someone in the middle editing the
changelogs but you should get into the habbit of keeping in mind who
uses/reads the stuff you produce. Some DDs may feel this extra editing step
is too annoying and insist on commit subjects being ready to go changelog
entries. YMMV.


    43a8c * | d/control: Point Vcs to new location (salsa/$  Samo Pogačnik  3w

Still doesn't point to the debian/ namespace.


For now I've cherry-picked

    51d5b * | d/control: Set myself as Maintainer            Samo Pogačnik  3w

adapted

    43a8c * | d/control: Point Vcs to new location (salsa/$  Samo Pogačnik  3w

and moved my repo to https://salsa.debian.org/debian/git-subrepo.

Further, since this repo is from when I was young-and-dumb I've deleted the
debian/* tags since they don't make sense. Like I've explained before the
tags are done by the DD *after* doing the upload, but I wasn't DD yet when
I made those and now they just not going to reflect what will end up in the
archive.

Since you've cloned my repo when they were still in there you'll want to
remove them too or the next (or first) time you `git push --tags` they'll
get uploaded too. Do this in your repo:

    $ git tag -d debian/0.4.3-1
    $ git tag -d debian/0.4.3-2

I also deleted the ci/salsa branch which I was just using to test the new
(at the time) salsaci pipeline. Seems the debian/ namespace is configured
to just run that by default now so no need for the config file. Neat. You
can reflect the branch deletion locally by doing

    $ git branch -D ci/salsa

but it's not as important as with the tags. It's just unnecesarry cruft on
your side.

I'm eagerly awaiting a well formed upstream update on top of my repo plus
patch to fix the completion I can pull.

Do note if it'll take a while to prepare a patch it would be better to
upload to NEW soon and add the fix later. That way the ftp-master review is
paralellized with our work. So maybe focus on sending me just the
import-ref result first.

Thanks,
--Daniel

Attachment: signature.asc
Description: PGP signature

Reply via email to