git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v8 4/8] notes: allow use of the "rewrite" terminology for merge strategies

From
Jacob Keller <jacob.keller@gmail.com>
Date
Aug 17, 2015, 18:28 UTC
Message-ID
<CA+P7+xq5qTPVsSq0JOSUm5o9w=szD1c2oFm0e3s_f97E0=Zh+A@mail.gmail.com>
In-Reply-To
<CALKQrgfLzWdRxC5saBXJ_-iKmVDfs+mBfDKKrSU2-tP7eO5+Zg@mail.gmail.com>
Hi,
On Mon, Aug 17, 2015 at 5:54 AM, Johan Herland <johan@herland.net> wrote:
Show 24 quoted lines
> On Mon, Aug 17, 2015 at 10:46 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> notes-merge.c already re-uses the same functions for the automatic merge
>> strategies used by the rewrite functionality. Teach the -s/--strategy
>> option how to interpret the equivalent rewrite terminology for
>> consistency.
>
> I'm somewhat negative to this patch. IMHO, adding the rewrite modes as
> merge strategy synonyms adds no benefit - only potential confusion -
> to the existing merge strategies. Words that have a sensible meaning
> in the context of rewrite, do not necessarily have the same sensible
> meaning in the context of merge (and vice versa). I'd rather have the
> rewrite code map ignore/overwrite/concatenate to ours/theirs/union,
> without teaching the notes-merge code about these words. Or maybe even
> drop this patch (and the next?) entirely, and let the future author
> (who implements notes rewrite in terms of notes merge) decide how to
> deal with this? By committing to these synonyms now, you might
> actually be making things harder for the future author: once the
> synonyms are part of the user-visible and documented interface, they
> cannot easily be removed/changed again.
>
> ...Johan
>

I am ok dropping these, I really only added them after Junio brought it up. I think that documenting union/concatenate is fine, but I think that ours/theirs is very confusing. However, I think you're right that a future author can deal with this when working on rewrite -> merge. I don't think we need to do this now.

I'll drop these patches, but I will leave the parse_notes_merge_strategy in it's location here.

We could document concatenate as a synonym of union but I don't think that is a big deal.

Regards, Jake

Previous: Johan HerlandNext: Junio C Hamano
Message 7 of 14 in “implement notes.mergeStrategy option”
  1. 0/8 implement notes.mergeStrategy optionJacob Keller, Aug 17, 2015
  2. 1/8 notes: document cat_sort_uniq rewriteModeJacob Keller, Aug 17, 2015
  3. 2/8 notes: extract enum notes_merge_strategy to notes-utils.hJacob Keller, Aug 17, 2015
  4. 3/8 note: extract parse_notes_merge_strategy to notes-utilsJacob Keller, Aug 17, 2015
  5. 4/8 notes: allow use of the "rewrite" terminology for merge strategiesJacob Keller, Aug 17, 2015
  6. Johan HerlandAug 17, 2015
  7. Jacob KellerAug 17, 2015
  8. Junio C HamanoAug 17, 2015
  9. 5/8 notes: implement parse_combine_rewrite_fn using parse_notes_merge_strategyJacob Keller, Aug 17, 2015
  10. 6/8 notes: add tests for --commit/--abort/--strategy exclusivityJacob Keller, Aug 17, 2015
  11. 7/8 notes: add notes.mergeStrategy option to select default strategyJacob Keller, Aug 17, 2015
  12. 8/8 notes: teach git-notes about notes.<ref>.mergeStrategy optionJacob Keller, Aug 17, 2015
  13. Johan HerlandAug 17, 2015
  14. Jacob KellerAug 17, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.