From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
This is kind of a re-roll of [1] (wow, apparently it took me almost
two months to get done). The goal was, then and now, to teach "git
reset" to work on an unborn branch and to not require a commit when a
tree would do. This time, I also made some tangential improvements
along the way, mostly related to readability and performance.
As usual, the risker patches are towards the end. In particular, I
find it hard to evaluate how risky the last patch is. That last patch
is responsible for much of the improvements in the timing table below,
so it would be nice if it doesn't break things too badly (test pass,
of course). The timings are best-of-five, wall time.
Command Before After
reset (warm) 0.23 0.07
reset -q (warm) 0.23 0.03
reset . (warm) 0.09 0.07
reset -q . (warm) 0.09 0.03
reset --keep (warm) 0.31 0.29
reset --keep -q (warm) 0.31 0.29
reset (cold) 9.74 2.60
reset -q (cold) 9.85 0.37
reset . (cold) 2.66 2.51
reset -q . (cold) 2.59 0.33
reset --keep (cold) 7.58 7.52
reset --keep -q (cold) 7.37 7.21
[1] http://thread.gmane.org/gmane.comp.version-control.git/210568/focus=210855
Martin von Zweigbergk (19):
reset $pathspec: no need to discard index
reset $pathspec: exit with code 0 if successful
reset.c: pass pathspec around instead of (prefix, argv) pair
reset: don't allow "git reset -- $pathspec" in bare repo
reset.c: extract function for parsing arguments
reset.c: remove unnecessary variable 'i'
reset.c: extract function for updating {ORIG,}HEAD
reset.c: share call to die_if_unmerged_cache()
reset.c: replace switch by if-else
reset --keep: only write index file once
reset: avoid redundant error message
reset.c: move update_index_refresh() call out of read_from_tree()
reset.c: move lock, write and commit out of update_index_refresh()
reset [--mixed]: don't write index file twice
reset.c: finish entire cmd_reset() whether or not pathspec is given
reset [--mixed] --quiet: don't refresh index
reset $sha1 $pathspec: require $sha1 only to be treeish
reset: allow reset on unborn branch
reset [--mixed]: use diff-based reset whether or not pathspec was
given
builtin/reset.c | 281 +++++++++++++++++++----------------------
t/t2013-checkout-submodule.sh | 2 +-
t/t7102-reset.sh | 26 +++-
t/t7106-reset-unborn-branch.sh | 52 ++++++++
4 files changed, 200 insertions(+), 161 deletions(-)
create mode 100755 t/t7106-reset-unborn-branch.sh
--
1.8.1.rc3.331.g1ef2165
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
"git reset $pathspec" currently exits with a non-zero exit code if the
worktree is dirty after resetting, which is inconsistent with reset
without pathspec, and it makes it harder to know whether the command
really failed. Change it to exit with code 0 regardless of whether the
worktree is dirty so that non-zero indicates an error.
This makes the 4 "disambiguation" test cases in t7102 clearer since
they all used to "fail", 3 of which "failed" due to changes in the
work tree. Now only the ambiguous one fails.
---
I suppose this makes documenting the exit code unnecessary, since
"return zero iff successful" is probably understood to be the default.
The variable in refresh_index() containing the value to be returned is
called has_errors. I'm guessing I shouldn't take the name too
seriously.
builtin/reset.c | 8 +++-----
t/t2013-checkout-submodule.sh | 2 +-
t/t7102-reset.sh | 18 ++++++++++++------
3 files changed, 16 insertions(+), 12 deletions(-)
@@ -402,7 +403,8 @@ test_expect_success 'test resetting the index at give paths' '>sub/file2&&gitupdate-index--addsub/file1sub/file2&&T=$(gitwrite-tree)&&-test_must_failgitresetHEADsub/file2&&+gitresetHEADsub/file2&&+test_must_failgitdiff--quiet&&U=$(gitwrite-tree)&&echo"$T"&&echo"$U"&&
@@ -440,7 +442,8 @@ test_expect_success 'resetting specific path that is unmerged' 'echo"100644 $F3 3 file2"}|gitupdate-index--index-info&&gitls-files-u&&-test_must_failgitresetHEADfile2&&+gitresetHEADfile2&&+test_must_failgitdiff--quiet&&gitdiff-index--exit-code--cachedHEAD'
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
When doing a mixed reset without paths, the index is locked, read,
reset, and written back as part of the actual reset operation (in
reset_index()). Then, when showing the list of worktree modifications,
we lock the index again, refresh it, and write it.
Change this so we only write the index once, making "git reset" a
little faster. It does mean that the index lock will be held a little
longer, but the difference is small compared to the time spent
refreshing the index.
There is one minor functional difference: We used to say "Could not
write new index file." if the first write failed, and "Could not
refresh index" if the second write failed. Now, we will only use the
first message.
This speeds up "git reset" a little on the linux-2.6 repo (best of
five, warm cache):
Before After
real 0m0.239s 0m0.214s
user 0m0.160s 0m0.130s
sys 0m0.070s 0m0.080s
---
builtin/reset.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
@@ -335,6 +335,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)err=reset_index(sha1,MIXED,quiet);if(err)die(_("Could not reset index file to revision '%s'."),rev);++if(reset_type==MIXED)/* Report what has not been updated. */+update_index_refresh(+quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(write_cache(newfd,active_cache,active_nr)||commit_locked_index(lock))die(_("Could not write new index file."));
@@ -346,15 +351,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)if(reset_type==HARD&&!update_ref_status&&!quiet)print_new_head_line(commit);-elseif(reset_type==MIXED){/* Report what has not been updated. */-structlock_file*index_lock=xcalloc(1,sizeof(structlock_file));-intfd=hold_locked_index(index_lock,1);-update_index_refresh(-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);-if(write_cache(fd,active_cache,active_nr)||-commit_locked_index(index_lock))-error("Could not refresh index");-}remove_branch_state();
@@ -295,8 +295,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)elseif(reset_type!=NONE)die(_("Cannot do %s reset with paths."),_(reset_type_names[reset_type]));-returnread_from_tree(pathspec,sha1,-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);}if(reset_type==NONE)reset_type=MIXED;/* by default */
@@ -308,6 +306,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("%s reset is not allowed in a bare repository"),_(reset_type_names[reset_type]));+if(pathspec)+returnread_from_tree(pathspec,sha1,+quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+/* Soft reset does not touch the index file nor the working tree*atall,butrequirestheminagoodorder.Otherresetsreset*theindexfiletothetreeobjectweareswitchingto.*/
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
By not returning from inside the "if (pathspec)" block, we can let the
pathspec-aware and pathspec-less code share a bit more, making it
easier to make future changes that should affect both cases. This also
highlights the similarity between read_from_tree() and reset_index().
---
Should error reporting be aligned too? Speaking of which,
do_diff_cache() never returns anything by 0. Is the return value for
future-proofing?
builtin/reset.c | 42 ++++++++++++++++++------------------------
1 file changed, 18 insertions(+), 24 deletions(-)
@@ -308,19 +308,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("%s reset is not allowed in a bare repository"),_(reset_type_names[reset_type]));-if(pathspec){-structlock_file*lock=xcalloc(1,sizeof(structlock_file));-intindex_fd=hold_locked_index(lock,1);-if(read_from_tree(pathspec,sha1))-return1;-update_index_refresh(-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);-if(write_cache(index_fd,active_cache,active_nr)||-commit_locked_index(lock))-returnerror("Could not write new index file.");-return0;-}-/* Soft reset does not touch the index file nor the working tree*atall,butrequirestheminagoodorder.Otherresetsreset*theindexfiletothetreeobjectweareswitchingto.*/
@@ -330,11 +317,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)if(reset_type!=SOFT){structlock_file*lock=xcalloc(1,sizeof(structlock_file));intnewfd=hold_locked_index(lock,1);-interr=reset_index(sha1,reset_type,quiet);-if(reset_type==KEEP&&!err)-err=reset_index(sha1,MIXED,quiet);-if(err)-die(_("Could not reset index file to revision '%s'."),rev);+if(pathspec){+if(read_from_tree(pathspec,sha1))+return1;+}else{+interr=reset_index(sha1,reset_type,quiet);+if(reset_type==KEEP&&!err)+err=reset_index(sha1,MIXED,quiet);+if(err)+die(_("Could not reset index file to revision '%s'."),rev);+}if(reset_type==MIXED)/* Report what has not been updated. */update_index_refresh(
@@ -345,14 +337,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("Could not write new index file."));}-/* Any resets update HEAD to the head being switched to,-*savingthepreviousheadinORIG_HEADbefore.*/-update_ref_status=update_refs(rev,sha1);+if(!pathspec){+/* Any resets without paths update HEAD to the head being+*switchedto,savingthepreviousheadinORIG_HEADbefore.*/+update_ref_status=update_refs(rev,sha1);-if(reset_type==HARD&&!update_ref_status&&!quiet)-print_new_head_line(commit);+if(reset_type==HARD&&!update_ref_status&&!quiet)+print_new_head_line(commit);-remove_branch_state();+remove_branch_state();+}returnupdate_ref_status;}
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
Thanks to b65982b (Optimize "diff-index --cached" using cache-tree,
2009-05-20), resetting with paths is much faster than resetting
without paths. Some timings for the linux-2.6 repo to illustrate this
(best of five, warm cache):
reset reset .
real 0m0.219s 0m0.080s
user 0m0.140s 0m0.040s
sys 0m0.070s 0m0.030s
These two commands should do the same thing, so instead of having the
user type the trailing " ." to get the faster do_diff_cache()-based
implementation, always use it when doing a mixed reset, with or
without paths (so "git reset $rev" would also be faster).
Comparing before and after (best of five):
Before After
reset (warm cache): 0.21 0.07
reset -q (warm cache) 0.17 0.03
reset (cold cache): 10.31 2.72
reset -q (cold cache) 7.64 0.38
---
Are unmerged entries handled the same? read_from_tree() calls
read_cache(), while reset_index() calls read_cache_unmerged(). I
haven't figured out if/why they should be different.
Are there other differences, or could unpack_trees() learn the same
optimization as do_diff_cache()? Actually, the commit mentioned above
does say
Tweak unpack_trees() logic that is used to read in the tree object
to catch the case where the tree entry we are looking at matches the
index as a whole by looking at the cache-tree.
If there are differences, we are clearly missing tests for them. And
it seems like any difference between them should be fixed, so "git
reset" and "git reset ." (from root of tree) do the same thing even
before this patch.
builtin/reset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
Declutter cmd_reset() a bit by moving out the argument parsing to its
own function.
---
builtin/reset.c | 71 ++++++++++++++++++++++++++++++---------------------------
1 file changed, 38 insertions(+), 33 deletions(-)
@@ -198,36 +198,10 @@ static void die_if_unmerged_cache(int reset_type)}-intcmd_reset(intargc,constchar**argv,constchar*prefix)-{-inti=0,reset_type=NONE,update_ref_status=0,quiet=0;-intpatch_mode=0;+constchar**parse_args(intargc,constchar**argv,constchar*prefix,constchar**rev_ret){+inti=0;constchar*rev="HEAD";-unsignedcharsha1[20],*orig=NULL,sha1_orig[20],-*old_orig=NULL,sha1_old_orig[20];-constchar**pathspec=NULL;-structcommit*commit;-structstrbufmsg=STRBUF_INIT;-conststructoptionoptions[]={-OPT__QUIET(&quiet,N_("be quiet, only report errors")),-OPT_SET_INT(0,"mixed",&reset_type,-N_("reset HEAD and index"),MIXED),-OPT_SET_INT(0,"soft",&reset_type,N_("reset only HEAD"),SOFT),-OPT_SET_INT(0,"hard",&reset_type,-N_("reset HEAD, index and working tree"),HARD),-OPT_SET_INT(0,"merge",&reset_type,-N_("reset HEAD, index and working tree"),MERGE),-OPT_SET_INT(0,"keep",&reset_type,-N_("reset HEAD but keep local changes"),KEEP),-OPT_BOOLEAN('p',"patch",&patch_mode,N_("select hunks interactively")),-OPT_END()-};--git_config(git_default_config,NULL);--argc=parse_options(argc,argv,prefix,options,git_reset_usage,-PARSE_OPT_KEEP_DASHDASH);-+unsignedcharunused[20];/**Possibleargumentsare:*
@@ -262,6 +236,40 @@ int cmd_reset(int argc, const char **argv, const char *prefix)verify_filename(prefix,argv[i],1);}}+*rev_ret=rev;+returni<argc?get_pathspec(prefix,argv+i):NULL;+}++intcmd_reset(intargc,constchar**argv,constchar*prefix)+{+intreset_type=NONE,update_ref_status=0,quiet=0;+intpatch_mode=0;+constchar*rev;+unsignedcharsha1[20],*orig=NULL,sha1_orig[20],+*old_orig=NULL,sha1_old_orig[20];+constchar**pathspec=NULL;+structcommit*commit;+structstrbufmsg=STRBUF_INIT;+conststructoptionoptions[]={+OPT__QUIET(&quiet,N_("be quiet, only report errors")),+OPT_SET_INT(0,"mixed",&reset_type,+N_("reset HEAD and index"),MIXED),+OPT_SET_INT(0,"soft",&reset_type,N_("reset only HEAD"),SOFT),+OPT_SET_INT(0,"hard",&reset_type,+N_("reset HEAD, index and working tree"),HARD),+OPT_SET_INT(0,"merge",&reset_type,+N_("reset HEAD, index and working tree"),MERGE),+OPT_SET_INT(0,"keep",&reset_type,+N_("reset HEAD but keep local changes"),KEEP),+OPT_BOOLEAN('p',"patch",&patch_mode,N_("select hunks interactively")),+OPT_END()+};++git_config(git_default_config,NULL);++argc=parse_options(argc,argv,prefix,options,git_reset_usage,+PARSE_OPT_KEEP_DASHDASH);+pathspec=parse_args(argc,argv,prefix,&rev);if(get_sha1_committish(rev,sha1))die(_("Failed to resolve '%s' as a valid ref."),rev);
@@ -277,9 +285,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("Could not parse object '%s'."),rev);hashcpy(sha1,commit->object.sha1);-if(i<argc)-pathspec=get_pathspec(prefix,argv+i);-if(patch_mode){if(reset_type!=NONE)die(_("--patch is incompatible with --{hard,mixed,soft}"));
@@ -351,18 +351,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)*savingthepreviousheadinORIG_HEADbefore.*/update_ref_status=update_refs(rev,sha1);-switch(reset_type){-caseHARD:-if(!update_ref_status&&!quiet)-print_new_head_line(commit);-break;-caseSOFT:/* Nothing else to do. */-break;-caseMIXED:/* Report what has not been updated. */+if(reset_type==HARD&&!update_ref_status&&!quiet)+print_new_head_line(commit);+elseif(reset_type==MIXED)/* Report what has not been updated. */update_index_refresh(0,NULL,quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);-break;-}remove_branch_state();
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
"git reset --keep" calls reset_index_file() twice, first doing a
two-way merge to the target revision, updating the index and worktree,
and then resetting the index. After each call, we write the index
file.
In the unlikely event that the second call to reset_index_file()
fails, the index will have been merged to the target revision, but
HEAD will not be updated, leaving the user with a dirty index.
By moving the locking, writing and committing out of
reset_index_file() and into the caller, we can avoid writing the index
twice, thereby making the sure we don't end up in the half-way reset
state. As a bonus, we speed up "git reset --keep" a little on the
linux-2.6 repo (best of five, warm cache):
Before After
real 0m0.315s 0m0.296s
user 0m0.290s 0m0.280s
sys 0m0.020s 0m0.010s
---
builtin/reset.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
@@ -38,14 +38,12 @@ static inline int is_merge(void)return!access(git_path("MERGE_HEAD"),F_OK);}-staticintreset_index_file(constunsignedchar*sha1,intreset_type,intquiet)+staticintreset_index(constunsignedchar*sha1,intreset_type,intquiet){intnr=1;-intnewfd;structtree_descdesc[2];structtree*tree;structunpack_trees_optionsopts;-structlock_file*lock=xcalloc(1,sizeof(structlock_file));memset(&opts,0,sizeof(opts));opts.head_idx=1;
@@ -67,8 +65,6 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quietopts.reset=1;}-newfd=hold_locked_index(lock,1);-read_cache_unmerged();if(reset_type==KEEP){
@@ -91,10 +87,6 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quietprime_cache_tree(&active_cache_tree,tree);}-if(write_cache(newfd,active_cache,active_nr)||-commit_locked_index(lock))-returnerror(_("Could not write new index file."));-return0;}
@@ -340,9 +332,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die_if_unmerged_cache(reset_type);if(reset_type!=SOFT){-interr=reset_index_file(sha1,reset_type,quiet);+structlock_file*lock=xcalloc(1,sizeof(structlock_file));+intnewfd=hold_locked_index(lock,1);+interr=reset_index(sha1,reset_type,quiet);if(reset_type==KEEP&&!err)-err=reset_index_file(sha1,MIXED,quiet);+err=reset_index(sha1,MIXED,quiet);+if(!err&&+(write_cache(newfd,active_cache,active_nr)||+commit_locked_index(lock))){+err=error(_("Could not write new index file."));+}if(err)die(_("Could not reset index file to revision '%s'."),rev);}
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
Since 34110cd (Make 'unpack_trees()' have a separate source and
destination index, 2008-03-06), the index no longer gets clobbered by
do_diff_cache() and we can remove the code for discarding and
re-reading it.
There are two paths to update_index_refresh() from cmd_reset(), but on
both paths, either read_cache() or read_cache_unmerged() will have
been called, so the call to read_cache() in this method is redundant
(although practically free).
This speeds up "git reset -- ." a little on the linux-2.6 repo (best
of five, warm cache):
Before After
real 0m0.093s 0m0.080s
user 0m0.040s 0m0.020s
sys 0m0.050s 0m0.050s
---
builtin/reset.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
@@ -126,9 +126,6 @@ static int update_index_refresh(int fd, struct lock_file *index_lock, int flags)fd=hold_locked_index(index_lock,1);}-if(read_cache()<0)-returnerror(_("Could not read index"));-result=refresh_index(&the_index,(flags),NULL,NULL,_("Unstaged changes after reset:"))?1:0;if(write_cache(fd,active_cache,active_nr)||
@@ -141,12 +138,6 @@ static void update_index_from_diff(struct diff_queue_struct *q,structdiff_options*opt,void*data){inti;-int*discard_flag=data;--/* do_diff_cache() mangled the index */-discard_cache();-*discard_flag=1;-read_cache();for(i=0;i<q->nr;i++){structdiff_filespec*one=q->queue[i]->one;
@@ -197,9 +186,6 @@ static int read_from_tree(const char *prefix, const char **argv,diff_flush(&opt);diff_tree_release_paths(&opt);-if(!index_was_discarded)-/* The index is still clobbered from do_diff_cache() */-discard_cache();returnupdate_index_refresh(index_fd,lock,refresh_flags);}
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
In preparation for the/a following patch, move the locking, writing
and committing of the index file out of update_index_refresh(). The
code duplication caused will soon be taken care of. What remains of
update_index_refresh() is just one line, but it is still called from
two places, so let's leave it for now.
In the process, we expose and fix the minor UI bug that makes us print
"Could not refresh index" when we fail to write the index file when
invoked with a pathspec. Copy the error message from the pathspec-less
codepath ("Could not write new index file.").
---
builtin/reset.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
@@ -109,19 +109,10 @@ static void print_new_head_line(struct commit *commit)printf("\n");}-staticintupdate_index_refresh(intfd,structlock_file*index_lock,intflags)+staticvoidupdate_index_refresh(intflags){-if(!index_lock){-index_lock=xcalloc(1,sizeof(structlock_file));-fd=hold_locked_index(index_lock,1);-}-refresh_index(&the_index,(flags),NULL,NULL,_("Unstaged changes after reset:"));-if(write_cache(fd,active_cache,active_nr)||-commit_locked_index(index_lock))-returnerror("Could not refresh index");-return0;}staticvoidupdate_index_from_diff(structdiff_queue_struct*q,
@@ -320,9 +311,14 @@ int cmd_reset(int argc, const char **argv, const char *prefix)if(pathspec){structlock_file*lock=xcalloc(1,sizeof(structlock_file));intindex_fd=hold_locked_index(lock,1);-returnread_from_tree(pathspec,sha1)||-update_index_refresh(index_fd,lock,-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(read_from_tree(pathspec,sha1))+return1;+update_index_refresh(+quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(write_cache(index_fd,active_cache,active_nr)||+commit_locked_index(lock))+returnerror("Could not write new index file.");+return0;}/* Soft reset does not touch the index file nor the working tree
@@ -350,9 +346,15 @@ int cmd_reset(int argc, const char **argv, const char *prefix)if(reset_type==HARD&&!update_ref_status&&!quiet)print_new_head_line(commit);-elseif(reset_type==MIXED)/* Report what has not been updated. */-update_index_refresh(0,NULL,-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+elseif(reset_type==MIXED){/* Report what has not been updated. */+structlock_file*index_lock=xcalloc(1,sizeof(structlock_file));+intfd=hold_locked_index(index_lock,1);+update_index_refresh(+quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(write_cache(fd,active_cache,active_nr)||+commit_locked_index(index_lock))+error("Could not refresh index");+}remove_branch_state();
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
Resetting with paths does not update HEAD and there is nothing else
that a commit should be needed for. Relax the argument parsing so only
a tree is required.
The sha1 is only passed to read_from_tree(), which already only
requires a tree.
The "rev" variable we pass to run_add_interactive() will resolve to a
tree. This is fine since interactive_reset only needs the parameter to
be a treeish and doesn't use it for display purposes.
---
Is it correct that interactive_reset does not use the revision
specifier for display purposes? Or, worse, that it requires it to be a
commit in some cases? I tried it and didn't see any problem.
Can the two blocks of code that look up commit or tree be made to
share more? I'm not very familiar with what functions are available. I
think I tried keeping a separate "struct object *object" to be able to
put the last three lines outside the blocks, but didn't like the
result.
builtin/reset.c | 46 ++++++++++++++++++++++++++--------------------
t/t7102-reset.sh | 8 ++++++++
2 files changed, 34 insertions(+), 20 deletions(-)
@@ -240,7 +243,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)constchar*rev;unsignedcharsha1[20];constchar**pathspec=NULL;-structcommit*commit;+structcommit*commit=NULL;conststructoptionoptions[]={OPT__QUIET(&quiet,N_("be quiet, only report errors")),OPT_SET_INT(0,"mixed",&reset_type,
@@ -262,19 +265,22 @@ int cmd_reset(int argc, const char **argv, const char *prefix)PARSE_OPT_KEEP_DASHDASH);pathspec=parse_args(argc,argv,prefix,&rev);-if(get_sha1_committish(rev,sha1))-die(_("Failed to resolve '%s' as a valid ref."),rev);--/*-*NOTE:As"git reset $treeish -- $path"shouldbeusableon-*anytree-ish,thisisnotstrictlycorrect.Wearenot-*movingtheHEADtoanycommit;wearemerelyresettingthe-*entriesintheindextothatofatreeish.-*/-commit=lookup_commit_reference(sha1);-if(!commit)-die(_("Could not parse object '%s'."),rev);-hashcpy(sha1,commit->object.sha1);+if(!pathspec){+if(get_sha1_committish(rev,sha1))+die(_("Failed to resolve '%s' as a valid revision."),rev);+commit=lookup_commit_reference(sha1);+if(!commit)+die(_("Could not parse object '%s'."),rev);+hashcpy(sha1,commit->object.sha1);+}else{+structtree*tree;+if(get_sha1_treeish(rev,sha1))+die(_("Failed to resolve '%s' as a valid tree."),rev);+tree=parse_tree_indirect(sha1);+if(!tree)+die(_("Could not parse object '%s'."),rev);+hashcpy(sha1,tree->object.sha1);+}if(patch_mode){if(reset_type!=NONE)
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
Use a single condition to guard the call to die_if_unmerged_cache for
both --soft and --keep. This avoids the small distraction of the
precondition check from the logic following it.
Also change an instance of
if (e)
err = err || f();
to the almost as short, but clearer
if (e && !err)
err = f();
(which is equivalent since we only care whether exit code is 0)
---
builtin/reset.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
@@ -336,15 +336,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)/* Soft reset does not touch the index file nor the working tree*atall,butrequirestheminagoodorder.Otherresetsreset*theindexfiletothetreeobjectweareswitchingto.*/-if(reset_type==SOFT)+if(reset_type==SOFT||reset_type==KEEP)die_if_unmerged_cache(reset_type);-else{-interr;-if(reset_type==KEEP)-die_if_unmerged_cache(reset_type);-err=reset_index_file(sha1,reset_type,quiet);-if(reset_type==KEEP)-err=err||reset_index_file(sha1,MIXED,quiet);++if(reset_type!=SOFT){+interr=reset_index_file(sha1,reset_type,quiet);+if(reset_type==KEEP&&!err)+err=reset_index_file(sha1,MIXED,quiet);if(err)die(_("Could not reset index file to revision '%s'."),rev);}
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
"git reset [--mixed]" without --quiet refreshes the index in order to
display the "Unstaged changes after reset". When --quiet is given,
that output is suppressed, removing the need to refresh the index.
Other porcelain commands that care about a refreshed index should
already be refreshing it, so running e.g. "git reset -q && git diff"
is still safe.
This commit together with 686b2de (oneway_merge(): only lstat() when
told to update worktree, 2012-12-20) removes all calls to lstat() the
worktree from the command.
This speeds up "git reset -q" a little on the linux-2.6 repo (best
of five, warm cache):
Before After
real 0m0.215s 0m0.176s
user 0m0.150s 0m0.130s
sys 0m0.060s 0m0.040s
And with cold cache (best of five):
Before After
real 0m11.351s 0m8.420s
user 0m0.230s 0m0.220s
sys 0m0.270s 0m0.060s
---
There is a test case in t7102 called '--mixed refreshes the index',
but it only checks that right output it printed. Is the test case not
testing right or not named right? As you can see, I suspect it's the
name/description that should change.
builtin/reset.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
@@ -328,9 +322,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("Could not reset index file to revision '%s'."),rev);}-if(reset_type==MIXED)/* Report what has not been updated. */-update_index_refresh(-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(reset_type==MIXED&&!quiet)/* Report what has not been updated. */+refresh_index(&the_index,REFRESH_IN_PORCELAIN,NULL,NULL,+_("Unstaged changes after reset:"));if(write_cache(newfd,active_cache,active_nr)||commit_locked_index(lock))
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
By extracting the code for updating the HEAD and ORIG_HEAD symbolic
references to a separate function, we declutter cmd_reset() a bit and
we make it clear that e.g. the four variables {,sha1_}{,old_}orig are
only used by this code.
---
builtin/reset.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)
@@ -332,17 +351,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)/* Any resets update HEAD to the head being switched to,*savingthepreviousheadinORIG_HEADbefore.*/-if(!get_sha1("ORIG_HEAD",sha1_old_orig))-old_orig=sha1_old_orig;-if(!get_sha1("HEAD",sha1_orig)){-orig=sha1_orig;-set_reflog_message(&msg,"updating ORIG_HEAD",NULL);-update_ref(msg.buf,"ORIG_HEAD",orig,old_orig,0,MSG_ON_ERR);-}-elseif(old_orig)-delete_ref("ORIG_HEAD",old_orig,0);-set_reflog_message(&msg,"updating HEAD",rev);-update_ref_status=update_ref(msg.buf,"HEAD",sha1,orig,0,MSG_ON_ERR);+update_ref_status=update_refs(rev,sha1);switch(reset_type){caseHARD:
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
Throughout most of parse_args(), the variable 'i' remains at 0. In the
remaining few cases, we can do pointer arithmentic on argv itself
instead.
---
This is clearly mostly a matter of taste. The remainder of the series
does not depend on it in any way.
builtin/reset.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
@@ -210,34 +209,34 @@ const char **parse_args(int argc, const char **argv, const char *prefix, const c*gitreset[-opts]--<paths>...*gitreset[-opts]<paths>...*-*Atthispoint,argv[i]pointsimmediatelyafter[-opts].+*Atthispoint,argvpointsimmediatelyafter[-opts].*/-if(i<argc){-if(!strcmp(argv[i],"--")){-i++;/* reset to HEAD, possibly with paths */-}elseif(i+1<argc&&!strcmp(argv[i+1],"--")){-rev=argv[i];-i+=2;+if(argc){+if(!strcmp(argv[0],"--")){+argv++;/* reset to HEAD, possibly with paths */+}elseif(argc>1&&!strcmp(argv[1],"--")){+rev=argv[0];+argv+=2;}/*-*Otherwise,argv[i]couldbeeither<rev>or<paths>and+*Otherwise,argv[0]couldbeeither<rev>or<paths>and*hastobeunambiguous.*/-elseif(!get_sha1_committish(argv[i],unused)){+elseif(!get_sha1_committish(argv[0],unused)){/*-*Ok,argv[i]lookslikearev;itshouldnot+*Ok,argv[0]lookslikearev;itshouldnot*beafilename.*/-verify_non_filename(prefix,argv[i]);-rev=argv[i++];+verify_non_filename(prefix,argv[0]);+rev=*argv++;}else{/* Otherwise we treat this as a filename */-verify_filename(prefix,argv[i],1);+verify_filename(prefix,argv[0],1);}}*rev_ret=rev;-returni<argc?get_pathspec(prefix,argv+i):NULL;+return*argv?get_pathspec(prefix,argv):NULL;}intcmd_reset(intargc,constchar**argv,constchar*prefix)
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
We use the path arguments in two places in reset.c: in
interactive_reset() and read_from_tree(). Both of these call
get_pathspec(), so we pass the (prefix, arv) pair to both
functions. Move the call to get_pathspec() out of these methods, for
two reasons: 1) One argument is simpler than two. 2) It lets us use
the (arguably clearer) "if (pathspec)" in place of "if (i < argc)".
---
If I understand correctly, this should be rebased on top of
nd/parse-pathspec. Please let me know.
builtin/reset.c | 27 ++++++++++-----------------
1 file changed, 10 insertions(+), 17 deletions(-)
@@ -287,22 +277,25 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("Could not parse object '%s'."),rev);hashcpy(sha1,commit->object.sha1);+if(i<argc)+pathspec=get_pathspec(prefix,argv+i);+if(patch_mode){if(reset_type!=NONE)die(_("--patch is incompatible with --{hard,mixed,soft}"));-returninteractive_reset(rev,argv+i,prefix);+returnrun_add_interactive(rev,"--patch=reset",pathspec);}/* git reset tree [--] paths... can be used to*loadchosenpathsfromthetreeintotheindexwithout*affectingtheworkingtreenorHEAD.*/-if(i<argc){+if(pathspec){if(reset_type==MIXED)warning(_("--mixed with paths is deprecated; use 'git reset -- <paths>' instead."));elseif(reset_type!=NONE)die(_("Cannot do %s reset with paths."),_(reset_type_names[reset_type]));-returnread_from_tree(prefix,argv+i,sha1,+returnread_from_tree(pathspec,sha1,quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);}if(reset_type==NONE)
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
If writing or committing the new index file fails, we print "Could not
write new index file." followed by "Could not reset index file to
revision $rev.". The first message seems to imply the second, so print
only the first message.
---
builtin/reset.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
@@ -337,13 +337,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)interr=reset_index(sha1,reset_type,quiet);if(reset_type==KEEP&&!err)err=reset_index(sha1,MIXED,quiet);-if(!err&&-(write_cache(newfd,active_cache,active_nr)||-commit_locked_index(lock))){-err=error(_("Could not write new index file."));-}if(err)die(_("Could not reset index file to revision '%s'."),rev);+if(write_cache(newfd,active_cache,active_nr)||+commit_locked_index(lock))+die(_("Could not write new index file."));}/* Any resets update HEAD to the head being switched to,
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
Some users seem to think, knowingly or not, that being on an unborn
branch is like having a commit with an empty tree checked out, but
when run on an unborn branch, "git reset" currently fails with:
fatal: Failed to resolve 'HEAD' as a valid ref.
Instead of making users figure out that they should run
git rm --cached -r .
, let's teach "git reset" without a revision argument, when on an
unborn branch, to behave as if the user asked to reset to an empty
tree. Don't take the analogy with an empty commit too far, though, but
still disallow explictly referring to HEAD in "git reset HEAD".
---
builtin/reset.c | 17 ++++++++------
t/t7106-reset-unborn-branch.sh | 52 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+), 7 deletions(-)
create mode 100755 t/t7106-reset-unborn-branch.sh
@@ -264,8 +264,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)argc=parse_options(argc,argv,prefix,options,git_reset_usage,PARSE_OPT_KEEP_DASHDASH);pathspec=parse_args(argc,argv,prefix,&rev);--if(!pathspec){+unborn=!strcmp(rev,"HEAD")&&get_sha1("HEAD",sha1);+if(unborn){+/* reset on unborn branch: treat as reset to empty tree */+hashcpy(sha1,EMPTY_TREE_SHA1_BIN);+}elseif(!pathspec){if(get_sha1_committish(rev,sha1))die(_("Failed to resolve '%s' as a valid revision."),rev);commit=lookup_commit_reference(sha1);
@@ -285,7 +288,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)if(patch_mode){if(reset_type!=NONE)die(_("--patch is incompatible with --{hard,mixed,soft}"));-returnrun_add_interactive(rev,"--patch=reset",pathspec);+returnrun_add_interactive(sha1_to_hex(sha1),"--patch=reset",pathspec);}/* git reset tree [--] paths... can be used to
@@ -337,16 +340,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("Could not write new index file."));}-if(!pathspec){+if(!pathspec&&!unborn){/* Any resets without paths update HEAD to the head being*switchedto,savingthepreviousheadinORIG_HEADbefore.*/update_ref_status=update_refs(rev,sha1);if(reset_type==HARD&&!update_ref_status&&!quiet)print_new_head_line(commit);--remove_branch_state();}+if(!pathspec)+remove_branch_state();returnupdate_ref_status;}
@@ -0,0 +1,52 @@+#!/bin/sh++test_description='git reset should work on unborn branch'+../test-lib.sh++test_expect_success'setup''+echoa>a&&+echob>b+'++test_expect_success'reset''+gitaddab&&+gitreset&&+test"$(gitls-files)"==""+'++test_expect_success'reset HEAD''+rm.git/index&&+gitaddab&&+test_must_failgitresetHEAD+'++test_expect_success'reset $file''+rm.git/index&&+gitaddab&&+gitreseta&&+test"$(gitls-files)"=="b"+'++test_expect_success'reset -p''+rm.git/index&&+gitadda&&+echoy|gitreset-p&&+test"$(gitls-files)"==""+'++test_expect_success'reset --soft is a no-op''+rm.git/index&&+gitadda&&+gitreset--soft+test"$(gitls-files)"=="a"+'++test_expect_success'reset --hard''+rm.git/index&&+gitadda&&+gitreset--hard&&+test"$(gitls-files)"==""&&+test_path_is_missinga+'++test_done
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
The final part of cmd_reset() essentially looks like:
if (pathspec) {
...
read_from_tree(...);
} else {
...
reset_index(...);
update_index_refresh(...);
...
}
where read_from_tree() internally also calls
update_index_refresh(). Move the call to update_index_refresh() out of
read_from_tree for symmetry with the 'else' block, making
read_from_tree() and reset_index() closer in functionality.
---
builtin/reset.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
@@ -321,9 +317,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("%s reset is not allowed in a bare repository"),_(reset_type_names[reset_type]));-if(pathspec)-returnread_from_tree(pathspec,sha1,-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(pathspec){+structlock_file*lock=xcalloc(1,sizeof(structlock_file));+intindex_fd=hold_locked_index(lock,1);+returnread_from_tree(pathspec,sha1)||+update_index_refresh(index_fd,lock,+quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+}/* Soft reset does not touch the index file nor the working tree*atall,butrequirestheminagoodorder.Otherresetsreset
From: Matt Kraai <hidden> Date: 2016-06-15 22:55:42
On Wed, Jan 09, 2013 at 12:16:00AM -0800, Martin von Zweigbergk wrote:
We use the path arguments in two places in reset.c: in
interactive_reset() and read_from_tree(). Both of these call
get_pathspec(), so we pass the (prefix, arv) pair to both
From: Jeff King <hidden> Date: 2016-06-15 22:55:42
On Wed, Jan 09, 2013 at 12:16:13AM -0800, Martin von Zweigbergk wrote:
"git reset [--mixed]" without --quiet refreshes the index in order to
display the "Unstaged changes after reset". When --quiet is given,
that output is suppressed, removing the need to refresh the index.
Other porcelain commands that care about a refreshed index should
already be refreshing it, so running e.g. "git reset -q && git diff"
is still safe.
Hmm. But "git reset -q && git diff-files" would not be?
We have never been very clear about which commands refresh the index.
Since "reset" is about manipulating the index, I'd expect it to be
refreshed afterwards. On the other hand, since we have never guaranteed
anything, perhaps a careful script should always use "git update-index
--refresh". I would not be too surprised if some of our own scripts are
not that careful, though.
-Peff
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:42
On Wed, Jan 9, 2013 at 9:01 AM, Jeff King [off-list ref] wrote:
On Wed, Jan 09, 2013 at 12:16:13AM -0800, Martin von Zweigbergk wrote:
quoted
"git reset [--mixed]" without --quiet refreshes the index in order to
display the "Unstaged changes after reset". When --quiet is given,
that output is suppressed, removing the need to refresh the index.
Other porcelain commands that care about a refreshed index should
already be refreshing it, so running e.g. "git reset -q && git diff"
is still safe.
Hmm. But "git reset -q && git diff-files" would not be?
Right. Actually, "git reset -q && git diff" was perhaps not a good
example, because its analogous plumbing command would be "git reset -q
&& git diff-files -p", which is also safe. But, as you say, "git reset
-q && git diff-files" (without -p) might list files for which only the
stat information has changed.
We have never been very clear about which commands refresh the index.
Yes, git-reset's documentation doesn't mention it.
Since "reset" is about manipulating the index, I'd expect it to be
refreshed afterwards. On the other hand, since we have never guaranteed
anything, perhaps a careful script should always use "git update-index
--refresh".
Since "git diff-files" is a plumbing command, users of it to a
hopefully a bit more careful than regular users, but you never know.
I would not be too surprised if some of our own scripts are
not that careful, though.
I didn't find any, but I might have missed something.
Regardless, this patch was tangential. The goal of this series can be
achieved independently of this patch, so if it's too risky, we can
drop easily drop it.
Also, even though it does make "git reset -q" faster, I'm not sure how
important that is in practice. Most use cases would probably refresh
the index afterwards anyway. In such cases, the improvement on warm
cache would still be there, but the relative improvement in the cold
cache case would be pretty much gone (since the entire tree would be
stat'ed by the following refresh anyway).
Martin
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
Since 34110cd (Make 'unpack_trees()' have a separate source and
destination index, 2008-03-06), the index no longer gets clobbered by
do_diff_cache() and we can remove the code for discarding and
re-reading it.
There are two paths to update_index_refresh() from cmd_reset(), but on
both paths, either read_cache() or read_cache_unmerged() will have
been called, so the call to read_cache() in this method is redundant
(although practically free).
This speeds up "git reset -- ." a little on the linux-2.6 repo (best
of five, warm cache):
Before After
real 0m0.093s 0m0.080s
user 0m0.040s 0m0.020s
sys 0m0.050s 0m0.050s
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
@@ -126,9 +126,6 @@ static int update_index_refresh(int fd, struct lock_file *index_lock, int flags)fd=hold_locked_index(index_lock,1);}-if(read_cache()<0)-returnerror(_("Could not read index"));-result=refresh_index(&the_index,(flags),NULL,NULL,_("Unstaged changes after reset:"))?1:0;if(write_cache(fd,active_cache,active_nr)||
@@ -141,12 +138,6 @@ static void update_index_from_diff(struct diff_queue_struct *q,structdiff_options*opt,void*data){inti;-int*discard_flag=data;--/* do_diff_cache() mangled the index */-discard_cache();-*discard_flag=1;-read_cache();for(i=0;i<q->nr;i++){structdiff_filespec*one=q->queue[i]->one;
@@ -197,9 +186,6 @@ static int read_from_tree(const char *prefix, const char **argv,diff_flush(&opt);diff_tree_release_paths(&opt);-if(!index_was_discarded)-/* The index is still clobbered from do_diff_cache() */-discard_cache();returnupdate_index_refresh(index_fd,lock,refresh_flags);}
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
We use the path arguments in two places in reset.c: in
interactive_reset() and read_from_tree(). Both of these call
get_pathspec(), so we pass the (prefix, argv) pair to both
functions. Move the call to get_pathspec() out of these methods, for
two reasons: 1) One argument is simpler than two. 2) It lets us use
the (arguably clearer) "if (pathspec)" in place of "if (i < argc)".
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 27 ++++++++++-----------------
1 file changed, 10 insertions(+), 17 deletions(-)
@@ -287,22 +277,25 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("Could not parse object '%s'."),rev);hashcpy(sha1,commit->object.sha1);+if(i<argc)+pathspec=get_pathspec(prefix,argv+i);+if(patch_mode){if(reset_type!=NONE)die(_("--patch is incompatible with --{hard,mixed,soft}"));-returninteractive_reset(rev,argv+i,prefix);+returnrun_add_interactive(rev,"--patch=reset",pathspec);}/* git reset tree [--] paths... can be used to*loadchosenpathsfromthetreeintotheindexwithout*affectingtheworkingtreenorHEAD.*/-if(i<argc){+if(pathspec){if(reset_type==MIXED)warning(_("--mixed with paths is deprecated; use 'git reset -- <paths>' instead."));elseif(reset_type!=NONE)die(_("Cannot do %s reset with paths."),_(reset_type_names[reset_type]));-returnread_from_tree(prefix,argv+i,sha1,+returnread_from_tree(pathspec,sha1,quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);}if(reset_type==NONE)
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
Running e.g. "git reset ." in a bare repo results in an index file
being created from the HEAD commit. The differences compared to the
index are then printed as usual, but since there is no worktree, it
will appear as if all files are deleted. For example, in a bare clone
of git.git:
Unstaged changes after reset:
D .gitattributes
D .gitignore
D .mailmap
...
This happens because the check for is_bare_repository() happens after
we branch off into read_from_tree() to reset with paths. Fix by moving
the branching point after the check.
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
@@ -295,8 +295,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)elseif(reset_type!=NONE)die(_("Cannot do %s reset with paths."),_(reset_type_names[reset_type]));-returnread_from_tree(pathspec,sha1,-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);}if(reset_type==NONE)reset_type=MIXED;/* by default */
@@ -308,6 +306,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("%s reset is not allowed in a bare repository"),_(reset_type_names[reset_type]));+if(pathspec)+returnread_from_tree(pathspec,sha1,+quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+/* Soft reset does not touch the index file nor the working tree*atall,butrequirestheminagoodorder.Otherresetsreset*theindexfiletothetreeobjectweareswitchingto.*/
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
Resetting with paths does not update HEAD and there is nothing else
that a commit should be needed for. Relax the argument parsing so only
a tree is required.
The sha1 is only passed to read_from_tree(), which already only
requires a tree.
The "rev" variable we pass to run_add_interactive() will resolve to a
tree. This is fine since interactive_reset only needs the parameter to
be a treeish and doesn't use it for display purposes.
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 48 +++++++++++++++++++++++++++---------------------
t/t7102-reset.sh | 8 ++++++++
2 files changed, 35 insertions(+), 21 deletions(-)
@@ -241,7 +244,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)constchar*rev;unsignedcharsha1[20];constchar**pathspec=NULL;-structcommit*commit;conststructoptionoptions[]={OPT__QUIET(&quiet,N_("be quiet, only report errors")),OPT_SET_INT(0,"mixed",&reset_type,
@@ -263,19 +265,23 @@ int cmd_reset(int argc, const char **argv, const char *prefix)PARSE_OPT_KEEP_DASHDASH);pathspec=parse_args(argv,prefix,&rev);-if(get_sha1_committish(rev,sha1))-die(_("Failed to resolve '%s' as a valid ref."),rev);--/*-*NOTE:As"git reset $treeish -- $path"shouldbeusableon-*anytree-ish,thisisnotstrictlycorrect.Wearenot-*movingtheHEADtoanycommit;wearemerelyresettingthe-*entriesintheindextothatofatreeish.-*/-commit=lookup_commit_reference(sha1);-if(!commit)-die(_("Could not parse object '%s'."),rev);-hashcpy(sha1,commit->object.sha1);+if(!pathspec){+structcommit*commit;+if(get_sha1_committish(rev,sha1))+die(_("Failed to resolve '%s' as a valid revision."),rev);+commit=lookup_commit_reference(sha1);+if(!commit)+die(_("Could not parse object '%s'."),rev);+hashcpy(sha1,commit->object.sha1);+}else{+structtree*tree;+if(get_sha1_treeish(rev,sha1))+die(_("Failed to resolve '%s' as a valid tree."),rev);+tree=parse_tree_indirect(sha1);+if(!tree)+die(_("Could not parse object '%s'."),rev);+hashcpy(sha1,tree->object.sha1);+}if(patch_mode){if(reset_type!=NONE)
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
"git reset $pathspec" currently exits with a non-zero exit code if the
worktree is dirty after resetting, which is inconsistent with reset
without pathspec, and it makes it harder to know whether the command
really failed. Change it to exit with code 0 regardless of whether the
worktree is dirty so that non-zero indicates an error.
This makes the 4 "disambiguation" test cases in t7102 clearer since
they all used to "fail", 3 of which "failed" due to changes in the
work tree. Now only the ambiguous one fails.
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 8 +++-----
t/t2013-checkout-submodule.sh | 2 +-
t/t7102-reset.sh | 18 ++++++++++++------
3 files changed, 16 insertions(+), 12 deletions(-)
@@ -402,7 +403,8 @@ test_expect_success 'test resetting the index at give paths' '>sub/file2&&gitupdate-index--addsub/file1sub/file2&&T=$(gitwrite-tree)&&-test_must_failgitresetHEADsub/file2&&+gitresetHEADsub/file2&&+test_must_failgitdiff--quiet&&U=$(gitwrite-tree)&&echo"$T"&&echo"$U"&&
@@ -440,7 +442,8 @@ test_expect_success 'resetting specific path that is unmerged' 'echo"100644 $F3 3 file2"}|gitupdate-index--index-info&&gitls-files-u&&-test_must_failgitresetHEADfile2&&+gitresetHEADfile2&&+test_must_failgitdiff--quiet&&gitdiff-index--exit-code--cachedHEAD'
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
Thanks to b65982b (Optimize "diff-index --cached" using cache-tree,
2009-05-20), resetting with paths is much faster than resetting
without paths. Some timings for the linux-2.6 repo to illustrate this
(best of five, warm cache):
reset reset .
real 0m0.219s 0m0.080s
user 0m0.140s 0m0.040s
sys 0m0.070s 0m0.030s
These two commands should do the same thing, so instead of having the
user type the trailing " ." to get the faster do_diff_cache()-based
implementation, always use it when doing a mixed reset, with or
without paths (so "git reset $rev" would also be faster).
Timing "git reset" shows that it indeed becomes as fast as
"git reset ." after this patch.
Signed-off-by: Martin von Zweigbergk <redacted>
---
It seems like a better solution would be for unpack_trees() learn the
same tricks as do_diff_cache(). I'm leaving that a challange for the
reader :-). I did have a look a unpack_trees(), but it looked rather
overwhelming.
builtin/reset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
By not returning from inside the "if (pathspec)" block, we can let the
pathspec-aware and pathspec-less code share a bit more, making it
easier to make future changes that should affect both cases. This also
highlights the similarity between read_from_tree() and reset_index().
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 42 ++++++++++++++++++------------------------
1 file changed, 18 insertions(+), 24 deletions(-)
@@ -309,19 +309,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("%s reset is not allowed in a bare repository"),_(reset_type_names[reset_type]));-if(pathspec){-structlock_file*lock=xcalloc(1,sizeof(structlock_file));-intindex_fd=hold_locked_index(lock,1);-if(read_from_tree(pathspec,sha1))-return1;-update_index_refresh(-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);-if(write_cache(index_fd,active_cache,active_nr)||-commit_locked_index(lock))-returnerror("Could not write new index file.");-return0;-}-/* Soft reset does not touch the index file nor the working tree*atall,butrequirestheminagoodorder.Otherresetsreset*theindexfiletothetreeobjectweareswitchingto.*/
@@ -331,11 +318,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)if(reset_type!=SOFT){structlock_file*lock=xcalloc(1,sizeof(structlock_file));intnewfd=hold_locked_index(lock,1);-interr=reset_index(sha1,reset_type,quiet);-if(reset_type==KEEP&&!err)-err=reset_index(sha1,MIXED,quiet);-if(err)-die(_("Could not reset index file to revision '%s'."),rev);+if(pathspec){+if(read_from_tree(pathspec,sha1))+return1;+}else{+interr=reset_index(sha1,reset_type,quiet);+if(reset_type==KEEP&&!err)+err=reset_index(sha1,MIXED,quiet);+if(err)+die(_("Could not reset index file to revision '%s'."),rev);+}if(reset_type==MIXED)/* Report what has not been updated. */update_index_refresh(
@@ -346,14 +338,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("Could not write new index file."));}-/* Any resets update HEAD to the head being switched to,-*savingthepreviousheadinORIG_HEADbefore.*/-update_ref_status=update_refs(rev,sha1);+if(!pathspec){+/* Any resets without paths update HEAD to the head being+*switchedto,savingthepreviousheadinORIG_HEADbefore.*/+update_ref_status=update_refs(rev,sha1);-if(reset_type==HARD&&!update_ref_status&&!quiet)-print_new_head_line(commit);+if(reset_type==HARD&&!update_ref_status&&!quiet)+print_new_head_line(commit);-remove_branch_state();+remove_branch_state();+}returnupdate_ref_status;}
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
When doing a mixed reset without paths, the index is locked, read,
reset, and written back as part of the actual reset operation (in
reset_index()). Then, when showing the list of worktree modifications,
we lock the index again, refresh it, and write it.
Change this so we only write the index once, making "git reset" a
little faster. It does mean that the index lock will be held a little
longer, but the difference is small compared to the time spent
refreshing the index.
There is one minor functional difference: We used to say "Could not
write new index file." if the first write failed, and "Could not
refresh index" if the second write failed. Now, we will only use the
first message.
This speeds up "git reset" a little on the linux-2.6 repo (best of
five, warm cache):
Before After
real 0m0.239s 0m0.214s
user 0m0.160s 0m0.130s
sys 0m0.070s 0m0.080s
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
@@ -336,6 +336,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)err=reset_index(sha1,MIXED,quiet);if(err)die(_("Could not reset index file to revision '%s'."),rev);++if(reset_type==MIXED)/* Report what has not been updated. */+update_index_refresh(+quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(write_cache(newfd,active_cache,active_nr)||commit_locked_index(lock))die(_("Could not write new index file."));
@@ -347,15 +352,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)if(reset_type==HARD&&!update_ref_status&&!quiet)print_new_head_line(commit);-elseif(reset_type==MIXED){/* Report what has not been updated. */-structlock_file*index_lock=xcalloc(1,sizeof(structlock_file));-intfd=hold_locked_index(index_lock,1);-update_index_refresh(-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);-if(write_cache(fd,active_cache,active_nr)||-commit_locked_index(index_lock))-error("Could not refresh index");-}remove_branch_state();
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
If writing or committing the new index file fails, we print "Could not
write new index file." followed by "Could not reset index file to
revision $rev.". The first message seems to imply the second, so print
only the first message.
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
@@ -338,13 +338,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)interr=reset_index(sha1,reset_type,quiet);if(reset_type==KEEP&&!err)err=reset_index(sha1,MIXED,quiet);-if(!err&&-(write_cache(newfd,active_cache,active_nr)||-commit_locked_index(lock))){-err=error(_("Could not write new index file."));-}if(err)die(_("Could not reset index file to revision '%s'."),rev);+if(write_cache(newfd,active_cache,active_nr)||+commit_locked_index(lock))+die(_("Could not write new index file."));}/* Any resets update HEAD to the head being switched to,
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
Declutter cmd_reset() a bit by moving out the argument parsing to its
own function.
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 70 +++++++++++++++++++++++++++++++--------------------------
1 file changed, 38 insertions(+), 32 deletions(-)
@@ -198,36 +198,11 @@ static void die_if_unmerged_cache(int reset_type)}-intcmd_reset(intargc,constchar**argv,constchar*prefix)+staticconstchar**parse_args(intargc,constchar**argv,constchar*prefix,constchar**rev_ret){-inti=0,reset_type=NONE,update_ref_status=0,quiet=0;-intpatch_mode=0;+inti=0;constchar*rev="HEAD";-unsignedcharsha1[20],*orig=NULL,sha1_orig[20],-*old_orig=NULL,sha1_old_orig[20];-constchar**pathspec=NULL;-structcommit*commit;-structstrbufmsg=STRBUF_INIT;-conststructoptionoptions[]={-OPT__QUIET(&quiet,N_("be quiet, only report errors")),-OPT_SET_INT(0,"mixed",&reset_type,-N_("reset HEAD and index"),MIXED),-OPT_SET_INT(0,"soft",&reset_type,N_("reset only HEAD"),SOFT),-OPT_SET_INT(0,"hard",&reset_type,-N_("reset HEAD, index and working tree"),HARD),-OPT_SET_INT(0,"merge",&reset_type,-N_("reset HEAD, index and working tree"),MERGE),-OPT_SET_INT(0,"keep",&reset_type,-N_("reset HEAD but keep local changes"),KEEP),-OPT_BOOLEAN('p',"patch",&patch_mode,N_("select hunks interactively")),-OPT_END()-};--git_config(git_default_config,NULL);--argc=parse_options(argc,argv,prefix,options,git_reset_usage,-PARSE_OPT_KEEP_DASHDASH);-+unsignedcharunused[20];/**Possibleargumentsare:*
@@ -262,6 +237,40 @@ int cmd_reset(int argc, const char **argv, const char *prefix)verify_filename(prefix,argv[i],1);}}+*rev_ret=rev;+returni<argc?get_pathspec(prefix,argv+i):NULL;+}++intcmd_reset(intargc,constchar**argv,constchar*prefix)+{+intreset_type=NONE,update_ref_status=0,quiet=0;+intpatch_mode=0;+constchar*rev;+unsignedcharsha1[20],*orig=NULL,sha1_orig[20],+*old_orig=NULL,sha1_old_orig[20];+constchar**pathspec=NULL;+structcommit*commit;+structstrbufmsg=STRBUF_INIT;+conststructoptionoptions[]={+OPT__QUIET(&quiet,N_("be quiet, only report errors")),+OPT_SET_INT(0,"mixed",&reset_type,+N_("reset HEAD and index"),MIXED),+OPT_SET_INT(0,"soft",&reset_type,N_("reset only HEAD"),SOFT),+OPT_SET_INT(0,"hard",&reset_type,+N_("reset HEAD, index and working tree"),HARD),+OPT_SET_INT(0,"merge",&reset_type,+N_("reset HEAD, index and working tree"),MERGE),+OPT_SET_INT(0,"keep",&reset_type,+N_("reset HEAD but keep local changes"),KEEP),+OPT_BOOLEAN('p',"patch",&patch_mode,N_("select hunks interactively")),+OPT_END()+};++git_config(git_default_config,NULL);++argc=parse_options(argc,argv,prefix,options,git_reset_usage,+PARSE_OPT_KEEP_DASHDASH);+pathspec=parse_args(argc,argv,prefix,&rev);if(get_sha1_committish(rev,sha1))die(_("Failed to resolve '%s' as a valid ref."),rev);
@@ -277,9 +286,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("Could not parse object '%s'."),rev);hashcpy(sha1,commit->object.sha1);-if(i<argc)-pathspec=get_pathspec(prefix,argv+i);-if(patch_mode){if(reset_type!=NONE)die(_("--patch is incompatible with --{hard,mixed,soft}"));
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
Some users seem to think, knowingly or not, that being on an unborn
branch is like having a commit with an empty tree checked out, but
when run on an unborn branch, "git reset" currently fails with:
fatal: Failed to resolve 'HEAD' as a valid ref.
Instead of making users figure out that they should run
git rm --cached -r .
, let's teach "git reset" without a revision argument, when on an
unborn branch, to behave as if the user asked to reset to an empty
tree. Don't take the analogy with an empty commit too far, though, but
still disallow explictly referring to HEAD in "git reset HEAD".
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 16 ++++++++-----
t/t7106-reset-unborn-branch.sh | 52 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+), 6 deletions(-)
create mode 100755 t/t7106-reset-unborn-branch.sh
@@ -265,7 +265,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)PARSE_OPT_KEEP_DASHDASH);pathspec=parse_args(argv,prefix,&rev);-if(!pathspec){+unborn=!strcmp(rev,"HEAD")&&get_sha1("HEAD",sha1);+if(unborn){+/* reset on unborn branch: treat as reset to empty tree */+hashcpy(sha1,EMPTY_TREE_SHA1_BIN);+}elseif(!pathspec){structcommit*commit;if(get_sha1_committish(rev,sha1))die(_("Failed to resolve '%s' as a valid revision."),rev);
@@ -286,7 +290,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)if(patch_mode){if(reset_type!=NONE)die(_("--patch is incompatible with --{hard,mixed,soft}"));-returnrun_add_interactive(rev,"--patch=reset",pathspec);+returnrun_add_interactive(sha1_to_hex(sha1),"--patch=reset",pathspec);}/* git reset tree [--] paths... can be used to
@@ -340,16 +344,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("Could not write new index file."));}-if(!pathspec){+if(!pathspec&&!unborn){/* Any resets without paths update HEAD to the head being*switchedto,savingthepreviousheadinORIG_HEADbefore.*/update_ref_status=update_refs(rev,sha1);if(reset_type==HARD&&!update_ref_status&&!quiet)print_new_head_line(lookup_commit_reference(sha1));--remove_branch_state();}+if(!pathspec)+remove_branch_state();returnupdate_ref_status;}
@@ -0,0 +1,52 @@+#!/bin/sh++test_description='git reset should work on unborn branch'+../test-lib.sh++test_expect_success'setup''+echoa>a&&+echob>b+'++test_expect_success'reset''+gitaddab&&+gitreset&&+test"$(gitls-files)"=""+'++test_expect_success'reset HEAD''+rm.git/index&&+gitaddab&&+test_must_failgitresetHEAD+'++test_expect_success'reset $file''+rm.git/index&&+gitaddab&&+gitreseta&&+test"$(gitls-files)"="b"+'++test_expect_success'reset -p''+rm.git/index&&+gitadda&&+echoy|gitreset-p&&+test"$(gitls-files)"=""+'++test_expect_success'reset --soft is a no-op''+rm.git/index&&+gitadda&&+gitreset--soft+test"$(gitls-files)"="a"+'++test_expect_success'reset --hard''+rm.git/index&&+gitadda&&+gitreset--hard&&+test"$(gitls-files)"=""&&+test_path_is_missinga+'++test_done
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
In preparation for the/a following patch, move the locking, writing
and committing of the index file out of update_index_refresh(). The
code duplication caused will soon be taken care of. What remains of
update_index_refresh() is just one line, but it is still called from
two places, so let's leave it for now.
In the process, we expose and fix the minor UI bug that makes us print
"Could not refresh index" when we fail to write the index file when
invoked with a pathspec. Copy the error message from the pathspec-less
codepath ("Could not write new index file.").
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
@@ -109,19 +109,10 @@ static void print_new_head_line(struct commit *commit)printf("\n");}-staticintupdate_index_refresh(intfd,structlock_file*index_lock,intflags)+staticvoidupdate_index_refresh(intflags){-if(!index_lock){-index_lock=xcalloc(1,sizeof(structlock_file));-fd=hold_locked_index(index_lock,1);-}-refresh_index(&the_index,(flags),NULL,NULL,_("Unstaged changes after reset:"));-if(write_cache(fd,active_cache,active_nr)||-commit_locked_index(index_lock))-returnerror("Could not refresh index");-return0;}staticvoidupdate_index_from_diff(structdiff_queue_struct*q,
@@ -321,9 +312,14 @@ int cmd_reset(int argc, const char **argv, const char *prefix)if(pathspec){structlock_file*lock=xcalloc(1,sizeof(structlock_file));intindex_fd=hold_locked_index(lock,1);-returnread_from_tree(pathspec,sha1)||-update_index_refresh(index_fd,lock,-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(read_from_tree(pathspec,sha1))+return1;+update_index_refresh(+quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(write_cache(index_fd,active_cache,active_nr)||+commit_locked_index(lock))+returnerror("Could not write new index file.");+return0;}/* Soft reset does not touch the index file nor the working tree
@@ -351,9 +347,15 @@ int cmd_reset(int argc, const char **argv, const char *prefix)if(reset_type==HARD&&!update_ref_status&&!quiet)print_new_head_line(commit);-elseif(reset_type==MIXED)/* Report what has not been updated. */-update_index_refresh(0,NULL,-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+elseif(reset_type==MIXED){/* Report what has not been updated. */+structlock_file*index_lock=xcalloc(1,sizeof(structlock_file));+intfd=hold_locked_index(index_lock,1);+update_index_refresh(+quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(write_cache(fd,active_cache,active_nr)||+commit_locked_index(index_lock))+error("Could not refresh index");+}remove_branch_state();
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
The switch statement towards the end of reset.c is missing case arms
for KEEP and MERGE for no obvious reason, and soon the only non-empty
case arm will be the one for HARD. So let's proactively replace it by
if-else, which will let us move one if statement out without leaving
funny-looking left-overs.
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
@@ -349,18 +349,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)*savingthepreviousheadinORIG_HEADbefore.*/update_ref_status=update_refs(rev,sha1);-switch(reset_type){-caseHARD:-if(!update_ref_status&&!quiet)-print_new_head_line(commit);-break;-caseSOFT:/* Nothing else to do. */-break;-caseMIXED:/* Report what has not been updated. */+if(reset_type==HARD&&!update_ref_status&&!quiet)+print_new_head_line(commit);+elseif(reset_type==MIXED)/* Report what has not been updated. */update_index_refresh(0,NULL,quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);-break;-}remove_branch_state();
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
Now that there is only one caller left to the single-line method
update_index_refresh(), inline it.
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
@@ -329,9 +323,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("Could not reset index file to revision '%s'."),rev);}-if(reset_type==MIXED)/* Report what has not been updated. */-update_index_refresh(-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(reset_type==MIXED){/* Report what has not been updated. */+intflags=quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN;+refresh_index(&the_index,flags,NULL,NULL,+_("Unstaged changes after reset:"));+}if(write_cache(newfd,active_cache,active_nr)||commit_locked_index(lock))
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
By extracting the code for updating the HEAD and ORIG_HEAD symbolic
references to a separate function, we declutter cmd_reset() a bit and
we make it clear that e.g. the four variables {,sha1_}{,old_}orig are
only used by this code.
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)
@@ -333,17 +352,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)/* Any resets update HEAD to the head being switched to,*savingthepreviousheadinORIG_HEADbefore.*/-if(!get_sha1("ORIG_HEAD",sha1_old_orig))-old_orig=sha1_old_orig;-if(!get_sha1("HEAD",sha1_orig)){-orig=sha1_orig;-set_reflog_message(&msg,"updating ORIG_HEAD",NULL);-update_ref(msg.buf,"ORIG_HEAD",orig,old_orig,0,MSG_ON_ERR);-}-elseif(old_orig)-delete_ref("ORIG_HEAD",old_orig,0);-set_reflog_message(&msg,"updating HEAD",rev);-update_ref_status=update_ref(msg.buf,"HEAD",sha1,orig,0,MSG_ON_ERR);+update_ref_status=update_refs(rev,sha1);switch(reset_type){caseHARD:
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
"git reset --keep" calls reset_index_file() twice, first doing a
two-way merge to the target revision, updating the index and worktree,
and then resetting the index. After each call, we write the index
file.
In the unlikely event that the second call to reset_index_file()
fails, the index will have been merged to the target revision, but
HEAD will not be updated, leaving the user with a dirty index.
By moving the locking, writing and committing out of
reset_index_file() and into the caller, we can avoid writing the index
twice, thereby making the sure we don't end up in the half-way reset
state. As a bonus, we speed up "git reset --keep" a little on the
linux-2.6 repo (best of five, warm cache):
Before After
real 0m0.315s 0m0.296s
user 0m0.290s 0m0.280s
sys 0m0.020s 0m0.010s
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
@@ -38,14 +38,12 @@ static inline int is_merge(void)return!access(git_path("MERGE_HEAD"),F_OK);}-staticintreset_index_file(constunsignedchar*sha1,intreset_type,intquiet)+staticintreset_index(constunsignedchar*sha1,intreset_type,intquiet){intnr=1;-intnewfd;structtree_descdesc[2];structtree*tree;structunpack_trees_optionsopts;-structlock_file*lock=xcalloc(1,sizeof(structlock_file));memset(&opts,0,sizeof(opts));opts.head_idx=1;
@@ -67,8 +65,6 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quietopts.reset=1;}-newfd=hold_locked_index(lock,1);-read_cache_unmerged();if(reset_type==KEEP){
@@ -91,10 +87,6 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quietprime_cache_tree(&active_cache_tree,tree);}-if(write_cache(newfd,active_cache,active_nr)||-commit_locked_index(lock))-returnerror(_("Could not write new index file."));-return0;}
@@ -341,9 +333,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die_if_unmerged_cache(reset_type);if(reset_type!=SOFT){-interr=reset_index_file(sha1,reset_type,quiet);+structlock_file*lock=xcalloc(1,sizeof(structlock_file));+intnewfd=hold_locked_index(lock,1);+interr=reset_index(sha1,reset_type,quiet);if(reset_type==KEEP&&!err)-err=reset_index_file(sha1,MIXED,quiet);+err=reset_index(sha1,MIXED,quiet);+if(!err&&+(write_cache(newfd,active_cache,active_nr)||+commit_locked_index(lock))){+err=error(_("Could not write new index file."));+}if(err)die(_("Could not reset index file to revision '%s'."),rev);}
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
Changes since v1:
- Spelling fixes.
- Explained how "git reset -- $pathspec" in bare repo is broken.
- Provided motivation for replacement of switch by if-else
- Fixed argv/argc handling by removing use of argc.
- Replaced "don't refresh index on --quiet" patch by one that just
inlines update_index_refresh()
- Incorporated fixes from Junio's repo
- Provided some motivation for "replace switch by if-else" amd moved
the patch later in the series.
Thanks for reviewing!
Martin von Zweigbergk (19):
reset $pathspec: no need to discard index
reset $pathspec: exit with code 0 if successful
reset.c: pass pathspec around instead of (prefix, argv) pair
reset: don't allow "git reset -- $pathspec" in bare repo
reset.c: extract function for parsing arguments
reset.c: remove unnecessary variable 'i'
reset.c: extract function for updating {ORIG_,}HEAD
reset.c: share call to die_if_unmerged_cache()
reset --keep: only write index file once
reset: avoid redundant error message
reset.c: replace switch by if-else
reset.c: move update_index_refresh() call out of read_from_tree()
reset.c: move lock, write and commit out of update_index_refresh()
reset [--mixed]: only write index file once
reset.c: finish entire cmd_reset() whether or not pathspec is given
reset.c: inline update_index_refresh()
reset $sha1 $pathspec: require $sha1 only to be treeish
reset: allow reset on unborn branch
reset [--mixed]: use diff-based reset whether or not pathspec was
given
builtin/reset.c | 283 +++++++++++++++++++----------------------
t/t2013-checkout-submodule.sh | 2 +-
t/t7102-reset.sh | 26 +++-
t/t7106-reset-unborn-branch.sh | 52 ++++++++
4 files changed, 203 insertions(+), 160 deletions(-)
create mode 100755 t/t7106-reset-unborn-branch.sh
--
1.8.1.1.454.gce43f05
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
The final part of cmd_reset() essentially looks like:
if (pathspec) {
...
read_from_tree(...);
} else {
...
reset_index(...);
update_index_refresh(...);
...
}
where read_from_tree() internally also calls
update_index_refresh(). Move the call to update_index_refresh() out of
read_from_tree for symmetry with the 'else' block, making
read_from_tree() and reset_index() closer in functionality.
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
@@ -322,9 +318,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)die(_("%s reset is not allowed in a bare repository"),_(reset_type_names[reset_type]));-if(pathspec)-returnread_from_tree(pathspec,sha1,-quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+if(pathspec){+structlock_file*lock=xcalloc(1,sizeof(structlock_file));+intindex_fd=hold_locked_index(lock,1);+returnread_from_tree(pathspec,sha1)||+update_index_refresh(index_fd,lock,+quiet?REFRESH_QUIET:REFRESH_IN_PORCELAIN);+}/* Soft reset does not touch the index file nor the working tree*atall,butrequirestheminagoodorder.Otherresetsreset
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
Use a single condition to guard the call to die_if_unmerged_cache for
both --soft and --keep. This avoids the small distraction of the
precondition check from the logic following it.
Also change an instance of
if (e)
err = err || f();
to the almost as short, but clearer
if (e && !err)
err = f();
(which is equivalent since we only care whether exit code is 0)
Signed-off-by: Martin von Zweigbergk <redacted>
---
builtin/reset.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
@@ -337,15 +337,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)/* Soft reset does not touch the index file nor the working tree*atall,butrequirestheminagoodorder.Otherresetsreset*theindexfiletothetreeobjectweareswitchingto.*/-if(reset_type==SOFT)+if(reset_type==SOFT||reset_type==KEEP)die_if_unmerged_cache(reset_type);-else{-interr;-if(reset_type==KEEP)-die_if_unmerged_cache(reset_type);-err=reset_index_file(sha1,reset_type,quiet);-if(reset_type==KEEP)-err=err||reset_index_file(sha1,MIXED,quiet);++if(reset_type!=SOFT){+interr=reset_index_file(sha1,reset_type,quiet);+if(reset_type==KEEP&&!err)+err=reset_index_file(sha1,MIXED,quiet);if(err)die(_("Could not reset index file to revision '%s'."),rev);}
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:46
Throughout most of parse_args(), the variable 'i' remains at 0. Many
references are still made to the variable even when it could only have
the value 0. This made at least me, who has relatively little
experience with C programming styles, think that parts of the function
was meant to be part of a loop. To avoid such confusion, remove the
variable and also the 'argc' parameter and check for NULL trailing
argv instead.
Signed-off-by: Martin von Zweigbergk <redacted>
---
I explained a bit more why I was confused by the current style, but
I'm also perfectly happy if you just drop the patch (there would be
some minor conflicts in a later patch, though).
builtin/reset.c | 33 ++++++++++++++++-----------------
1 file changed, 16 insertions(+), 17 deletions(-)
@@ -211,34 +210,34 @@ static const char **parse_args(int argc, const char **argv, const char *prefix,*gitreset[-opts]--<paths>...*gitreset[-opts]<paths>...*-*Atthispoint,argv[i]pointsimmediatelyafter[-opts].+*Atthispoint,argvpointsimmediatelyafter[-opts].*/-if(i<argc){-if(!strcmp(argv[i],"--")){-i++;/* reset to HEAD, possibly with paths */-}elseif(i+1<argc&&!strcmp(argv[i+1],"--")){-rev=argv[i];-i+=2;+if(argv[0]){+if(!strcmp(argv[0],"--")){+argv++;/* reset to HEAD, possibly with paths */+}elseif(argv[1]&&!strcmp(argv[1],"--")){+rev=argv[0];+argv+=2;}/*-*Otherwise,argv[i]couldbeeither<rev>or<paths>and+*Otherwise,argv[0]couldbeeither<rev>or<paths>and*hastobeunambiguous.*/-elseif(!get_sha1_committish(argv[i],unused)){+elseif(!get_sha1_committish(argv[0],unused)){/*-*Ok,argv[i]lookslikearev;itshouldnot+*Ok,argv[0]lookslikearev;itshouldnot*beafilename.*/-verify_non_filename(prefix,argv[i]);-rev=argv[i++];+verify_non_filename(prefix,argv[0]);+rev=*argv++;}else{/* Otherwise we treat this as a filename */-verify_filename(prefix,argv[i],1);+verify_filename(prefix,argv[0],1);}}*rev_ret=rev;-returni<argc?get_pathspec(prefix,argv+i):NULL;+returnargv[0]?get_pathspec(prefix,argv):NULL;}intcmd_reset(intargc,constchar**argv,constchar*prefix)
@@ -270,7 +269,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)argc=parse_options(argc,argv,prefix,options,git_reset_usage,PARSE_OPT_KEEP_DASHDASH);-pathspec=parse_args(argc,argv,prefix,&rev);+pathspec=parse_args(argv,prefix,&rev);if(get_sha1_committish(rev,sha1))die(_("Failed to resolve '%s' as a valid ref."),rev);
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:47
---
Sorry, I forgot the documentation updates. I hope this looks ok. Can
you squash this in, Junio? Thanks.
I don't think any documentation update is necessary for the "reset on
unborn branch" patch. Let me know if you think differently.
Documentation/git-reset.txt | 18 +++++++++---------
builtin/reset.c | 4 ++--
2 files changed, 11 insertions(+), 11 deletions(-)
@@ -8,20 +8,20 @@ git-reset - Reset current HEAD to the specified state SYNOPSIS -------- [verse]-'git reset' [-q] [<commit>] [--] <paths>...-'git reset' (--patch | -p) [<commit>] [--] [<paths>...]+'git reset' [-q] [<tree-ish>] [--] <paths>...+'git reset' (--patch | -p) [<tree-sh>] [--] [<paths>...] 'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [<commit>] DESCRIPTION ------------In the first and second form, copy entries from <commit> to the index.+In the first and second form, copy entries from <tree-ish> to the index. In the third form, set the current branch head (HEAD) to <commit>, optionally-modifying index and working tree to match. The <commit> defaults to HEAD-in all forms.+modifying index and working tree to match. The <tree-ish>/<commit> defaults+to HEAD in all forms.-'git reset' [-q] [<commit>] [--] <paths>...::+'git reset' [-q] [<tree-ish>] [--] <paths>...:: This form resets the index entries for all <paths> to their- state at <commit>. (It does not affect the working tree, nor+ state at <tree-ish>. (It does not affect the working tree, nor the current branch.) + This means that `git reset <paths>` is the opposite of `git add
@@ -34,9 +34,9 @@ Alternatively, using linkgit:git-checkout[1] and specifying a commit, you can copy the contents of a path out of a commit to the index and to the working tree in one go.-'git reset' (--patch | -p) [<commit>] [--] [<paths>...]::+'git reset' (--patch | -p) [<tree-ish>] [--] [<paths>...]:: Interactively select hunks in the difference between the index- and <commit> (defaults to HEAD). The chosen hunks are applied+ and <tree-ish> (defaults to HEAD). The chosen hunks are applied in reverse to the index. + This means that `git reset -p` is the opposite of `git add -p`, i.e.
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 22:55:47
On Wed, Jan 16, 2013 at 10:00 AM, Martin von Zweigbergk
[off-list ref] wrote:
---
Sorry, I forgot the documentation updates. I hope this looks ok. Can
you squash this in, Junio? Thanks.
I see the series just entered 'next', so I guess it would have to go
on top then. Perhaps with a commit message like as simple as the
following. Let me know if you prefer it to be resent as a proper
patch. Sorry about the noise.
reset: update documentation to require only tree-ish with paths
When resetting with paths, we no longer require a commit argument, but
only a tree-ish. Update the documentation and synopsis accordingly.