public inbox for
 help / color / mirror / Atom feed
From: Adam Dinwoodie <>
Subject: Re: [ITP] git-filter-repo 2.34.0
Date: Tue, 8 Mar 2022 09:28:04 +0000	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Mon, Mar 07, 2022 at 07:11:24PM -0500, James Morris wrote:
> Hello,
> I'd like to maintain the package for git-filter-repo, a Python script
> to quickly edit git history. It's MIT licensed, available in both
> Debian and Fedora, and I've also just recently packaged
> git-filter-repo up for MSYS2.

Oh excellent!  I've been thinking about doing this myself, but I'm very
happy for someone else to do the work!

> Cygport package files can be found here:

A few thoughts from me before I think this is good to go:

- You've patched the shebang from `/usr/bin/env python3` to
  `/usr/bin/python3`.  To what end?  /usr/bin/env is part of coreutils
  for Cygwin, so there shouldn't be any risk that it won't be installed.
  If someone has their own compiled python3 in /usr/local/bin, they'd
  probably expect that to be used, so I don't think I'd change the
  shebang unless there's some clear and specific reason for doing so.

- You're changing the shebang with both a patch file and with a line in
  src_compile; you don't need to do both!  I suspect this is an artefact
  of how Cygport packages the source files, but AIUI the canonical way
  to do this sort of patching with Cygport is to drop the sed line from
  your .cygport file and just keep the patch file that gets generated.

- You've set the category as both Devel and Python.  IMO (I've not
  checked what the general consensus on this is) this shouldn't be in
  Python: it's a tool that happens to use Python, but I'd expect the
  Python category to be for things that are specifically useful to
  people doing Python dev, so things like libraries that can be usefully
  imported in a Python module, or tools for debugging Python scripts.  I
  think this should only be in the Devel category.

- That said, I think ideally you'd also be packaging,
  which does provide a Python library that users can import.  At that
  point, this would unambiguously belong in both categories.

- You're installing the main script into /usr/bin.  I think the
  executable should probably be installed into /usr/libexec/git-core,
  along with other Git executables.  This is what `git --exec-path`
  returns, and matches what's described in  More generally,
  I think you should be emulating one of the installation mechanisms in, either using the provided makefile, or following the
  described steps in the "Manual Installation" section of that doc.

- Currently Cygport can't run the test suite.  Ideally it'd be able to;
  it seems unlikely that there would be Cygwin-specific regressions in
  this code, but it's not out of the question, and given upstream
  provide a test suite, it seems a shame to not use it.

That all said, this is clearly very close to ready to ship, and as I
say, it'll definitely be useful once it's available.  Thank you!


  reply	other threads:[~2022-03-08  9:28 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-08  0:11 James Morris
2022-03-08  9:28 ` Adam Dinwoodie [this message]
2022-03-08 20:42   ` James Morris
2022-03-11 21:54     ` Adam Dinwoodie
2022-03-12 12:59       ` Jon Turney
2022-03-10 20:52 ` Marco Atzeri
2022-03-11 13:37   ` Jon Turney
2022-04-08 10:26     ` Jon Turney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).