[RFC/PATCH v2] create a skeleton for the command git rebase --status

[RFC/PATCH v2] create a skeleton for the command git rebase --status

[Guillaume Pagès]

Preparatory commit for a git rebase --status command. This command
will indicate the state of the process in the rebase, and the reason
why it stopped.

Signed-off-by: Guillaume Pagès <guillaume.pages@ensimag.grenoble-inp.fr>
---

The observations from Matthieu Moy have been taken into account 

It is an almost empty code sent to validate the global architecture of
this command.  It is written in C because git status is already in C
and it seems that it is the current tendency to port shell code to
C. Moreover will likely use code from wt_status to implement this
functionnality. The command calls a helper from a shell script, as it
is made in bisect (bisect--helper.c).

 Makefile                         |  2 ++
 builtin.h                        |  1 +
 builtin/rebase--status--helper.c | 23 +++++++++++++++++++++++
 git-rebase.sh                    |  6 +++++-
 git.c                            |  1 +
 rebase--status.c                 |  6 ++++++
 rebase--status.h                 |  7 +++++++
 7 files changed, 45 insertions(+), 1 deletion(-)
 create mode 100644 builtin/rebase--status--helper.c
 create mode 100644 rebase--status.c
 create mode 100644 rebase--status.h

diff --git a/Makefile b/Makefile
index e0caec3..e3b3e63 100644
--- a/Makefile
+++ b/Makefile
@@ -853,6 +853,7 @@ LIB_OBJS += prompt.o
 LIB_OBJS += quote.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
+LIB_OBJS += rebase--status.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
 LIB_OBJS += remote.o
@@ -969,6 +970,7 @@ BUILTIN_OBJS += builtin/prune-packed.o
 BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase--status--helper.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
 BUILTIN_OBJS += builtin/remote.o
diff --git a/builtin.h b/builtin.h
index c47c110..5071a08 100644
--- a/builtin.h
+++ b/builtin.h
@@ -99,6 +99,7 @@ extern int cmd_prune(int argc, const char **argv, const char *prefix);
 extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase_status__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_remote(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase--status--helper.c b/builtin/rebase--status--helper.c
new file mode 100644
index 0000000..efda29c
--- /dev/null
+++ b/builtin/rebase--status--helper.c
@@ -0,0 +1,23 @@
+#include "builtin.h"
+#include "cache.h"
+#include "parse-options.h"
+#include "rebase--status.h"
+
+static const char * const git_rebase_status_helper_usage[] = {
+	N_("git rebase--status--helper"),
+	NULL
+};
+
+int cmd_rebase_status__helper(int argc, const char **argv, const char *prefix)
+{
+	struct option options[] = {
+		
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_rebase_status_helper_usage, 0);
+
+
+	/* next-all */
+	return rebase_status();
+}
diff --git a/git-rebase.sh b/git-rebase.sh
index 47ca3b9..4e1f3e1 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -43,6 +43,7 @@ continue!          continue
 abort!             abort and check out the original branch
 skip!              skip current patch and continue
 edit-todo!         edit the todo list during an interactive rebase
+status!            show the status of the current rebase
 "
 . git-sh-setup
 . git-sh-i18n
@@ -238,7 +239,7 @@ do
 	--verify)
 		ok_to_skip_pre_rebase=
 		;;
-	--continue|--skip|--abort|--edit-todo)
+	--continue|--skip|--abort|--edit-todo|--status)
 		test $total_argc -eq 2 || usage
 		action=${1##--}
 		;;
@@ -401,6 +402,9 @@ abort)
 edit-todo)
 	run_specific_rebase
 	;;
+status)
+	exec git rebase--status--helper
+	;;
 esac
 
 # Make sure no rebase is in progress
diff --git a/git.c b/git.c
index 9efd1a3..3ebc144 100644
--- a/git.c
+++ b/git.c
@@ -410,6 +410,7 @@ static struct cmd_struct commands[] = {
 	{ "prune-packed", cmd_prune_packed, RUN_SETUP },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "read-tree", cmd_read_tree, RUN_SETUP },
+	{ "rebase--status--helper", cmd_rebase_status__helper, RUN_SETUP },
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
 	{ "remote", cmd_remote, RUN_SETUP },
diff --git a/rebase--status.c b/rebase--status.c
new file mode 100644
index 0000000..1962349
--- /dev/null
+++ b/rebase--status.c
@@ -0,0 +1,6 @@
+#include "rebase--status.h"
+
+int rebase_status(){
+	printf("git rebase --status is not yet implemented\n");
+	return 0;
+}
diff --git a/rebase--status.h b/rebase--status.h
new file mode 100644
index 0000000..17d22a1
--- /dev/null
+++ b/rebase--status.h
@@ -0,0 +1,7 @@
+#ifndef REBASE__STATUS_H
+#define REBASE__STATUS_H
+
+
+extern int rebase_status();
+
+#endif
-- 
2.0.5.6.g818b3fd

Re: [RFC/PATCH v2] create a skeleton for the command git rebase --status

[Matthieu Moy]

Guillaume Pagès <guillaume.pages@ensimag.grenoble-inp.fr> writes:

> It is an almost empty code sent to validate the global architecture of
> this command.  It is written in C because git status is already in C
> and it seems that it is the current tendency to port shell code to
> C. Moreover will likely use code from wt_status to implement this
> functionnality. The command calls a helper from a shell script, as it
> is made in bisect (bisect--helper.c).

I think this paragraph makes sense in the commit message. My previous
remark was about the wording, not about the relevance of the arguments.

> +static const char * const git_rebase_status_helper_usage[] = {
> +	N_("git rebase--status--helper"),
> +	NULL
> +};
> +
> +int cmd_rebase_status__helper(int argc, const char **argv, const char *prefix)
> +{
> +	struct option options[] = {
> +		
> +	};

You need to tell parse_options when the array is over (that's C ...) =>
OPT_END().

> +	argc = parse_options(argc, argv, prefix, options,
> +			     git_rebase_status_helper_usage, 0);

Actually, you don't need option-parsing at all since you don't pass
arguments to the helper, but why not have this in case we need it later.

No need to resend now, you'll send a new version when you have something
to build on top (or if someone has more important remarks).

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

Re: [RFC/PATCH v2] create a skeleton for the command git rebase --status

[Guillaume Pages]

Hi, 

"Paul Tan" <pyokagan@gmail.com> : 
> I haven't kept up with the discussion, but I'm wondering: since you 
> need the functionality in wt-status.c, why not implement it in git 
> status? In fact, git-status already shows if there is a rebase in 
> progress, so why not extend it to say which patch/todo line the rebase 
> stopped on? It feels much more natural to me to use git-status to 
> check the status of the rebase, instead of git rebase --status. 
> Thanks, Paul 

It's a question I have asked myself but since git rebase --status will 
certainly display more information maybe it's better to separate the 
two commands, and to indicate the user that git rebase --status exists 
in git status during a rebase. If we choose to use git status, it 
could be an option to display the full information since it currently 
gives very few information. I'm not sure what -verbose does but it 
could be its role. 

Thanks, 

Guillaume Pagès 

Re: [RFC/PATCH v2] create a skeleton for the command git rebase --status

[Matthieu Moy]

Guillaume Pages <guillaume.pages@ensimag.grenoble-inp.fr> writes:

> Hi, 
>
> "Paul Tan" <pyokagan@gmail.com> : 
>> I haven't kept up with the discussion, but I'm wondering: since you 
>> need the functionality in wt-status.c, why not implement it in git 
>> status? In fact, git-status already shows if there is a rebase in 
>> progress, so why not extend it to say which patch/todo line the rebase 
>> stopped on? It feels much more natural to me to use git-status to 
>> check the status of the rebase, instead of git rebase --status. 
>> Thanks, Paul 
>
> It's a question I have asked myself but since git rebase --status will 
> certainly display more information 

Historically, I had the idea of "git rebase --status" and Ensimag
students started working on it 2 years ago. The same question came back
then during the discussion: why not just in status? This lead to the
header in the output of "git status" ("You are currently ...", for
rebase/am/bisect/...).

But I think there are more relevant information to show (e.g. list of
already applied commits, remaining list of commits, possibly truncated
if the list is overly long, and information that rebase gave you when
stopping like the path to the file being applied). Having them all in
"git status" would make the output really long, for little benefit in
day-to-day use.

So, to me, it makes sense to have a separate command "tell me everything
you know about the current state of the rebase".

> If we choose to use git status, it could be an option to display the
> full information since it currently gives very few information. I'm
> not sure what -verbose does but it could be its role.

"git status --verbose" is already taken for "show me the diff together
with status".

(I'm also dreaming of a "git status --tutor" that would show detailed
explanations with pointers to the documentation & all for each section
of "status". Useless for non-beginners, but could be a nice way to teach
Git to beginners)

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

Re: [RFC/PATCH v2] create a skeleton for the command git rebase --status

[Junio C Hamano]

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

> But I think there are more relevant information to show (e.g. list of
> already applied commits, remaining list of commits, possibly truncated
> if the list is overly long, and information that rebase gave you when
> stopping like the path to the file being applied). Having them all in
> "git status" would make the output really long, for little benefit in
> day-to-day use.

Sorry, I do not quite agree with this reasoning.

Isn't "git status" during a rebase that shows "really long"
information to help the rebase operation a good thing?  In
day-to-day use when you are not in the middle of rebase, the command
would not show what remains to be done, would it?

I may be biased, because I rarely use 'git status' while running
'git rebase' (with or without interactive).  But to me, 'git diff'
would be a more appropriate tool to help me unstuck in managing the
current step of conflict resolution than 'git status' gives me
during either a rebase or a merge as "Unmerged paths" anyway.

If this topic enhances 'git status' with the in-progress rebase
information, I'd view it as turning 'git status' from 'a more or
less useless command during rebase' to 'a useful command'.

Re: [RFC/PATCH v2] create a skeleton for the command git rebase --status

[Matthieu Moy]

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

> If this topic enhances 'git status' with the in-progress rebase
> information, I'd view it as turning 'git status' from 'a more or
> less useless command during rebase' to 'a useful command'.

For day-to-day operations, what we already have in status already
qualifies as 'useful command' to me:

$ git status
rebase in progress; onto 7f9a792
You are currently rebasing branch 'master' on '7f9a792'.
  (fix conflicts and then run "git rebase --continue")
  (use "git rebase --skip" to skip this patch)
  (use "git rebase --abort" to check out the original branch)

I like the output of "git status" to be concise. OTOH, there are tons of
information in .git/rebase-merge/ that could be displayed to the user.

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

Re: [RFC/PATCH v2] create a skeleton for the command git rebase --status

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> If this topic enhances 'git status' with the in-progress rebase
>> information, I'd view it as turning 'git status' from 'a more or
>> less useless command during rebase' to 'a useful command'.
>
> For day-to-day operations, what we already have in status already
> qualifies as 'useful command' to me:
>
> $ git status
> rebase in progress; onto 7f9a792
> You are currently rebasing branch 'master' on '7f9a792'.
>   (fix conflicts and then run "git rebase --continue")
>   (use "git rebase --skip" to skip this patch)
>   (use "git rebase --abort" to check out the original branch)

Not really.  How would you decide if 7f9a792 is worth keeping or
rebase is better be aborted without knowing where you are?

> I like the output of "git status" to be concise.

Sure, as long as concise and useful, I am all for it.  The above
however does not show anything I already know in my prompt.  I would
say no thanks to "concise and useless".

> OTOH, there are tons of information in .git/rebase-merge/ that
> could be displayed to the user.

Surely, that is why "git status" during a rebase should show them.

Re: [RFC/PATCH v2] create a skeleton for the command git rebase --status

[Guillaume Pages]

After this discussion I eventually agree that it would be better
upgrading git status than creating a new command.When people use git
status, it means that they need information to continue their work, so
if you don't even know that you are in a rebase, you will very likely
need information about the current rebase.

During a classic rebase we could have output like:

rebase in progress; onto d9d448a You are currently rebasing branch
'branche1' on 'd9d448a'.  (fix conflicts and then run "git rebase
--continue") (use "git rebase --skip" to skip this patch) (use "git
rebase --abort" to check out the original branch) (5 commits applied,
3 remainings) Failed to apply:

252c273 commit message

Unmerged paths: (use "git reset HEAD <file>..." to unstage) (use "git
add <file>..." to mark resolution)

both modified: file1


And during an interactive rebase:

rebase in progress; onto 247c883 You are currently editing a commit
while rebasing branch 'b1.1' on '247c883'.  (use "git commit --amend"
to amend the current commit) (use "git rebase --continue" once you are
satisfied with your changes)

Last commands done (5 commands done) :

pick 62609785 commit message1 reword 85ae9001 new commit message2

(See more at .git/rebase-merge/done)

Next commands to do (3 remainings commands) :

squash 62609785 commit message3 pick 85ae9001 commit message4

(See more at .git/rebase-merge/git-rebase-todo)

Changes not staged for commit: (use "git add <file>..." to update what
will be committed) (use "git checkout -- <file>..." to discard changes
in working directory)

modified: file1 ...

Is it a good practice to send the user find information in the .git
directory?

Thanks

Guillaume

Re: [RFC/PATCH v2] create a skeleton for the command git rebase --status

[Matthieu Moy]

Guillaume Pages <guillaume.pages@ensimag.grenoble-inp.fr> writes:

> rebase in progress; onto d9d448a You are currently rebasing branch
> 'branche1' on 'd9d448a'.  (fix conflicts and then run "git rebase
> --continue") (use "git rebase --skip" to skip this patch) (use "git
> rebase --abort" to check out the original branch) (5 commits applied,
> 3 remainings) Failed to apply:
>
> 252c273 commit message

You messed-up something with the formatting, but I agree that this would
be nice. I'd reverse order between the hints ("(use ... to ...)") and
the "Failed to apply: ...".

> And during an interactive rebase:
>
> rebase in progress;

This could even become "interactive rebase in progress".

Most of the time, you're supposed to remember whether you ran "git
rebase" with -i, but a typical use-case is when a newbie requests help
like "I don't know what I did, but can you fix it?", and then any
information can be valuable.

> Last commands done (5 commands done) :
>
> pick 62609785 commit message1 reword 85ae9001 new commit message2
>
> (See more at .git/rebase-merge/done)
>
> Next commands to do (3 remainings commands) :
>
> squash 62609785 commit message3 pick 85ae9001 commit message4
>
> (See more at .git/rebase-merge/git-rebase-todo)

Not sure the blank lines are meant to be there, but I wouldn't put them
in the actual output. I'd format it as

Last commands done (5 commands done) :
  pick 62609785 commit message1
  (see more at .git/rebase-merge/done)

(lower-case "see" to be consistant with other hints)

> Is it a good practice to send the user find information in the .git
> directory?

We usually avoid doing that and provide commands to do this (e.g. "git
rebase --edit-todo" instead of asking the user to do $EDITOR
.git/rebase-merge/git-rebase-todo), but the ones you show seem OK to me.

There's already at least one instance of this when a rebase fails:

  Patch failed at 0001 foo
  The copy of the patch that failed is found in:
     /tmp/clone/.git/rebase-apply/patch

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

Re: [RFC/PATCH v2] create a skeleton for the command git rebase --status

[Junio C Hamano]

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

> Guillaume Pages <guillaume.pages@ensimag.grenoble-inp.fr> writes:
> ...
>> And during an interactive rebase:
>>
>> rebase in progress;
>
> This could even become "interactive rebase in progress".
>
> Most of the time, you're supposed to remember whether you ran "git
> rebase" with -i, but a typical use-case is when a newbie requests help
> like "I don't know what I did, but can you fix it?", and then any
> information can be valuable.

Yeah, I like that.

> Not sure the blank lines are meant to be there, but I wouldn't put them
> in the actual output. I'd format it as
>
> Last commands done (5 commands done) :
>   pick 62609785 commit message1
>   (see more at .git/rebase-merge/done)
>
> (lower-case "see" to be consistant with other hints)

Nice.

>> Is it a good practice to send the user find information in the .git
>> directory?
>
> We usually avoid doing that and provide commands to do this (e.g. "git
> rebase --edit-todo" instead of asking the user to do $EDITOR
> .git/rebase-merge/git-rebase-todo), but the ones you show seem OK to me.

Yeah, I think it is OK to suggest peeking into (not writing) files
like this.  We may update the internal implementation but then the
suggestion can and should be updated when that happens.