"tag from <refname>" gives KeyError

[Samuel Bronson]

This code in processors/generic_processor.py appears to assume that the "from" revision for a "tag" command will always be expressed as a "mark", like :1. This is not the case; for example see the following slightly altered (to avoid bug #400960) and severely abbreviated output from a slightly altered version of git's contrib/fast-import/import-tars.perl (also provided as an attachment):

commit refs/heads/import-tars
author Matthew Luckie <email address hidden> 1087095178 +0000
committer Samuel Bronson <email address hidden> 1247888914 +0000
data 43
Imported from scamper-cvs-20040613.tar.gz.

deleteall

tag scamper-cvs-20040613
from refs/heads/import-tars
tagger Matthew Luckie <email address hidden> 1087095178 +0000
data 29
Package scamper-cvs-20040613

This is documented in the git-fast-import(1) manpage:

           Here <committish> is any of the following:

           · The name of an existing branch already in fast-import’s
               internal branch table. If fast-import doesn’t know the name,
               its treated as a SHA-1 expression.

           · A mark reference, :<idnum>, where <idnum> is the mark number.

               The reason fast-import uses : to denote a mark reference is
               this character is not legal in a Git branch name. The leading :
               makes it easy to distinguish between the mark 42 (:42) and the
               branch 42 (42 or refs/heads/42), or an abbreviated SHA-1 which
               happened to consist only of base-10 digits.

[two other, fairly git-specific forms are also documented]

[Samuel Bronson (naesten)]

Okay, I've written a test on branch lp:~naesten/bzr-fastimport/401249-tag-from-ref.

As for a fix, I'm looking at the code, and it looks to me as though there ought to be a method on cache_manager.CacheManager to look up a committish -- as it stands, the logic seems to be tied up in the head tracking code, which is *probably* not something we want to call from GenericProcessor._set_tag(), though I'm not positive about that.

[Samuel Bronson (naesten)]

It turns out that this issue also affects commits, meaning that the logic is not tangled up in the head tracking code -- it's just *not there*.

Pushed a change to the test to lp:~naesten/bzr-fastimport/401249-tag-from-ref to demonstrate this; I should probably rename the test, but I'm not sure to what ...

[Ian Clatworthy (ian-clatworthy)]

At a minimum, we need to trap the KeyError. For tags, I'd be ok with reporting a warning, skipping that tag and continuing. For other uses of from (e.g. merge commits), we need to make the call as to whether that's acceptable of not.

[Samuel Bronson (naesten)]

This bug is about failure to implement the following form of committish:

           · The name of an existing branch already in fast-import’s
               internal branch table. If fast-import doesn’t know the name,
               its treated as a SHA-1 expression.

If you want a bug about failure to trap the KeyError, you can make your own bug ;-).

[Jelmer Vernooij (jelmer)]

I've merged the test case (and made it raise KnownFailure), but the bug is not fixed yet