1. 08 Oct, 2011 1 commit
    • Jeff King's avatar
      fix phantom untracked files when core.ignorecase is set · 2548183b
      Jeff King authored
      When core.ignorecase is turned on and there are stale index
      entries, "git commit" can sometimes report directories as
      untracked, even though they contain tracked files.
      
      You can see an example of this with:
      
          # make a case-insensitive repo
          git init repo && cd repo &&
          git config core.ignorecase true &&
      
          # with some tracked files in a subdir
          mkdir subdir &&
          > subdir/one &&
          > subdir/two &&
          git add . &&
          git commit -m base &&
      
          # now make the index entries stale
          touch subdir/* &&
      
          # and then ask commit to update those entries and show
          # us the status template
          git commit -a
      
      which will report "subdir/"  as untracked, even though it
      clearly contains two tracked files. What is happening in the
      commit program is this:
      
        1. We load the index, and for each entry, insert it into the index's
           name_hash. In addition, if ignorecase is turned on, we make an
           entry in the name_hash for the directory (e.g., "contrib/"), which
           uses the following code from 5102c617's hash_index_entry_directories:
      
              hash = hash_name(ce->name, ptr - ce->name);
              if (!lookup_hash(hash, &istate->name_hash)) {
                      pos = insert_hash(hash, &istate->name_hash);
      		if (pos) {
      			ce->next = *pos;
      			*pos = ce;
      		}
              }
      
           Note that we only add the directory entry if there is not already an
           entry.
      
        2. We run add_files_to_cache, which gets updated information for each
           cache entry. It helpfully inserts this information into the cache,
           which calls replace_index_entry. This in turn calls
           remove_name_hash() on the old entry, and add_name_hash() on the new
           one. But remove_name_hash doesn't actually remove from the hash, it
           only marks it as "no longer interesting" (from cache.h):
      
            /*
             * We don't actually *remove* it, we can just mark it invalid so that
             * we won't find it in lookups.
             *
             * Not only would we have to search the lists (simple enough), but
             * we'd also have to rehash other hash buckets in case this makes the
             * hash bucket empty (common). So it's much better to just mark
             * it.
             */
            static inline void remove_name_hash(struct cache_entry *ce)
            {
                    ce->ce_flags |= CE_UNHASHED;
            }
      
           This is OK in the specific-file case, since the entries in the hash
           form a linked list, and we can just skip the "not here anymore"
           entries during lookup.
      
           But for the directory hash entry, we will _not_ write a new entry,
           because there is already one there: the old one that is actually no
           longer interesting!
      
        3. While traversing the directories, we end up in the
           directory_exists_in_index_icase function to see if a directory is
           interesting. This in turn checks index_name_exists, which will
           look up the directory in the index's name_hash. We see the old,
           deleted record, and assume there is nothing interesting. The
           directory gets marked as untracked, even though there are index
           entries in it.
      
      The problem is in the code I showed above:
      
              hash = hash_name(ce->name, ptr - ce->name);
              if (!lookup_hash(hash, &istate->name_hash)) {
                      pos = insert_hash(hash, &istate->name_hash);
      		if (pos) {
      			ce->next = *pos;
      			*pos = ce;
      		}
              }
      
      Having a single cache entry that represents the directory is
      not enough; that entry may go away if the index is changed.
      It may be tempting to say that the problem is in our removal
      method; if we removed the entry entirely instead of simply
      marking it as "not here anymore", then we would know we need
      to insert a new entry. But that only covers this particular
      case of remove-replace. In the more general case, consider
      something like this:
      
        1. We add "foo/bar" and "foo/baz" to the index. Each gets
           their own entry in name_hash, plus we make a "foo/"
           entry that points to "foo/bar".
      
        2. We remove the "foo/bar" entry from the index, and from
           the name_hash.
      
        3. We ask if "foo/" exists, and see no entry, even though
           "foo/baz" exists.
      
      So we need that directory entry to have the list of _all_
      cache entries that indicate that the directory is tracked.
      So that implies making a linked list as we do for other
      entries, like:
      
        hash = hash_name(ce->name, ptr - ce->name);
        pos = insert_hash(hash, &istate->name_hash);
        if (pos) {
      	  ce->next = *pos;
      	  *pos = ce;
        }
      
      But that's not right either. In fact, it shows a second bug
      in the current code, which is that the "ce->next" pointer is
      supposed to be linking entries for a specific filename
      entry, but here we are overwriting it for the directory
      entry. So the same cache entry ends up in two linked lists,
      but they share the same "next" pointer.
      
      As it turns out, this second bug can't be triggered in the
      current code. The "if (pos)" conditional is totally dead
      code; pos will only be non-NULL if there was an existing
      hash entry, and we already checked that there wasn't one
      through our call to lookup_hash.
      
      But fixing the first bug means taking out that call to
      lookup_hash, which is going to activate the buggy dead code,
      and we'll end up splicing the two linked lists together.
      
      So we need to have a separate next pointer for the list in
      the directory bucket, and we need to traverse that list in
      index_name_exists when we are looking up a directory.
      
      This bloats "struct cache_entry" by a few bytes. Which is
      annoying, because it's only necessary when core.ignorecase
      is enabled. There's not an easy way around it, short of
      separating out the "next" pointers from cache_entry entirely
      (i.e., having a separate "cache_entry_list" struct that gets
      stored in the name_hash). In practice, it probably doesn't
      matter; we have thousands of cache entries, compared to the
      millions of objects (where adding 4 bytes to the struct
      actually does impact performance).
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      2548183b
  2. 26 Jun, 2011 2 commits
  3. 24 Jun, 2011 2 commits
  4. 22 Jun, 2011 3 commits
  5. 21 Jun, 2011 2 commits
  6. 20 Jun, 2011 3 commits
  7. 19 Jun, 2011 3 commits
  8. 17 Jun, 2011 4 commits
    • Jeff King's avatar
      tests: link shell libraries into valgrind directory · 36bfb0e5
      Jeff King authored
      When we run tests under valgrind, we symlink anything
      executable that starts with git-* or test-* into a special
      valgrind bin directory, and then make that our
      GIT_EXEC_PATH.
      
      However, shell libraries like git-sh-setup do not have the
      executable bit marked, and did not get symlinked.  This
      means that any test looking for shell libraries in our
      exec-path would fail to find them, even though that is a
      fine thing to do when testing against a regular git build
      (or in a git install, for that matter).
      
      t2300 demonstrated this problem. The fix is to symlink these
      shell libraries directly into the valgrind directory.
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      36bfb0e5
    • Jeff King's avatar
      t/Makefile: pass test opts to valgrind target properly · 7ef4d6b9
      Jeff King authored
      The valgrind target just reinvokes make with GIT_TEST_OPTS
      set to "--valgrind". However, it does this using an
      environment variable, which means GIT_TEST_OPTS in your
      config.mak would override it, and "make valgrind" would
      simply run the test suite without valgrind on.
      
      Instead, we should pass GIT_TEST_OPTS on the command-line,
      overriding what's in config.mak, and take care to append to
      whatever the user has there already.
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      7ef4d6b9
    • Junio C Hamano's avatar
      Merge branch 'ab/i18n-scripts-basic' · 179aae51
      Junio C Hamano authored
      * ab/i18n-scripts-basic:
        sh-i18n--envsubst.c: do not #include getopt.h
      179aae51
    • Brandon Casey's avatar
      sh-i18n--envsubst.c: do not #include getopt.h · 7c1fdd70
      Brandon Casey authored
      The getopt.h header file is not used.  It's inclusion is left over from the
      original version of this source.  Additionally, getopt.h does not exist on
      all platforms (SunOS 5.7) and will cause a compilation failure.  So, let's
      remove it.
      Signed-off-by: default avatarBrandon Casey <casey@nrlssc.navy.mil>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      7c1fdd70
  9. 16 Jun, 2011 2 commits
  10. 09 Jun, 2011 4 commits
  11. 07 Jun, 2011 2 commits
  12. 06 Jun, 2011 4 commits
  13. 05 Jun, 2011 1 commit
  14. 03 Jun, 2011 1 commit
  15. 02 Jun, 2011 2 commits
  16. 01 Jun, 2011 4 commits