Re: [ITP] git-filter-repo 2.34.0

[Adam Dinwoodie <adam@dinwoodie.org>]

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:
> https://drive.google.com/drive/folders/12RXG0804TmR9ZF2STpV7LXqpJMucYAfx?usp=sharing

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 git_filter_repo.py,
  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 INSTALL.md.  More generally,
  I think you should be emulating one of the installation mechanisms in
  INSTALL.md, 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!

Adam