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

commit-graph: Add a way to write commit-graph files #5778

Merged
merged 6 commits into from
Aug 30, 2021

Conversation

lhchavez
Copy link
Contributor

This change adds the git_commit_graph_writer_* functions to allow to
write and create commit-graph files from .idx/.pack files or
git_revwalks.

Part of: #5757

@lhchavez lhchavez mentioned this pull request Jan 10, 2021
8 tasks
@lhchavez lhchavez force-pushed the cgraph-write branch 2 times, most recently from a1bf7a4 to 93ab8d8 Compare January 10, 2021 21:56
@lhchavez lhchavez marked this pull request as ready for review January 10, 2021 21:57
@lhchavez lhchavez force-pushed the cgraph-write branch 2 times, most recently from 8a7fe96 to 012cfe4 Compare July 27, 2021 13:24
This change adds the git_commit_graph_writer_* functions to allow to
write and create `commit-graph` files from `.idx`/`.pack` files or
`git_revwalk`s.

Part of: libgit2#5757
Copy link
Member

@ethomson ethomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your work on commit graphs, this is really exciting stuff. A few minor nits and a few questions. One additional question - are you using some tool to format your code? Your formatting seems consistent with itself but not particularly consistent with the rest of the project. Curious if this is your style, a tool, etc...

src/commit_graph.c Outdated Show resolved Hide resolved
src/commit_graph.c Outdated Show resolved Hide resolved
src/commit_graph.c Outdated Show resolved Hide resolved
src/commit_graph.c Outdated Show resolved Hide resolved
src/commit_graph.c Outdated Show resolved Hide resolved
src/commit_graph.c Outdated Show resolved Hide resolved
src/commit_graph.c Outdated Show resolved Hide resolved
include/git2/sys/commit_graph.h Show resolved Hide resolved
@lhchavez
Copy link
Contributor Author

Thanks again for your work on commit graphs, this is really exciting stuff. A few minor nits and a few questions. One additional question - are you using some tool to format your code? Your formatting seems consistent with itself but not particularly consistent with the rest of the project. Curious if this is your style, a tool, etc...

yes! i am using clang-format with settings that tried to be as consistent as possible with the rest of the codebase (but apparently didn't meet the goal u_u).

* Added the `PenaltyBreakAssignment: 1000` clang-format option to avoid
  breaking statements around the assignment operator.
* Avoided using the dot initializer syntax.
* Avoided casting allocations.
* Also avoided casting `void *`.
@lhchavez
Copy link
Contributor Author

lhchavez commented Aug 1, 2021

Thanks again for your work on commit graphs, this is really exciting stuff. A few minor nits and a few questions. One additional question - are you using some tool to format your code? Your formatting seems consistent with itself but not particularly consistent with the rest of the project. Curious if this is your style, a tool, etc...

yes! i am using clang-format with settings that tried to be as consistent as possible with the rest of the codebase (but apparently didn't meet the goal u_u).

these are the current settings i have:

# .clang-format
---
AlignAfterOpenBracket: AlwaysBreak
AlignConsecutiveAssignments: false
AlignTrailingComments: true
AlignOperands: false
AllowAllParametersOfDeclarationOnNextLine: false
AllowShortBlocksOnASingleLine: false
AllowShortFunctionsOnASingleLine: false
AllowShortIfStatementsOnASingleLine: false
BasedOnStyle: WebKit
BinPackArguments: false
BinPackParameters: false
BreakBeforeBraces: Linux
ColumnLimit: 100
ContinuationIndentWidth: 16
Cpp11BracedListStyle: true
ForEachMacros:
- git_vector_foreach
- git_vector_rforeach
- git_array_foreach
IncludeCategories:
- Priority: 1
  Regex: ^<
- Priority: 2
  Regex: ^[<"]git2/
- Priority: 3
  Regex: ^"
IndentCaseLabels: false
IndentWidth: 8
MaxEmptyLinesToKeep: 2
PointerAlignment: Right
SortIncludes: false
SpacesBeforeTrailingComments: 2
TypenameMacros:
- GIT_EXTERN
UseTab: Always
PenaltyBreakAssignment: 1000
...

@ethomson
Copy link
Member

Sorry it's taken so long to get back to this, #5974 took a lot more time than I expected. 😢

One question about the struct initializer. Otherwise, I'm going to take a whack at tweaking clang-format to see if we can get it a little closer to our style.

Then I'm looking forward to merging this and cutting 1.2.0 with all your improvements. Thanks for all the hard work - and your patience in getting them reviewed.

@ethomson
Copy link
Member

Sorry about pulling the rug out from under you with the array changes. Thanks for the iteration. 👍

1 similar comment
@ethomson
Copy link
Member

Sorry about pulling the rug out from under you with the array changes. Thanks for the iteration. 👍

@ethomson
Copy link
Member

I did a few formatting fixes because I had it beaten into my head that we should have binary operators at the end of a line not the beginning. I apologize for this compulsion, but there it is.

Manually merged. Thanks for all this work!

@ethomson ethomson merged commit 7d9ebdc into libgit2:main Aug 30, 2021
@lhchavez lhchavez deleted the cgraph-write branch August 30, 2021 22:23
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