Skip to content

Commit

Permalink
fix bug on -I with rename
Browse files Browse the repository at this point in the history
  • Loading branch information
johnkerl committed Sep 2, 2019
1 parent c6ba387 commit 9e2cd33
Show file tree
Hide file tree
Showing 9 changed files with 162 additions and 11 deletions.
22 changes: 17 additions & 5 deletions c/cli/mlrcli.c
Original file line number Diff line number Diff line change
Expand Up @@ -326,9 +326,18 @@ sllv_t* cli_parse_mappers(char** argv, int* pargi, int argc, cli_opts_t* popts,
main_usage_short(stderr, MLR_GLOBALS.bargv0);
exit(1);
}

// Note that the command-line parsers can operate destructively on argv, e.g. verbs
// which take comma-delimited field names splitting on commas. For this reason we
// need to duplicate argv on each in-place run within the streamer module. But before
// that ever happens, here we run through the verb-parsers once to find out where it
// is on the command line that the verbs and their arguments end and the filenames
// begin.

char** xargv = popts->do_in_place ? copy_argv(popts->argv) : popts->argv;
while (TRUE) {
check_arg_count(argv, argi, argc, 1);
char* verb = argv[argi];
check_arg_count(xargv, argi, argc, 1);
char* verb = xargv[argi];

mapper_setup_t* pmapper_setup = look_up_mapper_setup(verb);
if (pmapper_setup == NULL) {
Expand All @@ -338,7 +347,7 @@ sllv_t* cli_parse_mappers(char** argv, int* pargi, int argc, cli_opts_t* popts,
}

if ((argc - argi) >= 2) {
if (streq(argv[argi+1], "-h") || streq(argv[argi+1], "--help")) {
if (streq(xargv[argi+1], "-h") || streq(xargv[argi+1], "--help")) {
pmapper_setup->pusage_func(stdout, MLR_GLOBALS.bargv0, verb);
exit(0);
}
Expand All @@ -347,7 +356,7 @@ sllv_t* cli_parse_mappers(char** argv, int* pargi, int argc, cli_opts_t* popts,
// It's up to the parse func to print its usage on CLI-parse failure.
// Also note: this assumes main reader/writer opts are all parsed
// *before* mapper parse-CLI methods are invoked.
mapper_t* pmapper = pmapper_setup->pparse_func(&argi, argc, argv,
mapper_t* pmapper = pmapper_setup->pparse_func(&argi, argc, xargv,
&popts->reader_opts, &popts->writer_opts);
if (pmapper == NULL) {
exit(1);
Expand All @@ -360,10 +369,13 @@ sllv_t* cli_parse_mappers(char** argv, int* pargi, int argc, cli_opts_t* popts,

sllv_append(pmapper_list, pmapper);

if (argi >= argc || !streq(argv[argi], "then"))
if (argi >= argc || !streq(xargv[argi], "then"))
break;
argi++;
}
if (popts->do_in_place) {
free_argv_copy(xargv);
}

*pargi = argi;
return pmapper_list;
Expand Down
2 changes: 1 addition & 1 deletion c/draft-release-notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@

## Bugfixes:

* ...
* https://github.com/johnkerl/miller/issues/253 fixes a bug in the [**label**](http://johnkerl.org/miller/doc/reference-verbs.html#label) when one or more names are common between old and new.
27 changes: 27 additions & 0 deletions c/lib/mlrutil.c
Original file line number Diff line number Diff line change
Expand Up @@ -602,3 +602,30 @@ char* alloc_suffixed_temp_file_name(char* filename) {

return output;
}

// ----------------------------------------------------------------
// The convention for argv-style arrays is that they're null-terminated.
// So we loop through once to find the length.
char** copy_argv(char** argv) {
int length = 0;
int argi;
for (argi = 0; argv[argi]; argi++) {
length++;
}

char** copy = mlr_malloc_or_die((length + 1) * sizeof(char*));
for (argi = 0; argv[argi]; argi++) {
copy[argi] = mlr_strdup_or_die(argv[argi]);
}

copy[length] = 0;

return copy;
}

void free_argv_copy(char** copy) {
for (int argi = 0; copy[argi]; argi++) {
free(copy[argi]);
}
free(copy);
}
3 changes: 3 additions & 0 deletions c/lib/mlrutil.h
Original file line number Diff line number Diff line change
Expand Up @@ -179,4 +179,7 @@ char* read_fp_into_memory(FILE* fp, size_t* psize);
// Returns a copy of the filename with random characters attached to the end.
char* alloc_suffixed_temp_file_name(char* filename);

char** copy_argv(char** argv);
void free_argv_copy(char** argv);

#endif // MLRUTIL_H
3 changes: 3 additions & 0 deletions c/mapping/mapper_rename.c
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "lib/mlr_globals.h"
#include "lib/mlrutil.h"
#include "lib/mlrregex.h"
#include "lib/string_builder.h"
Expand Down Expand Up @@ -88,6 +89,8 @@ static mapper_t* mapper_rename_parse_cli(int* pargi, int argc, char** argv,

slls_t* pnames = slls_from_line(argv[*pargi], ',', FALSE);
if ((pnames->length % 2) != 0) {
fprintf(stderr, "%s %s: name-list must have even length; got \"%s\".\n",
MLR_GLOBALS.bargv0, verb, argv[*pargi]);
mapper_rename_usage(stderr, argv[0], verb);
return NULL;
}
Expand Down
86 changes: 86 additions & 0 deletions c/reg_test/expected/out
Original file line number Diff line number Diff line change
Expand Up @@ -41114,14 +41114,100 @@ mlr cat then nothing then cat ./reg_test/input/abixy
================================================================
IN-PLACE PROCESSING

cat /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp1
a=pan,b=pan,i=1,x=0.3467901443380824,y=0.7268028627434533
a=eks,b=pan,i=2,x=0.7586799647899636,y=0.5221511083334797
a=wye,b=wye,i=3,x=0.20460330576630303,y=0.33831852551664776
a=eks,b=wye,i=4,x=0.38139939387114097,y=0.13418874328430463
a=wye,b=pan,i=5,x=0.5732889198020006,y=0.8636244699032729
a=zee,b=pan,i=6,x=0.5271261600918548,y=0.49322128674835697
a=eks,b=zee,i=7,x=0.6117840605678454,y=0.1878849191181694
a=zee,b=wye,i=8,x=0.5985540091064224,y=0.976181385699006
a=hat,b=wye,i=9,x=0.03144187646093577,y=0.7495507603507059
a=pan,b=wye,i=10,x=0.5026260055412137,y=0.9526183602969864

cat /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp2
a=pan,b=pan,i=1,x=0.3467901443380824,y=0.7268028627434533
a=eks,b=pan,i=2,x=0.7586799647899636,y=0.5221511083334797
a=wye,b=wye,i=3,x=0.20460330576630303,y=0.33831852551664776
a=eks,b=wye,i=4,x=0.38139939387114097,y=0.13418874328430463
a=wye,b=pan,i=5,x=0.5732889198020006,y=0.8636244699032729
a=zee,b=pan,i=6,x=0.5271261600918548,y=0.49322128674835697
a=eks,b=zee,i=7,x=0.6117840605678454,y=0.1878849191181694
a=zee,b=wye,i=8,x=0.5985540091064224,y=0.976181385699006
a=hat,b=wye,i=9,x=0.03144187646093577,y=0.7495507603507059
a=pan,b=wye,i=10,x=0.5026260055412137,y=0.9526183602969864

mlr -I --opprint head -n 2 /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp1 /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp2

cat /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp1
a b i x y
pan pan 1 0.3467901443380824 0.7268028627434533
eks pan 2 0.7586799647899636 0.5221511083334797

cat /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp2
a b i x y
pan pan 1 0.3467901443380824 0.7268028627434533
eks pan 2 0.7586799647899636 0.5221511083334797

mlr -I --opprint head -n 2
mlr: -I option (in-place operation) requires input files.

mlr -I --opprint -n head -n 2 /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp1
mlr: -I option (in-place operation) requires input files.

cat /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp1
a=pan,b=pan,i=1,x=0.3467901443380824,y=0.7268028627434533
a=eks,b=pan,i=2,x=0.7586799647899636,y=0.5221511083334797
a=wye,b=wye,i=3,x=0.20460330576630303,y=0.33831852551664776
a=eks,b=wye,i=4,x=0.38139939387114097,y=0.13418874328430463
a=wye,b=pan,i=5,x=0.5732889198020006,y=0.8636244699032729
a=zee,b=pan,i=6,x=0.5271261600918548,y=0.49322128674835697
a=eks,b=zee,i=7,x=0.6117840605678454,y=0.1878849191181694
a=zee,b=wye,i=8,x=0.5985540091064224,y=0.976181385699006
a=hat,b=wye,i=9,x=0.03144187646093577,y=0.7495507603507059
a=pan,b=wye,i=10,x=0.5026260055412137,y=0.9526183602969864

cat /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp2
a=pan,b=pan,i=1,x=0.3467901443380824,y=0.7268028627434533
a=eks,b=pan,i=2,x=0.7586799647899636,y=0.5221511083334797
a=wye,b=wye,i=3,x=0.20460330576630303,y=0.33831852551664776
a=eks,b=wye,i=4,x=0.38139939387114097,y=0.13418874328430463
a=wye,b=pan,i=5,x=0.5732889198020006,y=0.8636244699032729
a=zee,b=pan,i=6,x=0.5271261600918548,y=0.49322128674835697
a=eks,b=zee,i=7,x=0.6117840605678454,y=0.1878849191181694
a=zee,b=wye,i=8,x=0.5985540091064224,y=0.976181385699006
a=hat,b=wye,i=9,x=0.03144187646093577,y=0.7495507603507059
a=pan,b=wye,i=10,x=0.5026260055412137,y=0.9526183602969864

mlr -I --opprint rename a,AYE,b,BEE /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp1 /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp2

cat /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp1
AYE BEE i x y
pan pan 1 0.3467901443380824 0.7268028627434533
eks pan 2 0.7586799647899636 0.5221511083334797
wye wye 3 0.20460330576630303 0.33831852551664776
eks wye 4 0.38139939387114097 0.13418874328430463
wye pan 5 0.5732889198020006 0.8636244699032729
zee pan 6 0.5271261600918548 0.49322128674835697
eks zee 7 0.6117840605678454 0.1878849191181694
zee wye 8 0.5985540091064224 0.976181385699006
hat wye 9 0.03144187646093577 0.7495507603507059
pan wye 10 0.5026260055412137 0.9526183602969864

cat /Users/kerl/pub_http_internet/miller-releases/miller-head/c/output-regtest/abixy.temp2
AYE BEE i x y
pan pan 1 0.3467901443380824 0.7268028627434533
eks pan 2 0.7586799647899636 0.5221511083334797
wye wye 3 0.20460330576630303 0.33831852551664776
eks wye 4 0.38139939387114097 0.13418874328430463
wye pan 5 0.5732889198020006 0.8636244699032729
zee pan 6 0.5271261600918548 0.49322128674835697
eks zee 7 0.6117840605678454 0.1878849191181694
zee wye 8 0.5985540091064224 0.976181385699006
hat wye 9 0.03144187646093577 0.7495507603507059
pan wye 10 0.5026260055412137 0.9526183602969864


================================================================
MAPPER TEE REDIRECTS
Expand Down
16 changes: 12 additions & 4 deletions c/reg_test/run
Original file line number Diff line number Diff line change
Expand Up @@ -4359,15 +4359,23 @@ announce IN-PLACE PROCESSING

cp $indir/abixy $outdir/abixy.temp1
cp $indir/abixy $outdir/abixy.temp2
cat $outdir/abixy.temp1
cat $outdir/abixy.temp2
run_cat $outdir/abixy.temp1
run_cat $outdir/abixy.temp2
run_mlr -I --opprint head -n 2 $outdir/abixy.temp1 $outdir/abixy.temp2
cat $outdir/abixy.temp1
cat $outdir/abixy.temp2
run_cat $outdir/abixy.temp1
run_cat $outdir/abixy.temp2

mlr_expect_fail -I --opprint head -n 2 < $outdir/abixy.temp1
mlr_expect_fail -I --opprint -n head -n 2 $outdir/abixy.temp1

cp $indir/abixy $outdir/abixy.temp1
cp $indir/abixy $outdir/abixy.temp2
run_cat $outdir/abixy.temp1
run_cat $outdir/abixy.temp2
run_mlr -I --opprint rename a,AYE,b,BEE $outdir/abixy.temp1 $outdir/abixy.temp2
run_cat $outdir/abixy.temp1
run_cat $outdir/abixy.temp2

# ----------------------------------------------------------------
announce MAPPER TEE REDIRECTS

Expand Down
10 changes: 9 additions & 1 deletion c/stream/stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,15 @@ static int do_stream_chained_in_place(context_t* pctx, cli_opts_t* popts) {
lrec_reader_t* plrec_reader = lrec_reader_alloc_or_die(&popts->reader_opts);
lrec_writer_t* plrec_writer = lrec_writer_alloc_or_die(&popts->writer_opts);

// Note that the command-line parsers can operate destructively on argv,
// e.g. verbs which take comma-delimited field names splitting on commas.
// For this reason we need to duplicate argv on each run. We need to free
// after processing in case mappers have retained pointers into argv.

int argi = popts->mapper_argb;
int unused;
sllv_t* pmapper_list = cli_parse_mappers(popts->argv, &argi, popts->argc, popts, &unused);
char** argv_copy = copy_argv(popts->argv);
sllv_t* pmapper_list = cli_parse_mappers(argv_copy, &argi, popts->argc, popts, &unused);
MLR_INTERNAL_CODING_ERROR_IF(pmapper_list->length < 1); // Should not have been allowed by the CLI parser.

char* filename = pe->value;
Expand Down Expand Up @@ -109,6 +115,8 @@ static int do_stream_chained_in_place(context_t* pctx, cli_opts_t* popts) {
plrec_writer->pfree_func(plrec_writer, pctx);

mapper_chain_free(pmapper_list, pctx);

free_argv_copy(argv_copy);
}

return ok;
Expand Down
4 changes: 4 additions & 0 deletions c/todo.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ FUNDAM:

* valgrind all since 5.5.0

* in-place:
- more UT
- also check for adding no_mmap

* linear rename:
- new UTs

Expand Down

0 comments on commit 9e2cd33

Please sign in to comment.