Edgewall Software
Modify

Opened 7 years ago

Last modified 10 days ago

#11992 new defect

Allow git commit hashes to be abbreviated to 7 characters

Reported by: olly@… Owned by:
Priority: normal Milestone: next-major-releases
Component: plugin/git Version: 1.0.4
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

Currently an abbreviated git commit hash is only recognised by trac if it's at least 8 characters. But git itself defaults to abbreviating to 7 characters (if the 7 character abbreviation is unique in the repo) - to demonstrate this:

olly@pippikin:~/git/xapian$ git log -1 --oneline
575eab1 docs/: Add a document about character encoding, as suggested by James Aylett in #550.

The fix is a single character change - I'll attach a patch in a moment.

This seems to affect 1.0.4 and trunk.

Attachments (1)

trac-allow-7-character-git-hash.patch (364 bytes ) - added by olly@… 7 years ago.
proposed fix

Download all attachments as: .zip

Change History (16)

by olly@…, 7 years ago

proposed fix

comment:1 by olly@…, 7 years ago

BTW, we've been using this patch on http://trac.xapian.org/ for a few months, and it's working well for us.

comment:3 by Peter Suter, 7 years ago

Hm, the Git plugin has two config options shortrev_len (defaults to 7) and wiki_shortrev_len (defaults to 40).

comment:4 by Olly Betts <olly@…>, 7 years ago

I appreciate that abbreviated hashes can be (and indeed can become) ambiguous, and that a longer minimum length reduces the likelihood, but I think trac's default really should match git's.

Are you suggesting it using shortrev_len here rather than hard-coding a length?

in reply to:  4 ; comment:5 by Peter Suter, 7 years ago

Replying to Olly Betts <olly@…>:

I appreciate that abbreviated hashes can be (and indeed can become) ambiguous, and that a longer minimum length reduces the likelihood, but I think trac's default really should match git's.

I can see the logic of matching Git's default. This is not Git specific code though. Mercurial typically uses 12 characters.

Are you suggesting it using shortrev_len here rather than hard-coding a length?

I think [git] wiki_shortrev_len would be the relevant one for this case. Have you tried just changing that to 7 in your trac.ini?

This code was changed in #4261 from 6 to 8 to avoid false matches. I'm not sure what the best solution is. It does seem a bit weird that the Git plugin basically reimplements this and that the core functionality is not configurable.

in reply to:  5 ; comment:6 by Ryan J Ollos, 7 years ago

Replying to psuter:

Replying to Olly Betts <olly@…>: I can see the logic of matching Git's default. This is not Git specific code though. Mercurial typically uses 12 characters.

In that case, should the functionality be moved from the GitPlugin to the versioncontrol package?

Are you suggesting it using shortrev_len here rather than hard-coding a length?

I think [git] wiki_shortrev_len would be the relevant one for this case. Have you tried just changing that to 7 in your trac.ini?

Make sure to use wikishortrev_len as the option name. wiki_shortrev_len would be a better choice though - we should consider renaming it.

wikishortrev_len is 40 for this Trac instance. I can't figure out what is going on here:

I vaguely remember being confused about this in the past and discussing somewhere, but I can't locate any conversation at the moment.

in reply to:  6 comment:7 by Peter Suter, 7 years ago

(Another false match with 6 characters.)

Replying to rjollos:

Make sure to use wikishortrev_len

Oops, thanks for noticing.

Replying to psuter:

Mercurial typically uses 12 characters.

Hm, the mercurial-plugin actually also has its own competing functionality with 12+ characters, explaining this:

While the generic one only matches [...] wrapped links (with 8+ characters):

So the git plugin was not involved at all here.

In that case, should the functionality be moved from the GitPlugin to the versioncontrol package?

Possibly, yes. The current situation seems a bit confusing for sure.

comment:8 by Ryan J Ollos, 7 years ago

Milestone: next-major-releases

comment:9 by Peter Suter, 5 years ago

#12760 was closed as a duplicate.

comment:10 by strk@…, 5 years ago

I'm also having this problem, shortrev_len is not being honoured when set to 7 (which should be also the default, according to TracIni#git-shortrev_len-option. It's weird because using changeset:<short-hash>/git works great instead, so there's an asymmetricity between changeset:... and [...]

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

in reply to:  6 ; comment:11 by Jun Omae, 5 years ago

Replying to Ryan J Ollos:

In that case, should the functionality be moved from the GitPlugin to the versioncontrol package?

Sounds good.

Another issue, the hex-string wiki syntax in tracopt.v.git.git_fs works only for default repository. The hex-string wiki syntax in mercurial-plugin has same issue. Changeset id syntax in t.v.web_ui.changeset works for both default repository and non-default repository (r1 for default, r1/reponame for non-default).

The proposed patch is okay to me.

Since default of [git] shortrev_len is 7, even though I try to paste hex-string (7 chars) from changeset view between [ and ] in wiki, it doesn't work. It would annoy me.

comment:12 by Ryan J Ollos, 5 years ago

Proposed changes to try and clarify the documentation (for trunk):

  • tracopt/versioncontrol/git/git_fs.py

    diff --git a/tracopt/versioncontrol/git/git_fs.py b/tracopt/versioncontrol/git/git_fs.py
    index b43943a0a..13a71caee 100644
    a b class GitConnector(Component):  
    272272        """Wrap `GitRepository` in `CachedRepository`.""")
    273273
    274274    shortrev_len = IntOption('git', 'shortrev_len', 7,
    275         """The length to which a sha1 will be abbreviated to (must
    276         be >= 4 and <= 40).
     275        """The length at which a sha1 is abbreviated (must be >= 4
     276        and <= 40).
    277277        """)
    278278
    279279    wiki_shortrev_len = IntOption('git', 'wikishortrev_len', 40,
    280         """The minimum length for which a hex-string is formatted as
    281         a changeset TracLink (must be >= 4 and <= 40).
     280        """The minimum length for which a hex-string in wiki content
     281        is formatted as a changeset TracLink (must be >= 4 and <= 40).
    282282        """)
    283283
    284284    trac_user_rlookup = BoolOption('git', 'trac_user_rlookup', 'false',1

comment:13 by Ryan J Ollos, 5 years ago

comment:12 change committed to trunk in r16096.

comment:14 by ebouaziz@…, 5 years ago

About this option and the feature it is related to,

it also collides with the plantuml plugin: the hash in the name of the generated image is replaced in the page source (with a message saying the changeset does not exists in the repo) and the image is not displayed.

in reply to:  11 comment:15 by Ryan J Ollos, 4 years ago

Replying to Jun Omae:

Since default of [git] shortrev_len is 7, even though I try to paste hex-string (7 chars) from changeset view between [ and ] in wiki, it doesn't work. It would annoy me.

I've also experienced this frustration when copying a hash from the changeset page. For the interim, I've changed [git] shortrev_len to 8 for this site.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.