Re: [PATCH v3 4/9] rebase -i: also expand/collapse the SHA-1s via the rebase--helper

> Johannes Schindelin <johannes.schindelin@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?

> > +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?