Re: [elpa] scratch/email-revision-details da3795b 1/2: * elpa-admin.el (elpaa--get-release-revision): Workaround git bug

Re: [elpa] scratch/email-revision-details da3795b 1/2: * elpa-admin.el (elpaa--get-release-revision): Workaround git bug

[Stefan Monnier]

> diff --git a/elpa-admin.el b/elpa-admin.el
> index e26108b..fa01001 100644
> --- a/elpa-admin.el
> +++ b/elpa-admin.el
> @@ -184,7 +184,11 @@ commit which modified the \"Version:\" pseudo header."
>                        "--pretty=format:%H"
>                        "-L" (concat "/^;;* *\\(Package-\\)\\?Version:/,+1:"
>                                     (file-name-nondirectory mainfile))))
> -                    (buffer-string)
> +                    ;; The --no-patch (aka -s) option does not work
> +                    ;; with "git log -L<from>,<to>:<path>" before git
> +                    ;; version 2.22; so capture only the first line.
> +                    (buffer-substring-no-properties
> +                     (goto-char (point-min)) (line-end-position))
>                    (cons 'error (buffer-string))))))
>          (if (stringp release-rev)
>              (progn

Feel free to push this to `master`, it looks good to me.


        Stefan

Re: [elpa] scratch/email-revision-details

[Phil Sainty]

On 2021-08-07 03:30, Stefan Monnier wrote:
> Feel free to push this to `master`, it looks good to me.

Done.  (Well, pushed to elpa-admin.)

For the other commit (just re-pushed to my scratch branch, but
the code is unchanged), I'm not sure how to test it, aside from
testing in isolation the part which actually gets the revision
details from git for a given `release-rev' value (which I've
tested against so-long.el).

I've gone with the full compliment of author+commit timestamps
in both relative and absolute formats, which looks like this:

; * lisp/so-long.el: Bump version for the GNU ELPA build
Authored 3 days ago on Wed, 4 Aug 2021 17:17:23 +1200
Committed 3 days ago on Wed, 4 Aug 2021 17:17:23 +1200
Revision b84986af52d4e9debace2850a5ec106f51e38e61

I think they are all useful for this purpose.

The relative times are helpful because they're such an obvious
indication if the value is significantly different to what
you'd expected.

The absolute times are still useful because this is arriving
by email, and so you're never reading it at the time it was
generated.

The author time is helpful because it doesn't change when
the code is rebased; so if the commit in question was written
much earlier than the expected commit, you again get that
immediate information that something is wrong.

And the commit time is still useful alongside the author time,
as it will match the time you actually modified the that commit,
so you can mentally match those together too.


If the code looks sane to you, are you able to test it, or
let me know how to safely do that?  I see the commented debug
line ahead of the (message-send) call, but I don't know whether
that's the only code that I'd need to be concerned about.


cheers,
-Phil

Re: [elpa] scratch/email-revision-details

[Stefan Monnier]

Phil Sainty [2021-08-08 01:45:20] wrote:
> On 2021-08-07 03:30, Stefan Monnier wrote:
>> Feel free to push this to `master`, it looks good to me.
> Done.

Thanks.

> (Well, pushed to elpa-admin.)

Oops, indeed.

> For the other commit (just re-pushed to my scratch branch, but
> the code is unchanged), I'm not sure how to test it, aside from
> testing in isolation the part which actually gets the revision
> details from git for a given `release-rev' value (which I've
> tested against so-long.el).

I think you can test it as follows:

- Add to `elpa-config` the following elements:

      (email-to              "Phil Sainty <psainty@orcon.net.nz>")
      (email-from            "Phil Sainty <psainty@orcon.net.nz>")
      (email-reply-to        "Phil Sainty <psainty@orcon.net.nz>")

  [ Or use some other email address, of course.  ]

- `rm archive/*`
- `make build/so-long`

This should presumably build a "new" release tarball and send an email
to the `email-to` address (with Cc to the package's maintainer).

The email is sent by Emacs, so the actual sending may fail depending on
how your Emacs config is setup.

> I've gone with the full compliment of author+commit timestamps
> in both relative and absolute formats, which looks like this:
>
> ; * lisp/so-long.el: Bump version for the GNU ELPA build
> Authored 3 days ago on Wed, 4 Aug 2021 17:17:23 +1200
> Committed 3 days ago on Wed, 4 Aug 2021 17:17:23 +1200
> Revision b84986af52d4e9debace2850a5ec106f51e38e61

That looks great (I like those times).
I'd just add a short line before that explaining what those lines
represent (for the non-expert user who's not familiar with VCS), and
add some indentation to clarify that the lines belong together.
Maybe something like:

    Built from revision <COMMITID>:
      ; * lisp/so-long.el: Bump version for the GNU ELPA build
      Authored 3 days ago on Wed, 4 Aug 2021 17:17:23 +1200
      Committed 3 days ago on Wed, 4 Aug 2021 17:17:23 +1200

If we wanted to get fancy we could remove one of the two last lines in
case (like here) the times are equal (since that should be a fairly
common case).

> If the code looks sane to you, are you able to test it, or
> let me know how to safely do that?  I see the commented debug
> line ahead of the (message-send) call, but I don't know whether
> that's the only code that I'd need to be concerned about.

You could uncomment that line and then instead of `make build/so-long`
I think you can use

    emacs -l $(pwd)/admin/elpa-admin.el \
          -f elpaa-batch-make-one-package so-long

But I'm not completely sure: when I used that debug line, I think
I called internal functions more directly from an interactive
Emacs session (I had those things still fairly fresh in my mind).

Looking at my question/suggestion not to use a global var and to fetch
the commit's info only when we actually send the message, I see that
`elpaa--release-email` can't easily do that right now because it doesn't
know the commitid.

I think the best way to do that is:

- Make it so `elpaa--release-email` is called directly from
  `elpaa--make-one-tarball` when `revision-function` is non-nil.
  [ AFAICT `elpaa--release-email` is called if-and-only-if
    `revision-function` is non-nil and always in the same way, so this
    would improve/simplify the code.  ]
- Change `elpaa--make-one-tarball` to remember the commit id returned by
  `revision-function`, so we can pass it to `elpaa--release-email`.
- Then we can write a new function to get the commitid's data using the
  same

      (let* ((mainfile (file-truename
                        (expand-file-name (elpaa--main-file pkg-spec)
                                          (elpaa--dirname dir))))
             (default-directory (file-name-directory mainfile))

  as in `elpaa--get-release-revision`.
- use that function from `elpaa--release-email`.


        Stefan

Re: [elpa] scratch/email-revision-details

[Stefan Monnier]

> - Add to `elpa-config` the following elements:
>
>       (email-to              "Phil Sainty <psainty@orcon.net.nz>")
>       (email-from            "Phil Sainty <psainty@orcon.net.nz>")
>       (email-reply-to        "Phil Sainty <psainty@orcon.net.nz>")
>
>   [ Or use some other email address, of course.  ]
>
> - `rm archive/*`
> - `make build/so-long`
>
> This should presumably build a "new" release tarball and send an email
> to the `email-to` address (with Cc to the package's maintainer).
>
> The email is sent by Emacs, so the actual sending may fail depending on
> how your Emacs config is setup.

Actually, your config won't make a difference because this will run
Emacs in batch mode, which doesn't read your init file.
In elpa.gnu.org I circumvent that by setting

    (setq send-mail-function #'sendmail-send-it)

in a `site-start.el` file.

You can get similar results with something like:

    make EMACS="emacs --batch --eval '(setq send-mail-function (function sendmail-send-it))'" \
         build/so-long


-- Stefan

Re: [elpa] scratch/email-revision-details

[Phil Sainty]

On 2021-08-08 10:38, Stefan Monnier wrote:
> - Make it so `elpaa--release-email` is called directly from
>   `elpaa--make-one-tarball` when `revision-function` is non-nil.
>   [ AFAICT `elpaa--release-email` is called if-and-only-if
>     `revision-function` is non-nil and always in the same way

That's true, but the `revision-function' being passed varies...

> - Change `elpaa--make-one-tarball` to remember the commit id returned
>   by `revision-function`, so we can pass it to `elpaa--release-email`.

...and it wasn't clear to me that it was safe to assume that
the value being returned by any particular `revision-function'
was guaranteed to be a git commit -- the single case where
`elpaa--get-release-revision' was called was the only one that
I'd been trying to target.

Looking again, `elpaa--get-last-release' seems to be responsible
for providing the revision value in other situations, and it's
similarly grabbing it from git (interestingly this code is getting
the first line of the git output explicitly, as in my workaround
for that git bug), so your suggestion makes sense.


-Phil