Re: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
On Wed, Aug 28, 2013 at 04:18:03PM -0400, Jeff King wrote: > On Wed, Aug 28, 2013 at 01:05:38PM -0700, Junio C Hamano wrote: > > > What are our plans to help existing scripts people have written over > > time, especially before "status -s" was invented, that will be > > broken by use of this? > > I thought that our response to parsing the long output of "git status" > was always "you are doing it wrong". The right way has always been to > run the plumbing tools yourself, followed eventually by the --porcelain > mode to "git status" being blessed as a convenient plumbing. > > I will not say that people might not do it anyway, but at what point do > we say "you were warned"? It already has changed. At cPanel, we had code that broke with a new version of git because the output of git status changed between 1.7.11 and 1.8.3. We fixed it to use --porcelain and had no problems. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
David Aguilar writes: > I have a poor imagination and cannot imagine why it needs to be > switchable. I could not either, but I found the reason in the commit message: eff80a9fd990 Some users do want to write a line that begin with a pound sign, #, in their commit log message. Many tracking system recognise a token of # form, for example. The support we offer these use cases is not very friendly to the end users. They have a choice between - Don't do it. Avoid such a line by rewrapping or indenting; and - Use --cleanup=whitespace but remove all the hint lines we add. Give them a way to set a custom comment char, e.g. $ git -c core.commentchar="%" commit so that they do not have to do either of the two workarounds. I personnally think allowing an escape scheme (\#) would have been better. But as Junio said, it's too late. My change is not about commentchar customizability, but about disabling the comment in "git status". -- 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: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
Jonathan Nieder writes:
> Matthieu Moy wrote:
>
>>In the long run, if users
>> like the non-prefix output, it may make sense to flip the default value
>> to true.
>
> Hmm, do you mean that the configuration is to give the change-averse
> some time to get used to the new output? I think it would make sense
> to do it all at once (and even think that that would be less
> confusing).
The actual reason was that I initially thought the change would be much
more controversial. I wanted to live with the option on for some time to
see how much I liked it, and allow others to do the same without forcing
the change.
If everybody agree that status it better without the comment, and that
the switch does not need a config option, then I'm fine with dropping
the config option.
We can also keep the config option, but document it as a transitional
option only ("the behavior changed in Git XXX, to keep the old behavior,
set this to YYY"). Then it could be status.oldStyle instead.
--
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: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
Matthieu Moy wrote: > Historically, "git status" needed to prefix each output line with '#' so > that the output could be added as comment to the commit message. This > prefix comment has no real purpose when "git status" is ran from the > command-line, and this may distract users from the real content. > > Allow the user to disable this prefix comment. Why not do this unconditionally? >In the long run, if users > like the non-prefix output, it may make sense to flip the default value > to true. Hmm, do you mean that the configuration is to give the change-averse some time to get used to the new output? I think it would make sense to do it all at once (and even think that that would be less confusing). The rest of the idea and execution seem sane. Thanks, Jonathan -- 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: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
David Aguilar writes: > On Wed, Aug 28, 2013 at 2:39 PM, Junio C Hamano wrote: >> Jeff King writes: >> >>> On Wed, Aug 28, 2013 at 01:05:38PM -0700, Junio C Hamano wrote: >>> What are our plans to help existing scripts people have written over time, especially before "status -s" was invented, that will be broken by use of this? >>> >>> I thought that our response to parsing the long output of "git status" >>> was always "you are doing it wrong". The right way has always been to >>> run the plumbing tools yourself, followed eventually by the --porcelain >>> mode to "git status" being blessed as a convenient plumbing. >>> >>> I will not say that people might not do it anyway, but at what point do >>> we say "you were warned"? >> >> OK, sounds good enough. > > I personally think it's a little strange for this to be configurable. > > I have a poor imagination and cannot imagine why it needs to be > switchable. If someone switches it to "a" does that mean that any > commit message line that starts with the letter "a" will be filtered > out? > > Specifically, core.commentchar seems really unnecessary to me. What > is the benefit? > > I do see downsides -- folks do parse the output, they don't read the > release notes, they don't know any better, but, hey, "it works", so > they'll be broken just because someone doesn't like "#"? > > What about hooks that write custom commit messages? Do they need to > start caring about core.commentchar? They are valid questions, I think, but are best asked to those who wanted core.commentchar configuration, not to people involved in this thread. Also unfortunately it is too late by two releases to ask that question. -- 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: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
On Wed, Aug 28, 2013 at 2:39 PM, Junio C Hamano wrote: > Jeff King writes: > >> On Wed, Aug 28, 2013 at 01:05:38PM -0700, Junio C Hamano wrote: >> >>> What are our plans to help existing scripts people have written over >>> time, especially before "status -s" was invented, that will be >>> broken by use of this? >> >> I thought that our response to parsing the long output of "git status" >> was always "you are doing it wrong". The right way has always been to >> run the plumbing tools yourself, followed eventually by the --porcelain >> mode to "git status" being blessed as a convenient plumbing. >> >> I will not say that people might not do it anyway, but at what point do >> we say "you were warned"? > > OK, sounds good enough. I personally think it's a little strange for this to be configurable. I have a poor imagination and cannot imagine why it needs to be switchable. If someone switches it to "a" does that mean that any commit message line that starts with the letter "a" will be filtered out? Specifically, core.commentchar seems really unnecessary to me. What is the benefit? I do see downsides -- folks do parse the output, they don't read the release notes, they don't know any better, but, hey, "it works", so they'll be broken just because someone doesn't like "#"? What about hooks that write custom commit messages? Do they need to start caring about core.commentchar? Curious, -- David -- 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: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
Matthieu Moy writes: > Junio C Hamano writes: > >> Matthieu Moy writes: >> >>> diff --git a/Documentation/config.txt b/Documentation/config.txt >>> index ec57a15..dacf4b9 100644 >>> --- a/Documentation/config.txt >>> +++ b/Documentation/config.txt >>> @@ -2118,6 +2118,11 @@ status.branch:: >>> Set to true to enable --branch by default in linkgit:git-status[1]. >>> The option --no-branch takes precedence over this variable. >>> >>> +status.displayCommentChar:: >>> + If set to false, linkgit:git-status[1] will not prefix each >>> + line with the comment char (`core.commentchar`, i.e. `#` by >>> + default). Defaults to true. >> >> We prefix core.commentchar followed by a single space for non-empty >> lines; is that worth mentioning, I wonder? > > (and sometimes # is followed by a tab) > > I don't think it's worth the trouble. The goal is not to document the > format precisely, but to explain the user how to use the variable. > >> Also I envision that we would extend core.commentchar to be more >> than a single character. Is "displayCommentChar" still a good name >> for this setting? > > It is as good as core.commentChar ;-). Not really; taken together with "# has a space after it", calling it "prefix" in the mechanism to omit it makes tons of sense. >> What are our plans to help existing scripts people have written over >> time, especially before "status -s" was invented, that will be >> broken by use of this? > > I don't know what's the best plan, but I think any sensible plan starts > by waiting for a few releases, so that Git version not implementing > status.displayCommentChar become rare enough. So we can delay the actual > plan until next year I guess. Actually as Peff pointed out, we've already told number of times that "status" output is for human consumption and scripts should not attempt to parse it, long before we gave them -s/--porcelain options, so we are good without "auto". -- 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: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
Matthieu Moy writes: > diff --git a/wt-status.c b/wt-status.c > index cb24f1f..97068d5 100644 > --- a/wt-status.c > +++ b/wt-status.c > @@ -774,12 +778,21 @@ static void wt_status_print_tracking(struct wt_status > *s) > if (!format_tracking_info(branch, &sb)) > return; > > + char comment_line_string[3]; > + int i = 0; decl-after-statement; queued with a fix to be squashed in. -- 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: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
Jeff King writes: > On Wed, Aug 28, 2013 at 01:05:38PM -0700, Junio C Hamano wrote: > >> What are our plans to help existing scripts people have written over >> time, especially before "status -s" was invented, that will be >> broken by use of this? > > I thought that our response to parsing the long output of "git status" > was always "you are doing it wrong". The right way has always been to > run the plumbing tools yourself, followed eventually by the --porcelain > mode to "git status" being blessed as a convenient plumbing. > > I will not say that people might not do it anyway, but at what point do > we say "you were warned"? OK, sounds good enough. -- 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: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
Junio C Hamano writes:
> Matthieu Moy writes:
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index ec57a15..dacf4b9 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -2118,6 +2118,11 @@ status.branch::
>> Set to true to enable --branch by default in linkgit:git-status[1].
>> The option --no-branch takes precedence over this variable.
>>
>> +status.displayCommentChar::
>> +If set to false, linkgit:git-status[1] will not prefix each
>> +line with the comment char (`core.commentchar`, i.e. `#` by
>> +default). Defaults to true.
>
> We prefix core.commentchar followed by a single space for non-empty
> lines; is that worth mentioning, I wonder?
(and sometimes # is followed by a tab)
I don't think it's worth the trouble. The goal is not to document the
format precisely, but to explain the user how to use the variable.
> Also I envision that we would extend core.commentchar to be more
> than a single character. Is "displayCommentChar" still a good name
> for this setting?
It is as good as core.commentChar ;-).
I chose the variable name to remind commentChar (after finding
commentChar, one can grep it and find status.displayCommentChar). I tend
to think that it's better than omitCommentPrefix, but I can change is
people think it's better.
> What are our plans to help existing scripts people have written over
> time, especially before "status -s" was invented, that will be
> broken by use of this?
I don't know what's the best plan, but I think any sensible plan starts
by waiting for a few releases, so that Git version not implementing
status.displayCommentChar become rare enough. So we can delay the actual
plan until next year I guess.
That said, I had an idea that may help the transition: allow "auto" as a
value, just like we did for colors. A patch implementing this (on top of
the previous series) is below. Good point: "auto" is a safe value, as it
won't break scripts. There is one drawback, though: "auto" is not a
really good default value in the long run, since newcommers may wonder
why there are differences between "git status" and "git status | cat".
I still find this tempting, and it would make the transition so much
easier. I would even argue to flip the default to "auto" for Git 2.0
then (after all, we didn't even wait for this to change color.ui).
Maybe, later, a transition from "auto" to "never" could be done. Or
perhaps it's not so terrible to keep auto (my "ls" and "ls | cat"
produce very different outputs, and it never bugged me).
diff --git a/builtin/commit.c b/builtin/commit.c
index d4cfced..00e9487 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -163,6 +163,21 @@ static void determine_whence(struct wt_status *s)
s->whence = whence;
}
+static void determine_comment_mode(struct wt_status *s)
+{
+ if (s->display_comment_char == COMMENT_AUTO) {
+ /*
+* determine_comment_mode is used only for cmd_status,
+* which always prints to stdout.
+*/
+ if (isatty(1) || pager_in_use()) {
+ s->display_comment_char = COMMENT_NEVER;
+ } else {
+ s->display_comment_char = COMMENT_ALWAYS;
+ }
+ }
+}
+
static void rollback_index_files(void)
{
switch (commit_style) {
@@ -1183,6 +1198,20 @@ static int git_status_config(const char *k, const char
*v, void *cb)
return 0;
}
if (!strcmp(k, "status.displaycommentchar")) {
+ if (v) {
+ if (!strcasecmp(v, "never")) {
+ s->display_comment_char = COMMENT_NEVER;
+ return 0;
+ }
+ if (!strcasecmp(v, "always")) {
+ s->display_comment_char = COMMENT_ALWAYS;
+ return 0;
+ }
+ if (!strcasecmp(v, "auto")) {
+ s->display_comment_char = COMMENT_AUTO;
+ return 0;
+ }
+ }
s->display_comment_char = git_config_bool(k, v);
return 0;
}
@@ -1254,6 +1283,7 @@ int cmd_status(int argc, const char **argv, const char
*prefix)
gitmodules_config();
git_config(git_status_config, &s);
determine_whence(&s);
+ determine_comment_mode(&s);
argc = parse_options(argc, argv, prefix,
builtin_status_options,
builtin_status_usage, 0);
@@ -1504,7 +1534,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
* Ignore status.displayCommentChar: we do need comments in
* COMMIT_EDITMSG.
*/
- s.display_comment_char = 1;
+ s.display_comment_char = COMMENT_ALWAYS;
determine_w
Re: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
On Wed, Aug 28, 2013 at 01:05:38PM -0700, Junio C Hamano wrote: > What are our plans to help existing scripts people have written over > time, especially before "status -s" was invented, that will be > broken by use of this? I thought that our response to parsing the long output of "git status" was always "you are doing it wrong". The right way has always been to run the plumbing tools yourself, followed eventually by the --porcelain mode to "git status" being blessed as a convenient plumbing. I will not say that people might not do it anyway, but at what point do we say "you were warned"? -Peff -- 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: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
Junio C Hamano writes: > "status.omitCommentPrefix" that defaults to false might be a better > setting, I suspect. Or status.showCommentPrefix that defaults to true; I didn't mean to say a setting that defaults to false is preferred over one that defaults to true in my message. -- 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: [RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
Matthieu Moy writes:
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ec57a15..dacf4b9 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2118,6 +2118,11 @@ status.branch::
> Set to true to enable --branch by default in linkgit:git-status[1].
> The option --no-branch takes precedence over this variable.
>
> +status.displayCommentChar::
> + If set to false, linkgit:git-status[1] will not prefix each
> + line with the comment char (`core.commentchar`, i.e. `#` by
> + default). Defaults to true.
We prefix core.commentchar followed by a single space for non-empty
lines; is that worth mentioning, I wonder?
Also I envision that we would extend core.commentchar to be more
than a single character. Is "displayCommentChar" still a good name
for this setting?
"status.omitCommentPrefix" that defaults to false might be a better
setting, I suspect. I further suspect that in the longer term, we
may want to consider flipping its default to true (for Git 2.0, it
is too late). I do not think we need these comment prefix in the
human readable "status" output---after all, there is no line that is
not commented (other than "nothing added to commit" at the end) in
the current output, so the comment prefix only wastes the screen
real estate.
What are our plans to help existing scripts people have written over
time, especially before "status -s" was invented, that will be
broken by use of this?
> @@ -663,17 +666,18 @@ static void wt_status_print_submodule_summary(struct
> wt_status *s, int uncommitt
> char summary_limit[64];
> char index[PATH_MAX];
> const char *env[] = { NULL, NULL };
> - const char *argv[8];
> + const char *argv[9];
>
> env[0] =index;
> argv[0] = "submodule";
> argv[1] = "summary";
> argv[2] = uncommitted ? "--files" : "--cached";
> argv[3] = "--for-status";
> - argv[4] = "--summary-limit";
> - argv[5] = summary_limit;
> - argv[6] = uncommitted ? NULL : (s->amend ? "HEAD^" : "HEAD");
> - argv[7] = NULL;
> + argv[4] = s->display_comment_char ? "--display-comment-char" :
> "--no-display-comment-char";
> + argv[5] = "--summary-limit";
> + argv[6] = summary_limit;
> + argv[7] = uncommitted ? NULL : (s->amend ? "HEAD^" : "HEAD");
> + argv[8] = NULL;
Not strictly your fault, but we should lose these funny indent after
"=" and use argv_array API here.
--
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
[RFC/PATCH v2 3/3] status: introduce status.displayCommentChar to disable display of #
Historically, "git status" needed to prefix each output line with '#' so
that the output could be added as comment to the commit message. This
prefix comment has no real purpose when "git status" is ran from the
command-line, and this may distract users from the real content.
Allow the user to disable this prefix comment. In the long run, if users
like the non-prefix output, it may make sense to flip the default value
to true.
Obviously, status.displayCommentChar applies to "git status" but is
ignored by "git commit", so the status is still commented in
COMMIT_EDITMSG.
Signed-off-by: Matthieu Moy
---
Change since v1:
* removed perl -pi use.
* test and fix submodule summary.
Documentation/config.txt | 5 +
builtin/commit.c | 9 +
t/t7508-status.sh| 43 +++
wt-status.c | 37 +
wt-status.h | 1 +
5 files changed, 83 insertions(+), 12 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ec57a15..dacf4b9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2118,6 +2118,11 @@ status.branch::
Set to true to enable --branch by default in linkgit:git-status[1].
The option --no-branch takes precedence over this variable.
+status.displayCommentChar::
+ If set to false, linkgit:git-status[1] will not prefix each
+ line with the comment char (`core.commentchar`, i.e. `#` by
+ default). Defaults to true.
+
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/builtin/commit.c b/builtin/commit.c
index 10acc53..d4cfced 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1182,6 +1182,10 @@ static int git_status_config(const char *k, const char
*v, void *cb)
s->use_color = git_config_colorbool(k, v);
return 0;
}
+ if (!strcmp(k, "status.displaycommentchar")) {
+ s->display_comment_char = git_config_bool(k, v);
+ return 0;
+ }
if (!prefixcmp(k, "status.color.") || !prefixcmp(k, "color.status.")) {
int slot = parse_status_slot(k, 13);
if (slot < 0)
@@ -1496,6 +1500,11 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
gitmodules_config();
git_config(git_commit_config, &s);
status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
+ /*
+* Ignore status.displayCommentChar: we do need comments in
+* COMMIT_EDITMSG.
+*/
+ s.display_comment_char = 1;
determine_whence(&s);
s.colopts = 0;
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index ac3d0fe..d0d7c4a 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -113,6 +113,39 @@ test_expect_success 'status (2)' '
test_i18ncmp expect output
'
+strip_comments () {
+ sed "s/^\# //; s/^\#$//; s/^#\t/\t/" <"$1" >"$1".tmp &&
+ rm "$1" && mv "$1".tmp "$1"
+}
+
+test_expect_success 'status with status.displayCommentChar=false' '
+ strip_comments expect &&
+ git -c status.displayCommentChar=false status >output &&
+ test_i18ncmp expect output
+'
+
+test_expect_success 'setup fake editor' '
+ cat >.git/editor <<-\EOF &&
+ #! /bin/sh
+ cp "$1" output
+EOF
+ chmod 755 .git/editor
+'
+
+commit_template_commented () {
+ (
+ EDITOR=.git/editor &&
+ export EDITOR &&
+ # Fails due to empty message
+ test_must_fail git commit
+ ) &&
+ ! grep '^[^#]' output
+}
+
+test_expect_success 'commit ignores status.displayCommentChar=false in
COMMIT_EDITMSG' '
+ commit_template_commented
+'
+
cat >expect <<\EOF
# On branch master
# Changes to be committed:
@@ -872,6 +905,16 @@ test_expect_success 'status submodule summary' '
test_i18ncmp expect output
'
+test_expect_success 'status submodule summary with
status.displayCommentChar=false' '
+ strip_comments expect &&
+ git -c status.displayCommentChar=false status >output &&
+ test_i18ncmp expect output
+'
+
+test_expect_success 'commit with submodule summary ignores displayCommentChar'
'
+ commit_template_commented
+'
+
cat >expect fp, color, &sb);
if (trail)
fprintf(s->fp, "%s", trail);
@@ -58,7 +60,7 @@ static void status_vprintf(struct wt_status *s, int at_bol,
const char *color,
eol = strchr(line, '\n');
strbuf_reset(&linebuf);
- if (at_bol) {
+ if (at_bol && s
