Split the concerns of the commit creation functions #3075

carlosmn · 2015-04-26T00:52:58Z

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 to git-commit with 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" to git_commit_create_on() and we already have enough confusion about what we mean by head in function names.

The names are whatever I came up with, better names may come up.

ethomson · 2015-05-01T14:31:02Z

src/commit.c

+
+	/* TODO: how do we handle octopus merges? */
+	if (state == GIT_REPOSITORY_STATE_MERGE) {
+		if ((error = git_reference_name_to_id(&current_id, repo, GIT_MERGE_HEAD_FILE)) < 0)


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.

We should probably use some function that can return a list of ids given MERGE_HEAD instead.

ethomson · 2015-05-01T14:32:45Z

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
/cc @vmg
/cc @jamill

ethomson · 2015-05-01T14:34:02Z

Then again, re-reading #2681 I am growing increasingly happy with _create_on...

carlosmn · 2016-03-04T13:31:05Z

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 git_commit_options wherein we store the data. Adding small variations otherwise leads to combinatorial explosion or only some variants gaining options.

carlosmn · 2017-05-20T12:02:31Z

I've rebased on top of the current master and fixed the issue with having multiple entries in MERGE_HEAD. I'm still not sure what we should name all of these things or whether we even want _fromstate() to update the reference.

Maybe the ones that do update the ref belong in the git_repository namespace? Maybe we only want to have the convenience function that does create the commit act on HEAD since it's the only one which has semantics in the git tool?

carlosmn · 2017-05-27T22:18:06Z

We now have _fromstate which does not touch references and _fromstate_on_head which does. I figure this is enough to solve the most common use-cases, and anything more complex you just have to go ahead and do the few steps contained therein yourself.

pks-t · 2018-03-22T13:29:19Z

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.

carlosmn · 2018-03-22T15:28:22Z

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

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

pks-t · 2018-03-26T12:12:22Z

include/git2/commit.h

@@ -340,6 +352,22 @@ GIT_EXTERN(int) git_commit_create(
 	size_t parent_count,
 	const git_commit *parents[]);

+/**
+ * Create a commit from the current state and update the current branch


"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

pks-t · 2018-03-26T12:13:08Z

include/git2/commit.h

+ *
+ * @see git_commit_create
+ */
+GIT_EXTERN(int) git_commit_create_on(


What about git_commit_create_on_ref? That would make clear what on refers to

pks-t · 2018-03-26T12:15:02Z

include/git2/commit.h

+/**
+ * Create a commit from the current state and update the current branch
+ *
+ * Like `git_commit_create_fromstate`; additionally the current branch will be


current head, not branch? We'd still update HEAD in case it is detached

pks-t · 2018-03-26T12:20:31Z

include/git2/commit.h

+ *
+ * @see git_commit_create
+ */
+GIT_EXTERN(int) git_commit_create_fromstate(


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.

fromindex might hint that one can pass in an index

fromrepo reminds me of cherry-picking

simple is 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

pks-t · 2018-06-15T08:56:33Z

include/git2/commit.h

@@ -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(


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 only only 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.

ethomson · 2024-03-09T21:55:51Z

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.

ethomson reviewed May 1, 2015
View reviewed changes

carlosmn force-pushed the cmn/commit-on branch from eb386d4 to 9d99de3 Compare November 1, 2015 22:11

carlosmn force-pushed the cmn/commit-on branch from 9d99de3 to 8a29c6e Compare November 11, 2015 00:28

carlosmn force-pushed the cmn/commit-on branch from 8a29c6e to 8087273 Compare May 20, 2017 11:24

carlosmn force-pushed the cmn/commit-on branch from 17c7d9e to a859b61 Compare October 8, 2017 20:24

carlosmn added 4 commits March 22, 2018 16:09

commit: introduce _fromstate()

8f212f6

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.

Update CHANGELOG with the commit function changes

b31cd43

commit: rearrange _fromstate and _on_head variants

e8eba1c

We now have `_fromstate` and `_fromstate_on_head`, the latter of which updates the current branch to point to the new commit.

carlosmn force-pushed the cmn/commit-on branch from a859b61 to e8eba1c Compare March 22, 2018 15:27

examples: adjust merge to git_commit_create_on

37a0165

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 reviewed Mar 26, 2018

View reviewed changes

pks-t reviewed Jun 15, 2018

View reviewed changes

tiennou mentioned this pull request Jan 14, 2019

Add sign capability to git_rebase_commit #4913

Merged

tiennou mentioned this pull request Jun 15, 2019

Commit creation API streamlining #5116

Closed

Base automatically changed from master to main January 7, 2021 10:09

ethomson mentioned this pull request May 12, 2021

Notes from contributors conference call: 2018-06-11 #5869

Closed

ethomson dismissed a stale review via 37a0165 July 26, 2021 20:28

ethomson closed this Mar 9, 2024

Split the concerns of the commit creation functions #3075

Split the concerns of the commit creation functions #3075

Uh oh!

Conversation

carlosmn commented Apr 26, 2015

Uh oh!

ethomson May 1, 2015

Choose a reason for hiding this comment

Uh oh!

carlosmn May 4, 2015

Choose a reason for hiding this comment

Uh oh!

ethomson commented May 1, 2015

Uh oh!

ethomson commented May 1, 2015

Uh oh!

carlosmn commented Mar 4, 2016

Uh oh!

carlosmn commented May 20, 2017

Uh oh!

carlosmn commented May 27, 2017

Uh oh!

pks-t commented Mar 22, 2018

Uh oh!

carlosmn commented Mar 22, 2018

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

pks-t Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

pks-t Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

pks-t Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

pks-t Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

pks-t Jun 15, 2018

Choose a reason for hiding this comment

Uh oh!

ethomson commented Mar 9, 2024

Uh oh!

Uh oh!