Re: [PATCH v4 2/8] status: cleanup API to wt_status_print

[Jeff Hostetler]

On 08/03/2016 05:36 PM, Junio C Hamano wrote:

Jeff Hostetler  writes:


From: Jeff Hostetler 
diff --git a/wt-status.h b/wt-status.h
index 2023a3c..a859a12 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -43,6 +43,15 @@ struct wt_status_change_data {
unsigned new_submodule_commits : 1;
  };

+ enum wt_status_format {
+   STATUS_FORMAT_NONE = 0,
+   STATUS_FORMAT_LONG,
+   STATUS_FORMAT_SHORT,
+   STATUS_FORMAT_PORCELAIN,
+
+   STATUS_FORMAT_UNSPECIFIED
+ };


Is it your editor, or is it your MUA?  This definition is indented
by one SP, which is funny.

Also throughout the series, I saw a handful of blank lines that
should be empty but are not (e.g. three tabs and nothing else on a
new line).  I've fixed them up all but I won't be sending an
interdiff just for them, so please make sure they won't resurface
when/if you reroll.


That's odd.  I'll double check everything and trim
them in case I need to resubmit this.  Sorry.

Jeff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/8] status: cleanup API to wt_status_print

[Junio C Hamano]

Jeff Hostetler  writes:

> From: Jeff Hostetler 
>
> Refactor the API between builtin/commit.c and wt-status.[ch].
> Hide details of the various wt_*status_print() routines inside
> wt-status.c behind a single (new) wt_status_print() routine
> and eliminate the switch statements from builtin/commit.c
>
> This will allow us to more easily add new status formats
> and isolate that within wt-status.c
>
> Signed-off-by: Jeff Hostetler 
> Signed-off-by: Jeff Hostetler 

Again, are these the same person?  I won't repeat this for the
remainder of the series.

> diff --git a/wt-status.h b/wt-status.h
> index 2023a3c..a859a12 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -43,6 +43,15 @@ struct wt_status_change_data {
>   unsigned new_submodule_commits : 1;
>  };
>  
> + enum wt_status_format {
> + STATUS_FORMAT_NONE = 0,
> + STATUS_FORMAT_LONG,
> + STATUS_FORMAT_SHORT,
> + STATUS_FORMAT_PORCELAIN,
> +
> + STATUS_FORMAT_UNSPECIFIED
> + };

Is it your editor, or is it your MUA?  This definition is indented
by one SP, which is funny.

Also throughout the series, I saw a handful of blank lines that
should be empty but are not (e.g. three tabs and nothing else on a
new line).  I've fixed them up all but I won't be sending an
interdiff just for them, so please make sure they won't resurface
when/if you reroll.

>  struct wt_status {
>   int is_initial;
>   char *branch;
> @@ -66,6 +75,8 @@ struct wt_status {
>   int show_branch;
>   int hints;
>  
> + enum wt_status_format status_format;
> +
>   /* These are computed during processing of the individual sections */
>   int commitable;
>   int workdir_dirty;
> @@ -99,6 +110,7 @@ struct wt_status_state {
>  void wt_status_truncate_message_at_cut_line(struct strbuf *);
>  void wt_status_add_cut_line(FILE *fp);
>  void wt_status_prepare(struct wt_status *s);
> +void wt_status_print(struct wt_status *s);
>  void wt_status_collect(struct wt_status *s);
>  void wt_status_get_state(struct wt_status_state *state, int 
> get_detached_from);
>  int wt_status_check_rebase(const struct worktree *wt,
> @@ -106,10 +118,6 @@ int wt_status_check_rebase(const struct worktree *wt,
>  int wt_status_check_bisect(const struct worktree *wt,
>  struct wt_status_state *state);
>  
> -void wt_longstatus_print(struct wt_status *s);
> -void wt_shortstatus_print(struct wt_status *s);
> -void wt_porcelain_print(struct wt_status *s);
> -

I think I agreed during the previous review round that shifting the
division of labor boundary between the command and wt-status.[ch]
this way, to have the latter do more printing, is a good idea, and I
still do.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/8] status: cleanup API to wt_status_print

[Jeff Hostetler]

From: Jeff Hostetler 

Refactor the API between builtin/commit.c and wt-status.[ch].
Hide details of the various wt_*status_print() routines inside
wt-status.c behind a single (new) wt_status_print() routine
and eliminate the switch statements from builtin/commit.c

This will allow us to more easily add new status formats
and isolate that within wt-status.c

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c | 51 +--
 wt-status.c  | 25 ++---
 wt-status.h  | 16 
 3 files changed, 43 insertions(+), 49 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index b80273b..a792deb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -142,14 +142,7 @@ static int show_ignored_in_status, have_option_m;
 static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
-static enum status_format {
-   STATUS_FORMAT_NONE = 0,
-   STATUS_FORMAT_LONG,
-   STATUS_FORMAT_SHORT,
-   STATUS_FORMAT_PORCELAIN,
-
-   STATUS_FORMAT_UNSPECIFIED
-} status_format = STATUS_FORMAT_UNSPECIFIED;
+static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
@@ -500,24 +493,11 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
s->fp = fp;
s->nowarn = nowarn;
s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+   s->status_format = status_format;
+   s->ignore_submodule_arg = ignore_submodule_arg;
 
wt_status_collect(s);
-
-   switch (status_format) {
-   case STATUS_FORMAT_SHORT:
-   wt_shortstatus_print(s);
-   break;
-   case STATUS_FORMAT_PORCELAIN:
-   wt_porcelain_print(s);
-   break;
-   case STATUS_FORMAT_UNSPECIFIED:
-   die("BUG: finalize_deferred_config() should have been called");
-   break;
-   case STATUS_FORMAT_NONE:
-   case STATUS_FORMAT_LONG:
-   wt_longstatus_print(s);
-   break;
-   }
+   wt_status_print(s);
 
return s->commitable;
 }
@@ -1099,7 +1079,7 @@ static const char *read_commit_message(const char *name)
  * is not in effect here.
  */
 static struct status_deferred_config {
-   enum status_format status_format;
+   enum wt_status_format status_format;
int show_branch;
 } status_deferred_config = {
STATUS_FORMAT_UNSPECIFIED,
@@ -1381,6 +1361,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
 
s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
s.ignore_submodule_arg = ignore_submodule_arg;
+   s.status_format = status_format;
+   s.verbose = verbose;
+
wt_status_collect();
 
if (0 <= fd)
@@ -1389,23 +1372,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
if (s.relative_paths)
s.prefix = prefix;
 
-   switch (status_format) {
-   case STATUS_FORMAT_SHORT:
-   wt_shortstatus_print();
-   break;
-   case STATUS_FORMAT_PORCELAIN:
-   wt_porcelain_print();
-   break;
-   case STATUS_FORMAT_UNSPECIFIED:
-   die("BUG: finalize_deferred_config() should have been called");
-   break;
-   case STATUS_FORMAT_NONE:
-   case STATUS_FORMAT_LONG:
-   s.verbose = verbose;
-   s.ignore_submodule_arg = ignore_submodule_arg;
-   wt_longstatus_print();
-   break;
-   }
+   wt_status_print();
return 0;
 }
 
diff --git a/wt-status.c b/wt-status.c
index b9a58fd..a9031e4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1447,7 +1447,7 @@ static void wt_longstatus_print_state(struct wt_status *s,
show_bisect_in_progress(s, state, state_color);
 }
 
-void wt_longstatus_print(struct wt_status *s)
+static void wt_longstatus_print(struct wt_status *s)
 {
const char *branch_color = color(WT_STATUS_ONBRANCH, s);
const char *branch_status_color = color(WT_STATUS_HEADER, s);
@@ -1714,7 +1714,7 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
fputc(s->null_termination ? '\0' : '\n', s->fp);
 }
 
-void wt_shortstatus_print(struct wt_status *s)
+static void wt_shortstatus_print(struct wt_status *s)
 {
int i;
 
@@ -1746,7 +1746,7 @@ void wt_shortstatus_print(struct wt_status *s)
}
 }
 
-void wt_porcelain_print(struct wt_status *s)
+static void wt_porcelain_print(struct wt_status *s)
 {
s->use_color = 0;
s->relative_paths = 0;
@@ -1754,3 +1754,22 @@ void wt_porcelain_print(struct wt_status *s)
s->no_gettext = 1;
wt_shortstatus_print(s);
 }
+
+void wt_status_print(struct wt_status *s)
+{
+   switch