git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: Warren He <pickydaemon@gmail.com>, johannes.schindelin@gmx.de
Cc: git@vger.kernel.org, wh109@yahoo.com
Subject: Re: [PATCH v2] rebase: introduce --update-branches option
Date: Mon, 9 Sep 2019 11:44:47 +0100	[thread overview]
Message-ID: <31a37eb1-8a75-40f7-7d1c-a8b7b9d75f92@gmail.com> (raw)
In-Reply-To: <20190907234413.1591-1-wh109@yahoo.com>

Hi Warren

On 08/09/2019 00:44, Warren He wrote:
> Everyone in this thread, thanks for your support and encouragement.
> [...]
>> It should not really imply `--interactive`, but `--rebase-merges`.
> 
> `imply_interactive` doesn't fully switch on `--interactive`, i.e., causing the
> editor to open. It only selects the backend, which I think we're saying is the
> right thing. I've dropped the `-i` from the test description.
> 
> And we don't really have to imply --rebase-merges, in case someone would prefer
> to linearize things, which who knows? Running that non-rebase-merges command in
> the example scenario from my original post should give something like this:

I think it would probably be less confusing to default to preserving 
merges and having an option to turn that off - people are going to be 
surprised if their history is linearized.

> ```
>   A - B              (master)
>         \
>           F          (feat-f)
>             \
>               E      (feat-e)
>                 \
>                   H  (my-dev)
> ```
> 
> So for now I haven't moved the implementation into `make_script_with_merges`.
>
 > [...]>
>> [handling `fixup`/`squash` chains]
> 
> I've moved `todo_list_add_branch_updates` to run before
> `todo_list_rearrange_squash`. The rearranging pulls fixups out, causing the
> branch update to "fall" onto the items before, and reinserts them between a
> commit and its branch update, casing them to be included in the updated branch.
> which is my opinion of the right thing to do. I've added a test about this with
> the following scenario:
> 
> ```
>   A - B  (master)
>     \
>       I - J - fixup! I                 (fixup-early)
> 		      \
> 			K - fixup! J  (fixup-late)
> ```
> 
> which results in the following todo list with `--autosquash`:
> 
> ```
> pick 9eadc32 I
> fixup 265fa32 fixup! I
> pick a0754fc J
> fixup e7d1999 fixup! J
> exec git branch -f fixup-early
> pick c8bc4af K
> ```

That makes sense I think

>> I'd like to suggest [`test_cmp_rev`] instead
> 
> I've updated the test to use `test_cmp_rev`. It's not with your suggested
> invocation though. We don't update the `C` tag. I've referred to the rebased `C`
> with `test_cmp_rev linear-early HEAD^` and similar for the other checks.

That sounds right

> * * *
> 
> And then there's the discussion about using `exec git branch -f`. To summarize
> the issues collected from the entire thread:
> 
> 1. the changes aren't atomically applied at the end of the rebase
> 2. it fails when the branch is checked out in a worktree
> 3. it clobbers the branch if anything else updates it during the rebase
> 4. the way we prepare the unprefixed branch doesn't work right some exotic cases
> 5. the reflog message it leaves is uninformative
> 
> For #4, I think we've lucked out actually. The `load_ref_decorations` routine we
> use determines that a ref is `DECORATION_REF_LOCAL` under the condition
> `starts_with(refname, "refs/heads/")` (log-tree.c:114, add_ref_decoration), so
> `prettify_refname` will find the prefix and skip it. But that's an invariant
> maintained by two pieces of code pretty far away from each other.
> 
> For #5, for the convenience of readers, the reflog entry it leaves looks like this:
> 
> ```
> 00873f2 feat-e@{0}: branch: Reset to HEAD
> ```
> 
> Not great.
> 
> I haven't made any changes to this yet, but I've thought about what I want. My
> favorite so far is to add a new todo command that just does everything right. It
> would make a temparary ref `refs/rewritten-heads/xxx` (or something), and update
> `refs/heads/xxx` at the end.

I think that's the best way to do it. If we had a command like 'branch 
<branch-name>' that creates a ref to remember the current HEAD and saves 
the current branch head. Then at the end rebase can update the branches 
to point to the saved commits if the branch is unchanged. If the rebase 
is aborted then we don't end up with some branches updated and others not.

Side Note
   I'd avoid creating another worktree local ref refs/rewritten-heads/.
   Either store them under refs/rewritten/ or refs/worktree/

Best Wishes

Phillip

> 
> I agree that requiring a separate update-ref step at the end of the todo list is
> unfriendly. Manually putting in some branch update commands and then realizing
> that they weren't applied would be extremely frustrating. I don't see the option
> of using existing tools as the easiest-to-use solution.
> 
> I'm reluctant to combine this with the existing `label` command. So far it
> sounds like we generally want to be more willing to skip branch updates while
> performing the rebase, with aforementioned scenarios where something else
> updates the branch before we do, or if the branch becomes checked out in a
> worktree. We don't want to mess up the structure of a `rebase -r` as a result of
> skipping some branch updates. I think it would be conceptually simpler and
> implementation-wise less tricky if we didn't combine it with the `label` and
> `reset` system.
> 
> Warren He (1):
>    rebase: introduce --update-branches option
> 
>   Documentation/git-rebase.txt      |  9 ++++
>   builtin/rebase.c                  | 11 ++++-
>   sequencer.c                       | 58 +++++++++++++++++++++++-
>   sequencer.h                       |  6 ++-
>   t/t3431-rebase-update-branches.sh | 94 +++++++++++++++++++++++++++++++++++++++
>   5 files changed, 173 insertions(+), 5 deletions(-)
>   create mode 100755 t/t3431-rebase-update-branches.sh
> 

  parent reply	other threads:[~2019-09-09 10:44 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-02 23:41 [PATCH] rebase: introduce --update-branches option Warren He
2019-09-02 23:41 ` Warren He
2019-09-03 13:26   ` Johannes Schindelin
2019-09-07 23:44     ` [PATCH v2] " Warren He
2019-09-07 23:44       ` Warren He
2019-09-09 10:44       ` Phillip Wood [this message]
2019-09-09 14:13         ` Johannes Schindelin
2019-09-09 18:11           ` Junio C Hamano
2019-09-23  9:40           ` Phillip Wood
2019-09-03  0:50 ` [PATCH] " brian m. carlson
2019-09-03  1:21   ` Junio C Hamano
2019-09-03  1:39     ` Taylor Blau
2019-09-07 23:41       ` Warren He
2019-09-08 18:04         ` brian m. carlson
2019-09-03 12:19 ` Phillip Wood
2019-09-07 23:43   ` Warren He

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=31a37eb1-8a75-40f7-7d1c-a8b7b9d75f92@gmail.com \
    --to=phillip.wood123@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=pickydaemon@gmail.com \
    --cc=wh109@yahoo.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).