{"thread":{"id":"38724","subject":"[PATCH] [GSoC][MICRO] Forbid \"log --graph --no-walk\"","startedAt":"2015-03-06T08:55:10Z","lastAt":"2015-03-18T05:34:01Z","messageCount":13,"participants":["Dongcan Jiang","Eric Sunshine","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"257137","messageId":"1425632110-31863-1-git-send-email-dongcan.jiang@gmail.com","threadId":"38724","inReplyTo":null,"subject":"[PATCH] [GSoC][MICRO] Forbid \"log --graph --no-walk\"","fromName":"Dongcan Jiang","fromEmail":"dongcan.jiang@gmail.com","sentAt":"2015-03-06T08:55:10Z","receivedAt":"2015-03-06T08:55:10Z","isPatch":true,"sender":{"key":"dongcan.jiang@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10103762?v=4"},"body":"Because --graph is about connected history while --no-walk is about discrete points.\n\nrevision.c: Judge whether --graph and --no-walk come together when running git-log.\nbuildin/log.c: Set git-log cmd flag.\nDocumentation/rev-list-options.txt: Add specification on the forbidden usage.\n\nSigned-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>\n---\n Documentation/rev-list-options.txt | 2 ++\n builtin/log.c                      | 1 +\n revision.c                         | 4 ++++\n revision.h                         | 3 +++\n 4 files changed, 10 insertions(+)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..eea2c0a 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -679,6 +679,7 @@ endif::git-rev-list[]\n \tgiven on the command line. Otherwise (if `sorted` or no argument\n \twas given), the commits are shown in reverse chronological order\n \tby commit time.\n+\tCannot be combined with `--graph` when running git-log.\n \n --do-walk::\n \tOverrides a previous `--no-walk`.\n@@ -781,6 +782,7 @@ you would get an output like this:\n \ton the left hand side of the output.  This may cause extra lines\n \tto be printed in between commits, in order for the graph history\n \tto be drawn properly.\n+\tCannot be combined with `--no-walk` when running git-log.\n +\n This enables parent rewriting, see 'History Simplification' below.\n +\ndiff --git a/builtin/log.c b/builtin/log.c\nindex dd8f3fc..7bf5adb 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)\n \tgit_config(git_log_config, NULL);\n \n \tinit_revisions(&rev, prefix);\n+\trev.cmd_is_log = 1;\n \trev.always_show_header = 1;\n \tmemset(&opt, 0, sizeof(opt));\n \topt.def = \"HEAD\";\ndiff --git a/revision.c b/revision.c\nindex 66520c6..5f62c89 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n \n \trevs->commit_format = CMIT_FMT_DEFAULT;\n \n+\trevs->cmd_is_log = 0;\n+\n \tinit_grep_defaults();\n \tgrep_init(&revs->grep_filter, prefix);\n \trevs->grep_filter.status_only = 1;\n@@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \tif (revs->reflog_info && revs->graph)\n \t\tdie(\"cannot combine --walk-reflogs with --graph\");\n+\tif (revs->no_walk && revs->graph && revs->cmd_is_log)\n+\t\tdie(\"cannot combine --no-walk with --graph when running git-log\");\n \tif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n \t\tdie(\"cannot use --grep-reflog without --walk-reflogs\");\n \ndiff --git a/revision.h b/revision.h\nindex 0ea8b4e..255982a 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -146,6 +146,9 @@ struct rev_info {\n \t\t\ttrack_first_time:1,\n \t\t\tlinear:1;\n \n+\t/* cmd type */\n+\tunsigned int  cmd_is_log:1;\n+\n \tenum date_mode date_mode;\n \n \tunsigned int\tabbrev;\n-- \n2.3.1.251.g83036f8\n"},{"id":"257143","messageId":"CAPig+cQDAurk2WeTNZPVTnQ8GJSQOpWyDxHNCK_ww25ZdLtFwQ@mail.gmail.com","threadId":"38724","inReplyTo":"1425632110-31863-1-git-send-email-dongcan.jiang@gmail.com","subject":"Re: [PATCH] [GSoC][MICRO] Forbid \"log --graph --no-walk\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-06T09:56:53Z","receivedAt":"2015-03-06T09:56:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 6, 2015 at 3:55 AM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:\n> Forbid \"log --graph --no-walk\n\nStyle: drop capitalization in the Subject: line. Also prefix with the\ncommand or module being modified, followed by a colon. So:\n\n    log: forbid combining --graph and --no-walk\n\nor:\n\n    revision: forbid combining --graph and --no-walk\n\n> Because --graph is about connected history while --no-walk is about discrete points.\n\nOkay. You might also want to cite the wider discussion[1].\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/216083\n\n> revision.c: Judge whether --graph and --no-walk come together when running git-log.\n> buildin/log.c: Set git-log cmd flag.\n> Documentation/rev-list-options.txt: Add specification on the forbidden usage.\n\nNo need to repeat in prose what the patch itself states more clearly\nand concisely.\n\nAlso, such a change should be accompanied by new test(s).\n\n> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>\n> ---\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index 4ed8587..eea2c0a 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -679,6 +679,7 @@ endif::git-rev-list[]\n>         given on the command line. Otherwise (if `sorted` or no argument\n>         was given), the commits are shown in reverse chronological order\n>         by commit time.\n> +       Cannot be combined with `--graph` when running git-log.\n>\n>  --do-walk::\n>         Overrides a previous `--no-walk`.\n> @@ -781,6 +782,7 @@ you would get an output like this:\n>         on the left hand side of the output.  This may cause extra lines\n>         to be printed in between commits, in order for the graph history\n>         to be drawn properly.\n> +       Cannot be combined with `--no-walk` when running git-log.\n\nNice to see documentation updates. More below.\n\n>  This enables parent rewriting, see 'History Simplification' below.\n>  +\n> diff --git a/builtin/log.c b/builtin/log.c\n> index dd8f3fc..7bf5adb 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)\n>         git_config(git_log_config, NULL);\n>\n>         init_revisions(&rev, prefix);\n> +       rev.cmd_is_log = 1;\n>         rev.always_show_header = 1;\n>         memset(&opt, 0, sizeof(opt));\n>         opt.def = \"HEAD\";\n> diff --git a/revision.c b/revision.c\n> index 66520c6..5f62c89 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n>\n>         revs->commit_format = CMIT_FMT_DEFAULT;\n>\n> +       revs->cmd_is_log = 0;\n> +\n>         init_grep_defaults();\n>         grep_init(&revs->grep_filter, prefix);\n>         revs->grep_filter.status_only = 1;\n> @@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>\n>         if (revs->reflog_info && revs->graph)\n>                 die(\"cannot combine --walk-reflogs with --graph\");\n> +       if (revs->no_walk && revs->graph && revs->cmd_is_log)\n\nPlacing 'revs->cmd_is_log' first would make it clear at a glance that\nthis restriction impacts 'log' only (but see question below):\n\n    if (revs->cmd_is_log && revs->no_walk && revs->graph)\n\n> +               die(\"cannot combine --no-walk with --graph when running git-log\");\n>         if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n>                 die(\"cannot use --grep-reflog without --walk-reflogs\");\n>\n> diff --git a/revision.h b/revision.h\n> index 0ea8b4e..255982a 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -146,6 +146,9 @@ struct rev_info {\n>                         track_first_time:1,\n>                         linear:1;\n>\n> +       /* cmd type */\n> +       unsigned int  cmd_is_log:1;\n\nGenuine question: Despite the GSoC micro-project mentioning only\n'log', is it ever meaningful for these two options to be specified\ntogether? I suspect not, but it would be nice to hear from someone\nmore familiar with the issue. If not specific to 'log', then the patch\ncan be simplified a good deal.\n\n>         enum date_mode date_mode;\n>\n>         unsigned int    abbrev;\n> --\n"},{"id":"257145","messageId":"54F97C44.8070204@web.de","threadId":"38724","inReplyTo":"1425632110-31863-1-git-send-email-dongcan.jiang@gmail.com","subject":"Re: [PATCH] [GSoC][MICRO] Forbid \"log --graph --no-walk\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-03-06T10:07:00Z","receivedAt":"2015-03-06T10:07:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.03.2015 um 09:55 schrieb Dongcan Jiang:\n> Because --graph is about connected history while --no-walk is about discrete points.\n>\n> revision.c: Judge whether --graph and --no-walk come together when running git-log.\n> buildin/log.c: Set git-log cmd flag.\n> Documentation/rev-list-options.txt: Add specification on the forbidden usage.\n>\n> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>\n> ---\n>   Documentation/rev-list-options.txt | 2 ++\n>   builtin/log.c                      | 1 +\n>   revision.c                         | 4 ++++\n>   revision.h                         | 3 +++\n>   4 files changed, 10 insertions(+)\n>\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index 4ed8587..eea2c0a 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -679,6 +679,7 @@ endif::git-rev-list[]\n>   \tgiven on the command line. Otherwise (if `sorted` or no argument\n>   \twas given), the commits are shown in reverse chronological order\n>   \tby commit time.\n> +\tCannot be combined with `--graph` when running git-log.\n>\n>   --do-walk::\n>   \tOverrides a previous `--no-walk`.\n> @@ -781,6 +782,7 @@ you would get an output like this:\n>   \ton the left hand side of the output.  This may cause extra lines\n>   \tto be printed in between commits, in order for the graph history\n>   \tto be drawn properly.\n> +\tCannot be combined with `--no-walk` when running git-log.\n>   +\n>   This enables parent rewriting, see 'History Simplification' below.\n>   +\n> diff --git a/builtin/log.c b/builtin/log.c\n> index dd8f3fc..7bf5adb 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)\n>   \tgit_config(git_log_config, NULL);\n>\n>   \tinit_revisions(&rev, prefix);\n> +\trev.cmd_is_log = 1;\n>   \trev.always_show_header = 1;\n>   \tmemset(&opt, 0, sizeof(opt));\n>   \topt.def = \"HEAD\";\n> diff --git a/revision.c b/revision.c\n> index 66520c6..5f62c89 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n>\n>   \trevs->commit_format = CMIT_FMT_DEFAULT;\n>\n> +\trevs->cmd_is_log = 0;\n> +\n>   \tinit_grep_defaults();\n>   \tgrep_init(&revs->grep_filter, prefix);\n>   \trevs->grep_filter.status_only = 1;\n> @@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>\n>   \tif (revs->reflog_info && revs->graph)\n>   \t\tdie(\"cannot combine --walk-reflogs with --graph\");\n> +\tif (revs->no_walk && revs->graph && revs->cmd_is_log)\n> +\t\tdie(\"cannot combine --no-walk with --graph when running git-log\");\n\nWhy only for git log?  Doesn't the justification given in the commit \nmessage above apply in general?\n\n>   \tif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n>   \t\tdie(\"cannot use --grep-reflog without --walk-reflogs\");\n>\n> diff --git a/revision.h b/revision.h\n> index 0ea8b4e..255982a 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -146,6 +146,9 @@ struct rev_info {\n>   \t\t\ttrack_first_time:1,\n>   \t\t\tlinear:1;\n>\n> +\t/* cmd type */\n> +\tunsigned int  cmd_is_log:1;\n> +\n>   \tenum date_mode date_mode;\n>\n>   \tunsigned int\tabbrev;\n>\n"},{"id":"257158","messageId":"CABwkPcoBG61YY0N4b33gdMq2cC1oQ_wFV7KaGGu3wYfuY9inGw@mail.gmail.com","threadId":"38724","inReplyTo":"CAPig+cQDAurk2WeTNZPVTnQ8GJSQOpWyDxHNCK_ww25ZdLtFwQ@mail.gmail.com","subject":"Re: [PATCH] [GSoC][MICRO] Forbid \"log --graph --no-walk\"","fromName":"Dongcan Jiang","fromEmail":"dongcan.jiang@gmail.com","sentAt":"2015-03-06T13:13:26Z","receivedAt":"2015-03-06T13:13:26Z","isPatch":true,"sender":{"key":"dongcan.jiang@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10103762?v=4"},"body":"Hi, Eric and René\n\nThanks for your suggestions. Good ideas!\n\n> Genuine question: Despite the GSoC micro-project mentioning only\n> 'log', is it ever meaningful for these two options to be specified\n> together? I suspect not, but it would be nice to hear from someone\n> more familiar with the issue. If not specific to 'log', then the > patch\n> can be simplified a good deal.\n\n> Why only for git log?  Doesn't the justification given in the\n> commit message above apply in general?\n\nAt first, I also tried to only judge the value of \"revs->no_walk &&\nrevs->graph\", but unfortunately, it failed to pass all cases in\nt4052-stat-output.sh.\ne.g. command \"git show --stat --graph\" failed to get the correct result.\n\nFinally, this is because that \"revs->no_walk\" gets set when it comes\nto \"git show --stat\". That's why I add the parameter\n\"revs->cmd_is_log\" as judgement. Of course, it causes the limitation\nyou've mentioned. I will consider better solution in the next patch\nedition as soon as possible.\n\nBest Regards\n\nDongcan\n\n2015-03-06 17:56 GMT+08:00 Eric Sunshine <sunshine@sunshineco.com>:\n> On Fri, Mar 6, 2015 at 3:55 AM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:\n>> Forbid \"log --graph --no-walk\n>\n> Style: drop capitalization in the Subject: line. Also prefix with the\n> command or module being modified, followed by a colon. So:\n>\n>     log: forbid combining --graph and --no-walk\n>\n> or:\n>\n>     revision: forbid combining --graph and --no-walk\n>\n>> Because --graph is about connected history while --no-walk is about discrete points.\n>\n> Okay. You might also want to cite the wider discussion[1].\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/216083\n>\n>> revision.c: Judge whether --graph and --no-walk come together when running git-log.\n>> buildin/log.c: Set git-log cmd flag.\n>> Documentation/rev-list-options.txt: Add specification on the forbidden usage.\n>\n> No need to repeat in prose what the patch itself states more clearly\n> and concisely.\n>\n> Also, such a change should be accompanied by new test(s).\n>\n>> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>\n>> ---\n>> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n>> index 4ed8587..eea2c0a 100644\n>> --- a/Documentation/rev-list-options.txt\n>> +++ b/Documentation/rev-list-options.txt\n>> @@ -679,6 +679,7 @@ endif::git-rev-list[]\n>>         given on the command line. Otherwise (if `sorted` or no argument\n>>         was given), the commits are shown in reverse chronological order\n>>         by commit time.\n>> +       Cannot be combined with `--graph` when running git-log.\n>>\n>>  --do-walk::\n>>         Overrides a previous `--no-walk`.\n>> @@ -781,6 +782,7 @@ you would get an output like this:\n>>         on the left hand side of the output.  This may cause extra lines\n>>         to be printed in between commits, in order for the graph history\n>>         to be drawn properly.\n>> +       Cannot be combined with `--no-walk` when running git-log.\n>\n> Nice to see documentation updates. More below.\n>\n>>  This enables parent rewriting, see 'History Simplification' below.\n>>  +\n>> diff --git a/builtin/log.c b/builtin/log.c\n>> index dd8f3fc..7bf5adb 100644\n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n>> @@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)\n>>         git_config(git_log_config, NULL);\n>>\n>>         init_revisions(&rev, prefix);\n>> +       rev.cmd_is_log = 1;\n>>         rev.always_show_header = 1;\n>>         memset(&opt, 0, sizeof(opt));\n>>         opt.def = \"HEAD\";\n>> diff --git a/revision.c b/revision.c\n>> index 66520c6..5f62c89 100644\n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n>>\n>>         revs->commit_format = CMIT_FMT_DEFAULT;\n>>\n>> +       revs->cmd_is_log = 0;\n>> +\n>>         init_grep_defaults();\n>>         grep_init(&revs->grep_filter, prefix);\n>>         revs->grep_filter.status_only = 1;\n>> @@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>>\n>>         if (revs->reflog_info && revs->graph)\n>>                 die(\"cannot combine --walk-reflogs with --graph\");\n>> +       if (revs->no_walk && revs->graph && revs->cmd_is_log)\n>\n> Placing 'revs->cmd_is_log' first would make it clear at a glance that\n> this restriction impacts 'log' only (but see question below):\n>\n>     if (revs->cmd_is_log && revs->no_walk && revs->graph)\n>\n>> +               die(\"cannot combine --no-walk with --graph when running git-log\");\n>>         if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n>>                 die(\"cannot use --grep-reflog without --walk-reflogs\");\n>>\n>> diff --git a/revision.h b/revision.h\n>> index 0ea8b4e..255982a 100644\n>> --- a/revision.h\n>> +++ b/revision.h\n>> @@ -146,6 +146,9 @@ struct rev_info {\n>>                         track_first_time:1,\n>>                         linear:1;\n>>\n>> +       /* cmd type */\n>> +       unsigned int  cmd_is_log:1;\n>\n> Genuine question: Despite the GSoC micro-project mentioning only\n> 'log', is it ever meaningful for these two options to be specified\n> together? I suspect not, but it would be nice to hear from someone\n> more familiar with the issue. If not specific to 'log', then the patch\n> can be simplified a good deal.\n>\n>>         enum date_mode date_mode;\n>>\n>>         unsigned int    abbrev;\n>> --\n\n\n\n-- \n江东灿（Dongcan Jiang）\nTeam of Search Engine & Web Mining\nSchool of Electronic Engineering & Computer Science\nPeking University, Beijing, 100871, P.R.China\n"},{"id":"257185","messageId":"xmqqioeeyn6q.fsf@gitster.dls.corp.google.com","threadId":"38724","inReplyTo":"CABwkPcoBG61YY0N4b33gdMq2cC1oQ_wFV7KaGGu3wYfuY9inGw@mail.gmail.com","subject":"Re: [PATCH] [GSoC][MICRO] Forbid \"log --graph --no-walk\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-06T18:51:09Z","receivedAt":"2015-03-06T18:51:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dongcan Jiang <dongcan.jiang@gmail.com> writes:\n\n> At first, I also tried to only judge the value of \"revs->no_walk &&\n> revs->graph\", but unfortunately, it failed to pass all cases in\n> t4052-stat-output.sh.\n> e.g. command \"git show --stat --graph\" failed to get the correct result.\n>\n> Finally, this is because that \"revs->no_walk\" gets set when it comes\n> to \"git show --stat\".\n\nWhen \"git show\" is given a range, it turns no-walk off and becomes a\ncommand about a connected history.  Otherwise, it is a command about\ndiscrete point(s).\n\nBecause \"git show --graph A B C\" and \"git log --graph --no-walk A B\nC\" are moral equivalents, if we are forbidding the latter, we should\nforbid the former.\n\n\"git show A\" (and no other revs, just a single point), however,\ncould be thought of as an equivalent for \"git log -1 A\", even though\nthat interpretation is based on a wrong world view (because \"show\none and stop\" is not the reason why the result \"git show A\" gives\nhas only A and nothing else).  And it makes sort of sense to allow\n\"git show --graph A\" that is an equivalent to \"git log -1 --graph A\"\nunder that interpretation.\n\nI was tempted to say the existing test is wrong to expect that\n\"--graph\" does anything meaningful for \"git show\" that is run for a\nnon-range.  But doing the \"use of both is wrong for log\" change\nwithout special casing \"show\" (and instead \"fixing\" t4052) would be\na change in behaviour, which we try to avoid in general.\n\nI'd prefer a solution that does not waste a new bit in revision\nstructure in general, but if we were to waste one bit to special\ncase \"show\", it shouldn't be \"fail only for the log command\".  It\nshould be \"allow this stupidity only for the show command for\nbackward compatibility\" bit instead, I think.\n"},{"id":"257226","messageId":"3de5837561c07fbf8d6187578fc37b3dbf2ea5f7.1425702676.git.dongcan.jiang@gmail.com","threadId":"38724","inReplyTo":"1425632110-31863-1-git-send-email-dongcan.jiang@gmail.com","subject":"[PATCH v2/GSoC/MICRO] revision: forbid combining --graph and --no-walk","fromName":"Dongcan Jiang","fromEmail":"dongcan.jiang@gmail.com","sentAt":"2015-03-07T04:56:47Z","receivedAt":"2015-03-07T04:56:47Z","isPatch":true,"sender":{"key":"dongcan.jiang@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10103762?v=4"},"body":"Because --graph is about connected history while\n--no-walk is about discrete points. [1]\n\nIt's a pity that git-show has to allow such combination\nin order to make t4052-stat-output.sh compatible. [2]\n\n2 testcases have been added to test this patch.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/216083\n[2]: http://article.gmane.org/gmane.comp.version-control.git/264950\n\nSigned-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>\n\nThanks-to: Eric Sunshine, René Scharfe, Junio C Hamano\n---\n Documentation/rev-list-options.txt | 2 ++\n builtin/log.c                      | 1 +\n revision.c                         | 4 ++++\n revision.h                         | 3 +++\n t/t4202-log.sh                     | 6 ++++++\n t/t6014-rev-list-all.sh            | 6 ++++++\n 6 files changed, 22 insertions(+)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..136abdf 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -679,6 +679,7 @@ endif::git-rev-list[]\n \tgiven on the command line. Otherwise (if `sorted` or no argument\n \twas given), the commits are shown in reverse chronological order\n \tby commit time.\n+\tCannot be combined with `--graph`.\n\n --do-walk::\n \tOverrides a previous `--no-walk`.\n@@ -781,6 +782,7 @@ you would get an output like this:\n \ton the left hand side of the output.  This may cause extra lines\n \tto be printed in between commits, in order for the graph history\n \tto be drawn properly.\n+\tCannot be combined with `--no-walk`.\n +\n This enables parent rewriting, see 'History Simplification' below.\n +\ndiff --git a/builtin/log.c b/builtin/log.c\nindex dd8f3fc..5b5d028 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -524,6 +524,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n\n \tmemset(&match_all, 0, sizeof(match_all));\n \tinit_revisions(&rev, prefix);\n+\trev.cmd_is_show = 1;\n \trev.diff = 1;\n \trev.always_show_header = 1;\n \trev.no_walk = REVISION_WALK_NO_WALK_SORTED;\ndiff --git a/revision.c b/revision.c\nindex 66520c6..5d6fbef 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n\n \trevs->commit_format = CMIT_FMT_DEFAULT;\n\n+\trevs->cmd_is_show = 0;\n+\n \tinit_grep_defaults();\n \tgrep_init(&revs->grep_filter, prefix);\n \trevs->grep_filter.status_only = 1;\n@@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n\n \tif (revs->reflog_info && revs->graph)\n \t\tdie(\"cannot combine --walk-reflogs with --graph\");\n+\tif (!revs->cmd_is_show && revs->no_walk && revs->graph)\n+\t\tdie(\"cannot combine --no-walk with --graph\");\n \tif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n \t\tdie(\"cannot use --grep-reflog without --walk-reflogs\");\n\ndiff --git a/revision.h b/revision.h\nindex 0ea8b4e..378c3bf 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -146,6 +146,9 @@ struct rev_info {\n \t\t\ttrack_first_time:1,\n \t\t\tlinear:1;\n\n+\t/* cmd type */\n+\tunsigned int  cmd_is_show:1;\n+\n \tenum date_mode date_mode;\n\n \tunsigned int\tabbrev;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 5f2b290..fed162e 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -887,4 +887,10 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \tgrep \"^| | gpg: Good signature\" actual\n '\n\n+test_expect_success 'log --graph --no-walk is forbidden' '\n+\techo \"fatal: cannot combine --no-walk with --graph\" >expect-error &&\n+\ttest_must_fail git log --graph --no-walk 2>error &&\n+\ttest_cmp expect-error error\n+'\n+\n test_done\ndiff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh\nindex 991ab4a..574e8d9 100755\n--- a/t/t6014-rev-list-all.sh\n+++ b/t/t6014-rev-list-all.sh\n@@ -35,4 +35,10 @@ test_expect_success 'repack does not lose detached HEAD' '\n\n '\n\n+test_expect_success 'rev-list --graph --no-walk is forbidden' '\n+\techo \"fatal: cannot combine --no-walk with --graph\" >expect-error &&\n+\ttest_must_fail git rev-list --graph --no-walk 2>error &&\n+\ttest_cmp expect-error error\n+'\n+\n test_done\n--\n2.3.1.253.g3de5837\n"},{"id":"257341","messageId":"xmqqwq2rqnvp.fsf@gitster.dls.corp.google.com","threadId":"38724","inReplyTo":"3de5837561c07fbf8d6187578fc37b3dbf2ea5f7.1425702676.git.dongcan.jiang@gmail.com","subject":"Re: [PATCH v2/GSoC/MICRO] revision: forbid combining --graph and --no-walk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-08T19:40:10Z","receivedAt":"2015-03-08T19:40:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dongcan Jiang <dongcan.jiang@gmail.com> writes:\n\n> Because --graph is about connected history while\n> --no-walk is about discrete points. [1]\n\nThe convention around here is that the title of the patch on the\nSubject line is *not* the beginning part of the first sentence, you\nwould need to phrase the above more like:\n\n        Because \"--graph\" is about ... discrete points, it does not\n        make sense to allow giving these two options at the same\n        time.\n\nto make it a complete sentence.\n\n> It's a pity that git-show has to allow such combination\n> in order to make t4052-stat-output.sh compatible. [2]\n\nIf you feel \"It's a pity\", it actually is OK to make a good argument\nfor \"fixing\" the test without adding the workaround.  A replacement\ncommit message for the above two lines for such an approach might\nlook like:\n\n        This change makes a few calls to \"show --graph\" fail in\n        t4052, but asking to show one commit _with_ graph is a\n        nonsensical thing to do.  Correct these offending tests that\n        expected \"--graph\" to be useful by removing \"--graph\" in\n        their invocations (and adjusting the expected output, of\n        course).\n\nThat way, people who do find it actually useful that \"show --graph HEAD\"\nthat shows something like this\n\n        $ git show --graph -s 52d5bf778\n        *   commit 52d5bf77875275bbfc1bf1d7b690f355d5c869e4\n        |\\  Merge: 36ab768 189c860\n        | | Author: Junio C Hamano <gitster@pobox.com>\n        | | Date:   Fri Mar 6 15:02:33 2015 -0800\n        | | \n        | |     Merge branch 'bw/kwset-use-unsigned'\n        ...\n\ncan react and argue why it is useful to them.  \n\nIt is important to notice that your \"It's a pity\" will no longer\napply, if we are keeping the feature because it is useful.  It would\nbe clear that we would be keeping the feature not because we are too\nlazy to correct tests, but because it is actually useful.\n\n> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>\n>\n> Thanks-to: Eric Sunshine, René Scharfe, Junio C Hamano\n\nThese look unusual for a few reasons: S-o-b is not at the end, we\nusually say Helped-by: instead, and we do not use Thanks-to: with\nmultiple names on a single line.\n\nPlease do not try to be original without a good reason.  We may\nstart counting the number of times people appear on these footers to\nsee how much contribution those who do not directly author commits\n(read: those who mentor others) are making.\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 5f2b290..fed162e 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -887,4 +887,10 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '\n>  \tgrep \"^| | gpg: Good signature\" actual\n>  '\n>\n> +test_expect_success 'log --graph --no-walk is forbidden' '\n> +\techo \"fatal: cannot combine --no-walk with --graph\" >expect-error &&\n> +\ttest_must_fail git log --graph --no-walk 2>error &&\n> +\ttest_cmp expect-error error\n> +'\n\nI do not think we want to check exact phrasing of the error\nmessage here.  Just the second line (without the \" 2>error &&\"\nat the end) should be sufficient.\n\n> +test_expect_success 'rev-list --graph --no-walk is forbidden' '\n> +\techo \"fatal: cannot combine --no-walk with --graph\" >expect-error &&\n> +\ttest_must_fail git rev-list --graph --no-walk 2>error &&\n> +\ttest_cmp expect-error error\n> +'\n\nThe same comment (about preferring not to check the error message)\napplies here, and more importantly, this is not a good test because\n\"git rev-list --graph\" without the forbidden \"--no-walk\" would fail\nfor other reasons.  Perhaps\n\n\ttest_must_fail git rev-list --graph --no-walk HEAD\n\nor something, assuming that there is already a commit pointed by\nHEAD at this point in the test (I didn't check).\n\nThanks.\n"},{"id":"257358","messageId":"297580e4cf8a1152224394ce27f67e2457657615.1425865346.git.dongcan.jiang@gmail.com","threadId":"38724","inReplyTo":"1425632110-31863-1-git-send-email-dongcan.jiang@gmail.com","subject":"[PATCH v3/GSoC/MICRO] revision: forbid combining --graph and --no-walk","fromName":"Dongcan Jiang","fromEmail":"dongcan.jiang@gmail.com","sentAt":"2015-03-09T04:09:54Z","receivedAt":"2015-03-09T04:09:54Z","isPatch":true,"sender":{"key":"dongcan.jiang@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10103762?v=4"},"body":"Because \"--graph\" is about connected history while --no-walk\nis about discrete points, it does not make sense to allow\ngiving these two options at the same time. [1]\n\nThis change allows git-show to have such options' combination\nas a special case, because git-show itself has underlying\n--no-walk option, while \"git show --graph\" is a legal and\nuseful operation which is tested in t4052. [2,3]\n\n2 testcases have been added to test this patch.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/216083\n[2]: http://article.gmane.org/gmane.comp.version-control.git/264950\n[3]: http://article.gmane.org/gmane.comp.version-control.git/265107\n\nHelped-By: Eric Sunshine <sunshine@sunshineco.com>\nHelped-By: René Scharfe <l.s.r@web.de>\nHelped-By: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>\n---\n Documentation/rev-list-options.txt | 2 ++\n builtin/log.c                      | 1 +\n revision.c                         | 4 ++++\n revision.h                         | 3 +++\n t/t4202-log.sh                     | 4 ++++\n t/t6014-rev-list-all.sh            | 4 ++++\n 6 files changed, 18 insertions(+)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..136abdf 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -679,6 +679,7 @@ endif::git-rev-list[]\n \tgiven on the command line. Otherwise (if `sorted` or no argument\n \twas given), the commits are shown in reverse chronological order\n \tby commit time.\n+\tCannot be combined with `--graph`.\n \n --do-walk::\n \tOverrides a previous `--no-walk`.\n@@ -781,6 +782,7 @@ you would get an output like this:\n \ton the left hand side of the output.  This may cause extra lines\n \tto be printed in between commits, in order for the graph history\n \tto be drawn properly.\n+\tCannot be combined with `--no-walk`.\n +\n This enables parent rewriting, see 'History Simplification' below.\n +\ndiff --git a/builtin/log.c b/builtin/log.c\nindex dd8f3fc..5b5d028 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -524,6 +524,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \n \tmemset(&match_all, 0, sizeof(match_all));\n \tinit_revisions(&rev, prefix);\n+\trev.cmd_is_show = 1;\n \trev.diff = 1;\n \trev.always_show_header = 1;\n \trev.no_walk = REVISION_WALK_NO_WALK_SORTED;\ndiff --git a/revision.c b/revision.c\nindex 66520c6..5d6fbef 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n \n \trevs->commit_format = CMIT_FMT_DEFAULT;\n \n+\trevs->cmd_is_show = 0;\n+\n \tinit_grep_defaults();\n \tgrep_init(&revs->grep_filter, prefix);\n \trevs->grep_filter.status_only = 1;\n@@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \tif (revs->reflog_info && revs->graph)\n \t\tdie(\"cannot combine --walk-reflogs with --graph\");\n+\tif (!revs->cmd_is_show && revs->no_walk && revs->graph)\n+\t\tdie(\"cannot combine --no-walk with --graph\");\n \tif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n \t\tdie(\"cannot use --grep-reflog without --walk-reflogs\");\n \ndiff --git a/revision.h b/revision.h\nindex 0ea8b4e..378c3bf 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -146,6 +146,9 @@ struct rev_info {\n \t\t\ttrack_first_time:1,\n \t\t\tlinear:1;\n \n+\t/* cmd type */\n+\tunsigned int  cmd_is_show:1;\n+\n \tenum date_mode date_mode;\n \n \tunsigned int\tabbrev;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 5f2b290..f111705 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -887,4 +887,8 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \tgrep \"^| | gpg: Good signature\" actual\n '\n \n+test_expect_success 'log --graph --no-walk is forbidden' '\n+\ttest_must_fail git log --graph --no-walk\n+'\n+\n test_done\ndiff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh\nindex 991ab4a..c9bedd2 100755\n--- a/t/t6014-rev-list-all.sh\n+++ b/t/t6014-rev-list-all.sh\n@@ -35,4 +35,8 @@ test_expect_success 'repack does not lose detached HEAD' '\n \n '\n \n+test_expect_success 'rev-list --graph --no-walk is forbidden' '\n+\ttest_must_fail git rev-list --graph --no-walk HEAD\n+'\n+\n test_done\n-- \n2.3.1.252.ge67f612\n"},{"id":"257499","messageId":"xmqqegowzg4l.fsf@gitster.dls.corp.google.com","threadId":"38724","inReplyTo":"297580e4cf8a1152224394ce27f67e2457657615.1425865346.git.dongcan.jiang@gmail.com","subject":"Re: [PATCH v3/GSoC/MICRO] revision: forbid combining --graph and --no-walk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-10T21:39:38Z","receivedAt":"2015-03-10T21:39:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dongcan Jiang <dongcan.jiang@gmail.com> writes:\n\n> Because \"--graph\" is about connected history while --no-walk\n> is about discrete points, it does not make sense to allow\n> giving these two options at the same time. [1]\n>\n> This change allows git-show to have such options' combination\n> as a special case, because git-show itself has underlying\n> --no-walk option, while \"git show --graph\" is a legal and\n> useful operation which is tested in t4052. [2,3]\n\nHmph, I actually was hoping to see that you would either (1) explain\nwhy this special case is not useful, fix t4052 and do without\ncmd_is_show bit, or (2) explain why this special case _is_ useful in\na more concrete terms.\n\n\"X is legal and tested\" does not automatically imply that whatever\nrandom thing the implementation does, and the test whose expectation\nmatches what it does, is a well-thought-out and a useful operation.\nIf you are going in the direction (2), it would have been better if\nthe reason why \"git show --graph one_commit\" is useful is given here\nin your own words.\n\nYou do not want to force those who are reading this log message 6\nmonths down the road to visit [2,3] for more details.\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index dd8f3fc..5b5d028 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -524,6 +524,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>  \n>  \tmemset(&match_all, 0, sizeof(match_all));\n>  \tinit_revisions(&rev, prefix);\n> +\trev.cmd_is_show = 1;\n>  \trev.diff = 1;\n>  \trev.always_show_header = 1;\n>  \trev.no_walk = REVISION_WALK_NO_WALK_SORTED;\n\nOK.\n\n> diff --git a/revision.c b/revision.c\n> index 66520c6..5d6fbef 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)\n>  \n>  \trevs->commit_format = CMIT_FMT_DEFAULT;\n>  \n> +\trevs->cmd_is_show = 0;\n> +\n\nThe new assignment and a blank line shouldn't be necessary; the\nmemset() at the beginning is there so that you do not have to do\nthis.\n\n> diff --git a/revision.h b/revision.h\n> index 0ea8b4e..378c3bf 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -146,6 +146,9 @@ struct rev_info {\n>  \t\t\ttrack_first_time:1,\n>  \t\t\tlinear:1;\n>  \n> +\t/* cmd type */\n> +\tunsigned int  cmd_is_show:1;\n\nIf you are going to comment, imagine that somebody will want to add\na new subcommand in \"git log\" family in the future, and try to help\nhim decide if he wants to flip this bit for his subcommand with that\ncomment.  He would scratch his head, reading \"cmd type?\", and wonder\n\"Hmmmm, what makes 'show' special?  Is my new command also special\nlike 'show' is?  What makes my new command the same cmd type as\n'show' (or different)?\"  The comment does not help him answer these\nquestions very much.\n\nAn alternative is to not to add the misleading comment; cmd_is_show\nis clear enough indication for such a person that he does not want\nthe bit set because whatever new subcommand he is adding is not\n'show'.\n\nThis is becoming to appear more and more that cmd_is_show is \"allow\ncombined use of graph and no-walk only to avoid breaking a few\ntests\", not \"in the context of show, graph and no-walk is useful\",\nat least to me.  Perhaps the comment should say\n\n\t/*\n         * special case to prevent 'git show --graph' that does not\n         * walk from triggering the usual \"--no-walk and --graph cannot\n         * be used together\" error.\n         */\n\tunsigned int cmd_is_show:1;\n\nor even name the variable more explicitly, i.e.\n\n\tunsigned int allow_graph_and_no_walk:1;\n\nI dunno.\n\nThe tests look fine.  Thanks.\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 5f2b290..f111705 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -887,4 +887,8 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '\n>  \tgrep \"^| | gpg: Good signature\" actual\n>  '\n>  \n> +test_expect_success 'log --graph --no-walk is forbidden' '\n> +\ttest_must_fail git log --graph --no-walk\n> +'\n> +\n>  test_done\n> diff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh\n> index 991ab4a..c9bedd2 100755\n> --- a/t/t6014-rev-list-all.sh\n> +++ b/t/t6014-rev-list-all.sh\n> @@ -35,4 +35,8 @@ test_expect_success 'repack does not lose detached HEAD' '\n>  \n>  '\n>  \n> +test_expect_success 'rev-list --graph --no-walk is forbidden' '\n> +\ttest_must_fail git rev-list --graph --no-walk HEAD\n> +'\n> +\n>  test_done\n"},{"id":"257526","messageId":"c6c08cdbbc0060c6d834c86d13a142cde290dc5c.1426039099.git.dongcan.jiang@gmail.com","threadId":"38724","inReplyTo":"1425632110-31863-1-git-send-email-dongcan.jiang@gmail.com","subject":"[PATCH v4/GSoC/MICRO] revision: forbid combining --graph and --no-walk","fromName":"Dongcan Jiang","fromEmail":"dongcan.jiang@gmail.com","sentAt":"2015-03-11T02:13:02Z","receivedAt":"2015-03-11T02:13:02Z","isPatch":true,"sender":{"key":"dongcan.jiang@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10103762?v=4"},"body":"Because \"--graph\" is about connected history while --no-walk\nis about discrete points, it does not make sense to allow\ngiving these two options at the same time. [1]\n\nThis change makes a few calls to \"show --graph\" fail in\nt4052, but asking to show one commit with graph is a\nnonsensical thing to do. Thus, tests on \"show --graph\" in\nt4052 have been removed. [2,3] Same tests on \"show\" without\n--graph option have already been tested in 4052.\n\n3 testcases have been added to test this patch.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/216083\n[2]: http://article.gmane.org/gmane.comp.version-control.git/264950\n[3]: http://article.gmane.org/gmane.comp.version-control.git/265107\n\nHelped-By: Eric Sunshine <sunshine@sunshineco.com>\nHelped-By: René Scharfe <l.s.r@web.de>\nHelped-By: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>\n---\n Documentation/rev-list-options.txt |  2 ++\n revision.c                         |  2 ++\n t/t4052-stat-output.sh             | 14 +++++++-------\n t/t4202-log.sh                     |  4 ++++\n t/t6014-rev-list-all.sh            |  4 ++++\n t/t7007-show.sh                    |  4 ++++\n 6 files changed, 23 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..136abdf 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -679,6 +679,7 @@ endif::git-rev-list[]\n \tgiven on the command line. Otherwise (if `sorted` or no argument\n \twas given), the commits are shown in reverse chronological order\n \tby commit time.\n+\tCannot be combined with `--graph`.\n \n --do-walk::\n \tOverrides a previous `--no-walk`.\n@@ -781,6 +782,7 @@ you would get an output like this:\n \ton the left hand side of the output.  This may cause extra lines\n \tto be printed in between commits, in order for the graph history\n \tto be drawn properly.\n+\tCannot be combined with `--no-walk`.\n +\n This enables parent rewriting, see 'History Simplification' below.\n +\ndiff --git a/revision.c b/revision.c\nindex 66520c6..6cd91dd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2339,6 +2339,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \tif (revs->reflog_info && revs->graph)\n \t\tdie(\"cannot combine --walk-reflogs with --graph\");\n+\tif (revs->no_walk && revs->graph)\n+\t\tdie(\"cannot combine --no-walk with --graph\");\n \tif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n \t\tdie(\"cannot use --grep-reflog without --walk-reflogs\");\n \ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex b68afef..a989e8f 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -99,7 +99,7 @@ do\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff -a \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --graph $verb COLUMNS (big change)\" '\n \t\tCOLUMNS=200 git $cmd $args --graph >output\n@@ -127,7 +127,7 @@ do\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff -a \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --graph $verb not enough COLUMNS (big change)\" '\n \t\tCOLUMNS=40 git $cmd $args --graph >output\n@@ -155,7 +155,7 @@ do\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff -a \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --graph $verb statGraphWidth config\" '\n \t\tgit -c diff.statGraphWidth=26 $cmd $args --graph >output\n@@ -196,7 +196,7 @@ do\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff -a \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --stat-width=width --graph with big change\" '\n \t\tgit $cmd $args --stat-width=40 --graph >output\n@@ -236,7 +236,7 @@ do\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff -a \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --stat=width --graph with big change is balanced\" '\n \t\tgit $cmd $args --stat-width=60 --graph >output &&\n@@ -270,7 +270,7 @@ do\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff -a \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --graph $verb COLUMNS (long filename)\" '\n \t\tCOLUMNS=200 git $cmd $args --graph >output\n@@ -299,7 +299,7 @@ do\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff -a \"$cmd\" != show || continue\n \n \ttest_expect_success COLUMNS_CAN_BE_1 \\\n \t\t\"$cmd --graph $verb prefix greater than COLUMNS (big change)\" '\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 5f2b290..f111705 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -887,4 +887,8 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \tgrep \"^| | gpg: Good signature\" actual\n '\n \n+test_expect_success 'log --graph --no-walk is forbidden' '\n+\ttest_must_fail git log --graph --no-walk\n+'\n+\n test_done\ndiff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh\nindex 991ab4a..c9bedd2 100755\n--- a/t/t6014-rev-list-all.sh\n+++ b/t/t6014-rev-list-all.sh\n@@ -35,4 +35,8 @@ test_expect_success 'repack does not lose detached HEAD' '\n \n '\n \n+test_expect_success 'rev-list --graph --no-walk is forbidden' '\n+\ttest_must_fail git rev-list --graph --no-walk HEAD\n+'\n+\n test_done\ndiff --git a/t/t7007-show.sh b/t/t7007-show.sh\nindex 1b824fe..42d3db6 100755\n--- a/t/t7007-show.sh\n+++ b/t/t7007-show.sh\n@@ -124,4 +124,8 @@ test_expect_success '--quiet suppresses diff' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'show --graph is forbidden' '\n+  test_must_fail git show --graph HEAD\n+'\n+\n test_done\n-- \n2.3.1.252.ge67f612.dirty\n"},{"id":"257900","messageId":"xmqq4mpjmcvv.fsf@gitster.dls.corp.google.com","threadId":"38724","inReplyTo":"c6c08cdbbc0060c6d834c86d13a142cde290dc5c.1426039099.git.dongcan.jiang@gmail.com","subject":"Re: [PATCH v4/GSoC/MICRO] revision: forbid combining --graph and --no-walk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T23:18:28Z","receivedAt":"2015-03-17T23:18:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dongcan Jiang <dongcan.jiang@gmail.com> writes:\n\n> Because \"--graph\" is about connected history while --no-walk\n> is about discrete points, it does not make sense to allow\n> giving these two options at the same time. [1]\n>\n> This change makes a few calls to \"show --graph\" fail in\n> t4052, but asking to show one commit with graph is a\n> nonsensical thing to do. Thus, tests on \"show --graph\" in\n> t4052 have been removed. [2,3] Same tests on \"show\" without\n> --graph option have already been tested in 4052.\n\nThis looks almost perfect.\n>\n> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\n> index b68afef..a989e8f 100755\n> --- a/t/t4052-stat-output.sh\n> +++ b/t/t4052-stat-output.sh\n> @@ -99,7 +99,7 @@ do\n>  \t\ttest_cmp \"$expect\" actual\n>  \t'\n>  \n> -\ttest \"$cmd\" != diff || continue\n> +\ttest \"$cmd\" != diff -a \"$cmd\" != show || continue\n\nI think we avoid -a and -o used with test (don't we have a write-up\non this somewhere?).  Write it like this\n\n\ttest \"$cmd\" != diff &&\n        test \"$cmd\" != show || continue\n\nor perhaps like this\n\n\tcase \"$cmd\" in diff|show) continue ;; esac\n\nOther than that I do not see anything objectionable.\n\nThanks, good job.\n"},{"id":"257901","messageId":"CAPig+cRUqwcunX8mGKbsyBZPeTrz92jqvBJtDKfXvVzHvbun2g@mail.gmail.com","threadId":"38724","inReplyTo":"xmqq4mpjmcvv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4/GSoC/MICRO] revision: forbid combining --graph and --no-walk","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-17T23:24:40Z","receivedAt":"2015-03-17T23:24:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 17, 2015 at 7:18 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Dongcan Jiang <dongcan.jiang@gmail.com> writes:\n>> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\n>> index b68afef..a989e8f 100755\n>> --- a/t/t4052-stat-output.sh\n>> +++ b/t/t4052-stat-output.sh\n>> @@ -99,7 +99,7 @@ do\n>>               test_cmp \"$expect\" actual\n>>       '\n>>\n>> -     test \"$cmd\" != diff || continue\n>> +     test \"$cmd\" != diff -a \"$cmd\" != show || continue\n>\n> I think we avoid -a and -o used with test (don't we have a write-up\n> on this somewhere?).\n\nThe very last item in the shell script section of CodingGuidelines discusses it.\n\n> Write it like this\n>\n>         test \"$cmd\" != diff &&\n>         test \"$cmd\" != show || continue\n>\n> or perhaps like this\n>\n>         case \"$cmd\" in diff|show) continue ;; esac\n"},{"id":"257904","messageId":"075bb408bff8cbbd4dfaed91d2b21361546d2c39.1426643446.git.dongcan.jiang@gmail.com","threadId":"38724","inReplyTo":"1425632110-31863-1-git-send-email-dongcan.jiang@gmail.com","subject":"[PATCH v5/GSoC/MICRO] revision: forbid combining --graph and --no-walk","fromName":"Dongcan Jiang","fromEmail":"dongcan.jiang@gmail.com","sentAt":"2015-03-18T05:34:01Z","receivedAt":"2015-03-18T05:34:01Z","isPatch":true,"sender":{"key":"dongcan.jiang@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10103762?v=4"},"body":"Because \"--graph\" is about connected history while --no-walk\nis about discrete points, it does not make sense to allow\ngiving these two options at the same time. [1]\n\nThis change makes a few calls to \"show --graph\" fail in\nt4052, but asking to show one commit with graph is a\nnonsensical thing to do. Thus, tests on \"show --graph\" in\nt4052 have been removed. [2,3] Same tests on \"show\" without\n--graph option have already been tested in 4052.\n\n3 testcases have been added to test this patch.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/216083\n[2]: http://article.gmane.org/gmane.comp.version-control.git/264950\n[3]: http://article.gmane.org/gmane.comp.version-control.git/265107\n\nHelped-By: Eric Sunshine <sunshine@sunshineco.com>\nHelped-By: René Scharfe <l.s.r@web.de>\nHelped-By: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>\n---\n Documentation/rev-list-options.txt |  2 ++\n revision.c                         |  2 ++\n t/t4052-stat-output.sh             | 14 +++++++-------\n t/t4202-log.sh                     |  4 ++++\n t/t6014-rev-list-all.sh            |  4 ++++\n t/t7007-show.sh                    |  4 ++++\n 6 files changed, 23 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..136abdf 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -679,6 +679,7 @@ endif::git-rev-list[]\n \tgiven on the command line. Otherwise (if `sorted` or no argument\n \twas given), the commits are shown in reverse chronological order\n \tby commit time.\n+\tCannot be combined with `--graph`.\n \n --do-walk::\n \tOverrides a previous `--no-walk`.\n@@ -781,6 +782,7 @@ you would get an output like this:\n \ton the left hand side of the output.  This may cause extra lines\n \tto be printed in between commits, in order for the graph history\n \tto be drawn properly.\n+\tCannot be combined with `--no-walk`.\n +\n This enables parent rewriting, see 'History Simplification' below.\n +\ndiff --git a/revision.c b/revision.c\nindex 66520c6..6cd91dd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2339,6 +2339,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \tif (revs->reflog_info && revs->graph)\n \t\tdie(\"cannot combine --walk-reflogs with --graph\");\n+\tif (revs->no_walk && revs->graph)\n+\t\tdie(\"cannot combine --no-walk with --graph\");\n \tif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n \t\tdie(\"cannot use --grep-reflog without --walk-reflogs\");\n \ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex b68afef..54f10cf 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -99,7 +99,7 @@ do\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff && test \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --graph $verb COLUMNS (big change)\" '\n \t\tCOLUMNS=200 git $cmd $args --graph >output\n@@ -127,7 +127,7 @@ do\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff && test \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --graph $verb not enough COLUMNS (big change)\" '\n \t\tCOLUMNS=40 git $cmd $args --graph >output\n@@ -155,7 +155,7 @@ do\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff && test \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --graph $verb statGraphWidth config\" '\n \t\tgit -c diff.statGraphWidth=26 $cmd $args --graph >output\n@@ -196,7 +196,7 @@ do\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff && test \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --stat-width=width --graph with big change\" '\n \t\tgit $cmd $args --stat-width=40 --graph >output\n@@ -236,7 +236,7 @@ do\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff && test \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --stat=width --graph with big change is balanced\" '\n \t\tgit $cmd $args --stat-width=60 --graph >output &&\n@@ -270,7 +270,7 @@ do\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff && test \"$cmd\" != show || continue\n \n \ttest_expect_success \"$cmd --graph $verb COLUMNS (long filename)\" '\n \t\tCOLUMNS=200 git $cmd $args --graph >output\n@@ -299,7 +299,7 @@ do\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n-\ttest \"$cmd\" != diff || continue\n+\ttest \"$cmd\" != diff && test \"$cmd\" != show || continue\n \n \ttest_expect_success COLUMNS_CAN_BE_1 \\\n \t\t\"$cmd --graph $verb prefix greater than COLUMNS (big change)\" '\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 5f2b290..f111705 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -887,4 +887,8 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \tgrep \"^| | gpg: Good signature\" actual\n '\n \n+test_expect_success 'log --graph --no-walk is forbidden' '\n+\ttest_must_fail git log --graph --no-walk\n+'\n+\n test_done\ndiff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh\nindex 991ab4a..c9bedd2 100755\n--- a/t/t6014-rev-list-all.sh\n+++ b/t/t6014-rev-list-all.sh\n@@ -35,4 +35,8 @@ test_expect_success 'repack does not lose detached HEAD' '\n \n '\n \n+test_expect_success 'rev-list --graph --no-walk is forbidden' '\n+\ttest_must_fail git rev-list --graph --no-walk HEAD\n+'\n+\n test_done\ndiff --git a/t/t7007-show.sh b/t/t7007-show.sh\nindex 1b824fe..42d3db6 100755\n--- a/t/t7007-show.sh\n+++ b/t/t7007-show.sh\n@@ -124,4 +124,8 @@ test_expect_success '--quiet suppresses diff' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'show --graph is forbidden' '\n+  test_must_fail git show --graph HEAD\n+'\n+\n test_done\n-- \n2.3.2.224.g6c2f212\n"}]}