Hi Ævar & Peff,

On Wed, 13 Mar 2019, Jeff King wrote:

> On Wed, Mar 13, 2019 at 05:11:44PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> > > And this is where the loose object cache interferes with this
> > > feature: if *some* loose object was read whose hash shares the same
> > > first two digits with a commit that was not yet created when that
> > > loose object was created, then we fail to find that new commit by
> > > its short name in `get_oid()`, and the interactive rebase fails with
> > > an obscure error message like:
> > >
> > > 	error: invalid line 1: pick 6568fef
> > > 	error: please fix this using 'git rebase --edit-todo'.
> 
> Are we 100% sure this part is necessary? From my understanding of the
> problem, even without any ambiguity get_oid() could fail due to just
> plain not finding the object in question.

Indeed. It could be a typo, for example. Which is why that error message
is so helpful.

> > As a further improvement, is there a good reason for why we wouldn't
> > pass something down to the oid machinery to say "we're only interested
> > in commits". I have a WIP series somewhere to generalize that more, but
> > e.g.  here locally:
> 
> We have get_oid_commit() and get_oid_committish() already. Should rebase
> just be using those? (I think we probably want "commit()", because we do
> not expect a "pick" line to have a tag, for example.

I did think about this while developing this patch series, and decided
against conflating concerns.

And I was totally right to do so! Because I do have an internal ticket
that talks about allowing `reset v2.20.1`, which is a tag, not a commit.

Granted, it is easy to work around: just use `reset v2.20.1^0`, but it is
quite annoying that we do not allow this at the moment: even if we do
allow `get_oid()` to resolve the tag, we don't peel it to the commit.

#leftoverbits

Ciao,
Dscho