Thanks. Without digging deep at all, a quick answer:
Evidently, that is a corner case that doesn't have any/sufficient test coverage. But it might not be worth it to add it.
First of all, it is a bug that this NodeError shows up as a server error. It should show a nice error, as it (hopefully) happens if you specify an invalid repo, and invalid revision, or a completely invalid file path. Even a 404 would be better, but Kallithea rarely does that on invalid input data. Perhaps the exception handling is missing for NodeError or in general. Perhaps it should handle NodeError too, or perhaps it should raise another exception.
It is also a bug that the message in the exception isn't suitable for display to the user. That should be fixed too.
In general, when showing the diff of a file that has been added or removed, it would be nice to clearly show it as added / removed and a diff against an empty file. That seems to be approximately what happens. But perhaps not making it sufficiently clear if a file was added/removed or just is empty. There might be something there that would be nice to get fixed too.
If showing the diff of a file that either has been moved to or from the file name (or copied), it would be nice to show the diff across the rename and make it clear that it has been renamed. At least in URLs that can be reached from the Kallithea UI. I'm not sure that is handled correctly in all cases. Perhaps not at all. We should perhaps spend more time looking at the changeset data before deciding exactly which diff to show. But also, while it might be feasible for Mercurial where renames are tracked and easily available, it might not be feasible for Git where renames have to be guessed from looking at the changesets. This might be nice-to-have area, but perhaps not worth it.
For both add/remove and rename to/from, I don't think Kallithea should behave differently if there is a directory instead of "nothing". We don't even have to tell about the directory. Both Mercurial and Git only track files, and directories only exist as collections of files. Perhaps, if the found node is a dir, just raise NodeDoesNotExistError and go to the "file not found" branch?
Also, there is no prior art for comparing directories in Kallithea. Introducing that would not be a bug fix but proper development. I don't think that is worth it. Just throw a clear error message.
Great if you can work on fixing this area. Please try to separate the fixes for each issue in clean commits.
/Mads On 12/11/2023 00:40, Branko Majic wrote:
Ok, I've managed to hack up a quick fix for this, but... It does look a bit ugly. Feels like there's too much repetitive code in there as well. The patch is attached to the mail. Looking at what the server does when you pass in a path that is directory in both changesets, it really should not throw an internal error for it. It should probably just do a diff on two changesets that are basically empty (like the whole NodeDoesNotExistError handling), or try to display some form of information to user (or maybe give a 404 or something similar). So, before I start delving into this with proper patch, any opinions on it? :) Best regards, Branko _______________________________________________ kallithea-general mailing list kallithea-general@sfconservancy.org https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
_______________________________________________ kallithea-general mailing list kallithea-general@sfconservancy.org https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
