Avoid bad rebase amend #582

ParthGala2k · 2020-03-15T14:46:05Z

This is a draft PR regarding this[#210] issue.
I built and tested it on a test repository and it doesn't yet solve the issue.
Although I have currently done as per your recommendation of writing the pick
commit to a file and then comparing it while processing the fixup, I was wondering
what difference it would make to write the commit to a variable rather(since we are
just copying a single commit) ie is there a particular advantage in writing to file ?

Also I'm having trouble running the program on gdb, is there any reference for that?

dscho · 2020-03-15T22:02:44Z

Also I'm having trouble running the program on gdb, is there any reference for that?

The best I can offer is https://github.com/git-for-windows/git/wiki/Debugging-Git.

dscho

This is already a pretty good start!

I left a couple suggestions, and I think that the offsets used in the peek_command() calls are too far off, and that's most likely why the test suite fails. All of my concerns will be addressed very easily, I am sure.

dscho · 2020-03-15T22:11:59Z

sequencer.c

+ * has merge conflicts with the base here and we want to avoid amending
+ * f1 onto p1 if p is skipped.
+ */
+static GIT_PATH_FUNC(rebase_path_bad_fixup, "rebase-merge/avoid-bad-fixup")


To make this code easier to read, I think we should not skimp on the avoid part and call the variable rebase_path_avoid_bad_fixup.

dscho · 2020-03-15T22:16:20Z

sequencer.c

@@ -3811,6 +3821,7 @@ static int pick_commits(struct repository *r,
 		struct todo_item *item = todo_list->items + todo_list->current;
 		const char *arg = todo_item_get_arg(todo_list, item);
 		int check_todo = 0;
+		struct object_id head;


Let's move this further down into the if (item->command <= TODO_SQUASH) block, to restrict the scope further.

dscho · 2020-03-15T22:32:56Z

sequencer.c

+					return error(_("cannot read HEAD"));
+
+				p = oid_to_hex(&head);
+				write_message(p, strlen(p), rebase_path_bad_fixup(), 1);


Good idea, but I would actually write this only in case of a failed TODO_PICK, TODO_EDIT or TODO_REWORD, i.e. if is_pick_or_similar() returns 1 (but make sure that we don't do it if is_fixup() returns 1).

Further, I think that peek_command(todo_list, 1) is looking too far in the future. We are about to call do_pick_commit() with item->command, so that's the command to look at. Besides, at this point peek_command(todo_list, 0) already looks at the next command.

So I think that the logic should go more like this:

struct strbuf buf = STRBUF_INIT; struct object_id head, previous_head; [...] res = 0; if (is_fixup(item->command)) { if (read_oneliner(&buf, rebase_path_avoid_bad_fixup(), 1) > 0) { if (get_oid(buf.buf, &previous_head)) { error(_("invalid file contents '%s' of '%s'"), buf.buf, rebase_path_avoid_bad_fixup()); strbuf_release(&buf); } strbuf_release(&buf); if (get_oid("HEAD", &head)) return error(_("could not read HEAD")); if (oideq(&head, &previous_head)) res = error(_("commit to fix up was skipped")); /* reschedule */ } } else if (is_pick_or_similar(item->command) && is_fixup(peek_command(todo_list, 0)) { char hex[GIT_MAX_HEXSZ + 1]; if (get_oid("HEAD", &head)) return error(_("cannot read HEAD")); oid_to_hex_r(hex, &head); write_message(hex, the_hash_algo->hexsz, rebase_path_avoid_bad_fixup(), 0); } if (!res) res = do_pick_commit(r, item->command, item->commit, opts, is_final_fixup(todo_list), &check_todo);

I still have 2 questions right now,

Is there any particular advantage in writing to file, since we are only writing a single value ?

What does oid_to_hex_r do differently, ie is it equivalent to writing
hex = oid_to_hex(&head); ?

struct strbuf buf = STRBUF_INIT;
And how about reducing this variable's scope even further ? Into

if (is_fixup(item->command)) { if (read_oneliner(&buf, rebase_path_avoid_bad_fixup(), 1) > 0) { if (get_oid(buf.buf, &previous_head)) { error(_("invalid file contents '%s' of '%s'"), buf.buf, rebase_path_avoid_bad_fixup()); strbuf_release(&buf); } strbuf_release(&buf); if (get_oid("HEAD", &head)) return error(_("could not read HEAD")); if (oideq(&head, &previous_head)) res = error(_("commit to fix up was skipped")); /* reschedule */ } }

The value is short all right, but remember: the most likely scenario is that the pick failed due to merge conflicts, the user --skips it, and then the fixup has no correct commit to target.

In other words: git stopped running between writing the value and reading it. Those are two separate processes, therefore the value must be persisted to disk.

About reducing the scope even further: sure, as long indentation issues don't make the code unreadable?

phil-blain · 2020-03-17T02:29:25Z

Hi @ParthGala2k !

Also I'm having trouble running the program on gdb, is there any reference for that?

The CodingGuidelines document also has some guidance for using the system-installed GDB.

If you compiled GDB yourself you will need to invoke it like this:

GIT_DEBUGGER="/path/to/gdb --args" ./bin-wrappers/git <command> <args...>

I you are on macOS and want to use LLDB, that would be:

GIT_DEBUGGER="lldb --" ./bin-wrappers/git <command> <args...>

Sometimes Git command spawn other Git commands under the hood, using fork+exec. You will recognize this by a call to either run_command or more rarely directly to start_command.

If the code your are trying to debug is in an instance of git spawned (fork+exec) by the main git process, you will need to use the set detach-on-fork off option in GDB, which works on Linux but not on macOS (I don't know about anything about Windows). I had success doing that on Ubuntu 17.10 using a self-compiled GDB 8.3.1, but not with the system-installed GDB 7.1.2.

Another option is to put a call to sleep() just after the fork in start_command, then open another instance of GDB and attach to the new process, e.g. with

gdb -p $(pgrep -n git)

Then you can control both instances independently.

Addendum: there is also some interesting guidelines on the old wiki, that might still be useful for debugging tests: https://git.wiki.kernel.org/index.php/What_to_do_when_a_test_fails

ParthGala2k · 2020-03-31T15:25:51Z

@dscho, any updates here ?

dscho · 2020-03-31T15:47:52Z

Didn't you get excellent feedback from @phil-blain ?

ParthGala2k · 2020-04-02T16:13:21Z

I meant to say about the pull request, not gdb here. Although the tests have passed, the the purpose is not yet solved.
Here's my doubt about gdb, how do I start debugging the git code while I run a test repository which I have made specifically for catching this bug.

phil-blain · 2020-04-02T18:49:41Z

Here's my doubt about gdb, how do I start debugging the git code while I run a test repository which I have made specifically for catching this bug.

If by "run a test repository" you mean running a specifically-crafted test case in the Git test suite, then the old wiki page which I linked above mentions an approach that might work (I haven't tried it myself though):

In really hard cases, you might need to run a failing command in the debugger (typically gdb, although you might prefer a graphical version thereof). If you prefix a call with gdb <&6 >&3 2>&4, you might need to pass the --args option to gdb to prevent gdb from eating Git's command line options (e.g. git commit -m initial && becomes now gdb <&6 >&3 2>&4 --args git commit -m initial).

So if git <command> <args> fails in your test, replacing that line with

 gdb <&6 >&3 2>&4 --args git <command> <args>

should run the test and open GDB at the right point, where you could insert breakpoints and then run the command in question.

dscho · 2020-04-02T21:12:08Z

So if git <command> <args> fails in your test, replacing that line with
 gdb <&6 >&3 2>&4 --args git <command> <args>
should run the test and open GDB at the right point, where you could insert breakpoints and then run the command in question.

I think these days, you can simply prefix the git command with debug and that should work, too.

This file stores info of the commit at a pick before a pick-fixup/squash chain eg. for this series of commands p1---p---f1---s---f.... The commit info for HEAD corresponding to pick p1 will be stored in file here. Although the name suggests bad_fixup it is actually pick p that has merge conflicts with the base here and we want to avoid applying fixup to f1 onto p1 if p is skipped. Currently if pick p results in a conflict, and is skipped f1 gets fixed up to p1. Signed-off-by: Parth Gala <[email protected]>

The condition here in the 'else if' statement verifies that the commit whose hash is written to file is actually that of pick p1 since we are writing before the call to do_pick_commit, refer f7f082c9bf p could also be an edit or reword but not fixup or squash p1---p---f1---f---f... The writing to file is done based upon the implementation of the `intend_to_amend` function in sequencer.c which similarly copies a single hash to a file. The `if` ensures that we are dealing with a fixup whose preceding pick/edit/reword had merge conflicts and was skipped. The file contents are compared with the current head position and if the two are same it means that the conflicting pick was skipped. Hence we error out without amending HEAD. Signed-off-by: Parth Gala <[email protected]>

ParthGala2k · 2020-04-18T19:00:25Z

This version works as expected.
And I have written a test case in a separate file because I was unsure in which file to write it.
But here are its contents :

#!/bin/sh
#

test_description='git rebase -i master to-be-rebased'

. ./test-lib.sh

. "$TEST_DIRECTORY"/lib-rebase.sh

test_expect_success setup '
        echo line 1 > file &&
        echo line 2 >> file &&
        git add file &&
        git commit -m "Initial commit" &&

        sed -i "1s/line/change/1" file &&
        git commit -a -m "Change to line 1" &&

        git checkout -b to-be-rebased HEAD~ &&
        sed -i "1s/line/another change/1" file &&
        git commit -a -m "Another change to line 1" &&

        echo random_change >> file &&
        git commit -a -m "Random change on line 3"
'
test_expect_failure 'git rebase --skip should fail here' '
        {
                set_fake_editor &&
                test_must_fail env FAKE_LINES="1 fixup 2" \
                git rebase -i master to-be-rebased
        } &&
        git rebase --skip
'

test_done

I will include it in the next PR after feedback on it.
And thanks, @phil-blain, all those links above were extremely useful 👍

dscho · 2020-04-18T20:18:11Z

Given that this is all about the --autosquash mode, I would believe that https://github.com/git/git/blob/master/t/t3415-rebase-autosquash.sh would make for a good home. There might even be enough setup there already to avoid having to do another setup step.

ParthGala2k mentioned this pull request Mar 15, 2020

rebase -i: error on fixup/squash when the corresponding pick was skipped #210

Open

dscho requested changes Mar 15, 2020

View reviewed changes

ParthGala2k force-pushed the avoid_bad_rebase_amend branch from 7aa3a0e to d1ef114 Compare March 18, 2020 10:17

ParthGala2k added 2 commits April 19, 2020 00:13

ParthGala2k force-pushed the avoid_bad_rebase_amend branch from d1ef114 to c11ad1c Compare April 18, 2020 18:54

phil-blain mentioned this pull request Jun 2, 2020

Add a page with hints about debugging Git git/git.github.io#435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid bad rebase amend #582

Avoid bad rebase amend #582

ParthGala2k commented Mar 15, 2020

dscho commented Mar 15, 2020

dscho left a comment

dscho Mar 15, 2020

dscho Mar 15, 2020

dscho Mar 15, 2020

ParthGala2k Mar 17, 2020 •

edited

Loading

ParthGala2k Mar 17, 2020

dscho Mar 18, 2020

phil-blain commented Mar 17, 2020 •

edited

Loading

ParthGala2k commented Mar 31, 2020

dscho commented Mar 31, 2020

ParthGala2k commented Apr 2, 2020

phil-blain commented Apr 2, 2020 •

edited

Loading

dscho commented Apr 2, 2020

ParthGala2k commented Apr 18, 2020 •

edited

Loading

dscho commented Apr 18, 2020

Avoid bad rebase amend #582

Are you sure you want to change the base?

Avoid bad rebase amend #582

Conversation

ParthGala2k commented Mar 15, 2020

dscho commented Mar 15, 2020

dscho left a comment

Choose a reason for hiding this comment

dscho Mar 15, 2020

Choose a reason for hiding this comment

dscho Mar 15, 2020

Choose a reason for hiding this comment

dscho Mar 15, 2020

Choose a reason for hiding this comment

ParthGala2k Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

ParthGala2k Mar 17, 2020

Choose a reason for hiding this comment

dscho Mar 18, 2020

Choose a reason for hiding this comment

phil-blain commented Mar 17, 2020 • edited Loading

ParthGala2k commented Mar 31, 2020

dscho commented Mar 31, 2020

ParthGala2k commented Apr 2, 2020

phil-blain commented Apr 2, 2020 • edited Loading

dscho commented Apr 2, 2020

ParthGala2k commented Apr 18, 2020 • edited Loading

dscho commented Apr 18, 2020

ParthGala2k Mar 17, 2020 •

edited

Loading

phil-blain commented Mar 17, 2020 •

edited

Loading

phil-blain commented Apr 2, 2020 •

edited

Loading

ParthGala2k commented Apr 18, 2020 •

edited

Loading