Support for --stdin-paths in commit, add, etc

Support for --stdin-paths in commit, add, etc

[Alexandr Miloslavskiy]

Hello,

Hello,

This is a follow-up for previous discussion [1].

In our git UI, we sometimes run into OS commandline length limit when
trying to specify a list of pathspecs. For example, this happens when
user selects a large set of files and wants to commit them. As a
workaround, we split files and issue multiple git commands.

This has multiple disadvantages:
1) Some commands are a lot slower this way. For example, commit-amend
    can take 45 minutes instead of 30 seconds, because git will do a lot
    of unnecessary work.
2) The operation is no longer completed in one "transaction".
3) This makes our code more complicated.

Our suggestion is to change commands such as 'commit', 'add', etc to
also work with --stdin-paths. If a command already supports stdin for
any purpose, then trying to use both options will return an error.

We're ready to develop the necessary patches to make this possible.

[1] 
https://public-inbox.org/git/c3be6eff-365b-96b8-16d2-0528612fc1fc@syntevo.com/T/#u

Re: Support for --stdin-paths in commit, add, etc

[Junio C Hamano]

Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> writes:

> In our git UI, we sometimes run into OS commandline length limit when
> trying to specify a list of pathspecs. For example, this happens when
> user selects a large set of files and wants to commit them. As a
> workaround, we split files and issue multiple git commands.

Yes, for plumbing commands meant for scripting, we really should
strive to give scriptors way(s) to avoid the command line limit,
and --stdin-paths is a very good approach to do so.

Re: Support for --stdin-paths in commit, add, etc

[Jeff King]

On Wed, Jul 31, 2019 at 05:45:12PM +0200, Alexandr Miloslavskiy wrote:

> In our git UI, we sometimes run into OS commandline length limit when
> trying to specify a list of pathspecs. For example, this happens when
> user selects a large set of files and wants to commit them. As a
> workaround, we split files and issue multiple git commands.
> 
> This has multiple disadvantages:
> 1) Some commands are a lot slower this way. For example, commit-amend
>    can take 45 minutes instead of 30 seconds, because git will do a lot
>    of unnecessary work.
> 2) The operation is no longer completed in one "transaction".
> 3) This makes our code more complicated.
> 
> Our suggestion is to change commands such as 'commit', 'add', etc to
> also work with --stdin-paths. If a command already supports stdin for
> any purpose, then trying to use both options will return an error.

I don't have any real objection to adding stdin support for more
commands. Bu tin the specific case you're discussing, it seems like
using "git update-index" might already solve your problem. It's the
intended plumbing for scripted index updates, and it already supports
receiving paths from stdin.

-Peff

Re: Support for --stdin-paths in commit, add, etc

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... But in the specific case you're discussing, it seems like
> using "git update-index" might already solve your problem. It's the
> intended plumbing for scripted index updates, and it already supports
> receiving paths from stdin.

Sounds good.

Re: Support for --stdin-paths in commit, add, etc

[Alexandr Miloslavskiy]

On 31.07.2019 19:19, Jeff King wrote:
> I don't have any real objection to adding stdin support for more
> commands. Bu tin the specific case you're discussing, it seems like
> using "git update-index" might already solve your problem. It's the
> intended plumbing for scripted index updates, and it already supports
> receiving paths from stdin.

I have now studied which git commands already use commandline splitting
in our application. For some of them, I didn't find comparable plumbing;
for others, I feel that a lot of new edge cases will arise, and it will
need a lot of testing to make sure things work as expected.

Therefore, for us it would be best if high-level commands also accepted
--stdin-paths. If I develop good enough patches for that, will you
accept them?

We're interested in these high-level commands:
1) git add
2) git rm
3) git checkout
4) git reset
5) git stash
6) git commit

Here's the list of detailed commands and plumbings I found:
01) git add
     'git update-index' doesn't seem to be able to skip ignored files.
02) git add --force
     Probably 'git update-index --add --stdin'.
03) git checkout
     Probably 'git checkout-index --stdin'
04) git checkout HEAD
     Didn't find a plumbing to only affect named paths.
05) git checkout --ours
     Probably 'git checkout-index --stage=2 --force --stdin'
06) git checkout --theirs
     Probably 'git checkout-index --stage=3 --force --stdin'
07) git rm [--force] [--cached]
     Probably 'git update-index --force-remove'
     Didn't find how to delete files from working tree.
08) git reset -q HEAD
     Didn't find a plumbing to only affect named paths.
09) git add --update
     Probably 'git update-index --again --add --stdin'
     Not sure that --again is good replacement.
10) git stash push [--keep-index] [--include-untracked] [--message]
     Didn't find plumbing for stashes.
11) git commit [--allow-empty] [--amend] [--signoff] [--no-verify]
       --file=CommitMessage.txt -o
     Didn't find a plumbing to only affect named staged paths.

Re: Support for --stdin-paths in commit, add, etc

[René Scharfe]

Am 31.07.19 um 17:45 schrieb Alexandr Miloslavskiy:
> Our suggestion is to change commands such as 'commit', 'add', etc to
> also work with --stdin-paths. If a command already supports stdin for
> any purpose, then trying to use both options will return an error.

Would it make sense to have a --paths-file parameter instead that allows
reading paths from a given file and honors the convention of reading
from stdin with the special argument "-"?  Reading from stdin would
still only work for one parameter at a time, but paths could also be
passed via a regular file or named pipe, allowing for more flexibility.

René

Re: Support for --stdin-paths in commit, add, etc

[Alexandr Miloslavskiy]

On 01.08.2019 16:26, René Scharfe wrote:
> Would it make sense to have a --paths-file parameter instead

Both approaches (stdin or file) will work well for us. File sounds
easier from programming perspective. However, in previous discussion, 
there was some concern about possible security problems with this approach.

Re: Support for --stdin-paths in commit, add, etc

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Would it make sense to have a --paths-file parameter instead that allows
> reading paths from a given file and honors the convention of reading
> from stdin with the special argument "-"?  Reading from stdin would
> still only work for one parameter at a time, but paths could also be
> passed via a regular file or named pipe, allowing for more flexibility.

The command's primary function itself may want to use the standard
input stream to read things other than the list of paths; if we are
not limiting ourselves to plumbing (i.e. doing one thing well), the
"--stdin-paths" may not be a general solution that is good enough.

So, reading paths from a file (which could be "-" as you suggest)
would be a good solution for that.

Re: Support for --stdin-paths in commit, add, etc

[Alexandr Miloslavskiy]

On 01.08.2019 17:56, Junio C Hamano wrote:
> So, reading paths from a file (which could be "-" as you suggest)
> would be a good solution for that.

To summarize:
1) Implement --paths-file for high-level commands.
2) '--paths-file -' would mean reading from stdin.

Is that something you're ready to accept patches for?

Re: Support for --stdin-paths in commit, add, etc

[Phillip Wood]

On 01/08/2019 14:25, Alexandr Miloslavskiy wrote:
> On 31.07.2019 19:19, Jeff King wrote:
>> I don't have any real objection to adding stdin support for more
>> commands. Bu tin the specific case you're discussing, it seems like
>> using "git update-index" might already solve your problem. It's the
>> intended plumbing for scripted index updates, and it already supports
>> receiving paths from stdin.
> 
> I have now studied which git commands already use commandline splitting
> in our application. For some of them, I didn't find comparable plumbing;
> for others, I feel that a lot of new edge cases will arise, and it will
> need a lot of testing to make sure things work as expected.
> 
> Therefore, for us it would be best if high-level commands also accepted
> --stdin-paths. If I develop good enough patches for that, will you
> accept them?
> 
> We're interested in these high-level commands:
> 1) git add
> 2) git rm
> 3) git checkout
> 4) git reset
> 5) git stash
> 6) git commit
> 
> Here's the list of detailed commands and plumbings I found:
> 01) git add
>      'git update-index' doesn't seem to be able to skip ignored files.

No but it only takes paths not pathspecs, can you filter out the ignored 
paths first? From a UI point of view it would be better not to allow 
users to select ignored files if you don't want to be able to add them. 
If you want to use a pathspec then you can do 'git ls-files 
--exclude-standard -o -z <pathspec ...> | git update-index --add -z --stdin'
git check-ignore --stdin should help if you have a list of paths and you 
want to remove the ignored ones (it's not great as it tells you which 
ones are ignored rather than filtering the list for you)

> 02) git add --force
>      Probably 'git update-index --add --stdin'.
> 03) git checkout
>      Probably 'git checkout-index --stdin'
> 04) git checkout HEAD
>      Didn't find a plumbing to only affect named paths.

You can use a temporary index and do
GIT_INDEX_FILE=$tmp_index git read-tree HEAD && 
GIT_INDEX_FILE=$tmp_index git checkout-index --stdin &&
rm $tmp_index

> 05) git checkout --ours
>      Probably 'git checkout-index --stage=2 --force --stdin'
> 06) git checkout --theirs
>      Probably 'git checkout-index --stage=3 --force --stdin'
> 07) git rm [--force] [--cached]
>      Probably 'git update-index --force-remove'
>      Didn't find how to delete files from working tree.

You have to delete them yourself.

> 08) git reset -q HEAD
>      Didn't find a plumbing to only affect named paths.

It's a bit of a pain but you can use 'git update-index -q --unmerged 
--refresh && git diff-index --cached -z HEAD' to get a list of paths in 
the index that differ from HEAD. The list contains the old oid for each 
path (see the man page for the format) and you can feed those into 'git 
update-index --index-info' to update the index.

> 09) git add --update
>      Probably 'git update-index --again --add --stdin'
>      Not sure that --again is good replacement.

or something like 'git update-index -q --refresh && git diff-files 
--name-only -z | git update-index -z --stdin'

> 10) git stash push [--keep-index] [--include-untracked] [--message]
>      Didn't find plumbing for stashes.

stashes are just commits really so there are no plumbing commands 
specifically related to them.

> 11) git commit [--allow-empty] [--amend] [--signoff] [--no-verify]
>        --file=CommitMessage.txt -o
>      Didn't find a plumbing to only affect named staged paths.

You can use a temporary index, add the files you want to commit with 
update-index --stdin and then run 'git commit'

When I've been scripting I've sometimes wished that diff-index and 
diff-files had a --stdin option.

Best Wishes

Phillip

Re: Support for --stdin-paths in commit, add, etc

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> No but it only takes paths not pathspecs, can you filter out the
> ignored paths first? From a UI point of view it would be better not to
> allow users to select ignored files if you don't want to be able to
> add them. If you want to use a pathspec then you can do 'git ls-files
> --exclude-standard -o -z <pathspec ...> | git update-index --add -z
> --stdin'
> ...
> You can use a temporary index, add the files you want to commit with
> update-index --stdin and then run 'git commit'

All true.  Perhaps we need a separate tutorial for scripters to
teach them how to properly combine the plumbing commands?

> When I've been scripting I've sometimes wished that diff-index and
> diff-files had a --stdin option.

Sounds fair.

Re: Support for --stdin-paths in commit, add, etc

[Alexandr Miloslavskiy]

On 01.08.2019 22:26, Junio C Hamano wrote:
> All true.  Perhaps we need a separate tutorial for scripters to
> teach them how to properly combine the plumbing commands?

@Phillip thanks for your effort!

However, I must admit that for us, this cure is worse then the problem.

We're not exactly scripting, rather we're assisting users to make
their work with git more convenient/efficient. If we replace all the
familiar commands with exotic combinations of 2-3 chained plumbing,
this will definitely leave most users completely clueless about what
our UI is doing (we have a window that shows all executed commands).

On top of that, I think that my quick analysis only uncovered like
1/3 of all the new edge cases (compared to using high-level commands)
and the solutions already look quite scary to me. Taking the other 2/3
into account, it really feels that this would be a path of lots of pain
and many bugfixes.

Re: Support for --stdin-paths in commit, add, etc

[Junio C Hamano]

Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> writes:

> On 01.08.2019 17:56, Junio C Hamano wrote:
>> So, reading paths from a file (which could be "-" as you suggest)
>> would be a good solution for that.
>
> To summarize:
> 1) Implement --paths-file for high-level commands.
> 2) '--paths-file -' would mean reading from stdin.
>
> Is that something you're ready to accept patches for?

To be honest, I am personally not enthused to any move that
encourages scripting around Porcelain commands, but being able to
feed a set of paths not from the command line may still be useful
for some of them and I've taken and I'll take changes that are
against my taste, as long as there are wide support by others.

So I will *not* say "I can tell you upfront that such patches will
be waste of time as they will not be even looked at".

That does not mean that any patch along that line will automatically
be accepted, of course, so the answer to "am I ready to accept"
question is a definite no.  No, I am not ready---we will have to
look at the actual patches before deciding.

Re: Support for --stdin-paths in commit, add, etc

[Alexandr Miloslavskiy]

On 01.08.2019 22:45, Junio C Hamano wrote:
> That does not mean that any patch along that line will automatically
> be accepted, of course, so the answer to "am I ready to accept"
> question is a definite no.  No, I am not ready---we will have to
> look at the actual patches before deciding.

That's why I previously mentioned "good enough" patches :)

Thanks for your time! I guess we'll start with a patch for 'git commit'
and see how it goes.

Re: Support for --stdin-paths in commit, add, etc

[Johannes Schindelin]

Hi Alexandr,

On Thu, 1 Aug 2019, Alexandr Miloslavskiy wrote:

> 4) git reset

Please note that I have a patch series (currently on hold because of the
v2.23.0 feature freeze) to teach `git reset` an `--stdin` option:
https://github.com/gitgitgadget/git/pull/133

Ciao,
Johannes

Re: Support for --stdin-paths in commit, add, etc

[Johannes Schindelin]

Hi Alexandr,

On Thu, 1 Aug 2019, Alexandr Miloslavskiy wrote:

> On 01.08.2019 22:45, Junio C Hamano wrote:
> > That does not mean that any patch along that line will automatically
> > be accepted, of course, so the answer to "am I ready to accept"
> > question is a definite no.  No, I am not ready---we will have to
> > look at the actual patches before deciding.
>
> That's why I previously mentioned "good enough" patches :)
>
> Thanks for your time! I guess we'll start with a patch for 'git commit'
> and see how it goes.

From past experience, I find that it is important to also implement the
`-z` option (which traditionally means: accept items via the command
line that are delimited by NULs).

And you might find inspiration in my patch to read the paths passed to
`git reset` from stdin:
https://github.com/gitgitgadget/git/pull/133/commits/1db4ef0ec1ec

I guess you could use a similar approach just after the
`parse_pathspec()` call in `prepare_index()`:
https://github.com/gitgitgadget/git/blob/1db4ef0ec1ec/builtin/commit.c#L339-L342

Ciao,
Johannes

Re: Support for --stdin-paths in commit, add, etc

[Alexandr Miloslavskiy]

@Johannes, thanks a lot for your tips!

On 02.08.2019 13:40, Johannes Schindelin wrote:
>  From past experience, I find that it is important to also implement the
> `-z` option (which traditionally means: accept items via the command
> line that are delimited by NULs).
> 
> And you might find inspiration in my patch to read the paths passed to
> `git reset` from stdin:
> https://github.com/gitgitgadget/git/pull/133/commits/1db4ef0ec1ec
> 
> I guess you could use a similar approach just after the
> `parse_pathspec()` call in `prepare_index()`:
> https://github.com/gitgitgadget/git/blob/1db4ef0ec1ec/builtin/commit.c#L339-L342

Re: Support for --stdin-paths in commit, add, etc

[Alexandr Miloslavskiy]

On 02.08.2019 13:33, Johannes Schindelin wrote:
> Please note that I have a patch series (currently on hold because of the
> v2.23.0 feature freeze) to teach `git reset` an `--stdin` option:
> https://github.com/gitgitgadget/git/pull/133

Perfect! Less work for me :)
Once your patch is accepted, could you please send me a mail?

Re: Support for --stdin-paths in commit, add, etc

[Alexandr Miloslavskiy]

On 02.08.2019 13:33, Johannes Schindelin wrote:
> to teach `git reset` an `--stdin` option:

I think it should be changed to follow the approach decided
here [1] - that is, use '--paths-file', and '--paths-file -'
will mean "from stdin"

[1] 
https://public-inbox.org/git/a6610e94-6318-b962-5dd0-ca379def3bba@syntevo.com/

Re: Support for --stdin-paths in commit, add, etc

[Johannes Schindelin]

Hi Alexandr,

On Fri, 2 Aug 2019, Alexandr Miloslavskiy wrote:

> On 02.08.2019 13:33, Johannes Schindelin wrote:
> > to teach `git reset` an `--stdin` option:
>
> I think it should be changed to follow the approach decided
> here [1] - that is, use '--paths-file', and '--paths-file -'
> will mean "from stdin"
>
> [1]
> https://public-inbox.org/git/a6610e94-6318-b962-5dd0-ca379def3bba@syntevo.com/

I am actually not sure about that. The use case I have for `git reset
--stdin` is an IDE which wants to bulk-reset paths (and runs into
command-line limitations without `--stdin`!), and definitely does *not*
want to write a file.

So I _really_ do not need `--paths-file`, and I doubt that anybody else
needs it.

Besides...

-- snip --
$ git grep -l -e --stdin v2.23.0-rc0 -- Documentation/git-\*.txt
v2.23.0-rc0:Documentation/git-check-attr.txt
v2.23.0-rc0:Documentation/git-check-ignore.txt
v2.23.0-rc0:Documentation/git-check-mailmap.txt
v2.23.0-rc0:Documentation/git-checkout-index.txt
v2.23.0-rc0:Documentation/git-cherry-pick.txt
v2.23.0-rc0:Documentation/git-commit-graph.txt
v2.23.0-rc0:Documentation/git-diff-tree.txt
v2.23.0-rc0:Documentation/git-fetch-pack.txt
v2.23.0-rc0:Documentation/git-hash-object.txt
v2.23.0-rc0:Documentation/git-http-fetch.txt
v2.23.0-rc0:Documentation/git-index-pack.txt
v2.23.0-rc0:Documentation/git-ls-tree.txt
v2.23.0-rc0:Documentation/git-name-rev.txt
v2.23.0-rc0:Documentation/git-notes.txt
v2.23.0-rc0:Documentation/git-rev-list.txt
v2.23.0-rc0:Documentation/git-send-pack.txt
v2.23.0-rc0:Documentation/git-svn.txt
v2.23.0-rc0:Documentation/git-update-index.txt
v2.23.0-rc0:Documentation/git-update-ref.txt

-- snap --

So. Lots of precedent. Conversely:

-- snip --
$ git grep -l -e --paths v2.23.0-rc0 -- Documentation/git-\*.txt

-- snap --

Zilch. So I find the advice you received, let's say, interesting.

In `git hash-object`, where it isn't obvious whether you would want to
pass in options or paths via stdin, there seems to be `--stdin-paths`. I
am open to that.

I am not open to implementing `--paths` nor `--paths-file` in `git
reset`.

Ciao,
Johannes

Re: Support for --stdin-paths in commit, add, etc

[Johannes Schindelin]

Hi Alexandr,

On Fri, 2 Aug 2019, Alexandr Miloslavskiy wrote:

> On 02.08.2019 13:33, Johannes Schindelin wrote:
> > Please note that I have a patch series (currently on hold because of the
> > v2.23.0 feature freeze) to teach `git reset` an `--stdin` option:
> > https://github.com/gitgitgadget/git/pull/133
>
> Perfect! Less work for me :)
> Once your patch is accepted, could you please send me a mail?

How about subscribing to that PR so you get updates without requiring me
to remember to send you a manual email?

Ciao,
Johannes

Re: Support for --stdin-paths in commit, add, etc

[Alexandr Miloslavskiy]

On 02.08.2019 16:48, Johannes Schindelin wrote:
> How about subscribing to that PR so you get updates without requiring me
> to remember to send you a manual email?

This option evaded me :) Subscribed, thanks!

Re: Support for --stdin-paths in commit, add, etc

[Alexandr Miloslavskiy]

On 02.08.2019 16:47, Johannes Schindelin wrote:

 > and definitely does *not* want to write a file.

In this case it can use `--paths-file -`, which means stdin.

> Zilch. So I find the advice you received, let's say, interesting.

I understood that `--paths-file` is suggested to avoid possible
stdin collisions in high-level commands.

It seems that only plumbing commands accept --stdin for paths now.

Both me and you are now trying to address high-level commands, which is
new. This explains why grep doesn't currently find `--paths-file`.

The approach sounds reasonable to me as a long-term strategy:
1) Won't require rework if someone wants file instead
2) stdin is still supported for those who want it
3) There's a way to avoid potential collisions in stdin