Re: [PATCH v4] remote: create fetch.credentialsInUrl config

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

The initializer should be "= { 0 }" not "= { NULL }". In other words, we shouldn't have to care if the first member in the struct happens to be of a pointer type, and we shouldn't have to change between 0 and NULL whenever the type of the first member changes.

Even though it frowns upon assigning 0 to a pointer variable or a pointer member in a struct, sparse knows that such an initializer is OK.

cf. https://lore.kernel.org/git/YVJSwuqjolz28+mG@coredump.intra.peff.net/

Please have a blank line after the variable decl. 

url_normalize() returns "char *", and you get NULL when parsing fails; out_info->err may also help when it happens, but I think we will call url_normalize() again later in the existing caller, and it will give whatever error message we need to give appropriately, so it is OK to silently jump to the loop_cleanup label from here.

I wonder what should happen to "https://username:@localhost" (i.e. an empty string is used as a password). I am fine if we allow it as an obvious "this is like an anonymous ftp; anybody can connect" use case, but I do not know how useful it would be in practice.

I am also fine if certain authentication scheme needs username and password, the latter of which is never used because the "real thing" like Kerberos kicks in when the real authentication happens but becasue something needs to be there to "trigger" the authentication, and that is why we deliberately allow an empty string case unwarned. But if this is a deliberate thing, we'd need to caution future developers about it.

I do not think passwd_ofs can ever be 0 if we have an embedded password in the URL, so checking it may be a better approach, if we care about an empty-string case. I can be persuaded either way.

We obviously could introduce another local variable that is set based on the "value" before we enter the loop to "optimize", but this is an error codepath, so I do not mind repeated strcmp() on the constant value in the loop.

I do have to wonder what we should do when value is none of the three we know about. Right now, it makes the function an expensive noop, so upfront at the beginning of the function, we might need something like

	int to_warn_not_die;

	if (git_config_get_string_tmp(..."))
 		return;
	if (!strcmp("warn", value))
		to_warn_not_die = 1;
	else if (!strcmp("die", value))
		to_warn_not_die = 0;
	else
		return;

anyway, in which case we would also do

	if (to_warn_not_die)
		warning(...);
	else
		die(...);

in the loop, perhaps?  I dunno.

I wonder if die_message() may want to report all the offending URL for a given remote (with a concluding die() after the loop). I am OK with dying at the first offence, though. In the worst case, the end user experience would be:

    $ git fetch there
    die() about the first URL for the nickname
    $ edit .git/config
    $ git fetch there
    die() about the second URL for the nickname

The user will learn after getting the same error message twice and scan through the other URLs when editing .git/config for the second time to fix the second URL. If I were writing this patch, I would probably play lazy and die on the first offender.

Reusing warnings file for die messages is probably OK ;-)

An extra test with an empty string as a password would have caught the differences between using passwd_off and passwd_len to detect the presence of a password here.

Taking all together, I'll queue the following on top as a separate fix-up patch, but I may well be giving (some) bad pieces of advice, so I will wait for others to comment.

Thanks.

 remote.c              | 26 ++++++++++++++++++--------
 t/t5516-fetch-push.sh |  4 ++++
 t/t5601-clone.sh      |  4 ++++
 3 files changed, 26 insertions(+), 8 deletions(-)