{"thread":{"id":"22998","subject":"\"git stash list\" shows HEAD reflog","startedAt":"2010-03-12T14:52:45Z","lastAt":"2010-03-14T06:57:03Z","messageCount":11,"participants":["Vladimir Panteleev","René Scharfe","Dave Olszewski","Pete Harlan","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"136660","messageId":"op.u9gl97fstuzx1w@cybershadow.mshome.net","threadId":"22998","inReplyTo":null,"subject":"\"git stash list\" shows HEAD reflog","fromName":"Vladimir Panteleev","fromEmail":"vladimir@thecybershadow.net","sentAt":"2010-03-12T14:52:45Z","receivedAt":"2010-03-12T14:52:45Z","isPatch":false,"sender":{"key":"vladimir@thecybershadow.net","avatar":null},"body":"I stumbled upon a curious problem with a repository: the command \"git  \nstash list\" displayed the HEAD reflog instead of the stash list.\n\nThe problem was caused by a very long line in \".git/logs/refs/stash\". (The  \nstash was based on a commit imported from Subversion, the commit message  \nof which didn't follow git conventions.) The entire line was longer than  \n1023 characters, which is the buffer size passed to fgets in  \nfor_each_recent_reflog_ent. The validation check (buf[len-1] != '\\n')  \ncauses the line to be skipped. The fix should be simple - if the line read  \ndidn't fit in the buffer, add a newline anyway instead of skipping the  \nline entirely.\n\nThat doesn't explain why git displayed the HEAD reflog, though. That seems  \nto happen thanks to the check (revs->def && !revs->pending.nr) in  \nsetup_revisions (\"HEAD\" is the default, as specified in the caller  \ncmd_log_init). It looks like (ideally) git shouldn't rely on whether  \nrevs->pending is empty to decide whether to use the default, but rather if  \na ref was specified by the user or not.\n\n-- \nBest regards,\n  Vladimir                            mailto:vladimir@thecybershadow.net\n"},{"id":"136722","messageId":"4B9BCD6E.4090902@lsrfire.ath.cx","threadId":"22998","inReplyTo":"op.u9gl97fstuzx1w@cybershadow.mshome.net","subject":"Re: \"git stash list\" shows HEAD reflog","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-03-13T17:37:50Z","receivedAt":"2010-03-13T17:37:50Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.03.2010 15:52, schrieb Vladimir Panteleev:\n> I stumbled upon a curious problem with a repository: the command \"git\n> stash list\" displayed the HEAD reflog instead of the stash list.\n> \n> The problem was caused by a very long line in \".git/logs/refs/stash\".\n> (The stash was based on a commit imported from Subversion, the commit\n> message of which didn't follow git conventions.) The entire line was\n> longer than 1023 characters, which is the buffer size passed to fgets in\n> for_each_recent_reflog_ent. The validation check (buf[len-1] != '\\n')\n> causes the line to be skipped. The fix should be simple - if the line\n> read didn't fit in the buffer, add a newline anyway instead of skipping\n> the line entirely.\n\nThanks, nice analysis.  Patch below; it uses strbuf instead of truncating\nthe long message, though.\n\n> That doesn't explain why git displayed the HEAD reflog, though. That\n> seems to happen thanks to the check (revs->def && !revs->pending.nr) in\n> setup_revisions (\"HEAD\" is the default, as specified in the caller\n> cmd_log_init). It looks like (ideally) git shouldn't rely on whether\n> revs->pending is empty to decide whether to use the default, but rather\n> if a ref was specified by the user or not.\n\nWe could add some kind of check there, but with the patch applied I can't\ntrigger this second issue any more.  It would be nice to have a test script\nalong with such a sanity check.  Any idea how to cause this error, perhaps\nwith another type of invalid reflog file?\n\nRené\n\n\n-- >8 --\nSubject: for_each_recent_reflog_ent(): use strbuf, fix offset handling\n\nAs Vladimir reported, \"git log -g refs/stash\" surprisingly showed the reflog\nof HEAD if the message in the reflog file was too long.  To fix this, convert\nfor_each_recent_reflog_ent() to use strbuf_getwholeline() instead of fgets(),\nfor safety and to avoid any size limits for reflog entries.\n\nAlso reverse the logic of the part of the function that only looks at file\ntails.  It used to close the file if fgets() succeeded.  The following\nfgets() call in the while loop was likely to fail in this case, too, so\npassing an offset to for_each_recent_reflog_ent() never worked.  Change it to\nerror out if strbuf_getwholeline() fails instead.\n\nReported-by: Vladimir Panteleev <vladimir@thecybershadow.net>\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n refs.c |   22 ++++++++++++----------\n 1 files changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f3fcbe0..63e30d7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1574,7 +1574,7 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long ofs,\n {\n \tconst char *logfile;\n \tFILE *logfp;\n-\tchar buf[1024];\n+\tstruct strbuf sb = STRBUF_INIT;\n \tint ret = 0;\n \n \tlogfile = git_path(\"logs/%s\", ref);\n@@ -1587,24 +1587,24 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long ofs,\n \t\tif (fstat(fileno(logfp), &statbuf) ||\n \t\t    statbuf.st_size < ofs ||\n \t\t    fseek(logfp, -ofs, SEEK_END) ||\n-\t\t    fgets(buf, sizeof(buf), logfp)) {\n+\t\t    strbuf_getwholeline(&sb, logfp, '\\n')) {\n \t\t\tfclose(logfp);\n+\t\t\tstrbuf_release(&sb);\n \t\t\treturn -1;\n \t\t}\n \t}\n \n-\twhile (fgets(buf, sizeof(buf), logfp)) {\n+\twhile (!strbuf_getwholeline(&sb, logfp, '\\n')) {\n \t\tunsigned char osha1[20], nsha1[20];\n \t\tchar *email_end, *message;\n \t\tunsigned long timestamp;\n-\t\tint len, tz;\n+\t\tint tz;\n \n \t\t/* old SP new SP name <email> SP time TAB msg LF */\n-\t\tlen = strlen(buf);\n-\t\tif (len < 83 || buf[len-1] != '\\n' ||\n-\t\t    get_sha1_hex(buf, osha1) || buf[40] != ' ' ||\n-\t\t    get_sha1_hex(buf + 41, nsha1) || buf[81] != ' ' ||\n-\t\t    !(email_end = strchr(buf + 82, '>')) ||\n+\t\tif (sb.len < 83 || sb.buf[sb.len - 1] != '\\n' ||\n+\t\t    get_sha1_hex(sb.buf, osha1) || sb.buf[40] != ' ' ||\n+\t\t    get_sha1_hex(sb.buf + 41, nsha1) || sb.buf[81] != ' ' ||\n+\t\t    !(email_end = strchr(sb.buf + 82, '>')) ||\n \t\t    email_end[1] != ' ' ||\n \t\t    !(timestamp = strtoul(email_end + 2, &message, 10)) ||\n \t\t    !message || message[0] != ' ' ||\n@@ -1618,11 +1618,13 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long ofs,\n \t\t\tmessage += 6;\n \t\telse\n \t\t\tmessage += 7;\n-\t\tret = fn(osha1, nsha1, buf+82, timestamp, tz, message, cb_data);\n+\t\tret = fn(osha1, nsha1, sb.buf + 82, timestamp, tz, message,\n+\t\t\t cb_data);\n \t\tif (ret)\n \t\t\tbreak;\n \t}\n \tfclose(logfp);\n+\tstrbuf_release(&sb);\n \treturn ret;\n }\n \n-- \n1.7.0.2\n"},{"id":"136724","messageId":"alpine.DEB.2.00.1003130939240.796@narbuckle.genericorp.net","threadId":"22998","inReplyTo":"4B9BCD6E.4090902@lsrfire.ath.cx","subject":"Re: Re: \"git stash list\" shows HEAD reflog","fromName":"Dave Olszewski","fromEmail":"cxreg@pobox.com","sentAt":"2010-03-13T17:41:27Z","receivedAt":"2010-03-13T17:41:27Z","isPatch":false,"sender":{"key":"cxreg@pobox.com","avatar":"https://avatars.githubusercontent.com/u/55474?v=4"},"body":"On Sat, 13 Mar 2010, Ren? Scharfe wrote:\n\n> Am 12.03.2010 15:52, schrieb Vladimir Panteleev:\n\n>> That doesn't explain why git displayed the HEAD reflog, though. That\n>> seems to happen thanks to the check (revs->def && !revs->pending.nr) in\n>> setup_revisions (\"HEAD\" is the default, as specified in the caller\n>> cmd_log_init). It looks like (ideally) git shouldn't rely on whether\n>> revs->pending is empty to decide whether to use the default, but rather\n>> if a ref was specified by the user or not.\n>\n> We could add some kind of check there, but with the patch applied I can't\n> trigger this second issue any more.  It would be nice to have a test script\n> along with such a sanity check.  Any idea how to cause this error, perhaps\n> with another type of invalid reflog file?\n\nI actually noticed this last week.  You can reproduce it by doing \"git\nreflog\" on a branch which has been idle for longer than the expiration.\nAny 0-byte files in logs/refs/heads would give me this same behavior.\n\n     Dave\n"},{"id":"136725","messageId":"4B9BF171.2000102@lsrfire.ath.cx","threadId":"22998","inReplyTo":"alpine.DEB.2.00.1003130939240.796@narbuckle.genericorp.net","subject":"Re: Re: \"git stash list\" shows HEAD reflog","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-03-13T20:11:29Z","receivedAt":"2010-03-13T20:11:29Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.03.2010 18:41, schrieb Dave Olszewski:\n> On Sat, 13 Mar 2010, Ren? Scharfe wrote:\n> \n>> Am 12.03.2010 15:52, schrieb Vladimir Panteleev:\n> \n>>> That doesn't explain why git displayed the HEAD reflog, though. That\n>>> seems to happen thanks to the check (revs->def && !revs->pending.nr) in\n>>> setup_revisions (\"HEAD\" is the default, as specified in the caller\n>>> cmd_log_init). It looks like (ideally) git shouldn't rely on whether\n>>> revs->pending is empty to decide whether to use the default, but rather\n>>> if a ref was specified by the user or not.\n>>\n>> We could add some kind of check there, but with the patch applied I can't\n>> trigger this second issue any more.  It would be nice to have a test\n>> script\n>> along with such a sanity check.  Any idea how to cause this error,\n>> perhaps\n>> with another type of invalid reflog file?\n> \n> I actually noticed this last week.  You can reproduce it by doing \"git\n> reflog\" on a branch which has been idle for longer than the expiration.\n> Any 0-byte files in logs/refs/heads would give me this same behavior.\n> \n>     Dave\n\nPerhaps something like this?\n---\n revision.c             |    4 ++++\n t/t1411-reflog-show.sh |   13 +++++++++++++\n 2 files changed, 17 insertions(+), 0 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 29721ec..6991475 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -896,6 +896,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \tstruct object *object;\n \tunsigned char sha1[20];\n \tint local_flags;\n+\tint empty_after, empty_before = !revs->pending.nr;\n \n \tdotdot = strstr(arg, \"..\");\n \tif (dotdot) {\n@@ -971,6 +972,9 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \t\tverify_non_filename(revs->prefix, arg);\n \tobject = get_reference(revs, arg, sha1, flags ^ local_flags);\n \tadd_pending_object_with_mode(revs, object, arg, mode);\n+\tempty_after = !revs->pending.nr;\n+\tif (empty_before && empty_after)\n+\t\tdie(\"bad revision '%s' (empty reflog?)\", arg);\n \treturn 0;\n }\n \ndiff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh\nindex c18ed8e..3f48c2d 100755\n--- a/t/t1411-reflog-show.sh\n+++ b/t/t1411-reflog-show.sh\n@@ -64,4 +64,17 @@ test_expect_success 'using --date= shows reflog date (oneline)' '\n \ttest_cmp expect actual\n '\n \n+: >expected.out\n+cat >expected.err <<'EOF'\n+fatal: bad revision 'empty' (empty reflog?)\n+EOF\n+test_expect_success 'empty reflog file' '\n+\tgit branch empty &&\n+\t: >.git/logs/refs/heads/empty &&\n+\n+\ttest_must_fail git log -g empty >actual.out 2>actual.err &&\n+\ttest_cmp expected.out actual.out &&\n+\ttest_cmp expected.err actual.err\n+'\n+\n test_done\n-- \n1.7.0.2\n"},{"id":"136729","messageId":"alpine.DEB.2.00.1003131312540.796@narbuckle.genericorp.net","threadId":"22998","inReplyTo":"4B9BF171.2000102@lsrfire.ath.cx","subject":"Re: Re: [git] Re: \"git stash list\" shows HEAD reflog","fromName":"Dave Olszewski","fromEmail":"cxreg@pobox.com","sentAt":"2010-03-13T21:21:15Z","receivedAt":"2010-03-13T21:21:15Z","isPatch":false,"sender":{"key":"cxreg@pobox.com","avatar":"https://avatars.githubusercontent.com/u/55474?v=4"},"body":"On Sat, 13 Mar 2010, Ren? Scharfe wrote:\n\n> Am 13.03.2010 18:41, schrieb Dave Olszewski:\n>> On Sat, 13 Mar 2010, Ren? Scharfe wrote:\n>>\n>>> Am 12.03.2010 15:52, schrieb Vladimir Panteleev:\n>>\n>>>> That doesn't explain why git displayed the HEAD reflog, though. That\n>>>> seems to happen thanks to the check (revs->def && !revs->pending.nr) in\n>>>> setup_revisions (\"HEAD\" is the default, as specified in the caller\n>>>> cmd_log_init). It looks like (ideally) git shouldn't rely on whether\n>>>> revs->pending is empty to decide whether to use the default, but rather\n>>>> if a ref was specified by the user or not.\n>>>\n>>> We could add some kind of check there, but with the patch applied I can't\n>>> trigger this second issue any more.  It would be nice to have a test\n>>> script\n>>> along with such a sanity check.  Any idea how to cause this error,\n>>> perhaps\n>>> with another type of invalid reflog file?\n>>\n>> I actually noticed this last week.  You can reproduce it by doing \"git\n>> reflog\" on a branch which has been idle for longer than the expiration.\n>> Any 0-byte files in logs/refs/heads would give me this same behavior.\n>>\n>>     Dave\n>\n> Perhaps something like this?\n\nMaybe, although I'm not sure if dying here is the right behavior.  Is an\nempty reflog really an error?  I was testing a patch along the lines of\nwhat Vladimir proposed, which was simply to not set the default rev if a\nvalid user-specified argument was found, whether or not it contains\ncommits.\n\n> ---\n> revision.c             |    4 ++++\n> t/t1411-reflog-show.sh |   13 +++++++++++++\n> 2 files changed, 17 insertions(+), 0 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 29721ec..6991475 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -896,6 +896,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n> \tstruct object *object;\n> \tunsigned char sha1[20];\n> \tint local_flags;\n> +\tint empty_after, empty_before = !revs->pending.nr;\n>\n> \tdotdot = strstr(arg, \"..\");\n> \tif (dotdot) {\n> @@ -971,6 +972,9 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n> \t\tverify_non_filename(revs->prefix, arg);\n> \tobject = get_reference(revs, arg, sha1, flags ^ local_flags);\n> \tadd_pending_object_with_mode(revs, object, arg, mode);\n> +\tempty_after = !revs->pending.nr;\n> +\tif (empty_before && empty_after)\n> +\t\tdie(\"bad revision '%s' (empty reflog?)\", arg);\n> \treturn 0;\n> }\n>\n> diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh\n> index c18ed8e..3f48c2d 100755\n> --- a/t/t1411-reflog-show.sh\n> +++ b/t/t1411-reflog-show.sh\n> @@ -64,4 +64,17 @@ test_expect_success 'using --date= shows reflog date (oneline)' '\n> \ttest_cmp expect actual\n> '\n>\n> +: >expected.out\n> +cat >expected.err <<'EOF'\n> +fatal: bad revision 'empty' (empty reflog?)\n> +EOF\n> +test_expect_success 'empty reflog file' '\n> +\tgit branch empty &&\n> +\t: >.git/logs/refs/heads/empty &&\n> +\n> +\ttest_must_fail git log -g empty >actual.out 2>actual.err &&\n> +\ttest_cmp expected.out actual.out &&\n> +\ttest_cmp expected.err actual.err\n> +'\n> +\n> test_done\n> -- \n> 1.7.0.2\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n>\n>\n"},{"id":"136730","messageId":"4B9C0713.30407@pcharlan.com","threadId":"22998","inReplyTo":"4B9BCD6E.4090902@lsrfire.ath.cx","subject":"Re: \"git stash list\" shows HEAD reflog","fromName":"Pete Harlan","fromEmail":"pgit@pcharlan.com","sentAt":"2010-03-13T21:43:47Z","receivedAt":"2010-03-13T21:43:47Z","isPatch":false,"sender":{"key":"pgit@pcharlan.com","avatar":null},"body":"On 03/13/2010 09:37 AM, René Scharfe wrote:\n> As Vladimir reported, \"git log -g refs/stash\" surprisingly showed the reflog\n> of HEAD if the message in the reflog file was too long.  To fix this, convert\n> for_each_recent_reflog_ent() to use strbuf_getwholeline() instead of fgets(),\n> for safety and to avoid any size limits for reflog entries.\n\nWas the old code actually unsafe?  If not, then perhaps the commit\nmessage would be clearer if \", for safety and\" were removed.\n\n--Pete\n\n> Also reverse the logic of the part of the function that only looks at file\n> tails.  It used to close the file if fgets() succeeded.  The following\n> fgets() call in the while loop was likely to fail in this case, too, so\n> passing an offset to for_each_recent_reflog_ent() never worked.  Change it to\n> error out if strbuf_getwholeline() fails instead.\n> \n> Reported-by: Vladimir Panteleev <vladimir@thecybershadow.net>\n> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n> ---\n>  refs.c |   22 ++++++++++++----------\n>  1 files changed, 12 insertions(+), 10 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index f3fcbe0..63e30d7 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1574,7 +1574,7 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long ofs,\n>  {\n>  \tconst char *logfile;\n>  \tFILE *logfp;\n> -\tchar buf[1024];\n> +\tstruct strbuf sb = STRBUF_INIT;\n>  \tint ret = 0;\n>  \n>  \tlogfile = git_path(\"logs/%s\", ref);\n> @@ -1587,24 +1587,24 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long ofs,\n>  \t\tif (fstat(fileno(logfp), &statbuf) ||\n>  \t\t    statbuf.st_size < ofs ||\n>  \t\t    fseek(logfp, -ofs, SEEK_END) ||\n> -\t\t    fgets(buf, sizeof(buf), logfp)) {\n> +\t\t    strbuf_getwholeline(&sb, logfp, '\\n')) {\n>  \t\t\tfclose(logfp);\n> +\t\t\tstrbuf_release(&sb);\n>  \t\t\treturn -1;\n>  \t\t}\n>  \t}\n>  \n> -\twhile (fgets(buf, sizeof(buf), logfp)) {\n> +\twhile (!strbuf_getwholeline(&sb, logfp, '\\n')) {\n>  \t\tunsigned char osha1[20], nsha1[20];\n>  \t\tchar *email_end, *message;\n>  \t\tunsigned long timestamp;\n> -\t\tint len, tz;\n> +\t\tint tz;\n>  \n>  \t\t/* old SP new SP name <email> SP time TAB msg LF */\n> -\t\tlen = strlen(buf);\n> -\t\tif (len < 83 || buf[len-1] != '\\n' ||\n> -\t\t    get_sha1_hex(buf, osha1) || buf[40] != ' ' ||\n> -\t\t    get_sha1_hex(buf + 41, nsha1) || buf[81] != ' ' ||\n> -\t\t    !(email_end = strchr(buf + 82, '>')) ||\n> +\t\tif (sb.len < 83 || sb.buf[sb.len - 1] != '\\n' ||\n> +\t\t    get_sha1_hex(sb.buf, osha1) || sb.buf[40] != ' ' ||\n> +\t\t    get_sha1_hex(sb.buf + 41, nsha1) || sb.buf[81] != ' ' ||\n> +\t\t    !(email_end = strchr(sb.buf + 82, '>')) ||\n>  \t\t    email_end[1] != ' ' ||\n>  \t\t    !(timestamp = strtoul(email_end + 2, &message, 10)) ||\n>  \t\t    !message || message[0] != ' ' ||\n> @@ -1618,11 +1618,13 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long ofs,\n>  \t\t\tmessage += 6;\n>  \t\telse\n>  \t\t\tmessage += 7;\n> -\t\tret = fn(osha1, nsha1, buf+82, timestamp, tz, message, cb_data);\n> +\t\tret = fn(osha1, nsha1, sb.buf + 82, timestamp, tz, message,\n> +\t\t\t cb_data);\n>  \t\tif (ret)\n>  \t\t\tbreak;\n>  \t}\n>  \tfclose(logfp);\n> +\tstrbuf_release(&sb);\n>  \treturn ret;\n>  }\n>  \n"},{"id":"136731","messageId":"4B9C086D.10004@lsrfire.ath.cx","threadId":"22998","inReplyTo":"alpine.DEB.2.00.1003131312540.796@narbuckle.genericorp.net","subject":"Re: \"git stash list\" shows HEAD reflog","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-03-13T21:49:33Z","receivedAt":"2010-03-13T21:49:33Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.03.2010 22:21, schrieb Dave Olszewski:\n> Maybe, although I'm not sure if dying here is the right behavior.  Is an\n> empty reflog really an error?\n\nYeah, that might be a bit heavy-handed.  *ahem*\n\n> I was testing a patch along the lines of\n> what Vladimir proposed, which was simply to not set the default rev if a\n> valid user-specified argument was found, whether or not it contains\n> commits.\n\nSounds more like it.  How did the tests go?  Does it result in empty\noutput (which is what I would expect from an empty reflog, now that I\nstopped and thought about it for a second)?\n\nRené\n"},{"id":"136737","messageId":"4B9C0CB1.3000308@lsrfire.ath.cx","threadId":"22998","inReplyTo":"4B9C0713.30407@pcharlan.com","subject":"Re: \"git stash list\" shows HEAD reflog","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-03-13T22:07:45Z","receivedAt":"2010-03-13T22:07:45Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.03.2010 22:43, schrieb Pete Harlan:\n> On 03/13/2010 09:37 AM, René Scharfe wrote:\n>> As Vladimir reported, \"git log -g refs/stash\" surprisingly showed the reflog\n>> of HEAD if the message in the reflog file was too long.  To fix this, convert\n>> for_each_recent_reflog_ent() to use strbuf_getwholeline() instead of fgets(),\n>> for safety and to avoid any size limits for reflog entries.\n> \n> Was the old code actually unsafe?  If not, then perhaps the commit\n> message would be clearer if \", for safety and\" were removed.\n\nThe function silently dropped valid (if long) reflog entries on the\nfloor.  It's certainly debatable if not doing so is \"safer\" or merely\n\"more complete\".  The sentence is already long enough in any case, so I\ndon't mind dropping this part.\n\nRené\n"},{"id":"136742","messageId":"1268520425-31889-1-git-send-email-cxreg@pobox.com","threadId":"22998","inReplyTo":"4B9C086D.10004@lsrfire.ath.cx","subject":"[PATCH] don't use default revision if a rev was specified","fromName":"Dave Olszewski","fromEmail":"cxreg@pobox.com","sentAt":"2010-03-13T22:47:05Z","receivedAt":"2010-03-13T22:47:05Z","isPatch":true,"sender":{"key":"cxreg@pobox.com","avatar":"https://avatars.githubusercontent.com/u/55474?v=4"},"body":"If a revision is specified, it happens not to have any commits, don't\nuse the default revision.  By doing so, surprising and undesired\nbehavior can happen, such as showing the reflog for HEAD when a branch\nwas specified.\n\nSigned-off-by: Dave Olszewski <cxreg@pobox.com>\n---\n>> I was testing a patch along the lines of\n>> what Vladimir proposed, which was simply to not set the default rev if a\n>> valid user-specified argument was found, whether or not it contains\n>> commits.\n>\n>Sounds more like it.  How did the tests go?  Does it result in empty\n>output (which is what I would expect from an empty reflog, now that I\n>stopped and thought about it for a second)?\n\nIt seems to work ok(tm)\n\n revision.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 29721ec..490b484 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1334,7 +1334,7 @@ static void append_prune_data(const char ***prune_data, const char **av)\n  */\n int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def)\n {\n-\tint i, flags, left, seen_dashdash, read_from_stdin;\n+\tint i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0;\n \tconst char **prune_data = NULL;\n \n \t/* First, search for \"--\" */\n@@ -1460,6 +1460,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n \t\t\tappend_prune_data(&prune_data, argv + i);\n \t\t\tbreak;\n \t\t}\n+\t\telse\n+\t\t\tgot_rev_arg = 1;\n \t}\n \n \tif (prune_data)\n@@ -1469,7 +1471,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n \t\trevs->def = def;\n \tif (revs->show_merge)\n \t\tprepare_show_merge(revs);\n-\tif (revs->def && !revs->pending.nr) {\n+\tif (revs->def && !revs->pending.nr && !got_rev_arg) {\n \t\tunsigned char sha1[20];\n \t\tstruct object *object;\n \t\tunsigned mode;\n-- \n1.7.0.2.202.g4e870.dirty\n"},{"id":"136744","messageId":"4B9C1D77.3080007@lsrfire.ath.cx","threadId":"22998","inReplyTo":"1268520425-31889-1-git-send-email-cxreg@pobox.com","subject":"Re: [PATCH] don't use default revision if a rev was specified","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-03-13T23:19:19Z","receivedAt":"2010-03-13T23:19:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.03.2010 23:47, schrieb Dave Olszewski:\n> If a revision is specified, it happens not to have any commits, don't\n> use the default revision.  By doing so, surprising and undesired\n> behavior can happen, such as showing the reflog for HEAD when a branch\n> was specified.\n> \n> Signed-off-by: Dave Olszewski <cxreg@pobox.com>\n> ---\n>>> I was testing a patch along the lines of\n>>> what Vladimir proposed, which was simply to not set the default rev if a\n>>> valid user-specified argument was found, whether or not it contains\n>>> commits.\n>>\n>> Sounds more like it.  How did the tests go?  Does it result in empty\n>> output (which is what I would expect from an empty reflog, now that I\n>> stopped and thought about it for a second)?\n> \n> It seems to work ok(tm)\n> \n>  revision.c |    6 ++++--\n>  1 files changed, 4 insertions(+), 2 deletions(-)\n\nThanks.  And here's an updated, squash-able test.\n\ndiff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh\nindex c18ed8e..ba25ff3 100755\n--- a/t/t1411-reflog-show.sh\n+++ b/t/t1411-reflog-show.sh\n@@ -64,4 +64,13 @@ test_expect_success 'using --date= shows reflog date (oneline)' '\n \ttest_cmp expect actual\n '\n \n+: >expect\n+test_expect_success 'empty reflog file' '\n+\tgit branch empty &&\n+\t: >.git/logs/refs/heads/empty &&\n+\n+\tgit log -g empty >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"136751","messageId":"7vzl2bqtkw.fsf@alter.siamese.dyndns.org","threadId":"22998","inReplyTo":"1268520425-31889-1-git-send-email-cxreg@pobox.com","subject":"Re: [PATCH] don't use default revision if a rev was specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-14T06:57:03Z","receivedAt":"2010-03-14T06:57:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Olszewski <cxreg@pobox.com> writes:\n\n> If a revision is specified, it happens not to have any commits, don't\n> use the default revision.  By doing so, surprising and undesired\n> behavior can happen, such as showing the reflog for HEAD when a branch\n> was specified.\n>\n> Signed-off-by: Dave Olszewski <cxreg@pobox.com>\n\nThanks, will queue.\n"}]}