* Re: [PATCH] clean up confusing suggestion for commit references
From: Heiko Voigt @ 2016-10-11 16:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqoa2sdnjt.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 10, 2016 at 12:14:14PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Mon, Oct 10, 2016 at 11:24:01AM -0700, Junio C Hamano wrote:
> >
> >> I no longer have preference either way myself, even though I was in
> >> favor of no-quotes simply because I had an alias to produce that
> >> format and was used to it.
> >
> > I'll admit that I don't care _that_ much and am happy to leave it up to
> > individual authors, as long as nobody quotes SubmittingPatches at me as
> > some kind of gospel when I use the no-quotes form.
>
> ;-).
>
> I just do not want to hear "gitk (or was it git-gui) produces quoted
> form, why are you recommending no-quoted form in SubmittingPatches?"
>
> I'd say "use common sense; sometimes it is less confusing to read
> without quotes and it is perfectly OK to do so if that is the case".
I do not care about which format it should be either. I just wanted to
be clear about whatever should be used. Since it seems we will allow
both, I am also fine with leaving the description as it is ;-)
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Junio C Hamano @ 2016-10-11 16:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610111212090.35196@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> I am personally fine with this line; two things come to mind:
>>
>> - This would work just fine as-is with Linus's change to turn
>> DEFAULT_ABBREV to -1.
>>
>> - It appears that it is more fashionable to use
>> strbuf_add_unique_abbrev() these days.
>
> Right, I actually looked at this place when I tried to decide where I
> could use that function. Somehow I thought I'd not break up the flow here.
>
> But since you asked so nicely,...
When I say "I am fine", I am not asking you to change anything.
Some of the places that have been updated recently to use
strbuf_add_unique_abbrev() in other topics did improve the
readability of the code, but I do not think it would universally
be true.
^ permalink raw reply
* [PATCH] gitk: Fix Japanese translation for "marked commit"
From: Satoshi Yasushima @ 2016-10-11 16:36 UTC (permalink / raw)
To: git; +Cc: Satothi Yasushima
Signed-off-by: Satoshi Yasushima <s.yasushima@gmail.com>
---
po/ja.po | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/po/ja.po b/po/ja.po
index f143753..510306b 100644
--- a/po/ja.po
+++ b/po/ja.po
@@ -2,16 +2,17 @@
# Copyright (C) 2005-2015 Paul Mackerras
# This file is distributed under the same license as the gitk package.
#
-# YOKOTA Hiroshi <yokota@netlab.cs.tsukuba.ac.jp>, 2015.
# Mizar <mizar.jp@gmail.com>, 2009.
# Junio C Hamano <gitster@pobox.com>, 2009.
+# YOKOTA Hiroshi <yokota@netlab.cs.tsukuba.ac.jp>, 2015.
+# Satoshi Yasushima <s.yasushima@gmail.com>, 2016.
msgid ""
msgstr ""
"Project-Id-Version: gitk\n"
"Report-Msgid-Bugs-To: \n"
"POT-Creation-Date: 2015-05-17 14:32+1000\n"
"PO-Revision-Date: 2015-11-12 13:00+0900\n"
-"Last-Translator: YOKOTA Hiroshi <yokota@netlab.cs.tsukuba.ac.jp>\n"
+"Last-Translator: Satoshi Yasushima <s.yasushima@gmail.com>\n"
"Language-Team: Japanese\n"
"Language: ja\n"
"MIME-Version: 1.0\n"
@@ -314,11 +315,11 @@ msgstr "マークを付けたコミットと比較する"
#: gitk:2630 gitk:2641
msgid "Diff this -> marked commit"
-msgstr "これと選択したコミットのdiffを見る"
+msgstr "これとマークを付けたコミットのdiffを見る"
#: gitk:2631 gitk:2642
msgid "Diff marked commit -> this"
-msgstr "選択したコミットとこれのdiffを見る"
+msgstr "マークを付けたコミットとこれのdiffを見る"
#: gitk:2632
msgid "Revert this commit"
--
2.10.0.windows.1
^ permalink raw reply related
* Re: [PATCH v4 4/4] mergetool: honor -O<orderfile>
From: Junio C Hamano @ 2016-10-11 17:05 UTC (permalink / raw)
To: David Aguilar; +Cc: Git Mailing List, Johannes Sixt, Luis Gutierrez
In-Reply-To: <20161011050623.GA14278@gmail.com>
David Aguilar <davvid@gmail.com> writes:
>> I only see 4/4 in v4; am I correct to assume that 1-3/4 of v4 are
>> the same as their counterparts in v3?
>
> Yes, 1-3 are unchanged since v3.
> Thanks for checking,
I'll queue these four with Reviewed-by's from j6t.
Thanks, both.
^ permalink raw reply
* Re: [PATCH 11/28] attr: (re)introduce git_check_attr() and struct git_attr_check
From: Brandon Williams @ 2016-10-11 16:59 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, pclouds
In-Reply-To: <20161011002115.23312-12-sbeller@google.com>
On 10/10, Stefan Beller wrote:
> From: Junio C Hamano <gitster@pobox.com>
>
> A common pattern to check N attributes for many paths is to
>
> (1) prepare an array A of N git_attr_check_elem items;
> (2) call git_attr() to intern the N attribute names and fill A;
Does the word 'intern' here mean internalize? It took me a few reads to
stop picturing college students running around an office :)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Junio C Hamano @ 2016-10-11 16:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610111238470.35196@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> In the end, I decided to actually *not* use strbuf_add_unique_abbrev()
> here because it really makes the code very much too ugly after the
> introduction of short_commit_name():
It's perfectly fine not to use that function when it does not make
sense; we shouldn't use it (or anything for that matter) just for
the sake of using it.
^ permalink raw reply
* Re: [PATCH 26/28] attr: make git_attr_counted static
From: Brandon Williams @ 2016-10-11 17:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, pclouds
In-Reply-To: <20161011002115.23312-27-sbeller@google.com>
On 10/10, Stefan Beller wrote:
> It's not used outside the attr code, so let's keep it private.
>
> Change-Id: I0d15e0f2ea944b31d68b9cf1a4edecac0ca2170d
Looks like you forgot to remove this :)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 11/28] attr: (re)introduce git_check_attr() and struct git_attr_check
From: Junio C Hamano @ 2016-10-11 17:42 UTC (permalink / raw)
To: Brandon Williams; +Cc: Stefan Beller, git, pclouds
In-Reply-To: <20161011165936.GA9085@google.com>
Brandon Williams <bmwill@google.com> writes:
> On 10/10, Stefan Beller wrote:
>> From: Junio C Hamano <gitster@pobox.com>
>>
>> A common pattern to check N attributes for many paths is to
>>
>> (1) prepare an array A of N git_attr_check_elem items;
>> (2) call git_attr() to intern the N attribute names and fill A;
>
> Does the word 'intern' here mean internalize? It took me a few reads to
> stop picturing college students running around an office :)
The verb comes from Lisp world where you "intern" a string to make a
symbol.
^ permalink raw reply
* Re: [PATCH 28/28] attr: convert to new threadsafe API
From: Brandon Williams @ 2016-10-11 17:45 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, pclouds
In-Reply-To: <20161011002115.23312-29-sbeller@google.com>
On 10/10, Stefan Beller wrote:
> be initalized once as that part will be read only after its
initialized
> initialisation, the answer may be different for each path.
should this be the US spelling 'initialization'?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v4 0/7] Add --format to tag verification
From: Santiago Torres @ 2016-10-11 17:43 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters
In-Reply-To: <20161007210721.20437-1-santiago@nyu.edu>
[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]
Hi,
I noticed there were no replies for this thread. I was curious if it got
buried because I sent it on the Friday evening before a long weekend.
I don't mean to pressure or anything.
Thanks!
-Santiago.
On Fri, Oct 07, 2016 at 05:07:14PM -0400, santiago@nyu.edu wrote:
> From: Santiago Torres <santiago@nyu.edu>
>
> This is the fourth iteration of the series in [1][2][3], which comes as a
> result of the discussion in [4]. The main goal of this patch series is to bring
> --format to git tag verification so that upper-layer tools can inspect the
> content of a tag and make decisions based on those contents.
>
> In this re-woll we:
>
> * Fixed the author fields and signed off by's throughout the patch
> series
>
> * Added two more patches with unit tests to ensure the format specifier
> behaves as expected
>
> * Fixed a missing initialization for the format specifier in verify-tag which
> caused a crash.
>
> * Fixed an outdated git commit message that had the previous name of a
> function definition.
>
> Thanks,
> -Santiago
>
> [1] http://public-inbox.org/git/20160930221806.3398-1-santiago@nyu.edu/
> [2] http://public-inbox.org/git/20160922185317.349-1-santiago@nyu.edu/
> [3] http://public-inbox.org/git/20160926224233.32702-1-santiago@nyu.edu/
> [4] http://public-inbox.org/git/20160607195608.16643-1-santiago@nyu.edu/
>
>
> Lukas Puehringer (4):
> tag: add format specifier to gpg_verify_tag
> gpg-interface, tag: add GPG_VERIFY_QUIET flag
> ref-filter: add function to print single ref_array_item
> builtin/tag: add --format argument for tag -v
>
> Santiago Torres (3):
> builtin/verify-tag: add --format to verify-tag
> t/t7030-verify-tag: Add --format specifier tests
> t/t7004-tag: Add --format specifier tests
>
> Documentation/git-tag.txt | 2 +-
> Documentation/git-verify-tag.txt | 2 +-
> builtin/tag.c | 34 +++++++++++++++++++++++-----------
> builtin/verify-tag.c | 13 +++++++++++--
> gpg-interface.h | 1 +
> ref-filter.c | 10 ++++++++++
> ref-filter.h | 3 +++
> t/t7004-tag.sh | 16 ++++++++++++++++
> t/t7030-verify-tag.sh | 16 ++++++++++++++++
> tag.c | 22 +++++++++++++++-------
> tag.h | 4 ++--
> 11 files changed, 99 insertions(+), 24 deletions(-)
>
> --
> 2.10.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] reset: support the --stdin option
From: Junio C Hamano @ 2016-10-11 17:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <2c7a52e43be710c7f37c4886629bda38df183c21.1476202100.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> + if (read_from_stdin) {
> + strbuf_getline_fn getline_fn = nul_term_line ?
> + strbuf_getline_nul : strbuf_getline_lf;
> + int flags = PATHSPEC_PREFER_FULL |
> + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP;
> + struct strbuf buf = STRBUF_INIT;
> + struct strbuf unquoted = STRBUF_INIT;
> +
> + if (patch_mode)
> + die(_("--stdin is incompatible with --patch"));
> +
> + if (pathspec.nr)
> + die(_("--stdin is incompatible with path arguments"));
> +
> + if (patch_mode)
> + flags |= PATHSPEC_PREFIX_ORIGIN;
Didn't we already die above under that mode?
> + while (getline_fn(&buf, stdin) != EOF) {
> + if (!nul_term_line && buf.buf[0] == '"') {
> + strbuf_reset(&unquoted);
> + if (unquote_c_style(&unquoted, buf.buf, NULL))
> + die(_("line is badly quoted"));
> + strbuf_swap(&buf, &unquoted);
> + }
> + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> + stdin_paths[stdin_nr++] = xstrdup(buf.buf);
> + strbuf_reset(&buf);
> + }
> + strbuf_release(&unquoted);
> + strbuf_release(&buf);
> +
> + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> + stdin_paths[stdin_nr++] = NULL;
It makes sense to collect, but...
> + parse_pathspec(&pathspec, 0, flags, prefix,
> + (const char **)stdin_paths);
...letting them be used as if they are pathspec is wrong when
stdin_paths[] contain wildcard, isn't it?
I think flags |= PATHSPEC_LITERAL_PATH can help fixing it. 0/2 said
this mimicks checkout-index and I think it should by not treating
the input as wildcarded patterns (i.e. "echo '*.c' | reset --stdin"
shouldn't be the way to reset all .c files --- that's something we
would want to add to the test, I guess).
Thanks.
^ permalink raw reply
* Re: [PATCH 28/28] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-11 17:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <xmqqmviaaina.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 11, 2016 at 10:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> This revamps the API of the attr subsystem to be thread safe.
>> Before we had the question and its results in one struct type.
>> The typical usage of the API was
>>
>> static struct git_attr_check;
>
> I think you meant "*check" at the end, perhaps?
>
>>
>> if (!check)
>> check = git_attr_check_initl("text", NULL);
>>
>> git_check_attr(path, check);
>> act_on(check->value[0]);
>>
>> * While the check for attributes to be questioned only need to
>> be initalized once as that part will be read only after its
>> initialisation, the answer may be different for each path.
>> Because of that we need to decouple the check and the answer,
>> such that each thread can obtain an answer for the path it
>> is currently processing.
>
> Yes, it is good to separate questions and answers. I think answers
> should be owned by the caller, though. I do not think of a good
> reason why you want to make it impossible to do something like this:
>
> struct git_attr_check_result *result_1 = ...allocate...;
> struct git_attr_check_result *result_2 = ...allocate...;
>
> loop {
> struct strbuf path = next_path();
> git_check_attr(result_1, path.buf, check_1);
> if (strbuf_strip_suffix(&path, ".c")) {
> strbuf_addstr(&path, ".h");
> git_check_attr(result_2, path.buf, check_2);
> do something using result_1[] and result_2[];
> } else {
> do something using result_1[];
> }
> }
>
> Do we already have a design of the "result" thing that is concrete
> enough to require us to declare that the result is owned by the
> implementation and asking another question has to destroy the answer
> to the previous question? Otherwise, I'd rather not to see us make
> the API unnecessarily hard to use. While I do want us to avoid
> overengineering for excessive flexibility, I somehow feel "you
> cannot control the lifetime of the result, it is owned by the
> subsystem" falls the other side of the line.
True, we had that issue for other APIs (IIRC path related things,
with 4 static buffers that round robin). I did not like that design decision,
but I felt it was okay, as the above did not occur to me.
In the case above, we could just copy the result_1->values and
then re-use the result_1, but I agree that this may be somewhat error prone
if you're not familiar with the decisions in this series.
So in case of the caller owning the result, we could pull the static
trick for now
and only use a different approach when we use it in actual threaded code, i.e.
the code in convert.c could become:
static struct git_attr_check *check;
static struct git_attr_result *result;
if (!check) {
check = git_attr_check_initl("crlf", "ident",
"filter", "eol", "text", NULL);
result = git_attr_result_alloc(5);
user_convert_tail = &user_convert;
git_config(read_convert_config, NULL);
}
if (!git_check_attr(path, check, result)) {
...
>> + empty `struct git_attr_check` as alloced by git_attr_check_alloc()
>
> "allocated", not "alloced".
ok.
>
>> + can be prepared by calling `git_attr_check_alloc()` function and
>> + then attributes you want to ask about can be added to it with
>> + `git_attr_check_append()` function.
>> + git_attr_check_initl is thread safe, i.e. you can call it
>
> Spell it `git_attr_check_initl()` for consistency.
ok.
>
>> + from different threads at the same time; internally however only one
>> + call at a time is processed. If the calls from different threads have
>> + the same arguments, the returned `git_attr_check` may be the same.
>
> I find this description a bit confusing. At least the way I
> envisioned was that when this piece of code is run by multiple
> people at the same time,
>
> static struct git_attr_check *check = NULL;
> git_attr_check_initl(&check, ...);
>
> we won't waste the "check" by allocated by the first one by
> overwriting it with another one allocated by the second one. So
> "the same arguments" does not come into the picture. A single
> variable is either
>
> * already allocated and initialised by the an earlier call to
> initl() by somebody else, or
>
> * originally NULL when you call initl(), and the implementation
> makes sure that other people wait while you allocate, initialise
> and assign it to the variable, or
>
> * originally NULL when you call initl(), but the implementation
> notices that somebody else is doing the second bullet point
> above, and you wait until that somebody else finishes and then
> you return without doing anything (because by that time, "check"
> is filled by that other party doing the second bullet point
> above).
>
> There is no need to check for "the same arguments".
>
I see. So we assume that there are no different arguments at the same time,
i.e. all threads run the same code when it comes to attrs.
Brandon wrote:
> On 10/10, Stefan Beller wrote:
>> be initalized once as that part will be read only after its
>> initialized
>> initialisation, the answer may be different for each path.
> should this be the US spelling 'initialization'?
Yes, we'd want to be consistent, indeed. Sometimes the British spelling
slips through as that's what I learned in high school.
Specifically for initialise:
$ git grep initialise
contrib/examples/git-notes.sh: die "Will not
initialise with empty tree"
object.h: * it can return "yes we have, and here is a half-initialised object"
object.h: * half-initialised objects, the caller is expected to initialize them
revision.c:static struct treesame_state *initialise_treesame(struct
rev_info *revs, struct commit *commit)
revision.c: ts = initialise_treesame(revs, commit);
+ a lot of french translations.
The American spelling is found a lot more.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 28/28] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-11 17:40 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, pclouds
In-Reply-To: <20161011002115.23312-29-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> This revamps the API of the attr subsystem to be thread safe.
> Before we had the question and its results in one struct type.
> The typical usage of the API was
>
> static struct git_attr_check;
I think you meant "*check" at the end, perhaps?
>
> if (!check)
> check = git_attr_check_initl("text", NULL);
>
> git_check_attr(path, check);
> act_on(check->value[0]);
>
> * While the check for attributes to be questioned only need to
> be initalized once as that part will be read only after its
> initialisation, the answer may be different for each path.
> Because of that we need to decouple the check and the answer,
> such that each thread can obtain an answer for the path it
> is currently processing.
Yes, it is good to separate questions and answers. I think answers
should be owned by the caller, though. I do not think of a good
reason why you want to make it impossible to do something like this:
struct git_attr_check_result *result_1 = ...allocate...;
struct git_attr_check_result *result_2 = ...allocate...;
loop {
struct strbuf path = next_path();
git_check_attr(result_1, path.buf, check_1);
if (strbuf_strip_suffix(&path, ".c")) {
strbuf_addstr(&path, ".h");
git_check_attr(result_2, path.buf, check_2);
do something using result_1[] and result_2[];
} else {
do something using result_1[];
}
}
Do we already have a design of the "result" thing that is concrete
enough to require us to declare that the result is owned by the
implementation and asking another question has to destroy the answer
to the previous question? Otherwise, I'd rather not to see us make
the API unnecessarily hard to use. While I do want us to avoid
overengineering for excessive flexibility, I somehow feel "you
cannot control the lifetime of the result, it is owned by the
subsystem" falls the other side of the line.
> diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
> index 92fc32a..2059aab 100644
> --- a/Documentation/technical/api-gitattributes.txt
> +++ b/Documentation/technical/api-gitattributes.txt
> @@ -8,6 +8,18 @@ attributes to set of paths.
> Data Structure
> --------------
>
> +extern struct git_attr *git_attr(const char *);
> +
> +/*
> + * Return the name of the attribute represented by the argument. The
> + * return value is a pointer to a null-delimited string that is part
> + * of the internal data structure; it should not be modified or freed.
> + */
> +extern const char *git_attr_name(const struct git_attr *);
> +
> +extern int attr_name_valid(const char *name, size_t namelen);
> +extern void invalid_attr_name_message(struct strbuf *, const char *, int);
> +
> `struct git_attr`::
>
> An attribute is an opaque object that is identified by its name.
> @@ -16,15 +28,17 @@ Data Structure
> of no interest to the calling programs. The name of the
> attribute can be retrieved by calling `git_attr_name()`.
>
> -`struct git_attr_check_elem`::
> -
> - This structure represents one attribute and its value.
> -
> `struct git_attr_check`::
>
> - This structure represents a collection of `git_attr_check_elem`.
> + This structure represents a collection of `struct git_attrs`.
> It is passed to `git_check_attr()` function, specifying the
> - attributes to check, and receives their values.
> + attributes to check, and receives their values into a corresponding
> + `struct git_attr_result`.
> +
> +`struct git_attr_result`::
> +
> + This structure represents a collection of results to its
> + corresponding `struct git_attr_check`, that has the same order.
>
>
> Attribute Values
> @@ -56,16 +70,22 @@ Querying Specific Attributes
> * Prepare `struct git_attr_check` using git_attr_check_initl()
> function, enumerating the names of attributes whose values you are
> interested in, terminated with a NULL pointer. Alternatively, an
> - empty `struct git_attr_check` can be prepared by calling
> - `git_attr_check_alloc()` function and then attributes you want to
> - ask about can be added to it with `git_attr_check_append()`
> - function.
> + empty `struct git_attr_check` as alloced by git_attr_check_alloc()
"allocated", not "alloced".
> + can be prepared by calling `git_attr_check_alloc()` function and
> + then attributes you want to ask about can be added to it with
> + `git_attr_check_append()` function.
> + git_attr_check_initl is thread safe, i.e. you can call it
Spell it `git_attr_check_initl()` for consistency.
> + from different threads at the same time; internally however only one
> + call at a time is processed. If the calls from different threads have
> + the same arguments, the returned `git_attr_check` may be the same.
I find this description a bit confusing. At least the way I
envisioned was that when this piece of code is run by multiple
people at the same time,
static struct git_attr_check *check = NULL;
git_attr_check_initl(&check, ...);
we won't waste the "check" by allocated by the first one by
overwriting it with another one allocated by the second one. So
"the same arguments" does not come into the picture. A single
variable is either
* already allocated and initialised by the an earlier call to
initl() by somebody else, or
* originally NULL when you call initl(), and the implementation
makes sure that other people wait while you allocate, initialise
and assign it to the variable, or
* originally NULL when you call initl(), but the implementation
notices that somebody else is doing the second bullet point
above, and you wait until that somebody else finishes and then
you return without doing anything (because by that time, "check"
is filled by that other party doing the second bullet point
above).
There is no need to check for "the same arguments".
^ permalink raw reply
* Re: [PATCH 28/28] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-11 18:23 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <CAGZ79kaQKDdJfMOjDKEK_dZJhgj+R7rByQS++B3OOBy6uO1x2w@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> I find this description a bit confusing. At least the way I
>> envisioned was that when this piece of code is run by multiple
>> people at the same time,
>>
>> static struct git_attr_check *check = NULL;
>> git_attr_check_initl(&check, ...);
>>
>> we won't waste the "check" by allocated by the first one by
>> overwriting it with another one allocated by the second one. So
>> "the same arguments" does not come into the picture. A single
>> variable is either
>>
>> * already allocated and initialised by the an earlier call to
>> initl() by somebody else, or
>>
>> * originally NULL when you call initl(), and the implementation
>> makes sure that other people wait while you allocate, initialise
>> and assign it to the variable, or
>>
>> * originally NULL when you call initl(), but the implementation
>> notices that somebody else is doing the second bullet point
>> above, and you wait until that somebody else finishes and then
>> you return without doing anything (because by that time, "check"
>> is filled by that other party doing the second bullet point
>> above).
>>
>> There is no need to check for "the same arguments".
>>
>
> I see. So we assume that there are no different arguments at the same time,
> i.e. all threads run the same code when it comes to attrs.
Sorry, but I fail to see how you can jump to that conclusion.
Puzzled.
You can have many different callsites (think: hits "git grep" finds)
that call git_attr_check_initl() and they all may be asking for
different set of attributes. As long as they are using different
"check" instance to receive these sets of attributes, they are OK.
It is insane to use the same "check" variable to receive sets of
attributes for different attributes, be it from the same call or
different one, it is insane to do this:
func(char *anotherattr)
{
static struct git_attr_check *check = NULL;
git_attr_check_initl(&check, "one", anotherattr, ...);
... use "check" to ask question ...
}
The whole point of having a static, initialize-once instance of
"check" is so that initl() can do potentially heavy preparation just
once and keep reusing it. Allowing a later caller of func() to pass
a value of anotherattr that is different from the one used in the
first call that did cause initl() to allocate-initialise-assign to
"check" is simply insane, even there is no threading issue.
And in a threaded environment it is even worse; the first thread may
call initl() to get one set of attributes in "check" and it may be
about to ask the question, while the second call may call initl()
and by your definition it will notice they have different sets of
attributes and returns different "check"? Either the earlier one is
leaked, or it gets destroyed even though the first thread hasn't
finished with "check" it got.
It is perfectly OK to drop "static" from the above example code.
Then it no longer is insane--it is perfectly normal code whose
inefficiency cannot be avoided because it wants to do dynamic
queries.
^ permalink raw reply
* Re: [PATCH 17/28] attr: expose validity check for attribute names
From: Brandon Williams @ 2016-10-11 17:28 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, pclouds
In-Reply-To: <20161011002115.23312-18-sbeller@google.com>
On 10/10, Stefan Beller wrote:
> From: Junio C Hamano <gitster@pobox.com>
> -static int invalid_attr_name(const char *name, int namelen)
> +int attr_name_valid(const char *name, size_t namelen)
> {
> /*
> * Attribute name cannot begin with '-' and must consist of
> * characters from [-A-Za-z0-9_.].
> */
> if (namelen <= 0 || *name == '-')
> - return -1;
> + return 0;
> while (namelen--) {
> char ch = *name++;
> if (! (ch == '-' || ch == '.' || ch == '_' ||
> ('0' <= ch && ch <= '9') ||
> ('a' <= ch && ch <= 'z') ||
> ('A' <= ch && ch <= 'Z')) )
> - return -1;
> + return 0;
> }
> - return 0;
> + return -1;
> +}
Whats the reason behind returning -1 for a valid attr name vs 1?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 17/28] attr: expose validity check for attribute names
From: Stefan Beller @ 2016-10-11 18:28 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <20161011172805.GB9085@google.com>
On Tue, Oct 11, 2016 at 10:28 AM, Brandon Williams <bmwill@google.com> wrote:
> On 10/10, Stefan Beller wrote:
>> From: Junio C Hamano <gitster@pobox.com>
>> -static int invalid_attr_name(const char *name, int namelen)
>> +int attr_name_valid(const char *name, size_t namelen)
>> {
>> /*
>> * Attribute name cannot begin with '-' and must consist of
>> * characters from [-A-Za-z0-9_.].
>> */
>> if (namelen <= 0 || *name == '-')
>> - return -1;
>> + return 0;
>> while (namelen--) {
>> char ch = *name++;
>> if (! (ch == '-' || ch == '.' || ch == '_' ||
>> ('0' <= ch && ch <= '9') ||
>> ('a' <= ch && ch <= 'z') ||
>> ('A' <= ch && ch <= 'Z')) )
>> - return -1;
>> + return 0;
>> }
>> - return 0;
>> + return -1;
>> +}
>
> Whats the reason behind returning -1 for a valid attr name vs 1?
>
Usually we prefer negative values for errors, whereas 0,1,2...
is used for actual return values.
If you're not really interested in the value, but rather "does it work at all?"
you can use it via
if (function() < 0)
die_errno("function has error");
Otherwise you'd do a
int value = function();
if (value < 0)
die(...);
switch(value) {
default:
case 0:
doZeroThing(); break;
case 1:
doOtherThing();
}
At first I thought the two different return path for -1 are different
error classes (one being just a minor error compared to the other),
but they are not, so having the same code there makes sense.
So I think we could change it to +1 in this instance, as a non zero
value would just indicate the attr name is not valid, but not necessarily
an error.
^ permalink raw reply
* Re: [PATCH 17/28] attr: expose validity check for attribute names
From: Stefan Beller @ 2016-10-11 18:44 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <20161011184010.GA16014@google.com>
On Tue, Oct 11, 2016 at 11:40 AM, Brandon Williams <bmwill@google.com> wrote:
> Wouldn't a +1 indicate that the attr name is valid while the 0
> indicates that it is invalid?
Right.
> It looks to me like we are checking each
> character and if we run into one that doesn't work then we have an error
> and return 0 indicating that the attr name we are checking is invalid,
> otherwise the name is valid and we would want to return a +1 indicating
> a success.
>
> Am I understanding the intent of this function properly?
>
I will change it to +1; I was just rambling about when you may
expect -1 in this project. :)
^ permalink raw reply
* Allow "git shortlog" to group by committer information
From: Linus Torvalds @ 2016-10-11 18:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 482 bytes --]
In some situations you may want to group the commits not by author,
but by committer instead.
For example, when I just wanted to look up what I'm still missing from
linux-next in the current merge window, I don't care so much about who
wrote a patch, as what git tree it came from, which generally boils
down to "who committed it".
So make git shortlog take a "-c" or "--committer" option to switch
grouping to that.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 2398 bytes --]
builtin/shortlog.c | 15 ++++++++++++---
shortlog.h | 1 +
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index ba0e1154a..c9585d475 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -117,11 +117,15 @@ static void read_from_stdin(struct shortlog *log)
{
struct strbuf author = STRBUF_INIT;
struct strbuf oneline = STRBUF_INIT;
+ static const char *author_match[2] = { "Author: ", "author " };
+ static const char *committer_match[2] = { "Commit: ", "committer " };
+ const char **match;
+ match = log->committer ? committer_match : author_match;
while (strbuf_getline_lf(&author, stdin) != EOF) {
const char *v;
- if (!skip_prefix(author.buf, "Author: ", &v) &&
- !skip_prefix(author.buf, "author ", &v))
+ if (!skip_prefix(author.buf, match[0], &v) &&
+ !skip_prefix(author.buf, match[1], &v))
continue;
while (strbuf_getline_lf(&oneline, stdin) != EOF &&
oneline.len)
@@ -140,6 +144,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
struct strbuf author = STRBUF_INIT;
struct strbuf oneline = STRBUF_INIT;
struct pretty_print_context ctx = {0};
+ const char *fmt;
ctx.fmt = CMIT_FMT_USERFORMAT;
ctx.abbrev = log->abbrev;
@@ -148,7 +153,9 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
ctx.date_mode.type = DATE_NORMAL;
ctx.output_encoding = get_log_output_encoding();
- format_commit_message(commit, "%an <%ae>", &author, &ctx);
+ fmt = log->committer ? "%cn <%ce>" : "%an <%ae>";
+
+ format_commit_message(commit, fmt, &author, &ctx);
if (!log->summary) {
if (log->user_format)
pretty_print_commit(&ctx, commit, &oneline);
@@ -238,6 +245,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
int nongit = !startup_info->have_repository;
const struct option options[] = {
+ OPT_BOOL('c', "committer", &log.committer,
+ N_("Group by committer rather than author")),
OPT_BOOL('n', "numbered", &log.sort_by_number,
N_("sort output according to the number of commits per author")),
OPT_BOOL('s', "summary", &log.summary,
diff --git a/shortlog.h b/shortlog.h
index 5a326c686..5d64cfe92 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -13,6 +13,7 @@ struct shortlog {
int in2;
int user_format;
int abbrev;
+ int committer;
char *common_repo_prefix;
int email;
^ permalink raw reply related
* Re: [PATCH 17/28] attr: expose validity check for attribute names
From: Brandon Williams @ 2016-10-11 18:40 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <CAGZ79kbqikTtUGxXFB3W9k1nL6ST2iwRRi5zEuL9BOpHxZWc8A@mail.gmail.com>
On 10/11, Stefan Beller wrote:
> On Tue, Oct 11, 2016 at 10:28 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 10/10, Stefan Beller wrote:
> >> From: Junio C Hamano <gitster@pobox.com>
> >> -static int invalid_attr_name(const char *name, int namelen)
> >> +int attr_name_valid(const char *name, size_t namelen)
> >> {
> >> /*
> >> * Attribute name cannot begin with '-' and must consist of
> >> * characters from [-A-Za-z0-9_.].
> >> */
> >> if (namelen <= 0 || *name == '-')
> >> - return -1;
> >> + return 0;
> >> while (namelen--) {
> >> char ch = *name++;
> >> if (! (ch == '-' || ch == '.' || ch == '_' ||
> >> ('0' <= ch && ch <= '9') ||
> >> ('a' <= ch && ch <= 'z') ||
> >> ('A' <= ch && ch <= 'Z')) )
> >> - return -1;
> >> + return 0;
> >> }
> >> - return 0;
> >> + return -1;
> >> +}
> >
> > Whats the reason behind returning -1 for a valid attr name vs 1?
> >
> At first I thought the two different return path for -1 are different
> error classes (one being just a minor error compared to the other),
> but they are not, so having the same code there makes sense.
>
> So I think we could change it to +1 in this instance, as a non zero
> value would just indicate the attr name is not valid, but not necessarily
> an error.
Wouldn't a +1 indicate that the attr name is valid while the 0
indicates that it is invalid? It looks to me like we are checking each
character and if we run into one that doesn't work then we have an error
and return 0 indicating that the attr name we are checking is invalid,
otherwise the name is valid and we would want to return a +1 indicating
a success.
Am I understanding the intent of this function properly?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 17/28] attr: expose validity check for attribute names
From: Brandon Williams @ 2016-10-11 18:49 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <CAGZ79kaM_5MGwPPAhQKWgCGirbHvioBY7_PtkKxza8qV6ZyRnw@mail.gmail.com>
On 10/11, Stefan Beller wrote:
> On Tue, Oct 11, 2016 at 11:40 AM, Brandon Williams <bmwill@google.com> wrote:
>
> > Wouldn't a +1 indicate that the attr name is valid while the 0
> > indicates that it is invalid?
>
> Right.
>
> > It looks to me like we are checking each
> > character and if we run into one that doesn't work then we have an error
> > and return 0 indicating that the attr name we are checking is invalid,
> > otherwise the name is valid and we would want to return a +1 indicating
> > a success.
> >
> > Am I understanding the intent of this function properly?
> >
>
> I will change it to +1; I was just rambling about when you may
> expect -1 in this project. :)
Ah got it :D
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 28/28] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-11 18:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <xmqqa8eaagoq.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 11, 2016 at 11:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> I find this description a bit confusing. At least the way I
>>> envisioned was that when this piece of code is run by multiple
>>> people at the same time,
>>>
>>> static struct git_attr_check *check = NULL;
>>> git_attr_check_initl(&check, ...);
>>>
>>> we won't waste the "check" by allocated by the first one by
>>> overwriting it with another one allocated by the second one. So
>>> "the same arguments" does not come into the picture. A single
>>> variable is either
>>>
>>> * already allocated and initialised by the an earlier call to
>>> initl() by somebody else, or
>>>
>>> * originally NULL when you call initl(), and the implementation
>>> makes sure that other people wait while you allocate, initialise
>>> and assign it to the variable, or
>>>
>>> * originally NULL when you call initl(), but the implementation
>>> notices that somebody else is doing the second bullet point
>>> above, and you wait until that somebody else finishes and then
>>> you return without doing anything (because by that time, "check"
>>> is filled by that other party doing the second bullet point
>>> above).
>>>
>>> There is no need to check for "the same arguments".
>>>
>>
>> I see. So we assume that there are no different arguments at the same time,
>> i.e. all threads run the same code when it comes to attrs.
>
> Sorry, but I fail to see how you can jump to that conclusion.
> Puzzled.
>
> You can have many different callsites (think: hits "git grep" finds)
> that call git_attr_check_initl() and they all may be asking for
> different set of attributes. As long as they are using different
> "check" instance to receive these sets of attributes, they are OK.
Right, but that requires a mutex per callsite; up to now I imagined
a global mutex only, which is how I came to the conclusion.
>
> It is insane to use the same "check" variable to receive sets of
> attributes for different attributes,
I agree.
> be it from the same call or
> different one, it is insane to do this:
>
> func(char *anotherattr)
> {
> static struct git_attr_check *check = NULL;
> git_attr_check_initl(&check, "one", anotherattr, ...);
>
> ... use "check" to ask question ...
> }
>
> The whole point of having a static, initialize-once instance of
> "check" is so that initl() can do potentially heavy preparation just
> once and keep reusing it. Allowing a later caller of func() to pass
> a value of anotherattr that is different from the one used in the
> first call that did cause initl() to allocate-initialise-assign to
> "check" is simply insane, even there is no threading issue.
I was imagining a file.c like that:
static struct git_attr_check *check = NULL;
void one_func()
{
git_attr_check_initl(&check, "one", ...);
...
}
void two_func()
{
git_attr_check_initl(&check, "two", ...);
...
}
int foo_cmd(const char *prefix int argc, char **argv)
{
foreach_path(get_paths(...))
one_func();
check = NULL;
foreach_path(get_paths(...))
two_func();
}
This is correct single threaded code, but as soon as you want to
put phase one,two into threads, as they can be parallelized, this
goes horribly wrong.
>
> And in a threaded environment it is even worse; the first thread may
> call initl() to get one set of attributes in "check" and it may be
> about to ask the question, while the second call may call initl()
> and by your definition it will notice they have different sets of
> attributes and returns different "check"? Either the earlier one is
> leaked, or it gets destroyed even though the first thread hasn't
> finished with "check" it got.
>
> It is perfectly OK to drop "static" from the above example code.
> Then it no longer is insane--it is perfectly normal code whose
> inefficiency cannot be avoided because it wants to do dynamic
> queries.
I think we had a misunderstanding here, as I was just assuming a
single mutex later on.
^ permalink raw reply
* Re: [PATCH v3 08/25] sequencer: strip CR from the todo script
From: Junio C Hamano @ 2016-10-11 18:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <5a639610c09bf1dacc8143603f321a8ea23cd270.1476120229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> It is not unheard of that editors on Windows write CR/LF even if the
> file originally had only LF. This is particularly awkward for exec lines
> of a rebase -i todo sheet. Take for example the insn "exec echo": The
> shell script parser splits at the LF and leaves the CR attached to
> "echo", which leads to the unknown command "echo\r".
>
> Work around that by stripping CR when reading the todo commands, as we
> already do for LF.
>
> This happens to fix t9903.14 and .15 in MSYS1 environments (with the
> rebase--helper patches based on this patch series): the todo script
> constructed in such a setup contains CR/LF thanks to MSYS1 runtime's
> cleverness.
>
> Based on a report and a patch by Johannes Sixt.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 678fdf3..cee7e50 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -774,6 +774,9 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
>
> next_p = *eol ? eol + 1 /* skip LF */ : eol;
>
> + if (p != eol && eol[-1] == '\r')
> + eol--; /* skip Carriage Return */
micronit: s/skip/strip/ ;-)
> +
> item = append_new_todo(todo_list);
> item->offset_in_buf = p - todo_list->buf.buf;
> if (parse_insn_line(item, p, eol)) {
^ permalink raw reply
* Re: [PATCH 0/2] Support `git reset --stdin`
From: Jeff King @ 2016-10-11 18:34 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1476202100.git.johannes.schindelin@gmx.de>
On Tue, Oct 11, 2016 at 06:08:56PM +0200, Johannes Schindelin wrote:
> This feature was missing, and made it cumbersome for third-party
> tools to reset a lot of paths in one go.
>
> Support for --stdin has been added, following builtin/checkout-index.c's
> example.
Is git-reset the right layer to add scripting features? I thought we
usually pushed people doing mass index manipulation to use update-index
or read-tree. Is there something that reset makes easy that is hard with
those tools (I could imagine "--hard", but I see it is not supported
with your patch).
Not that I'm necessarily opposed to the patch, I was just surprised.
-Peff
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Jeff King @ 2016-10-11 19:01 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <CA+55aFzWkE43rSm-TJNKkHq4F3eOiGR0-Bo9V1=a1s=vQ0KPqQ@mail.gmail.com>
On Tue, Oct 11, 2016 at 11:45:58AM -0700, Linus Torvalds wrote:
> In some situations you may want to group the commits not by author,
> but by committer instead.
>
> For example, when I just wanted to look up what I'm still missing from
> linux-next in the current merge window, I don't care so much about who
> wrote a patch, as what git tree it came from, which generally boils
> down to "who committed it".
>
> So make git shortlog take a "-c" or "--committer" option to switch
> grouping to that.
I made a very similar patch as part of a larger series:
http://public-inbox.org/git/20151229073515.GK8842@sigill.intra.peff.net/
but never followed through with it because it wasn't clear that grouping
by anything besides author was actually useful to anybody.
My implementation is a little more complicated because it's also setting
things up for grouping by trailers (so you can group by "signed-off-by",
for example). I don't know if that's useful to your or not.
I'm fine with this less invasive version, but a few suggestions:
- do you want to call it --group-by=committer (with --group-by=author
as the default), which could later extend naturally to other forms of
grouping?
- you might want to steal the tests and documentation from my patch
(though obviously they would need tweaked to match your interface)
-Peff
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Linus Torvalds @ 2016-10-11 19:07 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <20161011190103.fovcwsze77hkew4t@sigill.intra.peff.net>
On Tue, Oct 11, 2016 at 12:01 PM, Jeff King <peff@peff.net> wrote:
>
> My implementation is a little more complicated because it's also setting
> things up for grouping by trailers (so you can group by "signed-off-by",
> for example). I don't know if that's useful to your or not.
Hmm. Maybe in theory. But probably not in reality - it's just not
unique enough (ie there are generally multiple, and if you choose the
first/last, it should be the same as author/committer, so it doesn't
actually add anything).
There are possibly other things that *could* be grouped by and might be useful:
- main subdirectory it touches (I've often wanted that)
- rough size of diff or number of files it touches
but realistically both are painful enough that it probably doesn't
make sense to do in some low-level helper.
> I'm fine with this less invasive version, but a few suggestions:
>
> - do you want to call it --group-by=committer (with --group-by=author
> as the default), which could later extend naturally to other forms of
> grouping?
Honestly, it's probably the more generic one, but especially for
one-off commands that aren't that common, it's a pain to write. When
testing it, I literally just used "-c" for that reason.
I wrote the patch because I've wanted this before, but it's a "once or
twice a merge window" thing for me, so ..
> - you might want to steal the tests and documentation from my patch
> (though obviously they would need tweaked to match your interface)
Heh. Yes.
Linus
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox