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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2011, 21:45 UTC
Message-ID: <7vzkq4opaw.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1297134518-4387-1-git-send-email-martin.von.zweigbergk@gmail.com>

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

Show 9 quoted lines

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

Show 12 quoted lines

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

Show 8 quoted lines

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

Show 5 quoted lines

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

Show 21 quoted lines

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