Skip to content

[rush] Properly handle Git worktrees - #994

Merged
Pete Gonzalez (octogonz) merged 2 commits into
microsoft:masterfrom
ecraig12345:git-worktrees
Dec 13, 2018
Merged

Pete Gonzalez (octogonz) merged 2 commits into
microsoft:masterfrom
ecraig12345:git-worktrees

Conversation

@ecraig12345

Copy link
Copy Markdown
Member

When copying Git hooks in a Git working directory which is a worktree, the destination should be under the Git metadata directory for the current worktree. Copying into path.join(repoInfo.worktreeGitDir, 'hooks') (repoInfo is returned by git-repo-info) will give the correct results for both worktrees and normal working directories.

For example, if my main copy of a Git repo is at D:\git\fabric-react and the worktree I'm building in is at D:\git\fabric-react2, the hooks should get copied into D:\git\fabric-react\.git\worktrees\fabric-react2\hooks (not D:\git\fabric-react2\.git\hooks as it was previously attempting).

Updating git-repo-info to a new version which supports worktrees also makes Git email policy work in worktrees. I added typings to the latest git-repo-info version too, so the custom typings can be removed.

(Fixes #990)

}
} catch (ex) {
// ignore
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What kind of exceptions are we expecting to catch and ignore here?

If this throws and catches an exception as part of a normal expected state that is not an error (an "unexceptional exception"), is there a test we can add that would avoid that? (Unexceptional exceptions make debugging harder, because when you try to use the "break on exceptions" feature it constantly stops at exceptions that don't reflect any actual problem.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a comment and changed the try/catch to wrap only the most relevant part (which actually is very unlikely to throw)

Comment thread apps/rush-lib/src/api/ChangeFile.ts Outdated
let branch: string | undefined = undefined;
try {
branch = gitInfo().branch;
branch = Git.getGitInfo()!.branch;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

!.branch [](start = 31, length = 8)

How do we know that getGitInfo() doesn't return undefined here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It could, but the try/catch would catch it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually I'll remove the try/catch and do an if instead

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Agreed. A null reference exception is always a program malfunction. We would never intentionally catch this sort of exception.


In reply to: 241253975 [](ancestors = 241253975)

@octogonz Pete Gonzalez (octogonz) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

:shipit:

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.

2 participants