Send real stats in the progress callback instead of % by technoweenie · Pull Request #100 · git-lfs/git-lfs · GitHub
Skip to content

Send real stats in the progress callback instead of % - #100

Merged
technoweenie merged 3 commits into
masterfrom
progress-callback
Aug 18, 2014
Merged

technoweenie merged 3 commits into
masterfrom
progress-callback

Conversation

@technoweenie

Copy link
Copy Markdown
Contributor

The format is now:

{event} {index}/{totalFiles} {sentBytes}/{totalBytes} {filename}

One caveat: The initial entry will have 0/0 sent/total bytes. This is because we don't always know the size ahead of time. Should we leave that first message out then?

/cc @joshaber

The format is now:

{event} {index}/{totalFiles} {sentBytes}/{totalBytes} {filename}
@joshaber

Copy link
Copy Markdown

@technoweenie

Copy link
Copy Markdown
Contributor Author

Okay, this no longer sends the initial log line with 0 bytes. Also, the GIT_MEDIA_PROGRESS env var requires the file:// scheme. This will give us some flexibility to allow multiple progress formats (such as JSON) in case the current format proves unwieldy.

@joshaber

Copy link
Copy Markdown

👍

@technoweenie

Copy link
Copy Markdown
Contributor Author

@joshaber: Is that a 👍 that this works for you?

@joshaber

Copy link
Copy Markdown

Oh sorry, that was 👍 on the idea.

Just tested it out. If I say GIT_MEDIA_PROGRESS=file:///var/folders/z5/gjqy6fdj7fz3w7kh61wdvpg80000gn/T/D5E7A996-BE96-4194-899D-07D2B484089C git clone git@github.com:github/gitmediatest.git then it doesn't actually output to that file, but it does create a lovely file/var/folders/z5/gjqy6fdj7fz3w7kh61wdvpg80000gn/T/D5E7A996-BE96-4194-899D-07D2B484089C file in the repo's directory :trollface:

@jspahrsummers

Copy link
Copy Markdown

@joshaber Why don't you pass a path instead of a file URL?

@joshaber

Copy link
Copy Markdown

@jspahrsummers See above:

Also, the GIT_MEDIA_PROGRESS env var requires the file:// scheme. This will give us some flexibility to allow multiple progress formats (such as JSON) in case the current format proves unwieldy.

@technoweenie

Copy link
Copy Markdown
Contributor Author

If the file:// thing is stupid (which matches my suspicions), we can just add GIT_MEDIA_PROGRESS_FORMAT or something when we change the format too.

@technoweenie

Copy link
Copy Markdown
Contributor Author

technoweenie added a commit that referenced this pull request Aug 18, 2014
Send real stats in the progress callback instead of %
@technoweenie
technoweenie merged commit 4aee116 into master Aug 18, 2014
@technoweenie
technoweenie deleted the progress-callback branch August 18, 2014 16:21
chrisd8088 added a commit that referenced this pull request Aug 5, 2026
As described in a previous commit in this PR, several of our Git LFS
commands may write progress log messages with a negative value in
the denominator of the fraction representing the amount of data
downloaded so far for an individual object.  Specifically, the
"git lfs fetch", "git lfs pull", and "git lfs smudge" commands
will write such log messages when a progress log is enabled with
the GIT_LFS_PROGRESS environment variable and the response to an
HTTP object download request is content-encoded and therefore lacks
a Content-Length header.

As described in a previous commit in this PR, for historical reasons
progress log messages are written either by the anonymous callback
function created by the CopyCallbackFile() method of the GitFilter
structure in our "lfs" package, or by the logBytes() method of the
Meter structure in our "tq" package.  However, the format of the
messages is identical regardless of which function or method
generates them.

When the logBytes() method was first introduced in commit
710862b of PR #476, it duplicated
the format of the progress log messages generated by the callback
function created by the CopyCallbackFile() method.  The current
progress log message format in that method dates from commit
36fa23d of PR #100, but the original
version of the method and its callback function themselves were added
in commit fd41906 of PR #97.

While we likely should refactor the logBytes() method and the
CopyCallbackFile() method's callback function to share a common
implementation, that change falls outside of the scope of this PR,
so we defer it to a future PR.

However, one of the changes we made in a prior commit in this PR
now ensures that if an object's total size is initially unknown,
when an EOF condition is encountered while reading the object's
data, then a final progress log message is guaranteed to be written
with the total size of the object represented as equal to the
actual number of bytes that were read.  Further, this final
progress log entry will be output regardless of whether the
CopyCallbackFile() method's callback function or the logBytes()
method is being used to write messages to the log file.

While this change affects the content of the last progress log
entry written for a given object, until the EOF condition is
reached, intermediate log messages may be generated which still
have a negative value in the denominator of the fraction
representing the amount of the object's data processed so far.

These negative values occur because the length of the data to be
read is initially unknown.  At present, the only circumstance in
which this is possible is when the HTTP response to an object
download request lacks a Content-Length header, as will be the
case when the response body is gzip-encoded, as we noted in the
following comment on PR #6011:

  #6011 (comment)

PR #6011 resolves a bug in our "git lfs filter-progress" and
"git lfs clean" commands whereby they can pass an incorrect
"totalSize" parameter to the CopyWithCallback() function.  The only
solution for this problem is for the commands to simply not attempt
to determine the total size of an object.  As a consequence, we
expect that our "git lfs filter-progress" and "git lfs clean" commands
will never know in advance the length of the object data they are
reading from Git, and it will become a more common occurrence for
the CopyCallbackFile() method's callback function and the logBytes()
method to receive a "total" parameter with a negative value.

We therefore now refine the format of the progress log when the
total size of an object is unknown by writing a series of question
mark characters ("?") in place of the denominator of the fraction
which represents the amount of the object's data processed so far.
We adjust the number of these question mark characters to match
the length of the numerator, e.g., "123/???" and "12345/?????".

To avoid implementing the necessary string formatting logic in
both the CopyCallbackFile() method's callback function and the
logBytes() method, we introduce a new FormatFraction() function
to our "tools" package and then substitute its output in place
of the appropriate fraction field in the log messages.

We then add a TestFormatFraction() Go test function which verifies
that the FormatFraction() function generates the expected output
under a range of conditions, including when the number of bytes
read so far exceeds the value provided as the total size of an
object, and when the total size of an object is unknown and should
be represented using various numbers of question mark characters.

Finally, we update the comments in several of the tests we added
to the "t/t-progress.sh" shell script in a prior commit of this PR,
and we revise the "initial total unknown" test case for our
TestCopyCallbackFileThrottle() test function.  In that test case,
we update the expected contents of the progress log because the
intermediate entry which precedes the final entry should now have
a series of question mark characters rather than a "-1" value in
the denominator of the fraction which represents the amount of the
object's data processed so far.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants