* 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
* Re: Allow "git shortlog" to group by committer information
From: Jeff King @ 2016-10-11 19:17 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <CA+55aFzw24pHGOYFBFVvTbU1Cudcr8zcPt_RvdQSxrKY5weCbQ@mail.gmail.com>
On Tue, Oct 11, 2016 at 12:07:40PM -0700, Linus Torvalds wrote:
> 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).
The implementation I did credited each commit multiple times if the
trailer appeared more than once. If you want to play with it, you can
fetch it from:
git://github.com/peff jk/shortlog-ident
and then something like:
git shortlog --ident=reviewed-by --format='...reviewed %an'
works. I haven't found it to really be useful for more than toy
statistic gathering, though.
> 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.
Yeah, I think there's a lot of policy there in what counts as "main",
the rough sizes, etc. I've definitely done queries like that before, but
usually by piping "log --numstat" into perl. It's a minor pain to get
the data into perl data structures, but once you have it, you have a lot
more flexibility in what you can compute.
That might be aided by providing more structured machine-readable output
from git, like JSON (which I don't particularly like, but it's kind-of a
standard, and it sure as hell beats XML). But obviously that's another
topic entirely.
> > 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.
It's not the end of the world to call it "-c" now, and later define "-c"
as a shorthand for "--group-by=committer", if and when the latter comes
into existence.
Keep in mind that shortlog takes arbitrary revision options, too, and
"-c" is defined there for combined diffs. I can't think of a good reason
to want to pass it to shortlog, though, so I don't think it's a big
loss.
-Peff
^ permalink raw reply
* Re: [PATCH v3 13/25] sequencer: prepare for rebase -i's commit functionality
From: Junio C Hamano @ 2016-10-11 19:17 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Jakub Narębski, Johannes Sixt, Michael Haggerty
In-Reply-To: <9b2782e64f678ab2d261042b63fafc3e4a519295.1476120229.git.johannes.schindelin@gmx.de>
> @@ -370,19 +383,79 @@ static int is_index_unchanged(void)
> }
>
> /*
> + * Read the author-script file into an environment block, ready for use in
> + * run_command(), that can be free()d afterwards.
> + */
> +static char **read_author_script(void)
> +{
> + struct strbuf script = STRBUF_INIT;
> + int i, count = 0;
> + char *p, *p2, **env;
> + size_t env_size;
> +
> + if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
> + return NULL;
> +
> + for (p = script.buf; *p; p++)
> + if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> + strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> + else if (*p == '\'')
> + strbuf_splice(&script, p-- - script.buf, 1, "", 0);
> + else if (*p == '\n') {
> + *p = '\0';
> + count++;
> + }
Hmph, didn't we recently add parse_key_value_squoted() to build
read_author_script() in builtin/am.c on top of it, so that this
piece of code can also take advantage of and share the parser?
> +/*
Offtopic: this line and the beginning of the new comment block that
begins with "Read the author-script" above show a suboptimal marking
of what is added and what is left. I wonder "diff-indent-heuristic"
topic by Michael can help to make it look better.
> * If we are cherry-pick, and if the merge did not result in
> * hand-editing, we will hit this commit and inherit the original
> * author date and name.
> + *
> * If we are revert, or if our cherry-pick results in a hand merge,
> * we had better say that the current user is responsible for that.
> + *
> + * An exception is when run_git_commit() is called during an
> + * interactive rebase: in that case, we will want to retain the
> + * author metadata.
> */
^ permalink raw reply
* interactive rebase should better highlight the not-applying commit
From: Joshua N Pritikin @ 2016-10-11 19:07 UTC (permalink / raw)
To: git
As of GIT 2.8.1, if you do an interactive rebase and get some conflict
in the stack of patches then the commit with the conflict is buried in
4-5 lines of output. It is visually difficult to immediately pick out
which commit did not apply cleanly. I suggest highlighting the 1 line
commit summary in red or green or some color to help it stand out from
all the other output.
I decided to suggest this change after I realized that I probably
skipped a commit during an interactive rebase instead of resolving the
conflict. I knew I had to skip some commit so I assumed that I just need
to skip without reading the commit summary carefully. Now it is 7-15
days after I did the erroneous rebase. I had to spend a few hours today
with GIT's archaeology tools to find the lost code.
I assume somebody familiar with GIT's code base could make this change
in about 10 minutes.
--
Joshua N. Pritikin, Ph.D.
Virginia Institute for Psychiatric and Behavioral Genetics
Virginia Commonwealth University
PO Box 980126
800 E Leigh St, Biotech One, Suite 1-133
Richmond, VA 23219
http://people.virginia.edu/~jnp3bc
^ permalink raw reply
* Re: [PATCH v3 12/25] sequencer: remember the onelines when parsing the todo file
From: Junio C Hamano @ 2016-10-11 19:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <f22bdcda3f748813e6b9fd08dc65f11c6f9f4b1e.1476120229.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The `git-rebase-todo` file contains a list of commands. Most of those
> commands have the form
>
> <verb> <sha1> <oneline>
>
> The <oneline> is displayed primarily for the user's convenience, as
> rebase -i really interprets only the <verb> <sha1> part. However, there
> are *some* places in interactive rebase where the <oneline> is used to
> display messages, e.g. for reporting at which commit we stopped.
>
> So let's just remember it when parsing the todo file; we keep a copy of
> the entire todo file anyway (to write out the new `done` and
> `git-rebase-todo` file just before processing each command), so all we
> need to do is remember the begin offsets and lengths.
>
> As we will have to parse and remember the command-line for `exec` commands
> later, we do not call the field "oneline" but rather "arg" (and will reuse
> that for exec's command-line).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index afc494e..7ba5e07 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -708,6 +708,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> struct todo_item {
> enum todo_command command;
> struct commit *commit;
> + const char *arg;
> + int arg_len;
> size_t offset_in_buf;
micronit: you can make it to size_t and lose the cast below, no?
> };
>
> @@ -759,6 +761,9 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
> status = get_sha1(bol, commit_sha1);
> *end_of_object_name = saved;
>
> + item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> + item->arg_len = (int)(eol - item->arg);
> +
> if (status < 0)
> return -1;
>
> @@ -911,6 +916,8 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
>
> item->command = command;
> item->commit = commit;
> + item->arg = NULL;
> + item->arg_len = 0;
> item->offset_in_buf = todo_list->buf.len;
> subject_len = find_commit_subject(commit_buffer, &subject);
> strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
^ permalink raw reply
* Re: [PATCH] contrib: add credential helper for libsecret
From: Junio C Hamano @ 2016-10-11 19:36 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: Mantas Mikulėnas, git
In-Reply-To: <1476198080.3876.8.camel@kaarsemaker.net>
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:
>
>> + s = g_hash_table_lookup(attributes, "user");
>> + if (s) {
>> + g_free(c->username);
>> + c->username = g_strdup(s);
>> + }
>
> This always overwrites c->username, the original gnome-keyring version
> only does that when the username isn't set. Other than that it looks
> good to me.
>
> Reviewed-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> Tested-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Thanks for a review. I'll wait until one of (1) a squashable patch
to address the "we do not want unconditional overwrite" issue, (2) a
reroll from Mantas to do the same, or (3) a counter-argument from
somebody to explain why unconditional overwrite is a good idea here
(but not in the original) appears.
^ permalink raw reply
* Re: [PATCH 0/2] Support `git reset --stdin`
From: Junio C Hamano @ 2016-10-11 19:27 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20161011183448.yasglfjelo4kgrq4@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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.
If read-tree had pathspec support (i.e. "read these and only these
paths given from the command line into the index from a given
tree-ish"), that would have been the most natural place to extend
with "oh, by the way, instead of the command line, you can feed the
paths on the standard input".
But it doesn't have one.
^ 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