Re: [PATCH] Allow tracking branches to set up rebase by default.

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