Hi, Jelmer!

On Mar 21, Jelmer Vernooij wrote:

> bzr uncommit only really changes the branch tip, it doesn't change the
> working tree or the repository. Either way, an exception being raised
> at a certain point should never lead to data loss or corruption,
> especially for things that are just used for display. If you can still
> reproduce the corruption issue, please file a bug.

this needs uncommit to be aborted in a specific place, which cannot be
done without modifying the code (bzr or bzr-gtk, or a special plugin).
I don't know whether it'll qualify as a valid bug report.

> The decision whether to fall back to displaying a URL should be made
> by the caller of this particular function - returning the URL here
> makes that pretty hard.

Changed to raise an exception.

> > As you like. I can change it back, return a tuple everywhere, and
> > join it into a string in the bzr-gtk. Should I?
> Please do - at the moment the only caller is the bzr command line UI
> which indeed uses a colon, but other callers may not. Related to this,
> what do you think about returning the bug tracker instance and not just
> the abbreviation? That way the caller can decide what from the bug
> tracker to use, and they don't have to do any additional lookups.

I cannot return a tracker instead of an abbreviation.
Because the abbreviation is not a property of the base BugTracker class,
every sub-class implements it differently and in a general case I cannot
find the abbreviation, if given a tracker only.

But if you'd like I can change this method to return a tuple (tracker,
abbreviation, bug_id). Or I can change the BugTracker class to provide
the abbreviation.

> > > > +def encode_fixes_bug_ids(fixes, branch):
...
> Why would gcommit use strings rather than tuples? I imagine e.g.
> selecting a bug tracker from a drop-down box and then entering a bug
> number. Either way, even if both bzr-gtk and bzr's command-line use a
> colon-separated string it's UI code that doesn't really belong in
> bzrlib.bugtracker.

Changed.

> > > BzrCommandError is mainly intended to be used in the command line
> > > client (e.g. bzrlib/builtins.py) and probably not as useful in an
> > > API as the UnknownBugTrackerAbbreviaton and MalformedBugIdentifier
> > > exceptions. 
> > Sure. I can remove try/except, and put it back in the caller.
> > But then in the UnknownBugTrackerAbbreviation, I could only say
> > "Unrecognized bug tracker: %s. Commit refused.", I won't have access to
> > the bug id there. Is that ok?
> That seems like a regression from what we have at the moment. I also
> wonder how useful this refactoring still is after we move the
> exception handling back to bzrlib.builtins. Do we really need
> encode_fixes_bug_ids ? 

Yes, you're right. If we move exception handling back to bzrlib.builtins
and also move splitting of a bug id string on ':' back to
bzrlib.builtins, then there will be nothing left here and this
function - encode_fixes_bug_ids - is not needed anymore.

Removed.

> You should probably also add a couple of tests for lookup_id_from_url.
> If I can help with this, please let me know.

Does that mean that you agree with the patch in principle and only wants
the details to be polished? Then I'll need to add the tests, and add
lookup_id_from_url to other bugtracker classes.

Note that reverting the url is not the only possible solution. Bzr
could, for example, store the original bug id in the changeset, together
with or instead of a bug url. Then there would be no need to revert the
url back to bug id.

Regards,
Sergei