[PATCH 3 of 3] ui: Add option to set number of revision hash digits displayed

Eric M. Hopper hopper at omnifarious.org
Sun Jul 26 17:04:07 CDT 2009


On Fri, 2009-07-24 at 14:33 -0500, Matt Mackall wrote:
> On Fri, 2009-07-24 at 12:20 -0700, Eric M. Hopper wrote:
> > On Fri, 2009-07-24 at 13:52 -0500, Matt Mackall wrote:
> > > On Fri, 2009-07-24 at 11:29 -0700, Eric Hopper wrote:
> > > > mercurial/cmdutil.py         |   4 ++--
> > > >  mercurial/commands.py        |  14 +++++++-------
> > > >  mercurial/dispatch.py        |   2 ++
> > > >  mercurial/patch.py           |   2 +-
> > > >  mercurial/ui.py              |  29 +++++++++++++++++++++++++++++
> > > >  tests/test-debugcomplete.out |   2 ++
> > > >  tests/test-extension.out     |   2 ++
> > > >  7 files changed, 45 insertions(+), 10 deletions(-)
> > > 
> > > Not really happy with this one. I've been careful to keep ui largely
> > > unaware of Mercurial's implementation details. The biggest exception is
> > > path handling, and I've been planning to pull that out too. If we start
> > > putting helper functions in ui, it's going to burst a dam and ui is
> > > going to grow dozens of random helpers and become very ad-hoc.
> > > 
> > > Couldn't we just update a node._shortdigits?
> > 
> > That could be done.  The problem is that there are a few places in which
> > node.short is used which aren't intended for display on the UI.  So the
> > node module would have to get a new function that was specifically
> > designed to format node identifiers for ui output.  That would be
> > perfectly reasonable to do though.
> 
> I would do the reverse: change the non-ui users to a different name or
> possibly back to hex. What are these users?

I went through and did an inventory and took some notes.  There is one
place where its used that I consider wrong, and one another where I
consider it quite distressing.

Additionally, URLs make for an interesting case.  Ideally you would like
URL identifiers to live for the entire lifetime of a repository.  But I
can think of a use-case in which 12 hex digits is likely to prove to be
too small.  If, for example, you had some kind of distributed filesystem
built on top of Mercurial that did automated commits you could easily
end up with a repository with over a million commits in it, and at that
point collisions between 12 digit hex ids come with the realm of
possibility.  This means that if you still want URLs that last forever
you will have to plan ahead for such cases and go for longer URLs.

The case I consider very distressing is where short node identifiers are
stuffed into environment variables given to external programs.
Environment variable values used to pass important information are not a
UI feature, they are an internal feature, and those cases should be
using hex.

Lastly, changectx and filectx implement __str__ as a wrapper around
short.  I find this worrisome because it then becomes very hard to tell
where its used.  I think that changectx and filectx should have an
explicit routine for getting a node identifier and that the __str__
function should return a value that's a little more ornate than a bare
hex short node identifier to discourage people from using the result in
certain ways.

I'll attach the notes I took.  Keep in mind that these are very
off-the-cuff.

-- 
A word is nothing more or less than the series of historical
connotations given to it. That's HOW we derive meaning, and to claim
that there is an arbitrary meaning of words above and beyond the way
people use them is a blatant misunderstanding of the nature of language.
-- Anonymous blogger
-- Eric Hopper (hopper at omnifarious.org http://www.omnifarious.org/~hopper)--
-------------- next part --------------
cmdutil.make_filename - Not sure about this one.

cmdutil.changeset_printer._show - Should probably use stretchy size.

cmdutil.changeset_templater - formatnode needs to be fixed to be
                              stretchy.

cmdutil.walkchangerevs - Should definitely use stretchy size

commands.debugindex - Having the short be longer than 12 characters
                      messes up the formatting, but that's likely OK.

commands.export - uses cmdutil.make_filename

commands.diff - Should it ever use short?  I suppose.  Definitely should
                be stretchy though.

commands.tag - Should definitely be size adjustable.

commands.tags - Should also definitely be stretchy

context.changectx.__str__ - Hmmm, this could be used in a whole lot of
                            places.  I don't think this was the best
                            choice of str function for this type.

context.filectx.__str__ - Hmmm, again this could be used in a whole lot
                          of places.  I don't think this was the best
                          choice of str function for this type.


error.LookupError.__init__ - Should probably be stretchy

filemerge.filemerge - Wow, those environment variables should definitely
                      be using full node hashes.  The programs that read
                      those variables can shorten the hashes themselves
                      for display if they need it.

hbisect.bisect - Yes, should be stretchy

help - short is mentioned in a few places and the description should
       likely be updated slightly.

localrepo.localrepository.lookup - Yes, should be stretchy.

localrepo.localrepository.findincoming - Yes, should be stretchy

localrepo.localrepository.findoutgoing - Yes, should be stretchy

localrepo.localrepository.addchangegroup - Yes, should be stretchy

patch.diff - Yes, should be stretch, should possibly not even use short

repair._bundle - Used to create filename, maybe should be stretchy,
                 should possibly not even use short.

tags.findglobaltags2 - Yes, should be stretchy.

tags._readtagcache - Yes, should be stretchy

tags._writetagcache - Yes, should be stretchy

templatefilters.filters - Needs a special function to work properly and
                          maybe shouldn't be replaced as it might be
                          used for URL building.

verify._verify.checkentry - Yes, should be stretchy

verify._verify - Yes, should be stretchy

hgext.bookmarks.setcurrent - Yes.

hgext.bugzilla - Various uses...  hard to say, a lot of them are
                 templates.

hgext.extdiff.snapshot - Used in constructing directory names, probably
                         doesn't matter.

hgext.fetch..fetch - Should be stretchy

hgext.gpg - numerous, hard to say because code is twisty, but probably.

hgext.hgcia.ciamsg.* - These should probably be independently
                       configurable.

hgext.hgk.* - Lots of uses, should probably be stretchy.

hgext.keyword.kwtemplater - Use of short in templates, I don't think it
                            should at all.  Those strings might last
                            forever in some moldy binary.

hgext.mq.* - All uses should be stretchy.

hgext.notify - Use of short filter in templates.

hgext.transplant - All uses should be stretchy.

hgext.win32text - All uses should be stretchy

templates - No template uses 'short' without a 'node|' before it.  Only
            map-cmdline.default uses node|formatnode.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 664 bytes
Desc: This is a digitally signed message part
Url : http://selenic.com/pipermail/mercurial-devel/attachments/20090726/9a0dd4a4/attachment.pgp 


More information about the Mercurial-devel mailing list