Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Basil L. Contovounesios]

ohwoeowho@gmail.com (Oleh Krehel) writes:

> branch: master
> commit fd5410217ff23810edc16e97c10934ad622f8e4b
> Author: Oleh Krehel <ohwoeowho@gmail.com>
> Commit: Oleh Krehel <ohwoeowho@gmail.com>
>
>     * lisp/files.el (file-size-function): New defcustom

[I wish changes like this (and its recent predecessors, which caused
 test failues that others had to fix), even if small, to central
 user-facing features were instead discussed and tested a little before
 being unilaterally pushed.]

> ---
>  etc/NEWS      |  1 +
>  lisp/files.el | 10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/etc/NEWS b/etc/NEWS
> index c875fc6..5cbe60c 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -400,6 +400,7 @@ mode they are described in the manual "(emacs) Display".
>  ---
>  ** New variable 'xref-file-name-display' controls the display of file
>  names in xref buffers.
> +** New variable `file-size-function' controls how file sizes are displayed

Which file sizes?

>  \f
>  * Editing Changes in Emacs 27.1
> diff --git a/lisp/files.el b/lisp/files.el
> index 238aa37..e26b482 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -6696,13 +6696,21 @@ This variable is obsolete; Emacs no longer uses it."
>  			"ignored, as Emacs uses `file-system-info' instead"
>  			"27.1")
>  
> +(defcustom file-size-function #'file-size-human-readable
> +  "Function that transforms the number of bytes into a human-readable string."

The number of which bytes?  I think a phrase similar to "for display"
would be more accurate than "human-readable" here.

> +  :type '(choice

Did you try radio+function-item instead of choice+const?  I usually find
the former nicer, as recommended in (info "(elisp) Composite Types").

> +          (const :tag "default" file-size-human-readable)
> +          (const :tag "iec"

Nit: Please capitalise and uppercase these tags, respectively.

> +           (lambda (size) (file-size-human-readable size 'iec " ")))

Please do not use an unevaluated anonymous function here.

> +          (function :tag "Custom function")))
>
>  (defun get-free-disk-space (dir)
>    "String describing the amount of free space on DIR's file system.
>  If DIR's free space cannot be obtained, this function returns nil."
>    (save-match-data
>      (let ((avail (nth 2 (file-system-info dir))))
>        (if avail
> -          (file-size-human-readable avail 'iec " ")))))
> +          (funcall file-size-function avail)))))
>  
>  ;; The following expression replaces `dired-move-to-filename-regexp'.
>  (defvar directory-listing-before-filename-regexp

Thanks for working on this,

-- 
Basil

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Sat, 20 Jul 2019 17:53:13 +0100
> Cc: Oleh Krehel <ohwoeowho@gmail.com>
> 
> ohwoeowho@gmail.com (Oleh Krehel) writes:
> 
> > branch: master
> > commit fd5410217ff23810edc16e97c10934ad622f8e4b
> > Author: Oleh Krehel <ohwoeowho@gmail.com>
> > Commit: Oleh Krehel <ohwoeowho@gmail.com>
> >
> >     * lisp/files.el (file-size-function): New defcustom
> 
> [I wish changes like this (and its recent predecessors, which caused
>  test failues that others had to fix), even if small, to central
>  user-facing features were instead discussed and tested a little before
>  being unilaterally pushed.]

Seconded.

> > +(defcustom file-size-function #'file-size-human-readable
> > +  "Function that transforms the number of bytes into a human-readable string."
> 
> The number of which bytes?  I think a phrase similar to "for display"
> would be more accurate than "human-readable" here.
> 
> > +  :type '(choice
> 
> Did you try radio+function-item instead of choice+const?  I usually find
> the former nicer, as recommended in (info "(elisp) Composite Types").
> 
> > +          (const :tag "default" file-size-human-readable)
> > +          (const :tag "iec"
> 
> Nit: Please capitalise and uppercase these tags, respectively.
> 
> > +           (lambda (size) (file-size-human-readable size 'iec " ")))
> 
> Please do not use an unevaluated anonymous function here.
> 
> > +          (function :tag "Custom function")))

This defcustom also needs a :version tag.

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Oleh Krehel]

Hi all,

> > [I wish changes like this (and its recent predecessors, which caused
> >  test failues that others had to fix)

Sorry about that. I didn't realize where the tests were, since there's
no "make test" at top-level.

> > even if small, to central
> >  user-facing features were instead discussed and tested a little before
> >  being unilaterally pushed.]
>
> Seconded.

While I agree in general, in this case the user-visible change was
done around a year ago, and I noticed only now when I switched to
using Emacs27. The change from a year ago was displaying the amount of
free space in kilobytes, which is almost unusable.
I added a custom var with a reasonable default that any user can
easily change. Very uncontroversial change, IMO.

> > Did you try radio+function-item instead of choice+const?  I usually find
> > the former nicer, as recommended in (info "(elisp) Composite Types").

OK.

> > > +          (const :tag "default" file-size-human-readable)
> > > +          (const :tag "iec"
> >
> > Nit: Please capitalise and uppercase these tags, respectively.

OK.

> > > +           (lambda (size) (file-size-human-readable size 'iec " ")))
> >
> > Please do not use an unevaluated anonymous function here.

OK.

> This defcustom also needs a :version tag.

OK. I've pushed the requested changes.

regards,
Oleh

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Eli Zaretskii]

> From: Oleh Krehel <ohwoeowho@gmail.com>
> Date: Sat, 20 Jul 2019 19:38:28 +0200
> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>,
>  emacs-devel <emacs-devel@gnu.org>
> 
> > > even if small, to central
> > >  user-facing features were instead discussed and tested a little before
> > >  being unilaterally pushed.]
> >
> > Seconded.
> 
> While I agree in general, in this case the user-visible change was
> done around a year ago, and I noticed only now when I switched to
> using Emacs27. The change from a year ago was displaying the amount of
> free space in kilobytes, which is almost unusable.

Which change was that?

> I added a custom var with a reasonable default that any user can
> easily change. Very uncontroversial change, IMO.

As you see, the change turned out to be at least somewhat
controversial.

IME, it is always best to discuss these issues first.  It delays
pushing a change, but that is not a problem for the master branch.

Thanks.

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Oleh Krehel]

> > While I agree in general, in this case the user-visible change was
> > done around a year ago, and I noticed only now when I switched to
> > using Emacs27. The change from a year ago was displaying the amount of
> > free space in kilobytes, which is almost unusable.
>
> Which change was that?

The change which obsoleted `directory-free-space-args'. I was using
the setting "-Pmh" to give me a reasonable output. But after the
variable became obsolete, no replacement was added to allow for the
same nice output of free space in gigabytes.

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Eli Zaretskii]

> From: Oleh Krehel <ohwoeowho@gmail.com>
> Date: Sat, 20 Jul 2019 19:50:21 +0200
> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>, emacs-devel <emacs-devel@gnu.org>
> 
> > > While I agree in general, in this case the user-visible change was
> > > done around a year ago, and I noticed only now when I switched to
> > > using Emacs27. The change from a year ago was displaying the amount of
> > > free space in kilobytes, which is almost unusable.
> >
> > Which change was that?
> 
> The change which obsoleted `directory-free-space-args'. I was using
> the setting "-Pmh" to give me a reasonable output. But after the
> variable became obsolete, no replacement was added to allow for the
> same nice output of free space in gigabytes.

Having a change to fix some past changes means there's more reasons
for discussion, not less.  Because someone already made a decision,
and they might have their reasons.  Discarding those reasons without a
discussion might have adverse effects on some use cases.

So please in the future do post such changes and ask for comments.
TIA

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Basil L. Contovounesios]

Oleh Krehel <ohwoeowho@gmail.com> writes:

>> > [I wish changes like this (and its recent predecessors, which caused
>> >  test failues that others had to fix)
>
> Sorry about that. I didn't realize where the tests were, since there's
> no "make test" at top-level.

There's 'make check' and the file test/README, as mentioned under
"Testing your changes" in CONTRIBUTE.

>> > even if small, to central
>> >  user-facing features were instead discussed and tested a little before
>> >  being unilaterally pushed.]
>>
>> Seconded.
>
> While I agree in general, in this case the user-visible change was
> done around a year ago, and I noticed only now when I switched to
> using Emacs27. The change from a year ago was displaying the amount of
> free space in kilobytes, which is almost unusable.
> I added a custom var with a reasonable default that any user can
> easily change. Very uncontroversial change, IMO.

New user options in central places like files.el (as opposed to some
specialised package) usually warrant a RFC so as to establish which need
they are addressing, whether this need masks some other issue, and
whether the proposed change covers this need sufficiently well.

Internal variables can come and go, but user options are user-facing
contracts that are harder to change or get rid of.

>> > Did you try radio+function-item instead of choice+const?  I usually find
>> > the former nicer, as recommended in (info "(elisp) Composite Types").
>
> OK.
>
>> > > +          (const :tag "default" file-size-human-readable)
>> > > +          (const :tag "iec"
>> >
>> > Nit: Please capitalise and uppercase these tags, respectively.
>
> OK.
>
>> > > +           (lambda (size) (file-size-human-readable size 'iec " ")))
>> >
>> > Please do not use an unevaluated anonymous function here.
>
> OK.

Further to [1], I should clarify that anonymous functions shouldn't be
used as values in a user option's :type at all, regardless of whether
the function is evaluated.

[1]: * lisp/files.el (file-size-function): Add :version tag
45fc6f203e 2019-07-20 19:31:07 +0200
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=45fc6f203e2fef528cb2bb0d7c0140e160c974e2

>> This defcustom also needs a :version tag.
>
> OK. I've pushed the requested changes.

Thanks,

-- 
Basil

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

20 juli 2019 kl. 20.50 skrev Basil L. Contovounesios <contovob@tcd.ie>:
> 
> New user options in central places like files.el (as opposed to some
> specialised package) usually warrant a RFC so as to establish which need
> they are addressing, whether this need masks some other issue, and
> whether the proposed change covers this need sufficiently well.
> 
> Internal variables can come and go, but user options are user-facing
> contracts that are harder to change or get rid of.

Agreed, and this defcustom (file-size-function) has a much more general-sounding name and description than merited by its effects (which are limited to the "available disk space" number in Dired and that's it).

Attached is a minimal clean-up which remedies some of the flaws, but perhaps we should instead roll back the defcustom and all related changes entirely until a cohesive proposal is forthcoming, as per your and Eli's comments. Fighting it out in the source isn't really going to be productive.


[-- Attachment #2: 0001-Clean-up-file-size-function-with-better-default-valu.patch --]
[-- Type: application/octet-stream, Size: 1675 bytes --]

From e48fd413e4bb70829c4a5783198facdb6422a2b2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 22 Jul 2019 17:10:37 +0200
Subject: [PATCH] Clean up file-size-function, with better default value

* lisp/files.el (size-human-readable-iec): New.
(file-size-function): Better default value.  Eliminate lambda.
Better default for custom choice.  Put in group `dired'.
---
 lisp/files.el | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 70865ebcdf..868e23f0d1 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6698,13 +6698,17 @@ directory-free-space-args
 			"ignored, as Emacs uses `file-system-info' instead"
 			"27.1")
 
-(defcustom file-size-function #'file-size-human-readable
+(defun size-human-readable-iec (size)
+  "Human-readable string for SIZE bytes, using IEC prefixes."
+  (file-size-human-readable size 'iec " "))
+
+(defcustom file-size-function #'size-human-readable-iec
   "Function that transforms the number of bytes into a human-readable string."
   :type `(radio
-          (function-item :tag "Default" file-size-human-readable)
-          (function-item :tag "IEC"
-                         ,(lambda (size) (file-size-human-readable size 'iec " ")))
-          (function :tag "Custom function"))
+          (function-item :tag "IEC" size-human-readable-iec)
+          (function-item :tag "Traditional" file-size-human-readable)
+          (function :tag "Custom function" number-to-string))
+  :group 'dired
   :version "27.1")
 
 (defun get-free-disk-space (dir)
-- 
2.20.1 (Apple Git-117)


[-- Attachment #3: Type: text/plain, Size: 2 bytes --]

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Oleh Krehel]

> Agreed, and this defcustom (file-size-function) has a much more general-sounding name and description than merited by its effects (which are limited to the "available disk space" number in Dired and that's it).

The name is fine in my opinion. It's a derivation of
`file-size-human-readable'. The postfix "-function" means it's a
customizable variable that will be `funcall'-ed. Here's how it works:

    (defcustom ediff-window-setup-function #'ediff-setup-windows-default
      "..."
      :type '(choice
              (const ediff-setup-windows-default)
              (const ediff-setup-windows-multiframe)
              (const ediff-setup-windows-plain)
              (function :tag "Other function")))

Ideally, just from the name `ediff-window-setup-function', we know
which values it can assume.  As is the case for `file-size-function':
it can assume a value 'file-size-human-readable', they share the same
prefix.

In addition, the function is used for available space in Dired only
*now*. In the future it can be used in other places as well. And the
users will benefit of being able to customize how file sizes are
shown.

> Attached is a minimal clean-up which remedies some of the flaws, but perhaps we should instead roll back the defcustom and all related changes entirely until a cohesive proposal is forthcoming, as per your and Eli's comments. Fighting it out in the source isn't really going to be productive.

Yet it's exactly what you're trying to do with your patch! Once again,
you try to have the default format be:

    total used in directory 153M available 496.4 GiB

in place of the more common one produced by "df -kh ." or "du -hs .".

    total used in directory 153M available 496.4G

regards,
Oleh

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Stefan Monnier]

>> Agreed, and this defcustom (file-size-function) has a much more
>> general-sounding name and description than merited by its effects (which
>> are limited to the "available disk space" number in Dired and that's it).
> The name is fine in my opinion. It's a derivation of
> `file-size-human-readable'. The postfix "-function" means it's a
> customizable variable that will be `funcall'-ed. Here's how it works:

FWIW, I agree that the name is slightly misleading, in that it seems to
expect a function which takes a file name and returns the size of the
file (most naturally represented as an integer number of bytes).

So maybe something like `file-size-human-readable-function` is a better
choice (then make file-size-human-readable call it and rename the
current body of file-size-human-readable to something else,
e.g. file-size-human-readable--default tho maybe that name shouldn't
include "file" since it can also be used for other sizes).


        Stefan

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]

22 juli 2019 kl. 22.14 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
> 
> FWIW, I agree that the name is slightly misleading, in that it seems to
> expect a function which takes a file name and returns the size of the
> file (most naturally represented as an integer number of bytes).
> 
> So maybe something like `file-size-human-readable-function` is a better
> choice (then make file-size-human-readable call it and rename the
> current body of file-size-human-readable to something else,
> e.g. file-size-human-readable--default tho maybe that name shouldn't
> include "file" since it can also be used for other sizes).

Right, it is also intended for describing disk space, memory usage, etc. Given that, I ended up with `byte-count-to-string-function' which is passable but doesn't feel entirely satisfactory. Better name suggestions are welcome.

Since `file-size-human-readable' is used outside Emacs and a change in semantics might break assumptions, I left it untouched. Instead, most calls to that function were replaced with (function byte-count-to-string SIZE). If you would rather that we add a function for this (byte-count-to-string, maybe) instead of using the variable directly, this can be arranged.

22 juli 2019 kl. 19.29 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> Thanks.  Please consider calling this out in NEWS and maybe also in
> the ELisp manual.

NEWS updated; thanks for reminding me. I couldn't find a natural place for it in the manual.

Oleh, you had strong personal preferences for a particular format, and the customisable variable should now accommodate your wishes in more places than before. The default format was chosen for readability and standards compliance, and for being unambiguous.


[-- Attachment #2: 0001-Clean-up-file-size-function.patch --]
[-- Type: application/octet-stream, Size: 8651 bytes --]

From b183cebae24e1d7611f57c0336efd973049df340 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 22 Jul 2019 17:10:37 +0200
Subject: [PATCH] Clean up file-size-function

* lisp/files.el (file-size-human-readable-iec): New.
(file-size-function): Rename to byte-count-to-string-function.  Better
default value.  Eliminate lambda.  Better default for custom choice.
Put in group `files'.  More descriptive doc string.  Move.
(out-of-memory-warning-percentage, warn-maybe-out-of-memory)
(get-free-disk-space):
* lisp/dired.el (dired-number-of-marked-files):
* lisp/url/url-http.el (url-http-simple-after-change-function)
(url-http-content-length-after-change-function):
Use byte-count-to-string-function.
* test/lisp/files-test.el (files-test-file-size-human-readable):
Test file-size-human-readable-iec.
---
 etc/NEWS                 |  3 ++-
 lisp/dired.el            |  4 ++--
 lisp/files.el            | 34 ++++++++++++++++++++--------------
 lisp/url/url-http.el     | 10 +++++-----
 test/lisp/files-tests.el |  7 ++++++-
 5 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 5673fc1b42..2fc52bea52 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -405,7 +405,8 @@ mode they are described in the manual "(emacs) Display".
 ** New variable 'xref-file-name-display' controls the display of file
 names in xref buffers.
 
-** New variable `file-size-function' controls how file sizes are displayed.
+** New customizable variable 'byte-count-to-string-function'.
+It is used for displaying file sizes and disk space in some cases.
 
 \f
 * Editing Changes in Emacs 27.1
diff --git a/lisp/dired.el b/lisp/dired.el
index c455a5cde4..640d62fd41 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -3630,12 +3630,12 @@ dired-number-of-marked-files
                         sum (file-attribute-size (file-attributes file)))))
     (if (zerop nmarked)
         (message "No marked files"))
-    (message "%d marked file%s (%sB total size)"
+    (message "%d marked file%s (%s total size)"
              nmarked
              (if (= nmarked 1)
                  ""
                "s")
-             (file-size-human-readable size))))
+             (funcall byte-count-to-string-function size))))
 
 (defun dired-mark-files-containing-regexp (regexp &optional marker-char)
   "Mark all files with contents containing REGEXP for use in later commands.
diff --git a/lisp/files.el b/lisp/files.el
index 81ca948bd2..728b0c28f0 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1419,6 +1419,21 @@ file-size-human-readable
               (if (string= prefixed-unit "") "" (or space ""))
               prefixed-unit))))
 
+(defun file-size-human-readable-iec (size)
+  "Human-readable string for SIZE bytes, using IEC prefixes."
+  (file-size-human-readable size 'iec " "))
+
+(defcustom byte-count-to-string-function #'file-size-human-readable-iec
+  "Function that turns a number of bytes into a human-readable string.
+It is for use when displaying file sizes and disk space where other
+constraints do not force a specific format."
+  :type `(radio
+          (function-item :tag "IEC" file-size-human-readable-iec)
+          (function-item :tag "Traditional" file-size-human-readable)
+          (function :tag "Custom function" number-to-string))
+  :group 'files
+  :version "27.1")
+
 (defcustom mounted-file-systems
   (if (memq system-type '(windows-nt cygwin))
       "^//[^/]+/"
@@ -2086,7 +2101,7 @@ out-of-memory-warning-percentage
 (defun files--ask-user-about-large-file (size op-type filename offer-raw)
   (let ((prompt (format "File %s is large (%s), really %s?"
 		        (file-name-nondirectory filename)
-		        (file-size-human-readable size 'iec " ") op-type)))
+		        (funcall byte-count-to-string-function size) op-type)))
     (if (not offer-raw)
         (if (y-or-n-p prompt) nil 'abort)
       (let* ((use-dialog (and (display-popup-menus-p)
@@ -2138,10 +2153,10 @@ warn-maybe-out-of-memory
 exceeds the %S%% of currently available free memory (%s).
 If that fails, try to open it with `find-file-literally'
 \(but note that some characters might be displayed incorrectly)."
-	     (file-size-human-readable size 'iec " ")
+	     (funcall byte-count-to-string-function size)
 	     out-of-memory-warning-percentage
-	     (file-size-human-readable (* total-free-memory 1024)
-                                       'iec " "))))))))
+	     (funcall byte-count-to-string-function
+                      (* total-free-memory 1024)))))))))
 
 (defun files--message (format &rest args)
   "Like `message', except sometimes don't print to minibuffer.
@@ -6698,22 +6713,13 @@ directory-free-space-args
 			"ignored, as Emacs uses `file-system-info' instead"
 			"27.1")
 
-(defcustom file-size-function #'file-size-human-readable
-  "Function that transforms the number of bytes into a human-readable string."
-  :type `(radio
-          (function-item :tag "Default" file-size-human-readable)
-          (function-item :tag "IEC"
-                         ,(lambda (size) (file-size-human-readable size 'iec " ")))
-          (function :tag "Custom function"))
-  :version "27.1")
-
 (defun get-free-disk-space (dir)
   "String describing the amount of free space on DIR's file system.
 If DIR's free space cannot be obtained, this function returns nil."
   (save-match-data
     (let ((avail (nth 2 (file-system-info dir))))
       (if avail
-          (funcall file-size-function avail)))))
+          (funcall byte-count-to-string-function avail)))))
 
 ;; The following expression replaces `dired-move-to-filename-regexp'.
 (defvar directory-listing-before-filename-regexp
diff --git a/lisp/url/url-http.el b/lisp/url/url-http.el
index 527760118d..e34f90f6f3 100644
--- a/lisp/url/url-http.el
+++ b/lisp/url/url-http.el
@@ -1017,7 +1017,7 @@ url-http-simple-after-change-function
   ;; Function used when we do NOT know how long the document is going to be
   ;; Just _very_ simple 'downloaded %d' type of info.
   (url-lazy-message "Reading %s..."
-                    (file-size-human-readable (buffer-size) 'iec " ")))
+                    (funcall byte-count-to-string-function (buffer-size))))
 
 (defun url-http-content-length-after-change-function (_st nd _length)
   "Function used when we DO know how long the document is going to be.
@@ -1030,16 +1030,16 @@ url-http-content-length-after-change-function
        (url-percentage (- nd url-http-end-of-headers)
 		       url-http-content-length)
        url-http-content-type
-       (file-size-human-readable (- nd url-http-end-of-headers) 'iec " ")
-       (file-size-human-readable url-http-content-length 'iec " ")
+       (funcall byte-count-to-string-function (- nd url-http-end-of-headers))
+       (funcall byte-count-to-string-function url-http-content-length)
        (url-percentage (- nd url-http-end-of-headers)
 		       url-http-content-length))
     (url-display-percentage
      "Reading... %s of %s (%d%%)"
      (url-percentage (- nd url-http-end-of-headers)
 		     url-http-content-length)
-     (file-size-human-readable (- nd url-http-end-of-headers) 'iec " ")
-     (file-size-human-readable url-http-content-length 'iec " ")
+     (funcall byte-count-to-string-function (- nd url-http-end-of-headers))
+     (funcall byte-count-to-string-function url-http-content-length)
      (url-percentage (- nd url-http-end-of-headers)
 		     url-http-content-length)))
 
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index df2c3f47ae..ed23f7675c 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1280,7 +1280,12 @@ files-tests-file-attributes-equal
   (should (equal (file-size-human-readable 4294967296 'iec " ") "4 GiB"))
   (should (equal (file-size-human-readable 10000 nil " " "bit") "9.8 kbit"))
   (should (equal (file-size-human-readable 10000 'si " " "bit") "10 kbit"))
-  (should (equal (file-size-human-readable 10000 'iec " " "bit") "9.8 Kibit")))
+  (should (equal (file-size-human-readable 10000 'iec " " "bit") "9.8 Kibit"))
+
+  (should (equal (file-size-human-readable-iec 0) "0 B"))
+  (should (equal (file-size-human-readable-iec 1) "1 B"))
+  (should (equal (file-size-human-readable-iec 9621) "9.4 KiB"))
+  (should (equal (file-size-human-readable-iec 72528034765) "67.5 GiB")))
 
 (ert-deftest files-test-magic-mode-alist-re-baseline ()
   "Test magic-mode-alist with RE, expected behaviour for match."
-- 
2.20.1 (Apple Git-117)

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Basil L. Contovounesios]

Mattias Engdegård <mattiase@acm.org> writes:

> Right, it is also intended for describing disk space, memory usage, etc. Given
> that, I ended up with `byte-count-to-string-function' which is passable but
> doesn't feel entirely satisfactory. Better name suggestions are welcome.

Only some minor nits from me:

> +(defun file-size-human-readable-iec (size)
> +  "Human-readable string for SIZE bytes, using IEC prefixes."
> +  (file-size-human-readable size 'iec " "))
> +
> +(defcustom byte-count-to-string-function #'file-size-human-readable-iec
> +  "Function that turns a number of bytes into a human-readable string.
> +It is for use when displaying file sizes and disk space where other
> +constraints do not force a specific format."
> +  :type `(radio

The backtick no longer seems necessary.

> +          (function-item :tag "IEC" file-size-human-readable-iec)
> +          (function-item :tag "Traditional" file-size-human-readable)

Isn't :tag a no-op for function-items?

> +          (function :tag "Custom function" number-to-string))

This seems to work, but my reading of the manual is that
number-to-string must be given as the value of the :value tag, not on
its own.  Does the manual need updating in this regard?

Thanks,

-- 
Basil

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

26 juli 2019 kl. 16.39 skrev Basil L. Contovounesios <contovob@tcd.ie>:
> 
> The backtick no longer seems necessary.

Fixed.

> Isn't :tag a no-op for function-items?

Correct; removed.

>> +          (function :tag "Custom function" number-to-string))
> 
> This seems to work, but my reading of the manual is that
> number-to-string must be given as the value of the :value tag, not on
> its own.  Does the manual need updating in this regard?

Don't know if it was intended behaviour, but I've added the :value keyword in the patch.

Thank you!


[-- Attachment #2: 0001-Clean-up-file-size-function.patch --]
[-- Type: application/octet-stream, Size: 8628 bytes --]

From bcbb6b49d78e1f3690ef90052e5c49a438b38ef2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 22 Jul 2019 17:10:37 +0200
Subject: [PATCH] Clean up file-size-function

* lisp/files.el (file-size-human-readable-iec): New.
(file-size-function): Rename to byte-count-to-string-function.  Better
default value.  Eliminate lambda.  Better default for custom choice.
Put in group `files'.  More descriptive doc string.  Move.
(out-of-memory-warning-percentage, warn-maybe-out-of-memory)
(get-free-disk-space):
* lisp/dired.el (dired-number-of-marked-files):
* lisp/url/url-http.el (url-http-simple-after-change-function)
(url-http-content-length-after-change-function):
Use byte-count-to-string-function.
* test/lisp/files-test.el (files-test-file-size-human-readable):
Test file-size-human-readable-iec.
---
 etc/NEWS                 |  3 ++-
 lisp/dired.el            |  4 ++--
 lisp/files.el            | 34 ++++++++++++++++++++--------------
 lisp/url/url-http.el     | 10 +++++-----
 test/lisp/files-tests.el |  7 ++++++-
 5 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 5673fc1b42..2fc52bea52 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -405,7 +405,8 @@ mode they are described in the manual "(emacs) Display".
 ** New variable 'xref-file-name-display' controls the display of file
 names in xref buffers.
 
-** New variable `file-size-function' controls how file sizes are displayed.
+** New customizable variable 'byte-count-to-string-function'.
+It is used for displaying file sizes and disk space in some cases.
 
 \f
 * Editing Changes in Emacs 27.1
diff --git a/lisp/dired.el b/lisp/dired.el
index c455a5cde4..640d62fd41 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -3630,12 +3630,12 @@ dired-number-of-marked-files
                         sum (file-attribute-size (file-attributes file)))))
     (if (zerop nmarked)
         (message "No marked files"))
-    (message "%d marked file%s (%sB total size)"
+    (message "%d marked file%s (%s total size)"
              nmarked
              (if (= nmarked 1)
                  ""
                "s")
-             (file-size-human-readable size))))
+             (funcall byte-count-to-string-function size))))
 
 (defun dired-mark-files-containing-regexp (regexp &optional marker-char)
   "Mark all files with contents containing REGEXP for use in later commands.
diff --git a/lisp/files.el b/lisp/files.el
index 81ca948bd2..34a2b1d936 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1419,6 +1419,21 @@ file-size-human-readable
               (if (string= prefixed-unit "") "" (or space ""))
               prefixed-unit))))
 
+(defun file-size-human-readable-iec (size)
+  "Human-readable string for SIZE bytes, using IEC prefixes."
+  (file-size-human-readable size 'iec " "))
+
+(defcustom byte-count-to-string-function #'file-size-human-readable-iec
+  "Function that turns a number of bytes into a human-readable string.
+It is for use when displaying file sizes and disk space where other
+constraints do not force a specific format."
+  :type '(radio
+          (function-item file-size-human-readable-iec)
+          (function-item file-size-human-readable)
+          (function :tag "Custom function" :value number-to-string))
+  :group 'files
+  :version "27.1")
+
 (defcustom mounted-file-systems
   (if (memq system-type '(windows-nt cygwin))
       "^//[^/]+/"
@@ -2086,7 +2101,7 @@ out-of-memory-warning-percentage
 (defun files--ask-user-about-large-file (size op-type filename offer-raw)
   (let ((prompt (format "File %s is large (%s), really %s?"
 		        (file-name-nondirectory filename)
-		        (file-size-human-readable size 'iec " ") op-type)))
+		        (funcall byte-count-to-string-function size) op-type)))
     (if (not offer-raw)
         (if (y-or-n-p prompt) nil 'abort)
       (let* ((use-dialog (and (display-popup-menus-p)
@@ -2138,10 +2153,10 @@ warn-maybe-out-of-memory
 exceeds the %S%% of currently available free memory (%s).
 If that fails, try to open it with `find-file-literally'
 \(but note that some characters might be displayed incorrectly)."
-	     (file-size-human-readable size 'iec " ")
+	     (funcall byte-count-to-string-function size)
 	     out-of-memory-warning-percentage
-	     (file-size-human-readable (* total-free-memory 1024)
-                                       'iec " "))))))))
+	     (funcall byte-count-to-string-function
+                      (* total-free-memory 1024)))))))))
 
 (defun files--message (format &rest args)
   "Like `message', except sometimes don't print to minibuffer.
@@ -6698,22 +6713,13 @@ directory-free-space-args
 			"ignored, as Emacs uses `file-system-info' instead"
 			"27.1")
 
-(defcustom file-size-function #'file-size-human-readable
-  "Function that transforms the number of bytes into a human-readable string."
-  :type `(radio
-          (function-item :tag "Default" file-size-human-readable)
-          (function-item :tag "IEC"
-                         ,(lambda (size) (file-size-human-readable size 'iec " ")))
-          (function :tag "Custom function"))
-  :version "27.1")
-
 (defun get-free-disk-space (dir)
   "String describing the amount of free space on DIR's file system.
 If DIR's free space cannot be obtained, this function returns nil."
   (save-match-data
     (let ((avail (nth 2 (file-system-info dir))))
       (if avail
-          (funcall file-size-function avail)))))
+          (funcall byte-count-to-string-function avail)))))
 
 ;; The following expression replaces `dired-move-to-filename-regexp'.
 (defvar directory-listing-before-filename-regexp
diff --git a/lisp/url/url-http.el b/lisp/url/url-http.el
index 527760118d..e34f90f6f3 100644
--- a/lisp/url/url-http.el
+++ b/lisp/url/url-http.el
@@ -1017,7 +1017,7 @@ url-http-simple-after-change-function
   ;; Function used when we do NOT know how long the document is going to be
   ;; Just _very_ simple 'downloaded %d' type of info.
   (url-lazy-message "Reading %s..."
-                    (file-size-human-readable (buffer-size) 'iec " ")))
+                    (funcall byte-count-to-string-function (buffer-size))))
 
 (defun url-http-content-length-after-change-function (_st nd _length)
   "Function used when we DO know how long the document is going to be.
@@ -1030,16 +1030,16 @@ url-http-content-length-after-change-function
        (url-percentage (- nd url-http-end-of-headers)
 		       url-http-content-length)
        url-http-content-type
-       (file-size-human-readable (- nd url-http-end-of-headers) 'iec " ")
-       (file-size-human-readable url-http-content-length 'iec " ")
+       (funcall byte-count-to-string-function (- nd url-http-end-of-headers))
+       (funcall byte-count-to-string-function url-http-content-length)
        (url-percentage (- nd url-http-end-of-headers)
 		       url-http-content-length))
     (url-display-percentage
      "Reading... %s of %s (%d%%)"
      (url-percentage (- nd url-http-end-of-headers)
 		     url-http-content-length)
-     (file-size-human-readable (- nd url-http-end-of-headers) 'iec " ")
-     (file-size-human-readable url-http-content-length 'iec " ")
+     (funcall byte-count-to-string-function (- nd url-http-end-of-headers))
+     (funcall byte-count-to-string-function url-http-content-length)
      (url-percentage (- nd url-http-end-of-headers)
 		     url-http-content-length)))
 
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index df2c3f47ae..ed23f7675c 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1280,7 +1280,12 @@ files-tests-file-attributes-equal
   (should (equal (file-size-human-readable 4294967296 'iec " ") "4 GiB"))
   (should (equal (file-size-human-readable 10000 nil " " "bit") "9.8 kbit"))
   (should (equal (file-size-human-readable 10000 'si " " "bit") "10 kbit"))
-  (should (equal (file-size-human-readable 10000 'iec " " "bit") "9.8 Kibit")))
+  (should (equal (file-size-human-readable 10000 'iec " " "bit") "9.8 Kibit"))
+
+  (should (equal (file-size-human-readable-iec 0) "0 B"))
+  (should (equal (file-size-human-readable-iec 1) "1 B"))
+  (should (equal (file-size-human-readable-iec 9621) "9.4 KiB"))
+  (should (equal (file-size-human-readable-iec 72528034765) "67.5 GiB")))
 
 (ert-deftest files-test-magic-mode-alist-re-baseline ()
   "Test magic-mode-alist with RE, expected behaviour for match."
-- 
2.20.1 (Apple Git-117)

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Mon, 22 Jul 2019 17:26:10 +0200
> Cc: Oleh Krehel <ohwoeowho@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>         emacs-devel <emacs-devel@gnu.org>
> 
> Agreed, and this defcustom (file-size-function) has a much more general-sounding name and description than merited by its effects (which are limited to the "available disk space" number in Dired and that's it).
> 
> Attached is a minimal clean-up which remedies some of the flaws, but perhaps we should instead roll back the defcustom and all related changes entirely until a cohesive proposal is forthcoming, as per your and Eli's comments. Fighting it out in the source isn't really going to be productive.

Thanks.  Please consider calling this out in NEWS and maybe also in
the ELisp manual.

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

20 juli 2019 kl. 18.53 skrev Basil L. Contovounesios <contovob@tcd.ie>:
> 
> [I wish changes like this (and its recent predecessors, which caused
> test failues that others had to fix), even if small, to central
> user-facing features were instead discussed and tested a little before
> being unilaterally pushed.]

So sorry for breaking things, and I'll be more careful. Surely the right correction is to fix the test, unless the change itself somehow was deemed questionable (which I never saw any indication of)?

Proposed patch attached. With it applied, the test passes again.


[-- Attachment #2: 0001-Make-tramp-test-regexp-more-robust.patch --]
[-- Type: application/octet-stream, Size: 1003 bytes --]

From a6ef72c0b6bea52d359858e519ea050a071f1d1c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sat, 20 Jul 2019 19:16:38 +0200
Subject: [PATCH] Make tramp test regexp more robust

* test/lisp/net/tramp-tests.el (tramp-test17-insert-directory):
Match a greater variety of human-readable size values.
---
 test/lisp/net/tramp-tests.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index 64ba0414e8..1404ef39d5 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -2827,7 +2827,7 @@ tramp--test-print-duration
 	       (looking-at-p
 		(concat
 		 ;; There might be a summary line.
-		 "\\(total.+[[:digit:]]+[KMGTPEZY]?\n\\)?"
+		 "\\(total.+[[:digit:]]+ ?[kKMGTPEZY]?i?B?\n\\)?"
 		 ;; We don't know in which order ".", ".." and "foo" appear.
 		 (format
 		  "\\(.+ %s\\( ->.+\\)?\n\\)\\{%d\\}"
-- 
2.20.1 (Apple Git-117)

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Basil L. Contovounesios]

Mattias Engdegård <mattiase@acm.org> writes:

> Surely the right correction is to fix the test, unless the change
> itself somehow was deemed questionable (which I never saw any
> indication of)?

There was no indication because there was no discussion of the change to
begin with, AFAICT.

> Proposed patch attached. With it applied, the test passes again.

Thanks.  Note that tests are already passing, I think thanks to Glenn's
latest fix:

Update a tramp test for get-free-disk-space change
decfdb091a 2019-07-18 08:03:36 -0700
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=decfdb091ab94bc83564821d29ac0832b45c8d00

-- 
Basil

Re: [Emacs-diffs] master fd54102: * lisp/files.el (file-size-function): New defcustom

[Mattias Engdegård]

20 juli 2019 kl. 20.48 skrev Basil L. Contovounesios <contovob@tcd.ie>:
> 
> Thanks.  Note that tests are already passing, I think thanks to Glenn's
> latest fix:
> 
> Update a tramp test for get-free-disk-space change
> decfdb091a 2019-07-18 08:03:36 -0700
> https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=decfdb091ab94bc83564821d29ac0832b45c8d00

Ah yes, but insufficient for the IEC form which I introduced. I propose it to be the default, because it is more readable and unambiguous.