Re: [PATCH v6 04/15] sequencer: introduce new commands to reset the revision
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
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
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
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
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
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