Subject: Re: bug#35362: 26.2; [debbugs.el] Automate commit -> debbugs flow (posting patches, closing bugs after pushing)

From: Michael Albinus <[email-masked]>
Date: Thu, 25 Apr 2019 14:16:33 +0200

Noam Postavsky <[email-masked]> writes:

Hi Noam,

I believe your patch is almost OK, so let's apply it. But of course, I
still have some few remarks :-)

>>> Invoke the command @code{debbugs-gnu-pick-commits} and press
>>
>> Should we give this command a key binding?
>
> Maybe, but then the question is what map to bind it in.  Is it okay to
> start adding things into vc-git-log-view-mode-map?

Yep. Maybe we bind "c" for it, because other candidates (d/D/p/P) seem
to be bound already.

Furthermore, we might mention debbugs-gnu-pick-commits in the
;;; Commentary section of debbugs-gnu.el.

>> I don't understand (yet), how the description of this variable helps. Is
>> the user expected to change the value herself, somehow? Otherwise, we
>> might to keep this description out.
>
> The idea is you can add entries that can work with other packages, e.g.,
> magit.

OK, this part I don't understand yet (I'm not a magit user, for
example). But I don't mind.

> --- a/packages/debbugs/debbugs-gnu.el
> +++ b/packages/debbugs/debbugs-gnu.el

> +    (add-hook 'message-send-actions
> +              (lambda () (remhash bugid debbugs-cache-data))
> +              nil t)))

Do we  need to declare message-send-actions?

> +(defun debbugs-gnus-jump-to-bug (bugid)
> +  "Display buffer associated with BUGID with `pop-to-buffer'.
> +Use `gnus-read-ephemeral-emacs-bug-group' instead if there is no such buffer."

Maybe you could add at least a FIXME for the lack of rmail support.

> +(defun debbugs-gnu--git-insert (&rest args)
> +  "Insert output of running git with ARGS.
> +Throws error if git returns non-zero.  Uses `vc-git-program'."

Don't mention vc-git-program.

> +  (unless (eql 0 (apply #'vc-git--call '(t t) args))

Declare vc-git--call.

> +(defun debbugs-gnu--git-remote-info ()
> +  "Return (REMOTE . INFO-ALIST)."

Say, that REMOTE is a string, which must be matched by REMOTE-REGEXP of
debbugs-gnu-git-remote-info-alist. Refer to the docstring of that user option.

> +           (dir (make-temp-file (format "patches-for-bug%d-" bugnum) t))

I'm curious: Why a trailing "-" in the buffer name?

> +           (deldir (lambda ()
> +                     (delete-directory dir t)
> +                     (remove-hook 'message-exit-actions deldir t)

Do we  need to declare message-exit-actions?

> +    (dolist (patch (directory-files dir t "\\`[^.]"))
> +      (mml-attach-file patch type "patch" disposition))

Do we  need to declare mml-attach-file?

> +`debbugs-gnu-pick-commits' and jump to read bug in prepration for

Typo.

> --- a/packages/debbugs/debbugs-ug.texi
> +++ b/packages/debbugs/debbugs-ug.texi

> +The string inserted to describe an announced commit is controlled by
> +the variable @code{debbugs-gnu-commit-description-format}, it is a

It is a user option.

> +subitem is appended to the commit description.  By default this
> +variable is configured for the GNU Emacs and GNU ELPA repositories,

That's also a user option.

Best regards, Michael.