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

[Chad Miller]

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.

[Lukáš Lalinský (luks)]

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.

[Ian Clatworthy (ian-clatworthy)]

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

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

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.