Re: "git stash list" shows HEAD reflog

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Am 12.03.2010 15:52, schrieb Vladimir Panteleev:
> I stumbled upon a curious problem with a repository: the command "git
> stash list" displayed the HEAD reflog instead of the stash list.
> 
> The problem was caused by a very long line in ".git/logs/refs/stash".
> (The stash was based on a commit imported from Subversion, the commit
> message of which didn't follow git conventions.) The entire line was
> longer than 1023 characters, which is the buffer size passed to fgets in
> for_each_recent_reflog_ent. The validation check (buf[len-1] != '\n')
> causes the line to be skipped. The fix should be simple - if the line
> read didn't fit in the buffer, add a newline anyway instead of skipping
> the line entirely.

Thanks, nice analysis.  Patch below; it uses strbuf instead of truncating
the long message, though.

> That doesn't explain why git displayed the HEAD reflog, though. That
> seems to happen thanks to the check (revs->def && !revs->pending.nr) in
> setup_revisions ("HEAD" is the default, as specified in the caller
> cmd_log_init). It looks like (ideally) git shouldn't rely on whether
> revs->pending is empty to decide whether to use the default, but rather
> if a ref was specified by the user or not.

We could add some kind of check there, but with the patch applied I can't
trigger this second issue any more.  It would be nice to have a test script
along with such a sanity check.  Any idea how to cause this error, perhaps
with another type of invalid reflog file?

René


-- >8 --
Subject: for_each_recent_reflog_ent(): use strbuf, fix offset handling

As Vladimir reported, "git log -g refs/stash" surprisingly showed the reflog
of HEAD if the message in the reflog file was too long.  To fix this, convert
for_each_recent_reflog_ent() to use strbuf_getwholeline() instead of fgets(),
for safety and to avoid any size limits for reflog entries.

Also reverse the logic of the part of the function that only looks at file
tails.  It used to close the file if fgets() succeeded.  The following
fgets() call in the while loop was likely to fail in this case, too, so
passing an offset to for_each_recent_reflog_ent() never worked.  Change it to
error out if strbuf_getwholeline() fails instead.

Reported-by: Vladimir Panteleev <vladimir@thecybershadow.net>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 refs.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index f3fcbe0..63e30d7 100644
--- a/refs.c
+++ b/refs.c
@@ -1574,7 +1574,7 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long ofs,
 {
 	const char *logfile;
 	FILE *logfp;
-	char buf[1024];
+	struct strbuf sb = STRBUF_INIT;
 	int ret = 0;
 
 	logfile = git_path("logs/%s", ref);
@@ -1587,24 +1587,24 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long ofs,
 		if (fstat(fileno(logfp), &statbuf) ||
 		    statbuf.st_size < ofs ||
 		    fseek(logfp, -ofs, SEEK_END) ||
-		    fgets(buf, sizeof(buf), logfp)) {
+		    strbuf_getwholeline(&sb, logfp, '\n')) {
 			fclose(logfp);
+			strbuf_release(&sb);
 			return -1;
 		}
 	}
 
-	while (fgets(buf, sizeof(buf), logfp)) {
+	while (!strbuf_getwholeline(&sb, logfp, '\n')) {
 		unsigned char osha1[20], nsha1[20];
 		char *email_end, *message;
 		unsigned long timestamp;
-		int len, tz;
+		int tz;
 
 		/* old SP new SP name <email> SP time TAB msg LF */
-		len = strlen(buf);
-		if (len < 83 || buf[len-1] != '\n' ||
-		    get_sha1_hex(buf, osha1) || buf[40] != ' ' ||
-		    get_sha1_hex(buf + 41, nsha1) || buf[81] != ' ' ||
-		    !(email_end = strchr(buf + 82, '>')) ||
+		if (sb.len < 83 || sb.buf[sb.len - 1] != '\n' ||
+		    get_sha1_hex(sb.buf, osha1) || sb.buf[40] != ' ' ||
+		    get_sha1_hex(sb.buf + 41, nsha1) || sb.buf[81] != ' ' ||
+		    !(email_end = strchr(sb.buf + 82, '>')) ||
 		    email_end[1] != ' ' ||
 		    !(timestamp = strtoul(email_end + 2, &message, 10)) ||
 		    !message || message[0] != ' ' ||
@@ -1618,11 +1618,13 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long ofs,
 			message += 6;
 		else
 			message += 7;
-		ret = fn(osha1, nsha1, buf+82, timestamp, tz, message, cb_data);
+		ret = fn(osha1, nsha1, sb.buf + 82, timestamp, tz, message,
+			 cb_data);
 		if (ret)
 			break;
 	}
 	fclose(logfp);
+	strbuf_release(&sb);
 	return ret;
 }
 
-- 
1.7.0.2