1422 Properly Color Failed Remote Push Messages #1429

maplion · 2018-01-17T03:24:45Z

area: transport.c, push.c, advice.c

problem: This is an attempt to resolve an issue I experience with people that are new to Git -- especially colleagues in a team setting -- where they miss that their push to a remote location failed because the failure and success both return a block of white text.

solution: I received some guidance from the community on how to do it myself and I did my best to model after existing code (especially since C is not my wheelhouse). When a push to remote errors/is rejected/fails, the output text for the rejection message and the error is colored red and the advice hints are colored yellow.

Before:

After:

#1422 (comment)

Signed-off-by: Ryan Dammrose ryandammrose@gmail.com

dscho · 2018-01-17T18:56:46Z

That looks pretty awesome already!

I am mostly offline this week, and Git v2.16.0 is probably coming out in a few hours (meaning: I'll have to spend some time on Git for Windows v2.16.0, despite being theoretically offline), but I will definitely review this PR properly next week (unless somebody else beats me to it).

Thank you so much for working on this!

kgybels · 2018-01-31T21:38:11Z

@maplion The spelling advise is only used in the function name. The variables all use advice. Can you change your code to follow this convention? The reason is that advise is a verb (like other function names e.g. print, load, save, ...), and advice is a noun.

Please also squash the following commits into one:

Properly Color Failed Remote Push Messages #1422 Properly Color Failed Remote Push Messages
Remove debug comments
Rename ADVISE_COLOR_ADVISE to ADVISE_COLOR_HINT
Remove debugging comments

Because of the way vs/master is updated, it might be better to place your work on top of master. Can you please, base your work on top of master without merges, and force push? Also edit the PR and change the base to master.

maplion · 2018-01-31T22:08:40Z

@kgybels Roger, roger. I think I follow all your requests and will do my best to amend this. Thank you.

dscho · 2018-01-31T22:10:24Z

@maplion you're doing real great! FYI I am planning on releasing another Git for Windows version tomorrow (there have been a couple of CVEs in components used by Git for Windows, and while they all do not quite look exploitable to me, I would rather be safe than sorry), and should have time to review this PR properly after that. Sorry for the delay!

maplion · 2018-02-01T08:43:39Z

@kgybels I think all has been updated as requested:

I left the function names with "advise" and changed all variable names to "advice" following the convention.
I renamed ADVISE_COLOR_ADVISE to ADVISE_COLOR_HINT
I rebuilt off of master and force-pushed with the updated commit and changed the pull-request to point to master.

Let me know if I missed anything. Thank you.

advice.c

@@ -20,6 +21,33 @@ int advice_add_embedded_repo = 1;
 int advice_ignored_hook = 1;
 int advice_waiting_for_editor = 1;

+static int advice_use_color = -1;


advice.c

+
+static const char *advise_get_color(enum color_advice ix)
+{
+	if (want_color(advice_use_color))


advice.c

@@ -59,7 +87,8 @@ void advise(const char *advice, ...)

 	for (cp = buf.buf; *cp; cp = np) {
 		np = strchrnul(cp, '\n');
-		fprintf(stderr,	_("hint: %.*s\n"), (int)(np - cp), cp);
+		fprintf(stderr,	_("%shint: %.*s%s\n"), advise_get_color(ADVICE_COLOR_HINT),
+			(int)(np - cp), cp, advise_get_color(ADVICE_COLOR_RESET));


builtin/push.c

 		error(_("failed to push some refs to '%s'"), transport->url);
+		fprintf(stderr, "%s", push_get_color(PUSH_COLOR_RESET));


transport.c

@@ -326,7 +354,11 @@ static void print_ref_status(char flag, const char *summary,
 		else
 			fprintf(stdout, "%s\n", summary);
 	} else {
-		fprintf(stderr, " %c %-*s ", flag, summary_width, summary);
+		if (strstr(summary, "rejected") != NULL || strstr(summary, "failure") != NULL)


dscho · 2018-02-01T13:49:16Z

This looks pretty good already, and you took it already farther than I can help: I do not really know core Git's preference regarding coloring of stderr.

So I would really like to take this to the Git mailing list.

Before I can do that, can I ask you to provide a commit message? There are a couple of really important things that the commit message should do:

it should describe the motivation for the patch
if necessary, it should discuss icky technical choices, if any have been necessary (such as using want_color() even if it targets stdout while we want to target stderr)
it should contain a "Signed-off-by:" line (essentially stating that you are legally entitled to contribute your patch under the GPL-v2, which is mainly necessary if you do this as part of your day job)

Maybe a good example to follow would be cf2f735. Or git-for-windows/build-extra@a64fe115.

We even have a wiki page that should give some good advice about writing commit messages: https://github.com/git-for-windows/git/wiki/Good-commits

Once you have this commit message, and once you have given your okay to contribute this to the Git mailing list, I will take it from there. Deal?

maplion · 2018-02-01T15:31:17Z

@dscho Thank you for the information. I'll review this in length when I get the chance and will be happy to assist updating the necessaries to help you get this into the Git email chain if that is the next step.

dscho · 2018-02-02T10:22:45Z

Thanks, @maplion!

This is an attempt to resolve an issue I experience with people that are new to Git -- especially colleagues in a team setting -- where they miss that their push to a remote location failed because the failure and success both return a block of white text. An example is if I push something to a remote repository and then a colleague attempts to push to the same remote repository and the push fails because it requires him to pull first, but he doesn't notice because a success and failure both return a block of white text. He then continues about his business, thinking it has been successfully pushed. My solution was to try to change the stderr and hint colors (red and yellow, respectively) so whenever there is a failure when pushing to a remote repository that fails, it is more noticeable. The challenge was that it seemed that stderr has never been colored and I attempted to utilize what was already established; but this meant using functions like want_color() even if it targets stdout while I wanted to target stderr. Additionally, to check for all rejection types, I did a strstr check in transport.c, but this code could be problematic if there is need for translation. Signed-off-by: Ryan Dammrose ryandammrose@gmail.com

maplion · 2018-02-04T09:15:39Z

@dscho I think things should be ready for you to take to the Git mailing list. Sorry it took me so long -- been a busy week. Let me know if you need anything else.

dscho · 2018-02-16T13:09:03Z

Finally managed to contribute it (with a little touchups: compiling it with DEVELOPER=1 and GCC showed that the *_config() functions were not actually used): https://public-inbox.org/git/cover.1518783709.git.johannes.schindelin@gmx.de/

maplion · 2018-04-05T16:50:15Z

@dscho I was just checking in to see what the status of this was? I was curious if there is a chance we'll see it in a release in the near future?

dscho · 2018-04-05T18:08:41Z

@maplion sorry, this fell completely off my radar! Thanks for the prod!

dscho · 2018-04-05T22:49:55Z

Phew. Five hours... here's the result:
https://public-inbox.org/git/cover.1522968472.git.johannes.schindelin@gmx.de/

maplion · 2018-04-05T22:57:46Z

@dscho thanks a lot for the view into the discussion; it's a good learning experience.

Certain errors, e.g. when pushing failed due to a non-fast-forwarding change, [are now colorful](git-for-windows/git#1429). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

To help users discern large chunks of white text (when the push succeeds) from large chunks of white text (when the push fails), let's add some color to the latter. This closes #1429 and fixes #1422 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

git-for-windows-ci force-pushed the vs/master branch 3 times, most recently from 6c5dcbc to 9fb3edf Compare January 22, 2018 14:10

git-for-windows-ci force-pushed the vs/master branch from 9fb3edf to 20fe102 Compare January 31, 2018 22:10

git-for-windows-ci force-pushed the vs/master branch from 20fe102 to 64a7848 Compare January 31, 2018 23:10

maplion changed the base branch from vs/master to master February 1, 2018 07:43

maplion closed this Feb 1, 2018

maplion force-pushed the 1422-colorize-push-errors branch from bb15d03 to 3016f0c Compare February 1, 2018 07:45

maplion reopened this Feb 1, 2018

dscho reviewed Feb 1, 2018

View reviewed changes

advice.c

@@ -20,6 +21,33 @@ int advice_add_embedded_repo = 1;

int advice_ignored_hook = 1;

int advice_waiting_for_editor = 1;

static int advice_use_color = -1;

This comment was marked as off-topic.

# to view

This comment was marked as off-topic.

# to view

dscho reviewed Feb 1, 2018

View reviewed changes

advice.c

static const char *advise_get_color(enum color_advice ix)

{

if (want_color(advice_use_color))

This comment was marked as off-topic.

# to view

dscho reviewed Feb 1, 2018

View reviewed changes

builtin/push.c

error(_("failed to push some refs to '%s'"), transport->url);

fprintf(stderr, "%s", push_get_color(PUSH_COLOR_RESET));

This comment was marked as off-topic.

# to view

dscho reviewed Feb 1, 2018

View reviewed changes

maplion force-pushed the 1422-colorize-push-errors branch from 3392faa to 81981e3 Compare February 4, 2018 09:13

dscho added this to the v2.17.0(2) milestone May 17, 2018

dscho closed this in b911995 May 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1422 Properly Color Failed Remote Push Messages #1429

1422 Properly Color Failed Remote Push Messages #1429

maplion commented Jan 17, 2018 •

edited

Loading

dscho commented Jan 17, 2018

kgybels commented Jan 31, 2018

maplion commented Jan 31, 2018

dscho commented Jan 31, 2018

maplion commented Feb 1, 2018 •

edited

Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

dscho commented Feb 1, 2018

maplion commented Feb 1, 2018

dscho commented Feb 2, 2018

maplion commented Feb 4, 2018

dscho commented Feb 16, 2018

maplion commented Apr 5, 2018

dscho commented Apr 5, 2018

dscho commented Apr 5, 2018

maplion commented Apr 5, 2018

		error(_("failed to push some refs to '%s'"), transport->url);
		fprintf(stderr, "%s", push_get_color(PUSH_COLOR_RESET));

1422 Properly Color Failed Remote Push Messages #1429

1422 Properly Color Failed Remote Push Messages #1429

Conversation

maplion commented Jan 17, 2018 • edited Loading

dscho commented Jan 17, 2018

kgybels commented Jan 31, 2018

maplion commented Jan 31, 2018

dscho commented Jan 31, 2018

maplion commented Feb 1, 2018 • edited Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

dscho commented Feb 1, 2018

maplion commented Feb 1, 2018

dscho commented Feb 2, 2018

maplion commented Feb 4, 2018

dscho commented Feb 16, 2018

maplion commented Apr 5, 2018

dscho commented Apr 5, 2018

dscho commented Apr 5, 2018

maplion commented Apr 5, 2018

maplion commented Jan 17, 2018 •

edited

Loading

maplion commented Feb 1, 2018 •

edited

Loading