Re: [PATCH v2 03/19] read-cache: move index v2 specific functions to their own file

2013-07-19 Thread Thomas Gummerer
Duy Nguyen pclo...@gmail.com writes:

 On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer t.gumme...@gmail.com 
 wrote:
 @@ -489,8 +479,8 @@ extern void *read_blob_data_from_index(struct 
 index_state *, const char *, unsig
  #define CE_MATCH_RACY_IS_DIRTY 02
  /* do stat comparison even if CE_SKIP_WORKTREE is true */
  #define CE_MATCH_IGNORE_SKIP_WORKTREE  04
 -extern int ie_match_stat(const struct index_state *, const struct 
 cache_entry *, struct stat *, unsigned int);
 -extern int ie_modified(const struct index_state *, const struct cache_entry 
 *, struct stat *, unsigned int);
 +extern int ie_match_stat(struct index_state *, const struct cache_entry *, 
 struct stat *, unsigned int);
 +extern int ie_modified(struct index_state *, const struct cache_entry *, 
 struct stat *, unsigned int);


 I would rather we keep const struct index_state* if we could. I
 tried putting const back and found that ce_match_stat_basic() calls
 set_istate_ops(), which writes to struct index_state. Putting
 set_istate_ops() in ce_match_stat_basic() may seem convenient, but
 does not make much sense (why would a match_stat function update
 index_ops?). I think you could move it out and

  - read_index calls set_istate_ops
  - (bonus) discard_index probably should reset version field to zero
 and clear internal_ops
  - the callers that use index without read_index must call
 initialize_index() or something, which in turn calls set_istate_ops.
 initialize_index() may take the preferred index version
  - do not let update-index modifies version field directly when
 --index-version is given. Wrap it with set_index_version() or
 something, so we can do internal conversion from one version to
 another if needed
  - remove set_istate_ops in write_index(), we may need internal_ops
 long before writing. When write_index is called, internal_ops should
 be already initialized

Ok, this makes sense.  The only thing that I'm a little worried about is
catching all code paths that need to initialize the index.  I'll
implement these suggestions in the re-roll.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 03/19] read-cache: move index v2 specific functions to their own file

2013-07-13 Thread Duy Nguyen
On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer t.gumme...@gmail.com wrote:
 @@ -489,8 +479,8 @@ extern void *read_blob_data_from_index(struct index_state 
 *, const char *, unsig
  #define CE_MATCH_RACY_IS_DIRTY 02
  /* do stat comparison even if CE_SKIP_WORKTREE is true */
  #define CE_MATCH_IGNORE_SKIP_WORKTREE  04
 -extern int ie_match_stat(const struct index_state *, const struct 
 cache_entry *, struct stat *, unsigned int);
 -extern int ie_modified(const struct index_state *, const struct cache_entry 
 *, struct stat *, unsigned int);
 +extern int ie_match_stat(struct index_state *, const struct cache_entry *, 
 struct stat *, unsigned int);
 +extern int ie_modified(struct index_state *, const struct cache_entry *, 
 struct stat *, unsigned int);


I would rather we keep const struct index_state* if we could. I
tried putting const back and found that ce_match_stat_basic() calls
set_istate_ops(), which writes to struct index_state. Putting
set_istate_ops() in ce_match_stat_basic() may seem convenient, but
does not make much sense (why would a match_stat function update
index_ops?). I think you could move it out and

 - read_index calls set_istate_ops
 - (bonus) discard_index probably should reset version field to zero
and clear internal_ops
 - the callers that use index without read_index must call
initialize_index() or something, which in turn calls set_istate_ops.
initialize_index() may take the preferred index version
 - do not let update-index modifies version field directly when
--index-version is given. Wrap it with set_index_version() or
something, so we can do internal conversion from one version to
another if needed
 - remove set_istate_ops in write_index(), we may need internal_ops
long before writing. When write_index is called, internal_ops should
be already initialized
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html