Re: svn commit: r1834612 - in /subversion/branches/1.10.x-issue4758/subversion: libsvn_client/shelve.c tests/cmdline/shelve_tests.py

There are three issues here:

1.

Philip Martin wrote:
> There are SVN_ERR between these two hunks. If one of these returns an
> error we will fail to restore the old config.

There are four other places where libsvn_client functions modify ctx in place. They are all modifying the notification callback, with the following pattern:

  modify ctx->xxx
  err = func();
  restore ctx->xxx
  SVN_ERR(err);

To solve that problem I will use the same pattern as above.

r1834835, added to backport nomination in r1834836.

Note that this fix is for the 1.10.x branch only, as this bit of shelving code doesn't exist on trunk.

2.

> I did also worry about thread safety: it's not safe to modify the
> context like that if the context can be shared across multiple threads.
> However the context also includes batons and those typically point to
> mutable data which cannot be shared across threads either. I suppose we
> should document that a client should only access a context from one
> thread at a time.

Documenting that sounds reasonable. Like this, just above typedef struct svn_client_ctx_t, do you think?
[[[
 /**
  * Client context
  *
+ * A client should only access a context from one thread at a time, as
+ * the context includes batons that typically point to mutable data which
+ * cannot be safely shared across threads.
+ *
  * @defgroup clnt_ctx Client context management
  *
  * @{
  */
]]]

3.

>> Bert Huijben writes:
>>> This resets quite a bit more configuration that might still be
>>> necessary. [...]
>>> I think you should duplicate the hash and explicitly remove a few keys.
>
> If we do remove keys then I think DIFF_CMD and DIFF_EXTENSIONS are the
> ones.

That seems like the correct form of solution, in general.

In this case, however, the only reference to 'ctx->config' in the whole of libsvn_client/diff.c is passing it to create_diff_writer_info() to configure an external diff tool, and I don't think it is used by any called functions either.

So I think this is OK as it is.

- Julian
Received on 2018-07-02 13:20:19 CEST