Thank you for the feedback Denton & Phillip! On Fri, Dec 6, 2019 at 5:52 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > On 06/12/2019 01:48, Denton Liu wrote: > > nit: * should be attached to the variable name. > > I think you also need to free it once you've called get_fork_point() as > well. Yup. Got it. > On 06/12/2019 01:48, Denton Liu wrote: > > > >> + dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name); > > > > Also, thinking about this more, would it be possible to put the dwim_ref > > logic into get_fork_point() directly? There are currently only these two > > callers so I suspect it should be fine and it'll result in cleaner > > logic. > > If you do that then it would be better to use error() rather than die() > in get_fork_point() and return an error to the caller as we try to avoid > adding code to libgit that dies. This lets the caller handle any cleanup > that they need to before exiting. Would the signature of get_fork_point change to be something like: int get_fork_point(const char *refname, struct commit *commit, struct commit **fork_point, struct strbuf *err) If not, could you point me to an example of some existing code that does what you're talking about? > On 06/12/2019 01:48, Denton Liu wrote: > > It's not obvious why this was failing in the first place. Perhaps we > > could document it better in the commit message? > > > > Maybe something like: > > > > We used to pass in the upstream_name directly into the > > get_fork_point() machinery. However, get_fork_point() was > > expecting a fully qualified ref name even though most users use > > the short name for branches. This resulted in `--fork-point` not > > working as expected since, without the full ref name, the reflog > > lookup would fail and it would behave as if we weren't passing > > in `--fork-point` at all. Sounds good. > > Also, I'm not why this test case in particular that was duplicated (and > > not the one above) given that the first three `--fork-point` test cases > > fail without the change to rebase. Perhaps we want to duplicate all > > "refs/heads/master" tests with a corresponding "master" test? I only duplicated one so that there would only be one test case that would fail if a regression around getting the fork point with a short ref name was introduced. I just happened to pick that one because it was closest to the rebase command I was running when I found the bug :) I can include some of the above reasoning in the commit message. Alternatively: * I could duplicate all of tests * I could change all of the tests to use the short ref name I'm leaning towards just leaving one test (maybe with a comment?) for the short ref name --fork-point so that there is more resolution around where a bug could be on test failure. Let me know what you think, Alex
