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

[Eric Wong <normalperson@yhbt.net>]

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