Re: [PATCH 1/9] transport-helper: add 'force' to 'export' helpers

[Felipe Contreras <felipe.contreras@gmail.com>]

Richard Hansen wrote:
> On 2013-08-29 11:23, Felipe Contreras wrote:
> > Otherwise they cannot know when to force the push or not (other than
> > hacks).
> > 
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  transport-helper.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/transport-helper.c b/transport-helper.c
> > index 63cabc3..62051a6 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -814,6 +814,9 @@ static int push_refs_with_export(struct transport *transport,
> >  			die("helper %s does not support dry-run", data->name);
> >  	}
> >  
> > +	if (flags & TRANSPORT_PUSH_FORCE)
> > +		set_helper_option(transport, "force", "true");
> 
> Should the return value of set_helper_option() be checked?

Yeah, it would make sense. I guess we want to die() if the user does
'git push -f' and the remote helper doesn't support that.

> Also, should there be a #define TRANS_OPT_FORCE "force" with

I don't see the point of that. Defines are useful when you want to change the
value string, so you don't have to change the string everywhere, but we
definitely don't want to do that, as that would break backwards compatibility,
so TRANS_OPT_KEEP would always be "keep" so it's just a way to tire our
fingers.

> TRANS_OPT_FORCE added to boolean_options[]?

I don't see how that would help us, the only thing that would achieve is to map:


  set_helper_option(transport, "force", 1);

to

  set_helper_option(transport, "force", "true");

But we are already doing that.

Moreover, the code is already doing something similar for all the other options.

  set_helper_option(t, "progress", t->progress ? "true" : "false");
  set_helper_option(t, "verbosity", buf);
  set_helper_option(transport, "servpath", exec);
  set_helper_option(transport, "dry-run", "true");

-- 
Felipe Contreras