Re: Add new functions to mark/unmark/delete all bookmarks

[Matthew White]

> Hi, Matthew.  Thanks so much for this.  I have reviewed the patch
> against 'master' (since it applies cleanly there, whereas it does not
> quite apply cleanly to 'emacs-27').  It is very good; I have just
> some minor comments below.

Hi Karl. I'm flattered, thanks!

I appreciate so much that you took the time to review my patch, and
I eagerly followed your suggestions!

You will find attached both a patch that applies to master, and one
that applies to emacs-27... They are the same in term of changes.

I'm still waiting a reply to the copyright assignment... But in the
meantime we can freely discuss about the patch...

> 1) It helps to include a summary line at the top of your commit
> message, followed by a blank line.  The summary line should be
> limited to 50 characters or fewer, if possible.  (This is all
> documented in the "Commit messages" section in the 'CONTRIBUTE' file
> in the top level of the Emacs source tree, by the way).  In your
> case, the Subject header of your email almost does the job -- we can
> shorten it a bit to fit within 50 characters:
> 
>   "Add ability to mark/unmark/delete all bookmarks"

Thanks for pointing that out. Now that I read the 'CONTRIBUTE' file,
I have some questions (by the way, I needed a remainder to read...):

1) I did commit a message with a summary line, as you described...

   [ Actually my line was the subject of the thread, yours (above)
     is better, hence I switched to your idea, thanks again ;) ]

   And indeed I use `git format-patch -1` as suggested in 'CONTRIBUTE'
   to produce a patch, but the command "strips" the summary line from
   the commit message and uses it as 'Subject:' in the patch, where it
   is prefixed with '[PATCH] ' if the option -k isn't also given...

   Once the patch is applied via `git am`, the 'Subject:' is put back
   in the commit message as summary, without the '[PATCH] ' prefix.

   So, I wonder if you used `git am` to apply the patch...? Could you
   tip me off, please?

2) With which quotes should I surround the variable/function names in
   the commit message? `' or '' or ‘’? I could not figure that out...

> 2) In the commit message, you don't need to repeat the filename ("*
> lisp/bookmark.el") each time.  You can just write it once, and then
> underneath it show everything affected in that file.  Taking the
> first three entries in your original log message as an example:
> 
>    * lisp/bookmark.el (bookmark-delete-all): New function to delete
> all bookmarks.
>    * lisp/bookmark.el (bookmark-bmenu-mark-all): New function to mark
> all bookmarks in the bookmark list buffer.
>    * lisp/bookmark.el (bookmark-bmenu-unmark-all): New function to
> unmark all bookmarks in the bookmark list buffer.
> 
> ...they would instead be written like this:
> 
>    * lisp/bookmark.el (bookmark-delete-all): New function to delete
> all bookmarks.
>      (bookmark-bmenu-mark-all): New function to mark all bookmarks in
> the bookmark list buffer. 
>      (bookmark-bmenu-unmark-all): New function to unmark all
> bookmarks in the bookmark list buffer.
> 
> Quick digression:
> 
> You can use 'git log' to see this style used in past Emacs commits.

[x] Don't repeat the file name each time in the commit message.

    ...And yes, I did take a look at the `git log`...

> Better to call ARG something like NO-CONFIRM, so its name reflects
> its purpose.
>
> And a suggestion: have the prompt say "Permanently delete all
> bookmarks? " instead.

In `bookmark-delete-all':

[x] Chage ARG to NO-CONFIRM.
[x] Prompt "Permanently delete all bookmarks? ".

> Because of subtle assumptions associated with English word order, the
> current prompt ("Delete all bookmarks permanently? ") implies that
> the alternative might be to delete the bookmarks non-permanently.

That's a very smart thought... I'm truly impressed!

> Regarding the "FIXME" comment: that assumption is embedded throughout
> the bookmark-bmenu-* code, so I think there's no need for the "FIXME"
> (and indeed the comment may cause confusion, since its presence calls
> the assumption into question).
> 
> It might be good to make a followup change in which we document the
> last-line-is-empty assumption, but that's a separate change of course.

[x] Remove the "FIXME" code comments.

> >+       ;; FIXME: This assumes that the last line is empty.
> >+       (while (not (eobp))

> These tests are a huge contribution -- thanks for taking the time to
> write them.

Following your guidelines is very instructing, Karl!

I also:

[x] Change `nameN' and `fileN' into `name-N' and `file-N' (i.e. name0
    -> name-0) in all new functions and in the new test-list.bmk file.
[x] Indent new functions to split long lines into shorter ones.
[x] bookmark-tests.el (bookmark-test-bmenu-any-marks-list): New function
    to test `bookmark-bmenu-any-marks' with a list of bookmarks.

> This patch looks good to me.  I've tested it myself and it works as
> advertised, and the new tests pass 'make check'.
> 
> If you have time to revise the patch slightly as per above (if you
> agree with the comments, of course -- otherwise, let's discuss here)
> that would be great.  I see that Stefan has already sent you the
> assignment paperwork.
> 
> Best regards,
> -Karl

Again, thank you very much Karl for all your effort reviewing my patch,
and especially for your brilliant insights!

I hope to get back soon with good news about the copyright assignment.

Catch you later!

master-Add-ability-to-mark-unmark-delete-all-bookmarks.patch
Description: Text Data

emacs-27-Add-ability-to-mark-unmark-delete-all-bookmarks.patch
Description: Text Data

pgp1lSTaZxMcu.pgp
Description: OpenPGP digital signature