Re: [PATCH 2/5 v2] unpack_trees: group errors by type

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 15, 2010, 12:58 UTC
Message-ID: <vpqljagzc39.fsf@bauges.imag.fr>
In-Reply-To: <1276604576-28092-3-git-send-email-diane.gasselin@ensimag.imag.fr>

Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:

Show 26 quoted lines

> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -60,6 +60,92 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>  }
>  
>  /*
> + * add error messages on path <path> and action <action>
> + * corresponding to the type <e> with the message <msg>
> + * indicating if it should be display in porcelain or not
> + */
> +static int add_rejected_path(struct unpack_trees_options *o,
> +			     enum unpack_trees_error e,
> +			     const char *path,
> +			     const char *action,
> +			     int porcelain,
> +			     const char *msg)
> +{
> +	struct rejected_paths_list *newentry;
> +	struct rejected_paths **rp;
> +	/*
> +	 * simply display the given error message if in plumbing mode
> +	 */
> +	if (!porcelain)
> +		o->show_all_errors = 0;
> +	if (!o->show_all_errors)
> +		return error(msg, path, action);

I don't fully understand what you're doing with show_all_errors and porcelain here. From the caller, "porcelain" is true iff the corresponding error message has been set in o. But if you can infer whether you're in porcelain from the error messages, why do you need show_all_errors in addition?

Show 5 quoted lines

>  static int reject_merge(struct cache_entry *ce, struct unpack_trees_options *o)
>  {
> -	return error(ERRORMSG(o, would_overwrite), ce->name);
> +	return add_rejected_path(o, would_overwrite, ce->name, NULL,
> +				 (o && (o)->msgs.would_overwrite),

Parenthesis around (o) are distracting and useless. I guess you copy-pasted from a macro (for which parentheses should definitely be used in case the macro is called on an arbitrary expression).

Show 17 quoted lines

> @@ -874,8 +964,16 @@ static int verify_uptodate_1(struct cache_entry *ce,
>  	}
>  	if (errno == ENOENT)
>  		return 0;
> -	return o->gently ? -1 :
> -		error(error_msg, ce->name);
> +	if (error == sparse_not_uptodate_file)
> +		return o->gently ? -1 :
> +			add_rejected_path(o, sparse_not_uptodate_file, ce->name, NULL,
> +					  (o && (o)->msgs.sparse_not_uptodate_file),
> +					  ERRORMSG(o, sparse_not_uptodate_file));
> +	else
> +		return o->gently ? -1 :
> +			add_rejected_path(o, not_uptodate_file, ce->name, NULL,
> +					  (o && (o)->msgs.not_uptodate_file),
> +					  ERRORMSG(o, not_uptodate_file));
>  }

Isn't that a complex way of saying

	int porcelain;
	if (error == sparse_not_uptodate_file)
		porcelain = o && o->msgs.sparse_not_uptodate_file;
	else
		porcelain = o && o->msgs.not_uptodate_file;
	return o->gently ? -1 :
			add_rejected_path(o, error, ce->name, NULL,
					  porcelain, ERRORMSG(o, error));

Also, I'm not sure I understand why you're attaching the error message string to each rejected_paths entry. Wouldn't it be more sensible to use o->msg in display_error_msgs() instead?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/