Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit

From: Duy Nguyen <pclouds@gmail.com>
Date: Jul 15, 2014, 10:23 UTC
Message-ID: <20140715102314.GA8273@lanh>
In-Reply-To: <CAPc5daVH=i72Y4dA8TefPLfFB79Cvw7STPnQf_f10cBeYbg2ug@mail.gmail.com>

On Mon, Jul 14, 2014 at 11:38:06PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> On Mon, Jul 14, 2014 at 7:15 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > It makes me wonder if a cleaner way of rebuilding cache-treei in this
> > case is from git-add--interactive.perl, or by simply spawn "git
> > update-index --rebuild-cache-tree" after running
> > git-add--interactive.perl.
> 
> We could check if the cache-tree has fully been populated by
> "add -i" and limit the rebuilding by "git commit -p" main process,
> but if "add -i" did not do so, there is no reason why "git commit -p"
> should not do so, without relying on the implementation of "add -i"
> to do so.
> 
> At least to me, what you suggested sounds nothing more than
> a cop-out; instead of lifting the limitation of the API by enhancing
> it with reopen-lock-file, punting to shift the burden elsewhere. I
> am not quite sure why that is cleaner, but perhaps I am missing
> something.

Reopen-lock-file to me does not sound like an enhancement, at least in the index update context. We have always updated the index by writing to a new file, then rename. Now if the new write_locked_index() blows up after reopen_lock_file(), we have no way but die() because index_lock.filename can no longer be trusted.

Doing it in a separate process keeps the tradition. And if the second write fails, we still have good index_lock.filename. We could also do the same here with something like this without new process (lightly tested):

-- 8< --

diff --git a/builtin/commit.c b/builtin/commit.c
index 606c890..c2b4875 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -340,6 +340,18 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 
 		discard_cache();
 		read_cache_from(index_lock.filename);
+		if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
+			static struct lock_file second_index_lock;
+			hold_lock_file_for_update(&second_index_lock,
+						  index_lock.filename,
+						  LOCK_DIE_ON_ERROR);
+			if (write_locked_index(&the_index, &second_index_lock,
+					       COMMIT_LOCK)) {
+				warning(_("Failed to update main cache tree"));
+				rollback_lock_file(&second_index_lock);
+			}
+		} else
+			warning(_("Failed to update main cache tree"));
 
 		commit_style = COMMIT_NORMAL;
 		return index_lock.filename;
-- 8< --

I was worried about the dependency between second_index_lock and
index_lock, but at cleanup, all we do is rollback, which is safe
regardless of dependencies. Just need to make sure we commit
second_index_lock before index_lock.

> In the longer run, it would be plausible that somebody would want
> to rewrite "git-add -i" and will make it just a C API call away without
> having to spawn a separate process. You cannot punt by saying
> "make 'add -i' responsible for it" at that point; you will be doing
> what 'add -i' would be doing yourself, no?

Yes, but at current rewrite rate I'd bet it won't happen in the next
two years at least.
--
Duy