A new substitution special string %(sh {shell-command}) that substitutes external command's output #1231
Conversation
psprint
changed the title
|
I agree the expansion system could use some love.
I feel like it's a bad idea to substitute then the contents of the %(sh) block is pure shell, no other funny syntax. Another potential problem is that you have single quotes around the shell expansion. |
| return false; | ||
| if (value[size-1]=='\n') | ||
| value[size-1]='\0'; | ||
| return string_format_from(format->buf, &format->bufpos, "%s", value); |
There was a problem hiding this comment.
the entire output becomes one string argument.
This is good for some cases but if we want to compute multiple arguments for a tig command, it becomes awkward.
An eval command might remedy that but I'm not sure if this is a good design, see also https://en.wikipedia.org/wiki/Inner-platform_effect
Maybe instead of
bind generic T :goto '%(sh git rev-list %(commit)~..|head -1)'
we should write
bind generic T %echo goto "$(git rev-list "$tig_commit"~..|head -1)"
where the % is the magic key for "run this shell command and run the output as tig command"
Something like this seems like a reasonably general approach but I'm sure there are many things I didn't consider.
Sadly I don't have a lot of time these days.
There was a problem hiding this comment.
Do you mean that it's not possible to embed %(commit( and others inside %(sh …)? When you write: the entire output becomes one string argument.? Or something else? Because I've implemented support for this in second patch 4301605.
an external command, captures its output and substitutes it in place of the %-string. Can be used as follows: bind generic T :echo '%(sh printf "Hi :)")'
Allowed are e.g.: %(prompt %(commit)), etc. and also recursive like %(sh printf %(sh printf)).
psprint
force-pushed
the
sh-substitution
branch
from
4301605
to
7058dce
Compare
|
@koutcher ping |
|
@krobelus ping? |
|
The patch is precious also because |
|
If the delay is because the Isn't it welcomed to provide flexible |
| if (value == NULL) | ||
| return false; | ||
| return string_format_from(format->buf, &format->bufpos, "%s", value); | ||
| } | ||
| if (!prefixcmp(name, "%(sh")) { |
There was a problem hiding this comment.
we put spaces around binary operators
| while (isspace(*c)) | ||
| c++; | ||
| } | ||
| if (!*c||strlen(c)==8) |
There was a problem hiding this comment.
what if this fixed-size-buffer overflows?
| char value[SIZEOF_STR]={0}; | ||
| const char *cstart = name + STRING_SIZE("%(sh"); | ||
| const int clen = end - cstart - 1; | ||
| int size; |
There was a problem hiding this comment.
this means we support recursive substitutions, like in
bind generic T :goto '%(sh git rev-list %(commit)..|tail -1)'
I think that's a bad idea because it means that inside the %(sh) expression, we need to take care of escaping both shell and Tigs %-substitutions.
We should design something that's hard to misuse.
On gitter, in a discussion there has been an idea from user @krobelus:
– it was an idea of a %(…)-like substitution string that would substitute the given external command's output. I have implemented the idea in this PR, as
%(sh …)special string, similar to%(prompt …).This PR implements a new substitution special string
%(sh {shell-command})that runs an external command, captures its output and substitutes it in place of the%-string. It can be used as follows:As it can be seen, the command is run via
sh -c '{command}', so environment variable substitutions can be used.The implementation works OK with limitation inherited from
%(prompt )– in the argument there shouldn't be any other substitutions like%(commit), because they will not be substituted. Any ideas were in the code to implement this? It would be needed for e.g.: