Skip to content

Conversation

@Mikachu2333
Copy link
Collaborator

问题描述

  1. 在win环境下,无法取到正确的documents目录
  2. 原有的path函数用的是str拼接,也没有校验和规范化,很多地方需要靠人力更正校验
  3. fix Windows上获取 “系统 Documents” 目录的方式有问题 #245

方案与实现

  1. 改进PR的actions文件,现在可以正确测试了
  2. 使用 <sys/stat.h> -> stat() (Linux) or <shlobj.h> -> GetFileAttributesA() (Windows) 来校验文件夹存在与否
  3. 修正原有的 xy_str_strip 函数在特殊情况下可能导致悬空指针的问题
  4. 重写获取win环境下pwsh配置文件的方法,使用了 <shlobj.h> 定义的 SHGetFolderPathA() 函数
  5. 增加路径规范函数 xy_normalize_path ():去除两端引号、规范化(统一成 / 而非 \\,避免转义问题)、加引号
  6. 所有的路径处理现在都需要先经过 xy_path_canonicalize() 规范再进一步处理
  7. 移除单、双引号与增加单引号也抽象成了函数方便调用(移除引号的逻辑是故意的,用于应对奇怪的情景)
  8. 增加形式类似字符串的路径拼接、多路径拼接
  9. 针对linux的 ~/xxx 目录做了特殊处理,由函数 xy_normalize_path() 负责
  10. 更新 xy_parent_dir() ,因为此前已规范路径所以不需要分情况讨论了
  11. 增加了一些测试内容
  12. actions降低build频率,只有在第一次创建pr、草稿转正式、reopen、request review时自动构建,提供手动构建功能(这也是其他项目的通常做法)

其他

测试通过校验:https://github.com/Mikachu2333/chsrc/actions/runs/16813216314

Comment on lines +1088 to +1105
if (!path) return NULL;

if (xy_on_windows)
char *new = xy_str_strip (path);

// 处理 ~ 或 ~/ 开头的路径
if (xy_streql (new, "~"))
{
if (xy_str_start_with (new, "~/"))
{
// 或 %USERPROFILE%
new = xy_strjoin (3, xy_os_home, "\\",
xy_str_delete_prefix (new, "~/"));
}
new = xy_str_gsub (new, "/", "\\");
// 单独的 ~ 直接替换为 home 目录
free (new);
new = xy_strdup (xy_os_home);
}
else
else if (xy_str_start_with (new, "~/"))
{
if (xy_str_start_with (new, "~/"))
{
new = xy_strjoin (3, xy_os_home, "/",
xy_str_delete_prefix (new, "~/"));
}
// ~/ 开头的路径
char *relative_part = xy_str_delete_prefix (new, "~/");
char *home_path = xy_2pathjoin (xy_os_home, relative_part);
free (new);
new = home_path;
Copy link
Contributor

Choose a reason for hiding this comment

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

xy_normalize_path() 已经用在了 chsrc 多处地方,其要义并非只是如注释的这两条,实际上,它还担负着:维护者用类似 UNIX 路径 / 的写法时,在 Windows 中依然返回出来的是类似 Windows 的路径 \\

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

我专门为win换用/的原因就是\\的转义问题在win的部分特殊情况下会被多次转义导致实际未传参成功。

例如在c语言中的\\会被转义为\,而在一些较古早的win软件上,它作为命令行参数传递时其又会被win二次转义,变成无意义的单个转义符号,进而导致整条命令失效,所以在部分软件会在编程时使用\\\\这么个邪门玩意避免问题的发生……(这当然太蠢了)

但是,/就没这么多事……

* 4. 删除路径开头和结尾的多余斜杠(除根路径外)
* 5. 输出时两边加单引号
*/
static char *
Copy link
Contributor

Choose a reason for hiding this comment

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

添加了这个函数,相当于现在同时有:

  1. xy_normalize_path()
  2. xy_path_canonicalize()

两个函数的名称本身已经无法区分意义,而且就注释目的来看,该新增函数目的也是尝试进行规范化,两者功能开始竞争,应该合并为同一个函数。

canonical 一词的含义是 “标准化/公认权威化”,但是却把所有 \\ 转换为 /,这相当于以 UNIX 路径为权威化。我认为,这可能是你想要引入一种中间表示,用 UNIX 路径统领所有项目中的路径表示方法。但是我在另一条评论中已经提到,Windows 上,需要用 Windows 表示法。

最后,输出时两边加单引号,我能够理解,这在放在命令行场景时是有用的,然而这个和函数本身的语义无关,路径就应该是路径本身,而不是添加了额外的引号。额外添加引号的需求,不应该由这个函数完成,而由调用方自己完成。

if (!path) return false;

char *normalized_path = xy_normalize_path (path);
if (!normalized_path) 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.

xy_normalize_path() 按照现在的更改,当 path 不为 NULL 的时候,不可能为 NULL

怎么需要又判断一次NULL呢?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

因为我不确定该函数如果后续被更改时是否会出问题所以提前到处都塞了NULL(感觉很需要panic info这种东西……)

if (!normalized_path) return false;

// 去除可能存在的引号
char *clean_path = xy_str_remove_quotes (normalized_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

xy_normalize_path() 按照现在的更改,不可能添加引号。

所以你会看到,如果你在 xy_path_canonicalize() 添加了引号,而又不知道二者调用关系的时候,总是需要到处添加删除引号的逻辑。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

确实如此,这样命令行传参时有个大好处,不用担心生成的路径会因为其中包含空格而失效,并且也不用担心忘记了手动加引号。

不过现在看了确实是太过于多此一举,我应该在命令行的路径处直接使用加引号的函数,在canonicalize里面返回正常路径的

@ccmywish
Copy link
Contributor

ccmywish commented Aug 8, 2025

为了解决 #245 不需要这么多提交

所以,显然这是可以拆分的。

只满足了 test/ 里的测试是不够的,因为很多 recipe 直接用了 xy 里的函数,你还需要看调用者现在是否依然能工作。

@ccmywish
Copy link
Contributor

ccmywish commented Aug 8, 2025

关于路径:

  1. normalize 规范化(删除非法表示等处理)
  2. 接受维护者在 Windows 上用UNIX 法表示
  3. 不同平台下应不同表示
  4. 内部处理的时候又应该如何表示

需要多方面考虑后给出一个系统性方案,经过讨论后再实现可能更合适。

@ccmywish
Copy link
Contributor

ccmywish commented Aug 8, 2025

@Mikachu2333

我理解你的热情,这个 PR 里有部分代码是很好的。但是你需要考虑到持续的可维护性,搭配 AI 编程有的时候只能给出一种 “看起来能工作” 的方案。

在这种波及范围相当大的修改,应当慎之又慎。最好的PR实践应该是:仅在能确定的范围内进行小程度修改,比如,本来目标是A,但是发现可以通过重构(假设称为B),B的出现可以修改/简化A的实现方法。此时你会想要把A和B合并在同一个PR中。但是,你没有办法确认B是一定合适的、会被接受的。此时应该放弃B的想法,只提交实现A,只有A被接受以后,再尝试进行B。

@ccmywish ccmywish requested a review from Copilot August 8, 2025 01:55
Copy link

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 improves path handling and retrieval across Windows and Unix systems. It addresses issues with obtaining the correct documents directory on Windows, replaces string concatenation with proper path handling functions, and adds comprehensive path normalization.

Key changes include:

  • Replaced string-based path operations with standardized path handling functions
  • Added Windows-specific document directory retrieval using SHGetFolderPathA()
  • Implemented comprehensive path normalization and canonicalization functions
  • Fixed memory management issues in the xy_str_strip function

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
lib/xy.h Major rewrite of path handling functions, added Windows API integration, improved memory management
test/xy.c Added test cases for new path joining functions and corrected test assertion
.github/workflows/PR-test.yml Updated CI workflow triggers and added PowerShell profile creation for Windows tests

char *last = new + len - 1;
if (*start == '\0')
{
free(original);
Copy link

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

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

[nitpick] Missing space after 'free' function call. Should be 'free (original);' to match the project's coding style.

Copilot uses AI. Check for mistakes.
if (*start == '\0')
{
free(original);
return xy_strdup ("");
Copy link

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

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

[nitpick] Inconsistent spacing in function call. Should be 'xy_strdup ("");' but the empty string parameter doesn't follow the spacing pattern used elsewhere.

Copilot uses AI. Check for mistakes.
char *normalized = xy_str_remove_quotes (path);

// 将反斜杠转换为正斜杠
char *temp = xy_str_gsub (normalized, "\\\\", "/");
Copy link

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

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

The escaped backslashes '\\' are difficult to read and maintain. Consider using a string constant or adding a comment to clarify this represents double backslashes.

Suggested change
char *temp = xy_str_gsub (normalized, "\\\\", "/");
char *temp = xy_str_gsub (normalized, XY_DOUBLE_BACKSLASH, "/");

Copilot uses AI. Check for mistakes.
free (normalized1);
free (normalized2);

return xy_strjoin (3, "'", result, "'");
Copy link

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

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

Memory leak: 'result' is not freed before returning the joined string. This will cause memory leaks in path operations.

Suggested change
return xy_strjoin (3, "'", result, "'");
char *final_result = xy_strjoin (3, "'", result, "'");
free(result);
return final_result;

Copilot uses AI. Check for mistakes.
normalized[len - 1] = '\0';
}

return xy_str_add_quotes (normalized);
Copy link

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

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

Memory leak: 'normalized' is not freed before calling xy_str_add_quotes, which creates a new string. This will cause memory leaks.

Suggested change
return xy_str_add_quotes (normalized);
char *quoted = xy_str_add_quotes (normalized);
free (normalized);
return quoted;

Copilot uses AI. Check for mistakes.
*last = '\0';

free (dir);
return xy_str_add_quotes (clean_dir);
Copy link

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

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

Memory leak: 'clean_dir' is not freed before calling xy_str_add_quotes. This will cause memory leaks in the xy_parent_dir function.

Suggested change
return xy_str_add_quotes (clean_dir);
char *quoted = xy_str_add_quotes (clean_dir);
free (clean_dir);
return quoted;

Copilot uses AI. Check for mistakes.
@Mikachu2333
Copy link
Collaborator Author

那我就先改win 的doc目录吧,剩下的路径处理再说

@Mikachu2333
Copy link
Collaborator Author

这玩意确实还是拆了一点点改吧

@Mikachu2333 Mikachu2333 deleted the feat/path_improve branch August 17, 2025 05:47
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