Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Philip Oakley]

From: "Johannes Schindelin" <johannes.schinde...@gmx.de>

On Mon, 23 Apr 2018, Philip Oakley wrote:

From: "Johannes Schindelin" <johannes.schinde...@gmx.de> : Monday, April 
23,

2018 1:03 PM
Subject: Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[...]
>
> > > label onto
> > >
> > > # Branch abc
> > > reset onto
> >
> > Is this reset strictly necessary. We are already there @head.
>
> No, this is not strictly necessary, but

I've realised my misunderstanding. I was thinking this (and others) was
equivalent to

$  git reset <thatHead'onto'> # maybe even --hard,

i.e. affecting the worktree


Oh, but it *is* affecting the worktree. In this case, since we label HEAD
and then immediately reset to the label, there is just nothing to change.

Consider this example, though:

label onto

# Branch: from-philip
reset onto
pick abcdef something
label from-philip

# Branch: with-love
reset onto
pick 012345 else
label with-love

reset onto
merge -C 98765 from-philip
merge -C 43210 with-love

Only in the first instance is the `reset onto` a no-op, an incidental one.
After picking `something` and labeling the result as `from-philip`,
though, the next `reset onto` really resets the worktree.


rather that just being a movement of the Head rev (though I may be having
brain fade here regarding untracked files etc..)


The current way of doing things does not allow the `reset` to overwrite
untracked, nor ignored files (I think, I only verified the former, not the
latter).

But yeah, it is not just a movement of HEAD. It does reset the worktree,
although quite a bit more gently (and safely) than `git reset --hard`. In
that respect, this patch series is a drastic improvement over the Git
garden shears (which is the shell script I use in Git for Windows which
inspired this here patch series).

thanks for clarifying. Yes my reasoning  was a total brain fade ... Along 
with the fact that it's a soft/safe/gentle reset.

--
Philip 

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Martin,

On Tue, 24 Apr 2018, Martin Ågren wrote:

> On 23 April 2018 at 17:54, Phillip Wood  wrote:
> > I'm fine with leaving it, I've might get round to doing a small series to
> > clean things up slightly in a few weeks. At the moment
> > setup_unpack_trees_porcelain() leaks memory as it is called for each merge
> > and allocates new strings each time. It would also be nice if the error
> > messages reflected the command, so it said 'cherry-pick', 'revert' or
> > 'reset' rather than 'merge'
> 
> This is a small patch series to introduce and use
> `clear_unpack_trees_porcelain()`.

Great. Now I have no excuse but must change the sequencer code to output
"reset" instead of "merge" ;-)

Seriously speaking again: thank you for those patches. This is truly
exciting! I mean, we all touch the same code and move it forward, and
somehow it all works out.

Ciao,
Dscho

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Philip,

On Mon, 23 Apr 2018, Philip Oakley wrote:

> From: "Johannes Schindelin" <johannes.schinde...@gmx.de> : Monday, April 23,
> 2018 1:03 PM
> Subject: Re: [PATCH v8 06/16] sequencer: introduce the `merge` command
> 
> [...]
> >
> > > > label onto
> > > >
> > > > # Branch abc
> > > > reset onto
> > >
> > > Is this reset strictly necessary. We are already there @head.
> >
> > No, this is not strictly necessary, but
> 
> I've realised my misunderstanding. I was thinking this (and others) was
> equivalent to
> 
> $  git reset <thatHead'onto'> # maybe even --hard,
> 
> i.e. affecting the worktree

Oh, but it *is* affecting the worktree. In this case, since we label HEAD
and then immediately reset to the label, there is just nothing to change.

Consider this example, though:

label onto

# Branch: from-philip
reset onto
pick abcdef something
label from-philip

# Branch: with-love
reset onto
pick 012345 else
label with-love

reset onto
merge -C 98765 from-philip
merge -C 43210 with-love

Only in the first instance is the `reset onto` a no-op, an incidental one.
After picking `something` and labeling the result as `from-philip`,
though, the next `reset onto` really resets the worktree.

> rather that just being a movement of the Head rev (though I may be having
> brain fade here regarding untracked files etc..)

The current way of doing things does not allow the `reset` to overwrite
untracked, nor ignored files (I think, I only verified the former, not the
latter).

But yeah, it is not just a movement of HEAD. It does reset the worktree,
although quite a bit more gently (and safely) than `git reset --hard`. In
that respect, this patch series is a drastic improvement over the Git
garden shears (which is the shell script I use in Git for Windows which
inspired this here patch series).

Ciao,
Dscho

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Martin Ågren]

Hi Phillip,

On 23 April 2018 at 17:54, Phillip Wood  wrote:
> I'm fine with leaving it, I've might get round to doing a small series to
> clean things up slightly in a few weeks. At the moment
> setup_unpack_trees_porcelain() leaks memory as it is called for each merge
> and allocates new strings each time. It would also be nice if the error
> messages reflected the command, so it said 'cherry-pick', 'revert' or
> 'reset' rather than 'merge'

This is a small patch series to introduce and use
`clear_unpack_trees_porcelain()`. Since Elijah is doing substantial
rewrites to one of the users of `setup_unpack_trees_porcelain()` [1], I
think we should hold off on these for now to avoid a quite evil merge.
(I haven't studied the details enough to be confident, but I think the
calls to `setup_...()` and `clear_...()` might need to be moved up the
call-chain.)

I'm posting this so we don't repeat each other's work. If you feel like
picking these up and running with them, go ahead. Otherwise, I will try
to get them in as soon as Elijah's series lands. I'll keep you posted.

[1] 
https://public-inbox.org/git/can0hesqujbommgay+5xomqxcgohtxxf1mjbmy_l7y+aa4eg...@mail.gmail.com/#t

Martin

Martin Ågren (2):
  merge: setup `opts` later in `checkout_fast_forward()`
  unpack_trees_options: free messages when done

 unpack-trees.h |  5 +
 builtin/checkout.c |  1 +
 merge-recursive.c  |  1 +
 merge.c| 35 ---
 unpack-trees.c | 11 +++
 5 files changed, 38 insertions(+), 15 deletions(-)

-- 
2.17.0

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Philip Oakley]

From: "Johannes Schindelin" <johannes.schinde...@gmx.de> : Monday, April 23, 
2018 1:03 PM

Subject: Re: [PATCH v8 06/16] sequencer: introduce the `merge` command



Hi Philip,


[...]



> label onto
>
> # Branch abc
> reset onto

Is this reset strictly necessary. We are already there @head.


No, this is not strictly necessary, but


I've realised my misunderstanding. I was thinking this (and others) was 
equivalent to


$  git reset <thatHead'onto'> # maybe even --hard,

i.e. affecting the worktree

rather that just being a movement of the Head rev (though I may be having 
brain fade here regarding untracked files etc..)




- it makes it easier to auto-generate (otherwise you would have to keep
 track of the "current HEAD" while generating that todo list, and

- if I keep the `reset onto` there, then it is *a lot* easier to reorder
 topic branches.

Ciao,
Dscho


Thanks

Philip 

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Phillip Wood]

On 23/04/18 13:20, Johannes Schindelin wrote:

Hi Phillip,

On Sat, 21 Apr 2018, Phillip Wood wrote:


On 21/04/18 11:33, Johannes Schindelin wrote:

This patch is part of the effort to reimplement `--preserve-merges` with
a substantially improved design, a design that has been developed in the
Git for Windows project to maintain the dozens of Windows-specific patch
series on top of upstream Git.

The previous patch implemented the `label` and `reset` commands to label
commits and to reset to labeled commits. This patch adds the `merge`
command, with the following syntax:


The two patches seem to have been fused together in this series.


Indeed. I have yet to investigate further how that happened, it could be a
bug in my series after all.


If the reset command fails because it would overwrite untracked files it
says

error: Untracked working tree file 'b' would be overwritten by merge.

Followed by the hint to edit the todo file. Saying 'merge' rather 'reset' is
possibly confusing to users. Perhaps it could call
setup_unpack_trees_porcelain(), though that would need to be extended to
handle 'reset'.


Yes, and it changes global state :-(

Maybe we can leave it as-is for now? After all, it should be clear to the
user what is happening. The most important part is the "Untracked working
tree file"...


I'm fine with leaving it, I've might get round to doing a small series 
to clean things up slightly in a few weeks. At the moment 
setup_unpack_trees_porcelain() leaks memory as it is called for each 
merge and allocates new strings each time. It would also be nice if the 
error messages reflected the command, so it said 'cherry-pick', 'revert' 
or 'reset' rather than 'merge'



Also it currently refuses to overwrite ignored files which is either
annoying or safe depending on one's point of view.


Let me think about that. My gut feeling says: if it is easy to do, then
let's nuke ignored files, but keep untracked files. But I do not think
that the unpack-trees machinery was taught to know about .gitignore...

Seeing as `label` and `reset` really are mostly about revisions we see
along the lines, I think that the common case will *not* overwrite any
untracked files, ever. You would have to use `reset` on a
not-previously-seen commit and/or add `exec` commands designed to
interfere with the `reset`.

So I tend to want to not bother with discerning between untracked and
ignored files here.


I don't think it's a pressing concern. In the past I once had a patch 
series that removed some tracked files in favor of having the build 
system generate them and added them to .gitignore. Each time I rebased I 
had to manually remove them at some stage which was annoying but that is 
quite a rare occurrence.


Best Wishes

Phillip



Ciao,
Dscho

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Phillip,

On Sun, 22 Apr 2018, Phillip Wood wrote:

> On 21/04/18 16:56, Phillip Wood wrote:
> > On 21/04/18 11:33, Johannes Schindelin wrote:
> >> This patch is part of the effort to reimplement `--preserve-merges` with
> >> a substantially improved design, a design that has been developed in the
> >> Git for Windows project to maintain the dozens of Windows-specific patch
> >> series on top of upstream Git.
> >>
> >> The previous patch implemented the `label` and `reset` commands to label
> >> commits and to reset to labeled commits. This patch adds the `merge`
> >> command, with the following syntax:
> > 
> > The two patches seem to have been fused together in this series.
> > 
> > If the reset command fails because it would overwrite untracked files it
> > says
> > 
> > error: Untracked working tree file 'b' would be overwritten by merge.
> > 
> > Followed by the hint to edit the todo file. Saying 'merge' rather
> > 'reset' is possibly confusing to users. Perhaps it could call
> > setup_unpack_trees_porcelain(), though that would need to be extended to
> > handle 'reset'.
> 
> 
> > Also it currently refuses to overwrite ignored files
> > which is either annoying or safe depending on one's point of view.
> 
> Looking at the existing code this is consistent with (most) of the rest
> of the sequencer. The code to fast-forward commits will overwrite
> ignored files, and I think the initial checkout will as well but the
> rest (picking commits and the new merge command) will not.

I never thought about that... but then, I never came close to encountering
such an issue, as I do not typically turn ignored files into tracked ones
;-)

Ciao,
Dscho

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Phillip,

On Sat, 21 Apr 2018, Phillip Wood wrote:

> On 21/04/18 11:33, Johannes Schindelin wrote:
> > This patch is part of the effort to reimplement `--preserve-merges` with
> > a substantially improved design, a design that has been developed in the
> > Git for Windows project to maintain the dozens of Windows-specific patch
> > series on top of upstream Git.
> > 
> > The previous patch implemented the `label` and `reset` commands to label
> > commits and to reset to labeled commits. This patch adds the `merge`
> > command, with the following syntax:
> 
> The two patches seem to have been fused together in this series.

Indeed. I have yet to investigate further how that happened, it could be a
bug in my series after all.

> If the reset command fails because it would overwrite untracked files it
> says
> 
> error: Untracked working tree file 'b' would be overwritten by merge.
> 
> Followed by the hint to edit the todo file. Saying 'merge' rather 'reset' is
> possibly confusing to users. Perhaps it could call
> setup_unpack_trees_porcelain(), though that would need to be extended to
> handle 'reset'.

Yes, and it changes global state :-(

Maybe we can leave it as-is for now? After all, it should be clear to the
user what is happening. The most important part is the "Untracked working
tree file"...

> Also it currently refuses to overwrite ignored files which is either
> annoying or safe depending on one's point of view.

Let me think about that. My gut feeling says: if it is easy to do, then
let's nuke ignored files, but keep untracked files. But I do not think
that the unpack-trees machinery was taught to know about .gitignore...

Seeing as `label` and `reset` really are mostly about revisions we see
along the lines, I think that the common case will *not* overwrite any
untracked files, ever. You would have to use `reset` on a
not-previously-seen commit and/or add `exec` commands designed to
interfere with the `reset`.

So I tend to want to not bother with discerning between untracked and
ignored files here.

Ciao,
Dscho

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Philip,

On Sun, 22 Apr 2018, Philip Oakley wrote:

> From: "Johannes Schindelin" 
> > This patch is part of the effort to reimplement `--preserve-merges` with
> > a substantially improved design, a design that has been developed in the
> > Git for Windows project to maintain the dozens of Windows-specific patch
> > series on top of upstream Git.
> >
> > The previous patch implemented the `label` and `reset` commands to label
> 
> The previous patch was [Patch 05/16] git-rebase--interactive: clarify
> arguments, so this statement doesn't appear to be true. Has a patch been
> missed or re-ordered? Or should it be simply "This patch implements" ?
> Likewise the patch subject would be updated.

As Phillip guessed correctly, it was a mistaken `git commit --amend`.

> > commits and to reset to labeled commits. This patch adds the `merge`
> 
> s/adds/also adds/ ?

No, as I really want to keep those two commits separate. I disentangled
them.

> > command, with the following syntax:
> >
> > merge [-C ]  # 
> >
> > The  parameter in this instance is the *original* merge commit,
> > whose author and message will be used for the merge commit that is about
> > to be created.
> >
> > The  parameter refers to the (possibly rewritten) revision to
> > merge. Let's see an example of a todo list:
> >
> The example ought to also note that `label onto` is to
> `# label current HEAD with a name`, seeing as this is the first occurance.
> It may be obvious in retrospect, but not at first reading.

I added some sentence to describe what `label onto` does and why.

> > label onto
> >
> > # Branch abc
> > reset onto
> 
> Is this reset strictly necessary. We are already there @head.

No, this is not strictly necessary, but

- it makes it easier to auto-generate (otherwise you would have to keep
  track of the "current HEAD" while generating that todo list, and

- if I keep the `reset onto` there, then it is *a lot* easier to reorder
  topic branches.

Ciao,
Dscho

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Phillip Wood]

On 21/04/18 16:56, Phillip Wood wrote:
> On 21/04/18 11:33, Johannes Schindelin wrote:
>> This patch is part of the effort to reimplement `--preserve-merges` with
>> a substantially improved design, a design that has been developed in the
>> Git for Windows project to maintain the dozens of Windows-specific patch
>> series on top of upstream Git.
>>
>> The previous patch implemented the `label` and `reset` commands to label
>> commits and to reset to labeled commits. This patch adds the `merge`
>> command, with the following syntax:
> 
> The two patches seem to have been fused together in this series.
> 
> If the reset command fails because it would overwrite untracked files it
> says
> 
> error: Untracked working tree file 'b' would be overwritten by merge.
> 
> Followed by the hint to edit the todo file. Saying 'merge' rather
> 'reset' is possibly confusing to users. Perhaps it could call
> setup_unpack_trees_porcelain(), though that would need to be extended to
> handle 'reset'.


> Also it currently refuses to overwrite ignored files
> which is either annoying or safe depending on one's point of view.

Looking at the existing code this is consistent with (most) of the rest
of the sequencer. The code to fast-forward commits will overwrite
ignored files, and I think the initial checkout will as well but the
rest (picking commits and the new merge command) will not.

> Best Wishes
> 
> Phillip
> 
>>
>> merge [-C ]  # 
>>
>> The  parameter in this instance is the *original* merge commit,
>> whose author and message will be used for the merge commit that is about
>> to be created.
>>
>> The  parameter refers to the (possibly rewritten) revision to
>> merge. Let's see an example of a todo list:
>>
>> label onto
>>
>> # Branch abc
>> reset onto
>> pick deadbeef Hello, world!
>> label abc
>>
>> reset onto
>> pick cafecafe And now for something completely different
>> merge -C baaabaaa abc # Merge the branch 'abc' into master
>>
>> To edit the merge commit's message (a "reword" for merges, if you will),
>> use `-c` (lower-case) instead of `-C`; this convention was borrowed from
>> `git commit` that also supports `-c` and `-C` with similar meanings.
>>
>> To create *new* merges, i.e. without copying the commit message from an
>> existing commit, simply omit the `-C ` parameter (which will
>> open an editor for the merge message):
>>
>> merge abc
>>
>> This comes in handy when splitting a branch into two or more branches.
>>
>> Note: this patch only adds support for recursive merges, to keep things
>> simple. Support for octopus merges will be added later in a separate
>> patch series, support for merges using strategies other than the
>> recursive merge is left for the future.
>>
>> Signed-off-by: Johannes Schindelin 
>> ---
>>   git-rebase--interactive.sh |   6 +
>>   sequencer.c    | 407 -
>>   2 files changed, 406 insertions(+), 7 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index e1b865f43f2..ccd5254d1c9 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -162,6 +162,12 @@ s, squash  = use commit, but meld into
>> previous commit
>>   f, fixup  = like \"squash\", but discard this commit's log
>> message
>>   x, exec  = run command (the rest of the line) using shell
>>   d, drop  = remove commit
>> +l, label  = label current HEAD with a name
>> +t, reset  = reset HEAD to a label
>> +m, merge [-C  | -c ]  [# ]
>> +.   create a merge commit using the original merge commit's
>> +.   message (or the oneline, if no original merge commit was
>> +.   specified). Use -c  to reword the commit message.
>>     These lines can be re-ordered; they are executed from top to bottom.
>>   " | git stripspace --comment-lines >>"$todo"
>> diff --git a/sequencer.c b/sequencer.c
>> index 01443e0f245..35fcacbdf0f 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -23,6 +23,8 @@
>>   #include "hashmap.h"
>>   #include "notes-utils.h"
>>   #include "sigchain.h"
>> +#include "unpack-trees.h"
>> +#include "worktree.h"
>>     #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>>   @@ -120,6 +122,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha,
>> "rebase-merge/stopped-sha")
>>   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 listing refs that need to be deleted after
>> the rebase
>> + * finishes. This is used by the `label` command to record the need
>> for cleanup.
>> + */
>> +static GIT_PATH_FUNC(rebase_path_refs_to_delete,
>> "rebase-merge/refs-to-delete")
>> +
>>   /*
>>    * The following files are written by git-rebase just after parsing the
>>    * command-line (and are only consumed, not modified, by the
>> sequencer).
>> @@ -244,18 +253,34 @@ 

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Philip Oakley]

From: "Johannes Schindelin" 

This patch is part of the effort to reimplement `--preserve-merges` with
a substantially improved design, a design that has been developed in the
Git for Windows project to maintain the dozens of Windows-specific patch
series on top of upstream Git.

The previous patch implemented the `label` and `reset` commands to label


The previous patch was [Patch 05/16] git-rebase--interactive: clarify 
arguments, so this statement doesn't appear to be true. Has a patch been 
missed or re-ordered? Or should it be simply "This patch implements" ? 
Likewise the patch subject would be updated.



commits and to reset to labeled commits. This patch adds the `merge`


s/adds/also adds/ ?


command, with the following syntax:

merge [-C ]  # 

The  parameter in this instance is the *original* merge commit,
whose author and message will be used for the merge commit that is about
to be created.

The  parameter refers to the (possibly rewritten) revision to
merge. Let's see an example of a todo list:


The example ought to also note that `label onto` is to
`# label current HEAD with a name`, seeing as this is the first occurance.
It may be obvious in retrospect, but not at first reading.


label onto

# Branch abc
reset onto


Is this reset strictly necessary. We are already there @head.


pick deadbeef Hello, world!
label abc

reset onto
pick cafecafe And now for something completely different
merge -C baaabaaa abc # Merge the branch 'abc' into master

To edit the merge commit's message (a "reword" for merges, if you will),
use `-c` (lower-case) instead of `-C`; this convention was borrowed from
`git commit` that also supports `-c` and `-C` with similar meanings.

To create *new* merges, i.e. without copying the commit message from an
existing commit, simply omit the `-C ` parameter (which will
open an editor for the merge message):

merge abc

This comes in handy when splitting a branch into two or more branches.

Note: this patch only adds support for recursive merges, to keep things
simple. Support for octopus merges will be added later in a separate
patch series, support for merges using strategies other than the
recursive merge is left for the future.

Signed-off-by: Johannes Schindelin 
---
git-rebase--interactive.sh |   6 +
sequencer.c| 407 -
2 files changed, 406 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e1b865f43f2..ccd5254d1c9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -162,6 +162,12 @@ s, squash  = use commit, but meld into 
previous commit

f, fixup  = like \"squash\", but discard this commit's log message
x, exec  = run command (the rest of the line) using shell
d, drop  = remove commit
+l, label  = label current HEAD with a name
+t, reset  = reset HEAD to a label
+m, merge [-C  | -c ]  [# ]
+.   create a merge commit using the original merge commit's
+.   message (or the oneline, if no original merge commit was
+.   specified). Use -c  to reword the commit message.

These lines can be re-ordered; they are executed from top to bottom.
" | git stripspace --comment-lines >>"$todo"
diff --git a/sequencer.c b/sequencer.c
index 01443e0f245..35fcacbdf0f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -23,6 +23,8 @@
#include "hashmap.h"
#include "notes-utils.h"
#include "sigchain.h"
+#include "unpack-trees.h"
+#include "worktree.h"

#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"

@@ -120,6 +122,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, 
"rebase-merge/stopped-sha")
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 listing refs that need to be deleted after the 
rebase
+ * finishes. This is used by the `label` command to record the need for 
cleanup.

+ */
+static GIT_PATH_FUNC(rebase_path_refs_to_delete, 
"rebase-merge/refs-to-delete")

+
/*
 * The following files are written by git-rebase just after parsing the
 * command-line (and are only consumed, not modified, by the sequencer).
@@ -244,18 +253,34 @@ static const char *gpg_sign_opt_quoted(struct 
replay_opts *opts)


int sequencer_remove_state(struct replay_opts *opts)
{
- struct strbuf dir = STRBUF_INIT;
+ struct strbuf buf = STRBUF_INIT;
 int i;

+ if (is_rebase_i(opts) &&
+ strbuf_read_file(, rebase_path_refs_to_delete(), 0) > 0) {
+ char *p = buf.buf;
+ while (*p) {
+ char *eol = strchr(p, '\n');
+ if (eol)
+ *eol = '\0';
+ if (delete_ref("(rebase -i) cleanup", p, NULL, 0) < 0)
+ warning(_("could not delete '%s'"), p);
+ if (!eol)
+ break;
+ p = eol + 1;
+ }
+ }
+
 free(opts->gpg_sign);
 free(opts->strategy);
 for (i = 0; i < opts->xopts_nr; i++)
 free(opts->xopts[i]);
 free(opts->xopts);

- strbuf_addstr(, get_dir(opts));
- remove_dir_recursively(, 0);
- 

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Philip Oakley]

From: "Johannes Schindelin" 

This patch is part of the effort to reimplement `--preserve-merges` with
a substantially improved design, a design that has been developed in the
Git for Windows project to maintain the dozens of Windows-specific patch
series on top of upstream Git.

The previous patch implemented the `label` and `reset` commands to label


The previous patch was [Patch 05/16] git-rebase--interactive: clarify
arguments, so this statement doesn't appear to be true. Has a patch been
missed or re-ordered? Or should it be simply "This patch implements" ?
Likewise the patch subject would be updated.


commits and to reset to labeled commits. This patch adds the `merge`


s/adds/also adds/ ?


command, with the following syntax:

merge [-C ]  # 

The  parameter in this instance is the *original* merge commit,
whose author and message will be used for the merge commit that is about
to be created.

The  parameter refers to the (possibly rewritten) revision to
merge. Let's see an example of a todo list:


The example ought to also note that `label onto` is to
`# label current HEAD with a name`, seeing as this is the first occurance.
It may be obvious in retrospect, but not at first reading.


label onto

# Branch abc
reset onto


Is this reset strictly necessary. We are already there @head.


pick deadbeef Hello, world!
label abc

reset onto
pick cafecafe And now for something completely different
merge -C baaabaaa abc # Merge the branch 'abc' into master

To edit the merge commit's message (a "reword" for merges, if you will),
use `-c` (lower-case) instead of `-C`; this convention was borrowed from
`git commit` that also supports `-c` and `-C` with similar meanings.

To create *new* merges, i.e. without copying the commit message from an
existing commit, simply omit the `-C ` parameter (which will
open an editor for the merge message):

merge abc

This comes in handy when splitting a branch into two or more branches.

Note: this patch only adds support for recursive merges, to keep things
simple. Support for octopus merges will be added later in a separate
patch series, support for merges using strategies other than the
recursive merge is left for the future.

Signed-off-by: Johannes Schindelin 
---
git-rebase--interactive.sh |   6 +
sequencer.c| 407 -
2 files changed, 406 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e1b865f43f2..ccd5254d1c9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -162,6 +162,12 @@ s, squash  = use commit, but meld into
previous commit
f, fixup  = like \"squash\", but discard this commit's log message
x, exec  = run command (the rest of the line) using shell
d, drop  = remove commit
+l, label  = label current HEAD with a name
+t, reset  = reset HEAD to a label
+m, merge [-C  | -c ]  [# ]
+.   create a merge commit using the original merge commit's
+.   message (or the oneline, if no original merge commit was
+.   specified). Use -c  to reword the commit message.

These lines can be re-ordered; they are executed from top to bottom.
" | git stripspace --comment-lines >>"$todo"
diff --git a/sequencer.c b/sequencer.c
index 01443e0f245..35fcacbdf0f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -23,6 +23,8 @@
#include "hashmap.h"
#include "notes-utils.h"
#include "sigchain.h"
+#include "unpack-trees.h"
+#include "worktree.h"

#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"

@@ -120,6 +122,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha,
"rebase-merge/stopped-sha")
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 listing refs that need to be deleted after the
rebase
+ * finishes. This is used by the `label` command to record the need for
cleanup.
+ */
+static GIT_PATH_FUNC(rebase_path_refs_to_delete,
"rebase-merge/refs-to-delete")
+
/*
 * The following files are written by git-rebase just after parsing the
 * command-line (and are only consumed, not modified, by the sequencer).
@@ -244,18 +253,34 @@ static const char *gpg_sign_opt_quoted(struct
replay_opts *opts)

int sequencer_remove_state(struct replay_opts *opts)
{
- struct strbuf dir = STRBUF_INIT;
+ struct strbuf buf = STRBUF_INIT;
 int i;

+ if (is_rebase_i(opts) &&
+ strbuf_read_file(, rebase_path_refs_to_delete(), 0) > 0) {
+ char *p = buf.buf;
+ while (*p) {
+ char *eol = strchr(p, '\n');
+ if (eol)
+ *eol = '\0';
+ if (delete_ref("(rebase -i) cleanup", p, NULL, 0) < 0)
+ warning(_("could not delete '%s'"), p);
+ if (!eol)
+ break;
+ p = eol + 1;
+ }
+ }
+
 free(opts->gpg_sign);
 free(opts->strategy);
 for (i = 0; i < opts->xopts_nr; i++)
 free(opts->xopts[i]);
 free(opts->xopts);

- strbuf_addstr(, get_dir(opts));
- remove_dir_recursively(, 0);
- strbuf_release();
+ 

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Phillip Wood]

On 21/04/18 11:33, Johannes Schindelin wrote:

This patch is part of the effort to reimplement `--preserve-merges` with
a substantially improved design, a design that has been developed in the
Git for Windows project to maintain the dozens of Windows-specific patch
series on top of upstream Git.

The previous patch implemented the `label` and `reset` commands to label
commits and to reset to labeled commits. This patch adds the `merge`
command, with the following syntax:


The two patches seem to have been fused together in this series.

If the reset command fails because it would overwrite untracked files it 
says


error: Untracked working tree file 'b' would be overwritten by merge.

Followed by the hint to edit the todo file. Saying 'merge' rather 
'reset' is possibly confusing to users. Perhaps it could call 
setup_unpack_trees_porcelain(), though that would need to be extended to 
handle 'reset'. Also it currently refuses to overwrite ignored files 
which is either annoying or safe depending on one's point of view.


Best Wishes

Phillip



merge [-C ]  # 

The  parameter in this instance is the *original* merge commit,
whose author and message will be used for the merge commit that is about
to be created.

The  parameter refers to the (possibly rewritten) revision to
merge. Let's see an example of a todo list:

label onto

# Branch abc
reset onto
pick deadbeef Hello, world!
label abc

reset onto
pick cafecafe And now for something completely different
merge -C baaabaaa abc # Merge the branch 'abc' into master

To edit the merge commit's message (a "reword" for merges, if you will),
use `-c` (lower-case) instead of `-C`; this convention was borrowed from
`git commit` that also supports `-c` and `-C` with similar meanings.

To create *new* merges, i.e. without copying the commit message from an
existing commit, simply omit the `-C ` parameter (which will
open an editor for the merge message):

merge abc

This comes in handy when splitting a branch into two or more branches.

Note: this patch only adds support for recursive merges, to keep things
simple. Support for octopus merges will be added later in a separate
patch series, support for merges using strategies other than the
recursive merge is left for the future.

Signed-off-by: Johannes Schindelin 
---
  git-rebase--interactive.sh |   6 +
  sequencer.c| 407 -
  2 files changed, 406 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e1b865f43f2..ccd5254d1c9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -162,6 +162,12 @@ s, squash  = use commit, but meld into previous 
commit
  f, fixup  = like \"squash\", but discard this commit's log message
  x, exec  = run command (the rest of the line) using shell
  d, drop  = remove commit
+l, label  = label current HEAD with a name
+t, reset  = reset HEAD to a label
+m, merge [-C  | -c ]  [# ]
+.   create a merge commit using the original merge commit's
+.   message (or the oneline, if no original merge commit was
+.   specified). Use -c  to reword the commit message.
  
  These lines can be re-ordered; they are executed from top to bottom.

  " | git stripspace --comment-lines >>"$todo"
diff --git a/sequencer.c b/sequencer.c
index 01443e0f245..35fcacbdf0f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -23,6 +23,8 @@
  #include "hashmap.h"
  #include "notes-utils.h"
  #include "sigchain.h"
+#include "unpack-trees.h"
+#include "worktree.h"
  
  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
  
@@ -120,6 +122,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")

  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 listing refs that need to be deleted after the rebase
+ * finishes. This is used by the `label` command to record the need for 
cleanup.
+ */
+static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
+
  /*
   * The following files are written by git-rebase just after parsing the
   * command-line (and are only consumed, not modified, by the sequencer).
@@ -244,18 +253,34 @@ static const char *gpg_sign_opt_quoted(struct replay_opts 
*opts)
  
  int sequencer_remove_state(struct replay_opts *opts)

  {
-   struct strbuf dir = STRBUF_INIT;
+   struct strbuf buf = STRBUF_INIT;
int i;
  
+	if (is_rebase_i(opts) &&

+   strbuf_read_file(, rebase_path_refs_to_delete(), 0) > 0) {
+   char *p = buf.buf;
+   while (*p) {
+   char *eol = strchr(p, '\n');
+   if (eol)
+   *eol = '\0';
+   if (delete_ref("(rebase -i) 

[PATCH v8 06/16] sequencer: introduce the `merge` command

[Johannes Schindelin]

This patch is part of the effort to reimplement `--preserve-merges` with
a substantially improved design, a design that has been developed in the
Git for Windows project to maintain the dozens of Windows-specific patch
series on top of upstream Git.

The previous patch implemented the `label` and `reset` commands to label
commits and to reset to labeled commits. This patch adds the `merge`
command, with the following syntax:

merge [-C ]  # 

The  parameter in this instance is the *original* merge commit,
whose author and message will be used for the merge commit that is about
to be created.

The  parameter refers to the (possibly rewritten) revision to
merge. Let's see an example of a todo list:

label onto

# Branch abc
reset onto
pick deadbeef Hello, world!
label abc

reset onto
pick cafecafe And now for something completely different
merge -C baaabaaa abc # Merge the branch 'abc' into master

To edit the merge commit's message (a "reword" for merges, if you will),
use `-c` (lower-case) instead of `-C`; this convention was borrowed from
`git commit` that also supports `-c` and `-C` with similar meanings.

To create *new* merges, i.e. without copying the commit message from an
existing commit, simply omit the `-C ` parameter (which will
open an editor for the merge message):

merge abc

This comes in handy when splitting a branch into two or more branches.

Note: this patch only adds support for recursive merges, to keep things
simple. Support for octopus merges will be added later in a separate
patch series, support for merges using strategies other than the
recursive merge is left for the future.

Signed-off-by: Johannes Schindelin 
---
 git-rebase--interactive.sh |   6 +
 sequencer.c| 407 -
 2 files changed, 406 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e1b865f43f2..ccd5254d1c9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -162,6 +162,12 @@ s, squash  = use commit, but meld into previous 
commit
 f, fixup  = like \"squash\", but discard this commit's log message
 x, exec  = run command (the rest of the line) using shell
 d, drop  = remove commit
+l, label  = label current HEAD with a name
+t, reset  = reset HEAD to a label
+m, merge [-C  | -c ]  [# ]
+.   create a merge commit using the original merge commit's
+.   message (or the oneline, if no original merge commit was
+.   specified). Use -c  to reword the commit message.
 
 These lines can be re-ordered; they are executed from top to bottom.
 " | git stripspace --comment-lines >>"$todo"
diff --git a/sequencer.c b/sequencer.c
index 01443e0f245..35fcacbdf0f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -23,6 +23,8 @@
 #include "hashmap.h"
 #include "notes-utils.h"
 #include "sigchain.h"
+#include "unpack-trees.h"
+#include "worktree.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -120,6 +122,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, 
"rebase-merge/stopped-sha")
 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 listing refs that need to be deleted after the rebase
+ * finishes. This is used by the `label` command to record the need for 
cleanup.
+ */
+static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
+
 /*
  * The following files are written by git-rebase just after parsing the
  * command-line (and are only consumed, not modified, by the sequencer).
@@ -244,18 +253,34 @@ static const char *gpg_sign_opt_quoted(struct replay_opts 
*opts)
 
 int sequencer_remove_state(struct replay_opts *opts)
 {
-   struct strbuf dir = STRBUF_INIT;
+   struct strbuf buf = STRBUF_INIT;
int i;
 
+   if (is_rebase_i(opts) &&
+   strbuf_read_file(, rebase_path_refs_to_delete(), 0) > 0) {
+   char *p = buf.buf;
+   while (*p) {
+   char *eol = strchr(p, '\n');
+   if (eol)
+   *eol = '\0';
+   if (delete_ref("(rebase -i) cleanup", p, NULL, 0) < 0)
+   warning(_("could not delete '%s'"), p);
+   if (!eol)
+   break;
+   p = eol + 1;
+   }
+   }
+
free(opts->gpg_sign);
free(opts->strategy);
for (i = 0; i < opts->xopts_nr; i++)
free(opts->xopts[i]);
free(opts->xopts);
 
-   strbuf_addstr(, get_dir(opts));
-   remove_dir_recursively(, 0);
-   strbuf_release();
+   strbuf_reset();
+   strbuf_addstr(, get_dir(opts));
+   remove_dir_recursively(, 0);
+   strbuf_release();
 
return 0;
 }