threads / patch / 41240

patchstatus: be prepared for not-yet-started interactive rebase

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

## tl;dr

10 messages between Jan 22, 2016 and Jan 27, 2017. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Johannes Schindelin· Jan 22, 2016, 16:28 UTC · lore
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(-)
Show changes to wt-status.c +15 −7
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
Matthieu Moy· Jan 22, 2016, 16:45 UTC · re: Johannes Schindelin · lore

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

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/
Johannes Schindelin· Jan 22, 2016, 17:11 UTC · re: Matthieu Moy · lore

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

Hi Matthieu,
On Fri, 22 Jan 2016, Matthieu Moy wrote:
Show 7 quoted lines
> 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

Junio C Hamano· Jan 22, 2016, 18:38 UTC · re: Matthieu Moy · lore

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

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 7 quoted lines
> 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.

Johannes Schindelin· Jan 22, 2016, 19:02 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Fri, 22 Jan 2016, Junio C Hamano wrote:
Show 12 quoted lines
> 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

Johannes Schindelin· Jan 26, 2017, 16:08 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Jan 26, 2017, 16:08 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 2 files +29 −4

t/t7512-status-help.sh, wt-status.c

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
Matthieu Moy· Jan 26, 2017, 18:04 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 11 quoted lines
> 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.
Show 10 quoted lines
> @@ -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/
Stefan Beller· Jan 26, 2017, 18:02 UTC · re: Johannes Schindelin · lore

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

On Thu, Jan 26, 2017 at 8:08 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 60 quoted lines
> 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
Johannes Schindelin· Jan 27, 2017, 10:49 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Thu, 26 Jan 2017, Stefan Beller wrote:
Show 10 quoted lines
> 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

← back to recent threads