[MERGE] Fix #175520 by implementing a --deep option for log

[Vincent Ladeuil]

>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at internode.on.net> writes:

    Ian> Vincent Ladeuil wrote:
    >>>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at internode.on.net> writes:

    >> So, in that patch, can we agree on handling only two cases as
    >> done here ?:
    >> - file -> file id against the working tree by default
    >> - file -> any file id if --old is used
    >> 
    >> Otherwise we need yet another mean to access even older file ids.

    Ian> I can agree to that. Part of my point though was that
    Ian> your patch isn't finding old file-ids and using them:
    Ian> it's matching on names (which is great) but then doesn't
    Ian> use the file-ids found.

    Ian> Practically, it probably doesn't matter in 99% of cases
    Ian> *but* bzr is unique partly because we (claim to) handle
    Ian> renames correctly, so that implies we ought to use the
    Ian> file-id once we know it IMO.

File ids used in the working tree can be found via dirstate. No
longer used file ids have to be searched in the
inventories. Since we have to search all inventories, there is no
place to reuse the file id once it's discovered.

    >> I should have explained my motivations better though (but I'd
    >> really like if Robert could elaborate on why he thinks it's a
    >> bug?): I view the actual display of the merging revisions as a
    >> bug or a misfeature or may be just one more missing option due
    >> to:
    >> 
    >> - our actual dotted revno numbering scheme,
    >> - the fact that we don't display revisions *removing* a file.

    Ian> The latter is a bug right?

Yes.

    >> I'm almost sure that the main reason they are displayed here is
    >> to identify the mainline merging revision *number* to reuse that
    >> number for either another bzr log or a bzr annotate. If we had a
    >> dotted revno numbering where merged revisions start with mainline
    >> merging revno instead... they won't be needed anymore.
    >> 
    >> As in:
    >> 
    >> - do 'log -v FILE' and get just the revisions modifying the file
    >> (as a rough sieve),
    >> 
    >> - from there do 'log -c mainline.revno' from the interesting
    >> dotted revno[s] obtained above

    Ian> I suspect it's more than the merging revision number that's
    Ian> interesting to users, e.g. the message and committer are useful.

Of course, I was referring to the fact that if you show the log
for a single dotted revno, you have no way to find the mainline
revision is it merged into. That's why bzr log FILE shows those
revisions. And it address a very common need: most of the time I
don't really care what precise revision introduced a change, I'm
more interested in knowing when that change landed, so I want the
mainline revision. But very often it's easier to find the dotted
revision because I searched via the -m flag because I know some
keyword that is mentioned there.

    Ian> Using multiple log commands is certainly a workaround.

I don't think so. At least, I rarely find the revision I'm
searching for with the first try...

When everything else fails I do bzr log >whole.log and search in
the file but that's hardly count as using a single log command...


<snip/>
 
    Ian> One of the good things about writing the help first is that
    Ian> it can assist in teasing out the right UI. I call it HDD. :-)

:-)

    Ian> Can't you just put the two if blocks after each other
    Ian> with exactly the same effect?  In fact, it really feels
    Ian> like the checking of path_filter needs to go higher in
    Ian> the code, as a parameter to either
    Ian> calculate_view_revisions() or make_log_rev_iterator().
    >> 
    >> Unfortunately, no.
    >> 
    >> To elaborate: I first tried to do it at a higher level (in
    >> calculate_view_revisions like _filter_revisions_touching_file_id)
    >> but the consequence is that the revisions are then selected
    >> *before* any display occurs.
    >> 
    >> This is a key design choice for file_id (which quickly identified
    >> the revisions) but --old means *all* revisions should be searched
    >> and that's slow as hell (It can't take minutes or even hours with
    >> our actual formats). So doing the filtering here, means some
    >> display occurs early and at least the user can start reading
    >> while we search the rest.

    Ian> Sure, but enhancing make_log_rev_iterator() is the other higher
    Ian> location in the code and it shouldn't suffer from the same
    Ian> problem?

make_log_rev_iterator just process the various adapters, at best
you mean one of the adapters and that may be a way but see other
related points in '[RFC] About log [-v] [--deep] [file|dir]*'.

    >> >> +    def test_log_old_file_deep(self):
    >> >> +        log = self.run_bzr(['log', '-v', '--short', '--deep', 'f1'])[0]
    >> >> +        # revnos 1 and 2 appear and mention adding and deleting f1
    >> >> +        self.assertContainsRe(log, '(?sm)^ +2 joe.*D  f1\n.*^ +1 joe.*A  f1\n')
    >> >> +        self.assertNotContainsRe(log, '(?sm)^ +3 joe')
    >> 
    Ian> Hmmm, this is testing exactly the same parameters as
    Ian> "test_log_file_deep_short".
    >> 
    >> Same parameters but a different file (f1 instead of f2) with a
    >> different history.

    Ian> I got that. :-) At a minimum, I'd put the two tests next to each
    Ian> other

ok.

    Ian> (or combine them even).

Urgh :-) Defect localization ftw !

     Vincent