Re: [PATCH v2 2/2] push: propagate push-options with --recurse-submodules

[Brandon Williams]

On 04/05, Jacob Keller wrote:
> On Fri, Mar 31, 2017 at 5:19 PM, Jonathan Nieder  wrote:
> > Brandon Williams wrote:
> >
> >> Teach push --recurse-submodules to propagate push-options recursively to
> >> the pushes performed in the submodules.
> >
> > Some time in the future we may want "push --recurse-submodules" to do a
> > dry run pass before doing the final push, so that if it is known that
> > some of the pushes wouldn't succeed (e.g. due to not being
> > fast-forward, or the server not being reachable, or the server not
> > supporting push options) then git could stop early instead of some
> > succeeding and some failing.
> >
> > But that's a larger and separate change from this one.  Users of push
> > --recurse-submodules today know they are effectively asking for
> > multiple pushes that are not guaranteed to succeed or fail together.
> >
> 
> If you want it to be truly atomic it will require more effort than the
> above. Suppose that you do a dry-run first, and then find out
> everything will succeed. After this, you do the real pushes. But in
> between these two commands something could have changed, and you could
> still end up with a non-atomic set of pushes.
> 
> I think that's ok and better than before, but it should be noted that
> you stll don't guarantee that all the pushes succeed or fail together.
> 
> I'm really not sure if you even can make these pushes work atomically
> considering they are going to different hosts.

Yeah, really it becomes impossible to have submodules pushed to multiple
hosts in an atomic way.  I think what Jonathan is mentioning is a way to
ensure that the options and everything are supported by the server you
are communicating with (for each submodule), though you could just run
an explicit dry-run yourself.

If you truly wanted the superproject and all submodules updated in an
atomic way you would need some other server side service (e.g. gerrit)
to ensure that it was done properly.

-- 
Brandon Williams

Re: [PATCH v2 2/2] push: propagate push-options with --recurse-submodules

[Jacob Keller]

On Fri, Mar 31, 2017 at 5:19 PM, Jonathan Nieder  wrote:
> Brandon Williams wrote:
>
>> Teach push --recurse-submodules to propagate push-options recursively to
>> the pushes performed in the submodules.
>
> Some time in the future we may want "push --recurse-submodules" to do a
> dry run pass before doing the final push, so that if it is known that
> some of the pushes wouldn't succeed (e.g. due to not being
> fast-forward, or the server not being reachable, or the server not
> supporting push options) then git could stop early instead of some
> succeeding and some failing.
>
> But that's a larger and separate change from this one.  Users of push
> --recurse-submodules today know they are effectively asking for
> multiple pushes that are not guaranteed to succeed or fail together.
>

If you want it to be truly atomic it will require more effort than the
above. Suppose that you do a dry-run first, and then find out
everything will succeed. After this, you do the real pushes. But in
between these two commands something could have changed, and you could
still end up with a non-atomic set of pushes.

I think that's ok and better than before, but it should be noted that
you stll don't guarantee that all the pushes succeed or fail together.

I'm really not sure if you even can make these pushes work atomically
considering they are going to different hosts.

Thanks,
Jake

Re: [PATCH v2 2/2] push: propagate push-options with --recurse-submodules

[Jonathan Nieder]

Brandon Williams wrote:

> Teach push --recurse-submodules to propagate push-options recursively to
> the pushes performed in the submodules.

Some time in the future we may want "push --recurse-submodules" to do a
dry run pass before doing the final push, so that if it is known that
some of the pushes wouldn't succeed (e.g. due to not being
fast-forward, or the server not being reachable, or the server not
supporting push options) then git could stop early instead of some
succeeding and some failing.

But that's a larger and separate change from this one.  Users of push
--recurse-submodules today know they are effectively asking for
multiple pushes that are not guaranteed to succeed or fail together.

> Signed-off-by: Brandon Williams 
> ---
>  submodule.c | 13 +++--
>  submodule.h |  1 +
>  t/t5545-push-options.sh | 39 +++
>  transport.c |  1 +
>  4 files changed, 52 insertions(+), 2 deletions(-)

For what it's worth,
Reviewed-by: Jonathan Nieder 

[PATCH v2 2/2] push: propagate push-options with --recurse-submodules

[Brandon Williams]

Teach push --recurse-submodules to propagate push-options recursively to
the pushes performed in the submodules.

Signed-off-by: Brandon Williams 
---
 submodule.c | 13 +++--
 submodule.h |  1 +
 t/t5545-push-options.sh | 39 +++
 transport.c |  1 +
 4 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index c52d6634c..de444be61 100644
--- a/submodule.c
+++ b/submodule.c
@@ -782,7 +782,9 @@ int find_unpushed_submodules(struct sha1_array *commits,
return needs_pushing->nr;
 }
 
-static int push_submodule(const char *path, int dry_run)
+static int push_submodule(const char *path,
+ const struct string_list *push_options,
+ int dry_run)
 {
if (add_submodule_odb(path))
return 1;
@@ -793,6 +795,12 @@ static int push_submodule(const char *path, int dry_run)
if (dry_run)
argv_array_push(, "--dry-run");
 
+   if (push_options && push_options->nr) {
+   const struct string_list_item *item;
+   for_each_string_list_item(item, push_options)
+   argv_array_pushf(, "--push-option=%s",
+item->string);
+   }
prepare_submodule_repo_env(_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
@@ -807,6 +815,7 @@ static int push_submodule(const char *path, int dry_run)
 
 int push_unpushed_submodules(struct sha1_array *commits,
 const char *remotes_name,
+const struct string_list *push_options,
 int dry_run)
 {
int i, ret = 1;
@@ -818,7 +827,7 @@ int push_unpushed_submodules(struct sha1_array *commits,
for (i = 0; i < needs_pushing.nr; i++) {
const char *path = needs_pushing.items[i].string;
fprintf(stderr, "Pushing submodule '%s'\n", path);
-   if (!push_submodule(path, dry_run)) {
+   if (!push_submodule(path, push_options, dry_run)) {
fprintf(stderr, "Unable to push submodule '%s'\n", 
path);
ret = 0;
}
diff --git a/submodule.h b/submodule.h
index 8a8bc49dc..0e26430fd 100644
--- a/submodule.h
+++ b/submodule.h
@@ -92,6 +92,7 @@ extern int find_unpushed_submodules(struct sha1_array 
*commits,
struct string_list *needs_pushing);
 extern int push_unpushed_submodules(struct sha1_array *commits,
const char *remotes_name,
+   const struct string_list *push_options,
int dry_run);
 extern void connect_work_tree_and_git_dir(const char *work_tree, const char 
*git_dir);
 extern int parallel_submodules(void);
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 97065e62b..32c513c78 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -142,6 +142,45 @@ test_expect_success 'push options work properly across 
http' '
test_cmp expect actual
 '
 
+test_expect_success 'push options and submodules' '
+   test_when_finished "rm -rf parent" &&
+   test_when_finished "rm -rf parent_upstream" &&
+   mk_repo_pair &&
+   git -C upstream config receive.advertisePushOptions true &&
+   cp -r upstream parent_upstream &&
+   test_commit -C upstream one &&
+
+   test_create_repo parent &&
+   git -C parent remote add up ../parent_upstream &&
+   test_commit -C parent one &&
+   git -C parent push --mirror up &&
+
+   git -C parent submodule add ../upstream workbench &&
+   git -C parent commit -m "add submoule" &&
+
+   test_commit -C parent/workbench two &&
+   git -C parent add workbench &&
+   git -C parent commit -m "update workbench" &&
+
+   git -C parent push \
+   --push-option=asdf --push-option="more structured text" \
+   --recurse-submodules=on-demand up master &&
+
+   git -C upstream rev-parse --verify master >expect &&
+   git -C parent/workbench rev-parse --verify master >actual &&
+   test_cmp expect actual &&
+
+   git -C parent_upstream rev-parse --verify master >expect &&
+   git -C parent rev-parse --verify master >actual &&
+   test_cmp expect actual &&
+
+   printf "asdf\nmore structured text\n" >expect &&
+   test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+   test_cmp expect upstream/.git/hooks/post-receive.push_options &&
+   test_cmp expect parent_upstream/.git/hooks/pre-receive.push_options &&
+   test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
+'
+
 stop_httpd
 
 test_done
diff --git a/transport.c b/transport.c
index 417ed7f19..64e60b635