Re: [GSoC] [PATCH 3/3] submodule--helper: introduce add-clone subcommand

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2021, 19:57 UTC
Message-ID: <xmqqr1g9ew2f.fsf@gitster.g>
In-Reply-To: <20210706181936.34087-4-raykar.ath@gmail.com>

Atharva Raykar <raykar.ath@gmail.com> writes:

Show 8 quoted lines

> Let's add a new "add-clone" subcommand to `git submodule--helper` with
> the goal of converting part of the shell code in git-submodule.sh
> related to `git submodule add` into C code. This new subcommand clones
> the repository that is to be added, and checks out to the appropriate
> branch.
>
> This is meant to be a faithful conversion that leaves the behaviour of
> 'submodule add' unchanged.

Makes sense.

Show 6 quoted lines

> The 'die' that is used in git-submodule.sh is not the same as the
> 'die()' in C--the latter prefixes with 'fatal:' and exits with an error
> code of 128, while the shell die exits with code 1.
>
> Introduce a custom die routine, that can be used by converted
> subcommands to emulate the shell 'die'.

I suspect that installing this with set_die_routine() might be going too far. If some of the lower-level helper routines we call from here have to die (e.g. our call results in xmalloc() getting called and we run out of memory), die() called there will also end up calling our submodule_die(), not just new calls to die() you are adding in this patch. Calling submodule_die() directly from the code you convert from the scripted version where we used to call die of the scripted version would be fine, though.

I suspect that it would be OK to use the standard die() instead, with the minimum adjustment as needed, namely, we may have to

 * Adjust the messages the scripted version of the caller gave to
   the scripted version of die, if needed (e.g. if the scripted
   version added "fatal:" prefix itself to compensate for the lack
   of it in the scripted "die", we can drop the prefix and call the
   standard die());

 * Adjust the tests if they care about the differences between
   exiting 128 and 1.

Show 7 quoted lines

> +static NORETURN void submodule_die(const char *err, va_list params)
> +{
> +	vfprintf(stderr, err, params);
> +	fputc('\n', stderr);
> +	fflush(stderr);
> +	exit(1);
> +}

Other than that, all three patches looked quite reasonable.

Thanks.