* [PATCH 1/2] name-rev: allow passing multiple patterns using --refs
From: Jacob Keller @ 2016-12-07 2:32 UTC (permalink / raw)
To: git; +Cc: Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
Allow git name-rev to take a string list of patterns instead of only
a single pattern. All patterns are matched inclusively, meaning that
a ref only needs to match one pattern to be included.
Add tests and documentation for this change.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Documentation/git-name-rev.txt | 4 +++-
builtin/name-rev.c | 41 +++++++++++++++++++++++++-----------
t/t6007-rev-list-cherry-pick-file.sh | 30 ++++++++++++++++++++++++++
3 files changed, 62 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index ca28fb8e2a07..7433627db12d 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -26,7 +26,9 @@ OPTIONS
--refs=<pattern>::
Only use refs whose names match a given shell pattern. The pattern
- can be one of branch name, tag name or fully qualified ref name.
+ can be one of branch name, tag name or fully qualified ref name. If
+ given multiple times, use refs whose names match any of the given shell
+ patterns. Use `--no-refs` to clear any previous ref patterns given.
--all::
List all commits reachable from all refs
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48b65e8..000a2a700ed3 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -108,7 +108,7 @@ static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
struct name_ref_data {
int tags_only;
int name_only;
- const char *ref_filter;
+ struct string_list ref_filters;
};
static struct tip_table {
@@ -150,16 +150,33 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
if (data->tags_only && !starts_with(path, "refs/tags/"))
return 0;
- if (data->ref_filter) {
- switch (subpath_matches(path, data->ref_filter)) {
- case -1: /* did not match */
- return 0;
- case 0: /* matched fully */
- break;
- default: /* matched subpath */
- can_abbreviate_output = 1;
- break;
+ if (data->ref_filters.nr) {
+ struct string_list_item *item;
+ int matched = 0;
+
+ /* See if any of the patterns match. */
+ for_each_string_list_item(item, &data->ref_filters) {
+ /*
+ * We want to check every pattern even if we already
+ * found a match, just in case one of the later
+ * patterns could abbreviate the output.
+ */
+ switch (subpath_matches(path, item->string)) {
+ case -1: /* did not match */
+ break;
+ case 0: /* matched fully */
+ matched = 1;
+ break;
+ default: /* matched subpath */
+ matched = 1;
+ can_abbreviate_output = 1;
+ break;
+ }
}
+
+ /* If none of the patterns matched, stop now */
+ if (!matched)
+ return 0;
}
add_to_tip_table(oid->hash, path, can_abbreviate_output);
@@ -306,11 +323,11 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
{
struct object_array revs = OBJECT_ARRAY_INIT;
int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
- struct name_ref_data data = { 0, 0, NULL };
+ struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP };
struct option opts[] = {
OPT_BOOL(0, "name-only", &data.name_only, N_("print only names (no SHA-1)")),
OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")),
- OPT_STRING(0, "refs", &data.ref_filter, N_("pattern"),
+ OPT_STRING_LIST(0, "refs", &data.ref_filters, N_("pattern"),
N_("only use refs matching <pattern>")),
OPT_GROUP(""),
OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 1408b608eb03..d072ec43b016 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -99,6 +99,36 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
test_cmp actual.named expect
'
+test_expect_success 'name-rev multiple --refs combine inclusive' '
+ git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+ git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
+ < actual > actual.named &&
+ test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+<tags/F
+$(git rev-list --left-right --right-only --cherry-pick F...E -- bar)
+EOF
+
+test_expect_success 'name-rev --refs excludes non-matched patterns' '
+ git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+ git name-rev --stdin --name-only --refs="*tags/F" \
+ < actual > actual.named &&
+ test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+$(git rev-list --left-right --cherry-pick F...E -- bar)
+EOF
+
+test_expect_success 'name-rev --no-refs clears the refs list' '
+ git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+ git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
+ < actual > actual.named &&
+ test_cmp actual.named expect
+'
+
cat >expect <<EOF
+tags/F
=tags/D
--
2.11.0.rc2.152.g4d04e67
^ permalink raw reply related
* [PATCH 2/2] describe: add support for multiple match patterns
From: Jacob Keller @ 2016-12-07 2:32 UTC (permalink / raw)
To: git; +Cc: Jacob Keller
In-Reply-To: <20161207023259.29355-1-jacob.e.keller@intel.com>
From: Jacob Keller <jacob.keller@gmail.com>
Convert "--match" into a string list that accumulates patterns. If any
patterns are given, then include all tags that match at least one
pattern. This allows the user to construct multiple small patterns and
compose them. If desired, a future patch could expand functionality by
adding a new option to determine the style of combining multiple
patterns.
The primary use of this feature can be seen when trying to find which
release tag a given commit made it into. Suppose that you version all
your official releases such as "v1.2", "v1.3", "v1.4", "v2.1" and so on.
Now, you also have other tags which represent -rc releases and other
such tags. If you want to find the first major release that contains
a given commit you might try
git describe --contains --match="v?.?" <commit>
This will work as long as you have only single digits. But if you start
adding multiple digits, the pattern becomes not enough to match all the
tags you wanted while excluding the ones you didn't.
By implementing multiple --match invocations, this can be avoided by
simply passing multiple match patterns to the command:
git describe --contains --match="v[0-9].[0-9]" --match="v[0-9].[0-9][0-9]" ...
The end result is the ability to easily search tag space for which
tag included a given commit, without including -rc and other tags which
aren't interesting to you.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Documentation/git-describe.txt | 6 +++++-
builtin/describe.c | 31 ++++++++++++++++++++++++-------
t/t6120-describe.sh | 19 +++++++++++++++++++
3 files changed, 48 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index e4ac448ff565..c85f2811ce28 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -83,7 +83,11 @@ OPTIONS
--match <pattern>::
Only consider tags matching the given `glob(7)` pattern,
excluding the "refs/tags/" prefix. This can be used to avoid
- leaking private tags from the repository.
+ leaking private tags from the repository, or to shrink the scope of
+ searched tags to avoid -rc tags or similar. If given multiple a list
+ of patterns will be accumulated, and tags matching any of the patterns
+ will be considered. Use `--no-match` to clear and reset the list of
+ patterns.
--always::
Show uniquely abbreviated commit object as fallback.
diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a157efc..e3ceab65e273 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -28,7 +28,7 @@ static int abbrev = -1; /* unspecified */
static int max_candidates = 10;
static struct hashmap names;
static int have_util;
-static const char *pattern;
+static struct string_list patterns = STRING_LIST_INIT_NODUP;
static int always;
static const char *dirty;
@@ -129,9 +129,25 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi
if (!all && !is_tag)
return 0;
- /* Accept only tags that match the pattern, if given */
- if (pattern && (!is_tag || wildmatch(pattern, path + 10, 0, NULL)))
- return 0;
+ /*
+ * If we're given patterns, accept only tags which match at least one
+ * pattern.
+ */
+ if (patterns.nr) {
+ struct string_list_item *item;
+
+ if (!is_tag)
+ return 0;
+
+ for_each_string_list_item(item, &patterns) {
+ if (!wildmatch(item->string, path + 10, 0, NULL)) {
+ break;
+ }
+
+ /* If we get here, no pattern matched. */
+ return 0;
+ }
+ }
/* Is it annotated? */
if (!peel_ref(path, peeled.hash)) {
@@ -404,7 +420,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
N_("only output exact matches"), 0),
OPT_INTEGER(0, "candidates", &max_candidates,
N_("consider <n> most recent tags (default: 10)")),
- OPT_STRING(0, "match", &pattern, N_("pattern"),
+ OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
N_("only consider tags matching <pattern>")),
OPT_BOOL(0, "always", &always,
N_("show abbreviated commit object as fallback")),
@@ -430,6 +446,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
die(_("--long is incompatible with --abbrev=0"));
if (contains) {
+ struct string_list_item *item;
struct argv_array args;
argv_array_init(&args);
@@ -440,8 +457,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
argv_array_push(&args, "--always");
if (!all) {
argv_array_push(&args, "--tags");
- if (pattern)
- argv_array_pushf(&args, "--refs=refs/tags/%s", pattern);
+ for_each_string_list_item(item, &patterns)
+ argv_array_pushf(&args, "--refs=refs/tags/%s", item->string);
}
if (argc)
argv_array_pushv(&args, argv);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 85f269411cb3..9e5db9b87a1f 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -182,6 +182,10 @@ check_describe "test2-lightweight-*" --tags --match="test2-*"
check_describe "test2-lightweight-*" --long --tags --match="test2-*" HEAD^
+check_describe "test1-lightweight-*" --long --tags --match="test1-*" --match="test2-*" HEAD^
+
+check_describe "test2-lightweight-*" --long --tags --match="test1-*" --no-match --match="test2-*" HEAD^
+
test_expect_success 'name-rev with exact tags' '
echo A >expect &&
tag_object=$(git rev-parse refs/tags/A) &&
@@ -206,4 +210,19 @@ test_expect_success 'describe --contains with the exact tags' '
test_cmp expect actual
'
+test_expect_success 'describe --contains and --match' '
+ echo "A^0" >expect &&
+ tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+ test_must_fail git describe --contains --match="B" $tagged_commit &&
+ git describe --contains --match="B" --match="A" $tagged_commit >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'describe --contains and --no-match' '
+ echo "A^0" >expect &&
+ tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+ git describe --contains --match="B" --no-match $tagged_commit >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.11.0.rc2.152.g4d04e67
^ permalink raw reply related
* git add -p with new file
From: Ariel @ 2016-12-07 1:18 UTC (permalink / raw)
To: git
If you do git add -p new_file it says:
No changes.
Which is a rather confusing message. I would expect it to show me the
content of the file in patch form, in the normal way that -p works, let me
edit it, etc.
(Note: I am aware I can do -N first, but when I specifically enter the
name of a new file I feel it should figure out what I mean.)
-Ariel
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Ramsay Jones @ 2016-12-07 1:12 UTC (permalink / raw)
To: Brandon Williams, Junio C Hamano; +Cc: git, sbeller, peff, jacob.keller
In-Reply-To: <20161207001018.GD103573@google.com>
On 07/12/16 00:10, Brandon Williams wrote:
> On 12/06, Junio C Hamano wrote:
>> POSIX cares about treating "//" at the very beginning of the path
>> specially. Is that supposed to be handled here, or by a lot higher
>> level up in the callchain?
>
> What exactly does "//" mean in this context? (I'm just naive in this
> area)
This refers to a UNC path (ie Universal Naming Convention) which
is used to refer to servers, printers and other 'network resources'.
Although this started (many moons ago) in unix, it isn't used too
much outside of windows networks! (where it is usually denoted by
\\servername\path).
You can see the relics of unix UNC paths if you look at the wording
for basename() in the POSIX standard. Note the special treatment of
the path which 'is exactly "//"', see http://pubs.opengroup.org/onlinepubs/009695399/functions/basename.html
ATB,
Ramsay Jones
^ permalink raw reply
* (no subject)
From: jbh @ 2016-12-07 1:00 UTC (permalink / raw)
To: git
In-Reply-To: <c35518dc-5278-bd89-569c-398e70aa8dc8@drbeat.li>
unsubscribe
^ permalink raw reply
* Re: [PATCH v4 1/3] update-unicode.sh: automatically download newer definition files
From: Beat Bolli @ 2016-12-07 0:17 UTC (permalink / raw)
To: Junio C Hamano, Torsten Bögershausen; +Cc: git
In-Reply-To: <xmqqmvga3yod.fsf@gitster.mtv.corp.google.com>
On 05.12.16 21:31, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> On Sat, Dec 03, 2016 at 10:00:47PM +0100, Beat Bolli wrote:
>>> Checking just for the unicode data files' existence is not sufficient;
>>> we should also download them if a newer version exists on the Unicode
>>> consortium's servers. Option -N of wget does this nicely for us.
>>>
>>> Reviewed-by: Torsten Boegershausen <tboegi@web.de>
>>
>> Minor remark (Not sure if this motivates v5, may be Junio can fix it locally?)
>> s/oe/ö/
>>
>> Beside this: Thanks again (and I learned about the -N option of wget)
>
> Will fix up while queuing (only 1/3 needs it, 2/3 has it right).
>
> Also, I'll do s/update-unicode.sh/update_unicode.sh/ on the title
> and the message to match the reality. At some point we might want
> to fix the reality to match people's expectations, though.
Thanks, Junio. This was a bit sloppy of me.
I really appreciate your regard for the small things!
Cheers, Beat
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Brandon Williams @ 2016-12-07 0:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, peff, jacob.keller
In-Reply-To: <xmqqtwagy65q.fsf@gitster.mtv.corp.google.com>
On 12/06, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > +/* removes the last path component from 'path' except if 'path' is root */
> > +static void strip_last_component(struct strbuf *path)
> > +{
> > + if (path->len > 1) {
> > + char *last_slash = find_last_dir_sep(path->buf);
> > + strbuf_setlen(path, last_slash - path->buf);
> > + }
> > +}
>
> You use find_last_dir_sep() which takes care of "Windows uses
> backslash" issue. Is this function expected to be fed something
> like "C:\My Files\foo.txt" and more importantly "C:\My Files"? Or
> is that handled by a lot higher level up in the callchain? I am
> reacting the comparison of path->len and 1 here.
This function should accept both absolute and relative paths, which
means it should probably accept "C:\My Files". I wasn't thinking about
windows 100% of the time while writing this so I'm hoping that a windows
expert will point things like this out to me :). So in reality this
check shouldn't be (path->len > 1) but rather some function is_only_root
which would check if the strbuf only contains a string which looks like
root.
>
> Also is the input expected to be normalized? Are we expected to be
> fed something like "/a//b/./c/../d/e" and react sensibly, or is that
> handled by a lot higher level up in the callchain?
Yes it should handle paths like that sensibly.
> > +/* gets the next component in 'remaining' and places it in 'next' */
> > +static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> > +{
> > + char *start = NULL;
> > + char *end = NULL;
> > +
> > + strbuf_reset(next);
> > +
> > + /* look for the next component */
> > + /* Skip sequences of multiple path-separators */
> > + for (start = remaining->buf; is_dir_sep(*start); start++)
> > + /* nothing */;
>
> Style:
> ; /* nothing */
k will fix.
>
> > + /* Find end of the path component */
> > + for (end = start; *end && !is_dir_sep(*end); end++)
> > + /* nothing */;
> > +
> > + strbuf_add(next, start, end - start);
>
> OK, so this was given "///foo/bar" in "remaining" and appended
> 'foo/' to "next". I.e. deduping of slashes is handled here.
>
> POSIX cares about treating "//" at the very beginning of the path
> specially. Is that supposed to be handled here, or by a lot higher
> level up in the callchain?
What exactly does "//" mean in this context? (I'm just naive in this
area)
>
> > + /* remove the component from 'remaining' */
> > + strbuf_remove(remaining, 0, end - remaining->buf);
> > +}
> > +
> > /* We allow "recursive" symbolic links. Only within reason, though. */
> > -#define MAXDEPTH 5
> > +#define MAXSYMLINKS 5
> >
> > /*
> > * Return the real path (i.e., absolute path, with symlinks resolved
> > @@ -21,7 +51,6 @@ int is_directory(const char *path)
> > * absolute_path().) The return value is a pointer to a static
> > * buffer.
> > *
> > * The directory part of path (i.e., everything up to the last
> > * dir_sep) must denote a valid, existing directory, but the last
> > * component need not exist. If die_on_error is set, then die with an
> > @@ -33,22 +62,16 @@ int is_directory(const char *path)
> > */
> > static const char *real_path_internal(const char *path, int die_on_error)
> > {
> > + static struct strbuf resolved = STRBUF_INIT;
>
> This being 'static' would probably mean that this is not reentrant,
> which goes against the title of the patch.
Yes I mentioned in the cover letter that this was the one last thing
that needed to be changed to make it reentrant. I wanted to get this
out for review sooner since this is the biggest change (getting rid of
the chdir() calls) and dropping the static here would just require
making the callers own the memory which should hopefully be an easy
refactor. It just would have taken a bit more time to actually do that
refactor now. I'm slowly working on it while this is being reviewed and
could be ready to send out when rerolling this patch. Sorry for having
a slightly misleading patch title :)
--
Brandon Williams
^ permalink raw reply
* Re: Merging .gitmodules files
From: Junio C Hamano @ 2016-12-06 23:54 UTC (permalink / raw)
To: David Turner; +Cc: git@vger.kernel.org
In-Reply-To: <8d266034ae5940efbec3eef372a6ce43@exmbdft7.ad.twosigma.com>
David Turner <David.Turner@twosigma.com> writes:
> I could set my .gitattributes for the .gitmodules file to use a
> custom merge driver. But: (a) I don't see an off-the-shelf one
> that does what I want ("union" happens to work in the add/add
> case, but not in the add/remove case or other cases) and (b) I
> would have to rewrite my whole history in order to have the
> .gitmodules file exist at every commit (or find a way to get
> .git/info/attributes into each of my users' clones) and (c) this
> should work correctly without customization; Git already treats
> the .gitmodules file as special for commands like "status";
> there's no reason it shouldn't do so for merge and rebase.
> ... if I did have time, do others agree that it would be
> reasonable to special-case this file? (Naturally, before doing
> the merge, we would check that the file was in fact parseable as a
> git config file; merging two changed gitmodules files of which
> either is unparseable would fall back to merging as text).
I do not see a fundamental reason why Git shouldn't know what
.gitmodules file is and how it should merge.
Our philosophy has always been "give users enough flexibility so
that they can experiment and come up with a solution that is general
enough (i.e. you can do it with custom merge driver), and then fold
it back into the core if it is an issue that is general enough and
it makes sense for the core to care about (i.e. my "why not?"
above). If you already have a custom merge driver, then you have
already cleared the first step ;-)
^ permalink raw reply
* Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C
From: Stephan Beyer @ 2016-12-06 23:54 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPPTnfOYsL8OB5x5n9cm2s+chMwg=xo459kVnFV9bB+5Hw@mail.gmail.com>
Hi Pranit,
On 12/06/2016 11:40 PM, Pranit Bauva wrote:
> On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
>> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>>> +static int bisect_state(struct bisect_terms *terms, const char **argv,
>>> + int argc)
>>> +{
>>> + const char *state = argv[0];
>>> +
>>> + get_terms(terms);
>>> + if (check_and_set_terms(terms, state))
>>> + return -1;
>>> +
>>> + if (!argc)
>>> + die(_("Please call `--bisect-state` with at least one argument"));
>>
>> I think this check should move to cmd_bisect__helper. There are also the
>> other argument number checks.
>
> Not really. After the whole conversion, the cmdmode will cease to
> exists while bisect_state will be called directly, thus it is
> important to check it here.
Okay, that's a point.
In that case, you should probably use "return error()" again and the
message (mentioning "--bisect-state") does not make sense when
--bisect-state ceases to exist.
>>> +
>>> + if (argc == 1 && one_of(state, terms->term_good,
>>> + terms->term_bad, "skip", NULL)) {
>>> + const char *bisected_head = xstrdup(bisect_head());
>>> + const char *hex[1];
>>
>> Maybe:
>> const char *hex;
>>
>>> + unsigned char sha1[20];
>>> +
>>> + if (get_sha1(bisected_head, sha1))
>>> + die(_("Bad rev input: %s"), bisected_head);
>>
>> And instead of...
>>
>>> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
>>> + return -1;
>>> +
>>> + *hex = xstrdup(sha1_to_hex(sha1));
>>> + if (check_expected_revs(hex, 1))
>>> + return -1;
>>
>> ... simply:
>>
>> hex = xstrdup(sha1_to_hex(sha1));
>> if (set_state(terms, state, hex)) {
>> free(hex);
>> return -1;
>> }
>> free(hex);
>>
>> where:
>
> Yes I am planning to convert all places with hex rather than the sha1
> but not yet, maybe in an another patch series because currently a lot
> of things revolve around sha1 and changing its behaviour wouldn't
> really be a part of a porting patch series.
I was not suggesting a change of behavior, I was suggesting a simple
helper function set_state() to get rid of code duplication above and
some lines below:
>> ... And replace this:
>>
>>> + const char **hex_string = (const char **) &hex.items[i].string;
>>> + if(bisect_write(state, *hex_string, terms, 0)) {
>>> + string_list_clear(&hex, 0);
>>> + return -1;
>>> + }
>>> + if (check_expected_revs(hex_string, 1)) {
>>> + string_list_clear(&hex, 0);
>>> + return -1;
>>> + }
>>
>> by:
>>
>> const char *hex_str = hex.items[i].string;
>> if (set_state(terms, state, hex_string)) {
>> string_list_clear(&hex, 0);
>> return -1;
>> }
See?
>>> @@ -184,8 +137,8 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
>>> state="$TERM_GOOD"
>>> fi
>>>
>>> - # We have to use a subshell because "bisect_state" can exit.
>>> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
>>> + # We have to use a subshell because "--bisect-state" can exit.
>>> + ( git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" )
>>
>> The new comment is funny, but you don't need a subshell here any
>> longer.
>
> True, but right now I didn't want to modify that part of the source
> code to remove the comment. I will remove the comment all together
> when I port bisect_run()
For most of the patches, I was commenting on the current state, not on
the big picture.
Still I think that it is better to remove the comment and the subshell
instead of making the comment weird ("yes the builtin can exit, but why
do we need a subshell?" vs "yes the shell function calls exit, so we
need a subshell because we do not want to exit this shell script")
~Stephan
^ permalink raw reply
* What's cooking in git.git (Dec 2016, #01; Tue, 6)
From: Junio C Hamano @ 2016-12-06 23:46 UTC (permalink / raw)
To: git
Here are the topics that have been cooking. Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'. The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.
The 'next' branch has been rewound, 'maint' now is for maintenance
fixes post v2.11 release.
The description of many new topics in this issue of the report is
probably sketchier than it should be. Refinements are very much
appreciated.
You can find the changes described here in the integration branches
of the repositories listed at
http://git-blame.blogspot.com/p/git-public-repositories.html
--------------------------------------------------
[Graduated to "master"]
* jk/common-main (2016-11-29) 1 commit
(merged to 'next' on 2016-11-29 at 2985e7efba)
+ common-main: stop munging argv[0] path
Fix for a small regression in a topic already in 'master'.
--------------------------------------------------
[New Topics]
* ak/lazy-prereq-mktemp (2016-11-29) 1 commit
- t7610: clean up foo.XXXXXX tmpdir
Test code clean-up.
Will merge to 'next'.
* da/mergetool-trust-exit-code (2016-11-29) 2 commits
- mergetools/vimdiff: trust Vim's exit code
- mergetool: honor mergetool.$tool.trustExitCode for built-in tools
mergetool.<tool>.trustExitCode configuration variable did not apply
to built-in tools, but now it does.
Will merge to 'next'.
* jb/diff-no-index-no-abbrev (2016-11-29) 1 commit
- diff: handle --no-abbrev outside of repository
"git diff --no-index" did not take "--no-abbrev" option.
Will merge to 'next'.
* vk/p4-submit-shelve (2016-11-29) 1 commit
- git-p4: allow submit to create shelved changelists.
(this branch is used by ld/p4-update-shelve.)
Will merge to 'next'.
* jk/http-walker-limit-redirect-2.9 (2016-12-06) 5 commits
- http: treat http-alternates like redirects
- http: make redirects more obvious
- remote-curl: rename shadowed options variable
- http: always update the base URL for redirects
- http: simplify update_url_from_redirect
(this branch is used by jk/http-walker-limit-redirect.)
Transport with dumb http can be fooled into following foreign URLs
that the end user does not intend to, especially with the server
side redirects and http-alternates mechanism, which can lead to
security issues. Tighten the redirection and make it more obvious
to the end user when it happens.
Will merge to 'next'.
* jk/http-walker-limit-redirect (2016-12-06) 2 commits
- http-walker: complain about non-404 loose object errors
- Merge branch 'ew/http-walker' into jk/http-walker-limit-redirect
(this branch uses jk/http-walker-limit-redirect-2.9.)
Update the error messages from the dumb-http client when it fails
to obtain loose objects; we used to give sensible error message
only upon 404 but we now forbid unexpected redirects that needs to
be reported with something sensible.
Will merge to 'next'.
* ah/grammos (2016-12-05) 3 commits
- clone,fetch: explain the shallow-clone option a little more clearly
- receive-pack: improve English grammar of denyCurrentBranch message
- bisect: improve English grammar of not-ancestors message
A few messages have been fixed for their grammatical errors.
Will merge to 'next'.
* ak/commit-only-allow-empty (2016-12-05) 1 commit
- commit: make --only --allow-empty work without paths
"git commit --allow-empty --only" (no pathspec) with dirty index
ought to be an acceptable way to create a new commit that does not
change any paths, but it was forbidden (perhaps because nobody
needed it).
Will merge to 'next'.
* bb/unicode-9.0 (2016-12-05) 3 commits
- unicode_width.h: update the tables to Unicode 9.0
- update_unicode.sh: strip the plane offsets from the double_width[] table
- update_unicode.sh: automatically download newer definition files
The character width table has been updated to match Unicode 9.0
Will merge to 'next'.
* ld/p4-update-shelve (2016-12-05) 1 commit
- git-p4: support updating an existing shelved changelist
(this branch uses vk/p4-submit-shelve.)
Will merge to 'next'.
* ld/p4-worktree (2016-12-05) 1 commit
- git-p4: support secondary working trees managed by "git worktree"
Iffy.
cf. <20161202224319.5385-2-luke@diamand.org>
* ls/p4-empty-file-on-lfs (2016-12-05) 1 commit
- git-p4: fix empty file processing for large file system backend GitLFS
"git p4" LFS support was broken when LFS stores an empty blob.
Will merge to 'next'.
* ls/p4-retry-thrice (2016-12-05) 1 commit
- git-p4: add config to retry p4 commands; retry 3 times by default
Will merge to 'next'.
* ls/t0021-fixup (2016-12-05) 1 commit
- t0021: minor filter process test cleanup
Will merge to 'next'.
* ls/travis-update-p4-and-lfs (2016-12-05) 1 commit
- travis-ci: update P4 to 16.2 and GitLFS to 1.5.2 in Linux build
The default Travis-CI configuration specifies newer P4 and GitLFS.
Will merge to 'next'.
* sb/t3600-cleanup (2016-12-05) 1 commit
- t3600: remove useless redirect
Code cleanup.
Will merge to 'next'.
* sb/unpack-trees-grammofix (2016-12-05) 1 commit
- unpack-trees: fix grammar for untracked files in directories
Will merge to 'next'.
* da/difftool-dir-diff-fix (2016-12-06) 1 commit
- difftool: fix dir-diff index creation when in a subdirectory
"git difftool --dir-diff" had a minor regression when started from
a subdirectory, which has been fixed.
Will merge to 'next'.
* jc/lockfile-silent-on-error (2016-12-06) 1 commit
- lockfile: LOCK_SILENT_ON_ERROR
Recent change to use "hold_lock*()" without die-on-error in the
merge machinery made an error from "git merge" harder to spot.
Introduce an opt-in "be silent because it has been verified that
this particular caller produces a sensible error message itself"
flag, and make everybody else show the error message when it
returns to the caller with -1 without dying.
* jk/stash-disable-renames-internally (2016-12-06) 1 commit
- stash: prefer plumbing over git-diff
When diff.renames configuration is on (and with Git 2.9 and later,
it is enabled by default, which made it worse), "git stash"
misbehaved if a file is removed and another file with a very
similar content is added.
Will merge to 'next'.
* jk/xdiff-drop-xdl-fast-hash (2016-12-06) 1 commit
- xdiff: drop XDL_FAST_HASH
Retire the "fast hash" that had disastrous performance issues in
some corner cases.
Will cook in 'next'.
* ls/filter-process (2016-12-06) 1 commit
- docs: warn about possible '=' in clean/smudge filter process values
Doc update.
Will merge to 'next'.
* nd/for-each-ref-ignore-case (2016-12-05) 1 commit
- tag, branch, for-each-ref: add --ignore-case for sorting and filtering
"git branch --list" and friends learned "--ignore-case" option to
optionally sort branches and tags case insensitively.
Will merge to 'next'.
* rj/git-version-gen-do-not-force-abbrev (2016-12-06) 1 commit
- GIT-VERSION-GEN: do not force abbreviation length used by 'describe'
A minor build update.
Will merge to 'next'.
--------------------------------------------------
[Stalled]
* jc/retire-compaction-heuristics (2016-11-02) 3 commits
- SQUASH???
- SQUASH???
- diff: retire the original experimental "compaction" heuristics
Waiting for a reroll.
* jc/abbrev-autoscale-config (2016-11-01) 1 commit
- config.abbrev: document the new default that auto-scales
Waiting for a reroll.
* jk/nofollow-attr-ignore (2016-11-02) 5 commits
- exclude: do not respect symlinks for in-tree .gitignore
- attr: do not respect symlinks for in-tree .gitattributes
- exclude: convert "check_index" into a flags field
- attr: convert "macro_ok" into a flags field
- add open_nofollow() helper
As we do not follow symbolic links when reading control files like
.gitignore and .gitattributes from the index, match the behaviour
and not follow symbolic links when reading them from the working
tree. This also tightens security a bit by not leaking contents of
an unrelated file in the error messages when it is pointed at by
one of these files that is a symbolic link.
Perhaps we want to cover .gitmodules too with the same mechanism?
* nd/worktree-move (2016-11-28) 11 commits
. worktree remove: new command
. worktree move: refuse to move worktrees with submodules
. worktree move: accept destination as directory
. worktree move: new command
. worktree.c: add update_worktree_location()
. worktree.c: add validate_worktree()
. copy.c: convert copy_file() to copy_dir_recursively()
. copy.c: style fix
. copy.c: convert bb_(p)error_msg to error(_errno)
. copy.c: delete unused code in copy_file()
. copy.c: import copy_file() from busybox
(this branch uses nd/worktree-list-fixup; is tangled with sb/submodule-embed-gitdir.)
"git worktree" learned move and remove subcommands.
Reported to break builds on Windows.
* jc/bundle (2016-03-03) 6 commits
- index-pack: --clone-bundle option
- Merge branch 'jc/index-pack' into jc/bundle
- bundle v3: the beginning
- bundle: keep a copy of bundle file name in the in-core bundle header
- bundle: plug resource leak
- bundle doc: 'verify' is not about verifying the bundle
The beginning of "split bundle", which could be one of the
ingredients to allow "git clone" traffic off of the core server
network to CDN.
While I think it would make it easier for people to experiment and
build on if the topic is merged to 'next', I am at the same time a
bit reluctant to merge an unproven new topic that introduces a new
file format, which we may end up having to support til the end of
time. It is likely that to support a "prime clone from CDN", it
would need a lot more than just "these are the heads and the pack
data is over there", so this may not be sufficient.
Will discard.
* mh/connect (2016-06-06) 10 commits
- connect: [host:port] is legacy for ssh
- connect: move ssh command line preparation to a separate function
- connect: actively reject git:// urls with a user part
- connect: change the --diag-url output to separate user and host
- connect: make parse_connect_url() return the user part of the url as a separate value
- connect: group CONNECT_DIAG_URL handling code
- connect: make parse_connect_url() return separated host and port
- connect: re-derive a host:port string from the separate host and port variables
- connect: call get_host_and_port() earlier
- connect: document why we sometimes call get_port after get_host_and_port
Rewrite Git-URL parsing routine (hopefully) without changing any
behaviour.
It has been two months without any support. We may want to discard
this.
* ec/annotate-deleted (2015-11-20) 1 commit
- annotate: skip checking working tree if a revision is provided
Usability fix for annotate-specific "<file> <rev>" syntax with deleted
files.
Has been waiting for a review for too long without seeing anything.
Will discard.
* dk/gc-more-wo-pack (2016-01-13) 4 commits
- gc: clean garbage .bitmap files from pack dir
- t5304: ensure non-garbage files are not deleted
- t5304: test .bitmap garbage files
- prepare_packed_git(): find more garbage
Follow-on to dk/gc-idx-wo-pack topic, to clean up stale
.bitmap and .keep files.
Has been waiting for a reroll for too long.
cf. <xmqq60ypbeng.fsf@gitster.mtv.corp.google.com>
Will discard.
* jc/diff-b-m (2015-02-23) 5 commits
. WIPWIP
. WIP: diff-b-m
- diffcore-rename: allow easier debugging
- diffcore-rename.c: add locate_rename_src()
- diffcore-break: allow debugging
"git diff -B -M" produced incorrect patch when the postimage of a
completely rewritten file is similar to the preimage of a removed
file; such a resulting file must not be expressed as a rename from
other place.
The fix in this patch is broken, unfortunately.
Will discard.
--------------------------------------------------
[Cooking]
* jc/renormalize-merge-kill-safer-crlf (2016-12-01) 4 commits
- convert: git cherry-pick -Xrenormalize did not work
- Merge branch 'tb/t0027-raciness-fix' into jc/renormalize-merge-kill-safer-crlf
- merge-recursive: handle NULL in add_cacheinfo() correctly
- cherry-pick: demonstrate a segmentation fault
Fix a corner case in merge-recursive regression that crept in
during 2.10 development cycle.
Will merge to 'next'.
* js/difftool-builtin (2016-11-28) 2 commits
- difftool: implement the functionality in the builtin
- difftool: add a skeleton for the upcoming builtin
Rewrite a scripted porcelain "git difftool" in C.
Under discussion.
* nd/qsort-in-merge-recursive (2016-11-28) 1 commit
- merge-recursive.c: use string_list_sort instead of qsort
Code simplification.
Will merge to 'next'.
* bw/push-dry-run (2016-11-23) 2 commits
- push: fix --dry-run to not push submodules
- push: --dry-run updates submodules when --recurse-submodules=on-demand
(this branch uses hv/submodule-not-yet-pushed-fix; is tangled with sb/push-make-submodule-check-the-default.)
"git push --dry-run --recurse-submodule=on-demand" wasn't
"--dry-run" in the submodules.
Will merge to 'next'.
* sb/push-make-submodule-check-the-default (2016-11-29) 2 commits
- push: change submodule default to check when submodules exist
- submodule add: extend force flag to add existing repos
(this branch uses hv/submodule-not-yet-pushed-fix; is tangled with bw/push-dry-run.)
Turn the default of "push.recurseSubmodules" to "check" when
submodules seem to be in use.
Will merge to 'next'.
* jk/rev-parse-symbolic-parents-fix (2016-11-16) 1 commit
- rev-parse: fix parent shorthands with --symbolic
"git rev-parse --symbolic" failed with a more recent notation like
"HEAD^-1" and "HEAD^!".
Will merge to 'next'.
* nd/worktree-list-fixup (2016-11-28) 5 commits
- worktree list: keep the list sorted
- worktree.c: get_worktrees() takes a new flag argument
- get_worktrees() must return main worktree as first item even on error
- worktree: reorder an if statement
- worktree.c: zero new 'struct worktree' on allocation
(this branch is used by nd/worktree-move and sb/submodule-embed-gitdir.)
The output from "git worktree list" was made in readdir() order,
and was unstable.
Will merge to 'next'.
* jk/trailers-placeholder-in-pretty (2016-11-21) 2 commits
- ref-filter: add support to display trailers as part of contents
- pretty: add %(trailers) format for displaying trailers of a commit message
In addition to %(subject), %(body), "log --pretty=format:..."
learned a new placeholder %(trailers).
Will merge to 'next'.
* sb/submodule-embed-gitdir (2016-12-05) 5 commits
- submodule: add embed-git-dir function
- worktree: get worktrees from submodules
- test-lib-functions.sh: teach test_commit -C <dir>
- submodule helper: support super prefix
- submodule: use absolute path for computing relative path connecting
(this branch uses nd/worktree-list-fixup; is tangled with nd/worktree-move.)
A new submodule helper "git submodule embedgitdirs" to make it
easier to move embedded .git/ directory for submodules in a
superproject to .git/modules/ (and point the latter with the former
that is turned into a "gitdir:" file) has been added.
Waiting for review.
* dt/empty-submodule-in-merge (2016-11-17) 1 commit
- submodules: allow empty working-tree dirs in merge/cherry-pick
An empty directory in a working tree that can simply be nuked used
to interfere while merging or cherry-picking a change to create a
submodule directory there, which has been fixed..
Will merge to 'next'.
* bw/grep-recurse-submodules (2016-11-22) 6 commits
- grep: search history of moved submodules
- grep: enable recurse-submodules to work on <tree> objects
- grep: optionally recurse into submodules
- grep: add submodules as a grep source type
- submodules: load gitmodules file from commit sha1
- submodules: add helper functions to determine presence of submodules
"git grep" learns to optionally recurse into submodules
Has anybody else seen t7814 being flakey with this series?
* dt/smart-http-detect-server-going-away (2016-11-18) 2 commits
(merged to 'next' on 2016-12-05 at 3ea70d01af)
+ upload-pack: optionally allow fetching any sha1
+ remote-curl: don't hang when a server dies before any output
Originally merged to 'next' on 2016-11-21
When the http server gives an incomplete response to a smart-http
rpc call, it could lead to client waiting for a full response that
will never come. Teach the client side to notice this condition
and abort the transfer.
An improvement counterproposal has failed.
cf. <20161114194049.mktpsvgdhex2f4zv@sigill.intra.peff.net>
Will cook in 'next'.
* mm/push-social-engineering-attack-doc (2016-11-14) 1 commit
(merged to 'next' on 2016-12-05 at 9a2b5bd1a9)
+ doc: mention transfer data leaks in more places
Originally merged to 'next' on 2016-11-16
Doc update on fetching and pushing.
Will cook in 'next'.
* jc/compression-config (2016-11-15) 1 commit
(merged to 'next' on 2016-12-05 at 323769ca07)
+ compression: unify pack.compression configuration parsing
Originally merged to 'next' on 2016-11-23
Compression setting for producing packfiles were spread across
three codepaths, one of which did not honor any configuration.
Unify these so that all of them honor core.compression and
pack.compression variables the same way.
Will cook in 'next'.
* mm/gc-safety-doc (2016-11-16) 1 commit
(merged to 'next' on 2016-12-05 at 031ecc1886)
+ git-gc.txt: expand discussion of races with other processes
Originally merged to 'next' on 2016-11-17
Doc update.
Will cook in 'next'.
* hv/submodule-not-yet-pushed-fix (2016-11-16) 4 commits
(merged to 'next' on 2016-12-05 at c9d729fca2)
+ submodule_needs_pushing(): explain the behaviour when we cannot answer
+ batch check whether submodule needs pushing into one call
+ serialize collection of refs that contain submodule changes
+ serialize collection of changed submodules
(this branch is used by bw/push-dry-run and sb/push-make-submodule-check-the-default.)
Originally merged to 'next' on 2016-11-21
The code in "git push" to compute if any commit being pushed in the
superproject binds a commit in a submodule that hasn't been pushed
out was overly inefficient, making it unusable even for a small
project that does not have any submodule but have a reasonable
number of refs.
Will cook in 'next'.
* kn/ref-filter-branch-list (2016-11-15) 18 commits
- for-each-ref: do not segv with %(HEAD) on an unborn branch
- branch: implement '--format' option
- branch: use ref-filter printing APIs
- branch, tag: use porcelain output
- ref-filter: allow porcelain to translate messages in the output
- ref-filter: add `:dir` and `:base` options for ref printing atoms
- ref-filter: make remote_ref_atom_parser() use refname_atom_parser_internal()
- ref-filter: introduce symref_atom_parser() and refname_atom_parser()
- ref-filter: introduce refname_atom_parser_internal()
- ref-filter: make "%(symref)" atom work with the ':short' modifier
- ref-filter: add support for %(upstream:track,nobracket)
- ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
- ref-filter: introduce format_ref_array_item()
- ref-filter: move get_head_description() from branch.c
- ref-filter: modify "%(objectname:short)" to take length
- ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
- ref-filter: include reference to 'used_atom' within 'atom_value'
- ref-filter: implement %(if), %(then), and %(else) atoms
The code to list branches in "git branch" has been consolidated
with the more generic ref-filter API.
Rerolled, reviewed, looking good.
Expecting a reroll.
cf. <20161108201211.25213-1-Karthik.188@gmail.com>
cf. <CAOLa=ZQqe3vEj_428d41vd_4kfjzsm87Wam6Zm2dhXWkPdJ8Rw@mail.gmail.com>
cf. <xmqq7f84tqa7.fsf_-_@gitster.mtv.corp.google.com>
* bw/transport-protocol-policy (2016-12-05) 5 commits
- transport: add from_user parameter to is_transport_allowed
- http: create function to get curl allowed protocols
- http: always warn if libcurl version is too old
- transport: add protocol policy config option
- lib-proto-disable: variable name fix
Finer-grained control of what protocols are allowed for transports
during clone/fetch/push have been enabled via a new configuration
mechanism.
* jt/fetch-no-redundant-tag-fetch-map (2016-11-11) 1 commit
(merged to 'next' on 2016-12-05 at 432f9469a7)
+ fetch: do not redundantly calculate tag refmap
Originally merged to 'next' on 2016-11-16
Code cleanup to avoid using redundant refspecs while fetching with
the --tags option.
Will cook in 'next'.
* sb/submodule-config-cleanup (2016-11-22) 3 commits
(merged to 'next' on 2016-12-05 at 658b8764bf)
+ submodule-config: clarify parsing of null_sha1 element
+ submodule-config: rename commit_sha1 to treeish_name
+ submodule config: inline config_from_{name, path}
Originally merged to 'next' on 2016-11-23
Minor code clean-up.
Will cook in 'next'.
* jc/push-default-explicit (2016-10-31) 2 commits
(merged to 'next' on 2016-12-05 at d63f3777af)
+ push: test pushing ambiguously named branches
+ push: do not use potentially ambiguous default refspec
Originally merged to 'next' on 2016-11-01
A lazy "git push" without refspec did not internally use a fully
specified refspec to perform 'current', 'simple', or 'upstream'
push, causing unnecessary "ambiguous ref" errors.
Will cook in 'next'.
* jt/use-trailer-api-in-commands (2016-11-29) 5 commits
- sequencer: use trailer's trailer layout
- trailer: have function to describe trailer layout
- trailer: avoid unnecessary splitting on lines
- commit: make ignore_non_trailer take buf/len
- trailer: be stricter in parsing separators
Commands that operate on a log message and add lines to the trailer
blocks, such as "format-patch -s", "cherry-pick (-x|-s)", and
"commit -s", have been taught to use the logic of and share the
code with "git interpret-trailer".
Will merge to 'next'.
* nd/rebase-forget (2016-11-28) 1 commit
- rebase: add --forget to cleanup rebase, leave everything else untouched
"git rebase" learned "--forget" option, which allows a user to
remove the metadata left by an earlier "git rebase" that was
manually aborted without using "git rebase --abort".
Will merge to 'next'.
* jc/git-open-cloexec (2016-11-02) 3 commits
- sha1_file: stop opening files with O_NOATIME
- git_open_cloexec(): use fcntl(2) w/ FD_CLOEXEC fallback
- git_open(): untangle possible NOATIME and CLOEXEC interactions
The codeflow of setting NOATIME and CLOEXEC on file descriptors Git
opens has been simplified.
We may want to drop the tip one.
* jk/no-looking-at-dotgit-outside-repo-final (2016-10-26) 1 commit
(merged to 'next' on 2016-12-05 at 0c77e39cd5)
+ setup_git_env: avoid blind fall-back to ".git"
Originally merged to 'next' on 2016-10-26
This is the endgame of the topic to avoid blindly falling back to
".git" when the setup sequence said we are _not_ in Git repository.
A corner case that happens to work right now may be broken by a
call to die("BUG").
Will cook in 'next'.
* jc/reset-unmerge (2016-10-24) 1 commit
- reset: --unmerge
After "git add" is run prematurely during a conflict resolution,
"git diff" can no longer be used as a way to sanity check by
looking at the combined diff. "git reset" learned a new
"--unmerge" option to recover from this situation.
Will discard.
This may not be needed, given that update-index has a similar
option.
* jc/merge-base-fp-only (2016-10-19) 8 commits
. merge-base: fp experiment
- merge: allow to use only the fp-only merge bases
- merge-base: limit the output to bases that are on first-parent chain
- merge-base: mark bases that are on first-parent chain
- merge-base: expose get_merge_bases_many_0() a bit more
- merge-base: stop moving commits around in remove_redundant()
- sha1_name: remove ONELINE_SEEN bit
- commit: simplify fastpath of merge-base
An experiment of merge-base that ignores common ancestors that are
not on the first parent chain.
Will discard.
The whole premise feels wrong.
* tb/convert-stream-check (2016-10-27) 2 commits
- convert.c: stream and fast search for binary
- read-cache: factor out get_sha1_from_index() helper
End-of-line conversion sometimes needs to see if the current blob
in the index has NULs and CRs to base its decision. We used to
always get a full statistics over the blob, but in many cases we
can return early when we have seen "enough" (e.g. if we see a
single NUL, the blob will be handled as binary). The codepaths
have been optimized by using streaming interface.
Will discard.
Retracted.
cf. <20161102071646.GA5094@tb-raspi>
* pb/bisect (2016-10-18) 27 commits
- bisect--helper: remove the dequote in bisect_start()
- bisect--helper: retire `--bisect-auto-next` subcommand
- bisect--helper: retire `--bisect-autostart` subcommand
- bisect--helper: retire `--bisect-write` subcommand
- bisect--helper: `bisect_replay` shell function in C
- bisect--helper: `bisect_log` shell function in C
- bisect--helper: retire `--write-terms` subcommand
- bisect--helper: retire `--check-expected-revs` subcommand
- bisect--helper: `bisect_state` & `bisect_head` shell function in C
- bisect--helper: `bisect_autostart` shell function in C
- bisect--helper: retire `--next-all` subcommand
- bisect--helper: retire `--bisect-clean-state` subcommand
- bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
- t6030: no cleanup with bad merge base
- bisect--helper: `bisect_start` shell function partially in C
- bisect--helper: `get_terms` & `bisect_terms` shell function in C
- bisect--helper: `bisect_next_check` & bisect_voc shell function in C
- bisect--helper: `check_and_set_terms` shell function in C
- bisect--helper: `bisect_write` shell function in C
- bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
- bisect--helper: `bisect_reset` shell function in C
- wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()
- t6030: explicitly test for bisection cleanup
- bisect--helper: `bisect_clean_state` shell function in C
- bisect--helper: `write_terms` shell function in C
- bisect: rewrite `check_term_format` shell function in C
- bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Move more parts of "git bisect" to C.
Waiting for review.
* st/verify-tag (2016-10-10) 7 commits
- t/t7004-tag: Add --format specifier tests
- t/t7030-verify-tag: Add --format specifier tests
- builtin/tag: add --format argument for tag -v
- builtin/verify-tag: add --format to verify-tag
- tag: add format specifier to gpg_verify_tag
- ref-filter: add function to print single ref_array_item
- gpg-interface, tag: add GPG_VERIFY_QUIET flag
"git tag" and "git verify-tag" learned to put GPG verification
status in their "--format=<placeholders>" output format.
Waiting for a reroll.
cf. <20161007210721.20437-1-santiago@nyu.edu>
* sb/attr (2016-11-11) 35 commits
- completion: clone can initialize specific submodules
- clone: add --init-submodule=<pathspec> switch
- submodule update: add `--init-default-path` switch
- pathspec: allow escaped query values
- pathspec: allow querying for attributes
- pathspec: move prefix check out of the inner loop
- pathspec: move long magic parsing out of prefix_pathspec
- Documentation: fix a typo
- attr: keep attr stack for each check
- attr: convert to new threadsafe API
- attr: make git_check_attr_counted static
- attr.c: outline the future plans by heavily commenting
- attr.c: always pass check[] to collect_some_attrs()
- attr.c: introduce empty_attr_check_elems()
- attr.c: correct ugly hack for git_all_attrs()
- attr.c: rename a local variable check
- attr.c: pass struct git_attr_check down the callchain
- attr.c: add push_stack() helper
- attr: support quoting pathname patterns in C style
- attr: expose validity check for attribute names
- attr: add counted string version of git_check_attr()
- attr: retire git_check_attrs() API
- attr: convert git_check_attrs() callers to use the new API
- attr: convert git_all_attrs() to use "struct git_attr_check"
- attr: (re)introduce git_check_attr() and struct git_attr_check
- attr: rename function and struct related to checking attributes
- attr.c: plug small leak in parse_attr_line()
- attr.c: tighten constness around "git_attr" structure
- attr.c: simplify macroexpand_one()
- attr.c: mark where #if DEBUG ends more clearly
- attr.c: complete a sentence in a comment
- attr.c: explain the lack of attr-name syntax check in parse_attr()
- attr.c: update a stale comment on "struct match_attr"
- attr.c: use strchrnul() to scan for one line
- commit.c: use strchrnul() to scan for one line
The attributes API has been updated so that it can later be
optimized using the knowledge of which attributes are queried.
Building on top of the updated API, the pathspec machinery learned
to select only paths with given attributes set.
Waiting for review.
* va/i18n-perl-scripts (2016-11-11) 16 commits
- i18n: difftool: mark warnings for translation
- i18n: send-email: mark composing message for translation
- i18n: send-email: mark string with interpolation for translation
- i18n: send-email: mark warnings and errors for translation
- i18n: send-email: mark strings for translation
- i18n: add--interactive: mark status words for translation
- i18n: add--interactive: remove %patch_modes entries
- i18n: add--interactive: mark edit_hunk_manually message for translation
- i18n: add--interactive: i18n of help_patch_cmd
- i18n: add--interactive: mark patch prompt for translation
- i18n: add--interactive: mark plural strings
- i18n: clean.c: match string with git-add--interactive.perl
- i18n: add--interactive: mark strings with interpolation for translation
- i18n: add--interactive: mark simple here-documents for translation
- i18n: add--interactive: mark strings for translation
- Git.pm: add subroutines for commenting lines
Porcelain scripts written in Perl are getting internationalized.
Waiting for review.
* jc/latin-1 (2016-09-26) 2 commits
(merged to 'next' on 2016-12-05 at fb549caa12)
+ utf8: accept "latin-1" as ISO-8859-1
+ utf8: refactor code to decide fallback encoding
Originally merged to 'next' on 2016-09-28
Some platforms no longer understand "latin-1" that is still seen in
the wild in e-mail headers; replace them with "iso-8859-1" that is
more widely known when conversion fails from/to it.
Will cook in 'next'.
* sg/fix-versioncmp-with-common-suffix (2016-09-08) 5 commits
- versioncmp: cope with common leading parts in versionsort.prereleaseSuffix
- versioncmp: pass full tagnames to swap_prereleases()
- t7004-tag: add version sort tests to show prerelease reordering issues
- t7004-tag: use test_config helper
- t7004-tag: delete unnecessary tags with test_when_finished
The prereleaseSuffix feature of version comparison that is used in
"git tag -l" did not correctly when two or more prereleases for the
same release were present (e.g. when 2.0, 2.0-beta1, and 2.0-beta2
are there and the code needs to compare 2.0-beta1 and 2.0-beta2).
Waiting for a reroll.
cf. <20160908223727.Horde.jVOOJ278ssZ3qkyjkmyqZD-@webmail.informatik.kit.edu>
* jc/pull-rebase-ff (2016-11-29) 1 commit
- pull: fast-forward "pull --rebase=true"
"git pull --rebase", when there is no new commits on our side since
we forked from the upstream, should be able to fast-forward without
invoking "git rebase", but it didn't.
* jc/merge-drop-old-syntax (2015-04-29) 1 commit
(merged to 'next' on 2016-12-05 at 041946dae0)
+ merge: drop 'git merge <message> HEAD <commit>' syntax
Originally merged to 'next' on 2016-10-11
Stop supporting "git merge <message> HEAD <commit>" syntax that has
been deprecated since October 2007, and issues a deprecation
warning message since v2.5.0.
Will cook in 'next'.
^ permalink raw reply
* Merging .gitmodules files
From: David Turner @ 2016-12-06 23:48 UTC (permalink / raw)
To: git@vger.kernel.org
Consider two commits: one adds file A, and the other adds file B. These commits don't conflict; you can merge them with no problem.
But if the two commits instead add submodules A and B, and you try to merge, you'll likely get a conflict in .gitmodules. This seems wrong; .gitmodules happens to be a plain text file as an implementation detail, but in terms of interpretation, it is more like a map of maps (name1 -> {path -> "...", url -> "..."}, name2 -> ...).
We (Two Sigma) keep our .gitmodules file in alphabetical order (so we don't use git submodule add -- our .gitmodules file is instead generated by some more complicated offline process). But even for ordinary .gitmodules files, order is not important so long as it's consistent.
I could set my .gitattributes for the .gitmodules file to use a custom merge driver. But: (a) I don't see an off-the-shelf one that does what I want ("union" happens to work in the add/add case, but not in the add/remove case or other cases) and (b) I would have to rewrite my whole history in order to have the .gitmodules file exist at every commit (or find a way to get .git/info/attributes into each of my users' clones) and (c) this should work correctly without customization; Git already treats the .gitmodules file as special for commands like "status"; there's no reason it shouldn't do so for merge and rebase.
I'm not sure I'll necessarily have time to implement this -- for our use case ( http://github.com/twosigma/git-meta ), we might be able to get away with doing it in JS, and using something like https://github.com/mirkokiefer/diff-merge-patch#sets . But if I did have time, do others agree that it would be reasonable to special-case this file? (Naturally, before doing the merge, we would check that the file was in fact parseable as a git config file; merging two changed gitmodules files of which either is unparseable would fall back to merging as text).
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Junio C Hamano @ 2016-12-06 23:44 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, peff, jacob.keller
In-Reply-To: <1480964316-99305-2-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> +/* removes the last path component from 'path' except if 'path' is root */
> +static void strip_last_component(struct strbuf *path)
> +{
> + if (path->len > 1) {
> + char *last_slash = find_last_dir_sep(path->buf);
> + strbuf_setlen(path, last_slash - path->buf);
> + }
> +}
You use find_last_dir_sep() which takes care of "Windows uses
backslash" issue. Is this function expected to be fed something
like "C:\My Files\foo.txt" and more importantly "C:\My Files"? Or
is that handled by a lot higher level up in the callchain? I am
reacting the comparison of path->len and 1 here.
Also is the input expected to be normalized? Are we expected to be
fed something like "/a//b/./c/../d/e" and react sensibly, or is that
handled by a lot higher level up in the callchain?
> +/* gets the next component in 'remaining' and places it in 'next' */
> +static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> +{
> + char *start = NULL;
> + char *end = NULL;
> +
> + strbuf_reset(next);
> +
> + /* look for the next component */
> + /* Skip sequences of multiple path-separators */
> + for (start = remaining->buf; is_dir_sep(*start); start++)
> + /* nothing */;
Style:
; /* nothing */
> + /* Find end of the path component */
> + for (end = start; *end && !is_dir_sep(*end); end++)
> + /* nothing */;
> +
> + strbuf_add(next, start, end - start);
OK, so this was given "///foo/bar" in "remaining" and appended
'foo/' to "next". I.e. deduping of slashes is handled here.
POSIX cares about treating "//" at the very beginning of the path
specially. Is that supposed to be handled here, or by a lot higher
level up in the callchain?
> + /* remove the component from 'remaining' */
> + strbuf_remove(remaining, 0, end - remaining->buf);
> +}
> +
> /* We allow "recursive" symbolic links. Only within reason, though. */
> -#define MAXDEPTH 5
> +#define MAXSYMLINKS 5
>
> /*
> * Return the real path (i.e., absolute path, with symlinks resolved
> @@ -21,7 +51,6 @@ int is_directory(const char *path)
> * absolute_path().) The return value is a pointer to a static
> * buffer.
> *
> * The directory part of path (i.e., everything up to the last
> * dir_sep) must denote a valid, existing directory, but the last
> * component need not exist. If die_on_error is set, then die with an
> @@ -33,22 +62,16 @@ int is_directory(const char *path)
> */
> static const char *real_path_internal(const char *path, int die_on_error)
> {
> + static struct strbuf resolved = STRBUF_INIT;
This being 'static' would probably mean that this is not reentrant,
which goes against the title of the patch.
^ permalink raw reply
* Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C
From: Stephan Beyer @ 2016-12-06 23:40 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List, Christian Couder, Lars Schneider
In-Reply-To: <CAFZEwPNmB7rYvUTPy6dvfqfbUsjDeEcteLBBH5Wk-G_suE+YTw@mail.gmail.com>
Hi Pranit and Christian and Lars ;)
On 12/07/2016 12:02 AM, Pranit Bauva wrote:
> On Tue, Nov 22, 2016 at 6:19 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
>> Okay Pranit,
>>
>> this is the last patch for me to review in this series.
>>
>> Now that I have a coarse overview of what you did, I have the general
>> remark that imho the "terms" variable should simply be global instead of
>> being passed around all the time.
>
> In a personal conversation with my mentors, we thought it is the best
> fit to keep it in a struct and pass it around so that it is easier in
> libification.
I guess you had that conversation at the beginning of the project and I
think that at that stage I would have recommended it that way, too.
However, looking at the v15 bisect--helper.c, it looks like it is rather
a pain to do it that way. I mean, you use the terms like they were
global variables: you initialize them in cmd_bisect__helper() and then
you always pass them around; you update them "globally", never using
restricted scope, never ever use a second instantiation besides the one
of cmd_bisect__helper(). These are all signs that *in the current code*
using (static) global variables is the better way (making the code simpler).
The libification argument may be worth considering, though. I am not
sure if there is really a use-case where you need two different
instantiations of the terms, *except* if the lib allows bisecting with
different terms at the same time (in different repos, of course). Is the
lib "aware" of such use-cases like doing stuff in different repos at the
same time? If that's the case, not using global variables is the right
thing to do. In any other case I can imagine, I'd suggest to use global
variables.
~Stephan
^ permalink raw reply
* Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C
From: Stephan Beyer @ 2016-12-06 23:20 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPNmB7rYvUTPy6dvfqfbUsjDeEcteLBBH5Wk-G_suE+YTw@mail.gmail.com>
Hey Pranit,
On 12/07/2016 12:02 AM, Pranit Bauva wrote:
>>> +static int bisect_replay(struct bisect_terms *terms, const char *filename)
>>> +{
>>> + struct strbuf line = STRBUF_INIT;
>>> + struct strbuf word = STRBUF_INIT;
>>> + FILE *fp = NULL;
>>
>> (The initialization is not necessary here.)
>
> Um. I think it is. Otherwise if it goes to the finish block before you
> try to operate on fp, it will cause a seg fault.
You are right, thanks!
>>> + while (strbuf_getline(&line, fp) != EOF) {
>>> + int pos = 0;
>>> + while (pos < line.len) {
>>> + pos = get_next_word(line.buf, pos, &word);
>>> +
>>> + if (!strcmp(word.buf, "git")) {
>>> + continue;
>>> + } else if (!strcmp(word.buf, "git-bisect")) {
>>> + continue;
>>> + } else if (!strcmp(word.buf, "bisect")) {
>>> + continue;
>>> + } else if (!strcmp(word.buf, "#")) {
>>> + break;
>>
>> Maybe it is more robust to check whether word.buf begins with #
>
> Assuming that you meant "# ", yes.
No, if I get it right "# " can never occur because the word.buf never
contains a space.
What I meant was that you are currently ignoring everything after a
"# ", so comments like
# foo
are ignored.
However, imagine a user changes the file by hand (he probably should not
do it but, hey, it's git: unixy, hacky ... and he thinks he knows what
he does) and then we have in the file something like
#foo
which makes perfectly sense when you are used to programming languages
with # as comment-till-eol marker. The problem is that your current code
does expect "#" as a single word and would hence not recognize #foo as a
comment.
I hope I made it clear why I suggested to test if the word *begins* with
"#" (not "# ").
~Stephan
^ permalink raw reply
* Re: [PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
From: Stephan Beyer @ 2016-12-06 23:05 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPNZCSGzeabHMH6wihz-51OUMMtnBMsffwJJVm9G8Fn=tw@mail.gmail.com>
Hey Pranit,
On 12/06/2016 10:14 PM, Pranit Bauva wrote:
>>> +
>>> + 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.
>
> Ah. This is because of the message. If I do the other way, then it
> won't match the output in one of the tests in t/t6030 thus, I am
> keeping it that way in order to avoid modifying the file t/t6030.
I think I was unclear here. I was referring to the coding/layouting
style, not to the string. I mean like writing:
printf(_("Your current terms are %s for the old state\n"
"and "%s for the new state.\n"),
terms->term_good, terms->term_bad);
The string fed to _() is the same, but it is split in a different (imho
more readable) way: after the "\n", not after the "and ".
>>> + 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(...)".
>
> I should use return error()
When you reroll your patches, please also check if you always put _()
around your error()s ;) (Hmmm... On the other hand, it might be arguable
if translations are useful for errors that only occur when people hack
git-bisect or use the bisect--helper directly... This makes me feel like
all those errors should be prefixed by some "BUG: " marker since the
ordinary user only sees them when there is a bug. But I don't feel in
the position to decide or recommend such a thing, so it's just a thought.)
~Stephan
^ permalink raw reply
* Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C
From: Pranit Bauva @ 2016-12-06 23:02 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <1816d5b4-a4c1-7c97-09ff-b11001501423@gmx.net>
Hey Stephan,
On Tue, Nov 22, 2016 at 6:19 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Okay Pranit,
>
> this is the last patch for me to review in this series.
>
> Now that I have a coarse overview of what you did, I have the general
> remark that imho the "terms" variable should simply be global instead of
> being passed around all the time.
In a personal conversation with my mentors, we thought it is the best
fit to keep it in a struct and pass it around so that it is easier in
libification.
> I also had some other remarks but I forgot them... maybe they come to my
> mind again when I see patch series v16.
>
> I also want to remark again that I am not a Git developer and only
> reviewed this because of my interest in git-bisect. So some of my
> suggestions might conflict with other beliefs here. For example, I
> consider it very bad style to leak memory... but Git is rather written
> as a scripting tool than a genuine library, so perhaps many people here
> do not care about it as long as it works...
Thanks for taking out your time to review my series extremely
carefully. I will try to post a v16 next week probably.
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index c18ca07..b367d8d 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -601,7 +602,6 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
>> terms->term_good = arg;
>> } else if (!strcmp(arg, "--term-bad") ||
>> !strcmp(arg, "--term-new")) {
>> - const char *next_arg;
>
> This should already have been removed in patch 15/27, not here.
>
>> @@ -875,6 +875,117 @@ static int bisect_log(void)
>> return status;
>> }
>>
>> +static int get_next_word(const char *line, int pos, struct strbuf *word)
>> +{
>> + int i, len = strlen(line), begin = 0;
>> + strbuf_reset(word);
>> + for (i = pos; i < len; i++) {
>> + if (line[i] == ' ' && begin)
>> + return i + 1;
>> +
>> + if (!begin)
>> + begin = 1;
>> + strbuf_addch(word, line[i]);
>> + }
>> +
>> + return i;
>> +}
>> +
>> +static int bisect_replay(struct bisect_terms *terms, const char *filename)
>> +{
>> + struct strbuf line = STRBUF_INIT;
>> + struct strbuf word = STRBUF_INIT;
>> + FILE *fp = NULL;
>
> (The initialization is not necessary here.)
Um. I think it is. Otherwise if it goes to the finish block before you
try to operate on fp, it will cause a seg fault.
>> + int res = 0;
>> +
>> + if (is_empty_or_missing_file(filename)) {
>> + error(_("no such file with name '%s' exists"), filename);
>
> The error message is misleading if the file exists but is empty.
> Maybe something like "cannot read file '%s' for replaying"?
Okay will change.
>> + res = -1;
>> + goto finish;
>
> goto fail;
> :D
>
>> + }
>> +
>> + if (bisect_reset(NULL)) {
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> +
>> + fp = fopen(filename, "r");
>> + if (!fp) {
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> +
>> + while (strbuf_getline(&line, fp) != EOF) {
>> + int pos = 0;
>> + while (pos < line.len) {
>> + pos = get_next_word(line.buf, pos, &word);
>> +
>> + if (!strcmp(word.buf, "git")) {
>> + continue;
>> + } else if (!strcmp(word.buf, "git-bisect")) {
>> + continue;
>> + } else if (!strcmp(word.buf, "bisect")) {
>> + continue;
>> + } else if (!strcmp(word.buf, "#")) {
>> + break;
>
> Maybe it is more robust to check whether word.buf begins with #
Assuming that you meant "# ", yes.
>> + }
>> +
>> + get_terms(terms);
>> + if (check_and_set_terms(terms, word.buf)) {
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> +
>> + if (!strcmp(word.buf, "start")) {
>> + struct argv_array argv = ARGV_ARRAY_INIT;
>> + sq_dequote_to_argv_array(line.buf+pos, &argv);
>> + if (bisect_start(terms, 0, argv.argv, argv.argc)) {
>> + argv_array_clear(&argv);
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> + argv_array_clear(&argv);
>> + break;
>> + }
>> +
>> + if (one_of(word.buf, terms->term_good,
>> + terms->term_bad, "skip", NULL)) {
>> + if (bisect_write(word.buf, line.buf+pos, terms, 0)) {
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> + break;
>> + }
>> +
>> + if (!strcmp(word.buf, "terms")) {
>> + struct argv_array argv = ARGV_ARRAY_INIT;
>> + sq_dequote_to_argv_array(line.buf+pos, &argv);
>> + if (bisect_terms(terms, argv.argv, argv.argc)) {
>> + argv_array_clear(&argv);
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> + argv_array_clear(&argv);
>> + break;
>> + }
>> +
>> + error(_("?? what are you talking about?"));
>
> I know this string is taken from the original source. However, even
> something like error(_("Replay file contains rubbish (\"%s\")"),
> word.buf) is more informative.
Okay will change.
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> + }
>> + goto finish;
>
> +fail:
> + res = -1;
>
> I just wanted to make finally clear what I was referring to by the
> "goto fail" trick. :D
I got it. Will update accordingly.
> Also I think the readability could be improved by extracting the body of
> the outer while loop into an extra function replay_line(). Then most of
> my suggested "goto fail;" lines simply become "return -1;" :)
>
>> @@ -998,6 +1112,13 @@ int cmd_bisect__helper(...)
>> die(_("--bisect-log requires 0 arguments"));
>> res = bisect_log();
>> break;
>> + case BISECT_REPLAY:
>> + if (argc != 1)
>> + die(_("--bisect-replay requires 1 argument"));
>
> I'd keep the (already translated) string from the original source:
> "No logfile given"
Sure
Regards.
Pranit Bauva
^ permalink raw reply
* Re: [PATCH v15 10/27] bisect--helper: `check_and_set_terms` shell function in C
From: Pranit Bauva @ 2016-12-06 22:43 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <b7691fac-9642-f87d-f23f-5175b5ead05b@gmx.net>
Hey Stephan,
On Fri, Nov 18, 2016 at 1:55 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> 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.
Sure.
>> 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?
Yeah it is safe.
>> @@ -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.
I will take care about freeing the memory.
Regards,
Pranit Bauva
^ permalink raw reply
* Re: [PATCH v15 22/27] bisect--helper: `bisect_log` shell function in C
From: Pranit Bauva @ 2016-12-06 22:42 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <c49a6151-0827-cae5-0569-8f05515be0f9@gmx.net>
Hey Stephan,
On Fri, Nov 18, 2016 at 3:17 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> 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);
Sure!
>> + 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;
This one seems better!
^ permalink raw reply
* Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C
From: Pranit Bauva @ 2016-12-06 22:40 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <0cc22155-2b3f-b69c-9ed4-2b1c55e40eee@gmx.net>
Hey Stephan,
On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> Reimplement the `bisect_state` shell function in C and also add a
>> subcommand `--bisect-state` to `git-bisect--helper` to call it from
>> git-bisect.sh .
>>
>> Using `--bisect-state` subcommand is a temporary measure to port shell
>> function to C so as to use the existing test suite. As more functions
>> are ported, this subcommand will be retired and will be called by some
>> other methods.
>>
>> `bisect_head` is called from `bisect_state` thus its not required to
>> introduce another subcommand.
>
> Missing comma before "thus", and "it is" (or "it's") instead of "its" :)
Sure, will fix.
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 1767916..1481c6d 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -784,6 +786,79 @@ static int bisect_autostart(struct bisect_terms *terms)
>> return 0;
>> }
>>
>> +static char *bisect_head(void)
>> +{
>> + if (is_empty_or_missing_file(git_path_bisect_head()))
>> + return "HEAD";
>> + else
>> + return "BISECT_HEAD";
>> +}
>
> This is very shellish.
> In C I'd expect something like
>
> static int bisect_head_sha1(unsigned char *sha)
> {
> int res;
> if (is_empty_or_missing_file(git_path_bisect_head()))
> res = get_sha1("HEAD", sha);
> else
> res = get_sha1("BISECT_HEAD", sha);
>
> if (res)
> return error(_("Could not find BISECT_HEAD or HEAD."));
>
> return 0;
> }
>
>> +
>> +static int bisect_state(struct bisect_terms *terms, const char **argv,
>> + int argc)
>> +{
>> + const char *state = argv[0];
>> +
>> + get_terms(terms);
>> + if (check_and_set_terms(terms, state))
>> + return -1;
>> +
>> + if (!argc)
>> + die(_("Please call `--bisect-state` with at least one argument"));
>
> I think this check should move to cmd_bisect__helper. There are also the
> other argument number checks.
Not really. After the whole conversion, the cmdmode will cease to
exists while bisect_state will be called directly, thus it is
important to check it here.
>> +
>> + if (argc == 1 && one_of(state, terms->term_good,
>> + terms->term_bad, "skip", NULL)) {
>> + const char *bisected_head = xstrdup(bisect_head());
>> + const char *hex[1];
>
> Maybe:
> const char *hex;
>
>> + unsigned char sha1[20];
>> +
>> + if (get_sha1(bisected_head, sha1))
>> + die(_("Bad rev input: %s"), bisected_head);
>
> And instead of...
>
>> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
>> + return -1;
>> +
>> + *hex = xstrdup(sha1_to_hex(sha1));
>> + if (check_expected_revs(hex, 1))
>> + return -1;
>
> ... simply:
>
> hex = xstrdup(sha1_to_hex(sha1));
> if (set_state(terms, state, hex)) {
> free(hex);
> return -1;
> }
> free(hex);
>
> where:
Yes I am planning to convert all places with hex rather than the sha1
but not yet, maybe in an another patch series because currently a lot
of things revolve around sha1 and changing its behaviour wouldn't
really be a part of a porting patch series.
> static int set_state(struct bisect_terms *terms, const char *state,
> const char *hex)
> {
> if (bisect_write(state, hex, terms, 0))
> return -1;
> if (check_expected_revs(&hex, 1))
> return -1;
> return 0;
> }
>
>> + return bisect_auto_next(terms, NULL);
>> + }
>> +
>> + if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
>> + one_of(state, terms->term_good, "skip", NULL)) {
>> + int i;
>> + struct string_list hex = STRING_LIST_INIT_DUP;
>> +
>> + for (i = 1; i < argc; i++) {
>> + unsigned char sha1[20];
>> +
>> + if (get_sha1(argv[i], sha1)) {
>> + string_list_clear(&hex, 0);
>> + die(_("Bad rev input: %s"), argv[i]);
>> + }
>> + string_list_append(&hex, sha1_to_hex(sha1));
>> + }
>> + for (i = 0; i < hex.nr; i++) {
>
> ... And replace this:
>
>> + const char **hex_string = (const char **) &hex.items[i].string;
>> + if(bisect_write(state, *hex_string, terms, 0)) {
>> + string_list_clear(&hex, 0);
>> + return -1;
>> + }
>> + if (check_expected_revs(hex_string, 1)) {
>> + string_list_clear(&hex, 0);
>> + return -1;
>> + }
>
> by:
>
> const char *hex_str = hex.items[i].string;
> if (set_state(terms, state, hex_string)) {
> string_list_clear(&hex, 0);
> return -1;
> }
>
>> + }
>> + string_list_clear(&hex, 0);
>> + return bisect_auto_next(terms, NULL);
>> + }
>> +
>> + if (!strcmp(state, terms->term_bad))
>> + die(_("'git bisect %s' can take only one argument."),
>> + terms->term_bad);
>> +
>> + return -1;
>> +}
>> +
>> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> {
>> enum {
>> @@ -823,6 +899,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> N_("verify the next bisection state then find the next bisection state"), BISECT_AUTO_NEXT),
>> OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
>> N_("start the bisection if BISECT_START empty or missing"), BISECT_AUTOSTART),
>> + OPT_CMDMODE(0, "bisect-state", &cmdmode,
>> + N_("mark the state of ref (or refs)"), BISECT_STATE),
>
> "mark the state of the given revs"
>
> Note that rev != ref
Might have been a typo. Will fix.
>> @@ -902,6 +980,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> terms.term_bad = "bad";
>> res = bisect_autostart(&terms);
>> break;
>> + case BISECT_STATE:
>> + if (argc == 0)
>> + die(_("--bisect-state requires at least 1 argument"));
>
> "at least one revision"
Okay, that would make it more specific.
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index cd56551..a9eebbb 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -61,44 +51,7 @@ bisect_skip() {
>> esac
>> all="$all $revs"
>> done
>> - eval bisect_state 'skip' $all
> [...deleted lines...]
>> + eval git bisect--helper --bisect-state 'skip' $all
>
> I think you don't need "eval" here any longer.
Yes, I wouldn't
>> @@ -184,8 +137,8 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
>> state="$TERM_GOOD"
>> fi
>>
>> - # We have to use a subshell because "bisect_state" can exit.
>> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
>> + # We have to use a subshell because "--bisect-state" can exit.
>> + ( git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" )
True, but right now I didn't want to modify that part of the source
code to remove the comment. I will remove the comment all together
when I port bisect_run()
Regards,
Pranit Bauva
^ permalink raw reply
* Re: [PATCH 10/17] pathspec: simpler logic to prefix original pathspec elements
From: Brandon Williams @ 2016-12-06 22:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <CAGZ79kausxZMinZymG8mE4jQ1pi4yJ80WRBUGFhUK7mmfOBCvg@mail.gmail.com>
On 12/06, Stefan Beller wrote:
> On Tue, Dec 6, 2016 at 1:51 PM, Brandon Williams <bmwill@google.com> wrote:
>
> > struct strbuf sb = STRBUF_INIT;
> > - if (prefixlen && !literal_global) {
> > - /* Preserve the actual prefix length of each pattern */
> > - if (short_magic)
> > - prefix_short_magic(&sb, prefixlen, short_magic);
> > - else if (long_magic_end) {
> > - strbuf_add(&sb, elt, long_magic_end - elt);
> > - strbuf_addf(&sb, ",prefix:%d)", prefixlen);
> > - } else
> > - strbuf_addf(&sb, ":(prefix:%d)", prefixlen);
>
> This fixes the issue with add -p . mentioned somewhere else on the mailing list.
>
> > - }
> > +
> > + /* Preserve the actual prefix length of each pattern */
> > + prefix_magic(&sb, prefixlen, element_magic);
> > +
>
> Did you find a reason why we passed magic literally, i.e. short magic
> was passed as short magic and long magic as long magic before?
>
> I cannot think of any reason why that would have been the case,
> but I assume there had to be a reason for that.
nope, perhaps it was because we technically already have the long magic
string and the short magic needs to be converted to long magic (as you
can't mix short and long magic).
> Another note: This collides with the attr system refactoring, which I
> postpone redoing until the submodule checkout is done, so maybe
> you want to pickup this patch:
> https://public-inbox.org/git/20161110203428.30512-31-sbeller@google.com/
> which only relies on one patch prior
> https://public-inbox.org/git/20161110203428.30512-30-sbeller@google.com/
After looking at those patches I think I do something extremely similar
in a future patch in this series, the parse_long_magic patch.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 16/17] pathspec: small readability changes
From: Stefan Beller @ 2016-12-06 22:30 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <1481061106-117775-17-git-send-email-bmwill@google.com>
On Tue, Dec 6, 2016 at 1:51 PM, Brandon Williams <bmwill@google.com> wrote:
> A few small changes to improve readability. This is done by grouping related
> assignments, adding blank lines, ensuring lines are <80 characters, etc.
The 'etc' sounds a bit sloppy in the commit message.
Maybe s/etc/and adding proper comments/ ?
Code looks good.
^ permalink raw reply
* Re: [PATCH 10/17] pathspec: simpler logic to prefix original pathspec elements
From: Stefan Beller @ 2016-12-06 22:20 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <1481061106-117775-11-git-send-email-bmwill@google.com>
On Tue, Dec 6, 2016 at 1:51 PM, Brandon Williams <bmwill@google.com> wrote:
> struct strbuf sb = STRBUF_INIT;
> - if (prefixlen && !literal_global) {
> - /* Preserve the actual prefix length of each pattern */
> - if (short_magic)
> - prefix_short_magic(&sb, prefixlen, short_magic);
> - else if (long_magic_end) {
> - strbuf_add(&sb, elt, long_magic_end - elt);
> - strbuf_addf(&sb, ",prefix:%d)", prefixlen);
> - } else
> - strbuf_addf(&sb, ":(prefix:%d)", prefixlen);
This fixes the issue with add -p . mentioned somewhere else on the mailing list.
> - }
> +
> + /* Preserve the actual prefix length of each pattern */
> + prefix_magic(&sb, prefixlen, element_magic);
> +
Did you find a reason why we passed magic literally, i.e. short magic
was passed as short magic and long magic as long magic before?
I cannot think of any reason why that would have been the case,
but I assume there had to be a reason for that.
Another note: This collides with the attr system refactoring, which I
postpone redoing until the submodule checkout is done, so maybe
you want to pickup this patch:
https://public-inbox.org/git/20161110203428.30512-31-sbeller@google.com/
which only relies on one patch prior
https://public-inbox.org/git/20161110203428.30512-30-sbeller@google.com/
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-06 22:24 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, sbeller, bburky, jrnieder
In-Reply-To: <20161206135113.i7nlr45vg7uzgfcn@sigill.intra.peff.net>
On 12/06, Jeff King wrote:
> On Mon, Dec 05, 2016 at 12:04:52PM -0800, Junio C Hamano wrote:
>
> > > I'm sending out another reroll of this series so that in Jeff's he can
> > > just call 'get_curl_allowed_protocols(-1)' for the non-redirection curl
> > > option, which should make this test stop barfing.
> >
> > I was hoping to eventually merge Peff's series to older maintenance
> > tracks. How bad would it be if we rebased the v8 of this series
> > together with Peff's series to say v2.9 (or even older if it does
> > not look too bad)?
>
> My series actually fixes existing security problems, so I'd consider it
> a bug-fix. I _think_ Brandon's series is purely about allowing more
> expressiveness in the whitelist policy, and so could be considered more
> of a feature.
Yes this was really the main intent on my series.
> So one option is to apply my series for older 'maint', and then just
> rebase Brandon's on top of that for 'master'.
>
> I don't know if that makes things any easier. I feel funny saying "no,
> no, mine preempts yours because it is more maint-worthy", but I think
> that order does make sense.
>
> I think it would be OK to put Brandon's on maint, too, though. It is a
> refactor of an existing security feature to make it more featureful, but
> the way it is implemented could not cause security regressions unless
> you use the new feature (IOW, we still respect the whitelist environment
> exactly as before).
Either way let me know if there is something I need to do.
--
Brandon Williams
^ permalink raw reply
* Re* [BUG] Index.lock error message regression in git 2.11.0
From: Junio C Hamano @ 2016-12-06 22:15 UTC (permalink / raw)
To: Robbie Iannucci; +Cc: git, Johannes Schindelin
In-Reply-To: <xmqqbmwozppx.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Perhaps the attached would fix it (not even compile tested, though)?
>
> I would prefer to make 0 to mean "show error but return -1", 1 to
> mean "die on error", and 2 to mean "be silent and return -1 on
> error", though. Asking to be silent should be the exception for
> this error from usability and safety's point of view, and requiring
> such exceptional callers to pass LOCK_SILENT_ON_ERROR would be
> easier to "git grep" for them.
So here is the "You have to ask explicitly, if you know that it is
safe to be silent" version with a proper log message.
-- >8 --
Subject: [PATCH] lockfile: LOCK_SILENT_ON_ERROR
Recent "libify merge machinery" stopped from passing die_on_error
bit to hold_locked_index(), and lost an error message when there are
competing update in progress with "git merge --ff-only $commit", for
example. The command still exits with a non-zero status, but that
is not of much help for an interactive user. The last thing the
command says is "Updating $from..$to". We used to follow it with a
big error message that makes it clear that "merge --ff-only" did not
succeed.
Introduce a new bit "LOCK_SILENT_ON_ERROR" that can be passed by
callers that do want to silence the message (because they either
make it a non-error by doing something else, or they show their own
error message to explain the situation), and show the error message
we used to give for everybody else, including the caller that was
touched by the libification in question.
I would not be surprised if some existing calls to hold_lock*()
functions that pass die_on_error=0 need to be updated to pass
LOCK_SILENT_ON_ERROR, and when this fix is taken alone, it may look
like a regression, but we are better off starting louder and squelch
the ones that we find safe to make silent than the other way around.
Reported-by: Robbie Iannucci <iannucci@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
lockfile.c | 11 +++++++++--
lockfile.h | 8 +++++++-
2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 9268cdf325..f7e8104449 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -174,8 +174,15 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
int flags, long timeout_ms)
{
int fd = lock_file_timeout(lk, path, flags, timeout_ms);
- if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
- unable_to_lock_die(path, errno);
+ if (fd < 0) {
+ if (flags & LOCK_DIE_ON_ERROR)
+ unable_to_lock_die(path, errno);
+ else if (!(flags & LOCK_SILENT_ON_ERROR)) {
+ struct strbuf buf = STRBUF_INIT;
+ unable_to_lock_message(path, errno, &buf);
+ error("%s", buf.buf);
+ }
+ }
return fd;
}
diff --git a/lockfile.h b/lockfile.h
index d26ad27b2b..98b4862254 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -129,9 +129,15 @@ struct lock_file {
/*
* If a lock is already taken for the file, `die()` with an error
* message. If this flag is not specified, trying to lock a file that
- * is already locked returns -1 to the caller.
+ * is already locked gives the same error message and returns -1 to
+ * the caller.
*/
#define LOCK_DIE_ON_ERROR 1
+/*
+ * ... or the function can be told to be totally silent and return
+ * -1 to the caller upon error with this flag
+ */
+#define LOCK_SILENT_ON_ERROR 2
/*
* Usually symbolic links in the destination path are resolved. This
--
2.11.0-270-g0b6beed61f
^ permalink raw reply related
* Re: [BUG] Index.lock error message regression in git 2.11.0
From: Junio C Hamano @ 2016-12-06 21:56 UTC (permalink / raw)
To: Robbie Iannucci; +Cc: git, Johannes Schindelin
In-Reply-To: <CA+q_oBdHytoeSD-hmLx_N473M8XinjqckvE35Re3eNpQRWYjHQ@mail.gmail.com>
Robbie Iannucci <iannucci@google.com> writes:
> I just upgraded to 2.11.0 from 2.10.2, and I noticed that some
> commands no longer print an error message when the `index.lock` file
> exists (such as `git merge --ff-only`).
>
> It appears this bug was introduced in
> 55f5704da69d3e6836620f01bee0093ad5e331e8 (sequencer: lib'ify
> checkout_fast_forward()). I determined this by running the attached
> bisect script (on OS X, but I don't think that matters; I've also seen
> the error message missing on Linux):
Yes, I can see that this is not limited to OSX. The command does
exit with non-zero status, but that is not of much help for an
interactive user.
$ git checkout v2.11.0^0
$ >.git/index.lock
$ git merge --ff-only master; echo $?
Updating 454cb6bd52..8d7a455ed5
1
$ exit
We can see that it attempted to update, but because there is no
indication that it failed, the user can easily be misled to think
that we are now at the tip of the master branch.
We used to give "fatal: Unable to create ..." followed by "Another
git process seems to be running..." advice, that would have helped
the user from the confusion.
I do not think it is the right solution to call hold_locked_index()
with die_on_error=1 from the codepath changed by your 55f5704da6
("sequencer: lib'ify checkout_fast_forward()", 2016-09-09). Perhaps
the caller of checkout_fast_forward() needs to learn how/why the
function returned error and diagnose this case and a message? Or
perhaps turn that die_on_error parameter from boolean to tricolor
(i.e. the caller does not want you to die, but the caller knows that
you have more information to give better error message than it does,
so please show an error message instead of silently returning -1)?
Perhaps the attached would fix it (not even compile tested, though)?
I would prefer to make 0 to mean "show error but return -1", 1 to
mean "die on error", and 2 to mean "be silent and return -1 on
error", though. Asking to be silent should be the exception for
this error from usability and safety's point of view, and requiring
such exceptional callers to pass LOCK_SILENT_ON_ERROR would be
easier to "git grep" for them.
Dscho, what do you think?
lockfile.c | 11 +++++++++--
lockfile.h | 5 +++++
merge.c | 2 +-
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 9268cdf325..f190caddb0 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -174,8 +174,15 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
int flags, long timeout_ms)
{
int fd = lock_file_timeout(lk, path, flags, timeout_ms);
- if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
- unable_to_lock_die(path, errno);
+ if (fd < 0) {
+ if (flags & LOCK_DIE_ON_ERROR)
+ unable_to_lock_die(path, errno);
+ else if (flags & LOCK_ERROR_ON_ERROR) {
+ struct strbuf buf = STRBUF_INIT;
+ unable_to_lock_message(path, errno, &buf);
+ error("%s", buf.buf);
+ }
+ }
return fd;
}
diff --git a/lockfile.h b/lockfile.h
index d26ad27b2b..6cba9c8142 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -132,6 +132,11 @@ struct lock_file {
* is already locked returns -1 to the caller.
*/
#define LOCK_DIE_ON_ERROR 1
+/*
+ * ... or the function can be told to show the usual error message
+ * and still return -1 to the caller with this flag
+ */
+#define LOCK_ERROR_ON_ERROR 2
/*
* Usually symbolic links in the destination path are resolved. This
diff --git a/merge.c b/merge.c
index 23866c9165..9e2e4f1a22 100644
--- a/merge.c
+++ b/merge.c
@@ -57,7 +57,7 @@ int checkout_fast_forward(const unsigned char *head,
refresh_cache(REFRESH_QUIET);
- if (hold_locked_index(lock_file, 0) < 0)
+ if (hold_locked_index(lock_file, LOCK_ERROR_ON_ERROR) < 0)
return -1;
memset(&trees, 0, sizeof(trees));
^ permalink raw reply related
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