Merge lp:~bialix/bzr/commit-rev-id into lp:bzr

[Alexander Belchenko]

[Alexander Belchenko (bialix)]

This patch fixes suspicious code: self.rev_id set to None and later set to valid value. But before get valif value value of self.rev_id (which is None) used to report data. There is rev_id local variable, it seems it should be used instead in report.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Alexander Belchenko wrote:
> Alexander Belchenko has proposed merging lp:~bialix/bzr/commit-rev-id into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This patch fixes suspicious code: self.rev_id set to None and later set to valid value. But before get valif value value of self.rev_id (which is None) used to report data. There is rev_id local variable, it seems it should be used instead in report.
>

Would this be a better fix?

=== modified file 'bzrlib/commit.py'
- --- bzrlib/commit.py 2009-10-15 02:53:30 +0000
+++ bzrlib/commit.py 2009-11-16 17:19:54 +0000
@@ -289,7 +289,7 @@
         self.local = local
         self.master_branch = None
         self.recursive = recursive
- - self.rev_id = None
+ self.rev_id = rev_id
         # self.specific_files is None to indicate no filter, or any
iterable to
         # indicate a filter - [] means no files at all, as per
iter_changes.
         if specific_files is not None:

I haven't tracked down where 'rev_id' is used yet, but if you are
passing in a rev_id, then I would expect it to be set...

 review: needs_information

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAksBifEACgkQJdeBCYSNAANU1wCeL6r4jGLfDi/gH1WWx1qvKVpc
tTYAoJhU9JgrBZl0+QFW3wSsXp4RilY4
=NGwz
-----END PGP SIGNATURE-----

review: Needs Information

[Alexander Belchenko (bialix)]

I can't say is it better fix. If we count lines -- then yes, your is shorter therefore better.

rev_id itself is mandatory argument of _commit function:

    def _commit(self, operation, message, timestamp, timezone, committer,
            specific_files, rev_id, allow_pointless, strict, verbose, revprops,
            working_tree, local, reporter, config, message_callback, recursive,
            exclude, possible_master_transports):

Value of rev_id is passed then to create builder:

        self.builder = self.branch.get_commit_builder(self.parents,
            self.config, timestamp, timezone, committer, revprops, rev_id)

And when commit is done builder returns real rev_id which is assigned then to self.rev_id:

            self.rev_id = self.builder.commit(self.message)

With your proposed change I'd suggest to using self.rev_id instead of rev_id in the get_commit_builder call, i.e. the best fix may be then as following:

=== modified file 'bzrlib/commit.py'
--- bzrlib/commit.py 2009-10-15 02:53:30 +0000
+++ bzrlib/commit.py 2009-11-16 18:08:29 +0000
@@ -289,7 +289,7 @@
         self.local = local
         self.master_branch = None
         self.recursive = recursive
- self.rev_id = None
+ self.rev_id = rev_id
         # self.specific_files is None to indicate no filter, or any iterable to
         # indicate a filter - [] means no files at all, as per iter_changes.
         if specific_files is not None:
@@ -371,7 +371,7 @@
         # Collect the changes
         self._set_progress_stage("Collecting changes", counter=True)
         self.builder = self.branch.get_commit_builder(self.parents,
- self.config, timestamp, timezone, committer, revprops, rev_id)
+ self.config, timestamp, timezone, committer, revprops, self.rev_id)

         try:
             self.builder.will_record_deletes()

I'm just not sure about intent of using rev_id and self.rev_id.

[Alexander Belchenko (bialix)]

I mean: I'm not sure about intent of separate use of rev_id and self.rev_id.

In Erlang the value can be assigned only once. So from this POV my first patch is better.

[Robert Collins (lifeless)]

self.rev_id is what should be used.

[Martin Pool (mbp)]

I think some of our use of methodobjects is questionable, as is putting variables onto objects when they're really not naturally object state but something more like local variables.

However, given that this code is already done this way, John's patch of 2009-11-17 is better.

If it's possible to remove self.rev_id altogether that could be good, though that would constitute an API break.

review: Needs Fixing

[Alexander Belchenko (bialix)]

Please, reject it.

review: Disapprove

Unmerged revisions

4798. By Alexander Belchenko on 2009-11-16

fixed incorrect usage of self.rev_id

=== modified file 'bzrlib/commit.py'
--- bzrlib/commit.py	2009-10-15 02:53:30 +0000
+++ bzrlib/commit.py	2009-11-16 13:45:28 +0000
@@ -289,7 +289,6 @@
         self.local = local
         self.master_branch = None
         self.recursive = recursive
-        self.rev_id = None
         # self.specific_files is None to indicate no filter, or any iterable to
         # indicate a filter - [] means no files at all, as per iter_changes.
         if specific_files is not None:
@@ -382,7 +381,7 @@
                 master_location = self.branch.base
 
             # report the start of the commit
-            self.reporter.started(new_revno, self.rev_id, master_location)
+            self.reporter.started(new_revno, rev_id, master_location)
 
             self._update_builder_with_changes()
             self._check_pointless()