git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCHv5 1/3] wt-status.*: better advices for git status added

From
Junio C Hamano <gitster@pobox.com>
Date
May 31, 2012, 21:36 UTC
Message-ID
<7vmx4o58zc.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1338477344-15940-1-git-send-email-Lucien.Kong@ensimag.imag.fr>
Kong Lucien <Lucien.Kong@ensimag.imag.fr> writes:
Show 8 quoted lines
> This patch provides new informative help messages in the display of
> 'git status' (at the top) during conflicts, rebase, am, bisect or
> cherry-pick process.
>
> The new messages are not shown when using options such as -s or
> --porcelain. The messages about the current situation of the user are
> always displayed but the advices on what the user needs to do in order
> to resume a rebase/bisect /am/ commit after resolving conflicts can be
Is there a reason why there is a SP after bisect here?
Show 7 quoted lines
> hidden by setting advice.statushints to 'false' in the config file.
>
> Thus, information about the updated advice.statushints key are added
> in Documentation/config.txt.
>
> Also, the test t7060-wt-status.sh is now working with the new help
> messages. Tests about suggestions of "git rm" are also added.

Looks much better and getting ready for being at least on 'pu' if not 'next', it seems.

Show 5 quoted lines
> Signed-off-by: Kong Lucien <Lucien.Kong@ensimag.imag.fr>
> Signed-off-by: Duperray Valentin <Valentin.Duperray@ensimag.imag.fr>
> Signed-off-by: Jonas Franck <Franck.Jonas@ensimag.imag.fr>
> Signed-off-by: Nguy Thomas <Thomas.Nguy@ensimag.imag.fr>
> Signed-off-by: Nguyen Huynh Khoi Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>

How is your development process work, by the way? Does everybody on this list have code in this patch? Or are you just listing people who are in the same class taught by Matthieu who reviewed and commented on this patch?

Show 16 quoted lines
>  Documentation/config.txt |    2 +
>  t/t7060-wtstatus.sh      |   71 +++++++++++++++++++++
>  wt-status.c              |  154 ++++++++++++++++++++++++++++++++++++++++++++++
>  wt-status.h              |   11 +++
>  4 files changed, 238 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 915cb5a..52f5009 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -162,6 +162,8 @@ advice.*::
>  		Directions on how to stage/unstage/add shown in the
>  		output of linkgit:git-status[1] and the template shown
>  		when writing commit messages.
> +		Directions on how to end the current process shown
> +		in the output of linkgit:git-status[1].

I kind of find this hard to read. Perhaps it shouldn't be a separate sentence, but instead should just enhance the existing limited "stage/unstage/add" set? The list of updated, modified and untracked paths with instructions given by the current code is an incomplete description of the current state, and I view this patch as filling the missing bits to it to make the output more complete.

So from that point of view, perhaps more like this?

- Directions on how to stage/unstage/add shown in the - output of linkgit:git-status[1] and the template shown - when writing commit messages. + Show directions on how to proceed from the current state + in the output of linkgit:git-status[1] and in the template + shown when writing commit messages in linkgit:git-commit[1].

Show 16 quoted lines
> diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
> index b8cb490..8a6d68a 100755
> --- a/t/t7060-wtstatus.sh
> +++ b/t/t7060-wtstatus.sh
> @@ -30,6 +30,8 @@ test_expect_success 'Report new path with conflict' '
>  
>  cat >expect <<EOF
>  # On branch side
> +# You have unmerged paths; fix conflicts and run "git commit".
> +#
>  # Unmerged paths:
>  #   (use "git add/rm <file>..." as appropriate to mark resolution)
>  #
> @@ -118,4 +120,73 @@ test_expect_success 'git diff-index --cached -C shows 2 copies + 1 unmerged' '
>  	test_cmp expected actual
>  '

Note what filenames are used to store the expected output and grab the actual output out of the command.

Show 5 quoted lines
> +
> +test_expect_success 'status when conflicts with add and rm advice (deleted by them)' '
> +	git init git &&
> +	cd git &&
> +	test_when_finished "cd ../ && rm -rf git" &&

This is a tangent, but this sequence is wrong. If "git init" succeeds but "cd git" fails for some reason, you wouldn't clean the new test repository, and worse, you go up and try to remove a random "git" that is different from what you created here.

Perhaps I am being a bit superstitous, but I would feel a lot safer to see if this were written like this:

        git init git &&
        (
		cd git &&
                ... the rest of this test ...
	) &&
        rm -fr git

to avoid mistakes. For example, if you add any "cd somewhere-else" in the remainder of the test, your test_when_finished to go to a relative "cd ../" wouldn't do you any good.

Show 29 quoted lines
> +	test_commit init main.txt init &&
> +	git checkout -b second_branch &&
> +	git rm main.txt &&
> +	git commit -m "main.txt deleted on second_branch" &&
> +	test_commit second conflict.txt second &&
> +	git checkout master &&
> +	test_commit on_second main.txt on_second &&
> +	test_commit master conflict.txt master &&
> +	test_must_fail git merge second_branch &&
> +	cat >expect <<-\EOF &&
> +	# On branch master
> +	# You have unmerged paths; fix conflicts and run "git commit".
> +	#
> +	# Unmerged paths:
> +	#   (use "git add/rm <file>..." as appropriate to mark resolution)
> +	#
> +	#	both added:         conflict.txt
> +	#	deleted by them:    main.txt
> +	#
> +	# Untracked files:
> +	#   (use "git add <file>..." to include in what will be committed)
> +	#
> +	#	expect
> +	#	output
> +	no changes added to commit (use "git add" and/or "git commit -a")
> +	EOF
> +	git status >output &&
> +	test_cmp expect output
> +'

And rename "output" to "actual" to match the existing practice (unless there is a compelling reason not to).

Show 32 quoted lines
> diff --git a/wt-status.c b/wt-status.c
> index dd6d8c4..f4ba021 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -23,6 +23,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
>  	GIT_COLOR_GREEN,  /* WT_STATUS_LOCAL_BRANCH */
>  	GIT_COLOR_RED,    /* WT_STATUS_REMOTE_BRANCH */
>  	GIT_COLOR_NIL,    /* WT_STATUS_ONBRANCH */
> +	GIT_COLOR_NORMAL, /* WT_STATUS_IN_PROGRESS */
>  };
>  
>  static const char *color(int slot, struct wt_status *s)
> @@ -728,6 +729,158 @@ static void wt_status_print_tracking(struct wt_status *s)
>  	color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "#");
>  }
>  
> +static int has_unmerged(struct wt_status *s)
> +{
> +	int i;
> +
> +	for (i = 0; i < s->change.nr; i++) {
> +		struct wt_status_change_data *d;
> +		d = s->change.items[i].util;
> +		if (d->stagemask)
> +			return 1;
> +	}
> +	return 0;
> +}
> +
> +static void merge_in_progress_show(struct wt_status *s,
> +				struct wt_status_state *state,
> +				const char *color)

Minor nit: perhaps "show-merge-in-progress" would read more naturally (same for all other helpers).

Show 9 quoted lines
> +{
> +	if (has_unmerged(s))
> +		status_printf_ln(s, color, _("You have unmerged paths%s"),
> +			advice_status_hints
> +			? _("; fix conflicts and run \"git commit\".") : ".");
> +	else
> +		status_printf_ln(s, color, _("You are still merging%s"),
> +			advice_status_hints
> +			? _(", run \"git commit\" to conclude merge.") : ".");

The former uses ';' while the latter uses ','. There is a use of ':' in the rebase-in-progress helper. Be consistent (if I were writing this code, I would pick ';').

Would this qualify as a "language lego" i18n/l10n people loathe? IOW would it be better to have four independent messages?

        "You have unmerged paths."
        "You are still merging."
        "You have unmerged paths; fix conflicts and..."
	"You are still merging; fun \"git commit\" to conclude..."

This is not a rhetorical question. There is the same issue in the rebase-in-progress helper.

Show 9 quoted lines
> +static void am_in_progress_show(struct wt_status *s,
> +				struct wt_status_state *state,
> +				const char *color)
> +{
> +	status_printf_ln(s, color,
> +		_("You are in the middle of an am session:"));
> +	if (state->am_empty_patch)
> +		status_printf_ln(s, color,
> +			_("The current patch is empty!"));
This situation may deserve a "; run 'am --skip' to skip it." advice.
Show 13 quoted lines
> +static void rebase_in_progress_show(struct wt_status *s,
> +				struct wt_status_state *state,
> +				const char *color)
> +{
> +	if (has_unmerged(s)) {
> +		status_printf_ln(s, color, _("You are currently rebasing%s"),
> +			advice_status_hints
> +			? _(": fix conflicts and then run \"git rebase --continue\".") : ".");
> +		if (advice_status_hints) {
> +			status_printf_ln(s, color,
> +				_("  If you would prefer to skip this patch, instead run \"git rebase --skip\"."));
> +			status_printf_ln(s, color,
> +				_("  To check out  the original branch and stop rebasing run \"git rebase --abort\"."));
Double-space in the middle intended?

This line looks overly long. Does it fit (with the # prefix) on typical 80-column terminals?

Show 5 quoted lines
> +		}
> +	} else if (state->rebase_in_progress) {
> +		status_printf_ln(s, color, _("You are currently rebasing%s"),
> +			advice_status_hints
> +			? _(": all conflicts fixed: run \"git rebase --continue\".") : ".");
The above will show this:
# You are currently rebasing: all conflicts fixed: run "git rebase --continue.

That's 78 cols, which is pushing the right edge too closely, especially considering that translations tend to make messages longer.

Aside from questionable use of ':' (I think ';' is better for the first one, and '.' is better for the latter; and s/run/Run/ to begin a new sentence), I think this is nicer than the merge-in-progress message after the user has already marked all paths resolved, which says "You are still merging" without saying "you have resolved everything". Perhaps you meant to hint that by saying "still" (as opposed to "currently"), but the distinction feels a bit too subtle.

Show 7 quoted lines
> +	} else {
> +		status_printf_ln(s, color, _("You are currently editing a commit during a rebase."));
> +		if (advice_status_hints) {
> +			status_printf_ln(s, color, _("  You can amend the commit with"));
> +			status_printf_ln(s, color, _("	git commit --amend"));
> +			status_printf_ln(s, color, _("  Once you are satisfied with your changes, run"));
> +			status_printf_ln(s, color, _("	git rebase --continue"));

For an advice in "git status" output, the above may be appropriate, but would the user see this in "git commit" template, and if so, isn't it because the user typed "git commit --amend"? Does it make sense to suggest to run "git commit --amend" in that context?

The same comment applies to all new advice messages added by this patch.

Thanks.
Previous: Junio C HamanoNext: konglu@minatec.inpg.fr
Message 43 of 97 in “t7512-status-warnings.sh : better advices for git status”
  1. t7512-status-warnings.sh : better advices for git statusKong Lucien, May 24, 2012
  2. Matthieu MoyMay 24, 2012
  3. 1/2 wt-status: better advices for git statusKong Lucien, May 26, 2012
  4. 2/2 t7512-status-warnings.sh: better advices for git statusKong Lucien, May 26, 2012
  5. Matthieu MoyMay 27, 2012
  6. Matthieu MoyMay 28, 2012
  7. Nguyen Thai Ngoc DuyMay 26, 2012
  8. NGUY ThomasMay 26, 2012
  9. Matthieu MoyMay 27, 2012
  10. Matthieu MoyMay 27, 2012
  11. Junio C HamanoMay 28, 2012
  12. Matthieu MoyMay 28, 2012
  13. Junio C HamanoMay 30, 2012
  14. konglu@minatec.inpg.frMay 28, 2012
  15. 1/2 wt-status.*: better advices for git status addedKong Lucien, May 28, 2012
  16. 2/2 t7512-status-warnings.sh: better advices for git statusKong Lucien, May 28, 2012
  17. Matthieu MoyMay 28, 2012
  18. Matthieu MoyMay 28, 2012
  19. Junio C HamanoMay 29, 2012
  20. konglu@minatec.inpg.frMay 30, 2012
  21. Junio C HamanoMay 30, 2012
  22. Matthieu MoyMay 31, 2012
  23. 1/3 wt-status.*: better advices for git status addedKong Lucien, May 30, 2012
  24. 2/3 t7512-status-warnings.sh: better advices for git statusKong Lucien, May 30, 2012
  25. 3/3 Advices about 'git rm' during conflicts (unmerged paths) more relevantKong Lucien, May 30, 2012
  26. Junio C HamanoMay 30, 2012
  27. konglu@minatec.inpg.frMay 30, 2012
  28. Matthieu MoyMay 31, 2012
  29. konglu@minatec.inpg.frMay 31, 2012
  30. Junio C HamanoMay 30, 2012
  31. Junio C HamanoMay 30, 2012
  32. Matthieu MoyMay 31, 2012
  33. Matthieu MoyMay 31, 2012
  34. Matthieu MoyMay 31, 2012
  35. Andrew ArdillMay 31, 2012
  36. 1/3 wt-status.*: better advices for git status addedKong Lucien, May 31, 2012
  37. 2/3 t7512-status-help.sh: better advices for git statusKong Lucien, May 31, 2012
  38. 3/3 status: don't suggest "git rm" or "git add" if not appropriateKong Lucien, May 31, 2012
  39. Matthieu MoyJun 1, 2012
  40. Phil HordJun 1, 2012
  41. konglu@minatec.inpg.frJun 1, 2012
  42. Junio C HamanoJun 1, 2012
  43. Junio C HamanoMay 31, 2012
  44. konglu@minatec.inpg.frJun 1, 2012
  45. Matthieu MoyJun 1, 2012
  46. Junio C HamanoJun 1, 2012
  47. Junio C HamanoJun 1, 2012
  48. konglu@minatec.inpg.frJun 1, 2012
  49. Matthieu MoyJun 1, 2012
  50. konglu@minatec.inpg.frJun 1, 2012
  51. Phil HordJun 1, 2012
  52. Junio C HamanoJun 1, 2012
  53. Phil HordJun 4, 2012
  54. 1/4 wt-status.*: better advices for git status addedKong Lucien, Jun 3, 2012
  55. 2/4 t7512-status-help.sh: better advices for git statusKong Lucien, Jun 3, 2012
  56. Junio C HamanoJun 3, 2012
  57. konglu@minatec.inpg.frJun 4, 2012
  58. Junio C HamanoJun 4, 2012
  59. Junio C HamanoJun 4, 2012
  60. 3/4 status: don't suggest "git rm" or "git add" if not appropriateKong Lucien, Jun 3, 2012
  61. Matthieu MoyJun 3, 2012
  62. 4/4 status: better advices when splitting a commit (during rebase -i)Kong Lucien, Jun 3, 2012
  63. Junio C HamanoJun 3, 2012
  64. Phil HordJun 4, 2012
  65. konglu@minatec.inpg.frJun 5, 2012
  66. Junio C HamanoJun 3, 2012
  67. konglu@minatec.inpg.frJun 4, 2012
  68. Junio C HamanoJun 4, 2012
  69. 1/4 wt-status.*: better advices for git status addedKong Lucien, Jun 4, 2012
  70. 2/4 t7512-status-help.sh: better advices for git statusKong Lucien, Jun 4, 2012
  71. Matthieu MoyJun 4, 2012
  72. Junio C HamanoJun 4, 2012
  73. 3/4 status: don't suggest "git rm" or "git add" if not appropriateKong Lucien, Jun 4, 2012
  74. 4/4 status: better advices when splitting a commit (during rebase -i)Kong Lucien, Jun 4, 2012
  75. Junio C HamanoJun 4, 2012
  76. konglu@minatec.inpg.frJun 5, 2012
  77. Matthieu MoyJun 5, 2012
  78. 1/4 wt-status.*: better advices for git status addedLucien Kong, Jun 5, 2012
  79. 2/4 t7512-status-help.sh: better advices for git statusLucien Kong, Jun 5, 2012
  80. 3/4 status: don't suggest "git rm" or "git add" if not appropriateLucien Kong, Jun 5, 2012
  81. 4/4 status: better advices when splitting a commit (during rebase -i)Lucien Kong, Jun 5, 2012
  82. Junio C HamanoJun 5, 2012
  83. konglu@minatec.inpg.frJun 8, 2012
  84. Junio C HamanoJun 8, 2012
  85. 1/4 wt-status.*: better advices for git status addedLucien Kong, Jun 7, 2012
  86. 2/4 t7512-status-help.sh: better advices for git statusLucien Kong, Jun 7, 2012
  87. 3/4 status: don't suggest "git rm" or "git add" if not appropriateLucien Kong, Jun 7, 2012
  88. 4/4 status: better advices when splitting a commit (during rebase -i)Lucien Kong, Jun 7, 2012
  89. Junio C HamanoJun 7, 2012
  90. Junio C HamanoJun 7, 2012
  91. konglu@minatec.inpg.frJun 7, 2012
  92. 1/4 wt-status.*: better advices for git status addedLucien Kong, Jun 10, 2012
  93. 2/4 t7512-status-help.sh: better advices for git statusLucien Kong, Jun 10, 2012
  94. 3/4 status: don't suggest "git rm" or "git add" if not appropriateLucien Kong, Jun 10, 2012
  95. 4/4 status: better advices when splitting a commit (during rebase -i)Lucien Kong, Jun 10, 2012
  96. Junio C HamanoJun 11, 2012
  97. konglu@minatec.inpg.frJun 12, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.