Thank you for the feedback Denton & Phillip!

On Fri, Dec 6, 2019 at 5:52 AM Phillip Wood [off-list ref] wrote:

On 06/12/2019 01:48, Denton Liu wrote:

quoted

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:

quoted

quoted

+            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:

quoted

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.

quoted

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