[apparmor] [administrivia] git commit message style discussion

[Steve Beattie]

[Discussing the immediate issue before opening to the larger topic.]

On Thu, Nov 02, 2017 at 01:00:50PM -0700, John Johansen wrote:
> > We walked through a merge yesterday with this merge request:
> > 
> >   https://gitlab.com/apparmor/apparmor/merge_requests/1
> > 
> > The audit trail of who merged the code is implicitly present in the
> > merge commit. By default, there's no information about who reviewed the
> > changes but the merge commit contains a mention of where to find the
> > merge request and that page will contain much more info about who
> > reviewed which parts of the merge request.
> > 
> That makes the dangerous assumption we keep our infrastructure on gitlab,
> and don't endup migrating again (this is the 4 or 5 migration in the
> projects history).

Indeed[0].

> I would strongly prefer having that information
> integral to the commit message.
> 
> > I'm fine with the default merge commit message. I think Steve had an
> > issue with the subject line of the default merge commit message. I'll
> > let him voice his opposition to it and maybe he'll have a better suggestion.

It's part of a bigger discussion that I've been meaning to raise about
our commit messages. The issue I have with something like:

>  Merge branch 'make-variable' into 'master'
>
>  all: Use the MAKE variable
>
>  See merge request apparmor/apparmor!1

is twofold.

The first issue is that I often use tools that initially only expose
the --one-line subject for commits (e.g. for identifying fixes to
cherry-pick back to older releases). So I want as much identifying
information in the one-line commit message to distinguish what a
specific commit is about. Having it be "merged branch 'some-topic'
into 'master'" is kinda wasteful and depends heavily on the person
naming the branch to be merged in a sufficiently descriptive manner.
(An example: Tyler named his merge branch that added cscope files to
the gitignore file as 'csope' -- in a year, would you guess from the
name that that merge only affected the gitignore file?)

(As an aside, there is I think a cultural difference between bzr tools
and git tools: in bzr, when looking at a commit history (whether
one-line or not), the individual commits of merged branches are elided
or unexpanded, whereas git tools seem to incorporate/expand them. I'm
used to and prefer the bzr behavior because if I'm spelunking history,
I don't have to do anything to ignore an irrelevant branch, whereas
I don't have that option with git without making the tool work on
specific revisions. To give an example, I've put up screenshots from
bzr qlog, gitg, and tig viewing the apparmor tree at

  https://imgur.com/a/sUMHs

(gitk screenshot elided because it's an even bigger usability
disaster.) I will say that gitlab's generated graph is pretty nice:

  https://gitlab.com/apparmor/apparmor/network/master

but even it, like every other git tool I've looked at (caveat, I haven't
look at that many) doesn't let you contract or expand branches.)

The second issue is related to John's, in that for a merge of a single
commit, the above message is... okay, since the actual commit will
likely have most of the justification for the merge proposal captured
in the commit message itself. For multi-commit merges, I'd prefer to
see the explanation/justification for the merge in the merge commit --
think the explanation that goes in message [00/xx] of e.g. a quilt
series, Adding the reviewer information there, as John suggested in
IRC, would be good, too.

> uhmm, no that really fails the migration test

To be fair, we failed a bit with the migration from bzr to git, in
that particularly for cherry-picked commits pulled to older releases,
we used trunk bzr revisions as back-references. With a bunch of manual
effort, I might have been able to convert into something universally
meaningful. But I agree that going forward, we should try to avoid
losing such information/references.

Thanks for the discussion.

[0] Historical transitions: Immunix internal hosting + bugzilla -> Novell
    Forge -> Launchpad -> Gitlab. It also marks the fourth VCS used:
    CVS (private) -> svn (private) -[history break]-> svn (public) -> bzr -> git.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20171103/49e036a1/attachment.sig>