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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Sep 23, 2013, 02:39 UTC
Message-ID: <523fa9df8ed1b_5c910e3e801fa@nysa.mail>
In-Reply-To: <523FAA59.6040200@bbn.com>

Richard Hansen wrote:

Show 21 quoted lines

> 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