[PATCH 0/2] git checkout: one bugfix and one cosmetic change

[PATCH 0/2] git checkout: one bugfix and one cosmetic change

[Kjetil Barvik]

Just one small bugfix patch, and one small cosmetic change.

By the way, I wonder how often the list of 'Primary Authors' and
'Contributors' on the webpage http://git-scm.com/about is updated.
Should'nt it be updated when a new release, like v1.6.2, is made?

Kjetil Barvik (2):
  checkout bugfix: use stat.mtime instead of stat.ctime in two places
  make the ST_{C,M}TIME_NSEC macros more function like

[PATCH 1/2] checkout bugfix: use stat.mtime instead of stat.ctime in two places

[Kjetil Barvik]

Commit e1afca4fd "write_index(): update index_state->timestamp after
flushing to disk" on 2009-02-23 used stat.ctime to record the
timestamp of the index-file.  This is wrong, so fix this and use the
correct stat.mtime timestamp instead.

Commit 110c46a909 "Not all systems use st_[cm]tim field for ns
resolution file timestamp" on 2009-03-08, has a similar bug for the
builtin-fetch-pack.c file.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 builtin-fetch-pack.c |    2 +-
 read-cache.c         |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 0b1a356..d571253 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -806,7 +806,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 				die("shallow file was removed during fetch");
 		} else if (st.st_mtime != mtime.sec
 #ifdef USE_NSEC
-				|| ST_CTIME_NSEC(st) != mtime.nsec
+				|| ST_MTIME_NSEC(st) != mtime.nsec
 #endif
 			  )
 			die("shallow file was changed during fetch");
diff --git a/read-cache.c b/read-cache.c
index 7f74c8d..3f58711 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1563,8 +1563,8 @@ int write_index(struct index_state *istate, int newfd)
 
 	if (ce_flush(&c, newfd) || fstat(newfd, &st))
 		return -1;
-	istate->timestamp.sec = (unsigned int)st.st_ctime;
-	istate->timestamp.nsec = ST_CTIME_NSEC(st);
+	istate->timestamp.sec = (unsigned int)st.st_mtime;
+	istate->timestamp.nsec = ST_MTIME_NSEC(st);
 	return 0;
 }
 
-- 
1.6.2.GIT

[PATCH 2/2] make the ST_{C,M}TIME_NSEC macros more function like

[Kjetil Barvik]

Make the macros take a pointer to a 'struct stat'. This is so that it
should be easier to understand what is going on, and that the macros
can later be implemented as a inline function if we want to.

Impact: cosmetic change

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 builtin-fetch-pack.c |    4 ++--
 git-compat-util.h    |    8 ++++----
 read-cache.c         |   12 ++++++------
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index d571253..0cd50f3 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -800,13 +800,13 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 		int fd;
 
 		mtime.sec = st.st_mtime;
-		mtime.nsec = ST_MTIME_NSEC(st);
+		mtime.nsec = ST_MTIME_NSEC(&st);
 		if (stat(shallow, &st)) {
 			if (mtime.sec)
 				die("shallow file was removed during fetch");
 		} else if (st.st_mtime != mtime.sec
 #ifdef USE_NSEC
-				|| ST_MTIME_NSEC(st) != mtime.nsec
+				|| ST_MTIME_NSEC(&st) != mtime.nsec
 #endif
 			  )
 			die("shallow file was changed during fetch");
diff --git a/git-compat-util.h b/git-compat-util.h
index 1906253..4a633be 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -394,11 +394,11 @@ void git_qsort(void *base, size_t nmemb, size_t size,
 #define ST_MTIME_NSEC(st) 0
 #else
 #ifdef USE_ST_TIMESPEC
-#define ST_CTIME_NSEC(st) ((unsigned int)((st).st_ctimespec.tv_nsec))
-#define ST_MTIME_NSEC(st) ((unsigned int)((st).st_mtimespec.tv_nsec))
+#define ST_CTIME_NSEC(st) ((unsigned int)((st)->st_ctimespec.tv_nsec))
+#define ST_MTIME_NSEC(st) ((unsigned int)((st)->st_mtimespec.tv_nsec))
 #else
-#define ST_CTIME_NSEC(st) ((unsigned int)((st).st_ctim.tv_nsec))
-#define ST_MTIME_NSEC(st) ((unsigned int)((st).st_mtim.tv_nsec))
+#define ST_CTIME_NSEC(st) ((unsigned int)((st)->st_ctim.tv_nsec))
+#define ST_MTIME_NSEC(st) ((unsigned int)((st)->st_mtim.tv_nsec))
 #endif
 #endif
 
diff --git a/read-cache.c b/read-cache.c
index 3f58711..cff85e3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -69,8 +69,8 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
 {
 	ce->ce_ctime.sec = (unsigned int)st->st_ctime;
 	ce->ce_mtime.sec = (unsigned int)st->st_mtime;
-	ce->ce_ctime.nsec = ST_CTIME_NSEC(*st);
-	ce->ce_mtime.nsec = ST_MTIME_NSEC(*st);
+	ce->ce_ctime.nsec = ST_CTIME_NSEC(st);
+	ce->ce_mtime.nsec = ST_MTIME_NSEC(st);
 	ce->ce_dev = st->st_dev;
 	ce->ce_ino = st->st_ino;
 	ce->ce_uid = st->st_uid;
@@ -204,9 +204,9 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 		changed |= CTIME_CHANGED;
 
 #ifdef USE_NSEC
-	if (ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
+	if (ce->ce_mtime.nsec != ST_MTIME_NSEC(st))
 		changed |= MTIME_CHANGED;
-	if (trust_ctime && ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
+	if (trust_ctime && ce->ce_ctime.nsec != ST_CTIME_NSEC(st))
 		changed |= CTIME_CHANGED;
 #endif
 
@@ -1299,7 +1299,7 @@ int read_index_from(struct index_state *istate, const char *path)
 		dst_offset += ce_size(ce);
 	}
 	istate->timestamp.sec = st.st_mtime;
-	istate->timestamp.nsec = ST_MTIME_NSEC(st);
+	istate->timestamp.nsec = ST_MTIME_NSEC(&st);
 
 	while (src_offset <= mmap_size - 20 - 8) {
 		/* After an array of active_nr index entries,
@@ -1564,7 +1564,7 @@ int write_index(struct index_state *istate, int newfd)
 	if (ce_flush(&c, newfd) || fstat(newfd, &st))
 		return -1;
 	istate->timestamp.sec = (unsigned int)st.st_mtime;
-	istate->timestamp.nsec = ST_MTIME_NSEC(st);
+	istate->timestamp.nsec = ST_MTIME_NSEC(&st);
 	return 0;
 }
 
-- 
1.6.2.GIT

Re: [PATCH 2/2] make the ST_{C,M}TIME_NSEC macros more function like

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

> Make the macros take a pointer to a 'struct stat'. This is so that it
> should be easier to understand what is going on, and that the macros
> can later be implemented as a inline function if we want to.
>
> Impact: cosmetic change

Hmm,...

I have to wonder if this cosmetic change is an improvement, though.

I do not have a strong feeling either way, but I think it makes it clear
that these two macros are not lvalues if you do not pass a pointer but
instead pass a structure.  An inline function can still take a structure
passed by value as an argument anyway, no?

Re: [PATCH 2/2] make the ST_{C,M}TIME_NSEC macros more function like

[Kjetil Barvik]

Junio C Hamano <gitster@pobox.com> writes:

> Kjetil Barvik <barvik@broadpark.no> writes:
>
>> Make the macros take a pointer to a 'struct stat'. This is so that it
>> should be easier to understand what is going on, and that the macros
>> can later be implemented as a inline function if we want to.
>>
>> Impact: cosmetic change
>
> Hmm,...
>
> I have to wonder if this cosmetic change is an improvement, though.
>
> I do not have a strong feeling either way, but I think it makes it
> clear that these two macros are not lvalues if you do not pass a
> pointer but instead pass a structure.  An inline function can still
> take a structure passed by value as an argument anyway, no?

  It seems to woork from a small gcc test, but since C has call-by-
  value, and http://en.wikipedia.org/wiki/Call_by_value#Call_by_value
  says:

    [...] in C or Pascal, calling a function with a large structure as
    an argument will cause the entire structure to be copied,
    potentially causing serious performance degradation, and mutations
    to the structure are invisible to the caller. [...]

  So in my eyes it make more sense to be consistent and take the address
  of all struct like objects (&st in this case) for all arguments to
  "function-like" things.

  But, since these 2 are macros, which use textual substitution, I guess
  things will work correctly either way, and the compiled result will be
  the same.  But, I still like the more "function friendly" macros.

  -- kjetil

Re: [PATCH 2/2] make the ST_{C,M}TIME_NSEC macros more function like

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

>     [...] in C or Pascal, calling a function with a large structure as
>     an argument will cause the entire structure to be copied,
>     potentially causing serious performance degradation, and mutations
>     to the structure are invisible to the caller. [...]
>
>   So in my eyes it make more sense to be consistent and take the address
>   of all struct like objects (&st in this case) for all arguments to
>   "function-like" things.

Notice the "mutations to the structure are invisible to the caller" part.
The call site of st_ctime_nsec(st) can be sure that st won't be modified,
without checking the definition of the function.

Which is actually a nice property.  When st_ctime_nsec(st) is implemented as
a macro, you _could_ write it in such a way to mutate what is in st, but
the implementation does not do so, and will be unlikely to in the future,
so I think writing it as if it is a function that receives a structure by
value will help readers of the calling code.

And the readability is what we should optimize for when picking from two
ways to write it, and when the generated code is the same.

Re: [PATCH 2/2] make the ST_{C,M}TIME_NSEC macros more function like

[Kjetil Barvik]

Junio C Hamano <gitster@pobox.com> writes:

> Kjetil Barvik <barvik@broadpark.no> writes:
>
>>     [...] in C or Pascal, calling a function with a large structure as
>>     an argument will cause the entire structure to be copied,
>>     potentially causing serious performance degradation, and mutations
>>     to the structure are invisible to the caller. [...]
>>
>>   So in my eyes it make more sense to be consistent and take the address
>>   of all struct like objects (&st in this case) for all arguments to
>>   "function-like" things.
>
> Notice the "mutations to the structure are invisible to the caller" part.
> The call site of st_ctime_nsec(st) can be sure that st won't be modified,
> without checking the definition of the function.
>
> Which is actually a nice property.  When st_ctime_nsec(st) is implemented as
> a macro, you _could_ write it in such a way to mutate what is in st, but
> the implementation does not do so, and will be unlikely to in the future,
> so I think writing it as if it is a function that receives a structure by
> value will help readers of the calling code.
>
> And the readability is what we should optimize for when picking from two
> ways to write it, and when the generated code is the same.

  OK, I guess we can dropp this patch!  :-)

  -- kjetil

Re: [PATCH 0/2] git checkout: one bugfix and one cosmetic change

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

> Just one small bugfix patch, and one small cosmetic change.
>
> By the way, I wonder how often the list of 'Primary Authors' and
> 'Contributors' on the webpage http://git-scm.com/about is updated.
> Should'nt it be updated when a new release, like v1.6.2, is made?

Thanks for noticing.  Though git-scm.com is not under my control, the site
is considered the official git homepage these days, and I am glad to see
improvements to its contents discussed here.  I do not see Scott very
often on this list these days, so I am CC'ing him.

Re: [PATCH 0/2] git checkout: one bugfix and one cosmetic change

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Kjetil Barvik <barvik@broadpark.no> writes:
>
>> Just one small bugfix patch, and one small cosmetic change.
>>
>> By the way, I wonder how often the list of 'Primary Authors' and
>> 'Contributors' on the webpage http://git-scm.com/about is updated.
>> Should'nt it be updated when a new release, like v1.6.2, is made?
>
> Thanks for noticing.  Though git-scm.com is not under my control, the site
> is considered the official git homepage these days, and I am glad to see
> improvements to its contents discussed here.  I do not see Scott very
> often on this list these days, so I am CC'ing him.

Ragarding the list on the page, I have one thought (not complete enough to
be called suggestion) and one datapoint:

 (1) The boundary between the "Primary Authors" vs "Contributors" seems to
     be set at 50 commits with the current table.  This would mean that we
     will have a lot more primary authors as project progresses.  Is this
     desirable?

     We have 14999 non-merge commits as of 1.6.2; perhaps a per-cent (or a
     half per-cent) cutoff rule would give a more balanced and consistent
     view in the longer term [*1*]?

 (2) This script:

     $ git shortlog -s v1.6.1 | sed -e 's/^[ 0-9]*//' >/var/tmp/1
     $ git shortlog -s v1.6.2 | sed -e 's/^[ 0-9]*//' >/var/tmp/2
     $ comm -13 /var/tmp/[12]

     produces the list of new contributors.  There are 39 names [*2*].

[Footnotes]

*1* A more drastic change would be not to have two lists, but just one.

*2* Thanks and welcome.

    Allan Caffee, Ben Walton, Benjamin Kramer, Benjamin Sergeant, Benoit
    Sigoure, Danijel Tasov, David J. Mellor, Devin Doucette, Dévai Tamás,
    Elijah Newren, Eric Kidd, Fabian Franz, Felipe Contreras, Geoffrey
    Thomas, Henrik Austad, Jacob Helwig, Jake Goulding, Jeremy White,
    Johannes Gilger, Jonas Flodén, Keith Cascio, Kjetil Barvik, Marc
    Branchaud, Marc-Andre Lureau, Nazri Ramliy, Pat Notz, Paul Jarc, Peter
    Oberndorfer, Ray Chuan, Roy Lee, Serge van den Boom, Sergei Organov,
    Stefan Karpinski, Tay Ray Chuan, Ted Pavlic, Tor Arne Vestbø, Vitaly
    "_Vi" Shukela, Väinö Järvelä, jidanni@jidanni.org.

Re: [PATCH 0/2] git checkout: one bugfix and one cosmetic change

[Michael J Gruber]

Kjetil Barvik venit, vidit, dixit 15.03.2009 12:38:
> Just one small bugfix patch, and one small cosmetic change.
> 
> By the way, I wonder how often the list of 'Primary Authors' and
> 'Contributors' on the webpage http://git-scm.com/about is updated.
> Should'nt it be updated when a new release, like v1.6.2, is made?

Assuming it looks at all non-merge commits on master, I can tell you it
has been updated after Dec 18 09:55:53 2008 -0800 and before Jan 14
09:29:24 2009 -0800 ;)

Cheers,
Michael

Re: [PATCH 0/2] git checkout: one bugfix and one cosmetic change

[Kris Shannon]

2009/3/15 Kjetil Barvik <barvik@broadpark.no>:
> Just one small bugfix patch, and one small cosmetic change.
>
> By the way, I wonder how often the list of 'Primary Authors' and
> 'Contributors' on the webpage http://git-scm.com/about is updated.
> Should'nt it be updated when a new release, like v1.6.2, is made?
>

I was rather surprised to see my name on that list.  A quick git log
showed my one contribution to git-parse-remote way pack in
August 2005.

I'd forgotten about that and was feeling all warm and fuzzy until I did:
git log -- git-parse-remote

and saw that it was deleted a week later :(

Re: [PATCH 0/2] git checkout: one bugfix and one cosmetic change

[Jeff King]

On Tue, Mar 17, 2009 at 03:56:12PM +1100, Kris Shannon wrote:

> I was rather surprised to see my name on that list.  A quick git log
> showed my one contribution to git-parse-remote way pack in
> August 2005.
> 
> I'd forgotten about that and was feeling all warm and fuzzy until I did:
> git log -- git-parse-remote
> 
> and saw that it was deleted a week later :(

Heh. The current list just counts commits, which is nice and fast. But
one could also "git blame" all of the content from master and credit
people based either on:

  - number of surviving lines in the current codebase (which obviously
    would give very rankings for people, as the number of lines added
    in a commit is not constant)

  - number of commits which have surviving lines

Doing such a calculation would be pretty slow, though, I imagine. And it
would of course remove you from the list. :)

-Peff

Re: [PATCH 0/2] git checkout: one bugfix and one cosmetic change

[Michael J Gruber]

Jeff King venit, vidit, dixit 17.03.2009 09:43:
> On Tue, Mar 17, 2009 at 03:56:12PM +1100, Kris Shannon wrote:
> 
>> I was rather surprised to see my name on that list.  A quick git log
>> showed my one contribution to git-parse-remote way pack in
>> August 2005.
>>
>> I'd forgotten about that and was feeling all warm and fuzzy until I did:
>> git log -- git-parse-remote
>>
>> and saw that it was deleted a week later :(
> 
> Heh. The current list just counts commits, which is nice and fast. But
> one could also "git blame" all of the content from master and credit
> people based either on:
> 
>   - number of surviving lines in the current codebase (which obviously
>     would give very rankings for people, as the number of lines added
>     in a commit is not constant)
> 
>   - number of commits which have surviving lines
> 
> Doing such a calculation would be pretty slow, though, I imagine. And it
> would of course remove you from the list. :)
> 
> -Peff

Maybe we can forge a statement by Canonical, claiming they were among
the top contributors to git? Then GKH would do all the statistics for us ;)

Michael