On Thu, Jul 29, 2021 at 3:21 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
> When performing a rebase, rmdir() is called on the folder .git/logs. On
> Unix rmdir() exits without deleting anything in case .git/logs is a
> symbolic link but the equivalent functions on Windows (_rmdir, _wrmdir
> and RemoveDirectoryW) do not behave the same and remove the folder if it
> is symlinked even if it is not empty.
>
> This creates issues when folders in .git/ are symlinks which is
> especially the case when git-repo[1] is used.
"issues" is a rather nebulous word. It doesn't help the reader
understand what actually goes wrong. Presumably some step later in the
rebase fails? Or is the problem that subsequent interaction with
git-repo fails because the link which git-repo (presumably) created
disappears out from underneath it?
> This commit updates mingw_rmdir() so that its behavior is the same as
> Linux rmdir() in case of symbolic links.
Okay, makes sense so far as the above explanation goes. But it also
feels like a band-aid fix to support a use-case which only works by
accident on Unix, if I'm reading between the lines correctly. That is,
presumably rmdir(".git/logs") is an intended cleanup action but the
fact that the cleanup doesn't happen as intended when it is a symlink
is not intentional behavior, thus git-repo's reliance upon that
behavior is questionable.
I guess this would also help tools which replace .git/worktrees with a
symlink since git-worktree -- as a cleanup -- removes its
.git/worktrees directory when the last worktree is removed...
> Signed-off-by: Thomas Bétous <tomspycell@xxxxxxxxx>
> ---
> compat/mingw.c | 15 +++++++++++++++
> diff --git a/compat/mingw.c b/compat/mingw.c
> @@ -341,6 +341,21 @@ int mingw_rmdir(const char *pathname)
> + /*
> + * Contrary to Linux rmdir(), Windows' _wrmdir() and _rmdir()
> + * will remove the directory at the path if it is a symbolic link
> + * which leads to issues when symlinks are used in the .git folder
> + * (in the context of git-repo for instance). So before calling _wrmdir()
> + * we first check if the path is a symbolic link. If it is, we exit
> + * and return the same error as Linux rmdir() in this case (ENOTDIR).
> + */
> + if (!mingw_lstat(pathname, &st) && S_ISLNK(st.st_mode)) {
> + errno = ENOTDIR;
> + return -1;
> + }
Unfortunately, this code comment doesn't help future readers
understand when/how this case can be triggered, which means that
anyone working on this code in the future will have to consult the
commit message anyhow in order to obtain the necessary understanding.
This suggests that either the comment should be dropped altogether,
thus forcing the person to consult the commit message (probably
undesirable), or improved to give enough information to understand
when/how it can be triggered.
