Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: >> + 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. > 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.
