Merge lp:~amanica/bzr/log_returns_too_much into lp:~bzr/bzr/trunk-old
Status: | Superseded | ||||
---|---|---|---|---|---|
Proposed branch: | lp:~amanica/bzr/log_returns_too_much | ||||
Merge into: | lp:~bzr/bzr/trunk-old | ||||
Diff against target: | 90 lines | ||||
To merge this branch: | bzr merge lp:~amanica/bzr/log_returns_too_much | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vincent Ladeuil | 2009-07-07 | Approve on 2009-07-09 | |
Ian Clatworthy | 2009-07-09 | Pending | |
Review via email:
|
This proposal has been superseded by a proposal from 2009-07-10.
Marius Kruger (amanica) wrote : | # |
Vincent Ladeuil (vila) wrote : | # |
>>>>> "amanica" == Marius Kruger <email address hidden> writes:
amanica> Marius Kruger has proposed merging
amanica> lp:~amanica/bzr/log_returns_too_much into lp:bzr.
Thanks a lot for working on that !
amanica> Requested reviews:
amanica> bzr-core (bzr-core)
amanica> Fixed this by adding a stop condition to
amanica> _DefaultLogGene
amanica> It can't be done at a lower level because some of
amanica> the other filters depend on having all the
amanica> log-revisions' parents.
amanica> This causes a behaviour change as explained in the bug:
amanica> doing a `bzr log -n0 -r 2345` would no longer return its parents eg 2338.3.1
Hurrah !
This has bugged me for a long time. I'm pretty sure it's related
(but not only) to 3936.3.30, could you have a look and share your
thoughts ?
<snip/>
amanica> === modified file 'bzrlib/log.py'
amanica> --- bzrlib/log.py 2009-06-10 03:56:49 +0000
amanica> +++ bzrlib/log.py 2009-07-07 20:22:22 +0000
amanica> @@ -400,6 +400,14 @@
amanica> log_count += 1
amanica> if log_count >= limit:
amanica> return
amanica> + # we need to filter the revisions here again since
amanica> + # the lower levels also return the revision parents
amanica> + if rqst.get(
amanica> + if self.end_rev_id == rev_id:
amanica> + return
amanica> + else:
amanica> + if self.start_rev_id == rev_id:
amanica> + return
Can you add the bug number in that comment as a reminder ? Your
rationale sounds good, we may want to revisit that part in the
future (Ian intended to push his design further but delayed it
post-2.0).
amanica> === modified file 'bzrlib/
amanica> --- bzrlib/
amanica> +++ bzrlib/
amanica> @@ -277,7 +277,7 @@
amanica> # tags don't propagate if we don't merge
amanica> self.run_bzr('merge ../branch1', working_
amanica> branch2_
amanica> - log = self.run_bzr("log -n0 -r-1", working_
amanica> + log = self.run_bzr("log -n0 -r-2..-1", working_
amanica> self.assertCont
amanica> log = self.run_bzr("log -n0 -r3.1.1", working_
amanica> self.assertCont
That sounds wrong, can you explain the link with your
modification ? Is that because before your fix an additional
revision was output ?
If that's true then your fix is *really* needed and demonstrates
that 'log' was definitely not correct. If it's not, it be good to
understand what happened here.
I feel convinced by your rationale and the test modifications,
except for the one noted above for which I'd like an explanation,
otherwise...
Vincent Ladeuil (vila) wrote : | # |
vila> reviewer ian-clatworthy
vila> review approve
Hmpf,
must start the command line with a space
must start the command line with a space
must start the command line with a space
must start the command line with a space
must start the command line with a space
must start the command line with a space
must start the command line with a space
must start the command line with a space
must start the command line with a space
must start the command line with a space
reviewer ian-clatworthy
review approve
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Vincent Ladeuil wrote:
>>>>>> "amanica" == Marius Kruger <email address hidden> writes:
>
>
> amanica> Marius Kruger has proposed merging
> amanica> lp:~amanica/bzr/log_returns_too_much into lp:bzr.
>
> Thanks a lot for working on that !
>
> amanica> Requested reviews:
> amanica> bzr-core (bzr-core)
>
> amanica> Fixed this by adding a stop condition to
> amanica> _DefaultLogGene
>
> amanica> It can't be done at a lower level because some of
> amanica> the other filters depend on having all the
> amanica> log-revisions' parents.
>
> amanica> This causes a behaviour change as explained in the bug:
> amanica> doing a `bzr log -n0 -r 2345` would no longer return its parents eg 2338.3.1
>
> Hurrah !
>
> This has bugged me for a long time. I'm pretty sure it's related
> (but not only) to 3936.3.30, could you have a look and share your
> thoughts ?
>
So here are my thoughts...
If we are going to do this then:
1) "bzr log -r 10" should show just the revision 10
2) "bzr log -c 10" should show all the changes that were merged into 10
3) "bzr log -r 9..11" should *not* show the revision for 9, but show
everything that was merged into 10 and 11.
Basically, we would unify it with similar commands (like diff) which for
"-r X..Y" mean "show what is in Y that is not in X".
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkp
kJcAnj5aW/
=8wKN
-----END PGP SIGNATURE-----
Marius Kruger (amanica) wrote : | # |
2009/7/9 Vincent Ladeuil <email address hidden>
>
> >>>>> "amanica" == Marius Kruger <email address hidden> writes:
>
> Thanks a lot for working on that !
cool, np
> amanica> Requested reviews:
> amanica> bzr-core (bzr-core)
>
> amanica> Fixed this by adding a stop condition to
> amanica> _DefaultLogGene
>
> amanica> It can't be done at a lower level because some of
> amanica> the other filters depend on having all the
> amanica> log-revisions' parents.
>
> amanica> This causes a behaviour change as explained in the bug:
> amanica> doing a `bzr log -n0 -r 2345` would no longer return its parents eg 2338.3.1
>
> Hurrah !
>
> This has bugged me for a long time. I'm pretty sure it's related
> (but not only) to 3936.3.30, could you have a look and share your
> thoughts ?
uhm, I don't know all the history but it could be that the code path
change in this
revision changed what you saw log returning.
So the reason why we need to do it at this level is that
in order to filter on the files we need all the merges,
but on a higher level we can choose to hide those merge again
after we do that filtering.
(I updated the comment now to explain this better)
(bzr diff -c 3936.3.30 --diff-
def _log_revision_
# Get the base revisions, filtering by the revision range.
# Note that we always generate the merge revisions because
# filter_
calls vvv
def _generate_
delayed_
# On large trees, generating the merge graph can take 30-60 seconds
# so we delay doing it until a merge is detected, incrementally
# returning initial (non-merge) revisions while we can.
calls vvv
def _graph_
rebase_
"""Calculate revisions to view including merges, newest to oldest.
calls vvv
def iter_merge_
"""Walk the revisions for a branch in merge sorted order.
> amanica> === modified file 'bzrlib/log.py'
> amanica> --- bzrlib/log.py 2009-06-10 03:56:49 +0000
> amanica> +++ bzrlib/log.py 2009-07-07 20:22:22 +0000
> amanica> @@ -400,6 +400,14 @@
> amanica> log_count += 1
> amanica> if log_count >= limit:
> amanica> return
> amanica> + # we need to filter the revisions here again since
> amanica> + # the lower levels also return the revision parents
> amanica> + if rqst.get(
> amanica> + if self.end_rev_id == rev_id:
> amanica> + return
> amanica> + else:
> amanica> + if self.start_rev_id == rev_id:
> amanica> + return
>
> Can you add the bug number in that comment as a reminder ?
done
>
> amanica> === m...
- 4517. By Marius Kruger on 2009-07-10
-
update comment as per review
Marius Kruger (amanica) wrote : | # |
2009/7/9 John A Meinel <email address hidden>
>
> So here are my thoughts...
thanks
> If we are going to do this then:
>
> 1) "bzr log -r 10" should show just the revision 10
that is what this patch does
> 2) "bzr log -c 10" should show all the changes that were merged into 10
sounds like a must have feature otherwise its really difficult to
figure out what happened
(I resorted to bzr explorer & qlog to be able to successfully navigate this)
bzr diff seems to work that way too.
> 3) "bzr log -r 9..11" should *not* show the revision for 9, but show
> everything that was merged into 10 and 11.
>
> Basically, we would unify it with similar commands (like diff) which for
> "-r X..Y" mean "show what is in Y that is not in X".
That is https:/
which was more or less my next step.
I like your suggestions, it would make log&diff more consistent.
I'm keen to implement them as soon as time permits.
Robert Collins (lifeless) wrote : | # |
On Thu, 2009-07-09 at 14:06 +0000, John A Meinel wrote:
> So here are my thoughts...
>
> If we are going to do this then:
>
> 1) "bzr log -r 10" should show just the revision 10
> 2) "bzr log -c 10" should show all the changes that were merged into 10
> 3) "bzr log -r 9..11" should *not* show the revision for 9, but show
> everything that was merged into 10 and 11.
>
> Basically, we would unify it with similar commands (like diff) which for
> "-r X..Y" mean "show what is in Y that is not in X".
+1
--
Martin Pool (mbp) wrote : | # |
2009/7/10 Robert Collins <email address hidden>:
> On Thu, 2009-07-09 at 14:06 +0000, John A Meinel wrote:
>
>> So here are my thoughts...
>>
>> If we are going to do this then:
>>
>> 1) "bzr log -r 10" should show just the revision 10
>> 2) "bzr log -c 10" should show all the changes that were merged into 10
>> 3) "bzr log -r 9..11" should *not* show the revision for 9, but show
>> everything that was merged into 10 and 11.
>>
>> Basically, we would unify it with similar commands (like diff) which for
>> "-r X..Y" mean "show what is in Y that is not in X".
>
> +1
+1
--
Martin <http://
- 4518. By Marius Kruger on 2009-07-19
-
merge with bzr.dev. no conflicts. put news in its place.
Unmerged revisions
- 4518. By Marius Kruger on 2009-07-19
-
merge with bzr.dev. no conflicts. put news in its place.
- 4517. By Marius Kruger on 2009-07-10
-
update comment as per review
- 4516. By Marius Kruger on 2009-07-07
-
add news
- 4515. By Marius Kruger on 2009-07-07
-
when we go forward we need to stop at the end-revision not the start-revision
- 4514. By Marius Kruger on 2009-07-07
-
use Logger.start_rev_id in stead of reading/checking is again
- 4513. By Marius Kruger on 2009-07-07
-
stop logging at the stop-revision, and start updating the tests for it
- 4512. By Marius Kruger on 2009-07-06
-
add test_merges_
partial_ range_ignore_ before_ lower_bound
Updating diff...
An updated diff will be available in a few minutes. Reload to see the changes.
Fixed this by adding a stop condition to rator.iter_ log_revisions( )
_DefaultLogGene
It can't be done at a lower level because some of the other filters depend on having all the log-revisions' parents.
This causes a behaviour change as explained in the bug:
doing a `bzr log -n0 -r 2345` would no longer return its parents eg 2338.3.1