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

From: Jeff King <peff@peff.net>
Date: Mar 1, 2016, 01:36 UTC
Message-ID: <20160301013616.GA18301@sigill.intra.peff.net>
In-Reply-To: <CA+P7+xrr61wO0XrhXCEbSLPbSo7HxxzDWnq=6K14fvyo7RfscA@mail.gmail.com>

On Mon, Feb 29, 2016 at 03:44:51PM -0800, Jacob Keller wrote:

Show 15 quoted lines

> On Mon, Feb 29, 2016 at 3:39 PM, Stefan Beller <sbeller@google.com> wrote:
> > 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.
> >
> 
> I am not sure, but I don't think it particularly matters what we use.
> Most of this is pretty much copied as suggested by Peff.

Yes, this dates back to 3cf8fe1 (t5550: test HTTP authentication and userinfo decoding, 2010-11-14), and the point is just to have an "@" in each field. And then because that's the only username defined in the apache setup, it's the one all the tests use. :)

I also have found it off-putting, because it looks like "host" is supposed to be meaningful. Perhaps the original author had a case where their web authentication involved a hostname (which seems plausible for a hosting site which covers multiple domains).

It might be nice to fix that, either by using something like "us@r" and "p@ssword", or possibly by just introducing a new, easier to read alternative to lib-httpd/passwd and using it, as most sites are not testing URL parsing (though I suppose that using the @-filled ones consistently means we _do_ test the other code paths more thoroughly).

But either way, I don't think it should be part of this series, which has already expanded well beyond its original 1-patch goal.

Show 7 quoted lines

> > Why set set_askpass a second time here?
> 
> Interesingly, it fails unless I add this line with:
> [...]
> I really don't understand why adding the extra askpass setting fixes
> this? Possibly because the query and expect files are appended to? Or
> something else subtle going on?

Right. The askpass test-helper logs the queries it gets, and expect_askpass makes sure that we got the right set of queries. It has to append to the query-log because it may be invoked multiple times (e.g., once for the username, once for the password). So we have to clear the query log, and that is one of the things that set_askpass does (the other, obviously, is writing the user/pass values for askpass to feed back to git). Perhaps it should have been called init_askpass or something, but I don't think it's worth changing now.

You could get away with just:

  >"$TRASH_DIRECTORY/askpass-query"

but IMHO it is less obscure to just call set_askpass a second time.

-Peff