search

sourcegraph / go-vcs

manipulate and inspect VCS repositories in Go
https://sourcegraph.com/sourcegraph/go-vcs
Other
79 stars 20 forks source link

vcs/gitcmd: need a way to UpdateEverything and propogate error/stderr #99

Open devastating opened 7 years ago

devastating commented 7 years ago

Hello,

We use http://gitolite.com/gitolite/ as our Git host, and an issue we have is that if a "git push" encounters error with one of the mirroring server (e.g. fail to push to mirror A, but succeed to mirror B), the following "git fetch" command will put a warning message in stderr, even if the "git fetch" works correctly.

This breaks the usage of "gitcmd.UpdateEverything" as it tries to parse the stderr. Could I work with your team to come up with a fix so that we could optionally skip the parsing stderr part?

I've come up with a diff https://github.com/devastating/go-vcs/commit/c42b5bcbca473c4a7a279d7a228c6b898f688b61 to attempt to address the issue. It should not break anything, but does create a new public API. Do you have any suggestions? Thank you so much.

Nick

dmitshur commented 7 years ago

the following "git fetch" command will put a warning message in stderr, even if the "git fetch" works correctly.

What kind of warning message is it? Is it maybe possible to detect it in parseRemoteUpdate and skip over it?

Another option, albeit not very clean, is to add a new field to RemoteOpts to disable parsing result of updating everything. It's not very clean because RemoteOpts are embedded in CloneOpt, which is used by Clone, so it doesn't really belong there.

devastating commented 7 years ago

Unfortunately, I think the error message is gitolite-only so I don't think we should parse it. You can see the similar issue in this thread: https://groups.google.com/forum/#!topic/gitolite/Yhr-HGgaqu4

We've also considered adding additional ops to RemoteOpts like you suggested, and like you said, it did not seem to fit there.

As a result, in the commit I made, I've added a new API, since we are going to add public method/member anyway. Any other thoughts are welcome.

devastating commented 7 years ago

@shurcooL any other suggestions that I could make a change?

dmitshur commented 7 years ago

We've also considered adding additional ops to RemoteOpts like you suggested, and like you said, it did not seem to fit there.

As a result, in the commit I made, I've added a new API, since we are going to add public method/member anyway. Any other thoughts are welcome.

A new method is going to affect the users of this package too, so if you're changing the API, it probably makes sense to do what leads to the best API in the end, rather than trying to make a minimal change.

You can look into the discussion at #79 for further information or insight.

I don't have further suggestions at this time.

That said, I'm not sure who has ownership (i.e., can make decisions about breaking API changes) over this package now.

devastating commented 7 years ago

@shurcooL Thank you for your response..seems like this was an existing issue for a while (whether or not parsing the output).