git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH v2 00/18] builtin rebase options
Date: Thu, 06 Sep 2018 12:50:10 -0700	[thread overview]
Message-ID: <xmqqmusuz9ql.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <pull.33.v2.git.gitgitgadget@gmail.com> (Johannes Schindelin via GitGitGadget's message of "Tue, 04 Sep 2018 14:59:47 -0700 (PDT)")

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> This patch series completes the support for all rebase options in the
> builtin rebase, e.g. --signoff, rerere-autoupdate, etc.
>
> It is based on pk/rebase -in-c-3-acts.

... which in turn was based on pk/rebase-in-c-2-basic that just got
rerolled, so I would assume that you want pk/rebase-in-c-3-acts I
have rebased on top of the result of applying the updated 2-basic
series.

I've rebuilt the collection of topics up to pk/rebase-in-c-6-final
with these two updated series twice, once doing it manually, like I
did the last time, and another using "rebase -i -r" on top of the
updated pk/rebase-in-c-4-opts.  The resulting trees match, of
course.

I did it twice to try out how it feels to use "rebase -i -r" because
I wanted to make sure what we are shipping in 'master' behaves
sensibly ;-)

Two things I noticed about the recreation of the merge ...

	Reminder to bystanders.  We need to merge ag/rebase-i-in-c
	topic on top of pk/reabse-in-c-5-test topic before applying
	a patch to adjust rebase to call rebase-i using the latter's
	new calling convention.  The topics look like

	- pk/rebase-in-c has three patches on master
	- pk/rebase-in-c-2-basic builds on it, and being replaced
	- pk/rebase-in-c-3-acts builds on 2-basic (no update this time)
	- pk/rebase-in-c-4-opts builds on 3-acts, and being replaced
	- pk/rebase-in-c-5-test builds on 4-opts (no update this time)
	- js/rebase-in-c-5.5 builds on 5-test and merges ag/rebase-in-c
	  topic before applying one patch on it (no update this time)
	- pk/rebase-in-c-6-final builds on 5.5 (no update this time)

	and we are replacing 2-basic with 11 patches and 4-opts with
	18 patches.

... using "rebase -i -r" are that 

 (1) it rebuilt, or at least offered to rebuild, the entire side
     branch, even though there is absolutely no need to.  Leaving
     "pick"s untouched, based on the correct fork point, resulted in
     all picks fast forwarded, but it was somewhat alarming.

 (2) "merge -C <original merge commit> ag/rebase-i-in-c" appeared as
     the insn to merge in the (possibly rebuilt) side branch.  And
     just like "commit -C", it took the merge message from the
     original merge commit, which means that the summary of the
     merged side branch is kept stale.  In this particular case, I
     did not even want to see ag/rebase-i-in-c topic touched, so I
     knew I want to keep the original merge summary, but if the user
     took the offer to rewrite the side branch (e.g. with a "reword"
     to retitle), using the original merge message would probably
     disappoint the user.

I think (1) actually is a feature.  Not everybody is an integrator
who does not want to touch any commit on the topic branch(es) while
rebuilding a single-strand-of-pearls that has many commits and an
occasional merge of the tip of another topic branch.  It's just that
the feature does not suit the workflow I use when I am playing the
top-level integrator role.

I am not sure what should be the ideal behaviour for (2).  I would
imagine that

 - I do want to keep the original title the merge (e.g. "into
   <target branch>", if left to "git merge" to come up with the
   title during "rebase -i" session, would be lost and become "into
   HEAD", which is not what we want);

 - I do want to keep the original commentary in the merge (e.g. what
   you would see in "git log --first-parent master..next" that gives
   summary of each topic getting merged) so that I can update it as
   needed; but 

 - I do want the topic summary fmt-merge-msg produces to be based on
   the updated side branch.

I am not sure if the last item can reliably be filtered out of the
original and replaced with newly generated summary.  If we can do
so, that would be ideal, I guess.

Another observation was that after rebuiding pk/rebase-in-c-6^0 on
top of the updated pk'/rebase-in-c-4 using "rebase -i -r", I of
course still needed to "branch -f" to update pk/rebase-in-c-5,
js/reabse-in-c-5.5, and pk/rebase-in-c-6 branches to point at
appropriate commits.  I do not think it is a good idea to let
"rebase -i" munge these dependent branches by default, but it might
be worth considering it as an option.  Since I want to be more in
control of what happens to the tips of topic branches, I did not
mind at all having to run "branch -f" and having the chance to run
"diff" before doing so, but at the same time, that means doing these
manually in steps building 5 on 4, 5.5 on 5 and then 6 on 5.5,
instead of building 6 on top of 4 using "rebase -i" and then tagging
the intermediate states, gives me more control without forcing me
more work.

I guess that is the answer to a question you asked earlier, which I
haven't answered so far because I didn't have a good grasp of where
my preference was coming from when it was asked.  Now I know, so...


  parent reply	other threads:[~2018-09-06 19:50 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-08 15:21 [GSoC] [PATCH 00/18] builtin rebase options Pratik Karki
2018-08-08 15:21 ` [PATCH 01/18] builtin rebase: allow selecting the rebase "backend" Pratik Karki
2018-08-08 15:21 ` [PATCH 02/18] builtin rebase: support --signoff Pratik Karki
2018-08-08 15:21 ` [PATCH 03/18] builtin rebase: support --rerere-autoupdate Pratik Karki
2018-08-08 15:21 ` [PATCH 04/18] builtin rebase: support --committer-date-is-author-date Pratik Karki
2018-08-08 15:21 ` [PATCH 05/18] builtin rebase: support `ignore-whitespace` option Pratik Karki
2018-08-08 15:21 ` [PATCH 06/18] builtin rebase: support `ignore-date` option Pratik Karki
2018-08-08 15:21 ` [PATCH 07/18] builtin rebase: support `keep-empty` option Pratik Karki
2018-08-24 16:04   ` Johannes Schindelin
2018-08-08 15:21 ` [PATCH 08/18] builtin rebase: support `--autosquash` Pratik Karki
2018-08-08 15:21 ` [PATCH 09/18] builtin rebase: support `--gpg-sign` option Pratik Karki
2018-08-08 15:21 ` [PATCH 10/18] builtin rebase: support `-C` and `--whitespace=<type>` Pratik Karki
2018-08-08 15:21 ` [PATCH 11/18] builtin rebase: support `--autostash` option Pratik Karki
2018-08-18 15:59   ` Duy Nguyen
2018-08-24 16:06     ` Johannes Schindelin
2018-08-08 15:21 ` [PATCH 12/18] builtin rebase: support `--exec` Pratik Karki
2018-08-08 15:21 ` [PATCH 13/18] builtin rebase: support `--allow-empty-message` option Pratik Karki
2018-08-08 15:21 ` [PATCH 14/18] builtin rebase: support --rebase-merges[=[no-]rebase-cousins] Pratik Karki
2018-08-08 15:21 ` [PATCH 15/18] merge-base --fork-point: extract libified function Pratik Karki
2018-08-08 15:21 ` [PATCH 16/18] builtin rebase: support `fork-point` option Pratik Karki
2018-08-08 15:21 ` [PATCH 17/18] builtin rebase: add support for custom merge strategies Pratik Karki
2018-08-08 15:21 ` [PATCH 18/18] builtin rebase: support --root Pratik Karki
2018-09-04 21:59 ` [PATCH v2 00/18] builtin rebase options Johannes Schindelin via GitGitGadget
2018-09-04 21:59   ` [PATCH v2 01/18] builtin rebase: allow selecting the rebase "backend" Pratik Karki via GitGitGadget
2018-09-04 21:59   ` [PATCH v2 02/18] builtin rebase: support --signoff Pratik Karki via GitGitGadget
2018-09-04 21:59   ` [PATCH v2 03/18] builtin rebase: support --rerere-autoupdate Pratik Karki via GitGitGadget
2018-09-04 21:59   ` [PATCH v2 04/18] builtin rebase: support --committer-date-is-author-date Pratik Karki via GitGitGadget
2018-09-04 21:59   ` [PATCH v2 05/18] builtin rebase: support `ignore-whitespace` option Pratik Karki via GitGitGadget
2018-09-04 21:59   ` [PATCH v2 06/18] builtin rebase: support `ignore-date` option Pratik Karki via GitGitGadget
2018-09-04 21:59   ` [PATCH v2 07/18] builtin rebase: support `keep-empty` option Pratik Karki via GitGitGadget
2018-09-04 21:59   ` [PATCH v2 08/18] builtin rebase: support `--autosquash` Pratik Karki via GitGitGadget
2018-09-04 22:00   ` [PATCH v2 09/18] builtin rebase: support `--gpg-sign` option Pratik Karki via GitGitGadget
2018-09-04 22:00   ` [PATCH v2 10/18] builtin rebase: support `-C` and `--whitespace=<type>` Pratik Karki via GitGitGadget
2018-09-04 22:00   ` [PATCH v2 11/18] builtin rebase: support `--autostash` option Pratik Karki via GitGitGadget
2018-09-04 22:00   ` [PATCH v2 12/18] builtin rebase: support `--exec` Pratik Karki via GitGitGadget
2018-09-04 22:00   ` [PATCH v2 13/18] builtin rebase: support `--allow-empty-message` option Pratik Karki via GitGitGadget
2018-09-04 22:00   ` [PATCH v2 14/18] builtin rebase: support --rebase-merges[=[no-]rebase-cousins] Pratik Karki via GitGitGadget
2018-09-04 22:00   ` [PATCH v2 15/18] merge-base --fork-point: extract libified function Pratik Karki via GitGitGadget
2018-09-04 22:00   ` [PATCH v2 16/18] builtin rebase: support `fork-point` option Pratik Karki via GitGitGadget
2018-09-04 22:00   ` [PATCH v2 17/18] builtin rebase: add support for custom merge strategies Pratik Karki via GitGitGadget
2018-09-04 22:00   ` [PATCH v2 18/18] builtin rebase: support --root Pratik Karki via GitGitGadget
2018-09-06 19:50   ` Junio C Hamano [this message]
2018-09-06 20:38     ` [PATCH v2 00/18] builtin rebase options Junio C Hamano
2018-10-12 12:01     ` 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

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

  git send-email \
    --in-reply-to=xmqqmusuz9ql.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    /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).