Re: [PATCH] git-status: show short sequencer state
Phil Hord ho...@cisco.com writes: + if (state-substate==WT_SUBSTATE_NOMINAL) status_printf_ln(s, color, _(The current patch is empty.)); This looks weird. First, spaces around == (here and below). Then, the logic is unintuitive. The if suggests everything is allright, and the message below is very specific. This at least deserves a comment. Yes, I agree. It was less clear but more reasonable before I tried to clear it up some. It's driven by the short-token printer. The state is you're in a 'git am' but I do not see any conflicted files. Therefore, your patch must be empty. This was my guess, but I wouldn't have needed to guess if there was a comment in the code ;-). I'll try to make this more explicit. Currently the short-status version will say either am or am \n conflicted when a 'git am' is in progress. The logical path to follow if I re-add 'git-am-empty' state tracker is for this to now show either am \n am-is-empty or am \n conflicted. But I think I should suppress the am-is-empty report in that case. What do you think I don't think you should remove it from the output (no strong opinion). My point was just that the code looked weird. +static void wt_print_token(struct wt_status *s, const char *color, const char *token) +{ + color_fprintf(s-fp, color, %s, token); + fputc(s-null_termination ? '\0' : '\n', s-fp); +} The output format seems to be meant only for machine-consumption. Is there any case when we'd want color? [...] [...]I thought I might be going back there, or that I might combine this with full 'git status' again somehow, and colors seemed appropriate still. [...] So I can remove this color decorator until someone finds a need for it. I'm fine with both options, with a slight preference for removing them. My own use-case involves $PS1. That makes sense (indeed, the implementation of status hints was slightly inspired from what the bash prompt in contrib/completion/git-prompt.sh does). The next step could be to use your porcelain there instead of checking manually file existance. You may want to add a short note about this motivation in the commit message. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-status: show short sequencer state
Phil Hord ho...@cisco.com writes: + merge a git-merge is in progress + am a git-am is in progress + rebase a git-rebase is in progress + rebase-interactive a git-rebase--interactive is in progress + cherry-picka git-cherry-pick is in progress + bisect a git-bisect is in progress Avoid using git-foo syntax in documentation, it suggests that this is valid command, which isn't true anymore. `git foo` seems the most common syntax. Also, git-rebase--interactive is not user-visible = `git rebase --interactive`. - if (state-am_empty_patch) + if (state-substate==WT_SUBSTATE_NOMINAL) status_printf_ln(s, color, _(The current patch is empty.)); This looks weird. First, spaces around == (here and below). Then, the logic is unintuitive. The if suggests everything is allright, and the message below is very specific. This at least deserves a comment. if (advice_status_hints) { - if (!state-am_empty_patch) + if (state-substate==WT_SUBSTATE_CONFLICTED) Spaces around ==. +static void wt_print_token(struct wt_status *s, const char *color, const char *token) +{ + color_fprintf(s-fp, color, %s, token); + fputc(s-null_termination ? '\0' : '\n', s-fp); +} The output format seems to be meant only for machine-consumption. Is there any case when we'd want color? I'd say we can disable coloring completely for this format (normally, color=auto does the right thing, but I prefer being 100% sure I'll get no color when writing scripts) +static void wt_shortstatus_print_sequencer(struct wt_status *s) [...] +void wt_sequencer_print(struct wt_status *s) +{ + wt_shortstatus_print_sequencer(s); +} + Why do you need this trivial wrapper? Other than that, I like the idea (although I have no concrete use-case in mind), but I didn't actually test the patch. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-status: show short sequencer state
Matthieu Moy wrote: Phil Hord ho...@cisco.com writes: +merge a git-merge is in progress +am a git-am is in progress +rebase a git-rebase is in progress +rebase-interactive a git-rebase--interactive is in progress +cherry-picka git-cherry-pick is in progress +bisect a git-bisect is in progress Avoid using git-foo syntax in documentation, it suggests that this is valid command, which isn't true anymore. `git foo` seems the most common syntax. Also, git-rebase--interactive is not user-visible = `git rebase --interactive`. Thanks. -if (state-am_empty_patch) +if (state-substate==WT_SUBSTATE_NOMINAL) status_printf_ln(s, color, _(The current patch is empty.)); This looks weird. First, spaces around == (here and below). Then, the logic is unintuitive. The if suggests everything is allright, and the message below is very specific. This at least deserves a comment. Yes, I agree. It was less clear but more reasonable before I tried to clear it up some. It's driven by the short-token printer. The state is you're in a 'git am' but I do not see any conflicted files. Therefore, your patch must be empty. I suspect this may be a leaving from the git-am machinery where it did not choose to clean up after itself if the patch was empty, but perhaps it should have. I seldom use git-am, so I do not know if this code reflects a common and useful state. I'll try to make this more explicit. Currently the short-status version will say either am or am \n conflicted when a 'git am' is in progress. The logical path to follow if I re-add 'git-am-empty' state tracker is for this to now show either am \n am-is-empty or am \n conflicted. But I think I should suppress the am-is-empty report in that case. What do you think if (advice_status_hints) { -if (!state-am_empty_patch) +if (state-substate==WT_SUBSTATE_CONFLICTED) Spaces around ==. +static void wt_print_token(struct wt_status *s, const char *color, const char *token) +{ +color_fprintf(s-fp, color, %s, token); +fputc(s-null_termination ? '\0' : '\n', s-fp); +} The output format seems to be meant only for machine-consumption. Is there any case when we'd want color? I'd say we can disable coloring completely for this format (normally, color=auto does the right thing, but I prefer being 100% sure I'll get no color when writing scripts) Originally I had this output nested in the normal 'git status --short' output, like a shortened form of the advice. Then, 'git-status --porcelain' would show the tokens without color, but 'git status --short' would show them with color. I thought I might be going back there, or that I might combine this with full 'git status' again somehow, and colors seemed appropriate still. The --short status report is too confusing when tokens may or may-not appear, and it would likely break some scripts, even though they should be using --porcelain. And the full status already has its long versions of the same text. So I can remove this color decorator until someone finds a need for it. +static void wt_shortstatus_print_sequencer(struct wt_status *s) [...] +void wt_sequencer_print(struct wt_status *s) +{ +wt_shortstatus_print_sequencer(s); +} + Why do you need this trivial wrapper? Another left-over from its previous multiple versions. I'll simplify it. Other than that, I like the idea (although I have no concrete use-case in mind), but I didn't actually test the patch. My own use-case involves $PS1. I keep running into you cannot cherry-pick because you are in the middle of a rebase in submodules in which I have long forgotten about the failed action and have gone on to write many new commits, or switched branches, or worse. I do not know what 'git rebase --abort' will give me in those cases, and I wonder what work I might have lost for having been interrupted in the middle of that action in the past. These tokens will help me decorate my prompt to remind me I left some baggage untended. Thanks for the feedback. Phil -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html