Re: [PATCH v5 03/10] transport-helper: add 'force' to 'export' helpers
On Fri, Nov 1, 2013 at 12:35 PM, Junio C Hamano wrote: > Subject: [PATCH] transport-helper: demote lack of "force" option to a warning > > It would have been a good conservative position to take, if there > were no existing helpers, to die when the user asked to force a push > through a transport helper mechanism and the helper script hasn't > been updated to handle the "force" option. > > There however are existing helpers in the field and none of them > obviously has been taught about the option yet. If a helper has not > been updated to understand "force", but the user has happily been > accepting the result of "git push $there +master" (perhaps because > he has never pushed a non-ff history so far), the change made > previously in this series will fail the push, which would be a minor > regression for such a user. > > Demote lack of "force" option from a "die()" to a "warning()". By > doing so, such a user now gets a warning that notifies him 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 does need "force", he gets the same old behaviour, > possibly a broken one, out of the helper that has not been update, s/update/updated/ > but again he does see the warning. > > Helped-by: Felipe Contreras > Signed-off-by: Junio C Hamano -- 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 v5 03/10] transport-helper: add 'force' to 'export' helpers
Felipe Contreras writes:
> On Fri, Nov 1, 2013 at 8:49 AM, Junio C Hamano wrote:
>
>> Because updated helpers know 'force' option, such a s/die/warning/
>> change would not affect them at all.
>
> Yeah, that makes sense.
OK, then let's queue this separately on top, so that we can revert
it more easily when our "s/die/warning/ should be safe and more
friendly to existing users" reasoning we discussed is found to be
faulty later.
-- >8 --
Subject: [PATCH] transport-helper: demote lack of "force" option to a warning
It would have been a good conservative position to take, if there
were no existing helpers, to die when the user asked to force a push
through a transport helper mechanism and the helper script hasn't
been updated to handle the "force" option.
There however are existing helpers in the field and none of them
obviously has been taught about the option yet. If a helper has not
been updated to understand "force", but the user has happily been
accepting the result of "git push $there +master" (perhaps because
he has never pushed a non-ff history so far), the change made
previously in this series will fail the push, which would be a minor
regression for such a user.
Demote lack of "force" option from a "die()" to a "warning()". By
doing so, such a user now gets a warning that notifies him 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 does need "force", he gets the same old behaviour,
possibly a broken one, out of the helper that has not been update,
but again he does see the warning.
Helped-by: Felipe Contreras
Signed-off-by: Junio C Hamano
---
transport-helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/transport-helper.c b/transport-helper.c
index ef91882..6b167ea 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -862,7 +862,7 @@ static int push_refs_with_export(struct transport
*transport,
if (flags & TRANSPORT_PUSH_FORCE) {
if (set_helper_option(transport, "force", "true") != 0)
- die("helper %s does not support 'force'", data->name);
+ warning("helper %s does not support 'force'",
data->name);
}
helper = get_helper(transport);
--
1.8.5-rc0-205-g5b7460b
--
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 v5 03/10] transport-helper: add 'force' to 'export' helpers
On Fri, Nov 1, 2013 at 8:49 AM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>> Felipe Contreras writes:
>>
>>> On Thu, Oct 31, 2013 at 12:21 PM, Junio C Hamano wrote:
Felipe Contreras writes:
> 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.
>>>
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
--
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 v5 03/10] transport-helper: add 'force' to 'export' helpers
Junio C Hamano writes:
> Felipe Contreras writes:
>
>> On Thu, Oct 31, 2013 at 12:21 PM, Junio C Hamano wrote:
>>> Felipe Contreras writes:
>>>
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.
>>
>>> 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?
--
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 v5 03/10] transport-helper: add 'force' to 'export' helpers
Felipe Contreras writes:
> On Thu, Oct 31, 2013 at 12:21 PM, Junio C Hamano wrote:
>> Felipe Contreras writes:
>>
>>> 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.
>
>> 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.
--
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 v5 03/10] transport-helper: add 'force' to 'export' helpers
On Thu, Oct 31, 2013 at 12:21 PM, Junio C Hamano wrote:
> Felipe Contreras writes:
>
>> 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.
> 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.
--
Felipe Contreras
--
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 v5 03/10] transport-helper: add 'force' to 'export' helpers
Felipe Contreras writes:
> 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?
Should it fail even when the current value of B is an ancestor of A
(i.e. when an unforced push would succeed)?
--
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
