Re: [PATCH 1/4] status: factor two rebase-related messages together

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 1/4] status: factor two rebase-related messages together

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:05:13

Guillaume Pagès  [off-list ref] writes:
---
 wt-status.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

Hmmm, it obviously does not break anything but it is not obvious why
this is a good change.

Is it that you wanted to have a single instance of "if on a branch,
we say 'you are rebasing that branch', otherwise we say 'you are
rebasing'"?  Even then, I am not sure if this code movement was the
best way to do so (an obvious alternative is to use a shared helper
function and call from the two arms of if/elseif/... chain).
quoted hunk
diff --git a/wt-status.c b/wt-status.c
index 33452f1..fec6e85 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1032,7 +1032,7 @@ static void show_rebase_in_progress(struct wt_status *s,
 {
 	struct stat st;
 
-	if (has_unmerged(s)) {
+	if (has_unmerged(s) || state->rebase_in_progress || !stat(git_path("MERGE_MSG"), &st)) {
 		if (state->branch)
 			status_printf_ln(s, color,
 					 _("You are currently rebasing branch '%s' on '%s'."),
@@ -1042,25 +1042,17 @@ static void show_rebase_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 					 _("You are currently rebasing."));
 		if (s->hints) {
-			status_printf_ln(s, color,
-				_("  (fix conflicts and then run \"git rebase --continue\")"));
-			status_printf_ln(s, color,
-				_("  (use \"git rebase --skip\" to skip this patch)"));
-			status_printf_ln(s, color,
-				_("  (use \"git rebase --abort\" to check out the original branch)"));
+			if (has_unmerged(s)) {
+				status_printf_ln(s, color,
+					_("  (fix conflicts and then run \"git rebase --continue\")"));
+				status_printf_ln(s, color,
+					_("  (use \"git rebase --skip\" to skip this patch)"));
+				status_printf_ln(s, color,
+					_("  (use \"git rebase --abort\" to check out the original branch)"));
+			} else
+				status_printf_ln(s, color,
+					_("  (all conflicts fixed: run \"git rebase --continue\")"));
 		}
-	} else if (state->rebase_in_progress || !stat(git_path("MERGE_MSG"), &st)) {
-		if (state->branch)
-			status_printf_ln(s, color,
-					 _("You are currently rebasing branch '%s' on '%s'."),
-					 state->branch,
-					 state->onto);
-		else
-			status_printf_ln(s, color,
-					 _("You are currently rebasing."));
-		if (s->hints)
-			status_printf_ln(s, color,
-				_("  (all conflicts fixed: run \"git rebase --continue\")"));
 	} else if (split_commit_in_progress(s)) {
 		if (state->branch)
 			status_printf_ln(s, color,

Re: [PATCH 1/4] status: factor two rebase-related messages together

From: Guillaume Pages <hidden>
Date: 2016-06-15 23:05:13

"Junio C Hamano" [off-list ref] writes:
Hmmm, it obviously does not break anything but it is not obvious why 
this is a good change. 
Is it that you wanted to have a single instance of "if on a branch, 
we say 'you are rebasing that branch', otherwise we say 'you are 
rebasing'"? Even then, I am not sure if this code movement was the 
best way to do so (an obvious alternative is to use a shared helper 
function and call from the two arms of if/elseif/... chain). 
I made this change because at first sight, this piece of code was 
difficult to read for me. There was two long branches very similar
and I had to spot the differences, and the actual differences were
at the very end of the branches so I had to check back what the
condition was about. It seems now much more natural to me: the part
in common of both branches is in OR-condition and the differences 
between branches are gathered with the test on the variable they
depend.

By the way, I agree that this change is not absolutely necessary.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help