Add git_status_file_at - #4318
Conversation
|
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 I say that especially since |
|
If I make a change to |
|
Looks like AppVeyor failed because of credentials tests. |
|
I'd also like to do a similar thing with |
|
Actually on further inspection I see that |
|
As noted above, I'm adding The question remains: if I'm changing |
|
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. |
|
Ok, thanks! |
|
I've pushed my new changes as described above: added the options field and |
|
Looks like I forgot to update the unit tests. |
|
Looks like AppVeyor failed with a Windows ssl problem. |
|
One of the Travis builds seems to have stalled out on |
|
I'll requeue these builds. |
|
Thanks. AppVeyor errors look unrelated again. |
|
Sigh. No idea what's going on with the AppVeyor build. |
|
Did #4347 fix the build issues seen here? |
|
Yup, exactly. Can you please rebase your changes onto my fix? |
|
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.
|
|
@Uncommon did you re-run cmake? It caches locations to things that are probably now missing. If you have not, delete your |
|
I also had to delete tests/clar.suite. Thanks to @pks-t for that pointer. |
60f1afd to
7315d47
Compare
|
Rebased, and tests are passing for me locally. |
|
Poke again. Builds are green. I have a feature waiting on this, so I'd like to get it in. |
pks-t
left a comment
There was a problem hiding this comment.
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.
| GIT_EXTERN(int) git_status_file_at( | ||
| unsigned int *status_flags, | ||
| git_repository *repo, | ||
| const char *path, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| head = opts->head_tree; | ||
| } | ||
|
|
||
| else { |
There was a problem hiding this comment.
We usually have closing braces and else branches on the same line.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I went ahead and did this one too.
There was a problem hiding this comment.
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?
| 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; |
There was a problem hiding this comment.
Typically, we do not use camelcase variable names but instead use underscores.
| cl_git_mkfile("empty_standard_repo/file1", "ping"); | ||
| stage_and_commit(repo, "file1"); | ||
|
|
||
| git_repository_head_tree(&parentTree, repo); |
There was a problem hiding this comment.
Please wrap this in an cl_git_pass.
|
|
||
| opts.show = GIT_STATUS_SHOW_INDEX_AND_WORKDIR; | ||
| opts.head_tree = parentTree; | ||
| git_status_list_new(&statuslist, repo, &opts); |
| git_status_list_new(&statuslist, repo, &opts); | ||
|
|
||
| cl_assert_equal_sz(1, git_status_list_entrycount(statuslist)); | ||
| status = git_status_byindex(statuslist, 0); |
There was a problem hiding this comment.
Please wrap this in an cl_assert, so that we do not segfault on a NULL pointer here.
7315d47 to
495f2b3
Compare
|
Updated with a little bug fix. Coming soon, I'll remove |
|
@pks-t Requested changes to the unit test code have been addressed. I've also removed |
|
Any further thoughts on this? |
|
Sorry, I'm currently busy with a move. I'll find some time next week to have a further look |
|
It's been a couple of weeks, so I'm bumping this again. |
There was a problem hiding this comment.
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.
| GIT_EXTERN(int) git_status_list_new_at( | ||
| git_status_list **out, | ||
| git_repository *repo, | ||
| const git_status_options *opts, |
There was a problem hiding this comment.
THe options field is usually the last one in our functions.
| git_status_list **out, | ||
| git_repository *repo, | ||
| const git_status_options *opts, | ||
| git_tree *head); |
There was a problem hiding this comment.
Please try to match indetation as it exists in surrounding source code
| git_status_list **out, | ||
| git_repository *repo, | ||
| const git_status_options *opts, | ||
| git_tree *head); |
There was a problem hiding this comment.
Can't the head argument also be const?
| done: | ||
| git_tree_free(head); | ||
| return error; | ||
| } |
There was a problem hiding this comment.
Same here. Please try to match existing indentation (tabs instead of spaces).
| * @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 |
There was a problem hiding this comment.
Please specify that head can actually be NULL (behaving as an empty iterator then).
| * @return 0 on success or error code | ||
| */ | ||
| GIT_EXTERN(int) git_status_list_new_at( | ||
| git_status_list **out, |
There was a problem hiding this comment.
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.
| @@ -173,12 +173,17 @@ typedef enum { | |||
| * The `pathspec` is an array of path patterns to match (using | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -1299,7 +1299,8 @@ void test_status_worktree__at_head_parent(void) | |||
| cl_git_rewritefile("empty_standard_repo/file2", "pyng"); | |||
There was a problem hiding this comment.
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?
| git_repository *repo, | ||
| const char *path); | ||
|
|
||
| /** |
There was a problem hiding this comment.
Please squash this commit into the initial commit where you created that function
| head = opts->head_tree; | ||
| } | ||
| else { | ||
| } else { |
There was a problem hiding this comment.
Please squash that commit into the original commit where you created that line
b738bab to
c7a08b8
Compare
|
Updated and squashed. Thanks for the notes. BTW where is the commit message style documented? I don't see it in CONTRIBUTING or CONVENTIONS. |
|
Oops, forgot to update the test. |
c7a08b8 to
daeab8f
Compare
|
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) |
| } | ||
| } | ||
|
|
||
| @@ -173,12 +173,17 @@ typedef enum { | |||
| * The `pathspec` is an array of path patterns to match (using | |||
There was a problem hiding this comment.
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.
|
|
||
| git_tree_free(parent_tree); | ||
| git_status_list_free(statuslist); | ||
| } |
There was a problem hiding this comment.
You are using spaces instead of tabs here
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Please drop this whitespace addition.
| * | ||
| * 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. |
There was a problem hiding this comment.
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
treeis the tree to be used for comparison to the working directory and index; defaults to HEAD.
| git_status_show_t show; | ||
| unsigned int flags; | ||
| git_strarray pathspec; | ||
| git_tree *tree; |
There was a problem hiding this comment.
We use the term baseline for the overrideable HEAD in git_checkout. I wonder if baseline would be more consistent here, too?
|
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. |
daeab8f to
1d8ea38
Compare
|
Fixed and rebased. Thanks for the reviews. |
|
Yeah, I think You still forgot to fix up whitespaces for your test. As soon as that is amended I'll be happy to merge. |
1d8ea38 to
4ccacdc
Compare
|
Oops, forgot about the tabs. Thanks. |
…rees other than HEAD
|
AWESOME. Thanks for this! ✨ |
This change takes
git_status_list_newand splits out a new function calledgit_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.