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

[Ian Clatworthy]

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.

I can agree to that. Part of my point though was that your patch
isn't finding old file-ids and using them: it's matching on names
(which is great) but then doesn't use the file-ids found.
Practically, it probably doesn't matter in 99% of cases *but* bzr
is unique partly because we (claim to) handle renames correctly,
so that implies we ought to use the file-id once we know it IMO.

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

The latter is a bug right?

> 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

I suspect it's more than the merging revision number that's
interesting to users, e.g. the message and committer are useful.
Using multiple log commands is certainly a workaround.

> I know the argument is pretty weak (even if I *used* it in real
> life because I couldn't find another way to find such scenarios),
> but on the other hand, I also feel that log is the main tool to
> explore a project history and I don't think we offer all the
> needed basic features and the ability to combine them.
> 
> 'missing' for example select revisions in a way that can't be
>  reproduced from log. Marius Kruger's attempt with
>  [MERGE][RFC][1.11] implement `missing --revision 3..4
>  --other-revision 3..4` should, IMHO, be doable by log (it isn't
>  right now).
> 
> So I think *that* patch is just scratching the surface of a
> deeper problem.

I'm yet to look at Marius' patch and the discussion around it.
I hope to soon.


>     robert> I don't like it because, it makes log -v FILE
>     robert> meaningless, every leaf revision will show just the
>     robert> files name.
> 
> And the associated action yes (added, modified, deleted). That's
> meaningful for me.
> 
> Note that the main motivation here is that log -v FILE on big to
> huge working trees is actually *useless*, the generated output
> also being impossible to grep unless we make it possible to use a
> different format (see the thread named '[RFC] About log [-v]
> [--deep] [file|dir]*').
> 
> Since it sounds that both points of view can't be reconciled, do
> we need one more option or should we just use verbosity_level ?
> In that case '-vv' will restore the previous behavior ?

I think using verbosity_level is sufficient.

>     Ian> I'd like to see the help for the log command expanded
>     Ian> quite a bit as part of this patch. I think it needs to
>     Ian> explain file-id-based vs file-path-based logging and
>     Ian> include examples of both.
> 
> Ok, I could use some help here though :)

One of the good things about writing the help first is that
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.

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


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

I got that. :-) At a minimum, I'd put the two tests next to each
other (or combine them even).

>     Ian> In summary, thanks for working on this - getting a
>     Ian> rich-featured, bug-free, high-performing log command
>     Ian> is quite a challenge for us given our UI desires.
>     Ian> I like most of the refactoring you did but feel this
>     Ian> patch isn't ready for merging just yet, at least not
>     Ian> without a bit more discussion re the UI and implementation
>     Ian> alternatives.
> 
> That's why I also started *two* related threads:
> 
> - [RFC] About log [-v] [--deep] [file|dir]*
> 
> - [RFC] How various commands display revisions
> 
> Don't hesitate to give feedback there too,

OK. I remember reading the latter but suspect I missed the first
one. I'll look it up soon.

Ian C.