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

[Karl Fogel]

On 25 Jul 2020, Matthew White wrote:
>> 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.

Thanks!  I'll test against both branches, then.

>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?

I didn't use 'git am' the previous time, but I will this time.

I wish that the output produced by 'git format-patch' were both a little more 
self-identifying and more self-explanatory.  While I recognized the patch as 
such output, I forgot that the Subject line therefore would contain the first 
line of the commit message :-).  Had I reviewed the commit message after 
application, I would have seen that line appear, of course.  Anyway, you did 
the right thing here -- I just need to adjust my own process a bit.

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

I saw the followup discussion about this, with the answer being 'like so'.

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

Great.

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

Heh -- sorry there just happened to be a misleading commit message in the 
recent history, yeah.

>> 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? ".

Thanks.

>> 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!

Well, I used to be an English teacher :-).

>> 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.

Thanks.

>Following your guidelines is very instructing, Karl!

It's a pleasure to review a well-done patch!

>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.

*nod*  Got it; will take a look at those changes.

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

Sounds good.

On both branches, I'll apply the corresponding new changesets, run 'make 
check', and test manually.  I'll also look over the diffs again.  I'll follow 
up here after I've done so.

I'm actually not sure yet which branch we should apply on -- I think 'emacs-27' 
is the right answer, as per CONTRIBUTE, but if anyone knows better please say 
so.

(I would make sure to use 'git am' to apply, to preserve attribution and 
history information, of course.)

Best regards,
-Karl

signature.asc
Description: PGP signature