Skip to content

Conversation

@mmonge
Copy link

@mmonge mmonge commented Aug 31, 2012

I just migrate a repo from SVN to Git, some commits did not have message and gitlab shows HTTP 500 when it loads commits index view.

Added an "if" to check Nil and show a warning instead of HTTP 500.

@travisbot
Copy link

This pull request passes (merged bae83625 into ed89125).

@travisbot
Copy link

This pull request passes (merged ef4c3a68 into ed89125).

@dzaporozhets
Copy link
Contributor

I think it should be handled in commit but not in gfm

@mmonge
Copy link
Author

mmonge commented Aug 31, 2012

I thought the same but the error is only creating the link... as a Git user i prefer not to see that warning while viewing the commit message.

But if you still think is better at commit, then i'll move it there tomorrow.

@dzaporozhets
Copy link
Contributor

i think commit should always has a message. At lease something like "-- no message"

@dzaporozhets
Copy link
Contributor

what do you think?

@riyad
Copy link
Contributor

riyad commented Aug 31, 2012

It really seems to be common (at least for converted repos) to have commits with no message.
I think it should be solved it in the Model or Grit layers.
I think it should be something "loud" and "ugly" like "[NO COMMIT MESSAGE]" so you can see it is a bad thing. ;)

I just migrate a repo from SVN to Git, some commits did not have message and gitlab raises Nil Exception when it loads commits index view.

    Added a condition on commit model to check blank? on commit message and show a warning instead of a Exception.
@mmonge
Copy link
Author

mmonge commented Aug 31, 2012

@randx you are right, i made the modification and it looks ok to me

Took me a while understand a little bit the commit model :-P

@travisbot
Copy link

This pull request passes (merged 6d00eb6 into ed954eb).

@IsNull
Copy link

IsNull commented Sep 2, 2012

Kind of a duplicate, see #1317

@mmonge
Copy link
Author

mmonge commented Sep 2, 2012

@IsNull is right, i checked the solution from #1317 and it solves the problem, but @randx suggested a warning message from the model, may be if it's changed to:

  safe_message.split(/\n/, 2).first || "[NO COMMIT MESSAGE]"

Because in the HTML generates an empty link:

<a href="/repo/commits/adbf0b0b94a7823d1d2265a94732a9131ba021f1" class="commit_short_id">adbf0b0b9</a>
<strong class="commit-author-name">John Smith</strong>
<span class="dash">–</span>
<img alt="Cc4fa6c88cb12543c758f7b235d77ee5?s=40&amp;d=identicon" class="avatar" src="http://www.gravatar.com/avatar/cc4fa6c88cb12543c758f7b235d77ee5?s=40&amp;d=identicon" width="16">

<!-- SEE EMPTY LINK -->
<a href="/repo/commits/adbf0b0b94a7823d1d2265a94732a9131ba021f1" class="row_title"></a>

<span class="committed_ago">
  4 months ago
  &nbsp;
</span>

@randx please choose the solution that you think goes better with the current standard of the project, i'm new in this project and i do not know the standard yet :-) . As i said, #1317 also solves the problem.

@dzaporozhets
Copy link
Contributor

Why you defined setter method?

@mmonge
Copy link
Author

mmonge commented Sep 2, 2012

Because i was not sure how delegate works, i researched a little and looks like a way to access methods directly from a child resource and it said that is used to save writing getter/setter methods in models, so i understand that if i removed the delegate, i should replace it with getter/setter methods.

Am i wrong? Or in this case the setter is not used at all? Or there is a better way to do it keeping the delegate?

I'm sorry if i make to many questions, i really like learning about ruby and rails. I hope not to be a nuisance.

@dzaporozhets
Copy link
Contributor

we dont delegate setter method so its no need to define it.
also replace
self.commit.message.blank? ? '[NO COMMIT MESSAGE]' : self.commit.message
to
self.commit.message || '[NO COMMIT MESSAGE]'

cause we dont need to handle empty string etc. We afraid only of nil

@IsNull
Copy link

IsNull commented Sep 3, 2012

Is this [NO COMMIT MESSAGE] really necessary? It will look very strange and unreadable on Repos which have lots of untitled commits, which tends to happen often when Repos are converted from SVN.

It is also ambiguous as no message is obviously empty. No need to say that there is nothing. :)

@dzaporozhets
Copy link
Contributor

missing commit message is a bad thing in git. I want users get this ugly message.

@IsNull
Copy link

IsNull commented Sep 3, 2012

It is a bad thing, and I also tend to force my team to use a message on every commit.

I want users get this ugly message.

Questionable if this is the right approach. ;)

Probably make it a setting in gitlab.yml like:
commit.emptyMessageReplacement=[NO COMMIT MESSAGE]

This way, anyone can adjust it.

@dzaporozhets
Copy link
Contributor

Changed my mind. I've decided it will be logical to move it under decorator.

@dzaporozhets
Copy link
Contributor

fixed by c7cfe3d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants