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

Re: [PATCHv2 1/2] wt-status: better advices for git status

From
Junio C Hamano <gitster@pobox.com>
Date
May 28, 2012, 04:57 UTC
Message-ID
<7v1um47vik.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1338035905-24166-1-git-send-email-Lucien.Kong@ensimag.imag.fr>

Kong Lucien <Lucien.Kong@ensimag.imag.fr> writes: Kong Lucien <Lucien.Kong@ensimag.imag.fr> writes:

> This patch provides more information about your current state after a git status command (in the cases of conflicts, before and after they are resolved, a rebase or a bisect process).
> This would help users to know what they are currently doing, in a more accurate way.
Please fix these overlong lines.

The description is unclear what problem it tries to solve and how, and invites many questions. Does it add more lines at the top? At the bottom? How verbosely? By pushing down existing information by adding extra lines somewhere, the existing output lines may be made harder to read, but how badly? How does this affect "status -s/status --porcelain" output? What does the new code do to figure out the "current state"? By heuristics? How often does the heuristics get it wrong and in what circumstances?

Show 12 quoted lines
> diff --git a/wt-status.c b/wt-status.c
> index dd6d8c4..9839280 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -15,6 +15,7 @@
>  
>  static char default_wt_status_colors[][COLOR_MAXLEN] = {
>  	GIT_COLOR_NORMAL, /* WT_STATUS_HEADER */
> +	GIT_COLOR_NORMAL, /* WT_STATUS_IN_PROGRESS */
>  	GIT_COLOR_GREEN,  /* WT_STATUS_UPDATED */
>  	GIT_COLOR_RED,    /* WT_STATUS_CHANGED */
>  	GIT_COLOR_RED,    /* WT_STATUS_UNTRACKED */
Why add new stuff in the middle, not at the end?
Show 10 quoted lines
> @@ -728,6 +729,92 @@ static void wt_status_print_tracking(struct wt_status *s)
>  	color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "#");
>  }
>  
> +static void wt_status_print_in_progress(struct wt_status *s)
> +{
> +	int i;
> +	const char *c = color(WT_STATUS_IN_PROGRESS, s);
> +	const char *git_dir = getenv(GIT_DIR_ENVIRONMENT);
> +	const char* path;
Show 5 quoted lines
> +	int unmerged_state = 0;
> +	int rebase_state = 0;
> +	int rebase_interactive_state = 0;
> +	int am_state = 0;
> +	int bisect_state = 0;

These are not independent (you cannot be in bisect and am at the same time). Why five independent variables?

> +	int conflict = 0;
How is this different from "unmerged"?
Show 10 quoted lines
> +	for (i = 0; i < s->change.nr; i++) {
> +		struct wt_status_change_data *d;
> +		struct string_list_item *it;
> +		it = &(s->change.items[i]);
> +		d = it->util;
> +		if (d->stagemask) {
> +			conflict = 1;
> +			continue;
> +		}
> +	}

That "continue" looks like a no-op. Mental note: conflict seems to remember if there was any path that was unmerged.

> +	path = mkpath("%s/MERGE_HEAD", git_dir);
> +	if (!access(path, R_OK))
> +		unmerged_state = 1;

Ahh, so "unmerged" is "conflicted during merge" (as opposed to rebase_state is "conflicted during rebase")? Doesn't the naming sound odd? If it were "merge_state", it might have made a bit more sense (but again, these are not independent conditions, so multiple variables do not make sense).

Show 18 quoted lines
> +	path = mkpath("%s/rebase-apply", git_dir);
> +	if (!access(path, R_OK)) {
> +		path = mkpath("%s/rebase-apply/applying", git_dir);
> +		if (!access(path, R_OK))
> +			am_state = 1;
> +		else
> +			rebase_state = 1;
> +	}
> +	else {
> +		path = mkpath("%s/rebase-merge", git_dir);
> +		if (!access(path, R_OK)) {
> +			path = mkpath("%s/rebase-merge/interactive", git_dir);
> +			if (!access(path, R_OK))
> +				rebase_interactive_state = 1;
> +			else
> +				rebase_state = 1;
> +		}
> +	}

The above if/else makes it clear that if you are in "am" you can never be in "rebase -i", but doesn't it strike you odd that the check for MERGE_HEAD is not cascaded the same way? I.e. if you know you are in "merge", you cannot be "am" nor "rebase", but you check the latter anyway even after you know you are in "merge".

> +	path = mkpath("%s/BISECT_LOG", git_dir);
> +	if (!access(path, R_OK))
> +		bisect_state = 1;
Likewise.
> +	if(bisect_state) {
s/if/if /;
Show 6 quoted lines
> +		status_printf_ln(s, c, _("You are currently bisecting."));
> +		status_printf_ln(s, c, _("To get back to the original branch run \"git bisect reset\""));
> +		wt_status_print_trailer(s);
> +	}
> +
> +	if(unmerged_state) {
Likewise.
Show 6 quoted lines
> +		if (conflict)
> +			status_printf_ln(s, c, _("You have unmerged paths: fix conflicts and then commit the result."));
> +		else
> +			status_printf_ln(s, c, _("You are still merging, run \"git commit\" to conclude merge."));
> +		wt_status_print_trailer(s);
> +	}
It is annoying that the above does things in random order, i.e.
	if (are we in X)
        	set state to X
	if (are we in Y)
        	set state to Y
	else if (are we in Z)
		set state to Z
	if (are we in W)
		set state to W
	if (Z)
        	say things about Z
	if (X)
		say things about X
	if (Y)
		say things about Y

Such a code structure invites bugs and missed cases (e.g. you do not seem to say anything after you detect that you are in "am").

Show 7 quoted lines
> +	if(rebase_state || rebase_interactive_state) {
> +		if (conflict) {
> +			status_printf_ln(s, c, _("You are currently rebasing: fix conflicts and then run \"git rebase -- continue\"."));
> +			status_printf_ln(s, c, _("If you would prefer to skip this patch, instead run \"git rebase --skip\"."));
> +			status_printf_ln(s, c, _("To check out  the original branch and stop rebasing run \"git rebase --abort\"."));
> +		}
> +		else {
	if (...) {
		...
	} else {
		...
	}
> +			if (rebase_state)
Why extra level of nesting?
Show 8 quoted lines
> +				status_printf_ln(s, c, _("You are currently rebasing: all conflicts fixed; run \"git rebase --continue\"."));
> +			else {
> +				status_printf_ln(s, c, _("You are currently editing in a rebase progress."));
> +				status_printf_ln(s, c, _("You can amend the commit with"));
> +				status_printf_ln(s, c, _("	git commit --amend"));
> +				status_printf_ln(s, c, _("Once you are satisfied with your changes, run"));
> +				status_printf_ln(s, c, _("	git rebase --continue"));
> +			}

I am not sure if this level of verbosity is a good thing, given that you are adding this near the very beginning of the output. When you have many conflicted or modified paths, these advice messages will scroll off the top.

Oh, another thing. Perhaps these (both detection logic and output) should be protected with a new advise.* configuration variable, no?

Previous: Matthieu MoyNext: Matthieu Moy
Message 11 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.