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

Show git-notes #6984

Merged
merged 13 commits into from May 24, 2019

Conversation

@CyberShadow
Copy link
Contributor

commented May 18, 2019

This PR implements basic support for displaying notes created by git notes (or software supporting it, such as magit).

The notes are read from Git's default ref location (refs/notes/commits), and thus it is assumed that the ref is pushed at that location to Gitea.

The implementation here is minimal; if the idea is OK, I can do any necessary changes.

@CyberShadow CyberShadow force-pushed the CyberShadow:show-git-notes branch 3 times, most recently from 434c841 to 5b8e243 May 18, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 19, 2019

Codecov Report

Merging #6984 into master will decrease coverage by <.01%.
The diff coverage is 29.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6984      +/-   ##
==========================================
- Coverage   41.49%   41.49%   -0.01%     
==========================================
  Files         440      441       +1     
  Lines       59459    59507      +48     
==========================================
+ Hits        24674    24692      +18     
- Misses      31566    31589      +23     
- Partials     3219     3226       +7
Impacted Files Coverage Δ
routers/repo/commit.go 24.23% <0%> (-0.77%) ⬇️
modules/templates/helper.go 47.35% <0%> (-1.08%) ⬇️
modules/git/notes.go 43.75% <43.75%> (ø)
models/repo_list.go 73.09% <0%> (+1.01%) ⬆️
models/unit.go 67.56% <0%> (+5.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5a98a2...dcbef74. Read the comment docs.

@lunny

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Any screenshot? And could you change the text on template to locale_en_US.ini?

@lunny lunny added the kind/feature label May 19, 2019

@lafriks lafriks added this to the 1.9.0 milestone May 19, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

I like the idea of this - however, is there any way to avoid reading the whole thing in memory and or making the page too big to render?

How about test the size of the note - if it's less than say 5Kb (most notes will be) then read it in otherwise provide a link that that the UI can asynchronously read. You can stick that as an API route and then you can test the note reading infrastructure relatively easy.

I think in general we should be more careful around ReadAll and page sizes.

@CyberShadow

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Any screenshot?

Sure:

Although git displays notes directly beneath commit messages, I opted to place the notes outside the commit box in Gitea's UI, because their authorship is not tied to the commit's author/committer. Placing them right by the commit message would make it look like they were written by the same person as the rest of the commit.

And could you change the text on template to locale_en_US.ini?

Done.

I like the idea of this - however, is there any way to avoid reading the whole thing in memory and or making the page too big to render?

Sorry, could you please explain why you think this is necessary?

As far as I know, the typical use of git-notes is editable additions to commit messages. The git-notes manual states this as well, so, there's the implication that notes and commit messages shouldn't be treated too differently. I'm not familiar of a use case for git-notes which would involve placing large amounts of data in the notes objects.

Even though it's possible to create arbitrarily large git notes, it's also possible to create arbitrarily large commit messages. As far as I can see, Gitea will always display the entire commit message, without limiting it, and without providing a view to see it in its entirety. So, it doesn't make sense to me to do this for git notes without also doing it for commit messages, considering that there don't seem to be any pertinent distinctions between the two.

Show resolved Hide resolved routers/repo/commit.go Outdated

CyberShadow added some commits May 18, 2019

@CyberShadow CyberShadow force-pushed the CyberShadow:show-git-notes branch from 9a675e4 to 157f709 May 19, 2019

@lunny

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Could you display the author and time of the git note?

@CyberShadow

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

I think it's possible, by looking at the last commit in the notes ref history which updated the note's path.

Note that it would be more accurate to say that it would be the person who last edited the note, since if you want to add a note to a commit and a note already exists for that commit, you have no choice but to edit the existing note.

@CyberShadow CyberShadow force-pushed the CyberShadow:show-git-notes branch from 268bb32 to 5002f09 May 20, 2019

@CyberShadow

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

Could you display the author and time of the git note?

Done, how does this look?

@CyberShadow CyberShadow force-pushed the CyberShadow:show-git-notes branch from 5002f09 to a101240 May 20, 2019

@CyberShadow CyberShadow force-pushed the CyberShadow:show-git-notes branch from a101240 to 7542929 May 20, 2019

@lafriks

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Could this be added in frame together with commit message?

@CyberShadow

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Something like this?

It doesn't look good in the dark theme, will likely need new CSS rules there.

@sapk

This comment has been minimized.

Copy link
Member

commented May 21, 2019

AFAIK, notes are not signed by gpg so maybe it is better to display them under the signature.

@CyberShadow

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

You certainly can sign the notes (by making signed commits in the notes ref), but that's a completely distinct signature from the commit that the notes are annotating.

Edit: You're right, git-notes does not sign commits made in the notes ref even when commit signing is otherwise enabled, so this is not a circumstance that we need to worry about.

@sapk

sapk approved these changes May 21, 2019

Copy link
Member

left a comment

This PR could be improved by :

  • Filtering display of big note that could break ui
  • Add tests to validate GetNote func
  • Displaying that a note exist (like gpg lock) in commit list and a preview on passing over
  • I personally would prefer to display notes like GPG. Attached to the commit block to display that it is linked but under GPG (since itsn't verifed by it)

But this lay down some simple bases that works and could be improved later. And since it is not a main feature those changes are not blocking from my point of view. LGTM

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels May 21, 2019

@CyberShadow CyberShadow force-pushed the CyberShadow:show-git-notes branch from 7373f70 to 8f937fa May 21, 2019

@CyberShadow

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Filtering display of big note that could break ui

I think that if we do this, it should also be done for commit messages.

Add tests to validate GetNote func

Test added :)

Displaying that a note exist (like gpg lock) in commit list and a preview on passing over

Good idea, I may submit a follow-up PR adding this.

I personally would prefer to display notes like GPG. Attached to the commit block to display that it is linked but under GPG (since itsn't verifed by it)

Do you have a mockup of your idea? Would it be essentially as the second screenshot I posted but without the gap between the blocks?

@sapk

This comment has been minimized.

Copy link
Member

commented May 21, 2019

It should also be done for commit messages.

Yes but that is for an other PR. It could be good to do it directly for notes but that isn't a blocking point (at least for me) as I suppose that people using note will understand and it could be fixed later.

Do you have a mockup of your idea?

I have done some testing here : sapk-fork@2f453e7
image
The main problem is that bottom class style does a lot to the GPG commit formatting. For testing I had to use inline css to counter that. The closest class would be ui attached block header.
I suggest you to not take time on this until other people validate that they prefer this way ;-)

@lafriks

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Yes I was thinking about something like what @sapk did

@CyberShadow CyberShadow force-pushed the CyberShadow:show-git-notes branch from 8f937fa to b20bf0a May 22, 2019

sapk and others added some commits May 21, 2019

@CyberShadow

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

I like what you did with the header so I adapted that:

dark
light

I am not sure about gluing the note block to the commit block, for two reasons:

  1. I think it looks a bit too clumped-up, it's not obvious to me at a first glance where the information about the commit ends and information about the note begins.
  2. Hypothetically, a repository can contain multiple refs storing git notes. These can be due to having been written by different authors, or generated by different tools. It's possible that in the future users of Gitea might want to show notes from more than one ref (e.g. by having a list of refs as a per-repository config); in this case there probably should be one bubble per notes ref containing a note for the commit.

Of course you have the final word :)

@techknowlogick

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@CyberShadow I like what you did in the screenshots, as it will allow showing that notes are GPG signed similar to how commits are (to be done in a future PR of course).

@lafriks

This comment has been minimized.

Copy link
Member

commented May 23, 2019

This looks really nice now

Show resolved Hide resolved routers/repo/commit.go Outdated
Show resolved Hide resolved routers/repo/commit.go Outdated
@lafriks

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Those two small suggestions but otherwise looks good

CyberShadow and others added some commits May 23, 2019

Apply suggestions from code review
Co-Authored-By: Lauris BH <lauris@nix.lv>

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels May 24, 2019

@lafriks lafriks merged commit a98e085 into go-gitea:master May 24, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details

@lafriks lafriks added the changelog label May 24, 2019

@lafriks

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Thanks for PR ;)

jeffliu27 added a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019

Show git-notes (go-gitea#6984)
* Show git-notes

* Make git-notes heading text localizable

* Refactor git-notes data fetching to a separate function

* Display the author and time of git notes

* Move note bubble inside the commit bubble

* Revert "Move note bubble inside the commit bubble"

This reverts commit c0951fe.

* Add test for git-notes

* testing ui

* Polish CSS

* Apply suggestions from code review

Co-Authored-By: Lauris BH <lauris@nix.lv>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
8 participants
You can’t perform that action at this time.