From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 19 Feb 2014 20:42:59 GMT
Subject: Re: [PATCH v3 24/25] prune: strategies for linked checkouts
Message-ID: <xmqqy5167s18.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqq38je9739.fsf@gitster.dls.corp.google.com>

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

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
> (Only nitpicks during this round of review).
>
>> +	if (get_device_or_die(path) != get_device_or_die(get_git_dir())) {
>> +		strbuf_reset(&sb);
>> +		strbuf_addf(&sb, "%s/locked", sb_repo.buf);
>> +		write_file(sb.buf, 1, "located on a different file system\n");
>> +		keep_locked = 1;
>> +	} else {
>> +		strbuf_reset(&sb);
>> +		strbuf_addf(&sb, "%s/link", sb_repo.buf);
>> +		link(sb_git.buf, sb.buf); /* it's ok to fail */
>
> If so, perhaps tell that to the code by saying something like
>
> 		(void) link(...);
>
> ?
>
> But why is it OK to fail in the first place?  If we couldn't link,
> don't you want to fall back to the lock codepath?  After all, the
> "are we on the same device?" check is to cope with the case where
> we cannot link and that alternate codepath is supposed to be
> prepared to handle the "ah, we cannot link" case correctly, no?

In other words, couldn't that part more like this?  That is, we
optimisiticly try the link(2) first and if it fails for whatever
reason fall back to whatever magic the lock codepath implements.

+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "%s/link", sb_repo.buf);
+	if (link(sb_git.buf, sb.buf) < 0) {
+		strbuf_reset(&sb);
+		strbuf_addf(&sb, "%s/locked", sb_repo.buf);
+		write_file(sb.buf, 1, "located on a different file system\n");
+		keep_locked = 1;
+	}
+