Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 1, 2016, 17:26 UTC
Message-ID: <xmqqh9gq85yc.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kbDaV=i0augzh5RgGYpTWXOuTLx=7Occhc-6iE+0pBVNg@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 23 quoted lines

> On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>>
>> +test_expect_success 'cmdline credential config passes into submodules' '
>> +       git init super &&
>> +       set_askpass user@host pass@host &&
>
> I wonder why we use pass@host as a password, it seems confusing to me.
> p@ssword would have worked if we wanted to test a password containing an @,
> pass@host doesn't quite fit my mental model of how passwords work.
> No need to change anything, better be consistent with the rest of the tests.
>
>
>> +       (
>> +               cd super &&
>> +               git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
>> +               git commit -m "add submodule"
>> +       ) &&
>> +       set_askpass wrong pass@host &&
>> +       test_must_fail git clone --recursive super super-clone &&
>> +       rm -rf super-clone &&
>> +       set_askpass wrong pass@host &&
>
> Why set set_askpass a second time here?

I find this in t/lib-httpd.sh:

        set_askpass() {
                >"$TRASH_DIRECTORY/askpass-query" &&
                echo "$1" >"$TRASH_DIRECTORY/askpass-user" &&
                echo "$2" >"$TRASH_DIRECTORY/askpass-pass"
        }

and expect_askpass peeks at askpass-query to see if Git asked the right questions.

I think askpass-query is cleared here because the author of the test suite expected that the helpers are used in such a way that you always (1) set-askpass once, (2) run a Git command that asks credentials, (3) use expect_askpass to validate and do these three steps as a logical unit?

That "clone" the test expects to fail does ask the credential, so even though the test does not check if the "clone" asked the right question, it finishes the three-step logical unit, and then you need to clear askpass-query.

It may have been cleaner if you had clear_askpass_query helper that is called (1) at the beginning of set_askpass instead of this manual clearing, (2) at the end of expect_askpass, as the exchange has been tested already at that point, and (3) in place of expect_askpass if you choose not to call it (e.g. this place, instead of the second set_askpass, you would say clear_askpass_query), perhaps, but I do know if that is worth it.