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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 1, 2020, 17:47 UTC
Message-ID: <xmqqv9mj5c9o.fsf@gitster.c.googlers.com>
In-Reply-To: <20200331064456.GA15850@danh.dev>

Danh Doan <congdanhqx@gmail.com> writes:

Show 18 quoted lines

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

Show 10 quoted lines

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

Show 12 quoted lines

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