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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
