Re: [PATCH] mergetool: don't skip modify/remove conflicts

[Junio C Hamano <gitster@pobox.com>]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> diff --git a/rerere.h b/rerere.h
> index eaa9004..107a2bc 100644
> --- a/rerere.h
> +++ b/rerere.h
> @@ -6,6 +6,9 @@
>  #define RERERE_AUTOUPDATE   01
>  #define RERERE_NOAUTOUPDATE 02
>  
> +extern void *RERERE_UTIL_PUNTED;

This is for paths without three-stages, i.e. ones rerere won't help. 
Needs some comment for these two symbols.

Leading "RERERE_" is necessary for clarifying the namespace, PUNTED is
necessary because it defines what the symbols means, but why do we need
UTIL in the name?  Drop it.

> +extern void *RERERE_UTIL_STAGED;

This is for what kind?  The contents on the filesystem is ready to go,
added to the index, but still in MERGE_RR (i.e. "git rerere" not run yet)?

Is the real problem that git-mergetool is not running rerere when it
should, I wonder...

> diff --git a/rerere.c b/rerere.c
> index eb47f97..61cac54 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -7,6 +7,10 @@
>  #include "ll-merge.h"
>  #include "attr.h"
>  
> +#define RERERE_UTIL_ELIGIBLE NULL
> +void *RERERE_UTIL_PUNTED = &RERERE_UTIL_PUNTED;
> +void *RERERE_UTIL_STAGED = &RERERE_UTIL_STAGED;
> +

Same for "ELIGIBLE"; describe what it means.

>  	for (i = 0; i < active_nr; i++) {
>  		struct cache_entry *e = active_cache[i];
>  		struct string_list_item *it;
>  
> -		if (!ce_stage(e))
> +		if (!ce_stage(e)) {
>  			continue;
> +		}

Unnecessary change.

>  		it = string_list_insert(conflict, (const char *)e->name);
> -		it->util = NULL;
> +		it->util = RERERE_UTIL_PUNTED;
>  		if (ce_stage(e) == 1) {
> +			it->util = RERERE_UTIL_STAGED;

Hmm, I thought that you were taling about paths that the user
hand-resolved and then ran "git add" on.  Why is this marked "STAGED"?

Either your logic is wrong, or the name of the symbol is.

In any case, the original code is letting rerere handle both two-way
merge (stage #2 and #3 exist without state #1) and three-way merge (all
three stages exist) case, and changing only the body of this if statement
smells there is extremely fishy going on.

> @@ -487,8 +494,9 @@ static int do_plain_rerere(struct string_list *rr, int fd)
>  
>  	for (i = 0; i < conflict.nr; i++) {
>  		const char *path = conflict.items[i].string;
> -		if (!conflict.items[i].util)
> -			continue; /* punted */
> +		if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
> +			conflict.items[i].util == RERERE_UTIL_STAGED)
> +			continue;
>  		if (!string_list_has_string(rr, path)) {
>  			unsigned char sha1[20];
>  			char *hex;
> @@ -648,8 +656,9 @@ int rerere_forget(const char **pathspec)
>  	find_conflict(&conflict);
>  	for (i = 0; i < conflict.nr; i++) {
>  		struct string_list_item *it = &conflict.items[i];
> -		if (!conflict.items[i].util)
> -			continue; /* punted */
> +		if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
> +			conflict.items[i].util == RERERE_UTIL_STAGED)
> +			continue;

There are a few repetition of "if it is marked with PUNTED or STAGED"; can
you make it into a small helper function and give it a _meaningful_ name?
What does it mean for an entry to be marked with either of these marks?

Thanks.