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

Use commit message instead of the SHA in fixup #5

Merged
merged 1 commit into from
Dec 12, 2018

Conversation

nightscape
Copy link
Contributor

Hi @tummychow,

first of all: amazing project!!
Would it be possible to use the commit message instead of the SHA in the fixup?
Many tools do this (e.g. Magit in Emacs) and imo it makes sense because it will be both more readable and also continue to work if another rebase creates new SHAs.
I just hacked this together in the Github editor because, so it might not even compile, but I hope the intent is clear 😃

@nightscape
Copy link
Contributor Author

Ok, just compiled and tested it locally.
Seems to work 🎉

@tummychow
Copy link
Owner

the problem with this approach is that commit subjects are not a unique identifier. if two commits in the stack have the same commit subject, git rebase --autosquash will always move the fixup to the oldest commit with that subject, because it can't tell which one you actually meant. concrete example:

mkdir foo
cd foo
git init
# zero line file
touch foo
git add foo
git commit -m foo

# one line file
cat > foo <<EOF
foo
EOF
git commit -am foo

# two line file
cat > foo <<EOF
foo
bar
EOF
git commit -am foo

# modify the first line
cat > foo <<EOF
foo bar
bar
EOF
# produces the commit message:
# fixup! foo
git commit -a --fixup @^

# this will produce a merge conflict
# git tries to fixup into @^^ when it should actually fixup into @^
# but all the commits have the same subject so it can't tell the difference
GIT_SEQUENCE_EDITOR=true git rebase -i --autosquash @^^^

in this situation, if you ran git absorb instead of git commit --fixup, there would be no merge conflict, because git absorb would use an unambiguous commit message, and the rebase would know which commit to fixup into.

the optimal behavior here is to check all the commits in the stack for duplicated subjects. if there are no duplicated subjects, then it's safe to use commit subjects in the fixup message, otherwise we have to use commit shas. an acceptable alternative would be to put this behavior behind a command line flag. if you add either or both of those things then i'll merge this, otherwise i'll merge it eventually when i feel like adding those parts myself

@nightscape
Copy link
Contributor Author

Something like the above fixup?
If you want to be 100% accurate, it probably is still not sufficient:
If as you mentioned Git takes the oldest commit whose message the fixup matches, then this commit could be outside of the searched stack if you rebase --autosquash a range longer then the maxStack size.

@tummychow
Copy link
Owner

yep, that fixup works for me. did you want to autosquash before i merged it?

and yeah, there's not much we can do if the absorb and rebase are run against different base commits. one of the TODOs is to run rebase --autosquash automatically if the user desires, so that they can't accidentally run the rebase over the wrong range of commits

@nightscape
Copy link
Contributor Author

I squashed the commits and adapted the README slightly.
Good to merge from my side 😃

@tummychow tummychow merged commit e37669e into tummychow:master Dec 12, 2018
tummychow added a commit that referenced this pull request Dec 12, 2018
thanks to @nightscape for the original patch in #5

- use the commit summary rather than the entire commit message
- fix bug where the absorb commit gets created before iterating through
  the entire stack (the map of message counts would be incomplete and
  there might be a duplicate further down in the stack that we haven't
  seen yet)
@tummychow
Copy link
Owner

tweaked so that the message count hashmap gets generated once for the entire stack (otherwise, there's a bug where the absorb commit happens before you've counted all the commits in the stack, and it might miss a duplicate). thanks again for patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants