On Thu, Apr 26, 2018 at 05:48:35PM +0000, Robin H. Johnson wrote:
> On Thu, Apr 26, 2018 at 06:43:56PM +0200, Duy Nguyen wrote:
> > On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud <[email protected]>
> > wrote:
> > > Are we all that sure that the performance hit is that drastic? After all,
> > > we've just done write_entry(). Calling utime() at that point should just
> > > hit the filesystem cache.
> > I have a feeling this has "this is linux" assumption. Anybody knows
> > how freebsd, mac os x and windows behave?
> I don't know sorry. futimens might be better here if it can be used
> before the fd is closed.
>
> > > * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
> > > identical so the above loop does nothing. Offhand I'm not even sure how a
> > > hook might get the right files in this case.
> > Would a hook that gives you the list of updated files (in the exact
> > same order that git updates) help?
> Yes, that, along with the target revision I think would allow most or
> all of the desired behaviors mentioned in this thread *.
Target revision should be available in the index. But this gives me an
idea to another thing that bugs me: sending the list to the hook means
I have to deal with separator (\n or NUL?) or escaping. This mentions
of index makes me take a different direction. I could produce a small
index that contains just what is modified, then you can retrieve
whatever info you want with `git ls-files` or even `git show` after
pointing $GIT_INDEX_FILE to it.
So it's basically what the following (hacky) patch does. It adds
support for a new hook named post-checkout-modified. This hook will
prepares $GIT_DIR/index.modified which contains just the files
git-checkout has touched and deletes it after the hook finishes.
My test hook is pretty simple just to dump out what in there
#!/bin/sh
GIT_INDEX_FILE=`git rev-parse --git-path index.modified` git ls-files
--stage
and it seems to work.
Of course, this does not give you the checkout order. But checkout
order has always been sorted order by path if I remember correctly and
it's unlikely to change (and I don't think you really need that exact
order anyway)
-- 8< --
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b49b582071..92b30cd05f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -52,6 +52,8 @@ struct checkout_opts {
const char *prefix;
struct pathspec pathspec;
struct tree *source_tree;
+
+ struct index_state istate_modified;
};
static int post_checkout_hook(struct commit *old_commit, struct commit
*new_commit,
@@ -470,7 +472,7 @@ static void setup_branch_path(struct branch_info *branch)
branch->path = strbuf_detach(&buf, NULL);
}
-static int merge_working_tree(const struct checkout_opts *opts,
+static int merge_working_tree(struct checkout_opts *opts,
struct branch_info *old_branch_info,
struct branch_info *new_branch_info,
int *writeout_error)
@@ -595,6 +597,27 @@ static int merge_working_tree(const struct checkout_opts
*opts,
if (!cache_tree_fully_valid(active_cache_tree))
cache_tree_update(&the_index, WRITE_TREE_SILENT |
WRITE_TREE_REPAIR);
+ if (find_hook("post-checkout-modified")) {
+ int i;
+
+ for (i = 0; i < the_index.cache_nr; i++) {
+ struct cache_entry *ce = the_index.cache[i];
+ struct cache_entry *new_ce;
+
+ /*
+ * Hack: this is an abuse of this flag, hidden
+ * dependency with write_locked_index()
+ */
+ if (!(ce->ce_flags & CE_UPDATE_IN_BASE))
+ continue;
+
+ new_ce = xcalloc(1, cache_entry_size(ce_namelen(ce)));
+ memcpy(new_ce, ce, cache_entry_size(ce_namelen(ce)));
+ add_index_entry(&opts->istate_modified, new_ce,
+ ADD_CACHE_JUST_APPEND);
+ }
+ }
+
if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
die(_("unable to write new index file"));
@@ -811,7 +834,7 @@ static void orphaned_commit_warning(struct commit
*old_commit, struct commit *ne
clear_commit_marks_all(ALL_REV_FLAGS);
}
-static int switch_branches(const struct checkout_opts *opts,
+static int switch_branches(struct checkout_opts *opts,
struct branch_info *new_branch_info)
{
int ret = 0;
@@ -848,6 +871,16 @@ static int switch_branches(const struct checkout_opts
*opts,
update_refs_for_switch(opts, &old_branch_info, new_branch_info);
+ if (find_hook("post-checkout-modified")) {
+ struct lock_file lock_file = LOCK_INIT;
+
+ hold_lock_file_for_update(&lock_file,
git_path("index.modified"),
+ LOCK_DIE_ON_ERROR);
+ write_locked_index(&opts->istate_modified, &lock_file,
COMMIT_LOCK);
+ run_hook_le(NULL, "post-checkout-modified", NULL);
+ discard_index(&opts->istate_modified);
+ unlink(git_path("index.modified"));
+ }
ret = post_checkout_hook(old_branch_info.commit,
new_branch_info->commit, 1);
free(path_to_free);
return ret || writeout_error;
-- 8< --
> It also needs to fire in cases like 'git reset --hard $REV'.
>
> * For this case, I just need the mtimes to be consistent within a single
> checkout, I don't need them to have specific values.
hmm.. I didn't realize that this command is also affected by mgorny's
patch and a bunch others that use unpack_trees() (git-merge comes to
mind), which may be questionable.
A "post-checkout" hook does not sound right to fire in this case. I
think you can just go with "git checkout -f $REV" and achieve the same
thing.
--
Duy
