[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of --depth: should define WC-depth for omitted-items?)

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Mon, 28 Apr 2008 17:57:52 -0400

"Rui, Guo" <timmyguo_at_mail.ustc.edu.cn> writes:
> On Fri, Apr 25, 2008 at 03:46:31PM -0700, David Glasser wrote:
>> 2008/4/25 Rui, Guo <timmyguo_at_mail.ustc.edu.cn>:
>> > I attach the patch in this mail to fix this in-consistency. This
>> > is my first code patch and takes me some time to go through the
>> > code.
>> > Function svn_wc_add2() is the one that is responsible to actually
>> > add items to wc and schedule an add at commit time. It does not
>> > take a depth parameter and is not appropriate to be tuned to
>> > accept one, since it handles both directory and file items. The
>> > function always use an infinity depth when adding new directory,
>> > since there is no way to get the proper depth value. (And there
>> > is a comment dated back to r26607 -- by you, Karl -- declaring
>> > that this is a proper behavior.) So, the proper depth should be
>> > adjusted by the caller of svn_wc_add2() when needed.
>>
>> Hmm. I'm not sure I follow here. I agree that svn_wc_add2 doesn't
>> provide a way to specify the right depth, but I think it would be
>> perfectly reasonable to just add a depth argument to it, which is
>> ignored for file arguments.
>>
>> I'm not sure that the "write something incorrect to disk, then write
>> the correct thing" is better than just "introduce arguments that let
>> you write the right thing in the first place".
>
> Well, it's a hard decision here and I'm not sure that I made the right
> decision. The standpoint is that svn_wc_add2() is used in many
> situations: add, merge and copy etc. The depth only makes sense in a
> small piece of situations. That is, adding a directory (not files,
> which should be much more than dir) in 'svn add' command. If such a
> parameter is introduced, most of the time it only add the burden to
> the caller (to fill in a nonsense) and looks ugly.

I think I made my earlier decision about svn_wc_add2() without all the
information we have now. If it just took a 'depth' argument, we could
ignore it for files (or we could require that files be called with
'svn_depth_unknown' and check for it, to sanity-check the caller).

Possibly other uses (outside of 'svn add') might also find the 'depth'
argument useful. In any case, once we admit that addition of a
directory is an operation involving depth questions, we might as well
reflect that in the API. If many callers have to pass
svn_depth_infinity, at least that's readable.

> An alternative should be just introduce a specialized svn_wc_add_dir()
> only for 'svn add' and don't make it deprecate svn_wc_add2() (as we
> generally do when we introduce something like svn_wc_add3()). Does
> this seem to be a better choice? In this way, the default_and_fix way
> will still have to be used, to reuse the implementation in
> svn_wc_add2(). However, this dirty trick will not expose to the
> client.

Well, we'd want the depth as we're crawling down and doing the add, so
we can avoid doing more work than necessary.

Basically, I think just adding depth to the regular add API is okay.

>> > Currently, the following two behavior is maintained: 1) existing
>> > dir is never touched for 'svn add --force --depth=xxx
>> > existing_dir'; 2) intermediate parents is set to svn_depth_empty
>> > for 'svn add --parents target_deep_in_subtree'.
>>
>> Good insight; I wouldn't have thought about the --parents case! I
>> haven't completely thought things through to be sure if the
>> parents-should-be-empty choice is correct, but the fact that it's
>> worth thinking about is a great catch.
>
> Well, this is a hard decision too, just as the decision to patch 'svn
> add' itself. :) I think it makes sense to add the parent as empty. If
> the user cares about something directly under the parent directory,
> why not add the entire parent tree in the first place?

Either empty or infinity.

Actually, I think infinity may be better. Imagine that you

   svn add --depth=files --parents NEW_DIR_1/NEW_DIR_2/FINAL_DIR
   svn commit -m "...blah blah blah..."

There might be a lot of subdirectories under FINAL_DIR, but they won't
be added, only FINAL_DIR itself and its file children. Great.

But later, after you do that, someone else adds NEW_DIR_1/FISH/ to the
repository. What should happen if you run 'svn up' now?

I think you should receive NEW_DIR_1/FISH. The alternative is that you
don't find out that someone has been adding things in the directory you
created -- which would be fine if you'd *requested* that directory with
a limiting depth, but you didn't. The '--depth=files' in the original
'svn add' really referred to the final target, not to the intermediate
stuff -- the intermediate dirs are just things you had to add in order
to position FINAL_DIR correctly. If stuff appears in those intermediate
directories later, you'd probably like to know. Not definitely, but
probably.

>> > I think it would be handy to have an notification when there are
>> > items newly added (since last update) but just excluded by the
>> > depth setting of current wc. This should include a new file/dir
>> > immediately under a existing directory that is set to empty/files,
>> > but should not include a new item deeply under a totally excluded
>> > subtree. What do you think about it?
>>
>> Hmm. In a sense that completely defeats the point of
>> depth-in-the-repos-reporter (on the API level), which is "tell the
>> repository to give me a very finely-tuned directory delta". It does
>> seem like it might be a useful optional feature for users, though.
>
> As you have mentioned previously in this thread. Whatever we choose to
> do in the code, the user may complain in a oppose direction. Why not
> give them a notice if we choose to preclude the update? This may
> relieve their anger. :-)

I just think we have more important things to do right now. This might
be a nice feature, but it is unimportant compared to, say, a depth
deselection interface :-).

> This feature will need to tune the API. However, since the
> notification only happens at the boundary that is defined by the
> ambient depth of wc (in other words, we don't need to dig into
> excluded sub-trees), it's possible to make it available with only
> implement level modification. The "tell the repository to give me a
> very finely-tuned directory delta" semantics should still be
> maintained.

I'm not sure exactly what you meant. Could you give an example?

Thanks,
-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-28 23:58:20 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.