Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions packages/vscode/src/test/utils/getRepo.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
import { getRepo } from "../../utils/git.util";

describe("getRepo", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

오우.. 감동입니다 😭 첫 PR부터 Test case 가 포함되어 있네요!!!

describe("Valid Git URL formats", () => {
it("HTTPS URL with .git extension should parse correctly", () => {
const url = "https://github.com/owner/repo.git";
const result = getRepo(url);

expect(result).toEqual({
owner: "owner",
repo: "repo",
});
});

Comment on lines +6 to +14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(사소) Test case에서도 동일한 logic들은 (stateless하다면) 밖으로 빼도 괜찮을 것 같습니다 👍
혹시나 수정하실 예정이면 다음 PR에 해주셔도 좋을 것 같습니다!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

아, URL(input)만 딱 다르고, 다른 부분이 모두 동일하다면 TC의 입력값을 다르게 하는
parameterized test로 바꾸는 것도 괜찮을 것 같습니다!

https://23life.tistory.com/entry/jest-testeach%EB%A5%BC-%ED%86%B5%ED%95%B4-%EC%97%AC%EB%9F%AC-%EC%9E%85%EB%A0%A5%EA%B0%92-%ED%85%8C%EC%8A%A4%ED%8A%B8-%EA%B0%84%EA%B2%B0%ED%95%98%EA%B2%8C-%EA%B5%AC%ED%98%84%ED%95%98%EA%B8%B0

해당 건 이슈로 등록해주셔도 좋을 것 같습니다!
(다른 tc들도 이런 것 있는지 찾아보는 것도 좋을 것 같습니다)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

의견 너무 감사드립니다!!!

it.each 사용하려고 했는데, 현재 vscode 프로젝트에서 테스트 라이브러리로 mocha, jest 두개가 사용되고 있어 *.spec.ts 가 mocha 로 타입추론이 되네요ㅠㅠ

vscode extention 에서는 mocha를 공식적으로 권장하고 있는데 저희 프로젝트에서는 mocha가 설정만 되어있고 샘플 테스트 코드만 구현되어있는 상태라 어느쪽으로 통일해야할지 고민이 됩니다!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

그러고보니 vscode 쪽은 거의 tc 가 없긴하네요.
getGitLog.spec.ts 하나 밖에 없으니

*.spec.ts를 mocha 안쓰고 jest로 변경해주셔도 괜찮습니다!!!
다음 PR에서 진행하시지요 : )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

넵! mocha 걷어내는 작업 따로 Issue 파서 진행하도록 하겠습니다ㅎㅎ

it("SSH URL with .git extension should parse correctly", () => {
const url = "git@github.com:owner/repo.git";
const result = getRepo(url);

expect(result).toEqual({
owner: "owner",
repo: "repo",
});
});

it("HTTPS URL without .git extension should parse correctly", () => {
const url = "https://github.com/owner/repo";
const result = getRepo(url);

expect(result).toEqual({
owner: "owner",
repo: "repo",
});
});

it("SSH URL without .git extension should parse correctly", () => {
const url = "git@github.com:owner/repo";
const result = getRepo(url);

expect(result).toEqual({
owner: "owner",
repo: "repo",
});
});

it("HTTPS URL with username should parse correctly", () => {
const url = "https://username@github.com/owner/repo.git";
const result = getRepo(url);

expect(result).toEqual({
owner: "owner",
repo: "repo",
});
});

it("URL with trailing slash should parse correctly", () => {
const url = "https://github.com/owner/repo/";
const result = getRepo(url);

expect(result).toEqual({
owner: "owner",
repo: "repo",
});
});

it("Complex repo name with hyphens should parse correctly", () => {
const url = "https://github.com/githru/githru-vscode-ext.git";
const result = getRepo(url);

expect(result).toEqual({
owner: "githru",
repo: "githru-vscode-ext",
});
});

it("Azure DevOps URL should parse correctly", () => {
const url = "https://organization@dev.azure.com/organization/organization-sub-name/_git/repository-name";
const result = getRepo(url);

expect(result).toEqual({
owner: "organization",
repo: "repository-name",
});
});

it("Azure DevOps URL without username should parse correctly", () => {
const url = "https://dev.azure.com/organization/organization-sub-name/_git/repository-name";
Comment on lines +75 to +86

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

저희는 사내에서 github.sec.**.net을 쓰고 있습니다 😸

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

github에 .net으로 끝나는 도메인도 있는지는 몰랐네요ㅎㅎ 정책이 정해진다면 해당 케이스도 추가하도록 하겠습니다!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

samsung.net 입니다 👿

const result = getRepo(url);

expect(result).toEqual({
owner: "organization",
repo: "repository-name",
});
});
});

describe("Invalid Git URL formats", () => {
it("should throw error for invalid URL format", () => {
const invalidUrl = "invalid-url-format";

expect(() => getRepo(invalidUrl)).toThrow(
'Invalid Git remote config format: "invalid-url-format". Expected format: [https?://|git@]github.com/owner/repo[.git] or https://organization@dev.azure.com/organization/project/_git/repository-name'
Comment on lines +96 to +101

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Negative case 까지!! 완벽하네요!

);
});

it("should throw error for non-GitHub URL", () => {
const nonGithubUrl = "https://gitlab.com/owner/repo.git";

expect(() => getRepo(nonGithubUrl)).toThrow(
'Invalid Git remote config format: "https://gitlab.com/owner/repo.git". Expected format: [https?://|git@]github.com/owner/repo[.git] or https://organization@dev.azure.com/organization/project/_git/repository-name'
);
});

it("should throw error for URL without owner/repo", () => {
const incompleteUrl = "https://github.com/";

expect(() => getRepo(incompleteUrl)).toThrow(
'Invalid Git remote config format: "https://github.com/". Expected format: [https?://|git@]github.com/owner/repo[.git] or https://organization@dev.azure.com/organization/project/_git/repository-name'
);
});
});
});
32 changes: 23 additions & 9 deletions packages/vscode/src/utils/git.util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@
if (await isExecutable(file)) {
try {
return await getGitExecutable(file);
} catch (_) {}

Check warning on line 116 in packages/vscode/src/utils/git.util.ts

View workflow job for this annotation

GitHub Actions / build (20.x)

Empty block statement
}
}
return Promise.reject<GitExecutable>();
Expand Down Expand Up @@ -149,7 +149,7 @@
for (let i = 0; i < paths.length; i++) {
try {
return await getGitExecutable(paths[i]);
} catch (_) {}

Check warning on line 152 in packages/vscode/src/utils/git.util.ts

View workflow job for this annotation

GitHub Actions / build (20.x)

Empty block statement
}
throw new Error("None of the provided paths are a Git executable");
}
Expand Down Expand Up @@ -288,19 +288,33 @@
}

export const getRepo = (gitRemoteConfig: string) => {
const gitRemoteConfigPattern =
/(?:https?|git)(?::\/\/(?:\w+@)?|@)(?:github\.com)(?:\/|:)(?:(?<owner>[^/]+?)\/(?<repo>[^/.]+))(?:\.git|\/)?(\S*)$/m;
const gitRemote = gitRemoteConfig.match(gitRemoteConfigPattern)?.groups;
if (!gitRemote) {
throw new Error("git remote config should be: [https?://|git@]${domain}/${owner}/${repo}.git");
const gitHubPattern =
/(?:https?|git)(?::\/\/(?:\w+@)?|@)(?:github\.com)(?:\/|:)(?:(?<owner>[^/]+?)\/(?<repo>[^/]+?))(?:\.git|\/)?$/m;
const azureDevOpsPattern =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(사소, 추후에..) azure 말고 다른 형태의 url이 들어오면 어떻게 대응하면 좋을까요?
모든 케이스에 대해서 이렇게 추가할 수는 없을 것 같긴 하네요!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

정책적으로도 좀 얘기해봐도 좋을 것 같긴 합니다 : )
알려진(known source) public repo만 한다던지... 하는 정책을 세울 수도 있으니!

@SingTheCode SingTheCode Jul 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

domain, owner, repo가 remote url에 할당되어있는 방법이 저마다 달라서 말씀주신대로 public repo에 한해서만 대응하면 좋을 것 같습니다!

정책적으로 정해진다면 사용자에게 다음 액션을 유도하는 것도 사용성을 높일 수 있는 방법이라고 생각됩니다ㅎㅎ (OO, OO repo만 지원됩니다. 또는 현재는 지원하지 않는 형식의 레포지토리입니다. 메일로 현재 레포의 주소를 알려주세요 (메일 전송 버튼 클릭)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

domain, owner, repo가 remote url에 할당되어있는 방법이 저마다 달라서 말씀주신대로 public repo에 한해서만 대응하면 좋을 것 같습니다!

정책적으로 정해진다면 사용자에게 다음 액션을 유도하는 것도 사용성을 높일 수 있는 방법이라고 생각됩니다ㅎㅎ (OO, OO repo만 지원됩니다. 또는 현재는 지원하지 않는 형식의 레포지토리입니다. 메일로 현재 레포의 주소를 알려주세요 (메일 전송 버튼 클릭)

넵 이 내용에 대해서는 정책이나 지식 정도로 지정해서, 다른 contributor들도 알 수 있게 하면 좋을 것 같습니다.
조금 더 다른 분들과 토론 해봐도 좋을 것 같아서
위 내용 그대로 새로 이슈로 만들어주시고 discussion 라벨 달아주시면 어떨까요?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

이슈 생성해서 공유하겠습니다! 감사합니다ㅎㅎ

/https:\/\/(?:\w+@)?dev\.azure\.com\/(?<owner>[^/]+?)\/(?<project>[^/]+?)\/_git\/(?<repo>[^/]+?)(?:\/)?$/m;
const patterns = [gitHubPattern, azureDevOpsPattern];

let gitRemote: { owner: string; repo: string } | null = null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(궁금) null 을 굳이 넣으신 이유는 무엇일까용?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

gitRemote 의 타입을 { owner: string; repo: string } 로만 설정하고 초기값을 설정하지 않거나 { owner: '', repo: '' }으로 설정할 수 있지만, gitRemote가 정상적인 플로우로 값이 할당되지 않았을 때 비어있다라는 것을 명시하는 것이 목적입니다!

에러 발생 조건으로 !owner.length || !repo.length 를 넣을 수도 있지만 !gitRemote가 더 명시적인 코드라고 생각했습니다ㅎㅎ

위 내용은 의견 주시는대로 변경해도 괜찮습니다ㅎㅎ

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

오. 넵. 그렇군요
(궁금) 혹시 이 경우에 flow의 에러로 undefined가 아닌 null이 할당되는 경우가 있을까요?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

gitRemote 의 변수에 값을 할당하지 않는(undefined) 상황을 가정해서 말씀하시는게 맞을까요??
제가 봤을 때는 없어보입니다! 혹시 그럴 수 있는 상황이 있으면 알려주시면 감사하겠습니다ㅎㅎ

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

gitRemote 의 변수에 값을 할당하지 않는(undefined) 상황을 가정해서 말씀하시는게 맞을까요?? 제가 봤을 때는 없어보입니다! 혹시 그럴 수 있는 상황이 있으면 알려주시면 감사하겠습니다ㅎㅎ

아 특별히 null과 undefined를 엄격하게 나누는 기준이 있을까 해서 여쭤봤습니다 : )
스타일들이 다 다르니까요 ㅎㅎ


for (const pattern of patterns) {
const match = gitRemoteConfig.match(pattern);
if (!match?.groups) continue;

const { owner, repo } = match.groups;
if (owner && repo) {
const repoWithoutGit = repo.replace(/\.git$/, "");
gitRemote = { owner, repo: repoWithoutGit };
break;
}
}

const { owner, repo } = gitRemote;
if (!owner || !repo) {
throw new Error("no owner/repo");
if (!gitRemote) {
throw new Error(
`Invalid Git remote config format: "${gitRemoteConfig}". Expected format: [https?://|git@]github.com/owner/repo[.git] or https://organization@dev.azure.com/organization/project/_git/repository-name`
);
}

return { owner, repo };
return gitRemote;
};

export async function getBranches(
Expand Down
Loading