From: Dustin Sallings <dustin@spy.net>
Date: Sat, 10 May 2008 22:36:13 GMT
Subject: Re: [PATCH] Allow tracking branches to set up rebase by default.
Message-ID: <1206EB02-2AE6-4A97-AF00-D16B8E686D9B@spy.net>
In-Reply-To: <7vfxsq9f3n.fsf@gitster.siamese.dyndns.org>


On May 10, 2008, at 11:41, Junio C Hamano wrote:

> - die_bad_config() takes a variable name without surrounding  
> explanatory
>   text.  If you actually tested your patch and looked at the error
>   message, it would have been blatantly obvious and you would have
>   noticed it.  Not a good sign.
>
> - Test not just the success cases but failure cases; test not just
>   explicitly configured cases but also default cases.

	You're right, I didn't test misconfiguration cases.  I've updated the  
patch per your recommendations with a lot more tests and have manually  
tested the error messages to ensure they make sense.

	Does it make sense in the cases where a value is bad to list the  
valid values?  I don't see a precedent (so I don't know what format  
would be most desirable), but it might be friendly.

> Also I suspect that we would want to test cases where
> autosetuprebase is given but autosetupmerge is not.

	This code isn't so much dependent on that variable, but on the  
effects of that variable, which may be overridden by the --track and -- 
no-track parameters.  I think it'd be best to leave the effects of  
that variable in its own tests wrt tracking since this code doesn't  
kick in until after the tracking is set up.

> Another thing we might want to address is to move parsing of branch.*
> configuration variables out of git_default_config().  They are  
> unnecessary
> for majority of commands that do not create new branches.  But that  
> would
> be a separate topic if we were to do so.

	I agree with this.  I tried initially to not have it be part of the  
default config parser, but it seemed a lot more disruptive, so I just  
followed autosetupmerge.

	Thanks for your input again.  New patch to follow.

-- 
Dustin Sallings