fix(git): sort commits in topological order

[bobrik]

closes #188
closes #38

Description

Commit a1b4b5b ("fix(git): sort the commits in topological order"), changed the order from TIME to TIME | TOPOLOGICAL. According to the docs, this is equivalent to git log --date-sort:

• https://github.com/libgit2/libgit2/blob/v1.7.1/include/git2/revwalk.h#L33-L38

This breaks down in the following scenario:

1. A PR is open.
2. A release v1 is made.
3. A PR is merged.
4. A relase v2 is made.

The git history for this would be:

- v2 release
- pr merge commit
- v1 release
- actual pr commit

This directly spills into the changelog produced by git-cliff, misattributing commits that were merged in v2 to v1 retroactively when v2 is made.

You can see this with git log. If you pass --topo-order there:

- v2 relase
- pr merge commit
- actual pr commit
- v1 relase

With this change we only pass Sort::TOPOLOGICAL in git-cliff, which produces the very same results as git log --topo-order.

How Has This Been Tested?

Tested on a repo with the scenario described above.

Screenshots / Logs (if applicable)

Here's the git log output that is causing commit ZTC-886 to be misattributed to v2.0.6 when it was only merged after the release happened.

commit d3fcb284e764a90d3bfecbaac60c51ba93d3e44e (HEAD, tag: v2.0.7)
Date:   Mon Oct 9 15:27:35 2023 +0100

    Release 2.0.7

commit 508ac156deeef509938128b5bce62ab5b956f565
Merge: a778700 adf03ec
Date:   Tue Oct 3 15:43:16 2023 +0000

    Pull request #89: ZTC-1201

    Merge to main

    * commit 'adf03ec4246bf94e8fd534986f314dda6a9a996c':
      ZTC-1201

commit adf03ec4246bf94e8fd534986f314dda6a9a996c
Date:   Mon Oct 2 14:06:47 2023 +0100

    ZTC-1201

commit a778700021276d2c6f59756687011629b88f7e36
Merge: a1ef654 989e7cf
Date:   Mon Oct 2 12:22:38 2023 +0000

    Pull request #86: ZTC-886

    Merge to main

    * commit '989e7cfe4ec199f56a5b05f6b5d66d6acd6c761b':
      ZTC-886

commit a1ef654fd337d12464394152b5d86467680e28ae (tag: v2.0.6, origin/release/2.0.6, release/2.0.6)
Date:   Fri Sep 29 17:51:49 2023 +0100

    Release 2.0.6

commit f2046939b0b0e178f9f2c0c170a14368d53e2e15 (origin/jpaiva/OXY-1224)
Date:   Fri Sep 29 16:58:29 2023 +0100

    OXY-1224

commit 989e7cfe4ec199f56a5b05f6b5d66d6acd6c761b
Date:   Fri Sep 29 15:54:54 2023 +0100

    ZTC-886

Types of Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (no code change)
• Refactor (refactoring production code)
• Other

Checklist:

• My code follows the code style of this project.
• I have updated the documentation accordingly.
• I have formatted the code with rustfmt.
• I checked the lints with clippy.
• I have added tests to cover my changes.
• All new and existing tests passed.

[welcome]

Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.42%. Comparing base (128f503) to head (fbd6834).
Report is 387 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #415   +/-   ##
=======================================
  Coverage   42.42%   42.42%           
=======================================
  Files          15       15           
  Lines         995      995           
=======================================
  Hits          422      422           
  Misses        573      573           

Flag	Coverage Δ
unit-tests	42.42% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[orhun]

Hey, thanks for the fix! 🐻

To understand this better, can you share the git-cliff output before/after this change based on the mentioned Git history?

Also, I'm thinking whether if #188 and #38 are related to this issue or not. Can you check and confirm if they are related?

[bobrik]

Before:

2.0.7
- 2023-10-09 Release 2.0.7
- 2023-10-02 ZTC-1201

2.0.6
- 2023-09-29 Release 2.0.6
- 2023-09-29 OXY-1224
- 2023-09-29 ZTC-886
...

After:

2.0.7
- 2023-10-09 Release 2.0.7
- 2023-10-02 ZTC-1201
- 2023-09-29 ZTC-886

2.0.6
- 2023-09-29 Release 2.0.6
- 2023-09-29 OXY-1224
...

Issues #188 and #38 indeed look like they would be fixed with this change.

[orhun]

After:

Oh yeah, that indeed looks correct.

Issues 188 and 38 indeed look like they would be fixed with this change.

That's awesome. They were open for a long time because I couldn't figure out what's going on. Have you reproduced the cases mentioned there? i.e. Are we confident to close them after this PR is merged?

One last thing, I'm assuming there won't be cases where this breaks any uses cases, right? It should be changed anyways since it is the wrong behavior.

Lastly, thanks for coming out of nowhere and fixing this! 🐻

[bobrik]

That's awesome. They were open for a long time because I couldn't figure out what's going on. Have you reproduced the cases mentioned there? i.e. Are we confident to close them after this PR is merged?

I have not reproduced them, but they do look functionally identical. I'd be surprised if this change doesn't fix them.

One last thing, I'm assuming there won't be cases where this breaks any uses cases, right? It should be changed anyways since it is the wrong behavior.

I don't think anything would break here.

[orhun]

Thanks for your contribution!

[welcome]

Congrats on merging your first pull request! ⛰️