Re: [RFC PATCH] checkout: Force matching mtime between files

[Duy Nguyen <pclouds@gmail.com>]

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 <marcnarc@xiplink.com> 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