{"thread":{"id":"34609","subject":"[PATCH/RFC] add: support saving the last <n> versions of the index","startedAt":"2013-08-04T06:28:40Z","lastAt":"2013-08-09T13:32:49Z","messageCount":4,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224547","messageId":"1375597720-13236-1-git-send-email-pclouds@gmail.com","threadId":"34609","inReplyTo":null,"subject":"[PATCH/RFC] add: support saving the last <n> versions of the index","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-04T06:28:40Z","receivedAt":"2013-08-04T06:28:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When you do \"git add foo\", change \"foo\" and \"git add foo\" again, the\nprevious foo version is still in the repository, but its SHA-1 may be\nlost. \"git fsck --lost-found\" may help, but it may take more time to\nfind out which blob is the old \"foo\".\n\nThis patch adds support for saving old index files, so that people\ncould look back more easily. In the case of \"old foo\", the user could\nuse ls-files with the previous of the index to determine SHA-1 of \"old\nfoo\".\n\nWhen core.indexlogsize is defined to <n>, every time the index\nchanges(**), the old version is stored as $GIT_DIR/index-<i>, where\n<i> is from 0..<n>-1. <i> increased after every change and wrapped\naround at <n>-1. The current <i> is stored in $GIT_DIR/index as new\nILOG extension. ILOG extension also contains HEAD's tree SHA-1 and a\nline describing the change.\n\n(**) The index can change in many ways, but only changes by \"git add\"\n(and maybe \"git update-index\" as well) are backed up. Non-SHA1 updates\nare not worth watching. SHA-1 changes that end up in a commit\nare tracked by reflog already. \"git mv\" and \"git rm\" may be worth\nwatching too.\n---\n http://thread.gmane.org/gmane.comp.version-control.git/231594 reminds\n me that I have had something like this for a long time but never\n finished it. So here we go again.\n\n I think the basic idea is ok (new index versions like v5 may have\n problems though). The UI is harder. We need something like \"git\n reflog\" and perhaps something like @{N} syntax to access old indexes.\n Being able to diff between two indexes are even better but that might\n be a lot of work, diff-ing \"git ls-files --stage\" output should be\n ok. Hmm?\n\n builtin/add.c |  9 ++++++++-\n cache.h       |  5 +++++\n config.c      |  5 +++++\n environment.c |  1 +\n lockfile.c    | 12 +++++++++++-\n read-cache.c  | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 6 files changed, 85 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 8266a9c..90e580c 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -15,6 +15,7 @@\n #include \"diffcore.h\"\n #include \"revision.h\"\n #include \"bulk-checkin.h\"\n+#include \"quote.h\"\n \n static const char * const builtin_add_usage[] = {\n \tN_(\"git add [options] [--] <pathspec>...\"),\n@@ -454,9 +455,15 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tchar *seen = NULL;\n \tint implicit_dot = 0;\n \tstruct update_callback_data update_data;\n+\tstruct strbuf log = STRBUF_INIT;\n \n \tgit_config(add_config, NULL);\n \n+\tif (index_log_size) {\n+\t\tstrbuf_addf(&log, \"[%s] add \", prefix ? prefix : \"\");\n+\t\tsq_quote_argv(&log, argv, 0);\n+\t}\n+\n \targc = parse_options(argc, argv, prefix, builtin_add_options,\n \t\t\t  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);\n \tif (patch_interactive)\n@@ -600,7 +607,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n  finish:\n \tif (active_cache_changed) {\n-\t\tif (write_cache(newfd, active_cache, active_nr) ||\n+\t\tif (write_index_with_log(&the_index, newfd, log.buf) ||\n \t\t    commit_locked_index(&lock_file))\n \t\t\tdie(_(\"Unable to write new index file\"));\n \t}\ndiff --git a/cache.h b/cache.h\nindex 85b544f..7655c4b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -274,9 +274,12 @@ struct index_state {\n \tstruct cache_tree *cache_tree;\n \tstruct cache_time timestamp;\n \tunsigned name_hash_initialized : 1,\n+\t\t log_updated : 1,\n \t\t initialized : 1;\n \tstruct hash_table name_hash;\n \tstruct hash_table dir_hash;\n+\tunsigned int log_index;\n+\tstruct strbuf *log;\n };\n \n extern struct index_state the_index;\n@@ -454,6 +457,7 @@ extern int read_index_from(struct index_state *, const char *path);\n extern int is_index_unborn(struct index_state *);\n extern int read_index_unmerged(struct index_state *);\n extern int write_index(struct index_state *, int newfd);\n+extern int write_index_with_log(struct index_state *, int newfd, const char *log);\n extern int discard_index(struct index_state *);\n extern int unmerged_index(const struct index_state *);\n extern int verify_path(const char *path);\n@@ -595,6 +599,7 @@ extern int fsync_object_files;\n extern int core_preload_index;\n extern int core_apply_sparse_checkout;\n extern int precomposed_unicode;\n+extern unsigned long index_log_size;\n \n /*\n  * The character that begins a commented line in user-editable file\ndiff --git a/config.c b/config.c\nindex e13a7b6..f5e40fb 100644\n--- a/config.c\n+++ b/config.c\n@@ -831,6 +831,11 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.indexlogsize\")) {\n+\t\tindex_log_size = git_config_ulong(var, value);\n+\t\treturn 0;\n+\t}\n+\n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn 0;\n }\ndiff --git a/environment.c b/environment.c\nindex 5398c36..12f01f7 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -62,6 +62,7 @@ int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n struct startup_info *startup_info;\n unsigned long pack_size_limit_cfg;\n+unsigned long index_log_size;\n \n /*\n  * The character that begins a commented line in user-editable file\ndiff --git a/lockfile.c b/lockfile.c\nindex 8fbcb6a..880a4e4 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -267,8 +267,18 @@ int commit_locked_index(struct lock_file *lk)\n \t\tlk->filename[0] = 0;\n \t\treturn 0;\n \t}\n-\telse\n+\telse {\n+\t\tif (the_index.log_updated) {\n+\t\t\tchar src[PATH_MAX], backup[PATH_MAX];\n+\t\t\tint len = strlen(lk->filename) - 5; /* .lock */\n+\t\t\tmemcpy(src, lk->filename, len);\n+\t\t\tsrc[len] = 0;\n+\t\t\tmemcpy(backup, lk->filename, len);\n+\t\t\tsprintf(backup+len, \"-%u\", the_index.log_index);\n+\t\t\trename(src, backup); /* does it matter if it fails? */\n+\t\t}\n \t\treturn commit_lock_file(lk);\n+\t}\n }\n \n void rollback_lock_file(struct lock_file *lk)\ndiff --git a/read-cache.c b/read-cache.c\nindex c3d5e35..9d8300a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -33,6 +33,7 @@ static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int reall\n #define CACHE_EXT(s) ( (s[0]<<24)|(s[1]<<16)|(s[2]<<8)|(s[3]) )\n #define CACHE_EXT_TREE 0x54524545\t/* \"TREE\" */\n #define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* \"REUC\" */\n+#define CACHE_EXT_INDEX_LOG 0x494C4F47\t  /* \"ILOG\" */\n \n struct index_state the_index;\n \n@@ -1297,6 +1298,18 @@ static int read_index_extension(struct index_state *istate,\n \tcase CACHE_EXT_RESOLVE_UNDO:\n \t\tistate->resolve_undo = resolve_undo_read(data, sz);\n \t\tbreak;\n+\tcase CACHE_EXT_INDEX_LOG:\n+\t\tif (!istate->log) {\n+\t\t\tistate->log = xmalloc(sizeof(struct strbuf *));\n+\t\t\tstrbuf_init(istate->log, 128);\n+\t\t}\n+\t\tstrbuf_add(istate->log, data, sz);\n+\t\tif (sscanf(istate->log->buf, \"%u\", &istate->log_index) != 1) {\n+\t\t\tstrbuf_release(istate->log);\n+\t\t\tfree(istate->log);\n+\t\t\tistate->log = NULL;\n+\t\t}\n+\t\tbreak;\n \tdefault:\n \t\tif (*ext < 'A' || 'Z' < *ext)\n \t\t\treturn error(\"index uses %.4s extension, which we do not understand\",\n@@ -1538,6 +1551,15 @@ int discard_index(struct index_state *istate)\n \tfree(istate->cache);\n \tistate->cache = NULL;\n \tistate->cache_alloc = 0;\n+\tistate->log_updated = 0;\n+\tistate->log_index = 0;\n+\tif (istate->log) {\n+\t\tstrbuf_release(istate->log);\n+\t\tfree(istate->log);\n+\t\tistate->log = NULL;\n+\t}\n+\n+\t/* no need to throw away allocated active_cache */\n \treturn 0;\n }\n \n@@ -1771,7 +1793,8 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile\n \t\trollback_lock_file(lockfile);\n }\n \n-int write_index(struct index_state *istate, int newfd)\n+int write_index_with_log(struct index_state *istate, int newfd,\n+\t\t\t const char *log)\n {\n \tgit_SHA_CTX c;\n \tstruct cache_header hdr;\n@@ -1847,6 +1870,32 @@ int write_index(struct index_state *istate, int newfd)\n \t\t\treturn -1;\n \t}\n \n+\tif (index_log_size) {\n+\t\tunsigned char sha1[20];\n+\t\tif (log) {\n+\t\t\tistate->log_index = (istate->log_index + 1) % index_log_size;\n+\t\t\tistate->log_updated = 1;\n+\t\t} else\n+\t\t\tlog = \"\";\n+\t\tif (get_sha1(\"HEAD^{tree}\", sha1))\n+\t\t\thashclr(sha1);\n+\t\tif (!istate->log) {\n+\t\t\tistate->log = xmalloc(sizeof(struct strbuf *));\n+\t\t\tstrbuf_init(istate->log, 128);\n+\t\t} else\n+\t\t\tstrbuf_reset(istate->log);\n+\t\tstrbuf_addf(istate->log, \"%u %s %s %s\",\n+\t\t\t    istate->log_index, sha1_to_hex(sha1),\n+\t\t\t    git_committer_info(0), log);\n+\n+\t\terr = write_index_ext_header(&c, newfd, CACHE_EXT_INDEX_LOG,\n+\t\t\t\t\t     istate->log->len) < 0\n+\t\t\t|| ce_write(&c, newfd, istate->log->buf,\n+\t\t\t\t    istate->log->len) < 0;\n+\t\tif (err)\n+\t\t\treturn -1;\n+\t}\n+\n \tif (ce_flush(&c, newfd) || fstat(newfd, &st))\n \t\treturn -1;\n \tistate->timestamp.sec = (unsigned int)st.st_mtime;\n@@ -1854,6 +1903,11 @@ int write_index(struct index_state *istate, int newfd)\n \treturn 0;\n }\n \n+int write_index(struct index_state *istate, int newfd)\n+{\n+\treturn write_index_with_log(istate, newfd, NULL);\n+}\n+\n /*\n  * Read the index file that is potentially unmerged into given\n  * index_state, dropping any unmerged entries.  Returns true if\n-- \n1.8.2.83.gc99314b\n"},{"id":"224804","messageId":"1375966270-10968-1-git-send-email-pclouds@gmail.com","threadId":"34609","inReplyTo":"1375597720-13236-1-git-send-email-pclouds@gmail.com","subject":"[PATCH/RFC v2] read-cache: save index entry updates in ILOG index extension","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-08T12:51:10Z","receivedAt":"2013-08-08T12:51:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If you have something different from both worktree and HEAD in index,\nthen accidentally do \"git add foo\", you may find it hard to recover\nthe previous version of foo in index. This is especially true when you\ndo \"git add -p\" with manual patch editing.\n\nThis patch makes sure that every operation that modifies the index\nfrom worktree or stdin is recorded as list of (path, SHA-1) in index\nwith command+arguments of the operation.\n\nWhen you make such a mistake, you can look at ILOG extension with\n(unimplemented) \"git ls-files --generation=X [ -- <paths>]\" where X is\nfrom 1 (the most recent operation) to N (the least recent) . \"X\" could\neven be \"all\" to list all generations.\n\nSHA-1 syntax is also going to be extended to support :-N:path syntax\nto get an entry from generation N, for convenience.\n\nOld operation's updates are removed as new ones are added to keep the\nsize under 1 MB. ILOG keeps minimum 10 operations regardless of its\nsize. These contansts should be configurable later one. ILOG content\nwill be compressed later on so that it leaves minimum\nfootprint. Because it's only needed at index writing time, read-only\noperations won't pay the cost for decompressing and compressing ILOG.\n\nILOG may also be used in a different way to implement \"git add\n--undo\". Before updating entries, git-add could save _old_ entries to\nILOG (and mark to-be-added entries as \"deleted/something\" for\nexample). It then can use this information to revert the\noperation. Similar candidates are \"git commit -a\" (destroying current\nindex) or \"git merge\" and of course \"git mv\" and \"git rm\"\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/add.c          |   1 +\n builtin/apply.c        |   1 +\n builtin/update-index.c |   1 +\n cache.h                |   3 ++\n read-cache.c           | 121 +++++++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 127 insertions(+)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 8266a9c..d000f8a 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -456,6 +456,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tstruct update_callback_data update_data;\n \n \tgit_config(add_config, NULL);\n+\tlog_index_changes(prefix, argv);\n \n \targc = parse_options(argc, argv, prefix, builtin_add_options,\n \t\t\t  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 50912c9..fc43ea8 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -4423,6 +4423,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n \tprefix = prefix_;\n \tprefix_length = prefix ? strlen(prefix) : 0;\n \tgit_config(git_apply_config, NULL);\n+\tlog_index_changes(prefix, argv);\n \tif (apply_default_whitespace)\n \t\tparse_whitespace_option(apply_default_whitespace);\n \tif (apply_default_ignorewhitespace)\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex c317981..aa757cb 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -799,6 +799,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(update_index_usage, options);\n \n \tgit_config(git_default_config, NULL);\n+\tlog_index_changes(prefix, argv);\n \n \t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\n \tlock_file = xcalloc(1, sizeof(struct lock_file));\ndiff --git a/cache.h b/cache.h\nindex 85b544f..a2156bf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -168,6 +168,7 @@ struct cache_entry {\n \n /* used to temporarily mark paths matched by pathspecs */\n #define CE_MATCHED           (1 << 26)\n+#define CE_BASE              (1 << 27)\n \n /*\n  * Extended on-disk flags\n@@ -277,6 +278,7 @@ struct index_state {\n \t\t initialized : 1;\n \tstruct hash_table name_hash;\n \tstruct hash_table dir_hash;\n+\tstruct strbuf *index_log;\n };\n \n extern struct index_state the_index;\n@@ -481,6 +483,7 @@ extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned ch\n extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);\n extern int index_name_is_other(const struct index_state *, const char *, int);\n extern void *read_blob_data_from_index(struct index_state *, const char *, unsigned long *);\n+extern void log_index_changes(const char *prefix, const char **argv);\n \n /* do stat comparison even if CE_VALID is true */\n #define CE_MATCH_IGNORE_VALID\t\t01\ndiff --git a/read-cache.c b/read-cache.c\nindex c3d5e35..4021667 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -14,6 +14,7 @@\n #include \"resolve-undo.h\"\n #include \"strbuf.h\"\n #include \"varint.h\"\n+#include \"quote.h\"\n \n static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really);\n \n@@ -33,8 +34,10 @@ static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int reall\n #define CACHE_EXT(s) ( (s[0]<<24)|(s[1]<<16)|(s[2]<<8)|(s[3]) )\n #define CACHE_EXT_TREE 0x54524545\t/* \"TREE\" */\n #define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* \"REUC\" */\n+#define CACHE_EXT_INDEX_LOG 0x494C4F47 /* \"ILOG\" */\n \n struct index_state the_index;\n+static struct strbuf log_message = STRBUF_INIT;\n \n static void set_index_entry(struct index_state *istate, int nr, struct cache_entry *ce)\n {\n@@ -1297,6 +1300,14 @@ static int read_index_extension(struct index_state *istate,\n \tcase CACHE_EXT_RESOLVE_UNDO:\n \t\tistate->resolve_undo = resolve_undo_read(data, sz);\n \t\tbreak;\n+\tcase CACHE_EXT_INDEX_LOG:\n+\t\tif (!istate->index_log) {\n+\t\t\tistate->index_log = xmalloc(sizeof(*istate->index_log));\n+\t\t\tstrbuf_init(istate->index_log, sz);\n+\t\t}\n+\t\tstrbuf_reset(istate->index_log);\n+\t\tstrbuf_add(istate->index_log, data, sz);\n+\t\tbreak;\n \tdefault:\n \t\tif (*ext < 'A' || 'Z' < *ext)\n \t\t\treturn error(\"index uses %.4s extension, which we do not understand\",\n@@ -1509,6 +1520,14 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\tsrc_offset += extsize;\n \t}\n \tmunmap(mmap, mmap_size);\n+\tif (istate == &the_index) {\n+\t\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\t\tstruct cache_entry *ce = istate->cache[i];\n+\t\t\tif (ce_stage(ce))\n+\t\t\t\tcontinue;\n+\t\t\tce->ce_flags |= CE_BASE;\n+\t\t}\n+\t}\n \treturn istate->cache_nr;\n \n unmap:\n@@ -1538,6 +1557,11 @@ int discard_index(struct index_state *istate)\n \tfree(istate->cache);\n \tistate->cache = NULL;\n \tistate->cache_alloc = 0;\n+\tif (istate->index_log) {\n+\t\tstrbuf_release(istate->index_log);\n+\t\tfree(istate->index_log);\n+\t\tistate->index_log = NULL;\n+\t}\n \treturn 0;\n }\n \n@@ -1771,6 +1795,81 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile\n \t\trollback_lock_file(lockfile);\n }\n \n+void log_index_changes(const char *prefix, const char **argv)\n+{\n+\tif (prefix || argv) {\n+\t\tif (prefix)\n+\t\t\tstrbuf_addf(&log_message, \"[%s]\", prefix);\n+\t\tsq_quote_argv(&log_message, argv, 0);\n+\t} else\n+\t\tstrbuf_setlen(&log_message, 0);\n+}\n+\n+static void get_updated_entries(struct index_state *istate,\n+\t\t\t\tstruct cache_entry ***cache_out,\n+\t\t\t\tunsigned int *cache_nr_out)\n+{\n+\tstruct cache_entry **cache;\n+\tunsigned int i, nr, cache_nr = 0;\n+\n+\t*cache_nr_out = 0;\n+\t*cache_out = NULL;\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tif (istate->cache[i]->ce_flags & CE_BASE)\n+\t\t\tcontinue;\n+\t\tcache_nr++;\n+\t}\n+\tif (!cache_nr)\n+\t\treturn;\n+\n+\tcache = xmalloc(cache_nr * sizeof(*istate->cache));\n+\tfor (i = nr = 0; i < istate->cache_nr; i++) {\n+\t\tstruct cache_entry *ce = istate->cache[i];\n+\t\tif (ce->ce_flags & CE_BASE)\n+\t\t\tcontinue;\n+\t\tcache[nr++] = ce;\n+\t}\n+\t*cache_out = cache;\n+\t*cache_nr_out = cache_nr;\n+}\n+\n+static void write_index_log(struct strbuf *sb,\n+\t\t\t    const struct strbuf *old_log,\n+\t\t\t    const struct strbuf *msg,\n+\t\t\t    struct cache_entry **cache,\n+\t\t\t    unsigned int cache_nr)\n+{\n+\tstruct strbuf body = STRBUF_INIT;\n+\tunsigned int i, size, nr, body_len, hdr_len;\n+\tconst char *end, *p;\n+\tstrbuf_addf(&body, \"%s%c\", msg->buf, '\\0');\n+\tfor (i = 0; i < cache_nr; i++)\n+\t\tstrbuf_addf(&body, \"%s %s%c\", sha1_to_hex(cache[i]->sha1),\n+\t\t\t    cache[i]->name, '\\0');\n+\tstrbuf_addf(sb, \"%u %u%c\", (unsigned int)cache_nr, (unsigned int)body.len, '\\0');\n+\tstrbuf_addbuf(sb, &body);\n+\tstrbuf_release(&body);\n+\n+\tif (!old_log)\n+\t\treturn;\n+\n+\tsize = sb->len;\n+\tnr = cache_nr;\n+\tend = old_log->buf + old_log->len;\n+\tp = old_log->buf;\n+\twhile (p < end && (size < 1024 * 1024 || nr < 10)) {\n+\t\tif (sscanf(p, \"%u %u\", &cache_nr, &body_len) != 2) {\n+\t\t\terror(\"fail to parse old index log at %u\", (unsigned int)(p - old_log->buf));\n+\t\t\tbreak;\n+\t\t}\n+\t\thdr_len = strlen(p) + 1;\n+\t\tstrbuf_add(sb, p, hdr_len + body_len);\n+\t\tsize += body_len;\n+\t\tnr += cache_nr;\n+\t\tp += hdr_len + body_len;\n+\t}\n+}\n+\n int write_index(struct index_state *istate, int newfd)\n {\n \tgit_SHA_CTX c;\n@@ -1780,6 +1879,11 @@ int write_index(struct index_state *istate, int newfd)\n \tint entries = istate->cache_nr;\n \tstruct stat st;\n \tstruct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n+\tunsigned int index_log_nr = 0;\n+\tstruct cache_entry **index_log_entries = NULL;\n+\n+\tif (istate == &the_index && log_message.len)\n+\t\tget_updated_entries(istate, &index_log_entries, &index_log_nr);\n \n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n@@ -1846,6 +1950,23 @@ int write_index(struct index_state *istate, int newfd)\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n+\tif (index_log_entries && log_message.len) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\twrite_index_log(&sb, istate->index_log, &log_message,\n+\t\t\t\tindex_log_entries, index_log_nr);\n+\t\terr = write_index_ext_header(&c, newfd, CACHE_EXT_INDEX_LOG,\n+\t\t\t\t\t     sb.len) < 0\n+\t\t\t|| ce_write(&c, newfd, sb.buf,\n+\t\t\t\t    sb.len) < 0;\n+\t\tif (istate->index_log)\n+\t\t\tstrbuf_release(istate->index_log);\n+\t\telse\n+\t\t\tistate->index_log = xmalloc(sizeof(*istate->index_log));\n+\t\t*istate->index_log = sb;\n+\t\tif (err)\n+\t\t\treturn -1;\n+\t}\n+\tfree(index_log_entries);\n \n \tif (ce_flush(&c, newfd) || fstat(newfd, &st))\n \t\treturn -1;\n-- \n1.8.2.83.gc99314b\n"},{"id":"224841","messageId":"7veha49g1x.fsf@alter.siamese.dyndns.org","threadId":"34609","inReplyTo":"1375966270-10968-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH/RFC v2] read-cache: save index entry updates in ILOG index extension","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-08T18:46:18Z","receivedAt":"2013-08-08T18:46:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Old operation's updates are removed as new ones are added to keep the\n> size under 1 MB. ILOG keeps minimum 10 operations regardless of its\n> size. These contansts should be configurable later one. ILOG content\n> will be compressed later on so that it leaves minimum\n> footprint.\n\nList of <sha-1, pathname> tuples would not compress well, I suspect.\n\n> Because it's only needed at index writing time, read-only\n> operations won't pay the cost for decompressing and compressing ILOG.\n\nAnother idea is to lazily read existing ILOG by (1) keeping just an\noffset in the originating index file at read_index() time and (2)\nreading it on demand when \":-4:Makefile\" was asked for.\n\n> diff --git a/cache.h b/cache.h\n> index 85b544f..a2156bf 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -168,6 +168,7 @@ struct cache_entry {\n>  \n>  /* used to temporarily mark paths matched by pathspecs */\n>  #define CE_MATCHED           (1 << 26)\n> +#define CE_BASE              (1 << 27)\n\nAs this is not about pathspec match, please have its own comment\nline (or a blank line, if this goes without comment) above this new\nline.\n\n> @@ -277,6 +278,7 @@ struct index_state {\n>  \t\t initialized : 1;\n>  \tstruct hash_table name_hash;\n>  \tstruct hash_table dir_hash;\n> +\tstruct strbuf *index_log;\n>  };\n\nSane to have this as a per-index_state variable.\n\n> +extern void log_index_changes(const char *prefix, const char **argv);\n\nNot sane to name this function _index_anything and not to have index_state\nas its first parameter, breaking the naming convention.\n\n> diff --git a/read-cache.c b/read-cache.c\n> index c3d5e35..4021667 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -14,6 +14,7 @@\n>  #include \"resolve-undo.h\"\n>  #include \"strbuf.h\"\n>  #include \"varint.h\"\n> +#include \"quote.h\"\n>  \n>  static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really);\n>  \n> @@ -33,8 +34,10 @@ static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int reall\n>  #define CACHE_EXT(s) ( (s[0]<<24)|(s[1]<<16)|(s[2]<<8)|(s[3]) )\n>  #define CACHE_EXT_TREE 0x54524545\t/* \"TREE\" */\n>  #define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* \"REUC\" */\n> +#define CACHE_EXT_INDEX_LOG 0x494C4F47 /* \"ILOG\" */\n>  \n>  struct index_state the_index;\n> +static struct strbuf log_message = STRBUF_INIT;\n\nNot sane to have a single global here.  Give the callers a helper\nfunction to prepare a ilog message into a strbuf they prepare before\nthey muck with argc/argv with parse_options(), and have them pass\nthat strbuf to\n\n\tlog_index_changes(struct index_state *, struct strbuf *);\n\nand supply the usual\n\n\t#define log_cache_changes(logmsg) log_index_changes(&the_index, (logmsg))\n\nmacro, inside \"#ifndef NO_THE_INDEX_COMPATIBILITY_MACROS\" section.\n\n> @@ -1509,6 +1520,14 @@ int read_index_from(struct index_state *istate, const char *path)\n>  \t\tsrc_offset += extsize;\n>  \t}\n>  \tmunmap(mmap, mmap_size);\n> +\tif (istate == &the_index) {\n\nYuck.  Why can't the callers operate on their own copies of the\nin-core index and get the same benefit?\n\n> +\t\tfor (i = 0; i < istate->cache_nr; i++) {\n> +\t\t\tstruct cache_entry *ce = istate->cache[i];\n> +\t\t\tif (ce_stage(ce))\n> +\t\t\t\tcontinue;\n> +\t\t\tce->ce_flags |= CE_BASE;\n> +\t\t}\n> +\t}\n>  \treturn istate->cache_nr;\n\n> +static void get_updated_entries(struct index_state *istate,\n> +\t\t\t\tstruct cache_entry ***cache_out,\n> +\t\t\t\tunsigned int *cache_nr_out)\n> +{\n> +\tstruct cache_entry **cache;\n> +\tunsigned int i, nr, cache_nr = 0;\n> +\n> +\t*cache_nr_out = 0;\n> +\t*cache_out = NULL;\n> +\tfor (i = 0; i < istate->cache_nr; i++) {\n> +\t\tif (istate->cache[i]->ce_flags & CE_BASE)\n> +\t\t\tcontinue;\n> +\t\tcache_nr++;\n> +\t}\n> +\tif (!cache_nr)\n> +\t\treturn;\n> +\tcache = xmalloc(cache_nr * sizeof(*istate->cache));\n> +\tfor (i = nr = 0; i < istate->cache_nr; i++) {\n> +\t\tstruct cache_entry *ce = istate->cache[i];\n> +\t\tif (ce->ce_flags & CE_BASE)\n> +\t\t\tcontinue;\n> +\t\tcache[nr++] = ce;\n> +\t}\n> +\t*cache_out = cache;\n> +\t*cache_nr_out = cache_nr;\n> +}\n\nI can read what the function does, but I do not understand the\nassumption this code makes.\n\nIs this assuming that any newly created cache_entry that goes to\nthe_index via add_index_entry() will not have CE_BASE bit set?  Some\ncodepaths try to preserve the flags bit they do not care and/or\nunderstand (e.g. rename_index_entry_at() creates a new ce with a new\nname, and keeps most of the flags bit except for the name-hash state\nbits), so CE_BASE will be propagated from the original to the new\none, I think.\n\nYou seem to be recording the state _after_ the change---that can be\nread without the extension, can't it?  I thought we care more about\nthe state that was _lost_ by the change.\n\nRecording the state after the change misses deleted entries, doesn't\nit?\n\n> +static void write_index_log(struct strbuf *sb,\n> +\t\t\t    const struct strbuf *old_log,\n> +\t\t\t    const struct strbuf *msg,\n> +\t\t\t    struct cache_entry **cache,\n> +\t\t\t    unsigned int cache_nr)\n> +{\n> +\tstruct strbuf body = STRBUF_INIT;\n> +\tunsigned int i, size, nr, body_len, hdr_len;\n> +\tconst char *end, *p;\n> +\tstrbuf_addf(&body, \"%s%c\", msg->buf, '\\0');\n> +\tfor (i = 0; i < cache_nr; i++)\n> +\t\tstrbuf_addf(&body, \"%s %s%c\", sha1_to_hex(cache[i]->sha1),\n> +\t\t\t    cache[i]->name, '\\0');\n\nWe do not care about file modes (e.g. \"update-index --chmod\")?\n\n> +\tstrbuf_addf(sb, \"%u %u%c\", (unsigned int)cache_nr, (unsigned int)body.len, '\\0');\n\nOK, so the header records how many entries there are and how big the\nrecord is, followed by a list of tuples that describe what got\nchanged.\n\n> +\tstrbuf_addbuf(sb, &body);\n> +\tstrbuf_release(&body);\n> +\n> +\tif (!old_log)\n> +\t\treturn;\n> +\n> +\tsize = sb->len;\n> +\tnr = cache_nr;\n> +\tend = old_log->buf + old_log->len;\n> +\tp = old_log->buf;\n> +\twhile (p < end && (size < 1024 * 1024 || nr < 10)) {\n> +\t\tif (sscanf(p, \"%u %u\", &cache_nr, &body_len) != 2) {\n> +\t\t\terror(\"fail to parse old index log at %u\", (unsigned int)(p - old_log->buf));\n> +\t\t\tbreak;\n> +\t\t}\n\nAnd that header is used to determine how much to bulk copy from the\nstack of old records.\n\n>  int write_index(struct index_state *istate, int newfd)\n>  {\n>  \tgit_SHA_CTX c;\n> @@ -1780,6 +1879,11 @@ int write_index(struct index_state *istate, int newfd)\n>  \tint entries = istate->cache_nr;\n>  \tstruct stat st;\n>  \tstruct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n> +\tunsigned int index_log_nr = 0;\n> +\tstruct cache_entry **index_log_entries = NULL;\n> +\n> +\tif (istate == &the_index && log_message.len)\n> +\t\tget_updated_entries(istate, &index_log_entries, &index_log_nr);\n\nYuck about \"istate == &the_index\" again.\n\n>  \tfor (i = removed = extended = 0; i < entries; i++) {\n>  \t\tif (cache[i]->ce_flags & CE_REMOVE)\n> @@ -1846,6 +1950,23 @@ int write_index(struct index_state *istate, int newfd)\n>  \t\tif (err)\n>  \t\t\treturn -1;\n>  \t}\n> +\tif (index_log_entries && log_message.len) {\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\twrite_index_log(&sb, istate->index_log, &log_message,\n> +\t\t\t\tindex_log_entries, index_log_nr);\n> +\t\terr = write_index_ext_header(&c, newfd, CACHE_EXT_INDEX_LOG,\n> +\t\t\t\t\t     sb.len) < 0\n> +\t\t\t|| ce_write(&c, newfd, sb.buf,\n> +\t\t\t\t    sb.len) < 0;\n> +\t\tif (istate->index_log)\n> +\t\t\tstrbuf_release(istate->index_log);\n> +\t\telse\n> +\t\t\tistate->index_log = xmalloc(sizeof(*istate->index_log));\n> +\t\t*istate->index_log = sb;\n> +\t\tif (err)\n> +\t\t\treturn -1;\n> +\t}\n> +\tfree(index_log_entries);\n>  \n>  \tif (ce_flush(&c, newfd) || fstat(newfd, &st))\n>  \t\treturn -1;\n"},{"id":"224903","messageId":"CACsJy8CbS_ZCZOsGY2KoTU=tid1ZH0wDbwcpu2oy9T9RV+N+BQ@mail.gmail.com","threadId":"34609","inReplyTo":"7veha49g1x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC v2] read-cache: save index entry updates in ILOG index extension","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-09T13:32:49Z","receivedAt":"2013-08-09T13:32:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Aug 9, 2013 at 1:46 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> Old operation's updates are removed as new ones are added to keep the\n>> size under 1 MB. ILOG keeps minimum 10 operations regardless of its\n>> size. These contansts should be configurable later one. ILOG content\n>> will be compressed later on so that it leaves minimum\n>> footprint.\n>\n> List of <sha-1, pathname> tuples would not compress well, I suspect.\n\nI was hoping that it still compresses well the discrete segments of\npathname. In the worst case we can group sha-1 together, separate from\npathnames.\n\n>> Because it's only needed at index writing time, read-only\n>> operations won't pay the cost for decompressing and compressing ILOG.\n>\n> Another idea is to lazily read existing ILOG by (1) keeping just an\n> offset in the originating index file at read_index() time and (2)\n> reading it on demand when \":-4:Makefile\" was asked for.\n\nWe need to go through ILOG extension anyway for index sha-1\nverification, so it's already read in kernel buffer. What we save is a\njust malloc. But I'll try.\n\n>\n>> diff --git a/cache.h b/cache.h\n>> index 85b544f..a2156bf 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -168,6 +168,7 @@ struct cache_entry {\n>>\n>>  /* used to temporarily mark paths matched by pathspecs */\n>>  #define CE_MATCHED           (1 << 26)\n>> +#define CE_BASE              (1 << 27)\n>\n> As this is not about pathspec match, please have its own comment\n> line (or a blank line, if this goes without comment) above this new\n> line.\n\nThis patch is more about the idea, whether it makes sense. You would\n(and did) find the patch somewhat disgusting later on.\n\n>> @@ -277,6 +278,7 @@ struct index_state {\n>>                initialized : 1;\n>>       struct hash_table name_hash;\n>>       struct hash_table dir_hash;\n>> +     struct strbuf *index_log;\n>>  };\n>\n> Sane to have this as a per-index_state variable.\n>\n>> +extern void log_index_changes(const char *prefix, const char **argv);\n>\n> Not sane to name this function _index_anything and not to have index_state\n> as its first parameter, breaking the naming convention.\n\nThe reason I can't put index_state there is because this function is\ncalled early, often before read_cache is is called. And I can't caller\nit later because argv would be ruined by parse_options(). An option is\nto convert argv to a string unconditionally in git.c, then\nlog_index_changes can be called much later, and with index_state\npointer.\n\n>> +static void get_updated_entries(struct index_state *istate,\n>> +                             struct cache_entry ***cache_out,\n>> +                             unsigned int *cache_nr_out)\n>> +{\n>> +     struct cache_entry **cache;\n>> +     unsigned int i, nr, cache_nr = 0;\n>> +\n>> +     *cache_nr_out = 0;\n>> +     *cache_out = NULL;\n>> +     for (i = 0; i < istate->cache_nr; i++) {\n>> +             if (istate->cache[i]->ce_flags & CE_BASE)\n>> +                     continue;\n>> +             cache_nr++;\n>> +     }\n>> +     if (!cache_nr)\n>> +             return;\n>> +     cache = xmalloc(cache_nr * sizeof(*istate->cache));\n>> +     for (i = nr = 0; i < istate->cache_nr; i++) {\n>> +             struct cache_entry *ce = istate->cache[i];\n>> +             if (ce->ce_flags & CE_BASE)\n>> +                     continue;\n>> +             cache[nr++] = ce;\n>> +     }\n>> +     *cache_out = cache;\n>> +     *cache_nr_out = cache_nr;\n>> +}\n>\n> I can read what the function does, but I do not understand the\n> assumption this code makes.\n>\n> Is this assuming that any newly created cache_entry that goes to\n> the_index via add_index_entry() will not have CE_BASE bit set?  Some\n> codepaths try to preserve the flags bit they do not care and/or\n> understand (e.g. rename_index_entry_at() creates a new ce with a new\n> name, and keeps most of the flags bit except for the name-hash state\n> bits), so CE_BASE will be propagated from the original to the new\n> one, I think.\n>\n> You seem to be recording the state _after_ the change---that can be\n> read without the extension, can't it?  I thought we care more about\n> the state that was _lost_ by the change.\n>\n> Recording the state after the change misses deleted entries, doesn't\n> it?\n\nRight. At the end of the commit message I mentioned about \"git add\n--undo\". After I wrote it, I became more convinced it's the way to go.\nThat should be the UI, instead of letting the user hunt the right\nentry through the index log. And then caller has the responsibility to\ntrack changes and feed it to read-cache (CE_BASE trick is gone). And\nit would record something like raw diff: a pair of old/new sha-1 and a\npath name. This helps differentiate modified, deleted and added\nentries that \"git add --undo\" may need to undo.\n\n>> +static void write_index_log(struct strbuf *sb,\n>> +                         const struct strbuf *old_log,\n>> +                         const struct strbuf *msg,\n>> +                         struct cache_entry **cache,\n>> +                         unsigned int cache_nr)\n>> +{\n>> +     struct strbuf body = STRBUF_INIT;\n>> +     unsigned int i, size, nr, body_len, hdr_len;\n>> +     const char *end, *p;\n>> +     strbuf_addf(&body, \"%s%c\", msg->buf, '\\0');\n>> +     for (i = 0; i < cache_nr; i++)\n>> +             strbuf_addf(&body, \"%s %s%c\", sha1_to_hex(cache[i]->sha1),\n>> +                         cache[i]->name, '\\0');\n>\n> We do not care about file modes (e.g. \"update-index --chmod\")?\n\nNot as valuable in my opinion. But if I implement \"git add --undo\", I\nprobably need to pay attention to file modes or some users may get\nupset as it's not a \"real undo\" otherwise.\n-- \nDuy\n"}]}