gitbutler-edit-mode: make edit mode commit have parent#12935
gitbutler-edit-mode: make edit mode commit have parent#12935jonathantanmy2 wants to merge 1 commit intomasterfrom
Conversation
jonathantanmy2
requested review from
Byron,
Caleb-T-Owens and
krlvi
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
When entering edit mode, a temporary commit needs to be created if either the commit being edited (C) or the parent of the commit being edited (P) is conflicted, so that (among other things) `git status` shows something reasonable. Currently, this temporary commit is created with no parents and a commit message duplicated from P. This is probably less user-friendly than it could be - the lack of parents means that the user cannot see C's place in the commit graph, and the duplicated commit message might make the user wonder if P was also changed. Therefore, when entering edit mode, if a temporary commit is needed, create it with parent P and a commit message that informs the user of what's going on.
jonathantanmy2
force-pushed
the
jt/parent
branch
from
f1a1550 to
d5af032
Compare
|
There are some e2e-playwright tests that are failing because I've changed the commit message of the temporary commit. If people think my change is a good idea, I'll go back and fix those. |
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
Thanks so much, I can't wait to land this to help with VBH removal!
| const SELF_CONFLICTED :&[u8]= b"(temporary commit)\n\nThis commit was created to serve as a temporary HEAD because a conflicted commit is being edited.\n"; | ||
|
|
||
| const PARENT_CONFLICTED:&[u8] = b"(temporary commit)\n\nThis commit was created to serve as a temporary HEAD because the parent of the commit being edited is conflicted.\n"; |
There was a problem hiding this comment.
Is this something we have to point out?
My intuitive preference would be that everything looks 'normal', and that certainly includes the commit message.
There was a problem hiding this comment.
Let's take the SELF_CONFLICTED case. The commit graph in question is something like:
C (conflicted)
|
P
and after entering edit mode editing C, it's (X is the temporary commit, which we apparently need because we want to use the "ours" tree as a base):
X [HEAD] + changes in index
|
P
So the question is what should the commit message of X be. Previously, it was the same as P but now I have switched it to what you see in the PR. I think what's in the PR makes more sense than duplicating the commit message of P (and thus seeing the same commit message twice), but maybe I'm overlooking something.
For reference, the PARENT_CONFLICTED case has the following commit graph:
C
|
P (conflicted)
and after entering edit mode:
X [HEAD] + changes in index
|
P (conflicted)
X is the autoresolution of P.
There was a problem hiding this comment.
Ah, I see. Maybe this is also something that can be more Git like by asking what Git does in similar situations.
So the question is what should the commit message of X be. Previously, it was the same as P [..]
Since we are at the desired commit and want to do something like amend on it, it seems the C should have the commit message of C, and not P (or something temporary).
In a way, the state is like we emptied the commit we want to edit, and the next commit (or the button press in GB) should amend to that commit, so the commit message I expect would always be the one of the commit I am editing, conflicted or not.
To me there is no other choice than that, so I wonder if I am missing something crucial here despite the excellent explanation of what's happening.
There was a problem hiding this comment.
So the question is what should the commit message of X be. Previously, it was the same as P [..]
Since we are at the desired commit and want to do something like
amendon it, it seems the C should have the commit message of C, and not P (or something temporary).
The current behavior is as if we git checkout C and git reset --soft HEAD^ (typical commands to use when splitting a commit). But I do like your proposal that we are doing something like amend - it is very similar to what we do when doing git rebase -i (which I do think is the equivalent of "edit mode" in plain Git).
In a way, the state is like we emptied the commit we want to edit, and the next commit (or the button press in GB) should amend to that commit, so the commit message I expect would always be the one of the commit I am editing, conflicted or not.
I don't think the commit should necessarily be emptied, but I don't feel too strongly about that.
To me there is no other choice than that, so I wonder if I am missing something crucial here despite the excellent explanation of what's happening.
Of course there are other choices, but your choice does seem like the best one so far. Thanks for the brainstorming. I'll implement it early next week and see what other people think (my biggest concern is backwards compatibility with existing edit mode metadata stored on disk, but I'll think through it more closely when I implement it).
There was a problem hiding this comment.
I don't think the commit should necessarily be emptied, but I don't feel too strongly about that.
Yes, let's scratch that. This was coming from StackedGit and is called "spill" there, which is something else.
When editing, I think it's fine to just make it easy to git amend, which does indeed seem like what edit-mode is in the non-conflicting case.
[..] (my biggest concern is backwards compatibility with existing edit mode metadata stored on disk, but I'll think through it more closely when I implement it).
Oh, I wasn't aware it has more than the gitbutler/edit reference, and I am definitely looking forward to learning more about that.
2 participants
When entering edit mode, a temporary commit needs to be created if either the commit being edited (C) or the parent of the commit being edited (P) is conflicted, so that (among other things)
git statusshows something reasonable. Currently, this temporary commit is created with no parents and a commit message duplicated from P.This is probably less user-friendly than it could be - the lack of parents means that the user cannot see C's place in the commit graph, and the duplicated commit message might make the user wonder if P was also changed. Therefore, when entering edit mode, if a temporary commit is needed, create it with parent P and a commit message that informs the user of what's going on.