Split the concerns of the commit creation functions#3075
Conversation
There was a problem hiding this comment.
In the octopus merge case, MERGE_HEAD will contain the id of each head being merged one per line. I suspect that reference_name_to_id will fail on this as the file will have what it believes to be trailing garbage.
There was a problem hiding this comment.
We should probably use some function that can return a list of ids given MERGE_HEAD instead.
|
I like this a lot, except I agree that the names aren't perfect. I'm gonna cc a bunch of people in case they have a clever idea. /cc @nulltoken |
|
Then again, re-reading #2681 I am growing increasingly happy with |
carlosmn
force-pushed
the
cmn/commit-on
branch
from
eb386d4 to
9d99de3
Compare
carlosmn
force-pushed
the
cmn/commit-on
branch
from
9d99de3 to
8a29c6e
Compare
|
To work around the naming issue, and other annoying stuff we have in this part of the code, I think we might want to accept a |
|
I've rebased on top of the current master and fixed the issue with having multiple entries in Maybe the ones that do update the ref belong in the |
|
We now have |
carlosmn
force-pushed
the
cmn/commit-on
branch
from
17c7d9e to
a859b61
Compare
|
You want to revisit this PR and rebase it before I'm doing a review? Will do so as soon as v0.27.0 is out |
This variant of the commit creation function takes the reference to update, the tree and the parents from the current branch, index and merging state, allowing for simpler use of a common use-case.
Currently git_commit_create() takes on creating a commit and updating a reference. Provide a better interface by splitting up each of the concerns into named functions for each. git_commit_create() will only create the commit, it will not modify any reference. git_commit_create_on() takes a reference name to update and git_commit_create_on_head() is a convenience function to update HEAD.
We now have `_fromstate` and `_fromstate_on_head`, the latter of which updates the current branch to point to the new commit.
|
Rebased. |
This should be a good place to eventually make use of `_fromstate` depending on what we're trying to show in this example.
pks-t
left a comment
pks-t
left a comment
There was a problem hiding this comment.
I didn't review the implementation just yet. I feel like it will probably be simple enough, and the bikeshedding on the API is probably more important right now
| const git_commit *parents[]); | ||
|
|
||
| /** | ||
| * Create a commit from the current state and update the current branch |
There was a problem hiding this comment.
"state" should probably be stated more clearly. I guess you mean the current index state, but somebody might mistake that for being the current worktree state
| * | ||
| * @see git_commit_create | ||
| */ | ||
| GIT_EXTERN(int) git_commit_create_on( |
There was a problem hiding this comment.
What about git_commit_create_on_ref? That would make clear what on refers to
| /** | ||
| * Create a commit from the current state and update the current branch | ||
| * | ||
| * Like `git_commit_create_fromstate`; additionally the current branch will be |
There was a problem hiding this comment.
current head, not branch? We'd still update HEAD in case it is detached
| * | ||
| * @see git_commit_create | ||
| */ | ||
| GIT_EXTERN(int) git_commit_create_fromstate( |
There was a problem hiding this comment.
fromstate is rather generic. It leaves one wondering which state one refers to. That being said, I don't have any suggestions which would be clearly better.
fromindexmight hint that one can pass in an indexfromreporeminds me of cherry-pickingsimpleis just stupid
One alternative might be git_commit_create_from_staged, which seems to me like the best option. It clearly states that we use the index to create the commit
| @@ -329,6 +321,26 @@ GIT_EXTERN(int) git_commit_extract_signature(git_buf *signature, git_buf *signed | |||
| * the given reference will be updated to point to it | |||
| */ | |||
| GIT_EXTERN(int) git_commit_create( | |||
There was a problem hiding this comment.
So, to reiterate the API change here. The only backwards-incompatible change we're doing in this PR is to rename git_commit_create to git_commit_create_on. As this is a rather central function that is probably in use by a lot of downstream users of libgit2, this is going to break most of them. While the fix is trivial, it is going to be a huge pain e.g. for distro maintainers, who'd have to patch a whole lot of reverse-dependencies of libgit2 to keep them working.
So I'd heavily favour to have a non-breaking API change here. Keep git_commit_create as is and instead create a new function that handles creation of a commit without updating a reference. The pain point is that I myself have no idea what to call that function. The following list simply sucks:
- git_commit_create_v2: version numbers are rather generic and don't tell much about the function's changed intention
- git_commit_create_only: the
onlyonly makes sense if you know the context of updating refs - git_commit_create_norefupdate: well, this at least says what is different from the other commit function
- git_commit_create_obj: dunno, might be just weird
I think we should either just keep git_commit_create untouched or mark it as deprecated. Breaking users without a proper deprecation period and without really good reasons feels irresponsible to me.
|
We've had a few changes since this was opened, to improve our commit APIs. I don't think that it's encompassed all the suggestions here, but it is improved. Closing this as stale. |
3 participants
As discussed on #2681 this PR splits up the commit creation functions into creating the commit and those which update a reference as well.
_fromstate()brings us closer togit-commitwith its defaults for the parents and tree.I don't love
git_commit_create_on_head()since you can get the same by passing"HEAD"togit_commit_create_on()and we already have enough confusion about what we mean byheadin function names.The names are whatever I came up with, better names may come up.