Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

Jakub Narębski [off-list ref] writes:

quoted

diff --git a/sequencer.c b/sequencer.c
index 06759d4..3398774 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 struct todo_item {
 	enum todo_command command;
 	struct commit *commit;
+	const char *arg;
+	int arg_len;

Why 'arg', and not 'oneline', or 'subject'?
I'm not saying it is bad name.

I am not sure what the "commit" field of type "struct commit *" is
for.  It is not needed until it is the commit's turn to be picked or
reverted; if we end up stopping in the middle, parsing the commit
object for later steps will end up being wasted effort.

Also, when the sequencer becomes one sequencer to rule them all, the
command set may contain something that does not even mention any
commit at all (think: exec).

So I am not sure if we want a parsed commit there (I would not
object if we kept the texual object name read from the file,
though).  The "one sequencer to rule them all" may even have to say
"now give name ':1' to the result of the previous operation" in one
step and in another later step have an instruction "merge ':1'".
When that happens, you cannot even pre-populate the commit object
when the sequencer reads the file, as the commit has not yet been
created at that point.

W dniu 31.08.2016 o 21:10, Junio C Hamano pisze:

Jakub Narębski [off-list ref] writes:

quoted

quoted

diff --git a/sequencer.c b/sequencer.c
index 06759d4..3398774 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 struct todo_item {
 	enum todo_command command;
 	struct commit *commit;
+	const char *arg;
+	int arg_len;

I am not sure what the "commit" field of type "struct commit *" is
for.  It is not needed until it is the commit's turn to be picked or
reverted; if we end up stopping in the middle, parsing the commit
object for later steps will end up being wasted effort.

From what I understand this was what sequencer did before this
series, so it is not a regression (I think; the commit parsing
was in different function, but I think at the same place in
the callchain).

Also, when the sequencer becomes one sequencer to rule them all, the
command set may contain something that does not even mention any
commit at all (think: exec).

The "exec" line is a bit of exception, all other rebase -i commands
take commit as parameter.  It could always use NULL.

So I am not sure if we want a parsed commit there (I would not
object if we kept the texual object name read from the file,
though).  The "one sequencer to rule them all" may even have to say
"now give name ':1' to the result of the previous operation" in one
step and in another later step have an instruction "merge ':1'".
When that happens, you cannot even pre-populate the commit object
when the sequencer reads the file, as the commit has not yet been
created at that point.

True, --preserve-merges rebase is well, different.

Best,
-- 
Jakub Narebski

Hi Junio,

On Wed, 31 Aug 2016, Junio C Hamano wrote:

Jakub Narębski [off-list ref] writes:

quoted

quoted

diff --git a/sequencer.c b/sequencer.c
index 06759d4..3398774 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 struct todo_item {
 	enum todo_command command;
 	struct commit *commit;
+	const char *arg;
+	int arg_len;

Why 'arg', and not 'oneline', or 'subject'?
I'm not saying it is bad name.

I am not sure what the "commit" field of type "struct commit *" is
for.  It is not needed until it is the commit's turn to be picked or
reverted; if we end up stopping in the middle, parsing the commit
object for later steps will end up being wasted effort.

No, it won't be wasted effort, as we *validate* the todo script this way.
And since we may very well need the info later (most rebases do not fail
in the middle), we store it, too.

Also, when the sequencer becomes one sequencer to rule them all, the
command set may contain something that does not even mention any
commit at all (think: exec).

Right, in which case the "commit" field will have the value... *drum
roll*... NULL!

So I am not sure if we want a parsed commit there (I would not
object if we kept the texual object name read from the file,
though).  The "one sequencer to rule them all" may even have to say
"now give name ':1' to the result of the previous operation" in one
step and in another later step have an instruction "merge ':1'".
When that happens, you cannot even pre-populate the commit object
when the sequencer reads the file, as the commit has not yet been
created at that point.

These considerations are pretty hypothetical. I would even place a bet
that we will *never* have ":1" as names, not if I have anything to say...
;-)

What is not so hypothetical is that after parsing the todo_list, we will
have to act on the information contained therein. For example we will have
to cherry-pick some of the indicated commits (requiring a struct commit *
for use in do_pick_commit()). Another example: we may need to determine
the oneline for use in fixup!/squash! reordering.

So: keeping *that* aspect of the previous todo_list parsing, i.e. store a
pointer to the already-parsed commit, is the right thing to do.

Ciao,
Dscho

Hi Kuba & Junio,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

W dniu 31.08.2016 o 21:10, Junio C Hamano pisze:

quoted

Jakub Narębski [off-list ref] writes:

quoted

quoted

diff --git a/sequencer.c b/sequencer.c
index 06759d4..3398774 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 struct todo_item {
 	enum todo_command command;
 	struct commit *commit;
+	const char *arg;
+	int arg_len;

quoted

I am not sure what the "commit" field of type "struct commit *" is
for.  It is not needed until it is the commit's turn to be picked or
reverted; if we end up stopping in the middle, parsing the commit
object for later steps will end up being wasted effort.

From what I understand this was what sequencer did before this
series, so it is not a regression (I think; the commit parsing
was in different function, but I think at the same place in
the callchain).

Exactly.

quoted

Also, when the sequencer becomes one sequencer to rule them all, the
command set may contain something that does not even mention any
commit at all (think: exec).

The "exec" line is a bit of exception, all other rebase -i commands
take commit as parameter.  It could always use NULL.

There is also "noop".

quoted

So I am not sure if we want a parsed commit there (I would not
object if we kept the texual object name read from the file,
though).  The "one sequencer to rule them all" may even have to say
"now give name ':1' to the result of the previous operation" in one
step and in another later step have an instruction "merge ':1'".
When that happens, you cannot even pre-populate the commit object
when the sequencer reads the file, as the commit has not yet been
created at that point.

True, --preserve-merges rebase is well, different.

It is mis-designed. And I can be that harsh because it was my design.

In the meantime I came up with a much better design, and implemented it as
a shell script on top of rebase -i. Since shell scripts run like slow
molasses, even more so on Windows, I have a loose plan to implement its
functionality as a new --recreate-merges option, and to deprecate
--preserve-merges when that new option works.

It needs to be a new option (not a --preserve-merges=v2) because it is a
totally different beast. For starters, it does not need its own code path
that overrides pick_one, as --preserve-merges does.

But I get way ahead of myself. First we need to get these last few bits
and pieces in place to accelerate (non --preserve-merges) rebase -i.

Ciao,
Dscho

W dniu 01.09.2016 o 11:37, Johannes Schindelin pisze:

On Wed, 31 Aug 2016, Junio C Hamano wrote:

quoted

Jakub Narębski [off-list ref] writes:

quoted

quoted

diff --git a/sequencer.c b/sequencer.c
index 06759d4..3398774 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 struct todo_item {
 	enum todo_command command;
 	struct commit *commit;
+	const char *arg;
+	int arg_len;

Why 'arg', and not 'oneline', or 'subject'?
I'm not saying it is bad name.

I am not sure what the "commit" field of type "struct commit *" is
for.  It is not needed until it is the commit's turn to be picked or
reverted; if we end up stopping in the middle, parsing the commit
object for later steps will end up being wasted effort.

No, it won't be wasted effort, as we *validate* the todo script this way.
And since we may very well need the info later (most rebases do not fail
in the middle), we store it, too.

The question was (I think) whether we should do eager parsing of
commits, or whether we can do lazy parsing by postponing full parsing
"until it is the commit's turn to be picked or reverted", and possibly
when saving todo file.

I wonder how probable is situation where we save instruction sheet
for interactive rebase, with shortened SHA-1, and during rebase
shortened SHA-1 stops being unambiguous...

                               [...] after parsing the todo_list, we will
have to act on the information contained therein. For example we will have
to cherry-pick some of the indicated commits (requiring a struct commit *
for use in do_pick_commit()). Another example: we may need to determine
the oneline for use in fixup!/squash! reordering.

So: keeping *that* aspect of the previous todo_list parsing, i.e. store a
pointer to the already-parsed commit, is the right thing to do.

The above probably means that eager eval is better

Best,
-- 
Jakub Narębski

Hello,

W dniu 01.09.2016 o 15:12, Johannes Schindelin pisze:

On Wed, 31 Aug 2016, Jakub Narębski wrote:

quoted

W dniu 31.08.2016 o 21:10, Junio C Hamano pisze:

quoted

quoted

So I am not sure if we want a parsed commit there (I would not
object if we kept the texual object name read from the file,
though).  The "one sequencer to rule them all" may even have to say
"now give name ':1' to the result of the previous operation" in one
step and in another later step have an instruction "merge ':1'".
When that happens, you cannot even pre-populate the commit object
when the sequencer reads the file, as the commit has not yet been
created at that point.

True, --preserve-merges rebase is well, different.

It is mis-designed. And I can be that harsh because it was my design.

In the meantime I came up with a much better design, and implemented it as
a shell script on top of rebase -i. Since shell scripts run like slow
molasses, even more so on Windows, I have a loose plan to implement its
functionality as a new --recreate-merges option, and to deprecate
--preserve-merges when that new option works.

It needs to be a new option (not a --preserve-merges=v2) because it is a
totally different beast. For starters, it does not need its own code path
that overrides pick_one, as --preserve-merges does.

Better preserving for merges (with cleanly defined sematics)
would be certainly nice to have.

But I get way ahead of myself. First we need to get these last few bits
and pieces in place to accelerate (non --preserve-merges) rebase -i.

But it can wait, right.

-- 
Jakub Narębski