On Mon, Jul 14, 2014 at 11:38:06PM -0700, Junio C Hamano wrote:
> On Mon, Jul 14, 2014 at 7:15 PM, Duy Nguyen <pclo...@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
--
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

Reply via email to