[PATCH 0/5] A couple of sequencer cleanups

[PATCH 0/5] A couple of sequencer cleanups

[Johannes Schindelin]

While working on patches to teach `git rebase -i` to recreate branch topology
properly, i.e. replace the design of `--preserve-merges` by something much
better, I stumbled across a couple of issues that I thought I should fix on the
way.

The patches are based on lb/rebase-i-short-command-names, mainly because the
new `--recreate-merges` patches benefit from the patch "rebase -i: update
functions to use a flags parameter", and I was too lazy to resolve the merge
conflicts while rebasing the patch "sequencer: do not invent whitespace when
transforming OIDs" to the current `master` branch.

Oh, and by the way, the `--recreate-merges` feature already works. I used it
to develop the patches themselves. I do not have time to pass one last time
over them, so they'll have to wait for next year to see the Git mailing list.
If anyone wants to have a look over them, play with them, or even wants to
review the patches:

	https://github.com/git/git/compare/master...dscho:sequencer-shears


Johannes Schindelin (5):
  rebase: do not continue when the todo list generation failed
  sequencer: strip bogus LF at end of error messages
  sequencer: remove superfluous conditional
  sequencer: report when noop has an argument
  sequencer: do not invent whitespace when transforming OIDs

 git-rebase--interactive.sh |  3 ++-
 sequencer.c                | 30 ++++++++++++++++++------------
 2 files changed, 20 insertions(+), 13 deletions(-)


base-commit: 1795993488bef1b48e4224db096e9d12df075db2
Based-On: lb/rebase-i-short-command-names at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git lb/rebase-i-short-command-names
Published-As: https://github.com/dscho/git/releases/tag/sequencer-cleanups-v1
Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-cleanups-v1
-- 
2.15.1.windows.2

[PATCH 1/5] rebase: do not continue when the todo list generation failed

[Johannes Schindelin]

This is a *really* long-standing bug. As a matter of fact, this bug has
been with us from the very beginning of `rebase -i`: 1b1dce4bae7 (Teach
rebase an interactive mode, 2007-06-25), where the output of `rev-list`
was piped to `sed` (and any failure of the `rev-list` process would go
completely undetected).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e3f5a0abf3c..b7f95672bd9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -893,7 +893,8 @@ fi
 if test t != "$preserve_merges"
 then
 	git rebase--helper --make-script ${keep_empty:+--keep-empty} \
-		$revisions ${restrict_revision+^$restrict_revision} >"$todo"
+		$revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
+	die "$(gettext "Could not generate todo list")"
 else
 	format=$(git config --get rebase.instructionFormat)
 	# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse
-- 
2.15.1.windows.2

[PATCH 2/5] sequencer: strip bogus LF at end of error messages

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 115085d39ca..8e6b6289be6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -491,7 +491,7 @@ static int is_index_unchanged(void)
 	struct commit *head_commit;
 
 	if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL))
-		return error(_("could not resolve HEAD commit\n"));
+		return error(_("could not resolve HEAD commit"));
 
 	head_commit = lookup_commit(&head_oid);
 
@@ -511,7 +511,7 @@ static int is_index_unchanged(void)
 
 	if (!cache_tree_fully_valid(active_cache_tree))
 		if (cache_tree_update(&the_index, 0))
-			return error(_("unable to update cache tree\n"));
+			return error(_("unable to update cache tree"));
 
 	return !oidcmp(&active_cache_tree->oid,
 		       &head_commit->tree->object.oid);
@@ -697,12 +697,12 @@ static int is_original_commit_empty(struct commit *commit)
 	const struct object_id *ptree_oid;
 
 	if (parse_commit(commit))
-		return error(_("could not parse commit %s\n"),
+		return error(_("could not parse commit %s"),
 			     oid_to_hex(&commit->object.oid));
 	if (commit->parents) {
 		struct commit *parent = commit->parents->item;
 		if (parse_commit(parent))
-			return error(_("could not parse parent commit %s\n"),
+			return error(_("could not parse parent commit %s"),
 				oid_to_hex(&parent->object.oid));
 		ptree_oid = &parent->tree->object.oid;
 	} else {
-- 
2.15.1.windows.2

[PATCH 3/5] sequencer: remove superfluous conditional

[Johannes Schindelin]

In a conditional block that is only reached when handling a TODO_REWORD
(as seen even from a 3-line context), there is absolutely no need to
nest another block under the identical condition.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8e6b6289be6..38266c3c228 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1011,9 +1011,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			opts);
 		if (res || command != TODO_REWORD)
 			goto leave;
-		flags |= EDIT_MSG | AMEND_MSG;
-		if (command == TODO_REWORD)
-			flags |= VERIFY_MSG;
+		flags |= EDIT_MSG | AMEND_MSG | VERIFY_MSG;
 		msg_file = NULL;
 		goto fast_forward_edit;
 	}
-- 
2.15.1.windows.2

[PATCH 4/5] sequencer: report when noop has an argument

[Johannes Schindelin]

The noop command cannot accept any argument, but we never told the user
about any bogus argument. Fix that.

while at it, mention clearly when an argument is required but missing
(for commands *other* than noop).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 38266c3c228..5632415ea2d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1259,18 +1259,23 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
 	if (i >= TODO_COMMENT)
 		return -1;
 
+	/* Eat up extra spaces/ tabs before object name */
+	padding = strspn(bol, " \t");
+	bol += padding;
+
 	if (item->command == TODO_NOOP) {
+		if (bol != eol)
+			return error(_("%s does not accept arguments: '%s'"),
+				     command_to_string(item->command), bol);
 		item->commit = NULL;
 		item->arg = bol;
 		item->arg_len = eol - bol;
 		return 0;
 	}
 
-	/* Eat up extra spaces/ tabs before object name */
-	padding = strspn(bol, " \t");
 	if (!padding)
-		return -1;
-	bol += padding;
+		return error(_("missing arguments for %s"),
+			     command_to_string(item->command));
 
 	if (item->command == TODO_EXEC) {
 		item->commit = NULL;
-- 
2.15.1.windows.2

[PATCH 5/5] sequencer: do not invent whitespace when transforming OIDs

[Johannes Schindelin]

For commands that do not have an argument, there is no need to append a
trailing space at the end of the line.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 5632415ea2d..970842e3fcc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2584,7 +2584,10 @@ int transform_todos(unsigned flags)
 			strbuf_addf(&buf, " %s", oid);
 		}
 		/* add all the rest */
-		strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg);
+		if (!item->arg_len)
+			strbuf_addch(&buf, '\n');
+		else
+			strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg);
 	}
 
 	i = write_message(buf.buf, buf.len, todo_file, 0);
-- 
2.15.1.windows.2

Re: [PATCH 0/5] A couple of sequencer cleanups

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Johannes Schindelin (5):
>   rebase: do not continue when the todo list generation failed
>   sequencer: strip bogus LF at end of error messages
>   sequencer: remove superfluous conditional
>   sequencer: report when noop has an argument
>   sequencer: do not invent whitespace when transforming OIDs

All looked obviously correct and good clean-up changes.  

Thanks; will queue.

Re: [PATCH 5/5] sequencer: do not invent whitespace when transforming OIDs

[Liam Beguin]

Hi Johannes,

On 23 December 2017 at 00:56, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> For commands that do not have an argument, there is no need to append a
> trailing space at the end of the line.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 5632415ea2d..970842e3fcc 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2584,7 +2584,10 @@ int transform_todos(unsigned flags)
>                         strbuf_addf(&buf, " %s", oid);
>                 }
>                 /* add all the rest */
> -               strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg);
> +               if (!item->arg_len)
> +                       strbuf_addch(&buf, '\n');
> +               else
> +                       strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg);

I also went with that when I was working on this but I thought leaving the
extra whitespace would make the code a little shorter.
Other than that, this change and the others look good.

>         }
>
>         i = write_message(buf.buf, buf.len, todo_file, 0);
> --
> 2.15.1.windows.2

Thanks,
Liam