Re: [PATCH 1/1] t6042: work around speed optimization on Windows
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jan 16, 2019, 20:30 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1901162108140.41@tvgsbejvaqbjf.bet>
- In-Reply-To
- <CABPp-BHOkVKVpZy2RKj-ofoajGT0rgrb2TpQsXprk1_yZwtVfA@mail.gmail.com>
Hi Elijah,
On Wed, 16 Jan 2019, Elijah Newren wrote:
Show 42 quoted lines
> On Wed, Jan 16, 2019 at 5:37 AM Johannes Schindelin via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > 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.
Right, the cover letter has the benefit of being written last (so I have thought about the issue the longest before casting my thoughts into words while writing it, longer than when I come up with the commit message, which is typically even before I test things thoroughly).
Show 9 quoted lines
> 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?
Sure. I added this paragraph:
Functionally, we do not change anything because we replace two `git mv <old> <new>` calls (where `<new>` is completely overwritten and `git add`ed later anyway) by `git rm <old>` calls (removing other files, too, that are also completely overwritten and `git add`ed later).
This should explain the intention nicely, would you agree?
Show 19 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.True. But it *is* replaced and `git add`ed later, too.
> Since both are added back later it'll still work either way on linux, > but was it suffering from the same problem on Windows?
Yes. All 6 versions of the file are written in quick succession. So yes, *any* of these 6 versions that are intended to replace another of said 6 versions will suffer from the exact same problem.
> It too was a case of both the filesize before and after remaining the > same despite contents changing, so it certainly seem possible.
Yep.
Show 13 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 did not make sense for me to use two *different* ways to perform essentially the same operation. I know I would scratch my head until it hurts if I read intentionally different command cadences, to figure out why they are different. Only to find out that they are not intended to be different? I don't think so.
> 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
Indeed. I would *hate* to lose time over reading those test cases in 6 months, just because they use different paradigms (even if they want to do the same thing).
And I have to spend *way* too much time reading test scripts. So I am very adamant about making my own life easier here.
Point in case: it took me quite a good while to understand the code that I fixed in this patch, because the underlying intention was not immediately accessible to me. The test failure happens in the *next* test case, after all. That was a head scratcher, too: the problem was in a test case that *passed*, not in the one that failed.
It all comes back to what I have been harping on for at least a year: our test suite is not really optimized for figuring out how to fix test failures. There is nothing immediately actionable when anything fails. A developer has to spend considerable time to find out what is going wrong if a test failure happens (is it flakey? Is it a bug in the *test*? Is it a *true regression*?)
Of course, this is not a new problem. And I *do* see that some contributors are as interested as I am in a more useful test suite, one where a failed test *really* indicates a regression, one where the output of a failed test has enough evidence to start fixing the bug right away (rather than spending time on understanding the intention of what was tested first). For example, when `wc -l` invocations were replaced by `test_line_count`, not only did we address portability problems (where BSD `wc` would indent the output), we also changed the code to indicate quite clearly *what* was tested. We do not yet output the contents of the file when `test_i18ngrep` fails, but that would also be a step in the right direction.
Here's hoping that our test suite improves in that respect: to make it easier and quicker to fix regressions.
Ciao, Dscho
Show 10 quoted lines
> > mv -f file_v3 one && > > mv -f file_v6 three && > > git add one three && > > -- > > gitgitgadget > > > Thanks, > Elijah >