{"thread":{"id":"21975","subject":"[RFC/PATCH] gitweb: link to toggle 'no merges' option","startedAt":"2009-12-17T09:05:53Z","lastAt":"2009-12-17T21:14:25Z","messageCount":5,"participants":["Giuseppe Bilotta","Etienne Vallette d'Osia","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"130021","messageId":"1261040753-4859-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"21975","inReplyTo":null,"subject":"[RFC/PATCH] gitweb: link to toggle 'no merges' option","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-12-17T09:05:53Z","receivedAt":"2009-12-17T09:05:53Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"In views that support --no-merges, introduce a link that toggles the\noption.\n\nSigned-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n\n---\n gitweb/gitweb.css  |   11 +++++++++++\n gitweb/gitweb.perl |   14 ++++++++++++++\n 2 files changed, 25 insertions(+), 0 deletions(-)\n\nThis is something I've been wanting for a while. There are a number of\nthings that don't 'click' with this proof of concept, and I'm coming to\nthe list to hear the opinion of users and developers on how to improve\nthe thing.\n\nThe patch is live at http://git.oblomov.eu/, an example affected page is\nhttp://git.oblomov.eu/git/shortlog\n\nThings that are sure to change:\n\n * the aesthetics and location of the toggle link (it shows on mousehover\n   in the title). Other options I've considered are:\n    + next to pagination (first | prev | next), either before or after\n      the existing entries\n    + on mouseover for the table section that refers to the (short)log;\n      this would make it possible to put it summary view too, for example\n\n * if you toggle merge view when not on the first page, the reference\n   (first) commit in the view is likely to change drastically, which\n   causes confusion. I have not found a satisfactory solution for this,\n   since the obvious way to 'lock' the view (start paginating from the\n   current top commit) prevents prev/next navigation\n\ndiff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\nindex 50067f2..0da6ef0 100644\n--- a/gitweb/gitweb.css\n+++ b/gitweb/gitweb.css\n@@ -572,3 +572,14 @@ span.match {\n div.binary {\n \tfont-style: italic;\n }\n+\n+span.merge_toggle a {\n+\tfont-size: 66%;\n+\tcolor: white !important;\n+\tfont-weight: normal;\n+\tvertical-align: top;\n+\ttext-decoration:none;\n+\tvisibility: hidden;\n+}\n+\n+*:hover > span.merge_toggle a { visibility:visible }\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7e477af..a63f419 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3118,11 +3118,15 @@ sub git_header_html {\n \tmy $status = shift || \"200 OK\";\n \tmy $expires = shift;\n \n+\tmy $can_have_merges = grep(/^$action$/, @{$allowed_options{'--no-merges'}});\n+\tmy $has_merges = !grep(/^--no-merges$/, @extra_options);\n+\n \tmy $title = \"$site_name\";\n \tif (defined $project) {\n \t\t$title .= \" - \" . to_utf8($project);\n \t\tif (defined $action) {\n \t\t\t$title .= \"/$action\";\n+\t\t\t$title .= \" (no merges)\" unless $has_merges;\n \t\t\tif (defined $file_name) {\n \t\t\t\t$title .= \" - \" . esc_path($file_name);\n \t\t\t\tif ($action eq \"tree\" && $file_name !~ m|/$|) {\n@@ -3235,6 +3239,16 @@ EOF\n \t\tprint $cgi->a({-href => href(action=>\"summary\")}, esc_html($project));\n \t\tif (defined $action) {\n \t\t\tprint \" / $action\";\n+\t\t\tif ($can_have_merges) {\n+\t\t\t\tprint \" <span class='merge_toggle'>\";\n+\t\t\t\tif ($has_merges) {\n+\t\t\t\t\tprintf('<a href=\"%s\">hide merges</a>', href(-replay=>1, 'extra_options'=>('--no-merges', @extra_options)));\n+\t\t\t\t} else {\n+\t\t\t\t\tmy @href_extra = grep(!/^--no-merges$/, @extra_options);\n+\t\t\t\t\tprintf('<a href=\"%s\">show merges</a>', href(-replay=>1, 'extra_options'=>@href_extra));\n+\t\t\t\t}\n+\t\t\t\tprint \"</span>\";\n+\t\t\t}\n \t\t}\n \t\tprint \"\\n\";\n \t}\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"130030","messageId":"hgda76$tf5$1@ger.gmane.org","threadId":"21975","inReplyTo":"1261040753-4859-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [RFC/PATCH] gitweb: link to toggle 'no merges' option","fromName":"Etienne Vallette d'Osia","fromEmail":"etienne.vallettedosia@gmail.com","sentAt":"2009-12-17T13:03:58Z","receivedAt":"2009-12-17T13:03:58Z","isPatch":true,"sender":{"key":"etienne.vallettedosia@gmail.com","avatar":null},"body":"Oh, this is exactly what I wanted for a long long time...\nI hope this feature will be merged soon !\n\nLe 12/17/2009 10:05 AM, Giuseppe Bilotta a écrit :\n> In views that support --no-merges, introduce a link that toggles the\n> option.\n> \n> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n> \n> ---\n>  gitweb/gitweb.css  |   11 +++++++++++\n>  gitweb/gitweb.perl |   14 ++++++++++++++\n>  2 files changed, 25 insertions(+), 0 deletions(-)\n> \n> This is something I've been wanting for a while. There are a number of\n> things that don't 'click' with this proof of concept, and I'm coming to\n> the list to hear the opinion of users and developers on how to improve\n> the thing.\n> \n> The patch is live at http://git.oblomov.eu/, an example affected page is\n> http://git.oblomov.eu/git/shortlog\n> \n> Things that are sure to change:\n> \n>  * the aesthetics and location of the toggle link (it shows on mousehover\n>    in the title). Other options I've considered are:\n>     + next to pagination (first | prev | next), either before or after\n>       the existing entries\n>     + on mouseover for the table section that refers to the (short)log;\n>       this would make it possible to put it summary view too, for example\n> \n>  * if you toggle merge view when not on the first page, the reference\n>    (first) commit in the view is likely to change drastically, which\n>    causes confusion. I have not found a satisfactory solution for this,\n>    since the obvious way to 'lock' the view (start paginating from the\n>    current top commit) prevents prev/next navigation\n> \n> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\n> index 50067f2..0da6ef0 100644\n> --- a/gitweb/gitweb.css\n> +++ b/gitweb/gitweb.css\n> @@ -572,3 +572,14 @@ span.match {\n>  div.binary {\n>  \tfont-style: italic;\n>  }\n> +\n> +span.merge_toggle a {\n> +\tfont-size: 66%;\n> +\tcolor: white !important;\n> +\tfont-weight: normal;\n> +\tvertical-align: top;\n> +\ttext-decoration:none;\n> +\tvisibility: hidden;\n> +}\n> +\n> +*:hover > span.merge_toggle a { visibility:visible }\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 7e477af..a63f419 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3118,11 +3118,15 @@ sub git_header_html {\n>  \tmy $status = shift || \"200 OK\";\n>  \tmy $expires = shift;\n>  \n> +\tmy $can_have_merges = grep(/^$action$/, @{$allowed_options{'--no-merges'}});\n> +\tmy $has_merges = !grep(/^--no-merges$/, @extra_options);\n> +\n>  \tmy $title = \"$site_name\";\n>  \tif (defined $project) {\n>  \t\t$title .= \" - \" . to_utf8($project);\n>  \t\tif (defined $action) {\n>  \t\t\t$title .= \"/$action\";\n> +\t\t\t$title .= \" (no merges)\" unless $has_merges;\n>  \t\t\tif (defined $file_name) {\n>  \t\t\t\t$title .= \" - \" . esc_path($file_name);\n>  \t\t\t\tif ($action eq \"tree\" && $file_name !~ m|/$|) {\n> @@ -3235,6 +3239,16 @@ EOF\n>  \t\tprint $cgi->a({-href => href(action=>\"summary\")}, esc_html($project));\n>  \t\tif (defined $action) {\n>  \t\t\tprint \" / $action\";\n> +\t\t\tif ($can_have_merges) {\n> +\t\t\t\tprint \" <span class='merge_toggle'>\";\n> +\t\t\t\tif ($has_merges) {\n> +\t\t\t\t\tprintf('<a href=\"%s\">hide merges</a>', href(-replay=>1, 'extra_options'=>('--no-merges', @extra_options)));\n> +\t\t\t\t} else {\n> +\t\t\t\t\tmy @href_extra = grep(!/^--no-merges$/, @extra_options);\n> +\t\t\t\t\tprintf('<a href=\"%s\">show merges</a>', href(-replay=>1, 'extra_options'=>@href_extra));\n> +\t\t\t\t}\n> +\t\t\t\tprint \"</span>\";\n> +\t\t\t}\n>  \t\t}\n>  \t\tprint \"\\n\";\n>  \t}\n\n--\nEtienne Vallette d'Osia\n"},{"id":"130034","messageId":"200912171637.45810.jnareb@gmail.com","threadId":"21975","inReplyTo":"1261040753-4859-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [RFC/PATCH] gitweb: link to toggle 'no merges' option","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-12-17T15:37:44Z","receivedAt":"2009-12-17T15:37:44Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 17 Dec 2009 10:05 +0100, Giuseppe Bilotta wrote:\n\n> In views that support --no-merges, introduce a link that toggles the\n> option.\n\nI like this idea of introducing interface for so far hidden feature\n(well, except hidden in one of feed <link ...> in page header).\n \n> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n> \n> ---\n>  gitweb/gitweb.css  |   11 +++++++++++\n>  gitweb/gitweb.perl |   14 ++++++++++++++\n>  2 files changed, 25 insertions(+), 0 deletions(-)\n> \n> This is something I've been wanting for a while. There are a number of\n> things that don't 'click' with this proof of concept, and I'm coming to\n> the list to hear the opinion of users and developers on how to improve\n> the thing.\n> \n> The patch is live at http://git.oblomov.eu/, an example affected page is\n> http://git.oblomov.eu/git/shortlog\n\nYou should tell here that one must put mouse over main header (the one\nwith 'projects / project / action' breadcrumbs) for the new link to be\nvisible... because I was wondering where is this new link.\n\n> \n> Things that are sure to change:\n> \n>  * the aesthetics and location of the toggle link (it shows on mousehover\n>    in the title). \n\nIt is not only the question where to put link, but also where to put\nthe *code* (where the code should belong to).\n\nAt first I thought: WTF? Why the feature that deals with log-like views\nis put in very generic and common for all actions/views git_header_html\nsubroutine?  Especially after change that made all loglike views use\ncommon infrastructure of git_log_generic.\n\nBut then I realized that it is specific example of *generic* feature\nthat deals with extra_options... which admittedly is currently limited\nto '--no-merges' only.  So if it is put in git_header_html, then all\nviews with HTML output (which does not include 'atom' and 'rss' actions,\nbut which actions IIRC have their own handling of '--no-merges')\nwhich have support for extra_options would have ability to turn them\non and off.\n\nWhat you need to add (if this link is to be in git_header_html) is\nto create links only when $status == 200, because otherwise the link\nwould be present also for error pages, which I think is wrong.\n\n>    Other options I've considered are: \n>     + next to pagination (first | prev | next), either before or after\n>       the existing entries\n\nThis would fit with the fact that sometimes present \"patches\" link is on\nthe line with pagination links, after pagination links.\n\nBut this secondary navigation bar is about other views, and extra_options\nis about modifying current view, and functions more like toggles.  OTOH\nwe have such toggle for 'blame' <-> 'blame_incremental' switch in \nsecondary navigation bar.\n\nAlso this would mean that each view type would have to handle extra_options\nitself, as secondary navigation bar is very much action-specific.  Not\nthat it matters now, with only single '--no-merges' option supported.\n\n>     + on mouseover for the table section that refers to the (short)log;\n>       this would make it possible to put it summary view too, for example\n\nThis would mean having link inside link, as those headers in summary view\nfunctions as link to 'shortlog' view (quite useful I think), and to the\nproject summary in the 'shortlog' view itself (I'm not sure how useful \nthat is).  We already have problems with ref markers being links inside\nlinks and having broken layout in some strict XHTML conformance browsers.\n\n\nIn short: I am not sure what would be the best solution.  Nevertheless\nI think that link should be more visible, and perhaps more toggle-like.\n\n>  * if you toggle merge view when not on the first page, the reference\n>    (first) commit in the view is likely to change drastically, which\n>    causes confusion. I have not found a satisfactory solution for this,\n>    since the obvious way to 'lock' the view (start paginating from the\n>    current top commit) prevents prev/next navigation\n\nAlternate solution would be to clean page (start from 0th page) when\nchanging to '--no-merges'.\n\nYou would probably need something like 'skip' option, with number of\ncommits, not number of pages to skip, and/or 'skip_to' which takes\ncommit-id.  But i have not thought about this much... \n\n> \n> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\n> index 50067f2..0da6ef0 100644\n> --- a/gitweb/gitweb.css\n> +++ b/gitweb/gitweb.css\n> @@ -572,3 +572,14 @@ span.match {\n>  div.binary {\n>  \tfont-style: italic;\n>  }\n> +\n> +span.merge_toggle a {\n> +\tfont-size: 66%;\n> +\tcolor: white !important;\n> +\tfont-weight: normal;\n> +\tvertical-align: top;\n> +\ttext-decoration:none;\n> +\tvisibility: hidden;\n> +}\n\nI think it should be more visible.\n\nOtherwise only people \"in the know\" would be able to use this.\n\n> +\n> +*:hover > span.merge_toggle a { visibility:visible }\n\nI'd rather not have this rule to have different style, i.e. not all\nin single line.  Unless it is for RFC only...\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 7e477af..a63f419 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3118,11 +3118,15 @@ sub git_header_html {\n>  \tmy $status = shift || \"200 OK\";\n>  \tmy $expires = shift;\n>  \n> +\tmy $can_have_merges = grep(/^$action$/, @{$allowed_options{'--no-merges'}});\n> +\tmy $has_merges = !grep(/^--no-merges$/, @extra_options);\n> +\n\nWouldn't it be better to use straight\n\n +\tmy $no_merges = grep(/^--no-merges$/, @extra_options);\n\nBecause $has_merges is true also for example for 'tree' view... which\nabsolutely doesn't make any sense whatsoever.\n\n\nIn more generic solution (which could be perhaps put in a separate commit)\nit could be:\n\n  +\tmy %extra_options = map { $_ => 0 } keys %$allowed_options;\n  +\t$extra_options{$_} = 1 foreach @extra_options;\n\nwhich means that %extra_options is hash which keys are allowed options,\nand which has true value for allowed option which is actually used.\n\nOr something like that, if above is to cryptic.\n\n>  \tmy $title = \"$site_name\";\n>  \tif (defined $project) {\n>  \t\t$title .= \" - \" . to_utf8($project);\n>  \t\tif (defined $action) {\n>  \t\t\t$title .= \"/$action\";\n> +\t\t\t$title .= \" (no merges)\" unless $has_merges;\n\nWouldn't it be better to use\n\n  +\t\t\t$title .= \" (no merges)\" if $no_merges;\n\nMore straightforward, and without misleading $has_merges.\n\n>  \t\t\tif (defined $file_name) {\n>  \t\t\t\t$title .= \" - \" . esc_path($file_name);\n>  \t\t\t\tif ($action eq \"tree\" && $file_name !~ m|/$|) {\n> @@ -3235,6 +3239,16 @@ EOF\n>  \t\tprint $cgi->a({-href => href(action=>\"summary\")}, esc_html($project));\n>  \t\tif (defined $action) {\n>  \t\t\tprint \" / $action\";\n> +\t\t\tif ($can_have_merges) {\n> +\t\t\t\tprint \" <span class='merge_toggle'>\";\n> +\t\t\t\tif ($has_merges) {\n> +\t\t\t\t\tprintf('<a href=\"%s\">hide merges</a>', href(-replay=>1, 'extra_options'=>('--no-merges', @extra_options)));\n\nIt would be more symmetric to use:\n\n  +\t\t\t\t\tmy @href_extra = ('--no-merges', @extra_options);\n  +\t\t\t\t\tprintf('<a href=\"%s\">hide merges</a>', href(-replay=>1, 'extra_options'=>@href_extra));\n\n\n> +\t\t\t\t} else {\n> +\t\t\t\t\tmy @href_extra = grep(!/^--no-merges$/, @extra_options);\n> +\t\t\t\t\tprintf('<a href=\"%s\">show merges</a>', href(-replay=>1, 'extra_options'=>@href_extra));\n> +\t\t\t\t}\n\nand then this conditional could be simplified a bit.\n\n> +\t\t\t\tprint \"</span>\";\n> +\t\t\t}\n>  \t\t}\n>  \t\tprint \"\\n\";\n>  \t}\n> -- \n> 1.6.3.rc1.192.gdbfcb\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"130042","messageId":"cb7bb73a0912171241j56ecd2f1y3dc66cf3b86bd784@mail.gmail.com","threadId":"21975","inReplyTo":"200912171637.45810.jnareb@gmail.com","subject":"Re: [RFC/PATCH] gitweb: link to toggle 'no merges' option","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-12-17T20:41:10Z","receivedAt":"2009-12-17T20:41:10Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/12/17 Jakub Narebski <jnareb@gmail.com>:\n> On Thu, 17 Dec 2009 10:05 +0100, Giuseppe Bilotta wrote:\n>> This is something I've been wanting for a while. There are a number of\n>> things that don't 'click' with this proof of concept, and I'm coming to\n>> the list to hear the opinion of users and developers on how to improve\n>> the thing.\n>>\n>> The patch is live at http://git.oblomov.eu/, an example affected page is\n>> http://git.oblomov.eu/git/shortlog\n>\n> You should tell here that one must put mouse over main header (the one\n> with 'projects / project / action' breadcrumbs) for the new link to be\n> visible... because I was wondering where is this new link.\n\nI mentioned it right below, but I do realize that people might have\ngone looking for it before reading further down ;-)\n\n>> Things that are sure to change:\n>>\n>>  * the aesthetics and location of the toggle link (it shows on mousehover\n>>    in the title).\n>\n> It is not only the question where to put link, but also where to put\n> the *code* (where the code should belong to).\n>\n> At first I thought: WTF? Why the feature that deals with log-like views\n> is put in very generic and common for all actions/views git_header_html\n> subroutine?  Especially after change that made all loglike views use\n> common infrastructure of git_log_generic.\n>\n> But then I realized that it is specific example of *generic* feature\n> that deals with extra_options... which admittedly is currently limited\n> to '--no-merges' only.  So if it is put in git_header_html, then all\n> views with HTML output (which does not include 'atom' and 'rss' actions,\n> but which actions IIRC have their own handling of '--no-merges')\n> which have support for extra_options would have ability to turn them\n> on and off.\n\nExactly. Although the code as it is now is not really flexible enough\nto handle other cases. I see further down a couple of your ideas would\nmake this kind of thing more extensible.\n\n> What you need to add (if this link is to be in git_header_html) is\n> to create links only when $status == 200, because otherwise the link\n> would be present also for error pages, which I think is wrong.\n\nGood point. That's an easy fix too 8-)\n\n>>    Other options I've considered are:\n>>     + next to pagination (first | prev | next), either before or after\n>>       the existing entries\n>\n> This would fit with the fact that sometimes present \"patches\" link is on\n> the line with pagination links, after pagination links.\n>\n> But this secondary navigation bar is about other views, and extra_options\n> is about modifying current view, and functions more like toggles.  OTOH\n> we have such toggle for 'blame' <-> 'blame_incremental' switch in\n> secondary navigation bar.\n\nWell, at least for this particular option the toggle is closer in\nconcept to pagination (which also modifies the 'current' view, in a\nway).\n\n> Also this would mean that each view type would have to handle extra_options\n> itself, as secondary navigation bar is very much action-specific.  Not\n> that it matters now, with only single '--no-merges' option supported.\n\nEven with more options, I have half an idea on how to refactor this\nhandling in a way that should make it easy for views to handle the\nextra options.\n\n>>     + on mouseover for the table section that refers to the (short)log;\n>>       this would make it possible to put it summary view too, for example\n>\n> This would mean having link inside link, as those headers in summary view\n> functions as link to 'shortlog' view (quite useful I think), and to the\n> project summary in the 'shortlog' view itself (I'm not sure how useful\n> that is).  We already have problems with ref markers being links inside\n> links and having broken layout in some strict XHTML conformance browsers.\n\n(I still think that prohibiting link-in-link is idiotic, but this is\nnot the place where this should be discussed)\n\n> In short: I am not sure what would be the best solution.  Nevertheless\n> I think that link should be more visible, and perhaps more toggle-like.\n\nI'll see if I can cook something up.\n\n>>  * if you toggle merge view when not on the first page, the reference\n>>    (first) commit in the view is likely to change drastically, which\n>>    causes confusion. I have not found a satisfactory solution for this,\n>>    since the obvious way to 'lock' the view (start paginating from the\n>>    current top commit) prevents prev/next navigation\n>\n> Alternate solution would be to clean page (start from 0th page) when\n> changing to '--no-merges'.\n>\n> You would probably need something like 'skip' option, with number of\n> commits, not number of pages to skip, and/or 'skip_to' which takes\n> commit-id.  But i have not thought about this much...\n\nIIRC the log code uses pagination the way it is now because that\nreflects the git log works. This would make a different approach more\ntime consuming on the script part.\n\n>> +\n>> +*:hover > span.merge_toggle a { visibility:visible }\n>\n> I'd rather not have this rule to have different style, i.e. not all\n> in single line.  Unless it is for RFC only...\n\nI can make it more like the others.\n\n>> +     my $can_have_merges = grep(/^$action$/, @{$allowed_options{'--no-merges'}});\n>> +     my $has_merges = !grep(/^--no-merges$/, @extra_options);\n>> +\n>\n> Wouldn't it be better to use straight\n>\n>  +      my $no_merges = grep(/^--no-merges$/, @extra_options);\n>\n> Because $has_merges is true also for example for 'tree' view... which\n> absolutely doesn't make any sense whatsoever.\n\nThe reason why I have two vars is that one checks if we care about the\noption, and the other is to see if it's enabled or not. We don't want\nthe 'show merges' toggle to appear in view which don't handle the\n--no-merge option.\n\n> In more generic solution (which could be perhaps put in a separate commit)\n> it could be:\n>\n>  +     my %extra_options = map { $_ => 0 } keys %$allowed_options;\n>  +     $extra_options{$_} = 1 foreach @extra_options;\n>\n> which means that %extra_options is hash which keys are allowed options,\n> and which has true value for allowed option which is actually used.\n\nI like this approach. It's also easier to extend to other options, if\nand when we'll have them.\n\nI'll try to roll out a new patch with a cleaner design and taking your\nsuggestions into considerations\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"130043","messageId":"200912172214.27001.jnareb@gmail.com","threadId":"21975","inReplyTo":"cb7bb73a0912171241j56ecd2f1y3dc66cf3b86bd784@mail.gmail.com","subject":"Re: [RFC/PATCH] gitweb: link to toggle 'no merges' option","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-12-17T21:14:25Z","receivedAt":"2009-12-17T21:14:25Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n> 2009/12/17 Jakub Narebski <jnareb@gmail.com>:\n>> On Thu, 17 Dec 2009 10:05 +0100, Giuseppe Bilotta wrote:\n\n[...]\n>>> +     my $can_have_merges = grep(/^$action$/, @{$allowed_options{'--no-merges'}});\n>>> +     my $has_merges = !grep(/^--no-merges$/, @extra_options);\n>>> +\n>>\n>> Wouldn't it be better to use straight\n>>\n>>  +      my $no_merges = grep(/^--no-merges$/, @extra_options);\n>>\n>> Because $has_merges is true also for example for 'tree' view... which\n>> absolutely doesn't make any sense whatsoever.\n> \n> The reason why I have two vars is that one checks if we care about the\n> option, and the other is to see if it's enabled or not. We don't want\n> the 'show merges' toggle to appear in view which don't handle the\n> --no-merge option.\n\nPerhaps I didn't made myself clear.\n\nWhat I wanted to ask is to switch $has_merges to $no_merges, not to remove\n$can_have_merges.  Its a question of semantics of $has_merges, which do not\nmean that action has (handles) merges.\n\nThis is of course the question of style, but I think this would make code\nmore maintainable.\n\n\nOf course if you go %extra_options hash route this issue wouldn't matter.\n\n-- \nJakub Narebski\nPoland\n"}]}