Re: [PATCH v5 03/10] transport-helper: add 'force' to 'export' helpers

Junio C Hamano [off-list ref] writes:

Felipe Contreras [off-list ref] writes:

quoted

On Thu, Oct 31, 2013 at 12:21 PM, Junio C Hamano [off-list ref] wrote:

quoted

Felipe Contreras [off-list ref] writes:

quoted

Otherwise they cannot know when to force the push or not (other than
hacks).
...

diff --git a/transport-helper.c b/transport-helper.c
index d05fc7c..ed238e5 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -854,6 +854,11 @@ static int push_refs_with_export(struct transport *transport,
                      die("helper %s does not support dry-run", data->name);
      }

+     if (flags & TRANSPORT_PUSH_FORCE) {
+             if (set_helper_option(transport, "force", "true") != 0)
+                     die("helper %s does not support 'force'", data->name);
+     }
+

Does this cause a "git push --force $there A:B" to fail when $there
is a destination that goes via an existing helper does not suport
"force" option?

Yes.

quoted

Should it fail even when the current value of B is an ancestor of A
(i.e. when an unforced push would succeed)?

It might make sense to fail only when the push is non-fast-forward,
but it's not so straight-forward to implement.

OK; let's see if anybody screams by cooking the series in 'next'
(that is, when other issues people may discover in the series are
addressed).

Thanks.

Actually, thinking about it again, would it be a better alternative
to issue an error message and continue, instead of dying here
(i.e. replace the above die() with error() or even warning())?

That way, if the helper has not been updated to support 'force', but
the user has been happily accepting the result he gets from "git
push $there +master" (perhaps because he has never pushed a non-ff
history so far), he now gets a warning that essentially says that
his push has been working by accident, even when the particular
ff-push that showed the error message goes through successfully.

If his push through the helper that has not been updated does need
'force', he gets the same old behaviour, possibly a broken one, out
of the helper, and again he does get the warning.

Because updated helpers know 'force' option, such a s/die/warning/
change would not affect them at all.

Am I missing something?

On Fri, Nov 1, 2013 at 8:49 AM, Junio C Hamano [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:

quoted

Felipe Contreras [off-list ref] writes:

quoted

On Thu, Oct 31, 2013 at 12:21 PM, Junio C Hamano [off-list ref] wrote:

quoted

Felipe Contreras [off-list ref] writes:

quoted

Otherwise they cannot know when to force the push or not (other than
hacks).
...

diff --git a/transport-helper.c b/transport-helper.c
index d05fc7c..ed238e5 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -854,6 +854,11 @@ static int push_refs_with_export(struct transport *transport,
                      die("helper %s does not support dry-run", data->name);
      }

+     if (flags & TRANSPORT_PUSH_FORCE) {
+             if (set_helper_option(transport, "force", "true") != 0)
+                     die("helper %s does not support 'force'", data->name);
+     }
+

Does this cause a "git push --force $there A:B" to fail when $there
is a destination that goes via an existing helper does not suport
"force" option?

Yes.

quoted

Should it fail even when the current value of B is an ancestor of A
(i.e. when an unforced push would succeed)?

It might make sense to fail only when the push is non-fast-forward,
but it's not so straight-forward to implement.

OK; let's see if anybody screams by cooking the series in 'next'
(that is, when other issues people may discover in the series are
addressed).

Thanks.

Actually, thinking about it again, would it be a better alternative
to issue an error message and continue, instead of dying here
(i.e. replace the above die() with error() or even warning())?

That way, if the helper has not been updated to support 'force', but
the user has been happily accepting the result he gets from "git
push $there +master" (perhaps because he has never pushed a non-ff
history so far), he now gets a warning that essentially says that
his push has been working by accident, even when the particular
ff-push that showed the error message goes through successfully.

If his push through the helper that has not been updated does need
'force', he gets the same old behaviour, possibly a broken one, out
of the helper, and again he does get the warning.

Because updated helpers know 'force' option, such a s/die/warning/
change would not affect them at all.

Yeah, that makes sense.

-- 
Felipe Contreras