[PATCH] status: be prepared for not-yet-started interactive rebase

[PATCH] status: be prepared for not-yet-started interactive rebase

[Johannes Schindelin]

From: Junio C Hamano <gitster@pobox.com>

Some developers might want to call `git status` in a working
directory where they just started an interactive rebase, but the
edit script is still opened in the editor.

Let's show a meaningful message in such cases.

[jes: suppressed the "No commands remaining" message when the
interactive rebase has not started yet.]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index bba2596..ce8dfda 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1068,14 +1068,17 @@ static void abbrev_sha1_in_line(struct strbuf *line)
 
 }
 
-static void read_rebase_todolist(const char *fname, struct string_list *lines)
+static int read_rebase_todolist(const char *fname, struct string_list *lines)
 {
 	struct strbuf line = STRBUF_INIT;
 	FILE *f = fopen(git_path("%s", fname), "r");
 
-	if (!f)
+	if (!f) {
+		if (errno == ENOENT)
+			return -1;
 		die_errno("Could not open file %s for reading",
 			  git_path("%s", fname));
+	}
 	while (!strbuf_getline(&line, f, '\n')) {
 		if (line.len && line.buf[0] == comment_line_char)
 			continue;
@@ -1085,6 +1088,7 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)
 		abbrev_sha1_in_line(&line);
 		string_list_append(lines, line.buf);
 	}
+	return 0;
 }
 
 static void show_rebase_information(struct wt_status *s,
@@ -1098,10 +1102,12 @@ static void show_rebase_information(struct wt_status *s,
 		struct string_list have_done = STRING_LIST_INIT_DUP;
 		struct string_list yet_to_do = STRING_LIST_INIT_DUP;
 
-		read_rebase_todolist("rebase-merge/done", &have_done);
-		read_rebase_todolist("rebase-merge/git-rebase-todo", &yet_to_do);
-
-		if (have_done.nr == 0)
+		if ((read_rebase_todolist("rebase-merge/done", &have_done)) ||
+		    (read_rebase_todolist("rebase-merge/git-rebase-todo",
+				  &yet_to_do)))
+			status_printf_ln(s, color,
+				_("rebase-i not started yet."));
+		else if (have_done.nr == 0)
 			status_printf_ln(s, color, _("No commands done."));
 		else {
 			status_printf_ln(s, color,
@@ -1119,7 +1125,9 @@ static void show_rebase_information(struct wt_status *s,
 					_("  (see more in file %s)"), git_path("rebase-merge/done"));
 		}
 
-		if (yet_to_do.nr == 0)
+		if (have_done.nr == 0)
+			; /* do nothing */
+		else if (yet_to_do.nr == 0)
 			status_printf_ln(s, color,
 					 _("No commands remaining."));
 		else {
-- 
2.7.0.windows.1.7.g55a05c8

Re: [PATCH] status: be prepared for not-yet-started interactive rebase

[Matthieu Moy]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

>  wt-status.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)

Looks good to me. You may want to add a test by overriding $EDITOR to a
script doing "git status >actual" if you want to have fun with testing.

Thanks,

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

Re: [PATCH] status: be prepared for not-yet-started interactive rebase

[Johannes Schindelin]

Hi Matthieu,

On Fri, 22 Jan 2016, Matthieu Moy wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> >  wt-status.c | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> Looks good to me. You may want to add a test by overriding $EDITOR to a
> script doing "git status >actual" if you want to have fun with testing.

Sadly, I lack the time.

Ciao,
Dscho

Re: [PATCH] status: be prepared for not-yet-started interactive rebase

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>>  wt-status.c | 22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> Looks good to me. You may want to add a test by overriding $EDITOR to a
> script doing "git status >actual" if you want to have fun with testing.

I am unhappy that the code does not read 'rebase-todo' at all when
'done' is missing.

If we cannot read 'todo', that would mean we shouldn't be in this
function in the first place, which is a sign of something more
serious; somebody created 'interactive' but did not leave 'todo' to
read for us--why?

A missing 'done' is much more benign and making us not to barf is a
worthy thing to do, but we shouldn't be removing protection against
a more serious error as its side effect.  That is what I am unhappy
about this change.

Re: [PATCH] status: be prepared for not-yet-started interactive rebase

[Johannes Schindelin]

Hi Junio,

On Fri, 22 Jan 2016, Junio C Hamano wrote:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >
> >>  wt-status.c | 22 +++++++++++++++-------
> >>  1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > Looks good to me. You may want to add a test by overriding $EDITOR to a
> > script doing "git status >actual" if you want to have fun with testing.
> 
> I am unhappy that the code does not read 'rebase-todo' at all when
> 'done' is missing.

At this point is virtually certain that the edit script is opened in an
editor, and very likely to change. So it makes little sense to look at the
todo.

Ciao,
Dscho

[PATCH v2 0/1] Let `git status` handle a not-yet-started `rebase -i` gracefully

[Johannes Schindelin]

When the `done` file is missing, we die()d. This is not necessary, we
can do much better than that.

Changes since v1:

- When `done` is missing, we still read `git-rebase-todo` and report the
  next steps.

- We now report a missing git-rebase-todo.

- Added a test (thanks, Matthieu, for prodding me into working harder
  ;-)).

- As I changed so much, I took authorship of the patch.


Johannes Schindelin (1):
  status: be prepared for not-yet-started interactive rebase

 t/t7512-status-help.sh | 19 +++++++++++++++++++
 wt-status.c            | 14 ++++++++++----
 2 files changed, 29 insertions(+), 4 deletions(-)


base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Published-As: https://github.com/dscho/git/releases/tag/wt-status-v2
Fetch-It-Via: git fetch https://github.com/dscho/git wt-status-v2

Interdiff vs v1:

 diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
 index 5c3db656df..458608cc1e 100755
 --- a/t/t7512-status-help.sh
 +++ b/t/t7512-status-help.sh
 @@ -944,4 +944,23 @@ EOF
  	test_i18ncmp expected actual
  '
  
 +test_expect_success 'status: handle not-yet-started rebase -i gracefully' '
 +	ONTO=$(git rev-parse --short HEAD^) &&
 +	COMMIT=$(git rev-parse --short HEAD) &&
 +	EDITOR="git status --untracked-files=no >actual" git rebase -i HEAD^ &&
 +	cat >expected <<EOF &&
 +On branch several_commits
 +No commands done.
 +Next command to do (1 remaining command):
 +   pick $COMMIT four_commit
 +  (use "git rebase --edit-todo" to view and edit)
 +You are currently editing a commit while rebasing branch '\''several_commits'\'' on '\''$ONTO'\''.
 +  (use "git commit --amend" to amend the current commit)
 +  (use "git rebase --continue" once you are satisfied with your changes)
 +
 +nothing to commit (use -u to show untracked files)
 +EOF
 +	test_i18ncmp expected actual
 +'
 +
  test_done
 diff --git a/wt-status.c b/wt-status.c
 index 13afe66649..4dff0b3e21 100644
 --- a/wt-status.c
 +++ b/wt-status.c
 @@ -1169,12 +1169,12 @@ static void show_rebase_information(struct wt_status *s,
  		struct string_list have_done = STRING_LIST_INIT_DUP;
  		struct string_list yet_to_do = STRING_LIST_INIT_DUP;
  
 -		if ((read_rebase_todolist("rebase-merge/done", &have_done)) ||
 -		    (read_rebase_todolist("rebase-merge/git-rebase-todo",
 -				  &yet_to_do)))
 +		read_rebase_todolist("rebase-merge/done", &have_done);
 +		if (read_rebase_todolist("rebase-merge/git-rebase-todo",
 +					 &yet_to_do))
  			status_printf_ln(s, color,
 -				_("rebase-i not started yet."));
 -		else if (have_done.nr == 0)
 +				_("git-rebase-todo is missing."));
 +		if (have_done.nr == 0)
  			status_printf_ln(s, color, _("No commands done."));
  		else {
  			status_printf_ln(s, color,
 @@ -1192,9 +1192,7 @@ static void show_rebase_information(struct wt_status *s,
  					_("  (see more in file %s)"), git_path("rebase-merge/done"));
  		}
  
 -		if (have_done.nr == 0)
 -			; /* do nothing */
 -		else if (yet_to_do.nr == 0)
 +		if (yet_to_do.nr == 0)
  			status_printf_ln(s, color,
  					 _("No commands remaining."));
  		else {

-- 
2.11.1.windows.prerelease.2.9.g3014b57

[PATCH v2 1/1] status: be prepared for not-yet-started interactive rebase

[Johannes Schindelin]

Some developers might want to call `git status` in a working
directory where they just started an interactive rebase, but the
edit script is still opened in the editor.

Let's show a meaningful message in such cases.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t7512-status-help.sh | 19 +++++++++++++++++++
 wt-status.c            | 14 ++++++++++----
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 5c3db656df..458608cc1e 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -944,4 +944,23 @@ EOF
 	test_i18ncmp expected actual
 '
 
+test_expect_success 'status: handle not-yet-started rebase -i gracefully' '
+	ONTO=$(git rev-parse --short HEAD^) &&
+	COMMIT=$(git rev-parse --short HEAD) &&
+	EDITOR="git status --untracked-files=no >actual" git rebase -i HEAD^ &&
+	cat >expected <<EOF &&
+On branch several_commits
+No commands done.
+Next command to do (1 remaining command):
+   pick $COMMIT four_commit
+  (use "git rebase --edit-todo" to view and edit)
+You are currently editing a commit while rebasing branch '\''several_commits'\'' on '\''$ONTO'\''.
+  (use "git commit --amend" to amend the current commit)
+  (use "git rebase --continue" once you are satisfied with your changes)
+
+nothing to commit (use -u to show untracked files)
+EOF
+	test_i18ncmp expected actual
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index a715e71906..4dff0b3e21 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1135,14 +1135,17 @@ static void abbrev_sha1_in_line(struct strbuf *line)
 	strbuf_list_free(split);
 }
 
-static void read_rebase_todolist(const char *fname, struct string_list *lines)
+static int read_rebase_todolist(const char *fname, struct string_list *lines)
 {
 	struct strbuf line = STRBUF_INIT;
 	FILE *f = fopen(git_path("%s", fname), "r");
 
-	if (!f)
+	if (!f) {
+		if (errno == ENOENT)
+			return -1;
 		die_errno("Could not open file %s for reading",
 			  git_path("%s", fname));
+	}
 	while (!strbuf_getline_lf(&line, f)) {
 		if (line.len && line.buf[0] == comment_line_char)
 			continue;
@@ -1152,6 +1155,7 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)
 		abbrev_sha1_in_line(&line);
 		string_list_append(lines, line.buf);
 	}
+	return 0;
 }
 
 static void show_rebase_information(struct wt_status *s,
@@ -1166,8 +1170,10 @@ static void show_rebase_information(struct wt_status *s,
 		struct string_list yet_to_do = STRING_LIST_INIT_DUP;
 
 		read_rebase_todolist("rebase-merge/done", &have_done);
-		read_rebase_todolist("rebase-merge/git-rebase-todo", &yet_to_do);
-
+		if (read_rebase_todolist("rebase-merge/git-rebase-todo",
+					 &yet_to_do))
+			status_printf_ln(s, color,
+				_("git-rebase-todo is missing."));
 		if (have_done.nr == 0)
 			status_printf_ln(s, color, _("No commands done."));
 		else {
-- 
2.11.1.windows.prerelease.2.9.g3014b57

Re: [PATCH v2 1/1] status: be prepared for not-yet-started interactive rebase

[Stefan Beller]

On Thu, Jan 26, 2017 at 8:08 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Some developers might want to call `git status` in a working
> directory where they just started an interactive rebase, but the
> edit script is still opened in the editor.
>
> Let's show a meaningful message in such cases.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t7512-status-help.sh | 19 +++++++++++++++++++
>  wt-status.c            | 14 ++++++++++----
>  2 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
> index 5c3db656df..458608cc1e 100755
> --- a/t/t7512-status-help.sh
> +++ b/t/t7512-status-help.sh
> @@ -944,4 +944,23 @@ EOF
>         test_i18ncmp expected actual
>  '
>
> +test_expect_success 'status: handle not-yet-started rebase -i gracefully' '
> +       ONTO=$(git rev-parse --short HEAD^) &&
> +       COMMIT=$(git rev-parse --short HEAD) &&
> +       EDITOR="git status --untracked-files=no >actual" git rebase -i HEAD^ &&
> +       cat >expected <<EOF &&
> +On branch several_commits
> +No commands done.
> +Next command to do (1 remaining command):
> +   pick $COMMIT four_commit
> +  (use "git rebase --edit-todo" to view and edit)
> +You are currently editing a commit while rebasing branch '\''several_commits'\'' on '\''$ONTO'\''.
> +  (use "git commit --amend" to amend the current commit)
> +  (use "git rebase --continue" once you are satisfied with your changes)
> +
> +nothing to commit (use -u to show untracked files)
> +EOF
> +       test_i18ncmp expected actual
> +'
> +
>  test_done
> diff --git a/wt-status.c b/wt-status.c
> index a715e71906..4dff0b3e21 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1135,14 +1135,17 @@ static void abbrev_sha1_in_line(struct strbuf *line)
>         strbuf_list_free(split);
>  }
>
> -static void read_rebase_todolist(const char *fname, struct string_list *lines)
> +static int read_rebase_todolist(const char *fname, struct string_list *lines)
>  {
>         struct strbuf line = STRBUF_INIT;
>         FILE *f = fopen(git_path("%s", fname), "r");
>
> -       if (!f)
> +       if (!f) {
> +               if (errno == ENOENT)
> +                       return -1;
>                 die_errno("Could not open file %s for reading",
>                           git_path("%s", fname));

While at it, fix the translation with die_errno(_(..),..) ?
(The errno message is translated already by the system,
which make untranslated die_errno things awkward for the users.)

Otherwise the patch looks good to me

Re: [PATCH v2 1/1] status: be prepared for not-yet-started interactive rebase

[Johannes Schindelin]

Hi Stefan,

On Thu, 26 Jan 2017, Stefan Beller wrote:

> On Thu, Jan 26, 2017 at 8:08 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > -       if (!f)
> > +       if (!f) {
> > +               if (errno == ENOENT)
> > +                       return -1;
> >                 die_errno("Could not open file %s for reading",
> >                           git_path("%s", fname));
> 
> While at it, fix the translation with die_errno(_(..),..) ?

That is not the purpose of my patch. But feel free to offer a follow-up
patch!

Ciao,
Johannes

Re: [PATCH v2 1/1] status: be prepared for not-yet-started interactive rebase

[Matthieu Moy]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Some developers might want to call `git status` in a working
> directory where they just started an interactive rebase, but the
> edit script is still opened in the editor.
>
> Let's show a meaningful message in such cases.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t7512-status-help.sh | 19 +++++++++++++++++++
>  wt-status.c            | 14 ++++++++++----
>  2 files changed, 29 insertions(+), 4 deletions(-)

The patch looks good to me.

> @@ -1166,8 +1170,10 @@ static void show_rebase_information(struct wt_status *s,
>  		struct string_list yet_to_do = STRING_LIST_INIT_DUP;
>  
>  		read_rebase_todolist("rebase-merge/done", &have_done);
> -		read_rebase_todolist("rebase-merge/git-rebase-todo", &yet_to_do);
> -
> +		if (read_rebase_todolist("rebase-merge/git-rebase-todo",
> +					 &yet_to_do))
> +			status_printf_ln(s, color,
> +				_("git-rebase-todo is missing."));

I first was surprised not to see this "git-rebase-todo" in the output of
status, but the testcase tests a missing 'done', not a missing todo, so
it's normal.

Thanks,

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