On Thu, Feb 04, 2016 at 01:53:25PM -0800, Junio C Hamano wrote:
>[..]
> "by adding a new configuration variable" is a bit weak. Help the
> reader by mentioning what it is called and what it does in the same
> sentence.
>
> Perhaps like this?
>
> -- >8 --
>[..]
>
Looks good, I'll just take that :)
> ident: add user.useConfigOnly boolean for when ident shouldn't be guessed
>[..]
> > }
> > + if (strict && ident_use_config_only && !(ident_config_given & IDENT_MAIL_GIVEN))
> > + die("user.useConfigOnly set but no mail given");
> > }
>
> By folding the line just like you did for "name" above, you do not
> have to worry about an overlong line here.
Consistency is important. Will fix here too, though it got fixed later
in the cleanup.
>[..]
> > + git add foo &&
> > + EDITOR=: VISUAL=: git commit -m foo &&
>
> What is the point of these one-shot assignments to the environment
> variables?
>
> "git commit -m <msg>" does not invoke the editor unless given "-e",
> and EDITOR=: is done early in test-lib.sh already, so I am puzzled.
>
> Besides, if you are worried about some stray environment variable,
> overriding EDITOR and VISUAL would not guard you against a stray
> GIT_EDITOR, which takes the precedence, I think.
Being new to this testing framework, I tried learning the trade from
other tests. Maybe I goofed, or the other tests need cleaning?
>
> > + # Setup a likely user.useConfigOnly use case
> > + unset GIT_AUTHOR_NAME &&
> > + unset GIT_AUTHOR_EMAIL &&
>
> Doesn't unset fail when the variable is not set (we have sane_unset
> helper for that)?
Sure.
>[..]
> > +test_expect_success 'fails committing if clone email is not set, but EMAIL set' '
> > + prepare && about_to_commit &&
> > +
> > + EMAIL=test@xxxxxxxx EDITOR=: VISUAL=: test_must_fail git commit -m msg
>
> This is a good place to use the "test_must_fail env" pattern, i.e.
>
> test_must_fail env EMAIL=test@xxxxxxxx git commit -m msg
>
> I would think.
Yes, and the fixed test still passes. Will resubmit the patches.
--
Dan Aloni
--
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