Patchwork histedit: remove history_source

login
register
mail settings
Submitter Boris Penev
Date July 30, 2014, 1:53 a.m.
Message ID <4076ff9a7bb016e779da.1406685195@localhost.localdomain>
Download mbox | patch
Permalink /patch/5209/
State Rejected
Headers show

Comments

Boris Penev - July 30, 2014, 1:53 a.m.
# HG changeset patch
# User Boris Penev <bob53181@yahoo.com>
# Date 1406685063 -3600
#      Wed Jul 30 02:51:03 2014 +0100
# Branch stable
# Node ID 4076ff9a7bb016e779da879c77b2de2e0e79a082
# Parent  ad56fc55cbc3870d257e163469c687088627283b
histedit: remove history_source

History source is polluting the commit messages. This is
visible when converting the repository to git but the fact that
it stays in the history is bad enough by itself. This patch
removes the needless logging. I do not know why you would need
this but it possibly could be non-default, activated by an
argument to the histedit command.
Augie Fackler - July 30, 2014, 6:15 p.m.
On Tue, Jul 29, 2014 at 9:53 PM, Boris Penev <bob53181@yahoo.com> wrote:
> # HG changeset patch
> # User Boris Penev <bob53181@yahoo.com>
> # Date 1406685063 -3600
> #      Wed Jul 30 02:51:03 2014 +0100
> # Branch stable
> # Node ID 4076ff9a7bb016e779da879c77b2de2e0e79a082
> # Parent  ad56fc55cbc3870d257e163469c687088627283b
> histedit: remove history_source

No thanks.

> History source is polluting the commit messages. This is
> visible when converting the repository to git but the fact that
> it stays in the history is bad enough by itself.

Ignoring hg-git, why do you care about some invisible metadata in your history?

> This patch
> removes the needless logging. I do not know why you would need
> this but it possibly could be non-default, activated by an
> argument to the histedit command.
>
> diff -r ad56fc55cbc3 -r 4076ff9a7bb0 hgext/histedit.py
> --- a/hgext/histedit.py Mon Jul 28 10:05:17 2014 +0200
> +++ b/hgext/histedit.py Wed Jul 30 02:51:03 2014 +0100
> @@ -188,8 +188,6 @@
>
>      This function ensure we apply the same treatment to all changesets.
>
> -    - Add a 'histedit_source' entry in extra.
> -
>      Note that fold have its own separated logic because its handling is a bit
>      different and not easily factored out of the fold method.
>      """
> @@ -199,9 +197,6 @@
>          try:
>              repo.ui.setconfig('phases', 'new-commit', phasemin,
>                                'histedit')
> -            extra = kwargs.get('extra', {}).copy()
> -            extra['histedit_source'] = src.hex()
> -            kwargs['extra'] = extra
>              return repo.commit(**kwargs)
>          finally:
>              repo.ui.restoreconfig(phasebackup)
> @@ -289,7 +284,6 @@
>          message = first.description()
>      user = commitopts.get('user')
>      date = commitopts.get('date')
> -    extra = commitopts.get('extra')
>
>      parents = (first.p1().node(), first.p2().node())
>      new = context.memctx(repo,
> @@ -299,7 +293,6 @@
>                           filectxfn=filectxfn,
>                           user=user,
>                           date=date,
> -                         extra=extra,
>                           editor=cmdutil.getcommiteditor(edit=True))
>      return repo.commitctx(new)
>
> @@ -316,7 +309,7 @@
>      # drop the second merge parent
>      commit = commitfuncfor(repo, oldctx)
>      n = commit(text=oldctx.description(), user=oldctx.user(),
> -               date=oldctx.date(), extra=oldctx.extra())
> +               date=oldctx.date())
>      if n is None:
>          ui.warn(_('%s: empty changeset\n')
>                       % node.hex(ha))
> @@ -341,7 +334,7 @@
>          raise error.InterventionRequired(
>              _('Fix up the change and run hg histedit --continue'))
>      n = repo.commit(text='fold-temp-revision %s' % ha, user=oldctx.user(),
> -                    date=oldctx.date(), extra=oldctx.extra())
> +                    date=oldctx.date())
>      if n is None:
>          ui.warn(_('%s: empty changeset')
>                       % node.hex(ha))
> @@ -367,12 +360,6 @@
>      commitopts['message'] = newmessage
>      # date
>      commitopts['date'] = max(ctx.date(), oldctx.date())
> -    extra = ctx.extra().copy()
> -    # histedit_source
> -    # note: ctx is likely a temporary commit but that the best we can do here
> -    #       This is sufficient to solve issue3681 anyway
> -    extra['histedit_source'] = '%s,%s' % (ctx.hex(), oldctx.hex())
> -    commitopts['extra'] = extra
>      phasebackup = repo.ui.backupconfig('phases', 'new-commit')
>      try:
>          phasemin = max(ctx.phase(), oldctx.phase())
> @@ -405,7 +392,6 @@
>      message = oldctx.description()
>      commit = commitfuncfor(repo, oldctx)
>      new = commit(text=message, user=oldctx.user(), date=oldctx.date(),
> -                 extra=oldctx.extra(),
>                   editor=cmdutil.getcommiteditor(edit=True))
>      newctx = repo[new]
>      if oldctx.node() != newctx.node():
> @@ -686,7 +672,7 @@
>          editor = cmdutil.getcommiteditor(edit=editopt)
>          commit = commitfuncfor(repo, ctx)
>          new = commit(text=message, user=ctx.user(),
> -                     date=ctx.date(), extra=ctx.extra(),
> +                     date=ctx.date(),
>                       editor=editor)
>          if new is not None:
>              newchildren.append(new)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - July 30, 2014, 6:21 p.m.
On 07/29/2014 06:53 PM, Boris Penev wrote:
> # HG changeset patch
> # User Boris Penev <bob53181@yahoo.com>
> # Date 1406685063 -3600
> #      Wed Jul 30 02:51:03 2014 +0100
> # Branch stable
> # Node ID 4076ff9a7bb016e779da879c77b2de2e0e79a082
> # Parent  ad56fc55cbc3870d257e163469c687088627283b
> histedit: remove history_source
>
> History source is polluting the commit messages. This is
> visible when converting the repository to git but the fact that
> it stays in the history is bad enough by itself. This patch
> removes the needless logging. I do not know why you would need
> this but it possibly could be non-default, activated by an
> argument to the histedit command.

This change is actually useful for at least two things:

1) keeping relevant meta data when using --keep
2) Ensuring the a pure happen only history creation. A property hihgly 
critical to any distributed system

Doesn't git have a way to quietly store arbitraty metadata?
Pierre-Yves David - July 30, 2014, 6:51 p.m.
On 07/30/2014 11:21 AM, Pierre-Yves David wrote:

> 2) Ensuring the a pure happen only history creation. A property hihgly
> critical to any distributed system

"pure happen only" should have been "pure append only"
Siddharth Agarwal - July 30, 2014, 6:54 p.m.
On 07/30/2014 11:21 AM, Pierre-Yves David wrote:
>
> This change is actually useful for at least two things:
>
> 1) keeping relevant meta data when using --keep
> 2) Ensuring the a pure happen only history creation. A property hihgly 
> critical to any distributed system
>
> Doesn't git have a way to quietly store arbitraty metadata?

There are Git notes, but they're not an input to the commit hash so 
storing hg metadata in there would mean hg-git loses its 1:1 mapping 
between Mercurial and Git revisions. Other than that I'm not aware of 
anything.
Augie Fackler - July 31, 2014, 12:03 p.m.
On Jul 30, 2014, at 2:54 PM, Siddharth Agarwal <sid@less-broken.com> wrote:

> On 07/30/2014 11:21 AM, Pierre-Yves David wrote:
>> 
>> This change is actually useful for at least two things:
>> 
>> 1) keeping relevant meta data when using --keep
>> 2) Ensuring the a pure happen only history creation. A property hihgly critical to any distributed system
>> 
>> Doesn't git have a way to quietly store arbitraty metadata?
> 
> There are Git notes, but they're not an input to the commit hash so storing hg metadata in there would mean hg-git loses its 1:1 mapping between Mercurial and Git revisions. Other than that I'm not aware of anything.

I did just notice that a noop histedit seemed to rewrite shas - that's definitely a bug that we should fix.

(I was in the middle of a big project, so I didn't pursue it when I saw it.)
Boris Penev - Aug. 20, 2014, 10:28 p.m.
On Thu Jul 31 07:03:03 CDT 2014, Augie Fackler wrote:
> I did just notice that a noop histedit seemed to rewrite shas -
> that's definitely a bug that we should fix.
>
> (I was in the middle of a big project, so I didn't pursue it when I
> saw it.)

Does it mean that the proposed behavior will be added to histedit 
(either as primary/default or secondary)? I expected a more concrete yes 
or no for the time passed.
Pierre-Yves David - Aug. 20, 2014, 10:45 p.m.
On 08/20/2014 03:28 PM, Boris Penev wrote:
> On Thu Jul 31 07:03:03 CDT 2014, Augie Fackler wrote:
>> I did just notice that a noop histedit seemed to rewrite shas -
>> that's definitely a bug that we should fix.
>>
>> (I was in the middle of a big project, so I didn't pursue it when I
>> saw it.)
>
> Does it mean that the proposed behavior will be added to histedit
> (either as primary/default or secondary)? I expected a more concrete yes
> or no for the time passed.

Can you be more precise about what you mean by "the proposed behavior" ?
Boris Penev - Aug. 21, 2014, 11:09 a.m.
On 08/21/2014 01:45 AM, Pierre-Yves David wrote:
> On 08/20/2014 03:28 PM, Boris Penev wrote:
>> Does it mean that the proposed behavior will be added to histedit
>> (either as primary/default or secondary)? I expected a more
>> concrete yes or no for the time passed.
>
> Can you be more precise about what you mean by "the proposed
> behavior" ?

I mean to not keep irrelevant metadata in the commit messages.
Maybe triggered by --keep=false or something like that.
Often you really do not need to keep such information in your repository
and it is particularly annoying when keeping a git mirror (all affected
commits show histedit_source information in git log).
Jordi GutiƩrrez Hermoso - Aug. 21, 2014, 2:08 p.m.
On Thu, 2014-08-21 at 14:09 +0300, Boris Penev wrote:
> On 08/21/2014 01:45 AM, Pierre-Yves David wrote:
> > On 08/20/2014 03:28 PM, Boris Penev wrote:
> >> Does it mean that the proposed behavior will be added to histedit
> >> (either as primary/default or secondary)? I expected a more
> >> concrete yes or no for the time passed.
> >
> > Can you be more precise about what you mean by "the proposed
> > behavior" ?
> 
> I mean to not keep irrelevant metadata in the commit messages.

It's not irrelevant, and they're not in the commit messages (at least,
not in hg). They are useful for when you want to restore history.
Augie didn't decide to add these fields to the histedited commit just
to annoy you. They're sort like a precambrian obsolescence marker like
what you see with hg evolve.

I think the real problem is that when you push with hg-git to a git
repo, hg-git will translate extra: fields into lines of the git commit
message. I don't like this either, and *in this context* it is
irrelevant (git users typically don't care about hg's hashes).

In that case, I think your code-killing wrath should be better
directed to these lines,:

    https://bitbucket.org/durin42/hg-git/src/83e1c73136251271d8acbf92e05100ea175f904b/hggit/git_handler.py?at=default#cl-599

Also, those lines have a purpose too: to preserve all of the data in
the round-trip of the git<->hg conversion. I think the best thing to
do is to add an option to hg-git to not send that data to git for when
you don't care about that round trip, or to make hg-git add that data
in a location where the git UI won't show it to most git users. Kiln
Harmony does this by storing it in *git*'s extra field:

    http://blog.fogcreek.com/kiln-harmony-internals-the-basics/

- Jordi G. H.

Patch

diff -r ad56fc55cbc3 -r 4076ff9a7bb0 hgext/histedit.py
--- a/hgext/histedit.py	Mon Jul 28 10:05:17 2014 +0200
+++ b/hgext/histedit.py	Wed Jul 30 02:51:03 2014 +0100
@@ -188,8 +188,6 @@ 
 
     This function ensure we apply the same treatment to all changesets.
 
-    - Add a 'histedit_source' entry in extra.
-
     Note that fold have its own separated logic because its handling is a bit
     different and not easily factored out of the fold method.
     """
@@ -199,9 +197,6 @@ 
         try:
             repo.ui.setconfig('phases', 'new-commit', phasemin,
                               'histedit')
-            extra = kwargs.get('extra', {}).copy()
-            extra['histedit_source'] = src.hex()
-            kwargs['extra'] = extra
             return repo.commit(**kwargs)
         finally:
             repo.ui.restoreconfig(phasebackup)
@@ -289,7 +284,6 @@ 
         message = first.description()
     user = commitopts.get('user')
     date = commitopts.get('date')
-    extra = commitopts.get('extra')
 
     parents = (first.p1().node(), first.p2().node())
     new = context.memctx(repo,
@@ -299,7 +293,6 @@ 
                          filectxfn=filectxfn,
                          user=user,
                          date=date,
-                         extra=extra,
                          editor=cmdutil.getcommiteditor(edit=True))
     return repo.commitctx(new)
 
@@ -316,7 +309,7 @@ 
     # drop the second merge parent
     commit = commitfuncfor(repo, oldctx)
     n = commit(text=oldctx.description(), user=oldctx.user(),
-               date=oldctx.date(), extra=oldctx.extra())
+               date=oldctx.date())
     if n is None:
         ui.warn(_('%s: empty changeset\n')
                      % node.hex(ha))
@@ -341,7 +334,7 @@ 
         raise error.InterventionRequired(
             _('Fix up the change and run hg histedit --continue'))
     n = repo.commit(text='fold-temp-revision %s' % ha, user=oldctx.user(),
-                    date=oldctx.date(), extra=oldctx.extra())
+                    date=oldctx.date())
     if n is None:
         ui.warn(_('%s: empty changeset')
                      % node.hex(ha))
@@ -367,12 +360,6 @@ 
     commitopts['message'] = newmessage
     # date
     commitopts['date'] = max(ctx.date(), oldctx.date())
-    extra = ctx.extra().copy()
-    # histedit_source
-    # note: ctx is likely a temporary commit but that the best we can do here
-    #       This is sufficient to solve issue3681 anyway
-    extra['histedit_source'] = '%s,%s' % (ctx.hex(), oldctx.hex())
-    commitopts['extra'] = extra
     phasebackup = repo.ui.backupconfig('phases', 'new-commit')
     try:
         phasemin = max(ctx.phase(), oldctx.phase())
@@ -405,7 +392,6 @@ 
     message = oldctx.description()
     commit = commitfuncfor(repo, oldctx)
     new = commit(text=message, user=oldctx.user(), date=oldctx.date(),
-                 extra=oldctx.extra(),
                  editor=cmdutil.getcommiteditor(edit=True))
     newctx = repo[new]
     if oldctx.node() != newctx.node():
@@ -686,7 +672,7 @@ 
         editor = cmdutil.getcommiteditor(edit=editopt)
         commit = commitfuncfor(repo, ctx)
         new = commit(text=message, user=ctx.user(),
-                     date=ctx.date(), extra=ctx.extra(),
+                     date=ctx.date(),
                      editor=editor)
         if new is not None:
             newchildren.append(new)