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.

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

In-place edit (-i) issues (was: assertion failure in main.c line 518 & coredump) #704

Closed
alanbur opened this issue Feb 18, 2015 · 19 comments
Assignees
Labels
Milestone

Comments

@alanbur
Copy link

alanbur commented Feb 18, 2015

This behaviour seems a little severe:

$ jq --version
jq-1.5rc1-31-g3b8d081
$ cat a
[1,2,3]
$ cat b
{"a":4,"b":5,"c":6}
$ jq '.' -i a b
Assertion failed: first_file != 0 && !strcmp(first_file, "-"), file main.c, line 518, function main
Abort (core dumped)
@nicowilliams
Copy link
Contributor

Thanks for the report!

@nicowilliams
Copy link
Contributor

Fixed. This time with a test.

@nicowilliams nicowilliams added this to the 1.5 release milestone Feb 19, 2015
@nicowilliams nicowilliams self-assigned this Feb 19, 2015
@alanbur
Copy link
Author

alanbur commented Feb 19, 2015

Don't think it's quite right - although I don't get the assert, the first file ends up containing the merged content of both the first and second files, and the first file contains all the syntax-colouring escape codes even though the output is a file.

@nicowilliams
Copy link
Contributor

The coloring is clearly a bug (fix coming up), but -i is intended to put all the output in the first file. If you don't want this, use --argfile.

What would the alternative be? To in-place edit every file? I suppose. What do others thing?

@nicowilliams nicowilliams reopened this Feb 19, 2015
@nicowilliams nicowilliams changed the title assertion failure in main.c line 518 & coredump In-place edit (-i) issues (was: assertion failure in main.c line 518 & coredump) Feb 19, 2015
@pkoppstein
Copy link
Contributor

@nicowilliams asked:

What would the alternative be? To in-place edit every file?

Yes.

The inspiration (if not model) for jq's -i option seems to be sed's -i option, and sed's behavior is perfectly reasonable in this respect (as in others). To avoid confusion, I think it would be reasonable for jq to take the view (so to speak) that the -i option is a switch (!) that is intended to change its behavior to be more like (maybe even much more like) sed's. (Yes, I often use -i.bak)

@nicowilliams
Copy link
Contributor

In-place editing of each file is a significant rototill, particularly after the refactoring I did of main.c. I'll have to make process() a utility (fine, I wanted to anyways). For each file to be processed I'll have to keep track of whether it is to be edited in place, and... what to do about color?

@pkoppstein
Copy link
Contributor

@nicowilliams wrote:

rototill .... what to do about color?

Maybe the appropriate thing to do is to take -i out of 1.5.

As for color -- as I suggested, I think you'll feel greatly liberated if you take the view that -i is a SWITCH!

@alanbur
Copy link
Author

alanbur commented Feb 19, 2015

The documentation for -i says "Edit the (first) file in place" so I guess what it's doing now is (partly) as advertised, what I didn't expect to happen is for all the following files to be inserted into the first one as that doesn't really seem to be an in-place edit. I think there are perhaps 3 choices:

  • Have -i do in-place edits of all the following files
  • Report an error and exit if -i is used with more than 1 file argument
  • Remove the -i functionality

I think the first is probably the best, but I understand it might be a fair amount of work.

@nicowilliams
Copy link
Contributor

I'll make it do the right thing. For color I'll have it recompute dumpopts every time. All of this machinery will move into libjq, I think.

@alanbur
Copy link
Author

alanbur commented Feb 19, 2015

Thanks. As an aside, I hadn't yet figured out that there was a libjq, interesting - I'll go investigate.

@nicowilliams
Copy link
Contributor

@alanbur Yeah, take a look at libjq if it's of any use to you. There are two APIs in it: the jv API (everything to do with parsing and formatting JSON texts, and the internal representation of JSON values) and the jq API (everything to do with evaluating jq programs).

@stedolan's jv API is the best C JSON API I have seen yet. In particular it's built around COW, and this makes it very easy to write C code that uses this API. The code styling is @stedolan's, and it's not at all like the Solaris cstyle that you and I are accustomed to.

EDIT: I should add that I find @stedolan's cstyle very comfortable.

@nicowilliams
Copy link
Contributor

Alright, the new plan is as follows:

  • each -n and -s (and various other) option will toggle the corresponding behaviors
  • -i will become -i file (-f is not really -f file; option processing is not that smart, but it will have to become so) and will process and edit in-place just that file
  • Each file will be processed as we happen upon it while processing options.
  • If -n is in effect, any remaining non-option arguments will be made available as $ARGS

It should be possible to do something like:

$ cat > t.jq
#!jq -fin
[., $ARGS]
$ chmod 755 t.jq
$ jq -n '"hello"' > a.json
$ ./t.jq a.json 'one argument' 'another argument'
$ cat a.json
["hello", ["one argument", "another argument"]]
$

This is nice in part because it means there's no need to add more options to say "remaining arguments are strings".

OTOH, this means that positional order of options becomes [more] important. I can live with that.

@pkoppstein
Copy link
Contributor

@nicowilliams wrote:

-i will become -i file

Why not adopt a sed (or at least sed-like) approach, i.e. with a preference (or requirement) for a suffix?

(Since sed does not (as a matter of policy) require true in-place editing (same inode), making the suffix a requirement would not be such a bad thing, would it?)

@nicowilliams
Copy link
Contributor

I'll think about the sed -i approach. (Obviously nothing can do "true in-place editing", keeping the same inode, since there's no way to do that atomically, and that matters.)

@pkoppstein
Copy link
Contributor

@nicowilliams wrote:

Obviously ...

It's a minor point, but I think there is a useful sense in which ed does "true in-place editing".

More importantly, it seems to me that there's some kind of lesson here, regarding seemingly small changes that turn out to be significant distractions. It still seems to me that pulling "-i" from 1.5 might be the wise thing to do.

@nicowilliams
Copy link
Contributor

The problem with "true" in-place editing is the lack of atomicity. A SQLite3 can manage fine. An interactive editor can also do in-place edits (because it can have a backup file and offer a recovery mode). But if a file is being watched for, the only atomic operations that work are rename and unlink. (Yes, that's POSIX, but Win32 nowadays allows a file opener to request POSIX semantics as to renames and unlinks, FWIW.)

As jq is not interactive, I think the right thing to do is to rename into place -- of that much I am convinced.

@alanbur
Copy link
Author

alanbur commented Feb 20, 2015

@nicowilliams thanks for the libjq info. I speak Scala, so 2-space indents and functional-style programming are perfectly comfortable :-)

@nicowilliams
Copy link
Contributor

The new plan is to process files as file arguments are encountered instead of accumulating them in a list to process after all option processing is complete. This will allow slurping some files and not others, for example, if we make -s toggle (probably a bad idea) or add an inverse of -s (a better idea just like there is for color vs. monochrome). Though I don't want to add more options, so -s won't get an inverse and won't toggle.

Where does that leave -i? It either has to be global (all file arguments are edited in-place) or per-file argument. The former has the benefit of familiarity via sed(1), but it feels wrong, and so does the latter. I've argued before that it's trivial to write a wrapper to stream editors to edit files in place, and I did write such a thing (https://github.com/nicowilliams/inplace), so I'm inclined to drop -i and leave it for another day, as @pkoppstein suggests. Using an external tool for in-place stream editing has the benefit of allowing one to get either flavor of in-place editing (rename-into-place vs. write-over) without complicating the stream editor interface.

I should extend inplace to have a mode where it consumes stdin to get the new data, so one could:

$ jq -f prog some-file.json | inplace -f some-file.json

instead of having to:

$ inplace some-file.json jq -f prog -

@nicowilliams
Copy link
Contributor

The other thing is that this really begs for a builtin to do this from within a jq program. I know...

nicowilliams added a commit that referenced this issue Mar 6, 2015
In-place editing should be implemented with builtins for file I/O.
@ghost ghost mentioned this issue Oct 23, 2015
klemensn added a commit to klemensn/jq that referenced this issue Oct 21, 2023
b82c231 "Remove -i option (jqlang#704)" removed its last usage in 2015.

Spotted while looking for code could potentially write/create/modify files.
nicowilliams pushed a commit that referenced this issue Oct 22, 2023
b82c231 "Remove -i option (#704)" removed its last usage in 2015.

Spotted while looking for code could potentially write/create/modify files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants