Re: [Outreachy PATCH] credential-store: add lockTimeout config option

[Junio C Hamano <gitster@pobox.com>]

Okhuomon Ajayi <okhuomonajayi54@gmail.com> writes:

> diff --git a/builtin/credential-store.c b/builtin/credential-store.c
> index bc1453c6b2..9fef54d218 100644
> --- a/builtin/credential-store.c
> +++ b/builtin/credential-store.c
> @@ -67,7 +67,9 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
>  {
>  	int timeout_ms = 1000;
>  
> -	repo_config_get_int(the_repository, "credentialstore.locktimeoutms", &timeout_ms);
> +	/* lockTimeoutMS is a deprecated synonym of lockTimeout. */
> +	if (repo_config_get_int(the_repository, "credentialstore.locktimeout", &timeout_ms))
> +		repo_config_get_int(the_repository, "credentialstore.locktimeoutms", &timeout_ms);
>  	if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)
>  		die_errno(_("unable to get credential storage lock in %d ms"), timeout_ms);

Around this point the file is already full of overly long lines, but
the patch is making it even worse.

> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index c1cd60edd0..408f26bb10 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -210,4 +210,60 @@ test_expect_success 'get: store file can contain empty/bogus lines' '
>  	EOF
>  '
>  
> +# Holding creds.lock makes the next store fail. The error reports the
> +# timeout that was actually used, so "in 0 ms" means that name won.
> +test_expect_success 'credentialStore.lockTimeoutMS=0 gives up immediately' '
> +	test_when_finished "rm -f creds creds.lock" &&
> +	>"creds.lock" &&
> +	test_must_fail git -c credentialStore.lockTimeoutMS=0 \
> +		credential-store --file=creds store 2>err <<-\EOF &&
> +	protocol=https
> +	host=example.com
> +	username=store-user
> +	password=store-pass
> +	EOF
> +	test_grep "unable to get credential storage lock in 0 ms" err
> +'
> +
> +test_expect_success 'credentialStore.lockTimeout=0 gives up immediately' '
> +	test_when_finished "rm -f creds creds.lock" &&
> +	>"creds.lock" &&
> +	test_must_fail git -c credentialStore.lockTimeout=0 \
> +		credential-store --file=creds store 2>err <<-\EOF &&
> +	protocol=https
> +	host=example.com
> +	username=store-user
> +	password=store-pass
> +	EOF
> +	test_grep "unable to get credential storage lock in 0 ms" err
> +'

As it is necessary to keep the above two in sync forever, it may
make sense to do this instead:

	for v lockTimeout lockTimeoutMS
	do
		test_expect_success "credentialStore.$v=0 gives up immediately" '
			...
			test_must_fail git -c credentialStore.$v=0 \
			credential-store --file=creds store 2>err <<-\EOF &&
			...
			test_grep "unable to get ..." err
		'
	done

It is a common mistake to use double-quote in an attempt to have $v
expanded before it is passed to text_expect_success function, but
the variable $v is dereferenced correctly inside the body of the
test that is quoted with single-quote as it is eval'ed, so the way
the above is written is right.

> +test_expect_success 'credentialStore.lockTimeout wins when both are set' '
> +	test_when_finished "rm -f creds creds.lock" &&
> +	>"creds.lock" &&
> +	test_must_fail git \
> +		-c credentialStore.lockTimeout=0 \
> +		-c credentialStore.lockTimeoutMS=500 \
> +		credential-store --file=creds store 2>err <<-\EOF &&
> +	protocol=https
> +	host=example.com
> +	username=store-user
> +	password=store-pass
> +	EOF
> +	test_grep "unable to get credential storage lock in 0 ms" err
> +'

This is understandable.  The presense of locktimeout=0 hides
whatever locktimeoutms is set to, so it should fail the same way as
the earlier two tests.  Wait, nobody unlocks creds.lock, so what
should happen if the line that sets locktimeout=0 is removed?

Ahh, the key to this test is that we make sure "in 0 ms" part of the
error message does not say "in 500 ms" to ensure which of the two
variables are being used.  It is not like the credential-store command
would fail with 0ms timeout but succees with 500ms timeout.

> +test_expect_success 'credentialStore.lockTimeout allows a store' '
> +	test_when_finished "rm -f creds" &&
> +	git -c credentialStore.lockTimeout=1000 \
> +		credential-store --file=creds store <<-\EOF &&
> +	protocol=https
> +	host=example.com
> +	username=store-user
> +	password=store-pass
> +	EOF
> +	echo "https://store-user:store-pass@example.com" >expect &&
> +	test_cmp expect creds
> +'

This one I I am not sure what it is trying to test, though.  Would
the test start failing as the machine is loaded more and 1000 ms
becomes insufficient?  No.  The thing is not locked, nobody takes a
lock competing with our test, so with any value of the timeout we
should succeed, no?  Or would it fail if the timeout is set to 0ms?

Assuming that there is a reason to run this test, should it also be
testing locktimeoutms to ensure both variables behave the same way
when set independently, similarly to the two tests I rewrote into a
for loop?

Thanks.