Add git_status_file_at #4318

Uncommon · 2017-07-24T15:06:59Z

This change takes git_status_list_new and splits out a new function called git_status_list_new_at (I'm totally open to alternative names) which takes a tree parameter to compare against instead of the HEAD tree.

The purpose of this is to get a preview of an amend commit, where the supplied tree will be from the head commit's first parent.

ethomson · 2017-07-24T15:15:49Z

Thanks for the PR. I think that I need to think a bit more about this, but let me give you my hot take:

I think that this should be an option in git_status_options instead of a new API. Sorry - I know that you asked about this in slack, and I didn't give you an answer. I wasn't sure at the time and didn't have much time to consider it (I was at a conference last week), but having seen it now, I do think that it would be preferable.

I say that especially since status is well-defined as a HEAD<->index<->workdir diff. It seems very weird to call something status that isn't actually looking at the HEAD. It seems a little nicer to make this an option instead of a new API. 🤔

Uncommon · 2017-07-24T15:28:39Z

If I make a change to git_status_options, do I need to bump the version number?

Uncommon · 2017-07-24T15:31:47Z

Looks like AppVeyor failed because of credentials tests.

Uncommon · 2017-07-27T23:00:40Z

I'd also like to do a similar thing with git_status_file which doesn't have a corresponding options parameter, so in that case I'd have to do a separate function (or add a parameter). I think it would be best to use the same approach for bothgit_status_file and git_status_list_new.

Uncommon · 2017-07-28T14:56:45Z

Actually on further inspection I see that git_status_file uses git_status_options internally. So maybe the thing to do is to split git_status_file and extend git_status_options, leaving the other interfaces as they are.

Uncommon · 2017-08-09T23:02:12Z

As noted above, I'm adding git_status_file_at and extending git_status_options with a head_tree field, and therefore taking git_status_list_new_at back out. I'll push it once I've done some more testing.

The question remains: if I'm changing git_status_options, does GIT_STATUS_OPTIONS_VERSION need to be bumped?

pks-t · 2017-09-01T07:19:42Z

Hi @Uncommon, sorry for the long delay. Right now in the pre-1.0 world, we have no guaranted stable API and so all releases come with a SOVERSION-bump. So all users of our library will have link against the newer version anyway, making the version useless right now. We will start bumping option versions only after the first stable release.

Uncommon · 2017-09-01T15:12:49Z

Ok, thanks!

Uncommon · 2017-09-05T18:39:21Z

I've pushed my new changes as described above: added the options field and git_status_file_at instead of git_status_list_new_at.

Uncommon · 2017-09-05T22:21:04Z

Looks like I forgot to update the unit tests.

Uncommon · 2017-09-06T16:29:06Z

Looks like AppVeyor failed with a Windows ssl problem.

Uncommon · 2017-09-07T17:19:16Z

One of the Travis builds seems to have stalled out on brew update. How do you re-run it?

ethomson · 2017-09-08T15:34:57Z

I'll requeue these builds.

Uncommon · 2017-09-10T03:55:44Z

Thanks. AppVeyor errors look unrelated again.

ethomson · 2017-09-10T12:29:01Z

Sigh. No idea what's going on with the AppVeyor build.

Uncommon · 2017-09-19T21:02:08Z

Did #4347 fix the build issues seen here?

pks-t · 2017-09-20T12:33:52Z

Yup, exactly. Can you please rebase your changes onto my fix?

Uncommon · 2017-09-20T15:12:08Z

I'm unable to build now. Apparently it's looking for the macOS 10.12 SDK, which I no longer have since upgrading to Xcode 9.

CMake Warning at /usr/local/Cellar/cmake/3.5.2/share/cmake/Modules/Platform/Darwin-Initialize.cmake:132 (message):
Ignoring CMAKE_OSX_SYSROOT value:

/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk

because the directory does not exist.
Call Stack (most recent call first):
/usr/local/Cellar/cmake/3.5.2/share/cmake/Modules/CMakeSystemSpecificInitialize.cmake:18 (include)
CMakeLists.txt:14 (PROJECT)

CMake Error at /usr/local/Cellar/cmake/3.5.2/share/cmake/Modules/Platform/Darwin.cmake:76 (message):
CMAKE_OSX_DEPLOYMENT_TARGET is '10.8' but CMAKE_OSX_SYSROOT:

""

is not set to a MacOSX SDK with a recognized version. Either set
CMAKE_OSX_SYSROOT to a valid SDK or set CMAKE_OSX_DEPLOYMENT_TARGET to
empty.
Call Stack (most recent call first):
/usr/local/Cellar/cmake/3.5.2/share/cmake/Modules/CMakeSystemSpecificInformation.cmake:36 (include)
CMakeLists.txt:14 (PROJECT)

-- Configuring incomplete, errors occurred!
See also "/Users/i58922/Developer/Personal/Xit/objective-git/External/libgit2/build/CMakeFiles/CMakeOutput.log".
See also "/Users/i58922/Developer/Personal/Xit/objective-git/External/libgit2/build/CMakeFiles/CMakeError.log".
make: *** [cmake_check_build_system] Error 1

ethomson · 2017-09-20T15:28:27Z

@Uncommon did you re-run cmake? It caches locations to things that are probably now missing. If you have not, delete your build directory, create a new one, and run cmake again.

Uncommon · 2017-09-20T15:37:02Z

I also had to delete tests/clar.suite. Thanks to @pks-t for that pointer.

Uncommon · 2017-09-26T17:16:59Z

Rebased, and tests are passing for me locally.

Uncommon · 2017-10-04T15:54:53Z

Poke again. Builds are green. I have a feature waiting on this, so I'd like to get it in.

pks-t

Sorry that I chime in that late with some change requests.

It mostly looks good, except that I'm not that happy with the git_status_file_at API. I'd definitly prefer to have that be a generic git_status_file_ext one for improved maintainability later on.

pks-t · 2017-10-06T07:34:34Z

include/git2/status.h

+	unsigned int *status_flags,
+	git_repository *repo,
+	const char *path,
+	git_tree *head_tree);


So one problem with this new API is, as @ethomson already mentioned, that we still have to add new calls whenever we want some new option in here. Ideally, we'd have designed git_status_file to just take a git_status_options structure here, so that we can add more options later on without adding new functions for all of them.

As we do not want to break the API here, we obviously cannot change git_status_file right now. What we can do though is to add a git_status_file_ext(unsigned int *, git_repository *, const char *, const git_status_options *), which fixes that shortcoming. See for example git_status_foreach and git_status_foreach_ext, where we fixed the same shortcoming.

So I'd prefer to introduce that one instead of having to potentially maintain more git_status_file_foobar functions in the future.

Actually, I was suggesting not adding a new git_status_file API at all. I was hoping that the change in the options struct would be enough.

git_status_file is a helper method. It should be for the 99% use cases. If you want something special (like a different HEAD tree, which is super uncommon) then I don't mind saying that you have to do a little more work to get there.

In other words, I am not in favor of adding new git_status_file-like functions, and would encourage you to just call git_status_foreach_ext (with the new opts structure) for this very special use case.

So you think I should take the logic from git_status_file_at and move it up into my app? Either way is fine with me.

Well, if you have git_status_foreach_ext handle the new tree option then most of it is already done for you, I guess. But otherwise yes

pks-t · 2017-10-06T07:34:58Z

src/status.c

 	}
-
+	else {


We usually have closing braces and else branches on the same line.

Oops, I missed this in my first round of fixes. Looking at other files, I don't know if "usually" is accurate :) but if it's preferred I can certainly make the change.

I went ahead and did this one too.

Well, sure. We're being inconsistent with many things, but at least I try to avoid introducing more inconsistencies ;) And in this case we're definitly being consistent most of the time with closing brace and else on the same line.

This still seems to not be fixed here -- did you forget to push your amends?

pks-t · 2017-10-06T07:37:29Z

tests/status/worktree.c

+  git_repository *repo = cl_git_sandbox_init("empty_standard_repo");
+  git_status_options opts = GIT_STATUS_OPTIONS_INIT;
+  git_status_list *statuslist;
+  git_tree *parentTree;


Typically, we do not use camelcase variable names but instead use underscores.

This still applies

pks-t · 2017-10-06T07:38:21Z

tests/status/worktree.c

+  cl_git_mkfile("empty_standard_repo/file1", "ping");
+  stage_and_commit(repo, "file1");
+
+  git_repository_head_tree(&parentTree, repo);


Please wrap this in an cl_git_pass.

This also still applies :)

pks-t · 2017-10-06T07:38:37Z

tests/status/worktree.c

+
+  opts.show = GIT_STATUS_SHOW_INDEX_AND_WORKDIR;
+  opts.head_tree = parentTree;
+  git_status_list_new(&statuslist, repo, &opts);


Same, please use cl_git_pass

pks-t · 2017-10-06T07:39:22Z

tests/status/worktree.c

+  git_status_list_new(&statuslist, repo, &opts);
+
+  cl_assert_equal_sz(1, git_status_list_entrycount(statuslist));
+  status = git_status_byindex(statuslist, 0);


Please wrap this in an cl_assert, so that we do not segfault on a NULL pointer here.

Uncommon · 2017-10-20T22:57:05Z

Updated with a little bug fix. Coming soon, I'll remove git_status_file_at, but I'll still need head_tree in the options.

Uncommon · 2017-10-23T17:15:55Z

@pks-t Requested changes to the unit test code have been addressed. I've also removed git_status_file_at, leaving only the head field in the options.

Uncommon · 2017-10-31T17:53:19Z

Any further thoughts on this?

pks-t · 2017-11-03T20:20:07Z

Sorry, I'm currently busy with a move. I'll find some time next week to have a further look

Uncommon · 2017-11-17T15:12:07Z

It's been a couple of weeks, so I'm bumping this again.

pks-t

I'm really sorry for taking so long to review this. :/ There's a lot of outdated new comments, as I prefer to go through PRs commit by commit. It would be nice if you amended your existing commits instead of just building up your fixes, such that the history becomes much easier to review.

pks-t · 2017-11-21T13:58:56Z

include/git2/status.h

+GIT_EXTERN(int) git_status_list_new_at(
+  git_status_list **out,
+  git_repository *repo,
+  const git_status_options *opts,


THe options field is usually the last one in our functions.

pks-t · 2017-11-21T13:59:33Z

include/git2/status.h

+  git_status_list **out,
+  git_repository *repo,
+  const git_status_options *opts,
+  git_tree *head);


Please try to match indetation as it exists in surrounding source code

pks-t · 2017-11-21T14:00:19Z

include/git2/status.h

+  git_status_list **out,
+  git_repository *repo,
+  const git_status_options *opts,
+  git_tree *head);


Can't the head argument also be const?

pks-t · 2017-11-21T14:00:43Z

src/status.c

+done:
+  git_tree_free(head);
+  return error;
+}


Same here. Please try to match existing indentation (tabs instead of spaces).

pks-t · 2017-11-21T14:01:39Z

include/git2/status.h

+ * @param out Pointer to store the status results in
+ * @param repo Repository object
+ * @param opts Status options structure
+ * @param head Tree to compare the index to


Please specify that head can actually be NULL (behaving as an empty iterator then).

pks-t · 2017-11-21T14:15:29Z

include/git2/status.h

+ * @return 0 on success or error code
+ */
+GIT_EXTERN(int) git_status_list_new_at(
+  git_status_list **out,


I'm a little hesitant regarding this function's name. at doesn't really tell the user anything about what it should be at, actually. An option would be git_status_list_new_with_tree, for example.

pks-t · 2017-11-21T14:16:43Z

include/git2/status.h

@@ -173,12 +173,17 @@ typedef enum {
 * The `pathspec` is an array of path patterns to match (using


Pleas mind the preferred commit message style. First again, the area-prefix ("status" in this case), and then please also mind that we wrap the body at 80 characters. Also, please try to make the message more imperative instead arguing in place of the project, not yourself.

I've now created PR #4422 to document our style. There's still two minor violations:

the first letter after the area-prefix is usually lowercased

the subject is some characters too long

Proposed: "status: add option to compare to trees other than head". You could add some reasoning in the commit message body, but that's only optional.

pks-t · 2017-11-21T14:17:37Z

tests/status/worktree.c

@@ -1299,7 +1299,8 @@ void test_status_worktree__at_head_parent(void)
  cl_git_rewritefile("empty_standard_repo/file2", "pyng");


Same regaring the commit message. Furthermore, it feels like this should be squashed in to the initial commit where the test has been created with git_status_list_new taking the option?

pks-t · 2017-11-21T14:20:27Z

include/git2/status.h

@@ -298,31 +298,6 @@ GIT_EXTERN(int) git_status_file(
 	git_repository *repo,
 	const char *path);

-/**


Please squash this commit into the initial commit where you created that function

pks-t · 2017-11-21T14:20:48Z

src/status.c

@@ -283,8 +283,7 @@ int git_status_list_new(

 	if (opts != NULL && opts->head_tree != NULL) {
 		head = opts->head_tree;
-	}
-	else {
+	} else {


Please squash that commit into the original commit where you created that line

Uncommon · 2017-11-21T15:29:02Z

Updated and squashed. Thanks for the notes.

BTW where is the commit message style documented? I don't see it in CONTRIBUTING or CONVENTIONS.

Uncommon · 2017-11-21T17:38:28Z

Oops, forgot to update the test.

pks-t · 2017-11-22T15:27:18Z

Thanks for yout update!

Good hint regarding the commit messages. It's mostly a thing I try to personally point at where reasonable, the others are more lenient on that side. I'll document that and review your changes coming Friday (hopefully sigh)

pks-t

Thanks a lot @Uncommon. Despite three really minor stylistic nits which require fixing up, I'm now completely happy with this PR regarding functionality. I'd like to hear @ethomson opinion though if he's happy with that approach now.

pks-t · 2017-11-24T11:04:22Z

src/status.c

 	}
-
+	


There's trailing whitespace here

pks-t · 2017-11-24T11:36:15Z

include/git2/status.h

@@ -173,12 +173,17 @@ typedef enum {
 * The `pathspec` is an array of path patterns to match (using


I've now created PR #4422 to document our style. There's still two minor violations:

the first letter after the area-prefix is usually lowercased

the subject is some characters too long

Proposed: "status: add option to compare to trees other than head". You could add some reasoning in the commit message body, but that's only optional.

pks-t · 2017-11-24T11:36:54Z

tests/status/worktree.c

+
+  git_tree_free(parent_tree);
+  git_status_list_free(statuslist);
+}


You are using spaces instead of tabs here

ethomson · 2017-11-06T20:02:39Z

src/status.c

 	}
-
+	


Please drop this whitespace addition.

ethomson · 2017-11-24T11:45:13Z

include/git2/status.h

+ *
+ * The `tree` is a tree to be used instead of the head commit's tree for
+ * comparing with the index. For example, this can be useful when preparing
+ * to do an amend commit.


I don't think that the discussion of an amend commit makes sense in the context of this documentation. You should also explicitly indicate that it defaults to HEAD. (I know that you're implicitly stating it). Something like:

The tree is the tree to be used for comparison to the working directory and index; defaults to HEAD.

ethomson · 2017-11-24T11:45:16Z

include/git2/status.h

 */
 typedef struct {
 	unsigned int      version;
 	git_status_show_t show;
 	unsigned int      flags;
 	git_strarray      pathspec;
+	git_tree          *tree;


We use the term baseline for the overrideable HEAD in git_checkout. I wonder if baseline would be more consistent here, too?

ethomson · 2017-11-24T11:46:30Z

Yeah, I think this approach is great. Thanks for all your work on this @Uncommon. I might tweak some names for consistency with some other APIs but otherwise this is looking good.

Uncommon · 2017-11-27T15:31:05Z

Fixed and rebased. Thanks for the reviews.

pks-t · 2017-11-30T13:21:26Z

Yeah, I think baseline is perfect, thanks for that.

You still forgot to fix up whitespaces for your test. As soon as that is amended I'll be happy to merge.

Uncommon · 2017-11-30T14:57:22Z

Oops, forgot about the tabs. Thanks.

…rees other than HEAD

ethomson · 2017-12-01T11:45:50Z

AWESOME. Thanks for this! ✨

Uncommon changed the title ~~Add git_status_list_new_at~~ Add git_status_file_at Sep 5, 2017

Uncommon mentioned this pull request Sep 5, 2017

Implement amend Uncommon/Xit#66

Closed

5 tasks

Uncommon force-pushed the amend_status branch from 60f1afd to 7315d47 Compare September 26, 2017 17:16

pks-t requested changes Oct 6, 2017

View reviewed changes

Uncommon force-pushed the amend_status branch from 7315d47 to 495f2b3 Compare October 20, 2017 22:55

pks-t requested changes Nov 21, 2017

View reviewed changes

Uncommon force-pushed the amend_status branch from b738bab to c7a08b8 Compare November 21, 2017 15:27

Uncommon force-pushed the amend_status branch from c7a08b8 to daeab8f Compare November 21, 2017 17:38

pks-t requested changes Nov 24, 2017

View reviewed changes

ethomson reviewed Nov 24, 2017

View reviewed changes

Uncommon force-pushed the amend_status branch from daeab8f to 1d8ea38 Compare November 27, 2017 15:30

Uncommon force-pushed the amend_status branch from 1d8ea38 to 4ccacdc Compare November 30, 2017 14:56

status: Add a baseline field to git_status_options for comparing to t…

4ccacdc

…rees other than HEAD

ethomson merged commit 429bb35 into libgit2:master Dec 1, 2017

Uncommon deleted the amend_status branch December 1, 2017 15:17

Uncommon restored the amend_status branch December 1, 2017 22:58

		@@ -173,12 +173,17 @@ typedef enum {
		* The `pathspec` is an array of path patterns to match (using

		@@ -1299,7 +1299,8 @@ void test_status_worktree__at_head_parent(void)
		cl_git_rewritefile("empty_standard_repo/file2", "pyng");

Add git_status_file_at #4318

Add git_status_file_at #4318

Conversation

Uncommon commented Jul 24, 2017

ethomson commented Jul 24, 2017

Uncommon commented Jul 24, 2017 • edited

Uncommon commented Jul 24, 2017

Uncommon commented Jul 27, 2017

Uncommon commented Jul 28, 2017

Uncommon commented Aug 9, 2017

pks-t commented Sep 1, 2017

Uncommon commented Sep 1, 2017

Uncommon commented Sep 5, 2017 • edited

Uncommon commented Sep 5, 2017

Uncommon commented Sep 6, 2017

Uncommon commented Sep 7, 2017

ethomson commented Sep 8, 2017

Uncommon commented Sep 10, 2017

ethomson commented Sep 10, 2017

Uncommon commented Sep 19, 2017

pks-t commented Sep 20, 2017

Uncommon commented Sep 20, 2017

ethomson commented Sep 20, 2017

Uncommon commented Sep 20, 2017

Uncommon commented Sep 26, 2017

Uncommon commented Oct 4, 2017

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uncommon Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uncommon commented Oct 20, 2017

Uncommon commented Oct 23, 2017 • edited

Uncommon commented Oct 31, 2017

pks-t commented Nov 3, 2017

Uncommon commented Nov 17, 2017

pks-t left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uncommon commented Nov 21, 2017

Uncommon commented Nov 21, 2017

pks-t commented Nov 22, 2017

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethomson commented Nov 24, 2017

Uncommon commented Nov 27, 2017

pks-t commented Nov 30, 2017

Uncommon commented Nov 30, 2017

ethomson commented Dec 1, 2017

Uncommon commented Jul 24, 2017 •

edited

Uncommon commented Sep 5, 2017 •

edited

Uncommon Oct 10, 2017 •

edited

Uncommon commented Oct 23, 2017 •

edited

pks-t left a comment •

edited