Re: [PATCH v7 1/4] notes: document cat_sort_uniq rewriteMode

On Sat, Aug 15, 2015 at 12:53 AM, Jacob Keller [off-list ref] wrote:

On Fri, Aug 14, 2015 at 3:11 PM, Junio C Hamano [off-list ref] wrote:

quoted

Jacob Keller [off-list ref] writes:

quoted

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 75ec02e8e90a..de67ad1fdedf 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -1947,8 +1947,8 @@ notes.rewriteMode::
      When copying notes during a rewrite (see the
      "notes.rewrite.<command>" option), determines what to do if
      the target commit already has a note.  Must be one of
-     `overwrite`, `concatenate`, or `ignore`.  Defaults to
-     `concatenate`.
+     `overwrite`, `concatenate`, `cat_sort_uniq`, or `ignore`.
+     Defaults to `concatenate`.
 +
 This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`
 environment variable.

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 851518d531b5..674682b34b83 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt

@@ -331,7 +331,8 @@ environment variable.
 notes.rewriteMode::
      When copying notes during a rewrite, what to do if the target
      commit already has a note.  Must be one of `overwrite`,
-     `concatenate`, and `ignore`.  Defaults to `concatenate`.
+     `concatenate`, `cat_sort_uniq`, or `ignore`.  Defaults to
+     `concatenate`.
 +
 This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`
 environment variable.

This obviously is not a problem introduced by this patch, but I
wonder why we have two similar but different set of modes for
rewrtie and merge.

We do. Rewrite builds directly on top of the combine_notes_* functions
that are part of the core/low-level notes code, added in 73f464b5
(2010-02-13, Refactor notes concatenation into a flexible interface for
combining notes). Notes merge (and its various strategies) were added
later, and also build on top of these combine_notes_* functions (see
merge_one_change() in notes-merge.c). In addition, notes-merge adds the
'manual' strategy, which depends on surrounding machinery that
notes-merge provides, but that the low-level combine_notes_* functions
cannot depend on.

quoted

 Isn't 'overwrite' like 'ours', 'ignore' like
'theirs', and 'concat' like 'union', and if these are similar
enough, perhaps it would be helpful to the end user if we unified
the terms (or accepted both as synonyms for backward compatibility)?

The mapping (which is contained in merge_one_change() in notes-merge.c)
is:

  'manual'        -> [custom handling in notes-merge]
  'ours'          -> combine_notes_ignore [or simply a no-op]
  'theirs'        -> combine_notes_overwrite
  'union'         -> combine_notes_concatenate
  'cat_sort_uniq' -> combine_notes_cat_sort_uniq

quoted

Also I notice that you cannot manually reconcile while rewriting;
don't we want to have 'manual' there, too, I wonder?

No, as stated above, 'manual' requires some external mechanism for
creating a "worktree" where the notes conflicts can be checked out
and manipulated by the user. The guts of notes.c is not the correct
place to do that. I don't know if it's easy to rewrite "rewrite" to
do a (partial) notes merge instead of using the combine_notes_*
functions directly, but I imagine that would be the best way forward.

quoted

[jc: Cc'ed Thomas who invented rewrite back when merge was not even
there, and Johan who added merge]

I was not sure. I believe that re-write doesn't do the same thing as
merge? I think we could make all of them handle the "overwrite", which
is basically a synonym of, I think "theirs" depending on the direction
of the "merge".

Correct.

I don't know if re-write actually supports manual mode at all!

It doesn't.

Maybe we could make merge support the other names as synonyms, and
then code re-write in terms of merging?

Adding the synonyms is fine, but I wouldn't publicize it heavily, as
the meaning of words like "overwrite" and "ignore" can become quite
confusing in the context of a merge.

Reimplementing re-write in terms of merge is probably a good idea,
though.

I wasn't sure so I chose only to document the mode that was missing.

IMHO, that's good for now.


...Johan


-- 
Johan Herland, [off-list ref]
www.herland.net