Handling Push Errors in Porcelain

[chamila]

Hi,

I'm having trouble understanding the recommended way to handle errors from Git when using the Porcelain API. I'm reading the documentation at [1] for reference.

During clone, push, and other operations how is the caller supposed to handle errors in the Git layer? For an example, consider the following code for pushing to a remote.

            try:
                porcelain.push(
                    repo=repo_path,
                    remote_location="fork",
                    refspecs=bytes(f"HEAD:refs/heads/{branch}", "utf-8"),
                )

                logger.info("github push done")
            except Exception as e:
                logger.error("error while pushing changes to remote", err=e)
                return False
            else:
                return True

Now this wouldn't do the trick since porcelain.push() doesn't throw an error if for example, push fails in Git because of an auth issue.

I played around with the `errstream` argument, however other than acting as the stderr, it doesn't provide any definitive method to detect if the push operation failed. I could attach a file handler to `errstream` and afterwards detect if the words "Push of * failed" is in that file, however I feel that approach could be really brittle.

Have I gone down the wrong path here and is there a better way to handle errors in this scenario?

I'm guessing if we raise an Error at [2] like we do at [3], this could be solved. Is there a reason for not raising an error at [2]?

Also, another way to tackle this would be to return a list of errors from push() method, so that the caller can decide how to act.

[1] - https://www.dulwich.io/docs/api/dulwich.porcelain.html#dulwich.porcelain.push

[2] - https://github.com/jelmer/dulwich/blob/master/dulwich/porcelain.py#L1084-L1090

[3] - https://github.com/jelmer/dulwich/blob/master/dulwich/porcelain.py#L1075-L1078

Thanks!

Regards,

Chamila

https://chamila.dev/blog

[Jelmer Vernooij]

Hi Chamila,

We're not raising an exception today since push can be partially successful - e.g. if you're pushing multiple refs then some may be successful, while others fail.

I'm open to changing the interface - we could e.g. return an object that contained a dict mapping each ref to its results in addition to writing to stderr.

Jelmer

--
You received this message because you are subscribed to the Google Groups "dulwich-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to dulwich-discu...@googlegroups.com.
To view this discussion on the web, visit https://groups.google.com/d/msgid/dulwich-discuss/CAB2A5_%3DrGG-UXrZeSqZEzjoD9NGT%3Dj8qmB3mCYjVwPjytmPr5w%40mail.gmail.com.

[chamila]

Hi Jelmer,

> We're not raising an exception today since push can be partially successful - e.g. if you're pushing multiple refs then some may be successful, while others fail.

Thanks for your reply! Yeah, I think I can understand the reasons behind not raising an error. And even if we do decide to do so now, it will break a lot of  existing code, right?

Let me see if I can implement that change related to returning the status, so that it would contain something like

{

    "ref1": {

      "error": True,

      "messages": [

        "error message 1",

        "error message 2"

      ]

    },

    "ref2": {

      "error": False,

      "messages": []

      }

    }

}

Regards,

Chamila

https://chamila.dev/blog

[Jelmer Vernooij]

Hi Chamila,

For style reasons, I'd prefer to return an object rather than a dictionary but otherwise that all makes sense to me.

PRs very much welcome :)

Jelmer