Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper
gt; - cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \ >> >> - done && \ >> > >> > This indeed looks like a mess... >> > >> >> + ./install_programs \ >> >> + --X="$$X" \ >> >> + --RM="$(RM)" \ >> >> + --bindir="$$bindir" \ >> >> + --bindir-relative="$(bindir_relative_SQ)" \ >> >> + --execdir="$$execdir" \ >> >> + --destdir-from-execdir="$$destdir_from_execdir_SQ" \ >> >> + --flag-install-symlinks="$(INSTALL_SYMLINKS)" \ >> >> + --flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \ >> >> + >> >> --flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \ >> >> + --list-bindir-standalone="git$X $(filter >> >> $(install_bindir_programs),$(ALL_PROGRAMS))" \ >> >> + --list-bindir-git-dashed="$(filter >> >> $(install_bindir_programs),$(BUILT_INS))" \ >> >> + --list-execdir-git-dashed="$(BUILT_INS)" \ >> >> + --list-execdir-curl-aliases="$(REMOTE_CURL_ALIASES)" && \ >> >> ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X" >> >> >> >> .PHONY: install-gitweb install-doc install-man install-man-perl >> >> install-html install-info install-pdf >> >> diff --git a/install_programs b/install_programs >> >> new file mode 100755 >> >> index 00..e287108112 >> >> --- /dev/null >> >> +++ b/install_programs >> >> @@ -0,0 +1,89 @@ >> >> +#!/bin/sh >> >> + >> >> +while test $# != 0 >> >> +do >> >> + case "$1" in >> >> + --X=*) >> >> + X="${1#--X=}" >> >> + ;; >> >> + --RM=*) >> >> + RM="${1#--RM=}" >> >> + ;; >> >> + --bindir=*) >> >> + bindir="${1#--bindir=}" >> >> + ;; >> >> + --bindir-relative=*) >> >> + bindir_relative="${1#--bindir-relative=}" >> >> + ;; >> >> + --execdir=*) >> >> + execdir="${1#--execdir=}" >> >> + ;; >> >> + --destdir-from-execdir=*) >> >> + destdir_from_execdir="${1#--destdir-from-execdir=}" >> >> + ;; >> >> + --flag-install-symlinks=*) >> >> + INSTALL_SYMLINKS="${1#--flag-install-symlinks=}" >> >> + ;; >> >> + --flag-no-install-hardlinks=*) >> >> + NO_INSTALL_HARDLINKS="${1#--flag-no-install-hardlinks=}" >> >> + ;; >> >> + --flag-no-cross-directory-hardlinks=*) >> >> + >> >> NO_CROSS_DIRECTORY_HARDLINKS="${1#--flag-no-cross-directory-hardlinks=}" >> >> + ;; >> >> + --list-bindir-standalone=*) >> >> + list_bindir_standalone="${1#--list-bindir-standalone=}" >> >> + ;; >> >> + --list-bindir-git-dashed=*) >> >> + list_bindir_git_dashed="${1#--list-bindir-git-dashed=}" >> >> + ;; >> >> + --list-execdir-git-dashed=*) >> >> + list_execdir_git_dashed="${1#--list-execdir-git-dashed=}" >> >> + ;; >> >> + --list-execdir-curl-aliases=*) >> >> + list_execdir_curl_aliases="${1#--list-execdir-curl-aliases=}" >> >> + ;; >> >> + >> >> + *) >> >> + echo "Unknown option $1" >> >> + exit 1 >> >> + ;; >> >> + esac >> >> + shift >> >> +done && >> >> +{ test "$bindir/" = "$execdir/" || >> >> + for p in $list_bindir_standalone; do >> >> + $RM "$execdir/$p" && >> >> + test -n "$INSTALL_SYMLINKS" && >> >> + ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" || >> >> + { test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" && >> >> + ln "$bindir/$p" "$execdir/$p" 2>/dev/null || >> >> + cp &
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Slavica Djukic writes: >>> + git var GIT_COMMITTER_IDENT >actual && >>> + test_cmp expected actual && >> I am not sure what you are testing with this step. There is nothing >> that changed environment variables or configuration since we ran >> "git var" above. Why does this test suspect that somebody in the >> future may break the expectation that after running 'git add' and/or >> 'git stash', our committer identity may have been changed, and how >> would such a breakage happen? > In previous re-roll, you suggested that test should be improved so > that when > reasonable identity is present, git stash executes under that > identity, and not > under the fallback one. Yes, but for that you'd need to be checking the resulting commit object that represents the stash entry. It should be created under the substitute identity. > Here I'm just making sure that after calling > git stash, > we still have same reasonable identity present. I do not think such a test would detect it, even when "git stash" incorrectly used the fallback identity to create the stash entry.
Re: [PATCH] ref-filter: don't look for objects when outside of a repository
Jeff King writes: > If you are arguing that even in a repo we should reject "authorname" > early (just as we would outside of a repo), I could buy that. Yup, that (and replace 'authorname' with anything that won't work with missing objects) for consistency was what I meant.
Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin
Johannes Schindelin writes: > Meaning: essentially, `rebase.useBuiltin` was defaulting to `false`, and > if a user installed Git for Windows with the experimental built-in rebase, > it was set to `true` in the system config. Oh, that changes the picture entirely. If that was what was shipped to Windows users for 2.19.X, then I'd say we should be able to trust the switch well enough. I just somehow thought that everybody in the Windows land has been using the -in-C version. >> Having said that, assuming that the switching back to scripted >> version works correctly and assuming that we want to expose this to >> end users, I think the patch text makes sense. > > Indeed. > > I would still love to see the built-in rebase to be used by default in > v2.20.0, and I am reasonably sure that the escape hatch works (because, as > I told you above, it worked in the reverse, making the built-in rebase an > opt-in in Git for Windows v2.19.1). Good. That makes things a lot simpler. Thanks.
Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin
Hi Junio, On Fri, 16 Nov 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > 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. > > I actually thought that everybody understood that this was merely an > aid to be used during the development of the feature and never meant > to be given to the end users. It may have been from git.git's point of view, but from Git for Windows' point of view it was always meant to be a real feature flag. > With my devil's advocate hat on, how much do we trust it as an > escape hatch? As you know, only a fraction of the bug reports about the built-in rebase came in from Git for Windows: the autostash-with-submodules bug and the perf-regression one. By my counting that is 2 out of 5 bugs coming in via that route. One of the reasons for that was that the built-in rebase that was shipped in Git for Windows v2.19.1 was marked as experimental. And the way I could mark it experimental was by flipping the default to executing the scripted version: https://github.com/git-for-windows/git/commit/cff1a96cfe (you will note that I added the same escape hatch for `git stash` by adding back `git-stash.sh` as `git-legacy-stash.sh` and imitating the same dance as for built-in `rebase`, and I also added back the scripted `git-rebase--interactive.sh` for use by `git-legacy-rebase.sh`). Meaning: essentially, `rebase.useBuiltin` was defaulting to `false`, and if a user installed Git for Windows with the experimental built-in rebase, it was set to `true` in the system config. There was not a single complaint about the scripted `git rebase` being broken in any way. So we *do* have some real-world testing of that feature. (Obviously I have no numbers about Git for Windows' usage, apart from download numbers, and they do not say how many users opted in and how many did not, but Git for Windows v2.19.1 was downloaded more than 2.7 million times so far and I think it is safe to assume that some percentage tested that feature.) > After all, the codepath to hide the "rebase in C" implementation and use > the scripted version were never in 'master' (or 'next' for that matter) > with this variable turned off, so I am reasonably sure it had no serious > exposure to real world usage. See above for a counter-argument. > Having said that, assuming that the switching back to scripted > version works correctly and assuming that we want to expose this to > end users, I think the patch text makes sense. Indeed. I would still love to see the built-in rebase to be used by default in v2.20.0, and I am reasonably sure that the escape hatch works (because, as I told you above, it worked in the reverse, making the built-in rebase an opt-in in Git for Windows v2.19.1). Ciao, Dscho
Re: [PATCH] ref-filter: don't look for objects when outside of a repository
On Fri, Nov 16, 2018 at 02:09:07PM +0900, Junio C Hamano wrote: > >> I see problems in both directions: > >> > >> - sorting by "objectname" works now, but it's marked with SOURCE_OBJ, > >>and would be forbidden with your patch. I'm actually not sure if > >>SOURCE_OBJ is accurate; we shouldn't need to access the object to > >>show it (and we are probably wasting effort loading the full contents > >>for tools like for-each-ref). > >> > >>However, that's not the full story. For objectname:short, it _does_ call > >>find_unique_abbrev(). So we expect to have an object directory. > > > > Oops, I'm apparently bad at reading. It is in fact SOURCE_OTHER, which > > makes sense (outside of this whole "--sort outside a repo thing"). > > > > But we'd ideally distinguish between "objectname" (which should be OK > > outside a repo) and "objectname:short" (which currently segfaults). > > Arguably, use of ref-filter machinery in ls-remote, whether it is > given from inside or outside a repo, was a mistake in 1fb20dfd > ("ls-remote: create '--sort' option", 2018-04-09), as the whole > point of "ls-remote" is to peek the list of refs and it is perfectly > normal that the objects listed are not available. I think it's conceptually reasonable to use the ref-filter machinery. It's just that it was underprepared to handle this out-of-repo case. I think we're not too far off, though. > "ls-remote --sort=authorname" that is run in a repository may not > segfault on a ref that points at a yet-to-be-fetched commit, but it > cannot be doing anything sensible. Is it still better to silently > produce a nonsense result than refusing to --sort no matter what the > sort keys are, whether we are inside or outside a repository? I don't think we produce silent nonsense in the current code (or after any of the discussed solutions), either in a repo or out. We say "fatal: missing object ..." inside a repo if the request cannot be fulfilled. That's not incredibly illuminating, perhaps, but it means we fulfill whatever we _can_ on behalf of the user's request, and bail otherwise. If you are arguing that even in a repo we should reject "authorname" early (just as we would outside of a repo), I could buy that. Technically we can make it work sometimes (if we happen to have fetched everything the other side has), but behaving consistently (and with a decent error message) may trump that. -Peff
Re: [PATCH v3 00/11] fast export and import fixes and features
On Thu, Nov 15, 2018 at 11:59:45PM -0800, Elijah Newren wrote: > This is a series of small fixes and features for fast-export and > fast-import, mostly on the fast-export side. > > Changes since v2 (full range-diff below): > * Dropped the final patch; going to try to use Peff's suggestion of > rev-list and diff-tree to get what I need instead > * Inserted a new patch at the beginning to convert pre-existing sha1 > stuff to oid (rename sha1_to_hex() -> oid_to_hex(), rename > anonymize_sha1() to anonymize_oid(), etc.) > * Modified other patches in the series to add calls to oid_to_hex() rather > than sha1_to_hex() Thanks, these changes all look good to me. I have no more nits to pick. :) -Peff
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Hi Junio, On 16-Nov-18 6:55 AM, Junio C Hamano wrote: Slavica Djukic writes: +test_expect_failure 'stash works when user.name and user.email are not set' ' + git reset && + git var GIT_COMMITTER_IDENT >expected && All the other existing test pieces in this file calls the expected result "expect"; is there a reason why this patch needs to be different (e.g. 'expect' file left by the earlier step needs to be kept unmodified for later use, or something like that)? If not, please avoid making a difference in irrelevant details, as that would waste time of readers by forcing them to guess if there is such a reason that readers cannot immediately see. There is no specific reason for file to be "expected", I'll update that. Anyway, we grab the committer ident we use by default during the test with this command. OK. + >1 && + git add 1 && + git stash && And we make sure we can create stash. + git var GIT_COMMITTER_IDENT >actual && + test_cmp expected actual && I am not sure what you are testing with this step. There is nothing that changed environment variables or configuration since we ran "git var" above. Why does this test suspect that somebody in the future may break the expectation that after running 'git add' and/or 'git stash', our committer identity may have been changed, and how would such a breakage happen? In previous re-roll, you suggested that test should be improved so that when reasonable identity is present, git stash executes under that identity, and not under the fallback one. Here I'm just making sure that after calling git stash, we still have same reasonable identity present. + >2 && + git add 2 && + test_config user.useconfigonly true && + test_config stash.usebuiltin true && Now we start using use-config-only, so unsetting environment variables will cause trouble when Git insists on having an explicitly configured identities. Makes sense. + ( + 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 && And then we try the same test, but without environment or config. Since we are unsetting the environment, in order to be nice for future test writers, we do this in a subshell, so that we do not have to restore the original values of environment variables. Don't we need to be nice the same way for configuration variables, though? We _know_ that nobody sets user.{email,name} config up to this point in the test sequence, so that is why we do not do a "save before test and then restore to the original" dance on them. Even though we are relying on the fact that these two variables are left unset in the configuration file, we unconfig them here anyway, and I do think it is a good idea for documentation purposes (i.e. we are not documenting what we assume the config before running this test would be; we are documenting what state we want these two variables are in when running this "git stash"---that is, they are both unset). So these later part of this test piece makes sense. I still do not know what you wanted to check in the earlier part of the test, though. + git stash + ) +' + test_done Thank you, Slavica
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Junio C Hamano writes: > Slavica Djukic writes: > >> +test_expect_failure 'stash works when user.name and user.email are not set' >> ' >> +git reset && >> +git var GIT_COMMITTER_IDENT >expected && > ... > Anyway, we grab the committer ident we use by default during the > test with this command. OK. > >> +>1 && >> +git add 1 && >> +git stash && > > And we make sure we can create stash. > >> +git var GIT_COMMITTER_IDENT >actual && >> +test_cmp expected actual && > > I am not sure what you are testing with this step. There is nothing > that changed environment variables or configuration since we ran > "git var" above. Why does this test suspect that somebody in the > future may break the expectation that after running 'git add' and/or > 'git stash', our committer identity may have been changed, and how > would such a breakage happen? Just a note. "git var GIT_COMMITTER_IDENT" has timestamp in it, so a naïve reader might wonder what would happen if "git add 1" and "git stash" took more than one second. But it won't be a problem in this case as the committer date comes from the environment GIT_COMMITTER_DATE, which is set to a fixed known value and is incremented only by calling test_commit helper function, which does not happen between the two "git var" calls. In any case, I am not sure I understand the point of comparing two output from "git var" invocations we see ablve in this test.
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Junio C Hamano writes: > Now we start using use-config-only, so unsetting environment > variables will cause trouble when Git insists on having an > explicitly configured identities. Makes sense. > >> +( >> +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 && > > And then we try the same test, but without environment or config. I suspect that it makes sense to replace the "git stash" we see below with something like this: test_must_fail git commit -m should fail && echo "git stash " >expect && echo >2 && git stash && git show -s --format="%(authorname) <%(authoremail)>" refs/stash >actual && test_cmp expect actual That is - we make sure "commit" would not go through, to make sure our preparation to unset environment variables was sufficient; - we make sure "stash" does succeed (which is the primary thing you are interested in); - we make sure the resulting "stash" is not created under our default identity but under our fallback one. >> +git stash >> +) >> +' >> + >> test_done
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Slavica Djukic writes: > +test_expect_failure 'stash works when user.name and user.email are not set' ' > + git reset && > + git var GIT_COMMITTER_IDENT >expected && All the other existing test pieces in this file calls the expected result "expect"; is there a reason why this patch needs to be different (e.g. 'expect' file left by the earlier step needs to be kept unmodified for later use, or something like that)? If not, please avoid making a difference in irrelevant details, as that would waste time of readers by forcing them to guess if there is such a reason that readers cannot immediately see. Anyway, we grab the committer ident we use by default during the test with this command. OK. > + >1 && > + git add 1 && > + git stash && And we make sure we can create stash. > + git var GIT_COMMITTER_IDENT >actual && > + test_cmp expected actual && I am not sure what you are testing with this step. There is nothing that changed environment variables or configuration since we ran "git var" above. Why does this test suspect that somebody in the future may break the expectation that after running 'git add' and/or 'git stash', our committer identity may have been changed, and how would such a breakage happen? > + >2 && > + git add 2 && > + test_config user.useconfigonly true && > + test_config stash.usebuiltin true && Now we start using use-config-only, so unsetting environment variables will cause trouble when Git insists on having an explicitly configured identities. Makes sense. > + ( > + 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 && And then we try the same test, but without environment or config. Since we are unsetting the environment, in order to be nice for future test writers, we do this in a subshell, so that we do not have to restore the original values of environment variables. Don't we need to be nice the same way for configuration variables, though? We _know_ that nobody sets user.{email,name} config up to this point in the test sequence, so that is why we do not do a "save before test and then restore to the original" dance on them. Even though we are relying on the fact that these two variables are left unset in the configuration file, we unconfig them here anyway, and I do think it is a good idea for documentation purposes (i.e. we are not documenting what we assume the config before running this test would be; we are documenting what state we want these two variables are in when running this "git stash"---that is, they are both unset). So these later part of this test piece makes sense. I still do not know what you wanted to check in the earlier part of the test, though. > + git stash > + ) > +' > + > test_done
Re: [PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity
Slavica Djukic writes: > 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 That looks like a sensible implementation to me. > 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' ' This line claims to the readers of patch that the known breakage this known test piece demonstrated has been corrected, but they need to refresh their memory by going back to the previous patch to see if this "failure-to-success" flipping is done to the right test piece, and what exactly the test piece tested to see the existing breakage, because all the interesting part of the test are chomped outside the post-context of this hunk. Unless the fix is fairly complex, adding ought-to-succeed tests that expect success that break when the code change gets omitted from the patch in the same patch as the fix itself (i.e. squash patch 1/2 and patch 2/2 into a single patch) would be more helpful for the readers (it also helps cherry-picking the fix later to earlier maintenance tracks if it becomes necessary). > git reset && > git var GIT_COMMITTER_IDENT >expected && > >1 &&
Re: [PATCH] ref-filter: don't look for objects when outside of a repository
Jeff King writes: > On Thu, Nov 15, 2018 at 04:38:44AM -0500, Jeff King wrote: > >> Is SOURCE_NONE a complete match for what we want? >> >> I see problems in both directions: >> >> - sorting by "objectname" works now, but it's marked with SOURCE_OBJ, >>and would be forbidden with your patch. I'm actually not sure if >>SOURCE_OBJ is accurate; we shouldn't need to access the object to >>show it (and we are probably wasting effort loading the full contents >>for tools like for-each-ref). >> >>However, that's not the full story. For objectname:short, it _does_ call >>find_unique_abbrev(). So we expect to have an object directory. > > Oops, I'm apparently bad at reading. It is in fact SOURCE_OTHER, which > makes sense (outside of this whole "--sort outside a repo thing"). > > But we'd ideally distinguish between "objectname" (which should be OK > outside a repo) and "objectname:short" (which currently segfaults). Arguably, use of ref-filter machinery in ls-remote, whether it is given from inside or outside a repo, was a mistake in 1fb20dfd ("ls-remote: create '--sort' option", 2018-04-09), as the whole point of "ls-remote" is to peek the list of refs and it is perfectly normal that the objects listed are not available. "ls-remote --sort=authorname" that is run in a repository may not segfault on a ref that points at a yet-to-be-fetched commit, but it cannot be doing anything sensible. Is it still better to silently produce a nonsense result than refusing to --sort no matter what the sort keys are, whether we are inside or outside a repository?
Re: [[PATCH v2] ] INSTALL: add macOS gettext and sdk header explanation to INSTALL
On Thu, Nov 15, 2018 at 11:07 PM Junio C Hamano wrote: > yanke131...@gmail.com writes: > > * add macOS gettext explanation to get the i18n locale translation take > > effect in macOS, as the most polular way of gettext > > install in macOS, the gettext is not linked by default, this commit give > > a tip on this thing. > > Also I am not quite sure what it wants to say. Perhaps you meant > to say something like this? > > Explain how to make the gettext library usable on macOS, as > with the most popular way to install it, it won't be linked > to /usr/local. > > I think the part that I had most trouble understanding was your use > of the verb "link"; it was unclear (and I am guessing) that you > meant there are missing links on the filesystem to make stuff from > gettext package available to programs that want to build with it [...] You inferred correctly, and your rewritten text conveys the needed information. > > * add macOS Command Line Tool sdk header explanation to get correct build > > in macOS 10.14+, as the CLT not install > > the header by default, we need install it by self, this commit give a way > > to install the loss headers. > > Similarly, is > > Explain how to install the Command Line Tool SDK headers > manually on macOS 10.14+ in order to correctly build Git, as > they are not installed by default. > > what you meant? Also correct. > > +In macOS, can install gettext with brew as "brew install gettext" > > +and "brew link --force gettext", the gettext is keg-only so brew not > > link > > +it to /usr/local by default, so link operation is necessary, or you can > > +follow the brew tips after install gettext. > > My best guess of what you wanted to say is > > On macOS, `gettext` can be installed with `brew install > gettext`, but because the `gettext` package is keg-only and > is not made available in `/usr/local` by default. `brew s/./,/ > link --force gettext` must be run after `brew install > gettext` to correct this to use i18n features of Git. > > but now the sentence structure is quite different and I no longer > know if that is what you meant to say. And it does not help that I > am not a Mac user. Aside from the minor punctuation issue, your rewrite correctly captures the intent and is understandable. > > If not link gettext correctly, > > +the git after build will not have correct locale translations, english > > is the > > +default language. > > If my rephrasing above is correct, then these three lines become > unnecessary, I think. Yep. > > + - In macOs 10.14, the Command Line Tool not contains sdk headers as > > before, so > > +need install Command Line Tool 10.14 and install the headers with > > command > > On macOS 10.14, the Command Line Tool no longer contains the > SDK headers; you need to also install them with the command: > > > +If not install the sdk headers correctly, git build will get errors > > blew, factly is > > +is because of this problem. > > I can guess this wants to say "without sdk headers your attempt to > build Git will blow up in your face", but not quite. > > Unless you install the SDK headers, building git will fail > with error messages like the following: Although, as you note for the other case, this sentence and the following example error message are likely unnecessary.
Re: [PATCH 0/3] clone: respect configured fetch respecs during initial fetch
Jeff King writes: > On Wed, Nov 14, 2018 at 11:46:17AM +0100, SZEDER Gábor wrote: > >> 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). > > Thanks for sticking with this! > > I skimmed over the old discussion, mostly just to make sure there wasn't > anything subtle that might have been forgotten. But nope, all of the > subtlety went away because of the refspec API you mentioned. > > The whole series looks good to me. Thanks, both.
Re: insteadOf and git-request-pull output
Konstantin Ryabitsev writes: > On Thu, Nov 15, 2018 at 07:54:32PM +0100, Ævar Arnfjörð Bjarmason wrote: >> > I think that if we use the "principle of least surprise," insteadOf >> > rules shouldn't be applied for git-request-pull URLs. >> >> I haven't used request-pull so I don't have much of an opinion on this, >> but do you think the same applies to 'git remote get-url '? >> >> I.e. should it also show the original unmunged URL, or the munged one as >> it does now? > > I don't know, maybe both? As opposed to git-request-pull, this is not > exposing the insteadOf URL to someone other than the person who set it > up, so even if it does return the munged URL, it wouldn't be unexpected. Yeah, I think the local "git remote" should show the rewritten URL (because that information is not available otherwise) and it is OK to optionally show the original. Also, I think it would be nice if request-pull gave external-facing names. After all, insteadOf address is for your own use (e.g. maybe pointing at corporate mirror), and not for the intended recipient of request-pull. In short, I think I agree with things you are saying in this exchange.
Re: [[PATCH v2] ] INSTALL: add macOS gettext and sdk header explanation to INSTALL
yanke131...@gmail.com writes: > Subject: Re: [[PATCH v2] ] INSTALL: add macOS gettext and sdk header > explanation to INSTALL The above should look more like Subject: [PATCH v2] INSTALL: add macOS ... > From: out0fmemory This line is to give information that records who the patch was writtten by, and later Signed-off-by: line certifies that the person (which typically is, and in this case also is, the person who wrote it) has the right to contribute it to the project under the project's licensing terms. We want to see the name and address on these two lines to match. > * add macOS gettext explanation to get the i18n locale translation take > effect in macOS, as the most polular way of gettext > install in macOS, the gettext is not linked by default, this commit give a > tip on this thing. > Please wrap overlong lines to ~70 cols. Also I am not quite sure what it wants to say. Perhaps you meant to say something like this? Explain how to make the gettext library usable on macOS, as with the most popular way to install it, it won't be linked to /usr/local. I think the part that I had most trouble understanding was your use of the verb "link"; it was unclear (and I am guessing) that you meant there are missing links on the filesystem to make stuff from gettext package available to programs that want to build with it, and instead your original text (aside from grammatical issues) sounded as if you were reporting lack of linker flags when building binary or something. > * add macOS Command Line Tool sdk header explanation to get correct build in > macOS 10.14+, as the CLT not install > the header by default, we need install it by self, this commit give a way > to install the loss headers. Similarly, is Explain how to install the Command Line Tool SDK headers manually on macOS 10.14+ in order to correctly build Git, as they are not installed by default. what you meant? > Signed-off-by: yanke > --- > INSTALL | 20 > 1 file changed, 20 insertions(+) > > diff --git a/INSTALL b/INSTALL > index c39006e8e7..ed4bd29f8f 100644 > --- a/INSTALL > +++ b/INSTALL > @@ -165,6 +165,13 @@ 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 gettext with brew as "brew install gettext" > +and "brew link --force gettext", the gettext is keg-only so brew not link > +it to /usr/local by default, so link operation is necessary, or you can > +follow the brew tips after install gettext. Sorry, but I cannot quite understand this overlong and grammatically unparsable single sentence. There is no subject for verb phrase "can install" at the beginning of the sentence where I already got stuck X-<. My best guess of what you wanted to say is On macOS, `gettext` can be installed with `brew install gettext`, but because the `gettext` package is keg-only and is not made available in `/usr/local` by default. `brew link --force gettext` must be run after `brew install gettext` to correct this to use i18n features of Git. but now the sentence structure is quite different and I no longer know if that is what you meant to say. And it does not help that I am not a Mac user. > If not link gettext correctly, > +the git after build will not have correct locale translations, english > is the > +default language. > + If my rephrasing above is correct, then these three lines become unnecessary, I think. > + - In macOs 10.14, the Command Line Tool not contains sdk headers as > before, so > +need install Command Line Tool 10.14 and install the headers with command On macOS 10.14, the Command Line Tool no longer contains the SDK headers; you need to also install them with the command: > +"sudo installer -pkg > /Library/Developer/CommandLineTools/Packages/macOS_SDK_headers_for_macOS_10.14.pkg > -target". > +If not install the sdk headers correctly, git build will get errors > blew, factly is > +is because of this problem. I can guess this wants to say "without sdk headers your attempt to build Git will blow up in your face", but not quite. Unless you install the SDK headers, building git will fail with error messages like the following: Perhaps. > +ld: archive has no table of contents file 'xdiff/lib.a' for architecture > x86_64 > +clang: error: linker command failed with exit code 1 (use -v to see > invocation) > +make: *** [Makefile:2369: git-fast-import] Error 1 > +ld: archive has no table of contents file 'xdiff/lib.a' for architecture > x86_64 > +clang:
Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin
Ævar Arnfjörð Bjarmason writes: > 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. I actually thought that everybody understood that this was merely an aid to be used during the development of the feature and never meant to be given to the end users. With my devil's advocate hat on, how much do we trust it as an escape hatch? After all, the codepath to hide the "rebase in C" implementation and use the scripted version were never in 'master' (or 'next' for that matter) with this variable turned off, so I am reasonably sure it had no serious exposure to real world usage. Having said that, assuming that the switching back to scripted version works correctly and assuming that we want to expose this to end users, I think the patch text makes sense. Thanks. > 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.
Re: [RFC PATCH 1/2] commit: don't add scissors line if one exists
Denton Liu writes: >> 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. That is exactly what the paragraph you are responding to questions. Is the code that adds a scissors line before this function is called done the right way? Shouldn't it be doing something differnetly? Looking for an existing scissors looking line in this function does not let this function differenciate two cases, i.e. we deliberately added one already before calling this function (in which case this function should not add another one), or we didn't add anything on our own, but the material supplied by the end user had one (in which case, not adding ours is losing information---imagine that the user notices a scissors-looking line that came from the original maerial and want to munge it, as it is part of proper message, so that it would remain in the committed result, but because [PATCH 1/2] stopped adding a scissors line at the right location, the user would have to guess where to add one). There must be an explicit way (e.g. a bit in a flag word parameter given to this function) for the caller who knows when the new code in [PATCH 2/2] triggers, to tell this function not to add another one, instead of a sloppy (and less efficient) "lets's scan to see if there already is a scissors looking line". > 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. Doing the "is there already a scissors looing line" approach will *make* it harder to fix that issue, so the patch is making things worse.
Re: [PATCH v3 1/1] protocol: advertise multiple supported versions
Josh Steadmon writes: >> 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. OK, let me try to see if I understand correctly by rephrasing. If the client does not say anything about which version it prefers (because it only knows about version 0 without even realizing that there is a version negotiation exchange), we substitute the lack of proposed versions with string "version=0", and the strcmp()s I saw and was puzzled by are all about special casing such a client. But we would end up behaving the same way (at least when judged only by externally visible effects) to a client that is aware of version negotiation and tells us it prefers version 0 (and nothing else) with the selection logic anyway. Did I get it right? If so, yeah, I think it makes sense to avoid two codepaths that ends up doing the same thing by removing the special case. > 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: Confusing behavior with ignored submodules and `git commit -a`
On 2018-11-15, Stefan Beller wrote: > On Thu, Nov 15, 2018 at 1:33 PM Michael Forney wrote: >> Well, currently the submodule config can be disabled in diff_flags by >> setting override_submodule_config=1. However, I'm thinking it may be >> simpler to selectively *enable* the submodule config in diff_flags >> where it is needed instead of disabling it everywhere else (i.e. >> use_submodule_config instead of override_submodule_config). > > This sounds like undoing the good(?) part of the series that introduced > this regression, as before that we selectively loaded the submodule > config, which lead to confusion when you forgot it. Selectively *enabling* > the submodule config sounds like that state before? > > Or do we *only* talk about enabling the ignore flag, while loading the > rest of the submodule config automatic? Yes, that is what I meant. I believe the automatic loading of submodule config is the right thing to do, it just uncovered cases where the current handling of override_submodule_config is not quite sufficient. My suggestion of replacing override_submodule_config with use_submodule_config is because it seems like there are fewer places where we want to apply the ignore config than not. I think it should only apply in diffs against the working tree and when staging changes to the index (unless specified explicitly). The documentation just mentions the "diff family", but all but one of the possible values for submodule..ignore ("all") don't make sense unless comparing with the working tree. This is also how show/log -p behaved in git <2.15. So I think that clarifying that it is about modifications *to the working tree* would be a good idea. >> I'm also starting to see why this is tricky. The only difference that >> diff.c:run_diff_files sees between `git add inner` and `git add --all` >> is whether the index entry matched the pathspec exactly or not. > > Unrelated to the trickiness, I think we'd need to document the behavior > of the -a flag in git-add and git-commit better as adding the diff below > will depart from the "all" rule again, which I thought was a strong > motivator for Brandons series (IIRC). Can you explain what you mean by the "all" rule? >> Here is a work-in-progress diff that seems to have the correct >> behavior in all cases I tried. Can you think of any cases that it >> breaks? I'm not quite sure of the consequences of having diff_change >> and diff_addremove always ignore the submodule config; git-diff and >> git-status still seem to work correctly. >> >> diff --git a/builtin/add.c b/builtin/add.c >> index f65c17229..9902f7742 100644 >> --- a/builtin/add.c >> +++ b/builtin/add.c >> @@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix, >> rev.diffopt.output_format = DIFF_FORMAT_CALLBACK; >> rev.diffopt.format_callback = update_callback; >> rev.diffopt.format_callback_data = >> - rev.diffopt.flags.override_submodule_config = 1; > > This line partially reverts 5556808, taking 02f2f56bc377c28 > into account. Correct. The problem with 55568086 is that add_files_to_cache is used by both git-add and git-commit, regardless of whether --all was specified. So, while it made it possible to stage ignored submodules, it also made it so the submodule ignore config was overridden in all uses of git-add and git-commit. So, this diff attempts to make it so the ignore config is only applied when the pathspec matches exactly rather than just always overriding the ignore config. >> diff --git a/diff-lib.c b/diff-lib.c >> index 83fce5151..fbb048cca 100644 >> --- a/diff-lib.c >> +++ b/diff-lib.c >> @@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry >> *ce, struct stat *st) >> static int match_stat_with_submodule(struct diff_options *diffopt, >> const struct cache_entry *ce, >> struct stat *st, unsigned ce_option, >> -unsigned *dirty_submodule) >> +unsigned *dirty_submodule, >> +int exact) >> [...]; > > This is an interesting take so far as it is all about *detecting* change > here via stat information and not like the previous (before the regression) > where it was about correcting output. > > match_stat_with_submodule would grow its documentation to be > slightly more complicated as a result. Yes, this is one part I'm not quite happy with. I wonder if instead match_stat_with_submodule could be made simpler by moving the ie_match_stat call up to the two call sites, and then it could be selectively called by run_diff_files depending on the value of matched. >> diff --git a/diff.c b/diff.c >> index e38d1ecaf..73dc75286 100644 >> --- a/diff.c >> +++ b/diff.c >> [...] >> -static int is_submodule_ignored(const char *path, struct diff_options >> *options) >> -{ >> [...] >> - if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, >> options))
Re: Confusing behavior with ignored submodules and `git commit -a`
On Thu, Nov 15, 2018 at 1:33 PM Michael Forney wrote: > > On 2018-11-15, Stefan Beller wrote: > > On Wed, Nov 14, 2018 at 10:05 PM Michael Forney > > wrote: > >> 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. > > > > Maybe selectively unloading or overwriting the config? > > > > Or we can change is_submodule_ignored() in diff.c > > to be only applied selectively whether we are running the > > right command? For this approach we'd have to figure out the > > set of commands to which the ignore config should apply or > > not (and come up with a more concise documentation then) > > > > This approach sounds appealing to me as it would cover > > new commands as well and we'd only have a central point > > where the decision for ignoring is made. > > Well, currently the submodule config can be disabled in diff_flags by > setting override_submodule_config=1. However, I'm thinking it may be > simpler to selectively *enable* the submodule config in diff_flags > where it is needed instead of disabling it everywhere else (i.e. > use_submodule_config instead of override_submodule_config). This sounds like undoing the good(?) part of the series that introduced this regression, as before that we selectively loaded the submodule config, which lead to confusion when you forgot it. Selectively *enabling* the submodule config sounds like that state before? Or do we *only* talk about enabling the ignore flag, while loading the rest of the submodule config automatic? > I'm also starting to see why this is tricky. The only difference that > diff.c:run_diff_files sees between `git add inner` and `git add --all` > is whether the index entry matched the pathspec exactly or not. Unrelated to the trickiness, I think we'd need to document the behavior of the -a flag in git-add and git-commit better as adding the diff below will depart from the "all" rule again, which I thought was a strong motivator for Brandons series (IIRC). > Here is a work-in-progress diff that seems to have the correct > behavior in all cases I tried. Can you think of any cases that it > breaks? I'm not quite sure of the consequences of having diff_change > and diff_addremove always ignore the submodule config; git-diff and > git-status still seem to work correctly. > > diff --git a/builtin/add.c b/builtin/add.c > index f65c17229..9902f7742 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix, > rev.diffopt.output_format = DIFF_FORMAT_CALLBACK; > rev.diffopt.format_callback = update_callback; > rev.diffopt.format_callback_data = > - rev.diffopt.flags.override_submodule_config = 1; This line partially reverts 5556808, taking 02f2f56bc377c28 into account. > diff --git a/diff-lib.c b/diff-lib.c > index 83fce5151..fbb048cca 100644 > --- a/diff-lib.c > +++ b/diff-lib.c > @@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry > *ce, struct stat *st) > static int match_stat_with_submodule(struct diff_options *diffopt, > const struct cache_entry *ce, > struct stat *st, unsigned ce_option, > -unsigned *dirty_submodule) > +unsigned *dirty_submodule, > +int exact) > [...]; This is an interesting take so far as it is all about *detecting* change here via stat information and not like the previous (before the regression) where it was about correcting output. match_stat_with_submodule would grow its documentation to be slightly more complicated as a result. > diff --git a/diff.c b/diff.c > index e38d1ecaf..73dc75286 100644 > --- a/diff.c > +++ b/diff.c > [...] > -static int is_submodule_ignored(const char *path, struct diff_options > *options) > -{ > [...] > - if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options)) > + if (S_ISGITLINK(mode) && options->flags.ignore_submodules) > return; This basically inlines the function is_submodule_ignored, except for the part: if (!options->flags.override_submodule_config) set_diffopt_flags_from_submodule_config(options, path); but that was taken care off in match_stat_with_submodule in diff-lib? This WIP looks really promising, thanks for looking into this! Stefan
Re: [PATCHv3 00/23] Bring more repository handles into our code base
> Please have a look at the last 4 patches specifically as they were new in > the last iteration (but did not receive any comment), as they demonstrate > and fix a problem that is only exposed when using GIT_TEST_COMMIT_GRAPH=1 > for the test suite. Thanks. I only reviewed patches 18 and 20-23, as only those are different from the previous iteration according to the range-diff. I've written my comments about patch 18 already [1], and the other patches look good to me. In patch 21, I could go either way about whether it's more desirable to pass the pool or the repository to the freeing functions. Thanks for discovering the issue that patch 23 illustrates. I thought that the tests were written carefully enough in that the_repository didn't have any relevant objects or configurations (all relevant data was in a path that is not the default repository), but apparently some still slipped through. [1] https://public-inbox.org/git/20181115195416.21890-1-jonathanta...@google.com/
Re: Confusing behavior with ignored submodules and `git commit -a`
On 2018-11-15, Michael Forney wrote: > Here is a work-in-progress diff that seems to have the correct > behavior in all cases I tried. I was hoping that gmail wouldn't mess with the whitespace, but apparently it has, sorry about that. Let me try again. --- builtin/add.c | 1 - diff-lib.c| 15 +-- diff.c| 22 ++ 3 files changed, 11 insertions(+), 27 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index f65c17229..9902f7742 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix, rev.diffopt.output_format = DIFF_FORMAT_CALLBACK; rev.diffopt.format_callback = update_callback; rev.diffopt.format_callback_data = - rev.diffopt.flags.override_submodule_config = 1; rev.max_count = 0; /* do not compare unmerged paths with stage #2 */ run_diff_files(, DIFF_RACY_IS_MODIFIED); clear_pathspec(_data); diff --git a/diff-lib.c b/diff-lib.c index 83fce5151..fbb048cca 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry *ce, struct stat *st) static int match_stat_with_submodule(struct diff_options *diffopt, const struct cache_entry *ce, struct stat *st, unsigned ce_option, -unsigned *dirty_submodule) +unsigned *dirty_submodule, +int exact) { int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option); if (S_ISGITLINK(ce->ce_mode)) { struct diff_flags orig_flags = diffopt->flags; - if (!diffopt->flags.override_submodule_config) + if (!diffopt->flags.override_submodule_config && !exact) set_diffopt_flags_from_submodule_config(diffopt, ce->name); if (diffopt->flags.ignore_submodules) changed = 0; @@ -88,7 +89,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt, int run_diff_files(struct rev_info *revs, unsigned int option) { - int entries, i; + int entries, i, matched; int diff_unmerged_stage = revs->max_count; unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED) ? CE_MATCH_RACY_IS_DIRTY : 0); @@ -110,7 +111,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option) if (diff_can_quit_early(>diffopt)) break; - if (!ce_path_match(istate, ce, >prune_data, NULL)) + matched = ce_path_match(istate, ce, >prune_data, NULL); + if (!matched) continue; if (ce_stage(ce)) { @@ -226,7 +228,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option) } changed = match_stat_with_submodule(>diffopt, ce, , - ce_option, _submodule); + ce_option, _submodule, + matched == MATCHED_EXACTLY); newmode = ce_mode_from_stat(ce, st.st_mode); } @@ -292,7 +295,7 @@ static int get_stat_data(const struct cache_entry *ce, return -1; } changed = match_stat_with_submodule(diffopt, ce, , - 0, dirty_submodule); + 0, dirty_submodule, 0); if (changed) { mode = ce_mode_from_stat(ce, st.st_mode); oid = _oid; diff --git a/diff.c b/diff.c index e38d1ecaf..73dc75286 100644 --- a/diff.c +++ b/diff.c @@ -6209,24 +6209,6 @@ int diff_can_quit_early(struct diff_options *opt) opt->flags.has_changes); } -/* - * Shall changes to this submodule be ignored? - * - * Submodule changes can be configured to be ignored separately for each path, - * but that configuration can be overridden from the command line. - */ -static int is_submodule_ignored(const char *path, struct diff_options *options) -{ - int ignored = 0; - struct diff_flags orig_flags = options->flags; - if (!options->flags.override_submodule_config) - set_diffopt_flags_from_submodule_config(options, path); - if (options->flags.ignore_submodules) - ignored = 1; - options->flags = orig_flags; - return ignored; -} - void diff_addremove(struct diff_options *options, int addremove, unsigned mode, const struct object_id *oid, @@ -6235,7 +6217,7 @@ void diff_addremove(struct diff_options *options, { struct diff_filespec *one, *two; - if
Re: [PATCH] remote-curl: die on server-side errors
On 2018.11.14 02:00, Jeff King wrote: > On Tue, Nov 13, 2018 at 07:49:15PM -0500, Jeff King wrote: > > > Yes, the packet_read_line_buf() interface will both advance the buf > > pointer and decrement the length. So if we want to "peek", we have to > > do so with a copy (there's a peek function if you use the packet_reader > > interface, but that might be overkill here). > > > > You can rewrite it like this, which is a pretty faithful conversion and > > passes the tests (but see below). > > [...] > > Here's a version which is less faithful, but I think does sensible > things in all cases, and is much easier to follow. I get a little > nervous just because it tightens some cases, and one never knows if > other implementations might be relying on the looseness. E.g.: > > - in the current code we'd still drop back to dumb http if the server > tells us "application/x-git-upload-pack" but the initial pktline > doesn't start with "#" (even though if it _does_ have "#" at > position 5 but isn't a valid pktline, we'd complain!) > > - right now the v2 protocol does not require the server to say > "application/x-git-upload-pack" for the content-type > > This patch tightens both of those (I also made a few stylistic tweaks, > and added the ERR condition to show where it would go). I dunno. Part of > me sees this as a nice cleanup, but maybe it is better to just leave it > alone. A lot of these behaviors are just how it happens to work now, and > not part of the spec, but we don't know what might be relying on them. At least according to the protocol-v2 and http-protocol docs, the stricter behavior seems correct: For the first point above, dumb servers should never use an "application/x-git-*" content type (http-protocol.txt line 163-167). For the second point, the docs require v2 servers to use "application/x-git-*" content types. protocol-v2.txt lines 63-65 state that v2 clients should make a smart http request, while http-protocol.txt lines 247-252 state that a smart server's response type must be "application/x-git-*". Of course we don't know if other implementations follow the spec, but ISTM that this patch at least doesn't contradict how we've promised the protocols should work. If no one has any objections, I'll include the diff below in v2. Thanks for the help Jeff! > diff --git a/remote-curl.c b/remote-curl.c > index 762a55a75f..1adb96311b 100644 > --- a/remote-curl.c > +++ b/remote-curl.c > @@ -330,9 +330,61 @@ static int get_protocol_http_header(enum > protocol_version version, > return 0; > } > > +static void check_smart_http(struct discovery *d, const char *service, > + struct strbuf *type) > +{ > + char *src_buf; > + size_t src_len; > + char *line; > + const char *p; > + > + if (!skip_prefix(type->buf, "application/x-", ) || > + !skip_prefix(p, service, ) || > + strcmp(p, "-advertisement")) > + return; > + > + /* > + * We speculatively try to read a packet, which means we must preserve > + * the original buf/len pair in some cases. > + */ > + src_buf = d->buf; > + src_len = d->len; > + line = packet_read_line_buf(_buf, _len, NULL); > + if (!line) > + die("invalid server response; expected service, got flush > packet"); > + > + if (skip_prefix(line, "# service=", ) && !strcmp(p, service)) { > + /* > + * The header can include additional metadata lines, up > + * until a packet flush marker. Ignore these now, but > + * in the future we might start to scan them. > + */ > + while (packet_read_line_buf(_buf, _len, NULL)) > + ; > + > + /* > + * v0 smart http; callers expect us to soak up the > + * service and header packets > + */ > + d->buf = src_buf; > + d->len = src_len; > + d->proto_git = 1; > + > + } else if (starts_with(line, "version 2")) { /* should be strcmp()? */ > + /* > + * v2 smart http; do not consume version packet, which will > + * be handled elsewhere. > + */ > + d->proto_git = 1; > + } else if (skip_prefix(line, "ERR ", )) { > + die(_("remote error: %s"), p); > + } else { > + die("invalid server response; got '%s'", line); > + } > +} > + > static struct discovery *discover_refs(const char *service, int for_push) > { > - struct strbuf exp = STRBUF_INIT; > struct strbuf type = STRBUF_INIT; > struct strbuf charset = STRBUF_INIT; > struct strbuf buffer = STRBUF_INIT; > @@ -405,38 +457,8 @@ static struct discovery *discover_refs(const char > *service, int for_push) > last->buf_alloc = strbuf_detach(, >len); > last->buf = last->buf_alloc; > > - strbuf_addf(, "application/x-%s-advertisement", service); > - if
Re: Confusing behavior with ignored submodules and `git commit -a`
On 2018-11-15, Stefan Beller wrote: > On Wed, Nov 14, 2018 at 10:05 PM Michael Forney > wrote: >> 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. > > Maybe selectively unloading or overwriting the config? > > Or we can change is_submodule_ignored() in diff.c > to be only applied selectively whether we are running the > right command? For this approach we'd have to figure out the > set of commands to which the ignore config should apply or > not (and come up with a more concise documentation then) > > This approach sounds appealing to me as it would cover > new commands as well and we'd only have a central point > where the decision for ignoring is made. Well, currently the submodule config can be disabled in diff_flags by setting override_submodule_config=1. However, I'm thinking it may be simpler to selectively *enable* the submodule config in diff_flags where it is needed instead of disabling it everywhere else (i.e. use_submodule_config instead of override_submodule_config). I'm also starting to see why this is tricky. The only difference that diff.c:run_diff_files sees between `git add inner` and `git add --all` is whether the index entry matched the pathspec exactly or not. Here is a work-in-progress diff that seems to have the correct behavior in all cases I tried. Can you think of any cases that it breaks? I'm not quite sure of the consequences of having diff_change and diff_addremove always ignore the submodule config; git-diff and git-status still seem to work correctly. diff --git a/builtin/add.c b/builtin/add.c index f65c17229..9902f7742 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix, rev.diffopt.output_format = DIFF_FORMAT_CALLBACK; rev.diffopt.format_callback = update_callback; rev.diffopt.format_callback_data = - rev.diffopt.flags.override_submodule_config = 1; rev.max_count = 0; /* do not compare unmerged paths with stage #2 */ run_diff_files(, DIFF_RACY_IS_MODIFIED); clear_pathspec(_data); diff --git a/diff-lib.c b/diff-lib.c index 83fce5151..fbb048cca 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry *ce, struct stat *st) static int match_stat_with_submodule(struct diff_options *diffopt, const struct cache_entry *ce, struct stat *st, unsigned ce_option, -unsigned *dirty_submodule) +unsigned *dirty_submodule, +int exact) { int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option); if (S_ISGITLINK(ce->ce_mode)) { struct diff_flags orig_flags = diffopt->flags; - if (!diffopt->flags.override_submodule_config) + if (!diffopt->flags.override_submodule_config && !exact) set_diffopt_flags_from_submodule_config(diffopt, ce->name); if (diffopt->flags.ignore_submodules) changed = 0; @@ -88,7 +89,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt, int run_diff_files(struct rev_info *revs, unsigned int option) { - int entries, i; + int entries, i, matched; int diff_unmerged_stage = revs->max_count; unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED) ? CE_MATCH_RACY_IS_DIRTY : 0); @@ -110,7 +111,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option) if (diff_can_quit_early(>diffopt)) break; - if (!ce_path_match(istate, ce, >prune_data, NULL)) + matched = ce_path_match(istate, ce, >prune_data, NULL); + if (!matched) continue; if (ce_stage(ce)) { @@ -226,7 +228,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option) } changed = match_stat_with_submodule(>diffopt, ce, , - ce_option, _submodule); + ce_option, _submodule, + matched == MATCHED_EXACTLY); newmode = ce_mode_from_stat(ce, st.st_mode); } @@ -292,7 +295,7 @@ static int get_stat_data(const struct cache_entry *ce, return -1; }
Re: [PATCH 18/23] submodule: use submodule repos for object lookup
On Thu, Nov 15, 2018 at 11:54 AM Jonathan Tan wrote: > > > +/* > > + * Initialize 'out' based on the provided submodule path. > > + * > > + * Unlike repo_submodule_init, this tolerates submodules not present > > + * in .gitmodules. This function exists only to preserve historical > > behavior, > > + * > > + * Returns 0 on success, -1 when the submodule is not present. > > */ > > -static void show_submodule_header(struct diff_options *o, const char *path, > > +static struct repository *open_submodule(const char *path) > > The function documentation needs to be reworded - there's no "out", and > the return value is now a possibly NULL pointer to struct repository. Noted. > > > +{ > > + struct strbuf sb = STRBUF_INIT; > > + struct repository *out = xmalloc(sizeof(*out)); > > + > > + if (submodule_to_gitdir(, path) || repo_init(out, sb.buf, NULL)) { > > + strbuf_release(); > > + free(out); > > + return NULL; > > + } > > + > > + out->submodule_prefix = xstrdup(path); > > I've discussed this submodule_prefix line before [1] - do we really need > this? Tests pass even if I remove this line. We might not need it yet as the tests indicate, but it's the right thing to do: /* * Path from the root of the top-level superproject down to this * repository. This is only non-NULL if the repository is initialized * as a submodule of another repository. */ We're not (yet) using this string in our error reporting, but I anticipate that we'll do eventually. > Other than that, this patch looks good. Thanks, Stefan
Re: Confusing behavior with ignored submodules and `git commit -a`
On Wed, Nov 14, 2018 at 10:05 PM Michael Forney wrote: > > +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. Maybe selectively unloading or overwriting the config? Or we can change is_submodule_ignored() in diff.c to be only applied selectively whether we are running the right command? For this approach we'd have to figure out the set of commands to which the ignore config should apply or not (and come up with a more concise documentation then) This approach sounds appealing to me as it would cover new commands as well and we'd only have a central point where the decision for ignoring is made. Stefan
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
Hi Peff, On Thu, 15 Nov 2018, Jeff King wrote: > On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote: > > > I cannot claim that I wrapped my head around your explanation or your > > diff (I am busy trying to prepare Git for Windows' `master` for > > rebasing to v2.20.0-rc0), but it does fix the problem. Thank you so > > much! > > > > The line `test_expect_code 1 ...` needs to be adjusted to > > `test_expect_code 128`, of course, and to discern from the fixed > > problem (which also exits with code 128), the error output should be > > verified, like so: > > > > -- snip -- > > test_expect_success 'try to create a bundle with empty ref count' ' > > test_must_fail git bundle create foobar.bundle master..master 2>err && > > test_i18ngrep "Refusing to create empty bundle" err > > ' > > -- snap -- > > It seems like we should be checking that the stale lockfile isn't left, > which is the real problem (the warning is annoying, but is a symptom of > the same thing). I.e., something like: > > test_must_fail git bundle create foobar.bundle master..master && > test_path_is_missing foobar.bundle.lock > > That would already pass on non-Windows platforms, but that's OK. It will > never give a false failure. > > If you don't mind, can you confirm that the test above fails without > either of the two patches under discussion? This test succeeds with your patch as well as with Gaël's, and fails when neither patch is applied. So you're right, it is the much better test. > > Do you want to integrate this test into your patch and run with it, or > > do you want me to shepherd your patch? > > I'll wrap it up with a commit message and a test. Thank you so much! Dscho
Re: [PATCH 18/23] submodule: use submodule repos for object lookup
> +/* > + * Initialize 'out' based on the provided submodule path. > + * > + * Unlike repo_submodule_init, this tolerates submodules not present > + * in .gitmodules. This function exists only to preserve historical behavior, > + * > + * Returns 0 on success, -1 when the submodule is not present. > */ > -static void show_submodule_header(struct diff_options *o, const char *path, > +static struct repository *open_submodule(const char *path) The function documentation needs to be reworded - there's no "out", and the return value is now a possibly NULL pointer to struct repository. > +{ > + struct strbuf sb = STRBUF_INIT; > + struct repository *out = xmalloc(sizeof(*out)); > + > + if (submodule_to_gitdir(, path) || repo_init(out, sb.buf, NULL)) { > + strbuf_release(); > + free(out); > + return NULL; > + } > + > + out->submodule_prefix = xstrdup(path); I've discussed this submodule_prefix line before [1] - do we really need this? Tests pass even if I remove this line. Other than that, this patch looks good. [1] https://public-inbox.org/git/20181019203750.110741-1-jonathanta...@google.com/
Re: Confusing behavior with ignored submodules and `git commit -a`
> 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. > > 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. That's good. > >> > 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). and both are broken currently (commit -a will commit a submodule if it is changed, and it will also not show that in log, but it did show that it is committing it in the commit message template) > I just came across someone else affected by this issue: > https://github.com/git/git/commit/55568086#commitcomment-27137460 Point taken.
Re: insteadOf and git-request-pull output
On Thu, Nov 15, 2018 at 07:54:32PM +0100, Ævar Arnfjörð Bjarmason wrote: > > I think that if we use the "principle of least surprise," insteadOf > > rules shouldn't be applied for git-request-pull URLs. > > I haven't used request-pull so I don't have much of an opinion on this, > but do you think the same applies to 'git remote get-url '? > > I.e. should it also show the original unmunged URL, or the munged one as > it does now? I don't know, maybe both? As opposed to git-request-pull, this is not exposing the insteadOf URL to someone other than the person who set it up, so even if it does return the munged URL, it wouldn't be unexpected. -K
Re: insteadOf and git-request-pull output
On Thu, Nov 15 2018, Konstantin Ryabitsev wrote: > Hi, all: > > Looks like setting url.insteadOf rules alters the output of > git-request-pull. I'm not sure that's the intended use of insteadOf, > which is supposed to replace URLs for local use, not to expose them > publicly (but I may be wrong). E.g.: > > $ git request-pull HEAD^ git://foo.example.com/example | grep example > git://foo.example.com/example > > $ git config url.ssh://bar.insteadOf git://foo > > $ git request-pull HEAD^ git://foo.example.com/example | grep example > ssh://bar.example.com/example > > I think that if we use the "principle of least surprise," insteadOf > rules shouldn't be applied for git-request-pull URLs. I haven't used request-pull so I don't have much of an opinion on this, but do you think the same applies to 'git remote get-url '? I.e. should it also show the original unmunged URL, or the munged one as it does now?
Re: [PATCH 1/1] mingw: replace an obsolete link with the superseding one
Am 15.11.18 um 12:22 schrieb Johannes Schindelin via GitGitGadget: From: Johannes Schindelin The MSDN documentation has been superseded by Microsoft Docs (which is backed by a repository on GitHub containing many, many files in Markdown format). Signed-off-by: Johannes Schindelin --- compat/mingw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index d2f4fabb44..9e42b0ee26 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1028,8 +1028,8 @@ char *mingw_getcwd(char *pointer, int len) } /* - * See http://msdn2.microsoft.com/en-us/library/17w5ykft(vs.71).aspx - * (Parsing C++ Command-Line Arguments) + * See "Parsing C++ Command-Line Arguments" at Microsoft's Docs: + * https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments */ static const char *quote_arg(const char *arg) { Thank you! That's much better than the original obfuscated page name. -- Hannes
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
On Thu, Nov 15, 2018 at 1:59 PM Johannes Schindelin wrote: > So yes, we are trying to unlink the `.lock` file, and as far as I can tell > that > `unlink()` call comes from the tempfile cleanup asked for by Martin. However, > as > we still have a handle open to that file, that call fails. > > I do not think that there is any better way to fix this than to close the file > explicitly. I may be talking nonsense here but I remember someone mentioned something about deleting a file while it's still open on Windows and after a quick internet search, it may be FILE_SHARE_DELETE. Since _we_ open these files, can we just open them with this to be able to delete them? -- Duy
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote: > I cannot claim that I wrapped my head around your explanation or your diff (I > am > busy trying to prepare Git for Windows' `master` for rebasing to v2.20.0-rc0), > but it does fix the problem. Thank you so much! > > The line `test_expect_code 1 ...` needs to be adjusted to `test_expect_code > 128`, of course, and to discern from the fixed problem (which also exits with > code 128), the error output should be verified, like so: > > -- snip -- > test_expect_success 'try to create a bundle with empty ref count' ' > test_must_fail git bundle create foobar.bundle master..master 2>err && > test_i18ngrep "Refusing to create empty bundle" err > ' > -- snap -- It seems like we should be checking that the stale lockfile isn't left, which is the real problem (the warning is annoying, but is a symptom of the same thing). I.e., something like: test_must_fail git bundle create foobar.bundle master..master && test_path_is_missing foobar.bundle.lock That would already pass on non-Windows platforms, but that's OK. It will never give a false failure. If you don't mind, can you confirm that the test above fails without either of the two patches under discussion? > Do you want to integrate this test into your patch and run with it, or do you > want me to shepherd your patch? I'll wrap it up with a commit message and a test. -Peff
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
Hi Peff, On Thu, 15 Nov 2018, Jeff King wrote: > On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote: > > > > 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. > > > > Without the fix, the added regression test fails thusly: > > > > -- snip -- > > [...] > > ++ test_expect_code 1 git bundle create foobar.bundle master..master > > ++ want_code=1 > > ++ shift > > ++ git bundle create foobar.bundle master..master > > fatal: Refusing to create empty bundle. > > warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash > > directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied > > Hmph. So who has it open, and why isn't the tempfile code working as > designed? > > Aha, I see the problem. We dup() the descriptor in create_bundle(). So > the patch _is_ necessary and (fairly) correct. But the explanation > probably ought to be something like: > > In create_bundle(), we duplicate the lockfile descriptor via dup(). > This means that even though the lockfile code carefully calls close() > before unlinking the lockfile, we may still have the file open. Unix > systems don't care, but under Windows, this prevents the unlink > (causing an annoying warning and a stale lockfile). > > But that also means that all of the other places we could die (e.g., in > write_or_die) are going to have the same problem. We've fixed only one. > Is there a way we can avoid doing the dup() in the first place? > > The comment there explains that we duplicate because write_pack_data() > will close the descriptor. Unfortunately, that's hard to change because > it comes from run-command. But we don't actually need the descriptor > ourselves after it's closed; we're just trying to appease the lockfile > code; see e54c347c1c (create_bundle(): duplicate file descriptor to > avoid closing it twice, 2015-08-10). > > We just need some reasonable way of telling the lock code what's > happening. Something like the patch below, which is a moral revert of > e54c347c1c, but instead wrapping the "lock->fd = -1" in an official API. > > Does this make your warning go away? > > diff --git a/bundle.c b/bundle.c > [...] I cannot claim that I wrapped my head around your explanation or your diff (I am busy trying to prepare Git for Windows' `master` for rebasing to v2.20.0-rc0), but it does fix the problem. Thank you so much! The line `test_expect_code 1 ...` needs to be adjusted to `test_expect_code 128`, of course, and to discern from the fixed problem (which also exits with code 128), the error output should be verified, like so: -- snip -- test_expect_success 'try to create a bundle with empty ref count' ' test_must_fail git bundle create foobar.bundle master..master 2>err && test_i18ngrep "Refusing to create empty bundle" err ' -- snap -- Do you want to integrate this test into your patch and run with it, or do you want me to shepherd your patch? Ciao, Dscho
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Hi Peff, On Thu, 15 Nov 2018, Jeff King wrote: > On Thu, Nov 15, 2018 at 09:40:38AM +0100, Johannes Schindelin wrote: > > > From @chucklu: > > > > > my user case is like this : > > > > > > When I want to cherr-pick commits from A to G (ABCDEFG), image C and E > > > are merge commits. Then I will get lots of popup like: > > > > > >The previous cherry-pick is now empty, possibly due to conflict > > >resolution. > > >If you wish to commit it anyway, use: > > > > > >git commit --allow-empty > > > > > >If you wish to skip this commit, use: > > > > > >git reset > > > > > >Then "git cherry-pick --continue" will resume cherry-picking > > >the remaining commits. > > > > My quick interpretation of this is that the user actually needs a way to > > skip silently commits which are now empty. > > If it's always intended to be used with cherry-pick, shouldn't > cherry-pick learn a --keep-empty (like rebase has)? That would avoid > even stopping for this case in the first place. I'd go for the other way round: --skip-empty. However, given the very unhappy turn in that Git for Windows ticket (somebody asks for a feature, then just sits back, and does not even confirm that the analysis covers their use case, let alone participates in this discussion), I am personally not really interested in driving this one any further. Tanushree proved that they know how to contribute to the Git mailing list, as a pre-requisite for the Outreachy project, and that is the positive outcome of this thread as far as I am concerned. I am pretty happy about that, too. Ciao, Dscho
Re: [PATCH] technical doc: add a design doc for the evolve command
On Thu, Nov 15 2018, sxe...@google.com wrote: > +Detailed design > +=== > +Obsolescence information is stored as a graph of meta-commits. A meta-commit > is > +a specially-formatted merge commit that describes how one commit was created > +from others. > + > +Meta-commits look like this: > + > +$ git cat-file -p > +tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 > +parent aa7ce55545bf2c14bef48db91af1a74e2347539a > +parent d64309ee51d0af12723b6cb027fc9f195b15a5e9 > +parent 7e1bbcd3a0fa854a7a9eac9bf1eea6465de98136 > +author Stefan Xenos 1540841596 -0700 > +committer Stefan Xenos 1540841596 -0700 > +parent-type content > +parent-type obsolete > +parent-type origin > + > +This says “commit aa7ce555 makes commit d64309ee obsolete. It was created by > +cherry-picking commit 7e1bbcd3”. > + > +The tree for meta-commits is always the empty tree whose hash matches > +4b825dc642cb6eb9a060e54bf8d69288fbee4904 exactly, but future versions of git > may > +attach other trees here. For forward-compatibility fsck should ignore such > trees > +if found on future repository versions. Similarly, current versions of git > +should always fill in an empty commit comment and tools like fsck should > ignore > +the content of the commit comment if present in a future repository version. > +This will allow future versions of git to add metadata to the meta-commit > +comments or tree without breaking forwards compatibility. > + > +Parent-type > +--- > +The “parent-type” field in the commit header identifies a commit as a > +meta-commit and indicates the meaning for each of its parents. It is never > +present for normal commits. It is a list of enum values whose order matches > the > +order of the parents. Possible parent types are: > + > +- content: the content parent identifies the commit that this meta-commit is > + describing. > +- obsolete: indicates that this parent is made obsolete by the content > parent. > +- origin: indicates that this parent was generated from the given commit. > + > +There must be exactly one content parent for each meta-commit and it is > always > +be the first parent. The content commit will always be a normal commit and > not a > +meta-commit. However, future versions of git may create meta-commits for > other > +meta-commits and the fsck tool must be aware of this for forwards > compatibility. > + > +A meta-commit can have zero or more obsolete parents. An amend operation > creates > +a single obsolete parent. A merge used to resolve divergence (see divergence, > +below) will create multiple obsolete parents. A meta-commit may have zero > +obsolete parents if it describes a cherry-pick or squash merge that copies > one > +or more commits but does not replace them. > + > +A meta-commit can have zero or more origin parents. A cherry-pick creates a > +single origin parent. Certain types of squash merge will create multiple > origin > +parents. > + > +An obsolete parent or origin parent may be either a normal commit (indicating > +the oldest-known version of a change) or another meta-commit (for a change > that > +has already been modified one or more times). I think it's worth pointing out for those that are rusty on commit object details (but I checked) is that the reason for it not being: tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 parent aa7ce55545bf2c14bef48db91af1a74e2347539a parent-type content parent d64309ee51d0af12723b6cb027fc9f195b15a5e9 parent-type obsolete parent 7e1bbcd3a0fa854a7a9eac9bf1eea6465de98136 parent-type origin author Stefan Xenos 1540841596 -0700 committer Stefan Xenos 1540841596 -0700 Which would be easier to read, is that we're very sensitive to the order of the first few fields (tree -> parent -> author -> committer) and fsck will error out if we interjected a new field.
Re: approxidate woes
On Thu, Nov 15, 2018 at 09:48:54AM -0500, Jeff King wrote: > I don't think "48h" does what you expect either: > > $ t/helper/test-date approxidate now > now -> 2018-11-15 14:43:32 + > > $ t/helper/test-date approxidate 48h > 48h -> 2018-11-15 14:43:34 + > > $ t/helper/test-date approxidate 48.hours > 48.hours -> 2018-11-13 14:43:38 + Whoops, those should all be: t/helper/test-tool date approxidate ... in recent versions of Git (I was bisecting something earlier, and has a crufty test-date left over from an old version!). Adding new unit aliases would be something like: diff --git a/date.c b/date.c index 9bc15df6f9..eb477d1601 100644 --- a/date.c +++ b/date.c @@ -1016,6 +1016,7 @@ static const struct typelen { { "minutes", 60 }, { "hours", 60*60 }, { "days", 24*60*60 }, + { "d", 24*60*60 }, { "weeks", 7*24*60*60 }, { NULL } }; but I suspect we need to tighten up the string matching a bit. I think that would allow "2 dogs" to be parsed as "days". -Peff
Re: approxidate woes
On Thu, Nov 15, 2018 at 02:45:28PM +0100, Andreas Krey wrote: > I've now located why our backup repo shrinks every month: > > git gc --prune=2d > > doesn't do what I expected, and differs a lot from --prune=48h. Yeah, it understands "2 days", but not "d" as a unit. I don't think "48h" does what you expect either: $ t/helper/test-date approxidate now now -> 2018-11-15 14:43:32 + $ t/helper/test-date approxidate 48h 48h -> 2018-11-15 14:43:34 + $ t/helper/test-date approxidate 48.hours 48.hours -> 2018-11-13 14:43:38 + It might be reasonable to teach approxidate these obvious shorthands (one tricky one is "m"; normally I'd say "minute", but in Git timescales "month" is more likely). > Mildly irritating, and worse, hard to find in the documentation. > I failed at the latter and fell back to the sources, finding > './bin-wrappers/test-date approxidate' for trying. > > Where would I look? I don't think approxidate is really documented at all. It started as Linus's idea of "handle what people would probably say", and the fixes over the years have mostly been "eh, that's crazy, let's do better with this input". You'd have to reverse engineer it a bit from the source, unfortunately. -Peff
Re: [RFC] cherry-pick notes to find out cherry-picks from the origin
On Wed, Oct 17, 2018 at 07:39:21AM -0700, Tejun Heo wrote: > A while ago, I proposed changes to name-rev and describe so that they > can identify the commits cherry-picked from the one which is being > shown. > > > https://public-inbox.org/git/20180726153714.gx1934...@devbig577.frc2.facebook.com/T/ > > While the use-cases - e.g. tracking down which release / stable > branches a given commit ended up in - weren't controversial, it was > suggested that it'd make more sense to use notes to link cherry-picks > instead of building the feature into name-rev. Sorry for the slow reply. This was on my to-look-at pile, but for some reason I accidentally put in my done pile. > The patch appended to this message implements most of it (sans tests > and documentation). It's composed of the following two parts. > > * A new built-in command note-cherry-picks, which walks the specified > commits and if they're marked with the cherry-pick trailer, adds the > backlink to the origin commit using Cherry-picked-to tag in a > cherry-picks note. That makes sense. I think this could also be an option to cherry-pick, to instruct it to create the note when the cherry-pick is made. But you may still want a command to backfill older cherry-picks, or those done by other people who do not care themselves about maintaining the notes tree. It _feels_ like this is something that should be do-able by plugging a few commands together, rather than writing a new C program. I.e., something like: git rev-list --format='%(trailers)' HEAD | perl -lne ' /^commit ([0-9]+)/ and $commit = $1; /^\(cherry picked from commit ([0-9]+)/ and print "$commit $1"; ' | while read from to; do # One process per note isn't very efficient. Ideally there would # be an "append --stdin" mode. Double points if it understands # how to avoid adding existing lines. git notes append -m "Cherry-picked-to: $to" $from done which is roughly what your program is doing. Not that I'm entirely opposed to doing something in C (we've been moving away from shell scripts anyway). But mostly I am wondering if we can leverage existing tools, and fill in their gaps in a way that lets people easily do similar things. And on that note... > * When formatting a cherry-picks note for display, nested cherry-picks > are followed from each Cherry-picked-to tag and printed out with > matching indentations. That makes sense to me, but does this have to be strictly related to cherry-picks? I.e., in the more generic form, could we have a way of marking a note as "transitive" for display, and the notes-display code would automatically recognize and walk hashes? That would serve your purpose, but would also allow similar things to easily be done in the future. > Combined with name-rev --stdin, it can produce outputs like the following. > [...] Yeah, that looks pretty good. > Locally, the notes can be kept up-to-date with a trivial post-commit > hook which invokes note-cherry-picks on the new commit; however, I'm > having a bit of trouble figuring out a way to keep it up-to-date when > multiple trees are involved. AFAICS, there are two options. > > 1. Ensuring that the notes are always generated on local commits and >whenever new commits are received through fetch/pulls. > > 2. Ensuring that the notes are always generated on local commits and >transported with push/pulls. > > 3. A hybrid approach - also generate notes on the receiving end and >ensure that fetch/pulls receives the notes together (ie. similar to >--tags option to git-fetch). > > #1 seems simpler and more robust to me. Unfortunately, I can't see a > way to implement any of the three options with the existing hooks. > For #1, there's no post-fetch hook. For #2 and #3, there doesn't seem > to be a fool-proof way to make sure that the notes are transported > together. Any suggestions would be greatly appreciated. Yeah, I think (1) is the simplest: it becomes a purely local thing that you've generated these annotations. Unfortunately, no, I don't think there's anything like a post-fetch hook. This might be a good reason to have one. One can always do "git fetch && update-notes" of course, but having fetch feed the script the set of updated ref tips would be very helpful (so you know you can traverse from $old..$new looking for cherry-picks). For (2) and (3), you can push/pull a notes tree, but setting up the refspecs and config is fairly manual. Using the cherry-pick option I suggested above would most locally-made picks, but you'd probably still need to backfill sometimes anyway. I don't think you'd need to be (nor am I sure you _could_ be) as fancy as fetching notes and their respective commits together. The notes are bound together in a single tree, so you cannot say "I don't have commit 1234abcd, I don't need the note for it". You get the whole tree. And that is not such a bad thing. The big reason _not_
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote: > > 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. > > Without the fix, the added regression test fails thusly: > > -- snip -- > [...] > ++ test_expect_code 1 git bundle create foobar.bundle master..master > ++ want_code=1 > ++ shift > ++ git bundle create foobar.bundle master..master > fatal: Refusing to create empty bundle. > warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash > directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied Hmph. So who has it open, and why isn't the tempfile code working as designed? Aha, I see the problem. We dup() the descriptor in create_bundle(). So the patch _is_ necessary and (fairly) correct. But the explanation probably ought to be something like: In create_bundle(), we duplicate the lockfile descriptor via dup(). This means that even though the lockfile code carefully calls close() before unlinking the lockfile, we may still have the file open. Unix systems don't care, but under Windows, this prevents the unlink (causing an annoying warning and a stale lockfile). But that also means that all of the other places we could die (e.g., in write_or_die) are going to have the same problem. We've fixed only one. Is there a way we can avoid doing the dup() in the first place? The comment there explains that we duplicate because write_pack_data() will close the descriptor. Unfortunately, that's hard to change because it comes from run-command. But we don't actually need the descriptor ourselves after it's closed; we're just trying to appease the lockfile code; see e54c347c1c (create_bundle(): duplicate file descriptor to avoid closing it twice, 2015-08-10). We just need some reasonable way of telling the lock code what's happening. Something like the patch below, which is a moral revert of e54c347c1c, but instead wrapping the "lock->fd = -1" in an official API. Does this make your warning go away? diff --git a/bundle.c b/bundle.c index 1ef584b93b..dc26551b83 100644 --- a/bundle.c +++ b/bundle.c @@ -244,7 +244,7 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs) /* Write the pack data to bundle_fd, then close it if it is > 1. */ -static int write_pack_data(int bundle_fd, struct rev_info *revs) +static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_info *revs) { struct child_process pack_objects = CHILD_PROCESS_INIT; int i; @@ -256,6 +256,14 @@ static int write_pack_data(int bundle_fd, struct rev_info *revs) pack_objects.in = -1; pack_objects.out = bundle_fd; pack_objects.git_cmd = 1; + + /* +* At this point we know that start_command is going to close our +* bundle_fd, whether successful or not. Tell the lock code that +* it is no longer in charge of it, so we don't try to double-close. +*/ + lock_file_release_descriptor(lock); + if (start_command(_objects)) return error(_("Could not spawn pack-objects")); @@ -421,21 +429,10 @@ int create_bundle(struct bundle_header *header, const char *path, bundle_to_stdout = !strcmp(path, "-"); if (bundle_to_stdout) bundle_fd = 1; - else { + else bundle_fd = hold_lock_file_for_update(, path, LOCK_DIE_ON_ERROR); - /* -* write_pack_data() will close the fd passed to it, -* but commit_lock_file() will also try to close the -* lockfile's fd. So make a copy of the file -* descriptor to avoid trying to close it twice. -*/ - bundle_fd = dup(bundle_fd); - if (bundle_fd < 0) - die_errno("unable to dup file descriptor"); - } - /* write signature */ write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature)); @@ -463,10 +460,8 @@ int create_bundle(struct bundle_header *header, const char *path, goto err; /* write pack */ - if (write_pack_data(bundle_fd, )) { - bundle_fd = -1; /* already closed by the above call */ + if (write_pack_data(bundle_fd, , )) goto err;
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
Hi Peff, On Wed, 14 Nov 2018, Jeff King wrote: > 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. Without the fix, the added regression test fails thusly: -- snip -- [...] ++ test_expect_code 1 git bundle create foobar.bundle master..master ++ want_code=1 ++ shift ++ git bundle create foobar.bundle master..master fatal: Refusing to create empty bundle. warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied ++ exit_code=128 ++ test 128 = 1 ++ echo 'test_expect_code: command exited with 128, we wanted 1 git bundle create foobar.bundle master..master' test_expect_code: command exited with 128, we wanted 1 git bundle create foobar.bundle master..master ++ return 1 error: last command exited with $?=1 not ok 9 - try to create a bundle with empty ref count # # test_expect_code 1 git bundle create foobar.bundle master..master # -- snap -- So yes, we are trying to unlink the `.lock` file, and as far as I can tell that `unlink()` call comes from the tempfile cleanup asked for by Martin. However, as we still have a handle open to that file, that call fails. I do not think that there is any better way to fix this than to close the file explicitly. If we tried to just close whatever file descriptor is still open to that file before deleting it, we would possibly cause problems in code that is still to be executed and assumes that it has a perfectly valid file descriptor. Besides, trying to do this kind of "automatically" won't work, like, at all, when it is one child process that holds an open file descriptor while another process wants to delete the file. Ciao, Dscho
Re: [PATCH] technical doc: add a design doc for the evolve command
Hi Stefan, On Wed, 14 Nov 2018, sxe...@google.com wrote: > 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. Thanks, this is a good discussion starter. > +Objective > +- > +Track the edits to a commit over time in an obsolescence graph. I am not sure that we necessarily need this to be a graph. I think part of the problems with not being able to GC *any* of this is by this requirement to have it stored in a graph, rather than having mappings from which you could reconstruct any non-GC'ed parts of that graph, if you really want. > +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. Okay. > +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. Why is this missing most notably `hg evolve`? Also, there should be *at least* a brief introduction how `hg evolve` works. They do have the benefit of real-world testing, and probably encountered problems and came up with solutions, and we would be remiss if we did not learn from them. Also, please do not forget `git imerge`.
Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories
On Thu, Nov 15, 2018 at 01:29:58PM +0100, Johannes Schindelin wrote: > > 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). > > I fear that that might buy us a ton of trouble. Just like we override the > system config, we should override the templates. Yes, it might. I guess it just seems plausible to me that somebody would expect GIT_TEST_INSTALLED to be as close to the installed experience as possible. I dunno. I do not use it myself. At any rate, my point was that for GIT_TEST_INSTALLED, either: 1. We can use a known-clean set of templates (either our local templates/blt, or an even-cleaner empty set). or 2. We do not need to specify any template, and it will just use whatever it came installed with. And in either case, we do not have to worry about asking it "where are your templates?". -Peff
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Hi Slavica, this looks very good to me. Just one grammar thing: On Wed, 14 Nov 2018, Slavica Djukic wrote: > 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. In a later commit [...] Otherwise, I would be totally fine with this version being merged. Ciao, Johannes > > 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 > >
Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories
Hi Junio, On Wed, 14 Nov 2018, Jeff King wrote: > 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). I fear that that might buy us a ton of trouble. Just like we override the system config, we should override the templates. Ciao, Dscho
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
dex 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. Right. It is not exactly a democracy over here, but I do agree that both of your opinions combined weigh more than my single one. Besides, you have a good point: the documentation is not so much about being technically correct, but about being helpful to the users. And the new version is probably more helpful than the old version. > > > -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? > > The code is still correctly guarding, the diff is just confusing. > Sorry about that. To explain, the code previously was basically: > > if git_am_opt: > if interactive: > if incompatible_opts: > show_error_about_interactive_and_am_incompatibilities > if rebase-merge: > if incompatible_opts > show_error_about_merge_and_am_incompatibilities > > which was a triply nested if, and the first (git_am_opt) and third > (incompatible_opts) were slightly redundant; the latter being a subset > of the former. Ah, that's what I missed! Thank you for your explanation. > Perhaps I should have done a separate cleanup patch before this that > changed it to: > > if incomptable_opts: > if interactive: > show_error_about_interactive_and_am_incompatibilities > if rebase-merge: > show_error_about_merge_and_am_incompatibilities > > Before then having this patch coalesce that down to: > > if incomptable_opts: > if interactive: > show_error_about_incompatible_options > > Since it tripped up both you and Phillip, I'll add this preliminary > cleanup as a separate commit with accompanying explanation in my next > re-roll. Thank you, much appreciated. > > > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh > > > index 0392e36d23..04d6c71899 100755 > > > --- a/t/t3406-rebase-message.sh > > > +++ b/t/t3406-rebase-message.sh > > > @@ -17,14 +17,9 @@ test_expect_success 'setup' ' > > > git tag start > > > ' > > > > > > -cat >expect <<\EOF > > > -Already applied: 0001 A > > > -Already applied: 0002 B > > > -Committed: 0003 Z > > > -EOF > > > - > > > > As I had mentioned in the previous round: I think we can retain these > > messages for the `--merge` mode. And I think we should: users of --merge > > have most likely become to expect them. > > > > The most elegant way would probably be by adding code to the sequencer > > that outputs these lines if in "merge" mode (and add a flag to say that we > > *are* in "merge" mode). > > > > To that end, the `make_script()` function would not generate `# pick` but > > `drop` lines, I think, so that the sequencer can see those and print the > > `Already applied: ` lines. And a successful `TODO_PICK` would > > write out `Committed: `. > > I'll take a look, but is this a case where you want it only when > rebase previously showed them, or should it also affect other cases > such as --rebase-merges or -
Re: [PATCH 0/3] clone: respect configured fetch respecs during initial fetch
On Wed, Nov 14, 2018 at 11:46:17AM +0100, SZEDER Gábor wrote: > 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). Thanks for sticking with this! I skimmed over the old discussion, mostly just to make sure there wasn't anything subtle that might have been forgotten. But nope, all of the subtlety went away because of the refspec API you mentioned. The whole series looks good to me. -Peff
Re: [PATCH 3/3] Documentation/clone: document ignored configuration variables
On Wed, Nov 14, 2018 at 11:46:20AM +0100, SZEDER Gábor wrote: > 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. Good idea. > 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. This looks good. I considered at first that this might want to go in a BUGS section of the manpage, but it makes the most sense being right next to the definition of "-c". -Peff
Re: [PATCH 2/3] clone: respect additional configured fetch refspecs during initial fetch
On Wed, Nov 14, 2018 at 11:46:19AM +0100, SZEDER Gábor wrote: > 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. Nicely explained. > 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='. Good. This is all sufficiently subtle that we definitely want to check the related cases. > @@ -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); Wow, this is so much nicer than the older versions. :) I wondered briefly whether this ought to only kick in when the user didn't specify any refspecs. I.e., there is no way with this to say "don't fetch refs/heads/*, but this other thing instead". But the way the existing documentation is written, it's pretty clear that it's about adding to the list of refspecs. We can always something like "--no-default-refspec" later if somebody wants it. > [...] Most of the rest of the patch is just swapping out "rs" for "remote->fetch", which makes sense. So the implementation looks good to me. > diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh > index 39329eb7a8..60c1ba951b 100755 > --- a/t/t5611-clone-config.sh > +++ b/t/t5611-clone-config.sh > @@ -45,6 +45,53 @@ test_expect_success 'clone -c config is available during > clone' ' > test_cmp expect child/file > ' > > +test_expect_success 'clone -c remote.origin.fetch= works' ' > + rm -rf child && > + git update-ref refs/grab/it refs/heads/master && > + git update-ref refs/leave/out refs/heads/master && Checking one in and one out; nice. > + git clone -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" . child && > + git -C child for-each-ref --format="%(refname)" >actual && > + > + cat >expect <<-\EOF && > + refs/grab/it > + refs/heads/master > + refs/remotes/origin/HEAD > + refs/remotes/origin/master > + EOF > + test_cmp expect actual > +' Makes sense. > +test_expect_success 'git -c remote.origin.fetch= clone works' ' > + rm -rf child && > + git -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" clone . child && > + git -C child for-each-ref --format="%(refname)" >actual && > + > + cat >expect <<-\EOF && > + refs/grab/it > + refs/heads/master > + refs/remotes/origin/HEAD > + refs/remotes/origin/master > + EOF > + test_cmp expect actual > +' This relies on establishing grab/it in the previous test. A minor nit, but we could break that more clear by breaking it out into its own "set up refs for extra refspec tests" block. > +test_expect_success 'clone -c remote..fetch= > --origin=' ' > + rm -rf child && > + git clone --origin=upstream \ > + -c "remote.upstream.fetch=+refs/grab/*:refs/grab/*" \ > +
Re: [PATCH 1/3] clone: use a more appropriate variable name for the default refspec
On Wed, Nov 14, 2018 at 11:46:18AM +0100, SZEDER Gábor wrote: > 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. Yep, this is much nicer. -Peff
Re: [PATCH] INSTALL: add macOS gettext and sdk header explanation to INSTALL
Thanks for review, I will summit a new patch soon to improve this. Jonathan Nieder 于2018年11月15日周四 上午11:05写道: > > 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
Re: [PATCH] ref-filter: don't look for objects when outside of a repository
On Thu, Nov 15, 2018 at 04:38:44AM -0500, Jeff King wrote: > Is SOURCE_NONE a complete match for what we want? > > I see problems in both directions: > > - sorting by "objectname" works now, but it's marked with SOURCE_OBJ, >and would be forbidden with your patch. I'm actually not sure if >SOURCE_OBJ is accurate; we shouldn't need to access the object to >show it (and we are probably wasting effort loading the full contents >for tools like for-each-ref). > >However, that's not the full story. For objectname:short, it _does_ call >find_unique_abbrev(). So we expect to have an object directory. Oops, I'm apparently bad at reading. It is in fact SOURCE_OTHER, which makes sense (outside of this whole "--sort outside a repo thing"). But we'd ideally distinguish between "objectname" (which should be OK outside a repo) and "objectname:short" (which currently segfaults). -Peff
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
On Thu, Nov 15, 2018 at 09:40:38AM +0100, Johannes Schindelin wrote: > From @chucklu: > > > my user case is like this : > > > > When I want to cherr-pick commits from A to G (ABCDEFG), image C and E > > are merge commits. Then I will get lots of popup like: > > > >The previous cherry-pick is now empty, possibly due to conflict > >resolution. > >If you wish to commit it anyway, use: > > > >git commit --allow-empty > > > >If you wish to skip this commit, use: > > > >git reset > > > >Then "git cherry-pick --continue" will resume cherry-picking > >the remaining commits. > > My quick interpretation of this is that the user actually needs a way to > skip silently commits which are now empty. If it's always intended to be used with cherry-pick, shouldn't cherry-pick learn a --keep-empty (like rebase has)? That would avoid even stopping for this case in the first place. -Peff
Re: [PATCH] ref-filter: don't look for objects when outside of a repository
On Wed, Nov 14, 2018 at 01:27:25PM +0100, SZEDER Gábor wrote: > 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. Thanks for picking up this loose end. I like the general approach here, but... > 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); Is SOURCE_NONE a complete match for what we want? I see problems in both directions: - sorting by "objectname" works now, but it's marked with SOURCE_OBJ, and would be forbidden with your patch. I'm actually not sure if SOURCE_OBJ is accurate; we shouldn't need to access the object to show it (and we are probably wasting effort loading the full contents for tools like for-each-ref). However, that's not the full story. For objectname:short, it _does_ call find_unique_abbrev(). So we expect to have an object directory. - sorting by "HEAD" hits a BUG(), and would still be allowed with your patch. So I like the idea here that the particular atoms would tell us whether they're going to need to be in a repository or not, but I think the annotations have to be cleaned up first. > 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 > +' Regardless of our solution, we probably want to add an extra test making sure that something vanilla like: nongit git ls-remote --sort=v:refname "$TRASH_DIRECTORY" continues to work (we do test ls-remote outside a repo already, but not with a sort specifier). -Peff
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 asked for clarification at https://github.com/git-for-windows/git/issues/1854 and in my best imitation of Lt Tawney Madison, I report back: >From @chucklu: > my user case is like this : > > When I want to cherr-pick commits from A to G (ABCDEFG), image C and E > are merge commits. Then I will get lots of popup like: > >The previous cherry-pick is now empty, possibly due to conflict >resolution. >If you wish to commit it anyway, use: > >git commit --allow-empty > >If you wish to skip this commit, use: > >git reset > >Then "git cherry-pick --continue" will resume cherry-picking >the remaining commits. My quick interpretation of this is that the user actually needs a way to skip silently commits which are now empty. Ciao, Dscho
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.
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
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
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/^ \(.*\) $/\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? The code is still correctly guarding, the diff is just confusing. Sorry about that. To explain, the code previously was basically: if git_am_opt: if interactive: if incompatible_opts: show_error_about_interactive_and_am_incompatibilities if rebase-merge: if incompatible_opts show_error_about_merge_and_am_incompatibilities which was a triply nested if, and the first (git_am_opt) and third (incompatible_opts) were slightly redundant; the latter being a subset of the former. Perhaps I should have done a separate cleanup patch before this that changed it to: if incomptable_opts: if interactive: show_error_about_interactive_and_am_incompatibilities if rebase-merge: show_error_about_merge_and_am_incompatibilities Before then having this patch coalesce that down to: if incomptable_opts: if interactive: show_error_about_incompatible_options Since it tripped up both you and Phillip, I'll add this preliminary cleanup as a separate commit with accompanying explanation in my next re-roll. > > @@ -672,7 +664,7 @@ require_clean_work_tree "rebase" "$(gettext "Please > > commit or stash them.")" > > # but this should be done only when upstream and onto are the same > > # and if this is not an interactive rebase. > > mb=$(git merge-base "$onto" "$orig_head") > > -if test -z "$interactive_rebase" && test "$upstream" = "$onto" && > > +if test -z "$actually_interactive" && test "$upstream" = "$onto" && > > test "$mb" = "$onto" && test -z "$restrict_revision" && > > # linear history? > > ! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > > > /dev/null > > @@ -716,6 +708,19 @@ then > > GIT_PAGER='' git diff --stat --summary "$mb" "$onto" > > fi > > > > +if test -z "$actually_interactive" && test "$mb" = "$orig_head" > > +then > > + # If the $onto is a proper descendant of the tip of the branch, then > > + # we just fast-forwarded. > > This comment is misleading if it comes before, rather than after, the > actual `checkout -q` command. > > > + say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")" > > + GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \ > > + git checkout -q "$onto^0" || die "could not detach HEAD" > > + git update-ref ORIG_HEAD $orig_head > > + move_to_original_branch > > + finish_rebase > > + exit 0 > > +fi > > + > > test -n "$interactive_rebase" && run_specific_rebase > > > > # Detach HEAD and reset the tree > > @@ -725,16 +730,6 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout > > $onto_name" \ > > git checkout -q "$onto^0" || die "could not detach HEAD" > > git update-ref ORIG_HEAD $orig_head > > It is a pity that this hunk header hides the lines that you copied into > the following conditional before moving it before the "if interactive, run > it" line: > > > > > -# If the $onto is a proper descendant of the tip of the branch, then > > -# we just fast-forwarded. > > -if test "$mb" = "$orig_head" > > -then > > - say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")" > > - move_to_original_branch > > - finish_rebase > > - exit 0 > > -fi > > - > > if test -n "$rebase_root" > > then > > revisions="$onto..$orig_head" > > What you did is correct, if duplicating code, and definitely the easiest > way to do it. Just move the comment, and we're good here. Will do. > > > diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh > > deleted file mode 100644 > > [.
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
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.
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) r
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
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
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
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