| Message ID | 78bd6bc2c02f1daf13938a738d8eae56b5f6b74c.1572984842.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: use start_delayed_progress() | expand |
On Tue, Nov 05, 2019 at 08:14:02PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > When writing a commit-graph, we show progress along several commit > walks. When we use start_delayed_progress(), the progress line will > only appear if that step takes a decent amount of time. > > However, one place was missed: computing generation numbers. This is > normally a very fast operation as all commits have been parsed in a > previous step. But, this is showing up for all users no matter how few > commits are being added. Yep, makes sense (especially that it should all the other progress as part of the same process). > Now that we changed this method, very fast commands show no progess at > all. This means we need to stop testing for seeing these progress lines > in the test suite. I think this is OK for now, though it does make me wonder if "--progress" ought to perhaps override "delayed" in some instances, since it's a positive signal from the caller that they're interested in seeing progress. -Peff
On 11/5/2019 11:09 PM, Jeff King wrote: > On Tue, Nov 05, 2019 at 08:14:02PM +0000, Derrick Stolee via GitGitGadget wrote: > >> From: Derrick Stolee <dstolee@microsoft.com> >> >> When writing a commit-graph, we show progress along several commit >> walks. When we use start_delayed_progress(), the progress line will >> only appear if that step takes a decent amount of time. >> >> However, one place was missed: computing generation numbers. This is >> normally a very fast operation as all commits have been parsed in a >> previous step. But, this is showing up for all users no matter how few >> commits are being added. > > Yep, makes sense (especially that it should all the other progress as > part of the same process). > >> Now that we changed this method, very fast commands show no progess at >> all. This means we need to stop testing for seeing these progress lines >> in the test suite. > > I think this is OK for now, though it does make me wonder if > "--progress" ought to perhaps override "delayed" in some instances, > since it's a positive signal from the caller that they're interested in > seeing progress. I was thinking that we could start with a GIT_TEST_PROGRESS environment variable to force all delayed progress to act like non-delayed progress. That would at least give us confirmation on these kinds of tests. The impact of doing that inside the progress code could be large, so perhaps that is best left for a follow-up. Thanks, -Stolee
Jeff King <peff@peff.net> writes: > I think this is OK for now, though it does make me wonder if > "--progress" ought to perhaps override "delayed" in some instances, > since it's a positive signal from the caller that they're interested in > seeing progress. I did have the same reaction after seeing the change to 5318 where the expected output from "git commit-graph write --progress" has become unreliable. I think there are possibly three kinds of folks: - I do not want the output smudged with any progress (e.g. I am a script); - I want to see progress if it takes very long, but do not waste vertical screen real estate if it does not make me wait (e.g. I am an interactive user who occasionally wants a cue to leave the keyboard to grab coffee); and - I want to see all progress (... now who am I? Taking a screenshot to write a tutorial or something???). In the ideal world, the three choices above should probably be "--progress=(no|auto|always)" where not having any defaults to one of them (probably "auto", as the code can use isatty() to further turn it to "no"). Making "--progress" to mean "--progress=always" is OK, but it leaves no way to override an earlier --[no-]progress on the command line, which feels somewhat satisfying. Thanks.
On Wed, Nov 06, 2019 at 08:21:48AM -0500, Derrick Stolee wrote: > >> Now that we changed this method, very fast commands show no progess at > >> all. This means we need to stop testing for seeing these progress lines > >> in the test suite. > > > > I think this is OK for now, though it does make me wonder if > > "--progress" ought to perhaps override "delayed" in some instances, > > since it's a positive signal from the caller that they're interested in > > seeing progress. > > I was thinking that we could start with a GIT_TEST_PROGRESS environment > variable to force all delayed progress to act like non-delayed progress. > That would at least give us confirmation on these kinds of tests. I think this could actually be a non-test variable. E.g., something like this: diff --git a/progress.c b/progress.c index 0063559aab..74b90e8898 100644 --- a/progress.c +++ b/progress.c @@ -14,6 +14,7 @@ #include "strbuf.h" #include "trace.h" #include "utf8.h" +#include "config.h" #define TP_IDX_MAX 8 @@ -269,7 +270,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total, struct progress *start_delayed_progress(const char *title, uint64_t total) { - return start_progress_delay(title, total, 2, 0); + int delay_in_secs = git_env_ulong("GIT_PROGRESS_DELAY", 2); + return start_progress_delay(title, total, delay_in_secs, 0); } struct progress *start_progress(const char *title, uint64_t total) which lets you ask for more verbose progress. There are times when I'd use something like this for general debugging. Though these days I might suggest that something like GIT_TRACE2_PERF hook the progress code to output. That's a bit more complicated to implement, though. -Peff
On Thu, Nov 07, 2019 at 01:37:52PM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > I think this is OK for now, though it does make me wonder if > > "--progress" ought to perhaps override "delayed" in some instances, > > since it's a positive signal from the caller that they're interested in > > seeing progress. > > I did have the same reaction after seeing the change to 5318 where > the expected output from "git commit-graph write --progress" has > become unreliable. > > I think there are possibly three kinds of folks: > > - I do not want the output smudged with any progress (e.g. I am a > script); > > - I want to see progress if it takes very long, but do not waste > vertical screen real estate if it does not make me wait (e.g. I > am an interactive user who occasionally wants a cue to leave the > keyboard to grab coffee); and > > - I want to see all progress (... now who am I? Taking a > screenshot to write a tutorial or something???). I think type 3 may be people who want to understand more about the program flow, and where it's at when it sees an error. > In the ideal world, the three choices above should probably be > "--progress=(no|auto|always)" where not having any defaults to one > of them (probably "auto", as the code can use isatty() to further > turn it to "no"). I think any no/auto/always here is tricky, because it already has a meaning: to use or disregard isatty(2). And overriding that might be independent of the "type" (think pack-objects on a server generating output that's going over the wire; we have to tell it "yes, definitely show progress even though there is no terminal", but that has nothing to do with any "delay" decisions). -Peff
Jeff King <peff@peff.net> writes: > I think any no/auto/always here is tricky, because it already has a > meaning: to use or disregard isatty(2). And overriding that might be > independent of the "type". Yes, your "let the users optionally give the delay" in the other subthread makes a lot more sense. Thanks.
On 11/7/2019 1:40 AM, Jeff King wrote: > On Wed, Nov 06, 2019 at 08:21:48AM -0500, Derrick Stolee wrote: > >>>> Now that we changed this method, very fast commands show no progess at >>>> all. This means we need to stop testing for seeing these progress lines >>>> in the test suite. >>> >>> I think this is OK for now, though it does make me wonder if >>> "--progress" ought to perhaps override "delayed" in some instances, >>> since it's a positive signal from the caller that they're interested in >>> seeing progress. >> >> I was thinking that we could start with a GIT_TEST_PROGRESS environment >> variable to force all delayed progress to act like non-delayed progress. >> That would at least give us confirmation on these kinds of tests. > > I think this could actually be a non-test variable. E.g., something like > this: > > diff --git a/progress.c b/progress.c > index 0063559aab..74b90e8898 100644 > --- a/progress.c > +++ b/progress.c > @@ -14,6 +14,7 @@ > #include "strbuf.h" > #include "trace.h" > #include "utf8.h" > +#include "config.h" > > #define TP_IDX_MAX 8 > > @@ -269,7 +270,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total, > > struct progress *start_delayed_progress(const char *title, uint64_t total) > { > - return start_progress_delay(title, total, 2, 0); > + int delay_in_secs = git_env_ulong("GIT_PROGRESS_DELAY", 2); > + return start_progress_delay(title, total, delay_in_secs, 0); > } > > struct progress *start_progress(const char *title, uint64_t total) I like this idea. It allows us to force the progress on in tests, and for users to tweak their preferred delay. That includes _increasing_ the delay if they want to. > which lets you ask for more verbose progress. There are times when I'd > use something like this for general debugging. Though these days I might > suggest that something like GIT_TRACE2_PERF hook the progress code to > output. That's a bit more complicated to implement, though. Would it make sense to make delay_in_secs a local static variable, so we remember it between calls? That would allow us to check the environment only once (not that it is usually expensive). -Stolee
diff --git a/commit-graph.c b/commit-graph.c index 0aea7b2ae5..071e1c6e9b 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1103,7 +1103,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) struct commit_list *list = NULL; if (ctx->report_progress) - ctx->progress = start_progress( + ctx->progress = start_delayed_progress( _("Computing commit graph generation numbers"), ctx->commits.nr); for (i = 0; i < ctx->commits.nr; i++) { diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index d42b3efe39..8759ff0f3a 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -130,12 +130,6 @@ test_expect_success 'commit-graph write progress off for redirected stderr' ' test_line_count = 0 err ' -test_expect_success 'commit-graph write force progress on for stderr' ' - cd "$TRASH_DIRECTORY/full" && - git commit-graph write --progress 2>err && - test_file_not_empty err -' - test_expect_success 'commit-graph write with the --no-progress option' ' cd "$TRASH_DIRECTORY/full" && git commit-graph write --no-progress 2>err && diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index c0f04dc6b0..23faa5345f 100755 --- a/t/t6500-gc.sh +++ b/t/t6500-gc.sh @@ -102,20 +102,6 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre test_line_count = 2 new # There is one new pack and its .idx ' -test_expect_success 'gc --no-quiet' ' - git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && - test_must_be_empty stdout && - test_line_count = 1 stderr && - test_i18ngrep "Computing commit graph generation numbers" stderr -' - -test_expect_success TTY 'with TTY: gc --no-quiet' ' - test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr && - test_must_be_empty stdout && - test_i18ngrep "Enumerating objects" stderr && - test_i18ngrep "Computing commit graph generation numbers" stderr -' - test_expect_success 'gc --quiet' ' git -c gc.writeCommitGraph=true gc --quiet >stdout 2>stderr && test_must_be_empty stdout &&