Re: [PATCH v2 06/19] read-cache: add index reading api

From: Duy Nguyen <pclouds@gmail.com>
Date: Jul 14, 2013, 03:21 UTC
Message-ID: <CACsJy8A7T8KRuTRoC9-2RXR21gWsgPt1uA6xa6BhjK2k3gF2CA@mail.gmail.com>
In-Reply-To: <1373650024-3001-7-git-send-email-t.gummerer@gmail.com>

On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:

Show 8 quoted lines

> @@ -238,7 +245,6 @@ static int read_index_v2(struct index_state *istate, void *mmap,
>                 disk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);
>                 ce = create_from_disk(disk_ce, &consumed, previous_name);
>                 set_index_entry(istate, i, ce);
> -
>                 src_offset += consumed;
>         }
>         strbuf_release(&previous_name_buf);

Nit pick. This is unnecessary.

> +int for_each_index_entry_v2(struct index_state *istate, each_cache_entry_fn fn, void *cb_data)
> +{
> +       int i, ret = 0;
> +       struct filter_opts *opts= istate->filter_opts;

Nitpick. space before "=".

Show 10 quoted lines

> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -996,6 +996,7 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti
>                 memmove(istate->cache + pos + 1,
>                         istate->cache + pos,
>                         (istate->cache_nr - pos - 1) * sizeof(ce));
> +
>         set_index_entry(istate, pos, ce);
>         istate->cache_changed = 1;
>         return 0;

NP: unnecessary change.

Show 19 quoted lines

> +int set_internal_ops(struct index_state *istate)
> +{
> +       if (!istate->internal_ops && istate->cache)
> +               istate->internal_ops = &v2_internal_ops;
> +       if (!istate->internal_ops)
> +               return 0;
> +       return 1;
> +}
> +
> +/*
> + * Execute fn for each index entry which is currently in istate.  Data
> + * can be given to the function using the cb_data parameter.
> + */
> +int for_each_index_entry(struct index_state *istate, each_cache_entry_fn fn, void *cb_data)
> +{
> +       if (!set_internal_ops(istate))
> +               return 0;
> +       return istate->internal_ops->for_each_index_entry(istate, fn, cb_data);
>  }

set_internal_ops should have been called in read_index and initialize_index.

Show 7 quoted lines

> @@ -1374,6 +1409,7 @@ int discard_index(struct index_state *istate)
>         free(istate->cache);
>         istate->cache = NULL;
>         istate->cache_alloc = 0;
> +       istate->filter_opts = NULL;
>         return 0;
>  }

Why is internal_ops not reset here?

Show 16 quoted lines

> --- a/read-cache.h
> +++ b/read-cache.h
> @@ -24,11 +24,17 @@ struct cache_version_header {
>  struct index_ops {
>         int (*match_stat_basic)(const struct cache_entry *ce, struct stat *st, int changed);
>         int (*verify_hdr)(void *mmap, unsigned long size);
> -       int (*read_index)(struct index_state *istate, void *mmap, unsigned long mmap_size);
> +       int (*read_index)(struct index_state *istate, void *mmap, unsigned long mmap_size,
> +                         struct filter_opts *opts);
>         int (*write_index)(struct index_state *istate, int newfd);
>  };
>
> +struct internal_ops {
> +       int (*for_each_index_entry)(struct index_state *istate, each_cache_entry_fn fn, void *cb_data);
> +};
> +

I wonder if we do need separate internal_ops from index_ops. Why not merge internal_ops in index_ops? -- Duy