git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: git <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>,
	Alban Gruin <alban.gruin@gmail.com>,
	Pratik Karki <predatoramigo@gmail.com>,
	Christian Couder <christian.couder@gmail.com>,
	Wink Saville <wink@saville.com>
Subject: Re: [PATCH 2/6] sequencer: learn about the special "fake root commit" handling
Date: Sat, 28 Apr 2018 09:11:54 -0700	[thread overview]
Message-ID: <CAGZ79kZJJadXVhcMfxqq2kH=F-6nKVuhOc_s-vgj_9w5YcOxkg@mail.gmail.com> (raw)
In-Reply-To: <42db734a98059fcfd67627aecc93cc8f0439fd37.1524868165.git.johannes.schindelin@gmx.de>

On Fri, Apr 27, 2018 at 3:31 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When an interactive rebase wants to recreate a root commit, it
> - first creates a new, empty root commit,
> - checks it out,
> - converts the next `pick` command so that it amends the empty root
>   commit
>
> Introduce support in the sequencer to handle such an empty root commit,
> by looking for the file <GIT_DIR>/rebase-merge/squash-onto; if it exists
> and contains a commit name, the sequencer will compare the HEAD to said
> root commit, and if identical, a new root commit will be created.
>
> While converting scripted code into proper, portable C, we also do away
> with the old "amend with an empty commit message, then cherry-pick
> without committing, then amend again" dance and replace it with code
> that uses the internal API properly to do exactly what we want: create a
> new root commit.
>
> To keep the implementation simple, we always spawn `git commit` to create
> new root commits.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  sequencer.h |   4 ++
>  2 files changed, 105 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 90c8218aa9a..fc124596b53 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -125,6 +125,12 @@ static GIT_PATH_FUNC(rebase_path_rewritten_list, "rebase-merge/rewritten-list")
>  static GIT_PATH_FUNC(rebase_path_rewritten_pending,
>         "rebase-merge/rewritten-pending")
>
> +/*
> + * The path of the file containig the OID of the "squash onto" commit, i.e.
> + * the dummy commit used for `reset [new root]`.
> + */
> +static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
> +
>  /*
>   * The path of the file listing refs that need to be deleted after the rebase
>   * finishes. This is used by the `label` command to record the need for cleanup.
> @@ -470,7 +476,8 @@ static int fast_forward_to(const struct object_id *to, const struct object_id *f
>         transaction = ref_transaction_begin(&err);
>         if (!transaction ||
>             ref_transaction_update(transaction, "HEAD",
> -                                  to, unborn ? &null_oid : from,
> +                                  to, unborn && !is_rebase_i(opts) ?
> +                                  &null_oid : from,
>                                    0, sb.buf, &err) ||
>             ref_transaction_commit(transaction, &err)) {
>                 ref_transaction_free(transaction);
> @@ -692,6 +699,42 @@ static char *get_author(const char *message)
>         return NULL;
>  }
>
> +static const char *read_author_ident(struct strbuf *buf)

This seems to be the counter part of write_author_script(*msg),
would it make sense to either rename this to read_author_script
or rename the counter part to write_author_ident ?

> +{
> +       char *p, *p2;
> +
> +       if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)

The 256 is a hint for read_file how to size the buffer initially.
If not given it defaults to 8k, which presumably is too much for
an author identity.



> +       for (p = buf->buf; *p; p++)
> +               if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> +                       strbuf_splice(buf, p - buf->buf, p2 - p, "'", 1);
> +               else if (*p == '\'')
> +                       strbuf_splice(buf, p-- - buf->buf, 1, "", 0);

This part could be prefixed with
    /* un-escape text: turn \\ into ' and remove single quotes. */

> +       if (skip_prefix(buf->buf, "GIT_AUTHOR_NAME=", (const char **)&p)) {
> +               strbuf_splice(buf, 0, p - buf->buf, "", 0);
> +               p = strchr(buf->buf, '\n');
> +               if (skip_prefix(p, "\nGIT_AUTHOR_EMAIL=", (const char **)&p2)) {
> +                       strbuf_splice(buf, p - buf->buf, p2 - p, " <", 2);
> +                       p = strchr(p, '\n');
> +                       if (skip_prefix(p, "\nGIT_AUTHOR_DATE=@",
> +                                       (const char **)&p2)) {
> +                               strbuf_splice(buf, p - buf->buf, p2 - p,
> +                                             "> ", 2);
> +                               p = strchr(p, '\n');
> +                               if (p) {
> +                                       strbuf_setlen(buf, p - buf->buf);
> +                                       return buf->buf;

So here we have read GIT_AUTHOR_NAME, _EMAIL
and _DATE in that order and converted it to its form
"name <email> date" in a single line.

It would be better to invert the conditions and keep
the indentation level lower by:

    if (!skip_prefix(...))
        goto warning_and_return;
    strbuf_splice(...);
    ...

I wondered if we want to factor out the conversion of
"author string in commit form" to "author information
in script form" into their own functions, and keep the reading
writing out of them. But then again we only need them in
these use cases for now, and such a refactoring can happen
later if needed.

> +       warning(_("could not parse '%s'"), rebase_path_author_script());

This function needs all three environment variables in its correct order,
which sounds a little brittle, but then again we do not expect manual
editing of that file, but expect it to be written by Git.

> @@ -1369,6 +1448,12 @@ static int is_fixup(enum todo_command command)
>         return command == TODO_FIXUP || command == TODO_SQUASH;
>  }
>
> +/* Does this command create a (non-merge) commit? */
> +static int is_pick_or_similar(enum todo_command command)
> +{
> +       return command <= TODO_SQUASH;
> +}

This code looks scary.
Fortunately the enum todo_command hints that the order matters,
such that we're probably protected from wild reordering in the future,
however this implies that the section /* commands that handle commits */
comes first and that TODO_SQUASH is the last entry of that section.
So maybe we'd want to add a /* must be last in section */ to
TODO_squash and also document that the section must be first?

Do we have other code that needs a very specific ordering
with similar further assumptions (section being first/last, a
command being first/last in their section)?

I wondered what the _or_similar means and by looking up
that enum, I would think a name like

static int handles_single_commit(enum todo_command)

might be better?

> @@ -1523,7 +1608,14 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>                         return error(_("your index file is unmerged."));
>         } else {
>                 unborn = get_oid("HEAD", &head);
> -               if (unborn)
> +               /* Do we want to generate a root commit? */
> +               if (is_pick_or_similar(command) && opts->have_squash_onto &&
> +                   !oidcmp(&head, &opts->squash_onto)) {
> +                       if (is_fixup(command))
> +                               return error(_("cannot fixup root commit"));

I would expect you also cannot squash into root commit?

Thanks,
Stefan

  reply	other threads:[~2018-04-28 16:11 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-27 22:29 [PATCH 0/6] Let the sequencer handle `git rebase -i --root` Johannes Schindelin
2018-04-27 22:30 ` [PATCH 1/6] sequencer: extract helper to update active_cache_tree Johannes Schindelin
2018-04-28 15:28   ` Stefan Beller
2018-04-27 22:31 ` [PATCH 2/6] sequencer: learn about the special "fake root commit" handling Johannes Schindelin
2018-04-28 16:11   ` Stefan Beller [this message]
2018-04-29 12:33     ` Johannes Schindelin
2018-04-29 21:44       ` Stefan Beller
2018-04-27 22:31 ` [PATCH 3/6] rebase -i --root: let the sequencer handle even the initial part Johannes Schindelin
2018-04-28 16:19   ` Stefan Beller
2018-04-29 12:34     ` Johannes Schindelin
2018-04-27 22:31 ` [PATCH 4/6] sequencer: allow introducing new root commits Johannes Schindelin
2018-04-27 22:31 ` [PATCH 5/6] rebase --rebase-merges: a "merge" into a new root is a fast-forward Johannes Schindelin
2018-04-27 22:31 ` [PATCH 6/6] rebase --rebase-merges: root commits can be cousins, too Johannes Schindelin
2018-05-03 23:01 ` [PATCH v2 0/6] Let the sequencer handle `git rebase -i --root` Johannes Schindelin
2018-05-03 23:01   ` [PATCH v2 1/6] sequencer: extract helper to update active_cache_tree Johannes Schindelin
2018-05-03 23:01   ` [PATCH v2 2/6] sequencer: learn about the special "fake root commit" handling Johannes Schindelin
2018-05-03 23:01   ` [PATCH v2 3/6] rebase -i --root: let the sequencer handle even the initial part Johannes Schindelin
2018-05-03 23:01   ` [PATCH v2 4/6] sequencer: allow introducing new root commits Johannes Schindelin
2018-05-03 23:01   ` [PATCH v2 5/6] rebase --rebase-merges: a "merge" into a new root is a fast-forward Johannes Schindelin
2018-05-03 23:01   ` [PATCH v2 6/6] rebase --rebase-merges: root commits can be cousins, too Johannes Schindelin
2018-05-04 19:55   ` [PATCH v2 0/6] Let the sequencer handle `git rebase -i --root` Stefan Beller
2018-05-05 19:24     ` Johannes Schindelin

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGZ79kZJJadXVhcMfxqq2kH=F-6nKVuhOc_s-vgj_9w5YcOxkg@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=alban.gruin@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=predatoramigo@gmail.com \
    --cc=wink@saville.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).