GNU bug report logs - #71438
[PATCH] Allow ping to receive optional arguments

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 71438 in the body.
You can then email your comments to 71438 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>
To: pbreton <at> cs.umb.edu
Cc: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Allow ping to receive optional arguments
Date: Sat, 8 Jun 2024 12:28:00 -0300

[Message part 1 (text/plain, inline)]

Hi! This small patch makes it so that the "ping" interactive
function can receive optional arguments.
This allows the caller to change its argument every time. If no
arguments are passed, the default variable will be used.

Prior to this (if I am not mistaken), one had to change the
variable each time.

I would love to get some feedback! Thanks in advance.

Version: GNU Emacs 29.3 (build 1, x86_64-pc-linux-gnu,
GTK+ Version 3.24.41, cairo version 1.18.0) of 2024-05-18

PS: This is my first time sending an email to the emacs
mailing list. I CC'd  bug-gnu-emacs <at> gnu.org because it
was the address that the CONTRIBUTE file stated one
should send patches to. I apologize in advance if I made
a mistake.

[Message part 2 (text/html, inline)]

[0001-ping-Added-optional-arguments.patch (text/x-patch, attachment)]

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Peter Breton <peter_breton <at> yahoo.com>
To: torsi <at> fi.uba.ar, pbreton <at> cs.umb.edu
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: [PATCH] Allow ping to receive optional arguments
Date: Sat, 8 Jun 2024 18:48:26 +0000 (UTC)

[Message part 1 (text/plain, inline)]

Looks good to me 😀 

Peter 
 
  On Sat, Jun 8, 2024 at 11:28 AM, TOMAS FABRIZIO ORSI<torsi <at> fi.uba.ar> wrote:   Hi! This small patch makes it so that the "ping" interactive function can receive optional arguments.This allows the caller to change its argument every time. If no 
arguments are passed, the default variable will be used.

Prior to this (if I am not mistaken), one had to change the variable each time.
I would love to get some feedback! Thanks in advance.

Version: GNU Emacs 29.3 (build 1, x86_64-pc-linux-gnu, 
GTK+ Version 3.24.41, cairo version 1.18.0) of 2024-05-18

PS: This is my first time sending an email to the emacsmailing list. I CC'd  bug-gnu-emacs <at> gnu.org because it 
was the address that the CONTRIBUTE file stated one 
should send patches to. I apologize in advance if I madea mistake.

  

[Message part 2 (text/html, inline)]

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>
To: Peter Breton <peter_breton <at> yahoo.com>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: [PATCH] Allow ping to receive optional arguments
Date: Sat, 8 Jun 2024 15:57:32 -0300

[Message part 1 (text/plain, inline)]

Hi Peter,

Looks good to me 😀
>

I am very glad to hear this ^_^. Thank you for the quick and kind response.
What comes next? What's the standard procedure?

Best regards,
- Fabrizio

[Message part 2 (text/html, inline)]

Message #14 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>, pbreton <at> cs.umb.edu
Cc: 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Sun, 9 Jun 2024 05:02:51 -0700

TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar> writes:

> Hi! This small patch makes it so that the "ping" interactive
> function can receive optional arguments.
> This allows the caller to change its argument every time. If no
> arguments are passed, the default variable will be used.
>
> Prior to this (if I am not mistaken), one had to change the
> variable each time.
>
> I would love to get some feedback! Thanks in advance.
>
> Version: GNU Emacs 29.3 (build 1, x86_64-pc-linux-gnu,
> GTK+ Version 3.24.41, cairo version 1.18.0) of 2024-05-18
>
> PS: This is my first time sending an email to the emacs
> mailing list. I CC'd  bug-gnu-emacs <at> gnu.org because it
> was the address that the CONTRIBUTE file stated one
> should send patches to. I apologize in advance if I made
> a mistake.

Thanks for your contribution to Emacs!

This is the right list to send your patch to.

> From de527784bb1f6e60a65291e5ab798328fd18c8e0 Mon Sep 17 00:00:00 2001
> From: Tomas Fabrizio Orsi <torsi <at> fi.uba.ar>
> Date: Sat, 8 Jun 2024 12:11:18 -0300
> Subject: [PATCH] ping: Added optional arguments
>
> Signed-off-by: Tomas Fabrizio Orsi <torsi <at> fi.uba.ar>

Consider adding a ChangeLog entry to the commit message, see CONTRIBUTE.

Note that we don't usually use "Signed-off-by" trailers so that can
probably be removed as redundant.

Does this deserve to be announced in NEWS?

> ---
>  lisp/net/net-utils.el | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/lisp/net/net-utils.el b/lisp/net/net-utils.el
> index 83842cd..6f69326 100644
> --- a/lisp/net/net-utils.el
> +++ b/lisp/net/net-utils.el
> @@ -78,7 +78,7 @@
>
>  ;; On GNU/Linux and Irix, the system's ping program seems to send packets
>  ;; indefinitely unless told otherwise
> -(defcustom ping-program-options
> +(defcustom ping-program-default-options
>    (and (eq system-type 'gnu/linux)
>         (list "-c" "4"))
>    "Options for the ping program.

This needs an `define-obsolete-variable-alias'.

> @@ -425,22 +425,25 @@ This variable is only used if the variable
>       options)))
>
>  ;;;###autoload
> -(defun ping (host)
> +(defun ping (host &optional options)
>    "Ping HOST.
> +Optional argument OPTIONS sets which options will be passed to `ping-program'
> +If OPTIONS is not set, then `ping-program-default-options' will be used.
>  If your system's ping continues until interrupted, you can try setting
> -`ping-program-options'."
> +`ping-program-default-options'."
>    (interactive
>     (list (let ((default (ffap-machine-at-point)))
> -           (read-string (format-prompt "Ping host" default) nil nil default))))
> -  (let ((options
> -	 (if ping-program-options
> -	     (append ping-program-options (list host))
> -	   (list host))))
> +           (read-string (format-prompt "Ping host" default) nil nil default))
> +         (split-string (read-string (format-prompt "Ping options (RET for defaults)" nil) nil nil nil) " ")))
> +  (let ((full-command
> +	 (if (or (equal options (list "")) (not options))
> +	     (append ping-program-default-options (list host))
> +	     (append options (list host)))))
>      (net-utils-run-program
>       (concat "Ping" " " host)
>       (concat "** Ping ** " ping-program " ** " host)
>       ping-program
> -     options)))
> +     full-command)))
>
>  ;;;###autoload
>  (defun nslookup-host (host &optional name-server)
> --
> 2.44.2

There is an extra RET before you can ping, so the change is
backwards-incompatible.  I rarely if ever use `M-x ping`, so I can't
give a very informed opinion here, but:

- Perhaps we should make it prompt for options only with a prefix
  command?

- Or an option that reverts back to the old behaviour (or enables the
  new)?

- Or is the new behaviour simply more useful and should be enabled
  unconditionally?  At the very least, this needs some kind of
  rationale.

Let's see if anyone else has any thoughts here.

Message #17 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: pbreton <at> cs.umb.edu, torsi <at> fi.uba.ar, 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Sun, 09 Jun 2024 16:47:32 +0300

> Cc: 71438 <at> debbugs.gnu.org
> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Sun, 9 Jun 2024 05:02:51 -0700
> 
> Does this deserve to be announced in NEWS?

Yes, IMO.

> > -(defcustom ping-program-options
> > +(defcustom ping-program-default-options
> >    (and (eq system-type 'gnu/linux)
> >         (list "-c" "4"))
> >    "Options for the ping program.
> 
> This needs an `define-obsolete-variable-alias'.

I actually wonder why change the name of the option at all?

> There is an extra RET before you can ping, so the change is
> backwards-incompatible.  I rarely if ever use `M-x ping`, so I can't
> give a very informed opinion here, but:
> 
> - Perhaps we should make it prompt for options only with a prefix
>   command?

Yes, I think we should do that, in which case the change is no longer
backward-incompatible.

Message #20 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>
To: Eli Zaretskii <eliz <at> gnu.org>, Stefan Kangas <stefankangas <at> gmail.com>
Cc: pbreton <at> cs.umb.edu, 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Sun, 9 Jun 2024 12:03:37 -0300

Hello Eli and Stefan,

Thanks for the quick response.
I'll reply to both of your emails here; I hope I am not breaking Emacs
email etiquette.

Stefan Kangas said:
> Consider adding a ChangeLog entry to the commit message, see CONTRIBUTE.

Noted. I thought my patch was too small to be mentioned in ChangeLog.
I'll keep this in mind for future patches :D.
As a side note, I found the CONTRIBUTE file very helpful.
I personally found
https://www.gnu.org/software/emacs/manual/html_node/emacs/Contributing.html
to be a bit unclear.

Stefan Kangas said:
> Note that we don't usually use "Signed-off-by" trailers so that can
> probably be removed as redundant.

Noted. I'll remove that as well. Do patches need to be PGP signed?


Eli Zaretskii said:
> I actually wonder why change the name of the option at all?

I thought that changing the name of the variable would better
indicate its intended purpose. Since, with the patch, those
variables would only be as a fallback/default (hence the name
change). In retrospect, maybe it's a bit unnecessary and the
name should be kept the same.

Eli Zaretskii said:
> Yes, I think we should do that, in which case the change is no longer
backward-incompatible.

Great! What's the procedure now? Should I upload a new patch to this
same thread with the changes you both mentioned?
As a side note, any additional notes?
Any style comments? Elisp advice? Any other function I should look into
to "imitate" the style of a good use of prefix argument?

Thank you for all the feedback,

Best regards,

- Fabrizio

Message #23 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>, Eli Zaretskii <eliz <at> gnu.org>
Cc: pbreton <at> cs.umb.edu, 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Sun, 9 Jun 2024 11:38:50 -0400

TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar> writes:

> Do patches need to be PGP signed?

We don't mind if people want to do that, but we don't ask them to.

In my experience, very few patches end up being PGP signed.

Message #26 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>
Cc: pbreton <at> cs.umb.edu, stefankangas <at> gmail.com, 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Sun, 09 Jun 2024 18:21:13 +0300

> From: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>
> Date: Sun, 9 Jun 2024 12:03:37 -0300
> Cc: pbreton <at> cs.umb.edu, 71438 <at> debbugs.gnu.org
> 
> Stefan Kangas said:
> > Consider adding a ChangeLog entry to the commit message, see CONTRIBUTE.
> 
> Noted. I thought my patch was too small to be mentioned in ChangeLog.
> I'll keep this in mind for future patches :D.
> As a side note, I found the CONTRIBUTE file very helpful.
> I personally found
> https://www.gnu.org/software/emacs/manual/html_node/emacs/Contributing.html
> to be a bit unclear.

I suggest instead to read the file CONTRIBUTE in the top-level
directory of the Emacs source tree.

> Eli Zaretskii said:
> > Yes, I think we should do that, in which case the change is no longer
> backward-incompatible.
> 
> Great! What's the procedure now? Should I upload a new patch to this
> same thread with the changes you both mentioned?

Yes, please .

> As a side note, any additional notes?
> Any style comments?

Just one: try to keep lines shorter than 80 columns, and break longer
lines into several ones.

> Elisp advice? Any other function I should look into
> to "imitate" the style of a good use of prefix argument?

Any command that uses the fact of being invoked with a prefix argument
to modify its behavior will do.

Message #29 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>
To: Stefan Kangas <stefankangas <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: pbreton <at> cs.umb.edu, 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Sun, 9 Jun 2024 12:48:26 -0300

[Message part 1 (text/plain, inline)]

Hello Stefan and Eli,

Here I added the use of a prefix argument. I tested and seems to be
working fine ^_^. As a matter of fact, I think it's even better now!
Pretty handy thing.

I tried my best to break at the 80 character column, whilst
maintaining readability.
I also went back on the variable ping-program-options'
name change. Eli raised a good point.

Would appreciate some feedback.

Best regards,

- Fabrizio

[0001-ping-Added-optional-arguments.patch (text/x-patch, attachment)]

Message #32 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>
To: Stefan Kangas <stefankangas <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: pbreton <at> cs.umb.edu, 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Sun, 16 Jun 2024 18:46:46 -0300

Hello Stefan and Eli,

I am sorry to bother you again. I know you two must be very busy.

Did you have the chance to look at the patch?
The patch did not have the changes to changelog because I was
unsure if there were going to be any additional comments regarding
the e-lisp code.
Should I send a patch with the changelog modified?

Best regards,

Thank you for all of you work

- Fabrizio

Message #35 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>, Eli Zaretskii <eliz <at> gnu.org>
Cc: pbreton <at> cs.umb.edu, 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Sun, 16 Jun 2024 18:14:59 -0700

TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar> writes:

> Did you have the chance to look at the patch?
> The patch did not have the changes to changelog because I was
> unsure if there were going to be any additional comments regarding
> the e-lisp code.
> Should I send a patch with the changelog modified?

Sorry for the late reply.

I think this patch (to be applied on top of yours) might provide a
slighly better experience.  I also made an attempt at improving the
docstring.  What do you think?

diff --git a/lisp/net/net-utils.el b/lisp/net/net-utils.el
index c37d9976cf2..8fb417da241 100644
--- a/lisp/net/net-utils.el
+++ b/lisp/net/net-utils.el
@@ -425,24 +425,30 @@ traceroute
      options)))

 ;;;###autoload
-(defun ping (host &optional options)
-  "Ping HOST.
-Optional argument OPTIONS sets which options will be passed to `ping-program'
-With a \\[universal-argument] prefix arg, prompt the user for OPTIONS.
-If called interactively with no prefix arg, then `ping-program-options'
-will be used.
-If OPTIONS is not set, then `ping-program-options' will be used.
-If your system's ping continues until interrupted, you can try setting
-`ping-program-options'."
+(defun ping (host &optional flags)
+  "Ping HOST using `ping-program'.
+
+The user option `ping-program-options' is passed as flags to
+`ping-program'.  With a \\[universal-argument] prefix arg, prompt the
+user for the flags to pass.
+
+When called from Lisp, the optional argument FLAGS, if non-nil, is a
+list of strings that will be passed as flags for the `ping-program'.  If
+FLAGS is nil, `ping-program-options' will be used.
+
+If your system's ping continues until interrupted, you can try using a
+prefix argument or setting `ping-program-options'."
   (interactive
    (list (let ((default (ffap-machine-at-point)))
            (read-string (format-prompt "Ping host" default) nil nil default))
-         (if current-prefix-arg (split-string (read-string
-            (format-prompt "Ping options (RET for defaults)" nil) nil
nil nil) " "))))
+         (when current-prefix-arg
+           (split-string
+            (read-string (format-prompt "Ping options" ping-program-options)
+                         nil nil ping-program-options)))))
   (let ((full-command
-	 (if (or (equal options (list "")) (not options))
+	 (if (or (equal flags (list "")) (not flags))
 	     (append ping-program-options (list host))
-	     (append options (list host)))))
+	     (append flags (list host)))))
     (net-utils-run-program
      (concat "Ping" " " host)
      (concat "** Ping ** " ping-program " ** " host)

Message #38 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: pbreton <at> cs.umb.edu, Eli Zaretskii <eliz <at> gnu.org>, 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Sun, 16 Jun 2024 23:03:23 -0300

Hello Stefan,

> Sorry for the late reply.
No worries Stefan! I'm glad you are taking the time to help me with the patch.
Thank you for all of the work you do!

> I think this patch (to be applied on top of yours) might provide a
> slighly better experience.  I also made an attempt at improving the
> docstring.  What do you think?
The docstring is greatly improved; without a doubt.

However, I've been thinking. Both my original patch and yours
ask the user for additional flags if *any* prefix argument is
passed.
Isn't that closing the door for further additions to this
function?

Maybe, it would be better if (for instance) "0" is passed
as a prefix argument.
In that way, if any other additions were to be made,
those could use a different prefix argument (1,2,3, and so on);
thus avoiding backwards incompatibility.

So, instead of using when/if; cond could be used.

What do you think?

Best regards,

- Fabrizio

Message #41 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>
Cc: pbreton <at> cs.umb.edu, Eli Zaretskii <eliz <at> gnu.org>, 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Sun, 16 Jun 2024 23:19:59 -0700

TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar> writes:

> However, I've been thinking. Both my original patch and yours
> ask the user for additional flags if *any* prefix argument is
> passed.
> Isn't that closing the door for further additions to this
> function?
>
> Maybe, it would be better if (for instance) "0" is passed
> as a prefix argument.
> In that way, if any other additions were to be made,
> those could use a different prefix argument (1,2,3, and so on);
> thus avoiding backwards incompatibility.

This is not a problem, and we can easily make such changes later.

One prefix argument is equivalent to 4 (C-u C-u is 16, and so on).
See (info "(elisp) Prefix Command Arguments").

Message #44 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: pbreton <at> cs.umb.edu, Eli Zaretskii <eliz <at> gnu.org>, 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Tue, 18 Jun 2024 23:17:14 -0300

[Message part 1 (text/plain, inline)]

Hello Stefan and Eli,

I apologize for my late reply. I wanted to thank you both for
your work and patience.

And Stefan for teaching me the committing etiquette.

I added a "NetUtils" heading in etc/NEWS, which
I am a little hesitant about. I was torn between
creating a new Heading and using "Miscellaneous".

I am looking forward to your feedback.

Have a good week,

- Fabrizio

[0003-ping-can-now-receive-a-prefix-argument.patch (text/x-patch, attachment)]

Message #47 received at 71438 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar>
Cc: pbreton <at> cs.umb.edu, Eli Zaretskii <eliz <at> gnu.org>, 71438 <at> debbugs.gnu.org
Subject: Re: bug#71438: [PATCH] Allow ping to receive optional arguments
Date: Thu, 20 Jun 2024 18:45:34 +0000

close 71438 30.1
thanks

TOMAS FABRIZIO ORSI <torsi <at> fi.uba.ar> writes:

> Hello Stefan and Eli,
>
> I apologize for my late reply. I wanted to thank you both for
> your work and patience.
>
> And Stefan for teaching me the committing etiquette.
>
> I added a "NetUtils" heading in etc/NEWS, which
> I am a little hesitant about. I was torn between
> creating a new Heading and using "Miscellaneous".
>
> I am looking forward to your feedback.

I've installed it on master as commit 72f2b01e318 with some minor
touch-ups.  I'm consequently closing this bug.

This brings us to the limit of what we can accept from you without a
copyright assignment to the Free Software Foundation.  Please let us
know if you plan any further contributions, and we can send you the form
to get you started with the copyright assignment process.

Thanks for contributing to Emacs!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.