From: Phillip Wood <phillip.wood@talktalk.net>
Date: Fri, 13 Apr 2018 15:27:18 GMT
Subject: Re: [PATCH v6 15/15] rebase -i --rebase-merges: add a section to the man page
Message-ID: <5c968cfe-b897-e308-d41a-9948529b796f@talktalk.net>
In-Reply-To: <nycvar.QRO.7.76.6.1804121102260.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>

On 12/04/18 10:30, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Wed, 11 Apr 2018, Phillip Wood wrote:
> 
>> On 10/04/18 13:30, Johannes Schindelin wrote:
>>
>> Firstly let me say that I think expanding the documentation and having an
>> example is an excellent idea.
> 
> Thanks! At first, I meant to leave this for others to contribute, but I
> think it makes sense for me to describe it, as I do have a little bit of
> experience with rebasing merges.
> 
>>> +
>>> +------------
>>> +label onto
>>> +
>>> +# Branch: refactor-button
>>> +reset onto
>>> +pick 123456 Extract a generic Button class from the DownloadButton one
>>> +pick 654321 Use the Button class for all buttons
>>> +label refactor-button
>>> +
>>> +# Branch: report-a-bug
>>> +reset refactor-button # Use the Button class for all buttons
>>> +pick abcdef Add the feedback button
>>> +label report-a-bug
>>> +
>>> +reset onto
>>> +merge -C a1b2c3 refactor-button # Merge 'refactor-button'
>>> +merge -C 6f5e4d report-a-bug # Merge 'report-a-bug'
>>> +------------
>>> +
>>> +In contrast to a regular interactive rebase, there are `label`, `reset` and
>>> +`merge` commands in addition to `pick` ones.
>>> +
>>> +The `label` command puts a label to whatever will be the current
>>
>> s/puts a label to/associates a label with/ would be clearer I think. Maybe
>> s/whatever will be the current revision/the current HEAD/ an well?
> 
> Thanks, I incorporated both changes here.
> 
>>> +revision when that command is executed. Internally, these labels are
>>> +worktree-local refs that will be deleted when the rebase finishes or
>>> +when it is aborted.
>>
>> I agree they should be deleted when the rebase is aborted but I cannot see any
>> changes to git-rebase.sh to make that happen. I think they should also be
>> deleted by 'rebase --quit'.
> 
> Oh right! For some reason I thought I already hooked up rebase--helper
> --abort when rebase was called with --abort or quit, but I had not managed
> yet. I think I will leave this for later, or for GSoC, or something.
> 
> In the meantime, I'll just drop the "or when it is aborted.".
> 
>>> That way, rebase operations in multiple worktrees
>>> +linked to the same repository do not interfere with one another.
>>> +
>>> +The `reset` command is essentially a `git reset --hard` to the specified
>>> +revision (typically a previously-labeled one).
>>
>> s/labeled/labelled/
> 
> As Eric pointed out, I am using 'murricane spelling here (or is it
> speling? Ya never know these days).

:-)

>> I think it would be worthwhile to point out that unlike the other commands
>> this will not preserve untracked files. Maybe something like
>> "Note that unlike the `pick` or `merge` commands or initial checkout when the
>> rebase starts the `reset` command will overwrite any untracked files."
> 
> You know what? You just pointed out a bug in my thinking. Previously, I
> thought that this is impossible, that you cannot overwrite untracked files
> because we labeled this revision previously, so the only new files to
> write by `reset` were tracked files previous. But that forgets `exec` and
> `reset` with unlabeled revisions (e.g. for cousins).
> 
> So I changed the `reset` command to refuse overwriting untracked files...

That sounds like the safest plan

Thanks

Phillip
> 
> Thank you for improving this patch series!
> Dscho
>