Shourya Shukla <shouryashukla.oo@xxxxxxxxx> writes:
> From: Prathamesh Chavan <pc44800@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> Mentored-by: Stefan Beller <stefanbeller@xxxxxxxxx>
> Signed-off-by: Prathamesh Chavan <pc44800@xxxxxxxxx>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@xxxxxxxxx>
> ---
> 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.
