Skip to content

Comments

Git tool#4238

Closed
psachdeva-ms wants to merge 3 commits intomicrosoft:mainfrom
psachdeva-ms:git_tool
Closed

Git tool#4238
psachdeva-ms wants to merge 3 commits intomicrosoft:mainfrom
psachdeva-ms:git_tool

Conversation

@psachdeva-ms
Copy link
Collaborator

This adds the functionality in the git tool to add new remotes and create worktrees from existing repositories.
This helps in saving time when repeated kernel builds happen with minor changes.

Signed-off-by: Piyush Sachdeva <psachdeva@microsoft.com>
Signed-off-by: Piyush Sachdeva <s.piyush1024@gmail.com>
Signed-off-by: Piyush Sachdeva <psachdeva@microsoft.com>
Signed-off-by: Piyush Sachdeva <s.piyush1024@gmail.com>
Signed-off-by: Piyush Sachdeva <psachdeva@microsoft.com>
Signed-off-by: Piyush Sachdeva <s.piyush1024@gmail.com>
Copy link
Contributor

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 adds git remote management and worktree functionality to the Git tool class, enabling operations such as adding/removing remotes, creating/managing worktrees, and fetching from specific remotes. These features are designed to optimize repeated kernel builds by allowing multiple working directories from a single repository.

Changes:

  • Enhanced the fetch method to support fetching from specific remotes
  • Added remote management methods (list, exists, add, remove, set-url, get-url) for managing git remotes
  • Added worktree management methods (add, list, remove, prune, exists, is_branch_checked_out) for creating and managing git worktrees

Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

Trailing space in the command string may result in unexpected formatting. The command is built as "worktree add ... {path} " with a trailing space, which could cause issues or look inconsistent in logs. Consider removing the trailing space or ensuring it's intentional for the git command syntax.

Suggested change
cmd += f" {path} "
cmd += f" {path}"

Copilot uses AI. Check for mistakes.
Comment on lines +407 to +409
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The error message "Even the origin remote doesn't exist?" is misleading and doesn't match the context. This method lists all remotes and can succeed even with no remotes (empty output), so the assertion should not assume that the origin remote must exist. Consider removing this redundant assertion or updating the message to be more accurate.

Suggested change
result.assert_exit_code(
message=f"Even the origin remote doesn't exist?", include_output=True
)

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

Inconsistent use of logging flags. The method uses no_info_log=False while most other methods in this class use no_info_log=True for similar git remote operations (see remote_add, remote_remove, remote_get_url). For consistency, this should likely be no_info_log=True.

Suggested change
no_info_log=False,
no_info_log=True,

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

This commented-out code should either be removed or implemented. The comment suggests an alternative approach to handle branch name conflicts by appending the RUN_ID, but the current implementation simply detaches instead. Either remove this line if detaching is the intended behavior, or implement the commented logic if that's the desired approach.

Suggested change
# new_branch = f"{new_branch}_{constants.RUN_ID}"

Copilot uses AI. Check for mistakes.
Comment on lines +624 to +626
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

Potential KeyError if a worktree doesn't have a "path" key. The worktree_list parsing logic can create worktrees with missing keys depending on the git output. Add proper error handling or ensure all worktrees have a "path" key before accessing it. Consider using wt.get("path") instead.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

Potential KeyError if a worktree doesn't have a "branch" key. The worktree_list parsing logic may create worktrees with missing keys depending on the git output format. Add proper error handling or ensure all worktrees have a "branch" key before accessing it. Consider using wt.get("branch", "") and handling the empty case appropriately.

Suggested change
wt_branch = wt["branch"]
wt_branch = wt.get("branch")
if not wt_branch:
continue

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

Variable detach is not used.

Suggested change
detach = True

Copilot uses AI. Check for mistakes.
)
result.assert_exit_code(
message=f"Even the origin remote doesn't exist?", include_output=True
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code will never be executed since previous step already contains the judgement of expected_exit_code

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