Re: [PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning
On Thu, Aug 04, 2016 at 11:24:32PM -0700, Junio C Hamano wrote: > I do not know if we want to worry about st_add(1, strlen(s1)) > overflow around here, though. > [...] > + size_t len = strlen(s1) + 1; I wondered that, too, but I don't think it's possible. To overflow the size_t with "+1", strlen() must return the maximum value that it can hold. But such a string would need one more byte than that, for its trailing NUL. So assuming you cannot have a string that exceeds size_t in the first place, I think it is impossible to overflow here. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)
On Thu, Aug 04, 2016 at 11:34:35PM +, Eric Wong wrote: > Junio C Hamano wrote: > > [Graduated to "master"] > > > * ew/http-walker (2016-07-18) 4 commits > > (merged to 'next' on 2016-07-18 at a430a97) > > + list: avoid incompatibility with *BSD sys/queue.h > > (merged to 'next' on 2016-07-13 at 8585c03) > > + http-walker: reduce O(n) ops with doubly-linked list > > Yay! This finally introduces the Linux kernel linked list > into git. I'm not sure if it's worth the effort to introduce > cleanup commits to start using it in places where we already > have doubly-linked list implementations: > > (+Cc Nicolas and Lukas) > * sha1_file.c delta_base_cache_lru is open codes this > * builtin/pack-redundant.c could probably be adapted, too > ... any more? I just introduced another doubly-linked list in [1]. It adds some MRU features on top of the list, but it could in theory be built on top of a generic doubly-linked list. It's also possible the delta-base-cache stuff could build on top of that same code, but I didn't look closely at it. -Peff [1] http://public-inbox.org/git/20160729040659.gc22...@sigill.intra.peff.net/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] trace: disable key after write error
On Thu, Aug 04, 2016 at 05:22:44PM -0400, Jeff King wrote:
> On Thu, Aug 04, 2016 at 01:45:11PM -0700, Junio C Hamano wrote:
>
> > Jeff King writes:
> >
> > > If we get a write error writing to a trace descriptor, the
> > > error isn't likely to go away if we keep writing. Instead,
> > > you'll just get the same error over and over. E.g., try:
> > >
> > > GIT_TRACE_PACKET=42 git ls-remote >/dev/null
> > >
> > > You don't really need to see:
> > >
> > > warning: unable to write trace for GIT_TRACE_PACKET: Bad file descriptor
> > >
> > > hundreds of times. We could fallback to tracing to stderr,
> > > as we do in the error code-path for open(), but there's not
> > > much point. If the user fed us a bogus descriptor, they're
> > > probably better off fixing their invocation. And if they
> > > didn't, and we saw a transient error (e.g., ENOSPC writing
> > > to a file), it probably doesn't help anybody to have half of
> > > the trace in a file, and half on stderr.
> >
> > Yes, I think I like this better than "we cannot open the named file,
> > so let's trace into standard error stream" that is done in the code
> > in the context of [3/7]. We should do the same over there.
>
> Yeah, I was tempted to strip that out, too. I'll look into preparing a
> patch on top.
Here's a patch that can go on the tip of jk/trace-fixup.
-- >8 --
Subject: [PATCH] trace: do not fall back to stderr
If the trace code cannot open a specified file, or does not
understand the contents of the GIT_TRACE variable, it falls
back to printing trace output to stderr.
This is an attempt to be helpful, but in practice it just
ends up annoying. The user was trying to get the output to
go somewhere else, so spewing it to stderr does not really
accomplish that. And as it's intended for debugging, they
can presumably re-run the command with their error
corrected.
So instead of falling back, this patch disables bogus trace
keys for the rest of the program, just as we do for write
errors. We can drop the "Defaulting to..." part of the error
message entirely; after seeing "cannot open '/foo'", the
user can assume that tracing is skipped.
Signed-off-by: Jeff King
---
trace.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/trace.c b/trace.c
index 083eb98..7508aea 100644
--- a/trace.c
+++ b/trace.c
@@ -61,10 +61,9 @@ static int get_trace_fd(struct trace_key *key)
else if (is_absolute_path(trace)) {
int fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);
if (fd == -1) {
- warning("could not open '%s' for tracing: %s\n"
- " Defaulting to tracing on stderr...",
+ warning("could not open '%s' for tracing: %s",
trace, strerror(errno));
- key->fd = STDERR_FILENO;
+ trace_disable(key);
} else {
key->fd = fd;
key->need_close = 1;
@@ -72,10 +71,9 @@ static int get_trace_fd(struct trace_key *key)
} else {
warning("unknown trace value for '%s': %s\n"
" If you want to trace into a file, then please
set %s\n"
- " to an absolute pathname (starting with /)\n"
- " Defaulting to tracing on stderr...",
+ " to an absolute pathname (starting with /)",
key->key, trace, key->key);
- key->fd = STDERR_FILENO;
+ trace_disable(key);
}
key->initialized = 1;
--
2.9.2.707.g48ee8b7
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] trace: use warning() for printing trace errors
On Thu, Aug 04, 2016 at 02:28:09PM -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > I wondered if that would then let us drop set_warn_routine(), but it
> > looks like there are other warning() calls it cares about. So that would
> > invalidate the last paragraph here, though I still think converting the
> > trace errors to warning() is a reasonable thing to do.
>
> Yes. That is why tonight's pushout will have this series in 'jch'
> (that is a point on a linear history between 'master' and 'pu') and
> tentatively ejects cc/apply-am topic out of 'pu', expecting it to be
> rerolled.
Here's a replacement patch 3. Same code, but it clarifies the
warn_routine situation in the commit message.
-- >8 --
Subject: [PATCH] trace: use warning() for printing trace errors
Right now we just fprintf() straight to stderr, which can
make the output hard to distinguish. It would be helpful to
give it one of our usual prefixes like "error:", "warning:",
etc.
It doesn't make sense to use error() here, as the trace code
is "optional" debugging code. If something goes wrong, we
should warn the user, but saying "error" implies the actual
git operation had a problem. So warning() is the only sane
choice.
Note that this does end up calling warn_routine() to do the
formatting. This is probably a good thing, since they are
clearly trying to hook messages before they make it to
stderr. However, it also means that in theory somebody who
tries to trace from their warn_routine() could cause a loop.
This seems rather unlikely in practice (we've never even
overridden the default warn_builtin routine before, and
recent discussions to do so would just install a noop
routine).
Signed-off-by: Jeff King
---
trace.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/trace.c b/trace.c
index bdbe149..6a77e4d 100644
--- a/trace.c
+++ b/trace.c
@@ -61,9 +61,8 @@ static int get_trace_fd(struct trace_key *key)
else if (is_absolute_path(trace)) {
int fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);
if (fd == -1) {
- fprintf(stderr,
- "Could not open '%s' for tracing: %s\n"
- "Defaulting to tracing on stderr...\n",
+ warning("Could not open '%s' for tracing: %s\n"
+ "Defaulting to tracing on stderr...",
trace, strerror(errno));
key->fd = STDERR_FILENO;
} else {
@@ -71,10 +70,10 @@ static int get_trace_fd(struct trace_key *key)
key->need_close = 1;
}
} else {
- fprintf(stderr, "What does '%s' for %s mean?\n"
+ warning("What does '%s' for %s mean?\n"
"If you want to trace into a file, then please set "
"%s to an absolute pathname (starting with /).\n"
- "Defaulting to tracing on stderr...\n",
+ "Defaulting to tracing on stderr...",
trace, key->key, key->key);
key->fd = STDERR_FILENO;
}
@@ -135,7 +134,7 @@ static int prepare_trace_line(const char *file, int line,
static void trace_write(struct trace_key *key, const void *buf, unsigned len)
{
if (write_in_full(get_trace_fd(key), buf, len) < 0)
- fprintf(stderr, "%s: write error (%s)\n", err_msg,
strerror(errno));
+ warning("%s: write error (%s)", err_msg, strerror(errno));
}
void trace_verbatim(struct trace_key *key, const void *buf, unsigned len)
--
2.9.2.707.g48ee8b7
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] trace: use warning() for printing trace errors
On Thu, Aug 4, 2016 at 11:28 PM, Junio C Hamano wrote: > Jeff King writes: > >> I wondered if that would then let us drop set_warn_routine(), but it >> looks like there are other warning() calls it cares about. So that would >> invalidate the last paragraph here, though I still think converting the >> trace errors to warning() is a reasonable thing to do. > > Yes. That is why tonight's pushout will have this series in 'jch' > (that is a point on a linear history between 'master' and 'pu') and > tentatively ejects cc/apply-am topic out of 'pu', expecting it to be > rerolled. Ok, I will reroll soon with Peff's suggested changes. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/7] trace: cosmetic fixes for error messages
On Thu, Aug 04, 2016 at 01:42:12PM -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > I think it would be nicer to still to print:
> >
> > warning: first line
> > warning: second line
> >
> > etc. We do that for "advice:", but not the rest of the vreportf
> > functions. It might be nice to do that, but we'd have to go back to
> > printing into a buffer (since we can't break up the incoming format
> > string that we feed to fprintf).
>
> Yes, yes.
Actually, I guess in this case we could easily do:
warning("something");
warning("something else");
etc (the lines are fairly stand-alone, so I don't think it runs afoul of
the usual translator-lego problem; not to mention that these aren't
actually translated). I don't really care that much between that and the
indented output, but if there's a preference, I'm happy to re-roll with
that.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)
(Fixed Nico's address) Jeff King wrote: > On Thu, Aug 04, 2016 at 11:34:35PM +, Eric Wong wrote: > > Junio C Hamano wrote: > > > [Graduated to "master"] > > > > > * ew/http-walker (2016-07-18) 4 commits > > > (merged to 'next' on 2016-07-18 at a430a97) > > > + list: avoid incompatibility with *BSD sys/queue.h > > > (merged to 'next' on 2016-07-13 at 8585c03) > > > + http-walker: reduce O(n) ops with doubly-linked list > > > > Yay! This finally introduces the Linux kernel linked list > > into git. I'm not sure if it's worth the effort to introduce > > cleanup commits to start using it in places where we already > > have doubly-linked list implementations: > > > > (+Cc Nicolas and Lukas) > > * sha1_file.c delta_base_cache_lru is open codes this > > * builtin/pack-redundant.c could probably be adapted, too > > ... any more? > > I just introduced another doubly-linked list in [1]. It adds some MRU > features on top of the list, but it could in theory be built on top of a > generic doubly-linked list. Yes, and you'd be avoiding the extra mallocs and be able to use list_entry (aka `container_of`) so it could be faster, too. I was thinking packed_git could also be a doubly-linked list anyways since it would allow easier removal of unlinked pack entries. My use case would be long-running "cat-file --batch" processes being able to detect unlinked packs after someone else runs GC. > It's also possible the delta-base-cache stuff could build on top of that > same code, but I didn't look closely at it. > > -Peff > > [1] http://public-inbox.org/git/20160729040659.gc22...@sigill.intra.peff.net/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)
On Fri, Aug 05, 2016 at 08:02:31AM +, Eric Wong wrote: > > I just introduced another doubly-linked list in [1]. It adds some MRU > > features on top of the list, but it could in theory be built on top of a > > generic doubly-linked list. > > Yes, and you'd be avoiding the extra mallocs and be able to use > list_entry (aka `container_of`) so it could be faster, too. I'm not sure which mallocs you mean. I allocate one struct per node, which seems like a requirement for a linked list. If you mean holding an extra list struct around an existing pointer (rather than shoving the prev/next pointers into the pointed-to- item), then yes, we could do that. But it feels like a bit dirty, since the point of the list is explicitly to provide an alternate ordering over an existing set of items. It also doesn't make a big difference for my use case. All I really care about is the speed of delete-from-middle-and-insert-at-front, which is trivially O(1) and involves no mallocs. > I was thinking packed_git could also be a doubly-linked list > anyways since it would allow easier removal of unlinked pack > entries. My use case would be long-running "cat-file --batch" > processes being able to detect unlinked packs after someone > else runs GC. We never remove packed_git structs, but it is not because of the list data structure. We may be holding open mmaps to packs that are deleted and continue using them. And in some cases other code may even hold pointers to our packed_git structs. So you'd have to figure out some memory ownership questions first. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
On Thu, Aug 04, 2016 at 02:12:22PM -0700, Junio C Hamano wrote: > > 2. Minor fixups noticed by maintainer, fixed while applying. > > This includes different kinds of things: > > a) Trivially correct fixes given in other people's review. > > b) Minor fixups by the maintainer, to code. > > c) Minor fixups by the maintainer, to proposed log message. > > d) "apply --whitespace=fix" whose result I do not even actively >keep track of. > > [...] > > I think I can > > * stop taking 2-a). This is less work for me, but some >contributors are leaky and can lose obviously good suggestions, >so I am not sure if that is an overall win for the quality of the >end product; Actually, I think the 2-a class is what often saves a re-roll. Somebody points out a typo in a commit message or a comment, and it quite often gets picked up by you without having another round-trip to the list. If you want to save work by not doing so, that's fine with me. But this is the gamble I was talking about. I think it's actually often less work to do the fixup than to look at another re-roll (especially with the "leaky contributor" thing where you have to make sure all fixes were applied). So it's a win if it saves the re-roll, but sometimes you end up having to look at the re-roll anyway. > * do a separate "SQUASH???" commit and send them out for 2-b). >This is a lot more work for a large series, but about the same >amount of work (except for "remembering to send them out" part) >for a small-ish topic. A contributor needs to realize that I >deal with _all_ the incoming topics, not just from topics from >one contributor, and small additional work tends to add up. I think these are largely the same as 2-a. You are just wearing two hats, reviewer and maintainer. Which I guess lets you take a shortcut sometimes (and just fix without mentioning it), but fundamentally the "gamble" aspect is the same, I think. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
Hi Stefan, On Thu, 4 Aug 2016, Stefan Beller wrote: > On Thu, Aug 4, 2016 at 8:58 AM, Johannes Schindelin > wrote: > > > >> If we were to change our workflows drastically, I'd propose to go a > >> way[1] similar to notedb in Gerrit, or git-series, > > > > Gerrit is a huge, non-distributed system. Complex, too. If we change > > the patch submission process, we should make things *easier*, not > > *harder*. So I think Gerrit is pretty much out of the question. > > I did not propose to use Gerrit or git-series or git appraise. > > However whenever I see these projects talking to each other, the talk > focuses on a "unified review storage format" pretty often, which would > make them compatible with each other. Unless you have a splendid idea how to integrate that unified format with our review process on the mailing list, I think this makes for a fine discussion elsewhere. I'd really like to focus on the Git project and its patch contribution process in this here thread. > So then I could choose git-series, while you could go with git appraise > (although that is written in go, so maybe too fancy already ;) I think you misunderstood me in a big way. New languages are awesome. I play with new toys whenever I find the time (and streamlining the contribution process would give me more time for that). You are talking to a person who implemented the Householder transformation in Postscript, a 6502 assembler in Forth, and a music composing system in Emacs Lisp. No language is to fancy for me. "me", as in "me, personally". Now let's think about Git for a moment, and its language choices, and the rationale behind them. The majority of the critically important code is written in C. Is C a good language? Decent, yes, but of course also limiting, Resource leaks are very easy to overlook, and we have a share of them. No object orientation, so when we need to "subclass", we have no compile time safety. The pre-processor constructs make static analysis nigh impossible. Plenty of downsides. So why was it chosen? Developers are *familiar* with it, that's why. Similar considerations apply to the use of shell scripts, and to Perl, to a certain extent. I am not talking about contrib/ of course. That's fair game, it contains only non-critical/fringe stuff. Note that the same rationale goes for choosing to accept patch submissions via mail to a list that is not subscribers-only. When it comes to inviting developers to contribute to your project, personal preferences become irrelevant, the deciding factor becomes how easy it is to join. Is the language popular, many developers already familiar with it? Is the build system readily available? Are the maintainers responsive? I vividly remember my reaction to Darcs, for example. It's written in Haskell. I am a mathematician originally, so Haskell appeals to me. Did the choice of language appear to be designed to keep contributors out? To me, it looked that way. Other example: submitGit. I really like what its intention. For a while, I even hoped to move my *own* patch submissions to submitGit. I planned to help get the kinks out of the code by contributing to it. It is written in Scala, using a web application and testing framework I have not encountered elsewhere. I struggled with installing it locally and wrapping my head around the coding paradigms, for 1.5 days (which was all I could afford at that time). Then I had to give up. Which made me very sad. I would not have written my mail-path-series.sh tool if submitGit had been written in node.js, for example, with which I am familiar enough to jump right in. So I hope you understand better now why I find Rust a poor choice for something like git-series, because it should not waste contributors' time by insisting that their price of entry is learning a new language they are unfamiliar with, using a new packaging system, installing a new build setup. I would find Clojure, Crystal or Swift just as poor a choice. Even node.js. It is just too much of a "Keep Out" sign for busy developers. And all the developers worth their salt are busy. > > Additionally, I would like to point out that defining a way to store > > reviews in Git is not necessarily improving the way our code > > contribution process works. If you want to record the discussions > > revolving around the code, I think public-inbox already does a pretty > > good job at that. > > Yeah recording is great, but we want to talk about replying and > modifying a series? So if I see a patch flying by on the mailing list, > ideally I could attach a "!fixup, signed off by Stefan" thing to that > patch. (I said "thing" as I do not necessarily mean email here. Right. I briefly considered suggesting a new tool that would operate on attachments, integrating tightly with the local git.git checkout. Briefly. I had to reject this idea because I do not think that requiring new tools just to contribute to Git would fly well. > > I guess I have no really good idea yet, either,
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
Hi Eric, On Thu, 4 Aug 2016, Eric Wong wrote: > Stefan Beller wrote: > > On Thu, Aug 4, 2016 at 8:58 AM, Johannes Schindelin > > wrote: > > > I guess I have no really good idea yet, either, how to retain the ease of > > > access of sending mails to the list, yet somehow keep a strong tie with > > > the original data stored in Git. > > > > Does it have to be email? Transmitting text could be solved > > differently as well. > > I've thought a lot about this over the years still think email > is the least bad. Not only that: people are *familiar* with it. And they have *access* to it. > Anti-spam tools for other messaging systems are far behind, > proprietary, or non-existent. bugzilla.kernel.org has been hit > hard lately and I see plenty of bug-tracker-to-mail spam as a > result from projects that use web-based bug trackers. Plus, they are all centralized. Do you want to *require* contibutors to register with a new service? > I guess a blockchain (*coin) implementation might work (like > hashcash is used for email anti-spam), but the ones I've glanced > at all look like a bigger waste of electricity than email > filters. I am not even so much concerned with ecological considerations here. Just the price of entry would be prohibitive. > Of course, centralized systems are unacceptable to me; > and with that I'll never claim any network service I run > will be reliable :) Hehehe. I guess that's why the public-inbox is backed by a Git repository... BTW is it auto-mirrored anywhere? Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)
Jeff King wrote: > On Fri, Aug 05, 2016 at 08:02:31AM +, Eric Wong wrote: > > > > I just introduced another doubly-linked list in [1]. It adds some MRU > > > features on top of the list, but it could in theory be built on top of a > > > generic doubly-linked list. > > > > Yes, and you'd be avoiding the extra mallocs and be able to use > > list_entry (aka `container_of`) so it could be faster, too. > > I'm not sure which mallocs you mean. I allocate one struct per node, > which seems like a requirement for a linked list. If you mean holding an > extra list struct around an existing pointer (rather than shoving the > prev/next pointers into the pointed-to- item), then yes, we could do > that. But it feels like a bit dirty, since the point of the list is > explicitly to provide an alternate ordering over an existing set of > items. This pattern to avoid that one malloc-per-node using list_entry (container_of) is actually a common idiom in the Linux kernel and Userspace RCU (URCU). Fwiw, I find it less error-prone and easier-to-follow than the "void *"-first-element thing we do with hashmap. > It also doesn't make a big difference for my use case. All I really care > about is the speed of delete-from-middle-and-insert-at-front, which is > trivially O(1) and involves no mallocs. > > > I was thinking packed_git could also be a doubly-linked list > > anyways since it would allow easier removal of unlinked pack > > entries. My use case would be long-running "cat-file --batch" > > processes being able to detect unlinked packs after someone > > else runs GC. > > We never remove packed_git structs, but it is not because of the list > data structure. We may be holding open mmaps to packs that are deleted > and continue using them. And in some cases other code may even hold > pointers to our packed_git structs. So you'd have to figure out some > memory ownership questions first. Yes, it's easier to replace a running process once in a while :) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)
On Fri, Aug 05, 2016 at 08:26:30AM +, Eric Wong wrote: > > I'm not sure which mallocs you mean. I allocate one struct per node, > > which seems like a requirement for a linked list. If you mean holding an > > extra list struct around an existing pointer (rather than shoving the > > prev/next pointers into the pointed-to- item), then yes, we could do > > that. But it feels like a bit dirty, since the point of the list is > > explicitly to provide an alternate ordering over an existing set of > > items. > > This pattern to avoid that one malloc-per-node using list_entry > (container_of) is actually a common idiom in the Linux kernel > and Userspace RCU (URCU). Fwiw, I find it less error-prone and > easier-to-follow than the "void *"-first-element thing we do > with hashmap. My big problem with it is that it gets confusing when a particular struct is a member of several lists. We have had bugs in git where a struct ended up being used in two different lists, but accidentally using the same "next" pointer. So you need one "list_head" for each list that your struct may be a part of. Sometimes that's simple, but it's awkward when the code which wants the list is different than the code which "owns" the struct. Besides leaking concerns across modules, the struct may not want to pay the memory price for storing pointers for all of the possible lists it could be a member of. For instance, I think it would be a mistake to convert the current commit_list code to something like this. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
Johannes Schindelin wrote: Agreed on all above points :> > On Thu, 4 Aug 2016, Eric Wong wrote: > > Of course, centralized systems are unacceptable to me; > > and with that I'll never claim any network service I run > > will be reliable :) > > Hehehe. I guess that's why the public-inbox is backed by a Git > repository... BTW is it auto-mirrored anywhere? Yep, and the code is AGPL so others can always replicate it. I just have the onions mentioned at the bottom of the HTML pages running something like: torsocks git fetch && public-inbox-index && sleep 30 in a loop[1]. http://hjrcffqmbrq6wope.onion/git http://czquwvybam4bgbro.onion/git I do encourage anybody who is able to, to run their own mirrors (off their existing email subscription) so my MX doesn't become an SPOF, either. I sorta documented it in my original announcement: https://public-inbox.org/git/20160710004813.ga20...@dcvr.yhbt.net/ https://public-inbox.org/INSTALL Feel free to ask me (+ m...@public-inbox.org) for install/running questions related to public-inbox (haven't gotten to making it work outside of Debian stable, though). [1] one of my far-off goals for git be: "git fetch --wait-for-update" to avoid needless wakeups -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)
Jeff King wrote:
> On Fri, Aug 05, 2016 at 08:26:30AM +, Eric Wong wrote:
>
> > > I'm not sure which mallocs you mean. I allocate one struct per node,
> > > which seems like a requirement for a linked list. If you mean holding an
> > > extra list struct around an existing pointer (rather than shoving the
> > > prev/next pointers into the pointed-to- item), then yes, we could do
> > > that. But it feels like a bit dirty, since the point of the list is
> > > explicitly to provide an alternate ordering over an existing set of
> > > items.
> >
> > This pattern to avoid that one malloc-per-node using list_entry
> > (container_of) is actually a common idiom in the Linux kernel
> > and Userspace RCU (URCU). Fwiw, I find it less error-prone and
> > easier-to-follow than the "void *"-first-element thing we do
> > with hashmap.
>
> My big problem with it is that it gets confusing when a particular
> struct is a member of several lists. We have had bugs in git where
> a struct ended up being used in two different lists, but accidentally
> using the same "next" pointer.
It might actually be easier since you would rarely (if ever)
touch the "next"/"prev" fields in your code. This encourages
users to give meaningful names to list_head fields.
> So you need one "list_head" for each list that your struct may be a part
> of. Sometimes that's simple, but it's awkward when the code which wants
> the list is different than the code which "owns" the struct. Besides
> leaking concerns across modules, the struct may not want to pay the
> memory price for storing pointers for all of the possible lists it could
> be a member of.
Yes, the key is this list is flexible enough to be used either way:
/* there are millions of these structs in practice */
struct common_struct {
struct list_head hot_ent;
...
};
/* and only a handful of these */
struct rarely_used_wrapper {
struct list_head cold_ent;
struct common_struct *common;
...
};
> For instance, I think it would be a mistake to convert the current
> commit_list code to something like this.
Of course, often a doubly-linked list is not needed or the extra
pointer is too expensive. Linux/URCU have hlist for this reason.
I'm no expert on git internals, either; but there can be
readability improvements, too. For example, I find http-walker.c
is easier-to-follow after 94e99012fc7a
("http-walker: reduce O(n) ops with doubly-linked list")
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] more archives of this list
On Sun, Jul 10, 2016 at 12:48:13AM +, Eric Wong wrote:
> Very much a work-in-progress, but NNTP and HTTP/HTTPS sorta work
> based on stuff that is on gmane and stuff I'm accumulating by
> being a subscriber.
I checked this out when you posted it, and have been using it the past
few weeks. I really like it. I find the URL structure much easier to
navigate than gmane.
I do find it visually a little harder to navigate through threads,
because there's not much styling there, and the messages seem to run
into one another. I don't know if a border around the divs or something
would help. I'm really terrible at that kind of visual design.
> HTTP URLs are clonable, but I've generated the following fast-export dump:
>
> https://public-inbox.org/.temp/git.vger.kernel.org-6c38c917e55c.gz
> (362M)
> [...]
> In contrast, bundles and packs delta poorly and only get down
> around 750-800M with aggressive packing
I pulled this down. It is indeed rather huge, and git doesn't perform
all that well with it. All the usual "git is not a database" things
apply, I think.
I noticed in particular that traversing the object graph is _really_
slow. This is very sensitive to the "branchiness" of the tree. I notice
that you use a single level of hash (e.g., d4/9a37e4974...). Since there
almost 300K messages, the average 2nd-level tree has over 1000 entries
in it, and each commit changes exactly one entry.
So what happens during a traversal is that we see some tree A, look at
all of its entries, and see each of its blobs. Then we see A', the same
tree with one entry different, and we still have to walk each of those
thousand entries, looking up each in a hash only to find "yep, we
already saw that blob".
Whereas if your tree is more tree-like (rather than list-like), you can
cull unchanged sub-trees more frequently. The tradeoff, though, is the
extra overhead in storing the sha1 for the extra level of tree
indirection.
Here are some timing and size results for various incarnations of the
packfile. The sizes come from:
git cat-file --batch-all-objects \
--batch-check='%(objectsize:disk) %(objecttype)' |
perl -lne '
/(\d+) (.*)/; $count{$2}++; $size{$2} += $1;
END { print "$size{$_} ($count{$_}) $_" for sort(keys(%count))
}'
And the timings are just "git rev-list --objects --all".
Here's the initial sizes after fast-import:
536339725 (291113) blob
63767736 (291154) commit
929164567 (582290) tree
Yikes, fast-import does a really terrible job of tree deltas (actually,
I'm not even sure it finds tree deltas at all). Notice that blob
contents are bigger than the fast-import stream (which contains all of
those contents!). That's unfortunate, but comes from the fact that we
zlib deflate the objects individually. Whereas the fast-import stream
was compressed as a whole, so the common elements between the emails get
a really good compression ratio.
There was discussion a long time ago about storing a common zlib
dictionary in the packfile and using it for all of the objects. I don't
recall whether there were any patches, though. It does create some
complications with serving clones/fetches, as they may ask for a subset
of the objects (so you have to send them the whole dictionary, which may
be a lot of overhead if you're only fetching a few objects).
Anyway, here are numbers after an aggressive repack:
628307898 (291113) blob
63209416 (291154) commit
44342440 (582290) tree
Much better trees. Ironically the blobs got worse. I think there are
just too many with similar names and sizes for our heuristics to do a
good job of finding deltas.
Here's what running rev-list looks like:
real6m4.933s
user6m4.124s
sys 0m0.616s
Yow, that's pretty painful. Without bitmaps, that's an operation that
every single clone would need to run.
Here's what it looks like with an extra level of hashing (so storing
"12/34/abcd..." instead of "12/34abcd..."):
628308433 (291113) blob
63207951 (291154) commit
60654550 (873339) tree
We're storing a lot more trees, and spending 16MB extra on tree storage.
But here's the rev-list time:
real0m55.120s
user0m55.016s
sys 0m0.096s
I didn't try doing an extra level of hashing on top of that (i.e.,
"12/34/ab/cd..."). It might help, but I suspect it's diminishing returns
versus the cost of accessing the extra trees.
The other thing that would probably make a big difference is avoiding
the one-commit-per-message pattern. The commit objects aren't that big,
but each one involves 2 new trees (one with ~1000 entries, and one with
256 entries). If you batched them into blocks of, say, 10 minutes, that
drops the number of commits by half.
Which I computed with:
git log --reverse --format=%at |
sort -n |
perl -lne '
if (!@block) {
@block = ($_);
} else {
my $diff = $_ - $block[0];
if ($diff >= 0 && $diff < 600) {
push
Re: [ANNOUNCE] more archives of this list
On Fri, Aug 05, 2016 at 05:28:05AM -0400, Jeff King wrote:
> On Sun, Jul 10, 2016 at 12:48:13AM +, Eric Wong wrote:
>
> > Very much a work-in-progress, but NNTP and HTTP/HTTPS sorta work
> > based on stuff that is on gmane and stuff I'm accumulating by
> > being a subscriber.
>
> I checked this out when you posted it, and have been using it the past
> few weeks. I really like it. I find the URL structure much easier to
> navigate than gmane.
>
> I do find it visually a little harder to navigate through threads,
> because there's not much styling there, and the messages seem to run
> into one another. I don't know if a border around the divs or something
> would help. I'm really terrible at that kind of visual design.
I took a peek at doing something simple like:
pre {
border-style: solid;
border-width: 1px;
background: #dd;
}
but it looks like there's no HTML structure at all to the current
output. It's just one big tag with various levels of indentation
to represent the messages.
So I guess a potential first step would be actually representing a
thread as:
parent message...
reply...
and so on.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 03/10] pkt-line: add packet_flush_gentle()
> On 02 Aug 2016, at 21:56, Torsten Bögershausen wrote: > > On Sun, Jul 31, 2016 at 11:45:08PM +0200, Lars Schneider wrote: >> >>> On 31 Jul 2016, at 22:36, Torstem Bögershausen wrote: >>> >>> >>> Am 29.07.2016 um 20:37 schrieb larsxschnei...@gmail.com: From: Lars Schneider packet_flush() would die in case of a write error even though for some callers an error would be acceptable. >>> What happens if there is a write error ? >>> Basically the protocol is out of synch. >>> Lenght information is mixed up with payload, or the other way >>> around. >>> It may be, that the consequences of a write error are acceptable, >>> because a filter is allowed to fail. >>> What is not acceptable is a "broken" protocol. >>> The consequence schould be to close the fd and tear down all >>> resources. connected to it. >>> In our case to terminate the external filter daemon in some way, >>> and to never use this instance again. >> >> Correct! That is exactly what is happening in kill_protocol2_filter() >> here: > > Wait a second. > Is kill the same as shutdown ? > I would expect that No, kill is used if the filter behaved strangely or signaled an error. "Shutdown" is a graceful shutdown. However, that might not be an ideal name. See the bottom of my discussion with Peff here: http://public-inbox.org/git/74C2CEA6-EAAB-406F-8B37-969654955413%40gmail.com/ > The process terminates itself as soon as it detects EOF. > As there is nothing more read. > > Then the next question: The combination of kill & protocol in kill_protocol(), > what does it mean ? I renamed that function to "kill_multi_file_filter". Initially I called the multi file filter "protocol" (bad decision I know) and named the functions accordingly. > Is it more like a graceful shutdown_protocol() ? Yes. Thanks, Lars-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] more archives of this list
Jeff King wrote: > On Fri, Aug 05, 2016 at 05:28:05AM -0400, Jeff King wrote: > > > On Sun, Jul 10, 2016 at 12:48:13AM +, Eric Wong wrote: > > > > > Very much a work-in-progress, but NNTP and HTTP/HTTPS sorta work > > > based on stuff that is on gmane and stuff I'm accumulating by > > > being a subscriber. > > > > I checked this out when you posted it, and have been using it the past > > few weeks. I really like it. I find the URL structure much easier to > > navigate than gmane. Thanks :> > > I do find it visually a little harder to navigate through threads, > > because there's not much styling there, and the messages seem to run > > into one another. I don't know if a border around the divs or something > > would help. I'm really terrible at that kind of visual design. > > I took a peek at doing something simple like: I'm trying to keep the visual design consistent across browsers without CSS support (I mainly use w3m) and CSS scares me: http://thejh.net/misc/website-terminal-copy-paste (and JavaScript has me cowering in a corner behind a chair :x) > but it looks like there's no HTML structure at all to the current > output. It's just one big tag with various levels of indentation > to represent the messages. > > So I guess a potential first step would be actually representing a > thread as: > > > parent message... > > reply... > > The, ... in the /$MID/t/ (as opposed to /$MID/T/) endpoint might be what you're looking for. See the "[flat|threaded]" links. I run out of horizontal space with the giant fonts I like to use, but it might be preferable for some folks. And about the 2/38 tree structure; I am starting to avoid it (for Xapian users, at least) but haven't performed the reindex on all my servers, yet: https://public-inbox.org/meta/20160805010300.7053-1-e%4080x24.org/ Of course, Xapian is only an optional dependency and won't affect git internals. I might end up having to nuke history occasionally anyways, (in case somebody invokes the DMCA, or there's enough spam to warrant permanent deletion) (Will try to digest the rest of your message later). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Designing the filter process protocol (was: Re: [PATCH v3 10/10] convert: add filter..process option)
> On 03 Aug 2016, at 20:30, Jakub Narębski wrote:
>
> The ultimate goal is to be able to run filter drivers faster for both `clean`
> and `smudge` operations. This is done by starting filter driver once per
> git command invocation, instead of once per file being processed. Git needs
> to pass actual contents of files to filter driver, and get its output.
>
> We want the protocol between Git and filter driver process to be extensible,
> so that new features can be added without modifying protocol.
>
>
> 1. CONFIGURATION
>
> As I wrote, there are different ways of configuring new-type filter driver:
>
> ...
>
> # Using a single variable for new filter type, and decide on which phase
> (which operation) is supported by filter driver during the handshake
> *(current approach)*
>
> [filter "protocol"]
> process = rot13-filtes.pl
>
> PROS: per-file and per-command filters possible with precedence rule;
> extensible to other types of drivers: textconv, diff, etc.
> only one invocation for commands which use both clean and smudge
> CONS: need single driver to be responsible for both clean and smudge;
> need to run driver to know that it does not support given
> operation (workaround exists)
>
>
> 2. HANDSHAKE (INITIALIZATION)
>
> Next, there is deciding on and designing the handshake between Git (between
> Git command) and the filter driver process. With the
> `filter..process`
> solution the driver needs to tell which operations among (for now) "clean"
> and "smudge" it does support. Plus it provides a way to extend protocol,
> adding new features, like support for streaming, cleaning from file or
> smudging to file, providing size upfront, perhaps even progress report.
>
> Current handshake consist of filter driver printing a signature, version
> number and capabilities, in that order. Git checks that it is well formed
> and matches expectations, and notes which of "clean" and "smudge" operations
> are supported by the filter.
>
> There is no interaction from the Git side in the handshake, for example to
> set options and expectations common to all files being filtered. Take
> one possible extension of protocol: supporting streaming. The filter
> driver needs to know whether it needs to read all the input, or whether
> it can start printing output while input is incoming (e.g. to reduce
> memory consumption)... though we may simply decide it to be next version
> of the protocol.
>
> On the other hand if the handshake began with Git sending some initializer
> info to the filter driver, we probably could detect one-shot filter
> misconfigured as process-filter.
OK, I'll look into this.
> Note that we need some way of deciding where handshake ends, either by
> specifying number of entries (currently: three lines / pkt-line packets),
> or providing some terminator ("smart" transport protocol uses flush packet
> for this).
>
> ...
Would you be OK with Peff's suggestion?
version=2
clean=true
smudge=true
http://public-inbox.org/git/20160803224619.bwtbvmslhuicx2qi%40sigill.intra.peff.net/
> 3. SENDING CONTENTS (FILE TO BE FILTERED AND FILTER OUTPUT)
>
> Next thing to design is decision how to send contents to be filtered
> to the filter driver process, and how to get filtered output from the
> filter driver process.
>
> ...
>
> # Send/receive data file by file, using some kind of chunking,
> with a end-of-file marker. The solution used by Git is
> pkt-line, with flush packet used to signal end of file.
>
> This is protocol used by the current implementation.
>
> PROS:
> - no need to know size upfront, so easier streaming support
> - you can signal error that happened during output, after
> some data were sent, as well as error known upfront
> - tracing support for free (GIT_TRACE_PACKET)
> CONS:
> - filter driver program slightly more difficult to implement
> - some negligible amount of overhead
>
> If we want in the end to implement streaming, then the last solution
> is the way to go.
>
>
> 4. PER-FILE HANDSHAKE - SENDING FILE TO FILTER
>
> Let's assume that for simplicity we want to implement (for now) only
> the synchronous (non-streaming) case, where we send whole contents
> of a file to filter driver process, and *then* read filter driver
> output.
> ...
>
> If we are using pkt-line, then the convention is that text lines
> are terminated using LF ("\n") character. This needs to be stated
> explicitly in the documentation for filter..process writers.
>
>git> packet: [operation] clean size=67\n
>
> We could denote that it is operation name, but it is obvious from
> position in the stream, thus not really needed.
I would prefer not to mix command and size in one packet as it
makes parsing a little more difficult.
> ...
>
> The Git would sent contents of the file to be filtered, using
> as many pack lines as needed (note: large file support needs
> to be tested,
Re: [PATCH v3 01/10] pkt-line: extract set_packet_header()
> On 03 Aug 2016, at 22:05, Jakub Narębski wrote:
>
> [This response might have been invalidated by v4]
>
> W dniu 01.08.2016 o 13:33, Lars Schneider pisze:
>>> On 30 Jul 2016, at 12:30, Jakub Narębski wrote:
>
#define hex(a) (hexchar[(a) & 15])
>>>
>>> I guess that this is inherited from the original, but this preprocessor
>>> macro is local to the format_header() / set_packet_header() function,
>>> and would not work outside it. Therefore I think we should #undef it
>>> after set_packet_header(), just in case somebody mistakes it for
>>> a generic hex() function. Perhaps even put it inside set_packet_header(),
>>> together with #undef.
>>>
>>> But I might be mistaken... let's check... no, it isn't used outside it.
>>
>> Agreed. Would that be OK?
>>
>> static void set_packet_header(char *buf, const int size)
>> {
>> static char hexchar[] = "0123456789abcdef";
>> #define hex(a) (hexchar[(a) & 15])
>> buf[0] = hex(size >> 12);
>> buf[1] = hex(size >> 8);
>> buf[2] = hex(size >> 4);
>> buf[3] = hex(size);
>> #undef hex
>> }
>
> That's better, though I wonder if we need to start #defines at begining
> of line. But I think current proposal is O.K.
>
>
> Either this (which has unnecessary larger scope)
>
> #define hex(a) (hexchar[(a) & 15])
> static void set_packet_header(char *buf, const int size)
> {
> static char hexchar[] = "0123456789abcdef";
>
> buf[0] = hex(size >> 12);
> buf[1] = hex(size >> 8);
> buf[2] = hex(size >> 4);
> buf[3] = hex(size);
> }
> #undef hex
>
> or this (which looks worse)
>
> static void set_packet_header(char *buf, const int size)
> {
> static char hexchar[] = "0123456789abcdef";
> #define hex(a) (hexchar[(a) & 15])
> buf[0] = hex(size >> 12);
> buf[1] = hex(size >> 8);
> buf[2] = hex(size >> 4);
> buf[3] = hex(size);
> #undef hex
> }
>
I probably will drop this patch as Junio is not convinced that it
is a good idea:
http://public-inbox.org/git/xmqqd1lp8v2o.fsf%40gitster.mtv.corp.google.com/
- Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
On Thu, Aug 04, 2016 at 09:42:18AM -0700, Stefan Beller wrote: > On Thu, Aug 4, 2016 at 8:58 AM, Johannes Schindelin > wrote: > > > >> If we were to change our workflows drastically, I'd propose to > >> go a way[1] similar to notedb in Gerrit, or git-series, > > > > Gerrit is a huge, non-distributed system. Complex, too. If we change the > > patch submission process, we should make things *easier*, not *harder*. So > > I think Gerrit is pretty much out of the question. > > I did not propose to use Gerrit or git-series or git appraise. > > However whenever I see these projects talking to each other, the talk focuses > on > a "unified review storage format" pretty often, which would make them > compatible > with each other. So then I could choose git-series, while you could go with > git appraise (although that is written in go, so maybe too fancy already ;) > Or there could be another new program written in C that follows the "review > format". This "unified review storage format" really does seem to be the missing piece. The tool I've been working on for the past year (git-candidate) was initially aimed at contrib[1], and was written in perl solely to satisfy contrib rules. It would have been python otherwise. The feedback from that thread[1], was that while git-candidate itself seemed interesting it would be unreasonable to bless a particular tool's format. So it seems to me that even if git-series had been written in perl rather than rust it could have expected a similar response to that of git-candidate, possibly. As Stefan says, if we're able to establish a standard for storing review data in git then it doesn't really matter what the tools are written in. For what it's worth my possibly quite shoddy attempt at a library implementing a possible review format for git[2] is written in perl, mostly to satisfy contrib requirements. > > > > Even requiring every contributor to register with GitHub would be too much > > of a limitation, I would wager. > > > > And when I said I have zero interest in tools that use the "latest and > > greatest language", I was hinting at git-series. Rust may be a fine and > > wonderful language. Implementing git-series in Rust, however, immediately > > limited the potential engagement with developers dramatically. Ironically contrib's language requirements actually raised the bar for me because it meant that I had to learn perl. > > > > Additionally, I would like to point out that defining a way to store > > reviews in Git is not necessarily improving the way our code contribution > > process works. If you want to record the discussions revolving around the > > code, I think public-inbox already does a pretty good job at that. I agree, and must apologise if this response has been too off topic, in any case I hope at least some of it was useful to someone. Hope this helps, Richard Ipsum [1]: http://www.mail-archive.com/git%40vger.kernel.org/msg80972.html [2]: https://bitbucket.org/richardipsum/perl-notedb -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/10] pkt-line: add direct_packet_write() and direct_packet_write_data()
> On 03 Aug 2016, at 22:12, Jakub Narębski wrote:
>
> [This response might have been invalidated by v4]
>
> W dniu 01.08.2016 o 14:00, Lars Schneider pisze:
>>> On 30 Jul 2016, at 12:49, Jakub Narębski wrote:
>>> W dniu 30.07.2016 o 01:37, larsxschnei...@gmail.com pisze:
Sometimes pkt-line data is already available in a buffer and it would
be a waste of resources to write the packet using packet_write() which
would copy the existing buffer into a strbuf before writing it.
If the caller has control over the buffer creation then the
PKTLINE_DATA_START macro can be used to skip the header and write
directly into the data section of a pkt-line (PKTLINE_DATA_LEN bytes
would be the maximum). direct_packet_write() would take this buffer,
adjust the pkt-line header and write it.
If the caller has no control over the buffer creation then
direct_packet_write_data() can be used. This function creates a pkt-line
header. Afterwards the header and the data buffer are written using two
consecutive write calls.
>>>
>>> I don't quite understand what do you mean by "caller has control
>>> over the buffer creation". Do you mean that caller either can write
>>> over the buffer, or cannot overwrite the buffer? Or do you mean that
>>> caller either can allocate buffer to hold header, or is getting
>>> only the data?
>>
>> How about this:
>>
>> [...]
>>
>> If the caller creates the buffer then a proper pkt-line buffer with header
>> and data section can be created. The PKTLINE_DATA_START macro can be used
>> to skip the header section and write directly to the data section
>> (PKTLINE_DATA_LEN
>> bytes would be the maximum). direct_packet_write() would take this buffer,
>> fill the pkt-line header section with the appropriate data length value and
>> write the entire buffer.
>>
>> If the caller does not create the buffer, and consequently cannot leave room
>> for the pkt-line header, then direct_packet_write_data() can be used. This
>> function creates an extra buffer for the pkt-line header and afterwards
>> writes
>> the header buffer and the data buffer with two consecutive write calls.
>>
>> ---
>> Is that more clear?
>
> Yes, I think it is more clear.
>
> The only thing that could be improved is to perhaps instead of using
>
> "then a proper pkt-line buffer with header and data section can be created"
>
> it might be more clear to write
>
> "then a proper pkt-line buffer with data section and a place for pkt-line
> header"
OK. I changed it to
"If the caller has control over the buffer creation then a proper pkt-line
buffer with header and data section can be allocated. The
PKTLINE_DATA_START macro can be used to skip the header and write
directly into the data section of a pkt-line (PKTLINE_DATA_LEN bytes
would be the maximum)..."
However, I am not yet sure if I can/will keep this patch:
http://public-inbox.org/git/xmqqeg645x6b.fsf%40gitster.mtv.corp.google.com/
>
+{
+ int ret = 0;
+ char hdr[4];
+ set_packet_header(hdr, sizeof(hdr) + size);
+ packet_trace(buf, size, 1);
+ if (gentle) {
+ ret = (
+ !write_or_whine_pipe(fd, hdr, sizeof(hdr), "pkt-line
header") ||
>>>
>>> You can write '4' here, no need for sizeof(hdr)... though compiler would
>>> optimize it away.
>>
>> Right, it would be optimized. However, I don't like the 4 there either. OK
>> to use a macro
>> instead? PKTLINE_HEADER_LEN ?
>
> Did you mean
>
>+ char hdr[PKTLINE_HEADER_LEN];
>+ set_packet_header(hdr, sizeof(hdr) + size);
yes!
+ !write_or_whine_pipe(fd, buf, size, "pkt-line data")
+ );
>>>
>>> Do we want to try to write "pkt-line data" if "pkt-line header" failed?
>>> If not, perhaps De Morgan-ize it
>>>
>>> + ret = !(
>>> + write_or_whine_pipe(fd, hdr, sizeof(hdr), "pkt-line
>>> header") &&
>>> + write_or_whine_pipe(fd, buf, size, "pkt-line data")
>>> + );
>>
>>
>> Original:
>> ret = (
>> !write_or_whine_pipe(fd, hdr, sizeof(hdr), "pkt-line
>> header") ||
>> !write_or_whine_pipe(fd, data, size, "pkt-line data")
>> );
>>
>> Well, if the first write call fails (return == 0), then it is negated and
>> evaluates to true.
>> I would think the second call is not evaluated, then?!
>
> This is true both for || and for &&, as in C logical boolean operators
> short-circuit.
True. That's why I did not get your "de morganize" it comment... what would de
morgan change?
>
>> Should I make this more explicit with a if clause?
>
> No need.
OK
Thanks,
Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 11/12] convert: add filter..process option
> On 04 Aug 2016, at 00:46, Jeff King wrote: > > On Wed, Aug 03, 2016 at 11:48:00PM +0200, Lars Schneider wrote: > >> OK. Is this the v2 discussion you are referring to? >> http://public-inbox.org/git/1461972887-22100-1-git-send-email-sbeller%40google.com/ >> >> What format do you suggest? >> >> packet: git< git-filter-protocol\n >> packet: git< version=2\n >> packet: git< capability=clean\n >> packet: git< capability=smudge\n >> packet: git< >> >> or >> >> packet: git< git-filter-protocol\n >> packet: git< version=2\n >> packet: git< capability\n >> packet: git< clean\n >> packet: git< smudge\n >> packet: git< >> >> or ... ? >> >> I would prefer the first one, I think. > > How about: > > version=2 > clean=true > smudge=true > > > ? Then we do not have to care about multiple "capability" keys (so > something naively parsing this could just store them in a string list, > for example). Alright. I will go with this solution. Thanks, Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 07/12] run-command: add clean_on_exit_handler
> On 04 Aug 2016, at 01:15, Jeff King wrote: > > On Thu, Aug 04, 2016 at 01:09:57AM +0200, Lars Schneider wrote: > >>> Or better yet, do not require a shutdown at all. The filter sees EOF and >>> knows there is nothing more to do. If we are in the middle of an >>> operation, then it knows git died. If not, then presumably git had >>> nothing else to say (and really, it is not the filter's business if git >>> saw an error or not). >> >> EOF? The filter is supposed to process multiple files. How would one EOF >> indicate that we are done? > > I think we may be talking about two different EOFs. > > Git sends a file in pkt-line format, and the flush marks EOF for that > file. But the filter keeps running, waiting for more input. This can > happen multiple times. Correct. > Eventually git calls close() on the descriptor, and the filter sees the > "real" EOF (i.e., read() returns 0). That is the signal that git is > done. Right. > >>> I'm not sure if calling that "shutdown" makes sense, though. It's almost >>> more of a checkpoint (and I wonder if git would ever want to >>> "checkpoint" without hanging up the connection). >> >> OK, I agree that the naming might not be ideal. But "checkpoint" does not >> convey that it is only executed once after all blobs are filtered?! > > Does the filter need to care? It's told to do any deferred work, and to > report back when it's done. The fact that git is calling it before it > decides to exit is not the filter's business (and you can imagine for > something like fast-import, it might want to feed files to something > like LFS, too; it already checkpoints occasionally to avoid lost work, > and would presumably want to ask LFS to checkpoint, too). > >> I understand that Git might not want to wait for the filter... > > If git _doesn't_ want to wait for the filter, I don't think you need a > checkpoint at all. True. However, I wonder if it could be useful if the filter is allowed to do some finishing work *before* Git returns to the user. > The filter just does its deferred work when it sees > git hang up the connection (i.e., the "real" EOF from above). Yeah it could do that. But then the filter cannot do things like modifying the index after the fact... however, that might be considered nasty by the Git community anyways... I am thinking about dropping this patch in the next roll as it is not strictly necessary for my current use case. Thanks, Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 10/10] convert: add filter..process option
> On 04 Aug 2016, at 12:18, Jakub Narębski wrote:
>
> ...
+
+ sigchain_push(SIGPIPE, SIG_IGN);
>>>
>>> Hmmm... ignoring SIGPIPE was good for one-shot filters. Is it still
>>> O.K. for per-command persistent ones?
>>
>> Very good question. You are right... we don't want to ignore any errors
>> during the protocol... I will remove it.
>
> I was actually just wondering.
>
> Actually the default behavior if SIGPIPE is not ignored (or if the
> SIGPIPE signal is not blocked / masked out) is to *terminate* the
> writing program, which we do not want.
>
> The correct solution is to check for error during write, and check
> if errno is set to EPIPE. This means that reader (filter driver
> process) has closed pipe, usually due to crash, and we need to handle
> that sanely, either restarting or quitting while providing sane
> information about error to the user.
>
> Well, we might want to set a signal handler for SIGPIPE, not just
> simply ignore it (especially for streaming case; stop streaming
> if filter driver crashed); though signal handlers are quite limited
> about what might be done in them. But that's for the future.
>
>
> Read from closed pipe returns EOF; write to closed pipe results in
> SIGPIPE and returns -1 (setting errno to EPIPE).
OK, I think I understand. I will address that in the next round.
>>> ...
>>> Or maybe extract writing the header for a file into a separate function?
>>> This one gets a bit long...
>>
>> Maybe... but I think that would make it harder to understand the protocol. I
>> think I would prefer to have all the communication in one function layer.
>
> I don't understand your reasoning here ("make it harder to understand the
> protocol"). If you choose good names for function writing header, then
> the main function would be the high-level view of protocol, e.g.
>
> git>
> git>
> git>
> git>
>
> git<
> git<
> git<
> git<
>
OK, I will move the header into a separate function.
...
+ cat ../test.o >test.r &&
>>>
>>> Err, the above is just copying file, isn't it?
>>> Maybe it was copied from other tests, I have not checked.
>>
>> It was created in the "setup" test.
>
> What I meant here (among other things) is that you uselessly use
> 'cat' to copy files:
>
>+ cp ../test.o test.r &&
Ah right. No idea why I did that. I'll use cp, of course :-)
+ echo "test22" >test2.r &&
+ mkdir testsubdir &&
+ echo "test333" >testsubdir/test3.r &&
>>>
>>> All right, we test text file, we test binary file (I assume), we test
>>> file in a subdirectory. What about testing empty file? Or large file
>>> which would not fit in the stdin/stdout buffer (as EXPENSIVE test)?
>>
>> No binary file. The main reason for this test is to check multiple files.
>> I'll add a empty file. A large file is tested in the next test.
>
> I assume that this large file is binary file; what matters is that it
> includes NUL character ("\0"), i.e. zero byte, checking that there is
> no error that would terminate it at NUL.
Good idea! I will add a small test file with \0 bytes in between to test
binaries.
Thanks,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
Am 04.08.2016 um 12:45 nachm. schrieb Junio C Hamano : > Andrew Keller writes: > >> In summary, I think I prefer #2 from a usability point of view, however I’m >> having >> trouble proving that #1 is actually *bad* and should be disallowed. > > Yeah, I agree with your argument from the usability and safety point > of view. > >> Any thoughts? Would it be better for the pre-commit hook to be >> officially allowed to edit the index [1], or would it be better >> for the pre-commit hook to explicitly *not* be allowed to edit the >> index [2], or would it be yet even better to simply leave it as it >> is? > > It is clear that our stance has been the third one so far. > > Another thing I did not see in your analysis is what happens if the > user is doing a partial commit, and how the changes made by > pre-commit hook is propagated back to the main index and the working > tree. > > The HEAD may have a file with contents in the "original" state, the > index may have the file with "update 1", and the working tree file > may have it with "update 2". After the commit is made, the user > will continue working from a state where the HEAD and the index have > "update 1", and the working tree has "update 2". "git diff file" > output before and after the commit will be identical (i.e. the > difference between "update 1" and "update 2") as expected. Excellent point — one I had discovered myself but neglected to include in my email. In my post-commit hook, I have logic in both versions of my experiment that disallows [1] fixing up diffs that are partially staged. Both scripts then update both the index and the working copy. (Sort of like how rebase works — clean working directory required, and then it updates the index and the work tree) [1] In version #1, if any files it wants to change are partially staged, it prints a detailed error message and aborts the commit outright. In version #2, the pre-commit hook sees the change it _wants_ to make, informs the user that he/she should run the fixup command, aborts the commit, and when the user runs the fixup command, the fixup command sees the partially staged file, prints the same detailed error message, and dies. Thanks for your help on this. it’s really been interesting. I’ll leave it as-is for now. Thanks, - Andrew Keller -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5533: make it pass on case-sensitive filesystems
Hi Junio, On Thu, 4 Aug 2016, Junio C Hamano wrote: > Johannes Schindelin writes: > > > The newly-added test case wants to commit a file "c.t" (note the lower > > case) when a previous test case already committed a file "C.t". This > > confuses Git to the point that it thinks "c.t" was not staged when "git > > add c.t" was called. > > > > Simply make the naming of the test commits consistent with the previous > > test cases: use upper-case, and advance in the alphabet. > > > > This came up in local work to rebase the Windows-specific patches to the > > current `next` branch. An identical fix was suggested by John Keeping. > > > > Signed-off-by: Johannes Schindelin > > --- > > Published-As: > > https://github.com/dscho/git/releases/tag/t5533-case-insensitive-v1 > > Fetch-It-Via: git fetch https://github.com/dscho/git > > t5533-case-insensitive-v1 > > Thanks. It may make it easier to see to have a blank line here, > separating them from the diffstat. Good suggestion! I made it so: https://github.com/dscho/mail-patch-series/commit/1776cb18 > > base-commit: 9813b109b4ec6630220e5f3d8aff275e23cba59e > > A totally unrelated tangent. > > This line turns out to be less than useful at least in this > particular case. > > The fix is meant for jk/push-force-with-lease-creation topic, but I > had to find it out by the old fashioned way, i.e. running blame for > these lines in 'pu' to find eee98e74f9 is the culprit and then > running "git branch --with eee98e74f9". The only thing the line > made easier is I _could_ start the blame at the named commit (which > is on 'next') instead of 'pu'. When I took that "base-commit" > series, I was hoping that it would give us a lot more useful > information. Sorry for that. The way my mail-patch-series.sh script works is that it tries to determine which branch between `master`, `next` or `pu` is the base (and it then submits *all* commits that are on top of that branch). So my branch was indeed based on `next` for that reason, not on top of `jk/push-force-with-lease-creation`. Otherwise, I would have resubmitted John's patches because the script would have determined that my patch is on top of `master`, not on top of `next`. Will try to think of a better way, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git mv: do not keep slash in `git mv dir non-existing-dir/`
When calling `rename("dir", "non-existing-dir/")` on Linux, it silently
succeeds, stripping the trailing slash of the second argument.
This is all good and dandy but this behavior disagrees with the specs at
http://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html
that state clearly regarding the 2nd parameter (called `new`):
If the `new` argument does not resolve to an existing directory
entry for a file of type directory and the `new` argument
contains at least one non- character and ends with one
or more trailing characters after all symbolic links
have been processed, `rename()` shall fail.
Of course, we would like `git mv dir non-existing-dir/` to succeed (and
rename the directory "dir" to "non-existing-dir"). Let's be extra
careful to remove the trailing slash in that case.
This lets t7001-mv.sh pass in Bash on Windows.
Signed-off-by: Johannes Schindelin
---
There is unfortunately another problem with Git running in Bash on
Windows: the credential cache daemon somehow gets stuck. I guess
this is some timing issue, in more than one way: right now, I do
not really have time to pursue this further.
Published-As: https://github.com/dscho/git/releases/tag/bash-on-windows-v1
Fetch-It-Via: git fetch https://github.com/dscho/git bash-on-windows-v1
builtin/mv.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index a201426..446a316 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -104,7 +104,7 @@ static int index_range_of_same_dir(const char *src, int
length,
int cmd_mv(int argc, const char **argv, const char *prefix)
{
- int i, gitmodules_modified = 0;
+ int i, flags, gitmodules_modified = 0;
int verbose = 0, show_only = 0, force = 0, ignore_errors = 0;
struct option builtin_mv_options[] = {
OPT__VERBOSE(&verbose, N_("be verbose")),
@@ -134,10 +134,13 @@ int cmd_mv(int argc, const char **argv, const char
*prefix)
modes = xcalloc(argc, sizeof(enum update_mode));
/*
* Keep trailing slash, needed to let
-* "git mv file no-such-dir/" error out.
+* "git mv file no-such-dir/" error out, except in the case
+* "git mv directory no-such-dir/".
*/
- dest_path = internal_copy_pathspec(prefix, argv + argc, 1,
- KEEP_TRAILING_SLASH);
+ flags = KEEP_TRAILING_SLASH;
+ if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
+ flags = 0;
+ dest_path = internal_copy_pathspec(prefix, argv + argc, 1, flags);
submodule_gitfile = xcalloc(argc, sizeof(char *));
if (dest_path[0][0] == '\0')
--
2.9.0.281.g286a8d9
base-commit: c6b0597e9ac7277e148e2fd4d7615ac6e0bfb661
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/12] pkt-line: extract set_packet_header()
> On 04 Aug 2016, at 18:14, Junio C Hamano wrote: > > Jeff King writes: > >> The cost of write() may vary on other platforms, but the cost of memcpy >> generally shouldn't. So I'm inclined to say that it is not really worth >> micro-optimizing the interface. >> >> I think the other issue is that format_packet() only lets you send >> string data via "%s", so it cannot be used for arbitrary data that may >> contain NULs. So we do need _some_ other interface to let you send a raw >> data packet, and it's going to look similar to the direct_packet_write() >> thing. > > OK. That is a much better argument than "I already stuff the length > bytes in my buffer" (which will invite "How about stop doing that?") > to justify a new "I have N bytes of data, send it out", whose > signature would look more like write(2) and deserve to be called > packet_write() but unfortunately the name is taken by what should > have called packet_fmt() or something, but that squats on a good > name packet_write(). Sigh. Well, my argument wasn't meant to be offensive. It was just an idea that I published the to get feedback. Now I understand that it wasn't a particular good idea (thanks Peff for the performance test!). However, besides the bogus performance argument I introduced that function to allow packet writs to fail using the `gentle` parameter: http://public-inbox.org/git/D116610C-F33A-43DA-A49D-0B33958822E5%40gmail.com/ Would you be OK if I introduce packet_write_gently() that returns `0` if the write was OK and `-1` if it failed? Thanks, Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
On Wed, Aug 3, 2016 at 6:07 PM, Johannes Schindelin wrote: > It would be a totally different matter, of course, if you used the > branches I publish via my GitHub repository, added fixup! and squash! > commits, published the result to a public repository and then told me to > pull from there, that would make things easier. We could even introduce a > reword! construct, to make the review of the suggested edits of the commit > message easier. On the topic of fixup and squash and everything. Is anyone else annoyed that the commit title is taken for fixup!, squash! instructions? After you have added a few of them, "git log --oneline" becomes useless. All you see is "fixup! A", "fixup! A", "fixup! B", "fixup! A". Would it be better to let the user control the title? We still need the cue "fixup!", "squash!"... at the beginning of the title, but the original commit reference is appended at the end, like s-o-b lines. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] more archives of this list
On Fri, Aug 5, 2016 at 11:28 AM, Jeff King wrote: > There was discussion a long time ago about storing a common zlib > dictionary in the packfile and using it for all of the objects. I don't > recall whether there were any patches, though. It does create some > complications with serving clones/fetches, as they may ask for a subset > of the objects (so you have to send them the whole dictionary, which may > be a lot of overhead if you're only fetching a few objects). I'm nit picking since it's not actually "all objects". But pack v4 patches have two dictionaries (or more? i don't remember) for commits and trees :) -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
Hi Duy, On Fri, 5 Aug 2016, Duy Nguyen wrote: > On the topic of fixup and squash and everything. Is anyone else > annoyed that the commit title is taken for fixup!, squash! > instructions? I do not know about others, but I am not annoyed by those commit titles. I think they make tons of sense. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
Hi Richard, On Fri, 5 Aug 2016, Richard Ipsum wrote: > On Thu, Aug 04, 2016 at 09:42:18AM -0700, Stefan Beller wrote: > > On Thu, Aug 4, 2016 at 8:58 AM, Johannes Schindelin > > wrote: > > > > > >> If we were to change our workflows drastically, I'd propose to go a > > >> way[1] similar to notedb in Gerrit, or git-series, > > > > > > Gerrit is a huge, non-distributed system. Complex, too. If we change > > > the patch submission process, we should make things *easier*, not > > > *harder*. So I think Gerrit is pretty much out of the question. > > > > I did not propose to use Gerrit or git-series or git appraise. > > > > However whenever I see these projects talking to each other, the talk > > focuses on a "unified review storage format" pretty often, which would > > make them compatible with each other. So then I could choose > > git-series, while you could go with git appraise (although that is > > written in go, so maybe too fancy already ;) Or there could be another > > new program written in C that follows the "review format". > > This "unified review storage format" really does seem to be the missing > piece. FWIW I do not think so. The real trick will be to come up with an improvement to the process that lets Junio and Peff continue to work as before, because It Works For Them, while at the same time letting other people (such as myself) use easy-to-configure tools that add substantial convenience. Which, to me, means that the missing piece is a clever idea how to integrate with the mail-based process, without requiring everybody and her dog to switch to a specific mail client. > The tool I've been working on for the past year (git-candidate) was > initially aimed at contrib[1], and was written in perl solely to satisfy > contrib rules. It would have been python otherwise. Oh...? $ git ls-files contrib/\*.py | wc -l 4 And for that matter: $ git ls-files contrib/\*.go | wc -l 4 In fact, there are even PHP scripts: $ git ls-files contrib | sed -n 's/.*\.//p' | sort | grep -v '.' | uniq | tr '\n' ' ' bash c el Git go perl php pl pm py rst sh tcsh txt zsh But again, I do not think that it makes sense to focus too much on a language, or on a file format, before we came up with a strategy how to *not* require everybody to change their current ways. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning
Hi Junio & René, On Thu, 4 Aug 2016, Junio C Hamano wrote: > Let's try it this way. How about this as a replacement? I like it (with the if (s2) test intead of if (s1), of course). But please record René as author, maybe mentioning myself with a "Diagnosed-by:" line. FWIW today's `pu` does pass the test suite without problems in my CI setup. This setup will from now on test next & pu in the Git for Windows SDK, and rebase Git for Windows' current master to git.git's maint, master, next & pu, every morning after a weekday (unless I forget to turn on my laptop, that is). Once it stabilizes, I will figure out a way how to publish the logs, because playing Lt Tawney Madison gets old real quick. Thanks, Dscho
Re: [PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning
Hi Junio,
On Thu, 4 Aug 2016, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > With GCC 6, the strdup() function is declared with the "nonnull"
> > attribute, stating that it is not allowed to pass a NULL value as
> > parameter.
> >
> > In nedmalloc()'s reimplementation of strdup(), Postel's Law is heeded
> > and NULL parameters are handled gracefully. GCC 6 complains about that
> > now because it thinks that NULL cannot be passed to strdup() anyway.
> >
> > Let's just shut up GCC >= 6 in that case and go on with our lives.
> >
> > See https://gcc.gnu.org/gcc-6/porting_to.html for details.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > compat/nedmalloc/nedmalloc.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
> > index 677d1b2..3f28c0b 100644
> > --- a/compat/nedmalloc/nedmalloc.c
> > +++ b/compat/nedmalloc/nedmalloc.c
> > @@ -956,6 +956,9 @@ void **nedpindependent_comalloc(nedpool *p, size_t
> > elems, size_t *sizes, void **
> > char *strdup(const char *s1)
> > {
> > char *s2 = 0;
> > +#if __GNUC__ >= 6
> > +#pragma GCC diagnostic ignored "-Wnonnull-compare"
> > +#endif
> > if (s1) {
> > size_t len = strlen(s1) + 1;
> > s2 = malloc(len);
>
> Is it a common convention to place "#pragma GCC diagnostic"
> immediately before the statement you want to affect, and have the
> same pragma in effect until the end of the compilation unit?
Uh oh. This was a brain fart. I somehow confused the way pragmas work with
the way __attribute__s work. You are correct, of course, that the pragma
affects the entire remainder of the file, not just this statement.
Luckily, René came up with a much more elegant solution, so that Git's
history does not have to shame me eternally.
Ciao,
Dscho
Re: [PATCH v6 16/16] merge-recursive: flush output buffer even when erroring out
Hi Junio, On Thu, 4 Aug 2016, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Ever since 66a155b (Enable output buffering in merge-recursive., > > 2007-01-14), we had a problem: When the merge failed in a fatal way, all > > regular output was swallowed because we called die() and did not get a > > chance to drain the output buffers. > > OK. Even though I really wanted to see somebody else review this > series as well, I finished reading it through one more time before > that happened, which is unfortunate because I think this is ready to > start cooking in 'next' even though I no longer have much faith in > my eyes alone after staring at this series so many times---you start > missing details. Yeah, well, it is a rather crucial piece of the code. But then, I really tried my best to re-review the series a couple of times (with my primary focus on robustness, not elegance) after working on different tasks for a couple of days. Combined with my long-term dogfooding and my readiness to jump on any breakage I may have introduced, I am relatively confident nevertheless. Thank you so much for your patient reviews! Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: storing cover letter of a patch series?
"Michael S. Tsirkin" writes:
> On Thu, Sep 10, 2015 at 11:39:49AM -0700, Junio C Hamano wrote:
>> The problem with "empty commit trick" is that it is a commit whose
>> sole purpose is to describe the series, and its presence makes it
>> clear where the series ends, but the topology does not tell where
>> the series begins, so it is an unsatisifactory half-measure.
>
> Actually, when using topic branches the series always ends at head, so
> it's better to keep the empty commit where series begins.
But that would mean that you would need to destroy and recreate more
commits than you would need to. If you have a five-commit series
(with the bottom "description" one, you would have six commits) and
you are already happy with the bottom two but want to update the
third one, you wuld have to "rebase -i" all six of them, reword the
bottom "description" to adjust it to describe the new version of the
third one _before_ you even do the actual update of the third one.
That somehow feels backwards, and that backward-ness comes from the
fact that you abused a single-parent commit for the purpose it is
not meant to be used (i.e. they are to describe individual changes),
because you did not find a better existing mechanism (and I suspect
there isn't any, in which case the solution is to invent one, not
abusing an existing mechanism that is not suited for it).
If this were part of a workflow like this, I would understand it:
* Build a N-commit series on a topic.
* You keep a "local integration testing" branch ("lit"), forked
from a mainline and updated _every time_ you do something to your
topics. You may or may not publish this branch. This is the
aggregation of what you locally have done, a convenient place to
test individual topics together before they get published.
* A new topic, when you merge it to the "lit" branch, you describe
the cover as the merge commit message.
* When you updated an existing topic, you tell a tool like "rebase
-i -p" to recreate "lit" branch on top of the mainline. This
would give you an opportunity to update the cover.
Now the tool support for the last one is the missing piece. In
addition to what "rebase -i -p" would, it at least need to
automatically figure out which topics have been updated, so that
their merge commit log messages need to be given in the editor to
update, while carrying over the merge log message for other topics
intact (by default).
With that, you should also be able to teach "format-patch --cover"
to take these merge messages on "lit" into account when it creates
the cover letter.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
Hi Junio, On Thu, 4 Aug 2016, Junio C Hamano wrote: > Jeff King writes: > > > Like you, I have occasionally been bitten by Junio doing a fixup, and > > then I end up re-rolling, and lose that fixup. > > ... which usually is caught when I receive the reroll, as I try to apply > to the same base and compare the result with the previous round. I find it incredibly nice of you to do those fix-ups, sorry to state that as clearly only now. It's just that I hate to see your time wasted, in particular when *I* waste it because I missed the fix-ups in `pu`. > > But I think such fixups are a calculated risk. Sometimes they save a > > lot of time, both for the maintainer and the contributor, when they > > manage to prevent another round-trip of the patch series to the list. > > Yes. FWIW I am more than just willing to spend a little more time on applying fix-ups and re-rolling patch series (and dual-publishing them via my public repository), if it helps lighten your burden. > > IOW, if the flow is something like: > > > > 1. Contributor sends patches. People review. > > > > 2. Minor fixups noticed by maintainer, fixed while applying. > > This includes different kinds of things: > > a) Trivially correct fixes given in other people's review. > > b) Minor fixups by the maintainer, to code. > > c) Minor fixups by the maintainer, to proposed log message. > > d) "apply --whitespace=fix" whose result I do not even actively >keep track of. > > > 3. Only one small fixup needed from review. Contributor sends > > squashable patch. Maintainer squashes. > > > > then I think that is a net win over sending the whole series again, for > > the contributor (who does not bother sending), reviewers (who really > > only need to look at the interdiff, which is what that squash is in the > > first place), and the maintainer (who can squash just as easily as > > re-applying the whole series). > > > And that is the flip side. If the flow above does not happen, then step > > 2 just becomes a pain. > > I think I can > > * stop taking 2-a). This is less work for me, but some >contributors are leaky and can lose obviously good suggestions, >so I am not sure if that is an overall win for the quality of the >end product; If you had a `git commit --reword` command to touch up commit messages, would that help you, together with the `git commit --fixup` command for code changes? The branches in `pu` would have your fix-ups as strictly separate commits on top of the contributed patches, and the branches would need to be sent through rebase -i before merging to `next`, of course. The idea would be to not forget your fixups in subsequent iterations, but simply rebase them on top of the new iteration. It would still not solve my problem that there is no convenient way to jump from your commits in `pu` to the corresponding ones in my local branch. But that is my problem, not yours. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git mv: do not keep slash in `git mv dir non-existing-dir/`
Johannes Schindelin writes:
> When calling `rename("dir", "non-existing-dir/")` on Linux, it silently
> succeeds, stripping the trailing slash of the second argument.
>
> This is all good and dandy but this behavior disagrees with the specs at
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html
>
> that state clearly regarding the 2nd parameter (called `new`):
>
> If the `new` argument does not resolve to an existing directory
> entry for a file of type directory and the `new` argument
> contains at least one non- character and ends with one
> or more trailing characters after all symbolic links
> have been processed, `rename()` shall fail.
I agree with all of the above. But
> Of course, we would like `git mv dir non-existing-dir/` to succeed (and
> rename the directory "dir" to "non-existing-dir").
I do not think I want that. When I say "mv A B/", I want it to fail
if I made a typo for B; the trailing slash after B is an explicit
statement "I expect B to exist and I want A to appear at B/A".
Current Git behaviour on Linux seems to allow "git mv dir no-such-dir/"
but "dir" is renamed to "no-such-dir", which fails two expectations,
and I think this is broken. If Windows port does not share this
breakage, that is a good thing. We should fix Git behaviour on Linux
instead, I would think.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git mv: do not keep slash in `git mv dir non-existing-dir/`
Hi Junio,
On Fri, 5 Aug 2016, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > When calling `rename("dir", "non-existing-dir/")` on Linux, it silently
> > succeeds, stripping the trailing slash of the second argument.
> >
> > This is all good and dandy but this behavior disagrees with the specs at
> >
> > http://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html
> >
> > that state clearly regarding the 2nd parameter (called `new`):
> >
> > If the `new` argument does not resolve to an existing directory
> > entry for a file of type directory and the `new` argument
> > contains at least one non- character and ends with one
> > or more trailing characters after all symbolic links
> > have been processed, `rename()` shall fail.
>
> I agree with all of the above. But
>
> > Of course, we would like `git mv dir non-existing-dir/` to succeed (and
> > rename the directory "dir" to "non-existing-dir").
>
> I do not think I want that. When I say "mv A B/", I want it to fail
> if I made a typo for B; the trailing slash after B is an explicit
> statement "I expect B to exist and I want A to appear at B/A".
Please note that t7001 *specifically* tests for the opposite of what you
want, then ;-)
https://github.com/git/git/blob/v2.9.2/t/t7001-mv.sh#L79-L80
> Current Git behaviour on Linux seems to allow "git mv dir no-such-dir/"
> but "dir" is renamed to "no-such-dir", which fails two expectations,
> and I think this is broken. If Windows port does not share this
> breakage, that is a good thing. We should fix Git behaviour on Linux
> instead, I would think.
To be precise, Git for Windows displays the same behavior as Git on Linux,
because rename("dir", "no-such-dir/") succeeds.
The breakage fixed by this here patch happens when running plain Linux Git
in "Bash on Windows" (i.e. Bash on Ubuntu on Windows, the new Linux
subsystem of Windows, allowing to run unmodified Linux binaries on Windows
without the need for a Virtual Machine).
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] rebase--interactive: Add "sign" command
Hi Peff, On Wed, 3 Aug 2016, Jeff King wrote: > On Wed, Aug 03, 2016 at 09:08:48AM -0700, Junio C Hamano wrote: > > > > However, I could imagine that we actually want this to be more > > > extensible. After all, all you are doing is to introduce a new > > > rebase -i command that does nothing else than shelling out to a > > > command. > > > > Yup, I tend to agree. > > > > Adding "sign" feature (i.e. make it pass -S to "commit [--amend]") may > > be a good thing, but adding "sign" command to do so is not a great > > design. > > I'm not sure what you mean by "feature" here, but it reminded me of > Michael's proposal to allow options to todo lines: > > http://public-inbox.org/git/530da00e.4090...@alum.mit.edu/ > > which would allow: > > pick -S 1234abcd > > If that's what you meant, I think it is a good idea. :) I looked at the code in git-rebase--interactive.sh again and stumbled over something important: if you "pick" a commit, it *already* uses the information provided to the rebase command via the -S option, *unless* the pick fast-forwards. That is, I came to believe that the "sign" command is unnecessary, and that the --force-rebase option in conjunction with the -S option is what should be used. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning
Johannes Schindelin writes: > This setup will from now on test next & pu in the Git for Windows SDK, and > rebase Git for Windows' current master to git.git's maint, master, next & > pu, every morning after a weekday (unless I forget to turn on my laptop, > that is). Sounds really good. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git mv: do not keep slash in `git mv dir non-existing-dir/`
Johannes Schindelin writes: > Please note that t7001 *specifically* tests for the opposite of what you > want, then ;-) Yes, I know. I am torn. It seems "mv A B/" (not "git mv") when B does not exist does the same "wrong" thing, so the existing behaviour is at least consistent with the command line tools on Linux, and more importantly, existing users of "git mv A B/" have expected it to ignore the trailing slash for a long time, so let's not change the expected behaviour. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/12] pkt-line: extract set_packet_header()
Lars Schneider writes: > However, besides the bogus performance argument I introduced that function > to allow packet writs to fail using the `gentle` parameter: > http://public-inbox.org/git/D116610C-F33A-43DA-A49D-0B33958822E5%40gmail.com/ > > Would you be OK if I introduce packet_write_gently() that returns `0` if the > write was OK and `-1` if it failed? Yes, I agree with you that it would be a good thing to have a _gently() variant that lets the caller deal with possible error conditions itself instead of dying. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 6/8] status: print branch info with --porcelain=v2 --branch
Jeff Hostetler writes:
> /*
> + * Print branch information for porcelain v2 output. These lines
> + * are printed when the '--branch' parameter is given.
> + *
> + *# branch.oid
> + *# branch.head
Just bikeshedding, but ...
> + if (!s->branch)
> + fprintf(s->fp, "# branch.head %s%c", "(unknown)", eol);
> + else {
> + if (!strcmp(s->branch, "HEAD")) {
> + fprintf(s->fp, "# branch.head %s%c", "(detached)", eol);
> +
> + if (state.rebase_in_progress ||
> state.rebase_interactive_in_progress)
> + branch_name = state.onto;
> + else if (state.detached_from)
> + branch_name = state.detached_from;
> + else
> + branch_name = "";
> + } else {
> + branch_name = NULL;
> + skip_prefix(s->branch, "refs/heads/", &branch_name);
> +
> + fprintf(s->fp, "# branch.head %s%c", branch_name, eol);
... given that we are showing branch name, perhaps "branch.name"
instead of "branch.head" is more appropriate?
I wondered if "# " prefix before these lines is useful, by the way,
and initially thought that the fact that these lines begin with
"branch." and not with the "1/2/u $key" sufficient clue for whoever
reads them, but the reader can tell which kind of record it is by
reading the first two characters of each line (i.e. if "# " that is
not the usual "change info for a single file"), so it is actually a
good idea.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/12] pkt-line: extract set_packet_header()
> On 04 Aug 2016, at 18:14, Junio C Hamano wrote: > > Jeff King writes: > >> The cost of write() may vary on other platforms, but the cost of memcpy >> generally shouldn't. So I'm inclined to say that it is not really worth >> micro-optimizing the interface. >> >> I think the other issue is that format_packet() only lets you send >> string data via "%s", so it cannot be used for arbitrary data that may >> contain NULs. So we do need _some_ other interface to let you send a raw >> data packet, and it's going to look similar to the direct_packet_write() >> thing. > > OK. That is a much better argument than "I already stuff the length > bytes in my buffer" (which will invite "How about stop doing that?") > to justify a new "I have N bytes of data, send it out", whose > signature would look more like write(2) and deserve to be called > packet_write() but unfortunately the name is taken by what should > have called packet_fmt() or something, but that squats on a good > name packet_write(). Sigh. "Sigh" means, a series preparation patch that renames "packet_write()" to "paket_write_fmt()" would not be a good idea? It is used 59 times currently... - Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/12] pkt-line: extract set_packet_header()
On Fri, Aug 5, 2016 at 10:31 AM, Lars Schneider wrote: > >> On 04 Aug 2016, at 18:14, Junio C Hamano wrote: >> >> signature would look more like write(2) and deserve to be called >> packet_write() but unfortunately the name is taken by what should >> have called packet_fmt() or something, but that squats on a good >> name packet_write(). Sigh. > > "Sigh" means, a series preparation patch that renames "packet_write()" > to "paket_write_fmt()" would not be a good idea? It is used 59 times > currently... It would be a good idea in the longer term, I would think. I just wasn't sure if you are willing to volunteer, and in-flight topics will tolerate, such a change right now. I have a feeling that all the current callsites are fairly stable and no in-flight topic touches them, so if you feel like doing so, please go ahead ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 7/8] git-status.txt: describe --porcelain=v2 format
Jeff Hostetler writes: > +Porcelain Format Version 2 > +~~ > + > +Version 2 format adds more detailed information about the state of > +the worktree and changed items. Version 2 also defines an extensible > +set of easy to parse optional headers. > + > +Header lines start with "#" and are added in response to specific > +command line arguments. Parsers should ignore headers they > +don't recognize. > + > +### Branch Headers > + > +If `--branch` is given, a series of header lines are printed with > +information about the current branch. > + > +Line Notes > + > +# branch.oid | (initial)Current commit. > +# branch.head | (detached) Current branch. In an earlier review I made a noise about "branch.head", but reading this together with others in a context, "branch.head" is good and I do not see a need to bikeshed it into "branch.name". > +# branch.upstream If upstream is set. > +# branch.ab + - If upstream is set and > + the commit is present. > + > + > +### Changed Tracked Entries > + > +Following the headers, a series of lines are printed for tracked > +entries. One of three different line formats may be used to describe > +an entry depending on the type of change. Tracked entries are printed > +in an undefined order; parsers should allow for a mixture of the 3 > +line types in any order. > + > +Ordinary changed entries have the following format: > + > +1 > + > +Renamed or copied entries have the following format: > + > +2 > + > +Field Meaning > + > +A 2 character field containing the staged and > +unstaged XY values described in the short format, > +with unchanged indicated by a "." rather than > +a space. > + A 4 character field describing the submodule state. > +"N..." when the entry is not a submodule. > +"S" when the entry is a submodule. > + is "C" if the commit changed; otherwise ".". > + is "M" if it has tracked changes; otherwise ".". > + is "U" if there are untracked changes; otherwise ".". > +The 6 character octal file mode in the HEAD. We do want "octal" to be spelled out, but I doubt that we want to guarantee to the reading scripts that this will always be 6 characters. > +The octal file mode in the index. > +The octal file mode in the worktree. > +The SHA1 value in the HEAD. > +The SHA1 value in the index. Please avoid inventing a word "SHA1 value" that does not appear in Documentation/glossary-content.txt; s/SHA1 value/object name/g. This comment applies to the desciption for "U" lines below, too. > + The rename or copied percentage score. For example "R100" > +or "C75". Documentation/diff-format.txt seems to call this differently. The rename or copy "score" number, e.g. "R100", "C75". perhaps? > + The current pathname. Unless you are talking about "2" line, saying "current" is somewhat weird. I _think_ is the name in the index and in the working tree, as opposed to the original path in the HEAD, but the distinction does not matter for "1" or "U" lines. As this table is meant to be shared between "1" and "2", perhaps The pathname. In a renamed/copied entry, this is the path in the index and in the working tree. The pathname in the commit at HEAD. This is only present in a renamed/copied entry, and tells where the renamed/copied contents came from. or something like that may clarify it a bit better. Then in the table for "U" lines, we can just say "The pathname". > + When the `-z` option is used, the 2 pathnames are separated > +with a NUL (ASCII 0x00) byte; otherwise, a tab (ASCII 0x09) > +byte separates them. > + The original path. This is only present when the entry > +has been renamed or copied. If it is explained as "the original path", wouldn't it be easier to read if it were called "" instead of ""? > +When the `-z` option is given, pathnames are printed as is and > +without any quoting and lines are terminated with a NUL (ASCII 0x00) > +byte. > + > +Otherwise, all pathnames will be "C-quoted" if they contain any tab, > +linefeed, double quote, or backslash characters. In C-quoting, these > +characters will be replaced with the corresponding C-style escape > +sequences and the resulting pathname will be double quoted. Looks good. I think I saw "C-Quoted" elsewhere (perhaps in a in-code comment), which
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
Hi Johannes, On Fri, Aug 5, 2016 at 1:20 AM, Johannes Schindelin wrote: > Hi Stefan, > > On Thu, 4 Aug 2016, Stefan Beller wrote: > >> On Thu, Aug 4, 2016 at 8:58 AM, Johannes Schindelin >> wrote: >> > >> >> If we were to change our workflows drastically, I'd propose to go a >> >> way[1] similar to notedb in Gerrit, or git-series, >> > >> > Gerrit is a huge, non-distributed system. Complex, too. If we change >> > the patch submission process, we should make things *easier*, not >> > *harder*. So I think Gerrit is pretty much out of the question. >> >> I did not propose to use Gerrit or git-series or git appraise. >> >> However whenever I see these projects talking to each other, the talk >> focuses on a "unified review storage format" pretty often, which would >> make them compatible with each other. > > Unless you have a splendid idea how to integrate that unified format with > our review process on the mailing list, I think this makes for a fine > discussion elsewhere. I'd really like to focus on the Git project and its > patch contribution process in this here thread. > >> So then I could choose git-series, while you could go with git appraise >> (although that is written in go, so maybe too fancy already ;) > > I think you misunderstood me in a big way. > > New languages are awesome. I play with new toys whenever I find the time > (and streamlining the contribution process would give me more time for > that). You are talking to a person who implemented the Householder > transformation in Postscript, a 6502 assembler in Forth, and a music > composing system in Emacs Lisp. No language is to fancy for me. "me", as > in "me, personally". > > Now let's think about Git for a moment, and its language choices, and the > rationale behind them. The majority of the critically important code is > written in C. Is C a good language? Decent, yes, but of course also > limiting, Resource leaks are very easy to overlook, and we have a share of > them. No object orientation, so when we need to "subclass", we have no > compile time safety. The pre-processor constructs make static analysis > nigh impossible. Plenty of downsides. So why was it chosen? Developers are > *familiar* with it, that's why. Similar considerations apply to the use of > shell scripts, and to Perl, to a certain extent. Which is changing slowly over time IMHO. The "new generation" of developers may not have in-depth knowledge of shelll or perl any more, but rather python or java maybe. > > I am not talking about contrib/ of course. That's fair game, it contains > only non-critical/fringe stuff. > > Note that the same rationale goes for choosing to accept patch submissions > via mail to a list that is not subscribers-only. > > When it comes to inviting developers to contribute to your project, > personal preferences become irrelevant, the deciding factor becomes how > easy it is to join. Is the language popular, many developers already > familiar with it? Is the build system readily available? Are the > maintainers responsive? I agree. I really do. I had some discussion at lunch yesterday about different attitudes of open source projects towards new contributions. An example was the Eclipse projects that scare off potential new contributors (specially these fly by single patch submissions) as you need to interact through their instance of Gerrit after signing a CLA. Git on the other hand doesn't do a bad job, e.g. I started with a patch that ought to be a single shot drive by patch. About 780 others wrote one patch and were never seen again: $ git shortlog -sne |grep 1 |wc -l 788 So our process is not too bad for the time. However email is as a communication tool is dying [1] eventually? Let's examine if we get less one time contributions over time: for i in $(seq 11 -1 1) ; do x_1=$(git shortlog -sne --since $i.years --until $(($i-1)).years |grep 1 |wc -l) printf "$i\t$x_1\n" done 11 81 10 94 9175 8148 7139 6118 5108 4149 3109 2106 199 Looking at these numbers the number of one time patch contributions spiked 9 years ago and declined since then (4 years ago seems to be an outlier) And I do not think the decline of one off contributions is because Git as a project is in decline, but has other reasons. Maybe the project is in better shape now that there are less one-off worthy contributions? Or the contribution process is not appealing to a lot of new comers? [1] I could not find a scientific paper that evaluates communcation habits of people, but these may give a feel for it: https://www.quora.com/Why-are-young-people-abandoning-email https://news.ycombinator.com/item?id=3554466 http://www.twistimage.com/blog/archives/nobody-uses-email-anymore/ http://www.emailisnotdead.com/ > > I vividly remember my reaction to Darcs, for example. It's written in > Haskell. I am a mathematician originally, so Haskell appeals to me. Did
Re: [PATCH v4 8/8] status: tests for --porcelain=v2
Jeff Hostetler writes: > +## > +## Confirm output prior to initial commit. > +## > + > +test_expect_success pre_initial_commit_0 ' Bikeshedding, but our codebase seems to prefer "expect" vs "actual". $ git grep -e 'test_cmp expect ' t/ | wc -l 1882 $ git grep -e 'test_cmp expected ' t/ | wc -l 888 > + cat >expected <<-EOF && > + # branch.oid (initial) > + # branch.head master > + ? actual > + ? dir1/ > + ? expected > + ? file_x > + ? file_y > + ? file_z > + EOF Perhaps throw these two entries to .gitignore to allow new tests in the future could also use expect.1 vs actual.1 and somesuch? cat >.gitignore <<-\EOF && expect* actual* EOF > +test_expect_success pre_initial_commit_1 ' > + git add file_x file_y file_z dir1 && > + SHA_A=`git hash-object -t blob -- dir1/file_a` && > + SHA_B=`git hash-object -t blob -- dir1/file_b` && > + SHA_X=`git hash-object -t blob -- file_x` && > + SHA_Y=`git hash-object -t blob -- file_y` && > + SHA_Z=`git hash-object -t blob -- file_z` && Please use $(commannd) instead of `command`. Also "SHA" is probably a bad prefix; either use "SHA_1" to be technically correct, or better yet use "OID", as we are moving towards abstracting the exact hash function name away. > + SHA_ZERO= && I think we made $_z40 available to you from t/test-lib.sh. > +## Try -z on the above > +test_expect_success pre_initial_commit_2 ' > + cat >expected.lf <<-EOF && > + # branch.oid (initial) > + # branch.head master > + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_A dir1/file_a > + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_B dir1/file_b > + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_X file_x > + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Y file_y > + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Z file_z > + ? actual > + ? expected > + EOF > + perl -pe y/\\012/\\000/ expected && > + rm expected.lf && As you immediately remove expected.lf, the first "cat" process is rather pointless. You can redirect here text <<-EOF directly into perl instead. Also it would probably help to add a new helper "lf_to_nul" in t/test-lib-functions.sh around the place where nul_to_q, ..., tz_to_tab_space helpers are defined, which would allow us to say lf_to_nul >expect <<-EOF && ... EOF > +test_expect_success initial_commit_3 ' > + git mv file_y renamed_y && > + H0=`git rev-parse HEAD` && > + > + cat >expected.q <<-EOF && > + # branch.oid $H0 > + # branch.head master > + 1 M. N... 100644 100644 100644 $SHA_X $SHA_X1 file_x > + 1 D. N... 100644 00 00 $SHA_Z $SHA_ZERO file_z > + 2 R. N... 100644 100644 100644 $SHA_Y $SHA_Y R100 renamed_yQfile_y > + ? actual > + ? expected > + EOF > + q_to_tab expected && > + rm expected.q && The same comment applies (redirect directly into q_to_tab). > +## > +## Ignore a file > +## > + > +test_expect_success ignore_file_0 ' > + echo x.ign >.gitignore && > + echo "ignore me" >x.ign && > + H1=`git rev-parse HEAD` && > + > + cat >expected <<-EOF && > + # branch.oid $H1 > + # branch.head master > + ? .gitignore > + ? actual > + ? expected > + ! x.ign > + EOF > + > + git status --porcelain=v2 --branch --ignored --untracked-files=all > >actual && > + rm x.ign && > + rm .gitignore && > + test_cmp expected actual > +' You do not seem to be checking a feature is not triggered when not asked throughout this test, e.g. making sure the output does not have the "# branch.*" lines when --branch is not given, "! x.ign" is not shown when --ignored is not given, etc. > +## > +## Test upstream fields in branch header > +## > + > +test_expect_success 'upstream_fields_0' ' > + git checkout master && > + git clone . sub_repo && > + ( > + ## Confirm local master tracks remote master. > + cd sub_repo && > + HUF=`git rev-parse HEAD` && > + ... > + git status --porcelain=v2 --branch --ignored > --untracked-files=all >actual && > + test_cmp expected actual > + ) && > + rm -rf sub_repo It probably is a good idea to use test_when_finished immediately before "git clone . sub_repo" to arrange this to happen even when any test in the subshell fails. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: [PATCH v4 6/8] status: print branch info with --porcelain=v2 --branch
On 08/05/2016 01:01 PM, Junio C Hamano wrote:
Jeff Hostetler writes:
/*
+ * Print branch information for porcelain v2 output. These lines
+ * are printed when the '--branch' parameter is given.
+ *
+ *# branch.oid
+ *# branch.head
Just bikeshedding, but ...
+ if (!s->branch)
+ fprintf(s->fp, "# branch.head %s%c", "(unknown)", eol);
+ else {
+ if (!strcmp(s->branch, "HEAD")) {
+ fprintf(s->fp, "# branch.head %s%c", "(detached)", eol);
+
+ if (state.rebase_in_progress ||
state.rebase_interactive_in_progress)
+ branch_name = state.onto;
+ else if (state.detached_from)
+ branch_name = state.detached_from;
+ else
+ branch_name = "";
+ } else {
+ branch_name = NULL;
+ skip_prefix(s->branch, "refs/heads/", &branch_name);
+
+ fprintf(s->fp, "# branch.head %s%c", branch_name, eol);
... given that we are showing branch name, perhaps "branch.name"
instead of "branch.head" is more appropriate?
Either way is fine with me. I just saw your comments on
commit v4-7/8. And leaving it as is is fine.
I wondered if "# " prefix before these lines is useful, by the way,
and initially thought that the fact that these lines begin with
"branch." and not with the "1/2/u $key" sufficient clue for whoever
reads them, but the reader can tell which kind of record it is by
reading the first two characters of each line (i.e. if "# " that is
not the usual "change info for a single file"), so it is actually a
good idea.
Yes, I used the "#" prefix to indicate a header line so that
parsers can always look at the first character and decide.
I set it up so the "--branch" argument creates "# branch.*" headers.
In my first patch series, I included worktree state information
(such as in-a-rebase and the rebase step counts), but it was thought
that that should be an independent effort. So, using this model,
if we later do add a "--state" argument, it would create "# state.*"
headers. And we would not have to change the --porcelain=v2 version
number.
Jeff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] more archives of this list
Jeff King wrote:
> On Fri, Aug 05, 2016 at 05:28:05AM -0400, Jeff King wrote:
> > I do find it visually a little harder to navigate through threads,
> > because there's not much styling there, and the messages seem to run
> > into one another. I don't know if a border around the divs or something
> > would help. I'm really terrible at that kind of visual design.
>
> I took a peek at doing something simple like:
>
> pre {
> border-style: solid;
> border-width: 1px;
> background: #dd;
> }
>
> but it looks like there's no HTML structure at all to the current
> output. It's just one big tag with various levels of indentation
> to represent the messages.
I added an between each message so the /T/ view ought to be more
readable:
https://public-inbox.org/meta/20160805181459.24420-...@80x24.org/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 7/8] git-status.txt: describe --porcelain=v2 format
On 08/05/2016 01:50 PM, Junio C Hamano wrote: Jeff Hostetler writes: +Porcelain Format Version 2 +~~ + +Version 2 format adds more detailed information about the state of +the worktree and changed items. Version 2 also defines an extensible +set of easy to parse optional headers. + +Header lines start with "#" and are added in response to specific +command line arguments. Parsers should ignore headers they +don't recognize. + +### Branch Headers + +If `--branch` is given, a series of header lines are printed with +information about the current branch. + +Line Notes + +# branch.oid | (initial)Current commit. +# branch.head | (detached) Current branch. In an earlier review I made a noise about "branch.head", but reading this together with others in a context, "branch.head" is good and I do not see a need to bikeshed it into "branch.name". right. +# branch.upstream If upstream is set. +# branch.ab + - If upstream is set and + the commit is present. + + +### Changed Tracked Entries + +Following the headers, a series of lines are printed for tracked +entries. One of three different line formats may be used to describe +an entry depending on the type of change. Tracked entries are printed +in an undefined order; parsers should allow for a mixture of the 3 +line types in any order. + +Ordinary changed entries have the following format: + +1 + +Renamed or copied entries have the following format: + +2 + +Field Meaning + +A 2 character field containing the staged and +unstaged XY values described in the short format, +with unchanged indicated by a "." rather than +a space. + A 4 character field describing the submodule state. +"N..." when the entry is not a submodule. +"S" when the entry is a submodule. + is "C" if the commit changed; otherwise ".". + is "M" if it has tracked changes; otherwise ".". + is "U" if there are untracked changes; otherwise ".". +The 6 character octal file mode in the HEAD. We do want "octal" to be spelled out, but I doubt that we want to guarantee to the reading scripts that this will always be 6 characters. good point. "octal" is sufficient. +The octal file mode in the index. +The octal file mode in the worktree. +The SHA1 value in the HEAD. +The SHA1 value in the index. Please avoid inventing a word "SHA1 value" that does not appear in Documentation/glossary-content.txt; s/SHA1 value/object name/g. This comment applies to the desciption for "U" lines below, too. ok. + The rename or copied percentage score. For example "R100" +or "C75". Documentation/diff-format.txt seems to call this differently. The rename or copy "score" number, e.g. "R100", "C75". perhaps? right, this is the same score. + The current pathname. Unless you are talking about "2" line, saying "current" is somewhat weird. I _think_ is the name in the index and in the working tree, as opposed to the original path in the HEAD, but the distinction does not matter for "1" or "U" lines. Right. The index path, since we only report renames for head-vs-index. As this table is meant to be shared between "1" and "2", perhaps The pathname. In a renamed/copied entry, this is the path in the index and in the working tree. The pathname in the commit at HEAD. This is only present in a renamed/copied entry, and tells where the renamed/copied contents came from. or something like that may clarify it a bit better. Then in the table for "U" lines, we can just say "The pathname". + When the `-z` option is used, the 2 pathnames are separated +with a NUL (ASCII 0x00) byte; otherwise, a tab (ASCII 0x09) +byte separates them. + The original path. This is only present when the entry +has been renamed or copied. If it is explained as "the original path", wouldn't it be easier to read if it were called "" instead of ""? yes. +When the `-z` option is given, pathnames are printed as is and +without any quoting and lines are terminated with a NUL (ASCII 0x00) +byte. + +Otherwise, all pathnames will be "C-quoted" if they contain any tab, +linefeed, double quote, or backslash characters. In C-quoting, these +characters will be replaced with the corresponding C-style escape +sequences and the resulting pathname will be double quote
Re: [PATCH v4 8/8] status: tests for --porcelain=v2
On 08/05/2016 02:12 PM, Junio C Hamano wrote: Jeff Hostetler writes: +## +## Confirm output prior to initial commit. +## + +test_expect_success pre_initial_commit_0 ' Bikeshedding, but our codebase seems to prefer "expect" vs "actual". $ git grep -e 'test_cmp expect ' t/ | wc -l 1882 $ git grep -e 'test_cmp expected ' t/ | wc -l 888 + cat >expected <<-EOF && + # branch.oid (initial) + # branch.head master + ? actual + ? dir1/ + ? expected + ? file_x + ? file_y + ? file_z + EOF Perhaps throw these two entries to .gitignore to allow new tests in the future could also use expect.1 vs actual.1 and somesuch? cat >.gitignore <<-\EOF && expect* actual* EOF +test_expect_success pre_initial_commit_1 ' + git add file_x file_y file_z dir1 && + SHA_A=`git hash-object -t blob -- dir1/file_a` && + SHA_B=`git hash-object -t blob -- dir1/file_b` && + SHA_X=`git hash-object -t blob -- file_x` && + SHA_Y=`git hash-object -t blob -- file_y` && + SHA_Z=`git hash-object -t blob -- file_z` && Please use $(commannd) instead of `command`. Also "SHA" is probably a bad prefix; either use "SHA_1" to be technically correct, or better yet use "OID", as we are moving towards abstracting the exact hash function name away. + SHA_ZERO= && I think we made $_z40 available to you from t/test-lib.sh. +## Try -z on the above +test_expect_success pre_initial_commit_2 ' + cat >expected.lf <<-EOF && + # branch.oid (initial) + # branch.head master + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_A dir1/file_a + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_B dir1/file_b + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_X file_x + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Y file_y + 1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Z file_z + ? actual + ? expected + EOF + perl -pe y/\\012/\\000/ expected && + rm expected.lf && As you immediately remove expected.lf, the first "cat" process is rather pointless. You can redirect here text <<-EOF directly into perl instead. Also it would probably help to add a new helper "lf_to_nul" in t/test-lib-functions.sh around the place where nul_to_q, ..., tz_to_tab_space helpers are defined, which would allow us to say lf_to_nul >expect <<-EOF && ... EOF +test_expect_success initial_commit_3 ' + git mv file_y renamed_y && + H0=`git rev-parse HEAD` && + + cat >expected.q <<-EOF && + # branch.oid $H0 + # branch.head master + 1 M. N... 100644 100644 100644 $SHA_X $SHA_X1 file_x + 1 D. N... 100644 00 00 $SHA_Z $SHA_ZERO file_z + 2 R. N... 100644 100644 100644 $SHA_Y $SHA_Y R100 renamed_yQfile_y + ? actual + ? expected + EOF + q_to_tab expected && + rm expected.q && The same comment applies (redirect directly into q_to_tab). +## +## Ignore a file +## + +test_expect_success ignore_file_0 ' + echo x.ign >.gitignore && + echo "ignore me" >x.ign && + H1=`git rev-parse HEAD` && + + cat >expected <<-EOF && + # branch.oid $H1 + # branch.head master + ? .gitignore + ? actual + ? expected + ! x.ign + EOF + + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual && + rm x.ign && + rm .gitignore && + test_cmp expected actual +' You do not seem to be checking a feature is not triggered when not asked throughout this test, e.g. making sure the output does not have the "# branch.*" lines when --branch is not given, "! x.ign" is not shown when --ignored is not given, etc. +## +## Test upstream fields in branch header +## + +test_expect_success 'upstream_fields_0' ' + git checkout master && + git clone . sub_repo && + ( + ## Confirm local master tracks remote master. + cd sub_repo && + HUF=`git rev-parse HEAD` && + ... + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual && + test_cmp expected actual + ) && + rm -rf sub_repo It probably is a good idea to use test_when_finished immediately before "git clone . sub_repo" to arrange this to happen even when any test in the subshell fails. Lots of good points here (and on the earlier commits in this series). I'll address and send up a new version shortly.
Re: [PATCH 3/7] trace: use warning() for printing trace errors
Christian Couder writes: > On Thu, Aug 4, 2016 at 11:28 PM, Junio C Hamano wrote: >> Jeff King writes: >> >>> I wondered if that would then let us drop set_warn_routine(), but it >>> looks like there are other warning() calls it cares about. So that would >>> invalidate the last paragraph here, though I still think converting the >>> trace errors to warning() is a reasonable thing to do. >> >> Yes. That is why tonight's pushout will have this series in 'jch' >> (that is a point on a linear history between 'master' and 'pu') and >> tentatively ejects cc/apply-am topic out of 'pu', expecting it to be >> rerolled. > > Ok, I will reroll soon with Peff's suggested changes. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
From: "Duy Nguyen" On Wed, Aug 3, 2016 at 6:07 PM, Johannes Schindelin wrote: It would be a totally different matter, of course, if you used the branches I publish via my GitHub repository, added fixup! and squash! commits, published the result to a public repository and then told me to pull from there, that would make things easier. We could even introduce a reword! construct, to make the review of the suggested edits of the commit message easier. On the topic of fixup and squash and everything. Is anyone else annoyed that the commit title is taken for fixup!, squash! instructions? After you have added a few of them, "git log --oneline" becomes useless. All you see is "fixup! A", "fixup! A", "fixup! B", "fixup! A". Would it be better to let the user control the title? We still need the cue "fixup!", "squash!"... at the beginning of the title, but the original commit reference is appended at the end, like s-o-b lines. In the same vein, I always want,with my workflow, to use "fixup! ". This would be to save trying to retype the title correctly, and simply use the abbreviated sha1, which would nicely allow an extra short summaty of what the fixup is about. Philip -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
Johannes Schindelin wrote: > On Thu, 4 Aug 2016, Stefan Beller wrote: > > git send-email/format-patch recently learned to include a base commit > > You may have noticed that my mail-patch-series.sh-generated code > submissions contain that base commit. But they still do not contain the > SHA-1s of my local commits corresponding to the patches, and even if they > did, the replies with suggested edits would most likely have lost said > information. > > I also hate to break it to you that git-send-email is not going to be part > of any solution. I think it ought to be. Some reasons I like emailing patches are: * there's no taking it back once it's sent * it's backed up within seconds by thousands of subscribers :) * doesn't require the reader to have an active connection to fetch out-of-band * doesn't require the reader to be on the same machine capable of cloning/building the project There are times when I've been on a slow machine, or offline when I wanted to read some patches. However, I do like including a pull request in cover letters of a patch series (not necessary for one-offs). But on a side note, I also find it depressing that SMTP is uncompressed and TLS compression is (still?) unsafe. At least I use ssh tunnels w/ compression for IMAP/SMTP to my own server. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] convert: add filter..process option
Lars Schneider wrote:
> > On 27 Jul 2016, at 11:41, Eric Wong wrote:
> > larsxschnei...@gmail.com wrote:
> >> +static int apply_protocol_filter(const char *path, const char *src,
> >> size_t len,
> >> + int fd, struct strbuf *dst,
> >> const char *cmd,
> >> + const char *filter_type)
> >> +{
> >
> >
> >
> >> + if (fd >= 0 && !src) {
> >> + ret &= fstat(fd, &file_stat) != -1;
> >> + len = file_stat.st_size;
> >
> > Same truncation bug I noticed earlier; what I originally meant
> > is the `len' arg probably ought to be off_t, here, not size_t.
> > 32-bit x86 Linux systems have 32-bit size_t (unsigned), but
> > large file support means off_t is 64-bits (signed).
>
> OK. Would it be OK to keep size_t for this patch series?
I think there should at least be a truncation warning (or die)
for larger-than-4GB files on 32-bit. I don't know how common
they are for git-lfs users.
Perhaps using xsize_t in git-compat-util.h works for now:
len = xsize_t(file_stat.st_size);
(sorry, I haven't had much time to look at your other updates)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references
> - ${reference:+--reference "$reference"} \
> + ${reference:+"$reference"} \
Note how this changed the API of the submodule--helper.
Currently we pass in --reference $reference
and $reference consists of the string "--reference" and the actual
reference. So it looked like '--reference' '--reference=foo'
That is why we can pass the argument unseen to clone in the helper
via
argv_array_push(&child->args, suc->reference);
This is fixed now.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
git fatal error on fetching
Debian Testing with v4.5.5-1 kernel, git v2.8.1.
I seem to have a broken git setup somehow on this machine - I'm
currently testing the ability to fetch PRs from Github following [0],
e.g. testing with a PR on dattobd[1]:
git clone https://github.com/datto/dattobd.git
cd dattobd
git fetch origin pull/41/head:pull_41
git checkout pull_41
This works fine in a VM, but on my main machine, the fetch fails:
fatal: failed to read object :
Bad file descriptor
I moved my local config out of the way ('~/.gitconfig'), but nothing
changed. Any ideas for how to debug this?
Thanks for any help.
0: https://gist.github.com/piscisaureus/3342247#gistcomment-1625076
1: https://github.com/datto/dattobd/pull/41
signature.asc
Description: OpenPGP digital signature
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
On Mon, Sep 17, 2001 at 10:00:00AM +, Stefan Beller wrote: > But both send-email as well as mail-patch-series as well as git-series > are all about the *sending* part. Not about the back and forth part, i.e. > these don't deal with: "here is a fixup on top". And by that I mean > receiving mails and applying them. git-am is there as a front-end > once you obtained the mail, but from what I get, your original problem > is to get up to date with the latest state, that is either in pu or a proposed > fixup mail on top of your series? git-series, at least, is intended to handle the back-and-forth: if you actually publish the series and not just the final result, someone could pull the series, make a (non-fast-forwarding) change to that, make a new series commit, and publish their modified version of your series. You could then incorporate that change. One of the use cases I developed it for was collaborative development of a patch series. (That workflow still needs a lot more tool assistance to become fully usable, not least of which to assist with the process of merging changes to the series. Working on that.) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] git clone: Marry --recursive and --reference
Stefan Beller writes:
> Currently when cloning a superproject with --recursive and --reference
> only the superproject learns about its alternates. The submodules are
> cloned independently, which may incur lots of network costs.
>
> Assume that the reference repository has the submodules at the same
> paths as the to-be-cloned submodule and try to setup alternates from
> there.
I am a bit fuzzy about what scenario you are trying to help. Is it
something like this? [*1*]
* The superproject is at git://frotz.xz/super.git but you locally
have a mirror at /var/cache/super.git (bare) repository.
* "git clone --refererence /var/cache/super.git git://frotz.xz/super.git"
would borrow that existing local mirror to cut down the traffic.
* The super.git project uses git://nitfol.xz/xyzzy.git as its
submodule. You locally have a mirror of it at
/var/cache/xyzzy.git (bare) repository.
* The xyzzy submodule is bound at libs/xyzzy in the superproject
tree.
* You want the "clone" command above with "--recursive" to do "the
right thing". That is, the clone of the superproject borrows
from /var/cache/super.git local mirror, and the clone of xyzzy
that would be made at .git/modules/xyzzy in the superproject
would borrow from /var/cache/xyzzy.git local mirror.
What I am not sure about is how /var/cache/xyzzy.git should be
automatically derived from the information given from the command
line of "clone" and what the clone of the superproject contains.
You'd need to make certain assumptions, like
- The parent directory of the --reference for superproject,
i.e. /var/cache/, is the directry that has many other mirrors;
- The .gitmodules file in superproject says a "xyzzy" project from
git://nitfol.xz/xyzzy.git must be checked out at "libs/xyzzy",
and it is a reasonable assumption that we would find a local
mirror at /var/cache/xyzzy.git (the /var/cache prefix comes from
the above assumptino).
There may be some other assumptions you are making. I am wondering
if your specific assumption is applicable generally.
[Footnote]
*1* Another common arrangement could be that you have one full clone
of the superproject at /var/cache/super.git and its submodule
xyzzy in /var/cache/super.git/modules/xyzzy and assuming that
would allow you to guess where to borrow submodules from. But
the result of assuming this layout would already be different
from the scenario I gave the above. If the organization uses
the xyzzy project only in the context of the frotz superproject,
"borrow from submodule repository inside $GIT_DIR/modules/ of
the superproject" may be sensible, but that makes use of "xyzzy"
as an independent project quite awkward (also you may have
another superproject that is unrelated to super.git that uses
the same "xyzzy" project as its submodule). The more "xyzzy"
project is suitable to be used as "submodule", the more it makes
sense to have its mirror independent from the superprojects that
use it, by using /var/cache/{super,xyzzy}.git layout.
IOW, both layouts are equally sensible; what layout (either one
of the above two, or something entirely different) is your "at
the same paths" assumption meant to serve well, and what is the
plan to serve other layouts?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] more archives of this list
On Fri, Aug 05, 2016 at 05:04:27PM +0200, Duy Nguyen wrote: > On Fri, Aug 5, 2016 at 11:28 AM, Jeff King wrote: > > There was discussion a long time ago about storing a common zlib > > dictionary in the packfile and using it for all of the objects. I don't > > recall whether there were any patches, though. It does create some > > complications with serving clones/fetches, as they may ask for a subset > > of the objects (so you have to send them the whole dictionary, which may > > be a lot of overhead if you're only fetching a few objects). > > I'm nit picking since it's not actually "all objects". But pack v4 > patches have two dictionaries (or more? i don't remember) for commits > and trees :) I couldn't remember if that zlib stuff was part of packv4 or not. I like many of the ideas in pack v4, but I do worry a lot about the compatibility issues, as packv2 is the on-the-wire format. Being able to send bytes directly off disk with minimal processing is a key thing that makes running a large git hosting site practical. One of the things that makes nervous is having to do on-the-fly conversion when serving fetches and clones (but to be clear that is just gut nervousness; I haven't done any actual testing with the proto-packv4 patches). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] more archives of this list
On Fri, Aug 05, 2016 at 06:19:57PM +, Eric Wong wrote:
> Jeff King wrote:
> > On Fri, Aug 05, 2016 at 05:28:05AM -0400, Jeff King wrote:
> > > I do find it visually a little harder to navigate through threads,
> > > because there's not much styling there, and the messages seem to run
> > > into one another. I don't know if a border around the divs or something
> > > would help. I'm really terrible at that kind of visual design.
> >
> > I took a peek at doing something simple like:
> >
> > pre {
> > border-style: solid;
> > border-width: 1px;
> > background: #dd;
> > }
> >
> > but it looks like there's no HTML structure at all to the current
> > output. It's just one big tag with various levels of indentation
> > to represent the messages.
>
> I added an between each message so the /T/ view ought to be more
> readable:
>
> https://public-inbox.org/meta/20160805181459.24420-...@80x24.org/
Thanks. That's definitely an improvement. I still think the styling
could go further, but I don't expect you to do it. It's something I may
look into, but I should probably try to clear out my backlog of
"to-review" patches before I go off spending time on it. :)
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] t7408: modernize style
Stefan Beller writes: > No functional change intended. This commit only changes formatting > to the style we recently use, e.g. starting the body of a test with a > single quote on the same line as the header, and then having the test > indented in the following lines. > > Whenever we change directories, we do that in subshells. All look sensible; I think it is OK to also do minor and trivial tweaks here, but let's see what 2/6 does. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] use strbuf_addstr() instead of strbuf_addf() with "%s"
Call strbuf_addstr() for adding a simple string to a strbuf instead of
using the heavier strbuf_addf(). This is shorter and documents the
intent more clearly.
Signed-off-by: Rene Scharfe
---
builtin/fmt-merge-msg.c | 2 +-
http.c | 2 +-
sequencer.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index e5658c3..ac84e99 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -272,7 +272,7 @@ static int cmp_string_list_util_as_integral(const void *a_,
const void *b_)
static void add_people_count(struct strbuf *out, struct string_list *people)
{
if (people->nr == 1)
- strbuf_addf(out, "%s", people->items[0].string);
+ strbuf_addstr(out, people->items[0].string);
else if (people->nr == 2)
strbuf_addf(out, "%s (%d) and %s (%d)",
people->items[0].string,
diff --git a/http.c b/http.c
index e81dd13..cd40b01 100644
--- a/http.c
+++ b/http.c
@@ -1225,7 +1225,7 @@ void append_remote_object_url(struct strbuf *buf, const
char *url,
strbuf_addf(buf, "objects/%.*s/", 2, hex);
if (!only_two_digit_prefix)
- strbuf_addf(buf, "%s", hex+2);
+ strbuf_addstr(buf, hex + 2);
}
char *get_remote_object_url(const char *url, const char *hex,
diff --git a/sequencer.c b/sequencer.c
index cdfac82..7b1eb14 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -112,7 +112,7 @@ static void remove_sequencer_state(void)
{
struct strbuf seq_dir = STRBUF_INIT;
- strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));
+ strbuf_addstr(&seq_dir, git_path(SEQ_DIR));
remove_dir_recursively(&seq_dir, 0);
strbuf_release(&seq_dir);
}
--
2.9.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] use CHILD_PROCESS_INIT to initialize automatic variables
Initialize struct child_process variables already when they're defined.
That's shorter and saves a function call.
Signed-off-by: Rene Scharfe
---
builtin/submodule--helper.c | 3 +--
builtin/worktree.c | 6 ++
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b22352b..957f42a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -444,8 +444,7 @@ static int module_name(int argc, const char **argv, const
char *prefix)
static int clone_submodule(const char *path, const char *gitdir, const char
*url,
const char *depth, const char *reference, int quiet)
{
- struct child_process cp;
- child_process_init(&cp);
+ struct child_process cp = CHILD_PROCESS_INIT;
argv_array_push(&cp.args, "clone");
argv_array_push(&cp.args, "--no-checkout");
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5a41788..6dcf7bd 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -194,7 +194,7 @@ static int add_worktree(const char *path, const char
*refname,
struct strbuf sb = STRBUF_INIT;
const char *name;
struct stat st;
- struct child_process cp;
+ struct child_process cp = CHILD_PROCESS_INIT;
struct argv_array child_env = ARGV_ARRAY_INIT;
int counter = 0, len, ret;
struct strbuf symref = STRBUF_INIT;
@@ -273,7 +273,6 @@ static int add_worktree(const char *path, const char
*refname,
argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
- memset(&cp, 0, sizeof(cp));
cp.git_cmd = 1;
if (commit)
@@ -365,8 +364,7 @@ static int add(int ac, const char **av, const char *prefix)
}
if (opts.new_branch) {
- struct child_process cp;
- memset(&cp, 0, sizeof(cp));
+ struct child_process cp = CHILD_PROCESS_INIT;
cp.git_cmd = 1;
argv_array_push(&cp.args, "branch");
if (opts.force_new_branch)
--
2.9.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] merge-recursive: use STRING_LIST_INIT_NODUP
Initialize a string_list right when it's defined. That's shorter, saves
a function call and makes it more obvious that we're using the NODUP
variant here.
Signed-off-by: Rene Scharfe
---
merge-recursive.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index a4a1195..97e6737 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -409,7 +409,7 @@ static void record_df_conflict_files(struct merge_options
*o,
* and the file need to be present, then the D/F file will be
* reinstated with a new unique name at the time it is processed.
*/
- struct string_list df_sorted_entries;
+ struct string_list df_sorted_entries = STRING_LIST_INIT_NODUP;
const char *last_file = NULL;
int last_len = 0;
int i;
@@ -422,7 +422,6 @@ static void record_df_conflict_files(struct merge_options
*o,
return;
/* Ensure D/F conflicts are adjacent in the entries list. */
- memset(&df_sorted_entries, 0, sizeof(struct string_list));
for (i = 0; i < entries->nr; i++) {
struct string_list_item *next = &entries->items[i];
string_list_append(&df_sorted_entries, next->string)->util =
--
2.9.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] t7408: merge short tests, factor out testing method
Stefan Beller writes:
> Tests consisting of one line each can be consolidated to have fewer tests
> to run as well as fewer lines of code.
>
> When having just a few git commands, do not create a new shell but
> use the -C flag in Git to execute in the correct directory.
Good motivations.
> Signed-off-by: Stefan Beller
> ---
> t/t7408-submodule-reference.sh | 50
> +++---
> 1 file changed, 18 insertions(+), 32 deletions(-)
>
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index afcc629..1416cbd 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -10,6 +10,16 @@ base_dir=$(pwd)
>
> U=$base_dir/UPLOAD_LOG
Is this line needed anywhere?
We (perhaps unfortunately) still need $base_dir because we want to
give an absolute file:/// URL to "submodule add", but I do not think
we use $U, so let's get rid of it.
> +test_alternate_usage()
> +{
According to Documentation/CodingGuidelines, this should be:
test_alternate_usage () {
Somehow the helper name sounds as if it is testing if an alternate
is used correctly (i.e. the machinery may attempt to use alternate
but not in a correct way), not testing if an alternate is correctly
used (i.e. the machinery incorrectly forgets to use an alternate at
all), but maybe it is just me.
> + alternates_file=$1
> + working_dir=$2
These are good (they can be on a single line), but you would
want &&-chain just as other lines.
> + test_line_count = 1 $alternates_file &&
This needs to quote "$alternates_file" especially in a helper
function you have no control over future callers of.
I wonder if we want to check the actual contents of the alternate;
it may force us to worry about the infamous "should we expect
$(pwd)/something or $PWD/something" if we did so, so it is not a
strong suggestion.
> + echo "0 objects, 0 kilobytes" >expect &&
> + git -C $working_dir count-objects >current &&
> + diff expect current
It is more customary to name two "expect" vs "actual", and compare
them using "test_cmp" not "diff".
> +}
> +
> test_expect_success 'preparing first repository' '
> test_create_repo A &&
> (
> @@ -42,44 +52,20 @@ test_expect_success 'preparing superproject' '
> )
> '
>
> -test_expect_success 'submodule add --reference' '
> +test_expect_success 'submodule add --reference uses alternates' '
> (
> cd super &&
> git submodule add --reference ../B "file://$base_dir/A" sub &&
> git commit -m B-super-added
> - )
> -'
> -
> -test_expect_success 'after add: existence of info/alternates' '
> - test_line_count = 1 super/.git/modules/sub/objects/info/alternates
> -'
> -
> -test_expect_success 'that reference gets used with add' '
> - (
> - cd super/sub &&
> - echo "0 objects, 0 kilobytes" > expected &&
> - git count-objects > current &&
> - diff expected current
> - )
> -'
Completely unrelated tangent, but after seeing the "how would we
make a more intelligent choice of the diff boundary" topic, I
wondered if we can notice that at this point there is a logical
boundary and do something intelligent about it. All the removed
lines above have become "test_alternate" we see below, while all the
removed lines below upto "test_alternate" correspond to the updated
test at the end.
> -test_expect_success 'cloning superproject' '
> - git clone super super-clone
> -'
> -
> -test_expect_success 'update with reference' '
> - cd super-clone && git submodule update --init --reference ../B
> -'
> -
> -test_expect_success 'after update: existence of info/alternates' '
> - test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates
> + ) &&
> + test_alternate_usage super/.git/modules/sub/objects/info/alternates
> super/sub
> '
>
> -test_expect_success 'that reference gets used with update' '
> - cd super-clone/sub &&
> - echo "0 objects, 0 kilobytes" > expected &&
> - git count-objects > current &&
> - diff expected current
> +test_expect_success 'updating superproject keeps alternates' '
> + test_when_finished "rm -rf super-clone" &&
This one is new; we do not remove A, B or super. Does it matter if
we leave super-clone behind? Is super-clone so special?
> + git clone super super-clone &&
> + git -C super-clone submodule update --init --reference ../B &&
> + test_alternate_usage
> super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
> '
>
> test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] submodule--helper module-clone: allow multiple references
Stefan Beller writes: > Allow users to pass in multiple references, just as clone accepts multiple > references as well. As these are passed-thru to the underlying "git clone", this change makes perfect sense to me. Will queue. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] wt-status.c: mark a file-local symbol as static
Signed-off-by: Ramsay Jones
---
Hi Jeff,
If you need to re-roll your 'jh/status-v2-porcelain' branch, could
you please squash this into the relevant patch (37f7104f, "status:
print per-file porcelain v2 status data", 02-08-2016).
Thanks!
ATB,
Ramsay Jones
wt-status.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/wt-status.c b/wt-status.c
index 3396bf5..a80400e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2152,7 +2152,7 @@ static void wt_porcelain_v2_print_other(
* []*
*
*/
-void wt_porcelain_v2_print(struct wt_status *s)
+static void wt_porcelain_v2_print(struct wt_status *s)
{
struct wt_status_change_data *d;
struct string_list_item *it;
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] xdiffi.c: mark some file-local symbols as static
Signed-off-by: Ramsay Jones
---
Hi Michael,
If you need to re-roll your 'mh/diff-indent-heuristic' branch, could
you please squash this into the relevant patch (e199b6e2, "diff: improve
positioning of add/delete blocks in diffs", 04-08-2016).
Thanks!
ATB,
Ramsay Jones
xdiff/xdiffi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 80307f5..90226f8 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -499,7 +499,7 @@ struct split_measurement {
/*
* Fill m with information about a hypothetical split of xdf above line split.
*/
-void measure_split(const xdfile_t *xdf, long split, struct split_measurement
*m)
+static void measure_split(const xdfile_t *xdf, long split, struct
split_measurement *m)
{
long i;
@@ -557,7 +557,7 @@ void measure_split(const xdfile_t *xdf, long split, struct
split_measurement *m)
* Also see that project if you want to improve the weights based on, for
example,
* a larger or more diverse corpus.
*/
-int score_split(const struct split_measurement *m)
+static int score_split(const struct split_measurement *m)
{
/*
* A place to accumulate bonus factors (positive makes this index more
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] merge: use string_list_split() in add_strategies()
Call string_list_split() for cutting a space separated list into pieces
instead of reimplementing it based on struct strategy. The attr member
of struct strategy was not used split_merge_strategies(); it was a pure
string operation. Also be nice and clean up once we're done splitting;
the old code didn't bother freeing any of the allocated memory.
Signed-off-by: Rene Scharfe
---
builtin/merge.c | 44 ++--
1 file changed, 10 insertions(+), 34 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 19b3bc2..a95a801 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -30,6 +30,7 @@
#include "fmt-merge-msg.h"
#include "gpg-interface.h"
#include "sequencer.h"
+#include "string-list.h"
#define DEFAULT_TWOHEAD (1<<0)
#define DEFAULT_OCTOPUS (1<<1)
@@ -703,42 +704,17 @@ static int count_unmerged_entries(void)
return ret;
}
-static void split_merge_strategies(const char *string, struct strategy **list,
- int *nr, int *alloc)
-{
- char *p, *q, *buf;
-
- if (!string)
- return;
-
- buf = xstrdup(string);
- q = buf;
- for (;;) {
- p = strchr(q, ' ');
- if (!p) {
- ALLOC_GROW(*list, *nr + 1, *alloc);
- (*list)[(*nr)++].name = xstrdup(q);
- free(buf);
- return;
- } else {
- *p = '\0';
- ALLOC_GROW(*list, *nr + 1, *alloc);
- (*list)[(*nr)++].name = xstrdup(q);
- q = ++p;
- }
- }
-}
-
static void add_strategies(const char *string, unsigned attr)
{
- struct strategy *list = NULL;
- int list_alloc = 0, list_nr = 0, i;
-
- memset(&list, 0, sizeof(list));
- split_merge_strategies(string, &list, &list_nr, &list_alloc);
- if (list) {
- for (i = 0; i < list_nr; i++)
- append_strategy(get_strategy(list[i].name));
+ int i;
+
+ if (string) {
+ struct string_list list = STRING_LIST_INIT_DUP;
+ struct string_list_item *item;
+ string_list_split(&list, string, ' ', -1);
+ for_each_string_list_item(item, &list)
+ append_strategy(get_strategy(item->string));
+ string_list_clear(&list, 0);
return;
}
for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
--
2.9.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
Stefan Beller wrote:
> On Fri, Aug 5, 2016 at 1:20 AM, Johannes Schindelin
> wrote:
> > Yet another option would be to have a tool that integrates with the Git
> > repository of the Git mailing list represented by public-inbox.
>
> So my first reaction to that would be: you could push you patches to
> that public inbox and it is translated to emails sent on your behalf.
>
> Which is reinventing submitGit just in a more accessible language?
Maybe vger.kernel.org could open its submission (587) port like
Debian does. Unfortunately, this hurts the ability to Cc:
folks directly and makes vger even more of an SPOF
(I guess submitGit also has this problem)
Fwiw, I have a public-inbox for my miscellaneous patch barf at
s...@80x24.org - https://80x24.org/spew/
http://ou63pmih66umazou.onion/spew/
I will throw up my untested/half-tested patches for various
projects so I can still access it across various NATs and
firewalls.
Using Tor for send-email often works (depending on which
blacklists the exit node is on), but is totally optional
(you'd expose your IP).
==> ~/bin/spew <==
#!/bin/sh
exec torsocks git send-email \
--smtp-domain=80x24.org \
--smtp-debug=1 \
--smtp-server-port=submission \
--smtp-server=80x24.org \
--to ${dest-s...@80x24.org} \
--suppress-cc=all \
"$@"
All: Feel free to use it for any Free Software projects, too;
or better yet, host your own :)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Tue, Aug 02, 2016 at 10:12:14AM -0400, Jeff Hostetler wrote:
> +static void wt_porcelain_v2_print_unmerged_entry(
> + struct string_list_item *it,
> + struct wt_status *s)
> +{
> + struct wt_status_change_data *d = it->util;
> + const struct cache_entry *ce;
> + struct strbuf buf_current = STRBUF_INIT;
> + const char *path_current = NULL;
> + int pos, stage, sum;
> + struct {
> + int mode;
> + struct object_id oid;
> + } stages[3];
> + char *key;
> [...]
> + switch (d->stagemask) {
> + case 1: key = "DD"; break; /* both deleted */
> + case 2: key = "AU"; break; /* added by us */
> + case 3: key = "UD"; break; /* deleted by them */
> + case 4: key = "UA"; break; /* added by them */
> + case 5: key = "DU"; break; /* deleted by us */
> + case 6: key = "AA"; break; /* both added */
> + case 7: key = "UU"; break; /* both modified */
> + }
> [...]
> + fprintf(s->fp, "%c %s %s %06o %06o %06o %06o %s %s %s %s%c",
> + unmerged_prefix, key, submodule_token,
Coverity complains that "key" can be uninitialized here. I think it's
wrong, and just doesn't know that d->stagemask is constrained to 1-7.
But perhaps it is worth adding a:
default:
die("BUG: unhandled unmerged status %x", d->stagemask);
to the end of the switch. That would shut up Coverity, and give us a
better indication if our constraint is violated.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references
Stefan Beller writes:
>> - ${reference:+--reference "$reference"} \
>> + ${reference:+"$reference"} \
>
> Note how this changed the API of the submodule--helper.
> Currently we pass in --reference $reference
> and $reference consists of the string "--reference" and the actual
> reference. So it looked like '--reference' '--reference=foo'
So this change is a strict bugfix. The code without this patch
had cmd_update that places "--reference=foo" in $reference, but
the call to "git submodule--helper" it makes added an unnecessary
"--reference" in front of it.
I was wondering why there is no corresponding change to add
"--reference" on the code that calls cmd_update(). Thanks for
saving my time ;-)
Perhaps that needs to go into the log message, though. Allowing
multiple references is not that interesting from the POV of the
codebase immediately after this step and only deserves a "by the
way" mention.
Subject: submodule--helper: fix cmd_update()
cmd_update places "--reference=foo" in $reference, but the call to
"git submodule--helper" it makes adds an unnecessary "--reference"
in front of it. The underlying "git clone" was called with two
command options "--reference" and "--reference=foo" because of
this.
While at it, prepare the code to accept multiple references, as
the underlying "git clone" can happily take more than one.
or something like that.
Needless to say, "While at it" could become a separate step, and a
pure bugfix part may even want to become a separate and more urgent
topic that can go to 'maint', with a test to prevent regression.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Fri, Aug 5, 2016 at 2:02 PM, Jeff King wrote:
> On Tue, Aug 02, 2016 at 10:12:14AM -0400, Jeff Hostetler wrote:
>> + switch (d->stagemask) {
>> + case 1: key = "DD"; break; /* both deleted */
>> + ...
>> + case 7: key = "UU"; break; /* both modified */
>> + }
>> [...]
>> + fprintf(s->fp, "%c %s %s %06o %06o %06o %06o %s %s %s %s%c",
>> + unmerged_prefix, key, submodule_token,
>
> Coverity complains that "key" can be uninitialized here. I think it's
> wrong, and just doesn't know that d->stagemask is constrained to 1-7.
>
> But perhaps it is worth adding a:
>
> default:
> die("BUG: unhandled unmerged status %x", d->stagemask);
>
> to the end of the switch. That would shut up Coverity, and give us a
> better indication if our constraint is violated.
This is pure curiosity but I wonder if Coverity shuts up if we
instead switched on (d->stagemask & 07). Your "default: BUG"
suggestion is what we should use as a real fix, of course.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Fri, Aug 05, 2016 at 02:09:48PM -0700, Junio C Hamano wrote:
> On Fri, Aug 5, 2016 at 2:02 PM, Jeff King wrote:
> > On Tue, Aug 02, 2016 at 10:12:14AM -0400, Jeff Hostetler wrote:
> >> + switch (d->stagemask) {
> >> + case 1: key = "DD"; break; /* both deleted */
> >> + ...
> >> + case 7: key = "UU"; break; /* both modified */
> >> + }
> >> [...]
> >> + fprintf(s->fp, "%c %s %s %06o %06o %06o %06o %s %s %s %s%c",
> >> + unmerged_prefix, key, submodule_token,
> >
> > Coverity complains that "key" can be uninitialized here. I think it's
> > wrong, and just doesn't know that d->stagemask is constrained to 1-7.
> >
> > But perhaps it is worth adding a:
> >
> > default:
> > die("BUG: unhandled unmerged status %x", d->stagemask);
> >
> > to the end of the switch. That would shut up Coverity, and give us a
> > better indication if our constraint is violated.
>
> This is pure curiosity but I wonder if Coverity shuts up if we
> instead switched on (d->stagemask & 07). Your "default: BUG"
> suggestion is what we should use as a real fix, of course.
I suspect yes. It's pretty clever about analyzing the flow and placing
constraints on values (though in this case "& 07" includes "0", which is
not handled in the switch).
Unfortunately it is hard for me to test a one-off, as running it locally
is a complete pain. Stefan set it up long ago to pull "pu" and email out
the results from their central servers, so I just scan those emails for
things that look like real issues.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] submodule update: add super-reference flag
Stefan Beller writes:
> Signed-off-by: Stefan Beller
> ---
That's a bit sketchy description. From the title, I expected that
we would see one additional 'unsigned super_reference : 1;' field in
some structure, but that is not what is happening. The log message
needs to describe what these string list are and why they are needed
a bit better.
At least, please don't name a multiple_word field "multipleword" ;-)
> @@ -715,6 +716,15 @@ static int prepare_to_clone_next_submodule(const struct
> cache_entry *ce,
> for_each_string_list_item(item, &suc->references)
> argv_array_pushl(&child->args, "--reference",
> item->string, NULL);
> }
> + if (suc->superreferences.nr) {
> + struct string_list_item *item;
> + for_each_string_list_item(item, &suc->superreferences) {
> + strbuf_reset(&sb);
> + argv_array_pushf(&child->args, "--reference=%s/%s",
> + relative_path(item->string,
> suc->prefix, &sb),
> + sub->path);
The phrase "super reference" made me imagine it is some kind of
"reference" that is applicable to the superproject, but this code
smells like it is a "prefix to create reference suited for use in
submodule". Whatever it is, it should be explained better (than "no
desciption" which is what we have here ;-), and given a name that
match the explanation.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references
On Fri, Aug 5, 2016 at 2:06 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>>> - ${reference:+--reference "$reference"} \
>>> + ${reference:+"$reference"} \
>>
>> Note how this changed the API of the submodule--helper.
>> Currently we pass in --reference $reference
>> and $reference consists of the string "--reference" and the actual
>> reference. So it looked like '--reference' '--reference=foo'
>
> So this change is a strict bugfix. The code without this patch
> had cmd_update that places "--reference=foo" in $reference, but
> the call to "git submodule--helper" it makes added an unnecessary
> "--reference" in front of it.
I thought about rolling it as a strict bugfix; but the bug is shaded by the
inverse bug in the helper, so the user would never see an issue.
It is more a cleanup of the internal communication between the shell
script and the helper written in C. (And that communication is
supposed to not be user visible or relevant)
>
> I was wondering why there is no corresponding change to add
> "--reference" on the code that calls cmd_update(). Thanks for
> saving my time ;-)
When I wrote the patch I cared more about the while-at-it part than
the bug fix as I do not consider the bug worth to merge down to maint
or even fast tracking it as a bug fix. It's just changing the internal
communication to be clearer.
>
> Perhaps that needs to go into the log message, though. Allowing
> multiple references is not that interesting from the POV of the
> codebase immediately after this step and only deserves a "by the
> way" mention.
>
> Subject: submodule--helper: fix cmd_update()
>
> cmd_update places "--reference=foo" in $reference, but the call to
> "git submodule--helper" it makes adds an unnecessary "--reference"
> in front of it. The underlying "git clone" was called with two
> command options "--reference" and "--reference=foo" because of
> this.
>
> While at it, prepare the code to accept multiple references, as
> the underlying "git clone" can happily take more than one.
>
> or something like that.
>
> Needless to say, "While at it" could become a separate step, and a
> pure bugfix part may even want to become a separate and more urgent
> topic that can go to 'maint', with a test to prevent regression.
Sure. I'll do so in a reroll.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 07/12] run-command: add clean_on_exit_handler
On 2016-08-05 15.08, Lars Schneider wrote: [] > Yeah it could do that. But then the filter cannot do things like > modifying the index after the fact... however, that might be considered > nasty by the Git community anyways... I am thinking about dropping > this patch in the next roll as it is not strictly necessary for my > current use case. (Thanks Peff for helping me out with the EOF explanation) I would say that a filter is a filter, and should do nothing else than filtering one file, (or a stream). When you want to modify the index, a hook may be your friend. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: storing cover letter of a patch series?
On Friday, August 05, 2016 08:39:58 AM you wrote: > * A new topic, when you merge it to the "lit" branch, you > describe the cover as the merge commit message. > > * When you updated an existing topic, you tell a tool > like "rebase -i -p" to recreate "lit" branch on top of > the mainline. This would give you an opportunity to > update the cover. This is a neat idea. How would this work if there is no merge commit (mainline hasn't moved)? -Martin -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Fri, Aug 5, 2016 at 2:14 PM, Jeff King wrote: > > Unfortunately it is hard for me to test a one-off, as running it locally > is a complete pain. Stefan set it up long ago to pull "pu" and email out > the results from their central servers, so I just scan those emails for > things that look like real issues. Ah, running check on 'pu' is an excellent way to catch issues early, before they hit 'next'. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] git clone: Marry --recursive and --reference
On Fri, Aug 5, 2016 at 12:47 PM, Junio C Hamano wrote: > * You want the "clone" command above with "--recursive" to do "the >right thing". That is, the clone of the superproject borrows >from /var/cache/super.git local mirror, and the clone of xyzzy >that would be made at .git/modules/xyzzy in the superproject >would borrow from /var/cache/xyzzy.git local mirror. This is not what I intend to solve here. The solution in 6/6 solves the scenario as you outlined in [1]. > > What I am not sure about is how /var/cache/xyzzy.git should be > automatically derived from the information given from the command > line of "clone" and what the clone of the superproject contains. Generally speaking you cannot do that without assumptions. The scenario in [1] can be done without assumptions of the locations of the submodules. The only requirement for [1] is to have submodules checked out, which is a rather strong requirement, as that doesn't help you when you want to reference multiple superrpojects with incomplete submodule checkout. (Given all of them together may or may not produce the full set of references) > > IOW, both layouts are equally sensible; what layout (either one > of the above two, or something entirely different) is your "at > the same paths" assumption meant to serve well, and what is the > plan to serve other layouts? > The plan for other layouts might be git submodule update --reference-dir /var/cache/ ? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: storing cover letter of a patch series?
On Fri, Aug 5, 2016 at 2:20 PM, Martin Fick wrote: > On Friday, August 05, 2016 08:39:58 AM you wrote: >> * A new topic, when you merge it to the "lit" branch, you >> describe the cover as the merge commit message. >> >> * When you updated an existing topic, you tell a tool >> like "rebase -i -p" to recreate "lit" branch on top of >> the mainline. This would give you an opportunity to >> update the cover. > > This is a neat idea. How would this work if there is no > merge commit (mainline hasn't moved)? Sorry, I do not understand your question. You always merge into your own "lit", which is based on (some) version of the mainline. If a topic builds on top of the mainline, you "merge --no-ff" it into "lit". Because no merges on "lit" will be part of the future mainline anyway, even the project frowns upon a "no-ff" merge, that will not be a problem. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] wt-status.c: mark a file-local symbol as static
On 08/05/2016 04:55 PM, Ramsay Jones wrote:
Signed-off-by: Ramsay Jones
---
Hi Jeff,
If you need to re-roll your 'jh/status-v2-porcelain' branch, could
you please squash this into the relevant patch (37f7104f, "status:
print per-file porcelain v2 status data", 02-08-2016).
Thanks!
ATB,
Ramsay Jones
wt-status.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/wt-status.c b/wt-status.c
index 3396bf5..a80400e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2152,7 +2152,7 @@ static void wt_porcelain_v2_print_other(
* []*
*
*/
-void wt_porcelain_v2_print(struct wt_status *s)
+static void wt_porcelain_v2_print(struct wt_status *s)
{
struct wt_status_change_data *d;
struct string_list_item *it;
got it. thanks!
Jeff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On 08/05/2016 05:02 PM, Jeff King wrote:
On Tue, Aug 02, 2016 at 10:12:14AM -0400, Jeff Hostetler wrote:
+static void wt_porcelain_v2_print_unmerged_entry(
+ struct string_list_item *it,
+ struct wt_status *s)
+{
+ struct wt_status_change_data *d = it->util;
+ const struct cache_entry *ce;
+ struct strbuf buf_current = STRBUF_INIT;
+ const char *path_current = NULL;
+ int pos, stage, sum;
+ struct {
+ int mode;
+ struct object_id oid;
+ } stages[3];
+ char *key;
[...]
+ switch (d->stagemask) {
+ case 1: key = "DD"; break; /* both deleted */
+ case 2: key = "AU"; break; /* added by us */
+ case 3: key = "UD"; break; /* deleted by them */
+ case 4: key = "UA"; break; /* added by them */
+ case 5: key = "DU"; break; /* deleted by us */
+ case 6: key = "AA"; break; /* both added */
+ case 7: key = "UU"; break; /* both modified */
+ }
[...]
+ fprintf(s->fp, "%c %s %s %06o %06o %06o %06o %s %s %s %s%c",
+ unmerged_prefix, key, submodule_token,
Coverity complains that "key" can be uninitialized here. I think it's
wrong, and just doesn't know that d->stagemask is constrained to 1-7.
But perhaps it is worth adding a:
default:
die("BUG: unhandled unmerged status %x", d->stagemask);
to the end of the switch. That would shut up Coverity, and give us a
better indication if our constraint is violated.
-Peff
got it. thanks!
Jeff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references
Stefan Beller writes: > I thought about rolling it as a strict bugfix; but the bug is shaded by the > inverse bug in the helper, so the user would never see an issue. Ahh, OK, because the helper accepts "--reference" "--reference=foo" as a OPT_STRING whose value happens to be "--reference=foo", and then uses if (suc->reference) argv_array_push(&child->args, suc->reference) where suc->reference _is_ "--reference=foo" when invoking the underlying "git clone", it cancels out. Then it is OK. In fact there is NO bug. It just is that update_clone subcommand used a convention different from others that took the whole --option=arg as a parameter to --reference option. It could be argued that it is an API bug between git-submodule.sh and git-submodule--helper, but nobody else goes through this "weird" interface, so it is not worth splitting the patch. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references
On Fri, Aug 5, 2016 at 2:31 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> I thought about rolling it as a strict bugfix; but the bug is shaded by the >> inverse bug in the helper, so the user would never see an issue. > > Ahh, OK, because the helper accepts "--reference" "--reference=foo" > as a OPT_STRING whose value happens to be "--reference=foo", and > then uses > > if (suc->reference) > argv_array_push(&child->args, suc->reference) > > where suc->reference _is_ "--reference=foo" when invoking the > underlying "git clone", it cancels out. > > Then it is OK. > > In fact there is NO bug. It just is that update_clone subcommand > used a convention different from others that took the whole > --option=arg as a parameter to --reference option. It could be > argued that it is an API bug between git-submodule.sh and > git-submodule--helper, but nobody else goes through this "weird" > interface, so it is not worth splitting the patch. I'll mention the fix of the API bug in the reroll then. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 11/12] convert: add filter..process option
On 2016-08-03 18.42, larsxschnei...@gmail.com wrote: > The filter is expected to respond with the result content in zero > or more pkt-line packets and a flush packet at the end. Finally, a > "result=success" packet is expected if everything went well. > > packet: git< SMUDGED_CONTENT > packet: git< > packet: git< result=success\n > I would really send the diagnostics/return codes before the content. > If the result content is empty then the filter is expected to respond > only with a flush packet and a "result=success" packet. > > packet: git< > packet: git< result=success\n > Which may be: packet: git< result=success\n packet: git< SMUDGED_CONTENT packet: git< or for an empty file: packet: git< result=success\n packet: git< SMUDGED_CONTENT packet: git< or in case of an error: packet: git< result=reject\n # And this will not send the "" packet Does this makes sense ? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] clone: reference flag is used for submodules as well
Stefan Beller writes: > When giving a --reference while also giving --recurse, the alternates > for the submodules are assumed to be in the superproject as well. > > In case they are not, we error out when cloning the submodule. > However the update command succeeds as usual (with no alternates). I covered most of what I want to say on this in my reply to 0/6; I do not have strong objection against what single layout you chose to support, nor I have strong opinion on which single layout we should support by default, or what mechanism, if any, we should give users to specify different layout. But please make sure the choice you made is explained for the users. The end-user documentation should talk about the effect of giving these two options together. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Fri, Aug 5, 2016 at 2:14 PM, Jeff King wrote:
>
> Unfortunately it is hard for me to test a one-off, as running it locally
> is a complete pain. Stefan set it up long ago to pull "pu" and email out
> the results from their central servers, so I just scan those emails for
> things that look like real issues.
>
I am glad this provides actual value. :)
(I tend to ignore the emails nowadays as I am focusing on other stuff.
I'll get back to down the number issues eventually, I promise ;)
Running one offs is "relatively" easy.
I did that for other projects once upon a time.
Here is my script to run the build:
#!/bin/bash
. .profile
cd coverity/git
git clean -dfx
git fetch --all
git checkout origin/pu
git am ../git_strbuf_stop_out_of_bounds_coverity.patch
descrip="scripted upload scanning github.com/gitster/git pu"
name=$(git describe)
cov-build --dir cov-int make
tar czvf git-${name}.tgz cov-int
curl --form project=git \
--form token= \
--form email= \
--form file=@git-${name}.tgz \
--form version="${name}" \
--form description="${descrip}" \
https://scan.coverity.com/builds?project=git
git clean -dfx
You can get a token if you look around, e.g. at
https://scan.coverity.com/projects/git/builds/new
and the email address is the one you signed up with
coverity (or the one from Github, if signed up via Github)
I am currently applying one patch on top of pu,
(id: 1434536209-31350-1-git-send-email-pclo...@gmail.com
I can resend if you don't have that.)
I am running this script as a cron job, but it can be run "as is"
as well, you'd just need to toss in the one-off test patch.
Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] git clone: Marry --recursive and --reference
Stefan Beller writes: > The plan for other layouts might be > > git submodule update --reference-dir /var/cache/ That is not a plan for "other layouts", but a plan for "the other layout that was mentioned as a possibility". As I said, both layouts are equally plausible, and I do not know if we need to plan for layouts other than these two, but we should consider if we want to add one new option every time we find a new layout we need to support, or we want a general framework that is more flexible and allows us to make the "borrow from the $GIT_DIR/modules/ of the repository the superproject borrows from" a mere special case of it. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/8] status: print per-file porcelain v2 status data
On Fri, Aug 5, 2016 at 2:43 PM, Stefan Beller wrote: > cov-build --dir cov-int make For this you need to download their analytic tool and put it in your $PATH. Let me see if I need to update the tool so it enables finding more potential issues. So that was an initial hurdle. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 11/12] convert: add filter..process option
> On 05 Aug 2016, at 23:34, Torsten Bögershausen wrote: > > On 2016-08-03 18.42, larsxschnei...@gmail.com wrote: >> The filter is expected to respond with the result content in zero >> or more pkt-line packets and a flush packet at the end. Finally, a >> "result=success" packet is expected if everything went well. >> >> packet: git< SMUDGED_CONTENT >> packet: git< >> packet: git< result=success\n >> > I would really send the diagnostics/return codes before the content. > >> If the result content is empty then the filter is expected to respond >> only with a flush packet and a "result=success" packet. >> >> packet: git< >> packet: git< result=success\n >> > > Which may be: > > packet: git< result=success\n > packet: git< SMUDGED_CONTENT > packet: git< > > or for an empty file: > > packet: git< result=success\n > packet: git< SMUDGED_CONTENT > packet: git< I think you meant: packet: git< result=success\n packet: git< Right? > > or in case of an error: > packet: git< result=reject\n > # And this will not send the "" packet > > Does this makes sense ? I see your point. However, I think your suggestion would not work in the true streaming case as the filter wouldn't know upfront if the operation will succeed, right? Thanks for the review, Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 07/12] run-command: add clean_on_exit_handler
> On 05 Aug 2016, at 23:19, Torsten Bögershausen wrote: > > On 2016-08-05 15.08, Lars Schneider wrote: > > [] >> Yeah it could do that. But then the filter cannot do things like >> modifying the index after the fact... however, that might be considered >> nasty by the Git community anyways... I am thinking about dropping >> this patch in the next roll as it is not strictly necessary for my >> current use case. > (Thanks Peff for helping me out with the EOF explanation) > > I would say that a filter is a filter, and should do nothing else than > filtering > one file, > (or a stream). > When you want to modify the index, a hook may be your friend. Agreed. I will remove that feature. Thanks, Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning
Johannes Schindelin writes:
> Hi Junio & René,
>
> On Thu, 4 Aug 2016, Junio C Hamano wrote:
>
>> Let's try it this way. How about this as a replacement?
>
> I like it (with the if (s2) test intead of if (s1), of course). But please
> record René as author, maybe mentioning myself with a "Diagnosed-by:"
> line.
Hmph. I cannot do that unilaterally without waiting for René to
respond, though. In any case, with only header and footer changes,
here is what will appear in 'pu'.
Thanks.
-- >8 --
From: René Scharfe
Date: Thu, 4 Aug 2016 23:56:54 +0200
Subject: [PATCH] nedmalloc: work around overzealous GCC 6 warning
With GCC 6, the strdup() function is declared with the "nonnull"
attribute, stating that it is not allowed to pass a NULL value as
parameter.
In nedmalloc()'s reimplementation of strdup(), Postel's Law is heeded
and NULL parameters are handled gracefully. GCC 6 complains about that
now because it thinks that NULL cannot be passed to strdup() anyway.
Because the callers in this project of strdup() must be prepared to
call any implementation of strdup() supplied by the platform, so it
is pointless to pretend that it is OK to call it with NULL.
Remove the conditional based on NULL-ness of the input; this
squelches the warning.
See https://gcc.gnu.org/gcc-6/porting_to.html for details.
Diagnosed-by: Johannes Schindelin
Signed-off-by: René Scharfe
Signed-off-by: Junio C Hamano
---
compat/nedmalloc/nedmalloc.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
index 677d1b2..2d4ef59 100644
--- a/compat/nedmalloc/nedmalloc.c
+++ b/compat/nedmalloc/nedmalloc.c
@@ -955,12 +955,11 @@ void **nedpindependent_comalloc(nedpool *p, size_t elems,
size_t *sizes, void **
*/
char *strdup(const char *s1)
{
- char *s2 = 0;
- if (s1) {
- size_t len = strlen(s1) + 1;
- s2 = malloc(len);
+ size_t len = strlen(s1) + 1;
+ char *s2 = malloc(len);
+
+ if (s2)
memcpy(s2, s1, len);
- }
return s2;
}
#endif
--
2.9.2-766-gd7972a8
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/9] status: cleanup API to wt_status_print
From: Jeff Hostetler
Refactor the API between builtin/commit.c and wt-status.[ch].
Hide the details of the various wt_*status_print() routines inside
wt-status.c behind a single (new) wt_status_print() routine.
Eliminate the switch statements from builtin/commit.c.
Allow details of new status formats to be isolated within wt-status.c
Signed-off-by: Jeff Hostetler
---
builtin/commit.c | 51 +--
wt-status.c | 25 ++---
wt-status.h | 16
3 files changed, 43 insertions(+), 49 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index b80273b..a792deb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -142,14 +142,7 @@ static int show_ignored_in_status, have_option_m;
static const char *only_include_assumed;
static struct strbuf message = STRBUF_INIT;
-static enum status_format {
- STATUS_FORMAT_NONE = 0,
- STATUS_FORMAT_LONG,
- STATUS_FORMAT_SHORT,
- STATUS_FORMAT_PORCELAIN,
-
- STATUS_FORMAT_UNSPECIFIED
-} status_format = STATUS_FORMAT_UNSPECIFIED;
+static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
static int opt_parse_m(const struct option *opt, const char *arg, int unset)
{
@@ -500,24 +493,11 @@ static int run_status(FILE *fp, const char *index_file,
const char *prefix, int
s->fp = fp;
s->nowarn = nowarn;
s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+ s->status_format = status_format;
+ s->ignore_submodule_arg = ignore_submodule_arg;
wt_status_collect(s);
-
- switch (status_format) {
- case STATUS_FORMAT_SHORT:
- wt_shortstatus_print(s);
- break;
- case STATUS_FORMAT_PORCELAIN:
- wt_porcelain_print(s);
- break;
- case STATUS_FORMAT_UNSPECIFIED:
- die("BUG: finalize_deferred_config() should have been called");
- break;
- case STATUS_FORMAT_NONE:
- case STATUS_FORMAT_LONG:
- wt_longstatus_print(s);
- break;
- }
+ wt_status_print(s);
return s->commitable;
}
@@ -1099,7 +1079,7 @@ static const char *read_commit_message(const char *name)
* is not in effect here.
*/
static struct status_deferred_config {
- enum status_format status_format;
+ enum wt_status_format status_format;
int show_branch;
} status_deferred_config = {
STATUS_FORMAT_UNSPECIFIED,
@@ -1381,6 +1361,9 @@ int cmd_status(int argc, const char **argv, const char
*prefix)
s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
s.ignore_submodule_arg = ignore_submodule_arg;
+ s.status_format = status_format;
+ s.verbose = verbose;
+
wt_status_collect(&s);
if (0 <= fd)
@@ -1389,23 +1372,7 @@ int cmd_status(int argc, const char **argv, const char
*prefix)
if (s.relative_paths)
s.prefix = prefix;
- switch (status_format) {
- case STATUS_FORMAT_SHORT:
- wt_shortstatus_print(&s);
- break;
- case STATUS_FORMAT_PORCELAIN:
- wt_porcelain_print(&s);
- break;
- case STATUS_FORMAT_UNSPECIFIED:
- die("BUG: finalize_deferred_config() should have been called");
- break;
- case STATUS_FORMAT_NONE:
- case STATUS_FORMAT_LONG:
- s.verbose = verbose;
- s.ignore_submodule_arg = ignore_submodule_arg;
- wt_longstatus_print(&s);
- break;
- }
+ wt_status_print(&s);
return 0;
}
diff --git a/wt-status.c b/wt-status.c
index b9a58fd..a9031e4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1447,7 +1447,7 @@ static void wt_longstatus_print_state(struct wt_status *s,
show_bisect_in_progress(s, state, state_color);
}
-void wt_longstatus_print(struct wt_status *s)
+static void wt_longstatus_print(struct wt_status *s)
{
const char *branch_color = color(WT_STATUS_ONBRANCH, s);
const char *branch_status_color = color(WT_STATUS_HEADER, s);
@@ -1714,7 +1714,7 @@ static void wt_shortstatus_print_tracking(struct
wt_status *s)
fputc(s->null_termination ? '\0' : '\n', s->fp);
}
-void wt_shortstatus_print(struct wt_status *s)
+static void wt_shortstatus_print(struct wt_status *s)
{
int i;
@@ -1746,7 +1746,7 @@ void wt_shortstatus_print(struct wt_status *s)
}
}
-void wt_porcelain_print(struct wt_status *s)
+static void wt_porcelain_print(struct wt_status *s)
{
s->use_color = 0;
s->relative_paths = 0;
@@ -1754,3 +1754,22 @@ void wt_porcelain_print(struct wt_status *s)
s->no_gettext = 1;
wt_shortstatus_print(s);
}
+
+void wt_status_print(struct wt_status *s)
+{
+ switch (s->status_format) {
+ case STATUS_FORMAT_SHORT:
+ wt_shortstatus_print(s);
+ break;
+ ca
[PATCH v5 6/9] status: print branch info with --porcelain=v2 --branch
From: Jeff Hostetler
Expand porcelain v2 output to include branch and tracking
branch information. This includes the commit id, the branch,
the upstream branch, and the ahead and behind counts.
Signed-off-by: Jeff Hostetler
---
builtin/commit.c | 5
wt-status.c | 90
wt-status.h | 1 +
3 files changed, 96 insertions(+)
diff --git a/builtin/commit.c b/builtin/commit.c
index 3d222d3..5504afe 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -510,6 +510,8 @@ static int run_status(FILE *fp, const char *index_file,
const char *prefix, int
s->fp = fp;
s->nowarn = nowarn;
s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+ if (!s->is_initial)
+ hashcpy(s->sha1_commit, sha1);
s->status_format = status_format;
s->ignore_submodule_arg = ignore_submodule_arg;
@@ -1378,6 +1380,9 @@ int cmd_status(int argc, const char **argv, const char
*prefix)
fd = hold_locked_index(&index_lock, 0);
s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
+ if (!s.is_initial)
+ hashcpy(s.sha1_commit, sha1);
+
s.ignore_submodule_arg = ignore_submodule_arg;
s.status_format = status_format;
s.verbose = verbose;
diff --git a/wt-status.c b/wt-status.c
index 4b6a090..51405d2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1814,6 +1814,92 @@ static void wt_porcelain_print(struct wt_status *s)
}
/*
+ * Print branch information for porcelain v2 output. These lines
+ * are printed when the '--branch' parameter is given.
+ *
+ *# branch.oid
+ *# branch.head
+ * [# branch.upstream
+ * [# branch.ab + -]]
+ *
+ * ::= the current commit hash or the the literal
+ * "(initial)" to indicate an initialized repo
+ * with no commits.
+ *
+ * ::= the current branch name or
+ * "(detached)" literal when detached head or
+ * "(unknown)" when something is wrong.
+ *
+ * ::= the upstream branch name, when set.
+ *
+ *::= integer ahead value, when upstream set
+ * and the commit is present (not gone).
+ *
+ * ::= integer behind value, when upstream set
+ * and commit is present.
+ *
+ *
+ * The end-of-line is defined by the -z flag.
+ *
+ * ::= NUL when -z,
+ * LF when NOT -z.
+ *
+ */
+static void wt_porcelain_v2_print_tracking(struct wt_status *s)
+{
+ struct branch *branch;
+ const char *base;
+ const char *branch_name;
+ struct wt_status_state state;
+ int ab_info, nr_ahead, nr_behind;
+ char eol = s->null_termination ? '\0' : '\n';
+
+ memset(&state, 0, sizeof(state));
+ wt_status_get_state(&state, s->branch && !strcmp(s->branch, "HEAD"));
+
+ fprintf(s->fp, "# branch.oid %s%c",
+ (s->is_initial ? "(initial)" :
sha1_to_hex(s->sha1_commit)),
+ eol);
+
+ if (!s->branch)
+ fprintf(s->fp, "# branch.head %s%c", "(unknown)", eol);
+ else {
+ if (!strcmp(s->branch, "HEAD")) {
+ fprintf(s->fp, "# branch.head %s%c", "(detached)", eol);
+
+ if (state.rebase_in_progress ||
state.rebase_interactive_in_progress)
+ branch_name = state.onto;
+ else if (state.detached_from)
+ branch_name = state.detached_from;
+ else
+ branch_name = "";
+ } else {
+ branch_name = NULL;
+ skip_prefix(s->branch, "refs/heads/", &branch_name);
+
+ fprintf(s->fp, "# branch.head %s%c", branch_name, eol);
+ }
+
+ /* Lookup stats on the upstream tracking branch, if set. */
+ branch = branch_get(branch_name);
+ base = NULL;
+ ab_info = (stat_tracking_info(branch, &nr_ahead, &nr_behind,
&base) == 0);
+ if (base) {
+ base = shorten_unambiguous_ref(base, 0);
+ fprintf(s->fp, "# branch.upstream %s%c", base, eol);
+ free((char *)base);
+
+ if (ab_info)
+ fprintf(s->fp, "# branch.ab +%d -%d%c",
nr_ahead, nr_behind, eol);
+ }
+ }
+
+ free(state.branch);
+ free(state.onto);
+ free(state.detached_from);
+}
+
+/*
* Convert various submodule status values into a
* fixed-length string of characters in the buffer provided.
*/
@@ -2059,6 +2145,7 @@ static void wt_porcelain_v2_print_other(
/*
* Print porcelain V2 status.
*
+ * []
* []*
* []*
* []*
@@ -2071,6 +2158,9 @@ static void wt_porcelain_v2_print(struct wt_status *s)
struct string_list_item *it;
