[PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase

[PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase

[Johannes Schindelin via GitGitGadget]

The builtin rebase and the builtin interactive rebase have been developed
independently, on purpose: Google Summer of Code rules specifically state
that students have to work on independent projects, they cannot collaborate
on the same project.

The reason is probably the very fine tradition in academia to prohibit
teamwork, which makes grading easier (at the expense of not exactly
preparing the students for the real world, unless they want to stay in
academia).

One fallout is that the rebase-in-c and rebase-i-in-c patches cause no merge
conflicts but a royal number of tests in the test suite to fail.

It is easy to explain why: rebase-in-c was developed under the assumption
that all rebase backends are implemented in Unix shell script and can be
sourced via . git-rebase--<backend>, which is no longer true with 
rebase-i-in-c, where git-rebase--interactive is a hard-linked builtin.

This patch fixes that.

Note: while this patch targets pk/rebase-in-c-6-final, it will not work
correctly without ag/rebase-i-in-c. So my suggestion is to rewrite the 
pk/rebas-in-c-6-final branch by first merging ag/rebase-i-in-c, then
applying this here patch, and only then cherry-pick "rebase: default to
using the builtin rebase".

Johannes Schindelin (1):
  builtin rebase: prepare for builtin rebase -i

 builtin/rebase.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)


base-commit: a5bb2345d2d414aba04e18531de1e0f041f0434a
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-23%2Fdscho%2Frebase-in-c-6-final-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-23/dscho/rebase-in-c-6-final-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/23
-- 
gitgitgadget

[PATCH 1/1] builtin rebase: prepare for builtin rebase -i

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It is no longer a shell script, so we need to call it in a different way
than the other backends.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index c8d632b6f4..87590047b3 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -326,6 +326,13 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
 	}
 }
 
+static const char *resolvemsg =
+N_("Resolve all conflicts manually, mark them as resolved with\n"
+"\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"
+"You can instead skip this commit: run \"git rebase --skip\".\n"
+"To abort and get back to the state before \"git rebase\", run "
+"\"git rebase --abort\".");
+
 static int run_specific_rebase(struct rebase_options *opts)
 {
 	const char *argv[] = { NULL, NULL };
@@ -333,6 +340,79 @@ static int run_specific_rebase(struct rebase_options *opts)
 	int status;
 	const char *backend, *backend_func;
 
+	if (opts->type == REBASE_INTERACTIVE) {
+		/* Run builtin interactive rebase */
+		struct child_process child = CHILD_PROCESS_INIT;
+
+		argv_array_pushf(&child.env_array, "GIT_CHERRY_PICK_HELP=%s",
+				 resolvemsg);
+		if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
+			argv_array_push(&child.env_array, "GIT_EDITOR=:");
+			opts->autosquash = 0;
+		}
+
+		child.git_cmd = 1;
+		argv_array_push(&child.args, "rebase--interactive");
+
+		if (opts->action)
+			argv_array_pushf(&child.args, "--%s", opts->action);
+		if (opts->keep_empty)
+			argv_array_push(&child.args, "--keep-empty");
+		if (opts->rebase_merges)
+			argv_array_push(&child.args, "--rebase-merges");
+		if (opts->rebase_cousins)
+			argv_array_push(&child.args, "--rebase-cousins");
+		if (opts->autosquash)
+			argv_array_push(&child.args, "--autosquash");
+		if (opts->flags & REBASE_VERBOSE)
+			argv_array_push(&child.args, "--verbose");
+		if (opts->flags & REBASE_FORCE)
+			argv_array_push(&child.args, "--no-ff");
+		if (opts->restrict_revision)
+			argv_array_pushf(&child.args,
+					 "--restrict-revision=^%s",
+					 oid_to_hex(&opts->restrict_revision->object.oid));
+		if (opts->upstream)
+			argv_array_pushf(&child.args, "--upstream=%s",
+					 oid_to_hex(&opts->upstream->object.oid));
+		if (opts->onto)
+			argv_array_pushf(&child.args, "--onto=%s",
+					 oid_to_hex(&opts->onto->object.oid));
+		if (opts->squash_onto)
+			argv_array_pushf(&child.args, "--squash-onto=%s",
+					 oid_to_hex(opts->squash_onto));
+		if (opts->onto_name)
+			argv_array_pushf(&child.args, "--onto-name=%s",
+					 opts->onto_name);
+		argv_array_pushf(&child.args, "--head-name=%s",
+				 opts->head_name ?
+				 opts->head_name : "detached HEAD");
+		if (opts->strategy)
+			argv_array_pushf(&child.args, "--strategy=%s",
+					 opts->strategy);
+		if (opts->strategy_opts)
+			argv_array_pushf(&child.args, "--strategy-opts=%s",
+					 opts->strategy_opts);
+		if (opts->switch_to)
+			argv_array_pushf(&child.args, "--switch-to=%s",
+					 opts->switch_to);
+		if (opts->cmd)
+			argv_array_pushf(&child.args, "--cmd=%s", opts->cmd);
+		if (opts->allow_empty_message)
+			argv_array_push(&child.args, "--allow-empty-message");
+		if (opts->allow_rerere_autoupdate > 0)
+			argv_array_push(&child.args, "--rerere-autoupdate");
+		else if (opts->allow_rerere_autoupdate == 0)
+			argv_array_push(&child.args, "--no-rerere-autoupdate");
+		if (opts->gpg_sign_opt)
+			argv_array_push(&child.args, opts->gpg_sign_opt);
+		if (opts->signoff)
+			argv_array_push(&child.args, "--signoff");
+
+		status = run_command(&child);
+		goto finished_rebase;
+	}
+
 	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
 	add_var(&script_snippet, "state_dir", opts->state_dir);
 
@@ -418,6 +498,7 @@ static int run_specific_rebase(struct rebase_options *opts)
 	argv[0] = script_snippet.buf;
 
 	status = run_command_v_opt(argv, RUN_USING_SHELL);
+finished_rebase:
 	if (opts->dont_finish_rebase)
 		; /* do nothing */
 	else if (status == 0) {
-- 
gitgitgadget

Re: [PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> This patch fixes that.
>
> Note: while this patch targets pk/rebase-in-c-6-final, it will not work
> correctly without ag/rebase-i-in-c. So my suggestion is to rewrite the 
> pk/rebas-in-c-6-final branch by first merging ag/rebase-i-in-c, then
> applying this here patch, and only then cherry-pick "rebase: default to
> using the builtin rebase".

Yup, that sounds like a very sensible structure.

Re: [PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:

[nice description snipped]
> This patch fixes that.

Please include this information in the commit message.  It's super
helpful to find this kind of information about why a patch does what
it does when encountering a patch later "in the wild" (in git log -S
output).

Thanks,
Jonathan

Re: [PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase

[Johannes Schindelin]

Hi Jonathan,

On Wed, 22 Aug 2018, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> 
> [nice description snipped]
> > This patch fixes that.
> 
> Please include this information in the commit message.  It's super
> helpful to find this kind of information about why a patch does what
> it does when encountering a patch later "in the wild" (in git log -S
> output).

I thought I did include the relevant part? As to the full back story: I
was repeatedly dressed down by Junio in recent attempts to include more
motivation in my commit messages. So I am reluctant to do as you say,
because Junio is the BDFL here.

Ciao,
Dscho

Re: [PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Please include this information in the commit message.  It's super
>> helpful to find this kind of information about why a patch does what
>> it does when encountering a patch later "in the wild" (in git log -S
>> output).
>
> I thought I did include the relevant part? As to the full back story: I
> was repeatedly dressed down by Junio in recent attempts to include more
> motivation in my commit messages. So I am reluctant to do as you say,
> because Junio is the BDFL here.

I do recall discouraging you from including irrelevant rant/whine in
the log message a few times in the recent past, and also I do recall
you never listening to me.  Don't make me an excuse.

I think what Jonathan finds helpful is the other half of the story
of what you did write in [1/1].  You wrote that it is no longer a
shell script and needs to follow a separate calling convention.
What was missing from that description that was given in [0/1] is
why the original "rebase-in-c" series was done while pretending that
the other effort "rebase-i-in-c" did not even exist, which made it
necessary to do this change as a separate step.

And I tend to agree that it _is_ a relevant story in this case.

Re: [PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase

[Johannes Schindelin]

Hi Junio,

On Mon, 27 Aug 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Please include this information in the commit message.  It's super
> >> helpful to find this kind of information about why a patch does what
> >> it does when encountering a patch later "in the wild" (in git log -S
> >> output).
> >
> > I thought I did include the relevant part? As to the full back story: I
> > was repeatedly dressed down by Junio in recent attempts to include more
> > motivation in my commit messages. So I am reluctant to do as you say,
> > because Junio is the BDFL here.
> 
> I do recall discouraging you from including irrelevant rant/whine in
> the log message a few times in the recent past, and also I do recall
> you never listening to me.  Don't make me an excuse.

Junio, I would really appreciate less emotional, and more professional
conduct from you.

> I think what Jonathan finds helpful is the other half of the story

I will await Jonathan's clarification.

Ciao,
Dscho

> of what you did write in [1/1].  You wrote that it is no longer a
> shell script and needs to follow a separate calling convention.
> What was missing from that description that was given in [0/1] is
> why the original "rebase-in-c" series was done while pretending that
> the other effort "rebase-i-in-c" did not even exist, which made it
> necessary to do this change as a separate step.
> 
> And I tend to agree that it _is_ a relevant story in this case.
> 
> 

Re: [PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I do recall discouraging you from including irrelevant rant/whine in
>> the log message a few times in the recent past, and also I do recall
>> you never listening to me.  Don't make me an excuse.
>
> Junio, I would really appreciate less emotional, and more professional
> conduct from you.

Which part is unprofessional?  

Being caught and corrected with truth immediately after badmouthing
another by lying may hurt, but that is your problem, not mine.

Re: [PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase

[Johannes Schindelin]

Hi Junio,

On Tue, 28 Aug 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I do recall discouraging you from including irrelevant rant/whine in
> >> the log message a few times in the recent past, and also I do recall
> >> you never listening to me.  Don't make me an excuse.
> >
> > Junio, I would really appreciate less emotional, and more professional
> > conduct from you.
> 
> Which part is unprofessional?  
> 
> Being caught and corrected with truth immediately after badmouthing
> another by lying may hurt, but that is your problem, not mine.

You did not catch me doing anything bad.

You caught me telling Jonathan about having been criticized by you, for
including background information *I* found relevant and *you* found
irrelevant. And that was very important in this context, as he asked me to
include something that I expected you to find irrelevant, too.

Of course, confusingly, this time you found it relevant, even if it was as
unrelated to the patch as in the previous case.

So I am getting the impression that your critique was not actually
professionally motivated, but very personal. And I do not appreciate that.

I do not need to say anything more about this topic.

Ciao,
Dscho

Re: [PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase

[Jonathan Nieder]

Johannes Schindelin wrote:
> On Mon, 27 Aug 2018, Junio C Hamano wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>> Jonathan Nieder wrote:

>>>> Please include this information in the commit message.  It's super
>>>> helpful to find this kind of information about why a patch does what
>>>> it does when encountering a patch later "in the wild" (in git log -S
>>>> output).
[...]
>> I think what Jonathan finds helpful is the other half of the story
>
> I will await Jonathan's clarification.

Junio's understanding is correct.

More generally, I greatly appreciate the kind of motivation and
backstory that you write in cover letters, and I wish that more of
that would find its way into the commit messages instead.  Really I
wish (and don't take this the wrong way --- I am not asking you to
write it unless it's your own itch) that GitGitGadget would put the
cover letter in single-patch series after the "---" line in the
individual patches, since that would make it easier for reviewers to
point out what content from the cover letter would be useful to add to
the commit message.

That said, this is minor and not a reason to reroll this patch.  It was
more that I wanted to give the hint for later patches.

Thanks much and hope that helps,
Jonathan

Re: [PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase

[Johannes Schindelin]

Hi Jonathan,

On Tue, 28 Aug 2018, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> > On Mon, 27 Aug 2018, Junio C Hamano wrote:
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >>> Jonathan Nieder wrote:
> 
> >>>> Please include this information in the commit message.  It's super
> >>>> helpful to find this kind of information about why a patch does what
> >>>> it does when encountering a patch later "in the wild" (in git log -S
> >>>> output).
> [...]
> >> I think what Jonathan finds helpful is the other half of the story
> >
> > I will await Jonathan's clarification.
> 
> Junio's understanding is correct.
> 
> More generally, I greatly appreciate the kind of motivation and
> backstory that you write in cover letters, and I wish that more of
> that would find its way into the commit messages instead.  Really I
> wish (and don't take this the wrong way --- I am not asking you to
> write it unless it's your own itch) that GitGitGadget would put the
> cover letter in single-patch series after the "---" line in the
> individual patches, since that would make it easier for reviewers to
> point out what content from the cover letter would be useful to add to
> the commit message.
> 
> That said, this is minor and not a reason to reroll this patch.  It was
> more that I wanted to give the hint for later patches.
> 
> Thanks much and hope that helps,

It does.

I'll "rick-roll" a new iteration, as I just realized that (contrary to my
recollection; I guess I'll need that vacation) the commit message is
*seriously* lacking. I thought I had remembered that I copy-edited the
commit message into the PR description. Clearly that was not the case.

Thanks for the clarification that triggered my looking,
Dscho

[PATCH v2 0/1] Teach the builtin rebase about the builtin interactive rebase

[Johannes Schindelin via GitGitGadget]

The builtin rebase and the builtin interactive rebase have been developed
independently, on purpose: Google Summer of Code rules specifically state
that students have to work on independent projects, they cannot collaborate
on the same project.

The reason is probably the very fine tradition in academia to prohibit
teamwork, which makes grading easier (at the expense of not exactly
preparing the students for the real world, unless they want to stay in
academia).

One fallout is that the rebase-in-c and rebase-i-in-c patches cause no merge
conflicts but a royal number of tests in the test suite to fail.

It is easy to explain why: rebase-in-c was developed under the assumption
that all rebase backends are implemented in Unix shell script and can be
sourced via . git-rebase--<backend>, which is no longer true with 
rebase-i-in-c, where git-rebase--interactive is a hard-linked builtin.

This patch fixes that.

Note: while this patch targets pk/rebase-in-c-6-final, it will not work
correctly without ag/rebase-i-in-c. So my suggestion is to rewrite the 
pk/rebas-in-c-6-final branch by first merging ag/rebase-i-in-c, then
applying this here patch, and only then cherry-pick "rebase: default to
using the builtin rebase".

Changes since v1:

 * replaced the too-terse commit message by a copy-edited version of this
   cover letter (leaving out only the rant about disallowing teamwork).

Johannes Schindelin (1):
  builtin rebase: prepare for builtin rebase -i

 builtin/rebase.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)


base-commit: ae497a044508ebaac1794dcdd7ad04f8685686b2
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-23%2Fdscho%2Frebase-in-c-6-final-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-23/dscho/rebase-in-c-6-final-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/23

Range-diff vs v1:

 1:  29d49819fa ! 1:  5403014be7 builtin rebase: prepare for builtin rebase -i
     @@ -2,8 +2,21 @@
      
          builtin rebase: prepare for builtin rebase -i
      
     -    It is no longer a shell script, so we need to call it in a different way
     -    than the other backends.
     +    The builtin rebase and the builtin interactive rebase have been
     +    developed independently, on purpose: Google Summer of Code rules
     +    specifically state that students have to work on independent projects,
     +    they cannot collaborate on the same project.
     +
     +    One fallout is that the rebase-in-c and rebase-i-in-c patches cause no
     +    merge conflicts but a royal number of tests in the test suite to fail.
     +
     +    It is easy to explain why: rebase-in-c was developed under the
     +    assumption that all rebase backends are implemented in Unix shell script
     +    and can be sourced via `. git-rebase--<backend>`, which is no longer
     +    true with rebase-i-in-c, where git-rebase--interactive is a hard-linked
     +    builtin.
     +
     +    This patch fixes that.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      

-- 
gitgitgadget

[PATCH v2 1/1] builtin rebase: prepare for builtin rebase -i

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The builtin rebase and the builtin interactive rebase have been
developed independently, on purpose: Google Summer of Code rules
specifically state that students have to work on independent projects,
they cannot collaborate on the same project.

One fallout is that the rebase-in-c and rebase-i-in-c patches cause no
merge conflicts but a royal number of tests in the test suite to fail.

It is easy to explain why: rebase-in-c was developed under the
assumption that all rebase backends are implemented in Unix shell script
and can be sourced via `. git-rebase--<backend>`, which is no longer
true with rebase-i-in-c, where git-rebase--interactive is a hard-linked
builtin.

This patch fixes that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4e69458161..99fd5d4017 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -326,6 +326,13 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
 	}
 }
 
+static const char *resolvemsg =
+N_("Resolve all conflicts manually, mark them as resolved with\n"
+"\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"
+"You can instead skip this commit: run \"git rebase --skip\".\n"
+"To abort and get back to the state before \"git rebase\", run "
+"\"git rebase --abort\".");
+
 static int run_specific_rebase(struct rebase_options *opts)
 {
 	const char *argv[] = { NULL, NULL };
@@ -333,6 +340,79 @@ static int run_specific_rebase(struct rebase_options *opts)
 	int status;
 	const char *backend, *backend_func;
 
+	if (opts->type == REBASE_INTERACTIVE) {
+		/* Run builtin interactive rebase */
+		struct child_process child = CHILD_PROCESS_INIT;
+
+		argv_array_pushf(&child.env_array, "GIT_CHERRY_PICK_HELP=%s",
+				 resolvemsg);
+		if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
+			argv_array_push(&child.env_array, "GIT_EDITOR=:");
+			opts->autosquash = 0;
+		}
+
+		child.git_cmd = 1;
+		argv_array_push(&child.args, "rebase--interactive");
+
+		if (opts->action)
+			argv_array_pushf(&child.args, "--%s", opts->action);
+		if (opts->keep_empty)
+			argv_array_push(&child.args, "--keep-empty");
+		if (opts->rebase_merges)
+			argv_array_push(&child.args, "--rebase-merges");
+		if (opts->rebase_cousins)
+			argv_array_push(&child.args, "--rebase-cousins");
+		if (opts->autosquash)
+			argv_array_push(&child.args, "--autosquash");
+		if (opts->flags & REBASE_VERBOSE)
+			argv_array_push(&child.args, "--verbose");
+		if (opts->flags & REBASE_FORCE)
+			argv_array_push(&child.args, "--no-ff");
+		if (opts->restrict_revision)
+			argv_array_pushf(&child.args,
+					 "--restrict-revision=^%s",
+					 oid_to_hex(&opts->restrict_revision->object.oid));
+		if (opts->upstream)
+			argv_array_pushf(&child.args, "--upstream=%s",
+					 oid_to_hex(&opts->upstream->object.oid));
+		if (opts->onto)
+			argv_array_pushf(&child.args, "--onto=%s",
+					 oid_to_hex(&opts->onto->object.oid));
+		if (opts->squash_onto)
+			argv_array_pushf(&child.args, "--squash-onto=%s",
+					 oid_to_hex(opts->squash_onto));
+		if (opts->onto_name)
+			argv_array_pushf(&child.args, "--onto-name=%s",
+					 opts->onto_name);
+		argv_array_pushf(&child.args, "--head-name=%s",
+				 opts->head_name ?
+				 opts->head_name : "detached HEAD");
+		if (opts->strategy)
+			argv_array_pushf(&child.args, "--strategy=%s",
+					 opts->strategy);
+		if (opts->strategy_opts)
+			argv_array_pushf(&child.args, "--strategy-opts=%s",
+					 opts->strategy_opts);
+		if (opts->switch_to)
+			argv_array_pushf(&child.args, "--switch-to=%s",
+					 opts->switch_to);
+		if (opts->cmd)
+			argv_array_pushf(&child.args, "--cmd=%s", opts->cmd);
+		if (opts->allow_empty_message)
+			argv_array_push(&child.args, "--allow-empty-message");
+		if (opts->allow_rerere_autoupdate > 0)
+			argv_array_push(&child.args, "--rerere-autoupdate");
+		else if (opts->allow_rerere_autoupdate == 0)
+			argv_array_push(&child.args, "--no-rerere-autoupdate");
+		if (opts->gpg_sign_opt)
+			argv_array_push(&child.args, opts->gpg_sign_opt);
+		if (opts->signoff)
+			argv_array_push(&child.args, "--signoff");
+
+		status = run_command(&child);
+		goto finished_rebase;
+	}
+
 	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
 	add_var(&script_snippet, "state_dir", opts->state_dir);
 
@@ -418,6 +498,7 @@ static int run_specific_rebase(struct rebase_options *opts)
 	argv[0] = script_snippet.buf;
 
 	status = run_command_v_opt(argv, RUN_USING_SHELL);
+finished_rebase:
 	if (opts->dont_finish_rebase)
 		; /* do nothing */
 	else if (status == 0) {
-- 
gitgitgadget

Re: [PATCH v2 1/1] builtin rebase: prepare for builtin rebase -i

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The builtin rebase and the builtin interactive rebase have been
> developed independently, on purpose: Google Summer of Code rules
> specifically state that students have to work on independent projects,
> they cannot collaborate on the same project.

A much better description, especially without the less relevant "the
reason probably is..." omitted from here.  The author's personal
guess, while adding it does not help understanding what is already
in the above paragraph an iota, is still a fine reading material in
the cover letter 0/1, though.

> One fallout is that the rebase-in-c and rebase-i-in-c patches cause no
> merge conflicts but a royal number of tests in the test suite to fail.
>
> It is easy to explain why: rebase-in-c was developed under the
> assumption that all rebase backends are implemented in Unix shell script
> and can be sourced via `. git-rebase--<backend>`, which is no longer
> true with rebase-i-in-c, where git-rebase--interactive is a hard-linked
> builtin.
>
> This patch fixes that.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/rebase.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)


Will replace by doing:

    $ git checkout js/rebase-in-c-5.5-work-with-rebase-i-in-c
    $ git checkout HEAD^
    $ git am -s mbox
    $ git range-diff @{-1}...
    $ git checkout -B @{-1}

    $ git checkout pk/rebase-i-in-c-6-final
    $ git rebase --onto js/rebase-in-c-5.5-work-with-rebase-i-in-c \
          js/rebase-in-c-5.5-work-with-rebase-i-in-c@{1} HEAD^0
    $ git range-diff @{-1}...
    $ git checkout -B @{-1}

to update the two topics and then rebuilding the integration
branches the usual way.  I also need to replace the "other" topic
used in this topic, so the actual procedure would be a bit more
involved than the above, though.

Thanks.

Re: [PATCH v2 1/1] builtin rebase: prepare for builtin rebase -i

[Johannes Schindelin]

Hi Junio,

On Wed, 29 Aug 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The builtin rebase and the builtin interactive rebase have been
> > developed independently, on purpose: Google Summer of Code rules
> > specifically state that students have to work on independent projects,
> > they cannot collaborate on the same project.
> 
> A much better description, especially without the less relevant "the
> reason probably is..." omitted from here.  The author's personal
> guess, while adding it does not help understanding what is already
> in the above paragraph an iota, is still a fine reading material in
> the cover letter 0/1, though.

I addressed Jonathan's concern, though.

> > One fallout is that the rebase-in-c and rebase-i-in-c patches cause no
> > merge conflicts but a royal number of tests in the test suite to fail.
> >
> > It is easy to explain why: rebase-in-c was developed under the
> > assumption that all rebase backends are implemented in Unix shell script
> > and can be sourced via `. git-rebase--<backend>`, which is no longer
> > true with rebase-i-in-c, where git-rebase--interactive is a hard-linked
> > builtin.
> >
> > This patch fixes that.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/rebase.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> 
> 
> Will replace by doing:
> 
>     $ git checkout js/rebase-in-c-5.5-work-with-rebase-i-in-c
>     $ git checkout HEAD^
>     $ git am -s mbox
>     $ git range-diff @{-1}...
>     $ git checkout -B @{-1}
> 
>     $ git checkout pk/rebase-i-in-c-6-final
>     $ git rebase --onto js/rebase-in-c-5.5-work-with-rebase-i-in-c \
>           js/rebase-in-c-5.5-work-with-rebase-i-in-c@{1} HEAD^0
>     $ git range-diff @{-1}...
>     $ git checkout -B @{-1}
> 
> to update the two topics and then rebuilding the integration
> branches the usual way.  I also need to replace the "other" topic
> used in this topic, so the actual procedure would be a bit more
> involved than the above, though.

Is there any reason why you avoid using `git rebase -ir` here? This should
be so much easier via

	git checkout pk/rebase-i-in-c-6-final
	git rebase -ir js/rebase-in-c-5.5-work-with-rebase-i-in-c^

and then inserting this at the appropriate position, followed by the `git
range-diff @{-1}...`:

	git am -s mbox
	git update-ref js/rebase-in-c-5.5-work-with-rebase-i-in-c HEAD

Ciao,
Dscho

Re: [PATCH v2 1/1] builtin rebase: prepare for builtin rebase -i

[Jeff King]

On Thu, Aug 30, 2018 at 01:03:41PM +0200, Johannes Schindelin wrote:

> > Will replace by doing:
> > 
> >     $ git checkout js/rebase-in-c-5.5-work-with-rebase-i-in-c
> >     $ git checkout HEAD^
> >     $ git am -s mbox
> >     $ git range-diff @{-1}...
> >     $ git checkout -B @{-1}
> > 
> >     $ git checkout pk/rebase-i-in-c-6-final
> >     $ git rebase --onto js/rebase-in-c-5.5-work-with-rebase-i-in-c \
> >           js/rebase-in-c-5.5-work-with-rebase-i-in-c@{1} HEAD^0
> >     $ git range-diff @{-1}...
> >     $ git checkout -B @{-1}
> > 
> > to update the two topics and then rebuilding the integration
> > branches the usual way.  I also need to replace the "other" topic
> > used in this topic, so the actual procedure would be a bit more
> > involved than the above, though.
> 
> Is there any reason why you avoid using `git rebase -ir` here? This should
> be so much easier via
> 
> 	git checkout pk/rebase-i-in-c-6-final
> 	git rebase -ir js/rebase-in-c-5.5-work-with-rebase-i-in-c^
> 
> and then inserting this at the appropriate position, followed by the `git
> range-diff @{-1}...`:
> 
> 	git am -s mbox
> 	git update-ref js/rebase-in-c-5.5-work-with-rebase-i-in-c HEAD

Related discussion, including a fantasy tangent by me (downthread):

  https://public-inbox.org/git/20180727080807.GA11932@sigill.intra.peff.net/#t

-Peff

Re: [PATCH v2 1/1] builtin rebase: prepare for builtin rebase -i

[Johannes Schindelin]

Hi Peff,

On Thu, 30 Aug 2018, Jeff King wrote:

> On Thu, Aug 30, 2018 at 01:03:41PM +0200, Johannes Schindelin wrote:
> 
> > > Will replace by doing:
> > > 
> > >     $ git checkout js/rebase-in-c-5.5-work-with-rebase-i-in-c
> > >     $ git checkout HEAD^
> > >     $ git am -s mbox
> > >     $ git range-diff @{-1}...
> > >     $ git checkout -B @{-1}
> > > 
> > >     $ git checkout pk/rebase-i-in-c-6-final
> > >     $ git rebase --onto js/rebase-in-c-5.5-work-with-rebase-i-in-c \
> > >           js/rebase-in-c-5.5-work-with-rebase-i-in-c@{1} HEAD^0
> > >     $ git range-diff @{-1}...
> > >     $ git checkout -B @{-1}
> > > 
> > > to update the two topics and then rebuilding the integration
> > > branches the usual way.  I also need to replace the "other" topic
> > > used in this topic, so the actual procedure would be a bit more
> > > involved than the above, though.
> > 
> > Is there any reason why you avoid using `git rebase -ir` here? This should
> > be so much easier via
> > 
> > 	git checkout pk/rebase-i-in-c-6-final
> > 	git rebase -ir js/rebase-in-c-5.5-work-with-rebase-i-in-c^
> > 
> > and then inserting this at the appropriate position, followed by the `git
> > range-diff @{-1}...`:
> > 
> > 	git am -s mbox
> > 	git update-ref js/rebase-in-c-5.5-work-with-rebase-i-in-c HEAD
> 
> Related discussion, including a fantasy tangent by me (downthread):
> 
>   https://public-inbox.org/git/20180727080807.GA11932@sigill.intra.peff.net/#t

I have no idea what you meant there...

Ciao,
Dscho

Re: [PATCH v2 1/1] builtin rebase: prepare for builtin rebase -i

[Jeff King]

On Fri, Aug 31, 2018 at 10:38:44PM +0200, Johannes Schindelin wrote:

> > > Is there any reason why you avoid using `git rebase -ir` here? This should
> > > be so much easier via
> > > 
> > > 	git checkout pk/rebase-i-in-c-6-final
> > > 	git rebase -ir js/rebase-in-c-5.5-work-with-rebase-i-in-c^
> > > 
> > > and then inserting this at the appropriate position, followed by the `git
> > > range-diff @{-1}...`:
> > > 
> > > 	git am -s mbox
> > > 	git update-ref js/rebase-in-c-5.5-work-with-rebase-i-in-c HEAD
> > 
> > Related discussion, including a fantasy tangent by me (downthread):
> > 
> >   https://public-inbox.org/git/20180727080807.GA11932@sigill.intra.peff.net/#t
> 
> I have no idea what you meant there...

I thought you were asking why Junio does not just use "git am" from
inside "git rebase". I asked the same thing recently, and the answer is
because he is afraid of how the two interact. I dug a little into it
(the fantasy part is that I laid out a dream for how operations like
this could safely stack).

-Peff

[PATCH v3 0/1] Teach the builtin rebase about the builtin interactive rebase

[Johannes Schindelin via GitGitGadget]

The builtin rebase and the builtin interactive rebase have been developed
independently, on purpose: Google Summer of Code rules specifically state
that students have to work on independent projects, they cannot collaborate
on the same project.

The reason is probably the very fine tradition in academia to prohibit
teamwork, which makes grading easier (at the expense of not exactly
preparing the students for the real world, unless they want to stay in
academia).

One fallout is that the rebase-in-c and rebase-i-in-c patches cause no merge
conflicts but a royal number of tests in the test suite to fail.

It is easy to explain why: rebase-in-c was developed under the assumption
that all rebase backends are implemented in Unix shell script and can be
sourced via . git-rebase--<backend>, which is no longer true with 
rebase-i-in-c, where git-rebase--interactive is a hard-linked builtin.

This patch fixes that.

Note: while this patch targets pk/rebase-in-c-6-final, it will not work
correctly without ag/rebase-i-in-c. So my suggestion is to rewrite the 
pk/rebas-in-c-6-final branch by first merging ag/rebase-i-in-c, then
applying this here patch, and only then cherry-pick "rebase: default to
using the builtin rebase".

Changes since v2:

 * Prepare for the break command, by skipping the call to finish_rebase() 
   for interactive rebases altogether (the built-in interactive rebase
   already takes care of that).
 * Remove a no-longer-used case arm (skipped by the newly-introduced code).

Changes since v1:

 * replaced the too-terse commit message by a copy-edited version of this
   cover letter (leaving out only the rant about disallowing teamwork).

Johannes Schindelin (1):
  builtin rebase: prepare for builtin rebase -i

 builtin/rebase.c | 87 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 83 insertions(+), 4 deletions(-)


base-commit: 67e0df2f391ec4177942a4d8b70e530aa5653c0d
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-23%2Fdscho%2Frebase-in-c-6-final-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-23/dscho/rebase-in-c-6-final-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/23

Range-diff vs v2:

 1:  5403014be7 ! 1:  db1652ef3e builtin rebase: prepare for builtin rebase -i
     @@ -18,6 +18,15 @@
      
          This patch fixes that.
      
     +    Please note that we also skip the finish_rebase() call for interactive
     +    rebases because the built-in interactive rebase already takes care of
     +    that. This is needed to support the upcoming `break` command that wants
     +    to interrupt the rebase with exit code 0 (and naturally wants to keep
     +    the state directory intact when doing so).
     +
     +    While at it, remove the `case` arm for the interactive rebase that is
     +    now skipped in favor of the short-cut to the built-in rebase.
     +
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
      diff --git a/builtin/rebase.c b/builtin/rebase.c
     @@ -117,6 +126,17 @@
       	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
       	add_var(&script_snippet, "state_dir", opts->state_dir);
       
     +@@
     + 		backend = "git-rebase--am";
     + 		backend_func = "git_rebase__am";
     + 		break;
     +-	case REBASE_INTERACTIVE:
     +-		backend = "git-rebase--interactive";
     +-		backend_func = "git_rebase__interactive";
     +-		break;
     + 	case REBASE_MERGE:
     + 		backend = "git-rebase--merge";
     + 		backend_func = "git_rebase__merge";
      @@
       	argv[0] = script_snippet.buf;
       
     @@ -124,4 +144,8 @@
      +finished_rebase:
       	if (opts->dont_finish_rebase)
       		; /* do nothing */
     ++	else if (opts->type == REBASE_INTERACTIVE)
     ++		; /* interactive rebase cleans up after itself */
       	else if (status == 0) {
     + 		if (!file_exists(state_dir_path("stopped-sha", opts)))
     + 			finish_rebase(opts);

-- 
gitgitgadget

[PATCH v3 1/1] builtin rebase: prepare for builtin rebase -i

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The builtin rebase and the builtin interactive rebase have been
developed independently, on purpose: Google Summer of Code rules
specifically state that students have to work on independent projects,
they cannot collaborate on the same project.

One fallout is that the rebase-in-c and rebase-i-in-c patches cause no
merge conflicts but a royal number of tests in the test suite to fail.

It is easy to explain why: rebase-in-c was developed under the
assumption that all rebase backends are implemented in Unix shell script
and can be sourced via `. git-rebase--<backend>`, which is no longer
true with rebase-i-in-c, where git-rebase--interactive is a hard-linked
builtin.

This patch fixes that.

Please note that we also skip the finish_rebase() call for interactive
rebases because the built-in interactive rebase already takes care of
that. This is needed to support the upcoming `break` command that wants
to interrupt the rebase with exit code 0 (and naturally wants to keep
the state directory intact when doing so).

While at it, remove the `case` arm for the interactive rebase that is
now skipped in favor of the short-cut to the built-in rebase.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 87 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 83 insertions(+), 4 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1a697d70c9..20f7159cf2 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -326,6 +326,13 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
 	}
 }
 
+static const char *resolvemsg =
+N_("Resolve all conflicts manually, mark them as resolved with\n"
+"\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"
+"You can instead skip this commit: run \"git rebase --skip\".\n"
+"To abort and get back to the state before \"git rebase\", run "
+"\"git rebase --abort\".");
+
 static int run_specific_rebase(struct rebase_options *opts)
 {
 	const char *argv[] = { NULL, NULL };
@@ -333,6 +340,79 @@ static int run_specific_rebase(struct rebase_options *opts)
 	int status;
 	const char *backend, *backend_func;
 
+	if (opts->type == REBASE_INTERACTIVE) {
+		/* Run builtin interactive rebase */
+		struct child_process child = CHILD_PROCESS_INIT;
+
+		argv_array_pushf(&child.env_array, "GIT_CHERRY_PICK_HELP=%s",
+				 resolvemsg);
+		if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
+			argv_array_push(&child.env_array, "GIT_EDITOR=:");
+			opts->autosquash = 0;
+		}
+
+		child.git_cmd = 1;
+		argv_array_push(&child.args, "rebase--interactive");
+
+		if (opts->action)
+			argv_array_pushf(&child.args, "--%s", opts->action);
+		if (opts->keep_empty)
+			argv_array_push(&child.args, "--keep-empty");
+		if (opts->rebase_merges)
+			argv_array_push(&child.args, "--rebase-merges");
+		if (opts->rebase_cousins)
+			argv_array_push(&child.args, "--rebase-cousins");
+		if (opts->autosquash)
+			argv_array_push(&child.args, "--autosquash");
+		if (opts->flags & REBASE_VERBOSE)
+			argv_array_push(&child.args, "--verbose");
+		if (opts->flags & REBASE_FORCE)
+			argv_array_push(&child.args, "--no-ff");
+		if (opts->restrict_revision)
+			argv_array_pushf(&child.args,
+					 "--restrict-revision=^%s",
+					 oid_to_hex(&opts->restrict_revision->object.oid));
+		if (opts->upstream)
+			argv_array_pushf(&child.args, "--upstream=%s",
+					 oid_to_hex(&opts->upstream->object.oid));
+		if (opts->onto)
+			argv_array_pushf(&child.args, "--onto=%s",
+					 oid_to_hex(&opts->onto->object.oid));
+		if (opts->squash_onto)
+			argv_array_pushf(&child.args, "--squash-onto=%s",
+					 oid_to_hex(opts->squash_onto));
+		if (opts->onto_name)
+			argv_array_pushf(&child.args, "--onto-name=%s",
+					 opts->onto_name);
+		argv_array_pushf(&child.args, "--head-name=%s",
+				 opts->head_name ?
+				 opts->head_name : "detached HEAD");
+		if (opts->strategy)
+			argv_array_pushf(&child.args, "--strategy=%s",
+					 opts->strategy);
+		if (opts->strategy_opts)
+			argv_array_pushf(&child.args, "--strategy-opts=%s",
+					 opts->strategy_opts);
+		if (opts->switch_to)
+			argv_array_pushf(&child.args, "--switch-to=%s",
+					 opts->switch_to);
+		if (opts->cmd)
+			argv_array_pushf(&child.args, "--cmd=%s", opts->cmd);
+		if (opts->allow_empty_message)
+			argv_array_push(&child.args, "--allow-empty-message");
+		if (opts->allow_rerere_autoupdate > 0)
+			argv_array_push(&child.args, "--rerere-autoupdate");
+		else if (opts->allow_rerere_autoupdate == 0)
+			argv_array_push(&child.args, "--no-rerere-autoupdate");
+		if (opts->gpg_sign_opt)
+			argv_array_push(&child.args, opts->gpg_sign_opt);
+		if (opts->signoff)
+			argv_array_push(&child.args, "--signoff");
+
+		status = run_command(&child);
+		goto finished_rebase;
+	}
+
 	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
 	add_var(&script_snippet, "state_dir", opts->state_dir);
 
@@ -395,10 +475,6 @@ static int run_specific_rebase(struct rebase_options *opts)
 		backend = "git-rebase--am";
 		backend_func = "git_rebase__am";
 		break;
-	case REBASE_INTERACTIVE:
-		backend = "git-rebase--interactive";
-		backend_func = "git_rebase__interactive";
-		break;
 	case REBASE_MERGE:
 		backend = "git-rebase--merge";
 		backend_func = "git_rebase__merge";
@@ -418,8 +494,11 @@ static int run_specific_rebase(struct rebase_options *opts)
 	argv[0] = script_snippet.buf;
 
 	status = run_command_v_opt(argv, RUN_USING_SHELL);
+finished_rebase:
 	if (opts->dont_finish_rebase)
 		; /* do nothing */
+	else if (opts->type == REBASE_INTERACTIVE)
+		; /* interactive rebase cleans up after itself */
 	else if (status == 0) {
 		if (!file_exists(state_dir_path("stopped-sha", opts)))
 			finish_rebase(opts);
-- 
gitgitgadget

Re: [PATCH v3 0/1] Teach the builtin rebase about the builtin interactive rebase

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> Note: while this patch targets pk/rebase-in-c-6-final, it will not work
> correctly without ag/rebase-i-in-c. So my suggestion is to rewrite the 
> pk/rebas-in-c-6-final branch by first merging ag/rebase-i-in-c, then
> applying this here patch, and only then cherry-pick "rebase: default to
> using the builtin rebase".

Is this a stale comment in the context of v3, where we pretty much
know how the resulting topic should intertwine with other topics?  I
am trying to see if I have do to anything differently this time, or
just replacing the single commit while keeping the structure around
that commit the same is fine.

Also, I see there is a new iteration v8 of ag/rebase-i-in-c on the
list, sent on Sep 27th (you were CC'ed but I suspect it was before
you came back).  Are you happy with that update?  Otherwise, we
should make sure that topic is solid enough before extending
(meaning: I'd replace this one while keeping ag/rebase-i-in-c that
has been cooking in 'pu', without updating the latter).

> Changes since v2:
>
>  * Prepare for the break command, by skipping the call to finish_rebase() 
>    for interactive rebases altogether (the built-in interactive rebase
>    already takes care of that).

Thanks.

Re: [PATCH v3 0/1] Teach the builtin rebase about the builtin interactive rebase

[Johannes Schindelin]

Hi Junio,

On Sun, 7 Oct 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > Note: while this patch targets pk/rebase-in-c-6-final, it will not work
> > correctly without ag/rebase-i-in-c. So my suggestion is to rewrite the 
> > pk/rebas-in-c-6-final branch by first merging ag/rebase-i-in-c, then
> > applying this here patch, and only then cherry-pick "rebase: default to
> > using the builtin rebase".
> 
> Is this a stale comment in the context of v3, where we pretty much
> know how the resulting topic should intertwine with other topics?  I
> am trying to see if I have do to anything differently this time, or
> just replacing the single commit while keeping the structure around
> that commit the same is fine.

Just replacing the single commit. For technical reasons, I still have to
target pk/rebase-in-c-6-final in my branch.

> Also, I see there is a new iteration v8 of ag/rebase-i-in-c on the
> list, sent on Sep 27th (you were CC'ed but I suspect it was before
> you came back).  Are you happy with that update?

To be quite honest, I did not yet have time to look over it, but I
verified that it has my suggested fix for the -S option.

Ciao,
Dscho