summaryrefslogtreecommitdiff
path: root/unpack-trees.c
AgeCommit message (Collapse)AuthorFilesLines
2009-03-17Merge branch 'jc/maint-1.6.0-read-tree-overlay'Libravatar Junio C Hamano1-3/+3
* jc/maint-1.6.0-read-tree-overlay: read-tree A B C: do not create a bogus index and do not segfault
2009-03-12read-tree A B C: do not create a bogus index and do not segfaultLibravatar Junio C Hamano1-3/+3
"git read-tree A B C..." without the "-m" (merge) option is a way to read these trees on top of each other to get an overlay of them. An ancient commit ee6566e (Rewrite read-tree, 2005-09-05) passed the ADD_CACHE_SKIP_DFCHECK flag when calling add_index_entry() to add the paths obtained from these trees to the index, but it is an incorrect use of the flag. The flag is meant to be used by callers who know the addition of the entry does not introduce a D/F conflict to the index in order to avoid the overhead of checking. This bug resulted in a bogus index that records both "x" and "x/z" as a blob after reading three trees that have paths ("x"), ("x", "y"), and ("x/z", "y") respectively. 34110cd (Make 'unpack_trees()' have a separate source and destination index, 2008-03-06) refactored the callsites of add_index_entry() incorrectly and added more codepaths that use this flag when it shouldn't be used. Also, 0190457 (Move 'unpack_trees()' over to 'traverse_trees()' interface, 2008-03-05) introduced a bug to call add_index_entry() for the tree that does not have the path in it, passing NULL as a cache entry. This caused reading multiple trees, one of which has path "x" but another doesn't, to segfault. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-03-07Record ns-timestamps if possible, but do not use it without USE_NSECLibravatar Kjetil Barvik1-2/+0
Traditionally, the lack of USE_NSEC meant "do not record nor use the nanosecond resolution part of the file timestamps". To avoid problems on filesystems that lose the ns part when the metadata is flushed to the disk and then later read back in, disabling USE_NSEC has been a good idea in general. If you are on a filesystem without such an issue, it does not hurt to read and store them in the cached stat data in the index entries even if your git is compiled without USE_NSEC. The index left with such a version of git can be read by git compiled with USE_NSEC and it can make use of the nanosecond part to optimize the check to see if the path on the filesystem hsa been modified since we last looked at. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-02-19verify_uptodate(): add ce_uptodate(ce) testLibravatar Kjetil Barvik1-1/+1
If we inside verify_uptodate() can already tell from the ce entry that it is already uptodate by testing it with ce_uptodate(ce), there is no need to call lstat(2) and ie_match_stat() afterwards. And, reading from the commit log message from: commit eadb5831342bb2e756fa05c03642c4aa1929d4f5 Author: Junio C Hamano <gitster@pobox.com> Date: Fri Jan 18 23:45:24 2008 -0800 Avoid running lstat(2) on the same cache entry. this also seems to be correct usage of the ce_uptodate() macro introduced by that patch. This will avoid lots of lstat(2) calls in some cases, for example by running the 'git checkout' command. Signed-off-by: Kjetil Barvik <barvik@broadpark.no> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-02-19make USE_NSEC work as expectedLibravatar Kjetil Barvik1-2/+6
Since the filesystem ext4 is now defined as stable in Linux v2.6.28, and ext4 supports nanonsecond resolution timestamps natively, it is time to make USE_NSEC work as expected. This will make racy git situations less likely to happen. For 'git checkout' this means it will be less likely that we have to open, read the contents of the file into RAM, and check if file is really modified or not. The result sould be a litle less used CPU time, less pagefaults and a litle faster program, at least for 'git checkout'. Since the number of possible racy git situations would increase when disks gets faster, this patch would be more and more helpfull as times go by. For a fast Solid State Disk, this patch should be helpfull. Note that, when file operations starts to take less than 1 nanosecond, one would again start to get more racy git situations. For more info on racy git, see Documentation/technical/racy-git.txt For more info on ext4, see http://kernelnewbies.org/Ext4 Signed-off-by: Kjetil Barvik <barvik@broadpark.no> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-02-18check_updates(): effective removal of cache entries marked CE_REMOVELibravatar Kjetil Barvik1-3/+1
Below is oprofile output from GIT command 'git chekcout -q my-v2.6.25' (move from tag v2.6.27 to tag v2.6.25 of the Linux kernel): CPU: Core 2, speed 1999.95 MHz (estimated) Counted CPU_CLK_UNHALTED events (Clock cycles when not halted) with a unit mask of 0x00 (Unhalted core cycles) count 20000 Counted INST_RETIRED_ANY_P events (number of instructions retired) with a unit mask of 0x00 (No unit mask) count 20000 CPU_CLK_UNHALT...|INST_RETIRED:2...| samples| %| samples| %| ------------------------------------ 409247 100.000 342878 100.000 git CPU_CLK_UNHALT...|INST_RETIRED:2...| samples| %| samples| %| ------------------------------------ 260476 63.6476 257843 75.1996 libz.so.1.2.3 100876 24.6492 64378 18.7758 kernel-2.6.28.4_2.vmlinux 30850 7.5382 7874 2.2964 libc-2.9.so 14775 3.6103 8390 2.4469 git 2020 0.4936 4325 1.2614 libcrypto.so.0.9.8 191 0.0467 32 0.0093 libpthread-2.9.so 58 0.0142 36 0.0105 ld-2.9.so 1 2.4e-04 0 0 libldap-2.3.so.0.2.31 Detail list of the top 20 function entries (libz counted in one blob): CPU_CLK_UNHALTED INST_RETIRED_ANY_P samples % samples % image name symbol name 260476 63.6862 257843 75.2725 libz.so.1.2.3 /lib/libz.so.1.2.3 16587 4.0555 3636 1.0615 libc-2.9.so memcpy 7710 1.8851 277 0.0809 libc-2.9.so memmove 3679 0.8995 1108 0.3235 kernel-2.6.28.4_2.vmlinux d_validate 3546 0.8670 2607 0.7611 kernel-2.6.28.4_2.vmlinux __getblk 3174 0.7760 1813 0.5293 libc-2.9.so _int_malloc 2396 0.5858 3681 1.0746 kernel-2.6.28.4_2.vmlinux copy_to_user 2270 0.5550 2528 0.7380 kernel-2.6.28.4_2.vmlinux __link_path_walk 2205 0.5391 1797 0.5246 kernel-2.6.28.4_2.vmlinux ext4_mark_iloc_dirty 2103 0.5142 1203 0.3512 kernel-2.6.28.4_2.vmlinux find_first_zero_bit 2077 0.5078 997 0.2911 kernel-2.6.28.4_2.vmlinux do_get_write_access 2070 0.5061 514 0.1501 git cache_name_compare 2043 0.4995 1501 0.4382 kernel-2.6.28.4_2.vmlinux rcu_irq_exit 2022 0.4944 1732 0.5056 kernel-2.6.28.4_2.vmlinux __ext4_get_inode_loc 2020 0.4939 4325 1.2626 libcrypto.so.0.9.8 /usr/lib/libcrypto.so.0.9.8 1965 0.4804 1384 0.4040 git patch_delta 1708 0.4176 984 0.2873 kernel-2.6.28.4_2.vmlinux rcu_sched_grace_period 1682 0.4112 727 0.2122 kernel-2.6.28.4_2.vmlinux sysfs_slab_alias 1659 0.4056 290 0.0847 git find_pack_entry_one 1480 0.3619 1307 0.3816 kernel-2.6.28.4_2.vmlinux ext4_writepage_trans_blocks Notice the memmove line, where the CPU did 7710 / 277 = 27.8 cycles per instruction, and compared to the total cycles spent inside the source code of GIT for this command, all the memmove() calls translates to (7710 * 100) / 14775 = 52.2% of this. Retesting with a GIT program compiled for gcov usage, I found out that the memmove() calls came from remove_index_entry_at() in read-cache.c, where we have: memmove(istate->cache + pos, istate->cache + pos + 1, (istate->cache_nr - pos) * sizeof(struct cache_entry *)); remove_index_entry_at() is called 4902 times from check_updates() in unpack-trees.c, and each time called we move each cache_entry pointers (from the removed one) one step to the left. Since we have 28828 entries in the cache this time, and if we on average move half of them each time, we in total move approximately 4902 * 0.5 * 28828 * 4 = 282 629 712 bytes, or twice this amount if each pointer is 8 bytes (64 bit). OK, is seems that the function check_updates() is called 28 times, so the estimated guess above had been more correct if check_updates() had been called only once, but the point is: we get lots of bytes moved. To fix this, and use an O(N) algorithm instead, where N is the number of cache_entries, we delete/remove all entries in one loop through all entries. From a retest, the new remove_marked_cache_entries() from the patch below, ended up with the following output line from oprofile: 46 0.0105 15 0.0041 git remove_marked_cache_entries If we can trust the numbers from oprofile in this case, we saved approximately ((7710 - 46) * 20000) / (2 * 1000 * 1000 * 1000) = 0.077 seconds CPU time with this fix for this particular test. And notice that now the CPU did only 46 / 15 = 3.1 cycles/instruction. Signed-off-by: Kjetil Barvik <barvik@broadpark.no> Acked-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-02-09unlink_entry(): introduce schedule_dir_for_removal()Libravatar Kjetil Barvik1-24/+6
Currently inside unlink_entry() if we get a successful removal of one file with unlink(), we try to remove the leading directories each and every time. So if one directory containing 200 files is moved to an other location we get 199 failed calls to rmdir() and 1 successful call. To fix this and avoid some unnecessary calls to rmdir(), we schedule each directory for removal and wait much longer before we do the real call to rmdir(). Since the unlink_entry() function is called with alphabetically sorted names, this new function end up being very effective to avoid unnecessary calls to rmdir(). In some cases over 95% of all calls to rmdir() is removed with this patch. Signed-off-by: Kjetil Barvik <barvik@broadpark.no> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-02-09lstat_cache(): swap func(length, string) into func(string, length)Libravatar Kjetil Barvik1-2/+2
Swap function argument pair (length, string) into (string, length) to conform with the commonly used order inside the GIT source code. Also, add a note about this fact into the coding guidelines. Signed-off-by: Kjetil Barvik <barvik@broadpark.no> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-01-31Merge branch 'maint'Libravatar Junio C Hamano1-3/+6
* maint: merge: fix out-of-bounds memory access
2009-01-31Merge branch 'maint-1.6.0' into maintLibravatar Junio C Hamano1-3/+6
* maint-1.6.0: merge: fix out-of-bounds memory access
2009-01-31merge: fix out-of-bounds memory accessLibravatar René Scharfe1-3/+6
The parameter n of unpack_callback() can have a value of up to MAX_UNPACK_TREES. The check at the top of unpack_trees() (its only (indirect) caller) makes sure it cannot exceed this limit. unpack_callback() passes it and the array src to unpack_nondirectories(), which has this loop: for (i = 0; i < n; i++) { /* ... */ src[i + o->merge] = o->df_conflict_entry; o->merge can be 0 or 1, so unpack_nondirectories() potentially accesses the array src at index MAX_UNPACK_TREES. This patch makes it big enough. Reported-by: Ingo Molnar <mingo@elte.hu> Signed-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-01-25Merge branch 'kb/lstat-cache'Libravatar Junio C Hamano1-2/+2
* kb/lstat-cache: lstat_cache(): introduce clear_lstat_cache() function lstat_cache(): introduce invalidate_lstat_cache() function lstat_cache(): introduce has_dirs_only_path() function lstat_cache(): introduce has_symlink_or_noent_leading_path() function lstat_cache(): more cache effective symlink/directory detection
2009-01-23Merge branch 'cb/maint-unpack-trees-absense' into maintLibravatar Junio C Hamano1-18/+16
* cb/maint-unpack-trees-absense: unpack-trees: remove redundant path search in verify_absent unpack-trees: fix path search bug in verify_absent unpack-trees: handle failure in verify_absent
2009-01-18lstat_cache(): introduce has_symlink_or_noent_leading_path() functionLibravatar Kjetil Barvik1-2/+2
In some cases, especially inside the unpack-trees.c file, and inside the verify_absent() function, we can avoid some unnecessary calls to lstat(), if the lstat_cache() function can also be told to keep track of non-existing directories. So we update the lstat_cache() function to handle this new fact, introduce a new wrapper function, and the result is that we save lots of lstat() calls for a removed directory which previously contained lots of files, when we call this new wrapper of lstat_cache() instead of the old one. We do similar changes inside the unlink_entry() function, since if we can already say that the leading directory component of a pathname does not exist, it is not necessary to try to remove a pathname below it! Thanks to Junio C Hamano, Linus Torvalds and Rene Scharfe for valuable comments to this patch! Signed-off-by: Kjetil Barvik <barvik@broadpark.no> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-01-13Merge branch 'cb/maint-unpack-trees-absense'Libravatar Junio C Hamano1-18/+16
* cb/maint-unpack-trees-absense: unpack-trees: remove redundant path search in verify_absent unpack-trees: fix path search bug in verify_absent unpack-trees: handle failure in verify_absent
2009-01-05unpack-trees: remove redundant path search in verify_absentLibravatar Clemens Buchacher1-6/+2
Since the only caller, verify_absent, relies on the fact that o->pos points to the next index entry anyways, there is no need to recompute its position. Furthermore, if a nondirectory entry were found, this would return too early, because there could still be an untracked directory in the way. This is currently not a problem, because verify_absent is only called if the index does not have this entry. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-01-05unpack-trees: fix path search bug in verify_absentLibravatar Clemens Buchacher1-10/+10
Commit 0cf73755 (unpack-trees.c: assume submodules are clean during check-out) changed an argument to verify_absent from 'path' to 'ce', which is however shadowed by a local variable of the same name. The bug triggers if verify_absent is used on a tree entry, for which the index contains one or more subsequent directories of the same length. The affected subdirectories are removed from the index. The testcase included in this commit bisects to 55218834 (checkout: do not lose staged removal), which reveals the bug in this case, but is otherwise unrelated. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2009-01-05unpack-trees: handle failure in verify_absentLibravatar Clemens Buchacher1-3/+5
Commit 203a2fe1 (Allow callers of unpack_trees() to handle failure) changed the "die on error" behavior to "return failure code". verify_absent did not handle errors returned by verify_clean_subdirectory, however. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-11-11Fix non-literal format in printf-style callsLibravatar Daniel Lowe1-1/+1
These were found using gcc 4.3.2-1ubuntu11 with the warning: warning: format not a string literal and no format arguments Incorporated suggestions from Brandon Casey <casey@nrlssc.navy.mil>. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-11-01correct cache_entry allocationLibravatar Jeff King1-1/+1
Most cache_entry structs are allocated by using the cache_entry_size macro, which rounds the size of the struct up to the nearest multiple of 8 bytes (presumably to avoid memory fragmentation). There is one exception: the special "conflict entry" is allocated with an empty name, and so is explicitly given just one extra byte to hold the NUL. However, later code doesn't realize that this particular struct has been allocated differently, and happily tries reading and copying it based on the ce_size macro, which assumes the 8-byte alignment. This can lead to reading uninitalized data, though since that data is simply padding, there shouldn't be any problem as a result. Still, it makes sense to hold the padding assumption so as not to surprise later maintainers. This fixes valgrind errors in t1005, t3030, t4002, and t4114. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-09-09checkout: do not lose staged removalLibravatar Junio C Hamano1-1/+10
The logic to checkout a different commit implements the safety to never lose user's local changes. For example, switching from a commit to another commit, when you have changed a path that is different between them, need to merge your changes to the version from the switched-to commit, which you may not necessarily be able to resolve easily. By default, "git checkout" refused to switch branches, to give you a chance to stash your local changes (or use "-m" to merge, accepting the risks of getting conflicts). This safety, however, had one deliberate hole since early June 2005. When your local change was to remove a path (and optionally to stage that removal), the command checked out the path from the switched-to commit nevertheless. This was to allow an initial checkout to happen smoothly (e.g. an initial checkout is done by starting with an empty index and switching from the commit at the HEAD to the same commit). We can tighten the rule slightly to allow this special case to pass, without losing sight of removal explicitly done by the user, by noticing if the index is truly empty when the operation begins. For historical background, see: http://thread.gmane.org/gmane.comp.version-control.git/4641/focus=4646 This case is marked as *0* in the message, which both Linus and I said "it feels somewhat wrong but otherwise we cannot start from an empty index". Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-08-23unpack_trees(): protect the handcrafted in-core index from read_cache()Libravatar Junio C Hamano1-0/+1
unpack_trees() rebuilds the in-core index from scratch by allocating a new structure and finishing it off by copying the built one to the final index. The resulting in-core index is Ok for most use, but read_cache() does not recognize it as such. The function is meant to be no-op if you already have loaded the index, until you call discard_cache(). This change the way read_cache() detects an already initialized in-core index, by introducing an extra bit, and marks the handcrafted in-core index as initialized, to avoid this problem. A better fix in the longer term would be to change the read_cache() API so that it will always discard and re-read from the on-disk index to avoid confusion. But there are higher level API that have relied on the current semantics, and they and their users all need to get converted, which is outside the scope of 'maint' track. An example of such a higher level API is write_cache_as_tree(), which is used by git-write-tree as well as later Porcelains like git-merge, revert and cherry-pick. In the longer term, we should remove read_cache() from there and add one to cmd_write_tree(); other callers expect that the in-core index they prepared is what gets written as a tree so no other change is necessary for this particular codepath. The original version of this patch marked the index by pointing an otherwise wasted malloc'ed memory with o->result.alloc, but this version uses Linus's idea to use a new "initialized" bit, which is conceptually much cleaner. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-05-29unpack_trees(): allow callers to differentiate worktree errors from merge errorsLibravatar Junio C Hamano1-3/+7
Instead of uniformly returning -1 on any error, this teaches unpack_trees() to return -2 when the merge itself is Ok but worktree refuses to get updated. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-05-19unpack-trees: allow Porcelain to give different error messagesLibravatar Junio C Hamano1-14/+41
The plumbing output is sacred as it is an API. We _could_ change it if it is broken in such a way that it cannot convey necessary information fully, but we just do not _reword_ for the sake of rewording. If somebody does not like it, s/he is complaining too late. S/he should have been here in early May 2005 and make the language used by the API closer to what humans read. S/he wasn't here. Too bad, and it is too late. And people who complain should look at a bigger picture. Look at what was suggested by one of them and think for five seconds: $ git checkout mytopic -fatal: Entry 'frotz' not uptodate. Cannot merge. +fatal: Entry 'frotz' has local changes. Cannot merge. If you do not see something wrong with this output, your brain has already been rotten with use of git for too long a time. Nobody asked us to "merge" but why are we talking about "Cannot merge"? This patch introduces a mechanism to allow Porcelains to specify messages that are different from the ones that is given by the underlying plumbing implementation of read-tree, so that we can reword the message Porcelains give without disrupting the output from the plumbing. $ git-checkout pu error: You have local changes to 'Makefile'; cannot switch branches. There are other places that ask unpack_trees() to n-way merge, detect issues and let it issue error message on its own, but I did this as a demonstration and replaced only one message. Yes I know about C99 structure initializers. I'd love to use them but we try to be nice to compilers without it. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-05-10Optimize symlink/directory detectionLibravatar Linus Torvalds1-7/+5
This is the base for making symlink detection in the middle fo a pathname saner and (much) more efficient. Under various loads, we want to verify that the full path leading up to a filename is a real directory tree, and that when we successfully do an 'lstat()' on a filename, we don't get a false positive due to a symlink in the middle of the path that git should have seen as a symlink, not as a normal path component. The 'has_symlink_leading_path()' function already did this, and cached a single level of symlink information, but didn't cache the _lack_ of a symlink, so the normal behaviour was actually the wrong way around, and we ended up doing an 'lstat()' on each path component to check that it was a real directory. This caches the last detected full directory and symlink entries, and speeds up especially deep directory structures a lot by avoiding to lstat() all the directories leading up to each entry in the index. [ This can - and should - probably be extended upon so that we eventually never do a bare 'lstat()' on any path entries at *all* when checking the index, but always check the full path carefully. Right now we do not generally check the whole path for all our normal quick index revalidation. We should also make sure that we're careful about all the invalidation, ie when we remove a link and replace it by a directory we should invalidate the symlink cache if it matches (and vice versa for the directory cache). But regardless, the basic function needs to be sane to do that. The old 'has_symlink_leading_path()' was not capable enough - or indeed the code readable enough - to really do that sanely. So I'm pushing this as not just an optimization, but as a base for further work. ] Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-04-09Make unpack-tree update removed files before any updated filesLibravatar Linus Torvalds1-2/+7
This is immaterial on sane filesystems, but if you have a broken (aka case-insensitive) filesystem, and the objective is to remove the file 'abc' and replace it with the file 'Abc', then we must make sure to do the removal first. Otherwise, you'd first update the file 'Abc' - which would just overwrite the file 'abc' due to the broken case-insensitive filesystem - and then remove file 'abc' - which would now brokenly remove the just updated file 'Abc' on that broken filesystem. By doing removals first, this won't happen. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-04-09Make branch merging aware of underlying case-insensitive filsystemsLibravatar Linus Torvalds1-0/+26
If we find an unexpected file, see if that filename perhaps exists in a case-insensitive way in the index, and whether the file matches that. If so, ignore it as a known pre-existing file of a different name. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-04-09Make hash_name_lookup able to do case-independent lookupsLibravatar Linus Torvalds1-1/+1
Right now nobody uses it, but "index_name_exists()" gets a flag so you can enable it on a case-by-case basis. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-04-09Make "index_name_exists()" return the cache_entry it foundLibravatar Linus Torvalds1-4/+4
This allows verify_absent() in unpack_trees() to use the hash chains rather than looking it up using the binary search. Perhaps more importantly, it's also going to be useful for the next phase, where we actually start looking at the cache entry when we do case-insensitive lookups and checking the result. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-03-18Fix read-tree not to discard errorsLibravatar Junio C Hamano1-3/+6
This fixes the issue identified with recently added tests to t1004 Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-03-16Don't update unchanged merge entriesLibravatar Linus Torvalds1-3/+6
In commit 34110cd4e394e3f92c01a4709689b384c34645d8 ("Make 'unpack_trees()' have a separate source and destination index") I introduced a really stupid bug in that it would always add merged entries with the CE_UPDATE flag set. That caused us to always re-write the file, even when it was already up-to-date in the source index. Not only is that really stupid from a performance angle, but more importantly it's actively wrong: if we have dirty state in the tree when we merge, overwriting it with the result of the merge will incorrectly overwrite that dirty state. This trivially fixes the problem - simply don't set the CE_UPDATE flag when the merge result matches the old state. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-03-14Fix recent 'unpack_trees()'-related changes breaking 'git stash'Libravatar Linus Torvalds1-0/+2
On Sat, 15 Mar 2008, SZEDER G?bor wrote: > > The testcase usually fails during the first 25 run, but sometimes it > runs more than 100 times before failing. Damn, this series has had more subtle issues than I ever expected. 'git stash' creates its saved working tree object with: # state of the working tree w_tree=$( ( rm -f "$TMP-index" && cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" && GIT_INDEX_FILE="$TMP-index" && export GIT_INDEX_FILE && git read-tree -m $i_tree && git add -u && git write-tree && rm -f "$TMP-index" ) ) || die "Cannot save the current worktree state" which creates a new index file with the updates, and writes the tree from that. We have this logic where we compare the timestamp of the index with the timestamp of the files and we then write them out "smudged" if they are the same, and it basically depends on the fact that the date on the index file is compared with the date encoded in the stat information itself. And what is going on is: - we create a new index file with that "cp". We are careful to preserve the timestamps by using "-p", so this one should be all ok. - then we *update* that index by resetting it to the tree with git read-tree, but now we do *not* preserve the timestamp on this new copy any more, even though we copy over all the timestamps on the files that are indexed from the stat information! Now, we always had that problem when re-writing the index, but we had this clever workaround in the writing part: if the source had racily clean entries, then when we wrote those out (and thus can't depend on the index fiel timestamp showing that they are racily clean any more!), we would smudge them when writing. IOW, we handle this issue by having write_index() do this: for (i = 0; i < entries; i++) { ... if (is_racy_timestamp(istate, ce)) ce_smudge_racily_clean_entry(ce); .. when writing out entries. And that all took care of it, because now when we wrote the new index, we'd change the timestamp on the index, yes, but we'd smudge the entries we wrote out, so now the resulting index would still show that file as not-up-to-date any more. But with commit 34110cd4e394e3f92c01a4709689b384c34645d8 ("Make 'unpack_trees()' have a separate source and destination index"), this logic no longer triggers, because we now write out the "result" index, and that one never got its timestamp updated from the source index, so it had lost all that "is_racy_timestamp()" information! This trivial patch fixes it. It looks trivial, and it's a simple fix, but boy did it take me way too much thinking and explaining to myself to explain why there was a problem in the first place! The trivial fix is to just copy the index timestamp from the source index into the result index. But we only do this if we *have* a source index, of course, and if we will even bother to use the result. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-03-13read-tree() and unpack_trees(): use consistent limitLibravatar Junio C Hamano1-3/+3
read-tree -m can read up to MAX_TREES, which was arbitrarily set to 8 since August 2007 (4 is needed to deal with 2 merge-base case). However, the updated unpack_trees() code had an advertised limit of 4 (which it enforced). In reality the code was prepared to take only 3 trees and giving 4 caused it to stomp on its stack. Rename the MAX_TREES constant to MAX_UNPACK_TREES, move it to the unpack-trees.h common header file, and use it from both places to avoid future confusion. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-03-10unpack_trees(): fix diff-index regression.Libravatar Linus Torvalds1-2/+0
When skip_unmerged option is not given, unpack_trees() should not just skip unmerged cache entries but keep them in the result for the caller to sort them out. For callers other than diff-index, the incoming index should never be unmerged, but diff-index is a special case caller. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-03-10traverse_trees_recursive(): propagate merge errors upLibravatar Junio C Hamano1-3/+4
There were few places where merge errors detected deeper in the call chain were ignored and not propagated up the callchain to the caller. Most notably, this caused switching branches with "git checkout" to ignore a path modified in a work tree are different between the HEAD version and the commit being switched to, which it internally notices but ignores it, resulting in an incorrect two-way merge and loss of the change in the work tree. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-03-09unpack_trees(): minor memory leak fix in unused destination indexLibravatar Linus Torvalds1-0/+1
This adds a "discard_index(&o->result)" to the failure path, to reclaim memory from an in-core index we built but ended up not using. The *big* memory leak comes from the fact that we leak the cache_entry things left and right. That's a very traditional and deliberate leak: because we used to build up the cache entries by just mapping them directly in from the index file (and we emulate that in modern times by allocating them from one big array), we can't actually free them one-by-one. So doing the "discard_index()" will free the hash tables etc, which is good, and it will free the "istate->alloc" but that is never set on the result because we don't get the result from the index read. So we don't actually free the individual cache entries themselves that got created from the trees. That's not something new, btw. We never did. But some day we should just add a flag to the cache_entry() that it's a "free one by one" kind, and then we could/should do it. In the meantime, this one-liner will fix *some* of the memory leaks, but not that old traditional one. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-03-09Make 'unpack_trees()' have a separate source and destination indexLibravatar Linus Torvalds1-77/+72
We will always unpack into our own internal index, but we will take the source from wherever specified, and we will optionally write the result to a specified index (optionally, because not everybody even _wants_ any result: the index diffing really wants to just walk the tree and index in parallel). This ends up removing a fair number more lines than it adds, for the simple reason that we can now skip all the crud that tried to be oh-so-careful about maintaining our position in the index as we were traversing and modifying it. Since we don't actually modify the source index any more, we can just update the 'o->pos' pointer without worrying about whether an index entry got removed or replaced or added to. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-03-09Make 'unpack_trees()' take the index to work on as an argumentLibravatar Linus Torvalds1-39/+40
This is just a very mechanical conversion, and makes everybody set it to '&the_index' before calling, but at least it makes it more explicit where we work with the index. The next stage would be to split that index usage up into a 'source' and a 'destination' index, so that we can unpack into a different index than we started out from. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-03-09Move 'unpack_trees()' over to 'traverse_trees()' interfaceLibravatar Linus Torvalds1-281/+249
This not only deletes more code than it adds, it gets rid of a singularly hard-to-understand function (unpack_trees_rec()), and replaces it with a set of smaller and simpler functions that use the generic tree traversal mechanism to walk over one or more git trees in parallel. It's still not the most wonderful interface, and by no means is the new code easy to understand either, but it's at least a bit less opaque. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-02-27Merge branch 'db/checkout'Libravatar Junio C Hamano1-56/+98
* db/checkout: (21 commits) checkout: error out when index is unmerged even with -m checkout: show progress when checkout takes long time while switching branches Add merge-subtree back checkout: updates to tracking report builtin-checkout.c: Remove unused prefix arguments in switch_branches path checkout: work from a subdirectory checkout: tone down the "forked status" diagnostic messages Clean up reporting differences on branch switch builtin-checkout.c: fix possible usage segfault checkout: notice when the switched branch is behind or forked Build in checkout Move code to clean up after a branch change to branch.c Library function to check for unmerged index entries Use diff -u instead of diff in t7201 Move create_branch into a library file Build-in merge-recursive Add "skip_unmerged" option to unpack_trees. Discard "deleted" cache entries after using them to update the working tree Send unpack-trees debugging output to stderr Add flag to make unpack_trees() not print errors. ... Conflicts: Makefile
2008-02-24Be more verbose when checkout takes a long timeLibravatar Linus Torvalds1-1/+1
So I find it irritating when git thinks for a long time without telling me what's taking so long. And by "long time" I definitely mean less than two seconds, which is already way too long for me. This hits me when doing a large pull and the checkout takes a long time, or when just switching to another branch that is old and again checkout takes a while. Now, git read-tree already had support for the "-v" flag that does nice updates about what's going on, but it was delayed by two seconds, and if the thing had already done more than half by then it would be quiet even after that, so in practice it meant that we migth be quiet for up to four seconds. Much too long. So this patch changes the timeout to just one second, which makes it much more palatable to me. The other thing this patch does is that "git checkout" now doesn't disable the "-v" flag when doing its thing, and only disables the output when given the -q flag. When allowing "checkout -m" to fall back to a 3-way merge, the users will see the error message from straight "checkout", so we will tell them that we do fall back to make them look less scary. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-02-22Use helper function for copying index entry informationLibravatar Linus Torvalds1-1/+1
We used to just memcpy() the index entry when we copied the stat() and SHA1 hash information, which worked well enough back when the index entry was just an exact bit-for-bit representation of the information on disk. However, these days we actually have various management information in the cache entry too, and we should be careful to not overwrite it when we copy the stat information from another index entry. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-02-16Merge branch 'jc/gitignore-ends-with-slash'Libravatar Junio C Hamano1-1/+2
* jc/gitignore-ends-with-slash: gitignore: lazily find dtype gitignore(5): Allow "foo/" in ignore list to match directory "foo"
2008-02-09Add "skip_unmerged" option to unpack_trees.Libravatar Daniel Barkalow1-3/+17
This option allows the caller to reset everything that isn't unmerged, leaving the unmerged things to be resolved. If, after a merge of "working" and "HEAD", this is used with "HEAD" (reset, !update), the result will be that all of the changes from "local" are in the working tree but not added to the index (either with the index clean but unchanged, or with the index unmerged, depending on whether there are conflicts). This will be used in checkout -m. Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
2008-02-09Discard "deleted" cache entries after using them to update the working treeLibravatar Daniel Barkalow1-7/+9
Way back in read-tree.c, we used a mode 0 cache entry to indicate that an entry had been deleted, so that the update code would remove the working tree file, and we would just skip it when writing out the index file afterward. These days, unpack_trees is a library function, and it is still leaving these entries in the active cache. Furthermore, unpack_trees doesn't correctly ignore those entries, and who knows what other code wouldn't expect them to be there, but just isn't yet called after a call to unpack_trees. To avoid having other code trip over these entries, have check_updates() remove them after it removes the working tree files. While we're at it, simplify the loop in check_updates(), and avoid passing global variables as parameters to check_updates(): there is only one call site anyway. Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
2008-02-09Send unpack-trees debugging output to stderrLibravatar Daniel Barkalow1-7/+7
This is to keep git-stash from getting confused if you're debugging unpack-trees. Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
2008-02-09Add flag to make unpack_trees() not print errors.Libravatar Daniel Barkalow1-14/+29
(This applies only to errors where a plausible operation is impossible due to the particular data, not to errors resulting from misuse of the merge functions.) This will allow builtin-checkout to suppress merge errors if it's going to try more merging methods. Additionally, if unpack_trees() returns with an error, but without printing anything, it will roll back any changes to the index (by rereading the index, currently). This obviously could be done by the caller, but chances are that the caller would forget and debugging this is difficult. Also, future implementations may give unpack_trees() a more efficient way of undoing its changes than the caller could. Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
2008-02-09Allow callers of unpack_trees() to handle failureLibravatar Daniel Barkalow1-37/+48
Return an error from unpack_trees() instead of calling die(), and exit with an error in read-tree, builtin-commit, and diff-lib. merge-recursive already expected an error return from unpack_trees, so it doesn't need to be changed. The merge function can return negative to abort. This will be used in builtin-checkout -m. Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
2008-02-05gitignore: lazily find dtypeLibravatar Junio C Hamano1-1/+2
When we process "foo/" entries in gitignore files on a system that does not have d_type member in "struct dirent", the earlier implementation ran lstat(2) separately when matching with entries that came from the command line, in-tree .gitignore files, and $GIT_DIR/info/excludes file. This optimizes it by delaying the lstat(2) call until it becomes absolutely necessary. The initial idea for this change was by Jeff King, but I optimized it further to pass pointers to around. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2008-02-05gitignore(5): Allow "foo/" in ignore list to match directory "foo"Libravatar Junio C Hamano1-1/+1
A pattern "foo/" in the exclude list did not match directory "foo", but a pattern "foo" did. This attempts to extend the exclude mechanism so that it would while not matching a regular file or a symbolic link "foo". In order to differentiate a directory and non directory, this passes down the type of path being checked to excluded() function. A downside is that the recursive directory walk may need to run lstat(2) more often on systems whose "struct dirent" do not give the type of the entry; earlier it did not have to do so for an excluded path, but we now need to figure out if a path is a directory before deciding to exclude it. This is especially bad because an idea similar to the earlier CE_UPTODATE optimization to reduce number of lstat(2) calls would by definition not apply to the codepaths involved, as (1) directories will not be registered in the index, and (2) excluded paths will not be in the index anyway. Signed-off-by: Junio C Hamano <gitster@pobox.com>