[MERGE] Remove the basis_tree parameter to record_iter_changes.

[Robert Collins]

On Fri, 2009-03-20 at 09:12 -0500, John Arbash Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Robert Collins wrote:
> ...
> 
> >>
> >> ^- another comment here for:
> >> # change: (old-path, new-path, file_id, new_ie)
> > 
> > Actually, this is a terrible smell. Whats the point of docstrings on
> > make_delta etc if we are reinventing them with comments everywhere.
> 
> We chose to go with tuples for performance, but they are awful for
> maintaining code. It is a real pain to have to go look at the doc string
> of iter_changes() and _make_delta() every time I'm trying to verify
> "what is foo[4] again?"
> 
> I agree there is the the potential for skew, but if we get skew in a
> tuple-based api, things are rather difficult anyway. I don't really
> *like* putting the comments in, I just like it more than having to keep
> extra windows open so I can remind myself what the things I'm looking at
> really are.

Compromise then: I'll put such comments in whenever I open a window to
look one up?



> >> +                    if (carry_over_possible and
> >> +                        parent_entry.executable == entry.executable):
> >> +                            # Check the file length, content hash after
> >> reading
> >> +                            # the file.
> >> +                            nostore_sha = parent_entry.text_sha1
> >> +                    else:
> >> +                        nostore_sha = None
> >>
> >> ^- you take the time to skip "nostore_sha" in the case that
> >> entry.executable changed, but why not also set it in the case that the
> >> entry.size has changed?
> > 
> > If the size has changed the sha has to have too.
> 
> I agree that is true for correctness, but you wouldn't *have* to sha if
> the size changed. (I guess ultimately you have to do it at least 1 time,
> since the inventory wants to record the current value.)

nostore_sha1 says to the versioned file 'if the sha matches this, do not
add the object to the versioned file'. If we know we want to add the
object, it must be None, otherwise we will not add the object. The case
when we don't know that we want to add the object are when there are no
metadata changes and the file looks identical as far as we have checked.
At this point in the code path we haven't done a sha1 check (and
dirstate will avoid sha calculating when it doesn't get a cache hit) so
thats why we set nostore_sha. Setting it when the file size has changed
would be a bug.

> > The race is that we may record a change that didn't happen. iter_changes
> > detects that NEWS has altered. someone concurrently does a manual
> > 'revert NEWS' (e.g. saves the file in vim).
> > 
> 
> Wouldn't that hit the "nostore_sha" path? I guess not if you set the
> executable bit? But it looks like entry.executable is fixed at this
> point? (It should probably be fixed from the stat of file_obj that you
> get back if possible.)

Like I said, I wasn't worried about the executable bit so much. But yes
there may be a race there as well.

It won't hit the nostore_sha path if there were other metadata changes involved.

-Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090323/bfa2eab5/attachment.pgp