Bojangalic/merge stelae and git command #64

BojanG99 · 2024-12-17T10:53:22Z

Description (e.g. "Related to ...", etc.)

Related to #54

Code review checklist (for code reviewer to complete)

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
Title summarizes what is changing
Commit messages are meaningful (see this for details)
Tests have been included and/or updated, as appropriate
Docstrings have been included and/or updated, as appropriate
Standalone docs have been updated accordingly

Automated tests, benchmarks and linters

You can run the tests, lints and benchmarks that are run on CI locally with:

just ci

n-dusan

Thanks for all your work on this! As we discussed after reviewing the code together, we still need to add a couple of tests to cover some edge cases. Also, since there are currently five commits, including one with unresolved conflicts, it’d be great to clean that up.

To keep the history tidy, you can run git rebase -i @~5 and squash the five commits down to two. Once that’s done, we should be in good shape.

n-dusan · 2024-12-18T09:40:41Z

src/server/api/routes.rs

@@ -72,6 +80,10 @@ pub fn register_app<
        )
        .app_data(web::Data::new(state.clone()));

+    app = app
+        .service(web::scope("/_git").service(get_blob))


Suggested change

.service(web::scope("/_git").service(get_blob))

.service(web::scope("/_stelae").service(get_blob))

n-dusan · 2024-12-18T09:41:07Z

src/server/api/routes.rs

+#[route("/{namespace}/{name}", method = "GET", method = "HEAD")]
+#[tracing::instrument(name = "Retrieving a Git blob", skip(path, data, info))]
+#[expect(
+    clippy::future_not_send,
+    reason = "We don't worry about git2-rs not implementing `Send` trait"
+)]
+async fn get_blob(
+    path: web::Path<(String, String)>,
+    info: web::Query<Info>,
+    data: web::Data<AppState>,
+) -> impl Responder {
+    let (namespace, name) = path.into_inner();
+    let info_struct: Info = info.into_inner();
+    let commitish = info_struct.commitish;
+    let remainder = info_struct.remainder.unwrap_or_default();
+    let archive_path = &data.archive_path;
+    let blob = Repo::find_blob(archive_path, &namespace, &name, &remainder, &commitish);
+    let blob_path = clean_path(&remainder);
+    let contenttype = get_contenttype(&blob_path);
+    match blob {
+        Ok(content) => HttpResponse::Ok().insert_header(contenttype).body(content),
+        Err(error) => blob_error_response(&error, &namespace, &name),
+    }
+}
+
+/// A centralised place to match potentially unsafe internal errors to safe user-facing error responses
+#[expect(clippy::wildcard_enum_match_arm, reason = "Allows _ for enum matching")]
+#[tracing::instrument(name = "Error with Git blob request", skip(error, namespace, name))]
+fn blob_error_response(error: &anyhow::Error, namespace: &str, name: &str) -> HttpResponse {
+    tracing::error!("{error}",);
+    if let Some(git_error) = error.downcast_ref::<git2::Error>() {
+        return match git_error.code() {
+            // TODO: check this is the right error
+            ErrorCode::NotFound => {
+                HttpResponse::NotFound().body(format!("repo {namespace}/{name} does not exist"))
+            }
+            _ => HttpResponse::InternalServerError().body("Unexpected Git error"),
+        };
+    }
+    match error {
+        // TODO: Obviously it's better to use custom `Error` types
+        _ if error.to_string() == GIT_REQUEST_NOT_FOUND => {
+            HttpResponse::NotFound().body(HTTPError::NotFound.to_string())
+        }
+        _ => HttpResponse::InternalServerError().body(HTTPError::InternalServerError.to_string()),
+    }
+}


suggestion: move to api/stelae/mod.rs

n-dusan · 2024-12-18T09:41:23Z

src/server/api/routes.rs

+#[derive(Debug, Deserialize)]
+struct Info {
+    /// commit of the repo
+    commitish: String,
+    /// path of the file
+    remainder: Option<String>,
+}


suggestion: api/stelae/requests/mod.rs

n-dusan · 2024-12-18T09:41:51Z

src/server/api/state.rs

+    /// path to the Stelae archive
+    fn archive_path(&self) -> &PathBuf;


suggestion: we don't need this, I think. Can get path from Archive struct

n-dusan · 2025-01-03T19:18:12Z