[PATCH 1 of 2] commit: return 1 for interactive commit with no changes (issue5397)

[Augie Fackler]

On Thu, Oct 13, 2016 at 04:13:43PM +0200, Pierre-Yves David wrote:
>
>
> On 10/13/2016 03:43 PM, Philippe Pepiot wrote:
> >
> >
> >On 10/13/2016 03:22 PM, Pierre-Yves David wrote:
> >>
> >>
> >>On 10/13/2016 02:32 PM, Philippe Pepiot wrote:
> >>># HG changeset patch
> >>># User Philippe Pepiot <philippe.pepiot at logilab.fr>
> >>># Date 1476266974 -7200
> >>>#      Wed Oct 12 12:09:34 2016 +0200
> >>># Node ID fa75185b8901e58d4b1117985e9f3f20e89c4e01
> >>># Parent  b85fa6bf298be07804a74d8fdec0d19fdbc6d740
> >>># EXP-Topic record-return-code
> >>>commit: return 1 for interactive commit with no changes (issue5397)
> >>>
> >>>For consistency with non interactive commit
> >>>
> >>>diff --git a/mercurial/commands.py b/mercurial/commands.py
> >>>--- a/mercurial/commands.py
> >>>+++ b/mercurial/commands.py
> >>>@@ -1648,9 +1648,9 @@ def commit(ui, repo, *pats, **opts):
> >>> def _docommit(ui, repo, *pats, **opts):
> >>>     if opts.get('interactive'):
> >>>         opts.pop('interactive')
> >>>-        cmdutil.dorecord(ui, repo, commit, None, False,
> >>>-                        cmdutil.recordfilter, *pats, **opts)
> >>>-        return
> >>>+        ret = cmdutil.dorecord(ui, repo, commit, None, False,
> >>>+                               cmdutil.recordfilter, *pats, **opts)
> >>>+        return 1 if ret == 0 else ret
> >>
> >>I'm confused about this return value. "if ret == 0" we return 1; if
> >>return is not zero, we return the value. Do we never return 0 or am I
> >>missing something ?
> >
> >dorecord() return either 0 if there is no changes to record or the
> >return of commitfunc passed in arguments. Here commitfunc = commit that
> >return 1 or None, here is the trick.
> >
> >I agree this is confusing (see also how shelve use dorecord), maybe we
> >could raise a custom exception instead of returning 0 in dorecord to
> >avoid collisions with commitfunc return value, but I'm not sure how this
> >could affect 3rd party extensions ?
>
> Hum, the smaller improvement I can see is to add this explanation as an
> inline comment. I think we should also clean up the dorecord return to
> something better, but we are probably too late in the cycle for that.

FWIW, I'm fine with this patch as-is, and a comment added as a
followup since it's a little wonky.

It's definitely too late in the cycle to go monkeying with dorecord's interface.

>
> Cheers,
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel