Commit-graph write refactor (was: Create commit-graph file format v2) #112
Conversation
derrickstolee
force-pushed
the
graph/v2-head
branch
from
9639fbb
to
693900b
Compare
|
/submit |
|
Submitted as pull.112.git.gitgitgadget@gmail.com |
derrickstolee
force-pushed
the
graph/v2-head
branch
from
693900b
to
5c71c96
Compare
|
This patch series was integrated into pu via git@8d3546d. |
|
This patch series was integrated into pu via git@658a94d. |
|
This patch series was integrated into pu via git@91be9bc. |
|
This patch series was integrated into pu via git@33e44ed. |
|
This patch series was integrated into pu via git@6e9ee14. |
|
This patch series was integrated into pu via git@dfa18f9. |
|
This patch series was integrated into pu via git@d05d56a. |
|
This patch series was integrated into pu via git@2def745. |
|
This patch series was integrated into pu via git@bc97d10. |
|
This patch series was integrated into pu via git@b683987. |
|
This patch series was integrated into pu via git@1766ce8. |
|
This patch series was integrated into pu via git@67f01ef. |
|
This patch series was integrated into pu via git@17621ec. |
|
This patch series was integrated into pu via git@ab7f4a0. |
|
This patch series was integrated into pu via git@959fea7. |
|
This patch series was integrated into pu via git@442341c. |
|
This patch series was integrated into pu via git@f241268. |
|
This patch series was integrated into pu via git@15bfe2f. |
|
This patch series was integrated into pu via git@8900ddd. |
|
This patch series was integrated into pu via git@ce883b5. |
|
This patch series was integrated into pu via git@331e75e. |
|
This patch series was integrated into pu via git@dc406b1. |
|
This patch series was integrated into pu via git@99264ef. |
|
This patch series was integrated into pu via git@cf08e39. |
|
This patch series was integrated into pu via git@f38aadc. |
|
This patch series was integrated into pu via git@4bac5e4. |
|
This patch series was integrated into pu via git@0451ee8. |
|
This patch series was integrated into pu via git@45d6d38. |
|
This patch series was integrated into pu via git@74a038f. |
|
This patch series was integrated into pu via git@dab905d. |
|
This patch series was integrated into pu via git@4bb9f10. |
|
This patch series was integrated into pu via git@f94249a. |
|
This patch series was integrated into pu via git@7c0ae5a. |
|
/submit |
|
Submitted as pull.112.v5.git.gitgitgadget@gmail.com |
|
This patch series was integrated into pu via git@757609d. |
|
This patch series was integrated into pu via git@abdd229. |
|
This patch series was integrated into pu via git@689028a. |
|
This patch series was integrated into pu via git@89578ff. |
|
This patch series was integrated into pu via git@db83265. |
|
This patch series was integrated into pu via git@498f562. |
|
This patch series was integrated into pu via git@20728a9. |
|
This patch series was integrated into pu via git@55bca58. |
|
This patch series was integrated into next via git@5430eaf. |
|
This patch series was integrated into pu via git@61b9de1. |
|
This patch series was integrated into pu via git@c2a28e9. |
|
This patch series was integrated into pu via git@f3801a6. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -141,6 +141,7 @@ static int graph_write(int argc, const char **argv) | |||
| struct string_list *pack_indexes = NULL; | |||
There was a problem hiding this comment.
On the Git mailing list, SZEDER Gábor wrote (reply to this):
On Wed, Jun 12, 2019 at 06:29:37AM -0700, Derrick Stolee via GitGitGadget wrote:
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index e80c1cac02..3b6fd0d728 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -23,6 +23,14 @@ test_expect_success 'write graph with no packs' '
> test_path_is_file info/commit-graph
> '
>
> +test_expect_success 'close with correct error on bad input' '
> + cd "$TRASH_DIRECTORY/full" &&
> + echo doesnotexist >in &&
> + { git commit-graph write --stdin-packs <in 2>stderr; ret=$?; } &&
> + test "$ret" = 1 &&
This could be:
test_expect_code 1 git commit-graph write --stdin-packs <in 2>stderr
> + test_i18ngrep "error adding pack" stderr
> +'
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 6/29/2019 1:23 PM, SZEDER Gábor wrote:
> On Wed, Jun 12, 2019 at 06:29:37AM -0700, Derrick Stolee via GitGitGadget wrote:
>> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
>> index e80c1cac02..3b6fd0d728 100755
>> --- a/t/t5318-commit-graph.sh
>> +++ b/t/t5318-commit-graph.sh
>> @@ -23,6 +23,14 @@ test_expect_success 'write graph with no packs' '
>> test_path_is_file info/commit-graph
>> '
>>
>> +test_expect_success 'close with correct error on bad input' '
>> + cd "$TRASH_DIRECTORY/full" &&
>> + echo doesnotexist >in &&
>> + { git commit-graph write --stdin-packs <in 2>stderr; ret=$?; } &&
>> + test "$ret" = 1 &&
>
> This could be:
>
> test_expect_code 1 git commit-graph write --stdin-packs <in 2>stderr
>
>
>> + test_i18ngrep "error adding pack" stderr
>> +'
Thanks!, you are right! test_expect_code is what I should have used here
instead of finding the "ret=$?" trick in t0005-signals.sh, which needs to
do more interesting logic on the return code.
Here is your suggestion as a diff. Junio: could you squash this in, or
should I submit a full patch?
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 22cb9d66430..4391007f4c1 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -26,8 +26,7 @@ test_expect_success 'write graph with no packs' '
test_expect_success 'close with correct error on bad input' '
cd "$TRASH_DIRECTORY/full" &&
echo doesnotexist >in &&
- { git commit-graph write --stdin-packs <in 2>stderr; ret=$?; } &&
- test "$ret" = 1 &&
+ test_expect_code 1 git commit-graph write --stdin-packs <in 2>stderr &&
test_i18ngrep "error adding pack" stderr
'
I took inventory of when we are using "=$?" in the test scripts and saw
this was the only one that could easily be removed. Every other place is
doing something that can't be replaced by test_expect_code.
Thanks,
-Stolee
This series replaces ds/commit-graph-file-v2, and I'm using the same gitgitgadget PR to continue the version numbers and hopefully make that clear. This is a slight modification on patches 1-11 from the incremental file format RFC [0].
The commit-graph feature is growing, thanks to all of the contributions by several community members. This also means that the write_commit_graph() method is a bit unwieldy now. This series refactors that method to use a write_commit_graph_context struct that is passed between several smaller methods. The final result should be a write_commit_graph() method that has a clear set of steps. Future changes should then be easier to understand.
Patches 1-4: these are small changes which either fix issues or just
provide clean-up. These are mostly borrowed from
ds/commit-graph-format-v2.
Patches 5-11: these provide a non-functional refactor of
write_commit_graph() into several methods using a "struct
write_commit_graph_context" to share across the methods.
Updates to commits previously in this thread:
"commit-graph: remove Future Work section" no longer says that 'verify' takes as long as 'write'. [1]
"commit-graph: return with errors during write" now has a test to check we don't die(). [2]
Ævar: Looking at the old thread, I only saw two comments that still apply to this series [1] [2]. Please point me to any comments I have missed.
Updates in V5:
API calls are updated to return 0 on success and a negative value on failure.
Stopped passing 'errno' through an API function, instead returns -1.
"extracting methods" -> "extracting helper functions" in commit messages.
flags are now unsigned ints.
Thanks,
-Stolee
[0] https://public-inbox.org/git/pull.184.git.gitgitgadget@gmail.com/
[1] https://public-inbox.org/git/87o94mql0a.fsf@evledraar.gmail.com/
[2] https://public-inbox.org/git/87pnp2qlkv.fsf@evledraar.gmail.com/
Cc: sandals@crustytoothpaste.net, avarab@gmail.com, peff@peff.net