Re: [PATCH v6 04/15] sequencer: introduce new commands to reset the revision

[Phillip Wood]

On 15/04/18 18:17, Philip Oakley wrote:
> From: "Phillip Wood" 
> : Friday, April 13, 2018 11:03 AM
>> If a label or reset command fails it is likely to be due to a
>> typo. Rescheduling the command would make it easier for the user to fix
>> the problem as they can just run 'git rebase --edit-todo'. 
> 
> Is this worth noting in the command documentation? "If the label or
> reset command fails then fix
> the problem by runnning 'git rebase --edit-todo'." ?
> 
> Just a thought.

Yes that's a good idea, thanks

>> It also
>> ensures that the problem has actually been fixed when the rebase
>> continues. I think you could do it like this
>>
> 
> -- 
> Philip
> (also @dunelm, 73-79..)
That's a bit before me (94-00) were you there when they were building
the hill colleges and some of the science site?

Best Wishes

Phillip

Re: [PATCH v6 04/15] sequencer: introduce new commands to reset the revision

[Philip Oakley]

From: "Phillip Wood" 
: Friday, April 13, 2018 11:03 AM

If a label or reset command fails it is likely to be due to a
typo. Rescheduling the command would make it easier for the user to fix
the problem as they can just run 'git rebase --edit-todo'. 


Is this worth noting in the command documentation? 
"If the label or reset command fails then fix

the problem by runnning 'git rebase --edit-todo'." ?

Just a thought.


It also
ensures that the problem has actually been fixed when the rebase
continues. I think you could do it like this



--
Philip
(also @dunelm, 73-79..)

Re: [PATCH v6 04/15] sequencer: introduce new commands to reset the revision

[Phillip Wood]

On 10/04/18 13:29, Johannes Schindelin wrote:
> In the upcoming commits, we will teach the sequencer to rebase merges.
> This will be done in a very different way from the unfortunate design of
> `git rebase --preserve-merges` (which does not allow for reordering
> commits, or changing the branch topology).
> 
> The main idea is to introduce new todo list commands, to support
> labeling the current revision with a given name, resetting the current
> revision to a previous state, and  merging labeled revisions.
> 
> This idea was developed in Git for Windows' Git garden shears (that are
> used to maintain Git for Windows' "thicket of branches" on top of
> upstream Git), and this patch is part of the effort to make it available
> to a wider audience, as well as to make the entire process more robust
> (by implementing it in a safe and portable language rather than a Unix
> shell script).
> 
> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
> 
>   label 
>   reset 
> 
> Internally, the `label ` command creates the ref
> `refs/rewritten/`. This makes it possible to work with the labeled
> revisions interactively, or in a scripted fashion (e.g. via the todo
> list command `exec`).
> 
> These temporary refs are removed upon sequencer_remove_state(), so that
> even a `git rebase --abort` cleans them up.
> 
> We disallow '#' as label because that character will be used as separator
> in the upcoming `merge` command.
> 
> Later in this patch series, we will mark the `refs/rewritten/` refs as
> worktree-local, to allow for interactive rebases to be run in parallel in
> worktrees linked to the same repository.
> 
> Signed-off-by: Johannes Schindelin 

If a label or reset command fails it is likely to be due to a
typo. Rescheduling the command would make it easier for the user to fix
the problem as they can just run 'git rebase --edit-todo'. It also
ensures that the problem has actually been fixed when the rebase
continues. I think you could do it like this

--->8---
From: Phillip Wood 
Subject: [PATCH] fixup! sequencer: introduce new commands to reset the revision

Signed-off-by: Phillip Wood 
---
 sequencer.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 809df1ce48..e1b9be7327 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3029,6 +3029,13 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
} else if (!is_noop(item->command))
return error(_("unknown command %d"), item->command);
 
+   if (res < 0 && (item->command == TODO_LABEL ||
+   item->command == TODO_RESET)) {
+   /* Reschedule */
+   todo_list->current--;
+   save_todo(todo_list, opts);
+   return res;
+   }
todo_list->current++;
if (res)
return res;
-- 
2.17.0

Re: [PATCH v6 04/15] sequencer: introduce new commands to reset the revision

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  writes:

> Hi Sergey,
>
> On Wed, 11 Apr 2018, Sergey Organov wrote:
>
>> Johannes Schindelin  writes:
>> 
>> [...]
>> 
>> > We disallow '#' as label because that character will be used as
>> > separator in the upcoming `merge` command.
>> 
>> Please consider to use # not only in `merge` and `reset`, but in the
>> rest of the commands as well, to unify this new syntax. I.e., right now
>> it seems to be:
>> 
>> pick  abcd A commit message
>> merge beaf # B commit message
>> 
>> I suggest to turn it to:
>> 
>> pick  abcd # A commit message
>> merge beaf # B commit message
>
> First of all, that alignment of pick's and merge's first arguments?

As if it has anything to do with the topic of the issue!

Just a nice look. Let it be:

pick abcd # A commit message
merge beaf # B commit message

if it's that essential indeed.

> That does not exist. If you want aligned arguments, you have to use the
> rebase.abbreviateCommands feature.

It's changing the subject.

> Second: this change would break backwards-compatibility. For almost eleven
> years, we generated `pick abcdef0123 A commit message`.

I thought we already agreed that you have no backward compatibility
issues with this new feature, as it's a new feature, complete re-design,
as you put it yourself:

"This design flaw cannot be fixed. Not without a complete re-design, at
least. This patch series offers such a re-design."

At least could you please answer plain yes/no to this simple question: is
this feature a complete re-design or not? yes/no, please!

> Even if there are no scripts that rely on this form, power users have
> gotten used to it, and I can tell you from experience how unsettling
> even minor visual changes are in everyday operations.
> In short: no, we cannot do that.

You can do that, provided it's complete re-design indeed. You don't wish
to, but you can. Nothing will break and things will be at least a little
bit cleaner.

Each directive having its own dedicated syntax... gosh! No luck getting
syntax description, I'm afraid.

> Just like your proposal to conflate the `merge` and `pick` commands

There was never such proposal. The proposal was not to introduce new
`merge` command when there is already `pick` that could simply be
extended to pick any commit, whatever number of parents it happens to
have.

But provided you decline to even put a # before the commit message...
that proposal is simply a pie in the sky.

> for some perception of consistency: The user experience is more
> important than individual persons' sense of elegance (that might not
> even be shared with the majority).

It's about consistency indeed. Consistent handling of commits is
essential. Consistency is one of the things that bring positive user
experience. You disagree?

Besides, it was bad user experience that forced you to re-design, isn't
it? I'm afraid you miss good opportunity to fix some of your former
mistakes and you make some new. As the discussion goes, it seems you'd
never admit it, the design is set in stone, and my attempts are in fact
pointless.

Overall, I hereby withdraw all my pending suggestions to improve this
patch series.

-- Sergey

Re: [PATCH v6 04/15] sequencer: introduce new commands to reset the revision

[Johannes Schindelin]

Hi Sergey,

On Wed, 11 Apr 2018, Sergey Organov wrote:

> Johannes Schindelin  writes:
> 
> [...]
> 
> > We disallow '#' as label because that character will be used as
> > separator in the upcoming `merge` command.
> 
> Please consider to use # not only in `merge` and `reset`, but in the
> rest of the commands as well, to unify this new syntax. I.e., right now
> it seems to be:
> 
> pick  abcd A commit message
> merge beaf # B commit message
> 
> I suggest to turn it to:
> 
> pick  abcd # A commit message
> merge beaf # B commit message

First of all, that alignment of pick's and merge's first arguments? That
does not exist. If you want aligned arguments, you have to use the
rebase.abbreviateCommands feature.

Second: this change would break backwards-compatibility. For almost eleven
years, we generated `pick abcdef0123 A commit message`. Even if there are
no scripts that rely on this form, power users have gotten used to it, and
I can tell you from experience how unsettling even minor visual changes
are in everyday operations.

In short: no, we cannot do that. Just like your proposal to conflate the
`merge` and `pick` commands for some perception of consistency: The user
experience is more important than individual persons' sense of elegance
(that might not even be shared with the majority).

Ciao,
Johannes

Re: [PATCH v6 04/15] sequencer: introduce new commands to reset the revision

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  writes:

[...]

> We disallow '#' as label because that character will be used as separator
> in the upcoming `merge` command.

Please consider to use # not only in `merge` and `reset`, but in the rest
of the commands as well, to unify this new syntax. I.e., right now it
seems to be:

pick  abcd A commit message
merge beaf # B commit message

I suggest to turn it to:

pick  abcd # A commit message
merge beaf # B commit message

So that the # is finally universally the start of comment.

While we are at this, I couldn't find any even semi-formal syntax
description of the entire todo list. Is there one already? Are you going
to provide one?

-- Sergey

[PATCH v6 04/15] sequencer: introduce new commands to reset the revision

[Johannes Schindelin]

In the upcoming commits, we will teach the sequencer to rebase merges.
This will be done in a very different way from the unfortunate design of
`git rebase --preserve-merges` (which does not allow for reordering
commits, or changing the branch topology).

The main idea is to introduce new todo list commands, to support
labeling the current revision with a given name, resetting the current
revision to a previous state, and  merging labeled revisions.

This idea was developed in Git for Windows' Git garden shears (that are
used to maintain Git for Windows' "thicket of branches" on top of
upstream Git), and this patch is part of the effort to make it available
to a wider audience, as well as to make the entire process more robust
(by implementing it in a safe and portable language rather than a Unix
shell script).

This commit implements the commands to label, and to reset to, given
revisions. The syntax is:

label 
reset 

Internally, the `label ` command creates the ref
`refs/rewritten/`. This makes it possible to work with the labeled
revisions interactively, or in a scripted fashion (e.g. via the todo
list command `exec`).

These temporary refs are removed upon sequencer_remove_state(), so that
even a `git rebase --abort` cleans them up.

We disallow '#' as label because that character will be used as separator
in the upcoming `merge` command.

Later in this patch series, we will mark the `refs/rewritten/` refs as
worktree-local, to allow for interactive rebases to be run in parallel in
worktrees linked to the same repository.

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

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e1b865f43f2..e8d3a7d7588 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -162,6 +162,8 @@ 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
 
 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 1ee70d843c1..c63d47f5e09 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,33 @@ 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 (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;
 }
@@ -1279,6 +1303,8 @@ enum todo_command {
TODO_SQUASH,
/* commands that do something else than handling a single commit */
TODO_EXEC,
+   TODO_LABEL,
+   TODO_RESET,
/* commands that do nothing but are counted for reporting progress */
TODO_NOOP,
TODO_DROP,
@@ -1297,6 +1323,8 @@ static struct {
{ 'f', "fixup" },
{