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

Thanks for your comments.

Le 9 juin 2010 18:50, Junio C Hamano [off-list ref] a écrit :

Diane Gasselin [off-list ref] writes:

quoted

+/*
+ * Store error messages in an array, each case
+ * corresponding to a error message type
+ */
+typedef enum {
+     would_overwrite,
+     not_uptodate_file,
+     not_uptodate_dir,
+     would_lose_untracked,
+     would_lose_untracked_removed,
+     sparse_not_uptodate_file
+} unpack_trees_error;
+#define NB_UNPACK_TREES_ERROR 6
+struct rejected_files *unpack_rejects[NB_UNPACK_TREES_ERROR];

You folks seem to like global variables a lot...  Isn't there a struct
passed throughout the callchain in unpack_trees that you can attach this
information to?

At first, I wanted to avoid of having a global variable but I was not
sure if I could add my error structure to an existing structure and I
did not want to overload the callchain with a new parameter.
So now, I attached my structure to struct unpack_trees_options.

I also corrected all the style errors and the following remarks.
Thanks.

Also "rejected_files" is not as technically correct (there are symlinks)
as "rejected_paths".

Style: we don't encourage "typedef enum { ... } unpack_trees_error";
instead we tend to just say "enum unpack_trees_error" both in the
definition and in the use.

quoted

+     if (!porcelain) {
+             error(msg,file,action);
+             return -1;
+     }

Style:
       if (!porcelain)
               return error(msg, file, action);

quoted

+static void free_rejected_files(unpack_trees_error e)
+{
+     while(unpack_rejects[e]->list) {

Style:
       while (unpack_rejects[e]->list) {

quoted

+static void display_error_msgs()
+{
+     int i;
+     int hasPorcelain = 0;

Style: we don't encourage camelCase.

Whichever way spelled, "has porcelain?" is puzzling.

Is this about "are we issuing error messages as a Porcelain program, or
are we a plumbing without noisy error messages?"  Or is this about "have
we said anything in the loop, and if so finish the message with
'Aborting'"?  If the former, I would name it after "we are Porcelain";
if the latter, I would name it after "we said something".

quoted

+     for (i=0; i<NB_UNPACK_TREES_ERROR; i++) {

Style:

       for (i = 0; i < NB_UNPACK_TREES_ERROR; i++) {

quoted

+             if (unpack_rejects[i] && unpack_rejects[i]->list) {
+                     hasPorcelain = 1;
+                     struct rejected_files_list *f = unpack_rejects[i]->list;
+                     char *action = unpack_rejects[i]->action;
+                     char *file = malloc(unpack_rejects[i]->size+1);
+                     *file = '\0';
+                     while (f) {
+                             strcat(file,"\t");
+                             strcat(file,f->file);
+                             strcat(file,"\n");
+                             f = f->next;
+                     }
+                     error(unpack_rejects[i]->msg,file,action);
+                     free_rejected_files(i);

It feels wrong to malloc() inside the loop (and without freeing, which is
worse).  At least the code should use strbuf to do something like:

       struct strbuf indented = STRBUF_INIT;
       for (f = unpack_rejects[i]->list; f; f = f->next)
               strbuf_addf(&indented, "\t%s\n", f->file);
       error(..., indented.buf, action);
       strbuf_release(&indented);