Re: [PATCH v4 2/2] merge-ort: return early when failing to write a blob

>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In the previous commit, we fixed a segmentation fault when a tree object
> could not be written.
>
> However, before the tree object is written, `merge-ort` wants to write
> out a blob object (except in cases where the merge results in a blob
> that already exists in the database). And this can fail, too, but we
> ignore that write failure so far.
>
> Since we will always write out a new tree object in addition to the blob
> (and if the blob did not exist in the database yet, we can be certain
> that the tree object did not exist yet), the merge will _still_ fail at
> that point, but it does unnecessary work by continuing after the blob
> could not be written.

> Let's pay close attention and error out early if the blob could not be
> written. This reduces the error output of t4301.25 ("merge-ort fails
> gracefully in a read-only repository") from:
>
>         error: insufficient permission for adding an object to repository database ./objects
>         error: error: Unable to add numbers to database
>         error: insufficient permission for adding an object to repository database ./objects
>         error: error: Unable to add greeting to database
>         error: insufficient permission for adding an object to repository database ./objects
>         fatal: failure to merge
>
> to:
>
>         error: insufficient permission for adding an object to repository database ./objects
>         error: error: Unable to add numbers to database
>         fatal: failure to merge
>
> Note: This patch adjusts two variable declarations from `unsigned` to
> `int` because their purpose is to hold the return value of
> `handle_content_merge()`, which is of type `int`. The existing users of
> those variables are only interested whether that variable is zero or
> non-zero, therefore this type change does not affect the existing code.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  merge-ort.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index f3bdce1041a..e5f41cce481 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -2807,6 +2807,8 @@ static int process_renames(struct merge_options *opt,
>                                                            pathnames,
>                                                            1 + 2 * opt->priv->call_depth,
>                                                            &merged);
> +                       if (clean_merge < 0)
> +                               return -1;
>                         if (!clean_merge &&
>                             merged.mode == side1->stages[1].mode &&
>                             oideq(&merged.oid, &side1->stages[1].oid))
> @@ -2916,7 +2918,7 @@ static int process_renames(struct merge_options *opt,
>                         struct version_info merged;
>
>                         struct conflict_info *base, *side1, *side2;
> -                       unsigned clean;
> +                       int clean;
>
>                         pathnames[0] = oldpath;
>                         pathnames[other_source_index] = oldpath;
> @@ -2937,6 +2939,8 @@ static int process_renames(struct merge_options *opt,
>                                                      pathnames,
>                                                      1 + 2 * opt->priv->call_depth,
>                                                      &merged);
> +                       if (clean < 0)
> +                               return -1;
>
>                         memcpy(&newinfo->stages[target_index], &merged,
>                                sizeof(merged));
> @@ -3806,10 +3810,10 @@ static int write_completed_directory(struct merge_options *opt,
>  }
>
>  /* Per entry merge function */
> -static void process_entry(struct merge_options *opt,
> -                         const char *path,
> -                         struct conflict_info *ci,
> -                         struct directory_versions *dir_metadata)
> +static int process_entry(struct merge_options *opt,
> +                        const char *path,
> +                        struct conflict_info *ci,
> +                        struct directory_versions *dir_metadata)
>  {
>         int df_file_index = 0;
>
> @@ -3823,7 +3827,7 @@ static void process_entry(struct merge_options *opt,
>                 record_entry_for_tree(dir_metadata, path, &ci->merged);
>                 if (ci->filemask == 0)
>                         /* nothing else to handle */
> -                       return;
> +                       return 0;
>                 assert(ci->df_conflict);
>         }
>
> @@ -3870,7 +3874,7 @@ static void process_entry(struct merge_options *opt,
>                  */
>                 if (ci->filemask == 1) {
>                         ci->filemask = 0;
> -                       return;
> +                       return 0;
>                 }
>
>                 /*
> @@ -4065,7 +4069,7 @@ static void process_entry(struct merge_options *opt,
>         } else if (ci->filemask >= 6) {
>                 /* Need a two-way or three-way content merge */
>                 struct version_info merged_file;
> -               unsigned clean_merge;
> +               int clean_merge;
>                 struct version_info *o = &ci->stages[0];
>                 struct version_info *a = &ci->stages[1];
>                 struct version_info *b = &ci->stages[2];
> @@ -4074,6 +4078,8 @@ static void process_entry(struct merge_options *opt,
>                                                    ci->pathnames,
>                                                    opt->priv->call_depth * 2,
>                                                    &merged_file);
> +               if (clean_merge < 0)
> +                       return -1;
>                 ci->merged.clean = clean_merge &&
>                                    !ci->df_conflict && !ci->path_conflict;
>                 ci->merged.result.mode = merged_file.mode;
> @@ -4169,6 +4175,7 @@ static void process_entry(struct merge_options *opt,
>
>         /* Record metadata for ci->merged in dir_metadata */
>         record_entry_for_tree(dir_metadata, path, &ci->merged);
> +       return 0;
>  }
>
>  static void prefetch_for_content_merges(struct merge_options *opt,
> @@ -4285,7 +4292,10 @@ static int process_entries(struct merge_options *opt,
>                         record_entry_for_tree(&dir_metadata, path, mi);
>                 else {
>                         struct conflict_info *ci = (struct conflict_info *)mi;
> -                       process_entry(opt, path, ci, &dir_metadata);
> +                       if (process_entry(opt, path, ci, &dir_metadata) < 0) {
> +                               ret = -1;
> +                               goto cleanup;
> +                       };
>                 }
>         }
>         trace2_region_leave("merge", "processing", opt->repo);
> --
> gitgitgadget