[RFC PATCH] checkout: Force matching mtime between files

[RFC PATCH] checkout: Force matching mtime between files

[Michał Górny]

Currently git does not control mtimes of files being checked out.  This
means that the only assumption you could make is that all files created
or modified within a single checkout action will have mtime between
start time and end time of this checkout.  The relations between mtimes
of different files depend on the order in which they are checked out,
filesystem speed and timestamp precision.

Git repositories sometimes contain both generated and respective source
files.  For example, the Gentoo 'user syncing' repository combines
source ebuild files with generated metadata cache for user convenience.
Ideally, the 'git checkout' would be fast enough that (combined with low
timestamp precision) all files created or modified within a single
checkout would have matching timestamp.  However, in reality the cache
files may end up being wrongly 'older' than source file, requiring
unnecessary recheck.

The opposite problem (cache files not being regenerated when they should
have been) might also occur.  However, it could not be solved without
preserving timestamps, therefore it is out of scope of this change.

In order to avoid unnecessary cache mismatches, force a matching mtime
between all files created by a single checkout action.  This seems to be
the best course of action.  Matching mtimes do not trigger cache
updates.  They also match the concept of 'checkout' being an atomic
action.  Finally, this change does not break backwards compatibility
as the new result is a subset of the possible previous results.

For example, let's say that 'git checkout' creates two files in order,
with respective timestamps T1 and T2.  Before this patch, T1 <= T2.
After this patch, T1 == T2 which also satisfies T1 <= T2.

A similar problem was previously being addressed via git-restore-mtime
tool.  However, that solution is unnecessarily complex for Gentoo's use
case and does not seem to be suitable for 'seamless' integration.

The patch integrates mtime forcing via adding an additional member of
'struct checkout'.  This seemed the simplest way of adding it without
having to modify prototypes and adjust multiple call sites.  The member
is set to the current time in check_updates() function and is afterwards
enforced by checkout_entry().

The patch uses utime() rather than futimes() as that seems to be
the function used everywhere else in git and provided some MinGW
compatibility code.

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 cache.h        |  1 +
 entry.c        | 12 +++++++++++-
 unpack-trees.c |  1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index bbaf5c349..9f0a7c867 100644
--- a/cache.h
+++ b/cache.h
@@ -1526,6 +1526,7 @@ struct checkout {
 	const char *base_dir;
 	int base_dir_len;
 	struct delayed_checkout *delayed_checkout;
+	time_t checkout_mtime;
 	unsigned force:1,
 		 quiet:1,
 		 not_new:1,
diff --git a/entry.c b/entry.c
index 2101201a1..7ee5a6d28 100644
--- a/entry.c
+++ b/entry.c
@@ -411,6 +411,7 @@ int checkout_entry(struct cache_entry *ce,
 {
 	static struct strbuf path = STRBUF_INIT;
 	struct stat st;
+	int ret;
 
 	if (topath)
 		return write_entry(ce, topath, state, 1);
@@ -473,5 +474,14 @@ int checkout_entry(struct cache_entry *ce,
 		return 0;
 
 	create_directories(path.buf, path.len, state);
-	return write_entry(ce, path.buf, state, 0);
+	ret = write_entry(ce, path.buf, state, 0);
+
+	if (ret == 0 && state->checkout_mtime != 0) {
+		struct utimbuf t;
+		t.modtime = state->checkout_mtime;
+		if (utime(path.buf, &t) < 0)
+			warning_errno("failed utime() on %s", path.buf);
+	}
+
+	return ret;
 }
diff --git a/unpack-trees.c b/unpack-trees.c
index e73745051..e1efefb68 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -346,6 +346,7 @@ static int check_updates(struct unpack_trees_options *o)
 	state.quiet = 1;
 	state.refresh_cache = 1;
 	state.istate = index;
+	state.checkout_mtime = time(NULL);
 
 	progress = get_progress(o);
 
-- 
2.17.0

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

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

On Fri, Apr 13, 2018 at 07:01:29PM +0200, Michał Górny wrote:
> Currently git does not control mtimes of files being checked out.  This
> means that the only assumption you could make is that all files created
> or modified within a single checkout action will have mtime between
> start time and end time of this checkout.  The relations between mtimes
> of different files depend on the order in which they are checked out,
> filesystem speed and timestamp precision.
> ...
Junio: ping for review or inclusion of this patch?

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 1113 bytes --]

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

[Junio C Hamano]

"Robin H. Johnson" <robbat2@gentoo.org> writes:

> On Fri, Apr 13, 2018 at 07:01:29PM +0200, Michał Górny wrote:
>> Currently git does not control mtimes of files being checked out.  This
>> means that the only assumption you could make is that all files created
>> or modified within a single checkout action will have mtime between
>> start time and end time of this checkout.  The relations between mtimes
>> of different files depend on the order in which they are checked out,
>> filesystem speed and timestamp precision.
>> ...
> Junio: ping for review or inclusion of this patch?

I personally did not think this is a good idea and not worth any
code contamination with calls to utime().  Is there anybody sane who
thought this was a good idea in the discussion thread?

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

[Marc Branchaud]

On 2018-04-13 01:01 PM, Michał Górny wrote:
> Currently git does not control mtimes of files being checked out.  This
> means that the only assumption you could make is that all files created
> or modified within a single checkout action will have mtime between
> start time and end time of this checkout.  The relations between mtimes
> of different files depend on the order in which they are checked out,
> filesystem speed and timestamp precision.

Thanks for scratching this old itch [1]!

Big +1 from me.  We've had incremental smoke-test rebuilds fail because 
of inconsistent file timestamps.

		M.

[1] https://public-inbox.org/git/50C79D1F.1080709@xiplink.com/

> Git repositories sometimes contain both generated and respective source
> files.  For example, the Gentoo 'user syncing' repository combines
> source ebuild files with generated metadata cache for user convenience.
> Ideally, the 'git checkout' would be fast enough that (combined with low
> timestamp precision) all files created or modified within a single
> checkout would have matching timestamp.  However, in reality the cache
> files may end up being wrongly 'older' than source file, requiring
> unnecessary recheck.
> 
> The opposite problem (cache files not being regenerated when they should
> have been) might also occur.  However, it could not be solved without
> preserving timestamps, therefore it is out of scope of this change.
> 
> In order to avoid unnecessary cache mismatches, force a matching mtime
> between all files created by a single checkout action.  This seems to be
> the best course of action.  Matching mtimes do not trigger cache
> updates.  They also match the concept of 'checkout' being an atomic
> action.  Finally, this change does not break backwards compatibility
> as the new result is a subset of the possible previous results.
> 
> For example, let's say that 'git checkout' creates two files in order,
> with respective timestamps T1 and T2.  Before this patch, T1 <= T2.
> After this patch, T1 == T2 which also satisfies T1 <= T2.
> 
> A similar problem was previously being addressed via git-restore-mtime
> tool.  However, that solution is unnecessarily complex for Gentoo's use
> case and does not seem to be suitable for 'seamless' integration.
> 
> The patch integrates mtime forcing via adding an additional member of
> 'struct checkout'.  This seemed the simplest way of adding it without
> having to modify prototypes and adjust multiple call sites.  The member
> is set to the current time in check_updates() function and is afterwards
> enforced by checkout_entry().
> 
> The patch uses utime() rather than futimes() as that seems to be
> the function used everywhere else in git and provided some MinGW
> compatibility code.
> 
> Signed-off-by: Michał Górny <mgorny@gentoo.org>
> ---
>   cache.h        |  1 +
>   entry.c        | 12 +++++++++++-
>   unpack-trees.c |  1 +
>   3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/cache.h b/cache.h
> index bbaf5c349..9f0a7c867 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1526,6 +1526,7 @@ struct checkout {
>   	const char *base_dir;
>   	int base_dir_len;
>   	struct delayed_checkout *delayed_checkout;
> +	time_t checkout_mtime;
>   	unsigned force:1,
>   		 quiet:1,
>   		 not_new:1,
> diff --git a/entry.c b/entry.c
> index 2101201a1..7ee5a6d28 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -411,6 +411,7 @@ int checkout_entry(struct cache_entry *ce,
>   {
>   	static struct strbuf path = STRBUF_INIT;
>   	struct stat st;
> +	int ret;
>   
>   	if (topath)
>   		return write_entry(ce, topath, state, 1);
> @@ -473,5 +474,14 @@ int checkout_entry(struct cache_entry *ce,
>   		return 0;
>   
>   	create_directories(path.buf, path.len, state);
> -	return write_entry(ce, path.buf, state, 0);
> +	ret = write_entry(ce, path.buf, state, 0);
> +
> +	if (ret == 0 && state->checkout_mtime != 0) {
> +		struct utimbuf t;
> +		t.modtime = state->checkout_mtime;
> +		if (utime(path.buf, &t) < 0)
> +			warning_errno("failed utime() on %s", path.buf);
> +	}
> +
> +	return ret;
>   }
> diff --git a/unpack-trees.c b/unpack-trees.c
> index e73745051..e1efefb68 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -346,6 +346,7 @@ static int check_updates(struct unpack_trees_options *o)
>   	state.quiet = 1;
>   	state.refresh_cache = 1;
>   	state.istate = index;
> +	state.checkout_mtime = time(NULL);
>   
>   	progress = get_progress(o);
>   
> 

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

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

On Fri, Apr 13, 2018 at 07:01:29PM +0200, Michał Górny wrote:
> --- a/entry.c
> +++ b/entry.c
> @@ -411,6 +411,7 @@ int checkout_entry(struct cache_entry *ce,
>  {
>  	static struct strbuf path = STRBUF_INIT;
>  	struct stat st;
> +	int ret;
>  
>  	if (topath)
>  		return write_entry(ce, topath, state, 1);
mgorny: Should the topath case trigger utime as well?

Other questions:
- Would there be be any value in hoisting the utime change into
  write_entry's finish block rather than having it in checkout_entry?
- Should mtimes on directories be set if the directory is explicitly
  created?
- Maybe using futimens on supported platforms?

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 1113 bytes --]

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

[Michał Górny]

W dniu śro, 25.04.2018 o godzinie 06∶58 +0000, użytkownik Robin H.
Johnson napisał:
> On Fri, Apr 13, 2018 at 07:01:29PM +0200, Michał Górny wrote:
> > --- a/entry.c
> > +++ b/entry.c
> > @@ -411,6 +411,7 @@ int checkout_entry(struct cache_entry *ce,
> >  {
> >  	static struct strbuf path = STRBUF_INIT;
> >  	struct stat st;
> > +	int ret;
> >  
> >  	if (topath)
> >  		return write_entry(ce, topath, state, 1);
> 
> mgorny: Should the topath case trigger utime as well?

I don't think so.  AFAIU topath case is only used by 'git checkout-index 
--all', and it implies that data is written to temporary files rather
than actual working copy files, so mtimes do not really matter there.

> 
> Other questions:
> - Would there be be any value in hoisting the utime change into
>   write_entry's finish block rather than having it in checkout_entry?

I've attempted to reduce the scope of my changes to minimum, therefore
checkout_entry() seemed like the 'closest' thing to where it is set
and where it could be implemented.  I see no problem in doing that
in write_entry(), though.  In fact, it might be useful to do that before
filling the stat cache.

> - Should mtimes on directories be set if the directory is explicitly
>   created?

I don't think there's really a purpose in that.  I can't think of any
reason why anybody would be able to use directory mtimes reliably, so
maybe keeping their standard behavior is better here.

> - Maybe using futimens on supported platforms?

I'm all for that.  However, this is something the git maintainers should
decide as it probably implies some maintenance burden.

-- 
Best regards,
Michał Górny

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

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]

On Tue, Apr 24, 2018 at 08:41:07AM +0900, Junio C Hamano wrote:
> "Robin H. Johnson" <robbat2@gentoo.org> writes:
> 
> > On Fri, Apr 13, 2018 at 07:01:29PM +0200, Michał Górny wrote:
> >> Currently git does not control mtimes of files being checked out.  This
> >> means that the only assumption you could make is that all files created
> >> or modified within a single checkout action will have mtime between
> >> start time and end time of this checkout.  The relations between mtimes
> >> of different files depend on the order in which they are checked out,
> >> filesystem speed and timestamp precision.
> >> ...
> > Junio: ping for review or inclusion of this patch?
> 
> I personally did not think this is a good idea and not worth any
> code contamination with calls to utime().  Is there anybody sane who
> thought this was a good idea in the discussion thread?
Nobody responded to the original message until after I pinged about it
again.

Since that, one person DID respond, stating that it fixed an issue they
had previously reported 6 years ago.

In the thread from 6 years ago, you asked about tar's behavior for
mtimes. 'tar xf' restores mtimes from the tar archive, so relative
ordering after restore would be the same, and would only rebuild if the
original source happened to be dirty.

This behavior is already non-deterministic in Git, and would be improved
by the patch.

On a machine with high resolution timestamps or large enough repo that
checkout takes a long time, an initial checkout of multiple files does
not guarantee the ordering of mtimes of those files. Checking out (A,B)
could wind up with them having a different relative mtimes.

For this example, we are doing a checkout of two files A,B being written
(either due to initial checkout, or both have changed for some reason).

The example system has this as a property:
- "touch A B" => mtime(A) < mtime(B)
- "touch B A" => mtime(A) > mtime(B)
[touch should not re-order arguments, nor apply the same mtime to all
files. Linux touch at this point makes the syscall of 'utimensat(0,
NULL, NULL, 0)' on each file descriptor]

Existing behavior:
mtime(A), mtime(B) are independent of each other, and depend on the
exact order of file checkout, along with the resolution of timestamps
and how much other work is taking place. If the filesystem has low
resolution of timestamps, or the checkout is sufficiently small/fast,
the mtimes are likely to be identical already.

New behavior:
Strictly mtime(A) == mtime(B)

Example makefile rule:
B: A

Human explanation of makefile rule:
file B depends on file A

If the build system triggers on:

* mtime(A) > mtime(B): [strictly greater]
** Old Behavior: It will depend on the exact checkout order. Sometimes
   it will already not rebuild.
** New Behavior: always rebuild, as mtime(A) == mtime(B)

* mtime(A) >= mtime(B): [greater or equal]
** Old Behavior: it will depend on the exact checkout order. Sometimes it
   will, sometimes it won't.
** New Behavior: will not rebuild, as mtime(A) == mtime(B)

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 1113 bytes --]

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

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 23 2018, Junio C. Hamano wrote:

> "Robin H. Johnson" <robbat2@gentoo.org> writes:
>
>> On Fri, Apr 13, 2018 at 07:01:29PM +0200, Michał Górny wrote:
>>> Currently git does not control mtimes of files being checked out.  This
>>> means that the only assumption you could make is that all files created
>>> or modified within a single checkout action will have mtime between
>>> start time and end time of this checkout.  The relations between mtimes
>>> of different files depend on the order in which they are checked out,
>>> filesystem speed and timestamp precision.
>>> ...
>> Junio: ping for review or inclusion of this patch?
>
> I personally did not think this is a good idea and not worth any
> code contamination with calls to utime().  Is there anybody sane who
> thought this was a good idea in the discussion thread?

I think given that this keeps coming up it would be nice if Git had some
easy facility to do this type of thing, because it's clearly a common
use-case, but I don't think this is the route that should be taken.

By doing it this way we're imposing a fixed cost on checkouts for people
who don't care about this, and this will be mutually exclusive with
other potential approaches.

I think a sane path towards something like this is closer to:

 1. Add ability to have multiple hooks, similar to what I had as a WIP
    in
    https://public-inbox.org/git/CACBZZX6j6q2DUN_Z-Pnent1u714dVNPFBrL_PiEQyLmCzLUVxg@mail.gmail.com/
    and what
    e.g. https://docs.gitlab.com/ee/administration/custom_hooks.html
    implements (partially) in the wild.

 2. Add some config so we can have hook search paths, and ship with a
    default search path for hooks shipped with git.

 3. Then users can trivially enable e.g the
    post-checkout-consistent-mtimes hook, which could either ship with
    git itself, or be its own project.

As it is we've had several threads recently where people have wanted
different mtime solutions, e.g. consistent mtimes within one checkout,
or the mtime of when the file was last changed according to git etc.

I don't think it's a sane approach that the git checkout code learn to
do all of that natively, but I *do* think it's sane that we improve hook
support so we can run whatever custom logic people want at the right
time, which could run around re-setting mtimes.

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

[Junio C Hamano]

"Robin H. Johnson" <robbat2@gentoo.org> writes:

> In the thread from 6 years ago, you asked about tar's behavior for
> mtimes. 'tar xf' restores mtimes from the tar archive, so relative
> ordering after restore would be the same, and would only rebuild if the
> original source happened to be dirty.
>
> This behavior is already non-deterministic in Git, and would be improved
> by the patch.

But Git is not an archiver (tar), but is a source code control
system, so I do not think we should spend any extra cycles to
"improve" its behaviour wrt the relative ordering, at least for the
default case.  Only those who rely on having build artifact *and*
source should pay the runtime (and preferrably also the
maintainance) cost.

The best approach to do so is to have those people do the "touch"
thing in their own post-checkout hook.  People who use Git as the
source control system won't have to pay runtime cost of doing the
touch thing, and we do not have to maintain such a hook script.
Only those who use the "feature" would.

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

[Marc Branchaud]

On 2018-04-25 04:48 AM, Junio C Hamano wrote:
> "Robin H. Johnson" <robbat2@gentoo.org> writes:
> 
>> In the thread from 6 years ago, you asked about tar's behavior for
>> mtimes. 'tar xf' restores mtimes from the tar archive, so relative
>> ordering after restore would be the same, and would only rebuild if the
>> original source happened to be dirty.
>>
>> This behavior is already non-deterministic in Git, and would be improved
>> by the patch.
> 
> But Git is not an archiver (tar), but is a source code control
> system, so I do not think we should spend any extra cycles to
> "improve" its behaviour wrt the relative ordering, at least for the
> default case.  Only those who rely on having build artifact *and*
> source should pay the runtime (and preferrably also the
> maintainance) cost.

Anyone who uses "make" or some other mtime-based tool is affected by 
this.  I agree that it's not "Everyone" but it sure is a lot of people.

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.

> The best approach to do so is to have those people do the "touch"
> thing in their own post-checkout hook.  People who use Git as the
> source control system won't have to pay runtime cost of doing the
> touch thing, and we do not have to maintain such a hook script.
> Only those who use the "feature" would.

The post-checkout hook approach is not exactly straightforward.

Naively, it's simply

	for F in `git diff --name-only $1 $2`; do touch "$F"; done

But consider:

* Symlinks can cause the wrong file to be touched.  (Granted, Michał's 
proposed patch also doesn't deal with symlinks.)  Let's assume that a 
hook can be crafted will all possible sophistication.  There are still 
some fundamental problems:

* 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.

* The hook has to be set up in every repo and submodule (at least until 
something like Ævar's experiments come to fruition).

* A fresh clone can't run the hook.  This is especially important when 
dealing with submodules.  (In one case where we were bit by this, make 
though that half of a fresh submodule clone's files were stale, and 
decided to re-autoconf the entire thing.)


I just don't think the hook approach can completely solve the problem.

I appreciate Ævar's concern that there are more than just two mtime 
requests floating around.  But I think git's users are best served by a 
built-in approach, with a config setting to control the desired mtime 
handling (defaulting to the current behaviour).  People who want a 
different mtime solution will at least have a clear place in the code to 
propose a patch.

		M.

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

[Robin H. Johnson]

On Wed, Apr 25, 2018 at 11:18:26AM -0400, Marc Branchaud wrote:
> > The best approach to do so is to have those people do the "touch"
> > thing in their own post-checkout hook.  People who use Git as the
> > source control system won't have to pay runtime cost of doing the
> > touch thing, and we do not have to maintain such a hook script.
> > Only those who use the "feature" would.
> 
> The post-checkout hook approach is not exactly straightforward.
> 
> Naively, it's simply
> 
> 	for F in `git diff --name-only $1 $2`; do touch "$F"; done
Even this naive attempt gets it wrong: successive files have increasing
times.  You need to capture the time at the start, and use it as the time
for the files.
  touch /tmp/ref && \
  for F in `git diff --name-only $1 $2`; do touch -r /tmp/ref "$F"; done && \
  rm /tmp/ref
(or pass a fixed time into touch).

> But consider:
> 
> * Symlinks can cause the wrong file to be touched.  (Granted, Michał's 
> proposed patch also doesn't deal with symlinks.) 
Yes, it blindly touches the file, and rather than trying to do
utimensat's AT_SYMLINK_NOFOLLOW flag.

> Let's assume that a 
> hook can be crafted will all possible sophistication.  There are still 
> some fundamental problems:
>
> * 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.
Yes, this would need to be a new hook that knows more than post-checkout
presently does.

post-checkout right now runs AFTER the worktree has been updated, and
only gets the refs of old/new HEAD and if the branch was changed.
It does NOT know which files were actually modified, and since it

If a hook is added for that, then this behavior can be implemented in
the hook. Alternatively adding a pre-checkout hook to absorb some state
of the unmodified worktree (this could be a bit expensive).

> * The hook has to be set up in every repo and submodule (at least until 
> something like Ævar's experiments come to fruition).
> 
> * A fresh clone can't run the hook.  This is especially important when 
> dealing with submodules.  (In one case where we were bit by this, make 
> though that half of a fresh submodule clone's files were stale, and 
> decided to re-autoconf the entire thing.)
The fresh clone case really matters for my usage, where new clones are
firing in CI/CD processes.

> I just don't think the hook approach can completely solve the problem.
> 
> I appreciate Ævar's concern that there are more than just two mtime 
> requests floating around.  But I think git's users are best served by a 
> built-in approach, with a config setting to control the desired mtime 
> handling (defaulting to the current behaviour).  People who want a 
> different mtime solution will at least have a clear place in the code to 
> propose a patch.
+1 as long as we can set the behavior during the clone.

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

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

[Junio C Hamano]

Marc Branchaud <marcnarc@xiplink.com> writes:

>> But Git is not an archiver (tar), but is a source code control
>> system, so I do not think we should spend any extra cycles to
>> "improve" its behaviour wrt the relative ordering, at least for the
>> default case.  Only those who rely on having build artifact *and*
>> source should pay the runtime (and preferrably also the
>> maintainance) cost.
>
> Anyone who uses "make" or some other mtime-based tool is affected by
> this.  I agree that it's not "Everyone" but it sure is a lot of
> people.

That's an exaggerated misrepresentation.  Only those who put build
artifacts as well as source to SCM *AND* depend on mtime are
affected.

A shipped tarball often contain configure.in as well as generated
configure, so that consumers can just say ./configure without having
the whole autoconf toolchain to regenerate it (I also heard horror
stories that this is done to control the exact version of autoconf
to avoid compatibility issues), but do people arrange configure to
be regenerated from configure.in in their Makefile of such a project
automatically when building the default target?  In any case, that is
a tarball usecase, not a SCM one.

> 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 do not know about others, but I personally am more disburbed by
the conceptual ugliness that comes from having to have such a piece
of code in the codebase.

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

[Marc Branchaud]

On 2018-04-25 09:25 PM, Junio C Hamano wrote:
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
>>> But Git is not an archiver (tar), but is a source code control
>>> system, so I do not think we should spend any extra cycles to
>>> "improve" its behaviour wrt the relative ordering, at least for the
>>> default case.  Only those who rely on having build artifact *and*
>>> source should pay the runtime (and preferrably also the
>>> maintainance) cost.
>>
>> Anyone who uses "make" or some other mtime-based tool is affected by
>> this.  I agree that it's not "Everyone" but it sure is a lot of
>> people.
> 
> That's an exaggerated misrepresentation.  Only those who put build
> artifacts as well as source to SCM *AND* depend on mtime are
> affected.
> 
> A shipped tarball often contain configure.in as well as generated
> configure, so that consumers can just say ./configure without having
> the whole autoconf toolchain to regenerate it (I also heard horror
> stories that this is done to control the exact version of autoconf
> to avoid compatibility issues), but do people arrange configure to
> be regenerated from configure.in in their Makefile of such a project
> automatically when building the default target?

Yes.  I've seen many automake-generated Makefiles with "recheck" 
machinery that'll conveniently rerun autoconf if "necessary".

(And those horror stories are true, BTW.)

> In any case, that is
> a tarball usecase, not a SCM one.

No, it's an SCM case when you need to have the project's code as part of 
your own.  I can't make everyone do things the Right Way, so I'm stuck 
using projects that are not 100% pure-source, because they "helpfully" 
release their code after the autoconf step for some reason.

>> 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 do not know about others, but I personally am more disburbed by
> the conceptual ugliness that comes from having to have such a piece
> of code in the codebase.

The ugliness arises from the problem being solved.  It's not git's fault 
that the world is so stupid.

That git might be willing to suffer a bit of self-mutilation for the 
benefit of its users should be seen as a point of pride.

		M.

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

[Michał Górny]

W dniu czw, 26.04.2018 o godzinie 10∶25 +0900, użytkownik Junio C Hamano
napisał:
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
> > > But Git is not an archiver (tar), but is a source code control
> > > system, so I do not think we should spend any extra cycles to
> > > "improve" its behaviour wrt the relative ordering, at least for the
> > > default case.  Only those who rely on having build artifact *and*
> > > source should pay the runtime (and preferrably also the
> > > maintainance) cost.
> > 
> > Anyone who uses "make" or some other mtime-based tool is affected by
> > this.  I agree that it's not "Everyone" but it sure is a lot of
> > people.
> 
> That's an exaggerated misrepresentation.  Only those who put build
> artifacts as well as source to SCM *AND* depend on mtime are
> affected.
> 
> A shipped tarball often contain configure.in as well as generated
> configure, so that consumers can just say ./configure without having
> the whole autoconf toolchain to regenerate it (I also heard horror
> stories that this is done to control the exact version of autoconf
> to avoid compatibility issues), but do people arrange configure to
> be regenerated from configure.in in their Makefile of such a project
> automatically when building the default target?  In any case, that is
> a tarball usecase, not a SCM one.
> 
> > 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 do not know about others, but I personally am more disburbed by
> the conceptual ugliness that comes from having to have such a piece
> of code in the codebase.

For the record, we're using this with ebuilds and respective cache files
(which are expensive to generate).  We are using separate repository
which combines sources and cache files to keep the development
repository clean.  I have researched different solutions for this but
git turned out the best option for incremental updates for us.

Tarballs are out of question, unless you expect users to fetch >100 MiB
every time, and they are also expensive to update.  Deltas of tarballs
are just slow and require storing a lot of extra data.  Rsync is not
very efficient at frequent updates, and has significant overhead
on every run.  With all its disadvantages, git is still something that
lets our users fetch updates frequently with minimal network overhead.

So what did I do to deserve being called insane here?  Is it because I
wanted to use the tools that work for us?  Because I figured out that I
can improve our use case without really harming anyone in the process?

-- 
Best regards,
Michał Górny

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

[Duy Nguyen]

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?

> The post-checkout hook approach is not exactly straightforward.
>
> Naively, it's simply
>
>         for F in `git diff --name-only $1 $2`; do touch "$F"; done
>
> But consider:
>
> * Symlinks can cause the wrong file to be touched.  (Granted, Michał's
> proposed patch also doesn't deal with symlinks.)  Let's assume that a hook
> can be crafted will all possible sophistication.  There are still some
> fundamental problems:
>
> * 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?

> * The hook has to be set up in every repo and submodule (at least until
> something like Ævar's experiments come to fruition).
>
> * A fresh clone can't run the hook.  This is especially important when
> dealing with submodules.  (In one case where we were bit by this, make
> though that half of a fresh submodule clone's files were stale, and decided
> to re-autoconf the entire thing.)

This to me sounds like something we should be able to improve in
general. The alternative is "git clone --no-checkout" then checkout
manually (which I see jenkins plugin does) is really not optimal even
if it works.
-- 
Duy

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

[Duy Nguyen]

On Wed, Apr 25, 2018 at 10:41:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
>  2. Add some config so we can have hook search paths, and ship with a
>     default search path for hooks shipped with git.

I would go for something like this instead of search paths. This
allows you to specify a path to any specific hook in hooks.* config
group. This is basically "$GIT_DIR/hooks directory in config file" but
with lower priority than those in $GIT_DIR/hooks.

Now we can do something like


    git -c hooks.post-checkout=/path/to/some/script clone ...

but of course I would need to fix the FIXME or this hook config is
only effective just this one time. (And of course you could put it in
~/.gitconfig)

-- 8< --
diff --git a/builtin/clone.c b/builtin/clone.c
index 7df5932b85..143413ed2d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1063,6 +1063,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin);
 	}
 
+	/*
+	 * FIXME: we should keep all custom config settings via
+	 * "git  -c ..." in $GIT_DIR/config.
+	 */
+
 	strbuf_addf(&value, "+%s*:%s*", src_ref_prefix, branch_top.buf);
 	strbuf_addf(&key, "remote.%s.url", option_origin);
 	git_config_set(key.buf, repo);
diff --git a/run-command.c b/run-command.c
index 84899e423f..ee5b6ea329 100644
--- a/run-command.c
+++ b/run-command.c
@@ -7,6 +7,7 @@
 #include "strbuf.h"
 #include "string-list.h"
 #include "quote.h"
+#include "config.h"
 
 void child_process_init(struct child_process *child)
 {
@@ -1256,6 +1257,8 @@ const char *find_hook(const char *name)
 	strbuf_reset(&path);
 	strbuf_git_path(&path, "hooks/%s", name);
 	if (access(path.buf, X_OK) < 0) {
+		const char *cfg_hook;
+		struct strbuf cfg_key = STRBUF_INIT;
 		int err = errno;
 
 #ifdef STRIP_EXTENSION
@@ -1278,9 +1281,14 @@ const char *find_hook(const char *name)
 				       path.buf);
 			}
 		}
-		return NULL;
+
+		strbuf_reset(&path);
+		strbuf_addf(&cfg_key, "hooks.%s", name);
+		if (!git_config_get_pathname(cfg_key.buf, &cfg_hook))
+			strbuf_addstr(&path, cfg_hook);
+		strbuf_release(&cfg_key);
 	}
-	return path.buf;
+	return path.len ? path.buf : NULL;
 }
 
 int run_hook_ve(const char *const *env, const char *name, va_list args)
-- 8< --
--
Duy

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

[Robin H. Johnson]

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 *. 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.

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

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

[Robin H. Johnson]

On Thu, Apr 26, 2018 at 07:15:40PM +0200, Duy Nguyen wrote:
> On Wed, Apr 25, 2018 at 10:41:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
> >  2. Add some config so we can have hook search paths, and ship with a
> >     default search path for hooks shipped with git.
> 
> I would go for something like this instead of search paths. This
> allows you to specify a path to any specific hook in hooks.* config
> group. This is basically "$GIT_DIR/hooks directory in config file" but
> with lower priority than those in $GIT_DIR/hooks.
> 
> Now we can do something like
> 
> 
>     git -c hooks.post-checkout=/path/to/some/script clone ...
> 
> but of course I would need to fix the FIXME or this hook config is
> only effective just this one time. (And of course you could put it in
> ~/.gitconfig)
The FIXME leaves something ambiguous:
How do you differentiate between -c behavior that should be
one-time-only vs persisted by being written to $GIT_DIR/config during
$GIR_DIR init?

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

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

[SZEDER Gábor]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1394 bytes --]

> On Wed, Apr 25, 2018 at 10:41:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
> >  2. Add some config so we can have hook search paths, and ship with a
> >     default search path for hooks shipped with git.
> 
> I would go for something like this instead of search paths. This
> allows you to specify a path to any specific hook in hooks.* config
> group. This is basically "$GIT_DIR/hooks directory in config file" but
> with lower priority than those in $GIT_DIR/hooks.
> 
> Now we can do something like
> 
> 
>     git -c hooks.post-checkout=/path/to/some/script clone ...
> 
> but of course I would need to fix the FIXME or this hook config is
> only effective just this one time. (And of course you could put it in
> ~/.gitconfig)
> 
> -- 8< --
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 7df5932b85..143413ed2d 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1063,6 +1063,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin);
>  	}
>  
> +	/*
> +	 * FIXME: we should keep all custom config settings via
> +	 * "git  -c ..." in $GIT_DIR/config.
> +	 */
> +

We definitely should not, see the difference between 'git -c ... clone
url' and 'git clone -c ... url'

BTW, wouldn't running

  git clone --template=/path/to/template/dir/with/hooks/

invoke the post-checkout hook in there?

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

[Duy Nguyen]

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

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

[Duy Nguyen]

On Thu, Apr 26, 2018 at 07:53:58PM +0200, SZEDER Gábor wrote:
> BTW, wouldn't running
> 
>   git clone --template=/path/to/template/dir/with/hooks/
> 
> invoke the post-checkout hook in there?
> 

Yes but it's cumbersome, preparing a template just for one extra
hook. I never like this template thing to be honest.
--
Duy

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

[Duy Nguyen]

On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
>> The best approach to do so is to have those people do the "touch"
>> thing in their own post-checkout hook.  People who use Git as the
>> source control system won't have to pay runtime cost of doing the
>> touch thing, and we do not have to maintain such a hook script.
>> Only those who use the "feature" would.
>
>
> The post-checkout hook approach is not exactly straightforward.

I am revisiting this because I'm not even happy with my
post-checkout-modified hook suggestion, so..

>
> Naively, it's simply
>
>         for F in `git diff --name-only $1 $2`; do touch "$F"; done
>
> But consider:
>
> * Symlinks can cause the wrong file to be touched.  (Granted, Michał's
> proposed patch also doesn't deal with symlinks.)  Let's assume that a hook
> can be crafted will all possible sophistication.  There are still some
> fundamental problems:

OK so this one could be tricky to get right, but it's possible.

>
> * 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.

This is a limitation of the current post-checkout hook. $3==0 from the
hook lets us know this is not a branch switch, but it does not really
tell you the affected paths. If it somehow passes all the given
pathspec to you, then you should be able to do "git ls-files --
$pathspec" which gives you the exact same set of paths that
git-checkout updates. We could do this by setting $4 to "--" and put
all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
the above example.

There is  third case here, if you do "git checkout <tree-ish> --
path/to/file" then it cannot be covered by the current design. I guess
we could set $3 to '2' (retrieve from a tree) to indicate this in
addition to 0 (from index) and 1 (from switching branch) and then $1
could be the tree in question (pathspecs are passed the same way
above)

[1] I wonder if we could have a more generic approach to pass
pathspecs via environment, which could work for more than just this
one hook. Not sure if it's a good idea though.

> * The hook has to be set up in every repo and submodule (at least until
> something like Ævar's experiments come to fruition).

Either --template or core.hooksPath would solve this, or I'll try to
get my "hooks.* config" patch in. I think that one is a good thing to
do anyway because it allows more flexible hook management (and it
could allow multiple hooks of the same name too). With this, we could
avoid adding more command-specific config like core.fsmonitor or
uploadpack.packObjectsHook which to me are hooks.

Another option is extend core.hooksPath for searching multiple places
instead of just one like AEvar suggested.

> * A fresh clone can't run the hook.  This is especially important when
> dealing with submodules.  (In one case where we were bit by this, make
> though that half of a fresh submodule clone's files were stale, and decided
> to re-autoconf the entire thing.)

Both --template and config-based hooks should let you handle this case.

So, I think if we improve the hook system to give more information
(which is something we definitely should do), we could do this without
adding special cases in git. I'm not saying that we should never add
special cases, but at least this lets us play with different kinds of
post-checkout activities before we decide which one would be best
implemented in git.
-- 
Duy

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

[Michał Górny]

W dniu śro, 25.04.2018 o godzinie 11∶18 -0400, użytkownik Marc Branchaud
napisał:
> On 2018-04-25 04:48 AM, Junio C Hamano wrote:
> > "Robin H. Johnson" <robbat2@gentoo.org> writes:
> > 
> > > In the thread from 6 years ago, you asked about tar's behavior for
> > > mtimes. 'tar xf' restores mtimes from the tar archive, so relative
> > > ordering after restore would be the same, and would only rebuild if the
> > > original source happened to be dirty.
> > > 
> > > This behavior is already non-deterministic in Git, and would be improved
> > > by the patch.
> > 
> > But Git is not an archiver (tar), but is a source code control
> > system, so I do not think we should spend any extra cycles to
> > "improve" its behaviour wrt the relative ordering, at least for the
> > default case.  Only those who rely on having build artifact *and*
> > source should pay the runtime (and preferrably also the
> > maintainance) cost.
> 
> Anyone who uses "make" or some other mtime-based tool is affected by 
> this.  I agree that it's not "Everyone" but it sure is a lot of people.
> 
> 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.
> 
> > The best approach to do so is to have those people do the "touch"
> > thing in their own post-checkout hook.  People who use Git as the
> > source control system won't have to pay runtime cost of doing the
> > touch thing, and we do not have to maintain such a hook script.
> > Only those who use the "feature" would.
> 
> The post-checkout hook approach is not exactly straightforward.
> 
> Naively, it's simply
> 
> 	for F in `git diff --name-only $1 $2`; do touch "$F"; done
> 
> But consider:
> 
> * Symlinks can cause the wrong file to be touched.  (Granted, Michał's 
> proposed patch also doesn't deal with symlinks.)  Let's assume that a 
> hook can be crafted will all possible sophistication.  There are still 
> some fundamental problems:
> 
> * 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.
> 
> * The hook has to be set up in every repo and submodule (at least until 
> something like Ævar's experiments come to fruition).
> 
> * A fresh clone can't run the hook.  This is especially important when 
> dealing with submodules.  (In one case where we were bit by this, make 
> though that half of a fresh submodule clone's files were stale, and 
> decided to re-autoconf the entire thing.)
> 
> 
> I just don't think the hook approach can completely solve the problem.
> 

There's also the performance aspect.  If we deal with checkouts that
include 1000+ files on a busy system (i.e. when mtimes really become
relevant), calling utime() instantly has a good chance of hitting warm
cache.  On the other hand, post-checkout hook has a greater risk of
running cold cache, i.e. writing to all inodes twice.

-- 
Best regards,
Michał Górny

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

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 27, 2018 at 7:18 PM, Michał Górny <mgorny@gentoo.org> wrote:
> W dniu śro, 25.04.2018 o godzinie 11∶18 -0400, użytkownik Marc Branchaud
> napisał:
>> On 2018-04-25 04:48 AM, Junio C Hamano wrote:
>> > "Robin H. Johnson" <robbat2@gentoo.org> writes:
>> >
>> > > In the thread from 6 years ago, you asked about tar's behavior for
>> > > mtimes. 'tar xf' restores mtimes from the tar archive, so relative
>> > > ordering after restore would be the same, and would only rebuild if the
>> > > original source happened to be dirty.
>> > >
>> > > This behavior is already non-deterministic in Git, and would be improved
>> > > by the patch.
>> >
>> > But Git is not an archiver (tar), but is a source code control
>> > system, so I do not think we should spend any extra cycles to
>> > "improve" its behaviour wrt the relative ordering, at least for the
>> > default case.  Only those who rely on having build artifact *and*
>> > source should pay the runtime (and preferrably also the
>> > maintainance) cost.
>>
>> Anyone who uses "make" or some other mtime-based tool is affected by
>> this.  I agree that it's not "Everyone" but it sure is a lot of people.
>>
>> 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.
>>
>> > The best approach to do so is to have those people do the "touch"
>> > thing in their own post-checkout hook.  People who use Git as the
>> > source control system won't have to pay runtime cost of doing the
>> > touch thing, and we do not have to maintain such a hook script.
>> > Only those who use the "feature" would.
>>
>> The post-checkout hook approach is not exactly straightforward.
>>
>> Naively, it's simply
>>
>>       for F in `git diff --name-only $1 $2`; do touch "$F"; done
>>
>> But consider:
>>
>> * Symlinks can cause the wrong file to be touched.  (Granted, Michał's
>> proposed patch also doesn't deal with symlinks.)  Let's assume that a
>> hook can be crafted will all possible sophistication.  There are still
>> some fundamental problems:
>>
>> * 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.
>>
>> * The hook has to be set up in every repo and submodule (at least until
>> something like Ævar's experiments come to fruition).
>>
>> * A fresh clone can't run the hook.  This is especially important when
>> dealing with submodules.  (In one case where we were bit by this, make
>> though that half of a fresh submodule clone's files were stale, and
>> decided to re-autoconf the entire thing.)
>>
>>
>> I just don't think the hook approach can completely solve the problem.
>>
>
> There's also the performance aspect.  If we deal with checkouts that
> include 1000+ files on a busy system (i.e. when mtimes really become
> relevant), calling utime() instantly has a good chance of hitting warm
> cache.  On the other hand, post-checkout hook has a greater risk of
> running cold cache, i.e. writing to all inodes twice.

The FS cache is evicted on a LRU basis. What you're saying is true,
but in the two different implementations there's maybe a 2-3 second
gap between what git is doing and the post-checkout hook is doing. If
the system is under such memory pressure that you've evicted the pages
you just touched you're probably screwed anyway. Maybe I've missed
something here, but this point seems moot.

There's certainly other good arguments against using the current hook
implementation for this, e.g. not being able to do this on clone as
noted upthread.

I think patches that made this configurable in some way in git would
be worth looking at, and due to the subject matter it might make sense
to have it in the core distribution as a non-hook, but I think the
default behavior should always be what it is now, since almost nobody
cares about these edge case,s and users should have to opt-in to use
behavior to work around them.

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

[Elijah Newren]

On Fri, Apr 27, 2018 at 10:03 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
>>
>> * 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.
>
> This is a limitation of the current post-checkout hook. $3==0 from the
> hook lets us know this is not a branch switch, but it does not really
> tell you the affected paths. If it somehow passes all the given
> pathspec to you, then you should be able to do "git ls-files --
> $pathspec" which gives you the exact same set of paths that
> git-checkout updates. We could do this by setting $4 to "--" and put
> all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
> the above example.
>
> There is  third case here, if you do "git checkout <tree-ish> --
> path/to/file" then it cannot be covered by the current design. I guess
> we could set $3 to '2' (retrieve from a tree) to indicate this in
> addition to 0 (from index) and 1 (from switching branch) and then $1
> could be the tree in question (pathspecs are passed the same way
> above)
>
> [1] I wonder if we could have a more generic approach to pass
> pathspecs via environment, which could work for more than just this
> one hook. Not sure if it's a good idea though.

Here's a crazy idea -- maybe instead of a list of pathspecs you just
provide the timestamp of when git checkout started.  Then the hook
could walk the tree, find all files with modification times at least
that late, and modify them all back to the the timestamp of when the
git checkout started.

Would that be enough?  Is that too crazy?

Sure, people could concurrently edit a file or run another program
that modified files, but if you're doing that you're already playing
race games with whether your next incremental build is going to be
able to be correct.  (Some (annoying) IDEs explicitly lock you out
from editing files during a build to attempt to avoid this very
problem.)

That does leave one other caveat: If people intentionally do really
weird stuff with having files with modification timestamps far in the
future.  However, it seems likely that the group of people doing that,
if non-zero in number, is likely to be dis-joint with the group of
folks that want this special
uniform-timestamp-across-files-in-a-checkout behavior.

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

[Marc Branchaud]

On 2018-04-27 01:03 PM, Duy Nguyen wrote:
> On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
>>> The best approach to do so is to have those people do the "touch"
>>> thing in their own post-checkout hook.  People who use Git as the
>>> source control system won't have to pay runtime cost of doing the
>>> touch thing, and we do not have to maintain such a hook script.
>>> Only those who use the "feature" would.
>>
>>
>> The post-checkout hook approach is not exactly straightforward.
> 
> I am revisiting this because I'm not even happy with my
> post-checkout-modified hook suggestion, so..
> 
>>
>> Naively, it's simply
>>
>>          for F in `git diff --name-only $1 $2`; do touch "$F"; done
>>
>> But consider:
>>
>> * Symlinks can cause the wrong file to be touched.  (Granted, Michał's
>> proposed patch also doesn't deal with symlinks.)  Let's assume that a hook
>> can be crafted will all possible sophistication.  There are still some
>> fundamental problems:
> 
> OK so this one could be tricky to get right, but it's possible.
> 
>>
>> * 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.
> 
> This is a limitation of the current post-checkout hook. $3==0 from the
> hook lets us know this is not a branch switch, but it does not really
> tell you the affected paths. If it somehow passes all the given
> pathspec to you, then you should be able to do "git ls-files --
> $pathspec" which gives you the exact same set of paths that
> git-checkout updates. We could do this by setting $4 to "--" and put
> all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
> the above example.
> 
> There is  third case here, if you do "git checkout <tree-ish> --
> path/to/file" then it cannot be covered by the current design. I guess
> we could set $3 to '2' (retrieve from a tree) to indicate this in
> addition to 0 (from index) and 1 (from switching branch) and then $1
> could be the tree in question (pathspecs are passed the same way
> above)
> 
> [1] I wonder if we could have a more generic approach to pass
> pathspecs via environment, which could work for more than just this
> one hook. Not sure if it's a good idea though.

I think there needs to be something other than listing all the paths in 
the command is viable, because it's too easy to hit some 
command-line-length limit.  It would also be good if hook authors didn't 
have to re-invent the wheel of determining the changed paths for every 
corner-case.

My first instinct is to write them one-per-line on the hook's stdin. 
That's probably not generic enough for most hooks, but it seems like a 
good approach for this proposal.

Throwing them into a temporary file with a known name is also good --- 
better, I think, than stuffing them into an environment variable.

		M.

>> * The hook has to be set up in every repo and submodule (at least until
>> something like Ævar's experiments come to fruition).
> 
> Either --template or core.hooksPath would solve this, or I'll try to
> get my "hooks.* config" patch in. I think that one is a good thing to
> do anyway because it allows more flexible hook management (and it
> could allow multiple hooks of the same name too). With this, we could
> avoid adding more command-specific config like core.fsmonitor or
> uploadpack.packObjectsHook which to me are hooks.
> 
> Another option is extend core.hooksPath for searching multiple places
> instead of just one like AEvar suggested.



>> * A fresh clone can't run the hook.  This is especially important when
>> dealing with submodules.  (In one case where we were bit by this, make
>> though that half of a fresh submodule clone's files were stale, and decided
>> to re-autoconf the entire thing.)
> 
> Both --template and config-based hooks should let you handle this case.
> 
> So, I think if we improve the hook system to give more information
> (which is something we definitely should do), we could do this without
> adding special cases in git. I'm not saying that we should never add
> special cases, but at least this lets us play with different kinds of
> post-checkout activities before we decide which one would be best
> implemented in git.
> 

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

[Duy Nguyen]

On Fri, Apr 27, 2018 at 11:08 PM, Elijah Newren <newren@gmail.com> wrote:
> On Fri, Apr 27, 2018 at 10:03 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
>>>
>>> * 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.
>>
>> This is a limitation of the current post-checkout hook. $3==0 from the
>> hook lets us know this is not a branch switch, but it does not really
>> tell you the affected paths. If it somehow passes all the given
>> pathspec to you, then you should be able to do "git ls-files --
>> $pathspec" which gives you the exact same set of paths that
>> git-checkout updates. We could do this by setting $4 to "--" and put
>> all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
>> the above example.
>>
>> There is  third case here, if you do "git checkout <tree-ish> --
>> path/to/file" then it cannot be covered by the current design. I guess
>> we could set $3 to '2' (retrieve from a tree) to indicate this in
>> addition to 0 (from index) and 1 (from switching branch) and then $1
>> could be the tree in question (pathspecs are passed the same way
>> above)
>>
>> [1] I wonder if we could have a more generic approach to pass
>> pathspecs via environment, which could work for more than just this
>> one hook. Not sure if it's a good idea though.
>
> Here's a crazy idea -- maybe instead of a list of pathspecs you just
> provide the timestamp of when git checkout started.  Then the hook
> could walk the tree, find all files with modification times at least
> that late, and modify them all back to the the timestamp of when the
> git checkout started.
>
> Would that be enough?  Is that too crazy?

For this use case? Probably (assuming that timestamp precision does
not cause problems).

I'm more concerned about what info post-checkout hook should provide
but does not. Giving hook writer a way to get the list of updated
files lets them do more fancy stuff while providing checkout start
time probably only helps just this one case.

Providing start time in general for all hooks sounds like a good thing
though (and simple enough to implement).
-- 
Duy

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

[Duy Nguyen]

On Fri, Apr 27, 2018 at 11:08 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
>> On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud <marcnarc@xiplink.com>
>> This is a limitation of the current post-checkout hook. $3==0 from the
>> hook lets us know this is not a branch switch, but it does not really
>> tell you the affected paths. If it somehow passes all the given
>> pathspec to you, then you should be able to do "git ls-files --
>> $pathspec" which gives you the exact same set of paths that
>> git-checkout updates. We could do this by setting $4 to "--" and put
>> all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
>> the above example.
>>
>> There is  third case here, if you do "git checkout <tree-ish> --
>> path/to/file" then it cannot be covered by the current design. I guess
>> we could set $3 to '2' (retrieve from a tree) to indicate this in
>> addition to 0 (from index) and 1 (from switching branch) and then $1
>> could be the tree in question (pathspecs are passed the same way
>> above)
>>
>> [1] I wonder if we could have a more generic approach to pass
>> pathspecs via environment, which could work for more than just this
>> one hook. Not sure if it's a good idea though.
>
>
> I think there needs to be something other than listing all the paths in the
> command is viable, because it's too easy to hit some command-line-length
> limit.

I send pathspecs, not paths. If you type "git checkout -- foo/" then I
send exactly "foo/" not every paths in it. You can always figure that
out with git-ls-files.

Sure this can still hit command length limit when you do "git checkout
-- foo/*" and have lots of files in foo just one more param from
hitting the limit, then the hook may hit the limit because we need
more command line arguments. But this is the corner case I don't think
we should really need to care about.

> It would also be good if hook authors didn't have to re-invent the
> wheel of determining the changed paths for every corner-case.

Flexibility vs convenience I guess. A sample hook as template should
help the reinvention.

> My first instinct is to write them one-per-line on the hook's stdin. That's
> probably not generic enough for most hooks, but it seems like a good
> approach for this proposal.
>
> Throwing them into a temporary file with a known name is also good ---
> better, I think, than stuffing them into an environment variable.

This goes back to my post-checkout-modified proposal. If you're
writing to file, might as well reuse the index format. Then you can
read it with ls-files (which lets you decide path separator or even
quoting, I'm not sure) and it also provides some more info like file
hashes, access time...
-- 
Duy

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

[Duy Nguyen]

On Thu, Apr 26, 2018 at 4:46 PM, Michał Górny <mgorny@gentoo.org> wrote:
> For the record, we're using this with ebuilds and respective cache files
> (which are expensive to generate).  We are using separate repository
> which combines sources and cache files to keep the development
> repository clean.  I have researched different solutions for this but
> git turned out the best option for incremental updates for us.
>
> Tarballs are out of question, unless you expect users to fetch >100 MiB
> every time, and they are also expensive to update.  Deltas of tarballs
> are just slow and require storing a lot of extra data.  Rsync is not
> very efficient at frequent updates, and has significant overhead
> on every run.  With all its disadvantages, git is still something that
> lets our users fetch updates frequently with minimal network overhead.

I assume you're talking about the metadata directory in gentoo-x86
repo. This specific case could be solved by renaming metadata to
_metadata or something to put it on the top. "git checkout" always
updates files in strcmp(path) order. This guarantees time(_metadata)
<= time(ebuild) for all ebuilds without any extra touching (either in
git or in a post-checkout hook)

The behavior has been this way since forever and as far as I can tell
very unlikely to change at least for branch switching (major changes
involved around the index). It's a bit easier to accidentally change
how "git checkout -- path" works though. I don't know if we could just
make this checkout order a promise and guarantee not to break it
though. For it it does not sound like it adds extra maintenance
burden.
-- 
Duy

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

[Michał Górny]

W dniu sob, 28.04.2018 o godzinie 16∶23 +0200, użytkownik Duy Nguyen
napisał:
> On Thu, Apr 26, 2018 at 4:46 PM, Michał Górny <mgorny@gentoo.org> wrote:
> > For the record, we're using this with ebuilds and respective cache files
> > (which are expensive to generate).  We are using separate repository
> > which combines sources and cache files to keep the development
> > repository clean.  I have researched different solutions for this but
> > git turned out the best option for incremental updates for us.
> > 
> > Tarballs are out of question, unless you expect users to fetch >100 MiB
> > every time, and they are also expensive to update.  Deltas of tarballs
> > are just slow and require storing a lot of extra data.  Rsync is not
> > very efficient at frequent updates, and has significant overhead
> > on every run.  With all its disadvantages, git is still something that
> > lets our users fetch updates frequently with minimal network overhead.
> 
> I assume you're talking about the metadata directory in gentoo-x86
> repo. This specific case could be solved by renaming metadata to
> _metadata or something to put it on the top. "git checkout" always
> updates files in strcmp(path) order. This guarantees time(_metadata)
> <= time(ebuild) for all ebuilds without any extra touching (either in
> git or in a post-checkout hook)

We can't really rename it without breaking compatibility with all
package managers out there.  Preparing to do such a major change for
the sake of abusing implementation detail of git doesn't look like
a worthwhile idea.

> 
> The behavior has been this way since forever and as far as I can tell
> very unlikely to change at least for branch switching (major changes
> involved around the index). It's a bit easier to accidentally change
> how "git checkout -- path" works though. I don't know if we could just
> make this checkout order a promise and guarantee not to break it
> though. For it it does not sound like it adds extra maintenance
> burden.

-- 
Best regards,
Michał Górny

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Here's a crazy idea -- maybe instead of a list of pathspecs you just
> provide the timestamp of when git checkout started.  Then the hook
> could walk the tree, find all files with modification times at least
> that late, and modify them all back to the the timestamp of when the
> git checkout started.
>
> Would that be enough?  Is that too crazy?
>
> Sure, people could concurrently edit a file or run another program
> that modified files, but if you're doing that you're already playing
> race games with whether your next incremental build is going to be
> able to be correct.  (Some (annoying) IDEs explicitly lock you out
> from editing files during a build to attempt to avoid this very
> problem.)
>
> That does leave one other caveat: If people intentionally do really
> weird stuff with having files with modification timestamps far in the
> future.  However, it seems likely that the group of people doing that,
> if non-zero in number, is likely to be dis-joint with the group of
> folks that want this special
> uniform-timestamp-across-files-in-a-checkout behavior.

These two groups may share the same degree of insanity ;-)

But the single timestamp idea certainly sounds workable, except that
care must be taken to make sure we really grab the fs timestamp (it
is not uncommon for ">F; stat F" to yield quite different time from
"date", when the filesystem is on a remote box).

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> 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.

That's somewhere in between a tail wagging the dog and a hammer
looking like a solution even when you have a screw.  By passing a
temporary index, you may be able to claim that you are feeding the
necessary information without corruption and in a readable and
native format of Git, and make it up to the reader to grab the paths
out of it, but

 (1) the contents, and probably the cached stat information, in that
     temporary index is duplicated and wasted; you know from the
     time you design this thing that all you want to convey is a
     list of paths.

 (2) it is totally unclear who is responsible for cleaning the
     temporary file up.

 (3) the recipient must walk and carefully grab the path, certainly
     has to "deal with separator (\n or NUL?) or escaping" anyway,
     especially if the reason you use a temporary index is because
     "they can use ls-files on it".  They need to read from ls-files
     anyway, so that is no better than feeding ls-files compatible
     input from the standard input of the hook script.

no?

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

[Duy Nguyen]

On Mon, Apr 30, 2018 at 1:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> 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.
>
> That's somewhere in between a tail wagging the dog and a hammer
> looking like a solution even when you have a screw.  By passing a
> temporary index, you may be able to claim that you are feeding the
> necessary information without corruption and in a readable and
> native format of Git, and make it up to the reader to grab the paths
> out of it, but
>
>  (1) the contents, and probably the cached stat information, in that
>      temporary index is duplicated and wasted; you know from the
>      time you design this thing that all you want to convey is a
>      list of paths.

Yep, I was not happy about this. Which I why I moved to update the
hook calling convention to pass pathspec to the hook instead.

>  (2) it is totally unclear who is responsible for cleaning the
>      temporary file up.

The one that creates it deletes it, which is git.

>  (3) the recipient must walk and carefully grab the path, certainly
>      has to "deal with separator (\n or NUL?) or escaping" anyway,
>      especially if the reason you use a temporary index is because
>      "they can use ls-files on it".  They need to read from ls-files
>      anyway, so that is no better than feeding ls-files compatible
>      input from the standard input of the hook script.

Err "ls-files compatible input" or "ls-files compatible _output_"? If
by input you mean the pathspec we give to ls-files, I agree. If it's
standard ls-files --stage output , letting the hook call ls-files lets
it select the output format it wants (including the potential json
output in the future), which is more flexible.

>
> no?



-- 
Duy

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

[Jeff King]

On Fri, Apr 13, 2018 at 07:01:29PM +0200, Michał Górny wrote:

> In order to avoid unnecessary cache mismatches, force a matching mtime
> between all files created by a single checkout action.  This seems to be
> the best course of action.  Matching mtimes do not trigger cache
> updates.  They also match the concept of 'checkout' being an atomic
> action.  Finally, this change does not break backwards compatibility
> as the new result is a subset of the possible previous results.

There's one case that might be regressed. As long as we assume time
always moves forward, I think you're right, but...

> diff --git a/unpack-trees.c b/unpack-trees.c
> index e73745051..e1efefb68 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -346,6 +346,7 @@ static int check_updates(struct unpack_trees_options *o)
>  	state.quiet = 1;
>  	state.refresh_cache = 1;
>  	state.istate = index;
> +	state.checkout_mtime = time(NULL);

ISTR that it's possible for "system time" to be different from
"filesystem time". Is there any case where the time we get from
time(NULL) might vary wildly from what would be written by the
filesystem if we were to simply write the file? E.g., perhaps on some
types of network-mounted filesystems.

The files in your checkout would all be consistent, but they might be
inconsistent with other files _not_ created by Git (e.g., one might be
saved in your editor). Now you may have introduced skew that cause
"make" to do the wrong thing, because your source and target files are
really operating from two different clocks.

I really don't know how possible or common this is, but I feel like I've
been warned about this distinction in the past. I wouldn't be surprised
to find that it's an archaic thing found only on ancient versions of
NFS, and oral tradition passed down the warnings. But I also would not
be surprised if it's still possible and common.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The files in your checkout would all be consistent, but they might be
> inconsistent with other files _not_ created by Git (e.g., one might be
> saved in your editor). Now you may have introduced skew that cause
> "make" to do the wrong thing, because your source and target files are
> really operating from two different clocks.
>
> I really don't know how possible or common this is, but I feel like I've
> been warned about this distinction in the past. I wouldn't be surprised
> to find that it's an archaic thing found only on ancient versions of
> NFS, and oral tradition passed down the warnings. But I also would not
> be surprised if it's still possible and common.

It was pretty common back when I still was on NFS ;-)  I do not
think we care too deeply about a working tree that spans across
filesystem boundary, so one possible workaround is to read the fs
timestamp back out of the _first_ file we write in the process, and
then consistently use that time for the rest of the files that are
checked out by the same process with utimes().  

I personally still do not think it is worth the complication; these
projects are "holding it wrong" by placing build artifacts in SCM
(not in tarball) ;-).