* [PATCH v4 0/5] Implement git stash as a builtin command @ 2017-06-08 0:55 Joel Teichroeb 2017-06-08 0:55 ` [PATCH v4 1/5] stash: add test for stash create with no files Joel Teichroeb ` (5 more replies) 0 siblings, 6 replies; 26+ messages in thread From: Joel Teichroeb @ 2017-06-08 0:55 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Cc: Joel Teichroeb I've rewritten git stash as a builtin c command. All tests pass, and I've added two new tests. Test coverage is around 95% with the only things missing coverage being error handlers. Changes since v3: * Fixed formatting issues * Fixed a bug with stash branch and added a new test for it * Fixed review comments Outstanding issue: * Not all argv array memory is cleaned up Joel Teichroeb (5): stash: add test for stash create with no files stash: Add a test for when apply fails during stash branch stash: add test for stashing in a detached state merge: close the index lock when not writing the new index stash: implement builtin stash Makefile | 2 +- builtin.h | 1 + builtin/stash.c | 1224 +++++++++++++++++++++++++ git-stash.sh => contrib/examples/git-stash.sh | 0 git.c | 1 + merge-recursive.c | 9 +- t/t3903-stash.sh | 34 + 7 files changed, 1267 insertions(+), 4 deletions(-) create mode 100644 builtin/stash.c rename git-stash.sh => contrib/examples/git-stash.sh (100%) -- 2.13.0 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 1/5] stash: add test for stash create with no files 2017-06-08 0:55 [PATCH v4 0/5] Implement git stash as a builtin command Joel Teichroeb @ 2017-06-08 0:55 ` Joel Teichroeb 2017-06-13 19:31 ` Junio C Hamano 2017-06-08 0:55 ` [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch Joel Teichroeb ` (4 subsequent siblings) 5 siblings, 1 reply; 26+ messages in thread From: Joel Teichroeb @ 2017-06-08 0:55 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Cc: Joel Teichroeb Ensure the command gives the correct return code Signed-off-by: Joel Teichroeb <joel@teichroeb.net> --- t/t3903-stash.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 3b4bed5c9a..cc923e6335 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' ' test foo = "$(cat file/file)" ' +test_expect_success 'stash create - no changes' ' + git stash clear && + test_when_finished "git reset --hard HEAD" && + git reset --hard && + git stash create >actual && + test_must_be_empty actual +' + test_expect_success 'stash branch - no stashes on stack, stash-like argument' ' git stash clear && test_when_finished "git reset --hard HEAD" && -- 2.13.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/5] stash: add test for stash create with no files 2017-06-08 0:55 ` [PATCH v4 1/5] stash: add test for stash create with no files Joel Teichroeb @ 2017-06-13 19:31 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2017-06-13 19:31 UTC (permalink / raw) To: Joel Teichroeb Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Joel Teichroeb <joel@teichroeb.net> writes: > Ensure the command gives the correct return code OK. When you know what the correct return code is, we'd prefer to see it spelled out, i.e. Ensure that the command succeeds. Or did you mean that the command outputs nothing? The test itself looks obviously correct ;-) > Signed-off-by: Joel Teichroeb <joel@teichroeb.net> > --- > t/t3903-stash.sh | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 3b4bed5c9a..cc923e6335 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' ' > test foo = "$(cat file/file)" > ' > > +test_expect_success 'stash create - no changes' ' > + git stash clear && > + test_when_finished "git reset --hard HEAD" && > + git reset --hard && > + git stash create >actual && > + test_must_be_empty actual > +' > + > test_expect_success 'stash branch - no stashes on stack, stash-like argument' ' > git stash clear && > test_when_finished "git reset --hard HEAD" && ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch 2017-06-08 0:55 [PATCH v4 0/5] Implement git stash as a builtin command Joel Teichroeb 2017-06-08 0:55 ` [PATCH v4 1/5] stash: add test for stash create with no files Joel Teichroeb @ 2017-06-08 0:55 ` Joel Teichroeb 2017-06-13 19:40 ` Junio C Hamano 2017-06-08 0:55 ` [PATCH v4 3/5] stash: add test for stashing in a detached state Joel Teichroeb ` (3 subsequent siblings) 5 siblings, 1 reply; 26+ messages in thread From: Joel Teichroeb @ 2017-06-08 0:55 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Cc: Joel Teichroeb If the return value of merge recurisve is not checked, the stash could end up being dropped even though it was not applied properly Signed-off-by: Joel Teichroeb <joel@teichroeb.net> --- t/t3903-stash.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index cc923e6335..5399fb05ca 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the stash if the branch exists git rev-parse stash@{0} -- ' +test_expect_success 'stash branch should not drop the stash if the apply fails' ' + git stash clear && + git reset HEAD~1 --hard && + echo foo >file && + git add file && + git commit -m initial && + echo bar >file && + git stash && + echo baz >file && + test_when_finished "git checkout master" && + test_must_fail git stash branch new_branch stash@{0} && + git rev-parse stash@{0} -- +' + test_expect_success 'stash apply shows status same as git status (relative to current directory)' ' git stash clear && echo 1 >subdir/subfile1 && -- 2.13.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch 2017-06-08 0:55 ` [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch Joel Teichroeb @ 2017-06-13 19:40 ` Junio C Hamano 2017-06-13 19:54 ` Joel Teichroeb 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2017-06-13 19:40 UTC (permalink / raw) To: Joel Teichroeb Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Joel Teichroeb <joel@teichroeb.net> writes: > If the return value of merge recurisve is not checked, the stash could end > up being dropped even though it was not applied properly s/recurisve/recursive/ > Signed-off-by: Joel Teichroeb <joel@teichroeb.net> > --- > t/t3903-stash.sh | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index cc923e6335..5399fb05ca 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the stash if the branch exists > git rev-parse stash@{0} -- > ' > > +test_expect_success 'stash branch should not drop the stash if the apply fails' ' > + git stash clear && > + git reset HEAD~1 --hard && > + echo foo >file && > + git add file && > + git commit -m initial && It's not quite intuitive to call a non-root commit "initial" ;-) > + echo bar >file && > + git stash && > + echo baz >file && OK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}. > + test_when_finished "git checkout master" && > + test_must_fail git stash branch new_branch stash@{0} && Hmph. Do we blindly checkout new_branch out of stash@{0}^1 and unstash, but because 'file' in the working tree is dirty, we fail to apply the stash and stop? This sounds like a bug to me. Shouldn't we be staying on 'master', and fail without even creating 'new_branch', when this happens? In any case we should be testing what branch we are on after this step. What branch should we be on after "git stash branch" fails? > + git rev-parse stash@{0} -- > +' > + > test_expect_success 'stash apply shows status same as git status (relative to current directory)' ' > git stash clear && > echo 1 >subdir/subfile1 && ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch 2017-06-13 19:40 ` Junio C Hamano @ 2017-06-13 19:54 ` Joel Teichroeb 0 siblings, 0 replies; 26+ messages in thread From: Joel Teichroeb @ 2017-06-13 19:54 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder On Tue, Jun 13, 2017 at 12:40 PM, Junio C Hamano <gitster@pobox.com> wrote: > Joel Teichroeb <joel@teichroeb.net> writes: > >> If the return value of merge recurisve is not checked, the stash could end >> up being dropped even though it was not applied properly > > s/recurisve/recursive/ > >> Signed-off-by: Joel Teichroeb <joel@teichroeb.net> >> --- >> t/t3903-stash.sh | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh >> index cc923e6335..5399fb05ca 100755 >> --- a/t/t3903-stash.sh >> +++ b/t/t3903-stash.sh >> @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the stash if the branch exists >> git rev-parse stash@{0} -- >> ' >> >> +test_expect_success 'stash branch should not drop the stash if the apply fails' ' >> + git stash clear && >> + git reset HEAD~1 --hard && >> + echo foo >file && >> + git add file && >> + git commit -m initial && > > It's not quite intuitive to call a non-root commit "initial" ;-) > >> + echo bar >file && >> + git stash && >> + echo baz >file && > > OK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}. > >> + test_when_finished "git checkout master" && >> + test_must_fail git stash branch new_branch stash@{0} && > > Hmph. Do we blindly checkout new_branch out of stash@{0}^1 and > unstash, but because 'file' in the working tree is dirty, we fail to > apply the stash and stop? > > This sounds like a bug to me. Shouldn't we be staying on 'master', > and fail without even creating 'new_branch', when this happens? Good point. The existing behavior is to create new_branch and check it out. I'm not sure what the correct state should be then. Create new_branch, checkout new_branch, fail to apply, checkout master? Should it then delete new_branch? Is there a way instead to test applying the stash before creating the branch without actually applying it? Something like putting merge_recursive into some kind of dry-run mode? > > In any case we should be testing what branch we are on after this > step. What branch should we be on after "git stash branch" fails? > >> + git rev-parse stash@{0} -- >> +' >> + >> test_expect_success 'stash apply shows status same as git status (relative to current directory)' ' >> git stash clear && >> echo 1 >subdir/subfile1 && ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 3/5] stash: add test for stashing in a detached state 2017-06-08 0:55 [PATCH v4 0/5] Implement git stash as a builtin command Joel Teichroeb 2017-06-08 0:55 ` [PATCH v4 1/5] stash: add test for stash create with no files Joel Teichroeb 2017-06-08 0:55 ` [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch Joel Teichroeb @ 2017-06-08 0:55 ` Joel Teichroeb 2017-06-13 19:45 ` Junio C Hamano 2017-06-08 0:55 ` [PATCH v4 4/5] merge: close the index lock when not writing the new index Joel Teichroeb ` (2 subsequent siblings) 5 siblings, 1 reply; 26+ messages in thread From: Joel Teichroeb @ 2017-06-08 0:55 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Cc: Joel Teichroeb Signed-off-by: Joel Teichroeb <joel@teichroeb.net> --- t/t3903-stash.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 5399fb05ca..ce4c8fe3d6 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for the message' ' test_cmp expect actual ' +test_expect_success 'create in a detached state' ' + test_when_finished "git checkout master" && + git checkout HEAD~1 && + >foo && + git add foo && + STASH_ID=$(git stash create) && + HEAD_ID=$(git rev-parse --short HEAD) && + echo "WIP on (no branch): ${HEAD_ID} initial" >expect && + git show --pretty=%s -s ${STASH_ID} >actual && + test_cmp expect actual +' + test_expect_success 'stash -- <pathspec> stashes and restores the file' ' >foo && >bar && -- 2.13.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 3/5] stash: add test for stashing in a detached state 2017-06-08 0:55 ` [PATCH v4 3/5] stash: add test for stashing in a detached state Joel Teichroeb @ 2017-06-13 19:45 ` Junio C Hamano 2017-06-13 19:48 ` Joel Teichroeb 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2017-06-13 19:45 UTC (permalink / raw) To: Joel Teichroeb Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Joel Teichroeb <joel@teichroeb.net> writes: > Signed-off-by: Joel Teichroeb <joel@teichroeb.net> > --- > t/t3903-stash.sh | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 5399fb05ca..ce4c8fe3d6 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for the message' ' > test_cmp expect actual > ' > > +test_expect_success 'create in a detached state' ' > + test_when_finished "git checkout master" && > + git checkout HEAD~1 && > + >foo && > + git add foo && > + STASH_ID=$(git stash create) && > + HEAD_ID=$(git rev-parse --short HEAD) && > + echo "WIP on (no branch): ${HEAD_ID} initial" >expect && > + git show --pretty=%s -s ${STASH_ID} >actual && > + test_cmp expect actual > +' Hmph. Is the title automatically given to the stash the only/primary thing that is of interest to us in this test? I think we care more about that we record the right thing in the resulting stash and also after creating the stash the working tree and the index becomes clean. Shouldn't we be testing that? If "git stash create" fails to make the working tree and the index clean, then "git checkout master" run by when-finished will carry the local modifications with us, which probably is not what you meant. You'd need "reset --hard" there, too, perhaps? > test_expect_success 'stash -- <pathspec> stashes and restores the file' ' > >foo && > >bar && ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 3/5] stash: add test for stashing in a detached state 2017-06-13 19:45 ` Junio C Hamano @ 2017-06-13 19:48 ` Joel Teichroeb 2017-06-13 20:58 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Joel Teichroeb @ 2017-06-13 19:48 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder On Tue, Jun 13, 2017 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote: > Joel Teichroeb <joel@teichroeb.net> writes: > >> Signed-off-by: Joel Teichroeb <joel@teichroeb.net> >> --- >> t/t3903-stash.sh | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh >> index 5399fb05ca..ce4c8fe3d6 100755 >> --- a/t/t3903-stash.sh >> +++ b/t/t3903-stash.sh >> @@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for the message' ' >> test_cmp expect actual >> ' >> >> +test_expect_success 'create in a detached state' ' >> + test_when_finished "git checkout master" && >> + git checkout HEAD~1 && >> + >foo && >> + git add foo && >> + STASH_ID=$(git stash create) && >> + HEAD_ID=$(git rev-parse --short HEAD) && >> + echo "WIP on (no branch): ${HEAD_ID} initial" >expect && >> + git show --pretty=%s -s ${STASH_ID} >actual && >> + test_cmp expect actual >> +' > > Hmph. Is the title automatically given to the stash the > only/primary thing that is of interest to us in this test? I think > we care more about that we record the right thing in the resulting > stash and also after creating the stash the working tree and the > index becomes clean. Shouldn't we be testing that? In this case, the title is really what I wanted to test. There are other tests already to make sure that stash create works, but there were no tests to ensure that a stash was created with the correct title when not on a branch. That being said though, I'll add more validation as more validation is always better. > > If "git stash create" fails to make the working tree and the index > clean, then "git checkout master" run by when-finished will carry > the local modifications with us, which probably is not what you > meant. You'd need "reset --hard" there, too, perhaps? Agreed. > >> test_expect_success 'stash -- <pathspec> stashes and restores the file' ' >> >foo && >> >bar && ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 3/5] stash: add test for stashing in a detached state 2017-06-13 19:48 ` Joel Teichroeb @ 2017-06-13 20:58 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2017-06-13 20:58 UTC (permalink / raw) To: Joel Teichroeb Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Joel Teichroeb <joel@teichroeb.net> writes: >>> +test_expect_success 'create in a detached state' ' >>> + test_when_finished "git checkout master" && >>> + git checkout HEAD~1 && >>> + >foo && >>> + git add foo && >>> + STASH_ID=$(git stash create) && >>> + HEAD_ID=$(git rev-parse --short HEAD) && >>> + echo "WIP on (no branch): ${HEAD_ID} initial" >expect && >>> + git show --pretty=%s -s ${STASH_ID} >actual && >>> + test_cmp expect actual >>> +' >> >> Hmph. Is the title automatically given to the stash the >> only/primary thing that is of interest to us in this test? I think >> we care more about that we record the right thing in the resulting >> stash and also after creating the stash the working tree and the >> index becomes clean. Shouldn't we be testing that? > > In this case, the title is really what I wanted to test. There are > other tests already to make sure that stash create works, but there > were no tests to ensure that a stash was created with the correct > title when not on a branch. Ah, OK. Thanks. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 4/5] merge: close the index lock when not writing the new index 2017-06-08 0:55 [PATCH v4 0/5] Implement git stash as a builtin command Joel Teichroeb ` (2 preceding siblings ...) 2017-06-08 0:55 ` [PATCH v4 3/5] stash: add test for stashing in a detached state Joel Teichroeb @ 2017-06-08 0:55 ` Joel Teichroeb 2017-06-13 19:47 ` Junio C Hamano 2017-06-08 0:55 ` [PATCH v4 5/5] stash: implement builtin stash Joel Teichroeb 2017-06-11 17:40 ` [PATCH v4 0/5] Implement git stash as a builtin command Joel Teichroeb 5 siblings, 1 reply; 26+ messages in thread From: Joel Teichroeb @ 2017-06-08 0:55 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Cc: Joel Teichroeb If the merge does not have anything to do, it does not unlock the index, causing any further index operations to fail. Thus, always unlock the index regardless of outcome. Signed-off-by: Joel Teichroeb <joel@teichroeb.net> --- merge-recursive.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index ae5238d82c..16bb5512ef 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2145,9 +2145,12 @@ int merge_recursive_generic(struct merge_options *o, if (clean < 0) return clean; - if (active_cache_changed && - write_locked_index(&the_index, lock, COMMIT_LOCK)) - return err(o, _("Unable to write index.")); + if (active_cache_changed) { + if (write_locked_index(&the_index, lock, COMMIT_LOCK)) + return err(o, _("Unable to write index.")); + } else { + rollback_lock_file(lock); + } return clean ? 0 : 1; } -- 2.13.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/5] merge: close the index lock when not writing the new index 2017-06-08 0:55 ` [PATCH v4 4/5] merge: close the index lock when not writing the new index Joel Teichroeb @ 2017-06-13 19:47 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2017-06-13 19:47 UTC (permalink / raw) To: Joel Teichroeb Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Joel Teichroeb <joel@teichroeb.net> writes: > If the merge does not have anything to do, it does not unlock the index, > causing any further index operations to fail. Thus, always unlock the index > regardless of outcome. > > Signed-off-by: Joel Teichroeb <joel@teichroeb.net> > --- This one makes sense. So far, nobody who calls this function performs further index manipulations and letting the atexit handlers automatically release the lock was sufficient. This allows new callers to do more work on the index after a merge finishes. > merge-recursive.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/merge-recursive.c b/merge-recursive.c > index ae5238d82c..16bb5512ef 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -2145,9 +2145,12 @@ int merge_recursive_generic(struct merge_options *o, > if (clean < 0) > return clean; > > - if (active_cache_changed && > - write_locked_index(&the_index, lock, COMMIT_LOCK)) > - return err(o, _("Unable to write index.")); > + if (active_cache_changed) { > + if (write_locked_index(&the_index, lock, COMMIT_LOCK)) > + return err(o, _("Unable to write index.")); > + } else { > + rollback_lock_file(lock); > + } > > return clean ? 0 : 1; > } ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 5/5] stash: implement builtin stash 2017-06-08 0:55 [PATCH v4 0/5] Implement git stash as a builtin command Joel Teichroeb ` (3 preceding siblings ...) 2017-06-08 0:55 ` [PATCH v4 4/5] merge: close the index lock when not writing the new index Joel Teichroeb @ 2017-06-08 0:55 ` Joel Teichroeb 2017-06-11 21:27 ` Thomas Gummerer ` (3 more replies) 2017-06-11 17:40 ` [PATCH v4 0/5] Implement git stash as a builtin command Joel Teichroeb 5 siblings, 4 replies; 26+ messages in thread From: Joel Teichroeb @ 2017-06-08 0:55 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Cc: Joel Teichroeb Implement all git stash functionality as a builtin command Signed-off-by: Joel Teichroeb <joel@teichroeb.net> --- Makefile | 2 +- builtin.h | 1 + builtin/stash.c | 1224 +++++++++++++++++++++++++ git-stash.sh => contrib/examples/git-stash.sh | 0 git.c | 1 + 5 files changed, 1227 insertions(+), 1 deletion(-) create mode 100644 builtin/stash.c rename git-stash.sh => contrib/examples/git-stash.sh (100%) diff --git a/Makefile b/Makefile index 7c621f7f76..3364d87630 100644 --- a/Makefile +++ b/Makefile @@ -525,7 +525,6 @@ SCRIPT_SH += git-quiltimport.sh SCRIPT_SH += git-rebase.sh SCRIPT_SH += git-remote-testgit.sh SCRIPT_SH += git-request-pull.sh -SCRIPT_SH += git-stash.sh SCRIPT_SH += git-submodule.sh SCRIPT_SH += git-web--browse.sh @@ -965,6 +964,7 @@ BUILTIN_OBJS += builtin/send-pack.o BUILTIN_OBJS += builtin/shortlog.o BUILTIN_OBJS += builtin/show-branch.o BUILTIN_OBJS += builtin/show-ref.o +BUILTIN_OBJS += builtin/stash.o BUILTIN_OBJS += builtin/stripspace.o BUILTIN_OBJS += builtin/submodule--helper.o BUILTIN_OBJS += builtin/symbolic-ref.o diff --git a/builtin.h b/builtin.h index 498ac80d07..fa59481420 100644 --- a/builtin.h +++ b/builtin.h @@ -119,6 +119,7 @@ extern int cmd_shortlog(int argc, const char **argv, const char *prefix); extern int cmd_show(int argc, const char **argv, const char *prefix); extern int cmd_show_branch(int argc, const char **argv, const char *prefix); extern int cmd_status(int argc, const char **argv, const char *prefix); +extern int cmd_stash(int argc, const char **argv, const char *prefix); extern int cmd_stripspace(int argc, const char **argv, const char *prefix); extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix); extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix); diff --git a/builtin/stash.c b/builtin/stash.c new file mode 100644 index 0000000000..a9680f2909 --- /dev/null +++ b/builtin/stash.c @@ -0,0 +1,1224 @@ +#include "builtin.h" +#include "parse-options.h" +#include "refs.h" +#include "tree.h" +#include "lockfile.h" +#include "object.h" +#include "tree-walk.h" +#include "cache-tree.h" +#include "unpack-trees.h" +#include "diff.h" +#include "revision.h" +#include "commit.h" +#include "diffcore.h" +#include "merge-recursive.h" +#include "argv-array.h" +#include "run-command.h" + +static const char * const git_stash_usage[] = { + N_("git stash list [<options>]"), + N_("git stash show [<stash>]"), + N_("git stash drop [-q|--quiet] [<stash>]"), + N_("git stash ( pop | apply ) [--index] [-q|--quiet] [<stash>]"), + N_("git stash branch <branchname> [<stash>]"), + N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"), + N_(" [-u|--include-untracked] [-a|--all] [<message>]]"), + N_("git stash clear"), + N_("git stash create [<message>]"), + N_("git stash store [-m|--message <message>] [-q|--quiet] <commit>"), + NULL +}; + +static const char * const git_stash_list_usage[] = { + N_("git stash list [<options>]"), + NULL +}; + +static const char * const git_stash_show_usage[] = { + N_("git stash show [<stash>]"), + NULL +}; + +static const char * const git_stash_drop_usage[] = { + N_("git stash drop [-q|--quiet] [<stash>]"), + NULL +}; + +static const char * const git_stash_pop_usage[] = { + N_("git stash pop [--index] [-q|--quiet] [<stash>]"), + NULL +}; + +static const char * const git_stash_apply_usage[] = { + N_("git stash apply [--index] [-q|--quiet] [<stash>]"), + NULL +}; + +static const char * const git_stash_branch_usage[] = { + N_("git stash branch <branchname> [<stash>]"), + NULL +}; + +static const char * const git_stash_save_usage[] = { + N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"), + N_(" [-u|--include-untracked] [-a|--all] [<message>]]"), + NULL +}; + +static const char * const git_stash_clear_usage[] = { + N_("git stash clear"), + NULL +}; + +static const char * const git_stash_create_usage[] = { + N_("git stash create [<message>]"), + NULL +}; + +static const char * const git_stash_store_usage[] = { + N_("git stash store [-m|--message <message>] [-q|--quiet] <commit>"), + NULL +}; + +static const char *ref_stash = "refs/stash"; +static int quiet = 0; +static struct lock_file lock_file; +static char stash_index_path[64]; + +struct stash_info { + struct object_id w_commit; + struct object_id b_commit; + struct object_id i_commit; + struct object_id u_commit; + struct object_id w_tree; + struct object_id b_tree; + struct object_id i_tree; + struct object_id u_tree; + const char *message; + const char *revision; + int is_stash_ref; + int has_u; + const char *patch; +}; + +static int untracked_files(struct strbuf *out, int include_untracked, + int include_ignored, const char **argv) +{ + struct child_process cp = CHILD_PROCESS_INIT; + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "ls-files", "-o", "-z", NULL); + if (include_untracked && !include_ignored) + argv_array_push(&cp.args, "--exclude-standard"); + argv_array_push(&cp.args, "--"); + if (argv) + argv_array_pushv(&cp.args, argv); + return pipe_command(&cp, NULL, 0, out, 0, NULL, 0); +} + +static int check_no_changes(const char *prefix, int include_untracked, + int include_ignored, const char **argv) +{ + struct argv_array args1 = ARGV_ARRAY_INIT; + struct argv_array args2 = ARGV_ARRAY_INIT; + struct strbuf out = STRBUF_INIT; + int ret; + + argv_array_pushl(&args1, "diff-index", "--quiet", "--cached", "HEAD", + "--ignore-submodules", "--", NULL); + if (argv) + argv_array_pushv(&args1, argv); + + argv_array_pushl(&args2, "diff-files", "--quiet", "--ignore-submodules", + "--", NULL); + if (argv) + argv_array_pushv(&args2, argv); + + if (include_untracked) + untracked_files(&out, include_untracked, include_ignored, argv); + + ret = cmd_diff_index(args1.argc, args1.argv, prefix) == 0 && + cmd_diff_files(args2.argc, args2.argv, prefix) == 0 && + (!include_untracked || out.len == 0); + strbuf_release(&out); + return ret; +} + +static int get_stash_info(struct stash_info *info, const char *commit) +{ + struct strbuf w_commit_rev = STRBUF_INIT; + struct strbuf b_commit_rev = STRBUF_INIT; + struct strbuf w_tree_rev = STRBUF_INIT; + struct strbuf b_tree_rev = STRBUF_INIT; + struct strbuf i_tree_rev = STRBUF_INIT; + struct strbuf u_tree_rev = STRBUF_INIT; + struct strbuf commit_buf = STRBUF_INIT; + struct strbuf symbolic = STRBUF_INIT; + struct strbuf out = STRBUF_INIT; + int ret; + const char *revision = commit; + char *end_of_rev; + struct child_process cp = CHILD_PROCESS_INIT; + info->is_stash_ref = 0; + + if (commit == NULL) { + strbuf_addf(&commit_buf, "%s@{0}", ref_stash); + revision = commit_buf.buf; + } else if (strlen(commit) < 3) { + strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit); + revision = commit_buf.buf; + } + info->revision = revision; + + strbuf_addf(&w_commit_rev, "%s", revision); + strbuf_addf(&b_commit_rev, "%s^1", revision); + strbuf_addf(&w_tree_rev, "%s:", revision); + strbuf_addf(&b_tree_rev, "%s^1:", revision); + strbuf_addf(&i_tree_rev, "%s^2:", revision); + strbuf_addf(&u_tree_rev, "%s^3:", revision); + + ret = (get_sha1(w_commit_rev.buf, info->w_commit.hash) == 0 && + get_sha1(b_commit_rev.buf, info->b_commit.hash) == 0 && + get_sha1(w_tree_rev.buf, info->w_tree.hash) == 0 && + get_sha1(b_tree_rev.buf, info->b_tree.hash) == 0 && + get_sha1(i_tree_rev.buf, info->i_tree.hash) == 0); + + strbuf_release(&w_commit_rev); + strbuf_release(&b_commit_rev); + strbuf_release(&w_tree_rev); + strbuf_release(&b_tree_rev); + strbuf_release(&i_tree_rev); + + if (!ret) + return error(_("%s is not a valid reference"), revision); + + info->has_u = get_sha1(u_tree_rev.buf, info->u_tree.hash) == 0; + + strbuf_release(&u_tree_rev); + + end_of_rev = strchrnul(revision, '@'); + strbuf_add(&symbolic, revision, end_of_rev - revision); + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "rev-parse", "--symbolic-full-name", NULL); + argv_array_pushf(&cp.args, "%s", symbolic.buf); + strbuf_release(&symbolic); + pipe_command(&cp, NULL, 0, &out, 0, NULL, 0); + + if (out.len-1 == strlen(ref_stash)) + info->is_stash_ref = strncmp(out.buf, ref_stash, out.len-1) == 0; + strbuf_release(&out); + + return !ret; +} + +static void stash_create_callback(struct diff_queue_struct *q, + struct diff_options *opt, void *cbdata) +{ + int i; + + for (i = 0; i < q->nr; i++) { + struct diff_filepair *p = q->queue[i]; + const char *path = p->one->path; + struct stat st; + remove_file_from_index(&the_index, path); + if (!lstat(path, &st)) + add_to_index(&the_index, path, &st, 0); + } +} + +/* + * Untracked files are stored by themselves in a parentless commit, for + * ease of unpacking later. + */ +static int save_untracked(struct stash_info *info, const char *message, + int include_untracked, int include_ignored, const char **argv) +{ + struct child_process cp = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + struct object_id orig_tree; + int ret; + const char *index_file = get_index_file(); + + set_alternate_index_output(stash_index_path); + untracked_files(&out, include_untracked, include_ignored, argv); + + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "update-index", "-z", "--add", "--remove", + "--stdin", NULL); + argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path); + + if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) { + strbuf_release(&out); + return 1; + } + + strbuf_reset(&out); + + discard_cache(); + read_cache_from(stash_index_path); + + write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL); + discard_cache(); + + read_cache_from(stash_index_path); + + write_cache_as_tree(info->u_tree.hash, 0, NULL); + strbuf_addf(&out, "untracked files on %s", message); + + ret = commit_tree(out.buf, out.len, info->u_tree.hash, NULL, + info->u_commit.hash, NULL, NULL); + strbuf_release(&out); + if (ret) + return 1; + + set_alternate_index_output(index_file); + discard_cache(); + read_cache(); + + return 0; +} + +static int save_working_tree(struct stash_info *info, const char *prefix, + const char **argv) +{ + struct object_id orig_tree; + struct rev_info rev; + int nr_trees = 1; + struct tree_desc t[MAX_UNPACK_TREES]; + struct tree *tree; + struct unpack_trees_options opts; + struct object *obj; + + discard_cache(); + tree = parse_tree_indirect(&info->i_tree); + prime_cache_tree(&the_index, tree); + write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0, NULL); + discard_cache(); + + read_cache_from(stash_index_path); + + memset(&opts, 0, sizeof(opts)); + + parse_tree(tree); + + opts.head_idx = 1; + opts.src_index = &the_index; + opts.dst_index = &the_index; + opts.merge = 1; + opts.fn = oneway_merge; + + init_tree_desc(t, tree->buffer, tree->size); + + if (unpack_trees(nr_trees, t, &opts)) + return 1; + + init_revisions(&rev, prefix); + setup_revisions(0, NULL, &rev, NULL); + rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK; + rev.diffopt.format_callback = stash_create_callback; + DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS); + + parse_pathspec(&rev.prune_data, 0, 0, prefix, argv); + + diff_setup_done(&rev.diffopt); + obj = parse_object(&info->b_commit); + add_pending_object(&rev, obj, ""); + if (run_diff_index(&rev, 0)) + return 1; + + if (write_cache_as_tree(info->w_tree.hash, 0, NULL)) + return 1; + + discard_cache(); + read_cache(); + + return 0; +} + +static int patch_working_tree(struct stash_info *info, const char *prefix, + const char **argv) +{ + struct argv_array args = ARGV_ARRAY_INIT; + struct child_process cp = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + size_t unused; + const char *index_file = get_index_file(); + + argv_array_pushl(&args, "read-tree", "HEAD", NULL); + argv_array_pushf(&args, "--index-output=%s", stash_index_path); + cmd_read_tree(args.argc, args.argv, prefix); + + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "add--interactive", "--patch=stash", "--", NULL); + argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path); + if (run_command(&cp)) + return 1; + + discard_cache(); + read_cache_from(stash_index_path); + + if (write_cache_as_tree(info->w_tree.hash, 0, NULL)) + return 1; + + child_process_init(&cp); + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "diff-tree", "-p", "HEAD", NULL); + argv_array_push(&cp.args, sha1_to_hex(info->w_tree.hash)); + argv_array_push(&cp.args, "--"); + if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0) || out.len == 0) + return 1; + + info->patch = strbuf_detach(&out, &unused); + + set_alternate_index_output(index_file); + discard_cache(); + read_cache(); + + return 0; +} + +static int do_create_stash(struct stash_info *info, const char *prefix, + const char *message, int include_untracked, int include_ignored, + int patch, const char **argv) +{ + struct object_id curr_head; + char *branch_path = NULL; + const char *branch_name = NULL; + struct commit_list *parents = NULL; + struct strbuf out_message = STRBUF_INIT; + struct strbuf out = STRBUF_INIT; + struct pretty_print_context ctx = {0}; + + struct commit *c = NULL; + const char *hash; + + read_cache_preload(NULL); + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); + if (check_no_changes(prefix, include_untracked, include_ignored, argv)) + return 1; + + if (get_sha1_tree("HEAD", info->b_commit.hash)) + return error(_("You do not have the initial commit yet")); + + branch_path = resolve_refdup("HEAD", 0, curr_head.hash, NULL); + + if (branch_path == NULL || strcmp(branch_path, "HEAD") == 0) + branch_name = "(no branch)"; + else + skip_prefix(branch_path, "refs/heads/", &branch_name); + + c = lookup_commit(&info->b_commit); + + ctx.output_encoding = get_log_output_encoding(); + ctx.abbrev = 1; + ctx.fmt = CMIT_FMT_ONELINE; + hash = find_unique_abbrev(c->object.oid.hash, DEFAULT_ABBREV); + + strbuf_addf(&out_message, "%s: %s ", branch_name, hash); + + pretty_print_commit(&ctx, c, &out_message); + + strbuf_addf(&out, "index on %s\n", out_message.buf); + + commit_list_insert(lookup_commit(&info->b_commit), &parents); + + if (write_cache_as_tree(info->i_tree.hash, 0, NULL)) + return error(_("git write-tree failed to write a tree")); + + if (commit_tree(out.buf, out.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL)) + return error(_("Cannot save the current index state")); + + strbuf_reset(&out); + + if (include_untracked) { + if (save_untracked(info, out_message.buf, include_untracked, include_ignored, argv)) + return error(_("Cannot save the untracked files")); + } + + if (patch) { + if (patch_working_tree(info, prefix, argv)) + return error(_("Cannot save the current worktree state")); + } else { + if (save_working_tree(info, prefix, argv)) + return error(_("Cannot save the current worktree state")); + } + parents = NULL; + + if (include_untracked) + commit_list_insert(lookup_commit(&info->u_commit), &parents); + + commit_list_insert(lookup_commit(&info->i_commit), &parents); + commit_list_insert(lookup_commit(&info->b_commit), &parents); + + if (message != NULL && strlen(message) != 0) + strbuf_addf(&out, "On %s: %s\n", branch_name, message); + else + strbuf_addf(&out, "WIP on %s\n", out_message.buf); + + if (commit_tree(out.buf, out.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL)) + return error(_("Cannot record working tree state")); + + info->message = out.buf; + + strbuf_release(&out_message); + free(branch_path); + + return 0; +} + +static int create_stash(int argc, const char **argv, const char *prefix) +{ + int include_untracked = 0; + const char *message = NULL; + struct stash_info info; + int ret; + struct strbuf out = STRBUF_INIT; + struct option options[] = { + OPT_BOOL('u', "include-untracked", &include_untracked, + N_("stash untracked filed")), + OPT_STRING('m', "message", &message, N_("message"), + N_("stash commit message")), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, + git_stash_create_usage, 0); + + if (argc != 0) { + int i; + for (i = 0; i < argc; ++i) { + if (i != 0) { + strbuf_addf(&out, " "); + } + strbuf_addf(&out, "%s", argv[i]); + } + message = out.buf; + } + + ret = do_create_stash(&info, prefix, message, include_untracked, 0, 0, NULL); + + strbuf_release(&out); + + if (ret) + return 0; + + printf("%s\n", sha1_to_hex(info.w_commit.hash)); + return 0; +} + +static int do_store_stash(const char *prefix, int quiet, const char *message, + struct object_id commit) +{ + int ret; + ret = update_ref(message, ref_stash, commit.hash, NULL, + REF_FORCE_CREATE_REFLOG, UPDATE_REFS_DIE_ON_ERR); + + if (ret && !quiet) + return error(_("Cannot update %s with %s"), ref_stash, sha1_to_hex(commit.hash)); + + return ret; +} + +static int store_stash(int argc, const char **argv, const char *prefix) +{ + const char *message = "Create via \"git stash store\"."; + const char *commit = NULL; + struct object_id obj; + struct option options[] = { + OPT_STRING('m', "message", &message, N_("message"), + N_("stash commit message")), + OPT__QUIET(&quiet, N_("be quiet, only report errors")), + OPT_END() + }; + argc = parse_options(argc, argv, prefix, options, git_stash_store_usage, 0); + + if (argc != 1) + return error(_("\"git stash store\" requires one <commit> argument")); + + commit = argv[0]; + + if (get_sha1(commit, obj.hash)) { + fprintf_ln(stderr, _("fatal: %s: not a valid SHA1"), commit); + fprintf_ln(stderr, _("cannot update %s with %s"), ref_stash, commit); + return 1; + } + + return do_store_stash(prefix, quiet, message, obj); +} + +static int do_clear_stash(void) +{ + struct object_id obj; + if (get_sha1(ref_stash, obj.hash)) + return 0; + + return delete_ref(NULL, ref_stash, obj.hash, 0); +} + +static int clear_stash(int argc, const char **argv, const char *prefix) +{ + struct option options[] = { + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, git_stash_clear_usage, PARSE_OPT_STOP_AT_NON_OPTION); + + if (argc != 0) + return error(_("git stash clear with parameters is unimplemented")); + + return do_clear_stash(); +} + +static int reset_tree(struct object_id i_tree, int update, int reset) +{ + struct unpack_trees_options opts; + int nr_trees = 1; + struct tree_desc t[MAX_UNPACK_TREES]; + struct tree *tree; + + read_cache_preload(NULL); + if (refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL)) + return 1; + + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); + + memset(&opts, 0, sizeof(opts)); + + tree = parse_tree_indirect(&i_tree); + if (parse_tree(tree)) + return 1; + + init_tree_desc(t, tree->buffer, tree->size); + + opts.head_idx = 1; + opts.src_index = &the_index; + opts.dst_index = &the_index; + opts.merge = 1; + opts.reset = reset; + opts.update = update; + opts.fn = oneway_merge; + + if (unpack_trees(nr_trees, t, &opts)) + return 1; + + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) { + error(_("unable to write new index file")); + return 1; + } + + return 0; +} + +static int do_push_stash(const char *prefix, const char *message, + int keep_index, int include_untracked, int include_ignored, int patch, + const char **argv) +{ + int res; + struct stash_info info; + + if (patch && include_untracked) + return error(_("can't use --patch and --include-untracked or --all at the same time")); + + if (!include_untracked) { + struct child_process cp = CHILD_PROCESS_INIT; + + cp.git_cmd = 1; + cp.no_stdout = 1; + argv_array_pushl(&cp.args, "ls-files", "--error-unmatch", "--", NULL); + if (argv) + argv_array_pushv(&cp.args, argv); + res = run_command(&cp); + if (res) + return 1; + } + + read_cache_preload(NULL); + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); + if (check_no_changes(prefix, include_untracked, include_ignored, argv)) { + printf_ln(_("No local changes to save")); + return 0; + } + + if (!reflog_exists(ref_stash)) { + if (do_clear_stash()) + return error(_("Cannot initialize stash")); + } + + if (do_create_stash(&info, prefix, message, include_untracked, include_ignored, patch, argv)) + return 1; + res = do_store_stash(prefix, 1, info.message, info.w_commit); + + if (res == 0 && !quiet) + printf(_("Saved working directory and index state %s"), info.message); + + if (!patch) { + if (argv && *argv) { + struct argv_array args = ARGV_ARRAY_INIT; + struct argv_array args2 = ARGV_ARRAY_INIT; + struct child_process cp = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + argv_array_pushl(&args, "reset", "--quiet", "--", NULL); + argv_array_pushv(&args, argv); + cmd_reset(args.argc, args.argv, prefix); + + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "ls-files", "-z", "--modified", "--", + NULL); + argv_array_pushv(&cp.args, argv); + pipe_command(&cp, NULL, 0, &out, 0, NULL, 0); + + child_process_init(&cp); + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "checkout-index", "-z", "--force", + "--stdin", NULL); + pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0); + strbuf_release(&out); + + argv_array_pushl(&args2, "clean", "--force", "-d", "--quiet", "--", + NULL); + argv_array_pushv(&args2, argv); + cmd_clean(args2.argc, args2.argv, prefix); + } else { + struct argv_array args = ARGV_ARRAY_INIT; + argv_array_pushl(&args, "reset", "--hard", "--quiet", NULL); + cmd_reset(args.argc, args.argv, prefix); + } + + if (include_untracked) { + struct argv_array args = ARGV_ARRAY_INIT; + argv_array_pushl(&args, "clean", "--force", "--quiet", "-d", NULL); + if (include_ignored) + argv_array_push(&args, "-x"); + argv_array_push(&args, "--"); + if (argv) + argv_array_pushv(&args, argv); + cmd_clean(args.argc, args.argv, prefix); + } + + if (keep_index) { + struct child_process cp = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + + reset_tree(info.i_tree, 0, 1); + + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "ls-files", "-z", "--modified", "--", + NULL); + argv_array_pushv(&cp.args, argv); + if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0)) + return 1; + + child_process_init(&cp); + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "checkout-index", "-z", "--force", + "--stdin", NULL); + if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) + return 1; + strbuf_release(&out); + } + } else { + struct child_process cp = CHILD_PROCESS_INIT; + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "apply", "-R", NULL); + if (pipe_command(&cp, info.patch, strlen(info.patch), NULL, 0, NULL, 0)) + return error(_("Cannot remove worktree changes")); + + if (!keep_index) { + struct argv_array args = ARGV_ARRAY_INIT; + argv_array_pushl(&args, "reset", "--quiet", "--", NULL); + if (argv) + argv_array_pushv(&args, argv); + cmd_reset(args.argc, args.argv, prefix); + } + } + + return 0; +} + +static int push_stash(int argc, const char **argv, const char *prefix) +{ + const char *message = NULL; + int include_untracked = 0; + int include_ignored = 0; + int patch = 0; + int keep_index_set = -1; + int keep_index = 0; + struct option options[] = { + OPT_BOOL('u', "include-untracked", &include_untracked, + N_("stash untracked filed")), + OPT_BOOL('a', "all", &include_ignored, + N_("stash ignored untracked files")), + OPT_BOOL('k', "keep-index", &keep_index_set, + N_("restore the index after applying the stash")), + OPT__QUIET(&quiet, N_("be quiet, only report errors")), + OPT_STRING('m', "message", &message, N_("message"), + N_("stash commit message")), + OPT_BOOL('p', "patch", &patch, + N_("edit current diff and apply")), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, + git_stash_save_usage, PARSE_OPT_STOP_AT_NON_OPTION); + + if (include_ignored) + include_untracked = 1; + + if (keep_index_set != -1) + keep_index = keep_index_set; + else if (patch) + keep_index = 1; + + return do_push_stash(prefix, message, keep_index, include_untracked, include_ignored, patch, argv); +} + +static int save_stash(int argc, const char **argv, const char *prefix) +{ + const char *message = NULL; + int include_untracked = 0; + int include_ignored = 0; + int patch = 0; + int keep_index_set = -1; + int keep_index = 0; + int ret; + struct strbuf out = STRBUF_INIT; + struct option options[] = { + OPT_BOOL('u', "include-untracked", &include_untracked, + N_("stash untracked filed")), + OPT_BOOL('a', "all", &include_ignored, + N_("stash ignored untracked files")), + OPT_BOOL('k', "keep-index", &keep_index_set, + N_("restore the index after applying the stash")), + OPT__QUIET(&quiet, N_("be quiet, only report errors")), + OPT_STRING('m', "message", &message, N_("message"), + N_("stash commit message")), + OPT_BOOL('p', "patch", &patch, + N_("edit current diff and apply")), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, + git_stash_save_usage, PARSE_OPT_STOP_AT_NON_OPTION); + + if (include_ignored) + include_untracked = 1; + + if (keep_index_set != -1) + keep_index = keep_index_set; + else if (patch) + keep_index = 1; + + if (argc != 0) { + int i; + for (i = 0; i < argc; ++i) { + if (i != 0) + strbuf_addf(&out, " "); + strbuf_addf(&out, "%s", argv[i]); + } + message = out.buf; + } + + ret = do_push_stash(prefix, message, keep_index, include_untracked, + include_ignored, patch, NULL); + strbuf_release(&out); + return ret; +} + +static int do_apply_stash(const char *prefix, struct stash_info *info, int index) +{ + struct merge_options o; + struct object_id c_tree; + struct object_id index_tree; + const struct object_id *bases[1]; + int bases_count = 1; + struct commit *result; + int ret; + + read_cache_preload(NULL); + if (refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL)) + return 1; + + if (write_cache_as_tree(c_tree.hash, 0, NULL)) + return 1; + + if (index) { + if (hashcmp(info->b_tree.hash, info->i_tree.hash) == 0 || hashcmp(c_tree.hash, info->i_tree.hash) == 0) { + index = 0; + } else { + struct child_process cp = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + struct argv_array args = ARGV_ARRAY_INIT; + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL); + argv_array_pushf(&cp.args, "%s^2^..%s^2", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash)); + if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0)) + return 1; + + child_process_init(&cp); + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "apply", "--cached", NULL); + if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) + return 1; + + strbuf_release(&out); + discard_cache(); + read_cache(); + if (write_cache_as_tree(index_tree.hash, 0, NULL)) + return 1; + + argv_array_push(&args, "reset"); + cmd_reset(args.argc, args.argv, prefix); + } + } + + if (info->has_u) { + struct argv_array args = ARGV_ARRAY_INIT; + struct child_process cp = CHILD_PROCESS_INIT; + const char *index_file = get_index_file(); + + argv_array_push(&args, "read-tree"); + argv_array_push(&args, sha1_to_hex(info->u_tree.hash)); + argv_array_pushf(&args, "--index-output=%s", stash_index_path); + + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "checkout-index", "--all", NULL); + argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path); + + if (cmd_read_tree(args.argc, args.argv, prefix) || + run_command(&cp)) { + return error(_("Could not restore untracked files from stash")); + } + set_alternate_index_output(index_file); + } + + init_merge_options(&o); + + o.branch1 = "Updated upstream"; + o.branch2 = "Stashed changes"; + + if (hashcmp(info->b_tree.hash, c_tree.hash) == 0) + o.branch1 = "Version stash was based on"; + + if (quiet) + o.verbosity = 0; + + if (o.verbosity >= 3) + printf_ln(_("Merging %s with %s"), o.branch1, o.branch2); + + bases[0] = &info->b_tree; + + ret = merge_recursive_generic(&o, &c_tree, &info->w_tree, bases_count, bases, &result); + if (ret != 0) { + struct argv_array args = ARGV_ARRAY_INIT; + argv_array_push(&args, "rerere"); + cmd_rerere(args.argc, args.argv, prefix); + + if (index) + printf_ln(_("Index was not unstashed.")); + + return ret; + } + + if (index) { + ret = reset_tree(index_tree, 0, 0); + } else { + struct child_process cp = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "diff-index", "--cached", "--name-only", "--diff-filter=A", NULL); + argv_array_push(&cp.args, sha1_to_hex(c_tree.hash)); + ret = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0); + if (ret) + return 1; + + ret = reset_tree(c_tree, 0, 1); + if (ret) + return 1; + + child_process_init(&cp); + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "update-index", "--add", "--stdin", NULL); + ret = pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0); + if (ret) + return 1; + + strbuf_release(&out); + discard_cache(); + read_cache(); + } + + if (!quiet) { + struct argv_array args = ARGV_ARRAY_INIT; + argv_array_push(&args, "status"); + cmd_status(args.argc, args.argv, prefix); + } + + return 0; +} + +static int apply_stash(int argc, const char **argv, const char *prefix) +{ + const char *commit = NULL; + int index = 0; + struct stash_info info; + struct option options[] = { + OPT__QUIET(&quiet, N_("be quiet, only report errors")), + OPT_BOOL(0, "index", &index, + N_("attempt to ininstate the index")), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, + git_stash_apply_usage, 0); + + if (argc == 1) { + commit = argv[0]; + } + + if (get_stash_info(&info, commit)) + return 1; + + return do_apply_stash(prefix, &info, index); +} + +static int do_drop_stash(const char *prefix, struct stash_info *info) +{ + struct argv_array args = ARGV_ARRAY_INIT; + int ret; + struct child_process cp = CHILD_PROCESS_INIT; + + argv_array_pushl(&args, "reflog", "delete", "--updateref", "--rewrite", NULL); + argv_array_push(&args, info->revision); + ret = cmd_reflog(args.argc, args.argv, prefix); + if (ret == 0) { + if (!quiet) { + printf(_("Dropped %s (%s)\n"), info->revision, sha1_to_hex(info->w_commit.hash)); + } + } else { + return error(_("%s: Could not drop stash entry"), info->revision); + } + + cp.git_cmd = 1; + /* Even though --quiet is specified, rev-parse still outputs the hash */ + cp.no_stdout = 1; + argv_array_pushl(&cp.args, "rev-parse", "--verify", "--quiet", NULL); + argv_array_pushf(&cp.args, "%s@{0}", ref_stash); + ret = run_command(&cp); + + if (ret) + do_clear_stash(); + + return 0; +} + +static int drop_stash(int argc, const char **argv, const char *prefix) +{ + const char *commit = NULL; + struct stash_info info; + struct option options[] = { + OPT__QUIET(&quiet, N_("be quiet, only report errors")), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, + git_stash_drop_usage, 0); + + if (argc == 1) + commit = argv[0]; + + if (get_stash_info(&info, commit)) + return 1; + + if (!info.is_stash_ref) + return error(_("'%s' is not a stash reference"), commit); + + return do_drop_stash(prefix, &info); +} + +static int list_stash(int argc, const char **argv, const char *prefix) +{ + struct option options[] = { + OPT_END() + }; + + struct object_id obj; + struct argv_array args = ARGV_ARRAY_INIT; + int ret; + + argc = parse_options(argc, argv, prefix, options, + git_stash_list_usage, PARSE_OPT_KEEP_UNKNOWN); + + if (get_sha1(ref_stash, obj.hash)) + return 0; + + argv_array_pushl(&args, "log", "--format=%gd: %gs", "-g", "--first-parent", "-m", NULL); + argv_array_pushv(&args, argv); + argv_array_push(&args, ref_stash); + ret = cmd_log(args.argc, args.argv, prefix); + if (ret) + return 1; + + return 0; +} + +static int show_stash(int argc, const char **argv, const char *prefix) +{ + struct argv_array args = ARGV_ARRAY_INIT; + struct stash_info info; + const char *commit = NULL; + int numstat = 0; + int patch = 0; + int ret; + + struct option options[] = { + OPT_BOOL(0, "numstat", &numstat, + N_("Shows number of added and deleted lines in decimal notation")), + OPT_BOOL('p', "patch", &patch, + N_("Generate patch")), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, + git_stash_show_usage, 0); + + if (argc == 1) + commit = argv[0]; + + if (get_stash_info(&info, commit)) + return 1; + + argv_array_push(&args, "diff"); + if (numstat) + argv_array_push(&args, "--numstat"); + else if (patch) + argv_array_push(&args, "-p"); + else + argv_array_push(&args, "--stat"); + + argv_array_push(&args, sha1_to_hex(info.b_commit.hash)); + argv_array_push(&args, sha1_to_hex(info.w_commit.hash)); + ret = cmd_diff(args.argc, args.argv, prefix); + return ret; +} + +static int pop_stash(int argc, const char **argv, const char *prefix) +{ + int index = 0; + const char *commit = NULL; + struct stash_info info; + struct option options[] = { + OPT__QUIET(&quiet, N_("be quiet, only report errors")), + OPT_BOOL(0, "index", &index, + N_("attempt to ininstate the index")), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, + git_stash_pop_usage, 0); + + if (argc == 1) + commit = argv[0]; + + if (get_stash_info(&info, commit)) + return 1; + + if (!info.is_stash_ref) + return error(_("'%s' is not a stash reference"), commit); + + if (do_apply_stash(prefix, &info, index)) + return 1; + + return do_drop_stash(prefix, &info); +} + +static int branch_stash(int argc, const char **argv, const char *prefix) +{ + const char *commit = NULL, *branch = NULL; + int ret; + struct argv_array args = ARGV_ARRAY_INIT; + struct stash_info info; + struct option options[] = { + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, + git_stash_branch_usage, 0); + + if (argc != 0) { + branch = argv[0]; + if (argc == 2) + commit = argv[1]; + } + + if (get_stash_info(&info, commit)) + return 1; + + argv_array_pushl(&args, "checkout", "-b", NULL); + argv_array_push(&args, branch); + argv_array_push(&args, sha1_to_hex(info.b_commit.hash)); + ret = cmd_checkout(args.argc, args.argv, prefix); + if (ret) + return 1; + + ret = do_apply_stash(prefix, &info, 1); + if (ret == 0 && info.is_stash_ref) + ret = do_drop_stash(prefix, &info); + + return ret; +} + +int cmd_stash(int argc, const char **argv, const char *prefix) +{ + int result = 0; + pid_t pid = getpid(); + + struct option options[] = { + OPT_END() + }; + + git_config(git_default_config, NULL); + + xsnprintf(stash_index_path, 64, ".git/index.stash.%d", pid); + + argc = parse_options(argc, argv, prefix, options, git_stash_usage, + PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH); + + if (argc < 1) { + result = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL); + } else if (!strcmp(argv[0], "list")) + result = list_stash(argc, argv, prefix); + else if (!strcmp(argv[0], "show")) + result = show_stash(argc, argv, prefix); + else if (!strcmp(argv[0], "save")) + result = save_stash(argc, argv, prefix); + else if (!strcmp(argv[0], "push")) + result = push_stash(argc, argv, prefix); + else if (!strcmp(argv[0], "apply")) + result = apply_stash(argc, argv, prefix); + else if (!strcmp(argv[0], "clear")) + result = clear_stash(argc, argv, prefix); + else if (!strcmp(argv[0], "create")) + result = create_stash(argc, argv, prefix); + else if (!strcmp(argv[0], "store")) + result = store_stash(argc, argv, prefix); + else if (!strcmp(argv[0], "drop")) + result = drop_stash(argc, argv, prefix); + else if (!strcmp(argv[0], "pop")) + result = pop_stash(argc, argv, prefix); + else if (!strcmp(argv[0], "branch")) + result = branch_stash(argc, argv, prefix); + else { + if (argv[0][0] == '-') { + struct argv_array args = ARGV_ARRAY_INIT; + argv_array_push(&args, "push"); + argv_array_pushv(&args, argv); + result = push_stash(args.argc, args.argv, prefix); + if (!result) + printf_ln(_("To restore them type \"git stash apply\"")); + } else { + error(_("unknown subcommand: %s"), argv[0]); + result = 1; + } + } + + return result; +} diff --git a/git-stash.sh b/contrib/examples/git-stash.sh similarity index 100% rename from git-stash.sh rename to contrib/examples/git-stash.sh diff --git a/git.c b/git.c index 8ff44f081d..4531011cdc 100644 --- a/git.c +++ b/git.c @@ -491,6 +491,7 @@ static struct cmd_struct commands[] = { { "show-branch", cmd_show_branch, RUN_SETUP }, { "show-ref", cmd_show_ref, RUN_SETUP }, { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE }, + { "stash", cmd_stash, RUN_SETUP | NEED_WORK_TREE }, { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE }, { "stripspace", cmd_stripspace }, { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX}, -- 2.13.0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-08 0:55 ` [PATCH v4 5/5] stash: implement builtin stash Joel Teichroeb @ 2017-06-11 21:27 ` Thomas Gummerer 2017-06-20 2:37 ` Joel Teichroeb 2017-06-16 16:15 ` Junio C Hamano ` (2 subsequent siblings) 3 siblings, 1 reply; 26+ messages in thread From: Thomas Gummerer @ 2017-06-11 21:27 UTC (permalink / raw) To: Joel Teichroeb Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder On 06/07, Joel Teichroeb wrote: > Implement all git stash functionality as a builtin command > > Signed-off-by: Joel Teichroeb <joel@teichroeb.net> > --- Thanks for working on this. A few comments from me below. Mainly on stash push, as that's what I'm most familiar with, and all I had time for today. Hope it helps :) > Makefile | 2 +- > builtin.h | 1 + > builtin/stash.c | 1224 +++++++++++++++++++++++++ > git-stash.sh => contrib/examples/git-stash.sh | 0 > git.c | 1 + > 5 files changed, 1227 insertions(+), 1 deletion(-) > create mode 100644 builtin/stash.c > rename git-stash.sh => contrib/examples/git-stash.sh (100%) [...] > > + > +static const char * const git_stash_usage[] = { > + N_("git stash list [<options>]"), > + N_("git stash show [<stash>]"), > + N_("git stash drop [-q|--quiet] [<stash>]"), > + N_("git stash ( pop | apply ) [--index] [-q|--quiet] [<stash>]"), > + N_("git stash branch <branchname> [<stash>]"), > + N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"), > + N_(" [-u|--include-untracked] [-a|--all] [<message>]]"), This is missing the newly introduced push command. > + N_("git stash clear"), > + N_("git stash create [<message>]"), > + N_("git stash store [-m|--message <message>] [-q|--quiet] <commit>"), create and store are not currently advertised in the usage. I think this is intentional, because those commands are intended to be used only in scripts. I don't have a particularly strong opinion on whether they should be added or not, but if we do add them I think we should do so consciously in a separate commit, instead of adding them on in this commit. > + NULL > +}; > + > +static const char * const git_stash_list_usage[] = { > + N_("git stash list [<options>]"), > + NULL > +}; > + > +static const char * const git_stash_show_usage[] = { > + N_("git stash show [<stash>]"), > + NULL > +}; > + > +static const char * const git_stash_drop_usage[] = { > + N_("git stash drop [-q|--quiet] [<stash>]"), > + NULL > +}; > + > +static const char * const git_stash_pop_usage[] = { > + N_("git stash pop [--index] [-q|--quiet] [<stash>]"), > + NULL > +}; > + > +static const char * const git_stash_apply_usage[] = { > + N_("git stash apply [--index] [-q|--quiet] [<stash>]"), > + NULL > +}; > + > +static const char * const git_stash_branch_usage[] = { > + N_("git stash branch <branchname> [<stash>]"), > + NULL > +}; > + > +static const char * const git_stash_save_usage[] = { > + N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"), > + N_(" [-u|--include-untracked] [-a|--all] [<message>]]"), > + NULL > +}; > + > +static const char * const git_stash_clear_usage[] = { > + N_("git stash clear"), > + NULL > +}; > + > +static const char * const git_stash_create_usage[] = { > + N_("git stash create [<message>]"), > + NULL > +}; > + > +static const char * const git_stash_store_usage[] = { > + N_("git stash store [-m|--message <message>] [-q|--quiet] <commit>"), > + NULL > +}; > + [...] > + > +static int do_push_stash(const char *prefix, const char *message, > + int keep_index, int include_untracked, int include_ignored, int patch, > + const char **argv) argv here is a list of pathspecs. I think this would be a bit easier to follow if the argument was called "pathspecs". > +{ > + int res; > + struct stash_info info; > + > + if (patch && include_untracked) > + return error(_("can't use --patch and --include-untracked or --all at the same time")); > + > + if (!include_untracked) { > + struct child_process cp = CHILD_PROCESS_INIT; > + > + cp.git_cmd = 1; > + cp.no_stdout = 1; > + argv_array_pushl(&cp.args, "ls-files", "--error-unmatch", "--", NULL); > + if (argv) > + argv_array_pushv(&cp.args, argv); > + res = run_command(&cp); > + if (res) > + return 1; > + } > + > + read_cache_preload(NULL); > + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); > + if (check_no_changes(prefix, include_untracked, include_ignored, argv)) { > + printf_ln(_("No local changes to save")); > + return 0; > + } > + > + if (!reflog_exists(ref_stash)) { > + if (do_clear_stash()) > + return error(_("Cannot initialize stash")); > + } > + > + if (do_create_stash(&info, prefix, message, include_untracked, include_ignored, patch, argv)) > + return 1; > + res = do_store_stash(prefix, 1, info.message, info.w_commit); > + > + if (res == 0 && !quiet) Sometimes the function is used directly in the if, and sometimes the res variable is used. I think it would be nicer to consistently use one or the other. My preference would be to always use the functions directly, as res is not used anywhere other than the if. Also I think we prefer using (!res) instead of (res == 0) for checking return values. > + printf(_("Saved working directory and index state %s"), info.message); > + > + if (!patch) { > + if (argv && *argv) { > + struct argv_array args = ARGV_ARRAY_INIT; > + struct argv_array args2 = ARGV_ARRAY_INIT; > + struct child_process cp = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + argv_array_pushl(&args, "reset", "--quiet", "--", NULL); > + argv_array_pushv(&args, argv); > + cmd_reset(args.argc, args.argv, prefix); > + > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "ls-files", "-z", "--modified", "--", > + NULL); > + argv_array_pushv(&cp.args, argv); > + pipe_command(&cp, NULL, 0, &out, 0, NULL, 0); > + > + child_process_init(&cp); > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "checkout-index", "-z", "--force", > + "--stdin", NULL); > + pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0); > + strbuf_release(&out); > + > + argv_array_pushl(&args2, "clean", "--force", "-d", "--quiet", "--", > + NULL); > + argv_array_pushv(&args2, argv); > + cmd_clean(args2.argc, args2.argv, prefix); > + } else { > + struct argv_array args = ARGV_ARRAY_INIT; > + argv_array_pushl(&args, "reset", "--hard", "--quiet", NULL); > + cmd_reset(args.argc, args.argv, prefix); > + } > + > + if (include_untracked) { > + struct argv_array args = ARGV_ARRAY_INIT; > + argv_array_pushl(&args, "clean", "--force", "--quiet", "-d", NULL); > + if (include_ignored) > + argv_array_push(&args, "-x"); > + argv_array_push(&args, "--"); > + if (argv) > + argv_array_pushv(&args, argv); > + cmd_clean(args.argc, args.argv, prefix); > + } > + > + if (keep_index) { > + struct child_process cp = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + > + reset_tree(info.i_tree, 0, 1); > + > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "ls-files", "-z", "--modified", "--", > + NULL); > + argv_array_pushv(&cp.args, argv); > + if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0)) > + return 1; > + > + child_process_init(&cp); > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "checkout-index", "-z", "--force", > + "--stdin", NULL); > + if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) > + return 1; > + strbuf_release(&out); > + } > + } else { > + struct child_process cp = CHILD_PROCESS_INIT; > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "apply", "-R", NULL); > + if (pipe_command(&cp, info.patch, strlen(info.patch), NULL, 0, NULL, 0)) > + return error(_("Cannot remove worktree changes")); > + > + if (!keep_index) { > + struct argv_array args = ARGV_ARRAY_INIT; > + argv_array_pushl(&args, "reset", "--quiet", "--", NULL); > + if (argv) > + argv_array_pushv(&args, argv); > + cmd_reset(args.argc, args.argv, prefix); > + } > + } > + > + return 0; > +} > + > +static int push_stash(int argc, const char **argv, const char *prefix) > +{ > + const char *message = NULL; > + int include_untracked = 0; > + int include_ignored = 0; > + int patch = 0; > + int keep_index_set = -1; > + int keep_index = 0; > + struct option options[] = { > + OPT_BOOL('u', "include-untracked", &include_untracked, > + N_("stash untracked filed")), > + OPT_BOOL('a', "all", &include_ignored, > + N_("stash ignored untracked files")), > + OPT_BOOL('k', "keep-index", &keep_index_set, > + N_("restore the index after applying the stash")), > + OPT__QUIET(&quiet, N_("be quiet, only report errors")), > + OPT_STRING('m', "message", &message, N_("message"), > + N_("stash commit message")), > + OPT_BOOL('p', "patch", &patch, > + N_("edit current diff and apply")), > + OPT_END() > + }; > + > + argc = parse_options(argc, argv, prefix, options, > + git_stash_save_usage, PARSE_OPT_STOP_AT_NON_OPTION); "git_stash_save_usage" is slightly different from the usage for push, which we should display here. We probably should introduce "git_stash_push_usage" for this. > + if (include_ignored) > + include_untracked = 1; > + > + if (keep_index_set != -1) > + keep_index = keep_index_set; > + else if (patch) > + keep_index = 1; > + > + return do_push_stash(prefix, message, keep_index, include_untracked, include_ignored, patch, argv); > +} > + [...] > + > +int cmd_stash(int argc, const char **argv, const char *prefix) > +{ > + int result = 0; > + pid_t pid = getpid(); > + > + struct option options[] = { > + OPT_END() > + }; > + > + git_config(git_default_config, NULL); > + > + xsnprintf(stash_index_path, 64, ".git/index.stash.%d", pid); > + > + argc = parse_options(argc, argv, prefix, options, git_stash_usage, > + PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH); > + > + if (argc < 1) { > + result = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL); > + } else if (!strcmp(argv[0], "list")) > + result = list_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "show")) > + result = show_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "save")) > + result = save_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "push")) > + result = push_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "apply")) > + result = apply_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "clear")) > + result = clear_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "create")) > + result = create_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "store")) > + result = store_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "drop")) > + result = drop_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "pop")) > + result = pop_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "branch")) > + result = branch_stash(argc, argv, prefix); > + else { > + if (argv[0][0] == '-') { > + struct argv_array args = ARGV_ARRAY_INIT; > + argv_array_push(&args, "push"); > + argv_array_pushv(&args, argv); > + result = push_stash(args.argc, args.argv, prefix); This is a bit of a change in behaviour to what we currently have. The rules we decided on are as follows: - "git stash -p" is an alias for "git stash push -p". - "git stash" with only option arguments is an alias for "git stash push" with those same arguments. non-option arguments can be specified after a "--" for disambiguation. The above makes "git stash -*" a alias for "git stash push -*". This would result in a change of behaviour, for example in the case where someone would use "git stash -this is a test-". In that case the current behaviour is to create a stash with the message "-this is a test-", while the above would end up making git stash error out. The discussion on how we came up with those rules can be found at http://public-inbox.org/git/20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net/. > + if (!result) > + printf_ln(_("To restore them type \"git stash apply\"")); In the shell script this is only displayed when the stash_push in the case where git stash is invoked with no arguments, not in the push case if I read this correctly. So the two lines above should go in the (argc < 1) case I think. > + } else { > + error(_("unknown subcommand: %s"), argv[0]); Currently we're displaying the whole usage string in this case. I think we should keep doing that. > + result = 1; > + } > + } > + > + return result; > +} > diff --git a/git-stash.sh b/contrib/examples/git-stash.sh > similarity index 100% > rename from git-stash.sh > rename to contrib/examples/git-stash.sh > diff --git a/git.c b/git.c > index 8ff44f081d..4531011cdc 100644 > --- a/git.c > +++ b/git.c > @@ -491,6 +491,7 @@ static struct cmd_struct commands[] = { > { "show-branch", cmd_show_branch, RUN_SETUP }, > { "show-ref", cmd_show_ref, RUN_SETUP }, > { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE }, > + { "stash", cmd_stash, RUN_SETUP | NEED_WORK_TREE }, > { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE }, > { "stripspace", cmd_stripspace }, > { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX}, > -- > 2.13.0 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-11 21:27 ` Thomas Gummerer @ 2017-06-20 2:37 ` Joel Teichroeb 2017-06-25 21:09 ` Thomas Gummerer 0 siblings, 1 reply; 26+ messages in thread From: Joel Teichroeb @ 2017-06-20 2:37 UTC (permalink / raw) To: Thomas Gummerer Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder On Sun, Jun 11, 2017 at 2:27 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote: >> + >> +int cmd_stash(int argc, const char **argv, const char *prefix) >> +{ >> + int result = 0; >> + pid_t pid = getpid(); >> + >> + struct option options[] = { >> + OPT_END() >> + }; >> + >> + git_config(git_default_config, NULL); >> + >> + xsnprintf(stash_index_path, 64, ".git/index.stash.%d", pid); >> + >> + argc = parse_options(argc, argv, prefix, options, git_stash_usage, >> + PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH); >> + >> + if (argc < 1) { >> + result = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL); >> + } else if (!strcmp(argv[0], "list")) >> + result = list_stash(argc, argv, prefix); >> + else if (!strcmp(argv[0], "show")) >> + result = show_stash(argc, argv, prefix); >> + else if (!strcmp(argv[0], "save")) >> + result = save_stash(argc, argv, prefix); >> + else if (!strcmp(argv[0], "push")) >> + result = push_stash(argc, argv, prefix); >> + else if (!strcmp(argv[0], "apply")) >> + result = apply_stash(argc, argv, prefix); >> + else if (!strcmp(argv[0], "clear")) >> + result = clear_stash(argc, argv, prefix); >> + else if (!strcmp(argv[0], "create")) >> + result = create_stash(argc, argv, prefix); >> + else if (!strcmp(argv[0], "store")) >> + result = store_stash(argc, argv, prefix); >> + else if (!strcmp(argv[0], "drop")) >> + result = drop_stash(argc, argv, prefix); >> + else if (!strcmp(argv[0], "pop")) >> + result = pop_stash(argc, argv, prefix); >> + else if (!strcmp(argv[0], "branch")) >> + result = branch_stash(argc, argv, prefix); >> + else { >> + if (argv[0][0] == '-') { >> + struct argv_array args = ARGV_ARRAY_INIT; >> + argv_array_push(&args, "push"); >> + argv_array_pushv(&args, argv); >> + result = push_stash(args.argc, args.argv, prefix); > > This is a bit of a change in behaviour to what we currently have. > > The rules we decided on are as follows: > > - "git stash -p" is an alias for "git stash push -p". > - "git stash" with only option arguments is an alias for "git stash > push" with those same arguments. non-option arguments can be > specified after a "--" for disambiguation. > > The above makes "git stash -*" a alias for "git stash push -*". This > would result in a change of behaviour, for example in the case where > someone would use "git stash -this is a test-". In that case the > current behaviour is to create a stash with the message "-this is a > test-", while the above would end up making git stash error out. The > discussion on how we came up with those rules can be found at > http://public-inbox.org/git/20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net/. I don't really like the "argv[0][0] == '-'" logic, but it doesn't seem to have the flaw you pointed out: $ ./git stash -this is a test- error: unknown switch `t' usage: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet] [...] I'm not sure this is the best possible error message, but it's just as useful as the message from the old version. > >> + if (!result) >> + printf_ln(_("To restore them type \"git stash apply\"")); > > In the shell script this is only displayed when the stash_push in the > case where git stash is invoked with no arguments, not in the push > case if I read this correctly. So the two lines above should go in > the (argc < 1) case I think. I think it's correct as is. One of the tests checks for this string to be output, and if I move the line, the test fails. I agreed with all the other points you raised, and they will be fixed in my next revision. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-20 2:37 ` Joel Teichroeb @ 2017-06-25 21:09 ` Thomas Gummerer 2017-06-26 7:53 ` Matthieu Moy 0 siblings, 1 reply; 26+ messages in thread From: Thomas Gummerer @ 2017-06-25 21:09 UTC (permalink / raw) To: Joel Teichroeb Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder, Matthieu Moy On 06/19, Joel Teichroeb wrote: > On Sun, Jun 11, 2017 at 2:27 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote: > >> + > >> +int cmd_stash(int argc, const char **argv, const char *prefix) > >> +{ > >> + int result = 0; > >> + pid_t pid = getpid(); > >> + > >> + struct option options[] = { > >> + OPT_END() > >> + }; > >> + > >> + git_config(git_default_config, NULL); > >> + > >> + xsnprintf(stash_index_path, 64, ".git/index.stash.%d", pid); > >> + > >> + argc = parse_options(argc, argv, prefix, options, git_stash_usage, > >> + PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH); > >> + > >> + if (argc < 1) { > >> + result = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL); > >> + } else if (!strcmp(argv[0], "list")) > >> + result = list_stash(argc, argv, prefix); > >> + else if (!strcmp(argv[0], "show")) > >> + result = show_stash(argc, argv, prefix); > >> + else if (!strcmp(argv[0], "save")) > >> + result = save_stash(argc, argv, prefix); > >> + else if (!strcmp(argv[0], "push")) > >> + result = push_stash(argc, argv, prefix); > >> + else if (!strcmp(argv[0], "apply")) > >> + result = apply_stash(argc, argv, prefix); > >> + else if (!strcmp(argv[0], "clear")) > >> + result = clear_stash(argc, argv, prefix); > >> + else if (!strcmp(argv[0], "create")) > >> + result = create_stash(argc, argv, prefix); > >> + else if (!strcmp(argv[0], "store")) > >> + result = store_stash(argc, argv, prefix); > >> + else if (!strcmp(argv[0], "drop")) > >> + result = drop_stash(argc, argv, prefix); > >> + else if (!strcmp(argv[0], "pop")) > >> + result = pop_stash(argc, argv, prefix); > >> + else if (!strcmp(argv[0], "branch")) > >> + result = branch_stash(argc, argv, prefix); > >> + else { > >> + if (argv[0][0] == '-') { > >> + struct argv_array args = ARGV_ARRAY_INIT; > >> + argv_array_push(&args, "push"); > >> + argv_array_pushv(&args, argv); > >> + result = push_stash(args.argc, args.argv, prefix); > > > > This is a bit of a change in behaviour to what we currently have. > > > > The rules we decided on are as follows: > > > > - "git stash -p" is an alias for "git stash push -p". > > - "git stash" with only option arguments is an alias for "git stash > > push" with those same arguments. non-option arguments can be > > specified after a "--" for disambiguation. > > > > The above makes "git stash -*" a alias for "git stash push -*". This > > would result in a change of behaviour, for example in the case where > > someone would use "git stash -this is a test-". In that case the > > current behaviour is to create a stash with the message "-this is a > > test-", while the above would end up making git stash error out. The > > discussion on how we came up with those rules can be found at > > http://public-inbox.org/git/20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net/. > > I don't really like the "argv[0][0] == '-'" logic, but it doesn't seem > to have the flaw you pointed out: > $ ./git stash -this is a test- > error: unknown switch `t' > usage: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet] > [...] I just went through the thread again, to remind myself why we did it this way. The example I had above was the wrong example, sorry. The message at [1] explains it better. Essentially by implementing the rules I mentioned we wanted to avoid the potential confusion of what does 'git stash -q drop' mean. Before the rewrite this fails and shows the usage. After the rewrite this would try to stash everything matching the pathspec drop, which might be confusing for users. [1]: http://public-inbox.org/git/20170213214521.pkjesijdlus36tnp@sigill.intra.peff.net/ > I'm not sure this is the best possible error message, but it's just as > useful as the message from the old version. > > > > >> + if (!result) > >> + printf_ln(_("To restore them type \"git stash apply\"")); > > > > In the shell script this is only displayed when the stash_push in the > > case where git stash is invoked with no arguments, not in the push > > case if I read this correctly. So the two lines above should go in > > the (argc < 1) case I think. > > I think it's correct as is. One of the tests checks for this string to > be output, and if I move the line, the test fails. Right, that test that fails only when the "To restore..." string is printed to stdout. So moving the "printf_ln()" to the line you did only makes sure it's not printed there. Reading the code again and trying to trigger this print in the shell script stash makes me think this is not even possible to trigger there anymore. After the line test -n "$seen_non_option" || set "push" "$@" it's not possible that $# is 0 anymore, so this will never be printed. From a quick look at the history it seems like it wasn't possible to trigger that codepath for a while. If I'm reading things correctly 3c2eb80fe3 ("stash: simplify defaulting to "save" and reject unknown options", 2009-08-18) seems to have introduced the small change in behaviour. As I don't think anyone has complained since then, I'd just leave it as is, which makes git stash with no options a little less verbose. [Adding Matthieu to cc as author of the above mentioned commit] > I agreed with all the other points you raised, and they will be fixed > in my next revision. Thanks! ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-25 21:09 ` Thomas Gummerer @ 2017-06-26 7:53 ` Matthieu Moy 2017-06-27 14:53 ` Thomas Gummerer 0 siblings, 1 reply; 26+ messages in thread From: Matthieu Moy @ 2017-06-26 7:53 UTC (permalink / raw) To: Thomas Gummerer Cc: Joel Teichroeb, Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Thomas Gummerer <t.gummerer@gmail.com> writes: > After the line > > test -n "$seen_non_option" || set "push" "$@" > > it's not possible that $# is 0 anymore, so this will never be > printed. From a quick look at the history it seems like it wasn't > possible to trigger that codepath for a while. If I'm reading things > correctly 3c2eb80fe3 ("stash: simplify defaulting to "save" and reject > unknown options", 2009-08-18) seems to have introduced the small > change in behaviour. Indeed. That wasn't on purpose, but I seem to have turned this case $# in 0) push_stash && say "$(gettext "(To restore them type \"git stash apply\")")" ;; into dead code. > As I don't think anyone has complained since then, I'd just leave it > as is, which makes git stash with no options a little less verbose. I agree it's OK to keep is as-is, but the original logic (give a bit more advice when "stash push" was DWIM-ed) made sense too, so it can make sense to re-activate it while porting to C. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-26 7:53 ` Matthieu Moy @ 2017-06-27 14:53 ` Thomas Gummerer 0 siblings, 0 replies; 26+ messages in thread From: Thomas Gummerer @ 2017-06-27 14:53 UTC (permalink / raw) To: Matthieu Moy Cc: Joel Teichroeb, Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder On Mon, Jun 26, 2017 at 8:53 AM Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote: > > Thomas Gummerer <t.gummerer@gmail.com> writes: > > > After the line > > > > test -n "$seen_non_option" || set "push" "$@" > > > > it's not possible that $# is 0 anymore, so this will never be > > printed. From a quick look at the history it seems like it wasn't > > possible to trigger that codepath for a while. If I'm reading things > > correctly 3c2eb80fe3 ("stash: simplify defaulting to "save" and reject > > unknown options", 2009-08-18) seems to have introduced the small > > change in behaviour. > > Indeed. That wasn't on purpose, but I seem to have turned this > > case $# in > 0) > push_stash && > say "$(gettext "(To restore them type \"git stash apply\")")" > ;; > > into dead code. > > > As I don't think anyone has complained since then, I'd just leave it > > as is, which makes git stash with no options a little less verbose. > > I agree it's OK to keep is as-is, but the original logic (give a bit > more advice when "stash push" was DWIM-ed) made sense too, so it can > make sense to re-activate it while porting to C. I'd be happy either way. If we decide to restore the original behaviour, I think it should be done in a separate patch, as the test case will need some adjustments. That way we can keep this patch purely as the conversion step, which makes it a bit easier to review. > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-08 0:55 ` [PATCH v4 5/5] stash: implement builtin stash Joel Teichroeb 2017-06-11 21:27 ` Thomas Gummerer @ 2017-06-16 16:15 ` Junio C Hamano 2017-06-16 22:47 ` Junio C Hamano 2017-06-22 17:07 ` Junio C Hamano 3 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2017-06-16 16:15 UTC (permalink / raw) To: Joel Teichroeb Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Joel Teichroeb <joel@teichroeb.net> writes: > diff --git a/builtin/stash.c b/builtin/stash.c > new file mode 100644 > index 0000000000..a9680f2909 > --- /dev/null > +++ b/builtin/stash.c > ... > +static const char *ref_stash = "refs/stash"; > +static int quiet = 0; Let BSS take care of zero-initialization, i.e. drop " = 0". > +static int untracked_files(struct strbuf *out, int include_untracked, > + int include_ignored, const char **argv) > +{ > + struct child_process cp = CHILD_PROCESS_INIT; > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "ls-files", "-o", "-z", NULL); > + if (include_untracked && !include_ignored) > + argv_array_push(&cp.args, "--exclude-standard"); > + argv_array_push(&cp.args, "--"); > + if (argv) > + argv_array_pushv(&cp.args, argv); > + return pipe_command(&cp, NULL, 0, out, 0, NULL, 0); > +} Seeing that include_untracked and include_ignored always come in a pair throughout the program, I wondered if it may be better to use a single "unsigned include" with two bits #define INCLUDE_UNTRACKED 01 #define INCLUDE_IGNORED 02 to pass around. As long as we envision that we will not gain other kind of "do we include X?" in the future, what your patch does is fine, I would say. > +static int check_no_changes(const char *prefix, int include_untracked, > + int include_ignored, const char **argv) > +{ > + struct argv_array args1 = ARGV_ARRAY_INIT; > + struct argv_array args2 = ARGV_ARRAY_INIT; > + struct strbuf out = STRBUF_INIT; > + int ret; > + > + argv_array_pushl(&args1, "diff-index", "--quiet", "--cached", "HEAD", > + "--ignore-submodules", "--", NULL); > + if (argv) > + argv_array_pushv(&args1, argv); > + > + argv_array_pushl(&args2, "diff-files", "--quiet", "--ignore-submodules", > + "--", NULL); > + if (argv) > + argv_array_pushv(&args2, argv); > + > + if (include_untracked) > + untracked_files(&out, include_untracked, include_ignored, argv); > + > + ret = cmd_diff_index(args1.argc, args1.argv, prefix) == 0 && > + cmd_diff_files(args2.argc, args2.argv, prefix) == 0 && > + (!include_untracked || out.len == 0); When diff_index() finds there are modified paths, you do not have to call diff_files() or untracked_files() at all (and you do not even have to set-up args2). Doesn't the above leak args.argv[] when && short circuits? This is a tangent, but it is somewhat unusual to call cmd_foo() as a subroutine. I think cmd_diff_*() are written reasonably well to allow them to be called in this way safely, and there are a few existing commands that already do so, so it may be OK. > + strbuf_release(&out); > + return ret; > +} > + > +static int get_stash_info(struct stash_info *info, const char *commit) > +{ > + struct strbuf w_commit_rev = STRBUF_INIT; > + struct strbuf b_commit_rev = STRBUF_INIT; > + struct strbuf w_tree_rev = STRBUF_INIT; > + struct strbuf b_tree_rev = STRBUF_INIT; > + struct strbuf i_tree_rev = STRBUF_INIT; > + struct strbuf u_tree_rev = STRBUF_INIT; > + struct strbuf commit_buf = STRBUF_INIT; > + struct strbuf symbolic = STRBUF_INIT; > + struct strbuf out = STRBUF_INIT; > + int ret; > + const char *revision = commit; > + char *end_of_rev; > + struct child_process cp = CHILD_PROCESS_INIT; > + info->is_stash_ref = 0; > + > + if (commit == NULL) { > + strbuf_addf(&commit_buf, "%s@{0}", ref_stash); > + revision = commit_buf.buf; > + } else if (strlen(commit) < 3) { This is a bit sloppy (the original is even sloppier but it is in shell, so it is more excusable ;-). This code thinks that anything with @{<num>} must be longer than 3 because it has to have @{}, and that is where the strlen() comes from, I think, but the magic number 3 appears without explanation here. What the code actually needs to do is to see if the stash entry specification came in "commit" (which by the way is a bit misnamed parameter) is a bare number and use refs/stash@{<that number>} only in that case, I think. strspn() might be useful. > + strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit); > + revision = commit_buf.buf; > + } > + info->revision = revision; > + > + strbuf_addf(&w_commit_rev, "%s", revision); > + strbuf_addf(&b_commit_rev, "%s^1", revision); > + strbuf_addf(&w_tree_rev, "%s:", revision); > + strbuf_addf(&b_tree_rev, "%s^1:", revision); > + strbuf_addf(&i_tree_rev, "%s^2:", revision); > + strbuf_addf(&u_tree_rev, "%s^3:", revision); > + > + ret = (get_sha1(w_commit_rev.buf, info->w_commit.hash) == 0 && > + get_sha1(b_commit_rev.buf, info->b_commit.hash) == 0 && > + get_sha1(w_tree_rev.buf, info->w_tree.hash) == 0 && > + get_sha1(b_tree_rev.buf, info->b_tree.hash) == 0 && > + get_sha1(i_tree_rev.buf, info->i_tree.hash) == 0); It's more conventional to check for errors with !get_sha1(params), not a long-hand comparision with 0. > + strbuf_release(&w_commit_rev); > + strbuf_release(&b_commit_rev); > + strbuf_release(&w_tree_rev); > + strbuf_release(&b_tree_rev); > + strbuf_release(&i_tree_rev); > + > + if (!ret) > + return error(_("%s is not a valid reference"), revision); We are leaking u_tree_rev.buf upon early return. > + info->has_u = get_sha1(u_tree_rev.buf, info->u_tree.hash) == 0; > + > + strbuf_release(&u_tree_rev); > + > + end_of_rev = strchrnul(revision, '@'); > + strbuf_add(&symbolic, revision, end_of_rev - revision); > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "rev-parse", "--symbolic-full-name", NULL); > + argv_array_pushf(&cp.args, "%s", symbolic.buf); > + strbuf_release(&symbolic); > + pipe_command(&cp, NULL, 0, &out, 0, NULL, 0); > + > + if (out.len-1 == strlen(ref_stash)) > + info->is_stash_ref = strncmp(out.buf, ref_stash, out.len-1) == 0; Favor !strncmp(params) over comparision with 0. Style: Have SP around both sides of binary operator "-". > + strbuf_release(&out); > + return !ret; Hmph, where did we last assign ret in this code? Didn't we check that value and returned error already, which means we know what !ret is when the control reaches here? > +} I have to move to another building, so I'll stop here for now, but will continue later. Thanks. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-08 0:55 ` [PATCH v4 5/5] stash: implement builtin stash Joel Teichroeb 2017-06-11 21:27 ` Thomas Gummerer 2017-06-16 16:15 ` Junio C Hamano @ 2017-06-16 22:47 ` Junio C Hamano 2017-06-19 13:16 ` Johannes Schindelin 2017-06-20 2:12 ` Joel Teichroeb 2017-06-22 17:07 ` Junio C Hamano 3 siblings, 2 replies; 26+ messages in thread From: Junio C Hamano @ 2017-06-16 22:47 UTC (permalink / raw) To: Joel Teichroeb Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Joel Teichroeb <joel@teichroeb.net> writes: > +static void stash_create_callback(struct diff_queue_struct *q, > + struct diff_options *opt, void *cbdata) > +{ > + int i; > + > + for (i = 0; i < q->nr; i++) { > + struct diff_filepair *p = q->queue[i]; > + const char *path = p->one->path; > + struct stat st; The order is somewhat ugly. Move "struct stat st;" that does not have any initialization at the beginning. > + remove_file_from_index(&the_index, path); > + if (!lstat(path, &st)) > + add_to_index(&the_index, path, &st, 0); > + } > +} So this will be called with list of paths that are different from the working tree and the index, and adds all the paths the index knows about to the index from the working tree? Sounds OK, but I am not sure if that is "stash_create_callback()". Surely it is _part_ of creating a stash, but it would be better to name it to reflect which part of creating a stash this helper is about. I think this is about recording the working tree state, so I would have expected "record" and/or "working_tree" in its name. > +/* > + * Untracked files are stored by themselves in a parentless commit, for > + * ease of unpacking later. > + */ > +static int save_untracked(struct stash_info *info, const char *message, > + int include_untracked, int include_ignored, const char **argv) > +{ > + struct child_process cp = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + struct object_id orig_tree; > + int ret; > + const char *index_file = get_index_file(); > + > + set_alternate_index_output(stash_index_path); > + untracked_files(&out, include_untracked, include_ignored, argv); > + > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "update-index", "-z", "--add", "--remove", > + "--stdin", NULL); > + argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path); > + > + if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) { > + strbuf_release(&out); > + return 1; > + } > + OK, that's a very straight-forward way of doing this, and as we do not care too much about performance in this initial conversion to C, it is even sensible. In a later update after this patch lands, you may want to use dir.c's fill_directory() API to find the untracked files and add them yourself internally, without running ls-files (in untracked_files()) or update-index (here) as subprocesses, but that is in the future. Let's get this round finished. > + strbuf_reset(&out); > + > + discard_cache(); > + read_cache_from(stash_index_path); > + > + write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL); SP before "NULL". > + discard_cache(); > + > + read_cache_from(stash_index_path); Hmph, what did anybody change in the on-disk stash_index (or contents in the_index) since you read_cache_from()? > + write_cache_as_tree(info->u_tree.hash, 0, NULL); Then you write exactly the same index contents again, this time to info->u_tree here. I am not sure why you need to do this twice, and I do not see how orig_tree.hash you wrote earlier is used? > + strbuf_addf(&out, "untracked files on %s", message); > + > + ret = commit_tree(out.buf, out.len, info->u_tree.hash, NULL, > + info->u_commit.hash, NULL, NULL); > + strbuf_release(&out); > + if (ret) > + return 1; > + > + set_alternate_index_output(index_file); > + discard_cache(); > + read_cache(); > + > + return 0; > +} OK, except for minor nits, this seems to correctly replicate what u_commit=$(...) does in create_stash shell function in the original. > +static int save_working_tree(struct stash_info *info, const char *prefix, > + const char **argv) > +{ > + struct object_id orig_tree; > + struct rev_info rev; > + int nr_trees = 1; > + struct tree_desc t[MAX_UNPACK_TREES]; > + struct tree *tree; > + struct unpack_trees_options opts; > + struct object *obj; > + > + discard_cache(); > + tree = parse_tree_indirect(&info->i_tree); > + prime_cache_tree(&the_index, tree); > + write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0, NULL); > + discard_cache(); Hmph, the caller of this function did read_cache(), refresh_index(), and write_cache_as_tree(), and the result is in info->i_tree. The above sequence discards, reads that tree into the index and writes the same tree again. Which seems like a huge no-op. IIUC, the write_cache_as_tree() the caller already did should have already primed the cache-tree structure, too. These five lines are puzzling. > + read_cache_from(stash_index_path); Hmph, it is unclear who wrote what state to this $TMPindex from this codeflow. Do you really want to read from there? I am guessing that this part corresponds to w_tree=$( ... ) in create_stash shell function, which does "read-tree --index-output=$TMPindex -m $i_tree" starting from the real $GIT_DIR/index and the call to unpack_tree() that follows here is that "read-tree". A one-way "read-tree -m" is purely a performance measure and the resulting index will have the entries in $i_tree no matter what index contents you start from, so you may not have seen an incorrect result per-se, but I suspect that you do not want to be reading from $TMPindex here. Puzzled... > + > + memset(&opts, 0, sizeof(opts)); > + > + parse_tree(tree); > + > + opts.head_idx = 1; > + opts.src_index = &the_index; > + opts.dst_index = &the_index; > + opts.merge = 1; > + opts.fn = oneway_merge; > + > + init_tree_desc(t, tree->buffer, tree->size); > + > + if (unpack_trees(nr_trees, t, &opts)) > + return 1; > + > + init_revisions(&rev, prefix); > + setup_revisions(0, NULL, &rev, NULL); > + rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK; > + rev.diffopt.format_callback = stash_create_callback; > + DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS); > + > + parse_pathspec(&rev.prune_data, 0, 0, prefix, argv); > + > + diff_setup_done(&rev.diffopt); > + obj = parse_object(&info->b_commit); > + add_pending_object(&rev, obj, ""); > + if (run_diff_index(&rev, 0)) > + return 1; > + > + if (write_cache_as_tree(info->w_tree.hash, 0, NULL)) > + return 1; > + > + discard_cache(); > + read_cache(); > + > + return 0; > +} This part otherwise looks like a correct way to grab changes to the working tree into w_tree. Again, I need to stop here for now. Will continue later. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-16 22:47 ` Junio C Hamano @ 2017-06-19 13:16 ` Johannes Schindelin 2017-06-19 13:20 ` Jeff King 2017-06-20 2:12 ` Joel Teichroeb 1 sibling, 1 reply; 26+ messages in thread From: Johannes Schindelin @ 2017-06-19 13:16 UTC (permalink / raw) To: Junio C Hamano Cc: Joel Teichroeb, Git Mailing List, Ævar Arnfjörð Bjarmason, Jeff King, Christian Couder Hi Junio, On Fri, 16 Jun 2017, Junio C Hamano wrote: > Joel Teichroeb <joel@teichroeb.net> writes: > > > +static void stash_create_callback(struct diff_queue_struct *q, > > + struct diff_options *opt, void *cbdata) > > +{ > > + int i; > > + > > + for (i = 0; i < q->nr; i++) { > > + struct diff_filepair *p = q->queue[i]; > > + const char *path = p->one->path; > > + struct stat st; > > The order is somewhat ugly. Move "struct stat st;" that does not > have any initialization at the beginning. Let's not call it "ugly". You may find it ugly, but maybe you may want to avoid contributors feeling judged negatively, either. Instead, let's say that it is preferred in Git's source code to declare uninitialized variables first, and then declare variables which are initialized at the same time. This convention, however, would need to be documented in CodingGuidelines first. We do not want to make contributors feel dumb now, do we? In this particular case, I also wonder whether it is worth the time to point out an unwritten (and not always obeyed) rule. The variable block is small enough that it does not matter much in which order the variables are declared. However, trying to be very strict even in such a small matter may well cost us contributors (and it is dubious whether the most critical parts of our technical debt has anything to do with small code style issues similar to this one). It's not like our bar of entry to new contributors is very low, exactly... And if you disagree with this assessment, you should point out the same issues in literally all of my patches, as I always put initialized variables first, uninitialized last. > > + strbuf_reset(&out); > > + > > + discard_cache(); > > + read_cache_from(stash_index_path); > > + > > + write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL); > > SP before "NULL". If only we had automated source code formatting, saving us from these distractions during patch review. The rest of the review, modulo all the "Hmpf"s, seems helpful enough that I will try to find time to review the next iteration of this patch series (with a fresh mind, as I only skimmed the previous iteration) instead of adding my comments here. Ciao, Dscho ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-19 13:16 ` Johannes Schindelin @ 2017-06-19 13:20 ` Jeff King 0 siblings, 0 replies; 26+ messages in thread From: Jeff King @ 2017-06-19 13:20 UTC (permalink / raw) To: Johannes Schindelin Cc: Junio C Hamano, Joel Teichroeb, Git Mailing List, Ævar Arnfjörð Bjarmason, Christian Couder On Mon, Jun 19, 2017 at 03:16:48PM +0200, Johannes Schindelin wrote: > And if you disagree with this assessment, you should point out the same > issues in literally all of my patches, as I always put initialized > variables first, uninitialized last. Yeah, I am scratching my head here. If we do have a convention for ordering, I'd have thought it is that (and I'd probably put statics even above initialized variables). -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-16 22:47 ` Junio C Hamano 2017-06-19 13:16 ` Johannes Schindelin @ 2017-06-20 2:12 ` Joel Teichroeb 2017-06-22 17:23 ` Junio C Hamano 1 sibling, 1 reply; 26+ messages in thread From: Joel Teichroeb @ 2017-06-20 2:12 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder On Fri, Jun 16, 2017 at 3:47 PM, Junio C Hamano <gitster@pobox.com> wrote: > Joel Teichroeb <joel@teichroeb.net> writes: >> +/* >> + * Untracked files are stored by themselves in a parentless commit, for >> + * ease of unpacking later. >> + */ >> +static int save_untracked(struct stash_info *info, const char *message, >> + int include_untracked, int include_ignored, const char **argv) >> +{ >> + struct child_process cp = CHILD_PROCESS_INIT; >> + struct strbuf out = STRBUF_INIT; >> + struct object_id orig_tree; >> + int ret; >> + const char *index_file = get_index_file(); >> + >> + set_alternate_index_output(stash_index_path); >> + untracked_files(&out, include_untracked, include_ignored, argv); >> + >> + cp.git_cmd = 1; >> + argv_array_pushl(&cp.args, "update-index", "-z", "--add", "--remove", >> + "--stdin", NULL); >> + argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path); >> + >> + if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) { >> + strbuf_release(&out); >> + return 1; >> + } >> + > > OK, that's a very straight-forward way of doing this, and as we do > not care too much about performance in this initial conversion to C, > it is even sensible. In a later update after this patch lands, you > may want to use dir.c's fill_directory() API to find the untracked > files and add them yourself internally, without running ls-files (in > untracked_files()) or update-index (here) as subprocesses, but that > is in the future. Let's get this round finished. > >> + strbuf_reset(&out); >> + >> + discard_cache(); >> + read_cache_from(stash_index_path); >> + >> + write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL); > > SP before "NULL". > >> + discard_cache(); >> + >> + read_cache_from(stash_index_path); > > Hmph, what did anybody change in the on-disk stash_index (or > contents in the_index) since you read_cache_from()? > >> + write_cache_as_tree(info->u_tree.hash, 0, NULL); > > Then you write exactly the same index contents again, this time to > info->u_tree here. I am not sure why you need to do this twice, and > I do not see how orig_tree.hash you wrote earlier is used? > I'm not sure I understand what's happening here either. When I was writing this, it was essentially a lot of trial and error in order to get the index handling correct. Getting rid of any single one of these lines makes the test fail. At some point I'd like to redo all the index handling parts here, as I think I can do without an additional index, but I'd need to make sure the error handling is perfect first. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-20 2:12 ` Joel Teichroeb @ 2017-06-22 17:23 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2017-06-22 17:23 UTC (permalink / raw) To: Joel Teichroeb Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Joel Teichroeb <joel@teichroeb.net> writes: > On Fri, Jun 16, 2017 at 3:47 PM, Junio C Hamano <gitster@pobox.com> wrote: > ... >> Then you write exactly the same index contents again, this time to >> info->u_tree here. I am not sure why you need to do this twice, and >> I do not see how orig_tree.hash you wrote earlier is used? > > I'm not sure I understand what's happening here either. When I was > writing this, it was essentially a lot of trial and error in order to > get the index handling correct.... Thanks for being honest. I agree that we do not want to say "we do not yet know the exact mechanism how X happens, but X does happen" for any value of X (in this case "the code happens to do the same thing as the original"). In biology or physics experiments, that may be how science advances, but it is different when it comes for us to explain our own code ;-). After all, its our creation. I haven't followed the big picture in your codepath, but if you had something like this, I can see how you need a seemingly unneeded reading of the index: function A discard and read index do A's thing function B discard and read index do B's thing function C discard and read index if (some condition) do things that involves smudging the index call A else call B function D read index if (some other condition) call A else do things that involves smudging the index call B That is, when the division of labor for preparing the in-core index is not very well defined between the caller and the callee. When function C calls function B, the index is unnecessarily discarded and read at the beginning of function B, but if you remove it without changing anything else, the call to it from function D would break. One way to fix it would be to make the two helpers work from the given in-core index, iow, make their callers responsible for preparing the in-core index to desired state, i.e. function A do A's thing function B do B's thing function C discard and read index if (some condition) do things that involves smudging the index discard and read index call A else call B function D read index if (some other condition) call A else do things that involves smudging the index discard and read index call B Again, I didn't follow the big picture callpath in your patch, so the above may not be why your extra read-index calls are needed, and I do not know which of your functions correspond to A, B, C and D in the above illustration. But I think you get the idea. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/5] stash: implement builtin stash 2017-06-08 0:55 ` [PATCH v4 5/5] stash: implement builtin stash Joel Teichroeb ` (2 preceding siblings ...) 2017-06-16 22:47 ` Junio C Hamano @ 2017-06-22 17:07 ` Junio C Hamano 3 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2017-06-22 17:07 UTC (permalink / raw) To: Joel Teichroeb Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder Joel Teichroeb <joel@teichroeb.net> writes: > +static int patch_working_tree(struct stash_info *info, const char *prefix, > + const char **argv) > +{ > + struct argv_array args = ARGV_ARRAY_INIT; > + struct child_process cp = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + size_t unused; > + const char *index_file = get_index_file(); > + > + argv_array_pushl(&args, "read-tree", "HEAD", NULL); > + argv_array_pushf(&args, "--index-output=%s", stash_index_path); > + cmd_read_tree(args.argc, args.argv, prefix); I do not think if cmd_read_tree() is prepared to be called like this, and even if it happens to be OK, I do not think we should rely on it. In general, cmd_foo() that implements subcommand "foo" expects only to be called from main(), and expects that the calling main() will exit with its return status. This has implications that you, who abuse a cmd_foo() function as if it is a reusable helper function, need to watch out for. For example, cmd_foo() may use static global variables in builtin/foo.c that are initialized in a certain way before it starts, so calling cmd_foo() twice may not work correctly (the first invocation may change these variables and they won't be reset when it returns). cmd_foo() can and do leave resources unreclaimed, because it expects the calling main() to exit immediately, leaving descriptors it creates open or chunks of memory it allocates unfreed. cmd_foo() also can and do die() without returning the control to the caller (this last item does not make much difference to this particular codepath, as you'd end up dying soon if this read-tree fails anyway, but in general you'd want to give a more specific error message when it happens, i.e. instead of an error message cmd_read_tree() internally gives, you want to say "Cannot save the current worktree state"). > + > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "add--interactive", "--patch=stash", "--", NULL); > + argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path); > + if (run_command(&cp)) > + return 1; Unlike the above direct call to cmd_read_tree(), the way this invokes "git add--interactive" is kosher. By returning non-zero (by the way, the prevailing convention in this codebase is to return negative for an error), you give a chance to the caller of this helper function to say "Cannot save the current worktree state". > + discard_cache(); > + read_cache_from(stash_index_path); > + > + if (write_cache_as_tree(info->w_tree.hash, 0, NULL)) > + return 1; OK. > + child_process_init(&cp); > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "diff-tree", "-p", "HEAD", NULL); > + argv_array_push(&cp.args, sha1_to_hex(info->w_tree.hash)); > + argv_array_push(&cp.args, "--"); > + if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0) || out.len == 0) > + return 1; This "diff-tree" call is also reasonable. Instead of getting the patch text into a temporary file (which is what the original did), we slurp it into a strbuf "out", and pass it out to the caller by storing in info->patch. OK. > + info->patch = strbuf_detach(&out, &unused); You can pass NULL instead of a throw-away variable &unused. > + > + set_alternate_index_output(index_file); > + discard_cache(); > + read_cache(); > + > + return 0; > +} So this looks fairly faithful rewrite to C of a half of the create_stash shell function (we already reviewed the other half done in your save_working_tree() function). > +static int do_create_stash(struct stash_info *info, const char *prefix, > + const char *message, int include_untracked, int include_ignored, > + int patch, const char **argv) > +{ > + struct object_id curr_head; > + char *branch_path = NULL; > + const char *branch_name = NULL; > + struct commit_list *parents = NULL; > + struct strbuf out_message = STRBUF_INIT; > + struct strbuf out = STRBUF_INIT; > + struct pretty_print_context ctx = {0}; > + > + struct commit *c = NULL; > + const char *hash; > + > + read_cache_preload(NULL); > + refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); > + if (check_no_changes(prefix, include_untracked, include_ignored, argv)) > + return 1; We find there is nothing to stash, so we tell the caller that fact by returning 1. The caller can tell that this is a "different kind of success" and is not an error by checking the sign of the return value. > + if (get_sha1_tree("HEAD", info->b_commit.hash)) > + return error(_("You do not have the initial commit yet")); And this is an error from the caller's point of view (error() returns -1). > + branch_path = resolve_refdup("HEAD", 0, curr_head.hash, NULL); > + > + if (branch_path == NULL || strcmp(branch_path, "HEAD") == 0) > + branch_name = "(no branch)"; > + else > + skip_prefix(branch_path, "refs/heads/", &branch_name); > + > + c = lookup_commit(&info->b_commit); > + > + ctx.output_encoding = get_log_output_encoding(); > + ctx.abbrev = 1; > + ctx.fmt = CMIT_FMT_ONELINE; > + hash = find_unique_abbrev(c->object.oid.hash, DEFAULT_ABBREV); > + > + strbuf_addf(&out_message, "%s: %s ", branch_name, hash); > + > + pretty_print_commit(&ctx, c, &out_message); > + > + strbuf_addf(&out, "index on %s\n", out_message.buf); OK, the above roughly correspond to "# state of the base commit" part of the original. This message is created with msg=$(printf '%s: %s' "$branch" "$head") and your "branch_name" is computed to be the same as $branch, and $head in the original is head=$(git rev-list --oneline -n 1 HEAD--) which is your "hash" with the oneline. The whole $msg corresponds to your "out_message.buf". Looks correct. > + commit_list_insert(lookup_commit(&info->b_commit), &parents); > + > + if (write_cache_as_tree(info->i_tree.hash, 0, NULL)) > + return error(_("git write-tree failed to write a tree")); Shouldn't the user also see "Cannot save the current index state" message in this case? A failure by write-tree is an implementation detail and the latter is what matters more to the end user. > + if (commit_tree(out.buf, out.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL)) > + return error(_("Cannot save the current index state")); > + > + strbuf_reset(&out); > + > + if (include_untracked) { > + if (save_untracked(info, out_message.buf, include_untracked, include_ignored, argv)) > + return error(_("Cannot save the untracked files")); > + } > + > + if (patch) { > + if (patch_working_tree(info, prefix, argv)) > + return error(_("Cannot save the current worktree state")); > + } else { > + if (save_working_tree(info, prefix, argv)) > + return error(_("Cannot save the current worktree state")); > + } > + parents = NULL; The elements on the parents list here are leaked here, by early returns we see above and also at the end of this function. > + if (include_untracked) > + commit_list_insert(lookup_commit(&info->u_commit), &parents); > + > + commit_list_insert(lookup_commit(&info->i_commit), &parents); > + commit_list_insert(lookup_commit(&info->b_commit), &parents); > + > + if (message != NULL && strlen(message) != 0) > + strbuf_addf(&out, "On %s: %s\n", branch_name, message); > + else > + strbuf_addf(&out, "WIP on %s\n", out_message.buf); > + > + if (commit_tree(out.buf, out.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL)) > + return error(_("Cannot record working tree state")); > + > + info->message = out.buf; > + > + strbuf_release(&out_message); > + free(branch_path); > + > + return 0; > +} > +static int create_stash(int argc, const char **argv, const char *prefix) > +{ > + int include_untracked = 0; > + const char *message = NULL; > + struct stash_info info; > + int ret; > + struct strbuf out = STRBUF_INIT; > + struct option options[] = { > + OPT_BOOL('u', "include-untracked", &include_untracked, > + N_("stash untracked filed")), > + OPT_STRING('m', "message", &message, N_("message"), > + N_("stash commit message")), > + OPT_END() > + }; > + > + argc = parse_options(argc, argv, prefix, options, > + git_stash_create_usage, 0); > + > + if (argc != 0) { > + int i; > + for (i = 0; i < argc; ++i) { > + if (i != 0) { > + strbuf_addf(&out, " "); > + } Style tips: Do not enclose a single statement inside a block. We prefer "if (i)" over "if (i != 0)". We prefer "if (!i)" over "if (i == 0)". We prefer "i++" over "++i", UNLESS you use the value before increment. > + strbuf_addf(&out, "%s", argv[i]); > + } > + message = out.buf; > + } > + > + ret = do_create_stash(&info, prefix, message, include_untracked, 0, 0, NULL); > + > + strbuf_release(&out); I do not see a need for "message" variable; you can just pass out.buf to do_create_stash(). > + > + if (ret) > + return 0; > + > + printf("%s\n", sha1_to_hex(info.w_commit.hash)); > + return 0; > +} ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/5] Implement git stash as a builtin command 2017-06-08 0:55 [PATCH v4 0/5] Implement git stash as a builtin command Joel Teichroeb ` (4 preceding siblings ...) 2017-06-08 0:55 ` [PATCH v4 5/5] stash: implement builtin stash Joel Teichroeb @ 2017-06-11 17:40 ` Joel Teichroeb 5 siblings, 0 replies; 26+ messages in thread From: Joel Teichroeb @ 2017-06-11 17:40 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Jeff King, Christian Couder I haven't seen any response. Would it be possible for anyone to review? Thanks, Joel On 6/7/2017 5:55 PM, Joel Teichroeb wrote: > I've rewritten git stash as a builtin c command. All tests pass, > and I've added two new tests. Test coverage is around 95% with the > only things missing coverage being error handlers. > > Changes since v3: > * Fixed formatting issues > * Fixed a bug with stash branch and added a new test for it > * Fixed review comments > > Outstanding issue: > * Not all argv array memory is cleaned up > > Joel Teichroeb (5): > stash: add test for stash create with no files > stash: Add a test for when apply fails during stash branch > stash: add test for stashing in a detached state > merge: close the index lock when not writing the new index > stash: implement builtin stash > > Makefile | 2 +- > builtin.h | 1 + > builtin/stash.c | 1224 +++++++++++++++++++++++++ > git-stash.sh => contrib/examples/git-stash.sh | 0 > git.c | 1 + > merge-recursive.c | 9 +- > t/t3903-stash.sh | 34 + > 7 files changed, 1267 insertions(+), 4 deletions(-) > create mode 100644 builtin/stash.c > rename git-stash.sh => contrib/examples/git-stash.sh (100%) > ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-06-27 14:53 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-08 0:55 [PATCH v4 0/5] Implement git stash as a builtin command Joel Teichroeb 2017-06-08 0:55 ` [PATCH v4 1/5] stash: add test for stash create with no files Joel Teichroeb 2017-06-13 19:31 ` Junio C Hamano 2017-06-08 0:55 ` [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch Joel Teichroeb 2017-06-13 19:40 ` Junio C Hamano 2017-06-13 19:54 ` Joel Teichroeb 2017-06-08 0:55 ` [PATCH v4 3/5] stash: add test for stashing in a detached state Joel Teichroeb 2017-06-13 19:45 ` Junio C Hamano 2017-06-13 19:48 ` Joel Teichroeb 2017-06-13 20:58 ` Junio C Hamano 2017-06-08 0:55 ` [PATCH v4 4/5] merge: close the index lock when not writing the new index Joel Teichroeb 2017-06-13 19:47 ` Junio C Hamano 2017-06-08 0:55 ` [PATCH v4 5/5] stash: implement builtin stash Joel Teichroeb 2017-06-11 21:27 ` Thomas Gummerer 2017-06-20 2:37 ` Joel Teichroeb 2017-06-25 21:09 ` Thomas Gummerer 2017-06-26 7:53 ` Matthieu Moy 2017-06-27 14:53 ` Thomas Gummerer 2017-06-16 16:15 ` Junio C Hamano 2017-06-16 22:47 ` Junio C Hamano 2017-06-19 13:16 ` Johannes Schindelin 2017-06-19 13:20 ` Jeff King 2017-06-20 2:12 ` Joel Teichroeb 2017-06-22 17:23 ` Junio C Hamano 2017-06-22 17:07 ` Junio C Hamano 2017-06-11 17:40 ` [PATCH v4 0/5] Implement git stash as a builtin command Joel Teichroeb
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).