From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sun, 10 Jun 2007 08:14:40 GMT
Subject: Re: [PATCH 06/21] Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'
Message-ID: <Pine.LNX.4.64.0706100909211.4059@racer.site>
In-Reply-To: <200706092334.32855.johan@herland.net>

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> On Saturday 09 June 2007, Alex Riesen wrote:
> > On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > > Signed-off-by: Johan Herland <johan@herland.net>
> > > ---
> > >  mktag.c |   29 ++++++++++++++++-------------
> > >  1 files changed, 16 insertions(+), 13 deletions(-)
> > 
> > What is this change good for?
> > How did you justify the type selection for your
> > loop index variable?
> > 
> > IOW,  the patch looks very useless.
> 
> I agree. By itself, the patch is useless.

Then it shouldn't be there.

It seems that you do not place the cuts between patches at the 
_conceptual_ layer. Therefore, they seem intrusive and often the meaning 
evades me.

So, if I understood the purpose of this patch series correctly, namely to 
use the same verification routines both for creation as for validation of 
tags, you could have

	- moved one function into the library (the stricter one), saying 
	  "move this_function() into libgit.a to make it usable from 
	   git-bla" in the commit body,

	- used that from the other program, removing the now-unused 
	  function,

	- and then changed the behaviour to be more chatty or some such.

As it is, you have a mix of conceptually different changes in almost every 
patch, and some changes that conceptually belong into the same patch, are 
not.

Be that as may, I think it is not a good change to reuse the same function 
like you did, exactly because one version _should_ be more forgiving than 
the other.

Ciao,
Dscho