summaryrefslogtreecommitdiff
path: root/t/t6046-merge-skip-unneeded-updates.sh
AgeCommit message (Collapse)Author
2020-03-13t6022, t6046: fix flaky files-are-updated checksElijah Newren
Several tests wanted to verify that files were actually modified by a merge, which it would do by checking that the mtime was updated. In order to avoid problems with the merge completing so fast that the mtime at the beginning and end of the operation was the same, these tests would first set the mtime of a file to something "old". This "old" value was usually determined as current system clock minus one second, truncated to the nearest integer. Unfortunately, it appears the system clock and filesystem clock are different and comparing across the two runs into race problems resulting in flaky tests. From https://stackoverflow.com/questions/14392975/timestamp-accuracy-on-ext4-sub-millsecond: date will call the gettimeofday system call which will always return the most accurate time available based on the cached kernel time, adjusted by the CPU cycle time if available to give nanosecond resolution. The timestamps stored in the file system however, are only based on the cached kernel time. ie The time calculated at the last timer interrupt. and from https://apenwarr.ca/log/20181113: Does mtime get set to >= the current time? No, this depends on clock granularity. For example, gettimeofday() can return times in microseconds on my system, but ext4 rounds timestamps down to the previous ~10ms (but not exactly 10ms) increment, with the surprising result that a newly-created file is almost always created in the past: $ python -c " import os, time t0 = time.time() open('testfile', 'w').close() print os.stat('testfile').st_mtime - t0 " -0.00234484672546 So, instead of trying to compare across what are effectively two different clocks, just avoid using the system clock. Any new updates to files have to give an mtime at least as big as what is already in the file, so we could define "old" as one second before the mtime found in the file before the merge starts. But, to avoid problems with leap seconds, ntp updates, filesystems that only provide two second resolution, and other such weirdness, let's just pick an hour before the mtime found in the file before the merge starts. Also, clarify in one test where we check the mtime of different files that it really was intentional. I totally forgot the reasons for that and assumed it was a bug when asked. Reported-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-27t6022, t6046: test expected behavior instead of testing a proxy for itElijah Newren
In t6022, we were testing for file being overwritten (or not) based on an output message instead of checking for the file being overwritten. Since we can check for the file being overwritten via mtime updates, check that instead. In t6046, we were largely checking for both the expected behavior and a proxy for it, which is unnecessary. The calls to test-tool also were a bit cryptic. Make them a little clearer. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-10-23t604[236]: do not run setup in separate testsElijah Newren
Transform the setup "tests" to setup functions, and have the actual tests call the setup functions. Advantages: * Should make life easier for people working with webby CI/PR builds who have to abuse mice (and their own index finger as well) in order to switch from viewing one testcase to another. Sounds awful; hopefully this will improve things for them. * Improves re-runnability: any failed test in any of these three files can now be re-run in isolation, e.g. ./t6042* --ver --imm -x --run=21 whereas before it would require two tests to be specified to the --run argument, the other needing to be picked out as the relevant setup test from one or two tests before. * Importantly, this still keeps the "setup" and "test" sections somewhat separate to make it easier for readers to discern what is just ancillary setup and what the intent of the test is. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-04-08merge-recursive: switch directory rename detection defaultElijah Newren
When all of x/a, x/b, and x/c have moved to z/a, z/b, and z/c on one branch, there is a question about whether x/d added on a different branch should remain at x/d or appear at z/d when the two branches are merged. There are different possible viewpoints here: A) The file was placed at x/d; it's unrelated to the other files in x/ so it doesn't matter that all the files from x/ moved to z/ on one branch; x/d should still remain at x/d. B) x/d is related to the other files in x/, and x/ was renamed to z/; therefore x/d should be moved to z/d. Since there was no ability to detect directory renames prior to git-2.18, users experienced (A) regardless of context. Choice (B) was implemented in git-2.18, with no option to go back to (A), and has been in use since. However, one user reported that the merge results did not match their expectations, making the change of default problematic, especially since there was no notice printed when directory rename detection moved files. Note that there is also a third possibility here: C) There are different answers depending on the context and content that cannot be determined by git, so this is a conflict. Use a higher stage in the index to record the conflict and notify the user of the potential issue instead of silently selecting a resolution for them. Add an option for users to specify their preference for whether to use directory rename detection, and default to (C). Even when directory rename detection is on, add notice messages about files moved into new directories. As a sidenote, x/d did not have to be a new file here; it could have already existed at some other path and been renamed to x/d, with directory rename detection just renaming it again to z/d. Thus, it's not just new files, but also a modification to all rename types (normal renames, rename/add, rename/delete, rename/rename(1to1), rename/rename(1to2), and rename/rename(2to1)). Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-07-16t6046/t9833: fix use of "VAR=VAL cmd" with a shell functionEric Sunshine
Unlike "FOO=bar cmd" one-shot environment variable assignments which exist only for the invocation of 'cmd', those assigned by "FOO=bar shell_func" exist within the running shell and continue to do so until the process exits (or are explicitly unset). It is unlikely that this behavior was intended by the test author. In these particular tests, the "FOO=bar shell_func" invocations are already in subshells, so the assignments don't last too long, don't appear to harm subsequent commands in the same subshells, and don't affect other tests in the same scripts, however, the usage is nevertheless misleading and poor practice, so fix the tests to assign and export the environment variables in the usual fashion. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-08merge-recursive: fix check for skipability of working tree updatesElijah Newren
The can-working-tree-updates-be-skipped check has had a long and blemished history. The update can be skipped iff: a) The merge is clean b) The merge matches what was in HEAD (content, mode, pathname) c) The target path is usable (i.e. not involved in D/F conflict) Traditionally, we split b into parts: b1) The merged result matches the content and mode found in HEAD b2) The merged target path existed in HEAD Steps a & b1 are easy to check; we have always gotten those right. While it is easy to overlook step c, this was fixed seven years ago with commit 4ab9a157d069 ("merge_content(): Check whether D/F conflicts are still present", 2010-09-20). merge-recursive didn't have a readily available way to directly check step b2, so various approximations were used: * In commit b2c8c0a76274 ("merge-recursive: When we detect we can skip an update, actually skip it", 2011-02-28), it was noted that although the code claimed it was skipping the update, it did not actually skip the update. The code was made to skip it, but used lstat(path, ...) as an approximation to path-was-tracked-in-index-before-merge. * In commit 5b448b853030 ("merge-recursive: When we detect we can skip an update, actually skip it", 2011-08-11), the problem with using lstat was noted. It was changed to the approximation path2 && strcmp(path, path2) which is also wrong. !path2 || strcmp(path, path2) would have been better, but would have fallen short with directory renames. * In c5b761fb2711 ("merge-recursive: ensure we write updates for directory-renamed file", 2018-02-14), the problem with the previous approximation was noted and changed to was_tracked(path) That looks close to what we were trying to answer, but was_tracked() as implemented at the time should have been named is_tracked(); it returned something different than what we were looking for. * To make matters more complex, fixing was_tracked() isn't sufficient because the splitting of b into b1 and b2 is wrong. Consider the following merge with a rename/add conflict: side A: modify foo, add unrelated bar side B: rename foo->bar (but don't modify the mode or contents) In this case, the three-way merge of original foo, A's foo, and B's bar will result in a desired pathname of bar with the same mode/contents that A had for foo. Thus, A had the right mode and contents for the file, and it had the right pathname present (namely, bar), but the bar that was present was unrelated to the contents, so the working tree update was not skippable. Fix this by introducing a new function: was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) and use it to directly check for condition b. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-05-08t6046: testcases checking whether updates can be skipped in a mergeElijah Newren
Add several tests checking whether updates can be skipped in a merge. Also add several similar testcases for where updates cannot be skipped in a merge to make sure that we skip if and only if we should. In particular: * Testcase 1a (particularly 1a-check-L) would have pointed out the problem Linus has been dealing with for year with his merges[1]. * Testcase 2a (particularly 2a-check-L) would have pointed out the problem with my directory-rename-series before it broke master[2]. * Testcases 3[ab] (particularly 3a-check-L) provide a simpler testcase than 12b of t6043 making that one easier to understand. * There are several complementary testcases to make sure we're not just fixing those particular issues while regressing in the opposite direction. * There are also a pair of tests for the special case when a merge results in a skippable update AND the user has dirty modifications to the path. [1] https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fXJRkvU1m_RHBG54NOoaZPA@mail.gmail.com/ [2] https://public-inbox.org/git/xmqqmuya43cs.fsf@gitster-ct.c.googlers.com/ Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>