On 19/06/17 05:45 AM, Johannes Schindelin wrote:

Hi Liam,

On Sat, 17 Jun 2017, Liam Beguin wrote:

quoted

On 16/06/17 09:56 AM, Johannes Schindelin wrote:

quoted

On Thu, 15 Jun 2017, Liam Beguin wrote:

quoted

On 14/06/17 09:08 AM, Johannes Schindelin wrote:

quoted

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