thoughts about shelving

[Stefan Kueng <to...@gmail.com>]

Hi,

I know I'm a little bit late, but now I'm implementing the new shelf 
APIs in TSVN so it's now that I have some thoughts and suggestions:

* there are deprecated APIs already, even though they weren't there in 
1.10.0, for example svn_client_shelf_save_new_version - since they're 
all new in 1.10.1, why not just remove the deprecated ones and keep the 
current one?
* when users update to the new version, existing shelves can't be used 
anymore since they're not compatible. Is there a way to convert the old 
shelves to the new format? If not then that means users would lose those 
saved shelves.
* I generate an API doc with Doxygen. But some comments are not properly 
formatted for this: if a comment is after a variable, the comment must 
be formatted properly otherwise the Doxygen docs apply the comment to 
the wrong variable. For example, svn_client_shelf_version_t looks like 
the attached image. To make this work properly, the struct should look 
like this:

typedef struct svn_client_shelf_version_t
  {
    /** Public fields (read-only for public use) */
    svn_client_shelf_t *shelf;
    apr_time_t mtime;  /**< time-stamp of this version */

    /** Private fields */
    const char *files_dir_abspath;  /**< abspath of the storage area */
    int version_number;  /**< version number starting from 1 */
  } svn_client_shelf_version_t;

Note the '<' in the field comments.

that's it for now. But since I'm not finished yet implementing 
everything in TSVN I might have some more comments soon.

Stefan

Re: thoughts about shelving

[Julian Foad <ju...@apache.org>]

Julian Foad wrote:
> Greg Stein wrote:
>> When I envisioned the implementation of shelving, I figured that we would use the sql database to its best effect.

Let me be more concrete.

>>  Store new pristines (which is logically just key->fulltext).

Pristine file texts add up to a large fraction of the total disk storage space requirements, so for that reason this would be worth doing, but their management forms only a small subset of the complexity of storing tree changes.

>> Maybe create an artificial wc_id to construct an alternate tree of ACTUAL_NODE entries where you can store node information of the items on the shelf.

What's needed is a consistent API for storing and retrieving tree changes. Such an API is needed for getting changes into and out of a shelf, just the same as for getting changes into and out of the main WC storage. There are 3 parts to such an API relevant to the main WC storage:

1. the base
2. the committable changes
3. uncommittable WC state (conflicts, changelists, etc.)

For (1) the base, the "reporter" is the required API definition, and implementations to both update the base layer and read (report) the base layer through this API exist.

For (2) committable changes, the "delta editor" is the right API, by definition. The client-layer "commit" function of course sends WC changes out through a delta editor API through the RA layer. This implementation is rather tightly coupled to that RA-layer consumer. An implementation of a delta editor API for making changes in the WC doesn't exist at all. And it should.

For (3) uncommittable changes, an interface definition doesn't even exist. My shelving design doesn't attempt to support part 3, largely because I knew the API didn't exist. If it did, it would be relatively easy for shelving to support it.

See my recent comments in https://issues.apache.org/jira/browse/SVN-4745 (Shelving: "shelf-diff").

With these interfaces in place it would be relatively straightforward to implement the logic of shelving. And then making the implementation use the efficient existing DB storage later would become the priority.

But first my task is to put those interfaces in places, uncoupling and extracting the ones that already partly exist and writing the new ones.

- Julian

Re: thoughts about shelving

[Julian Foad <ju...@apache.org>]

Julian Foad wrote:
> [...] Flat files (no longer flat files) are easier to mess with at
> this stage.
I meant, "Flat files (no longer patch files)...".

- Julian

Re: thoughts about shelving

[Julian Foad <ju...@apache.org>]

Greg Stein wrote:
> When I envisioned the implementation of shelving, I figured that we
> would use the sql database to its best effect. Store new pristines
> (which is logically just key->fulltext). Maybe create an artificial
> wc_id to construct an alternate tree of ACTUAL_NODE entries where you
> can store node information of the items on the shelf.> 
> We have a database. I see no reason to fall back to messing with
> patch files.
Hi Greg. I agree that (re-)using existing DB will be a better storage
subsystem, and would welcome developments in that direction. My priority
at this stage is getting the high level API and architecture and feature
set right. Flat files (no longer flat files) are easier to mess with at
this stage.
- Julian

Re: thoughts about shelving

[Greg Stein <gs...@gmail.com>]

On Sat, Sep 15, 2018 at 4:03 AM Julian Foad <ju...@apache.org> wrote:
>...

> > * when users update to the new version, existing shelves can't be used
> > anymore since they're not compatible. Is there a way to convert the old
> > shelves to the new format? If not then that means users would lose those
> > saved shelves.
>
> There isn't a way to convert old shelves (which are patch files) to the
> new format.
>
> However, as they are just patch files, there is an easy manual work-around
> to still apply them -- using "svn patch <full path>".
>

When I envisioned the implementation of shelving, I figured that we would
use the sql database to its best effect. Store new pristines (which is
logically just key->fulltext). Maybe create an artificial wc_id to
construct an alternate tree of ACTUAL_NODE entries where you can store node
information of the items on the shelf.

We have a database. I see no reason to fall back to messing with patch
files.

>...

Cheers,
-g

Re: thoughts about shelving

[Julian Foad <ju...@apache.org>]

Stefan Kueng wrote:
> A few more thought, mostly for the next release:
> 
> * svn_client_shelf_save_new_version3 takes two callback functions, but 
> they both have the same type. So why not just change that API to take 
> one callback but provide a 'failed' flag instead?

Could do! I suppose that would mean less boilerplate code for you (the client) to write.

> * when a shelve operation fails, we have to remove the version with 
> svn_client_shelf_delete_newer_versions. But if there are no more 
> versions left, the shelf itself is not removed. So while testing I ended 
> up with several empty shelves. I'm now using 
> svn_client_shelf_get_all_versions to count how many versions are left 
> and if there are none I'm calling svn_client_shelf_delete. I think 
> svn_client_shelf_delete_newer_versions should do that automatically. 
> Unless of course there's a reason why empty shelves can exist?

I'm not entirely happy with the current relationship between a "shelf" object as a container of "shelf version" objects. At the moment a "shelf version" can only exist as a child of a "shelf". It is intentional that you can have a shelf that has no versions. It can still have a log message. (The user might want to name their shelf and write their log message before they start coding, for example, but that isn't a compelling use case.)

I am considering partly inverting the relationship so a single-version shelf would be the primary object and a series-of-versions (for checkpointing and rollback) would be a higher level object that is a collection of single-version shelf objects. What do you think?

> * maybe this one can make it into 1.11: the doc comments don't mention 
> that the version numbers are 1 based, not 0 based. [...]

Thanks. I'll add it.

> * some thoughts [...] you first have to open/create the shelf, save the new 
> version and close the shelf. [...] I would expect one API call [like]
> svn_client_shelf(shelveName, wcPath, logMsg, depth, keeplocal);
> and it would do everything automatically.

I agree access to a higher level API would be useful, and I'll make a mental note to add that.

At the same time I think it's essential that clients do have access to the lower-level building blocks of the operation, in order to build more interesting and different variants from what I implemented for the command-line client.

So I would like to provide two levels of API. You won't then have to user the lower-level APIs if you don't want to, but any third-party who wants to innovate on the idea will then have the tools to do so.

> in case you're interested, here are screenshots of the shelve and 
> unshelve dialog I did in TSVN:
> https://svn.osdn.net/svnroot/tortoisesvn/trunk/doc/images/en/Shelve.png
> https://svn.osdn.net/svnroot/tortoisesvn/trunk/doc/images/en/Unshelve.png

Thanks for sharing those!

-- 
- Julian

Re: thoughts about shelving

[Stefan Kueng <to...@gmail.com>]

A few more thought, mostly for the next release:

* svn_client_shelf_save_new_version3 takes two callback functions, but 
they both have the same type. So why not just change that API to take 
one callback but provide a 'failed' flag instead?

* when a shelve operation fails, we have to remove the version with 
svn_client_shelf_delete_newer_versions. But if there are no more 
versions left, the shelf itself is not removed. So while testing I ended 
up with several empty shelves. I'm now using 
svn_client_shelf_get_all_versions to count how many versions are left 
and if there are none I'm calling svn_client_shelf_delete. I think 
svn_client_shelf_delete_newer_versions should do that automatically. 
Unless of course there's a reason why empty shelves can exist?

* maybe this one can make it into 1.11: the doc comments don't mention 
that the version numbers are 1 based, not 0 based. On my first test, I 
ended up calling svn_client_shelf_version_open with version = 0, which 
of course returned an error. So I noticed it right away. But I still 
think this should be in the doc comments - I assume that most devs using 
this API are not VB programmers :)

* some thoughts in general: I think for svn_client_ API's, the 
shelf-API's don't really 'fit' in. The way they work now they're very 
flexible, but not really from a client point of view. For example, to do 
a shelve operation you first have to open/create the shelf, save the new 
version and close the shelf. And if there are errors, you also have to 
clean up separately. For an svn_client_ API I would expect one API call 
that does this job, including cleaning up in case of an error. Compared 
to e.g. svn_client_log() that API does the whole job, no need to first 
open the connection, send the request and get the data, then close the 
connection - the API does all this in one call.
So for the shelf API I would expect it something like this:
svn_client_shelf(shelveName, wcPath, logMsg, depth, keeplocal);
and it would do everything automatically.
Not very urgent, just something you might want to consider.


in case you're interested, here are screenshots of the shelve and 
unshelve dialog I did in TSVN:
https://svn.osdn.net/svnroot/tortoisesvn/trunk/doc/images/en/Shelve.png
https://svn.osdn.net/svnroot/tortoisesvn/trunk/doc/images/en/Unshelve.png


Stefan

Re: thoughts about shelving

[Julian Foad <ju...@apache.org>]

Stefan Kueng wrote on 2018-09-15:
> Hi,
> 
> I know I'm a little bit late, but now I'm implementing the new shelf 
> APIs in TSVN so it's now that I have some thoughts and suggestions:
> 
> * there are deprecated APIs already, even though they weren't there in 
> 1.10.0, for example svn_client_shelf_save_new_version - since they're 
> all new in 1.10.1, why not just remove the deprecated ones and keep the 
> current one?

Good point -- I can remove those.

> * when users update to the new version, existing shelves can't be used 
> anymore since they're not compatible. Is there a way to convert the old 
> shelves to the new format? If not then that means users would lose those 
> saved shelves.

There isn't a way to convert old shelves (which are patch files) to the new format.

However, as they are just patch files, there is an easy manual work-around to still apply them -- using "svn patch <full path>".

I filed issue https://issues.apache.org/jira/browse/SVN-4774 "Shelving: presence of old 1.10 shelves breaks shelf-list" . While fixing that I can probably make some other changes, e.g. adding a description of how to manually recover an old shelf.

> * I generate an API doc with Doxygen. But some comments are not properly 
> formatted for this: if a comment is after a variable, the comment must 
> be formatted properly otherwise the Doxygen docs apply the comment to 
> the wrong variable. For example, svn_client_shelf_version_t looks like 
> the attached image. To make this work properly, the struct should look 
> like this:
> 
> typedef struct svn_client_shelf_version_t
>   {
>     /** Public fields (read-only for public use) */
>     svn_client_shelf_t *shelf;
>     apr_time_t mtime;  /**< time-stamp of this version */
> 
>     /** Private fields */
>     const char *files_dir_abspath;  /**< abspath of the storage area */
>     int version_number;  /**< version number starting from 1 */
>   } svn_client_shelf_version_t;
> 
> Note the '<' in the field comments.

Thanks for that fix. I'll commit it.

> that's it for now. But since I'm not finished yet implementing 
> everything in TSVN I might have some more comments soon.

Please do! Even bigger things that can't be fixed for 1.11 can be fixed immediately afterwards.

-- 
- Julian