{"thread":{"id":"53199","subject":"[PATCH] config.txt: move closing \"----\" to cover entire listing","startedAt":"2020-04-09T10:36:15Z","lastAt":"2020-04-10T07:09:28Z","messageCount":5,"participants":["Martin Ågren","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"395113","messageId":"20200409103541.23743-1-martin.agren@gmail.com","threadId":"53199","inReplyTo":null,"subject":"[PATCH] config.txt: move closing \"----\" to cover entire listing","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-09T10:35:41Z","receivedAt":"2020-04-09T10:36:15Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit 1925fe0c8a (\"Documentation: wrap config listings in \"----\"\",\n2019-09-07) wrapped this fairly large block of example config directives\nin \"----\". The closing \"----\" ended up a few lines too early though.\nMake sure to include the trailing \"IncludeIf.onbranch:...\" example, too.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Not sure how I managed to botch this in 1925fe0c8a.\n\n Documentation/config.txt | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2450589a0e..74009d5402 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -220,12 +220,12 @@ Example\n ; affected by the condition\n [includeIf \"gitdir:/path/to/group/\"]\n \tpath = foo.inc\n-----\n \n-\t; include only if we are in a worktree where foo-branch is\n-\t; currently checked out\n-\t[includeIf \"onbranch:foo-branch\"]\n-\t\tpath = foo.inc\n+; include only if we are in a worktree where foo-branch is\n+; currently checked out\n+[includeIf \"onbranch:foo-branch\"]\n+\tpath = foo.inc\n+----\n \n Values\n ~~~~~~\n-- \n2.26.0\n\n"},{"id":"395124","messageId":"20200409141452.GC3494212@coredump.intra.peff.net","threadId":"53199","inReplyTo":"20200409103541.23743-1-martin.agren@gmail.com","subject":"Re: [PATCH] config.txt: move closing \"----\" to cover entire listing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-09T14:14:52Z","receivedAt":"2020-04-09T14:14:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 09, 2020 at 12:35:41PM +0200, Martin Ågren wrote:\n\n> Commit 1925fe0c8a (\"Documentation: wrap config listings in \"----\"\",\n> 2019-09-07) wrapped this fairly large block of example config directives\n> in \"----\". The closing \"----\" ended up a few lines too early though.\n> Make sure to include the trailing \"IncludeIf.onbranch:...\" example, too.\n> \n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  Not sure how I managed to botch this in 1925fe0c8a.\n\nI managed to botch the review, as well. :)\n\nThis looks good to me. Some observations below.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 2450589a0e..74009d5402 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -220,12 +220,12 @@ Example\n>  ; affected by the condition\n>  [includeIf \"gitdir:/path/to/group/\"]\n>  \tpath = foo.inc\n> -----\n>  \n> -\t; include only if we are in a worktree where foo-branch is\n> -\t; currently checked out\n> -\t[includeIf \"onbranch:foo-branch\"]\n> -\t\tpath = foo.inc\n> +; include only if we are in a worktree where foo-branch is\n> +; currently checked out\n> +[includeIf \"onbranch:foo-branch\"]\n> +\tpath = foo.inc\n> +----\n\nI had to stare at this for a moment before realizing that the \"-----\" is\nnot 5 dashes in context, but the removal of the old, misplaced 4-dash\nline.\n\nI checked it with doc-diff, but was surprised to find no change. That's\nbecause the manpage shows it the same either way (the indented chunk is\njust a different example, but two examples back to back render the same\nas a single one). But you can see the difference in the HTML version,\nwhere the final example isn't in the grey box.\n\nThat explains why I didn't see the issue when running doc-diff on the\noriginal bug. I wonder if we could teach doc-diff to look at the HTML,\ntoo. I'm not sure how, though. Certainly html2text or similar would get\nus something diff-able, but without the visual elements (like the grey\nbox), I don't know that it's much more valuable than the manpages.\n\n-Peff\n"},{"id":"395128","messageId":"CAN0heSp3=8fTGb5Ciuvr8+EevNSrAfskvrrS_PN=VPhKJyB5jA@mail.gmail.com","threadId":"53199","inReplyTo":"20200409141452.GC3494212@coredump.intra.peff.net","subject":"Re: [PATCH] config.txt: move closing \"----\" to cover entire listing","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-09T15:00:34Z","receivedAt":"2020-04-09T15:00:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 9 Apr 2020 at 16:14, Jeff King <peff@peff.net> wrote:\n>\n> On Thu, Apr 09, 2020 at 12:35:41PM +0200, Martin Ågren wrote:\n>\n> >  Not sure how I managed to botch this in 1925fe0c8a.\n>\n> I managed to botch the review, as well. :)\n\n:)\n\n> I checked it with doc-diff, but was surprised to find no change. That's\n> because the manpage shows it the same either way (the indented chunk is\n> just a different example, but two examples back to back render the same\n> as a single one). But you can see the difference in the HTML version,\n> where the final example isn't in the grey box.\n\nAh, you're using AsciiDoc. With Asciidoctor, there is a change in\nindentation of the \"path = foo.inc\" line with this new, proposed patch.\nThe original commit reduced the number of occurrences of such\nAsciiDoc/tor differences around this spot, but failed to bring the\nnumber all the way down to zero. Now, finally, that discrepancy will be\nfixed.\n\n> That explains why I didn't see the issue when running doc-diff on the\n> original bug. I wonder if we could teach doc-diff to look at the HTML,\n> too. I'm not sure how, though. Certainly html2text or similar would get\n> us something diff-able, but without the visual elements (like the grey\n> box), I don't know that it's much more valuable than the manpages.\n\nAt one point I considered trying out diffoscope for this. It should\nallegedly be good at comparing \"everything\". But being good at\neverything, it wanted to pull in a discouragingly large number of\ndependencies, so I never actually tried it out. It doesn't explicitly\nclaim to know html or manpages (but does mention xml and pdf), so I\ndunno.\n\nMartin\n"},{"id":"395130","messageId":"20200409155159.GA4129050@coredump.intra.peff.net","threadId":"53199","inReplyTo":"CAN0heSp3=8fTGb5Ciuvr8+EevNSrAfskvrrS_PN=VPhKJyB5jA@mail.gmail.com","subject":"Re: [PATCH] config.txt: move closing \"----\" to cover entire listing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-09T15:51:59Z","receivedAt":"2020-04-09T15:52:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 09, 2020 at 05:00:34PM +0200, Martin Ågren wrote:\n\n> > That explains why I didn't see the issue when running doc-diff on the\n> > original bug. I wonder if we could teach doc-diff to look at the HTML,\n> > too. I'm not sure how, though. Certainly html2text or similar would get\n> > us something diff-able, but without the visual elements (like the grey\n> > box), I don't know that it's much more valuable than the manpages.\n> \n> At one point I considered trying out diffoscope for this. It should\n> allegedly be good at comparing \"everything\". But being good at\n> everything, it wanted to pull in a discouragingly large number of\n> dependencies, so I never actually tried it out. It doesn't explicitly\n> claim to know html or manpages (but does mention xml and pdf), so I\n> dunno.\n\nI tried it just now, and it's not that clever. A regular \"diff -r\" of\nthe before and after HTML yields what you'd expect:\n\n--- old/git-config.html\t2020-04-09 11:38:19.312436125 -0400\n+++ new/git-config.html\t2020-04-09 11:38:40.028385850 -0400\n@@ -1678,11 +1678,9 @@\n ; file (if the condition is true); their location is not\n ; affected by the condition\n [includeIf \"gitdir:/path/to/group/\"]\n-        path = foo.inc</code></pre>\n-</div></div>\n-<div class=\"literalblock\">\n-<div class=\"content\">\n-<pre><code>; include only if we are in a worktree where foo-branch is\n+        path = foo.inc\n+\n+; include only if we are in a worktree where foo-branch is\n ; currently checked out\n [includeIf \"onbranch:foo-branch\"]\n         path = foo.inc</code></pre>\n\nA diffoscope diff yields the same, plus it complains about differing\ntimestamps on all of the files. I don't think it's doing anything clever\nwith respect to HTML formatting.\n\n-Peff\n"},{"id":"395170","messageId":"CAN0heSqF2minprAb2Ss-rE94cWngm7-A-E8uB8XuOnu8SGz_Qg@mail.gmail.com","threadId":"53199","inReplyTo":"20200409155159.GA4129050@coredump.intra.peff.net","subject":"Re: [PATCH] config.txt: move closing \"----\" to cover entire listing","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-10T07:09:13Z","receivedAt":"2020-04-10T07:09:28Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 9 Apr 2020 at 17:52, Jeff King <peff@peff.net> wrote:\n>\n> On Thu, Apr 09, 2020 at 05:00:34PM +0200, Martin Ågren wrote:\n>\n> > At one point I considered trying out diffoscope for this. It should\n> > allegedly be good at comparing \"everything\". But being good at\n> > everything, it wanted to pull in a discouragingly large number of\n> > dependencies, so I never actually tried it out. It doesn't explicitly\n> > claim to know html or manpages (but does mention xml and pdf), so I\n> > dunno.\n>\n> I tried it just now, and it's not that clever. A regular \"diff -r\" of\n> the before and after HTML yields what you'd expect:\n>\n> A diffoscope diff yields the same, plus it complains about differing\n> timestamps on all of the files. I don't think it's doing anything clever\n> with respect to HTML formatting.\n\nI see, thanks for trying it out.\n\nMartin\n"}]}