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

Atharva Raykar [off-list ref] writes:

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.

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.

+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.

On 08-Jul-2021, at 01:27, Junio C Hamano [off-list ref] wrote:

Atharva Raykar [off-list ref] writes:

quoted

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.

quoted

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.

Okay, will do. The latter will not affect the tests, but the inclusion
of a 'fatal:' prefix will require me to adjust one test that checks for
the error message.