500 Internal Server Error with 2-way diff and commit that renames file to its parent directory

[Mads Kiilerich]

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 at sfconservancy.org
> https://lists.sfconservancy.org/mailman/listinfo/kallithea-general