bug#65245: Possible code removal in eshell/diff (em-unix.el)

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Richard Hopkins]

Hi,
I think the use of `compilation-finish-functions' and
#'current-window-configuration can be removed from #'eshell/diff
as it's no longer called and the intended functionality is achieved
elsewhere.

For example the below works with/without the 
`compilation-finish-functions'.

   $ emacs -Q -f eshell
   # in the eshell buffer
   echo a > a
   echo b > b
   diff -u a b
   # pops a #'diff-mode window with font-lock enabled; press q to quit

Here are some commits showing the introduction and then others
that help make it no longer relevant.

2001-04-24 ef59cfc6638a8ac3fccbee88482d142e95c32f81
	   Use `compilation-finish-functions' in #'eshell/diff

2004-02-02 c010ecfa9a26549486bb00c39221813c02c615de
            Bind q to #'quit-window in `diff-mode-shared-map'

2004-02-11 540d066630c55ab957dc08aa7abbca7775c9bd7b
	   #'diff - don't use compile any more, use diff-mode instead.

2011-02-01 abef340a0c5304054a2a5e6d95e1d825501f0e7a
	   `diff-mode-shared-map' inherit from `special-mode-map'

Best,
Richard

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Stefan Kangas]

close 65245 30.1
thanks

Richard Hopkins <emacs@unbit.co.uk> writes:

> I think the use of `compilation-finish-functions' and
> #'current-window-configuration can be removed from #'eshell/diff
> as it's no longer called and the intended functionality is achieved
> elsewhere.

I think your analysis is correct and it holds up in my testing.  I
have therefore removed that part on master along with associated
cleanups.

Thanks for the bug report.

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Eli Zaretskii]

> Cc: 65245@debbugs.gnu.org
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 13 Aug 2023 15:18:29 +0200
> 
> Richard Hopkins <emacs@unbit.co.uk> writes:
> 
> > I think the use of `compilation-finish-functions' and
> > #'current-window-configuration can be removed from #'eshell/diff
> > as it's no longer called and the intended functionality is achieved
> > elsewhere.
> 
> I think your analysis is correct and it holds up in my testing.  I
> have therefore removed that part on master along with associated
> cleanups.

It would have been prudent to wait for the Eshell maintainer to chime
in, before rushing with the removal.  The suggestion for removal was
posted just one day ago, and that is not long enough to assume the
maintainer is not available.

Jim, any comments on this matter?

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> It would have been prudent to wait for the Eshell maintainer

Thanks, I'll keep that in mind for next time.  Kudos to Jim for taking
on the work of maintaining eshell!

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Jim Porter]

On 8/13/2023 9:49 AM, Stefan Kangas wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>> It would have been prudent to wait for the Eshell maintainer
> 
> Thanks, I'll keep that in mind for next time.  Kudos to Jim for taking
> on the work of maintaining eshell!

Thanks, this all looks right to me, though I haven't tested it out in 
practice. For minor changes like this (especially for code that long 
predates my involvement), I don't think there's any real harm in others 
committing changes without first getting an ok from me. Still, it's 
probably reasonable to be on the safe side and let me take a look first 
(unless I'm nonresponsive for a while, of course).

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Eli Zaretskii]

> Date: Sun, 13 Aug 2023 10:32:34 -0700
> Cc: emacs@unbit.co.uk, 65245@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 8/13/2023 9:49 AM, Stefan Kangas wrote:
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> >> It would have been prudent to wait for the Eshell maintainer
> > 
> > Thanks, I'll keep that in mind for next time.  Kudos to Jim for taking
> > on the work of maintaining eshell!
> 
> Thanks, this all looks right to me, though I haven't tested it out in 
> practice. For minor changes like this (especially for code that long 
> predates my involvement), I don't think there's any real harm in others 
> committing changes without first getting an ok from me. Still, it's 
> probably reasonable to be on the safe side and let me take a look first 
> (unless I'm nonresponsive for a while, of course).

Changes in any Emacs package that has an active maintainer should be
coordinated with that maintainer.  The only exceptions are obvious
fixes like typos, clear omissions, and other such stuff.

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> Changes in any Emacs package that has an active maintainer should be
> coordinated with that maintainer.  The only exceptions are obvious
> fixes like typos, clear omissions, and other such stuff.

According to admin/MAINTAINERS, Amin Bandali is the official eshell
maintainer. Could we please fix this?

Best regards, Michael.

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Stefan Kangas]

[Copying in Amin Bandali.]

Michael Albinus <michael.albinus@gmx.de> writes:

> According to admin/MAINTAINERS, Amin Bandali is the official eshell
> maintainer. Could we please fix this?

Perhaps that should also be moved "Maintainer:" headers in
eshell/*.el.  It would presumably make the information easier to find
for hapless developers.

BTW, I see some conflicting information for ERC in admin/maintainers
and lisp/erc.el.  The former lists Kelvin White, while the latter
lists Amin Bandali and F. Jason Park.

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Jim Porter <jporterbugs@gmail.com>,  emacs@unbit.co.uk,
>   65245@debbugs.gnu.org,  stefankangas@gmail.com
> Date: Mon, 14 Aug 2023 09:26:07 +0200
> 
> > Changes in any Emacs package that has an active maintainer should be
> > coordinated with that maintainer.  The only exceptions are obvious
> > fixes like typos, clear omissions, and other such stuff.
> 
> According to admin/MAINTAINERS, Amin Bandali is the official eshell
> maintainer. Could we please fix this?

Jim, care to add yourself to that file?

(Btw, I never consult that file.  I always look at "git log" instead.)

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Jim Porter]

On 8/14/2023 5:04 AM, Eli Zaretskii wrote:
> Jim, care to add yourself to that file?
> 
> (Btw, I never consult that file.  I always look at "git log" instead.)

Now done. I left Amin Bandali's entry in the "willing to maintain" 
section since it seems that multiple people can have some level of 
maintainership, and I certainly have no problem sharing the maintenance 
with others who are interested.

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Amin Bandali]

Hi Stefan, all,

Stefan Kangas writes:

> [Copying in Amin Bandali.]
>
> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> According to admin/MAINTAINERS, Amin Bandali is the official eshell
>> maintainer. Could we please fix this?
>
> Perhaps that should also be moved "Maintainer:" headers in
> eshell/*.el.  It would presumably make the information easier to find
> for hapless developers.

Regarding eshell, I never assumed its maintenance; rather, I'd added
myself to the second section of admin/MAINTAINERS, indicating that I'd
be willing to try maintaining it if no one else does.  But it appears
that things changed recently, with Jim stepping up to maintain it per
046b5eb90d2a (thanks Jim!).

> BTW, I see some conflicting information for ERC in admin/maintainers
> and lisp/erc.el.  The former lists Kelvin White, while the latter
> lists Amin Bandali and F. Jason Park.
>

Regarding ERC, lisp/erc/erc.el is the current and correct one:
F. Jason Park (J.P.) and I are its co-maintainers.  As far as I can
tell, neither Kelvin White nor Vibhav Pant have made any commits to
lisp/erc/ in years.

Thanks,
-a

bug#65245: Possible code removal in eshell/diff (em-unix.el)

[Stefan Kangas]

Amin Bandali <bandali@gnu.org> writes:

> > BTW, I see some conflicting information for ERC in admin/maintainers
> > and lisp/erc.el.  The former lists Kelvin White, while the latter
> > lists Amin Bandali and F. Jason Park.
>
> Regarding ERC, lisp/erc/erc.el is the current and correct one:
> F. Jason Park (J.P.) and I are its co-maintainers.  As far as I can
> tell, neither Kelvin White nor Vibhav Pant have made any commits to
> lisp/erc/ in years.

I'm copying in Kelvin White and Vibhav Pant, in case they have anything to add.