From: Danh Doan <congdanhqx@gmail.com>
To: Junio C Hamano <gitster@pobox.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: Thu, 2 Apr 2020 08:09:52 +0700 [thread overview]
Message-ID: <20200402010952.GB2764@danh.dev> (raw)
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
next prev parent reply other threads:[~2020-04-02 1:09 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-30 20:03 Support for `--no-gpg-sign` in git rebase, cherry-pick, etc Dominic Chen
2020-03-31 6:44 ` [PATCH] rebase.c: teach --no-gpg-sign to git-rebase Danh Doan
2020-04-01 17:47 ` Junio C Hamano
2020-04-02 1:09 ` Danh Doan [this message]
2020-04-02 10:15 ` [PATCH v2 0/5] Honour and Document --no-gpg-sign Đoàn Trần Công Danh
2020-04-02 10:15 ` [PATCH v2 1/5] rebase.c: honour --no-gpg-sign Đoàn Trần Công Danh
2020-04-03 5:21 ` Martin Ågren
2020-04-02 10:15 ` [PATCH v2 2/5] cherry-pick/revert: honour --no-gpg-sign in all case Đoàn Trần Công Danh
2020-04-02 10:15 ` [PATCH v2 3/5] Documentation: document am --no-gpg-sign Đoàn Trần Công Danh
2020-04-02 10:15 ` [PATCH v2 4/5] Documentation: reword commit --no-gpg-sign Đoàn Trần Công Danh
2020-04-02 10:15 ` [PATCH v2 5/5] Documentation: document merge option --no-gpg-sign Đoàn Trần Công Danh
2020-04-02 18:07 ` Junio C Hamano
2020-04-03 0:25 ` Danh Doan
2020-04-03 10:28 ` [PATCH v3 0/6] Honour and Document --no-gpg-sign Đoàn Trần Công Danh
2020-04-03 10:28 ` [PATCH v3 1/6] rebase.c: honour --no-gpg-sign Đoàn Trần Công Danh
2020-04-03 10:28 ` [PATCH v3 2/6] cherry-pick/revert: honour --no-gpg-sign in all case Đoàn Trần Công Danh
2020-04-03 13:43 ` Phillip Wood
2020-04-03 14:26 ` Danh Doan
2020-04-03 10:28 ` [PATCH v3 3/6] Documentation: document am --no-gpg-sign Đoàn Trần Công Danh
2020-04-03 10:28 ` [PATCH v3 4/6] Documentation: reword commit --no-gpg-sign Đoàn Trần Công Danh
2020-04-03 10:28 ` [PATCH v3 5/6] Documentation: merge commit-tree --[no-]gpg-sign Đoàn Trần Công Danh
2020-04-03 10:28 ` [PATCH v3 6/6] Documentation: document merge option --no-gpg-sign Đoàn Trần Công Danh
2020-04-03 18:40 ` [PATCH v3 0/6] Honour and Document --no-gpg-sign Junio C Hamano
2020-04-04 14:36 ` Martin Ågren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200402010952.GB2764@danh.dev \
--to=congdanhqx@gmail.com \
--cc=d.c.ddcc@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).