Re: [PATCH] fsckObjects tests: show how v2.17.1 can exploit downstream
On Fri, Jun 01, 2018 at 10:42:00AM +0900, Junio C Hamano wrote: > Jeff King writes: > > > I haven't tested it, but I suspect that doing multiple fetches could > > result in passing bad objects through a fetch.fsckObjects filter. > > Because the objects aren't quarantined on fetch, and because > > fsck_finish() requires the objects to be installed into place, they may > > ... > > I think in the long run fetch should implement a similar quarantine > > procedure to what happens on push. > > Interesting. > > I wonder if we can teach quickfetch codepath to notice the presence > of fsckObjects, instead of doing a full quarantine. We can easily > enumerate those objects that were already in the object database but > outside of the reachability chain before we pretend that we fetched > them and make them reachable, and check the content integrity of > them, no? Yes, we could. But it kind of feels like plugging holes in the dike. That saves "fetch" from referencing them accidentally, but other git programs may see and react to them. E.g., you're just an "update-ref" away from referencing the bad history. I don't expect that most attackers can then convince a victim to reference the rejected objects, but it feels a lot more hand-wavy than just saying "we don't let these objects into the repository in the first place". -Peff
Re: [RFC PATCH 3/4] color.ui config: don't die on unknown values
On Thu, May 31, 2018 at 09:17:41AM +0200, Ævar Arnfjörð Bjarmason wrote: > > Ævar Arnfjörð Bjarmason writes: > > > >> Before this change git will die on any unknown color.ui values: > >> > >> $ git -c color.ui=doesnotexist show > >> fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid > >> unit > > > > I do not think "unit" is correct, so there may be some room for > > improvement. For _this_ particular case, I agree that it is not the > > end of the world if we did not color the output (because we do not > > know what the 'doesnotyetexist' token from the future is trying to > > tell us), but as a general principle, we should diagnose and die, if > > a misconfiguration is easy to correct. > > Many users (including myself) use the same ~/.gitconfig on many > different machines with different git versions. Maybe at some point I'm > willing to set the new setting to a value I know is supported on most of > them, but it sucks at that point if I logging into 1-3% of old machines > ends up killing git on any invocation. One way I've dealt with this in the past is by breaking my config into multiple files, and using an "[include]" for the relevant ones in each environment. That's not quite turn-key, because you need some way to decide which to include and which not to, and there's no good way to have Git invoke that. Some options I've pondered: - we could add [includeIf "version:2.18.0"] for a minimum-version check - we could add [includeIf "env:FOO"] to check "$FOO" as a boolean. That punts the work to your shell environment, but it's flexible enough to let you decide however you like (checking git version, machine name, etc) - similarly, we could add [includeIf "sh:test -f /etc/foo"], but running actual shell is nasty for a lot of reasons. Relying on the environment punts on that. You can actually do the environment thing today, but it's a bit hacky. E.g., with this in your .profile or similar: git version | perl -ne ' my $ok = /git version (\d+\.\d+)/ && $1 >= 2.15; exit !$ok; ' && export GIT_CONFIG_PARAMETERS="'include.path=$HOME/.gitconfig-2.15'" I know that's more work than just having Git ignore config it doesn't understand, but it's also a lot more flexible (instead of just ignoring and using the defaults, you can say "for this version do X, for that one do Y"). -Peff
Re: [PATCH 1/4] config doc: move color.ui documentation to one place
On Thu, May 31, 2018 at 09:09:58AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Instead let's briefly describe what each variable is for, and then > >> copy/paste a new boilerplate saying that this variable takes the exact > >> same values as color.ui, and if it's unset it'll fallback to whatever > >> color.ui is set to. > > > > Definitely an improvement. Though I wonder if we should go further and > > show the user the relationship in the documentation explicitly. Like: > > > > color.:: > > A boolean to enable/disable color in a particular part of Git, > > overriding `color.ui` (see `color.ui` for possible values). The > > current set of systems is: > > > > advice:: > > Hints shown with the "hint:" prefix, controlled by > > `advice.*` config. > > > > branch:: > > Output from the linkgit:git-branch[1] command. > > > > ...etc... > > > > We could possibly do the same with color... Or maybe even > > make a single hierarchical list of systems, and then the color slots > > under each. I think if our mental model in adding these options is > > to have this kind of hierarchy, then it makes sense to communicate it > > explicitly to the user and get them using the same mental model. > > I wouldn't be opposed to some twist on that, but I really dislike the > variables that are documented in such a way that you can't find them in > the documentation by searching for their fully qualified name. Yeah, good point. We could probably write "color.advice", etc, in the second level list. It's redundant, but more explicit. And we still benefit from the grouping. -Peff
Re: [RFC PATCH 4/4] color.ui config: add "isatty" setting
On Thu, May 31, 2018 at 09:01:49AM +0200, Ævar Arnfjörð Bjarmason wrote: > > Is there some case where a pager can only handle color if _it's_ output > > is going to a tty, and otherwise not? > > Maybe I'm missing something but how is that something we can deal with? > We just: > > 1. use isatty() to see if we're attached to a terminal > 2a. If yes and no pager is invoked (e.g. git status) and "auto" we use colors > 2b. If yes and we're invoking a pager (e.g. git log) and "auto" we use colors > 3b. At this point we're writing to the pager so isatty() is false, > but we set our own GIT_PAGER_IN_USE and turn on colors if "auto" > > I suppose we can imagine a pager that sometimes emits to a terminal and > sometimes e.g. opens a browser with the output, and we could ourselves > somehow detect this... I was imagining something where we remembered the original isatty() value (from before the pager) and then reacted to that. But no, I don't actually have a use case there. I was trying to think through possible reasons to want this "isatty" version of color.ui. > As noted in the cover letter I started writing this whole thing before > understanding some of the subtleties, and now I think this "isatty" > thing is probably pretty useless, and was wondering if others wanted it. OK, I agree that it doesn't seem all that useful. > Reasons to take it are: > > 1) To make user intent clearer. I.e. we could just also make it a > synonym for color.ui=auto & color.pager=false and those used to > isatty semantics skimming the docs would more easily find the right > thing. I'd much prefer just having a documentation patch that uses the word "isatty", if that's something we think a user might search for (which seems plausible to me). > 2) If there are any cases where isatty() is true, but we can detect via > other means (e.g. inspecting other env variables) that non-pager > output can't handle colors some of the time. Of course if we find > such cases isatty() would suck more, but that's presumably what > isatty() purists want :) Yeah, I think we can punt on that until such an "other means" comes along. -Peff
[PATCH] f4b04cf65 ("git-subtree: I suggest to add following lines into commit message", 2018-05-31)
From f4b04cf65c4b220c8127ed2692c057fab76392a9 Mon Sep 17 00:00:00 2001
From: kx
Date: Thu, 31 May 2018 22:47:49 +0300
Subject: [PATCH 1/3] I suggest to add following lines into commit
message
created by 'git subtree add' command:
git-subtree-repo: https://github.com/.../remote.git
git-subtree-ref: master
this allow us to list externals using 'git subtree --list':
foo https://github.com/.../remote.git branch master HEAD
And also when option '-d' is applayed and remote repository
has been updated we can show help message like following:
$ git subtree -d --list
Looking for externals...
Commit: cc0436022362174bf04b0aac504913d4ccbd8b90
foo https://github.com/.../remote.git branch master HEAD
The 'foo' subtree seems not updated:
original revision: e46649ead5b895cd8d27be734241f50fff4daa60
reemote revision: b23fec043212d37e549c9c1515ea3b2a955206df
You can update 'foo' subtree by following command:
git subtree pull --prefix=foo https://github.com/.../remote.git
master
---
contrib/subtree/git-subtree.sh | 141
contrib/subtree/git-subtree.txt | 5 ++
2 files changed, 146 insertions(+)
diff --git a/contrib/subtree/git-subtree.sh
b/contrib/subtree/git-subtree.sh
index d3f39a862..969649530 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -15,8 +15,10 @@ git subtree merge --prefix=
git subtree pull --prefix=
git subtree push --prefix=
git subtree split --prefix=
+git subtree --list
--
h,helpshow the help
+l,listshow externals
q quiet
d show debug messages
P,prefix= the name of the subdir to split out
@@ -48,6 +50,9 @@ annotate=
squash=
message=
prefix=
+repo=
+ref=
+list=
debug () {
if test -n "$debug"
@@ -77,6 +82,119 @@ assert () {
fi
}
+show_externals () {
+ debug "Looking for externals..."
+ dir=
+ rev=
+ updated=
+ commit=
+ repo=
+ ref=
+ git log --grep="^git-subtree-dir: .*\$" \
+ --no-show-signature --pretty=format:'START
%H%n%s%n%n%b%nEND%n' |
+ while read a b junk
+ do
+case "$a" in
+START)
+ commit="$b"
+ ;;
+git-subtree-dir:)
+ if test -n "$dir"
+ then
+if test "$dir" = "$b"
+then
+ break
+fi
+ fi
+ dir="$b"
+ ;;
+git-subtree-split:)
+ rev="$b"
+ ;;
+git-subtree-repo:)
+ repo="$b"
+ ;;
+git-subtree-ref:)
+ ref="$b"
+ ;;
+END)
+ if test -n "$dir" -a -e "$dir"
+ then
+debug ""
+debug "Commit: $commit"
+if test -n "$repo"
+then
+ output="$(git ls-remote $repo)"
+ if test -n "$ref"
+ then
+if $(echo "$output" | grep -q $ref)
+then
+ hash="$(echo "$output" | grep "$ref\$" | cut -f1
-d$'\t')"
+ if test "$hash" != "$rev"
+ then
+updated="$rev"
+rev="$hash"
+ fi
+fi
+ fi
+
+ output="$(echo "$output" | grep $rev)"
+ if test -n "$output"
+ then
+echo "$output" |
+while read h r junk
+do
+ case "$r" in
+HEAD)
+ head="yes"
+ ;;
+refs/heads/*)
+ name=$(basename $r)
+ type="branch"
+ echo -n "$dir $repo $type $name"
+ if test "$head" = "yes"
+ then
+echo " HEAD"
+ else
+echo " $rev"
+ fi
+ break
+ ;;
+refs/tags/*)
+ name=$(basename $r)
+ type="tag"
+ echo "$dir $repo $type $name $rev"
+ break
+ ;;
+ esac
+done
+if test -n "$updated"
+then
+ debug ""
+ debug "The '$dir' subtree seems not updated:"
+ debug " original revision: $updated"
+ debug "reemote revision: $rev"
+ debug "You can update '$dir' subtree by following
command:"
+ debug ""
+ debug " git subtree pull --prefix=$dir $repo $ref"
+ debug ""
+fi
+ fi
+else
+ echo "$dir $rev"
+fi
+ fi
+
+ rev=
+ updated=
+ commit=
+ repo=
+ ref=
+ ;;
+esac
+ done
+}
+
while test $# -gt 0
do
@@ -137,6 +255,9 @@ do
--no-squash)
squash=
;;
+ -l)
+ list=yes
+ ;;
--)
break
;;
@@ -149,6 +270,12 @@ done
command="$1"
shift
+if test "$list" = "yes"
+then
+ show_externals
+ exit 0
+fi
+
case "$command" in
add|merge|pull)
default=
@@ -421,6 +548,18 @@ add_msg ()
Re: [PATCH v4 8/9] checkout: add advice for ambiguous "checkout "
Junio C Hamano writes:
> Ævar Arnfjörð Bjarmason writes:
>
>> @@ -1269,6 +1270,16 @@ int cmd_checkout(int argc, const char **argv, const
>> char *prefix)
>> if (opts.patch_mode || opts.pathspec.nr) {
>> int ret = checkout_paths(, new_branch_info.name,
>> _remotes_matched);
>> +if (ret && dwim_remotes_matched > 1 &&
>> +advice_checkout_ambiguous_remote_branch_name)
>> +advise(_("The argument '%s' matched more than one
>> remote tracking branch.\n"
>> + "We found %d remotes with a reference that
>> matched. So we fell back\n"
>> + "on trying to resolve the argument as a path,
>> but failed there too!\n"
>> + "\n"
>> + "Perhaps you meant fully qualify the branch
>> name? E.g. origin/\n"
>> + "instead of ?"),
>> + argv[0],
>> + dwim_remotes_matched);
>> return ret;
>
> Do we give "checkout -p no-such-file" the above wall of text?
>
> Somehow checkout_paths(), which is "we were given a tree-ish and
> pathspec and told to grab the matching paths out of it and stuff
> them to the index and the working tree", is a wrong place to be
> doing the "oh, what the caller thought was pathspec may turn out to
> be a rev, so check that too for such a confused caller". Shouldn't
> the caller be doing all that (which would mean we wan't need to pass
> "remotes-matched" to the function, as the helper has nothing to do
> with deciding which arg is the tree-ish).
Well, upon closer inspection adding *dwim_remotes_matched parameter
to checkout_paths() done in an earlier step seems to be totally
bogus and only serves the purpose of confusing reviewers. The
function does not touch the pointer in any way---it does not use the
pointer to return its findings, and it does not use an earlier
findings to affect its behaviour by dereferencing it.
The dwim_remotes_matched is set by an earlier call to
parse_branchname_arg(), which does gain an int* parameter in this
series. And that addition _does_ make sense. That codepath is
where the "do we have many remotes that could match, or none, or
unique?" determination is made.
[PATCH] t9104: kosherly remove remote refs
As there are plans to implement other ref storage systems, let's use a way to remove remote refs that does not depend on refs being files. This makes it clear to readers that this test does not depend on which ref backend is used. Suggested-by: Michael Haggerty Helped-by: Jeff King Signed-off-by: Christian Couder --- This was suggested and discussed in: https://public-inbox.org/git/20180525085906.ga2...@sigill.intra.peff.net/ t/t9104-git-svn-follow-parent.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh index 9c49b6c1fe..5e0ad19177 100755 --- a/t/t9104-git-svn-follow-parent.sh +++ b/t/t9104-git-svn-follow-parent.sh @@ -215,7 +215,9 @@ test_expect_success "multi-fetch continues to work" " " test_expect_success "multi-fetch works off a 'clean' repository" ' - rm -rf "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" && + rm -rf "$GIT_DIR/svn" && + git for-each-ref --format="option no-deref%0adelete %(refname)" refs/remotes | + git update-ref --stdin && git reflog expire --all --expire=all && mkdir "$GIT_DIR/svn" && git svn multi-fetch -- 2.17.0.1035.g12039e008f
Re: [PATCH v4 8/9] checkout: add advice for ambiguous "checkout "
Ævar Arnfjörð Bjarmason writes:
> @@ -1269,6 +1270,16 @@ int cmd_checkout(int argc, const char **argv, const
> char *prefix)
> if (opts.patch_mode || opts.pathspec.nr) {
> int ret = checkout_paths(, new_branch_info.name,
>_remotes_matched);
> + if (ret && dwim_remotes_matched > 1 &&
> + advice_checkout_ambiguous_remote_branch_name)
> + advise(_("The argument '%s' matched more than one
> remote tracking branch.\n"
> + "We found %d remotes with a reference that
> matched. So we fell back\n"
> + "on trying to resolve the argument as a path,
> but failed there too!\n"
> + "\n"
> + "Perhaps you meant fully qualify the branch
> name? E.g. origin/\n"
> + "instead of ?"),
> +argv[0],
> +dwim_remotes_matched);
> return ret;
Do we give "checkout -p no-such-file" the above wall of text?
Somehow checkout_paths(), which is "we were given a tree-ish and
pathspec and told to grab the matching paths out of it and stuff
them to the index and the working tree", is a wrong place to be
doing the "oh, what the caller thought was pathspec may turn out to
be a rev, so check that too for such a confused caller". Shouldn't
the caller be doing all that (which would mean we wan't need to pass
"remotes-matched" to the function, as the helper has nothing to do
with deciding which arg is the tree-ish).
Re: [PATCH v4 4/9] checkout.[ch]: introduce an *_INIT macro
Ævar Arnfjörð Bjarmason writes:
> Add an *_INIT macro for the tracking_name_data similar to what exists
> elsewhere in the codebase, e.g. OID_ARRAY_INIT in sha1-array.h. This
> will make it more idiomatic in later changes to add more fields to the
> struct & its initialization macro.
Makes sense; Thomas's comment on 3/9 still stands at this point, as
we have no outside users of the definition of the callback data yet
at this point.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> checkout.c | 2 +-
> checkout.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/checkout.c b/checkout.c
> index 8d68f75ad1..629fc1d5c4 100644
> --- a/checkout.c
> +++ b/checkout.c
> @@ -25,7 +25,7 @@ static int check_tracking_name(struct remote *remote, void
> *cb_data)
>
> const char *unique_tracking_name(const char *name, struct object_id *oid)
> {
> - struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
> + struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
> cb_data.src_ref = xstrfmt("refs/heads/%s", name);
> cb_data.dst_oid = oid;
> for_each_remote(check_tracking_name, _data);
> diff --git a/checkout.h b/checkout.h
> index 04b52f9ffe..a61ec93e65 100644
> --- a/checkout.h
> +++ b/checkout.h
> @@ -10,6 +10,8 @@ struct tracking_name_data {
> int unique;
> };
>
> +#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
> +
> /*
> * Check if the branch name uniquely matches a branch name on a remote
> * tracking branch. Return the name of the remote if such a branch
Re: [PATCH v4 1/9] checkout tests: index should be clean after dwim checkout
Ævar Arnfjörð Bjarmason writes:
> Assert that whenever there's a DWIM checkout that the index should be
> clean afterwards, in addition to the correct branch being checked-out.
> ...
> So let's amend the tests (mostly added in) 399e4a1c56 ("t2024: Add
> tests verifying current DWIM behavior of 'git checkout '",
> 2013-04-21) to always assert that "status" is clean after we run
> "checkout", that's being done with "-uno" because there's going to be
> some untracked files related to the test itself which we don't care
> about.
It might not be absolutely necessary to state, but it would be
helpful to say that you are assuming to start a checkout (DWIM or
otherwise) from a clean state; without the assumption, the readers
need to think for a few breaths why "the index should be clean" is
true.
The intention and the implementation of the change both mostly look
good to me from a quick read.
> test_expect_success 'setup' '
> test_commit my_master &&
> git init repo_a &&
> @@ -55,6 +61,7 @@ test_expect_success 'checkout of non-existing branch fails'
> '
> test_might_fail git branch -D xyzzy &&
>
> test_must_fail git checkout xyzzy &&
> + status_uno_is_clean &&
> test_must_fail git rev-parse --verify refs/heads/xyzzy &&
> test_branch master
> '
> @@ -64,8 +71,10 @@ test_expect_success 'checkout of branch from multiple
> remotes fails #1' '
> test_might_fail git branch -D foo &&
>
> test_must_fail git checkout foo &&
> + status_uno_is_clean &&
> test_must_fail git rev-parse --verify refs/heads/foo &&
> - test_branch master
> + test_branch master &&
> + status_uno_is_clean
Hmm, what's the point of this second one?
> '
Re: [PATCH] fetch: do not pass ref-prefixes for fetch by exact SHA1
Junio C Hamano wrote: > Jonathan Nieder writes: >> This patch adds a test to check this behavior that notices another >> behavior difference between protocol v0 and v2 in the process. Add a >> NEEDSWORK comment to clear it up. > > Thanks. > > I wonder if there is a more effective way to smoke out other bugs > remaining in proto v2. When the fetch-by-SHA1 feature was added > originally, we certainly would have added a test or two to make sure > it won't break. The root cause of this breakage is that we lack the > ability to easily exercise proto v2 on these existing tests that > were written back in the proto v0 days. It there were such a way > (like, a common set of tests that are run with all supported > protos), we would have caught the breakge even before the topic hit > 'next'. I had a similar thought. I am not sure I agree about the root cause, but root causes are generally slippery to define. Because this bug had significant internal impact, we came up with a few next steps: - shore up protocol v2 test coverage, as you described - arrange for long refactoring series we submit to be divided up for the team to review, to avoid reviewer fatigue. Hopefully this will make us a better example for other submitters of long series. We're open to cooperating with others --- maybe we can set up a volunteer reviewer brigade to get a more diverse set of eyes on each series --- though organizing that is harder. - improve telemetry for our internal deployment, to get earlier notice when Git is producing more errors. I suspect other installations may want something like this too --- e.g. I think this is one of the benefits of what Jeff Hostetler is starting to build with json-writer. - help internal users triage errors from Git (like those decision trees parents have to help decide when to bring a child to the doctor), so that we get earlier notice and can roll back and report upstream more quickly when they've run into a Git bug Or in other words, please expect more in this area soon, and feel free to pester me if the test coverage doesn't arrive. :) Thanks, Jonathan
Re: [PATCH 5/5] refspec.c: use rhs in parse_refspec instead of potentially uninitialized item->dst
Junio C Hamano writes:
> Perhaps a better fisx is to explicitly assign NULL to item->dst when
> we see there is no right-hand-side.
-- >8 --
Subject: [PATCH] refspec-api: avoid uninitialized field in refspec item
When parse_refspec() function was created at 3eec3700 ("refspec:
factor out parsing a single refspec", 2018-05-16) to take a caller
supplied piece of memory to fill parsed refspec_item, it forgot that
a refspec without colon must set item->dst to NULL to let the users
of refspec know that the result of the fetch does not get stored in
an ref on our side.
Signed-off-by: Junio C Hamano
---
* The original before that change filled a callee prepared piece of
memory that was obtained from xcalloc(), and did not need to
explicitly assign NULL to the field after noticing that there is
no colon in the refspec, so it is understandable how this
misconvesion happened.
refspec.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/refspec.c b/refspec.c
index 97e76e8b1d..6e45365a23 100644
--- a/refspec.c
+++ b/refspec.c
@@ -48,6 +48,8 @@ static int parse_refspec(struct refspec_item *item, const
char *refspec, int fet
size_t rlen = strlen(++rhs);
is_glob = (1 <= rlen && strchr(rhs, '*'));
item->dst = xstrndup(rhs, rlen);
+ } else {
+ item->dst = NULL;
}
llen = (rhs ? (rhs - lhs - 1) : strlen(lhs));
--
2.18.0-rc0
Re: [RFC PATCH 4/6] commit-graph: avoid writing when repo is shallow
Derrick Stolee writes:
> Shallow clones do not interact well with the commit-graph feature for
> several reasons. Instead of doing the hard thing to fix those
> interactions, instead prevent reading or writing a commit-graph file for
> shallow repositories.
The latter instead would want to vanish, I would guess.
>
> Signed-off-by: Derrick Stolee
> ---
> commit-graph.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 95af4ed519..80e377b90f 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -208,6 +208,9 @@ static void prepare_commit_graph(void)
> return;
> prepare_commit_graph_run_once = 1;
>
> + if (is_repository_shallow())
> + return;
> +
> obj_dir = get_object_directory();
> prepare_commit_graph_one(obj_dir);
> prepare_alt_odb();
> @@ -711,6 +714,15 @@ void write_commit_graph(const char *obj_dir,
> int num_extra_edges;
> struct commit_list *parent;
>
> + /*
> + * Shallow clones are not supproted, as they create bad
> + * generation skips as they are un-shallowed.
> + */
> + if (is_repository_shallow()) {
> + warning("writing a commit-graph in a shallow repository is not
> supported");
> + return;
> + }
> +
> oids.nr = 0;
> oids.alloc = approximate_object_count() / 4;
Re: is there a reason pre-commit.sample uses "git diff-index"?
Stefan Beller writes: > plumbing command, so the likelihood of git-log calls in scripts out > there is high. > > So maybe the community should strive to be more aggressive about > changing the porcelain interface for the better. To me, these two paragraphs are being incoherent. If plumbing these days lag behind "log" Porcelain and tempt script writers more towards "log", we should aggressively reject attempts to change the "log" Porcelain behaviour to keep it stable, until a suitable plumbing that scripters can rely on catches up. Or course, aggressively improving plumbing would be a good solution to that problem as well ;-)
Re: [PATCH v4 9/9] checkout & worktree: introduce checkout.defaultRemote
Thomas Gummerer writes: >> I considered splitting this into checkout.defaultRemote and >> worktree.defaultRemote, but it's probably less confusing to break our >> own rules that anything shared between config should live in core.* >> than have two config settings, and I couldn't come up with a short >> name under core.* that made sense (core.defaultRemoteForCheckout?). I do think "checkout" in name is grately helpful. I do not see why it is a bad idea for the worktree codepath to pay attention to the checkout.defaultRemote configuration variable, especially when those who are discussing this thread agree "checkout" and "worktree add" are quite similar in end-users' minds.
Re: [PATCH v4 3/9] checkout.[ch]: move struct declaration into *.h
Thomas Gummerer writes: > We seem to have plenty of structs defined in '.c' files, if they are > only needed there. Its use also seems to be single purpose for the > callback data, so I'm a bit puzzled how having this in a header file > instead of the .c file would be helpful? > > I feel like having only the "public" part in the header file also > helps developers that are just looking for documentation of the > functions they are looking at, by having less things to go through, > that they wouldn't necessarily care about. Yup, sounds like a sensible criterion to choose between <*.h> and <*.c>.
Re: [PATCH] fetch: do not pass ref-prefixes for fetch by exact SHA1
Jonathan Nieder writes: > When v2.18.0-rc0~10^2~1 (refspec: consolidate ref-prefix generation > logic, 2018-05-16) factored out the ref-prefix generation code for > reuse, it left out the 'if (!item->exact_sha1)' test in the original > ref-prefix generation code. As a result, fetches by SHA-1 generate > ref-prefixes as though the SHA-1 being fetched were an abbreviated ref > name: > > $ GIT_TRACE_PACKET=1 bin-wrappers/git -c protocol.version=2 \ > fetch origin 12039e008f9a4e3394f3f94f8ea897785cb09448 > [...] > packet:fetch> ref-prefix 12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/tags/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/heads/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448/HEAD > packet:fetch> > > If there is another ref name on the command line or the object being > fetched is already available locally, then that's mostly harmless. > But otherwise, we error out with > > fatal: no matching remote head > > since the server did not send any refs we are interested in. Filter > out the exact_sha1 refspecs to avoid this. > > This patch adds a test to check this behavior that notices another > behavior difference between protocol v0 and v2 in the process. Add a > NEEDSWORK comment to clear it up. Thanks. I wonder if there is a more effective way to smoke out other bugs remaining in proto v2. When the fetch-by-SHA1 feature was added originally, we certainly would have added a test or two to make sure it won't break. The root cause of this breakage is that we lack the ability to easily exercise proto v2 on these existing tests that were written back in the proto v0 days. It there were such a way (like, a common set of tests that are run with all supported protos), we would have caught the breakge even before the topic hit 'next'.
Re: [PATCH 5/5] refspec.c: use rhs in parse_refspec instead of potentially uninitialized item->dst
Stefan Beller writes:
> 'item->dst' has not been assigned if '!rhs' is true. As the caller is allowed
> to pass in uninitialized
> memory (we don't assume 'item' was zeroed out before calling), this fixes an
> access to
> uninitialized memory.
Did I miss the other 4 patches that this might depend on it?
> diff --git a/refspec.c b/refspec.c
> index c59a4ccf1e5..ea169dec0d3 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -108,7 +108,7 @@ static int parse_refspec(struct refspec_item *item, const
> char *refspec, int fet
>* - empty is not allowed.
>* - otherwise it must be a valid looking ref.
>*/
> - if (!item->dst) {
> + if (!rhs) {
> if (check_refname_format(item->src, flags))
> return 0;
> } else if (!*item->dst) {
Perhaps a better fisx is to explicitly assign NULL to item->dst when
we see there is no right-hand-side.
Aside from the "uninitialized" issue, the original if/else cascade
around here makes a lot more sense than the updated version. If we
do not leave item->dst uninitialized, the control (and the readers'
understanding) can flow without having to carry the invariant
"item->dst is set ONLY when rhs != NULL" throughout this codepath,
in order to understand that this if/else cascade is asking: is
pointer NULL? then do one thing, otherwise is pointee NUL? then do
another thing, otherwise we have a non-empty string so do something
on it.
[L10N] Kickoff of translation for Git 2.18.0 round 1
Hi, Git v2.18.0-rc0 has been released, and it's time to start new round of git l10n. This time there are 108 updated messages need to be translated since last update: l10n: git.pot: v2.18.0 round 1 (108 new, 14 removed) Generate po/git.pot from v2.18.0-rc0 for git v2.18.0 l10n round 1. Signed-off-by: Jiang Xin You can get it from the usual place: https://github.com/git-l10n/git-po/ As how to update your XX.po and help to translate Git, please see "Updating a XX.po file" and other sections in “po/README" file. -- Jiang Xin
Re: [PATCH] fsckObjects tests: show how v2.17.1 can exploit downstream
Jeff King writes: > I haven't tested it, but I suspect that doing multiple fetches could > result in passing bad objects through a fetch.fsckObjects filter. > Because the objects aren't quarantined on fetch, and because > fsck_finish() requires the objects to be installed into place, they may > ... > I think in the long run fetch should implement a similar quarantine > procedure to what happens on push. Interesting. I wonder if we can teach quickfetch codepath to notice the presence of fsckObjects, instead of doing a full quarantine. We can easily enumerate those objects that were already in the object database but outside of the reachability chain before we pretend that we fetched them and make them reachable, and check the content integrity of them, no?
Re: [PATCH] branch: issue "-l" deprecation warning after pager starts
Jeff King writes: > So I think you're proposing: > > - step 0: warn about "-l" when it actually gets used, and otherwise > continue ignoring > > - step 1: turn "-l" into "--list" > > - step 2: there is no step 2 > > ... So I > guess the right rule is to warn when we are not in list-mode, and > otherwise quietly accept it. > > That does mean that anybody who misses the deprecation warning may be > surprised when "branch -l foo" starts listing instead of creating a > branch with a reflog (whereas in the current 3-step plan, we have a > period in the middle where that's a hard error). That may be OK, though, > and is a natural consequence of getting to the end step sooner (even > with a 3-step plan, anybody who skips the versions in the middle _could_ > be surprised). Thanks for a concise and readably summary ;-)
Re: Is origin/HEAD only being created on clone a bug? #leftoverbits
Ævar Arnfjörð Bjarmason writes: > We already have to deal with this special case of origin/HEAD > being re-pointed in a repository that we "clone", so we would just > do whatever happens to a repository that's cloned. OK. Not visiting that issue while we discuss this "origin/HEAD is useful, so create it even for non-initial-clone case" topic makes it simpler to discuss.
Re: [PATCH v6 1/2] blame: prevent error if range ends past end of file
isteph...@atlassian.com writes: > From: Isabella Stephens > > If the -L option is used to specify a line range in git blame, and the > end of the range is past the end of the file, git will fail with a fatal > error. This commit prevents such behavior - instead we display the blame > for existing lines within the specified range. Tests are amended > accordingly. > > This commit also fixes two corner cases. Blaming -L n,-(n+1) now blames > the first n lines of a file rather than from n to the end of the file. > Blaming -L ,-n will be treated as -L 1,-n and blame the first line of > the file, rather than blaming the whole file. > > Signed-off-by: Isabella Stephens > --- > builtin/blame.c | 4 ++-- > line-range.c | 2 +- > t/t8003-blame-corner-cases.sh | 12 > 3 files changed, 11 insertions(+), 7 deletions(-) Don't t800[12]-*.sh need adjustment for this change, too?
Re: [PATCH v6 2/2] log: prevent error if line range ends past end of file
isteph...@atlassian.com writes: > From: Isabella Stephens > > If the -L option is used to specify a line range in git log, and the end > of the range is past the end of the file, git will fail with a fatal > error. This commit prevents such behaviour - instead we perform the log > for existing lines within the specified range. > > This commit also fixes a corner case where -L ,-n:file would be treated > as a log over the whole file. Now we treat this as -L 1,-n:file and > blame the first line of the file instead. > > Signed-off-by: Isabella Stephens > --- > line-log.c | 8 +--- > t/t4211-line-log.sh | 5 ++--- > 2 files changed, 7 insertions(+), 6 deletions(-) Thanks. Will queue.
Re: [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo
On 5/31/2018 2:33 PM, Stefan Beller wrote: On Thu, May 31, 2018 at 10:40 AM, Derrick Stolee wrote: The commit-graph file stores a condensed version of the commit history. This helps speed up several operations involving commit walks. This feature does not work well if those commits "change" using features like commit grafts, replace objects, or shallow clones. Since the commit-graph feature is optional, hidden behind the 'core.commitGraph' config option, and requires manual maintenance (until ds/commit-graph-fsck' is merged), these issues only arise for expert users who decided to opt-in. This RFC is a first attempt at rectify the issues that come up when these features interact. I have not succeeded entirely, as I will discuss below. The first two "DO NOT MERGE" commits are not intended to be part of the main product, but are ways to help the full test suite run while computing and consuming commit-graph files. This is acheived by calling write_commit_graph_reachable() from `git fetch` and `git commit`. I believe this is the only dependence on ds/commit-graph-fsck. The other commits should only depend on ds/commit-graph-lockfile-fix. I applied these patches on top of ds/commit-graph-fsck (it would have been nice if you mentioned that it applies there) Running the test suite with all patches applied results in: ./t0410-partial-clone.sh(Wstat: 256 Tests: 15 Failed: 2) Failed tests: 5, 8 ./t5307-pack-missing-commit.sh (Wstat: 256 Tests: 5 Failed: 2) Failed tests: 3-4 ./t5500-fetch-pack.sh (Wstat: 256 Tests: 353 Failed: 1) Failed test: 348 ./t6011-rev-list-with-bad-commit.sh (Wstat: 256 Tests: 6 Failed: 1) Failed test: 4 ./t6024-recursive-merge.sh (Wstat: 256 Tests: 6 Failed: 1) Failed test: 4 which you identified as 4x noise and t5500 as not understood. $ GIT_TRACE=1 ./t5500-fetch-pack.sh -d -i -v -x [...] expecting success: git -C shallow12 fetch --shallow-exclude one origin && git -C shallow12 log --pretty=tformat:%s origin/master >actual && test_write_lines three two >expected && test_cmp expected actual ++ git -C shallow12 fetch --shallow-exclude one origin trace: built-in: git fetch --shallow-exclude one origin trace: run_command: unset GIT_PREFIX; 'git-upload-pack '\''/u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.'\''' trace: run_command: git --shallow-file pack-objects --revs --thin --stdout --shallow --progress --delta-base-offset --include-tag trace: built-in: git pack-objects --revs --thin --stdout --shallow --progress --delta-base-offset --include-tag remote: Counting objects: 4, done. remote: Compressing objects: 100% (3/3), done. remote: Total 4 (delta 0), reused 0 (delta 0) trace: run_command: git --shallow-file unpack-objects --pack_header=2,4 trace: built-in: git unpack-objects --pack_header=2,4 Unpacking objects: 100% (4/4), done. trace: run_command: git rev-list --objects --stdin --not --all --quiet trace: built-in: git rev-list --objects --stdin --not --all --quiet trace: run_command: unset GIT_PREFIX; 'git-upload-pack '\''/u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.'\''' trace: run_command: git pack-objects --revs --thin --stdout --progress --delta-base-offset trace: built-in: git pack-objects --revs --thin --stdout --progress --delta-base-offset remote: Total 0 (delta 0), reused 0 (delta 0) trace: run_command: git unpack-objects --pack_header=2,0 trace: built-in: git unpack-objects --pack_header=2,0 trace: run_command: git rev-list --objects --stdin --not --all --quiet trace: built-in: git rev-list --objects --stdin --not --all --quiet From file:///u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/. * [new tag] one-> one * [new tag] two-> two run_processes_parallel: preparing to run up to 1 tasks run_processes_parallel: done trace: run_command: git gc --auto trace: built-in: git gc --auto ++ git -C shallow12 log --pretty=tformat:%s origin/master trace: built-in: git log '--pretty=tformat:%s' origin/master ++ test_write_lines three two ++ printf '%s\n' three two ++ test_cmp expected actual ++ diff -u expected actual --- expected 2018-05-31 18:14:25.944540810 + +++ actual 2018-05-31 18:14:25.944540810 + @@ -1,2 +1,3 @@ three two +one error: last command exited with $?=1 not ok 348 - fetch exclude tag one # # git -C shallow12 fetch --shallow-exclude one origin && # git -C shallow12 log --pretty=tformat:%s origin/master >actual && # test_write_lines three two >expected && # test_cmp expected actual # After these changes, there is one test case that still fails, and I cannot understand why: t5500-fetch-pack.sh Failed test: 348 This test fails when performing a `git fetch --shallow-exclude`. When I halt the test using `t5500-fetch-pack.sh -d -i` and navigate to the directory to replay the fetch it performs as expected. What is "as expected" ? When I insert a test_pause
Re: [PATCH] RelNotes: remove duplicate release note
Elijah Newren writes: > In the 2.18 cycle, directory rename detection was merged, then reverted, > then reworked in such a way to fix another prominent bug in addition to > the original problem causing it to be reverted. When the reworked series > was merged, we ended up with two nearly duplicate release notes. Remove > the second copy, but preserve the information about the extra bug fix. Thanks.
Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*
Ævar Arnfjörð Bjarmason writes: > It's our documentation that should be clearly stating those reasons. If > we're not saying anything about these being historical bugs, then e.g. I > (not knowing the implementation) wouldn't have turned this on globally > on my site knowing that because I have none of these now I'm *very* > unlikely to have them in the future. > > That's different from something that just happens rarely, because a rare > non-historical event can be expected to happen in the future. Interesting. If I did not know Git at all, I would decide completely opposite---because I have none of these now, I'm very unlikely to have them in the future, so I would leave fsck. alone to the generally recommended state (i.e. not customizing without understanding what I am doing). That way, (1) if that unlikely thing happens, I would notice and have a chance to deal with it, and (2) otherwise, I wouldn't have to worry about that unlikely event at all. And that decision would not change even if I _knew_ these knobs' categories were invented to align with bugs and anomalies in older implementations of Git. > >> Between "fsck. makes sense only when you use these rare and >> you-probably-never-heard-of tools ongoing basis" and "when you >> already have (slightly)broken objects, naming each of them in >> skiplist, rather than covering the class, is better because you want >> *new* instances of the same breakage", I'd imagine the latter would be >> more helpful. >> >> In any case, let's see if there are more input to this topic and >> then wrap it up in v3 ;-) >> >> Thanks.
Re: What's cooking in git.git (May 2018, #04; Wed, 30)
Duy Nguyen writes: > On Wed, May 30, 2018 at 8:38 AM, Junio C Hamano wrote: >> * sb/object-store-alloc (2018-05-16) 13 commits >> - alloc: allow arbitrary repositories for alloc functions >> - object: allow create_object to handle arbitrary repositories >> - object: allow grow_object_hash to handle arbitrary repositories >> - alloc: add repository argument to alloc_commit_index >> - alloc: add repository argument to alloc_report >> - alloc: add repository argument to alloc_object_node >> - alloc: add repository argument to alloc_tag_node >> - alloc: add repository argument to alloc_commit_node >> - alloc: add repository argument to alloc_tree_node >> - alloc: add repository argument to alloc_blob_node >> - object: add repository argument to grow_object_hash >> - object: add repository argument to create_object >> - repository: introduce parsed objects field >> (this branch is used by sb/object-store-grafts.) >> >> The conversion to pass "the_repository" and then "a_repository" >> throughout the object access API continues. >> >> Is this ready for 'next'? > > I think so. Stefan could remove the comment "/* TODO: what about > commit->util? */" if he wants since nd/commit-util-to-slab is already > in next. But this is really minor and does not need fixing to land on > 'next'. Thanks.
Re: is there a reason pre-commit.sample uses "git diff-index"?
On Thu, May 31, 2018 at 4:09 PM, Robert P. J. Day wrote: > On Fri, 1 Jun 2018, Johannes Sixt wrote: > >> Am 31.05.2018 um 19:27 schrieb Robert P. J. Day: >> > On Thu, 31 May 2018, Duy Nguyen wrote: >> >> git diff-index is "plumbing", designed for writing scripts. "git >> >> diff" on the other hand is for users and its behavior may change >> >> even if it breaks backward compatibility. >> > >> >ah, this was a philosophical underpinning i was unaware of. i >> > see occasional explanations of git porcelain versus plumbing, but >> > i don't recall anyone simply stating that the plumbing is meant to >> > have a long-term stability that is not guaranteed for the >> > porcelain. >> >> So, there you have it. ;) Plumbing commands offer long-term >> stability. That is not just philosophical, but practically relevant. >> >> >in any event, this does mean that, stability issues aside, "git >> > diff" would apparently have worked just fine for that hook. >> >> It may have worked just fine. You should still not use it. >> >> Didn't you say that you are teaching git and hooks? Then you should >> teach the right thing, and the right thing is to use plumbing for >> scripts. > > sure, i agree, but i don't recall *ever* running across the claim > that the "plumbing" commands had a long-term stability and backward > compatibility that the porcelain commands did not. is that mentioned > anywhere? `man git` LOW-LEVEL COMMANDS (PLUMBING) Although Git includes its own porcelain layer, its low-level commands are sufficient to support development of alternative porcelains. Developers of such porcelains might start by reading about git-update- index(1) and git-read-tree(1). The interface (input, output, set of options and the semantics) to these low-level commands are meant to be a lot more stable than Porcelain level commands, because these commands are primarily for scripted use. The interface to Porcelain commands on the other hand are subject to change in order to improve the end user experience. One example that Junio seemed to worry about was 940a911f8ec (log: if --decorate is not given, default to --decorate=auto, 2017-03-23), as git log seems to be used pseudo-plumbing-ly as there is no good and equally powerful plumbing command, so the likelihood of git-log calls in scripts out there is high. So maybe the community should strive to be more aggressive about changing the porcelain interface for the better. One thing that we discussed internally for example is changing the output of the porcelain output of fetch, pull and push to give less cryptic output, but rather a one line progress bar (and only show errors when they occur). I think that would be an improvement, as I don't think many people care about the exact numbers of objects transferred in a push/fetch, but rather want to have an estimate of the time left for example. Also a one line progress bar might save some precious screen real estate. Stefan
Re: What's cooking in git.git (May 2018, #04; Wed, 30)
Taylor Blau writes: > I have these patches mostly updated on my copy (available at > https://github.com/ttaylorr/git/compare/tb/grep-column) but am out of > the office for the next week, so I will polish and send these on June > 8th. > >> * tb/grep-only-matching (2018-05-14) 2 commits >> - builtin/grep.c: teach '-o', '--only-matching' to 'git-grep' >> - grep.c: extract show_line_header() >> (this branch uses tb/grep-column.) > > This topic is done, but will be frustrating to merge after the changes > in tb/grep-column. I'll update this topic once tb/grep-column graduates > to master, that way they will both apply cleanly. Thanks for an update.
Re: is there a reason pre-commit.sample uses "git diff-index"?
On Fri, 1 Jun 2018, Johannes Sixt wrote: > Am 31.05.2018 um 19:27 schrieb Robert P. J. Day: > > On Thu, 31 May 2018, Duy Nguyen wrote: > >> git diff-index is "plumbing", designed for writing scripts. "git > >> diff" on the other hand is for users and its behavior may change > >> even if it breaks backward compatibility. > > > >ah, this was a philosophical underpinning i was unaware of. i > > see occasional explanations of git porcelain versus plumbing, but > > i don't recall anyone simply stating that the plumbing is meant to > > have a long-term stability that is not guaranteed for the > > porcelain. > > So, there you have it. ;) Plumbing commands offer long-term > stability. That is not just philosophical, but practically relevant. > > >in any event, this does mean that, stability issues aside, "git > > diff" would apparently have worked just fine for that hook. > > It may have worked just fine. You should still not use it. > > Didn't you say that you are teaching git and hooks? Then you should > teach the right thing, and the right thing is to use plumbing for > scripts. sure, i agree, but i don't recall *ever* running across the claim that the "plumbing" commands had a long-term stability and backward compatibility that the porcelain commands did not. is that mentioned anywhere? rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: is there a reason pre-commit.sample uses "git diff-index"?
Am 31.05.2018 um 19:27 schrieb Robert P. J. Day: On Thu, 31 May 2018, Duy Nguyen wrote: git diff-index is "plumbing", designed for writing scripts. "git diff" on the other hand is for users and its behavior may change even if it breaks backward compatibility. ah, this was a philosophical underpinning i was unaware of. i see occasional explanations of git porcelain versus plumbing, but i don't recall anyone simply stating that the plumbing is meant to have a long-term stability that is not guaranteed for the porcelain. So, there you have it. ;) Plumbing commands offer long-term stability. That is not just philosophical, but practically relevant. in any event, this does mean that, stability issues aside, "git diff" would apparently have worked just fine for that hook. It may have worked just fine. You should still not use it. Didn't you say that you are teaching git and hooks? Then you should teach the right thing, and the right thing is to use plumbing for scripts. -- Hannes
[PATCH 2/2] index-pack: handle --strict checks of non-repo packs
Commit 73c3f0f704 (index-pack: check .gitmodules files with
--strict, 2018-05-04) added a call to add_packed_git(), with
the intent that the newly-indexed objects would be available
to the process when we run fsck_finish(). But that's not
what add_packed_git() does. It only allocates the struct,
and you must install_packed_git() on the result. So that
call was effectively doing nothing (except leaking a
struct).
But wait, we passed all of the tests! Does that mean we
don't need the call at all?
For normal cases, no. When we run "index-pack --stdin"
inside a repository, we write the new pack into the object
directory. If fsck_finish() needs to access one of the new
objects, then our initial lookup will fail to find it, but
we'll follow up by running reprepare_packed_git() and
looking again. That logic was meant to handle somebody else
repacking simultaneously, but it ends up working for us
here.
But there is a case that does need this, that we were not
testing. You can run "git index-pack foo.pack" on any file,
even when it is not inside the object directory. Or you may
not even be in a repository at all! This case fails without
doing the proper install_packed_git() call.
We can make this work by adding the install call.
Note that we should be prepared to handle add_packed_git()
failing. We can just silently ignore this case, though. If
fsck_finish() later needs the objects and they're not
available, it will complain itself. And if it doesn't
(because we were able to resolve the whole fsck in the first
pass), then it actually isn't an interesting error at all.
Signed-off-by: Jeff King
---
builtin/index-pack.c | 8 ++--
t/t7415-submodule-names.sh | 10 ++
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 4ab31ed388..74fe2973e1 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1482,8 +1482,12 @@ static void final(const char *final_pack_name, const
char *curr_pack_name,
} else
chmod(final_index_name, 0444);
- if (do_fsck_object)
- add_packed_git(final_index_name, strlen(final_index_name), 0);
+ if (do_fsck_object) {
+ struct packed_git *p;
+ p = add_packed_git(final_index_name, strlen(final_index_name),
0);
+ if (p)
+ install_packed_git(the_repository, p);
+ }
if (!from_stdin) {
printf("%s\n", sha1_to_hex(hash));
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index a770d92a55..4157e1a134 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -122,6 +122,16 @@ test_expect_success 'transfer.fsckObjects handles odd pack
(index)' '
test_must_fail git -C dst.git index-pack --strict --stdin output &&
+ # Make sure we fail due to bad gitmodules content, not because we
+ # could not read the blob in the first place.
+ grep gitmodulesName output
+'
+
test_expect_success 'fsck detects symlinked .gitmodules file' '
git init symlink &&
(
--
2.17.1.1443.gd58a3f03b7
[PATCH 1/2] prepare_commit_graft: treat non-repository as a noop
The parse_commit_buffer() function consults lookup_commit_graft() to see if we need to rewrite parents. The latter will look at $GIT_DIR/info/grafts. If you're outside of a repository, then this will trigger a BUG() as of b1ef400eec (setup_git_env: avoid blind fall-back to ".git", 2016-10-20). It's probably uncommon to actually parse a commit outside of a repository, but you can see it in action with: cd /not/a/git/repo git index-pack --strict /some/file.pack This works fine without --strict, but the fsck checks will try to parse any commits, triggering the BUG(). We can fix that by teaching the graft code to behave as if there are no grafts when we aren't in a repository. Arguably index-pack (and fsck) are wrong to consider grafts at all. So another solution is to disable grafts entirely for those commands. But given that the graft feature is deprecated anyway, it's not worth even thinking through the ramifications that might have. There is one other corner case I considered here. What should: cd /not/a/git/repo export GIT_GRAFT_FILE=/file/with/grafts git index-pack --strict /some/file.pack do? We don't have a repository, but the user has pointed us directly at a graft file, which we could respect. I believe this case did work that way prior to b1ef400eec. However, fixing it now would be pretty invasive. Back then we would just call into setup_git_env() even without a repository. But these days it actually takes a git_dir argument. So there would be a fair bit of refactoring of the setup code involved. Given the obscurity of this case, plus the fact that grafts are deprecated and probably shouldn't work under index-pack anyway, it's not worth pursuing further. This patch at least un-breaks the common case where you're _not_ using grafts, but we BUG() anyway trying to even find that out. Signed-off-by: Jeff King --- commit.c | 3 +++ t/t5300-pack-object.sh | 6 ++ 2 files changed, 9 insertions(+) diff --git a/commit.c b/commit.c index b0e57cc440..0030e79940 100644 --- a/commit.c +++ b/commit.c @@ -207,6 +207,9 @@ static void prepare_commit_graft(void) if (commit_graft_prepared) return; + if (!startup_info->have_repository) + return; + graft_file = get_graft_file(); read_graft_file(graft_file); /* make sure shallows are read */ diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index 87a590c4a9..2336d09dcc 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -421,6 +421,12 @@ test_expect_success 'index-pack works in non-repo' ' test_path_is_file foo.idx ' +test_expect_success 'index-pack --strict works in non-repo' ' + rm -f foo.idx && + nongit git index-pack --strict ../foo.pack && + test_path_is_file foo.idx +' + test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'index-pack --threads=N or pack.threads=N warns when no pthreads' ' test_must_fail git index-pack --threads=2 2>err && grep ^warning: err >warnings && -- 2.17.1.1443.gd58a3f03b7
[PATCH 0/2] index-pack out-of-repo fixups
Coverity reported that the recently-added call to add_packed_git() in index-pack.c actually does nothing. It's right, and it turns out this is a minor bug in the .gitmodules fsck patches in v2.17.1. I say minor because it errs on the side of complaining too much (so it's not a security problem) and it comes up only in what I can imagine is a pretty obscure case. The second patch here fixes that. But while looking into it, I notice a much older breakage in running "index-pack --strict" outside of a repository entirely. That's fixed by the first patch. [1/2]: prepare_commit_graft: treat non-repository as a noop [2/2]: index-pack: handle --strict checks of non-repo packs builtin/index-pack.c | 8 ++-- commit.c | 3 +++ t/t5300-pack-object.sh | 6 ++ t/t7415-submodule-names.sh | 10 ++ 4 files changed, 25 insertions(+), 2 deletions(-) -Peff
Re: [PATCH v4 9/9] checkout & worktree: introduce checkout.defaultRemote
On 05/31, Ævar Arnfjörð Bjarmason wrote:
> Introduce a checkout.defaultRemote setting which can be used to
> designate a remote to prefer (via checkout.defaultRemote=origin) when
> running e.g. "git checkout master" to mean origin/master, even though
> there's other remotes that have the "master" branch.
>
> I want this because it's very handy to use this workflow to checkout a
> repository and create a topic branch, then get back to a "master" as
> retrieved from upstream:
>
> (
> cd /tmp &&
> rm -rf tbdiff &&
> git clone g...@github.com:trast/tbdiff.git &&
> cd tbdiff &&
> git branch -m topic &&
> git checkout master
> )
>
> That will output:
>
> Branch 'master' set up to track remote branch 'master' from 'origin'.
> Switched to a new branch 'master'
>
> But as soon as a new remote is added (e.g. just to inspect something
> from someone else) the DWIMery goes away:
>
> (
> cd /tmp &&
> rm -rf tbdiff &&
> git clone g...@github.com:trast/tbdiff.git &&
> cd tbdiff &&
> git branch -m topic &&
> git remote add avar g...@github.com:avar/tbdiff.git &&
> git fetch avar &&
> git checkout master
> )
>
> Will output (without the advice output added earlier in this series):
>
> error: pathspec 'master' did not match any file(s) known to git.
>
> The new checkout.defaultRemote config allows me to say that whenever
> that ambiguity comes up I'd like to prefer "origin", and it'll still
> work as though the only remote I had was "origin".
>
> Also adjust the advice.checkoutAmbiguousRemoteBranchName message to
> mention this new config setting to the user, the full output on my
> git.git is now (the last paragraph is new):
>
> $ ./git --exec-path=$PWD checkout master
> error: pathspec 'master' did not match any file(s) known to git.
> hint: The argument 'master' matched more than one remote tracking branch.
> hint: We found 26 remotes with a reference that matched. So we fell back
> hint: on trying to resolve the argument as a path, but failed there too!
> hint:
> hint: Perhaps you meant fully qualify the branch name? E.g. origin/
> hint: instead of ?
> hint:
> hint: If you'd like to always have checkouts of 'master' prefer one
> remote,
> hint: e.g. the 'origin' remote, consider setting
> checkout.defaultRemote=origin
> hint: in your config. See the 'git-config' manual page for details.
>
> I considered splitting this into checkout.defaultRemote and
> worktree.defaultRemote, but it's probably less confusing to break our
> own rules that anything shared between config should live in core.*
> than have two config settings, and I couldn't come up with a short
> name under core.* that made sense (core.defaultRemoteForCheckout?).
I agree that splitting this into two variables would be needlessly
confusing. 'checkout' and 'worktree add' are similar enough in
spirit, that users only setting one of the configuration variables
would end up confused at some point. Because the commands are so
similar, I also feel like it would be okay to break our own rules
here, and use the 'core.defaultRemote' name you suggested (I also
can't come up with anything better in core.* right now).
> See also 70c9ac2f19 ("DWIM "git checkout frotz" to "git checkout -b
> frotz origin/frotz"", 2009-10-18) which introduced this DWIM feature
> to begin with, and 4e85333197 ("worktree: make add
> dwim", 2017-11-26) which added it to git-worktree.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> Documentation/config.txt | 21 -
> Documentation/git-checkout.txt | 9 +
> Documentation/git-worktree.txt | 9 +
> builtin/checkout.c | 15 +++
> checkout.c | 21 -
> checkout.h | 5 -
> t/t2024-checkout-dwim.sh | 12
> t/t2025-worktree-add.sh| 21 +
> 8 files changed, 106 insertions(+), 7 deletions(-)
>
> [snip]
>
> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index ca5fc9c798..8cb77bddeb 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -38,6 +38,15 @@ equivalent to
> $ git checkout -b --track /
>
> +
> +If the branch exists in multiple remotes and one of them is named by
> +the `checkout.defaultRemote` configuration variable, we'll use that
> +one for the purposes of disambiguation, even if the `` isn't
> +unique across all remotes. Set it to
> +e.g. `checkout.defaultRemote=origin` to always checkout remote
> +branches from there if ` is ambiguous but exists on the
s/`/&`/
> +'origin' remote. See also `checkout.defaultRemote` in
> +linkgit:git-config[1].
> ++
> You could omit , in which case the command degenerates to
> "check out the current branch", which is a glorified no-op with
>
Re: [PATCH v4 9/9] checkout & worktree: introduce checkout.defaultRemote
Hi Ævar,
Sorry for chiming in late. I have a couple of thoughts:
> (
> cd /tmp &&
> rm -rf tbdiff &&
> git clone g...@github.com:trast/tbdiff.git &&
> cd tbdiff &&
> git branch -m topic &&
> git checkout master
> )
>
> That will output:
>
> Branch 'master' set up to track remote branch 'master' from 'origin'.
> Switched to a new branch 'master'
I thought master is already there after the clone operation and
you'd merely switch back to the local branch that was created at
clone time?
$ git clone g...@github.com:trast/tbdiff.git && cd tbdiff
$ git branch
* master
$ cat .git/config
...
[branch "master"]
remote = origin
merge = refs/heads/master
But the observation is right, we get that message. When do we
do the setup for the master branch specifically?
>
> But as soon as a new remote is added (e.g. just to inspect something
> from someone else) the DWIMery goes away:
>
> (
> cd /tmp &&
> rm -rf tbdiff &&
> git clone g...@github.com:trast/tbdiff.git &&
> cd tbdiff &&
> git branch -m topic &&
> git remote add avar g...@github.com:avar/tbdiff.git &&
> git fetch avar &&
> git checkout master
> )
>
> Will output (without the advice output added earlier in this series):
>
> error: pathspec 'master' did not match any file(s) known to git.
>
> The new checkout.defaultRemote config allows me to say that whenever
> that ambiguity comes up I'd like to prefer "origin", and it'll still
> work as though the only remote I had was "origin".
>
> Also adjust the advice.checkoutAmbiguousRemoteBranchName message to
> mention this new config setting to the user, the full output on my
> git.git is now (the last paragraph is new):
>
> $ ./git --exec-path=$PWD checkout master
> error: pathspec 'master' did not match any file(s) known to git.
> hint: The argument 'master' matched more than one remote tracking branch.
> hint: We found 26 remotes with a reference that matched. So we fell back
> hint: on trying to resolve the argument as a path, but failed there too!
> hint:
> hint: Perhaps you meant fully qualify the branch name? E.g. origin/
s/meant fully/meant to fully/
s/? E.g./?\nFor example/
> hint: instead of ?
In builtin/submodule--helper.c there is get_default_remote() which also
hardcodes "origin". I think that is a safe thing to do.
> hint:
> hint: If you'd like to always have checkouts of 'master' prefer one
> remote,
> hint: e.g. the 'origin' remote, consider setting
> checkout.defaultRemote=origin
> hint: in your config. See the 'git-config' manual page for details.
his new setting elevates one remote over all others, which may
be enough for most setups and not confusing, too.
Consider the following:
git clone https://kernel.googlesource.com/pub/scm/git/git && cd git
git remote add gitster https://github.com/gitster/git
git remote add interesting-patches https://github.com/avar/git
git remote add my-github https://github.com/stefanbeller/git
git checkout master
This probably means I want to have origin/master (from kernel.org)
git checkout ab/checkout-implicit-remote
This probably wants to have it from gitster/ (as it is not found on kernel.org);
I am not sure if it would want to look at interesting-patches/ that mirrors
github, but probably if it were not to be found at gitster.
So maybe we rather want a setup to give a defined priority for
the search order:
git config dwim.remoteSearchOrder origin gitster avar
Stepping back a bit, there is already an order in the config file
for the remotes, and that order is used for example for 'fetch --all'.
I wonder if we want to take that order? (Or are the days of hand
editing the config over and this is too arcane? We would need a
config command to re order remotes). Then we could just have a
boolean switch to use the config order on ambiguity.
Although you might want to have a different order for fetching
and looking for the right checkout.
> I considered splitting this into checkout.defaultRemote and
> worktree.defaultRemote, but it's probably less confusing to break our
> own rules that anything shared between config should live in core.*
> than have two config settings, and I couldn't come up with a short
> name under core.* that made sense (core.defaultRemoteForCheckout?).
core.dwimRemote ? It's a bit cryptic, though.
> See also 70c9ac2f19 ("DWIM "git checkout frotz" to "git checkout -b
> frotz origin/frotz"", 2009-10-18) which introduced this DWIM feature
> to begin with, and 4e85333197 ("worktree: make add
> dwim", 2017-11-26) which added it to git-worktree.
Stefan
Re: [PATCH v4 3/9] checkout.[ch]: move struct declaration into *.h
On 05/31, Ævar Arnfjörð Bjarmason wrote:
> Move the tracking_name_data struct used in checkout.c into its
> corresponding header file. This wasn't done in 7c85a87c54 ("checkout:
> factor out functions to new lib file", 2017-11-26) when checkout.[ch]
> were created, and is more consistent with the rest of the codebase.
We seem to have plenty of structs defined in '.c' files, if they are
only needed there. Its use also seems to be single purpose for the
callback data, so I'm a bit puzzled how having this in a header file
instead of the .c file would be helpful?
I feel like having only the "public" part in the header file also
helps developers that are just looking for documentation of the
functions they are looking at, by having less things to go through,
that they wouldn't necessarily care about.
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> checkout.c | 7 ---
> checkout.h | 7 +++
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/checkout.c b/checkout.c
> index bdefc888ba..8d68f75ad1 100644
> --- a/checkout.c
> +++ b/checkout.c
> @@ -3,13 +3,6 @@
> #include "refspec.h"
> #include "checkout.h"
>
> -struct tracking_name_data {
> - /* const */ char *src_ref;
> - char *dst_ref;
> - struct object_id *dst_oid;
> - int unique;
> -};
> -
> static int check_tracking_name(struct remote *remote, void *cb_data)
> {
> struct tracking_name_data *cb = cb_data;
> diff --git a/checkout.h b/checkout.h
> index 4cd4cd1c23..04b52f9ffe 100644
> --- a/checkout.h
> +++ b/checkout.h
> @@ -3,6 +3,13 @@
>
> #include "cache.h"
>
> +struct tracking_name_data {
> + /* const */ char *src_ref;
> + char *dst_ref;
> + struct object_id *dst_oid;
> + int unique;
> +};
> +
> /*
> * Check if the branch name uniquely matches a branch name on a remote
> * tracking branch. Return the name of the remote if such a branch
> --
> 2.17.0.290.gded63e768a
>
wir bieten 2% Kredite
Sehr geehrte Damen und Herren, Sie brauchen Geld? Sie sind auf der suche nach einem Darlehen? Seriös und unkompliziert? Dann sind Sie hier bei uns genau richtig. Durch unsere jahrelange Erfahrung und kompetente Beratung sind wir Europaweit tätig. Wir bieten jedem ein GÜNSTIGES Darlehen zu TOP Konditionen an. Darlehnen zwischen 5000 CHF/Euro bis zu 20 Millionen CHF/Euro möglich. Wir erheben dazu 2% Zinssatz. Lassen Sie sich von unserem kompetenten Team beraten. Zögern Sie nicht und kontaktieren Sie mich unter für weitere Infos & Anfragen unter der eingeblendeten Email Adresse. Ich freue mich von Ihnen zu hören.
[PATCH v4 9/9] checkout & worktree: introduce checkout.defaultRemote
Introduce a checkout.defaultRemote setting which can be used to
designate a remote to prefer (via checkout.defaultRemote=origin) when
running e.g. "git checkout master" to mean origin/master, even though
there's other remotes that have the "master" branch.
I want this because it's very handy to use this workflow to checkout a
repository and create a topic branch, then get back to a "master" as
retrieved from upstream:
(
cd /tmp &&
rm -rf tbdiff &&
git clone g...@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git checkout master
)
That will output:
Branch 'master' set up to track remote branch 'master' from 'origin'.
Switched to a new branch 'master'
But as soon as a new remote is added (e.g. just to inspect something
from someone else) the DWIMery goes away:
(
cd /tmp &&
rm -rf tbdiff &&
git clone g...@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git remote add avar g...@github.com:avar/tbdiff.git &&
git fetch avar &&
git checkout master
)
Will output (without the advice output added earlier in this series):
error: pathspec 'master' did not match any file(s) known to git.
The new checkout.defaultRemote config allows me to say that whenever
that ambiguity comes up I'd like to prefer "origin", and it'll still
work as though the only remote I had was "origin".
Also adjust the advice.checkoutAmbiguousRemoteBranchName message to
mention this new config setting to the user, the full output on my
git.git is now (the last paragraph is new):
$ ./git --exec-path=$PWD checkout master
error: pathspec 'master' did not match any file(s) known to git.
hint: The argument 'master' matched more than one remote tracking branch.
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
hint: Perhaps you meant fully qualify the branch name? E.g. origin/
hint: instead of ?
hint:
hint: If you'd like to always have checkouts of 'master' prefer one remote,
hint: e.g. the 'origin' remote, consider setting
checkout.defaultRemote=origin
hint: in your config. See the 'git-config' manual page for details.
I considered splitting this into checkout.defaultRemote and
worktree.defaultRemote, but it's probably less confusing to break our
own rules that anything shared between config should live in core.*
than have two config settings, and I couldn't come up with a short
name under core.* that made sense (core.defaultRemoteForCheckout?).
See also 70c9ac2f19 ("DWIM "git checkout frotz" to "git checkout -b
frotz origin/frotz"", 2009-10-18) which introduced this DWIM feature
to begin with, and 4e85333197 ("worktree: make add
dwim", 2017-11-26) which added it to git-worktree.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/config.txt | 21 -
Documentation/git-checkout.txt | 9 +
Documentation/git-worktree.txt | 9 +
builtin/checkout.c | 15 +++
checkout.c | 21 -
checkout.h | 5 -
t/t2024-checkout-dwim.sh | 12
t/t2025-worktree-add.sh| 21 +
8 files changed, 106 insertions(+), 7 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 08d3e70989..e0d92217ac 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -350,7 +350,10 @@ advice.*::
remote tracking branch on more than one remote in
situations where an unambiguous argument would have
otherwise caused a remote-tracking branch to be
- checked out.
+ checked out. See the `checkout.defaultRemote`
+ configuration variable for how to set a given remote
+ to used by default in some situations where this
+ advice would be printed.
amWorkDir::
Advice that shows the location of the patch file when
linkgit:git-am[1] fails to apply it.
@@ -1105,6 +1108,22 @@ browser..path::
browse HTML help (see `-w` option in linkgit:git-help[1]) or a
working repository in gitweb (see linkgit:git-instaweb[1]).
+checkout.defaultRemote::
+ When you run 'git checkout ' and only have one
+ remote, it may implicitly fall back on checking out and
+ tracking e.g. 'origin/'. This stops working as soon
+ as you have more than one remote with a ''
+ reference. This setting allows for setting the name of a
+ preferred remote that should always win when it comes to
+ disambiguation. The typical use-case is to set this to
+ `origin`.
++
+Currently this is used by linkgit:git-checkout[1] when 'git checkout
+' will checkout the '' branch on
[PATCH v4 8/9] checkout: add advice for ambiguous "checkout "
As the "checkout" documentation describes:
If is not found but there does exist a tracking branch in
exactly one remote (call it ) with a matching name, treat
as equivalent to [...] /
hint: instead of ?
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/config.txt | 7 +++
advice.c | 2 ++
advice.h | 1 +
builtin/checkout.c | 11 +++
t/t2024-checkout-dwim.sh | 14 ++
5 files changed, 35 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7d8383433c..08d3e70989 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -344,6 +344,13 @@ advice.*::
Advice shown when you used linkgit:git-checkout[1] to
move to the detach HEAD state, to instruct how to create
a local branch after the fact.
+ checkoutAmbiguousRemoteBranchName::
+ Advice shown when the argument to
+ linkgit:git-checkout[1] ambiguously resolves to a
+ remote tracking branch on more than one remote in
+ situations where an unambiguous argument would have
+ otherwise caused a remote-tracking branch to be
+ checked out.
amWorkDir::
Advice that shows the location of the patch file when
linkgit:git-am[1] fails to apply it.
diff --git a/advice.c b/advice.c
index 370a56d054..75e7dede90 100644
--- a/advice.c
+++ b/advice.c
@@ -21,6 +21,7 @@ int advice_add_embedded_repo = 1;
int advice_ignored_hook = 1;
int advice_waiting_for_editor = 1;
int advice_graft_file_deprecated = 1;
+int advice_checkout_ambiguous_remote_branch_name = 1;
static int advice_use_color = -1;
static char advice_colors[][COLOR_MAXLEN] = {
@@ -72,6 +73,7 @@ static struct {
{ "ignoredhook", _ignored_hook },
{ "waitingforeditor", _waiting_for_editor },
{ "graftfiledeprecated", _graft_file_deprecated },
+ { "checkoutambiguousremotebranchname",
_checkout_ambiguous_remote_branch_name },
/* make this an alias for backward compatibility */
{ "pushnonfastforward", _push_update_rejected }
diff --git a/advice.h b/advice.h
index 9f5064e82a..4d11d51d43 100644
--- a/advice.h
+++ b/advice.h
@@ -22,6 +22,7 @@ extern int advice_add_embedded_repo;
extern int advice_ignored_hook;
extern int advice_waiting_for_editor;
extern int advice_graft_file_deprecated;
+extern int advice_checkout_ambiguous_remote_branch_name;
int git_default_advice_config(const char *var, const char *value);
__attribute__((format (printf, 1, 2)))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 423e056acd..710369a60a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -22,6 +22,7 @@
#include "resolve-undo.h"
#include "submodule-config.h"
#include "submodule.h"
+#include "advice.h"
static const char * const checkout_usage[] = {
N_("git checkout [] "),
@@ -1269,6 +1270,16 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
if (opts.patch_mode || opts.pathspec.nr) {
int ret = checkout_paths(, new_branch_info.name,
_remotes_matched);
+ if (ret && dwim_remotes_matched > 1 &&
+ advice_checkout_ambiguous_remote_branch_name)
+ advise(_("The argument '%s' matched more than one
remote tracking branch.\n"
+"We found %d remotes with a reference that
matched. So we fell back\n"
+"on trying to resolve the argument as a path,
but failed there too!\n"
+"\n"
+"Perhaps you meant fully qualify the branch
name? E.g. origin/\n"
+"instead of ?"),
+ argv[0],
+ dwim_remotes_matched);
return ret;
} else {
return checkout_branch(, _branch_info);
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 29c1eada17..14735f5bb8 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -77,6 +77,20 @@ test_expect_success 'checkout of branch from multiple
remotes fails #1' '
status_uno_is_clean
'
+test_expect_success 'checkout of branch from multiple remotes fails with
advice' '
+ git checkout -B master &&
+ test_might_fail git branch -D foo &&
+ test_must_fail git checkout foo 2>stderr &&
+ test_branch master &&
+ status_uno_is_clean &&
+ test_i18ngrep "^hint: " stderr &&
+ test_must_fail git -c advice.checkoutAmbiguousRemoteBranchName=false \
+ checkout foo 2>stderr &&
+ test_branch master &&
+ status_uno_is_clean &&
+ test_i18ngrep ! "^hint: " stderr
+'
+
test_expect_success 'checkout of branch from a single remote succeeds #1' '
git checkout
[PATCH v4 3/9] checkout.[ch]: move struct declaration into *.h
Move the tracking_name_data struct used in checkout.c into its
corresponding header file. This wasn't done in 7c85a87c54 ("checkout:
factor out functions to new lib file", 2017-11-26) when checkout.[ch]
were created, and is more consistent with the rest of the codebase.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
checkout.c | 7 ---
checkout.h | 7 +++
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/checkout.c b/checkout.c
index bdefc888ba..8d68f75ad1 100644
--- a/checkout.c
+++ b/checkout.c
@@ -3,13 +3,6 @@
#include "refspec.h"
#include "checkout.h"
-struct tracking_name_data {
- /* const */ char *src_ref;
- char *dst_ref;
- struct object_id *dst_oid;
- int unique;
-};
-
static int check_tracking_name(struct remote *remote, void *cb_data)
{
struct tracking_name_data *cb = cb_data;
diff --git a/checkout.h b/checkout.h
index 4cd4cd1c23..04b52f9ffe 100644
--- a/checkout.h
+++ b/checkout.h
@@ -3,6 +3,13 @@
#include "cache.h"
+struct tracking_name_data {
+ /* const */ char *src_ref;
+ char *dst_ref;
+ struct object_id *dst_oid;
+ int unique;
+};
+
/*
* Check if the branch name uniquely matches a branch name on a remote
* tracking branch. Return the name of the remote if such a branch
--
2.17.0.290.gded63e768a
[PATCH v4 7/9] builtin/checkout.c: use "ret" variable for return
There is no point in doing this right now, but in later change the
"ret" variable will be inspected. This change makes that meaningful
change smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
builtin/checkout.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index ec7cf93b4a..423e056acd 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1266,9 +1266,11 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
}
UNLEAK(opts);
- if (opts.patch_mode || opts.pathspec.nr)
- return checkout_paths(, new_branch_info.name,
- _remotes_matched);
- else
+ if (opts.patch_mode || opts.pathspec.nr) {
+ int ret = checkout_paths(, new_branch_info.name,
+_remotes_matched);
+ return ret;
+ } else {
return checkout_branch(, _branch_info);
+ }
}
--
2.17.0.290.gded63e768a
[PATCH v4 4/9] checkout.[ch]: introduce an *_INIT macro
Add an *_INIT macro for the tracking_name_data similar to what exists
elsewhere in the codebase, e.g. OID_ARRAY_INIT in sha1-array.h. This
will make it more idiomatic in later changes to add more fields to the
struct & its initialization macro.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
checkout.c | 2 +-
checkout.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/checkout.c b/checkout.c
index 8d68f75ad1..629fc1d5c4 100644
--- a/checkout.c
+++ b/checkout.c
@@ -25,7 +25,7 @@ static int check_tracking_name(struct remote *remote, void
*cb_data)
const char *unique_tracking_name(const char *name, struct object_id *oid)
{
- struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
+ struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, _data);
diff --git a/checkout.h b/checkout.h
index 04b52f9ffe..a61ec93e65 100644
--- a/checkout.h
+++ b/checkout.h
@@ -10,6 +10,8 @@ struct tracking_name_data {
int unique;
};
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
+
/*
* Check if the branch name uniquely matches a branch name on a remote
* tracking branch. Return the name of the remote if such a branch
--
2.17.0.290.gded63e768a
[PATCH v4 1/9] checkout tests: index should be clean after dwim checkout
Assert that whenever there's a DWIM checkout that the index should be
clean afterwards, in addition to the correct branch being checked-out.
The way the DWIM checkout code in checkout.[ch] works is by looping
over all remotes, and for each remote trying to find if a given
reference name only exists on that remote, or if it exists anywhere
else.
This is done by starting out with a `unique = 1` tracking variable in
a struct shared by the entire loop, which will get set to `0` if the
data reference is not unique.
Thus if we find a match we know the dst_oid member of
tracking_name_data must be correct, since it's associated with the
only reference on the only remote that could have matched our query.
But if there was ever a mismatch there for some reason we might end up
with the correct branch checked out, but at the wrong oid, which would
show whatever the difference between the two staged in the
index (checkout branch A, stage changes from the state of branch B).
So let's amend the tests (mostly added in) 399e4a1c56 ("t2024: Add
tests verifying current DWIM behavior of 'git checkout '",
2013-04-21) to always assert that "status" is clean after we run
"checkout", that's being done with "-uno" because there's going to be
some untracked files related to the test itself which we don't care
about.
Then if we ever run into this sort of regression, either in the
existing code or with a new feature, we'll know.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t2024-checkout-dwim.sh | 32 +++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 3e5ac81bd2..29c1eada17 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -23,6 +23,12 @@ test_branch_upstream () {
test_cmp expect.upstream actual.upstream
}
+status_uno_is_clean() {
+ >status.expect &&
+ git status -uno --porcelain >status.actual &&
+ test_cmp status.expect status.actual
+}
+
test_expect_success 'setup' '
test_commit my_master &&
git init repo_a &&
@@ -55,6 +61,7 @@ test_expect_success 'checkout of non-existing branch fails' '
test_might_fail git branch -D xyzzy &&
test_must_fail git checkout xyzzy &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/xyzzy &&
test_branch master
'
@@ -64,8 +71,10 @@ test_expect_success 'checkout of branch from multiple
remotes fails #1' '
test_might_fail git branch -D foo &&
test_must_fail git checkout foo &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/foo &&
- test_branch master
+ test_branch master &&
+ status_uno_is_clean
'
test_expect_success 'checkout of branch from a single remote succeeds #1' '
@@ -73,6 +82,7 @@ test_expect_success 'checkout of branch from a single remote
succeeds #1' '
test_might_fail git branch -D bar &&
git checkout bar &&
+ status_uno_is_clean &&
test_branch bar &&
test_cmp_rev remotes/repo_a/bar HEAD &&
test_branch_upstream bar repo_a bar
@@ -83,6 +93,7 @@ test_expect_success 'checkout of branch from a single remote
succeeds #2' '
test_might_fail git branch -D baz &&
git checkout baz &&
+ status_uno_is_clean &&
test_branch baz &&
test_cmp_rev remotes/other_b/baz HEAD &&
test_branch_upstream baz repo_b baz
@@ -90,6 +101,7 @@ test_expect_success 'checkout of branch from a single remote
succeeds #2' '
test_expect_success '--no-guess suppresses branch auto-vivification' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D bar &&
test_must_fail git checkout --no-guess bar &&
@@ -99,6 +111,7 @@ test_expect_success '--no-guess suppresses branch
auto-vivification' '
test_expect_success 'setup more remotes with unconventional refspecs' '
git checkout -B master &&
+ status_uno_is_clean &&
git init repo_c &&
(
cd repo_c &&
@@ -128,27 +141,33 @@ test_expect_success 'setup more remotes with
unconventional refspecs' '
test_expect_success 'checkout of branch from multiple remotes fails #2' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D bar &&
test_must_fail git checkout bar &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/bar &&
test_branch master
'
test_expect_success 'checkout of branch from multiple remotes fails #3' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D baz &&
test_must_fail git checkout baz &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/baz &&
test_branch master
'
test_expect_success 'checkout of branch from a single remote
[PATCH v4 0/9] ambiguous checkout UI & checkout.defaultRemote
v4 started as a simple bug-fix for this one-part series, but since it's not going to make 2.18.0 at this point I thought I'd do some more work on it. Comments on patches below: Ævar Arnfjörð Bjarmason (9): checkout tests: index should be clean after dwim checkout Tests that would have revealed the bug in v3. checkout.h: wrap the arguments to unique_tracking_name() checkout.[ch]: move struct declaration into *.h Boring moving code around. checkout.[ch]: introduce an *_INIT macro Make checkout.h have a TRACKING_NAME_DATA_INIT for its struct. checkout.[ch]: change "unique" member to "num_matches" checkout: pass the "num_matches" up to callers builtin/checkout.c: use "ret" variable for return Refactoring with no changes yet to make subsequent changes smaller. checkout: add advice for ambiguous "checkout " Even if checkout.defaultRemote is off we now print advice telling the user why their "git checkout branch" didn't work. checkout & worktree: introduce checkout.defaultRemote It's now called checkout.defaultRemote not checkout.implicitRemote on Junio's suggestion. On reflection that's better. Improved tests for git-worktree (similar to the dwim checkout tests improvements earlier), and the the documentation for git-checkout & git-worktree. I'm omitting the tbdiff because most of it's because of the new patches in this series. Better just to read them. Documentation/config.txt | 26 +++ Documentation/git-checkout.txt | 9 ++ Documentation/git-worktree.txt | 9 ++ advice.c | 2 ++ advice.h | 1 + builtin/checkout.c | 44 -- builtin/worktree.c | 4 +-- checkout.c | 37 +++--- checkout.h | 16 +- t/t2024-checkout-dwim.sh | 58 +- t/t2025-worktree-add.sh| 21 11 files changed, 203 insertions(+), 24 deletions(-) -- 2.17.0.290.gded63e768a
[PATCH v4 6/9] checkout: pass the "num_matches" up to callers
Pass the previously added "num_matches" struct value up to the callers
of unique_tracking_name(). This will allow callers to optionally print
better error messages in a later change.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
builtin/checkout.c | 16 +++-
builtin/worktree.c | 4 ++--
checkout.c | 5 -
checkout.h | 3 ++-
4 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2e1d2376d2..ec7cf93b4a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -239,7 +239,8 @@ static int checkout_merged(int pos, const struct checkout
*state)
}
static int checkout_paths(const struct checkout_opts *opts,
- const char *revision)
+ const char *revision,
+ int *dwim_remotes_matched)
{
int pos;
struct checkout state = CHECKOUT_INIT;
@@ -878,7 +879,8 @@ static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new_branch_info,
struct checkout_opts *opts,
- struct object_id *rev)
+ struct object_id *rev,
+ int *dwim_remotes_matched)
{
struct tree **source_tree = >source_tree;
const char **new_branch = >new_branch;
@@ -972,7 +974,8 @@ static int parse_branchname_arg(int argc, const char **argv,
recover_with_dwim = 0;
if (recover_with_dwim) {
- const char *remote = unique_tracking_name(arg, rev);
+ const char *remote = unique_tracking_name(arg, rev,
+
dwim_remotes_matched);
if (remote) {
*new_branch = arg;
arg = remote;
@@ -1109,6 +1112,7 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
struct branch_info new_branch_info;
char *conflict_style = NULL;
int dwim_new_local_branch = 1;
+ int dwim_remotes_matched = 0;
struct option options[] = {
OPT__QUIET(, N_("suppress progress reporting")),
OPT_STRING('b', NULL, _branch, N_("branch"),
@@ -1219,7 +1223,8 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
opts.track == BRANCH_TRACK_UNSPECIFIED &&
!opts.new_branch;
int n = parse_branchname_arg(argc, argv, dwim_ok,
-_branch_info, , );
+_branch_info, , ,
+_remotes_matched);
argv += n;
argc -= n;
}
@@ -1262,7 +1267,8 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
UNLEAK(opts);
if (opts.patch_mode || opts.pathspec.nr)
- return checkout_paths(, new_branch_info.name);
+ return checkout_paths(, new_branch_info.name,
+ _remotes_matched);
else
return checkout_branch(, _branch_info);
}
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5c7d2bb180..a763dbdccb 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -412,7 +412,7 @@ static const char *dwim_branch(const char *path, const char
**new_branch)
if (guess_remote) {
struct object_id oid;
const char *remote =
- unique_tracking_name(*new_branch, );
+ unique_tracking_name(*new_branch, , NULL);
return remote;
}
return NULL;
@@ -484,7 +484,7 @@ static int add(int ac, const char **av, const char *prefix)
commit = lookup_commit_reference_by_name(branch);
if (!commit) {
- remote = unique_tracking_name(branch, );
+ remote = unique_tracking_name(branch, , NULL);
if (remote) {
new_branch = branch;
branch = remote;
diff --git a/checkout.c b/checkout.c
index 7ce5306bc7..c578782baa 100644
--- a/checkout.c
+++ b/checkout.c
@@ -23,12 +23,15 @@ static int check_tracking_name(struct remote *remote, void
*cb_data)
return 0;
}
-const char *unique_tracking_name(const char *name, struct object_id *oid)
+const char *unique_tracking_name(const char *name, struct object_id *oid,
+int *dwim_remotes_matched)
{
struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, _data);
+ if
[PATCH v4 2/9] checkout.h: wrap the arguments to unique_tracking_name()
The line was too long already, and will be longer still when a later change adds another argument. Signed-off-by: Ævar Arnfjörð Bjarmason --- checkout.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/checkout.h b/checkout.h index 9980711179..4cd4cd1c23 100644 --- a/checkout.h +++ b/checkout.h @@ -8,6 +8,7 @@ * tracking branch. Return the name of the remote if such a branch * exists, NULL otherwise. */ -extern const char *unique_tracking_name(const char *name, struct object_id *oid); +extern const char *unique_tracking_name(const char *name, + struct object_id *oid); #endif /* CHECKOUT_H */ -- 2.17.0.290.gded63e768a
[PATCH v4 5/9] checkout.[ch]: change "unique" member to "num_matches"
Internally track how many matches we find in the check_tracking_name()
callback. Nothing uses this now, but it will be made use of in a later
change.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
checkout.c | 4 ++--
checkout.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/checkout.c b/checkout.c
index 629fc1d5c4..7ce5306bc7 100644
--- a/checkout.c
+++ b/checkout.c
@@ -14,9 +14,9 @@ static int check_tracking_name(struct remote *remote, void
*cb_data)
free(query.dst);
return 0;
}
+ cb->num_matches++;
if (cb->dst_ref) {
free(query.dst);
- cb->unique = 0;
return 0;
}
cb->dst_ref = query.dst;
@@ -30,7 +30,7 @@ const char *unique_tracking_name(const char *name, struct
object_id *oid)
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, _data);
free(cb_data.src_ref);
- if (cb_data.unique)
+ if (cb_data.num_matches == 1)
return cb_data.dst_ref;
free(cb_data.dst_ref);
return NULL;
diff --git a/checkout.h b/checkout.h
index a61ec93e65..2decb9b820 100644
--- a/checkout.h
+++ b/checkout.h
@@ -7,10 +7,10 @@ struct tracking_name_data {
/* const */ char *src_ref;
char *dst_ref;
struct object_id *dst_oid;
- int unique;
+ int num_matches;
};
-#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
/*
* Check if the branch name uniquely matches a branch name on a remote
--
2.17.0.290.gded63e768a
Re: [GSoC][PATCH 0/2] rebase -i: rewrite append_todo_help() in C
On Thu, May 31, 2018 at 12:33 PM, Alban Gruin wrote: > Hi Stefan, > > Le 31/05/2018 à 20:44, Stefan Beller a écrit : >> On Thu, May 31, 2018 at 10:48 AM, Phillip Wood >> wrote: >>> Hi Alban, it's great to see you working on this >>> >>> On 31/05/18 12:01, Alban Gruin wrote: This series rewrites append_todo_help() from shell to C. This is part of the effort to rewrite interactive rebase in C. The first commit rewrites append_todo_help() in C (the C version covers a bit more than the old shell version), adds some parameters to rebase--helper, etc. >>> >>> I've had a read of the first patch and I think it looks fine, my only >>> comment would be that the help for '--edit-todo' is a bit misleading at >>> the moment as currently it's just a flag to tell rebase-helper that the >>> todo list is being edited rather than actually implementing the >>> functionality to edit the list (but hopefully that will follow in the >>> future). >> >> Would you have better suggestions for the name of the flag? >> Of the top of my head: >> --write-edit-todo >> --hint-todo-edit >> --include-todo-edit-hint >> not sure I like these names, though they seem to reflect the >> nature of that flag a little bit better. >> > > As my next patch series will probably be about rewriting edit-todo in C, > do you really think I should rename the flag? If you reroll, you could think of doing that. If you have the next series prepared already that build on top, it may not be worth it. >> If you feel strongly, I'd rather see Alban drop this second patch and >> move on instead of waiting for our argument to settle. ( I do not feel >> strongly about it, but put it out as a suggestion as that seemed like >> it would lead to a better end state for the project). >> > > Okay, so I drop this patch and reroll the other? Sure, but maybe give Philip some time to react?
Re: [RFC PATCH 1/6] DO NOT MERGE: compute commit-graph on every commit
On Thu, May 31, 2018 at 10:41 AM, Derrick Stolee wrote:
> Also enable core.commitGraph and gc.commitGraph. Helps to test the
> commit-graph feature with the rest of the test infrastructure.
>
> Signed-off-by: Derrick Stolee
> ---
> builtin/commit.c | 5 +
> builtin/gc.c | 2 +-
> environment.c| 2 +-
> 3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index e8e8d13be4..8751b816c1 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -32,6 +32,7 @@
> #include "column.h"
> #include "sequencer.h"
> #include "mailmap.h"
> +#include "commit-graph.h"
>
> static const char * const builtin_commit_usage[] = {
> N_("git commit [] [--] ..."),
> @@ -1623,5 +1624,9 @@ int cmd_commit(int argc, const char **argv, const char
> *prefix)
>
> UNLEAK(err);
> UNLEAK(sb);
> +
> + if (core_commit_graph)
> + write_commit_graph_reachable(get_object_directory(), 1);
> +
I'd personally put it before the UNLEAKS, as then
we have the cleanup at the end of the function and
not scattered somewhere in the middle.
Re: [GSoC][PATCH 0/2] rebase -i: rewrite append_todo_help() in C
Hi Stefan, Le 31/05/2018 à 20:44, Stefan Beller a écrit : > On Thu, May 31, 2018 at 10:48 AM, Phillip Wood > wrote: >> Hi Alban, it's great to see you working on this >> >> On 31/05/18 12:01, Alban Gruin wrote: >>> This series rewrites append_todo_help() from shell to C. This is part >>> of the effort to rewrite interactive rebase in C. >>> >>> The first commit rewrites append_todo_help() in C (the C version >>> covers a bit more than the old shell version), adds some parameters to >>> rebase--helper, etc. >> >> I've had a read of the first patch and I think it looks fine, my only >> comment would be that the help for '--edit-todo' is a bit misleading at >> the moment as currently it's just a flag to tell rebase-helper that the >> todo list is being edited rather than actually implementing the >> functionality to edit the list (but hopefully that will follow in the >> future). > > Would you have better suggestions for the name of the flag? > Of the top of my head: > --write-edit-todo > --hint-todo-edit > --include-todo-edit-hint > not sure I like these names, though they seem to reflect the > nature of that flag a little bit better. > As my next patch series will probably be about rewriting edit-todo in C, do you really think I should rename the flag? > If you feel strongly, I'd rather see Alban drop this second patch and > move on instead of waiting for our argument to settle. ( I do not feel > strongly about it, but put it out as a suggestion as that seemed like > it would lead to a better end state for the project). > Okay, so I drop this patch and reroll the other? Cheers, Alban
Re: [RFC PATCH 5/6] fetch: destroy commit graph on shallow parameters
On Thu, May 31, 2018 at 10:41 AM, Derrick Stolee wrote:
> The commit-graph feature is not compatible with history-rewriting
> features such as shallow clones.
I associate "history rewriting" with changing objects in the history.
For example interactive rebase or the BFG cleaner[1] / filter-branch
to remove certain commits from other commits as parents.
This history rewriting leads to different sha1s, and the commit
graph feature is compatible with that in the sense that you can
add all the new sha1s /commits to the graph and prune out the
old unreferenced commits.
Shallow clones are not rewriting history IMHO, as the sha1s do
not change. What changes is the assumption of presence of
the parent commits (which makes it hard to compute the
generation number), by the grafting trick, that "overlays" (?)
history.
This is more of a nit, though.
[1] https://rtyley.github.io/bfg-repo-cleaner/
> When running a 'git fetch' with
> any of the shallow/unshallow options, destroy the commit-graph
> file and override core.commitGraph to be false.
We do that *before* the actual fetch happens such that
the improved negotiation of the future cannot even try to
benefit from generation numbers?
We do it at fetch time instead of other local operations
as that is an entry point to commit-graph incompatible
features. Would this also be needed in clone?
I was about to ask if a more fine grained inclusion to
lookup_commit would make sense, but that is explicitely
called out in the cover letter as 'too hard for now'.
>
> Signed-off-by: Derrick Stolee
> ---
> builtin/fetch.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index af896e4b74..2001dca881 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1452,6 +1452,12 @@ int cmd_fetch(int argc, const char **argv, const char
> *prefix)
> }
> }
>
> + if (update_shallow || depth || deepen_since || deepen_not.nr ||
> + deepen_relative || unshallow || update_shallow ||
> is_repository_shallow()) {
> + destroy_commit_graph(get_object_directory());
> + core_commit_graph = 0;
Re: [GSoC][PATCH 0/2] rebase -i: rewrite append_todo_help() in C
Hi Phillip, Le 31/05/2018 à 19:48, Phillip Wood a écrit : > Hi Alban, it's great to see you working on this > > On 31/05/18 12:01, Alban Gruin wrote: >> This series rewrites append_todo_help() from shell to C. This is part >> of the effort to rewrite interactive rebase in C. >> >> The first commit rewrites append_todo_help() in C (the C version >> covers a bit more than the old shell version), adds some parameters to >> rebase--helper, etc. > > I've had a read of the first patch and I think it looks fine, my only > comment would be that the help for '--edit-todo' is a bit misleading at > the moment as currently it's just a flag to tell rebase-helper that the > todo list is being edited rather than actually implementing the > functionality to edit the list Right, what do you think about something like “appends the edit-todo message to the todo list”? > (but hopefully that will follow in the > future). > This is the next step :) Cheers, Alban
Re: [PATCH v2 2/2] note git-secur...@googlegroups.com in more places
On 05/30, brian m. carlson wrote:
> On Wed, May 30, 2018 at 09:52:55PM +0100, Thomas Gummerer wrote:
> > Add a mention of the security mailing list to the README, and to
> > Documentation/SubmittingPatches.. 2caa7b8d27 ("git manpage: note
> > git-secur...@googlegroups.com", 2018-03-08) already added it to the
> > man page, but for developers either the README, or the documentation
> > on how to contribute (SubmittingPatches) may be the first place to
> > look.
> >
> > Use the same wording as we already have on the git-scm.com website and
> > in the man page for the README, while the wording is adjusted in
> > SubmittingPatches to match the surrounding document better.
> >
> > Signed-off-by: Thomas Gummerer
> > ---
> > Documentation/SubmittingPatches | 13 +
> > README.md | 3 +++
> > 2 files changed, 16 insertions(+)
> >
> > diff --git a/Documentation/SubmittingPatches
> > b/Documentation/SubmittingPatches
> > index 27553128f5..c8f9deb391 100644
> > --- a/Documentation/SubmittingPatches
> > +++ b/Documentation/SubmittingPatches
> > @@ -176,6 +176,12 @@ that is fine, but please mark it as such.
> > [[send-patches]]
> > === Sending your patches.
> >
> > +:security-ml: footnoteref:[security-ml,The Git Security mailing list:
> > git-secur...@googlegroups.com]
> > +
> > +Before sending any patches, please note that patches that may be
> > +security relevant should be submitted privately to the Git Security
> > +mailing list{security-ml}, instead of the public mailing list.
> > +
> > Learn to use format-patch and send-email if possible. These commands
> > are optimized for the workflow of sending patches, avoiding many ways
> > your existing e-mail client that is optimized for "multipart/*" mime
> > @@ -259,6 +265,13 @@ patch, format it as "multipart/signed", not a
> > text/plain message
> > that starts with `-BEGIN PGP SIGNED MESSAGE-`. That is
> > not a text/plain, it's something else.
> >
> > +:security-ml-ref: footnoteref:[security-ml]
>
> My only feedback here is that using the footnoteref syntax to refer to
> the previous footnote potentially makes this a little less readable for
> plain text users, although it also reduces duplication. I'm not sure I
> feel strongly one way or the other on this.
Yeah, using the plain footnote syntax we end up with two footnotes
that are exactly the same, which felt a little awkward. But I don't
feel strongly either, so if the consensus is to duplicate the footnote
for better readability in plain text I'm happy to change that.
To really improve the readability we'd probably have to duplicate the
attribute as well, which I wanted to avoid (altough it's not
completely possible with the footnoteref syntax either).
> Otherwise, this looked fine to me.
> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204
Re: [RFC PATCH 4/6] commit-graph: avoid writing when repo is shallow
On Thu, May 31, 2018 at 10:41 AM, Derrick Stolee wrote:
> Shallow clones do not interact well with the commit-graph feature for
> several reasons. Instead of doing the hard thing to fix those
> interactions, instead prevent reading or writing a commit-graph file for
> shallow repositories.
Makes sense.
>
> Signed-off-by: Derrick Stolee
> ---
> commit-graph.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 95af4ed519..80e377b90f 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -208,6 +208,9 @@ static void prepare_commit_graph(void)
> return;
> prepare_commit_graph_run_once = 1;
>
> + if (is_repository_shallow())
> + return;
> +
> obj_dir = get_object_directory();
> prepare_commit_graph_one(obj_dir);
> prepare_alt_odb();
> @@ -711,6 +714,15 @@ void write_commit_graph(const char *obj_dir,
> int num_extra_edges;
> struct commit_list *parent;
>
> + /*
> +* Shallow clones are not supproted, as they create bad
supported
> +* generation skips as they are un-shallowed.
> +*/
> + if (is_repository_shallow()) {
> + warning("writing a commit-graph in a shallow repository is
> not supported");
_() ?
Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote
On Thu, May 31, 2018 at 5:07 AM, Johannes Schindelin
wrote:
> Hi Stefan,
>
> On Wed, 30 May 2018, Stefan Beller wrote:
>
>> Signed-off-by: Stefan Beller
>> ---
>> builtin/submodule--helper.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 7c3cd9dbeba..96024fee1b1 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char
>> **argv, const char *prefix)
>> if (remote)
>> printf("%s\n", remote);
>>
>> + free(remote);
>
> Makes sense.
>
> Out of curiosity (and because a cover letter is missing): how did you
> stumble over these? Coverity?
Yes I found them on coverity as I wanted to find out how bad their
false positives are these days. So I looked at the most recent findings.
I somehow imagined that we could redefine the _INIT macros which
usually lead to false positives (just alloc memory instead of
pointing them all at the same memory for _INIT), but that experiment
did not work out.
Thanks,
Stefan
Re: [PATCH 2/3] sequencer.c: free author variable when merging fails
On Thu, May 31, 2018 at 5:04 AM, Johannes Schindelin
wrote:
> Hi Stefan,
>
> On Wed, 30 May 2018, Stefan Beller wrote:
>
>> Signed-off-by: Stefan Beller
>> ---
>>
>> This was a deliberate oversight in f241ff0d0a9 (prepare the builtins for a
>> libified merge_recursive(), 2016-07-26)
>
> No, it was not deliberate. It was inadvertent, most likely ;-)
ok, I am not just bad at writing commit messages, but
also bad at reading other peoples commit messages. ;)
"As this patch is already complex enough, we
leave that change for a later patch." is what lead me to
believe it was deliberate.
>> - if (res < 0)
>> + if (res < 0) {
>> + free(author);
>> return res;
>
> Why not `goto leave;` instead? I wonder what is happening to the commit
> message: can we be certain at this point that it was not set yet? And
> also: should we call `update_abort_safety_file()`?
I think so, but wasn't sure. I wrote these patches before
my usual morning routine. I'll change that.
Thanks,
Stefan
Re: [GSoC][PATCH 0/2] rebase -i: rewrite append_todo_help() in C
On Thu, May 31, 2018 at 10:48 AM, Phillip Wood wrote: > Hi Alban, it's great to see you working on this > > On 31/05/18 12:01, Alban Gruin wrote: >> This series rewrites append_todo_help() from shell to C. This is part >> of the effort to rewrite interactive rebase in C. >> >> The first commit rewrites append_todo_help() in C (the C version >> covers a bit more than the old shell version), adds some parameters to >> rebase--helper, etc. > > I've had a read of the first patch and I think it looks fine, my only > comment would be that the help for '--edit-todo' is a bit misleading at > the moment as currently it's just a flag to tell rebase-helper that the > todo list is being edited rather than actually implementing the > functionality to edit the list (but hopefully that will follow in the > future). Would you have better suggestions for the name of the flag? Of the top of my head: --write-edit-todo --hint-todo-edit --include-todo-edit-hint not sure I like these names, though they seem to reflect the nature of that flag a little bit better. >> The second one strips newlines from append_todo_help() messages, which >> require to update the translations. This change was advised to me by >> Stefan Beller, but Johannes Schindelin voiced concerns. I don’t really >> have a strong opinion about it, so feel free to give yours. > > I'm not sure I understand what the point of this patch is, if the > newlines are unnecessary then I'd just omit them from the first patch - > am I missing something? > The new lines are part of the output and are currently in the part to be translated: For example from the German translation file: #: git-rebase--interactive.sh:171 msgid "" "\n" "Do not remove any line. Use 'drop' explicitly to remove a commit.\n" msgstr "" "\n" "Keine Zeile entfernen. Benutzen Sie 'drop', um explizit einen Commit zu\n" "entfernen.\n" After patch 2 is applied, the translators only see "Do not remove any line. Use 'drop' explicitly to remove a commit." as a need to translate, and the two additional new lines (one in front and one after the string) are just put in place autormatically. Usually we do not want to play sentence lego, but this is a whole sentence for translation; it is rather about formatting the output for the terminal, adding new lines to separate some messages. I thought this patch would just show goodwill towards translators that do not need to replicate the formatting exactly. If you feel strongly, I'd rather see Alban drop this second patch and move on instead of waiting for our argument to settle. ( I do not feel strongly about it, but put it out as a suggestion as that seemed like it would lead to a better end state for the project). Thanks, Stefan
Re: Bug: Install from .tar.xz fails without write permission on /usr/local/share/man/man3
Hi, On Thu, May 31, 2018 at 6:30 PM, wrote: > > I was trying to build git 2.9.5 as a normal user, as I have no root access > on a cluster with outdated software. > > The build fails, unless I change the PREFIX=/usr/local line in > per/perl.mak:80 to a folder where I have write permission. > Apparently, perl.mak does not honour the --prefix= setting of ./configure. > > Is it possible to change perl.mak to honor the PREFIX? I don't think we will support old versions like v2.9.X. There was a security release and we only released v2.17.1, v2.13.7, v2.14.4, v2.15.2 and v2.16.4: https://public-inbox.org/git/xmqqy3g2flb6@gitster-ct.c.googlers.com/ So it looks like v2.13.X is the oldest version we support. Do you really need v2.9.5? Best, Christian.
Re: [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo
On Thu, May 31, 2018 at 10:40 AM, Derrick Stolee wrote: > The commit-graph file stores a condensed version of the commit history. > This helps speed up several operations involving commit walks. This > feature does not work well if those commits "change" using features like > commit grafts, replace objects, or shallow clones. > > Since the commit-graph feature is optional, hidden behind the > 'core.commitGraph' config option, and requires manual maintenance (until > ds/commit-graph-fsck' is merged), these issues only arise for expert > users who decided to opt-in. > > This RFC is a first attempt at rectify the issues that come up when > these features interact. I have not succeeded entirely, as I will > discuss below. > > The first two "DO NOT MERGE" commits are not intended to be part of the > main product, but are ways to help the full test suite run while > computing and consuming commit-graph files. This is acheived by calling > write_commit_graph_reachable() from `git fetch` and `git commit`. I > believe this is the only dependence on ds/commit-graph-fsck. The other > commits should only depend on ds/commit-graph-lockfile-fix. I applied these patches on top of ds/commit-graph-fsck (it would have been nice if you mentioned that it applies there) Running the test suite with all patches applied results in: ./t0410-partial-clone.sh(Wstat: 256 Tests: 15 Failed: 2) Failed tests: 5, 8 ./t5307-pack-missing-commit.sh (Wstat: 256 Tests: 5 Failed: 2) Failed tests: 3-4 ./t5500-fetch-pack.sh (Wstat: 256 Tests: 353 Failed: 1) Failed test: 348 ./t6011-rev-list-with-bad-commit.sh (Wstat: 256 Tests: 6 Failed: 1) Failed test: 4 ./t6024-recursive-merge.sh (Wstat: 256 Tests: 6 Failed: 1) Failed test: 4 which you identified as 4x noise and t5500 as not understood. $ GIT_TRACE=1 ./t5500-fetch-pack.sh -d -i -v -x [...] expecting success: git -C shallow12 fetch --shallow-exclude one origin && git -C shallow12 log --pretty=tformat:%s origin/master >actual && test_write_lines three two >expected && test_cmp expected actual ++ git -C shallow12 fetch --shallow-exclude one origin trace: built-in: git fetch --shallow-exclude one origin trace: run_command: unset GIT_PREFIX; 'git-upload-pack '\''/u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.'\''' trace: run_command: git --shallow-file pack-objects --revs --thin --stdout --shallow --progress --delta-base-offset --include-tag trace: built-in: git pack-objects --revs --thin --stdout --shallow --progress --delta-base-offset --include-tag remote: Counting objects: 4, done. remote: Compressing objects: 100% (3/3), done. remote: Total 4 (delta 0), reused 0 (delta 0) trace: run_command: git --shallow-file unpack-objects --pack_header=2,4 trace: built-in: git unpack-objects --pack_header=2,4 Unpacking objects: 100% (4/4), done. trace: run_command: git rev-list --objects --stdin --not --all --quiet trace: built-in: git rev-list --objects --stdin --not --all --quiet trace: run_command: unset GIT_PREFIX; 'git-upload-pack '\''/u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.'\''' trace: run_command: git pack-objects --revs --thin --stdout --progress --delta-base-offset trace: built-in: git pack-objects --revs --thin --stdout --progress --delta-base-offset remote: Total 0 (delta 0), reused 0 (delta 0) trace: run_command: git unpack-objects --pack_header=2,0 trace: built-in: git unpack-objects --pack_header=2,0 trace: run_command: git rev-list --objects --stdin --not --all --quiet trace: built-in: git rev-list --objects --stdin --not --all --quiet >From file:///u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/. * [new tag] one-> one * [new tag] two-> two run_processes_parallel: preparing to run up to 1 tasks run_processes_parallel: done trace: run_command: git gc --auto trace: built-in: git gc --auto ++ git -C shallow12 log --pretty=tformat:%s origin/master trace: built-in: git log '--pretty=tformat:%s' origin/master ++ test_write_lines three two ++ printf '%s\n' three two ++ test_cmp expected actual ++ diff -u expected actual --- expected 2018-05-31 18:14:25.944540810 + +++ actual 2018-05-31 18:14:25.944540810 + @@ -1,2 +1,3 @@ three two +one error: last command exited with $?=1 not ok 348 - fetch exclude tag one # # git -C shallow12 fetch --shallow-exclude one origin && # git -C shallow12 log --pretty=tformat:%s origin/master >actual && # test_write_lines three two >expected && # test_cmp expected actual # > After these changes, there is one test case that still fails, and I > cannot understand why: > > t5500-fetch-pack.sh Failed test: 348 > > This test fails when performing a `git fetch --shallow-exclude`. When I > halt the test using `t5500-fetch-pack.sh -d -i` and navigate to the > directory to replay the fetch it performs as expected. What is "as expected" ? When I insert a test_pause before
Re: [GSoC][PATCH 0/2] rebase -i: rewrite append_todo_help() in C
Hi Alban, it's great to see you working on this On 31/05/18 12:01, Alban Gruin wrote: > This series rewrites append_todo_help() from shell to C. This is part > of the effort to rewrite interactive rebase in C. > > The first commit rewrites append_todo_help() in C (the C version > covers a bit more than the old shell version), adds some parameters to > rebase--helper, etc. I've had a read of the first patch and I think it looks fine, my only comment would be that the help for '--edit-todo' is a bit misleading at the moment as currently it's just a flag to tell rebase-helper that the todo list is being edited rather than actually implementing the functionality to edit the list (but hopefully that will follow in the future). > > The second one strips newlines from append_todo_help() messages, which > require to update the translations. This change was advised to me by > Stefan Beller, but Johannes Schindelin voiced concerns. I don’t really > have a strong opinion about it, so feel free to give yours. I'm not sure I understand what the point of this patch is, if the newlines are unnecessary then I'd just omit them from the first patch - am I missing something? Best Wishes Phillip > Alban Gruin (2): > rebase--interactive: rewrite append_todo_help() in C > sequencer: remove newlines from append_todo_help() messages > > builtin/rebase--helper.c | 10 ++-- > git-rebase--interactive.sh | 52 ++--- > sequencer.c| 64 > ++ > sequencer.h| 1 + > 4 files changed, 75 insertions(+), 52 deletions(-) >
Re: is there a reason pre-commit.sample uses "git diff-index"?
On Thu, May 31, 2018 at 7:27 PM, Robert P. J. Day wrote: > On Thu, 31 May 2018, Duy Nguyen wrote: > >> On Thu, May 31, 2018 at 6:38 PM, Robert P. J. Day >> wrote: >> > >> > was going over some hooks and writing some tutorials for some of >> > the commit-related, client-side hooks, and was wondering (perhaps >> > stupidly) why the pre-commit.sample hook uses, as its last line: >> > >> > exec git diff-index --check --cached $against -- >> >^^ >> > >> > as in, could this not be done equivalently with just git diff, not >> > git diff-index? i just did a quick test and it seems to do the >> > same thing, but i've never taken a close look at git diff-index >> > before so i may just be clueless about some important distinction. >> >> git diff-index is "plumbing", designed for writing scripts. "git >> diff" on the other hand is for users and its behavior may change >> even if it breaks backward compatibility. > > ah, this was a philosophical underpinning i was unaware of. i see > occasional explanations of git porcelain versus plumbing, but i don't > recall anyone simply stating that the plumbing is meant to have a > long-term stability that is not guaranteed for the porcelain. > > in any event, this does mean that, stability issues aside, "git > diff" would apparently have worked just fine for that hook. I think there are also stuff like config variables which can change porcelain command behavior but usually not plumbing. Command exit code may be another area where porcelain and plumbing differ. But in this particular case, I think "git diff" works fine (but still should not be used unless you're just writing a quick throwaway script). -- Duy
[RFC PATCH 1/6] DO NOT MERGE: compute commit-graph on every commit
Also enable core.commitGraph and gc.commitGraph. Helps to test the
commit-graph feature with the rest of the test infrastructure.
Signed-off-by: Derrick Stolee
---
builtin/commit.c | 5 +
builtin/gc.c | 2 +-
environment.c| 2 +-
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index e8e8d13be4..8751b816c1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -32,6 +32,7 @@
#include "column.h"
#include "sequencer.h"
#include "mailmap.h"
+#include "commit-graph.h"
static const char * const builtin_commit_usage[] = {
N_("git commit [] [--] ..."),
@@ -1623,5 +1624,9 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
UNLEAK(err);
UNLEAK(sb);
+
+ if (core_commit_graph)
+ write_commit_graph_reachable(get_object_directory(), 1);
+
return 0;
}
diff --git a/builtin/gc.c b/builtin/gc.c
index efd214a59f..999c09d8e2 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -35,7 +35,7 @@ static int aggressive_depth = 50;
static int aggressive_window = 250;
static int gc_auto_threshold = 6700;
static int gc_auto_pack_limit = 50;
-static int gc_commit_graph = 0;
+static int gc_commit_graph = 1;
static int detach_auto = 1;
static timestamp_t gc_log_expire_time;
static const char *gc_log_expire = "1.day.ago";
diff --git a/environment.c b/environment.c
index 8853e2f0dd..fdb2d56d90 100644
--- a/environment.c
+++ b/environment.c
@@ -62,7 +62,7 @@ enum push_default_type push_default =
PUSH_DEFAULT_UNSPECIFIED;
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
char *notes_ref_name;
int grafts_replace_parents = 1;
-int core_commit_graph;
+int core_commit_graph = 1;
int core_apply_sparse_checkout;
int merge_log_config = -1;
int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
--
2.16.2.338.gcfe06ae955
[RFC PATCH 4/6] commit-graph: avoid writing when repo is shallow
Shallow clones do not interact well with the commit-graph feature for
several reasons. Instead of doing the hard thing to fix those
interactions, instead prevent reading or writing a commit-graph file for
shallow repositories.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 12
1 file changed, 12 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index 95af4ed519..80e377b90f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -208,6 +208,9 @@ static void prepare_commit_graph(void)
return;
prepare_commit_graph_run_once = 1;
+ if (is_repository_shallow())
+ return;
+
obj_dir = get_object_directory();
prepare_commit_graph_one(obj_dir);
prepare_alt_odb();
@@ -711,6 +714,15 @@ void write_commit_graph(const char *obj_dir,
int num_extra_edges;
struct commit_list *parent;
+ /*
+* Shallow clones are not supproted, as they create bad
+* generation skips as they are un-shallowed.
+*/
+ if (is_repository_shallow()) {
+ warning("writing a commit-graph in a shallow repository is not
supported");
+ return;
+ }
+
oids.nr = 0;
oids.alloc = approximate_object_count() / 4;
--
2.16.2.338.gcfe06ae955
[RFC PATCH 6/6] commit-graph: revert to odb on missing parents
The commit-graph format includes a way to specify a parent is
"missing" from the commit-graph (i.e. we do not have a record of
that parent in our list of object IDs, and hence cannot provide
a graph position). For mose cases, this does not occur due to
the close_reachable() method adding all reachable commits. However,
in a shallow clone, we will try to record the parents of a commit
on the shallow boundary, but the parents are not in the repository.
The GRAPH_PARENT_MISSING value that is stored in the format is
purposeful, especially for future plans to make the commit-graph file
incremental or transporting sections of a commit-graph file across
the network.
In the meantime, check if a commit has a missing parent while filling
its details from the commit-graph. If a parent is missing, still
assign the generation number and graph position for that item, but
report that the commit-graph failed to fill the contents. Then the
caller is responsible for filling the rest of the data from a commit
buffer.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 33 ++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 80e377b90f..3e33d061fe 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -278,17 +278,44 @@ static int fill_commit_in_graph(struct commit *item,
struct commit_graph *g, uin
struct commit_list **pptr;
const unsigned char *commit_data = g->chunk_commit_data + (g->hash_len
+ 16) * pos;
- item->object.parsed = 1;
+ item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
item->graph_pos = pos;
+ /*
+* If we have any edges marked as GRAPH_PARENT_MISSING, we must not
parse any
+* more of this object and leave it to the commit buffer to parse.
+*/
+ edge_value = get_be32(commit_data + g->hash_len);
+ if (edge_value == GRAPH_PARENT_MISSING)
+ return 0;
+ if (edge_value == GRAPH_PARENT_NONE)
+ goto continue_parsing;
+
+ edge_value = get_be32(commit_data + g->hash_len + 4);
+ if (edge_value == GRAPH_PARENT_MISSING)
+ return 0;
+ if (edge_value == GRAPH_PARENT_NONE)
+ goto continue_parsing;
+ if (!(edge_value & GRAPH_OCTOPUS_EDGES_NEEDED))
+ goto continue_parsing;
+
+ parent_data_ptr = (uint32_t*)(g->chunk_large_edges +
+ 4 * (uint64_t)(edge_value & GRAPH_EDGE_LAST_MASK));
+ do {
+ edge_value = get_be32(parent_data_ptr);
+ if (edge_value == GRAPH_PARENT_MISSING)
+ return 0;
+ parent_data_ptr++;
+ } while (!(edge_value & GRAPH_LAST_EDGE));
+
+continue_parsing:
+ item->object.parsed = 1;
item->maybe_tree = NULL;
date_high = get_be32(commit_data + g->hash_len + 8) & 0x3;
date_low = get_be32(commit_data + g->hash_len + 12);
item->date = (timestamp_t)((date_high << 32) | date_low);
- item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
-
pptr = >parents;
edge_value = get_be32(commit_data + g->hash_len);
--
2.16.2.338.gcfe06ae955
[RFC PATCH 3/6] commit-graph: enable replace-object and grafts
Create destroy_commit_graph() method to delete the commit-graph file
when history is altered by a replace-object call. If the commit-graph
is rebuilt after that, we will load the correct object while reading
the commit-graph.
When parsing a commit, first check if the commit was grafted. If so,
then ignore the commit-graph for that commit and insted use the parents
loaded by parsing the commit buffer and comparing against the graft
file.
Signed-off-by: Derrick Stolee
---
builtin/replace.c | 3 +++
commit-graph.c| 20 +++-
commit-graph.h| 9 +
commit.c | 5 +
4 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index 9f01f3fc7f..d553aadcdc 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -15,6 +15,7 @@
#include "parse-options.h"
#include "run-command.h"
#include "tag.h"
+#include "commit-graph.h"
static const char * const git_replace_usage[] = {
N_("git replace [-f] "),
@@ -468,6 +469,8 @@ int cmd_replace(int argc, const char **argv, const char
*prefix)
usage_msg_opt("--raw only makes sense with --edit",
git_replace_usage, options);
+ destroy_commit_graph(get_object_directory());
+
switch (cmdmode) {
case MODE_DELETE:
if (argc < 1)
diff --git a/commit-graph.c b/commit-graph.c
index e9195dfb17..95af4ed519 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -6,6 +6,7 @@
#include "pack.h"
#include "packfile.h"
#include "commit.h"
+#include "dir.h"
#include "object.h"
#include "refs.h"
#include "revision.h"
@@ -240,15 +241,22 @@ static struct commit_list **insert_parent_or_die(struct
commit_graph *g,
{
struct commit *c;
struct object_id oid;
+ const unsigned char *real;
if (pos >= g->num_commits)
die("invalid parent position %"PRIu64, pos);
hashcpy(oid.hash, g->chunk_oid_lookup + g->hash_len * pos);
+
+ real = lookup_replace_object(oid.hash);
+
c = lookup_commit();
if (!c)
die("could not find commit %s", oid_to_hex());
- c->graph_pos = pos;
+
+ if (!hashcmp(real, oid.hash))
+ c->graph_pos = pos;
+
return _list_insert(c, pptr)->next;
}
@@ -1019,3 +1027,13 @@ int verify_commit_graph(struct commit_graph *g)
return verify_commit_graph_error;
}
+
+void destroy_commit_graph(const char *obj_dir)
+{
+ char *graph_name;
+ close_commit_graph();
+
+ graph_name = get_commit_graph_filename(obj_dir);
+ remove_path(graph_name);
+ FREE_AND_NULL(graph_name);
+}
diff --git a/commit-graph.h b/commit-graph.h
index 9a06a5f188..1d17da1582 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -56,4 +56,13 @@ void write_commit_graph(const char *obj_dir,
int verify_commit_graph(struct commit_graph *g);
+/*
+ * Delete the commit-graph file in the given object directory.
+ *
+ * WARNING: this deletes data, so should only be used when
+ * performing history-altering actions like replace-object
+ * or grafts.
+ */
+void destroy_commit_graph(const char *obj_dir);
+
#endif
diff --git a/commit.c b/commit.c
index 6eaed0174c..2fe31cde77 100644
--- a/commit.c
+++ b/commit.c
@@ -403,6 +403,11 @@ int parse_commit_internal(struct commit *item, int
quiet_on_missing, int use_com
return -1;
if (item->object.parsed)
return 0;
+
+ prepare_commit_graft();
+ if (commit_graft_pos(item->object.oid.hash) >= 0)
+ use_commit_graph = 0;
+
if (use_commit_graph && parse_commit_in_graph(item))
return 0;
buffer = read_sha1_file(item->object.oid.hash, , );
--
2.16.2.338.gcfe06ae955
[RFC PATCH 5/6] fetch: destroy commit graph on shallow parameters
The commit-graph feature is not compatible with history-rewriting
features such as shallow clones. When running a 'git fetch' with
any of the shallow/unshallow options, destroy the commit-graph
file and override core.commitGraph to be false.
Signed-off-by: Derrick Stolee
---
builtin/fetch.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index af896e4b74..2001dca881 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1452,6 +1452,12 @@ int cmd_fetch(int argc, const char **argv, const char
*prefix)
}
}
+ if (update_shallow || depth || deepen_since || deepen_not.nr ||
+ deepen_relative || unshallow || update_shallow ||
is_repository_shallow()) {
+ destroy_commit_graph(get_object_directory());
+ core_commit_graph = 0;
+ }
+
if (remote) {
if (filter_options.choice || repository_format_partial_clone)
fetch_one_setup_partial(remote);
--
2.16.2.338.gcfe06ae955
[RFC PATCH 2/6] DO NOT MERGE: write commit-graph on every fetch
THIS IS ONLY FOR TESTING TO INCREASE TEST COVERAGE
Signed-off-by: Derrick Stolee
---
builtin/fetch.c | 4
1 file changed, 4 insertions(+)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8ee998ea2e..af896e4b74 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -20,6 +20,7 @@
#include "utf8.h"
#include "packfile.h"
#include "list-objects-filter-options.h"
+#include "commit-graph.h"
static const char * const builtin_fetch_usage[] = {
N_("git fetch [] [ [...]]"),
@@ -1480,6 +1481,9 @@ int cmd_fetch(int argc, const char **argv, const char
*prefix)
close_all_packs();
+ if (core_commit_graph)
+ write_commit_graph_reachable(get_object_directory(), 1);
+
argv_array_pushl(_gc_auto, "gc", "--auto", NULL);
if (verbosity < 0)
argv_array_push(_gc_auto, "--quiet");
--
2.16.2.338.gcfe06ae955
[RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo
The commit-graph file stores a condensed version of the commit history. This helps speed up several operations involving commit walks. This feature does not work well if those commits "change" using features like commit grafts, replace objects, or shallow clones. Since the commit-graph feature is optional, hidden behind the 'core.commitGraph' config option, and requires manual maintenance (until ds/commit-graph-fsck' is merged), these issues only arise for expert users who decided to opt-in. This RFC is a first attempt at rectify the issues that come up when these features interact. I have not succeeded entirely, as I will discuss below. The first two "DO NOT MERGE" commits are not intended to be part of the main product, but are ways to help the full test suite run while computing and consuming commit-graph files. This is acheived by calling write_commit_graph_reachable() from `git fetch` and `git commit`. I believe this is the only dependence on ds/commit-graph-fsck. The other commits should only depend on ds/commit-graph-lockfile-fix. Running the full test suite after these DO NO MERGE commits results in the following test failures, which I categorize by type of failure. The following tests are red herrings. Most work by deleting a commit from the object database and trying to demonstrate a failure. Others rely on how certain objects are loaded. These are not bugs, but will add noise if you run the tests suite with this patch. t0410-partial-clone.sh Failed tests: 5, 8 t5307-pack-missing-commit.shFailed tests: 3-4 t6011-rev-list-with-bad-commit.sh Failed test: 4 t6024-recursive-merge.shFailed test: 4 The following tests are fixed in "commit-graph: enable replace-object and grafts". t6001-rev-list-graft.sh Failed tests: 3, 5, 7, 9, 11, 13 t6050-replace.shFailed tests: 11-15, 23-24, 29 The following tests involve shallow clones. t5500-fetch-pack.sh Failed tests: 30-31, 34, 348-349 t5537-fetch-shallow.sh Failed tests: 4-7, 9 t5802-connect-helper.sh Failed test: 3 These tests are mostly fixed by three commits: * commit-graph: avoid writing when repo is shallow * fetch: destroy commit graph on shallow parameters * commit-graph: revert to odb on missing parents Each of these cases cover a different interaction that occurs with shallow clones. Some are due to a commit becoming shallow, while others are due to a commit becoming unshallow (and hence invalidating its generation number). After these changes, there is one test case that still fails, and I cannot understand why: t5500-fetch-pack.sh Failed test: 348 This test fails when performing a `git fetch --shallow-exclude`. When I halt the test using `t5500-fetch-pack.sh -d -i` and navigate to the directory to replay the fetch it performs as expected. After struggling with it for a while, I figured I should just put this series up for discussion. Maybe someone with more experience in shallow clones can point out the obvious issues I'm having. Thanks, -Stolee Derrick Stolee (6): DO NOT MERGE: compute commit-graph on every commit DO NOT MERGE: write commit-graph on every fetch commit-graph: enable replace-object and grafts commit-graph: avoid writing when repo is shallow fetch: destroy commit graph on shallow parameters commit-graph: revert to odb on missing parents builtin/commit.c | 5 + builtin/fetch.c | 10 + builtin/gc.c | 2 +- builtin/replace.c | 3 +++ commit-graph.c| 65 +++ commit-graph.h| 9 commit.c | 5 + environment.c | 2 +- 8 files changed, 95 insertions(+), 6 deletions(-) -- 2.16.2.338.gcfe06ae955
Re: is there a reason pre-commit.sample uses "git diff-index"?
On Thu, 31 May 2018, Duy Nguyen wrote: > On Thu, May 31, 2018 at 6:38 PM, Robert P. J. Day > wrote: > > > > was going over some hooks and writing some tutorials for some of > > the commit-related, client-side hooks, and was wondering (perhaps > > stupidly) why the pre-commit.sample hook uses, as its last line: > > > > exec git diff-index --check --cached $against -- > >^^ > > > > as in, could this not be done equivalently with just git diff, not > > git diff-index? i just did a quick test and it seems to do the > > same thing, but i've never taken a close look at git diff-index > > before so i may just be clueless about some important distinction. > > git diff-index is "plumbing", designed for writing scripts. "git > diff" on the other hand is for users and its behavior may change > even if it breaks backward compatibility. ah, this was a philosophical underpinning i was unaware of. i see occasional explanations of git porcelain versus plumbing, but i don't recall anyone simply stating that the plumbing is meant to have a long-term stability that is not guaranteed for the porcelain. in any event, this does mean that, stability issues aside, "git diff" would apparently have worked just fine for that hook. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: is there a reason pre-commit.sample uses "git diff-index"?
On Thu, May 31, 2018 at 6:38 PM, Robert P. J. Day wrote: > > was going over some hooks and writing some tutorials for some of the > commit-related, client-side hooks, and was wondering (perhaps > stupidly) why the pre-commit.sample hook uses, as its last line: > > exec git diff-index --check --cached $against -- >^^ > > as in, could this not be done equivalently with just git diff, not git > diff-index? i just did a quick test and it seems to do the same thing, > but i've never taken a close look at git diff-index before so i may > just be clueless about some important distinction. git diff-index is "plumbing", designed for writing scripts. "git diff" on the other hand is for users and its behavior may change even if it breaks backward compatibility. -- Duy
is there a reason pre-commit.sample uses "git diff-index"?
was going over some hooks and writing some tutorials for some of the commit-related, client-side hooks, and was wondering (perhaps stupidly) why the pre-commit.sample hook uses, as its last line: exec git diff-index --check --cached $against -- ^^ as in, could this not be done equivalently with just git diff, not git diff-index? i just did a quick test and it seems to do the same thing, but i've never taken a close look at git diff-index before so i may just be clueless about some important distinction. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: Git Vulnerability Announced?
On Thu, May 31, 2018 at 04:00:38PM +, Erika Voss wrote: > Yes here is what was sent to me - > > https://www.theregister.co.uk/2018/05/30/git_vulnerability_could_lead_to_an_attack_of_the_repo_clones/ > https://www.debian.org/security/2018/dsa-4212 Yeah, the release announcement from the project is at: https://public-inbox.org/git/xmqqy3g2flb6@gitster-ct.c.googlers.com/ > The one that I could find from online was: > https://git-scm.com/download/mac > > But, the latest version available on this site was 2.17.0, which does > not include the security patch. The binary installs for MacOS are done by a third party, and sometimes lag the source releases. You can build it from source yourself, either from a tarball: https://git.kernel.org/pub/software/scm/git/git-2.17.1.tar.gz or by cloning with git: https://kernel.org/pub/scm/git/git.git There are some instructions in the INSTALL file, which you can also read online: https://github.com/git/git/blob/master/INSTALL You can also use Homebrew to install, which usually updates to new versions pretty promptly: https://brew.sh/ -Peff
Bug: Install from .tar.xz fails without write permission on /usr/local/share/man/man3
Hi, I was trying to build git 2.9.5 as a normal user, as I have no root access on a cluster with outdated software. The build fails, unless I change the PREFIX=/usr/local line in per/perl.mak:80 to a folder where I have write permission. Apparently, perl.mak does not honour the --prefix= setting of ./configure. Is it possible to change perl.mak to honor the PREFIX? Best, Mo Steps to reproduce: wget https://mirrors.edge.kernel.org/pub/software/scm/git/git-2.9.5.tar.xz tar xf git-2.9.5.tar.xz cd git-2.9.5 ./configure --prefix=$HOME/.usr make make install # fails Output (last lines): install -d -m 755 '/qg-10/data/AGR-QG/lell/.usr/share/locale' (cd po/build/locale && gtar cf - .) | \ (cd '/qg-10/data/AGR-QG/lell/.usr/share/locale' && umask 022 && gtar xof -) make -C perl prefix='/qg-10/data/AGR-QG/lell/.usr' DESTDIR='' install make[1]: Entering directory `/qg-10/data/AGR-QG/lell/.usr/git-2.9.5/perl' make[2]: Entering directory `/qg-10/data/AGR-QG/lell/.usr/git-2.9.5/perl' ERROR: Can't create '/usr/local/share/man/man3' Do not have write permissions on '/usr/local/share/man/man3' at -e line 1. make[2]: *** [pure_site_install] Error 13 make[2]: Leaving directory `/qg-10/data/AGR-QG/lell/.usr/git-2.9.5/perl' make[1]: *** [install] Error 2 make[1]: Leaving directory `/qg-10/data/AGR-QG/lell/.usr/git-2.9.5/perl' make: *** [install] Error 2
Re: Git Vulnerability Announced?
Hi Randall, Yes here is what was sent to me - https://www.theregister.co.uk/2018/05/30/git_vulnerability_could_lead_to_an_attack_of_the_repo_clones/ https://www.debian.org/security/2018/dsa-4212 The one that I could find from online was: https://git-scm.com/download/mac But, the latest version available on this site was 2.17.0, which does not include the security patch. Thank you, Erika On 5/31/18, 8:59 AM, "Randall S. Becker" wrote: On May 31, 2018 11:57 AM, Erika Voss wrote: > There was an article I came across yesterday identifying a vulnerability to > patch our Git environments. I don’t see one that is available for our Mac > Clients - is there a more recent one that I can download that is available to > patch the 2.17.0 version? Do you have a reference, CVE number, or other information about this vulnerability? Cheers, Randall -- Brief whoami: NonStop developer since approximately 2112884442 UNIX developer since approximately 421664400 -- In my real life, I talk too much.
RE: Git Vulnerability Announced?
On May 31, 2018 11:57 AM, Erika Voss wrote: > There was an article I came across yesterday identifying a vulnerability to > patch our Git environments. I don’t see one that is available for our Mac > Clients - is there a more recent one that I can download that is available to > patch the 2.17.0 version? Do you have a reference, CVE number, or other information about this vulnerability? Cheers, Randall -- Brief whoami: NonStop developer since approximately 2112884442 UNIX developer since approximately 421664400 -- In my real life, I talk too much.
Git Vulnerability Announced?
Good morning, There was an article I came across yesterday identifying a vulnerability to patch our Git environments. I don’t see one that is available for our Mac Clients - is there a more recent one that I can download that is available to patch the 2.17.0 version? Thank you, Erika
Re: [PATCH] fetch: do not pass ref-prefixes for fetch by exact SHA1
Thanks for finding this, I don't know how I missed moving that bit over when factoring it out. Well I guess I sort of rewrote it and combined two pieces of logic so that's how. Anyway, this looks right and thanks for adding the test. On Thu, May 31, 2018 at 12:23 AM, Jonathan Nieder wrote: > When v2.18.0-rc0~10^2~1 (refspec: consolidate ref-prefix generation > logic, 2018-05-16) factored out the ref-prefix generation code for > reuse, it left out the 'if (!item->exact_sha1)' test in the original > ref-prefix generation code. As a result, fetches by SHA-1 generate > ref-prefixes as though the SHA-1 being fetched were an abbreviated ref > name: > > $ GIT_TRACE_PACKET=1 bin-wrappers/git -c protocol.version=2 \ > fetch origin 12039e008f9a4e3394f3f94f8ea897785cb09448 > [...] > packet:fetch> ref-prefix 12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/tags/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/heads/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448/HEAD > packet:fetch> > > If there is another ref name on the command line or the object being > fetched is already available locally, then that's mostly harmless. > But otherwise, we error out with > > fatal: no matching remote head > > since the server did not send any refs we are interested in. Filter > out the exact_sha1 refspecs to avoid this. > > This patch adds a test to check this behavior that notices another > behavior difference between protocol v0 and v2 in the process. Add a > NEEDSWORK comment to clear it up. > > Signed-off-by: Jonathan Nieder > --- > Here's the change described in > https://public-inbox.org/git/20180531010739.gb36...@aiede.svl.corp.google.com/ > as a proper patch. > > Thoughts of all kinds welcome, as always. > > refspec.c | 2 ++ > refspec.h | 4 > t/t5516-fetch-push.sh | 19 +++ > 3 files changed, 25 insertions(+) > > diff --git a/refspec.c b/refspec.c > index c59a4ccf1e..ada7854f7a 100644 > --- a/refspec.c > +++ b/refspec.c > @@ -202,6 +202,8 @@ void refspec_ref_prefixes(const struct refspec *rs, > const struct refspec_item *item = >items[i]; > const char *prefix = NULL; > > + if (item->exact_sha1) > + continue; > if (rs->fetch == REFSPEC_FETCH) > prefix = item->src; > else if (item->dst) > diff --git a/refspec.h b/refspec.h > index 01b700e094..3a9363887c 100644 > --- a/refspec.h > +++ b/refspec.h > @@ -42,6 +42,10 @@ void refspec_clear(struct refspec *rs); > int valid_fetch_refspec(const char *refspec); > > struct argv_array; > +/* > + * Determine what values to pass to the peer in ref-prefix lines > + * (see Documentation/technical/protocol-v2.txt). > + */ > void refspec_ref_prefixes(const struct refspec *rs, > struct argv_array *ref_prefixes); > > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index f4d28288f0..a5077d8b7c 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1121,6 +1121,25 @@ test_expect_success 'fetch exact SHA1' ' > ) > ' > > +test_expect_success 'fetch exact SHA1 in protocol v2' ' > + mk_test testrepo heads/master hidden/one && > + git push testrepo master:refs/hidden/one && > + git -C testrepo config transfer.hiderefs refs/hidden && > + check_push_result testrepo $the_commit hidden/one && > + > + mk_child testrepo child && > + git -C child config protocol.version 2 && > + > + # make sure $the_commit does not exist here > + git -C child repack -a -d && > + git -C child prune && > + test_must_fail git -C child cat-file -t $the_commit && > + > + # fetching the hidden object succeeds by default > + # NEEDSWORK: should this match the v0 behavior instead? > + git -C child fetch -v ../testrepo $the_commit:refs/heads/copy > +' > + > for configallowtipsha1inwant in true false > do > test_expect_success "shallow fetch reachable SHA1 (but not a ref), > allowtipsha1inwant=$configallowtipsha1inwant" ' > -- > 2.17.1.1185.g55be947832 >
Re: Weird revision walk behaviour
On 31/05/2018 08:43, Jeff King wrote: If there are zero parents (neither relevant nor irrelevant), is it still TREESAME? I would say in theory yes. Not sure - I think roots are such a special case that TREESAME effectively doesn't matter. We always test for roots first. So what I was proposing would be to rewrite the parents to the empty set. That feels a bit radical - I believe we need to retain (some) parent information for modes that show it (eg the dangling unfilled circles in gitk). And making it a root I think could cause other problems with making it look like we have a disjoint history. I believe the next simplification step may be trying to follow down to the common root. What next here? It looks like we have a proposed solution. Do you want to try to work up a set of tests based on what you wrote earlier? I was hoping Gábor would carry on, as he's made a start... I was just planning to back-seat drive. I'd also love to hear from Junio as the expert in this area, but I think he's been a bit busy with maintainer stuff recently. So maybe I should just be patient. :) Likewise - I have been quite deep into this, but it was a quite short window of investigation a long time ago, and I've not looked at it since. Would like input from someone with more active knowledge. Kevin
Re: [PATCH v3 11/20] commit-graph: verify root tree OIDs
On 5/30/2018 6:24 PM, Jakub Narebski wrote:
Derrick Stolee writes:
The 'verify' subcommand must compare the commit content parsed from the
commit-graph and compare it against the content in the object database.
You have "compare" twice in the above sentence.
Use lookup_commit() and parse_commit_in_graph_one() to parse the commits
from the graph and compare against a commit that is loaded separately
and parsed directly from the object database.
All right, that looks like a nice extension of what was done in previous
patch. We want to check that cache (serialized commit graph) matches
reality (object database).
Add checks for the root tree OID.
All right; isn't it that now we check almost all information from
commit-graph that hs match in object database (with exception of commit
parents, I think).
Signed-off-by: Derrick Stolee
---
commit-graph.c | 17 -
t/t5318-commit-graph.sh | 7 +++
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/commit-graph.c b/commit-graph.c
index 0420ebcd87..19ea369fc6 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -880,6 +880,8 @@ int verify_commit_graph(struct commit_graph *g)
return verify_commit_graph_error;
NOTE: we will be checking Commit Data chunk; I think it would be good
idea to verify that size of Commit Data chunk matches (N * (H + 16) bytes)
that format gives us, so that we don't accidentally red outside of
memory if something got screwed up (like number of commits being wrong,
or file truncated).
This is actually how we calculate 'num_commits' during
load_commit_graph_one():
if (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)
{
graph->num_commits = (chunk_offset - last_chunk_offset)
/ graph->hash_len;
}
So, if the chunk doesn't match N*(H+16), we detect this because
FANOUT[255] != N.
(There is one caveat here: (chunk_offset - last_chunk_offset) may not be
a multiple of hash_len, and "accidentally" truncate to N in the
division. I'll add more careful checks for this.)
We also check out-of-bounds offsets in that method.
for (i = 0; i < g->num_commits; i++) {
+ struct commit *graph_commit;
+
hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
if (i && oidcmp(_oid, _oid) >= 0)
@@ -897,6 +899,11 @@ int verify_commit_graph(struct commit_graph *g)
cur_fanout_pos++;
}
+
+ graph_commit = lookup_commit(_oid);
So now I see why we add all commits to memory (to hash structure).
+ if (!parse_commit_in_graph_one(g, graph_commit))
+ graph_report("failed to parse %s from commit-graph",
+oid_to_hex(_oid));
All right, this verifies that commit in OID Lookup chunk has parse-able
data in Commit Data chunk, isn't it?
}
while (cur_fanout_pos < 256) {
@@ -913,16 +920,24 @@ int verify_commit_graph(struct commit_graph *g)
return verify_commit_graph_error;
for (i = 0; i < g->num_commits; i++) {
- struct commit *odb_commit;
+ struct commit *graph_commit, *odb_commit;
hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
+ graph_commit = lookup_commit(_oid);
odb_commit = (struct commit *)create_object(cur_oid.hash,
alloc_commit_node());
All right, so we have commit data from graph, and commit data from the
object database.
if (parse_commit_internal(odb_commit, 0, 0)) {
graph_report("failed to parse %s from object database",
oid_to_hex(_oid));
continue;
}
+
+ if (oidcmp(_commit_tree_in_graph_one(g,
graph_commit)->object.oid,
+ get_commit_tree_oid(odb_commit)))
+ graph_report("root tree OID for commit %s in commit-graph is
%s != %s",
+oid_to_hex(_oid),
+
oid_to_hex(get_commit_tree_oid(graph_commit)),
+
oid_to_hex(get_commit_tree_oid(odb_commit)));
Maybe explicitly say that it doesn't match the value from the object
database; OTOH this may be too verbose.
}
return verify_commit_graph_error;
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 996a016239..21cc8e82f3 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -267,6 +267,8 @@ GRAPH_BYTE_FANOUT2=`expr $GRAPH_FANOUT_OFFSET + 4 \* 255`
GRAPH_OID_LOOKUP_OFFSET=`expr $GRAPH_FANOUT_OFFSET + 4 \* 256`
GRAPH_BYTE_OID_LOOKUP_ORDER=`expr $GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 8`
GRAPH_BYTE_OID_LOOKUP_MISSING=`expr $GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 4
+ 10`
+GRAPH_COMMIT_DATA_OFFSET=`expr $GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \*
$NUM_COMMITS`
+GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET
Re: [PATCH v3 10/20] commit-graph: verify objects exist
On 5/30/2018 3:22 PM, Jakub Narebski wrote:
Derrick Stolee writes:
In the 'verify' subcommand, load commits directly from the object
database to ensure they exist. Parse by skipping the commit-graph.
All right, before we check that the commit data matches, we need to
check that all the commits in cache (in the serialized commit graph) are
present in real data (in the object database of the repository).
What's nice of this series is that the operation that actually removes
unreachable commits from the object database, that is `git gc`, would
also update commit-gaph file.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 20
t/t5318-commit-graph.sh | 7 +++
2 files changed, 27 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index cbd1aae514..0420ebcd87 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -238,6 +238,10 @@ static struct commit_list **insert_parent_or_die(struct
commit_graph *g,
{
struct commit *c;
struct object_id oid;
+
+ if (pos >= g->num_commits)
+ die("invalid parent position %"PRIu64, pos);
+
This change is not described in the commit message.
This change should go in "commit-graph: verify parent list" which adds a
test that fails without it. Thanks.
hashcpy(oid.hash, g->chunk_oid_lookup + g->hash_len * pos);
c = lookup_commit();
if (!c)
@@ -905,5 +909,21 @@ int verify_commit_graph(struct commit_graph *g)
cur_fanout_pos++;
}
+ if (verify_commit_graph_error)
+ return verify_commit_graph_error;
All right, so we by default short-circuit so that errors found earlier
wouldn't cause crash when checking other things.
Is it needed, though, in this case? Though I guess it is better to be
conservative; lso by terminating after a batch of one type of errors we
don't get many different error messages from the same error (i.e. error
propagation).
+
+ for (i = 0; i < g->num_commits; i++) {
+ struct commit *odb_commit;
+
+ hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
+
+ odb_commit = (struct commit *)create_object(cur_oid.hash,
alloc_commit_node());
Do we really need to keep all those commits from the object database in
memory (in the object::obj_hash hash table)? Perhaps using something
like Flywheel / Recycler pattern would be a better solution (if
possible)?
Though perhaps this doesn't matter much with respect to memory use.
+ if (parse_commit_internal(odb_commit, 0, 0)) {
Just a reminder to myself: the signature is
int parse_commit_internal(struct commit *item, int quiet_on_missing, int
use_commit_graph)
Hmmm... I wonder if with two boolean paramaters wouldn't it be better to
use flags parameter, i.e.
int parse_commit_internal(struct commit *item, int flags)
...
parse_commit_internal(commit, QUIET_ON_MISSING | USE_COMMIT_GRAPH)
But I guess that it is not worth it (especially for internal-ish
function).
+ graph_report("failed to parse %s from object database",
+oid_to_hex(_oid));
Wouldn't parse_commit_internal() show it's own error message, in
addition to the one above?
+ continue;
+ }
+ }
+
return verify_commit_graph_error;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index c050ef980b..996a016239 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -247,6 +247,7 @@ test_expect_success 'git commit-graph verify' '
git commit-graph verify >output
'
+NUM_COMMITS=9
HASH_LEN=20
GRAPH_BYTE_VERSION=4
GRAPH_BYTE_HASH=5
@@ -265,6 +266,7 @@ GRAPH_BYTE_FANOUT1=`expr $GRAPH_FANOUT_OFFSET + 4 \* 4`
GRAPH_BYTE_FANOUT2=`expr $GRAPH_FANOUT_OFFSET + 4 \* 255`
GRAPH_OID_LOOKUP_OFFSET=`expr $GRAPH_FANOUT_OFFSET + 4 \* 256`
GRAPH_BYTE_OID_LOOKUP_ORDER=`expr $GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 8`
+GRAPH_BYTE_OID_LOOKUP_MISSING=`expr $GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 4
+ 10`
All right, so we modify 10-the byte of OID of 5-th commit out of 9,
am I correct here?
# usage: corrupt_graph_and_verify
# Manipulates the commit-graph file at the position
@@ -334,4 +336,9 @@ test_expect_success 'detect incorrect OID order' '
"incorrect OID order"
'
+test_expect_success 'detect OID not in object database' '
+ corrupt_graph_and_verify $GRAPH_BYTE_OID_LOOKUP_MISSING "\01" \
+ "from object database"
+'
I guess that if we ensure that OIDs are constant, you have chosen the
change to actually corrupt the OID in OID Lookup chunk to point to OID
that is not in the object database (while still not violating the
constraint that OID in OID Lookup chunk needs to be sorted).
+
test_done
All right (well, except for `expr ... ` --> $(( ... )) change).
Gratulujeme k získaniu 650 000,00 €
Gratulujeme, že ste získali € 650,000.00 v miliónoch Euro / Google Propagacné remízy sa konali 1. mája 2018. Obrátte sa na nášho reklamného zástupcu s nasledujúcimi informáciami o nárokoch na tento e-mail. janosiklubos...@gmail.com 1. Celé meno: 2. Adresa: 3. Pohlavie: 4. Vek: 5. Zamestnanie 6. Telefón: Robert Avtandiltayn Online koordinátor
Re: [PATCH v2 01/18] Add a function to solve least-cost assignment problems
Hi Brian, On Wed, 30 May 2018, brian m. carlson wrote: > On Wed, May 30, 2018 at 09:14:06AM -0700, Stefan Beller wrote: > > Good point. I remember my initial reaction to the file names was > > expecting some hungarian notation, which totally didn't make sense, so > > I refrained from commenting. Searching the web for the algorithm, > > maybe 'lapjv.c' is adequate? (short for "Linear Assignment Problem > > Jonker Volgenant") Matlab has a function named lapjv solving the same > > problem, so it would fall in line with the outside world. > > > > Out of interest, why is it called hungarian in the first place? (I > > presume that comes from some background of DScho in image processing > > or such, so the the answer will be interesting for sure:) > > I think it's because tbdiff uses the hungarian Python module, which > implements the Hungarian method, also known as the Kuhn-Munkres > algorithm, for solving the linear assignment problem. This is the > Jonker-Volgenant algorithm, which solves the same problem. It's faster, > but less tolerant. > > At least this is what I just learned after about ten minutes of > searching. You learned well. The Assignment Problem (or "Linear Assignment Problem") is generally solved by the Hungarian algorithm. I forgot why it is called that way. Kuhn-Munkres came up with a simplification of the algorithm IIRC but it still is O(N^4). Then Jonker-Volgenant took a very different approach that somehow results in O(N^3). It's been *years* since I studied both implementations, so I cannot really explain what they do, and how the latter achieves its order-of-magnitude better performance. And after reading these mails, I agree that the name "hungarian" might be confusing. I also think that "lapjv" is confusing. In general, I try to let Matlab conventions inform on my naming as little as possible, and I find my naming fu a lot better for it. So in this case, how about `linear-assignment.c`? Ciao, Dscho
Re: Git installer bugs
Hi Stefan, just to close the loop: On Wed, 30 May 2018, Stefan Beller wrote: > On Wed, May 30, 2018 at 12:48 PM, John Meyer wrote: > > Ran the installer, selected the option to not modify the path & the path > > was modified anyway... it removed git from the path (it was there from a > > prior install). I should NOT have to manually fix my path after an update, > > even the option to add git to the path should be smart enough to recognize > > it's there already & leave the path unmodified (sorry, I know that's 2 > > different bugs in 1 email, but they are related). > > Are you talking about Git for Windows? > Please file a bug at https://github.com/git-for-windows/git/issues/new I filed it: https://github.com/git-for-windows/git/issues/1696 Ciao, Johannes
Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote
Hi Stefan,
On Wed, 30 May 2018, Stefan Beller wrote:
> Signed-off-by: Stefan Beller
> ---
> builtin/submodule--helper.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 7c3cd9dbeba..96024fee1b1 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char
> **argv, const char *prefix)
> if (remote)
> printf("%s\n", remote);
>
> + free(remote);
Makes sense.
Out of curiosity (and because a cover letter is missing): how did you
stumble over these? Coverity?
Ciao,
Dscho
> return 0;
> }
>
> --
> 2.17.1.1185.g55be947832-goog
>
>
Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file
Hi Stefan,
I am Cc:ing Michael, the original author of the fixed commit.
On Wed, 30 May 2018, Stefan Beller wrote:
> Signed-off-by: Stefan Beller
> ---
>
> This was an oversight in 01caf20d57a (load_contents(): don't try to mmap an
> empty file, 2018-01-24).
>
> This and the following 2 patches apply on master.
>
> refs/packed-backend.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index cec3fb9e00f..d447a731da0 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -499,6 +499,7 @@ static int load_contents(struct snapshot *snapshot)
> size = xsize_t(st.st_size);
>
> if (!size) {
> + close(fd);
Good catch,
Dscho
> return 0;
> } else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
> snapshot->buf = xmalloc(size);
> --
> 2.17.1.1185.g55be947832-goog
>
>
Re: [PATCH 2/3] sequencer.c: free author variable when merging fails
Hi Stefan,
On Wed, 30 May 2018, Stefan Beller wrote:
> Signed-off-by: Stefan Beller
> ---
>
> This was a deliberate oversight in f241ff0d0a9 (prepare the builtins for a
> libified merge_recursive(), 2016-07-26)
No, it was not deliberate. It was inadvertent, most likely ;-)
> sequencer.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 72b4d8ecae3..5c93586cc1c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1771,8 +1771,10 @@ static int do_pick_commit(enum todo_command command,
> struct commit *commit,
> else if (!opts->strategy || !strcmp(opts->strategy, "recursive") ||
> command == TODO_REVERT) {
> res = do_recursive_merge(base, next, base_label, next_label,
>, , opts);
> - if (res < 0)
> + if (res < 0) {
> + free(author);
> return res;
Why not `goto leave;` instead? I wonder what is happening to the commit
message: can we be certain at this point that it was not set yet? And
also: should we call `update_abort_safety_file()`?
Ciao,
Dscho
> + }
> res |= write_message(msgbuf.buf, msgbuf.len,
>git_path_merge_msg(), 0);
> } else {
> --
> 2.17.1.1185.g55be947832-goog
>
>
Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file
On Wed, May 30, 2018 at 7:03 PM, Stefan Beller wrote:
> Signed-off-by: Stefan Beller
> ---
>
> This was an oversight in 01caf20d57a (load_contents(): don't try to mmap an
> empty file, 2018-01-24).
>
> This and the following 2 patches apply on master.
>
> refs/packed-backend.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index cec3fb9e00f..d447a731da0 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -499,6 +499,7 @@ static int load_contents(struct snapshot *snapshot)
> size = xsize_t(st.st_size);
>
> if (!size) {
> + close(fd);
> return 0;
> } else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
> snapshot->buf = xmalloc(size);
> --
> 2.17.1.1185.g55be947832-goog
>
+1.
Thanks,
Michael
Re: [PATCH] sequencer: ensure labels that are object IDs are rewritten
Hi Brian, On Wed, 30 May 2018, brian m. carlson wrote: > On Wed, May 30, 2018 at 11:54:27AM +0200, Johannes Schindelin wrote: > > > > + third=$(git rev-parse HEAD) && > > > + git checkout -b labels master && > > > + git merge --no-commit third && > > > + test_tick && > > > + git commit -m "Merge commit '\''$third'\'' into labels" && > > > > Here, the test_tick is required because we commit via `git commit`. > > > > BTW another thing that I had been meaning to address but totally forgot is > > this '\'' ugliness. I had been meaning to define SQ="'" before all test > > cases and then use $SQ everywhere. Not your problem, though. > > > > > + cp script-from-scratch-orig script-from-scratch && > > > > There is nothing in that script that you need. Why not simply > > > > echo noop >script-from-scratch > > > > or if you care about the branch, > > > > echo reset $third >script-from-scratch > > That would be simpler. You read my mind: I needed some script to make > the sequence editor work, but anything would be fine. I would not go that far. Sometimes I wish I could read minds. More often, I am glad that I cannot. I simply guessed correctly in this case ;-) But yes, I think it would not only be simpler, but would also avoid the head-scratching why the earlier `cp script-from-scratch script-from-scratch-orig`, and it would also make it more robust to future changes (e.g. if somebody decides to move test cases around, or introduce prereqs that skip some). Ciao, Dscho
[GSoC][PATCH 0/2] rebase -i: rewrite append_todo_help() in C
This series rewrites append_todo_help() from shell to C. This is part of the effort to rewrite interactive rebase in C. The first commit rewrites append_todo_help() in C (the C version covers a bit more than the old shell version), adds some parameters to rebase--helper, etc. The second one strips newlines from append_todo_help() messages, which require to update the translations. This change was advised to me by Stefan Beller, but Johannes Schindelin voiced concerns. I don’t really have a strong opinion about it, so feel free to give yours. Alban Gruin (2): rebase--interactive: rewrite append_todo_help() in C sequencer: remove newlines from append_todo_help() messages builtin/rebase--helper.c | 10 ++-- git-rebase--interactive.sh | 52 ++--- sequencer.c| 64 ++ sequencer.h| 1 + 4 files changed, 75 insertions(+), 52 deletions(-) -- 2.16.4
[GSoC][PATCH 1/2] rebase--interactive: rewrite append_todo_help() in C
This rewrites append_todo_help() from shell to C. It also incorporates
some parts of initiate_action() and complete_action() that also write
help texts to the todo file.
Two flags are added to rebase--helper.c: one to call
append_todo_help() (`--append-todo-help`), and another one to tell
append_todo_help() to write the help text suited for the edit-todo
mode (`--edit-todo`).
Finally, append_todo_help() is removed from git-rebase--interactive.sh
to use `rebase--helper --append-todo-help` instead.
Signed-off-by: Alban Gruin
---
builtin/rebase--helper.c | 10 ++--
git-rebase--interactive.sh | 52 ++--
sequencer.c| 60 ++
sequencer.h| 1 +
4 files changed, 71 insertions(+), 52 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc..ad3a3a7eb 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0, edit_todo = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC
+ ADD_EXEC, APPEND_TODO_HELP
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -27,6 +27,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
N_("keep original branch points of cousins")),
+ OPT_BOOL(0, "edit-todo", _todo,
+N_("edit the todo list during an interactive rebase")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -45,6 +47,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
OPT_CMDMODE(0, "add-exec-commands", ,
N_("insert exec commands in todo list"), ADD_EXEC),
+ OPT_CMDMODE(0, "append-todo-help", ,
+ N_("insert the help in the todo list"),
APPEND_TODO_HELP),
OPT_END()
};
@@ -84,5 +88,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!rearrange_squash();
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
+ if (command == APPEND_TODO_HELP && argc == 1)
+ return !!append_todo_help(edit_todo, keep_empty);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9884ecd71..419c54068 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -39,38 +39,6 @@ comment_for_reflog () {
esac
}
-append_todo_help () {
- gettext "
-Commands:
-p, pick = use commit
-r, reword = use commit, but edit the commit message
-e, edit = use commit, but stop for amending
-s, squash = use commit, but meld into previous commit
-f, fixup = like \"squash\", but discard this commit's log message
-x, exec = run command (the rest of the line) using shell
-d, drop = remove commit
-l, label = label current HEAD with a name
-t, reset = reset HEAD to a label
-m, merge [-C | -c ] [# ]
-. create a merge commit using the original merge commit's
-. message (or the oneline, if no original merge commit was
-. specified). Use -c to reword the commit message.
-
-These lines can be re-ordered; they are executed from top to bottom.
-" | git stripspace --comment-lines >>"$todo"
-
- if test $(get_missing_commit_check_level) = error
- then
- gettext "
-Do not remove any line. Use 'drop' explicitly to remove a commit.
-" | git stripspace --comment-lines >>"$todo"
- else
- gettext "
-If you remove a line here THAT COMMIT WILL BE LOST.
-" | git stripspace --comment-lines >>"$todo"
- fi
-}
-
die_abort () {
apply_autostash
rm -rf "$state_dir"
@@ -143,13 +111,7 @@ initiate_action () {
git stripspace --strip-comments <"$todo" >"$todo".new
mv -f "$todo".new "$todo"
collapse_todo_ids
- append_todo_help
- gettext "
-You are editing the
[GSoC][PATCH 2/2] sequencer: remove newlines from append_todo_help() messages
This removes newlines before and after the messages in
append_todo_help(). This is done to avoid confusions from
translators.
These newlines are now inserted with
`strbuf_add_commented_lines()`. Messages were ended by two newlines
characters, but here we only insert one at a time. This is because
`strbuf_add_commented_lines()` automatically inserts a newline at the
end of the input, and ignore the last from the input.
Signed-off-by: Alban Gruin
---
sequencer.c | 24 ++--
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 9b291845e..9ab6c28d7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4228,7 +4228,7 @@ int append_todo_help(unsigned edit_todo, unsigned
keep_empty)
struct strbuf buf = STRBUF_INIT;
FILE *todo;
int ret;
- const char *msg = _("\nCommands:\n"
+ const char *msg = _("Commands:\n"
"p, pick = use commit\n"
"r, reword = use commit, but edit the commit message\n"
"e, edit = use commit, but stop for amending\n"
@@ -4243,33 +4243,37 @@ int append_todo_help(unsigned edit_todo, unsigned
keep_empty)
". message (or the oneline, if no original merge commit was\n"
". specified). Use -c to reword the commit message.\n"
"\n"
-"These lines can be re-ordered; they are executed from top to bottom.\n");
+"These lines can be re-ordered; they are executed from top to bottom.");
todo = fopen_or_warn(rebase_path_todo(), "a");
if (!todo)
return 1;
+ strbuf_add_commented_lines(, "\n", 1);
strbuf_add_commented_lines(, msg, strlen(msg));
if (get_missing_commit_check_level() == CHECK_ERROR)
- msg = _("\nDo not remove any line. Use 'drop' "
-"explicitly to remove a commit.\n");
+ msg = _("Do not remove any line. Use 'drop' "
+"explicitly to remove a commit.");
else
- msg = _("\nIf you remove a line here "
-"THAT COMMIT WILL BE LOST.\n");
+ msg = _("If you remove a line here "
+"THAT COMMIT WILL BE LOST.");
+ strbuf_add_commented_lines(, "\n", 1);
strbuf_add_commented_lines(, msg, strlen(msg));
if (edit_todo)
- msg = _("\nYou are editing the todo file "
+ msg = _("You are editing the todo file "
"of an ongoing interactive rebase.\n"
"To continue rebase after editing, run:\n"
- "git rebase --continue\n\n");
+ "git rebase --continue");
else
- msg = _("\nHowever, if you remove everything, "
- "the rebase will be aborted.\n\n");
+ msg = _("However, if you remove everything, "
+ "the rebase will be aborted.");
+ strbuf_add_commented_lines(, "\n", 1);
strbuf_add_commented_lines(, msg, strlen(msg));
+ strbuf_add_commented_lines(, "\n", 1);
if (!keep_empty) {
msg = _("Note that empty commits are commented out");
--
2.16.4
Re: [PATCH v3] checkout & worktree: introduce checkout.implicitRemote
On Thu, May 24 2018, Ævar Arnfjörð Bjarmason wrote:
> struct tracking_name_data {
> /* const */ char *src_ref;
> char *dst_ref;
> struct object_id *dst_oid;
> int unique;
> + const char *implicit_remote;
> + char *implicit_dst_ref;
> };
There's a bug here that I'll fix in a v3. We need to have a implicit_*
variant for dst_oid as well. Currently this will be buggy and check out
origin/, but then check the index out to the tree of whatever
the last / we iterated over was.
Easiy fix and I already have it locally, I just want to improve some of
the testing. I missed it because in my tests I'd just re-add the same
remote again, so the trees were the same.
> static int check_tracking_name(struct remote *remote, void *cb_data)
> @@ -20,6 +23,8 @@ static int check_tracking_name(struct remote *remote, void
> *cb_data)
> free(query.dst);
> return 0;
> }
> + if (cb->implicit_remote && !strcmp(remote->name, cb->implicit_remote))
> + cb->implicit_dst_ref = xstrdup(query.dst);
> if (cb->dst_ref) {
> free(query.dst);
> cb->unique = 0;
> @@ -31,13 +36,21 @@ static int check_tracking_name(struct remote *remote,
> void *cb_data)
>
> const char *unique_tracking_name(const char *name, struct object_id *oid)
> {
> - struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
> + const char *implicit_remote = NULL;
> + struct tracking_name_data cb_data = { NULL, NULL, NULL, 1, NULL, NULL };
> + if (!git_config_get_string_const("checkout.implicitremote",
> _remote))
> + cb_data.implicit_remote = implicit_remote;
> cb_data.src_ref = xstrfmt("refs/heads/%s", name);
> cb_data.dst_oid = oid;
> for_each_remote(check_tracking_name, _data);
> free(cb_data.src_ref);
> - if (cb_data.unique)
> + free((char *)implicit_remote);
> + if (cb_data.unique) {
> + free(cb_data.implicit_dst_ref);
> return cb_data.dst_ref;
> + }
> free(cb_data.dst_ref);
> + if (cb_data.implicit_dst_ref)
> + return cb_data.implicit_dst_ref;
> return NULL;
> }
> diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
> index 3e5ac81bd2..da6bd74bbc 100755
> --- a/t/t2024-checkout-dwim.sh
> +++ b/t/t2024-checkout-dwim.sh
> @@ -68,6 +68,16 @@ test_expect_success 'checkout of branch from multiple
> remotes fails #1' '
> test_branch master
> '
>
> +test_expect_success 'checkout of branch from multiple remotes succeeds with
> checkout.implicitRemote #1' '
> + git checkout -B master &&
> + test_might_fail git branch -D foo &&
> +
> + git -c checkout.implicitRemote=repo_a checkout foo &&
> + test_branch foo &&
> + test_cmp_rev remotes/repo_a/foo HEAD &&
> + test_branch_upstream foo repo_a foo
> +'
> +
> test_expect_success 'checkout of branch from a single remote succeeds #1' '
> git checkout -B master &&
> test_might_fail git branch -D bar &&
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 2240498924..271a6413f0 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -402,6 +402,24 @@ test_expect_success '"add" dwims' '
> )
> '
>
> +test_expect_success '"add" dwims with
> checkout.implicitRemote' '
> + test_when_finished rm -rf repo_upstream repo_dwim foo &&
> + setup_remote_repo repo_upstream repo_dwim &&
> + git init repo_dwim &&
> + (
> + cd repo_dwim &&
> + git remote add repo_upstream2 ../repo_upstream &&
> + git fetch repo_upstream2 &&
> + test_must_fail git worktree add ../foo foo &&
> + git -c checkout.implicitRemote=repo_upstream worktree add
> ../foo foo
> + ) &&
> + (
> + cd foo &&
> + test_branch_upstream foo repo_upstream foo &&
> + test_cmp_rev refs/remotes/repo_upstream/foo refs/heads/foo
> + )
> +'
> +
> test_expect_success 'git worktree add does not match remote' '
> test_when_finished rm -rf repo_a repo_b foo &&
> setup_remote_repo repo_a repo_b &&
Re: Is origin/HEAD only being created on clone a bug? #leftoverbits
On Wed, May 30 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > >> If you make an initial commit and push to a remote repo "origin", you >> don't get a remote origin/HEAD reference, and a "fetch" won't create it >> either. >> ... >> Some code spelunking reveals remote_head_points_at, guess_remote_head() >> etc. in builtin/clone.c. I.e. this is special-cased as part of the >> "clone". > > Correct. Originally, there was *no* way in the protocol to carry > the information, so the code always had to guess. The point of > setting origin/HEAD was mostly so that you can say "log origin.." > and rely on it getting dwimmed down to "refs/remotes/%s/HEAD..", > and it wasn't a common practice to interact with multiple remotes > with remote tracking branches (integrator interacting with dozens > of remotes, responding to pull requests using explicit URL but > without configured remotes was not uncommon), so it was sufficient > for "git clone" to create it, and "git remote add" did not exist > back then anyway. > > There are two aspects in my answer to your question. > > - If we create additional remote (that is, other than the one we >get when we create a repository via "clone", so if your "origin" >is from "git init there && cd there && git remote add origin", it >does count in this category), should we get a remote-tracking >symref $name/HEAD so that we can say "log $name.."? > >We absolutely should. We (eh, rather, those who added "remote >add"; this was not my itch and I am using "royal we" in this >sentence) just did not bother to and I think it is a bug that you >cannot say "log $name.." Of course, it is just a "git symbolic-ref" >away to make it possible locally, so it is understandable if >"remote add" did not bother to. > > - When we fetch from a remote that has refs/remotes/$name/HEAD, and >if the protocol notices that their HEAD today is pointing to a >branch different from what our side has, should we repoint ours >to match? > >I am leaning against doing this, but mostly out of superstition. >Namely, I feel uneasy about the fact that the meaning of "log >..origin" changes across a fetch in this sequence: > > log ..origin && fetch origin && log ..origin > >Without repointing origin/HEAD, two occurrences of "log ..origin" >both means "how much ahead the primary branch we have been >interested in from this remote is, relative to our effort?". >Even though we fully expect that two "log ..origin" would report >different results (after all, that is the whole point of doing >another one after "fetch" in such a sequence like this example), >our question is about the same "primary branch we have been >interested in". But once fetch starts messing with where >origin/HEAD points at, that would no longer be the case, which is >why I am against doing something magical like that. We already have to deal with this special case of origin/HEAD being re-pointed in a repository that we "clone", so we would just do whatever happens to a repository that's cloned. I.e. the "clone" sets the origin/HEAD up as a one-off, and then keeps updating it on the basis of updating existing refs. We'd similarly set it up as a one-off if we ever "fetch" and notice that the ref doesn't exist yet, and then we'd update it in the same way we update it now. So this seems like a non-issue to me as far as me coming up with some patch to one-off write the origin/HEAD on the first "fetch", or am I missing something?
Re: [PATCH 3/4] config doc: elaborate on what transfer.fsckObjects does
On Fri, May 25 2018, Junio C Hamano wrote: > Eric Sunshine writes: > >>> +will instead be left unreferenced in the repository. That's considered >>> +a bug, and hopefully future git release will implement a quarantine >>> +for the "fetch" side as well. >> >> If this was a "BUGS" section in a man-page, the above might be less >> scary. In this context, however, I wonder if it makes sense to tone it >> down a bit: >> >> On the fetch side, malformed objects will instead be left >> unreferenced in the repository. (However, in the future, such >> objects may be quarantined for "fetch", as well.) > > I had an impression that nobody else sayd it is considered as a > bug. Do we need to say it in this series? I'd rather not--with or > without such a future modification (or lack of plan thereof), > teaching the fetch side to pay attention to the various fsck tweaks > is an improvement. I changed this in v2 to tone it down, but given what Jeff's mentioned in https://public-inbox.org/git/20180531060259.ge17...@sigill.intra.peff.net/ I'm inclined to bring back some stronger wording for it. Something like: Due to the non-quarantine nature of the fetch.fsckObjects implementation it can not be relied upon like receive.fsckObjects can. As objects are unpacked they're written to the object store, so there can be cases where malicious objects get introduced even though the "fetch" fail, only to have a subsequent "fetch" succeed because only new incoming objects are checked, not those that have already been written to the store. This is considered a bug and will likely be fixed in future versions of git. For now the paranoid need to find some way to emulate the quarantine environment if they'd like the same protection as "push". E.g. in the case of an internal mirror do the mirroring in two steps, one to fetch the untrusted objects, and then do a second "push" (which will use the quarantine) to another internal repo, and have internal clients consume this pushed-to repository, or embargo internal fetches and only allow them once a full "fsck" has run (and no new fetches have happened in the meantime).
[PATCH] fetch: do not pass ref-prefixes for fetch by exact SHA1
When v2.18.0-rc0~10^2~1 (refspec: consolidate ref-prefix generation logic, 2018-05-16) factored out the ref-prefix generation code for reuse, it left out the 'if (!item->exact_sha1)' test in the original ref-prefix generation code. As a result, fetches by SHA-1 generate ref-prefixes as though the SHA-1 being fetched were an abbreviated ref name: $ GIT_TRACE_PACKET=1 bin-wrappers/git -c protocol.version=2 \ fetch origin 12039e008f9a4e3394f3f94f8ea897785cb09448 [...] packet:fetch> ref-prefix 12039e008f9a4e3394f3f94f8ea897785cb09448 packet:fetch> ref-prefix refs/12039e008f9a4e3394f3f94f8ea897785cb09448 packet:fetch> ref-prefix refs/tags/12039e008f9a4e3394f3f94f8ea897785cb09448 packet:fetch> ref-prefix refs/heads/12039e008f9a4e3394f3f94f8ea897785cb09448 packet:fetch> ref-prefix refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448 packet:fetch> ref-prefix refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448/HEAD packet:fetch> If there is another ref name on the command line or the object being fetched is already available locally, then that's mostly harmless. But otherwise, we error out with fatal: no matching remote head since the server did not send any refs we are interested in. Filter out the exact_sha1 refspecs to avoid this. This patch adds a test to check this behavior that notices another behavior difference between protocol v0 and v2 in the process. Add a NEEDSWORK comment to clear it up. Signed-off-by: Jonathan Nieder --- Here's the change described in https://public-inbox.org/git/20180531010739.gb36...@aiede.svl.corp.google.com/ as a proper patch. Thoughts of all kinds welcome, as always. refspec.c | 2 ++ refspec.h | 4 t/t5516-fetch-push.sh | 19 +++ 3 files changed, 25 insertions(+) diff --git a/refspec.c b/refspec.c index c59a4ccf1e..ada7854f7a 100644 --- a/refspec.c +++ b/refspec.c @@ -202,6 +202,8 @@ void refspec_ref_prefixes(const struct refspec *rs, const struct refspec_item *item = >items[i]; const char *prefix = NULL; + if (item->exact_sha1) + continue; if (rs->fetch == REFSPEC_FETCH) prefix = item->src; else if (item->dst) diff --git a/refspec.h b/refspec.h index 01b700e094..3a9363887c 100644 --- a/refspec.h +++ b/refspec.h @@ -42,6 +42,10 @@ void refspec_clear(struct refspec *rs); int valid_fetch_refspec(const char *refspec); struct argv_array; +/* + * Determine what values to pass to the peer in ref-prefix lines + * (see Documentation/technical/protocol-v2.txt). + */ void refspec_ref_prefixes(const struct refspec *rs, struct argv_array *ref_prefixes); diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index f4d28288f0..a5077d8b7c 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1121,6 +1121,25 @@ test_expect_success 'fetch exact SHA1' ' ) ' +test_expect_success 'fetch exact SHA1 in protocol v2' ' + mk_test testrepo heads/master hidden/one && + git push testrepo master:refs/hidden/one && + git -C testrepo config transfer.hiderefs refs/hidden && + check_push_result testrepo $the_commit hidden/one && + + mk_child testrepo child && + git -C child config protocol.version 2 && + + # make sure $the_commit does not exist here + git -C child repack -a -d && + git -C child prune && + test_must_fail git -C child cat-file -t $the_commit && + + # fetching the hidden object succeeds by default + # NEEDSWORK: should this match the v0 behavior instead? + git -C child fetch -v ../testrepo $the_commit:refs/heads/copy +' + for configallowtipsha1inwant in true false do test_expect_success "shallow fetch reachable SHA1 (but not a ref), allowtipsha1inwant=$configallowtipsha1inwant" ' -- 2.17.1.1185.g55be947832
Re: [PATCH v2 5/5] fetch: implement fetch.fsck.*
On Wed, May 30 2018, Junio C Hamano wrote:
> Earlier I mumbled "this 4-patch series generally looks good but I
> need to re-read the implementation step"; I meant this 5-patch
> series and here is my impression after re-reading the implementation
> step.
>
> Ævar Arnfjörð Bjarmason writes:
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index f97f21c022..f69cd31ad0 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1426,6 +1426,16 @@ fetch.fsckObjects::
>> ...
>
> Nicely written.
>
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index 490c38f833..9e4282788e 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -19,6 +19,7 @@
>> #include "sha1-array.h"
>> #include "oidset.h"
>> #include "packfile.h"
>> +#include "fsck.h"
>>
>> static int transfer_unpack_limit = -1;
>> static int fetch_unpack_limit = -1;
>> @@ -33,6 +34,7 @@ static int agent_supported;
>> static int server_supports_filtering;
>> static struct lock_file shallow_lock;
>> static const char *alternate_shallow_file;
>> +static struct strbuf fsck_msg_types = STRBUF_INIT;
>
> s/msg_types[]/exclude[]/ or something, as this is not just about "we
> tolerate known and future instances of 0-padded mode in trees", but
> also "we know this and that object is bad so do not complain" as well.
I copied the fsck_msg_types variable as-is from builtin/receive-pack.c
which Johannes added in 5d477a334a ("fsck (receive-pack): allow demoting
errors to warnings", 2015-06-22).
That's not a justification for doing the same here, but does your
critique also extend to that variable name, so I could fix it there
while I'm at it?
> Other than that, the implementation looks good.
>
>> diff --git a/t/t5504-fetch-receive-strict.sh
>> b/t/t5504-fetch-receive-strict.sh
>> index 49d3621a92..b7f5222c2b 100755
>> --- a/t/t5504-fetch-receive-strict.sh
>> +++ b/t/t5504-fetch-receive-strict.sh
>> @@ -135,6 +135,20 @@ test_expect_success 'push with receive.fsck.skipList' '
>> git push --porcelain dst bogus
>> '
>>
>> +test_expect_success 'fetch with fetch.fsck.skipList' '
>> +commit="$(git hash-object -t commit -w --stdin > +refspec=refs/heads/bogus:refs/heads/bogus &&
>> +git push . $commit:refs/heads/bogus &&
>> +rm -rf dst &&
>> +git init dst &&
>> +git --git-dir=dst/.git config fetch.fsckObjects true &&
>> +test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec &&
>> +git --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP &&
>> +echo $commit >dst/.git/SKIP &&
>> +git --git-dir=dst/.git fetch "file://$(pwd)" $refspec
>> +'
>
> If the test does not change meaning when file://$(pwd) is replaced
> with "." (or ".." or whatever if other tests below moves cwd
> around), I'd think it is preferrable. Seeing $(pwd) always makes me
> nervous about Windows.
Thanks. Will fix.
Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*
On Wed, May 30 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > >> On Mon, May 28, 2018 at 11:45 AM, Junio C Hamano wrote: >>> If the project has some tool constraints and have to accept new >>> "broken" objects on ongoing basis, then fsck. facility may >>> make sense, but that is probably a very narrow special use case. >> >> That makes sense. I'll reword this bit. >> ... >> I'll try to clarify this, but I think we really should have some bit >> there about historical tools. Realistically no new git tools produce >> these, so the user needs to be made aware of what the trade-off of >> turning these on is. >> >> The reality of that is that these objects are exceedingly rare, and >> mostly found in various old repositories. Something like that need to >> be mentioned so the user can weigh the trade-off of turning this on. > > Rare or not, once we say "avoid fsck. unless you have a good > reason not to", wouldn't that be sufficient? It's our documentation that should be clearly stating those reasons. If we're not saying anything about these being historical bugs, then e.g. I (not knowing the implementation) wouldn't have turned this on globally on my site knowing that because I have none of these now I'm *very* unlikely to have them in the future. That's different from something that just happens rarely, because a rare non-historical event can be expected to happen in the future. > Between "fsck. makes sense only when you use these rare and > you-probably-never-heard-of tools ongoing basis" and "when you > already have (slightly)broken objects, naming each of them in > skiplist, rather than covering the class, is better because you want > *new* instances of the same breakage", I'd imagine the latter would be > more helpful. > > In any case, let's see if there are more input to this topic and > then wrap it up in v3 ;-) > > Thanks.
Re: [RFC PATCH 3/4] color.ui config: don't die on unknown values
On Wed, May 30 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > >> Before this change git will die on any unknown color.ui values: >> >> $ git -c color.ui=doesnotexist show >> fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid >> unit > > I do not think "unit" is correct, so there may be some room for > improvement. For _this_ particular case, I agree that it is not the > end of the world if we did not color the output (because we do not > know what the 'doesnotyetexist' token from the future is trying to > tell us), but as a general principle, we should diagnose and die, if > a misconfiguration is easy to correct. Many users (including myself) use the same ~/.gitconfig on many different machines with different git versions. Maybe at some point I'm willing to set the new setting to a value I know is supported on most of them, but it sucks at that point if I logging into 1-3% of old machines ends up killing git on any invocation. > than blindly go ahead and do random things that the end-user did not > expect by giving something we do not (but they thought they do) > understand. I think this is highly dependent on what variables we give this treatment. There may be some where we genuinely have no idea what they mean, but in this case and for http.sslVersion (which warns, doesn't die on unknown values) it's reasonable to assume that degrading to a known value is better than outright dying. > If we really want to introduce "here is a setting you may not > understand, in which case you may safely ignore", the right way to > do so is to follow the model the index extension took, where from > the syntax of the unknown thing an old/existing code can tell if it > is optional. Forcing all codepaths to forever ignore what they do > not understand and what they happen to think is a safe fallback is > simply being irresponsible---the existing code does not understand > the new setting so they do not even know if their "current > behaviour" as a fallback is a safe and sensible one from the point > of view of the person who asked for the feature from the future. This seems needlessly complex. color.ui is one of the most prominent config variales, so you're proposing we split it up into some dual-key arrangement and force all users to migrate? I think just following what we're doing with http.sslVersion makes more sense.
