Re: [PATCH] use child_process_init() to initialize struct child_process variables
Am 29.10.2014 um 18:21 schrieb Jeff King:
On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote:
diff --git a/trailer.c b/trailer.c
index 8514566..7ff036c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -237,7 +237,7 @@ static const char *apply_command(const char *command,
const char *arg)
strbuf_replace(cmd, TRAILER_ARG_STRING, arg);
argv[0] = cmd.buf;
-memset(cp, 0, sizeof(cp));
+child_process_init(cp);
cp.argv = argv;
cp.env = local_repo_env;
cp.no_stdin = 1;
I think this one can use CHILD_PROCESS_INIT in the declaration. I guess
it is debatable whether that is actually preferable, but I tend to think
it is cleaner and less error-prone.
Agreed, thanks.
-- 8 --
Subject: [PATCH] trailer: use CHILD_PROCESS_INIT in apply_command()
Initialize the struct child_process variable cp at declaration time.
This is shorter, saves a function call and prevents using the variable
before initialization by mistake.
Suggested-by: Jeff King p...@peff.net
Signed-off-by: Rene Scharfe l@web.de
---
trailer.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/trailer.c b/trailer.c
index 7ff036c..6ae7865 100644
--- a/trailer.c
+++ b/trailer.c
@@ -228,7 +228,7 @@ static const char *apply_command(const char *command, const
char *arg)
{
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
- struct child_process cp;
+ struct child_process cp = CHILD_PROCESS_INIT;
const char *argv[] = {NULL, NULL};
const char *result;
@@ -237,7 +237,6 @@ static const char *apply_command(const char *command, const
char *arg)
strbuf_replace(cmd, TRAILER_ARG_STRING, arg);
argv[0] = cmd.buf;
- child_process_init(cp);
cp.argv = argv;
cp.env = local_repo_env;
cp.no_stdin = 1;
--
2.1.3
--
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] use child_process_init() to initialize struct child_process variables
On Sun, Nov 09, 2014 at 02:49:58PM +0100, René Scharfe wrote: -- 8 -- Subject: [PATCH] trailer: use CHILD_PROCESS_INIT in apply_command() Initialize the struct child_process variable cp at declaration time. This is shorter, saves a function call and prevents using the variable before initialization by mistake. Suggested-by: Jeff King p...@peff.net Signed-off-by: Rene Scharfe l@web.de --- trailer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) Thanks, both this one and the other you just sent (to use child_process.args in more places) look good to me. -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] use child_process_init() to initialize struct child_process variables
From: Junio C Hamano gits...@pobox.com Sent: Monday, November 03, 2014 11:42 PM Jeff King p...@peff.net writes: I peeked at libgit2 and I think it does not support bundles at all yet, so that is safe. Grepping for bundle in dulwich turns up no hits, either. Looks like JGit does support them. I did a very brief test, and it seems to silently ignore a HEAD ref that has the NUL (I guess maybe it just rejects it as a malformed refname). We could make JGit happier either by: 1. Only including the symref magic in ambiguous cases, so that regular ones Just Work as usual. 2. Including two lines, like: $sha1 HEAD\0symref=refs/heads/master $sha1 HEAD which JGit does the right thing with (and git.git seems to, as well). Sounds sensible, even though it looks ugly X-. I believe that the 'two HEADs' mechanism would also fall foul of the 'duplicate refs' warning (untested). 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] use child_process_init() to initialize struct child_process variables
From: Jeff King p...@peff.net Subject: Re: [PATCH] use child_process_init() to initialize struct child_process variables On Tue, Nov 04, 2014 at 01:56:15PM -0800, Junio C Hamano wrote: 2. Including two lines, like: $sha1 HEAD\0symref=refs/heads/master $sha1 HEAD which JGit does the right thing with (and git.git seems to, as well). Sounds sensible, even though it looks ugly X-. I have a mild preference for a syntax that is more similar to the on-wire protocol, so that connect.c::parse_feature_value() can be reused to parse it and possibly annotate_refs_with_symref_info() can also be reused by calling it from transport.c::get_refs_from_bundle(). Yeah, what I wrote above was the simplest thing that could work, and does not need to be the final form. I know that you already know what I'm about to describe below, Junio, but I want to expand on the situation for the benefit of onlookers (and potential implementers like Philip). I think I'm keeping up ;-) The online protocol is hampered by the if you see something after a NUL, it is a capabilities string, and you must throw out the previous capabilities string and replace it with this one historical rule. And that's why we cannot do: $sha1 refs/heads/master\0thin-pack side-band etc $sha1 HEAD\0symref=refs/heads/master as it would throw out thin-pack, side-band, etc. Instead we do it more like: $sha1 refs/heads/master\0thin-pack side-band etc symref=HEAD:refs/heads/master $sha1 HEAD to shove _all_ of the symref mappings into the capability string, rather than letting them ride along with their respective refs. The downside is that we are bounded in the number of symref mappings we can send (by the maximum length for a single pkt-line), and therefore send only the value of HEAD. The bundle code is not bound by this historical legacy, and could do it in a different (and more efficient and flexible) way. But it is probably saner to just keep them identical. It makes the code simpler, and having bundle as the only transport which has the extra flexibility does not really buy us much (and probably just invites confusion). -Peff Obviously bundles are always off-line, so it's reasonable to be cautious about using an on-line sideband method, though the re-use of a standard format is good. Finding the right parsing method will be important, as well as ensuring there are no races from the update of unsorted refs. 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] use child_process_init() to initialize struct child_process variables
On Wed, Nov 05, 2014 at 01:35:21PM -, Philip Oakley wrote: 2. Including two lines, like: [...] I believe that the 'two HEADs' mechanism would also fall foul of the 'duplicate refs' warning (untested). It didn't in my very brief testing of what I posted above, but maybe there is some other case that triggers it that I didn't exercise. I grepped through the code and the only duplicate ref warning I see comes from the refs.c code, which comes from commit_packed_refs(). If the duplicate line is HEAD, I think it shouldn't trigger that, as it is not a regular ref. That would explain why I didn't see it in my testing. -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] use child_process_init() to initialize struct child_process variables
From: Jeff King p...@peff.net On Wed, Nov 05, 2014 at 01:35:21PM -, Philip Oakley wrote: 2. Including two lines, like: [...] I believe that the 'two HEADs' mechanism would also fall foul of the 'duplicate refs' warning (untested). It didn't in my very brief testing of what I posted above, but maybe there is some other case that triggers it that I didn't exercise. I'd been testing the inclusion of a duplicate of the ref that matched the HEAD symref (rather than HEAD itself), and had hit that message a few times, hence my concern. I grepped through the code and the only duplicate ref warning I see comes from the refs.c code, which comes from commit_packed_refs(). I had it from is_dup_ref(), also in refs.c, though I may have followed the call stack incorrectly back to the bundle effects. If the duplicate line is HEAD, I think it shouldn't trigger that, as it is not a regular ref. That would explain why I didn't see it in my testing. I've now also done a test and found the same (no warning/error) when there are two HEADs listed in the bundle preamble. Sorry for the confusion. -- 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] use child_process_init() to initialize struct child_process variables
Junio C Hamano gits...@pobox.com writes: Jeff King p...@peff.net writes: I peeked at libgit2 and I think it does not support bundles at all yet, so that is safe. Grepping for bundle in dulwich turns up no hits, either. Looks like JGit does support them. I did a very brief test, and it seems to silently ignore a HEAD ref that has the NUL (I guess maybe it just rejects it as a malformed refname). We could make JGit happier either by: 1. Only including the symref magic in ambiguous cases, so that regular ones Just Work as usual. 2. Including two lines, like: $sha1 HEAD\0symref=refs/heads/master $sha1 HEAD which JGit does the right thing with (and git.git seems to, as well). Sounds sensible, even though it looks ugly X-. I have a mild preference for a syntax that is more similar to the on-wire protocol, so that connect.c::parse_feature_value() can be reused to parse it and possibly annotate_refs_with_symref_info() can also be reused by calling it from transport.c::get_refs_from_bundle(). -- 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] use child_process_init() to initialize struct child_process variables
On Tue, Nov 04, 2014 at 01:56:15PM -0800, Junio C Hamano wrote: 2. Including two lines, like: $sha1 HEAD\0symref=refs/heads/master $sha1 HEAD which JGit does the right thing with (and git.git seems to, as well). Sounds sensible, even though it looks ugly X-. I have a mild preference for a syntax that is more similar to the on-wire protocol, so that connect.c::parse_feature_value() can be reused to parse it and possibly annotate_refs_with_symref_info() can also be reused by calling it from transport.c::get_refs_from_bundle(). Yeah, what I wrote above was the simplest thing that could work, and does not need to be the final form. I know that you already know what I'm about to describe below, Junio, but I want to expand on the situation for the benefit of onlookers (and potential implementers like Philip). The online protocol is hampered by the if you see something after a NUL, it is a capabilities string, and you must throw out the previous capabilities string and replace it with this one historical rule. And that's why we cannot do: $sha1 refs/heads/master\0thin-pack side-band etc $sha1 HEAD\0symref=refs/heads/master as it would throw out thin-pack, side-band, etc. Instead we do it more like: $sha1 refs/heads/master\0thin-pack side-band etc symref=HEAD:refs/heads/master $sha1 HEAD to shove _all_ of the symref mappings into the capability string, rather than letting them ride along with their respective refs. The downside is that we are bounded in the number of symref mappings we can send (by the maximum length for a single pkt-line), and therefore send only the value of HEAD. The bundle code is not bound by this historical legacy, and could do it in a different (and more efficient and flexible) way. But it is probably saner to just keep them identical. It makes the code simpler, and having bundle as the only transport which has the extra flexibility does not really buy us much (and probably just invites confusion). -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] use child_process_init() to initialize struct child_process variables
Jeff King p...@peff.net writes: The bundle code is not bound by this historical legacy, and could do it in a different (and more efficient and flexible) way. But it is probably saner to just keep them identical. It makes the code simpler, and having bundle as the only transport which has the extra flexibility does not really buy us much (and probably just invites confusion). Yeah, so let's have only symref=HEAD:refs/heads/master for now. I would like to have the protocol update on the on-wire side during 2015 to lift various limits and correct inefficiencies (the largest of which is the who speaks first issue). We should make sure that the bundle format can be enhanced to match when it happens. 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] use child_process_init() to initialize struct child_process variables
Philip Oakley philipoak...@iee.org writes: This certainly looks the way to go. The one extra question would be whether the symref should be included by default when HEAD is present, or only if there was possible ambiguity between the other listed refs. Just include the \0symref=... for any symbolic ref you mention, and the ref in question does not even have to be HEAD, I would say. The mechanism chosen should be something that will be transparently ignored by existing implementations, there is no need to make the data format conditional. If the new implementations of the reading side want to make a choice between following the new \0symref=... and ignoring it and use the traditional heuristics for some unknown/unanticipated reason, that should be the choice for the readers, not for the writers. -- 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] use child_process_init() to initialize struct child_process variables
On Mon, Nov 03, 2014 at 10:26:48AM -0800, Junio C Hamano wrote: Philip Oakley philipoak...@iee.org writes: This certainly looks the way to go. The one extra question would be whether the symref should be included by default when HEAD is present, or only if there was possible ambiguity between the other listed refs. Just include the \0symref=... for any symbolic ref you mention, and the ref in question does not even have to be HEAD, I would say. The mechanism chosen should be something that will be transparently ignored by existing implementations, there is no need to make the data format conditional. One thing I glossed over in my suggestion of the NUL trick: it works on git.git, but no clue about elsewhere. I can imagine that other non-C implementations might treat the whole thing (NUL and extra data included) as the refname. Back when we did the NUL trick to the online protocol, git.git was the only serious implementation. But nowadays we should at least consider the impact on JGit, libgit2, and/or dulwich (breaking them is not necessarily a showstopper IMHO, but we should at least know what we are breaking). I peeked at libgit2 and I think it does not support bundles at all yet, so that is safe. Grepping for bundle in dulwich turns up no hits, either. Looks like JGit does support them. I did a very brief test, and it seems to silently ignore a HEAD ref that has the NUL (I guess maybe it just rejects it as a malformed refname). We could make JGit happier either by: 1. Only including the symref magic in ambiguous cases, so that regular ones Just Work as usual. 2. Including two lines, like: $sha1 HEAD\0symref=refs/heads/master $sha1 HEAD which JGit does the right thing with (and git.git seems to, as well). -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] use child_process_init() to initialize struct child_process variables
Jeff King p...@peff.net writes: I peeked at libgit2 and I think it does not support bundles at all yet, so that is safe. Grepping for bundle in dulwich turns up no hits, either. Looks like JGit does support them. I did a very brief test, and it seems to silently ignore a HEAD ref that has the NUL (I guess maybe it just rejects it as a malformed refname). We could make JGit happier either by: 1. Only including the symref magic in ambiguous cases, so that regular ones Just Work as usual. 2. Including two lines, like: $sha1 HEAD\0symref=refs/heads/master $sha1 HEAD which JGit does the right thing with (and git.git seems to, as well). Sounds sensible, even though it looks ugly X-. -- 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] use child_process_init() to initialize struct child_process variables
From: Jeff King p...@peff.net On Fri, Oct 31, 2014 at 02:48:17PM -0700, Junio C Hamano wrote: Programs that read a pack data stream unpack-objects were originally designed to ignore cruft after the pack data stream ends, and because the bundle file format ends with pack data stream, you should have been able to append extra information at the end without breaking older clients. It's an option, I'd been looking at sneaking the information into the refs header section. Alas, this principle is still true for unpack-objects, but index-pack broke it fairly early on, and we use the latter to deal with bundles, so we cannot just tuck extra info at the end of an existing bundle. You'd instead need a new option to create a bundle that cannot be read by existing clients X-. I think you could use a similar NUL-trick to what we do in the online I like this 'trick'. I'd not appreciated the use of the null separator for breaking a line into separate strings that way before (I'd understood it, just never appreciated it!). protocol, and have a ref section like: ...sha1... refs/heads/master ...sha1... refs/heads/confused-with-master ...sha1... HEAD\0symref=refs/heads/master The current parser reads into a strbuf up to the newline, but we ignore everything after the NUL, treating it like a C string. Prior to using strbufs, we used fgets, which behaves similarly (you could not know from fgets that there is extra data after the NUL, but that is OK; we only want older versions to ignore the data, not do anything useful with it). This certainly looks the way to go. The one extra question would be whether the symref should be included by default when HEAD is present, or only if there was possible ambiguity between the other listed refs. Previously I'd assumed the latter. The former would appear stronger, as long as the symref was within the listed refs, and excluded otherwise. 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] use child_process_init() to initialize struct child_process variables
Philip Oakley philipoak...@iee.org writes: As a side project (slow time) I've been looking at the loss of the HEAD symbolic ref when multiple heads are bundled that point at the same rev. That is, when the HEAD detection heuristic fails. It think you are talking about the logic used by the clone, where - if there is only one branch ref that matches the value of HEAD, that is the branch; - if there are more than one refs that match the value of HEAD, and if one of them is 'master', then that is the branch; - if there are more than one refs that match the value of HEAD, and if none of them is 'master', then pick the earliest one. So you would be in trouble _if_ you have two refs pointing at the same commit, one of them being 'master', and the current branch is the other ref. All other cases you shouldn't have to change the file format and have older client understand which branch is the current one. Programs that read a pack data stream unpack-objects were originally designed to ignore cruft after the pack data stream ends, and because the bundle file format ends with pack data stream, you should have been able to append extra information at the end without breaking older clients. Alas, this principle is still true for unpack-objects, but index-pack broke it fairly early on, and we use the latter to deal with bundles, so we cannot just tuck extra info at the end of an existing bundle. You'd instead need a new option to create a bundle that cannot be read by existing clients X-. -- 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] use child_process_init() to initialize struct child_process variables
On Fri, Oct 31, 2014 at 02:48:17PM -0700, Junio C Hamano wrote: Programs that read a pack data stream unpack-objects were originally designed to ignore cruft after the pack data stream ends, and because the bundle file format ends with pack data stream, you should have been able to append extra information at the end without breaking older clients. Alas, this principle is still true for unpack-objects, but index-pack broke it fairly early on, and we use the latter to deal with bundles, so we cannot just tuck extra info at the end of an existing bundle. You'd instead need a new option to create a bundle that cannot be read by existing clients X-. I think you could use a similar NUL-trick to what we do in the online protocol, and have a ref section like: ...sha1... refs/heads/master ...sha1... refs/heads/confused-with-master ...sha1... HEAD\0symref=refs/heads/master The current parser reads into a strbuf up to the newline, but we ignore everything after the NUL, treating it like a C string. Prior to using strbufs, we used fgets, which behaves similarly (you could not know from fgets that there is extra data after the NUL, but that is OK; we only want older versions to ignore the data, not do anything useful with 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] use child_process_init() to initialize struct child_process variables
Junio C Hamano gits...@pobox.com writes:
Probably three helper functions:
- The first is to find tops and bottoms (this translates fuzzy
specifications such as --since 30.days into a more concrete
revision range ^A ^B ... Z to establish bundle prerequisites),
which is done by running a rev-list --boundary.
- The second is to show refs, while paying attention to things like
--10 maint master which may result in the tip of 'maint' not
being shown at all. I am not sure if this part can/should take
advantage of revs.cmdline, though.
- The last is to create the actual pack data.
I agree with your analysis on the change in column.c and trailer.c
Thanks.
So here are a few patches on top of René's change. This is the
third point in the above list.
-- 8 --
Subject: [PATCH] bundle: split out a helper function to create a pack data
The create_bundle() function, while it does one single logical thing
and tries to do it well, that single logical thing takes a rather
large implementation.
Let's start separating what it does into smaller steps to make it
easier what is going on. This is a first step to separate out the
actual pack-data generation, after the earlier part of the function
figures out which part of the history to place in the bundle.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
bundle.c | 64 +---
1 file changed, 37 insertions(+), 27 deletions(-)
diff --git a/bundle.c b/bundle.c
index c846092..9c87532 100644
--- a/bundle.c
+++ b/bundle.c
@@ -235,6 +235,41 @@ out:
return result;
}
+static int write_pack_data(int bundle_fd, struct lock_file *lock, struct
rev_info *revs)
+{
+ struct child_process pack_objects = CHILD_PROCESS_INIT;
+ int i;
+
+ argv_array_pushl(pack_objects.args,
+pack-objects, --all-progress-implied,
+--stdout, --thin, --delta-base-offset,
+NULL);
+ pack_objects.in = -1;
+ pack_objects.out = bundle_fd;
+ pack_objects.git_cmd = 1;
+ if (start_command(pack_objects))
+ return error(_(Could not spawn pack-objects));
+
+ /*
+* start_command closed bundle_fd if it was 1
+* so set the lock fd to -1 so commit_lock_file()
+* won't fail trying to close it.
+*/
+ lock-fd = -1;
+
+ for (i = 0; i revs-pending.nr; i++) {
+ struct object *object = revs-pending.objects[i].item;
+ if (object-flags UNINTERESTING)
+ write_or_die(pack_objects.in, ^, 1);
+ write_or_die(pack_objects.in, sha1_to_hex(object-sha1), 40);
+ write_or_die(pack_objects.in, \n, 1);
+ }
+ close(pack_objects.in);
+ if (finish_command(pack_objects))
+ return error(_(pack-objects died));
+ return 0;
+}
+
int create_bundle(struct bundle_header *header, const char *path,
int argc, const char **argv)
{
@@ -381,34 +416,9 @@ int create_bundle(struct bundle_header *header, const char
*path,
write_or_die(bundle_fd, \n, 1);
/* write pack */
- child_process_init(rls);
- argv_array_pushl(rls.args,
-pack-objects, --all-progress-implied,
---stdout, --thin, --delta-base-offset,
-NULL);
- rls.in = -1;
- rls.out = bundle_fd;
- rls.git_cmd = 1;
- if (start_command(rls))
- return error(_(Could not spawn pack-objects));
-
- /*
-* start_command closed bundle_fd if it was 1
-* so set the lock fd to -1 so commit_lock_file()
-* won't fail trying to close it.
-*/
- lock.fd = -1;
+ if (write_pack_data(bundle_fd, lock, revs))
+ return -1;
- for (i = 0; i revs.pending.nr; i++) {
- struct object *object = revs.pending.objects[i].item;
- if (object-flags UNINTERESTING)
- write_or_die(rls.in, ^, 1);
- write_or_die(rls.in, sha1_to_hex(object-sha1), 40);
- write_or_die(rls.in, \n, 1);
- }
- close(rls.in);
- if (finish_command(rls))
- return error(_(pack-objects died));
if (!bundle_to_stdout) {
if (commit_lock_file(lock))
die_errno(_(cannot create '%s'), path);
--
2.1.3-612-g493e79e
--
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] use child_process_init() to initialize struct child_process variables
On Thu, Oct 30, 2014 at 11:07:39AM -0700, Junio C Hamano wrote: -- 8 -- Subject: [PATCH] bundle: split out a helper function to create a pack data s/a pack data/pack data/ The create_bundle() function, while it does one single logical thing and tries to do it well, that single logical thing takes a rather large implementation. I had minor trouble parsing this. I think it might be more clearly said as just: The create_bundle() function, while it does one single logical thing, takes a rather large implementation to do so. Let's start separating what it does into smaller steps to make it easier what is going on. This is a first step to separate out the s/easier/ to see/ bundle.c | 64 +--- 1 file changed, 37 insertions(+), 27 deletions(-) The patch itself looked OK to me. -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] use child_process_init() to initialize struct child_process variables
On Wed, Oct 29, 2014 at 12:16:05PM -0700, Junio C Hamano wrote:
Probably three helper functions:
- The first is to find tops and bottoms (this translates fuzzy
specifications such as --since 30.days into a more concrete
revision range ^A ^B ... Z to establish bundle prerequisites),
which is done by running a rev-list --boundary.
- The second is to show refs, while paying attention to things like
--10 maint master which may result in the tip of 'maint' not
being shown at all. I am not sure if this part can/should take
advantage of revs.cmdline, though.
- The last is to create the actual pack data.
I agree with your analysis on the change in column.c and trailer.c
I was not planning to work on this, but since you did the first and
third bullet points, I think it makes sense to start the second one with
this cleanup:
-- 8 --
Subject: bundle: split out ref writing from bundle_create
The bundle_create() function has a number of logical steps:
process the input, write the refs, and write the packfile.
Recent commits split the first and third into separate
sub-functions. It's worth splitting the middle step out,
too, if only because it makes the progression of the steps
more obvious.
Signed-off-by: Jeff King p...@peff.net
---
Obviously this should be dropped if somebody is actively working on the
revs.cmdline thing you mentioned, as it would conflict horribly. But I
think it is a nice step for somebody working on it later, because the
revs.cmdline changes should be isolated to write_bundle_refs.
bundle.c | 97 ++--
1 file changed, 58 insertions(+), 39 deletions(-)
diff --git a/bundle.c b/bundle.c
index 0ca8737..ca4803b 100644
--- a/bundle.c
+++ b/bundle.c
@@ -310,43 +310,22 @@ static int compute_and_write_prerequistes(int bundle_fd,
return 0;
}
-int create_bundle(struct bundle_header *header, const char *path,
- int argc, const char **argv)
+/*
+ * Write out bundle refs based on the tips already
+ * parsed into revs.pending. As a side effect, may
+ * manipulate revs.pending to include additional
+ * necessary objects (like tags).
+ *
+ * Returns the number of refs written, or negative
+ * on error.
+ */
+static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
{
- static struct lock_file lock;
- int bundle_fd = -1;
- int bundle_to_stdout;
- int i, ref_count = 0;
- struct rev_info revs;
-
- bundle_to_stdout = !strcmp(path, -);
- if (bundle_to_stdout)
- bundle_fd = 1;
- else
- bundle_fd = hold_lock_file_for_update(lock, path,
- LOCK_DIE_ON_ERROR);
-
- /* write signature */
- write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
-
- /* init revs to list objects for pack-objects later */
- save_commit_buffer = 0;
- init_revisions(revs, NULL);
-
- /* write prerequisites */
- if (compute_and_write_prerequistes(bundle_fd, revs, argc, argv))
- return -1;
-
- /* write references */
- argc = setup_revisions(argc, argv, revs, NULL);
-
- if (argc 1)
- return error(_(unrecognized argument: %s), argv[1]);
-
- object_array_remove_duplicates(revs.pending);
+ int i;
+ int ref_count = 0;
- for (i = 0; i revs.pending.nr; i++) {
- struct object_array_entry *e = revs.pending.objects + i;
+ for (i = 0; i revs-pending.nr; i++) {
+ struct object_array_entry *e = revs-pending.objects + i;
unsigned char sha1[20];
char *ref;
const char *display_ref;
@@ -361,7 +340,7 @@ int create_bundle(struct bundle_header *header, const char
*path,
display_ref = (flag REF_ISSYMREF) ? e-name : ref;
if (e-item-type == OBJ_TAG
- !is_tag_in_date_range(e-item, revs)) {
+ !is_tag_in_date_range(e-item, revs)) {
e-item-flags |= UNINTERESTING;
continue;
}
@@ -407,7 +386,7 @@ int create_bundle(struct bundle_header *header, const char
*path,
*/
obj = parse_object_or_die(sha1, e-name);
obj-flags |= SHOWN;
- add_pending_object(revs, obj, e-name);
+ add_pending_object(revs, obj, e-name);
}
free(ref);
continue;
@@ -420,11 +399,51 @@ int create_bundle(struct bundle_header *header, const
char *path,
write_or_die(bundle_fd, \n, 1);
free(ref);
}
- if (!ref_count)
- die(_(Refusing to create empty bundle.));
/* end header */
Re: [PATCH] use child_process_init() to initialize struct child_process variables
From: Jeff King p...@peff.net
On Wed, Oct 29, 2014 at 12:16:05PM -0700, Junio C Hamano wrote:
Probably three helper functions:
- The first is to find tops and bottoms (this translates fuzzy
specifications such as --since 30.days into a more concrete
revision range ^A ^B ... Z to establish bundle prerequisites),
which is done by running a rev-list --boundary.
- The second is to show refs, while paying attention to things like
--10 maint master which may result in the tip of 'maint' not
being shown at all. I am not sure if this part can/should take
advantage of revs.cmdline, though.
- The last is to create the actual pack data.
I agree with your analysis on the change in column.c and trailer.c
I was not planning to work on this, but since you did the first and
third bullet points, I think it makes sense to start the second one
with
this cleanup:
-- 8 --
Subject: bundle: split out ref writing from bundle_create
The bundle_create() function has a number of logical steps:
process the input, write the refs, and write the packfile.
Recent commits split the first and third into separate
sub-functions. It's worth splitting the middle step out,
too, if only because it makes the progression of the steps
more obvious.
Signed-off-by: Jeff King p...@peff.net
---
Obviously this should be dropped if somebody is actively working on
the
revs.cmdline thing you mentioned, as it would conflict horribly. But I
think it is a nice step for somebody working on it later, because the
revs.cmdline changes should be isolated to write_bundle_refs.
As a side project (slow time) I've been looking at the loss of the HEAD
symbolic ref when multiple heads are bundled that point at the same rev.
That is, when the HEAD detection heuristic fails.
What I've noticed so far is that a duplicated ref (added to the bundle
manually) simply creates a warning (currently, and the warning names
that ref) rather than failing when cloned (and by implication fetched).
Not only that the bundle verify command does not report any error for
such a bundle containing a duplicate ref.
Given that, if the refs were sorted, and HEAD was listed last (as is the
case with '--all'), then one could add the duplicate ref immediately
after HEAD, of it's symbolic ref. I.e if a duplicate ref is found after
HEAD then that ref is the true HEAD ref. This duplicate ref would only
need to be present if there are multiple (two or more) heads that point
to the same rev, and the HEAD isn't detatched. Sorting is necessary to
ensure that HEAD is last and its duplicate refs/head ref immediately
follows.
Thus the first step is to ensure that the positive refs list is sorted
such that HEAD (and it's ilk) is last.
I'd also planned an option '--HEAD' which would add such a duplicate ref
to a V2 bundle (resulting in a warning for older users which displays
the duplicate head ref!)
An option '--V3' would be the same as '--HEAD' but would also change the
bundle string version number (to V3), and so would not be acceptable to
older systems (for those that require such separation) but the
clone/fetch on a newer system would detect the V3 in the header string
and then use the extra ref rather than the heuristic to determine HEAD
(with appropriate checks).
I hadn't finished my studies of the refs.c code to fully understand what
I'd need to change, but hopefully the changes in this patch can be
aligned in the same direction (or the errors in my reasoning be pointed
out;-)
The need to sort the refs in this method would separate the
determination of the correct refs from the writing of the refs. All
assuming the idea has merit...
--
Philip
bundle.c | 97
++--
1 file changed, 58 insertions(+), 39 deletions(-)
diff --git a/bundle.c b/bundle.c
index 0ca8737..ca4803b 100644
--- a/bundle.c
+++ b/bundle.c
@@ -310,43 +310,22 @@ static int compute_and_write_prerequistes(int
bundle_fd,
return 0;
}
-int create_bundle(struct bundle_header *header, const char *path,
- int argc, const char **argv)
+/*
+ * Write out bundle refs based on the tips already
+ * parsed into revs.pending. As a side effect, may
+ * manipulate revs.pending to include additional
+ * necessary objects (like tags).
+ *
+ * Returns the number of refs written, or negative
+ * on error.
+ */
+static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
{
- static struct lock_file lock;
- int bundle_fd = -1;
- int bundle_to_stdout;
- int i, ref_count = 0;
- struct rev_info revs;
-
- bundle_to_stdout = !strcmp(path, -);
- if (bundle_to_stdout)
- bundle_fd = 1;
- else
- bundle_fd = hold_lock_file_for_update(lock, path,
- LOCK_DIE_ON_ERROR);
-
- /* write signature */
- write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
-
- /* init revs to list objects for pack-objects later */
- save_commit_buffer = 0;
- init_revisions(revs, NULL);
-
- /* write prerequisites */
- if
Re: [PATCH] use child_process_init() to initialize struct child_process variables
On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote: --- a/bundle.c +++ b/bundle.c @@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path, write_or_die(bundle_fd, \n, 1); /* write pack */ - memset(rls, 0, sizeof(rls)); + child_process_init(rls); argv_array_pushl(rls.args, pack-objects, --all-progress-implied, --stdout, --thin, --delta-base-offset, I wondered if this one could use CHILD_PROCESS_INIT in the declaration instead. And indeed, we _do_ use CHILD_PROCESS_INIT there, but we use the same variable twice for two different child processes in the same function. Besides variable reuse being slightly confusing, the name rls (which presumably stands for rev-list for the first child) means nothing here, where we are calling pack-objects. Maybe it would be cleaner to introduce a second variable? I also suspect the function would be a lot more readable broken into two sub-functions (reading from rev-list and writing to pack-objects), but I did not look closely enough to see whether there were any complicating factors. diff --git a/column.c b/column.c index 8082a94..786abe6 100644 --- a/column.c +++ b/column.c @@ -374,7 +374,7 @@ int run_column_filter(int colopts, const struct column_options *opts) if (fd_out != -1) return -1; - memset(column_process, 0, sizeof(column_process)); + child_process_init(column_process); argv = column_process.args; argv_array_push(argv, column); This one uses a static child_process which needs to be reinitialized on each run of the function. So it definitely needs child_process_init. diff --git a/trailer.c b/trailer.c index 8514566..7ff036c 100644 --- a/trailer.c +++ b/trailer.c @@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg) strbuf_replace(cmd, TRAILER_ARG_STRING, arg); argv[0] = cmd.buf; - memset(cp, 0, sizeof(cp)); + child_process_init(cp); cp.argv = argv; cp.env = local_repo_env; cp.no_stdin = 1; I think this one can use CHILD_PROCESS_INIT in the declaration. I guess it is debatable whether that is actually preferable, but I tend to think it is cleaner and less error-prone. -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] use child_process_init() to initialize struct child_process variables
Jeff King p...@peff.net writes: On Tue, Oct 28, 2014 at 09:52:34PM +0100, René Scharfe wrote: --- a/bundle.c +++ b/bundle.c @@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path, write_or_die(bundle_fd, \n, 1); /* write pack */ -memset(rls, 0, sizeof(rls)); +child_process_init(rls); argv_array_pushl(rls.args, pack-objects, --all-progress-implied, --stdout, --thin, --delta-base-offset, I wondered if this one could use CHILD_PROCESS_INIT in the declaration instead. And indeed, we _do_ use CHILD_PROCESS_INIT there, but we use the same variable twice for two different child processes in the same function. Besides variable reuse being slightly confusing, the name rls (which presumably stands for rev-list for the first child) means nothing here, where we are calling pack-objects. Maybe it would be cleaner to introduce a second variable? It has been this way since day one at b1daf300 (Replace fork_with_pipe in bundle with run_command, 2007-03-12); I agree that two variables might make things less confusing. I also suspect the function would be a lot more readable broken into two sub-functions (reading from rev-list and writing to pack-objects), but I did not look closely enough to see whether there were any complicating factors. Probably three helper functions: - The first is to find tops and bottoms (this translates fuzzy specifications such as --since 30.days into a more concrete revision range ^A ^B ... Z to establish bundle prerequisites), which is done by running a rev-list --boundary. - The second is to show refs, while paying attention to things like --10 maint master which may result in the tip of 'maint' not being shown at all. I am not sure if this part can/should take advantage of revs.cmdline, though. - The last is to create the actual pack data. I agree with your analysis on the change in column.c and trailer.c 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] use child_process_init() to initialize struct child_process variables
Sent from my BlackBerry 10 smartphone on the Rogers network. Original Message From: René Scharfe Sent: Tuesday, October 28, 2014 16:59 To: Git Mailing List Cc: Junio C Hamano Subject: [PATCH] use child_process_init() to initialize struct child_process variables Call child_process_init() instead of zeroing the memory of variables of type struct child_process by hand before use because the former is both clearer and shorter. Signed-off-by: Rene Scharfe l@web.de --- bundle.c | 2 +- column.c | 2 +- trailer.c | 2 +- transport-helper.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bundle.c b/bundle.c index fa67057..c846092 100644 --- a/bundle.c +++ b/bundle.c @@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path, write_or_die(bundle_fd, \n, 1); /* write pack */ - memset(rls, 0, sizeof(rls)); + child_process_init(rls); argv_array_pushl(rls.args, pack-objects, --all-progress-implied, --stdout, --thin, --delta-base-offset, diff --git a/column.c b/column.c index 8082a94..786abe6 100644 --- a/column.c +++ b/column.c @@ -374,7 +374,7 @@ int run_column_filter(int colopts, const struct column_options *opts) if (fd_out != -1) return -1; - memset(column_process, 0, sizeof(column_process)); + child_process_init(column_process); argv = column_process.args; argv_array_push(argv, column); diff --git a/trailer.c b/trailer.c index 8514566..7ff036c 100644 --- a/trailer.c +++ b/trailer.c @@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg) strbuf_replace(cmd, TRAILER_ARG_STRING, arg); argv[0] = cmd.buf; - memset(cp, 0, sizeof(cp)); + child_process_init(cp); cp.argv = argv; cp.env = local_repo_env; cp.no_stdin = 1; diff --git a/transport-helper.c b/transport-helper.c index 6cd9dd1..0224687 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -414,7 +414,7 @@ static int get_exporter(struct transport *transport, struct child_process *helper = get_helper(transport); int i; - memset(fastexport, 0, sizeof(*fastexport)); + child_process_init(fastexport); /* we need to duplicate helper-in because we want to use it after * fastexport is done with it. */ -- 2.1.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 -- 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
