* Re: [PATCH 0/2] Fix remote_is_configured()
From: Jeff King @ 2017-01-17 21:45 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Thomas Gummerer, Andrew Arnott
In-Reply-To: <cover.1484687919.git.johannes.schindelin@gmx.de>
On Tue, Jan 17, 2017 at 10:18:58PM +0100, Johannes Schindelin wrote:
> A surprising behavior triggered the bug report in
> https://github.com/git-for-windows/git/issues/888: the mere existence of
> the config setting "remote.origin.prune" (in this instance, configured
> via ~/.gitconfig so that it applies to all repositories) fooled `git
> remote rename <source> <target>` into believing that the <target> remote
> is already there.
>
> This patch pair demonstrates the problem, and then fixes it (along with
> potential similar problems, such as setting an HTTP proxy for remotes of
> a given name via ~/.gitconfig).
I thought it was intentional that any config "created" the remote, even
without a url field. E.g., you can set:
[remote "https://example.com/foo/git"]
proxy = localhost:1234
and then a bare "git fetch https://example.com/foo/git" will respect it.
I admit that "prune" is probably useless without a fetch refspec,
though.
Note that I don't disagree that the rule "it's not a remote unless it
has a url" would be a lot saner. But I have a feeling you may be
breaking some existing setups.
I grepped around in the list but I couldn't find any relevant
discussion. So maybe I just dreamed it.
-Peff
^ permalink raw reply
* Re: [PATCH] document index_name_pos
From: Junio C Hamano @ 2017-01-17 21:43 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170117204642.31514-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>> These placeholders are meant to encourage those people who dove into
>> the code to update it, so from that point of view, I think removing
>> it is backwards.
>
> Yes, I am currently understanding and writing up documentation for
> index_name_pos. If I recall the latest discussion where we want to have
> documentation, I think a quorum favored documentation in the header itself,
> c.f. strbuf.h, string-list.h for the most desired state. (Although we do have
> Documentation/technical/api-string-list.txt as well ...)
>
> So maybe starting like this?
That is very good. Let's drop that file from Documentation/technical
and do it like this (meaning, take both patches from you).
Thanks.
>
> Thanks,
> Stefan
>
> cache.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 1b67f078dd..e168e4e073 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -575,7 +575,20 @@ extern int verify_path(const char *path);
> extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
> extern void adjust_dirname_case(struct index_state *istate, char *name);
> extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
> +
> +/*
> + * Searches for an entry defined by name and namelen in the given index.
> + * If the return value is positive (including 0) it is the position of an
> + * exact match. If the return value is negative, the negated value minus 1 is the
> + * position where the entry would be inserted.
> + * Example: In the current index we have the files a,c,d:
> + * index_name_pos(&index, "a", 1) -> 0
> + * index_name_pos(&index, "b", 1) -> -1
> + * index_name_pos(&index, "c", 1) -> 1
> + * index_name_pos(&index, "d", 1) -> 2
> + */
> extern int index_name_pos(const struct index_state *, const char *name, int namelen);
> +
> #define ADD_CACHE_OK_TO_ADD 1 /* Ok to add */
> #define ADD_CACHE_OK_TO_REPLACE 2 /* Ok to replace file/directory */
> #define ADD_CACHE_SKIP_DFCHECK 4 /* Ok to skip DF conflict checks */
^ permalink raw reply
* Re: [PATCH] CodingGuidelines: clarify multi-line brace style
From: Junio C Hamano @ 2017-01-17 21:41 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <20170117200503.3iwaedrgfq544b4i@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yeah. I obviously was adapting the original text, and I think I left too
> much of the wishy-washy-ness in. As the point of the patch is to avoid
> that, let's take your suggestion. A re-rolled patch is below.
>
> Now the patch is at least self-consistent. The bigger question remains
> of: do we want to dictate these rules, or did we prefer the vague
> version?
I would say no to make all of them "rules to be dictated". It is OK
to leave some things "gray".
But obviously others can vote differently ;-)
> I _thought_ the vague rules were doing fine, but this whole discussion
> has made me think otherwise. And I'd just as soon not ever have to
> repeat it. ;-/
>
> OTOH, I really do not want to review a bunch of patches that do nothing
> but change brace style (and I am sure there is a mix of styles already
> in the code base).
Yes, exactly, that is why the last sentence is in there below.
>> When the project says it does not have strong preference
>> among multiple choices, you are welcome to write your new
>> code using any of them, as long as you are consistent with
>> surrounding code. Do not change style of existing code only
>> to flip among these styles, though.
> The existing document says:
>
> Make your code readable and sensible, and don't try to be clever.
>
> As for more concrete guidelines, just imitate the existing code
> (this is a good guideline, no matter which project you are
> contributing to). It is always preferable to match the _local_
> convention. New code added to Git suite is expected to match
> the overall style of existing code. Modifications to existing
> code is expected to match the style the surrounding code already
> uses (even if it doesn't match the overall style of existing code).
>
> But if you must have a list of rules, here they are.
>
> I guess that is the place to expound on how to interpret the rules. I
> dunno. Some of the individual rules that go into "gray areas" already
> spell out the "surrounding code" rule.
We are not saying one important thing and that was what invited
useless arguing this time: For "gray" things, the choice is yours in
your new code, but do not churn existing code for "gray" guidelines.
If we made _that_ clear as a rule we dictate, hopefully we'd have
less chance to see "a bunch of patches that do nothing but change
brace style", I would think.
> -- >8 --
> Subject: [PATCH] CodingGuidelines: clarify multi-line brace style
>
> There are some "gray areas" around when to omit braces from
> a conditional or loop body. Since that seems to have
> resulted in some arguments, let's be a little more clear
> about our preferred style.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Documentation/CodingGuidelines | 37 ++++++++++++++++++++++++++++++++-----
> 1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 4cd95da6b..a4191aa38 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -206,11 +206,38 @@ For C programs:
> x = 1;
> }
>
> - is frowned upon. A gray area is when the statement extends
> - over a few lines, and/or you have a lengthy comment atop of
> - it. Also, like in the Linux kernel, if there is a long list
> - of "else if" statements, it can make sense to add braces to
> - single line blocks.
> + is frowned upon. But there are a few exceptions:
> +
> + - When the statement extends over a few lines (e.g., a while loop
> + with an embedded conditional, or a comment). E.g.:
> +
> + while (foo) {
> + if (x)
> + one();
> + else
> + two();
> + }
> +
> + if (foo) {
> + /*
> + * This one requires some explanation,
> + * so we're better off with braces to make
> + * it obvious that the indentation is correct.
> + */
> + doit();
> + }
> +
> + - When there are multiple arms to a conditional and some of them
> + require braces, enclose even a single line block in braces for
> + consistency. E.g.:
> +
> + if (foo) {
> + doit();
> + } else {
> + one();
> + two();
> + three();
> + }
>
> - We try to avoid assignments in the condition of an "if" statement.
^ permalink raw reply
* Re: [PATCH v5 0/3] Turn the difftool into a builtin
From: Junio C Hamano @ 2017-01-17 21:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1484668473.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> - replaced the cross-validation with the Perl script by a patch that
> retires the Perl script instead.
Yup. That makes things a lot simpler. While we try to be careful
during major rewrite, we usually do not go extra careful to leave
two competing implementations in-tree.
Will replace js/difftool-builtin topic. Thanks.
^ permalink raw reply
* Re: [PATCH 3/6] fsck: prepare dummy objects for --connectivity-check
From: Jeff King @ 2017-01-17 21:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty, Johannes Schindelin
In-Reply-To: <xmqqa8ape6b6.fsf@gitster.mtv.corp.google.com>
On Tue, Jan 17, 2017 at 01:15:57PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > +static void mark_object_for_connectivity(const unsigned char *sha1)
> > +{
> > + struct object *obj = lookup_object(sha1);
> > +
> > + /*
> > + * Setting the object type here isn't strictly necessary here for a
> > + * connectivity check.
>
> Drop one of these "here"s?
Yeah.
> > The cmd_fsck() part of the diff is pretty nasty without
> > "-b".
>
> True. I also wonder if swapping the if/else arms make the end
> result and the patch easier to read. i.e.
>
> + if (connectivity_only) {
> + mark loose for connectivity;
> + mark packed for connectivity;
> + } else {
> ... existing code comes here reindented ...
> }
>
> But the patch makes sense.
Yeah. It doesn't help the diff, but the end result is more readable. And
the conditional lacks a negation, which is nice. Will change.
> > +test_expect_success 'fsck --connectivity-only with explicit head' '
> > + (
> > + cd connectivity-only &&
> > + test_must_fail git fsck --connectivity-only $tree
> > + )
> > +'
>
> Most of the earlier "tree=..." assignments are done in subshells,
> and it is not clear which tree this refers to. Is this the one that
> was written in 'rev-list --verify-objects with bad sha1' that has
> been removed in its when-finished handler?
Doh. It was meant to be the one from the earlier --connectivity-only
check. But that is doubly silly, even, because the tree variable in that
case is holding the filename, not the sha1. The right thing is to call
rev-parse again, but of course that doesn't work because the repo has
been corrupted. :-/
Here's a re-roll with the two changes above (and the notice thing you
mentioned elsewhere), plus a test that actually checks the right thing
(fails before this commit and passes after).
If everything else looks good, you can queue it along with the rest.
Otherwise if I need a re-roll, it will be in the next version.
-- >8 --
Subject: fsck: prepare dummy objects for --connectivity-check
Normally fsck makes a pass over all objects to check their
integrity, and then follows up with a reachability check to
make sure we have all of the referenced objects (and to know
which ones are dangling). The latter checks for the HAS_OBJ
flag in obj->flags to see if we found the object in the
first pass.
Commit 02976bf85 (fsck: introduce `git fsck --connectivity-only`,
2015-06-22) taught fsck to skip the initial pass, and to
fallback to has_sha1_file() instead of the HAS_OBJ check.
However, it converted only one HAS_OBJ check to use
has_sha1_file(). But there are many other places in
builtin/fsck.c that assume that the flag is set (or that
lookup_object() will return an object at all). This leads to
several bugs with --connectivity-only:
1. mark_object() will not queue objects for examination,
so recursively following links from commits to trees,
etc, did nothing. I.e., we were checking the
reachability of hardly anything at all.
2. When a set of heads is given on the command-line, we
use lookup_object() to see if they exist. But without
the initial pass, we assume nothing exists.
3. When loading reflog entries, we do a similar
lookup_object() check, and complain that the reflog is
broken if the object doesn't exist in our hash.
So in short, --connectivity-only is broken pretty badly, and
will claim that your repository is fine when it's not.
Presumably nobody noticed for a few reasons.
One is that the embedded test does not actually test the
recursive nature of the reachability check. All of the
missing objects are still in the index, and we directly
check items from the index. This patch modifies the test to
delete the index, which shows off breakage (1).
Another is that --connectivity-only just skips the initial
pass for loose objects. So on a real repository, the packed
objects were still checked correctly. But on the flipside,
it means that "git fsck --connectivity-only" still checks
the sha1 of all of the packed objects, nullifying its
original purpose of being a faster git-fsck.
And of course the final problem is that the bug only shows
up when there _is_ corruption, which is rare. So anybody
running "git fsck --connectivity-only" proactively would
assume it was being thorough, when it was not.
One possibility for fixing this is to find all of the spots
that rely on HAS_OBJ and tweak them for the connectivity-only
case. But besides the risk that we might miss a spot (and I
found three already, corresponding to the three bugs above),
there are other parts of fsck that _can't_ work without a
full list of objects. E.g., the list of dangling objects.
Instead, let's make the connectivity-only case look more
like the normal case. Rather than skip the initial pass
completely, we'll do an abbreviated one that sets up the
HAS_OBJ flag for each object, without actually loading the
object data.
That's simple and fast, and we don't have to care about the
connectivity_only flag in the rest of the code at all.
While we're at it, let's make sure we treat loose and packed
objects the same (i.e., setting up dummy objects for both
and skipping the actual sha1 check). That makes the
connectivity-only check actually fast on a real repo (40
seconds versus 180 seconds on my copy of linux.git).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fsck.c | 120 +++++++++++++++++++++++++++++++++++++++++++++-----------
t/t1450-fsck.sh | 29 +++++++++++++-
2 files changed, 124 insertions(+), 25 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 3e67203f9..75e836e2f 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -205,8 +205,6 @@ static void check_reachable_object(struct object *obj)
if (!(obj->flags & HAS_OBJ)) {
if (has_sha1_pack(obj->oid.hash))
return; /* it is in pack - forget about it */
- if (connectivity_only && has_object_file(&obj->oid))
- return;
printf("missing %s %s\n", typename(obj->type),
describe_object(obj));
errors_found |= ERROR_REACHABLE;
@@ -584,6 +582,79 @@ static int fsck_cache_tree(struct cache_tree *it)
return err;
}
+static void mark_object_for_connectivity(const unsigned char *sha1)
+{
+ struct object *obj = lookup_object(sha1);
+
+ /*
+ * Setting the object type here isn't strictly necessary for a
+ * connectivity check. In most cases, our walk will expect a certain
+ * type (e.g., a tree referencing a blob) and will use lookup_blob() to
+ * assign the type. But doing it here has two advantages:
+ *
+ * 1. When the fsck_walk code looks at objects that _don't_ come from
+ * links (e.g., the tip of a ref), it may complain about the
+ * "unknown object type".
+ *
+ * 2. This serves as a nice cross-check that the graph links are
+ * sane. So --connectivity-only does not check that the bits of
+ * blobs are not corrupted, but it _does_ check that 100644 tree
+ * entries point to blobs, and so forth.
+ *
+ * Unfortunately we can't just use parse_object() here, because the
+ * whole point of --connectivity-only is to avoid reading the object
+ * data more than necessary.
+ */
+ if (!obj || obj->type == OBJ_NONE) {
+ enum object_type type = sha1_object_info(sha1, NULL);
+ switch (type) {
+ case OBJ_BAD:
+ error("%s: unable to read object type",
+ sha1_to_hex(sha1));
+ break;
+ case OBJ_COMMIT:
+ obj = (struct object *)lookup_commit(sha1);
+ break;
+ case OBJ_TREE:
+ obj = (struct object *)lookup_tree(sha1);
+ break;
+ case OBJ_BLOB:
+ obj = (struct object *)lookup_blob(sha1);
+ break;
+ case OBJ_TAG:
+ obj = (struct object *)lookup_tag(sha1);
+ break;
+ default:
+ error("%s: unknown object type %d",
+ sha1_to_hex(sha1), type);
+ }
+
+ if (!obj || obj->type == OBJ_NONE) {
+ errors_found |= ERROR_OBJECT;
+ return;
+ }
+ }
+
+ obj->flags |= HAS_OBJ;
+}
+
+static int mark_loose_for_connectivity(const unsigned char *sha1,
+ const char *path,
+ void *data)
+{
+ mark_object_for_connectivity(sha1);
+ return 0;
+}
+
+static int mark_packed_for_connectivity(const unsigned char *sha1,
+ struct packed_git *pack,
+ uint32_t pos,
+ void *data)
+{
+ mark_object_for_connectivity(sha1);
+ return 0;
+}
+
static char const * const fsck_usage[] = {
N_("git fsck [<options>] [<object>...]"),
NULL
@@ -640,38 +711,41 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
git_config(fsck_config, NULL);
fsck_head_link();
- if (!connectivity_only) {
+ if (connectivity_only) {
+ for_each_loose_object(mark_loose_for_connectivity, NULL, 0);
+ for_each_packed_object(mark_packed_for_connectivity, NULL, 0);
+ } else {
fsck_object_dir(get_object_directory());
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next)
fsck_object_dir(alt->path);
- }
- if (check_full) {
- struct packed_git *p;
- uint32_t total = 0, count = 0;
- struct progress *progress = NULL;
+ if (check_full) {
+ struct packed_git *p;
+ uint32_t total = 0, count = 0;
+ struct progress *progress = NULL;
+
+ prepare_packed_git();
- prepare_packed_git();
+ if (show_progress) {
+ for (p = packed_git; p; p = p->next) {
+ if (open_pack_index(p))
+ continue;
+ total += p->num_objects;
+ }
- if (show_progress) {
+ progress = start_progress(_("Checking objects"), total);
+ }
for (p = packed_git; p; p = p->next) {
- if (open_pack_index(p))
- continue;
- total += p->num_objects;
+ /* verify gives error messages itself */
+ if (verify_pack(p, fsck_obj_buffer,
+ progress, count))
+ errors_found |= ERROR_PACK;
+ count += p->num_objects;
}
-
- progress = start_progress(_("Checking objects"), total);
- }
- for (p = packed_git; p; p = p->next) {
- /* verify gives error messages itself */
- if (verify_pack(p, fsck_obj_buffer,
- progress, count))
- errors_found |= ERROR_PACK;
- count += p->num_objects;
+ stop_progress(&progress);
}
- stop_progress(&progress);
}
heads = 0;
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index e88ec7747..4d1c3ba66 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -523,9 +523,21 @@ test_expect_success 'fsck --connectivity-only' '
touch empty &&
git add empty &&
test_commit empty &&
+
+ # Drop the index now; we want to be sure that we
+ # recursively notice the broken objects
+ # because they are reachable from refs, not because
+ # they are in the index.
+ rm -f .git/index &&
+
+ # corrupt the blob, but in a way that we can still identify
+ # its type. That lets us see that --connectivity-only is
+ # not actually looking at the contents, but leaves it
+ # free to examine the type if it chooses.
empty=.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 &&
- rm -f $empty &&
- echo invalid >$empty &&
+ blob=$(echo unrelated | git hash-object -w --stdin) &&
+ mv $(sha1_file $blob) $empty &&
+
test_must_fail git fsck --strict &&
git fsck --strict --connectivity-only &&
tree=$(git rev-parse HEAD:) &&
@@ -537,6 +549,19 @@ test_expect_success 'fsck --connectivity-only' '
)
'
+test_expect_success 'fsck --connectivity-only with explicit head' '
+ rm -rf connectivity-only &&
+ git init connectivity-only &&
+ (
+ cd connectivity-only &&
+ test_commit foo &&
+ rm -f .git/index &&
+ tree=$(git rev-parse HEAD^{tree}) &&
+ remove_object $(git rev-parse HEAD:foo.t) &&
+ test_must_fail git fsck --connectivity-only $tree
+ )
+'
+
remove_loose_object () {
sha1="$(git rev-parse "$1")" &&
remainder=${sha1#??} &&
--
2.11.0.651.g41f4a5c4e
^ permalink raw reply related
* Re: [PATCH 3/6] fsck: prepare dummy objects for --connectivity-check
From: Junio C Hamano @ 2017-01-17 21:15 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty, Johannes Schindelin
In-Reply-To: <20170116213204.e7ykwowqzafkexqd@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> +static void mark_object_for_connectivity(const unsigned char *sha1)
> +{
> + struct object *obj = lookup_object(sha1);
> +
> + /*
> + * Setting the object type here isn't strictly necessary here for a
> + * connectivity check.
Drop one of these "here"s?
> The cmd_fsck() part of the diff is pretty nasty without
> "-b".
True. I also wonder if swapping the if/else arms make the end
result and the patch easier to read. i.e.
+ if (connectivity_only) {
+ mark loose for connectivity;
+ mark packed for connectivity;
+ } else {
... existing code comes here reindented ...
}
But the patch makes sense.
> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index e88ec7747..c1b2dda33 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -523,9 +523,21 @@ test_expect_success 'fsck --connectivity-only' '
> touch empty &&
> git add empty &&
> test_commit empty &&
> +
> + # Drop the index now; we want to be sure that we
> + # recursively notice that we notice the broken objects
> + # because they are reachable from refs, not because
> + # they are in the index.
> + rm -f .git/index &&
> +
> + # corrupt the blob, but in a way that we can still identify
> + # its type. That lets us see that --connectivity-only is
> + # not actually looking at the contents, but leaves it
> + # free to examine the type if it chooses.
> empty=.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 &&
> - rm -f $empty &&
> - echo invalid >$empty &&
> + blob=$(echo unrelated | git hash-object -w --stdin) &&
> + mv $(sha1_file $blob) $empty &&
> +
> test_must_fail git fsck --strict &&
> git fsck --strict --connectivity-only &&
> tree=$(git rev-parse HEAD:) &&
> @@ -537,6 +549,13 @@ test_expect_success 'fsck --connectivity-only' '
> )
> '
>
> +test_expect_success 'fsck --connectivity-only with explicit head' '
> + (
> + cd connectivity-only &&
> + test_must_fail git fsck --connectivity-only $tree
> + )
> +'
Most of the earlier "tree=..." assignments are done in subshells,
and it is not clear which tree this refers to. Is this the one that
was written in 'rev-list --verify-objects with bad sha1' that has
been removed in its when-finished handler?
> remove_loose_object () {
> sha1="$(git rev-parse "$1")" &&
> remainder=${sha1#??} &&
^ permalink raw reply
* [PATCH 2/2] Be more careful when determining whether a remote was configured
From: Johannes Schindelin @ 2017-01-17 21:19 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Thomas Gummerer, Andrew Arnott
In-Reply-To: <cover.1484687919.git.johannes.schindelin@gmx.de>
One of the really nice features of the ~/.gitconfig file is that users
can override defaults by their own preferred settings for all of their
repositories.
One such default that some users like to override is whether the
"origin" remote gets auto-pruned or not. The user would simply call
git config --global remote.origin.prune true
and from now on all "origin" remotes would be pruned automatically when
fetching into the local repository.
There is just one catch: now Git thinks that the "origin" remote is
configured, as it does not discern between having a remote whose
fetch (and/or push) URL and refspec is set, and merely having
preemptively-configured, global flags for specific remotes.
Let's fix this by telling Git that a remote is not configured unless any
fetch/push URL or refspect is configured explicitly.
As a special exception, we deem a remote configured also when *only* the
"vcs" setting is configured. The commit a31eeae27f (remote: use
remote_is_configured() for add and rename, 2016-02-16) specifically
extended our test suite to verify this, so it is safe to assume that there
has been at least one user with a legitimate use case for this.
This fixes https://github.com/git-for-windows/git/issues/888
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
remote.c | 9 ++++++++-
remote.h | 2 +-
t/t5505-remote.sh | 2 +-
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/remote.c b/remote.c
index ad6c5424ed..298f2f93fa 100644
--- a/remote.c
+++ b/remote.c
@@ -255,6 +255,7 @@ static void read_remotes_file(struct remote *remote)
if (!f)
return;
+ remote->configured = 1;
remote->origin = REMOTE_REMOTES;
while (strbuf_getline(&buf, f) != EOF) {
const char *v;
@@ -289,6 +290,7 @@ static void read_branches_file(struct remote *remote)
return;
}
+ remote->configured = 1;
remote->origin = REMOTE_BRANCHES;
/*
@@ -384,21 +386,25 @@ static int handle_config(const char *key, const char *value, void *cb)
if (git_config_string(&v, key, value))
return -1;
add_url(remote, v);
+ remote->configured = 1;
} else if (!strcmp(subkey, "pushurl")) {
const char *v;
if (git_config_string(&v, key, value))
return -1;
add_pushurl(remote, v);
+ remote->configured = 1;
} else if (!strcmp(subkey, "push")) {
const char *v;
if (git_config_string(&v, key, value))
return -1;
add_push_refspec(remote, v);
+ remote->configured = 1;
} else if (!strcmp(subkey, "fetch")) {
const char *v;
if (git_config_string(&v, key, value))
return -1;
add_fetch_refspec(remote, v);
+ remote->configured = 1;
} else if (!strcmp(subkey, "receivepack")) {
const char *v;
if (git_config_string(&v, key, value))
@@ -427,6 +433,7 @@ static int handle_config(const char *key, const char *value, void *cb)
return git_config_string((const char **)&remote->http_proxy_authmethod,
key, value);
} else if (!strcmp(subkey, "vcs")) {
+ remote->configured = 1;
return git_config_string(&remote->foreign_vcs, key, value);
}
return 0;
@@ -716,7 +723,7 @@ struct remote *pushremote_get(const char *name)
int remote_is_configured(struct remote *remote)
{
- return remote && remote->origin;
+ return remote && remote->configured;
}
int for_each_remote(each_remote_fn fn, void *priv)
diff --git a/remote.h b/remote.h
index 924881169d..7e6c8067bb 100644
--- a/remote.h
+++ b/remote.h
@@ -15,7 +15,7 @@ struct remote {
struct hashmap_entry ent; /* must be first */
const char *name;
- int origin;
+ int origin, configured;
const char *foreign_vcs;
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index d7e41e9230..09c9823002 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -764,7 +764,7 @@ test_expect_success 'rename a remote with name prefix of other remote' '
)
'
-test_expect_failure 'rename succeeds with existing remote.<target>.prune' '
+test_expect_success 'rename succeeds with existing remote.<target>.prune' '
git clone one four.four &&
(
cd four.four &&
--
2.11.0.windows.3
^ permalink raw reply related
* [PATCH 1/2] remote rename: demonstrate a bogus "remote exists" bug
From: Johannes Schindelin @ 2017-01-17 21:19 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Thomas Gummerer, Andrew Arnott
In-Reply-To: <cover.1484687919.git.johannes.schindelin@gmx.de>
Some users like to set `remote.origin.prune = true` in their ~/.gitconfig
so that all of their repositories use that default.
However, our code is ill-prepared for this, mistaking that single entry to
mean that there is already a remote of the name "origin", even if there is
not.
This patch adds a test case demonstrating this issue.
Reported by Andrew Arnott.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t5505-remote.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 8198d8eb05..d7e41e9230 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -764,6 +764,15 @@ test_expect_success 'rename a remote with name prefix of other remote' '
)
'
+test_expect_failure 'rename succeeds with existing remote.<target>.prune' '
+ git clone one four.four &&
+ (
+ cd four.four &&
+ git config remote.upstream.prune true &&
+ git remote rename origin upstream
+ )
+'
+
cat >remotes_origin <<EOF
URL: $(pwd)/one
Push: refs/heads/master:refs/heads/upstream
--
2.11.0.windows.3
^ permalink raw reply related
* [PATCH 0/2] Fix remote_is_configured()
From: Johannes Schindelin @ 2017-01-17 21:18 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Thomas Gummerer, Andrew Arnott
A surprising behavior triggered the bug report in
https://github.com/git-for-windows/git/issues/888: the mere existence of
the config setting "remote.origin.prune" (in this instance, configured
via ~/.gitconfig so that it applies to all repositories) fooled `git
remote rename <source> <target>` into believing that the <target> remote
is already there.
This patch pair demonstrates the problem, and then fixes it (along with
potential similar problems, such as setting an HTTP proxy for remotes of
a given name via ~/.gitconfig).
Johannes Schindelin (2):
remote rename: demonstrate a bogus "remote exists" bug
Be more careful when determining whether a remote was configured
remote.c | 9 ++++++++-
remote.h | 2 +-
t/t5505-remote.sh | 9 +++++++++
3 files changed, 18 insertions(+), 2 deletions(-)
base-commit: d7dffce1cebde29a0c4b309a79e4345450bf352a
Published-As: https://github.com/dscho/git/releases/tag/rename-remote-v1
Fetch-It-Via: git fetch https://github.com/dscho/git rename-remote-v1
--
2.11.0.windows.3
^ permalink raw reply
* Re: [PATCH 4/6] fsck: tighten error-checks of "git fsck <head>"
From: Junio C Hamano @ 2017-01-17 21:17 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty, Johannes Schindelin
In-Reply-To: <20170116213329.jk26zvcp7erzfc6l@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Instead of checking reachability from the refs, you can ask
> fsck to check from a particular set of heads. However, the
> error checking here is quite lax. In particular:
>
> 1. It claims lookup_object() will report an error, which
> is not true. It only does a hash lookup, and the user
> has no clue that their argument was skipped.
>
> 2. When either the name or sha1 cannot be resolved, we
> continue to exit with a successful error code, even
> though we didn't check what the user asked us to.
>
> This patch fixes both of these cases.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
Makes sense, too. Thanks.
^ permalink raw reply
* Re: [PATCH 3/6] fsck: prepare dummy objects for --connectivity-check
From: Junio C Hamano @ 2017-01-17 21:16 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty, Johannes Schindelin
In-Reply-To: <20170116213204.e7ykwowqzafkexqd@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> + # Drop the index now; we want to be sure that we
> + # recursively notice that we notice the broken objects
> + # because they are reachable from refs, not because
> + # they are in the index.
Rephrase to reduce redundunt "notice"s?
^ permalink raw reply
* Re: [PATCH 1/6] t1450: clean up sub-objects in duplicate-entry test
From: Jeff King @ 2017-01-17 20:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty, Johannes Schindelin
In-Reply-To: <xmqqlgu9e7dw.fsf@gitster.mtv.corp.google.com>
On Tue, Jan 17, 2017 at 12:52:43PM -0800, Junio C Hamano wrote:
> > Since the setup code happens inside a subshell, we can't
> > just set a variable for each object. However, we can stuff
> > all of the sha1s into the $T output variable, which is not
> > used for anything except cleanup.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > t/t1450-fsck.sh | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Thanks.
>
> It is tempting to move this loop to remove_object, but that is not
> necessary while the user is only this one.
I agree it would be less gross. I avoided it because I knew that I
hacked up remove_object() in the other topic.
-Peff
^ permalink raw reply
* Re: [PATCH 1/6] t1450: clean up sub-objects in duplicate-entry test
From: Junio C Hamano @ 2017-01-17 20:52 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty, Johannes Schindelin
In-Reply-To: <20170116212403.l7ca7crmt47id3mu@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This test creates a multi-level set of trees, but its
> cleanup routine only removes the top-level tree. After the
> test finishes, the inner tree and the blob it points to
> remain, making the inner tree dangling.
>
> A later test ("cleaned up") verifies that we've removed any
> cruft and "git fsck" output is clean. This passes only
> because of a bug in git-fsck which fails to notice dangling
> trees.
>
> In preparation for fixing the bug, let's teach this earlier
> test to clean up after itself correctly. We have to remove
> the inner tree (and therefore the blob, too, which becomes
> dangling after removing that tree).
>
> Since the setup code happens inside a subshell, we can't
> just set a variable for each object. However, we can stuff
> all of the sha1s into the $T output variable, which is not
> used for anything except cleanup.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t1450-fsck.sh | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Thanks.
It is tempting to move this loop to remove_object, but that is not
necessary while the user is only this one.
>
> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index ee7d4736d..6eef8b28e 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -189,14 +189,16 @@ test_expect_success 'commit with NUL in header' '
> '
>
> test_expect_success 'tree object with duplicate entries' '
> - test_when_finished "remove_object \$T" &&
> + test_when_finished "for i in \$T; do remove_object \$i; done" &&
> T=$(
> GIT_INDEX_FILE=test-index &&
> export GIT_INDEX_FILE &&
> rm -f test-index &&
> >x &&
> git add x &&
> + git rev-parse :x &&
> T=$(git write-tree) &&
> + echo $T &&
> (
> git cat-file tree $T &&
> git cat-file tree $T
^ permalink raw reply
* Re: [PATCH 2/6] fsck: report trees as dangling
From: Junio C Hamano @ 2017-01-17 20:57 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty, Johannes Schindelin
In-Reply-To: <20170116212535.cohvikwkju5zehr4@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> After checking connectivity, fsck looks through the list of
> any objects we've seen mentioned, and reports unreachable
> and un-"used" ones as dangling. However, it skips any object
> which is not marked as "parsed", as that is an object that
> we _don't_ have (but that somebody mentioned).
>
> Since 6e454b9a3 (clear parsed flag when we free tree
> buffers, 2013-06-05), that flag can't be relied on, and the
> correct method is to check the HAS_OBJ flag. The cleanup in
> that commit missed this callsite, though. As a result, we
> would generally fail to report dangling trees.
>
> We never noticed because there were no tests in this area
> (for trees or otherwise). Let's add some.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
Makes sense, and the new test is very easy to read, too.
Queued; thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Jeff King @ 2017-01-17 20:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <xmqqtw8xe8bp.fsf@gitster.mtv.corp.google.com>
On Tue, Jan 17, 2017 at 12:32:26PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > That was my general impression, too. But I seem to recall it was you in
> > a nearby thread saying that:
> >
> > if (foo)
> > bar();
> > else {
> > one();
> > two();
> > }
> >
> > was wrong. Maybe I misunderstood.
>
> If it were a new code written like the above, that would have been
> fine. If a new code written with both sides inside {}, that would
> have been fine, too.
>
> IIRC, it was that the original had {} on both, and a patch tried to
> turn that into the above, triggering "why are we churning between
> two acceptable forms?"
Ah, OK. I didn't follow that discussion closely enough to realize that.
-Peff
^ permalink raw reply
* [PATCH] document index_name_pos
From: Stefan Beller @ 2017-01-17 20:46 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <xmqqpojle85c.fsf@gitster.mtv.corp.google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
> These placeholders are meant to encourage those people who dove into
> the code to update it, so from that point of view, I think removing
> it is backwards.
Yes, I am currently understanding and writing up documentation for
index_name_pos. If I recall the latest discussion where we want to have
documentation, I think a quorum favored documentation in the header itself,
c.f. strbuf.h, string-list.h for the most desired state. (Although we do have
Documentation/technical/api-string-list.txt as well ...)
So maybe starting like this?
Thanks,
Stefan
cache.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/cache.h b/cache.h
index 1b67f078dd..e168e4e073 100644
--- a/cache.h
+++ b/cache.h
@@ -575,7 +575,20 @@ extern int verify_path(const char *path);
extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
extern void adjust_dirname_case(struct index_state *istate, char *name);
extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
+
+/*
+ * Searches for an entry defined by name and namelen in the given index.
+ * If the return value is positive (including 0) it is the position of an
+ * exact match. If the return value is negative, the negated value minus 1 is the
+ * position where the entry would be inserted.
+ * Example: In the current index we have the files a,c,d:
+ * index_name_pos(&index, "a", 1) -> 0
+ * index_name_pos(&index, "b", 1) -> -1
+ * index_name_pos(&index, "c", 1) -> 1
+ * index_name_pos(&index, "d", 1) -> 2
+ */
extern int index_name_pos(const struct index_state *, const char *name, int namelen);
+
#define ADD_CACHE_OK_TO_ADD 1 /* Ok to add */
#define ADD_CACHE_OK_TO_REPLACE 2 /* Ok to replace file/directory */
#define ADD_CACHE_SKIP_DFCHECK 4 /* Ok to skip DF conflict checks */
--
2.11.0.297.g298debce27
^ permalink raw reply related
* Re: [PATCH] documentation: remove unfinished documentation
From: Junio C Hamano @ 2017-01-17 20:36 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170117200147.25425-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> When looking for documentation for a specific function, you may be tempted
> to run
>
> git -C Documentation grep index_name_pos
>
> only to find the file technical/api-in-core-index.txt, which doesn't
> help for understanding the given function. It would be better to not find
> these functions in the documentation, such that people directly dive into
> the code instead.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> I run into this a couple of times now, so I put this out tentatively.
These placeholders are meant to encourage those people who dove into
the code to update it, so from that point of view, I think removing
it is backwards.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Junio C Hamano @ 2017-01-17 20:32 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <20170117193639.mt3x3md3nbh2qgws@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> That was my general impression, too. But I seem to recall it was you in
> a nearby thread saying that:
>
> if (foo)
> bar();
> else {
> one();
> two();
> }
>
> was wrong. Maybe I misunderstood.
If it were a new code written like the above, that would have been
fine. If a new code written with both sides inside {}, that would
have been fine, too.
IIRC, it was that the original had {} on both, and a patch tried to
turn that into the above, triggering "why are we churning between
two acceptable forms?"
^ permalink raw reply
* Re: [RFC] stash --continue
From: Junio C Hamano @ 2017-01-17 20:21 UTC (permalink / raw)
To: Stephan Beyer; +Cc: git
In-Reply-To: <cd784a4e-ee99-564e-81de-9f7f6cc26c67@gmx.net>
Stephan Beyer <s-beyer@gmx.net> writes:
> This led to the idea to have something like "git stash --continue"[1]
> that would expect the user to "git add" the resolved files (as "git
> status" suggests) but then leads to the expected result, i.e. the index
> being the same as before the conflict, the stash being dropped (if "pop"
> was used instead of "apply"), etc.
>
> Likewise, some "git stash --abort"[2] might be useful in case you did
> "git stash pop" with the wrong stash in mind.
>
> What do you think about that?
"git stash pop --continue" (and "git stash apply --continue") would
make quite a lot of sense. I like it very much primarily because it
will give us an opportunity to correct a major UI glitches around
applying stashed changes to the working tree.
Don't people find it strange that "stash pop" that applies cleanly
would not touch the index, leaving (an equivalent of) the changes
stashed earlier floating in the working tree, but "stash pop" that
conflicts and needs a three-way merge touches the index and the
usual way of concluding the manual conflict resolution is "git add"
the paths, meaning that the changes that were not ready hence
floating in the working tree back when the stash was made goes into
the index when the user concludes "stash pop"?
With an explicit "--continue", we can fix that so that we reset the
index to the HEAD. That way, whether the changes have conflict with
the HEAD's tree or not, the end user after "stash pop" will see the
changes in the working tree and "git diff" (no HEAD argument or
"--cached" option) will consistently show what came from the stash.
^ permalink raw reply
* Re: [PATCH] CodingGuidelines: clarify multi-line brace style
From: Jeff King @ 2017-01-17 20:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <xmqqfukhfpcc.fsf@gitster.mtv.corp.google.com>
On Tue, Jan 17, 2017 at 11:39:31AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> I think this is pretty clearly the "gray area" mentioned there. Which
> >> yes, does not say "definitely do it this way", but I hope makes it clear
> >> that you're supposed to use judgement about readability.
> >
> > So here's a patch.
> >
> > I know we've usually tried to keep this file to guidelines and not
> > rules, but clearly it has not been clear-cut enough in this instance.
>
> I have two "Huh?" with this patch.
>
> 1. Two exceptions are not treated the same way. The first one is
> "... extends over a few lines, it is excempt from the rule,
> period". The second one, is ambivalent by saying "it can make
> sense", implying that "it may not make sense", so I am not sure
> if this is clarifying that much.
>
> If we want to clarify, perhaps drop "it can make sense to" and
> say
>
> When there are multiple arms to a conditional, and some of
> them require braces, enclose even a single line block in
> braces for consistency.
>
> perhaps?
Yeah. I obviously was adapting the original text, and I think I left too
much of the wishy-washy-ness in. As the point of the patch is to avoid
that, let's take your suggestion. A re-rolled patch is below.
Now the patch is at least self-consistent. The bigger question remains
of: do we want to dictate these rules, or did we prefer the vague
version?
I _thought_ the vague rules were doing fine, but this whole discussion
has made me think otherwise. And I'd just as soon not ever have to
repeat it. ;-/
OTOH, I really do not want to review a bunch of patches that do nothing
but change brace style (and I am sure there is a mix of styles already
in the code base).
> 2. I actually think it is OK to leave some things "gray", but the
> confusion comes when people do not know what to do to things
> that are "gray", and they need a rule for that to be spelled
> out.
>
> When the project says it does not have strong preference
> among multiple choices, you are welcome to write your new
> code using any of them, as long as you are consistent with
> surrounding code. Do not change style of existing code only
> to flip among these styles, though.
>
> That obviously is not limited to the rule/guideline for braces.
The existing document says:
Make your code readable and sensible, and don't try to be clever.
As for more concrete guidelines, just imitate the existing code
(this is a good guideline, no matter which project you are
contributing to). It is always preferable to match the _local_
convention. New code added to Git suite is expected to match
the overall style of existing code. Modifications to existing
code is expected to match the style the surrounding code already
uses (even if it doesn't match the overall style of existing code).
But if you must have a list of rules, here they are.
I guess that is the place to expound on how to interpret the rules. I
dunno. Some of the individual rules that go into "gray areas" already
spell out the "surrounding code" rule.
-Peff
-- >8 --
Subject: [PATCH] CodingGuidelines: clarify multi-line brace style
There are some "gray areas" around when to omit braces from
a conditional or loop body. Since that seems to have
resulted in some arguments, let's be a little more clear
about our preferred style.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/CodingGuidelines | 37 ++++++++++++++++++++++++++++++++-----
1 file changed, 32 insertions(+), 5 deletions(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 4cd95da6b..a4191aa38 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -206,11 +206,38 @@ For C programs:
x = 1;
}
- is frowned upon. A gray area is when the statement extends
- over a few lines, and/or you have a lengthy comment atop of
- it. Also, like in the Linux kernel, if there is a long list
- of "else if" statements, it can make sense to add braces to
- single line blocks.
+ is frowned upon. But there are a few exceptions:
+
+ - When the statement extends over a few lines (e.g., a while loop
+ with an embedded conditional, or a comment). E.g.:
+
+ while (foo) {
+ if (x)
+ one();
+ else
+ two();
+ }
+
+ if (foo) {
+ /*
+ * This one requires some explanation,
+ * so we're better off with braces to make
+ * it obvious that the indentation is correct.
+ */
+ doit();
+ }
+
+ - When there are multiple arms to a conditional and some of them
+ require braces, enclose even a single line block in braces for
+ consistency. E.g.:
+
+ if (foo) {
+ doit();
+ } else {
+ one();
+ two();
+ three();
+ }
- We try to avoid assignments in the condition of an "if" statement.
--
2.11.0.651.g41f4a5c4e
^ permalink raw reply related
* [PATCH] documentation: remove unfinished documentation
From: Stefan Beller @ 2017-01-17 20:01 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
When looking for documentation for a specific function, you may be tempted
to run
git -C Documentation grep index_name_pos
only to find the file technical/api-in-core-index.txt, which doesn't
help for understanding the given function. It would be better to not find
these functions in the documentation, such that people directly dive into
the code instead.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
I run into this a couple of times now, so I put this out tentatively.
Thanks,
Stefan
Documentation/technical/api-in-core-index.txt | 21 ---------------------
1 file changed, 21 deletions(-)
delete mode 100644 Documentation/technical/api-in-core-index.txt
diff --git a/Documentation/technical/api-in-core-index.txt b/Documentation/technical/api-in-core-index.txt
deleted file mode 100644
index adbdbf5d75..0000000000
--- a/Documentation/technical/api-in-core-index.txt
+++ /dev/null
@@ -1,21 +0,0 @@
-in-core index API
-=================
-
-Talk about <read-cache.c> and <cache-tree.c>, things like:
-
-* cache -> the_index macros
-* read_index()
-* write_index()
-* ie_match_stat() and ie_modified(); how they are different and when to
- use which.
-* index_name_pos()
-* remove_index_entry_at()
-* remove_file_from_index()
-* add_file_to_index()
-* add_index_entry()
-* refresh_index()
-* discard_index()
-* cache_tree_invalidate_path()
-* cache_tree_update()
-
-(JC, Linus)
--
2.11.0.297.g298debce27
^ permalink raw reply related
* Re: [PATCH] Documentation/bisect: improve on (bad|new) and (good|bad)
From: Junio C Hamano @ 2017-01-17 19:58 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Christian Couder, git, Manuel Ullmann, Christian Couder
In-Reply-To: <vpqfukjpdnp.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>> But what if bad-A and bad-B have more than one merge bases? We
>> won't know which side the badness came from.
>>
>> o---o---o---bad-A
>> / \ /
>> -----Good---o---o---o /
>> \ / \
>> o---o---o---bad-B
>>
>> Being able to bisect the region of DAG bound by "^Good bad-A bad-B"
>> may have value in such a case. I dunno.
>
> I could help finding several guilty commits, but anyway you can't
> guarantee you'll find them all as soon as you use a binary search: if
> the history looks like
>
> --- Good --- Bad --- Good --- Good --- Bad --- Good --- Bad
>
> then without examining all commits, you can't tell how many good->bad
> switches occured.
>
> But keeping several bad commits wouldn't help keeping the set of
> potentially guilty commits small: bad commits appear on the positive
> side in "^Good bad-A bad-B", so having more bad commits mean having a
> larger DAG to explore (which is a bit counter-intuitive: without
> thinking about it I'd have said "more info => less commits to explore").
>
> So, if finding all guilty commits is not possible, I'm not sure how
> valuable it is to try to find several of them.
The criss-cross merge example, is not trying to find multiple
sources of badness. It still assumes [*1*] that there is only one
event that introduced the badness observed at bad-A and bad-B, both
of which inherited the badness from the same such event. Unlike a
case with a single/unique merge-base, we cannot say "we can start
from the merge-base, as their common badness must be coming from the
same place". The badness may exist in the first 'o' on the same
line as bad-A in the above picture, which is an ancestor of one
merge-base on that line and does not break the other merge base on
the same line as bad-B, for example.
> OTOH, keeping several good commits is needed to find a commit for which
> all parents are good and the commit is bad.
Yes, that is correct.
[Footnote]
*1* The assumption is what makes "bisect" workable. If the
assumption does not hold, then "bisect" would not give a useful
answer "where did I screw up?". It gives a fairly useless "I
found one bad commit whose parent is good---there is no
guarantee if that has anything to do with the badness you are
seeing at the tip".
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Junio C Hamano @ 2017-01-17 19:45 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1701161226090.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
>
> On Sun, 15 Jan 2017, Junio C Hamano wrote:
>
>> * js/prepare-sequencer-more (2017-01-09) 38 commits
>
> I think that it adds confusion rather than value to specifically use a
> different branch name than I indicated in my submission, unless there is a
> really good reason to do so (which I fail to see here).
I've been meaning to rename it to match yours, for the exact reason.
The only reason was I needed my time spent to deal with other
topics, and reusing the same topic name as I used very first time
was less work. I'll find time to update it (if you are curious, it
is not just "git branch -m").
Thanks.
> The only outstanding review comments I know about are your objection to
> the name of the read_env_script() function (which I shot down as bogus),
Not the "name", but the implementation not sharing the same code
with "am" codepath even though they originate from the same shell
function and serve the same purpose.
> and the rather real bug fix I sent out as a fixup! which you may want to
> squash in (in the alternative, I can mailbomb v4 of the entire sequencer-i
> patch series, that is your choice).
I'd rather see the "make elengant" thing redone in a more sensible
way, but I feel that it is waste of my time to help you see the
distinction between maintainable code and code that happens to work
with today's requirement, so I give up, hoping that other people
will later refactor the code that should be shared after the series
lands. I'll squash the fixup! thing in, and as I already said a few
days ago, I do not think we'd want v4 (rather we'd want to go
incremental).
^ permalink raw reply
* Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend
From: Junio C Hamano @ 2017-01-17 19:50 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <alpine.DEB.2.20.1701161129240.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > The original code is:
>> >
>> > . "$author_script"
>>
>> [...]
>>
>> If the code in the sequencer.c reads things other than the three
>> variables we ourselves set, and make them into environment variables
>> and propagate to subprocesses (hooks and editors), it would be a
>> bug. The original did not intend to do that (the dot-sourcing is
>> overly loose than reading three known variables and nothing else,
>> but is OK because we do not support the case where end users muck
>> with the file). Also, writing FOO=BAR alone (not "export FOO=BAR"
>> or "FOO=BAR; export FOO") to the file wouldn't have exported FOO to
>> subprocesses anyway.
>
> That analysis cannot be completely correct, as the GIT_AUTHOR_* variables
> *are* used by the `git commit` subprocess.
In the scripted version, do_with_author shell function explicitly
exports GIT_AUTHOR_* variables because they are the only ones we
care about that are read from existing commit and carried forward
via the author-script mechanism. We do not care about, and in fact
we do not intend to support, any other variables or shell commands
that appear in the author-script.
^ permalink raw reply
* Re: [PATCH] CodingGuidelines: clarify multi-line brace style
From: Junio C Hamano @ 2017-01-17 19:39 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <20170116220821.4tji5mrfcdbdpfuo@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> I think this is pretty clearly the "gray area" mentioned there. Which
>> yes, does not say "definitely do it this way", but I hope makes it clear
>> that you're supposed to use judgement about readability.
>
> So here's a patch.
>
> I know we've usually tried to keep this file to guidelines and not
> rules, but clearly it has not been clear-cut enough in this instance.
I have two "Huh?" with this patch.
1. Two exceptions are not treated the same way. The first one is
"... extends over a few lines, it is excempt from the rule,
period". The second one, is ambivalent by saying "it can make
sense", implying that "it may not make sense", so I am not sure
if this is clarifying that much.
If we want to clarify, perhaps drop "it can make sense to" and
say
When there are multiple arms to a conditional, and some of
them require braces, enclose even a single line block in
braces for consistency.
perhaps?
2. I actually think it is OK to leave some things "gray", but the
confusion comes when people do not know what to do to things
that are "gray", and they need a rule for that to be spelled
out.
When the project says it does not have strong preference
among multiple choices, you are welcome to write your new
code using any of them, as long as you are consistent with
surrounding code. Do not change style of existing code only
to flip among these styles, though.
That obviously is not limited to the rule/guideline for braces.
> -- >8 --
> Subject: [PATCH] CodingGuidelines: clarify multi-line brace style
>
> There are some "gray areas" around when to omit braces from
> a conditional or loop body. Since that seems to have
> resulted in some arguments, let's be a little more clear
> about our preferred style.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Documentation/CodingGuidelines | 37 ++++++++++++++++++++++++++++++++-----
> 1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 4cd95da6b..0e336e99d 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -206,11 +206,38 @@ For C programs:
> x = 1;
> }
>
> - is frowned upon. A gray area is when the statement extends
> - over a few lines, and/or you have a lengthy comment atop of
> - it. Also, like in the Linux kernel, if there is a long list
> - of "else if" statements, it can make sense to add braces to
> - single line blocks.
> + is frowned upon. But there are a few exceptions:
> +
> + - When the statement extends over a few lines (e.g., a while loop
> + with an embedded conditional, or a comment). E.g.:
> +
> + while (foo) {
> + if (x)
> + one();
> + else
> + two();
> + }
> +
> + if (foo) {
> + /*
> + * This one requires some explanation,
> + * so we're better off with braces to make
> + * it obvious that the indentation is correct.
> + */
> + doit();
> + }
> +
> + - When there are multiple arms to a conditional, it can make
> + sense to add braces to single line blocks for consistency.
> + E.g.:
> +
> + if (foo) {
> + doit();
> + } else {
> + one();
> + two();
> + three();
> + }
>
> - We try to avoid assignments in the condition of an "if" statement.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox