Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids
liam Beguin writes: > Good suggestion. Would transform_todos() work too? If the function is about munging multiple of them, then todo"s" would work well; I wasn't focusing on singular vs plural, as I thought the choice between them needs much less thought to make correctly.
Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids
Hi Johannes, On 04/12/17 09:42 AM, Johannes Schindelin wrote: > Hi Liam, > > On Sun, 3 Dec 2017, Liam Beguin wrote: > >> The transform_todo_ids function is a little hard to read. Lets try >> to make it easier by using more of the strbuf API. Also, since we'll >> soon be adding command abbreviations, let's rename the function so >> it's name reflects that change. > > I am not really a fan of the new name, and would prefer the old one, but > that's only a nit, not a reason to reject the patch. > You're right, it's probably not the best name. I'll change it to transform_todos() as we want the function name to reflect that it changes both parts of the todo. > The rest of it makes the code reads a lot nicer than before. Thank you, > Johannes > Thanks, Liam
Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids
Hi Junio, On 04/12/17 11:09 AM, Junio C Hamano wrote: > Johannes Schindelin writes: > >> On Sun, 3 Dec 2017, Liam Beguin wrote: >> >>> The transform_todo_ids function is a little hard to read. Lets try >>> to make it easier by using more of the strbuf API. Also, since we'll >>> soon be adding command abbreviations, let's rename the function so >>> it's name reflects that change. >> >> I am not really a fan of the new name, and would prefer the old one, but >> that's only a nit, not a reason to reject the patch. > > FWIW, I do think the new name goes backwards. The command uses to > remember what operations are to be carried out in which order using > a thing, and the name of that thing "todo list". We also called it > the "instruction sheet", and "insn" was a good term to call one item > on that sheet among other items. > Good point on saying this name change is going backwards. > But recent commits in the area are pushing us to call it "todo list" > consistently. An element in that list should be called "todo". > > A "todo" consists of two parts, "what operation is done" part and > "using what commit object" part. The original implementation of > this function affected only the latter part, and in that light, the > original name transform_todo_ids() is understandable. Now you are > planning to make it modify both parts, not just "ids", so it is > understandable that you would want to rename it. But I do not think > "insn" matches the recent trend. It even risks misunderstanding > (i.e. xfrm_todo_ids() is about modifying "using what commit" part, > so perhaps xfrm_todo_insns() is about modifying "what operation is > done" part---but that is different from what you want to do, which > is to update _both_ halves). > You're right! We do want the name to reflect that we intend to change both halves and not only the command. > Calling it just transform_todo() would probably be more in line with > the reason why you wanted to rename it in the first place. > Good suggestion. Would transform_todos() work too? I'll send an update tomorrow. Thanks, Liam
Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids
Johannes Schindelin writes: > On Sun, 3 Dec 2017, Liam Beguin wrote: > >> The transform_todo_ids function is a little hard to read. Lets try >> to make it easier by using more of the strbuf API. Also, since we'll >> soon be adding command abbreviations, let's rename the function so >> it's name reflects that change. > > I am not really a fan of the new name, and would prefer the old one, but > that's only a nit, not a reason to reject the patch. FWIW, I do think the new name goes backwards. The command uses to remember what operations are to be carried out in which order using a thing, and the name of that thing "todo list". We also called it the "instruction sheet", and "insn" was a good term to call one item on that sheet among other items. But recent commits in the area are pushing us to call it "todo list" consistently. An element in that list should be called "todo". A "todo" consists of two parts, "what operation is done" part and "using what commit object" part. The original implementation of this function affected only the latter part, and in that light, the original name transform_todo_ids() is understandable. Now you are planning to make it modify both parts, not just "ids", so it is understandable that you would want to rename it. But I do not think "insn" matches the recent trend. It even risks misunderstanding (i.e. xfrm_todo_ids() is about modifying "using what commit" part, so perhaps xfrm_todo_insns() is about modifying "what operation is done" part---but that is different from what you want to do, which is to update _both_ halves). Calling it just transform_todo() would probably be more in line with the reason why you wanted to rename it in the first place.
Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids
Hi Liam, On Sun, 3 Dec 2017, Liam Beguin wrote: > The transform_todo_ids function is a little hard to read. Lets try > to make it easier by using more of the strbuf API. Also, since we'll > soon be adding command abbreviations, let's rename the function so > it's name reflects that change. I am not really a fan of the new name, and would prefer the old one, but that's only a nit, not a reason to reject the patch. The rest of it makes the code reads a lot nicer than before. Thank you, Johannes
[PATCH v2 4/9] rebase -i: refactor transform_todo_ids
The transform_todo_ids function is a little hard to read. Lets try
to make it easier by using more of the strbuf API. Also, since we'll
soon be adding command abbreviations, let's rename the function so
it's name reflects that change.
Signed-off-by: Liam Beguin
---
builtin/rebase--helper.c | 4 +--
sequencer.c | 69
sequencer.h | 2 +-
3 files changed, 31 insertions(+), 44 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f8519363a393..7c06a27de821 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -55,9 +55,9 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
if (command == MAKE_SCRIPT && argc > 1)
return !!sequencer_make_script(keep_empty, stdout, argc, argv);
if (command == SHORTEN_SHA1S && argc == 1)
- return !!transform_todo_ids(1);
+ return !!transform_todo_insn(1);
if (command == EXPAND_SHA1S && argc == 1)
- return !!transform_todo_ids(0);
+ return !!transform_todo_insn(0);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 5033b049d995..0ff3c90e44bf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2494,60 +2494,47 @@ int sequencer_make_script(int keep_empty, FILE *out,
}
-int transform_todo_ids(int shorten_ids)
+int transform_todo_insn(int shorten_ids)
{
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
- int fd, res, i;
- FILE *out;
+ struct strbuf buf = STRBUF_INIT;
+ struct todo_item *item;
+ int i;
- strbuf_reset(&todo_list.buf);
- 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);
+ if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
return error(_("could not read '%s'."), todo_file);
- }
- close(fd);
- res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
- if (res) {
+ if (parse_insn_buffer(todo_list.buf.buf, &todo_list)) {
todo_list_release(&todo_list);
return error(_("unusable todo list: '%s'"), todo_file);
}
- 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;
-
- if (item->command >= TODO_EXEC && item->command != TODO_DROP)
- fwrite(p, eol - bol, 1, out);
- else {
- const char *id = shorten_ids ?
- short_commit_name(item->commit) :
- oid_to_hex(&item->commit->object.oid);
- int len;
-
- p += strspn(p, " \t"); /* left-trim command */
- len = strcspn(p, " \t"); /* length of command */
-
- fprintf(out, "%.*s %s %.*s\n",
- len, p, id, item->arg_len, item->arg);
+ for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
+ /* if the item is not a command write it and continue */
+ if (item->command >= TODO_COMMENT) {
+ strbuf_addf(&buf, "%.*s\n", item->arg_len, item->arg);
+ continue;
+ }
+
+ /* add command to the buffer */
+ strbuf_addstr(&buf, command_to_string(item->command));
+
+ /* add commit id */
+ if (item->commit) {
+ const char *oid = shorten_ids ?
+ short_commit_name(item->commit) :
+ oid_to_hex(&item->commit->object.oid);
+
+ strbuf_addf(&buf, " %s", oid);
}
+ /* add all the rest */
+ strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg);
}
- fclose(out);
+
+ i = write_message(buf.buf, buf.len, todo_file, 0);
todo_list_release(&todo_list);
- return 0;
+ return i;
}
enum check_level {
diff --git a/sequencer.h b/sequencer.h
index 6f3d3df82c0a..4e444e3bf1c4 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -48,7 +48,7 @@ int seq
