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

[Liam Beguin <liambeguin@gmail.com>]

On 19/06/17 05:45 AM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sat, 17 Jun 2017, Liam Beguin wrote:
> 
>> 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!

It's not such a bad experience, I've found the people on the list to 
be quite welcoming and supportive.

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

I'll wait a bit longer to see how this goes and if it doesn't move, I'll
try re-sending an update to that series.

> 
> My apologies,
> Dscho
> 

Thanks,
Liam