From: Jan Nieuwenhuizen <janneke-list@xs4all.nl>
Date: Mon, 04 Aug 2008 08:07:57 GMT
Subject: Re: [PATCH] git-svn.perl: Strip ChangeLog bits.
Message-ID: <1217837277.7649.24.camel@heerbeest>
In-Reply-To: <7vhca1u8to.fsf@gitster.siamese.dyndns.org>

On zo, 2008-08-03 at 13:45 -0700, Junio C Hamano wrote:

> Nice try, but after -rc1 we won't take feature enhancements on the
> 'master' branch.  The earliest this will appear is in 1.6.1.

Ok, I'm not that familiar with git development and I did not find any
newer/UNRELEASED list of features?

>  * Documentation; introduce this with heading --clean-changelog=<style>; I

Ok.  I tried ={gnu} first, which seems to be the style for multiple
choice arguments, but the document parser does not grok that.  {gnu|foo}
or {gnu|no-other-yet} did not really please me.

>    You seem to have taken the "arbitrary Perl snippet" part of my patch as
>    well, but it is not described here...

It all depends upon how you read the future.  I would most have chosen
to postpone that work until the second (or third) request for different
munging came in, but now that the code is already written...

>  * Script; two separate _clean_changelog and _clean_log_message variables
>    are not necessary (I removed the extra variable in the patch below).

Good, I didn't really look at that.

>    Your new tests do not seem to check these, but I think you should:

>    - what should happen without --clean-changelog=gnu?  (iow, additional
>      code does not regress the behaviour when this shiny new toy is not
>      used).

We could add a test to make sure that git-svn does not alter commit
messages, but it has little to do with this patch.

If this is not being tested atm, it is probably not deemed important
enough to test.  This could have regressed at any time.  

I would add a test for existing working code only if experience tells
you it is fragile and it (often) regresses, ie, when you fix a bug:
new/revised code.

>    - what should happen when an unknown style is given e.g. --clean-changelog=yak?

It would be nice if the script failed with an error message, telling
what the options are, but I do not really care that much about wrong
use.  You have that automatically if you use a sensible option parser,
this is where such a feature should be implemented, imho.

>    We prefer to use "test_cmp" for comparing expected and actual result,
>    not bare "cmp".

Ok.

> Here is what I tested and based the above comments on after minor fixes to
> ask comments from Eric.

Great, thanks.  We'll see what happens then.

Jan.

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond - The music typesetter
http://www.xs4all.nl/~jantien       | http://www.lilypond.org