* Re: Query on git commit amend
From: Jeff King @ 2011-12-06 19:11 UTC (permalink / raw)
To: Vijay Lakshminarayanan; +Cc: Viresh Kumar, git@vger.kernel.org, Shiraz HASHIM
In-Reply-To: <87fwgxwvn9.fsf@gmail.com>
On Tue, Dec 06, 2011 at 09:16:18PM +0530, Vijay Lakshminarayanan wrote:
> I've found
>
> $ GIT_EDITOR=cat git commit --amend
>
> useful.
>
> The benefit of this technique is that it even works for git-rebase -i.
I sometimes do a similar thing, but I don't use "cat". That will dump
all of the log message (including the generated template) to stdout
(i.e., the terminal), which is quite noisy. Instead, I use:
GIT_EDITOR=true git commit --amend
which silently leaves the file untouched.
-Peff
^ permalink raw reply
* Re: Query on git commit amend
From: Dirk Süsserott @ 2011-12-06 19:09 UTC (permalink / raw)
To: Konstantin Khomoutov; +Cc: Viresh Kumar, git@vger.kernel.org, Shiraz HASHIM
In-Reply-To: <20111206130138.119db519.kostix@domain007.com>
Am 06.12.2011 10:01 schrieb Konstantin Khomoutov:
> On Tue, 6 Dec 2011 13:53:00 +0530
> Viresh Kumar <viresh.kumar@st.com> wrote:
>
>> Suppose i want to add few new changes to my last commit (HEAD).
>> The way i do it is
>> $ git add all_changed_files
>> $ git commit --amend
>>
>> OR
>> $ git commit --amend -a
>>
>> With both these ways, i get a screen to edit the message too.
>>
>> I want to know if there is a way to skip this screen.
>>
>> i.e.
>> $ git commit --amend -a -some_other_option
>>
>> which simply adds new changes to existing commit, without asking to
>> change message.
>>
>> If there is no such way, then can we add a patch for this, if it
>> looks a valid case.
> git commit --amend -C HEAD
$ git commit --amend -C HEAD
works fine but will keep the authorship (name _and_ date). To change the
date to the current timestamp, use
$ git commit --amend -C HEAD --reset-author
Note that this will also change the author's name to yours, so it
depends on your case. The commiter's name and timestamp are always
updated to "you/now", independently of that option. To change only the
author's date, use --date=<date>.
Cheers,
Dirk
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)
From: Junio C Hamano @ 2011-12-06 19:12 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8AmwQxcKz9vhtvFJPPKpXeipufD_0OqoMv41SHQFZgqeQ@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> I thought this would be replaced by a new version [1] I posted a while
> ago (and forgot to address comments). Do you want me to rebase that on
> top of this or replace it?
I think what was merged to 'master' is already sane. If you have updates,
treat them just like other new topics.
Thanks.
^ permalink raw reply
* BUG: Confusing submodule error message
From: Seth Robertson @ 2011-12-06 19:30 UTC (permalink / raw)
To: git
Someone on #git just encountered a problem where `git init && git add . &&
git status` was failing with a message about a corrupted index.
error: bad index file sha1 signature
fatal: index file corrupt
fatal: git status --porcelain failed
This confused everyone for a while, until he provided access to the
directory to play with. I eventually tracked it down to a directory
in the tree which already had a .git directory in it. Unfortunately,
that .git repo was corrupted and was the one returning the message
about a corrupted index. The problem is that the error message we
were seeing did not provide any direct hints that submodules were
involved or that the problem was not at the top level (`git status
--porcelain` is admittedly an indirect hint to both). Here is a
recipe to reproduce a similar problem:
(mkdir -p z/foo; cd z/foo; git init; echo A>A; git add A; git commit -m A; cd ..; echo B>B; rm -f foo/.git/objects/*/*; git init; git add .; git status)
Providing an expanded error message which clarifies that this is
failing in a submodule directory makes everything clear.
----------------------------------------------------------------------
--- submodule.c~ 2011-12-02 14:25:08.000000000 -0500
+++ submodule.c 2011-12-06 14:13:00.554413432 -0500
@@ -714,7 +714,7 @@
close(cp.out);
if (finish_command(&cp))
- die("git status --porcelain failed");
+ die("git status --porcelain failed in submodule directory %s", path);
strbuf_release(&buf);
return dirty_submodule;
----------------------------------------------------------------------
Do more error messages in submodule.c need adjusting? It seems likely.
-Seth Robertson
^ permalink raw reply
* Re: Query on git commit amend
From: Junio C Hamano @ 2011-12-06 20:03 UTC (permalink / raw)
To: Vijay Lakshminarayanan; +Cc: Viresh Kumar, git@vger.kernel.org, Shiraz HASHIM
In-Reply-To: <87fwgxwvn9.fsf@gmail.com>
Vijay Lakshminarayanan <laksvij@gmail.com> writes:
> I've found
>
> $ GIT_EDITOR=cat git commit --amend
>
> useful.
Are you sure it is a cat?
I almost always use
$ EDITOR=: git commit --amend
when rewriting the contents without updating the message, but I think
we should allow people to say:
$ git commit --amend --no-edit
which is accepted from the command line but is not honoured.
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: Thomas Rast @ 2011-12-06 20:17 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20111206190217.GD9492@sigill.intra.peff.net>
Jeff King wrote:
> On Tue, Dec 06, 2011 at 05:35:08PM +0100, Thomas Rast wrote:
>
> > The cpp pattern, used for C and C++, would not match the start of a
> > declaration such as
> >
> > static char *prepare_index(int argc,
> >
> > because it did not allow for * anywhere between the various words that
> > constitute the modifiers, type and function name. Fix it.
> >
> > Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> > ---
> >
> > This is a really sneaky one-character bug that I cannot believe went
> > unnoticed for so long, seeing as there are plenty of instances within
> > git itself where it matters.
>
> Looks reasonable to me. You can see the difference, for instance, with:
>
> git show -U1 3c73a1d
>
> (The -U1 is because of the annoying "we will start looking for the
> header at the top of context, not the top of changes" behavior I
> mentioned last week).
Actually (sadly) I'll have to revise it. It doesn't match much of C++
either, and I haven't yet come up with a reasonable regex that
matches, say,
foo::Bar<int>::t& Baz::operator<<(
which I would call ludicrous, but it's valid C++.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: Jeff King @ 2011-12-06 20:19 UTC (permalink / raw)
To: Thomas Rast; +Cc: Junio C Hamano, git
In-Reply-To: <201112062117.57690.trast@student.ethz.ch>
On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:
> > Looks reasonable to me. You can see the difference, for instance, with:
> >
> > git show -U1 3c73a1d
> >
> > (The -U1 is because of the annoying "we will start looking for the
> > header at the top of context, not the top of changes" behavior I
> > mentioned last week).
>
> Actually (sadly) I'll have to revise it. It doesn't match much of C++
> either, and I haven't yet come up with a reasonable regex that
> matches, say,
>
> foo::Bar<int>::t& Baz::operator<<(
>
> which I would call ludicrous, but it's valid C++.
Ick, yeah. Maybe it is worth doing the "*" thing for now, and then
worrying about advanced C++ stuff on top as another patch. AFAICT, your
original patch is a strict improvement.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)
From: Ramkumar Ramachandra @ 2011-12-06 20:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
In-Reply-To: <7vliqph8a6.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano wrote:
> A few comments and thoughts, all minor.
>
> * On "revert: simplify getting commit subject in format_todo()"
>
> The old code used to use get_message() on cur->item, checked its return
> value (if cur->item is not parsed, or has already been used and its buffer
> cleared, cur->item->buffer would be NULL and get_message() returns -1) and
> issued an error. The new code uses find_commit_subject without first
> checking if cur->item->buffer is NULL.
>
> Does this mean the old code was overly defensive, or is the new code too
> lax?
>
> I understand that parse_insn_line() uses lookup_commit_reference() which
> calls parse_object() on the object name (and if it is a tag, deref_tag()
> will parse the tagged object until we see something that is not a tag), so
> we know cur->item is parsed before we see it, so I suspect you merely were
> being overly defensive, but I may be missing something.
Right. Actually, I was being overly defensive because I was being
lazy about having to deal the an empty-commit-subject case in the
parser. With "revert: make commit subjects in insn sheet optional",
that's no more a concern- I'll reorder these two patches, and explain
this detail in the commit message in the re-roll.
> * On "revert: make commit subjects in insn sheet optional"
>
> After finding the verb and advancing the pointer "bol" at the beginning of
> the object name, end_of_object_name variable points at the first SP or LF
> using strcspn(bol, " \n"), but I wonder why we are not grabbing a run of
> hexdigits instead, i.e. strspn(bol, "0123456789abcdef"). Is this so that
> we can allow something like "pick rr/revert-cherry-pick~3"?
Yes, it is exactly for that reason :)
> I also wonder if this should be (sorry for pcre) "(pick|revert)\s+(\S+)\s"
> instead, i.e. allow users with fat fingers to use one or more SP or even HT
> to separate the verb and the operand.
Hm, I'm not too enthusiastic about this change, because we don't
advertise hand-editing the instruction sheet yet: it's just some extra
parsing cruft along with guardian buffer-overflow code that buys us no
immediate benefits.
> The last test you added to 3510 in this patch runs test_line_count
> unconditionally, by the way.
Good catch. Missing the chaining '&&' seems to be quite a common
error: I vaguely remember seeing a patch to detect this sometime ago.
Jonathan?
> * On "revert: allow mixed pick and revert instructions"
>
> Reporting what we did not understand from parse_insn_line() is a good
> idea, but I think the line number should be reported at the beginning
> of the same line.
Makes sense. Do you like this?
diff --git a/builtin/revert.c b/builtin/revert.c
index e447449..cfa770e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -715,7 +715,8 @@ static int format_todo(struct strbuf *buf, struct
replay_insn_list *todo_list)
return 0;
}
-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
+static int parse_insn_line(char *bol, char *eol,
+ struct replay_insn_list *item, int lineno)
{
unsigned char commit_sha1[20];
char *end_of_object_name;
@@ -731,7 +732,8 @@ static int parse_insn_line(char *bol, char *eol,
struct replay_insn_list *item)
size_t len = strchrnul(bol, '\n') - bol;
if (len > 255)
len = 255;
- return error(_("Unrecognized action: %.*s"), (int)len, bol);
+ return error(_("Line %d: Unrecognized action: %.*s"),
+ lineno, (int)len, bol);
}
end_of_object_name = bol + strcspn(bol, " \n");
@@ -741,11 +743,11 @@ static int parse_insn_line(char *bol, char *eol,
struct replay_insn_list *item)
*end_of_object_name = saved;
if (status < 0)
- return error(_("Malformed object name: %s"), bol);
+ return error(_("Line %d: Malformed object name: %s"), lineno, bol);
item->operand = lookup_commit_reference(commit_sha1);
if (!item->operand)
- return error(_("Not a valid commit: %s"), bol);
+ return error(_("Line %d: Not a valid commit: %s"), lineno, bol);
item->next = NULL;
return 0;
@@ -760,8 +762,8 @@ static int parse_insn_buffer(char *buf, struct
replay_insn_list **todo_list)
for (i = 1; *p; i++) {
char *eol = strchrnul(p, '\n');
- if (parse_insn_line(p, eol, &item) < 0)
- return error(_("on line %d."), i);
+ if (parse_insn_line(p, eol, &item, lineno) < 0)
+ return -1;
next = replay_insn_list_append(item.action, item.operand, next);
p = *eol ? eol + 1 : eol;
}
--
> I'd say that I'd prefer queuing re-rolled patches posted on the list; I'll
> discard the rr/revert-cherry-pick (6a156fd) from my tree.
Thanks for letting me know -- will post shortly.
-- Ram
^ permalink raw reply related
* ANNOUNCE: Git for Windows 1.7.8
From: Pat Thoyts @ 2011-12-06 20:32 UTC (permalink / raw)
To: msysGit, Git Mailing List
This release brings the latest release of Git to Windows users.
Pre-built installers are available from
http://code.google.com/p/msysgit/downloads/list
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: Johannes Sixt @ 2011-12-06 20:52 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Rast, Junio C Hamano, git
In-Reply-To: <20111206201944.GB27930@sigill.intra.peff.net>
Am 06.12.2011 21:19, schrieb Jeff King:
> On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:
>
>>> Looks reasonable to me. You can see the difference, for instance, with:
>>>
>>> git show -U1 3c73a1d
>>>
>>> (The -U1 is because of the annoying "we will start looking for the
>>> header at the top of context, not the top of changes" behavior I
>>> mentioned last week).
>>
>> Actually (sadly) I'll have to revise it. It doesn't match much of C++
>> either, and I haven't yet come up with a reasonable regex that
>> matches, say,
>>
>> foo::Bar<int>::t& Baz::operator<<(
>>
>> which I would call ludicrous, but it's valid C++.
>
> Ick, yeah. Maybe it is worth doing the "*" thing for now, and then
> worrying about advanced C++ stuff on top as another patch. AFAICT, your
> original patch is a strict improvement.
Excuse me, where's the problem? The above example shows this
@@ -105,8 +105,8 @@ char *url_decode(const char *url)
struct strbuf out = STRBUF_INIT;
- const char *slash = strchr(url, '/');
+ const char *colon = strchr(url, ':');
...
with current 4cb5d10b. This looks quite correct, no?
-- Hannes
^ permalink raw reply
* Re: Query on git commit amend
From: Junio C Hamano @ 2011-12-06 21:00 UTC (permalink / raw)
To: git; +Cc: Vijay Lakshminarayanan, Viresh Kumar, Shiraz HASHIM
In-Reply-To: <7vobvlfowk.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I almost always use
>
> $ EDITOR=: git commit --amend
>
> when rewriting the contents without updating the message, but I think
> we should allow people to say:
>
> $ git commit --amend --no-edit
>
> which is accepted from the command line but is not honoured.
And this should fix it (only lightly tested).
-- >8 --
Subject: [PATCH] commit: honor --no-edit
After making fixes to the contents to be committed, it is not unusual to
update the current commit without rewording the message. Idioms to do
tell "commit --amend" that we do not need an editor have been:
$ EDITOR=: git commit --amend
$ git commit --amend -C HEAD
but that was only because a more natural
$ git commit --amend --no-edit
did not honour "--no-edit" option.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/commit.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 8f2bebe..48bea8f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -81,7 +81,8 @@ static const char *template_file;
static const char *author_message, *author_message_buffer;
static char *edit_message, *use_message;
static char *fixup_message, *squash_message;
-static int all, edit_flag, also, interactive, patch_interactive, only, amend, signoff;
+static int all, also, interactive, patch_interactive, only, amend, signoff;
+static int edit_flag = -1; /* unspecified */
static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
static int no_post_rewrite, allow_empty_message;
static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
@@ -141,7 +142,7 @@ static struct option builtin_commit_options[] = {
OPT_BOOLEAN(0, "reset-author", &renew_authorship, "the commit is authored by me now (used with -C-c/--amend)"),
OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
OPT_FILENAME('t', "template", &template_file, "use specified template file"),
- OPT_BOOLEAN('e', "edit", &edit_flag, "force edit of commit"),
+ OPT_BOOL('e', "edit", &edit_flag, "force edit of commit"),
OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
OPT_BOOLEAN(0, "status", &include_status, "include status in commit message template"),
/* end commit message options */
@@ -1020,8 +1021,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
if (logfile || message.len || use_message || fixup_message)
use_editor = 0;
- if (edit_flag)
- use_editor = 1;
+ if (0 <= edit_flag)
+ use_editor = edit_flag;
if (!use_editor)
setenv("GIT_EDITOR", ":", 1);
--
1.7.8.157.g03e55
^ permalink raw reply related
* Re: ANNOUNCE: Git for Windows 1.7.8
From: Erik Faye-Lund @ 2011-12-06 21:03 UTC (permalink / raw)
To: Pat Thoyts; +Cc: msysGit, Git Mailing List
In-Reply-To: <CABNJ2GJ_1Nf9rWev6BKE9zcqt5yrgq6PbaMLVRD705UapLHf0w@mail.gmail.com>
On Tue, Dec 6, 2011 at 9:32 PM, Pat Thoyts <patthoyts@gmail.com> wrote:
> This release brings the latest release of Git to Windows users.
>
> Pre-built installers are available from
> http://code.google.com/p/msysgit/downloads/list
Great work, thanks a lot!
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: René Scharfe @ 2011-12-06 21:07 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Thomas Rast, Junio C Hamano, git
In-Reply-To: <4EDE8086.9080303@kdbg.org>
Am 06.12.2011 21:52, schrieb Johannes Sixt:
> Am 06.12.2011 21:19, schrieb Jeff King:
>> On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:
>>
>>>> Looks reasonable to me. You can see the difference, for instance, with:
>>>>
>>>> git show -U1 3c73a1d
>>>>
>>>> (The -U1 is because of the annoying "we will start looking for the
>>>> header at the top of context, not the top of changes" behavior I
>>>> mentioned last week).
>>>
>>> Actually (sadly) I'll have to revise it. It doesn't match much of C++
>>> either, and I haven't yet come up with a reasonable regex that
>>> matches, say,
>>>
>>> foo::Bar<int>::t& Baz::operator<<(
>>>
>>> which I would call ludicrous, but it's valid C++.
>>
>> Ick, yeah. Maybe it is worth doing the "*" thing for now, and then
>> worrying about advanced C++ stuff on top as another patch. AFAICT, your
>> original patch is a strict improvement.
>
> Excuse me, where's the problem? The above example shows this
>
> @@ -105,8 +105,8 @@ char *url_decode(const char *url)
> struct strbuf out = STRBUF_INIT;
> - const char *slash = strchr(url, '/');
> + const char *colon = strchr(url, ':');
> ...
>
> with current 4cb5d10b. This looks quite correct, no?
That's with the default heuristic; try something like this first to turn
on userdiff:
$ echo url.c diff=cpp >>.gitattributes
René
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)
From: Luke Diamand @ 2011-12-06 21:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8vmqi98f.fsf@alter.siamese.dyndns.org>
On 06/12/11 05:01, Junio C Hamano wrote:
> Here are the topics that have been cooking. Commits prefixed with '-' are
> only in 'pu' (proposed updates) while commits prefixed with '+' are in
> 'next'.
>
>
> Will merge to 'next'.
>
> * ld/p4-labels-branches (2011-11-30) 4 commits
> - git-p4: importing labels should cope with missing owner
> - git-p4: add test for p4 labels
> - git-p4: cope with labels with empty descriptions
> - git-p4: handle p4 branches and labels containing shell chars
>
> I understand this has been retracted---please correct me otherwise.
> Will discard, expecting a reroll.
Yes, discard this one and I'll re-roll it with fixes for the other label
issues.
Thanks
Luke
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: Junio C Hamano @ 2011-12-06 21:15 UTC (permalink / raw)
To: Thomas Rast; +Cc: Jeff King, git
In-Reply-To: <201112062117.57690.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> Actually (sadly) I'll have to revise it. It doesn't match much of C++
> either, and I haven't yet come up with a reasonable regex that
> matches, say,
>
> foo::Bar<int>::t& Baz::operator<<(
>
> which I would call ludicrous, but it's valid C++.
Heh, I'd rather not see us go that route, which would either end up
implementing a C++ parser or reverting the heuristics back to "non-blank
at the beginning of the line" that was already reasonably useful.
^ permalink raw reply
* Re: Auto update submodules after merge and reset
From: Jens Lehmann @ 2011-12-06 21:32 UTC (permalink / raw)
To: Max Krasnyansky; +Cc: git
In-Reply-To: <4EDD6A8C.40008@qualcomm.com>
Am 06.12.2011 02:06, schrieb Max Krasnyansky:
> On 11/30/2011 12:31 AM, Jens Lehmann wrote:
>> I'm working on a patch series to teach Git to optionally update the submodules work trees on checkout, reset merge and so on, but I'm not there yet.
> Everything you suggested sounds great. We're on the same page (config option, etc).
> How far along are you? Do you have a tree I could pull from to play with things?
> I could help with testing, bug fixes and/or implementing parts of it. Let me know.
Great to hear that! Please see my GitHub repo for the current state:
https://github.com/jlehmann/git-submod-enhancements
It has two interesting branches:
git-checkout-recurse-submodules:
This was my first attempt to tell unpack_trees() to checkout submodules
and works quite well. Porcelain checks out submodules by default while
plumbing learned the --recurse-submodules option to do that (and git gui
and gitk use that option so stuff like "Revert Changes" does work on
submodules :-). I use it at work for some time and it works quite well,
but doesn't handle new or deleted submodules. And unfortunately the way
I added the flag to control submodule checkout doesn't allow to add a
per-submodule configuration option.
recursive_submodule_checkout:
This is where new development happens. I added the basic infrastructure
to have global and per-submodule configuration controlling the checkout
and ported the unpack_trees() changes from git-checkout-recurse-submodules
here. I also added removal and creation of submodules based on the now
moved gitdir. This branch has rudimentary tests but still needs quite some
work.
I expect to have some time around the end of year to move things forward.
It'd be cool if you could check the current state, after that we can
decide how to move the topic forward together.
> For now I implemented automatic submodules update using 'post-merge' hook. But obviously it does
> not handle reset and things. I'm thinking of adding 'post-reset' and 'pre-merge' that would be useful
> for this and maybe other things.
I doubt hooks can be more than a band aid for submodule checkout. I thought
about doing that too and came to the conclusion it will only handle some of
the issues. And you'll have to provide a real life use case to get a new
hook accepted ;-)
^ permalink raw reply
* [PATCH 0/2] run-command: Add EACCES diagnostics
From: Frans Klaver @ 2011-12-06 21:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <op.v5e8mgbc0aolir@keputer>
So here's a couple of patches that introduce some more elaborate investigation
into what went wrong when receiving EACCES. This is probably something that
could be expanded in the future, as running a command doesn't always produce
equally obvious error messages.
"run-command: Add checks after execvp fails with EACCES" provides some basic checks
on the permissions in PATH, and gives just a warning that none of its checks
indicate a problem, so the user should check at least the interpreter permissions.
"run-command: Add interpreter permissions check" actually adds interpreter checking.
---
run-command.c | 172 ++++++++++++++++++++++++++++++++++++++++++++++++
t/t0061-run-command.sh | 38 ++++++++++-
2 files changed, 209 insertions(+), 1 deletions(-)
^ permalink raw reply
* [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Frans Klaver @ 2011-12-06 21:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Frans Klaver
In-Reply-To: <1323207503-26581-1-git-send-email-fransklaver@gmail.com>
execvp returns ENOENT if a command was not found after searching PATH.
If path contains a directory that current user has insufficient
privileges to, EACCES is returned. This may still mean the program
wasn't found and may cause confusion to the user, especially when the
file mentioned doesn't exist -- that is, the user would expect NOENT to
be returned -- and the user was actually hoping for an alias to be executed.
To help users track down the core issue more easily, perform some checks
on the path and file permissions involved. Output errors when paths or
files don't have enough permissions.
Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
run-command.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
t/t0061-run-command.sh | 16 ++++++-
2 files changed, 133 insertions(+), 1 deletions(-)
diff --git a/run-command.c b/run-command.c
index 1c51043..5e38c5a 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,119 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
return code;
}
+#ifndef WIN32
+static int is_in_group(gid_t gid)
+{
+ gid_t *groups;
+ int ngroups, gc;
+ int yes;
+
+ if (gid == getgid())
+ return 1;
+
+ groups = NULL;
+ ngroups = getgroups(0, NULL);
+ if (ngroups > 0) {
+ groups = (gid_t *)xmalloc(ngroups * sizeof(gid_t));
+ if (getgroups(ngroups, groups) < 0) {
+ free(groups);
+ return 0;
+ }
+ }
+
+ yes = 0;
+ for (gc = 0; gc < ngroups; gc++)
+ if (groups[gc] == gid)
+ yes = 1;
+
+ free(groups);
+ return yes;
+}
+
+static int have_read_execute_permissions(const char *path)
+{
+ struct stat s;
+ trace_printf("checking '%s'\n", path);
+
+ if (stat(path, &s) < 0) {
+ trace_printf("could not stat '%s': %s\n",
+ path, strerror(errno));
+ return 0;
+ }
+ trace_printf("uid: %d, gid: %d\n", s.st_uid, s.st_gid);
+ trace_printf("mode: %o\n", s.st_mode);
+
+ /* check world permissions */
+ if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
+ return 1;
+
+ /* check group permissions & membership */
+ if ((s.st_mode&(S_IXGRP|S_IRGRP)) == (S_IXGRP|S_IRGRP) &&
+ is_in_group(s.st_gid))
+ return 1;
+
+ /* check owner permissions & ownership */
+ if ((s.st_mode&(S_IXUSR|S_IRUSR)) == (S_IXUSR|S_IRUSR) &&
+ s.st_uid == getuid())
+ return 1;
+
+ return 0;
+}
+
+static void diagnose_execvp_eacces(const char *cmd, const char **argv)
+{
+ /* man 2 execve states that EACCES is returned for:
+ * - Search permission is denied on a component of the path prefix
+ * of cmd or the name of a script interpreter
+ * - The file or script interpreter is not a regular file
+ * - Execute permission is denied for the file, script or ELF
+ * interpreter
+ * - The file system is mounted noexec
+ */
+ struct strbuf sb = STRBUF_INIT;
+ char *path = getenv("PATH");
+ char *next;
+
+ if (strchr(cmd, '/')) {
+ if (!have_read_execute_permissions(cmd))
+ error("no read/execute permissions on '%s'\n", cmd);
+ return;
+ }
+
+ for (;;) {
+ next = strchrnul(path, ':');
+ if (path < next)
+ strbuf_add(&sb, path, next - path);
+ else
+ strbuf_addch(&sb, '.');
+
+ if (!have_read_execute_permissions(sb.buf))
+ error("no read/execute permissions on '%s'\n", sb.buf);
+
+ if (sb.len && sb.buf[sb.len - 1] != '/')
+ strbuf_addch(&sb, '/');
+ strbuf_addstr(&sb, cmd);
+
+ if (file_exists(sb.buf)) {
+ if (!have_read_execute_permissions(sb.buf))
+ error("no read/execute permissions on '%s'\n",
+ sb.buf);
+ else
+ warn("file '%s' exists and permissions "
+ "seem OK.\nIf this is a script, see if you "
+ "have sufficient privileges to run the "
+ "interpreter", sb.buf);
+ }
+
+ strbuf_release(&sb);
+
+ if (!*next)
+ break;
+ path = next + 1;
+ }
+}
+#endif
+
int start_command(struct child_process *cmd)
{
int need_in, need_out, need_err;
@@ -285,6 +399,10 @@ fail_pipe:
error("cannot run %s: %s", cmd->argv[0],
strerror(ENOENT));
exit(127);
+ } else if (errno == EACCES) {
+ diagnose_execvp_eacces(cmd->argv[0], cmd->argv);
+ die("cannot exec '%s': %s", cmd->argv[0],
+ strerror(EACCES));
} else {
die_errno("cannot exec '%s'", cmd->argv[0]);
}
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 8d4938f..b39bd16 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -26,7 +26,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 +34,18 @@ 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 permisions' '
+ 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 &&
+ grep "no read/execute permissions on" err
+'
+
test_done
--
1.7.8
^ permalink raw reply related
* [PATCH 2/2] run-command: Add interpreter permissions check
From: Frans Klaver @ 2011-12-06 21:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Frans Klaver
In-Reply-To: <1323207503-26581-1-git-send-email-fransklaver@gmail.com>
If a script is started and the interpreter of that script given in the
shebang cannot be started due to permissions, we can get a rather
obscure situation. All permission checks pass for the script itself,
but we still get EACCES from execvp.
Try to find out if the above is the case and warn the user about it.
Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
run-command.c | 66 +++++++++++++++++++++++++++++++++++++++++++----
t/t0061-run-command.sh | 22 ++++++++++++++++
2 files changed, 82 insertions(+), 6 deletions(-)
diff --git a/run-command.c b/run-command.c
index 5e38c5a..b8cf8d4 100644
--- a/run-command.c
+++ b/run-command.c
@@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)
return 0;
}
+static void check_interpreter(const char *cmd)
+{
+ FILE *f;
+ struct strbuf sb = STRBUF_INIT;
+ /* bash reads an 80 character line when determining the interpreter.
+ * BSD apparently only allows 32 characters, as it is the size of
+ * your average binary executable header.
+ */
+ char firstline[80];
+ char *interpreter = NULL;
+ size_t s, i;
+
+ f = fopen(cmd, "r");
+ if (!f) {
+ error("cannot open file '%s': %s\n", cmd, strerror(errno));
+ return;
+ }
+
+ s = fread(firstline, 1, sizeof(firstline), f);
+ if (s < 2) {
+ trace_printf("cannot determine file type");
+ fclose(f);
+ return;
+ }
+
+ if (firstline[0] != '#' || firstline[1] != '!') {
+ trace_printf("file '%s' is not a script or"
+ " is a script without '#!'", cmd);
+ fclose(f);
+ return;
+ }
+
+ /* see if the given path has the executable bit set */
+ for (i = 2; i < s; i++) {
+ if (!interpreter && firstline[i] != ' ' && firstline[i] != '\t')
+ interpreter = firstline + i;
+
+ if (interpreter && (firstline[i] == ' ' ||
+ firstline[i] == '\n')) {
+ strbuf_add(&sb, interpreter,
+ (firstline + i) - interpreter);
+ break;
+ }
+ }
+ if (!sb.len) {
+ error("could not determine interpreter");
+ strbuf_release(&sb);
+ return;
+ }
+
+ if (!have_read_execute_permissions(sb.buf))
+ error("bad interpreter: no read/execute permissions on '%s'\n",
+ sb.buf);
+
+ strbuf_release(&sb);
+}
+
static void diagnose_execvp_eacces(const char *cmd, const char **argv)
{
/* man 2 execve states that EACCES is returned for:
@@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
char *next;
if (strchr(cmd, '/')) {
- if (!have_read_execute_permissions(cmd))
- error("no read/execute permissions on '%s'\n", cmd);
+ if (have_read_execute_permissions(cmd))
+ check_interpreter(cmd);
return;
}
@@ -233,10 +290,7 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
error("no read/execute permissions on '%s'\n",
sb.buf);
else
- warn("file '%s' exists and permissions "
- "seem OK.\nIf this is a script, see if you "
- "have sufficient privileges to run the "
- "interpreter", sb.buf);
+ check_interpreter(sb.buf);
}
strbuf_release(&sb);
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index b39bd16..39bfaef 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -13,6 +13,18 @@ cat >hello-script <<-EOF
EOF
>empty
+cat >someinterpreter <<-EOF
+ #!$SHELL_PATH
+ cat hello-script
+EOF
+>empty
+
+cat >incorrect-interpreter-script <<-EOF
+ #!someinterpreter
+ cat hello-script
+EOF
+>empty
+
test_expect_success 'start_command reports ENOENT' '
test-run-command start-command-ENOENT ./does-not-exist
'
@@ -48,4 +60,14 @@ test_expect_success POSIXPERM 'run_command reports EACCES, search path permision
grep "no read/execute permissions on" 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 &&
+ grep "bad interpreter" err
+'
+
test_done
--
1.7.8
^ permalink raw reply related
* Re: [PATCHv2 0/13] credential helpers
From: Junio C Hamano @ 2011-12-06 21:40 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111206062127.GA29046@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... You can now
> do: "git credential-store erase </dev/null" to erase everything
> (since you have provided no restrictions, it matches everything).
That "justification" does not sound so true to me but perhaps that is
because it is unclear what "erase" means and what it means to give the
operation parameters.
When I see "erase $foo", I would find it natural if $foo meant "if there
is something that matches $foo, then please remove it, but keep everything
else intact", and not the other way around "Match the existing entries
against a pattern (or a set of matching patterns) I am giving you, and
drop all the rest". So if I happen to give you an empty set, I would
expect nothing is removed.
Perhaps the root cause of the issue is that you are treating the input as
"restriction" instead of something that produces "positive matches"?
Confused.
^ permalink raw reply
* Odd issue - The Diffs That WILL NOT DIE.
From: Chris Patti @ 2011-12-06 21:43 UTC (permalink / raw)
To: git
I have a Homebrew installed version if Git 1.7.8 running on OSX Lion.
I'm seeing a very odd issue where these diffs I didn't create keep
recurring in a particular repository.
I've tried:
* Nuking the repo and re-cloning, cloning into a totally different
containing directory
* git reset --hard, git checkout -- of the offending file supposedly
containing the diffs
Is there some sort of uber persistent local cache that's bound to the
remote repository?
Thanks in advance,
-Chris
--
Christopher Patti - Geek At Large | GTalk: cpatti@gmail.com | AIM:
chrisfeohpatti | P: (260) 54PATTI
"Technology challenges art, art inspires technology." - John Lasseter, Pixar
^ permalink raw reply
* Re: Odd issue - The Diffs That WILL NOT DIE.
From: Frans Klaver @ 2011-12-06 21:47 UTC (permalink / raw)
To: git, Chris Patti
In-Reply-To: <CAJ8P3RBm=RhNf6LKLqprqX6Rqx0OgRnJR+=+-Qhg4PvpeqaUDg@mail.gmail.com>
On Tue, 06 Dec 2011 22:43:50 +0100, Chris Patti <cpatti@gmail.com> wrote:
> I have a Homebrew installed version if Git 1.7.8 running on OSX Lion.
>
> I'm seeing a very odd issue where these diffs I didn't create keep
> recurring in a particular repository.
Could you be a little more specific about the nature of the diffs? Is it
reproducible on another system? It sounds like newlines or whitespace. Does
$ git diff -w
produce the same result?
>
> I've tried:
>
> * Nuking the repo and re-cloning, cloning into a totally different
> containing directory
> * git reset --hard, git checkout -- of the offending file supposedly
> containing the diffs
>
> Is there some sort of uber persistent local cache that's bound to the
> remote repository?
>
> Thanks in advance,
> -Chris
>
^ permalink raw reply
* Re: Odd issue - The Diffs That WILL NOT DIE.
From: Carlos Martín Nieto @ 2011-12-06 21:51 UTC (permalink / raw)
To: Chris Patti; +Cc: git
In-Reply-To: <CAJ8P3RBm=RhNf6LKLqprqX6Rqx0OgRnJR+=+-Qhg4PvpeqaUDg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 895 bytes --]
On Tue, Dec 06, 2011 at 04:43:50PM -0500, Chris Patti wrote:
> I have a Homebrew installed version if Git 1.7.8 running on OSX Lion.
>
> I'm seeing a very odd issue where these diffs I didn't create keep
> recurring in a particular repository.
Which diffs? You haven't given us any? What files does this happen
with? Do they have any peculiarities?
If these are files with non-ASCII filenames, then you're hitting a
misfeature of the HFS+ filesystem (it lies when git asks it about
files).
>
> I've tried:
>
> * Nuking the repo and re-cloning, cloning into a totally different
> containing directory
> * git reset --hard, git checkout -- of the offending file supposedly
> containing the diffs
>
> Is there some sort of uber persistent local cache that's bound to the
> remote repository?
The remote repository shouldn't have anything to do with this.
cmn
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCHv2 12/13] credentials: add "store" helper
From: Junio C Hamano @ 2011-12-06 21:50 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111206062305.GL29233@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> +static void store_credential(const char *fn, struct credential *c)
> +{
> + struct strbuf buf = STRBUF_INIT;
> +
> + /*
> + * Sanity check that what we are storing is actually sensible.
> + * In particular, we can't make a URL without a protocol field.
> + * Without either a host or pathname (depending on the scheme),
> + * we have no primary key. And without a username and password,
> + * we are not actually storing a credential.
> + */
> + if (!c->protocol || !(c->host || c->path) ||
> + !c->username || !c->password)
> + return;
Very nicely explained. I wish all our patches had comments like this to
explain tricky bit that looks as if the choice was arbitrarily made but
in fact the logic was carefully constructed.
Thanks.
^ permalink raw reply
* Re: [PATCHv2 13/13] t: add test harness for external credential helpers
From: Junio C Hamano @ 2011-12-06 21:51 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111206062313.GM29233@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
> new file mode 100755
> index 0000000..79b046f
> --- /dev/null
> +++ b/t/t0303-credential-external.sh
> @@ -0,0 +1,23 @@
> ...
> +else
> + helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> + helper_test "$GIT_TEST_CREDENTIAL_HELPER"
> +# helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> +fi
Huh? Leftover debugging cruft?
^ 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