git: worktree_commit, Modify checking empty commit. Fixes #723

[onee-only]

Changed condition of checking empty commit from count of entries == 0 to createdHash == parentHash.

There was an edge case where the given commit is initial commit and worktree has no entries. So I made another control branch to check it.

I'm not aware of other edge cases it might have. If there is one, please let me know.

Connected to #723

[onee-only]

By the way, could you provide the answer here? -> #1045 (comment)
I would greatly appreciate it if you could satisfy my curiosity.

[onee-only]

I have some considerations.

1. Changing the commit msg affects its hash. So, empty commit with only changed msg will be commited without allow-empty flag.
2. Our signing logic creates signature with random source. So, I'm guessing empty commit with signkey will be commited without allow-empty flag.

I think comparing treeHash of parent and current tree is more appropriate than this approach.

What are your thoughts on it?

[AriehSchneier]

Can you please add a test that tests that an error is returned if you try to add an empty commit to a non-empty repo.

[AriehSchneier]

I think comparing treeHash of parent and current tree is more appropriate than this approach.

What are your thoughts on it?

I think that is what @pjbgf said here: #1050 (comment)
As well as here: #723 (comment)

And I think the current code will not correctly error for an empty commit on a non-empty repo (hence why I just requested that test)

[onee-only]

Oh! I should have checked the issue carefully. I made a mistake.
Sorry for taking your time.. 😬

[onee-only]

I changed it as requested. I expect this is the right approach.

There were test failures as I changed the logic. But I think those were not being tested right. So I changed the tests.

[AriehSchneier]

What should happen, and is there a test for, a merge commit which leaves the tree in the same state as it was (ie merging 2 branches with the same changes)? (Should that be considered an empty commit?)

[onee-only]

What should happen, and is there a test for, a merge commit which leaves the tree in the same state as it was (ie merging 2 branches with the same changes)? (Should that be considered an empty commit?)

I tested the behavior with git cli.

$ git log --all --decorate --oneline --graph

* 1778bba (main) anything
| * 1044aab (HEAD -> hi) ab
| * aaf0fc5 a
| * 2c9a900 second
| * 34b3b53 first
|/  
* a60d58b Init
* 

$ git diff hi main
-- no output --

If you merge main into hi, git will merge those two without error.

$ git merge main
Merge made by the 'ort' strategy.

Even if the worktree remains the same, seems like we'll need to store metadata of the commit (parent commits, etc).
So I think we'll have to create a commit if we want to follow the behavior of git cli.

Since go-git only implements fast-foward yet, I think we need to discuss it more when we support more methods.

[onee-only]

@pjbgf Could you re-run the checks? I'm not sure if this failure is related to this PR.

FAIL: <autogenerated>:1: ReceivePackSuite.TestDefaultBranch

/home/runner/work/go-git/go-git/plumbing/transport/test/receive_pack.go:80:
    c.Assert(err, IsNil)
... value *url.Error = &url.Error{Op:"Get", URL:"http://localhost:43743/basic.git/info/refs?service=git-receive-pack", Err:(*net.OpError)(0xc0002[56](https://github.com/go-git/go-git/actions/runs/8654261118/job/23731124667?pr=1050#step:6:57)230)} ("Get \"http://localhost:43743/basic.git/info/refs?service=git-receive-pack\": read tcp [::1]:35226->[::1]:43743: read: connection reset by peer")

[pjbgf]

What should happen, and is there a test for, a merge commit which leaves the tree in the same state as it was (ie merging 2 branches with the same changes)? (Should that be considered an empty commit?)

@AriehSchneier thanks for helping reviewing this PR. Do you think we need an additional test with a FF merge with a branch that has an empty commit on it? Or are you happy for this to be considered once we implement other merge types?

[AriehSchneier]

What should happen, and is there a test for, a merge commit which leaves the tree in the same state as it was (ie merging 2 branches with the same changes)? (Should that be considered an empty commit?)

@AriehSchneier thanks for helping reviewing this PR. Do you think we need an additional test with a FF merge with a branch that has an empty commit on it? Or are you happy for this to be considered once we implement other merge types?

Sorry for the slow reply I was on leave and hadn't checked my messages.

I'm happy with whatever you think is best (it's been a little while since I looked at the actual change), as long as when the merge types are added its clear that we need to (or don't need to) touch this code to keep it compliant. That said, if there is a possibility off adding more test cases now, I'm not sure why we wouldn't?