* [PATCH/RFC] ref-filter: support sorting case-insensitively
From: Nguyễn Thái Ngọc Duy @ 2016-11-17 10:21 UTC (permalink / raw)
To: git; +Cc: karthik.188, Nguyễn Thái Ngọc Duy
Similar to version:refname sorting refs by versions, icase:refname
will sort by refnames are usually, but strcasecmp will be used instead
of strcmp. This may be helpful sometimes when people name their
branches <group>-<details> but somebody names it <group>-, some goes
with <Group>-, or even <GROUP>-
Syntax is a big problem. This patch does not support
icase:version:refname or version:icase:refname, for example. If
version sorting learns about this thing, I think I prefer
iversion:refname...
Or perhaps we can. I'm losing touch with for-each-ref "pretty"
formats, I'm not quite sure what's the guideline here.
Another option is just use a symbol, like '-' or '*' to mark
case-insensitivity. But that does not look very descriptive. I don't
see any symbol suggesting this case stuff.
What do you think?
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-for-each-ref.txt | 3 ++-
ref-filter.c | 8 ++++++--
ref-filter.h | 1 +
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index f57e69bc83..e41005cf0e 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -171,7 +171,8 @@ For sorting purposes, fields with numeric values sort in numeric order
All other fields are used to sort in their byte-value order.
There is also an option to sort by versions, this can be done by using
-the fieldname `version:refname` or its alias `v:refname`.
+the fieldname `version:refname` or its alias `v:refname`. Prefixing
+"icase:" (e.g. `icase:refname`) makes sorting case-insensitive.
In any case, a field name that refers to a field inapplicable to
the object referred by the ref does not cause an error. It
diff --git a/ref-filter.c b/ref-filter.c
index d4c2931f3a..fd63b9c710 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1542,12 +1542,12 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
if (s->version)
cmp = versioncmp(va->s, vb->s);
else if (cmp_type == FIELD_STR)
- cmp = strcmp(va->s, vb->s);
+ cmp = s->strcmp(va->s, vb->s);
else {
if (va->ul < vb->ul)
cmp = -1;
else if (va->ul == vb->ul)
- cmp = strcmp(a->refname, b->refname);
+ cmp = s->strcmp(a->refname, b->refname);
else
cmp = 1;
}
@@ -1646,6 +1646,7 @@ struct ref_sorting *ref_default_sorting(void)
sorting->next = NULL;
sorting->atom = parse_ref_filter_atom(cstr_name, cstr_name + strlen(cstr_name));
+ sorting->strcmp = strcmp;
return sorting;
}
@@ -1660,6 +1661,7 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
s = xcalloc(1, sizeof(*s));
s->next = *sorting_tail;
+ s->strcmp = strcmp;
*sorting_tail = s;
if (*arg == '-') {
@@ -1669,6 +1671,8 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
if (skip_prefix(arg, "version:", &arg) ||
skip_prefix(arg, "v:", &arg))
s->version = 1;
+ else if (skip_prefix(arg, "icase:", &arg))
+ s->strcmp = strcasecmp;
len = strlen(arg);
s->atom = parse_ref_filter_atom(arg, arg+len);
return 0;
diff --git a/ref-filter.h b/ref-filter.h
index 14d435e2cc..ea2db565f1 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -30,6 +30,7 @@ struct ref_sorting {
int atom; /* index into used_atom array (internal) */
unsigned reverse : 1,
version : 1;
+ int (*strcmp)(const char *, const char *);
};
struct ref_array_item {
--
2.11.0.rc0.161.g80d5b92
^ permalink raw reply related
* Re: [Bug?] git notes are not copied during rebase
From: SZEDER Gábor @ 2016-11-17 10:30 UTC (permalink / raw)
To: Norbert Kiesel; +Cc: SZEDER Gábor, git
In-Reply-To: <CAM+g_NvFhyReNREpTYKbXKm=8QmSH1tnrTCyFm9HusOnfAbCCA@mail.gmail.com>
> I am currently a heavy user of rebasing and noticed that my notes
> don't get correctly applied, even if notes.rewrite.rebase is set
> explicitly to true (though manual says that is the default).
Setting 'notes.rewrite.rebase' is, as you mentioned, not necessary,
but not sufficient either. See here, especially the second paragraph:
notes.rewriteRef
When copying notes during a rewrite, specifies the (fully
qualified) ref whose notes should be copied. May be a glob, in
which case notes in all matching refs will be copied. You may also
specify this configuration several times.
Does not have a default value; you must configure this variable to
enable note rewriting.
Can be overridden with the GIT_NOTES_REWRITE_REF environment
variable.
Gábor
^ permalink raw reply
* RE: merge --no-ff is NOT mentioned in help
From: Vanderhoof, Tzadik @ 2016-11-17 14:03 UTC (permalink / raw)
To: Mike Rappazzo; +Cc: git@vger.kernel.org
In-Reply-To: <CANoM8SVXeeZsc40xgVqZep_9oT=J2h4mOO0Ksn+kb0g8Ct=KrQ@mail.gmail.com>
-----Original Message-----
From: Mike Rappazzo [mailto:rappazzo@gmail.com]
Sent: Wednesday, November 16, 2016 7:58 AM
To: Vanderhoof, Tzadik
Cc: git@vger.kernel.org
Subject: Re: merge --no-ff is NOT mentioned in help
>(Please reply inline)
>
>On Wed, Nov 16, 2016 at 10:48 AM, Vanderhoof, Tzadik <tzadik.vanderhoof@optum360.com> wrote:
>> I am running: git version 2.10.1.windows.1
>>
>> I typed: git merge -h
>>
>> and got:
>>
>> usage: git merge [<options>] [<commit>...]
>> or: git merge [<options>] <msg> HEAD <commit>
>> or: git merge --abort
>>
>> -n do not show a diffstat at the end of the merge
>> --stat show a diffstat at the end of the merge
>> --summary (synonym to --stat)
>> --log[=<n>] add (at most <n>) entries from shortlog to merge commit message
>> --squash create a single commit instead of doing a merge
>> --commit perform a commit if the merge succeeds (default)
>> -e, --edit edit message before committing
>> --ff allow fast-forward (default)
>> --ff-only abort if fast-forward is not possible
>> --rerere-autoupdate update the index with reused conflict resolution if possible
>> --verify-signatures verify that the named commit has a valid GPG signature
>> -s, --strategy <strategy>
>> merge strategy to use
>> -X, --strategy-option <option=value>
>> option for selected merge strategy
>> -m, --message <message>
>> merge commit message (for a non-fast-forward merge)
>> -v, --verbose be more verbose
>> -q, --quiet be more quiet
>> --abort abort the current in-progress merge
>> --allow-unrelated-histories
>> allow merging unrelated histories
>> --progress force progress reporting
>> -S, --gpg-sign[=<key-id>]
>> GPG sign commit
>> --overwrite-ignore update ignored files (default)
>>
>> Notice there is NO mention of the "--no-ff" option
>
>I understand. On my system I can reproduce this by providing a bad argument to `git merge`. This is the output from the arg setup. For "boolean" arguments (like '--ff'), there is an automatic counter argument with "no-" in there ('--no-ff') to disable the option. Maybe it would make sense to word the output to include both.
>
Would you accept a pull request from me for this change?
>
>>
>> -----Original Message-----
>> From: Mike Rappazzo [mailto:rappazzo@gmail.com]
>> Sent: Wednesday, November 16, 2016 7:37 AM
>> To: Vanderhoof, Tzadik
>> Cc: git@vger.kernel.org
>> Subject: Re: merge --no-ff is NOT mentioned in help
>>
>> On Wed, Nov 16, 2016 at 10:16 AM, Vanderhoof, Tzadik <tzadik.vanderhoof@optum360.com> wrote:
>>> When I do: "git merge -h" to get help, the option "--no-ff" is left out of the list of options.
>>
>> I am running git version 2.10.0, and running git merge --help contains these lines:
>>
>> --ff
>> When the merge resolves as a fast-forward, only update the branch pointer, without creating a merge commit. This is the default behavior.
>>
>> --no-ff
>> Create a merge commit even when the merge resolves as a fast-forward. This is the default behaviour when merging an annotated (and possibly signed) tag.
>>
>> --ff-only
>> Refuse to merge and exit with a non-zero status unless the current HEAD is already up-to-date or the merge can be resolved as a fast-forward.
This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.
^ permalink raw reply
* Re: merge --no-ff is NOT mentioned in help
From: Junio C Hamano @ 2016-11-17 17:10 UTC (permalink / raw)
To: Mike Rappazzo; +Cc: Vanderhoof, Tzadik, git@vger.kernel.org
In-Reply-To: <CANoM8SVXeeZsc40xgVqZep_9oT=J2h4mOO0Ksn+kb0g8Ct=KrQ@mail.gmail.com>
Mike Rappazzo <rappazzo@gmail.com> writes:
> (Please reply inline)
Indeed ;-)
> On Wed, Nov 16, 2016 at 10:48 AM, Vanderhoof, Tzadik
> <tzadik.vanderhoof@optum360.com> wrote:
>> I am running: git version 2.10.1.windows.1
>>
>> I typed: git merge -h
>>
>> and got:
>>
>> usage: git merge [<options>] [<commit>...]
>> or: git merge [<options>] <msg> HEAD <commit>
>> or: git merge --abort
>>
>> -n do not show a diffstat at the end of the merge
>>...
>> --overwrite-ignore update ignored files (default)
>>
>> Notice there is NO mention of the "--no-ff" option
>
> I understand. On my system I can reproduce this by providing a bad
> argument to `git merge`. This is the output from the arg setup. For
> "boolean" arguments (like '--ff'), there is an automatic counter
> argument with "no-" in there ('--no-ff') to disable the option. Maybe
> it would make sense to word the output to include both.
I think that was a deliberate design decision to avoid cluttering
the short help text with mention of both --option and --no-option.
People interested may want to try the attached single-liner patch to
see how the output from _ALL_ commands that use parse-options API
looks when given "-h". It could be that the result may not be too
bad.
I suspect that we may discover that some options that should be
marked with NONEG are not marked along the way, which need to be
fixed.
parse-options.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/parse-options.c b/parse-options.c
index 312a85dbde..348be6b240 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -626,7 +626,9 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
if (opts->long_name && opts->short_name)
pos += fprintf(outfile, ", ");
if (opts->long_name)
- pos += fprintf(outfile, "--%s", opts->long_name);
+ pos += fprintf(outfile, "--%s%s",
+ (opts->flags & PARSE_OPT_NONEG) ? "" : "[no-]",
+ opts->long_name);
if (opts->type == OPTION_NUMBER)
pos += utf8_fprintf(outfile, _("-NUM"));
^ permalink raw reply related
* Re: [PATCH v4 0/4] Speedup finding of unpushed submodules
From: Stefan Beller @ 2016-11-17 17:41 UTC (permalink / raw)
To: Heiko Voigt
Cc: Junio C Hamano, Brandon Williams, git@vger.kernel.org, Jeff King,
Jens Lehmann, Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <cover.1479308877.git.hvoigt@hvoigt.net>
On Wed, Nov 16, 2016 at 7:11 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> You can find the third iteration of this series here:
>
> http://public-inbox.org/git/cover.1479221071.git.hvoigt@hvoigt.net/
>
> All comments from the last iteration should be addressed.
>
> Cheers Heiko
Thanks for this series!
I looked at the updated diff of hv/submodule-not-yet-pushed-fix
(git diff a1a385d..250ab24) and the series looks good to me.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 01/16] submodule.h: add extern keyword to functions, break line before 80
From: Stefan Beller @ 2016-11-17 18:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, Brandon Williams, Jonathan Nieder,
Martin Fick, David Turner
In-Reply-To: <xmqqshqrp7i8.fsf@gitster.mtv.corp.google.com>
On Wed, Nov 16, 2016 at 11:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> submodule.h: add extern keyword to functions, break line before 80
>
> The former is probably a good change for consistency. As the latter
> change breaks a workflow around quickly checking the output from
> "git grep funcname \*.h", I am not sure if it is a good idea.
>
> Especially things like this look like a usability regression:
>
> -void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
> +extern void handle_ignore_submodules_arg(struct diff_options *diffopt,
> + const char *);
>
> Perhaps the name "diffopt" can be dropped from there if we want it
> to be shorter.
Does the Git community have an opinion on dropping the name?
Usually the meaning can be inferred from the type such as in this example.
"diffopt" doesn't add information to my understanding when looking at
the header file.
One (cherry-picked) counter example is:
/**
* Like `strbuf_getwholeline`, but operates on a file descriptor.
* It reads one character at a time, so it is very slow. Do not
* use it unless you need the correct position in the file
* descriptor.
*/
extern int strbuf_getwholeline_fd(struct strbuf *, int, int);
where I'd need a bit of time to figure out which of the 2 ints is the
fd and which is the line termination character.
So I'd rather have a broken line than dropping names,
as when grepping for names I'd look them up most of the time
anyway (to e.g. read additional documentation or just reading the
source)
> Names in prototypes are valuable for parameters of
> more generic types like "char *", especially when there are more
> than one parameters of the same type, but in this case the typename
> is specific enough for readers to tell what it is.
Ok, that seems a reasonable to me.
I'll squash the changes below, with an updated commit message:
submodule.h: add extern keyword to functions
As the upcoming series will add a lot of functions to the submodule
header, let's first make the header consistent to the rest of the project
by adding the extern keyword to functions.
As per the CodingGuidelines we try to stay below 80 characters per line,
so adapt all those functions to stay below 80 characters that are already
using more than one line. Those function using just one line are better
kept in one line than breaking them up into multiple lines just for the
goal of staying below the character limit as it makes grepping
for functions easier if they are one liners.
(diff on top of the patch under discussion)
diff --git a/submodule.h b/submodule.h
index afc58d0..2082847 100644
--- a/submodule.h
+++ b/submodule.h
@@ -33,17 +33,14 @@ extern int is_staging_gitmodules_ok(void);
extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);
extern int remove_path_from_gitmodules(const char *path);
extern void stage_updated_gitmodules(void);
-extern void set_diffopt_flags_from_submodule_config(
- struct diff_options *diffopt,
+extern void set_diffopt_flags_from_submodule_config(struct diff_options *,
const char *path);
extern int submodule_config(const char *var, const char *value, void *cb);
extern void gitmodules_config(void);
extern int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst);
-extern const char *submodule_strategy_to_string(
- const struct submodule_update_strategy *s);
-extern void handle_ignore_submodules_arg(struct diff_options *diffopt,
- const char *);
+extern const char *submodule_strategy_to_string(const struct
submodule_update_strategy *s);
+extern void handle_ignore_submodules_arg(struct diff_options *, const char *);
extern void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
struct object_id *one, struct object_id *two,
@@ -72,8 +69,7 @@ extern int find_unpushed_submodules(unsigned char
new_sha1[20],
struct string_list *needs_pushing);
extern int push_unpushed_submodules(unsigned char new_sha1[20],
const char *remotes_name);
-extern void connect_work_tree_and_git_dir(const char *work_tree,
- const char *git_dir);
+extern void connect_work_tree_and_git_dir(const char *work_tree,
const char *git_dir);
extern int parallel_submodules(void);
/*
^ permalink raw reply related
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Junio C Hamano @ 2016-11-17 18:35 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <CAOLa=ZSaTdACC60g6D6k5frjKkChbkBL8+kLJjNgoutLSe8mOQ@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> On Tue, Nov 15, 2016 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jacob Keller <jacob.keller@gmail.com> writes:
>> ...
>> I think you are going in the right direction. I had a similar
>> thought but built around a different axis. I.e. if strip=1 strips
>> one from the left, perhaps we want to have rstrip=1 that strips one
>> from the right, and also strip=-1 to mean strip everything except
>> one from the left and so on?
> ...
> If we do implement strip with negative numbers, it definitely
> would be neat, but to get the desired feature which I've mentioned
> below, we'd need to call strip twice, i.e
> to get remotes from /refs/foo/abc/xyz we'd need to do
> strip=1,strip=-1, which could be
> done but ...
... would be unnecessary if this is the only use case:
> strbuf_addf(&fmt,
> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
> local.buf, remote.buf);
You can "strip to leave only 2 components" and compare the result
with refs/remotes instead, no?
^ permalink raw reply
* Re: [PATCH 02/16] submodule: modernize ok_to_remove_submodule to use argv_array
From: Stefan Beller @ 2016-11-17 18:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git@vger.kernel.org, bmwill@google.com,
jrnieder@gmail.com, mogulguy10@gmail.com
In-Reply-To: <xmqqwpg3p7rm.fsf@gitster.mtv.corp.google.com>
On Wed, Nov 16, 2016 at 11:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
> David Turner <David.Turner@twosigma.com> writes:
>
>>> - "-u",
>> ...
>>> + argv_array_pushl(&cp.args, "status", "--porcelain", "-uall",
>>
>> This also changes -u to -uall, which is not mentioned in the
>> commit message. That should probably be called out.
>
> Or not making that change at all. Isn't "-u" the same as "-uall"?
Yes it is. My original line of thinking was to have it spelled out
clearly in case
it changes in the future, but then we could argue that the --porcelain parameter
ought to keep the default of -u to "all".
I'll undo that change then.
Thanks,
Stefan
^ permalink raw reply
* [PATCH v2 0/2] bug fix with push --dry-run and submodules
From: Brandon Williams @ 2016-11-17 18:46 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, hvoigt, sbeller, j6t
In-Reply-To: <1479172735-698-1-git-send-email-bmwill@google.com>
v2 of this series is just a small cleanup of removing a nested sub-shell from a
test and rebasing on the latest version of
'origin/hv/submodule-not-yet-pushed-fix'
As stated above this series is based on 'origin/hv/submodule-not-yet-pushed-fix'
Brandon Williams (2):
push: --dry-run updates submodules when --recurse-submodules=on-demand
push: fix --dry-run to not push submodules
submodule.c | 13 ++++++++-----
submodule.h | 4 +++-
t/t5531-deep-submodule-push.sh | 24 ++++++++++++++++++++++++
transport.c | 11 +++++++----
4 files changed, 42 insertions(+), 10 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [PATCH v2 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand
From: Brandon Williams @ 2016-11-17 18:46 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, hvoigt, sbeller, j6t
In-Reply-To: <1479408364-150268-1-git-send-email-bmwill@google.com>
This patch adds a test to illustrate how push run with --dry-run doesn't
actually perform a dry-run when push is configured to push submodules
on-demand. Instead all submodules which need to be pushed are actually
pushed to their remotes while any updates for the superproject are
performed as a dry-run. This is a bug and not the intended behaviour of
a dry-run.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
t/t5531-deep-submodule-push.sh | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 198ce84..7840032 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -427,7 +427,31 @@ test_expect_success 'push unpushable submodule recursively fails' '
cd submodule.git &&
git rev-parse master >../actual
) &&
+ test_when_finished git -C work reset --hard master^ &&
test_cmp expected actual
'
+test_expect_failure 'push --dry-run does not recursively update submodules' '
+ (
+ cd work/gar/bage &&
+ git checkout master &&
+ git rev-parse master >../../../expected_submodule &&
+ > junk9 &&
+ git add junk9 &&
+ git commit -m "Ninth junk" &&
+
+ # Go up to 'work' directory
+ cd ../.. &&
+ git checkout master &&
+ git rev-parse master >../expected_pub &&
+ git add gar/bage &&
+ git commit -m "Ninth commit for gar/bage" &&
+ git push --dry-run --recurse-submodules=on-demand ../pub.git master
+ ) &&
+ git -C submodule.git rev-parse master >actual_submodule &&
+ git -C pub.git rev-parse master >actual_pub &&
+ test_cmp expected_pub actual_pub &&
+ test_cmp expected_submodule actual_submodule
+'
+
test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 2/2] push: fix --dry-run to not push submodules
From: Brandon Williams @ 2016-11-17 18:46 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, hvoigt, sbeller, j6t
In-Reply-To: <1479408364-150268-1-git-send-email-bmwill@google.com>
Teach push to respect the --dry-run option when configured to
recursively push submodules 'on-demand'. This is done by passing the
--dry-run flag to the child process which performs a push for a
submodules when performing a dry-run.
In order to preserve good user experience, the additional check for
unpushed submodules is skipped during a dry-run when
--recurse-submodules=on-demand. The check is skipped because the submodule
pushes were performed as dry-runs and this check would always fail as the
submodules would still need to be pushed.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
submodule.c | 13 ++++++++-----
submodule.h | 4 +++-
t/t5531-deep-submodule-push.sh | 2 +-
transport.c | 11 +++++++----
4 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/submodule.c b/submodule.c
index b509488..de20588 100644
--- a/submodule.c
+++ b/submodule.c
@@ -683,16 +683,17 @@ int find_unpushed_submodules(struct sha1_array *commits,
return needs_pushing->nr;
}
-static int push_submodule(const char *path)
+static int push_submodule(const char *path, int dry_run)
{
if (add_submodule_odb(path))
return 1;
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
struct child_process cp = CHILD_PROCESS_INIT;
- const char *argv[] = {"push", NULL};
+ argv_array_push(&cp.args, "push");
+ if (dry_run)
+ argv_array_push(&cp.args, "--dry-run");
- cp.argv = argv;
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
@@ -705,7 +706,9 @@ static int push_submodule(const char *path)
return 1;
}
-int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_name)
+int push_unpushed_submodules(struct sha1_array *commits,
+ const char *remotes_name,
+ int dry_run)
{
int i, ret = 1;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
@@ -716,7 +719,7 @@ int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_nam
for (i = 0; i < needs_pushing.nr; i++) {
const char *path = needs_pushing.items[i].string;
fprintf(stderr, "Pushing submodule '%s'\n", path);
- if (!push_submodule(path)) {
+ if (!push_submodule(path, dry_run)) {
fprintf(stderr, "Unable to push submodule '%s'\n", path);
ret = 0;
}
diff --git a/submodule.h b/submodule.h
index 9454806..23d7668 100644
--- a/submodule.h
+++ b/submodule.h
@@ -65,7 +65,9 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c
const unsigned char a[20], const unsigned char b[20], int search);
int find_unpushed_submodules(struct sha1_array *commits, const char *remotes_name,
struct string_list *needs_pushing);
-int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_name);
+extern int push_unpushed_submodules(struct sha1_array *commits,
+ const char *remotes_name,
+ int dry_run);
void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
int parallel_submodules(void);
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 7840032..1524ff5 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -431,7 +431,7 @@ test_expect_success 'push unpushable submodule recursively fails' '
test_cmp expected actual
'
-test_expect_failure 'push --dry-run does not recursively update submodules' '
+test_expect_success 'push --dry-run does not recursively update submodules' '
(
cd work/gar/bage &&
git checkout master &&
diff --git a/transport.c b/transport.c
index c3fdd5d..940b75d 100644
--- a/transport.c
+++ b/transport.c
@@ -921,15 +921,18 @@ int transport_push(struct transport *transport,
if (!is_null_oid(&ref->new_oid))
sha1_array_append(&commits, ref->new_oid.hash);
- if (!push_unpushed_submodules(&commits, transport->remote->name)) {
+ if (!push_unpushed_submodules(&commits,
+ transport->remote->name,
+ pretend)) {
sha1_array_clear(&commits);
- die("Failed to push all needed submodules!");
+ die ("Failed to push all needed submodules!");
}
sha1_array_clear(&commits);
}
- if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
- TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {
+ if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
+ ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) &&
+ !pretend)) && !is_bare_repository()) {
struct ref *ref = remote_refs;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
struct sha1_array commits = SHA1_ARRAY_INIT;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules
From: Stefan Beller @ 2016-11-17 18:59 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Heiko Voigt, Johannes Sixt
In-Reply-To: <1479408364-150268-3-git-send-email-bmwill@google.com>
On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:
> sha1_array_clear(&commits);
> - die("Failed to push all needed submodules!");
> + die ("Failed to push all needed submodules!");
huh? Is this a whitespace change?
^ permalink raw reply
* Re: [PATCH v2 0/2] bug fix with push --dry-run and submodules
From: Stefan Beller @ 2016-11-17 19:01 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Heiko Voigt, Johannes Sixt
In-Reply-To: <1479408364-150268-1-git-send-email-bmwill@google.com>
On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:
> v2 of this series is just a small cleanup of removing a nested sub-shell from a
> test and rebasing on the latest version of
> 'origin/hv/submodule-not-yet-pushed-fix'
>
> As stated above this series is based on 'origin/hv/submodule-not-yet-pushed-fix'
an interdiff to v1 would be nice :)
Now t5531 is inconsistent in style,
how much time would you estimate to add a commit to refactor
that test to follow the style with excessive use of -C for
all the other tests and avoiding subshells there, too?
(It's mainly Windows that benefits from such a refactoring immediately,
but consistency is a really nice feat for the long term code health
of the Git project)
Apart from one nit (and this refactoring decision/consistency discussion),
the series looks good!
Thanks,
Stefan
^ permalink raw reply
* [Bug] gpgsign bashcompletion not available
From: NicoHood @ 2016-11-17 18:52 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 484 bytes --]
The gpgsign feature is not available via bashcompletion on git.
$ git config --global commit.
commit.status commit.template
$ git config --global commit.gpgsign true
$ git --version
git version 2.10.2
Due to this I could also find no option to automatically sign every tag
that I do. If this option does not exist yet, please add it to the todo
list. If those reports are tracked anywhere else, please also let me
know, so I can test that out.
Cheers,
Nico
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules
From: Brandon Williams @ 2016-11-17 19:02 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Heiko Voigt, Johannes Sixt
In-Reply-To: <CAGZ79kY1x1HWJFjiyFdMFh8S_Y1F0ecLB5-JPb+nPE0gujfF-A@mail.gmail.com>
On 11/17, Stefan Beller wrote:
> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:
>
> > sha1_array_clear(&commits);
> > - die("Failed to push all needed submodules!");
> > + die ("Failed to push all needed submodules!");
>
> huh? Is this a whitespace change?
That's odd...I didn't mean to add that lone space.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v2 0/2] bug fix with push --dry-run and submodules
From: Brandon Williams @ 2016-11-17 19:06 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Heiko Voigt, Johannes Sixt
In-Reply-To: <CAGZ79kYC51zC4nF3crtuJXK7uwK=Lh9X8LnRke5C87Vo46Gb+A@mail.gmail.com>
On 11/17, Stefan Beller wrote:
> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:
> > v2 of this series is just a small cleanup of removing a nested sub-shell from a
> > test and rebasing on the latest version of
> > 'origin/hv/submodule-not-yet-pushed-fix'
> >
> > As stated above this series is based on 'origin/hv/submodule-not-yet-pushed-fix'
>
> an interdiff to v1 would be nice :)
>
> Now t5531 is inconsistent in style,
> how much time would you estimate to add a commit to refactor
> that test to follow the style with excessive use of -C for
> all the other tests and avoiding subshells there, too?
I didn't change to an excessive use of the -C option, but rather
eliminated the nested-subshell and instead cd'ed to the required
directories in the subshell. Excessive use of -C seemed to greatly
reduce the readability of the test (at least it did to me).
--
Brandon Williams
^ permalink raw reply
* Re: merge --no-ff is NOT mentioned in help
From: Torsten Bögershausen @ 2016-11-17 19:18 UTC (permalink / raw)
To: Junio C Hamano, Mike Rappazzo; +Cc: Vanderhoof, Tzadik, git@vger.kernel.org
In-Reply-To: <xmqqr36anibl.fsf@gitster.mtv.corp.google.com>
On 17/11/16 18:10, Junio C Hamano wrote:
> Mike Rappazzo <rappazzo@gmail.com> writes:
>
>> (Please reply inline)
> Indeed ;-)
>
>> On Wed, Nov 16, 2016 at 10:48 AM, Vanderhoof, Tzadik
>> <tzadik.vanderhoof@optum360.com> wrote:
>>> I am running: git version 2.10.1.windows.1
>>>
>>> I typed: git merge -h
>>>
>>> and got:
>>>
>>> usage: git merge [<options>] [<commit>...]
>>> or: git merge [<options>] <msg> HEAD <commit>
>>> or: git merge --abort
>>>
>>> -n do not show a diffstat at the end of the merge
>>> ...
>>> --overwrite-ignore update ignored files (default)
>>>
>>> Notice there is NO mention of the "--no-ff" option
>> I understand. On my system I can reproduce this by providing a bad
>> argument to `git merge`. This is the output from the arg setup. For
>> "boolean" arguments (like '--ff'), there is an automatic counter
>> argument with "no-" in there ('--no-ff') to disable the option. Maybe
>> it would make sense to word the output to include both.
> I think that was a deliberate design decision to avoid cluttering
> the short help text with mention of both --option and --no-option.
>
> People interested may want to try the attached single-liner patch to
> see how the output from _ALL_ commands that use parse-options API
> looks when given "-h". It could be that the result may not be too
> bad.
>
> I suspect that we may discover that some options that should be
> marked with NONEG are not marked along the way, which need to be
> fixed.
>
>
> parse-options.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 312a85dbde..348be6b240 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -626,7 +626,9 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
> if (opts->long_name && opts->short_name)
> pos += fprintf(outfile, ", ");
> if (opts->long_name)
> - pos += fprintf(outfile, "--%s", opts->long_name);
> + pos += fprintf(outfile, "--%s%s",
> + (opts->flags & PARSE_OPT_NONEG) ? "" : "[no-]",
> + opts->long_name);
> if (opts->type == OPTION_NUMBER)
> pos += utf8_fprintf(outfile, _("-NUM"));
>
+1 from my side
(As I once spend some time to find out that the "no--" is automatically available)
^ permalink raw reply
* Re: [PATCH v7 16/17] branch: use ref-filter printing APIs
From: Junio C Hamano @ 2016-11-17 19:50 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jacob.keller
In-Reply-To: <20161108201211.25213-17-Karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> +static char *build_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
> +{
I understand that the return value of this function is used as if
the value given via --format=... option to for-each-ref.
> + struct strbuf fmt = STRBUF_INIT;
> + struct strbuf local = STRBUF_INIT;
> + struct strbuf remote = STRBUF_INIT;
> +
> + strbuf_addf(&fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)", branch_get_color(BRANCH_COLOR_CURRENT));
This switches between "* " and " " prefixed for each line of output
in "git branch --list" output, where an asterisk is used to mark the
branch that is currently checked out. OK.
> + if (filter->verbose) {
> + strbuf_addf(&local, "%%(align:%d,left)%%(refname:strip=2)%%(end)", maxwidth);
> + strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
> + strbuf_addf(&local, " %%(objectname:short=7) ");
> +
> + if (filter->verbose > 1)
> + strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
> + "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
> + branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
> + else
> + strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
> +
> + strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:strip=2)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
> + "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
> + branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
> + remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> + } else {
> + strbuf_addf(&local, "%%(refname:strip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> + branch_get_color(BRANCH_COLOR_RESET));
> + strbuf_addf(&remote, "%s%s%%(refname:strip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> + branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> + }
This block prepares "local" and "remote", two formats that are used
for local and remote branches.
> + strbuf_addf(&fmt, "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)", local.buf, remote.buf);
And this uses the %(if)...%(then)...%(else)...%(end) construct to
switch between these formats.
Sounds good.
One worry that I have is if the strings embedded in this function to
the final format are safe. As far as I can tell, the pieces of
strings that are literally inserted into the resulting format string
by this function are maxwidth, remote_prefix, and return values from
branch_get_color() calls.
The maxwidth is inserted via "%d" and made into decimal constant,
and there is no risk for it being in the resulting format. Are
the return values of branch_get_color() calls safe? I do not think
they can have '%' in them, but if they do, they need to be quoted.
The same worry exists for remote_prefix. Currently it can either be
an empty string or "remotes/", and is safe to be embedded in a
format string.
^ permalink raw reply
* Re: [PATCH 07/16] update submodules: introduce submodule_is_interesting
From: Stefan Beller @ 2016-11-17 20:03 UTC (permalink / raw)
To: David Turner
Cc: git@vger.kernel.org, bmwill@google.com, gitster@pobox.com,
jrnieder@gmail.com, mogulguy10@gmail.com
In-Reply-To: <e748abfad8b04a8eaaa10797d9324891@exmbdft7.ad.twosigma.com>
On Tue, Nov 15, 2016 at 4:14 PM, David Turner <David.Turner@twosigma.com> wrote:
>> +int submodule_is_interesting(const char *path, const unsigned char
>> +*sha1) {
>
> This is apparently only ever (in this series) called with null_sha1. So either this arg is unnecessary, or there are bugs elsewhere in the code.
I was torn when writing the series, as I initially had submodule_is_interesting
with no sha1 argument and it turned out to be buggy in my first
initial implementation,
which lead me to thinking the sha1 actually matters.
The line of thinking was similar to loading the submodules from the
submodule-config cache as that also has different values for different sha1s,
e.g. a submodule is only interesting if submodule.<name>.update != none,
which can have changed with different sha1s.
I refactored the series since then to call the _is_initeresting method
at different times
(before and after the actual checkout), such that we implicitly have
the correct sha1
while calling it.
So I would argue the sha1 argument is not needed. I'll remove it.
^ permalink raw reply
* Re: [PATCH 07/16] update submodules: introduce submodule_is_interesting
From: Stefan Beller @ 2016-11-17 20:08 UTC (permalink / raw)
To: Heiko Voigt
Cc: git@vger.kernel.org, Brandon Williams, Junio C Hamano,
Jonathan Nieder, Martin Fick, David Turner
In-Reply-To: <20161117105715.GC39230@book.hvoigt.net>
On Thu, Nov 17, 2016 at 2:57 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> It seems that you are only looking at the submodule config from a
> commit. Should a user be able to override this with local configuration?
> Haven't looked further in the patchseries so maybe that is somewhere
> else?
It turns out that in later patches we pass in null_sha1 only, which is
looking at the config and possible overrides.
I'll refactor to take no sha1 argument and use null_sha1 here directly.
^ permalink raw reply
* Re: [PATCH v15 10/27] bisect--helper: `check_and_set_terms` shell function in C
From: Stephan Beyer @ 2016-11-17 20:25 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1aca-0c26fb8c-404f-4f57-afe7-7ebb552a1002-000000@eu-west-1.amazonses.com>
Hi Pranit,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3f19b68..c6c11e3 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -20,6 +20,7 @@ static const char * const git_bisect_helper_usage[] = {
> N_("git bisect--helper --bisect-clean-state"),
> N_("git bisect--helper --bisect-reset [<commit>]"),
> N_("git bisect--helper --bisect-write <state> <revision> <TERM_GOOD> <TERM_BAD> [<nolog>]"),
> + N_("git bisect--helper --bisect-check-and-set-terms <command> <TERM_GOOD> <TERM_BAD>"),
Here's the same as in the previous patch... I'd not use
TERM_GOOD/TERM_BAD in capitals.
> NULL
> };
>
> @@ -212,6 +213,38 @@ static int bisect_write(const char *state, const char *rev,
> return retval;
> }
>
> +static int set_terms(struct bisect_terms *terms, const char *bad,
> + const char *good)
> +{
> + terms->term_good = xstrdup(good);
> + terms->term_bad = xstrdup(bad);
> + return write_terms(terms->term_bad, terms->term_good);
At this stage of the patch series I am wondering why you are setting
"terms" here, but I guess you'll need it later.
However, you are leaking memory here. Something like
free(terms->term_good);
free(terms->term_bad);
terms->term_good = xstrdup(good);
terms->term_bad = xstrdup(bad);
should be safe (because you've always used xstrdup() for the terms
members before). Or am I overseeing something?
> @@ -278,6 +314,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> terms.term_bad = xstrdup(argv[3]);
> res = bisect_write(argv[0], argv[1], &terms, nolog);
> break;
> + case CHECK_AND_SET_TERMS:
> + if (argc != 3)
> + die(_("--check-and-set-terms requires 3 arguments"));
> + terms.term_good = xstrdup(argv[1]);
> + terms.term_bad = xstrdup(argv[2]);
> + res = check_and_set_terms(&terms, argv[0]);
> + break;
Ha! When I reviewed the last patch, I asked you why you changed the code
from returning directly from each subcommand to setting res; break; and
then return res at the bottom of the function.
Now I see why this was useful. The two members of "terms" are again
leaking memory: you are allocating memory by using xstrdup() but you are
not freeing it.
(That also applies to the last patch.)
Cheers,
Stephan
^ permalink raw reply
* Re: [PATCH v15 11/27] bisect--helper: `bisect_next_check` & bisect_voc shell function in C
From: Stephan Beyer @ 2016-11-17 20:59 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1adb-ab4c90ed-d084-40b5-a037-f62c76e52ec4-000000@eu-west-1.amazonses.com>
Hi Pranit,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> Also reimplement `bisect_voc` shell function in C and call it from
> `bisect_next_check` implementation in C.
Please don't! ;D
> +static char *bisect_voc(char *revision_type)
> +{
> + if (!strcmp(revision_type, "bad"))
> + return "bad|new";
> + if (!strcmp(revision_type, "good"))
> + return "good|old";
> +
> + return NULL;
> +}
Why not simply use something like this:
static const char *voc[] = {
"bad|new",
"good|old",
};
Then...
> +static int bisect_next_check(const struct bisect_terms *terms,
> + const char *current_term)
> +{
> + int missing_good = 1, missing_bad = 1, retval = 0;
> + char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> + char *good_glob = xstrfmt("%s-*", terms->term_good);
> + char *bad_syn, *good_syn;
...you don't need bad_syn and good_syn...
> + bad_syn = xstrdup(bisect_voc("bad"));
> + good_syn = xstrdup(bisect_voc("good"));
...and hence not these two lines...
> + if (!is_empty_or_missing_file(git_path_bisect_start())) {
> + error(_("You need to give me at least one %s and "
> + "%s revision. You can use \"git bisect %s\" "
> + "and \"git bisect %s\" for that. \n"),
> + bad_syn, good_syn, bad_syn, good_syn);
...and write
voc[0], voc[1], voc[0], voc[1]);
instead...
> + retval = -1;
> + goto finish;
> + }
> + else {
> + error(_("You need to start by \"git bisect start\". You "
> + "then need to give me at least one %s and %s "
> + "revision. You can use \"git bisect %s\" and "
> + "\"git bisect %s\" for that.\n"),
> + good_syn, bad_syn, bad_syn, good_syn);
...and here
voc[1], voc[0], voc[0], voc[1]);
...
> + retval = -1;
> + goto finish;
> + }
> + goto finish;
> +finish:
> + if (!bad_ref)
> + free(bad_ref);
> + if (!good_glob)
> + free(good_glob);
> + if (!bad_syn)
> + free(bad_syn);
> + if (!good_syn)
> + free(good_syn);
...and you can remove the 4 lines above.
> + return retval;
> +}
Besides that, there are again some things that I've already mentioned
and that can be applied here, too, for example, not capitalizing
TERM_GOOD and TERM_BAD, the goto fail simplification, the terms memory leak.
Cheers
Stephan
^ permalink raw reply
* Re: [PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
From: Stephan Beyer @ 2016-11-17 21:32 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1ad5-0f90c88e-2077-4155-94e9-7d71dbbac38f-000000@eu-west-1.amazonses.com>
Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 317d671..6a5878c 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
[...]
> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int argc)
> +{
> + int i;
> + const char bisect_term_usage[] =
> +"git bisect--helper --bisect-terms [--term-good | --term-bad | ]"
> +"--term-old | --term-new";
Three things:
(1) Is that indentation intentional?
(2) You have a "]" at the end of the first part of the string instead of
the end of the second part.
(3) After the correction, bisect_term_usage and
git_bisect_helper_usage[7] are the same strings. I don't recommend to
use git_bisect_helper_usage[7] instead because keeping the index
up-to-date is a maintenance hell. (At the end of your patch series it is
a 3 instead of a 7.) However, if - for whatever reason - the usage of
bisect--helper --bisect-terms changes, you always have to sync the two
strings which is also nasty....
> +
> + if (get_terms(terms))
> + return error(_("no terms defined"));
> +
> + if (argc > 1) {
> + usage(bisect_term_usage);
> + return -1;
> + }
...and since you only use it once, why not simply do something like
return error(_("--bisect-term requires exactly one argument"));
and drop the definition of bisect_term_usage.
> +
> + if (argc == 0) {
> + printf(_("Your current terms are %s for the old state\nand "
> + "%s for the new state.\n"), terms->term_good,
> + terms->term_bad);
Very minor: It improves the readability if you'd split the string after
the \n and put the "and "in the next line.
> + return 0;
> + }
> +
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--term-good"))
> + printf("%s\n", terms->term_good);
> + else if (!strcmp(argv[i], "--term-bad"))
> + printf("%s\n", terms->term_bad);
> + else
> + die(_("invalid argument %s for 'git bisect "
> + "terms'.\nSupported options are: "
> + "--term-good|--term-old and "
> + "--term-bad|--term-new."), argv[i]);
Hm, "return error(...)" and "die(...)" seems to be quasi-equivalent in
this case. Because I am always looking from a library perspective, I'd
prefer "return error(...)".
> @@ -429,6 +492,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> terms.term_bad = xstrdup(argv[1]);
> res = bisect_next_check(&terms, argc == 3 ? argv[2] : NULL);
> break;
> + case BISECT_TERMS:
> + if (argc > 1)
> + die(_("--bisect-terms requires 0 or 1 argument"));
> + res = bisect_terms(&terms, argv, argc);
> + break;
Also here: "terms" is leaking...
~Stephan
^ permalink raw reply
* Re: [PATCH v15 22/27] bisect--helper: `bisect_log` shell function in C
From: Stephan Beyer @ 2016-11-17 21:47 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1b18-b81203b0-122f-4244-bfb2-9fac8ae71767-000000@eu-west-1.amazonses.com>
Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 493034c..c18ca07 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -858,6 +858,23 @@ static int bisect_state(struct bisect_terms *terms, const char **argv,
> return -1;
> }
>
> +static int bisect_log(void)
> +{
> + int fd, status;
> + fd = open(git_path_bisect_log(), O_RDONLY);
> + if (fd < 0)
> + return -1;
> +
> + status = copy_fd(fd, 1);
Perhaps
status = copy_fd(fd, STDOUT_FILENO);
> + if (status) {
> + close(fd);
> + return -1;
> + }
> +
> + close(fd);
> + return status;
> +}
That's weird.
Either get rid of the if() and actually use status:
status = copy_fd(fd, STDOUT_FILENO);
close(fd);
return status ? -1 : 0;
or get rid of status and use the if:
if (copy_fd(fd, STDOUT_FILENO)) {
close(fd);
return -1;
}
close(fd);
return 0;
I'd recommend the shorter variant ;)
~Stephan
^ permalink raw reply
* Re: [PATCH v7 16/17] branch: use ref-filter printing APIs
From: Junio C Hamano @ 2016-11-17 22:05 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jacob.keller
In-Reply-To: <xmqqinrlopge.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> One worry that I have is if the strings embedded in this function to
> the final format are safe. As far as I can tell, the pieces of
> strings that are literally inserted into the resulting format string
> by this function are maxwidth, remote_prefix, and return values from
> branch_get_color() calls.
>
> The maxwidth is inserted via "%d" and made into decimal constant,
> and there is no risk for it being in the resulting format. Are
> the return values of branch_get_color() calls safe? I do not think
> they can have '%' in them, but if they do, they need to be quoted.
> The same worry exists for remote_prefix. Currently it can either be
> an empty string or "remotes/", and is safe to be embedded in a
> format string.
In case it was not clear, in short, I do not think there is anything
broken in the code, but it is a longer-term improvement to introduce
a helper that takes a string and returns a version of the string
that is safely quoted to be used in the for-each-ref format string
use it like so:
strbuf_addf(&remote,
"%s"
"%%(align:%d,left)%s%%(refname:strip=2)%%(end)"
...
"%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
quote_literal_for_format(branch_get_color(BRANCH_COLOR_REMOTE)),
...);
and the implementation of the helper may look like:
const char *quote_literal_for_format(const char *s)
{
static strbuf buf = STRBUF_INIT;
strbuf_reset(&buf);
while (*s) {
const char *ep = strchrnul(s, '%');
if (s < ep)
strbuf_add(&buf, s, ep - s);
if (*ep == '%') {
strbuf_addstr(&buf, "%%");
s = ep + 1;
} else {
s = ep;
}
}
return buf.buf;
}
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox