{"thread":{"id":"39324","subject":"[PATCH 0/2] teach git pull to handle --log=","startedAt":"2015-05-13T09:17:39Z","lastAt":"2015-05-15T11:15:19Z","messageCount":9,"participants":["Paul Tan","Matthieu Moy","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"261156","messageId":"1431508661-21729-1-git-send-email-pyokagan@gmail.com","threadId":"39324","inReplyTo":null,"subject":"[PATCH 0/2] teach git pull to handle --log=","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-13T09:17:39Z","receivedAt":"2015-05-13T09:17:39Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since efb779f (merge, pull: add '--(no-)log' command line option,\n2008-04-06) git-pull supported the (--no-)log switch and would pass it\nto git-merge.\n\n96e9420 (merge: Make '--log' an integer option for number of shortlog\nentries, 2010-09-08) implemented support for the --log= switch, which\nwould explicitly set the number of shortlog entries. However, git-pull\ndoes not recognize this option, and will instead pass it to git-fetch,\nleading to \"unknown option\" errors.\n\nThis patch series implements a failing test that demonstrates the above,\nand teaches git-pull to handle the switch --log=.\n\nPaul Tan (2):\n t5524: test --log=1 limits shortlog length\n pull: handle --log=\n\n git-pull.sh | 4 ++--\n t/t5524-pull-msg.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\n-- \n2.1.4\n"},{"id":"261157","messageId":"1431508661-21729-2-git-send-email-pyokagan@gmail.com","threadId":"39324","inReplyTo":"1431508661-21729-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 1/2] t5524: test --log=1 limits shortlog length","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-13T09:17:40Z","receivedAt":"2015-05-13T09:17:40Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since efb779f (merge, pull: add '--(no-)log' command line option,\n2008-04-06) git-pull supported the (--no-)log switch and would pass it\nto git-merge.\n\n96e9420 (merge: Make '--log' an integer option for number of shortlog\nentries, 2010-09-08) implemented support for the --log= switch, which\nwould explicitly set the number of shortlog entries. However, git-pull\ndoes not recognize this option, and will instead pass it to git-fetch,\nleading to \"unknown option\" errors.\n\nImplement a failing test that demonstrates this.\n\nSigned-off-by: Paul Tan \n---\n t/t5524-pull-msg.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh\nindex 8cccecc..593405d 100755\n--- a/t/t5524-pull-msg.sh\n+++ b/t/t5524-pull-msg.sh\n@@ -17,6 +17,9 @@ test_expect_success setup '\n \t\tgit commit -m \"add bfile\"\n \t) &&\n \ttest_tick && test_tick &&\n+\techo \"second\" >afile &&\n+\tgit add afile &&\n+\tgit commit -m \"second commit\" &&\n \techo \"original $dollar\" >afile &&\n \tgit add afile &&\n \tgit commit -m \"do not clobber $dollar signs\"\n@@ -32,4 +35,18 @@ test_expect_success pull '\n )\n '\n \n+test_expect_failure '--log=1 limits shortlog length' '\n+(\n+\tcd cloned &&\n+\tgit reset --hard HEAD^ &&\n+\tverbose test \"$(cat afile)\" = original &&\n+\tverbose test \"$(cat bfile)\" = added &&\n+\tgit pull --log=1 &&\n+\tgit log -3 &&\n+\tgit cat-file commit HEAD >result &&\n+\tverbose grep Dollar result &&\n+\tverbose grep -v \"second commit\" result\n+)\n+'\n+\n test_done\n-- \n2.1.4\n"},{"id":"261158","messageId":"1431508661-21729-3-git-send-email-pyokagan@gmail.com","threadId":"39324","inReplyTo":"1431508661-21729-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 2/2] pull: handle --log=","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-13T09:17:41Z","receivedAt":"2015-05-13T09:17:41Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since efb779f (merge, pull: add '--(no-)log' command line option,\n2008-04-06) git-pull supported the (--no-)log switch and would pass it\nto git-merge.\n\n96e9420 (merge: Make '--log' an integer option for number of shortlog\nentries, 2010-09-08) implemented support for the --log= switch, which\nwould explicitly set the number of shortlog entries. However, git-pull\ndoes not recognize this option, and will instead pass it to git-fetch,\nleading to \"unknown option\" errors.\n\nFix this by matching --log=* in addition to --log and --no-log.\n\nThis fixes the failing test '--log=1 limits shortlog length'.\n\nSigned-off-by: Paul Tan \n---\n git-pull.sh | 4 ++--\n t/t5524-pull-msg.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 9ed01fd..5ff4545 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -81,8 +81,8 @@ do\n \t\tdiffstat=--no-stat ;;\n \t--stat|--summary)\n \t\tdiffstat=--stat ;;\n-\t--log|--no-log)\n-\t\tlog_arg=$1 ;;\n+\t--log|--log=*|--no-log)\n+\t\tlog_arg=\"$1\" ;;\n \t--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)\n \t\tno_commit=--no-commit ;;\n \t--c|--co|--com|--comm|--commi|--commit)\ndiff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh\nindex 593405d..64754b1 100755\n--- a/t/t5524-pull-msg.sh\n+++ b/t/t5524-pull-msg.sh\n@@ -35,7 +35,7 @@ test_expect_success pull '\n )\n '\n \n-test_expect_failure '--log=1 limits shortlog length' '\n+test_expect_success '--log=1 limits shortlog length' '\n (\n \tcd cloned &&\n \tgit reset --hard HEAD^ &&\n-- \n2.1.4\n"},{"id":"261161","messageId":"vpqlhgszujc.fsf@anie.imag.fr","threadId":"39324","inReplyTo":"1431508661-21729-3-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 2/2] pull: handle --log=","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-05-13T09:38:47Z","receivedAt":"2015-05-13T09:38:47Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Paul Tan writes:\n\n> -\t--log|--no-log)\n> -\t\tlog_arg=$1 ;;\n> +\t--log|--log=*|--no-log)\n> +\t\tlog_arg=\"$1\" ;;\n\nI think you actually don't need the double quotes here (var=$value works\neven if $value has spaces IIRC), but they don't harm and I prefer having\nthem.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"261163","messageId":"vpqd224zufs.fsf@anie.imag.fr","threadId":"39324","inReplyTo":"1431508661-21729-1-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 0/2] teach git pull to handle --log=","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-05-13T09:40:55Z","receivedAt":"2015-05-13T09:40:55Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Paul Tan writes:\n\n> Paul Tan (2):\n> t5524: test --log=1 limits shortlog length\n> pull: handle --log=\n>\n> git-pull.sh | 4 ++--\n> t/t5524-pull-msg.sh | 17 +++++++++++++++++\n> 2 files changed, 19 insertions(+), 2 deletions(-)\n\nI thought you forgot to update the documentation, but by the magic of\nascidoc includes, the feature was already documented even though it was\nnot implemented ;-).\n\nReviewed-by: Matthieu Moy \n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"261174","messageId":"c8dc145fcc56d38e04fc0e07ceb34999@www.dscho.org","threadId":"39324","inReplyTo":"vpqlhgszujc.fsf@anie.imag.fr","subject":"Re: [PATCH 2/2] pull: handle --log=","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-05-13T12:23:21Z","receivedAt":"2015-05-13T12:23:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Matthieu,\n\nOn 2015-05-13 11:38, Matthieu Moy wrote:\n> Paul Tan writes:\n> \n>> -\t--log|--no-log)\n>> -\t\tlog_arg=$1 ;;\n>> +\t--log|--log=*|--no-log)\n>> +\t\tlog_arg=\"$1\" ;;\n> \n> I think you actually don't need the double quotes here (var=$value works\n> even if $value has spaces IIRC), but they don't harm and I prefer having\n> them.\n\nI am far from a shell expert, but IIRC \"$1\" converts all whitespace to single spaces. In general, you therefore want to quote arguments, just in case.\n\nCiao,\nDscho\n"},{"id":"261200","messageId":"vpq3830kzxx.fsf@anie.imag.fr","threadId":"39324","inReplyTo":"c8dc145fcc56d38e04fc0e07ceb34999@www.dscho.org","subject":"Re: [PATCH 2/2] pull: handle --log=","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-05-13T20:03:22Z","receivedAt":"2015-05-13T20:03:22Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Johannes Schindelin writes:\n\n> Hi Matthieu,\n>\n> On 2015-05-13 11:38, Matthieu Moy wrote:\n>> Paul Tan writes:\n>> \n>>> -\t--log|--no-log)\n>>> -\t\tlog_arg=$1 ;;\n>>> +\t--log|--log=*|--no-log)\n>>> +\t\tlog_arg=\"$1\" ;;\n>> \n>> I think you actually don't need the double quotes here (var=$value works\n>> even if $value has spaces IIRC), but they don't harm and I prefer having\n>> them.\n>\n> I am far from a shell expert, but IIRC \"$1\" converts all whitespace to\n> single spaces.\n\nIn most places, $1 is split before being interpreted, but there are\nexceptions and actually the RHS of assignment is one of them. Just for\ncuriosity, I digged a reference:\n\nhttps://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Shell-Substitutions.html\nhttp://unix.stackexchange.com/questions/68694/when-is-double-quoting-necessary\n\n> In general, you therefore want to quote arguments, just in case.\n\nYes, and one benefit of quoting anyway is to avoid having to have the\ndiscussion we're having ;-).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"261243","messageId":"xmqqd22384om.fsf@gitster.dls.corp.google.com","threadId":"39324","inReplyTo":"1431508661-21729-1-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 0/2] teach git pull to handle --log=","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-14T17:11:37Z","receivedAt":"2015-05-14T17:11:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> Since efb779f (merge, pull: add '--(no-)log' command line option,\n> 2008-04-06) git-pull supported the (--no-)log switch and would pass it\n> to git-merge.\n>\n> 96e9420 (merge: Make '--log' an integer option for number of shortlog\n> entries, 2010-09-08) implemented support for the --log= switch, which\n> would explicitly set the number of shortlog entries. However, git-pull\n> does not recognize this option, and will instead pass it to git-fetch,\n> leading to \"unknown option\" errors.\n>\n> This patch series implements a failing test that demonstrates the above,\n> and teaches git-pull to handle the switch --log=.\n\nLooks good.\n\nOne advice; for a small patch like this one (and the \"pull.ff vs\nmerge.ff\" one, too), it is not necessary or even desirable to do a\ntwo-step \"first add a failure test and then another patch to fix and\nflip the expectation\" series. Just do the fix and add a test to\nexpect success.\n\nAfter all, the primary reason why we add test is *not* for you to\ndemonstrate that what you did works as expected. It is to catch\nother people breaking what you did in the future.\n\nThanks.\n"},{"id":"261333","messageId":"CACRoPnQ6cH2w=4DRcq+aPGz2ahRaU3Lnabgi5EbAC8BVRPHgzg@mail.gmail.com","threadId":"39324","inReplyTo":"xmqqd22384om.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/2] teach git pull to handle --log=","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-15T11:15:19Z","receivedAt":"2015-05-15T11:15:19Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Fri, May 15, 2015 at 1:11 AM, Junio C Hamano wrote:\n> One advice; for a small patch like this one (and the \"pull.ff vs\n> merge.ff\" one, too), it is not necessary or even desirable to do a\n> two-step \"first add a failure test and then another patch to fix and\n> flip the expectation\" series. Just do the fix and add a test to\n> expect success.\n>\n> After all, the primary reason why we add test is *not* for you to\n> demonstrate that what you did works as expected. It is to catch\n> other people breaking what you did in the future.\n\nSince I will be re-rolling all the patches to remove the use of\n\"verbose\" I guess I will squash the patches as well.\n\nThanks.\n"}]}