Bug 575629 - Error messages that do not end in newline are suppressed.
Summary: Error messages that do not end in newline are suppressed.
Status: RESOLVED FIXED
Alias: None
Product: JGit
Classification: Technology
Component: JGit (show other bugs)
Version: 5.11   Edit
Hardware: All All
: P3 minor (vote)
Target Milestone: 6.2   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-08-26 01:16 EDT by Joern Guy Suess CLA
Modified: 2022-03-19 18:09 EDT (History)
4 users (show)

See Also:


Attachments
Proposed patch. (1.51 KB, patch)
2021-08-26 01:16 EDT, Joern Guy Suess CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joern Guy Suess CLA 2021-08-26 01:16:07 EDT
Created attachment 287018 [details]
Proposed patch.

org.eclipse.jgit.transport.SideBandOutputStream.progress()

in commit 8dad905f20142e699faa974e7bd4cba6ced335cc

receives feedback from the git server, breaks it by line and forwards to the reporting facility (doProgressLine()).

If the string provided by the git server is not terminated with a new line, the message is suppressed. This leaves the user without information to the cause of the underlying failure.

Attached a patch that addresses the issue and uses higher-level and hopefully more transparent String processing features.
Comment 1 Matthias Sohn CLA 2021-08-26 03:42:36 EDT
- please push your patch to Gerrit [1] for review
- add a unit test for the scenario this patch is fixing
- JGit is still compiled with Java 8, hence we can't yet use String#lines and String#strip which are available since Java 11, see Bug 569917

[1] https://wiki.eclipse.org/EGit/Contributor_Guide#Contributing_Patches
Comment 2 Joern Guy Suess CLA 2021-09-09 22:03:20 EDT
I have a patch ready, but I cannot register my email with Gerrit, which means I cannot commit. I receive:

An error occurred
Error 422 (Unprocessable Entity): invalid token

Endpoint: /r/config/server/email.confirm

The documentation below states the token must be passed as an entity in the request. The email I have received contains it as a link.

https://gerrit-documentation.storage.googleapis.com/Documentation/3.1.0/rest-api-config.html#confirm-email

I cannot justify investing more time in the byzantine procedure. I will use an internal feature patch and hope some else will fix this in the future. Close wontfix if you like.
Comment 3 Thomas Wolf CLA 2022-02-25 02:50:45 EST
I'm not sure changing this would be correct. C git appears to do the same. See code in sideband.c, function demultiplex_sideband(). The bit after the last \r or \n is added to "scratch", but as far as I see it will never be written if no further sideband line is received.

The sideband protocol is designed to handle a single line being split over several sideband packets. This change would break that.

I do wonder, though, what C git does if the server only sends sideband messages without line terminator...

What was the real-world case that prompted this issue?
Comment 4 Thomas Wolf CLA 2022-02-25 03:08:50 EST
Perhaps a better approach would be to process any leftover sideband content when the SidebandInputStream is closed.
Comment 5 Jörn Guy Süß CLA 2022-02-25 05:35:52 EST
In our concrete case Gitlab was transferring information about credentials missing to the client. These were immediately copied to stdout in the standard client. I cannot understand how that could happen. Is the protocol parsing for a double line separator? Where is the requisite protocol description?
Comment 6 Jörn Guy Süß CLA 2022-02-25 05:36:49 EST
Is this it? https://git-scm.com/docs/pack-protocol#_pkt_line_format
Comment 7 Jörn Guy Süß CLA 2022-02-25 05:38:51 EST
Specifically, this: https://github.com/git/git/blob/master/Documentation/technical/protocol-common.txt
Comment 8 Thomas Wolf CLA 2022-02-25 07:33:16 EST
(In reply to Joern Guy Suess from comment #5)
> In our concrete case Gitlab was transferring information about credentials
> missing to the client. These were immediately copied to stdout in the
> standard client. I cannot understand how that could happen. Is the protocol
> parsing for a double line separator? Where is the requisite protocol
> description?

I would suppose that missing credentials is an error and is reported via side-band channel 3, which doesn't even go through this progress reporting on channel 2, and which results in a TransportException being thrown in JGit. C git writes it to stderr.
Comment 9 Jörn Guy Süß CLA 2022-02-25 07:39:46 EST
That does not explain that the patch I have submitted fixes the behaviour we have observed. The debugger also shows this. Never mind. I give up and post a how-to on Stack Overflow so users can understand where this is coming from. Maybe I will offer the patch in an attachment. Will post the SO link here when done. Please abandon the MR.
Comment 10 Thomas Wolf CLA 2022-02-26 07:30:15 EST
(In reply to Joern Guy Suess from comment #9)
> Please abandon the MR.

Done. I'll push early in 6.2 a proper fix based on the idea in comment 4.
Comment 11 Eclipse Genie CLA 2022-03-15 15:36:17 EDT
New Gerrit change created: https://git.eclipse.org/r/c/jgit/jgit/+/191926
Comment 12 Eclipse Genie CLA 2022-03-15 15:36:19 EDT
New Gerrit change created: https://git.eclipse.org/r/c/jgit/jgit/+/191927