threads / patch / 47485

patch, 5 partsA couple of sequencer cleanups

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

## tl;dr

8 messages between Dec 22, 2017 and Dec 27, 2017. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Johannes Schindelin· Dec 22, 2017, 23:55 UTC · lore

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
Johannes Schindelin· Dec 22, 2017, 23:55 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to git-rebase--interactive.sh +2 −1
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
Johannes Schindelin· Dec 22, 2017, 23:55 UTC · re: Johannes Schindelin · lore

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to sequencer.c +4 −4
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
Johannes Schindelin· Dec 22, 2017, 23:55 UTC · re: Johannes Schindelin · lore

[PATCH 3/5] sequencer: remove superfluous conditional

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(-)
Show changes to sequencer.c +1 −3
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
Johannes Schindelin· Dec 22, 2017, 23:55 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to sequencer.c +9 −4
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
Johannes Schindelin· Dec 22, 2017, 23:56 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to sequencer.c +4 −1
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
Liam Beguin· Dec 27, 2017, 22:19 UTC · re: Johannes Schindelin · lore

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

Hi Johannes,

On 23 December 2017 at 00:56, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 21 quoted lines
> 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.

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

Thanks, Liam

Junio C Hamano· Dec 27, 2017, 20:33 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 6 quoted lines
> 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.

← back to recent threads