On Wed, Feb 3, 2016 at 3:21 AM, Dan Aloni <alonid@xxxxxxxxx> wrote:
> On Tue, Feb 02, 2016 at 10:56:48PM -0500, Jeff King wrote:
>> On Tue, Feb 02, 2016 at 09:54:21PM +0200, Dan Aloni wrote:
>> > + if (strict && email && !strcmp(email, "(per-repo)")) {
>> > + die("email is '(per-repo)', suggesting to set specific email "
>> > + "for the current repo");
>> > + }
>>
>> I find it disappointing that we go back to looking for magic sequences
>> in the string. Could we perhaps do this more cleanly with a new config
>> option? Like a "user.guessIdent" which defaults to true, but people can
>> set to false. And without that, we do not do any automagic at all; we
>> get the values from the GIT_COMMITTER_* environment or the
>> user.{name,email} config variables, or we die().
>
> Agreed. New patch attached, feel free to amend.
Please re-send with the patch inline since reviewers will want to
comment on on specific bits of code inline as well. When the patch is
an attachment, this process becomes too onerous for reviewers, and
most will simply ignore the patch. Thanks.
Before sending v3 (inline), perhaps take note of the few issues below
which I noticed while quickly scanning the attachment:
* The final paragraph of the commit message appears to be outdated
since it still seems to be describing the approach taken by v1.
* Elsewhere, in the project, the spelling "email" is preferred over
"E-Mail"; that's true even in the files your patch is touching.
* In the documentation:s/mutiply/multiple/.
* The documentation doesn't seem to mention the default value of the
new config variable.
* The new config variable "user.explicit" has a more nebulous name
than Peff's suggestion of "user.guessIdent", which may make its intent
harder to determine without consulting documentation.
* Don't initialize static variables to 0 (let the .bss section do that
automatically).
* One space before && operator; not two.
* Drop unnecessary braces.
* Perhaps use test_config(), test_unconfig(), test_config_global() in
the test script rather than the manual git-config invocations in
subshells.
* test_expect_failure() is for indicating that a test will fail
because some feature is known to be broken, not for when you expect a
git command to fail in a controlled fashion. Instead, use
test_expect_success, and then use test_must_fail() within the body of
the test.
test_expect_success '...' '
... &&
test_must_fail git commit -m msg
'
* Do these new tests really deserve a new test script, or would they
fit into an existing script? (Genuine question.)
It's also reviewer-friendly to indicate the patch revision in the
subject "[PATCH v3]", and to describe what changed since the previous
version of the patch. Providing a gmane link to the previous version
is also very helpful.
--
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