Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(engine): implement PARSE #82

Merged
merged 22 commits into from
Sep 9, 2022
Merged

feat(engine): implement PARSE #82

merged 22 commits into from
Sep 9, 2022

Conversation

blcklamb
Copy link
Contributor

@blcklamb blcklamb commented Sep 4, 2022

TODO

#29

구현 상세 설명

git log spawn 기능 구현

  • child_process를 활용하여 코드 실행 시 실행한 레포지토리의 console에서 바로 git log를 실행시킬 수 있도록 하였습니다.

    child_process?

  • git log option은 이전 githru 코드를 참고하여 그대로 가져왔습니다.
    git --no-pager log --all --parents --numstat --date-order --pretty=fuller -c

    예시 출력은 다음과 같습니다.

commit a38625748a80945959ca5ff82555a60e8ac51186 9aca1fffa56dc46946f75a6573ed182d7f4c3837 (HEAD -> main, origin/main, origin/HEAD)
Author:     chaejung Kim <whenucan35@gmail.com>
AuthorDate: Sun Sep 4 20:17:59 2022 +0900
Commit:     chaejung Kim <whenucan35@gmail.com>
CommitDate: Sun Sep 4 20:17:59 2022 +0900

    feat(engine): sequenceNumber field add
    
    이후 DAG, STEM 작업을 위해 각 커밋의 순서를 나타내는 필드 추가

1       0       packages/analysis-engine/src/types/CommitRaw.ts

commit 9aca1fffa56dc46946f75a6573ed182d7f4c3837 e333e8cbfe442336c35bd943af03f264d9d28a43
Author:     chaejung Kim <whenucan35@gmail.com>
AuthorDate: Sun Sep 4 20:14:36 2022 +0900
Commit:     chaejung Kim <whenucan35@gmail.com>
CommitDate: Sun Sep 4 20:14:36 2022 +0900

    chore(engine): delete parseIndex.ts
    
    index.ts에 통합하여 기존 별도로 구분된 parse 전용 index는 삭제

0       8       packages/analysis-engine/src/parseIndex.ts

commit e333e8cbfe442336c35bd943af03f264d9d28a43 5e79f2f8af6411021e20f7807b495117c04b1a0c
Author:     chaejung Kim <whenucan35@gmail.com>
AuthorDate: Sun Sep 4 20:11:00 2022 +0900
Commit:     chaejung Kim <whenucan35@gmail.com>
CommitDate: Sun Sep 4 20:11:00 2022 +0900

    refact(engine): erase useless lines

1       1       packages/analysis-engine/src/parseLog.ts

...

commitList Interface 구성

  • packages/analysis-engine/src/types/CommitRaw.ts 참고
  • 이전 회의 때 정한 것에서 DAG 파트를 구현하고 계시는 @ooooorobo, @anpaul0615님께서 요청하신대로 새롭게 sequenceNumber가 추가되었습니다.

git log parse 기능 구현

  • packages/analysis-engine/src/parseLog.ts 참고
  • 로직은 다음과 같습니다.
  1. 위의 예시처럼 terminal에 뜨는 log를 한 줄씩 나눕니다. splitByNewLine

  2. 나눈 것을 한 줄씩 읽으면서 조건문으로 분류하는데, 다음과 같은 경우로 나뉩니다.

    나누면서 카테고리가 동일하게 분류된 것들을 한 데 모을 것입니다.

    2-1) commit으로 시작하는 경우

     새로운 commit의 시작을 의미합니다. 따라서 새로운 
     필수로 자기 자신 commit을 갖고, 경우에 따라 parent commit, branch, tag의 정보를 갖습니다.
    

    2-2) Author:로 시작하는 경우

     Author의 이름과 이메일 정보를 갖습니다.
    

    2-3) AuthorDate로 시작하는 경우

    2-4) Commit:으로 시작하는 경우

    2-5) CommitDate로 시작하는 경우

     CommitDate 다음 '\n' 아래로 commit message 정보를 갖습니다.
    

    2-6) 숫자 또는 '-'로 시작하는 경우

     differenceStatistic과 관련된 정보를 갖습니다.
     `{insertion}      {deletion}     {path}`
    
  3. 카테고리로 분류된 것들에서 동일한 index 값을 갖는 것들끼리 묶어서 JSON화 합니다.

  4. 해당 JSON 값에서 depth 제한을 풀고 return 합니다.
    inspect(JSONArray, { showHidden: false, depth: null, colors: true })

문제 해결 방법

  1. commit message가 여러 줄일 경우

image

한 줄씩 읽어가며 어떤 카테고리인지 구분할 때 commit message의 구분이 모호했습니다.
보통은 title 한 줄이지만, body가 포함된 경우도 종종 있었습니다.
message body에 '\n'이 포함되어 있어서 해당 문자로 구별할 수 없었습니다.

그래서 정확히 어떻게 출력되는지 살펴보았더니,
같은 한 줄이 아니라 commit message를 감싸는 것은 ""으로,
commit message 내의 빈 한 줄은 " "으로 구분되어 있었습니다.
따라서 CommitDate로 부터 mapping 시 이용하는 idx를 가지고 얼만큼의 idx를 갖는지 계산하여 commit message를 parse했습니다.

  1. 출력이 [Object]로 나오는 경우

image

JSON화 한 것을 바로 return 또는 console.log하는 경우 위 처럼 2 depth를 초과한 것에 대해 생략되어 정보가 표시됩니다. 따라서 출력 조건을 제어하기 위해 [node util](https://nodejs.org/api/util.html#util_util_inspect_object_options:~:text=%23-,util.inspect(object%5B%2C%20showHidden%5B%2C%20depth%5B%2C%20colors%5D%5D%5D),-%23) 옵션을 이용하여 해결하였습니다.

예상되는 문제

작동 시간 문제

현재 로직은 commit log를 전부 읽고 그 다음에 commit 수 만큼 또 다시 돌아서 JSON 정보를 만듭니다.
따라서 한 번 더 도는 것을 줄이기 위해 전부 읽으면서 바로 JSON를 만드는 것을 생각해보았는데 commit 간의 구분에서 감이 잡히지 않아,
우선 직관적으로 카테고리화를 선택한 것입니다.

@anpaul0615 님께서 commit 수가 10만 개가 넘어가는 경우 한 번 돌려보라 말씀해주셨는데,

아직 시도를 못 해봤습니다.

혹시 로직 상에 효율성을 높이기 위한 방법이 있다면 코멘트 부탁드리겠습니다.

++추가 문제 발생

terminal에 직접 git --no-pager log --all --parents --numstat --date-order --pretty=fuller -c 실행 시 branch 정보가 뜨는데,

image

spawn한 것으로 실행하면 branch 정보가 뜨지 않습니다.

image

이유는 찾고 있는데.. 일단 branch가 그대로 들어온다면 parse 로직은 문제가 없습니다

tags, author, authorDate, committer, commitDate, messages, differenceStatistics까지 추가하였습니다.
console.log로 찍을 때 2뎁스 이하로 내려가는 부분이 [object]로 표기되어 해당 depth를 설정하는 코드 추가
build 후 바로 git log 명령어와 parse를 실행하도록 수정했습니다.
parseToJSON(value)의 return 값이 CommitRaw(JSON)입니다.
index.ts에 통합하여 기존 별도로 구분된 parse 전용 index는 삭제
이후 DAG, STEM 작업을 위해 각 커밋의 순서를 나타내는 필드 추가
@blcklamb blcklamb self-assigned this Sep 4, 2022
@blcklamb blcklamb requested a review from a team as a code owner September 4, 2022 12:03
@@ -17,6 +17,7 @@ export interface GitUser {
}

export interface CommitRaw {
sequenceNumber: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Number를 명칭에 사용하지 않고, sequenceIndex와 같은 명칭으로 사용하면 좋겠습니다.
위치에 대한 의미이기 때문에 sequence도 괜찮을 것 같습니다.

@@ -0,0 +1,31 @@
import { spawn } from "child_process";

let git = spawn("git", [
Copy link
Contributor

Choose a reason for hiding this comment

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

const로 사용해도 괜찮지 않을까요?
getGitLog함수안에 있어도 괜찮을 것 같습니다.

gitLog += data.toString();
});

git.stderr.on("data", (data) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

"data"보다 error가 의미는 맞지 않을까요?

let indexCheckFileChanged = idx + 2;
let eachCommitMessage = "";
while (true) {
if (splitByNewLine[indexCheckFileChanged] === "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

while에 조건으로 넣으면 while(true)가 eslint에 걸리는 것을 우회할 수 있을 듯 합니다.

authorDates.push(str.split(": ")[1].trim());
} else if (str.slice(0, 7) === "Commit:") {
committers.push({
name: str.split(": ")[1].split("<")[0].trim(),
Copy link
Contributor

Choose a reason for hiding this comment

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

author, comitters 모두 비슷한 함수를 사용하고 있으니, 처리방식에 대한 부분을
getName / getEmail와 같이 함수로 꺼내면 좋을 것 같습니다.

@blcklamb
Copy link
Contributor Author

blcklamb commented Sep 4, 2022

현재 eslint error에 걸린 것들 작업하고 있으니 다시 push 하도록 하겠습니다 죄송합니다

Comment on lines 4 to 7
(async function () {
const value = await getGitLog();
parseToJSON(value);
})();
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 test 코드 인거죠?
따로 getGotLog 관련 test쪽으로 빼서 jest 로 테스트 하도록 만드는게 좋을 것 같습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

index.ts관련한 부분은 제가 작업했으며 관련 사항은
#80 요 PR에 작성해 두었습니다.
따로 설명을 드릴 것을 그랬네요. 혼동을 드리게 되어서 죄송합니다~

Comment on lines 4 to 7
FileChanged,
GitUser,
} from "./types/CommitRaw";
import { inspect } from "util";
Copy link
Contributor

Choose a reason for hiding this comment

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

util은 코드가 어디에 있는지요? 현재 PR에서는 안 보이는 것 같긴합니다.

Copy link
Contributor

@ansrlm ansrlm Sep 5, 2022

Choose a reason for hiding this comment

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

해당 부분은 https://nodejs.org/en/knowledge/getting-started/how-to-use-util-inspect/
내장 라이브러리인 util인데요,
inspect라는 함수는 console.log를 사용할 때 depth가 깊어지게 되면 log를 Object로 찍는 부분을 다 풀어서 보여줄 수 있습니다.

그러나 해당 로그를 찍는 행위는 "실제 실행하는 행위에서는 객체 자체를 return하기 때문에 필요 없겠다" 라는 생각이 드는데요
inspect를 하는 것이 아닌 git log를 parse한 결과에 대한 객체를 반환하는 것으로 함수 변경이 필요할 것 같습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

inspect 좋은것 하나 알아갑니다!
jest를 돌릴 때는 inspect 처럼 다 뿌려주니까 괜찮았는데, 다른 콘솔 디버깅할 때 사용하면 매우 유용하겠네요!

- getNameAndEmail 함수 추가
- eslint 걸리는 것 우회
- parseSpawn에서 오타 수정
- 선언자 수정
- sequenceNumber 이름 수정
@anpaul0615
Copy link
Contributor

anpaul0615 commented Sep 4, 2022

@anpaul0615 님께서 commit 수가 10만 개가 넘어가는 경우 한 번 돌려보라 말씀해주셨는데,
아직 시도를 못 해봤습니다.

혹시 오해가 있을까봐 말씀드리는건데ㅠ
이 부분은 채정님의 작업속도가 워낙 빠르셔서(!) 약간 도전과제(?) 같은 느낌으로 한번 말씀드려본겁니당..ㅠㅠ
(시간 되실때 한번 생각해봐주시면 감사하겠습니다 채정님ㅎㅎ)


혹시 로직 상에 효율성을 높이기 위한 방법이 있다면 코멘트 부탁드리겠습니다.

이 부분에 대해서는 git log 출력을 받아오는 부분과 parsing 부분 각각에 stream 처리를 적용해보시면 어떨까 싶습니다!

} else if (str.slice(0, 7) === "Commit:") {
getNameAndEmail(committers, str);
} else if (str.slice(0, 10) === "CommitDate") {
let indexCheckFileChanged = idx + 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

이러한 영역별 기능들을 따로 함수로 분리해도 좋을 것 같습니다.
STEM쪽 리뷰에서 early return관련해서 의견을 주신 부분이 있는데요,
https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html
해당 부분처럼 변경하는 것은 어떨까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

그러고 보니 전부 str.slice 라면, 전부 startswith로 해도 괜찮겠네요
(startswith로 하면 slice의 length를 지정할 필요가 없어서)

branch 및 tag가 파싱될 때 앞에 공백이 들어가는 것을 제거하기 위해 trim()을 썼습니다.
git log output 중 ref 정보가 terminal에서는 보여지지만 spawn을 했을 때  보여지지 않는 에러를 해결
- slice를 startswith으로 대체했습니다.
- if else 중첩을 줄이고 early return을 적용했습니다.
- JSONArray 선언 코드 위치를 사용 직전으로 옮겼습니다.
merge(engine): --decorate 이슈 해결 및 refactoring
- consistent-return 에러 해결을 위해 return false를 추가
});
}

return inspect(JSONArray, {
Copy link
Contributor

Choose a reason for hiding this comment

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

객체 자체의 변동사항이 없다면 JSONArray만 return하면 되지 않을까 합니다 ㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네 최종 전달에도 문제가 있을까해서 설정해 놓은 건데 디버깅 시 조절하는 옵션인 것이라 파악하고 JSONArray로 변경하겠습니다.
참고 링크

return tags[commitIdx].push(eachInfo.replace("tag: ", "").trim());
return branches[commitIdx].push(eachInfo.replace(")", "").trim());
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

return이 되는 부분이 없는 것 같습니다

JSONArray.push({
sequence: i,
id: ids[i],
parents: parents[i],
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/githru/githru-vscode-ext/pull/82/files#diff-468d48dbac0770a78a608db9978fddce2aaf75666bb518ebee8bcf07a7988702R16
를 참고하면 const 로 사용하는 parents / branches / tags는 2D array인데 비해서 데이터를 조합해서 넣는 영역은 1D array로 보입니다. 그렇기 때문에 cosnt로 사용하는 parents / branches / tags에 대해서 명칭을 변경하거나 어떤 장치를 두어야 할 필요가 있을 것 같습니다. (코드만 보면 단일 스트링이 들어가는 것처럼 보이기 때문)

Copy link
Contributor

@ansrlm ansrlm Sep 7, 2022

Choose a reason for hiding this comment

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

좋은 단어 선택인지는 의문이지만, 단편적으로 생각했을 때는 다음과 같은 방식이 있겠습니다.

type Branches: string[];
const branchesMatrix: Branches[];


// commit별 fileChanged를 분리시키기 위한 임시 index
let commitIdx = -1;
if (typeof splitByNewLine !== "undefined") {
Copy link
Contributor

Choose a reason for hiding this comment

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

궁금해서 질문드리는 부분입니다.

if(splitByNewLine) 가 들어가면 이슈가 발생하나요?

이부분도 early return으로 if(!splitByNewLine) return; 이 들어가도록 처리 가능할 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그러면 git log를 했을 때 아무 값도 출력되지 않는다면, undefined로 반환하는 것이 맞을까요?

그리고 return; 이렇게 early return을 한다면 아래와 같은 eslint error가 뜨는데,

image

이전에는 해당 error를 피하기 위해 forEach문을 돌 때 return false로 하여 넘어가도록 처리하여 딱히 문제가 없었습니다.

그런데 지금 최종 return으로 false를 반환하는 것이 괜찮을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

좋은 말씀이십니다. 이부분은 따로 확인이 필요할 것 같은데요, git log가 없는 케이스가 존재하나요? 만약에 존재한다면 undefined가 아니라 []를 반환하는게 프로그램 동작상 맞을 것 같습니다. 없는 케이스가 존재하지 않는다면 throw error가 맞을 것 같습니다. 에러 처리에 대한 정도에 따라서 말씀주신 것처럼 undefined를 반환할 수도 있을 것 같구요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋은 말씀이십니다. 이부분은 따로 확인이 필요할 것 같은데요, git log가 없는 케이스가 존재하나요? 만약에 존재한다면 undefined가 아니라 []를 반환하는게 프로그램 동작상 맞을 것 같습니다. 없는 케이스가 존재하지 않는다면 throw error가 맞을 것 같습니다. 에러 처리에 대한 정도에 따라서 말씀주신 것처럼 undefined를 반환할 수도 있을 것 같구요.

.git이 아예 존재하지 않을 때를 고려했습니다.
실행 전 예상하기로는 spawn 때 error를 반환할 줄 알았는데
해당 파이프라인으로 가지 않고 ""를 넘기도록 구현되어 있습니다.
문기님 말씀대로 해당 경우, []를 반환하도록 처리한 뒤 얼른 merge하도록 하겠습니다.

let branchAndTagsInfos = splitedCommitLine[1];
if (typeof branchAndTagsInfos !== "undefined") {
if (branchAndTagsInfos.startsWith("HEAD ->")) {
[, branchAndTagsInfos] = branchAndTagsInfos.split("HEAD ->");
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@ansrlm ansrlm left a comment

Choose a reason for hiding this comment

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

LGTM! 고생하셨습니다~

- inspect 출력 형식을 디버깅 때만 사용하여 제거
- 2D array의 경우 의미 파악을 위해 ~Matrix로 변수명을 변경하고 같은 의미를 가진 것에 대한 타입을 추가
parseToJSON("")일 때 early return이 되도록 추가
@blcklamb blcklamb merged commit a865484 into githru:main Sep 9, 2022
@hanseul-lee
Copy link
Contributor

@all-contributors please add @blcklamb for code

@allcontributors
Copy link
Contributor

@hanseul-lee

I've put up a pull request to add @blcklamb! 🎉

@hanseul-lee hanseul-lee added this to the v0.1.0 milestone Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants