mirror of
https://github.com/johndoe6345789/WizardMerge.git
synced 2026-04-24 13:44:55 +00:00
Address code review feedback
- Add sys/wait.h include for WEXITSTATUS macro - Check config command results before commit - Escape commit messages to prevent injection - Fix potential npos overflow in string trimming - Use std::filesystem::temp_directory_path() for portability - Fix base branch parameter issue (clone already at base_ref) - All tests still pass (17/17) Co-authored-by: johndoe6345789 <224850594+johndoe6345789@users.noreply.github.com>
This commit is contained in:
@@ -195,8 +195,9 @@ void PRController::resolvePR(
|
||||
response["note"] = "Git CLI not available - branch creation skipped";
|
||||
} else {
|
||||
// Clone repository to temporary location
|
||||
std::string temp_dir = "/tmp/wizardmerge_pr_" + std::to_string(pr_number) + "_" +
|
||||
std::to_string(std::time(nullptr));
|
||||
std::filesystem::path temp_base = std::filesystem::temp_directory_path();
|
||||
std::string temp_dir = (temp_base / ("wizardmerge_pr_" + std::to_string(pr_number) + "_" +
|
||||
std::to_string(std::time(nullptr)))).string();
|
||||
|
||||
// Build repository URL
|
||||
std::string repo_url;
|
||||
@@ -216,8 +217,8 @@ void PRController::resolvePR(
|
||||
if (!clone_result.success) {
|
||||
response["note"] = "Failed to clone repository: " + clone_result.error;
|
||||
} else {
|
||||
// Create new branch
|
||||
auto branch_result = create_branch(temp_dir, branch_name, pr.base_ref);
|
||||
// Create new branch (without base_branch parameter since we cloned from base_ref)
|
||||
auto branch_result = create_branch(temp_dir, branch_name);
|
||||
|
||||
if (!branch_result.success) {
|
||||
response["note"] = "Failed to create branch: " + branch_result.error;
|
||||
|
||||
@@ -9,6 +9,7 @@
|
||||
#include <sstream>
|
||||
#include <iostream>
|
||||
#include <filesystem>
|
||||
#include <sys/wait.h>
|
||||
|
||||
namespace wizardmerge {
|
||||
namespace git {
|
||||
@@ -141,13 +142,36 @@ GitResult commit(
|
||||
) {
|
||||
// Set user config if provided
|
||||
if (!config.user_name.empty() && !config.user_email.empty()) {
|
||||
execute_command(git_command(repo_path,
|
||||
auto name_result = execute_command(git_command(repo_path,
|
||||
"config user.name \"" + config.user_name + "\""));
|
||||
execute_command(git_command(repo_path,
|
||||
if (!name_result.success) {
|
||||
GitResult result;
|
||||
result.success = false;
|
||||
result.error = "Failed to set user.name: " + name_result.error;
|
||||
result.exit_code = name_result.exit_code;
|
||||
return result;
|
||||
}
|
||||
|
||||
auto email_result = execute_command(git_command(repo_path,
|
||||
"config user.email \"" + config.user_email + "\""));
|
||||
if (!email_result.success) {
|
||||
GitResult result;
|
||||
result.success = false;
|
||||
result.error = "Failed to set user.email: " + email_result.error;
|
||||
result.exit_code = email_result.exit_code;
|
||||
return result;
|
||||
}
|
||||
}
|
||||
|
||||
std::string cmd = "commit -m \"" + message + "\"";
|
||||
// Escape commit message for shell
|
||||
std::string escaped_message = message;
|
||||
size_t pos = 0;
|
||||
while ((pos = escaped_message.find("\"", pos)) != std::string::npos) {
|
||||
escaped_message.replace(pos, 1, "\\\"");
|
||||
pos += 2;
|
||||
}
|
||||
|
||||
std::string cmd = "commit -m \"" + escaped_message + "\"";
|
||||
return execute_command(git_command(repo_path, cmd));
|
||||
}
|
||||
|
||||
@@ -190,7 +214,14 @@ std::optional<std::string> get_current_branch(const std::string& repo_path) {
|
||||
|
||||
// Trim whitespace
|
||||
std::string branch = result.output;
|
||||
branch.erase(branch.find_last_not_of(" \n\r\t") + 1);
|
||||
size_t last_non_ws = branch.find_last_not_of(" \n\r\t");
|
||||
|
||||
if (last_non_ws == std::string::npos) {
|
||||
// String contains only whitespace
|
||||
return std::nullopt;
|
||||
}
|
||||
|
||||
branch.erase(last_non_ws + 1);
|
||||
|
||||
return branch;
|
||||
}
|
||||
|
||||
@@ -16,8 +16,9 @@ protected:
|
||||
std::string test_dir;
|
||||
|
||||
void SetUp() override {
|
||||
// Create temporary test directory
|
||||
test_dir = "/tmp/wizardmerge_git_test_" + std::to_string(time(nullptr));
|
||||
// Create temporary test directory using std::filesystem
|
||||
std::filesystem::path temp_base = std::filesystem::temp_directory_path();
|
||||
test_dir = (temp_base / ("wizardmerge_git_test_" + std::to_string(time(nullptr)))).string();
|
||||
fs::create_directories(test_dir);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user