Re: [PATCH v2 05/20] merge-ort: add an err() function similar to one from merge-recursive
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Nov 11, 2020, 13:58 UTC
- Message-ID
- <267dbe58-e319-0904-3552-bf56530181c3@gmail.com>
- In-Reply-To
- <20201102204344.342633-6-newren@gmail.com>
On 11/2/2020 3:43 PM, Elijah Newren wrote:
Show 37 quoted lines
> Various places in merge-recursive used an err() function when it hit
> some kind of unrecoverable error. That code was from the reusable bits
> of merge-recursive.c that we liked, such as merge_3way, writing object
> files to the object store, reading blobs from the object store, etc. So
> create a similar function to allow us to port that code over, and use it
> for when we detect problems returned from collect_merge_info()'s
> traverse_trees() call, which we will be adding next.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> merge-ort.c | 27 ++++++++++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index df97a54773..537da9f6df 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -61,11 +61,28 @@ struct conflict_info {
> unsigned match_mask:3;
> };
>
> +static int err(struct merge_options *opt, const char *err, ...)
> +{
> + va_list params;
> + struct strbuf sb = STRBUF_INIT;
> +
> + strbuf_addstr(&sb, "error: ");
> + va_start(params, err);
> + strbuf_vaddf(&sb, err, params);
> + va_end(params);
> +
> + error("%s", sb.buf);
> + strbuf_release(&sb);
> +
> + return -1;
> +}
> +This seems simple enough to have a duplicate copy lying around. Do you anticipate that all common code will live in the same file eventually? Or will these two static err() method always be duplicated?
Aside: I wonder if these errors could be logged using trace2 primitives, to assist diagnosing problems with merges. CC'ing Jeff Hostetler if he has an opinion.
Show 8 quoted lines
> static int collect_merge_info(struct merge_options *opt,
> struct tree *merge_base,
> struct tree *side1,
> struct tree *side2)
> {
> + /* TODO: Implement this using traverse_trees() */
> die("Not yet implemented.");
> }This comment looks to be applied to the wrong patch.
Thanks, -Stolee