Re: [PATCH v4 05/10] rebase -i: also expand/collapse the SHA-1s via the rebase--helper

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 29, 2017, 11:20 UTC
Message-ID: <alpine.DEB.2.21.1.1705291302160.3610@virtualbox>
In-Reply-To: <20170526031530.29162-1-liambeguin@gmail.com>

Hi Liam,

On Thu, 25 May 2017, Liam Beguin wrote:

Show 18 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> > index 821058d452d..9444c8d6c60 100644
> > --- a/builtin/rebase--helper.c
> > +++ b/builtin/rebase--helper.c
> > @@ -24,6 +24,10 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
> >  				ABORT),
> >  		OPT_CMDMODE(0, "make-script", &command,
> >  			N_("make rebase script"), MAKE_SCRIPT),
> > +		OPT_CMDMODE(0, "shorten-sha1s", &command,
> > +			N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
> > +		OPT_CMDMODE(0, "expand-sha1s", &command,
> > +			N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
> 
> Since work is being done to convert to `struct object_id` would it
> not be best to use a more generic name instead of 'sha1'?
> maybe something like {shorten,expand}-hashs

Good point. You suggest the use of "ids" later, and I think that is an even better name: what we try to do here is to expand/reduce the commit *identifiers*. The fact that they are hexadecimal representations of hashes is an implementation detail.

Show 35 quoted lines

> > diff --git a/sequencer.c b/sequencer.c
> > index 88819a1a2a9..201d45b1677 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2437,3 +2437,60 @@ int sequencer_make_script(int keep_empty, FILE *out,
> >  	strbuf_release(&buf);
> >  	return 0;
> >  }
> > +
> > +
> > +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);
> 
> As you pointed out last time, the name of the "todo script" can be a
> source of confusion. The migration to C could be a good opportunity to
> clarify this.

True. This was simply a copy-edited part, and I should have caught that.

> I don't know which is the preferred name but we could go with
> "todo list" as it is the most common across the code base.

Yep, my next iteration will use that term.

Show 23 quoted lines

> > +	}
> > +
> > +	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 *sha1 = shorten_sha1s ?
> > +				short_commit_name(item->commit) :
> > +				oid_to_hex(&item->commit->object.oid);
> 
> We could also use 'hash' or 'ids' here instead of 'sha1'.

Absolutely!

Thank you, Johannes