* [RFC 0/1] de-quote quoted-strings in mailinfo
From: Kevin Daudt @ 2016-09-13 23:46 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt
In-Reply-To: <20160913152622.2xtyn6mki6p6afsg@sigill.intra.peff.net>
This is my first 'big' C patch, so first an RFC.
This patch implements RFC2822 dequoting of quoted-pairs in quoted
strings, which was not done yet. This means removing the "\" as escape
character from header fields, but only quoted strings, and comments
(text between braces).
According to the RFC, comments can also appear in square brackets in the
e-mail domain, but that has not been implemented. In fact, just like
other functions, it just looks at the whole header line.
Please let me know what you think.
Kevin Daudt (1):
mailinfo: de-quote quoted-pair in header fields
mailinfo.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
t/t5100-mailinfo.sh | 5 +++++
t/t5100/quoted-pair.expect | 5 +++++
t/t5100/quoted-pair.in | 9 +++++++++
t/t5100/quoted-pair.info | 5 +++++
5 files changed, 70 insertions(+)
create mode 100644 t/t5100/quoted-pair.expect
create mode 100644 t/t5100/quoted-pair.in
create mode 100644 t/t5100/quoted-pair.info
--
2.10.0.rc2
^ permalink raw reply
* [RFC 0/1] de-quote quoted-strings in mailinfo
From: Kevin Daudt @ 2016-09-13 23:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt
This is my first 'big' C patch, so first an RFC.
This patch implements RFC2822 dequoting of quoted-pairs in quoted
strings, which was not done yet. This means removing the "\" as escape
character from header fields, but only quoted strings, and comments
(text between braces).
According to the RFC, comments can also appear in square brackets in the
e-mail domain, but that has not been implemented. In fact, just like
other functions, it just looks at the whole header line.
Please let me know what you think.
Kevin Daudt (1):
mailinfo: de-quote quoted-pair in header fields
mailinfo.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
t/t5100-mailinfo.sh | 5 +++++
t/t5100/quoted-pair.expect | 5 +++++
t/t5100/quoted-pair.in | 9 +++++++++
t/t5100/quoted-pair.info | 5 +++++
5 files changed, 70 insertions(+)
create mode 100644 t/t5100/quoted-pair.expect
create mode 100644 t/t5100/quoted-pair.in
create mode 100644 t/t5100/quoted-pair.info
--
2.10.0.rc2
^ permalink raw reply
* Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0
From: Stefan Beller @ 2016-09-13 23:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Christian Couder, git@vger.kernel.org
In-Reply-To: <xmqq60pztlz2.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 13, 2016 at 4:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So would we rather want to keep the ecbdata around for each file pair and
>> just reference that? I thought we deliberately want to avoid ecbdata, so maybe
>> we rather want to have another struct that keeps path related information
>> around (pointer to the blob and white space information).
>
> I would expect that there would be two structs, one per path
> "struct buffered_patch" that has the per-path thing, and another per
> line "struct buffered_patch_line" that describes what each line is,
> and has a pointer to the former.
>
Heh, I was trying to come up with a clever thing to save that pointer,
as we would need to have that pointer once per line, so in large patches
that would save a bit of space, but probably I should not try to be too
smart about it.
So I'd split up the struct line_emission into the two proposed
buffered_patch_line as well as buffered_patch.
However the naming is a bit off than I would expect. Historically you
had one patch per file, so it was natural to name a change of multiple
files a "patchset" (c.f. a commit in Gerrit is called "patchset"/revision)
Today as Git is quite successful, one "patch" is easily understood
as the equivalent of one patch, i.e. what format-patch produced.
So I'd prefer to go with buffer_filepair and buffer_line maybe?
^ permalink raw reply
* Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0
From: Junio C Hamano @ 2016-09-13 23:32 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jeff King, Christian Couder, git@vger.kernel.org
In-Reply-To: <CAGZ79kb4Rm=AT-hFutXW5jhZRL0ThDFr789=AbxKC-o+jQ341g@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> So would we rather want to keep the ecbdata around for each file pair and
> just reference that? I thought we deliberately want to avoid ecbdata, so maybe
> we rather want to have another struct that keeps path related information
> around (pointer to the blob and white space information).
I would expect that there would be two structs, one per path
"struct buffered_patch" that has the per-path thing, and another per
line "struct buffered_patch_line" that describes what each line is,
and has a pointer to the former.
^ permalink raw reply
* Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0
From: Stefan Beller @ 2016-09-13 23:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Christian Couder, git@vger.kernel.org
In-Reply-To: <xmqqlgyvtn6l.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 13, 2016 at 4:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +struct line_emission {
>> + const char *set;
>> + const char *line;
>> + const char *ws;
>> + const char *reset;
>> + int first;
>> + int len;
>> + int whitespace_check;
>> + unsigned ws_rule;
>> + int has_trailing_carriage_return;
>> + int has_trailing_newline;
>> +};
>
> It is somewhat strange to see whitespace things are per-line here.
> I'd understand it if it were per-path, though.
Yeah we have to have it at least per path as that is the granularity
the user can configure it.
So would we rather want to keep the ecbdata around for each file pair and
just reference that? I thought we deliberately want to avoid ecbdata, so maybe
we rather want to have another struct that keeps path related information
around (pointer to the blob and white space information).
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH] unpack-trees: pass checkout state explicitly to check_updates()
From: Junio C Hamano @ 2016-09-13 23:25 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <b57982ca-d508-5016-a187-fc3b829f9b0c@web.de>
René Scharfe <l.s.r@web.de> writes:
> Add a parameter for the struct checkout variable to check_updates()
> instead of using a static global variable. Passing it explicitly makes
> object ownership and usage more easily apparent. And we get rid of a
> static variable; those can be problematic in library-like code.
> ...
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 11c37fb..74d6dd4 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -218,8 +218,8 @@ static void unlink_entry(const struct cache_entry *ce)
> schedule_dir_for_removal(ce->name, ce_namelen(ce));
> }
>
> -static struct checkout state;
> ...
> @@ -1094,6 +1094,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> int i, ret;
> static struct cache_entry *dfc;
> struct exclude_list el;
> + struct checkout state;
Does the distinction between this thing in BSS implicitly cleared
and the new one on stack that does not seem to have any
initialization matter?
... goes and looks ...
OK, after this hunk we clear and set up everything in state, so
there is no difference in behaviour. Just we got rid of an
unnecessary file-scope global.
Nice. Thanks.
> if (len > MAX_UNPACK_TREES)
> die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
> @@ -1239,7 +1240,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> }
>
> o->src_index = NULL;
> - ret = check_updates(o) ? (-2) : 0;
> + ret = check_updates(o, &state) ? (-2) : 0;
> if (o->dst_index) {
> if (!ret) {
> if (!o->result.cache_tree)
^ permalink raw reply
* Re: [RFC/PATCH 10/17] submodule.c: convert show_submodule_summary to use emit_line_fmt
From: Stefan Beller @ 2016-09-13 23:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Christian Couder, git@vger.kernel.org
In-Reply-To: <xmqqpoo7tnco.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 13, 2016 at 4:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> In a later patch, I want to propose an option to detect&color
>> moved lines in a diff, which cannot be done in a one-pass over
>> the diff. Instead we need to go over the whole diff twice,
>> because we cannot detect the first line of the two corresponding
>> lines (+ and -) that got moved.
>>
>> So to prepare the diff machinery for two pass algorithms
>> (i.e. buffer it all up and then operate on the result),
>> move all emissions to places, such that the only emitting
>> function is emit_line_0.
>>
>> This prepares the code for submodules to go through the
>> emit_line_0 function.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> I wonder how this interacts with the jk/diff-submodule-diff-inline
> topic by Jacob that has graduated recently to 'master'. IIRC, it
> just lets a separate "git diff" instance that is spawned in the
> submodule directory emit its findings to the output of the driving
> "git diff" in the superproject.
>
The easiest way to find out is to merge HEAD^ of this patch series
(i.e. "diff: buffer output in emit_line_0") with whatever we suspect can
cause breakage for the goal of channeling everything though emit_line_*
functions. Looking at that series, I think I'll have to redo
this (maybe even including sb/diff-cleanup, to have it all in one series)
to capture all output there.
I suspected a breakage, but as the patch series grew larger and larger,
I first wanted to get into a working state before paying attention to solving
conflicts as resolving conflicts is easier when I know where this series is
headed.
Thanks!
Stefan
^ permalink raw reply
* Re: [PATCH] pathspec: removed unnecessary function prototypes
From: Junio C Hamano @ 2016-09-13 23:08 UTC (permalink / raw)
To: Jeff King; +Cc: Brandon Williams, git
In-Reply-To: <20160913181552.74bhacoa2q76yv6k@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Sep 13, 2016 at 09:52:51AM -0700, Brandon Williams wrote:
>
>> removed function prototypes from pathspec.h which don't have a
>> corresponding implementation.
>
> I'm always curious of the "why" in cases like this. Did we forget to add
> them? Did they get renamed? Did they go away?
>
> Looks like the latter; 5a76aff (add: convert to use parse_pathspec,
> 2013-07-14) just forgot to remove them.
Thanks for digging.
^ permalink raw reply
* Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0
From: Junio C Hamano @ 2016-09-13 23:06 UTC (permalink / raw)
To: Stefan Beller; +Cc: peff, chriscool, git
In-Reply-To: <20160913044613.1037-17-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> +struct line_emission {
> + const char *set;
> + const char *line;
> + const char *ws;
> + const char *reset;
> + int first;
> + int len;
> + int whitespace_check;
> + unsigned ws_rule;
> + int has_trailing_carriage_return;
> + int has_trailing_newline;
> +};
It is somewhat strange to see whitespace things are per-line here.
I'd understand it if it were per-path, though.
^ permalink raw reply
* Re: [RFC/PATCH 10/17] submodule.c: convert show_submodule_summary to use emit_line_fmt
From: Junio C Hamano @ 2016-09-13 23:02 UTC (permalink / raw)
To: Stefan Beller; +Cc: peff, chriscool, git
In-Reply-To: <20160913044613.1037-11-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> In a later patch, I want to propose an option to detect&color
> moved lines in a diff, which cannot be done in a one-pass over
> the diff. Instead we need to go over the whole diff twice,
> because we cannot detect the first line of the two corresponding
> lines (+ and -) that got moved.
>
> So to prepare the diff machinery for two pass algorithms
> (i.e. buffer it all up and then operate on the result),
> move all emissions to places, such that the only emitting
> function is emit_line_0.
>
> This prepares the code for submodules to go through the
> emit_line_0 function.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
I wonder how this interacts with the jk/diff-submodule-diff-inline
topic by Jacob that has graduated recently to 'master'. IIRC, it
just lets a separate "git diff" instance that is spawned in the
submodule directory emit its findings to the output of the driving
"git diff" in the superproject.
^ permalink raw reply
* Re: [RFC/PATCH 06/17] diff.c: convert fn_out_consume to use emit_line_*
From: Junio C Hamano @ 2016-09-13 22:56 UTC (permalink / raw)
To: Stefan Beller; +Cc: peff, chriscool, git
In-Reply-To: <20160913044613.1037-7-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> In a later patch, I want to propose an option to detect&color
> moved lines in a diff, which cannot be done in a one-pass over
> the diff. Instead we need to go over the whole diff twice,
> because we cannot detect the first line of the two corresponding
> lines (+ and -) that got moved.
>
> So to prepare the diff machinery for two pass algorithms
> (i.e. buffer it all up and then operate on the result),
> move all emissions to places, such that the only emitting
> function is emit_line_0.
>
> This covers the remaining parts of fn_out_consume.
name_x_tab are colored as before, which you are already aware of and
we'd need to find a way to handle, but other than that, this is a
no-op conversion, getting us closer to the goal of making everything
go through a single funnel.
> name_a_tab = strchr(ecbdata->label_path[0], ' ') ? "\t" : "";
> name_b_tab = strchr(ecbdata->label_path[1], ' ') ? "\t" : "";
> -
> - fprintf(o->file, "%s%s--- %s%s%s\n",
> - line_prefix, meta, ecbdata->label_path[0], reset, name_a_tab);
> - fprintf(o->file, "%s%s+++ %s%s%s\n",
> - line_prefix, meta, ecbdata->label_path[1], reset, name_b_tab);
> + emit_line_fmt(o, meta, reset, "--- %s%s\n",
> + ecbdata->label_path[0], name_a_tab);
> + emit_line_fmt(o, meta, reset, "+++ %s%s\n",
> + ecbdata->label_path[1], name_b_tab);
> ecbdata->label_path[0] = ecbdata->label_path[1] = NULL;
> }
^ permalink raw reply
* Re: [RFC/PATCH 05/17] diff.c: emit_line_0 can handle no color setting
From: Junio C Hamano @ 2016-09-13 22:51 UTC (permalink / raw)
To: Stefan Beller; +Cc: peff, chriscool, git
In-Reply-To: <20160913044613.1037-6-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> In a later patch, I want to propose an option to detect&color
> moved lines in a diff, which cannot be done in a one-pass over
> the diff. Instead we need to go over the whole diff twice,
> because we cannot detect the first line of the two corresponding
> lines (+ and -) that got moved.
>
> So to prepare the diff machinery for two pass algorithms
> (i.e. buffer it all up and then operate on the result),
> move all emissions to places, such that the only emitting
> function is emit_line_0.
>
> In later patches we may pass lines that are not colored to
> the central function emit_line_0, so we
> need to emit the color only when it is non-NULL.
Explained this way, a reader would find that this step is here
before the underlying code is ready--we are not even buffering
at this step yet.
But that is OK. It used to be that passing "" as set/reset was the
way to get a --no-color output. Now you can pass NULL instead of
empty strings. That would be an alternative explanation why this is
an acceptable change (as your later step probably has a good reason
why it cannot pass "" instead of NULL).
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index b6a40ae..5d57130 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -473,11 +473,13 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
> }
>
> if (len || !nofirst) {
> - fputs(set, file);
> + if (set)
> + fputs(set, file);
> if (!nofirst)
> fputc(first, file);
> fwrite(line, len, 1, file);
> - fputs(reset, file);
> + if (reset)
> + fputs(reset, file);
> }
> if (has_trailing_carriage_return)
> fputc('\r', file);
^ permalink raw reply
* Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()
From: Junio C Hamano @ 2016-09-13 22:44 UTC (permalink / raw)
To: Lars Schneider
Cc: Git Mailing List, peff, sbeller, Johannes.Schindelin, jnareb,
mlbright, tboegi, jacob.keller
In-Reply-To: <B138B703-08A6-45C4-9D54-80F87C75356B@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> On 13 Sep 2016, at 00:30, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> larsxschneider@gmail.com writes:
>>
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>
>>> packet_flush() would die in case of a write error even though for some
>>> callers an error would be acceptable. Add packet_flush_gently() which
>>> writes a pkt-line flush packet and returns `0` for success and `-1` for
>>> failure.
>>> ...
>>> +int packet_flush_gently(int fd)
>>> +{
>>> + packet_trace("0000", 4, 1);
>>> + if (write_in_full(fd, "0000", 4) == 4)
>>> + return 0;
>>> + error("flush packet write failed");
>>> + return -1;
>>
>> It is more idiomatic to do
>>
>> return error(...);
>>
>> but more importantly, does the caller even want an error message
>> unconditionally printed here?
>>
>> I suspect that it is a strong sign that the caller wants to be in
>> control of when and what error message is produced; otherwise it
>> wouldn't be calling the _gently() variant, no?
>
> Agreed!
I am also OK with the current form, too. Those who need to enhance
it to packet_flush_gently(int fd, int quiet) can come later.
^ permalink raw reply
* Re: [RFC/PATCH 01/17] diff: move line ending check into emit_hunk_header
From: Stefan Beller @ 2016-09-13 22:40 UTC (permalink / raw)
To: René Scharfe
Cc: Junio C Hamano, Jeff King, Christian Couder, git@vger.kernel.org
In-Reply-To: <848f36c9-4ac9-5316-1143-c47b48790847@web.de>
On Tue, Sep 13, 2016 at 7:42 AM, René Scharfe <l.s.r@web.de> wrote:
>>
>> strbuf_add(&msgbuf, line + len, org_len - len);
>> + if (line[org_len - 1] != '\n')
>> + strbuf_addch(&msgbuf, '\n');
>> +
>
>
> Using strbuf_complete_line() would be nicer.
That makes sense!
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Junio C Hamano @ 2016-09-13 22:34 UTC (permalink / raw)
To: Ben Peart; +Cc: git, pclouds, Ben Peart
In-Reply-To: <20160913142628.15440-1-benpeart@microsoft.com>
Ben Peart <peartben@gmail.com> writes:
> +static int needs_working_tree_merge(const struct checkout_opts *opts,
> + const struct branch_info *old,
> + const struct branch_info *new)
> +{
> +...
> +}
I do not think I need to repeat the same remarks on the conditions
in this helper, which hasn't changed since v2. Many "comments" in
the code do not explain why skipping is justified, or what they
claim to check looks to me just plain wrong.
For example, there is
/*
* If we're not creating a new branch, by definition we're changing
* the existing one so need to do the merge
*/
if (!opts->new_branch)
return 1;
but "git checkout" (no other argument) hits this condition. It
disables the most trivial optimization opportunity, because we are
not "creating".
"By definition, we're changing"? Really? Not quite.
If you disable this bogus check, "git checkout" (no other argument)
would be allowed to skip the merge_working_tree(), and that in turn
reveals another case that the helper is not checking when
unpack_trees() MUST be called.
Note: namely, when sparse checkout is in effect, switching from
HEAD to HEAD can nuke existing working tree files outside the
sparse pattern -- YUCK! See penultimate test in t1011 for
an example.
This yuckiness is not your fault, but needs_working_tree_merge()
logic you added needs to refrain from skipping unpack_trees() call
when sparse thing is in effect. I'd expect "git checkout -b foo"
instead of "git checkout" (no other argument) would fail to honor
the sparse thing and reveal this bug, because the above bogus
"!opts->new_branch" check will not protect you for that case.
In other words, these random series of "if (...) return 1" are bugs
hiding other real bugs and we need to reason about which ones are
bugs that are hiding what other bugs that are not covered by this
function. As Peff said earlier for v1, this is still an unreadable
mess. We need to figure out a way to make sure we are skipping on
the right condition and not accidentally hiding a bug of failing to
check the right condition. I offhand do not have a good suggestion
on this; sorry.
> static int merge_working_tree(const struct checkout_opts *opts,
> struct branch_info *old,
> struct branch_info *new,
> int *writeout_error)
> {
> + /*
> + * Optimize the performance of "git checkout -b foo" by avoiding
> + * the expensive merge, index and working directory updates if they
> + * are not needed.
> + */
> + if (!needs_working_tree_merge(opts, old, new))
> + return 0;
> +
> int ret;
> struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
With the change you made at the beginning of this function, it no
longer compiles with -Wdecl-after-stmt, but that is the smallest of
the problems.
It is a small step in the right direction to move the call to the
helper from the caller to this function, but it is a bit too small.
Notice that the lines after the above context look like this:
hold_locked_index(lock_file, 1);
if (read_cache_preload(NULL) < 0)
return error(_("index file corrupt"));
resolve_undo_clear();
if (opts->force) {
ret = reset_tree(new->commit->tree, opts, 1, writeout_error);
if (ret)
return ret;
} else {
struct tree_desc trees[2];
...
I would have expected that the check goes inside the "else" thing
that actually does a two-tree merge, and the helper loses the check
with opts->force, at least. That would still be a change smaller
than desired, but at least a meaningful improvement compared to the
previous one. As I have already pointed out, in the "else" clause
there is a check "is the index free of conflicted entries? if so
error out", and that must be honored in !opt->force case, no matter
what your needs_working_tree_merge() says. I also was hoping that
you would notice, when you were told about the unmerged check, by
reading the remainder of the merge_working_tree(), that we need to
call show_local_changes() when we are not doing force and when we
are not quiet---returning early like the above patch will never be
able to call that one downstream in the function.
Regardless of what the actual checks end up to be, the right place
to do this "optimization" would look more like:
builtin/checkout.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2b50a49..a6b9e17 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -508,14 +508,19 @@ static int merge_working_tree(const struct checkout_opts *opts,
topts.dir->flags |= DIR_SHOW_IGNORED;
setup_standard_excludes(topts.dir);
}
+
+ if ( we know we can skip the unpack ) {
+ ret = 0;
+ } else {
tree = parse_tree_indirect(old->commit ?
old->commit->object.oid.hash :
EMPTY_TREE_SHA1_BIN);
init_tree_desc(&trees[0], tree->buffer, tree->size);
tree = parse_tree_indirect(new->commit->object.oid.hash);
init_tree_desc(&trees[1], tree->buffer, tree->size);
-
ret = unpack_trees(2, trees, &topts);
+ }
+
if (ret == -1) {
/*
* Unpack couldn't do a trivial merge; either
I'd think. Note that the determination of "we can skip" would
involve knowing the object names of the two trees involved, so for
performance reasons, some of the parse-tree calls may have to come
before the call to "do we know we can skip?", but that does not
fundamentally change the basic code structure.
Thanks.
^ permalink raw reply related
* Re: [PATCH 06/16] diff: always try to set up the repository
From: Jeff King @ 2016-09-13 22:22 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Dennis Kaarsemaker,
Nguyễn Thái Ngọc Duy
In-Reply-To: <CAGZ79kYNfUmBG+9irQoZ+C6qfiiz0P7pGKHZVk-8569gk4+_EQ@mail.gmail.com>
On Tue, Sep 13, 2016 at 03:00:17PM -0700, Stefan Beller wrote:
> On Mon, Sep 12, 2016 at 8:23 PM, Jeff King <peff@peff.net> wrote:
>
> > 2. If you're in a subdirectory of a repository, then we
> > still try to read ".git/config", but it generally
> > doesn't exist. So "diff --no-index" there does not
> > respect repo config.
>
> Nit:
> So IIUC your cover letter even this /used/ to work but
> broke only recently? So I feel like the message is a bit
> misleading (i.e. you argue for a change in behavior instead of
> calling it a bug fix for a regression. I think a bug fix for a regression
> is harder to revert as compared to a "new" behavior)
No, this has always been broken. What broke recently is the tests
covered in patch 14 related to git-init.
IOW, we have always done this "blind read" of .git/config, ever since
the early days. It was always wrong, but was mostly overlooked because
it worked often enough and usually didn't cause other problems. But
because of the caching and lazy-reading done by git_config() these days
(and get_shared_repository() which builds on it), you can get quite
confusing and buggy effects if you lazy-read at the wrong time.
-Peff
^ permalink raw reply
* Re: [PATCH 16/16] init: reset cached config when entering new repo
From: Stefan Beller @ 2016-09-13 22:18 UTC (permalink / raw)
To: Jeff King
Cc: git@vger.kernel.org, Dennis Kaarsemaker,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20160913032422.xy22la6uw23hnvje@sigill.intra.peff.net>
I have reviewed all patches though I am no expert on
the init routines. They all look good except for the one
nit I noted for the commit message in patch 6.
With that, the whole series is:
Reviewed-by: Stefan Beller <sbeller@google.com>
Thanks!
^ permalink raw reply
* Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()
From: Lars Schneider @ 2016-09-13 22:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, peff, sbeller, Johannes.Schindelin, jnareb,
mlbright, tboegi, jacob.keller
In-Reply-To: <xmqqinu0y9v1.fsf@gitster.mtv.corp.google.com>
> On 13 Sep 2016, at 00:30, Junio C Hamano <gitster@pobox.com> wrote:
>
> larsxschneider@gmail.com writes:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> packet_flush() would die in case of a write error even though for some
>> callers an error would be acceptable. Add packet_flush_gently() which
>> writes a pkt-line flush packet and returns `0` for success and `-1` for
>> failure.
>> ...
>> +int packet_flush_gently(int fd)
>> +{
>> + packet_trace("0000", 4, 1);
>> + if (write_in_full(fd, "0000", 4) == 4)
>> + return 0;
>> + error("flush packet write failed");
>> + return -1;
>
> It is more idiomatic to do
>
> return error(...);
>
> but more importantly, does the caller even want an error message
> unconditionally printed here?
>
> I suspect that it is a strong sign that the caller wants to be in
> control of when and what error message is produced; otherwise it
> wouldn't be calling the _gently() variant, no?
Agreed!
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH v7 10/10] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-09-13 22:04 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Git Mailing List, Jeff King, Junio C Hamano, Stefan Beller,
Johannes.Schindelin, jnareb, mlbright, jacob.keller
In-Reply-To: <20160910164056.GA14646@tb-raspi>
> On 10 Sep 2016, at 17:40, Torsten Bögershausen <tboegi@web.de> wrote:
>
> []
>
> One general question up here, more comments inline.
> The current order for a clean-filter is like this, I removed the error handling:
>
> int convert_to_git()
> {
> ret |= apply_filter(path, src, len, -1, dst, filter);
> if (ret && dst) {
> src = dst->buf;
> len = dst->len;
> }
> ret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);
> return ret | ident_to_git(path, src, len, dst, ca.ident);
> }
>
> The first step is the clean filter, the CRLF-LF conversion (if needed),
> then ident.
> The current implementation streams the whole file content to the filter,
> (STDIN of the filter) and reads back STDOUT from the filter into a STRBUF.
> This is to use the UNIX-like STDIN--STDOUT method for writing a filter.
>
> However, int would_convert_to_git_filter_fd() and convert_to_git_filter_fd()
> offer a sort of short-cut:
> The filter reads from the file directly, and the output of the filter is
> read into a STRBUF.
Are you sure? As far as I understand the code the filter does not read from
the file in any case today. The functions would_convert_to_git_filter_fd() and
convert_to_git_filter_fd() just avoid avoid mapping the file in Git. The content
is still streamed via pipes:
https://github.com/git/git/commit/9035d75a2be9d80d82676504d69553245017f6d4
> It looks as if the multi-filter approach can use this in a similar way:
> Give the pathname to the filter, the filter opens the file for reading
> and stream the result via the pkt-line protocol into Git.
> This needs some more changes, and may be very well go into a separate patch
> series. (and should).
>
> What I am asking for:
> When a multi-filter is used, the content is handled to the filter via pkt-line,
> and the result is given to Git via pkt-line ?
> Nothing wrong with it, I just wonder, if it should be mentioned somewhere.
That is most certainly a good idea and the main reason I added "capabilities"
to the protocol. Joey Hess worked on this topic (not merged, yet) and I would
like to make this available to the long-running filter protocol as soon as the
feature is available:
http://public-inbox.org/git/1468277112-9909-1-git-send-email-joeyh@joeyh.name/
>> +sub packet_read {
>> + my $buffer;
>> + my $bytes_read = read STDIN, $buffer, 4;
>> + if ( $bytes_read == 0 ) {
>> +
>> + # EOF - Git stopped talking to us!
>> + exit();
>> + }
>> + elsif ( $bytes_read != 4 ) {
>> + die "invalid packet size '$bytes_read' field";
>> + }
>
> This is half-kosher, I would say,
> (And I really. really would like to see an implementation in C ;-)
Would you be willing to contribute a patch? :-)
> A read function may look like this:
>
> ret = read(0, &buffer, 4);
> if (ret < 0) {
> /* Error, nothing we can do */
> exit(1);
> } else if (ret == 0) {
> /* EOF */
> exit(0);
> } else if (ret < 4) {
> /*
> * Go and read more, until we have 4 bytes or EOF or Error */
> } else {
> /* Good case, see below */
> }
I see. However, my intention was to provide an absolute minimal
example to teach a reader how the protocol works. I consider
all proper error handling an exercise for the reader ;-)
>> +#define CAP_CLEAN (1u<<0)
>> +#define CAP_SMUDGE (1u<<1)
>
> Is CAP_ too generic, and GIT_FILTER_CAP (or so) less calling for trouble ?
I had something like that but Junio suggested these names in V4:
http://public-inbox.org/git/xmqq8twd8uld.fsf@gitster.mtv.corp.google.com/
>> +
>> + err = (strlen(filter_type) > PKTLINE_DATA_MAXLEN);
>
> Extra () needed ?
> More () in the code...
I thought it might improve readability, but I will remove them
if you think this would be more consistent with existing Git code.
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH v2 09/14] i18n: notes: mark error messages for translation
From: Junio C Hamano @ 2016-09-13 22:01 UTC (permalink / raw)
To: Jean-Noël AVILA
Cc: Vasco Almeida, git, Jiang Xin,
Ævar Arnfjörð Bjarmason
In-Reply-To: <2523910.zLOPm9a3mF@cayenne>
Jean-Noël AVILA <jn.avila@free.fr> writes:
>> Yes, these verbs are git notes subcommands....
>
> Having one sentence per action is cumbersome, but avoiding sentence lego is
> mandatory for proper i18n. How about just adding quotes around the subcommand
> and warn translators ?
I think that is a sensible way to go. I do not think it adds value
to "translate" the action names that needs to be typed verbatim in
the message.
^ permalink raw reply
* Re: [PATCH 06/16] diff: always try to set up the repository
From: Stefan Beller @ 2016-09-13 22:00 UTC (permalink / raw)
To: Jeff King
Cc: git@vger.kernel.org, Dennis Kaarsemaker,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20160913032336.cur3rrzpgepulqi6@sigill.intra.peff.net>
On Mon, Sep 12, 2016 at 8:23 PM, Jeff King <peff@peff.net> wrote:
> 2. If you're in a subdirectory of a repository, then we
> still try to read ".git/config", but it generally
> doesn't exist. So "diff --no-index" there does not
> respect repo config.
Nit:
So IIUC your cover letter even this /used/ to work but
broke only recently? So I feel like the message is a bit
misleading (i.e. you argue for a change in behavior instead of
calling it a bug fix for a regression. I think a bug fix for a regression
is harder to revert as compared to a "new" behavior)
I agree on the code, though.
^ permalink raw reply
* Left with empty files after "git stash pop" when system hung
From: Daniel Hahler @ 2016-09-13 21:39 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 1377 bytes --]
I have used "git stash --include-untracked", checked out another branch,
went back, and "git stash pop"ed the changes.
Then my system crashed/hung (music that was playing was repeated in a
loop). I have waited for some minutes, and then turned it off.
Afterwards, the repository in question was in a state where all files
contained in the stash were empty.
"git status" looked good on first sight: all the untracked and modified
files were listed there; but they were empty.
% git fsck --lost-found
error: object file .git/objects/04/1e659b5dbfd3f0be351a782b54743692875aec is empty
error: object file .git/objects/04/1e659b5dbfd3f0be351a782b54743692875aec is empty
fatal: loose object 041e659b5dbfd3f0be351a782b54743692875aec (stored in .git/objects/04/1e659b5dbfd3f0be351a782b54743692875aec) is corrupt
% find .git/objects -size 0|wc -l
12
I would have assumed that the "stash pop" operation would be "atomic",
i.e. it should not remove the stash object before other objects have
been written successfully.
The filesystem in question is ext4, and I am using Arch Linux.
I have removed all empty files in .git/objects and tried to find the
previous stash with `gitk --all $( git fsck | awk '{print $3}' )` then,
but it appears to have disappeared.
Please CC me in replies.
Cheers,
Daniel.
--
http://daniel.hahler.de/
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 163 bytes --]
^ permalink raw reply
* Re: [PATCH 01/16] t1007: factor out repeated setup
From: Stefan Beller @ 2016-09-13 21:42 UTC (permalink / raw)
To: Jeff King
Cc: git@vger.kernel.org, Dennis Kaarsemaker,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20160913032312.yheky2o25lmibvmb@sigill.intra.peff.net>
On Mon, Sep 12, 2016 at 8:23 PM, Jeff King <peff@peff.net> wrote:
> We have a series of 3 CRLF tests that do exactly the same
> (long) setup sequence. Let's pull it out into a common setup
> test, which is shorter, more efficient, and will make it
> easier to add new tests.
>
> Note that we don't have to worry about cleaning up any of
> the setup which was previously per-test; we call pop_repo
> after the CRLF tests, which cleans up everything.
>
> Signed-off-by: Jeff King <peff@peff.net>
This makes sense and looks good,
Thanks,
Reviewed-by: Stefan Beller <sbeller@google.com>
^ permalink raw reply
* Re: [ANNOUNCE] Git User's Survey 2016
From: Jakub Narębski @ 2016-09-13 20:36 UTC (permalink / raw)
To: Lars Schneider
Cc: David Bainbridge, git@vger.kernel.org, Doug Rathbone,
David Bainbridge, Stefan Beller, Andrew Ardill, Eric Wong
In-Reply-To: <0284BD61-8DCF-4EAD-8163-191C56AFD7C8@gmail.com>
On 13 September 2016 at 22:11, Lars Schneider <larsxschneider@gmail.com> wrote:
>> On 13 Sep 2016, at 17:54, Jakub Narębski <jnareb@gmail.com> wrote:
>> On 13 September 2016 at 18:15, David Bainbridge
>> <david.bainbridge@ericsson.com> wrote:
>>> Hi Jakub,
>>>
>>> You said:
>>> P.S. At request I can open a separate channel in survey, with
>>> a separate survey URL, so that responses from particular site
>>> or organization could be separated out.
>>>
>>> Please can you open a channel for use by Ericsson?
>>
>> Sent (privately to David).
>
> Could you send me a channel for Autodesk, too?
Here it is:
https://survs.com/survey/c51qiuw394
Please tell me if you want the channel anonymized in survey results
(after its closing).
--
Jakub Narębski
^ permalink raw reply
* Re: [RFC 0/3] http: avoid repeatedly adding curl easy to curlm
From: Junio C Hamano @ 2016-09-13 20:32 UTC (permalink / raw)
To: Eric Wong; +Cc: Yaroslav Halchenko, git, Jeff King
In-Reply-To: <20160913002557.10671-1-e@80x24.org>
Eric Wong <e@80x24.org> writes:
> The key patch here is 3/3 which seems like an obvious fix to
> adding the problem of adding a curl easy handle to a curl multi
> handle repeatedly.
Yeah, sounds like the right thing to do and 2/3 makes it really easy
to read the resulting code.
> I will investigate those failures in a week or two when I regain
> regular computer access.
Thanks. Will tentatively queue on 'pu' and wait for updates.
^ 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