On 2020-04-01 10:47:15-0700, Junio C Hamano <gitster@xxxxxxxxx> 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
