Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 3, 2016, 17:47 UTC
Message-ID: <CAPig+cSWN-wpcooqmYtFfZoDYpkhLoezSeu6bm9rSTvZ72jSEQ@mail.gmail.com>
In-Reply-To: <20160203082112.GA27454@gmail.com>

On Wed, Feb 3, 2016 at 3:21 AM, Dan Aloni <alonid@gmail.com> wrote:

Show 15 quoted lines

> 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.