git checkout: one bugfix and one cosmetic change

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

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.

---
 builtin-fetch-pack.c |    2 +-
 read-cache.c         |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

index 0b1a356..d571253 100644
@@ -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");

index 7f74c8d..3f58711 100644
@@ -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

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

---
 builtin-fetch-pack.c |    4 ++--
 git-compat-util.h    |    8 ++++----
 read-cache.c         |   12 ++++++------
 3 files changed, 12 insertions(+), 12 deletions(-)

index d571253..0cd50f3 100644
@@ -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");

index 1906253..4a633be 100644
@@ -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
 

index 3f58711..cff85e3 100644
@@ -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

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

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

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ä, [email protected].

Kjetil Barvik venit, vidit, dixit 15.03.2009 12:38:

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

2009/3/15 Kjetil Barvik <[email protected]>:

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

Jeff King venit, vidit, dixit 17.03.2009 09:43:

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