[PATCH] URL copied via "Copy URL to clipboard" results in error when pasted into Merge dialog

[Daniel Sahlberg]

Hi,

I have investigated the issue mentioned in the users group in the thread with the same subject as above [1]. I'm providing an analysis and a few different patches (choose 1 of those).

The root cause is that CopyUrlsCommand add \n at the end of the copied paths. It has the capability to copy multiple paths and URLs so I believe this is a good thing for consistency. However it could be modified [2] to only emit the \n if more than 1 path is being copied. I believe this is the least sensible path, both since it changes external behaviour.

Another solution could be to adjust the consumer of the URL, in MergeCommand.cpp, to remove any trailing newline from the URL. See [3].

However I realised that this behaviour only exists on "Merge a range of revisions". If doing a "Merge two different trees", the trailing newline is filtered already in the dialog. On inspection MergeWizardRevRange.cpp does m_urlCombo.GetWindowText(sUrl) while MergeWizardTree.cpp does m_urlCombo.GetString(). (MergeWizardTree.cpp actually does both, with GetWindowText() to check if the text is a URL). I've made a patch [4] to have MergeWizardRevRange.cpp use GetString() just like MergeWizardTree.cpp. It is actually reversing part of the changes in r24686 since previously. This is the patch I personally prefer, it seems cleaner to remove the \n on the source, it also aligns with hor "Merge two different trees" behave.

Kind regards,

Daniel

[1] https://groups.google.com/g/tortoisesvn/c/PHloNjOHids

[2] Attachment CopyUrlsCommand_DontSetLFOnSinglePath.txt

[3] Attachment MergeCommand_RemoveTrailingNewlineFromUrl.txt

[4] Attachment MergeWizardRevRange_GetUrlByGetString.txt

[Stefan]

thanks for all your patches!

I went with option 4, I agree that's the best solution.

one question though: how did you create the patch files? They have absolute paths (e.g. C:/Users/danie/AppData/Local/Temp/MergeWizardRevRange.cpp) and no context lines at all.

Stefan

[Daniel Sahlberg]

lördag 28 maj 2022 kl. 10:24:21 UTC+2 skrev Stefan:

thanks for all your patches!

I went with option 4, I agree that's the best solution.

(y), LGTM.

I realise you reworked the patch a bit and removed GetWindowText altogether (I thought about it but figured there was probably a reason it was used). For the matter of consistency, should the same be applied to MergeWizardTree? Something along the lines of [1]?

 

one question though: how did you create the patch files? They have absolute paths (e.g. C:/Users/danie/AppData/Local/Temp/MergeWizardRevRange.cpp) and no context lines at all.

Commit dialog.  Double click changed file (=> Opens TortoiseMerge). File, Create patch file. Save.

The attached files in this message are created from the Commit dialog, selected the file(s), right click and Create patch. It contains three context lines. (I believe this patch is created by TortoiseProc.exe?). However it contains no paths at all which is a bit strange since I started from a commit dialog in the root of the WC [4], so the commit dialog at least know the relative paths within the WC. 

TortoiseMerge seems to look at Software\TortoiseMerge\ContextLines (from the settings dialog), which I havn't set, and thus gets a default value of 0 from src/TortoiseMerge/MainFrm.cpp line 2950. I'm attaching a proposed patch [2] to change this to three, which seems a better default. There were a few additional places in the code looking at the above reg key, but with a default value of 1 that I didn't change. I'm not sure if it makes sense to have different default values for the same registry key. The attached patch [3] should change the default setting in the Settings dialog to 3 as well. 

/Daniel

[1] MergeWizardTree_DontUseGetWindowText.txt 

[2] TortoiseMerge_Unidiff_Default_ThreeLinesContext.txt

[3] SetMainPage_Default_ThreeLinesContext.txt

[4] commitdialog.png

[Stefan]

On Saturday, May 28, 2022 at 4:18:44 PM UTC+2 daniel.l...@gmail.com wrote:

lördag 28 maj 2022 kl. 10:24:21 UTC+2 skrev Stefan:

thanks for all your patches!

I went with option 4, I agree that's the best solution.

(y), LGTM.

I realise you reworked the patch a bit and removed GetWindowText altogether (I thought about it but figured there was probably a reason it was used). For the matter of consistency, should the same be applied to MergeWizardTree? Something along the lines of [1]?

now that you mention it: yes there was (and is) a reason for using GetWindowText: GetString can return an empty string sometimes (e.g. when using up/down to select from the history without hitting return). Not always, but sometimes.

So I'm going to change this back to using GetWindowText again and then trim the string from newlines.

 

TortoiseMerge seems to look at Software\TortoiseMerge\ContextLines (from the settings dialog), which I havn't set, and thus gets a default value of 0 from src/TortoiseMerge/MainFrm.cpp line 2950. I'm attaching a proposed patch [2] to change this to three, which seems a better default. There were a few additional places in the code looking at the above reg key, but with a default value of 1 that I didn't change. I'm not sure if it makes sense to have different default values for the same registry key. The attached patch [3] should change the default setting in the Settings dialog to 3 as well. 

Thanks!

I've applied your patches.

If you would register at osdn.net I can give you commit access so you can commit your fixes yourself :)

Stefan

[Daniel Sahlberg]

lördag 28 maj 2022 kl. 19:18:22 UTC+2 skrev Stefan:

On Saturday, May 28, 2022 at 4:18:44 PM UTC+2 daniel.l...@gmail.com wrote:

lördag 28 maj 2022 kl. 10:24:21 UTC+2 skrev Stefan:

thanks for all your patches!

I went with option 4, I agree that's the best solution.

(y), LGTM.

I realise you reworked the patch a bit and removed GetWindowText altogether (I thought about it but figured there was probably a reason it was used). For the matter of consistency, should the same be applied to MergeWizardTree? Something along the lines of [1]?

now that you mention it: yes there was (and is) a reason for using GetWindowText: GetString can return an empty string sometimes (e.g. when using up/down to select from the history without hitting return). Not always, but sometimes.

Oh.  Sometimes is never nice.

So I'm going to change this back to using GetWindowText again and then trim the string from newlines.

LGTM, again.

Just one more thing <tm>.  In MergeWizardTree.cpp, you first check the URL using GetWindowText but when setting the m_urlFrom and m_urlTo you still get the URL using GetString(). So I would re-order things as in patch [1]. 

TortoiseMerge seems to look at Software\TortoiseMerge\ContextLines (from the settings dialog), which I havn't set, and thus gets a default value of 0 from src/TortoiseMerge/MainFrm.cpp line 2950. I'm attaching a proposed patch [2] to change this to three, which seems a better default. There were a few additional places in the code looking at the above reg key, but with a default value of 1 that I didn't change. I'm not sure if it makes sense to have different default values for the same registry key. The attached patch [3] should change the default setting in the Settings dialog to 3 as well. 

Thanks!

I've applied your patches.

LGTM.

I will have a second look at TortoiseProc (as mentioned in the previous message) to see if I can make it emit a path base in to WC root instead of just the file name. I guess it might be difficult to do with TortoiseMerge since it basically compares two different files on the computer. It could, possibly, emit a path based in the WC root if either file is in a WC (in the patches in the first message, C:/Devel/tortoisesvn is a WC), but it will be impossible to do with the file from %TEMP%.

 

If you would register at osdn.net I can give you commit access so you can commit your fixes yourself :)

Login: danielsahlberg

Appreciate your confidence :-)

 

Kind regards,

Daniel

[1] MergeWizardTree_DontUseGetString.txt

 

[Stefan]

Just one more thing <tm>.  In MergeWizardTree.cpp, you first check the URL using GetWindowText but when setting the m_urlFrom and m_urlTo you still get the URL using GetString(). So I would re-order things as in patch [1]. 

nice catch!

You can commit the patch now yourself.

 

I will have a second look at TortoiseProc (as mentioned in the previous message) to see if I can make it emit a path base in to WC root instead of just the file name. I guess it might be difficult to do with TortoiseMerge since it basically compares two different files on the computer. It could, possibly, emit a path based in the WC root if either file is in a WC (in the patches in the first message, C:/Devel/tortoisesvn is a WC), but it will be impossible to do with the file from %TEMP%.

That would be good, yes.

However, it is usually enough to emit the filename: when applying a patch, TMerge automatically searches for a path that matches the paths/names in the patchfile.

 

 

If you would register at osdn.net I can give you commit access so you can commit your fixes yourself :)

Login: danielsahlberg

Appreciate your confidence :-)

You now have full access.

Stefan

[Daniel Sahlberg]

lördag 28 maj 2022 kl. 20:45:42 UTC+2 skrev Stefan:

Just one more thing <tm>.  In MergeWizardTree.cpp, you first check the URL using GetWindowText but when setting the m_urlFrom and m_urlTo you still get the URL using GetString(). So I would re-order things as in patch [1]. 

nice catch!

You can commit the patch now yourself.

r29402

 

I will have a second look at TortoiseProc (as mentioned in the previous message) to see if I can make it emit a path base in to WC root instead of just the file name. I guess it might be difficult to do with TortoiseMerge since it basically compares two different files on the computer. It could, possibly, emit a path based in the WC root if either file is in a WC (in the patches in the first message, C:/Devel/tortoisesvn is a WC), but it will be impossible to do with the file from %TEMP%.

That would be good, yes.

However, it is usually enough to emit the filename: when applying a patch, TMerge automatically searches for a path that matches the paths/names in the patchfile.

That's clever.

The command line 'svn diff' is basing the file names on the current working directory and not the WC root.

After some basic checks, it seems TortoiseProc is being fed a list of full paths without a notion for "working directory" and it bases the patch in the lowest common denominator of these paths.

Since there are already some differences and TMerge is clever enough to work them out I will probably ignore it (I've got other itches to scratch, more in a new thread).

If you would register at osdn.net I can give you commit access so you can commit your fixes yourself :)

Login: danielsahlberg

Appreciate your confidence :-)

You now have full access.

Thanks!

/Daniel