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

Allow merge analysis against any reference #4770

Merged
merged 3 commits into from
Nov 30, 2018

Conversation

tiennou
Copy link
Contributor

@tiennou tiennou commented Aug 17, 2018

This adds a new git_merge_analysis_any function that can be used to check a merge against any given reference, as right now we only support checking HEAD.

Inspired by libgit2/objective-git#629.

@tiennou tiennou force-pushed the feature/merge-analysis-any-branch branch from c7f758b to e93dacc Compare August 17, 2018 20:31
src/refs.h Outdated
* Otherwise, returns GIT_EINVALID if the reference is not symbolic,
* GIT_EUNBORNBRANCH if the reference is unborn, or another error code.
*/
int git_reference__resolve_unborn(git_reference **out_ref, git_repository *repo, const git_reference *ref);
Copy link
Member

Choose a reason for hiding this comment

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

What about git_reference__resolve_symbolic? resolve_unborn sounds a lot like it is only able to resolve unborn branches, which is not the case. It is rather a function which will resolve symbolic refs only and which doe not care whether the target is unborn or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, naming is somewhat confusing… The 2nd part of your "understanding" is wrong : it is the only function that checks and errors on unborn symbolic branches.

I could fold that check in git_reference_lookup_resolved, adding an allow_unborn parameter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to #4780.

* @param our_ref the reference to perform the analysis from
* @return 0 on success or error code
*/
GIT_EXTERN(int) git_merge_analysis_any(
Copy link
Member

Choose a reason for hiding this comment

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

git_merge_analysis_with_ref? any is much too generic in my opinion.

I also wonder about whether one should pass in our_ref as an annotated commit, as well. All the others are already annotated commits, and the first thing we do is to look up the annotated commit from the ref.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Argument documentation out-of-order. Also, a vararg version might be nice-to-have)

I can change the name, yes, but maybe if we make the function completely generic it's less of a problem… I'm on really shaky ground w.r.t what's considered valid mergeable objects, so I don't know if going full-on with annotated commits makes sense (like annotated_from_fetchhead).

I'd prefer to just switch the type of our_ref and keep mandatory arguments explicit.

Note that, since I've been doing some "customer support", AFAICT we're lacking a generic "annotated" git_merge function anyways, so the benefits to a fully generic-annotated version are minor.

src/refs.c Outdated
if (error != 0 && error != GIT_EUNBORNBRANCH)
return error;

return error == GIT_EUNBORNBRANCH ? true : false;
Copy link
Member

Choose a reason for hiding this comment

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

I think we usually avoid reusing the return code as value and error code. So maybe use an out parameter instead to return the boolean?

@pks-t
Copy link
Member

pks-t commented Aug 24, 2018

This is definitely a good thing to have, so I'm sold!

@tiennou tiennou force-pushed the feature/merge-analysis-any-branch branch from e93dacc to fb1b269 Compare August 25, 2018 00:39
@tiennou tiennou force-pushed the feature/merge-analysis-any-branch branch from fb1b269 to a8f64d0 Compare August 25, 2018 01:29
@tiennou
Copy link
Contributor Author

tiennou commented Aug 25, 2018

Rebased. I'll try to see if I can make merge handle any base branch without much work, so please don't merge as is.

@tiennou tiennou force-pushed the feature/merge-analysis-any-branch branch 2 times, most recently from 34b6ddd to e74189d Compare August 25, 2018 02:16
@pks-t
Copy link
Member

pks-t commented Aug 30, 2018

Okay, I'll hold off my review until then

@tiennou tiennou force-pushed the feature/merge-analysis-any-branch branch 2 times, most recently from 6a92427 to e578bd6 Compare October 16, 2018 21:13
@tiennou
Copy link
Contributor Author

tiennou commented Oct 16, 2018

Rebased, and ready for review. I won't do git_merge_for_ref, because I feel it requires too much fiddling around (eg. MERGE_HEAD and things), and doesn't look as useful as the use case for analyzing merges.

Copy link
Member

@pks-t pks-t left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, the only concern I have is about the missing cl_git_pass wrappers.

tests/merge/workdir/analysis.c Outdated Show resolved Hide resolved
tests/merge/workdir/analysis.c Outdated Show resolved Hide resolved
src/refs.c Outdated Show resolved Hide resolved
@tiennou tiennou force-pushed the feature/merge-analysis-any-branch branch from e578bd6 to c271f07 Compare October 19, 2018 17:16
This moves the current merge analysis code into a more generic version
that can work against any reference.

Also change the tests to check returned analysis values exactly.
@tiennou tiennou force-pushed the feature/merge-analysis-any-branch branch from c271f07 to cb71a9c Compare October 19, 2018 18:39
@tiennou
Copy link
Contributor Author

tiennou commented Oct 19, 2018

Fixed and rebased. Thanks for the review @pks-t !

else if (error == GIT_ENOTFOUND && git__strcmp(ref->name, GIT_HEAD_FILE) == 0)
*unborn = true;
else
*unborn = false;
Copy link
Member

Choose a reason for hiding this comment

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

Just making sure that I understand the decisions from #4780. I'm thinking that at present that if we encounter a broken symref that's not HEAD that we will return that unborn is false? Should this block look like:

if (error == GIT_ENOTFOUND && git__strcmp(ref->name, GIT_HEAD_FILE) == 0)
    *unborn = true;
else if (!error)
    *unborn = false;

return error;

(Ignoring the style around ifs and such, just trying to make sure that I understand the logic.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's true, a broken symref whose name is not HEAD will not be considered unborn, and will not error (at least here). This is the check we're doing git_repository_head BTW, with the HEAD check made more explicit.

I find your proposed block harder to reason about w.r.t to error, but I think it still returns errors for cases that would be fine (the 1st condition would still be GIT_ENOTFOUND at least). Something like this, if the inline return is not okay :

if (error == 0)
    *unborn = false;
else if (error == GIT_ENOTFOUND && git__strcmp(ref->name, GIT_HEAD_FILE) == 0) {
    *unborn = true;
    error = 0;
} else
    /* error will fallthrough */

Your version is a step after this one, so now I get it 😉. Yours is only missing the error = 0 in the "unborn" handler.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're very much correct, I was missing that error reset.

@pks-t
Copy link
Member

pks-t commented Oct 25, 2018

I'm happy with this PR now, thank you @tiennou! @ethomson, are you good, too?

@pks-t pks-t merged commit 0ddc609 into libgit2:master Nov 30, 2018
@pks-t
Copy link
Member

pks-t commented Nov 30, 2018

Thanks again, @tiennou!

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.

3 participants