Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C
Hi Alban
On 08/08/18 16:16, Alban Gruin wrote:
Hi Phillip,
Le 07/08/2018 à 15:57, Phillip Wood a écrit :
+ if (ret < 0)
+ error_errno(_("could not append help text to '%s'"),
rebase_path_todo());
+
+ fclose(todo);
You should definitely check the return value and return an error if
appropriate as fputs() might not actually write any data until you try
and close the file.
Is this worth a reroll, as this goes away in a later patch (refactored
in 08/20, removed in 12/20)?
Ah, I've only made it to patch 10 at the moment! If this is going to be
removed don't worry about it.
Best Wishes
Phillip
Best Wishes
Phillip
+ strbuf_release(&buf);
+
+ return ret;
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
new file mode 100644
index 00..47372624e0
--- /dev/null
+++ b/rebase-interactive.h
@@ -0,0 +1,6 @@
+#ifndef REBASE_INTERACTIVE_H
+#define REBASE_INTERACTIVE_H
+
+int append_todo_help(unsigned edit_todo, unsigned keep_empty);
+
+#endif
Cheers,
Alban
Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C
Hi Phillip,
Le 07/08/2018 à 15:57, Phillip Wood a écrit :
>> +if (ret < 0)
>> +error_errno(_("could not append help text to '%s'"),
>> rebase_path_todo());
>> +
>> +fclose(todo);
>
> You should definitely check the return value and return an error if
> appropriate as fputs() might not actually write any data until you try
> and close the file.
>
Is this worth a reroll, as this goes away in a later patch (refactored
in 08/20, removed in 12/20)?
> Best Wishes
>
> Phillip
>
>> +strbuf_release(&buf);
>> +
>> +return ret;
>> +}
>> diff --git a/rebase-interactive.h b/rebase-interactive.h
>> new file mode 100644
>> index 00..47372624e0
>> --- /dev/null
>> +++ b/rebase-interactive.h
>> @@ -0,0 +1,6 @@
>> +#ifndef REBASE_INTERACTIVE_H
>> +#define REBASE_INTERACTIVE_H
>> +
>> +int append_todo_help(unsigned edit_todo, unsigned keep_empty);
>> +
>> +#endif
>>
>
Cheers,
Alban
Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C
Hi Christian On 07/08/18 17:28, Christian Couder wrote: > On Tue, Aug 7, 2018 at 6:15 PM, Phillip Wood > wrote: >> On 07/08/18 16:25, Christian Couder wrote: >>> >>> I agree about checking the return value from fputs(), but it seems to >>> me that we don't usually check the value of fclose(). >> >> A quick grep shows you're right, there are only a handful of places where >> the return value of fclose() is checked (there aren't many checks for the >> return value of close() either), I'm don't think that is safe though given >> that write errors may only show up when the file gets flushed by closing it. > > I vaguely remember we tried to check those return values but it didn't > work well sometimes. > I think there's no point in checking the return value after a successful read, or when writing and the file is being closed (and then possibly removed) in some clean-up in an error path, but if the code cares about ensuring the data is written then I think it should be checking the return value, close_tempfile_gently() does for example. In the sequencer code there are some examples where it probably should be checking the return value, and places such as rewrite_file() and write_message() (which uses the lockfile api) where the return code is checked (there are a couple of recently added callers of write_message() in the code to recreate octopus merges that don't check its return value but all the other callers do). Best Wishes Phillip
Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C
On Tue, Aug 7, 2018 at 6:15 PM, Phillip Wood wrote: > On 07/08/18 16:25, Christian Couder wrote: >> >> I agree about checking the return value from fputs(), but it seems to >> me that we don't usually check the value of fclose(). > > A quick grep shows you're right, there are only a handful of places where > the return value of fclose() is checked (there aren't many checks for the > return value of close() either), I'm don't think that is safe though given > that write errors may only show up when the file gets flushed by closing it. I vaguely remember we tried to check those return values but it didn't work well sometimes.
Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C
Hi Christian
On 07/08/18 16:25, Christian Couder wrote:
Hi Phillip,
On Tue, Aug 7, 2018 at 3:57 PM, Phillip Wood wrote:
On 31/07/18 18:59, Alban Gruin wrote:
+
+ ret = fputs(buf.buf, todo);
It is not worth changing the patch just for this but strbuf_write()
might be clearer (you use it in a later patch)
+ if (ret < 0)
+ error_errno(_("could not append help text to '%s'"),
rebase_path_todo());
+
+ fclose(todo);
You should definitely check the return value and return an error if
appropriate as fputs() might not actually write any data until you try
and close the file.
I agree about checking the return value from fputs(), but it seems to
me that we don't usually check the value of fclose().
A quick grep shows you're right, there are only a handful of places
where the return value of fclose() is checked (there aren't many checks
for the return value of close() either), I'm don't think that is safe
though given that write errors may only show up when the file gets
flushed by closing it.
Best Wishes
Phillip
Thanks,
Christian.
Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C
Hi Phillip,
On Tue, Aug 7, 2018 at 3:57 PM, Phillip Wood wrote:
>
> On 31/07/18 18:59, Alban Gruin wrote:
>>
>> +
>> + ret = fputs(buf.buf, todo);
>
> It is not worth changing the patch just for this but strbuf_write()
> might be clearer (you use it in a later patch)
>
>> + if (ret < 0)
>> + error_errno(_("could not append help text to '%s'"),
>> rebase_path_todo());
>> +
>> + fclose(todo);
>
> You should definitely check the return value and return an error if
> appropriate as fputs() might not actually write any data until you try
> and close the file.
I agree about checking the return value from fputs(), but it seems to
me that we don't usually check the value of fclose().
Thanks,
Christian.
Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C
Hi Alban
On 31/07/18 18:59, Alban Gruin wrote:
> This rewrites append_todo_help() from shell to C. It also incorporates
> some parts of initiate_action() and complete_action() that also write
> help texts to the todo file.
>
> This also introduces the source file rebase-interactive.c. This file
> will contain functions necessary for interactive rebase that are too
> specific for the sequencer, and is part of libgit.a.
>
> Two flags are added to rebase--helper.c: one to call
> append_todo_help() (`--append-todo-help`), and another one to tell
> append_todo_help() to write the help text suited for the edit-todo
> mode (`--write-edit-todo`).
>
> Finally, append_todo_help() is removed from git-rebase--interactive.sh
> to use `rebase--helper --append-todo-help` instead.
>
> Signed-off-by: Alban Gruin
> ---
> No changes since v4.
>
> Makefile | 1 +
> builtin/rebase--helper.c | 11 --
> git-rebase--interactive.sh | 52 ++---
> rebase-interactive.c | 68 ++
> rebase-interactive.h | 6
> 5 files changed, 86 insertions(+), 52 deletions(-)
> create mode 100644 rebase-interactive.c
> create mode 100644 rebase-interactive.h
>
> diff --git a/Makefile b/Makefile
> index 08e5c54549..909a687857 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -922,6 +922,7 @@ LIB_OBJS += protocol.o
> LIB_OBJS += quote.o
> LIB_OBJS += reachable.o
> LIB_OBJS += read-cache.o
> +LIB_OBJS += rebase-interactive.o
> LIB_OBJS += reflog-walk.o
> LIB_OBJS += refs.o
> LIB_OBJS += refs/files-backend.o
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index f7c2a5fdc8..05e73e71d4 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -3,6 +3,7 @@
> #include "config.h"
> #include "parse-options.h"
> #include "sequencer.h"
> +#include "rebase-interactive.h"
>
> static const char * const builtin_rebase_helper_usage[] = {
> N_("git rebase--helper []"),
> @@ -12,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] =
> {
> int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
> {
> struct replay_opts opts = REPLAY_OPTS_INIT;
> - unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
> + unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo
> = 0;
> int abbreviate_commands = 0, rebase_cousins = -1;
> enum {
> CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
> CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> - ADD_EXEC
> + ADD_EXEC, APPEND_TODO_HELP
> } command = 0;
> struct option options[] = {
> OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
> @@ -27,6 +28,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
> char *prefix)
> OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge
> commits")),
> OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
>N_("keep original branch points of cousins")),
> + OPT_BOOL(0, "write-edit-todo", &write_edit_todo,
> + N_("append the edit-todo message to the todo-list")),
> OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
> CONTINUE),
> OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
> @@ -45,6 +48,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
> char *prefix)
> N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
> OPT_CMDMODE(0, "add-exec-commands", &command,
> N_("insert exec commands in todo list"), ADD_EXEC),
> + OPT_CMDMODE(0, "append-todo-help", &command,
> + N_("insert the help in the todo list"),
> APPEND_TODO_HELP),
> OPT_END()
> };
>
> @@ -84,5 +89,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
> char *prefix)
> return !!rearrange_squash();
> if (command == ADD_EXEC && argc == 2)
> return !!sequencer_add_exec_commands(argv[1]);
> + if (command == APPEND_TODO_HELP && argc == 1)
> + return !!append_todo_help(write_edit_todo, keep_empty);
> usage_with_options(builtin_rebase_helper_usage, options);
> }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 299ded2137..94c23a7af2 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -39,38 +39,6 @@ comment_for_reflog () {
> esac
> }
>
> -append_todo_help () {
> - gettext "
> -Commands:
> -p, pick = use commit
> -r, reword = use commit, but edit the commit message
> -e, edit = use commit, but stop for amending
> -s, squash = use commit, but meld into previous commit
> -f, fixup = like \"squash\", but discard this commit's log message
> -x, exec = run command (the r
