Skip to content

Conversation

@reddevilmidzy
Copy link
Member

@reddevilmidzy reddevilmidzy commented Jul 22, 2025

구현한 기능

  • 문제 생성시 uploads/{problem_id} 하위에 버전관리를 위한 .git 디렉터리와 "solutions/accepted", "solutions/rejected", "tests", "statements" 디렉터리 생성
  • git add & git commit 기능 구현
  • git log 조회 기능 구현

아직 스켈레톤 코드라 구현한 기능을 호출하는 코드는 작성하지 않았고 (그래서 #![allow(dead_code)] 붙여두었습니다), 우선 이 부분 먼저 리뷰 받고 싶어서 PR 올립니다.

part of #40

TODO

@reddevilmidzy reddevilmidzy requested a review from Copilot July 22, 2025 15:01

This comment was marked as outdated.

@w8385
Copy link
Member

w8385 commented Jul 22, 2025

solutions 디렉터리 하위에 accepted, rejected 서브 디렉터리는 verdict 구분을 위해 별도로 생성하는 건가요?

Comment on lines +235 to +232
async fn can_create_default_file() -> Result<(), std::io::Error> {
let problem_id = 12;
let git_manager = GitManager::new(problem_id);
Copy link
Member Author

Choose a reason for hiding this comment

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

현재 테스트 방식에는 이런 문제점이 있습니다.

  • 테스트가 종료시에 직접 remove_dir_all 수행
    만약 도중에 테스트가 실패하면 파일은 지워지지 않는다는 문제가 있습니다. 이 문제는 https://docs.rs/tempdir/latest/tempdir/ 이 crate를 사용한다면 해결할 수 있을 거 같습니다. (scope 벗어나면 drop 시에 디렉터리 삭제 수행)

  • 각 테스트마다 직접 problem_id를 부여하여 테스트를 진행
    이는 테스트가 동시에 실행되어도 각각에는 영향을 주지 않게 하기 위함이였는데 일일히 번호를 부여하는 방식이라 굉장히 비효율적입니다. 이렇게 구현한 이유는 아래 문제와 연관되어 있는데, production에서 사용하는 problem_id를 피하기 위함이였습니다.

  • production이랑 test 환경에서의 UPLOAD_DIR이 동일
    production에 test에서 사용하는 problem_id와 동일한 problem_id가 있다면 삭제될 위험이 있습니다.
    그래서 환경을 분리해야할 필요성을 느꼈는데, 이 작업을 해당 PR에 한번에 올라가면 굉장히 빵빵한 PR이 될 것 같아 일단 이렇게 구현하였습니다. 만약 환경이 분리된다면 각 테스트마다 직접 problem_id를 부여하여 테스트를 진행하는 방식도 그냥 랜덤으로 부여하여 해결할 수 있을 듯 합니다.

Copy link
Member

Choose a reason for hiding this comment

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

문제점은 다음 PR에서 개선하나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

넵! 환경 분리까지 진행하면 변경사항이 커질 것 같아 다음 PR에서 작업하려 합니다!

@reddevilmidzy
Copy link
Member Author

solutions 디렉터리 하위에 accepted, rejected 서브 디렉터리는 verdict 구분을 위해 별도로 생성하는 건가요?

네네 맞아요 polygon 보고 취할건 취하고 버릴건 버리긴 했는데, 현상태에서는 구분할 필요가 없어보이긴 하네요, 서브 디렉터리 없애도록 하겠습니다!

@reddevilmidzy reddevilmidzy force-pushed the 40-revision-check branch 2 times, most recently from 2c01286 to 9ed8dbc Compare July 22, 2025 15:35
@reddevilmidzy
Copy link
Member Author

ci 실행을 위한 force-push가 있었습니다
40-revisionmain으로 가기 전에 ce1547a 이 커밋은 드랍하고 갈게요

@utilForever
Copy link
Member

현재 CI가 실패하고 있는데 이건 이 PR에서 해결하나요?

@reddevilmidzy
Copy link
Member Author

현재 CI가 실패하고 있는데 이건 이 PR에서 해결하나요?

넵! sign이 안된 상태에서 커밋하려고 해서 실패하는거 같은데 이 PR에서 해결하고 다시 요청하겠습니다..!

@reddevilmidzy reddevilmidzy requested a review from Copilot July 25, 2025 14:57
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements Git-based version control for problem files, providing the foundation for tracking changes to problem data through standard Git operations.

  • Adds Git repository initialization and basic Git operations (add, commit, status, log) for problem files
  • Creates standardized directory structure for each problem with solutions, tests, and statements folders
  • Provides change tracking functionality through Git commit history and file status monitoring

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/file_manager/mod.rs Adds git module to the file manager
src/file_manager/git.rs Implements GitManager with Git operations and comprehensive tests
Cargo.toml Adds git2 dependency and moves reqwest to dev-dependencies
Comments suppressed due to low confidence (1)

src/file_manager/git.rs:232

  • The test cleanup in line 235 may fail if the directory doesn't exist, potentially causing test failures. Consider using remove_dir_all only if the path exists.
        assert!(Path::new(format!("{UPLOAD_DIR}/{problem_id}").as_str()).exists());

@reddevilmidzy
Copy link
Member Author

@utilForever @w8385 이 PR에서 추가로 수정할 부분이 없다면 merge하고 TODO에 작성한 기능들 구현하겠습니다!

Copy link
Member

@utilForever utilForever left a comment

Choose a reason for hiding this comment

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

다음 PR 올리실 때 한글 주석은 영어로 전부 바꿔주세요.

@reddevilmidzy reddevilmidzy merged commit 98d90b2 into 40-revision Aug 2, 2025
4 checks passed
@reddevilmidzy reddevilmidzy deleted the 40-revision-check branch August 2, 2025 16:09
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.

4 participants