[MERGE][#175520] use historical context for file logging

[Ian Clatworthy]

Vincent Ladeuil wrote:
> Some comments on the discussions before the review itself.
> 
> The outcome is a BB:tweak but ask for some tests rewriting.

Thanks on both counts. Apologies for the delayed response.

>>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at internode.on.net> writes:
> 
>     Ian> Adrian Wilkins wrote:
>     >>> If we start supporting file-ids in the UI, I'd like to do so
>     >>> across every command and allow you to specify "fileid:..."
>     >>> anywhere you can currently specify a file.
> 
> I disagree, I can't think of another command that needs file-id
> like log does. Care to elaborate ?

Sure. Right now, we use a 'revid:' prefix to give the internal
revision-id instead of the "normal" revno. For consistency, I think
we ought to treat file-handles the same, generally available way.

Right now, log might be the driver behind wanting to specify a file-id.
It's not hard to imagine the need spreading though to commands like
missing, annotate, status -c and diff -c.

>     Ian> FWIW, if this patch and my faster-log patch land, log
>     Ian> will begin showing revisions that deleted a file
>     Ian> *provided* one specifies a lower revision limit of some
>     Ian> kind and excludes the merge revisions.
> 
> Gee, if I'm supposed to remember those constraints before firing
> the command... I'm sure I will fail each time...

My prediction above was crap. In bzr.dev, I think -v will show
removals while it doesn't without it.

>     Ian> So the UI question becomes "What was that file called and
>     Ian> when did it get created/renamed/deleted?" My proposed answer
>     Ian> is that is adding an option to log that let's you filter on
>     Ian> status flags something like ...
> 
>     Ian>   bzr log --status-flags NRD
> 
> Meh. *I* don't care about filtering that, I care about the file
> life cycle and history (that's why I'm using log), but filtering
> *inside* that file history... that's a bit overkill for me.

Right. And I wasn't suggesting using --status-flags with a single
file argument. I saw it being more useful for telling me about all
the removals in a project or particular directory, say.

>>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at internode.on.net> writes:
> 
>     Ian> Ian Clatworthy wrote:
>     >> This patch fixes log so that the end revision is used
>     >> for mapping a FILE to a file-id. This means log can
>     >> now find deleted files. It also means renamed files
>     >> are matched against their name at the time.
> 
>     Ian> This updated version searches the start revision if the
>     Ian> file doesn't exist in the end revision.
> 
> Right, I guess that's the best we can do *without* a file-id
> option... but I really think you should update the log help with
> a complete explanation there...(Did *you* say HDD ? :-)

I did. :-)
I've put an "expanded log help" patch up for review that is now
semi-blocked on adding "help -v". That expanded help contains the
required explanations about this feature and its impact. I hope to
land it by 1.12final, if not 1.12rc.

>     Ian> +def _get_fileid_to_log(revision, tree, b, fp):
> 
> Doc string please.
> 
> And how about putting that helper in the log module ? No need to
> make builtins.py bigger than it currently is.

Done and done.

>     Ian> +    if revision is None:
>     Ian> +        if tree is None:
>     Ian> +            tree = b.basis_tree()
>     Ian> +        file_id = tree.path2id(fp)
>     Ian> +        if file_id is None:
>     Ian> +            # go back to when time began
>     Ian> +            rev1 = b.get_rev_id(1)
>     Ian> +            tree = b.repository.revision_tree(rev1)
>     Ian> +            file_id = tree.path2id(fp)
> 
> Wow, that's really desperate :-) 

Desperate but semantically consistent. :-)

> I frankly doubt it will often succeeds... but since failures to
> find the file id are trapped at the caller level, I'm fine with
> that.
> 
> As long as you add some direct tests for it (one more reason to
> put it in the log module).

I haven't done this bit yet. It's currently a private (to bzrlib) API
and isn't going to become a public one. In fact, it gets reworked a lot
in my multiple file/directory logging patch. I'll most likely add some
low level tests for the revised version in that branch. For now, *I*
feel the higher level tests adequately cover the functionality.

>     Ian> +    def test_log_file_historical(self):
>     Ian> +        """File matched against revision range, not current tree."""
>     Ian> +        self.prepare_tree(complex=True)
>     Ian> +
>     Ian> +        # Check logging a deleted file gives an error if the
>     Ian> +        # file isn't found at the end or start of the revision range
>     Ian> +        err_msg = "Path unknown at end or start of revision range: file2"
>     Ian> +        err = self.run_bzr('log file2', retcode=3)[1]
>     Ian> +        self.assertContainsRe(err, err_msg)
>     Ian> +
>     Ian> +        # Check logging a deleted file is ok if the file existed
>     Ian> +        # at the end the revision range
>     Ian> +        log, err = self.run_bzr('log -r..4 file2')
>     Ian> +        self.assertEquals('', err)
>     Ian> +        self.assertNotContainsRe(log, 'revno: 1\n')
>     Ian> +        self.assertContainsRe(log, 'revno: 2\n')
>     Ian> +        self.assertNotContainsRe(log, 'revno: 3\n')
>     Ian> +        self.assertContainsRe(log, 'revno: 3.1.1\n')
>     Ian> +        self.assertContainsRe(log, 'revno: 4\n')
>     Ian> +
>     Ian> +        # Check logging a deleted file is ok if the file existed
>     Ian> +        # at the start of the revision range
>     Ian> +        log, err = self.run_bzr('log file1')
>     Ian> +        self.assertEquals('', err)
>     Ian> +        self.assertContainsRe(log, 'revno: 1\n')
>     Ian> +        self.assertNotContainsRe(log, 'revno: 2\n')
>     Ian> +        self.assertNotContainsRe(log, 'revno: 3\n')
>     Ian> +        self.assertNotContainsRe(log, 'revno: 3.1.1\n')
>     Ian> +        self.assertNotContainsRe(log, 'revno: 4\n')
> 
> I'd really really prefer if we can define some dumb log formatter
> that just records the revisions it displays so that the tests can
> then use some 'rev in list' conditions.

That would be nice.

> And this test is at least three different ones (you have the
> comment to give them a name), split it please.

I did this as requested. I'm not totally convinced of the value of
doing so because I *like* seeing the tests collected together like
this, but it's not a big deal to me either way.

Thanks again for the review and overall design feedback.

Ian C.