Skip to content

Conversation

@michael-burke4
Copy link
Contributor

There were a couple changes missing from the transition from syzygy to singularity. This ports over those changes, and slightly improves them.

Copy link
Contributor

@charliemirabile charliemirabile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of bikeshedding about the exception handling that isn't critical given that we are hiding the plain button.

In order to test the current code path, I found that we still have git http transport enabled in cgit (i.e. the mechanism that would normal let you put the same url you are using for viewing the repo in cgit in to git clone) so you can try going to a path like /cgit/singularity/objects/6e/13308d6f7fccfe85231896018f3fde9091ccc5 (that is just the commit object that represents the master branch at time of writing) to trigger the utf decode error (since commit objects are random binary data), and to trigger the missing \n\n code, I just replicated the weird crash with submodules that we were having by making a repo with a submodule and then copying the full contents of .git/modules/whatever_module into /var/git/something in the container and then accessing /cgit/something.

If you want to button down the code even more, you could slap in a commit that adds enable-http-clone=0 to the cgitrc to disable the http transport support (though it is currently useful for testing this error path)

If you don't feel like these suggestions are worth implementing and just want to merge this, I would be ok with that—you can just reply and I will re-review to remove my changes requested.

@michael-burke4 michael-burke4 marked this pull request as draft April 4, 2024 18:35
@michael-burke4 michael-burke4 marked this pull request as ready for review April 9, 2024 20:11
Cgit plain view causes too many issues with our approach
of pasting cgit output into web page. Images
cause errors and raw html is placed into the html doc, so it
is rendered rather than being shown as plain text.
Plain button 404s now, might as well hide it!
Errors can happen, print them and tell the user there was
an internal server error.
This closes a sneaky method Charlie found for
tripping the utf-8 decode error in radius.py.
Http git clone didn't work anyway, so this doesn't,
remove any functionality
Configuring the names, descriptions, etc of repos in cgit
requires a good bit of manual configuring. All of this config
will be centralized in a single cgitrepos file. This file will
be included into the cgitrc, so changes to the list of repos or
their info won't need to touch the cgitrc directly.
orbit now pulls in the cgitrepos file and includes it in cgitrc.
Copy link
Contributor

@charliemirabile charliemirabile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants