Re: [PATCH 60/68] prefer memcpy to strcpy
On Thu, Sep 24 2015, Jeff King wrote: > This also eliminates calls to strcpy, which make auditing > the code base harder. Maybe may English parser is broken, but this doesn't immediately sound like what you meant to say. Also, in 29/68 you say "We drop calls to strcpy, which makes auditing the code base easier." Maybe it's all ok, since on second reading the first "make" probably refers to the plural "calls for strcpy", while in the second case "makes" refers to "[the dropping of] calls to strcpy". Rasmus -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] shallow.c: bit manipulation tweaks
First of all, 1 << 31 is technically undefined behaviour, so let's just
use an unsigned literal.
If i is 'signed int' and gcc doesn't know that i is positive, gcc
generates code to compute the C99-mandated values of "i / 32" and "i %
32", which is a lot more complicated than simple a simple shifts/mask.
The only caller of paint_down actually passes an "unsigned int" value,
but the prototype of paint_down causes (completely well-defined)
conversion to signed int, and gcc has no way of knowing that the
converted value is non-negative. Just make the id parameter unsigned.
In update_refstatus, the change in generated code is much smaller,
presumably because gcc is smart enough to see that i starts as 0 and is
only incremented, so it is allowed (per the UD of signed overflow) to
assume that i is always non-negative. But let's just help less smart
compilers generate good code anyway.
Signed-off-by: Rasmus Villemoes
---
shallow.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/shallow.c b/shallow.c
index 8b1c35d..5aec5a5 100644
--- a/shallow.c
+++ b/shallow.c
@@ -464,7 +464,7 @@ static uint32_t *paint_alloc(struct paint_info *info)
* all walked commits.
*/
static void paint_down(struct paint_info *info, const unsigned char *sha1,
- int id)
+ unsigned int id)
{
unsigned int i, nr;
struct commit_list *head = NULL;
@@ -476,7 +476,7 @@ static void paint_down(struct paint_info *info, const
unsigned char *sha1,
if (!c)
return;
memset(bitmap, 0, bitmap_size);
- bitmap[id / 32] |= (1 << (id % 32));
+ bitmap[id / 32] |= (1U << (id % 32));
commit_list_insert(c, &head);
while (head) {
struct commit_list *p;
@@ -650,11 +650,11 @@ static int add_ref(const char *refname, const struct
object_id *oid,
static void update_refstatus(int *ref_status, int nr, uint32_t *bitmap)
{
- int i;
+ unsigned int i;
if (!ref_status)
return;
for (i = 0; i < nr; i++)
- if (bitmap[i / 32] & (1 << (i % 32)))
+ if (bitmap[i / 32] & (1U << (i % 32)))
ref_status[i]++;
}
--
2.1.4
[PATCH 4/4] shallow.c: remove useless test
It seems to be odd to do x=y if x==y. Maybe there's a bug somewhere near this, but as is this is somewhat confusing. Signed-off-by: Rasmus Villemoes --- shallow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shallow.c b/shallow.c index 5aec5a5..7c28239 100644 --- a/shallow.c +++ b/shallow.c @@ -513,7 +513,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1, p->item); if (p->item->object.flags & SEEN) continue; - if (*p_refs == NULL || *p_refs == *refs) + if (*p_refs == NULL) *p_refs = *refs; commit_list_insert(p->item, &head); } -- 2.1.4
[PATCH 1/4] shallow.c: make paint_alloc slightly more robust
I have no idea if this is a real issue, but it's not obvious to me that
paint_alloc cannot be called with info->nr_bits greater than about
4M (\approx 8*COMMIT_SLAB_SIZE). In that case the new slab would be too
small. So just round up the allocation to the maximum of
COMMIT_SLAB_SIZE and size.
Signed-off-by: Rasmus Villemoes
---
shallow.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/shallow.c b/shallow.c
index 4d0b005..e21534a 100644
--- a/shallow.c
+++ b/shallow.c
@@ -445,11 +445,13 @@ static uint32_t *paint_alloc(struct paint_info *info)
unsigned size = nr * sizeof(uint32_t);
void *p;
if (!info->slab_count || info->free + size > info->end) {
+ unsigned alloc_size = size < COMMIT_SLAB_SIZE ?
+ COMMIT_SLAB_SIZE : size;
info->slab_count++;
REALLOC_ARRAY(info->slab, info->slab_count);
- info->free = xmalloc(COMMIT_SLAB_SIZE);
+ info->free = xmalloc(alloc_size);
info->slab[info->slab_count - 1] = info->free;
- info->end = info->free + COMMIT_SLAB_SIZE;
+ info->end = info->free + alloc_size;
}
p = info->free;
info->free += size;
--
2.1.4
[PATCH 2/4] shallow.c: avoid theoretical pointer wrap-around
The expression info->free+size is technically undefined behaviour in
exactly the case we want to test for. Moreover, the compiler is likely
to translate the expression to
(unsigned long)info->free + size > (unsigned long)info->end
where there's at least a theoretical chance that the LHS could wrap
around 0, giving a false negative.
This might as well be written using pointer subtraction avoiding these
issues.
Signed-off-by: Rasmus Villemoes
---
shallow.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/shallow.c b/shallow.c
index e21534a..8b1c35d 100644
--- a/shallow.c
+++ b/shallow.c
@@ -444,7 +444,7 @@ static uint32_t *paint_alloc(struct paint_info *info)
unsigned nr = (info->nr_bits + 31) / 32;
unsigned size = nr * sizeof(uint32_t);
void *p;
- if (!info->slab_count || info->free + size > info->end) {
+ if (!info->slab_count || size > info->end - info->free) {
unsigned alloc_size = size < COMMIT_SLAB_SIZE ?
COMMIT_SLAB_SIZE : size;
info->slab_count++;
--
2.1.4
Re: [PATCH] git-send-email: Add auto-cc to all body signatures
On Thu, Dec 08 2011, Joe Perches wrote: > On Thu, 2011-12-08 at 11:37 -0800, Junio C Hamano wrote: >> Joe Perches writes: >> > Many types of signatures are used by various projects. >> > The most common type is formatted: >> >"[some_signature_type]-by: First Last " >> > e.g: >> >"Reported-by: First Last " (no quotes are used) >> This is just a phrasing issue, but I am a bit reluctant about the name >> "signature". > > I've called all these markings signatures. > Maybe email-address-tags or another name could be used. > I'm not bothered one way or another by any chosen name. It's been four years, but I recently ran into this. I mistakenly thought that git would actually pick up cc addresses also from Reported-by, so the reporter ended up not being cc'ed. Is there any chance this could be revisited, or should I use a --cc-cmd to do what I want? Rasmus -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] send-email: don't cc *-by lines with '-' prefix
On 04/04/2019 11.27, Baruch Siach wrote: > Hi Junio, > > On Thu, Apr 04 2019, Junio C. Hamano wrote: >> Baruch Siach writes: >> Independently, I think it makes sense to do something like /^([a-z][a-z-]*-by|Cc): (.*)/i to tighten the match to exclude a non-trailer; that would have been sufficient for the original case that triggered this thread. >>> >>> Is there anything I need to do more to get this fix applied for the next >>> git release? >> >> Get "this" fix applied? I think we should tighten the regexp to >> exclude a non-trailer, which would have been sufficient for the >> original case without anything else in "this" fix. So in short, I >> do not think "this" fix won't be applied without further tweaking >> ;-) > > This is exactly what "this" patch (referenced in the title of "this" > thread) is doing: > > > https://public-inbox.org/git/eec56beab016182fb78fbd367fcfa97f2ca6a5ff.1552764410.git.bar...@tkos.co.il/ > > Am I missing something? My ack for Baruch's original patch, which AFAICT is identical with Junio's suggestion, still stands. FWIW, I'm against Joe's suggestion of stopping at a line matching /^---/, since it's not unlikely somebody does something like dmesg output bla bla in the commit message. Since all lines (except for some of the diff header lines) in the patch part begin with space, - or +, insisting on a the line starting with a letter should be sufficient for excluding any random Foo-by lines that may appear in the patch part. Rasmus
[PATCH] help: allow redirecting to help for aliased command
I often use 'git --help' as a quick way to get the documentation
for a command. However, I've also trained my muscle memory to use my
aliases (cp=cherry-pick, co=checkout etc.), which means that I often end
up doing
git cp --help
to which git correctly informs me that cp is an alias for
cherry-pick. However, I already knew that, and what I really wanted was
the man page for the cherry-pick command.
This introduces a help.followAlias config option that transparently
redirects to (the first word of) the alias text (provided of course it
is not a shell command), similar to the option for autocorrect of
misspelled commands.
The documentation in config.txt could probably be improved. Also, I
mimicked the autocorrect case in that the "Continuing to ..." text goes
to stderr, but because of that, I also print the "is aliased to" text to
stderr, which is different from the current behaviour of using
stdout. I'm not sure what the most correct thing is, but I assume --help
is mostly used interactively with stdout and stderr pointing at the same
place.
Signed-off-by: Rasmus Villemoes
---
Documentation/config.txt | 10 ++
builtin/help.c | 36 +---
2 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ad0f4510c3..8a1fc8064e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2105,6 +2105,16 @@ help.autoCorrect::
value is 0 - the command will be just shown but not executed.
This is the default.
+help.followAlias::
+ When requesting help for an alias, git prints a line of the
+ form "'' is aliased to ''". If this option is
+ set to a positive integer, git proceeds to show the help for
+ the first word of after the given number of
+ deciseconds. If the value of this option is negative, the
+ redirect happens immediately. If the value is 0 (which is the
+ default), or begins with an exclamation point, no
+ redirect takes place.
+
help.htmlPath::
Specify the path where the HTML documentation resides. File system paths
and URLs are supported. HTML pages will be prefixed with this path when
diff --git a/builtin/help.c b/builtin/help.c
index 8d4f6dd301..ef1c3f0916 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -34,6 +34,7 @@ enum help_format {
};
static const char *html_path;
+static int follow_alias;
static int show_all = 0;
static int show_guides = 0;
@@ -273,6 +274,10 @@ static int git_help_config(const char *var, const char
*value, void *cb)
html_path = xstrdup(value);
return 0;
}
+ if (!strcmp(var, "help.followalias")) {
+ follow_alias = git_config_int(var, value);
+ return 0;
+ }
if (!strcmp(var, "man.viewer")) {
if (!value)
return config_error_nonbool(var);
@@ -415,9 +420,34 @@ static const char *check_git_cmd(const char* cmd)
alias = alias_lookup(cmd);
if (alias) {
- printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
- free(alias);
- exit(0);
+ const char **argv;
+ int count;
+
+ if (!follow_alias || alias[0] == '!') {
+ printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
+ free(alias);
+ exit(0);
+ }
+ fprintf_ln(stderr, _("'%s' is aliased to '%s'"), cmd, alias);
+
+ /*
+* We use split_cmdline() to get the first word of the
+* alias, to ensure that we use the same rules as when
+* the alias is actually used. split_cmdline()
+* modifies alias in-place.
+*/
+ count = split_cmdline(alias, &argv);
+ if (count < 0)
+ die("Bad alias.%s string: %s", cmd,
+ split_cmdline_strerror(count));
+
+ if (follow_alias > 0) {
+ fprintf_ln(stderr,
+ _("Continuing to help for %s in %0.1f
seconds."),
+ alias, follow_alias/10.0);
+ sleep_millisec(follow_alias * 100);
+ }
+ return alias;
}
if (exclude_guides)
--
2.16.4
Re: [PATCH] help: allow redirecting to help for aliased command
On 2018-09-26 17:16, Junio C Hamano wrote:
> Rasmus Villemoes writes:
>
>> +/*
>> + * We use split_cmdline() to get the first word of the
>> + * alias, to ensure that we use the same rules as when
>> + * the alias is actually used. split_cmdline()
>> + * modifies alias in-place.
>> + */
>> +count = split_cmdline(alias, &argv);
>> +if (count < 0)
>> +die("Bad alias.%s string: %s", cmd,
>> +split_cmdline_strerror(count));
>> +
>> +if (follow_alias > 0) {
>> +fprintf_ln(stderr,
>> + _("Continuing to help for %s in %0.1f
>> seconds."),
>> + alias, follow_alias/10.0);
>> +sleep_millisec(follow_alias * 100);
>> +}
>> +return alias;
>
> If you have "[alias] cp = cherry-pick -n", split_cmdline discards
> "-n" and the follow-alias prompt does not even tell you that it did
> so,
That's not really true, as I deliberately did the split_cmdline after
printing the "is an alias for", but before "continuing to help for", so
this would precisely tell you
cp is an alias for 'cherry-pick -n'
continuing to help for 'cherry-pick' in 1.5 seconds
> and you get "git help cherry-pick". This code somehow expects
> you to know to jump to the section that describes the "--no-commit"
> option. I do not think that is a reasonable expectation.
No, in that case I would not expect git cp --help to jump to that
section anymore than I would expect "git cherry-pick -n --help" to
magically do that (and that would be impossible in general, if more
options are bundled in the alias).
> When you have "[alias] cp = cherry-pick -n", "git cp --help" should
> not do "git help cherry-pick". Only a single word that exactly
> matches a git command should get this treatment.
I considered that, and could certainly live with that. But it seems the
discussion took a different turn in another part of the thread, so I'll
continue there.
Rasmus
Re: [PATCH] help: allow redirecting to help for aliased command
On 2018-09-26 17:19, Duy Nguyen wrote:
> On Wed, Sep 26, 2018 at 4:42 PM Taylor Blau wrote:
>>> +
>>> + /*
>>> + * We use split_cmdline() to get the first word of the
>>> + * alias, to ensure that we use the same rules as when
>>> + * the alias is actually used. split_cmdline()
>>> + * modifies alias in-place.
>>> + */
>>> + count = split_cmdline(alias, &argv);
>>> + if (count < 0)
>>> + die("Bad alias.%s string: %s", cmd,
>>> + split_cmdline_strerror(count));
>>
>> Please wrap this in _() so that translators can translate it.
>
> Yes! And another nit. die(), error(), warning()... usually start the
> message with a lowercase letter because we already start the sentence
> with a prefix, like
>
> fatal: bad alias.blah blah
>
I'll keep these points in mind, but this was pure copy-paste from git.c.
Rasmus
Re: [PATCH] help: allow redirecting to help for aliased command
On 2018-09-26 20:12, Taylor Blau wrote: > > In the case where you are scripting (and want to know what 'git co' > means for programmatic usage), I think that there are two options. One, > which you note above, is the 'git -c help.followAlias=false ...' > approach, which I don't think is so bad for callers, given the tradeoff. > > Another way to go is 'git config alias.co', which should provide the > same answer. I think that either would be fine. The latter seems much more robust, since that will also tell you precisely whether co is an alias at all, and you don't have to parse -h/--help output (stripping out the 'is aliased to...' stuff, which might be complicated by i18n etc. etc.). So I don't think we should worry too much about scripted use of -h/--help. Rasmus
Re: [PATCH] help: allow redirecting to help for aliased command
On 2018-09-26 20:49, Jeff King wrote: > On Wed, Sep 26, 2018 at 08:16:36AM -0700, Junio C Hamano wrote: > >> >> If we expect users to use "git cp --help" a lot more often than "git >> help cp" (or the other way around), one way to give a nicer experience >> may be to unconditionally make "git cp --help" to directly show the >> manpage of cherry-pick, while keeping "git help cp" to never do >> that. Then those who want to remember what "co" is aliased to can >> ask "git help co". > > I like that direction much better. I also wondered if we could leverage > the "-h" versus "--help" distinction. The problem with printing the > alias definition along with "--help" is that the latter will start a > pager that obliterates what we wrote before (and hence all of this delay > trickery). > > But for "-h" we generally expect the command to output a usage message. > > So what if the rules were: > > - "git help cp" shows "cp is an alias for cherry-pick" (as it does > now) Sounds good. > - "git cp -h" shows "cp is an alias for cherry-pick", followed by > actually running "cherry-pick -h", which will show the usage > message. For a single-word command that does very little, since the > usage message starts with "cherry-pick". But if your alias is > actually "cp = cherry-pick -n", then it _is_ telling you extra > information. Funny, I never noticed this difference, and that '-h' for an alias would actually give more information than '--help'. I sort-of knew that -h would give the synopsis, so I guess I've just gotten used to always use --help, and just noticed that for aliases that doesn't provide much help. Adding the 'is an alias for' info to -h sounds quite sensible. And this could even work with "!" aliases: we define > it, and then it is up to the alias to handle "-h" sensibly. I'd be nervous about doing this, though, especially if we introduce this without a new opt-in config option (which seems to be the direction the discussion is taking). There are lots of commands that don't respond with a help message to -h, or that only recognize -h as the first word, or... There are really too many ways this could cause headaches. But, now that I test it, it seems that we already let the alias handle -h (and any other following words, with --help as the first word special-cased). So what you're suggesting is (correct me if I'm wrong) to _also_ intercept -h as the first word, and then print the alias info, in addition to spawning the alias with the entire argv as usual. The alias info would probably need to go to stderr in this case. > - "git cp --help" opens the manpage for cherry-pick. We don't bother > with the alias definition, as it's available through other means > (and thus we skip the obliteration/timing thing totally). It sounds like you suggest doing this unconditionally, and without any opt-in via config option or a short wait? That would certainly work for me. It is, in fact, how I expect 'git cp --help' to work, until I get reminded that it does not... Also, as Junio noted, is consistent with --help generally providing more information than -h - except that one loses the 'is an alias for' part for --help. > This really only works for non-! aliases. Those would continue to > show the alias definition. Yes. Thanks, Rasmus
[PATCH v2 3/3] git-help.txt: document "git help cmd" vs "git cmd --help" for aliases
This documents the existing behaviour of "git help cmd" when cmd is an alias, as well as providing a hint to use the "git cmd --help" form to be taken directly to the man page for the aliased command. Signed-off-by: Rasmus Villemoes --- Documentation/git-help.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt index 83d25d825a..37e85868fd 100644 --- a/Documentation/git-help.txt +++ b/Documentation/git-help.txt @@ -29,6 +29,10 @@ guide is brought up. The 'man' program is used by default for this purpose, but this can be overridden by other options or configuration variables. +If an alias is given, git prints a note explaining what it is an alias +for on standard output. To get the manual page for the aliased +command, use `git COMMAND --help`. + Note that `git --help ...` is identical to `git help ...` because the former is internally converted into the latter. -- 2.19.0
[PATCH v2 2/3] git.c: handle_alias: prepend alias info when first argument is -h
Most git commands respond to -h anywhere in the command line, or at
least as a first and lone argument, by printing the usage
information. For aliases, we can provide a little more information that
might be useful in interpreting/understanding the following output by
prepending a line telling that the command is an alias, and for what.
When one invokes a simple alias, such as "cp = cherry-pick"
with -h, this results in
$ git cp -h
'cp' is aliased to 'cherry-pick'
usage: git cherry-pick [] ...
...
When the alias consists of more than one word, this provides the
additional benefit of informing the user which options are implicit in
using the alias, e.g. with "cp = cherry-pick -n":
$ git cp -h
'cp' is aliased to 'cherry-pick -n'
usage: git cherry-pick [] ...
...
For shell commands, we cannot know how it responds to -h, but printing
this line to stderr should not hurt, and can help in figuring out what
is happening in a case like
$ git sc -h
'sc' is aliased to '!somecommand'
somecommand: invalid option '-h'
Suggested-by: Jeff King
Signed-off-by: Rasmus Villemoes
---
git.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/git.c b/git.c
index a6f4b44af5..0211c2d4c0 100644
--- a/git.c
+++ b/git.c
@@ -318,6 +318,9 @@ static int handle_alias(int *argcp, const char ***argv)
alias_command = (*argv)[0];
alias_string = alias_lookup(alias_command);
if (alias_string) {
+ if (*argcp > 1 && !strcmp((*argv)[1], "-h"))
+ fprintf_ln(stderr, _("'%s' is aliased to '%s'"),
+ alias_command, alias_string);
if (alias_string[0] == '!') {
struct child_process child = CHILD_PROCESS_INIT;
int nongit_ok;
--
2.19.0
[PATCH v2 1/3] help: redirect to aliased commands for "git cmd --help"
As discussed in the thread for v1 of this patch [1] [2], this changes the
rules for "git foo --help" when foo is an alias.
(0) When invoked as "git help foo", we continue to print the "foo is
aliased to bar" message and nothing else.
(1) If foo is an alias for a shell command, print "foo is aliased to
!bar" as usual.
(2) Otherwise, break the alias string into words, and pretend that "git
word0 --help" was called.
At least for me, getting the man page for git-cherry-pick directly with
"git cp --help" is more useful (and how I expect an alias to behave)
than the short "is aliased to" notice. It is also consistent with
"--help" generally providing more comprehensive help than "-h".
I believe that printing the "is aliased to" message also in case (2) has
value: Depending on pager setup, or if the user has help.format=web, the
message is still present immediately above the prompt when the user
quits the pager/returns to the terminal. That serves as an explanation
for why one was redirected to "man git-cherry-pick" from "git cp
--help", and if cp is actually 'cherry-pick -n', it reminds the user
that using cp has some flag implicitly set before firing off the next
command.
It also provides some useful info in case we end up erroring out, either
in the "bad alias string" check, or in the "No manual entry for gitbar"
case.
[1] https://public-inbox.org/git/20180926102636.30691-1...@rasmusvillemoes.dk/
[2] https://public-inbox.org/git/20180926184914.gc30...@sigill.intra.peff.net/
Signed-off-by: Rasmus Villemoes
---
builtin/help.c | 26 +++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/builtin/help.c b/builtin/help.c
index 8d4f6dd301..4802a06f37 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -415,9 +415,29 @@ static const char *check_git_cmd(const char* cmd)
alias = alias_lookup(cmd);
if (alias) {
- printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
- free(alias);
- exit(0);
+ const char **argv;
+ int count;
+
+ /*
+* If we were invoked as "git help cmd", or cmd is an
+* alias for a shell command, we inform the user what
+* cmd is an alias for and do nothing else.
+*/
+ if (!exclude_guides || alias[0] == '!') {
+ printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
+ free(alias);
+ exit(0);
+ }
+ /*
+* Otherwise, we pretend that the command was "git
+* word0 --help.
+*/
+ fprintf_ln(stderr, _("'%s' is aliased to '%s'"), cmd, alias);
+ count = split_cmdline(alias, &argv);
+ if (count < 0)
+ die(_("bad alias.%s string: %s"), cmd,
+ split_cmdline_strerror(count));
+ return alias;
}
if (exclude_guides)
--
2.19.0
Re: [PATCH v2 1/3] help: redirect to aliased commands for "git cmd --help"
On 2018-10-03 04:13, Jeff King wrote:
>> +/*
>> + * If we were invoked as "git help cmd", or cmd is an
>> + * alias for a shell command, we inform the user what
>> + * cmd is an alias for and do nothing else.
>> + */
>> +if (!exclude_guides || alias[0] == '!') {
>> +printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
>> +free(alias);
>> +exit(0);
>> +}
>
> I'm not sure I understand why exclude_guides is relevant. We check it
> below when we know that we _don't_ have an alias. Hrm. I guess you're
> using it here as a proxy for "git foo --help" being used instead of "git
> help foo".
Exactly. Perhaps it's abusing the existing machinery, but I didn't know
how else to distinguish the two cases, and didn't feel like introducing
another way of passing on the exact same information.
> The comment probably needs to spell out that exclude_guides
> is the same as your "we were invoked as...".
Will do. That will also make the string --exclude-guides (i.e., with a
dash) appear in the comment, making it more likely to be found should
anyone change when and how --exclude-guides is implied.
> I wonder if we could change the name of that option. It is an
> undocumented, hidden option that we use internally, so it should be OK
> to do so (or we could always add another one). That might prevent
> somebody in the future from using --exclude-guides in more places and
> breaking your assumption here.
Perhaps, but I think that's better left for a separate patch, if really
necessary even with the expanded comment.
>> +count = split_cmdline(alias, &argv);
>> +if (count < 0)
>> +die(_("bad alias.%s string: %s"), cmd,
>> +split_cmdline_strerror(count));
>> +return alias;
>
> So we split only to find argv[0] here. But then we don't return it. That
> works because the split is done in place, meaning we must have inserted
> a NUL in alias. That's sufficiently subtle that it might be worth
> spelling it out in a comment.
OK, I actually had precisely
+ /*
+* We use split_cmdline() to get the first word of the
+* alias, to ensure that we use the same rules as when
+* the alias is actually used. split_cmdline()
+* modifies alias in-place.
+*/
in v1, but thought it might be overly verbose. I'll put it back in.
> We don't need to free alias here as we do above, because we're passing
> it back. We should free argv, though, I think (not its elements, just
> the array itself).
Yeah, I thought about this, and removing free(argv) was the last thing I
did before sending v1 - because we were going to leak alias anyway. I'm
happy to put it back in, along with...
> Unfortunately the caller is going to leak our returned "alias", [...] I think
> it may be OK to overlook
> that and just UNLEAK() it in cmd_help().
...this. Except I'd rather do the UNLEAK in check_git_cmd (the
documentation does say "only from cmd_* functions or their direct
helpers") to make it a more targeted annotation.
Thanks,
Rasmus
Re: [PATCH v2 3/3] git-help.txt: document "git help cmd" vs "git cmd --help" for aliases
On 2018-10-03 04:18, Jeff King wrote: > On Mon, Oct 01, 2018 at 01:21:07PM +0200, Rasmus Villemoes wrote: > >> >> +If an alias is given, git prints a note explaining what it is an alias >> +for on standard output. To get the manual page for the aliased >> +command, use `git COMMAND --help`. > > Funny English: "what it is an...". Maybe: > > If an alias is given, git shows the definition of the alias on > standard output. To get the manual page... Much better, thanks. Rasmus
[PATCH v3 2/3] git.c: handle_alias: prepend alias info when first argument is -h
Most git commands respond to -h anywhere in the command line, or at
least as a first and lone argument, by printing the usage
information. For aliases, we can provide a little more information that
might be useful in interpreting/understanding the following output by
prepending a line telling that the command is an alias, and for what.
When one invokes a simple alias, such as "cp = cherry-pick"
with -h, this results in
$ git cp -h
'cp' is aliased to 'cherry-pick'
usage: git cherry-pick [] ...
...
When the alias consists of more than one word, this provides the
additional benefit of informing the user which options are implicit in
using the alias, e.g. with "cp = cherry-pick -n":
$ git cp -h
'cp' is aliased to 'cherry-pick -n'
usage: git cherry-pick [] ...
...
For shell commands, we cannot know how it responds to -h, but printing
this line to stderr should not hurt, and can help in figuring out what
is happening in a case like
$ git sc -h
'sc' is aliased to '!somecommand'
somecommand: invalid option '-h'
Suggested-by: Jeff King
Signed-off-by: Rasmus Villemoes
---
git.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/git.c b/git.c
index a6f4b44af5..0211c2d4c0 100644
--- a/git.c
+++ b/git.c
@@ -318,6 +318,9 @@ static int handle_alias(int *argcp, const char ***argv)
alias_command = (*argv)[0];
alias_string = alias_lookup(alias_command);
if (alias_string) {
+ if (*argcp > 1 && !strcmp((*argv)[1], "-h"))
+ fprintf_ln(stderr, _("'%s' is aliased to '%s'"),
+ alias_command, alias_string);
if (alias_string[0] == '!') {
struct child_process child = CHILD_PROCESS_INIT;
int nongit_ok;
--
2.19.0
[PATCH v3 0/3] alias help tweaks
v2: Added patches 2 and 3, made "git cmd --help" unconditionally (no config option, no delay) redirect to the aliased command's help, preserve pre-existing behaviour of the spelling "git help cmd". v3: Add some additional comments in patch 1 and avoid triggering leak checker reports. Use better wording in patch 3. Rasmus Villemoes (3): help: redirect to aliased commands for "git cmd --help" git.c: handle_alias: prepend alias info when first argument is -h git-help.txt: document "git help cmd" vs "git cmd --help" for aliases Documentation/git-help.txt | 4 builtin/help.c | 34 +++--- git.c | 3 +++ 3 files changed, 38 insertions(+), 3 deletions(-) -- 2.19.0
[PATCH v3 3/3] git-help.txt: document "git help cmd" vs "git cmd --help" for aliases
This documents the existing behaviour of "git help cmd" when cmd is an alias, as well as providing a hint to use the "git cmd --help" form to be taken directly to the man page for the aliased command. Signed-off-by: Rasmus Villemoes --- Documentation/git-help.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt index 83d25d825a..86a6b42345 100644 --- a/Documentation/git-help.txt +++ b/Documentation/git-help.txt @@ -29,6 +29,10 @@ guide is brought up. The 'man' program is used by default for this purpose, but this can be overridden by other options or configuration variables. +If an alias is given, git shows the definition of the alias on +standard output. To get the manual page for the aliased command, use +`git COMMAND --help`. + Note that `git --help ...` is identical to `git help ...` because the former is internally converted into the latter. -- 2.19.0
[PATCH v3 1/3] help: redirect to aliased commands for "git cmd --help"
As discussed in the thread for v1 of this patch [1] [2], this changes the
rules for "git foo --help" when foo is an alias.
(0) When invoked as "git help foo", we continue to print the "foo is
aliased to bar" message and nothing else.
(1) If foo is an alias for a shell command, print "foo is aliased to
!bar" as usual.
(2) Otherwise, break the alias string into words, and pretend that "git
word0 --help" was called.
At least for me, getting the man page for git-cherry-pick directly with
"git cp --help" is more useful (and how I expect an alias to behave)
than the short "is aliased to" notice. It is also consistent with
"--help" generally providing more comprehensive help than "-h".
I believe that printing the "is aliased to" message also in case (2) has
value: Depending on pager setup, or if the user has help.format=web, the
message is still present immediately above the prompt when the user
quits the pager/returns to the terminal. That serves as an explanation
for why one was redirected to "man git-cherry-pick" from "git cp
--help", and if cp is actually 'cherry-pick -n', it reminds the user
that using cp has some flag implicitly set before firing off the next
command.
It also provides some useful info in case we end up erroring out, either
in the "bad alias string" check, or in the "No manual entry for gitbar"
case.
[1] https://public-inbox.org/git/20180926102636.30691-1...@rasmusvillemoes.dk/
[2] https://public-inbox.org/git/20180926184914.gc30...@sigill.intra.peff.net/
Signed-off-by: Rasmus Villemoes
---
builtin/help.c | 34 +++---
1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/builtin/help.c b/builtin/help.c
index 8d4f6dd301..e0e3fe62e9 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -415,9 +415,37 @@ static const char *check_git_cmd(const char* cmd)
alias = alias_lookup(cmd);
if (alias) {
- printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
- free(alias);
- exit(0);
+ const char **argv;
+ int count;
+
+ /*
+* handle_builtin() in git.c rewrites "git cmd --help"
+* to "git help --exclude-guides cmd", so we can use
+* exclude_guides to distinguish "git cmd --help" from
+* "git help cmd". In the latter case, or if cmd is an
+* alias for a shell command, just print the alias
+* definition.
+*/
+ if (!exclude_guides || alias[0] == '!') {
+ printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
+ free(alias);
+ exit(0);
+ }
+ /*
+* Otherwise, we pretend that the command was "git
+* word0 --help". We use split_cmdline() to get the
+* first word of the alias, to ensure that we use the
+* same rules as when the alias is actually
+* used. split_cmdline() modifies alias in-place.
+*/
+ fprintf_ln(stderr, _("'%s' is aliased to '%s'"), cmd, alias);
+ count = split_cmdline(alias, &argv);
+ if (count < 0)
+ die(_("bad alias.%s string: %s"), cmd,
+ split_cmdline_strerror(count));
+ free(argv);
+ UNLEAK(alias);
+ return alias;
}
if (exclude_guides)
--
2.19.0
Re: [PATCH v3 1/3] help: redirect to aliased commands for "git cmd --help"
On 2018-10-05 10:19, Junio C Hamano wrote: > Rasmus Villemoes writes: > >> >> I believe that printing the "is aliased to" message also in case (2) has >> value: Depending on pager setup, or if the user has help.format=web, the >> message is still present immediately above the prompt when the user >> quits the pager/returns to the terminal. That serves as an explanation >> for why one was redirected to "man git-cherry-pick" from "git cp >> --help", and if cp is actually 'cherry-pick -n', it reminds the user >> that using cp has some flag implicitly set before firing off the next >> command. >> >> It also provides some useful info in case we end up erroring out, either >> in the "bad alias string" check, or in the "No manual entry for gitbar" >> case. > > These two paragraphs were misleading, because they sounded as if you > were lamenting that you were somehow forbidden from doing so even > though you believe doing it is the right thing. > > But that is not what is happening. I think we should update the (2) > above to mention what you actually do in the code, perhaps like so: Yes, what I wrote was probably better placed below ---. > and hopefully remain visible when help.format=web is used, >"git bar --help" errors out, or the manpage of "git bar" is >short enough. It may not help if the help shows manpage on or, as in my case, the pager does not clear the terminal. I even think that's the default behaviour (due to X in $LESS) - at least, I don't have any magic in the environment or .gitconfig to achieve this. So it's not visible while the man page is shown in the pager, but upon exit from the pager. > It also is strange to count from (0); if the patchset is rerolled > again, I'd prefer to see these start counting from (1), in which > case this item will become (3). If you prefer, I can send a v4. Rasmus
[PATCH v4 0/3] alias help tweaks
v2: Added patches 2 and 3, made "git cmd --help" unconditionally (no config option, no delay) redirect to the aliased command's help, preserve pre-existing behaviour of the spelling "git help cmd". v3: Add some additional comments in patch 1 and avoid triggering leak checker reports. Use better wording in patch 3. v4: Reword commit log in patch 1. Rasmus Villemoes (3): help: redirect to aliased commands for "git cmd --help" git.c: handle_alias: prepend alias info when first argument is -h git-help.txt: document "git help cmd" vs "git cmd --help" for aliases Documentation/git-help.txt | 4 builtin/help.c | 34 +++--- git.c | 3 +++ 3 files changed, 38 insertions(+), 3 deletions(-) -- 2.19.1.4.g721af0fda3
[PATCH v4 1/3] help: redirect to aliased commands for "git cmd --help"
As discussed in the thread for v1 of this patch [1] [2], this changes the
rules for "git foo --help" when foo is an alias.
(1) When invoked as "git help foo", we continue to print the "foo is
aliased to bar" message and nothing else.
(2) If foo is an alias for a shell command, print "foo is aliased to
!bar" as usual.
(3) Otherwise, print "foo is aliased to bar" to the standard error
stream, and then break the alias string into words and pretend as if
"git word[0] --help" were called.
Getting the man page for git-cherry-pick directly with "git cp --help"
is consistent with "--help" generally providing more comprehensive help
than "-h". Printing the alias definition to stderr means that in certain
cases (e.g. if help.format=web or if the pager uses an alternate screen
and does not clear the terminal), one has
'cp' is aliased to 'cherry-pick -n'
above the prompt when one returns to the terminal/quits the pager, which
is a useful reminder that using 'cp' has some flag implicitly set. There
are cases where this information vanishes or gets scrolled
away, but being printed to stderr, it should never hurt.
[1] https://public-inbox.org/git/20180926102636.30691-1...@rasmusvillemoes.dk/
[2] https://public-inbox.org/git/20180926184914.gc30...@sigill.intra.peff.net/
Signed-off-by: Rasmus Villemoes
---
builtin/help.c | 34 +++---
1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/builtin/help.c b/builtin/help.c
index 8d4f6dd301..e0e3fe62e9 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -415,9 +415,37 @@ static const char *check_git_cmd(const char* cmd)
alias = alias_lookup(cmd);
if (alias) {
- printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
- free(alias);
- exit(0);
+ const char **argv;
+ int count;
+
+ /*
+* handle_builtin() in git.c rewrites "git cmd --help"
+* to "git help --exclude-guides cmd", so we can use
+* exclude_guides to distinguish "git cmd --help" from
+* "git help cmd". In the latter case, or if cmd is an
+* alias for a shell command, just print the alias
+* definition.
+*/
+ if (!exclude_guides || alias[0] == '!') {
+ printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
+ free(alias);
+ exit(0);
+ }
+ /*
+* Otherwise, we pretend that the command was "git
+* word0 --help". We use split_cmdline() to get the
+* first word of the alias, to ensure that we use the
+* same rules as when the alias is actually
+* used. split_cmdline() modifies alias in-place.
+*/
+ fprintf_ln(stderr, _("'%s' is aliased to '%s'"), cmd, alias);
+ count = split_cmdline(alias, &argv);
+ if (count < 0)
+ die(_("bad alias.%s string: %s"), cmd,
+ split_cmdline_strerror(count));
+ free(argv);
+ UNLEAK(alias);
+ return alias;
}
if (exclude_guides)
--
2.19.1.4.g721af0fda3
[PATCH v4 2/3] git.c: handle_alias: prepend alias info when first argument is -h
Most git commands respond to -h anywhere in the command line, or at
least as a first and lone argument, by printing the usage
information. For aliases, we can provide a little more information that
might be useful in interpreting/understanding the following output by
prepending a line telling that the command is an alias, and for what.
When one invokes a simple alias, such as "cp = cherry-pick"
with -h, this results in
$ git cp -h
'cp' is aliased to 'cherry-pick'
usage: git cherry-pick [] ...
...
When the alias consists of more than one word, this provides the
additional benefit of informing the user which options are implicit in
using the alias, e.g. with "cp = cherry-pick -n":
$ git cp -h
'cp' is aliased to 'cherry-pick -n'
usage: git cherry-pick [] ...
...
For shell commands, we cannot know how it responds to -h, but printing
this line to stderr should not hurt, and can help in figuring out what
is happening in a case like
$ git sc -h
'sc' is aliased to '!somecommand'
somecommand: invalid option '-h'
Suggested-by: Jeff King
Signed-off-by: Rasmus Villemoes
---
git.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/git.c b/git.c
index a6f4b44af5..0211c2d4c0 100644
--- a/git.c
+++ b/git.c
@@ -318,6 +318,9 @@ static int handle_alias(int *argcp, const char ***argv)
alias_command = (*argv)[0];
alias_string = alias_lookup(alias_command);
if (alias_string) {
+ if (*argcp > 1 && !strcmp((*argv)[1], "-h"))
+ fprintf_ln(stderr, _("'%s' is aliased to '%s'"),
+ alias_command, alias_string);
if (alias_string[0] == '!') {
struct child_process child = CHILD_PROCESS_INIT;
int nongit_ok;
--
2.19.1.4.g721af0fda3
[PATCH v4 3/3] git-help.txt: document "git help cmd" vs "git cmd --help" for aliases
This documents the existing behaviour of "git help cmd" when cmd is an alias, as well as providing a hint to use the "git cmd --help" form to be taken directly to the man page for the aliased command. Signed-off-by: Rasmus Villemoes --- Documentation/git-help.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt index 83d25d825a..86a6b42345 100644 --- a/Documentation/git-help.txt +++ b/Documentation/git-help.txt @@ -29,6 +29,10 @@ guide is brought up. The 'man' program is used by default for this purpose, but this can be overridden by other options or configuration variables. +If an alias is given, git shows the definition of the alias on +standard output. To get the manual page for the aliased command, use +`git COMMAND --help`. + Note that `git --help ...` is identical to `git help ...` because the former is internally converted into the latter. -- 2.19.1.4.g721af0fda3
[PATCH 1/3] Documentation/git-send-email.txt: style fixes
For consistency, add full stops in a few places and outdent a line by one space. Signed-off-by: Rasmus Villemoes --- Documentation/git-send-email.txt | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 465a4ecbed..ea6ea512fe 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -321,16 +321,16 @@ Automating auto-cc of: + -- -- 'author' will avoid including the patch author -- 'self' will avoid including the sender +- 'author' will avoid including the patch author. +- 'self' will avoid including the sender. - 'cc' will avoid including anyone mentioned in Cc lines in the patch header except for self (use 'self' for that). - 'bodycc' will avoid including anyone mentioned in Cc lines in the patch body (commit message) except for self (use 'self' for that). - 'sob' will avoid including anyone mentioned in Signed-off-by lines except - for self (use 'self' for that). + for self (use 'self' for that). - 'cccmd' will avoid running the --cc-cmd. -- 'body' is equivalent to 'sob' + 'bodycc' +- 'body' is equivalent to 'sob' + 'bodycc'. - 'all' will suppress all auto cc values. -- + -- 2.19.1.6.g084f1d7761
[PATCH 0/3] send-email: Also pick up cc addresses from -by trailers
This has been attempted multiple times before, but I hope that this can make it in this time around. That *-by addresses are not automatically Cc'ed certainly still surprises people from time to time. I hope that this addresses all the concerns Junio had in https://lkml.org/lkml/2016/8/31/768 . For the name, I chose 'misc-by', since that has -by in its name. I am fine with absolutely any other name (bodyby, body-by, by-trailers, ...). I doubt we can find a short token that is completely self-explanatory, and note that one has to look in the man page anyway to know what 'sob' means in this line from 'git send-email -h': --suppress-cc * author, self, sob, cc, cccmd, body, bodycc, all. Rasmus Villemoes (3): Documentation/git-send-email.txt: style fixes send-email: only consider lines containing @ or <> for automatic Cc'ing send-email: also pick up cc addresses from -by trailers Documentation/git-send-email.txt | 11 +++ git-send-email.perl | 19 +-- 2 files changed, 20 insertions(+), 10 deletions(-) -- 2.19.1.6.g084f1d7761
[PATCH 3/3] send-email: also pick up cc addresses from -by trailers
When rerolling a patch series, including various Reviewed-by etc. that
may have come in, it is quite convenient to have git-send-email
automatically cc those people.
So pick up any *-by lines, with a new suppression category 'misc-by',
but special-case Signed-off-by, since that already has its own
suppression category. It seems natural to make 'misc-by' implied by
'body'.
Based-on-patch-by: Joe Perches
Signed-off-by: Rasmus Villemoes
---
Documentation/git-send-email.txt | 5 -
git-send-email.perl | 14 --
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index ea6ea512fe..f6010ac68b 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -329,8 +329,11 @@ Automating
patch body (commit message) except for self (use 'self' for that).
- 'sob' will avoid including anyone mentioned in Signed-off-by lines except
for self (use 'self' for that).
+- 'misc-by' will avoid including anyone mentioned in Acked-by,
+ Reviewed-by, Tested-by and other "-by" lines in the patch body,
+ except Signed-off-by (use 'sob' for that).
- 'cccmd' will avoid running the --cc-cmd.
-- 'body' is equivalent to 'sob' + 'bodycc'.
+- 'body' is equivalent to 'sob' + 'bodycc' + 'misc-by'.
- 'all' will suppress all auto cc values.
--
+
diff --git a/git-send-email.perl b/git-send-email.perl
index 1916159d2a..7a6391e5d8 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -94,7 +94,7 @@ sub usage {
--identity* Use the sendemail. options.
--to-cmd * Email To: via ` \$patch_path`
--cc-cmd * Email Cc: via ` \$patch_path`
---suppress-cc * author, self, sob, cc, cccmd, body,
bodycc, all.
+--suppress-cc * author, self, sob, cc, cccmd, body,
bodycc, misc-by, all.
--[no-]cc-cover* Email Cc: addresses in the cover letter.
--[no-]to-cover* Email To: addresses in the cover letter.
--[no-]signed-off-by-cc* Send to Signed-off-by: addresses. Default
on.
@@ -454,13 +454,13 @@ sub read_config {
if (@suppress_cc) {
foreach my $entry (@suppress_cc) {
die sprintf(__("Unknown --suppress-cc field: '%s'\n"), $entry)
- unless $entry =~
/^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/;
+ unless $entry =~
/^(?:all|cccmd|cc|author|self|sob|body|bodycc|misc-by)$/;
$suppress_cc{$entry} = 1;
}
}
if ($suppress_cc{'all'}) {
- foreach my $entry (qw (cccmd cc author self sob body bodycc)) {
+ foreach my $entry (qw (cccmd cc author self sob body bodycc misc-by)) {
$suppress_cc{$entry} = 1;
}
delete $suppress_cc{'all'};
@@ -471,7 +471,7 @@ sub read_config {
$suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc;
if ($suppress_cc{'body'}) {
- foreach my $entry (qw (sob bodycc)) {
+ foreach my $entry (qw (sob bodycc misc-by)) {
$suppress_cc{$entry} = 1;
}
delete $suppress_cc{'body'};
@@ -1681,7 +1681,7 @@ sub process_file {
# Now parse the message body
while(<$fh>) {
$message .= $_;
- if (/^(Signed-off-by|Cc): (.*)/i) {
+ if (/^([a-z-]*-by|Cc): (.*)/i) {
chomp;
my ($what, $c) = ($1, $2);
# strip garbage for the address we'll use:
@@ -1691,7 +1691,9 @@ sub process_file {
if ($sc eq $sender) {
next if ($suppress_cc{'self'});
} else {
- next if $suppress_cc{'sob'} and $what =~
/Signed-off-by/i;
+ next if $suppress_cc{'sob'} and $what =~
/^Signed-off-by$/i;
+ next if $suppress_cc{'misc-by'}
+ and $what =~ /-by$/i and $what !~
/^Signed-off-by$/i;
next if $suppress_cc{'bodycc'} and $what =~
/Cc/i;
}
if ($c !~ /.+@.+|<.+>/) {
--
2.19.1.6.g084f1d7761
[PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing
While the address sanitizations routines do accept local addresses, that
is almost never what is meant in a Cc or Signed-off-by trailer.
Looking through all the signed-off-by lines in the linux kernel tree
without a @, there are mostly two patterns: Either just a full name, or
a full name followed by (i.e., with the word at
instead of a @), and minor variations. For cc lines, the same patterns
appear, along with lots of "cc stable" variations that do not actually
name sta...@vger.kernel.org
Cc: stable # introduced pre-git times
cc: stable.kernel.org
In the cases, one gets a chance to interactively
fix it. But when there is no <> pair, it seems we end up just using the
first word as a (local) address.
As the number of cases where a local address really was meant is
likely (and anecdotally) quite small compared to the number of cases
where we end up cc'ing a garbage address, insist on at least a @ or a <>
pair being present.
This is also preparation for the next patch, where we are likely to
encounter even more non-addresses in -by lines, such as
Reported-by: Coverity
Patch-generated-by: Coccinelle
Signed-off-by: Rasmus Villemoes
---
git-send-email.perl | 5 +
1 file changed, 5 insertions(+)
diff --git a/git-send-email.perl b/git-send-email.perl
index 2be5dac337..1916159d2a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1694,6 +1694,11 @@ sub process_file {
next if $suppress_cc{'sob'} and $what =~
/Signed-off-by/i;
next if $suppress_cc{'bodycc'} and $what =~
/Cc/i;
}
+ if ($c !~ /.+@.+|<.+>/) {
+ printf("(body) Ignoring %s from line '%s'\n",
+ $what, $_) unless $quiet;
+ next;
+ }
push @cc, $c;
printf(__("(body) Adding cc: %s from line '%s'\n"),
$c, $_) unless $quiet;
--
2.19.1.6.g084f1d7761
Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing
On 2018-10-10 14:57, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Oct 10 2018, Rasmus Villemoes wrote:
>
>> +if ($c !~ /.+@.+|<.+>/) {
>> +printf("(body) Ignoring %s from line '%s'\n",
>> +$what, $_) unless $quiet;
>> +next;
>> +}
>> push @cc, $c;
>> printf(__("(body) Adding cc: %s from line '%s'\n"),
>> $c, $_) unless $quiet;
>
> There's a extract_valid_address() function in git-send-email already,
> shouldn't this be:
>
> if (!extract_valid_address($c)) {
> [...]
>
> Or is there a good reason not to use that function in this case?
>
I considered that (and also had a version where I simply insisted on a @
being present), but that means the user no longer would get prompted
about the cases where the address was just slightly obfuscated, e.g. the
Cc: John Doe
cases, which would be a regression, I guess. So I do want to pass such
cases through, and have them be dealt with when process_address_list
gets called.
So this is just a rather minimal and simple heuristic, which should
still be able to handle the vast majority of cases correctly, and at
least almost never exclude anything that might have a chance of becoming
a real address.
Rasmus
Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing
On 2018-10-11 08:06, Junio C Hamano wrote: > Rasmus Villemoes writes: > >> I considered that (and also had a version where I simply insisted on a @ >> being present), but that means the user no longer would get prompted >> about the cases where the address was just slightly obfuscated, e.g. the >> >> Cc: John Doe >> >> cases, which would be a regression, I guess. So I do want to pass such >> cases through, and have them be dealt with when process_address_list >> gets called. > > We are only tightening with this patch, and we were passing any > random things through with the original code anyway, so without > [PATCH 3/3], this step must be making it only better, but I have to > wonder one thing. > > You keep saying "get prompted" but are we sure we always stop and > ask (and preferrably---fail and abort when the end user is not > available at the terminal to interact) when we have such a > questionable address? > I dunno. I guess I've never considered non-interactive use of send-email. But the ask() in validate_address does have default q[uit], which I suppose gets used if stdin is /dev/null? I did do an experiment adding a bunch of the random odd patterns found in kernel commit messages to see how send-email reacted before/after this, and the only things that got filtered away (i.e., no longer prompted about) were things where the user probably couldn't easily fix it anyway. In the cases where there was a "Cc: stable" that might be fixed to the proper sta...@vger.kernel.org, the logic in extract_valid_address simply saw that as a local address, so we didn't use to be prompted, but simply sent to stable@localhost. Now we simply don't pass that through. So, for non-interactive use, I guess the effect of this patch is to allow more cases to complete succesfully, since we filter away (some) cases where extract_valid_address would cause us to prompt (and thus quit). So, it seems you're ok with this tightening, but some comment on the non-interactive use case should be made in the commit log? Or am I misunderstanding? Thanks, Rasmus
Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers
On 2018-10-11 08:18, Junio C Hamano wrote:
> Rasmus Villemoes writes:
> we now ...
>
>> +next if $suppress_cc{'sob'} and $what =~
>> /^Signed-off-by$/i;
>
> ... must make sure what we have is _exactly_ "signed-off-by" when
> 'sob' is suppressed. Makes sense.
>
>> +next if $suppress_cc{'misc-by'}
>> +and $what =~ /-by$/i and $what !~
>> /^Signed-off-by$/i;
>
> And this is the opposite side of the same coin, which also makes sense.
Yup, I started by just adding the misc-by line, then remembered that
people sometimes use not-signed-off-by variants, and went back and
anchored the s-o-b case. So now it's no longer so minimal, and...
> I wonder if it would make it easier to grok if we made the logic
> inside out, i.e.
>
> if ($sc eq $sender) {
> ...
> } else {
> if ($what =~ /^Signed-off-by$/i) {
> next if $suppress_cc{'sob'};
> } elsif ($what =~ /-by$/i) {
> next if $suppress_cc{'misc'};
> } elsif ($what =~ /^Cc$/i) {
> next if $suppress_cc{'bodycc'};>}
...yes, that's probably more readable.
Thanks,
Rasmus
Re: [PATCH 3/3] send-email: also pick up cc addresses from -by trailers
On 2018-10-16 07:57, Junio C Hamano wrote:
> Rasmus Villemoes writes:
>
>>> I wonder if it would make it easier to grok if we made the logic
>>> inside out, i.e.
>>>
>>> if ($sc eq $sender) {
>>> ...
>>> } else {
>>> if ($what =~ /^Signed-off-by$/i) {
>>> next if $suppress_cc{'sob'};
>>> } elsif ($what =~ /-by$/i) {
>>> next if $suppress_cc{'misc'};
>>> } elsif ($what =~ /^Cc$/i) {
>>> next if $suppress_cc{'bodycc'};>}
>>
>> ...yes, that's probably more readable.
>
> OK, unless there is more comments and suggestions for improvements,
> can you send in a final version sometime not in so distant future so
> that we won't forget?
Will do, I was just waiting for more comments to come in.
It may be surprising to existing users that
> the command now suddenly adds more addresses the user did not think
> would be added, but it would probably be easy enough for them to
> work around.
Yeah, I thought about that, but unfortunately the whole auto-cc business
is not built around some config options where we can add a new and
default false. Also note that there are also cases currently where the
user sends off a patch series and is surprised that lots of intended
recipients were not cc'ed (that's how I picked this subject up again; I
had a long series where I had put specific Cc's in each patch, at v2,
some of those had given a Reviewed-by, so I changed the tags, and a
--dry-run told me they wouldn't get the new version).
I suppose one could make use of -by addresses dependent on a new opt-in
config option, but that's not very elegant. Another option is, for a
release or two, to make a little (more) noise when picking up a -by
address - something like setting a flag in the ($what =~ /-by/) branch,
and testing that flag somewhere in send_message(). But I suppose the
message printed when needs_confirm eq "inform" is generic enough.
Rasmus
[PATCH v2 3/3] send-email: also pick up cc addresses from -by trailers
When rerolling a patch series, including various Reviewed-by etc. that
may have come in, it is quite convenient to have git-send-email
automatically cc those people.
So pick up any *-by lines, with a new suppression category 'misc-by',
but special-case Signed-off-by, since that already has its own
suppression category. It seems natural to make 'misc-by' implied by
'body'.
Based-on-patch-by: Joe Perches
Signed-off-by: Rasmus Villemoes
---
Documentation/git-send-email.txt | 5 -
git-send-email.perl | 19 ---
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index ea6ea512fe..f6010ac68b 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -329,8 +329,11 @@ Automating
patch body (commit message) except for self (use 'self' for that).
- 'sob' will avoid including anyone mentioned in Signed-off-by lines except
for self (use 'self' for that).
+- 'misc-by' will avoid including anyone mentioned in Acked-by,
+ Reviewed-by, Tested-by and other "-by" lines in the patch body,
+ except Signed-off-by (use 'sob' for that).
- 'cccmd' will avoid running the --cc-cmd.
-- 'body' is equivalent to 'sob' + 'bodycc'.
+- 'body' is equivalent to 'sob' + 'bodycc' + 'misc-by'.
- 'all' will suppress all auto cc values.
--
+
diff --git a/git-send-email.perl b/git-send-email.perl
index 1916159d2a..58c6aa9d0e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -94,7 +94,7 @@ sub usage {
--identity* Use the sendemail. options.
--to-cmd * Email To: via ` \$patch_path`
--cc-cmd * Email Cc: via ` \$patch_path`
---suppress-cc * author, self, sob, cc, cccmd, body,
bodycc, all.
+--suppress-cc * author, self, sob, cc, cccmd, body,
bodycc, misc-by, all.
--[no-]cc-cover* Email Cc: addresses in the cover letter.
--[no-]to-cover* Email To: addresses in the cover letter.
--[no-]signed-off-by-cc* Send to Signed-off-by: addresses. Default
on.
@@ -454,13 +454,13 @@ sub read_config {
if (@suppress_cc) {
foreach my $entry (@suppress_cc) {
die sprintf(__("Unknown --suppress-cc field: '%s'\n"), $entry)
- unless $entry =~
/^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/;
+ unless $entry =~
/^(?:all|cccmd|cc|author|self|sob|body|bodycc|misc-by)$/;
$suppress_cc{$entry} = 1;
}
}
if ($suppress_cc{'all'}) {
- foreach my $entry (qw (cccmd cc author self sob body bodycc)) {
+ foreach my $entry (qw (cccmd cc author self sob body bodycc misc-by)) {
$suppress_cc{$entry} = 1;
}
delete $suppress_cc{'all'};
@@ -471,7 +471,7 @@ sub read_config {
$suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc;
if ($suppress_cc{'body'}) {
- foreach my $entry (qw (sob bodycc)) {
+ foreach my $entry (qw (sob bodycc misc-by)) {
$suppress_cc{$entry} = 1;
}
delete $suppress_cc{'body'};
@@ -1681,7 +1681,7 @@ sub process_file {
# Now parse the message body
while(<$fh>) {
$message .= $_;
- if (/^(Signed-off-by|Cc): (.*)/i) {
+ if (/^([a-z-]*-by|Cc): (.*)/i) {
chomp;
my ($what, $c) = ($1, $2);
# strip garbage for the address we'll use:
@@ -1691,8 +1691,13 @@ sub process_file {
if ($sc eq $sender) {
next if ($suppress_cc{'self'});
} else {
- next if $suppress_cc{'sob'} and $what =~
/Signed-off-by/i;
- next if $suppress_cc{'bodycc'} and $what =~
/Cc/i;
+ if ($what =~ /^Signed-off-by$/i) {
+ next if $suppress_cc{'sob'};
+ } elsif ($what =~ /-by$/i) {
+ next if $suppress_cc{'misc-by'};
+ } elsif ($what =~ /Cc/i) {
+ next if $suppress_cc{'bodycc'};
+ }
}
if ($c !~ /.+@.+|<.+>/) {
printf("(body) Ignoring %s from line '%s'\n",
--
2.19.1.6.gbde171bbf5
[PATCH v2 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing
While the address sanitizations routines do accept local addresses, that
is almost never what is meant in a Cc or Signed-off-by trailer.
Looking through all the signed-off-by lines in the linux kernel tree
without a @, there are mostly two patterns: Either just a full name, or
a full name followed by (i.e., with the word at
instead of a @), and minor variations. For cc lines, the same patterns
appear, along with lots of "cc stable" variations that do not actually
name sta...@vger.kernel.org
Cc: stable # introduced pre-git times
cc: stable.kernel.org
In the cases, one gets a chance to interactively
fix it. But when there is no <> pair, it seems we end up just using the
first word as a (local) address.
As the number of cases where a local address really was meant is
likely (and anecdotally) quite small compared to the number of cases
where we end up cc'ing a garbage address, insist on at least a @ or a <>
pair being present.
This is also preparation for the next patch, where we are likely to
encounter even more non-addresses in -by lines, such as
Reported-by: Coverity
Patch-generated-by: Coccinelle
Signed-off-by: Rasmus Villemoes
---
git-send-email.perl | 5 +
1 file changed, 5 insertions(+)
diff --git a/git-send-email.perl b/git-send-email.perl
index 2be5dac337..1916159d2a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1694,6 +1694,11 @@ sub process_file {
next if $suppress_cc{'sob'} and $what =~
/Signed-off-by/i;
next if $suppress_cc{'bodycc'} and $what =~
/Cc/i;
}
+ if ($c !~ /.+@.+|<.+>/) {
+ printf("(body) Ignoring %s from line '%s'\n",
+ $what, $_) unless $quiet;
+ next;
+ }
push @cc, $c;
printf(__("(body) Adding cc: %s from line '%s'\n"),
$c, $_) unless $quiet;
--
2.19.1.6.gbde171bbf5
[PATCH v2 0/3] send-email: Also pick up cc addresses from -by trailers
This series extends the logic in git-send-email so that addresses appearing in various *-by: trailers (e.g. Reviewed-by, Acked-by, Tested-by) are picked up and added to the Cc list, in addition to the current logic that picks up Cc: and Signed-off-by: lines. This deliberately only picks up -by trailers (as opposed to any trailer), based on the heuristic that the -by suffix strongly suggests there's a (name +) email address after the colon. This avoids having to deal with BugID:, Link:, or other such tags. Still, widening to any -by trailer does increase the risk that we will pick up stuff that is not an email address, such as Reported-by: Coverity Patch-generated-by: Coccinelle where send-email then ends up cc'ing the local 'coverity' user. Patch 2 tries to weed out the obvious no-email-address-here cases, which should also help avoid cc'ing garbage (local) addresses for malformed cc and signed-off-by lines. Patch 3 is then mostly mechanical, introducing the misc-by suppression category and changing the regexp for matching trailer lines to include .*-by. Changes in v2: Rework logic in patch 3 as suggested by Junio. v1 cover letter: This has been attempted multiple times before, but I hope that this can make it in this time around. That *-by addresses are not automatically Cc'ed certainly still surprises people from time to time. I hope that this addresses all the concerns Junio had in https://lkml.org/lkml/2016/8/31/768 . For the name, I chose 'misc-by', since that has -by in its name. I am fine with absolutely any other name (bodyby, body-by, by-trailers, ...). I doubt we can find a short token that is completely self-explanatory, and note that one has to look in the man page anyway to know what 'sob' means in this line from 'git send-email -h': --suppress-cc * author, self, sob, cc, cccmd, body, bodycc, all. Rasmus Villemoes (3): Documentation/git-send-email.txt: style fixes send-email: only consider lines containing @ or <> for automatic Cc'ing send-email: also pick up cc addresses from -by trailers Documentation/git-send-email.txt | 11 +++ git-send-email.perl | 24 +--- 2 files changed, 24 insertions(+), 11 deletions(-) -- 2.19.1.6.gbde171bbf5
[PATCH v2 1/3] Documentation/git-send-email.txt: style fixes
For consistency, add full stops in a few places and outdent a line by one space. Signed-off-by: Rasmus Villemoes --- Documentation/git-send-email.txt | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 465a4ecbed..ea6ea512fe 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -321,16 +321,16 @@ Automating auto-cc of: + -- -- 'author' will avoid including the patch author -- 'self' will avoid including the sender +- 'author' will avoid including the patch author. +- 'self' will avoid including the sender. - 'cc' will avoid including anyone mentioned in Cc lines in the patch header except for self (use 'self' for that). - 'bodycc' will avoid including anyone mentioned in Cc lines in the patch body (commit message) except for self (use 'self' for that). - 'sob' will avoid including anyone mentioned in Signed-off-by lines except - for self (use 'self' for that). + for self (use 'self' for that). - 'cccmd' will avoid running the --cc-cmd. -- 'body' is equivalent to 'sob' + 'bodycc' +- 'body' is equivalent to 'sob' + 'bodycc'. - 'all' will suppress all auto cc values. -- + -- 2.19.1.6.gbde171bbf5
[RFC PATCH 0/5] builtin/grep.c: fix a tiny logic flaw
Background: I noticed that the condition in add_work() for when the producer could add a new item was oddly different from the other conditions on the todo_* bookkeeping variables - namely, in the other cases we want todo_a != todo_b, whereas in add_work the condition is todo_a+1!=todo_b. Another hint that something is slightly off is that the code would break down if TODO_SIZE was set to 1. The practical effect is negligible, and fixing it seems to be a bit involved, hence probably not worth the churn - and if that's the verdict, I suggest adding a comment in add_work() for future readers and/or people who copy the producer/consumer logic to their own code. Rasmus Villemoes (5): builtin/grep.c: change todo_* variables to unsigned builtin/grep.c: refactor loop in work_done() slightly builtin/grep.c: add shorthand for &todo[todo_end] in add_work() builtin/grep.c: add todo_item helper builtin/grep.c: fix fence-post error in add_work() builtin/grep.c | 40 1 file changed, 24 insertions(+), 16 deletions(-) -- 2.20.1
[RFC PATCH 4/5] builtin/grep.c: add todo_item helper
Use a helper for indexing into the todo array with any of the todo_*
variables, in preparation for not keeping those reduced mod TODO_SIZE.
Signed-off-by: Rasmus Villemoes
---
builtin/grep.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 92b9e6198d..35ed79b0dd 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -62,6 +62,11 @@ static unsigned int todo_start;
static unsigned int todo_end;
static unsigned int todo_done;
+static inline struct work_item *todo_item(unsigned int idx)
+{
+ return &todo[idx % ARRAY_SIZE(todo)];
+}
+
/* Has all work items been added? */
static int all_work_added;
@@ -101,7 +106,7 @@ static void add_work(struct grep_opt *opt, const struct
grep_source *gs)
pthread_cond_wait(&cond_write, &grep_mutex);
}
- w = &todo[todo_end];
+ w = todo_item(todo_end);
w->source = *gs;
if (opt->binary != GREP_BINARY_TEXT)
grep_source_load_driver(&w->source, opt->repo->index);
@@ -125,7 +130,7 @@ static struct work_item *get_work(void)
if (todo_start == todo_end && all_work_added) {
ret = NULL;
} else {
- ret = &todo[todo_start];
+ ret = todo_item(todo_start);
todo_start = (todo_start + 1) % ARRAY_SIZE(todo);
}
grep_unlock();
@@ -141,7 +146,7 @@ static void work_done(struct work_item *w)
old_done = todo_done;
for(; todo_done != todo_start;
todo_done = (todo_done+1) % ARRAY_SIZE(todo)) {
- w = &todo[todo_done];
+ w = todo_item(todo_done);
if (!w->done)
break;
if (w->out.len) {
--
2.20.1
[RFC PATCH 5/5] builtin/grep.c: fix fence-post error in add_work()
We're only using 127 of the slots in todo[], which can easily be seen
by adding this hack
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -93,6 +93,8 @@ static int skip_first_line;
static void add_work(struct grep_opt *opt, const struct grep_source *gs)
{
+ static int count;
+
grep_lock();
while ((todo_end+1) % ARRAY_SIZE(todo) == todo_done) {
@@ -108,6 +110,7 @@ static void add_work(struct grep_opt *opt, const struct
grep_source *gs)
todo_end = (todo_end + 1) % ARRAY_SIZE(todo);
pthread_cond_signal(&cond_add);
+ fprintf(stderr, "added work item %3d\n", ++count);
grep_unlock();
}
@@ -173,6 +176,7 @@ static void *run(void *arg)
int hit = 0;
struct grep_opt *opt = arg;
+ sleep(2);
while (1) {
struct work_item *w = get_work();
if (!w)
Of course, just removing the +1 after todo_end would be instant
deadlock, since nothing would ever change todo_end or todo_done from
0.
The problem boils down to the fact that arithmetic mod 128 cannot
capture the 129 possible values of end-done (which
is (end-start)+(start-done), i.e. the total number of items waiting to
be picked up or that have been picked up by a worker).
To fix this, don't keep the todo_* variables reduced mod 128, and only
do that when using them as indices into todo[]. Then we can rewrite
the condition in add_work() to the proper one: Wait until todo_end is
not a full round ahead of todo_done.
Signed-off-by: Rasmus Villemoes
---
builtin/grep.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 35ed79b0dd..ce158cabbb 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -102,7 +102,7 @@ static void add_work(struct grep_opt *opt, const struct
grep_source *gs)
grep_lock();
- while ((todo_end+1) % ARRAY_SIZE(todo) == todo_done) {
+ while (todo_end - todo_done == ARRAY_SIZE(todo)) {
pthread_cond_wait(&cond_write, &grep_mutex);
}
@@ -112,7 +112,7 @@ static void add_work(struct grep_opt *opt, const struct
grep_source *gs)
grep_source_load_driver(&w->source, opt->repo->index);
w->done = 0;
strbuf_reset(&w->out);
- todo_end = (todo_end + 1) % ARRAY_SIZE(todo);
+ todo_end += 1;
pthread_cond_signal(&cond_add);
grep_unlock();
@@ -131,7 +131,7 @@ static struct work_item *get_work(void)
ret = NULL;
} else {
ret = todo_item(todo_start);
- todo_start = (todo_start + 1) % ARRAY_SIZE(todo);
+ todo_start += 1;
}
grep_unlock();
return ret;
@@ -144,8 +144,7 @@ static void work_done(struct work_item *w)
grep_lock();
w->done = 1;
old_done = todo_done;
- for(; todo_done != todo_start;
- todo_done = (todo_done+1) % ARRAY_SIZE(todo)) {
+ for(; todo_done != todo_start; todo_done += 1) {
w = todo_item(todo_done);
if (!w->done)
break;
--
2.20.1
[RFC PATCH 1/5] builtin/grep.c: change todo_* variables to unsigned
In preparation for subsequent patches that make todo_* free-running
instead of reducing them mod TODO_SIZE, change their type to unsigned
to avoid undefined behaviour in case anybody ever greps more than 2
billion files.
Signed-off-by: Rasmus Villemoes
---
builtin/grep.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 580fd38f41..6c1e90d43b 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -58,9 +58,9 @@ struct work_item {
*/
#define TODO_SIZE 128
static struct work_item todo[TODO_SIZE];
-static int todo_start;
-static int todo_end;
-static int todo_done;
+static unsigned int todo_start;
+static unsigned int todo_end;
+static unsigned int todo_done;
/* Has all work items been added? */
static int all_work_added;
@@ -132,7 +132,7 @@ static struct work_item *get_work(void)
static void work_done(struct work_item *w)
{
- int old_done;
+ unsigned int old_done;
grep_lock();
w->done = 1;
--
2.20.1
[RFC PATCH 2/5] builtin/grep.c: refactor loop in work_done() slightly
As preparation for changing all accesses to the todo array to use a
helper function, do the .done check in the loop body.
Signed-off-by: Rasmus Villemoes
---
builtin/grep.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 6c1e90d43b..211ae54222 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -137,9 +137,11 @@ static void work_done(struct work_item *w)
grep_lock();
w->done = 1;
old_done = todo_done;
- for(; todo[todo_done].done && todo_done != todo_start;
+ for(; todo_done != todo_start;
todo_done = (todo_done+1) % ARRAY_SIZE(todo)) {
w = &todo[todo_done];
+ if (!w->done)
+ break;
if (w->out.len) {
const char *p = w->out.buf;
size_t len = w->out.len;
--
2.20.1
[RFC PATCH 3/5] builtin/grep.c: add shorthand for &todo[todo_end] in add_work()
Signed-off-by: Rasmus Villemoes
---
builtin/grep.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 211ae54222..92b9e6198d 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -93,18 +93,20 @@ static int skip_first_line;
static void add_work(struct grep_opt *opt, const struct grep_source *gs)
{
+ struct work_item *w;
+
grep_lock();
while ((todo_end+1) % ARRAY_SIZE(todo) == todo_done) {
pthread_cond_wait(&cond_write, &grep_mutex);
}
- todo[todo_end].source = *gs;
+ w = &todo[todo_end];
+ w->source = *gs;
if (opt->binary != GREP_BINARY_TEXT)
- grep_source_load_driver(&todo[todo_end].source,
- opt->repo->index);
- todo[todo_end].done = 0;
- strbuf_reset(&todo[todo_end].out);
+ grep_source_load_driver(&w->source, opt->repo->index);
+ w->done = 0;
+ strbuf_reset(&w->out);
todo_end = (todo_end + 1) % ARRAY_SIZE(todo);
pthread_cond_signal(&cond_add);
--
2.20.1
Re: [PATCH] send-email: don't cc *-by lines with '-' prefix
On 16/03/2019 20.26, Baruch Siach wrote:
> Since commit ef0cc1df90f6b ("send-email: also pick up cc addresses from
> -by trailers") in git version 2.20, git send-email adds to cc list
> addresses from all *-by lines. As a side effect a line with
> '-Signed-off-by' is now also added to cc. This makes send-email pick
> lines from patches that remove patch files from the git repo. This is
> common in the Buildroot project that often removes (and adds) patch
> files that have 'Signed-off-by' in their patch description part.
Yocto/OpenEmbedded and other projects do the same
> Consider only *-by lines that start with [a-z] (case insensitive) to
> avoid unrelated addresses in cc.
While I agree with Joe in principle that we really should not look
inside the diff part, all lines there start with [ +-], so we wouldn't
normally pick up anything from that due to the anchoring. Except for the
misc-by regexp that added hyphens to grab Reported-and-tested-by and
similar. So this is by far the simplest fix that doesn't hurt the common
use cases the misc-by handling was added to support, so
Acked-by: Rasmus Villemoes
Rasmus
feature request: allow commit.email config setting
As part of my dayjob, I did and still do some work on an upstream project. A while ago, I was granted commit access to that project. However, upstream asked that I would register with their system using a private email, or at least one that wouldn't change if I changed jobs, rather than my work email. Now, I (and my employer) would like that the work I do as part of my current job on that project has my work email in the Author field, but since the commit access was granted to me privately/personally, it would be nice if the Committer email was the one I used to register with their system. I can set GIT_COMMITTER_EMAIL in the environment, but that is rather inconvenient, since that means I have to remember to do that in the shell I'm using for that particular project, and I can't use that shell for other projects. So it would be really nice if I could set commit.email = $private-email in the local .git/config for that particular project. I don't personally have a use for commit.name (when missing, that should just use user.name as usual), but it would probably be most consistent to allow that too. I tried looking into ident.c, but it doesn't seem like it is straight-forward to implement. Probably fmt_ident, ident_default_email etc. would need to be passed information about what purpose the ident is to be used for. So before trying to implement this, I want to hear if this is a reasonable thing to support. Also, I'm sure there are some subtle semantics that would need to be decided and gotchas to watch out for. Rasmus
Re: feature request: allow commit.email config setting
On 2018-08-30 20:13, Eric Sunshine wrote: > On Thu, Aug 30, 2018 at 7:26 AM Rasmus Villemoes > wrote: >> I can set GIT_COMMITTER_EMAIL in the environment, but that is >> rather inconvenient, since that means I have to remember to do that in >> the shell I'm using for that particular project, and I can't use that >> shell for other projects. So it would be really nice if I could set >> commit.email = $private-email in the local .git/config for that >> particular project. > > Aside from modifying Git itself to support such a use-case, another > (perhaps more pragmatic) approach would be to use a tool, such as > direnv[1], which automatically sets environment variables for you > depending upon your current working directory, or just use some ad-hoc > shell programming to achieve the same (for instance, [2]). Thanks for the hint! I've actually had "git" as a function in my .bashrc for a long time, for implementing a ~/.githistory to help remember the sometimes rather complex git invocations, and keeping track of the context ($cwd, current branch, etc.) they were used in. It should be trivial to hook the environment settings based on $cwd into that. The only problem is that that gives me much less incentive to work on implementing the config support in git, but if I'm the only one with a use case, that's probably just as well. Rasmus
Re: [RFC PATCH] cherry: teach git cherry to respect pathspec
On 16 February 2018 at 02:15, Rasmus Villemoes wrote: > This is a very naive attempt at teaching git cherry to restrict > attention to a given pathspec. Very much RFC, hence no documentation > update or test cases for now. > Ping, any comments on this?
Documentation for update-index
The man page for update-index says -q Quiet. If --refresh finds that the index needs an update, the default behavior is to error out. This option makes git update-index continue anyway. --ignore-submodules Do not try to update submodules. This option is only respected when passed before --refresh. However, it seems that the "This option is only respected when passed before --refresh." also applies to -q (and --unmerged); at least I get different results from git update-index -q --refresh git update-index --refresh -q >From the documentation, that doesn't seem to be intentional, but the code in update-index.c seems to handle -q, --ignore-submodules, --ignore-missing and --unmerged the same way. Rasmus
[PATCH 0/3] a few grep patches
I believe the first two should be ok, but I'm not sure what I myself think of the third one. Perhaps the saving is not worth the complexity, but it does annoy my optimization nerve to see all the unnecessary duplicated work being done. Rasmus Villemoes (3): grep: move grep_source_init outside critical section grep: simplify grep_oid and grep_file grep: avoid one strdup() per file builtin/grep.c | 25 - grep.c | 8 ++-- 2 files changed, 18 insertions(+), 15 deletions(-) -- 2.15.1
[PATCH 2/3] grep: simplify grep_oid and grep_file
In the NO_PTHREADS or !num_threads case, this doesn't change
anything. In the threaded case, note that grep_source_init duplicates
its third argument, so there is no need to keep [path]buf.buf alive
across the call of add_work().
Signed-off-by: Rasmus Villemoes
---
builtin/grep.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 4a4f15172..593f48d59 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -326,18 +326,17 @@ static int grep_oid(struct grep_opt *opt, const struct
object_id *oid,
}
grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
+ strbuf_release(&pathbuf);
#ifndef NO_PTHREADS
if (num_threads) {
add_work(opt, &gs);
- strbuf_release(&pathbuf);
return 0;
} else
#endif
{
int hit;
- strbuf_release(&pathbuf);
hit = grep_source(opt, &gs);
grep_source_clear(&gs);
@@ -356,18 +355,17 @@ static int grep_file(struct grep_opt *opt, const char
*filename)
strbuf_addstr(&buf, filename);
grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
+ strbuf_release(&buf);
#ifndef NO_PTHREADS
if (num_threads) {
add_work(opt, &gs);
- strbuf_release(&buf);
return 0;
} else
#endif
{
int hit;
- strbuf_release(&buf);
hit = grep_source(opt, &gs);
grep_source_clear(&gs);
--
2.15.1
[PATCH 3/3] grep: avoid one strdup() per file
There is only one instance of grep_source_init(GREP_SOURCE_FILE), and in
that case the path and identifier arguments are equal - not just as
strings, but the same pointer is passed. So we can save some time and
memory by reusing the gs->path = xstrdup_or_null(path) we have already
done as gs->identifier, and changing grep_source_clear accordingly
to avoid a double free.
Signed-off-by: Rasmus Villemoes
---
grep.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/grep.c b/grep.c
index 3d7cd0e96..b1532b1b6 100644
--- a/grep.c
+++ b/grep.c
@@ -1972,7 +1972,8 @@ void grep_source_init(struct grep_source *gs, enum
grep_source_type type,
switch (type) {
case GREP_SOURCE_FILE:
- gs->identifier = xstrdup(identifier);
+ gs->identifier = identifier == path ?
+ gs->path : xstrdup(identifier);
break;
case GREP_SOURCE_OID:
gs->identifier = oiddup(identifier);
@@ -1986,7 +1987,10 @@ void grep_source_init(struct grep_source *gs, enum
grep_source_type type,
void grep_source_clear(struct grep_source *gs)
{
FREE_AND_NULL(gs->name);
- FREE_AND_NULL(gs->path);
+ if (gs->path == gs->identifier)
+ gs->path = NULL;
+ else
+ FREE_AND_NULL(gs->path);
FREE_AND_NULL(gs->identifier);
grep_source_clear_data(gs);
}
--
2.15.1
[PATCH 1/3] grep: move grep_source_init outside critical section
grep_source_init typically does three strdup()s, and in the threaded
case, the call from add_work() happens while holding grep_mutex.
We can thus reduce the time we hold grep_mutex by moving the
grep_source_init() call out of add_work(), and simply have add_work()
copy the initialized structure to the available slot in the todo
array.
This also simplifies the prototype of add_work(), since it no longer
needs to duplicate all the parameters of grep_source_init(). In the
callers of add_work(), we get to reduce the amount of code duplicated in
the threaded and non-threaded cases slightly (avoiding repeating the
"GREP_SOURCE_OID, pathbuf.buf, path, oid" argument list); a subsequent
cleanup patch will make that even more so.
Signed-off-by: Rasmus Villemoes
---
builtin/grep.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 3ca4ac80d..4a4f15172 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -92,8 +92,7 @@ static pthread_cond_t cond_result;
static int skip_first_line;
-static void add_work(struct grep_opt *opt, enum grep_source_type type,
-const char *name, const char *path, const void *id)
+static void add_work(struct grep_opt *opt, const struct grep_source *gs)
{
grep_lock();
@@ -101,7 +100,7 @@ static void add_work(struct grep_opt *opt, enum
grep_source_type type,
pthread_cond_wait(&cond_write, &grep_mutex);
}
- grep_source_init(&todo[todo_end].source, type, name, path, id);
+ todo[todo_end].source = *gs;
if (opt->binary != GREP_BINARY_TEXT)
grep_source_load_driver(&todo[todo_end].source);
todo[todo_end].done = 0;
@@ -317,6 +316,7 @@ static int grep_oid(struct grep_opt *opt, const struct
object_id *oid,
const char *path)
{
struct strbuf pathbuf = STRBUF_INIT;
+ struct grep_source gs;
if (opt->relative && opt->prefix_length) {
quote_path_relative(filename + tree_name_len, opt->prefix,
&pathbuf);
@@ -325,18 +325,18 @@ static int grep_oid(struct grep_opt *opt, const struct
object_id *oid,
strbuf_addstr(&pathbuf, filename);
}
+ grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
+
#ifndef NO_PTHREADS
if (num_threads) {
- add_work(opt, GREP_SOURCE_OID, pathbuf.buf, path, oid);
+ add_work(opt, &gs);
strbuf_release(&pathbuf);
return 0;
} else
#endif
{
- struct grep_source gs;
int hit;
- grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
strbuf_release(&pathbuf);
hit = grep_source(opt, &gs);
@@ -348,24 +348,25 @@ static int grep_oid(struct grep_opt *opt, const struct
object_id *oid,
static int grep_file(struct grep_opt *opt, const char *filename)
{
struct strbuf buf = STRBUF_INIT;
+ struct grep_source gs;
if (opt->relative && opt->prefix_length)
quote_path_relative(filename, opt->prefix, &buf);
else
strbuf_addstr(&buf, filename);
+ grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
+
#ifndef NO_PTHREADS
if (num_threads) {
- add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);
+ add_work(opt, &gs);
strbuf_release(&buf);
return 0;
} else
#endif
{
- struct grep_source gs;
int hit;
- grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename,
filename);
strbuf_release(&buf);
hit = grep_source(opt, &gs);
--
2.15.1
[RFC PATCH] cherry: teach git cherry to respect pathspec
This is a very naive attempt at teaching git cherry to restrict
attention to a given pathspec. Very much RFC, hence no documentation
update or test cases for now.
The first problem is the existing signature, with between 0 and 3
arguments. In order to preserve having defaults for limit, head,
upstream, I decided to make use of -- mandatory. The alternative seems
to be to only take a pathspec as arguments 4+, forcing the users to
write out the default values for head and/or limit (and how does one
actually write the default "no limit"?).
At first I just tried parse_pathspec directly to &revs.prune_data, but
that didn't seem to have any effect, and from staring at revision.c for
a while, it seems that one has to go through setup_revisions(). But that
fits well with insisting on the -- above, since we then have an argv[]
that ensures we never hit the handle_revision_arg() or
handle_revision_pseudo_opt() calls.
The motivation for this is that I'm in the process of switching a BSP
from a vendor kernel to one much closer to mainline's master branch. I
do need to apply/port some out-of-tree patches from the vendor kernel,
but it's much more managable if one can do a per-subsystem survey of
what might need porting. So I naively started by doing
git cherry -v linus/master LSDK-17.12-V4.9 v4.9.62 -- drivers/mtd/spi-nor/
assuming that would Just Work. I was somewhat surprised that this didn't
give any output, but digging into the source revealed that (1) there
isn't actually any pathspec handling and (2) any argc other than 1..3 is
treated as 0 - something which is probably also worth fixing.
With this patch, the command above does give something reasonable, and
t3500-cherry.sh also seems to pass.
Signed-off-by: Rasmus Villemoes
---
builtin/log.c | 33 +++--
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 46b4ca13e..2d4534b5c 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1890,6 +1890,8 @@ int cmd_cherry(int argc, const char **argv, const char
*prefix)
const char *head = "HEAD";
const char *limit = NULL;
int verbose = 0, abbrev = 0;
+ int pathspec_argc = 0;
+ int i;
struct option options[] = {
OPT__ABBREV(&abbrev),
@@ -1897,17 +1899,27 @@ int cmd_cherry(int argc, const char **argv, const char
*prefix)
OPT_END()
};
- argc = parse_options(argc, argv, prefix, options, cherry_usage, 0);
+ argc = parse_options(argc, argv, prefix, options, cherry_usage,
+PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
+PARSE_OPT_KEEP_DASHDASH);
+
+ for (i = 1; i < argc; ++i) {
+ if (!strcmp(argv[i], "--")) {
+ pathspec_argc = 1 + argc - i;
+ argc = i;
+ break;
+ }
+ }
switch (argc) {
+ case 4:
+ limit = argv[3];
+ /* FALLTHROUGH */
case 3:
- limit = argv[2];
+ head = argv[2];
/* FALLTHROUGH */
case 2:
- head = argv[1];
- /* FALLTHROUGH */
- case 1:
- upstream = argv[0];
+ upstream = argv[1];
break;
default:
current_branch = branch_get(NULL);
@@ -1921,6 +1933,15 @@ int cmd_cherry(int argc, const char **argv, const char
*prefix)
}
init_revisions(&revs, prefix);
+ if (pathspec_argc) {
+ /*
+* hack: this reuses the element of argv before "--"
+* as argv[0] - setup_revisions assumes argv[0] is
+* present and ignores it, and starts its search for
+* "--" at argv[1].
+*/
+ setup_revisions(pathspec_argc, &argv[argc-1], &revs, NULL);
+ }
revs.max_parents = 1;
if (add_pending_commit(head, &revs, 0))
--
2.15.1
[PATCH v2 0/2] two small grep patches
Changes in v2: - Drop patch 3 with dubious gain/complexity ratio - Add comments regarding ownership of grep_source I was a little torn between copy-pasting the comment or just saying "see above" in the second case. I think a memset would be confusing, at least unless one extends the comment to explain why one then does the memset despite the first half of the comment. Rasmus Villemoes (2): grep: move grep_source_init outside critical section grep: simplify grep_oid and grep_file builtin/grep.c | 33 - 1 file changed, 20 insertions(+), 13 deletions(-) -- 2.15.1
[PATCH v2 2/2] grep: simplify grep_oid and grep_file
In the NO_PTHREADS or !num_threads case, this doesn't change
anything. In the threaded case, note that grep_source_init duplicates
its third argument, so there is no need to keep [path]buf.buf alive
across the call of add_work().
Signed-off-by: Rasmus Villemoes
---
builtin/grep.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index aad422bb6..9a8e4fada 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -326,6 +326,7 @@ static int grep_oid(struct grep_opt *opt, const struct
object_id *oid,
}
grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
+ strbuf_release(&pathbuf);
#ifndef NO_PTHREADS
if (num_threads) {
@@ -334,14 +335,12 @@ static int grep_oid(struct grep_opt *opt, const struct
object_id *oid,
* its fields, so do not call grep_source_clear()
*/
add_work(opt, &gs);
- strbuf_release(&pathbuf);
return 0;
} else
#endif
{
int hit;
- strbuf_release(&pathbuf);
hit = grep_source(opt, &gs);
grep_source_clear(&gs);
@@ -360,6 +359,7 @@ static int grep_file(struct grep_opt *opt, const char
*filename)
strbuf_addstr(&buf, filename);
grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
+ strbuf_release(&buf);
#ifndef NO_PTHREADS
if (num_threads) {
@@ -368,14 +368,12 @@ static int grep_file(struct grep_opt *opt, const char
*filename)
* its fields, so do not call grep_source_clear()
*/
add_work(opt, &gs);
- strbuf_release(&buf);
return 0;
} else
#endif
{
int hit;
- strbuf_release(&buf);
hit = grep_source(opt, &gs);
grep_source_clear(&gs);
--
2.15.1
[PATCH v2 1/2] grep: move grep_source_init outside critical section
grep_source_init typically does three strdup()s, and in the threaded
case, the call from add_work() happens while holding grep_mutex.
We can thus reduce the time we hold grep_mutex by moving the
grep_source_init() call out of add_work(), and simply have add_work()
copy the initialized structure to the available slot in the todo
array.
This also simplifies the prototype of add_work(), since it no longer
needs to duplicate all the parameters of grep_source_init(). In the
callers of add_work(), we get to reduce the amount of code duplicated in
the threaded and non-threaded cases slightly (avoiding repeating the
long "GREP_SOURCE_OID, pathbuf.buf, path, oid" argument list); a
subsequent cleanup patch will make that even more so.
Signed-off-by: Rasmus Villemoes
---
builtin/grep.c | 27 ++-
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 3ca4ac80d..aad422bb6 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -92,8 +92,7 @@ static pthread_cond_t cond_result;
static int skip_first_line;
-static void add_work(struct grep_opt *opt, enum grep_source_type type,
-const char *name, const char *path, const void *id)
+static void add_work(struct grep_opt *opt, const struct grep_source *gs)
{
grep_lock();
@@ -101,7 +100,7 @@ static void add_work(struct grep_opt *opt, enum
grep_source_type type,
pthread_cond_wait(&cond_write, &grep_mutex);
}
- grep_source_init(&todo[todo_end].source, type, name, path, id);
+ todo[todo_end].source = *gs;
if (opt->binary != GREP_BINARY_TEXT)
grep_source_load_driver(&todo[todo_end].source);
todo[todo_end].done = 0;
@@ -317,6 +316,7 @@ static int grep_oid(struct grep_opt *opt, const struct
object_id *oid,
const char *path)
{
struct strbuf pathbuf = STRBUF_INIT;
+ struct grep_source gs;
if (opt->relative && opt->prefix_length) {
quote_path_relative(filename + tree_name_len, opt->prefix,
&pathbuf);
@@ -325,18 +325,22 @@ static int grep_oid(struct grep_opt *opt, const struct
object_id *oid,
strbuf_addstr(&pathbuf, filename);
}
+ grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
+
#ifndef NO_PTHREADS
if (num_threads) {
- add_work(opt, GREP_SOURCE_OID, pathbuf.buf, path, oid);
+ /*
+* add_work() copies gs and thus assumes ownership of
+* its fields, so do not call grep_source_clear()
+*/
+ add_work(opt, &gs);
strbuf_release(&pathbuf);
return 0;
} else
#endif
{
- struct grep_source gs;
int hit;
- grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
strbuf_release(&pathbuf);
hit = grep_source(opt, &gs);
@@ -348,24 +352,29 @@ static int grep_oid(struct grep_opt *opt, const struct
object_id *oid,
static int grep_file(struct grep_opt *opt, const char *filename)
{
struct strbuf buf = STRBUF_INIT;
+ struct grep_source gs;
if (opt->relative && opt->prefix_length)
quote_path_relative(filename, opt->prefix, &buf);
else
strbuf_addstr(&buf, filename);
+ grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
+
#ifndef NO_PTHREADS
if (num_threads) {
- add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);
+ /*
+* add_work() copies gs and thus assumes ownership of
+* its fields, so do not call grep_source_clear()
+*/
+ add_work(opt, &gs);
strbuf_release(&buf);
return 0;
} else
#endif
{
- struct grep_source gs;
int hit;
- grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename,
filename);
strbuf_release(&buf);
hit = grep_source(opt, &gs);
--
2.15.1
[PATCH 1/2] Documentation/config: add sendemail.tocmd to list preceding "See git-send-email(1)"
Signed-off-by: Rasmus Villemoes --- Documentation/config.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 671fcbaa0..d88fc9f63 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -3000,6 +3000,7 @@ sendemail.smtpPass:: sendemail.suppresscc:: sendemail.suppressFrom:: sendemail.to:: +sendemail.tocmd:: sendemail.smtpDomain:: sendemail.smtpServer:: sendemail.smtpServerPort:: -- 2.11.0
[PATCH 2/2] completion: add git config sendemail.tocmd
Signed-off-by: Rasmus Villemoes --- contrib/completion/git-completion.bash | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index fdd984d34..10607cdf2 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -2642,6 +2642,7 @@ _git_config () sendemail.suppressfrom sendemail.thread sendemail.to + sendemail.tocmd sendemail.validate sendemail.smtpbatchsize sendemail.smtprelogindelay -- 2.11.0
[RFC/PATCH 0/2] Introduce safe-include config feature
[trimming Ccs] This is an attempt at implementing the suggested safe-include config feature. It mostly has the semantics Junio suggested in the parent post, but it does not directly extend the current include directive; instead, it uses a separate safe-include directive. This is done so that if a repository is used with both old and new versions of git, the older versions will just silently ignore the safe-include, instead of ignoring include.safe and then proceeding to processing "path = ../project.gitconfig". Config variables are whitelisted using safe-include.whitelist; the value is interpreted as a whitespace-separated list of, possibly negated, patterns. Later patterns override earlier ones. If the feature is deemed worthwhile and my approach is acceptable, I'll go ahead and try to write some documentation. For now, there is just a small test script. Rasmus Villemoes (2): config: Add safe-include directive config: Add test of safe-include feature config.c | 91 +-- t/t1309-config-safe-include.sh | 96 ++ 2 files changed, 184 insertions(+), 3 deletions(-) create mode 100755 t/t1309-config-safe-include.sh -- 2.0.4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 1/2] config: Add safe-include directive
This adds a variant of the include directive, where only certain
config variables in the included files are honoured. The set of
honoured variables consists of those the user has mentioned in a
safe-include.whitelist directive, along with a small set of git.git
blessed ones.
This can, for example, be used by a project to supply a set of
suggested configuration variables, such as "diff.renames = true". The
project would provide these in e.g project.gitconfig, and the user then
has to explicitly opt-in by putting
[safe-include]
path = ../project.gitconfig
into .git/config, possibly preceding the path directive with a
whitelist directive.
The problem with simply using the ordinary include directive for this
purpose is that certain configuration variables (e.g. diff.external)
can allow arbitrary programs to be run.
Older versions of git do not understand the safe-include directives,
so they will effectively just ignore them.
Obviously, we must ignore safe-include.whitelist directives when we
are processing a safe-included file.
Signed-off-by: Rasmus Villemoes
---
config.c | 91 +---
1 file changed, 88 insertions(+), 3 deletions(-)
diff --git a/config.c b/config.c
index a677eb6..764cda1 100644
--- a/config.c
+++ b/config.c
@@ -11,6 +11,7 @@
#include "quote.h"
#include "hashmap.h"
#include "string-list.h"
+#include "wildmatch.h"
struct config_source {
struct config_source *prev;
@@ -39,6 +40,79 @@ static struct config_source *cf;
static int zlib_compression_seen;
+struct safe_var {
+ struct safe_var *next;
+ const char *pattern;
+ int blacklisted;
+};
+
+static int safe_include_depth;
+static struct safe_var *safe_var_head;
+
+static const char *builtin_safe_patterns[] = {
+ "diff.renames",
+};
+
+static int config_name_is_safe(const char *var)
+{
+ struct safe_var *sv;
+ unsigned i;
+
+ for (sv = safe_var_head; sv; sv = sv->next) {
+ /* Handle malformed patterns? */
+ if (wildmatch(sv->pattern, var, WM_CASEFOLD, NULL) == WM_MATCH)
+ return !sv->blacklisted;
+ }
+ for (i = 0; i < ARRAY_SIZE(builtin_safe_patterns); ++i) {
+ if (wildmatch(builtin_safe_patterns[i], var, WM_CASEFOLD, NULL)
== WM_MATCH)
+ return 1;
+ }
+
+ return 0;
+}
+
+static void config_add_safe_pattern(const char *p)
+{
+ struct safe_var *sv;
+ int blacklist = 0;
+
+ if (*p == '!') {
+ blacklist = 1;
+ ++p;
+ }
+ if (!*p)
+ return;
+ sv = xmalloc(sizeof(*sv));
+ sv->pattern = xstrdup(p);
+ sv->blacklisted = blacklist;
+ sv->next = safe_var_head;
+ safe_var_head = sv;
+}
+
+static void config_add_safe_names(const char *value)
+{
+ char *patterns = xstrdup(value);
+ char *p, *save;
+
+ /*
+* This allows giving multiple patterns in a single line, e.g.
+*
+* whitelist = !* foo.bar squirrel.*
+*
+* to override the builtin list of safe vars and only declare
+* foo.bar and the squirrel section safe. But it has the
+* obvious drawback that one cannot match subsection names
+* containing whitespace. The alternative is that the above
+* would have to be written on three separate whitelist lines.
+*/
+ for (p = strtok_r(patterns, " \t", &save); p; p = strtok_r(NULL, " \t",
&save)) {
+ config_add_safe_pattern(p);
+ }
+
+ free(patterns);
+}
+
+
/*
* Default config_set that contains key-value pairs from the usual set of
config
* config files (i.e repo specific .git/config, user wide ~/.gitconfig, XDG
@@ -142,12 +216,23 @@ int git_config_include(const char *var, const char
*value, void *data)
* Pass along all values, including "include" directives; this makes it
* possible to query information on the includes themselves.
*/
- ret = inc->fn(var, value, inc->data);
- if (ret < 0)
- return ret;
+ if (safe_include_depth == 0 || config_name_is_safe(var)) {
+ ret = inc->fn(var, value, inc->data);
+ if (ret < 0)
+ return ret;
+ }
if (!strcmp(var, "include.path"))
ret = handle_path_include(value, inc);
+ else if (safe_include_depth == 0
+&& !strcmp(var, "safe-include.whitelist")) {
+ config_add_safe_names(value);
+ }
+ else if (!strcmp(var, "safe-include.path")) {
+ safe_include_depth++;
+ ret = handle_path_include(value, inc);
+ safe_include_depth--;
+ }
return re
[RFC/PATCH 2/2] config: Add test of safe-include feature
This adds a script for testing various aspects of the safe-include feature. Signed-off-by: Rasmus Villemoes --- t/t1309-config-safe-include.sh | 96 ++ 1 file changed, 96 insertions(+) create mode 100755 t/t1309-config-safe-include.sh diff --git a/t/t1309-config-safe-include.sh b/t/t1309-config-safe-include.sh new file mode 100755 index 000..b8ccc94 --- /dev/null +++ b/t/t1309-config-safe-include.sh @@ -0,0 +1,96 @@ +#!/bin/sh + +test_description='test config file safe-include directives' +. ./test-lib.sh + + +test_expect_success 'blacklist by default' ' + echo "[diff]external = badprog" >project && + echo "[safe-include]path = project" >.gitconfig && + test_must_fail git config diff.external +' + + +test_expect_success 'builtin safe rules' ' + echo "[diff]renames = true" >project && + echo "[safe-include]path = project" >.gitconfig && + echo true >expect && + git config diff.renames >actual && + test_cmp expect actual +' + +test_expect_success 'user blacklist taking precedence' ' + echo "[diff]renames = true" >project && + cat >.gitconfig <<-\EOF && + [diff]renames = false + [safe-include]whitelist = !diff.renames + [safe-include]path = project + EOF + echo false >expect && + git config diff.renames >actual && + test_cmp expect actual +' + +test_expect_success 'wildcard matching' ' + cat >project <<-\EOF && + [test]beer = true + [test]bar = true + [test]foo = true + EOF + cat >.gitconfig <<-\EOF && + [safe-include]whitelist = test.b*r + [safe-include]path = project + EOF + printf "test.bar true\ntest.beer true\n" | sort >expect && + git config --get-regexp "^test" | sort >actual && + test_cmp expect actual +' + +test_expect_success 'ignore whitelist directives in safe-included files' ' + cat >project <<-\EOF && + [safe-include]whitelist = * + [diff]external = badprog + EOF + echo "[safe-include]path = project" >.gitconfig && + test_must_fail git config diff.external +' + +test_expect_success 'multiple whitelist/blacklist patterns in one line' ' + cat >.gitconfig <<-\EOF && + [safe-include]whitelist = !* foo.bar squirrel.* !squirrel.xyz + [safe-include]path = project + EOF + cat >project <<-\EOF && + [diff]renames = true + [foo]bar = bar + [squirrel]abc = abc + [squirrel]xyz = xyz + EOF + test_must_fail git config diff.renames && + test_must_fail git config squirrel.xyz && + echo bar >expect && + git config foo.bar >actual && + test_cmp expect actual + echo abc >expect && + git config squirrel.abc >actual && + test_cmp expect actual +' + +test_expect_success 'case insensitivity' ' + cat >.gitconfig <<-\EOF && + [safe-include]whitelist = Test.Abc test.xyz + [safe-include]path = project + EOF + cat >project <<-\EOF && + [test]abc = abc + [TeST]XyZ = XyZ + EOF + echo abc >expect && + git config test.abc >actual && + test_cmp expect actual && + echo XyZ >expect && + git config test.xyz >actual && + test_cmp expect actual +' + +test_done -- 2.0.4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 1/2] config: Add safe-include directive
Junio C Hamano wrote: > (by the way, we do not do dashes in names for configuration by > convention) OK. Actually, I now think I'd prefer a subsection [include "safe"], but I don't have any strong preferences regarding the names. > That syntax _could_ be just a relative path (e.g. project.gitconfig names > the file with that name at the top-level of the working tree), and if we are > to do so, we should forbid any relative path that escapes from the working > tree (e.g. ../project.gitconfig is forbidden, but down/down/../../.gitconfig > could be OK as it is the same as .gitconfig). For that matter, anything with > /./ and /../ in it can safely be forbidden without losing functionality. I agree that it would be most useful to interpret relative paths as being relative to the working tree. I'm not sure what would be gained by checking for ./ and ../ components, a symlink could easily be used to circumvent that. > And we can allow absolute path, e.g. /etc/gitconfig, of course, but I'd > prefer to at least initially forbid an absolute path, due to the same worries > I have against the "unset some variables defined in /etc/gitconfig" topic > we discussed earlier today in a separate thread. One might (ab)use the feature to only use some settings from a global file, e.g. [include "safe"] whitelist = !foo.* path = ~/extra.gitconfig But I'm fine with forbidding absolute paths until someone actually comes with such a use case. > Another design decision we would need to make is if it should be > allowed for a safe-included file to use safe-include directive to > include other files. Offhand I do not think of a reason we absolutely > need to support it, but there may be an interesting workflow enabled > if we did so. I dunno. After one level of safe-include, any safe-include can also be done as a normal include (but one may need to spell the path differently if the two included files are not both at the top of the working tree). One could imagine a project supplying lots of defaults and splitting those into separate files, each included from a single project.gitconfig. Anyway, my proposal allows nesting includes and safe-includes inside safe-includes; forbidding it would just be a matter of adding a safe_include_depth == 0 check in two places. (Then safe_include_depth probably could/should be renamed in_safe_include.) I think I have a slight preference to allowing nested includes, but if absolute paths are forbidden for safe-includes, they should also be forbidden for include-inside-safe-include. Rasmus -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] strbuf_getwholeline: use getc_unlocked
On Sun, Apr 05 2015, Jeff King wrote:
> which is back to the v2.0.0 number. Even with the extra strlen, it seems
> that what fgets does internally beats repeated getc calls. Which I guess
> is not too surprising, as each getc() will have to check for underflow
> in the buffer. Perhaps there is more room to micro-optimize
> strbuf_getwholeline, but I kind of doubt it.
>
> The big downside is that our input strings are no longer NUL-clean
> (reading "foo\0bar\n" would yield just "foo". I doubt that matters in
> the real world, but it does fail a few of the tests (e.g., t7008 tries
> to read a list of patterns which includes NUL, and we silently truncate
> the pattern rather than read in the NUL and barf).
>
> So we'd have to either:
>
> 1. Decide that doesn't matter.
>
> 2. Have callers specify a "damn the NULs, I want it fast" flag.
>
> 3. Find some alternative that is more robust than fgets, and faster
> than getc. I don't think there is anything in stdio, but I am not
> above dropping in a faster non-portable call if it is available,
> and then falling back to the current code otherwise.
getdelim is in POSIX 2008
(http://pubs.opengroup.org/stage7tc1/functions/getdelim.html), so should
be available on any half-{d,r}ecent platform. It obviously has the
advantage of having access to the internal stdio buffer, and by
definition handles embedded NULs. No idea if using such modern
interfaces in git is ok, though.
Implementation-wise, I think strbuf_getwholeline could be implemented
mostly as a simple wrapper for getdelim. If I'm reading the current code
and the posix spec for getdelim correctly, something like this should do
it (though obviously not meant to be included as-is):
diff --git a/strbuf.c b/strbuf.c
index 0346e74..d3338b9 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -434,6 +434,32 @@ int strbuf_getcwd(struct strbuf *sb)
return -1;
}
+#if USE_GETDELIM
+/* Hacky declaration to avoid messing with feature test macros. */
+ssize_t getdelim(char **, size_t *, int, FILE *);
+int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
+{
+ ssize_t r;
+
+ if (feof(fp))
+ return EOF;
+
+ strbuf_reset(sb);
+ if (!sb->alloc)
+ sb->buf = NULL;
+
+ r = getdelim(&sb->buf, &sb->alloc, term, fp);
+
+ if (r > 0) {
+ sb->len = r;
+ return 0;
+ }
+ assert(r == -1);
+ if (!sb->buf)
+ strbuf_init(sb);
+ return EOF;
+}
+#else
int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
{
int ch;
@@ -454,6 +480,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int
term)
sb->buf[sb->len] = '\0';
return 0;
}
+#endif
int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
{
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] strbuf_getwholeline: use getc_unlocked
On Tue, Apr 07 2015, Jeff King wrote: > On Tue, Apr 07, 2015 at 03:48:33PM +0200, Rasmus Villemoes wrote: > >> Implementation-wise, I think strbuf_getwholeline could be implemented >> mostly as a simple wrapper for getdelim. If I'm reading the current code >> and the posix spec for getdelim correctly, something like this should do >> it (though obviously not meant to be included as-is): > > I think it's close to what we want. strbuf_grow calls xrealloc, which > will call try_to_free_routine() and possibly die() for us. So we would > probably want to check errno on failure and respond similarly if we get > ENOMEM. Hm, I'm afraid it's not that simple. It seems that data may be lost from the stream if getdelim encounters ENOMEM: Looking at the glibc implementation (libio/iogetdelim.c), if reallocating the user buffer fails, -1 is returned (presumably with errno==ENOMEM set by realloc), and there's no way of knowing how many bytes were already copied to the buffer (cur_len). For regular files, I suppose one could do a ftell/fseek dance. For other cases, I don't see a reliable way to retry upon ENOMEM. Related thread on the posix mailing list: http://thread.gmane.org/gmane.comp.standards.posix.austin.general/10091 > I wonder if it is even worth doing the getc_unlocked dance at all. It > would potentially speed up the fallback code, but my hope that would be > that most systems would simply use the getdelim() version. Well, it's not really an intrusive patch, and 42% speedup is rather significant. And, of course, the above ENOMEM issue may mean that getdelim isn't usable after all. Rasmus -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-send-email: kill $prompting variable
The variable $prompting is weird. It is only read in one place (when
deciding whether to prompt for a Message-ID to use in In-Reply-To),
and it will be false unless we've taken the completely unrelated
branch filling in @initial_to.
Prompting should be done if the info is needed, not if some unrelated
item had to be prompted for. So kill $prompting.
Signed-off-by: Rasmus Villemoes
---
git-send-email.perl | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 2162478..f608d9b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -755,13 +755,11 @@ if (!defined $sender) {
# But it's a no-op to run sanitize_address on an already sanitized address.
$sender = sanitize_address($sender);
-my $prompting = 0;
if (!@initial_to && !defined $to_cmd) {
my $to = ask("Who should the emails be sent to (if any)? ",
default => "",
valid_re => qr/\@.*\./, confirm_only => 1);
push @initial_to, parse_address_line($to) if defined $to; #
sanitized/validated later
- $prompting++;
}
sub expand_aliases {
@@ -785,7 +783,7 @@ sub expand_one_alias {
@bcclist = expand_aliases(@bcclist);
@bcclist = validate_address_list(sanitize_address_list(@bcclist));
-if ($thread && !defined $initial_reply_to && $prompting) {
+if ($thread && !defined $initial_reply_to) {
$initial_reply_to = ask(
"Message-ID to be used as In-Reply-To for the first email (if
any)? ",
default => "",
--
1.8.4.rc3.1.g30eccb6
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] git-send-email: Cache generated message-ids, use them when prompting
This is mostly a proof of concept/RFC patch. The idea is for
git-send-email to store the message-ids it generates, along with the
Subject and Date headers of the message. When prompting for which
Message-ID should be used in In-Reply-To, display a list of recent
emails (identifed using the Date/Subject pairs; the message-ids
themselves are not for human consumption). Choosing from that list
will then use the corresponding message-id; otherwise, the behaviour
is as usual.
When composing v2 or v3 of a patch or patch series, this avoids the
need to get one's MUA to display the Message-ID of the earlier email
(which is cumbersome in some MUAs) and then copy-paste that.
If this idea is accepted, I'm certain I'll get to use the feature
immediately, since the patch is not quite ready for inclusion :-)
Signed-off-by: Rasmus Villemoes
---
git-send-email.perl | 101 +---
1 file changed, 96 insertions(+), 5 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index f608d9b..2e3685c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -26,6 +26,7 @@ use Data::Dumper;
use Term::ANSIColor;
use File::Temp qw/ tempdir tempfile /;
use File::Spec::Functions qw(catfile);
+use Date::Parse;
use Error qw(:try);
use Git;
@@ -203,6 +204,7 @@ my ($validate, $confirm);
my (@suppress_cc);
my ($auto_8bit_encoding);
my ($compose_encoding);
+my ($msgid_cache_file, $msgid_maxprompt);
my ($debug_net_smtp) = 0; # Net::SMTP, see send_message()
@@ -214,7 +216,7 @@ my %config_bool_settings = (
"signedoffcc" => [\$signed_off_by_cc, undef], # Deprecated
"validate" => [\$validate, 1],
"multiedit" => [\$multiedit, undef],
-"annotate" => [\$annotate, undef]
+"annotate" => [\$annotate, undef],
);
my %config_settings = (
@@ -237,6 +239,7 @@ my %config_settings = (
"from" => \$sender,
"assume8bitencoding" => \$auto_8bit_encoding,
"composeencoding" => \$compose_encoding,
+"msgidcachefile" => \$msgid_cache_file,
);
my %config_path_settings = (
@@ -311,6 +314,7 @@ my $rc = GetOptions("h" => \$help,
"8bit-encoding=s" => \$auto_8bit_encoding,
"compose-encoding=s" => \$compose_encoding,
"force" => \$force,
+ "msgid-cache-file=s" => \$msgid_cache_file,
);
usage() if $help;
@@ -784,10 +788,31 @@ sub expand_one_alias {
@bcclist = validate_address_list(sanitize_address_list(@bcclist));
if ($thread && !defined $initial_reply_to) {
- $initial_reply_to = ask(
- "Message-ID to be used as In-Reply-To for the first email (if
any)? ",
- default => "",
- valid_re => qr/\@.*\./, confirm_only => 1);
+ my @choices;
+ if ($msgid_cache_file) {
+ @choices = msgid_cache_getmatches();
+ }
+ if (@choices) {
+ my $prompt = '';
+ my $i = 0;
+ $prompt .= sprintf "(%d) [%s] %s\n", $i++, $_->{date},
$_->{subject}
+ for (@choices);
+ $prompt .= sprintf "Answer 0-%d to use the Message-ID of one of
the above\n", $#choices;
+ $prompt .= "Message-ID to be used as In-Reply-To for the first
email (if any)? ";
+ $initial_reply_to =
+ ask($prompt,
+ default => "",
+ valid_re => qr/\@.*\.|^[0-9]+$/, confirm_only => 1);
+ if ($initial_reply_to =~ /^[0-9]+$/ && $initial_reply_to <
@choices) {
+ $initial_reply_to = $choices[$initial_reply_to]{id};
+ }
+ }
+ else {
+ $initial_reply_to =
+ ask("Message-ID to be used as In-Reply-To for the first
email (if any)? ",
+ default => "",
+ valid_re => qr/\@.*\./, confirm_only => 1);
+ }
}
if (defined $initial_reply_to) {
$initial_reply_to =~ s/^\s*quit if $smtp;
+msgid_cache_write() if $msgid_cache_file;
+
sub unique_email_list {
my %seen;
my @emails;
@@ -1556,3 +1585,65 @@ sub body_or_subject_has_nonascii {
}
return 0;
}
+
+my @msgid_new_entries;
+
+# For now, use a simple tab-separated format:
+#
+#$id\t$date\t$subject\n
+sub msgid_cache_read {
+ my $fh;
+ my $line;
+ my @entries;
+ if (not open ($fh, '<', $msgid_cache_file)) {
+ # A non-existing cache file is ok, but should we warn if errno
!= ENOENT?
+ return ();
+ }
+ while ($line = <$fh&
[PATCH v2 0/2] git-send-email: Message-ID caching
This is a more polished attempt at implementing the message-id
caching. I apologize for the sloppiness (unrelated changes, unused
variables etc.) in the first version.
I have split the patch in two. The first half moves the choice-logic
inside ask(): If ask() decides to return the default immediately
(because the $term->{IN,OUT} checks fail), there's no reason to print
all the choices. In my first attempt, I handled this by passing a huge
prompt-string, but that made everything underlined (the prompt may be
emphasized in some other way on other terminals). Passing a different
valid_re and handling a numeric return is also slightly more
cumbersome for the caller than making ask() take care of it. There
might be other future uses of this 'choices' ability, but if the
message-id-caching is ultimately rejected, [1/2] can of course also be
dropped.
[2/2] is the new implementation. The two main changes are that old
entries are expunged when the file grows larger than, by default, 100
kB, and the old emails are scored according to a simple scheme (which
might need improvement). The input to the scoring is {first subject in
new batch, old subject, was the old email first in a batch?}. [*]
I also now simply store the unix time stamp instead of storing the
contents of the date header. This reduces processing time (no need to
parse the date header when reading the file), and eliminates the
Date::Parse dependency. The minor downside is that if the user has
changed time zone since the old email was sent, the date in the prompt
will not entirely match the Date:-header in the email that was sent.
I had to rearrange the existing code a little to make certain global
variables ($time, @files) contain the right thing at the right time,
but hopefully I haven't broken anything in the process.
[*] Suggestions for improving that heuristic are welcome. One thing
that might be worthwhile is to give words ending in a colon higher
weight.
Rasmus Villemoes (2):
git-send-email: add optional 'choices' parameter to the ask sub
git-send-email: Cache generated message-ids, use them when prompting
git-send-email.perl | 144 ---
1 file changed, 138 insertions(+), 6 deletions(-)
Thanks, Junio, for the comments. A few replies:
Junio C Hamano writes:
> Rasmus Villemoes writes:
>> my %config_path_settings = (
>> @@ -311,6 +314,7 @@ my $rc = GetOptions("h" => \$help,
>> "8bit-encoding=s" => \$auto_8bit_encoding,
>> "compose-encoding=s" => \$compose_encoding,
>> "force" => \$force,
>> +"msgid-cache-file=s" => \$msgid_cache_file,
>> );
>
> Is there a standard, recommended location we suggest users to store
> this?
I don't know. It is obviously a local, per-repository, thing. I don't
know enough about git's internals to know if something breaks if one
puts it in .git (say, ".git/msgid.cache"). If that's a bad idea, then
I think it should be in the root of the repository, and one would then
probably want to add it to .git/info/exclude.
If storing it under .git is possible, one could consider making the
option a boolean ('msgid-use-cache' ?) and always use
".git/msgid.cache".
>> +my @choices;
>> +if ($msgid_cache_file) {
>> +@choices = msgid_cache_getmatches();
>
> It is a bit strange that "filename is specified => we will find the
> latest 10" before seeing if we even check to see if that file
> exists. I would have expected that a two-step "filename is given =>
> try to read it" and "if we did read something => give choices"
> process would be used.
I'm not sure I understand how this is different from what I was or am
doing. Sure, I don't do the test for existence before calling
msgid_cache_getmatches(), but that will just return an empty list if
the file does not exist. That will end up doing the same thing as if
no cache file was given.
> Also "getmatches" that does not take any clue from what the caller
> knows (the title of the series, for example) seems much less useful
> than ideal.
Fixed in the new version.
>> +msgid_cache_this($message_id, $subject, $date) if ($msgid_cache_file &&
>> !$dry_run);
>
> Is this caching each and every one, even for things like "[PATCH 23/47]"?
Yes, but now I remember whether it was the first or not.
>> +msgid_cache_write() if $msgid_cache_file;
>
> Is this done under --dry-run?
Well, it was, but the msgid_cache_this() was not, so there was an
empty list of new entries. Of course, better to be explicit and safe,
so fixed.
>> +if (not open ($fh, '<', $msgid_cache_f
[PATCH 1/2] git-send-email: add optional 'choices' parameter to the ask sub
Make it possible for callers of ask() to provide a list of
choices. Entering an appropriate integer chooses from that list,
otherwise the input is treated as usual.
Each choice can either be a single string, which is used both for the
prompt and for the return value, or a two-element array ref, where the
zeroth element is the choice and the first is used for the prompt.
Signed-off-by: Rasmus Villemoes
---
git-send-email.perl | 11 +++
1 file changed, 11 insertions(+)
diff --git a/git-send-email.perl b/git-send-email.perl
index 2162478..ac3b02d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -680,11 +680,18 @@ sub ask {
my $valid_re = $arg{valid_re};
my $default = $arg{default};
my $confirm_only = $arg{confirm_only};
+ my $choices = $arg{choices} || [];
my $resp;
my $i = 0;
return defined $default ? $default : undef
unless defined $term->IN and defined fileno($term->IN) and
defined $term->OUT and defined fileno($term->OUT);
+ for (@$choices) {
+ printf "(%d) %s\n", $i++, ref($_) eq 'ARRAY' ? $_->[1] : $_;
+ }
+ printf "Enter 0-%d to choose from the above list\n", $i-1
+ if (@$choices);
+ $i = 0;
while ($i++ < 10) {
$resp = $term->readline($prompt);
if (!defined $resp) { # EOF
@@ -694,6 +701,10 @@ sub ask {
if ($resp eq '' and defined $default) {
return $default;
}
+ if (@$choices && $resp =~ m/^[0-9]+$/ && $resp < @$choices) {
+ my $c = $choices->[$resp];
+ return ref($c) eq 'ARRAY' ? $c->[0] : $c;
+ }
if (!defined $valid_re or $resp =~ /$valid_re/) {
return $resp;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] git-send-email: Cache generated message-ids, use them when prompting
Allow the user to specify a file (sendemail.msgidcachefile) in which
to store the message-ids generated by git-send-email, along with time
and subject information. When prompting for a Message-ID to be used in
In-Reply-To, that file can be used to generate a list of options.
When composing v2 or v3 of a patch or patch series, this avoids the
need to get one's MUA to display the Message-ID of the earlier email
(which is cumbersome in some MUAs) and then copy-paste that.
Listing all previously sent emails is useless, so currently only the
10 most "relevant" emails. "Relevant" is based on a simple scoring,
which might need to be revised: Count the words in the old subject
which also appear in the subject of the first email to be sent; add a
bonus if the old email was first in a batch (that is, [00/74] is more
likely to be relevant than [43/74]). Resort to comparing timestamps
(newer is more relevant) when the scores tie.
To limit disk usage, the oldest half of the cached entries are
expunged when the cache file exceeds sendemail.msgidcachemaxsize
(default 100kB). This also ensures that we will never have to read,
score, and sort 1000s of entries on each invocation of git-send-email.
Signed-off-by: Rasmus Villemoes
---
git-send-email.perl | 133 ---
1 file changed, 127 insertions(+), 6 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index ac3b02d..5094267 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -203,6 +203,7 @@ my ($validate, $confirm);
my (@suppress_cc);
my ($auto_8bit_encoding);
my ($compose_encoding);
+my ($msgid_cache_file, $msgid_cache_maxsize);
my ($debug_net_smtp) = 0; # Net::SMTP, see send_message()
@@ -237,6 +238,8 @@ my %config_settings = (
"from" => \$sender,
"assume8bitencoding" => \$auto_8bit_encoding,
"composeencoding" => \$compose_encoding,
+"msgidcachefile" => \$msgid_cache_file,
+"msgidcachemaxsize" => \$msgid_cache_maxsize,
);
my %config_path_settings = (
@@ -796,11 +799,23 @@ sub expand_one_alias {
@bcclist = expand_aliases(@bcclist);
@bcclist = validate_address_list(sanitize_address_list(@bcclist));
+if ($compose && $compose > 0) {
+ @files = ($compose_filename . ".final", @files);
+}
+
if ($thread && !defined $initial_reply_to && $prompting) {
+ my @choices = ();
+ if ($msgid_cache_file) {
+ my $first_subject = get_patch_subject($files[0]);
+ $first_subject =~ s/^GIT: //;
+ @choices = msgid_cache_getmatches($first_subject, 10);
+ @choices = map {[$_->{id}, sprintf "[%s] %s",
format_2822_time($_->{epoch}), $_->{subject}]} @choices;
+ }
$initial_reply_to = ask(
"Message-ID to be used as In-Reply-To for the first email (if
any)? ",
default => "",
- valid_re => qr/\@.*\./, confirm_only => 1);
+ valid_re => qr/\@.*\./, confirm_only => 1,
+ choices => \@choices);
}
if (defined $initial_reply_to) {
$initial_reply_to =~ s/^\s* 0) {
- @files = ($compose_filename . ".final", @files);
-}
-
# Variables we set as part of the loop over files
our ($message_id, %mail, $subject, $reply_to, $references, $message,
$needs_confirm, $message_num, $ask_default);
@@ -1136,7 +1147,7 @@ sub send_message {
my $to = join (",\n\t", @recipients);
@recipients = unique_email_list(@recipients,@cc,@bcclist);
@recipients = (map { extract_valid_address_or_die($_) } @recipients);
- my $date = format_2822_time($time++);
+ my $date = format_2822_time($time);
my $gitversion = '@@GIT_VERSION@@';
if ($gitversion =~ m/..GIT_VERSION../) {
$gitversion = Git::version();
@@ -1477,6 +1488,11 @@ foreach my $t (@files) {
my $message_was_sent = send_message();
+ if ($message_was_sent && $msgid_cache_file && !$dry_run) {
+ msgid_cache_this($message_id, $message_num == 1 ? 1 : 0, ,
$time, $subject);
+ }
+ $time++;
+
# set up for the next message
if ($thread && $message_was_sent &&
($chain_reply_to || !defined $reply_to || length($reply_to) ==
0 ||
@@ -1521,6 +1537,8 @@ sub cleanup_compose_files {
$smtp->quit if $smtp;
+msgid_cache_write() if $msgid_cache_file && !$dry_run;
+
sub unique_email_list {
my %seen;
my @emails;
@@ -1569,3 +1587,106 @@ sub body_or_subject_has_nonascii {
}
return 0;
}
+
+my @msgid_new_entries;
+sub msgid_cache_this {
+ my $msgid = shift;
+ my $first = shift;
+ my $epoch = shift;
+ my $subject = shift;
+ #
