Re: Two new Git policy checks

[Owen Taylor <otaylor redhat com>]

On Thu, 2009-04-16 at 15:51 +0200, Santi Béjar wrote:
> 2009/4/16 Owen Taylor <otaylor redhat com>:
> > I added two checks that get on each "new" commit that wasn't previously
> > in the repository. (Excluding the import phase.) Like all the other
> > Git/Help pages the referenced pages haven't been written yet.
> >
> > Check 1: User hasn't configured their email address
> > ===================================================
> >
> > If a commit has an Author: email that ends in 'localhost.localdomain'
> > or '(none)', then the push errors out with:
> 
> I suppose that Committer is also tested.

I didn't check Committer, no, because my feeling is that if you started
using git, and messed this up, you would likely be making new commits,
and not committing other people's changes. But it would be easy enough
to add a check on that as well.

> > ---
> > The commits you are trying to push contain the author email
> > address '$email'. Please configure your
> > username and email address. See:
> 
> s/username/name/
> 
> >
> >  http://live.gnome.org/Git/Help/AuthorEmail
> >
> > For instructions about how to do this and how to fix your
> > existing commits.
> 
> s/For/for/

Thanks for fixes, I'll make the updates.

> > Check 2: git pull introduced stray merges
> > =========================================
> 
> What is wrong with these merges? They are correct and normal if you do
> distributed commits.

They *may* be correct and normal. But the vast majority of merges of
this form I have seen are little pointless:

 --A----B-----D--
    \       /
     ---C--/

branches in the history because someone committed C then did a git pull
without --rebase. They just make history less readable.

I don't expect many people to be committing to git.gnome.org from a
truly fully distributed operation, but if they are, I still wouldn't
expect commit messages like:

 Merge branch 'master' of ssh://....

The check actually looks for the URL form like that. Because I'd expect
you to add remotes for your collaborators and fetch and merge those
remotes. As a data point, git.git doesn't have a single commit that
would trigger this policy test.

If it does become a problem we can make the check more specific and look
for (ssh|git)://git.gnome.org instead of arbitrary ssh URLs.

> >
> > If a commit has a subject that matches
> >
> >  Merge branch.*of.*\(git\|ssh\):
> >
> > Then the commit errors out with:
> >
> > ---
> > The commit:
> >
> > [git log output for commit]
> >
> > Looks like it was produced by typing 'git pull' without the --rebase
> > option when you had local changes. Running 'git  pull --rebase' now
> > will fix the problem.
> 
> s/local changes/local commits/

I think I'll go with 'locally committed changes'

> Possible additional text:
> 
> If you really want to push a merge you can make it with:
> 
> $ git merge otherbranch
> 
> > Then please try, 'git push' again. Please see:
> >
> >  http://live.gnome.org/Git/Help/ExtraMergeCommits
> > ---
> >
> > If you look through the commit lists, we are getting a lot of these
> > stray merges, even from people that should know better :-)
> 
> They are not bad per se. Are they *all* introducing extra unwanted
> commits? 

As far as I saw every merge of this form was extraneous. There were no
merges of repositories other than git.gnome.org, there were no merges of
branches other than the branch the committer was working on. And there
weren't even any conflicts.

> Then they should better check with:
> 
> git push --dry-run
> 
> what would be pushed, maybe with the help of gitk.

It's often not a question of not understanding how things work, but
rather thinking that you *do* understand it... forgetting that you have
some local change. Asking people to exercise great care to not make a
mistake we can catch doesn't make sense to me.

> Mistakes happen, but they should no harm the workflow for everybody.

If people's workflow is harmed, I'm sure they will complain and we can
figure out better and more specific checks. But I want to get something
in place that will prevent accumulating a lot of these merges.

> Maybe it is OK to do it while the people learn the new system, but no
> definitely.

There are always going to be new users and always going to be
experienced users who forgot to rebase.

- Owen