Re: [PATCH 08/12] merge-ort: provide a merge_get_conflicted_files() helper function

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Elijah,

On Sat, 22 Jan 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> After a merge, this function allows the user to extract the same
> information that would be printed by `ls-files -u` -- conflicted
> files with their mode, oid, and stage.

Hmm. Okay.

I am not really a fan of that output where we use a variable
number of lines per file. As in: in the regular case, where a file was
modified in divergent ways, we get three lines. But if it was deleted in
one branch, or if it was created in both branches, we have only two lines.

Frankly, I'd much rather have 3 lines for each and every conflict.

Granted, we currently only have three stages, and we can pretty much
guarantee that at least two of these stages are non-empty (otherwise where
would be the conflict?).

Meaning: Even if stage 3 is missing from the first conflict and stage 1 is
missing from the second conflict, in the output we would see stages 1, 2,
2, 3, i.e. a duplicate stage 2, signifying that we're talking about two
different conflicts.

But still. It makes me uneasy to have that much variability in
machine-consumable output.

And who knows, maybe if we're ever implementing more sophisticated merge
strategies for octopus merges, we might end up with more stages...

In other words...

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c | 31 +++++++++++++++++++++++++++++++
>  merge-ort.h | 21 +++++++++++++++++++++
>  2 files changed, 52 insertions(+)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index b78dde55ad9..5e7cea6cc8f 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4275,6 +4275,37 @@ void merge_display_update_messages(struct merge_options *opt,
>  	trace2_region_leave("merge", "display messages", opt->repo);
>  }
>
> +void merge_get_conflicted_files(struct merge_result *result,
> +				struct string_list *conflicted_files)
> +{
> +	struct hashmap_iter iter;
> +	struct strmap_entry *e;
> +	struct merge_options_internal *opti = result->priv;
> +
> +	strmap_for_each_entry(&opti->conflicted, &iter, e) {
> +		const char *path = e->key;
> +		struct conflict_info *ci = e->value;
> +		int i;
> +
> +		VERIFY_CI(ci);
> +
> +		for (i = MERGE_BASE; i <= MERGE_SIDE2; i++) {
> +			struct stage_info *si;
> +
> +			if (!(ci->filemask & (1ul << i)))
> +				continue;
> +
> +			si = xmalloc(sizeof(*si));
> +			si->stage = i+1;
> +			si->mode = ci->stages[i].mode;
> +			oidcpy(&si->oid, &ci->stages[i].oid);
> +			string_list_append(conflicted_files, path)->util = si;
> +		}
> +	}
> +	/* string_list_sort() uses a stable sort, so we're good */
> +	string_list_sort(conflicted_files);
> +}
> +
>  void merge_switch_to_result(struct merge_options *opt,
>  			    struct tree *head,
>  			    struct merge_result *result,
> diff --git a/merge-ort.h b/merge-ort.h
> index d643b47cb7c..e635a294ea8 100644
> --- a/merge-ort.h
> +++ b/merge-ort.h
> @@ -2,6 +2,7 @@
>  #define MERGE_ORT_H
>
>  #include "merge-recursive.h"
> +#include "hash.h"
>
>  struct commit;
>  struct tree;
> @@ -89,6 +90,26 @@ void merge_display_update_messages(struct merge_options *opt,
>  				   struct merge_result *result,
>  				   FILE *stream);
>
> +struct stage_info {
> +	struct object_id oid;
> +	int mode;
> +	int stage;
> +};

... I'd rather not tack this onto a `string_list` but instead have
something like:

	struct conflict_info {
		struct {
			struct object_id oid;
			int mode;
			const char *path;
		} stages[3]
	};

where `path` can be `NULL` to indicate that this stage is missing.

Apart from this concern about the overall design, the patch looks good, of
course.

Ciao,
Dscho

> +
> +/*
> + * Provide a list of path -> {struct stage_info*} mappings for
> + * all conflicted files.  Note that each path could appear up to three
> + * times in the list, corresponding to 3 different stage entries.  In short,
> + * this basically provides the info that would be printed by `ls-files -u`.
> + *
> + * result should have been populated by a call to
> + * one of the merge_incore_[non]recursive() functions.
> + *
> + * conflicted_files should be empty before calling this function.
> + */
> +void merge_get_conflicted_files(struct merge_result *result,
> +				struct string_list *conflicted_files);
> +
>  /* Do needed cleanup when not calling merge_switch_to_result() */
>  void merge_finalize(struct merge_options *opt,
>  		    struct merge_result *result);
> --
> gitgitgadget
>
>