Re: [PATCH v2] git svn : hook before 'git svn dcommit'

From: Frédéric Heitzmann <frederic.heitzmann@gmail.com>
Date: Aug 17, 2011, 14:35 UTC
Message-ID: <CALeToSUidWgiJ+PxuphnBZOQGNStNOO9==EswfLDpFr2GYy=nA@mail.gmail.com>
In-Reply-To: <CALeToSWJNK=q4iPwxNvgGin0T61oLKJd=b9F3cSSo0vVebrhhQ@mail.gmail.com>

Hi all.

Maybe I should give some more context to explain why a hook could be a potential improvement.

Let's consider the following workflow :
1) git svn clone from the SVN server, then git checkout -b topic
2) git commit some "reference data", before starting some optimization
or code refactoring.
** These reference data are not supposed to find their way to the SVN server **
Committing such "reference data" is just a convenience because git
does a great job to show how these data may or may not change during
the development process.
3) hack, test, commit ...
3 bis) it may happen that reference data change for some very good
reason (for instance some protocol change)
New reference data are then commited.

back to 3 ...

4) Before merging back to master and commitng to SVN, it is necessary
to remove commits with reference data (git rebase -i --onto master
master topic ...)
5) merge topic branch with master and git svn dcommit

-- end --

It is very easy to forget step 4, and svn commit lots of useless data.

Proposal 1)
* commit reference data with some specific mark in the commit message
(e.g. "NO_SVN")
* use pre-svn-dcommit hook to detect such commits

Proposal 2) (not fully feasable for what I know)
* git svn clone to a bare repo
* clone a working repo from the the bare repo.
* steps 2, 3, maybe 3bis, ... then 4
* push commits to the bare repo, while using pre-receive or update
hook to look for wrong commits, and abort if so.
* use post-receive hook to trigger git svn dcommit

Main drawback for proposal 2 (appart from needing 2 repo instead of one) is that each time you want to update your working repo, you have to git svn rebase the bare repo, then git pull.

Proposal 2bis)
* add a pre-send hook on the bare repo, and trigger some git svn
rebase with this hook.
I am not sure to see all the potential consequences of such a hook though.

All things begin equal, proposal 1 seems to be the easier path, but it is highly debatable.

-- Fred

ps : I had to resend this email because first attempt included HTML (sic). Very sorry if you receive it twice.

2011/8/17 Eric Wong <normalperson@yhbt.net>

Show 53 quoted lines

>
> Junio C Hamano <gitster@pobox.com> wrote:
> > Frédéric Heitzmann  <frederic.heitzmann@gmail.com> writes:
> >
> > > The 'pre-svn-dcommit' hook is called before 'git svn dcommit', which aborts
> > > if return value is not zero. The only parameter given to the hook is the
> > > reference given to 'git svn dcommit'. If no paramter was used, hook gets HEAD
> > > as its only parameter.
> >
> > It appears that this is in the same spirit as the pre-commit hook used in
> > "git commit", so it may not hurt but I do not know if having a separate
> > hook is the optimal approach to achieve what it wants to do.
> >
> > I notice that git-svn users have been happily using the subsystem without
> > need for any hook (not just pre-commit). Does "git svn" need an equivalent
> > of pre-commit hook? If so, does it need equivalents to other hooks as
> > well? I am not suggesting you to add support for a boatload of other hooks
> > in this patch---I am trying to see if this is really a necessary change to
> > begin with.
> >
> > Eric, do you want this one?
>
> I'm not sure.  I feel hooks should be avoided whenever possible, and
> a git-svn-specific hook for dcommit wouldn't place the same restriction
> as a server-side SVN hook for svn(1) users.
>
> Preventing certain commits from accidentally hitting the SVN server can
> be useful, I think.  On the other hand, I'm not sure if people who run
> accidental dcommits would remember to the pre-dcommit hook, either.
>
> Perhaps an interactive option for dcommit would be just as useful?
>
> Test cases are required for any new features of git-svn, though.
>
> > > +   system($hook, $head);
> > > +   if ($? == -1) {
> > > +           print "[pre_svn_dcommit_hook] failed to execute $hook: $!\n";
> > > +           return 1;
> > > +   } elsif ($? & 127) {
> > > +           printf "[pre_svn_dcommit_hook] child died with signal %d, %s coredump\n",
> > > +           ($? & 127),  ($? & 128) ? 'with' : 'without';
> > > +           return 1;
> > > +   } else {
> > > +           return $? >> 8;
> > > +   }
> > > +}
> >
> > Should these messages go to the standard output?
>
> Failure messages should definitely go to stderr.
>
> --
> Eric Wong