* [PATCH/RFC] commit-template: improve readability of commit template @ 2017-06-26 17:24 Kaartic Sivaraam 2017-06-26 21:59 ` Junio C Hamano 0 siblings, 1 reply; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-26 17:24 UTC (permalink / raw) To: gitster; +Cc: git * Previously the commit template didn't separate the distinct messages shown. This resulted in difficulty in interpreting it's content. Add new lines to separate the distinct parts of the template. * Previously the warning about usage of explicit paths without any options wasn't clear. Make it more clear so user gets what it's trying to say. Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> --- I've tried to improve the message specified in the commit. Hope it works correctly. Local test passed. builtin/commit.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 8d1cac062..0a5676b76 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -841,9 +841,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix, "with '%c' will be kept; you may remove them" " yourself if you want to.\n" "An empty message aborts the commit.\n"), comment_line_char); - if (only_include_assumed) + if (only_include_assumed) { + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); // Add new line for clarity status_printf_ln(s, GIT_COLOR_NORMAL, "%s", only_include_assumed); + } /* * These should never fail because they come from our own @@ -877,8 +879,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, (int)(ci.name_end - ci.name_begin), ci.name_begin, (int)(ci.mail_end - ci.mail_begin), ci.mail_begin); - if (ident_shown) - status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); // Add new line for clarity saved_color_setting = s->use_color; s->use_color = 0; @@ -1209,7 +1210,7 @@ static int parse_and_validate_options(int argc, const char *argv[], if (argc == 0 && (also || (only && !amend && !allow_empty))) die(_("No paths with --include/--only does not make sense.")); if (argc > 0 && !also && !only) - only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths..."); + only_include_assumed = _("Explicit paths (<paths>) specified without -i or -o; assuming --only <paths>"); if (!cleanup_arg || !strcmp(cleanup_arg, "default")) cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE; else if (!strcmp(cleanup_arg, "verbatim")) -- 2.11.0 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH/RFC] commit-template: improve readability of commit template 2017-06-26 17:24 [PATCH/RFC] commit-template: improve readability of commit template Kaartic Sivaraam @ 2017-06-26 21:59 ` Junio C Hamano 2017-06-27 17:22 ` Kaartic Sivaraam 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2017-06-26 21:59 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes: > * Previously the commit template didn't separate the > distinct messages shown. This resulted in difficulty > in interpreting it's content. Add new lines to separate > the distinct parts of the template. > > * Previously the warning about usage of explicit paths > without any options wasn't clear. Make it more clear > so user gets what it's trying to say. > We don't usually make a bullet list in log message. Please stick to a plain prose. "Previously" is superflous. Say what it does (e.g. "The commit template adds optional parts without extra blank lines to its normal output") in present tense and explain the ramifications of it (e.g. "I personally find that this makes it harder to find the optional bit"). > Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> > --- > I've tried to improve the message specified in the commit. Hope > it works correctly. > > Local test passed. Perhaps you would want to ensure that this change (if you find it valuable) will not get broken by other people in the future by writing a new test that ensures that these extra blank lines are always there when you think they are needed? I personally do not find these new blank lines are necessary, and this change wastes vertical screen real estate which is a limited resource, but that may be just me. I on the other hand do not think the result of this patch is overly worse than the status quo, either. > builtin/commit.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 8d1cac062..0a5676b76 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -841,9 +841,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > "with '%c' will be kept; you may remove them" > " yourself if you want to.\n" > "An empty message aborts the commit.\n"), comment_line_char); > - if (only_include_assumed) > + if (only_include_assumed) { > + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); // Add new line for clarity > status_printf_ln(s, GIT_COLOR_NORMAL, > "%s", only_include_assumed); > + } We do not use // comment in most parts of our codebase that are supposed to be platform neutral (iow, compat/ is exempt). But more importantly, wouldn't if (only_include_assumed) status_printf_ln(s, GIT_COLOR_NORMAL, - "%s", only_include_asssumed); + "\n%s", only_include_asssumed); be sufficient? > @@ -877,8 +879,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > (int)(ci.name_end - ci.name_begin), ci.name_begin, > (int)(ci.mail_end - ci.mail_begin), ci.mail_begin); > > - if (ident_shown) > - status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); > + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); // Add new line for clarity This does ensure that an extra blank line appears after the optional section (either after the "only/include assumed" message, or "writing for somebody else" message). If we were to go with this sparser output, I think we also should give an extra blank line before and after the "HEAD detached from cafebabe" message you would see: $ git checkout HEAD^0 $ git commit --allow-empty -o or "On branch blah" if you are on a branch. I think your change adds a blank before, but it does not have a separation before "Changes not staged for commit" line. > saved_color_setting = s->use_color; > s->use_color = 0; > @@ -1209,7 +1210,7 @@ static int parse_and_validate_options(int argc, const char *argv[], > if (argc == 0 && (also || (only && !amend && !allow_empty))) > die(_("No paths with --include/--only does not make sense.")); > if (argc > 0 && !also && !only) > - only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths..."); > + only_include_assumed = _("Explicit paths (<paths>) specified without -i or -o; assuming --only <paths>"); I think "paths (<paths>)" is excessive. If you are using <token> to hint that they refer to "commit -h" or "commit --help" output, then Explicit <paths> specified without -i or -o; assumign --only <paths> should be sufficient. Having said that, to be quite honest, I think this "assuming --only" message outlived its usefulness. This was necessary in very early days of Git because originally "git commit foo" did "git add foo && git commit" (i.e. "-i" was the default) and then later when we made "--only" the new default in order to match everybody else's SCM, we needed to remind users of older versions of Git that "git commit foo" now means "git commit --only foo", not "git commit -i foo" which they may have been used to. These days, hopefully nobody expects the "-i" semantics when they do "git commit foo", so perhaps it may be a better change to _remove_ the message altogether. And with that done, I wouldn't have reservations on this change (i.e. "is it worth wasting extra screen real estate, especially in the vertical direction?"), as instead of wasting 2 lines to give a message that is no longer useful in today's world, it will be removing one line ;-) Thanks. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH/RFC] commit-template: improve readability of commit template 2017-06-26 21:59 ` Junio C Hamano @ 2017-06-27 17:22 ` Kaartic Sivaraam 2017-06-27 17:56 ` Junio C Hamano 0 siblings, 1 reply; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-27 17:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Mon, 2017-06-26 at 14:59 -0700, Junio C Hamano wrote: > We don't usually make a bullet list in log message. Please stick to > a plain prose. > > "Previously" is superflous. Say what it does (e.g. "The commit > template adds optional parts without extra blank lines to its normal > output") in present tense and explain the ramifications of it > (e.g. "I personally find that this makes it harder to find the > optional bit"). > Corrected it. > Perhaps you would want to ensure that this change (if you find it > valuable) will not get broken by other people in the future by > writing a new test that ensures that these extra blank lines are > always there when you think they are needed? > See comment below. > I personally do not find these new blank lines are necessary, and > this change wastes vertical screen real estate which is a limited > resource, but that may be just me. I on the other hand do not think > the result of this patch is overly worse than the status quo, either. > I thought it's not good to trade-off readability for vertical space as the ultimate aim of the commit template (at least to me) is to convey information to the user about the commit that he's going to make. For which, I thought it made more sense to improve it's readability by adding new lines between different sections rather than constrain the output within a few lines. In case it's not worth it, I'll revert it back which isn't that big an issue, anyway. In case it's not suggested to revert this back, I'll add the tests. > We do not use // comment in most parts of our codebase that are > supposed to be platform neutral (iow, compat/ is exempt). > It seems to be a result of my ignorance that C allowed '//' comments only recently. > > @@ -877,8 +879,7 @@ static int prepare_to_commit(const char > > *index_file, const char *prefix, > > (int)(ci.name_end - > > ci.name_begin), ci.name_begin, > > (int)(ci.mail_end - > > ci.mail_begin), ci.mail_begin); > > > > - if (ident_shown) > > - status_printf_ln(s, GIT_COLOR_NORMAL, > > "%s", ""); > > + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); > > // Add new line for clarity > > This does ensure that an extra blank line appears after the optional > section (either after the "only/include assumed" message, or "writing > for somebody else" message). > > If we were to go with this sparser output, I think we also should > give an extra blank line before and after the "HEAD detached from > cafebabe" message you would see: > > $ git checkout HEAD^0 > $ git commit --allow-empty -o > > or "On branch blah" if you are on a branch. I think your change > adds a blank before, but it does not have a separation before > "Changes not staged for commit" line. > I actually didn't think of modifying that in order to keep it in line with the output of `git status`. Further, to me, adding *this* new line before the "Changes not staged for commit" (or something in it's place) seems to be wasting some vertical space with an added drawback that it's not in line with `git status`. > Having said that, to be quite honest, I think this "assuming --only" > message outlive > d its usefulness. This was necessary in very early > days of Git because originally "git commit foo" did "git add foo && > git commit" (i.e. "-i" was the default) and then later when we made > "--only" the new default in order to match everybody else's SCM, we > needed to remind users of older versions of Git that "git commit foo" > now means "git commit --only foo", not "git commit -i foo" which they > may have been used to. These days, hopefully nobody expects the "-i" > semantics when they do "git commit foo", so perhaps it may be a > better > change to _remove_ the message altogether. > > And with that done, I wouldn't have reservations on this change > (i.e. "is it worth wasting extra screen real estate, especially in > the vertical direction?"), as instead of wasting 2 lines to give a > message that is no longer useful in today's world, it will be > removing one line ;-) > You're right! It is worth saving lines if it's not useful these days. I have sent another mail to the mailing list to get any other possible improvements to the status. http://public-inbox.org/git/1498583779.2737.4.camel@gmail.com/ -- Regards, Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH/RFC] commit-template: improve readability of commit template 2017-06-27 17:22 ` Kaartic Sivaraam @ 2017-06-27 17:56 ` Junio C Hamano 2017-06-28 13:04 ` Kaartic Sivaraam 2017-06-28 13:29 ` [PATCH] " Kaartic Sivaraam 0 siblings, 2 replies; 25+ messages in thread From: Junio C Hamano @ 2017-06-27 17:56 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes: >> I personally do not find these new blank lines are necessary, and >> this change wastes vertical screen real estate which is a limited >> resource, but that may be just me. I on the other hand do not think >> the result of this patch is overly worse than the status quo, either. >> > I thought it's not good to trade-off readability for vertical space as > the ultimate aim of the commit template (at least to me) is to convey > information to the user about the commit that he's going to make. For > which, I thought it made more sense to improve it's readability by > adding new lines between different sections rather than constrain the > output within a few lines. You have to be careful when making a trade-off argument. It depends on how familiar you already are with the presentation. Those who are/got used to the order of things that come, they will know there is extra information when the block of lines are longer than usual without reading every character and then their eyes are guided to read what is extra, without having to waste precious screen real estate. Nobody will _stay_ a new user who is not yet familiar with the everyday output. >> If we were to go with this sparser output, I think we also should >> give an extra blank line before and after the "HEAD detached from >> cafebabe" message you would see: >> >> $ git checkout HEAD^0 >> $ git commit --allow-empty -o >> >> or "On branch blah" if you are on a branch. I think your change >> adds a blank before, but it does not have a separation before >> "Changes not staged for commit" line. >> > I actually didn't think of modifying that in order to keep it in line > with the output of `git status`. I was (and still am) assuming that if we make this change to "git commit", we should make matching change to "git status" as a given. > Further, to me, adding *this* new line > before the "Changes not staged for commit" (or something in it's place) > seems to be wasting some vertical space ... I think it is in line with your original reasoning why you wanted these extra blank lines to separate blocks of different kinds of information: - "Please do this" instruction at the beginning - Make sure you know the default is --only, not --include - By the way you are committing for that person, not you - This change is being committed on that branch - Here are the changes that are already in the index - Here are the changes that are not in the index - Here are untracked files Lack of a blank between the fourth block and the fifth block [*1*] makes it somewhat inconsistent, doesn't it? [Footnote] *1* Yes, we should think about removing the optional second block, as I think that it outlived its usefulness; if we are to do so, these become the third and the fourth blocks. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH/RFC] commit-template: improve readability of commit template 2017-06-27 17:56 ` Junio C Hamano @ 2017-06-28 13:04 ` Kaartic Sivaraam 2017-06-28 14:50 ` Kaartic Sivaraam 2017-06-28 13:29 ` [PATCH] " Kaartic Sivaraam 1 sibling, 1 reply; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-28 13:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Tue, 2017-06-27 at 10:56 -0700, Junio C Hamano wrote: > Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes: > > I thought it's not good to trade-off readability for vertical space > > as > > the ultimate aim of the commit template (at least to me) is to > > convey > > information to the user about the commit that he's going to make. > > For > > which, I thought it made more sense to improve it's readability by > > adding new lines between different sections rather than constrain > > the > > output within a few lines. > > You have to be careful when making a trade-off argument. It depends > on how familiar you already are with the presentation. Those who > are/got used to the order of things that come, they will know there > is extra information when the block of lines are longer than usual > without reading every character and then their eyes are guided to > read what is extra, without having to waste precious screen real > estate. Nobody will _stay_ a new user who is not yet familiar with > the everyday output. > You're right. I didn't consider the fact that experienced users would be affected as a result of this change, sorry about that. I thought, making this change would help the new users who would possibly find the commit template to be congested and let experienced users to get accustomed to this new output format. I thought this change would be a win-win (at least after people get accustomed to the new formatting). In case screen real estate is considered more important here, no issues. I'll drop that part of the change, happily. > > I actually didn't think of modifying that in order to keep it in > > line > > with the output of `git status`. > > I was (and still am) assuming that if we make this change to "git > commit", we should make matching change to "git status" as a given. I get it now. In that case, I don't think making the change would be a good choice for the following reasons, * I think vertical spacing matters more in the output printed to a console. * I myself find it odd to add a new line below the branch information possibly because I'm too accustomed to it's current output. I tried adding the new line, it seemed to be too spacious. It might be just me in this case. > > Further, to me, adding *this* new line > > before the "Changes not staged for commit" (or something in it's > > place) > > seems to be wasting some vertical space ... > > I think it is in line with your original reasoning why you wanted > these extra blank lines to separate blocks of different kinds of > information: > > - "Please do this" instruction at the beginning > - Make sure you know the default is --only, not --include > - By the way you are committing for that person, not you > - This change is being committed on that branch > - Here are the changes that are already in the index > - Here are the changes that are not in the index > - Here are untracked files > > Lack of a blank between the fourth block and the fifth block [*1*] > makes it somewhat inconsistent, doesn't it? > It does, for the given set of blocks. I didn't find it inconsistent as I thought the separate blocks as follows, - "Please do this" instruction at the beginning - Make sure you know the default is --only, not --include - By the way you are committing for that person, not you - Status of repository (git status) > [Footnote] > > *1* Yes, we should think about removing the optional second block, > as I think that it outlived its usefulness; if we are to do so, > these become the third and the fourth blocks. If I interpreted your previous email correctly, I thought we were doing it! I'll send a "typical" patch as a follow-up of this mail. -- Regards, Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH/RFC] commit-template: improve readability of commit template 2017-06-28 13:04 ` Kaartic Sivaraam @ 2017-06-28 14:50 ` Kaartic Sivaraam 0 siblings, 0 replies; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-28 14:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: git I might have been ignorant about something about git in my reply in the previous email (found below). In that case, please enlighten me. On Wed, 2017-06-28 at 18:34 +0530, Kaartic Sivaraam wrote: > On Tue, 2017-06-27 at 10:56 -0700, Junio C Hamano wrote: > > Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes: > > > I thought it's not good to trade-off readability for vertical > > > space > > > as > > > the ultimate aim of the commit template (at least to me) is to > > > convey > > > information to the user about the commit that he's going to make. > > > For > > > which, I thought it made more sense to improve it's readability > > > by > > > adding new lines between different sections rather than constrain > > > the > > > output within a few lines. > > > > You have to be careful when making a trade-off argument. It > > depends > > on how familiar you already are with the presentation. Those who > > are/got used to the order of things that come, they will know there > > is extra information when the block of lines are longer than usual > > without reading every character and then their eyes are guided to > > read what is extra, without having to waste precious screen real > > estate. Nobody will _stay_ a new user who is not yet familiar with > > the everyday output. > > > > You're right. I didn't consider the fact that experienced users would > be affected as a result of this change, sorry about that. I thought, > making this change would help the new users who would possibly find > the > commit template to be congested and let experienced users to get > accustomed to this new output format. I thought this change would be > a > win-win (at least after people get accustomed to the new > formatting). > > In case screen real estate is considered more important here, no > issues. I'll drop that part of the change, happily. > > > > I actually didn't think of modifying that in order to keep it in > > > line > > > with the output of `git status`. > > > > I was (and still am) assuming that if we make this change to "git > > commit", we should make matching change to "git status" as a given. > > I get it now. In that case, I don't think making the change would be > a > good choice for the following reasons, > > * I think vertical spacing matters more in the output printed to > a > console. > * I myself find it odd to add a new line below the branch > information possibly because I'm too accustomed to it's current > output. > > I tried adding the new line, it seemed to be too spacious. It might > be > just me in this case. > > > > Further, to me, adding *this* new line > > > before the "Changes not staged for commit" (or something in it's > > > place) > > > seems to be wasting some vertical space ... > > > > I think it is in line with your original reasoning why you wanted > > these extra blank lines to separate blocks of different kinds of > > information: > > > > - "Please do this" instruction at the beginning > > - Make sure you know the default is --only, not --include > > - By the way you are committing for that person, not you > > - This change is being committed on that branch > > - Here are the changes that are already in the index > > - Here are the changes that are not in the index > > - Here are untracked files > > > > Lack of a blank between the fourth block and the fifth block [*1*] > > makes it somewhat inconsistent, doesn't it? > > > > It does, for the given set of blocks. I didn't find it inconsistent > as > I thought the separate blocks as follows, > > - "Please do this" instruction at the beginning > - Make sure you know the default is --only, not --include > - By the way you are committing for that person, not you > - Status of repository (git status) > > > [Footnote] > > > > *1* Yes, we should think about removing the optional second block, > > as I think that it outlived its usefulness; if we are to do so, > > these become the third and the fourth blocks. > > If I interpreted your previous email correctly, I thought we were > doing > it! > > I'll send a "typical" patch as a follow-up of this mail. > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] commit-template: improve readability of commit template 2017-06-27 17:56 ` Junio C Hamano 2017-06-28 13:04 ` Kaartic Sivaraam @ 2017-06-28 13:29 ` Kaartic Sivaraam 2017-06-28 14:47 ` Kaartic Sivaraam 2017-06-28 16:48 ` Junio C Hamano 1 sibling, 2 replies; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-28 13:29 UTC (permalink / raw) To: gitster; +Cc: git The commit template adds the optional parts without a new line to distinguish them. This results in difficulty in interpreting it's content. Add new lines to separate the distinct parts of the template. The warning about usage of 'explicit paths without any corresponding options' has outlived it's purpose of warning users about the usage '--only' as default rather than '--include'. Remove it. Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> --- builtin/commit.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 8d1cac062..22d17e6f2 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -139,7 +139,6 @@ static enum commit_whence whence; static int sequencer_in_use; static int use_editor = 1, include_status = 1; static int show_ignored_in_status, have_option_m; -static const char *only_include_assumed; static struct strbuf message = STRBUF_INIT; static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED; @@ -841,9 +840,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix, "with '%c' will be kept; you may remove them" " yourself if you want to.\n" "An empty message aborts the commit.\n"), comment_line_char); - if (only_include_assumed) - status_printf_ln(s, GIT_COLOR_NORMAL, - "%s", only_include_assumed); /* * These should never fail because they come from our own @@ -877,8 +873,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, (int)(ci.name_end - ci.name_begin), ci.name_begin, (int)(ci.mail_end - ci.mail_begin), ci.mail_begin); - if (ident_shown) - status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new line for clarity */ saved_color_setting = s->use_color; s->use_color = 0; @@ -1208,8 +1203,6 @@ static int parse_and_validate_options(int argc, const char *argv[], die(_("Only one of --include/--only/--all/--interactive/--patch can be used.")); if (argc == 0 && (also || (only && !amend && !allow_empty))) die(_("No paths with --include/--only does not make sense.")); - if (argc > 0 && !also && !only) - only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths..."); if (!cleanup_arg || !strcmp(cleanup_arg, "default")) cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE; else if (!strcmp(cleanup_arg, "verbatim")) -- 2.11.0 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] commit-template: improve readability of commit template 2017-06-28 13:29 ` [PATCH] " Kaartic Sivaraam @ 2017-06-28 14:47 ` Kaartic Sivaraam 2017-06-28 16:48 ` Junio C Hamano 1 sibling, 0 replies; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-28 14:47 UTC (permalink / raw) To: gitster; +Cc: git I forgot to add the RFC in the subject in a hurry. Please consider it's presence. -- Regards, Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] commit-template: improve readability of commit template 2017-06-28 13:29 ` [PATCH] " Kaartic Sivaraam 2017-06-28 14:47 ` Kaartic Sivaraam @ 2017-06-28 16:48 ` Junio C Hamano 2017-06-29 17:01 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Kaartic Sivaraam 1 sibling, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2017-06-28 16:48 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes: > The commit template adds the optional parts without > a new line to distinguish them. This results in > difficulty in interpreting it's content. Add new > lines to separate the distinct parts of the template. > > The warning about usage of 'explicit paths without > any corresponding options' has outlived it's purpose of > warning users about the usage '--only' as default rather > than '--include'. Remove it. > > Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> > --- > builtin/commit.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) I think I can agree with both of the changes, but this commit does two things that may be better done in two separate commits. If I were doing this patch, I probably would make [PATCH 1/2] about removing the only_include_assumed (which is all except for the hunk at -877,8) and then [PATCH 2/2] about giving a separating blank line before the "git status" output. > The warning about usage of 'explicit paths without > any corresponding options' has outlived it's purpose of > warning users about the usage '--only' as default rather > than '--include'. Remove it. With a bit more digging of the history: The notice that "git commit <paths>" default to "git commit --only <paths>" was there since 756e3ee0 ("Merge branch 'jc/commit'", 2006-02-14). Back then, existing users of Git expected the command doing "git commit --include <paths>", and after we changed the behaviour of the command to align with other people's "$scm commit <paths>", we added the text to help them transition their expectations. Remove the message that now has outlived its usefulness. perhaps. Thanks. > diff --git a/builtin/commit.c b/builtin/commit.c > index 8d1cac062..22d17e6f2 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -139,7 +139,6 @@ static enum commit_whence whence; > static int sequencer_in_use; > static int use_editor = 1, include_status = 1; > static int show_ignored_in_status, have_option_m; > -static const char *only_include_assumed; > static struct strbuf message = STRBUF_INIT; > > static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED; > @@ -841,9 +840,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > "with '%c' will be kept; you may remove them" > " yourself if you want to.\n" > "An empty message aborts the commit.\n"), comment_line_char); > - if (only_include_assumed) > - status_printf_ln(s, GIT_COLOR_NORMAL, > - "%s", only_include_assumed); > > /* > * These should never fail because they come from our own > @@ -877,8 +873,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > (int)(ci.name_end - ci.name_begin), ci.name_begin, > (int)(ci.mail_end - ci.mail_begin), ci.mail_begin); > > - if (ident_shown) > - status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); > + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new line for clarity */ > > saved_color_setting = s->use_color; > s->use_color = 0; > @@ -1208,8 +1203,6 @@ static int parse_and_validate_options(int argc, const char *argv[], > die(_("Only one of --include/--only/--all/--interactive/--patch can be used.")); > if (argc == 0 && (also || (only && !amend && !allow_empty))) > die(_("No paths with --include/--only does not make sense.")); > - if (argc > 0 && !also && !only) > - only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths..."); > if (!cleanup_arg || !strcmp(cleanup_arg, "default")) > cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE; > else if (!strcmp(cleanup_arg, "verbatim")) ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/2] commit-template: remove outdated notice about explicit paths 2017-06-28 16:48 ` Junio C Hamano @ 2017-06-29 17:01 ` Kaartic Sivaraam 2017-06-29 17:01 ` [PATCH 2/2] commit-template: add new line before status information Kaartic Sivaraam 2017-06-29 17:56 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Junio C Hamano 0 siblings, 2 replies; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-29 17:01 UTC (permalink / raw) To: gitster; +Cc: git The notice that "git commit <paths>" default to "git commit --only <paths>" was there since 756e3ee0 ("Merge branch 'jc/commit'", 2006-02-14). Back then, existing users of Git expected the command doing "git commit --include <paths>", and after we changed the behaviour of the command to align with other people's "$scm commit <paths>", we added the text to help them transition their expectations. Remove the message that now has outlived its usefulness. --- builtin/commit.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 8d1cac062..64701c8f4 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -139,7 +139,6 @@ static enum commit_whence whence; static int sequencer_in_use; static int use_editor = 1, include_status = 1; static int show_ignored_in_status, have_option_m; -static const char *only_include_assumed; static struct strbuf message = STRBUF_INIT; static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED; @@ -841,9 +840,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix, "with '%c' will be kept; you may remove them" " yourself if you want to.\n" "An empty message aborts the commit.\n"), comment_line_char); - if (only_include_assumed) - status_printf_ln(s, GIT_COLOR_NORMAL, - "%s", only_include_assumed); /* * These should never fail because they come from our own @@ -1208,8 +1204,6 @@ static int parse_and_validate_options(int argc, const char *argv[], die(_("Only one of --include/--only/--all/--interactive/--patch can be used.")); if (argc == 0 && (also || (only && !amend && !allow_empty))) die(_("No paths with --include/--only does not make sense.")); - if (argc > 0 && !also && !only) - only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths..."); if (!cleanup_arg || !strcmp(cleanup_arg, "default")) cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE; else if (!strcmp(cleanup_arg, "verbatim")) -- 2.11.0 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/2] commit-template: add new line before status information 2017-06-29 17:01 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Kaartic Sivaraam @ 2017-06-29 17:01 ` Kaartic Sivaraam 2017-06-29 17:51 ` Junio C Hamano 2017-06-29 18:17 ` Junio C Hamano 2017-06-29 17:56 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Junio C Hamano 1 sibling, 2 replies; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-29 17:01 UTC (permalink / raw) To: gitster; +Cc: git The commit template adds the optional parts without a new line to distinguish them. This results in difficulty in interpreting it's content, specifically for inexperienced users. Add new lines to separate the distinct parts of the template. --- I tried writing tests to ensure that the new line is added but as it seems to require checking multi-line, special options of grep were required to check. I tried the following, test_expect_success 'new line found before status message' ' ! (GIT_EDITOR="cat >editor-input" git commit) && grep -Pz "#\n# On branch" editor-input ' It worked well locally but seems to make the build with GETTEXT_POISON=YesPlease to fail. So, I removed it. Not sure how to write a good test for this change, sorry :( builtin/commit.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 64701c8f4..22d17e6f2 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -873,8 +873,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, (int)(ci.name_end - ci.name_begin), ci.name_begin, (int)(ci.mail_end - ci.mail_begin), ci.mail_begin); - if (ident_shown) - status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new line for clarity */ saved_color_setting = s->use_color; s->use_color = 0; -- 2.11.0 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] commit-template: add new line before status information 2017-06-29 17:01 ` [PATCH 2/2] commit-template: add new line before status information Kaartic Sivaraam @ 2017-06-29 17:51 ` Junio C Hamano 2017-06-29 18:17 ` Junio C Hamano 1 sibling, 0 replies; 25+ messages in thread From: Junio C Hamano @ 2017-06-29 17:51 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes: > The commit template adds the optional parts without > a new line to distinguish them. This results in > difficulty in interpreting it's content, specifically > for inexperienced users. > > Add new lines to separate the distinct parts of the > template. > --- > I tried writing tests to ensure that the new line is added > but as it seems to require checking multi-line, special > options of grep were required to check. I tried the following, > > test_expect_success 'new line found before status message' ' > ! (GIT_EDITOR="cat >editor-input" git commit) && > grep -Pz "#\n# On branch" editor-input > ' > > It worked well locally but seems to make the build with > GETTEXT_POISON=YesPlease to fail. So, I removed it. > Not sure how to write a good test for this change, sorry :( The above is a good way to capture the input to the editor, but the test with "grep -P" which is not portable would not work well. You however should be able to prepare an expected output with cat >expect <<\-EOF && ... expected contents to editor-input here ... EOF and do "test_i18ncmp expect editor-input" instead of "grep -P". ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] commit-template: add new line before status information 2017-06-29 17:01 ` [PATCH 2/2] commit-template: add new line before status information Kaartic Sivaraam 2017-06-29 17:51 ` Junio C Hamano @ 2017-06-29 18:17 ` Junio C Hamano 2017-06-30 3:19 ` Kaartic Sivaraam 1 sibling, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2017-06-29 18:17 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes: > The commit template adds the optional parts without > a new line to distinguish them. This results in > difficulty in interpreting it's content, specifically > for inexperienced users. > > Add new lines to separate the distinct parts of the > template. The rationale of this has changed in this final version, hasn't it, especially with the removal of the "include/only warning" bit? We used to add a blank line to separate the "we are committing for somebody else", which is an optional part, from the status output, only when we have the optional part. Now with your change, we unconditionally have a blank before the part that would have been shown by "git status" to separate the two parts out. > --- > I tried writing tests to ensure that the new line is added > but as it seems to require checking multi-line, special > options of grep were required to check. I tried the following, > > test_expect_success 'new line found before status message' ' > ! (GIT_EDITOR="cat >editor-input" git commit) && > grep -Pz "#\n# On branch" editor-input > ' > > It worked well locally but seems to make the build with > GETTEXT_POISON=YesPlease to fail. So, I removed it. > Not sure how to write a good test for this change, sorry :( > > builtin/commit.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 64701c8f4..22d17e6f2 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -873,8 +873,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > (int)(ci.name_end - ci.name_begin), ci.name_begin, > (int)(ci.mail_end - ci.mail_begin), ci.mail_begin); > > - if (ident_shown) > - status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); > + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new line for clarity */ > > saved_color_setting = s->use_color; > s->use_color = 0; ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] commit-template: add new line before status information 2017-06-29 18:17 ` Junio C Hamano @ 2017-06-30 3:19 ` Kaartic Sivaraam 0 siblings, 0 replies; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-30 3:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, 2017-06-29 at 11:17 -0700, Junio C Hamano wrote: > The rationale of this has changed in this final version, hasn't it, > especially with the removal of the "include/only warning" bit? > > We used to add a blank line to separate the "we are committing for > somebody else", which is an optional part, from the status output, > only when we have the optional part. Now with your change, we > unconditionally have a blank before the part that would have been > shown by "git status" to separate the two parts out. > Yes, modified the commit message accordingly hoping that's what you meant. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] commit-template: remove outdated notice about explicit paths 2017-06-29 17:01 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Kaartic Sivaraam 2017-06-29 17:01 ` [PATCH 2/2] commit-template: add new line before status information Kaartic Sivaraam @ 2017-06-29 17:56 ` Junio C Hamano 2017-06-30 3:18 ` Kaartic Sivaraam 1 sibling, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2017-06-29 17:56 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes: > The notice that "git commit <paths>" default to "git commit > --only <paths>" was there since 756e3ee0 ("Merge branch > 'jc/commit'", 2006-02-14). Back then, existing users of Git > expected the command doing "git commit --include <paths>", and > after we changed the behaviour of the command to align with > other people's "$scm commit <paths>", we added the text to help > them transition their expectations. Remove the message that now > has outlived its usefulness. > --- > builtin/commit.c | 6 ------ > 1 file changed, 6 deletions(-) When I said "I would have ... if I were doing this", I merely meant exactly that---as I weren't doing it, I left it up to you. But you did it the way anyways, which is very nice of you ;-). Looks good. Should we consider this signed-off by you? Thanks. > diff --git a/builtin/commit.c b/builtin/commit.c > index 8d1cac062..64701c8f4 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -139,7 +139,6 @@ static enum commit_whence whence; > static int sequencer_in_use; > static int use_editor = 1, include_status = 1; > static int show_ignored_in_status, have_option_m; > -static const char *only_include_assumed; > static struct strbuf message = STRBUF_INIT; > > static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED; > @@ -841,9 +840,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > "with '%c' will be kept; you may remove them" > " yourself if you want to.\n" > "An empty message aborts the commit.\n"), comment_line_char); > - if (only_include_assumed) > - status_printf_ln(s, GIT_COLOR_NORMAL, > - "%s", only_include_assumed); > > /* > * These should never fail because they come from our own > @@ -1208,8 +1204,6 @@ static int parse_and_validate_options(int argc, const char *argv[], > die(_("Only one of --include/--only/--all/--interactive/--patch can be used.")); > if (argc == 0 && (also || (only && !amend && !allow_empty))) > die(_("No paths with --include/--only does not make sense.")); > - if (argc > 0 && !also && !only) > - only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths..."); > if (!cleanup_arg || !strcmp(cleanup_arg, "default")) > cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE; > else if (!strcmp(cleanup_arg, "verbatim")) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] commit-template: remove outdated notice about explicit paths 2017-06-29 17:56 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Junio C Hamano @ 2017-06-30 3:18 ` Kaartic Sivaraam 2017-06-30 12:12 ` Kaartic Sivaraam 0 siblings, 1 reply; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-30 3:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, 2017-06-29 at 10:56 -0700, Junio C Hamano wrote: > When I said "I would have ... if I were doing this", I merely meant > exactly that---as I weren't doing it, I left it up to you. But you > did it the way anyways, which is very nice of you ;-). > It made a *lot* of sense, that's why. :) > Looks good. Should we consider this signed-off by you? > Yeah, forgot that. Will follow-up with a patch. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/2] commit-template: remove outdated notice about explicit paths 2017-06-30 3:18 ` Kaartic Sivaraam @ 2017-06-30 12:12 ` Kaartic Sivaraam 2017-06-30 12:12 ` [PATCH 2/2] commit-template: distinguish status information unconditionally Kaartic Sivaraam 2017-07-09 17:54 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Kaartic Sivaraam 0 siblings, 2 replies; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-30 12:12 UTC (permalink / raw) To: gitster; +Cc: git The notice that "git commit <paths>" default to "git commit --only <paths>" was there since 756e3ee0 ("Merge branch 'jc/commit'", 2006-02-14). Back then, existing users of Git expected the command doing "git commit --include <paths>", and after the behaviour of the command was changed to align with other people's "$scm commit <paths>", the text was added to help them transition their expectations. Remove the message that now has outlived its usefulness. Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> --- Only commit message has been changed. Modified it a little to remove the self referencing words that didn't seem to suit well with the commit's author (that's just me :)) builtin/commit.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 8d1cac062..64701c8f4 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -139,7 +139,6 @@ static enum commit_whence whence; static int sequencer_in_use; static int use_editor = 1, include_status = 1; static int show_ignored_in_status, have_option_m; -static const char *only_include_assumed; static struct strbuf message = STRBUF_INIT; static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED; @@ -841,9 +840,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix, "with '%c' will be kept; you may remove them" " yourself if you want to.\n" "An empty message aborts the commit.\n"), comment_line_char); - if (only_include_assumed) - status_printf_ln(s, GIT_COLOR_NORMAL, - "%s", only_include_assumed); /* * These should never fail because they come from our own @@ -1208,8 +1204,6 @@ static int parse_and_validate_options(int argc, const char *argv[], die(_("Only one of --include/--only/--all/--interactive/--patch can be used.")); if (argc == 0 && (also || (only && !amend && !allow_empty))) die(_("No paths with --include/--only does not make sense.")); - if (argc > 0 && !also && !only) - only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths..."); if (!cleanup_arg || !strcmp(cleanup_arg, "default")) cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE; else if (!strcmp(cleanup_arg, "verbatim")) -- 2.11.0 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/2] commit-template: distinguish status information unconditionally 2017-06-30 12:12 ` Kaartic Sivaraam @ 2017-06-30 12:12 ` Kaartic Sivaraam 2017-06-30 14:52 ` Junio C Hamano 2017-07-09 17:54 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Kaartic Sivaraam 1 sibling, 1 reply; 25+ messages in thread From: Kaartic Sivaraam @ 2017-06-30 12:12 UTC (permalink / raw) To: gitster; +Cc: git The commit template adds the status information without adding a new line to distinguish them in the absence of optional parts. This results in difficulty in interpreting it's content, specifically for inexperienced users. Unconditionally, add new lines to separate the status message from the other parts of the commit-template to make it more readable. Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> --- In trying to make the test independent of the previous test, it got a bit long. Hope it's not doing anything that shouldn't be done. In case you would wonder, the 'git reset' line is to remove the files that were staged in previous test. I've done it in the same motive that lead to a bigger test. travis-ci builds passed builtin/commit.c | 3 +-- t/t7500-commit.sh | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 64701c8f4..22d17e6f2 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -873,8 +873,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, (int)(ci.name_end - ci.name_begin), ci.name_begin, (int)(ci.mail_end - ci.mail_begin), ci.mail_begin); - if (ident_shown) - status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new line for clarity */ saved_color_setting = s->use_color; s->use_color = 0; diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh index 116885a26..5739d3ed2 100755 --- a/t/t7500-commit.sh +++ b/t/t7500-commit.sh @@ -329,4 +329,27 @@ test_expect_success 'invalid message options when using --fixup' ' test_must_fail git commit --fixup HEAD~1 -F log ' +cat >expected-template <<EOF + +# Please enter the commit message for your changes. Lines starting +# with '#' will be ignored, and an empty message aborts the commit. +# +# Author: A U Thor <author@example.com> +# +# On branch commit-template-check +# Changes to be committed: +# new file: commit-template-check +# +# Untracked files not listed +EOF + +test_expect_success 'new line found before status message in commit template' ' + git checkout -b commit-template-check && + git reset --hard HEAD && + touch commit-template-check && + git add commit-template-check && + GIT_EDITOR="cat >editor-input" git commit --untracked-files=no --allow-empty-message && + test_i18ncmp expected-template editor-input +' + test_done -- 2.11.0 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] commit-template: distinguish status information unconditionally 2017-06-30 12:12 ` [PATCH 2/2] commit-template: distinguish status information unconditionally Kaartic Sivaraam @ 2017-06-30 14:52 ` Junio C Hamano 2017-07-01 1:59 ` Kaartic Sivaraam 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2017-06-30 14:52 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git Thanks, both looks good. Will queue. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] commit-template: distinguish status information unconditionally 2017-06-30 14:52 ` Junio C Hamano @ 2017-07-01 1:59 ` Kaartic Sivaraam 2017-07-01 11:44 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 25+ messages in thread From: Kaartic Sivaraam @ 2017-07-01 1:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Fri, 2017-06-30 at 07:52 -0700, Junio C Hamano wrote: > Thanks, both looks good. Will queue. You're welcome :) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] commit-template: distinguish status information unconditionally 2017-07-01 1:59 ` Kaartic Sivaraam @ 2017-07-01 11:44 ` Ævar Arnfjörð Bjarmason 2017-07-01 12:08 ` Kaartic Sivaraam 2017-07-01 17:21 ` Junio C Hamano 0 siblings, 2 replies; 25+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-07-01 11:44 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: Junio C Hamano, git On Sat, Jul 01 2017, Kaartic Sivaraam jotted: > On Fri, 2017-06-30 at 07:52 -0700, Junio C Hamano wrote: >> Thanks, both looks good.Will queue. > You're welcome :) Just as someone reading this from the sidelines, very nice to have someone working this part of the UI, but it would be much easier to review if you included before/after examples of changes, e.g. (for this hypothetical change): Before we'd say: # Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # Date: <date> # # On branch master # Your branch is up-to-date with 'origin/master'. Now: # Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # Date: <date> # # On branch master # Your current branch is up-to-date with 'origin/master'. And as a word-diff: [...] # Your {+current+} branch is up-to-date with 'origin/master'. Or something like that, much easier to read something like that than read the code and mentally glue together what it's going to change. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] commit-template: distinguish status information unconditionally 2017-07-01 11:44 ` Ævar Arnfjörð Bjarmason @ 2017-07-01 12:08 ` Kaartic Sivaraam 2017-07-01 17:21 ` Junio C Hamano 1 sibling, 0 replies; 25+ messages in thread From: Kaartic Sivaraam @ 2017-07-01 12:08 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Junio C Hamano, git On Sat, 2017-07-01 at 13:44 +0200, Ævar Arnfjörð Bjarmason wrote: > Just as someone reading this from the sidelines, very nice to have > someone working this part of the UI, but it would be much easier to > review if you included before/after examples of changes That's a good comment. Will try to follow that in future. :) -- Regards, Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] commit-template: distinguish status information unconditionally 2017-07-01 11:44 ` Ævar Arnfjörð Bjarmason 2017-07-01 12:08 ` Kaartic Sivaraam @ 2017-07-01 17:21 ` Junio C Hamano 1 sibling, 0 replies; 25+ messages in thread From: Junio C Hamano @ 2017-07-01 17:21 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Kaartic Sivaraam, git Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Just as someone reading this from the sidelines, very nice to have > someone working this part of the UI, but it would be much easier to > review if you included before/after examples of changes, e.g. (for this > hypothetical change): > > > Before we'd say: > > # Please enter the commit message for your changes. Lines starting > # with '#' will be ignored, and an empty message aborts the commit. > # > # Date: <date> > # > # On branch master > # Your branch is up-to-date with 'origin/master'. > > Now: > > # Please enter the commit message for your changes. Lines starting > # with '#' will be ignored, and an empty message aborts the commit. > # > # Date: <date> > # > # On branch master > # Your current branch is up-to-date with 'origin/master'. > > And as a word-diff: > > [...] > # Your {+current+} branch is up-to-date with 'origin/master'. > > Or something like that, much easier to read something like that than > read the code and mentally glue together what it's going to change. I think you gave an example that is different from what was done on purpose, so that Kaartic can respond with "I see what you mean; what I did is different from your example but this.", but it seems that the attempt failed X-<. I do not think the patch changes the output in a situation where the above "Before" would be shown. Instead, when the extra "Date: <date>" (or "Author: <author>") is not shown, the Before picture would look like: # Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # On branch master and the update makes it look like: # Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # On branch master Hope that helps. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] commit-template: remove outdated notice about explicit paths 2017-06-30 12:12 ` Kaartic Sivaraam 2017-06-30 12:12 ` [PATCH 2/2] commit-template: distinguish status information unconditionally Kaartic Sivaraam @ 2017-07-09 17:54 ` Kaartic Sivaraam 2017-07-09 18:54 ` Junio C Hamano 1 sibling, 1 reply; 25+ messages in thread From: Kaartic Sivaraam @ 2017-07-09 17:54 UTC (permalink / raw) To: gitster; +Cc: git On Fri, 2017-06-30 at 17:42 +0530, Kaartic Sivaraam wrote: > The notice that "git commit <paths>" default to "git commit > --only <paths>" was there since 756e3ee0 ("Merge branch > 'jc/commit'", 2006-02-14). Back then, existing users of Git > expected the command doing "git commit --include <paths>", and > after the behaviour of the command was changed to align with > other people's "$scm commit <paths>", the text was added to help > them transition their expectations. > > Remove the message that now has outlived its usefulness. > It just recently dawned on me that the message, Explicit paths specified without -i or -o; assuming --only paths.. is translated and "git grep" shows it's presence in the files present in the 'po' directory. What should be done to them? Should the translators be notified? BTW what does the word 'po' stand for in the first place? I digged a bit but couldn't find much from the log. -- Kaartic ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] commit-template: remove outdated notice about explicit paths 2017-07-09 17:54 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Kaartic Sivaraam @ 2017-07-09 18:54 ` Junio C Hamano 0 siblings, 0 replies; 25+ messages in thread From: Junio C Hamano @ 2017-07-09 18:54 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes: > It just recently dawned on me that the message, > > Explicit paths specified without -i or -o; assuming --only paths.. > > is translated and "git grep" shows it's presence in the files present > in the 'po' directory. What should be done to them? Leave them as they are. They are catalogued by the messages in the code and gives us the translated equivalent. If you removed a message, the only effect is that the corresponding message will not be asked for translation. Near the end of a development cycle, the internationalization coordinator will update the master message catalog and he'll remove it when notices that the message no longer is used (even if he fails to spot the obsolete and unused message, no harm is done). Thanks. ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-07-09 18:55 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-26 17:24 [PATCH/RFC] commit-template: improve readability of commit template Kaartic Sivaraam 2017-06-26 21:59 ` Junio C Hamano 2017-06-27 17:22 ` Kaartic Sivaraam 2017-06-27 17:56 ` Junio C Hamano 2017-06-28 13:04 ` Kaartic Sivaraam 2017-06-28 14:50 ` Kaartic Sivaraam 2017-06-28 13:29 ` [PATCH] " Kaartic Sivaraam 2017-06-28 14:47 ` Kaartic Sivaraam 2017-06-28 16:48 ` Junio C Hamano 2017-06-29 17:01 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Kaartic Sivaraam 2017-06-29 17:01 ` [PATCH 2/2] commit-template: add new line before status information Kaartic Sivaraam 2017-06-29 17:51 ` Junio C Hamano 2017-06-29 18:17 ` Junio C Hamano 2017-06-30 3:19 ` Kaartic Sivaraam 2017-06-29 17:56 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Junio C Hamano 2017-06-30 3:18 ` Kaartic Sivaraam 2017-06-30 12:12 ` Kaartic Sivaraam 2017-06-30 12:12 ` [PATCH 2/2] commit-template: distinguish status information unconditionally Kaartic Sivaraam 2017-06-30 14:52 ` Junio C Hamano 2017-07-01 1:59 ` Kaartic Sivaraam 2017-07-01 11:44 ` Ævar Arnfjörð Bjarmason 2017-07-01 12:08 ` Kaartic Sivaraam 2017-07-01 17:21 ` Junio C Hamano 2017-07-09 17:54 ` [PATCH 1/2] commit-template: remove outdated notice about explicit paths Kaartic Sivaraam 2017-07-09 18:54 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).