| Message ID | pull.1218.v3.git.git.1645817452.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | libify reflog | expand |
On Fri, Feb 25, 2022 at 07:30:49PM +0000, John Cai via GitGitGadget wrote: > John Cai (3): > stash: add tests to ensure reflog --rewrite --updatref behavior > reflog: libify delete reflog function and helpers > stash: call reflog_delete() in reflog.c > > Makefile | 1 + > builtin/reflog.c | 455 +---------------------------------------------- > builtin/stash.c | 18 +- > object.h | 2 +- > reflog.c | 432 ++++++++++++++++++++++++++++++++++++++++++++ > reflog.h | 43 +++++ > t/t3903-stash.sh | 65 +++++-- > 7 files changed, 539 insertions(+), 477 deletions(-) > create mode 100644 reflog.c > create mode 100644 reflog.h Thanks; I glossed over the discussion about tests (since it looks like you and Ævar already have a good handle on how things are going there). The rest of this version of the series (which I looked at more closely) looks good to me. Thanks Taylor
Hi, Just wanted to bump this thread. It'd be good to get another ack on these last set of changes. On 25 Feb 2022, at 14:38, Taylor Blau wrote: > On Fri, Feb 25, 2022 at 07:30:49PM +0000, John Cai via GitGitGadget wrote: >> John Cai (3): >> stash: add tests to ensure reflog --rewrite --updatref behavior >> reflog: libify delete reflog function and helpers >> stash: call reflog_delete() in reflog.c >> >> Makefile | 1 + >> builtin/reflog.c | 455 +---------------------------------------------- >> builtin/stash.c | 18 +- >> object.h | 2 +- >> reflog.c | 432 ++++++++++++++++++++++++++++++++++++++++++++ >> reflog.h | 43 +++++ >> t/t3903-stash.sh | 65 +++++-- >> 7 files changed, 539 insertions(+), 477 deletions(-) >> create mode 100644 reflog.c >> create mode 100644 reflog.h > > Thanks; I glossed over the discussion about tests (since it looks like > you and Ævar already have a good handle on how things are going there). > > The rest of this version of the series (which I looked at more closely) > looks good to me. > > Thanks > Taylor
On Wed, Mar 02 2022, John Cai wrote: > Just wanted to bump this thread. It'd be good to get another ack on these > last set of changes. Hi. Sorry that I didn't look at it earlier. This looks good to me and I think everything that's been brought up has been addressed. I left a nit on 1/3 suggesting a way to make that diff a bit smaller by using a subshell instead of refactoring an existing function. But I think with or without that & a ro-roll this would be good to advance to "next" etc. Thanks!
In [1], there was a discussion around a bug report of stash not recovering in the middle of the process when killed with ctl-c. It turned out to not be a bug we need to fix. However, out of that discussion came the idea of libifying reflog. This can stand alone as a code improvement. stash.c currently shells out to call reflog to delete reflogs. Libify reflog delete and call it from both builtin/reflog.c and builtin/stash.c. This patch has three parts: * add missing test coverage for git stash delete * libify reflog's delete functionality and move some of the helpers into a reflog.c library and call reflog_delete from builtin/reflog.c * call reflog_delete from builtin/stash.c Updates since v2: * removed unnecessary includes * adjusted wrapping/whitespace in reflog.h * adjusted test to be isolated from other tests since currently tests for stash depend on each other. There was some discussion around this and even a possibility to refactor the tests. However, it would have been a larger effort than is worth for this series, so instead I just made one of the tests I added be isolated from the others. Updates since v1: * added missing test coverage * squashed 1/3 and 2/3 together * moved enum into reflog.c * updated object.h's flag allocation mapping 1. https://lore.kernel.org/git/220126.86h79qe692.gmgdl@evledraar.gmail.com/ John Cai (3): stash: add tests to ensure reflog --rewrite --updatref behavior reflog: libify delete reflog function and helpers stash: call reflog_delete() in reflog.c Makefile | 1 + builtin/reflog.c | 455 +---------------------------------------------- builtin/stash.c | 18 +- object.h | 2 +- reflog.c | 432 ++++++++++++++++++++++++++++++++++++++++++++ reflog.h | 43 +++++ t/t3903-stash.sh | 65 +++++-- 7 files changed, 539 insertions(+), 477 deletions(-) create mode 100644 reflog.c create mode 100644 reflog.h base-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1218%2Fjohn-cai%2Fjc-libify-reflog-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1218/john-cai/jc-libify-reflog-v3 Pull-Request: https://github.com/git/git/pull/1218 Range-diff vs v2: 1: 6e136b62ca4 ! 1: 33299825fc4 stash: add test to ensure reflog --rewrite --updatref behavior @@ Metadata Author: John Cai <johncai86@gmail.com> ## Commit message ## - stash: add test to ensure reflog --rewrite --updatref behavior + stash: add tests to ensure reflog --rewrite --updatref behavior There is missing test coverage to ensure that the resulting reflogs after a git stash drop has had its old oid rewritten if applicable, and @@ Commit message Signed-off-by: John Cai <johncai86@gmail.com> ## t/t3903-stash.sh ## +@@ t/t3903-stash.sh: diff_cmp () { + rm -f "$1.compare" "$2.compare" + } + +-test_expect_success 'stash some dirty working directory' ' +- echo 1 >file && +- git add file && +- echo unrelated >other-file && +- git add other-file && ++setup_stash() { ++ repo_dir=$1 ++ if test -z $repo_dir; then ++ repo_dir="." ++ fi ++ ++ echo 1 >$repo_dir/file && ++ git -C $repo_dir add file && ++ echo unrelated >$repo_dir/other-file && ++ git -C $repo_dir add other-file && + test_tick && +- git commit -m initial && +- echo 2 >file && ++ git -C $repo_dir commit -m initial && ++ echo 2 >$repo_dir/file && + git add file && +- echo 3 >file && ++ echo 3 >$repo_dir/file && + test_tick && +- git stash && +- git diff-files --quiet && +- git diff-index --cached --quiet HEAD ++ git -C $repo_dir stash && ++ git -C $repo_dir diff-files --quiet && ++ git -C $repo_dir diff-index --cached --quiet HEAD ++} ++ ++test_expect_success 'stash some dirty working directory' ' ++ setup_stash + ' + + cat >expect <<EOF @@ t/t3903-stash.sh: test_expect_success 'drop middle stash by index' ' test 1 = $(git show HEAD:file) ' @@ t/t3903-stash.sh: test_expect_success 'drop middle stash by index' ' + test_cmp expect actual +' + -+test_expect_success 'drop stash reflog updates refs/stash with rewrite' ' -+ git reset --hard && -+ echo 9 >file && -+ git stash && -+ oid="$(git rev-parse stash@{0})" && -+ git stash drop stash@{1} && -+ cut -d" " -f1-2 .git/logs/refs/stash >actual && ++test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' ' ++ git init repo && ++ setup_stash repo && ++ echo 9 >repo/file && ++ ++ old_oid="$(git -C repo rev-parse stash@{0})" && ++ git -C repo stash && ++ new_oid="$(git -C repo rev-parse stash@{0})" && ++ ++ cat >expect <<-EOF && ++ $(test_oid zero) $old_oid ++ $old_oid $new_oid ++ EOF ++ cut -d" " -f1-2 repo/.git/logs/refs/stash >actual && ++ test_cmp expect actual && ++ ++ git -C repo stash drop stash@{1} && ++ cut -d" " -f1-2 repo/.git/logs/refs/stash >actual && + cat >expect <<-EOF && -+ $(test_oid zero) $oid ++ $(test_oid zero) $new_oid + EOF + test_cmp expect actual +' @@ t/t3903-stash.sh: test_expect_success 'drop middle stash by index' ' test_expect_success 'stash pop' ' git reset --hard && git stash pop && -- test 3 = $(cat file) && -+ test 9 = $(cat file) && - test 1 = $(git show :file) && - test 1 = $(git show HEAD:file) && - test 0 = $(git stash list | wc -l) 2: e7c950218b1 ! 2: 33adfee4ca6 reflog: libify delete reflog function and helpers @@ builtin/reflog.c @@ #include "builtin.h" #include "config.h" - #include "lockfile.h" +-#include "lockfile.h" -#include "object-store.h" - #include "repository.h" +-#include "repository.h" -#include "commit.h" -#include "refs.h" - #include "dir.h" +-#include "dir.h" -#include "tree-walk.h" - #include "diff.h" +-#include "diff.h" #include "revision.h" #include "reachable.h" #include "worktree.h" @@ object.h: struct object_array { ## reflog.c (new) ## @@ +#include "cache.h" -+#include "commit.h" +#include "object-store.h" -+#include "reachable.h" +#include "reflog.h" +#include "refs.h" +#include "revision.h" -+#include "tree-walk.h" +#include "worktree.h" + +/* Remember to update object flag allocation in object.h */ @@ reflog.h (new) @@ +#ifndef REFLOG_H +#define REFLOG_H -+ +#include "refs.h" + +struct cmd_reflog_expire_cb { @@ reflog.h (new) + unsigned int dry_run:1; +}; + -+int reflog_delete(const char *rev, enum expire_reflog_flags flags, int verbose); -+ ++int reflog_delete(const char *rev, enum expire_reflog_flags flags, ++ int verbose); +void reflog_expiry_cleanup(void *cb_data); -+ +void reflog_expiry_prepare(const char *refname, const struct object_id *oid, + void *cb_data); -+ +int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid, + const char *email, timestamp_t timestamp, int tz, + const char *message, void *cb_data); -+ +int count_reflog_ent(struct object_id *ooid, struct object_id *noid, + const char *email, timestamp_t timestamp, int tz, + const char *message, void *cb_data); -+ +int should_expire_reflog_ent_verbose(struct object_id *ooid, + struct object_id *noid, + const char *email, + timestamp_t timestamp, int tz, + const char *message, void *cb_data); -+ +#endif /* REFLOG_H */ 3: a023a70092b = 3: b17d8e5d43a stash: call reflog_delete() in reflog.c