--notes-from-tag does not preserve newlines

[nedtwigg]

Describe the bug

If an annotated tag has newlines, --notes-from-tag should preserve those newlines in the created release, but it strips all newlines.

Steps to reproduce the behavior

1. create an annotated tag which has newlines
2. create a release using gh release ... --notes-from-tag
3. the release it creates will have the content of the annotated tag but without the newlines

Expected vs actual behavior

See above.

Logs

Example:

• for public repo https://github.com/diffplug/blowdryer-diffplug
• git show 8.0.0

tag 8.0.0
Tagger: runner <runner@fv-az775-255.ywbp5beigl5e3k0catohwgwj5f.cx.internal.cloudapp.net>
Date:   Mon Jul 8 20:32:56 2024 +0000


### Changed
- **BREAKING** `spotlessChangelog` now creates GitHub releases by default ([#6](https://github.com/diffplug/blowdryer-diffplug/pull/6))
  - you need to update your `deploy.yml`
  - if you set the `tagPrefix`, then you'll need to update the `runAfterPush`
  - see PR above for details

commit c642275aebd3ce915fbd4a471117169e164c15a0 (tag: 8.0.0, origin/release, release)

• gh release create '8.0.0' --title '8.0.0' --notes-from-tag
• gh release view 8.0.0

8.0.0
nedtwigg released this about 5 minutes ago

  ### Changed - BREAKING  spotlessChangelog  now creates GitHub releases by default (#6                               
  https://github.com/diffplug/blowdryer-diffplug/pull/6)   - you need to update your  deploy.yml    - if you set the  
  tagPrefix , then you'll need to update the  runAfterPush    - see PR above for details                              


View on GitHub: https://github.com/diffplug/blowdryer-diffplug/releases/tag/8.0.0

[williammartin]

Hmmmm, interesting. So here's how we load the notes from an annotated tag:

git tag --list <tag> --format='%(contents:subject)%0a%0a%(contents:body)'

For your tag this shows:

➜  blowdryer-diffplug git:(main) git tag --list 8.0.0 --format='%(contents:subject)%0a%0a%(contents:body)' | cat
### Changed - **BREAKING** `spotlessChangelog` now creates GitHub releases by default ([#6](https://github.com/diffplug/blowdryer-diffplug/pull/6))   - you need to update your `deploy.yml`   - if you set the `tagPrefix`, then you'll need to update the `runAfterPush`   - see PR above for details

And if we look a bit closer, we can see that your entire annotation is being treated as the subject line:

➜  blowdryer-diffplug git:(main) git tag --list 8.0.0 --format='%(contents:subject)' | cat
### Changed - **BREAKING** `spotlessChangelog` now creates GitHub releases by default ([#6](https://github.com/diffplug/blowdryer-diffplug/pull/6))   - you need to update your `deploy.yml`   - if you set the `tagPrefix`, then you'll need to update the `runAfterPush`   - see PR above for details

Comparatively, if we actually have a subject line, things look correct:

➜  test-repo git:(main) git tag --list v1500 --format='%(contents:subject)%0a%0a%(contents:body)' | cat
This is a subject

This is a body with newlines
It really has newlines
For realsies

---

What do we do from here

I wonder if we should be using:

➜  git tag --list 8.0.0 --format='%(contents)' | cat
### Changed
- **BREAKING** `spotlessChangelog` now creates GitHub releases by default ([#6](https://github.com/diffplug/blowdryer-diffplug/pull/6))
  - you need to update your `deploy.yml`
  - if you set the `tagPrefix`, then you'll need to update the `runAfterPush`
  - see PR above for details

It definitely seems like if you wrote a tag annotation message with newlines, those should be preserved, regardless of how contents:subject line works.

I'm just concerned that if we used %(contents) we might end up with extra unexpected stuff in the message. size clearly doesn't appear, but what about signature? I don't have an example to check right now. Unfortunately there's no information in the original PR as to why we focused only on subject and body.

[babakks]

Here is my two cents on this.

It definitely seems like if you wrote a tag annotation message with newlines, those should be preserved, regardless of how contents:subject line works.

@williammartin I think the tag message is not well-formatted as of Git definitions. Git seems to create the subject line as single-line concatenation of all text (including single \ns) before the first blank line (like commit messages). For example, if you create the tag with this command then the subject and the body are all correctly assigned:

$ git tag -a -F <(echo "subject\n\nfoo\nbar\nbaz") v1
$ git tag --list v1 --format='%(contents:subject)' | cat
subject
$ git tag --list v1 --format='(contents:body)' | cat
foo
bar
baz

But if you do it like this, it will end up with the same issue:

$ git tag -a -F <(echo "subject line 1\nsubject line 2\n\nfoo\nbar\nbaz") v2
$ git tag --list v2 --format='%(contents:subject)' | cat
subject line 1 subject line 2
$ git tag --list v2 --format='(contents:body)' | cat
foo
bar
baz

Alternatively, using %(content) will include extra stuff as you suggested. Here's the result with a signature (i.e., tag was created with the -s option):

subject

foo
bar
baz
-----BEGIN PGP SIGNATURE-----
... (base64 signature data)
-----END PGP SIGNATURE-----

Truncating signature from the text is not impossible but doesn't seem like the best way to do it. You should also delete other extra things, if any. Also, this approach has to be backward-compatible with the existing behavior and keep the same output format for well-formated subject/body annotations.

I also thought of other small improvements, like using %(contents) only when %(contents:body) is empty, but it's not good enough. For example, if a tag annotation looks like below then such a solution wouldn't work.

### Changed
- ...
              <-- blank line here dividing body and subject
### Note
...

Conclusion

I really don't know what's the best way to resolve this issue. So far we should either:

1. Leave the implementation as is, simply because it expects a Git tag annotation format/standard.
2. Use the %(contents) field value and strip the extra parts, which should also keep the current behavior for well-formatted subject/body tag annotations.

@williammartin What do you think?

[williammartin]

Alright so I went into a bit of a rabbit-hole on this one, bear with me.

How does github.com display annotated tag messages?

Here's a new tag with a signature:

git tag -s -a -F <(echo "subject line 1\nsubject line 2\n\nfoo\nbar\nbaz") v3

git show v3

tag v3
Tagger: William Martin <[email protected]>
Date:   Mon Jul 22 16:15:13 2024 +0200

subject line 1
subject line 2

foo
bar
baz
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEZBFjd+c1zX61Yk2QL/Kr2zcVGFwFAmaeaXEACgkQL/Kr2zcV
GFyJGg//RarYvUM6QbbiqcpLNKoAYR8r1YPmeh2XHpBSOdE4oMdZb0JGmRkopHP/
SKZ/PqcLUkgJNdTo06WbCYPBUM7eA2r7gsU0Ck6HDFbE30UcWXZXSo4+h32G6JtT
tY2nhsRgd07sUUSBUG/Uuou5rrZva132s2oWXWAXOpVFWWfyNAURom+cULrN2Usj
tYne+BCuQugQfD1ylc2cXB03HQHdJHHXjzh8UUiBfpAcMDWc3kIQKYvEF6gEQAox
zj1t5geyAXYAf5GziP40rSu9NdhCTqd95/napFfV++oMX/e6LlWU1nnHo8ugos+/
cDF3//FBxznneqGLpVNl6u+usld0odD4HcHWVQslK4yiAXlHOR5GYpcvxGytdzLC
676x+P5BWUu2QTTSThTUC5fDICw1TKZd+OdG6zXmW0KxH3JQqRUlv9J/RgbRlHzl
vjhEDx5xFaJPPbWPnXP3f2nDIIufQeyIsI1PJBu2TFlg1aG+vrBMuKFxjwNDhrXO
niY0wYFr1I11/L1L/I4loKF+Pue2EvnzpdfN2M75Nl1C351TL7qs8wucRpEBcOs/
1hlSuXQy6iolsxVmHDdSdflSM0/58WbOM7ePiDEoZypvrtaVue9YQCBroabP5bTG
XeB3edu5IV8slz+mxtE3iNQ05Lg0Lu5oLLCojodxnNsSxTFLsPA=
=Ywh2
-----END PGP SIGNATURE-----
...

If we go to the /tags URL for this repo and we expand the tag, we don't see the signature:

So we can see that the platform has some mechanism of pulling the information we want with newlines preserved.

How does github.com do this?

Well, I had a look into the codebase and I could see that these contents were either loaded via git cat-file with some regex applied to split headers from the body, or via the rugged gem. In either case the result is the same, the contents of the message property on our Tag model contains the signature as we'd expect:

➜  test-repo git:(v3) git rev-parse v3
40ee00ea0397eec2349174e36a8a279050c8f7b6

irb(main):020:0> repo = Rugged::Repository.new('.')
=> #<Rugged::Repository:768540 {path: "/Users/williammartin/workspace/cli-triaging/test-repo/.git/"}>
irb(main):021:0> t = repo.lookup('40ee00ea0397eec2349174e36a8a279050c8f7b6')
=> #<Rugged::Tag::Annotation:780300 {name: "v3", message: "subject line 1\nsubject line 2\n\nfoo\nbar\nbaz\n-----BEGIN PGP SIGNATURE-----\n\niQIzBAABCAAdFiEEZBFjd+c1zX61Yk2QL/Kr2zcVGFwFAmaeaXEACgkQL/Kr2zcV\nGFyJGg//RarYvUM6QbbiqcpLNKoAYR8r1YPmeh2XHpBSOdE4oMdZb0JGmRkopHP/\nSKZ/PqcLUkgJNdTo06WbCYPBUM7eA2r7gsU0Ck6HDFbE30UcWXZXSo4+h32G6JtT\ntY2nhsRgd07sUUSBUG/Uuou5rrZva13...
irb(main):022:0> t.message
=> "subject line 1\nsubject line 2\n\nfoo\nbar\nbaz\n-----BEGIN PGP SIGNATURE-----\n\niQIzBAABCAAdFiEEZBFjd+c1zX61Yk2QL/Kr2zcVGFwFAmaeaXEACgkQL/Kr2zcV\nGFyJGg//RarYvUM6QbbiqcpLNKoAYR8r1YPmeh2XHpBSOdE4oMdZb0JGmRkopHP/\nSKZ/PqcLUkgJNdTo06WbCYPBUM7eA2r7gsU0Ck6HDFbE30UcWXZXSo4+h32G6JtT\ntY2nhsRgd07sUUSBUG/Uuou5rrZva132s2oWXWAXOpVFWWfyNAURom+cULrN2Usj\ntYne+BCuQugQfD1ylc2cXB03HQHdJHHXjzh8UUiBfpAcMDWc3kIQKYvEF6gEQAox\nzj1t5geyAXYAf5GziP40rSu9NdhCTqd95/napFfV++oMX/e6LlWU1nnHo8ugos+/\ncDF3//FBxznneqGLpVNl6u+usld0odD4HcHWVQslK4yiAXlHOR5GYpcvxGytdzLC\n676x+P5BWUu2QTTSThTUC5fDICw1TKZd+OdG6zXmW0KxH3JQqRUlv9J/RgbRlHzl\nvjhEDx5xFaJPPbWPnXP3f2nDIIufQeyIsI1PJBu2TFlg1aG+vrBMuKFxjwNDhrXO\nniY0wYFr1I11/L1L/I4loKF+Pue2EvnzpdfN2M75Nl1C351TL7qs8wucRpEBcOs/\n1hlSuXQy6iolsxVmHDdSdflSM0/58WbOM7ePiDEoZypvrtaVue9YQCBroabP5bTG\nXeB3edu5IV8slz+mxtE3iNQ05Lg0Lu5oLLCojodxnNsSxTFLsPA=\n=Ywh2\n-----END PGP SIGNATURE-----\n"

Then, given this message property, a regex is applied to split the signature from the rest:

    def extract_tag_signature(message)
      if message.nil?
        [nil, false]
      elsif !message.valid_encoding?
        [message, false]
      elsif message.match(/^#{GPG_SIGNATURE_PREFIX}$/)
        message = message.split("#{GPG_SIGNATURE_PREFIX}\n", 2).first
        [message, true]
      elsif message.match(/^#{SSH_SIGNATURE_PREFIX}$/)
        message = message.split("#{SSH_SIGNATURE_PREFIX}\n", 2).first
        [message, true]
      elsif message.match(/^#{SMIME_SIGNATURE_PREFIX}$/)
        message = message.split("#{SMIME_SIGNATURE_PREFIX}\n", 2).first
        [message, true]
      else
        [message, false]
      end
    end

Where the constants are:

    GPG_SIGNATURE_PREFIX   = "-----BEGIN PGP SIGNATURE-----".freeze
    SMIME_SIGNATURE_PREFIX = "-----BEGIN SIGNED MESSAGE-----".freeze
    SSH_SIGNATURE_PREFIX   = "-----BEGIN SSH SIGNATURE-----".freeze

So what should we do?

I believe that we should honour annotated messages as they are written. I have more confidence in following the platform's example than any other approach to doing so.

Therefore I believe we should use $(contents) and apply a similar regex stripping operation in the CLI.

[nedtwigg]

From the standpoint of an end-user

• I would not want to break existing clients of the command
• Your discussion above documents the issue very well (thanks!!) and it's easy for me to modify our tags with a clear subject line from here on out. For my purposes, this issue is now resolved.
• Perhaps --notes-from-tag could take an optional argument, e.e.g
  • --notes-from-tag gh-format
  • --notes-from-tag %(contents:subject)%0a%0a%(contents:body)
  • etc
  • but that seems super super low priority because it's so easy to workaround if you understand what's going on, which I now do. Thanks very much @williammartin and @babakks, very helpful!!

[babakks]

@nedtwigg I'm glad your issue is resolved. And yes, I too think adding the format argument just complicates things.

@williammartin Thanks for the investigation. I agree about the user experience, and it'll be nice to make it work just like the platform does. I'll submit a PR for this soon. I think the important part is for the changes to keep the current behavior intact for well-formatted annotations.

[babakks]

@williammartin I've submitted the PR for this. @andyfeller is already assigned for a review, but since you've already investigated this it'd be great to also have your review.

[williammartin]

I'm on vacation until the middle of August. I'm happy to review on my return if Andy doesn't want to. ✌

[andyfeller]

Firstly, great write up in the issue; makes sense, easy to follow, and I'm familiar with the topic so I'm comfortable reviewing. I had a few questions that aren't huge blockers, so will work to ensure we get @babakks PR shipped asap.