# [PATCH] reset: trivial refactoring 4 messages from 2013-06-13 to 2013-06-13. Participants: Felipe Contreras, Martin von Zweigbergk, Junio C Hamano. Thread: https://gitlist.dev/t/34128 ## Felipe Contreras, 2013-06-13 18:15 Subject: [PATCH] reset: trivial refactoring Message-ID: <1371147305-20771-1-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1371147305-20771-1-git-send-email-felipe.contreras%40gmail.com ``` After commit 3fde386 (reset [--mixed]: use diff-based reset whether or not pathspec was given), some code can't be reached, and other code can be moved to the 'reset_type == MIXED' check. Let's remove the check that can't be reached, and move the code is specific to MIXED. Signed-off-by: Felipe Contreras --- builtin/reset.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 6032131..68739ba 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -82,7 +82,7 @@ static int reset_index(const unsigned char *sha1, int reset_type, int quiet) if (unpack_trees(nr, desc, &opts)) return -1; - if (reset_type == MIXED || reset_type == HARD) { + if (reset_type == HARD) { tree = parse_tree_indirect(sha1); prime_cache_tree(&active_cache_tree, tree); } @@ -323,8 +323,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); int newfd = hold_locked_index(lock, 1); if (reset_type == MIXED) { + int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN; if (read_from_tree(pathspec, sha1)) return 1; + refresh_index(&the_index, flags, NULL, NULL, + _("Unstaged changes after reset:")); } else { int err = reset_index(sha1, reset_type, quiet); if (reset_type == KEEP && !err) @@ -333,12 +336,6 @@ 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. */ - int flags = 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)) die(_("Could not write new index file.")); -- 1.8.3.698.g079b096 ``` ## Martin von Zweigbergk, 2013-06-13 21:13 Subject: Re: [PATCH] reset: trivial refactoring Message-ID: URL: https://gitlist.dev/e/CANiSa6i5xL-V%2BW46zWXGzLc%2BdSO6a%2BV0BLPt7hFgGZGXpp03BQ%40mail.gmail.com In-Reply-To: <1371147305-20771-1-git-send-email-felipe.contreras@gmail.com> ``` On Thu, Jun 13, 2013 at 11:15 AM, Felipe Contreras wrote: > @@ -82,7 +82,7 @@ static int reset_index(const unsigned char *sha1, int reset_type, int quiet) > if (unpack_trees(nr, desc, &opts)) > return -1; > > - if (reset_type == MIXED || reset_type == HARD) { > + if (reset_type == HARD) { Are you sure that this can not be reached given that... > @@ -323,8 +323,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) > struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); > int newfd = hold_locked_index(lock, 1); > if (reset_type == MIXED) { > + int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN; > if (read_from_tree(pathspec, sha1)) > return 1; > + refresh_index(&the_index, flags, NULL, NULL, > + _("Unstaged changes after reset:")); > } else { > int err = reset_index(sha1, reset_type, quiet); > if (reset_type == KEEP && !err) ...the line after this one reads err = reset_index(sha1, MIXED, quiet); ? I don't know what the consequence of not calling prime_cache_tree() would be, though. The merging of the two if blocks looks good. Thanks. ``` ## Felipe Contreras, 2013-06-13 21:20 Subject: Re: [PATCH] reset: trivial refactoring Message-ID: URL: https://gitlist.dev/e/CAMP44s17nRzh157hyLMjKWi3ZOLqUui%3DuMVMwyffcGFFrucd_w%40mail.gmail.com In-Reply-To: ``` On Thu, Jun 13, 2013 at 4:13 PM, Martin von Zweigbergk wrote: > On Thu, Jun 13, 2013 at 11:15 AM, Felipe Contreras > wrote: >> @@ -82,7 +82,7 @@ static int reset_index(const unsigned char *sha1, int reset_type, int quiet) >> if (unpack_trees(nr, desc, &opts)) >> return -1; >> >> - if (reset_type == MIXED || reset_type == HARD) { >> + if (reset_type == HARD) { > > Are you sure that this can not be reached given that... > >> @@ -323,8 +323,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) >> struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); >> int newfd = hold_locked_index(lock, 1); >> if (reset_type == MIXED) { >> + int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN; >> if (read_from_tree(pathspec, sha1)) >> return 1; >> + refresh_index(&the_index, flags, NULL, NULL, >> + _("Unstaged changes after reset:")); >> } else { >> int err = reset_index(sha1, reset_type, quiet); >> if (reset_type == KEEP && !err) > > ...the line after this one reads > > err = reset_index(sha1, MIXED, quiet); That's true. Only the rest of the patch makes sense then. It seems there should be a way to have a single call reset_index(KEEP), so we don't have to call again with MIXED, but perhaps that's for later. -- Felipe Contreras ``` ## Junio C Hamano, 2013-06-13 21:34 Subject: Re: [PATCH] reset: trivial refactoring Message-ID: <7v38slu23u.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v38slu23u.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Martin von Zweigbergk writes: > ...the line after this one reads > > err = reset_index(sha1, MIXED, quiet); > > ? I don't know what the consequence of not calling prime_cache_tree() > would be, though. It does not affect correctness, but makes the subsequent "git status", the part that internally computes "diff-cache --index" to see what changes have been added to the index, more costly. After doing "reset --hard $commit" or just "reset $commit", we know that the contents of the index must match $commit^{tree}, and writing out any subpart of the index that corresponds to a directory (including the top-level one, i.e. the whole index) must match the corresponding subtree of $commit^{tree}. And that is why we prime the cache-tree that was discarded by unpack_trees() at the very end. Then incremental "git add" to the resulting index after that can invalidate only the parts of the index and cache-tree while relieving the next "write-tree" (most often done by the next "git commit") from having to compute the tree objects for parts of the index that have not been touched since the "reset" operation. I do not use "reset --keep $commit" very often myself, but IIRC, it is like "checkout $commit" (and not "checkout -m $commit") in that the resulting index matches $commit^{tree}, so I think priming the cache-tree just like --hard/--mixed is the right thing to do. ```