Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dscho opened this issue May 17, 2019 · 4 comments
Open
Labels
bug Something isn't working

Comments

@dscho
Copy link
Member

dscho commented May 17, 2019

Imagine this todo list:

pick a123 this will conflict
fixup b124 want to fix up

Now, as the oneline suggests, let's assume that the pick results in merge conflicts, and let's further assume that the user decided to skip it via git rebase --skip.

Obviously, the fixup should not blindly amend HEAD at this point, so we will probably want to error out with a helpful message.

@dscho dscho added the bug Something isn't working label May 17, 2019
@ParthGala2k
Copy link

I am unable to wrap my head around how pick causes a merge conflict here.
From what I understand, fixup will cause changes of b124 to be fused into commit a123, but since b124 was already made after a123 why would it conflict ?

@dscho
Copy link
Member Author

dscho commented Feb 13, 2020

I am unable to wrap my head around how pick causes a merge conflict here.

Imagine that the commit a123 changes the first line of the README, say, fixing a typo. Then, imagine that you rebase that on top of a commit that actually rewrote the entire first line of the README.

That'll conflict, right?

Now, let's imagine that the fixup commit b124 fixes another typo, but this time in another part of the README, and that this still applies after rebasing.

Obviously, we would want to skip a123 but not b124. But! We do not want b124 to be a fixup anymore!

@ParthGala2k
Copy link

Sorry for being away after asking so long.
I tried searching for the function where a merge conflict signal is raised which seems to be in merge-recursive.c in handle_content_merge()

if (!mfi->clean) {
                if (S_ISGITLINK(mfi->blob.mode))
                        reason = _("submodule");
                output(opt, 1, _("CONFLICT (%s): Merge conflict in %s"),
                                reason, path);
                if (ci && !df_conflict_remains)
                        if (update_stages(opt, path, o, a, b))
                                return -1;
}

But then the parts where the todo for interactive rebase is saved and HEAD is updated upon rebase --skip is in sequencer.c.
I suspect we might only have to put an extra level of checking somewhere something like
if('the next action in todo is a fixup or squash' && 'current commit has merge conflicts')
upon the command git rebase --skip.
But since sequencer does so many things I am unsure if I am right.
Can you guide me here ?

@dscho
Copy link
Member Author

dscho commented Feb 25, 2020

I tried searching for the function where a merge conflict signal is raised

That's probably not all that important for this ticket. It would probably make more sense to write a file into the state directory for the specific purpose of detecting when a fixup/squash is about to be applied but the initial commit of that fixup/squash was not successful.

One option I can think of is to write a file before the initial pick before a fixup/squash chain (using peek_command() to detect that situation) with the current commit as file contents. When the fixup/squash is processed, this file can be read (if it exists) and if HEAD is still at that commit, we can refuse.

You will have to familiarize yourself with the pick_commits() and the do_pick_commits() functions for this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants