Re: [PATCH v2 3/3] credential: handle `credential.<partial-URL>.<key>` again

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3098 bytes --]

Hi Carlo,

On Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:

> On Fri, Apr 24, 2020 at 02:16:45AM +0200, Johannes Schindelin wrote:
> > On Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:
> > > On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > > diff --git a/credential.c b/credential.c
> > > > index 52965a5122c..3505f6356d8 100644
> > > > --- a/credential.c
> > > > +++ b/credential.c
> > > > @@ -53,7 +53,13 @@ static int credential_config_callback(const char *var, const char *value,
> > > >  		char *url = xmemdupz(key, dot - key);
> > > >  		int matched;
> > > >
> > > > -		credential_from_url(&want, url);
> > > > +		if (credential_from_url_gently(&want, url, 1, 0) < 0) {
> > >
> > > definitely not worth a reroll, but just wondering if would make sense to call
> > > credential_from_url_gently(!quiet) here, just for consistency?
> >
> > We don't have any `quiet` variable here.
>
> my bad, should had been more explicit as it is confusing with all those
> booleans at the end without a flags parameter.
>
> I mean the call should be instead :
>
>   if (credential_from_url_gently(&want, url, 1, 1) < 0) {
>
> since we want to warn if the configuration is not supported just like is
> done after this check.

Since Junio expressed support for Jonathan's idea of using separate
functions that wrap one helper function, I went with that instead.

> > > other than that this series is looking great, under the assumption that there
> > > is going to be some more followup with non essential changes.
> >
> > I am sure I don't follow. What non-essential changes are you assuming will
> > follow up?
>
> the ones that were discussed with Jonathan in a differen thread :
>
> * using a flags parameter instead of two ints
> * whatever will be needed so it also works in master and maint

Oy, I am still under water just trying to get the MinGit for Windows
backports updated so that users can upgrade instead of complaining on bug
trackers and on Twitter...

But yes, that will be the next step. Now that I have a comprehensive test
case, it should be relatively easy.

> > > will chip in with an test helper for that series so we can hopefully keep our
> > > sanity next time someone touches that function again.
> >
> > Are the tests I added to t0300 not enough? Or do you think it will need a
> > native test helper that is included in `t/helper/test-tool` and exercised
> > in the test suite somehow?
>
> I think they are enough, it is only that is easier to quickly iterate with
> possible bad input with a helper. which is what I was offering for the next
> thread since its need is orthogonal to what you are trying to accomplish.

Usually I would agree with you. It's quicker to iterate, and a ton faster
to run (especially on Windows).

In this instance, I am going for a regression test case rather than a unit
test, though, because I really need to ensure that what end users are
trying to do on their machines will work (and continue to work).

Ciao,
Dscho