Feature #6092

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

Added by Phil Moorhouse over 7 years ago. Updated almost 7 years ago.

Status:ClosedStart date:2010-08-09
Priority:NormalDue date:
Assignee:Toshi MARUYAMA% Done:

100%

Category:SCM
Target version:1.1.0
Resolution:Fixed

Description

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.

patch.diff Magnifier (825 Bytes) Adam Soltys, 2010-08-12 07:30

patch.diff Magnifier (19.3 KB) Adam Soltys, 2010-08-15 00:06

patch.diff Magnifier (29 KB) Adam Soltys, 2010-08-15 06:10

revision_truncation_length_as_setting.diff Magnifier (19.3 KB) Adam Soltys, 2010-08-15 06:34

yuya-issue-4455-mq-ab997af9e-exclude-hg.diff Magnifier (14.9 KB) Toshi MARUYAMA, 2010-09-19 16:17

git-act-before.png (31.5 KB) Toshi MARUYAMA, 2010-09-19 16:17

git-act-after.png (24.3 KB) Toshi MARUYAMA, 2010-09-19 16:17

revision-truncate-20101213.diff Magnifier (17.8 KB) Toshi MARUYAMA, 2010-12-13 15:52

git-12-digits-activity.png (62.7 KB) Toshi MARUYAMA, 2010-12-13 15:52

git-12-digits-annotate.png (98.3 KB) Toshi MARUYAMA, 2010-12-13 15:52

git-12-digits-browse.png (181 KB) Toshi MARUYAMA, 2010-12-13 15:52

revision-truncate-20101213-fix-fixtures.diff Magnifier (1.09 KB) Toshi MARUYAMA, 2010-12-20 12:06


Related issues

Related to Redmine - Defect #3724: Mercurial repositories display revision ID instead of cha... Closed 2009-08-10
Related to Redmine - Defect #7047: Git adapter very slow when a commit modifies a lot of files New 2010-12-04
Related to Redmine - Defect #7146: Git adapter lost commits before 7 days from database late... Closed 2010-12-21
Related to Redmine - Defect #6013: git tab,browsing, very slow -- even after first time Closed 2010-08-02
Related to Redmine - Feature #2610: Git repository comments should use git convention for com... Closed 2009-01-28
Duplicated by Redmine - Feature #3244: Shorten git commit ids in Atom feeds and on activity page Closed 2009-04-27
Duplicated by Redmine - Feature #3945: Shorten revision ID for git events Closed 2009-09-30

Associated revisions

Revision 4613
Added by Toshi MARUYAMA almost 7 years ago

Changing revision label and identifier at SCM adapter level (#3724, #6092)

Contributed by Yuya Nishihara.

History

#1 Updated by Adam Soltys over 7 years ago

  • File patch.diffMagnifier added
  • Status changed from New to Resolved
  • % Done changed from 0 to 100

Attached patch applies the fix. Also available at http://github.com/asoltys/redmine

#2 Updated by Adam Soltys over 7 years ago

Oh, I should mention I didn't implement this as a configurable setting as requested. Maybe later...

#3 Updated by Felix Schäfer over 7 years ago

  • Category set to UI
  • Status changed from Resolved to New

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 :-)

#4 Updated by Adam Soltys over 7 years ago

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.

#5 Updated by Adam Soltys over 7 years ago

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!

#6 Updated by Toshi MARUYAMA over 7 years ago

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 ?

#7 Updated by Adam Soltys over 7 years ago

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!

#8 Updated by Adam Soltys over 7 years ago

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

#10 Updated by Toshi MARUYAMA over 7 years ago

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.

#11 Updated by Adam Soltys over 7 years ago

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.

#12 Updated by Jean-Baptiste Barth over 7 years ago

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.

#13 Updated by Toshi MARUYAMA over 7 years ago

Yuya's implementation of Mercurial overhaul (#4455) shows activity with mercurial style such as 12:ff70262d476d .
You can see at http://dev.openttdcoop.org/projects/ttrs/activity .

I pushed yuya's mq to my github repository branch .

#14 Updated by Felix Schäfer about 7 years ago

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.

#15 Updated by Toshi MARUYAMA about 7 years ago

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 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 of #4455 ).
This is the reason of Redmine 1.0.x mercurial adapter fetching performance problem.
Yuya fixed this problem at note-144 of #4455 and I pushed to my github hg-patches-svntrunk branch .

#16 Updated by Adam Soltys about 7 years ago

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.

#17 Updated by Yuya Nishihara about 7 years ago

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

#18 Updated by Toshi MARUYAMA about 7 years ago

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 .

#19 Updated by Toshi MARUYAMA about 7 years ago

I imported Yuya's MQ (Mercurial Queues) exclude Mercurial adapter to github and done minor change.
Now my github branch is 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.

#20 Updated by Toshi MARUYAMA about 7 years ago

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 of #4455.

#22 Updated by Toshi MARUYAMA about 7 years ago

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 of #1435 .
But current redmine support git branch, this patch is incompatible.

#23 Updated by Toshi MARUYAMA almost 7 years ago

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

And I pushed to my github.

This patch truncates 8 characters.

You can set truncating number.

I attach truncating 12 characters images.

#24 Updated by Toshi MARUYAMA almost 7 years ago

#3244, #3945 and #6092 are duplicate.

#26 Updated by Toshi MARUYAMA almost 7 years ago

  • Category changed from UI to SCM
  • Assignee set to Toshi MARUYAMA

#27 Updated by Toshi MARUYAMA almost 7 years ago

  • Status changed from New to Closed
  • Target version set to 1.1.0
  • Resolution set to Fixed

Implemented in svn trunk by r4613 and 1.1 by r4614.

Also available in: Atom PDF