[PATCH] Pre-lock script and general hook script fixes

[Daniel Sahlberg]

Hi,

I've been investigating the pre-lock script and found a few things that doesn't make sense to me.

The first file in the pre-lock script argument is always empty

This is because CSVNProgressDlg::CmdLock is calling the PreLock hook using m_selectedPaths, however LockCommand::Execute is setting m_targetPathList. I believe it is more consistent to use m_selectedPaths (as with Commit hooks) and I've attached a patch to call CSVNProgressDlg::SetSelectedList in addition to CSVNProgressDlg::SetPathList (which is needed for SetWindowTitle among others).

Parameter missmatch between documentation and implementation

The documentation (dug_settings_hooks.xml) list the following parameters for Pre-lock: PATH LOCK FORCE MESSAGEFILE ERROR CWD

Actual parameters seems to be missing the ERROR parameter.

It would be handy to be return an error message from the pre-lock script instead of just returning an error code and getting:

However since none of the other pre-... scripts have the error parameter I think this is by design.

I'm attaching a patch to adjust the documentation to actual behaviour, but if it is preferred I can instead create a patch to add the error parameter and read the contents of the file to the error string. (In that case I believe the ERROR parameter should be moved last to avoid messing up any existing pre-lock scripts that depend on the actual parameters. I also believe it would be good to add the same parameter to the other pre-... scripts for consistency.)

Difficulties finding the hook from Settings 

I have set a hook script in the general settings for TSVN like this.

I'm trying to lock C:\devel\test2\test.txt. However CHooks::FindItem doesn't find my pre-lock script. I had to resort to setting Working Copy Path to * to get a match.

I believe this is because CHooks:Create() is reading the hooks settings from the Registry and create a key.path as a CTSVNPath which only contains an m_sFwdslashPath. However CHooks::FindItem is working with a CTSVNPath object that have several of the strings defined and then map::find won't find the proper script.

I'm not completely sure how to solve this. I tried a naive solution to modify CHooks::FindItem to make sure the path only contained m_sFwdslashPath, however that failed when testing hooks set in the tsvn:prelockhook property (these contained a "fuller" path object). One possible solution would be to change the hookkey class to a plain CString (attached), I choosed GetWinPathString since it was already used in a few places to convert the path to CString.

Displaying the Lock comment when lock(ing) fails

The background of this whole thread was that I tried to create a pre-lock script that added the current computer name and WC path to the lock message. I can see the lock comment in "Check for modifications". However it would be very handy to get the lock comment already in the error message when SVN fails to aquire the lock.

I have made a very naive solution to catch m_bLockExists in CSVNProgressDlg::CmdLock and calling SVNInfo and then adding the comment using ReportString. It works [in principle], but fails miserably when locking more than one file (since the message is added in the end of the list and not next to the relevant file). Therefor I'm not attaching the patch.

Any ideas on this? Ideally Subversion would return the comment in addition to the W160035 "Path ... is already locked ..." error message. I will see if this is possible to add, it would also be beneficial for other clients than TSVN.

Kind regards,

Daniel Sahlberg

 

[Daniel Sahlberg]

Answering myself..

måndag 4 januari 2021 kl. 01:13:27 UTC+1 skrev Daniel Sahlberg:

Any ideas on this? Ideally Subversion would return the comment in addition to the W160035 "Path ... is already locked ..." error message. I will see if this is possible to add, it would also be beneficial for other clients than TSVN.

It  looked quite complicated to change W160035. So I digged deeper and think I have figured out a way get the lock  comments in the right place. The "Locked on DESKTOP-..." lines are the lock comments.

The Action column should/could probably be set to something. Also multi line lock messages should probably be handled in some way - now the lines are just concatenated to a single line (I haven't tried very long lock messages).

Kind regards

Daniel Sahlberg

[Stefan]

committed your patch in r29050

[Daniel Sahlberg]

Thanks!

Did you also take a look at the patches in the first message? Sorry if it caused extra work for you but I split it up to make it easier to review.

I realize I forgot to attach one of the patches to the first message, I'm attaching all three again for easier reference.

Kind regards,

Daniel

fredag 8 januari 2021 kl. 21:45:03 UTC+1 skrev Stefan:

committed your patch in r29050

[Stefan]

committed two of your patches in r29051, but I don't see the benefit of your hookkey-as-CString patch. Why do you think using a CString is better there?

[Daniel Sahlberg]

Thanks!

The problem I'm trying to solve is that I couldn't get the hooks to execute when I set them to a specific path (c:\devevel\test, which is my wc), only when using *. I stepped through the code in Visual Studio and I noticed that CHooks::FindItem didn't find a hook (it = find(key) always returned end()) for a path even though I had seen it being added from the registry just before.

My understanding is as follows:

* A CTSVNPath object contain a number of separate CStrings for storing the path. Depending on how it is set, only some are populated. (The rest are populated if requested).

* When the hookkey is created the path is constructed in different ways in different places of the code, for example in Hooks.cpp when reading from the Registry it is created using the constructor (which calls SetFromUnknown, in turn calling SetFwdslashPath) and in ParseAndInsertProjectProperty the wcRootPath has been constructed in a different way.

* The pathList in CHooks::FindItem again contain different members of CTSVNPath.

* The STL map::find function will only match a key if all members are exactly the same.

(The last point is merely an observation of behaviour, I can't give any references to this being expected behaviour).

By switching out the CTSVNPath to CString (and being strict about that string always being retrieved from CTSVNPath::GetWinPathString()) I'm able to find my hook scripts.

Initially I made a simpler patch in CHooks::FindItem which instead of just taking pathList[i] created a new CTSVNPath object that only had the forwardslashpath set. That worked for hook scripts from the registry however failed for hook scripts as project properties. For this to work all places where the hookkey path is set must create a new CTSVNPath with only the forwardslashpath set. [Of course it could also be using the backslashpath instead].

Sorry for the long explaination but I must admit C++ is not my primary langauge.

Kind regards,

Daniel Sahlberg

[Stefan]

Well, CTSVNPath has multiple private members, but when comparing paths that should never matter because each member is constructed as necessary.

If you have a situation where the hook scripts are not executed/found for a specific path, I'd like to have a clear example of that situation so I can fix the real problem - usind CString instead is not really a solution but I think only a workaround in this case.

[Daniel Sahlberg]

I have investigated further and it seems that pre-lock hook scripts on a specific working copy path[1] doesn't work in 1.14.0 (and up to r29050). Only pre-lock hook scripts on path *[2] works.

I made an initial note of this and then proceeded to work on the other patches. When I got back to this (well past midnight, which might have clouded my judgement) I thought that the map never found the path (as described above). However it turns out that the actual problem was already solved by the "LockCommand SetSelectedList" patch and I just misdiagnosed the whole thing. Adding insult to injury I misunderstood how std::map works and didn't notice that it actually use the operator<(CTSVNPath&, CTSVNPath&) function (which work perfectly fine).

All in all, please ignore the "hookkey as CString" patch.

Images for reference, in case someone else is struggling with pre-lock hook scripts in 1.14.0 and below.

[1] This hook script will not execute in 1.14.0 and below:

[2] This hook script will execute (for all WCs):