git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 15/24] read-cache: read index-v5

From
Duy Nguyen <pclouds@gmail.com>
Date
Aug 20, 2013, 14:01 UTC
Message-ID
<CACsJy8A1NpYa6nr6vHKLd-Tap3PObabfUB34U90MH_OdfQJN9Q@mail.gmail.com>
In-Reply-To
<1376854933-31241-16-git-send-email-t.gummerer@gmail.com>

General comment: a short comment before each function describing what the function does would be helpful. This only applies for complex functions (read_* ones). Of course verify_hdr does not require extra explanantion.

 On Mon, Aug 19, 2013 at 2:42 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 19 quoted lines
> +static struct directory_entry *directory_entry_from_ondisk(struct ondisk_directory_entry *ondisk,
> +                                                  const char *name,
> +                                                  size_t len)
> +{
> +       struct directory_entry *de = xmalloc(directory_entry_size(len));
> +
> +       memcpy(de->pathname, name, len);
> +       de->pathname[len] = '\0';
> +       de->de_flags      = ntoh_s(ondisk->flags);
> +       de->de_foffset    = ntoh_l(ondisk->foffset);
> +       de->de_cr         = ntoh_l(ondisk->cr);
> +       de->de_ncr        = ntoh_l(ondisk->ncr);
> +       de->de_nsubtrees  = ntoh_l(ondisk->nsubtrees);
> +       de->de_nfiles     = ntoh_l(ondisk->nfiles);
> +       de->de_nentries   = ntoh_l(ondisk->nentries);
> +       de->de_pathlen    = len;
> +       hashcpy(de->sha1, ondisk->sha1);
> +       return de;
> +}
This function leaves a lot of fields uninitialized..
Show 9 quoted lines
> +static struct directory_entry *read_directories(unsigned int *dir_offset,
> +                               unsigned int *dir_table_offset,
> +                               void *mmap,
> +                               int mmap_size)
> +{
> ....
> +       de = directory_entry_from_ondisk(disk_de, name, len);
> +       de->next = NULL;
> +       de->sub = NULL;

..and two of them are set to NULL here. Maybe directory_entry_from_ondisk() could be made to call init_directory_entry() instead so that we don't need to manually reset some fields here, which leaves me wondering why other fields are not important to reset. init_directory_entry() is introduced later in "write index-v5" patch, you so may want to move it up a few patches.

Show 13 quoted lines
> +static int read_entry(struct cache_entry **ce, char *pathname, size_t pathlen,
> +                     void *mmap, unsigned long mmap_size,
> +                     unsigned int first_entry_offset,
> +                     unsigned int foffsetblock)
> +{
> +       int len, offset_to_offset;
> +       char *name;
> +       uint32_t foffsetblockcrc, *filecrc, *beginning, *end, entry_offset;
> +       struct ondisk_cache_entry *disk_ce;
> +
> +       beginning = ptr_add(mmap, foffsetblock);
> +       end = ptr_add(mmap, foffsetblock + 4);
> +       len = ntoh_l(*end) - ntoh_l(*beginning) - sizeof(struct ondisk_cache_entry) - 5;

It took me a while to check and figure out " - 5" here means minus NUL and the crc. A short comment would help. I think there's also another -5 in read_directories(). Or maybe just rename len to namelen.

Show 14 quoted lines
> +struct conflict_entry *create_new_conflict(char *name, int len, int pathlen)
> +{
> +       struct conflict_entry *conflict_entry;
> +
> +       if (pathlen)
> +               pathlen++;
> +       conflict_entry = xmalloc(conflict_entry_size(len));
> +       conflict_entry->entries = NULL;
> +       conflict_entry->nfileconflicts = 0;
> +       conflict_entry->namelen = len;
> +       memcpy(conflict_entry->name, name, len);
> +       conflict_entry->name[len] = '\0';
> +       conflict_entry->pathlen = pathlen;
> +       conflict_entry->next = NULL;

A memset followed by memcpy and conflict_entry->pathlen = pathlen would make this shorter and won't miss new fields added in future.

Show 13 quoted lines
> +static int read_entries(struct index_state *istate, struct directory_entry *de,
> +                       unsigned int first_entry_offset, void *mmap,
> +                       unsigned long mmap_size, unsigned int *nr,
> +                       unsigned int foffsetblock)
> +{
> +       struct cache_entry *ce;
> +       int i, subdir = 0;
> +
> +       for (i = 0; i < de->de_nfiles; i++) {
> +               unsigned int subdir_foffsetblock = de->de_foffset + foffsetblock + (i * 4);
> +               if (read_entry(&ce, de->pathname, de->de_pathlen, mmap, mmap_size,
> +                              first_entry_offset, subdir_foffsetblock) < 0)
> +                       return -1;
You read one file entry, say abc/def...
Show 5 quoted lines
> +               while (subdir < de->de_nsubtrees &&
> +                      cache_name_compare(ce->name + de->de_pathlen,
> +                                         ce_namelen(ce) - de->de_pathlen,
> +                                         de->sub[subdir]->pathname + de->de_pathlen,
> +                                         de->sub[subdir]->de_pathlen - de->de_pathlen) > 0) {
Oh right the entry belongs the the substree "abc" so..
> +                       read_entries(istate, de->sub[subdir], first_entry_offset, mmap,
> +                                    mmap_size, nr, foffsetblock);
you recurse in, which will add following entries like abc/def and abc/xyz...
Show 5 quoted lines
> +                       subdir++;
> +               }
> +               if (!ce)
> +                       continue;
> +               set_index_entry(istate, (*nr)++, ce);

then back here after recusion and add abc/def, again, after abc/xyz. Did I read this code correctly?

Show 8 quoted lines
> +       }
> +       for (i = subdir; i < de->de_nsubtrees; i++) {
> +               read_entries(istate, de->sub[i], first_entry_offset, mmap,
> +                            mmap_size, nr, foffsetblock);
> +       }
> +       return 0;
> +}
> +
Show 15 quoted lines
> +static int read_index_v5(struct index_state *istate, void *mmap,
> +                        unsigned long mmap_size, struct filter_opts *opts)
> +{
> +       unsigned int entry_offset, ndirs, foffsetblock, nr = 0;
> +       struct directory_entry *root_directory, *de, *last_de;
> +       const char **paths = NULL;
> +       struct pathspec adjusted_pathspec;
> +       int need_root = 0, i;
> +
> +       root_directory = read_all_directories(istate, &entry_offset,
> +                                             &foffsetblock, &ndirs,
> +                                             mmap, mmap_size);
> +
> +       if (opts && opts->pathspec && opts->pathspec->nr) {
> +               need_root = 0;
need_root is already initialized at declaration.
Show 38 quoted lines
> +               paths = xmalloc((opts->pathspec->nr + 1)*sizeof(char *));
> +               paths[opts->pathspec->nr] = NULL;
> +               for (i = 0; i < opts->pathspec->nr; i++) {
> +                       char *super = strdup(opts->pathspec->items[i].match);
> +                       int len = strlen(super);
> +                       while (len && super[len - 1] == '/' && super[len - 2] == '/')
> +                               super[--len] = '\0'; /* strip all but one trailing slash */
> +                       while (len && super[--len] != '/')
> +                               ; /* scan backwards to next / */
> +                       if (len >= 0)
> +                               super[len--] = '\0';
> +                       if (len <= 0) {
> +                               need_root = 1;
> +                               break;
> +                       }
> +                       paths[i] = super;
> +               }
> +       }
> +
> +       if (!need_root)
> +               parse_pathspec(&adjusted_pathspec, PATHSPEC_ALL_MAGIC, PATHSPEC_PREFER_CWD, NULL, paths);
> +
> +       de = root_directory;
> +       last_de = de;
> +       while (de) {
> +               if (need_root ||
> +                   match_pathspec_depth(&adjusted_pathspec, de->pathname, de->de_pathlen, 0, NULL)) {
> +                       if (read_entries(istate, de, entry_offset,
> +                                        mmap, mmap_size, &nr,
> +                                        foffsetblock) < 0)
> +                               return -1;
> +               } else {
> +                       for (i = 0; i < de->de_nsubtrees; i++) {
> +                               last_de->next = de->sub[i];
> +                               last_de = last_de->next;
> +                       }
> +               }
> +               de = de->next;

I'm missing something here. read_entries is a function that reads all entries inside "de" including subdirectories and the first "de" is root_directory, which makes it read the whole index in. Because de->next is only set in this function, de->next after read_entries() is NULL, which termintates the loop and the else block never runs. It does not sound right..

-- 
Duy
Previous: Eric SunshineNext: Thomas Gummerer
Message 28 of 55 in “Index-v5”
  1. 00/24 Index-v5Thomas Gummerer, Aug 18, 2013
  2. 01/24 t2104: Don't fail for index versions other than [23]Thomas Gummerer, Aug 18, 2013
  3. 02/24 read-cache: use fixed width integer typesThomas Gummerer, Aug 18, 2013
  4. Eric SunshineAug 18, 2013
  5. Junio C HamanoAug 20, 2013
  6. Thomas GummererAug 21, 2013
  7. 03/24 read-cache: split index file version specific functionalityThomas Gummerer, Aug 18, 2013
  8. 04/24 read-cache: clear version in discard_index()Thomas Gummerer, Aug 18, 2013
  9. Junio C HamanoAug 20, 2013
  10. Thomas GummererAug 21, 2013
  11. 05/24 read-cache: move index v2 specific functions to their own fileThomas Gummerer, Aug 18, 2013
  12. 06/24 read-cache: Don't compare uid, gid and ino on cygwinThomas Gummerer, Aug 18, 2013
  13. Ramsay JonesAug 18, 2013
  14. Thomas GummererAug 20, 2013
  15. 07/24 read-cache: Re-read index if index file changedThomas Gummerer, Aug 18, 2013
  16. 08/24 add documentation for the index apiThomas Gummerer, Aug 18, 2013
  17. Eric SunshineAug 18, 2013
  18. 09/24 read-cache: add index reading apiThomas Gummerer, Aug 18, 2013
  19. 10/24 make sure partially read index is not changedThomas Gummerer, Aug 18, 2013
  20. Eric SunshineAug 18, 2013
  21. Thomas GummererAug 20, 2013
  22. 11/24 grep.c: use index apiThomas Gummerer, Aug 18, 2013
  23. 12/24 ls-files.c: use index apiThomas Gummerer, Aug 18, 2013
  24. 13/24 documentation: add documentation of the index-v5 file formatThomas Gummerer, Aug 18, 2013
  25. 14/24 read-cache: make in-memory format aware of stat_crcThomas Gummerer, Aug 18, 2013
  26. 15/24 read-cache: read index-v5Thomas Gummerer, Aug 18, 2013
  27. Eric SunshineAug 19, 2013
  28. Duy NguyenAug 20, 2013
  29. Thomas GummererAug 20, 2013
  30. Duy NguyenAug 21, 2013
  31. Duy NguyenAug 20, 2013
  32. Thomas GummererAug 20, 2013
  33. Duy NguyenAug 23, 2013
  34. 16/24 read-cache: read resolve-undo dataThomas Gummerer, Aug 18, 2013
  35. Eric SunshineAug 19, 2013
  36. 17/24 read-cache: read cache-tree in index-v5Thomas Gummerer, Aug 18, 2013
  37. Duy NguyenAug 24, 2013
  38. Thomas GummererNov 25, 2013
  39. 18/24 read-cache: write index-v5Thomas Gummerer, Aug 18, 2013
  40. Duy NguyenAug 24, 2013
  41. Thomas GummererNov 25, 2013
  42. Duy NguyenAug 24, 2013
  43. Duy NguyenAug 24, 2013
  44. 19/24 read-cache: write index-v5 cache-tree dataThomas Gummerer, Aug 18, 2013
  45. 20/24 read-cache: write resolve-undo data for index-v5Thomas Gummerer, Aug 18, 2013
  46. 21/24 update-index.c: rewrite index when index-version is givenThomas Gummerer, Aug 18, 2013
  47. 22/24 p0003-index.sh: add perf test for the index formatsThomas Gummerer, Aug 18, 2013
  48. 23/24 introduce GIT_INDEX_VERSION environment variableThomas Gummerer, Aug 18, 2013
  49. Duy NguyenAug 21, 2013
  50. Thomas GummererAug 21, 2013
  51. 24/24 test-lib: allow setting the index format versionThomas Gummerer, Aug 18, 2013
  52. Duy NguyenAug 24, 2013
  53. Junio C HamanoAug 25, 2013
  54. Duy NguyenAug 25, 2013
  55. Thomas GummererAug 31, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.