From: Danh Doan <congdanhqx@gmail.com>
Date: Thu, 02 Apr 2020 01:09:52 GMT
Subject: Re: [PATCH] rebase.c: teach --no-gpg-sign to git-rebase
Message-ID: <20200402010952.GB2764@danh.dev>
In-Reply-To: <xmqqv9mj5c9o.fsf@gitster.c.googlers.com>

On 2020-04-01 10:47:15-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Two points.  
> 
>  - There must be already an entry for '--gpg-sign'.  It would make
>    more sense to make this addtion a part of its description.
> 
>  - The --no-<option> form is not just to override a configured
>    default, but also to coumtermand an option given earlier on the
>    command line.  In other words "rebase -S --no-gpg-sign" without
>    any commit.gpgSign should work just fine.

That paragraph was copy-pasted from git-commit documentation.
I think it would need a clean up there, too.

And, mention of --no-gpg-sign in am, cherry-pick, revert,
merge-option.

While writing this, I've checked (again) all commands mentioned
--gpg-sign. To my surprise, "revert" (despite shares most of code with
"cherry-pick") doesn't honour --no-gpg-sign, either.

I'll teach "--no-gpg-sign" too revert and update all documentation for
this.

> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 27a07d4e78..a8cc5cfe0c 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -1593,6 +1593,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  
> >  	options.allow_empty_message = 1;
> >  	git_config(rebase_config, &options);
> > +	// options.gpg_sign_opt will be either "-S" or NULL
> > +	// It'll be freed later, hence, no skip-prefix
> 
> Don't use //- comments.
>
> > +	gpg_sign = options.gpg_sign_opt ? "" : NULL;
> 
> We've read configured commit.gpgSign in options.gpg_sign_opt; it is
> either a freeable "-S" or NULL depending on its value.  We initialize
> the local gpg_sign variable to either an unfreeable "" or NULL here.
> 
> Let's see how that local variable is later used here.  We know it is
> given as the target variable to OPTION_STRING, which will overwrite
> with the value given from the command line, so "" that is unfreeable
> avoids an unnecessary leak.
> 
>  - If we did not have --gpg-sign, or --no-gpg-sign, then the local
>    variable gpg_sign will stay to be either "" or NULL after
>    parse_options() returns.
> 
>  - If we had --gpg-sign or --no-gpg-sign, we will have the value
>    given from the last one of them on the command line in gpg_sign
>    after parse_options() returns.
> 
> 
> 
> > @@ -1823,10 +1826,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  	if (options.empty != EMPTY_UNSPECIFIED)
> >  		imply_merge(&options, "--empty");
> >  
> > -	if (gpg_sign) {
> > -		free(options.gpg_sign_opt);
> > +	free(options.gpg_sign_opt);
> > +	if (gpg_sign)
> >  		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
> > -	}
> > +	else
> > +		options.gpg_sign_opt = NULL;
> 
> Now we _always_ override options.gpg_sign_opt based on the value in
> the local gpg_sign variable, so the *ONLY* time options.gpg_sign_opt
> is used is immediately after git_config() returns to decide what
> value to assign to gpg_sign we saw above.  I *think* it would be
> much clearer to FREE_AND_NULL options.gpg_sign_out immediately after
> we initialize gpg_sign above, instead of freeing it here.

Make sense,

> Then you do not need the elese clause here, either.
> 
> This is a total tangent, but do we ever call cmd_rebase__interactive()
> these days?  It does not seem to do the config thing, and assigns the
> string taken from the command line to opts.gpg_sign_opt, which means
> that it is an error to free the field in any codepath that can be
> reached from there.

cmd_rebase__interactive go through different code path, and it doesn't
run into above line

> I suspect that after removing "rebase --preserve-merges", there is
> nobody that calls "git rebase--interactive", and at that point the
> function will be dead-code and can safely be removed.

I've grep-ed the code and it's look like only "rebase -p" call
cmd_rebase__interactive,

I've drafted a test, and "rebase -p" indeeds doesn't honour
"--no-gpg-sign",

Consider the deprecation of "--preserve-merges" is more than a year,
I think I'll mark that test as broken instead of trying to fix it.

-- 
Danh