Hi Alban
On 09/08/18 16:35, Phillip Wood wrote:
Hi Alban
On 09/08/18 14:30, Alban Gruin wrote:
Hi Phillip,
Le 08/08/2018 à 18:04, Phillip Wood a écrit :
+int edit_todo_list(unsigned flags)
+{
+ struct strbuf buf = STRBUF_INIT;
+ const char *todo_file = rebase_path_todo();
+ FILE *todo;
+
+ if (strbuf_read_file(&buf, todo_file, 0) < 0)
+ return error_errno(_("could not read '%s'."), todo_file);
+
+ strbuf_stripspace(&buf, 1);
+ todo = fopen_or_warn(todo_file, "w");
This truncates the existing file, if there are any errors writing the
new one then the user has lost the old one. write_message() in
sequencer.c avoids this problem by writing a new file and then
renaming
it if the write is successful, maybe it is worth exporting it so it
can
be used elsewhere.
+ if (!todo) {
+ strbuf_release(&buf);
+ return 1;
+ }
+
+ strbuf_write(&buf, todo);
+ fclose(todo);
There needs to be some error checking after the write and the close
(using write_message() would mean you only have to check for errors in
one place)
Right. Should I find a new nawe for write_message()?
That might be a good idea, I'm not sure what it should be though, maybe
write_file()?, perhaps someone else might have a better suggestion.
write_file() already exists in wrapper.c. I wondered, as this make use
of the lockfile API, perhaps it could be moved to lockfile.{c,h}, and
renamed to something like write_file_with_lock().
Hmm, I'm not sure about that, to me the use of the lockfile api is just
a way to get the semantics of an atomic update to the file. Looking in
wrapper.c there is write_file_buf() which could maybe be changed to to
an atomic update. Looking at the current callers to that function they
look like they would work with such a change (sadly none of them bother
to check the return value to see if the write was successful). I don't
think we need to worry about them being broken by having a stale
lockfile left over if there's an error as I think the lockfile api
automatically cleans them up but I'm not an expert on that api so it
would be worth checking with someone like Johannes or Jeff King (I seem
to remember him working in that area relatively recently).
On second thoughts the current function dies rather than returning an
error so it would be a bit of work to do the conversion properly. For
now you could just make write_message() available outside sequencer.c
and see if anyone comes up with a better name when v6 is reviewed.
Best Wishes
Phillip
Best Wishes
Phillip
Cheers,
Alban
