Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 7, 2020, 18:37 UTC
Message-ID: <xmqqtuv52877.fsf@gitster.c.googlers.com>
In-Reply-To: <20201007074538.25891-3-shouryashukla.oo@gmail.com>

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

Show 22 quoted lines

> From: Prathamesh Chavan <pc44800@gmail.com>
>
> Convert submodule subcommand 'add' to a builtin and call it via
> 'git-submodule.sh'.
>
> Also, since the command die()s out in case of absence of commits in the
> submodule, the keyword 'fatal' is prefixed in the error messages.
> Therefore, prepend the keyword in the expected output of test t7400.6.
>
> While at it, eliminate the extra preprocessor directive
> `#include "dir.h"` at the start of 'submodule--helper.c'.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Stefan Beller <stefanbeller@gmail.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
>  builtin/submodule--helper.c | 391 +++++++++++++++++++++++++++++++++++-
>  git-submodule.sh            | 161 +--------------
>  t/t7400-submodule-basic.sh  |   2 +-
>  3 files changed, 392 insertions(+), 162 deletions(-)

Whoa. That looks like a huge change. Makes me wonder if we want this split into multiple pieces, but let's read on.

Show 31 quoted lines

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index de5ad73bb8..ec0a50d032 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -19,7 +19,6 @@
>  #include "diffcore.h"
>  #include "diff.h"
>  #include "object-store.h"
> -#include "dir.h"
>  #include "advice.h"
>  
>  #define OPT_QUIET (1 << 0)
> @@ -2744,6 +2743,395 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
>  	return !!ret;
>  }
>  
> +struct add_data {
> +	const char *prefix;
> +	const char *branch;
> +	const char *reference_path;
> +	const char *sm_path;
> +	const char *sm_name;
> +	const char *repo;
> +	const char *realrepo;
> +	int depth;
> +	unsigned int force: 1;
> +	unsigned int quiet: 1;
> +	unsigned int progress: 1;
> +	unsigned int dissociate: 1;
> +};
> +#define ADD_DATA_INIT { NULL, NULL, NULL, NULL, NULL, NULL, NULL, 0, 0, 0, 0, 0 }

This is used in a context like this:

	struct add_data data = ADD_DATA_INIT;

It is a tangent, but wouldn't

	#define ADD_DATA_INIT { 0 }

be a more appropriate way to express that there is nothing other than the initialization to zero values going on?

> +/*
> + * Guess dir name from repository: strip leading '.*[/:]',
> + * strip trailing '[:/]*.git'.
> + */

The original also strips trailing '/'. The original does these in order:

 - if $repo ends with '/', remove that.  The above description does
   not mention it.

 - if the result of the above ends with zero or more ':', followed
   by zero or more '/', followed by ".git", drop the matching part.
   The above description sounds as if it will remove ":/:/:.git"
   from the end (and the code seems to have the same bug, as
   after_slash_or_colon won't allow the code to know if the previous
   character before ".git" was slash or colon).

 - if the result of the above has '/' or ':' in it, remove everything
   before it and '/' or ':' itself.

Show 27 quoted lines

> +static char *guess_dir_name(const char *repo)
> +{
> +	const char *p, *start, *end, *limit;
> +	int after_slash_or_colon;
> +
> +	after_slash_or_colon = 0;
> +	limit = repo + strlen(repo);
> +	start = repo;
> +	end = limit;
> +	for (p = repo; p < limit; p++) {
> +		if (starts_with(p, ".git")) {
> +			/* strip trailing '[:/]*.git' */
> +			if (!after_slash_or_colon)
> +				end = p;
> +			p += 3;
> +		} else if (*p == '/' || *p == ':') {
> +			/* strip leading '.*[/:]' */
> +			if (end == limit)
> +				end = p;
> +			after_slash_or_colon = 1;
> +		} else if (after_slash_or_colon) {
> +			start = p;
> +			end = limit;
> +			after_slash_or_colon = 0;
> +		}
> +	}
> +	return xstrndup(start, end - start);

So, this looks quite bogus and unnatural. Checking for ".git" at every position in the string is meaningless.

I wonder if something along the following (beware: not even compile tested or checked for off-by-ones) would be easier to follow and more faithful conversion to the original.

	ep = repo + strlen(repo);

        /*
         * eat trailing slashes - a conversion less faithful to
         * the original may want to loop to cull duplicated trailing
	 * slashes, but we can leave it as user-error for now.
	 */
	if (repo < ep - 1 && ep[-1] == '/')
		ep--;

	/* eat ":*/*\.git" at the tail */
	if (repo < ep - 4 && !memcmp(".git", ep - 4, 4)) {
		ep -= 4;
		while (repo < ep - 1 && ep[-1] == '/')
			ep--;
		while (repo < ep - 1 && ep[-1] == ':')
			ep--;
	}

	/* find the last ':' or '/' */
	for (sp = ep - 1; repo <= sp; sp--) {
		if (*sp == '/' || *sp == ':')
			break;
	}
        sp++; /* exclude '/' or ':' itself */

        /* sp point at the beginning, and ep points at the end */
	return xmemdupz(sp, ep - sp);

> +}

That's it for now; I didn't look at the remainder of this patch during this sitting before I have to move on, but I may revisit the rest at some other time.

Thanks.