GNU bug report logs - #22457
24.5; [PATCH] `dired-mark-if' should not count non-changes

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 22457 in the body.
You can then email your comments to 22457 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: Drew Adams <drew.adams <at> oracle.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.5; [PATCH] `dired-mark-if' should not count non-changes
Date: Sun, 24 Jan 2016 10:05:44 -0800 (PST)

`dired-mark-if' marks, unmarks, or flags Dired lines that satisfy its
PREDICATE argument.

But it should do nothing if a given line is already so marked, unmarked,
or flagged.  More importantly, it should not count that line as having
been marked, unmarked, or flagged.  The message that echoes the count of
changes should not take such non-changes into account.

(Also, trivially, the message should not end in a period (.).)

Yes, this is a change in behavior (the message) observed by users.  But
it is TRT, IMO.

Here is a fixed version of the macro.  The diff is trivial.

(defmacro dired-mark-if (predicate msg)
  "Mark files for PREDICATE, according to `dired-marker-char'.
PREDICATE is evaluated on each line, with point at beginning of line.
MSG is a noun phrase for the type of files being marked.
It should end with a noun that can be pluralized by adding `s'.
Return value is the number of files marked, or nil if none were marked."
  `(let ((inhibit-read-only  t)
         count)
    (save-excursion
      (setq count  0)
      (when ,msg (message "%s %ss%s..."
                          (cond ((eq dired-marker-char ?\040)            "Unmarking")
                                ((eq dired-del-marker dired-marker-char) "Flagging")
                                (t                                       "Marking"))
                          ,msg
                          (if (eq dired-del-marker dired-marker-char) " for deletion" "")))
      (goto-char (point-min))
      (while (not (eobp))
        (when ,predicate
          (unless (looking-at (char-to-string dired-marker-char))
            (delete-char 1) (insert dired-marker-char) (setq count  (1+ count))))
        (forward-line 1))
      (when ,msg (message "%s %s%s %s%s" count ,msg
                          (dired-plural-s count)
                          (if (eq dired-marker-char ?\040) "un" "")
                          (if (eq dired-marker-char dired-del-marker) "flagged" "marked"))))
    (and (> count 0)  count)))


In GNU Emacs 24.5.1 (i686-pc-mingw32)
 of 2015-04-11 on LEG570
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/usr --host=i686-pc-mingw32'

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 22457 <at> debbugs.gnu.org
Subject: Re: bug#22457: 24.5;
 [PATCH] `dired-mark-if' should not count non-changes
Date: Tue, 25 Jun 2019 16:42:27 +0200

Drew Adams <drew.adams <at> oracle.com> writes:

> `dired-mark-if' marks, unmarks, or flags Dired lines that satisfy its
> PREDICATE argument.
>
> But it should do nothing if a given line is already so marked, unmarked,
> or flagged.  More importantly, it should not count that line as having
> been marked, unmarked, or flagged.  The message that echoes the count of
> changes should not take such non-changes into account.

That sounds like a sensible change, and a cursory look at the code seems
to say that nothing uses the return value, anyway.  (So it shouldn't
break anything.)  (But my look was very cursory; this should be
confirmed by looking at the code closer.)

> Here is a fixed version of the macro.  The diff is trivial.

No diff was included, which, as usual, is a shame, because the macro has
changed slightly in the meantime.

>                           (cond ((eq dired-marker-char ?\040)            "Unmarking")

?\040 isn't the recommended way of saying "space".

>                                 ((eq dired-del-marker dired-marker-char) "Flagging")
>                                 (t                                       "Marking"))

Your lines are also too wide; they should be less than 80 characters
wide.

Could you re-spin your change and submit a patch?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 22457 <at> debbugs.gnu.org
Subject: RE: bug#22457: 24.5; [PATCH] `dired-mark-if' should not count
 non-changes
Date: Tue, 25 Jun 2019 08:33:14 -0700 (PDT)

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

> > Here is a fixed version of the macro.  The diff is trivial.
> 
> No diff was included, which, as usual, is a shame, because the macro has
> changed slightly in the meantime.

That makes no difference. ;-)  And the diff is still
trivial. ("The macro has changed slightly" == ?\040
became ?\s.  Nothing more)

> Your lines are also too wide; they should be less than 80 characters
> wide.

Oh, lines 92 chars are too wide for dired.el?
Then why does it have lots of lines wider than
80 chars, including lines up to 103 chars wide?

> Could you re-spin your change and submit a patch?

diff -u dired.el dired-2019-06-25a-patched.el
--- dired.el	2019-06-25 07:57:35.886354900 -0700
+++ dired-2019-06-25a-patched.el	2019-06-25 08:09:23.058466400 -0700
@@ -538,7 +538,7 @@
 ;;; Macros must be defined before they are used, for the byte compiler.
 
 (defmacro dired-mark-if (predicate msg)
-  "Mark all files for which PREDICATE evals to non-nil.
+  "Mark files for PREDICATE, according to `dired-marker-char'.
 PREDICATE is evaluated on each line, with point at beginning of line.
 MSG is a noun phrase for the type of files being marked.
 It should end with a noun that can be pluralized by adding `s'.
@@ -558,13 +558,11 @@
 		   "")))
       (goto-char (point-min))
       (while (not (eobp))
-        (if ,predicate
-            (progn
-              (delete-char 1)
-              (insert dired-marker-char)
-              (setq count (1+ count))))
+        (when ,predicate
+          (unless (looking-at-p (char-to-string dired-marker-char))
+            (delete-char 1) (insert dired-marker-char) (setq count (1+ count))))
         (forward-line 1))
-      (if ,msg (message "%s %s%s %s%s."
+      (when ,msg (message "%s %s%s %s%s"
                         count
                         ,msg
                         (dired-plural-s count)

----

Actually, the _attached_ definition is much better than
this - it's what I use in `dired+.el'.  The value returned
and the message indicate both the number matched and the
number changed.  And it has optional arg PLURAL, for
irregular plurals (e.g. "directories").

But if you want this one you'll have to do your own line
shortening, `diff', and adjustment of any callers you
might to want to pass an irregular PLURAL form.

[throw-dired-mark-if.el (application/octet-stream, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 22457 <at> debbugs.gnu.org
Subject: Re: bug#22457: 24.5;
 [PATCH] `dired-mark-if' should not count non-changes
Date: Tue, 25 Jun 2019 17:44:33 +0200

Drew Adams <drew.adams <at> oracle.com> writes:

>> Could you re-spin your change and submit a patch?
>
> diff -u dired.el dired-2019-06-25a-patched.el
> --- dired.el	2019-06-25 07:57:35.886354900 -0700
> +++ dired-2019-06-25a-patched.el	2019-06-25 08:09:23.058466400 -0700

Thanks; applied to the Emacs trunk.

As this is a user-visible change (but a good one, I think), it might be
controversial.  The change is basically what I put in NEWS:

--
*** The marking commands now report how many files were marked by the
command itself, not how many files are marked in total.
--

Perhaps somebody will want to weigh in here; feel free to revert the
commit if this is felt to be a confusing change.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 22457 <at> debbugs.gnu.org
Subject: Re: bug#22457: 24.5;
 [PATCH] `dired-mark-if' should not count non-changes
Date: Tue, 25 Jun 2019 17:46:58 +0200

And why is dired-mark-if a macro in the first place?  It's pretty long
to be a macro; it could be rewritten as a function that take a
predicate...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 22457 <at> debbugs.gnu.org
Subject: RE: bug#22457: 24.5; [PATCH] `dired-mark-if' should not count
 non-changes
Date: Tue, 25 Jun 2019 09:06:46 -0700 (PDT)

> And why is dired-mark-if a macro in the first place?  It's pretty long
> to be a macro; it could be rewritten as a function that take a
> predicate...

(Length of a macro's code makes no difference, if
the code using it is compiled.)

I would say leave this sleeping dog lie...

---

The PREDICATE arg is really code (a sexp), not
a predicate (function).

See also the use of `dired-mark-if' in command
`dired-mark-sexp' (in `dired-x.el').  See the
doc string of that command.

There too the PREDICATE arg is code (a sexp),
not a predicate.  And there you can use a set
of predefined variables in the "PREDICATE"
sexp that gets evaluated and then passed to
`dired-mark-if'.

See also the uses of `dired-mark-if' in
`dired-aux.el', where an anonymous function
is constructed using the macro.

This is the way things have been since about
Day One of Emacs.

Could things have been coded differently for
this?  Likely.  Should they be?  Doubtful.

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Drew Adams <drew.adams <at> oracle.com>, 22457 <at> debbugs.gnu.org
Subject: Re: bug#22457: 24.5;
 [PATCH] `dired-mark-if' should not count non-changes
Date: Wed, 26 Jun 2019 00:47:00 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> --
> *** The marking commands now report how many files were marked by the
> command itself, not how many files are marked in total.
> --

I don't think this exactly describes what the patch changed: it is not
about new vs total marks, but about marks of files that were already
marked before, right?

> Perhaps somebody will want to weigh in here; feel free to revert the
> commit if this is felt to be a confusing change.

IIUC: Maybe we could add to the summary message something like
"(N already were marked)" or so if a command finds existent marks on files
to be marked?  That might be useful information, even independent from
this change, because it might also be a hint for a pilot error.


Michael.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Drew Adams <drew.adams <at> oracle.com>, 22457 <at> debbugs.gnu.org
Subject: Re: bug#22457: 24.5;
 [PATCH] `dired-mark-if' should not count non-changes
Date: Wed, 26 Jun 2019 00:53:12 +0200

Michael Heerdegen <michael_heerdegen <at> web.de> writes:

> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
>
>> --
>> *** The marking commands now report how many files were marked by the
>> command itself, not how many files are marked in total.
>> --
>
> I don't think this exactly describes what the patch changed: it is not
> about new vs total marks, but about marks of files that were already
> marked before, right?

It's this code:

          (unless (looking-at-p (char-to-string dired-marker-char))
            (delete-char 1)
            (insert dired-marker-char)
            (setq count (1+ count))))

So now it says how many lines that are marked this time.  Previously it
gave you a total tally of all marked files.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 22457 <at> debbugs.gnu.org
Subject: Re: bug#22457: 24.5;
 [PATCH] `dired-mark-if' should not count non-changes
Date: Wed, 26 Jun 2019 01:02:47 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> > I don't think this exactly describes what the patch changed: it is not
> > about new vs total marks, but about marks of files that were already
> > marked before, right?
>
> It's this code:
>
>           (unless (looking-at-p (char-to-string dired-marker-char))
>             (delete-char 1)
>             (insert dired-marker-char)
>             (setq count (1+ count))))
>
> So now it says how many lines that are marked this time.  Previously it
> gave you a total tally of all marked files.

Yes, I think I understand.  My problem is that "[...] not how many files
are marked in total" sounds like as if marked files that were not even
touched by the command (that are not matched by the predicate) were
previously included.  You mean it relative to the files matched, right?

Oh, btw, this patch line

  (looking-at-p (char-to-string dired-marker-char))

is not good if dired-marker-char is a regexp special like . or ?, I
guess (regexp-quote missing)?

Michael

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 22457 <at> debbugs.gnu.org
Subject: Re: bug#22457: 24.5;
 [PATCH] `dired-mark-if' should not count non-changes
Date: Wed, 26 Jun 2019 01:12:25 +0200

Michael Heerdegen <michael_heerdegen <at> web.de> writes:

> Yes, I think I understand.  My problem is that "[...] not how many files
> are marked in total" sounds like as if marked files that were not even
> touched by the command (that are not matched by the predicate) were
> previously included.  You mean it relative to the files matched, right?

Well... this is the patch:

-        (if ,predicate
-            (progn
-              (delete-char 1)
-              (insert dired-marker-char)
-              (setq count (1+ count))))
+        (when ,predicate
+          (unless (looking-at-p (char-to-string dired-marker-char))
+            (delete-char 1)
+            (insert dired-marker-char)
+            (setq count (1+ count))))

So if the predicate matches, if there was a mark, it would delete the
mark, and then add it back, and then count it.

But you're right, files where the predicate doesn't say non-nil are not
counted, so it doesn't report the total number of marked files...

> Oh, btw, this patch line
>
>   (looking-at-p (char-to-string dired-marker-char))
>
> is not good if dired-marker-char is a regexp special like . or ?, I
> guess (regexp-quote missing)?

Indeed.  I'll fix it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Michael Heerdegen <michael_heerdegen <at> web.de>, Lars Ingebrigtsen
 <larsi <at> gnus.org>
Cc: 22457 <at> debbugs.gnu.org
Subject: RE: bug#22457: 24.5; [PATCH] `dired-mark-if' should not count
 non-changes
Date: Tue, 25 Jun 2019 16:17:02 -0700 (PDT)

> > *** The marking commands now report how many files were marked by the
> > command itself, not how many files are marked in total.
> > --
> 
> I don't think this exactly describes what the patch changed: it is not
> about new vs total marks, but about marks of files that were already
> marked before, right?

It's about making the reported count refer to
the number of marks that were changed or added.

The old way counted all of the matches.
The new way counts only matches that are not
already marked the same way.

> > Perhaps somebody will want to weigh in here; feel free to revert the
> > commit if this is felt to be a confusing change.
> 
> IIUC: Maybe we could add to the summary message something like
> "(N already were marked)" or so if a command finds existent marks on files
> to be marked?  That might be useful information, even independent from
> this change, because it might also be a hint for a pilot error.

That's what the "better" version does that I sent
as an _attachment_.

If some were already marked as directed it says:

"42 foobars matched, 35 newly marked"    or
"42 foobars matched, 35 newly unmarked"  or
"42 foobars matched, 35 newly flagged"   or
"42 foobars matched, 35 newly unflagged"

(If all that were matched were already marked
as directed then instead of "35" it says "0".)

Otherwise (no matches were already marked as
directed) it says:

"42 foobars newly marked"    or
"42 foobars newly unmarked"  or
"42 foobars newly flagged"   or
"42 foobars newly unflagged"

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Michael Heerdegen <michael_heerdegen <at> web.de>, Lars Ingebrigtsen
 <larsi <at> gnus.org>
Cc: 22457 <at> debbugs.gnu.org
Subject: RE: bug#22457: 24.5; [PATCH] `dired-mark-if' should not count
 non-changes
Date: Tue, 25 Jun 2019 16:26:58 -0700 (PDT)

> Oh, btw, this patch line
> 
>   (looking-at-p (char-to-string dired-marker-char))
> 
> is not good if dired-marker-char is a regexp special like . or ?, I
> guess (regexp-quote missing)?

It's a good point; it would be clearer to wrap the
arg to `looking-at-p' with `regexp-quote'.

It works as coded, however, because Emacs regexp
matching treats a special char such as `*' as
non-special in such a context.

I'm not saying it's good to rely on that (documented)
behavior - it's better to use `regexp-quote'.

See (elisp)`Syntax of Regexps':

  *Please note:* For historical compatibility, special characters are
  treated as ordinary ones if they are in contexts where their special
  meanings make no sense.  For example, '*foo' treats '*' as ordinary
  since there is no preceding expression on which the '*' can act.  It is
  poor practice to depend on this behavior; quote the special character
  anyway, regardless of where it appears.

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 22457 <at> debbugs.gnu.org
Subject: Re: bug#22457: 24.5;
 [PATCH] `dired-mark-if' should not count non-changes
Date: Wed, 26 Jun 2019 01:30:58 +0200

Drew Adams <drew.adams <at> oracle.com> writes:

> It works as coded, however, because Emacs regexp matching treats a
> special char such as `*' as non-special in such a context.

Oh, indeed.  But not for `.' I think.

Michael.

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 22457 <at> debbugs.gnu.org
Subject: Re: bug#22457: 24.5;
 [PATCH] `dired-mark-if' should not count non-changes
Date: Wed, 26 Jun 2019 01:28:24 +0200

Drew Adams <drew.adams <at> oracle.com> writes:

> That's what the "better" version does that I sent
> as an _attachment_.

Yes, something like that.

Michael.

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 22457 <at> debbugs.gnu.org
Subject: RE: bug#22457: 24.5; [PATCH] `dired-mark-if' should not count
 non-changes
Date: Tue, 25 Jun 2019 17:00:47 -0700 (PDT)

> > It works as coded, however, because Emacs regexp matching treats a
> > special char such as `*' as non-special in such a context.
> 
> Oh, indeed.  But not for `.' I think.

Right again.  (And yes, one can use `.' as a marker char.)

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

From: Andreas Schwab <schwab <at> suse.de>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>,
 Lars Ingebrigtsen <larsi <at> gnus.org>, 22457 <at> debbugs.gnu.org
Subject: Re: bug#22457: 24.5;
 [PATCH] `dired-mark-if' should not count non-changes
Date: Wed, 26 Jun 2019 10:50:24 +0200

On Jun 25 2019, Drew Adams <drew.adams <at> oracle.com> wrote:

>> Oh, btw, this patch line
>> 
>>   (looking-at-p (char-to-string dired-marker-char))
>> 
>> is not good if dired-marker-char is a regexp special like . or ?, I
>> guess (regexp-quote missing)?
>
> It's a good point; it would be clearer to wrap the
> arg to `looking-at-p' with `regexp-quote'.

I think it's better to say (eq (char-after) dired-marker-char).

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab <at> suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Andreas Schwab <schwab <at> suse.de>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>,
 Lars Ingebrigtsen <larsi <at> gnus.org>, 22457 <at> debbugs.gnu.org
Subject: RE: bug#22457: 24.5; [PATCH] `dired-mark-if' should not count
 non-changes
Date: Wed, 26 Jun 2019 06:32:46 -0700 (PDT)

> I think it's better to say (eq (char-after) dired-marker-char).

Yes, good.  Not a big deal either way, but
that's simpler and clearer.

Previous Next

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