"git stash list" shows HEAD reflog

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.

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.

-- 
Best regards,
  Vladimir                            mailto:vladimir@thecybershadow.net

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 <redacted>
Signed-off-by: Rene Scharfe <redacted>
---
 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

On Sat, 13 Mar 2010, Ren? Scharfe wrote:

Am 12.03.2010 15:52, schrieb Vladimir Panteleev:

quoted

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?

I actually noticed this last week.  You can reproduce it by doing "git
reflog" on a branch which has been idle for longer than the expiration.
Any 0-byte files in logs/refs/heads would give me this same behavior.

     Dave

Am 13.03.2010 18:41, schrieb Dave Olszewski:

On Sat, 13 Mar 2010, Ren? Scharfe wrote:

quoted

Am 12.03.2010 15:52, schrieb Vladimir Panteleev:

quoted

quoted

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?

I actually noticed this last week.  You can reproduce it by doing "git
reflog" on a branch which has been idle for longer than the expiration.
Any 0-byte files in logs/refs/heads would give me this same behavior.

    Dave

Perhaps something like this?
---
 revision.c             |    4 ++++
 t/t1411-reflog-show.sh |   13 +++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 29721ec..6991475 100644
--- a/revision.c
+++ b/revision.c

@@ -896,6 +896,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 	struct object *object;
 	unsigned char sha1[20];
 	int local_flags;
+	int empty_after, empty_before = !revs->pending.nr;
 
 	dotdot = strstr(arg, "..");
 	if (dotdot) {

@@ -971,6 +972,9 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 		verify_non_filename(revs->prefix, arg);
 	object = get_reference(revs, arg, sha1, flags ^ local_flags);
 	add_pending_object_with_mode(revs, object, arg, mode);
+	empty_after = !revs->pending.nr;
+	if (empty_before && empty_after)
+		die("bad revision '%s' (empty reflog?)", arg);
 	return 0;
 }
 

diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index c18ed8e..3f48c2d 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh

@@ -64,4 +64,17 @@ test_expect_success 'using --date= shows reflog date (oneline)' '
 	test_cmp expect actual
 '
 
+: >expected.out
+cat >expected.err <<'EOF'
+fatal: bad revision 'empty' (empty reflog?)
+EOF
+test_expect_success 'empty reflog file' '
+	git branch empty &&
+	: >.git/logs/refs/heads/empty &&
+
+	test_must_fail git log -g empty >actual.out 2>actual.err &&
+	test_cmp expected.out actual.out &&
+	test_cmp expected.err actual.err
+'
+
 test_done

-- 
1.7.0.2

On Sat, 13 Mar 2010, Ren? Scharfe wrote:

Am 13.03.2010 18:41, schrieb Dave Olszewski:

quoted

On Sat, 13 Mar 2010, Ren? Scharfe wrote:

quoted

Am 12.03.2010 15:52, schrieb Vladimir Panteleev:

quoted

quoted

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?

I actually noticed this last week.  You can reproduce it by doing "git
reflog" on a branch which has been idle for longer than the expiration.
Any 0-byte files in logs/refs/heads would give me this same behavior.

    Dave

Perhaps something like this?

Maybe, although I'm not sure if dying here is the right behavior.  Is an
empty reflog really an error?  I was testing a patch along the lines of
what Vladimir proposed, which was simply to not set the default rev if a
valid user-specified argument was found, whether or not it contains
commits.

On 03/13/2010 09:37 AM, René Scharfe wrote:

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.

Was the old code actually unsafe?  If not, then perhaps the commit
message would be clearer if ", for safety and" were removed.

--Pete

Am 13.03.2010 22:21, schrieb Dave Olszewski:

Maybe, although I'm not sure if dying here is the right behavior.  Is an
empty reflog really an error?

Yeah, that might be a bit heavy-handed.  *ahem*

I was testing a patch along the lines of
what Vladimir proposed, which was simply to not set the default rev if a
valid user-specified argument was found, whether or not it contains
commits.

Sounds more like it.  How did the tests go?  Does it result in empty
output (which is what I would expect from an empty reflog, now that I
stopped and thought about it for a second)?

René

Am 13.03.2010 22:43, schrieb Pete Harlan:

On 03/13/2010 09:37 AM, René Scharfe wrote:

quoted

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.

Was the old code actually unsafe?  If not, then perhaps the commit
message would be clearer if ", for safety and" were removed.

The function silently dropped valid (if long) reflog entries on the
floor.  It's certainly debatable if not doing so is "safer" or merely
"more complete".  The sentence is already long enough in any case, so I
don't mind dropping this part.

René

If a revision is specified, it happens not to have any commits, don't
use the default revision.  By doing so, surprising and undesired
behavior can happen, such as showing the reflog for HEAD when a branch
was specified.

Signed-off-by: Dave Olszewski <redacted>
---

quoted

I was testing a patch along the lines of
what Vladimir proposed, which was simply to not set the default rev if a
valid user-specified argument was found, whether or not it contains
commits.

Sounds more like it.  How did the tests go?  Does it result in empty
output (which is what I would expect from an empty reflog, now that I
stopped and thought about it for a second)?

It seems to work ok(tm)

 revision.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 29721ec..490b484 100644
--- a/revision.c
+++ b/revision.c

@@ -1334,7 +1334,7 @@ static void append_prune_data(const char ***prune_data, const char **av)
  */
 int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def)
 {
-	int i, flags, left, seen_dashdash, read_from_stdin;
+	int i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0;
 	const char **prune_data = NULL;
 
 	/* First, search for "--" */

@@ -1460,6 +1460,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 			append_prune_data(&prune_data, argv + i);
 			break;
 		}
+		else
+			got_rev_arg = 1;
 	}
 
 	if (prune_data)

@@ -1469,7 +1471,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		revs->def = def;
 	if (revs->show_merge)
 		prepare_show_merge(revs);
-	if (revs->def && !revs->pending.nr) {
+	if (revs->def && !revs->pending.nr && !got_rev_arg) {
 		unsigned char sha1[20];
 		struct object *object;
 		unsigned mode;

-- 
1.7.0.2.202.g4e870.dirty

Am 13.03.2010 23:47, schrieb Dave Olszewski:

If a revision is specified, it happens not to have any commits, don't
use the default revision.  By doing so, surprising and undesired
behavior can happen, such as showing the reflog for HEAD when a branch
was specified.

Signed-off-by: Dave Olszewski <redacted>
---

quoted

quoted

I was testing a patch along the lines of
what Vladimir proposed, which was simply to not set the default rev if a
valid user-specified argument was found, whether or not it contains
commits.

Sounds more like it.  How did the tests go?  Does it result in empty
output (which is what I would expect from an empty reflog, now that I
stopped and thought about it for a second)?

It seems to work ok(tm)

 revision.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Thanks.  And here's an updated, squash-able test.

diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index c18ed8e..ba25ff3 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh

@@ -64,4 +64,13 @@ test_expect_success 'using --date= shows reflog date (oneline)' '
 	test_cmp expect actual
 '
 
+: >expect
+test_expect_success 'empty reflog file' '
+	git branch empty &&
+	: >.git/logs/refs/heads/empty &&
+
+	git log -g empty >actual &&
+	test_cmp expect actual
+'
+
 test_done