git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Liam Beguin <liambeguin@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, avarab@gmail.com
Subject: Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper
Date: Mon, 27 Nov 2017 23:42:32 +0100 (CET)	[thread overview]
Message-ID: <alpine.DEB.2.21.1.1711272241590.6482@virtualbox> (raw)
In-Reply-To: <20171127045514.25647-4-liambeguin@gmail.com>

Hi Liam,

could I ask for a favor? I'd like the oneline to start with

	rebase -i -x: ...

(this would help future me to realize what this commit touches already
from the concise graph output I favor).

On Sun, 26 Nov 2017, Liam Beguin wrote:

> Recent work on `git-rebase--interactive` aim to convert shell code to C.
> Even if this is most likely not a big performance enhacement, let's
> convert it too since a comming change to abbreviate command names requires
> it to be updated.

Since Junio did not comment on the commit message: could you replace
`aim` by `aims`, `enhacement` by `enhancement` and `comming` by `coming`?

> @@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  			N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
>  		OPT_CMDMODE(0, "rearrange-squash", &command,
>  			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
> +		OPT_CMDMODE(0, "add-exec", &command,
> +			N_("insert exec commands in todo list"), ADD_EXEC),

Maybe `add-exec-commands`? I know it is longer to type, but these options do
not need to be typed interactively and the longer name would be consistent
with the function name.

> diff --git a/sequencer.c b/sequencer.c
> index fa94ed652d2c..810b7850748e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
>  	return 0;
>  }
>  

As the code in add_exec_commands() may appear convoluted (why not simply
append the command after any pick?), the original comment would be really
nice here:

	/*
	 * Add commands after pick and (series of) squash/fixup commands
	 * in the todo list.
	 */

> +int add_exec_commands(const char *command)
> +{
> +	const char *todo_file = rebase_path_todo();
> +	struct todo_list todo_list = TODO_LIST_INIT;
> +	int fd, res, i, first = 1;
> +	FILE *out;
> +
> +	strbuf_reset(&todo_list.buf);

The todo_list.buf has been initialized already (via TODO_LIST_INIT), no
need to reset it again.

> +	fd = open(todo_file, O_RDONLY);
> +	if (fd < 0)
> +		return error_errno(_("could not open '%s'"), todo_file);
> +	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> +		close(fd);
> +		return error(_("could not read '%s'."), todo_file);
> +	}
> +	close(fd);

As Junio pointed out so gently: there is a helper function that does this
all very conveniently for us:

	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
		return error_errno(_("could not read '%s'"), todo_file);

And as I realized looking at the surrounding code: you probably just
inherited my inelegant code by copy-editing from another function in
sequencer.c. Should you decide to add a preparatory patch to your patch
series that converts these other callers, or even refactors all that code
that reads the git-rebase-todo file and then parses it, I would be quite
happy... :-) (although I would understand if you deemed this outside the
purpose of your patch series).

> +	res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
> +	if (res) {
> +		todo_list_release(&todo_list);
> +		return error(_("unusable todo list: '%s'"), todo_file);
> +	}

The variable `res` is not really used here. Let's just put the
parse_insn_buffer() call inside the if ().

> +	out = fopen(todo_file, "w");
> +	if (!out) {
> +		todo_list_release(&todo_list);
> +		return error(_("unable to open '%s' for writing"), todo_file);
> +	}
> +	for (i = 0; i < todo_list.nr; i++) {
> +		struct todo_item *item = todo_list.items + i;
> +		int bol = item->offset_in_buf;
> +		const char *p = todo_list.buf.buf + bol;
> +		int eol = i + 1 < todo_list.nr ?
> +			todo_list.items[i + 1].offset_in_buf :
> +			todo_list.buf.len;

This smells like another copy-edited snippet that originated from my
brain, and I am not at all proud by the complexity I used there.

The function should also check for errors during writing. So how about
something like this instead?

	struct strbuf *buf = &todo_list.buf;
	size_t offset = 0, command_len = strlen(command);
	int first = 1, i;
	struct todo_item *item;

	...

	/* insert <command> before every pick except the first one */
	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++)
		if (item->command == TODO_PICK) {
			if (first)
				first = 0;
			else {
				strbuf_splice(buf,
					      item->offset_in_buf + offset, 0,
					      command, command_len);
				offset += command_len;
			}
		}

	/* append a final <command> */
	strbuf_complete_list(buf);
	strbuf_add(buf, command, command_len);

	i = write_message(buf->buf, buf->len, todo_file, 0);
	todo_list_release(&todo_list);
	return i;

Ciao,
Dscho

  parent reply	other threads:[~2017-11-27 22:42 UTC|newest]

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-27  4:55 [PATCH 0/5] rebase -i: add config to abbreviate command names Liam Beguin
2017-11-27  4:55 ` [PATCH 1/5] Documentation: move rebase.* configs to new file Liam Beguin
2017-11-27 21:27   ` Johannes Schindelin
2017-11-27  4:55 ` [PATCH 2/5] Documentation: use preferred name for the 'todo list' script Liam Beguin
2017-11-27 21:28   ` Johannes Schindelin
2017-11-27  4:55 ` [PATCH 3/5] rebase -i: add exec commands via the rebase--helper Liam Beguin
2017-11-27  5:14   ` Junio C Hamano
2017-11-27 21:41     ` Johannes Schindelin
2017-11-27 23:45       ` Junio C Hamano
2017-11-29  2:01     ` liam Beguin
2017-11-27 22:42   ` Johannes Schindelin [this message]
2017-11-27 23:48     ` Junio C Hamano
2017-11-29  2:06     ` liam Beguin
2017-11-29 21:35       ` Johannes Schindelin
2017-11-27  4:55 ` [PATCH 4/5] rebase -i: learn to abbreviate command names Liam Beguin
2017-11-27  5:19   ` Junio C Hamano
2017-11-29  2:08     ` liam Beguin
2017-11-27 23:04   ` Johannes Schindelin
2017-11-27 23:11     ` Jeff King
2017-11-29  2:11       ` liam Beguin
2017-11-29  2:10     ` liam Beguin
2017-11-29 21:40       ` Johannes Schindelin
2017-12-03  1:18         ` Junio C Hamano
2017-11-27  4:55 ` [PATCH 5/5] t3404: add test case for abbreviated commands Liam Beguin
2017-11-27  5:28   ` Junio C Hamano
2017-11-27 23:16   ` Johannes Schindelin
2017-11-27  5:23 ` [PATCH 0/5] rebase -i: add config to abbreviate command names Junio C Hamano
2017-11-29  1:56   ` liam Beguin
2017-12-03 22:17 ` [PATCH v2 0/9] " Liam Beguin
2017-12-03 22:17   ` [PATCH v2 1/9] Documentation: move rebase.* configs to new file Liam Beguin
2017-12-03 22:17   ` [PATCH v2 2/9] Documentation: use preferred name for the 'todo list' script Liam Beguin
2017-12-03 22:17   ` [PATCH v2 3/9] rebase -i: set commit to null in exec commands Liam Beguin
2017-12-03 22:17   ` [PATCH v2 4/9] rebase -i: refactor transform_todo_ids Liam Beguin
2017-12-04 14:42     ` Johannes Schindelin
2017-12-04 16:09       ` Junio C Hamano
2017-12-05  3:36         ` liam Beguin
2017-12-05 12:35           ` Junio C Hamano
2017-12-05  3:39       ` liam Beguin
2017-12-03 22:17   ` [PATCH v2 5/9] rebase -i: replace reference to sha1 with oid Liam Beguin
2017-12-03 22:17   ` [PATCH v2 6/9] rebase -i: update functions to use a flags parameter Liam Beguin
2017-12-04 15:46     ` Johannes Schindelin
2017-12-04 16:06       ` Johannes Schindelin
2017-12-05  3:42       ` liam Beguin
2017-12-05 12:37         ` Junio C Hamano
2017-12-05 12:41           ` Kerry, Richard
2017-12-05 14:42             ` liam Beguin
2017-12-05 16:05             ` Junio C Hamano
2017-12-05 16:14               ` Kerry, Richard
2017-12-03 22:17   ` [PATCH v2 7/9] rebase -i -x: add exec commands via the rebase--helper Liam Beguin
2017-12-03 22:17   ` [PATCH v2 8/9] rebase -i: learn to abbreviate command names Liam Beguin
2017-12-25 12:48     ` Duy Nguyen
2017-12-25 15:39       ` Liam Beguin
2017-12-25 23:58         ` Duy Nguyen
2017-12-27 19:15       ` Junio C Hamano
2017-12-27 21:58         ` Liam Beguin
2017-12-28 18:55           ` Junio C Hamano
2017-12-03 22:17   ` [PATCH v2 9/9] t3404: add test case for abbreviated commands Liam Beguin
2017-12-04 16:07   ` [PATCH v2 0/9] rebase -i: add config to abbreviate command names Johannes Schindelin
2017-12-05 17:52 ` Liam Beguin
2017-12-05 17:52   ` [PATCH v3 1/9] Documentation: move rebase.* configs to new file Liam Beguin
2017-12-05 17:52   ` [PATCH v3 2/9] Documentation: use preferred name for the 'todo list' script Liam Beguin
2017-12-05 17:52   ` [PATCH v3 3/9] rebase -i: set commit to null in exec commands Liam Beguin
2017-12-05 17:52   ` [PATCH v3 4/9] rebase -i: refactor transform_todo_ids Liam Beguin
2017-12-05 17:52   ` [PATCH v3 5/9] rebase -i: replace reference to sha1 with oid Liam Beguin
2017-12-05 17:52   ` [PATCH v3 6/9] rebase -i: update functions to use a flags parameter Liam Beguin
2017-12-05 17:52   ` [PATCH v3 7/9] rebase -i -x: add exec commands via the rebase--helper Liam Beguin
2017-12-05 17:52   ` [PATCH v3 8/9] rebase -i: learn to abbreviate command names Liam Beguin
2017-12-05 17:52   ` [PATCH v3 9/9] t3404: add test case for abbreviated commands Liam Beguin
2017-12-05 22:21   ` [PATCH v2 0/9] rebase -i: add config to abbreviate command names Junio C Hamano
2017-12-06  2:42     ` liam Beguin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.21.1.1711272241590.6482@virtualbox \
    --to=johannes.schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=liambeguin@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).