| Message ID | cover.1654552560.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | builtin/show-ref.c: support `--count` for limiting output | expand |
Taylor Blau <me@ttaylorr.com> writes: > This short patch series adds support for a new `--count` argument for limiting > the output of `show-ref` (à-la the `for-each-ref` option by the same name). It makes me wonder why we limit this to show-ref. $ git --pipe-to-head-N=3 any-command args... IOW, having to add an option like this feels absurd. > This is useful in contexts where a caller wants to avoid enumerating more > references than necessary (e.g., they only care whether a tag exists, but not > how many or what they are called) but doesn't have control of the output stream > (e.g., they are in Ruby and can't pipe the output to `head -n 1`). Are you saying that Ruby is incapable of run a command line like av[0] = "sh" av[1] = "-c" av[2] = "git show-ref blah | head -n 1" av[3] = NULL ?
On Mon, Jun 06, 2022 at 03:52:19PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > This short patch series adds support for a new `--count` argument for limiting > > the output of `show-ref` (à-la the `for-each-ref` option by the same name). > > It makes me wonder why we limit this to show-ref. > > $ git --pipe-to-head-N=3 any-command args... > > IOW, having to add an option like this feels absurd. I don't disagree. But `--pipe-to-head-N=3` feels like too broad a stroke. This series at least imitates `for-each-ref`'s `--count` option, which makes it feel acceptable to me (if not a little silly). > > This is useful in contexts where a caller wants to avoid enumerating more > > references than necessary (e.g., they only care whether a tag exists, but not > > how many or what they are called) but doesn't have control of the output stream > > (e.g., they are in Ruby and can't pipe the output to `head -n 1`). > > Are you saying that Ruby is incapable of run a command line like > > av[0] = "sh" > av[1] = "-c" > av[2] = "git show-ref blah | head -n 1" > av[3] = NULL No, Ruby is perfectly capable of doing that. But it involves an extra process (two, if `head` isn't a shell builtin) and the additional overhead of creating a pipe and passing data through it instead of writing directly to stdout. That isn't a complete show-stopper in most cases, but in ultra-latency-sensitive applications like GitHub is using show-ref for, being able to shave an extra process or so off matters. If you're strongly opposed to having `show-ref` match `for-each-ref`'s `--count` option, I won't be too sad. But I'm not in a huge rush to replace this series with `git --pipe-to-head-N=<n>` either, FWIW. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > If you're strongly opposed to having `show-ref` match `for-each-ref`'s > `--count` option, I won't be too sad. But I'm not in a huge rush to > replace this series with `git --pipe-to-head-N=<n>` either, FWIW. Heh, to me "git --pipe-to-head-N=<n>" smells equally absurd, too ;-)
On Mon, Jun 06 2022, Taylor Blau wrote: > On Mon, Jun 06, 2022 at 03:52:19PM -0700, Junio C Hamano wrote: >> Taylor Blau <me@ttaylorr.com> writes: >> >> > This short patch series adds support for a new `--count` argument for limiting >> > the output of `show-ref` (à-la the `for-each-ref` option by the same name). >> >> It makes me wonder why we limit this to show-ref. >> >> $ git --pipe-to-head-N=3 any-command args... >> >> IOW, having to add an option like this feels absurd. > > I don't disagree. But `--pipe-to-head-N=3` feels like too broad a > stroke. This series at least imitates `for-each-ref`'s `--count` > option, which makes it feel acceptable to me (if not a little silly). Yeah, although I do think it's worthwhile to think about where certain UX decisions are leading us, i.e. the logical conclusion here is to have every command that emits >1 lines support --count, which as your patch here shows needs special support, and even in your case you haven't implemented it in a way that's compatible with all existing options. B.t.w. why would a --count for --verify not just by supported have these be equivalent: # same git tag --count=3 --verify <name> git tag --verify <name> | head -n 3 >> > This is useful in contexts where a caller wants to avoid enumerating more >> > references than necessary (e.g., they only care whether a tag exists, but not >> > how many or what they are called) but doesn't have control of the output stream >> > (e.g., they are in Ruby and can't pipe the output to `head -n 1`). >> >> Are you saying that Ruby is incapable of run a command line like >> >> av[0] = "sh" >> av[1] = "-c" >> av[2] = "git show-ref blah | head -n 1" >> av[3] = NULL > > No, Ruby is perfectly capable of doing that. But it involves an extra > process (two, if `head` isn't a shell builtin) [...] Maybe this really is a limitation of ruby, or maybe I'm missing something, but doesn't it support just opening a process without "sh -c" and piping the output to your current process, as this perl command which makes use of execve() will do: $ perl -Mautodie=:all -wE ' my $i = 0; my $lim = shift; open my $fh, "-|", qw(git show-ref master); while (<$fh>) { last if $i++ >= $lim; print "$.: $_"; };' 10 Some quick searching for docs online suggests that Ruby's Open3 and/or Process.spawn might be the equivalent. Note that if you replace that qw(git show-ref master) with e.g.: "git show-ref master | tail -n 20" That you'll get 3x execve(), one sh -c, another for "git" and another for "tail", but in the first case you'll only get the execve(). Isn't that something that would make this workaround unnecessary? Well, maybe not because... > [...]and the additional > overhead of creating a pipe and passing data through it instead of > writing directly to stdout. It wouldn't take care of this part, but I'm struggling to think of cases where you'd be running this in the context of github.com and not already need to capture the output of the command. I.e. surely you're already piping stdout/stderr into your program, no?
On 6/6/2022 7:00 PM, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > >> If you're strongly opposed to having `show-ref` match `for-each-ref`'s >> `--count` option, I won't be too sad. But I'm not in a huge rush to >> replace this series with `git --pipe-to-head-N=<n>` either, FWIW. > > Heh, to me "git --pipe-to-head-N=<n>" smells equally absurd, too ;-) I wonder if we could teach Git to skip an extra process if we see git -c "core.pager=head -n=<N>" ... ? Thanks, -Stolee
On Tue, Jun 07, 2022 at 10:18:56AM +0200, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Jun 06 2022, Taylor Blau wrote: > > > On Mon, Jun 06, 2022 at 03:52:19PM -0700, Junio C Hamano wrote: > >> Taylor Blau <me@ttaylorr.com> writes: > >> > >> > This short patch series adds support for a new `--count` argument for limiting > >> > the output of `show-ref` (à-la the `for-each-ref` option by the same name). > >> > >> It makes me wonder why we limit this to show-ref. > >> > >> $ git --pipe-to-head-N=3 any-command args... > >> > >> IOW, having to add an option like this feels absurd. > > > > I don't disagree. But `--pipe-to-head-N=3` feels like too broad a > > stroke. This series at least imitates `for-each-ref`'s `--count` > > option, which makes it feel acceptable to me (if not a little silly). > > Yeah, although I do think it's worthwhile to think about where certain > UX decisions are leading us, i.e. the logical conclusion here is to have > every command that emits >1 lines support --count, which as your patch > here shows needs special support, and even in your case you haven't > implemented it in a way that's compatible with all existing options. To be clear, I don't think adding `--count` to every command is a good idea. But it exists in `for-each-ref`, and not in `show-ref`, and this series rectifies that gap in functionality. Perhaps `for-each-ref` shouldn't have `--count`, but it does, and has since that command's inception. > B.t.w. why would a --count for --verify not just by supported have these > be equivalent: > > # same > git tag --count=3 --verify <name> > git tag --verify <name> | head -n 3 (I'm not sure if you meant "git tag" here versus "git show-ref", but either way), `show-ref` in `--verify` mode outputs one line of output per line of input, so a caller can easily limit the output by limiting the input. > >> > This is useful in contexts where a caller wants to avoid enumerating more > >> > references than necessary (e.g., they only care whether a tag exists, but not > >> > how many or what they are called) but doesn't have control of the output stream > >> > (e.g., they are in Ruby and can't pipe the output to `head -n 1`). > >> > >> Are you saying that Ruby is incapable of run a command line like > >> > >> av[0] = "sh" > >> av[1] = "-c" > >> av[2] = "git show-ref blah | head -n 1" > >> av[3] = NULL > > > > No, Ruby is perfectly capable of doing that. But it involves an extra > > process (two, if `head` isn't a shell builtin) [...] > > Maybe this really is a limitation of ruby, or maybe I'm missing > something, but doesn't it support just opening a process without "sh -c" > and piping the output to your current process, as this perl command > which makes use of execve() will do: > > $ perl -Mautodie=:all -wE ' > my $i = 0; my $lim = shift; > open my $fh, "-|", qw(git show-ref master); > while (<$fh>) { > last if $i++ >= $lim; > print "$.: $_"; > };' 10 > > Some quick searching for docs online suggests that Ruby's Open3 and/or > Process.spawn might be the equivalent. To be clear, Ruby _does_ support something similar to what you demonstrated in Perl above, it just isn't easily accessible to our current infrastructure for spawning Git commands. > Isn't that something that would make this workaround unnecessary? Well, > maybe not because... > > > [...]and the additional > > overhead of creating a pipe and passing data through it instead of > > writing directly to stdout. > > It wouldn't take care of this part, but I'm struggling to think of cases > where you'd be running this in the context of github.com and not already > need to capture the output of the command. I.e. surely you're already > piping stdout/stderr into your program, no? Right, there's already a pipe in place to capture the output, but here I'm talking about an _additional_ pipe to feed `show-ref` first through to `head` and _then_ back out to the buffer in the calling Ruby program. Thanks, Taylor