patchbuiltin/merge.c: drop a parameter that is never used

Since the very beginning when we added the "renormalizing" parameter to this function with 7610fa57 (merge-recursive --renormalize, 2010-08-05), nobody seems to have ever referenced it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * ... or is there any "renormalization" the said commit meant to
   but forgot to do?

 builtin/merge.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Junio C Hamano wrote:

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

I suspect it's related to this TODO from rerere.c::handle_cache:

	/*
	 * NEEDSWORK: handle conflicts from merges with
	 * merge.renormalize set, too
	 */
	ll_merge(&result, path, &mmfile[0], NULL,
		 &mmfile[1], "ours",
		 &mmfile[2], "theirs", NULL);

But if someone has time for it, rather than plumbing in a useless parameter that goes nowhere, it would be better to add tests as a reminder of what is unimplemented. :)

Thanks, Jonathan

Two identical loops in suggest_conflicts() in merge, and do_recursive_merge() in sequencer, can use a single helper function extracted from the latter that prepares the "Conflicts:" hint that is meant to remind the user the paths for which merge conflicts had to be resolved to write a better commit log message.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 builtin/merge.c | 18 +++++-------------
 sequencer.c     | 35 ++++++++++++++++++++---------------
 sequencer.h     |  1 +
 3 files changed, 26 insertions(+), 28 deletions(-)

Just like other hints such as "Change to be committed" we show in the editor to remind the committer what paths were involved in the resulting commit to improve their log message, this section is merely a reminder. Traditionally, it was not made into comments primarily because it has to be generated outside wt-status infrastructure, and secondary it was meant as a bit stronger reminder than the rest (i.e. explaining how you resolved conflicts is much more important than mentioning what you did to every paths involved in the commit), but that still does not make this hint a hint, which should be commented out by default.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c                | 42 ++++++++++++++++++++---------------------
 sequencer.c                     |  7 +++----
 t/t3507-cherry-pick-conflict.sh | 22 +++++++++++++--------
 3 files changed, 38 insertions(+), 33 deletions(-)

On Fri, Oct 24, 2014 at 02:24:37PM -0700, Junio C Hamano wrote:

Yay. I like this new behavior much better.

Just to play devil's advocate for a moment, though, are we hurting people who find it useful to record that information in the commit message?

For the most part, combined-diff (and --cc) will show the interesting cases anyway. But if you take a whole file from one side of the merge, then there is nothing interesting for diff to show. Do people still want to get that more complete list of potentially interesting files? And if so, how do they do it? I think there really isn't a great way besides repeating the merge.

If that is the only casualty, I think it is probably a net-win. We may want better tooling around viewing the merge later, but that can wait until somebody steps up with a real use case (because even that conflict list may not be completely what they want; they may also want the list of files that were auto-merged successfully, for example). And I think there was work recently on a diff view for merge commits that involved recreating the merge (I do not remember the details, though).

This ends up adding a space followed by a tab. Besides being redundant, it makes my editor highlight it as a whitespace error. I realize this is a pretty minor nit, though.

-Peff

Jeff King <peff@peff.net> writes:

I thought about it, but ultimately, they are using it wrong if they did depend on them. As you said, the paths themselves are not that interesting, unless they are accompanied by description in the log message explaining what caused conflicts and how they were resolved at the semantic level. The hints are to remind them what conflicts in which paths to describe.

I do not mind "merge.commentConflictsHint = no" as a backward compatibility toggle, if somebody cares deeply about it, though.

Yup.

Also Christian's "trailer" series may want to learn the same trick we did to builtin/commit.c in this series, if it does not already know about possible trailing comment and blank lines.

Interesting ;-)

I do not think it is too hard to teach strbuf_commented_addf() about the leading HT, but that would be a separate topic; if squashing the SP-HT to HT is worth doing for this codepath, doing it at that helper would benefit all callers.

Junio C Hamano <gitster@pobox.com> writes:

-- >8 --
Subject: [PATCH] strbuf_add_lines(): avoid SP-HT sequence

The strbuf_add_commented_lines() function passes a pair of prefixes, one for a line that has some meat on it, and the other for an empty line. The former is set to a comment char followed by a SP, while the latter is set to just the comment char. This is to give a SP after the comment character, e.g. "# <user text>\n" and to avoid emitting an unsightly "# \n" in its output.

Teach the machinery to also use the latter space-less prefix when the payload line begins with a tab; otherwise we will end up showing "# \t<user text>\n" which is similarly unsightly.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 strbuf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On Mon, Oct 27, 2014 at 01:59:18PM -0700, Junio C Hamano wrote:

Thanks for doing this. I agree that this is the right place to put the magic, and the patch looks obviously correct. I also double-checked it with the "# Conflicts" patches and it addresses the problem I saw.

-Peff

On Mon, Oct 27, 2014 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:

The trailer series already tries to ignore comments and blank lines. This is the relevant function:

/*
 * Return the (0 based) index of the first trailer line or count if
 * there are no trailers. Trailers are searched only in the lines from
 * index (count - 1) down to index 0.
 */
static int find_trailer_start(struct strbuf **lines, int count)
{
    int start, only_spaces = 1;

    /*
     * Get the start of the trailers by looking starting from the end
     * for a line with only spaces before lines with one separator.
     */
    for (start = count - 1; start >= 0; start--) {
        if (lines[start]->buf[0] == comment_line_char)
            continue;
        if (contains_only_spaces(lines[start]->buf)) {
            if (only_spaces)
                continue;
            return start + 1;
        }
        if (strcspn(lines[start]->buf, separators) < lines[start]->len) {
            if (only_spaces)
                only_spaces = 0;
            continue;
        }
        return count;
    }

    return only_spaces ? count : 0;
}

But I am not sure sure that it does all of what you do to builtin/commit.c in the above patch. I will have a look. Anyway I would be happy to use an existing function or to refactor some existing code into a shared function if possible.

Thanks, Christian.

Jeff King wrote:

If you have time to experiment with tr/remerge-diff from pu[1], that would be welcome.

Maybe some day it can be the default behavior for 'log -p'.

Yes.

Thanks, Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/256591

On Mon, Oct 27, 2014 at 02:14:42PM -0700, Jonathan Nieder wrote:

Thanks, that was the topic I was thinking of.

It's not very often that I want to carefully investigate merge commits (usually it is when I am trying to help somebody track the addition or deletion of content that came as part of an evil merge), but I'll give it a try next time it comes up.

-Peff