Implementing merge with default options #295

Merged
merged 9 commits into from Dec 10, 2013

Projects

None yet

4 participants

@victorgp
Contributor
victorgp commented Dec 4, 2013

This pull request implements the merge operation based on the default options libgit2 defines.

The merge receives an Oid and returns a MergeResult object.

This MergeResult object is quite similar than the git_merge_result libgit2 implements. it has:

->is_fastforward: boolean field that indicates the merge was fastforward
->is_uptodate: boolean field that indicates the merge was already up to date
->fastforward_oid: python Oid object with the new fastforward Oid to be the new head (only if the merge was fastforward, None otherwise)
->status: dict with the repository status after the merge (if the merge was not fastforward, either there were conflicts or not, an empty dict otherwise)

Some notes:
-The merge, as it name says, only does the merge, it does not perform any commit nor updates the references (ex. in the fastforward case)
-This method does not allow customized parameters for the merge operation, i would like to leave that for a second iteration over this method and let's it work for the moment with the default options defined in the libgit2 constant GIT_MERGE_OPTS_INIT.

The tests i implemented would help to understand this behaviour easily.

Extra: there is also a minor change in the Repository_status method, i removed the args parameter because it wasn't being used anywhere within the method.

@jdavid

If there is an error here, I understand you don't have any merge result to free.

Owner

I think it depends on when the error was raised inside the git_merge function, but taking a look to that funcion, the merge_result value is set just before returning, therefore it can't fail after assigning the content to merge_result, so you are right, i should remove that git_merge_result_free.

But, anyway, i would prefer to initialize this pointer to NULL so i can check before it is freed if they got anything or not, so i don't need to know anything about the insides of the functions called, although i see you don't initialize any pointer to NULL in this library.

So, it's up to you, i don't really care, i can remove that line or initialize the pointer to NULL, which one do you prefer?

Yes, pygit2 assumes libgit2 does not set results when there is an error. Maybe some libgit2 developer, @carlosmn?, can say whether this is a safe assumption or not.

But yes if you prefer, initialize to NULL, and maybe just goto error for cleanup?

In case it encounters an error, a libgit2 function will free any resources it has allocated, but you cannot depend on the output parameters pointing to anything valid. They're either left alone or set to NULL (this depends a bit on who wrote the code), but what you definitely never need to do is call a free function on an output parameter in case of error.

Okey, so we are doing it right.

That said, error handling in pygit2 would be simplified if libgit2 did set return values to NULL on error systematically. Maybe I should open an issue with examples...

Owner

Ok, so if i understand correctly i need to remove that line that frees the variable.
But then, the same would happen with the git_merge_head_free(oid_merge_head) line used in error as we cannot be sure oid_merge_head is already set, therefore i think the correct solution is:
-remove the git_merge_head_free(oid_merge_head); in the error: section
-change git_merge_result_free(merge_result); for git_merge_head_free(oid_merge_head);

Do you agree?

yes it is right in new code

@jdavid

tabs, trailing whitespace and one blank line to remove (the trailing whitespace is in the blank line)

Owner

You are right, i'll fix that

@jdavid

No error check here. PyObject_New may return NULL.

Owner

You are right, i'll fix that

@jdavid
Member
jdavid commented Dec 5, 2013

Hello Victor, thanks for contributing.

We have the general policy to wrap git structs, so it should be like:

typedef struct {
    PyObject_HEAD
    git_merge_result *result;
} MergeResult;

Note that Repository_status is "expensive" and may fail. Do not keep it.

And please update the documentation docs/merge.rst

@jdavid

As for PEP 7 brace position is not good. See http://www.python.org/dev/peps/pep-0007/#code-lay-out

Owner

You are right, i'll fix that

@victorgp
Contributor
victorgp commented Dec 5, 2013

Thanks @jdavid for your comments.

About the MergeResult struct, you are right, i'll implement it in that way.

@victorgp
Contributor
victorgp commented Dec 9, 2013

Fixes done, please take a look and tell me if you like them.
Thanks

@jdavid
Member
jdavid commented Dec 10, 2013

Why do you need MergeResult.index?

merge = repo.merge(...)
merge.index

How that is better than:

merge = repo.merge(...)
repo.index

If MergeResult does not keep a reference to the repository, code would be simpler.

@victorgp
Contributor

Actually i don't need it but since the git_merge_result struct has an index field, i supposed you also wanted it here.

I'll remove it so it's simpler

@jdavid
Member
jdavid commented Dec 10, 2013

Okey, I missed that. Anyway if you don't need it just remove.

@victorgp
Contributor

Index removed!

@jdavid jdavid merged commit 1938147 into libgit2:master Dec 10, 2013

1 check passed

default The Travis CI build passed
Details
@alexband
alexband commented Jan 9, 2014

@victorgp does merge_result has any attributes to indicate this merge is viable? like will it produce conflicts?

there is 'is_fastforward', 'is_up_to_date', but no other identification for whether the merge is viable, (is it can be merged but not fastforward merge?)

@alexband
alexband commented Jan 9, 2014

sorry I found that merge_result has an index object, that explains

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment