Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

A new substitution special string %(sh {shell-command}) that substitutes external command's output #1231

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3cf262
A new substitution special string %(sh {shell-command}) that runs
psprint Aug 31, 2022
7058dce
Handle embedded %()-substitutions inside %(prompt) and %(sh)
psprint Sep 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 90 additions & 4 deletions src/argv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ format_expand_arg(struct format_context *format, const char *name, const char *e
const char *value;
const char *msgstart = name + STRING_SIZE("%(prompt");
const int msglen = end - msgstart - 1;

char *tmp;
if (end && msglen > 0 && string_format(msgbuf, "%.*s", msglen, msgstart)) {
const char *msg = msgbuf;

Expand All @@ -310,11 +310,39 @@ format_expand_arg(struct format_context *format, const char *name, const char *e
prompt = msg;
}

value = read_prompt(prompt);
tmp=argv_format_arg(&argv_env, prompt);
value = read_prompt(tmp);
free(tmp);
if (value == NULL)
return false;
return string_format_from(format->buf, &format->bufpos, "%s", value);
}
if (!prefixcmp(name, "%(sh")) {
Copy link
Contributor

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

char cbuf[SIZEOF_STR];
const char *c="";
char value[SIZEOF_STR]={0};
const char *cstart = name + STRING_SIZE("%(sh");
const int clen = end - cstart - 1;
int size;
Copy link
Contributor

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.

char *tmp;
FILE *cp;
if (end && clen > 0 && string_format(cbuf, "%.*s", clen, cstart)) {
c=cbuf;
while (isspace(*c))
c++;
}
if (!*c||strlen(c)==8)
Copy link
Contributor

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?

return false;
tmp=argv_format_arg(&argv_env, c);
cp=popen(tmp,"r");
free(tmp);
size=fread(value,1,SIZEOF_STR-1,cp);
if (value[0]==0 || size==0)
return false;
if (value[size-1]=='\n')
value[size-1]='\0';
return string_format_from(format->buf, &format->bufpos, "%s", value);
Copy link
Contributor

@krobelus krobelus Aug 31, 2022 •
edited
Loading

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.

Copy link
Contributor Author

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.

}

for (i = 0; i < format->vars_size; i++) {
if (string_enum_compare(name, vars[i].name, vars[i].namelen))
Expand All @@ -329,6 +357,64 @@ format_expand_arg(struct format_context *format, const char *name, const char *e
return false;
}

static const char*
get_closing_brace(const char *input)
{
const char *s=input,*cur, *tmp;
int idx,level=0,pair_idx;
int level_to_pos[3000]={[0 ... 2998]=-1, -2};
int pos_to_level[256]={[0 ... 254]=-1, -2};
int pair_map[3000]={[0 ... 2998]=-1, -2};
int final_pairs[3000]={[0 ... 2998]=-1, -2};
int first=-1;
while ( (cur = strstr(s,"(")), (tmp = strstr(s,")")), (cur&&cur<tmp?cur:(cur=tmp))) {
s=cur+1;
idx=cur-input;
if ( *cur == '(' ) {
pos_to_level[idx]=++level;
level_to_pos[level]=idx;
} else if ( *cur == ')' ) {
if ( level > 0 ) {
pair_idx=level_to_pos[level];
pos_to_level[idx]=level --;
if (input[pair_idx]=='(' && input[idx]==')' ||
input[pair_idx]==')' && input[idx]=='(' ) {
final_pairs[idx]=pair_idx;
final_pairs[pair_idx]=idx;
}
} else {
pos_to_level[idx]=-1;
}
}
}

for (idx=0;pos_to_level[idx]!=-2; idx++) {
bool ok=false;
if (pos_to_level[idx]==-1)
continue;
for (int j=0; final_pairs[j]!=-2;j++) {
if (final_pairs[j]==-1)
continue;
if (final_pairs[j]==idx) {
if (idx==0)
first=j;
ok=true;
break;
}
}
if (!ok)
return NULL;
}

if (pos_to_level[0] < 0)
return NULL;
if (first<0)
return NULL;

return input+first;
}


static bool
format_append_arg(struct format_context *format, const char ***dst_argv, const char *arg)
{
Expand All @@ -339,7 +425,7 @@ format_append_arg(struct format_context *format, const char ***dst_argv, const c
const char *var = strstr(arg, "%(");
const char *esc = strstr(arg, "%%");
bool is_escaped = esc && (esc < var || !var);
const char *closing = var && !is_escaped ? strchr(var, ')') : NULL;
const char *closing = var && !is_escaped ? get_closing_brace(var+1) : NULL;
const char *next = is_escaped ? esc + 2 : closing ? closing + 1 : NULL;
int len = var && !is_escaped ? var - arg : esc ? esc - arg + 1 : strlen(arg);

Expand All @@ -349,7 +435,7 @@ format_append_arg(struct format_context *format, const char ***dst_argv, const c
if (len && !string_format_from(format->buf, &format->bufpos, "%.*s", len, arg))
return false;

if (var && !is_escaped && !format_expand_arg(format, var, next))
if (var && !is_escaped && !format_expand_arg(format, var, next))
return false;

arg = next;
Expand Down