Re: [PATCH] rebase.c: teach --no-gpg-sign to git-rebase

From: Junio C Hamano <gitster@pobox.com>
To: Danh Doan <congdanhqx@gmail.com>
Cc: Dominic Chen <d.c.ddcc@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH] rebase.c: teach --no-gpg-sign to git-rebase
Date: Wed, 01 Apr 2020 10:47:15 -0700	[thread overview]
Message-ID: <xmqqv9mj5c9o.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200331064456.GA15850@danh.dev> (Danh Doan's message of "Tue, 31 Mar 2020 13:44:56 +0700")

Danh Doan <congdanhqx@gmail.com> writes:

> From: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> Subject: [PATCH] rebase.c: teach --no-gpg-sign to git-rebase
>
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index f7a6033607..54023cf3bb 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -358,6 +358,11 @@ See also INCOMPATIBLE OPTIONS below.
>  	defaults to the committer identity; if specified, it must be
>  	stuck to the option without a space.
>  
> +--no-gpg-sign::
> +	Countermand `commit.gpgSign` configuration variable that is
> +	set to force each and every commit to be signed.
> +
> +

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.

> 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.

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.

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.

Thanks.