Re: [GSoC][PATCH v5 04/20] rebase -i: rewrite the edit-todo functionality in C

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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).

Best Wishes

Phillip


Cheers,
Alban





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux