* [PATCH] config: don't segfault when given --path with a missing value
From: Carlos Martín Nieto @ 2012-11-15 18:10 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20121115161758.GC6157@sigill.intra.peff.net>
When given a variable without a value, such as '[section] var' and
asking git-config to treat it as a path, git_config_pathname returns
an error and doesn't modify its output parameter. show_config assumes
that the call is always successful and sets a variable to indicate
that vptr should be freed. In case of an error however, trying to do
this will cause the program to be killed, as it's pointing to memory
in the stack.
Detect the error and return immediately to avoid freeing or accessing
the uninitialed memory in the stack.
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
On Thu, Nov 15, 2012 at 08:11:50AM -0800, Jeff King wrote:
> Hmm, actually, we should probably propagate the error (I was thinking
> for some reason this was in the listing code, but it is really about
> getting a specific variable, and that variable does not have a sane
> format. We'll already have printed the non-bool error, so we should
> probably die. So more like:
>
> if (git_config_pathname(&vptr, key_, value_) < 0)
> return -1;
> must_free_vptr = 1;
Yeah, that's more sensible. I didn't notice that the buffer never gets
written to in this codepath, and the trying to print it out is silly
when we know that there is nothing valid to print. Thanks for the
review. I've included your test as well, which really makes all of
this your code. Do we have some equivalent of a Basically-writen-by
line?
builtin/config.c | 3 ++-
t/t1300-repo-config.sh | 5 +++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/builtin/config.c b/builtin/config.c
index 442ccc2..4dc5ffa 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -129,7 +129,8 @@ static int show_config(const char *key_, const char *value_, void *cb)
else
sprintf(value, "%d", v);
} else if (types == TYPE_PATH) {
- git_config_pathname(&vptr, key_, value_);
+ if (git_config_pathname(&vptr, key_, value_) < 0)
+ return -1;
must_free_vptr = 1;
} else if (value_) {
vptr = value_;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index a477453..17272e0 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -803,6 +803,11 @@ test_expect_success NOT_MINGW 'get --path copes with unset $HOME' '
test_cmp expect result
'
+test_expect_success 'get --path barfs on boolean variable' '
+ echo "[path]bool" >.git/config &&
+ test_must_fail git config --get --path path.bool
+'
+
cat > expect << EOF
[quote]
leading = " test"
--
1.8.0.316.g291341c
^ permalink raw reply related
* [PATCH v2 4/5] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Brandon Casey @ 2012-11-15 17:55 UTC (permalink / raw)
To: git; +Cc: Brandon Casey, Brandon Casey
In-Reply-To: <1352943474-15573-4-git-send-email-drafnel@gmail.com>
Currently, if the s-o-b footer of a commit message contains a
"(cherry picked from ..." line that was added by a previous cherry-pick -x,
it is not recognized as a s-o-b footer and will cause a newline to be
inserted before an additional s-o-b is added.
So, rework ends_rfc2822_footer to recognize the "(cherry picked from ..."
string as part of the footer. Plus mark the test in t3511 as fixed.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
Declare cherry_picked_prefix variable as static. This is the only change
with respect to v1.
-Brandon
sequencer.c | 44 +++++++++++++++++++++++++++++---------------
t/t3511-cherry-pick-x.sh | 2 +-
2 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 01edec2..213fa4f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -18,6 +18,7 @@
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
const char sign_off_header[] = "Signed-off-by: ";
+static const char cherry_picked_prefix[] = "(cherry picked from commit ";
static void remove_sequencer_state(void)
{
@@ -492,7 +493,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
if (opts->record_origin) {
- strbuf_addstr(&msgbuf, "(cherry picked from commit ");
+ strbuf_addstr(&msgbuf, cherry_picked_prefix);
strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
strbuf_addstr(&msgbuf, ")\n");
}
@@ -1017,13 +1018,34 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return pick_commits(todo_list, opts);
}
+static int is_rfc2822_line(const char *buf, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++) {
+ int ch = buf[i];
+ if (ch == ':')
+ break;
+ if (isalnum(ch) || (ch == '-'))
+ continue;
+ return 0;
+ }
+
+ return 1;
+}
+
+static int is_cherry_pick_from_line(const char *buf, int len)
+{
+ return (strlen(cherry_picked_prefix) + 41) <= len &&
+ !prefixcmp(buf, cherry_picked_prefix);
+}
+
static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
{
- int ch;
int hit = 0;
- int i, j, k;
+ int i, k;
int len = sb->len - ignore_footer;
- int first = 1;
+ int last_was_rfc2822 = 0;
const char *buf = sb->buf;
for (i = len - 1; i > 0; i--) {
@@ -1040,20 +1062,12 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
; /* do nothing */
k++;
- if ((buf[i] == ' ' || buf[i] == '\t') && !first)
+ if (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t'))
continue;
- first = 0;
-
- for (j = 0; i + j < len; j++) {
- ch = buf[i + j];
- if (ch == ':')
- break;
- if (isalnum(ch) ||
- (ch == '-'))
- continue;
+ if (!((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) ||
+ is_cherry_pick_from_line(buf+i, k-i)))
return 0;
- }
}
return 1;
}
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index b2098e0..785486e 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -63,7 +63,7 @@ test_expect_success 'cherry-pick -s not confused by rfc2822 continuation line' '
test_cmp expect actual
'
-test_expect_failure 'cherry-pick treats -s "(cherry picked from..." line as part of footer' '
+test_expect_success 'cherry-pick treats -s "(cherry picked from..." line as part of footer' '
pristine_detach initial &&
git cherry-pick -s rfc2822-cherry-base &&
cat <<-EOF >expect &&
--
1.8.0
^ permalink raw reply related
* Re: [PATCH] status: add advice on how to push/pull to tracking branch
From: Junio C Hamano @ 2012-11-15 17:53 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <1352976300-20159-1-git-send-email-Matthieu.Moy@imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> I thought this was obvious enough not to deserve an advice, but a
> colleague of mine had troubles with "commited but not pushed" changes.
> Maybe an additional advice would have helped. After all, it's an
> advice, and can be deactivated ...
>
> remote.c | 13 ++++++++++---
> t/t2020-checkout-detach.sh | 1 +
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 04fd9ea..9c19689 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1627,13 +1627,15 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
>
> base = branch->merge[0]->dst;
> base = shorten_unambiguous_ref(base, 0);
> - if (!num_theirs)
> + if (!num_theirs) {
> strbuf_addf(sb,
> Q_("Your branch is ahead of '%s' by %d commit.\n",
> "Your branch is ahead of '%s' by %d commits.\n",
> num_ours),
> base, num_ours);
> - else if (!num_ours)
> + strbuf_addf(sb,
> + _(" (use \"git push\" to publish your local commits)\n"));
> + } else if (!num_ours) {
The message should make it clear that the two words in double quotes
only hint what command is used to "publish your local commits" and
not to be taken as literal "here is what you exactly would type",
but I do not think that is what I would get from this if I were a
total newbie who would need this advise.
It is even more true given that this is showing an arbitrary, and
more likely than not a non-current branch, especially with the
recent move from "matching" to "simple" where a naive use of "git
push" is to push the branch that is currently checked out and no
other branches.
see 'git push --help' to learn how to publish your local commits
might be more appropriate.
> + strbuf_addf(sb,
> + _(" (use \"git pull\" to update your local branch)\n"));
> + } else {
Likewise, and the non-currentness of the branch being described is
even worse in here, as unlike "git push" that can still be used to
push a non-current branch, "git pull" is never to be used to update
local branch that is not current, which means the advice must mention
"git checkout" somewhere.
^ permalink raw reply
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
From: Jeff King @ 2012-11-15 17:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Felipe Contreras, git, Thomas Rast, Jonathan Nieder
In-Reply-To: <7vvcd6954q.fsf@alter.siamese.dyndns.org>
On Thu, Nov 15, 2012 at 08:56:37AM -0800, Junio C Hamano wrote:
> > I think a much more compelling argument/commit message for your
> > suggested patch would be:
> >
> > We currently prompt the user for the "From" address. This is an
> > inconvenience in the common case that the user has configured their
> > identity in the environment, but is meant as a safety check for when
> > git falls back to an implicitly generated identity (which may or may
> > not be valid).
> >
> > That safety check is not really necessary, though, as by default
> > send-email will prompt the user for a final confirmation before
> > sending out any message. The likelihood that a user has both bothered
> > to turn off this default _and_ not configured any identity (nor
> > checked that the automatic identity is valid) is rather low.
>
> This somehow reminds me of the first paragraph of f20f387 (commit:
> check committer identity more strictly, 2012-07-23).
>
> I never use "send-email driving format-patch" workflow myself, but I
> suspect there are people among who do so who are using --compose to
> do the cover letter of their series. Does the "confirmation as the
> last step" help them, or would they have to retype their message?
That is a good question. That confirmation step does come after they
have typed their cover letter. However, if they are using --compose,
they are dumped in their editor with something like:
From Jeff King <peff@peff.net> # This line is ignored.
GIT: Lines beginning in "GIT:" will be removed.
GIT: Consider including an overall diffstat or table of contents
GIT: for the patch you are writing.
GIT:
GIT: Clear the body content if you don't wish to send a summary.
From: Jeff King <peff@peff.net>
Subject:
In-Reply-To:
which I think would count as sufficient notice of the address being
used.
-Peff
^ permalink raw reply
* Re: [PATCH] wildmatch: correct isprint and isspace
From: "Jan H. Schönherr" @ 2012-11-15 17:13 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, rene.scharfe, Johannes Sixt, torvalds
In-Reply-To: <1352981983-22005-1-git-send-email-pclouds@gmail.com>
Am 15.11.2012 13:19, schrieb Nguyễn Thái Ngọc Duy:
> On Thu, Nov 15, 2012 at 2:30 AM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> > Nevertheless, it's unfortunate that we have an isspace() that *almost* does
> > what the widely known thing of the same name does. I'd shy away from
> > changing git's version directly, because it's used more than a hundred times
> > in the code, and estimating the impact of adding \v and \f to it.
> > Perhaps renaming it to isgitspace() is a good first step, followed by
> > adding a "standard" version of isspace() for wildmatch?
>
> There are just too many call sites of isspace() and there is a risk
> of new call sites coming in independently. So I think keeping isspace()
> as-is and using a different name for the standard version is probably
> a better choice.
After having a closer look, where wildmatch is actually used -- matching
filenames -- and I've not yet seen \v or \f in a filename, it's possibly
unnecessary to do anything about isspace() right now.
(It's probably more an issue that filenames can be localized, and we only
support unlocalized character classes.)
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 02f48f6..d4c3fda 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -486,6 +486,7 @@ extern const unsigned char sane_ctype[256];
> #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
> #define isascii(x) (((x) & ~0x7f) == 0)
> #define isspace(x) sane_istest(x,GIT_SPACE)
> +#define isspace_posix(x) (((x) >= 9 && (x) <= 13) || (x) == 32)
> #define isdigit(x) sane_istest(x,GIT_DIGIT)
> #define isalpha(x) sane_istest(x,GIT_ALPHA)
> #define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
> @@ -499,7 +500,8 @@ extern const unsigned char sane_ctype[256];
> #define isxdigit(x) (hexval_table[x] != -1)
This was from a previous patch, but maybe: "hexval_table[(unsigned char)x]"
> #define isprint(x) (sane_istest(x, GIT_ALPHA | GIT_DIGIT | GIT_SPACE | \
> GIT_PUNCT | GIT_REGEX_SPECIAL | GIT_GLOB_SPECIAL | \
> - GIT_PATHSPEC_MAGIC))
> + GIT_PATHSPEC_MAGIC) && \
> + (x) >= 32)
May I suggest the current is_print() implementation in master:
#define isprint(x) ((x) >= 0x20 && (x) <= 0x7e)
To summarize my opinion:
I no longer see a reason to correct isspace() (unless somebody with an actual
use case complains), and a more POSIXly isprint() is already in master.
=> Nothing to do. :)
Regards
Jan
^ permalink raw reply
* Re: creation of empty branches
From: Jeff King @ 2012-11-15 17:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Andrew Ardill, Angelo Borsotti, git
In-Reply-To: <7vmwyjan96.fsf@alter.siamese.dyndns.org>
On Wed, Nov 14, 2012 at 01:27:33PM -0800, Junio C Hamano wrote:
> > Instead of
> > fatal: Not a valid object name: 'master'.
> > perhaps
> > fatal: Cannot create branch 'foo' from empty branch 'master'. To
> > rename 'master' use 'git branch -m master foo'.
>
> The first new sentence is a definite improvement, but I do not think
> the advice in the second sentence is necessarily a good idea,
> because it is dubious that the user is likely to have wanted to
> rename 'master' to something else. "git branch foo master" (or its
> moral equivalent "git checkout -b foo" while on master) is a wish to
> have a history that ends in 'foo' *forked* from history of 'master',
> but because you do not even have anything on 'master' yet, you
> cannot fork the history, as you explained earlier (snipped). In
> that sense, 'empty branch' is a slight misnomer---as far as "git
> branch foo master" is concerned, the 'master' branch does not yet
> exist (and that is why we often call it an "unborn branch", not
> "empty").
>
> fatal: cannot fork master's history that does not exist yet.
>
> would be more accurate description of the situation.
I agree with most of your reasoning. I think simply using Andrew's first
sentence is a little more clear to a new user, even though it may be
less technically precise, but I don't have a strong opinion.
That still leaves "checkout -b". I do not see it as a problem that "git
branch foo" does not work whereas "git checkout -b foo" does; we
special-case the latter because it can do something sensible, whereas
there is nothing sensible for "git branch foo" to do. However, I think
it is missing one piece:
-- >8 --
Subject: checkout: print a message when switching unborn branches
When we switch to a new branch using checkout, we usually output a
message indicating what happened. However, when we switch from an unborn
branch to a new branch, we do not print anything, which may leave the
user wondering what happened.
The reason is that the unborn branch is a special case (see abe1998),
and does not follow the usual switch_branches code path. Let's add a
similar informational message to the special case to match the usual
code path.
Signed-off-by: Jeff King <peff@peff.net>
---
Two possible tweaks:
1. The message is the same as "git checkout -b" when we are actually
moving to a new branch. We could optionally mention that the branch
is empty or unborn.
2. We do not check whether the old unborn branch has the same name as
the new one. So you get "Switched to a new branch..." if you try to
checkout the same branch. We'd have to pass more information into
the special case to detect this. I don't know if we care.
builtin/checkout.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 781295b..a9c1b5a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -951,6 +951,9 @@ static int switch_unborn_to_new_branch(const struct checkout_opts *opts)
strbuf_addf(&branch_ref, "refs/heads/%s", opts->new_branch);
status = create_symref("HEAD", branch_ref.buf, "checkout -b");
strbuf_release(&branch_ref);
+ if (!opts->quiet)
+ fprintf(stderr, _("Switched to a new branch '%s'\n"),
+ opts->new_branch);
return status;
}
^ permalink raw reply related
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
From: Junio C Hamano @ 2012-11-15 16:56 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git, Thomas Rast, Jonathan Nieder
In-Reply-To: <20121115111334.GA1879@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think a much more compelling argument/commit message for your
> suggested patch would be:
>
> We currently prompt the user for the "From" address. This is an
> inconvenience in the common case that the user has configured their
> identity in the environment, but is meant as a safety check for when
> git falls back to an implicitly generated identity (which may or may
> not be valid).
>
> That safety check is not really necessary, though, as by default
> send-email will prompt the user for a final confirmation before
> sending out any message. The likelihood that a user has both bothered
> to turn off this default _and_ not configured any identity (nor
> checked that the automatic identity is valid) is rather low.
This somehow reminds me of the first paragraph of f20f387 (commit:
check committer identity more strictly, 2012-07-23).
I never use "send-email driving format-patch" workflow myself, but I
suspect there are people among who do so who are using --compose to
do the cover letter of their series. Does the "confirmation as the
last step" help them, or would they have to retype their message?
^ permalink raw reply
* Re: [PATCH v3 0/5] push: update remote tags only with force
From: Junio C Hamano @ 2012-11-15 16:50 UTC (permalink / raw)
To: Angelo Borsotti
Cc: Chris Rorvick, git, Drew Northup, Michael Haggerty, Philip Oakley,
Johannes Sixt, Kacper Kornet, Jeff King, Felipe Contreras
In-Reply-To: <CAB9Jk9DK9AWBe_cf6=-v0pyD9xdhSoYPsKmRnUOwjuAm=hewfA@mail.gmail.com>
Angelo Borsotti <angelo.borsotti@gmail.com> writes:
>> I am *not* convinced that the "refs/tags/ is the only special
>> hierarchy whose contents should not move" is a bad limitation we
>> should avoid, but if it indeed is a bad limitation, the above is one
>> possible way to think about avoiding it.
>
> What other hierarchy besides branches and tags is there? Do you have
> in mind some other that should not move?
People use their own hierarchies for various purposes that are not
pre-defined by git-core, e.g. refs/changes/, refs/pull/, etc.
Depending on the semantics the projects want out of these
hierarchies, some of them may well be considered "create-only".
^ permalink raw reply
* Re: [PATCHv2 1/8] test-lib: allow negation of prerequisites
From: Jonathan Nieder @ 2012-11-15 16:49 UTC (permalink / raw)
To: Jeff King; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
In-Reply-To: <20121115164228.GA18108@sigill.intra.peff.net>
Jeff King wrote:
> Yes. You can test it yourself with "bash t0000-basic.sh". The reason is
> that the "!" is part of history expansion, which is only enabled by
> default for interactive shells.
Nice to hear. Thanks much for looking into it.
> On Wed, Nov 14, 2012 at 11:46:58PM -0800, Jonathan Nieder wrote:
>> If it works everywhere, this patch would help me conquer my fear of
>> exclamation points in git's tests, which would be a comfort to me and
>> a very good thing.
^ permalink raw reply
* Re: [PATCHv2 1/8] test-lib: allow negation of prerequisites
From: Jeff King @ 2012-11-15 16:42 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
In-Reply-To: <20121115074658.GA8429@elie.Belkin>
On Wed, Nov 14, 2012 at 11:46:58PM -0800, Jonathan Nieder wrote:
> > +test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '
>
> I have a visceral nervousness when reading this code, from too much
> unpleasant experience of bash's csh-style !history expansion. Luckily
> bash does not treat ! specially in the '-o sh' mode used by tests.
I can understand, having been bitten by it many times myself (not only
is it counter-intuitively expanded inside double quotes, and not only
does it not do what you wanted, but it does not insert the command into
the history, so you cannot even go back to fix it easily).
> Does this feature work when running a test explicitly using
> "bash <name of test>"? That's something I do from time to time to
> figure out whether a weird behavior is shell-specific.
Yes. You can test it yourself with "bash t0000-basic.sh". The reason is
that the "!" is part of history expansion, which is only enabled by
default for interactive shells.
> If it works everywhere, this patch would help me conquer my fear of
> exclamation points in git's tests, which would be a comfort to me and
> a very good thing.
As far as I know, "!" should be safe in a non-interactive shell script.
-Peff
^ permalink raw reply
* Re: [PATCH v4 0/4] Introduce diff.submodule
From: Jeff King @ 2012-11-15 16:33 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <20121115162524.GE6157@sigill.intra.peff.net>
On Thu, Nov 15, 2012 at 08:25:26AM -0800, Jeff King wrote:
> > Thanks, this version looks good to me.
>
> Oh wait. I did not look closely enough. The point was to move the option
> parser _out_ of git_diff_ui_config into git_diff_basic_config, so that
> it only triggers for porcelain, not plumbing.
Oh no, I am having a muddled morning. It _is_ right. We want to move it
out of basic into ui. Sorry for the noise. I just woke up and am
thinking backwards.
It may be worth squashing this test into patch 3:
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index e401814..023439f 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -72,6 +72,21 @@ EOF
test_cmp expected actual
"
+test_expect_success 'diff.submodule does not affect plumbing' '
+ test_config diff.submodule log &&
+ git diff-index -p HEAD >actual &&
+ cat >expected <<-EOF &&
+ diff --git a/sm1 b/sm1
+ new file mode 160000
+ index 0000000..a2c4dab
+ --- /dev/null
+ +++ b/sm1
+ @@ -0,0 +1 @@
+ +Subproject commit $fullhead1
+ EOF
+ test_cmp expected actual
+'
+
commit_file sm1 &&
head2=$(add_file sm1 foo3)
BTW, while writing the test, I noticed two minor nits with your tests:
1. They can use test_config, which is simpler (you do not need to
unset yourself after the test) and safer (the unset happens via
test_when_finished, so it works even if the test fails).
2. You can still indent expected output when using <<-.
I don't know if it is worth re-rolling for them.
-Peff
^ permalink raw reply related
* Re: [PATCH v4 0/4] Introduce diff.submodule
From: Jeff King @ 2012-11-15 16:25 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <20121115162331.GD6157@sigill.intra.peff.net>
On Thu, Nov 15, 2012 at 08:23:34AM -0800, Jeff King wrote:
> On Tue, Nov 13, 2012 at 09:12:43PM +0530, Ramkumar Ramachandra wrote:
>
> > v1 is here: http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
> > v2 is here: http://mid.gmane.org/1351766630-4837-1-git-send-email-artagnon@gmail.com
> > v3 is here: http://mid.gmane.org/1352653146-3932-1-git-send-email-artagnon@gmail.com
> >
> > This version was prepared in response to Peff's review of v3.
> > What changed:
> > * Functional code simplified and moved to git_diff_ui_config.
> > * Peff contributed one additional patch to the series.
>
> Thanks, this version looks good to me.
Oh wait. I did not look closely enough. The point was to move the option
parser _out_ of git_diff_ui_config into git_diff_basic_config, so that
it only triggers for porcelain, not plumbing.
-Peff
^ permalink raw reply
* Re: [PATCH v4 0/4] Introduce diff.submodule
From: Jeff King @ 2012-11-15 16:23 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <1352821367-3611-1-git-send-email-artagnon@gmail.com>
On Tue, Nov 13, 2012 at 09:12:43PM +0530, Ramkumar Ramachandra wrote:
> v1 is here: http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
> v2 is here: http://mid.gmane.org/1351766630-4837-1-git-send-email-artagnon@gmail.com
> v3 is here: http://mid.gmane.org/1352653146-3932-1-git-send-email-artagnon@gmail.com
>
> This version was prepared in response to Peff's review of v3.
> What changed:
> * Functional code simplified and moved to git_diff_ui_config.
> * Peff contributed one additional patch to the series.
Thanks, this version looks good to me.
-Peff
^ permalink raw reply
* Re: [PATCH] config: don't segfault when given --path with a missing value
From: Jeff King @ 2012-11-15 16:18 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git
In-Reply-To: <20121115161149.GB6157@sigill.intra.peff.net>
On Thu, Nov 15, 2012 at 08:11:50AM -0800, Jeff King wrote:
> Hmm, actually, we should probably propagate the error (I was thinking
> for some reason this was in the listing code, but it is really about
> getting a specific variable, and that variable does not have a sane
> format. We'll already have printed the non-bool error, so we should
> probably die. So more like:
>
> if (git_config_pathname(&vptr, key_, value_) < 0)
> return -1;
> must_free_vptr = 1;
You may want to squash in this test, which triggers your original
problem, but also demonstrates the use of uninitialized memory (although
you need to run under valgrind or similar to reliably notice it).
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index e127f35..7c4c372 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -803,6 +803,11 @@ test_expect_success NOT_MINGW 'get --path copes with unset $HOME' '
test_cmp expect result
'
+test_expect_success 'get --path barfs on boolean variable' '
+ echo "[path]bool" >.git/config &&
+ test_must_fail git config --get --path path.bool
+'
+
cat > expect << EOF
[quote]
leading = " test"
-Peff
^ permalink raw reply related
* Re: [PATCH] config: don't segfault when given --path with a missing value
From: Jeff King @ 2012-11-15 16:11 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git
In-Reply-To: <20121115160847.GA6157@sigill.intra.peff.net>
On Thu, Nov 15, 2012 at 08:08:49AM -0800, Jeff King wrote:
> That is definitely the right thing to do. But do we also need to take
> note of the error for later? After this code:
>
> > } else if (types == TYPE_PATH) {
> > - git_config_pathname(&vptr, key_, value_);
> > - must_free_vptr = 1;
> > + must_free_vptr = !git_config_pathname(&vptr, key_, value_);
>
> We don't have any clue that nothing got written into vptr. Which means
> it still points at the stack buffer "value", which contains
> uninitialized bytes. We will later try to print it, thinking it has the
> expanded path in it.
>
> Do we need something like:
>
> if (!git_config_pathname(&vptr, key_, value_))
> must_free_vptr = 1;
> else
> vptr = "";
Hmm, actually, we should probably propagate the error (I was thinking
for some reason this was in the listing code, but it is really about
getting a specific variable, and that variable does not have a sane
format. We'll already have printed the non-bool error, so we should
probably die. So more like:
if (git_config_pathname(&vptr, key_, value_) < 0)
return -1;
must_free_vptr = 1;
-Peff
^ permalink raw reply
* Re: [PATCH] config: don't segfault when given --path with a missing value
From: Jeff King @ 2012-11-15 16:08 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git
In-Reply-To: <1352868604-20459-1-git-send-email-cmn@elego.de>
On Tue, Nov 13, 2012 at 08:50:04PM -0800, Carlos Martín Nieto wrote:
> When given a variable without a value, such as '[section] var' and
> asking git-config to treat it as a path, git_config_pathname returns
> an error and doesn't modify its output parameter. show_config assumes
> that the call is always successful and sets a variable to indicate
> that vptr should be freed. In case of an error however, trying to do
> this will cause the program to be killed, as it's pointing to memory
> in the stack.
Whoops.
> Set the must_free_vptr flag depending on the return value of
> git_config_pathname so it's accurate.
That is definitely the right thing to do. But do we also need to take
note of the error for later? After this code:
> } else if (types == TYPE_PATH) {
> - git_config_pathname(&vptr, key_, value_);
> - must_free_vptr = 1;
> + must_free_vptr = !git_config_pathname(&vptr, key_, value_);
We don't have any clue that nothing got written into vptr. Which means
it still points at the stack buffer "value", which contains
uninitialized bytes. We will later try to print it, thinking it has the
expanded path in it.
Do we need something like:
if (!git_config_pathname(&vptr, key_, value_))
must_free_vptr = 1;
else
vptr = "";
?
-Peff
^ permalink raw reply
* Re: [regression] Newer gits cannot clone any remote repos
From: Nguyen Thai Ngoc Duy @ 2012-11-15 12:51 UTC (permalink / raw)
To: Douglas Mencken; +Cc: Ramsay Jones, git
In-Reply-To: <CACYvZ7jMC5xw4LxiuG5m+=grpQEg+wZb_7BaU4Xn-r7ix=S-bw@mail.gmail.com>
On Wed, Nov 14, 2012 at 2:55 AM, Douglas Mencken <dougmencken@gmail.com> wrote:
>> Could you try re-building git with the
>> NO_THREAD_SAFE_PREAD build variable set?
>
> Yeah! It works!!!
>
> --- evil/Makefile
> +++ good/Makefile
> @@ -957,6 +957,7 @@
> HAVE_PATHS_H = YesPlease
> LIBC_CONTAINS_LIBINTL = YesPlease
> HAVE_DEV_TTY = YesPlease
> + NO_THREAD_SAFE_PREAD = YesPlease
> endif
> ifeq ($(uname_S),GNU/kFreeBSD)
> NO_STRLCPY = YesPlease
>
> With this, I do have correctly working git clone.
Sorry you had to figure that out the hard way. Could you make it a
proper patch? I'm surprised that Linux pread does not behave the same
way across platforms though. Or maybe it only happens with certain
Linux versions. What version are you using?
--
Duy
^ permalink raw reply
* Re: use cases for git namespaces
From: Sitaram Chamarty @ 2012-11-15 12:39 UTC (permalink / raw)
To: Git Mailing List
In-Reply-To: <CAMK1S_gczLajro0aZ5ftUmt_vhxA+yAr_5pCZknJ2bxhykYRXQ@mail.gmail.com>
On Thu, Nov 15, 2012 at 2:03 PM, Sitaram Chamarty <sitaramc@gmail.com> wrote:
> Hi,
>
> It seems to me that whatever namespaces can do, can functionally be
> done using just a subdirectory of branches. The only real
> differences I can see are (a) a client sees less branch clutter, and
> (b) a fetch/clone pulls down less if the big stuff is in another
> namespace.
>
> I would like to understand what other uses/reasons were thought of.
(I should mention that I am asking from the client perspective. I
know that on the server this has potential to save a lot of disk
space).
> I looked for discussion on the ml archives. I found the patch series
> but could not easily find much *discussion* of the feature and its
> design. I found one post [1] that indicated that "part of the
> rationale..." (being what I described above), but I would like to
> understand the *rest* of the rationale.
>
> Pointers to gmane are also fine, or brief descriptions of uses [being]
> made of this.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/175832/match=namespace
>
> Thanks
>
> --
> Sitaram
--
Sitaram
^ permalink raw reply
* [PATCH] Unify appending signoff in format-patch, commit and sequencer
From: Nguyễn Thái Ngọc Duy @ 2012-11-15 12:32 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
There are two implementations of append_signoff in log-tree.c and
sequencer.c, which do more or less the same thing. This patch
- teaches sequencer.c's append_signoff() not to append the signoff if
it's already there but not at the bottom
- removes log-tree.c's
- make sure "Signed-off-by: foo" in the middle of a line is not
counted as a sign off
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Interestingly this patch triggers the fault that it fixes.
I was surprised that there was no blank line before my S-o-b
and thought I broke something. It turns out I used unmodified
format-patch and it mistook the S-o-b quote as true S-o-b line.
The modified one puts the blank line back.
builtin/log.c | 13 +--------
log-tree.c | 92 ++++-------------------------------------------------------
revision.h | 2 +-
sequencer.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++----
4 files changed, 88 insertions(+), 105 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index e7b7db1..bb48344 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1058,7 +1058,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
struct commit *origin = NULL, *head = NULL;
const char *in_reply_to = NULL;
struct patch_ids ids;
- char *add_signoff = NULL;
struct strbuf buf = STRBUF_INIT;
int use_patch_format = 0;
int quiet = 0;
@@ -1154,16 +1153,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
PARSE_OPT_KEEP_DASHDASH);
- if (do_signoff) {
- const char *committer;
- const char *endpos;
- committer = git_committer_info(IDENT_STRICT);
- endpos = strchr(committer, '>');
- if (!endpos)
- die(_("bogus committer info %s"), committer);
- add_signoff = xmemdupz(committer, endpos - committer + 1);
- }
-
for (i = 0; i < extra_hdr.nr; i++) {
strbuf_addstr(&buf, extra_hdr.items[i].string);
strbuf_addch(&buf, '\n');
@@ -1354,7 +1343,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
total++;
start_number--;
}
- rev.add_signoff = add_signoff;
+ rev.add_signoff = do_signoff;
while (0 <= --nr) {
int shown;
commit = list[nr];
diff --git a/log-tree.c b/log-tree.c
index c894930..18cf006 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -9,6 +9,7 @@
#include "string-list.h"
#include "color.h"
#include "gpg-interface.h"
+#include "sequencer.h"
struct decoration name_decoration = { "object names" };
@@ -206,89 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
putchar(')');
}
-/*
- * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
- * Signed-off-by: and Acked-by: lines.
- */
-static int detect_any_signoff(char *letter, int size)
-{
- char *cp;
- int seen_colon = 0;
- int seen_at = 0;
- int seen_name = 0;
- int seen_head = 0;
-
- cp = letter + size;
- while (letter <= --cp && *cp == '\n')
- continue;
-
- while (letter <= cp) {
- char ch = *cp--;
- if (ch == '\n')
- break;
-
- if (!seen_at) {
- if (ch == '@')
- seen_at = 1;
- continue;
- }
- if (!seen_colon) {
- if (ch == '@')
- return 0;
- else if (ch == ':')
- seen_colon = 1;
- else
- seen_name = 1;
- continue;
- }
- if (('A' <= ch && ch <= 'Z') ||
- ('a' <= ch && ch <= 'z') ||
- ch == '-') {
- seen_head = 1;
- continue;
- }
- /* no empty last line doesn't match */
- return 0;
- }
- return seen_head && seen_name;
-}
-
-static void append_signoff(struct strbuf *sb, const char *signoff)
-{
- static const char signed_off_by[] = "Signed-off-by: ";
- size_t signoff_len = strlen(signoff);
- int has_signoff = 0;
- char *cp;
-
- cp = sb->buf;
-
- /* First see if we already have the sign-off by the signer */
- while ((cp = strstr(cp, signed_off_by))) {
-
- has_signoff = 1;
-
- cp += strlen(signed_off_by);
- if (cp + signoff_len >= sb->buf + sb->len)
- break;
- if (strncmp(cp, signoff, signoff_len))
- continue;
- if (!isspace(cp[signoff_len]))
- continue;
- /* we already have him */
- return;
- }
-
- if (!has_signoff)
- has_signoff = detect_any_signoff(sb->buf, sb->len);
-
- if (!has_signoff)
- strbuf_addch(sb, '\n');
-
- strbuf_addstr(sb, signed_off_by);
- strbuf_add(sb, signoff, signoff_len);
- strbuf_addch(sb, '\n');
-}
-
static unsigned int digits_in_number(unsigned int number)
{
unsigned int i = 10, result = 1;
@@ -651,8 +569,10 @@ void show_log(struct rev_info *opt)
/*
* And then the pretty-printed message itself
*/
- if (ctx.need_8bit_cte >= 0)
- ctx.need_8bit_cte = has_non_ascii(opt->add_signoff);
+ if (ctx.need_8bit_cte >= 0 && opt->add_signoff)
+ ctx.need_8bit_cte =
+ has_non_ascii(fmt_name(getenv("GIT_COMMITTER_NAME"),
+ getenv("GIT_COMMITTER_EMAIL")));
ctx.date_mode = opt->date_mode;
ctx.date_mode_explicit = opt->date_mode_explicit;
ctx.abbrev = opt->diffopt.abbrev;
@@ -663,7 +583,7 @@ void show_log(struct rev_info *opt)
pretty_print_commit(&ctx, commit, &msgbuf);
if (opt->add_signoff)
- append_signoff(&msgbuf, opt->add_signoff);
+ append_signoff(&msgbuf, 0);
if (opt->show_log_size) {
printf("log size %i\n", (int)msgbuf.len);
graph_show_oneline(opt->graph);
diff --git a/revision.h b/revision.h
index a95bd0b..af35325 100644
--- a/revision.h
+++ b/revision.h
@@ -136,7 +136,7 @@ struct rev_info {
int numbered_files;
char *message_id;
struct string_list *ref_message_ids;
- const char *add_signoff;
+ int add_signoff;
const char *extra_headers;
const char *log_reencode;
const char *subject_prefix;
diff --git a/sequencer.c b/sequencer.c
index be0cb8b..4eb59e4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1058,21 +1058,95 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
return 1;
}
+/*
+ * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
+ * Signed-off-by: and Acked-by: lines.
+ */
+static int detect_any_signoff(char *letter, int size)
+{
+ char *cp;
+ int seen_colon = 0;
+ int seen_at = 0;
+ int seen_name = 0;
+ int seen_head = 0;
+
+ cp = letter + size;
+ while (letter <= --cp && *cp == '\n')
+ continue;
+
+ while (letter <= cp) {
+ char ch = *cp--;
+ if (ch == '\n')
+ break;
+
+ if (!seen_at) {
+ if (ch == '@')
+ seen_at = 1;
+ continue;
+ }
+ if (!seen_colon) {
+ if (ch == '@')
+ return 0;
+ else if (ch == ':')
+ seen_colon = 1;
+ else
+ seen_name = 1;
+ continue;
+ }
+ if (('A' <= ch && ch <= 'Z') ||
+ ('a' <= ch && ch <= 'z') ||
+ ch == '-') {
+ seen_head = 1;
+ continue;
+ }
+ /* no empty last line doesn't match */
+ return 0;
+ }
+ return seen_head && seen_name;
+}
+
void append_signoff(struct strbuf *msgbuf, int ignore_footer)
{
struct strbuf sob = STRBUF_INIT;
- int i;
+ const char *cp;
+ int i, has_signoff = 0;
+ int signoff_header_len = strlen(sign_off_header);
strbuf_addstr(&sob, sign_off_header);
strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
getenv("GIT_COMMITTER_EMAIL")));
- strbuf_addch(&sob, '\n');
for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
; /* do nothing */
- if (prefixcmp(msgbuf->buf + i, sob.buf)) {
- if (!i || !ends_rfc2822_footer(msgbuf, ignore_footer))
- strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
- strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
+
+ /* First see if we already have the sign-off by the signer */
+ cp = msgbuf->buf;
+ while ((cp = strstr(cp, sign_off_header)) &&
+ cp + signoff_header_len < msgbuf->buf + msgbuf->len - ignore_footer) {
+
+ if (cp > msgbuf->buf && cp[-1] != '\n' && cp[-1] != '\r') {
+ /*
+ * Signed-off-by: found in the middle of the
+ * commit message is not really a sign off
+ */
+ cp += signoff_header_len;
+ continue;
+ }
+ has_signoff = 1;
+
+ if (cp + sob.len >= msgbuf->buf + msgbuf->len)
+ break;
+ if (!strncmp(cp, sob.buf, sob.len) && isspace(cp[sob.len]))
+ return; /* we already have him */
+ cp += signoff_header_len;
}
+
+ if (!has_signoff)
+ has_signoff = detect_any_signoff(msgbuf->buf,
+ msgbuf->len - ignore_footer);
+
+ if (!i || (!has_signoff && !ends_rfc2822_footer(msgbuf, ignore_footer)))
+ strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
+ strbuf_addch(&sob, '\n');
+ strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
strbuf_release(&sob);
}
--
1.8.0.151.g12dbe03
^ permalink raw reply related
* [PATCH] wildmatch: correct isprint and isspace
From: Nguyễn Thái Ngọc Duy @ 2012-11-15 12:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, schnhrr, rene.scharfe, Johannes Sixt, torvalds,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1352803572-14547-1-git-send-email-pclouds@gmail.com>
Current isprint() incorrectly includes control characters 9, 10 and
13, which is fixed by this patch.
Current isspace() lacks 11 and 12. But Git's isspace() has been
designed this way since the beginning and has over 100 call sites
relying on this. Instead of updating isspace() behavior (which could be
tricky as patches from other topics may come in parallel that assume
the old isspace()), a new isspace_posix() is introduced and used by
wildmatch.c. Other part of Git can be converted to use this new
function if it seems appropriate.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Sorry for the late response. I'll reply to everybody in one mail.
On Wed, Nov 14, 2012 at 1:58 AM, "Jan H. Schönherr" <schnhrr@cs.tu-berlin.de> wrote:
> An alternative to switching from 1-byte to 4-byte values (don't we have
> a 2-byte datatype?), would be to free up GIT_CNTRL and simply do:
>
> #define iscntrl(x) ((x) < 0x20)
No. 127 is also a control character.
On Wed, Nov 14, 2012 at 2:41 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> So we have two properties that overlap:
>
> SSSSSSSSSS
> CCCCCCCC
>
> You seem to generate partions:
>
> XXXYYYYYZZZZZ
>
> then assign individual bits to each partition. Now each entry in the
> lookup table has only one bit set. Then you define isxxx() to check for
> one of the two possible bits:
>
> iscntrl is X or Y
> isspace is Y or Z
>
> But shouldn't you just assign one bit for S and another one for C, have
> entries in the lookup table with more than one bit set, and check for
> only one bit in the isxxx macro?
>
> That way you don't run out of bits as easily as you do with this patch.
I need three sets of characters actually: control, spaces and
printable (which contains non-control spaces). Making it
(isspace(x) && (x) >= 32) is simpler and because isprint() is only used in
wildmatch, I don't need to think about performance penalty (yet).
On Thu, Nov 15, 2012 at 2:30 AM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> Nevertheless, it's unfortunate that we have an isspace() that *almost* does
> what the widely known thing of the same name does. I'd shy away from
> changing git's version directly, because it's used more than a hundred times
> in the code, and estimating the impact of adding \v and \f to it.
> Perhaps renaming it to isgitspace() is a good first step, followed by
> adding a "standard" version of isspace() for wildmatch?
There are just too many call sites of isspace() and there is a risk
of new call sites coming in independently. So I think keeping isspace()
as-is and using a different name for the standard version is probably
a better choice.
As the new isspace_posix() is only used by wildmatch, its performance
as of now is not critical and a simple macro like in this patch is
probably enough. We can optimize it later if we need to.
git-compat-util.h | 4 +++-
wildmatch.c | 8 ++------
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 02f48f6..d4c3fda 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -486,6 +486,7 @@ extern const unsigned char sane_ctype[256];
#define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
#define isascii(x) (((x) & ~0x7f) == 0)
#define isspace(x) sane_istest(x,GIT_SPACE)
+#define isspace_posix(x) (((x) >= 9 && (x) <= 13) || (x) == 32)
#define isdigit(x) sane_istest(x,GIT_DIGIT)
#define isalpha(x) sane_istest(x,GIT_ALPHA)
#define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
@@ -499,7 +500,8 @@ extern const unsigned char sane_ctype[256];
#define isxdigit(x) (hexval_table[x] != -1)
#define isprint(x) (sane_istest(x, GIT_ALPHA | GIT_DIGIT | GIT_SPACE | \
GIT_PUNCT | GIT_REGEX_SPECIAL | GIT_GLOB_SPECIAL | \
- GIT_PATHSPEC_MAGIC))
+ GIT_PATHSPEC_MAGIC) && \
+ (x) >= 32)
#define tolower(x) sane_case((unsigned char)(x), 0x20)
#define toupper(x) sane_case((unsigned char)(x), 0)
#define is_pathspec_magic(x) sane_istest(x,GIT_PATHSPEC_MAGIC)
diff --git a/wildmatch.c b/wildmatch.c
index 3972e26..fd74efd 100644
--- a/wildmatch.c
+++ b/wildmatch.c
@@ -37,11 +37,7 @@ typedef unsigned char uchar;
# define ISBLANK(c) ((c) == ' ' || (c) == '\t')
#endif
-#ifdef isgraph
-# define ISGRAPH(c) (ISASCII(c) && isgraph(c))
-#else
-# define ISGRAPH(c) (ISASCII(c) && isprint(c) && !isspace(c))
-#endif
+#define ISGRAPH(c) (ISASCII(c) && isprint(c) && !isspace_posix(c))
#define ISPRINT(c) (ISASCII(c) && isprint(c))
#define ISDIGIT(c) (ISASCII(c) && isdigit(c))
@@ -50,7 +46,7 @@ typedef unsigned char uchar;
#define ISCNTRL(c) (ISASCII(c) && iscntrl(c))
#define ISLOWER(c) (ISASCII(c) && islower(c))
#define ISPUNCT(c) (ISASCII(c) && ispunct(c))
-#define ISSPACE(c) (ISASCII(c) && isspace(c))
+#define ISSPACE(c) (ISASCII(c) && isspace_posix(c))
#define ISUPPER(c) (ISASCII(c) && isupper(c))
#define ISXDIGIT(c) (ISASCII(c) && isxdigit(c))
--
1.8.0.rc2.23.g1fb49df
^ permalink raw reply related
* [PATCH] tcsh-completion re-using git-completion.bash
From: Marc Khouzam @ 2012-11-15 11:51 UTC (permalink / raw)
To: szeder, git, felipe.contreras; +Cc: Marc Khouzam
In-Reply-To: <CAFj1UpHgPvdDeKZ-Ap7-aVx6p_pxT4a2F01ajmNa00txPyS=Qw@mail.gmail.com>
The current tcsh-completion support for Git, as can be found on the
Internet, takes the approach of defining the possible completions
explicitly. This has the obvious draw-back to require constant
updating as the Git code base evolves.
The approach taken by this commit is to to re-use the advanced bash
completion script and use its result for tcsh completion. This is
achieved by executing (versus sourcing) the bash script and
outputting the completion result for tcsh consumption.
Three solutions were looked at to implement this approach with (A)
being retained:
A) Modifications:
git-completion.bash and new git-completion.tcsh
Modify the existing git-completion.bash script to support
being sourced using bash (as now), but also executed using bash.
When being executed, the script will output the result of the
computed completion to be re-used elsewhere (e.g., in tcsh).
The modification to git-completion.bash is made not to be
tcsh-specific, but to allow future users to also re-use its
output. Therefore, to be general, git-completion.bash accepts a
second optional parameter, which is not used by tcsh, but could
prove useful for other users.
Pros:
1- allows the git-completion.bash script to easily be re-used
2- tcsh support is mostly isolated in git-completion.tcsh
Cons (for tcsh users only):
1- requires the user to copy both git-completion.tcsh and
git-completion.bash to ${HOME}
2- requires bash script to have a fixed name and location:
${HOME}/.git-completion.bash
B) Modifications:
git-completion.bash
Modify the existing git-completion.bash script to support
being sourced using bash (as now), but also executed using bash,
and sourced using tcsh.
Pros:
1- only requires the user to deal with a single file
2- maintenance more obvious for tcsh since it is entirely part
of the same git-completion.bash script.
Cons:
1- tcsh support could affect bash support as they share the
same script
2- small tcsh section must use syntax suitable for both tcsh
and bash and must be at the beginning of the script
3- requires script to have a fixed name and location:
${HOME}/.git-completion.sh (for tcsh users only)
C) Modifications:
New git-completion.tcsh
Provide a short tcsh script that converts git-completion.bash
into an executable script suitable to be used by tcsh.
Pros:
1- tcsh support is entirely isolated in git-completion.tcsh
2- new tcsh script can be as complex as needed
Cons (for tcsh users only):
1- requires the user to copy both git-completion.tcsh and
git-completion.bash to ${HOME}
2- requires bash script to have a fixed name and location:
${HOME}/.git-completion.bash
3- sourcing the new script will generate a third script
Approach (A) was selected to keep the tcsh completion support well
isolated without introducing excessive complexity.
Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
---
Here is the updated version of the patch.
I got git send-email to work, so I hope the formatting will be correct.
Thanks in advance.
Marc
contrib/completion/git-completion.bash | 47 ++++++++++++++++++++++++++++++++
contrib/completion/git-completion.tcsh | 29 +++++++++++++++++++
2 files changed, 76 insertions(+), 0 deletions(-)
create mode 100644 contrib/completion/git-completion.tcsh
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index be800e0..d71a016 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2481,3 +2481,50 @@ __git_complete gitk __gitk_main
if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
__git_complete git.exe __git_main
fi
+
+# Method that will output the result of the completion done by
+# the bash completion script, so that it can be re-used in another
+# context than the bash complete command.
+# It accepts 1 to 2 arguments:
+# 1: The command-line to complete
+# 2: The index of the word within argument #1 in which the cursor is
+# located (optional). If parameter 2 is not provided, it will be
+# determined as best possible using parameter 1.
+__git_complete_with_output ()
+{
+ # Set COMP_WORDS in a way that can be handled by the bash script.
+ COMP_WORDS=($1)
+
+ # Set COMP_CWORD to the cursor location as bash would.
+ if [ -n "${2-}" ]; then
+ COMP_CWORD=$2
+ else
+ # Assume the cursor is at the end of parameter #1.
+ # We must check for a space as the last character which will
+ # tell us that the previous word is complete and the cursor
+ # is on the next word.
+ if [ "${1: -1}" == " " ]; then
+ # The last character is a space, so our location is at the end
+ # of the command-line array
+ COMP_CWORD=${#COMP_WORDS[@]}
+ else
+ # The last character is not a space, so our location is on the
+ # last word of the command-line array, so we must decrement the
+ # count by 1
+ COMP_CWORD=$((${#COMP_WORDS[@]}-1))
+ fi
+ fi
+
+ # Call _git() or _gitk() of the bash script, based on the first
+ # element of the command-line
+ _${COMP_WORDS[0]}
+
+ local IFS=$'\n'
+ echo "${COMPREPLY[*]}"
+}
+
+if [ -n "${1-}" ] ; then
+ # If there is an argument, we know the script is being executed
+ # so go ahead and run the _git_complete_with_output function
+ __git_complete_with_output "${1-}" "${2-}"
+fi
diff --git a/contrib/completion/git-completion.tcsh b/contrib/completion/git-completion.tcsh
new file mode 100644
index 0000000..6096ea8
--- /dev/null
+++ b/contrib/completion/git-completion.tcsh
@@ -0,0 +1,29 @@
+#!tcsh
+#
+# tcsh completion support for core Git.
+#
+# Copyright (C) 2012 Marc Khouzam <marc.khouzam@gmail.com>
+# Distributed under the GNU General Public License, version 2.0.
+#
+# This script makes use of the git-completion.bash script to
+# determine the proper completion for git commands under tcsh.
+#
+# To use this completion script:
+#
+# 1) Copy both this file and the bash completion script to your ${HOME} directory
+# using the names ${HOME}/.git-completion.tcsh and ${HOME}/.git-completion.bash.
+# 2) Add the following line to your .tcshrc/.cshrc:
+# source ${HOME}/.git-completion.tcsh
+
+# One can change the below line to use a different location
+set __git_tcsh_completion_script = ${HOME}/.git-completion.bash
+
+# Check that the user put the script in the right place
+if ( ! -e ${__git_tcsh_completion_script} ) then
+ echo "ERROR in git-completion.tcsh script. Cannot find: ${__git_tcsh_completion_script}. Git completion will not work."
+ exit
+endif
+
+complete git 'p/*/`bash ${__git_tcsh_completion_script} "${COMMAND_LINE}" | sort | uniq`/'
+complete gitk 'p/*/`bash ${__git_tcsh_completion_script} "${COMMAND_LINE}" | sort | uniq`/'
+
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
From: Jeff King @ 2012-11-15 11:50 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <20121115111334.GA1879@sigill.intra.peff.net>
On Thu, Nov 15, 2012 at 03:13:47AM -0800, Jeff King wrote:
> I think a much more compelling argument/commit message for your
> suggested patch would be:
>
> We currently prompt the user for the "From" address. This is an
> inconvenience in the common case that the user has configured their
> identity in the environment, but is meant as a safety check for when
> git falls back to an implicitly generated identity (which may or may
> not be valid).
>
> That safety check is not really necessary, though, as by default
> send-email will prompt the user for a final confirmation before
> sending out any message. The likelihood that a user has both bothered
> to turn off this default _and_ not configured any identity (nor
> checked that the automatic identity is valid) is rather low.
If that is the route we want to go, then we should obviously drop my
series in favor of your final patch. I think it would also need a test
update, no?
I think a more concise commit message would help, too. I disagree with a
great deal of the reasoning in your existing message, but those parts
turn out not to be relevant. The crux of the issue is that the safety
check is not necessary because there is already one (i.e., point 8 of
your list). Feel free to use any or all of my text above.
>From my series, there were a few cleanups that might be worth salvaging.
Here is a rundown by patch:
[1/8]: test-lib: allow negation of prerequisites
This stands on its own, and is something I have wanted a few times in
the past. However, since there is no immediate user, I don't know if it
is worth doing or not.
[2/8]: t7502: factor out autoident prerequisite
A minor cleanup and possible help to future tests, but since there are
no other callers now, not sure if it is worth it.
[3/8]: ident: make user_ident_explicitly_given static
A cleanup that is worth doing, I think.
[4/8]: ident: keep separate "explicit" flags for author and committer
Another cleanup. This is "more correct", in that it handles the corner
cases I mentioned in the commit message. But no current code cares about
those corner cases, because the only real caller is git-commit, and this
is a purely internal interface. I could take or leave it.
[5/8]: var: accept multiple variables on the command line
The tests for this can be split out; we currently don't have "git var"
tests at all, so increasing our test coverage is reasonable. The
multiple variables thing is potentially useful, but there are simply not
that many callers of "git var", and nobody has been asking for such a
feature (we could use it to save a process in git-send-email, but it is
probably not worth the complexity).
[6/8]: var: provide explicit/implicit ident information
[7/8]: Git.pm: teach "ident" to query explicitness
These two should probably be dropped. They would lock us into supporting
the explicit/implicit variables in "git var", for no immediate benefit.
[8/8]: send-email: do not prompt for explicit repo ident
Obviously drop.
> I could accept that line of reasoning. I see that this argument is
> buried deep in your commit message, but I will admit to not reading your
> 9-point list of conditions all that closely, as the first 7 points are,
> in my opinion, not relevant (and I had already read and disagreed with
> them in other messages).
If it sounds like I am blaming you here, I am to some degree. But I am
also blaming myself. I should have read your commit message more
carefully, and I'm sorry for not doing so. I hope we can both try harder
to avoid getting side-tracked on arguing about issues that turned out
not to be important at all (of course, we cannot know which part of the
discussion will turn out to be important, but I think there some
obviously unproductive parts of this discussion).
-Peff
^ permalink raw reply
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
From: Jeff King @ 2012-11-15 11:13 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <20121115104345.GA32465@sigill.intra.peff.net>
On Thu, Nov 15, 2012 at 02:43:58AM -0800, Jeff King wrote:
> > And doesn't have any of the following:
> >
> > * configured user.name/user.email
> > * specified $EMAIL
> > * configured sendemail.from
> > * specified --from argument
> >
> > Very unlikely.
>
> That is certainly the opinion you have stated already. I'm not sure I
> agree. Linus, for example, was an advocate of such a configuration early
> on in git's history. I don't think he still runs that way, though.
>
> > And then, what would be the consequences of not receiving this prompt?
>
> An email would be sent with the generated identity.
I suspect you did not need me to answer that question, but were setting
it up as a rhetorical trap to mention the final confirmation, which I
failed to note in my response.
I think a much more compelling argument/commit message for your
suggested patch would be:
We currently prompt the user for the "From" address. This is an
inconvenience in the common case that the user has configured their
identity in the environment, but is meant as a safety check for when
git falls back to an implicitly generated identity (which may or may
not be valid).
That safety check is not really necessary, though, as by default
send-email will prompt the user for a final confirmation before
sending out any message. The likelihood that a user has both bothered
to turn off this default _and_ not configured any identity (nor
checked that the automatic identity is valid) is rather low.
I could accept that line of reasoning. I see that this argument is
buried deep in your commit message, but I will admit to not reading your
9-point list of conditions all that closely, as the first 7 points are,
in my opinion, not relevant (and I had already read and disagreed with
them in other messages).
-Peff
^ permalink raw reply
* [PATCH] status: add advice on how to push/pull to tracking branch
From: Matthieu Moy @ 2012-11-15 10:45 UTC (permalink / raw)
To: git, gitster; +Cc: Matthieu Moy
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
I thought this was obvious enough not to deserve an advice, but a
colleague of mine had troubles with "commited but not pushed" changes.
Maybe an additional advice would have helped. After all, it's an
advice, and can be deactivated ...
remote.c | 13 ++++++++++---
t/t2020-checkout-detach.sh | 1 +
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/remote.c b/remote.c
index 04fd9ea..9c19689 100644
--- a/remote.c
+++ b/remote.c
@@ -1627,13 +1627,15 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
base = branch->merge[0]->dst;
base = shorten_unambiguous_ref(base, 0);
- if (!num_theirs)
+ if (!num_theirs) {
strbuf_addf(sb,
Q_("Your branch is ahead of '%s' by %d commit.\n",
"Your branch is ahead of '%s' by %d commits.\n",
num_ours),
base, num_ours);
- else if (!num_ours)
+ strbuf_addf(sb,
+ _(" (use \"git push\" to publish your local commits)\n"));
+ } else if (!num_ours) {
strbuf_addf(sb,
Q_("Your branch is behind '%s' by %d commit, "
"and can be fast-forwarded.\n",
@@ -1641,7 +1643,9 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
"and can be fast-forwarded.\n",
num_theirs),
base, num_theirs);
- else
+ strbuf_addf(sb,
+ _(" (use \"git pull\" to update your local branch)\n"));
+ } else {
strbuf_addf(sb,
Q_("Your branch and '%s' have diverged,\n"
"and have %d and %d different commit each, "
@@ -1651,6 +1655,9 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
"respectively.\n",
num_theirs),
base, num_ours, num_theirs);
+ strbuf_addf(sb,
+ _(" (use \"git pull\" to merge the remote branch into yours)\n"));
+ }
return 1;
}
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 8100537..5d68729 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -151,6 +151,7 @@ test_expect_success 'checkout does not warn leaving reachable commit' '
cat >expect <<'EOF'
Your branch is behind 'master' by 1 commit, and can be fast-forwarded.
+ (use "git pull" to update your local branch)
EOF
test_expect_success 'tracking count is accurate after orphan check' '
reset &&
--
1.8.0.319.g8abfee4
^ permalink raw reply related
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
From: Jeff King @ 2012-11-15 10:43 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <CAMP44s2NBGDRLUKhBTU+kNy7Fyn8T6qm3nneSbS4rrNN1oPgdw@mail.gmail.com>
On Thu, Nov 15, 2012 at 11:28:46AM +0100, Felipe Contreras wrote:
> I tried both:
>
> ok 19 # skip implicit ident prompts for sender (missing AUTOIDENT of
> PERL,AUTOIDENT)
> ok 20 - broken implicit ident aborts send-email
>
> ok 19 - implicit ident prompts for sender
> ok 20 # skip broken implicit ident aborts send-email (missing
> !AUTOIDENT of PERL,!AUTOIDENT)
>
> However, it would be much easier if ident learned to check
> GIT_TEST_FAKE_HOSTNAME, or something.
Yes, it would be. It has two downsides:
1. The regular git code has to be instrumented to respect the
variable, so it can potentially affect git in production use
outside of the test suite. Since such code is simple, though, it is
probably not a big risk.
2. We would not actually exercise the code paths for doing
hostname and GECOS lookup. We do not test their resulting values,
so the coverage is not great now, but we do at least run the code,
which would let a run with "--valgrind" check it. I guess we could
go through the motions of assembling the ident and then replace
it at the end with the fake value.
I don't have a strong opinion either way.
> > One whose system is configured in such a way that git can produce an
> > automatic ident (i.e., has a non-blank GECOS name and a FQDN).
>
> And doesn't have any of the following:
>
> * configured user.name/user.email
> * specified $EMAIL
> * configured sendemail.from
> * specified --from argument
>
> Very unlikely.
That is certainly the opinion you have stated already. I'm not sure I
agree. Linus, for example, was an advocate of such a configuration early
on in git's history. I don't think he still runs that way, though.
> And then, what would be the consequences of not receiving this prompt?
An email would be sent with the generated identity.
-Peff
^ 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