Subject: Re: [RFC] git checkout $tree -- $path always rewrites files

From: Junio C Hamano <[email-masked]>
Date: Thu, 13 Nov 2014 11:15:27 -0800

Jeff King <[email-masked]> writes:

> On Sun, Nov 09, 2014 at 09:21:49AM -0800, Junio C Hamano wrote:
>
>> Jeff King <[email-masked]> writes:
>> 
>> > On Fri, Nov 07, 2014 at 11:35:59PM -0800, Junio C Hamano wrote:
>> >
>> >> I think that has direct linkage; what you have in mind I think is
>> >> http://thread.gmane.org/gmane.comp.version-control.git/234903/focus=234935
>> >
>> > Thanks for that link.
>> 
>> It was one of the items in the "git blame leftover bits" list
>> (websearch for that exact phrase), so I didn't have to do any
>> digging just for this thread ;-)
>> 
>> But I made a huge typo above.  s/I think/I do not think/;
>
> Oh. That might explain some of my confusion. :)

Yeah, tells me to never type on a tablet X-<.

>> I'd prefer that these two to be treated separately.
>
> Yeah, that makes sense after reading your emails. What I was really
> unclear on was whether the handling of deletion was a bug or a design
> choice, and it is the latter (if it were the former, we would not need a
> transition plan :) ).

Yeah, I think we agree to refrain from saying if that design choice
was a good one or bad one at least for now.

> Subject: checkout $tree: do not throw away unchanged index entries
>
> When we "git checkout $tree", we pull paths from $tree into
> the index, and then check the resulting entries out to the
> worktree. Our method for the first step is rather
> heavy-handed, though; it clobbers the entire existing index
> entry, even if the content is the same. This means we lose
> our stat information, leading checkout_entry to later
> rewrite the entire file with identical content.
>
> Instead, let's see if we have the identical entry already in
> the index, in which case we leave it in place. That lets
> checkout_entry do the right thing. Our tests cover two
> interesting cases:
>
>   1. We make sure that a file which has no changes is not
>      rewritten.
>
>   2. We make sure that we do update a file that is unchanged
>      in the index (versus $tree), but has working tree
>      changes. We keep the old index entry, and
>      checkout_entry is able to realize that our stat
>      information is out of date.
>
> Signed-off-by: Jeff King <[email-masked]>
> ---
> Note that the test refreshes the index manually (because we are tweaking
> the timestamp of file2). In normal use this should not be necessary
> (i.e., your entries should generally be uptodate). I did wonder if
> checkout should be refreshing the index itself, but it would a bunch of
> extra lstats in the common case.
>
>  builtin/checkout.c        | 31 +++++++++++++++++++++++++------
>  t/t2022-checkout-paths.sh | 17 +++++++++++++++++
>  2 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 5410dac..67cab4e 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -65,21 +65,40 @@ static int post_checkout_hook(struct commit *old, struct commit *new,
>  static int update_some(const unsigned char *sha1, const char *base, int baselen,
>  		const char *pathname, unsigned mode, int stage, void *context)
>  {
> ...
>  }

Makes sense, including the use of strbuf (otherwise you would
allocate ce and then discard when it turns out that it is not
needed, which is probably with the same allocation pressure, but
looks uglier).

> diff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh
> index 8e3545d..f46d049 100755
> --- a/t/t2022-checkout-paths.sh
> +++ b/t/t2022-checkout-paths.sh
> @@ -61,4 +61,21 @@ test_expect_success 'do not touch unmerged entries matching $path but not in $tr
>  	test_cmp expect.next0 actual.next0
>  '
>  
> +test_expect_success 'do not touch files that are already up-to-date' '
> +	git reset --hard &&
> +	echo one >file1 &&
> +	echo two >file2 &&
> +	git add file1 file2 &&
> +	git commit -m base &&
> +	echo modified >file1 &&
> +	test-chmtime =1000000000 file2 &&

Is the idea behind the hardcoded timestamp that this is sufficiently
old (Sep 2001) that we will not get in trouble comparing with the
real timestamp we get from the filesystem (which will definitely newer
than that anyway) no matter when we run this test (unless you have a
time-machine, that is)?

> +	git update-index -q --refresh &&
> +	git checkout HEAD -- file1 file2 &&
> +	echo one >expect &&
> +	test_cmp expect file1 &&
> +	echo "1000000000	file2" >expect &&
> +	test-chmtime -v +0 file2 >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done