Re: [PATCH v3 1/4] git-credential-store: support multiple credential files

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2015, 20:03 UTC
Message-ID: <xmqqy4myt4dt.fsf@gitster.dls.corp.google.com>
In-Reply-To: <vpqbnju7aex.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 20 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Paul Tan <pyokagan@gmail.com> writes:
>>
>>>> I think you could even get away without passing default_fn here, and
>>>> just use the rule "the first file in the list is the default". Unless
>>>> you are anticipating ever passing something else, but I couldn't think
>>>> of a case where that would be useful.
>>>
>>> Even though in this case the store_credential() function is not used
>>> anywhere else, from my personal API design experience I think that
>>> cementing the rule of "the first file in the list is the default" in
>>> the behavior of the function is not a good thing. For example, in the
>>> future, we may wish to keep the precedence ordering the same, but if
>>> none of the credential files exist, we create the XDG file by default
>>> instead.
>>
>> I am not sure if this is not a premature over-engineering
>
> I would say so if having this default_fn made the code more complex,

True, or if it made caller(s) be redundant or repeat themselves without a good reason. Otherwise we would end up having to drop the redundant and/or unnecessary arguments in future clean-up patches; I had to de-conflict a series with 7ce7c760 (convert: drop arguments other than 'path' from would_convert_to_git(), 2014-08-21) recently, which reminded me of this point ;-).

Show 14 quoted lines

> but
> here the code is basically
>
> +	if (default_fn)
> +		store_credential_file(default_fn, c);
>
> and
>
> -		store_credential(file, &c);
> +		store_credential(&fns, &c, fns.items[0].string);
>
> Taking the first element in the list wouldn't change much.
>
> I'm personally fine with both versions.

Turning the current code to drop the extra parameter and to use the first element instead wouldn't be a big change, but these things tend to add up, so unless this discussion immediately lead to a future enhancement plan to make use of the flexibility the parameter gives us, I'd rather see things kept simpler.

I do not terribly mind either way, either, though.