[PATCH 4/5] Log ref updates made by fetch.

[PATCH 4/5] Log ref updates made by fetch.

[Shawn Pearce]

If a ref is changed by http-fetch, local-fetch or ssh-fetch
record the change and the remote URL/name in the log for the ref.
This requires loading the config file to check logAllRefUpdates.

Also fixed a bug in the ref lock generation; the log file name was
not being produced right due to a bad prefix length.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

---

 fetch.c       |   17 +++++++++++++++--
 fetch.h       |    3 +++
 http-fetch.c  |    2 ++
 local-fetch.c |    2 ++
 refs.c        |    5 +++--
 ssh-fetch.c   |    2 ++
 6 files changed, 27 insertions(+), 4 deletions(-)

b929e2a5a8f80c8635cf3c54a6d766902cf87434
diff --git a/fetch.c b/fetch.c
index 8bdaacb..fd57684 100644
--- a/fetch.c
+++ b/fetch.c
@@ -8,6 +8,7 @@ #include "blob.h"
 #include "refs.h"
 
 const char *write_ref = NULL;
+const char *write_ref_log_details = NULL;
 
 const unsigned char *current_ref = NULL;
 
@@ -206,13 +207,17 @@ int pull(char *target)
 {
 	struct ref_lock *lock;
 	unsigned char sha1[20];
+	char *msg;
+	int ret;
 
 	save_commit_buffer = 0;
 	track_object_refs = 0;
 	if (write_ref) {
 		lock = lock_ref_sha1(write_ref, current_ref, 1);
-		if (!lock)
+		if (!lock) {
+			error("Can't lock ref %s", write_ref);
 			return -1;
+		}
 	}
 
 	if (!get_recover) {
@@ -234,7 +239,15 @@ int pull(char *target)
 	}
 
 	if (write_ref) {
-		return write_ref_sha1(lock, sha1, "git fetch");
+		if (write_ref_log_details) {
+			msg = xmalloc(strlen(write_ref_log_details) + 12);
+			sprintf(msg, "fetch from %s", write_ref_log_details);
+		} else
+			msg = NULL;
+		ret = write_ref_sha1(lock, sha1, msg ? msg : "fetch (unknown)");
+		if (msg)
+			free(msg);
+		return ret;
 	}
 	return 0;
 }
diff --git a/fetch.h b/fetch.h
index 9837a3d..0011548 100644
--- a/fetch.h
+++ b/fetch.h
@@ -25,6 +25,9 @@ extern int fetch_ref(char *ref, unsigned
 /* If set, the ref filename to write the target value to. */
 extern const char *write_ref;
 
+/* If set additional text will appear in the ref log. */
+extern const char *write_ref_log_details;
+
 /* If set, the hash that the current value of write_ref must be. */
 extern const unsigned char *current_ref;
 
diff --git a/http-fetch.c b/http-fetch.c
index 861644b..cc7bd1f 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -1223,6 +1223,7 @@ int main(int argc, char **argv)
 	int rc = 0;
 
 	setup_git_directory();
+	git_config(git_default_config);
 
 	while (arg < argc && argv[arg][0] == '-') {
 		if (argv[arg][1] == 't') {
@@ -1249,6 +1250,7 @@ int main(int argc, char **argv)
 	}
 	commit_id = argv[arg];
 	url = argv[arg + 1];
+	write_ref_log_details = url;
 
 	http_init();
 
diff --git a/local-fetch.c b/local-fetch.c
index fa9e697..ffa4887 100644
--- a/local-fetch.c
+++ b/local-fetch.c
@@ -208,6 +208,7 @@ int main(int argc, char **argv)
 	int arg = 1;
 
 	setup_git_directory();
+	git_config(git_default_config);
 
 	while (arg < argc && argv[arg][0] == '-') {
 		if (argv[arg][1] == 't')
@@ -239,6 +240,7 @@ int main(int argc, char **argv)
 		usage(local_pull_usage);
 	commit_id = argv[arg];
 	path = argv[arg + 1];
+	write_ref_log_details = path;
 
 	if (pull(commit_id))
 		return 1;
diff --git a/refs.c b/refs.c
index 31cf276..d3ddc82 100644
--- a/refs.c
+++ b/refs.c
@@ -142,6 +142,8 @@ static int do_for_each_ref(const char *b
 			namelen = strlen(de->d_name);
 			if (namelen > 255)
 				continue;
+			if (namelen>5 && !strcmp(de->d_name+namelen-5,".lock"))
+				continue;
 			memcpy(path + baselen, de->d_name, namelen+1);
 			if (stat(git_path("%s", path), &st) < 0)
 				continue;
@@ -296,7 +298,6 @@ static struct ref_lock* lock_ref_sha1_ba
 	plen = strlen(path) - plen;
 	path = resolve_ref(path, lock->old_sha1, mustexist);
 	if (!path) {
-		error("Can't read ref %s", path);
 		unlock_ref(lock);
 		return NULL;
 	}
@@ -326,7 +327,7 @@ struct ref_lock* lock_ref_sha1(const cha
 	if (check_ref_format(ref))
 		return NULL;
 	return lock_ref_sha1_basic(git_path("refs/%s", ref),
-		strlen(ref), old_sha1, mustexist);
+		5 + strlen(ref), old_sha1, mustexist);
 }
 
 struct ref_lock* lock_any_ref_for_update(const char *ref,
diff --git a/ssh-fetch.c b/ssh-fetch.c
index 4eb9e04..e3067b8 100644
--- a/ssh-fetch.c
+++ b/ssh-fetch.c
@@ -132,6 +132,7 @@ int main(int argc, char **argv)
 	if (!prog) prog = "git-ssh-upload";
 
 	setup_git_directory();
+	git_config(git_default_config);
 
 	while (arg < argc && argv[arg][0] == '-') {
 		if (argv[arg][1] == 't') {
@@ -158,6 +159,7 @@ int main(int argc, char **argv)
 	}
 	commit_id = argv[arg];
 	url = argv[arg + 1];
+	write_ref_log_details = url;
 
 	if (setup_connection(&fd_in, &fd_out, prog, url, arg, argv + 1))
 		return 1;
-- 
1.3.2.g7278

Re: [PATCH 4/5] Log ref updates made by fetch.

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

> diff --git a/refs.c b/refs.c
> index 31cf276..d3ddc82 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -142,6 +142,8 @@ static int do_for_each_ref(const char *b
>  			namelen = strlen(de->d_name);
>  			if (namelen > 255)
>  				continue;
> +			if (namelen>5 && !strcmp(de->d_name+namelen-5,".lock"))
> +				continue;
>  			memcpy(path + baselen, de->d_name, namelen+1);
>  			if (stat(git_path("%s", path), &st) < 0)
>  				continue;

Now this got me worried.  Until now I did not realize that we
are clobbering refnames that ends with ".lock" if another ref
with the name without ".locK" is updated.  Because we do not
forbid a name that ends with ".lock" to be used as a refname,
this is an accident waiting to happen.

Not your fault, though.  It was like this ever since the initial
version of refs.c was accepted by Linus.

We could do one of two things: officially forbid any refname
that ends with ".lock", or fix the lockfile naming convention.

Nobody should be relying on what the actual lockfile-to-be-
renamed-to-become-the-real-file is called.  I suspect we would
want to fix refs.c::ref_lock_file_name() to use a name that
would never be used as a refname.

We could make it begin with ".", so the lock file for the master
".git/refs/heads/master" would become ".git/refs/heads/.master",
for example.  That way, we cannot clobber a valid unrelated ref
(".master" is not a valid ref name), and as an added bonus, you
do not even have to have the above hunk.

Hmm?

Re: [PATCH 4/5] Log ref updates made by fetch.

[Shawn Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> Shawn Pearce <spearce@spearce.org> writes:
> 
> > diff --git a/refs.c b/refs.c
> > index 31cf276..d3ddc82 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -142,6 +142,8 @@ static int do_for_each_ref(const char *b
> >  			namelen = strlen(de->d_name);
> >  			if (namelen > 255)
> >  				continue;
> > +			if (namelen>5 && !strcmp(de->d_name+namelen-5,".lock"))
> > +				continue;
> >  			memcpy(path + baselen, de->d_name, namelen+1);
> >  			if (stat(git_path("%s", path), &st) < 0)
> >  				continue;
> 
> Now this got me worried.  Until now I did not realize that we
> are clobbering refnames that ends with ".lock" if another ref
> with the name without ".locK" is updated.  Because we do not
> forbid a name that ends with ".lock" to be used as a refname,
> this is an accident waiting to happen.
> 
> Not your fault, though.  It was like this ever since the initial
> version of refs.c was accepted by Linus.
> 
> We could do one of two things: officially forbid any refname
> that ends with ".lock", or fix the lockfile naming convention.
> 
> Nobody should be relying on what the actual lockfile-to-be-
> renamed-to-become-the-real-file is called.  I suspect we would
> want to fix refs.c::ref_lock_file_name() to use a name that
> would never be used as a refname.
> 
> We could make it begin with ".", so the lock file for the master
> ".git/refs/heads/master" would become ".git/refs/heads/.master",
> for example.  That way, we cannot clobber a valid unrelated ref
> (".master" is not a valid ref name), and as an added bonus, you
> do not even have to have the above hunk.

Not a bad idea.  I found this bug because fetch.c was trying to
work on the bob.lock while it was fetching "heads/bob".  Which
didn't make sense to me, so I put this skip thing in...

I'd want to get all of the "lockers" move to using the common
lock code before changing the lock name to ".master" (for example)
but yes that's a good idea.

-- 
Shawn.