* Re: Finding a tag that introduced a submodule change
From: Junio C Hamano @ 2017-03-03 18:04 UTC (permalink / raw)
To: Robert Dailey; +Cc: Git
In-Reply-To: <CAHd499CfJnPtLmi8qzr=_jrfCgMw85MOUv-wPKmAHFUyDFXhRA@mail.gmail.com>
Robert Dailey <rcdailey.lists@gmail.com> writes:
> Sometimes I run into a situation where I need to find out which
> release of the product a submodule change was introduced in. This is
> nontrivial, since there are no tags in the submodule itself.
Does your superproject rewind the commit in the submodule project as
it goes forward? That is, is this property guaranteed to hold by
your project's discipline:
Given any two commits C1 and C2 in the superproject, and the
commit in the submodule bound to C1's and C2's tree (call
them S1 and S2, respectively), if C1 is an ancestor of C2,
then S1 is the same as S2 or an ancestor of S2.
If so, I think you can do a bisection of the history in the
superproject. Pick an old commit in the superproject that binds an
old commit from the submodule that does not have the change and call
it "good". Similarly pick a new one in the superproject that binds
a newer commit from the submodule that does have the change, and
call it "bad". Then do
$ git bisect start $bad $good -- $path_to_submodule
which would suggest you to test commits that change what commit is
bound at the submodule's path.
When testing each of these commits, you would see if the commit
bound at the submodule's path has the change or not.
$ current=$(git rev-parse HEAD:$path_to_submodule)
would give you the object name of that commit, and then
$ git -C $path_to_submodule merge-base --is-ancestor $change $current
would tell you if the $change you are interested in is already
contained in that $current commit. Then you say "git bisect good"
if $current is too old to contain the $change, and "git bisect bad"
if $current is sufficiently new and contains the $change, to
continue.
If your superproject rewinds the commit in the submodule as it goes
forward, e.g. an older commit in the superproject used submodule
commit from day X, but somebody who made yesterday's commit in the
superproject realized that that submodule commit was broken and used
an older commit in the submodule from day (X-1), then you cannot
bisect. In such a case, I think you would essentially need to check
all superproject commits that changed the commit bound at the
submodule's path.
$ git rev-list $bad..$good -- $path_to_submodule
would give a list of such commits, and you would do the "merge-base"
check for all them to see which ones have and do not have the
$change (replace "HEAD" with the commit you are testing in the
computation that gives you $current).
^ permalink raw reply
* Re: [PATCH 1/3] Add --gui option to mergetool
From: Rémi Galan Alfonso @ 2017-03-03 18:06 UTC (permalink / raw)
To: Denton Liu, git; +Cc: davvid, Johannes.Schindelin
In-Reply-To: <20170303115738.GA13211@arch-attack.localdomain>
Hi,
Denton Liu <liu.denton@gmail.com> writes:
> [...]
>
> +test_expect_success 'gui mergetool' '
> + test_when_finished "git reset --hard" &&
> + test_when_finished "git config merge.tool mytool" &&
> + test_when_finished "git config --unset merge.guitool" &&
> + git config merge.tool badtool &&
> + git config merge.guitool mytool &&
You should be able to squash the lines
`test_when_finished "git config --unset merge.guitool" &&`
and
`git config merge.guitool mytool &&`
into
`test_config merge.guitool mytool`
(It is however not possible with merge.tool since you set it to a
specific value 'when_finished')
Thanks,
Rémi
^ permalink raw reply
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Junio C Hamano @ 2017-03-03 17:47 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Mike Crowe, git, Jeff King
In-Reply-To: <ae2b144a-5e39-8178-5161-1d8eb673b6f0@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> Understood, thanks for the explanation.
>
> quiet is not quite any more..
>
> Does the following fix help ?
>
> --- a/diff.c
> +++ b/diff.c
> @@ -2826,6 +2826,8 @@ int diff_populate_filespec(struct diff_filespec *s,
> unsigned int flags)
> enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
> ? SAFE_CRLF_WARN
> : safe_crlf);
> + if (size_only)
> + crlf_warn = SAFE_CRLF_FALSE;
If you were to go this route, it may be sufficient to change its
initialization from WARN to FALSE _unconditionally_, because this
function uses the convert_to_git() only to _show_ the differences by
computing canonical form out of working tree contents, and the
conversion is not done to _write_ into object database to create a
new object.
Having size_only here is not a sign of getting --quiet passed from
the command line, by the way.
^ permalink raw reply
* Re: Finding a tag that introduced a submodule change
From: Jacob Keller @ 2017-03-03 16:39 UTC (permalink / raw)
To: Robert Dailey; +Cc: Git
In-Reply-To: <CAHd499CfJnPtLmi8qzr=_jrfCgMw85MOUv-wPKmAHFUyDFXhRA@mail.gmail.com>
On Fri, Mar 3, 2017 at 7:40 AM, Robert Dailey <rcdailey.lists@gmail.com> wrote:
> I have a repository with a single submodule in it. Since the parent
> repository represents the code base for an actual product, I tag
> release versions in the parent repository. I do not put tags in the
> submodule since multiple other products may be using it there and I
> wanted to avoid ambiguous tags.
>
Hi,
I agree you shouldn't use tags in the submodules.
> Sometimes I run into a situation where I need to find out which
> release of the product a submodule change was introduced in. This is
> nontrivial, since there are no tags in the submodule itself. This is
> one thing I tried:
>
I've run into this exact problem at $DAYJOB.
> 1. Do a `git log` in the submodule to find the SHA1 representing the
> change I want to check for
> 2. In the parent repository, do a git log with pickaxe to determine
> when the submodule itself changed to the value of that SHA1.
> 3. Based on the result of #2, do a `git tag --contains` to see the
> lowest-version tag that contains the SHA1, which will identify the
> first release that introduced that change
>
> However, I was not able to get past #2 because apparently there are
> cases where when we move the submodule "forward", we skip over
> commits, so the value of the submodule itself never was set to that
> SHA1.
>
> I'm at a loss here on how to easily do this. Can someone recommend a
> way to do this? Obviously the easier the better, as I have to somehow
> train my team how to do this on their own.
>
> Thanks in advance.
So there's better ways to do this, but I do think there would be value
in adding some plumbing to make it easier.
Here is how I would do this, best if written into a shell script or
similar to automate the tricky part of #2
1. Do a git-log of the *parent* project, filtering out to show only
the path to the submodule
2. For each commit here, you find the new and old values of the
submodule pointer.
3. Use git merge-base --is-ancestor to ensure that "old" is an
ancestor of "submodule sha1id" and then
4. Use git-merge-base to ensure that "submodule sha1id" is an
anscestor of "new".
If both these are tree, then you know that the commit was included
into the parent project at this point.
I've had to do this once or twice, but I don't actually remember
exactly how I did 3. One sneaky way would be to add new tags for each
submodule change something like the following might work and be more
efficient. I'm not really sure but here's how I would go that route:
1. git log <limiting revision selection if you dont' want the entire
history> <path-to-submodule> --pretty=%h | parallel git ls-tree {}
path-to-submodule
The above more or less prints every submodule value as it changed over
time in the parent project.
Next, for each submodule change:
2. git -C <submodule> tag parent/<sha1id> <submodule change>
Create a new tag prefixed by "parent" that includes the sha1id of the
parent commit, and create it inside the submodule
3. git -C submodule describe --contains --match="parent/*" <submodule sha1id>
Once you're done you can also delete all the tags that are in the
"parent" prefix if you dont' really wanna see them again.
Basically, re-use the machinery to tag and then use describe
--contains to find the commit.
I *really* think a similar algorithm could be embedded as a plumbing
subcommand, since I think this is tedious to do by hand.
I'm not really sure if this is the "best" algorithm either, but it's
pretty much what I've used in the past. Either the tag way or the log
yourself one at a time way.
^ permalink raw reply
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Torsten Bögershausen @ 2017-03-03 17:02 UTC (permalink / raw)
To: Mike Crowe, Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <20170302200356.GA31318@mcrowe.com>
Understood, thanks for the explanation.
quiet is not quite any more..
Does the following fix help ?
--- a/diff.c
+++ b/diff.c
@@ -2826,6 +2826,8 @@ int diff_populate_filespec(struct diff_filespec *s,
unsigned int flags)
enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
? SAFE_CRLF_WARN
: safe_crlf);
+ if (size_only)
+ crlf_warn = SAFE_CRLF_FALSE;
^ permalink raw reply
* [PATCH v3 8/9] read_early_config(): really discover .git/
From: Johannes Schindelin @ 2017-03-03 17:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488562287.git.johannes.schindelin@gmx.de>
Earlier, we punted and simply assumed that we are in the top-level
directory of the project, and that there is no .git file but a .git/
directory so that we can read directly from .git/config.
However, that is not necessarily true. We may be in a subdirectory. Or
.git may be a gitfile. Or the environment variable GIT_DIR may be set.
To remedy this situation, we just refactored the way
setup_git_directory() discovers the .git/ directory, to make it
reusable, and more importantly, to leave all global variables and the
current working directory alone.
Let's discover the .git/ directory correctly in read_early_config() by
using that new function.
This fixes 4 known breakages in t7006.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.c | 33 ++++++++++++++-------------------
setup.c | 14 +++++++++++++-
| 8 ++++----
3 files changed, 31 insertions(+), 24 deletions(-)
diff --git a/config.c b/config.c
index 068fa4dcfa6..749623a9649 100644
--- a/config.c
+++ b/config.c
@@ -1414,34 +1414,29 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
void read_early_config(config_fn_t cb, void *data)
{
+ struct strbuf buf = STRBUF_INIT;
+
git_config_with_options(cb, data, NULL, 1);
/*
- * Note that this is a really dirty hack that does the wrong thing in
- * many cases. The crux of the problem is that we cannot run
- * setup_git_directory() early on in git's setup, so we have no idea if
- * we are in a repository or not, and therefore are not sure whether
- * and how to read repository-local config.
- *
- * So if we _aren't_ in a repository (or we are but we would reject its
- * core.repositoryformatversion), we'll read whatever is in .git/config
- * blindly. Similarly, if we _are_ in a repository, but not at the
- * root, we'll fail to find .git/config (because it's really
- * ../.git/config, etc), unless setup_git_directory() was already called.
- * See t7006 for a complete set of failures.
- *
- * However, we have historically provided this hack because it does
- * work some of the time (namely when you are at the top-level of a
- * valid repository), and would rarely make things worse (i.e., you do
- * not generally have a .git/config file sitting around).
+ * When we are not about to create a repository ourselves (init or
+ * clone) and when no .git/ directory was set up yet (in which case
+ * git_config_with_options() would already have picked up the
+ * repository config), we ask discover_git_directory() to figure out
+ * whether there is any repository config we should use (but unlike
+ * setup_git_directory_gently(), no global state is changed, most
+ * notably, the current working directory is still the same after
+ * the call).
*/
- if (!have_git_dir()) {
+ if (!have_git_dir() && discover_git_directory(&buf)) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
- repo_config.file = ".git/config";
+ strbuf_addstr(&buf, "/config");
+ repo_config.file = buf.buf;
git_config_with_options(cb, data, &repo_config, 1);
}
+ strbuf_release(&buf);
}
static void git_config_check_init(void);
diff --git a/setup.c b/setup.c
index 32ce023638a..5320ae37314 100644
--- a/setup.c
+++ b/setup.c
@@ -925,8 +925,9 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
const char *discover_git_directory(struct strbuf *gitdir)
{
- struct strbuf dir = STRBUF_INIT;
+ struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
size_t gitdir_offset = gitdir->len, cwd_len;
+ struct repository_format candidate;
if (strbuf_getcwd(&dir))
return NULL;
@@ -949,8 +950,19 @@ const char *discover_git_directory(struct strbuf *gitdir)
strbuf_addch(&dir, '/');
strbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);
}
+
+ strbuf_reset(&dir);
+ strbuf_addf(&dir, "%s/config", gitdir->buf + gitdir_offset);
+ read_repository_format(&candidate, dir.buf);
strbuf_release(&dir);
+ if (verify_repository_format(&candidate, &err) < 0) {
+ warning("ignoring git dir '%s': %s",
+ gitdir->buf + gitdir_offset, err.buf);
+ strbuf_release(&err);
+ return NULL;
+ }
+
return gitdir->buf;
}
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 304ae06c600..4f3794d415e 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -360,19 +360,19 @@ test_pager_choices 'git aliasedlog'
test_default_pager expect_success 'git -p aliasedlog'
test_PAGER_overrides expect_success 'git -p aliasedlog'
test_core_pager_overrides expect_success 'git -p aliasedlog'
-test_core_pager_subdir expect_failure 'git -p aliasedlog'
+test_core_pager_subdir expect_success 'git -p aliasedlog'
test_GIT_PAGER_overrides expect_success 'git -p aliasedlog'
test_default_pager expect_success 'git -p true'
test_PAGER_overrides expect_success 'git -p true'
test_core_pager_overrides expect_success 'git -p true'
-test_core_pager_subdir expect_failure 'git -p true'
+test_core_pager_subdir expect_success 'git -p true'
test_GIT_PAGER_overrides expect_success 'git -p true'
test_default_pager expect_success test_must_fail 'git -p request-pull'
test_PAGER_overrides expect_success test_must_fail 'git -p request-pull'
test_core_pager_overrides expect_success test_must_fail 'git -p request-pull'
-test_core_pager_subdir expect_failure test_must_fail 'git -p request-pull'
+test_core_pager_subdir expect_success test_must_fail 'git -p request-pull'
test_GIT_PAGER_overrides expect_success test_must_fail 'git -p request-pull'
test_default_pager expect_success test_must_fail 'git -p'
@@ -380,7 +380,7 @@ test_PAGER_overrides expect_success test_must_fail 'git -p'
test_local_config_ignored expect_failure test_must_fail 'git -p'
test_GIT_PAGER_overrides expect_success test_must_fail 'git -p'
-test_expect_failure TTY 'core.pager in repo config works and retains cwd' '
+test_expect_success TTY 'core.pager in repo config works and retains cwd' '
sane_unset GIT_PAGER &&
test_config core.pager "cat >cwd-retained" &&
(
--
2.12.0.windows.1.7.g94dafc3b124
^ permalink raw reply related
* [PATCH v3 9/9] Test read_early_config()
From: Johannes Schindelin @ 2017-03-03 17:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488562287.git.johannes.schindelin@gmx.de>
So far, we had no explicit tests of that function.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/helper/test-config.c | 15 +++++++++++++++
t/t1309-early-config.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
create mode 100755 t/t1309-early-config.sh
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 83a4f2ab869..8e3ed6a76cb 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -66,6 +66,16 @@ static int iterate_cb(const char *var, const char *value, void *data)
return 0;
}
+static int early_config_cb(const char *var, const char *value, void *vdata)
+{
+ const char *key = vdata;
+
+ if (!strcmp(key, var))
+ printf("%s\n", value);
+
+ return 0;
+}
+
int cmd_main(int argc, const char **argv)
{
int i, val;
@@ -73,6 +83,11 @@ int cmd_main(int argc, const char **argv)
const struct string_list *strptr;
struct config_set cs;
+ if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
+ read_early_config(early_config_cb, (void *)argv[2]);
+ return 0;
+ }
+
setup_git_directory();
git_configset_init(&cs);
diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
new file mode 100755
index 00000000000..0c55dee514c
--- /dev/null
+++ b/t/t1309-early-config.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='Test read_early_config()'
+
+. ./test-lib.sh
+
+test_expect_success 'read early config' '
+ test_config early.config correct &&
+ test-config read_early_config early.config >output &&
+ test correct = "$(cat output)"
+'
+
+test_expect_success 'in a sub-directory' '
+ test_config early.config sub &&
+ mkdir -p sub &&
+ (
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test sub = "$(cat output)"
+'
+
+test_expect_success 'ceiling' '
+ test_config early.config ceiling &&
+ mkdir -p sub &&
+ (
+ GIT_CEILING_DIRECTORIES="$PWD" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test -z "$(cat output)"
+'
+
+test_expect_success 'ceiling #2' '
+ mkdir -p xdg/git &&
+ git config -f xdg/git/config early.config xdg &&
+ test_config early.config ceiling &&
+ mkdir -p sub &&
+ (
+ XDG_CONFIG_HOME="$PWD"/xdg &&
+ GIT_CEILING_DIRECTORIES="$PWD" &&
+ export GIT_CEILING_DIRECTORIES XDG_CONFIG_HOME &&
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test xdg = "$(cat output)"
+'
+
+test_done
--
2.12.0.windows.1.7.g94dafc3b124
^ permalink raw reply related
* [PATCH v3 6/9] Make read_early_config() reusable
From: Johannes Schindelin @ 2017-03-03 17:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488562287.git.johannes.schindelin@gmx.de>
The pager configuration needs to be read early, possibly before
discovering any .git/ directory.
Let's not hide this function in pager.c, but make it available to other
callers.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
cache.h | 1 +
config.c | 31 +++++++++++++++++++++++++++++++
| 31 -------------------------------
3 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/cache.h b/cache.h
index 4d8eb38de74..8a4580f921d 100644
--- a/cache.h
+++ b/cache.h
@@ -1799,6 +1799,7 @@ extern int git_config_from_blob_sha1(config_fn_t fn, const char *name,
const unsigned char *sha1, void *data);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
+extern void read_early_config(config_fn_t cb, void *data);
extern void git_config(config_fn_t fn, void *);
extern int git_config_with_options(config_fn_t fn, void *,
struct git_config_source *config_source,
diff --git a/config.c b/config.c
index c6b874a7bf7..9cfbeafd04c 100644
--- a/config.c
+++ b/config.c
@@ -1412,6 +1412,37 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
}
}
+void read_early_config(config_fn_t cb, void *data)
+{
+ git_config_with_options(cb, data, NULL, 1);
+
+ /*
+ * Note that this is a really dirty hack that does the wrong thing in
+ * many cases. The crux of the problem is that we cannot run
+ * setup_git_directory() early on in git's setup, so we have no idea if
+ * we are in a repository or not, and therefore are not sure whether
+ * and how to read repository-local config.
+ *
+ * So if we _aren't_ in a repository (or we are but we would reject its
+ * core.repositoryformatversion), we'll read whatever is in .git/config
+ * blindly. Similarly, if we _are_ in a repository, but not at the
+ * root, we'll fail to find .git/config (because it's really
+ * ../.git/config, etc). See t7006 for a complete set of failures.
+ *
+ * However, we have historically provided this hack because it does
+ * work some of the time (namely when you are at the top-level of a
+ * valid repository), and would rarely make things worse (i.e., you do
+ * not generally have a .git/config file sitting around).
+ */
+ if (!startup_info->have_repository) {
+ struct git_config_source repo_config;
+
+ memset(&repo_config, 0, sizeof(repo_config));
+ repo_config.file = ".git/config";
+ git_config_with_options(cb, data, &repo_config, 1);
+ }
+}
+
static void git_config_check_init(void);
void git_config(config_fn_t fn, void *data)
--git a/pager.c b/pager.c
index ae796433630..73ca8bc3b17 100644
--- a/pager.c
+++ b/pager.c
@@ -43,37 +43,6 @@ static int core_pager_config(const char *var, const char *value, void *data)
return 0;
}
-static void read_early_config(config_fn_t cb, void *data)
-{
- git_config_with_options(cb, data, NULL, 1);
-
- /*
- * Note that this is a really dirty hack that does the wrong thing in
- * many cases. The crux of the problem is that we cannot run
- * setup_git_directory() early on in git's setup, so we have no idea if
- * we are in a repository or not, and therefore are not sure whether
- * and how to read repository-local config.
- *
- * So if we _aren't_ in a repository (or we are but we would reject its
- * core.repositoryformatversion), we'll read whatever is in .git/config
- * blindly. Similarly, if we _are_ in a repository, but not at the
- * root, we'll fail to find .git/config (because it's really
- * ../.git/config, etc). See t7006 for a complete set of failures.
- *
- * However, we have historically provided this hack because it does
- * work some of the time (namely when you are at the top-level of a
- * valid repository), and would rarely make things worse (i.e., you do
- * not generally have a .git/config file sitting around).
- */
- if (!startup_info->have_repository) {
- struct git_config_source repo_config;
-
- memset(&repo_config, 0, sizeof(repo_config));
- repo_config.file = ".git/config";
- git_config_with_options(cb, data, &repo_config, 1);
- }
-}
-
const char *git_pager(int stdout_is_tty)
{
const char *pager;
--
2.12.0.windows.1.7.g94dafc3b124
^ permalink raw reply related
* [PATCH v3 5/9] Export the discover_git_directory() function
From: Johannes Schindelin @ 2017-03-03 17:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488562287.git.johannes.schindelin@gmx.de>
The setup_git_directory_gently_1() function we modified earlier still
needs to return both the path to the .git/ directory as well as the
"cd-up" path to allow setup_git_directory() to retain its previous
behavior as if it changed the current working directory on its quest for
the .git/ directory.
This is a bit cumbersome to use if you only need to figure out the
(possibly absolute) path of the .git/ directory. Let's just provide
a convenient wrapper function with an easier signature that *just*
discovers the .git/ directory.
We will use it in a subsequent patch to support early config reading
better.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
cache.h | 2 ++
setup.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/cache.h b/cache.h
index 80b6372cf76..4d8eb38de74 100644
--- a/cache.h
+++ b/cache.h
@@ -518,6 +518,8 @@ extern void set_git_work_tree(const char *tree);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
extern void setup_work_tree(void);
+/* Find GIT_DIR without changing the working directory or other global state */
+extern const char *discover_git_directory(struct strbuf *gitdir);
extern const char *setup_git_directory_gently(int *);
extern const char *setup_git_directory(void);
extern char *prefix_path(const char *prefix, int len, const char *path);
diff --git a/setup.c b/setup.c
index 9a09bb41ab5..32ce023638a 100644
--- a/setup.c
+++ b/setup.c
@@ -923,6 +923,37 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
}
}
+const char *discover_git_directory(struct strbuf *gitdir)
+{
+ struct strbuf dir = STRBUF_INIT;
+ size_t gitdir_offset = gitdir->len, cwd_len;
+
+ if (strbuf_getcwd(&dir))
+ return NULL;
+
+ cwd_len = dir.len;
+ if (setup_git_directory_gently_1(&dir, gitdir) < 0) {
+ strbuf_release(&dir);
+ return NULL;
+ }
+
+ /*
+ * The returned gitdir is relative to dir, and if dir does not reflect
+ * the current working directory, we simply make the gitdir absolute.
+ */
+ if (dir.len < cwd_len && !is_absolute_path(gitdir->buf + gitdir_offset)) {
+ /* Avoid a trailing "/." */
+ if (!strcmp(".", gitdir->buf + gitdir_offset))
+ strbuf_setlen(gitdir, gitdir_offset);
+ else
+ strbuf_addch(&dir, '/');
+ strbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);
+ }
+ strbuf_release(&dir);
+
+ return gitdir->buf;
+}
+
const char *setup_git_directory_gently(int *nongit_ok)
{
struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir = STRBUF_INIT;
--
2.12.0.windows.1.7.g94dafc3b124
^ permalink raw reply related
* [PATCH v3 7/9] read_early_config(): avoid .git/config hack when unneeded
From: Johannes Schindelin @ 2017-03-03 17:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488562287.git.johannes.schindelin@gmx.de>
So far, we only look whether the startup_info claims to have seen a
git_dir.
However, do_git_config_sequence() (and consequently the
git_config_with_options() call used by read_early_config() asks the
have_git_dir() function whether we have a .git/ directory, which in turn
also looks at git_dir and at the environment variable GIT_DIR. And when
this is the case, the repository config is handled already, so we do not
have to do that again explicitly.
Let's just use the same function, have_git_dir(), to determine whether we
have to handle .git/config explicitly.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 9cfbeafd04c..068fa4dcfa6 100644
--- a/config.c
+++ b/config.c
@@ -1427,14 +1427,15 @@ void read_early_config(config_fn_t cb, void *data)
* core.repositoryformatversion), we'll read whatever is in .git/config
* blindly. Similarly, if we _are_ in a repository, but not at the
* root, we'll fail to find .git/config (because it's really
- * ../.git/config, etc). See t7006 for a complete set of failures.
+ * ../.git/config, etc), unless setup_git_directory() was already called.
+ * See t7006 for a complete set of failures.
*
* However, we have historically provided this hack because it does
* work some of the time (namely when you are at the top-level of a
* valid repository), and would rarely make things worse (i.e., you do
* not generally have a .git/config file sitting around).
*/
- if (!startup_info->have_repository) {
+ if (!have_git_dir()) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
--
2.12.0.windows.1.7.g94dafc3b124
^ permalink raw reply related
* [PATCH v3 4/9] setup_git_directory_1(): avoid changing global state
From: Johannes Schindelin @ 2017-03-03 17:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488562287.git.johannes.schindelin@gmx.de>
For historical reasons, Git searches for the .git/ directory (or the
.git file) by changing the working directory successively to the parent
directory of the current directory, until either anything was found or
until a ceiling or a mount point is hit.
Further global state may be changed in case a .git/ directory was found.
We do have a use case, though, where we would like to find the .git/
directory without having any global state touched, though: when we read
the early config e.g. for the pager or for alias expansion.
Let's just move all of code that changes any global state out of the
function `setup_git_directory_gently_1()` into
`setup_git_directory_gently()`.
In subsequent patches, we will use the _1() function in a new
`discover_git_directory()` function that we will then use for the early
config code.
Note: the new loop is a *little* tricky, as we have to handle the root
directory specially: we cannot simply strip away the last component
including the slash, as the root directory only has that slash. To remedy
that, we introduce the `min_offset` variable that holds the minimal length
of an absolute path, and using that to special-case the root directory,
including an early exit before trying to find the parent of the root
directory.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
setup.c | 189 ++++++++++++++++++++++++++++++++++++++--------------------------
1 file changed, 113 insertions(+), 76 deletions(-)
diff --git a/setup.c b/setup.c
index 91d884b6746..9a09bb41ab5 100644
--- a/setup.c
+++ b/setup.c
@@ -818,50 +818,49 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
}
}
+enum discovery_result {
+ GIT_DIR_NONE = 0,
+ GIT_DIR_EXPLICIT,
+ GIT_DIR_DISCOVERED,
+ GIT_DIR_BARE,
+ /* these are errors */
+ GIT_DIR_HIT_CEILING = -1,
+ GIT_DIR_HIT_MOUNT_POINT = -2
+};
+
/*
* We cannot decide in this function whether we are in the work tree or
* not, since the config can only be read _after_ this function was called.
+ *
+ * Also, we avoid changing any global state (such as the current working
+ * directory) to allow early callers.
+ *
+ * The directory where the search should start needs to be passed in via the
+ * `dir` parameter; upon return, the `dir` buffer will contain the path of
+ * the directory where the search ended, and `gitdir` will contain the path of
+ * the discovered .git/ directory, if any. This path may be relative against
+ * `dir` (i.e. *not* necessarily the cwd).
*/
-static const char *setup_git_directory_gently_1(int *nongit_ok)
+static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
+ struct strbuf *gitdir)
{
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
- static struct strbuf cwd = STRBUF_INIT;
- const char *gitdirenv, *ret;
- char *gitfile;
- int offset, offset_parent, ceil_offset = -1;
+ const char *gitdirenv;
+ int ceil_offset = -1, min_offset = has_dos_drive_prefix(dir->buf) ? 3 : 1;
dev_t current_device = 0;
int one_filesystem = 1;
/*
- * We may have read an incomplete configuration before
- * setting-up the git directory. If so, clear the cache so
- * that the next queries to the configuration reload complete
- * configuration (including the per-repo config file that we
- * ignored previously).
- */
- git_config_clear();
-
- /*
- * Let's assume that we are in a git repository.
- * If it turns out later that we are somewhere else, the value will be
- * updated accordingly.
- */
- if (nongit_ok)
- *nongit_ok = 0;
-
- if (strbuf_getcwd(&cwd))
- die_errno(_("Unable to read current working directory"));
- offset = cwd.len;
-
- /*
* If GIT_DIR is set explicitly, we're not going
* to do any discovery, but we still do repository
* validation.
*/
gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
- if (gitdirenv)
- return setup_explicit_git_dir(gitdirenv, &cwd, nongit_ok);
+ if (gitdirenv) {
+ strbuf_addstr(gitdir, gitdirenv);
+ return GIT_DIR_EXPLICIT;
+ }
if (env_ceiling_dirs) {
int empty_entry_found = 0;
@@ -869,15 +868,15 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
filter_string_list(&ceiling_dirs, 0,
canonicalize_ceiling_entry, &empty_entry_found);
- ceil_offset = longest_ancestor_length(cwd.buf, &ceiling_dirs);
+ ceil_offset = longest_ancestor_length(dir->buf, &ceiling_dirs);
string_list_clear(&ceiling_dirs, 0);
}
- if (ceil_offset < 0 && has_dos_drive_prefix(cwd.buf))
- ceil_offset = 1;
+ if (ceil_offset < 0)
+ ceil_offset = min_offset - 2;
/*
- * Test in the following order (relative to the cwd):
+ * Test in the following order (relative to the dir):
* - .git (file containing "gitdir: <path>")
* - .git/
* - ./ (bare)
@@ -889,62 +888,100 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
*/
one_filesystem = !git_env_bool("GIT_DISCOVERY_ACROSS_FILESYSTEM", 0);
if (one_filesystem)
- current_device = get_device_or_die(".", NULL, 0);
+ current_device = get_device_or_die(dir->buf, NULL, 0);
for (;;) {
- gitfile = (char*)read_gitfile(DEFAULT_GIT_DIR_ENVIRONMENT);
- if (gitfile)
- gitdirenv = gitfile = xstrdup(gitfile);
- else {
- if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
- gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
- }
-
+ int offset = dir->len;
+
+ if (offset > min_offset)
+ strbuf_addch(dir, '/');
+ strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
+ gitdirenv = read_gitfile(dir->buf);
+ if (!gitdirenv && is_git_directory(dir->buf))
+ gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
+ strbuf_setlen(dir, offset);
if (gitdirenv) {
- ret = setup_discovered_git_dir(gitdirenv,
- &cwd, offset,
- nongit_ok);
- free(gitfile);
- return ret;
+ strbuf_addstr(gitdir, gitdirenv);
+ return GIT_DIR_DISCOVERED;
}
- free(gitfile);
- if (is_git_directory("."))
- return setup_bare_git_dir(&cwd, offset, nongit_ok);
-
- offset_parent = offset;
- while (--offset_parent > ceil_offset &&
- !is_dir_sep(cwd.buf[offset_parent]));
- if (offset_parent <= ceil_offset)
- return setup_nongit(cwd.buf, nongit_ok);
- if (one_filesystem) {
- dev_t parent_device = get_device_or_die("..", cwd.buf,
- offset);
- if (parent_device != current_device) {
- if (nongit_ok) {
- if (chdir(cwd.buf))
- die_errno(_("Cannot come back to cwd"));
- *nongit_ok = 1;
- return NULL;
- }
- strbuf_setlen(&cwd, offset);
- die(_("Not a git repository (or any parent up to mount point %s)\n"
- "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
- cwd.buf);
- }
- }
- if (chdir("..")) {
- strbuf_setlen(&cwd, offset);
- die_errno(_("Cannot change to '%s/..'"), cwd.buf);
+ if (is_git_directory(dir->buf)) {
+ strbuf_addstr(gitdir, ".");
+ return GIT_DIR_BARE;
}
- offset = offset_parent;
+
+ if (offset <= min_offset)
+ return GIT_DIR_HIT_CEILING;
+
+ while (--offset > ceil_offset && !is_dir_sep(dir->buf[offset]));
+ if (offset <= ceil_offset)
+ return GIT_DIR_HIT_CEILING;
+
+ strbuf_setlen(dir, offset > min_offset ? offset : min_offset);
+ if (one_filesystem &&
+ current_device != get_device_or_die(dir->buf, NULL, offset))
+ return GIT_DIR_HIT_MOUNT_POINT;
}
}
const char *setup_git_directory_gently(int *nongit_ok)
{
+ struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir = STRBUF_INIT;
const char *prefix;
- prefix = setup_git_directory_gently_1(nongit_ok);
+ /*
+ * We may have read an incomplete configuration before
+ * setting-up the git directory. If so, clear the cache so
+ * that the next queries to the configuration reload complete
+ * configuration (including the per-repo config file that we
+ * ignored previously).
+ */
+ git_config_clear();
+
+ /*
+ * Let's assume that we are in a git repository.
+ * If it turns out later that we are somewhere else, the value will be
+ * updated accordingly.
+ */
+ if (nongit_ok)
+ *nongit_ok = 0;
+
+ if (strbuf_getcwd(&cwd))
+ die_errno(_("Unable to read current working directory"));
+ strbuf_addbuf(&dir, &cwd);
+
+ switch (setup_git_directory_gently_1(&dir, &gitdir)) {
+ case GIT_DIR_NONE:
+ prefix = NULL;
+ break;
+ case GIT_DIR_EXPLICIT:
+ prefix = setup_explicit_git_dir(gitdir.buf, &cwd, nongit_ok);
+ break;
+ case GIT_DIR_DISCOVERED:
+ if (dir.len < cwd.len && chdir(dir.buf))
+ die(_("Cannot change to '%s'"), dir.buf);
+ prefix = setup_discovered_git_dir(gitdir.buf, &cwd, dir.len,
+ nongit_ok);
+ break;
+ case GIT_DIR_BARE:
+ if (dir.len < cwd.len && chdir(dir.buf))
+ die(_("Cannot change to '%s'"), dir.buf);
+ prefix = setup_bare_git_dir(&cwd, dir.len, nongit_ok);
+ break;
+ case GIT_DIR_HIT_CEILING:
+ prefix = setup_nongit(cwd.buf, nongit_ok);
+ break;
+ case GIT_DIR_HIT_MOUNT_POINT:
+ if (nongit_ok) {
+ *nongit_ok = 1;
+ return NULL;
+ }
+ die(_("Not a git repository (or any parent up to mount point %s)\n"
+ "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
+ dir.buf);
+ default:
+ die("BUG: unhandled setup_git_directory_1() result");
+ }
+
if (prefix)
setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
else
--
2.12.0.windows.1.7.g94dafc3b124
^ permalink raw reply related
* [PATCH v3 3/9] Prepare setup_discovered_git_directory() the root directory
From: Johannes Schindelin @ 2017-03-03 17:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488562287.git.johannes.schindelin@gmx.de>
Currently, the offset parameter (indicating what part of the cwd
parameter corresponds to the current directory after discovering the
.git/ directory) is set to 0 when we are running in the root directory.
However, in the next patches we will avoid changing the current working
directory while searching for the .git/ directory, meaning that the
offset corresponding to the root directory will have to be 1 to reflect
that this directory is characterized by the path "/" (and not "").
So let's make sure that setup_discovered_git_directory() only tries to
append the trailing slash to non-root directories.
Note: the setup_bare_git_directory() does not need a corresponding
change, as it does not want to return a prefix.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
setup.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/setup.c b/setup.c
index c47619605fb..91d884b6746 100644
--- a/setup.c
+++ b/setup.c
@@ -721,8 +721,10 @@ static const char *setup_discovered_git_dir(const char *gitdir,
if (offset == cwd->len)
return NULL;
- /* Make "offset" point to past the '/', and add a '/' at the end */
- offset++;
+ /* Make "offset" point past the '/' (already the case for root dirs) */
+ if (offset != offset_1st_component(cwd->buf))
+ offset++;
+ /* Add a '/' at the end */
strbuf_addch(cwd, '/');
return cwd->buf + offset;
}
--
2.12.0.windows.1.7.g94dafc3b124
^ permalink raw reply related
* [PATCH v3 2/9] setup_git_directory(): use is_dir_sep() helper
From: Johannes Schindelin @ 2017-03-03 17:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488562287.git.johannes.schindelin@gmx.de>
It is okay in practice to test for forward slashes in the output of
getcwd(), because we go out of our way to convert backslashes to forward
slashes in getcwd()'s output on Windows.
Still, the correct way to test for a dir separator is by using the
helper function we introduced for that very purpose. It also serves as a
good documentation what the code tries to do (not "how").
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
setup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/setup.c b/setup.c
index 967f289f1ef..c47619605fb 100644
--- a/setup.c
+++ b/setup.c
@@ -910,7 +910,8 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
return setup_bare_git_dir(&cwd, offset, nongit_ok);
offset_parent = offset;
- while (--offset_parent > ceil_offset && cwd.buf[offset_parent] != '/');
+ while (--offset_parent > ceil_offset &&
+ !is_dir_sep(cwd.buf[offset_parent]));
if (offset_parent <= ceil_offset)
return setup_nongit(cwd.buf, nongit_ok);
if (one_filesystem) {
--
2.12.0.windows.1.7.g94dafc3b124
^ permalink raw reply related
* [PATCH v3 1/9] t7006: replace dubious test
From: Johannes Schindelin @ 2017-03-03 17:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488562287.git.johannes.schindelin@gmx.de>
The idea of the test case "git -p - core.pager is not used from
subdirectory" was to verify that the setup_git_directory() function had
not been called just to obtain the core.pager setting.
However, we are about to fix the early config machinery so that it
*does* work, without messing up the global state.
Once that is done, the core.pager setting *will* be used, even when
running from a subdirectory, and that is a Good Thing.
The intention of that test case, however, was to verify that the
setup_git_directory() function has not run, because it changes global
state such as the current working directory.
To keep that spirit, but fix the incorrect assumption, this patch
replaces that test case by a new one that verifies that the pager is
run in the subdirectory, i.e. that the current working directory has
not been changed at the time the pager is configured and launched, even
if the `rev-parse` command requires a .git/ directory and *will* change
the working directory.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
| 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index c8dc665f2fd..304ae06c600 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -378,9 +378,19 @@ test_GIT_PAGER_overrides expect_success test_must_fail 'git -p request-pull'
test_default_pager expect_success test_must_fail 'git -p'
test_PAGER_overrides expect_success test_must_fail 'git -p'
test_local_config_ignored expect_failure test_must_fail 'git -p'
-test_no_local_config_subdir expect_success test_must_fail 'git -p'
test_GIT_PAGER_overrides expect_success test_must_fail 'git -p'
+test_expect_failure TTY 'core.pager in repo config works and retains cwd' '
+ sane_unset GIT_PAGER &&
+ test_config core.pager "cat >cwd-retained" &&
+ (
+ cd sub &&
+ rm -f cwd-retained &&
+ test_terminal git -p rev-parse HEAD &&
+ test_path_is_file cwd-retained
+ )
+'
+
test_doesnt_paginate expect_failure test_must_fail 'git -p nonsense'
test_pager_choices 'git shortlog'
--
2.12.0.windows.1.7.g94dafc3b124
^ permalink raw reply related
* [PATCH v3 0/9] Fix the early config
From: Johannes Schindelin @ 2017-03-03 17:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
These patches are an attempt to make Git's startup sequence a bit less
surprising.
The idea here is to discover the .git/ directory gently (i.e. without
changing the current working directory, or global variables), and to use
it to read the .git/config file early, before we actually called
setup_git_directory() (if we ever do that).
This also allows us to fix the early config e.g. to determine the pager
or to resolve aliases in a non-surprising manner.
My dirty little secret is that I need to discover the Git directory
early, without changing global state, for usage statistics gathering in
the GVFS Git project, so I actually do not care all that much about the
early config, although it is a welcome fallout (and a good reason for
accepting these patches and thereby releasing me of more maintenance
burden :-)).
Notable notes:
- In contrast to earlier versions, I no longer special-case init and
clone. Peff pointed out that this adds technical debt, and that we can
actually argue (for consistency's sake) that early config reads the
current repository config (if any) even for init and clone.
- The read_early_config() function does not cache Git directory
discovery nor read values. If needed, this can be implemented later,
in a separate patch series.
- The alias handling in git.c could possibly benefit from this work, but
again, this is a separate topic from the current patch series.
Changes since v2:
- replaced `test -e` by `test_path_is_file`
- fixed premature "cwd -> dir" in 2/9
- the setup_git_directory_gently_1() function is no longer renamed
because it is not exported directly, anyway
- fixed the way setup_discovered_git_dir() expected the offset parameter
to exclude the trailing slash (which is not true for root
directories); also verified that setup_bare_git_dir() does not require
a corresponding patch
- switched to using size_t instead of int to save the length of the
strbuf in discover_git_directory()
- ensured that discover_git_directory() turns a relative gitdir into an
absolute one even if there is already some text in the strbuf
- clarified under which circumstances we turn a relative gitdir into an
absolute one
- avoided absolute gitdir with trailing "/." to be returned
- the commit that fixes the "really dirty hack" now rewords that comment
to reflect that it is no longer a really dirty hack
- dropped the special-casing of init and clone
- the discover_git_directory() function now correctly checks the
repository version, warning (and returning NULL) in case of a problem
Johannes Schindelin (9):
t7006: replace dubious test
setup_git_directory(): use is_dir_sep() helper
Prepare setup_discovered_git_directory() the root directory
setup_git_directory_1(): avoid changing global state
Export the discover_git_directory() function
Make read_early_config() reusable
read_early_config(): avoid .git/config hack when unneeded
read_early_config(): really discover .git/
Test read_early_config()
cache.h | 3 +
config.c | 27 ++++++
pager.c | 31 -------
setup.c | 237 ++++++++++++++++++++++++++++++++----------------
t/helper/test-config.c | 15 +++
t/t1309-early-config.sh | 50 ++++++++++
t/t7006-pager.sh | 18 +++-
7 files changed, 269 insertions(+), 112 deletions(-)
create mode 100755 t/t1309-early-config.sh
base-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858
Published-As: https://github.com/dscho/git/releases/tag/early-config-v3
Fetch-It-Via: git fetch https://github.com/dscho/git early-config-v3
Interdiff vs v2:
diff --git a/cache.h b/cache.h
index 0af7141242f..8a4580f921d 100644
--- a/cache.h
+++ b/cache.h
@@ -518,6 +518,7 @@ extern void set_git_work_tree(const char *tree);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
extern void setup_work_tree(void);
+/* Find GIT_DIR without changing the working directory or other global state */
extern const char *discover_git_directory(struct strbuf *gitdir);
extern const char *setup_git_directory_gently(int *);
extern const char *setup_git_directory(void);
@@ -2070,7 +2071,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
/* setup.c */
struct startup_info {
- int have_repository, creating_repository;
+ int have_repository;
const char *prefix;
};
extern struct startup_info *startup_info;
diff --git a/config.c b/config.c
index bcda397d42e..749623a9649 100644
--- a/config.c
+++ b/config.c
@@ -1419,25 +1419,16 @@ void read_early_config(config_fn_t cb, void *data)
git_config_with_options(cb, data, NULL, 1);
/*
- * Note that this is a really dirty hack that does the wrong thing in
- * many cases. The crux of the problem is that we cannot run
- * setup_git_directory() early on in git's setup, so we have no idea if
- * we are in a repository or not, and therefore are not sure whether
- * and how to read repository-local config.
- *
- * So if we _aren't_ in a repository (or we are but we would reject its
- * core.repositoryformatversion), we'll read whatever is in .git/config
- * blindly. Similarly, if we _are_ in a repository, but not at the
- * root, we'll fail to find .git/config (because it's really
- * ../.git/config, etc). See t7006 for a complete set of failures.
- *
- * However, we have historically provided this hack because it does
- * work some of the time (namely when you are at the top-level of a
- * valid repository), and would rarely make things worse (i.e., you do
- * not generally have a .git/config file sitting around).
+ * When we are not about to create a repository ourselves (init or
+ * clone) and when no .git/ directory was set up yet (in which case
+ * git_config_with_options() would already have picked up the
+ * repository config), we ask discover_git_directory() to figure out
+ * whether there is any repository config we should use (but unlike
+ * setup_git_directory_gently(), no global state is changed, most
+ * notably, the current working directory is still the same after
+ * the call).
*/
- if (!startup_info->creating_repository && !have_git_dir() &&
- discover_git_directory(&buf)) {
+ if (!have_git_dir() && discover_git_directory(&buf)) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
diff --git a/git.c b/git.c
index 9fb9bb90a21..33f52acbcc8 100644
--- a/git.c
+++ b/git.c
@@ -337,9 +337,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
struct stat st;
const char *prefix;
- if (p->fn == cmd_init_db || p->fn == cmd_clone)
- startup_info->creating_repository = 1;
-
prefix = NULL;
help = argc == 2 && !strcmp(argv[1], "-h");
if (!help) {
diff --git a/setup.c b/setup.c
index 7ceca6cc6ef..5320ae37314 100644
--- a/setup.c
+++ b/setup.c
@@ -721,8 +721,10 @@ static const char *setup_discovered_git_dir(const char *gitdir,
if (offset == cwd->len)
return NULL;
- /* Make "offset" point to past the '/', and add a '/' at the end */
- offset++;
+ /* Make "offset" point past the '/' (already the case for root dirs) */
+ if (offset != offset_1st_component(cwd->buf))
+ offset++;
+ /* Add a '/' at the end */
strbuf_addch(cwd, '/');
return cwd->buf + offset;
}
@@ -839,8 +841,8 @@ enum discovery_result {
* the discovered .git/ directory, if any. This path may be relative against
* `dir` (i.e. *not* necessarily the cwd).
*/
-static enum discovery_result discover_git_directory_1(struct strbuf *dir,
- struct strbuf *gitdir)
+static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
+ struct strbuf *gitdir)
{
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
@@ -923,24 +925,44 @@ static enum discovery_result discover_git_directory_1(struct strbuf *dir,
const char *discover_git_directory(struct strbuf *gitdir)
{
- struct strbuf dir = STRBUF_INIT;
- int len;
+ struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
+ size_t gitdir_offset = gitdir->len, cwd_len;
+ struct repository_format candidate;
if (strbuf_getcwd(&dir))
return NULL;
- len = dir.len;
- if (discover_git_directory_1(&dir, gitdir) < 0) {
+ cwd_len = dir.len;
+ if (setup_git_directory_gently_1(&dir, gitdir) < 0) {
strbuf_release(&dir);
return NULL;
}
- if (dir.len < len && !is_absolute_path(gitdir->buf)) {
- strbuf_addch(&dir, '/');
- strbuf_insert(gitdir, 0, dir.buf, dir.len);
+ /*
+ * The returned gitdir is relative to dir, and if dir does not reflect
+ * the current working directory, we simply make the gitdir absolute.
+ */
+ if (dir.len < cwd_len && !is_absolute_path(gitdir->buf + gitdir_offset)) {
+ /* Avoid a trailing "/." */
+ if (!strcmp(".", gitdir->buf + gitdir_offset))
+ strbuf_setlen(gitdir, gitdir_offset);
+ else
+ strbuf_addch(&dir, '/');
+ strbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);
}
+
+ strbuf_reset(&dir);
+ strbuf_addf(&dir, "%s/config", gitdir->buf + gitdir_offset);
+ read_repository_format(&candidate, dir.buf);
strbuf_release(&dir);
+ if (verify_repository_format(&candidate, &err) < 0) {
+ warning("ignoring git dir '%s': %s",
+ gitdir->buf + gitdir_offset, err.buf);
+ strbuf_release(&err);
+ return NULL;
+ }
+
return gitdir->buf;
}
@@ -970,7 +992,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
die_errno(_("Unable to read current working directory"));
strbuf_addbuf(&dir, &cwd);
- switch (discover_git_directory_1(&dir, &gitdir)) {
+ switch (setup_git_directory_gently_1(&dir, &gitdir)) {
case GIT_DIR_NONE:
prefix = NULL;
break;
@@ -1000,7 +1022,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
dir.buf);
default:
- die("BUG: unhandled discover_git_directory() result");
+ die("BUG: unhandled setup_git_directory_1() result");
}
if (prefix)
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index bf89340988b..4f3794d415e 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -387,7 +387,7 @@ test_expect_success TTY 'core.pager in repo config works and retains cwd' '
cd sub &&
rm -f cwd-retained &&
test_terminal git -p rev-parse HEAD &&
- test -e cwd-retained
+ test_path_is_file cwd-retained
)
'
--
2.12.0.windows.1.7.g94dafc3b124
^ permalink raw reply
* Re: [PATCH v5 24/24] t1406: new tests for submodule ref store
From: Michael Haggerty @ 2017-03-03 16:51 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-25-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> t/t1406-submodule-ref-store.sh (new +x) | 95 +++++++++++++++++++++++++++++++++
> 1 file changed, 95 insertions(+)
> create mode 100755 t/t1406-submodule-ref-store.sh
I wonder if you could reduce some of the repetition between this test
file and t1405, at least for the functions that should work for both the
main repository and for submodules? For example, if you were to define
GIT='git'
in one case and
GIT='git -C sub'
in the other, then maybe some of the setup code could be made to look
alike? Then the division could be one test file for the functions that
work for both main and submodules, and a second for the functions that
only work for main. Just a thought...
Michael
^ permalink raw reply
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Mike Crowe @ 2017-03-03 17:01 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git
In-Reply-To: <5d92d3b8-f438-9be5-9742-22f8cd8fe03d@web.de>
Hi Torsten,
Your patch has been superseded, but I thought I ought to answer your
questions rather than leave them hanging.
On Thursday 02 March 2017 at 19:17:00 +0100, Torsten Bögershausen wrote:
> On 2017-03-01 22:25, Mike Crowe wrote:
> > On Wednesday 01 March 2017 at 18:04:44 +0100, tboegi@web.de wrote:
> >> From: Junio C Hamano <gitster@pobox.com>
> >>
> >> git diff --quiet may take a short-cut to see if a file is changed
> >> in the working tree:
> >> Whenever the file size differs from what is recorded in the index,
> >> the file is assumed to be changed and git diff --quiet returns
> >> exit with code 1
> >>
> >> This shortcut must be suppressed whenever the line endings are converted
> >> or a filter is in use.
> >> The attributes say "* text=auto" and a file has
> >> "Hello\nWorld\n" in the index with a length of 12.
> >> The file in the working tree has "Hello\r\nWorld\r\n" with a length of 14.
> >> (Or even "Hello\r\nWorld\n").
> >> In this case "git add" will not do any changes to the index, and
> >> "git diff -quiet" should exit 0.
> >>
> >> Add calls to would_convert_to_git() before blindly saying that a different
> >> size means different content.
> >>
> >> Reported-By: Mike Crowe <mac@mcrowe.com>
> >> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> >> ---
> >> This is what I can come up with, collecting all the loose ends.
> >> I'm not sure if Mike wan't to have the Reported-By with a
> >> Signed-off-by ?
> >> The other question is, if the commit message summarizes the discussion
> >> well enough ?
> >>
> >> diff.c | 18 ++++++++++++++----
> >> t/t0028-diff-converted.sh | 27 +++++++++++++++++++++++++++
> >> 2 files changed, 41 insertions(+), 4 deletions(-)
> >> create mode 100755 t/t0028-diff-converted.sh
> >>
> >> diff --git a/diff.c b/diff.c
> >> index 051761b..c264758 100644
> >> --- a/diff.c
> >> +++ b/diff.c
> >> @@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)
> >> * differences.
> >> *
> >> * 2. At this point, the file is known to be modified,
> >> - * with the same mode and size, and the object
> >> - * name of one side is unknown. Need to inspect
> >> - * the identical contents.
> >> + * with the same mode and size, the object
> >> + * name of one side is unknown, or size comparison
> >> + * cannot be depended upon. Need to inspect the
> >> + * contents.
> >> */
> >> if (!DIFF_FILE_VALID(p->one) || /* (1) */
> >> !DIFF_FILE_VALID(p->two) ||
> >> @@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)
> >> (p->one->mode != p->two->mode) ||
> >> diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
> >> diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
> >> - (p->one->size != p->two->size) ||
> >> +
> >> + /*
> >> + * only if eol and other conversions are not involved,
> >> + * we can say that two contents of different sizes
> >> + * cannot be the same without checking their contents.
> >> + */
> >> + (!would_convert_to_git(p->one->path) &&
> >> + !would_convert_to_git(p->two->path) &&
> >> + (p->one->size != p->two->size)) ||
> >> +
> >> !diff_filespec_is_identical(p->one, p->two)) /* (2) */
> >> p->skip_stat_unmatch_result = 1;
> >> return p->skip_stat_unmatch_result;
> >> diff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh
> >> new file mode 100755
> >> index 0000000..3d5ab95
> >> --- /dev/null
> >> +++ b/t/t0028-diff-converted.sh
> >> @@ -0,0 +1,27 @@
> >> +#!/bin/sh
> >> +#
> >> +# Copyright (c) 2017 Mike Crowe
> >> +#
> >> +# These tests ensure that files changing line endings in the presence
> >> +# of .gitattributes to indicate that line endings should be ignored
> >> +# don't cause 'git diff' or 'git diff --quiet' to think that they have
> >> +# been changed.
> >> +
> >> +test_description='git diff with files that require CRLF conversion'
> >> +
> >> +. ./test-lib.sh
> >> +
> >> +test_expect_success setup '
> >> + echo "* text=auto" >.gitattributes &&
> >> + printf "Hello\r\nWorld\r\n" >crlf.txt &&
> >> + git add .gitattributes crlf.txt &&
> >> + git commit -m "initial"
> >> +'
> >> +
> >> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '
> >> + printf "Hello\r\nWorld\n" >crlf.txt &&
> >> + git status &&
> >> + git diff --quiet
> >> +'
> >> +
> >> +test_done
> >
[snip]
> > Also, I think I've found a behaviour change with this fix. Consider:
> >
> > echo "* text=auto" >.gitattributes
> > printf "Hello\r\nWorld\r\n" >crlf.txt
> That should give
> "Hello\nWorld\n" in the index:
>
> git add .gitattributes crlf.txt
> warning: CRLF will be replaced by LF in ttt/crlf.txt.
> The file will have its original line endings in your working directory.
> tb@mac:/tmp/ttt> git commit -m "initial"
> [master (root-commit) 354f657] initial
> 2 files changed, 3 insertions(+)
> create mode 100644 ttt/.gitattributes
> create mode 100644 ttt/crlf.txt
> tb@mac:/tmp/ttt> git ls-files --eol
> i/lf w/lf attr/text=auto .gitattributes
> i/lf w/crlf attr/text=auto crlf.txt
> tb@mac:/tmp/ttt>
>
> > git add .gitattributes crlf.txt
> > git commit -m "initial"
> >
> > printf "\r\n" >>crlf.txt
> >
> > With the above patch, both "git diff" and "git diff --quiet" report that
> > there are no changes. Previously Git would report the extra newline
> > correctly.
> Wait a second.
> Which extra newline "correctly" ?
The extra newline I appended with the printf "\r\n" >> crlf.txt
> The "git diff" command is about the changes which will be done to the index.
> Regardless if you have any of these in the working tree on disk:
>
> "Hello\nWorld\n"
> "Hello\nWorld\r\n"
> "Hello\r\nWorld\n"
> "Hello\r\nWorld\r\n"
>
> "git status" and "git diff --quiet"
> should not report any changes.
But I didn't have any of those. I ended up with:
"Hello\nWorld\n"
in the index, and
"Hello\r\nWorld\r\n\r\n"
in the working tree, but the extra newline was not reported by git diff.
> So I don't know if there is a mis-understanding about "git diff" on your side,
> or if I miss something.
I don't think it matters any more since Junio's patch didn't suffer from
this problem.
Thanks.
Mike.
^ permalink raw reply
* Re: [PATCH v2 8/9] read_early_config(): really discover .git/
From: Johannes Schindelin @ 2017-03-03 15:26 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <20170303050630.mw6asla65prku3vq@sigill.intra.peff.net>
Hi Peff,
On Fri, 3 Mar 2017, Jeff King wrote:
> I think the "dirty hack..." comment in read_early_config() can be
> condensed (or go away) now.
Yes, I made that change in response to a comment you made about an earlier
patch in this series.
> I think we _could_ do away with read_early_config() entirely, and just
> have the regular config code do this lookup when we're not already in a
> repo. But then we'd really need to depend on the "creating_repository"
> flag being managed correctly.
Well, that would be a major design change. I'm not really all that
comfortable with that...
> I think I prefer the idea that a few "early" spots like pager and alias
> config need to use this special function to access the config. That's
> less likely to cause surprises when some config option is picked up
> before we have run setup_git_directory().
Exactly. There is semantic meaning in calling read_early_config() vs
git_config().
> There is one surprising case that I think we need to deal with even now,
> though. If I do:
>
> git init repo
> git -C repo config pager.upload-pack 'echo whoops'
> git upload-pack repo
> cd repo && git upload-pack .
>
> the first one is OK, but the second reads and executes the pager config
> from the repo, even though we usually consider upload-pack to be OK to
> run in an untrusted repo. This _isn't_ a new thing in your patch, but
> just something I noticed while we are here.
>
> And maybe it is a non-issue. The early-config bits all happen via the
> git wrapper, and normally we use the direct dashed "git-upload-pack" to
> access the other repositories. Certainly I have been known to use "git
> -c ... upload-pack" while debugging various things.
>
> So I dunno. You really have to jump through some hoops for it to matter,
> but I just wonder if the git wrapper should be special-casing
> upload-pack, too.
While I agree that it may sound surprising, I would like to point out that
there *is* a semantic difference between `git upload-pack repo` and `git
-C repo upload-pack .`: the working directory is different. And given that
so much in Git's operations depend on the working directory, it makes
sense that those two invocations have slightly different semantics, too.
I also agree, obviously, that this is something that would need a separate
patch series to tackle ;-)
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v5 00/24] Remove submodule from files-backend.c
From: Michael Haggerty @ 2017-03-03 16:54 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-1-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> v5 goes a bit longer than v4, basically:
I've reviewed all of the patches and left a bunch of comments, mostly
superficial. I'm very happy about the way it's going, and especially
want to say how much I appreciate that you've done so much legwork and
added so many tests.
Michael
^ permalink raw reply
* Re: [PATCH v5 23/24] t1405: some basic tests on main ref store
From: Michael Haggerty @ 2017-03-03 16:43 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-24-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> t/t1405-main-ref-store.sh (new +x) | 123 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 123 insertions(+)
> create mode 100755 t/t1405-main-ref-store.sh
>
> diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
> new file mode 100755
> index 000000000..0999829f1
> --- /dev/null
> +++ b/t/t1405-main-ref-store.sh
> @@ -0,0 +1,123 @@
> +#!/bin/sh
> +
> +test_description='test main ref store api'
> +
> +. ./test-lib.sh
> +
> +RUN="test-ref-store main"
> +
> +test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '
> + test_commit one &&
> + N=`find .git/refs -type f | wc -l` &&
I think we prefer $() to ``. (Same throughout this file and also t1406
in the next commit.)
It's notable that these tests grep around the filesystem, so they won't
be applicable to future refs backends. Of course, "pack-refs" is
intrinsically only applicable to the files backend, so for this test
it's not surprising. But some tests could conceivably be written in a
generic way, so that they should pass for any refs backend.
Just food for thought; no need to change anything now.
> + test "$N" != 0 &&
> + $RUN pack-refs 3 &&
> + N=`find .git/refs -type f | wc -l`
Didn't you mean to test the final `$N`?
> +'
> +
> +test_expect_success 'peel_ref(new-tag)' '
> + git rev-parse HEAD >expected &&
> + git tag -a -m new-tag new-tag HEAD &&
> + $RUN peel-ref refs/tags/new-tag >actual &&
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'create_symref(FOO, refs/heads/master)' '
> + $RUN create-symref FOO refs/heads/master nothing &&
> + echo refs/heads/master >expected &&
> + git symbolic-ref FOO >actual &&
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' '
> + git rev-parse FOO -- &&
> + git rev-parse refs/tags/new-tag -- &&
> + $RUN delete-refs 0 FOO refs/tags/new-tag &&
> + test_must_fail git rev-parse FOO -- &&
> + test_must_fail git rev-parse refs/tags/new-tag --
> +'
> +
> +test_expect_success 'rename_refs(master, new-master)' '
> + git rev-parse master >expected &&
> + $RUN rename-ref refs/heads/master refs/heads/new-master &&
> + git rev-parse new-master >actual &&
> + test_cmp expected actual &&
> + test_commit recreate-master
> +'
Isn't HEAD set to `refs/heads/master` prior to this test? If so, then I
think it should become detached when you rename `refs/heads/master`. Or
maybe it should be changed to point at `refs/heads/new-master`, I can't
remember. In either case, it might be useful for the test to check that
the behavior matches the status quo, so we notice if the behavior ever
changes inadvertently.
> +
> +test_expect_success 'for_each_ref(refs/heads/)' '
> + $RUN for-each-ref refs/heads/ | cut -c 42- >actual &&
> + cat >expected <<-\EOF &&
> + master 0x0
> + new-master 0x0
> + EOF
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'resolve_ref(new-master)' '
> + SHA1=`git rev-parse new-master` &&
> + echo "$SHA1 refs/heads/new-master 0x0" >expected &&
> + $RUN resolve-ref refs/heads/new-master 0 >actual &&
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'verify_ref(new-master)' '
> + $RUN verify-ref refs/heads/new-master
> +'
> +
> +test_expect_success 'for_each_reflog()' '
> + $RUN for-each-reflog | cut -c 42- >actual &&
> + cat >expected <<-\EOF &&
> + refs/heads/master 0x0
> + refs/heads/new-master 0x0
> + HEAD 0x1
> + EOF
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'for_each_reflog_ent()' '
> + $RUN for-each-reflog-ent HEAD >actual &&
> + head -n1 actual | grep one &&
> + tail -n2 actual | head -n1 | grep recreate-master
> +'
> +
> +test_expect_success 'for_each_reflog_ent_reverse()' '
> + $RUN for-each-reflog-ent-reverse HEAD >actual &&
> + head -n1 actual | grep recreate-master &&
> + tail -n2 actual | head -n1 | grep one
> +'
> +
> +test_expect_success 'reflog_exists(HEAD)' '
> + $RUN reflog-exists HEAD
> +'
> +
> +test_expect_success 'delete_reflog(HEAD)' '
> + $RUN delete-reflog HEAD &&
> + ! test -f .git/logs/HEAD
Maybe also test the final state using `reflog-exists` to make sure that
function can also return false?
> +'
> +
> +test_expect_success 'create-reflog(HEAD)' '
> + $RUN create-reflog HEAD 1 &&
> + test -f .git/logs/HEAD
> +'
> +
> +test_expect_success 'delete_ref(refs/heads/foo)' '
> + git checkout -b foo &&
> + FOO_SHA1=`git rev-parse foo` &&
> + git checkout --detach &&
> + test_commit bar-commit &&
> + git checkout -b bar &&
> + BAR_SHA1=`git rev-parse bar` &&
> + $RUN update-ref updating refs/heads/foo $BAR_SHA1 $FOO_SHA1 0 &&
> + echo $BAR_SHA1 >expected &&
> + git rev-parse refs/heads/foo >actual &&
> + test_cmp expected actual
> +'
> +
> +test_expect_success 'delete_ref(refs/heads/foo)' '
> + SHA1=`git rev-parse foo` &&
> + git checkout --detach &&
> + $RUN delete-ref refs/heads/foo $SHA1 0 &&
> + test_must_fail git rev-parse refs/heads/foo --
> +'
The last two tests have the same name.
You might also want to test these functions when the old oid is
incorrect or when the reference is missing.
> +test_done
Thanks for all the new tests!
Michael
^ permalink raw reply
* rebase: confusing behaviour since --fork-point
From: Laszlo Kiss @ 2017-03-03 16:37 UTC (permalink / raw)
To: git; +Cc: ldubox-coding101
Hi all,
This will be a bit long, sorry in advance.
I've hit a problem with how rebase works since the introduction of the
--fork-point option. I initially thought it was a bug until the kind
folks over at git-for-windows patiently told me otherwise.
Consider the following:
-------8<-------
# On branch master which is at origin/master
# hack hack hack
git commit -am'First topic commit'
# optionally followed by more commits
# realize I want all this on a different branch instead
# (maybe I just forgot to create one; would be typical)
git checkout -b topic --track
git branch -f master origin/master
# optionally add more commits to 'topic' and/or update 'master'
# with changes to 'origin/master', then finally:
git rebase # or git pull --rebase
------->8-------
What happens is that 'First topic commit' is discarded because rebase,
with this syntax, defaults to --fork-point, which then works out from
master's reflog that the commit in question was previously part of the
upstream branch. The rebase is then carried out under the assumption
that, even though that specific commit is no longer in master,
something equivalent to it or superseding it is, or else it is meant to
be dropped - which would likely be the case if a complicated history
rewrite happened, but is not the case here.
There are two reasons why I think this is confusing and should be
changed:
(1) My mental model of git after some 5 years of using it is such that
I view the reflog as auxiliary information for purposes of mining
history or recovering from mistakes, and I rely only on the
contents and parent-child relationships of commits to work out what
a command I'm about to run will do. I would expect the vast
majority of users to have a similar mental model, in which case the
fact that the same commits can be rebased differently depending on
the data in the reflog will be equally confusing to those users.
(2) Most people don't like to rebase too often, consequently the time
elapsed from a user creating the topic branch and resetting its
upstream until actually rebasing it can be days or weeks, by the
end of which they are unlikely to remember the circumstances of
creating the branch. Even more alarmingly, they may have committed
dozens of further changes, and therefore may not even notice that
the first few of those silently disappeared. (I'm not making this
up: see discussion links below in PS.)
I believe the correct design would be to always make --no-fork-point
the default for rebase, and only use --fork-point when explicitly
specified. This would potentially be inconvenient for those who rebase
on top of complicated history rewrites, but said inconvenience would be
mitigated by several factors:
- Anyone doing rebases like that will already know that they can go
wrong in a million ways.
- Perhaps more crucially, they will have a way of noticing if it went
wrong, with all relevant information in short-term memory, so would
recover easily.
- If the rebase turns out to be really complex, they are likely to
resort to rebase -i, which shows full details of what is about to
happen, moreover it seems relatively simple to enhance the contents
of the rebase-todo file so that full information about the merge-base
and fork-point is readily available.
If changing the default is not an option, e.g. because of backwards
compatibility concerns, then some configurability could still be
helpful, e.g. rebase.useForkPoint = never / auto / always (default
auto, to keep the current behaviour). Although I suspect this would
just lead to everyone suggesting setting this to 'never'. In any case
this would provide a way to ensure that any git newbies in my
organization don't spend days trying to figure this out like I've just
done.
Assuming there is agreement to do one of the above (I don't even know
whose agreement is required), what's the process for getting it
implemented? Sorry, that probably counts as a dumb question, but I've
never been around open-source projects much & need someone to show me
the ropes.
Many thanks
Laszlo
PS. Further reading about the same topic if anyone is interested:
- http://marc.info/?l=git&m=140991293402880&w=2 (from this same mailing
list 2+ years ago, but I can see no clear conclusion)
- https://github.com/git-for-windows/git/issues/1076 (my bug report
where contacting this list was suggested)
- http://stackoverflow.com/questions/22790765 and
http://stackoverflow.com/questions/35320740 (various SO users being
confused and asking about / discussing the same thing)
^ permalink raw reply
* bisect-helper: we do not bisect --objects
From: Junio C Hamano @ 2017-03-03 16:16 UTC (permalink / raw)
To: git, Christian Couder
Ever since "bisect--helper" was introduced in 1bf072e366
("bisect--helper: implement "git bisect--helper"", 2009-03-26),
after setting up the "rev-list $bad --not $good_ones" machinery, the
code somehow prepared to mark the trees and blobs at the good boundary
as uninteresting, only when --objects option was given. This was kept
across a bit of refactoring done by 2ace9727be ("bisect: move common
bisect functionality to "bisect_common"", 2009-04-19) and survives
to this day.
However, "git bisect" does not care about tree/blob object
reachability at all---it purely works at the commit DAG level and
nobody passes (and nobody should pass) "--objects" option to the
underlying rev-list machinery. Remove the dead code.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Christian, do you recall what we were thinking when we added this
mark_edges_uninteresting() call in this program? If you don't,
don't worry--this was done more than 8 years ago. I am just
being curious and also a bit being cautious in case I am missing
something.
Thanks.
bisect.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/bisect.c b/bisect.c
index 30808cadf7..86c5929a23 100644
--- a/bisect.c
+++ b/bisect.c
@@ -634,8 +634,6 @@ static void bisect_common(struct rev_info *revs)
{
if (prepare_revision_walk(revs))
die("revision walk setup failed");
- if (revs->tree_objects)
- mark_edges_uninteresting(revs, NULL);
}
static void exit_if_skipped_commits(struct commit_list *tried,
^ permalink raw reply related
* Re: log -S/-G (aka pickaxe) searches binary files by default
From: Junio C Hamano @ 2017-03-03 16:07 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Braun, GIT Mailing-list
In-Reply-To: <20170303051721.r6pahs4vjtqqoevc@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Mar 02, 2017 at 05:36:17PM -0800, Junio C Hamano wrote:
> ...
>> > Is that on purpose?
>>
>> No, it's a mere oversight (as I do not think I never even thought
>> about special casing binary
>> files from day one, it is unlikely that you would find _any_ old
>> version of Git that behaves
>> differently).
>
> The email focuses on "-G", and I think it is wrong to look in binary
> files there, as "grep in diff" does not make sense for a binary file
> that we would refuse to diff.
Yeah, I agree.
> But the subject also mentions "-S". I always assumed it was intentional
> to look in binary files there, as it is searching for a pure byte
> sequence. I would not mind an option to disable that, but I think the
> default should remain on.
As the feature was built to be one of the core ingredients necessary
towards the 'ideal SCM' envisioned in
<http://public-inbox.org/git/Pine.LNX.4.58.0504150753440.7211@ppc970.osdl.org>
"-S" is about finding "a block of text". It was merely an oversight
that we didn't add explicit code to ignore binary when we introduced
the concept of "is this text? is it worth finding things in and
diffing binary files?".
I do agree that it may be too late and/or disruptive to change its
behaviour now, as people may grew expectations different from the
original motivation and design, though.
^ permalink raw reply
* Re: [PATCH v5 20/24] files-backend: avoid ref api targetting main ref store
From: Michael Haggerty @ 2017-03-03 16:03 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-21-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> A small step towards making files-backend works as a non-main ref store
> using the newly added store-aware API.
>
> For the record, `join` and `nm` on refs.o and files-backend.o tell me
> that files-backend no longer uses functions that defaults to
> get_main_ref_store().
Nice!
> I'm not yet comfortable at the idea of removing
> files_assert_main_repository() (or converting REF_STORE_MAIN to
> REF_STORE_WRITE). More staring and testing is required before that can
> happen. Well, except peel_ref(). I'm pretty sure that function is safe.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/files-backend.c | 85 ++++++++++++++++++++++++++++++----------------------
> 1 file changed, 49 insertions(+), 36 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index dafddefd3..09c280fd3 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> [...]
> @@ -3873,8 +3883,10 @@ static int files_transaction_commit(struct ref_store *ref_store,
> * head_ref within the transaction, then split_head_update()
> * arranges for the reflog of HEAD to be updated, too.
> */
> - head_ref = resolve_refdup("HEAD", RESOLVE_REF_NO_RECURSE,
> - head_oid.hash, &head_type);
> + head_ref = (char *)refs_resolve_ref_unsafe(ref_store, "HEAD",
> + RESOLVE_REF_NO_RECURSE,
> + head_oid.hash, &head_type);
> + head_ref = xstrdup_or_null(head_ref);
If you combine the last two statements, you can avoid the ugly cast.
> [...]
Michael
^ permalink raw reply
* Re: [PATCH 3/3] Remove outdated info in difftool manpage
From: Johannes Schindelin @ 2017-03-03 15:46 UTC (permalink / raw)
To: Denton Liu; +Cc: git, davvid
In-Reply-To: <20170303115751.GA13225@arch-attack.localdomain>
Hi Denton (or should I address you as Liu?),
On Fri, 3 Mar 2017, Denton Liu wrote:
> When difftool was rewritten in C, it removed the capability to read
> fallback configs from mergetool. This changes the documentation to
> reflect this.
Thanks for pointing that out. But that is probably an oversight on my
part, not an intentional change...
Ciao,
Johannes
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox