git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* Q: What happened to "--no-commit" merges?
@ 2019-01-22 20:55 Ulrich Windl
  2019-01-22 21:29 ` Elijah Newren
  0 siblings, 1 reply; 11+ messages in thread
From: Ulrich Windl @ 2019-01-22 20:55 UTC (permalink / raw)
  To: git

Hi!

Using git version 2.16.4 on OpenSUSE Leap 15.0, it seems that "--no-commit" no
longer does what it did before (AFAIR, but I mostly did --no-ff merges in
SLES11):
Like this (sorry German):

> git merge --no-commit local/f-linux-firefox 
Aktualisiere 520aaae..c11e3da
Fast-forward
 bin/fval.xsl | 133 +++++++++++++++++++++++++----------------------------------
 1 file changed, 57 insertions(+), 76 deletions(-)

> git status
Auf Branch f-linux-firefox
nichts zu committen, Arbeitsverzeichnis unverändert

### "nothing to commit"
git log indicates the changes were committed already

Reading
https://stackoverflow.com/questions/8640887/git-merge-without-auto-commit it
seems that without "--no-ff" this ioption is effectively ignored.
If so, I suggest to tell the user that --no-commit is useless in this case, and
let him confirm that he/she wants the changes (merge) to be committed (despite
of --no-commit).

Regards,
Ulrich



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Q: What happened to "--no-commit" merges?
  2019-01-22 20:55 Q: What happened to "--no-commit" merges? Ulrich Windl
@ 2019-01-22 21:29 ` Elijah Newren
  2019-01-23  7:04   ` Antw: " Ulrich Windl
  0 siblings, 1 reply; 11+ messages in thread
From: Elijah Newren @ 2019-01-22 21:29 UTC (permalink / raw)
  To: Ulrich Windl; +Cc: Git Mailing List

Hello,

On Tue, Jan 22, 2019 at 1:05 PM Ulrich Windl
<Ulrich.Windl@rz.uni-regensburg.de> wrote:
>
> Hi!
>
> Using git version 2.16.4 on OpenSUSE Leap 15.0, it seems that "--no-commit" no
> longer does what it did before (AFAIR, but I mostly did --no-ff merges in
> SLES11):
> Like this (sorry German):
>
> > git merge --no-commit local/f-linux-firefox
> Aktualisiere 520aaae..c11e3da
> Fast-forward

Ah, a fast foward, so there was nothing to commit; it could simply
update the branch to include commits that already existed.

>  bin/fval.xsl | 133 +++++++++++++++++++++++++----------------------------------
>  1 file changed, 57 insertions(+), 76 deletions(-)
>
> > git status
> Auf Branch f-linux-firefox
> nichts zu committen, Arbeitsverzeichnis unverändert
>
> ### "nothing to commit"
> git log indicates the changes were committed already

Indeed; the changes were committed before you ran "git merge"; they
were all part of the local/f-linux-firefox branch.

> Reading
> https://stackoverflow.com/questions/8640887/git-merge-without-auto-commit it
> seems that without "--no-ff" this ioption is effectively ignored.
> If so, I suggest to tell the user that --no-commit is useless in this case, and
> let him confirm that he/she wants the changes (merge) to be committed (despite
> of --no-commit).

--no-commit, to me, means don't create any new commits.  But you had a
case where there was no need to create a any new commits: your branch
(f-linux-firefox, I think?) had no commits that the other branch
(local/f-linux-firefox) lacked, but the other branch had at least one
you lacked.  So, merging could be done by just moving your branch
pointer to include all those existing commits.

If you want the branch to not get updated, then yes you'd need both
--no-ff and --no-commit in some cases.  But that's always been true.
It's possible in the past that you just didn't run into those cases.

Now, if you're suggesting that --no-commit should imply --no-ff,
that's interesting.  However, you are fundamentally changing the
operation at that point by making it so that a merge commit will be
created when the user runs `git commit` at the end -- it's not clear
to me that users will see a merge commit as wanted or needed and
having --no-commit imply that option might break expectations.  I'd be
more inclined to tell users who want --no-ff behavor to use that flag
and/or set the merge.ff config setting to false.

Alternatively, we could update the documentation to point out this
special case under --no-commit to point out that when an ff-update
occurs no commit creation is involved and thus --no-commit has no
effect.  Would that help?


Elijah

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Antw: Re: Q: What happened to "--no-commit" merges?
  2019-01-22 21:29 ` Elijah Newren
@ 2019-01-23  7:04   ` Ulrich Windl
  2019-02-18 18:41     ` Elijah Newren
  0 siblings, 1 reply; 11+ messages in thread
From: Ulrich Windl @ 2019-01-23  7:04 UTC (permalink / raw)
  To: Elijah Newren; +Cc: git

>>> Elijah Newren <newren@gmail.com> schrieb am 22.01.2019 um 22:29 in
Nachricht
<CABPp-BFGfWPAwKLMMMLdLu856UvrrSMYjYWXeVUxEqpspBxbsA@mail.gmail.com>:
> Hello,
> 
> On Tue, Jan 22, 2019 at 1:05 PM Ulrich Windl
> <Ulrich.Windl@rz.uni-regensburg.de> wrote:
>>
>> Hi!
>>
>> Using git version 2.16.4 on OpenSUSE Leap 15.0, it seems that "--no-commit"

> no
>> longer does what it did before (AFAIR, but I mostly did --no-ff merges in
>> SLES11):
>> Like this (sorry German):
>>
>> > git merge --no-commit local/f-linux-firefox
>> Aktualisiere 520aaae..c11e3da
>> Fast-forward
> 
> Ah, a fast foward, so there was nothing to commit; it could simply
> update the branch to include commits that already existed.
> 
>>  bin/fval.xsl | 133 
> +++++++++++++++++++++++++----------------------------------
>>  1 file changed, 57 insertions(+), 76 deletions(-)
>>
>> > git status
>> Auf Branch f-linux-firefox
>> nichts zu committen, Arbeitsverzeichnis unverändert
>>
>> ### "nothing to commit"
>> git log indicates the changes were committed already
> 
> Indeed; the changes were committed before you ran "git merge"; they
> were all part of the local/f-linux-firefox branch.

Actually no: The changes were on a different local "remote" branch; otherwise
I wouldn't need the merge, I guess

> 
>> Reading
>> https://stackoverflow.com/questions/8640887/git-merge-without-auto-commit
it
>> seems that without "--no-ff" this ioption is effectively ignored.

Note: If you see the number of upvotes to the answer there, it seems I'm not
the only one who got confused. ;-)

>> If so, I suggest to tell the user that --no-commit is useless in this case,

> and
>> let him confirm that he/she wants the changes (merge) to be committed 
> (despite
>> of --no-commit).
> 
> --no-commit, to me, means don't create any new commits.  But you had a
> case where there was no need to create a any new commits: your branch
> (f-linux-firefox, I think?) had no commits that the other branch
> (local/f-linux-firefox) lacked, but the other branch had at least one
> you lacked.  So, merging could be done by just moving your branch
> pointer to include all those existing commits.

Is moving commits from one branch to to another done without any new commit?
Just updating the refs, or what? I didn't know that.

> 
> If you want the branch to not get updated, then yes you'd need both
> --no-ff and --no-commit in some cases.  But that's always been true.
> It's possible in the past that you just didn't run into those cases.

So it seems a commit is something other than I'd expected: To me anything that
changes what "git log" outputs is a commit ;-) Or anything that chenges the
reflog...

> 
> Now, if you're suggesting that --no-commit should imply --no-ff,
> that's interesting.  However, you are fundamentally changing the
> operation at that point by making it so that a merge commit will be
> created when the user runs `git commit` at the end -- it's not clear
> to me that users will see a merge commit as wanted or needed and
> having --no-commit imply that option might break expectations.  I'd be
> more inclined to tell users who want --no-ff behavor to use that flag
> and/or set the merge.ff config setting to false.

No, it seems I didn't realize that a fast-forward actually is without a
commit.


> 
> Alternatively, we could update the documentation to point out this
> special case under --no-commit to point out that when an ff-update
> occurs no commit creation is involved and thus --no-commit has no
> effect.  Would that help?

Maybe (I'm unsure where the concepts are described best to check the current
version(s)) try to explain the concepts of "commit" and "fast forward" in some
greater detail. Maybe I was just expecting the wrong things to happen behind
the scenes. Maybe add a statement like "fast-forwards never create a new
commit, so --no-commit doesn't make sense when fast-forwarding."

Thanks for the explanations.

Regards,
Ulrich



^ permalink raw reply	[flat|nested] 11+ messages in thread

* (no subject)
  2019-01-23  7:04   ` Antw: " Ulrich Windl
@ 2019-02-18 18:41     ` Elijah Newren
       [not found]       ` <0A3130DD0200005B824A10E1@gwsmtp1.uni-regensburg.de>
  0 siblings, 1 reply; 11+ messages in thread
From: Elijah Newren @ 2019-02-18 18:41 UTC (permalink / raw)
  To: gitster; +Cc: git, Ulrich Windl, Elijah Newren

Hi Ulrich,

Sorry for the late reply...

On Tue, Jan 22, 2019 at 11:04 PM Ulrich Windl <Ulrich.Windl@rz.uni-regensburg.de> wrote:
> >>> Elijah Newren <newren@gmail.com> schrieb am 22.01.2019 um 22:29 in
> Nachricht
> > On Tue, Jan 22, 2019 at 1:05 PM Ulrich Windl
> > <Ulrich.Windl@rz.uni-regensburg.de> wrote:
> >>
> >> Using git version 2.16.4 on OpenSUSE Leap 15.0, it seems that "--no-commit"
> >> no
> >> longer does what it did before (AFAIR, but I mostly did --no-ff merges in
> >> SLES11):
> >>
> >> > git merge --no-commit local/f-linux-firefox
> >> Aktualisiere 520aaae..c11e3da
> >> Fast-forward
....
> > Indeed; the changes were committed before you ran "git merge"; they
> > were all part of the local/f-linux-firefox branch.
>
> Actually no: The changes were on a different local "remote" branch; otherwise
> I wouldn't need the merge, I guess

Yes, they were on a different branch, but since the commits already exist they don't need to be created again.  The current branch can just be updated from the current commit to the commit on the end of the other branch.  No merge is needed because the histories of the two branches have not diverged, so the merge short-circuits itself (doing what is sometimes called a "fast-forward merge" instead and avoiding almost all the normal merge logic).

> >> Reading
> >> https://stackoverflow.com/questions/8640887/git-merge-without-auto-commit
> it
> >> seems that without "--no-ff" this ioption is effectively ignored.
>
> Note: If you see the number of upvotes to the answer there, it seems I'm not
> the only one who got confused. ;-)

Indeed, and looking at that stackoverflow post, it makes it clear that the manpage is actually misleading.  I'll post a patch to correct it.

> Is moving commits from one branch to to another done without any new commit?
> Just updating the refs, or what? I didn't know that.

If the two branches have not diverged at all, and only one side has some commits that the other doesn't, then indeed there is no need for creating any new commits.  If the histories have diverged, then you need to create a merge commit and have a real merge.

> > If you want the branch to not get updated, then yes you'd need both
> > --no-ff and --no-commit in some cases.  But that's always been true.
> > It's possible in the past that you just didn't run into those cases.
>
> So it seems a commit is something other than I'd expected: To me anything that
> changes what "git log" outputs is a commit ;-) Or anything that chenges the
> reflog...

A commit is an object that records its parents (most commits have exactly one parent), its author, its committer, a commit message, and the tree involved.  Creating a new commit is a common reason to change the reflog and would cause git log to have more output for subsequent invocations.  Most merges will of necessity create a merge commit, to reflect that diverging histories have been merged (and such a commit will have more than one parent, one for each branch being merged).  But, as noted above, if histories haven't diverged then we don't need a new multi-parent commit; we can just short-circuit the merge logic and use the existing commits on the other branch.

> > Alternatively, we could update the documentation to point out this
> > special case under --no-commit to point out that when an ff-update
> > occurs no commit creation is involved and thus --no-commit has no
> > effect.  Would that help?
>
> Maybe (I'm unsure where the concepts are described best to check the current
> version(s)) try to explain the concepts of "commit" and "fast forward" in some
> greater detail. Maybe I was just expecting the wrong things to happen behind
> the scenes. Maybe add a statement like "fast-forwards never create a new
> commit, so --no-commit doesn't make sense when fast-forwarding."
>
> Thanks for the explanations.

Hopefully the patch below answers what you originally needed to know and prevents others from running into similar problems.

Thanks,
Elijah

-- 8< --
Subject: [PATCH] merge-options.txt: correct wording of --no-commit option

The former wording implied that --no-commit would always cause the
merge operation to abort and allow the user to make further changes
and/or provide a special commit message for the merge commit.  This
is not the case for fast-forward merges, as there is no merge commit
to create.  Without a merge commit, there is no place where it makes
sense to "stop the merge and allow the user to tweak changes"; doing
that would require a full rebase of some sort.

Modify the wording to correctly address fast-forward cases as well,
and suggest using --no-ff with --no-commit if the point is to ensure
that the merge aborts.

Reported-by: Ulrich Windl <Ulrich.Windl@rz.uni-regensburg.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/merge-options.txt | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index c2a263ba74..d1061b8cf7 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -3,9 +3,15 @@
 	Perform the merge and commit the result. This option can
 	be used to override --no-commit.
 +
-With --no-commit perform the merge but pretend the merge
-failed and do not autocommit, to give the user a chance to
-inspect and further tweak the merge result before committing.
+With --no-commit perform the merge and stop just before creating
+a merge commit, to give the user a chance to inspect and further
+tweak the merge result before committing.
++
+Note that fast-forward updates do not need to create a merge
+commit and therefore there is no way to stop those merges with
+--no-commit.  Thus, if you want to ensure your branch is not
+changed or updated by the merge command, use --no-ff with
+--no-commit.
 
 --edit::
 -e::
-- 
2.21.0.rc1.264.g6c9e06a32d


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Antw: Antw:
       [not found]       ` <0A3130DD0200005B824A10E1@gwsmtp1.uni-regensburg.de>
@ 2019-02-19  7:03         ` Ulrich Windl
  2019-02-19 17:07           ` [PATCH v2] merge-options.txt: correct wording of --no-commit option Elijah Newren
  0 siblings, 1 reply; 11+ messages in thread
From: Ulrich Windl @ 2019-02-19  7:03 UTC (permalink / raw)
  To: Elijah Newren, gitster; +Cc: git

>>> Elijah Newren <newren@gmail.com> schrieb am 18.02.2019 um 19:41 in
Nachricht
<20190218184147.7563-1-newren@gmail.com>:
> Hi Ulrich,
> 
> Sorry for the late reply...

No problem, thanks for the explanation. I'll "fast forward" directly to the
patch below and comment inline: -- Ulrich

[...]
> ‑‑ 8< ‑‑
> Subject: [PATCH] merge‑options.txt: correct wording of ‑‑no‑commit option
> 
> The former wording implied that ‑‑no‑commit would always cause the
> merge operation to abort and allow the user to make further changes

I think "abort" is not the perfect word, because the merge would rather be
"paused for commit" in my understanding; a "git merge --abort" is different,
right?

> and/or provide a special commit message for the merge commit.  This
> is not the case for fast‑forward merges, as there is no merge commit
> to create.  Without a merge commit, there is no place where it makes
> sense to "stop the merge and allow the user to tweak changes"; doing
> that would require a full rebase of some sort.

...and before trying the merge it's not always obvious whether the merge will
be fast-forward type or not. So actually the outcome of --no-commit depends on
the conents being merged, not on the command line options.

> 
> Modify the wording to correctly address fast‑forward cases as well,
> and suggest using ‑‑no‑ff with ‑‑no‑commit if the point is to ensure
> that the merge aborts.
> 
> Reported‑by: Ulrich Windl <Ulrich.Windl@rz.uni‑regensburg.de>
> Signed‑off‑by: Elijah Newren <newren@gmail.com>
> ‑‑‑
>  Documentation/merge‑options.txt | 12 +++++++++‑‑‑
>  1 file changed, 9 insertions(+), 3 deletions(‑)
> 
> diff ‑‑git a/Documentation/merge‑options.txt
b/Documentation/merge‑options.txt
> index c2a263ba74..d1061b8cf7 100644
> ‑‑‑ a/Documentation/merge‑options.txt
> +++ b/Documentation/merge‑options.txt
> @@ ‑3,9 +3,15 @@
>  	Perform the merge and commit the result. This option can
>  	be used to override ‑‑no‑commit.
>  +
> ‑With ‑‑no‑commit perform the merge but pretend the merge
> ‑failed and do not autocommit, to give the user a chance to
> ‑inspect and further tweak the merge result before committing.
> +With ‑‑no‑commit perform the merge and stop just before creating
> +a merge commit, to give the user a chance to inspect and further
> +tweak the merge result before committing.
> ++
> +Note that fast‑forward updates do not need to create a merge
> +commit and therefore there is no way to stop those merges with
> +‑‑no‑commit.  Thus, if you want to ensure your branch is not
> +changed or updated by the merge command, use ‑‑no‑ff with
> +‑‑no‑commit.
>  
>  ‑‑edit::
>  ‑e::
> ‑‑ 
> 2.21.0.rc1.264.g6c9e06a32d




^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] merge-options.txt: correct wording of --no-commit option
  2019-02-19  7:03         ` Antw: Antw: Ulrich Windl
@ 2019-02-19 17:07           ` Elijah Newren
  2019-02-19 19:32             ` Junio C Hamano
  2019-02-21 17:50             ` [PATCH v3] " Elijah Newren
  0 siblings, 2 replies; 11+ messages in thread
From: Elijah Newren @ 2019-02-19 17:07 UTC (permalink / raw)
  To: gitster; +Cc: git, Ulrich Windl, Elijah Newren

The former wording implied that --no-commit would always cause the
merge operation to "pause" and allow the user to make further changes
and/or provide a special commit message for the merge commit.  This
is not the case for fast-forward merges, as there is no merge commit
to create.  Without a merge commit, there is no place where it makes
sense to "stop the merge and allow the user to tweak changes"; doing
that would require a full rebase of some sort.

Since users may be unaware of whether their branches have diverged or
not, modify the wording to correctly address fast-forward cases as well
and suggest using --no-ff with --no-commit if the point is to ensure
that the merge stops before completing.

Reported-by: Ulrich Windl <Ulrich.Windl@rz.uni-regensburg.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Changes since v1:
  - Tweaked commit message

 Documentation/merge-options.txt | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index c2a263ba74..d1061b8cf7 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -3,9 +3,15 @@
 	Perform the merge and commit the result. This option can
 	be used to override --no-commit.
 +
-With --no-commit perform the merge but pretend the merge
-failed and do not autocommit, to give the user a chance to
-inspect and further tweak the merge result before committing.
+With --no-commit perform the merge and stop just before creating
+a merge commit, to give the user a chance to inspect and further
+tweak the merge result before committing.
++
+Note that fast-forward updates do not need to create a merge
+commit and therefore there is no way to stop those merges with
+--no-commit.  Thus, if you want to ensure your branch is not
+changed or updated by the merge command, use --no-ff with
+--no-commit.
 
 --edit::
 -e::
-- 
2.21.0.rc1.264.g6c9e06a32d


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] merge-options.txt: correct wording of --no-commit option
  2019-02-19 17:07           ` [PATCH v2] merge-options.txt: correct wording of --no-commit option Elijah Newren
@ 2019-02-19 19:32             ` Junio C Hamano
  2019-02-19 22:31               ` Elijah Newren
       [not found]               ` <07106FB4020000CD824A10E1@gwsmtp1.uni-regensburg.de>
  2019-02-21 17:50             ` [PATCH v3] " Elijah Newren
  1 sibling, 2 replies; 11+ messages in thread
From: Junio C Hamano @ 2019-02-19 19:32 UTC (permalink / raw)
  To: Elijah Newren; +Cc: git, Ulrich Windl

Elijah Newren <newren@gmail.com> writes:

> +With --no-commit perform the merge and stop just before creating
> +a merge commit, to give the user a chance to inspect and further
> +tweak the merge result before committing.
> ++
> +Note that fast-forward updates do not need to create a merge
> +commit and therefore there is no way to stop those merges with
> +--no-commit.  Thus, if you want to ensure your branch is not
> +changed or updated by the merge command, use --no-ff with
> +--no-commit.

While the above is an improvement (so I'll queue it on 'pu' not to
lose sight of it), I find the use of "do not need to" above somewhat
misleading.  It solicits a reaction "ok, we know it does not need
to, but it could prepare to create one to allow us to further muck
with it, no?".

IOW, a fast-forward by definition does not create a merge by itself,
so there is nowhere to stop during a creation of a merge.  So at
least:

	s/do not need to/do not/

It also may be a good idea to consider detecting this case and be a
bit more helpful, perhaps with end-user experience looking like...

  $ git checkout master^0
  $ git merge --no-commit next
  Updating 0d0ac3826a..ee538a81fe
  Fast-forward
    ...diffstat follows here...
  hint: merge completed without creating a commit.
  hint: if you wanted to prepare for a manually tweaked merge,
  hint: do "git reset --keep ORIG_HEAD" followed by
  hint: "git merge --no-ff --no-commit next".

or even

  $ git checkout master^0
  $ git merge --no-commit next
  warning: defaulting to --no-ff, given a --no-commit request
  Automatic merge went well; stopped before committing as requested
  hint: if you'd rather have a fast-forward without creating a commit,
  hint: do "git reset --keep next" now.

I do not have a strong preference among three (the third option
being not doing anything), but if pressed, I'd say that the last one
might be the most user-friendly, even though it feels a bit too
magical and trying to be smarter than its own good.

In any case, the hint for the "recovery" procedure needs to be
carefully written.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] merge-options.txt: correct wording of --no-commit option
  2019-02-19 19:32             ` Junio C Hamano
@ 2019-02-19 22:31               ` Elijah Newren
  2019-02-19 22:38                 ` Junio C Hamano
       [not found]               ` <07106FB4020000CD824A10E1@gwsmtp1.uni-regensburg.de>
  1 sibling, 1 reply; 11+ messages in thread
From: Elijah Newren @ 2019-02-19 22:31 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, Ulrich Windl

On Tue, Feb 19, 2019 at 11:32 AM Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
> > +With --no-commit perform the merge and stop just before creating
> > +a merge commit, to give the user a chance to inspect and further
> > +tweak the merge result before committing.
> > ++
> > +Note that fast-forward updates do not need to create a merge
> > +commit and therefore there is no way to stop those merges with
> > +--no-commit.  Thus, if you want to ensure your branch is not
> > +changed or updated by the merge command, use --no-ff with
> > +--no-commit.
>
> While the above is an improvement (so I'll queue it on 'pu' not to
> lose sight of it), I find the use of "do not need to" above somewhat
> misleading.  It solicits a reaction "ok, we know it does not need
> to, but it could prepare to create one to allow us to further muck
> with it, no?".
>
> IOW, a fast-forward by definition does not create a merge by itself,
> so there is nowhere to stop during a creation of a merge.  So at
> least:
>
>         s/do not need to/do not/

Yes, I agree that's a good change.  I'll wait a few days for other
feedback and resend with that and any other changes.

> It also may be a good idea to consider detecting this case and be a
> bit more helpful, perhaps with end-user experience looking like...
>
>   $ git checkout master^0
>   $ git merge --no-commit next
>   Updating 0d0ac3826a..ee538a81fe
>   Fast-forward
>     ...diffstat follows here...
>   hint: merge completed without creating a commit.
>   hint: if you wanted to prepare for a manually tweaked merge,
>   hint: do "git reset --keep ORIG_HEAD" followed by
>   hint: "git merge --no-ff --no-commit next".
>
> or even
>
>   $ git checkout master^0
>   $ git merge --no-commit next
>   warning: defaulting to --no-ff, given a --no-commit request
>   Automatic merge went well; stopped before committing as requested
>   hint: if you'd rather have a fast-forward without creating a commit,
>   hint: do "git reset --keep next" now.

Good points.  I thought of this last one before sending, though
without pre- and post- warnings/hints; without such text it definitely
seemed too magical and possibly leading to unexpected surprises in a
different direction, so I dismissed it without further thought.  But
the warnings/hints help.

> I do not have a strong preference among three (the third option
> being not doing anything), but if pressed, I'd say that the last one
> might be the most user-friendly, even though it feels a bit too
> magical and trying to be smarter than its own good.

I also lack a strong preference.  Maybe mark it #leftoverbits for
someone that does?

> In any case, the hint for the "recovery" procedure needs to be
> carefully written.

Yes.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] merge-options.txt: correct wording of --no-commit option
  2019-02-19 22:31               ` Elijah Newren
@ 2019-02-19 22:38                 ` Junio C Hamano
  0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2019-02-19 22:38 UTC (permalink / raw)
  To: Elijah Newren; +Cc: Git Mailing List, Ulrich Windl

Elijah Newren <newren@gmail.com> writes:

>>   $ git checkout master^0
>>   $ git merge --no-commit next
>>   warning: defaulting to --no-ff, given a --no-commit request
>>   Automatic merge went well; stopped before committing as requested
>>   hint: if you'd rather have a fast-forward without creating a commit,
>>   hint: do "git reset --keep next" now.
>
> Good points.  I thought of this last one before sending, though
> without pre- and post- warnings/hints; without such text it definitely
> seemed too magical and possibly leading to unexpected surprises in a
> different direction, so I dismissed it without further thought.  But
> the warnings/hints help.
>
>> I do not have a strong preference among three (the third option
>> being not doing anything), but if pressed, I'd say that the last one
>> might be the most user-friendly, even though it feels a bit too
>> magical and trying to be smarter than its own good.
>
> I also lack a strong preference.  Maybe mark it #leftoverbits for
> someone that does?

This definitely is outside the scope of the documentation update.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Antw: Re: [PATCH v2] merge-options.txt: correct wording of --no-commit option
       [not found]                 ` <AC7679FC02000011B9FD70CF@gwsmtp1.uni-regensburg.de>
@ 2019-02-20  7:29                   ` Ulrich Windl
  0 siblings, 0 replies; 11+ messages in thread
From: Ulrich Windl @ 2019-02-20  7:29 UTC (permalink / raw)
  To: Elijah Newren, gitster; +Cc: git

>>> Junio C Hamano <gitster@pobox.com> schrieb am 19.02.2019 um 20:32 in
Nachricht
<xmqqk1hv1sms.fsf@gitster-ct.c.googlers.com>:
> Elijah Newren <newren@gmail.com> writes:
> 
>> +With ‑‑no‑commit perform the merge and stop just before creating
>> +a merge commit, to give the user a chance to inspect and further
>> +tweak the merge result before committing.
>> ++
>> +Note that fast‑forward updates do not need to create a merge
>> +commit and therefore there is no way to stop those merges with
>> +‑‑no‑commit.  Thus, if you want to ensure your branch is not
>> +changed or updated by the merge command, use ‑‑no‑ff with
>> +‑‑no‑commit.
> 
> While the above is an improvement (so I'll queue it on 'pu' not to
> lose sight of it), I find the use of "do not need to" above somewhat
> misleading.  It solicits a reaction "ok, we know it does not need
> to, but it could prepare to create one to allow us to further muck
> with it, no?".
> 
> IOW, a fast‑forward by definition does not create a merge by itself,
> so there is nowhere to stop during a creation of a merge.  So at
> least:
> 
> 	s/do not need to/do not/

Agree.

> 
> It also may be a good idea to consider detecting this case and be a
> bit more helpful, perhaps with end‑user experience looking like...
> 
>   $ git checkout master^0
>   $ git merge ‑‑no‑commit next
>   Updating 0d0ac3826a..ee538a81fe
>   Fast‑forward
>     ...diffstat follows here...
>   hint: merge completed without creating a commit.
>   hint: if you wanted to prepare for a manually tweaked merge,
>   hint: do "git reset ‑‑keep ORIG_HEAD" followed by
>   hint: "git merge ‑‑no‑ff ‑‑no‑commit next".
> 
> or even
> 
>   $ git checkout master^0
>   $ git merge ‑‑no‑commit next
>   warning: defaulting to ‑‑no‑ff, given a ‑‑no‑commit request
>   Automatic merge went well; stopped before committing as requested
>   hint: if you'd rather have a fast‑forward without creating a commit,
>   hint: do "git reset ‑‑keep next" now.
> 
> I do not have a strong preference among three (the third option
> being not doing anything), but if pressed, I'd say that the last one
> might be the most user‑friendly, even though it feels a bit too
> magical and trying to be smarter than its own good.
> 
> In any case, the hint for the "recovery" procedure needs to be
> carefully written.

Actually I think if the user specified "--no-commit" and the merge turns out
to be fast-forward, the user could be asked whether to continue or not (instead
of undoinf afterwards); maybe when entering a response is not possible (batch
processing) the merge should be aborted due to "--no-commit" not being possible
(well actually there would never be a commit, even without that option). The
problem is that without prior inspection of the tree you cannot know whether
the merge will be fast-forward or not: fast-forward being an optimization (taht
is enabled by default) makes life more complicated here.

Regards,
Ulrich Windl


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3] merge-options.txt: correct wording of --no-commit option
  2019-02-19 17:07           ` [PATCH v2] merge-options.txt: correct wording of --no-commit option Elijah Newren
  2019-02-19 19:32             ` Junio C Hamano
@ 2019-02-21 17:50             ` Elijah Newren
  1 sibling, 0 replies; 11+ messages in thread
From: Elijah Newren @ 2019-02-21 17:50 UTC (permalink / raw)
  To: gitster; +Cc: git, Ulrich Windl, Elijah Newren

The former wording implied that --no-commit would always cause the
merge operation to "pause" and allow the user to make further changes
and/or provide a special commit message for the merge commit.  This
is not the case for fast-forward merges, as there is no merge commit
to create.  Without a merge commit, there is no place where it makes
sense to "stop the merge and allow the user to tweak changes"; doing
that would require a full rebase of some sort.

Since users may be unaware of whether their branches have diverged or
not, modify the wording to correctly address fast-forward cases as well
and suggest using --no-ff with --no-commit if the point is to ensure
that the merge stops before completing.

Reported-by: Ulrich Windl <Ulrich.Windl@rz.uni-regensburg.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Changes since v2:
  - Small wording change suggested by Junio (s/do not need to/do not/)

 Documentation/merge-options.txt | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index c2a263ba74..5be89168c6 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -3,9 +3,14 @@
 	Perform the merge and commit the result. This option can
 	be used to override --no-commit.
 +
-With --no-commit perform the merge but pretend the merge
-failed and do not autocommit, to give the user a chance to
-inspect and further tweak the merge result before committing.
+With --no-commit perform the merge and stop just before creating
+a merge commit, to give the user a chance to inspect and further
+tweak the merge result before committing.
++
+Note that fast-forward updates do not create a merge commit and
+therefore there is no way to stop those merges with --no-commit.
+Thus, if you want to ensure your branch is not changed or updated
+by the merge command, use --no-ff with --no-commit.
 
 --edit::
 -e::
-- 
2.21.0.rc2.262.g736ce73923


^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-02-21 17:50 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-22 20:55 Q: What happened to "--no-commit" merges? Ulrich Windl
2019-01-22 21:29 ` Elijah Newren
2019-01-23  7:04   ` Antw: " Ulrich Windl
2019-02-18 18:41     ` Elijah Newren
     [not found]       ` <0A3130DD0200005B824A10E1@gwsmtp1.uni-regensburg.de>
2019-02-19  7:03         ` Antw: Antw: Ulrich Windl
2019-02-19 17:07           ` [PATCH v2] merge-options.txt: correct wording of --no-commit option Elijah Newren
2019-02-19 19:32             ` Junio C Hamano
2019-02-19 22:31               ` Elijah Newren
2019-02-19 22:38                 ` Junio C Hamano
     [not found]               ` <07106FB4020000CD824A10E1@gwsmtp1.uni-regensburg.de>
     [not found]                 ` <AC7679FC02000011B9FD70CF@gwsmtp1.uni-regensburg.de>
2019-02-20  7:29                   ` Antw: " Ulrich Windl
2019-02-21 17:50             ` [PATCH v3] " Elijah Newren

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).