From: liam Beguin <liambeguin@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
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: Tue, 28 Nov 2017 21:06:53 -0500 [thread overview]
Message-ID: <6b4e8352-0583-11c2-43ac-ec4ab33cc554@gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.21.1.1711272241590.6482@virtualbox>
Hi Johannes,
Thanks for taking the time to review this.
On 27/11/17 05:42 PM, Johannes Schindelin wrote:
> 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).
Sure, I'll update the commit subject.
>
> 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`?
Ow.. sorry about that! I'll fix those and make sure to proofread better next time!
>
>> @@ -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.
Makes sense. It'll also be more consistent with the rest of the commands above.
>
>> 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.
> */
>
I'll make sure to include that comment.
The code is a bit convoluted as you say... I wanted to send it "as is" first
to get comments and update based on feedback from the list.
I just realized we could maybe add exec instructions only after pick commands
if we do add-exec-commands before rearrange-squash. I'll test it out.
>> +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).
>
You guessed well, I mostly did copy-editing... I thought I found this code
a little confusing because I'm not used to as much pointer gymnastics but
it reassures me a bit to read this :-). I'll see if I can come up with a
better solution.
>> + 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 ().
>
Will do.
>> + 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;
>
I'll see how I can include this if calling add-exec-commands before
rearrange-squash works. But it definitely is lighter to read.
> Ciao,
> Dscho
>
Thanks again,
Liam
next prev parent reply other threads:[~2017-11-29 2:07 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
2017-11-27 23:48 ` Junio C Hamano
2017-11-29 2:06 ` liam Beguin [this message]
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=6b4e8352-0583-11c2-43ac-ec4ab33cc554@gmail.com \
--to=liambeguin@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.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).