* Re: [PATCH v3 04/14] i18n: blame: mark error messages for translation
From: Junio C Hamano @ 2016-09-15 20:14 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA
In-Reply-To: <1473951548-31733-4-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> @@ -2790,7 +2790,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> else {
> o = get_origin(&sb, sb.final, path);
> if (fill_blob_sha1_and_mode(o))
> - die("no such path %s in %s", path, final_commit_name);
> + die(_("no such path %s in %s"), path, final_commit_name);
This was missing in the earlier round, which is good to make it translated.
> - die("file %s has only %lu lines", path, lno);
> + die(Q_("file %s has only %lu line",
> + "file %s has only %lu lines",
> + lno), path, lno);
Looks good here, too. I would have moved "lno)," at the beginning
of the third line to the end of the second line to make it easier to
read, but this is OK.
^ permalink raw reply
* Re: [PATCH v7 10/10] convert: add filter.<driver>.process option
From: Junio C Hamano @ 2016-09-15 20:04 UTC (permalink / raw)
To: Lars Schneider
Cc: Torsten Bögershausen, Git Mailing List, peff, sbeller,
Johannes.Schindelin, jnareb, mlbright, jacob.keller
In-Reply-To: <5E675A29-628F-4076-A5D9-E359CCF04295@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
> Wouldn't that complicate the pathname parsing on the filter side?
> Can't we just define in our filter protocol documentation that our
> "pathname" packet _always_ has a trailing "\n"? That would mean the
> receiver would know a packet "pathname=ABC\n\n" encodes the path
> "ABC\n" [1].
That's fine, too. If you declare that pathname over the protocol is
a binary thing, you can also define that the packet does not have
the terminating \n, i.e. the example encodes the path "ABC\n\n",
which is also OK ;-)
As long as the rule is clearly documented, easy for filter
implementors to follow it, and hard for them to get it wrong, I'd be
perfectly happy.
Thanks.
^ permalink raw reply
* Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2
From: Junio C Hamano @ 2016-09-15 20:01 UTC (permalink / raw)
To: René Scharfe; +Cc: Jeff King, Git List, brian m. carlson
In-Reply-To: <79f6cfb5-3b9b-2ae7-d9a3-5c1c65c7d4cf@web.de>
René Scharfe <l.s.r@web.de> writes:
> Take this for example:
>
> - strbuf_addf(&o->obuf, _("(bad commit)\n"));
> + strbuf_addstr(&o->obuf, _("(bad commit)\n"));
>
> If there's a language that uses percent signs instead of parens or as
> regular letters, then they need to be escaped in the translated string
> before, but not after the patch. As I wrote: silly.
Ahh, OK, so "This use of addf only has format part and nothing else,
hence the format part can be taken as-is" which is the Coccinelle rule
used to produce this patch is incomplete and always needs manual
inspection, in case the format part wanted to give a literal % in
the output. E.g. it is a bug to convert this
strbuf_addf(&buf, _("this is 100%% wrong!"));
to
strbuf_addstr(&buf, _("this is 100%% wrong!"));
Thanks for clarification. Perhaps the strbuf.cocci rule file can
have some comment to warn the person who builds *.patch file to look
for % in E2, or something?
^ permalink raw reply
* Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2
From: René Scharfe @ 2016-09-15 19:55 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Git List, brian m. carlson
In-Reply-To: <20160915193804.d2mmmeard2rj6vye@sigill.intra.peff.net>
Am 15.09.2016 um 21:38 schrieb Jeff King:
> On Thu, Sep 15, 2016 at 12:25:43PM -0700, Junio C Hamano wrote:
>
>>>> Silly question: Is there a natural language that uses percent signs
>>>> as letters or e.g. instead of commas? :)
>>>
>>> I don't know, but if they do, they'd better get used to escaping them.
>>> :)
>>
>> I do not know either, but I am curious where that question comes
>> from. I stared at this patch for a few minutes but couldn't guess.
>
> My initial thought is that the next step after picking this low-hanging
> fruit would be to find cases where the strings do not contain "%", and
> thus we do not have to care about formatting. But a case like:
>
> strbuf_addf(&buf, "this does not have any percents!", foo);
>
> is simply broken (albeit in a way that we ignore foo, so it's just ugly
> code, not a real bug).
>
> So I dunno. I too am curious.
Take this for example:
- strbuf_addf(&o->obuf, _("(bad commit)\n"));
+ strbuf_addstr(&o->obuf, _("(bad commit)\n"));
If there's a language that uses percent signs instead of parens or as
regular letters, then they need to be escaped in the translated string
before, but not after the patch. As I wrote: silly.
René
^ permalink raw reply
* Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()
From: Jeff King @ 2016-09-15 19:44 UTC (permalink / raw)
To: Lars Schneider
Cc: Junio C Hamano, Git Mailing List, sbeller, Johannes.Schindelin,
jnareb, mlbright, tboegi, jacob.keller
In-Reply-To: <385016D3-8298-4273-81E7-876CB25B2789@gmail.com>
On Thu, Sep 15, 2016 at 05:42:58PM +0100, Lars Schneider wrote:
> >>>> +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;
> [...]
> >>> 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.
>
> "caller wants to be in control [...] otherwise it wouldn't be calling
> the _gently() variant" convinced me. I would like to change it like
> this:
>
> trace_printf_key(&trace_packet, "flush packet write failed");
> return -1;
>
> Objections?
I'm not sure that a trace makes sense, because it means that 99% of the
time we are silent. AFAICT, the question is not "sometimes the user
needs to see an error and sometimes not, and they should decide before
starting the program". It is "sometimes the caller will report the error
to the user as appropriate, and sometimes we need to do so". And only
the calling code knows which is which.
So the "right" pattern is either:
1. Return -1 and the caller is responsible for telling the user.
or
2. Return -1 and stuff the error into an error strbuf, so it can be
passed up the call chain easily (and callers do not have to come up
with their own wording).
But if all current callers would just call error() themselves anyway,
then it's OK to punt on this and let somebody else handle it later if
they add a new caller who wants different behavior (and that is what
Junio was saying above, I think).
-Peff
^ permalink raw reply
* Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2
From: Jeff King @ 2016-09-15 19:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Git List, brian m. carlson
In-Reply-To: <xmqqbmzpnex4.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 15, 2016 at 12:25:43PM -0700, Junio C Hamano wrote:
> >> Silly question: Is there a natural language that uses percent signs
> >> as letters or e.g. instead of commas? :)
> >
> > I don't know, but if they do, they'd better get used to escaping them.
> > :)
>
> I do not know either, but I am curious where that question comes
> from. I stared at this patch for a few minutes but couldn't guess.
My initial thought is that the next step after picking this low-hanging
fruit would be to find cases where the strings do not contain "%", and
thus we do not have to care about formatting. But a case like:
strbuf_addf(&buf, "this does not have any percents!", foo);
is simply broken (albeit in a way that we ignore foo, so it's just ugly
code, not a real bug).
So I dunno. I too am curious.
-Peff
^ permalink raw reply
* Re: [PATCH v7 10/10] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-09-15 17:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Torsten Bögershausen, Git Mailing List, peff, sbeller,
Johannes.Schindelin, jnareb, mlbright, jacob.keller
In-Reply-To: <xmqqvaxzvjj0.fsf@gitster.mtv.corp.google.com>
> On 13 Sep 2016, at 17:42, Junio C Hamano <gitster@pobox.com> wrote:
>
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> I would really consider to treat pathnames as binary, and not add a trailing '\n',
>> are there other opinions ?
>
> It would be the most consistent if the same format as
> write_name_quoted() is used for this, I would think.
Is that the solution you had in mind?
quote_c_style(path, "ed_path, NULL, 0);
err = packet_write_fmt_gently(process->in, "pathname=%s\n", quoted_path.buf);
Wouldn't that complicate the pathname parsing on the filter side?
Can't we just define in our filter protocol documentation that our
"pathname" packet _always_ has a trailing "\n"? That would mean the
receiver would know a packet "pathname=ABC\n\n" encodes the path
"ABC\n" [1].
Thanks,
Lars
[1] Following Torsten's example in
http://public-inbox.org/git/96554f6d-988d-e0b8-7936-8d0f29a7564f@web.de )
^ permalink raw reply
* Re: [PATCH] pkt-line: mark a file-local symbol static
From: Lars Schneider @ 2016-09-15 16:46 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <6373d68b-574d-59f3-7b8d-60dd3a673806@ramsayjones.plus.com>
> On 14 Sep 2016, at 14:31, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Lars,
>
> If you need to re-roll your 'ls/filter-process' branch, could you
> please squash this into the relevant patch; commit 2afd9b22
> ("pkt-line: add packet_write_gently()", 08-09-2016).
>
> [...]
> -int packet_write_gently(const int fd_out, const char *buf, size_t size)
> +static int packet_write_gently(const int fd_out, const char *buf, size_t size)
> {
> static char packet_write_buffer[LARGE_PACKET_MAX];
Done!
Thank you,
Lars
^ permalink raw reply
* Re: [PATCH v7 04/10] pkt-line: add packet_flush_gently()
From: Lars Schneider @ 2016-09-15 16:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, peff, sbeller, Johannes.Schindelin, jnareb,
mlbright, tboegi, jacob.keller
In-Reply-To: <xmqq37l3v2rk.fsf@gitster.mtv.corp.google.com>
> On 13 Sep 2016, at 23:44, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
"caller wants to be in control [...] otherwise it wouldn't be calling
the _gently() variant" convinced me. I would like to change it like
this:
trace_printf_key(&trace_packet, "flush packet write failed");
return -1;
Objections?
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2
From: Junio C Hamano @ 2016-09-15 19:25 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Git List, brian m. carlson
In-Reply-To: <20160915184448.awipvg2kmlq7weei@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Sep 15, 2016 at 08:31:00PM +0200, René Scharfe wrote:
>
>> Replace uses of strbuf_addf() for adding strings with more lightweight
>> strbuf_addstr() calls. This makes the intent clearer and avoids
>> potential issues with printf format specifiers.
>>
>> 02962d36845b89145cd69f8bc65e015d78ae3434 already converted six cases,
>> this patch covers eleven more.
>
> Great, these all look obviously correct.
Yes.
>> Silly question: Is there a natural language that uses percent signs
>> as letters or e.g. instead of commas? :)
>
> I don't know, but if they do, they'd better get used to escaping them.
> :)
I do not know either, but I am curious where that question comes
from. I stared at this patch for a few minutes but couldn't guess.
^ permalink raw reply
* Re: [PATCH v2 21/25] sequencer: refactor write_message()
From: Junio C Hamano @ 2016-09-15 19:21 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Jakub Narębski
In-Reply-To: <e21e3391-bfc3-68b8-8eaf-0e15e5a436c5@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 11.09.2016 um 12:55 schrieb Johannes Schindelin:
>> -static int write_message(struct strbuf *msgbuf, const char *filename)
>> +static int write_with_lock_file(const char *filename,
>> + const void *buf, size_t len, int append_eol)
>> {
>> static struct lock_file msg_file;
>>
>> int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
>> if (msg_fd < 0)
>> return error_errno(_("Could not lock '%s'"), filename);
>> - if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
>> - return error_errno(_("Could not write to %s"), filename);
>> - strbuf_release(msgbuf);
>> + if (write_in_full(msg_fd, buf, len) < 0)
>> + return error_errno(_("Could not write to '%s'"), filename);
>> + if (append_eol && write(msg_fd, "\n", 1) < 0)
>> + return error_errno(_("Could not write eol to '%s"), filename);
>> if (commit_lock_file(&msg_file) < 0)
>> return error(_("Error wrapping up %s."), filename);
>>
>> return 0;
>> }
>
> The two error paths in the added lines should both
>
> rollback_lock_file(&msg_file);
>
> , I think. But I do notice that this is not exactly new, so...
It may not be new for this step, but overall the series is aiming to
libify the stuff, so we should fix fd and lockfile leaks like this
as we notice them.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 10/25] sequencer: get rid of the subcommand field
From: Junio C Hamano @ 2016-09-15 19:15 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <955727a7cf5bd64f1c1106b394095d5449a4a2dc.1473590966.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The subcommands are used exactly once, at the very beginning of
> sequencer_pick_revisions(), to determine what to do. This is an
> unnecessary level of indirection: we can simply call the correct
> function to begin with. So let's do that.
Makes sense. And the diffstat is also pleasant to the eyes.
> builtin/revert.c | 36 ++++++++++++++++--------------------
> sequencer.c | 35 +++++++++++------------------------
> sequencer.h | 13 ++++---------
> 3 files changed, 31 insertions(+), 53 deletions(-)
^ permalink raw reply
* Re: Tracking down a segfault in delta_base_cache
From: Jeff King @ 2016-09-15 18:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathon Mah, git
In-Reply-To: <xmqqa8f9oymk.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 15, 2016 at 10:34:43AM -0700, Junio C Hamano wrote:
> Jonathon Mah <me@jonathonmah.com> writes:
>
> >> On 2016-09-14, at 17:56, Jeff King <peff@peff.net> wrote:
> >>
> >> Have you tried with the patch in:
> >>
> >> http://public-inbox.org/git/20160912164616.vg33kldazuthff3d@sigill.intra.peff.net/
> > All the examples I've tried work when I use that. Thanks!
>
> Peff, thanks for a quick suggestion and Jonathon, thanks for a quick
> confirmation.
Better still would have been for me not to introduce the segfault in the
first place. ;)
-Peff
^ permalink raw reply
* Re: [PATCH] object: measure time needed for resolving hash collisions
From: Jeff King @ 2016-09-15 18:49 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kZ-rLVijKj31uYgYrLkSCCOaLKWrr8u=6LXnd=gmma+xQ@mail.gmail.com>
On Thu, Sep 15, 2016 at 09:26:22AM -0700, Stefan Beller wrote:
> > It may also be possible to really micro-optimize it on some platforms,
> > because we know the size in advance (I'd kind of expect the compiler to
> > do that, but if we're ending up in glibc memcmp then it sounds like it
> > is not the case).
>
> That stackoverflow link suggests that glibc already has microoptimisations
> for a variety of platforms.
It's definitely micro-optimized in glibc. What I was trying to say is
that if we are hitting the glibc implementation, then we know we are
handling the "20" at runtime. Whereas the compiler should know that "20"
is a constant, and could in theory skip the memcmp() call entirely in
favor of something like an unrolled loop.
-Peff
^ permalink raw reply
* Re: [PATCH v4 3/4] read-cache: introduce chmod_index_entry
From: Thomas Gummerer @ 2016-09-15 18:49 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Jan Keromnes,
Ingo Brückl, Edward Thomson
In-Reply-To: <xmqqsht2ozwx.fsf@gitster.mtv.corp.google.com>
On 09/14, Junio C Hamano wrote:
> I've queued this trivial SQUASH??? on top, which I think should be
> squashed into 3/4.
Yeah, I missed this. The SQUASH??? definitely makes sense, would be
great if you could just squash that in.
> Thanks.
Thanks the reviews and helping me getting the series in a good shape!
>
>
> read-cache.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 2445e30..c2b2e97 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -779,7 +779,7 @@ int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
> default:
> return -2;
> }
> - cache_tree_invalidate_path(&the_index, ce->name);
> + cache_tree_invalidate_path(istate, ce->name);
> ce->ce_flags |= CE_UPDATE_IN_BASE;
> istate->cache_changed |= CE_ENTRY_CHANGED;
>
> --
> 2.10.0-458-g8cce42d
>
--
Thomas
^ permalink raw reply
* Re: [PATCH] object: measure time needed for resolving hash collisions
From: Jeff King @ 2016-09-15 18:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git
In-Reply-To: <xmqq60pxoy4h.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 15, 2016 at 10:45:34AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Measuring _just_ the collisions is more like the patch below. In my
> > measurements it's more like 30ms, compared to 10s for all of the
> > hashcmps.
> >
> > So we really aren't dealing with collisions, but rather just verifying
> > that our hash landed at the right spot. And _any_ data structure is
> > going to have to do that.
>
> The reverse side of the coin may be if we can shrink the hashtable
> smaller and load it more heavily without sacrificing performance by
> making the necessary "have we landed at the right spot" check cheap
> enough, I guess.
I think that's where things like cuckoo hashing come into play. They
didn't have any effect for us because we already keep the table very
unloaded. But you could _probably_ increase the load factor without
sacrificing performance using a more clever scheme.
It's not clear to me that the current table size is a big problem,
though. It might be hurting us with cache effects, but I think the only
way we'd know is to measure.
-Peff
^ permalink raw reply
* Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2
From: Jeff King @ 2016-09-15 18:44 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano, brian m. carlson
In-Reply-To: <f7294ac5-8302-03fb-d756-81a1c029a813@web.de>
On Thu, Sep 15, 2016 at 08:31:00PM +0200, René Scharfe wrote:
> Replace uses of strbuf_addf() for adding strings with more lightweight
> strbuf_addstr() calls. This makes the intent clearer and avoids
> potential issues with printf format specifiers.
>
> 02962d36845b89145cd69f8bc65e015d78ae3434 already converted six cases,
> this patch covers eleven more.
Great, these all look obviously correct.
> A semantic patch for Coccinelle is included for easier checking for
> new cases that might be introduced in the future.
I think there was some discussion in brian's object_id patches about
whether we wanted to carry Coccinelle transformations in the tree, but I
don't remember the outcome. I don't have an opinion myself.
> Silly question: Is there a natural language that uses percent signs
> as letters or e.g. instead of commas? :)
I don't know, but if they do, they'd better get used to escaping them.
:)
-Peff
^ permalink raw reply
* [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2
From: René Scharfe @ 2016-09-15 18:31 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, brian m. carlson, Jeff King
Replace uses of strbuf_addf() for adding strings with more lightweight
strbuf_addstr() calls. This makes the intent clearer and avoids
potential issues with printf format specifiers.
02962d36845b89145cd69f8bc65e015d78ae3434 already converted six cases,
this patch covers eleven more.
A semantic patch for Coccinelle is included for easier checking for
new cases that might be introduced in the future.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Silly question: Is there a natural language that uses percent signs
as letters or e.g. instead of commas? :)
builtin/fmt-merge-msg.c | 2 +-
builtin/merge.c | 2 +-
builtin/submodule--helper.c | 5 +++--
contrib/coccinelle/strbuf.cocci | 5 +++++
merge-recursive.c | 2 +-
remote.c | 8 ++++----
wt-status.c | 6 +++---
7 files changed, 18 insertions(+), 12 deletions(-)
create mode 100644 contrib/coccinelle/strbuf.cocci
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index ac84e99..dc2e9e4 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -395,7 +395,7 @@ static void shortlog(const char *name,
for (i = 0; i < subjects.nr; i++)
if (i >= limit)
- strbuf_addf(out, " ...\n");
+ strbuf_addstr(out, " ...\n");
else
strbuf_addf(out, " %s\n", subjects.items[i].string);
diff --git a/builtin/merge.c b/builtin/merge.c
index 0ae099f..a8b57c7 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -940,7 +940,7 @@ static void write_merge_state(struct commit_list *remoteheads)
strbuf_reset(&buf);
if (fast_forward == FF_NO)
- strbuf_addf(&buf, "no-ff");
+ strbuf_addstr(&buf, "no-ff");
write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
}
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9d79f19..ad23155 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -859,8 +859,9 @@ static int update_clone_get_next_task(struct child_process *child,
ce = suc->failed_clones[index];
if (!prepare_to_clone_next_submodule(ce, child, suc, err)) {
suc->current ++;
- strbuf_addf(err, "BUG: submodule considered for cloning,"
- "doesn't need cloning any more?\n");
+ strbuf_addstr(err, "BUG: submodule considered for "
+ "cloning, doesn't need cloning "
+ "any more?\n");
return 0;
}
p = xmalloc(sizeof(*p));
diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
new file mode 100644
index 0000000..7932d48
--- /dev/null
+++ b/contrib/coccinelle/strbuf.cocci
@@ -0,0 +1,5 @@
+@@
+expression E1, E2;
+@@
+- strbuf_addf(E1, E2);
++ strbuf_addstr(E1, E2);
diff --git a/merge-recursive.c b/merge-recursive.c
index e349126..d2b191b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -206,7 +206,7 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)
find_unique_abbrev(commit->object.oid.hash,
DEFAULT_ABBREV));
if (parse_commit(commit) != 0)
- strbuf_addf(&o->obuf, _("(bad commit)\n"));
+ strbuf_addstr(&o->obuf, _("(bad commit)\n"));
else {
const char *title;
const char *msg = get_commit_buffer(commit, NULL);
diff --git a/remote.c b/remote.c
index d29850a..ad6c542 100644
--- a/remote.c
+++ b/remote.c
@@ -2073,7 +2073,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
_("Your branch is based on '%s', but the upstream is gone.\n"),
base);
if (advice_status_hints)
- strbuf_addf(sb,
+ strbuf_addstr(sb,
_(" (use \"git branch --unset-upstream\" to fixup)\n"));
} else if (!ours && !theirs) {
strbuf_addf(sb,
@@ -2086,7 +2086,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
ours),
base, ours);
if (advice_status_hints)
- strbuf_addf(sb,
+ strbuf_addstr(sb,
_(" (use \"git push\" to publish your local commits)\n"));
} else if (!ours) {
strbuf_addf(sb,
@@ -2097,7 +2097,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
theirs),
base, theirs);
if (advice_status_hints)
- strbuf_addf(sb,
+ strbuf_addstr(sb,
_(" (use \"git pull\" to update your local branch)\n"));
} else {
strbuf_addf(sb,
@@ -2110,7 +2110,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
ours + theirs),
base, ours, theirs);
if (advice_status_hints)
- strbuf_addf(sb,
+ strbuf_addstr(sb,
_(" (use \"git pull\" to merge the remote branch into yours)\n"));
}
free(base);
diff --git a/wt-status.c b/wt-status.c
index 539aac1..f928f0f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -367,11 +367,11 @@ static void wt_longstatus_print_change_data(struct wt_status *s,
if (d->new_submodule_commits || d->dirty_submodule) {
strbuf_addstr(&extra, " (");
if (d->new_submodule_commits)
- strbuf_addf(&extra, _("new commits, "));
+ strbuf_addstr(&extra, _("new commits, "));
if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
- strbuf_addf(&extra, _("modified content, "));
+ strbuf_addstr(&extra, _("modified content, "));
if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
- strbuf_addf(&extra, _("untracked content, "));
+ strbuf_addstr(&extra, _("untracked content, "));
strbuf_setlen(&extra, extra.len - 2);
strbuf_addch(&extra, ')');
}
--
2.10.0
^ permalink raw reply related
* [PATCH] add coccicheck make target
From: René Scharfe @ 2016-09-15 18:30 UTC (permalink / raw)
To: brian m. carlson, Git List; +Cc: Junio C Hamano
Provide a simple way to run Coccinelle against all source files, in the
form of a Makefile target. Running "make coccicheck" applies each
.cocci file in contrib/coccinelle/ on all source files. It generates
a .patch file for each .cocci file, containing the actual changes for
effecting the transformations described by the semantic patches.
Non-empty .patch files are reported. They can be applied to the work
tree using "patch -p0", but should be checked to e.g. make sure they
don't screw up formatting or create circular references.
Coccinelle's diagnostic output (stderr) is piped into .log files.
Linux has a much more elaborate make target of the same name; let's
start nice and easy.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Makefile | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/Makefile b/Makefile
index 7f18492..74b2788 100644
--- a/Makefile
+++ b/Makefile
@@ -461,6 +461,7 @@ CURL_CONFIG = curl-config
PTHREAD_LIBS = -lpthread
PTHREAD_CFLAGS =
GCOV = gcov
+SPATCH = spatch
export TCL_PATH TCLTK_PATH
@@ -2307,6 +2308,18 @@ check: common-cmds.h
exit 1; \
fi
+C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))
+%.cocci.patch: %.cocci $(C_SOURCES)
+ @echo ' ' SPATCH $<; \
+ for f in $(C_SOURCES); do \
+ $(SPATCH) --sp-file $< $$f; \
+ done >$@ 2>$@.log; \
+ if test -s $@; \
+ then \
+ echo ' ' SPATCH result: $@; \
+ fi
+coccicheck: $(patsubst %.cocci,%.cocci.patch,$(wildcard contrib/coccinelle/*.cocci))
+
### Installation rules
ifneq ($(filter /%,$(firstword $(template_dir))),)
@@ -2498,6 +2511,7 @@ clean: profile-clean coverage-clean
$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
+ $(RM) contrib/coccinelle/*.cocci.patch*
$(MAKE) -C Documentation/ clean
ifndef NO_PERL
$(MAKE) -C gitweb clean
--
2.10.0
^ permalink raw reply related
* [PATCH] contrib/coccinelle: fix semantic patch for oid_to_hex_r()
From: René Scharfe @ 2016-09-15 18:30 UTC (permalink / raw)
To: brian m. carlson, Git List; +Cc: Junio C Hamano
Both sha1_to_hex_r() and oid_to_hex_r() take two parameters, so use two
expressions in the semantic patch for transforming calls of the former
to the latter one.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
contrib/coccinelle/object_id.cocci | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci
index 8ccdbb5..0307624 100644
--- a/contrib/coccinelle/object_id.cocci
+++ b/contrib/coccinelle/object_id.cocci
@@ -23,16 +23,16 @@ expression E1;
+ oid_to_hex(E1)
@@
-expression E1;
+expression E1, E2;
@@
-- sha1_to_hex_r(E1.hash)
-+ oid_to_hex_r(&E1)
+- sha1_to_hex_r(E1, E2.hash)
++ oid_to_hex_r(E1, &E2)
@@
-expression E1;
+expression E1, E2;
@@
-- sha1_to_hex_r(E1->hash)
-+ oid_to_hex_r(E1)
+- sha1_to_hex_r(E1, E2->hash)
++ oid_to_hex_r(E1, E2)
@@
expression E1;
--
2.10.0
^ permalink raw reply related
* Re: [wishlist?] make submodule commands robust to having non-submodule Subprojects
From: Junio C Hamano @ 2016-09-15 18:29 UTC (permalink / raw)
To: Yaroslav Halchenko; +Cc: git@vger.kernel.org
In-Reply-To: <20160915181254.GN9830@onerussian.com>
Yaroslav Halchenko <yoh@onerussian.com> writes:
> do you foresee any unpleasant side-effects from above manual editing
> .gitmodules/submodule update --init ?
I do not think so; you essentially did what a canned command we
should have had should have done by hand because of a lack of such a
feature.
^ permalink raw reply
* Re: [wishlist?] make submodule commands robust to having non-submodule Subprojects
From: Junio C Hamano @ 2016-09-15 18:27 UTC (permalink / raw)
To: Stefan Beller; +Cc: Yaroslav Halchenko, git@vger.kernel.org
In-Reply-To: <CAGZ79kbYEqbr0h_0U=Ed-NRrsjb4XmznL8C45gTyJCVx+X-LTQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> When searching around the net, some people use half
> initialized submodules intentionally,...
>
> Not sure I agree with such a setup, but people use it.
In such a top-level project, people would not use "git submodule"
command, would they? I do not think anybody in this thread was
pushing to forbid such a use, and it may be perfectly fine if "git
submodule" does not work for such a gitlink; after all such a
subdirectory is not even meant to be a submodule.
> So how about this fictional work flow:
>
> $ git init top
> $ cd top
> $ git commit --allow-empty -m 'initial in top'
> $ git init sub
> $ git -C sub commit --allow-empty -m 'initial in sub'
> $ git add sub
> You added a gitlink, but no corresponding entry in
> .gitmodules is found. This is fine for gits core functionality, but
> the submodule command gets confused by this unless you add 'sub'
> to your .gitmodules via `git submodule add --already-in-tree \
> --reuse-submodules-origin-as-URL sub`. Alternatively you can make this
> message disappear by configuring advice.gitlinkPitfalls.
I am not sure if I agree with that direction.
If the trend in Git community collectively these days is to make
usage of submodules easier and smoother, I'd imagine that you would
want to teach "git add" that was given a submodule to "git submodule
add" instead by default, with an option "git add --no-gitmodules
sub" to disable it, or something like that.
> $ git submodule add --fixup-modules-file ./sub sub
> Adding .gitmodule entry only for `sub` to use `git -C remote
> show origin` as URL.
I agree that a feature like this is needed regardless of what
happens at "git add" time.
^ permalink raw reply
* Re: [wishlist?] make submodule commands robust to having non-submodule Subprojects
From: Stefan Beller @ 2016-09-15 18:16 UTC (permalink / raw)
To: Yaroslav Halchenko; +Cc: git@vger.kernel.org
In-Reply-To: <20160915181254.GN9830@onerussian.com>
On Thu, Sep 15, 2016 at 11:12 AM, Yaroslav Halchenko <yoh@onerussian.com> wrote:
>
> do you foresee any unpleasant side-effects from above manual editing
> .gitmodules/submodule update --init ?
I think that is fine, but un(der)documented. So you have to figure it out
from experience what to do exactly.
^ permalink raw reply
* Re: [wishlist?] make submodule commands robust to having non-submodule Subprojects
From: Stefan Beller @ 2016-09-15 18:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Yaroslav Halchenko, git@vger.kernel.org
In-Reply-To: <xmqqwpidniry.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 15, 2016 at 11:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I think that is a more pressing thing to address. Once we make it
> easier for the user to bring a half-initialized submodule properly
> into the world view of the submodule subsystem, we would have to
> worry about the reported failure case even less and you do not need
> to pile on workaround options to let things continue in a state that
> is half-broken (that is, in a state that is perfectly sane to the
> core layer, but is not liked by the submodule layer).
Heh, I see.
When searching around the net, some people use half
initialized submodules intentionally, e.g. I'll store some private keys in
sub and I publish the superproject asking for collaborators to my new shiny
webbased thing. The submodule containing the private keys never leaves
my hard drive, hence no .gitmodules entry is necessary.
Not sure I agree with such a setup, but people use it.
So how about this fictional work flow:
$ git init top
$ cd top
$ git commit --allow-empty -m 'initial in top'
$ git init sub
$ git -C sub commit --allow-empty -m 'initial in sub'
$ git add sub
You added a gitlink, but no corresponding entry in
.gitmodules is found. This is fine for gits core functionality, but
the submodule command gets confused by this unless you add 'sub'
to your .gitmodules via `git submodule add --already-in-tree \
--reuse-submodules-origin-as-URL sub`. Alternatively you can make this
message disappear by configuring advice.gitlinkPitfalls.
$ git submodule
... similar advice goes here...
fatal: no submodule mapping found in .gitmodules for path 'sub'
$ git submodule add --fixup-modules-file ./sub sub
Adding .gitmodule entry only for `sub` to use `git -C remote
show origin` as URL.
# user is happy now.
^ permalink raw reply
* Re: [wishlist?] make submodule commands robust to having non-submodule Subprojects
From: Yaroslav Halchenko @ 2016-09-15 18:12 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <xmqqwpidniry.fsf@gitster.mtv.corp.google.com>
On Thu, 15 Sep 2016, Junio C Hamano wrote:
> >> which then stops without even looking at other submodules.
> >> I think it would be more logical to make it a 'warning:' not a 'fatal:' and
> >> proceed.
> Making "git submodule" listing to continue from that point may be
> one thing, but do we have a sensible way in "git submodule add" to
> allow the user to recover from this condition? That is, "git add"
> is a right way to tell the core level that there is a gitlink, but
> as Yaroslav correctly observed in the early part of his message,
> having that gitlink alone is not good enough for the world view of
> "git submodule" that sits at higher-layer. And the usual way to
> tell the submodule layer that there is a submodule is with "git
> submodule add", but
> $ git init top
> $ cd top
> $ git commit --allow-empty -m 'initial in top'
> $ git init sub
> $ git -C sub commit --allow-empty -m 'initial in sub'
> $ git add sub
> $ git submodule
> fatal: no submodule mapping found in .gitmodules for path 'sub'
> $ git submodule add ./sub sub
> 'sub' already exists in the index
> $ git submodule add -f ./sub sub
> 'sub' already exists in the index
FWIW
I could have sworn that I have tried to 'submodule add' it and it
worked... but pragmatically I just did edit .gitmodules, added the
record for it, committed it, and then iirc git submodule update --init
which seemed to make git happy... FTR:
$> git submodule add ./sub ./sub
'sub' already exists in the index
$> git submodule add ./sub/ ./sub/
'sub' already exists in the index
$> vim .gitmodules
$> git add .gitmodules
cached/staged changes:
.gitmodules | 4 ++++
sub | 1 +
$> git submodule update --init
Submodule 'sub' (/tmp/111/top/sub) registered for path 'sub'
cached/staged changes:
.gitmodules | 4 ++++
sub | 1 +
$> git commit -m 'added finally'
[master aa6d912] added finally
2 files changed, 5 insertions(+)
create mode 100644 .gitmodules
create mode 160000 sub
$> git submodule
6f574b298ef51aebd36daafad450a3e38802ca03 sub (heads/master)
> I highly suspect that the user will then get stuck at this point,
> after trying to "submodule add" and then even attempting to force
> it.
> I think that is a more pressing thing to address. Once we make it
> easier for the user to bring a half-initialized submodule properly
> into the world view of the submodule subsystem, we would have to
> worry about the reported failure case even less and you do not need
> to pile on workaround options to let things continue in a state that
> is half-broken (that is, in a state that is perfectly sane to the
> core layer, but is not liked by the submodule layer).
do you foresee any unpleasant side-effects from above manual editing
.gitmodules/submodule update --init ?
--
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik
^ 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