Re: [PATCH v2 07/19] make sure partially read index is not changed

[Thomas Gummerer]

Duy Nguyen pclo...@gmail.com writes:

 On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer t.gumme...@gmail.com 
 wrote:
 A partially read index file currently cannot be written to disk.  Make
 sure that never happens, by erroring out when a caller tries to change a
 partially read index.  The caller is responsible for reading the whole
 index when it's trying to change it later.

 Forcing the caller to load the right part of the index file instead of
 re-reading it when changing it, gives a bit of a performance advantage,
 by avoiding to read parts of the index twice.

 If you want to be strict about this, put similar check in
 fill_stat_cache_info (used by entry.c), cache_tree_invalidate_path and
 convert the code base (maybe except unpack-trees.c, just put a check
 in the beginning of unpack_trees()) to use wrapper function to change
 ce_flags (some still update ce_flags directly, grep them). Some flags
 are in memory only and should be allowed to change in partial index,
 some are to be written on disk and should not be allowed.

I'm not sure anymore we should even be this strict.  A partially read
index will never make it to the disk, because write_index checks if it's
fully read.   I think the check in write_index and read_index_filtered
would be enough.

The only disadvantage would be that it takes a little longer to catch an
error that should never happen in the first place.  If it occurs the
user has bigger problems than the time that not having caught the
error earlier adds to the execution of the command.

I also don't see a clean way to add the check to fill_stat_cache_info or
cache_tree_invalidate_path, because we would need an additional
parameter for the index_state, or at least for index_state-filter_opts,
which doesn't do anything but check if the index is only partially loaded.
--
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 07/19] make sure partially read index is not changed

[Duy Nguyen]

On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer t.gumme...@gmail.com wrote:
 A partially read index file currently cannot be written to disk.  Make
 sure that never happens, by erroring out when a caller tries to change a
 partially read index.  The caller is responsible for reading the whole
 index when it's trying to change it later.

 Forcing the caller to load the right part of the index file instead of
 re-reading it when changing it, gives a bit of a performance advantage,
 by avoiding to read parts of the index twice.

If you want to be strict about this, put similar check in
fill_stat_cache_info (used by entry.c), cache_tree_invalidate_path and
convert the code base (maybe except unpack-trees.c, just put a check
in the beginning of unpack_trees()) to use wrapper function to change
ce_flags (some still update ce_flags directly, grep them). Some flags
are in memory only and should be allowed to change in partial index,
some are to be written on disk and should not be allowed.
-- 
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

[PATCH v2 07/19] make sure partially read index is not changed

[Thomas Gummerer]

A partially read index file currently cannot be written to disk.  Make
sure that never happens, by erroring out when a caller tries to change a
partially read index.  The caller is responsible for reading the whole
index when it's trying to change it later.

Forcing the caller to load the right part of the index file instead of
re-reading it when changing it, gives a bit of a performance advantage,
by avoiding to read parts of the index twice.

Signed-off-by: Thomas Gummerer t.gumme...@gmail.com
---
 builtin/update-index.c |  4 
 cache.h|  1 +
 read-cache-v2.c|  2 ++
 read-cache.c   | 10 ++
 4 files changed, 17 insertions(+)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 5c7762e..4c6e3a6 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -49,6 +49,8 @@ static int mark_ce_flags(const char *path, int flag, int mark)
int namelen = strlen(path);
int pos = cache_name_pos(path, namelen);
if (0 = pos) {
+   if (active_cache_partially_read)
+   die(BUG: Can't change a partially read index);
if (mark)
active_cache[pos]-ce_flags |= flag;
else
@@ -253,6 +255,8 @@ static void chmod_path(int flip, const char *path)
pos = cache_name_pos(path, strlen(path));
if (pos  0)
goto fail;
+   if (active_cache_partially_read)
+   die(BUG: Can't change a partially read index);
ce = active_cache[pos];
mode = ce-ce_mode;
if (!S_ISREG(mode))
diff --git a/cache.h b/cache.h
index d305d21..455b772 100644
--- a/cache.h
+++ b/cache.h
@@ -311,6 +311,7 @@ extern void free_name_hash(struct index_state *istate);
 #define active_alloc (the_index.cache_alloc)
 #define active_cache_changed (the_index.cache_changed)
 #define active_cache_tree (the_index.cache_tree)
+#define active_cache_partially_read (the_index.filter_opts)
 
 #define read_cache() read_index(the_index)
 #define read_cache_from(path) read_index_from(the_index, (path))
diff --git a/read-cache-v2.c b/read-cache-v2.c
index 51b618f..f3c0685 100644
--- a/read-cache-v2.c
+++ b/read-cache-v2.c
@@ -479,6 +479,8 @@ static int write_index_v2(struct index_state *istate, int 
newfd)
struct stat st;
struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
 
+   if (istate-filter_opts)
+   die(BUG: index: cannot write a partially read index);
for (i = removed = extended = 0; i  entries; i++) {
if (cache[i]-ce_flags  CE_REMOVE)
removed++;
diff --git a/read-cache.c b/read-cache.c
index 9053d43..ab716ed 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -30,6 +30,8 @@ static void replace_index_entry(struct index_state *istate, 
int nr, struct cache
 {
struct cache_entry *old = istate-cache[nr];
 
+   if (istate-filter_opts)
+   die(BUG: Can't change a partially read index);
remove_name_hash(istate, old);
set_index_entry(istate, nr, ce);
istate-cache_changed = 1;
@@ -467,6 +469,8 @@ int remove_index_entry_at(struct index_state *istate, int 
pos)
 {
struct cache_entry *ce = istate-cache[pos];
 
+   if (istate-filter_opts)
+   die(BUG: Can't change a partially read index);
record_resolve_undo(istate, ce);
remove_name_hash(istate, ce);
istate-cache_changed = 1;
@@ -973,6 +977,8 @@ int add_index_entry(struct index_state *istate, struct 
cache_entry *ce, int opti
 {
int pos;
 
+   if (istate-filter_opts)
+   die(BUG: Can't change a partially read index);
if (option  ADD_CACHE_JUST_APPEND)
pos = istate-cache_nr;
else {
@@ -1173,6 +1179,8 @@ int refresh_index(struct index_state *istate, unsigned 
int flags, const char **p
/* If we are doing --really-refresh that
 * means the index is not valid anymore.
 */
+   if (istate-filter_opts)
+   die(BUG: Can't change a partially read 
index);
ce-ce_flags = ~CE_VALID;
istate-cache_changed = 1;
}
@@ -1331,6 +1339,8 @@ int read_index_filtered_from(struct index_state *istate, 
const char *path,
void *mmap;
size_t mmap_size;
 
+   if (istate-filter_opts)
+   die(BUG: Can't re-read partially read index);
errno = EBUSY;
if (istate-initialized)
return istate-cache_nr;
-- 
1.8.3.453.g1dfc63d

--
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