Bug - Fixed "svn update" does not move files in local copy

[philmasterplus]

The require call of findScript needs to generate an error if there is more than one result.

Uh, sure. I don't have an opinion on this, and it probably won't affect me very much.

If we ask JS writers to explicitly specify a file is in the current directory or a subdirectory of the current directory and ask them to create a subdirectory of /scripts for their scripts does that eliminate name collisions with any other ash or JS script? I think yes but am open to being educated.

JS authors never had name collision troubles at run time because we always use relative paths to import other JS scripts, or because we bundle our code into one big script which imports nothing. Is there a problem that needs solving here...?

I'd like to focus on name collisions that happen during install time, which isn't tied to ASH or JS. It can be solved by enforcing strict directory structure replication.

If css and html files are expected to be in /relay and by analogy to /var/www/html are expected to be shared do we make certain named files part of the KoLmafia distribution (with all the baggage that implies) or to we voluntarily impose a naming convention on script writers in hopes of reducing collisions?

I lost your train of thought here. What "named files" are you talking about?

When writing relay scripts, we conventionally created one subdirectory per relay script and put our assets (CSS, browser-only JS) in there. Now that we know it's unsafe, we would have to give unique names to all assets.

Can we tell rhino to use the script's directory instead of /relay for name resolution?

Rhino doesn't use relay/ for module resolution at all. It uses KoLmafia's working directory and the scripts/ directory as the base directory.

Also, it doesn't search recursively for JavaScript files. If I import another JS script using require('test.js') (notice that it has no leading dot), Rhino will check /test.js and /scripts/test.js but look no further.

 

[fronobulax]

If we tell JS authors that all the files they care about and need in order to run have to be in a tree with a common root in /scripts then there will never be name collisions on install or update unless two different script writers choose the same directory name. "Don't Use a Directory Name Someone Else Is Already Using" is an acceptable solution as far as I am concerned although a scheme that reserved directory names could be developed if that was how someone wanted to spend their time.

The questions about Rhino and html and css were driven by the possibility that some files had to be in /relay in order for KoLmafia to do the right thing or for Rhino to find them. If everything can run from one directory elsewhere then there will be no collisions in /relay.

I seem to recall that relay scripts that override or annotate a KoL page have to be named in a particular way and in the days before JS there were some conventions developed so that two different scripts could modify the same page. If that is correct then JS writers who have to put something in /relay because of the hooks need to follow the convention and have everything else live elsewhere.

I'm not convinced that there is a real collision problem if git users would restrict their repository management to things that are known not to have issues with SVN and if JS users only used one subdirectory. I'd rather ask a script writer to follow certain conventions than write code that will still break if something is done outside of KoLmafia.

 

[philmasterplus]

I'm not convinced that there is a real collision problem

We did find a name collision problem in the wild between Cargo Shorts UI and Philter. It was due to both scripts providing an index.html in two different directories. These files were not trying to override the same page; they were completely unrelated files, with the unfortunate coincidence of having the same name.

This is what bean sees when he writes code for Cargo Shorts UI, and what he expects KoLmafia to create:

relay/
  cargo/
    index.html

This is what I see when I write code for Philter, and what I expect KoLmafia to create:

relay/
  philter-manager/
    index.html

In an ideal world, if a user installed both, KoLmafia would have installed them like this:

relay/
  cargo/
    index.html
  philter-manager/
    index.html

But one file ended up in a wrong directory because KoLmafia used findScriptFile() to rebase it onto the other file during "svn install":

relay/
  cargo/
    (No file)
  philter-manager/
    index.html    <= Overwritten by cargo's index.html

This was not caused by JavaScript, Git, or Subversion. It was caused by the code in SVNManager.java that calls findScriptFile().

The same problem could have happened with styles.css, data.json, or whatever file name we chose. It just happened to be index.html today.

I can accept having to give unique names to ASH and JS files. But doing so for every file greatly increases the surface area for error. If you still view this as a "won't fix", please reconsider.

 

[fronobulax]

Progress.

We have a remote repository with relay/a/index.html

We use install to create a local repository that has relay/a/index.html and relay/a/index.html in the KoLmafia "file system"

We have a second remote repository with relay/b/index.html.

We use install, perhaps followed by a change to index.html and an update command.

We expect the local repository to have relay/b/index.html and the mafia file system to have relay/a/index.html and relay/b/index.html.

Instead the local file system has overwritten relay/a/relay/index.html. with relay/b/index.html..

If this is a description of the problem, with perhaps a couple of details to be tweaked, then yes, it is a problem and yes it needs to be fixed. I will confirm I can reproduce and investigate with the expectation that KoLmafia needs to be "fixed".

Two things immediately make me wonder.

The findScripts code has extensions hard wired and special treatment related to them. From my memory of the code I'm not sure it will find index.html but if it does it will stop looking when it finds the first instance. So if findScripts is at all involved in finding index.html there is probably an error in doing so.

The use of the word "rebase" is interesting and if it indicates KoLmafia is executing a SVN rebase command I am perhaps concerned. rebase deals with the local and remote repositories. Period. So there is a possibility of a bug in the "copy" code executed as part of a rebase which would be a KoLmafia issue.

Replicating the above and understanding why will probably lead me down the right rabbit hole and let me identify a problem that exists and needs to be fixed in code.

Thanks.

 

[philmasterplus]

Thank you.

For what it's worth, I used the term "rebase" because SVNManager.java has a function named getRebase(). It is totally unrelated to rebasing in SVN, and I don't think KoLmafia does a SVN rebase. It just borrowed the terminology. I hope it didn't confuse you too much.

 

[MCroft]

Thank you.

Yes. Anything I can do to help, please let me know.

For what it's worth, I used the term "rebase" because SVNManager.java has a function named getRebase(). It is totally unrelated to rebasing in SVN, and I don't think KoLmafia does a SVN rebase. It just borrowed the terminology. I hope it didn't confuse you too much.

Yep, the code fragment on page 1 of this thread from SVNManager.java doPush() that we focused in on at the start is calling getRebase(). The term is overloaded and doesn't mean what SVN means by that term.

 

[fronobulax]

Yep. When we do a checkout it looks for an existing file of the same name but not the same name and directory. Looks like the problem has been there since 2013. Not sure what my fix will be but I gotta do something. thanks to @philmasterplus who constructed test repositories so I didn't have to.

 

[fronobulax]

I have a fix. It will not think the presence of /somedir/a.css is significant when preparing to create /someotherdir/a.css. Probably check it in tomorrow after I get another day with it.

I noted that uninstalling a script doesn't always delete every file but that is a feature of versions without my fix so it can be another discussion/bug report. For that matter unravelling code so that we distinguish between looking for a "script" file that can be anywhere in selected directories and a named file that is expected in a particular directory might be worthy of discussion.

 

[fronobulax]

r20743.

Gonna call this fixed since SVN is more aware of directories than it was before so the case where script A overwrites script B's file of a same name is no more.

There are still cases where SVN operations do not delete files and directories. These cases existed before this change - I checked - and there are philosophy issues raised in this thread that should probably be hammered out before changing code. New thread, if anyone really cares?

It is pretty clear that evolution in scripting has resulted in code that was originally designed to manage scripts and is now being asked to manage arbitrary files used by scripts. There is an opportunity for refactoring that could be explored. Again, perhaps a new thread?

 

[taltamir]

Maybe we need a mapping between the menu and a name there and a file. It might be nice to choose what's displayed, and not have the menu show ___auto_DO_NOT_RUN_FILES_IN_THIS_DIRECTORY

in regards to that specifically.
I think it wold be great if there was a way to instruct mafia itself that a specific file should be excluded from the scripts dropdown menu?
like starting the first line with
#exclude_dropdown
and some way for include to handle it. (either only use it to exclude from dropdown if it is the first line. or have the include command skip that specific line)

or maybe
exclude_dropdown("scripts/autoscend/util.ash");

 

[fronobulax]

Feature - Revert 20743?

See https://kolmafia.us/threads/svn-update-does-not-move-files-in-local-copy.26043/page-4#post-162486 and https://kolmafia.us/threads/20743-when-considering-overwrites-triggered-by-svn-match-directory-and-file-name-not-just-file.26094/ for background and related. discussion. Prior to r20743 it...

kolmafia.us

in hopes of gathering relevant comments in one place.