10 messages, 4 authors, 2016-06-15 · open the first message on its own page
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:54:37
Dan Johnson [off-list ref] writes:
I believe that is bad interaction with "--all" (probably a bug). If I am remembering correctly, --no-tags is internally a per-remote setting, so I'm guessing it's not getting set on all remotes here. I'll look into this more a bit later tonight. Does fetch --no-tags work when you specify a remote?
Thanks.
From: Dan Johnson <hidden>
Date: 2016-06-15 22:54:37
Reported-by: Oswald Buddenhagen <redacted> Signed-off-by: Dan Johnson <redacted> --- Junio C Hamano [off-list ref] writes:
Dan Johnson [off-list ref] writes:quoted
I believe that is bad interaction with "--all" (probably a bug). If I am remembering correctly, --no-tags is internally a per-remote setting, so I'm guessing it's not getting set on all remotes here. I'll look into this more a bit later tonight. Does fetch --no-tags work when you specify a remote?Thanks.
And here it is. Apparently we just don't pass those options through. I didn't look to see if there are any other options we should consider passing through; it's quite possible there are. I also have not written a test to ensure that this doesn't break in the future. I will hopefully have time for these things tomorrow. It's getting too late for me to be able to put sentences together, so hopefully this mail comes out readable ;) builtin/fetch.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index bb9a074..c6bcbdc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c@@ -857,6 +857,10 @@ static void add_options_to_argv(int *argc, const char **argv) argv[(*argc)++] = "--recurse-submodules"; else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) argv[(*argc)++] = "--recurse-submodules=on-demand"; + if (tags == TAGS_SET) + argv[(*argc)++] = "--tags"; + else if (tags == TAGS_UNSET) + argv[(*argc)++] = "--no-tags"; if (verbosity >= 2) argv[(*argc)++] = "-v"; if (verbosity >= 1)
--
1.7.11.1.59.gbc9e7dd.dirty
From: Jeff King <hidden>
Date: 2016-06-15 22:54:37
On Sat, Sep 01, 2012 at 12:25:33AM -0400, Dan Johnson wrote:
diff --git a/builtin/fetch.c b/builtin/fetch.c index bb9a074..c6bcbdc 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c@@ -857,6 +857,10 @@ static void add_options_to_argv(int *argc, const char **argv) argv[(*argc)++] = "--recurse-submodules"; else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) argv[(*argc)++] = "--recurse-submodules=on-demand"; + if (tags == TAGS_SET) + argv[(*argc)++] = "--tags"; + else if (tags == TAGS_UNSET) + argv[(*argc)++] = "--no-tags"; if (verbosity >= 2) argv[(*argc)++] = "-v"; if (verbosity >= 1)
Hmm. We allocate argv in fetch_multiple like this:
const char *argv[12] = { "fetch", "--append" };
and then add a bunch of options to it, along with the name of the
remote. By my count, the current code can hit exactly 12 (including the
terminating NULL) if all options are set. Your patch would make it
possible to overflow. Of course, I may be miscounting since it is
extremely error-prone to figure out the right number by tracing each
possible conditional.
Maybe we should switch it to a dynamic argv_array? Like this:
[1/2]: argv-array: add pop function
[2/2]: fetch: use argv_array instead of hand-building arrays
-Peff
From: Jeff King <hidden>
Date: 2016-06-15 22:54:37
Sometimes we build a set of similar command lines, differing only in the final arguments (e.g., "fetch --multiple"). To use argv_array for this, you have to either push the same set of elements repeatedly, or break the abstraction by manually manipulating the array's internal members. Instead, let's provide a sanctioned "pop" function to remove elements from the end. Signed-off-by: Jeff King <redacted> --- Documentation/technical/api-argv-array.txt | 4 ++++ argv-array.c | 9 +++++++++ argv-array.h | 1 + 3 files changed, 14 insertions(+)
diff --git a/Documentation/technical/api-argv-array.txt b/Documentation/technical/api-argv-array.txt
index 1b7d8f1..1a79781 100644
--- a/Documentation/technical/api-argv-array.txt
+++ b/Documentation/technical/api-argv-array.txt@@ -46,6 +46,10 @@ Functions Format a string and push it onto the end of the array. This is a convenience wrapper combining `strbuf_addf` and `argv_array_push`. +`argv_array_pop`:: + Remove the final element from the array. If there are no + elements in the array, do nothing. + `argv_array_clear`:: Free all memory associated with the array and return it to the initial, empty state.
diff --git a/argv-array.c b/argv-array.c
index 0b5f889..55e8443 100644
--- a/argv-array.c
+++ b/argv-array.c@@ -49,6 +49,15 @@ void argv_array_pushl(struct argv_array *array, ...) va_end(ap); } +void argv_array_pop(struct argv_array *array) +{ + if (!array->argc) + return; + free((char *)array->argv[array->argc - 1]); + array->argv[array->argc - 1] = NULL; + array->argc--; +} + void argv_array_clear(struct argv_array *array) { if (array->argv != empty_argv) {
diff --git a/argv-array.h b/argv-array.h
index b93a69c..f4b9866 100644
--- a/argv-array.h
+++ b/argv-array.h@@ -16,6 +16,7 @@ void argv_array_pushl(struct argv_array *, ...); __attribute__((format (printf,2,3))) void argv_array_pushf(struct argv_array *, const char *fmt, ...); void argv_array_pushl(struct argv_array *, ...); +void argv_array_pop(struct argv_array *); void argv_array_clear(struct argv_array *); #endif /* ARGV_ARRAY_H */
--
1.7.12.rc3.8.g89db099
From: Jeff King <hidden>
Date: 2016-06-15 22:54:37
Fetch invokes itself recursively when recursing into submodules or handling "fetch --multiple". In both cases, it builds the child's command line by pushing options onto a statically-sized array. In both cases, the array is currently just big enough to handle the largest possible case. However, this technique is brittle and error-prone, so let's replace it with a dynamic argv_array. Signed-off-by: Jeff King <redacted> --- Not very well tested by me, but hopefully it is simple enough that I managed not to screw it up. It may be that fetch_populated_submodules would also benefit from conversion (here I just pass in the argc and argv separately), but I didn't look. builtin/fetch.c | 47 +++++++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 22 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index bb9a074..b6a8be0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c@@ -14,6 +14,7 @@ #include "transport.h" #include "submodule.h" #include "connected.h" +#include "argv-array.h" static const char * const builtin_fetch_usage[] = { "git fetch [<options>] [<repository> [<refspec>...]]",
@@ -841,38 +842,35 @@ static int fetch_multiple(struct string_list *list) return 1; } -static void add_options_to_argv(int *argc, const char **argv) +static void add_options_to_argv(struct argv_array *argv) { if (dry_run) - argv[(*argc)++] = "--dry-run"; + argv_array_push(argv, "--dry-run"); if (prune) - argv[(*argc)++] = "--prune"; + argv_array_push(argv, "--prune"); if (update_head_ok) - argv[(*argc)++] = "--update-head-ok"; + argv_array_push(argv, "--update-head-ok"); if (force) - argv[(*argc)++] = "--force"; + argv_array_push(argv, "--force"); if (keep) - argv[(*argc)++] = "--keep"; + argv_array_push(argv, "--keep"); if (recurse_submodules == RECURSE_SUBMODULES_ON) - argv[(*argc)++] = "--recurse-submodules"; + argv_array_push(argv, "--recurse-submodules"); else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) - argv[(*argc)++] = "--recurse-submodules=on-demand"; + argv_array_push(argv, "--recurse-submodules=on-demand"); if (verbosity >= 2) - argv[(*argc)++] = "-v"; + argv_array_push(argv, "-v"); if (verbosity >= 1) - argv[(*argc)++] = "-v"; + argv_array_push(argv, "-v"); else if (verbosity < 0) - argv[(*argc)++] = "-q"; + argv_array_push(argv, "-q"); } static int fetch_multiple(struct string_list *list) { int i, result = 0; - const char *argv[12] = { "fetch", "--append" }; - int argc = 2; - - add_options_to_argv(&argc, argv); + struct argv_array argv = ARGV_ARRAY_INIT; if (!append && !dry_run) { int errcode = truncate_fetch_head();
@@ -880,18 +878,22 @@ static int fetch_multiple(struct string_list *list) return errcode; } + argv_array_pushl(&argv, "fetch", "--append", NULL); + add_options_to_argv(&argv); + for (i = 0; i < list->nr; i++) { const char *name = list->items[i].string; - argv[argc] = name; - argv[argc + 1] = NULL; + argv_array_push(&argv, name); if (verbosity >= 0) printf(_("Fetching %s\n"), name); - if (run_command_v_opt(argv, RUN_GIT_CMD)) { + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) { error(_("Could not fetch %s"), name); result = 1; } + argv_array_pop(&argv); } + argv_array_clear(&argv); return result; }
@@ -1007,13 +1009,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) } if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { - const char *options[10]; - int num_options = 0; - add_options_to_argv(&num_options, options); - result = fetch_populated_submodules(num_options, options, + struct argv_array options = ARGV_ARRAY_INIT; + + add_options_to_argv(&options); + result = fetch_populated_submodules(options.argc, options.argv, submodule_prefix, recurse_submodules, verbosity < 0); + argv_array_clear(&options); } /* All names were strdup()ed or strndup()ed */
--
1.7.12.rc3.8.g89db099
From: Jeff King <hidden>
Date: 2016-06-15 22:54:37
Since the array struct stores a "const char **" argv member (for compatibility with most of our argv-taking functions), we have to cast away the const-ness when freeing its elements. However, we used the wrong type when doing so. It doesn't make a difference since free() take a void pointer anyway, but it can be slightly confusing to a reader. Signed-off-by: Jeff King <redacted> --- Noticed this while I was adding the other free in argv_array_pop... argv-array.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/argv-array.c b/argv-array.c
index 55e8443..256741d 100644
--- a/argv-array.c
+++ b/argv-array.c@@ -63,7 +63,7 @@ void argv_array_clear(struct argv_array *array) if (array->argv != empty_argv) { int i; for (i = 0; i < array->argc; i++) - free((char **)array->argv[i]); + free((char *)array->argv[i]); free(array->argv); } argv_array_init(array);
--
1.7.12.rc3.8.g89db099
From: Jeff King <hidden>
Date: 2016-06-15 22:54:37
On Sat, Sep 01, 2012 at 07:32:07AM -0400, Jeff King wrote:
Since the array struct stores a "const char **" argv member (for compatibility with most of our argv-taking functions), we have to cast away the const-ness when freeing its elements. However, we used the wrong type when doing so. It doesn't make a difference since free() take a void pointer anyway, but it can be slightly confusing to a reader. Signed-off-by: Jeff King <redacted> --- Noticed this while I was adding the other free in argv_array_pop...
Argh, managed to botch the subject line. Here it is for real. -- >8 -- Since the array struct stores a "const char **" argv member (for compatibility with most of our argv-taking functions), we have to cast away the const-ness when freeing its elements. However, we used the wrong type when doing so. It doesn't make a difference since free() take a void pointer anyway, but it can be slightly confusing to a reader. Signed-off-by: Jeff King <redacted> --- argv-array.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/argv-array.c b/argv-array.c
index 55e8443..256741d 100644
--- a/argv-array.c
+++ b/argv-array.c@@ -63,7 +63,7 @@ void argv_array_clear(struct argv_array *array) if (array->argv != empty_argv) { int i; for (i = 0; i < array->argc; i++) - free((char **)array->argv[i]); + free((char *)array->argv[i]); free(array->argv); } argv_array_init(array);
--
1.7.12.rc3.8.g89db099
From: Jens Lehmann <hidden>
Date: 2016-06-15 22:54:37
Am 01.09.2012 13:27, schrieb Jeff King:
Fetch invokes itself recursively when recursing into submodules or handling "fetch --multiple". In both cases, it builds the child's command line by pushing options onto a statically-sized array. In both cases, the array is currently just big enough to handle the largest possible case. However, this technique is brittle and error-prone, so let's replace it with a dynamic argv_array. Signed-off-by: Jeff King <redacted> --- Not very well tested by me, but hopefully it is simple enough that I managed not to screw it up.
This is definitely an improvement, and I can't spot any problems either.
It may be that fetch_populated_submodules would also benefit from conversion (here I just pass in the argc and argv separately), but I didn't look.
Yes, it does some similar brittle stuff and should be changed to use the argv-array too. I'll look into that.
From: Jens Lehmann <hidden>
Date: 2016-06-15 22:54:37
fetch_populated_submodules() allocates the full argv array it uses to recurse into the submodules from the number of given options plus the six argv values it is going to add. It then initializes it with those values which won't change during the iteration and copies the given options into it. Inside the loop the two argv values different for each submodule get replaced with those currently valid. However, this technique is brittle and error-prone (as the comment to explain the magic number 6 indicates), so let's replace it with an argv_array. Instead of replacing the argv values, push them to the argv_array just before the run_command() call (including the option separating them) and pop them from the argv_array right after that. Signed-off-by: Jens Lehmann <redacted> --- Am 01.09.2012 16:34, schrieb Jens Lehmann:
Am 01.09.2012 13:27, schrieb Jeff King:quoted
It may be that fetch_populated_submodules would also benefit from conversion (here I just pass in the argc and argv separately), but I didn't look.Yes, it does some similar brittle stuff and should be changed to use the argv-array too. I'll look into that.
Maybe something like this on top of your two patches? I thought about adding an argv_array_cat() function to replace the for() loop copying the option values into the argv-array built inside fetch_populated_submodules(), but I suspect saving one line from the code is not worth it. Yet I didn't check if others would benefit from such a function too. builtin/fetch.c | 2 +- submodule.c | 31 ++++++++++++++++--------------- submodule.h | 3 ++- 3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b6a8be0..aaba61e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c@@ -1012,7 +1012,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) struct argv_array options = ARGV_ARRAY_INIT; add_options_to_argv(&options); - result = fetch_populated_submodules(options.argc, options.argv, + result = fetch_populated_submodules(&options, submodule_prefix, recurse_submodules, verbosity < 0);
diff --git a/submodule.c b/submodule.c
index 19dc6a6..51d48c2 100644
--- a/submodule.c
+++ b/submodule.c@@ -588,13 +588,13 @@ static void calculate_changed_submodule_paths(void) initialized_fetch_ref_tips = 0; } -int fetch_populated_submodules(int num_options, const char **options, +int fetch_populated_submodules(const struct argv_array *options, const char *prefix, int command_line_option, int quiet) { - int i, result = 0, argc = 0, default_argc; + int i, result = 0; struct child_process cp; - const char **argv; + struct argv_array argv = ARGV_ARRAY_INIT; struct string_list_item *name_for_path; const char *work_tree = get_git_work_tree(); if (!work_tree)
@@ -604,17 +604,13 @@ int fetch_populated_submodules(int num_options, const char **options, if (read_cache() < 0) die("index file corrupt"); - /* 6: "fetch" (options) --recurse-submodules-default default "--submodule-prefix" prefix NULL */ - argv = xcalloc(num_options + 6, sizeof(const char *)); - argv[argc++] = "fetch"; - for (i = 0; i < num_options; i++) - argv[argc++] = options[i]; - argv[argc++] = "--recurse-submodules-default"; - default_argc = argc++; - argv[argc++] = "--submodule-prefix"; + argv_array_push(&argv, "fetch"); + for (i = 0; i < options->argc; i++) + argv_array_push(&argv, options->argv[i]); + argv_array_push(&argv, "--recurse-submodules-default"); + /* default value, "--submodule-prefix" and its value are added later */ memset(&cp, 0, sizeof(cp)); - cp.argv = argv; cp.env = local_repo_env; cp.git_cmd = 1; cp.no_stdin = 1;
@@ -674,16 +670,21 @@ int fetch_populated_submodules(int num_options, const char **options, if (!quiet) printf("Fetching submodule %s%s\n", prefix, ce->name); cp.dir = submodule_path.buf; - argv[default_argc] = default_argv; - argv[argc] = submodule_prefix.buf; + argv_array_push(&argv, default_argv); + argv_array_push(&argv, "--submodule-prefix"); + argv_array_push(&argv, submodule_prefix.buf); + cp.argv = argv.argv; if (run_command(&cp)) result = 1; + argv_array_pop(&argv); + argv_array_pop(&argv); + argv_array_pop(&argv); } strbuf_release(&submodule_path); strbuf_release(&submodule_git_dir); strbuf_release(&submodule_prefix); } - free(argv); + argv_array_clear(&argv); out: string_list_clear(&changed_submodule_paths, 1); return result;
diff --git a/submodule.h b/submodule.h
index e105b0e..594b50d 100644
--- a/submodule.h
+++ b/submodule.h@@ -2,6 +2,7 @@ #define SUBMODULE_H struct diff_options; +struct argv_array; enum { RECURSE_SUBMODULES_ON_DEMAND = -1,
@@ -23,7 +24,7 @@ void show_submodule_summary(FILE *f, const char *path, const char *del, const char *add, const char *reset); void set_config_fetch_recurse_submodules(int value); void check_for_new_submodule_commits(unsigned char new_sha1[20]); -int fetch_populated_submodules(int num_options, const char **options, +int fetch_populated_submodules(const struct argv_array *options, const char *prefix, int command_line_option, int quiet); unsigned is_submodule_modified(const char *path, int ignore_untracked);
--
1.7.12.149.g47e61ec
From: Dan Johnson <hidden>
Date: 2016-06-15 22:54:38
When fetch is invoked with --all, we need to pass the tag-following preference to each individual fetch; without this, we will always auto-follow tags, preventing us from fetching the remote tags into a remote-specific namespace, for example. Reported-by: Oswald Buddenhagen <redacted> Signed-off-by: Dan Johnson <redacted> --- On Sat, Sep 1, 2012 at 7:22 AM, Jeff King [off-list ref] wrote:
Hmm. We allocate argv in fetch_multiple like this:
const char *argv[12] = { "fetch", "--append" };
and then add a bunch of options to it, along with the name of the
remote. By my count, the current code can hit exactly 12 (including the
terminating NULL) if all options are set. Your patch would make it
possible to overflow. Of course, I may be miscounting since it is
extremely error-prone to figure out the right number by tracing each
possible conditional.
Maybe we should switch it to a dynamic argv_array? Like this:
[1/2]: argv-array: add pop function
[2/2]: fetch: use argv_array instead of hand-building arraysThis version is re-rolled to be on top of jk/argv-array, avoiding the issue of the fixed-size array entirely. If needed, we could of course use the old version of this patch and bump the number, but I figure this is preferable. I've also added some test cases to cover this behavior, but I'm not entirely happy with them. I'm not sure if/how we should be testing the pass-through behavior of various arguments with fetch --all, but if so, we should probably do so more thouroughly than I have here, but that just seems like combining together tests of two unrelated things. It might just make more sense to ignore it and drop these tests, I don't know. Sorry this took me a few days to send, I just kept not getting around to it. builtin/fetch.c | 4 ++++ t/t5514-fetch-multiple.sh | 29 +++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 6196e91..4494aed 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c@@ -858,6 +858,10 @@ static void add_options_to_argv(struct argv_array *argv) argv_array_push(argv, "--recurse-submodules"); else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) argv_array_push(argv, "--recurse-submodules=on-demand"); + if (tags == TAGS_SET) + argv_array_push(argv, "--tags"); + else if (tags == TAGS_UNSET) + argv_array_push(argv, "--no-tags"); if (verbosity >= 2) argv_array_push(argv, "-v"); if (verbosity >= 1)
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index 227dd56..cbd2460 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh@@ -151,4 +151,33 @@ test_expect_success 'git fetch --multiple (ignoring skipFetchAll)' ' test_cmp ../expect output) ' +cat > expect << EOF +EOF + +test_expect_success 'git fetch --all --no-tags' ' + (git clone one test5 && + git clone test5 test6 && + (cd test5 && git tag test-tag) && + cd test6 && + git fetch --all --no-tags && + git tag >output && + test_cmp ../expect output) +' + +cat > expect << EOF +test-tag +EOF + +test_expect_success 'git fetch --all --tags' ' + (git clone one test7 && + git clone test7 test8 && + (cd test7 && + test_commit test-tag && + git reset --hard HEAD^) && + cd test8 && + git fetch --all --tags && + git tag >output && + test_cmp ../expect output) +' + test_done
--
1.7.11.1