| Message ID | 20180907224430.23859-2-timschumi@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v4,1/3] Add support for nested aliases | expand |
On Sat, Sep 8, 2018 at 12:44 AM Tim Schumacher <timschumi@gmx.de> wrote: > > Just printing the command that the user entered is not particularly > helpful when trying to find the alias that causes the loop. > > Print the history of substituted commands to help the user find the > offending alias. Mark the entrypoint of the loop with "<==" and the > last command (which looped back to the entrypoint) with "==>". An even simpler way to give this information is simply suggest the user tries again with GIT_TRACE=1. All alias expansion is shown there and we teach the user about GIT_TRACE. But your approach is probably more user friendly.
On Sat, Sep 08, 2018 at 03:34:34PM +0200, Duy Nguyen wrote: > On Sat, Sep 8, 2018 at 12:44 AM Tim Schumacher <timschumi@gmx.de> wrote: > > > > Just printing the command that the user entered is not particularly > > helpful when trying to find the alias that causes the loop. > > > > Print the history of substituted commands to help the user find the > > offending alias. Mark the entrypoint of the loop with "<==" and the > > last command (which looped back to the entrypoint) with "==>". > > An even simpler way to give this information is simply suggest the > user tries again with GIT_TRACE=1. All alias expansion is shown there > and we teach the user about GIT_TRACE. But your approach is probably > more user friendly. Good point. I'm OK with the amount of code here for the nicer message (but would be happy either way). If we were going to track cross-process loops like Ævar suggested, I think I'd rather go with a simple counter and just ask the user to run with GIT_TRACE when it exceeds some maximum sanity value. For two reasons: 1. Passing a counter through the environment is way simpler than an arbitrarily-sized list. 2. When you get into multiple processes, there's potentially more going on than just Git commands. You might have a git command which runs a hook which runs a third party script which runs a git command, which runs a hook, and so on. That full dump is going to be more useful. -Peff
diff --git a/git.c b/git.c index 15727c17f..a20eb4fa1 100644 --- a/git.c +++ b/git.c @@ -675,6 +675,7 @@ static int run_argv(int *argcp, const char ***argv) { int done_alias = 0; struct string_list cmd_list = STRING_LIST_INIT_NODUP; + struct string_list_item *seen; while (1) { /* @@ -692,9 +693,21 @@ static int run_argv(int *argcp, const char ***argv) /* .. then try the external ones */ execv_dashed_external(*argv); - if (unsorted_string_list_has_string(&cmd_list, *argv[0])) { + seen = unsorted_string_list_lookup(&cmd_list, *argv[0]); + if (seen) { + int i; + struct strbuf sb = STRBUF_INIT; + for (i = 0; i < cmd_list.nr; i++) { + struct string_list_item *item = &cmd_list.items[i]; + + strbuf_addf(&sb, "\n %s", item->string); + if (item == seen) + strbuf_addstr(&sb, " <=="); + else if (i == cmd_list.nr - 1) + strbuf_addstr(&sb, " ==>"); + } die(_("alias loop detected: expansion of '%s' does" - " not terminate"), cmd_list.items[0].string); + " not terminate:%s"), cmd_list.items[0].string, sb.buf); } string_list_append(&cmd_list, *argv[0]);
Just printing the command that the user entered is not particularly helpful when trying to find the alias that causes the loop. Print the history of substituted commands to help the user find the offending alias. Mark the entrypoint of the loop with "<==" and the last command (which looped back to the entrypoint) with "==>". Signed-off-by: Tim Schumacher <timschumi@gmx.de> --- I now went with Peff's suggested code and I added in an arrow that points away from the last command (which caused the loop). A "full" arrow (i.e. starts at the last command, goes upwards and ends at the entrypoint) would be more obvious/better, but adding much more code just for having a vertical line wasn't worth it for me. git.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)