git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: git@vger.kernel.org, christian.couder@gmail.com,
	kaartic.sivaraam@gmail.com, Johannes.Schindelin@gmx.de,
	liu.denton@gmail.com, Prathamesh Chavan <pc44800@gmail.com>,
	Christian Couder <chriscool@tuxfamily.org>,
	Stefan Beller <stefanbeller@gmail.com>
Subject: Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C
Date: Wed, 07 Oct 2020 11:37:48 -0700	[thread overview]
Message-ID: <xmqqtuv52877.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20201007074538.25891-3-shouryashukla.oo@gmail.com> (Shourya Shukla's message of "Wed, 7 Oct 2020 13:15:37 +0530")

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

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

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

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

  reply	other threads:[~2020-10-07 18:37 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-07  7:45 [PATCH v2 0/3] submodule: port subcommand add " Shourya Shukla
2020-10-07  7:45 ` [PATCH v2 1/3] dir: change the scope of function 'directory_exists_in_index()' Shourya Shukla
2020-10-07 18:05   ` Junio C Hamano
2020-10-12 10:11     ` Shourya Shukla
2020-11-18 23:25   ` Emily Shaffer
2020-10-07  7:45 ` [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C Shourya Shukla
2020-10-07 18:37   ` Junio C Hamano [this message]
2020-10-07 22:19   ` Junio C Hamano
2020-10-08 17:19   ` Junio C Hamano
2020-10-09  5:09   ` Junio C Hamano
2020-11-18 23:13     ` Jonathan Tan
2020-11-19  7:44       ` Ævar Arnfjörð Bjarmason
2020-11-19 12:38         ` Johannes Schindelin
2020-11-19 20:37           ` Junio C Hamano
2020-10-07  7:45 ` [PATCH v2 3/3] t7400: add test to check 'submodule add' for tracked paths Shourya Shukla
2020-11-19  0:03 ` [PATCH v2 0/3] submodule: port subcommand add from shell to C Josh Steadmon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqtuv52877.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=liu.denton@gmail.com \
    --cc=pc44800@gmail.com \
    --cc=shouryashukla.oo@gmail.com \
    --cc=stefanbeller@gmail.com \
    --subject='Re: [PATCH v2 2/3] submodule: port submodule subcommand '\''add'\'' from shell to C' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).