Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]

Hi Peff and Martin,

On Tue, 5 Feb 2019, Jeff King wrote:

> On Mon, Feb 04, 2019 at 09:50:37PM +0100, Martin Ågren wrote:
> 
> > `usage` tries to call $0, which might very well be "./doc-diff", so if
> > we `cd_to_toplevel` before calling `usage`, we'll end with an error to
> > the effect of "./doc-diff: not found" rather than a friendly `doc-diff
> > -h` output. This regressed in ad51743007 ("doc-diff: add --clean mode to
> > remove temporary working gunk", 2018-08-31) where we moved the call to
> > `cd_to_toplevel` to much earlier.
> > 
> > A general fix might be to teach git-sh-setup to save away the absolute
> > path for $0 and then use that, instead. I'm not aware of any portable
> > way of doing that, see, e.g., d2addc3b96 ("t7800: readlink may not be
> > available", 2016-05-31).
> > 
> > An early version of this patch moved `cd_to_toplevel` back to where it
> > was before ad51743007 and taught the "--clean" code to cd on its own.
> > But let's try instead to get rid of the cd-ing entirely. We don't really
> > need it and we can work with absolute paths instead. There's just one
> > use of $PWD that we need to adjust by simply dropping it.
> 
> Thanks, this version looks great to me!

Peff, you asked at the Contributors' Summit for a way to get notified when
CI fails for your patch, and I was hesitant to add it (even if it would be
straight-forward, really) because of the false positives.

This is one such example, as the test fails:

	https://dev.azure.com/gitgitgadget/git/_build/results?buildId=944

In particular, the tests t2024 and t5552 are broken for
ma/doc-diff-usage-fix on Windows. The reason seems to be that those are
already broken for the base commit that Junio picked:
jk/diff-rendered-docs (actually, not the tip of it, but the commit fixed
by Martin's patch).

Of course, I understand why Junio picks base commits that are far, far in
the past (and have regressions that current `master` does not have), it
makes sense from the point of view where the fixes should be as close to
the commits they fix.  The downside is that we cannot automate regression
testing more than we do now, with e.g. myself acting as curator of test
failures.

Ciao,
Dscho
Ciao,