PushInfo is documented to return a local reference, but a pushed commit need not be a reference

[zfigura]

Hi!

It is a perhaps little known fact that one can use git push to push something that is not a reference, with syntax like

git push 1a2b3c4d:refs/heads/remotebranch

This is possible with GitPython, but subtly broken. The Repo.push() command technically succeeds, but returns a zero-length array and no errors.

This is because when building a PushInfo struct, GitPython expects the local commit that is pushed to be a reference, and this is not always true. When unable to convert the local commit into a reference, GitPython simply gives up and doesn't build that PushInfo.

Fixing this would be relatively trivial, and I'd love to submit the patch to do it (props, by the way, for writing very straightforward and readable code.) The trouble is that this specific detail is documented:

info.local_ref      # Reference pointing to the local reference that was pushed
                    # It is None if the ref was deleted.

and any change we do here would imply breaking the API. How should we deal with this? I'd be glad to implement whatever solution is decided on.

[Byron]

Thanks for looking into this and offering a fix, it's much appreciated!

I suggest to keep local_ref as is and add another field like local_hash, which is set to the object id that was used for pushing, or None if that wasn't the case. If it is set, local_ref might be set to None instead. With the documentation adjusted, I'd think that the addition to the data model can represent the possible states.

Feel free to experiment and find other, better solutions though, everything goes except breaking the existing API :D. Thanks for your understanding.

[zfigura]

Feel free to experiment and find other, better solutions though, everything goes except breaking the existing API :D. Thanks for your understanding.

Yeah, it's tricky because it kind of depends on what counts as breaking the existing API... and I've seen maintainers take all sorts of positions on this.

Adding an extra local_hash field and setting local_ref to None seems reasonable, and something like that occurred to me even. The potential problem I see is that there might well be code out there that does something like

push_infos = repo.push(...)
for push_info in push_infos:
# do something with push_info.local_ref

which would previously "work", but would now crash.

A potentially safer approach, if we're worried about this, could be to just reroll the API entirely, i.e. add a "Repository.push2()" method. We don't even need to reroll PushInfo in this case; we just need to make sure that the v1 .push() never returns a PushInfo with a local_ref of None.

[Byron]

Thanks for the analysis!

local_ref is already declared as Union[SymbolicReference, None], so setting it to None while having a second field named local_hash seems to be the way to go to resolve this. Existing code keeps 'not crashing' and behaving as before, while code that wants to deal with this now finds the information they seek.

Thanks again for your continued help with this!