Re: [PATCH v3 1/9] am: stop ignoring errors reported by log_tree_diff()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 21, 2016, 18:59 UTC
- Message-ID
- <xmqqporapdu8.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <e1d68fa14e7938f7e3a07bb85db28aa84017710d.1466505222.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Note: there are more places in the builtin am code that ignore > errors returned from library functions. Fixing those is outside the > purview of the current patch series, though.
The caller of parse_mail() and parse_mail_rebase() is not prepared to see an error code in the returned value from these function, which are to return a boolean telling the caller to skip or use the patch file. At least the caller needs to notice negative return and made to die/exit(128), instead of silently skipping a corrupt or unopenable patch, no? Otherwise this will change the behaviour.
Show 54 quoted lines
> Cc: Paul Tan <pyokagan@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/am.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 3dfe70b..0e28a62 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1433,12 +1433,15 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
> /**
> * Writes `commit` as a patch to the state directory's "patch" file.
> */
> -static void write_commit_patch(const struct am_state *state, struct commit *commit)
> +static int write_commit_patch(const struct am_state *state, struct commit *commit)
> {
> struct rev_info rev_info;
> FILE *fp;
>
> - fp = xfopen(am_path(state, "patch"), "w");
> + fp = fopen(am_path(state, "patch"), "w");
> + if (!fp)
> + return error(_("Could not open %s for writing"),
> + am_path(state, "patch"));
> init_revisions(&rev_info, NULL);
> rev_info.diff = 1;
> rev_info.abbrev = 0;
> @@ -1453,7 +1456,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
> rev_info.diffopt.close_file = 1;
> add_pending_object(&rev_info, &commit->object, "");
> diff_setup_done(&rev_info.diffopt);
> - log_tree_commit(&rev_info, commit);
> + return log_tree_commit(&rev_info, commit);
> }
>
> /**
> @@ -1501,13 +1504,14 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
> unsigned char commit_sha1[GIT_SHA1_RAWSZ];
>
> if (get_mail_commit_sha1(commit_sha1, mail) < 0)
> - die(_("could not parse %s"), mail);
> + return error(_("could not parse %s"), mail);
>
> commit = lookup_commit_or_die(commit_sha1, mail);
>
> get_commit_info(state, commit);
>
> - write_commit_patch(state, commit);
> + if (write_commit_patch(state, commit) < 0)
> + return -1;
>
> hashcpy(state->orig_commit, commit_sha1);
> write_state_text(state, "original-commit", sha1_to_hex(commit_sha1));