Re: [PATCH 5/5] sequencer: do not invent whitespace when transforming OIDs

[Liam Beguin]

Hi Johannes,

On 23 December 2017 at 00:56, Johannes Schindelin
 wrote:
> For commands that do not have an argument, there is no need to append a
> trailing space at the end of the line.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 5632415ea2d..970842e3fcc 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2584,7 +2584,10 @@ int transform_todos(unsigned flags)
> strbuf_addf(, " %s", oid);
> }
> /* add all the rest */
> -   strbuf_addf(, " %.*s\n", item->arg_len, item->arg);
> +   if (!item->arg_len)
> +   strbuf_addch(, '\n');
> +   else
> +   strbuf_addf(, " %.*s\n", item->arg_len, 
> item->arg);

I also went with that when I was working on this but I thought leaving the
extra whitespace would make the code a little shorter.
Other than that, this change and the others look good.

> }
>
> i = write_message(buf.buf, buf.len, todo_file, 0);
> --
> 2.15.1.windows.2

Thanks,
Liam

Re: [PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Liam Beguin]

Hi Junio,


On 27 December 2017 at 20:15, Junio C Hamano <gits...@pobox.com> wrote:
> Duy Nguyen <pclo...@gmail.com> writes:
>
>> On Mon, Dec 4, 2017 at 5:17 AM, Liam Beguin <liambeg...@gmail.com> wrote:
>>> +static const char command_to_char(const enum todo_command command)
>>> +{
>>> +   if (command < TODO_COMMENT && todo_command_info[command].c)
>>> +   return todo_command_info[command].c;
>>> +   return comment_line_char;
>>> +}
>>
>> CC sequencer.o
>> sequencer.c:798:19: error: type qualifiers ignored on function return
>> type [-Werror=ignored-qualifiers]
>>  static const char command_to_char(const enum todo_command command)
>>^
>>
>> Maybe drop the first const.
>
> Thanks.  This topic has been in 'next' for quite some time and I
> wanted to merge it down to 'master' soonish, so I've added the
> following before doing so.

Thanks for taking the time. I had prepared the patch but was waiting to get
home to send it.
Only comment I have, maybe s/sequencer.c/rebase -i/ in the subject line
so it matches with the rest.

Since this came up, would it be a good thing to add -Wignored-qualifiers
to the DEVELOPER flags?

>
> -- >8 --
> From: Junio C Hamano <gits...@pobox.com>
> Date: Wed, 27 Dec 2017 11:12:45 -0800
> Subject: [PATCH] sequencer.c: drop 'const' from function return type
>
> With -Werror=ignored-qualifiers, a function that claims to return
> "const char" gets this error:
>
> CC sequencer.o
> sequencer.c:798:19: error: type qualifiers ignored on function return
> type [-Werror=ignored-qualifiers]
>  static const char command_to_char(const enum todo_command command)
>^
>
> Reported-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
> Signed-off-by: Junio C Hamano <gits...@pobox.com>
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 115085d39c..2a407cbe54 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -795,7 +795,7 @@ static const char *command_to_string(const enum 
> todo_command command)
> die("Unknown command: %d", command);
>  }
>
> -static const char command_to_char(const enum todo_command command)
> +static char command_to_char(const enum todo_command command)
>  {
> if (command < TODO_COMMENT && todo_command_info[command].c)
> return todo_command_info[command].c;
> --
> 2.15.1-597-g62d91a8972
>

Thanks,
Liam

Re: [PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Liam Beguin]

Hi Duy,

On Mon, 25 Dec 2017 at 07:48 Duy Nguyen <pclo...@gmail.com> wrote:
>
> On Mon, Dec 4, 2017 at 5:17 AM, Liam Beguin <liambeg...@gmail.com> wrote:
> > +static const char command_to_char(const enum todo_command command)
> > +{
> > +   if (command < TODO_COMMENT && todo_command_info[command].c)
> > +   return todo_command_info[command].c;
> > +   return comment_line_char;
> > +}
>
> CC sequencer.o
> sequencer.c:798:19: error: type qualifiers ignored on function return
> type [-Werror=ignored-qualifiers]
>  static const char command_to_char(const enum todo_command command)
>^
>
> Maybe drop the first const.

Sorry, that's another copy-edit error I made that slipped through...
I'm curious, how did you build to get this error to show?
I tried with the DEVELOPER 'flag' but nothing showed and -Wextra gave
way too much messages...
Did you just add -Wignored-qualifiers to CFLAGS?

> --
> Duy

Thanks,
Liam

Re: [PATCH v2 0/9] rebase -i: add config to abbreviate command names

[liam Beguin]

On 05/12/17 05:21 PM, Junio C Hamano wrote:
> Liam Beguin <liambeg...@gmail.com> writes:
> 
>> This series will add the 'rebase.abbreviateCommands' configuration
>> option to allow `git rebase -i` to default to the single-letter command
>> names when generating the todo list.
>>
>> Using single-letter command names can present two benefits. First, it
>> makes it easier to change the action since you only need to replace a
>> single character (i.e.: in vim "r" instead of
>> "ciw").  Second, using this with a large enough value of
>> 'core.abbrev' enables the lines of the todo list to remain aligned
>> making the files easier to read.
>>
>> Changes in V2:
>> - Refactor and rename 'transform_todo_ids'
>> - Replace SHA-1 by OID in rebase--helper.c
>> - Update todo list related functions to take a generic 'flags' parameter
>> - Rename 'add_exec_commands' function to 'sequencer_add_exec_commands'
>> - Rename 'add-exec' option to 'add-exec-commands'
>> - Use 'strbur_read_file' instead of rewriting it
>> - Make 'command_to_char' return 'comment_char_line' if no single-letter
>>   command name is defined
>> - Combine both tests into a single test case
>> - Update commit messages
>>
>> Changes in V2:
>> - Rename 'transform_todo_insn' to 'transform_todos'
>> - Fix flag name TODO_LIST_SHORTE{D,N}_IDS
> 
> I've replaced this series and pushed out the result.

Great! Thanks again,

> 
> Thanks.
> 

Liam

[PATCH v3 9/9] t3404: add test case for abbreviated commands

[Liam Beguin]

Make sure the todo list ends up using single-letter command
abbreviations when the rebase.abbreviateCommands is enabled.
This configuration option should not change anything else.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 t/t3404-rebase-interactive.sh | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6a82d1ed876d..481a3500900d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1260,6 +1260,28 @@ test_expect_success 'rebase -i respects 
rebase.missingCommitsCheck = error' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and 
exec' '
+   rebase_setup_and_clean abbrevcmd &&
+   test_commit "first" file1.txt "first line" first &&
+   test_commit "second" file1.txt "another line" second &&
+   test_commit "fixup! first" file2.txt "first line again" first_fixup &&
+   test_commit "squash! second" file1.txt "another line here" 
second_squash &&
+   cat >expected <<-EOF &&
+   p $(git rev-list --abbrev-commit -1 first) first
+   f $(git rev-list --abbrev-commit -1 first_fixup) fixup! first
+   x git show HEAD
+   p $(git rev-list --abbrev-commit -1 second) second
+   s $(git rev-list --abbrev-commit -1 second_squash) squash! second
+   x git show HEAD
+   EOF
+   git checkout abbrevcmd &&
+   set_cat_todo_editor &&
+   test_config rebase.abbreviateCommands true &&
+   test_must_fail git rebase -i --exec "git show HEAD" \
+   --autosquash master >actual &&
+   test_cmp expected actual
+'
+
 test_expect_success 'static check of bad command' '
rebase_setup_and_clean bad-cmd &&
set_fake_editor &&
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 5/9] rebase -i: replace reference to sha1 with oid

[Liam Beguin]

Since we are trying to abstract the hash function name elsewhere in the
code base, lets use OID instead of SHA-1 in the rebase--helper too.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 builtin/rebase--helper.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 8ad4779d1650..c3b8e4d401f8 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
struct replay_opts opts = REPLAY_OPTS_INIT;
int keep_empty = 0;
enum {
-   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
+   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
} command = 0;
struct option options[] = {
@@ -27,9 +27,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_CMDMODE(0, "make-script", ,
N_("make rebase script"), MAKE_SCRIPT),
OPT_CMDMODE(0, "shorten-ids", ,
-   N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
+   N_("shorten commit ids in the todo list"), 
SHORTEN_OIDS),
OPT_CMDMODE(0, "expand-ids", ,
-   N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
+   N_("expand commit ids in the todo list"), EXPAND_OIDS),
OPT_CMDMODE(0, "check-todo-list", ,
N_("check the todo list"), CHECK_TODO_LIST),
OPT_CMDMODE(0, "skip-unnecessary-picks", ,
@@ -54,9 +54,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-   if (command == SHORTEN_SHA1S && argc == 1)
+   if (command == SHORTEN_OIDS && argc == 1)
return !!transform_todos(1);
-   if (command == EXPAND_SHA1S && argc == 1)
+   if (command == EXPAND_OIDS && argc == 1)
return !!transform_todos(0);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 0/9] rebase -i: add config to abbreviate command names

[Liam Beguin]

Hi everyone,

This series will add the 'rebase.abbreviateCommands' configuration
option to allow `git rebase -i` to default to the single-letter command
names when generating the todo list.

Using single-letter command names can present two benefits. First, it
makes it easier to change the action since you only need to replace a
single character (i.e.: in vim "r" instead of
"ciw").  Second, using this with a large enough value of
'core.abbrev' enables the lines of the todo list to remain aligned
making the files easier to read.

Changes in V2:
- Refactor and rename 'transform_todo_ids'
- Replace SHA-1 by OID in rebase--helper.c
- Update todo list related functions to take a generic 'flags' parameter
- Rename 'add_exec_commands' function to 'sequencer_add_exec_commands'
- Rename 'add-exec' option to 'add-exec-commands'
- Use 'strbur_read_file' instead of rewriting it
- Make 'command_to_char' return 'comment_char_line' if no single-letter
  command name is defined
- Combine both tests into a single test case
- Update commit messages

Changes in V2:
- Rename 'transform_todo_insn' to 'transform_todos'
- Fix flag name TODO_LIST_SHORTE{D,N}_IDS

Liam Beguin (9):
  Documentation: move rebase.* configs to new file
  Documentation: use preferred name for the 'todo list' script
  rebase -i: set commit to null in exec commands
  rebase -i: refactor transform_todo_ids
  rebase -i: replace reference to sha1 with oid
  rebase -i: update functions to use a flags parameter
  rebase -i -x: add exec commands via the rebase--helper
  rebase -i: learn to abbreviate command names
  t3404: add test case for abbreviated commands

 Documentation/config.txt|  31 +---
 Documentation/git-rebase.txt|  19 +
 Documentation/rebase-config.txt |  52 +
 builtin/rebase--helper.c|  29 +---
 git-rebase--interactive.sh  |  23 +-
 sequencer.c | 126 +---
 sequencer.h |  10 ++-
 t/t3404-rebase-interactive.sh   |  22 ++
 8 files changed, 186 insertions(+), 126 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 6/9] rebase -i: update functions to use a flags parameter

[Liam Beguin]

Update functions used in the rebase--helper so that they take a generic
'flags' parameter instead of a growing list of options.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 builtin/rebase--helper.c | 13 +++--
 sequencer.c  |  9 +
 sequencer.h  |  8 +---
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index c3b8e4d401f8..1102ecb43b67 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   int keep_empty = 0;
+   unsigned flags = 0, keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
@@ -48,16 +48,17 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
argc = parse_options(argc, argv, NULL, options,
builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
+   flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+   flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
+
if (command == CONTINUE && argc == 1)
return !!sequencer_continue();
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
-   return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-   if (command == SHORTEN_OIDS && argc == 1)
-   return !!transform_todos(1);
-   if (command == EXPAND_OIDS && argc == 1)
-   return !!transform_todos(0);
+   return !!sequencer_make_script(stdout, argc, argv, flags);
+   if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
+   return !!transform_todos(flags);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index c9a661a8c4bd..8b0dd610c881 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2444,14 +2444,15 @@ void append_signoff(struct strbuf *msgbuf, int 
ignore_footer, unsigned flag)
strbuf_release();
 }
 
-int sequencer_make_script(int keep_empty, FILE *out,
-   int argc, const char **argv)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+ unsigned flags)
 {
char *format = NULL;
struct pretty_print_context pp = {0};
struct strbuf buf = STRBUF_INIT;
struct rev_info revs;
struct commit *commit;
+   int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 
init_revisions(, NULL);
revs.verbose_header = 1;
@@ -2494,7 +2495,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todos(int shorten_ids)
+int transform_todos(unsigned flags)
 {
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
@@ -2522,7 +2523,7 @@ int transform_todos(int shorten_ids)
 
/* add commit id */
if (item->commit) {
-   const char *oid = shorten_ids ?
+   const char *oid = flags & TODO_LIST_SHORTEN_IDS ?
  short_commit_name(item->commit) :
  oid_to_hex(>commit->object.oid);
 
diff --git a/sequencer.h b/sequencer.h
index 4f7f2c93f83e..68284e9762c8 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
 int sequencer_rollback(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-int sequencer_make_script(int keep_empty, FILE *out,
-   int argc, const char **argv);
+#define TODO_LIST_KEEP_EMPTY (1U << 0)
+#define TODO_LIST_SHORTEN_IDS (1U << 1)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+ unsigned flags);
 
-int transform_todos(int shorten_ids);
+int transform_todos(unsigned flags);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 7/9] rebase -i -x: add exec commands via the rebase--helper

[Liam Beguin]

Recent work on `git-rebase--interactive` aims to convert shell code to
C. Even if this is most likely not a big performance enhancement, let's
convert it too since a coming change to abbreviate command names
requires it to be updated.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 builtin/rebase--helper.c   |  7 ++-
 git-rebase--interactive.sh | 23 +-
 sequencer.c| 39 ++
 sequencer.h|  1 +
 4 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 1102ecb43b67..4229ea0dc122 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -15,7 +15,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
unsigned flags = 0, keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
-   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
+   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
+   ADD_EXEC
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
OPT_CMDMODE(0, "rearrange-squash", ,
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+   OPT_CMDMODE(0, "add-exec-commands", ,
+   N_("insert exec commands in todo list"), ADD_EXEC),
OPT_END()
};
 
@@ -65,5 +68,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!skip_unnecessary_picks();
if (command == REARRANGE_SQUASH && argc == 1)
return !!rearrange_squash();
+   if (command == ADD_EXEC && argc == 2)
+   return !!sequencer_add_exec_commands(argv[1]);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 437815669f00..e3f5a0abf3c7 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -722,27 +722,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
 }
 
-# Add commands after a pick or after a squash/fixup series
-# in the todo list.
-add_exec_commands () {
-   {
-   first=t
-   while read -r insn rest
-   do
-   case $insn in
-   pick)
-   test -n "$first" ||
-   printf "%s" "$cmd"
-   ;;
-   esac
-   printf "%s %s\n" "$insn" "$rest"
-   first=
-   done
-   printf "%s" "$cmd"
-   } <"$1" >"$1.new" &&
-   mv "$1.new" "$1"
-}
-
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
@@ -982,7 +961,7 @@ fi
 
 test -s "$todo" || echo noop >> "$todo"
 test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-test -n "$cmd" && add_exec_commands "$todo"
+test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
 
 todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
 todocount=${todocount##* }
diff --git a/sequencer.c b/sequencer.c
index 8b0dd610c881..892d242f6966 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2494,6 +2494,45 @@ int sequencer_make_script(FILE *out, int argc, const 
char **argv,
return 0;
 }
 
+/*
+ * Add commands after pick and (series of) squash/fixup commands
+ * in the todo list.
+ */
+int sequencer_add_exec_commands(const char *commands)
+{
+   const char *todo_file = rebase_path_todo();
+   struct todo_list todo_list = TODO_LIST_INIT;
+   struct todo_item *item;
+   struct strbuf *buf = _list.buf;
+   size_t offset = 0, commands_len = strlen(commands);
+   int i, first;
+
+   if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
+   return error(_("could not read '%s'."), todo_file);
+
+   if (parse_insn_buffer(todo_list.buf.buf, _list)) {
+   todo_list_release(_list);
+   return error(_("unusable todo list: '%s'"), todo_file);
+   }
+
+   first = 1;
+   /* insert  before every pick except the first one */
+   for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item

[PATCH v3 4/9] rebase -i: refactor transform_todo_ids

[Liam Beguin]

The transform_todo_ids function is a little hard to read. Lets try
to make it easier by using more of the strbuf API. Also, since we'll
soon be adding command abbreviations, let's rename the function so
it's name reflects that change.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 builtin/rebase--helper.c |  4 +--
 sequencer.c  | 69 
 sequencer.h  |  2 +-
 3 files changed, 31 insertions(+), 44 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f8519363a393..8ad4779d1650 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -55,9 +55,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
if (command == MAKE_SCRIPT && argc > 1)
return !!sequencer_make_script(keep_empty, stdout, argc, argv);
if (command == SHORTEN_SHA1S && argc == 1)
-   return !!transform_todo_ids(1);
+   return !!transform_todos(1);
if (command == EXPAND_SHA1S && argc == 1)
-   return !!transform_todo_ids(0);
+   return !!transform_todos(0);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 5033b049d995..c9a661a8c4bd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2494,60 +2494,47 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todo_ids(int shorten_ids)
+int transform_todos(int shorten_ids)
 {
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
-   int fd, res, i;
-   FILE *out;
+   struct strbuf buf = STRBUF_INIT;
+   struct todo_item *item;
+   int i;
 
-   strbuf_reset(_list.buf);
-   fd = open(todo_file, O_RDONLY);
-   if (fd < 0)
-   return error_errno(_("could not open '%s'"), todo_file);
-   if (strbuf_read(_list.buf, fd, 0) < 0) {
-   close(fd);
+   if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
return error(_("could not read '%s'."), todo_file);
-   }
-   close(fd);
 
-   res = parse_insn_buffer(todo_list.buf.buf, _list);
-   if (res) {
+   if (parse_insn_buffer(todo_list.buf.buf, _list)) {
todo_list_release(_list);
return error(_("unusable todo list: '%s'"), todo_file);
}
 
-   out = fopen(todo_file, "w");
-   if (!out) {
-   todo_list_release(_list);
-   return error(_("unable to open '%s' for writing"), todo_file);
-   }
-   for (i = 0; i < todo_list.nr; i++) {
-   struct todo_item *item = todo_list.items + i;
-   int bol = item->offset_in_buf;
-   const char *p = todo_list.buf.buf + bol;
-   int eol = i + 1 < todo_list.nr ?
-   todo_list.items[i + 1].offset_in_buf :
-   todo_list.buf.len;
-
-   if (item->command >= TODO_EXEC && item->command != TODO_DROP)
-   fwrite(p, eol - bol, 1, out);
-   else {
-   const char *id = shorten_ids ?
-   short_commit_name(item->commit) :
-   oid_to_hex(>commit->object.oid);
-   int len;
-
-   p += strspn(p, " \t"); /* left-trim command */
-   len = strcspn(p, " \t"); /* length of command */
-
-   fprintf(out, "%.*s %s %.*s\n",
-   len, p, id, item->arg_len, item->arg);
+   for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
+   /* if the item is not a command write it and continue */
+   if (item->command >= TODO_COMMENT) {
+   strbuf_addf(, "%.*s\n", item->arg_len, item->arg);
+   continue;
+   }
+
+   /* add command to the buffer */
+   strbuf_addstr(, command_to_string(item->command));
+
+   /* add commit id */
+   if (item->commit) {
+   const char *oid = shorten_ids ?
+ short_commit_name(item->commit) :
+ oid_to_hex(>commit->object.oid);
+
+   strbuf_addf(, " %s", oid);
}
+   /* add all the rest */
+   strbuf_addf(, " %.*s\n", item->arg_len, item->arg);
}
-   fclose(out);
+
+   i = write_message(buf.buf, buf.len, todo_file, 0);
todo_list_release(_list);
-  

[PATCH v3 2/9] Documentation: use preferred name for the 'todo list' script

[Liam Beguin]

Use "todo list" instead of "instruction list" or "todo-list" to
reduce further confusion regarding the name of this script.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/rebase-config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index dba088d7c68f..30ae08cb5a4b 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -23,10 +23,10 @@ rebase.missingCommitsCheck::
--edit-todo' can then be used to correct the error. If set to
"ignore", no checking is done.
To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
+   command in the todo list.
Defaults to "ignore".
 
 rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for the
-   instruction list during an interactive rebase.  The format will
+   todo list during an interactive rebase.  The format will
automatically have the long commit hash prepended to the format.
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 8/9] rebase -i: learn to abbreviate command names

[Liam Beguin]

`git rebase -i` already know how to interpret single-letter command
names. Teach it to generate the todo list with these same abbreviated
names.

Based-on-patch-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/rebase-config.txt | 20 
 builtin/rebase--helper.c|  3 +++
 sequencer.c | 16 ++--
 sequencer.h |  1 +
 4 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index 30ae08cb5a4b..42e1ba757564 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -30,3 +30,23 @@ rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for the
todo list during an interactive rebase.  The format will
automatically have the long commit hash prepended to the format.
+
+rebase.abbreviateCommands::
+   If set to true, `git rebase` will use abbreviated command names in the
+   todo list resulting in something like this:
++
+---
+   p deadbee The oneline of the commit
+   p fa1afe1 The oneline of the next commit
+   ...
+---
++
+instead of:
++
+---
+   pick deadbee The oneline of the commit
+   pick fa1afe1 The oneline of the next commit
+   ...
+---
++
+Defaults to false.
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 4229ea0dc122..7daee544b7b4 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,6 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
unsigned flags = 0, keep_empty = 0;
+   int abbreviate_commands = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
@@ -43,6 +44,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
};
 
git_config(git_default_config, NULL);
+   git_config_get_bool("rebase.abbreviatecommands", _commands);
 
opts.action = REPLAY_INTERACTIVE_REBASE;
opts.allow_ff = 1;
@@ -52,6 +54,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+   flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
 
if (command == CONTINUE && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 892d242f6966..115085d39ca8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -795,6 +795,13 @@ static const char *command_to_string(const enum 
todo_command command)
die("Unknown command: %d", command);
 }
 
+static const char command_to_char(const enum todo_command command)
+{
+   if (command < TODO_COMMENT && todo_command_info[command].c)
+   return todo_command_info[command].c;
+   return comment_line_char;
+}
+
 static int is_noop(const enum todo_command command)
 {
return TODO_NOOP <= command;
@@ -2453,6 +2460,7 @@ int sequencer_make_script(FILE *out, int argc, const char 
**argv,
struct rev_info revs;
struct commit *commit;
int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
+   const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
 
init_revisions(, NULL);
revs.verbose_header = 1;
@@ -2485,7 +2493,8 @@ int sequencer_make_script(FILE *out, int argc, const char 
**argv,
strbuf_reset();
if (!keep_empty && is_original_commit_empty(commit))
strbuf_addf(, "%c ", comment_line_char);
-   strbuf_addf(, "pick %s ", oid_to_hex(>object.oid));
+   strbuf_addf(, "%s %s ", insn,
+   oid_to_hex(>object.oid));
pretty_print_commit(, commit, );
strbuf_addch(, '\n');
fputs(buf.buf, out);
@@ -2558,7 +2567,10 @@ int transform_todos(unsigned flags)
}
 
/* add command to the buffer */
-   strbuf_addstr(, command_to_string(item->command));
+   if (flags & TODO_LIST_ABBREVIATE_CMDS)
+   strbuf_addch(, command_to_char(item->command));
+   else
+   strbuf_addstr(, command_to_string(item->command));
 
/* add commit id */
if (item->commit) {
diff --git a/sequencer.h b/sequencer

[PATCH v3 1/9] Documentation: move rebase.* configs to new file

[Liam Beguin]

Move all rebase.* configuration variables to a separate file in order to
remove duplicates, and include it in config.txt and git-rebase.txt.  The
new descriptions are mostly taken from config.txt as they are more
verbose.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/config.txt| 31 +--
 Documentation/git-rebase.txt| 19 +--
 Documentation/rebase-config.txt | 32 
 3 files changed, 34 insertions(+), 48 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 531649cb40ea..e424b7de90b5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2691,36 +2691,7 @@ push.recurseSubmodules::
is retained. You may override this configuration at time of push by
specifying '--recurse-submodules=check|on-demand|no'.
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   When set to true, automatically create a temporary stash entry
-   before the operation begins, and apply it after the operation
-   ends.  This means that you can run rebase on a dirty worktree.
-   However, use with care: the final stash application after a
-   successful rebase might result in non-trivial conflicts.
-   Defaults to false.
-
-rebase.missingCommitsCheck::
-   If set to "warn", git rebase -i will print a warning if some
-   commits are removed (e.g. a line was deleted), however the
-   rebase will still proceed. If set to "error", it will print
-   the previous warning and stop the rebase, 'git rebase
-   --edit-todo' can then be used to correct the error. If set to
-   "ignore", no checking is done.
-   To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
-   Defaults to "ignore".
-
-rebase.instructionFormat::
-   A format string, as specified in linkgit:git-log[1], to be used for
-   the instruction list during an interactive rebase.  The format will 
automatically
-   have the long commit hash prepended to the format.
+include::rebase-config.txt[]
 
 receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3cedfb0fd22b..8a861c1e0d69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -203,24 +203,7 @@ Alternatively, you can undo the 'git rebase' with
 CONFIGURATION
 -
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   If set to true enable `--autostash` option by default.
-
-rebase.missingCommitsCheck::
-   If set to "warn", print warnings about removed commits in
-   interactive mode. If set to "error", print the warnings and
-   stop the rebase. If set to "ignore", no checking is
-   done. "ignore" by default.
-
-rebase.instructionFormat::
-   Custom commit list format to use during an `--interactive` rebase.
+include::rebase-config.txt[]
 
 OPTIONS
 ---
diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
new file mode 100644
index ..dba088d7c68f
--- /dev/null
+++ b/Documentation/rebase-config.txt
@@ -0,0 +1,32 @@
+rebase.stat::
+   Whether to show a diffstat of what changed upstream since the last
+   rebase. False by default.
+
+rebase.autoSquash::
+   If set to true enable `--autosquash` option by default.
+
+rebase.autoStash::
+   When set to true, automatically create a temporary stash entry
+   before the operation begins, and apply it after the operation
+   ends.  This means that you can run rebase on a dirty worktree.
+   However, use with care: the final stash application after a
+   successful rebase might result in non-trivial conflicts.
+   This option can be overridden by the `--no-autostash` and
+   `--autostash` options of linkgit:git-rebase[1].
+   Defaults to false.
+
+rebase.missingCommitsCheck::
+   If set to "warn", git rebase -i will print a warning if some
+   commits are removed (e.g. a line was deleted), however the
+   rebase will still proceed. If set to "error", it will print
+   the previous warning and stop the rebase, 'git rebase
+   --edit-todo' can then be used to correct the error. If set to
+   "ignore", no checking is done.
+   To drop a commit without warning or error, use the `drop`
+   command in the todo-list.
+   Default

[PATCH v3 3/9] rebase -i: set commit to null in exec commands

[Liam Beguin]

Make sure commit is set to NULL when parsing exec instructions
from the todo list. If not, we may try to access an uninitialized
address later while updating the todo list.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index fa94ed652d2c..5033b049d995 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1268,6 +1268,7 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
bol += padding;
 
if (item->command == TODO_EXEC) {
+   item->commit = NULL;
item->arg = bol;
item->arg_len = (int)(eol - bol);
return 0;
-- 
2.15.1.280.g10402c1f5b5c

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[liam Beguin]

Hi,

On 05/12/17 07:41 AM, Kerry, Richard wrote:
> 
> "Shorted" is what happens when you put a piece of wire across the terminals 
> of a battery ... (bang, smoke, etc).
> It's short for "short-circuited".
> Yes, I think you mean "shortened" in this case.
> 

Thanks for the explanation.
Sorry, my eyes stopped at the lowercase 's' in Johannes message.
Will fix.

> Regards,
> Richard.
> 
> 
> 
> Richard Kerry
> BNCS Engineer, SI SOL Telco & Media Vertical Practice
> 
> T: +44 (0)20 3618 2669
> M: +44 (0)7812 325518
> Lync: +44 (0) 20 3618 0778
> Room G300, Stadium House, Wood Lane, London, W12 7TA
> richard.ke...@atos.net
> 
> 

[...]

Thanks,
Liam

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[liam Beguin]

Hi Johannes,

On 04/12/17 10:46 AM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sun, 3 Dec 2017, Liam Beguin wrote:
> 
>> diff --git a/sequencer.h b/sequencer.h
>> index 4e444e3bf1c4..3bb6b0658192 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
>>  int sequencer_rollback(struct replay_opts *opts);
>>  int sequencer_remove_state(struct replay_opts *opts);
>>  
>> -int sequencer_make_script(int keep_empty, FILE *out,
>> -int argc, const char **argv);
>> +#define TODO_LIST_KEEP_EMPTY (1U << 0)
>> +#define TODO_LIST_SHORTED_IDS (1U << 1)
> 
> Maybe SHORTEN_IDs? And either revert back to transform_todo_ids() or use
> SHORTEN_INSNS...
> 

I'll change it to TODO_LIST_SHORTED_IDs. TODO_LIST_SHORTED_INSNS would
suggest the flag changes both parts of the todo.

> Maybe also TRANSFORM_TODO_LIST_* and maybe move the #define's above the
> transform_todo_ids() function, i.e. one line further down?
> 
>> +int sequencer_make_script(FILE *out, int argc, const char **argv,
>> +  unsigned flags);
>>  
>> -int transform_todo_insn(int shorten_ids);
>> +int transform_todo_insn(unsigned flags);
>>  int check_todo_list(void);
>>  int skip_unnecessary_picks(void);
>>  int rearrange_squash(void);
> 
> Ciao,
> Johannes
> 

Thanks, 
Liam

Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids

[liam Beguin]

Hi Johannes,

On 04/12/17 09:42 AM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sun, 3 Dec 2017, Liam Beguin wrote:
> 
>> The transform_todo_ids function is a little hard to read. Lets try
>> to make it easier by using more of the strbuf API. Also, since we'll
>> soon be adding command abbreviations, let's rename the function so
>> it's name reflects that change.
> 
> I am not really a fan of the new name, and would prefer the old one, but
> that's only a nit, not a reason to reject the patch.
> 

You're right, it's probably not the best name. I'll change it to
transform_todos() as we want the function name to reflect that it changes
both parts of the todo.

> The rest of it makes the code reads a lot nicer than before. Thank you,
> Johannes
> 

Thanks,
Liam

Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids

[liam Beguin]

Hi Junio,

On 04/12/17 11:09 AM, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schinde...@gmx.de> writes:
> 
>> On Sun, 3 Dec 2017, Liam Beguin wrote:
>>
>>> The transform_todo_ids function is a little hard to read. Lets try
>>> to make it easier by using more of the strbuf API. Also, since we'll
>>> soon be adding command abbreviations, let's rename the function so
>>> it's name reflects that change.
>>
>> I am not really a fan of the new name, and would prefer the old one, but
>> that's only a nit, not a reason to reject the patch.
> 
> FWIW, I do think the new name goes backwards.  The command uses to
> remember what operations are to be carried out in which order using
> a thing, and the name of that thing "todo list".  We also called it
> the "instruction sheet", and "insn" was a good term to call one item
> on that sheet among other items.
> 

Good point on saying this name change is going backwards.

> But recent commits in the area are pushing us to call it "todo list"
> consistently.  An element in that list should be called "todo".
> 
> A "todo" consists of two parts, "what operation is done" part and
> "using what commit object" part.  The original implementation of
> this function affected only the latter part, and in that light, the
> original name transform_todo_ids() is understandable.  Now you are
> planning to make it modify both parts, not just "ids", so it is
> understandable that you would want to rename it.  But I do not think
> "insn" matches the recent trend.  It even risks misunderstanding
> (i.e. xfrm_todo_ids() is about modifying "using what commit" part,
> so perhaps xfrm_todo_insns() is about modifying "what operation is
> done" part---but that is different from what you want to do, which
> is to update _both_ halves).
> 

You're right! We do want the name to reflect that we intend to change
both halves and not only the command.

> Calling it just transform_todo() would probably be more in line with
> the reason why you wanted to rename it in the first place.
> 

Good suggestion. Would transform_todos() work too? I'll send an update
tomorrow.
Thanks, 

Liam

[PATCH v2 9/9] t3404: add test case for abbreviated commands

[Liam Beguin]

Make sure the todo list ends up using single-letter command
abbreviations when the rebase.abbreviateCommands is enabled.
This configuration option should not change anything else.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 t/t3404-rebase-interactive.sh | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6a82d1ed876d..481a3500900d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1260,6 +1260,28 @@ test_expect_success 'rebase -i respects 
rebase.missingCommitsCheck = error' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and 
exec' '
+   rebase_setup_and_clean abbrevcmd &&
+   test_commit "first" file1.txt "first line" first &&
+   test_commit "second" file1.txt "another line" second &&
+   test_commit "fixup! first" file2.txt "first line again" first_fixup &&
+   test_commit "squash! second" file1.txt "another line here" 
second_squash &&
+   cat >expected <<-EOF &&
+   p $(git rev-list --abbrev-commit -1 first) first
+   f $(git rev-list --abbrev-commit -1 first_fixup) fixup! first
+   x git show HEAD
+   p $(git rev-list --abbrev-commit -1 second) second
+   s $(git rev-list --abbrev-commit -1 second_squash) squash! second
+   x git show HEAD
+   EOF
+   git checkout abbrevcmd &&
+   set_cat_todo_editor &&
+   test_config rebase.abbreviateCommands true &&
+   test_must_fail git rebase -i --exec "git show HEAD" \
+   --autosquash master >actual &&
+   test_cmp expected actual
+'
+
 test_expect_success 'static check of bad command' '
rebase_setup_and_clean bad-cmd &&
set_fake_editor &&
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Liam Beguin]

Update functions used in the rebase--helper so that they take a generic
'flags' parameter instead of a growing list of options.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 builtin/rebase--helper.c | 13 +++--
 sequencer.c  |  9 +
 sequencer.h  |  8 +---
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index af0f91164fd0..fe814bf7229e 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   int keep_empty = 0;
+   unsigned flags = 0, keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
@@ -48,16 +48,17 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
argc = parse_options(argc, argv, NULL, options,
builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
+   flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+   flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTED_IDS : 0;
+
if (command == CONTINUE && argc == 1)
return !!sequencer_continue();
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
-   return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-   if (command == SHORTEN_OIDS && argc == 1)
-   return !!transform_todo_insn(1);
-   if (command == EXPAND_OIDS && argc == 1)
-   return !!transform_todo_insn(0);
+   return !!sequencer_make_script(stdout, argc, argv, flags);
+   if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
+   return !!transform_todo_insn(flags);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 0ff3c90e44bf..7d712811e9d1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2444,14 +2444,15 @@ void append_signoff(struct strbuf *msgbuf, int 
ignore_footer, unsigned flag)
strbuf_release();
 }
 
-int sequencer_make_script(int keep_empty, FILE *out,
-   int argc, const char **argv)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+ unsigned flags)
 {
char *format = NULL;
struct pretty_print_context pp = {0};
struct strbuf buf = STRBUF_INIT;
struct rev_info revs;
struct commit *commit;
+   int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 
init_revisions(, NULL);
revs.verbose_header = 1;
@@ -2494,7 +2495,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todo_insn(int shorten_ids)
+int transform_todo_insn(unsigned flags)
 {
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
@@ -2522,7 +2523,7 @@ int transform_todo_insn(int shorten_ids)
 
/* add commit id */
if (item->commit) {
-   const char *oid = shorten_ids ?
+   const char *oid = flags & TODO_LIST_SHORTED_IDS ?
  short_commit_name(item->commit) :
  oid_to_hex(>commit->object.oid);
 
diff --git a/sequencer.h b/sequencer.h
index 4e444e3bf1c4..3bb6b0658192 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
 int sequencer_rollback(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-int sequencer_make_script(int keep_empty, FILE *out,
-   int argc, const char **argv);
+#define TODO_LIST_KEEP_EMPTY (1U << 0)
+#define TODO_LIST_SHORTED_IDS (1U << 1)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+ unsigned flags);
 
-int transform_todo_insn(int shorten_ids);
+int transform_todo_insn(unsigned flags);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 3/9] rebase -i: set commit to null in exec commands

[Liam Beguin]

Make sure commit is set to NULL when parsing exec instructions
from the todo list. If not, we may try to access an uninitialized
address later while updating the todo list.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index fa94ed652d2c..5033b049d995 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1268,6 +1268,7 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
bol += padding;
 
if (item->command == TODO_EXEC) {
+   item->commit = NULL;
item->arg = bol;
item->arg_len = (int)(eol - bol);
return 0;
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Liam Beguin]

`git rebase -i` already know how to interpret single-letter command
names. Teach it to generate the todo list with these same abbreviated
names.

Based-on-patch-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/rebase-config.txt | 20 
 builtin/rebase--helper.c|  3 +++
 sequencer.c | 16 ++--
 sequencer.h |  1 +
 4 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index 30ae08cb5a4b..42e1ba757564 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -30,3 +30,23 @@ rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for the
todo list during an interactive rebase.  The format will
automatically have the long commit hash prepended to the format.
+
+rebase.abbreviateCommands::
+   If set to true, `git rebase` will use abbreviated command names in the
+   todo list resulting in something like this:
++
+---
+   p deadbee The oneline of the commit
+   p fa1afe1 The oneline of the next commit
+   ...
+---
++
+instead of:
++
+---
+   pick deadbee The oneline of the commit
+   pick fa1afe1 The oneline of the next commit
+   ...
+---
++
+Defaults to false.
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 03337e1484a2..2c51ddcfd3dd 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,6 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
unsigned flags = 0, keep_empty = 0;
+   int abbreviate_commands = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
@@ -43,6 +44,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
};
 
git_config(git_default_config, NULL);
+   git_config_get_bool("rebase.abbreviatecommands", _commands);
 
opts.action = REPLAY_INTERACTIVE_REBASE;
opts.allow_ff = 1;
@@ -52,6 +54,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+   flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTED_IDS : 0;
 
if (command == CONTINUE && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index bd047737082d..b752dcc52982 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -795,6 +795,13 @@ static const char *command_to_string(const enum 
todo_command command)
die("Unknown command: %d", command);
 }
 
+static const char command_to_char(const enum todo_command command)
+{
+   if (command < TODO_COMMENT && todo_command_info[command].c)
+   return todo_command_info[command].c;
+   return comment_line_char;
+}
+
 static int is_noop(const enum todo_command command)
 {
return TODO_NOOP <= command;
@@ -2453,6 +2460,7 @@ int sequencer_make_script(FILE *out, int argc, const char 
**argv,
struct rev_info revs;
struct commit *commit;
int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
+   const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
 
init_revisions(, NULL);
revs.verbose_header = 1;
@@ -2485,7 +2493,8 @@ int sequencer_make_script(FILE *out, int argc, const char 
**argv,
strbuf_reset();
if (!keep_empty && is_original_commit_empty(commit))
strbuf_addf(, "%c ", comment_line_char);
-   strbuf_addf(, "pick %s ", oid_to_hex(>object.oid));
+   strbuf_addf(, "%s %s ", insn,
+   oid_to_hex(>object.oid));
pretty_print_commit(, commit, );
strbuf_addch(, '\n');
fputs(buf.buf, out);
@@ -2558,7 +2567,10 @@ int transform_todo_insn(unsigned flags)
}
 
/* add command to the buffer */
-   strbuf_addstr(, command_to_string(item->command));
+   if (flags & TODO_LIST_ABBREVIATE_CMDS)
+   strbuf_addch(, command_to_char(item->command));
+   else
+   strbuf_addstr(, command_to_string(item->command));
 
/* add commit id */
if (item->commit) {
diff --git a/sequencer.h

[PATCH v2 4/9] rebase -i: refactor transform_todo_ids

[Liam Beguin]

The transform_todo_ids function is a little hard to read. Lets try
to make it easier by using more of the strbuf API. Also, since we'll
soon be adding command abbreviations, let's rename the function so
it's name reflects that change.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 builtin/rebase--helper.c |  4 +--
 sequencer.c  | 69 
 sequencer.h  |  2 +-
 3 files changed, 31 insertions(+), 44 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f8519363a393..7c06a27de821 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -55,9 +55,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
if (command == MAKE_SCRIPT && argc > 1)
return !!sequencer_make_script(keep_empty, stdout, argc, argv);
if (command == SHORTEN_SHA1S && argc == 1)
-   return !!transform_todo_ids(1);
+   return !!transform_todo_insn(1);
if (command == EXPAND_SHA1S && argc == 1)
-   return !!transform_todo_ids(0);
+   return !!transform_todo_insn(0);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 5033b049d995..0ff3c90e44bf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2494,60 +2494,47 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todo_ids(int shorten_ids)
+int transform_todo_insn(int shorten_ids)
 {
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
-   int fd, res, i;
-   FILE *out;
+   struct strbuf buf = STRBUF_INIT;
+   struct todo_item *item;
+   int i;
 
-   strbuf_reset(_list.buf);
-   fd = open(todo_file, O_RDONLY);
-   if (fd < 0)
-   return error_errno(_("could not open '%s'"), todo_file);
-   if (strbuf_read(_list.buf, fd, 0) < 0) {
-   close(fd);
+   if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
return error(_("could not read '%s'."), todo_file);
-   }
-   close(fd);
 
-   res = parse_insn_buffer(todo_list.buf.buf, _list);
-   if (res) {
+   if (parse_insn_buffer(todo_list.buf.buf, _list)) {
todo_list_release(_list);
return error(_("unusable todo list: '%s'"), todo_file);
}
 
-   out = fopen(todo_file, "w");
-   if (!out) {
-   todo_list_release(_list);
-   return error(_("unable to open '%s' for writing"), todo_file);
-   }
-   for (i = 0; i < todo_list.nr; i++) {
-   struct todo_item *item = todo_list.items + i;
-   int bol = item->offset_in_buf;
-   const char *p = todo_list.buf.buf + bol;
-   int eol = i + 1 < todo_list.nr ?
-   todo_list.items[i + 1].offset_in_buf :
-   todo_list.buf.len;
-
-   if (item->command >= TODO_EXEC && item->command != TODO_DROP)
-   fwrite(p, eol - bol, 1, out);
-   else {
-   const char *id = shorten_ids ?
-   short_commit_name(item->commit) :
-   oid_to_hex(>commit->object.oid);
-   int len;
-
-   p += strspn(p, " \t"); /* left-trim command */
-   len = strcspn(p, " \t"); /* length of command */
-
-   fprintf(out, "%.*s %s %.*s\n",
-   len, p, id, item->arg_len, item->arg);
+   for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
+   /* if the item is not a command write it and continue */
+   if (item->command >= TODO_COMMENT) {
+   strbuf_addf(, "%.*s\n", item->arg_len, item->arg);
+   continue;
+   }
+
+   /* add command to the buffer */
+   strbuf_addstr(, command_to_string(item->command));
+
+   /* add commit id */
+   if (item->commit) {
+   const char *oid = shorten_ids ?
+ short_commit_name(item->commit) :
+ oid_to_hex(>commit->object.oid);
+
+   strbuf_addf(, " %s", oid);
}
+   /* add all the rest */
+   strbuf_addf(, " %.*s\n", item->arg_len, item->arg);
}
-   fclose(out);
+
+   i = write_message(buf.buf, buf.len, todo_file, 0);
todo_list_release(_list);

[PATCH v2 7/9] rebase -i -x: add exec commands via the rebase--helper

[Liam Beguin]

Recent work on `git-rebase--interactive` aims to convert shell code to
C. Even if this is most likely not a big performance enhancement, let's
convert it too since a coming change to abbreviate command names
requires it to be updated.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 builtin/rebase--helper.c   |  7 ++-
 git-rebase--interactive.sh | 23 +--
 sequencer.c| 39 +++
 sequencer.h|  1 +
 4 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index fe814bf7229e..03337e1484a2 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -15,7 +15,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
unsigned flags = 0, keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
-   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
+   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
+   ADD_EXEC
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
OPT_CMDMODE(0, "rearrange-squash", ,
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+   OPT_CMDMODE(0, "add-exec-commands", ,
+   N_("insert exec commands in todo list"), ADD_EXEC),
OPT_END()
};
 
@@ -65,5 +68,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!skip_unnecessary_picks();
if (command == REARRANGE_SQUASH && argc == 1)
return !!rearrange_squash();
+   if (command == ADD_EXEC && argc == 2)
+   return !!sequencer_add_exec_commands(argv[1]);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 437815669f00..e3f5a0abf3c7 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -722,27 +722,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
 }
 
-# Add commands after a pick or after a squash/fixup series
-# in the todo list.
-add_exec_commands () {
-   {
-   first=t
-   while read -r insn rest
-   do
-   case $insn in
-   pick)
-   test -n "$first" ||
-   printf "%s" "$cmd"
-   ;;
-   esac
-   printf "%s %s\n" "$insn" "$rest"
-   first=
-   done
-   printf "%s" "$cmd"
-   } <"$1" >"$1.new" &&
-   mv "$1.new" "$1"
-}
-
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
@@ -982,7 +961,7 @@ fi
 
 test -s "$todo" || echo noop >> "$todo"
 test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-test -n "$cmd" && add_exec_commands "$todo"
+test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
 
 todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
 todocount=${todocount##* }
diff --git a/sequencer.c b/sequencer.c
index 7d712811e9d1..bd047737082d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2494,6 +2494,45 @@ int sequencer_make_script(FILE *out, int argc, const 
char **argv,
return 0;
 }
 
+/*
+ * Add commands after pick and (series of) squash/fixup commands
+ * in the todo list.
+ */
+int sequencer_add_exec_commands(const char *commands)
+{
+   const char *todo_file = rebase_path_todo();
+   struct todo_list todo_list = TODO_LIST_INIT;
+   struct todo_item *item;
+   struct strbuf *buf = _list.buf;
+   size_t offset = 0, commands_len = strlen(commands);
+   int i, first;
+
+   if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
+   return error(_("could not read '%s'."), todo_file);
+
+   if (parse_insn_buffer(todo_list.buf.buf, _list)) {
+   todo_list_release(_list);
+   return error(_("unusable todo list: '%s'"), todo_file);
+   }
+
+   first = 1;
+   /* insert  before every pick except the first one */
+   for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item

[PATCH v2 0/9] rebase -i: add config to abbreviate command names

[Liam Beguin]

Hi everyone,

This series will add the 'rebase.abbreviateCommands' configuration
option to allow `git rebase -i` to default to the single-letter command
names when generating the todo list.

Using single-letter command names can present two benefits. First, it
makes it easier to change the action since you only need to replace a
single character (i.e.: in vim "r" instead of
"ciw").  Second, using this with a large enough value of
'core.abbrev' enables the lines of the todo list to remain aligned
making the files easier to read.

Changes in V2:
- Refactor and rename 'transform_todo_ids'
- Replace SHA-1 by OID in rebase--helper.c
- Update todo list related functions to take a generic 'flags' parameter
- Rename 'add_exec_commands' function to 'sequencer_add_exec_commands'
- Rename 'add-exec' option to 'add-exec-commands'
- Use 'strbur_read_file' instead of rewriting it
- Make 'command_to_char' return 'comment_char_line' if no single-letter
  command name is defined
- Combine both tests into a single test case
- Update commit messages

Liam Beguin (9):
  Documentation: move rebase.* configs to new file
  Documentation: use preferred name for the 'todo list' script
  rebase -i: set commit to null in exec commands
  rebase -i: refactor transform_todo_ids
  rebase -i: replace reference to sha1 with oid
  rebase -i: update functions to use a flags parameter
  rebase -i -x: add exec commands via the rebase--helper
  rebase -i: learn to abbreviate command names
  t3404: add test case for abbreviated commands

 Documentation/config.txt|  31 +---
 Documentation/git-rebase.txt|  19 +
 Documentation/rebase-config.txt |  52 +
 builtin/rebase--helper.c|  29 +---
 git-rebase--interactive.sh  |  23 +-
 sequencer.c | 126 +---
 sequencer.h |  10 ++-
 t/t3404-rebase-interactive.sh   |  22 ++
 8 files changed, 186 insertions(+), 126 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 1/9] Documentation: move rebase.* configs to new file

[Liam Beguin]

Move all rebase.* configuration variables to a separate file in order to
remove duplicates, and include it in config.txt and git-rebase.txt.  The
new descriptions are mostly taken from config.txt as they are more
verbose.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/config.txt| 31 +--
 Documentation/git-rebase.txt| 19 +--
 Documentation/rebase-config.txt | 32 
 3 files changed, 34 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 531649cb40ea..e424b7de90b5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2691,36 +2691,7 @@ push.recurseSubmodules::
is retained. You may override this configuration at time of push by
specifying '--recurse-submodules=check|on-demand|no'.
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   When set to true, automatically create a temporary stash entry
-   before the operation begins, and apply it after the operation
-   ends.  This means that you can run rebase on a dirty worktree.
-   However, use with care: the final stash application after a
-   successful rebase might result in non-trivial conflicts.
-   Defaults to false.
-
-rebase.missingCommitsCheck::
-   If set to "warn", git rebase -i will print a warning if some
-   commits are removed (e.g. a line was deleted), however the
-   rebase will still proceed. If set to "error", it will print
-   the previous warning and stop the rebase, 'git rebase
-   --edit-todo' can then be used to correct the error. If set to
-   "ignore", no checking is done.
-   To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
-   Defaults to "ignore".
-
-rebase.instructionFormat::
-   A format string, as specified in linkgit:git-log[1], to be used for
-   the instruction list during an interactive rebase.  The format will 
automatically
-   have the long commit hash prepended to the format.
+include::rebase-config.txt[]
 
 receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3cedfb0fd22b..8a861c1e0d69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -203,24 +203,7 @@ Alternatively, you can undo the 'git rebase' with
 CONFIGURATION
 -
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   If set to true enable `--autostash` option by default.
-
-rebase.missingCommitsCheck::
-   If set to "warn", print warnings about removed commits in
-   interactive mode. If set to "error", print the warnings and
-   stop the rebase. If set to "ignore", no checking is
-   done. "ignore" by default.
-
-rebase.instructionFormat::
-   Custom commit list format to use during an `--interactive` rebase.
+include::rebase-config.txt[]
 
 OPTIONS
 ---
diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
new file mode 100644
index ..dba088d7c68f
--- /dev/null
+++ b/Documentation/rebase-config.txt
@@ -0,0 +1,32 @@
+rebase.stat::
+   Whether to show a diffstat of what changed upstream since the last
+   rebase. False by default.
+
+rebase.autoSquash::
+   If set to true enable `--autosquash` option by default.
+
+rebase.autoStash::
+   When set to true, automatically create a temporary stash entry
+   before the operation begins, and apply it after the operation
+   ends.  This means that you can run rebase on a dirty worktree.
+   However, use with care: the final stash application after a
+   successful rebase might result in non-trivial conflicts.
+   This option can be overridden by the `--no-autostash` and
+   `--autostash` options of linkgit:git-rebase[1].
+   Defaults to false.
+
+rebase.missingCommitsCheck::
+   If set to "warn", git rebase -i will print a warning if some
+   commits are removed (e.g. a line was deleted), however the
+   rebase will still proceed. If set to "error", it will print
+   the previous warning and stop the rebase, 'git rebase
+   --edit-todo' can then be used to correct the error. If set to
+   "ignore", no checking is done.
+   To drop a commit without warning or error, use the `drop`
+   command

[PATCH v2 2/9] Documentation: use preferred name for the 'todo list' script

[Liam Beguin]

Use "todo list" instead of "instruction list" or "todo-list" to
reduce further confusion regarding the name of this script.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/rebase-config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index dba088d7c68f..30ae08cb5a4b 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -23,10 +23,10 @@ rebase.missingCommitsCheck::
--edit-todo' can then be used to correct the error. If set to
"ignore", no checking is done.
To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
+   command in the todo list.
Defaults to "ignore".
 
 rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for the
-   instruction list during an interactive rebase.  The format will
+   todo list during an interactive rebase.  The format will
automatically have the long commit hash prepended to the format.
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 5/9] rebase -i: replace reference to sha1 with oid

[Liam Beguin]

Since we are trying to abstract the hash function name elsewhere in the
code base, lets use OID instead of SHA-1 in the rebase--helper too.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 builtin/rebase--helper.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 7c06a27de821..af0f91164fd0 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
struct replay_opts opts = REPLAY_OPTS_INIT;
int keep_empty = 0;
enum {
-   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
+   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
} command = 0;
struct option options[] = {
@@ -27,9 +27,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_CMDMODE(0, "make-script", ,
N_("make rebase script"), MAKE_SCRIPT),
OPT_CMDMODE(0, "shorten-ids", ,
-   N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
+   N_("shorten commit ids in the todo list"), 
SHORTEN_OIDS),
OPT_CMDMODE(0, "expand-ids", ,
-   N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
+   N_("expand commit ids in the todo list"), EXPAND_OIDS),
OPT_CMDMODE(0, "check-todo-list", ,
N_("check the todo list"), CHECK_TODO_LIST),
OPT_CMDMODE(0, "skip-unnecessary-picks", ,
@@ -54,9 +54,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-   if (command == SHORTEN_SHA1S && argc == 1)
+   if (command == SHORTEN_OIDS && argc == 1)
return !!transform_todo_insn(1);
-   if (command == EXPAND_SHA1S && argc == 1)
+   if (command == EXPAND_OIDS && argc == 1)
return !!transform_todo_insn(0);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
-- 
2.15.1.280.g10402c1f5b5c

Re: [PATCH 4/5] rebase -i: learn to abbreviate command names

[liam Beguin]

Hi Johannes,

On 27/11/17 06:04 PM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sun, 26 Nov 2017, Liam Beguin wrote:
> 
>> diff --git a/Documentation/rebase-config.txt 
>> b/Documentation/rebase-config.txt
>> index 30ae08cb5a4b..0820b60f6e12 100644
>> --- a/Documentation/rebase-config.txt
>> +++ b/Documentation/rebase-config.txt
>> @@ -30,3 +30,22 @@ rebase.instructionFormat::
>>  A format string, as specified in linkgit:git-log[1], to be used for the
>>  todo list during an interactive rebase.  The format will
>>  automatically have the long commit hash prepended to the format.
>> +
>> +rebase.abbreviateCommands::
>> +If set to true, `git rebase` will use abbreviated command names in the
>> +todo list resulting in something like this:
>> +
>> +---
>> +p deadbee The oneline of the commit
>> +p fa1afe1 The oneline of the next commit
>> +...
>> +---
> 
> I *think* that AsciiDoc will render this in a different way from what we
> want, but I am not an AsciiDoc expert. In my hands, I always had to add a
> single + in an otherwise empty line to start a new indented paragraph *and
> then continue with non-indented lines*.
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index 810b7850748e..aa01e8bd9280 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -795,6 +795,13 @@ static const char *command_to_string(const enum 
>> todo_command command)
>>  die("Unknown command: %d", command);
>>  }
>>  
>> +static const char command_to_char(const enum todo_command command)
>> +{
>> +if (command < TODO_COMMENT && todo_command_info[command].c)
>> +return todo_command_info[command].c;
>> +return -1;
> 
> My initial reaction was: should we return comment_line_char instead of -1
> here? Only after reading how this is called did I realize that the idea is
> to use full command names if there is no abbreviation. Not sure whether
> this is worth a code comment. What do you think?
> 

I guess it probably deserves a comment!

>> +}
>> +
>>  static int is_noop(const enum todo_command command)
>>  {
>>  return TODO_NOOP <= command;
>> @@ -1242,15 +1249,16 @@ static int parse_insn_line(struct todo_item *item, 
>> const char *bol, char *eol)
>>  return 0;
>>  }
>>  
>> -for (i = 0; i < TODO_COMMENT; i++)
>> +for (i = 0; i < TODO_COMMENT; i++) {
>>  if (skip_prefix(bol, todo_command_info[i].str, )) {
>>  item->command = i;
>>  break;
>> -} else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
>> +} else if (bol[1] == ' ' && *bol == command_to_char(i)) {
>>  bol++;
>>  item->command = i;
>>  break;
>>  }
>> +}
>>  if (i >= TODO_COMMENT)
>>  return -1;
>>  
> 
> I would prefer this hunk to be skipped, it does not really do anything if
> I understand correctly.

Ok, I was not so sure about this but thought it was probably worth it.
Will remove.

> 
>> @@ -2443,8 +2451,8 @@ void append_signoff(struct strbuf *msgbuf, int 
>> ignore_footer, unsigned flag)
>>  strbuf_release();
>>  }
>>  
>> -int sequencer_make_script(int keep_empty, FILE *out,
>> -int argc, const char **argv)
>> +int sequencer_make_script(int keep_empty, int abbreviate_commands, FILE 
>> *out,
>> +  int argc, const char **argv)
>>  {
>>  char *format = NULL;
>>  struct pretty_print_context pp = {0};
>> @@ -2483,7 +2491,9 @@ int sequencer_make_script(int keep_empty, FILE *out,
>>  strbuf_reset();
>>  if (!keep_empty && is_original_commit_empty(commit))
>>  strbuf_addf(, "%c ", comment_line_char);
>> -strbuf_addf(, "pick %s ", oid_to_hex(>object.oid));
>> +strbuf_addf(, "%s %s ",
>> +abbreviate_commands ? "p" : "pick",
>> +oid_to_hex(>object.oid));
> 
> I guess the compiler will optimize this code so that the conditional is
> evaluated only once. Not that this is performance critical ;-)

Is your guess enough? :-) If not, how could I make sure this is optimized?
Should I do that check before the while() loop?

> 
>>

Re: [PATCH 4/5] rebase -i: learn to abbreviate command names

[liam Beguin]

Hi Peff,

Thanks for taking the time to test this, I'll squash that patch in v2.

On 27/11/17 06:11 PM, Jeff King wrote:
> On Tue, Nov 28, 2017 at 12:04:45AM +0100, Johannes Schindelin wrote:
> 
>>> +rebase.abbreviateCommands::
>>> +   If set to true, `git rebase` will use abbreviated command names in the
>>> +   todo list resulting in something like this:
>>> +
>>> +---
>>> +   p deadbee The oneline of the commit
>>> +   p fa1afe1 The oneline of the next commit
>>> +   ...
>>> +---
>>
>> I *think* that AsciiDoc will render this in a different way from what we
>> want, but I am not an AsciiDoc expert. In my hands, I always had to add a
>> single + in an otherwise empty line to start a new indented paragraph *and
>> then continue with non-indented lines*.
> 
> Good catch. Interestingly enough, my asciidoc seems to render this
> as desired for the docbook/roff version, but has screwed-up indentation
> for the HTML version.
> 
> Fixing it as you suggest makes it look good in both (and I think you can
> never go wrong with "+"-continuation, aside from making the source a bit
> uglier).
> 
> Squashable patch below for convenience, since I did try it.
> 
> -Peff
> 
> diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
> index 0820b60f6e..42e1ba7575 100644
> --- a/Documentation/rebase-config.txt
> +++ b/Documentation/rebase-config.txt
> @@ -34,18 +34,19 @@ rebase.instructionFormat::
>  rebase.abbreviateCommands::
>   If set to true, `git rebase` will use abbreviated command names in the
>   todo list resulting in something like this:
> -
> ++
>  ---
>   p deadbee The oneline of the commit
>   p fa1afe1 The oneline of the next commit
>   ...
>  ---
> -
> - instead of:
> -
> ++
> +instead of:
> ++
>  ---
>   pick deadbee The oneline of the commit
>   pick fa1afe1 The oneline of the next commit
>   ...
>  ---
> - Defaults to false.
> ++
> +Defaults to false.
> 

Liam

Re: [PATCH 4/5] rebase -i: learn to abbreviate command names

[liam Beguin]

Hi Junio,

On 27/11/17 12:19 AM, Junio C Hamano wrote:
> Liam Beguin <liambeg...@gmail.com> writes:
> 
>>  if (command == MAKE_SCRIPT && argc > 1)
>> -return !!sequencer_make_script(keep_empty, stdout, argc, argv);
>> +return !!sequencer_make_script(keep_empty, abbreviate_commands,
>> +   stdout, argc, argv);
> 
> This suggests that a preliminary clean-up to update the parameter
> list of sequencer_make_script() is in order just before this step.
> How about making it like so, perhaps:
> 
> int sequencer_make_script(FILE *out, int ac, char **av, unsigned flags)
> 
> where keep_empty becomes just one bit in that flags word.  Then another
> bit in the same flags word can be used for this option.
> 
> Otherwise, every time somebody comes up with a new and shiny feature
> for the function, we'd end up adding more to its parameter list.
> 

Will do.
Thanks, 

Liam

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[liam Beguin]

Hi Johannes,

Thanks for taking the time to review this.

On 27/11/17 05:42 PM, Johannes Schindelin wrote:
> Hi Liam,
> 
> could I ask for a favor? I'd like the oneline to start with
> 
>   rebase -i -x: ...
> 
> (this would help future me to realize what this commit touches already
> from the concise graph output I favor).

Sure, I'll update the commit subject.

> 
> On Sun, 26 Nov 2017, Liam Beguin wrote:
> 
>> Recent work on `git-rebase--interactive` aim to convert shell code to C.
>> Even if this is most likely not a big performance enhacement, let's
>> convert it too since a comming change to abbreviate command names requires
>> it to be updated.
> 
> Since Junio did not comment on the commit message: could you replace
> `aim` by `aims`, `enhacement` by `enhancement` and `comming` by `coming`?

Ow.. sorry about that! I'll fix those and make sure to proofread better next 
time!

> 
>> @@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
>> char *prefix)
>>  N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
>>  OPT_CMDMODE(0, "rearrange-squash", ,
>>  N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
>> +OPT_CMDMODE(0, "add-exec", ,
>> +N_("insert exec commands in todo list"), ADD_EXEC),
> 
> Maybe `add-exec-commands`? I know it is longer to type, but these options do
> not need to be typed interactively and the longer name would be consistent
> with the function name.

Makes sense. It'll also be more consistent with the rest of the commands above.

> 
>> diff --git a/sequencer.c b/sequencer.c
>> index fa94ed652d2c..810b7850748e 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
>>  return 0;
>>  }
>>  
> 
> As the code in add_exec_commands() may appear convoluted (why not simply
> append the command after any pick?), the original comment would be really
> nice here:
> 
>   /*
>* Add commands after pick and (series of) squash/fixup commands
>* in the todo list.
>*/
> 

I'll make sure to include that comment.
The code is a bit convoluted as you say... I wanted to send it "as is" first
to get comments and update based on feedback from the list.

I just realized we could maybe add exec instructions only after pick commands
if we do add-exec-commands before rearrange-squash. I'll test it out.

>> +int add_exec_commands(const char *command)
>> +{
>> +const char *todo_file = rebase_path_todo();
>> +struct todo_list todo_list = TODO_LIST_INIT;
>> +int fd, res, i, first = 1;
>> +FILE *out;
>> +
>> +strbuf_reset(_list.buf);
> 
> The todo_list.buf has been initialized already (via TODO_LIST_INIT), no
> need to reset it again.
> 
>> +fd = open(todo_file, O_RDONLY);
>> +if (fd < 0)
>> +return error_errno(_("could not open '%s'"), todo_file);
>> +if (strbuf_read(_list.buf, fd, 0) < 0) {
>> +close(fd);
>> +return error(_("could not read '%s'."), todo_file);
>> +}
>> +close(fd);
> 
> As Junio pointed out so gently: there is a helper function that does this
> all very conveniently for us:
> 
>   if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
>   return error_errno(_("could not read '%s'"), todo_file);
> 
> And as I realized looking at the surrounding code: you probably just
> inherited my inelegant code by copy-editing from another function in
> sequencer.c. Should you decide to add a preparatory patch to your patch
> series that converts these other callers, or even refactors all that code
> that reads the git-rebase-todo file and then parses it, I would be quite
> happy... :-) (although I would understand if you deemed this outside the
> purpose of your patch series).
> 

You guessed well, I mostly did copy-editing... I thought I found this code
a little confusing because I'm not used to as much pointer gymnastics but
it reassures me a bit to read this :-). I'll see if I can come up with a
better solution.

>> +res = parse_insn_buffer(todo_list.buf.buf, _list);
>> +if (res) {
>> +todo_list_release(_list);
>> +return error(_("unusable todo list: '%s'"), todo_file);
>> +}
> 
> The variable `res` is not really used here. Let's just put the
> parse_insn_buffer() call inside the if ().
> 

Will do.

>> +out = fopen(todo_file, "w");
>> +

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[liam Beguin]

Hi Junio,

On 27/11/17 12:14 AM, Junio C Hamano wrote:
> Liam Beguin <liambeg...@gmail.com> writes:
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index fa94ed652d2c..810b7850748e 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
>>  return 0;
>>  }
>>  
>> +int add_exec_commands(const char *command)
>> +{
> 
> As the name of a public function, it does not feel that this hints
> it strongly enough that it is from and a part of sequencer.c API.
> 
>> +const char *todo_file = rebase_path_todo();
>> +struct todo_list todo_list = TODO_LIST_INIT;
>> +int fd, res, i, first = 1;
>> +FILE *out;
> 
> Having had to scan backwards while trying to see what the loop that
> uses this variable is doing and if it gets affected by things that
> happened before we entered the loop, I'd rather not to see 'first'
> initialized here, left unused for quite some time until the loop is
> entered.  It would make it a lot easier to follow if it is declared
> and left uninitilized here, and set to 1 immediately before the
> for() loop that uses it.
> 

I agree that moving 'first = 1' just above the for() loop makes it
more obvious. I'm not quite fond of how this is implemented, I just
'translated' the shell code and was hoping on maybe a few comments
on how to improve it.

>> +
>> +strbuf_reset(_list.buf);
>> +fd = open(todo_file, O_RDONLY);
>> +if (fd < 0)
>> +return error_errno(_("could not open '%s'"), todo_file);
>> +if (strbuf_read(_list.buf, fd, 0) < 0) {
>> +close(fd);
>> +return error(_("could not read '%s'."), todo_file);
>> +}
>> +close(fd);
> 
> Is this strbuf_read_file() written in longhand?

Thanks for pointing this out! I'll update. And as Johannes pointed out,
I've copied this from surrounding functions, I'll add a preparatory path
to update those too.

> 
>> +res = parse_insn_buffer(todo_list.buf.buf, _list);
>> +if (res) {
>> +todo_list_release(_list);
>> +return error(_("unusable todo list: '%s'"), todo_file);
>> +}
>> +
>> +out = fopen(todo_file, "w");
>> +if (!out) {
>> +todo_list_release(_list);
>> +return error(_("unable to open '%s' for writing"), todo_file);
>> +}
>> +for (i = 0; i < todo_list.nr; i++) {
>> +struct todo_item *item = todo_list.items + i;
>> +int bol = item->offset_in_buf;
>> +const char *p = todo_list.buf.buf + bol;
>> +int eol = i + 1 < todo_list.nr ?
>> +todo_list.items[i + 1].offset_in_buf :
>> +todo_list.buf.len;
> 
> Should bol and eol be of type size_t instead?  The values that get
> assigned to them from other structures are.
> 

Will do.
Thanks, 

Liam

Re: [PATCH 0/5] rebase -i: add config to abbreviate command names

[liam Beguin]

Hi Junio,

On 27/11/17 12:23 AM, Junio C Hamano wrote:
> Liam Beguin <liambeg...@gmail.com> writes:
> 
>> Liam Beguin (5):
>>   Documentation: move rebase.* configs to new file
>>   Documentation: use preferred name for the 'todo list' script
>>   rebase -i: add exec commands via the rebase--helper
>>   rebase -i: learn to abbreviate command names
>>   t3404: add test case for abbreviated commands
> 
> I didn't send any comment on [1&2/5] but they both looked good.
> 

Thanks for reviewing this. I'll go through your comments and post a
v2 shortly.

Thanks,
Liam

PS: I'm very sorry if someone received a few copies of this, I'm
having issues with my MUA! Hopefully, I've got it right this time...

[PATCH 0/5] rebase -i: add config to abbreviate command names

[Liam Beguin]

Hi everyone,

This series is a respin of something [1] I sent a few months ago. This
time, instead of shell, It's based on top of the C implementation of the
interactive rebase. I've also tried to address the comments that were
left in the last thread.

This series will add the 'rebase.abbreviateCommands' configuration
option to allow `git rebase -i` to default to the single-letter command
names when generating the todo list.

Using single-letter command names can present two benefits. First, it
makes it easier to change the action since you only need to replace a
single character (i.e.: in vim "r" instead of
"ciw").  Second, using this with a large enough value of
'core.abbrev' enables the lines of the todo list to remain aligned
making the files easier to read.

Changes since last time:
- Implement abbreviateCommands in rebase--helper
- Add note on the --[no-]autosquash option in rebase.autoSquash
- Add exec commands via the rebase--helper
- Add test case for rebase.abbreviateCommands

Liam Beguin (5):
  Documentation: move rebase.* configs to new file
  Documentation: use preferred name for the 'todo list' script
  rebase -i: add exec commands via the rebase--helper
  rebase -i: learn to abbreviate command names
  t3404: add test case for abbreviated commands

 Documentation/config.txt|  31 +
 Documentation/git-rebase.txt|  19 +---
 Documentation/rebase-config.txt |  51 
 builtin/rebase--helper.c|  17 +--
 git-rebase--interactive.sh  |  23 +
 sequencer.c | 100 ++--
 sequencer.h |   5 +-
 t/t3404-rebase-interactive.sh   |  32 +
 8 files changed, 186 insertions(+), 92 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

[1] https://public-inbox.org/git/20170502040048.9065-1-liambeg...@gmail.com/
--
2.15.0.321.g19bf2bb99cee.dirty

[PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[Liam Beguin]

Recent work on `git-rebase--interactive` aim to convert shell code to C.
Even if this is most likely not a big performance enhacement, let's
convert it too since a comming change to abbreviate command names requires
it to be updated.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 builtin/rebase--helper.c   |  7 ++-
 git-rebase--interactive.sh | 23 +--
 sequencer.c| 46 ++
 sequencer.h|  1 +
 4 files changed, 54 insertions(+), 23 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f8519363a393..9d94c874c5bb 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -15,7 +15,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
int keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
-   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
+   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
+   ADD_EXEC
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
OPT_CMDMODE(0, "rearrange-squash", ,
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+   OPT_CMDMODE(0, "add-exec", ,
+   N_("insert exec commands in todo list"), ADD_EXEC),
OPT_END()
};
 
@@ -64,5 +67,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!skip_unnecessary_picks();
if (command == REARRANGE_SQUASH && argc == 1)
return !!rearrange_squash();
+   if (command == ADD_EXEC && argc == 2)
+   return !!add_exec_commands(argv[1]);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 437815669f00..760334d3a8b3 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -722,27 +722,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
 }
 
-# Add commands after a pick or after a squash/fixup series
-# in the todo list.
-add_exec_commands () {
-   {
-   first=t
-   while read -r insn rest
-   do
-   case $insn in
-   pick)
-   test -n "$first" ||
-   printf "%s" "$cmd"
-   ;;
-   esac
-   printf "%s %s\n" "$insn" "$rest"
-   first=
-   done
-   printf "%s" "$cmd"
-   } <"$1" >"$1.new" &&
-   mv "$1.new" "$1"
-}
-
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
@@ -982,7 +961,7 @@ fi
 
 test -s "$todo" || echo noop >> "$todo"
 test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-test -n "$cmd" && add_exec_commands "$todo"
+test -n "$cmd" && git rebase--helper --add-exec "$cmd"
 
 todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
 todocount=${todocount##* }
diff --git a/sequencer.c b/sequencer.c
index fa94ed652d2c..810b7850748e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
return 0;
 }
 
+int add_exec_commands(const char *command)
+{
+   const char *todo_file = rebase_path_todo();
+   struct todo_list todo_list = TODO_LIST_INIT;
+   int fd, res, i, first = 1;
+   FILE *out;
+
+   strbuf_reset(_list.buf);
+   fd = open(todo_file, O_RDONLY);
+   if (fd < 0)
+   return error_errno(_("could not open '%s'"), todo_file);
+   if (strbuf_read(_list.buf, fd, 0) < 0) {
+   close(fd);
+   return error(_("could not read '%s'."), todo_file);
+   }
+   close(fd);
+
+   res = parse_insn_buffer(todo_list.buf.buf, _list);
+   if (res) {
+   todo_list_release(_list);
+   return error(_("unusable todo list: '%s'"), todo_file);
+   }
+
+   out = fopen(todo_file, "w");
+   if (!out) {
+   todo_list_release(_list);
+   return

[PATCH 4/5] rebase -i: learn to abbreviate command names

[Liam Beguin]

`git rebase -i` already know how to interpret single-letter command
names. Teach it to generate the todo list with these same abbreviated
names.

Based-on-patch-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/rebase-config.txt | 19 +++
 builtin/rebase--helper.c| 10 +---
 sequencer.c | 54 +
 sequencer.h |  4 +--
 4 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index 30ae08cb5a4b..0820b60f6e12 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -30,3 +30,22 @@ rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for the
todo list during an interactive rebase.  The format will
automatically have the long commit hash prepended to the format.
+
+rebase.abbreviateCommands::
+   If set to true, `git rebase` will use abbreviated command names in the
+   todo list resulting in something like this:
+
+---
+   p deadbee The oneline of the commit
+   p fa1afe1 The oneline of the next commit
+   ...
+---
+
+   instead of:
+
+---
+   pick deadbee The oneline of the commit
+   pick fa1afe1 The oneline of the next commit
+   ...
+---
+   Defaults to false.
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 9d94c874c5bb..7b1fe825a877 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   int keep_empty = 0;
+   int keep_empty = 0, abbreviate_commands = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
@@ -43,6 +43,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
};
 
git_config(git_default_config, NULL);
+   git_config_get_bool("rebase.abbreviatecommands", _commands);
 
opts.action = REPLAY_INTERACTIVE_REBASE;
opts.allow_ff = 1;
@@ -56,11 +57,12 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
-   return !!sequencer_make_script(keep_empty, stdout, argc, argv);
+   return !!sequencer_make_script(keep_empty, abbreviate_commands,
+  stdout, argc, argv);
if (command == SHORTEN_SHA1S && argc == 1)
-   return !!transform_todo_ids(1);
+   return !!transform_todo_ids(1, abbreviate_commands);
if (command == EXPAND_SHA1S && argc == 1)
-   return !!transform_todo_ids(0);
+   return !!transform_todo_ids(0, abbreviate_commands);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 810b7850748e..aa01e8bd9280 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -795,6 +795,13 @@ static const char *command_to_string(const enum 
todo_command command)
die("Unknown command: %d", command);
 }
 
+static const char command_to_char(const enum todo_command command)
+{
+   if (command < TODO_COMMENT && todo_command_info[command].c)
+   return todo_command_info[command].c;
+   return -1;
+}
+
 static int is_noop(const enum todo_command command)
 {
return TODO_NOOP <= command;
@@ -1242,15 +1249,16 @@ static int parse_insn_line(struct todo_item *item, 
const char *bol, char *eol)
return 0;
}
 
-   for (i = 0; i < TODO_COMMENT; i++)
+   for (i = 0; i < TODO_COMMENT; i++) {
if (skip_prefix(bol, todo_command_info[i].str, )) {
item->command = i;
break;
-   } else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
+   } else if (bol[1] == ' ' && *bol == command_to_char(i)) {
bol++;
item->command = i;
break;
}
+   }
if (i >= TODO_COMMENT)
return -1;
 
@@ -2443,8 +2451,8 @@ void append_signoff(struct strbuf *msgbuf, int 
ignore_footer, unsigned

[PATCH 5/5] t3404: add test case for abbreviated commands

[Liam Beguin]

Make sure the todo list ends up using single-letter command
abbreviations when the rebase.abbreviateCommands is enabled.
This configuration options should not change anything else.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 t/t3404-rebase-interactive.sh | 32 
 1 file changed, 32 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6a82d1ed876d..e460ebde3393 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1260,6 +1260,38 @@ test_expect_success 'rebase -i respects 
rebase.missingCommitsCheck = error' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'prepare rebase.abbreviateCommands' '
+   reset_rebase &&
+   git checkout -b abbrevcmd master &&
+   test_commit "first" file1.txt "first line" first &&
+   test_commit "second" file1.txt "another line" second &&
+   test_commit "fixup! first" file2.txt "first line again" first_fixup &&
+   test_commit "squash! second" file1.txt "another line here" second_squash
+'
+
+cat >expected <actual &&
+   test_cmp expected actual
+'
+
 test_expect_success 'static check of bad command' '
rebase_setup_and_clean bad-cmd &&
set_fake_editor &&
-- 
2.15.0.321.g19bf2bb99cee.dirty

[PATCH 1/5] Documentation: move rebase.* configs to new file

[Liam Beguin]

Move all rebase.* configuration variables to a separate file in order to
remove duplicates, and include it in config.txt and git-rebase.txt.  The
new descriptions are mostly taken from config.txt as they are more
verbose.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/config.txt| 31 +--
 Documentation/git-rebase.txt| 19 +--
 Documentation/rebase-config.txt | 32 
 3 files changed, 34 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 531649cb40ea..e424b7de90b5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2691,36 +2691,7 @@ push.recurseSubmodules::
is retained. You may override this configuration at time of push by
specifying '--recurse-submodules=check|on-demand|no'.
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   When set to true, automatically create a temporary stash entry
-   before the operation begins, and apply it after the operation
-   ends.  This means that you can run rebase on a dirty worktree.
-   However, use with care: the final stash application after a
-   successful rebase might result in non-trivial conflicts.
-   Defaults to false.
-
-rebase.missingCommitsCheck::
-   If set to "warn", git rebase -i will print a warning if some
-   commits are removed (e.g. a line was deleted), however the
-   rebase will still proceed. If set to "error", it will print
-   the previous warning and stop the rebase, 'git rebase
-   --edit-todo' can then be used to correct the error. If set to
-   "ignore", no checking is done.
-   To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
-   Defaults to "ignore".
-
-rebase.instructionFormat::
-   A format string, as specified in linkgit:git-log[1], to be used for
-   the instruction list during an interactive rebase.  The format will 
automatically
-   have the long commit hash prepended to the format.
+include::rebase-config.txt[]
 
 receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3cedfb0fd22b..8a861c1e0d69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -203,24 +203,7 @@ Alternatively, you can undo the 'git rebase' with
 CONFIGURATION
 -
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   If set to true enable `--autostash` option by default.
-
-rebase.missingCommitsCheck::
-   If set to "warn", print warnings about removed commits in
-   interactive mode. If set to "error", print the warnings and
-   stop the rebase. If set to "ignore", no checking is
-   done. "ignore" by default.
-
-rebase.instructionFormat::
-   Custom commit list format to use during an `--interactive` rebase.
+include::rebase-config.txt[]
 
 OPTIONS
 ---
diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
new file mode 100644
index ..dba088d7c68f
--- /dev/null
+++ b/Documentation/rebase-config.txt
@@ -0,0 +1,32 @@
+rebase.stat::
+   Whether to show a diffstat of what changed upstream since the last
+   rebase. False by default.
+
+rebase.autoSquash::
+   If set to true enable `--autosquash` option by default.
+
+rebase.autoStash::
+   When set to true, automatically create a temporary stash entry
+   before the operation begins, and apply it after the operation
+   ends.  This means that you can run rebase on a dirty worktree.
+   However, use with care: the final stash application after a
+   successful rebase might result in non-trivial conflicts.
+   This option can be overridden by the `--no-autostash` and
+   `--autostash` options of linkgit:git-rebase[1].
+   Defaults to false.
+
+rebase.missingCommitsCheck::
+   If set to "warn", git rebase -i will print a warning if some
+   commits are removed (e.g. a line was deleted), however the
+   rebase will still proceed. If set to "error", it will print
+   the previous warning and stop the rebase, 'git rebase
+   --edit-todo' can then be used to correct the error. If set to
+   "ignore", no checking is done.
+   To drop a commit without warning or error, use the `drop`
+   command

[PATCH 2/5] Documentation: use preferred name for the 'todo list' script

[Liam Beguin]

Use "todo list" instead of "instruction list" or "todo-list" to
reduce further confusion regarding the name of this script.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/rebase-config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index dba088d7c68f..30ae08cb5a4b 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -23,10 +23,10 @@ rebase.missingCommitsCheck::
--edit-todo' can then be used to correct the error. If set to
"ignore", no checking is done.
To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
+   command in the todo list.
Defaults to "ignore".
 
 rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for the
-   instruction list during an interactive rebase.  The format will
+   todo list during an interactive rebase.  The format will
automatically have the long commit hash prepended to the format.
-- 
2.15.0.321.g19bf2bb99cee.dirty

Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper

[Liam Beguin]

On 19/06/17 05:45 AM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sat, 17 Jun 2017, Liam Beguin wrote:
> 
>> On 16/06/17 09:56 AM, Johannes Schindelin wrote:
>>
>>> On Thu, 15 Jun 2017, Liam Beguin wrote:
>>>
>>>> On 14/06/17 09:08 AM, Johannes Schindelin wrote:
>>>>> diff --git a/sequencer.c b/sequencer.c
>>>>> index a697906d463..a0e020dab09 100644
>>>>> --- a/sequencer.c
>>>>> +++ b/sequencer.c
>>>>> @@ -2640,3 +2640,110 @@ int check_todo_list(void)
>>>>>  
>>>>>   return res;
>>>>>  }
>>>>> +
>>>>> +/* skip picking commits whose parents are unchanged */
>>>>> +int skip_unnecessary_picks(void)
>>>>> +{
>>>>> + const char *todo_file = rebase_path_todo();
>>>>> + struct strbuf buf = STRBUF_INIT;
>>>>> + struct todo_list todo_list = TODO_LIST_INIT;
>>>>> + struct object_id onto_oid, *oid = _oid, *parent_oid;
>>>>> + int fd, i;
>>>>> +
>>>>> + if (!read_oneliner(, rebase_path_onto(), 0))
>>>>> + return error(_("could not read 'onto'"));
>>>>> + if (get_sha1(buf.buf, onto_oid.hash)) {
>>>>
>>>> I missed this last time but we could also replace `get_sha1` with
>>>> `get_oid`
>>>
>>> Good point!
>>>
>>> I replaced this locally and force-pushed, but there is actually little
>>> chance of this patch series being integrated in a form with which I
>>> would be comfortable.
>>
>> Is there any chance of this moving forward? I was hoping to resend my
>> path to abbreviate command names on top of this.
> 
> We are at an impasse right now.
> 
> Junio wants me to change this code:
> 
> revs.pretty_given = 1;
> git_config_get_string("rebase.instructionFormat", );
> if (!format || !*format) {
> free(format);
> format = xstrdup("%s");
> }
> get_commit_format(format, );
> free(format);
> pp.fmt = revs.commit_format;
> pp.output_encoding = get_log_output_encoding();
> 
> if (setup_revisions(argc, argv, , NULL) > 1)
>   ...
> 
> which is reasonably compile-time safe, to something like this:
> 
>   struct strbuf userformat = STRBUF_INIT;
>   struct argv_array args = ARGV_ARRAY_INIT;
>   ...
>   for (i = 0; i < argc; i++)
>   argv_array_push(, argv[i]);
> git_config_get_string("rebase.instructionFormat", );
> if (!format || !*format)
>   argv_array_push(, "--format=%s");
>   else {
>   strbuf_addf(, "--format=%s", format);
> argv_array_push(, userformat.buf);
> }
> 
> if (setup_revisions(args.argc, args.argv, , NULL) > 1)
>   ...
>   argv_array_clear();
>   strbuf_release();
> 
> which is not compile-time safe, harder to read, sloppy coding in my book.
> 
> The reason for this suggestion is that one of the revision machinery's
> implementation details is an ugly little semi-secret: the pretty-printing
> machinery uses a global state, and that is why we need the "pretty_given"
> flag in the first place.
> 
> Junio thinks that it would be better to not use the pretty_given flag in
> this patch series. I disagree: It would be better to use the pretty_given
> flag, also as a good motivator to work on removing the global state in the
> pretty-printing machinery.
> 
> Junio wants to strong-arm me into accepting authorship for this sloppy
> coding, and I simply won't do it.
> 
> That's why we are at an impasse right now.
> 
> I am really, really sorry that this affects your patch series, as I had
> not foreseen Junio's insistence on the sloppy coding when I suggested that
> you rebase your work on top of my patch series. I just was really
> unprepared for this contention, and I am still surprised/shocked that this
> is even an issue up for discussion.
> 
> Be that as it may, I see that this is a very bad experience for a
> motivated contributor such as yourself. I am very sorry about that!

It's not such a bad experience, I've found the people on the list to 
be quite welcoming and supportive.

> 
> So it may actually be better for you to go forward with your original
> patch series targeting git-rebase--interactive.sh.

I'll wait a bit longer to see how this goes and if it doesn't move, I'll
try re-sending an update to that series.

> 
> My apologies,
> Dscho
> 

Thanks,
Liam 

Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper

[Liam Beguin]

Hi Johannes, 

On 16/06/17 09:56 AM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Thu, 15 Jun 2017, Liam Beguin wrote:
> 
>> On 14/06/17 09:08 AM, Johannes Schindelin wrote:
>>> diff --git a/sequencer.c b/sequencer.c
>>> index a697906d463..a0e020dab09 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -2640,3 +2640,110 @@ int check_todo_list(void)
>>>  
>>> return res;
>>>  }
>>> +
>>> +/* skip picking commits whose parents are unchanged */
>>> +int skip_unnecessary_picks(void)
>>> +{
>>> +   const char *todo_file = rebase_path_todo();
>>> +   struct strbuf buf = STRBUF_INIT;
>>> +   struct todo_list todo_list = TODO_LIST_INIT;
>>> +   struct object_id onto_oid, *oid = _oid, *parent_oid;
>>> +   int fd, i;
>>> +
>>> +   if (!read_oneliner(, rebase_path_onto(), 0))
>>> +   return error(_("could not read 'onto'"));
>>> +   if (get_sha1(buf.buf, onto_oid.hash)) {
>>
>> I missed this last time but we could also replace `get_sha1` with `get_oid`
> 
> Good point!
> 
> I replaced this locally and force-pushed, but there is actually little
> chance of this patch series being integrated in a form with which I would
> be comfortable.

Is there any chance of this moving forward? I was hoping to resend my path to
abbreviate command names on top of this.

> 
> Ciao,
> Dscho
> 
Thanks,

 - Liam

[PATCH v2 1/3] stash: update documentation to use 'stash entry'

[Liam Beguin]

Most of the time, a 'stash entry' is called a 'stash'. Lets try to make
this more consistent and use 'stash entry' instead.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/config.txt |  6 ++---
 Documentation/git-pull.txt   |  2 +-
 Documentation/git-rebase.txt |  2 +-
 Documentation/git-stash.txt  | 60 +++-
 Documentation/gitcli.txt |  2 +-
 git-stash.sh |  6 ++---
 6 files changed, 40 insertions(+), 38 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6278a5ae6a1..23b807065d92 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2620,7 +2620,7 @@ rebase.autoSquash::
If set to true enable `--autosquash` option by default.
 
 rebase.autoStash::
-   When set to true, automatically create a temporary stash
+   When set to true, automatically create a temporary stash entry
before the operation begins, and apply it after the operation
ends.  This means that you can run rebase on a dirty worktree.
However, use with care: the final stash application after a
@@ -3029,12 +3029,12 @@ status.submoduleSummary::
 
 stash.showPatch::
If this is set to true, the `git stash show` command without an
-   option will show the stash in patch form.  Defaults to false.
+   option will show the stash entry in patch form.  Defaults to false.
See description of 'show' command in linkgit:git-stash[1].
 
 stash.showStat::
If this is set to true, the `git stash show` command without an
-   option will show diffstat of the stash.  Defaults to true.
+   option will show diffstat of the stash entry.  Defaults to true.
See description of 'show' command in linkgit:git-stash[1].
 
 submodule..url::
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index e414185f5a6a..9db5e08f4a63 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -131,7 +131,7 @@ unless you have read linkgit:git-rebase[1] carefully.
 --autostash::
 --no-autostash::
Before starting rebase, stash local modifications away (see
-   linkgit:git-stash[1]) if needed, and apply the stash when
+   linkgit:git-stash[1]) if needed, and apply the stash entry when
done. `--no-autostash` is useful to override the `rebase.autoStash`
configuration variable (see linkgit:git-config[1]).
 +
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 53f4e14a..a5afd602d8eb 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -446,7 +446,7 @@ used to override and disable this setting.
 
 --autostash::
 --no-autostash::
-   Automatically create a temporary stash before the operation
+   Automatically create a temporary stash entry before the operation
begins, and apply it after the operation ends.  This means
that you can run rebase on a dirty worktree.  However, use
with care: the final stash application after a successful
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 70191d06b69e..00f95fee1faf 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -51,18 +51,18 @@ OPTIONS
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] []::
 push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] [-m|--message ] [--] [...]::
 
-   Save your local modifications to a new 'stash' and roll them
+   Save your local modifications to a new 'stash entry' and roll them
back to HEAD (in the working tree and in the index).
The  part is optional and gives
the description along with the stashed state.
 +
 For quickly making a snapshot, you can omit "push".  In this mode,
 non-option arguments are not allowed to prevent a misspelled
-subcommand from making an unwanted stash.  The two exceptions to this
+subcommand from making an unwanted stash entry.  The two exceptions to this
 are `stash -p` which acts as alias for `stash push -p` and pathspecs,
 which are allowed after a double hyphen `--` for disambiguation.
 +
-When pathspec is given to 'git stash push', the new stash records the
+When pathspec is given to 'git stash push', the new stash entry records the
 modified states only for the files that match the pathspec.  The index
 entries and working tree files are then rolled back to the state in
 HEAD only for these files, too, leaving files that do not match the
@@ -89,10 +89,10 @@ The `--patch` option implies `--keep-index`.  You can use
 
 list []::
 
-   List the stashes that you currently have.  Each 'stash' is listed
-   with its name (e.g. `stash@{0}` is the latest stash, `stash@{1}` is
+   List the stash entries that you currently have.  Each 'stash entry' is
+   listed with its name (e.g. `stash@{0}` is the latest 

[PATCH v2 2/3] status: add optional stash count information

[Liam Beguin]

Introduce '--show-stash' and its configuration option 'status.showStash'
to allow git-status to show information about currently stashed entries.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/config.txt |  5 +
 Documentation/git-status.txt |  3 +++
 builtin/commit.c |  6 ++
 t/t7508-status.sh| 32 
 wt-status.c  | 24 
 wt-status.h  |  1 +
 6 files changed, 71 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 23b807065d92..e83b0f641574 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2992,6 +2992,11 @@ status.displayCommentPrefix::
behavior of linkgit:git-status[1] in Git 1.8.4 and previous.
Defaults to false.
 
+status.showStash::
+   If set to true, linkgit:git-status[1] will display the number of
+   entries currently stashed away.
+   Defaults to false.
+
 status.showUntrackedFiles::
By default, linkgit:git-status[1] and linkgit:git-commit[1] show
files which are not currently tracked by Git. Directories which
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index d70abc6afe3a..d47f198f15cd 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -32,6 +32,9 @@ OPTIONS
 --branch::
Show the branch and tracking info even in short-format.
 
+--show-stash::
+   Show the number of entries currently stashed away.
+
 --porcelain[=]::
Give the output in an easy-to-parse format for scripts.
This is similar to the short output, but will remain stable
diff --git a/builtin/commit.c b/builtin/commit.c
index ef52457effc1..c089fb87e363 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1295,6 +1295,10 @@ static int git_status_config(const char *k, const char 
*v, void *cb)
status_deferred_config.show_branch = git_config_bool(k, v);
return 0;
}
+   if (!strcmp(k, "status.showstash")) {
+   s->show_stash = git_config_bool(k, v);
+   return 0;
+   }
if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
s->use_color = git_config_colorbool(k, v);
return 0;
@@ -1343,6 +1347,8 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
N_("show status concisely"), STATUS_FORMAT_SHORT),
OPT_BOOL('b', "branch", _branch,
 N_("show branch information")),
+   OPT_BOOL(0, "show-stash", _stash,
+N_("show stash information")),
{ OPTION_CALLBACK, 0, "porcelain", _format,
  N_("version"), N_("machine-readable output"),
  PARSE_OPT_OPTARG, opt_parse_porcelain },
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 79427840a4fa..7121a550c7ce 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1608,4 +1608,36 @@ test_expect_success 'git commit -m will commit a staged 
but ignored submodule' '
git config -f .gitmodules  --remove-section submodule.subname
 '
 
+test_expect_success 'show stash info with "--show-stash"' '
+   git reset --hard &&
+   git stash clear &&
+   echo 1 >file &&
+   git add file &&
+   git stash &&
+   git status >expected_default &&
+   git status --show-stash >expected_with_stash &&
+   test_i18ngrep "^Your stash currently has 1 entry$" expected_with_stash
+'
+
+test_expect_success 'no stash info with "--show-stash --no-show-stash"' '
+   git status --show-stash --no-show-stash >expected_without_stash &&
+   test_cmp expected_default expected_without_stash
+'
+
+test_expect_success '"status.showStash=false" weaker than "--show-stash"' '
+   git -c status.showStash=false status --show-stash >actual &&
+   test_cmp expected_with_stash actual
+'
+
+test_expect_success '"status.showStash=true" weaker than "--no-show-stash"' '
+   git -c status.showStash=true status --no-show-stash >actual &&
+   test_cmp expected_without_stash actual
+'
+
+test_expect_success 'no additionnal info if no stash entries' '
+   git stash clear &&
+   git -c status.showStash=true status >actual &&
+   test_cmp expected_without_stash actual
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index bf651f16fae8..7992a73902ae 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -137,6 +137,7 @@ void wt_status_prepare(struct wt_status *s)
s->untracked.strdup_strings = 1;
s->ignored.strdup_strings = 1;
   

[PATCH v2 3/3] glossary: define 'stash entry'

[Liam Beguin]

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/glossary-content.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/glossary-content.txt 
b/Documentation/glossary-content.txt
index 6e991c246915..b71b943b12ed 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -570,6 +570,10 @@ The most notable example is `HEAD`.
is created by giving the `--depth` option to linkgit:git-clone[1], and
its history can be later deepened with linkgit:git-fetch[1].
 
+[[def_stash]]stash entry::
+   An <<def_object,object>> used to temporarily store the contents of a
+   <<def_dirty,dirty>> working directory and the index for future reuse.
+
 [[def_submodule]]submodule::
A <<def_repository,repository>> that holds the history of a
separate project inside another repository (the latter of
-- 
2.9.4

[PATCH v2 0/3] add stash count information to git-status command

[Liam Beguin]

As discussed here [*1*], this allows `git status` to show the number of
entries currently stashed away.

I also tried to update the related parts of the documentation to use
'stash entry' instead of 'stash' as we agreed that it was a bit better.

*1* 
https://public-inbox.org/git/ca+b9myhrahtd+fdgzk5ahxw+hq_y_czmx9x6mxybcr9wspe...@mail.gmail.com/

Liam Beguin (3):
  stash: update documentation to use 'stash entry'
  status: add optional stash count information
  glossary: define 'stash entry'

 Documentation/config.txt   | 11 +--
 Documentation/git-pull.txt |  2 +-
 Documentation/git-rebase.txt   |  2 +-
 Documentation/git-stash.txt| 60 --
 Documentation/git-status.txt   |  3 ++
 Documentation/gitcli.txt   |  2 +-
 Documentation/glossary-content.txt |  4 +++
 builtin/commit.c   |  6 
 git-stash.sh   |  6 ++--
 t/t7508-status.sh  | 32 
 wt-status.c| 24 +++
 wt-status.h|  1 +
 12 files changed, 115 insertions(+), 38 deletions(-)


Base-commit: 97e2ff464302565877a00b8a9aa6a2d85bd1445e

Changes since v1:
 - update commit messages to be more consistent
 - improve Documentation based on feedback
 - move config lookup to `git_status_config()`
 - add '--show-stash' command line option to `git-status`
 - add tests for now option

Interdiff:
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 59979ad31dfe..00f95fee1faf 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -58,7 +58,7 @@ push [-p|--patch] [-k|--[no-]keep-index] 
[-u|--include-untracked] [-a|--all] [-q
 +
 For quickly making a snapshot, you can omit "push".  In this mode,
 non-option arguments are not allowed to prevent a misspelled
-subcommand from making an unwanted entry.  The two exceptions to this
+subcommand from making an unwanted stash entry.  The two exceptions to this
 are `stash -p` which acts as alias for `stash push -p` and pathspecs,
 which are allowed after a double hyphen `--` for disambiguation.
 +
@@ -106,10 +106,11 @@ command to control what is shown and how. See 
linkgit:git-log[1].
 show []::
 
Show the changes recorded in the stash entry as a diff between the
-   stashed entry and its original parent. When no `` is given, it
-   shows the latest one. By default, the command shows the diffstat, but
-   it will accept any format known to 'git diff' (e.g., `git stash show
-   -p stash@{1}` to view the second most recent entry in patch form).
+   stashed contents and the commit back when the stash entry was first
+   created. When no `` is given, it shows the latest one.
+   By default, the command shows the diffstat, but it will accept any
+   format known to 'git diff' (e.g., `git stash show -p stash@{1}`
+   to view the second most recent entry in patch form).
You can use stash.showStat and/or stash.showPatch config variables
to change the default behavior.
 
@@ -150,20 +151,20 @@ branch  []::
 This is useful if the branch on which you ran `git stash save` has
 changed enough that `git stash apply` fails due to conflicts. Since
 the stash entry is applied on top of the commit that was HEAD at the
-time `git stash` was run, it restores the originally stashed entry
+time `git stash` was run, it restores the originally stashed state
 with no conflicts.
 
 clear::
-   Remove all the stashed entries. Note that those entries will then
+   Remove all the stash entries. Note that those entries will then
be subject to pruning, and may be impossible to recover (see
'Examples' below for a possible strategy).
 
 drop [-q|--quiet] []::
 
-   Remove a single stashed entry from the stash list. When no ``
-   is given, it removes the latest one. i.e. `stash@{0}`, otherwise
-   `` must be a valid stash log reference of the form
-   `stash@{}`.
+   Remove a single stash entry from the list of stash entries.
+   When no `` is given, it removes the latest one.
+   i.e. `stash@{0}`, otherwise `` must be a valid stash
+   log reference of the form `stash@{}`.
 
 create::
 
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index d70abc6afe3a..d47f198f15cd 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -32,6 +32,9 @@ OPTIONS
 --branch::
Show the branch and tracking info even in short-format.
 
+--show-stash::
+   Show the number of entries currently stashed away.
+
 --porcelain[=]::
Give the output in an easy-to-parse format for scripts.
This is similar to the short output, but will remain stable
diff --git a/Documentation/glossary-content.txt 
b/Documentation/glossary-content.txt
index 026f66e7240a..b71b943b12ed 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentatio

Re: [PATCH 0/3] add stash count information to git-status command

[Liam Beguin]

Hi, 

On 16/06/17 08:16 AM, Jeff King wrote:
> On Fri, Jun 16, 2017 at 12:30:47AM -0400, Liam Beguin wrote:
> 
>> As discussed here [*1*], this allows `git status` to show the number of
>> entries currently stashed away.
>>
>> I also tried to update the related parts of the documentation to use
>> 'stash entry' instead of 'stash' as we agreed that it was a bit better.
>> I don't mind dropping the documentation update and using something like
>> "You have %d stash/stashes" in the status message if it makes the change
>> "too big".
> 
> I like the overall direction (including the documentation update). I
> noted a few minor problems in the various patches, though.

Thanks for reviewing! I'll try to send an update later today.

> 
> -Peff
> 

 - Liam

[PATCH 1/3] stash: update documentation to use 'stash entries'

[Liam Beguin]

Most of the time, a 'stash entry' is called a 'stash'
or a 'stash state'. Lets use 'stash entry' instead.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/config.txt |  6 +++---
 Documentation/git-pull.txt   |  2 +-
 Documentation/git-rebase.txt |  2 +-
 Documentation/git-stash.txt  | 49 ++--
 Documentation/gitcli.txt |  2 +-
 git-stash.sh |  6 +++---
 6 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6278a5ae6a1..23b807065d92 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2620,7 +2620,7 @@ rebase.autoSquash::
If set to true enable `--autosquash` option by default.
 
 rebase.autoStash::
-   When set to true, automatically create a temporary stash
+   When set to true, automatically create a temporary stash entry
before the operation begins, and apply it after the operation
ends.  This means that you can run rebase on a dirty worktree.
However, use with care: the final stash application after a
@@ -3029,12 +3029,12 @@ status.submoduleSummary::
 
 stash.showPatch::
If this is set to true, the `git stash show` command without an
-   option will show the stash in patch form.  Defaults to false.
+   option will show the stash entry in patch form.  Defaults to false.
See description of 'show' command in linkgit:git-stash[1].
 
 stash.showStat::
If this is set to true, the `git stash show` command without an
-   option will show diffstat of the stash.  Defaults to true.
+   option will show diffstat of the stash entry.  Defaults to true.
See description of 'show' command in linkgit:git-stash[1].
 
 submodule..url::
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index e414185f5a6a..9db5e08f4a63 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -131,7 +131,7 @@ unless you have read linkgit:git-rebase[1] carefully.
 --autostash::
 --no-autostash::
Before starting rebase, stash local modifications away (see
-   linkgit:git-stash[1]) if needed, and apply the stash when
+   linkgit:git-stash[1]) if needed, and apply the stash entry when
done. `--no-autostash` is useful to override the `rebase.autoStash`
configuration variable (see linkgit:git-config[1]).
 +
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 53f4e14a..a5afd602d8eb 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -446,7 +446,7 @@ used to override and disable this setting.
 
 --autostash::
 --no-autostash::
-   Automatically create a temporary stash before the operation
+   Automatically create a temporary stash entry before the operation
begins, and apply it after the operation ends.  This means
that you can run rebase on a dirty worktree.  However, use
with care: the final stash application after a successful
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 70191d06b69e..59979ad31dfe 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -51,18 +51,18 @@ OPTIONS
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] []::
 push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] [-m|--message ] [--] [...]::
 
-   Save your local modifications to a new 'stash' and roll them
+   Save your local modifications to a new 'stash entry' and roll them
back to HEAD (in the working tree and in the index).
The  part is optional and gives
the description along with the stashed state.
 +
 For quickly making a snapshot, you can omit "push".  In this mode,
 non-option arguments are not allowed to prevent a misspelled
-subcommand from making an unwanted stash.  The two exceptions to this
+subcommand from making an unwanted entry.  The two exceptions to this
 are `stash -p` which acts as alias for `stash push -p` and pathspecs,
 which are allowed after a double hyphen `--` for disambiguation.
 +
-When pathspec is given to 'git stash push', the new stash records the
+When pathspec is given to 'git stash push', the new stash entry records the
 modified states only for the files that match the pathspec.  The index
 entries and working tree files are then rolled back to the state in
 HEAD only for these files, too, leaving files that do not match the
@@ -89,10 +89,10 @@ The `--patch` option implies `--keep-index`.  You can use
 
 list []::
 
-   List the stashes that you currently have.  Each 'stash' is listed
-   with its name (e.g. `stash@{0}` is the latest stash, `stash@{1}` is
+   List the stash entries that you currently have.  Each 'stash entry' is
+   listed with its name (e.g. `stash@{0}` is the latest entry, `stash@{1}` 
is
the

[PATCH 2/3] wt-status: add optional stash status information

[Liam Beguin]

Add the `status.showStash` configuration option to allow git-status to
show information about currently stashed entries.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/config.txt |  5 +
 wt-status.c  | 24 
 2 files changed, 29 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 23b807065d92..e83b0f641574 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2992,6 +2992,11 @@ status.displayCommentPrefix::
behavior of linkgit:git-status[1] in Git 1.8.4 and previous.
Defaults to false.
 
+status.showStash::
+   If set to true, linkgit:git-status[1] will display the number of
+   entries currently stashed away.
+   Defaults to false.
+
 status.showUntrackedFiles::
By default, linkgit:git-status[1] and linkgit:git-commit[1] show
files which are not currently tracked by Git. Directories which
diff --git a/wt-status.c b/wt-status.c
index bf651f16fae8..7114eec123c8 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -801,6 +801,27 @@ static void wt_longstatus_print_changed(struct wt_status 
*s)
wt_longstatus_print_trailer(s);
 }
 
+static int stash_count_refs(struct object_id *ooid, struct object_id *noid,
+   const char *email, timestamp_t timestamp, int tz,
+   const char *message, void *cb_data)
+{
+   int *c = cb_data;
+   (*c)++;
+   return 0;
+}
+
+static void wt_longstatus_print_stash_summary(struct wt_status *s)
+{
+   int stash_count = 0;
+
+   for_each_reflog_ent("refs/stash", stash_count_refs, _count);
+   if (stash_count > 0)
+   status_printf_ln(s, GIT_COLOR_NORMAL,
+Q_("Your stash currently has %d entry",
+   "Your stash currently has %d entries", 
stash_count),
+stash_count);
+}
+
 static void wt_longstatus_print_submodule_summary(struct wt_status *s, int 
uncommitted)
 {
struct child_process sm_summary = CHILD_PROCESS_INIT;
@@ -1537,6 +1558,7 @@ 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);
struct wt_status_state state;
+   int show_stash = 0;
 
memset(, 0, sizeof(state));
wt_status_get_state(,
@@ -1642,6 +1664,8 @@ static void wt_longstatus_print(struct wt_status *s)
} else
printf(_("nothing to commit, working tree clean\n"));
}
+   if (!git_config_get_bool("status.showStash", _stash) && show_stash)
+   wt_longstatus_print_stash_summary(s);
 }
 
 static void wt_shortstatus_unmerged(struct string_list_item *it,
-- 
2.9.4

[PATCH 3/3] glossary: define stash entries

[Liam Beguin]

Add glossary entry for "stash entries".

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/glossary-content.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/glossary-content.txt 
b/Documentation/glossary-content.txt
index 6e991c246915..026f66e7240a 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -570,6 +570,10 @@ The most notable example is `HEAD`.
is created by giving the `--depth` option to linkgit:git-clone[1], and
its history can be later deepened with linkgit:git-fetch[1].
 
+[[def_stash]]stash entry::
+   An <<def_object,object>> used to temporarily store the content of a
+   <<def_dirty,dirty>> working directory for futur reuse.
+
 [[def_submodule]]submodule::
A <<def_repository,repository>> that holds the history of a
separate project inside another repository (the latter of
-- 
2.9.4

[PATCH 0/3] add stash count information to git-status command

[Liam Beguin]

As discussed here [*1*], this allows `git status` to show the number of
entries currently stashed away.

I also tried to update the related parts of the documentation to use
'stash entry' instead of 'stash' as we agreed that it was a bit better.
I don't mind dropping the documentation update and using something like
"You have %d stash/stashes" in the status message if it makes the change
"too big".

*1* 
https://public-inbox.org/git/ca+b9myhrahtd+fdgzk5ahxw+hq_y_czmx9x6mxybcr9wspe...@mail.gmail.com/

Liam Beguin (3):
  stash: update documentation to use 'stash entries'
  wt-status: add optional stash status information
  glossary: define stash entries

 Documentation/config.txt   | 11 ++---
 Documentation/git-pull.txt |  2 +-
 Documentation/git-rebase.txt   |  2 +-
 Documentation/git-stash.txt| 49 +++---
 Documentation/gitcli.txt   |  2 +-
 Documentation/glossary-content.txt |  4 
 git-stash.sh   |  6 ++---
 wt-status.c| 24 +++
 8 files changed, 67 insertions(+), 33 deletions(-)

-- 
2.9.4

Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper

[Liam Beguin]

Hi, 

On 14/06/17 09:08 AM, Johannes Schindelin wrote:
> diff --git a/sequencer.c b/sequencer.c
> index a697906d463..a0e020dab09 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2640,3 +2640,110 @@ int check_todo_list(void)
>  
>   return res;
>  }
> +
> +/* skip picking commits whose parents are unchanged */
> +int skip_unnecessary_picks(void)
> +{
> + const char *todo_file = rebase_path_todo();
> + struct strbuf buf = STRBUF_INIT;
> + struct todo_list todo_list = TODO_LIST_INIT;
> + struct object_id onto_oid, *oid = _oid, *parent_oid;
> + int fd, i;
> +
> + if (!read_oneliner(, rebase_path_onto(), 0))
> + return error(_("could not read 'onto'"));
> + if (get_sha1(buf.buf, onto_oid.hash)) {

I missed this last time but we could also replace `get_sha1` with `get_oid`

> + strbuf_release();
> + return error(_("need a HEAD to fixup"));
> + }
> + strbuf_release();
> +
> + fd = open(todo_file, O_RDONLY);
> + if (fd < 0) {
> + return error_errno(_("could not open '%s'"), todo_file);
> + }
> + if (strbuf_read(_list.buf, fd, 0) < 0) {
> + close(fd);
> + return error(_("could not read '%s'."), todo_file);
> + }
> + close(fd);
> + if (parse_insn_buffer(todo_list.buf.buf, _list) < 0) {
> + todo_list_release(_list);
> + return -1;
> + }
> +
> + for (i = 0; i < todo_list.nr; i++) {
> + struct todo_item *item = todo_list.items + i;
> +
> + if (item->command >= TODO_NOOP)
> + continue;
> + if (item->command != TODO_PICK)
> + break;
> + if (parse_commit(item->commit)) {
> + todo_list_release(_list);
> + return error(_("could not parse commit '%s'"),
> + oid_to_hex(>commit->object.oid));
> + }
> + if (!item->commit->parents)
> + break; /* root commit */
> + if (item->commit->parents->next)
> + break; /* merge commit */
> + parent_oid = >commit->parents->item->object.oid;
> + if (hashcmp(parent_oid->hash, oid->hash))
> + break;
> + oid = >commit->object.oid;
> + }
> + if (i > 0) {
> + int offset = i < todo_list.nr ?
> + todo_list.items[i].offset_in_buf : todo_list.buf.len;
> + const char *done_path = rebase_path_done();
> +
> + fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666);
> + if (fd < 0) {
> + error_errno(_("could not open '%s' for writing"),
> + done_path);
> + todo_list_release(_list);
> + return -1;
> + }
> + if (write_in_full(fd, todo_list.buf.buf, offset) < 0) {
> + error_errno(_("could not write to '%s'"), done_path);
> + todo_list_release(_list);
> + close(fd);
> + return -1;
> + }
> + close(fd);
> +
> + fd = open(rebase_path_todo(), O_WRONLY, 0666);
> + if (fd < 0) {
> + error_errno(_("could not open '%s' for writing"),
> + rebase_path_todo());
> + todo_list_release(_list);
> + return -1;
> + }
> + if (write_in_full(fd, todo_list.buf.buf + offset,
> + todo_list.buf.len - offset) < 0) {
> + error_errno(_("could not write to '%s'"),
> + rebase_path_todo());
> + close(fd);
> + todo_list_release(_list);
> + return -1;
> + }
> + if (ftruncate(fd, todo_list.buf.len - offset) < 0) {
> + error_errno(_("could not truncate '%s'"),
> + rebase_path_todo());
> + todo_list_release(_list);
> + close(fd);
> + return -1;
> + }
> + close(fd);
> +
> + todo_list.current = i;
> + if (is_fixup(peek_command(_list, 0)))
> + record_in_rewritten(oid, peek_command(_list, 0));
> + }
> +
> + todo_list_release(_list);
> + printf("%s\n", oid_to_hex(oid));
> +
> + return 0;
> +}
> diff --git a/sequencer.h b/sequencer.h
> index 878dd296f8c..04a57e09a1d 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -50,6 +50,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
>  
>  int transform_todo_ids(int shorten_ids);
>  int check_todo_list(void);
> +int skip_unnecessary_picks(void);
>  
>  extern const char sign_off_header[];
>  
> 

 - Liam 

Re: Feature Request: Show status of the stash in git status command

[liam Beguin]

Hi, 

On 13/06/17 02:42 AM, Konstantin Khomoutov wrote:
> On Mon, Jun 12, 2017 at 11:42:44PM -0400, liam Beguin wrote:
> 
> [...]
>>> Conceptually, the contents of the stash are *not* commits, even
>>> though the implementation happens to use a commit to represent each
>>> stash entry.  Perhaps "has %d entry/entries" is an improvement, but
>>> a quick scanning of an early part of "git stash --help" tells me
>>> that
>>
>> what's different between a stash and a commit? 
> 
> The same that exists between an interface and a concrete implementation
> in a programming language.

Makes sense, I thought there was a more fundamental difference.

> 
> "A stash entry" is a concept which is defined to keep explicitly
> recorded untracked files and which can be applied, shown and deleted
> from the stash bag (well, you can create a branch off it as well).

I've noticed this but I don't understand when it can be used.
I'll try to find out more on this.

> 
> The fact a stash entry is a merge commit of two synthetic commits is an
> implementation detail.  It can be very useful at times for power users,
> but regular Git users need not be concerned with this.
> 
> Another fact worth reiterating that what the UI displays to the user is
> better to match what the user reads in the docs. ;-)
> 

I'll make changes as suggested by Junio. I slightly prefer
"Your stash has %d entry/entries" over "You have %d stash/stashes" 
but I'll go with what's used elsewhere in the documentation. 

Thanks,

 - Liam

Re: Feature Request: Show status of the stash in git status command

[liam Beguin]

Hi, 

Thanks for the feedback. I'll be sending a patch with the updates shortly!

On 12/06/17 11:35 AM, Junio C Hamano wrote:
> liam Beguin <liambeg...@gmail.com> writes:
> 
>> +static int stash_count_refs(struct object_id *ooid, struct object_id *noid,
>> +const char *email, timestamp_t timestamp, int tz,
>> +const char *message, void *cb_data)
>> +{
>> +int *c = cb_data;
>> +(*c)++;
>> +return 0;
>> +}
> 
> Count up, and tell the caller to keep going by returning 0.  That
> sounds sane.
> 
>> +static void wt_longstatus_print_stash_summary(struct wt_status *s)
>> +{
>> +int stash_count = 0;
>> +
>> +for_each_reflog_ent("refs/stash", stash_count_refs, _count);
> 
> And do so with a counter initialized to 0.  Also sane.
> 
>> +if (stash_count > 0)
>> +status_printf_ln(s, GIT_COLOR_NORMAL,
>> + Q_("Your stash currently has %d commit",
>> +"Your stash currently has %d commits", 
>> stash_count),
>> + stash_count);
> 
> Conceptually, the contents of the stash are *not* commits, even
> though the implementation happens to use a commit to represent each
> stash entry.  Perhaps "has %d entry/entries" is an improvement, but
> a quick scanning of an early part of "git stash --help" tells me
> that

what's different between a stash and a commit? 

> 
>   You have 1 stash / You have 4 stashes
> 
> would be the best, as the documentation calls each entry "a stash".
> E.g. "list" is explained to list "the stashes", and "show "
> is explained to show the changes recorded in "the stash".
> 
>> +}
>> +
>>  static void wt_longstatus_print_submodule_summary(struct wt_status *s, int 
>> uncommitted)
>>  {
>>  struct child_process sm_summary = CHILD_PROCESS_INIT;
>> @@ -1536,6 +1557,7 @@ 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);
>>  struct wt_status_state state;
>> +int show_stash = 0;
>>  
>>  memset(, 0, sizeof(state));
>>  wt_status_get_state(,
>> @@ -1641,6 +1663,8 @@ static void wt_longstatus_print(struct wt_status *s)
>>  } else
>>  printf(_("nothing to commit, working tree clean\n"));
>>  }
>> +if (!git_config_get_bool("status.showStash", _stash) && show_stash)
>> +wt_longstatus_print_stash_summary(s);
>>  }
> 
> Try to get "status.showstash" as a boolean, and only when it
> succeeds and the value is true, give this extra info (i.e. when the
> variable does not exist, do not complain and do not show).  Sounds
> sensible.
> 
> Overall the logic looks good to me; just the phrasing is
> questionable, relative to the existing documentation.
> 
> Thanks.
> 

Thanks,

 - Liam 

Re: Feature Request: Show status of the stash in git status command

[liam Beguin]

Hi,

As it looks like something easy enough for a beginner, I though I could
give it a try. Here is what it looks like. If it's good enough, I'll 
add a few lines to document 'status.showStash' and send a patch.

There is one thing I've noticed though. When using 'git stash pop', it
shows the the number of stashes before dropping the commit and I'm not
quite sure how to address this.

--

diff --git a/wt-status.c b/wt-status.c
index 25aafc35c833..fab66d4cd72e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -801,6 +801,27 @@ static void wt_longstatus_print_changed(struct wt_status 
*s)
wt_longstatus_print_trailer(s);
 }
 
+static int stash_count_refs(struct object_id *ooid, struct object_id *noid,
+   const char *email, timestamp_t timestamp, int tz,
+   const char *message, void *cb_data)
+{
+   int *c = cb_data;
+   (*c)++;
+   return 0;
+}
+
+static void wt_longstatus_print_stash_summary(struct wt_status *s)
+{
+   int stash_count = 0;
+
+   for_each_reflog_ent("refs/stash", stash_count_refs, _count);
+   if (stash_count > 0)
+   status_printf_ln(s, GIT_COLOR_NORMAL,
+Q_("Your stash currently has %d commit",
+   "Your stash currently has %d commits", 
stash_count),
+stash_count);
+}
+
 static void wt_longstatus_print_submodule_summary(struct wt_status *s, int 
uncommitted)
 {
struct child_process sm_summary = CHILD_PROCESS_INIT;
@@ -1536,6 +1557,7 @@ 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);
struct wt_status_state state;
+   int show_stash = 0;
 
memset(, 0, sizeof(state));
wt_status_get_state(,
@@ -1641,6 +1663,8 @@ static void wt_longstatus_print(struct wt_status *s)
} else
printf(_("nothing to commit, working tree clean\n"));
}
+   if (!git_config_get_bool("status.showStash", _stash) && show_stash)
+   wt_longstatus_print_stash_summary(s);
 }
 
 static void wt_shortstatus_unmerged(struct string_list_item *it,




On 10/06/17 06:22 AM, Jeff King wrote:
> On Sat, Jun 10, 2017 at 06:12:28AM -0400, Samuel Lijin wrote:
> 
>> On Sat, Jun 10, 2017 at 4:25 AM, Jeff King  wrote:
>>> On Wed, Jun 07, 2017 at 06:46:18PM -0400, Houston Fortney wrote:
>>>
 I sometimes forget about something that I stashed. It would be nice if
 the git status command would just say "There are x entries in the
 stash." It can say nothing if there is nothing stashed so it is
 usually not adding clutter.
>>>
>>> I think the clutter issue would depend on your workflow around stash.
>>>
>>> Some people carry tidbits in their stash for days or weeks. E.g., I
>>> sometimes start on an idea and decide it's not worth pursuing (or more
>>> likely, I post a snippet of a patch as a "how about this" to the mailing
>>> list but don't plan on taking it further). Rather than run "git reset
>>> --hard", I usually "git stash" the result. That means if I really do
>>> decide I want it back, I can prowl through the stash list and find it.
>>>
>>> All of which is to say that if we had such a feature, it should probably
>>> be optional. For some people it would be very useful, and for others it
>>> would be a nuisance.
>>
>> Perhaps there should be a flag for this if it is implemented, say
>> status.showStash?
> 
> Yes, that was what I was thinking.
> 
> -Peff
> 

 - Liam

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[liam Beguin]

Hi Johannes,

Johannes Schindelin  writes:
> The first step of an interactive rebase is to generate the so-called "todo
> script", to be stored in the state directory as "git-rebase-todo" and to
> be edited by the user.
>
> Originally, we adjusted the output of `git log ` using a simple
> sed script. Over the course of the years, the code became more
> complicated. We now use shell scripting to edit the output of `git log`
> conditionally, depending whether to keep "empty" commits (i.e. commits
> that do not change any files).
>
> On platforms where shell scripting is not native, this can be a serious
> drag. And it opens the door for incompatibilities between platforms when
> it comes to shell scripting or to Unix-y commands.
>
> Let's just re-implement the todo script generation in plain C, using the
> revision machinery directly.
>
> This is substantially faster, improving the speed relative to the
> shell script version of the interactive rebase from 2x to 3x on Windows.
>
> Note that the rearrange_squash() function in git-rebase--interactive
> relied on the fact that we set the "format" variable to the config setting
> rebase.instructionFormat. Relying on a side effect like this is no good,
> hence we explicitly perform that assignment (possibly again) in
> rearrange_squash().
>
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/rebase--helper.c   |  8 +++-
>  git-rebase--interactive.sh | 44 +
>  sequencer.c| 49 
> ++
>  sequencer.h|  3 +++
>  4 files changed, 82 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index ca1ebb2fa18..821058d452d 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -11,15 +11,19 @@ static const char * const builtin_rebase_helper_usage[] = 
> {
>  int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  {
>   struct replay_opts opts = REPLAY_OPTS_INIT;
> + int keep_empty = 0;
>   enum {
> - CONTINUE = 1, ABORT
> + CONTINUE = 1, ABORT, MAKE_SCRIPT
>   } command = 0;
>   struct option options[] = {
>   OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
> + OPT_BOOL(0, "keep-empty", _empty, N_("keep empty 
> commits")),
>   OPT_CMDMODE(0, "continue", , N_("continue rebase"),
>   CONTINUE),
>   OPT_CMDMODE(0, "abort", , N_("abort rebase"),
>   ABORT),
> + OPT_CMDMODE(0, "make-script", ,
> + N_("make rebase script"), MAKE_SCRIPT),
>   OPT_END()
>   };

This is probably being picky, but we could also use a different name
here instead of 'rebase script'. This would help keep the name of this
script consistent as you already pointed out.
maybe something like 'make-todo-list' or just 'make-list'?

>  
> @@ -36,5 +40,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   return !!sequencer_continue();
>   if (command == ABORT && argc == 1)
>   return !!sequencer_remove_state();
> + if (command == MAKE_SCRIPT && argc > 1)
> + return !!sequencer_make_script(keep_empty, stdout, argc, argv);

same here.

>   usage_with_options(builtin_rebase_helper_usage, options);
>  }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 2c9c0165b5a..609e150d38f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -785,6 +785,7 @@ collapse_todo_ids() {
>  # each log message will be re-retrieved in order to normalize the
>  # autosquash arrangement
>  rearrange_squash () {
> + format=$(git config --get rebase.instructionFormat)
>   # extract fixup!/squash! lines and resolve any referenced sha1's
>   while read -r pick sha1 message
>   do
> @@ -1210,26 +1211,27 @@ else
>   revisions=$onto...$orig_head
>   shortrevisions=$shorthead
>  fi
> -format=$(git config --get rebase.instructionFormat)
> -# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H 
> to parse
> -git rev-list $merges_option --format="%m%H ${format:-%s}" \
> - --reverse --left-right --topo-order \
> - $revisions ${restrict_revision+^$restrict_revision} | \
> - sed -n "s/^>//p" |
> -while read -r sha1 rest
> -do
> -
> - if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit 
> $sha1
> - then
> - comment_out="$comment_char "
> - else
> - comment_out=
> - fi
> +if test t != "$preserve_merges"
> +then
> + git rebase--helper --make-script ${keep_empty:+--keep-empty} \
> + $revisions ${restrict_revision+^$restrict_revision} >"$todo"
> +else
> + format=$(git config --get rebase.instructionFormat)
> + # the 'rev-list .. | 

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[liam Beguin]

Hi Johannes,

On 29/05/17 06:59 AM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Thu, 25 May 2017, Liam Beguin wrote:
> 
>> Johannes Schindelin <johannes.schinde...@gmx.de> writes:
>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 130cc868e51..88819a1a2a9 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int 
>>> ignore_footer, unsigned flag)
>>>  
>>> strbuf_release();
>>>  }
>>> +
>>> +int sequencer_make_script(int keep_empty, FILE *out,
>>> +   int argc, const char **argv)
>>> +{
>>> +   char *format = NULL;
>>> +   struct pretty_print_context pp = {0};
>>> +   struct strbuf buf = STRBUF_INIT;
>>> +   struct rev_info revs;
>>> +   struct commit *commit;
>>> +
>>> +   init_revisions(, NULL);
>>> +   revs.verbose_header = 1;
>>> +   revs.max_parents = 1;
>>> +   revs.cherry_pick = 1;
>>> +   revs.limited = 1;
>>> +   revs.reverse = 1;
>>> +   revs.right_only = 1;
>>> +   revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
>>> +   revs.topo_order = 1;
>>> +
>>> +   revs.pretty_given = 1;
>>> +   git_config_get_string("rebase.instructionFormat", );
>>> +   if (!format || !*format) {
>>> +   free(format);
>>> +   format = xstrdup("%s");
>>> +   }
>>> +   get_commit_format(format, );
>>> +   free(format);
>>> +   pp.fmt = revs.commit_format;
>>> +   pp.output_encoding = get_log_output_encoding();
>>> +
>>> +   if (setup_revisions(argc, argv, , NULL) > 1)
>>> +   return error(_("make_script: unhandled options"));
>>> +
>>> +   if (prepare_revision_walk() < 0)
>>> +   return error(_("make_script: error preparing revisions"));
>>> +
>>> +   while ((commit = get_revision())) {
>>> +   strbuf_reset();
>>> +   if (!keep_empty && is_original_commit_empty(commit))
>>> +   strbuf_addf(, "%c ", comment_line_char);
>>
>> I've never had to use empty commits before, but while testing this, I
>> noticed that `git rebase -i --keep-empty` behaves differently if using
>> the --root option instead of a branch or something like 'HEAD~10'.
>> I also tested this on v2.13.0 and the behavior is the same.
> 
> FWIW the terminology "empty commit" is a pretty poor naming choice. This
> is totally not your fault at all. I just wish we had a much more intuitive
> name to describe a commit that does not introduce any changes to the tree.
> 
> And yes, doing this with --root is a bit of a hack. That's because --root
> is a bit of a hack.
> 
> I am curious, though, as to the exact differences you experienced. I mean,
> it could be buggy behavior that needs to be fixed (independently of this
> patch series, of course).
> 

Sorry, reading this I realized that I didn't give any details!
When using --root, --keep-empty has no effect. The empty commits
do not appear in the todo list and they are removed.
Also, when using --root without --keep-empty, the empty commits
do not show up as comments in the list.

> Ciao,
> Johannes
> 

Liam 

Re: [PATCH v4 03/10] rebase -i: remove useless indentation

[liam Beguin]

Hi Stefan,

On 26/05/17 01:50 PM, Stefan Beller wrote:
> On Thu, May 25, 2017 at 8:15 PM, Liam Beguin <liambeg...@gmail.com> wrote:
>> Hi Johannes,
>>
>> Johannes Schindelin <johannes.schinde...@gmx.de> writes:
>>> The commands used to be indented, and it is nice to look at, but when we
>>> transform the SHA-1s, the indentation is removed. So let's do away with it.
>>>
>>> For the moment, at least: when we will use the upcoming rebase--helper
>>> to transform the SHA-1s, we *will* keep the indentation and can
>>> reintroduce it. Yet, to be able to validate the rebase--helper against
>>> the output of the current shell script version, we need to remove the
>>> extra indentation.
>>>
>>> Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
>>> ---
>>>  git-rebase--interactive.sh | 14 +++---
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>>> index 609e150d38f..c40b1fd1d2e 100644
>>> --- a/git-rebase--interactive.sh
>>> +++ b/git-rebase--interactive.sh
>>> @@ -155,13 +155,13 @@ reschedule_last_action () {
>>>  append_todo_help () {
>>>   gettext "
>>>  Commands:
>>> - p, pick = use commit
>>> - r, reword = use commit, but edit the commit message
>>> - e, edit = use commit, but stop for amending
>>> - s, squash = use commit, but meld into previous commit
>>> - f, fixup = like \"squash\", but discard this commit's log message
>>> - x, exec = run command (the rest of the line) using shell
>>> - d, drop = remove commit
>>> +p, pick = use commit
>>> +r, reword = use commit, but edit the commit message
>>> +e, edit = use commit, but stop for amending
>>> +s, squash = use commit, but meld into previous commit
>>> +f, fixup = like \"squash\", but discard this commit's log message
>>> +x, exec = run command (the rest of the line) using shell
>>> +d, drop = remove commit
>>
>> do we also need to update all the translations since this is a `gettext`
>> function?
> 
> Translations are handled separately, later before a release is done.
> Separation of skills. ;)
> 
> As programming is quite complicated and involved we'd rather ask
> Johannes to only focus on the code in such a patch here and then later
> the translators will focus on getting the translation right. As the 
> translation
> tools are sophisticated, they will likely give the previous translation such
> that the translators see that there is only a white space change.

Thanks for the clarification, I was wondering how this part was handled.

> 
> But as the commit message hints at a later patch that will reintroduce the
> original indentation, maybe the translators won't even see that change?
> 
> For more information on how the translations work in the git workflow,
> see 951ea7656e (Merge tag 'l10n-2.13.0-rnd2.1' of
> git://github.com/git-l10n/git-po, 2017-05-09) or see
> https://public-inbox.org/git/canyiybgfdxj4jjtcd3ppxqsdn-twcc8dm8b9ov_3naszwsr...@mail.gmail.com/
> 

Liam 

[PATCH v4 10/10] rebase -i: rearrange fixup/squash lines using the rebase--helper

[Liam Beguin]

Hi Johannes,

Johannes Schindelin  writes:
> This operation has quadratic complexity, which is especially painful
> on Windows, where shell scripts are *already* slow (mainly due to the
> overhead of the POSIX emulation layer).
>
> Let's reimplement this with linear complexity (using a hash map to
> match the commits' subject lines) for the common case; Sadly, the
> fixup/squash feature's design neglected performance considerations,
> allowing arbitrary prefixes (read: `fixup! hell` will match the
> commit subject `hello world`), which means that we are stuck with
> quadratic performance in the worst case.
>
> The reimplemented logic also happens to fix a bug where commented-out
> lines (representing empty patches) were dropped by the previous code.
>
> While at it, clarify how the fixup/squash feature works in `git rebase
> -i`'s man page.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  Documentation/git-rebase.txt |  16 ++--
>  builtin/rebase--helper.c |   6 +-
>  git-rebase--interactive.sh   |  90 +---
>  sequencer.c  | 195 
> +++
>  sequencer.h  |   1 +
>  t/t3415-rebase-autosquash.sh |   2 +-
>  6 files changed, 212 insertions(+), 98 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 53f4e14..c5464aa5365 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -430,13 +430,15 @@ without an explicit `--interactive`.
>  --autosquash::
>  --no-autosquash::
>   When the commit log message begins with "squash! ..." (or
> - "fixup! ..."), and there is a commit whose title begins with
> - the same ..., automatically modify the todo list of rebase -i
> - so that the commit marked for squashing comes right after the
> - commit to be modified, and change the action of the moved
> - commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
> - "fixup! " or "squash! " after the first, in case you referred to an
> - earlier fixup/squash with `git commit --fixup/--squash`.
> + "fixup! ..."), and there is already a commit in the todo list that
> + matches the same `...`, automatically modify the todo list of rebase
> + -i so that the commit marked for squashing comes right after the
> + commit to be modified, and change the action of the moved commit
> + from `pick` to `squash` (or `fixup`).  A commit matches the `...` if
> + the commit subject matches, or if the `...` refers to the commit's
> + hash. As a fall-back, partial matches of the commit subject work,
> + too.  The recommended way to create fixup/squash commits is by using
> + the `--fixup`/`--squash` options of linkgit:git-commit[1].
>  +
>  This option is only valid when the `--interactive` option is used.
>  +
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index de3ccd9bfbc..e6591f01112 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   int keep_empty = 0;
>   enum {
>   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
> - CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS
> + CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
>   } command = 0;
>   struct option options[] = {
>   OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
> @@ -33,6 +33,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   N_("check the todo list"), CHECK_TODO_LIST),
>   OPT_CMDMODE(0, "skip-unnecessary-picks", ,
>   N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
> + OPT_CMDMODE(0, "rearrange-squash", ,
> + N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
>   OPT_END()
>   };
>  
> @@ -59,5 +61,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   return !!check_todo_list();
>   if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
>   return !!skip_unnecessary_picks();
> + if (command == REARRANGE_SQUASH && argc == 1)
> + return !!rearrange_squash();
>   usage_with_options(builtin_rebase_helper_usage, options);
>  }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 92e3ca1de3b..84c6e62518f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -721,94 +721,6 @@ collapse_todo_ids() {
>   git rebase--helper --shorten-sha1s
>  }
>  
> -# Rearrange the todo list that has both "pick sha1 msg" and
> -# "pick sha1 fixup!/squash! msg" appears in it so that the latter
> -# comes immediately after the former, and change "pick" to
> -# "fixup"/"squash".
> -#
> -# Note that if the config has specified a 

[PATCH v4 03/10] rebase -i: remove useless indentation

[Liam Beguin]

Hi Johannes,

Johannes Schindelin  writes:
> The commands used to be indented, and it is nice to look at, but when we
> transform the SHA-1s, the indentation is removed. So let's do away with it.
> 
> For the moment, at least: when we will use the upcoming rebase--helper
> to transform the SHA-1s, we *will* keep the indentation and can
> reintroduce it. Yet, to be able to validate the rebase--helper against
> the output of the current shell script version, we need to remove the
> extra indentation.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  git-rebase--interactive.sh | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 609e150d38f..c40b1fd1d2e 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -155,13 +155,13 @@ reschedule_last_action () {
>  append_todo_help () {
>   gettext "
>  Commands:
> - p, pick = use commit
> - r, reword = use commit, but edit the commit message
> - e, edit = use commit, but stop for amending
> - s, squash = use commit, but meld into previous commit
> - f, fixup = like \"squash\", but discard this commit's log message
> - x, exec = run command (the rest of the line) using shell
> - d, drop = remove commit
> +p, pick = use commit
> +r, reword = use commit, but edit the commit message
> +e, edit = use commit, but stop for amending
> +s, squash = use commit, but meld into previous commit
> +f, fixup = like \"squash\", but discard this commit's log message
> +x, exec = run command (the rest of the line) using shell
> +d, drop = remove commit

do we also need to update all the translations since this is a `gettext`
function?

>  
>  These lines can be re-ordered; they are executed from top to bottom.
>  " | git stripspace --comment-lines >>"$todo"
> -- 
> 2.12.2.windows.2.800.gede8f145e06

Liam

[PATCH v4 05/10] rebase -i: also expand/collapse the SHA-1s via the rebase--helper

[Liam Beguin]

Hi Johannes,

Johannes Schindelin  writes:
> This is crucial to improve performance on Windows, as the speed is now
> mostly dominated by the SHA-1 transformation (because it spawns a new
> rev-parse process for *every* line, and spawning processes is pretty
> slow from Git for Windows' MSYS2 Bash).
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/rebase--helper.c   | 10 +++-
>  git-rebase--interactive.sh | 27 ++
>  sequencer.c| 57 
> ++
>  sequencer.h|  2 ++
>  4 files changed, 70 insertions(+), 26 deletions(-)
> 
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index 821058d452d..9444c8d6c60 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -13,7 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   struct replay_opts opts = REPLAY_OPTS_INIT;
>   int keep_empty = 0;
>   enum {
> - CONTINUE = 1, ABORT, MAKE_SCRIPT
> + CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S
>   } command = 0;
>   struct option options[] = {
>   OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
> @@ -24,6 +24,10 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   ABORT),
>   OPT_CMDMODE(0, "make-script", ,
>   N_("make rebase script"), MAKE_SCRIPT),
> + OPT_CMDMODE(0, "shorten-sha1s", ,
> + N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
> + OPT_CMDMODE(0, "expand-sha1s", ,
> + N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),

Since work is being done to convert to `struct object_id` would it
not be best to use a more generic name instead of 'sha1'?
maybe something like {shorten,expand}-hashs

>   OPT_END()
>   };
>  
> @@ -42,5 +46,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   return !!sequencer_remove_state();
>   if (command == MAKE_SCRIPT && argc > 1)
>   return !!sequencer_make_script(keep_empty, stdout, argc, argv);
> + if (command == SHORTEN_SHA1S && argc == 1)
> + return !!transform_todo_ids(1);
> + if (command == EXPAND_SHA1S && argc == 1)
> + return !!transform_todo_ids(0);
>   usage_with_options(builtin_rebase_helper_usage, options);
>  }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 214af0372ba..82a1941c42c 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -750,35 +750,12 @@ skip_unnecessary_picks () {
>   die "$(gettext "Could not skip unnecessary pick commands")"
>  }
>  
> -transform_todo_ids () {
> - while read -r command rest
> - do
> - case "$command" in
> - "$comment_char"* | exec)
> - # Be careful for oddball commands like 'exec'
> - # that do not have a SHA-1 at the beginning of $rest.
> - ;;
> - *)
> - sha1=$(git rev-parse --verify --quiet "$@" ${rest%%[
>  ]*}) &&
> - if test "a$rest" = "a${rest#*[   ]}"
> - then
> - rest=$sha1
> - else
> - rest="$sha1 ${rest#*[]}"
> - fi
> - ;;
> - esac
> - printf '%s\n' "$command${rest:+ }$rest"
> - done <"$todo" >"$todo.new" &&
> - mv -f "$todo.new" "$todo"
> -}
> -
>  expand_todo_ids() {
> - transform_todo_ids
> + git rebase--helper --expand-sha1s
>  }
>  
>  collapse_todo_ids() {
> - transform_todo_ids --short
> + git rebase--helper --shorten-sha1s
>  }
>  
>  # Rearrange the todo list that has both "pick sha1 msg" and
> diff --git a/sequencer.c b/sequencer.c
> index 88819a1a2a9..201d45b1677 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2437,3 +2437,60 @@ int sequencer_make_script(int keep_empty, FILE *out,
>   strbuf_release();
>   return 0;
>  }
> +
> +
> +int transform_todo_ids(int shorten_sha1s)
> +{
> + const char *todo_file = rebase_path_todo();
> + struct todo_list todo_list = TODO_LIST_INIT;
> + int fd, res, i;
> + FILE *out;
> +
> + strbuf_reset(_list.buf);
> + fd = open(todo_file, O_RDONLY);
> + if (fd < 0)
> + return error_errno(_("could not open '%s'"), todo_file);
> + if (strbuf_read(_list.buf, fd, 0) < 0) {
> + close(fd);
> + return error(_("could not read '%s'."), todo_file);
> + }
> + close(fd);
> +
> + res = parse_insn_buffer(todo_list.buf.buf, _list);
> + if (res) {
> + todo_list_release(_list);
> + return error(_("unusable instruction 

[PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Liam Beguin]

Hi Johannes,

Johannes Schindelin  writes:
> The first step of an interactive rebase is to generate the so-called "todo
> script", to be stored in the state directory as "git-rebase-todo" and to
> be edited by the user.
> 
> Originally, we adjusted the output of `git log ` using a simple
> sed script. Over the course of the years, the code became more
> complicated. We now use shell scripting to edit the output of `git log`
> conditionally, depending whether to keep "empty" commits (i.e. commits
> that do not change any files).
> 
> On platforms where shell scripting is not native, this can be a serious
> drag. And it opens the door for incompatibilities between platforms when
> it comes to shell scripting or to Unix-y commands.
> 
> Let's just re-implement the todo script generation in plain C, using the
> revision machinery directly.
> 
> This is substantially faster, improving the speed relative to the
> shell script version of the interactive rebase from 2x to 3x on Windows.
> 
> Note that the rearrange_squash() function in git-rebase--interactive
> relied on the fact that we set the "format" variable to the config setting
> rebase.instructionFormat. Relying on a side effect like this is no good,
> hence we explicitly perform that assignment (possibly again) in
> rearrange_squash().
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/rebase--helper.c   |  8 +++-
>  git-rebase--interactive.sh | 44 +
>  sequencer.c| 49 
> ++
>  sequencer.h|  3 +++
>  4 files changed, 82 insertions(+), 22 deletions(-)
> 
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index ca1ebb2fa18..821058d452d 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -11,15 +11,19 @@ static const char * const builtin_rebase_helper_usage[] = 
> {
>  int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  {
>   struct replay_opts opts = REPLAY_OPTS_INIT;
> + int keep_empty = 0;
>   enum {
> - CONTINUE = 1, ABORT
> + CONTINUE = 1, ABORT, MAKE_SCRIPT
>   } command = 0;
>   struct option options[] = {
>   OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
> + OPT_BOOL(0, "keep-empty", _empty, N_("keep empty 
> commits")),
>   OPT_CMDMODE(0, "continue", , N_("continue rebase"),
>   CONTINUE),
>   OPT_CMDMODE(0, "abort", , N_("abort rebase"),
>   ABORT),
> + OPT_CMDMODE(0, "make-script", ,
> + N_("make rebase script"), MAKE_SCRIPT),
>   OPT_END()
>   };
>  
> @@ -36,5 +40,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   return !!sequencer_continue();
>   if (command == ABORT && argc == 1)
>   return !!sequencer_remove_state();
> + if (command == MAKE_SCRIPT && argc > 1)
> + return !!sequencer_make_script(keep_empty, stdout, argc, argv);
>   usage_with_options(builtin_rebase_helper_usage, options);
>  }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 2c9c0165b5a..609e150d38f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -785,6 +785,7 @@ collapse_todo_ids() {
>  # each log message will be re-retrieved in order to normalize the
>  # autosquash arrangement
>  rearrange_squash () {
> + format=$(git config --get rebase.instructionFormat)
>   # extract fixup!/squash! lines and resolve any referenced sha1's
>   while read -r pick sha1 message
>   do
> @@ -1210,26 +1211,27 @@ else
>   revisions=$onto...$orig_head
>   shortrevisions=$shorthead
>  fi
> -format=$(git config --get rebase.instructionFormat)
> -# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H 
> to parse
> -git rev-list $merges_option --format="%m%H ${format:-%s}" \
> - --reverse --left-right --topo-order \
> - $revisions ${restrict_revision+^$restrict_revision} | \
> - sed -n "s/^>//p" |
> -while read -r sha1 rest
> -do
> -
> - if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit 
> $sha1
> - then
> - comment_out="$comment_char "
> - else
> - comment_out=
> - fi
> +if test t != "$preserve_merges"
> +then
> + git rebase--helper --make-script ${keep_empty:+--keep-empty} \
> + $revisions ${restrict_revision+^$restrict_revision} >"$todo"
> +else
> + format=$(git config --get rebase.instructionFormat)
> + # the 'rev-list .. | sed' requires %m to parse; the instruction 
> requires %H to parse
> + git rev-list $merges_option --format="%m%H ${format:-%s}" \
> + --reverse --left-right --topo-order \
> + $revisions 

[PATCH v4 00/10] The final building block for a faster rebase -i

[Liam Beguin]

Hi Johannes,

Johannes Schindelin  writes:
> This patch series reimplements the expensive pre- and post-processing of
> the todo script in C.
>
> And it concludes the work I did to accelerate rebase -i.
>
> Changes since v3:
>
> - removed the no-longer-used transform_todo_ids shell function
>
> - simplified transform_todo_ids()'s command parsing
>
> - fixed two commits in check_todo_list(), renamed the unclear
>   `raise_error` variable to `advise_to_edit_todo`, build the message
>   about missing commits directly (without the detour to building a
>   commit_list) and instead of assigning an unused pointer to commit->util
>   the code now uses (void *)1.
>
> - return early from check_todo_list() when parsing failed, even if the
>   check level is something else than CHECK_IGNORE
>
> - the todo list is generated is again generated in the same way as
>   before when rebase.instructionFormat is empty: it was interpreted as
>   if it had not been set
>
> - added a test for empty rebase.instructionFormat settings
>
>
> Johannes Schindelin (10):
>   t3415: verify that an empty instructionFormat is handled as before
>   rebase -i: generate the script via rebase--helper
>   rebase -i: remove useless indentation
>   rebase -i: do not invent onelines when expanding/collapsing SHA-1s
>   rebase -i: also expand/collapse the SHA-1s via the rebase--helper
>   t3404: relax rebase.missingCommitsCheck tests
>   rebase -i: check for missing commits in the rebase--helper
>   rebase -i: skip unnecessary picks using the rebase--helper
>   t3415: test fixup with wrapped oneline
>   rebase -i: rearrange fixup/squash lines using the rebase--helper
>
>  Documentation/git-rebase.txt  |  16 +-
>  builtin/rebase--helper.c  |  29 ++-
>  git-rebase--interactive.sh| 373 -
>  sequencer.c   | 530 
> ++
>  sequencer.h   |   8 +
>  t/t3404-rebase-interactive.sh |  22 +-
>  t/t3415-rebase-autosquash.sh  |  28 ++-
>  7 files changed, 646 insertions(+), 360 deletions(-)
>
>
> base-commit: 027a3b943b444a3e3a76f9a89803fc10245b858f
> Based-On: rebase--helper at https://github.com/dscho/git
> Fetch-Base-Via: git fetch https://github.com/dscho/git rebase--helper
> Published-As: https://github.com/dscho/git/releases/tag/rebase-i-extra-v4
> Fetch-It-Via: git fetch https://github.com/dscho/git rebase-i-extra-v4
>

This is my first review so it's probably not the best you'll get, but
here it goes!

I rebased the series ontop of v2.13.0 and run the whole `make test` on
both revisions.
The changes do not seem to have introduced any evident breakage as the
output of `make test` did not change.

I tried to time the execution on an interactive rebase (on Linux) but
I did not notice a significant change in speed.
Do we have a way to measure performance / speed changes between version?

Liam

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Liam Beguin]

Hi,

On Mon, 2017-05-08 at 09:27 +0900, Junio C Hamano wrote:
> Liam Beguin <liambeg...@gmail.com> writes:
> 
> > Sorry for the delay, I don't mind switching to C but it would probably
> > be easier to see if the scripted version gets approved first.
> > If it does, I could then get started on the C implementation.
> > If you prefer I could also forget about the scripted version, make a C
> > implementation work and see if that gets approved.
> 
> I am not sure what "approved" would mean in the context of this
> project, though ;-) Your patch to the scripted version would
> certainly not be in the upcoming release.  If you define the
> "approval" as "it is queued to my tree somewhere", the patch would
> start its life like everybody else by getting merged to the 'pu'
> branch, where there already is a topic that removes the code you
> patch your enhancement into.
> 

By "approved", I guess I meant the list reaches an agreement. 

> The list _can_ agree that it is a good idea to have an option to
> populate the todo list with shortened insn words from the beginning
> (instead of merely accepting a short-hand while executing), which is
> what your patch wants to do, without actually having the updated
> scripted "rebase -i" merged in any of the integration branches in my
> tree.  If you meant by "approval" to have such a list concensus, I
> think you may already have one.  I personally do not think it is a
> great idea but I do not think it is a horrible one, either.  As long
> as it is an opt-in feature that many people find useful (which may
> be the case already, judging from the list traffic), I do not mind
> ;-)
> 

Ok, based on this, I'll send a new series based on the 'pu' branch.

Thanks again, 
Liam

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Liam Beguin]

Hi Junio,

On Wed, 2017-05-03 at 22:04 -0700, Junio C Hamano wrote:
> Johannes Schindelin  writes:
> 
> > > If 'git-rebase--interactive.sh' is bound to be replaced, I could
> > > just shrink this to the Documentation cleanup (patches 4 and 5)
> > > and rework the rest on top of your new implementation.
> > 
> > I kind of hoped that Junio would chime in with his verdict. That would be
> > the ultimate deciding factor, I think.
> 
> What I can predict is that within two or three release cycles
> (unless you completely lose interest) the todo-list generation will
> be all in C and that I anticipate that the C version may even be
> capable of generating different kind of todo command (e.g. to
> support tools like your Garden Shears more natively), so the
> mid-term direction definitely is that any enhancement would in the
> end needs to happen on top of or in coordination with the C rewrite
> we've been discussing recently.
> 
> I didn't know what the comfort levels of Liam working with scripted
> vs C code, and the "vertict" depends on that, I would think.  We may
> want to discuss the enhancement in the original scripted form Liam
> did with new tests while the C rewrite is still cooking, and either
> Liam, you or somebody else can make it work with your C rewrite when
> both are ready.  If Liam feels comfortable working with you and the
> code after the C rewrite that is still in-flight, it is also fine if
> the next iteration from Liam were on top of your series.
> 
> 

Sorry for the delay, I don't mind switching to C but it would probably
be easier to see if the scripted version gets approved first.
If it does, I could then get started on the C implementation.
If you prefer I could also forget about the scripted version, make a C
implementation work and see if that gets approved.

Thanks,
Liam

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Liam Beguin]

Hi Johannes, 

On Tue, 2017-05-02 at 17:48 +0200, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Tue, 2 May 2017, Liam Beguin wrote:
> 
> > Add the 'rebase.abbreviateCommands' configuration option to allow `git
> > rebase -i` to default to the single-letter command-names in the todo
> > list.
> > 
> > Using single-letter command-names can present two benefits.  First, it
> > makes it easier to change the action since you only need to replace a
> > single character (i.e.: in vim "r" instead of
> > "ciw").  Second, using this with a large enough value of
> > 'core.abbrev' enables the lines of the todo list to remain aligned
> > making the files easier to read.
> > 
> > Changes from v1 to v2:
> >  - Improve Documentation and commit message
> > 
> > Changes from v2 to v3:
> >  - Transform a single patch into a series
> >  - change option name from 'rebase.abbrevCmd' to 'rebase.abbreviateCommands'
> >  - abbreviate all commands (not just pick)
> >  - teach `git rebase -i --autosquash` to recognise single-letter 
> > command-names
> >  - move rebase configuration documentation to 
> > Documentation/rebase-config.txt
> >  - update Documentation to use the preferred naming for the todo list
> >  - update Documentation and commit messages according to feedback
> 
> Thank you for this pleasant read. It is an excellent contribution, and the
> way you communicate what you changed and why is very welcome.
> 

Thanks! and thank you for the support and help.

> I offered a couple of comments, my biggest one probably being that this
> patch series is crossing paths with my patch series that tries to move
> more functionality out of the git-rebase--interactive.sh script into the
> git-rebase--helper that is written in C, closely followed by my suggestion
> to fold at least part of the functionality into the SHA-1
> collapsing/expanding.
> 

I've seen a few messages about this migration, but I'm not yet sure I understand
the difference between the shell and the C implementations. Is the C version 
going
to replace 'git-rebase--interactive.sh'?

> If your patch series "wins", I can easily forward-port your changes to the
> rebase-i-extra branch, but it may actually make sense to build on top of
> the rebase-i-extra branch to begin with. If you agree: I pushed the
> proposed change to the `rebase-i-extra+abbrev` branch at
> https://github.com/dscho/git.
> 

If 'git-rebase--interactive.sh' is bound to be replaced, I could
just shrink this to the Documentation cleanup (patches 4 and 5)
and rework the rest on top of your new implementation.

> I look forward to see this story unfold!
> 
> Ciao,
> Johannes

Thanks, 
Liam

Re: [PATCH v3 3/6] rebase -i: add short command-name in --autosquash

[Liam Beguin]

Hi Johannes,

On Tue, 2017-05-02 at 17:34 +0200, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Tue, 2 May 2017, Liam Beguin wrote:
> 
> > teach `git rebase -i` to recognise short command-names when using the
> > '--autosquash' option. This allows commit with titles beginning with
> > "s! ..." and "f! ..." to be treated the same way as "squash! ..." and
> > "fixup! ..." respectively.
> 
> As the recommended way to generate those commits is by using the
> --fixup/--squash options of git-commit, and as there is *a much higher*
> chance of false positives when using a very short tell-tale such as `f!`
> (which could be an abbreviation for an expletive, likewise `s!`), I do not
> think we will want this change.
> 
> Let's keep handling just fixup!/squash!
> 
> Ciao,
> Johannes

I was not quite sure about this change. My guess was that since --autosquash
needs the whole commit title to find a match, the short version had little
probability of generating a false positive. I thought it made sense to include
the change in this series, but I understand why it's probably not a good idea
to take it. I'll remove it in the next series.

Thanks, 
Liam

[PATCH v3 3/6] rebase -i: add short command-name in --autosquash

[Liam Beguin]

teach `git rebase -i` to recognise short command-names when using the
'--autosquash' option. This allows commit with titles beginning with
"s! ..." and "f! ..." to be treated the same way as "squash! ..." and
"fixup! ..." respectively.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/git-rebase.txt | 2 ++
 git-rebase--interactive.sh   | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 53f4e14a..3e49d8b046ca 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -437,6 +437,8 @@ without an explicit `--interactive`.
commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
"fixup! " or "squash! " after the first, in case you referred to an
earlier fixup/squash with `git commit --fixup/--squash`.
+   Note that their short counterparts, namely "s! ..." and "f! ..."
+   behave the same way.
 +
 This option is only valid when the `--interactive` option is used.
 +
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4fa621062cdf..61450064c5c4 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -790,7 +790,7 @@ rearrange_squash () {
do
test -z "${format}" || message=$(git log -n 1 --format="%s" 
${sha1})
case "$message" in
-   "squash! "*|"fixup! "*)
+   "squash! "*|"s! "*|"fixup! "*|"f! "*)
action="${message%%!*}"
rest=$message
prefix=
@@ -798,7 +798,7 @@ rearrange_squash () {
while :
do
case "$rest" in
-   "squash! "*|"fixup! "*)
+   "squash! "*|"s! "*|"fixup! "*|"f! "*)
prefix="$prefix${rest%%!*},"
rest="${rest#*! }"
;;
-- 
2.9.3

[PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Liam Beguin]

Add the 'rebase.abbreviateCommands' configuration option to allow
`git rebase -i` to default to the single-letter command-names in
the todo list.

Using single-letter command-names can present two benefits.
First, it makes it easier to change the action since you only need to
replace a single character (i.e.: in vim "r" instead of
"ciw").
Second, using this with a large enough value of 'core.abbrev' enables the
lines of the todo list to remain aligned making the files easier to
read.

Changes from v1 to v2:
 - Improve Documentation and commit message

Changes from v2 to v3:
 - Transform a single patch into a series
 - change option name from 'rebase.abbrevCmd' to 'rebase.abbreviateCommands'
 - abbreviate all commands (not just pick)
 - teach `git rebase -i --autosquash` to recognise single-letter command-names
 - move rebase configuration documentation to Documentation/rebase-config.txt
 - update Documentation to use the preferred naming for the todo list
 - update Documentation and commit messages according to feedback

Liam Beguin (6):
  rebase -i: add abbreviated command-names handling
  rebase -i: add abbreviate_commands function
  rebase -i: add short command-name in --autosquash
  Documentation: move rebase.* config variables to a separate
rebase-config.txt
  Documentation: use prefered name for the 'todo list' script
  Documentation: document the rebase.abbreviateCommands option

 Documentation/config.txt| 31 +---
 Documentation/git-rebase.txt| 21 +++-
 Documentation/rebase-config.txt | 53 +
 git-rebase--interactive.sh  | 24 
 4 files changed, 78 insertions(+), 52 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

-- 
2.9.3

[PATCH v3 1/6] rebase -i: add abbreviated command-names handling

[Liam Beguin]

make sure 'add_exec_commands' and 'transform_todo_ids' also understand
the abbreviated versions of the command-names.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 git-rebase--interactive.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2c9c0165b5ab..9b8a030ff045 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -754,7 +754,7 @@ transform_todo_ids () {
while read -r command rest
do
case "$command" in
-   "$comment_char"* | exec)
+   "$comment_char"* |x|exec)
# Be careful for oddball commands like 'exec'
# that do not have a SHA-1 at the beginning of $rest.
;;
@@ -871,7 +871,7 @@ add_exec_commands () {
while read -r insn rest
do
case $insn in
-   pick)
+   p|pick)
test -n "$first" ||
printf "%s" "$cmd"
;;
-- 
2.9.3

[PATCH v3 2/6] rebase -i: add abbreviate_commands function

[Liam Beguin]

Once the rest of the processing is done, the `abbreviate_commands`
function is called. If the 'rebase.abbreviateCommands' option is set to
true, the function will replace each command-name by its abbreviated
form.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 git-rebase--interactive.sh | 16 
 1 file changed, 17 insertions(+)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9b8a030ff045..4fa621062cdf 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -884,6 +884,20 @@ add_exec_commands () {
mv "$1.new" "$1"
 }
 
+abbreviate_commands () {
+   test "$(git config --bool rebase.abbreviateCommands)" = true || return
+
+   while read -r command rest
+   do
+   case $command in
+   x|exec) command=x ;;
+   *)  command=${command%${command#?}} ;;
+   esac
+   printf "%s\n" "$command $rest"
+   done <"$1" >"$1.new" &&
+   mv -f "$1.new" "$1"
+}
+
 # Check if the SHA-1 passed as an argument is a
 # correct one, if not then print $2 in "$todo".badsha
 # $1: the SHA-1 to test
@@ -1143,6 +1158,7 @@ edit-todo)
git stripspace --strip-comments <"$todo" >"$todo".new
mv -f "$todo".new "$todo"
collapse_todo_ids
+   abbreviate_commands "$todo"
append_todo_help
gettext "
 You are editing the todo file of an ongoing interactive rebase.
@@ -1281,6 +1297,7 @@ fi
 test -s "$todo" || echo noop >> "$todo"
 test -n "$autosquash" && rearrange_squash "$todo"
 test -n "$cmd" && add_exec_commands "$todo"
+abbreviate_commands "$todo"
 
 todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
 todocount=${todocount##* }
-- 
2.9.3

[PATCH v3 4/6] Documentation: move rebase.* config variables to a separate rebase-config.txt

[Liam Beguin]

Move configuration variables to a separate file in order to remove
duplicates, and include it in config.txt and git-rebase.txt.
The new descriptions are taken from config.txt as they are more verbose.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/config.txt| 31 +--
 Documentation/git-rebase.txt| 19 +--
 Documentation/rebase-config.txt | 30 ++
 3 files changed, 32 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5155..6b647c504e8f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2583,36 +2583,7 @@ push.recurseSubmodules::
is retained. You may override this configuration at time of push by
specifying '--recurse-submodules=check|on-demand|no'.
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   When set to true, automatically create a temporary stash
-   before the operation begins, and apply it after the operation
-   ends.  This means that you can run rebase on a dirty worktree.
-   However, use with care: the final stash application after a
-   successful rebase might result in non-trivial conflicts.
-   Defaults to false.
-
-rebase.missingCommitsCheck::
-   If set to "warn", git rebase -i will print a warning if some
-   commits are removed (e.g. a line was deleted), however the
-   rebase will still proceed. If set to "error", it will print
-   the previous warning and stop the rebase, 'git rebase
-   --edit-todo' can then be used to correct the error. If set to
-   "ignore", no checking is done.
-   To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
-   Defaults to "ignore".
-
-rebase.instructionFormat::
-   A format string, as specified in linkgit:git-log[1], to be used for
-   the instruction list during an interactive rebase.  The format will 
automatically
-   have the long commit hash prepended to the format.
+include::rebase-config.txt[]
 
 receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3e49d8b046ca..702a46adfa18 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -203,24 +203,7 @@ Alternatively, you can undo the 'git rebase' with
 CONFIGURATION
 -
 
-rebase.stat::
-   Whether to show a diffstat of what changed upstream since the last
-   rebase. False by default.
-
-rebase.autoSquash::
-   If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-   If set to true enable `--autostash` option by default.
-
-rebase.missingCommitsCheck::
-   If set to "warn", print warnings about removed commits in
-   interactive mode. If set to "error", print the warnings and
-   stop the rebase. If set to "ignore", no checking is
-   done. "ignore" by default.
-
-rebase.instructionFormat::
-   Custom commit list format to use during an `--interactive` rebase.
+include::rebase-config.txt[]
 
 OPTIONS
 ---
diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
new file mode 100644
index ..71872131
--- /dev/null
+++ b/Documentation/rebase-config.txt
@@ -0,0 +1,30 @@
+rebase.stat::
+   Whether to show a diffstat of what changed upstream since the last
+   rebase. False by default.
+
+rebase.autoSquash::
+   If set to true enable `--autosquash` option by default.
+
+rebase.autoStash::
+   When set to true, automatically create a temporary stash
+   before the operation begins, and apply it after the operation
+   ends.  This means that you can run rebase on a dirty worktree.
+   However, use with care: the final stash application after a
+   successful rebase might result in non-trivial conflicts.
+   Defaults to false.
+
+rebase.missingCommitsCheck::
+   If set to "warn", git rebase -i will print a warning if some
+   commits are removed (e.g. a line was deleted), however the
+   rebase will still proceed. If set to "error", it will print
+   the previous warning and stop the rebase, 'git rebase
+   --edit-todo' can then be used to correct the error. If set to
+   "ignore", no checking is done.
+   To drop a commit without warning or error, use the `drop`
+   command in the todo-list.
+   Defaults to "ignore".
+
+rebase.instructionFormat::
+   A format string, as specified in linkgit:git-log[1], to be used fo

[PATCH v3 5/6] Documentation: use preferred name for the 'todo list' script

[Liam Beguin]

Use "todo list" instead of "instruction list" or "todo-list" to
reduce further confusion regarding the name of this script.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/rebase-config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index 71872131..a9b1d496e63a 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -21,10 +21,10 @@ rebase.missingCommitsCheck::
--edit-todo' can then be used to correct the error. If set to
"ignore", no checking is done.
To drop a commit without warning or error, use the `drop`
-   command in the todo-list.
+   command in the todo list.
Defaults to "ignore".
 
 rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for
-   the instruction list during an interactive rebase.  The format will 
automatically
+   the todo list during an interactive rebase.  The format will 
automatically
have the long commit hash prepended to the format.
-- 
2.9.3

[PATCH v3 6/6] Documentation: document the rebase.abbreviateCommands option

[Liam Beguin]

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 Documentation/rebase-config.txt | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index a9b1d496e63a..0f29b7d0b89a 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -28,3 +28,26 @@ rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for
the todo list during an interactive rebase.  The format will 
automatically
have the long commit hash prepended to the format.
+
+rebase.abbreviateCommands::
+   If set to true, `git rebase -i` will abbreviate all command-names in the
+   todo list resulting in something like this:
+---
+   p ae6c43a first commit title
+   f 0694310 fixup! first commit title
+   p bf25ea8 second commit title
+   s e8fbbfd squash! second commit title
+   ...
+---
+   instead of:
+---
+   pick ae6c43a first commit title
+   fixup 0694310 fixup! first commit title
+   pick bf25ea8 second commit title
+   squash e8fbbfd squash! second commit title
+   ...
+---
+   As shown above, using single-letter command-names better aligns the
+   todo list when full names have different lengths. Additionally, combined
+   with a large enough value of 'core.abbrev' (say 12), the todo list is
+   guaranteed to be fully aligned.
-- 
2.9.3

Re: [PATCH v2] rebase -i: add config to abbreviate command-names

[liam Beguin]

Hi Ævar,

On Wed, 2017-04-26 at 17:24 +0200, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Apr 25, 2017 at 6:43 AM, Liam Beguin <liambeg...@gmail.com> wrote:
> > Add the 'rebase.abbrevCmd' boolean config option to allow `git rebase -i`
> > to abbreviate the command-names in the instruction list.
> > 
> > This means that `git rebase -i` would print:
> > p deadbee The oneline of this commit
> > ...
> > 
> > instead of:
> > pick deadbee The oneline of this commit
> > ...
> > 
> > Using a single character command-name allows the lines to remain
> > aligned, making the whole set more readable.
> 
> Aside from the existing comments about the commit message from others,
> you should be noting that we *already* have these abbreviations for
> all the todo list options, and we note this in append_todo_help.
> 
> 
> > +rebase.abbrevCmd::
> > +   If set to true, `git rebase -i` will abbreviate the command-names 
> > in the
> > +   instruction list. This means that instead of looking like this,
> > +
> > [...]
> > +rebase.abbrevCmd::
> > +   If set to true, `git rebase -i` will abbreviate the command-names 
> > in the
> > +   instruction list. This means that instead of looking like this,
> > [...]
> 
> Better to split this out into a new *.txt file and use the include::*
> facility (grep for it) rather than copy/pasting this entirely across
> two files.
> 

Thanks for pointing this out, I'll update the documentation

> >  OPTIONS
> >  ---
> >  --onto ::
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 2c9c0165b5ab..9f3e82b79615 100644
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -1210,6 +1210,10 @@ else
> > revisions=$onto...$orig_head
> > shortrevisions=$shorthead
> >  fi
> > +
> > +rebasecmd=pick
> > +test "$(git config --bool --get rebase.abbrevCmd)" = true && rebasecmd=p
> 
> Rather than hardcoding "p" here maybe it would be worthhwile to make
> that into a variable used both here and in append_todo_help, maybe
> not...
> 

I'm not sure I understand, do you mean that the option should also affect the
message added by append_todo_help ?

> >  format=$(git config --get rebase.instructionFormat)
> >  # the 'rev-list .. | sed' requires %m to parse; the instruction requires 
> > %H to parse
> >  git rev-list $merges_option --format="%m%H ${format:-%s}" \
> > @@ -1228,7 +1232,7 @@ do
> > 
> > if test t != "$preserve_merges"
> > then
> > -   printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
> > +   printf '%s\n' "${comment_out}${rebasecmd} $sha1 $rest" 
> > >>"$todo"
> > else
> > if test -z "$rebase_root"
> > then
> > @@ -1246,7 +1250,7 @@ do
> > if test f = "$preserve"
> > then
> > touch "$rewritten"/$sha1
> > -   printf '%s\n' "${comment_out}pick $sha1 $rest" 
> > >>"$todo"
> > +   printf '%s\n' "${comment_out}${rebasecmd} $sha1 
> > $rest" >>"$todo"
> > fi
> > fi
> >  done
> 
> I haven't tried applying & running this patch, but it seems you
> definitely missed the case where --autosquash will add fixup or
> squash, that should be f or s with your patch, but you didn't change
> that code. See the rearrange_squash function.
> 
> Ditto for turning "exec" into "e" with --exec.
> 

I noticed this yesterday, I'll add both cases the next iteration.

> But if the motivation for this entire thing is to make sure the
> commands are aligned this doesn't fix that, because the sha1s can be
> of different lengths. So as others have pointed out maybe this entire
> thing should be dropped & replaced with some bool command to align the
> todo list, maybe turning that on by default.
> 
> Unless the real unstated reason is to make this easier to edit in vim
> or something, in which case this approach seems reasonable.

Keeping things aligned was the first motivation but the fact that it also
makes changing the action faster is also nice to have. I didn't think it
would help justify the patch.
The SHA1s not having the same length can easily be 'fixed' by setting a
higher value for 'core.abbrev'. 

Thanks, 
Liam 

Re: [PATCH v2] rebase -i: add config to abbreviate command-names

[liam Beguin]

Hi Johannes, 

On Tue, 2017-04-25 at 22:08 +0200, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Tue, 25 Apr 2017, Liam Beguin wrote:
> 
> > Add the 'rebase.abbrevCmd' boolean config option to allow `git rebase -i`
> > to abbreviate the command-names in the instruction list.
> > 
> > This means that `git rebase -i` would print:
> > p deadbee The oneline of this commit
> > ...
> > 
> > instead of:
> > pick deadbee The oneline of this commit
> > ...
> > 
> > Using a single character command-name allows the lines to remain
> > aligned, making the whole set more readable.
> > 
> > Signed-off-by: Liam Beguin <liambeg...@gmail.com>
> 
> Apart from either abbreviating commands after --edit-todo, or documenting
> explicitly that the new config option only concerns the initial todo list,
> there is another problem that just occurred to me: --exec.
> 
> When you call `git rebase -x "make DEVELOPER=1 -j15"`, the idea is to
> append an "exec make DEVELOPER=1 -j15" line after every pick line. The
> code in question looks like this:
> 
> add_exec_commands () {
> {
> first=t
> while read -r insn rest
> do
> case $insn in
> pick)
> test -n "$first" ||
> printf "%s" "$cmd"
> ;;
> esac
> printf "%s %s\n" "$insn" "$rest"
> first=
> done
> printf "%s" "$cmd"
> } <"$1" >"$1.new" &&
> mv "$1.new" "$1"
> }
> 
> Obviously, the git-rebase--interactive script expects at this point that
> the command is spelled out, so your patch needs to change the `pick)` case
> to `p|pick)`, I think.
> 
> In addition, since the rationale for the new option is to align the lines
> better, the `exec` would need to be replaced by `x`, and as multiple `-x`
> options are allowed, you would need something like this at the beginning
> of `add_exec_commands`, too:
> 
>   # abbreviate `exec` if rebase.abbrevCmd is true
>   test p != "$rebasecmd" ||
>   cmd="$(echo "$cmd" | sed 's/^exec/x/')"
> 



> Also:
> 
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 475e874d5155..8b1877f2df91 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -2614,6 +2614,25 @@ rebase.instructionFormat::
> >     the instruction list during an interactive rebase.  The format will 
> > automatically
> >     have the long commit hash prepended to the format.
> >  
> > +rebase.abbrevCmd::
> 
> It does not fail to amuse that the term "abbrevCmd" is abbreviated
> heavily itself. However, I would strongly suggest to avoid that. It would
> be much more pleasant to call the config option rebase.abbreviateCommands

I tried to use something similar to the rest of the options but I guess that
would be best.

> 
> > +rebase.abbrevCmd::
> > +   If set to true, `git rebase -i` will abbreviate the command-names in the
> > +   instruction list. This means that instead of looking like this,
> 
> This is by no means your fault, but it is really horrible by how many
> different names Git's documentation refers to the todo script, nothing
> short of confusing. It is the todo script (which I called it initially,
> maybe not a good name, but it has the merit of the longest tradition at
> least), the todo list, the instruction sheet, the rebase script, the
> instruction list... etc
> 
> However, the thing is called "todo list" elsewhere in the same file,
> therefore lets try to avoid even more confusion and use that term instead
> of "instruction list" here.

thanks for pointing this out, I was not quite sure what to call this list.

> 
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 2c9c0165b5ab..9f3e82b79615 100644
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -1210,6 +1210,10 @@ else
> >     revisions=$onto...$orig_head
> >     shortrevisions=$shorthead
> >  fi
> > +
> > +rebasecmd=pick
> > +test "$(git config --bool --get rebase.abbrevCmd)" = true && rebasecmd=p
> 
> A better name would be "pickcmd", as there are more rebase commands than
> just `pick` and what we want here is really only associated with one of
> those commands.

Wouldn't that make it confusing when the patch starts to handle other commands?
A common name across the script would limit further confusion.
I noticed that it is already called `action` in `rearrange_squash`.
would that do? (even though it has no reference to 'command')

> 
> Ciao,
> Johannes

Thanks for the detailed answer,
Liam

Re: [PATCH v2] rebase -i: add config to abbreviate command-names

[liam Beguin]

Hi Jake, 

On Tue, 2017-04-25 at 01:29 -0700, Jacob Keller wrote:
> On Mon, Apr 24, 2017 at 11:29 PM, Junio C Hamano  wrote:
> > Personally I am happy with the beginning of each instruction line
> > aligned, so from that point of view, this patch is a mild Meh to me,
> > even though I do a fair amount of "rebase -i" myself.  But obviously
> > I am not the only user of Git you need to please, so...
> 
> I would instead justify this as making it easier to change the action,
> since you only need to rewrite a single letter, which at least in vim
> takes "r" to change the action, vs slightly more keystrokes
> such as "ct  
> Also, if you change the default commit hash length, it becomes long
> enough to cover most commits and you see all commits at say 12 digits
> commit hash and everything is nicely aligned.
> 
> Thanks,
> Jake

Thanks,
Liam 

Re: [PATCH] rebase -i: add config to abbreviate command name

[liam BEGUIN]

Hi Johannes,

On Tue, 2017-04-25 at 21:45 +0200, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Mon, 24 Apr 2017, liam BEGUIN wrote:
> 
> > On Mon, 2017-04-24 at 12:26 +0200, Johannes Schindelin wrote:
> > 
> > > On Sun, 23 Apr 2017, Liam Beguin wrote:
> > > 
> > > > Add the 'rebase.abbrevCmd' boolean config option to allow the user
> > > > to abbreviate the default command name while editing the
> > > > 'git-rebase-todo' file.
> > > 
> > > This patch does not handle the `git rebase --edit-todo` subcommand.
> > > Intentional?
> > 
> > After a little more investigation, I'm not sure what should be added for
> > the `git rebase --edit-todo` subcommand. It seems like it uses the same
> > text that was added the first time (with `git rebase -i`).
> 
> Well, it uses whatever the user may have edited. It may surprise users
> that their `pick` does not get converted to `p` like all the original
> commands.
> 

It makes more sens to me now, I'll add it in next patch

> Ciao,
> Johannes

Thanks, 
Liam

Re: [PATCH v2] rebase -i: add config to abbreviate command-names

[liam BEGUIN]

Hi Johannes,


On Tue, 2017-04-25 at 23:23 +0200, Johannes Schindelin wrote:
> Hi Andreas,
> 
> On Tue, 25 Apr 2017, Andreas Schwab wrote:
> 
> > On Apr 25 2017, Liam Beguin <liambeg...@gmail.com> wrote:
> > 
> > > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > > index 475e874d5155..8b1877f2df91 100644
> > > --- a/Documentation/config.txt
> > > +++ b/Documentation/config.txt
> > > @@ -2614,6 +2614,25 @@ rebase.instructionFormat::
> > >   the instruction list during an interactive rebase.  The format will 
> > > automatically
> > >   have the long commit hash prepended to the format.
> > >  
> > > +rebase.abbrevCmd::
> > > + If set to true, `git rebase -i` will abbreviate the command-names in the
> > > + instruction list. This means that instead of looking like this,
> > > +
> > > +---
> > > + pick deadbee The oneline of this commit
> > > + pick fa1afe1 The oneline of the next commit
> > > + ...
> > > +---
> > > +
> > > + the list would use the short version of the command resulting in
> > > + something like this.
> > > +
> > > +---
> > > + p deadbee The oneline of this commit
> > > + p fa1afe1 The oneline of the next commit
> > > + ...
> > > +---
> > 
> > That doesn't explain the point of the option.
> 
> And what you forgot to say in order to make this a constructive criticism
> is: we probably want to add a sentence like this:
> 
> 
>   Using the one-letter abbreviations will align the lines better
>   in case that the non-abbreviated commands have different lengths.
> 
> Speaking of commands with different lengths, I just thought of fixup and
> squash. I do not think those are handled by the patch, but they should be
> (the `action` in the first loop of `rearrange_squash` should abbreviate
> via `test p != "$pickcmd" || action=${action%${action#?}}`).
> 

I just noticed this today, I'll make changes to handle this case. 

> Ciao,
> Johannes

Thanks,
Liam

[PATCH v2] rebase -i: add config to abbreviate command-names

[Liam Beguin]

Add the 'rebase.abbrevCmd' boolean config option to allow `git rebase -i`
to abbreviate the command-names in the instruction list.

This means that `git rebase -i` would print:
p deadbee The oneline of this commit
...

instead of:
pick deadbee The oneline of this commit
...

Using a single character command-name allows the lines to remain
aligned, making the whole set more readable.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
Changes since v1:
 - Improve Documentation and commit message

 Documentation/config.txt | 19 +++
 Documentation/git-rebase.txt | 19 +++
 git-rebase--interactive.sh   |  8 ++--
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5155..8b1877f2df91 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2614,6 +2614,25 @@ rebase.instructionFormat::
the instruction list during an interactive rebase.  The format will 
automatically
have the long commit hash prepended to the format.
 
+rebase.abbrevCmd::
+   If set to true, `git rebase -i` will abbreviate the command-names in the
+   instruction list. This means that instead of looking like this,
+
+---
+   pick deadbee The oneline of this commit
+   pick fa1afe1 The oneline of the next commit
+   ...
+---
+
+   the list would use the short version of the command resulting in
+   something like this.
+
+---
+   p deadbee The oneline of this commit
+   p fa1afe1 The oneline of the next commit
+   ...
+---
+
 receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
capability to its clients. If you don't want to advertise this
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 67d48e688315..7d97c0483241 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -222,6 +222,25 @@ rebase.missingCommitsCheck::
 rebase.instructionFormat::
Custom commit list format to use during an `--interactive` rebase.
 
+rebase.abbrevCmd::
+   If set to true, `git rebase -i` will abbreviate the command-names in the
+   instruction list. This means that instead of looking like this,
+
+---
+   pick deadbee The oneline of this commit
+   pick fa1afe1 The oneline of the next commit
+   ...
+---
+
+   the list would use the short version of the command resulting in
+   something like this.
+
+---
+   p deadbee The oneline of this commit
+   p fa1afe1 The oneline of the next commit
+   ...
+---
+
 OPTIONS
 ---
 --onto ::
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2c9c0165b5ab..9f3e82b79615 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1210,6 +1210,10 @@ else
revisions=$onto...$orig_head
shortrevisions=$shorthead
 fi
+
+rebasecmd=pick
+test "$(git config --bool --get rebase.abbrevCmd)" = true && rebasecmd=p
+
 format=$(git config --get rebase.instructionFormat)
 # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to 
parse
 git rev-list $merges_option --format="%m%H ${format:-%s}" \
@@ -1228,7 +1232,7 @@ do
 
if test t != "$preserve_merges"
then
-   printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
+   printf '%s\n' "${comment_out}${rebasecmd} $sha1 $rest" >>"$todo"
else
if test -z "$rebase_root"
then
@@ -1246,7 +1250,7 @@ do
if test f = "$preserve"
then
touch "$rewritten"/$sha1
-   printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
+   printf '%s\n' "${comment_out}${rebasecmd} $sha1 $rest" 
>>"$todo"
fi
fi
 done
-- 
2.9.3

[PATCH v2] rebase -i: add config to abbreviate command-names

[Liam Beguin]

Add the 'rebase.abbrevCmd' boolean config option to allow `git rebase -i`
to abbreviate the command-names in the instruction list.

This means that `git rebase -i` would print:
p deadbee The oneline of this commit
...

instead of:
pick deadbee The oneline of this commit
...

Using a single character command-name allows the lines to remain
aligned, making the whole set more readable.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
Changes since v1:
 - Improve Documentation and commit message

 Documentation/config.txt | 19 +++
 Documentation/git-rebase.txt | 19 +++
 git-rebase--interactive.sh   |  8 ++--
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5155..8b1877f2df91 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2614,6 +2614,25 @@ rebase.instructionFormat::
the instruction list during an interactive rebase.  The format will 
automatically
have the long commit hash prepended to the format.
 
+rebase.abbrevCmd::
+   If set to true, `git rebase -i` will abbreviate the command-names in the
+   instruction list. This means that instead of looking like this,
+
+---
+   pick deadbee The oneline of this commit
+   pick fa1afe1 The oneline of the next commit
+   ...
+---
+
+   the list would use the short version of the command resulting in
+   something like this.
+
+---
+   p deadbee The oneline of this commit
+   p fa1afe1 The oneline of the next commit
+   ...
+---
+
 receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
capability to its clients. If you don't want to advertise this
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 67d48e688315..7d97c0483241 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -222,6 +222,25 @@ rebase.missingCommitsCheck::
 rebase.instructionFormat::
Custom commit list format to use during an `--interactive` rebase.
 
+rebase.abbrevCmd::
+   If set to true, `git rebase -i` will abbreviate the command-names in the
+   instruction list. This means that instead of looking like this,
+
+---
+   pick deadbee The oneline of this commit
+   pick fa1afe1 The oneline of the next commit
+   ...
+---
+
+   the list would use the short version of the command resulting in
+   something like this.
+
+---
+   p deadbee The oneline of this commit
+   p fa1afe1 The oneline of the next commit
+   ...
+---
+
 OPTIONS
 ---
 --onto ::
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2c9c0165b5ab..9f3e82b79615 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1210,6 +1210,10 @@ else
revisions=$onto...$orig_head
shortrevisions=$shorthead
 fi
+
+rebasecmd=pick
+test "$(git config --bool --get rebase.abbrevCmd)" = true && rebasecmd=p
+
 format=$(git config --get rebase.instructionFormat)
 # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to 
parse
 git rev-list $merges_option --format="%m%H ${format:-%s}" \
@@ -1228,7 +1232,7 @@ do
 
if test t != "$preserve_merges"
then
-   printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
+   printf '%s\n' "${comment_out}${rebasecmd} $sha1 $rest" >>"$todo"
else
if test -z "$rebase_root"
then
@@ -1246,7 +1250,7 @@ do
if test f = "$preserve"
then
touch "$rewritten"/$sha1
-   printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
+   printf '%s\n' "${comment_out}${rebasecmd} $sha1 $rest" 
>>"$todo"
fi
fi
 done
-- 
2.9.3

Re: [PATCH] rebase -i: add config to abbreviate command name

[liam BEGUIN]

Hi Johannes,

On Mon, 2017-04-24 at 12:26 +0200, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sun, 23 Apr 2017, Liam Beguin wrote:
> 
> > Add the 'rebase.abbrevCmd' boolean config option to allow
> > the user to abbreviate the default command name while editing
> > the 'git-rebase-todo' file.
> 
> This patch does not handle the `git rebase --edit-todo` subcommand.
> Intentional?

After a little more investigation, I'm not sure what should be added for
the `git rebase --edit-todo` subcommand. It seems like it uses the same 
text that was added the first time (with `git rebase -i`). 
Do you have a bit more information about what you meant? 
I don't use this subcommand very often, I'm most likely missing something.

> 
> Ciao,
> Johannes

Thanks, 
Liam 

Re: [PATCH] rebase -i: add config to abbreviate command name

[liam BEGUIN]

Hi, 

On Mon, 2017-04-24 at 12:26 +0200, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sun, 23 Apr 2017, Liam Beguin wrote:
> 
> > Add the 'rebase.abbrevCmd' boolean config option to allow
> > the user to abbreviate the default command name while editing
> > the 'git-rebase-todo' file.
> 
> This patch does not handle the `git rebase --edit-todo` subcommand.
> Intentional?

no, this is not intentional, I'll make the changes.
 
> 
> Ciao,
> Johannes

Thanks, 
Liam

[PATCH] rebase -i: add config to abbreviate command name

[Liam Beguin]

Add the 'rebase.abbrevCmd' boolean config option to allow
the user to abbreviate the default command name while editing
the 'git-rebase-todo' file.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
Notes:

 *  This allows the lines to remain aligned when using single
letter commands.

 Documentation/config.txt | 3 +++
 Documentation/git-rebase.txt | 3 +++
 git-rebase--interactive.sh   | 8 ++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5155..59b64832aeb4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2614,6 +2614,9 @@ rebase.instructionFormat::
the instruction list during an interactive rebase.  The format will 
automatically
have the long commit hash prepended to the format.
 
+rebase.abbrevCmd::
+   If set to true, abbreviate command name in interactive mode.
+
 receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
capability to its clients. If you don't want to advertise this
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 67d48e688315..0c423d903625 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -222,6 +222,9 @@ rebase.missingCommitsCheck::
 rebase.instructionFormat::
Custom commit list format to use during an `--interactive` rebase.
 
+rebase.abbrevCmd::
+   If set to true, abbreviate command name in interactive mode.
+
 OPTIONS
 ---
 --onto ::
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2c9c0165b5ab..9f3e82b79615 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1210,6 +1210,10 @@ else
revisions=$onto...$orig_head
shortrevisions=$shorthead
 fi
+
+rebasecmd=pick
+test "$(git config --bool --get rebase.abbrevCmd)" = true && rebasecmd=p
+
 format=$(git config --get rebase.instructionFormat)
 # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to 
parse
 git rev-list $merges_option --format="%m%H ${format:-%s}" \
@@ -1228,7 +1232,7 @@ do
 
if test t != "$preserve_merges"
then
-   printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
+   printf '%s\n' "${comment_out}${rebasecmd} $sha1 $rest" >>"$todo"
else
if test -z "$rebase_root"
then
@@ -1246,7 +1250,7 @@ do
if test f = "$preserve"
then
touch "$rewritten"/$sha1
-   printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
+   printf '%s\n' "${comment_out}${rebasecmd} $sha1 $rest" 
>>"$todo"
fi
fi
 done
-- 
2.9.3