From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lucy.dinwoodie.org (b.8.0.0.8.9.b.0.2.f.0.9.2.a.d.b.d.a.0.2.5.1.e.d.0.b.8.0.1.0.0.2.ip6.arpa [IPv6:2001:8b0:de15:20ad:bda2:90f2:b98:8b]) by sourceware.org (Postfix) with ESMTPS id 2DDA1385843A for ; Tue, 8 Mar 2022 09:28:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2DDA1385843A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=dinwoodie.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dinwoodie.org Received: from adam by lucy.dinwoodie.org with local (Exim 4.94.2) (envelope-from ) id 1nRW8S-001VGD-VZ for cygwin-apps@cygwin.com; Tue, 08 Mar 2022 09:28:04 +0000 Date: Tue, 8 Mar 2022 09:28:04 +0000 From: Adam Dinwoodie To: cygwin-apps@cygwin.com Subject: Re: [ITP] git-filter-repo 2.34.0 Message-ID: <20220308092804.vbhtazpcg4yl7dck@lucy.dinwoodie.org> Reply-To: cygwin-apps@cygwin.com References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-0.8 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, KAM_NUMSUBJECT, PDS_RDNS_DYNAMIC_FP, RDNS_DYNAMIC, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: cygwin-apps@cygwin.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Cygwin package maintainer discussion list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Mar 2022 09:28:09 -0000 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