Hi Junio,
On Wed, 26 Apr 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schinde...@gmx.de> 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?
But of course it is now unused! Will fix.
> > +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.
I guess you're right. It is bug-for-bug equivalent to the previous shell
function, 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?
Sure, will fix.
Ciao,
Dscho
