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

Fix tag commit message #21693

Closed
wants to merge 7 commits into from
Closed

Fix tag commit message #21693

wants to merge 7 commits into from

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Nov 5, 2022

Fixes #21687

Changes in tag.go:

parseTagData gets called with the input

        object a0f5a9a9c6958455c818eecba2199846a96b2612
        type commit
        tag 5.0.4
        tagger silverwind <[email protected]> 1605812507 +0100

        * update deps, fix csstree-validator api (silverwind)
        -----BEGIN PGP SIGNATURE-----

        iQIzBAABCAAdFiEEWPb2jX6FS2mqyJRQLmK0HJOGlEMFAl+2wRsACgkQLmK0HJOG
        lENNjw/+Lyi7Chthc+ZHaFUYJLwZ9a0z/itLkMlQ6XzHIRcsSY0B3KXIIvwta8HV
        k9YcfQyIyWBECOVvUd+sdcPEkKrhT4npPQV31nqUFGONN7jRU9nuY7f4KHZ3mY9L
        KlaRC4ybivfB9qK315NbOVrkG4gckWoGfaTg/0VP4WSbvPVXMKOpRqJC2I+eqiFD
        S4NanKrLuf/pwDcNJ+Ix6QWKYs0lIxJp49QEiVyse0Nj7XwTVRtSc5hz4zrAO72i
        sTHTsiMIkPBCiwo9dzYBA7PjfpPMfJz4BzcUZt0phOU/jGEbDtZ0b16mlBYLl865
        fAuphh3fW3qOSB2eYvDtapLdantnzRg01p25RSThRbG9WDQAphJoIkWW3Evgr1/v
        qISjXNNZFmumz5BVEjfTtT85qdht2nsuwX+Loh7HIDF4Qkd2XpoJFJs5UhK/2B77
        JMZMBq+MwALCrk8Kb6/Kgu959mDtwwB614MHVbxbRLPzqVRLyKisVqA9Fpgj0lXD
        6iRwm9O2tdGskRt+ErfUmiYMwUIQsEwxcBI/iFjwZ756Xs19ZcIvy1e4cN/Mp0Gv
        v037qqQwgFwdIwLwfXGFagcfO7tXARvG5eHhwyzsujY8pZxo8PIKyPBzFqjl0SIE
        sJH6vCQfPoObtsf95OaNVbGaX3YO2uEAhfS4Uq+E558bcfgJzWg=
        =Ko7n
        -----END PGP SIGNATURE-----

As you can see, the message is not the same as for the commit. My change builds the expected message.

Changes in other files:
The old code overwrites data of the commit with data from the tag. That's the reason of the broken UI. Because of the wrong output from parseTagData the correct values were replaced with wrong values. In my tests both sides of the assignment contained the same values now so I removed the assignments. But I don't know if that's true all the time.

Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm this works at least for the no-gogit case. Also I fixed the unused fmt import.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Nov 7, 2022
@lunny
Copy link
Member

lunny commented Nov 8, 2022

CI failure is related.

@silverwind
Copy link
Member

silverwind commented Nov 9, 2022

I noticed that the same issue is visible on tag messages on a connected Drone instance. I'm not sure if this integration is push or pull, but I certainly wonder if those messages will be fixed there as well:

image

@silverwind
Copy link
Member

silverwind commented Nov 9, 2022

Checking further in the drone issue, I see that the cut-off message is delivered in head_commit via webhook as tag event:

"ref": "refs/tags/v1.9.1",
"head_commit": {
  "id": "24c26a39ac96b6facb6f0ffb7c0cb6826ebef48c",
  "message": "* update js deps"
}

Comparing a commit event, it is fine there already:

"ref": "refs/heads/master",
"commits": [
  {
    "id": "24c26a39ac96b6facb6f0ffb7c0cb6826ebef48c",
    "message": "v1.9.1\n\n* update js deps"
  }
"head_commit": {
   "id": "24c26a39ac96b6facb6f0ffb7c0cb6826ebef48c",
   "message": "v1.9.1\n\n* update js deps"
}

@silverwind silverwind added the outdated/backport/v1.18 This PR should be backported to Gitea 1.18 label Nov 9, 2022
@silverwind
Copy link
Member

silverwind commented Nov 12, 2022

Can confirm this also fixes the message in webhook tag events. Before:

"message": "* test commit (silverwind)",

After:

"message": "0.2.2\n\n* test commit (silverwind)\n",

So I'd say this is a very valuable fix.

@silverwind
Copy link
Member

Test failure is still related:

--- FAIL: TestGetTagCommitWithSignature (0.01s)
    repo_commit_test.go:50: 
          Error Trace:  /drone/src/modules/git/repo_commit_test.go:50
          Error:        Expected value not to be nil.
          Test:         TestGetTagCommitWithSignature
    repo_commit_test.go:52: 
          Error Trace:  /drone/src/modules/git/repo_commit_test.go:52
          Error:        Not equal: 
                        expected: "tag"
                        actual  : "Added short link\n"
                        
                        Diff:
                        --- Expected
                        +++ Actual
                        @@ -1 +1,2 @@
                        -tag
                        +Added short link
                        +
          Test:         TestGetTagCommitWithSignature

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Nov 13, 2022

I'm not sure anymore this is the solution we want. If there is a signature present, this sig will not match the message now...

@silverwind
Copy link
Member

silverwind commented Nov 13, 2022

Not sure I understand. Isn't the PGP signature supposed to sign the whole commit message, including the first line? At least this makes me believe there is no special case that would exclude the first line.

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Nov 13, 2022

Maybe I'm wrong but from the example in the first post the code now extracts the message

5.0.4

* update deps, fix csstree-validator api (silverwind)

If we would build the "tag data" again from our object we would end up with something like

tag 5.0.4
tagger silverwind <[email protected]> 1605812507 +0100

5.0.4

* update deps, fix csstree-validator api (silverwind)

and that would not match the signature. So it may not be desired to modify the message. It may be better to build the new format on the fly with a method like this?

func (t *Tag) String() string {
	if t.Name != "" {
		if t.Message == "" {
			return t.Name
		} else {
			return t.Name + "\n\n" + t.Message
		}
	}
	return t.Message
}

But that does not work here for example:

		commit.CommitMessage = tag.Message
		commit.Author = tag.Tagger
		commit.Signature = tag.Signature

I'm a bit puzzled...

@silverwind
Copy link
Member

silverwind commented Nov 13, 2022

Viewing this tag in git cli shows:

  1. The commit message has both the version title and the body
  2. The tag body itself does not have the version title because it's not part of the tag message

I'm thinking we should not even parse the tag when viewing repo commits as tags are just pointers to commits (with annotated tags also have a message attached).

$ git show 5.0.4
tag 5.0.4
Tagger: silverwind <[email protected]>
Date:   Thu Nov 19 20:01:47 2020 +0100

* update deps, fix csstree-validator api (silverwind)
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEWPb2jX6FS2mqyJRQLmK0HJOGlEMFAl+2wRsACgkQLmK0HJOG
lENNjw/+Lyi7Chthc+ZHaFUYJLwZ9a0z/itLkMlQ6XzHIRcsSY0B3KXIIvwta8HV
k9YcfQyIyWBECOVvUd+sdcPEkKrhT4npPQV31nqUFGONN7jRU9nuY7f4KHZ3mY9L
KlaRC4ybivfB9qK315NbOVrkG4gckWoGfaTg/0VP4WSbvPVXMKOpRqJC2I+eqiFD
S4NanKrLuf/pwDcNJ+Ix6QWKYs0lIxJp49QEiVyse0Nj7XwTVRtSc5hz4zrAO72i
sTHTsiMIkPBCiwo9dzYBA7PjfpPMfJz4BzcUZt0phOU/jGEbDtZ0b16mlBYLl865
fAuphh3fW3qOSB2eYvDtapLdantnzRg01p25RSThRbG9WDQAphJoIkWW3Evgr1/v
qISjXNNZFmumz5BVEjfTtT85qdht2nsuwX+Loh7HIDF4Qkd2XpoJFJs5UhK/2B77
JMZMBq+MwALCrk8Kb6/Kgu959mDtwwB614MHVbxbRLPzqVRLyKisVqA9Fpgj0lXD
6iRwm9O2tdGskRt+ErfUmiYMwUIQsEwxcBI/iFjwZ756Xs19ZcIvy1e4cN/Mp0Gv
v037qqQwgFwdIwLwfXGFagcfO7tXARvG5eHhwyzsujY8pZxo8PIKyPBzFqjl0SIE
sJH6vCQfPoObtsf95OaNVbGaX3YO2uEAhfS4Uq+E558bcfgJzWg=
=Ko7n
-----END PGP SIGNATURE-----

commit a0f5a9a9c6958455c818eecba2199846a96b2612 (tag: 5.0.4, local/master, gitea-try/master)
Author: silverwind <[email protected]>
Date:   Thu Nov 19 20:01:47 2020 +0100

    5.0.4

    * update deps, fix csstree-validator api (silverwind)

@silverwind
Copy link
Member

silverwind commented Nov 13, 2022

If it helps, the tag messages on my repos are generated here. Note that it supplies the changelog separately as the tag message, and that the tag message excludes the version line present in the commit message. Maybe that is where the confusion comes from.

tag.Message = tag.Name
} else if tag.Name != "" {
tag.Message = tag.Name + "\n\n" + tag.Message
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this joining is incorrect, we should not alter the tag message. What was wrong was just the retrieval of the tagged commit message.

tagObject.Message = tagObject.Name
} else if tagObject.Name != "" {
tagObject.Message = tagObject.Name + "\n\n" + tagObject.Message
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, I think we should not alter the tag message.

@@ -108,7 +108,7 @@ func (repo *Repository) getCommitFromBatchReader(rd *bufio.Reader, id SHA1) (*Co
return nil, err
}

commit.CommitMessage = strings.TrimSpace(tag.Message)
commit.CommitMessage = tag.Message
commit.Author = tag.Tagger
commit.Signature = tag.Signature
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we overwriting the commit's data here with incorrect data from the tag? I think this is the root of the problem and I guess if we just remove these three lines, the issue may be fixed.

silverwind added a commit to silverwind/gitea that referenced this pull request Nov 13, 2022
It is not correct to return tag data like the tag's message when
commit data requested. This changes fixes commit retrieval by tag for
both the latest commit in the UI and the commit info on tag webhook
events.

Fixes: go-gitea#21687
Replaces: go-gitea#21693
@silverwind
Copy link
Member

Alternative PR: #21804

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Nov 13, 2022

I added back the overwriting because of the tests and that was the point where I started to have doubts.

@silverwind
Copy link
Member

silverwind commented Nov 13, 2022

I see my changes are pretty similar to your first PR, minus the changes in modules/git/tag.go, we'll see if I run into the same issues, but adding back the overwriting is definitely not the right fix.

I've added a commit to the test repo that has both a signed commit and a signed tag pointing to it, that should satisfy the failing test at least.

@silverwind
Copy link
Member

silverwind commented Nov 13, 2022

The problem you might have been facing is that the test commit for TestGetTagCommitWithSignature is not actually signed properly as verified by git verify-commit. Only the test tag is signed properly. With my new commit, both tag and commit are signed correctly as validated by git verify-commit and git verify-tag.

@KN4CK3R KN4CK3R closed this Nov 13, 2022
@KN4CK3R KN4CK3R deleted the fix-21687 branch November 13, 2022 21:02
@zeripath zeripath removed the outdated/backport/v1.18 This PR should be backported to Gitea 1.18 label Jan 13, 2023
lunny added a commit that referenced this pull request Mar 2, 2023
It is not correct to return tag data when commit data is requested, so
remove the hacky code that overwrote parts of a commit with parts of a
tag.

This fixes commit retrieval by tag for both the latest commit in the UI
and the commit info on tag webhook events.

Fixes: #21687
Replaces: #21693

<img width="324" alt="Screenshot 2022-11-13 at 15 26 37"
src="https://user-images.githubusercontent.com/115237/201526975-736c6ea7-ad6a-467a-a823-9a63d6ecb718.png">

<img width="789" alt="image"
src="https://user-images.githubusercontent.com/115237/201526876-90a13ffc-1e5c-4d76-911b-f1ae51e8eaab.png">

---------

Co-authored-by: Lunny Xiao <[email protected]>
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Mar 3, 2023
It is not correct to return tag data when commit data is requested, so
remove the hacky code that overwrote parts of a commit with parts of a
tag.

This fixes commit retrieval by tag for both the latest commit in the UI
and the commit info on tag webhook events.

Fixes: go-gitea#21687
Replaces: go-gitea#21693

<img width="324" alt="Screenshot 2022-11-13 at 15 26 37"
src="https://user-images.githubusercontent.com/115237/201526975-736c6ea7-ad6a-467a-a823-9a63d6ecb718.png">

<img width="789" alt="image"
src="https://user-images.githubusercontent.com/115237/201526876-90a13ffc-1e5c-4d76-911b-f1ae51e8eaab.png">

---------

Co-authored-by: Lunny Xiao <[email protected]>
jolheiser pushed a commit that referenced this pull request Mar 3, 2023
Backport #21804

It is not correct to return tag data when commit data is requested, so
remove the hacky code that overwrote parts of a commit with parts of a
tag.

This fixes commit retrieval by tag for both the latest commit in the UI
and the commit info on tag webhook events.

Fixes: #21687
Replaces: #21693

<img width="324" alt="Screenshot 2022-11-13 at 15 26 37"
src="https://user-images.githubusercontent.com/115237/201526975-736c6ea7-ad6a-467a-a823-9a63d6ecb718.png">

<img width="789" alt="image"
src="https://user-images.githubusercontent.com/115237/201526876-90a13ffc-1e5c-4d76-911b-f1ae51e8eaab.png">

Co-authored-by: silverwind <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect latest commit message in UI when viewing tag
5 participants