Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty

From: Paul Smith <paul@mad-scientist.net>
Date: Nov 18, 2014, 17:46 UTC
Message-ID: <s934mtwo0zv.fsf@mad-scientist.net>
In-Reply-To: <xmqqy4r9yc5u.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

> Paul Smith <paul@mad-scientist.net> writes:

> No need to resend only to correct the above, even though there may
> be comments on the patch itself from me or others that may make you
> want to reroll this patch, in which case I'd like to see these nits
> gone.

I'll fix in the next iteration.

Show 7 quoted lines

>> -# don't recreate a workdir over an existing repository
>> -if test -e "$new_workdir"
>> +# make sure the links use full paths
>> +git_dir=$(cd "$git_dir"; pwd)
>
> With this change, the comment gets much harder to understand.  "What
> links?"  would be the reaction from those who are reading the patch.

I just moved this line; I don't think it had much more context beforehand, but I'll definitely rewrite the comment to be more clear.

>> +	if test $(ls -a1 "$new_workdir/." | wc -l) -ne 2

> I wonder if this check is portable for all platforms we care about,
> but that is OK, as it should be so for the ones I think of and care
> about ;-)

Do you mean "." and ".." representing an empty directory? That will work on any system where /bin/sh works, for sure.

Or do you mean using "ls" and "wc"?  I can easily avoid this.

Recall that new-workdir itself only works on systems that support symlinks; this limits its portability. For example it doesn't work on Windows (unfortunately). I spent the better part of a day a few months ago playing with the various "symlink-ish" capabilities of Windows NTFS and couldn't find any reliable way to make this work (although I have virtually no Windows fu).

> The script chdirs around; did you turn $new_workdir into an absolute
> path already, or given a relative $new_workdir this is attempting to
> remove a hierarchy that is different from what you created?

Good point, I'll fix this up

Show 28 quoted lines

>> +}
>> +siglist="0 1 2 15"
>> +trap cleanup $siglist
>>  
>> -# create the workdir
>> -mkdir -p "$new_workdir/.git" || die "unable to create \"$new_workdir\"!"
>> +# create embedded directories
>> +for x in logs
>> +do
>> +	mkdir -p "$new_workdir/.git/$x" || failed
>> +done
>>  
>>  # create the links to the original repo.  explicitly exclude index, HEAD and
>>  # logs/HEAD from the list since they are purely related to the current
>> working
>>  # directory, and should not be shared.
>>  for x in config refs logs/refs objects info hooks packed-refs remotes
>> rr-cache svn
>>  do
>> -	case $x in
>> -	*/*)
>> -		mkdir -p "$(dirname "$new_workdir/.git/$x")"
>> -		;;
>> -	esac
>
> What's this removal about?  If $new_workdir/.git/logs does not
> exist, would "ln -s $there/logs/refs $new_workdir/.git/logs/refs"
> succeed without first creating $new_workdir/.git/logs directory?

I split the creation of the directories from the symlinks: see the new loop above. This allows us to avoid the icky dirname stuff.

New iteration will follow shortly.

Cheers!