* 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
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
From: Felipe Contreras @ 2012-11-15 10:28 UTC (permalink / raw)
To: Jeff King; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <20121115083315.GA23377@sigill.intra.peff.net>
On Thu, Nov 15, 2012 at 9:33 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Nov 15, 2012 at 03:08:42AM +0100, Felipe Contreras wrote:
>
>> I don't think there's any need for all that, this does the trick:
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index aea66a0..503e551 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -748,16 +748,11 @@ if (!$force) {
>> }
>> }
>>
>> -my $prompting = 0;
>> if (!defined $sender) {
>> $sender = $repoauthor || $repocommitter || '';
>> - $sender = ask("Who should the emails appear to be from? [$sender] ",
>> - default => $sender,
>> - valid_re => qr/\@.*\./, confirm_only => 1);
>> - print "Emails will be sent from: ", $sender, "\n";
>> - $prompting++;
>> }
>>
>> +my $prompting = 0;
>>
>> This passes all the current tests and the ones you added.
>
> It may pass on your system, but it will not on a system that meets the
> AUTOIDENT prerequisite (it fails the new t9001.19 on my system; I
> suspect your system config is such that we skip t9001.19 and run
> t9001.20, whereas mine is the opposite).
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.
But then I realized I had to run 'make' again. Yes, my patch breaks
test 19, but see below.
>> Which kind of user will get the prompt with your patch, that would
>> miss it with mine?
>
> 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. And then, what would be the consequences of not
receiving this prompt?
Cheers.
--
Felipe Contreras
^ permalink raw reply
* use cases for git namespaces
From: Sitaram Chamarty @ 2012-11-15 8:33 UTC (permalink / raw)
To: Git Mailing List
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 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
^ permalink raw reply
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
From: Jeff King @ 2012-11-15 8:33 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <CAMP44s0d+g7bXCnOf55jZNNFS6uJ+4BDowx5uYxWBP4xA+-0zA@mail.gmail.com>
On Thu, Nov 15, 2012 at 03:08:42AM +0100, Felipe Contreras wrote:
> I don't think there's any need for all that, this does the trick:
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index aea66a0..503e551 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -748,16 +748,11 @@ if (!$force) {
> }
> }
>
> -my $prompting = 0;
> if (!defined $sender) {
> $sender = $repoauthor || $repocommitter || '';
> - $sender = ask("Who should the emails appear to be from? [$sender] ",
> - default => $sender,
> - valid_re => qr/\@.*\./, confirm_only => 1);
> - print "Emails will be sent from: ", $sender, "\n";
> - $prompting++;
> }
>
> +my $prompting = 0;
>
> This passes all the current tests and the ones you added.
It may pass on your system, but it will not on a system that meets the
AUTOIDENT prerequisite (it fails the new t9001.19 on my system; I
suspect your system config is such that we skip t9001.19 and run
t9001.20, whereas mine is the opposite).
> Which kind of user will get the prompt with your patch, that would
> miss it with mine?
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).
-Peff
^ permalink raw reply
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
From: Jonathan Nieder @ 2012-11-15 8:23 UTC (permalink / raw)
To: Jeff King; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
In-Reply-To: <20121115003640.GH17819@sigill.intra.peff.net>
Jeff King wrote:
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -191,15 +191,47 @@ test_expect_success $PREREQ 'Show all headers' '
>
> test_expect_success $PREREQ 'Prompting works' '
> clean_fake_sendmail &&
> - (echo "Example <from@example.com>"
> - echo "to@example.com"
> + (echo "to@example.com"
> echo ""
> ) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
> --smtp-server="$(pwd)/fake.sendmail" \
> $patches \
> 2>errors &&
> + grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
> + grep "^To: to@example.com\$" msgtxt1
> +'
The indentation seems strange here --- are the new "grep" lines
continuations of the git send-email line?
It's probably easier to change the structure completely:
clean_fake_sendmail &&
echo to@examples.com >prompt.input &&
echo >>prompt.input &&
GIT_SEND_EMAIL_NOTTY=1 \
git send-email --smtp-server=... $patches <prompt.input &&
grep "^From: A U Thor <authorident@example.com>\$" msgtxt1 &&
grep "^To: to@example.com\$" msgtxt1
> +test_expect_success $PREREQ,AUTOIDENT 'implicit ident prompts for sender' '
> + clean_fake_sendmail &&
> + (echo "Example <from@example.com>" &&
> + echo "to@example.com" &&
> + echo ""
> + ) |
> + (sane_unset GIT_AUTHOR_NAME &&
> + sane_unset GIT_AUTHOR_EMAIL &&
> + sane_unset GIT_COMMITTER_NAME &&
> + sane_unset GIT_COMMITTER_EMAIL &&
> + GIT_SEND_EMAIL_NOTTY=1 git send-email \
> + --smtp-server="$(pwd)/fake.sendmail" \
> + $patches \
> + 2>errors &&
> grep "^From: Example <from@example.com>\$" msgtxt1 &&
> grep "^To: to@example.com\$" msgtxt1
> + )
> +'
Likewise:
clean_fake_sendmail &&
echo "Example <from@example.com>" >prompt.in &&
echo to@example.com >>prompt.in
echo >>prompt.in &&
(
sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
GIT_SEND_EMAIL_NOTTY=1 \
git send-email --smtp-server=... $patches <prompt.in
) &&
grep "^From: Example <from@example.com>\$" msgtxt1 &&
grep "^To: to@example.com\$" msgtxt1
> +test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '
> + clean_fake_sendmail &&
> + (sane_unset GIT_AUTHOR_NAME &&
> + sane_unset GIT_AUTHOR_EMAIL &&
> + sane_unset GIT_COMMITTER_NAME &&
> + sane_unset GIT_COMMITTER_EMAIL &&
> + GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&
> + test_must_fail git send-email \
> + --smtp-server="$(pwd)/fake.sendmail" \
> + $patches </dev/null 2>errors.out &&
> + test_i18ngrep "tell me who you are" errors.out
> + )
> '
Likewise:
clean_fake_sendmail &&
(
sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
GIT_SEND_EMAIL_NOTTY=1 \
git send-email --smtp-server=... $patches </dev/null 2>err
) &&
test_i18ngrep "[Tt]ell me who you are" err
For what it's worth, with or without such changes,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks
From: Michael Haggerty @ 2012-11-15 8:18 UTC (permalink / raw)
To: David Aguilar
Cc: Junio C Hamano, Jiang Xin, Lea Wiemann, David Reiss,
Johannes Sixt, git, Lars R. Damerow, Jeff King, Marc Jordan
In-Reply-To: <CAJDDKr5F5EcXaTuPWgE5MZJQ=Of6MwW+RmRhhXOLyfQzanjEwQ@mail.gmail.com>
On 11/13/2012 09:50 PM, David Aguilar wrote:
> On Mon, Nov 12, 2012 at 9:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> The log message of the original commit (0454dd93bf) described the
>>> following scenario: a /home partition under which user home directories
>>> are automounted, and setting GIT_CEILING_DIRECTORIES=/home to avoid
>>> hitting /home/.git, /home/.git/objects, and /home/objects (which would
>>> attempt to automount those directories). I believe that this scenario
>>> would not be slowed down by my patches.
>>>
>>> How do you use GIT_CEILING_DIRECTORIES that the proposed changes cause a
>>> slowdown?
>>
>> Yeah, I was also wondering about that.
>>
>> David?
>
> I double-checked our configuration and all the parent directories
> of those listed in GIT_CEILING_DIRECTORIES are local,
> so our particular configuration would not have a performance hit.
>
> We do have multiple directories listed there. Some of them share
> a parent directory. I'm assuming the implementation is simple and
> does not try and avoid repeating the check when the parent dir is
> the same across multiple entries.
>
> In any case, it won't be a problem in practice based on my
> reading of the current code.
OK, so we're back to the following status: some people (including me)
are nervous that this change could cause a performance regression,
though it seems that the most sensible ways of using the
GIT_CEILING_DIRECTORIES feature would not be affected.
In favor: Currently, if a directory containing a symlink is added to
GIT_CEILING_DIRECTORIES, then GIT_CEILING_DIRECTORIES will not work, git
has no way of recognizing that there is a problem, and the only symptom
observable by the user is that the hoped-for performance improvement
from using GIT_CEILING_DIRECTORIES will not materialize (or will
disappear after a filesystem reorg) [1].
Against: The change will cause a performance regression if a
slow-to-stat directory is listed in GIT_CEILING_DIRECTORIES. The
slowdown will occur whenever git is run outside of a true git-managed
project, most nastily in the case of using __git_ps1 in a shell prompt.
I don't have a preference either way about whether these patches should
be merged.
Michael
[1] It is also conceivable that GIT_CEILING_DIRECTORIES is being used to
*hide* an enclosing git project rather than to inform git that there are
no enclosing projects, in which case the enclosing project would *not*
be hidden. This is in fact the mechanism by which the problem causes
failures in our test suite. But I don't expect that this is a common
real-world scenario, and anyway such a failure would be obvious to the
user and quickly fixed.
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCHv2 7/8] Git.pm: teach "ident" to query explicitness
From: Jonathan Nieder @ 2012-11-15 8:13 UTC (permalink / raw)
To: Jeff King; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
In-Reply-To: <20121115003628.GG17819@sigill.intra.peff.net>
Jeff King wrote:
> Signed-off-by: Jeff King <peff@peff.net>
For what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCHv2 5/8] var: accept multiple variables on the command line
From: Jonathan Nieder @ 2012-11-15 8:10 UTC (permalink / raw)
To: Jeff King; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
In-Reply-To: <20121115003504.GE17819@sigill.intra.peff.net>
Jeff King wrote:
> This patch lets callers specify multiple variables, and
> prints one per line.
[...]
> Signed-off-by: Jeff King <peff@peff.net>
Very pleasantly done --- thanks. For what it's worth, assuming this
is tested, I can't see any reason not to apply it.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCHv2 4/8] ident: keep separate "explicit" flags for author and committer
From: Jonathan Nieder @ 2012-11-15 8:04 UTC (permalink / raw)
To: Jeff King; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
In-Reply-To: <20121115003413.GD17819@sigill.intra.peff.net>
Jeff King wrote:
> 1. GIT_COMMITTER_* is set explicitly, but we fallback for
> GIT_AUTHOR. We claim the ident is explicit, even though
> the author is not.
>
> 2. GIT_AUTHOR_* is set and we ask for author ident, but
> not committer ident. We will claim the ident is
> implicit, even though it is explicit.
>
> This patch uses two variables instead of one, updates both
> when we set the "fallback" values, and updates them
> individually when we read from the environment.
Nice problem description. The fixed behavior makes sense to me, for
what it's worth.
Not about this patch, but: in case (1), shouldn't the author fall
back to $GIT_COMMITER_NAME?
^ permalink raw reply
* Re: [PATCHv2 3/8] ident: make user_ident_explicitly_given static
From: Jonathan Nieder @ 2012-11-15 7:51 UTC (permalink / raw)
To: Jeff King; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
In-Reply-To: <20121115003405.GC17819@sigill.intra.peff.net>
Jeff King wrote:
> In v1.5.6-rc0~56^2 (2008-05-04) "user_ident_explicitly_given"
> was introduced as a global for communication between config,
> ident, and builtin-commit. In v1.7.0-rc0~72^2 (2010-01-07)
> readers switched to using the common wrapper
> user_ident_sufficiently_given(). After v1.7.11-rc1~15^2~18
> (2012-05-21), the var is only written in ident.c.
>
> Now we can make it static, which will enable further
> refactoring without worrying about upsetting other code.
>
> Signed-off-by: Jeff King <peff@peff.net>
For what it's worth I liked the old commit message more. But I don't
mind either.
Hope that helps,
Jonathan
^ 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