* Re: sparse checkout - weird behavior
From: Paul Hammant @ 2017-01-26 3:21 UTC (permalink / raw)
To: git
Related bug (maybe the same). Reproduction:
$ git clone git@github.com:jekyll/jekyll.git --no-checkout
Cloning into 'jekyll'...
remote: Counting objects: 41331, done.
remote: Compressing objects: 100% (5/5), done.
remote: Total 41331 (delta 0), reused 0 (delta 0), pack-reused 41326
Receiving objects: 100% (41331/41331), 11.91 MiB | 9.15 MiB/s, done.
Resolving deltas: 100% (26530/26530), done.
$ cd jekyll
$ git config core.sparsecheckout true
$ echo 'CONDUCT.markdown' > .git/info/sparse-checkout
$ echo 'Gemfile' >> .git/info/sparse-checkout
$ echo 'Rakefile' >> .git/info/sparse-checkout
$ echo 'appveyor.yml' >> .git/info/sparse-checkout
$ git checkout --
Your branch is up-to-date with 'origin/master'.
$ ls
CONDUCT.markdown Gemfile Rakefile appveyor.yml lib
I was not expecting to see 'lib' in the resulting file list
I didn't say so before - I'm on a Mac, with a homebrew installed Git 2.11.0
- Paul
^ permalink raw reply
* Re: [PATCH] refs: add option core.logAllRefUpdates = always
From: Jeff King @ 2017-01-26 3:35 UTC (permalink / raw)
To: cornelius.weig; +Cc: gitster, git, novalis, pclouds
In-Reply-To: <20170126011654.21729-2-cornelius.weig@tngtech.com>
On Thu, Jan 26, 2017 at 02:16:54AM +0100, cornelius.weig@tngtech.com wrote:
> From: Cornelius Weig <cornelius.weig@tngtech.com>
>
> When core.logallrefupdates is true, we only create a new reflog for refs
> that are under certain well-known hierarchies. The reason is that we
> know that some hierarchies (like refs/tags) do not typically change, and
> that unknown hierarchies might not want reflogs at all (e.g., a
> hypothetical refs/foo might be meant to change often and drop old
> history immediately).
I tried to read this patch with fresh eyes. But given the history, you
may take my review with a grain of salt. :)
Overall it looks OK to me. A few comments below.
> This patch introduces a new "always" mode for the core.logallrefupdates
> option which will log updates to everything under refs/, regardless
> where in the hierarchy it is (we still will not log things like
> ORIG_HEAD and FETCH_HEAD, which are known to be transient).
I don't think my original had tests for this, but it might be worth
adding a test for this last bit (i.e., that an update of ORIG_HEAD does
not write a reflog when logallrefupdates is set to "always").
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index af2ae4c..2117616 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -517,10 +517,13 @@ core.logAllRefUpdates::
> "`$GIT_DIR/logs/<ref>`", by appending the new and old
> SHA-1, the date/time and the reason of the update, but
> only when the file exists. If this configuration
> - variable is set to true, missing "`$GIT_DIR/logs/<ref>`"
> + variable is set to `true`, missing "`$GIT_DIR/logs/<ref>`"
> file is automatically created for branch heads (i.e. under
> refs/heads/), remote refs (i.e. under refs/remotes/),
> - note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
> + `refs/heads/`), remote refs (i.e. under `refs/remotes/`),
> + note refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.
> + If it is set to `always`, then a missing reflog is automatically
> + created for any ref under `refs/`.
I guess the backtick fixups came from my original. It might be easier to
see the change if they were pulled into their own patch, but it's
probably not that big a deal.
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -150,7 +150,8 @@ This option is only applicable when listing tags without annotation lines.
> 'strip' removes both whitespace and commentary.
>
> --create-reflog::
> - Create a reflog for the tag.
> + Create a reflog for the tag. To globally enable reflogs for tags, see
> + `core.logAllRefUpdates` in linkgit:git-config[1].
This documentation tweak makes sense to me.
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 76d68fa..1d4d6a0 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -262,7 +262,7 @@ static int create_default_files(const char *template_path,
> const char *work_tree = get_git_work_tree();
> git_config_set("core.bare", "false");
> /* allow template config file to override the default */
> - if (log_all_ref_updates == -1)
> + if (log_all_ref_updates == LOG_REFS_UNSET)
> git_config_set("core.logallrefupdates", "true");
> if (needs_work_tree_config(original_git_dir, work_tree))
> git_config_set("core.worktree", work_tree);
I expected that this hunk would need tweaked due to refactoring around
init-db that happened earlier this year. But it seems fine.
> diff --git a/refs.c b/refs.c
> index 9bd0bc1..cd36b64 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -638,12 +638,17 @@ int copy_reflog_msg(char *buf, const char *msg)
>
> int should_autocreate_reflog(const char *refname)
> {
> - if (!log_all_ref_updates)
> + switch (log_all_ref_updates) {
> + case LOG_REFS_ALWAYS:
> + return 1;
> + case LOG_REFS_NORMAL:
> + return starts_with(refname, "refs/heads/") ||
> + starts_with(refname, "refs/remotes/") ||
> + starts_with(refname, "refs/notes/") ||
> + !strcmp(refname, "HEAD");
> + default:
> return 0;
> - return starts_with(refname, "refs/heads/") ||
> - starts_with(refname, "refs/remotes/") ||
> - starts_with(refname, "refs/notes/") ||
> - !strcmp(refname, "HEAD");
> + }
> }
And this function got broken out already by David in an earlier patch.
Looks good.
> @@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
> {
> int logfd, result, oflags = O_APPEND | O_WRONLY;
>
> - if (log_all_ref_updates < 0)
> - log_all_ref_updates = !is_bare_repository();
> + if (log_all_ref_updates == LOG_REFS_UNSET)
> + log_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
This hunk is new, I think. The enum values are set in such a way that
the original code would have continued to work, but I think using the
symbolic names is an improvement.
I assume you grepped for log_all_ref_updates to find this. I see only
one spot that now doesn't use the symbolic names. In builtin/checkout.c,
update_refs_for_switch() checks:
if (opts->new_branch_log && !log_all_ref_updates)
That looks buggy, as it would treat LOG_REFS_NORMAL and LOG_REFS_UNSET
the same, and I do not see us resolving the UNSET case to a true/false
value. But I don't think the bug is new in your patch; the default value
was "-1" already.
I doubt it can be triggered in practice, because either:
- the config value is set in the config file, and we pick up that
value, whether it's "true" or "false"
- it's unset, in which case our default would be to enable reflogs in
a non-bare repo. And since git-checkout would refuse to run in a
bare repo, we must be non-bare, and thus enabling reflogs does the
right thing.
But it works quite by accident. I wonder if we should this
"is_bare_repository" check into a function that can be called instead of
accessing log_all_ref_updates() directly.
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -93,6 +93,36 @@ test_expect_success 'update-ref creates reflogs with --create-reflog' '
> git reflog exists $outside
> '
>
> +test_expect_success 'core.logAllRefUpdates=true does not create reflog by default' '
> + test_config core.logAllRefUpdates true &&
> + test_when_finished "git update-ref -d $outside" &&
> + git update-ref $outside $A &&
> + git rev-parse $A >expect &&
> + git rev-parse $outside >actual &&
> + test_cmp expect actual &&
> + test_must_fail git reflog exists $outside
> +'
> +
> +test_expect_success 'core.logAllRefUpdates=always creates reflog by default' '
> + test_config core.logAllRefUpdates always &&
> + test_when_finished "git update-ref -d $outside" &&
> + git update-ref $outside $A &&
> + git rev-parse $A >expect &&
> + git rev-parse $outside >actual &&
> + test_cmp expect actual &&
> + git reflog exists $outside
> +'
Adding the tests to the existing --create-reflog tests is a good choice.
> +test_expect_success 'update-ref does not create reflog with --no-create-reflog if core.logAllRefUpdates=always' '
This test title is _really_ long, and will wrap in the output on
reasonable-sized terminals. Maybe '--no-create-reflog overrides
core.logAllRefUpdates=always' would be shorter?
> test_expect_success 'stdin creates reflogs with --create-reflog' '
> + test_when_finished "git update-ref -d $outside" &&
> echo "create $outside $m" >stdin &&
> git update-ref --create-reflog --stdin <stdin &&
> git rev-parse $m >expect &&
Adding missing cleanup. Good.
> +test_expect_success 'stdin does not create reflog when core.logAllRefUpdates=true' '
I don't mind these extra stdin tests, but IMHO they are just redundant.
The "--stdin --create-reflog" one makes sure the option is propagated
down via the --stdin machinery. But we know the config option is handled
at a low level anyway.
I guess it depends on how black-box we want the testing to be. It just
seems unlikely for a regression to be found here and not in the tests
above.
-Peff
^ permalink raw reply
* Re: [PATCH] Documentation: implement linkgit macro for Asciidoctor
From: Jeff King @ 2017-01-26 3:46 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Johannes Schindelin, Øyvind A. Holm
In-Reply-To: <20170126001344.445534-1-sandals@crustytoothpaste.net>
On Thu, Jan 26, 2017 at 12:13:44AM +0000, brian m. carlson wrote:
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 19c42eb60..d1b7a6865 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -179,10 +179,7 @@ ASCIIDOC = asciidoctor
> ASCIIDOC_CONF =
> ASCIIDOC_HTML = xhtml5
> ASCIIDOC_DOCBOOK = docbook45
> -ifdef ASCIIDOCTOR_EXTENSIONS_LAB
> -ASCIIDOC_EXTRA = -I$(ASCIIDOCTOR_EXTENSIONS_LAB) -rasciidoctor/extensions -rman-inline-macro
> -endif
> -ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;'
> +ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions -alitdd='&\#x2d;&\#x2d;'
Might be more readable to just leave the litdd part on its own line.
> diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
> new file mode 100644
> index 000000000..09f7088ee
> --- /dev/null
> +++ b/Documentation/asciidoctor-extensions.rb
> @@ -0,0 +1,28 @@
> +require 'asciidoctor'
> +require 'asciidoctor/extensions'
> +
> +module Git
> + module Documentation
> + class LinkGitProcessor < Asciidoctor::Extensions::InlineMacroProcessor
> + use_dsl
> +
> + named :chrome
> +
> + def process(parent, target, attrs)
> + if parent.document.basebackend? 'html'
> + prefix = parent.document.attr('git-relative-html-prefix')
> + %(<a href="#{prefix}#{target}.html">#{target}(#{attrs[1]})</a>\n)
> + elsif parent.document.basebackend? 'docbook'
> + %(<citerefentry>
> +<refentrytitle>#{target}</refentrytitle><manvolnum>#{attrs[1]}</manvolnum>
> +</citerefentry>
> +)
> + end
> + end
> + end
> + end
> +end
I think this looks reasonable. There's some boilerplate, but even as
somebody not familiar with asciidoctor, it's all quite obvious.
The multi-line string is kind of ugly because of the indentation.
Apparently Ruby has here-docs that will eat leading whitespace, but the
syntax was not introduce until Ruby 2.3, which is probably more recent
than we should count on.
I think you could write:
%(<citerefentry>
<refentrytitle>#{target}</refentrytitle><manvolnum>#{attrs[1]}</manvolnum>
</citerefentry>
).gsub(/^\s*/, "")
I don't know if that's too clever or not.
But either way, I like this better than introducing an extra dependency.
-Peff
^ permalink raw reply
* [PATCH 0/2] (re-)optimizing fsck --connectivity-only
From: Jeff King @ 2017-01-26 4:10 UTC (permalink / raw)
To: git
I've been playing around with the newly-fixed `fsck --connectivity-only`.
While it's much faster than it was, I was sad to find a case that is
still much slower than "git rev-list --objects --all".
This goes on top of my origin/jk/fsck-connectivity-check-fix, and gives
a noticeable speedup. IMHO it's worth considering as part of the same
topic (which is currently in 'next').
[1/2]: fsck: move typename() printing to its own function
[2/2]: fsck: lazily load types under --connectivity-only
builtin/fsck.c | 87 ++++++++++++++++++----------------------------------------
fsck.c | 4 +++
2 files changed, 31 insertions(+), 60 deletions(-)
-Peff
^ permalink raw reply
* [PATCH 1/2] fsck: move typename() printing to its own function
From: Jeff King @ 2017-01-26 4:11 UTC (permalink / raw)
To: git
In-Reply-To: <20170126041025.hqg3znwew7jxgxxg@sigill.intra.peff.net>
When an object has a problem, we mention its type. But we do
so by feeding the result of typename() directly to
fprintf(). This is potentially dangerous because typename()
can return NULL for some type values (like OBJ_NONE).
It's doubtful that this can be triggered in practice with
the current code, so this is probably not fixing a bug. But
it future-proofs us against modifications that make things
like OBJ_NONE more likely (and gives future patches a
central point to handle them).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fsck.c | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 57f529b41..3d5ced2d3 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -56,6 +56,17 @@ static const char *describe_object(struct object *obj)
return buf.buf;
}
+static const char *printable_type(struct object *obj)
+{
+ const char *ret;
+
+ ret = typename(obj->type);
+ if (!ret)
+ ret = "unknown";
+
+ return ret;
+}
+
static int fsck_config(const char *var, const char *value, void *cb)
{
if (strcmp(var, "fsck.skiplist") == 0) {
@@ -83,7 +94,7 @@ static void objreport(struct object *obj, const char *msg_type,
const char *err)
{
fprintf(stderr, "%s in %s %s: %s\n",
- msg_type, typename(obj->type), describe_object(obj), err);
+ msg_type, printable_type(obj), describe_object(obj), err);
}
static int objerror(struct object *obj, const char *err)
@@ -114,7 +125,7 @@ static int mark_object(struct object *obj, int type, void *data, struct fsck_opt
if (!obj) {
/* ... these references to parent->fld are safe here */
printf("broken link from %7s %s\n",
- typename(parent->type), describe_object(parent));
+ printable_type(parent), describe_object(parent));
printf("broken link from %7s %s\n",
(type == OBJ_ANY ? "unknown" : typename(type)), "unknown");
errors_found |= ERROR_REACHABLE;
@@ -131,9 +142,9 @@ static int mark_object(struct object *obj, int type, void *data, struct fsck_opt
if (!(obj->flags & HAS_OBJ)) {
if (parent && !has_object_file(&obj->oid)) {
printf("broken link from %7s %s\n",
- typename(parent->type), describe_object(parent));
+ printable_type(parent), describe_object(parent));
printf(" to %7s %s\n",
- typename(obj->type), describe_object(obj));
+ printable_type(obj), describe_object(obj));
errors_found |= ERROR_REACHABLE;
}
return 1;
@@ -205,7 +216,7 @@ 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 */
- printf("missing %s %s\n", typename(obj->type),
+ printf("missing %s %s\n", printable_type(obj),
describe_object(obj));
errors_found |= ERROR_REACHABLE;
return;
@@ -231,7 +242,7 @@ static void check_unreachable_object(struct object *obj)
* since this is something that is prunable.
*/
if (show_unreachable) {
- printf("unreachable %s %s\n", typename(obj->type),
+ printf("unreachable %s %s\n", printable_type(obj),
describe_object(obj));
return;
}
@@ -250,7 +261,7 @@ static void check_unreachable_object(struct object *obj)
*/
if (!obj->used) {
if (show_dangling)
- printf("dangling %s %s\n", typename(obj->type),
+ printf("dangling %s %s\n", printable_type(obj),
describe_object(obj));
if (write_lost_and_found) {
char *filename = git_pathdup("lost-found/%s/%s",
@@ -324,7 +335,7 @@ static int fsck_obj(struct object *obj)
if (verbose)
fprintf(stderr, "Checking %s %s\n",
- typename(obj->type), describe_object(obj));
+ printable_type(obj), describe_object(obj));
if (fsck_walk(obj, NULL, &fsck_obj_options))
objerror(obj, "broken links");
@@ -350,7 +361,7 @@ static int fsck_obj(struct object *obj)
struct tag *tag = (struct tag *) obj;
if (show_tags && tag->tagged) {
- printf("tagged %s %s", typename(tag->tagged->type),
+ printf("tagged %s %s", printable_type(tag->tagged),
describe_object(tag->tagged));
printf(" (%s) in %s\n", tag->tag,
describe_object(&tag->object));
--
2.11.0.840.gd37c5973a
^ permalink raw reply related
* [PATCH 2/2] fsck: lazily load types under --connectivity-only
From: Jeff King @ 2017-01-26 4:12 UTC (permalink / raw)
To: git
In-Reply-To: <20170126041025.hqg3znwew7jxgxxg@sigill.intra.peff.net>
The recent fixes to "fsck --connectivity-only" load all of
the objects with their correct types. This keeps the
connectivity-only code path close to the regular one, but it
also introduces some unnecessary inefficiency. While getting
the type of an object is cheap compared to actually opening
and parsing the object (as the non-connectivity-only case
would do), it's still not free.
For reachable non-blob objects, we end up having to parse
them later anyway (to see what they point to), making our
type lookup here redundant.
For unreachable objects, we might never hit them at all in
the reachability traversal, making the lookup completely
wasted. And in some cases, we might have quite a few
unreachable objects (e.g., when alternates are used for
shared object storage between repositories, it's normal for
there to be objects reachable from other repositories but
not the one running fsck).
The comment in mark_object_for_connectivity() claims two
benefits to getting the type up front:
1. We need to know the types during fsck_walk(). (And not
explicitly mentioned, but we also need them when
printing the types of broken or dangling commits).
We can address this by lazy-loading the types as
necessary. Most objects never need this lazy-load at
all, because they fall into one of these categories:
a. Reachable from our tips, and are coerced into the
correct type as we traverse (e.g., a parent link
will call lookup_commit(), which converts OBJ_NONE
to OBJ_COMMIT).
b. Unreachable, but not at the tip of a chunk of
unreachable history. We only mention the tips as
"dangling", so an unreachable commit which links
to hundreds of other objects needs only report the
type of the tip commit.
2. It serves as a cross-check that the coercion in (1a) is
correct (i.e., we'll complain about a parent link that
points to a blob). But we get most of this for free
already, because right after coercing, we'll parse any
non-blob objects. So we'd notice then if we expected a
commit and got a blob.
The one exception is when we expect a blob, in which
case we never actually read the object contents.
So this is a slight weakening, but given that the whole
point of --connectivity-only is to sacrifice some data
integrity checks for speed, this seems like an
acceptable tradeoff.
Here are before and after timings for an extreme case with
~5M reachable objects and another ~12M unreachable (it's the
torvalds/linux repository on GitHub, connected to shared
storage for all of the other kernel forks):
[before]
$ time git fsck --no-dangling --connectivity-only
real 3m4.323s
user 1m25.121s
sys 1m38.710s
[after]
$ time git fsck --no-dangling --connectivity-only
real 0m51.497s
user 0m49.575s
sys 0m1.776s
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fsck.c | 58 +++++++---------------------------------------------------
fsck.c | 4 ++++
2 files changed, 11 insertions(+), 51 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 3d5ced2d3..140357b6f 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -60,6 +60,12 @@ static const char *printable_type(struct object *obj)
{
const char *ret;
+ if (obj->type == OBJ_NONE) {
+ enum object_type type = sha1_object_info(obj->oid.hash, NULL);
+ if (type > 0)
+ object_as_type(obj, type, 0);
+ }
+
ret = typename(obj->type);
if (!ret)
ret = "unknown";
@@ -595,57 +601,7 @@ static int fsck_cache_tree(struct cache_tree *it)
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;
- }
- }
-
+ struct object *obj = lookup_unknown_object(sha1);
obj->flags |= HAS_OBJ;
}
diff --git a/fsck.c b/fsck.c
index 4a3069e20..939792752 100644
--- a/fsck.c
+++ b/fsck.c
@@ -458,6 +458,10 @@ int fsck_walk(struct object *obj, void *data, struct fsck_options *options)
{
if (!obj)
return -1;
+
+ if (obj->type == OBJ_NONE)
+ parse_object(obj->oid.hash);
+
switch (obj->type) {
case OBJ_BLOB:
return 0;
--
2.11.0.840.gd37c5973a
^ permalink raw reply related
* Re: sparse checkout - weird behavior
From: Jeff King @ 2017-01-26 4:57 UTC (permalink / raw)
To: Paul Hammant; +Cc: git
In-Reply-To: <CA+298Uh=bfCJq3hmVdGUsinAgKFQd86em_J_8fwB9jQR5PZVgQ@mail.gmail.com>
On Wed, Jan 25, 2017 at 09:59:38PM -0500, Paul Hammant wrote:
> Here's a simple reproducible bug - something unexpected in sparse-checkout mode:
>
> $ git clone git@github.com:jekyll/jekyll.git --no-checkout
> Cloning into 'jekyll'...
> remote: Counting objects: 41331, done.
> remote: Compressing objects: 100% (5/5), done.
> remote: Total 41331 (delta 0), reused 0 (delta 0), pack-reused 41326
> Receiving objects: 100% (41331/41331), 11.91 MiB | 7.98 MiB/s, done.
> Resolving deltas: 100% (26530/26530), done.
> $ cd jekyll
> $ ls
> $ git config core.sparsecheckout true
> $ echo 'docs*' > .git/info/sparse-checkout
> $ git read-tree -mu HEAD
> $ ls
> docs rake
>
> I didn't expect to see 'rake' amongst the results.
If you look inside the rake/ directory, you should see that only
"docs.rake" was checked out.
The sparse-checkout file uses the same parser as .git/info/exclude. One
important aspect of that file is that entries are _not_ left-anchored
unless they start with "/". So you asked Git to include files named
"docs*" anywhere in the tree.
You probably wanted just:
echo /docs >.git/info/sparse-checkout
-Peff
^ permalink raw reply
* Re: sparse checkout - weird behavior
From: Jeff King @ 2017-01-26 4:59 UTC (permalink / raw)
To: Paul Hammant; +Cc: git
In-Reply-To: <CA+298Ujx2wH2WnoYiOaWKoneBrF_E5VUXXSMqecGgNLYS0Wemg@mail.gmail.com>
On Wed, Jan 25, 2017 at 10:21:19PM -0500, Paul Hammant wrote:
> Related bug (maybe the same). Reproduction:
>
> $ git clone git@github.com:jekyll/jekyll.git --no-checkout
> Cloning into 'jekyll'...
> remote: Counting objects: 41331, done.
> remote: Compressing objects: 100% (5/5), done.
> remote: Total 41331 (delta 0), reused 0 (delta 0), pack-reused 41326
> Receiving objects: 100% (41331/41331), 11.91 MiB | 9.15 MiB/s, done.
> Resolving deltas: 100% (26530/26530), done.
> $ cd jekyll
> $ git config core.sparsecheckout true
> $ echo 'CONDUCT.markdown' > .git/info/sparse-checkout
> $ echo 'Gemfile' >> .git/info/sparse-checkout
> $ echo 'Rakefile' >> .git/info/sparse-checkout
> $ echo 'appveyor.yml' >> .git/info/sparse-checkout
> $ git checkout --
> Your branch is up-to-date with 'origin/master'.
> $ ls
> CONDUCT.markdown Gemfile Rakefile appveyor.yml lib
>
> I was not expecting to see 'lib' in the resulting file list
Yep, I think this is the same problem. Inside lib, you get only
"lib/theme_template/Gemfile", because it matches your unanchored
pattern. Using "/Gemfile" in the sparse-checkout file fixes it.
-Peff
^ permalink raw reply
* Re: Force Confirmation for Dropping Changed Lines
From: Jacob Keller @ 2017-01-26 5:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Hilco Wijbenga, Git Users
In-Reply-To: <xmqqd1fa7dqf.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 25, 2017 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Where did you get that "unset" from? If that is this paragraph in
> Documentation/gitattributes.txt:
>
Ok so that whole section of documentation is very confusing to me.
Maybe it could be improved for more readability. I'll see if I can't
help clear up some of my confusion.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] gpg-interface: Add some output from gpg when it errors out.
From: Jeff King @ 2017-01-26 5:21 UTC (permalink / raw)
To: Mike Hommey; +Cc: Junio C Hamano, git
In-Reply-To: <20170125235410.byxwmo7o7zdszzot@glandium.org>
On Thu, Jan 26, 2017 at 08:54:10AM +0900, Mike Hommey wrote:
> > Implementation-wise, I'd be happier if we do not add any new
> > callsites of strbuf_split(), which is a horrible interface. But
> > that is a minor detail.
>
> What would you suggest otherwise?
Try string_list_split() (or its in_place() variant, since it is probably
OK to hack up the string for your use case). Like this:
diff --git a/gpg-interface.c b/gpg-interface.c
index 2768bb307..051bb7d3e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -158,14 +158,16 @@ static int pipe_gpg_command(struct child_process *cmd,
/* Print out any line that doesn't start with [GNUPG:] if the gpg
* command failed. */
if (ret) {
- struct strbuf **err_lines = strbuf_split(err, '\n');
- for (struct strbuf **line = err_lines; *line; line++) {
- if (memcmp((*line)->buf, "[GNUPG:]", 8)) {
- strbuf_rtrim(*line);
- fprintf(stderr, "%s\n", (*line)->buf);
- }
+ struct string_list lines = STRING_LIST_INIT_NODUP;
+ int i;
+
+ string_list_split_in_place(&lines, err->buf, '\n', 0);
+ for (i = 0; i < lines.nr; i++) {
+ const char *line = lines.items[i].string;
+ if (!starts_with(line, "[GNUPG:]"))
+ fprintf(stderr, "%s\n", line);
}
- strbuf_list_free(err_lines);
+ string_list_clear(&lines, 0);
}
return ret;
}
Note that I also replaced the memcmp with starts_with(). That avoids the
magic number "8". I also suspect your original can read off the end of
the buffer when the line is shorter than 8 characters (e.g., if memcmp
does 64-bit loads).
-Peff
^ permalink raw reply related
* Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Johannes Sixt @ 2017-01-26 6:39 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Johannes Schindelin, David Aguilar, Ramsay Jones,
GIT Mailing-list
In-Reply-To: <20170125220101.et67ebkumsqosaku@sigill.intra.peff.net>
Am 25.01.2017 um 23:01 schrieb Jeff King:
> +#pragma GCC diagnostic ignored "-Wformat-zero-length"
Last time I used #pragma GCC in a cross-platform project, it triggered
an "unknown pragma" warning for MSVC. (It was the C++ compiler, I don't
know if the C compiler would also warn.) It would have to be spelled
like this:
#pragma warning(disable: 4068) /* MSVC: unknown pragma */
#pragma GCC diagnostic ignored "-Wformat-zero-length"
Dscho mentioned that he's compiling with MSVC. It would do him a favor.
-- Hannes
^ permalink raw reply
* Re: [PATCH] Documentation: implement linkgit macro for Asciidoctor
From: Eric Wong @ 2017-01-26 7:43 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, git, Johannes Schindelin, Øyvind A. Holm
In-Reply-To: <20170126034655.fwzow2mgkjj5dpek@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> On Thu, Jan 26, 2017 at 12:13:44AM +0000, brian m. carlson wrote:
> > +
> > + def process(parent, target, attrs)
> > + if parent.document.basebackend? 'html'
> > + prefix = parent.document.attr('git-relative-html-prefix')
> > + %(<a href="#{prefix}#{target}.html">#{target}(#{attrs[1]})</a>\n)
> > + elsif parent.document.basebackend? 'docbook'
> > + %(<citerefentry>
> > +<refentrytitle>#{target}</refentrytitle><manvolnum>#{attrs[1]}</manvolnum>
> > +</citerefentry>
> > +)
<snip>
> The multi-line string is kind of ugly because of the indentation.
> Apparently Ruby has here-docs that will eat leading whitespace, but the
> syntax was not introduce until Ruby 2.3, which is probably more recent
> than we should count on.
You can use '\' to continue long lines with any Ruby version:
"<citerefentry>" \
"<refentrytitle>#{target}</refentrytitle>" \
"<manvolnum>#{attrs[1]}</manvolnum>" \
"</citerefentry>"
The above happens during the parse phase, so there's no garbage
or method call overhead compared to the more-frequently seen '+'
or '<<' method calls to combine strings.
> I think you could write:
>
> %(<citerefentry>
> <refentrytitle>#{target}</refentrytitle><manvolnum>#{attrs[1]}</manvolnum>
> </citerefentry>
> ).gsub(/^\s*/, "")
>
> I don't know if that's too clever or not.
Ick...
> But either way, I like this better than introducing an extra dependency.
Agreed.
^ permalink raw reply
* Re: [PATCH 2/5] revision.c: group ref selection options together
From: Duy Nguyen @ 2017-01-26 9:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Jeff King, Jacob Keller
In-Reply-To: <xmqqo9yualqn.fsf@gitster.mtv.corp.google.com>
On Thu, Jan 26, 2017 at 4:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> * I am expecting that the new one yet to be introduced will not
> share the huge "switch (selector)" part, but does its own things
> in a separate function with a similar structure. The only thing
> common between these two functions would be the structure
> (i.e. it has a big "switch(selector)" that does different things
> depending on REF_SELECT_*) and a call to clear_* function.
Yep. The "new one" is demonstrated in 5/5.
> If we were to add a new kind of REF_SELECT_* (say
> REF_SELECT_NOTES just for the sake of being concrete), what
> changes will be needed to the code if the addition of "use reflog
> from this class of refs for decoration" feature was done with or
> without this step? I have a suspicion that the change will be
> simpler without this step.
The switch/case is to deal with new REF_SELECT_* (at least it's how I
imagine it). What I was worried about was, when a user adds
--select-notes, they may not be aware that it's in the same
all/branches/tags/remotes group that's supposed to work with
--decorate-reflog as well, and as a result "--decorate-reflog
--select-notes" is the same as "--select-notes".
With the switch/case, when you add a new enum item, at the least the
compiler should warn about unhandled cases. And we can have a new
"case REF_SELECT_NOTES:" for both --exclude and --decorate-reflog.
Without the switch/case, I guess it's still possible to do something
like
if (!strcmp(arg, "--select-notes")) {
if (preceded_by_exclude())
does_one_thing();
else if (preceded_by_decorate_reflog())
does_another_thing();
}
It's probably easier to maintain though, if all
decorate-reflog-related things are grouped together, rather than
spread out per option like the above.
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
From: Lars Schneider @ 2017-01-26 9:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git
In-Reply-To: <xmqq7f5i92jk.fsf@gitster.mtv.corp.google.com>
> On 25 Jan 2017, at 23:51, Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
>> I guess the way to dig would be to add a test that looks at the output
>> of "type mv" or something, push it to a Travis-hooked branch, and then
>> wait for the output
>
> Sounds tempting ;-)
Well, I tried that:
mv is /bin/mv
... and "/bin/mv" is exactly the version that I have on my machine.
The difference between Travis and my machine is that I changed the
default shell to ZSH with a few plugins [1]. If I run the test with
plain BASH on my Mac then I can reproduce the test failure. Therefore,
we might want to adjust the commit message if anyone else can reproduce
the problem on a Mac.
I can even reproduce the failure if I run the test with plain ZSH.
However, I can't find a plugin that defines an alias for "mv". Puzzled...
- Lars
[1] https://github.com/robbyrussell/oh-my-zsh
^ permalink raw reply
* Re: [PATCH 2/5] revision.c: group ref selection options together
From: Duy Nguyen @ 2017-01-26 9:18 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Jacob Keller
In-Reply-To: <20170125205037.cg3aebh5rsx4zb2l@sigill.intra.peff.net>
On Thu, Jan 26, 2017 at 3:50 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 25, 2017 at 07:50:51PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> These options have on thing in common: when specified right after
>> --exclude, they will de-select refs instead of selecting them by
>> default.
>>
>> This change makes it possible to introduce new options that use these
>> options in the same way as --exclude. Such an option would just
>> implement something like handle_refs_pseudo_opt().
>>
>> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
>> that similar functions like handle_refs_pseudo_opt() are forced to
>> handle all ref selector options, not skipping some by mistake, which may
>> revert the option back to default behavior (rev selection).
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> revision.c | 134 +++++++++++++++++++++++++++++++++++++++++++++----------------
>> 1 file changed, 100 insertions(+), 34 deletions(-)
>
> Hmm. I see what you're trying to do here, and abstract the repeated
> bits. But I'm not sure the line-count reflects a real simplification.
> Everything ends up converted to an enum, and then that enum just expands
> to similar C code.
It's not simplification, but hopefully for better maintainability. This
if (strcmp(arg, "--remotes")) {
if (preceded_by_exclide())
does_something();
else if (preceded_by_decorate())
does_another()
} else if (strcmp(arg, "--branches")) {
if (preceded_by_exclide())
does_something();
else if (preceded_by_decorate())
does_another()
}
starts to look ugly especially when the third "preceded_by_" comes
into picture. Putting all "does_something" in one group and
"does_another" in another, I think, gives us a better view how ref
selection is handled for a specific operation like --exclude or
--decorate-ref.
> I kind of expected that clear_ref_exclusion() would just become a more
> abstract clear_ref_selection(). For now it would clear exclusions, and
> then later learn to clear the decoration flags.
It may go that way, depending on how we handle these options for
decorate-reflog. The current load_ref_decorations() is not really
suited for fine-grained ref selection yet.
--
Duy
^ permalink raw reply
* Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude
From: Duy Nguyen @ 2017-01-26 9:28 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Jacob Keller
In-Reply-To: <20170125205718.ksqstdnazmgbkehy@sigill.intra.peff.net>
On Thu, Jan 26, 2017 at 3:57 AM, Jeff King <peff@peff.net> wrote:
> I don't think it means either. It means to include remotes in the
> selected revisions, but excluding the entries mentioned by --exclude.
>
> IOW:
>
> --exclude=foo --remotes
> include all remotes except refs/remotes/foo
>
> --exclude=foo --unrelated --remotes
> same
>
> --exclude=foo --decorate-reflog --remotes
> decorate reflogs of all remotes except "foo". Do _not_ use them
> as traversal tips.
>
> --decorate-reflog --exclude=foo --remotes
> same
>
> IOW, the ref-selector options build up until a group option is given,
> which acts on the built-up options (over that group) and then resets the
> built-up options. Doing "--unrelated" as above is orthogonal (though I
> think in practice nobody would do that, because it's hard to read).
This is because it makes sense to combine --exclude and
--decorate-reflog. But what about a new --something that conflicts
with either --exclude or --decorate-reflog? Should we simply catch
such combinations and error out (which may be a bit more complicated
than this patch, or maybe not)?
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
From: Lars Schneider @ 2017-01-26 9:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git
In-Reply-To: <D9F0976B-9F78-44BE-B9DD-CAB6506FA3A9@gmail.com>
> On 26 Jan 2017, at 10:14, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>
>> On 25 Jan 2017, at 23:51, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Jeff King <peff@peff.net> writes:
>>
>>> I guess the way to dig would be to add a test that looks at the output
>>> of "type mv" or something, push it to a Travis-hooked branch, and then
>>> wait for the output
>>
>> Sounds tempting ;-)
>
> Well, I tried that:
>
> mv is /bin/mv
>
> ... and "/bin/mv" is exactly the version that I have on my machine.
>
> The difference between Travis and my machine is that I changed the
> default shell to ZSH with a few plugins [1]. If I run the test with
> plain BASH on my Mac then I can reproduce the test failure. Therefore,
> we might want to adjust the commit message if anyone else can reproduce
> the problem on a Mac.
>
> I can even reproduce the failure if I run the test with plain ZSH.
> However, I can't find a plugin that defines an alias for "mv". Puzzled...
>
> - Lars
>
> [1] https://github.com/robbyrussell/oh-my-zsh
Oh. I must have made a mistake on my very first test run. I can reproduce
the failure with ZSH and my plugins... looks like it's a Mac OS problem
and no TravisCI only problem after all.
Sorry for the noise/confusion,
Lars
^ permalink raw reply
* Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Johannes Schindelin @ 2017-01-26 11:16 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, David Aguilar, Ramsay Jones, GIT Mailing-list
In-Reply-To: <20170125183542.pe5qolexqqx6jhsi@sigill.intra.peff.net>
Hi Peff,
On Wed, 25 Jan 2017, Jeff King wrote:
> On Wed, Jan 25, 2017 at 11:36:50AM +0100, Johannes Schindelin wrote:
>
> > > Gross, but at least it's self documenting. :)
> > >
> > > I guess a less horrible version of that is:
> > >
> > > static inline warning_blank_line(void)
> > > {
> > > warning("%s", "");
> > > }
> > >
> > > We'd potentially need a matching one for error(), but at last it avoids
> > > macro trickery.
> >
> > I fail to see how this function, or this definition, makes the code better
> > than simply calling `warning("%s", "");` and be done with it.
>
> The only advantage is that it is self-documenting, so somebody does not
> come through later and convert ("%s", "") back to ("").
We could switch the DEVELOPER option on by default, when gcc or clang is
used at least. Otherwise the DEVELOPER option (which I like very much)
would not be able to live up to its full potential.
Another thing we should consider: paying more attention to Continuous
Integration. At the moment, it happens quite frequently that `pu` builds
and passes the test suite fine on Linux, but neither on Windows nor on
MacOSX and it takes days to get the regressions fixed.
I vote for this patch:
> -- >8 --
> Subject: [PATCH] difftool: hack around -Wzero-length-format warning
>
> Building with "gcc -Wall" will complain that the format in:
>
> warning("")
>
> is empty. Which is true, but the warning is over-eager. We
> are calling the function for its side effect of printing
> "warning:", even with an empty string.
>
> Our DEVELOPER Makefile knob disables the warning, but not
> everybody uses it. Let's silence the warning in the code so
> that nobody reports it or tries to "fix" it.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/difftool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 42ad9e804..b5e85ab07 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
> warning(_("both files modified: '%s' and '%s'."),
> wtdir.buf, rdir.buf);
> warning(_("working tree file has been left."));
> - warning("");
> + warning("%s", "");
> err = 1;
> } else if (unlink(wtdir.buf) ||
> copy_file(wtdir.buf, rdir.buf, st.st_mode))
> --
> 2.11.0.840.gd37c5973a
Ciao,
Dscho
^ permalink raw reply
* Re: sparse checkout - weird behavior
From: Paul Hammant @ 2017-01-26 11:20 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170126045936.wyleenuwunhrvbn2@sigill.intra.peff.net>
Well I feel a bit silly. Thanks for responding.
On Wed, Jan 25, 2017 at 11:59 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 25, 2017 at 10:21:19PM -0500, Paul Hammant wrote:
>
>> Related bug (maybe the same). Reproduction:
>>
>> $ git clone git@github.com:jekyll/jekyll.git --no-checkout
>> Cloning into 'jekyll'...
>> remote: Counting objects: 41331, done.
>> remote: Compressing objects: 100% (5/5), done.
>> remote: Total 41331 (delta 0), reused 0 (delta 0), pack-reused 41326
>> Receiving objects: 100% (41331/41331), 11.91 MiB | 9.15 MiB/s, done.
>> Resolving deltas: 100% (26530/26530), done.
>> $ cd jekyll
>> $ git config core.sparsecheckout true
>> $ echo 'CONDUCT.markdown' > .git/info/sparse-checkout
>> $ echo 'Gemfile' >> .git/info/sparse-checkout
>> $ echo 'Rakefile' >> .git/info/sparse-checkout
>> $ echo 'appveyor.yml' >> .git/info/sparse-checkout
>> $ git checkout --
>> Your branch is up-to-date with 'origin/master'.
>> $ ls
>> CONDUCT.markdown Gemfile Rakefile appveyor.yml lib
>>
>> I was not expecting to see 'lib' in the resulting file list
>
> Yep, I think this is the same problem. Inside lib, you get only
> "lib/theme_template/Gemfile", because it matches your unanchored
> pattern. Using "/Gemfile" in the sparse-checkout file fixes it.
>
> -Peff
^ permalink raw reply
* Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Johannes Schindelin @ 2017-01-26 11:37 UTC (permalink / raw)
To: Johannes Sixt
Cc: Jeff King, Junio C Hamano, David Aguilar, Ramsay Jones,
GIT Mailing-list
In-Reply-To: <546179e0-1d6e-86f7-00cf-e13218b76de1@kdbg.org>
Hi Hannes,
On Thu, 26 Jan 2017, Johannes Sixt wrote:
> Am 25.01.2017 um 23:01 schrieb Jeff King:
> > +#pragma GCC diagnostic ignored "-Wformat-zero-length"
>
> Last time I used #pragma GCC in a cross-platform project, it triggered
> an "unknown pragma" warning for MSVC.
It is starting to become a little funny how many ways we can discuss the
resolution of the GCC compiler warning.
And it starts to show: we try to solve the thing in so many ways, just to
avoid the obviously most-trivial patch to change warning(""); to
warning("%s", "") (the change to warning(" "); would change behavior, but
I would be fine with that, too).
I am not really interested in any of these complicated workarounds. If you
gentle people decide they are better in Git's source code, go ahead. I do
not have to like what you are doing, I just have to work with it.
> (It was the C++ compiler, I don't know if the C compiler would also
> warn.) It would have to be spelled like this:
>
> #pragma warning(disable: 4068) /* MSVC: unknown pragma */
> #pragma GCC diagnostic ignored "-Wformat-zero-length"
>
> Dscho mentioned that he's compiling with MSVC. It would do him a favor.
I am compiling with MSVC, and the idea is to tap into that large number of
Windows developers who Git traditionally has had a really bad time
attracting. From that perspective, I would say it would not only do me a
favor, but anybody who builds Git for Windows using Visual Studio.
But we also have to consider whether it would do anybody a "dis-favor".
#pragma statements are by definition highly dependent on the compiler. I
have no idea whether there are developers out there building Git with
C compilers other than GCC, clang or MSVC (as I did back in the days on
IRIX and HP/UX), but there is quite the potential for problems here [*1*].
To keep Git's source code truly portable, the #pragma would have to be
guarded by a GCC-specific #ifdef ... #endif.
Ciao,
Dscho
Footnote *1*: This is just another instance where a discussion on the Git
mailing list reminds me of
http://thedailywtf.com/articles/The_Complicator_0x27_s_Gloves, as it tries
to avoid an obvious solution by trying to come up with a different
solution that in turn requires additional solutions to additional problems
caused by the alternative solution.
^ permalink raw reply
* Re: [PATCH] Documentation: implement linkgit macro for Asciidoctor
From: Johannes Schindelin @ 2017-01-26 11:43 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Jeff King, Øyvind A. Holm
In-Reply-To: <20170126001344.445534-1-sandals@crustytoothpaste.net>
Hi Brian,
On Thu, 26 Jan 2017, brian m. carlson wrote:
> AsciiDoc uses a configuration file to implement macros like linkgit,
> while Asciidoctor uses Ruby extensions. Implement a Ruby extension that
> implements the linkgit macro for Asciidoctor in the same way that
> asciidoc.conf does for AsciiDoc. Adjust the Makefile to use it by
> default.
I like it.
Thank you,
Johannes
^ permalink raw reply
* Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Johannes Schindelin @ 2017-01-26 12:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer
In-Reply-To: <xmqqinp2939j.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 25 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Now, with the patch in question (without the follow-up, which I would
> > like to ask you to ignore, just like you did so far), Git would not
> > figure out that your script calls PuTTY eventually. The work-around?
> > Easy:
> >
> > DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh
>
> Think about how you would explain that to an end-user in our document?
> You'll need to explain how exactly the auto-detection works, so that the
> user can "exploit" the loophole to do that. And what maintenance burden
> does it add when auto-detection is updated?
Fine, you do not like it. Saying so (instead of asking me questions) would
have been helpful.
> I think I know you well enough that you know well enough that it is too
> ugly to live, and I suspect that the above is a tongue-in-cheek "arguing
> for the sake of argument" and would not need a serious response, but
> just in case...
It was not tongue-in-cheek, I was being serious.
> Yes. Here is what comes on an obvious clean-up patch (which will be
> sent as a follow-up to this message).
I'd much rather prefer
https://github.com/git-for-windows/git/pull/1030 than your patch.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2] fsck: lazily load types under --connectivity-only
From: Johannes Schindelin @ 2017-01-26 12:07 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170126041206.5qfv7r7czbwfqvaa@sigill.intra.peff.net>
Hi Peff,
On Wed, 25 Jan 2017, Jeff King wrote:
> builtin/fsck.c | 58 +++++++---------------------------------------------------
> fsck.c | 4 ++++
> 2 files changed, 11 insertions(+), 51 deletions(-)
Patch looks good to my eyes.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] Retire the `relink` command
From: Johannes Schindelin @ 2017-01-26 12:17 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Junio C Hamano
In-Reply-To: <20170125232051.GA25810@whir>
Hi Eric,
On Wed, 25 Jan 2017, Eric Wong wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> > Back in the olden days, when all objects were loose and rubber boots
> > were made out of wood, it made sense to try to share (immutable)
> > objects between repositories.
> >
> > Ever since the arrival of pack files, it is but an anachronism.
> >
> > Let's move the script to the contrib/examples/ directory and no longer
> > offer it.
>
> On the other hand, we have no idea if there are still people
> using it for whatever reason...
>
> I suggest we have a deprecation period where:
I would be fine with a deprecation phase, but that decision is solely on
Junio's shoulders.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] mingw: allow hooks to be .exe files
From: Johannes Schindelin @ 2017-01-26 12:21 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20170125211529.jq4halip4ndbff5k@sigill.intra.peff.net>
Hi Peff,
On Wed, 25 Jan 2017, Jeff King wrote:
> On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
>
> > - if (access(path.buf, X_OK) < 0)
> > + if (access(path.buf, X_OK) < 0) {
> > +#ifdef STRIP_EXTENSION
> > + strbuf_addstr(&path, ".exe");
>
> I think STRIP_EXTENSION is a string. Should this line be:
>
> strbuf_addstr(&path, STRIP_EXTENSION);
Yep.
v2 coming,
Johannes
^ 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