Handle fast-forward merges properly in repository.performRebase#1457
Handle fast-forward merges properly in repository.performRebase#1457implausible merged 3 commits intonodegit:masterfrom
Conversation
|
Can we get a test added for this? Will this also fix #1318? |
|
I'll try and add a test. Yes, it will fix that issue. |
e3cece1 to
1c334c6
Compare
lib/repository.js
Outdated
| */ | ||
| function readRebaseMetadataFile(fileName, determineExistance) { | ||
| const fullPath = path.join(repository.path(), "rebase-merge", fileName); | ||
| if (determineExistance && !fse.existsSync(fullPath)) { |
There was a problem hiding this comment.
Let's use the async version of exists.
There was a problem hiding this comment.
Because of how promisify-node turns callbacks into promises, fse.exists has a bug where you need to call .catch to actually get the value you would expect from .then so it looks really horrible and is a bit confusing if you ask me. Either that or I can use the callback version, which I don't want to do either. That being said, I don't think the asynchronous version will really gain us anything in this instance so I would rather avoid adding
fse.exists(fillPath).catch(result => /* do work here */).then()
to this code block. What do you think?
There was a problem hiding this comment.
What bug is this in promisify-node and why can't we fix that there and then improve this code?
There was a problem hiding this comment.
It may not be promisify-node as the issue, depending on how you look at it. exists has one callback argument. promisify-node assumes that first argument is an error and the second argument is the value returned. exists doesn't adhere to nodejs callback best practices.
There was a problem hiding this comment.
That being said we can bump fse as later versions seem to have fixed this issue, from what I understand, or we can use a different function all together, like stat or access or something something.
The node documentation recommends we just run a read on the file and catch the error, which can also work.
Rebases currently expect the
rewrittenfile to exist when a rebase occurs and abeforeFinishFnis specified. This is not the case when the rebase results in a fast-forward merge as nothing is rewritten.