#146 viewing some commits gets 504

Closed
opened 1 month ago by bill-auger · 35 comments

viewing the following commit will cause the browser to hang for several minutes then finally load the dead spider page

5e5d17cb10

is unclear what is special about this commit - the diff is not particular large - other commits on the same repo load ok

i have this project mirrored all over - gogs seems to be the only one with this trouble - this is the diff for that commit if it helps:

https://github.com/bill-auger/loopidity/commit/5e5d17cb107e678bf7243636309cdc49bd48bc74

https://pagure.io/loopidity/c/5e5d17cb107e678bf7243636309cdc49bd48bc74?branch=master

https://gitlab.com/bill-auger/loopidity/commit/5e5d17cb107e678bf7243636309cdc49bd48bc74

https://bitbucket.org/bill-auger/loopidity/commits/5e5d17cb107e678bf7243636309cdc49bd48bc74

viewing the following commit will cause the browser to hang for several minutes then finally load the dead spider page https://notabug.org/bill-auger/loopidity/commit/5e5d17cb107e678bf7243636309cdc49bd48bc74 is unclear what is special about this commit - the diff is not particular large - other commits on the same repo load ok i have this project mirrored all over - gogs seems to be the only one with this trouble - this is the diff for that commit if it helps: https://github.com/bill-auger/loopidity/commit/5e5d17cb107e678bf7243636309cdc49bd48bc74 https://pagure.io/loopidity/c/5e5d17cb107e678bf7243636309cdc49bd48bc74?branch=master https://gitlab.com/bill-auger/loopidity/commit/5e5d17cb107e678bf7243636309cdc49bd48bc74 https://bitbucket.org/bill-auger/loopidity/commits/5e5d17cb107e678bf7243636309cdc49bd48bc74
zPlus commented 1 month ago

Confirm this. I also have the same problem.

Confirm this. I also have the same problem.

For some reason it seems that the 'git' command that gogs spawns takes way longer than normal to view those particular commits. I'll try to figure out why that is.

For some reason it seems that the 'git' command that gogs spawns takes way longer than normal to view those particular commits. I'll try to figure out why that is.
bill-auger commented 1 month ago
Poster

this one is curious - there does not seem to be anything peculiar about the diff - one thing to notice is that the commit message has (issue #30) in it - which makes a link but leads to a 404 - but if that is a bug i would think that would have been noticed sooner

this one is curious - there does not seem to be anything peculiar about the diff - one thing to notice is that the commit message has (issue #30) in it - which makes a link but leads to a 404 - but if that is a bug i would think that would have been noticed sooner

That is a good catch though! I'll try to reproduce that in that manner.

That is a good catch though! I'll try to reproduce that in that manner.
bill-auger commented 1 month ago
Poster

ok it's not the commit message - i rebased and changed the message and the new commit has the same issue

ok it's not the commit message - i rebased and changed the message and the new commit has the same issue
bill-auger commented 1 month ago
Poster

ok i split it up into one file per commit and isolated the problem to one file

4d2edb0020

ok i split it up into one file per commit and isolated the problem to one file https://notabug.org/bill-auger/loopidity/commit/4d2edb0020549a0751f5765277853d9bf5626184
bill-auger commented 1 month ago
Poster

yes definitely that one 'mk-clean' file is the troublemaker

i rebased again just to re-order the commits and the commit with that same file is having the same issue

ca4c4aa4c7

yes definitely that one 'mk-clean' file is the troublemaker i rebased again just to re-order the commits and the commit with that same file is having the same issue https://notabug.org/bill-auger/loopidity/commit/ca4c4aa4c7b192aae53abfee1938823e41fd61e3
bill-auger commented 1 month ago
Poster

simply renaming the file fixed the problem

mk-clean → mkclean

f75776d5c4

all these commit are on this branch https://notabug.org/bill-auger/loopidity/commits/deleteme2

simply renaming the file fixed the problem mk-clean → mkclean https://notabug.org/bill-auger/loopidity/commit/f75776d5c49bb28d06652f7f6d3c2fc17c0806a2 all these commit are on this branch https://notabug.org/bill-auger/loopidity/commits/deleteme2
bill-auger commented 1 month ago
Poster

a simple test case does not exhibit this https://notabug.org/bill-auger/gogs-filename-bug

a simple test case does not exhibit this https://notabug.org/bill-auger/gogs-filename-bug

O_o OK, I'll try to trace that problem. I wonder if it has anything to do with Gogs' attempts to do syntax highlighting and mimetype detection. I guess Gogs just stops functioning in the goroutine for that request and the git command is just waiting with a full pipe

O_o OK, I'll try to trace that problem. I wonder if it has anything to do with Gogs' attempts to do syntax highlighting and mimetype detection. I guess Gogs just stops functioning in the goroutine for that request and the git command is just waiting with a full pipe
bill-auger commented 1 month ago
Poster

this is quite bizzarre indeed - it seems not to be the filename either - simply adding an empty commit on top of the troubled one has no problem - this must be some crazy hashing bug where it just does not like being in that precise state

70a3042474

on this branch

https://notabug.org/bill-auger/loopidity/commits/deleteme3

this is quite bizzarre indeed - it seems not to be the filename either - simply adding an empty commit on top of the troubled one has no problem - this must be some crazy hashing bug where it just does not like being in that precise state https://notabug.org/bill-auger/loopidity/commit/70a30424744280d4b9815862100bc80f1d9cd8d2 on this branch https://notabug.org/bill-auger/loopidity/commits/deleteme3
bill-auger commented 1 month ago
Poster

also this is the identical diff as the troublesome commit (literally the same file copied from the other project) but alone in it's own project so that kinda rule out syntax highlighting yea?

a18edde893

also this is the identical diff as the troublesome commit (literally the same file copied from the other project) but alone in it's own project so that kinda rule out syntax highlighting yea? https://notabug.org/bill-auger/gogs-filename-bug/commit/a18edde893db3a3da5c6a9cd215fc5f3302e8f32

Well, it was a good hypothesis while it lasted :) Thanks for digging, if you find anything more please keep updating this ticket. I'm at work now so I can't look into it immediately.

Well, it was a good hypothesis while it lasted :) Thanks for digging, if you find anything more please keep updating this ticket. I'm at work now so I can't look into it immediately.
bill-auger commented 1 month ago
Poster

ok i have isolated the bug to this tiny test case

https://notabug.org/bill-auger/gogs-filename-bug/commits/master

  • commit 1 adds empty file - no problem
  • commit 2 make file executable 755 - bug exposed

again this test-case repo has no problems on github or pagure

ok i have isolated the bug to this tiny test case https://notabug.org/bill-auger/gogs-filename-bug/commits/master * [commit 1](https://notabug.org/bill-auger/gogs-filename-bug/commit/afe784cd89fe568af8f9dddf8061a5d4a9e90566) adds empty file - no problem * [commit 2](https://notabug.org/bill-auger/gogs-filename-bug/commit/75c6f218e2cce527a4855fab5802f5f9d5a4ada6) make file executable 755 - bug exposed again this test-case repo has no problems on github or pagure * https://github.com/bill-auger/gogs-filename-bug * https://pagure.io/gogs-filename-bug

uch... That's... really, really bad 0_0. I hope there's an easy fix for this. I'll have a look when I can.

uch... That's... really, really bad 0_0. I hope there's an easy fix for this. I'll have a look when I can.
zPlus commented 1 month ago

Also the reverse is true

  1. commit file with 0744 fine
  2. chmod 0644 and commit same problem

btw I think git only tracks the executable bit of the owner. All other bits should be irrelevant.

Also the reverse is true 1. commit file with `0744` **fine** 2. `chmod 0644` and commit **same problem** btw I think git only tracks the executable bit of the owner. All other bits should be irrelevant.
bill-auger commented 1 month ago
Poster

this is what it shows me:

$ git diff HEAD^
diff --git a/mk-clean b/mk-clean
old mode 100644
new mode 100755
this is what it shows me: ``` $ git diff HEAD^ diff --git a/mk-clean b/mk-clean old mode 100644 new mode 100755 ```

So, the hypothesis is that any commit that changes the file mode breaks? Or any commit that only changes file mode fails?

In the latter case it could just be an issue in the diff renderer.

So, the hypothesis is that any commit that changes the file mode breaks? Or any commit that *only* changes file mode fails? In the latter case it could just be an issue in the diff renderer.
zPlus commented 1 month ago

@bill-auger I only used 0644 and 0744, I guess git automatically sets the same bit everywhere...

@bill-auger I only used 0644 and 0744, I guess git automatically sets the same bit everywhere...
bill-auger commented 1 month ago
Poster

im trying to look this up - the format seems to be not documented - maybe @hp knows if that is that a standard notation but it is probably recording the setuid, setgid, and sticky bits as well as the standard user/group/world

is the filename at all important? can this be repeated with any filename?

im trying to look this up - the format seems to be not documented - maybe @hp knows if that is that a standard notation but it is probably recording the setuid, setgid, and sticky bits as well as the standard user/group/world is the filename at all important? can this be repeated with any filename?
zPlus commented 1 month ago

I tried to reproduce this both on try.gogs and try.gitea. They seem to work OK.

Relevant note: I've only pushed commits with a single chmod change.

I tried to reproduce this both on [try.gogs](https://try.gogs.io/choccolata/trygogs-filename-bug/commits/master) and [try.gitea](https://try.gitea.io/choccolata/gitea-filename-bug/commits/master). They seem to work OK. Relevant note: I've only pushed commits with a single `chmod` change.
bill-auger commented 1 month ago
Poster

ok i was gonna suggest lets just toss this one over the fence but now it looks like it notabug only?

ok i was gonna suggest lets just toss this one over the fence but now it looks like it notabug only?
zPlus commented 1 month ago

@hp it seems to happen in both cases

Scenario 1

1. push empty file **OK**
2. chmod and push **BREAK**

Scenario 2

1. push empty file **OK**
2. add new file with some text in it
3. chmod (empty file)
4. push in a single commit the new file and the chmod change **BREAK**

I've also tried chmod-ing with both empty and non-empty files to see if it would made any difference. Same issue instead.

@hp it seems to happen in both cases Scenario 1 ``` 1. push empty file **OK** 2. chmod and push **BREAK** ``` Scenario 2 ``` 1. push empty file **OK** 2. add new file with some text in it 3. chmod (empty file) 4. push in a single commit the new file and the chmod change **BREAK** ``` I've also tried `chmod`-ing with both empty and non-empty files to see if it would made any difference. Same issue instead.
bill-auger commented 1 month ago
Poster

yea it does not seem to matter what is in the file - but the name is important - have you tried with a different filename?

yea it does not seem to matter what is in the file - but the name is important - have you tried with a different filename?
zPlus commented 1 month ago

@bill-auger filename doesn't seem to make any difference for me... Do you have any particular name or combination that I could try?

@bill-auger filename doesn't seem to make any difference for me... Do you have any particular name or combination that I could try?
bill-auger commented 1 month ago
Poster

ok i just tried a different filename - same deal - the filename is not important and the file contents are not important

ok i just tried a different filename - same deal - the filename is not important and the file contents are not important
zPlus commented 1 month ago

@bill-auger This seems to work, could you try to reproduce?

  1. add empty file and commit it
  2. change file content
  3. change file mode
  4. commit changes

It looks to me that Gogs crashes when reading the diff, if the diff contains at least one file with only permission changes (some parsing error?). But since it works on try.gogs, they probably fixed it already (just my wild guess).

@bill-auger This seems to work, could you try to reproduce? 1. add empty file and commit it 2. change file content 3. change file mode 4. commit changes It looks to me that Gogs crashes when reading the diff, if the diff contains at least one file with only permission changes (some parsing error?). But since it works on try.gogs, they probably fixed it already (just my wild guess).
bill-auger commented 1 month ago
Poster

@hp -

sry forgot to answer directly your question - it seems to be any commit that changes the executable bit of any file but only if it changes only the executable bit - adding bytes to the file in the same commit as making it executable does not cause the problem for some reason

this branch demonstrates that:

https://notabug.org/bill-auger/gogs-filename-bug/commits/different-filename

@hp - sry forgot to answer directly your question - it seems to be any commit that changes the executable bit of any file but only if it changes only the executable bit - adding bytes to the file in the same commit as making it executable does not cause the problem for some reason this branch demonstrates that: https://notabug.org/bill-auger/gogs-filename-bug/commits/different-filename
bill-auger commented 1 month ago
Poster

@zPlus -

yes i think we both verified the same thing - the commit can change any number of files but the probelm is if any one files changes only it's executable bit

@zPlus - yes i think we both verified the same thing - the commit can change any number of files but the probelm is if any one files changes only it's executable bit
zPlus commented 1 month ago

The only significant change in the diff seems to be the index line, like for example index 0000000..e69de29; so I'd put my money on the fact that Gogs is looking for it even when there is no such line.

The only significant change in the diff seems to be the `index` line, like for example `index 0000000..e69de29`; so I'd put my money on the fact that Gogs is looking for it even when there is no such line.

Well, that should at least be easy enough to find in the code.

Well, that should at least be easy enough to find in the code.
zPlus commented 1 month ago

I hope I've pinned down the problem.

The diff stuff is in a sub-module at vendor/github.com/gogits/git-module/repo_diff.go. On line 137 there is a function called ParsePatch that tries to understand what a diff is about. In particular where there is CHECK_TYPE:, there is a switch checking the prefix of each line to determine the type of that particular diff. When only the file mode has changed, the diff looks like this

diff --git a/myfile b/myfile
old mode 100644
new mode 100755

apparently Gogs is looking for one of new file, deleted, index, or similarity index 100%; neither old mode nor new mode match any case, so it's probably returning NULL or something like that. They seem to have added an additional check for old mode a couple of months ago.

I hope this fixes the issue.

I hope I've pinned down the problem. The diff stuff is in a sub-module at `vendor/github.com/gogits/git-module/repo_diff.go`. On line [137](https://github.com/gogits/git-module/blob/1de103dca47a72afccccb4ccd6085110874f3551/repo_diff.go#L137) there is a function called `ParsePatch` that tries to understand what a `diff` is about. In particular where there is `CHECK_TYPE:`, there is a `switch` checking the prefix of each line to determine the type of that particular diff. When only the file mode has changed, the diff looks like this ```text diff --git a/myfile b/myfile old mode 100644 new mode 100755 ``` apparently Gogs is looking for one of `new file`, `deleted`, `index`, or `similarity index 100%`; neither `old mode` nor `new mode` match any `case`, so it's probably returning `NULL` or something like that. They seem to [have added an additional check for `old mode`](https://github.com/gogits/git-module/commit/1de103dca47a72afccccb4ccd6085110874f3551#diff-9b2fdd34fb38bd382de7b569c08a6bb2) a couple of months ago. I hope this fixes the issue.
bill-auger commented 1 month ago
Poster

brilliant @zPlus

brilliant @zPlus
zPlus commented 1 month ago

Team work :)

Let's hope this simple fix can solve the problem.

Team work :) Let's hope this simple fix can solve the problem.
zPlus commented 3 days ago

This issue has been fixed and can be closed. Gogs still doesn't show mode change in the UI (eg 0644 -> 0755) but this is only a minor nuisance; it doesn't crash anymore.

This issue [has been fixed](https://notabug.org/hp/gogs/src/master/vendor/github.com/gogits/git-module/repo_diff.go#L296) and can be closed. Gogs still doesn't show mode change in the UI (eg `0644 -> 0755`) but this is only a minor nuisance; it doesn't crash anymore.
hp closed 3 days ago
Sign in to join this conversation.
No Milestone
No assignee
3 Participants
Loading...
Cancel
Save
There is no content yet.