USLUGI REPETITORA
USLUGI REPETITORA 1-7 klass. Nizkie ceni 10:58:04 AM
Re: Confusing behavior with ignored submodules and `git commit -a`
+bmwill On 2018-11-14, Michael Forney wrote: > On 2018-10-25, Stefan Beller wrote: >> I guess reverting that commit is not a good idea now, as >> I would expect something to break. >> >> Maybe looking through the series 614ea03a71 >> (Merge branch 'bw/submodule-config-cleanup', 2017-08-26) >> to understand why it happened in the context would be a good start. > > Thanks, that's a good idea. I'll take a look through that series. Interesting. If I build git from master after reverting 55568086, I do indeed observe the issue it claims to fix (unable to add ignored submodules). However, if I build from 9ef23f91fc (the immediate parent of 55568086), I do not see the issue. Investigating this further, it seems that 55568086 addresses an issue that does not appear until later on in the series in ff6f1f564c (submodule-config: lazy-load a repository's .gitmodules file). Perhaps this was a result of reordering commits during a rebase. In other words, I get correct behavior until 55568086, and in 55568086..ff6f1f564c^ if I revert 55568086. Looking at ff6f1f564c, I don't really see anything that might be related to git-add, git-reset, or git-diff, so I'm guessing that this only worked before because the submodule config wasn't getting loaded during `git add` or `git reset`. Now that the config is loaded automatically, submodule..ignore started taking effect where it shouldn't. Unfortunately, this doesn't really get me much closer to finding a fix.
USLUGI REPETITORA
USLUGI REPETITORA 1-7 klass. Nizkie ceni 8:28:31 AM
Re: Confusing behavior with ignored submodules and `git commit -a`
On 2018-10-25, Stefan Beller wrote: > On Thu, Oct 25, 2018 at 11:03 AM Michael Forney > wrote: >> >> On 2018-03-16, Michael Forney wrote: >> > Hi, >> > >> > In the past few months have noticed some confusing behavior with >> > ignored submodules. I finally got around to bisecting this to commit >> > 5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure >> > submodules can be added or reset). > > Uh. :( > > See the discussion starting at > https://public-inbox.org/git/20170725213928.125998-4-bmw...@google.com/ > specifically > https://public-inbox.org/git/xmqqinieq49v@gitster.mtv.corp.google.com/ Thanks for the links. Let me explain how I'm using submodule..ignore. Maybe there's a better mechanism in git to deal with this (if .ignore is a misfeature). I have a git repository which contains a number of submodules that refer to external repositories. Some of these repositories need to patched in some way, so patches are stored alongside the submodules, and are applied when building. This mostly works fine, but causes submodules to show up as modified in `git status` and get updated with `git commit -a`. To resolve this, I've added `ignore = all` to .gitmodules for all the submodules that need patches applied. This way, I can explicitly `git add` the submodule when I want to update the base commit, but otherwise pretend that they are clean. This has worked pretty well for me, but less so since git 2.15 when this issue was introduced. Of course, I could maintain and publish forks of those repositories and use those as the remote for the submodules. However in many cases these patches are just temporary until they get applied upstream and a new release is made, and I don't really want to keep mirrors unnecessarily, or keep switching the submodule URL between upstream and my fork. >> > However, if I go to update `foo.txt` and >> > commit with `git commit -a`, changes to inner get recorded >> > unexpectedly. What's worse is the shortstat output of `git commit -a`, >> > and the diff output of `git show` give no indication that the >> > submodule was changed. > > This is really bad. git-status and git-commit share some code, > and we'll populate the commit message with a status output. > So it seems reasonable to expect the status and the commit to match, > i.e. if status tells me there is no change, then commit should not record > the submodule update. I just checked and if I don't specify a message on the command-line, the status output in the message template *does* mention that `inner` is getting updated. >> > There have been a couple occasions where I accidentally pushed local >> > changes to ignored submodules because of this. Since they don't show >> > up in the log output, it is difficult to figure out what actually has >> > gone wrong. > > How was it prevented before? Just by git commit -a not picking up the > submodule change? Yes. Previously, `git commit -a` would not pick up the change (unless I added it explicitly with `git add`), and `git log` would still show changes to ignored submodules (which is the behavior I want). > I guess reverting that commit is not a good idea now, as > I would expect something to break. > > Maybe looking through the series 614ea03a71 > (Merge branch 'bw/submodule-config-cleanup', 2017-08-26) > to understand why it happened in the context would be a good start. Thanks, that's a good idea. I'll take a look through that series. >> I accidentally pushed local changes to ignored submodules again due to >> this. >> >> Can anyone confirm whether this is the intended behavior of ignore? If >> it is, then at least the documentation needs an update saying that >> `commit -a` will commit all submodule changes, even if they are >> ignored. > > The docs say "(but it will nonetheless show up in the output of > status and commit when it has been staged)" as well, so that commit > sounds like a regression? I just came across someone else affected by this issue: https://github.com/git/git/commit/55568086#commitcomment-27137460
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
On Wed, Nov 14, 2018 at 02:08:48PM -0800, Stefan Beller wrote:
> On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren wrote:
> >
> > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget
> > wrote:
> > > However, the `.lock` file was still open and on Windows that means
> > > that it could not be deleted properly. This patch fixes that issue.
> >
> > Hmmm, doesn't the tempfile machinery remove the lock file when we die?
>
> On Windows this seems not to be the case. (Open files cannot be deleted
> as the open file is not kept by inode or similar but by the file path there?)
>
> Rewording your concern: Could the tempfile machinery be taught to
> work properly on Windows, e.g. by first closing all files and then deleting
> them afterwards?
It already tries to do so. See delete_tempfile(), or more likely in the
die() case, the remove_tempfiles() handler which is called at exit.
Are we sure this is still a problem?
I looked at the test to see if it would pass, but it is not even
checking anything about lockfiles! It just checks that we exit 1 by
returning up the callstack instead of calling die(). And of course it
would not have a problem under Linux either way. But if I run something
similar under strace, I see:
$ strace ./git bundle create foobar.bundle HEAD..HEAD
[...]
openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock",
O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
[...]
close(3)= 0
unlink("/home/peff/compile/git/foobar.bundle.lock") = 0
exit_group(128) = ?
which seems right.
-Peff
Re: [PATCH] INSTALL: add macOS gettext and sdk header explanation to INSTALL
Hi! yanke131...@gmail.com wrote: > From: out0fmemory > > --- > INSTALL | 7 +++ > 1 file changed, 7 insertions(+) Thanks for writing. A few bits of administrivia, from https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html: Can we forge your sign-off? See the section "certify your work" in SubmittingPatches for what this means. What name should we attribute this change to? Documentation/SubmittingPatches explains: Also notice that a real name is used in the Signed-off-by: line. Please don’t hide your real name. [...] > --- a/INSTALL > +++ b/INSTALL > @@ -165,6 +165,9 @@ Issues of note: > use English. Under autoconf the configure script will do this > automatically if it can't find libintl on the system. > > +In macOS, can install and link gettext with brew as "brew install > gettext" > +and "brew link --force gettext", the link operation is necessary. > + As context (e.g. to go in the commit message), can you tell us more about this? E.g. what happens if you don't perform the link operation? [...] > @@ -178,6 +181,10 @@ Issues of note: > will include them. Note that config.mak is not distributed; > the name is reserved for local settings. > > + - In macOs 10.14, the Command Line Tool not contains the headers as > before, so it > +need install Command Line Tool 10.14 and install the headers which > command > +"sudo installer -pkg > /Library/Developer/CommandLineTools/Packages/macOS_SDK_headers_for_macOS_10.14.pkg > -target". > + Likewise: what is the symptom if this isn't done? Is there more general advice we can put here that will survive past macOS 10.14? Thanks and hope that helps, Jonathan
[PATCH] INSTALL: add macOS gettext and sdk header explanation to INSTALL
From: out0fmemory --- INSTALL | 7 +++ 1 file changed, 7 insertions(+) diff --git a/INSTALL b/INSTALL index c39006e8e7..b7bfb53c12 100644 --- a/INSTALL +++ b/INSTALL @@ -165,6 +165,9 @@ Issues of note: use English. Under autoconf the configure script will do this automatically if it can't find libintl on the system. +In macOS, can install and link gettext with brew as "brew install gettext" +and "brew link --force gettext", the link operation is necessary. + - Python version 2.4 or later (but not 3.x, which is not supported by Perforce) is needed to use the git-p4 interface to Perforce. @@ -178,6 +181,10 @@ Issues of note: will include them. Note that config.mak is not distributed; the name is reserved for local settings. + - In macOs 10.14, the Command Line Tool not contains the headers as before, so it +need install Command Line Tool 10.14 and install the headers which command +"sudo installer -pkg /Library/Developer/CommandLineTools/Packages/macOS_SDK_headers_for_macOS_10.14.pkg -target". + - To build and install documentation suite, you need to have the asciidoc/xmlto toolchain. Because not many people are inclined to install the tools, the default build target -- 2.19.1.1052.gd166e6afe5
Urgent Respond Needed
I need am Honest Person to support and your cooperation with me in business of $26,700,000.00 thanks.
[PATCH] technical doc: add a design doc for the evolve command
From: Stefan Xenos This document describes what an obsolescence graph for git would look like, the behavior of the evolve command, and the changes planned for other commands. Signed-off-by: Stefan Xenos --- Documentation/technical/evolve.txt | 885 + 1 file changed, 885 insertions(+) create mode 100644 Documentation/technical/evolve.txt diff --git a/Documentation/technical/evolve.txt b/Documentation/technical/evolve.txt new file mode 100644 index 00..88470eada3 --- /dev/null +++ b/Documentation/technical/evolve.txt @@ -0,0 +1,885 @@ +Git Obsolescence Graph +== + +Objective +- +Track the edits to a commit over time in an obsolescence graph. + +Background +-- +Imagine you have three dependent changes up for review and you receive feedback +that requires editing all three changes. While you're editing one, more feedback +arrives on one of the others. What do you do? + +The evolve command is a convenient way to work with chains of commits that are +under review. Whenever you rebase or amend a commit, the repository remembers +that the old commit is obsolete and has been replaced by the new one. Then, at +some point in the future, you can run "git evolve" and the correct sequence of +rebases will occur in the correct order such that no commit has an obsolete +parent. + +Part of making the "evolve" command work involves tracking the edits to a commit +over time, which is why we need an obsolescence graph. However, the obsolescence +graph will also bring other benefits: + +- Users can view the history of a commit directly (the sequence of amends and + rebases it has undergone, orthogonal to the history of the branch it is on). +- It will be possible to quickly locate and list all the changes the user + currently has in progress. +- It can be used as part of other high-level commands that combine or split + changes. +- It can be used to decorate commits (in git log, gitk, etc) that are either + obsolete or are the tip of a work in progress. +- By pushing and pulling the obsolescence graph, users can collaborate more + easily on changes-in-progress. This is better than pushing and pulling the + changes themselves since the obsolescence graph can be used to locate a more + specific merge base, allowing for better merges between different versions of + the same change. +- It could be used to correctly rebase local changes and other local branches + after running git-filter-branch. +- It can replace the change-id footer used by gerrit. + +Similar technologies + +There are some other technologies that address the same end-user problem. + +Rebase -i can be used to solve the same problem, but users can't easily switch +tasks midway through an interactive rebase or have more than one interactive +rebase going on at the same time. It can't handle the case where you have +multiple changes sharing the same parent when that parent needs to be rebased +and won't let you collaborate with others on resolving a complicated interactive +rebase. You can think of rebase -i as a top-down approach and the evolve command +as the bottom-up approach to the same problem. + +Several patch queue managers have been built on top of git (such as topgit, +stgit, and quilt). They address the same user need. However they also rely on +state managed outside git that needs to be kept in sync. Such state can be +easily damaged when running a git native command that is unaware of the patch +queue. They also typically require an explicit initialization step to be done by +the user which creates workflow problems. + +Replacements (refs/replace) are superficially similar to obsolescences in that +they describe that one commit should be replaced by another. However, they +differ in both how they are created and how they are intended to be used. +Obsolescences are created automatically by the commands a user runs, and they +describe the user’s intent to perform a future rebase. Obsolete commits still +appear in branches, logs, etc like normal commits (possibly with an extra +decoration that marks them as obsolete). Replacements are typically created +explicitly by the user, they are meant to be kept around for a long time, and +they describe a replacement to be applied at read-time rather than as the input +to a future operation. When a replaced commit is queried, it is typically hidden +and swapped out with its replacement as though the replacement has already +occurred. + +Goals +- +Legend: Goals marked with P0 are required. Goals marked with Pn should be +attempted unless they interfere with goals marked with Pn-1. + +P0. All commands that modify commits (such as the normal commit --amend or +rebase command) should mark the old commit as being obsolete and replaced by +the new one. No additional commands should be required to keep the +obsolescence graph up-to-date. +P0. Any commit that may be involved in a future evolve
Re: [PATCH 1/3] eoie: default to not writing EOIE section
Junio C Hamano wrote: > Jonathan Nieder writes: >> 1. Using multiple versions of Git on a single machine. For example, >> some IDEs bundle a particular version of Git, which can be a >> different version from the system copy, or on a Mac, /usr/bin/git >> quickly goes out of sync with the Homebrew git in >> /usr/local/bin/git. > > Exactly this, especially the latter, is the answer to your > question in an earlier message: > >>> Am I understanding correctly? Can you give an example of when a user >>> would *want* to see this message and what they would do in response? > > The user may not be even aware of using another version of Git that > does not know how to take advantage of the version of Git you have > used in the repository, and it can be a mistake the user may want to > fix (e.g. by futzing with PATH). Ah, thanks much. I'll add a hint along those lines (e.g. warning: ignoring optional IEOT index extension hint: This is likely due to the file having been written by a newer hint: version of Git than is reading it. You can upgrade Git to hint: take advantage of performance improvements from the updated hint: file format. hint: hint: You can run "git config advice.unknownIndexExtension true" to hint: suppress this message. I am still vaguely uncomfortable with this since it seems analogous to warning that the server is advertising an unrecognized capability, but I can live with it. :) Patch coming in a few moments. Jonathan
Re: [PATCH 2/3] ieot: default to not writing IEOT section
Hi, Ben Peart wrote: > There is no way to get multi-threaded reads and NOT get the scary message > with older versions of git. Multi-threaded reads require the IEOT extension > to be written into the index and the existence of the IEOT extension in the > index will always generate the scary warning. This is where I think we differ. I want my local copy of Git to get multi-threaded reads as long as IEOT happens to be there, even if I am not ready to write IEOT myself yet. I understand that this differs from what you would prefer, so I'd like to find some compromise that makes us both happy. I've tried to suggest one: Make explicitly setting index.threads=true imply index.recordOffsetTable=true. That way, the default behavior is the behavior I prefer, and a client can simply set index.threads=true to get the behavior I think you are describing preferring. My preference is instead what I sent in patch 2/3 (for simplicity, especially since the default of index.recordOffsetTable=false would be only temporary), but this would work okay for me. I'll send this as a patch. If there is a reason it won't work for you, I would be very happy to learn more about why. Thanks, Jonathan
Re: [PATCH v2 08/11] fast-export: add --reference-excluded-parents option
On Wed, Nov 14, 2018 at 11:28 AM SZEDER Gábor wrote:
>
> On Tue, Nov 13, 2018 at 04:25:57PM -0800, Elijah Newren wrote:
> > diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> > index 2fef00436b..3cc98c31ad 100644
> > --- a/builtin/fast-export.c
> > +++ b/builtin/fast-export.c
> > @@ -37,6 +37,7 @@ static int fake_missing_tagger;
> > static int use_done_feature;
> > static int no_data;
> > static int full_tree;
> > +static int reference_excluded_commits;
> > static struct string_list extra_refs = STRING_LIST_INIT_NODUP;
> > static struct string_list tag_refs = STRING_LIST_INIT_NODUP;
> > static struct refspec refspecs = REFSPEC_INIT_FETCH;
> > @@ -596,7 +597,8 @@ static void handle_commit(struct commit *commit, struct
> > rev_info *rev,
> > message += 2;
> >
> > if (commit->parents &&
> > - get_object_mark(>parents->item->object) != 0 &&
> > + (get_object_mark(>parents->item->object) != 0 ||
> > + reference_excluded_commits) &&
> > !full_tree) {
> > parse_commit_or_die(commit->parents->item);
> > diff_tree_oid(get_commit_tree_oid(commit->parents->item),
> > @@ -644,13 +646,21 @@ static void handle_commit(struct commit *commit,
> > struct rev_info *rev,
> > unuse_commit_buffer(commit, commit_buffer);
> >
> > for (i = 0, p = commit->parents; p; p = p->next) {
> > - int mark = get_object_mark(>item->object);
> > - if (!mark)
> > + struct object *obj = >item->object;
> > + int mark = get_object_mark(obj);
> > +
> > + if (!mark && !reference_excluded_commits)
> > continue;
> > if (i == 0)
> > - printf("from :%d\n", mark);
> > + printf("from ");
> > + else
> > + printf("merge ");
> > + if (mark)
> > + printf(":%d\n", mark);
> > else
> > - printf("merge :%d\n", mark);
> > + printf("%s\n", sha1_to_hex(anonymize ?
> > +anonymize_sha1(>oid) :
> > +obj->oid.hash));
>
> Since we intend to move away from SHA-1, would this be a good time to
> add an anonymize_oid() function, "while at it"?
Since I already need to add a cleanup commit to remove the
pre-existing sha1_to_hex() calls, I'll just
s/anonymize_sha1/anonymize_oid/ while at it in the same commit; it's
not called from any other file.
> > i++;
> > }
> >
> > @@ -931,13 +941,22 @@ static void handle_tags_and_duplicates(struct
> > string_list *extras)
> > /*
> >* Getting here means we have a commit which
> >* was excluded by a negative refspec (e.g.
> > - * fast-export ^master master). If the user
> > + * fast-export ^master master). If we are
> > + * referencing excluded commits, set the ref
> > + * to the exact commit. Otherwise, the user
> >* wants the branch exported but every commit
> > - * in its history to be deleted, that sounds
> > - * like a ref deletion to me.
> > + * in its history to be deleted, which
> > basically
> > + * just means deletion of the ref.
> >*/
> > - printf("reset %s\nfrom %s\n\n",
> > -name, sha1_to_hex(null_sha1));
> > + if (!reference_excluded_commits) {
> > + /* delete the ref */
> > + printf("reset %s\nfrom %s\n\n",
> > +name, sha1_to_hex(null_sha1));
> > + continue;
> > + }
> > + /* set ref to commit using oid, not mark */
> > + printf("reset %s\nfrom %s\n\n", name,
> > +sha1_to_hex(commit->object.oid.hash));
>
> Please use oid_to_hex(>object.oid) instead.
Yeah, there were a couple others I introduced too. I'll fix them all up.
Re: [PATCH v2 04/11] fast-export: avoid dying when filtering by paths and old tags exist
On Wed, Nov 14, 2018 at 11:17 AM SZEDER Gábor wrote:
> On Tue, Nov 13, 2018 at 04:25:53PM -0800, Elijah Newren wrote:
> > diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> > index af724e9937..b984a44224 100644
> > --- a/builtin/fast-export.c
> > +++ b/builtin/fast-export.c
> > @@ -774,9 +774,12 @@ static void handle_tag(const char *name, struct tag
> > *tag)
> > break;
> > if (!(p->object.flags & TREESAME))
> > break;
> > - if (!p->parents)
> > - die("can't find replacement commit
> > for tag %s",
> > - oid_to_hex(>object.oid));
> > + if (!p->parents) {
> > + printf("reset %s\nfrom %s\n\n",
> > +name, sha1_to_hex(null_sha1));
>
> Please use oid_to_hex(_oid) instead.
Will do. Looks like origin/master:builtin/fast-export.c already had
two sha1_to_hex() calls, so I'll add a cleanup patch fixing those too.
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
Hi Phillip, On Mon, Nov 12, 2018 at 10:21 AM Phillip Wood wrote: > >> -Flags only understood by the am backend: > >> +The following options: > >> > >> * --committer-date-is-author-date > >> * --ignore-date > >> @@ -520,15 +512,12 @@ Flags only understood by the am backend: > >> * --ignore-whitespace > >> * -C > >> > >> -Flags understood by both merge and interactive backends: > >> +are incompatible with the following options: > >> > >> * --merge > >> * --strategy > >> * --strategy-option > >> * --allow-empty-message > >> - > >> -Flags only understood by the interactive backend: > >> - > > It's nice to see this being simplified :-) > >> -if test -n "$git_am_opt"; then > >> -incompatible_opts=$(echo " $git_am_opt " | \ > >> -sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/') > >> -if test -n "$interactive_rebase" > >> +incompatible_opts=$(echo " $git_am_opt " | \ > >> +sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/') > > > > Why are we no longer guarding this behind the condition that the user > > specified *any* option intended for the `am` backend? > > I was confused by this as well, what if the user asks for 'rebase > --exec= --ignore-whitespace'? They'd still get an error message about incompatible options; see my email to Dscho. However, since it tripped you both up, I'll make the clean up here a separate commit with some comments. > >> +if test -n "$incompatible_opts" > >> +then > >> +if test -n "$actually_interactive" || test "$do_merge" > >> then > >> -if test -n "$incompatible_opts" > >> -then > >> -die "$(gettext "error: cannot combine interactive > >> options (--interactive, --exec, --rebase-merges, --preserve-merges, > >> --keep-empty, --root + --onto) with am options ($incompatible_opts)")" > >> -fi > >> -fi > >> -if test -n "$do_merge"; then > >> -if test -n "$incompatible_opts" > >> -then > >> -die "$(gettext "error: cannot combine merge options > >> (--merge, --strategy, --strategy-option) with am options > >> ($incompatible_opts)")" > >> -fi > >> +die "$(gettext "error: cannot combine am options > >> ($incompatible_opts) with either interactive or merge options")" > >> fi > >> fi > >> > > If you want to change the error message here, I think you need to change > the corresponding message in builtin/rebase.c Indeed, will fix.
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
Hi Dscho, On Mon, Nov 12, 2018 at 8:21 AM Johannes Schindelin wrote: > > t3425: topology linearization was inconsistent across flavors of rebase, > > as already noted in a TODO comment in the testcase. This was not > > considered a bug before, so getting a different linearization due > > to switching out backends should not be considered a bug now. > > Ideally, the test would be fixed, then. If it fails for other reasons than > a real regression, it is not a very good regression test, is it. I agree, it's not the best regression test. It'd be better if it defined and tested for "the correct behavior", but I suspect it has some value in that it's is guaranteeing that the rebase flavors at least give a "somewhat reasonable" result. Sadly, It just allowed all three rebase types to have slightly different "somewhat reasonable" answers. > > > t5407: different rebase types varied slightly in how many times checkout > > or commit or equivalents were called based on a quick comparison > > of this tests and previous ones which covered different rebase > > flavors. I think this is just attributable to this difference. > > This concerns me. > > In bigger repositories (no, I am not talking about Linux kernel sized > ones, I consider those small-ish) there are a ton of files, and checkout > and commit (and even more so reset) sadly do not have a runtime complexity > growing with the number of modified files, but with the number of tracked > files (and some commands even with the number of worktree files). > > So a larger number of commit/checkout operations makes me expect > performance regressions. > > In this light, could you elaborate a bit on the differences you see > between rebase -i and rebase -m? I wrote this comment months ago and don't remember the full details. >From the wording and as best I remember, I suspect I was at least partially annoyed by the fact that these and other regression tests for rebase seemed to not be testing for "correct" behavior, but "existing" behavior. That wouldn't be so bad, except that existing behavior differed on the exact same test setup for different rebase backends and the differences in behavior were checked and enforced in the regression tests. So, it became a matter of taste as to how much things should be made identical and to which backend, or whether it was more important to at least just consolidate the code first and keep the results at least reasonable. At the time, I figured getting fewer backends, all with reasonable behavior was a bit more important, but since this difference is worrisome to you, I will try to dig further into it. > > t9903: --merge uses the interactive backend so the prompt expected is > > now REBASE-i. > > We should be able to make that test pass, still, by writing out a special > file (e.g. $state_dir/opt_m) and testing for that. Users are oddly upset > when their expectations are broken... (and I actually agree with them.) I agree users are upset when expectations are broken, but why would they expect REBASE-m? In fact, I thought the prompt was a reflection of what backend was in use, so that if someone reported an issue to the git mailing list or a power user wanted to know where the control files were located, they could look for them. As such, I thought that switching the prompt to REBASE-i was the right thing to do so that it would match user expectations. Yes, the backend changed; that's part of the release notes. Is there documentation somewhere that specifies the meaning of this prompt differently than the expectations I had somehow built up? > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > > index 3407d835bd..35084f5681 100644 > > --- a/Documentation/git-rebase.txt > > +++ b/Documentation/git-rebase.txt > > @@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below. > > INCOMPATIBLE OPTIONS > > > > > > -git-rebase has many flags that are incompatible with each other, > > -predominantly due to the fact that it has three different underlying > > -implementations: > > - > > - * one based on linkgit:git-am[1] (the default) > > - * one based on git-merge-recursive (merge backend) > > - * one based on linkgit:git-cherry-pick[1] (interactive backend) > > - > > Could we retain this part, with `s/three/two/` and `/git-merge/d`? *Maybe* > `s/interactive backend/interactive\/merge backend/` Removing this was actually a suggestion by Phillip back in June at the end of https://public-inbox.org/git/13ccb4d9-582b-d26b-f595-59cb0b703...@talktalk.net/, for the purpose of removing an implementation details that users don't need to know about. As noted elsewhere in the thread, between you and Phillip, I'll add some comments to the commit message about this. > > -if test -n "$git_am_opt"; then > > - incompatible_opts=$(echo " $git_am_opt " | \ > > - sed -e 's/ -q / /g' -e 's/^ \(.*\)
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren wrote: > > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget > wrote: > > However, the `.lock` file was still open and on Windows that means > > that it could not be deleted properly. This patch fixes that issue. > > Hmmm, doesn't the tempfile machinery remove the lock file when we die? On Windows this seems not to be the case. (Open files cannot be deleted as the open file is not kept by inode or similar but by the file path there?) Rewording your concern: Could the tempfile machinery be taught to work properly on Windows, e.g. by first closing all files and then deleting them afterwards? There was a refactoring of tempfiles merged in 89563ec379 (Merge branch 'jk/incore-lockfile-removal', 2017-09-19), which sounded promising at first, as it is dated after the original patch[1] date (June 2016), but it has no references for Windows being different, so we might still have the original issue; most of the lockfile infrastructure was done in 2015 via db86e61cbb (Merge branch 'mh/tempfile', 2015-08-25) [1] https://github.com/git-for-windows/git/pull/797
[PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity
The "git stash" command insists on having a usable user identity to
the same degree as the "git commit-tree" and "git commit" commands
do, because it uses the same codepath that creates commit objects
as these commands.
It is not strictly necesary to do so. Check if we will barf before
creating commit objects and then supply fake identity to please the
machinery that creates commits.
This is not that much of usability improvement, as the users who run
"git stash" would eventually want to record their changes that are
temporarily stored in the stashes in a more permanent history by
committing, and they must do "git config user.{name,email}" at that
point anyway, so arguably this change is only delaying a step that
is necessary to work in the repository.
Helped-by: Junio C Hamano
Signed-off-by: Slavica Djukic
---
git-stash.sh | 17 +
t/t3903-stash.sh | 2 +-
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/git-stash.sh b/git-stash.sh
index 94793c1a9..789ce2f41 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -55,6 +55,20 @@ untracked_files () {
git ls-files -o $z $excl_opt -- "$@"
}
+prepare_fallback_ident () {
+ if ! git -c user.useconfigonly=yes var GIT_COMMITTER_IDENT >/dev/null
2>&1
+ then
+ GIT_AUTHOR_NAME="git stash"
+ GIT_AUTHOR_EMAIL=git@stash
+ GIT_COMMITTER_NAME="git stash"
+ GIT_COMMITTER_EMAIL=git@stash
+ export GIT_AUTHOR_NAME
+ export GIT_AUTHOR_EMAIL
+ export GIT_COMMITTER_NAME
+ export GIT_COMMITTER_EMAIL
+ fi
+}
+
clear_stash () {
if test $# != 0
then
@@ -67,6 +81,9 @@ clear_stash () {
}
create_stash () {
+
+ prepare_fallback_ident
+
stash_msg=
untracked=
while test $# != 0
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index bab8bec67..0b0814421 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1096,7 +1096,7 @@ test_expect_success 'stash -- works with binary
files' '
test_path_is_file subdir/untracked
'
-test_expect_failure 'stash works when user.name and user.email are not set' '
+test_expect_success 'stash works when user.name and user.email are not set' '
git reset &&
git var GIT_COMMITTER_IDENT >expected &&
>1 &&
--
2.19.1.1052.gd166e6afe
[PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Add test to document that stash fails if user.name and user.email are not configured. In the later commit, test will be updated to expect success. Signed-off-by: Slavica Djukic --- t/t3903-stash.sh | 23 +++ 1 file changed, 23 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index cd216655b..bab8bec67 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1096,4 +1096,27 @@ test_expect_success 'stash -- works with binary files' ' test_path_is_file subdir/untracked ' +test_expect_failure 'stash works when user.name and user.email are not set' ' + git reset && + git var GIT_COMMITTER_IDENT >expected && + >1 && + git add 1 && + git stash && + git var GIT_COMMITTER_IDENT >actual && + test_cmp expected actual && + >2 && + git add 2 && + test_config user.useconfigonly true && + test_config stash.usebuiltin true && + ( + sane_unset GIT_AUTHOR_NAME && + sane_unset GIT_AUTHOR_EMAIL && + sane_unset GIT_COMMITTER_NAME && + sane_unset GIT_COMMITTER_EMAIL && + test_unconfig user.email && + test_unconfig user.name && + git stash + ) +' + test_done -- 2.19.1.1052.gd166e6afe
[PATCH v2 0/2] [Outreachy] make stash work if user.name and user.email are not configured
Changes since v1: *extend test to check whether git stash executes under valid ident (and not under fallback one) when there is such present *add prepare_fallback_ident() function to git-stash.sh to provide fallback identity Slavica Djukic (2): [Outreachy] t3903-stash: test without configured user.name and user.email [Outreachy] stash: tolerate missing user identity git-stash.sh | 17 + t/t3903-stash.sh | 23 +++ 2 files changed, 40 insertions(+) -- 2.19.1.1052.gd166e6afe
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget
wrote:
> However, the `.lock` file was still open and on Windows that means
> that it could not be deleted properly. This patch fixes that issue.
Hmmm, doesn't the tempfile machinery remove the lock file when we die?
> ref_count = write_bundle_refs(bundle_fd, );
> - if (!ref_count)
> - die(_("Refusing to create empty bundle."));
> - else if (ref_count < 0)
> + if (ref_count <= 0) {
> + if (!ref_count)
> + error(_("Refusing to create empty bundle."));
> goto err;
> + }
One less `die()` in libgit -- nice.
> +test_expect_success 'try to create a bundle with empty ref count' '
> + test_expect_code 1 git bundle create foobar.bundle master..master
> +'
This tries to make sure that we don't just die, but that we exit in a
"controlled" way with exit code 1. After reading the log message, I had
perhaps expected something like
test_must_fail git bundle [...] &&
test_path_is_missing foobar.bundle.lock
That relies on magically knowing the ".lock" suffix, but my point is
that for me (on Linux), this alternative test passes both before and
after the patch. Before, because tempfile.c cleans up; after, because
bundle.c does so. Doesn't this happen on Windows too? What am I missing?
Martin
Re: [PATCH v2 1/1] config: report a bug if git_dir exists without commondir
On Wed, Nov 14, 2018 at 05:59:02AM -0800, Johannes Schindelin via GitGitGadget
wrote:
> From: Johannes Schindelin
>
> This did happen at some stage, and was fixed relatively quickly. Make
> sure that we detect very quickly, too, should that happen again.
>
> Signed-off-by: Johannes Schindelin
> ---
> config.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/config.c b/config.c
> index 4051e38823..db6b0167c6 100644
> --- a/config.c
> +++ b/config.c
> @@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct
> config_options *opts,
>
> if (opts->commondir)
> repo_config = mkpathdup("%s/config", opts->commondir);
> + else if (opts->git_dir)
> + BUG("git_dir without commondir");
Yeah, I think this is the right thing to do. Thanks!
-Peff
Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories
On Wed, Nov 14, 2018 at 02:16:37PM +0100, Johannes Schindelin wrote: > > Makes perfect sense. Shouldn't we be asking where the template > > directory of the installed version is and using it instead of the > > freshly built one, no? > > It would make sense, but we don't know how to get that information, do we? > > $ git grep DEFAULT_GIT_TEMPLATE_DIR > Makefile: -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"' > builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR > builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR > "/usr/share/git-core/templates" > builtin/init-db.c: template_dir = to_free = > system_path(DEFAULT_GIT_TEMPLATE_DIR); > contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \ > > In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the > only user in the code is init-db.c which uses it in copy_templates(). > > And changing the code *now* to let us query Git where it thinks its > templates should be won't work, as this patch is about using the installed > Git (at whatever pre-compiled version that might be). Do we actually care where the templates are? I thought the point was to override for the built Git to use the built templates instead of the installed one. For an installed Git, shouldn't we not be overriding the templates at all? I.e.: if test -n "$GIT_TEST_INSTALLED" then "$GIT_TEST_INSTALLED/git" init else "$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt" fi >&3 2>&4 (That's all leaving aside the question of whether we ought to be using a clean template dir instead of this). -Peff
Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early
Hi Phillip,
On Wed, 14 Nov 2018, Phillip Wood wrote:
> Thanks for doing this, I think this patch is good.
Thanks!
> I've not checked the first patch as I think it is the same as before
> judging from the covering letter.
Indeed, that's what the range-diff said. Sorry for not stating this
explicitly.
Thank you for your review,
Dscho
>
> Best Wishes
>
> Phillip
>
> On 14/11/2018 16:25, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin
> >
> > It is a good idea to error out early upon seeing, say, `-Cbad`, rather
> > than starting the rebase only to have the `--am` backend complain later.
> >
> > Let's do this.
> >
> > The only options accepting parameters which we pass through to `git am`
> > (which may, or may not, forward them to `git apply`) are `-C` and
> > `--whitespace`. The other options we pass through do not accept
> > parameters, so we do not have to validate them here.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > builtin/rebase.c | 12 +++-
> > t/t3406-rebase-message.sh | 7 +++
> > 2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 96ffa80b71..571cf899d5 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv,
> > const char *prefix)
> >}
> >
> > for (i = 0; i < options.git_am_opts.argc; i++) {
> > - const char *option = options.git_am_opts.argv[i];
> > + const char *option = options.git_am_opts.argv[i], *p;
> > if (!strcmp(option, "--committer-date-is-author-date") ||
> > !strcmp(option, "--ignore-date") ||
> > !strcmp(option, "--whitespace=fix") ||
> > !strcmp(option, "--whitespace=strip"))
> > options.flags |= REBASE_FORCE;
> > + else if (skip_prefix(option, "-C", )) {
> > + while (*p)
> > + if (!isdigit(*(p++)))
> > + die(_("switch `C' expects a "
> > + "numerical value"));
> > + } else if (skip_prefix(option, "--whitespace=", )) {
> > + if (*p && strcmp(p, "warn") && strcmp(p, "nowarn")
> > &&
> > + strcmp(p, "error") && strcmp(p, "error-all"))
> > + die("Invalid whitespace option: '%s'", p);
> > + }
> >}
> >
> > if (!(options.flags & REBASE_NO_QUIET))
> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> > index 0392e36d23..2c79eed4fe 100755
> > --- a/t/t3406-rebase-message.sh
> > +++ b/t/t3406-rebase-message.sh
> > @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the
> > invalid ref' '
> > test_i18ngrep "invalid-ref" err
> > '
> >
> > +test_expect_success 'error out early upon -C or --whitespace='
> > '
> > + test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> > + test_i18ngrep "numerical value" err &&
> > + test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> > + test_i18ngrep "Invalid whitespace option" err
> > +'
> > +
> > test_done
> >
>
>
Re: [PATCH v3 1/1] protocol: advertise multiple supported versions
On 2018.11.14 11:38, Junio C Hamano wrote:
> Josh Steadmon writes:
>
> > On 2018.11.13 13:01, Junio C Hamano wrote:
> >> I am wondering if the code added by this patch outside this
> >> function, with if (strcmp(client_ad.buf, "version=0") sprinkled all
> >> over the place, works sensibly when the other side says "I prefer
> >> version=0 but I do not mind talking version=1".
> >
> > Depends on what you mean by "sensibly" :). In the current case, if the
> > client prefers v0, it will always end up using v0. After the fixes
> > described above, it will always use v0 unless the server no longer
> > supports v0. Is that what you would expect?
>
> Yes, that sounds like a sensible behaviour.
>
> What I was alludding to was a lot simpler, though. An advert string
> "version=0:version=1" from a client that prefers version 0 won't be
> !strcmp("version=0", advert) but seeing many strcmp() in the patch
> made me wonder.
Ah I see. The strcmp()s against "version=0" are special cases for where
it looks like the client does not understand any sort of version
negotiation. If we see multiple versions listed in the advert, then the
rest of the selection logic should do the right thing.
However, I think that it might work to remove the special cases. In the
event that the client is so old that it doesn't understand any form of
version negotiation, it should just ignore the version fields /
environment vars. If you think it's cleaner to remove the special cases,
let me know.
Re: [PATCH v4 0/1] Advertise multiple supported proto versions
On 2018.11.14 19:22, Junio C Hamano wrote: > Josh Steadmon writes: > > > Fix several bugs identified in v3, clarify commit message, and clean up > > extern keyword in protocol.h. > > It is good to descirbe the change relative to v3 here, which would > help those who are interested and reviewed v3. > > To help those who missed the boat and v4 is their first encounter > with this series, having the description relative to v3 alone and > nothing else is not very friendly, though. Ack. Will keep this in mind for future patches. > > + diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c > > + --- a/builtin/upload-archive.c > > + +++ b/builtin/upload-archive.c > > +@@ > > + #include "builtin.h" > > + #include "archive.h" > > + #include "pkt-line.h" > > ++#include "protocol.h" > > + #include "sideband.h" > > + #include "run-command.h" > > + #include "argv-array.h" > > +@@ > > + if (argc == 2 && !strcmp(argv[1], "-h")) > > + usage(upload_archive_usage); > > + > > ++ register_allowed_protocol_version(protocol_v0); > > ++ > > + /* > > +* Set up sideband subprocess. > > +* > > This one unfortunately seems to interact rather badly with your > js/remote-archive-v2 topic which is already in 'next', when this > topic is merged to 'pu', and my attempt to mechanically resolve > conflicts breaks t5000. Hmm, should we drop js/remote-archive-v2 then? Any solution that unblocks js/remote-archive-v2 will almost certainly depend on this patch.
[PATCH v2] Makefile: use FUZZ_CXXFLAGS for linking fuzzers
OSS-Fuzz requires C++-specific flags to link fuzzers. Passing these in CFLAGS causes lots of build warnings. Using separate FUZZ_CXXFLAGS avoids this. Signed-off-by: Josh Steadmon --- Since there's nothing else using CXXFLAGS, let's just make it explicit that these apply to the fuzzers. Range-diff against v1: 1: 1630a93f82 < -: -- Makefile: use CXXFLAGS for linking fuzzers -: -- > 1: 6b3d6dd7f0 Makefile: use FUZZ_CXXFLAGS for linking fuzzers Makefile | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index bbfbb4292d..0c05896797 100644 --- a/Makefile +++ b/Makefile @@ -3098,14 +3098,16 @@ cover_db_html: cover_db # An example command to build against libFuzzer from LLVM 4.0.0: # # make CC=clang CXX=clang++ \ -# CFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ +# FUZZ_CXXFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \ # fuzz-all # +FUZZ_CXXFLAGS ?= $(CFLAGS) + .PHONY: fuzz-all $(FUZZ_PROGRAMS): all - $(QUIET_LINK)$(CXX) $(CFLAGS) $(LIB_OBJS) $(BUILTIN_OBJS) \ + $(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) $(LIB_OBJS) $(BUILTIN_OBJS) \ $(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@ fuzz-all: $(FUZZ_PROGRAMS) -- 2.19.1.930.g4563a0d9d0-goog
Re: [PATCH v2 08/11] fast-export: add --reference-excluded-parents option
On Tue, Nov 13, 2018 at 04:25:57PM -0800, Elijah Newren wrote:
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 2fef00436b..3cc98c31ad 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -37,6 +37,7 @@ static int fake_missing_tagger;
> static int use_done_feature;
> static int no_data;
> static int full_tree;
> +static int reference_excluded_commits;
> static struct string_list extra_refs = STRING_LIST_INIT_NODUP;
> static struct string_list tag_refs = STRING_LIST_INIT_NODUP;
> static struct refspec refspecs = REFSPEC_INIT_FETCH;
> @@ -596,7 +597,8 @@ static void handle_commit(struct commit *commit, struct
> rev_info *rev,
> message += 2;
>
> if (commit->parents &&
> - get_object_mark(>parents->item->object) != 0 &&
> + (get_object_mark(>parents->item->object) != 0 ||
> + reference_excluded_commits) &&
> !full_tree) {
> parse_commit_or_die(commit->parents->item);
> diff_tree_oid(get_commit_tree_oid(commit->parents->item),
> @@ -644,13 +646,21 @@ static void handle_commit(struct commit *commit, struct
> rev_info *rev,
> unuse_commit_buffer(commit, commit_buffer);
>
> for (i = 0, p = commit->parents; p; p = p->next) {
> - int mark = get_object_mark(>item->object);
> - if (!mark)
> + struct object *obj = >item->object;
> + int mark = get_object_mark(obj);
> +
> + if (!mark && !reference_excluded_commits)
> continue;
> if (i == 0)
> - printf("from :%d\n", mark);
> + printf("from ");
> + else
> + printf("merge ");
> + if (mark)
> + printf(":%d\n", mark);
> else
> - printf("merge :%d\n", mark);
> + printf("%s\n", sha1_to_hex(anonymize ?
> +anonymize_sha1(>oid) :
> +obj->oid.hash));
Since we intend to move away from SHA-1, would this be a good time to
add an anonymize_oid() function, "while at it"?
> i++;
> }
>
> @@ -931,13 +941,22 @@ static void handle_tags_and_duplicates(struct
> string_list *extras)
> /*
>* Getting here means we have a commit which
>* was excluded by a negative refspec (e.g.
> - * fast-export ^master master). If the user
> + * fast-export ^master master). If we are
> + * referencing excluded commits, set the ref
> + * to the exact commit. Otherwise, the user
>* wants the branch exported but every commit
> - * in its history to be deleted, that sounds
> - * like a ref deletion to me.
> + * in its history to be deleted, which basically
> + * just means deletion of the ref.
>*/
> - printf("reset %s\nfrom %s\n\n",
> -name, sha1_to_hex(null_sha1));
> + if (!reference_excluded_commits) {
> + /* delete the ref */
> + printf("reset %s\nfrom %s\n\n",
> +name, sha1_to_hex(null_sha1));
> + continue;
> + }
> + /* set ref to commit using oid, not mark */
> + printf("reset %s\nfrom %s\n\n", name,
> +sha1_to_hex(commit->object.oid.hash));
Please use oid_to_hex(>object.oid) instead.
> continue;
> }
>
Re: [PATCH v2 04/11] fast-export: avoid dying when filtering by paths and old tags exist
On Tue, Nov 13, 2018 at 04:25:53PM -0800, Elijah Newren wrote:
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index af724e9937..b984a44224 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -774,9 +774,12 @@ static void handle_tag(const char *name, struct tag *tag)
> break;
> if (!(p->object.flags & TREESAME))
> break;
> - if (!p->parents)
> - die("can't find replacement commit for
> tag %s",
> - oid_to_hex(>object.oid));
> + if (!p->parents) {
> + printf("reset %s\nfrom %s\n\n",
> +name, sha1_to_hex(null_sha1));
Please use oid_to_hex(_oid) instead.
> + free(buf);
> + return;
> + }
> p = p->parents->item;
> }
> tagged_mark = get_object_mark(>object);
Re: [PATCH 8/9] sha1-file: use loose object cache for quick existence check
Am 13.11.2018 um 11:02 schrieb Ævar Arnfjörð Bjarmason:
> So here's the same test not against NFS, but the local ext4 fs (CO7;
> Linux 3.10) for sha1collisiondetection.git:
>
> Test origin/master
> peff/jk/loose-cacheavar/check-collisions-config
>
> --
> 0008.2: index-pack with 256*1 loose objects 0.02(0.02+0.00)
> 0.02(0.02+0.01) +0.0% 0.02(0.02+0.00) +0.0%
> 0008.3: index-pack with 256*10 loose objects 0.02(0.02+0.00)
> 0.03(0.03+0.00) +50.0% 0.02(0.02+0.00) +0.0%
> 0008.4: index-pack with 256*100 loose objects0.02(0.02+0.00)
> 0.17(0.16+0.01) +750.0%0.02(0.02+0.00) +0.0%
> 0008.5: index-pack with 256*250 loose objects0.02(0.02+0.00)
> 0.43(0.40+0.03) +2050.0% 0.02(0.02+0.00) +0.0%
> 0008.6: index-pack with 256*500 loose objects0.02(0.02+0.00)
> 0.88(0.80+0.09) +4300.0% 0.02(0.02+0.00) +0.0%
> 0008.7: index-pack with 256*750 loose objects0.02(0.02+0.00)
> 1.35(1.27+0.09) +6650.0% 0.02(0.02+0.00) +0.0%
> 0008.8: index-pack with 256*1000 loose objects 0.02(0.02+0.00)
> 1.83(1.70+0.14) +9050.0% 0.02(0.02+0.00) +0.0%
Ouch.
> And for mu.git, a ~20k object repo:
>
> Test origin/master
> peff/jk/loose-cache avar/check-collisions-config
>
> -
> 0008.2: index-pack with 256*1 loose objects 0.59(0.91+0.06)
> 0.58(0.93+0.03) -1.7% 0.57(0.89+0.04) -3.4%
> 0008.3: index-pack with 256*10 loose objects 0.59(0.91+0.07)
> 0.59(0.92+0.03) +0.0% 0.57(0.89+0.03) -3.4%
> 0008.4: index-pack with 256*100 loose objects0.59(0.91+0.05)
> 0.81(1.13+0.04) +37.3%0.58(0.91+0.04) -1.7%
> 0008.5: index-pack with 256*250 loose objects0.59(0.91+0.05)
> 1.23(1.51+0.08) +108.5% 0.58(0.91+0.04) -1.7%
> 0008.6: index-pack with 256*500 loose objects0.59(0.90+0.06)
> 1.96(2.20+0.12) +232.2% 0.58(0.91+0.04) -1.7%
> 0008.7: index-pack with 256*750 loose objects0.59(0.92+0.05)
> 2.72(2.92+0.17) +361.0% 0.58(0.90+0.04) -1.7%
> 0008.8: index-pack with 256*1000 loose objects 0.59(0.90+0.06)
> 3.50(3.67+0.21) +493.2% 0.57(0.90+0.04) -3.4%
>
> All of which is to say that I think it definitely makes sense to re-roll
> this with a perf test, and a switch to toggle it + docs explaining the
> caveats & pointing to the perf test. It's a clear win in some scenarios,
> but a big loss in others.
Right, but can we perhaps find a way to toggle it automatically, like
the special fetch-pack cache tried to do?
So the code needs to decide between using lstat() on individual loose
objects files or opendir()+readdir() to load the names in a whole
fan-out directory. Intuitively I'd try to solve it using red tape, by
measuring the duration of both kinds of calls, and then try to find a
heuristic based on those numbers. Is the overhead worth it?
But first, may I interest you in some further complication? We can
also use access(2) to check for the existence of files. It doesn't
need to fill in struct stat, so may have a slight advantage if we
don't need any of that information. The following patch is a
replacement for patch 8 and improves performance by ca. 3% with
git.git on an SSD for me; I'm curious to see how it does on NFS:
---
sha1-file.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/sha1-file.c b/sha1-file.c
index b77dacedc7..5315c37cbc 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -888,8 +888,13 @@ static int stat_sha1_file(struct repository *r, const
unsigned char *sha1,
prepare_alt_odb(r);
for (odb = r->objects->odb; odb; odb = odb->next) {
*path = odb_loose_path(odb, , sha1);
- if (!lstat(*path, st))
- return 0;
+ if (st) {
+ if (!lstat(*path, st))
+ return 0;
+ } else {
+ if (!access(*path, F_OK))
+ return 0;
+ }
}
return -1;
@@ -1171,7 +1176,8 @@ static int sha1_loose_object_info(struct repository *r,
if (!oi->typep && !oi->type_name && !oi->sizep && !oi->contentp) {
const char *path;
struct stat st;
- if (stat_sha1_file(r, sha1, , ) < 0)
+ struct stat *stp = oi->disk_sizep ? : NULL;
+ if (stat_sha1_file(r, sha1, stp, ) < 0)
return -1;
if (oi->disk_sizep)
*oi->disk_sizep = st.st_size;
@@ -1382,7 +1388,6 @@ void *read_object_file_extended(const struct object_id
*oid,
void
Re: [PATCH 3/3] index: do not warn about unrecognized extensions
On 11/13/2018 10:24 PM, Junio C Hamano wrote: Jonathan Nieder writes: We cannot change the past, but for index extensions of the future, there is a straightforward improvement: silence that message except when tracing. This way, the message is still available when debugging, but in everyday use it does not show up so (once most Git users have this patch) we can turn on new optional extensions right away without alarming people. That argument ignores the "let the users know they are using a stale version when they did use (either by accident or deliberately) a more recent one" value, though. Even if we consider that this is only for debugging, I am not sure if tracing is the right place to add. As long as the "optional extensions can be ignored without affecting the correctness" rule holds, there is nothing gained by letting these messages shown for debugging purposes Having recently written a couple of patches that utilize an optional extension - I actually found the warning to be a helpful debugging tool and would like to see them enabled via tracing. It would also be helpful to see the opposite - I'm looking for an optional extension but it is missing. The most common scenario was when I'd be testing my changes in different repos and forget that I needed to force an updated index to be written that contained the extension I was trying to test. The "silently ignore the optional extension" behavior is good for end users but as a developer, I'd like to be able to have it yell at me via tracing. :-) IMHO - if an end user has to turn on tracing, I view that as a failure on our part. No end user should have to understand the inner workings of git to be able to use it effectively. and if there is such a bug (e.g. we introduced an optional extension but the code that wrote an index with an optional extension wrote the non-optional part in such a way that it cannot be correctly handled without the extension that is supposed to be optional) we'd probably want to let users notice without having to explicitly go into a debugging session. If Googling for "ignoring FNCY ext" gives "discard your index with 'reset HEAD', because an index file with FNCY ext cannot be read without understanding it", that may prevent damages from happening in the first place. On the other hand, hiding it behind tracing would mean the user first need to exprience an unknown breakage first and then has to enable tracing among other 47 different things to diagnose and drill down to the root cause.
Re: [PATCH 2/3] ieot: default to not writing IEOT section
On 11/13/2018 4:08 PM, Jonathan Nieder wrote:
Hi again,
Ben Peart wrote:
On 11/13/2018 1:18 PM, Jonathan Nieder wrote:
Ben Peart wrote:
Why introduce a new setting to disable writing the IEOT extension instead of
just using the existing index.threads setting? If index.threads=1 then the
IEOT extension isn't written which (I believe) will accomplish the same
goal.
Do you mean defaulting to index.threads=1? I don't think that would
be a good default, but if you have a different change in mind then I'd
be happy to hear it.
Or do you mean that if the user has explicitly specified index.threads=true,
then that should imply index.recordOffsetTable=true so users only have
to set one setting to turn it on? I can imagine that working well.
Reading the index with multiple threads requires the EOIE and IEOT
extensions to exist in the index. If either extension doesn't exist, then
the code falls back to the single threaded path. That means you can't have
both 1) no warning for old versions of git and 2) multi-threaded reading for
new versions of git.
If you set index.threads=1, that will prevent the IEOT extension from being
written and there will be no "ignoring IEOT extension" warning in older
versions of git.
With this patch 'as is' you would have to set both index.threads=true and
index.recordOffsetTable=true to get multi-threaded index reads. If either
is set to false, it will silently drop back to single threaded reads.
Sorry, I'm still not understanding what you're proposing. What would be
- the default behavior
- the mechanism for changing that behavior
under your proposal?
I consider index.threads=1 to be a bad default. I would understand if
you are saying that that should be the default, and I tried to propose
a different way to achieve what you're looking for in the quoted reply
above (but I'm not understanding whether you like that proposal or
not).
Today, both the write logic (ie should we write out the IEOT extension)
and the read logic (should I use the IEOT, if available, and do
multi-threaded reading) are controlled by the single "index.threads"
setting. I would like to keep the settings as simple as possible to
prevent user confusion.
If we have two different settings (index.threads and
index.recordoffsettable) then the only combination that will result in
the user actually getting multi-threaded reads is when they are both set
to true. Any other combination will silently fail. I think it would be
confusing if you set index.threads=true and got no error message but
didn't get multi-threaded reads either (or vice versa).
If you want to prevent any of the scary "ignoring IEOT extension" from
ever happening then your only option is to turn off the IEOT writing by
default. The downside is that people have to discover and turn it on if
they want the improvements. This can be achieved by changing the
default for index.threads from "true" to "false."
diff --git a/config.c b/config.c
index 2ee29f6f86..86f5c14294 100644
--- a/config.c
+++ b/config.c
@@ -2291,7 +2291,7 @@ int git_config_get_fsmonitor(void)
int git_config_get_index_threads(void)
{
- int is_bool, val = 0;
+ int is_bool, val = 1;
val = git_env_ulong("GIT_TEST_INDEX_THREADS", 0);
if (val)
If you want to provide a way for a concerned user to disable the message
after the first time they have seen it, then they can be instructed to
run 'git config --global index.threads false'
There is no way to get multi-threaded reads and NOT get the scary
message with older versions of git. Multi-threaded reads require the
IEOT extension to be written into the index and the existence of the
IEOT extension in the index will always generate the scary warning.
Jonathan
Re: [RFC PATCH 1/2] commit: don't add scissors line if one exists
On Wed, Nov 14, 2018 at 05:06:32PM +0900, Junio C Hamano wrote: > Denton Liu writes: > > > If commit.cleanup = scissors is specified, don't produce a scissors line > > if one already exists in the commit message. > > It is good that you won't have two such lines in the end result, but > is this (1) hiding real problem under the rug? (2) losing information? > > If the current invocation of "git commit" added a scissors line in > the buffer to be edited already, and we are adding another one in > this function, is it possible that the real problem that somebody > else has called wt_status_add_cut_line() before this function is > called, in which case that other caller is what we need to fix, > instead of this one? > In patch 2/2, I intentionally inserted a scissors line into MERGE_MSG so this patch ensures that we don't get duplicate scissors. > If the existing line in the buffer came from the end user (perhaps > it was given from "-F ", etc., with "-e" option) or --amend, > how can we be sure if it is OK to lose everything after that > scissors looking line? In other words, the scissors looking line > may just be part of the log message, in which case we may want to > quote/escape it, so that the true scissors we append at a later > place in the buffer would be noticed without losing the text before > and after that scissors looking line we already had when this > function was called? > With the existing behaviour, any messages that contain a scissors looking line will get cut at the earliest scissors anyway, so I believe that this patch would not change the behaviour. If the users were dealing with commit messages with a scissors looking line, the current behaviour already requires users to be extra careful to ensure that the scissors don't get accidentally removed so in the interest of preserving the existing behaviour, I don't think that any extra information would be lost from this patch.
Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early
Hi Johannes
Thanks for doing this, I think this patch is good. I've not checked the
first patch as I think it is the same as before judging from the
covering letter.
Best Wishes
Phillip
On 14/11/2018 16:25, Johannes Schindelin via GitGitGadget wrote:
From: Johannes Schindelin
It is a good idea to error out early upon seeing, say, `-Cbad`, rather
than starting the rebase only to have the `--am` backend complain later.
Let's do this.
The only options accepting parameters which we pass through to `git am`
(which may, or may not, forward them to `git apply`) are `-C` and
`--whitespace`. The other options we pass through do not accept
parameters, so we do not have to validate them here.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 12 +++-
t/t3406-rebase-message.sh | 7 +++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 96ffa80b71..571cf899d5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
}
for (i = 0; i < options.git_am_opts.argc; i++) {
- const char *option = options.git_am_opts.argv[i];
+ const char *option = options.git_am_opts.argv[i], *p;
if (!strcmp(option, "--committer-date-is-author-date") ||
!strcmp(option, "--ignore-date") ||
!strcmp(option, "--whitespace=fix") ||
!strcmp(option, "--whitespace=strip"))
options.flags |= REBASE_FORCE;
+ else if (skip_prefix(option, "-C", )) {
+ while (*p)
+ if (!isdigit(*(p++)))
+ die(_("switch `C' expects a "
+ "numerical value"));
+ } else if (skip_prefix(option, "--whitespace=", )) {
+ if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
+ strcmp(p, "error") && strcmp(p, "error-all"))
+ die("Invalid whitespace option: '%s'", p);
+ }
}
if (!(options.flags & REBASE_NO_QUIET))
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 0392e36d23..2c79eed4fe 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
test_i18ngrep "invalid-ref" err
'
+test_expect_success 'error out early upon -C or --whitespace=' '
+ test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
+ test_i18ngrep "numerical value" err &&
+ test_must_fail git rebase --whitespace=bad HEAD 2>err &&
+ test_i18ngrep "Invalid whitespace option" err
+'
+
test_done
[PATCH v2 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories
From: Johannes Schindelin
It really makes very, very little sense to use a different git
executable than the one the caller indicated via setting the environment
variable GIT_TEST_INSTALLED.
Signed-off-by: Johannes Schindelin
---
t/test-lib-functions.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index d158c8d0bf..3472716651 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -923,7 +923,8 @@ test_create_repo () {
mkdir -p "$repo"
(
cd "$repo" || error "Cannot setup test environment"
- "$GIT_EXEC_PATH/git-init"
"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
+ "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+ "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
error "cannot run git init -- have you built things yet?"
mv .git/hooks .git/hooks-disabled
) || exit
--
gitgitgadget
[PATCH v2 4/5] tests: do not require Git to be built when testing an installed Git
From: Johannes Schindelin
We really only need the test helpers to be built in the worktree in that
case, but that is not what we test for.
On the other hand it is a perfect opportunity to verify that
`GIT_TEST_INSTALLED` points to a working Git.
So let's test the appropriate Git executable. While at it, also adjust
the error message in the `GIT_TEST_INSTALLED` case.
This patch is best viewed with `-w --patience`.
Helped-by: Jeff King
Signed-off-by: Johannes Schindelin
---
t/test-lib.sh | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 93883580a8..3d3a65ed0e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -51,10 +51,15 @@ export LSAN_OPTIONS
# It appears that people try to run tests without building...
-"$GIT_BUILD_DIR/git" >/dev/null
+"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git" >/dev/null
if test $? != 1
then
- echo >&2 'error: you do not seem to have built git yet.'
+ if test -n "$GIT_TEST_INSTALLED"
+ then
+ echo >&2 "error: there is no working Git at
'$GIT_TEST_INSTALLED'"
+ else
+ echo >&2 'error: you do not seem to have built git yet.'
+ fi
exit 1
fi
--
gitgitgadget
[PATCH v2 3/5] t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set
From: Johannes Schindelin It makes very, very little sense to test the built git-sh-i18n when the user asked specifically to test another one. Signed-off-by: Johannes Schindelin --- t/lib-gettext.sh | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh index eec757f104..9eb160c997 100644 --- a/t/lib-gettext.sh +++ b/t/lib-gettext.sh @@ -10,7 +10,12 @@ GIT_TEXTDOMAINDIR="$GIT_BUILD_DIR/po/build/locale" GIT_PO_PATH="$GIT_BUILD_DIR/po" export GIT_TEXTDOMAINDIR GIT_PO_PATH -. "$GIT_BUILD_DIR"/git-sh-i18n +if test -n "$GIT_TEST_INSTALLED" +then + . "$(git --exec-path)"/git-sh-i18n +else + . "$GIT_BUILD_DIR"/git-sh-i18n +fi if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON then -- gitgitgadget
[PATCH v2 5/5] tests: explicitly use `git.exe` on Windows
From: Johannes Schindelin
On Windows, when we refer to `/an/absolute/path/to/git`, it magically
resolves `git.exe` at that location. Except if something of the name
`git` exists next to that `git.exe`. So if we call `$BUILD_DIR/git`, it
will find `$BUILD_DIR/git.exe` *only* if there is not, say, a directory
called `$BUILD_DIR/git`.
Such a directory, however, exists in Git for Windows when building with
Visual Studio (our Visual Studio project generator defaults to putting
the build files into a directory whose name is the base name of the
corresponding `.exe`).
In the bin-wrappers/* scripts, we already take pains to use `git.exe`
rather than `git`, as this could pick up the wrong thing on Windows
(i.e. if there exists a `git` file or directory in the build directory).
Now we do the same in the tests' start-up code.
This also helps when testing an installed Git, as there might be even
more likely some stray file or directory in the way.
Note: the only way we can record whether the `.exe` suffix is by writing
it to the `GIT-BUILD-OPTIONS` file and sourcing it at the beginning of
`t/test-lib.sh`. This is not a requirement introduced by this patch, but
we move the call to be able to use the `$X` variable that holds the file
extension, if any.
Note also: the many, many calls to `git this` and `git that` are
unaffected, as the regular PATH search will find the `.exe` files on
Windows (and not be confused by a directory of the name `git` that is
in one of the directories listed in the `PATH` variable), while
`/path/to/git` would not, per se, know that it is looking for an
executable and happily prefer such a directory.
Signed-off-by: Johannes Schindelin
---
Makefile| 1 +
t/test-lib-functions.sh | 2 +-
t/test-lib.sh | 13 +
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/Makefile b/Makefile
index 016fdcdb81..21b3978744 100644
--- a/Makefile
+++ b/Makefile
@@ -2591,6 +2591,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+ @echo X=\'$(X)\' >>$@+
ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
endif
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3472716651..274cbc2d6e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -923,7 +923,7 @@ test_create_repo () {
mkdir -p "$repo"
(
cd "$repo" || error "Cannot setup test environment"
- "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+ "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" init \
"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
error "cannot run git init -- have you built things yet?"
mv .git/hooks .git/hooks-disabled
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3d3a65ed0e..e12addc324 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -49,9 +49,17 @@ export ASAN_OPTIONS
: ${LSAN_OPTIONS=abort_on_error=1}
export LSAN_OPTIONS
+if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+then
+ echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
+ exit 1
+fi
+. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+export PERL_PATH SHELL_PATH
+
# It appears that people try to run tests without building...
-"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git" >/dev/null
+"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
if test $? != 1
then
if test -n "$GIT_TEST_INSTALLED"
@@ -63,9 +71,6 @@ then
exit 1
fi
-. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-export PERL_PATH SHELL_PATH
-
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
case "$GIT_TEST_TEE_STARTED, $* " in
--
gitgitgadget
[PATCH v2 1/5] tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/
From: Johannes Schindelin
We really need to be able to find the test helpers... Really. This
change was forgotten when we moved the test helpers into t/helper/
Signed-off-by: Johannes Schindelin
---
t/test-lib.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index aba66cafa2..93883580a8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -957,7 +957,7 @@ elif test -n "$GIT_TEST_INSTALLED"
then
GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path) ||
error "Cannot run git from $GIT_TEST_INSTALLED."
- PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR:$PATH
+ PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
else # normal case, use ../bin-wrappers only unless $with_dashes:
git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"
--
gitgitgadget
[PATCH v2 0/5] tests: various improvements to the GIT_TEST_INSTALLED feature
By setting the GIT_TEST_INSTALLED variable to the path of an installed Git
executable, it is possible to run the test suite also on a specific
installed version (as opposed to a version built from scratch).
The only thing this needs that is unlikely to be installed is the test
helper(s).
However, there have been a few rough edges around that, identified in my
(still ongoing) work to support building Git in Visual Studio (where we do
not want to run GNU Make, and where we have no canonical way to create, say,
hard-linked copies of the built-in commands, and other work to let Git for
Windows play better with BusyBox.
Triggered by a comment of AEvar
[https://public-inbox.org/git/20181102223743.4331-1-ava...@gmail.com/], I
hereby contribute these assorted fixes for the GIT_TEST_INSTALLED feature.
Changes since v1:
* Now we verify in test-lib.sh also in the GIT_TEST_INSTALLED case whether
the Git executable is working (thanks, Peff!).
* The commit message of 5/5 was touched up.
Johannes Schindelin (5):
tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/
tests: respect GIT_TEST_INSTALLED when initializing repositories
t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set
tests: do not require Git to be built when testing an installed Git
tests: explicitly use `git.exe` on Windows
Makefile| 1 +
t/lib-gettext.sh| 7 ++-
t/test-lib-functions.sh | 3 ++-
t/test-lib.sh | 22 --
4 files changed, 25 insertions(+), 8 deletions(-)
base-commit: d166e6afe5f257217836ef24a73764eba390c58d
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-73%2Fdscho%2Ftest-git-installed-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-73/dscho/test-git-installed-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/73
Range-diff vs v1:
1: 2b04f9f086 = 1: 3b68e0fe8a tests: fix GIT_TEST_INSTALLED's PATH to
include t/helper/
2: 948b3dc146 = 2: 80d50d5932 tests: respect GIT_TEST_INSTALLED when
initializing repositories
3: eddea552e4 = 3: 49e408677a t/lib-gettext: test installed git-sh-i18n if
GIT_TEST_INSTALLED is set
4: 316e215e54 < -: -- tests: do not require Git to be built when
testing an installed Git
-: -- > 4: b801dc8027 tests: do not require Git to be built when
testing an installed Git
5: cd314e1384 ! 5: fbdb659de6 tests: explicitly use `git.exe` on Windows
@@ -2,6 +2,17 @@
tests: explicitly use `git.exe` on Windows
+On Windows, when we refer to `/an/absolute/path/to/git`, it magically
+resolves `git.exe` at that location. Except if something of the name
+`git` exists next to that `git.exe`. So if we call `$BUILD_DIR/git`,
it
+will find `$BUILD_DIR/git.exe` *only* if there is not, say, a
directory
+called `$BUILD_DIR/git`.
+
+Such a directory, however, exists in Git for Windows when building
with
+Visual Studio (our Visual Studio project generator defaults to putting
+the build files into a directory whose name is the base name of the
+corresponding `.exe`).
+
In the bin-wrappers/* scripts, we already take pains to use `git.exe`
rather than `git`, as this could pick up the wrong thing on Windows
(i.e. if there exists a `git` file or directory in the build
directory).
@@ -68,11 +79,12 @@
+
# It appears that people try to run tests without building...
--test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
-+test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git$X" >/dev/null ||
+-"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git" >/dev/null
++"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
if test $? != 1
then
- echo >&2 'error: you do not seem to have built git yet.'
+ if test -n "$GIT_TEST_INSTALLED"
+@@
exit 1
fi
--
gitgitgadget
[PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early
From: Johannes Schindelin
It is a good idea to error out early upon seeing, say, `-Cbad`, rather
than starting the rebase only to have the `--am` backend complain later.
Let's do this.
The only options accepting parameters which we pass through to `git am`
(which may, or may not, forward them to `git apply`) are `-C` and
`--whitespace`. The other options we pass through do not accept
parameters, so we do not have to validate them here.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 12 +++-
t/t3406-rebase-message.sh | 7 +++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 96ffa80b71..571cf899d5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
}
for (i = 0; i < options.git_am_opts.argc; i++) {
- const char *option = options.git_am_opts.argv[i];
+ const char *option = options.git_am_opts.argv[i], *p;
if (!strcmp(option, "--committer-date-is-author-date") ||
!strcmp(option, "--ignore-date") ||
!strcmp(option, "--whitespace=fix") ||
!strcmp(option, "--whitespace=strip"))
options.flags |= REBASE_FORCE;
+ else if (skip_prefix(option, "-C", )) {
+ while (*p)
+ if (!isdigit(*(p++)))
+ die(_("switch `C' expects a "
+ "numerical value"));
+ } else if (skip_prefix(option, "--whitespace=", )) {
+ if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
+ strcmp(p, "error") && strcmp(p, "error-all"))
+ die("Invalid whitespace option: '%s'", p);
+ }
}
if (!(options.flags & REBASE_NO_QUIET))
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 0392e36d23..2c79eed4fe 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
test_i18ngrep "invalid-ref" err
'
+test_expect_success 'error out early upon -C or --whitespace=' '
+ test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
+ test_i18ngrep "numerical value" err &&
+ test_must_fail git rebase --whitespace=bad HEAD 2>err &&
+ test_i18ngrep "Invalid whitespace option" err
+'
+
test_done
--
gitgitgadget
[PATCH v2 1/2] rebase: really just passthru the `git am` options
From: Johannes Schindelin
Currently, we parse the options intended for `git am` as if we wanted to
handle them in `git rebase`, and then reconstruct them painstakingly to
define the `git_am_opt` variable.
However, there is a much better way (that I was unaware of, at the time
when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.
It is intended for exactly this use case, where command-line options
want to be parsed into a separate `argv_array`.
Let's use this feature.
Incidentally, this also allows us to address a bug discovered by Phillip
Wood, where the built-in rebase failed to understand that the `-C`
option takes an optional argument.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 98 +---
1 file changed, 35 insertions(+), 63 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..96ffa80b71 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -87,7 +87,7 @@ struct rebase_options {
REBASE_FORCE = 1<<3,
REBASE_INTERACTIVE_EXPLICIT = 1<<4,
} flags;
- struct strbuf git_am_opt;
+ struct argv_array git_am_opts;
const char *action;
int signoff;
int allow_rerere_autoupdate;
@@ -339,7 +339,7 @@ N_("Resolve all conflicts manually, mark them as resolved
with\n"
static int run_specific_rebase(struct rebase_options *opts)
{
const char *argv[] = { NULL, NULL };
- struct strbuf script_snippet = STRBUF_INIT;
+ struct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;
int status;
const char *backend, *backend_func;
@@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options *opts)
oid_to_hex(>restrict_revision->object.oid) : NULL);
add_var(_snippet, "GIT_QUIET",
opts->flags & REBASE_NO_QUIET ? "" : "t");
- add_var(_snippet, "git_am_opt", opts->git_am_opt.buf);
+ sq_quote_argv_pretty(, opts->git_am_opts.argv);
+ add_var(_snippet, "git_am_opt", buf.buf);
+ strbuf_release();
add_var(_snippet, "verbose",
opts->flags & REBASE_VERBOSE ? "t" : "");
add_var(_snippet, "diffstat",
@@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
struct rebase_options options = {
.type = REBASE_UNSPECIFIED,
.flags = REBASE_NO_QUIET,
- .git_am_opt = STRBUF_INIT,
+ .git_am_opts = ARGV_ARRAY_INIT,
.allow_rerere_autoupdate = -1,
.allow_empty_message = 1,
.git_format_patch_opt = STRBUF_INIT,
@@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
ACTION_EDIT_TODO,
ACTION_SHOW_CURRENT_PATCH,
} action = NO_ACTION;
- int committer_date_is_author_date = 0;
- int ignore_date = 0;
- int ignore_whitespace = 0;
const char *gpg_sign = NULL;
- int opt_c = -1;
- struct string_list whitespace = STRING_LIST_INIT_NODUP;
struct string_list exec = STRING_LIST_INIT_NODUP;
const char *rebase_merges = NULL;
int fork_point = -1;
@@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
{OPTION_NEGBIT, 'n', "no-stat", , NULL,
N_("do not show diffstat of what changed upstream"),
PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
- OPT_BOOL(0, "ignore-whitespace", _whitespace,
-N_("passed to 'git apply'")),
OPT_BOOL(0, "signoff", ,
N_("add a Signed-off-by: line to each commit")),
- OPT_BOOL(0, "committer-date-is-author-date",
-_date_is_author_date,
-N_("passed to 'git am'")),
- OPT_BOOL(0, "ignore-date", _date,
-N_("passed to 'git am'")),
+ OPT_PASSTHRU_ARGV(0, "ignore-whitespace", _am_opts,
+ NULL, N_("passed to 'git am'"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
+ _am_opts, NULL,
+ N_("passed to 'git am'"), PARSE_OPT_NOARG),
+ OPT_PASSTHRU_ARGV(0, "ignore-date", _am_opts, NULL,
+ N_("passed to 'git am'"), PARSE_OPT_NOARG),
+ OPT_PASSTHRU_ARGV('C', NULL, _am_opts, N_("n"),
+ N_("passed to 'git apply'"), 0),
+ OPT_PASSTHRU_ARGV(0, "whitespace", _am_opts,
+ N_("action"), N_("passed to 'git apply'"), 0),
OPT_BIT('f', "force-rebase", ,
N_("cherry-pick all commits, even if unchanged"),
REBASE_FORCE),
@@ -856,10 +858,6
[PATCH v2 0/2] rebase: understand -C again, refactor
Phillip Wood reported a problem where the built-in rebase did not understand options like -C1, i.e. it did not expect the option argument. While investigating how to address this best, I stumbled upon OPT_PASSTHRU_ARGV (which I was so far happily unaware of). Instead of just fixing the -C bug, I decided to simply convert all of the options intended for git am (or, eventually, for git apply). This happens to fix that bug, and does so much more: it simplifies the entire logic (and removes more lines than it adds). Change since v1: * Introduce early parameter validation for the options passed through to git am. Johannes Schindelin (2): rebase: really just passthru the `git am` options rebase: validate -C and --whitespace= parameters early builtin/rebase.c | 108 -- t/t3406-rebase-message.sh | 7 +++ 2 files changed, 52 insertions(+), 63 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-76%2Fdscho%2Frebase-Cn-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-76/dscho/rebase-Cn-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/76 Range-diff vs v1: 1: dc36a45068 = 1: dc36a45068 rebase: really just passthru the `git am` options -: -- > 2: 4c2ba52766 rebase: validate -C and --whitespace= parameters early -- gitgitgadget
[PATCH] doc: move extensions.worktreeConfig to the right place
All config extensions are described in technical/repository-version.txt. I made a mistake of adding it in config.txt instead. This patch moves it back to where it belongs. Since repository-version.txt is not part of officially generated documents (it's not even part of DOC_HTML target), it's only visible to developers who read plain .txt files. Let's include it in gitrepository-layout.5 for more visibility. Some minor asciidoc fixes are required in repository-version.txt to make this happen. Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/config.txt | 7 - Documentation/gitrepository-layout.txt| 2 ++ .../technical/repository-version.txt | 26 ++- 3 files changed, 16 insertions(+), 19 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 3e735f1a9a..d87846faa6 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -292,13 +292,6 @@ include::config/advice.txt[] include::config/core.txt[] -extensions.worktreeConfig:: - If set, by default "git config" reads from both "config" and - "config.worktree" file from GIT_DIR in that order. In - multiple working directory mode, "config" file is shared while - "config.worktree" is per-working directory (i.e., it's in - GIT_COMMON_DIR/worktrees//config.worktree) - include::config/add.txt[] include::config/alias.txt[] diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt index d501af9d77..366dee238c 100644 --- a/Documentation/gitrepository-layout.txt +++ b/Documentation/gitrepository-layout.txt @@ -290,6 +290,8 @@ worktrees//locked:: worktrees//config.worktree:: Working directory specific configuration file. +include::technical/repository-version.txt[] + SEE ALSO linkgit:git-init[1], diff --git a/Documentation/technical/repository-version.txt b/Documentation/technical/repository-version.txt index e03eaccebc..7844ef30ff 100644 --- a/Documentation/technical/repository-version.txt +++ b/Documentation/technical/repository-version.txt @@ -1,5 +1,4 @@ -Git Repository Format Versions -== +== Git Repository Format Versions Every git repository is marked with a numeric version in the `core.repositoryformatversion` key of its `config` file. This version @@ -40,16 +39,14 @@ format by default. The currently defined format versions are: -Version `0` +=== Version `0` This is the format defined by the initial version of git, including but not limited to the format of the repository directory, the repository configuration file, and the object and ref storage. Specifying the complete behavior of git is beyond the scope of this document. -Version `1` +=== Version `1` This format is identical to version `0`, with the following exceptions: @@ -74,21 +71,18 @@ it here, in order to claim the name. The defined extensions are: -`noop` -~~ + `noop` This extension does not change git's behavior at all. It is useful only for testing format-1 compatibility. -`preciousObjects` -~ + `preciousObjects` When the config key `extensions.preciousObjects` is set to `true`, objects in the repository MUST NOT be deleted (e.g., by `git-prune` or `git repack -d`). -`partialclone` -~~ + `partialclone` When the config key `extensions.partialclone` is set, it indicates that the repo was created with a partial clone (or later performed @@ -98,3 +92,11 @@ and it promises that all such omitted objects can be fetched from it in the future. The value of this key is the name of the promisor remote. + + `worktreeConfig` + +If set, by default "git config" reads from both "config" and +"config.worktree" file from GIT_DIR in that order. In +multiple working directory mode, "config" file is shared while +"config.worktree" is per-working directory (i.e., it's in +GIT_COMMON_DIR/worktrees//config.worktree) -- 2.19.1.1320.g030759e4a3
Re: [PATCH v5 0/3] range-diff fixes
Hi Ævar,
On Tue, 13 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> Trivial updates since v4 addressing the feedback on that
> iteration. Hopefully this is the last one, range-diff with the last
> version:
This range-diff looks good to me.
Thanks,
Dscho
>
> 1: 5399e57513 = 1: f225173f43 range-diff doc: add a section about output
> stability
> 2: e56975df6c = 2: 77804ac641 range-diff: fix regression in passing along
> diff options
> 3: edfef733c7 ! 3: ed67dba073 range-diff: make diff option behavior (e.g.
> --stat) consistent
> @@ -17,8 +17,8 @@
>
> But we should behave consistently with "diff" in anticipation of such
> output being useful in the future, because it would make for
> confusing
> -UI if two "diff" and "range-diff" behaved differently when it came to
> -how they interpret diff options.
> +UI if "diff" and "range-diff" behaved differently when it came to how
> +they interpret diff options.
>
> The new behavior is also consistent with the existing documentation
> added in ba931edd28 ("range-diff: populate the man page",
> @@ -36,7 +36,7 @@
> memcpy(, diffopt, sizeof(opts));
> -opts.output_format |= DIFF_FORMAT_PATCH;
> +if (!opts.output_format)
> -+opts.output_format |= DIFF_FORMAT_PATCH;
> ++opts.output_format = DIFF_FORMAT_PATCH;
> opts.flags.suppress_diff_headers = 1;
> opts.flags.dual_color_diffed_diffs = dual_color;
> opts.output_prefix = output_prefix_cb;
> @@ -45,6 +45,12 @@
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@
> + '
> +
> + test_expect_success 'changed commit with --stat diff option' '
> +-four_spaces="" &&
> + git range-diff --no-color --stat topic...changed >actual &&
> + cat >expected <<-EOF &&
> 1: 4de457d = 1: a4b s/5/A/
>a => b | 0
>1 file changed, 0 insertions(+), 0 deletions(-)
>
> Ævar Arnfjörð Bjarmason (3):
> range-diff doc: add a section about output stability
> range-diff: fix regression in passing along diff options
> range-diff: make diff option behavior (e.g. --stat) consistent
>
> Documentation/git-range-diff.txt | 17 +
> range-diff.c | 3 ++-
> t/t3206-range-diff.sh| 30 ++
> 3 files changed, 49 insertions(+), 1 deletion(-)
>
> --
> 2.19.1.1182.g4ecb1133ce
>
>
Re: [PATCH v2] checkout: print something when checking out paths
On Wed, Nov 14, 2018 at 11:12 AM Junio C Hamano wrote: > > Nguyễn Thái Ngọc Duy writes: > > > One of the problems with "git checkout" is that it does so many > > different things and could confuse people specially when we fail to > > handle ambiguation correctly. > > You would have realized that this is way too noisy if you ran "make > test", which may have spewed something like this on the screen. Oh I realize it because it's part of my git build and I often use "git co ". I'm just telling (or kidding?) myself that I'm just so used to the old behavior and may need some time to feel comfortable with the new one. > [19:09:19] t4120-apply-popt.sh ok 1624 > ms ( 0.26 usr 0.21 sys + 5.31 cusr 3.51 csys = 9.29 CPU) > [19:09:20] t9164-git-svn-dcommit-concurrent.sh skipped: Perl > SVN libraries not found or unusable > [19:09:20] t1310-config-default.sh ok 177 > ms ( 0.07 usr 0.01 sys + 0.89 cusr 0.66 csys = 1.63 CPU) > ===( 20175;154 1297/? 155/? 6/? 3/3 2/? 4/? 4/? 3/? 5... > )===Checked out 1 path out of the index > Checked out 1 path out of the index > Checked out 1 path out of the index > Checked out 1 path out of the index > Checked out 1 path out of the index > [19:09:20] t1408-packed-refs.sh ... ok 310 > ms ( 0.06 usr 0.00 sys + 0.69 cusr 0.52 csys = 1.27 CPU) > [19:09:20] t0025-crlf-renormalize.sh .. ok 246 > ms ( 0.03 usr 0.01 sys + 0.34 cusr 0.22 csys = 0.60 CPU) > > I am very tempted to suggest to treat this as a training wheel and > enable only when checkout.showpathcount is set to true, or something > like that. Maybe we just drop it then. I'm not adding a training wheel. I'm trying to make this complex command safer somewhat. But maybe this is a wrong direction. I'll give the idea "switch-branch / restore-path alternative commands" a go some time. Then the new generation can just stick to those and old timers stay with "git checkout". -- Duy
[PATCH v2 1/1] bundle: cleanup lock files on error
From: =?UTF-8?q?Ga=C3=ABl=20Lhez?=
When an user tries to create an empty bundle via `git bundle create
` where `` resolves to an empty list (for
example, like `master..master`), the command fails and warns the user
about how it does not want to create empty bundle.
However, the `.lock` file was still open and on Windows that means
that it could not be deleted properly. This patch fixes that issue.
This closes https://github.com/git-for-windows/git/issues/790
Signed-off-by: Gaël Lhez
Signed-off-by: Johannes Schindelin
---
bundle.c| 7 ---
t/t5607-clone-bundle.sh | 4
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/bundle.c b/bundle.c
index 1ef584b93b..4e349feff9 100644
--- a/bundle.c
+++ b/bundle.c
@@ -457,10 +457,11 @@ int create_bundle(struct bundle_header *header, const
char *path,
object_array_remove_duplicates();
ref_count = write_bundle_refs(bundle_fd, );
- if (!ref_count)
- die(_("Refusing to create empty bundle."));
- else if (ref_count < 0)
+ if (ref_count <= 0) {
+ if (!ref_count)
+ error(_("Refusing to create empty bundle."));
goto err;
+ }
/* write pack */
if (write_pack_data(bundle_fd, )) {
diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index 348d9b3bc7..f84b875950 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -71,4 +71,8 @@ test_expect_success 'prerequisites with an empty commit
message' '
git bundle verify bundle
'
+test_expect_success 'try to create a bundle with empty ref count' '
+ test_expect_code 1 git bundle create foobar.bundle master..master
+'
+
test_done
--
gitgitgadget
[PATCH v2 0/1] bundle: fix issue when bundles would be empty
And yet another patch coming through Git for Windows... Change since v1: * Using a better oneline now. Gaël Lhez (1): bundle: cleanup lock files on error bundle.c| 7 --- t/t5607-clone-bundle.sh | 4 2 files changed, 8 insertions(+), 3 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-79%2Fdscho%2Fcreate-empty-bundle-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-79/dscho/create-empty-bundle-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/79 Range-diff vs v1: 1: 6276372ad3 ! 1: c7f05a bundle: refuse to create empty bundle @@ -1,6 +1,6 @@ Author: Gaël Lhez -bundle: refuse to create empty bundle +bundle: cleanup lock files on error When an user tries to create an empty bundle via `git bundle create ` where `` resolves to an empty list (for -- gitgitgadget
Re: [PATCH 1/1] bundle: refuse to create empty bundle
Hi Stefan, On Tue, 13 Nov 2018, Stefan Beller wrote: > On Tue, Nov 13, 2018 at 12:33 PM Gaël Lhez wrote: > > > > Hello, > > > > I don't know why I receive these message (and especially now given the time > > at which I pushed this) but I suppose someone (Johannes Schindelin ?) > > probably pushed back my original commit from git for windows github to GIT > > git repository. > > Yes that is pretty much what is happening. Johannes (GfW maintainer) > tries to push a lot of patches upstream to git and cc's people who > originally authored the patch. > Thanks for taking a look, again, even after this long time! > > > > > If you think "bundle: cleanup lock files on error" is better, then no > > problem with me. I'm not a native english speaker and I simply expressed > > the reason for my problem but - after reading back my commit - neither this > > mail' subject and my original commit subject (see > > https://github.com/git-for-windows/git/pull/797/commits/0ef742a1a92ae53188189238adbd16086fabf80a) > > express the core problem. > > I am not a native speaker either, which makes it extra hard to > understand some commits. ;-) So I propose a wording which would have > helped me. > > > As I'm not accustomed to pushing on GIT 'git repository' , please let me > > know if I have something else to do ? > > I don't know how Johannes handles pushing changes upstream, maybe he > will take on the work of resending a reworded patch. He will. > Let's hear his thoughts on it. I would guess you're more than welcome > to take your patches from GitForWindows into Git itself. As I merged the patch into Git for Windows' `master`, I consider it my responsibility to push this upstream (unless the contributor wants to take matters into their own hands). In the future, my hope is that GitGitGadget will make contributing patches to the Git mailing list a more convenient experience, to the point that it will hopefully be pretty much as easy as iterating a PR in https://github.com/git-for-windows/git. At that point, I will ask contributors to do exactly that in order to shepherd their patches into git.git. Ciao, Dscho > > Cheers, > Stefan >
Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories
Johannes Schindelin writes: > It would make sense, but we don't know how to get that information, do we? > ... > And changing the code *now* to let us query Git where it thinks its > templates should be won't work, as this patch is about using the installed > Git (at whatever pre-compiled version that might be). It won't work, but we can add something like "git var templatedir" to help those who want to further improve the test-installed mode next year, preparing for better future by sowing seeds now. In the meantime, using the same temlate dir as before is probably the best we can do. Two and a half tangential thoughts are: - But then, we need to make sure $GIT_BUILD_DIR/templates/blt/ is populated, if we do rely on them (i.e. we probably want to make sure we have built). - Yet, once installed, the contents of the templatedir can be arbitrarily munged by the end user, so anything that depends on what is in the template won't work as a reliable test piece. - Among what's in templates/blt/, we explicitly disable hooks at the beginning of the test repository creation to ensure no hooks interfere what we test by default, but we will get affected by what is in info/excludes. The contents of freshly-built one is empty, so it is unlikely that this will cause problems, but if we use installed templates, we cannot control what's in there, letting the tests get affected to random things the end-user happens to have. So after all, if we were to change anything, it might make better sense not to install anything from any templatedir. But of course, that is orthogonal to the test-install mode. If we want to make the test more robust by emptying the templates, we should do that also for the test-freshly-baked mode, too.
Re: [PATCH 5/5] tests: explicitly use `git.exe` on Windows
Johannes Schindelin writes: >> The latter half of this change is a good one. Given what the >> proposed log message of this patch says >> >> Note also: the many, many calls to `git this` and `git that` are >> unaffected, as the regular PATH search will find the `.exe` files on >> Windows (and not be confused by a directory of the name `git` that is >> in one of the directories listed in the `PATH` variable), while >> `/path/to/git` would not, per se, know that it is looking for an >> executable and happily prefer such a directory. >> >> which I read as "path-prefixed invocation, i.e. some/path/to/git, is >> bad, it must be spelled some/path/to/git.exe", I am surprised that >> tests ever worked on Windows without these five patches, as this >> line used to read like this: >> >> "$GIT_BUILD_DIR/git" >/dev/null >> if test $? != 1 >> then >> ... >> >> Wouldn't "$GIT_BUILD_DIR/git" have failed (and "executable not >> found" hopefully won't produce exit code 1) and stopped the test >> suite from running even after you built git and not under the >> test-installed-git mode? > > "$GIT_BUILD_DIR/git" did not fail until I regularly built with Visual > Studio (and my Visual Studio project generator generates a directory named > "git" to live alongside "git.exe"). > > And when it failed, I patched Git for Windows. Fast-forward, years later I > managed to contribute the patch we are discussing right now ;-) OK, I still cannot read it out of what you wrote in the proposed log message without aid, but in essense the crux of the problem is that invoking "some/path/to/git" finds "some/path/to/git.exe" only when there is no "some/path/to/git" directory at the same time, and if that directory exists, tries and fails to execute that directory, and the change in this patch protects us from that problem. Did I get it right? I'd really prefer to see it more clearly written in the log message so the next person who reads "git log" do not have to ask the same question to you. Assuming that I read it correctly, I think this patch as a whole makes tons of sense as a change to make the invocation more robust. Thanks.
Re: [PATCH 0/1] rebase: understand -C again, refactor
Hi Peff, On Wed, 14 Nov 2018, Jeff King wrote: > On Tue, Nov 13, 2018 at 04:38:24AM -0800, Johannes Schindelin via > GitGitGadget wrote: > > > Phillip Wood reported a problem where the built-in rebase did not understand > > options like -C1, i.e. it did not expect the option argument. > > > > While investigating how to address this best, I stumbled upon > > OPT_PASSTHRU_ARGV (which I was so far happily unaware of). > > I was unaware of it, too. Thanks, that makes me feel better ;-) > Looking at the OPT_PASSTHRU and its ARGV counterpart, I think the > original intent was that you'd pass through normal last-one-wins > individual options with OPT_PASSTHRU, and then list-like options with > OPT_PASSTHRU_ARGV. But here you've used the latter to pass sets of > individual last-one-wins options. > > That said, I think what you've done here is way simpler and more > readable than using a bunch of OPT_PASSTHRUs would have been. And even > if it was not the original intent of the ARGV variant, I can't see any > reason to avoid doing it this way. Thank you, that makes me feel *even* better ;-) Together with the extra-early error checking for incorrect -C and --whitespace options, I think we're in a much better place now. Ciao, Dscho
Re: [PATCH 1/1] rebase: really just passthru the `git am` options
Hi Phillip,
On Tue, 13 Nov 2018, Phillip Wood wrote:
> On 13/11/2018 19:21, Johannes Schindelin wrote:
> > Hi Phillip,
> >
> > On Tue, 13 Nov 2018, Phillip Wood wrote:
> >
> > > Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to
> > > break the error reporting
> > >
> > > Running
> > >bin/wrappers/git rebase --onto @ @^^ -Cbad
> > >
> > > Gives
> > >git encountered an error while preparing the patches to replay
> > >these revisions:
> > >
> > >
> > > 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c
> > >
> > >As a result, git cannot rebase them.
> > >
>
> git 2.19.1 gives
>
> First, rewinding head to replay your work on top of it...
> Applying: Ninth batch for 2.20
> error: switch `C' expects a numerical value
>
> So it has a clear message as to what the error is, this patch regresses that.
> It would be better if rebase detected the error before starting though.
I agree that that would make most sense: why start something when you can
know that it will fail later...
Let me try to add that test case that Junio wanted, and some early error
handling.
> > > If I do
> > >
> > >bin/wrappers/git rebase @^^ -Cbad
> > >
> > > I get no error, it just tells me that it does not need to rebase (which is
> > > true)
> >
> > Hmm. Isn't this the same behavior as with the scripted version?
>
> Ah you're right the script does not check if the option argument is valid.
>
> > Also: are
> > we sure that we want to allow options to come *after* the ``
> > argument?
>
> Maybe not but the scripted version does. I'm not sure if that is a good idea
> or not.
That behavior was never documented, though, was it?
Ciao,
Dscho
>
> Best Wishes
>
> Phillip
>
> > Ciao,
> > Dscho
> >
> > > Best Wishes
> > >
> > > Phillip
> > >
> > >
> > > On 13/11/2018 12:38, Johannes Schindelin via GitGitGadget wrote:
> > > > From: Johannes Schindelin
> > > >
> > > > Currently, we parse the options intended for `git am` as if we wanted to
> > > > handle them in `git rebase`, and then reconstruct them painstakingly to
> > > > define the `git_am_opt` variable.
> > > >
> > > > However, there is a much better way (that I was unaware of, at the time
> > > > when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.
> > > > It is intended for exactly this use case, where command-line options
> > > > want to be parsed into a separate `argv_array`.
> > > >
> > > > Let's use this feature.
> > > >
> > > > Incidentally, this also allows us to address a bug discovered by Phillip
> > > > Wood, where the built-in rebase failed to understand that the `-C`
> > > > option takes an optional argument.
> > > >
> > > > Signed-off-by: Johannes Schindelin
> > > > ---
> > > >builtin/rebase.c | 98
> > > >+---
> > > >1 file changed, 35 insertions(+), 63 deletions(-)
> > > >
> > > > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > > > index 0ee06aa363..96ffa80b71 100644
> > > > --- a/builtin/rebase.c
> > > > +++ b/builtin/rebase.c
> > > > @@ -87,7 +87,7 @@ struct rebase_options {
> > > > REBASE_FORCE = 1<<3,
> > > > REBASE_INTERACTIVE_EXPLICIT = 1<<4,
> > > > } flags;
> > > > - struct strbuf git_am_opt;
> > > > + struct argv_array git_am_opts;
> > > > const char *action;
> > > > int signoff;
> > > > int allow_rerere_autoupdate;
> > > > @@ -339,7 +339,7 @@ N_("Resolve all conflicts manually, mark them as
> > > > resolved with\n"
> > > >static int run_specific_rebase(struct rebase_options *opts)
> > > >{
> > > > const char *argv[] = { NULL, NULL };
> > > > - struct strbuf script_snippet = STRBUF_INIT;
> > > > + struct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;
> > > > int status;
> > > > const char *backend, *backend_func;
> > > >@@ -433,7 +433,9 @@ static int run_specific_rebase(struct
> > > > rebase_options
> > > > *opts)
> > > > oid_to_hex(>restrict_revision->object.oid) : NULL);
> > > > add_var(_snippet, "GIT_QUIET",
> > > > opts->flags & REBASE_NO_QUIET ? "" : "t");
> > > > - add_var(_snippet, "git_am_opt", opts->git_am_opt.buf);
> > > > + sq_quote_argv_pretty(, opts->git_am_opts.argv);
> > > > + add_var(_snippet, "git_am_opt", buf.buf);
> > > > + strbuf_release();
> > > > add_var(_snippet, "verbose",
> > > > opts->flags & REBASE_VERBOSE ? "t" : "");
> > > > add_var(_snippet, "diffstat",
> > > > @@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const
> > > > char
> > > > *prefix)
> > > > struct rebase_options options = {
> > > > .type = REBASE_UNSPECIFIED,
> > > > .flags = REBASE_NO_QUIET,
> > > > - .git_am_opt = STRBUF_INIT,
> > > > + .git_am_opts = ARGV_ARRAY_INIT,
> > > > .allow_rerere_autoupdate = -1,
> > > > .allow_empty_message = 1,
> > > > .git_format_patch_opt =
Re: [PATCH 0/2] rebase.useBuiltin doc & test mode
Hi Ævar, On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > On Wed, Nov 14 2018, Stefan Beller wrote: > > >> But maybe I'm being overly paranoid. What do those more familiar with > >> this think? > > > > I am not too worried, > > * as rebase is a main porcelain, that is even hard to use in a script. > > so any failures are not deep down in some automation, > > but when found exposed quickly (and hopefully reported). > > * 5541bd5b8f was merged to next a month ago; internally we > >distribute the next branch to Googlers (on a weekly basis) > >and we have not had any bug reports regarding rebase. > >(Maybe our environment is too strict for the wide range > > of bugs reported) > > I do the same at Booking.com (although at a more ad-hoc schedule) and > got the report whose fix is now sitting in "pu" noted upthread. > > I fear that these sorts of corporate environments, both Google's and > Booking's, end up testing a relatively narrow featureset. Most people > have similar enough workflows, e.g. just using "git pull --rebase", > I'd be surprised if we have more than 2-3 internal users who ever use > the --onto option for example. > > > * Johannes reported that the rebase is used in GfW > > > > https://public-inbox.org/git/nycvar.qro.7.76.6.1808241320540...@tvgsbejvaqbjf.bet/ > >https://github.com/git-for-windows/git/pull/1800 > >and from my cursory reading it is part of > >2.19.windows, which has a large user base. > > > >> (and would re-enable rebase.useBuiltin=true in > >> master right after 2.20 is out the door). > > > > That would be fine with me as well, but I'd rather > > document rebase.useBuiltin instead of flip-flopping > > the switch around the release. > > > > Have there been any fixes that are only in > > the C version (has the shell version already bitrotted)? > > That's a good question, one which I don't think we knew the answer to > before the following patches. I pay close attention to `git rebase` bug reports and patches (obviously), and there have not been any changes going into the built-in rebase/rebase -i that necessitated changes also in the scripted version. Ciao, Dscho > As it turns out no, we still run the tests without failures with > GIT_TEST_REBASE_USE_BUILTIN=false. > > Ævar Arnfjörð Bjarmason (2): > rebase doc: document rebase.useBuiltin > tests: add a special setup where rebase.useBuiltin is off > > Documentation/config/rebase.txt | 14 ++ > builtin/rebase.c| 5 - > t/README| 3 +++ > 3 files changed, 21 insertions(+), 1 deletion(-) > > -- > 2.19.1.1182.g4ecb1133ce > >
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Hi, On Wed, 14 Nov 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > Agreed. I'm happy to see the test for-loop gone as I noted in > > https://public-inbox.org/git/87d0rm7zeo@evledraar.gmail.com/ but as > > noted in that v3 feedback the whole "why would anyone want this?" > > explanation is still missing, and this still smells like a workaround > > for a bug we should be fixing elsewhere in the sequencing code. > > Thanks. I share the same impression that this is sweeping a bug > under a wrong rug. I agree that the scenario is under-explained. Of course, I have to say that this is not Tanushree's problem; They only copied what is in https://github.com/git-for-windows/git/issues/1854 and @chucklu did not grace us with an explanation, either. Based on historical context, I would wager a bet that the scenario is that some commits that may or may not have been in a different SCM originally and that may or may not have been empty and/or squashed in `master` need to be cherry-picked. But I agree that this should be clarified. I prodded the original wish-haver. Ciao, Dscho
[PATCH v2 0/1] Some left-over add-on for bw/config-h
Back when bw/config-h was developed (and backported to Git for Windows), I
came up with a patch to use git_dir if commondir is NULL, and contributed
that as v1 of this patch. However, it was deemed a bug if that happens, so
let's instead detect that condition and report it.
Change since v1:
* Be loud about this bug instead of papering over it.
Johannes Schindelin (1):
config: report a bug if git_dir exists without commondir
config.c | 2 ++
1 file changed, 2 insertions(+)
base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-78%2Fdscho%2Fbw%2Fconfig-h-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-78/dscho/bw/config-h-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/78
Range-diff vs v1:
1: a3854e4ed8 ! 1: 0767f98378 do_git_config_sequence(): fall back to git_dir
if commondir is NULL
@@ -1,8 +1,9 @@
Author: Johannes Schindelin
-do_git_config_sequence(): fall back to git_dir if commondir is NULL
+config: report a bug if git_dir exists without commondir
-Just some defensive programming.
+This did happen at some stage, and was fixed relatively quickly. Make
+sure that we detect very quickly, too, should that happen again.
Signed-off-by: Johannes Schindelin
@@ -14,7 +15,7 @@
if (opts->commondir)
repo_config = mkpathdup("%s/config", opts->commondir);
+ else if (opts->git_dir)
-+ repo_config = mkpathdup("%s/config", opts->git_dir);
++ BUG("git_dir without commondir");
else
repo_config = NULL;
--
gitgitgadget
[PATCH v2 1/1] config: report a bug if git_dir exists without commondir
From: Johannes Schindelin
This did happen at some stage, and was fixed relatively quickly. Make
sure that we detect very quickly, too, should that happen again.
Signed-off-by: Johannes Schindelin
---
config.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/config.c b/config.c
index 4051e38823..db6b0167c6 100644
--- a/config.c
+++ b/config.c
@@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct
config_options *opts,
if (opts->commondir)
repo_config = mkpathdup("%s/config", opts->commondir);
+ else if (opts->git_dir)
+ BUG("git_dir without commondir");
else
repo_config = NULL;
--
gitgitgadget
Re: [PATCH v2 2/2] tests: add a special setup where rebase.useBuiltin is off
On Wed, Nov 14 2018, Johannes Schindelin wrote: > Hi Ævar, > > On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > >> Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent >> to running with rebase.useBuiltin=false. This is needed to spot that >> we're not introducing any regressions in the legacy rebase version >> while we're carrying both it and the new builtin version. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason >> --- >> builtin/rebase.c | 5 - >> t/README | 4 >> 2 files changed, 8 insertions(+), 1 deletion(-) > > I am slightly surprised not to see any ci/ change in this diffstat. Did > you mean to add a test axis for Travis, or not? No, but that's a logical follow-up by someone more familiar with the CI setup. I'm using this so I can test versions of "next" when building my own package.
Re: [PATCH v2 2/2] tests: add a special setup where rebase.useBuiltin is off
Hi Ævar,
On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent
> to running with rebase.useBuiltin=false. This is needed to spot that
> we're not introducing any regressions in the legacy rebase version
> while we're carrying both it and the new builtin version.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> builtin/rebase.c | 5 -
> t/README | 4
> 2 files changed, 8 insertions(+), 1 deletion(-)
I am slightly surprised not to see any ci/ change in this diffstat. Did
you mean to add a test axis for Travis, or not?
Ciao,
Dscho
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0ee06aa363..68ad8c1149 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -48,7 +48,10 @@ static int use_builtin_rebase(void)
> {
> struct child_process cp = CHILD_PROCESS_INIT;
> struct strbuf out = STRBUF_INIT;
> - int ret;
> + int ret, env = git_env_bool("GIT_TEST_REBASE_USE_BUILTIN", -1);
> +
> + if (env != -1)
> + return env;
>
> argv_array_pushl(,
>"config", "--bool", "rebase.usebuiltin", NULL);
> diff --git a/t/README b/t/README
> index 242497455f..3df5d12e46 100644
> --- a/t/README
> +++ b/t/README
> @@ -339,6 +339,10 @@ for the index version specified. Can be set to any
> valid version
> GIT_TEST_PRELOAD_INDEX= exercises the preload-index code path
> by overriding the minimum number of cache entries required per thread.
>
> +GIT_TEST_REBASE_USE_BUILTIN=, when false, disables the
> +builtin version of git-rebase. See 'rebase.useBuiltin' in
> +git-config(1).
> +
> GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading
> of the index for the whole test suite by bypassing the default number of
> cache entries and thread minimums. Setting this to 1 will make the
> --
> 2.19.1.1182.g4ecb1133ce
>
>
Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin
Hi Ævar,
On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> The rebase.useBuiltin variable introduced in 55071ea248 ("rebase:
> start implementing it as a builtin", 2018-08-07) was turned on by
> default in 5541bd5b8f ("rebase: default to using the builtin rebase",
> 2018-08-08), but had no documentation.
>
> Let's document it so that users who run into any stability issues with
> the C rewrite know there's an escape hatch[1], and make it clear that
> needing to turn off builtin rebase means you've found a bug in git.
>
> 1. https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
Makes sense.
Thanks,
Dscho
> ---
> Documentation/config/rebase.txt | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index 42e1ba7575..f079bf6b7e 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -1,3 +1,17 @@
> +rebase.useBuiltin::
> + Set to `false` to use the legacy shellscript implementation of
> + linkgit:git-rebase[1]. Is `true` by default, which means use
> + the built-in rewrite of it in C.
> ++
> +The C rewrite is first included with Git version 2.20. This option
> +serves an an escape hatch to re-enable the legacy version in case any
> +bugs are found in the rewrite. This option and the shellscript version
> +of linkgit:git-rebase[1] will be removed in some future release.
> ++
> +If you find some reason to set this option to `false` other than
> +one-off testing you should report the behavior difference as a bug in
> +git.
> +
> rebase.stat::
> Whether to show a diffstat of what changed upstream since the last
> rebase. False by default.
> --
> 2.19.1.1182.g4ecb1133ce
>
>
Re: [PATCH 4/5] tests: do not require Git to be built when testing an installed Git
Hi Peff,
On Wed, 14 Nov 2018, Jeff King wrote:
> On Mon, Nov 12, 2018 at 05:48:37AM -0800, Johannes Schindelin via
> GitGitGadget wrote:
>
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 832ede5099..1ea20dc2dc 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -51,7 +51,7 @@ export LSAN_OPTIONS
> >
> >
> > # It appears that people try to run tests without building...
> > -"$GIT_BUILD_DIR/git" >/dev/null
> > +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
> > if test $? != 1
> > then
> > echo >&2 'error: you do not seem to have built git yet.'
>
> The original runs the command independently and then checks $?. Your
> replacement chains the ||. I think it works, because the only case that
> is different is if running git returns 0 (in which case we currently
> complain, but the new code would quietly accept this).
>
> That should never happen, but if it does we'd probably want to complain.
> And it's pretty subtle all around. Maybe this would be a bit clearer:
>
> if test -n "$GIT_TEST_INSTALLED"
> then
> : assume installed git is OK
> else
> "$GIT_BUILD_DIR/git" >/dev/null
> if test $? != 1
> then
> ... die ...
> fi
> fi
>
> Though arguably we should be checking that there is a git in our path in
> the first instance. So maybe:
>
> if test -n "$GIT_TEST_INSTALLED"
> "$GIT_TEST_INSTALLED/git" >/dev/null
> else
> "$GIT_BUILD_DIR/git" >/dev/null
> fi
> if test $? != 1 ...
You're right. I did it using `"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git"`
and I also adjust the error message now.
Will be fixed in the next iteration,
Dscho
>
> -Peff
>
Re: [PATCH 5/5] tests: explicitly use `git.exe` on Windows
Hi Junio,
On Wed, 14 Nov 2018, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget"
> writes:
>
> > diff --git a/Makefile b/Makefile
> > index bbfbb4292d..5df0118ce9 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2590,6 +2590,7 @@ GIT-BUILD-OPTIONS: FORCE
> > @echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst
> > ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
> > @echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
> > @echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
> > + @echo X=\'$(X)\' >>$@+
>
> Made me wonder if a single letter $(X) is a bit too cute to expose
> to the outside world; as a narrowly confined local convention in
> this single Makefile, it was perfectly fine. But it should do for
> now. Its terseness is attractive, and our eyes (I do not speak for
> those new to our codebase and build structure) are already used to
> seeing $X suffix. Somebody may later come and complain but I am OK
> to rename it to something like $EXE at that time, not now.
>
> > ifdef TEST_OUTPUT_DIRECTORY
> > @echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst
> > ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
> > endif
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index 801cc9b2ef..c167b2e1af 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -900,7 +900,7 @@ test_create_repo () {
> > mkdir -p "$repo"
> > (
> > cd "$repo" || error "Cannot setup test environment"
> > - "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
> > + "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" init \
>
> Good.
>
> > "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
> > error "cannot run git init -- have you built things yet?"
> > mv .git/hooks .git/hooks-disabled
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 1ea20dc2dc..3e2a9ce76d 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -49,18 +49,23 @@ export ASAN_OPTIONS
> > : ${LSAN_OPTIONS=abort_on_error=1}
> > export LSAN_OPTIONS
> >
> > +if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> > +then
> > + echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
> > + exit 1
> > +fi
>
> OK, this tells us that we at least attempted to build git once, even
> under test-installed mode, and hopefully people won't run $(MAKE) and
> immediately ^C it only to fool us by leaving this file while keeping
> git binary and t/helpers/ binary unbuilt.
>
> > +. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> > +export PERL_PATH SHELL_PATH
> > +
> >
> > # It appears that people try to run tests without building...
> > -test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
> > +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git$X" >/dev/null ||
>
> The latter half of this change is a good one. Given what the
> proposed log message of this patch says
>
> Note also: the many, many calls to `git this` and `git that` are
> unaffected, as the regular PATH search will find the `.exe` files on
> Windows (and not be confused by a directory of the name `git` that is
> in one of the directories listed in the `PATH` variable), while
> `/path/to/git` would not, per se, know that it is looking for an
> executable and happily prefer such a directory.
>
> which I read as "path-prefixed invocation, i.e. some/path/to/git, is
> bad, it must be spelled some/path/to/git.exe", I am surprised that
> tests ever worked on Windows without these five patches, as this
> line used to read like this:
>
> "$GIT_BUILD_DIR/git" >/dev/null
> if test $? != 1
> then
> ...
>
> Wouldn't "$GIT_BUILD_DIR/git" have failed (and "executable not
> found" hopefully won't produce exit code 1) and stopped the test
> suite from running even after you built git and not under the
> test-installed-git mode?
"$GIT_BUILD_DIR/git" did not fail until I regularly built with Visual
Studio (and my Visual Studio project generator generates a directory named
"git" to live alongside "git.exe").
And when it failed, I patched Git for Windows. Fast-forward, years later I
managed to contribute the patch we are discussing right now ;-)
So yes, it is primarily a concern when testing Git in specific setups
where a "git" directory can live next to the "git.exe" that we want to
test. Not necessarily a big deal for most developers on Windows.
Ciao,
Dscho
>
> > if test $? != 1
> > then
> > echo >&2 'error: you do not seem to have built git yet.'
> > exit 1
> > fi
> >
> > -. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> > -export PERL_PATH SHELL_PATH
> > -
> > # if --tee was passed, write the output not only to the terminal, but
> > # additionally to the file test-results/$BASENAME.out, too.
> > case "$GIT_TEST_TEE_STARTED, $* " in
>
Re: [PATCH 4/5] tests: do not require Git to be built when testing an installed Git
Hi Junio, On Wed, 14 Nov 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > From: Johannes Schindelin > > > > We really only need the test helpers in that case, but that is not what > > we test for. So let's skip the test for now when we know that we want to > > test an installed Git. > > True, but... hopefully we are making sure t/helpers/ has been built > in some other ways, though, right? We do it implicitly, in the test cases that use the helpers. However, t/test-lib.sh does not particularly verify that the test helpers have been built. And I think that is a good thing: we do have several test scripts, I would think, that do not require any test helper to begin with. These scripts can work pretty well without having to build anything (read: on a machine where there is not even a toolchain available to build things). Besides, it is pretty much only t/helper/test-tool these days, and it is unlikely that anybody has a `test-tool` in their `PATH`. If they do, they'll find out soon enough iff they test with `GIT_TEST_INSTALLED` and iff they do not have the test helper(s) in t/helper/ ;-) Ciao, Dscho > I do not offhand see how tests would work in a pristine checkout with > "cd t && sh t-*.sh" as t/test-lib.sh is not running $(MAKE) itself > (and the design of the test-lib.sh, as can be seen in the check this > part of it makes, is to just abort if we cannot test) with this patch > (and PATCH 1/5) under the test-installed mode, though. > > > Signed-off-by: Johannes Schindelin > > --- > > t/test-lib.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/t/test-lib.sh b/t/test-lib.sh > > index 832ede5099..1ea20dc2dc 100644 > > --- a/t/test-lib.sh > > +++ b/t/test-lib.sh > > @@ -51,7 +51,7 @@ export LSAN_OPTIONS > > > > > > # It appears that people try to run tests without building... > > -"$GIT_BUILD_DIR/git" >/dev/null > > +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null || > > if test $? != 1 > > then > > echo >&2 'error: you do not seem to have built git yet.' >
Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories
Hi Junio,
On Wed, 14 Nov 2018, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget"
> writes:
>
> > From: Johannes Schindelin
> >
> > It really makes very, very little sense to use a different git
> > executable than the one the caller indicated via setting the environment
> > variable GIT_TEST_INSTALLED.
>
> Makes perfect sense. Shouldn't we be asking where the template
> directory of the installed version is and using it instead of the
> freshly built one, no?
It would make sense, but we don't know how to get that information, do we?
$ git grep DEFAULT_GIT_TEMPLATE_DIR
Makefile: -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR
builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR
"/usr/share/git-core/templates"
builtin/init-db.c: template_dir = to_free =
system_path(DEFAULT_GIT_TEMPLATE_DIR);
contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \
In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the
only user in the code is init-db.c which uses it in copy_templates().
And changing the code *now* to let us query Git where it thinks its
templates should be won't work, as this patch is about using the installed
Git (at whatever pre-compiled version that might be).
Ciao,
Dscho
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > t/test-lib-functions.sh | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index 78d8c3783b..801cc9b2ef 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -900,7 +900,8 @@ test_create_repo () {
> > mkdir -p "$repo"
> > (
> > cd "$repo" || error "Cannot setup test environment"
> > - "$GIT_EXEC_PATH/git-init"
> > "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
> > + "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
> > + "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
> > error "cannot run git init -- have you built things yet?"
> > mv .git/hooks .git/hooks-disabled
> > ) || exit
>
Re: [ANNOUNCE] Git Merge Contributor's Summit Jan 31, 2019, Brussels
On Wed, Nov 14, 2018 at 12:31:22PM +, Luke Diamand wrote: > On Fri, 9 Nov 2018 at 10:48, Jeff King wrote: > > > > On Fri, Nov 09, 2018 at 10:44:10AM +, Luca Milanesio wrote: > > > > > > On 9 Nov 2018, at 10:42, Jeff King wrote: > > > > > > > > Git Merge 2019 is happening on February 1st. There will be a > > > > Contributor's Summit the day before. Here are the details: > > > > > > > > When: Thursday, January 31, 2019. 10am-5pm. > > > > Where: The Egg[1], Brussels, Belgium > > > > What: Round-table discussion about Git > > > > Who: All contributors to Git or related projects in the Git ecosystem > > > > are invited; if you're not sure if you qualify, please ask! > > > > > > Hi Jeff, > > > is Gerrit included in the "Git ecosystem"? > > > > Yeah, I think so. At least the Git parts of it. :) > > git-p4? > > (The git parts obviously!) Yes! -Peff
Re: [PATCH 4/5] tests: do not require Git to be built when testing an installed Git
On Mon, Nov 12, 2018 at 05:48:37AM -0800, Johannes Schindelin via GitGitGadget wrote: > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 832ede5099..1ea20dc2dc 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -51,7 +51,7 @@ export LSAN_OPTIONS > > > # It appears that people try to run tests without building... > -"$GIT_BUILD_DIR/git" >/dev/null > +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null || > if test $? != 1 > then > echo >&2 'error: you do not seem to have built git yet.' The original runs the command independently and then checks $?. Your replacement chains the ||. I think it works, because the only case that is different is if running git returns 0 (in which case we currently complain, but the new code would quietly accept this). That should never happen, but if it does we'd probably want to complain. And it's pretty subtle all around. Maybe this would be a bit clearer: if test -n "$GIT_TEST_INSTALLED" then : assume installed git is OK else "$GIT_BUILD_DIR/git" >/dev/null if test $? != 1 then ... die ... fi fi Though arguably we should be checking that there is a git in our path in the first instance. So maybe: if test -n "$GIT_TEST_INSTALLED" "$GIT_TEST_INSTALLED/git" >/dev/null else "$GIT_BUILD_DIR/git" >/dev/null fi if test $? != 1 ... -Peff
Re: [PATCH v3] index-pack: add ability to disable SHA-1 collision check
On Wed, Nov 14 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
>> Add a new core.checkCollisions setting. On by default, it can be set
>> to 'false' to disable the check for existing objects in sha1_object().
>> ...
>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
>> index 2004e25da2..4a3508aa9f 100644
>> --- a/builtin/index-pack.c
>> +++ b/builtin/index-pack.c
>> @@ -791,23 +791,24 @@ static void sha1_object(const void *data, struct
>> object_entry *obj_entry,
>> {
>> void *new_data = NULL;
>> int collision_test_needed = 0;
>> +int do_coll_check = git_config_get_collision_check();
>>
>> assert(data || obj_entry);
>>
>> -if (startup_info->have_repository) {
>> +if (do_coll_check && startup_info->have_repository) {
>> read_lock();
>> collision_test_needed =
>> has_sha1_file_with_flags(oid->hash, OBJECT_INFO_QUICK);
>> read_unlock();
>> }
>>
>> -if (collision_test_needed && !data) {
>> +if (do_coll_check && collision_test_needed && !data) {
>> read_lock();
>> if (!check_collison(obj_entry))
>> collision_test_needed = 0;
>> read_unlock();
>> }
>> -if (collision_test_needed) {
>> +if (do_coll_check && collision_test_needed) {
>
> If I am reading the patch correctly, The latter two changes are
> totally unnecessary. c-t-needed is true only when dO-coll_check
> allowed the initial "do we even have that object?" check to kick in
> and never set otherwise.
You're right. I was trying to do this in a few different ways and didn't
simplify this part.
> I am not all that enthused to the idea of sending a wrong message to
> our users, i.e. it is sometimes OK to sacrifice the security of
> collision detection.
I think I've made the case in side-threads that given the performance
numbers and the danger of an actual SHA-1 collision this is something
other powerusers would be interested in having.
> A small change like this is easy to adjust to apply to the codebase,
> even after today's codebase undergoes extensive modifications; quite
> honestly, I'd prefer not having to worry about it so close to the
> pre-release feature freeze.
Yeah, let's definitely wait with this under 2.20. I sent this out more
because I re-rolled it for an internal deployment, and wanted to get
some thoughts on what the plan should be for queuing up these two
related (no collision detection && loose cache) features.
Re: [ANNOUNCE] Git Merge Contributor's Summit Jan 31, 2019, Brussels
On Fri, 9 Nov 2018 at 10:48, Jeff King wrote: > > On Fri, Nov 09, 2018 at 10:44:10AM +, Luca Milanesio wrote: > > > > On 9 Nov 2018, at 10:42, Jeff King wrote: > > > > > > Git Merge 2019 is happening on February 1st. There will be a > > > Contributor's Summit the day before. Here are the details: > > > > > > When: Thursday, January 31, 2019. 10am-5pm. > > > Where: The Egg[1], Brussels, Belgium > > > What: Round-table discussion about Git > > > Who: All contributors to Git or related projects in the Git ecosystem > > > are invited; if you're not sure if you qualify, please ask! > > > > Hi Jeff, > > is Gerrit included in the "Git ecosystem"? > > Yeah, I think so. At least the Git parts of it. :) git-p4? (The git parts obviously!) > > -Peff
[PATCH] ref-filter: don't look for objects when outside of a repository
The command 'git ls-remote --sort=authordate ' segfaults when
run outside of a repository, ever since the introduction of its
'--sort' option in 1fb20dfd8e (ls-remote: create '--sort' option,
2018-04-09).
While in general the 'git ls-remote' command can be run outside of a
repository just fine, its '--sort=' option with certain keys does
require access to the referenced objects. This sorting is implemented
using the generic ref-filter sorting facility, which already handles
missing objects gracefully with the appropriate 'missing object
deadbeef for HEAD' message. However, being generic means that it
checks replace refs while trying to retrieve an object, and while
doing so it accesses the 'git_replace_ref_base' variable, which has
not been initialized and is still a NULL pointer when outside of a
repository, thus causing the segfault.
Make ref-filter more careful upfront while parsing the format string,
and make it error out when encountering a format atom requiring object
access when we are not in a repository. Also add a test to ensure
that 'git ls-remote --sort' fails gracefully when executed outside of
a repository.
Reported-by: H.Merijn Brand
Signed-off-by: SZEDER Gábor
---
On Tue, Sep 25, 2018 at 01:57:38PM -0700, Junio C Hamano wrote:
> SZEDER Gábor writes:
>
> > However, if we go for a more informative error message, then wouldn't
> > it be better to add this condition in populate_value() before it even
> > calls get_object()? Then we could also add the problematic format
> > specifier to the error message (I think, but didn't actually check),
> > just in case someone specified multiple sort keys.
>
> Even though I suspect that verify_ref_format() is the logically the
> right place to do this (after all, it is about seeing if the format
> makes sense, and a format that requires an object access used
> outside a repository should trigger an verification error), doing
> that in populate_value() probably strikes the best balance, I would
> think.
We are dealing with format specifiers used for sorting here, and those
don't go through verify_ref_format().
So how about this patch instead?
I think it will catch all cases where a user would try to use a format
specifier, for any purpose, requiring object access outside of a
repository (though I don't know whether there are any other cases
besides 'git ls-remote --sort=...'; but perhaps in the future
'ls-remote' will get a '--format' option as well), and it does so
before performing a potentially expensive query to the remote. OTOH,
it won't change the documented "missing object" error message when run
inside a repo but the necessary object is indeed missing.
ref-filter.c | 4
t/t5512-ls-remote.sh | 6 ++
2 files changed, 10 insertions(+)
diff --git a/ref-filter.c b/ref-filter.c
index 0c45ed9d94..a1290659af 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -534,6 +534,10 @@ static int parse_ref_filter_atom(const struct ref_format
*format,
if (ARRAY_SIZE(valid_atom) <= i)
return strbuf_addf_ret(err, -1, _("unknown field name: %.*s"),
(int)(ep-atom), atom);
+ if (valid_atom[i].source != SOURCE_NONE && !have_git_dir())
+ return strbuf_addf_ret(err, -1,
+ _("not a git repository, but the field
'%.*s' requires access to object data"),
+ (int)(ep-atom), atom);
/* Add it in, including the deref prefix */
at = used_atom_cnt;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 91ee6841c1..32e722db2e 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -302,6 +302,12 @@ test_expect_success 'ls-remote works outside repository' '
nongit git ls-remote dst.git
'
+test_expect_success 'ls-remote --sort fails gracefully outside repository' '
+ # Use a sort key that requires access to the referenced objects.
+ nongit test_must_fail git ls-remote --sort=authordate
"$TRASH_DIRECTORY" 2>err &&
+ test_i18ngrep "^fatal: not a git repository, but the field
'\''authordate'\'' requires access to object data" err
+'
+
test_expect_success 'ls-remote patterns work with all protocol versions' '
git for-each-ref --format="%(objectname)%(refname)" \
refs/heads/master refs/remotes/origin/master >expect &&
--
2.19.1.1182.gbfcc7ed3e6
Re: [PATCH v3 1/1] protocol: advertise multiple supported versions
On Wed, Nov 14, 2018 at 11:44:51AM +0900, Junio C Hamano wrote: > SZEDER Gábor writes: > > >> + if (tmp_allowed_versions[0] != config_version) > >> + for (int i = 1; i < nr_allowed_versions; i++) > > > > We don't do C99 yet, thus the declaration of a loop variable like this > > is not allowed and triggers compiler errors. > > I thought we did a weather-balloon to see if this bothers people who > build on minority platforms but > > git grep 'for (int' > > is coming up empty. > > We have been trying designated initializers with weather-balloon > changes (both arrays and struct fields) and I somehow thought that > we already were trying this out, but apparently that is not the > case. I thought so as well, and I run the exact same 'git grep' command before replying to Josh :) There was a discussion about such a weather-balloon patch [1] (which happened to use an unsigned loop variable, so our grep wouldn't have found it anyway), but it wasn't picked up because it required new options in CFLAGS and there was no standard way to do so [2]. [1] https://public-inbox.org/git/20170719181956.15845-1-sbel...@google.com/ [2] https://public-inbox.org/git/20170724170813.scceigybl5d3f...@sigill.intra.peff.net/
[PATCH 1/3] clone: use a more appropriate variable name for the default refspec
cmd_clone() declares two strbufs 'key' and 'value' on the same line, suggesting that they are used to contruct a config variable's name and value. However, this is not the case: 'key' is used to construct the names of multiple config variables, while 'value' is never used as a value for any of those config variables, or for any other config variable for that matter, but only to contruct the default fetch refspec. Let's rename 'value' to 'default_refspec' to make the intent clearer. Signed-off-by: SZEDER Gábor --- builtin/clone.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 15b142d646..ae1bf242c6 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -890,7 +890,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix) const struct ref *our_head_points_at; struct ref *mapped_refs; const struct ref *ref; - struct strbuf key = STRBUF_INIT, value = STRBUF_INIT; + struct strbuf key = STRBUF_INIT; + struct strbuf default_refspec = STRBUF_INIT; struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT; struct transport *transport = NULL; const char *src_ref_prefix = "refs/heads/"; @@ -1067,7 +1068,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix) strbuf_addf(_top, "refs/remotes/%s/", option_origin); } - strbuf_addf(, "+%s*:%s*", src_ref_prefix, branch_top.buf); strbuf_addf(, "remote.%s.url", option_origin); git_config_set(key.buf, repo); strbuf_reset(); @@ -1081,9 +1081,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (option_required_reference.nr || option_optional_reference.nr) setup_reference(); - refspec_append(, value.buf); - - strbuf_reset(); + strbuf_addf(_refspec, "+%s*:%s*", src_ref_prefix, + branch_top.buf); + refspec_append(, default_refspec.buf); remote = remote_get(option_origin); transport = transport_get(remote, remote->url[0]); @@ -1240,7 +1240,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) strbuf_release(_msg); strbuf_release(_top); strbuf_release(); - strbuf_release(); + strbuf_release(_refspec); junk_mode = JUNK_LEAVE_ALL; refspec_clear(); -- 2.19.1.1182.gbfcc7ed3e6
[PATCH 0/3] clone: respect configured fetch respecs during initial fetch
The initial fetch during a clone doesn't transfer refs matching
additional fetch refspecs given on the command line as configuration
variables, e.g. '-c remote.origin.fetch='. This contradicts
the documentation stating that configuration variables specified via
'git clone -c = ...' "take effect immediately after the
repository is initialized, but before the remote history is fetched"
and the given example specifically mentions "adding additional fetch
refspecs to the origin remote".
The second patch in this series makes it work. The other two are
while-at-its: the first is a little cleanup, and the last one
documents other known ignored configuration variables (but whose
functionality is available through command line options).
This patch series should have been marked as v6, but I chose to reset
the counter, because:
- v5 was sent out way over a year ago [1], and surely everybody has
forgotten about it since then anyway. But more importantly:
- A lot has happened since then, most notably we now have a refspec
API, which makes this patch series much simpler (now it only
touches 'builtin/clone.c', the previous version had to add stuff
to 'remote.{c,h}' as well).
[1] For reference, though I actually doubt it's worth looking up:
https://public-inbox.org/git/20170616173849.8071-1-szeder@gmail.com/T/#u
SZEDER Gábor (3):
clone: use a more appropriate variable name for the default refspec
clone: respect additional configured fetch refspecs during initial
fetch
Documentation/clone: document ignored configuration variables
Documentation/git-clone.txt | 6 +
builtin/clone.c | 33 +++---
t/t5611-clone-config.sh | 47 +
3 files changed, 72 insertions(+), 14 deletions(-)
--
2.19.1.1182.gbfcc7ed3e6
[PATCH 2/3] clone: respect additional configured fetch refspecs during initial fetch
The initial fetch during a clone doesn't transfer refs matching
additional fetch refspecs given on the command line as configuration
variables, e.g. '-c remote.origin.fetch='. This contradicts
the documentation stating that configuration variables specified via
'git clone -c = ...' "take effect immediately after the
repository is initialized, but before the remote history is fetched"
and the given example specifically mentions "adding additional fetch
refspecs to the origin remote". Furthermore, one-shot configuration
variables specified via 'git -c = clone ...', though not
written to the newly created repository's config file, live during the
lifetime of the 'clone' command, including the initial fetch. All
this implies that any fetch refspecs specified this way should already
be taken into account during the initial fetch.
The reason for this is that the initial fetch is not a fully fledged
'git fetch' but a bunch of direct calls into the fetch/transport
machinery with clone's own refs-to-refspec matching logic, which
bypasses parts of 'git fetch' processing configured fetch refspecs.
This logic only considers a single default refspec, potentially
influenced by options like '--single-branch' and '--mirror'. The
configured refspecs are, however, already read and parsed properly
when clone calls remote.c:remote_get(), but it never looks at the
parsed refspecs in the resulting 'struct remote'.
Modify clone to take the remote's configured fetch refspecs into
account to retrieve all matching refs during the initial fetch. Note
that we have to explicitly add the default fetch refspec to the
remote's refspecs, because at that point the remote only includes the
fetch refspecs specified on the command line.
Add tests to check that refspecs given both via 'git clone -c ...' and
'git -c ... clone' retrieve all refs matching either the default or
the additional refspecs, and that it works even when the user
specifies an alternative remote name via '--origin='.
Signed-off-by: SZEDER Gábor
---
builtin/clone.c | 25 +-
t/t5611-clone-config.sh | 47 +
2 files changed, 62 insertions(+), 10 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index ae1bf242c6..7c7f98c72c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -548,7 +548,7 @@ static struct ref *find_remote_branch(const struct ref
*refs, const char *branch
}
static struct ref *wanted_peer_refs(const struct ref *refs,
- struct refspec_item *refspec)
+ struct refspec *refspec)
{
struct ref *head = copy_ref(find_ref_by_name(refs, "HEAD"));
struct ref *local_refs = head;
@@ -569,13 +569,19 @@ static struct ref *wanted_peer_refs(const struct ref
*refs,
warning(_("Could not find remote branch %s to clone."),
option_branch);
else {
- get_fetch_map(remote_head, refspec, , 0);
+ int i;
+ for (i = 0; i < refspec->nr; i++)
+ get_fetch_map(remote_head, >items[i],
+ , 0);
/* if --branch=tag, pull the requested tag explicitly */
get_fetch_map(remote_head, tag_refspec, , 0);
}
- } else
- get_fetch_map(refs, refspec, , 0);
+ } else {
+ int i;
+ for (i = 0; i < refspec->nr; i++)
+ get_fetch_map(refs, >items[i], , 0);
+ }
if (!option_mirror && !option_single_branch && !option_no_tags)
get_fetch_map(refs, tag_refspec, , 0);
@@ -899,7 +905,6 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
int err = 0, complete_refs_before_fetch = 1;
int submodule_progress;
- struct refspec rs = REFSPEC_INIT_FETCH;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
fetch_if_missing = 0;
@@ -1081,11 +1086,12 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (option_required_reference.nr || option_optional_reference.nr)
setup_reference();
+ remote = remote_get(option_origin);
+
strbuf_addf(_refspec, "+%s*:%s*", src_ref_prefix,
branch_top.buf);
- refspec_append(, default_refspec.buf);
+ refspec_append(>fetch, default_refspec.buf);
- remote = remote_get(option_origin);
transport = transport_get(remote, remote->url[0]);
transport_set_verbosity(transport, option_verbosity, option_progress);
transport->family = family;
@@ -1140,7 +1146,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
argv_array_push(_prefixes, "HEAD");
- refspec_ref_prefixes(, _prefixes);
+ refspec_ref_prefixes(>fetch, _prefixes);
if (option_branch)
[PATCH 3/3] Documentation/clone: document ignored configuration variables
Due to limitations in the current implementation, some configuration
variables specified via 'git clone -c var=val' (or 'git -c var=val
clone') are ignored during the initial fetch and checkout.
Let the users know which configuration variables are known to be
ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the
documentation of 'git clone -c', along with hints to use the options
'--mirror' and '--no-tags' instead.
Signed-off-by: SZEDER Gábor
---
Documentation/git-clone.txt | 6 ++
1 file changed, 6 insertions(+)
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index a55536f0bf..2fd12524f9 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -189,6 +189,12 @@ objects from the source repository into a pack in the
cloned repository.
values are given for the same key, each value will be written to
the config file. This makes it safe, for example, to add
additional fetch refspecs to the origin remote.
++
+Due to limitations of the current implementation, some configuration
+variables do not take effect until after the initial fetch and checkout.
+Configuration variables known to not take effect are:
+`remote..mirror` and `remote..tagOpt`. Use the
+corresponding `--mirror` and `--no-tags` options instead.
--depth ::
Create a 'shallow' clone with a history truncated to the
--
2.19.1.1182.gbfcc7ed3e6
[PATCH] INSTALL: add macOS gettext and sdk header explanation to INSTALL
From: out0fmemory --- INSTALL | 7 +++ 1 file changed, 7 insertions(+) diff --git a/INSTALL b/INSTALL index c39006e8e7..b7bfb53c12 100644 --- a/INSTALL +++ b/INSTALL @@ -165,6 +165,9 @@ Issues of note: use English. Under autoconf the configure script will do this automatically if it can't find libintl on the system. +In macOS, can install and link gettext with brew as "brew install gettext" +and "brew link --force gettext", the link operation is necessary. + - Python version 2.4 or later (but not 3.x, which is not supported by Perforce) is needed to use the git-p4 interface to Perforce. @@ -178,6 +181,10 @@ Issues of note: will include them. Note that config.mak is not distributed; the name is reserved for local settings. + - In macOs 10.14, the Command Line Tool not contains the headers as before, so it +need install Command Line Tool 10.14 and install the headers which command +"sudo installer -pkg /Library/Developer/CommandLineTools/Packages/macOS_SDK_headers_for_macOS_10.14.pkg -target". + - To build and install documentation suite, you need to have the asciidoc/xmlto toolchain. Because not many people are inclined to install the tools, the default build target -- 2.19.1.1052.gd166e6afe5
Re: [PATCH v4 0/1] Advertise multiple supported proto versions
Josh Steadmon writes: > Fix several bugs identified in v3, clarify commit message, and clean up > extern keyword in protocol.h. It is good to descirbe the change relative to v3 here, which would help those who are interested and reviewed v3. To help those who missed the boat and v4 is their first encounter with this series, having the description relative to v3 alone and nothing else is not very friendly, though. > + diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c > + --- a/builtin/upload-archive.c > + +++ b/builtin/upload-archive.c > +@@ > + #include "builtin.h" > + #include "archive.h" > + #include "pkt-line.h" > ++#include "protocol.h" > + #include "sideband.h" > + #include "run-command.h" > + #include "argv-array.h" > +@@ > + if (argc == 2 && !strcmp(argv[1], "-h")) > + usage(upload_archive_usage); > + > ++register_allowed_protocol_version(protocol_v0); > ++ > + /* > + * Set up sideband subprocess. > + * This one unfortunately seems to interact rather badly with your js/remote-archive-v2 topic which is already in 'next', when this topic is merged to 'pu', and my attempt to mechanically resolve conflicts breaks t5000.
Re: [PATCH v2] checkout: print something when checking out paths
Nguyễn Thái Ngọc Duy writes: > One of the problems with "git checkout" is that it does so many > different things and could confuse people specially when we fail to > handle ambiguation correctly. You would have realized that this is way too noisy if you ran "make test", which may have spewed something like this on the screen. [19:09:19] t4120-apply-popt.sh ok 1624 ms ( 0.26 usr 0.21 sys + 5.31 cusr 3.51 csys = 9.29 CPU) [19:09:20] t9164-git-svn-dcommit-concurrent.sh skipped: Perl SVN libraries not found or unusable [19:09:20] t1310-config-default.sh ok 177 ms ( 0.07 usr 0.01 sys + 0.89 cusr 0.66 csys = 1.63 CPU) ===( 20175;154 1297/? 155/? 6/? 3/3 2/? 4/? 4/? 3/? 5... )===Checked out 1 path out of the index Checked out 1 path out of the index Checked out 1 path out of the index Checked out 1 path out of the index Checked out 1 path out of the index [19:09:20] t1408-packed-refs.sh ... ok 310 ms ( 0.06 usr 0.00 sys + 0.69 cusr 0.52 csys = 1.27 CPU) [19:09:20] t0025-crlf-renormalize.sh .. ok 246 ms ( 0.03 usr 0.01 sys + 0.34 cusr 0.22 csys = 0.60 CPU) I am very tempted to suggest to treat this as a training wheel and enable only when checkout.showpathcount is set to true, or something like that.
[PATCH v2 0/2] rebase.useBuiltin doc & test mode
I was missing a "=" in the t/README docs in 2/2 in v1. Also move the docs around slightly so this & my gettext series don't have conflicts. *** BLURB HERE *** Ævar Arnfjörð Bjarmason (2): rebase doc: document rebase.useBuiltin tests: add a special setup where rebase.useBuiltin is off Documentation/config/rebase.txt | 14 ++ builtin/rebase.c| 5 - t/README| 4 3 files changed, 22 insertions(+), 1 deletion(-) Range-diff: 1: a5508195c6 ! 1: ca87aacbfa tests: add a special setup where rebase.useBuiltin is off @@ -29,12 +29,13 @@ --- a/t/README +++ b/t/README @@ - index to be written after every 'git repack' command, and overrides the - 'core.multiPackIndex' setting to true. + GIT_TEST_PRELOAD_INDEX= exercises the preload-index code path + by overriding the minimum number of cache entries required per thread. -+GIT_TEST_REBASE_USE_BUILTIN, when false, disables the builtin -+version of git-rebase. See 'rebase.useBuiltin' in git-config(1). ++GIT_TEST_REBASE_USE_BUILTIN=, when false, disables the ++builtin version of git-rebase. See 'rebase.useBuiltin' in ++git-config(1). + - Naming Tests - - + GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading + of the index for the whole test suite by bypassing the default number of + cache entries and thread minimums. Setting this to 1 will make the -- 2.19.1.1182.g4ecb1133ce
[PATCH v2 1/2] rebase doc: document rebase.useBuiltin
The rebase.useBuiltin variable introduced in 55071ea248 ("rebase:
start implementing it as a builtin", 2018-08-07) was turned on by
default in 5541bd5b8f ("rebase: default to using the builtin rebase",
2018-08-08), but had no documentation.
Let's document it so that users who run into any stability issues with
the C rewrite know there's an escape hatch[1], and make it clear that
needing to turn off builtin rebase means you've found a bug in git.
1. https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/config/rebase.txt | 14 ++
1 file changed, 14 insertions(+)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index 42e1ba7575..f079bf6b7e 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -1,3 +1,17 @@
+rebase.useBuiltin::
+ Set to `false` to use the legacy shellscript implementation of
+ linkgit:git-rebase[1]. Is `true` by default, which means use
+ the built-in rewrite of it in C.
++
+The C rewrite is first included with Git version 2.20. This option
+serves an an escape hatch to re-enable the legacy version in case any
+bugs are found in the rewrite. This option and the shellscript version
+of linkgit:git-rebase[1] will be removed in some future release.
++
+If you find some reason to set this option to `false` other than
+one-off testing you should report the behavior difference as a bug in
+git.
+
rebase.stat::
Whether to show a diffstat of what changed upstream since the last
rebase. False by default.
--
2.19.1.1182.g4ecb1133ce
[PATCH v2 2/2] tests: add a special setup where rebase.useBuiltin is off
Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent
to running with rebase.useBuiltin=false. This is needed to spot that
we're not introducing any regressions in the legacy rebase version
while we're carrying both it and the new builtin version.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
builtin/rebase.c | 5 -
t/README | 4
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..68ad8c1149 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -48,7 +48,10 @@ static int use_builtin_rebase(void)
{
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf out = STRBUF_INIT;
- int ret;
+ int ret, env = git_env_bool("GIT_TEST_REBASE_USE_BUILTIN", -1);
+
+ if (env != -1)
+ return env;
argv_array_pushl(,
"config", "--bool", "rebase.usebuiltin", NULL);
diff --git a/t/README b/t/README
index 242497455f..3df5d12e46 100644
--- a/t/README
+++ b/t/README
@@ -339,6 +339,10 @@ for the index version specified. Can be set to any valid
version
GIT_TEST_PRELOAD_INDEX= exercises the preload-index code path
by overriding the minimum number of cache entries required per thread.
+GIT_TEST_REBASE_USE_BUILTIN=, when false, disables the
+builtin version of git-rebase. See 'rebase.useBuiltin' in
+git-config(1).
+
GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading
of the index for the whole test suite by bypassing the default number of
cache entries and thread minimums. Setting this to 1 will make the
--
2.19.1.1182.g4ecb1133ce
[PATCH 0/2] rebase.useBuiltin doc & test mode
On Wed, Nov 14 2018, Stefan Beller wrote: >> But maybe I'm being overly paranoid. What do those more familiar with >> this think? > > I am not too worried, > * as rebase is a main porcelain, that is even hard to use in a script. > so any failures are not deep down in some automation, > but when found exposed quickly (and hopefully reported). > * 5541bd5b8f was merged to next a month ago; internally we >distribute the next branch to Googlers (on a weekly basis) >and we have not had any bug reports regarding rebase. >(Maybe our environment is too strict for the wide range > of bugs reported) I do the same at Booking.com (although at a more ad-hoc schedule) and got the report whose fix is now sitting in "pu" noted upthread. I fear that these sorts of corporate environments, both Google's and Booking's, end up testing a relatively narrow featureset. Most people have similar enough workflows, e.g. just using "git pull --rebase", I'd be surprised if we have more than 2-3 internal users who ever use the --onto option for example. > * Johannes reported that the rebase is used in GfW > > https://public-inbox.org/git/nycvar.qro.7.76.6.1808241320540...@tvgsbejvaqbjf.bet/ >https://github.com/git-for-windows/git/pull/1800 >and from my cursory reading it is part of >2.19.windows, which has a large user base. > >> (and would re-enable rebase.useBuiltin=true in >> master right after 2.20 is out the door). > > That would be fine with me as well, but I'd rather > document rebase.useBuiltin instead of flip-flopping > the switch around the release. > > Have there been any fixes that are only in > the C version (has the shell version already bitrotted)? That's a good question, one which I don't think we knew the answer to before the following patches. As it turns out no, we still run the tests without failures with GIT_TEST_REBASE_USE_BUILTIN=false. Ævar Arnfjörð Bjarmason (2): rebase doc: document rebase.useBuiltin tests: add a special setup where rebase.useBuiltin is off Documentation/config/rebase.txt | 14 ++ builtin/rebase.c| 5 - t/README| 3 +++ 3 files changed, 21 insertions(+), 1 deletion(-) -- 2.19.1.1182.g4ecb1133ce
[PATCH 1/2] rebase doc: document rebase.useBuiltin
The rebase.useBuiltin variable introduced in 55071ea248 ("rebase:
start implementing it as a builtin", 2018-08-07) was turned on by
default in 5541bd5b8f ("rebase: default to using the builtin rebase",
2018-08-08), but had no documentation.
Let's document it so that users who run into any stability issues with
the C rewrite know there's an escape hatch[1], and make it clear that
needing to turn off builtin rebase means you've found a bug in git.
1. https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/config/rebase.txt | 14 ++
1 file changed, 14 insertions(+)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index 42e1ba7575..f079bf6b7e 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -1,3 +1,17 @@
+rebase.useBuiltin::
+ Set to `false` to use the legacy shellscript implementation of
+ linkgit:git-rebase[1]. Is `true` by default, which means use
+ the built-in rewrite of it in C.
++
+The C rewrite is first included with Git version 2.20. This option
+serves an an escape hatch to re-enable the legacy version in case any
+bugs are found in the rewrite. This option and the shellscript version
+of linkgit:git-rebase[1] will be removed in some future release.
++
+If you find some reason to set this option to `false` other than
+one-off testing you should report the behavior difference as a bug in
+git.
+
rebase.stat::
Whether to show a diffstat of what changed upstream since the last
rebase. False by default.
--
2.19.1.1182.g4ecb1133ce
[PATCH 2/2] tests: add a special setup where rebase.useBuiltin is off
Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent
to running with rebase.useBuiltin=false. This is needed to spot that
we're not introducing any regressions in the legacy rebase version
while we're carrying both it and the new builtin version.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
builtin/rebase.c | 5 -
t/README | 3 +++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..68ad8c1149 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -48,7 +48,10 @@ static int use_builtin_rebase(void)
{
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf out = STRBUF_INIT;
- int ret;
+ int ret, env = git_env_bool("GIT_TEST_REBASE_USE_BUILTIN", -1);
+
+ if (env != -1)
+ return env;
argv_array_pushl(,
"config", "--bool", "rebase.usebuiltin", NULL);
diff --git a/t/README b/t/README
index 242497455f..c719e08414 100644
--- a/t/README
+++ b/t/README
@@ -348,6 +348,9 @@ GIT_TEST_MULTI_PACK_INDEX=, when true, forces the
multi-pack-
index to be written after every 'git repack' command, and overrides the
'core.multiPackIndex' setting to true.
+GIT_TEST_REBASE_USE_BUILTIN, when false, disables the builtin
+version of git-rebase. See 'rebase.useBuiltin' in git-config(1).
+
Naming Tests
--
2.19.1.1182.g4ecb1133ce
Re: [PATCH] push: change needlessly ambiguous example in error
Matthieu Moy writes: > "Junio C Hamano" wrote: > >> > Where 'topic' is a tracking branch of 'origin/master' (I use >> > push.default=upstream). I only recently discovered that I could push to >> > 'HEAD" to do the same thing. So one ulterior motive is to make that more >> > prominent. > [...] >> Do we consider the current behaviour useful? Is it documented > > Yes, since 1750783 (Documentation: more git push examples, 2009-01-26). > > It may be an accident that the doc says "to the same name on the > remote." since it predates the introduction of push.default, but it > does say so and it's the actual behavior. > >> already and widely known? > > https://stackoverflow.com/questions/14031970/git-push-current-branch-shortcut > > 458 votes for the answer suggesting it. OK, that probably is good enough reason to keep the current behaviour and convince ourselves that there is nothing that needs to be done further on this topic. Thanks.
Re: [RFC PATCH 0/2] Fix scissors bug during merge conflict
On Wed, Nov 14, 2018 at 04:52:59PM +0900, Junio C Hamano wrote: > Denton Liu writes: > > > With this fix, the message becomes the following: > > > > Merge branch 'master' into new > > > > # >8 > > # Do not modify or remove the line above. > > # Everything below it will be ignored. > > # > > # Conflicts: > > # a > > I have a mixed feeling about this change and I certainly would not > call it a "fix". > > The reason why we give the list of conflicted paths that is > commented out is to remind the user of them in order to help them > describe what area of the codebase had overlapping changes, why, and > how the overlap was resolved, which is relevant information when > somebody else later needs to dig into the history to understand how > the code came into today's shape and why. For that reason, if a > careless user left conflicts list behind without describing these > details about the merge, it might be better to have the unexplained > list in the merge than nothing. > The reason why I implemented it this way is because the default cleanup setting (strip) produces this message: Merge branch 'master' into new # Conflicts: # a # # It looks like you may be committing a merge. # If this is not correct, please remove the file # .git/MERGE_HEAD # and try again. # Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # On branch new # All conflicts fixed but you are still merging. # # Changes to be committed: # modified: a # Which makes it seem like the `Conflicts:` section should be removed by default. > In theory, the above argument applies the same way for the paths to > be committed, but the list is fairly trivial to recreate with "git > diff $it^!", unlike "which paths had conflict", which can only be > found out by recreating the auto-merge. So in practice, the paths > that had conflicts is more worth showing than the paths that were > modified. > > So, I dunno. If we value the "more expensive list to reproduce", > the fix might be not to place it (and possibly the comments and > everything under the scissors line) behind a "# " comment char on > the line, without moving its position. If I understood correctly, then I have no strong opinions between uncommenting the Conflicts section by default and this change; I just think it'd be nice to have behaviour that's consistent. > > . > >
Re: [RFC PATCH 1/2] commit: don't add scissors line if one exists
Denton Liu writes: > If commit.cleanup = scissors is specified, don't produce a scissors line > if one already exists in the commit message. It is good that you won't have two such lines in the end result, but is this (1) hiding real problem under the rug? (2) losing information? If the current invocation of "git commit" added a scissors line in the buffer to be edited already, and we are adding another one in this function, is it possible that the real problem that somebody else has called wt_status_add_cut_line() before this function is called, in which case that other caller is what we need to fix, instead of this one? If the existing line in the buffer came from the end user (perhaps it was given from "-F ", etc., with "-e" option) or --amend, how can we be sure if it is OK to lose everything after that scissors looking line? In other words, the scissors looking line may just be part of the log message, in which case we may want to quote/escape it, so that the true scissors we append at a later place in the buffer would be noticed without losing the text before and after that scissors looking line we already had when this function was called?
