Re: 500 Internal Server Error with 2-way diff and commit that renames file to its parent directory
Hello and sorry for the late reply :) I am still interested in getting a nice fix for these couple of issues, although it might take me a bit of time (got a couple of side-projects currently related to my current desktop slowly kicking the bucket...). I'll try to address the issues one at a time - things which are happening on the backend side (this particular 500 being thrown) should probably be easier to handle, so I'll try to tackle those first. As for the user interface - it will be a bit harder, since I'm not that good with HTML/JS/CSS, but I will certainly give it a go. Hopefully I would just be able to reuse existing code and tweak it for better display etc. :) Best regards, Branko P.S. And I really appreciate the feedback, it can be a bit time consuming to provide it to people who are drive-by contributors :) -- Branko Majic XMPP: bra...@majic.rs Please use only Free formats when sending attachments to me. Бранко Мајић XMPP: bra...@majic.rs Молим вас да додатке шаљете искључиво у слободним форматима. pgpE76S0d498d.pgp Description: OpenPGP digital signature ___ kallithea-general mailing list kallithea-general@sfconservancy.org https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Re: 500 Internal Server Error with 2-way diff and commit that renames file to its parent directory
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
Re: 500 Internal Server Error with 2-way diff and commit that renames file to its parent directory
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
--
Branko Majic
XMPP: bra...@majic.rs
Please use only Free formats when sending attachments to me.
Бранко Мајић
XMPP: bra...@majic.rs
Молим вас да додатке шаљете искључиво у слободним форматима.
diff --git a/kallithea/controllers/files.py b/kallithea/controllers/files.py
--- a/kallithea/controllers/files.py
+++ b/kallithea/controllers/files.py
@@ -657,9 +657,6 @@
c.changeset_1 = c.db_repo_scm_instance.get_changeset(diff1)
try:
node1 = c.changeset_1.get_node(f_path)
-if node1.is_dir():
-raise NodeError('%s path is a %s not a file'
-% (node1, type(node1)))
except NodeDoesNotExistError:
c.changeset_1 = EmptyChangeset(cs=diff1,
revision=c.changeset_1.revision,
@@ -673,9 +670,6 @@
c.changeset_2 = c.db_repo_scm_instance.get_changeset(diff2)
try:
node2 = c.changeset_2.get_node(f_path)
-if node2.is_dir():
-raise NodeError('%s path is a %s not a file'
-% (node2, type(node2)))
except NodeDoesNotExistError:
c.changeset_2 = EmptyChangeset(cs=diff2,
revision=c.changeset_2.revision,
@@ -684,6 +678,24 @@
else:
c.changeset_2 = EmptyChangeset(repo=c.db_repo_scm_instance)
node2 = FileNode(f_path, '', changeset=c.changeset_2)
+
+if node1.is_dir() and node2.is_dir():
+raise NodeError('%s path is a %s not a file'
+% (node2, type(node2)))
+
+if node1.is_dir():
+c.changeset_1 = EmptyChangeset(cs=diff1,
+ revision=c.changeset_1.revision,
+ repo=c.db_repo_scm_instance)
+node1 = FileNode(f_path, '', changeset=c.changeset_1)
+
+if node2.is_dir():
+c.changeset_2 = EmptyChangeset(cs=diff2,
+ revision=c.changeset_2.revision,
+ repo=c.db_repo_scm_instance)
+node2 = FileNode(f_path, '', changeset=c.changeset_2)
+
+
except ChangesetDoesNotExistError as e:
msg = _('Such revision does not exist for this repository')
webutils.flash(msg, category='error')
pgpTxNQi7Uafp.pgp
Description: OpenPGP digital signature
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
