Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index

[Taylor Blau <me@ttaylorr.com>]

Hi Thomas,

On Fri, Sep 18, 2020 at 12:34:48PM -0400, Thomas Guyot-Sionnest wrote:
> Hi Taylor,
>
> On Fri, 18 Sep 2020 at 10:36, Taylor Blau <me@ttaylorr.com> wrote:
> > On Fri, Sep 18, 2020 at 07:32:56AM -0400, Thomas Guyot-Sionnest wrote:
> > > A very handy way to pass data to applications is to use the <() process
> > > substitution syntax in bash variants. It allow comparing files streamed
> > > from a remote server or doing on-the-fly stream processing to alter the
> > > diff. These are usually implemented as a symlink that points to a bogus
> > > name (ex "pipe:[209326419]") but opens as a pipe.
> >
> > This is true in bash, but sh does not support process substitution with
> > <().
>
> Bash, ksh, zsh and likely any more moden shell. Other programming
> languages also setup such pipes. It's much cleaner than creating temp
> files and cleaning them up and in some cases faster too (I've ran
> diff's like this over GB's of test data, it's very handy to remove
> known patterns that would cause needless diffs).

Oh, yes, I definitely agree that it will be useful for callers who use
more modern shells. I was making sure that we would still be able to run
this in the test suite (for us, that means making a new file that
sources lib-bash and tests only in environments where bash is
supported).

> Now you mention it, maybe I could do that stat first, rule this out
> from the beginning... less work for the general case.
>
> *untested*:
>
>     if (!lstat(name, &st)) {
>         if (!S_ISLNK(st.st_mode))
>             return(0);
>         if (!stat(name, &st)) {
>             if (!S_ISFIFO(st.st_mode))
>                 return(0);

I still don't think I quite understand why FIFOs aren't allowed...
>
>             /* We have a symlink that points to a pipe. If it's resolved
>              * target doesn't really exist we can safely assume it's a
>              * special file and use it */
>             struct strbuf sb = STRBUF_INIT;
>             strbuf_realpath(&sb, name, 0);
>             /* We're abusing strbuf_realpath here, it may append
>              * pipe:[NNNNNNNNN] to an abs path */
>             if (stat(sb.buf, &st))
>                 return(1); /* stat failed, special one */

By the time that I got to this point I think that what you wrote is much
easier to follow. Thanks.

>         }
>     }
>     return(0);
>
> TL;DR - the conditions we need:
>
> - lstat(name) == 0  // name exists
> - islink(lstat(name))  // name is a symlink
> - stat(name) == 0  // target of name is reachable
> - isfifo(stat(name))  // Target of name is a fifo
> - stat(realpath(readlink(name))) != 0  // Although we can reach it,
> name's destination doesn't actually exist.
>
> BTW is st/sb too confusing ? I took examples elsewhere in the code, I
> can rename them if it's easier to read.

No, it's fine. I think anecdotally I read 'struct strbuf buf' more than
I read 'struct strbuf sb', but I guess that's just the areas of code
that I happen to frequent, since some quick grepping around shows 462
uses of 'sb' followed by 425 uses of 'buf' (the next most common names
are 'err' and 'out', with 191 and 121 uses, respectively).

> > > +test_expect_success 'diff --no-index finds diff in piped subshells' '
> > > +     (
> > > +             set -- <(cat /dev/null) <(cat /dev/null)
>
> Precautions/portability. The file names are somewhat dynamic (at least
> the fd part...) this is to be sure I capture the names of the pipes
> that will be used (assuming the fd's will be reallocated in the same
> order which I think is fairly safe). An alternative is to sed "actual"
> to remove known variables, but then I hope it would be reliable (and
> can I use sed -r?). IIRC earlier versions of bash or on some systems a
> temp file could be used for these - although it defeats the purpose
> it's not a reason to fail....

OK.

> I cannot develop this on other systems but I tested the pipe names on
> Windows and Sunos, and also using ksh and zsh on Linux (zsh uses /proc
> directly, kss uses lower fd's which means it can easily clash with
> scripts if you don't use named fd's, but not our problem....)
>
> Thanks,
>
> Thomas

Thanks,
Taylor