Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 19, 2017, 09:45 UTC
Message-ID: <alpine.DEB.2.21.1.1706191127450.57822@virtualbox>
In-Reply-To: <529f6cc7-ab2b-72bb-bc51-f4cca4e98652@gmail.com>

Hi Liam,

On Sat, 17 Jun 2017, Liam Beguin wrote:

Show 38 quoted lines

> On 16/06/17 09:56 AM, Johannes Schindelin wrote:
> 
> > On Thu, 15 Jun 2017, Liam Beguin wrote:
> > 
> >> On 14/06/17 09:08 AM, Johannes Schindelin wrote:
> >>> diff --git a/sequencer.c b/sequencer.c
> >>> index a697906d463..a0e020dab09 100644
> >>> --- a/sequencer.c
> >>> +++ b/sequencer.c
> >>> @@ -2640,3 +2640,110 @@ int check_todo_list(void)
> >>>  
> >>>  	return res;
> >>>  }
> >>> +
> >>> +/* skip picking commits whose parents are unchanged */
> >>> +int skip_unnecessary_picks(void)
> >>> +{
> >>> +	const char *todo_file = rebase_path_todo();
> >>> +	struct strbuf buf = STRBUF_INIT;
> >>> +	struct todo_list todo_list = TODO_LIST_INIT;
> >>> +	struct object_id onto_oid, *oid = &onto_oid, *parent_oid;
> >>> +	int fd, i;
> >>> +
> >>> +	if (!read_oneliner(&buf, rebase_path_onto(), 0))
> >>> +		return error(_("could not read 'onto'"));
> >>> +	if (get_sha1(buf.buf, onto_oid.hash)) {
> >>
> >> I missed this last time but we could also replace `get_sha1` with
> >> `get_oid`
> > 
> > Good point!
> > 
> > I replaced this locally and force-pushed, but there is actually little
> > chance of this patch series being integrated in a form with which I
> > would be comfortable.
> 
> Is there any chance of this moving forward? I was hoping to resend my
> path to abbreviate command names on top of this.

We are at an impasse right now.

Junio wants me to change this code:

        revs.pretty_given = 1;
        git_config_get_string("rebase.instructionFormat", &format);
        if (!format || !*format) {
                free(format);
                format = xstrdup("%s");
        }
        get_commit_format(format, &revs);
        free(format);
        pp.fmt = revs.commit_format;
        pp.output_encoding = get_log_output_encoding();

        if (setup_revisions(argc, argv, &revs, NULL) > 1)
	...

which is reasonably compile-time safe, to something like this:

	struct strbuf userformat = STRBUF_INIT;
	struct argv_array args = ARGV_ARRAY_INIT;
	...
	for (i = 0; i < argc; i++)
		argv_array_push(&args, argv[i]);
        git_config_get_string("rebase.instructionFormat", &format);
        if (!format || !*format)
		argv_array_push(&args, "--format=%s");
	else {
		strbuf_addf(&userformat, "--format=%s", format);
                argv_array_push(&args, userformat.buf);
        }

        if (setup_revisions(args.argc, args.argv, &revs, NULL) > 1)
	...
	argv_array_clear(&args);
	strbuf_release(&userformat);

which is not compile-time safe, harder to read, sloppy coding in my book.

The reason for this suggestion is that one of the revision machinery's implementation details is an ugly little semi-secret: the pretty-printing machinery uses a global state, and that is why we need the "pretty_given" flag in the first place.

Junio thinks that it would be better to not use the pretty_given flag in this patch series. I disagree: It would be better to use the pretty_given flag, also as a good motivator to work on removing the global state in the pretty-printing machinery.

Junio wants to strong-arm me into accepting authorship for this sloppy coding, and I simply won't do it.

That's why we are at an impasse right now.

I am really, really sorry that this affects your patch series, as I had not foreseen Junio's insistence on the sloppy coding when I suggested that you rebase your work on top of my patch series. I just was really unprepared for this contention, and I am still surprised/shocked that this is even an issue up for discussion.

Be that as it may, I see that this is a very bad experience for a motivated contributor such as yourself. I am very sorry about that!

So it may actually be better for you to go forward with your original patch series targeting git-rebase--interactive.sh.

My apologies, Dscho