Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--notes-from-tag does not preserve newlines #9299

Closed
nedtwigg opened this issue Jul 8, 2024 · 8 comments · Fixed by #9385
Closed

--notes-from-tag does not preserve newlines #9299

nedtwigg opened this issue Jul 8, 2024 · 8 comments · Fixed by #9385
Labels
bug Something isn't working gh-release relating to the gh release command help wanted Contributions welcome p3 Affects a small number of users or is largely cosmetic

Comments

@nedtwigg
Copy link

nedtwigg commented Jul 8, 2024

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:

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
@nedtwigg nedtwigg added the bug Something isn't working label Jul 8, 2024
@cliAutomation cliAutomation added the needs-triage needs to be reviewed label Jul 8, 2024
@williammartin williammartin added the gh-release relating to the gh release command label Jul 18, 2024
@williammartin
Copy link
Member

williammartin commented Jul 18, 2024

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

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

%(contents:body)'

For your tag this shows:

➜  blowdryer-diffplug git:(main) git tag --list 8.0.0 --format='%(contents:subject)

%(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)

%(contents:body)' | cat
This is a subject

This is a body with newlines
It really has newlines
For realsies
image showing release created correctly from notes

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.

@williammartin williammartin added p3 Affects a small number of users or is largely cosmetic help wanted Contributions welcome and removed needs-triage needs to be reviewed labels Jul 18, 2024
@babakks
Copy link
Contributor

babakks commented Jul 21, 2024

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
Copy link
Member

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:

Image showing the tag message without 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
Copy link
Author

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) %(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
Copy link
Contributor

babakks commented Jul 25, 2024

@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
Copy link
Contributor

babakks commented Jul 29, 2024

@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
Copy link
Member

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

nedtwigg added a commit to diffplug/blowdryer-diffplug that referenced this issue Aug 4, 2024
@andyfeller
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working gh-release relating to the gh release command help wanted Contributions welcome p3 Affects a small number of users or is largely cosmetic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants