git merge branch --no-commit does commit fast forward merges

git merge branch --no-commit does commit fast forward merges

[Christoph Paulik]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]


Hi Git Mailinglist, 

git merge branch --no-commit does commit fast forward merges 
leaving users no way to change the merge results. The command only 
works as expected when also adding the --no-ff flag. Looking at 
the help text of the --no-commit flag I think that this might be a 
unintended. 

I've tested this on git 2.8.0.

This bug was first reported to magit 
(https://github.com/magit/magit/issues/2620) whose maintainer then 
suggested that it might be a git bug.

Best regards,
Christoph


-- 

-------------------------------------------------------
Christoph Paulik
Twitter, Github: @cpaulik
PGP: 8CFC D7DF 2867 B2DC 749B  1B0A 6E3B A262 5186 A0AC

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: git merge branch --no-commit does commit fast forward merges

[Jacob Keller]

On Sun, Apr 17, 2016 at 2:10 PM, Christoph Paulik <cpaulik@gmail.com> wrote:
>
> Hi Git Mailinglist,
> git merge branch --no-commit does commit fast forward merges leaving users
> no way to change the merge results. The command only works as expected when
> also adding the --no-ff flag. Looking at the help text of the --no-commit
> flag I think that this might be a unintended.
> I've tested this on git 2.8.0.
>
> This bug was first reported to magit
> (https://github.com/magit/magit/issues/2620) whose maintainer then suggested
> that it might be a git bug.
>
> Best regards,
> Christoph
>


To be fair, the user can undo it with "git reset HEAD@{1}" I believe?

Thanks,
Jake

Re: git merge branch --no-commit does commit fast forward merges

[Johannes Schindelin]

Hi Christoph,

On Sun, 17 Apr 2016, Christoph Paulik wrote:

> git merge branch --no-commit does commit fast forward merges leaving
> users no way to change the merge results.

No, this is not a bug. Please note that a fast-forward does not perform a
commit at all (and therefore "does commit fast forward" is an inaccurate
description).

> The command only works as expected when also adding the --no-ff flag.

Then you need to fix your expectations ;-)

A fast-forward *avoids* committing altogether. In that light, the current
behavior is correct.

Ciao,
Johannes

Re: git merge branch --no-commit does commit fast forward merges

[Andrew Ardill]

On 18 April 2016 at 16:26, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> > The command only works as expected when also adding the --no-ff flag.
>
> Then you need to fix your expectations ;-)

I *think* the core of this problem is that the intent of the end-user
does not align with the command options available.

In this use case (as far as I can tell), the user wants to see what
the result of a merge from somewhere else will look like, without
changing their HEAD.

While you are correct in saying a fast-forward does not create any new
commits, for the user it certainly looks like a whole slew of new
commits have been added. Moreover, the nature of the option means that
the user has to investigate if the merge is a fast-forward in order to
know what the outcome of the command will be.

If the merge is a fast-forward, --no-commit has no effect on the
outcome. If the merge is not a fast-forward, --no-commit has a huge
effect on the outcome.

If I see a --no-commit option, as an inexperienced user, I would be
quite surprised to find my HEAD changed after using it. It would be
far more intuitive, for that user, for --no-commit to imply --no-ff
however I suspect that such a change may well cause more problems then
it fixes.

What I wonder is, in what situation is the current behaviour is desirable?

While I agree that the option works as designed, I think its behaviour
is more surprising to the end user then it should be.

Regards,

Andrew Ardill

Re: git merge branch --no-commit does commit fast forward merges

[Christoph Paulik]

[-- Attachment #1: Type: text/plain, Size: 2223 bytes --]


My expectations from what should happen came mainly from the 
description of the --no-commit flag in the help:

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. 

So in the case of a fast-forward the flag does not pretend that 
the merge failed. 

Regards,
Christoph 
 

Andrew Ardill writes:

> On 18 April 2016 at 16:26, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>>
>> > The command only works as expected when also adding the 
>> > --no-ff flag.
>>
>> Then you need to fix your expectations ;-)
>
> I *think* the core of this problem is that the intent of the 
> end-user does not align with the command options available.
>
> In this use case (as far as I can tell), the user wants to see 
> what the result of a merge from somewhere else will look like, 
> without changing their HEAD.
>
> While you are correct in saying a fast-forward does not create 
> any new commits, for the user it certainly looks like a whole 
> slew of new commits have been added. Moreover, the nature of the 
> option means that the user has to investigate if the merge is a 
> fast-forward in order to know what the outcome of the command 
> will be.
>
> If the merge is a fast-forward, --no-commit has no effect on the 
> outcome. If the merge is not a fast-forward, --no-commit has a 
> huge effect on the outcome.
>
> If I see a --no-commit option, as an inexperienced user, I would 
> be quite surprised to find my HEAD changed after using it. It 
> would be far more intuitive, for that user, for --no-commit to 
> imply --no-ff however I suspect that such a change may well 
> cause more problems then it fixes.
>
> What I wonder is, in what situation is the current behaviour is 
> desirable?
>
> While I agree that the option works as designed, I think its 
> behaviour is more surprising to the end user then it should be.
>
> Regards,
>
> Andrew Ardill


-- 

-------------------------------------------------------
Christoph Paulik
Twitter, Github: @cpaulik
PGP: 8CFC D7DF 2867 B2DC 749B  1B0A 6E3B A262 5186 A0AC

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: git merge branch --no-commit does commit fast forward merges

[Andrew Ardill]

On 18 April 2016 at 17:23, Christoph Paulik <cpaulik@gmail.com> wrote:
> My expectations from what should happen came mainly from the description of
> the --no-commit flag in the help:
>
> 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.
> So in the case of a fast-forward the flag does not pretend that the merge
> failed.

Yes, I think the mis-alignment in expectations comes from a
technicality in the description you quote. The fast forward is in some
ways not really counted as a true merge, and no new commits are
created.

Thus, the merge progresses up to the point where a merge resolution
would have to take place, realises that there is no merge resolution
to do (it's just a fast forward!) and so exits out. Unfortunately, a
side effect of this is that the fast-forward has already happened and
so you are left with something different from what was expected.

I do think that the --no-commit option should imply --no-ff (as this
would make the behaviour consistent for end-users). I don't know if
this is something that would break scripts etc, but if so you could
make it implied only if we detect a terminal or something like is done
in other places.

Regards,

Andrew Ardill

Re: git merge branch --no-commit does commit fast forward merges

[Junio C Hamano]

Andrew Ardill <andrew.ardill@gmail.com> writes:

> Yes, I think the mis-alignment in expectations comes from a
> technicality in the description you quote. The fast forward is in some
> ways not really counted as a true merge, and no new commits are
> created.

Looking at 123ee3ca (Add --no-commit to git-merge/git-pull.,
2005-11-01) and $gmane/10998 [*1*], it is clear that "--no-commit"
was never meant as a "preview of what would happen".  The original
documentation update at 37465016 (Documentation: -merge and -pull:
describe merge strategies., 2005-11-04) was not great, but was
clarified at d8ae1d10 (Document the --no-commit flag better,
2005-11-04), and that version of text survives to this day.

The real reason why "--no-commit" was added was because back then
"git commit --amend" did not even exist; it appeared only at
b4019f04 (git-commit --amend, 2006-03-02).

What is (and was back then) the recommended way to see what changes
merging the other branch brings in to your branch, then?

There are at least three ways, all of which are better suited than
"--no-commit".

When you want to study and understand what changes other branch 
made since it forked from what you are working on, then

    $ git diff ...other_branch

would give you the change as a single ball of wax [*2*].

If you want to see individual changes explained by their authors,
you can also do

    $ git log -p ..other_branch

Finally, if you want to see what the merge result would look like,
you just do the merge.  Advancing the HEAD by one commit and then
going back once you are done is a cheap operation.  If you want to
avoid updating your branch for real, these days you can even do so
on a detached HEAD, unlike old days back when there was not even
"commit --amend".

    $ git checkout HEAD^0
    $ git merge other_branch

    $ git diff ORIG_HEAD     ;# what changed overall?
    $ git log -p ORIG_HEAD.. ;# inspect individual changes

    $ git checkout - ;# come back to the original branch

> I do think that the --no-commit option should imply --no-ff (as this
> would make the behaviour consistent for end-users). I don't know if
> this is something that would break scripts etc, but if so you could
> make it implied only if we detect a terminal or something like is done
> in other places.

If we were living in an ideal world where "git commit --amend" were
already there in November 2005, we wouldn't have "merge --no-commit"
or "pull --no-commit" in our system today, and in such a world, I
would agree that "try to populate the working tree and the index
with result of a hypothetical merge and stop without updating HEAD
nor creating MERGE_HEAD, only to show what would happen if I merged"
option could be a useful addition to these two commands.  And we may
choose to call such an option "--no-commit".  I agree that such an
option should probably imply "--no-ff".

But we are not living in that world.  Making "--no-commit" (which is
not that "try to populate and show" command) imply "--no-ff" will
break existing scripts.  And unlike cosmetic things like "do we show
in color", changing the behaviour of a command in a fundamental way
based on term and istty() is a sure way to make commands harder to
understand ("it works this way from the terminal, but it works
differently in my script. what is going on?"  is not a question we
are better off not seeing on this list).

Thanks.

[Notes and References]

*1* http://thread.gmane.org/gmane.comp.version-control.git/10998 

*2* Notice the three dots; it is a short-hand for

    $ git diff ^$(git merge-base HEAD other_branch) other_branch

Re: git merge branch --no-commit does commit fast forward merges

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Andrew Ardill <andrew.ardill@gmail.com> writes:
>
>> I do think that the --no-commit option should imply --no-ff (as this
>> would make the behaviour consistent for end-users). I don't know if
>> this is something that would break scripts etc, but if so you could
>> make it implied only if we detect a terminal or something like is done
>> in other places.
>
> But we are not living in that world.  Making "--no-commit" (which is
> not that "try to populate and show" command) imply "--no-ff" will
> break existing scripts....

Having said all that, there is one change we might want to consider,
with a plan to transition to cope with backward incompatibility.

A user who uses "--no-commit" does so with the intention to record a
resulting merge after amending the merge result in the working tree.
But there is nothing to amend and record, if the same "git merge"
without "--no-commit" wouldn't have created a merge commit (there
are two cases: (1) the other branch is a descendant of the current
branch, (2) the other branch is an ancestor of the current branch).

And the user would want to know that before doing further damange to
his history, so we may want to start warn when "--no-commit"
fast-forwarded or succeeded with "already up-to-date", with
deprecation notice, and eventually want to make it an error.

Those who want to do a scripted

	git merge --no-commit "$1" &&
        autoedit "$1" &&
        git commit

(where the script takes a branch name $1 and uses auto-edit to
further record the fact that a branch $1 was merged to somewhere in
the contents) would already be buggy if it wants to force no-ff, and
will get a warning (and later an error), with such a change.  And
fixing the script to add "--no-ff" next to "--no-commit" will also
stop the new warning/error.

[PATCH 1/2] merge: do not contaminate option_commit with --squash

[Junio C Hamano]

It is true that we do not make a commit when we are asked to do
"merge --squash", and the code does so by setting option_commit
variable to zero when seeing the squash option.  But this made it
impossible to see from the value of option_commit if --no-commit was
given from the command line, or --squash turned it off.

We check for the value of option_commit at only two places.  Check
for the value of squash at them, too.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Just a preliminary clean-up for the next one which is on topic.

 builtin/merge.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index bf2f261..de9d1b6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1237,11 +1237,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (verbosity < 0)
 		show_diffstat = 0;
 
-	if (squash) {
-		if (fast_forward == FF_NO)
-			die(_("You cannot combine --squash with --no-ff."));
-		option_commit = 0;
-	}
+	if (squash && fast_forward == FF_NO)
+		die(_("You cannot combine --squash with --no-ff."));
 
 	if (!argc) {
 		if (default_to_upstream)
@@ -1449,10 +1446,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * We are not doing octopus and not fast-forward.  Need
 		 * a real merge.
 		 */
-	else if (!remoteheads->next && !common->next && option_commit) {
+	else if (!remoteheads->next && !common->next && option_commit && !squash)
 		/*
 		 * We are not doing octopus, not fast-forward, and have
-		 * only one common.
+		 * only one common.  And we do want to create a new commit.
 		 */
 		refresh_cache(REFRESH_QUIET);
 		if (allow_trivial && fast_forward != FF_ONLY) {
@@ -1535,7 +1532,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		ret = try_merge_strategy(use_strategies[i]->name,
 					 common, remoteheads,
 					 head_commit, head_arg);
-		if (!option_commit && !ret) {
+		if ((!option_commit || squash) && !ret) {
 			merge_was_ok = 1;
 			/*
 			 * This is necessary here just to avoid writing
-- 
2.8.1-491-g88b9e4a

Re: [PATCH 1/2] merge: do not contaminate option_commit with --squash

[Johannes Schindelin]

Hi Junio,

On Tue, 26 Apr 2016, Junio C Hamano wrote:

> It is true that we do not make a commit when we are asked to do
> "merge --squash", and the code does so by setting option_commit
> variable to zero when seeing the squash option.  But this made it
> impossible to see from the value of option_commit if --no-commit was
> given from the command line, or --squash turned it off.
> 
> We check for the value of option_commit at only two places.  Check
> for the value of squash at them, too.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * Just a preliminary clean-up for the next one which is on topic.

I think it would make for a nice cleanup anyways.

Ciao,
Dscho

Re: [PATCH 1/2] merge: do not contaminate option_commit with --squash

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>>  * Just a preliminary clean-up for the next one which is on topic.
>
> I think it would make for a nice cleanup anyways.

There is a missing open-brace that causes a compilation error,
though ;-)

Re: [PATCH 1/2] merge: do not contaminate option_commit with --squash

[Johannes Schindelin]

Hi Junio,

On Wed, 27 Apr 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >>  * Just a preliminary clean-up for the next one which is on topic.
> >
> > I think it would make for a nice cleanup anyways.
> 
> There is a missing open-brace that causes a compilation error,
> though ;-)

Uh oh ;-) I guess I did not really test it...

Ciao,
Dscho

[PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Junio C Hamano]

A user who uses "--no-commit" does so with the intention to record a
resulting merge after amending the merge result in the working tree.
But there is nothing to amend and record, if the same "git merge"
without "--no-commit" wouldn't have created a merge commit (there
are two cases: (1) the other branch is a descendant of the current
branch, (2) the other branch is an ancestor of the current branch).

The user would want to know that before doing further damange to his
history.  When "merge --no-commit" fast-forwarded or succeeded with
"already up-to-date" or "fast-forward", give a warning message.

We may want to turn this into a die() after a transition period.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * The necessary update to avoid end-user mistake would look like
   this.  I am not queuing this or further working on it myself,
   as I am not sure if it is all that useful.

 builtin/merge.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index f641751..66c536d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1157,6 +1157,15 @@ static struct commit_list *collect_parents(struct commit *head_commit,
 	return remoteheads;
 }
 
+static void no_commit_impossible(const char *message)
+{
+	if (!option_commit) {
+		warning("%s\n%s", _(message),
+			_("--no-commit is impossible"));
+		warning(_("In future versions of Git, this will become an error."));
+	}
+}
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];
@@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * If head can reach all the merge then we are up to date.
 		 * but first the most common case of merging one remote.
 		 */
+		no_commit_impossible(_("Already up-to-date"));
 		finish_up_to_date("Already up-to-date.");
 		goto done;
 	} else if (fast_forward != FF_NO && !remoteheads->next &&
@@ -1412,6 +1422,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		struct strbuf msg = STRBUF_INIT;
 		struct commit *commit;
 
+		no_commit_impossible(_("Fast-forward"));
 		if (verbosity >= 0) {
 			char from[GIT_SHA1_HEXSZ + 1], to[GIT_SHA1_HEXSZ + 1];
 			find_unique_abbrev_r(from, head_commit->object.oid.hash,
@@ -1488,6 +1499,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			}
 		}
 		if (up_to_date) {
+			no_commit_impossible(_("Already up-to-date"));
 			finish_up_to_date("Already up-to-date. Yeeah!");
 			goto done;
 		}
-- 
2.8.1-491-g88b9e4a

Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Stefan Beller]

On Tue, Apr 26, 2016 at 2:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> +static void no_commit_impossible(const char *message)
> +{
> +       if (!option_commit) {
> +               warning("%s\n%s", _(message),
> +                       _("--no-commit is impossible"));
> +               warning(_("In future versions of Git, this will become an error."));
> +       }
> +}

During discussion of the parallel process framework
(sb/submodule-parallel-fetch~3),
you seemed very inclined on not having major decisions made deep
inside the helper
function, but rather at the main function to easier see the program flow IIRC.

This looks very similar to me as we'll have the no_commit_impossible function
which is a helper of cmd_merge. Following your advice there, I would
have expected to
have

    static void no_commit_impossible(const char *message)
    {
        warning("%s\n%s", _(message), _("--no-commit is impossible"));
        warning(_("In future versions of Git, this will become an error."));
    }

and later

    if (!option_commit)
        no_commit_impossible(_("Already up-to-date"));


> +
>  int cmd_merge(int argc, const char **argv, const char *prefix)
>  {
>         unsigned char result_tree[20];
> @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>                  * If head can reach all the merge then we are up to date.
>                  * but first the most common case of merging one remote.
>                  */
> +               no_commit_impossible(_("Already up-to-date"));
>                 finish_up_to_date("Already up-to-date.");

Coming back to this patch, in case of -v given, we'll
see ("Already up-to-date") twice?

If --quiet is given, do we want to suppress output
in no_commit_impossible?

Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> and later
>
>     if (!option_commit)
>         no_commit_impossible(_("Already up-to-date"));

It would be more legible, but because there are so few callsites in
an already shallow callchain, I do not think it makes that much of a
difference in this codepath either way.

>> +
>>  int cmd_merge(int argc, const char **argv, const char *prefix)
>>  {
>>         unsigned char result_tree[20];
>> @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>>                  * If head can reach all the merge then we are up to date.
>>                  * but first the most common case of merging one remote.
>>                  */
>> +               no_commit_impossible(_("Already up-to-date"));
>>                 finish_up_to_date("Already up-to-date.");
>
> Coming back to this patch, in case of -v given, we'll
> see ("Already up-to-date") twice?

One that explains why --no-commit is impossible in warning, and the
other is the final report of what happened, so yes.

> If --quiet is given, do we want to suppress output
> in no_commit_impossible?

While we are using warning(), we probably do want to.  When we
switch to die() at a major version boundary, we don't.

Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Eric Sunshine]

On Tue, Apr 26, 2016 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> A user who uses "--no-commit" does so with the intention to record a
> resulting merge after amending the merge result in the working tree.
> But there is nothing to amend and record, if the same "git merge"
> without "--no-commit" wouldn't have created a merge commit (there
> are two cases: (1) the other branch is a descendant of the current
> branch, (2) the other branch is an ancestor of the current branch).
>
> The user would want to know that before doing further damange to his

s/damange/damage/

> history.  When "merge --no-commit" fast-forwarded or succeeded with
> "already up-to-date" or "fast-forward", give a warning message.
>
> We may want to turn this into a die() after a transition period.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Johannes Sixt]

Am 26.04.2016 um 23:37 schrieb Junio C Hamano:
>   * The necessary update to avoid end-user mistake would look like
>     this.  I am not queuing this or further working on it myself,
>     as I am not sure if it is all that useful.

Whoever picks up this patch, be warned that the i18n coding should be 
corrected:

> +static void no_commit_impossible(const char *message)
> +{
> +	if (!option_commit) {
> +		warning("%s\n%s", _(message),

The i18n call around message is not required, because...

> +			_("--no-commit is impossible"));
> +		warning(_("In future versions of Git, this will become an error."));
> +	}
> +}
> +
>   int cmd_merge(int argc, const char **argv, const char *prefix)
>   {
>   	unsigned char result_tree[20];
> @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>   		 * If head can reach all the merge then we are up to date.
>   		 * but first the most common case of merging one remote.
>   		 */
> +		no_commit_impossible(_("Already up-to-date"));

... the call sites takes care of it.

-- Hannes

Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Johannes Schindelin]

Hi Junio,

On Tue, 26 Apr 2016, Junio C Hamano wrote:

> @@ -1157,6 +1157,15 @@ static struct commit_list *collect_parents(struct commit *head_commit,
>  	return remoteheads;
>  }
>  
> +static void no_commit_impossible(const char *message)
> +{
> +	if (!option_commit) {
> +		warning("%s\n%s", _(message),
> +			_("--no-commit is impossible"));
> +		warning(_("In future versions of Git, this will become an error."));
> +	}
> +}

I think this would be a step forward in usability, and I agree that this
is a great opportunity for the users who wish for this feature to get
involved and drive this forward, based on your excellent initial version.

I am not sure about this double negation "no_commit_impossible" (I only
understood what you meant because I had read the commit message first,
something I won't do when stumbling over this code later).

Maybe something like `disallow_no_commit`?

Ciao,
Dscho

Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I am not sure about this double negation "no_commit_impossible" (I only
> understood what you meant because I had read the commit message first,
> something I won't do when stumbling over this code later).
>
> Maybe something like `disallow_no_commit`?

That would be the best name once we start dying in there.  It might
be still better, even while we merely warn but let it pass, than the
double negative.  Or it may not.  I dunno.

Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Johannes Schindelin]

Hi Junio,

On Wed, 27 Apr 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > I am not sure about this double negation "no_commit_impossible" (I only
> > understood what you meant because I had read the commit message first,
> > something I won't do when stumbling over this code later).
> >
> > Maybe something like `disallow_no_commit`?
> 
> That would be the best name once we start dying in there.  It might
> be still better, even while we merely warn but let it pass, than the
> double negative.  Or it may not.  I dunno.

Actually, I should admit that I was really puzzled by the name at first. I
thought that some commits were impossible, but the function said that no
commit was impossible. So I thought: but what if a commit references
itself as parent, would that not be impossible? But actually, once SHA-1
collision attacks become feasible, I guess it would not be impossible.
Making for an excellent attack vector, say, on repository hosting sites
(which would now be stuck in infinite loops due to a violation of the
temporal prime directive).

So yeah, this was my thought process when I read no_commit_impossible.

;-)

Ciao,
Dscho

Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > Maybe something like `disallow_no_commit`?
>> 
>> That would be the best name once we start dying in there.  It might
>> be still better, even while we merely warn but let it pass, than the
>> double negative.  Or it may not.  I dunno.
>
> Actually, I should admit that I was really puzzled by the name at first. I
> thought that some commits were impossible, but the function said that no
> commit was impossible. So I thought: but what if a commit references
> itself as parent, would that not be impossible? But actually, once SHA-1
> collision attacks become feasible, I guess it would not be impossible.
> Making for an excellent attack vector, say, on repository hosting sites
> (which would now be stuck in infinite loops due to a violation of the
> temporal prime directive).
>
> So yeah, this was my thought process when I read no_commit_impossible.

We both know that the original name was terrible now.  It was meant
to mean "it is impossible to honor --no-commit option", nothing
more, and left the door open for deciding what should happen when
that condition holds (either "warn but succeed the merge anyway" or
"die to refuse"), but it is clear that the name did not convey that
successfully.