Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C

From: Junio C Hamano <gitster@pobox.com>
Date: May 24, 2020, 23:15 UTC
Message-ID: <xmqqtv05orgq.fsf@gitster.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.2005230012090.56@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 18 quoted lines

>> +	config_set_in_gitmodules_file_gently(config_name, opt_branch);
>
> What happens if this fails? E.g. when the permission is denied or disk is
> full? This C code would then still `return 0`, pretending that it
> succeeded. But the original shell script calls `git submodule--helper
> config [...]` which calls `module_config()`, which in turn passes through
> the return value of the `config_set_in_gitmodules_file_gently()` call.
>
> In other words, you need something like this:
>
> 	int ret;
>
> 	[...]
>
> 	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
>
> 	free(config_name);
> 	return ret;

Making sure we check the return value of helper functions we call is a good discipline, but this is not quite enough.

Show 5 quoted lines

> So I think we'll also need this (it's unrelated to your patch, at least
> unrelated enough that it merits its own, separate patch):
>
> -                       return commands[i].fn(argc - 1, argv + 1, prefix);
> +                       return !!commands[i].fn(argc - 1, argv + 1, prefix);

I checked (not all but most of the) functions in that commands[] table and they all seem to return 0 for success and positive non-zero for failure.

config_set_in_gitmodules_file_gently() takes the return value of a helper function in its 'ret', gives an warning if it is negative, and returns that 'ret' literally to the caller. You suggestion allows module_set_branch() return a negative value as-is. You'd need to return !!ret from there.

The "unrelated" change becomes only necessary if you do not do the !!ret in module_set_branch(); otherwise it is unneeded, I think.

Thanks.