threads / patch / 5626

patchadd receive.denyNonFastforwards config variable

Subject: [PATCH] add receive.denyNonFastforwards config variable

## tl;dr

18 messages between Sep 20, 2006 and Sep 21, 2006. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

Johannes Schindelin· Sep 20, 2006, 22:28 UTC · lore

If receive.denyNonFastforwards is set to true, git-receive-pack will deny non fast-forwards, i.e. forced updates. Most notably, a push to a repository which has that flag set to true will fail.

As a first user, 'git-init-db --shared' sets this flag, since in a shared setup, you are most unlikely to want forced pushes to succeed.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 builtin-init-db.c |    1 +
 cache.h           |    1 +
 environment.c     |    1 +
 receive-pack.c    |   12 ++++++++++++
 setup.c           |    2 ++
 5 files changed, 17 insertions(+), 0 deletions(-)
Show changes to 5 files +17 −0

builtin-init-db.c, cache.h, environment.c, receive-pack.c, setup.c

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 36c3088..e6a2d7d 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -310,6 +310,7 @@ int cmd_init_db(int argc, const char **a
 		 */
 		sprintf(buf, "%d", shared_repository);
 		git_config_set("core.sharedrepository", buf);
+		git_config_set("receive.denyNonFastforwards", "true");
 	}
 
 	return 0;
diff --git a/cache.h b/cache.h
index f2fdc00..2224c83 100644
--- a/cache.h
+++ b/cache.h
@@ -188,6 +188,7 @@ extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int shared_repository;
+extern int deny_non_fast_forwards;
 extern const char *apply_default_whitespace;
 extern int zlib_compression_level;
 
diff --git a/environment.c b/environment.c
index 84d870c..63b1d15 100644
--- a/environment.c
+++ b/environment.c
@@ -20,6 +20,7 @@ int warn_ambiguous_refs = 1;
 int repository_format_version;
 char git_commit_encoding[MAX_ENCODING_LENGTH] = "utf-8";
 int shared_repository = PERM_UMASK;
+int deny_non_fast_forwards = 0;
 const char *apply_default_whitespace;
 int zlib_compression_level = Z_DEFAULT_COMPRESSION;
 int pager_in_use;
diff --git a/receive-pack.c b/receive-pack.c
index 78f75da..c5ea2a1 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -2,6 +2,8 @@ #include "cache.h"
 #include "refs.h"
 #include "pkt-line.h"
 #include "run-command.h"
+#include "commit.h"
+#include "object.h"
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
@@ -127,6 +129,16 @@ static int update(struct command *cmd)
 		return error("unpack should have generated %s, "
 			     "but I can't find it!", new_hex);
 	}
+	if (deny_non_fast_forwards) {
+		struct commit *old_commit, *new_commit;
+		old_commit = (struct commit *)parse_object(old_sha1);
+		new_commit = (struct commit *)parse_object(new_sha1);
+		struct commit_list *bases = get_merge_bases(old_commit,
+				new_commit, 1);
+		if (!bases || hashcmp(old_sha1, bases->item->object.sha1))
+			return error("denying non-fast forward;"
+					" you should pull first");
+	}
 	safe_create_leading_directories(lock_name);
 
 	newfd = open(lock_name, O_CREAT | O_EXCL | O_WRONLY, 0666);
diff --git a/setup.c b/setup.c
index 2afdba4..9a46a58 100644
--- a/setup.c
+++ b/setup.c
@@ -244,6 +244,8 @@ int check_repository_format_version(cons
                repository_format_version = git_config_int(var, value);
 	else if (strcmp(var, "core.sharedrepository") == 0)
 		shared_repository = git_config_perm(var, value);
+	else if (strcmp(var, "receive.denynonfastforwards") == 0)
+		deny_non_fast_forwards = git_config_bool(var, value);
        return 0;
 }
 
-- 
1.4.2.1.g7521-dirty
Junio C Hamano· Sep 20, 2006, 22:44 UTC · re: Johannes Schindelin · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 6 quoted lines
> If receive.denyNonFastforwards is set to true, git-receive-pack will deny
> non fast-forwards, i.e. forced updates. Most notably, a push to a repository
> which has that flag set to true will fail.
>
> As a first user, 'git-init-db --shared' sets this flag, since in a shared
> setup, you are most unlikely to want forced pushes to succeed.
I am Ok with the general idea, but ...
Show 17 quoted lines
> @@ -127,6 +129,16 @@ static int update(struct command *cmd)
>  		return error("unpack should have generated %s, "
>  			     "but I can't find it!", new_hex);
>  	}
> +	if (deny_non_fast_forwards) {
> +		struct commit *old_commit, *new_commit;
> +		old_commit = (struct commit *)parse_object(old_sha1);
> +		new_commit = (struct commit *)parse_object(new_sha1);
> +		struct commit_list *bases = get_merge_bases(old_commit,
> +				new_commit, 1);
> +		if (!bases || hashcmp(old_sha1, bases->item->object.sha1))
> +			return error("denying non-fast forward;"
> +					" you should pull first");
> +	}
>  	safe_create_leading_directories(lock_name);
>  
>  	newfd = open(lock_name, O_CREAT | O_EXCL | O_WRONLY, 0666);

I am wondering if there can be more than one base and the old_sha1 is not returned as the first one.

Shawn Pearce· Sep 20, 2006, 22:46 UTC · re: Junio C Hamano · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Junio C Hamano <junkio@cox.net> wrote:
Show 31 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > If receive.denyNonFastforwards is set to true, git-receive-pack will deny
> > non fast-forwards, i.e. forced updates. Most notably, a push to a repository
> > which has that flag set to true will fail.
> >
> > As a first user, 'git-init-db --shared' sets this flag, since in a shared
> > setup, you are most unlikely to want forced pushes to succeed.
> 
> I am Ok with the general idea, but ...
> 
> > @@ -127,6 +129,16 @@ static int update(struct command *cmd)
> >  		return error("unpack should have generated %s, "
> >  			     "but I can't find it!", new_hex);
> >  	}
> > +	if (deny_non_fast_forwards) {
> > +		struct commit *old_commit, *new_commit;
> > +		old_commit = (struct commit *)parse_object(old_sha1);
> > +		new_commit = (struct commit *)parse_object(new_sha1);
> > +		struct commit_list *bases = get_merge_bases(old_commit,
> > +				new_commit, 1);
> > +		if (!bases || hashcmp(old_sha1, bases->item->object.sha1))
> > +			return error("denying non-fast forward;"
> > +					" you should pull first");
> > +	}
> >  	safe_create_leading_directories(lock_name);
> >  
> >  	newfd = open(lock_name, O_CREAT | O_EXCL | O_WRONLY, 0666);
> 
> I am wondering if there can be more than one base and the old_sha1
> is not returned as the first one.

Not to mention how does this work when the ref didn't exist? Is this entire block of code being bypassed by something not show in the context?

-- 
Shawn.
Johannes Schindelin· Sep 20, 2006, 23:07 UTC · re: Junio C Hamano · lore

If receive.denyNonFastforwards is set to true, git-receive-pack will deny non fast-forwards, i.e. forced updates. Most notably, a push to a repository which has that flag set will fail.

As a first user, 'git-init-db --shared' sets this flag, since in a shared setup, you are most unlikely to want forced pushes to succeed.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
	2nd try.
	No longer barfs on new refs, and tries all merge bases (even if I
	cannot come up with any scenario where there is more than one merge
	base in the case of a fast forward).
	Also is C99.
 builtin-init-db.c |    1 +
 cache.h           |    1 +
 environment.c     |    1 +
 receive-pack.c    |   16 ++++++++++++++++
 setup.c           |    2 ++
 5 files changed, 21 insertions(+), 0 deletions(-)
Show changes to 5 files +21 −0

builtin-init-db.c, cache.h, environment.c, receive-pack.c, setup.c

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 36c3088..e6a2d7d 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -310,6 +310,7 @@ int cmd_init_db(int argc, const char **a
 		 */
 		sprintf(buf, "%d", shared_repository);
 		git_config_set("core.sharedrepository", buf);
+		git_config_set("receive.denyNonFastforwards", "true");
 	}
 
 	return 0;
diff --git a/cache.h b/cache.h
index f2fdc00..2224c83 100644
--- a/cache.h
+++ b/cache.h
@@ -188,6 +188,7 @@ extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int shared_repository;
+extern int deny_non_fast_forwards;
 extern const char *apply_default_whitespace;
 extern int zlib_compression_level;
 
diff --git a/environment.c b/environment.c
index 84d870c..63b1d15 100644
--- a/environment.c
+++ b/environment.c
@@ -20,6 +20,7 @@ int warn_ambiguous_refs = 1;
 int repository_format_version;
 char git_commit_encoding[MAX_ENCODING_LENGTH] = "utf-8";
 int shared_repository = PERM_UMASK;
+int deny_non_fast_forwards = 0;
 const char *apply_default_whitespace;
 int zlib_compression_level = Z_DEFAULT_COMPRESSION;
 int pager_in_use;
diff --git a/receive-pack.c b/receive-pack.c
index 78f75da..a6ec9f9 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -2,6 +2,8 @@ #include "cache.h"
 #include "refs.h"
 #include "pkt-line.h"
 #include "run-command.h"
+#include "commit.h"
+#include "object.h"
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
@@ -127,6 +129,20 @@ static int update(struct command *cmd)
 		return error("unpack should have generated %s, "
 			     "but I can't find it!", new_hex);
 	}
+	if (deny_non_fast_forwards && !is_null_sha1(old_sha1)) {
+		struct commit *old_commit, *new_commit;
+		struct commit_list *bases;
+
+		old_commit = (struct commit *)parse_object(old_sha1);
+		new_commit = (struct commit *)parse_object(new_sha1);
+		for (bases = get_merge_bases(old_commit, new_commit, 1);
+				bases; bases = bases->next)
+			if (!hashcmp(old_sha1, bases->item->object.sha1))
+				break;
+		if (!bases)
+			return error("denying non-fast forward;"
+					" you should pull first");
+	}
 	safe_create_leading_directories(lock_name);
 
 	newfd = open(lock_name, O_CREAT | O_EXCL | O_WRONLY, 0666);
diff --git a/setup.c b/setup.c
index 2afdba4..9a46a58 100644
--- a/setup.c
+++ b/setup.c
@@ -244,6 +244,8 @@ int check_repository_format_version(cons
                repository_format_version = git_config_int(var, value);
 	else if (strcmp(var, "core.sharedrepository") == 0)
 		shared_repository = git_config_perm(var, value);
+	else if (strcmp(var, "receive.denynonfastforwards") == 0)
+		deny_non_fast_forwards = git_config_bool(var, value);
        return 0;
 }
 
-- 
1.4.2.1.g25e3-dirty
Junio C Hamano· Sep 20, 2006, 23:19 UTC · re: Johannes Schindelin · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
> If receive.denyNonFastforwards is set to true, git-receive-pack will deny
> non fast-forwards, i.e. forced updates. Most notably, a push to a repository
> which has that flag set will fail.
>
> As a first user, 'git-init-db --shared' sets this flag, since in a shared
> setup, you are most unlikely to want forced pushes to succeed.
>
> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Looks good.  Care to do a handful more tasks before we forget?
 Documentation/git-init-db.txt
 Documentation/config.txt
 Documentation/git-receive-pack.txt
 t/t5400-send-pack.sh or a new test t/t5401-push-into-shared-repo.sh
Johannes Schindelin· Sep 21, 2006, 00:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Hi,
On Wed, 20 Sep 2006, Junio C Hamano wrote:
Show 17 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > If receive.denyNonFastforwards is set to true, git-receive-pack will deny
> > non fast-forwards, i.e. forced updates. Most notably, a push to a repository
> > which has that flag set will fail.
> >
> > As a first user, 'git-init-db --shared' sets this flag, since in a shared
> > setup, you are most unlikely to want forced pushes to succeed.
> >
> > Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 
> Looks good.  Care to do a handful more tasks before we forget?
> 
>  Documentation/git-init-db.txt
>  Documentation/config.txt
>  Documentation/git-receive-pack.txt
>  t/t5400-send-pack.sh or a new test t/t5401-push-into-shared-repo.sh
I expected Jakub to ask for it ;-)

-- snip -- [PATCH] Document receive.denyNonFastforwards

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 Documentation/config.txt           |    7 +++++++
 Documentation/git-init-db.txt      |    4 ++++
 Documentation/git-receive-pack.txt |    2 ++
 t/t5400-send-pack.sh               |   10 ++++++++++
 4 files changed, 23 insertions(+), 0 deletions(-)
Show changes to 4 files +23 −0

Documentation/config.txt, Documentation/git-init-db.txt, Documentation/git-receive-pack.txt, t/t5400-send-pack.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 844cae4..6802d30 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -267,3 +267,10 @@ whatchanged.difftree::
 imap::
 	The configuration variables in the 'imap' section are described
 	in gitlink:git-imap-send[1].
+
+receive.denyNonFastforwads::
+	If set to true, git-receive-pack will deny a ref update which is
+	not a fast forward. Use this to prevent such an update via a push,
+	even if that push is forced. This configuration variable is
+	set when initializing a shared repository.
+	
diff --git a/Documentation/git-init-db.txt b/Documentation/git-init-db.txt
index 63cd5da..ca7d09d 100644
--- a/Documentation/git-init-db.txt
+++ b/Documentation/git-init-db.txt
@@ -48,6 +48,10 @@ is given:
  - 'all' (or 'world' or 'everybody'): Same as 'group', but make the repository
    readable by all users.
 
+By default, the configuration flag receive.denyNonFastforward is enabled
+in shared repositories, so that you cannot force a non fast-forwarding push
+into it.
+
 --
 
 
diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index f9457d4..0dfadc2 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -73,6 +73,8 @@ packed and is served via a dumb transpor
 There are other real-world examples of using update and
 post-update hooks found in the Documentation/howto directory.
 
+git-receive-pack honours the receive.denyNonFastforwards flag, which
+tells it if updates to a ref should be denied if they are not fast-forwards.
 
 OPTIONS
 -------
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index f3694ac..6be3c80 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -64,4 +64,14 @@ test_expect_success \
 	cmp victim/.git/refs/heads/master .git/refs/heads/master
 '
 
+test_expect_success \
+        'pushing with --force should be denied with denyNonFastforwards' '
+	cd victim &&
+	git-repo-config receive.denyNonFastforwards true &&
+	cd .. &&
+	git-update-ref refs/heads/master master^ &&
+	git-send-pack --force ./victim/.git/ master &&
+	! diff -u .git/refs/heads/master victim/.git/refs/heads/master
+'
+
 test_done
-- 
1.4.2.1.g6ad2-dirty
Junio C Hamano· Sep 21, 2006, 06:26 UTC · re: Johannes Schindelin · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 19 quoted lines
> diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
> index f3694ac..6be3c80 100755
> --- a/t/t5400-send-pack.sh
> +++ b/t/t5400-send-pack.sh
> @@ -64,4 +64,14 @@ test_expect_success \
>  	cmp victim/.git/refs/heads/master .git/refs/heads/master
>  '
>  
> +test_expect_success \
> +        'pushing with --force should be denied with denyNonFastforwards' '
> +	cd victim &&
> +	git-repo-config receive.denyNonFastforwards true &&
> +	cd .. &&
> +	git-update-ref refs/heads/master master^ &&
> +	git-send-pack --force ./victim/.git/ master &&
> +	! diff -u .git/refs/heads/master victim/.git/refs/heads/master
> +'
> +
>  test_done

The change shown by "git -SGIT_CONFIG -p t/Makefile" seems to interfere with this test. When I run it from shell by hand it works OK, but when I run it like this:

	cd t && make T=t5400*.sh

it barfs. s/git-send-pack/strace -f git-send-pack/ seems to indicate that the receiving end is trying to open .git/config (rightly, because you told it to do so three months ago ;-) while its $cwd is victim/.git and fails to read the setting you are trying to test. Not good.

I think the change to t/Makefile from 9c3796fc needs to be rethought. Most of the tests would be fine with .git/config (they never step outside t/trash/.git repository), but others need tweaking, including unsetting it (while unsetting HOME at the same time perhaps when you do not want to be affected). Also it may make sense to do the default setting of GIT_CONFIG in t/test-lib.sh, with a prominent warning why we explicitly set it to a known value.

In the meantime, I got things working with this patch on top of your test update.

---
Show changes to t/t5400-send-pack.sh +4 −0
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 6be3c80..be47ee2 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -64,6 +64,10 @@ test_expect_success \
 	cmp victim/.git/refs/heads/master .git/refs/heads/master
 '
 
+unset GIT_CONFIG GIT_CONFIG_LOCAL
+HOME=`pwd`/no-such-directory
+export HOME ;# this way we force the victim/.git/config to be used.
+
 test_expect_success \
         'pushing with --force should be denied with denyNonFastforwards' '
 	cd victim &&
Johannes Schindelin· Sep 21, 2006, 09:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Hi,
On Wed, 20 Sep 2006, Junio C Hamano wrote:
> I think the change to t/Makefile from 9c3796fc needs to be
> rethought.

I looked into 9c3796fc, and the change to t/Makefile is UTTER CRAP! What IDIOT has made that change? Oh wait, was me...

What about removing that creepy change to t/Makefile, and move your hack into test-lib.sh? It _should_ be safe, as _nothing_ in $HOME should affect the tests anyway.

Thoughts?

Ciao, Dscho

Junio C Hamano· Sep 20, 2006, 23:38 UTC · re: Johannes Schindelin · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 	No longer barfs on new refs, and tries all merge bases (even if I
> 	cannot come up with any scenario where there is more than one merge
> 	base in the case of a fast forward).

Hmm. If that is the case (and I think it is although I haven't come up with a proof), the test can be written like this:

	if (bases && !bases->next &&
            hashcmp(old_sha1, bases->item->object.sha1))
        	; /* happy */
	else
        	return error("not a fast forward");
perhaps?
Johannes Schindelin· Sep 21, 2006, 00:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Hi,
On Wed, 20 Sep 2006, Junio C Hamano wrote:
Show 8 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 	No longer barfs on new refs, and tries all merge bases (even if I
> > 	cannot come up with any scenario where there is more than one merge
> > 	base in the case of a fast forward).
> 
> Hmm.  If that is the case (and I think it is although I haven't
> come up with a proof),
>From git-fetch.sh:
            # Require fast-forward.
            mb=$(git-merge-base "$local" "$2") &&
            case "$2,$mb" in
            $local,*)
                if test -n "$verbose"
                then
                        echo >&2 "* $1: same as $3"
                fi
                ;;
            *,$local)
                echo >&2 "* $1: fast forward to $3"
                echo >&2 "  from $local to $2"
                git-update-ref -m "$rloga: fast-forward" "$1" "$2" "$local"
                ;;

So we indeed assumed that git-merge-base returns the old commit in the case of a fast-forward (git-merge-base returns just the first item of the result of get_merge_bases()).

Note that I have no proof that this assumption is true. It might be wrong in this case:

    X - a - b - c - Y
  /           /
o - d - e - f

where X is the old commit, and Y is the new commit. But I am too tired to test it right now.

Show 7 quoted lines
>... the test can be written like this:
> 
> 	if (bases && !bases->next &&
>             hashcmp(old_sha1, bases->item->object.sha1))
>         	; /* happy */
> 	else
>         	return error("not a fast forward");
Plus
	free_commit_list(bases);
as Jeff pointed out.

Ciao, Dscho

Junio C Hamano· Sep 21, 2006, 05:08 UTC · re: Johannes Schindelin · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 19 quoted lines
> On Wed, 20 Sep 2006, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > 	No longer barfs on new refs, and tries all merge bases (even if I
>> > 	cannot come up with any scenario where there is more than one merge
>> > 	base in the case of a fast forward).
>> 
>> Hmm.  If that is the case (and I think it is although I haven't
>> come up with a proof),
>
> From git-fetch.sh:
>
>             # Require fast-forward.
>             mb=$(git-merge-base "$local" "$2") &&
>...
> So we indeed assumed that git-merge-base returns the old commit in the 
> case of a fast-forward (git-merge-base returns just the first item of the 
> result of get_merge_bases()).

I think this was leftover from the days we naïvely assumed that multiple bases does not matter, and probably needs updating.

Show 9 quoted lines
> Note that I have no proof that this assumption is true. It might be wrong 
> in this case:
>
>     X - a - b - c - Y
>   /           /
> o - d - e - f
>
> where X is the old commit, and Y is the new commit. But I am too tired to 
> test it right now.
Jeff King· Sep 21, 2006, 05:52 UTC · re: Johannes Schindelin · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

On Thu, Sep 21, 2006 at 02:17:22AM +0200, Johannes Schindelin wrote:
Show 10 quoted lines
> So we indeed assumed that git-merge-base returns the old commit in the 
> case of a fast-forward (git-merge-base returns just the first item of the 
> result of get_merge_bases()).
> 
> Note that I have no proof that this assumption is true. It might be wrong 
> in this case:
> 
>     X - a - b - c - Y
>   /           /
> o - d - e - f

In your example, git-merge-base X Y returns X. In fact, I could only get one merge base out of git-merge-base --all. I tried looking in the tests to find a case that produced multiple merge bases, but I was unable to find one. Is there an example floating around somewhere?

-Peff
Junio C Hamano· Sep 21, 2006, 06:43 UTC · re: Jeff King · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
>>     X - a - b - c - Y
>>   /           /
>> o - d - e - f
>
> In your example, git-merge-base X Y returns X. In fact, I could only get
> one merge base out of git-merge-base --all. I tried looking in the tests
> to find a case that produced multiple merge bases, but I was unable to
> find one. Is there an example floating around somewhere?

There are quite a few in git.git itself. In the recent history, this counts 42, the answer to everything.

#!/bin/sh
LF='
'
git rev-list --parents master..next |
while read it p1 p2 octo
do
	case "$p2" in '') continue ;; esac
        mb=`git merge-base --all "$p1" "$p2"`
	case "$mb" in ?*"$LF"?*) echo "$p1 $p2" : $mb ;;
        esac
done
Johannes Schindelin· Sep 21, 2006, 09:08 UTC · re: Junio C Hamano · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Hi,
On Wed, 20 Sep 2006, Junio C Hamano wrote:
Show 22 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> >>     X - a - b - c - Y
> >>   /           /
> >> o - d - e - f
> >
> > In your example, git-merge-base X Y returns X. In fact, I could only get
> > one merge base out of git-merge-base --all. I tried looking in the tests
> > to find a case that produced multiple merge bases, but I was unable to
> > find one. Is there an example floating around somewhere?
> 
> There are quite a few in git.git itself.  In the recent history,
> this counts 42, the answer to everything.
> 
> #!/bin/sh
> LF='
> '
> git rev-list --parents master..next |
> while read it p1 p2 octo
> do
> 	case "$p2" in '') continue ;; esac
>         mb=`git merge-base --all "$p1" "$p2"`
At this point you should also check if $p2 is in $mb.
> 	case "$mb" in ?*"$LF"?*) echo "$p1 $p2" : $mb ;;
>         esac
> done

The question was, if there is a fast-forward with more than one merge base, i.e. if the loop we jump^H^H^H^Hdo over bases is necessary.

And no, I am not aware of any such example.

Probably because X would mark all ancestors (_including_ all other possible merge bases) as uninteresting.

Therefore, I am convinced that there is no fast-forward returning more than one merge base. It is technically impossible. And thus, our loop is not necessary, but what the heck.

Ciao, Dscho

Junio C Hamano· Sep 21, 2006, 09:25 UTC · re: Johannes Schindelin · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
>> Jeff King <peff@peff.net> writes:
>> 
>> >>     X - a - b - c - Y
>> >>   /           /
>> >> o - d - e - f
>
> And no, I am not aware of any such example.
>
> Probably because X would mark all ancestors (_including_ all other 
> possible merge bases) as uninteresting.
True.  Paint me convinced, too.
Jeff King· Sep 20, 2006, 23:45 UTC · re: Johannes Schindelin · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

On Thu, Sep 21, 2006 at 01:07:54AM +0200, Johannes Schindelin wrote:
Show 14 quoted lines
> +	if (deny_non_fast_forwards && !is_null_sha1(old_sha1)) {
> +		struct commit *old_commit, *new_commit;
> +		struct commit_list *bases;
> +
> +		old_commit = (struct commit *)parse_object(old_sha1);
> +		new_commit = (struct commit *)parse_object(new_sha1);
> +		for (bases = get_merge_bases(old_commit, new_commit, 1);
> +				bases; bases = bases->next)
> +			if (!hashcmp(old_sha1, bases->item->object.sha1))
> +				break;
> +		if (!bases)
> +			return error("denying non-fast forward;"
> +					" you should pull first");
> +	}

Memory leak on 'bases'. It shouldn't matter much because the program is short-lived, but I couldn't remember if we have a policy on such things with increasing lib-ification.

-Peff
Johannes Schindelin· Sep 21, 2006, 00:07 UTC · re: Jeff King · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

On Wed, 20 Sep 2006, Jeff King wrote:
Show 20 quoted lines
> On Thu, Sep 21, 2006 at 01:07:54AM +0200, Johannes Schindelin wrote:
> 
> > +	if (deny_non_fast_forwards && !is_null_sha1(old_sha1)) {
> > +		struct commit *old_commit, *new_commit;
> > +		struct commit_list *bases;
> > +
> > +		old_commit = (struct commit *)parse_object(old_sha1);
> > +		new_commit = (struct commit *)parse_object(new_sha1);
> > +		for (bases = get_merge_bases(old_commit, new_commit, 1);
> > +				bases; bases = bases->next)
> > +			if (!hashcmp(old_sha1, bases->item->object.sha1))
> > +				break;
> > +		if (!bases)
> > +			return error("denying non-fast forward;"
> > +					" you should pull first");
> > +	}
> 
> Memory leak on 'bases'. It shouldn't matter much because the program is
> short-lived, but I couldn't remember if we have a policy on such things
> with increasing lib-ification.
True. How about this:
-- snip --
---
 receive-pack.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Show changes to receive-pack.c +4 −2
diff --git a/receive-pack.c b/receive-pack.c
index a6ec9f9..d84fc2c 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -131,17 +131,19 @@ static int update(struct command *cmd)
 	}
 	if (deny_non_fast_forwards && !is_null_sha1(old_sha1)) {
 		struct commit *old_commit, *new_commit;
-		struct commit_list *bases;
+		struct commit_list *bases, *backup;
 
 		old_commit = (struct commit *)parse_object(old_sha1);
 		new_commit = (struct commit *)parse_object(new_sha1);
-		for (bases = get_merge_bases(old_commit, new_commit, 1);
+		backup = get_merge_bases(old_commit, new_commit, 1);
+		for (bases = backup;
 				bases; bases = bases->next)
 			if (!hashcmp(old_sha1, bases->item->object.sha1))
 				break;
 		if (!bases)
 			return error("denying non-fast forward;"
 					" you should pull first");
+		free_commit_list(backup);
 	}
 	safe_create_leading_directories(lock_name);
 
-- 
1.4.2.1.g6ad2-dirty
Junio C Hamano· Sep 21, 2006, 05:35 UTC · re: Johannes Schindelin · lore

Re: [PATCH] add receive.denyNonFastforwards config variable

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 19 quoted lines
> On Wed, 20 Sep 2006, Jeff King wrote:
>
>> Memory leak on 'bases'. It shouldn't matter much because the program is
>> short-lived, but I couldn't remember if we have a policy on such things
>> with increasing lib-ification.
>
> True. How about this:
>
> -- snip --
>
> +		backup = get_merge_bases(old_commit, new_commit, 1);
> +		for (bases = backup;
>  				bases; bases = bases->next)
>  			if (!hashcmp(old_sha1, bases->item->object.sha1))
>  				break;
>  		if (!bases)
>  			return error("denying non-fast forward;"
>  					" you should pull first");
> +		free_commit_list(backup);
Still leaking on the error path ;-).
Will fixup and apply; no need to resend.

← back to recent threads