| Message ID | 20200409103541.23743-1-martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config.txt: move closing "----" to cover entire listing | expand |
On Thu, Apr 09, 2020 at 12:35:41PM +0200, Martin Ågren wrote: > Commit 1925fe0c8a ("Documentation: wrap config listings in "----"", > 2019-09-07) wrapped this fairly large block of example config directives > in "----". The closing "----" ended up a few lines too early though. > Make sure to include the trailing "IncludeIf.onbranch:..." example, too. > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > Not sure how I managed to botch this in 1925fe0c8a. I managed to botch the review, as well. :) This looks good to me. Some observations below. > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 2450589a0e..74009d5402 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -220,12 +220,12 @@ Example > ; affected by the condition > [includeIf "gitdir:/path/to/group/"] > path = foo.inc > ----- > > - ; include only if we are in a worktree where foo-branch is > - ; currently checked out > - [includeIf "onbranch:foo-branch"] > - path = foo.inc > +; include only if we are in a worktree where foo-branch is > +; currently checked out > +[includeIf "onbranch:foo-branch"] > + path = foo.inc > +---- I had to stare at this for a moment before realizing that the "-----" is not 5 dashes in context, but the removal of the old, misplaced 4-dash line. I checked it with doc-diff, but was surprised to find no change. That's because the manpage shows it the same either way (the indented chunk is just a different example, but two examples back to back render the same as a single one). But you can see the difference in the HTML version, where the final example isn't in the grey box. That explains why I didn't see the issue when running doc-diff on the original bug. I wonder if we could teach doc-diff to look at the HTML, too. I'm not sure how, though. Certainly html2text or similar would get us something diff-able, but without the visual elements (like the grey box), I don't know that it's much more valuable than the manpages. -Peff
On Thu, 9 Apr 2020 at 16:14, Jeff King <peff@peff.net> wrote: > > On Thu, Apr 09, 2020 at 12:35:41PM +0200, Martin Ågren wrote: > > > Not sure how I managed to botch this in 1925fe0c8a. > > I managed to botch the review, as well. :) :) > I checked it with doc-diff, but was surprised to find no change. That's > because the manpage shows it the same either way (the indented chunk is > just a different example, but two examples back to back render the same > as a single one). But you can see the difference in the HTML version, > where the final example isn't in the grey box. Ah, you're using AsciiDoc. With Asciidoctor, there is a change in indentation of the "path = foo.inc" line with this new, proposed patch. The original commit reduced the number of occurrences of such AsciiDoc/tor differences around this spot, but failed to bring the number all the way down to zero. Now, finally, that discrepancy will be fixed. > That explains why I didn't see the issue when running doc-diff on the > original bug. I wonder if we could teach doc-diff to look at the HTML, > too. I'm not sure how, though. Certainly html2text or similar would get > us something diff-able, but without the visual elements (like the grey > box), I don't know that it's much more valuable than the manpages. At one point I considered trying out diffoscope for this. It should allegedly be good at comparing "everything". But being good at everything, it wanted to pull in a discouragingly large number of dependencies, so I never actually tried it out. It doesn't explicitly claim to know html or manpages (but does mention xml and pdf), so I dunno. Martin
On Thu, Apr 09, 2020 at 05:00:34PM +0200, Martin Ågren wrote: > > That explains why I didn't see the issue when running doc-diff on the > > original bug. I wonder if we could teach doc-diff to look at the HTML, > > too. I'm not sure how, though. Certainly html2text or similar would get > > us something diff-able, but without the visual elements (like the grey > > box), I don't know that it's much more valuable than the manpages. > > At one point I considered trying out diffoscope for this. It should > allegedly be good at comparing "everything". But being good at > everything, it wanted to pull in a discouragingly large number of > dependencies, so I never actually tried it out. It doesn't explicitly > claim to know html or manpages (but does mention xml and pdf), so I > dunno. I tried it just now, and it's not that clever. A regular "diff -r" of the before and after HTML yields what you'd expect: --- old/git-config.html 2020-04-09 11:38:19.312436125 -0400 +++ new/git-config.html 2020-04-09 11:38:40.028385850 -0400 @@ -1678,11 +1678,9 @@ ; file (if the condition is true); their location is not ; affected by the condition [includeIf "gitdir:/path/to/group/"] - path = foo.inc</code></pre> -</div></div> -<div class="literalblock"> -<div class="content"> -<pre><code>; include only if we are in a worktree where foo-branch is + path = foo.inc + +; include only if we are in a worktree where foo-branch is ; currently checked out [includeIf "onbranch:foo-branch"] path = foo.inc</code></pre> A diffoscope diff yields the same, plus it complains about differing timestamps on all of the files. I don't think it's doing anything clever with respect to HTML formatting. -Peff
On Thu, 9 Apr 2020 at 17:52, Jeff King <peff@peff.net> wrote: > > On Thu, Apr 09, 2020 at 05:00:34PM +0200, Martin Ågren wrote: > > > At one point I considered trying out diffoscope for this. It should > > allegedly be good at comparing "everything". But being good at > > everything, it wanted to pull in a discouragingly large number of > > dependencies, so I never actually tried it out. It doesn't explicitly > > claim to know html or manpages (but does mention xml and pdf), so I > > dunno. > > I tried it just now, and it's not that clever. A regular "diff -r" of > the before and after HTML yields what you'd expect: > > A diffoscope diff yields the same, plus it complains about differing > timestamps on all of the files. I don't think it's doing anything clever > with respect to HTML formatting. I see, thanks for trying it out. Martin
diff --git a/Documentation/config.txt b/Documentation/config.txt index 2450589a0e..74009d5402 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -220,12 +220,12 @@ Example ; affected by the condition [includeIf "gitdir:/path/to/group/"] path = foo.inc ----- - ; include only if we are in a worktree where foo-branch is - ; currently checked out - [includeIf "onbranch:foo-branch"] - path = foo.inc +; include only if we are in a worktree where foo-branch is +; currently checked out +[includeIf "onbranch:foo-branch"] + path = foo.inc +---- Values ~~~~~~
Commit 1925fe0c8a ("Documentation: wrap config listings in "----"", 2019-09-07) wrapped this fairly large block of example config directives in "----". The closing "----" ended up a few lines too early though. Make sure to include the trailing "IncludeIf.onbranch:..." example, too. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- Not sure how I managed to botch this in 1925fe0c8a. Documentation/config.txt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)