* [PATCH] add receive.denyNonFastforwards config variable
@ 2006-09-20 22:28 Johannes Schindelin
2006-09-20 22:44 ` Junio C Hamano
0 siblings, 1 reply; 18+ messages in thread
From: Johannes Schindelin @ 2006-09-20 22:28 UTC (permalink / raw)
To: Junio C Hamano, git, Shawn Pearce
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(-)
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-20 22:28 [PATCH] add receive.denyNonFastforwards config variable Johannes Schindelin
@ 2006-09-20 22:44 ` Junio C Hamano
2006-09-20 22:46 ` Shawn Pearce
2006-09-20 23:07 ` Johannes Schindelin
0 siblings, 2 replies; 18+ messages in thread
From: Junio C Hamano @ 2006-09-20 22:44 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Shawn Pearce
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.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-20 22:44 ` Junio C Hamano
@ 2006-09-20 22:46 ` Shawn Pearce
2006-09-20 23:07 ` Johannes Schindelin
1 sibling, 0 replies; 18+ messages in thread
From: Shawn Pearce @ 2006-09-20 22:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
Junio C Hamano <junkio@cox.net> wrote:
> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH] add receive.denyNonFastforwards config variable
2006-09-20 22:44 ` Junio C Hamano
2006-09-20 22:46 ` Shawn Pearce
@ 2006-09-20 23:07 ` Johannes Schindelin
2006-09-20 23:19 ` Junio C Hamano
` (2 more replies)
1 sibling, 3 replies; 18+ messages in thread
From: Johannes Schindelin @ 2006-09-20 23:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Shawn Pearce
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(-)
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-20 23:07 ` Johannes Schindelin
@ 2006-09-20 23:19 ` Junio C Hamano
2006-09-21 0:10 ` Johannes Schindelin
2006-09-20 23:38 ` Junio C Hamano
2006-09-20 23:45 ` Jeff King
2 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2006-09-20 23:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-20 23:19 ` Junio C Hamano
@ 2006-09-21 0:10 ` Johannes Schindelin
2006-09-21 6:26 ` Junio C Hamano
0 siblings, 1 reply; 18+ messages in thread
From: Johannes Schindelin @ 2006-09-21 0:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Hi,
On Wed, 20 Sep 2006, Junio C Hamano wrote:
> 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(-)
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-21 0:10 ` Johannes Schindelin
@ 2006-09-21 6:26 ` Junio C Hamano
2006-09-21 9:10 ` Johannes Schindelin
0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2006-09-21 6:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 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.
---
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 &&
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-21 6:26 ` Junio C Hamano
@ 2006-09-21 9:10 ` Johannes Schindelin
0 siblings, 0 replies; 18+ messages in thread
From: Johannes Schindelin @ 2006-09-21 9:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-20 23:07 ` Johannes Schindelin
2006-09-20 23:19 ` Junio C Hamano
@ 2006-09-20 23:38 ` Junio C Hamano
2006-09-21 0:17 ` Johannes Schindelin
2006-09-20 23:45 ` Jeff King
2 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2006-09-20 23:38 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
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?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-20 23:38 ` Junio C Hamano
@ 2006-09-21 0:17 ` Johannes Schindelin
2006-09-21 5:08 ` Junio C Hamano
2006-09-21 5:52 ` Jeff King
0 siblings, 2 replies; 18+ messages in thread
From: Johannes Schindelin @ 2006-09-21 0:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Hi,
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") &&
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.
>... 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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-21 0:17 ` Johannes Schindelin
@ 2006-09-21 5:08 ` Junio C Hamano
2006-09-21 5:52 ` Jeff King
1 sibling, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2006-09-21 5:08 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 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.
> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-21 0:17 ` Johannes Schindelin
2006-09-21 5:08 ` Junio C Hamano
@ 2006-09-21 5:52 ` Jeff King
2006-09-21 6:43 ` Junio C Hamano
1 sibling, 1 reply; 18+ messages in thread
From: Jeff King @ 2006-09-21 5:52 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
On Thu, Sep 21, 2006 at 02:17:22AM +0200, Johannes Schindelin wrote:
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-21 5:52 ` Jeff King
@ 2006-09-21 6:43 ` Junio C Hamano
2006-09-21 9:08 ` Johannes Schindelin
0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2006-09-21 6:43 UTC (permalink / raw)
To: Jeff King; +Cc: git
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"`
case "$mb" in ?*"$LF"?*) echo "$p1 $p2" : $mb ;;
esac
done
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-21 6:43 ` Junio C Hamano
@ 2006-09-21 9:08 ` Johannes Schindelin
2006-09-21 9:25 ` Junio C Hamano
0 siblings, 1 reply; 18+ messages in thread
From: Johannes Schindelin @ 2006-09-21 9:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
Hi,
On Wed, 20 Sep 2006, Junio C Hamano wrote:
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-21 9:08 ` Johannes Schindelin
@ 2006-09-21 9:25 ` Junio C Hamano
0 siblings, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2006-09-21 9:25 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-20 23:07 ` Johannes Schindelin
2006-09-20 23:19 ` Junio C Hamano
2006-09-20 23:38 ` Junio C Hamano
@ 2006-09-20 23:45 ` Jeff King
2006-09-21 0:07 ` Johannes Schindelin
2 siblings, 1 reply; 18+ messages in thread
From: Jeff King @ 2006-09-20 23:45 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git, Shawn Pearce
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.
-Peff
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-20 23:45 ` Jeff King
@ 2006-09-21 0:07 ` Johannes Schindelin
2006-09-21 5:35 ` Junio C Hamano
0 siblings, 1 reply; 18+ messages in thread
From: Johannes Schindelin @ 2006-09-21 0:07 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Shawn Pearce
On Wed, 20 Sep 2006, Jeff King wrote:
> 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(-)
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] add receive.denyNonFastforwards config variable
2006-09-21 0:07 ` Johannes Schindelin
@ 2006-09-21 5:35 ` Junio C Hamano
0 siblings, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2006-09-21 5:35 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2006-09-21 9:25 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-09-20 22:28 [PATCH] add receive.denyNonFastforwards config variable Johannes Schindelin
2006-09-20 22:44 ` Junio C Hamano
2006-09-20 22:46 ` Shawn Pearce
2006-09-20 23:07 ` Johannes Schindelin
2006-09-20 23:19 ` Junio C Hamano
2006-09-21 0:10 ` Johannes Schindelin
2006-09-21 6:26 ` Junio C Hamano
2006-09-21 9:10 ` Johannes Schindelin
2006-09-20 23:38 ` Junio C Hamano
2006-09-21 0:17 ` Johannes Schindelin
2006-09-21 5:08 ` Junio C Hamano
2006-09-21 5:52 ` Jeff King
2006-09-21 6:43 ` Junio C Hamano
2006-09-21 9:08 ` Johannes Schindelin
2006-09-21 9:25 ` Junio C Hamano
2006-09-20 23:45 ` Jeff King
2006-09-21 0:07 ` Johannes Schindelin
2006-09-21 5:35 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox