Re: [PATCH 1/1] t6042: work around speed optimization on Windows

From: Elijah Newren <newren@gmail.com>
Date: Jan 16, 2019, 15:56 UTC
Message-ID: <CABPp-BHOkVKVpZy2RKj-ofoajGT0rgrb2TpQsXprk1_yZwtVfA@mail.gmail.com>
In-Reply-To: <598de6652cdb19b9772f322f17600c3845f208cc.1547645839.git.gitgitgadget@gmail.com>

Hi Dscho,

On Wed, Jan 16, 2019 at 5:37 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 35 quoted lines

>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When Git determines whether a file has changed, it looks at the mtime,
> at the file size, and to detect changes even if the mtime is the same
> (on Windows, the mtime granularity is 100ns, read: if two files are
> written within the same 100ns time slot, they have the same mtime) and
> even if the file size is the same, Git also looks at the inode/device
> numbers.
>
> This design obviously comes from a Linux background, where `lstat()`
> calls were designed to be cheap.
>
> On Windows, there is no `lstat()`. It has to be emulated. And while
> obtaining the mtime and the file size is not all that expensive (you can
> get both with a single `GetFileAttributesW()` call), obtaining the
> equivalent of the inode and device numbers is very expensive (it
> requires a call to `GetFileInformationByHandle()`, which in turn
> requires a file handle, which is *a lot* more expensive than one might
> imagine).
>
> As it is very uncommon for developers to modify files within 100ns time
> slots, Git for Windows chooses not to fill inode/device numbers
> properly, but simply sets them to 0.
>
> However, in t6042 the files file_v1 and file_v2 are typically written
> within the same 100ns time slot, and they do not differ in file size. So
> the minor modification is not picked up.
>
> Let's work around this issue by avoiding the `git mv` calls in the
> 'mod6-setup: chains of rename/rename(1to2) and rename/rename(2to1)' test
> case. The target files are overwritten anyway, so it is not like we
> really rename those files. This fixes the issue because `git add` will
> now add the files as new files (as opposed to existing, just renamed
> files).

I actually read this before the cover letter (just responded in
opposite order), and the last paragraph alarmed me at first, because
it made it sound like it was dispensing with the need for rename
detection and thus breaking the intent of the testcase.  Granted,
looking at the code it becomes clear you're not changing the intent of
the testcase at all, just transforming the setup steps slightly.  In
your cover letter, you made this clear by stating that you were
replacing
    git mv <old> <new> && mv <file> <new> && git add <new>`
by
    git rm <old> && mv <file> <new> && git add <new>`
Perhaps something like that could be added here or other wording to
clarify that it's just an innocuous transformation of the setup steps?

Show 15 quoted lines

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t6042-merge-rename-corner-cases.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
> index 7cc34e7579..09dfa8bd92 100755
> --- a/t/t6042-merge-rename-corner-cases.sh
> +++ b/t/t6042-merge-rename-corner-cases.sh
> @@ -1175,7 +1175,7 @@ test_expect_success 'setup nested conflicts from rename/rename(2to1)' '
>
>                 # Handle the left side
>                 git checkout L &&
> -               git mv one three &&
> +               git rm one two &&

Here you not only remove the file being renamed ('one') but also a file being modified ('two'); you didn't mention the latter in your commit message. Since both are added back later it'll still work either way on linux, but was it suffering from the same problem on Windows? It too was a case of both the filesize before and after remaining the same despite contents changing, so it certainly seem possible.

Show 9 quoted lines

>                 mv -f file_v2 three &&
>                 mv -f file_v5 two &&
>                 git add two three &&
> @@ -1183,7 +1183,7 @@ test_expect_success 'setup nested conflicts from rename/rename(2to1)' '
>
>                 # Handle the right side
>                 git checkout R &&
> -               git mv two three &&
> +               git rm one two &&

Also here you remove both a renamed and a modified file -- though in this case I think the file sizes change for both (each increases by one character) so this hunk of the patch probably isn't needed. It doesn't hurt though, and could be considered future-proofing against possible changes to the setup files, and may also just be nice to make the two blocks of setup look more consistent

Show 5 quoted lines

>                 mv -f file_v3 one &&
>                 mv -f file_v6 three &&
>                 git add one three &&
> --
> gitgitgadget

Thanks, Elijah