Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 214af0372ba..52a19e0bdb3 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -774,11 +774,11 @@ transform_todo_ids () {
> }
>
> expand_todo_ids() {
> - transform_todo_ids
> + git rebase--helper --expand-sha1s
> }
>
> collapse_todo_ids() {
> - transform_todo_ids --short
> + git rebase--helper --shorten-sha1s
> }
Obviously correct ;-) But doesn't this make transform_todo_ids ()
helper unused and removable?
> +int transform_todo_ids(int shorten_sha1s)
> +{
> + const char *todo_file = rebase_path_todo();
> + struct todo_list todo_list = TODO_LIST_INIT;
> + int fd, res, i;
> + FILE *out;
> +
> + 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);
> + return error(_("could not read '%s'."), todo_file);
> + }
> + close(fd);
> +
> + res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
> + if (res) {
> + todo_list_release(&todo_list);
> + return error(_("unusable instruction sheet: '%s'"), todo_file);
> + }
> +
> + out = fopen(todo_file, "w");
The usual "open lockfile, write to it and then rename" dance is not
necessary for the purpose of preventing other people from reading
this file while we are writing to it. But if we fail inside this
function before we fclose(3) "out", the user will lose the todo
list. It probably is not a big deal, though.
> + 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 {
> + int eoc = strcspn(p, " \t");
> + const char *sha1 = shorten_sha1s ?
> + short_commit_name(item->commit) :
> + oid_to_hex(&item->commit->object.oid);
> +
> + if (!eoc) {
> + p += strspn(p, " \t");
> + eoc = strcspn(p, " \t");
> + }
It would be much easier to follow the logic if "int eoc" above were
a mere declaration without initialization and "skip to the
whitespaces" is done immediately before this if() statement. It's
not like the initialized value of eoc is needed there because it
participates in the computation of sha1, and also having the
assignment followed by "oops, the line begins with a whitespace"
recovery that is done here.
Wouldn't it be simpler to do:
else {
int eoc;
const char *sha1 = ...
p += strspn(p, " \t"); /* skip optional indent */
eoc = strcspn(p, " \t"); /* grab the command word */
without conditional?
> + fprintf(out, "%.*s %s %.*s\n",
> + eoc, p, sha1, item->arg_len, item->arg);
> + }
> + }
> + fclose(out);
> + todo_list_release(&todo_list);
> + return 0;
> +}
> diff --git a/sequencer.h b/sequencer.h
> index 83f2943b7a9..47a81034e76 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -48,6 +48,8 @@ int sequencer_remove_state(struct replay_opts *opts);
> int sequencer_make_script(int keep_empty, FILE *out,
> int argc, const char **argv);
>
> +int transform_todo_ids(int shorten_sha1s);
> +
> extern const char sign_off_header[];
>
> void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);