per-file message support into server: common config check {patch}

Bug #185224 reported by Chad Miller on 2008-01-23
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Bazaar
Medium
Unassigned

Bug Description

bzr-gtk already supports per-file commit messages. Most of its functionality should be moved into Bazaar core.

One notable reason to put these in one place is the risk of implementors getting the config key or values wrong. We shouldn't duplicate that code in several places.

Once added to the core, bzr-gtk should import this method, and on failure fall back to its current implementation. Qbzr and the built-in commit implementation can follow with implementations of per-file messages.

Chad Miller (cmiller) wrote :
Lukáš Lalinský (luks) wrote :

I can't speak for bzr core, but I'd rather not have this functionality in QBzr. I believe it unnecessarily fractions the information that makes sense only as a whole (because is an atomic operation and applies to _all_ files), making it harder to search/display/etc. commit messages and I personally don't see any benefit.

Lukáš Lalinský wrote:
> I can't speak for bzr core, but I'd rather not have this functionality
> in QBzr. I believe it unnecessarily fractions the information that makes
> sense only as a whole (because is an atomic operation and applies to
> _all_ files), making it harder to search/display/etc. commit messages
> and I personally don't see any benefit.

Some development groups want this feature because it allows them to
optionally provide *additional* information on a per file basis.
Per-file commit messages shouldn't replace the overall commit message,
just complement it.

Ian C.

Chad Miller (cmiller) wrote :

Yes, indeed, it is still an error to omit a global commit-message.

Per-file messages encourage specific descriptions or rationales that would otherwise be too verbose for changeset browsing. One shouldn't normally see these messages unless viewing a particular revision of a particular file.

John A Meinel (jameinel) wrote :

I would say this is would be better as a post to <email address hidden> rather than a bug report.

That said, here is a quick mini-review

1) It was originally meant as "per file commit messages" which got shortened to "per_file_commits" over time. I'm happy enough with "per_file_comments", but internally Bazaar calls it a commit "message" not a "comment". I realize some systems use the term "comments" here. I would probably prefer "per_file_messages".

2) I'm thinking it should probably be part of BranchConfig, as BranchConfig.get_enable_per_file_messages() rather than a standalone helper in msgeditor.py

3) I believe ConfigObj has support for "get_bool()" rather than having us do our own check, but it may not be exposed in an easy-to-use manner.

4) If we *are* going to merge it, then it would certainly need tests. Either in bzrlib/tests/test_msgeditor.py or bzrlib/tests/test_config.py depending on where the code goes in.
The tests should exercise what the default return value is, what happens when the value is set in branch.conf, what happens when it is an unrecognized value (I would probably rather fail with at least a warning if it is "foobar" rather than assuming that means "No".)

5) You have a mutter() but most users won't see it. I'm not sure if "note()" or "warning()" would be too obvious, but it would only nag when the user does a commit. I don't know of many people who habitually read .bzr.log except when an error occurs. (I know I don't.)

You might even consider using a DeprecationWarning since we would be switching to a new keyword, which seems very much a Deprecation action.

James Westby (james-w) on 2008-01-24
Changed in bzr:
importance: Undecided → Wishlist
status: New → In Progress
Vincent Ladeuil (vila) on 2010-09-18
Changed in bzr:
status: In Progress → Confirmed
importance: Wishlist → Medium
Jelmer Vernooij (jelmer) on 2011-02-01
tags: added: commit
Jelmer Vernooij (jelmer) on 2017-11-08
tags: added: check-for-breezy
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers