Re: [PATCH] Allow tracking branches to set up rebase by default.
On May 10, 2008, at 11:41, Junio C Hamano wrote:
Show 8 quoted lines
> - 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.
Show 6 quoted lines
> 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
Dustin Sallings <dustin@spy.net>