Re: [PATCH] git-new-workdir: add windows compatibility

From: Junio C Hamano <gitster@pobox.com>
Date: May 26, 2015, 04:03 UTC
Message-ID: <xmqqfv6k7zp3.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CADBZQ5iAKsSrdvBnFcdPcm9psaJo5B-H1zqJj0aRc+xx6cCFMQ@mail.gmail.com>

Daniel Smith <dansmith65@gmail.com> writes:

Show 14 quoted lines

> When running on Windows in MinGW, creating symbolic links via ln always
> failed.
>
> Using mklink instead of ln is the recommended method of creating links on
> Windows:
> http://stackoverflow.com/questions/18641864/git-bash-shell-fails-to-create-symbolic-links
>
> Script now branches on Windows to use mklink. This change should not affect
> unix systems.
>
> Signed-off-by: Daniel Smith <dansmith65@gmail.com>
>
> Has been tested on Windows 8.1 and OS X Yosemite.
> ---

Swap the "Has been tested..." and "Signed-off-by:" lines.

I'll defer to Windows folks if "mklink" is a sensible thing to use or not; I have no first-hand experience with Windows, but only heard that links are for admin user only or something like that, so I want to hear from people whose judgement on Windows matters I trust.

>
> +iswindows () {
> + [[ -n "$WINDIR" ]];
> +}

Please don't add unnecessary bash-isms. We have kept this script usable without stepping out of POSIX.

	test -n "$WINDIR"

Show 7 quoted lines

> -git_dir=$(cd "$git_dir" && pwd) || exit 1
> +if iswindows
> +then
> + git_dir=$(cd "$git_dir"; cmd.exe /c cd) || exit 1
> +else
> + git_dir=$(cd "$git_dir" && pwd) || exit 1
> +fi

Indentation of lines inside a new block is done with one more level of HT in our scripts, not with just one SP.

> - ln -s "$git_dir/$x" "$new_workdir/.git/$x" || failed
> + if iswindows
> + then

Move these into a helper shell function, starting from here...

Show 8 quoted lines

> + if test -d "$git_dir/$x"
> + then
> + # create directory symbolic link
> + isdir="/d"
> + fi
> + # convert path separator to backslash
> + targetPath=$(sed -e 's#^J:##' -e 's#/#\\#g' <<< "$git_dir/$x")
> + cmd.exe /c "mklink $isdir \"$new_workdir/.git/$x\" \"$targetPath\"" || failed

... up to here.  Also a few points about these new lines:

 * Use indentation when doing nested if/then/if/then/fi/fi block,
   i.e.

	if isWindows
        then
		if test -d "..."
                then
			isdir=/d
                fi
                target=..
                cmd.exe /c ...
	fi

 * "<<<" is a bash-ism, isn't it?

 * Use of "#" as s/// separator, when slash is not involved, looks
   ugly and makes it harder to read.

 * Is "J:" drive something special (unlike C: or D: drives)?

 * Can computation of targetPath fail?  IOW, shouldn't that line end
   with &&?

 * Share || failed between this part and POSIX part, i.e.

	if isWindows
        then
		ln_s_win "$new_workdir" "$x"
	else
		ln -s "$git_dir/$x" "$new_workdir'.git/$x"
	fi || failed

   where ln_s_win would be the "helper shell function" I suggested.

	ln_s_win () {
		if test -d "$git_dir/$2"
		then
                	isdir=/d
		fi
                target=$(printf "%s" "$git_dir/$2" | sed -e "...") &&
		cmd.exe /c "mklink $isdir ..."
	}

Show 6 quoted lines

> + else
> + ln -s "$git_dir/$x" "$new_workdir/.git/$x" || failed
> + fi
>  done
>
>  # commands below this are run in the context of the new workdir

Thanks.