Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references

[Stefan Beller]

On Fri, Aug 5, 2016 at 2:31 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> I thought about rolling it as a strict bugfix; but the bug is shaded by the
>> inverse bug in the helper, so the user would never see an issue.
>
> Ahh, OK, because the helper accepts "--reference" "--reference=foo"
> as a OPT_STRING whose value happens to be "--reference=foo", and
> then uses
>
> if (suc->reference)
> argv_array_push(>args, suc->reference)
>
> where suc->reference _is_ "--reference=foo" when invoking the
> underlying "git clone", it cancels out.
>
> Then it is OK.
>
> In fact there is NO bug.  It just is that update_clone subcommand
> used a convention different from others that took the whole
> --option=arg as a parameter to --reference option.  It could be
> argued that it is an API bug between git-submodule.sh and
> git-submodule--helper, but nobody else goes through this "weird"
> interface, so it is not worth splitting the patch.


I'll mention the fix of the API bug in the reroll then.
--
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

Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references

[Junio C Hamano]

Stefan Beller  writes:

> I thought about rolling it as a strict bugfix; but the bug is shaded by the
> inverse bug in the helper, so the user would never see an issue.

Ahh, OK, because the helper accepts "--reference" "--reference=foo"
as a OPT_STRING whose value happens to be "--reference=foo", and
then uses

if (suc->reference)
argv_array_push(>args, suc->reference)

where suc->reference _is_ "--reference=foo" when invoking the
underlying "git clone", it cancels out.

Then it is OK.

In fact there is NO bug.  It just is that update_clone subcommand
used a convention different from others that took the whole
--option=arg as a parameter to --reference option.  It could be
argued that it is an API bug between git-submodule.sh and
git-submodule--helper, but nobody else goes through this "weird"
interface, so it is not worth splitting the patch.
--
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

Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references

[Stefan Beller]

On Fri, Aug 5, 2016 at 2:06 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> -   ${reference:+--reference "$reference"} \
>>> +   ${reference:+"$reference"} \
>>
>> Note how this changed the API of the submodule--helper.
>> Currently we pass in --reference $reference
>> and $reference consists of the string "--reference" and the actual
>> reference. So it looked like '--reference' '--reference=foo'
>
> So this change is a strict bugfix.  The code without this patch
> had cmd_update that places "--reference=foo" in $reference, but
> the call to "git submodule--helper" it makes added an unnecessary
> "--reference" in front of it.

I thought about rolling it as a strict bugfix; but the bug is shaded by the
inverse bug in the helper, so the user would never see an issue.

It is more a cleanup of the internal communication between the shell
script and the helper written in C. (And that communication is
supposed to not be user visible or relevant)

>
> I was wondering why there is no corresponding change to add
> "--reference" on the code that calls cmd_update().  Thanks for
> saving my time ;-)

When I wrote the patch I cared more about the while-at-it part than
the bug fix as I do not consider the bug worth to merge down to maint
or even fast tracking it as a bug fix. It's just changing the internal
communication to be clearer.

>
> Perhaps that needs to go into the log message, though.  Allowing
> multiple references is not that interesting from the POV of the
> codebase immediately after this step and only deserves a "by the
> way" mention.
>
> Subject: submodule--helper: fix cmd_update()
>
> cmd_update places "--reference=foo" in $reference, but the call to
> "git submodule--helper" it makes adds an unnecessary "--reference"
> in front of it.  The underlying "git clone" was called with two
> command options "--reference" and "--reference=foo" because of
> this.
>
> While at it, prepare the code to accept multiple references, as
> the underlying "git clone" can happily take more than one.
>
> or something like that.
>
> Needless to say, "While at it" could become a separate step, and a
> pure bugfix part may even want to become a separate and more urgent
> topic that can go to 'maint', with a test to prevent regression.

Sure. I'll do so in a reroll.
--
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

Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references

[Junio C Hamano]

Stefan Beller  writes:

>> -   ${reference:+--reference "$reference"} \
>> +   ${reference:+"$reference"} \
>
> Note how this changed the API of the submodule--helper.
> Currently we pass in --reference $reference
> and $reference consists of the string "--reference" and the actual
> reference. So it looked like '--reference' '--reference=foo'

So this change is a strict bugfix.  The code without this patch
had cmd_update that places "--reference=foo" in $reference, but
the call to "git submodule--helper" it makes added an unnecessary
"--reference" in front of it.

I was wondering why there is no corresponding change to add
"--reference" on the code that calls cmd_update().  Thanks for
saving my time ;-)

Perhaps that needs to go into the log message, though.  Allowing
multiple references is not that interesting from the POV of the
codebase immediately after this step and only deserves a "by the
way" mention.

Subject: submodule--helper: fix cmd_update()

cmd_update places "--reference=foo" in $reference, but the call to
"git submodule--helper" it makes adds an unnecessary "--reference"
in front of it.  The underlying "git clone" was called with two
command options "--reference" and "--reference=foo" because of
this.

While at it, prepare the code to accept multiple references, as
the underlying "git clone" can happily take more than one.

or something like that.

Needless to say, "While at it" could become a separate step, and a
pure bugfix part may even want to become a separate and more urgent
topic that can go to 'maint', with a test to prevent regression.
--
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

Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references

[Stefan Beller]

> -   ${reference:+--reference "$reference"} \
> +   ${reference:+"$reference"} \

Note how this changed the API of the submodule--helper.
Currently we pass in --reference $reference
and $reference consists of the string "--reference" and the actual
reference. So it looked like '--reference' '--reference=foo'

That is why we can pass the argument unseen to clone in the helper
via

argv_array_push(>args, suc->reference);

This is fixed now.
--
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

[PATCH 4/6] submodule--helper update-clone: allow multiple references

[Stefan Beller]

Allow the user to pass in multiple references to update_clone.
Currently this is only internal API, but once the shell script is
replaced by a C version, this is needed.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 14 +-
 git-submodule.sh|  2 +-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 896a3ec..b6f297b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -583,7 +583,7 @@ struct submodule_update_clone {
/* configuration parameters which are passed on to the children */
int quiet;
int recommend_shallow;
-   const char *reference;
+   struct string_list references;
const char *depth;
const char *recursive_prefix;
const char *prefix;
@@ -599,7 +599,8 @@ struct submodule_update_clone {
int failed_clones_nr, failed_clones_alloc;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
-   SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
+   SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \
+   NULL, NULL, NULL, \
STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
 
 
@@ -709,8 +710,11 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_pushl(>args, "--path", sub->path, NULL);
argv_array_pushl(>args, "--name", sub->name, NULL);
argv_array_pushl(>args, "--url", url, NULL);
-   if (suc->reference)
-   argv_array_push(>args, suc->reference);
+   if (suc->references.nr) {
+   struct string_list_item *item;
+   for_each_string_list_item(item, >references)
+   argv_array_pushl(>args, "--reference", 
item->string, NULL);
+   }
if (suc->depth)
argv_array_push(>args, suc->depth);
 
@@ -829,7 +833,7 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
OPT_STRING(0, "update", ,
   N_("string"),
   N_("rebase, merge, checkout or none")),
-   OPT_STRING(0, "reference", , N_("repo"),
+   OPT_STRING_LIST(0, "reference", , N_("repo"),
   N_("reference repository")),
OPT_STRING(0, "depth", , "",
   N_("Create a shallow clone truncated to the "
diff --git a/git-submodule.sh b/git-submodule.sh
index 2b23ce6..526ea5d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -575,7 +575,7 @@ cmd_update()
${wt_prefix:+--prefix "$wt_prefix"} \
${prefix:+--recursive-prefix "$prefix"} \
${update:+--update "$update"} \
-   ${reference:+--reference "$reference"} \
+   ${reference:+"$reference"} \
${depth:+--depth "$depth"} \
${recommend_shallow:+"$recommend_shallow"} \
${jobs:+$jobs} \
-- 
2.9.2.572.g9d9644e.dirty

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