Re: [PATCHv5 0/8] git clone: Marry --recursive and --reference

[Junio C Hamano]

Stefan Beller  writes:

> As the comments only address programming work as opposed to design, the 
> interdiff is rather small:

Thanks.  Will queue.  I think we do not need that extra err strbuf
in add_one_reference(), but we can go either way and it is not worth
rerolling.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv5 0/8] git clone: Marry --recursive and --reference

[Stefan Beller]

v5:

Thanks Junio, Ramsay for comments.

As the comments only address programming work as opposed to design, the 
interdiff is rather small:

diff --git a/builtin/clone.c b/builtin/clone.c
index 0593aee..404c5e8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -285,23 +285,26 @@ static void strip_trailing_slashes(char *dir)
 
 static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
-   struct strbuf sb = STRBUF_INIT;
+   struct strbuf err = STRBUF_INIT;
int *required = cb_data;
-   char *ref_git = compute_alternate_path(item->string, );
+   char *ref_git = compute_alternate_path(item->string, );
 
if (!ref_git) {
if (*required)
-   die("%s", sb.buf);
+   die("%s", err.buf);
else
fprintf(stderr,
_("info: Could not add alternate for '%s': 
%s\n"),
-   item->string, sb.buf);
+   item->string, err.buf);
} else {
+   struct strbuf sb = STRBUF_INIT;
strbuf_addf(, "%s/objects", ref_git);
add_to_alternates_file(sb.buf);
+   strbuf_release();
}
 
-   strbuf_release();
+   strbuf_release();
+   free(ref_git);
return 0;
 }
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 472b1d9..681b91d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -484,7 +484,7 @@ struct submodule_alternate_setup {
 #define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \
SUBMODULE_ALTERNATE_ERROR_IGNORE, NULL }
 
-int add_possible_reference_from_superproject(
+static int add_possible_reference_from_superproject(
struct alternate_object_database *alt, void *sas_cb)
 {
struct submodule_alternate_setup *sas = sas_cb;
diff --git a/sha1_file.c b/sha1_file.c
index 2489653..0fe5aa3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -435,11 +435,12 @@ char *compute_alternate_path(const char *path, struct 
strbuf *err)
 {
char *ref_git = NULL;
const char *repo, *ref_git_s;
-   struct strbuf err_buf = STRBUF_INIT;
+   int seen_error = 0;
 
ref_git_s = real_path_if_valid(path);
if (!ref_git_s) {
-   strbuf_addf(_buf, _("path '%s' does not exist"), path);
+   seen_error = 1;
+   strbuf_addf(err, _("path '%s' does not exist"), path);
goto out;
} else
/*
@@ -462,40 +463,41 @@ char *compute_alternate_path(const char *path, struct 
strbuf *err)
ref_git = ref_git_git;
} else if (!is_directory(mkpath("%s/objects", ref_git))) {
struct strbuf sb = STRBUF_INIT;
+   seen_error = 1;
if (get_common_dir(, ref_git)) {
-   strbuf_addf(_buf,
+   strbuf_addf(err,
_("reference repository '%s' as a linked "
  "checkout is not supported yet."),
path);
goto out;
}
 
-   strbuf_addf(_buf, _("reference repository '%s' is not a "
+   strbuf_addf(err, _("reference repository '%s' is not a "
"local repository."), path);
goto out;
}
 
if (!access(mkpath("%s/shallow", ref_git), F_OK)) {
-   strbuf_addf(_buf, _("reference repository '%s' is shallow"),
+   strbuf_addf(err, _("reference repository '%s' is shallow"),
path);
+   seen_error = 1;
goto out;
}
 
if (!access(mkpath("%s/info/grafts", ref_git), F_OK)) {
-   strbuf_addf(_buf,
+   strbuf_addf(err,
_("reference repository '%s' is grafted"),
path);
+   seen_error = 1;
goto out;
}
 
 out:
-   if (err_buf.len) {
-   strbuf_addbuf(err, _buf);
+   if (seen_error) {
free(ref_git);
ref_git = NULL;
}
 
-   strbuf_release(_buf);
return ref_git;
 }
 
Thanks,
Stefan


v4:
Thanks to Junios critial questions regarding the design, I took a step back
to look at the bigger picture, again.

new patches:
  clone: factor out checking for an alternate path
  clone: recursive and reference option triggers submodule alternates
  
The last patch redesigns completely how we approach the problem.
Now there are no new command line options (that relate to the problem
of marrying --recursive and --reference), but instead we communicate
everything via configuration options to have a lasting effect (i.e.
submodule update remembers the decision of the initial setup)

Thanks,
Stefan

v3:

Thanks to Junios critial