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 <pclouds@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html