threads / patch / 34128

patchreset: trivial refactoring

Subject: [PATCH] reset: trivial refactoring

## tl;dr

4 messages between Jun 13, 2013 and Jun 13, 2013. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Felipe Contreras· Jun 13, 2013, 18:15 UTC · lore

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 <felipe.contreras@gmail.com>
---
 builtin/reset.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Show changes to builtin/reset.c +4 −7

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· Jun 13, 2013, 21:13 UTC · re: Felipe Contreras · lore

Re: [PATCH] reset: trivial refactoring

On Thu, Jun 13, 2013 at 11:15 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 6 quoted lines

> @@ -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...

Show 12 quoted lines

> @@ -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· Jun 13, 2013, 21:20 UTC · re: Martin von Zweigbergk · lore

Re: [PATCH] reset: trivial refactoring

On Thu, Jun 13, 2013 at 4:13 PM, Martin von Zweigbergk <martinvonz@gmail.com> wrote:

Show 27 quoted lines

> On Thu, Jun 13, 2013 at 11:15 AM, Felipe Contreras
> <felipe.contreras@gmail.com> 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· Jun 13, 2013, 21:34 UTC · re: Martin von Zweigbergk · lore

Re: [PATCH] reset: trivial refactoring

Martin von Zweigbergk <martinvonz@gmail.com> writes:

Show 6 quoted lines

> ...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.