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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Duy Nguyen <pclouds@xxxxxxxxx> writes:

> On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer <t.gummerer@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]