-
Notifications
You must be signed in to change notification settings - Fork 618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A new substitution special string %(sh {shell-command}) that substitutes external command's output #1231
base: master
Are you sure you want to change the base?
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)).
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we put spaces around binary operators
while (isspace(*c)) | ||
c++; | ||
} | ||
if (!*c||strlen(c)==8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.: