プロジェクト

全般

プロフィール

Vote #67825

完了

Truncate Git revision labels in Activity page/feed and allow configurable length

Admin Redmine さんがほぼ2年前に追加. ほぼ2年前に更新.

ステータス:
Closed
優先度:
通常
担当者:
-
カテゴリ:
SCM_3
対象バージョン:
開始日:
2010/08/09
期日:
進捗率:

100%

予定工数:
category_id:
3
version_id:
20
issue_org_id:
6092
author_id:
9865
assigned_to_id:
11192
comments:
27
status_id:
5
tracker_id:
2
plus1:
0
affected_version:
closed_on:
affected_version_id:
ステータス-->[Closed]

説明

Git uses hashes for the revision labels rather than an incremental counter, so a lot of space is used up just displaying the label in the activity page / feed that would otherwise show more useful info (such as a longer part of the commit message).

Redmine already truncates the Git revision labels to the first 8 chars on the Repository page, which is more than enough for most projects, so it would be good to see this on the Activity page. If we could also configure the length to be even shorter if required, that would be great. The href for any links would obviously still contain the full revision hash.


journals

Attached patch applies the fix. Also available at http://github.com/asoltys/redmine
--------------------------------------------------------------------------------
Oh, I should mention I didn't implement this as a configurable setting as requested. Maybe later...
--------------------------------------------------------------------------------
Please leave the status changes to the devs, thank you. (resolved is for patches in trunk but not in stable yet).

The patch looks good to me, I'm not sure we'd need it configurable either. If we take the git etiquette of 72 chars per line, add the "Revision " and ": " around the revision hash (11 chars total, might be somewhat different in other locales) and 8 chars for the revision hash, we end up with 91 chars, which leaves some space for the other locales to have longer strings.

@JB: I like the idea as it is, but I'd rather make a @short_revision@ attribute each adapter can override at will rather than a hard default like that. I especially think people using SCMs with sequential numbering would be quite surprised to see commit @10000000@ followed by commit @10000000@ :-)
--------------------------------------------------------------------------------
Agreed Felix. I'll work on moving it at least into a variable if not into a setting. Also, sorry about changing the ticket status, I'll know now not to do that anymore.
--------------------------------------------------------------------------------
Ok, I've implemented this as a setting now. I figure this way it's not necessary to have separate values in each adapter. I've defaulted the value to 8. If someone with an svn repository with over 10 million revisions needs to increase this, they can!
--------------------------------------------------------------------------------
Adam Soltys wrote:
> Ok, I've implemented this as a setting now. I figure this way it's not necessary to have separate values in each adapter. I've defaulted the value to 8. If someone with an svn repository with over 10 million revisions needs to increase this, they can!

Your patch is great!
Can you implement setting of "7.days" at source:tags/1.0.0/app/models/repository/git.rb#L50 and http://www.redmine.org/attachments/3272/git-fast-browse.patch in #4773 ?

--------------------------------------------------------------------------------
Toshi, ok, I did these two things as settings for you, but I don't actually think they should be included in trunk and presented to the average user. They're specific to git, for one thing, and they deal with some pretty obscure internal performance mechanisms.

A better solution in my opinion would be to revisit the git adapter and see if we can get some more performance out of it by default. If github can do it, so can we!
--------------------------------------------------------------------------------
I just realized I was incorrectly using the log_display_limit setting instead of the truncation setting in a couple places of code. Fixed that. Here's the patch again with just the truncation setting addressing the original issue, this time done properly.

I've also placed this change in a "for-eric" branch on my github fork and sent him a pull request. http://github.com/asoltys/redmine/tree/for-eric
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Adam Soltys wrote:
> Toshi, ok, I did these two things as settings for you, but I don't actually think they should be included in trunk and presented to the average user. They're specific to git, for one thing, and they deal with some pretty obscure internal performance mechanisms.
>

7.days problem is reported at #6013 and "git log -n 1 FILE_or_DIR" problem is reported at #5096.

--------------------------------------------------------------------------------
Thanks for pointing to those issues Toshi. I hadn't been following redmine development for the last year or so but I'm glad to see that other people have taken up the charge on these matters. I was exposed first-hand to some of the general performance problems that redmine has with distributed scm's back when I worked on #1406, so I've gained an appreciation and desire to fix these things once and for all.

I still don't agree with exposing these two settings at the moment because I still see them as temporary hacks to solve performance problems that I think we could do better on. In particular, I have some ideas about possibly pre-fetching and caching some repository history or implementing some more sophisticated on-demand lookups and partial history scans that might give us adequate performance without requiring users to sacrifice the "last revision" field or manually specify how far back to look for dirty changesets.

I need to do a bit more thinking and design-work before I tackle a full-on reimplementation of the git and mercurial adapters though. So in the meantime I'm just trying to get my feet wet again with contributing to redmine. I do have plans to become a regular contributor through the coming fall and winter though and repository management and performance will be a central area of interest for me.
--------------------------------------------------------------------------------
Didn't have the time to read everything but thank you guys for your work on these topics.

About the original issue, I agree we should truncate git's sha1 revision names but I have 2 worries about it :
* I don't like the idea of introducing a Setting for that. There are already bunches of useless settings in redmine, and consulting administration panel always give me the feeling that we have too many things here which leads to confusion and bloat software ; I'd rather turn it into a constant in the class in a first approach. I think nobody cares Github truncates revision names to 7 or 8 chars because nobody reads these IDs, they're just here to point to the commit. We should have the same approach, decide a number (6? 7? 8? 10?) and begin with this.
* after some thoughts about it, I think this modification must be applied at adapter level, only to SCM with non sequential revision numbers.

Adam: can you confirm the last patch is the only one to keep ? We can delete other patches so that it's clearer which one to work on.
--------------------------------------------------------------------------------
Yuya's implementation of Mercurial overhaul (#4455) shows activity with mercurial style such as "12:ff70262d476d":http://dev.openttdcoop.org/projects/ttrs/repository/revisions/ff70262d476d .
You can see at http://dev.openttdcoop.org/projects/ttrs/activity .

I pushed yuya's "mq":http://bitbucket.org/yuja/redmine-mq-issue4455 to my github repository "branch":http://github.com/marutosi/redmine/tree/issue-4455-yuya-mq-20100825 .
--------------------------------------------------------------------------------
Jean-Baptiste Barth wrote:
> About the original issue, I agree we should truncate git's sha1 revision names but I have 2 worries about it :
> * I don't like the idea of introducing a Setting for that. There are already bunches of useless settings in redmine, and consulting administration panel always give me the feeling that we have too many things here which leads to confusion and bloat software ; I'd rather turn it into a constant in the class in a first approach. I think nobody cares Github truncates revision names to 7 or 8 chars because nobody reads these IDs, they're just here to point to the commit. We should have the same approach, decide a number (6? 7? 8? 10?) and begin with this.
> * after some thoughts about it, I think this modification must be applied at adapter level, only to SCM with non sequential revision numbers.

Agreed, I'd go with 8 chars and an extra @short_revision@ virtual attribute in the adapter.
--------------------------------------------------------------------------------
Adam Soltys wrote:
> In particular, I have some ideas about possibly pre-fetching and caching some repository history or implementing some more sophisticated on-demand lookups and partial history scans that might give us adequate performance without requiring users to sacrifice the "last revision" field or manually specify how far back to look for dirty changesets.

7.days is only git adapter problem.
There are some ideas of git at after "note-13":http://www.redmine.org/issues/4773#note-13 of #4773 .

Because mercurial has revision number, Redmine can know mercurial "last committed revision"(tip) easily.
But mercurial tip *is not latest revision* (see "note-23":http://www.redmine.org/issues/4455#note-23 of #4455 ).
This is the reason of Redmine 1.0.x mercurial adapter fetching performance problem.
Yuya fixed this problem at "note-144":http://www.redmine.org/issues/4455#note-144 of #4455 and I pushed to my github "hg-patches-svntrunk branch":http://github.com/marutosi/redmine/tree/hg-patches-svntrunk .

--------------------------------------------------------------------------------
Thanks for looking at this Jean-Baptiste. I agree with you about the importance of avoiding software bloat and I don't like exposing obscure technical settings to the users either.

I'll have to look at the code though to see if it's possible to move this setting into the adapters. I think in some places where the revisions are being shortened we don't have access to the adapter objects, which is why I went with a global setting, but maybe I can figure something out. So yes, the latest patch supersedes the other ones, but hold off on looking at it because I'll probably be coming up with another one soon in light of this latest discussion.

Toshi, thanks again for the info. I looked at Yuya's branch quickly and it seems like he's done a lot of good work that I need to take a closer look at.
--------------------------------------------------------------------------------
Adam Soltys wrote:
> Toshi, thanks again for the info. I looked at Yuya's branch quickly and it seems like he's done a lot of good work that I need to take a closer look at.

Hi, in short, my patches try to switch revision formats by repository classes (Mercurial or not).
Implemented as follows:
* implement Changeset#format_identifier and Revision#format_identifier
* pass changeset or revision object to format_revision and link_to_revision helpers,
so that it can utilize #format_identifier
--------------------------------------------------------------------------------
Adam Soltys wrote:
> Toshi, ok, I did these two things as settings for you, but I don't actually think they should be included in trunk and presented to the average user. They're specific to git, for one thing, and they deal with some pretty obscure internal performance mechanisms.
>
> A better solution in my opinion would be to revisit the git adapter and see if we can get some more performance out of it by default. If github can do it, so can we!

Github seems to use Ajax for calling "git log -n 1 FILE_or_DIR".

Redmine calls "git log -n 1 FILE_or_DIR" not only in browsing directory tree but also in cat/diff/annotate.
Because entry() of abstract_adapter.rb calls entries() at source:tags/1.0.1/lib/redmine/scm/adapters/abstract_adapter.rb#L84 .
--------------------------------------------------------------------------------
I imported Yuya's "MQ":http://bitbucket.org/yuja/redmine-mq-issue4455/changeset/ab997af9efe7 (Mercurial Queues) exclude Mercurial adapter to "github":http://github.com/marutosi/redmine/commit/9f1b56ec3947b4c8e295c796907bfef4bb968360 and done "minor change":http://github.com/marutosi/redmine/commit/9225af5c2a4f5410f53cb8c2ec4f869e2dc11d9a.
Now my github branch is "issue-4455-yuya-mq-20100825-exclude-hg":http://github.com/marutosi/redmine/commits/issue-4455-yuya-mq-20100825-exclude-hg

And I attach the patch and git activity page images of source:tags/1.0.1/test/fixtures/repositories/git_repository.tar.gz.

--------------------------------------------------------------------------------
Adam Soltys wrote:
> I need to do a bit more thinking and design-work before I tackle a full-on reimplementation of the git and mercurial adapters though. So in the meantime I'm just trying to get my feet wet again with contributing to redmine. I do have plans to become a regular contributor through the coming fall and winter though and repository management and performance will be a central area of interest for me.

Adam, please consider #5357.
Mercurial has same problem (#3567).
Yuya fixed this problem at "note-144":http://www.redmine.org/issues/4455#note-144 of #4455.
--------------------------------------------------------------------------------
FYI:
"Bazaar merged revisions don't show up":http://www.redmine.org/boards/2/topics/17587
--------------------------------------------------------------------------------
Toshi MARUYAMA wrote:
> "git log -n 1 FILE_or_DIR" problem is reported at #5096.

I found JPL posted a patch for this problem at "note-13":http://www.redmine.org/issues/1435#note-13 of #1435 .
But current redmine support git branch, this patch is incompatible.

--------------------------------------------------------------------------------
I update the patch to set truncating character number at adapter level.
This patch contains unit tests.

And I pushed to my github.

* https://github.com/marutosi/redmine/commits/git-activity
* https://github.com/marutosi/redmine/commit/710d3f355de44d87ccb3ec0caf49ec77917ca16b

This patch truncates 8 characters.

You can set truncating number.

* blame/annotate
https://github.com/marutosi/redmine/blob/710d3f355de44d87/lib/redmine/scm/adapters/git_adapter.rb#L271
* Others
https://github.com/marutosi/redmine/blob/710d3f355de44d87/app/models/repository/git.rb#L39

I attach truncating 12 characters images.

--------------------------------------------------------------------------------
#3244, #3945 and #6092 are duplicate.
--------------------------------------------------------------------------------
attachment:revision-truncate-20101213.diff has a lack of fixtures at unit tests.
I fixed it.
revision-truncate-20101213-fix-fixtures.diff is additional patch for attachment:revision-truncate-20101213.diff .

And I pushed to my github.

* https://github.com/marutosi/redmine/commits/git-activity
* https://github.com/marutosi/redmine/commit/325ee1e73bafeae5567d3e3a7ba09d1d2181841a

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Implemented in svn trunk by r4613 and 1.1 by r4614.
--------------------------------------------------------------------------------


related_issues

relates,Closed,3724,Mercurial repositories display revision ID instead of changeset ID
relates,New,7047,Git adapter very slow when a commit modifies a lot of files
relates,Closed,7146,Git adapter lost commits before 7 days from database latest changeset
relates,Closed,6013,git tab,browsing, very slow -- even after first time
relates,Closed,2610,Git repository comments should use git convention for commit messages
duplicates,Closed,3244,Shorten git commit ids in Atom feeds and on activity page
duplicates,Closed,3945,Shorten revision ID for git events

Admin Redmine さんがほぼ2年前に更新

  • カテゴリSCM_3 にセット
  • 対象バージョン1.1.0_20 にセット

他の形式にエクスポート: Atom PDF

いいね!0
いいね!0