* Re: [PATCH 2/2] add a Makefile switch to avoid gettext translation in shell scripts
From: Junio C Hamano @ 2012-01-24 20:13 UTC (permalink / raw)
To: Alex Riesen
Cc: Git Mailing List, Ævar Arnfjörð, Jonathan Nieder
In-Reply-To: <CALxABCaGMabTLcCiYLv31YCiVY4OK7yEr4KL6e-0UMttMjGA_g@mail.gmail.com>
> Amen :)
Thanks.
^ permalink raw reply
* Re: {Spam?} push pull not working
From: Jeff King @ 2012-01-24 20:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Rick Bragg, GIT Mailing-list
In-Reply-To: <7vty3k3lmh.fsf@alter.siamese.dyndns.org>
On Tue, Jan 24, 2012 at 12:12:54PM -0800, Junio C Hamano wrote:
> > For example, imagine repo1 has two branches, "master" and "foo", and the
> > "master" branch is checked out. When you clone it, the resulting repo2
> > will have remote-tracking branches for both "master" and "foo", but will
> > only checkout the "master" branch. Now imagine you make commits on
> > "foo" in repo1, and then try to push. Git's default behavior is to push
> > only branches which match (by name) a branch on the destination. So we
> > would attempt to push "master" (which is up to date), but not "foo".
>
> Technically you are not saying anything incorrect, but the above is not an
> appropriate paragraph to give to a total newbie, I would have to say.
> [...]
Yeah, I agree with all of this. I was trying not to go into too much
detail because we knew so little about the situation (e.g., we don't
even know if repo2 in the example is bare or not!), but perhaps my
terseness made things even more confusing.
This might have been a better example (it exhibits the problem, but is
not an example of a terrible thing to be doing):
1. repo1 has a "master" branch
2. clone repo1 with "git clone --bare repo1 repo2". Repo2 now has a
master branch.
3. create a new "foo" branch in repo and commit on it
4. "git push ../repo2" from repo1. This is a sane thing to be doing,
but will not push the newly-created "foo" branch, as some users
might expect.
-Peff
^ permalink raw reply
* Re: [BUG] Git bisect not finding the right commit
From: Jeff King @ 2012-01-24 20:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Andreas J. Koenig
In-Reply-To: <7vk44mfdow.fsf@alter.siamese.dyndns.org>
On Fri, Jan 20, 2012 at 10:09:51AM -0800, Junio C Hamano wrote:
> > Yes, thank you for finding that out. X is actually v5.15.4-109-g3ea0c58
> > and since there was a long timespan between the start of the development
> > of the code and the merge (May-Nov), the gitk presentation got a bit
> > complex to read.
>
> (This is somewhat off-topic, so Andreas is on Cc: and the list is on To:)
>
> I doubt --simplify-by-decoration alone would make it easier to view such a
> complex and long history, but I wonder if we can use the same logic to
> help users in a case like this. Instead of only keeping tagged versions in
> the result to show topology, perhaps we can allow the user to feed a list
> of "key points in history" as command line arguments and apply the same
> kind of simplification to help visualizing the topology?
This is something I think I've wanted in the past. But unfortunately, I
can't remember the exact details, so I'm not sure how workable it would
have been. In particular, how painful is it in practice to come up with
the list of "key points" to make the graph sensible?
So I'll add my vague "yeah, that sounds good" and try to pay attention
next time the situation comes up as to whether such a feature would
actually help in practice.
-Peff
^ permalink raw reply
* Re: {Spam?} push pull not working
From: Junio C Hamano @ 2012-01-24 20:28 UTC (permalink / raw)
To: Jeff King; +Cc: Rick Bragg, GIT Mailing-list
In-Reply-To: <20120124201807.GA20145@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This might have been a better example (it exhibits the problem, but is
> not an example of a terrible thing to be doing):
>
> 1. repo1 has a "master" branch
>
> 2. clone repo1 with "git clone --bare repo1 repo2". Repo2 now has a
> master branch.
>
> 3. create a new "foo" branch in repo and commit on it
>
> 4. "git push ../repo2" from repo1. This is a sane thing to be doing,
> but will not push the newly-created "foo" branch, as some users
> might expect.
Yeah, that is pretty much the standard thing people would do, at least
before GitHub era ;-), to start a project in repo1, and then to publish
for others to fetch at repo2.
Thanks for clarification.
P.S. Did you have chance to take a look at the "grep" thing? I thought
"grep --textconv" would make sense, but I may be missing some large
pitfalls.
^ permalink raw reply
* Re: [BUG] Fail to add a module in a subdirectory if module is already cloned
From: Jens Lehmann @ 2012-01-24 21:10 UTC (permalink / raw)
To: Jehan Bing; +Cc: git
In-Reply-To: <jfmvpp$4v7$1@dough.gmane.org>
Am 24.01.2012 20:11, schrieb Jehan Bing:
> I'm getting an error if I try to add a module in a subdirectory and that module is already cloned.
> Here are the steps to reproduce (git 1.7.8.3):
>
> git init module
> cd module
> echo foo > foo
> git add foo
> git commit -m "init"
> cd ..
> git init super
> cd super
> echo foo > foo
> git add foo
> git commit -m "init"
> git branch b1
> git branch b2
> git checkout b1
> git submodule add ../module lib/module
> git commit -m "module"
> git checkout b2
> rm -rf lib
> git submodule add ../module lib/module
>
> The last command returns:
> fatal: Not a git repository: ../.git/modules/lib/module
> Unable to checkout submodule 'lib/module'
>
> The file lib/modules/.git contains:
> gitdir: ../.git/modules/lib/module
> (missing an additional "../")
>
> In branch b1, after adding the module, the file contained the full path:
> gitdir: /[...]/super/.git/modules/lib/module
> Or contains the correct relative path after checking out b1 later:
> gitdir: ../../.git/modules/lib/module
Thanks for your detailed report, I can reproduce that on current master.
The reason for this bug seems to be that in module_clonse() the name is
not properly initialized for added submodules (it gets set to the path
later), so the correct amount of leading "../"s for the git directory
is not computed properly. The attached diff fixes that for me, I will
send a patch as soon as I have extended a test case for this breakage.
diff --git a/git-submodule.sh b/git-submodule.sh
index 3adab93..9bb2e13 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -131,6 +131,7 @@ module_clone()
gitdir=
gitdir_base=
name=$(module_name "$path" 2>/dev/null)
+ test -n "$name" || name="$path"
base_path=$(dirname "$path")
gitdir=$(git rev-parse --git-dir)
^ permalink raw reply related
* Re: [BUG] Fail to add a module in a subdirectory if module is already cloned
From: Jens Lehmann @ 2012-01-24 21:13 UTC (permalink / raw)
To: Jehan Bing; +Cc: git
In-Reply-To: <4F1F1E5F.2030509@web.de>
Am 24.01.2012 22:10, schrieb Jens Lehmann:
> The reason for this bug seems to be that in module_clonse() the name is
> not properly initialized for added submodules ...
This should have read "for re-added submodules" ...
^ permalink raw reply
* Re: [BUG] Fail to add a module in a subdirectory if module is already cloned
From: Junio C Hamano @ 2012-01-24 21:24 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Jehan Bing, git
In-Reply-To: <4F1F1E5F.2030509@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
> The reason for this bug seems to be that in module_clonse() the name is
> not properly initialized for added submodules (it gets set to the path
> later), so the correct amount of leading "../"s for the git directory
> is not computed properly. The attached diff fixes that for me, I will
> send a patch as soon as I have extended a test case for this breakage.
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3adab93..9bb2e13 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -131,6 +131,7 @@ module_clone()
> gitdir=
> gitdir_base=
> name=$(module_name "$path" 2>/dev/null)
> + test -n "$name" || name="$path"
This somehow smells like sweeping a problem under the rug. Why doesn't
module_name find the already registered path in the first place?
I see "module_name" calls "git config -f .gitmodules" and I do not see any
cd_to_toplevel in git-submodule.sh that would ensure this call to access
the gitmodules file at the top-level of the superproject. Is that the real
reason why it is not finding what it should be finding?
^ permalink raw reply
* Re: [BUG] Fail to add a module in a subdirectory if module is already cloned
From: Jens Lehmann @ 2012-01-24 21:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jehan Bing, git
In-Reply-To: <7vhazk3ibk.fsf@alter.siamese.dyndns.org>
Am 24.01.2012 22:24, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>> The reason for this bug seems to be that in module_clonse() the name is
>> not properly initialized for added submodules (it gets set to the path
>> later), so the correct amount of leading "../"s for the git directory
>> is not computed properly. The attached diff fixes that for me, I will
>> send a patch as soon as I have extended a test case for this breakage.
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 3adab93..9bb2e13 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -131,6 +131,7 @@ module_clone()
>> gitdir=
>> gitdir_base=
>> name=$(module_name "$path" 2>/dev/null)
>> + test -n "$name" || name="$path"
>
> This somehow smells like sweeping a problem under the rug. Why doesn't
> module_name find the already registered path in the first place?
>
> I see "module_name" calls "git config -f .gitmodules" and I do not see any
> cd_to_toplevel in git-submodule.sh that would ensure this call to access
> the gitmodules file at the top-level of the superproject. Is that the real
> reason why it is not finding what it should be finding?
Nope, it's the fact that the .gitmodules file doesn't contain this name
because the branch was rewound. Please see my post where I proposed the
same change for a slightly different problem:
http://permalink.gmane.org/gmane.comp.version-control.git/187823
(just fast forward to the first hunk of my diff at the end)
I just didn't realize back then that this is needed even without the
other changes to work properly. The possibly missing cd_to_toplevel is
another problem, the OP started the submodule add in the top level
directory anyways.
^ permalink raw reply
* [PATCH] submodule add: fix breakage when re-adding a deep submodule
From: Jens Lehmann @ 2012-01-24 21:49 UTC (permalink / raw)
To: Jehan Bing; +Cc: git, Junio C Hamano
In-Reply-To: <4F1F1E5F.2030509@web.de>
Since recently a submodule with name <name> has its git directory in the
.git/modules/<name> directory of the superproject while the work tree
contains a gitfile pointing there.
When the same submodule is added on a branch where it wasn't present so
far (it is not found in the .gitmodules file), the name is not initialized
from the path as it should. This leads to a wrong path entered in the
gitfile when the .git/modules/<name> directory is found, as this happily
uses the - now empty - name. It then always points only a single directory
up, even if we have a path deeper in the directory hierarchy.
Fix that by initializing the name of the submodule early in module_clone()
if module_name() returned an empty name and add a test to catch that bug.
Reported-by: Jehan Bing <jehan@orb.com>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
Am 24.01.2012 22:10, schrieb Jens Lehmann:
> Am 24.01.2012 20:11, schrieb Jehan Bing:
>> I'm getting an error if I try to add a module in a subdirectory and that module is already cloned.
>> Here are the steps to reproduce (git 1.7.8.3):
...
> The reason for this bug seems to be that in module_clonse() the name is
> not properly initialized for added submodules (it gets set to the path
> later), so the correct amount of leading "../"s for the git directory
> is not computed properly. The attached diff fixes that for me, I will
> send a patch as soon as I have extended a test case for this breakage.
Which I now have.
git-submodule.sh | 1 +
t/t7406-submodule-update.sh | 8 ++++++++
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 3adab93..9bb2e13 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -131,6 +131,7 @@ module_clone()
gitdir=
gitdir_base=
name=$(module_name "$path" 2>/dev/null)
+ test -n "$name" || name="$path"
base_path=$(dirname "$path")
gitdir=$(git rev-parse --git-dir)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 33b292b..5b97222 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -611,4 +611,12 @@ test_expect_success 'submodule update places git-dir in superprojects git-dir re
)
'
+test_expect_success 'submodule add properly re-creates deeper level submodules' '
+ (cd super &&
+ git reset --hard master &&
+ rm -rf deeper/ &&
+ git submodule add ../submodule deeper/submodule
+ )
+'
+
test_done
--
1.7.9.rc2.3.g18574a
^ permalink raw reply related
* Re: [BUG] Fail to add a module in a subdirectory if module is already cloned
From: Jens Lehmann @ 2012-01-24 22:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <4F1F2642.1070707@web.de>
Am 24.01.2012 22:44, schrieb Jens Lehmann:
> Am 24.01.2012 22:24, schrieb Junio C Hamano:
>> I see "module_name" calls "git config -f .gitmodules" and I do not see any
>> cd_to_toplevel in git-submodule.sh that would ensure this call to access
>> the gitmodules file at the top-level of the superproject. Is that the real
>> reason why it is not finding what it should be finding?
Just for the record: I checked that and git-submodule does not set the
SUBDIRECTORY_OK environment variable so every time it is not run in the
top level directory it aborts with:
"You need to run this command from the toplevel of the working tree."
^ permalink raw reply
* [PATCH 1/5] t0061: Fix incorrect indentation
From: Frans Klaver @ 2012-01-24 22:32 UTC (permalink / raw)
To: git; +Cc: Junio C. Hamano, Jonathan Nieder, Johannes Sixt, Frans Klaver
In-Reply-To: <1327444346-6243-1-git-send-email-fransklaver@gmail.com>
The hello.sh script started with <tab>#!, which is not considered a
correct hash-bang line. The default interpreter happened to be the one
required for this specific test.
Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
t/t0061-run-command.sh | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 8d4938f..95e89bc 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -8,8 +8,8 @@ test_description='Test run command'
. ./test-lib.sh
cat >hello-script <<-EOF
- #!$SHELL_PATH
- cat hello-script
+#!$SHELL_PATH
+cat hello-script
EOF
>empty
--
1.7.8.1
^ permalink raw reply related
* [PATCH 3/5] run-command: Elaborate execvp error checking
From: Frans Klaver @ 2012-01-24 22:32 UTC (permalink / raw)
To: git; +Cc: Junio C. Hamano, Jonathan Nieder, Johannes Sixt, Frans Klaver
In-Reply-To: <1327444346-6243-1-git-send-email-fransklaver@gmail.com>
The interpretation of errors from execvp was rather terse. For user
convenience communication of the nature of the error can be improved.
This patch introduces a function with more elaborate investigation of
the errors encountered. After a failure occurs on a program or script,
inspect_failure will run through the PATH entries if necessary, and if
the file exists, perform some tests on the file. If nothing is found,
ENOENT is returned.
The function inspect_file() will try to find out if the file itself or
an interpreter caused the issue. Some scripts don't have the hash-bang
and will not be reported as scripts. If the interpreter cannot be found,
the existing behavior is kept -- i.e. ENOENT will be reported to the
parent process.
Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
run-command.c | 144 +++++++++++++++++++++++++++++++++++++++++++++---
t/t0061-run-command.sh | 6 +-
2 files changed, 140 insertions(+), 10 deletions(-)
diff --git a/run-command.c b/run-command.c
index 1c51043..17a65fe 100644
--- a/run-command.c
+++ b/run-command.c
@@ -2,6 +2,7 @@
#include "run-command.h"
#include "exec_cmd.h"
#include "argv-array.h"
+#include "dir.h"
static inline void close_pair(int fd[2])
{
@@ -134,6 +135,140 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
return code;
}
+#ifndef WIN32
+static int is_executable_file(const char *path)
+{
+ return access(path, X_OK) == 0 && !is_directory(path);
+}
+
+static int is_searchable(const char *path)
+{
+ return access(path, X_OK) == 0 && is_directory(path);
+}
+
+static void die_file_error(const char *file, int err)
+{
+ die("cannot exec '%s': %s", file, strerror(err));
+}
+
+static char *get_interpreter(const char *first_line)
+{
+ struct strbuf sb = STRBUF_INIT;
+ size_t start = strspn(first_line + 2, " \t") + 2;
+ size_t end = strcspn(first_line + start, " \t\r\n") + start;
+
+ if (start >= end)
+ return NULL;
+
+ strbuf_add(&sb, first_line + start, end - start);
+ return strbuf_detach(&sb, NULL);
+}
+
+static void inspect_file(struct strbuf *fn, int err, const char *argv0)
+{
+ /*
+ * Typical line length of 80. BSD only allows 32 bytes, but that
+ * won't really make a difference.
+ */
+ char buf[80];
+ size_t read;
+ FILE *file;
+ if (!is_executable_file(fn->buf)) {
+ strbuf_release(fn);
+ die_file_error(argv0, err);
+ }
+
+ file = fopen(fn->buf, "r");
+ if (!file) {
+ strbuf_release(fn);
+ die_file_error(argv0, err);
+ }
+
+ read = fread(buf, 1, sizeof(buf), file);
+ fclose(file);
+ if (read > 2 && buf[0] == '#' && buf[1] == '!') {
+ char *i = get_interpreter(buf);
+ char es[1024];
+ if (i) {
+ sprintf(es, "cannot exec '%s': "
+ "bad interpreter '%s': %s",
+ argv0, i,
+ strerror(err));
+ } else {
+ sprintf(es, "cannot exec '%s': "
+ "bad interpreter: %s",
+ argv0, strerror(err));
+ }
+ free(i);
+ strbuf_release(fn);
+ if (err == ENOENT) {
+ error("%s", es);
+ exit(127);
+ } else {
+ die("%s", es);
+ }
+ }
+
+ strbuf_release(fn);
+ die_file_error(argv0, err);
+}
+
+static void inspect_failure(const char *argv0, int silent_exec_failure)
+{
+ int err = errno;
+ struct strbuf sb = STRBUF_INIT;
+
+ /* errors not related to path */
+ if (errno == E2BIG || errno == ENOMEM)
+ die_file_error(argv0, err);
+
+ if (strchr(argv0, '/')) {
+ if (file_exists(argv0)) {
+ strbuf_add(&sb, argv0, strlen(argv0));
+ inspect_file(&sb, err, argv0);
+ }
+ } else {
+ char *path, *next;
+ path = getenv("PATH");
+ while (path) {
+ next = strchrnul(path, ':');
+ if (path < next)
+ strbuf_add(&sb, path, next - path);
+ else
+ strbuf_addch(&sb, '.');
+
+ if (!*next)
+ path = NULL;
+ else
+ path = next + 1;
+
+ if (!is_searchable(sb.buf)) {
+ strbuf_release(&sb);
+ continue;
+ }
+
+ if (sb.len && sb.buf[sb.len - 1] != '/')
+ strbuf_addch(&sb, '/');
+ strbuf_addstr(&sb, argv0);
+
+ if (file_exists(sb.buf))
+ inspect_file(&sb, err, argv0);
+
+ strbuf_release(&sb);
+ }
+ }
+
+ if (err == ENOENT) {
+ if (!silent_exec_failure)
+ error("cannot exec '%s': %s", argv0,
+ strerror(ENOENT));
+ exit(127);
+ } else {
+ die_file_error(argv0, err);
+ }
+}
+#endif
+
int start_command(struct child_process *cmd)
{
int need_in, need_out, need_err;
@@ -280,14 +415,7 @@ fail_pipe:
} else {
execvp(cmd->argv[0], (char *const*) cmd->argv);
}
- if (errno == ENOENT) {
- if (!cmd->silent_exec_failure)
- error("cannot run %s: %s", cmd->argv[0],
- strerror(ENOENT));
- exit(127);
- } else {
- die_errno("cannot exec '%s'", cmd->argv[0]);
- }
+ inspect_failure(cmd->argv[0], cmd->silent_exec_failure);
}
if (cmd->pid < 0)
error("cannot fork() for %s: %s", cmd->argv[0],
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 31eb3c3..14b4ea6 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -71,7 +71,8 @@ test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
chmod -x someinterpreter &&
test_must_fail test-run-command run-command ./hello.sh 2>err &&
- grep "fatal: cannot exec.*hello.sh" err
+ grep "fatal: cannot exec.*hello.sh" err &&
+ grep "bad interpreter" err
'
test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
@@ -79,7 +80,8 @@ test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
chmod +x hello.sh &&
test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err &&
- grep "error: cannot exec.*hello.sh" err
+ grep "error: cannot exec.*hello.sh" err &&
+ grep "bad interpreter" err
'
test_done
--
1.7.8.1
^ permalink raw reply related
* [PATCH 0/6 v3] Add execvp failure diagnostics
From: Frans Klaver @ 2012-01-24 22:32 UTC (permalink / raw)
To: git; +Cc: Junio C. Hamano, Jonathan Nieder, Johannes Sixt
This patch series replaces $gmane/187026. It aims to improve the
information provided after execvp fails in run-command.
This series takes a rather different approach than the previous
one. In the former the focus was on determining what might cause
an EACCES failure. This series focuses on trying to provide hints
on what went wrong on any error.
The resulting checking produces behavior rather like bash, with the
notable exception that we consider "Not found and PATH access issues"
a failure, where bash doesn't.
[PATCH 1/5] t0061: Fix incorrect indentation
[PATCH 2/5] t0061: Add tests
[PATCH 3/5] run-command: Elaborate execvp error checking
[PATCH 4/5] run-command: Warn if PATH entry cannot be searched
[PATCH 5/5] run-command: Error out if interpreter not found
run-command.c | 140 +++++++++++++++++++++++++++++++++++++++++++++---
t/t0061-run-command.sh | 57 ++++++++++++++++++-
2 files changed, 186 insertions(+), 11 deletions(-)
^ permalink raw reply
* [PATCH 4/5] run-command: Warn if PATH entry cannot be searched
From: Frans Klaver @ 2012-01-24 22:32 UTC (permalink / raw)
To: git; +Cc: Junio C. Hamano, Jonathan Nieder, Johannes Sixt, Frans Klaver
In-Reply-To: <1327444346-6243-1-git-send-email-fransklaver@gmail.com>
execvp will return EACCES when PATH entries are encountered that cannot
be searched and the requested command isn't found. Since this error only
arises in that particular case, warning the user is more appropriate
than producing an error.
Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
run-command.c | 1 +
t/t0061-run-command.sh | 3 ++-
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/run-command.c b/run-command.c
index 17a65fe..ab14910 100644
--- a/run-command.c
+++ b/run-command.c
@@ -243,6 +243,7 @@ static void inspect_failure(const char *argv0, int silent_exec_failure)
path = next + 1;
if (!is_searchable(sb.buf)) {
+ warning("cannot search '%s'", sb.buf);
strbuf_release(&sb);
continue;
}
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 14b4ea6..a4585b0 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -62,7 +62,8 @@ test_expect_success POSIXPERM 'run_command reports EACCES, search path perms' '
test_must_fail test-run-command run-command hello.sh 2>err &&
chmod 755 inaccessible &&
- grep "fatal: cannot exec.*hello.sh" err
+ grep "fatal: cannot exec.*hello.sh" err &&
+ grep "cannot search" err
'
test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
--
1.7.8.1
^ permalink raw reply related
* [PATCH 5/5] run-command: Error out if interpreter not found
From: Frans Klaver @ 2012-01-24 22:32 UTC (permalink / raw)
To: git; +Cc: Junio C. Hamano, Jonathan Nieder, Johannes Sixt, Frans Klaver
In-Reply-To: <1327444346-6243-1-git-send-email-fransklaver@gmail.com>
If the interpreter wasn't found, execvp returns ENOENT. The existing
error checking did not differentiate between file not found and
interpreter not found. While the former may be an incentive to start
inspecting aliases, the latter is an error because the desired script is
actually found.
This patch explicitly makes the interpreter failure a fatal error.
Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
As far as I'm concerned, this is a bug fix. However, since it really is
a change in git's behavior, we can still consider postponing this patch.
run-command.c | 7 +------
t/t0061-run-command.sh | 4 ++--
2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/run-command.c b/run-command.c
index ab14910..9179daf 100644
--- a/run-command.c
+++ b/run-command.c
@@ -201,12 +201,7 @@ static void inspect_file(struct strbuf *fn, int err, const char *argv0)
}
free(i);
strbuf_release(fn);
- if (err == ENOENT) {
- error("%s", es);
- exit(127);
- } else {
- die("%s", es);
- }
+ die("%s", es);
}
strbuf_release(fn);
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index a4585b0..f08163f 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -76,12 +76,12 @@ test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
grep "bad interpreter" err
'
-test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
+test_expect_success POSIXPERM 'run_command reports ENOENT, interpreter' '
cat non-existing-interpreter >hello.sh &&
chmod +x hello.sh &&
test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err &&
- grep "error: cannot exec.*hello.sh" err &&
+ grep "fatal: cannot exec.*hello.sh" err &&
grep "bad interpreter" err
'
--
1.7.8.1
^ permalink raw reply related
* [PATCH 2/5] t0061: Add tests
From: Frans Klaver @ 2012-01-24 22:32 UTC (permalink / raw)
To: git; +Cc: Junio C. Hamano, Jonathan Nieder, Johannes Sixt, Frans Klaver
In-Reply-To: <1327444346-6243-1-git-send-email-fransklaver@gmail.com>
Capture failure behavior when running into
- EACCES caused by search path permissions
- ENOENT caused by interpreter not found
Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
t/t0061-run-command.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 49 insertions(+), 1 deletions(-)
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 95e89bc..31eb3c3 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -13,6 +13,24 @@ cat hello-script
EOF
>empty
+cat >someinterpreter <<-EOF
+#!$SHELL_PATH
+cat hello-script
+EOF
+>empty
+
+cat >incorrect-interpreter-script <<-EOF
+#!someinterpreter
+cat hello-script
+EOF
+>empty
+
+cat >non-existing-interpreter <<-EOF
+#!nonexisting_interpreter
+cat hello-script
+EOF
+>empty
+
test_expect_success 'start_command reports ENOENT' '
test-run-command start-command-ENOENT ./does-not-exist
'
@@ -26,7 +44,7 @@ test_expect_success 'run_command can run a command' '
test_cmp empty err
'
-test_expect_success POSIXPERM 'run_command reports EACCES' '
+test_expect_success POSIXPERM 'run_command reports EACCES, file permissions' '
cat hello-script >hello.sh &&
chmod -x hello.sh &&
test_must_fail test-run-command run-command ./hello.sh 2>err &&
@@ -34,4 +52,34 @@ test_expect_success POSIXPERM 'run_command reports EACCES' '
grep "fatal: cannot exec.*hello.sh" err
'
+test_expect_success POSIXPERM 'run_command reports EACCES, search path perms' '
+ mkdir -p inaccessible &&
+ PATH=$(pwd)/inaccessible:$PATH &&
+ export PATH &&
+
+ cat hello-script >inaccessible/hello.sh &&
+ chmod 400 inaccessible &&
+ test_must_fail test-run-command run-command hello.sh 2>err &&
+ chmod 755 inaccessible &&
+
+ grep "fatal: cannot exec.*hello.sh" err
+'
+
+test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
+ cat incorrect-interpreter-script >hello.sh &&
+ chmod +x hello.sh &&
+ chmod -x someinterpreter &&
+ test_must_fail test-run-command run-command ./hello.sh 2>err &&
+
+ grep "fatal: cannot exec.*hello.sh" err
+'
+
+test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
+ cat non-existing-interpreter >hello.sh &&
+ chmod +x hello.sh &&
+ test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err &&
+
+ grep "error: cannot exec.*hello.sh" err
+'
+
test_done
--
1.7.8.1
^ permalink raw reply related
* Re: [BUG] Fail to add a module in a subdirectory if module is already cloned
From: Junio C Hamano @ 2012-01-24 22:38 UTC (permalink / raw)
To: Jens Lehmann; +Cc: git
In-Reply-To: <4F1F2D38.9050909@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
> Just for the record: I checked that and git-submodule does not set the
> SUBDIRECTORY_OK environment variable so every time it is not run in the
> top level directory it aborts with:
> "You need to run this command from the toplevel of the working tree."
Ah, Ok.
^ permalink raw reply
* Re: [PATCH 1/5] t0061: Fix incorrect indentation
From: Junio C Hamano @ 2012-01-24 22:39 UTC (permalink / raw)
To: Frans Klaver; +Cc: git, Jonathan Nieder, Johannes Sixt
In-Reply-To: <1327444346-6243-2-git-send-email-fransklaver@gmail.com>
Frans Klaver <fransklaver@gmail.com> writes:
> The hello.sh script started with <tab>#!, which is not considered a
> correct hash-bang line.
Isn't that exactly the reason why we start the here text with "<<-EOF",
not the usual "<<EOF"?
> required for this specific test.
>
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
> t/t0061-run-command.sh | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index 8d4938f..95e89bc 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -8,8 +8,8 @@ test_description='Test run command'
> . ./test-lib.sh
>
> cat >hello-script <<-EOF
> - #!$SHELL_PATH
> - cat hello-script
> +#!$SHELL_PATH
> +cat hello-script
> EOF
> >empty
^ permalink raw reply
* Re: [PATCH 1/5] t0061: Fix incorrect indentation
From: Jonathan Nieder @ 2012-01-24 22:40 UTC (permalink / raw)
To: Frans Klaver; +Cc: git, Junio C. Hamano, Johannes Sixt
In-Reply-To: <1327444346-6243-2-git-send-email-fransklaver@gmail.com>
Frans Klaver wrote:
> +++ b/t/t0061-run-command.sh
> @@ -8,8 +8,8 @@ test_description='Test run command'
> . ./test-lib.sh
>
> cat >hello-script <<-EOF
> - #!$SHELL_PATH
> - cat hello-script
> +#!$SHELL_PATH
> +cat hello-script
> EOF
Looks like a no-op --- the script already started with #! and no
leading tab for me. Does it behave differently on your machine?
^ permalink raw reply
* Re: [PATCH 2/5] t0061: Add tests
From: Jonathan Nieder @ 2012-01-24 22:56 UTC (permalink / raw)
To: Frans Klaver; +Cc: git, Junio C. Hamano, Johannes Sixt
In-Reply-To: <1327444346-6243-3-git-send-email-fransklaver@gmail.com>
Hi,
Frans Klaver wrote:
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -13,6 +13,24 @@ cat hello-script
> EOF
> >empty
>
> +cat >someinterpreter <<-EOF
> +#!$SHELL_PATH
> +cat hello-script
> +EOF
> +>empty
> +
> +cat >incorrect-interpreter-script <<-EOF
> +#!someinterpreter
> +cat hello-script
> +EOF
> +>empty
Thanks for writing tests. Some hints on mechanics below, and one on
strategy (see (*) below).
What is the point of repreatedly writing an empty file named "empty"?
I think this would be easier to read and maintain if scripts not
shared between multiple tests were written in the body of the relevant
tests. For example, that way it is easier to remember to remove a
helper script if the relevant test assertion changes to no longer need
it.
[...]
> @@ -26,7 +44,7 @@ test_expect_success 'run_command can run a command' '
> test_cmp empty err
> '
>
> test_expect_success POSIXPERM 'run_command reports EACCES' '
> cat hello-script >hello.sh &&
> chmod -x hello.sh &&
> test_must_fail test-run-command run-command ./hello.sh 2>err &&
[...]
> +test_expect_success POSIXPERM 'run_command reports EACCES, search path perms' '
> + mkdir -p inaccessible &&
> + PATH=$(pwd)/inaccessible:$PATH &&
> + export PATH &&
> +
> + cat hello-script >inaccessible/hello.sh &&
> + chmod 400 inaccessible &&
> + test_must_fail test-run-command run-command hello.sh 2>err &&
> + chmod 755 inaccessible &&
> +
> + grep "fatal: cannot exec.*hello.sh" err
> +'
(*) These tests would be easier to understand if squashed with the
relevant later patch in the series that changes the error message.
Maybe they could be less repetitive that way, too.
test_expect_success POSIXPERM 'diagnose command in inaccessible part of $PATH' '
mkdir -p subdir &&
cat hello-script >subdir/hello.sh &&
chmod +x subdir/hello.sh &&
chmod -x subdir &&
(
PATH=$(pwd)/inaccessible:$PATH &&
test_must_fail test-run-command run-command hello.sh 2>err
) &&
test_i18ngrep ...
'
[...]
> +test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
> + cat incorrect-interpreter-script >hello.sh &&
> + chmod +x hello.sh &&
> + chmod -x someinterpreter &&
> + test_must_fail test-run-command run-command ./hello.sh 2>err &&
> +
> + grep "fatal: cannot exec.*hello.sh" err
> +'
Is this the common case? Why would my interpreter be in the designated
spot but not marked executable? Is there some other motivating
example? (I'm genuinely curious; it's ok if the answer is "no".)
[...]
> +
> +test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
> + cat non-existing-interpreter >hello.sh &&
> + chmod +x hello.sh &&
> + test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err &&
> +
> + grep "error: cannot exec.*hello.sh" err
> +'
Maybe:
test_expect_success POSIXPERM 'diagnose missing interpreter' '
echo "#!/nonexistent/interpreter" >hello.sh &&
chmod +x hello.sh &&
test_must_fail test-run-command run-command hello.sh 2>err &&
test_i18ngrep ...
'
Hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH 3/5] run-command: Elaborate execvp error checking
From: Jonathan Nieder @ 2012-01-24 23:22 UTC (permalink / raw)
To: Frans Klaver; +Cc: git, Junio C. Hamano, Johannes Sixt
In-Reply-To: <1327444346-6243-4-git-send-email-fransklaver@gmail.com>
Klaver wrote:
> The interpretation of errors from execvp was rather terse. For user
> convenience communication of the nature of the error can be improved.
Could you give an example?
[...]
> --- a/run-command.c
> +++ b/run-command.c
> @@ -2,6 +2,7 @@
> #include "run-command.h"
> #include "exec_cmd.h"
> #include "argv-array.h"
> +#include "dir.h"
>
> static inline void close_pair(int fd[2])
> {
> @@ -134,6 +135,140 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
> return code;
> }
>
> +#ifndef WIN32
Not related to this patch, but I wonder if there should be a separate
run-command-unix.c file so these ifdefs would no longer be necessary.
What happens on Windows?
> +static void die_file_error(const char *file, int err)
> +{
> + die("cannot exec '%s': %s", file, strerror(err));
> +}
I suspect it might be clearer to use die() inline in the two call
sites so the reader does not have to figure out the calling
convention.
> +
> +static char *get_interpreter(const char *first_line)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + size_t start = strspn(first_line + 2, " \t") + 2;
> + size_t end = strcspn(first_line + start, " \t\r\n") + start;
> +
> + if (start >= end)
> + return NULL;
> +
> + strbuf_add(&sb, first_line + start, end - start);
> + return strbuf_detach(&sb, NULL);
> +}
What does this function do? What happens if first_line doesn't start
with "#!"? What should happen when there is a newline instead of a
command name? How about commands with quoting characters like " and
backslash --- are the semantics portable in these cases?
No need to use a strbuf here: xmemdupz would take care of the
allocation and copy more simply.
> +static void inspect_failure(const char *argv0, int silent_exec_failure)
> +{
> + int err = errno;
> + struct strbuf sb = STRBUF_INIT;
> +
> + /* errors not related to path */
> + if (errno == E2BIG || errno == ENOMEM)
> + die_file_error(argv0, err);
> +
> + if (strchr(argv0, '/')) {
> + if (file_exists(argv0)) {
> + strbuf_add(&sb, argv0, strlen(argv0));
> + inspect_file(&sb, err, argv0);
> + }
> + } else {
> + char *path, *next;
> + path = getenv("PATH");
I wonder if it's possible to rearrange this code to avoid deep
nesting. What does the function do, anyway? (If the reader has to
ask, it needs a comment or to be renamed.)
I guess the idea is to diagnose after the fact why execvp failed.
Might be simplest like this:
To diagnose execvp failure:
if filename does not contain a '/':
if we can't find it on the search path:
That's the problem, dummy!
replace filename with full path
if file does not exist:
just report strerror(errno)
if not executable:
...
if interpreter does not exist:
...
if interpreter not executable:
...
otherwise, just report strerror(errno)
with a separate function to find a command on the PATH, complaining
when it encounters an unsearchable entry.
Thanks for a fun read.
Jonathan
^ permalink raw reply
* Re: [PATCH 5/5] run-command: Error out if interpreter not found
From: Jonathan Nieder @ 2012-01-24 23:24 UTC (permalink / raw)
To: Frans Klaver; +Cc: git, Junio C. Hamano, Johannes Sixt
In-Reply-To: <1327444346-6243-6-git-send-email-fransklaver@gmail.com>
Frans Klaver wrote:
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -76,12 +76,12 @@ test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
> grep "bad interpreter" err
> '
>
> -test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
> +test_expect_success POSIXPERM 'run_command reports ENOENT, interpreter' '
> cat non-existing-interpreter >hello.sh &&
> chmod +x hello.sh &&
> test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err &&
>
> - grep "error: cannot exec.*hello.sh" err &&
> + grep "fatal: cannot exec.*hello.sh" err &&
Thanks. I'd suggest using "test_expect_code" rather than the detailed
wording of the message, since that is what scripts might want to rely
on.
What happens on Windows?
^ permalink raw reply
* Re: Autocompletion - commands no longer work as stand alone
From: SZEDER Gábor @ 2012-01-24 23:26 UTC (permalink / raw)
To: Nathan Bullock; +Cc: Junio C Hamano, git
In-Reply-To: <7vwr8mdvo8.fsf@alter.siamese.dyndns.org>
Hi,
> Nathan Bullock <nathanbullock@gmail.com> writes:
>
> > I have for a number of years had the following in my .bashrc
> >
> > alias br="git branch"
> > complete -F _git_branch br
> >
> > As well as similar commands for co and log.
> >
> > Recently though this broke, now when I type something like "br
> > mas<command completion>" it will occasionally complain with messages
> > like:
> > bash: [: 1: unary operator expected
> >
> > From digging through the source it looks like this was broken back in
> > April. (The commit is show at the bottom of this email.)
> >
> > So my questions are:
> > 1. Is it reasonable for things like _git_branch to work as a
> > standalone autocompletion function instead of having to go through
> > _git? I certainly like it to work as a standalone function. I also use
> > it to add autocompletion to other bash scripts that I use frequently.
> >
> > 2. If I add code that verifies that the variable cword exists at the
> > start of these functions and only if not call something like
> > _get_comp_words_by_ref -n =: cur words cword prev. Would that be
> > reasonable?
That would be too fragile, it will break if $cword is set in the
environment from which you invoke _git_<cmd>() completion functions
directly (i.e. not though _git()).
> > I think this should address the performance concerns that
> > caused these to be removed in the first place, but it may make the
> > code uglier.
Actually it was not a performance problem, but a cleanup in a patch
series to fix a zsh-related bug. Without this cleanup the bugfix
would have been much more intrusive.
http://thread.gmane.org/gmane.comp.version-control.git/172142/focus=172369
> > I have already added wrapper functions in my bashrc so that this is no
> > longer a problem for me, but there may be other people who start
> > hitting this as well once they start using newer versions of git.
This issue was reported earlier, so it seems there are people who
would like to use it. But getting $cur, $cword, etc. variables right
in _git_<cmd>() completion functions is just part of the problem,
there are other issues, as mentioned in the previous thread:
http://thread.gmane.org/gmane.comp.version-control.git/185184/focus=185232
Unfortunately, I couldn't come up with a solution yet that doesn't
introduce too much code churn and doesn't cause yet another
inconsistency between bash and zsh. I also haven't looked whether
there are other issues similar to that with _git_fetch() mentioned on
the above link.
Best,
Gábor
^ permalink raw reply
* Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import
From: Vitor Antunes @ 2012-01-25 1:23 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: Luke Diamand, Junio C Hamano, git
In-Reply-To: <20120123224012.GA10626@padd.com>
On Mon, Jan 23, 2012 at 10:40 PM, Pete Wyckoff <pw@padd.com> wrote:
> How about taking what's below and just squashing it in. It's
> incremental on your changes and would go well with Luke's series
> that fixes a bunch of scattered quoting issues similarly.
>
> The change to "describe %s" is unnecessary, but makes all the
> invocations look similar. You can leave it out.
I've squashed your patch, but kept the "describe %s" fix in a separate
commit.
>> BTW, and on an unrelated topic, are any test cases failing on your side?
>
> I do run the tests regularly, and your series is good. There's
> the 'clone --use-client-spec' one that is broken until my
> 2ea09b5 (git-p4: adjust test to adhere to stricter useClientSpec,
> 2012-01-11) is merged. It's on pu.
Tests in t9809-git-p4-client-view.sh were failing for me because I'm
using dash instead of bash. Please check patch below for a fix.
Test 15 of t9800-git-p4-basic.sh is still failing and I've not been able
to pinpoint the problem. I can send you the logs off-list, if you want.
Thanks,
Vitor
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index c9471d5..5b0ad99 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -31,7 +31,7 @@ client_view() {
#
check_files_exist() {
ok=0 &&
- num=${#@} &&
+ num=$# &&
for arg ; do
test_path_is_file "$arg" &&
ok=$(($ok + 1))
^ permalink raw reply related
* [PATCH] git-completion: workaround zsh COMPREPLY bug
From: Felipe Contreras @ 2012-01-25 1:37 UTC (permalink / raw)
To: git; +Cc: gitster, Matthieu Moy, Felipe Contreras
zsh adds a backslash (foo\ ) for each item in the COMPREPLY array if IFS
doesn't contain spaces. This issue has been reported[1], but there is no
solution yet.
This wasn't a problem due to another bug[2], which was fixed in zsh
version 4.3.12. After this change, 'git checkout ma<tab>' would resolve
to 'git checkout master\ '.
Aditionally, the introduction of __gitcomp_nl in commit a31e626
(completion: optimize refs completion) in git also made the problem
apparent, as Matthieu Moy reported.
The simplest and most generic solution is to hide all the changes we do
to IFS, so that "foo \nbar " is recognized by zsh as "foo bar". This
works on versions of git before and after the introduction of
__gitcomp_nl (a31e626), and versions of zsh before and after 4.3.12.
Once zsh is fixed, we should conditionally disable this workaround to
have the same benefits as bash users.
[1] http://www.zsh.org/mla/workers/2012/msg00053.html
[2] http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=2e25dfb8fd38dbef0a306282ffab1d343ce3ad8d
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
contrib/completion/git-completion.bash | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b0062ba..c83c734 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2631,6 +2631,10 @@ _git ()
# workaround zsh's bug that leaves 'words' as a special
# variable in versions < 4.3.12
typeset -h words
+
+ # another workaround for zsh because it would quote spaces in
+ # the COMPREPLY array if IFS doesn't contain spaces
+ typeset -h IFS
fi
local cur words cword prev
@@ -2687,6 +2691,10 @@ _gitk ()
# workaround zsh's bug that leaves 'words' as a special
# variable in versions < 4.3.12
typeset -h words
+
+ # another workaround for zsh because it would quote spaces in
+ # the COMPREPLY array if IFS doesn't contain spaces
+ typeset -h IFS
fi
local cur words cword prev
--
1.7.8.rc1.14.g248db
^ permalink raw reply related
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