Greg Hudson <ghudson@MIT.EDU> writes:
> As I kind of mentioned before, I don't think this really works. Files
> which haven't changed also have a target version. Fundamentally, the
> target version is really a property of the commit or update, not of
> the change to files and directories. Treating it otherwise isn't a
> very good way to nuance over the problems you mentioned.
>
> (Unless you're saying we should replace_dir and replace_file on every
> single file and directory in the hierarchy, just to set the target
> version.)
No, we certainly shouldn't do that. Of course, there are ways to bump
the revisions of unmentioned foos without ever calling replace_foo(),
but your point is well taken.
However, the real issue is that the *driver* of the editor can't know
whose revision numbers should get bumped, because that knowledge is
derived from the arguments the user updated with. For example,
assuming only foo.c and bar.c have changed, then the difference
between
svn update .
and
svn update foo.c bar.c
is only that in the former case, unchanged entities will have their
revision numbers bumped, and in the latter, only foo.c and bar.c will
have their revision numbers bumped.
But the sequence of editor calls is exactly the same in both cases!
Or, equivalently, the XML tree delta that would describe the update
would be exactly the same in both cases. So clearly, the driver
doesn't know whose revisions to touch.
But the editor itself (i.e., the editor and its baton) *can* know
whose revisions to touch, because it can be aware of what the user did
to result in the edit. We need to add a `apr_hash_t *targets'
argument, or somesuch, to svn_wc_get_update_editor(), so that when the
editor is created by the client library, it is born with the knowledge
of the user's arguments and therefore can bump the revision numbers of
entities never mentioned by the actual sequence of editor calls.
(That it's not there right now is just an oversight.)
I think your objection still holds, though, because passing the
target_revision as a parameter is a bad match for the behavior
described above. It wrongly implies that setting revision numbers is
the province of individual edit calls, instead of of the overall edit.
[Branko, was I correct to repeat the preposition there? :-) ]
Let's review the reasons we chose passing target_revision, instead of
having a set_target_revision() editor function or something similar:
1. If a special editor function is used to set the target revision,
we fall into "how/when/how-often can that be called?"
2. Parameterizing supposedly sets us up for the day when we're
updating from different repositories, and thus could have varying
target_revisions.
I think #2 is frankly premature. There are lots of things we'll have
to do for multiple repositories; I doubt planning this far ahead
really buys us anything. Maybe a fresh editor will have to be
constructed for each repository anyway, for example.
Meanwhile, the question raised by #1 is a good one, but it's not
fundamentally any different than asking, in regards to a parameter,
"When should this argument be passed as SVN_INVALID_REVNUM?" Whenever
you're running an editor in which every target_revision parameter
would be passed as the empty value, well, that corresponds to simply
not calling set_target_revision(). (And editor runs in which some
target_revisions are empty and others are filled are not a
circumstance we ever have, so it's weird that we would support it).
Another way to look at this is: we all pretty much agreed that the
"ideal" solution would be to pass the revision number as an argument
to editor creation. The only reason we don't meet that ideal is an
implementation-specific gotcha about when the network layer gets the
revision number. If we *could* do it, though, we would.
Given that, the closest thing to the unattainable ideal, assuming we
want to avoid a factory function, would be this:
/* Set the target revision for all adds and replaces below the
* directory indicated by DIR_BATON, except those shadowed by a
* subsequent call to this function.
*
* If this is not called, or is called with SVN_INVALID_REVNUM,
* then the editor may choose whatever revision it thinks
* appropriate, if one is needed.
*
* Editors that do not allow the driver to set the target_revision,
* such as commit editors, are free to ignore the call (that is,
* implement it such that it has no effect).
*/
svn_error_t *set_target_revision (void *dir_baton,
svn_revnum_t target_revision);
And we're free to implement this later, should the need arise:
/* Set the target revision for the file indicated by FILE_BATON,
* except as superseded by a subsequent call to this function.
*
* If it is not called or is called with SVN_INVALID_REVNUM, and no
* revision has been specified with set_dir_target_revision(), then
* the editor may choose whatever revision it thinks appropriate,
* if one is needed.
*
* Editors that do not allow the driver to set the target_revision,
* such as commit editors, are free to ignore the call (that is,
* implement it such that it has no effect).
*/
svn_error_t *set_file_target_revision (void *file_baton,
svn_revnum_t target_revision);
IMHO, Greg H's point is too strong to ignore, and I think we should
reverse the decision and go with the first of the above two, leaving
the other as an option for later.
(Another possibility is that the whole editor thang is not quite the
right match for the problem and that an entirely new thang is needed.
I can't think of anything that suits the problem better though. If
anyone can, please post! :-) ).
-Karl
Received on Sat Oct 21 14:36:19 2006