feat: Use git's --topo-order or --date-order instead of manual calculation

[kdheepak]

This seems to resolve #2 for me. With this change, I don't get any unstable layouts anymore (i.e. the order matches the output of git log) and I'm not able to trigger a segfault anymore.

Also resolves #3

[kdheepak]

Feel free to close this PR and use your patch if you'd like. I made more changes to remove all the warnings and remove the code that is not used.

[lusingander]

Great, thanks.

• As mentioned in Use git's --date-order or --topo-order  #3, we may need to think about how to handle stashes.
• The display seems to change quite a bit when we use --order=topo, so we may need to consider which is better.
  • I am testing graph drawing in tests/graph.rs.
• It would be good if we could avoid having to clone Commit.
  • In reality, this may not be a big problem.

[kdheepak]

I'm guessing some commit hashes are different because of timezone differences. I committed all the new images to this branch so we can discuss. I'll rebase and squash before merging if you'd like.

[kdheepak]

It would be good if we could avoid having to clone Commit.

I removed it in the most recent commit by storing the hashes instead.

The display seems to change quite a bit when we use --order=topo, so we may need to consider which is better.

Having looked through the test cases here, I don't see one or the other consistently better. And my personal opinion is that sticking to a graph that is similar to what git log --oneline --all produces is going to be good enough.

we may need to think about how to handle stashes.

I think the way you handled stashes is much nicer, but doing that while relying on the output of the git log --date-order is a little bit annoying I imagine. Personally, I don't use stashes that much and when I do, I'm not looking at the graph to see where the stashes were made, so I don't have a strong opinion here.

[lusingander]

Thank you for responding!

I'm guessing some commit hashes are different because of timezone differences. I committed all the new images to this branch so we can discuss. I'll rebase and squash before merging if you'd like.

Thank you for sharing. Looking at it again, it seems to be in pretty good, so this should be fine.

Regarding the dates in the image, you may be able to solve the problem by converting commiter_date to UTC in the following places.

https://github.com/lusingander/serie/blob/master/tests/graph.rs#L1069

The hash value appears to change when a merge commit occurs, not for a simple commit 🤔

I think the way you handled stashes is much nicer, but doing that while relying on the output of the git log --date-order is a little bit annoying I imagine. Personally, I don't use stashes that much and when I do, I'm not looking at the graph to see where the stashes were made, so I don't have a strong opinion here.

I see. I think it's better to display the stash as it was originally, so I think we should fix it.

[kdheepak]

Regarding the dates in the image, you may be able to solve the problem by converting commiter_date to UTC in the following places.

I fixed this in the most recent commit.

The hash value appears to change when a merge commit occurs, not for a simple commit 🤔

Yeah, I noticed that after. I initially assumed it was because of the dates but I'm still seeing it after changing it to UTC. I'm not sure what is going on there? Honestly I'm surprised that git hashes are reproducible at all, I assumed there was always some randomness added.

I see. I think it's better to display the stash as it was originally, so I think we should fix it.

I'm not sure how to do that though with the new implementation in this PR.

[lusingander]

I fixed this in the most recent commit.

Thanks, it certainly seems to have been fixed.

Yeah, I noticed that after. I initially assumed it was because of the dates but I'm still seeing it after changing it to UTC. I'm not sure what is going on there? Honestly I'm surprised that git hashes are reproducible at all, I assumed there was always some randomness added.

The changes you made only affect the display of the image, so they do not affect the commit date.

Since the following values ​​are set when creating a commit, all elements required to calculate SHA1 are fixed.

serie/tests/graph.rs

Lines 925 to 930 in 779bd6e

	.env("GIT_AUTHOR_NAME", "Author Name")
	.env("GIT_AUTHOR_EMAIL", "author@example.com")
	.env("GIT_AUTHOR_DATE", &datetime_str)
	.env("GIT_COMMITTER_NAME", "Committer Name")
	.env("GIT_COMMITTER_EMAIL", "committer@example.com")
	.env("GIT_COMMITTER_DATE", &datetime_str)

However, this is the same for merge, so I'm not sure what is affecting it 🤔

I'm not sure how to do that though with the new implementation in this PR.

I'll think about it. However I think the most straightforward approach would be to look at the commit list from the top and insert them as appropriate.

[kdheepak]

The changes you made only affect the display of the image, so they do not affect the commit date.

I added a new commit now that changes the parse_date function to not use local time. The tests pass the same though, so it is no longer causing a timezone issue, at least as of right now.

[lusingander]

Could you please share the branch_001 repository that is output under out/graph?

This is what I ran in my local environment:
branch_001.zip

[kdheepak]

Here's mine: branch_001.zip

[lusingander]

Thanks!
I found the cause. The immediate cause is that your merge commit contains an extra string as the commit message.

We may need to specify the message explicitly so that it is not dependent on the personal environment.

[kdheepak]

Nice find! I added a --no-log and now get the same commit message and commit hash as you do:

[kdheepak]

I added the original images back to the PR in a new commit, so if you squash merge on GitHub it'll not bloat your git history.

[lusingander]

Thank you for dealing with various issues!

The approach seems good, so it might be good to merge it for now and deal with the squash issue as a separate PR.

The hash issue is probably resolved, so it seems fine to commit images with updates, including squash (I want to fix it so that the test passes).

With that in mind, I've added a few minor comments.

[kdheepak]

I made all your requested changes!

[lusingander]

I really appreciate your assistance with looking into the issues. Thank you so much!