Re: [PATCH v2 2/2] rebase: find --fork-point with full ref

From: Alex Torok <alext9@gmail.com>
Date: Dec 6, 2019, 13:46 UTC
Message-ID: <CANmPhj38UqZiePEPbPJBwUTOEJrfA6j3aP8KBHvAY6EA-J7Nsw@mail.gmail.com>
In-Reply-To: <e898ad43-5649-8f4c-7c93-fab09197fc92@gmail.com>

Thank you for the feedback Denton & Phillip!

On Fri, Dec 6, 2019 at 5:52 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 5 quoted lines

> 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.

Show 13 quoted lines

> 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?

Show 13 quoted lines

> 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