Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index
Brandon Williams writes: > Convert 'parse_pathspec()' to take an index parameter. > > Since the index is only needed when the PATHSPEC_SUBMODULE_LEADING_PATH > and PATHSPEC_STRIP_SUBMODULE_SLASH flags are given, add a check > requiring a non-NULL index when either of these flags are given. > Convert callers which use these two flags to pass in an index while > having other callers pass in NULL. > > Now that pathspec.c does not use any cache macros and has no references > to 'the_index', mark it by defining NO_THE_INDEX_COMPATIBILITY_MACROS. > > Signed-off-by: Brandon Williams The same comment as 5/8 applies to this change, but it is a bit easier to judge, because it has so many callers, and for some builtins, especially manipulator commands like add, checkout, and commit, there may be a good reason why they want to keep the primary index while playing with an additional in-core index in a distant future. Does a pathspec parsed using one instance of index_state expected to work when matching against a path in another instance of index_state? Otherwise, passing a non-NULL istate to parse_pathspec() would tie the resulting pathspec to a particular index_state in some way and there may need a mechanism to catch an attempt to match paths in another index_state with such a pathspec as an error. Just speculating out loud...
Re: [PATCH 5/8] pathspec: convert strip_submodule_slash to take an index
Brandon Williams writes: > Signed-off-by: Brandon Williams > --- > pathspec.c | 9 + > 1 file changed, 5 insertions(+), 4 deletions(-) This conversion is not wrong per-se, but I wonder if there is a practical situation where we want to validate a pathspec element against an in-core index that does not represent the index currently in effect (aka "the_index"). I do not think of any offhand myself.
Re: [PATCH 4/8] pathspec: rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP
Brandon Williams writes: > Now that there is only a single flag which strips a submodule slash, > rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to > PATHSPEC_STRIP_SUBMODULE_SLASH. This is a logical follow-up to the previous step.
Re: [PATCH 3/8] pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag
Brandon Williams writes: > It's confusing to have two different 'strip submodule slash' flags which > do subtly different things. Both > PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and > PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP will accomplish the same task of > striping a slash from a pathspec which matches a submodule entry in the > index. The only difference is that > PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE will perform additional checks > and die if a pathspec has a leading path component which corresponds to > a submodule. This additional functionality should be split out into its > own flag. > > To this end, rename the PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to > PATHSPEC_SUBMODULE_LEADING_PATH and change its behavior to only die if a > path descends into a submodule. In addition add the > PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to callers which relied on the > old slash stripping functionality. "PATHSPEC_SUBMODULE_LEADING_PATH" feels like an unfinished sentence to me. Do I understand your description correctly if I say it is about "checking" the leading path to see if it overlaps with a submodule path? IOW, I am wondering if the name should have the word CHECK somewhere in it.
Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s
On Wed, May 10, 2017 at 12:33:44AM -0400, Jeff King wrote: > On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote: > > > > Hmm. That makes sense generally, as the request should succeed. But it > > > seems like we're creating a client that will sometimes succeed and > > > sometimes fail, and the reasoning will be somewhat opaque to the user. > > > I have a feeling I'm missing some context on when you'd expect this to > > > kick in. > > > > Specifically, someone I know was looking at building an application > > that is passed only a SHA-1 for the tip of a ref on a popular hosting > > site[1]. They wanted to run `git fetch URL SHA1`, but this failed > > because the site doesn't have upload.allowtipsha1inwant enabled. > > However the SHA1 was clearly in the output of git ls-remote. > > OK. So this is basically a case where we expect that the user knows what > they're doing. > > > For various reasons they expected this to work, because it works > > against other sites that do have upload.allowtipsha1inwant enabled. > > And I personally just expected it to work because the fetch client > > accepts SHA-1s, and the wire protocol uses "want SHA1" not "want ref", > > and the SHA-1 passed on the command line was currently in the > > advertisement when the connection opened, so its certainly something > > the server is currently willing to serve. > > Right, makes sense. I wondered if GitHub should be turning on > allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide > some internal refs[1], and they're things that users wouldn't want to > fetch. The problem for your case really is just on the client side, and > this patch fixes it. More broadly, I think it is desirable that any commit that can be reached from public refs can be fetched by an explicit sha1 without allowTipSHA1InWant. Mike
What's cooking in git.git (May 2017, #03; Wed, 10)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. Git 2.13, together with maintenance releases for 9 maintenance tracks, has been tagged. Let's wait for a few days to see if users find embarrassing regressions and then start the next cycle. In the meantime, please pay closer attention to the topics that have been stalled or waiting to be reviewed for too long and think of ways to help moving them forward, before you start sending your new shiny toys to the list. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * jh/verify-index-checksum-only-in-fsck (2017-04-27) 1 commit (merged to 'next' on 2017-04-28 at afb4c70061) + t1450: avoid use of "sed" on the index, which is a binary file Update an unportable constructin a new test. -- [New Topics] * ab/compat-regex-update (2017-05-09) 2 commits - compat/regex: update the gawk regex engine from upstream - compat/regex: add a README with a maintenance guide Will merge to 'next'. * jc/apply-fix-mismerge (2017-05-08) 1 commit (merged to 'next' on 2017-05-09 at e0b89532d0) + apply.c: fix whitespace-only mismerge Will merge to 'master'. * jt/push-options-doc (2017-05-10) 2 commits - receive-pack: verify push options in cert - docs: correct receive.advertisePushOptions default The receive-pack program now makes sure that the push certificate records the same set of push options used for pushing. Will merge to 'next'. * dt/unpack-save-untracked-cache-extension (2017-05-09) 1 commit - DONTMERGE: unpack-trees: preserve index extensions -- [Stalled] * mg/status-in-progress-info (2017-04-14) 1 commit - status: show in-progress info for short status "git status" learns an option to report various operations (e.g. "merging") that the user is in the middle of. It is still unclear if the participants decided that it is OK to spell "--inprogress" as a single word. * mg/name-rev-debug (2017-03-31) 4 commits - describe: pass --debug down to name-rev - name-rev: provide debug output - name-rev: favor describing with tags and use committer date to tiebreak - name-rev: refactor logic to see if a new candidate is a better name "git describe --debug --contains" did not add any meaningful information, even though without "--contains" it did. Have been expecting a reroll of the tip two, but it has not seen any activity for too long. cf. * nd/worktree-move (2017-04-20) 6 commits - worktree remove: new command - worktree move: refuse to move worktrees with submodules - worktree move: accept destination as directory - worktree move: new command - worktree.c: add update_worktree_location() - worktree.c: add validate_worktree() "git worktree" learned move and remove subcommands. Expecting a reroll. cf. <20170420101024.7593-1-pclo...@gmail.com> cf. <20170421145916.mknekgqzhxffu...@sigill.intra.peff.net> cf. * df/dir-iter-remove-subtree (2017-04-19) 5 commits - remove_subtree(): reimplement using iterators - dir_iterator: rewrite state machine model - dir_iterator: refactor dir_iterator_advance - remove_subtree(): test removing nested directories - dir_iterator: add tests for dir_iterator API Update the dir-iterator API and use it to reimplement remove_subtree(). A reroll exists that is based on the updated 'master', but I ran out of time trying to get it to work with other topics in flight in 'pu'. GSoC microproject. * jc/bundle (2016-03-03) 6 commits - index-pack: --clone-bundle option - Merge branch 'jc/index-pack' into jc/bundle - bundle v3: the beginning - bundle: keep a copy of bundle file name in the in-core bundle header - bundle: plug resource leak - bundle doc: 'verify' is not about verifying the bundle The beginning of "split bundle", which could be one of the ingredients to allow "git clone" traffic off of the core server network to CDN. This was surrected from a "to be discarded" pile, as from time to time people wonder about resumable clone that can be primed without bothering Git servers with dynamic packfile creation, and some people seem to think that the topic could serve as a useful building block for that goal. But nothing seem to have happend. Unless people really want it, I am inclined to discard this topic. Opinions? * ja/doc-l10n (2017-03-20) 3 commits . SQUASH??? . l10n: add git-add.txt to localized man pages . l10n: introduce framework for localizing man pages A proposal to use po4a to localize our manual pages. Will discard. Has been stalled for too long. *
[PATCH] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
Let's do the same for Macs, as it is BSD variant after all. Signed-off-by: Junio C Hamano --- config.mak.uname | 1 + 1 file changed, 1 insertion(+) diff --git a/config.mak.uname b/config.mak.uname index a25ffddb3e..1743890164 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -110,6 +110,7 @@ ifeq ($(uname_S),Darwin) BASIC_CFLAGS += -DPRECOMPOSE_UNICODE BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1 HAVE_BSD_SYSCTL = YesPlease + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),SunOS) NEEDS_SOCKET = YesPlease -- 2.13.0-336-gf73534b083
Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings
On Tue, May 02, 2017 at 03:22:23PM +0200, Ævar Arnfjörð Bjarmason wrote: > Is there any way with this to both supply CFLAGS & DEVELOPER=1 on the > command-line, to get my custom -O & these -W flags? I.e.: > > $ make DEVELOPER=1 V=1 > [...] -g -O2 -Wall -Werror -Wdeclaration-after-statement > -Wno-format-zero-length -Wold-style-definition -Woverflow > -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -I. [...] > $ make DEVELOPER=1 CFLAGS="-g -O0 -Wall" V=1 > [...] -g -O0 -Wall -I. [...] > > I thought the second case would prepend my "-g -O0 -Wall" but then be > followed by the various -W developer flags, but it isn't, am I just > doing something stupid, or is there no way to combine these two? The problem is that when you give "make" a variable on the command line, it overrides all of the modifications. So if you were to set that CFLAGS in your config.mak, I think everything would work as you expect. I actually do this in my config.mak: O = 0 CFLAGS += -g -O$(O) which lets me override the optimization level as a one-off on the command-line: make O=2 without disturbing the rest of the CFLAGS. I also do this: CFLAGS += $(EXTRA_CFLAGS) as a catch-all, so that I can do: make EXTRA_CFLAGS=-Wone-off-warning-that-I-am-testing Perhaps those are things the main Makefile should support. I dunno. You could see the full depths of my depravity at: https://github.com/peff/git/blame/meta/config/config.mak (I linked to the blame view because there are a lot of WTF bits in there that are explained by the commit messages). -Peff
Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s
On Tue, May 9, 2017 at 9:33 PM, Jeff King wrote: > On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote: > >> > Hmm. That makes sense generally, as the request should succeed. But it >> > seems like we're creating a client that will sometimes succeed and >> > sometimes fail, and the reasoning will be somewhat opaque to the user. >> > I have a feeling I'm missing some context on when you'd expect this to >> > kick in. >> >> Specifically, someone I know was looking at building an application >> that is passed only a SHA-1 for the tip of a ref on a popular hosting >> site[1]. They wanted to run `git fetch URL SHA1`, but this failed >> because the site doesn't have upload.allowtipsha1inwant enabled. >> However the SHA1 was clearly in the output of git ls-remote. > > OK. So this is basically a case where we expect that the user knows what > they're doing. > >> For various reasons they expected this to work, because it works >> against other sites that do have upload.allowtipsha1inwant enabled. >> And I personally just expected it to work because the fetch client >> accepts SHA-1s, and the wire protocol uses "want SHA1" not "want ref", >> and the SHA-1 passed on the command line was currently in the >> advertisement when the connection opened, so its certainly something >> the server is currently willing to serve. > > Right, makes sense. I wondered if GitHub should be turning on > allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide > some internal refs[1], and they're things that users wouldn't want to > fetch. The problem for your case really is just on the client side, and > this patch fixes it. > > Some of this context might be useful in the commit message. > > -Peff > > [1] The reachability checks from upload-pack don't actually do much on > GitHub, because you can generally access the objects via the API or > the web site anyway. So I'm not really opposed to turning on > allowTipSHA1InWant if it would be useful for users, but after > Jonathan's patch I don't see how it would be. Right, I agree. That is why we proposed this patch to the client, rather than a support request with GitHub to change server configuration. :)
Re: Script to rebase branches
On Tue, May 09, 2017 at 02:32:37PM +0200, Johannes Schindelin wrote: > > I didn't really expect anybody to use it verbatim, though. I was > > providing it more for inspiration. > > I deem it part of Git's mission is to avoid forcing everybody to write > scripts so specific to their own needs that they cannot be shared easily. Sure. I'd be happy if somebody used it as inspiration to make a tool for everybody, too. The two main reasons I don't polish and try to share the bits in my Meta/ more widely are: 1. Most of them are as much policy as they are implementation logic. So either you buy in completely to the worldview that I've assumed in my tools, or you end up fighting the tool (and by the time you make the tool configurable enough to handle all world-views, you haven't really helped anybody). I think the best thing to do with those logic bits is to add them as much as possible to Git itself (e.g., as command-line options). That keeps any personal scripts as thin wrappers that specify the policy. 2. Some of the features are really powerful but also really dangerous. For example, my "rebase" script (which rebases all my topics) and my "private" script (which builds my daily "private" version of Git to run) both write a shell snippet into $GIT_DIR/continue. And then I have a git-continue alias that looks like this: #!/bin/sh SUBDIRECTORY_OK=Yes . git-sh-setup cd_to_toplevel if test -f "$GIT_DIR/continue"; then eval "$(cat "$GIT_DIR/continue")" elif test -d "$GIT_DIR/rebase-merge"; then git rebase --continue elif test -d "$GIT_DIR/rebase-apply"; then if test -f "$GIT_DIR/rebase-apply/applying"; then git am --continue else git rebase --continue fi elif test -f "$GIT_DIR/CHERRY_PICK_HEAD"; then git cherry-pick --continue else echo >&2 "nothing to continue" exit 1 fi So when I run "git continue" it magically tries to pick up the operation in progress keep going. When it works, it works beautifully. But when it doesn't...well, you can dig yourself into a pretty confusing situation. It's worth it for me, because I can dig myself out. But I'm not sure it's something I'd encourage other people to use. -Peff
Re: Script to rebase branches
On Wed, May 10, 2017 at 07:47:26AM +0900, Junio C Hamano wrote: > > Yes, the script predates the invention of worktrees by several years. I > > have occasionally played with worktrees, but don't use them extensively > > (I'd usually use them for a one-off change, and then remove the > > worktree). > > I check out a different Meta/ at the top-level of my working tree > when working on Git, but I do use an equivalent of "worktree" to > have separate build areas for four integration branches. It is > trivial to check out Meta/ just once to the primary working tree and > symlink it to others ;-) Yeah, I guess I'd need to do that, too, if I used worktrees extensively. I think the specific problem with the rebase script is just that it expects to be able to checkout all the branches. > One thing that struck me odd about your "rebase" script was that it > didn't seem to have a special provision to handle a topic that > builds on another topic. I saw toposort, but is that sufficient? It topo-sorts so that a single run rebases everything (otherwise you may need to run N times, where N is the deepest dependency chain). But it also uses reflogs to try to find the fork point when the upstream topic has been rebased. The logic is in find_base(). Once upon a time it used "git pull --rebase", but there were some complications. These days I think it could probably use "rebase --fork-point", but I just never got around to testing it. -Peff
Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s
On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote: > > Hmm. That makes sense generally, as the request should succeed. But it > > seems like we're creating a client that will sometimes succeed and > > sometimes fail, and the reasoning will be somewhat opaque to the user. > > I have a feeling I'm missing some context on when you'd expect this to > > kick in. > > Specifically, someone I know was looking at building an application > that is passed only a SHA-1 for the tip of a ref on a popular hosting > site[1]. They wanted to run `git fetch URL SHA1`, but this failed > because the site doesn't have upload.allowtipsha1inwant enabled. > However the SHA1 was clearly in the output of git ls-remote. OK. So this is basically a case where we expect that the user knows what they're doing. > For various reasons they expected this to work, because it works > against other sites that do have upload.allowtipsha1inwant enabled. > And I personally just expected it to work because the fetch client > accepts SHA-1s, and the wire protocol uses "want SHA1" not "want ref", > and the SHA-1 passed on the command line was currently in the > advertisement when the connection opened, so its certainly something > the server is currently willing to serve. Right, makes sense. I wondered if GitHub should be turning on allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide some internal refs[1], and they're things that users wouldn't want to fetch. The problem for your case really is just on the client side, and this patch fixes it. Some of this context might be useful in the commit message. -Peff [1] The reachability checks from upload-pack don't actually do much on GitHub, because you can generally access the objects via the API or the web site anyway. So I'm not really opposed to turning on allowTipSHA1InWant if it would be useful for users, but after Jonathan's patch I don't see how it would be.
Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s
On Tue, May 9, 2017 at 3:16 PM, Jeff King wrote: > On Tue, May 09, 2017 at 11:20:42AM -0700, Jonathan Tan wrote: > >> fetch-pack, when fetching a literal SHA-1 from a server that is not >> configured with uploadpack.allowtipsha1inwant (or similar), always >> returns an error message of the form "Server does not allow request for >> unadvertised object %s". However, it is sometimes the case that such >> object is advertised. >> >> Teach fetch-pack to also check the SHA-1s of the refs in the received >> ref advertisement if a literal SHA-1 was given by the user. > > Hmm. That makes sense generally, as the request should succeed. But it > seems like we're creating a client that will sometimes succeed and > sometimes fail, and the reasoning will be somewhat opaque to the user. > I have a feeling I'm missing some context on when you'd expect this to > kick in. Specifically, someone I know was looking at building an application that is passed only a SHA-1 for the tip of a ref on a popular hosting site[1]. They wanted to run `git fetch URL SHA1`, but this failed because the site doesn't have upload.allowtipsha1inwant enabled. However the SHA1 was clearly in the output of git ls-remote. So a workaround is to do this in shell, which is just weird: r=$(git ls-remote $url | grep ^$sha1); if [ -n "$r" ]; then exec git fetch $url $r:refs/tmp/foo else echo >&2 "cannot find $sha1" exit 1 fi For various reasons they expected this to work, because it works against other sites that do have upload.allowtipsha1inwant enabled. And I personally just expected it to work because the fetch client accepts SHA-1s, and the wire protocol uses "want SHA1" not "want ref", and the SHA-1 passed on the command line was currently in the advertisement when the connection opened, so its certainly something the server is currently willing to serve. The application in question is using a SHA-1 rather than a ref name, because thats what it was given by something else. And the other thing basically wants this to fail-fast if it can't get that exact SHA-1. So to pass a ref name to git fetch the supplier would have to actually pass a tuple of ref+sha1, and then the fetcher has to check that the ref it just obtained provides the sha1 it expected. So this all turned into a bug report by me to Jonathan Tan, because git fetch violated my assumption of what it would do if I handed it a SHA-1 and the SHA-1 was currently the tip of a remote ref. [1] github.com
Schönen Tag
Hallo Ich bin Frau Victoria. G.Harry, ein Finanz-Manager für eine private Kreditvergabe Agentur, Uni Kredit Darlehen UK. Ich bin hier, um ein Darlehen Programm, das Ihnen helfen, Ihre finanzielle und wirtschaftliche Situation zu verbessern und entlasten Sie alle finanziellen Krise / Problem. Wir bieten kurz- und langfristige Darlehen an Kunden mit einer Rate von 3%, z.B. Auto Darlehen, persönliche Darlehen, Business-Darlehen (klein oder groß), etc. Wenn Sie daran interessiert sind, ein Darlehen von uns zu erhalten, können Sie uns über diese E-Mail kontaktieren: uni_cred...@outlook.com Auch Beratung zum Ausfüllen des Antragsformulars unten: Name: Sex: Land / Adresse Benötigte Menge: Dauer des Darlehens: Zweck: Telefon: Vielen Dank für Ihre Schirmherrschaft, Frau Victoria .G. Harry
Re: [BUG] :(attr) pathspecs can die("BUG") in the tree-walker
On Tue, May 09, 2017 at 03:52:19PM -0700, Brandon Williams wrote: > > $ git log HEAD -- ':(attr:-diff)' > > fatal: BUG:tree-walk.c:947: unsupported magic 40 > > > > Whoops. This is presumably ls-tree is protected, but I think we are > > missing a GUARD_PATHSPEC call somewhere. > > > > This isn't a huge deal, as the correct behavior is probably to die like > > ls-tree does, but we probably shouldn't be hitting BUG assertions as a > > general rule. > > The die("BUG: ..."); is from a GAURD_PATHSPEC call. What really needs > to happen is to update the magic_mask passed into the parse_pathspec > call which initializes the pathspec object. Its this magic_mask which > catches unsupported magic and prints a better message. Ah, right, I always get the pathspec safety bits mixed up. I think the parse_pathspec() call in setup_revisions is a bit permissive: parse_pathspec(&revs->prune_data, 0, 0, revs->prefix, prune_data.path); You _shouldn't_ need to audit all the callers when you add new pathspec magic. The callers should be masking out the bits that they know they support, but this one isn't. It looks like there are a number of such permissive calls, though. I guess it was an attempt to not have to list them manually at each call site (but then we suffer from the exact problem I ran into). > I guess this means I (or someone else :D) should audit all the > parse_pathspec calls and ensure that 'attr' magic is turned off until we > won't run into these GAURD_PATHSPEC die's. Of course it would be nicer still if the 'attr' magic actually worked via the tree-walking code. :) -Peff
Re: [ANNOUNCE] Git v2.12.3 and others
Junio C Hamano wrote: > Maintenance releases Git v2.4.12, v2.5.6, v2.6.7, v2.7.5, v2.8.5, > v2.9.4, v2.10.3, v2.11.2, and v2.12.3 have been tagged and are now > available at the usual places. > > These are primarily to fix a recently disclosed problem with "git > shell", which may allow a user who comes over SSH to run an > interactive pager by causing it to spawn "git upload-pack --help" > (CVE-2017-8386). Some (like v2.12.3) have other fixes that have > been accumulating included as well. > > "git-shell" is a restricted login shell that can be used on a server > to prevent SSH clients from running any programs except those needed > for git fetches and pushes. If you are not running a server, or if > your server has not been explicitly configured to use git-shell as a > login shell, you are not affected. Also note that sites running "git > shell" behind gitolite are NOT vulnerable. Thanks. Credit for discovering this bug goes to Timo Schmid, ERNW GmbH. They will probably have a blog post soon with more details. 1.6.1 is the earliest git version affected (so this goes back pretty far). > The tarballs are found at: > > https://www.kernel.org/pub/software/scm/git/ > > The following public repositories all have a copy of these tags: > > url = https://kernel.googlesource.com/pub/scm/git/git > url = git://repo.or.cz/alt-git.git > url = git://git.sourceforge.jp/gitroot/git-core/git.git > url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core > url = https://github.com/gitster/git Sincerely, Jonathan
[ANNOUNCE] Git v2.12.3 and others
Maintenance releases Git v2.4.12, v2.5.6, v2.6.7, v2.7.5, v2.8.5, v2.9.4, v2.10.3, v2.11.2, and v2.12.3 have been tagged and are now available at the usual places. These are primarily to fix a recently disclosed problem with "git shell", which may allow a user who comes over SSH to run an interactive pager by causing it to spawn "git upload-pack --help" (CVE-2017-8386). Some (like v2.12.3) have other fixes that have been accumulating included as well. "git-shell" is a restricted login shell that can be used on a server to prevent SSH clients from running any programs except those needed for git fetches and pushes. If you are not running a server, or if your server has not been explicitly configured to use git-shell as a login shell, you are not affected. Also note that sites running "git shell" behind gitolite are NOT vulnerable. The tarballs are found at: https://www.kernel.org/pub/software/scm/git/ The following public repositories all have a copy of these tags: url = https://kernel.googlesource.com/pub/scm/git/git url = git://repo.or.cz/alt-git.git url = git://git.sourceforge.jp/gitroot/git-core/git.git url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core url = https://github.com/gitster/git
[ANNOUNCE] Git v2.13.0
The latest feature release Git v2.13.0 is now available at the usual places. It is comprised of 729 non-merge commits since v2.12.0, contributed by 65 people, 15 of which are new faces. This release also contains the security patch in v2.12.3 and others to fix CVE-2017-8386. The tarballs are found at: https://www.kernel.org/pub/software/scm/git/ The following public repositories all have a copy of the 'v2.13.0' tag and the 'master' branch that the tag points at: url = https://kernel.googlesource.com/pub/scm/git/git url = git://repo.or.cz/alt-git.git url = git://git.sourceforge.jp/gitroot/git-core/git.git url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core url = https://github.com/gitster/git New contributors whose contributions weren't in v2.12.0 are as follows. Welcome to the Git development community! Allan Xavier, Andreas Heiduk, Devin J. Pohly, Devin Lehmacher, Hiroshi Shirosaki, Johan Hovold, Maxim Moseychuk, Mostyn Bramley-Moore, Prathamesh Chavan, Quentin Pradet, René Genz, Segev Finer, Sergey Ryazanov, Stephen Hicks, and Valery Tolstov. Returning contributors who helped this release are as follows. Thanks for your continued support. Ævar Arnfjörð Bjarmason, Alexander Shopov, Alex Henrie, Brandon Williams, brian m. carlson, Christian Couder, Cornelius Weig, David Aguilar, David Turner, Eric Wong, Giuseppe Bilotta, Jacob Keller, Jean-Noel Avila, Jeff Hostetler, Jeff King, Jiang Xin, Johannes Schindelin, Jonathan Nieder, Jonathan Tan, Jordi Mas, Junio C Hamano, Karthik Nayak, Kevin Willford, Kyle Meyer, Lars Schneider, Linus Torvalds, Luke Diamand, Matt McCutchen, Michael Haggerty, Michael J Gruber, Michael Rappazzo, Mike Hommey, Nguyễn Thái Ngọc Duy, Patrick Steinhardt, Peter Krefting, Ralf Thielow, Ramsay Jones, Ray Chen, René Scharfe, Ross Lagerwall, Santiago Torres, Sebastian Schuberth, Simon Ruderich, Stefan Beller, SZEDER Gábor, Thomas Gummerer, Torsten Bögershausen, Trần Ngọc Quân, Vasco Almeida, and Vegard Nossum. Git 2.13 Release Notes == Backward compatibility notes. * Use of an empty string as a pathspec element that is used for 'everything matches' is still warned and Git asks users to use a more explicit '.' for that instead. The hope is that existing users will not mind this change, and eventually the warning can be turned into a hard error, upgrading the deprecation into removal of this (mis)feature. That is not scheduled to happen in the upcoming release (yet). * The historical argument order "git merge HEAD ..." has been deprecated for quite some time, and is now removed. * The default location "~/.git-credential-cache/socket" for the socket used to communicate with the credential-cache daemon has been moved to "~/.cache/git/credential/socket". * Git now avoids blindly falling back to ".git" when the setup sequence said we are _not_ in Git repository. A corner case that happens to work right now may be broken by a call to die("BUG"). We've tried hard to locate such cases and fixed them, but there might still be cases that need to be addressed--bug reports are greatly appreciated. Updates since v2.12 --- UI, Workflows & Features * "git describe" and "git name-rev" have been taught to take more than one refname patterns to restrict the set of refs to base their naming output on, and also learned to take negative patterns to name refs not to be used for naming via their "--exclude" option. * Deletion of a branch "foo/bar" could remove .git/refs/heads/foo once there no longer is any other branch whose name begins with "foo/", but we didn't do so so far. Now we do. * When "git merge" detects a path that is renamed in one history while the other history deleted (or modified) it, it now reports both paths to help the user understand what is going on in the two histories being merged. * The part in "http.." configuration variable can now be spelled with '*' that serves as wildcard. E.g. "http.https://*.example.com.proxy"; can be used to specify the proxy used for https://a.example.com, https://b.example.com, etc., i.e. any host in the example.com domain. * "git tag" did not leave useful message when adding a new entry to reflog; this was left unnoticed for a long time because refs/tags/* doesn't keep reflog by default. * The "negative" pathspec feature was somewhat more cumbersome to use than necessary in that its short-hand used "!" which needed to be escaped from shells, and it required "exclude from what?" specified. * The command line options for ssh invocation needs to be tweaked for some implementations of SSH (e.g. PuTTY plink wants "-P " while OpenSSH wants "-p " to specify port to connect to), and the variant was guessed when GIT_SSH environment variable is used to specify it.
Re: [PATCH v2] travis-ci: retry if Git for Windows CI returns HTTP error 502 or 503
Lars Schneider writes: >>> It would be great if we could test this is a little bit in pu. >> >> This has been in 'pu' for a while. >> >> As the patch simply discards 502 (and others), it is unclear if the >> failing test on 'next' is now gone, or the attempt to run 'pu' >> happened to be lucky not to get one, from the output we can see in >> https://travis-ci.org/git/git/jobs/229867212 >> >> Are you comfortable enough to move this forward? > > Yes, please move it forward. I haven't seen a "502 - Web server > received an invalid response" on pu for a while. That means the > patch should work as expected. Will do, thanks. > Unrelated to this patch I have, however, seen two kinds of timeouts: > > (1) Timeout in the "notStarted" state. This job eventually finished > with a failure but it did start only *after* 3h: > https://travis-ci.org/git/git/jobs/230225611 > > (2) Timeout in the "in progress" state. This job eventually finished > successfully but it took longer than 3h: > https://travis-ci.org/git/git/jobs/229867248 > > Right now the timeout generates potential false negative results. > I would like to change that and respond with a successful build > *before* we approach the 3h timeout. This means we could generate > false positives. Although this is not ideal, I think that is the better > compromise as a failing Windows build would usually fail quickly > (e.g. in the compile step). > > What do you guys think? Would you be OK with that reasoning? > If the Git for Windows builds get more stable over time then > we could reevaluate this compromise. I'd rather see a false breakage on Windows build (i.e. "this might have succeeded given enough time, but it didn't finish within the alloted time") than a false sucess (i.e. "we successfully launched and the build is still running, so let's assume the test succeeds"). Because I do not pay attention to what the overall build page [*1*] says about a particular branch tip, and I instead look at the summary list of the indiviaul "Build Jobs", e.g. [*2*]), seeing errored/failed on [*1*] does not bother me personally, if that is what you are getting at. [References] *1* https://travis-ci.org/git/git/builds/ *2* https://travis-ci.org/git/git/builds/230235081
Re: [PATCH] fixup! use perl instead of sed
On Wed, May 10, 2017 at 12:38 AM, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > >> On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan >> wrote: >> ... >>> # Tweak the push output to make the push option outside the cert >>> # different, then replay it on a fresh dst, checking that ff is not >>> # deleted. >>> - sed -i "s/\\([^ ]\\)bar/\\1baz/" push && >>> + perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak && >>> prepare_dst && >>> git -C dst config receive.certnonceseed sekrit && >>> git -C dst config receive.advertisepushoptions 1 && >>> - git receive-pack dst out && >>> + git receive-pack dst out && >> >> The test should have a PERL prerequisite now, that's missing. > > For a single-liner like this, our stance has always been that t/ > scripts can assume _some_ version of Perl interpreter available for > use, cf. t/README where it lists prerequisites and explains them: > > - PERL > >Git wasn't compiled with NO_PERL=YesPlease. > >Even without the PERL prerequisite, tests can assume there is a >usable perl interpreter at $PERL_PATH, though it need not be >particularly modern. > > So unless "receive-pack" that is being tested here requires Perl at > runtime, we do not want PERL prerequisite for this test. Oops, sorry about that. >> Also using \1 will likely be deprecated in future versions of perl at >> some point: >> >> $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/" >> \1 better written as $1 at -e line 1. >> hifoo > > Very good advice from a Perl expert; thanks. No problem. Hopefully my noisy advice will at least lead to fixing a bug in perl if there is one :)
Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning
Ramsay Jones writes: > Yeah, I had a similar comment in the commit message (but much more > verbose than your concise addition above), but I edited it several > times, without finding a wording that I liked. I eventually removed > it, because it didn't really add any value. :( I tend to agree that the proposed additional comment does not add much value. It assures the readers that we (at the time of applying this patch) know that the earlier use of ULL was not done with a good reason but was merely an accident, and strengthens the claim that this is a good change, but the correctness of the change is already obvious, and the readers would understand without being explained where the incorrectness we have to fix with this patch came from, I would think. .
Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning
Ramsay Jones writes: > In a similar vein, on systems which use a 64-bit representation of the > 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with > the value 0777ULL. Although this does not cause any warning > messages to be issued, it would be more appropriate for this constant > to use an 'UL' type suffix rather than 'ULL'. ... it is more appropriate because we know the recipient is "unsigned long", not "unsigned long long", in this case? As opposed to the case of timestamp_t, which is opaque and could be "unsigned long long"? That makes sense to me, even though it took a bit of thinking aloud to understand. Looks good; thanks.
Re: [PATCH v2 00/21]
Duy Nguyen writes: > On Sun, May 7, 2017 at 11:20 AM, Junio C Hamano wrote: >> On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano wrote: >>> >>> Nguyễn Thái Ngọc Duy writes: >>> >>> > Changes since v1: >>> > >>> > - fopen_or_warn() and warn_on_fopen_errors() are introduced. The >>> >latter's name is probably not great... >>> > - A new patch (first one) to convert a bunch to using xfopen() >>> > - Fix test failure on Windows, found by Johannes Sixt >>> > - Fix the memory leak in log.c, found by Jeff >>> > >>> > There are still a couple of fopen() remained, but I'm getting slow >>> > again so let's get these in first and worry about the rest when >>> > somebody has time. >> >> Hmm, is this topic responsible for recent breakage Travis claims on MacOS X? >> >> https://travis-ci.org/git/git/jobs/229585098#L3091 >> >> seems to expect an error when test-config is fed a-directory but we are >> not getting the expected warning and error? > > Sounds about right. Let me see if defining FREAD_READS_DIRECTORIES on > MacOS X makes travis happy. Thanks. I should have suspected that myself to save a round-trip.
Re: [PATCH v4 11/25] checkout: fix memory leak
Johannes Schindelin writes: >> > A leak is better than a use after free, so >> > let's be extra careful here. Would it leave the index inconsistent? Or >> > perhaps freeing it has become safe in the meantime? >> > >> > @Junio: Do you remember the reason for the leaks in 0cf8581e330 >> > (checkout -m: recreate merge when checking out of unmerged index). >> >> Yes. >> >> In the very old days it was not allowed to free(3) contents of >> active_cache[] and this was an old brain fart that came out of >> inertia. We are manufacturing a brand new ce, only to feed it to >> checkout_entry() without even registering it to the active_cache[] >> array, and the ancient rule doesn't even apply to such a case. >> >> So I think it was safe to free(3) even back then. > > So this patch is good, then? Unless I from this year is failing to spot a breakage that will be caused in the codepath that I from year 2008 and René spotted, I think freeing ce after we are done updating the working tree file with it is safe. I'd need to find time to make sure, though. >> > And result_buf is still leaked here, right? >> >> Good spotting. It typically would make a larger leak than a single >> ce, I would suppose ;-) > > I saw you added this as a fixup! commit. If you don't mind, and if no > other changes are requested, would you mind rebase'ing this yourself? I think it would be better to leave the fix as a separate patch. It wasn't spotted by Coverity in the first place ;-)
Re: [BUG] :(attr) pathspecs can die("BUG") in the tree-walker
On 05/09, Jeff King wrote: > I was playing with the new :(attr) pathspecs in the upcoming v2.13 > today, and noticed: > > $ git ls-files -- ':(attr:-diff)' > t/t0110/url-1 > t/t0110/url-10 > [etc] > > So far so good. > > $ git ls-tree HEAD -- ':(attr:-diff)' > fatal: :(attr:-diff): pathspec magic not supported by this command: 'attr' > > Bummer, but I understand that sometimes the options need to be plumbed > through to work everywhere. > > $ git log HEAD -- ':(attr:-diff)' > fatal: BUG:tree-walk.c:947: unsupported magic 40 > > Whoops. This is presumably ls-tree is protected, but I think we are > missing a GUARD_PATHSPEC call somewhere. > > This isn't a huge deal, as the correct behavior is probably to die like > ls-tree does, but we probably shouldn't be hitting BUG assertions as a > general rule. > > -Peff The die("BUG: ..."); is from a GAURD_PATHSPEC call. What really needs to happen is to update the magic_mask passed into the parse_pathspec call which initializes the pathspec object. Its this magic_mask which catches unsupported magic and prints a better message. I guess this means I (or someone else :D) should audit all the parse_pathspec calls and ensure that 'attr' magic is turned off until we won't run into these GAURD_PATHSPEC die's. -- Brandon Williams
Re: Script to rebase branches
Jeff King writes: >> That requires Meta/ to be checked out and up-to-date. I'd bet there are >> exactly two people who fall into that category. > > Actually, it is not Junio's Meta that needs checked out, but rather the > "meta" branch where you will find that "rebase" script. If other people > find them useful, the set of scripts could perhaps be transitioned to a > namespace that is appropriate to go into people's $PATH. > > I didn't really expect anybody to use it verbatim, though. I was > providing it more for inspiration. > >> Also, I see that you do not use worktrees. Otherwise your script would >> fail. > > Yes, the script predates the invention of worktrees by several years. I > have occasionally played with worktrees, but don't use them extensively > (I'd usually use them for a one-off change, and then remove the > worktree). I check out a different Meta/ at the top-level of my working tree when working on Git, but I do use an equivalent of "worktree" to have separate build areas for four integration branches. It is trivial to check out Meta/ just once to the primary working tree and symlink it to others ;-) One thing that struck me odd about your "rebase" script was that it didn't seem to have a special provision to handle a topic that builds on another topic. I saw toposort, but is that sufficient?
Re: [PATCH] fixup! use perl instead of sed
Ævar Arnfjörð Bjarmason writes: > On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan wrote: > ... >> # Tweak the push output to make the push option outside the cert >> # different, then replay it on a fresh dst, checking that ff is not >> # deleted. >> - sed -i "s/\\([^ ]\\)bar/\\1baz/" push && >> + perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak && >> prepare_dst && >> git -C dst config receive.certnonceseed sekrit && >> git -C dst config receive.advertisepushoptions 1 && >> - git receive-pack dst out && >> + git receive-pack dst out && > > The test should have a PERL prerequisite now, that's missing. For a single-liner like this, our stance has always been that t/ scripts can assume _some_ version of Perl interpreter available for use, cf. t/README where it lists prerequisites and explains them: - PERL Git wasn't compiled with NO_PERL=YesPlease. Even without the PERL prerequisite, tests can assume there is a usable perl interpreter at $PERL_PATH, though it need not be particularly modern. So unless "receive-pack" that is being tested here requires Perl at runtime, we do not want PERL prerequisite for this test. > Also using \1 will likely be deprecated in future versions of perl at > some point: > > $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/" > \1 better written as $1 at -e line 1. > hifoo Very good advice from a Perl expert; thanks.
[BUG] :(attr) pathspecs can die("BUG") in the tree-walker
I was playing with the new :(attr) pathspecs in the upcoming v2.13 today, and noticed: $ git ls-files -- ':(attr:-diff)' t/t0110/url-1 t/t0110/url-10 [etc] So far so good. $ git ls-tree HEAD -- ':(attr:-diff)' fatal: :(attr:-diff): pathspec magic not supported by this command: 'attr' Bummer, but I understand that sometimes the options need to be plumbed through to work everywhere. $ git log HEAD -- ':(attr:-diff)' fatal: BUG:tree-walk.c:947: unsupported magic 40 Whoops. This is presumably ls-tree is protected, but I think we are missing a GUARD_PATHSPEC call somewhere. This isn't a huge deal, as the correct behavior is probably to die like ls-tree does, but we probably shouldn't be hitting BUG assertions as a general rule. -Peff
Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s
On Tue, May 09, 2017 at 11:20:42AM -0700, Jonathan Tan wrote: > fetch-pack, when fetching a literal SHA-1 from a server that is not > configured with uploadpack.allowtipsha1inwant (or similar), always > returns an error message of the form "Server does not allow request for > unadvertised object %s". However, it is sometimes the case that such > object is advertised. > > Teach fetch-pack to also check the SHA-1s of the refs in the received > ref advertisement if a literal SHA-1 was given by the user. Hmm. That makes sense generally, as the request should succeed. But it seems like we're creating a client that will sometimes succeed and sometimes fail, and the reasoning will be somewhat opaque to the user. I have a feeling I'm missing some context on when you'd expect this to kick in. > +static int is_literal_sha1(const struct ref *ref) > +{ > + struct object_id oid; > + return !get_oid_hex(ref->name, &oid) && > +!ref->name[40] && > +!oidcmp(&oid, &ref->old_oid); > +} I think the preferred method these days is to avoid the bare "40": struct object_oid oid; const char *end; return !parse_oid_hex(ref->name, &oid, &end) && !*end && !oidcmp(&oid, &ref->old_oid); I was confused at first why we need this oidcmp() and the one below. But this one is checking "does the name parse to itself", and the other is checking "does this parse to our sought ref?". So both checks are needed. > + for (i = 0; i < nr_sought; i++) { > + struct ref *s = sought[i]; > + if (!strcmp(ref->name, s->name) || > + (is_literal_sha1(s) && > + !oidcmp(&ref->old_oid, &s->old_oid))) { > + keep = 1; > + s->match_status = REF_MATCHED; > } > - i++; > } This will reparse ref->name as an oid via is_literal_sha1() for each pass through the loop. Should it be hoisted out? Maybe that is just premature optimization, though. Other than those minor nits, the code itself looks fine to me. -Peff
Re: [PATCH v3 00/53] object_id part 8
On 05/06, brian m. carlson wrote: > This is the eighth series of patches to convert unsigned char [20] to > struct object_id. This series converts lookup_commit, lookup_blob, > lookup_tree, lookup_tag, and finally parse_object to struct object_id. > > A small number of functions have temporaries inserted during the > conversion in order to allow conversion of functions that still need to > take unsigned char *; they are removed either later in the series or > will be in a future series. > > This series can be fetched from the object-id-part8 branch from either > of the follwing: > > https://github.com/bk2204/git > https://git.crustytoothpaste.net/git/bmc/git.git > > Changes from v2: > * Remove spurious space after ampersand. > * Undo more needless line rewrapping. > * Expand computation for notes path. > * Remove check for line->len with parse_oid_hex. > > Changes from v1: > * Rebase on master. This led to a conflict with the ref-cache changes in > patch > 39. Extra-careful review here would be welcome. > * Undo the needless line rewrapping. > * Fix the commit message typo. > * Use GIT_MAX_RAWSZ instead of struct object_id for the pack checksum. I wasn't able to find any issues with v3. Looks good. -- Brandon Williams
RE: git client debug with customer ssh client
On May 5, 2017 7:50 AM Pierre J. Ludwick wrote: > How can we get more info from git client? Any helps suggestions welcomed? It might be helpful to put a full trace in OpenSSH. Running ssh with -vvv should give you a lot of noise. I have used https://en.wikibooks.org/wiki/OpenSSH/Logging_and_Troubleshooting to pull information when the platform's OpenSSH port was done. If you need access to the ported code, the sources are available in the usual spot at ITUGLIB. Cheers, Randall
Re: [PATCH] fixup! use perl instead of sed
On Tue, May 9, 2017 at 10:43 PM, Johannes Sixt wrote: > Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason: >> >> Finally, you can just use -i like you did with sed, no need for the >> tempfile: > > > Nope. Some implementations of perl attempt to remove the file that it has > just opened. That doesn't work on Windows. You have to supply a backup file > name as in `perl -i.bak ...` :-( This should have been fixed in 2002, and is in 5.8, the oldest perl release we support: https://github.com/Perl/perl5/commit/c030f24b81 & http://www.nntp.perl.org/group/perl.perl5.porters/2002/05/msg60275.html But maybe __CYGWIN__ isn't always defined on Windows, maybe this was a mingw build or something and perl was missing a test for this when support for that was added? This is obviously a trivial issue for git, but if it's a bug in perl I'd like to fix that. >> >> $ echo hibar >push >> $ perl -pi -e 's/([^ ])bar/$1baz/' push >> $ cat push >> hibaz > > > -- Hannes
Re: [PATCH] fixup! use perl instead of sed
On 05/09/2017 01:43 PM, Johannes Sixt wrote: Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason: Finally, you can just use -i like you did with sed, no need for the tempfile: Nope. Some implementations of perl attempt to remove the file that it has just opened. That doesn't work on Windows. You have to supply a backup file name as in `perl -i.bak ...` :-( $ echo hibar >push $ perl -pi -e 's/([^ ])bar/$1baz/' push $ cat push hibaz -- Hannes Thanks - sent a fixup [1] in reply to my PATCH v3 cover letter (but forgot to CC you). [1] <20170509210158.17898-1-jonathanta...@google.com>
[PATCH v3] fixup! don't use perl -i because it's not portable
Signed-off-by: Jonathan Tan --- Ah...thanks, Johannes, for spotting this. Here's a fixup. t/t5534-push-signed.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh index 177b933a7..807267b7f 100755 --- a/t/t5534-push-signed.sh +++ b/t/t5534-push-signed.sh @@ -152,11 +152,11 @@ test_expect_success GPG,PERL 'inconsistent push options in signed push not allow # Tweak the push output to make the push option outside the cert # different, then replay it on a fresh dst, checking that ff is not # deleted. - perl -pi -e "s/([^ ])bar/\$1baz/" push && + perl -pe "s/([^ ])bar/\$1baz/" push >push.tweak && prepare_dst && git -C dst config receive.certnonceseed sekrit && git -C dst config receive.advertisepushoptions 1 && - git receive-pack dst out && + git receive-pack dst out && git -C dst rev-parse ff && grep "inconsistent push options" out ' -- 2.13.0.rc2.291.g57267f2277-goog
Re: [PATCH] fixup! use perl instead of sed
Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason: Finally, you can just use -i like you did with sed, no need for the tempfile: Nope. Some implementations of perl attempt to remove the file that it has just opened. That doesn't work on Windows. You have to supply a backup file name as in `perl -i.bak ...` :-( $ echo hibar >push $ perl -pi -e 's/([^ ])bar/$1baz/' push $ cat push hibaz -- Hannes
Re: [noob] is this normal behavior
Hi Harry, Both behaviours you report are normal, specifically: On 09/05/2017 15:02, Harry Putnam wrote: > Shouldn't files that changed but are already being tracked just show > up as modified and not need adding? > ... > Since that file is already being tracked; shouldn't `git status' show > that file as modified but ready to be committed instead of a file that > is modified but needs to be added before a commit can happen? No, it shouldn`t - even though the file you`ve modified is already being tracked, it doesn`t have to mean you want the modification inside your very next commit, and Git doesn`t force that upon you. That is where index (or "staging area") comes in handy, allowing you to `git add` only the changes you actually want to commit now, leaving the others for later decision. You don`t even have to add all the modifications inside the single file at once, for example by using `git add --patch`[1] you can select just some of the them, fine tuning what gets committed and when. With some precaution/steps needed not to commit broken project states by accident, and some discipline not to overdo it, this allows you to fully focus on the actual work you do, making logically unrelated changes in place as you see fit, on the go, and only later organizing them into logically grouped commits, through diligent use of `git add`. > Another side of this is that a `git diff FILE' only works before an > `git add .' operation is done. > > That is, if I run `git diff FILE' AFTER `git add' .. no diff is > reported, even though it is not committed yet. > > So, for example: if I'm committing and in the vi buffer of the commit > and want to see a diff of FILE to aid my log notes. > > git diff FILE will report nothing whatever. > > Is that expected behavior? Yes, that is as expected - in the form you`ve given, `git diff` shows the differences between your working tree and index (staging area), so only changes you haven`t added yet. Once you `git add` the changes from the working to the index, there are no more differences to show, so no diff is produced. If you want to see the added changes, what will be included in the commit if you make it, you can use `git diff --cached`, as per git-diff[2] documentation (--staged can also be used instead, being a synonym for --cached, but maybe easier to remember, relating it to "staging area"). That option shows differences between the staging area (index) and the specific commit - with none provided, it implies your currently checked-out position (referred to as HEAD), usually being your latest/previous commit, which is exactly what you`re interested in. As a side note, if you think you don`t need it, you can skip staging area and commit all the modifications/deletions without a need of adding them first by using `git commit --all`, as per git-commit[3] documentation. Just pay attention that untracked files are not affected, you still need to add them first to tell Git to start tracking them, including them in the next commit, but that seems to align nicely with your expectations already. I personally find the staging area to be one of the greatest Git possibilities, but I do understand beginners getting confused by it, as admittedly I once was myself. In the end, you may want to ask questions like this on Git users mailing list[4] on Google Groups, being a a nice place for beginners to get answers to their concerns. [1] https://git-scm.com/docs/git-add [2] https://git-scm.com/docs/git-diff [3] https://git-scm.com/docs/git-commit [4] https://groups.google.com/forum/?fromgroups#!forum/git-users Regards, Buga
Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning
On 09/05/17 11:24, Johannes Schindelin wrote: > Hi Ramsay, > > On Mon, 8 May 2017, Ramsay Jones wrote: > >> Commit dddbad728c ("timestamp_t: a new data type for timestamps", >> 26-04-2017) introduced a new typedef 'timestamp_t', as a synonym for an >> unsigned long, which was used at the time to represent timestamps in >> git. A later commit 28f4aee3fb ("use uintmax_t for timestamps", >> 26-04-2017) changed the typedef to use an 'uintmax_t' for the timestamp >> representation type. >> >> When building on a 32-bit Linux system, sparse complains that a constant >> (USTAR_MAX_MTIME) used to detect a 'far-future mtime' timestamp, is too >> large; 'warning: constant 0777UL is so big it is unsigned long >> long' on lines 335 and 338 of archive-tar.c. Note that both gcc and >> clang only issue a warning if this constant is used in a context that >> requires an 'unsigned long' (rather than an uintmax_t). (Since TIME_MAX >> is no longer equal to 0x, even on a 32-bit system, the macro >> USTAR_MAX_MTIME is set to 0777UL, which cannot be represented as >> an 'unsigned long' constant). >> >> In order to suppress the warning, change the definition of the macro >> constant USTAR_MAX_MTIME to use an 'ULL' type suffix. >> >> In a similar vein, on systems which use a 64-bit representation of the >> 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with >> the value 0777ULL. Although this does not cause any warning >> messages to be issued, it would be more appropriate for this constant >> to use an 'UL' type suffix rather than 'ULL'. > > The reason for the current situation is that an earlier fix for > the USTAR_MAX_MTIME constant was applied to the USTAR_MAX_SIZE > constant by mistake. Yeah, I had a similar comment in the commit message (but much more verbose than your concise addition above), but I edited it several times, without finding a wording that I liked. I eventually removed it, because it didn't really add any value. :( >> Signed-off-by: Ramsay Jones > > With that addition to the commit message: ACK This patch is now in the 'next' branch, so I guess it's too late to add this to the commit message (which I would be quite happy to do). Well, at the beginning of the next cycle, 'next' will be rebuilt, so I guess (if we remember!) this patch could be updated then. ATB, Ramsay Jones
Re: How to `git status' without scrambling modified with new, etc
Samuel Lijin writes: > You can use `git status -s` and match on the modification type (M > corresponds to modified, A to new files). See the man page for more > details on the interface. ahh yes. Just the ticket thanks
[PATCH v3 2/2] receive-pack: verify push options in cert
In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack was taught to include push options both within the signed cert (if the push is a signed push) and outside the signed cert; however, receive-pack ignores push options within the cert, only handling push options outside the cert. Teach receive-pack, in the case that push options are provided for a signed push, to verify that the push options both within the cert and outside the cert are consistent. This sets in stone the requirement that send-pack redundantly send its push options in 2 places, but I think that this is better than the alternatives. Sending push options only within the cert is backwards-incompatible with existing Git servers (which read push options only from outside the cert), and sending push options only outside the cert means that the push options are not signed for. Signed-off-by: Jonathan Tan --- Documentation/technical/pack-protocol.txt | 32 +++ builtin/receive-pack.c| 51 +-- t/t5534-push-signed.sh| 37 ++ 3 files changed, 112 insertions(+), 8 deletions(-) diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index 5b0ba3ef2..a34917153 100644 --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the obj-id currently on the server, the obj-id the client would like to update it to and the name of the reference. -This list is followed by a flush-pkt. Then the push options are transmitted -one per packet followed by another flush-pkt. After that the packfile that -should contain all the objects that the server will need to complete the new -references will be sent. +This list is followed by a flush-pkt. - update-request= *shallow ( command-list | push-cert ) [packfile] + update-requests = *shallow ( command-list | push-cert ) shallow = PKT-LINE("shallow" SP obj-id) @@ -500,12 +497,35 @@ references will be sent. PKT-LINE("pusher" SP ident LF) PKT-LINE("pushee" SP url LF) PKT-LINE("nonce" SP nonce LF) + *PKT-LINE("push-option" SP push-option LF) PKT-LINE(LF) *PKT-LINE(command LF) *PKT-LINE(gpg-signature-lines LF) PKT-LINE("push-cert-end" LF) - packfile = "PACK" 28*(OCTET) + push-option = 1*( VCHAR | SP ) + + +If the server has advertised the 'push-options' capability and the client has +specified 'push-options' as part of the capability list above, the client then +sends its push options followed by a flush-pkt. + + + push-options = *PKT-LINE(push-option) flush-pkt + + +For backwards compatibility with older Git servers, if the client sends a push +cert and push options, it MUST send its push options both embedded within the +push cert and after the push cert. (Note that the push options within the cert +are prefixed, but the push options after the cert are not.) Both these lists +MUST be the same, modulo the prefix. + +After that the packfile that +should contain all the objects that the server will need to complete the new +references will be sent. + + + packfile = "PACK" 28*(OCTET) If the receiving end does not support delete-refs, the sending end MUST diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index f96834f42..fff5c7a54 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -473,7 +473,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp) * after dropping "_commit" from its name and possibly moving it out * of commit.c */ -static char *find_header(const char *msg, size_t len, const char *key) +static char *find_header(const char *msg, size_t len, const char *key, +const char **next_line) { int key_len = strlen(key); const char *line = msg; @@ -486,6 +487,8 @@ static char *find_header(const char *msg, size_t len, const char *key) if (line + key_len < eol && !memcmp(line, key, key_len) && line[key_len] == ' ') { int offset = key_len + 1; + if (next_line) + *next_line = *eol ? eol + 1 : eol; return xmemdupz(line + offset, (eol - line) - offset); } line = *eol ? eol + 1 : NULL; @@ -495,7 +498,7 @@ static char *find_header(const char *msg, size_t len, const char *key) static const char *check_nonce(const char *buf, size_t len) { - char *nonce = find_header(buf, len, "nonce"); + char *nonce = find_header(buf, len, "nonce", NULL); unsigned long stamp, ostamp; char *bohmac, *expect = NU
[PATCH v3 0/2] Clarify interaction between signed pushes and push options
Changes from v2: - incorporated Junio's suggestions - incorporated Ævar's suggestions Jonathan Tan (2): docs: correct receive.advertisePushOptions default receive-pack: verify push options in cert Documentation/config.txt | 5 ++- Documentation/technical/pack-protocol.txt | 32 +++ builtin/receive-pack.c| 51 +-- t/t5534-push-signed.sh| 37 ++ 4 files changed, 114 insertions(+), 11 deletions(-) -- 2.13.0.rc2.291.g57267f2277-goog
[PATCH v3 1/2] docs: correct receive.advertisePushOptions default
In commit c714e45 ("receive-pack: implement advertising and receiving push options", 2016-07-14), receive-pack was taught to (among other things) advertise that it understood push options, depending on configuration. It was documented that it advertised such ability by default; however, it actually does not. (In that commit, notice that advertise_push_options defaults to 0, unlike advertise_atomic_push which defaults to 1.) Update the documentation to state that it does not advertise the ability by default. Signed-off-by: Jonathan Tan --- Documentation/config.txt | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 475e874d5..3a847a62e 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2620,9 +2620,8 @@ receive.advertiseAtomic:: capability, set this variable to false. receive.advertisePushOptions:: - By default, git-receive-pack will advertise the push options - capability to its clients. If you don't want to advertise this - capability, set this variable to false. + When set to true, git-receive-pack will advertise the push options + capability to its clients. False by default. receive.autogc:: By default, git-receive-pack will run "git-gc --auto" after -- 2.13.0.rc2.291.g57267f2277-goog
[PATCH 3/8] pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag
It's confusing to have two different 'strip submodule slash' flags which do subtly different things. Both PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP will accomplish the same task of striping a slash from a pathspec which matches a submodule entry in the index. The only difference is that PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE will perform additional checks and die if a pathspec has a leading path component which corresponds to a submodule. This additional functionality should be split out into its own flag. To this end, rename the PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to PATHSPEC_SUBMODULE_LEADING_PATH and change its behavior to only die if a path descends into a submodule. In addition add the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to callers which relied on the old slash stripping functionality. Signed-off-by: Brandon Williams --- builtin/add.c | 3 ++- builtin/check-ignore.c | 3 ++- pathspec.c | 32 pathspec.h | 9 +++-- 4 files changed, 23 insertions(+), 24 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index ec58e3679..2aa9aeab9 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -389,7 +389,8 @@ int cmd_add(int argc, const char **argv, const char *prefix) parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_FULL | PATHSPEC_SYMLINK_LEADING_PATH | - PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP | + PATHSPEC_SUBMODULE_LEADING_PATH, prefix, argv); if (add_new_files) { diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c index 1d73d3ca3..73237b2b1 100644 --- a/builtin/check-ignore.c +++ b/builtin/check-ignore.c @@ -87,7 +87,8 @@ static int check_ignore(struct dir_struct *dir, parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP, PATHSPEC_SYMLINK_LEADING_PATH | - PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE | + PATHSPEC_SUBMODULE_LEADING_PATH | + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP | PATHSPEC_KEEP_ORDER, prefix, argv); diff --git a/pathspec.c b/pathspec.c index e37256c3b..f37d5769d 100644 --- a/pathspec.c +++ b/pathspec.c @@ -398,29 +398,29 @@ static void strip_submodule_slash_cheap(struct pathspec_item *item) } } -static void strip_submodule_slash_expensive(struct pathspec_item *item) +static void die_path_inside_submodule(const struct pathspec_item *item, + const struct index_state *istate) { int i; - for (i = 0; i < active_nr; i++) { - struct cache_entry *ce = active_cache[i]; + for (i = 0; i < istate->cache_nr; i++) { + struct cache_entry *ce = istate->cache[i]; int ce_len = ce_namelen(ce); if (!S_ISGITLINK(ce->ce_mode)) continue; - if (item->len <= ce_len || item->match[ce_len] != '/' || - memcmp(ce->name, item->match, ce_len)) + if (item->len <= ce_len) + continue; + if (item->match[ce_len] != '/') + continue; + if (strncmp(ce->name, item->match, ce_len)) + continue; + if (item->len == ce_len + 1) continue; - if (item->len == ce_len + 1) { - /* strip trailing slash */ - item->len--; - item->match[item->len] = '\0'; - } else { - die(_("Pathspec '%s' is in submodule '%.*s'"), - item->original, ce_len, ce->name); - } + die(_("Pathspec '%s' is in submodule '%.*s'"), + item->original, ce_len, ce->name); } } @@ -499,9 +499,6 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags, if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) strip_submodule_slash_cheap(item); - if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE) - strip_submodule_slash_expensive(item); - if (magic & PATHSPEC_LITERAL) { item->nowildcard_len = item->len; } else { @@ -639,6 +636,9 @@ void parse_pathspec(struct pathspec *pathspec, die(_("pathspec '%s' is beyond a symbolic link"), entry); } + if (flags & PATHSPEC_SUBMODULE_LEADING_PATH) + die_path_inside_submodule(item + i, &the_index); + if (item[i].nowildcard_len < item[i].len) pathspec->has_wildcard = 1; pathspec->magic |= item[i].magic
[PATCH 5/8] pathspec: convert strip_submodule_slash to take an index
Signed-off-by: Brandon Williams --- pathspec.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pathspec.c b/pathspec.c index a1297ca02..cff069536 100644 --- a/pathspec.c +++ b/pathspec.c @@ -386,12 +386,13 @@ static const char *parse_element_magic(unsigned *magic, int *prefix_len, return parse_short_magic(magic, elem); } -static void strip_submodule_slash(struct pathspec_item *item) +static void strip_submodule_slash(struct pathspec_item *item, + const struct index_state *istate) { if (item->len >= 1 && item->match[item->len - 1] == '/') { - int i = cache_name_pos(item->match, item->len - 1); + int i = index_name_pos(istate, item->match, item->len - 1); - if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) { + if (i >= 0 && S_ISGITLINK(istate->cache[i]->ce_mode)) { item->len--; item->match[item->len] = '\0'; } @@ -497,7 +498,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags, } if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH) - strip_submodule_slash(item); + strip_submodule_slash(item, &the_index); if (magic & PATHSPEC_LITERAL) { item->nowildcard_len = item->len; -- 2.13.0.rc2.291.g57267f2277-goog
[PATCH 6/8] pathspec: convert find_pathspecs_matching_against_index to take an index
Signed-off-by: Brandon Williams --- builtin/add.c | 4 ++-- builtin/check-ignore.c | 2 +- pathspec.c | 10 ++ pathspec.h | 7 +-- 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index 51d7a5506..4e3bf20c2 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -136,7 +136,7 @@ static char *prune_directory(struct dir_struct *dir, struct pathspec *pathspec, *dst++ = entry; } dir->nr = dst - dir->entries; - add_pathspec_matches_against_index(pathspec, seen); + add_pathspec_matches_against_index(pathspec, &the_index, seen); return seen; } @@ -418,7 +418,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) int i; if (!seen) - seen = find_pathspecs_matching_against_index(&pathspec); + seen = find_pathspecs_matching_against_index(&pathspec, &the_index); /* * file_exists() assumes exact match diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c index 694e4c61b..446b76dcf 100644 --- a/builtin/check-ignore.c +++ b/builtin/check-ignore.c @@ -97,7 +97,7 @@ static int check_ignore(struct dir_struct *dir, * should not be ignored, in order to be consistent with * 'git status', 'git add' etc. */ - seen = find_pathspecs_matching_against_index(&pathspec); + seen = find_pathspecs_matching_against_index(&pathspec, &the_index); for (i = 0; i < pathspec.nr; i++) { full_path = pathspec.items[i].match; exclude = NULL; diff --git a/pathspec.c b/pathspec.c index cff069536..bbd71b48b 100644 --- a/pathspec.c +++ b/pathspec.c @@ -17,6 +17,7 @@ * to use find_pathspecs_matching_against_index() instead. */ void add_pathspec_matches_against_index(const struct pathspec *pathspec, + const struct index_state *istate, char *seen) { int num_unmatched = 0, i; @@ -32,8 +33,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec, num_unmatched++; if (!num_unmatched) return; - for (i = 0; i < active_nr; i++) { - const struct cache_entry *ce = active_cache[i]; + for (i = 0; i < istate->cache_nr; i++) { + const struct cache_entry *ce = istate->cache[i]; ce_path_match(ce, pathspec, seen); } } @@ -46,10 +47,11 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec, * nature of the "closest" (i.e. most specific) matches which each of the * given pathspecs achieves against all items in the index. */ -char *find_pathspecs_matching_against_index(const struct pathspec *pathspec) +char *find_pathspecs_matching_against_index(const struct pathspec *pathspec, + const struct index_state *istate) { char *seen = xcalloc(pathspec->nr, 1); - add_pathspec_matches_against_index(pathspec, seen); + add_pathspec_matches_against_index(pathspec, istate, seen); return seen; } diff --git a/pathspec.h b/pathspec.h index dd6456d5d..dfb22440a 100644 --- a/pathspec.h +++ b/pathspec.h @@ -103,7 +103,10 @@ static inline int ps_strcmp(const struct pathspec_item *item, return strcmp(s1, s2); } -extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec); -extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen); +extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, + const struct index_state *istate, + char *seen); +extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec, + const struct index_state *istate); #endif /* PATHSPEC_H */ -- 2.13.0.rc2.291.g57267f2277-goog
[PATCH 7/8] pathspec: convert init_pathspec_item to take an index
Signed-off-by: Brandon Williams --- pathspec.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pathspec.c b/pathspec.c index bbd71b48b..c7ed8b3fb 100644 --- a/pathspec.c +++ b/pathspec.c @@ -430,7 +430,9 @@ static void die_path_inside_submodule(const struct pathspec_item *item, /* * Perform the initialization of a pathspec_item based on a pathspec element. */ -static void init_pathspec_item(struct pathspec_item *item, unsigned flags, +static void init_pathspec_item(struct pathspec_item *item, + const struct index_state *istate, + unsigned flags, const char *prefix, int prefixlen, const char *elt) { @@ -500,7 +502,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags, } if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH) - strip_submodule_slash(item, &the_index); + strip_submodule_slash(item, istate); if (magic & PATHSPEC_LITERAL) { item->nowildcard_len = item->len; @@ -627,7 +629,8 @@ void parse_pathspec(struct pathspec *pathspec, for (i = 0; i < n; i++) { entry = argv[i]; - init_pathspec_item(item + i, flags, prefix, prefixlen, entry); + init_pathspec_item(item + i, &the_index, flags, + prefix, prefixlen, entry); if (item[i].magic & PATHSPEC_EXCLUDE) nr_exclude++; @@ -653,7 +656,7 @@ void parse_pathspec(struct pathspec *pathspec, */ if (nr_exclude == n) { int plen = (!(flags & PATHSPEC_PREFER_CWD)) ? 0 : prefixlen; - init_pathspec_item(item + n, 0, prefix, plen, ""); + init_pathspec_item(item + n, &the_index, 0, prefix, plen, ""); pathspec->nr++; } -- 2.13.0.rc2.291.g57267f2277-goog
[PATCH 2/8] submodule: add die_in_unpopulated_submodule function
Currently 'git add' is the only command which dies when launched from an unpopulated submodule (the place-holder directory for a submodule which hasn't been checked out). This is triggered implicitly by passing the PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to 'parse_pathspec()'. Instead make this desire more explicit by creating a function 'die_in_unpopulated_submodule()' which dies if the provided 'prefix' has a leading path component which matches a submodule in the the index. Signed-off-by: Brandon Williams --- builtin/add.c| 3 +++ pathspec.c | 29 - submodule.c | 30 ++ submodule.h | 2 ++ t/t6134-pathspec-in-submodule.sh | 6 +- 5 files changed, 36 insertions(+), 34 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index 9f53f020d..ec58e3679 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -17,6 +17,7 @@ #include "revision.h" #include "bulk-checkin.h" #include "argv-array.h" +#include "submodule.h" static const char * const builtin_add_usage[] = { N_("git add [] [--] ..."), @@ -379,6 +380,8 @@ int cmd_add(int argc, const char **argv, const char *prefix) if (read_cache() < 0) die(_("index file corrupt")); + die_in_unpopulated_submodule(&the_index, prefix); + /* * Check the "pathspec '%s' did not match any files" block * below before enabling new magic. diff --git a/pathspec.c b/pathspec.c index 61b5b1273..e37256c3b 100644 --- a/pathspec.c +++ b/pathspec.c @@ -424,27 +424,6 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item) } } -static void die_inside_submodule_path(struct pathspec_item *item) -{ - int i; - - for (i = 0; i < active_nr; i++) { - struct cache_entry *ce = active_cache[i]; - int ce_len = ce_namelen(ce); - - if (!S_ISGITLINK(ce->ce_mode)) - continue; - - if (item->len < ce_len || - !(item->match[ce_len] == '/' || item->match[ce_len] == '\0') || - memcmp(ce->name, item->match, ce_len)) - continue; - - die(_("Pathspec '%s' is in submodule '%.*s'"), - item->original, ce_len, ce->name); - } -} - /* * Perform the initialization of a pathspec_item based on a pathspec element. */ @@ -547,14 +526,6 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags, /* sanity checks, pathspec matchers assume these are sane */ if (item->nowildcard_len > item->len || item->prefix > item->len) { - /* -* This case can be triggered by the user pointing us to a -* pathspec inside a submodule, which is an input error. -* Detect that here and complain, but fallback in the -* non-submodule case to a BUG, as we have no idea what -* would trigger that. -*/ - die_inside_submodule_path(item); die ("BUG: error initializing pathspec_item"); } } diff --git a/submodule.c b/submodule.c index 7c3c4b17f..9e0502f25 100644 --- a/submodule.c +++ b/submodule.c @@ -281,6 +281,36 @@ int is_submodule_populated_gently(const char *path, int *return_error_code) return ret; } +/* + * Dies if the provided 'prefix' corresponds to an unpopulated submodule + */ +void die_in_unpopulated_submodule(const struct index_state *istate, + const char *prefix) +{ + int i, prefixlen; + + if (!prefix) + return; + + prefixlen = strlen(prefix); + + for (i = 0; i < istate->cache_nr; i++) { + struct cache_entry *ce = istate->cache[i]; + int ce_len = ce_namelen(ce); + + if (!S_ISGITLINK(ce->ce_mode)) + continue; + if (prefixlen <= ce_len) + continue; + if (strncmp(ce->name, prefix, ce_len)) + continue; + if (prefix[ce_len] != '/') + continue; + + die(_("in unpopulated submodule '%s'"), ce->name); + } +} + int parse_submodule_update_strategy(const char *value, struct submodule_update_strategy *dst) { diff --git a/submodule.h b/submodule.h index 8a8bc49dc..f4fe6374d 100644 --- a/submodule.h +++ b/submodule.h @@ -48,6 +48,8 @@ extern int is_submodule_initialized(const char *path); * Otherwise the return error code is the same as of resolve_gitdir_gently. */ extern int is_submodule_populated_gently(const char *path, int *return_error_code); +extern void die_in_unpopulated_submodule(const struct index_state *istate, +const char *prefix); extern int parse_submodule_update_stra
[PATCH 8/8] pathspec: convert parse_pathspec to take an index
Convert 'parse_pathspec()' to take an index parameter. Since the index is only needed when the PATHSPEC_SUBMODULE_LEADING_PATH and PATHSPEC_STRIP_SUBMODULE_SLASH flags are given, add a check requiring a non-NULL index when either of these flags are given. Convert callers which use these two flags to pass in an index while having other callers pass in NULL. Now that pathspec.c does not use any cache macros and has no references to 'the_index', mark it by defining NO_THE_INDEX_COMPATIBILITY_MACROS. Signed-off-by: Brandon Williams --- archive.c | 4 ++-- builtin/add.c | 4 ++-- builtin/blame.c | 2 +- builtin/check-ignore.c | 2 +- builtin/checkout.c | 2 +- builtin/clean.c | 2 +- builtin/commit.c| 4 ++-- builtin/grep.c | 2 +- builtin/ls-files.c | 4 ++-- builtin/ls-tree.c | 5 +++-- builtin/rerere.c| 2 +- builtin/reset.c | 2 +- builtin/rm.c| 2 +- builtin/submodule--helper.c | 2 +- builtin/update-index.c | 2 +- line-log.c | 2 +- pathspec.c | 13 ++--- pathspec.h | 1 + revision.c | 5 +++-- submodule.c | 2 +- tree-diff.c | 2 +- 21 files changed, 38 insertions(+), 28 deletions(-) diff --git a/archive.c b/archive.c index 60b889198..ce9b30f2e 100644 --- a/archive.c +++ b/archive.c @@ -306,7 +306,7 @@ static int path_exists(struct tree *tree, const char *path) struct pathspec pathspec; int ret; - parse_pathspec(&pathspec, 0, 0, "", paths); + parse_pathspec(&pathspec, NULL, 0, 0, "", paths); pathspec.recursive = 1; ret = read_tree_recursive(tree, "", 0, 0, &pathspec, reject_entry, &pathspec); @@ -322,7 +322,7 @@ static void parse_pathspec_arg(const char **pathspec, * Also if pathspec patterns are dependent, we're in big * trouble as we test each one separately */ - parse_pathspec(&ar_args->pathspec, 0, + parse_pathspec(&ar_args->pathspec, NULL, 0, PATHSPEC_PREFER_FULL, "", pathspec); ar_args->pathspec.recursive = 1; diff --git a/builtin/add.c b/builtin/add.c index 4e3bf20c2..23606db39 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -181,7 +181,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch) { struct pathspec pathspec; - parse_pathspec(&pathspec, 0, + parse_pathspec(&pathspec, NULL, 0, PATHSPEC_PREFER_FULL | PATHSPEC_SYMLINK_LEADING_PATH | PATHSPEC_PREFIX_ORIGIN, @@ -386,7 +386,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) * Check the "pathspec '%s' did not match any files" block * below before enabling new magic. */ - parse_pathspec(&pathspec, 0, + parse_pathspec(&pathspec, &the_index, 0, PATHSPEC_PREFER_FULL | PATHSPEC_SYMLINK_LEADING_PATH | PATHSPEC_STRIP_SUBMODULE_SLASH | diff --git a/builtin/blame.c b/builtin/blame.c index f7aa95f4b..e37837c17 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -557,7 +557,7 @@ static struct origin *find_origin(struct scoreboard *sb, paths[0] = origin->path; paths[1] = NULL; - parse_pathspec(&diff_opts.pathspec, + parse_pathspec(&diff_opts.pathspec, NULL, PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL, PATHSPEC_LITERAL_PATH, "", paths); diff_setup_done(&diff_opts); diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c index 446b76dcf..90169e79a 100644 --- a/builtin/check-ignore.c +++ b/builtin/check-ignore.c @@ -84,7 +84,7 @@ static int check_ignore(struct dir_struct *dir, * check-ignore just needs paths. Magic beyond :/ is really * irrelevant. */ - parse_pathspec(&pathspec, + parse_pathspec(&pathspec, &the_index, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP, PATHSPEC_SYMLINK_LEADING_PATH | PATHSPEC_SUBMODULE_LEADING_PATH | diff --git a/builtin/checkout.c b/builtin/checkout.c index bfa5419f3..cb8ed09f6 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1274,7 +1274,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) } if (argc) { - parse_pathspec(&opts.pathspec, 0, + parse_pathspec(&opts.pathspec, NULL, 0, opts.patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, prefix, argv); diff --git a/builtin/clean.c b/builtin/clean.c index d861f836a..ebc980b4f 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -926,7 +926,7 @@ int cmd_clean(int argc, c
[PATCH 1/8] pathspec: provide a more descriptive die message
The current message displayed upon an internal error in 'init_pathspec_item()' isn't very descriptive and doesn't provide much context to where the error occurred. Update the error message to provide more context to where the error occured. Signed-off-by: Brandon Williams --- pathspec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pathspec.c b/pathspec.c index 303efda83..61b5b1273 100644 --- a/pathspec.c +++ b/pathspec.c @@ -555,7 +555,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags, * would trigger that. */ die_inside_submodule_path(item); - die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)"); + die ("BUG: error initializing pathspec_item"); } } -- 2.13.0.rc2.291.g57267f2277-goog
[PATCH 0/8] convert pathspec.c to take an index parameter
This is another conversion series to convert the pathspec library code to take in an index parameter instead of relying on cache macros or on the global variable 'the_index'. While I was working in the pathspec code I thought it would be good to do a little more cleanup and make the API cleaner. More specifically consolidating the 'strip submodule slash' flags into a single flag while splitting out the 'submodule leading path' check performed in the expensive case into its own flag. Brandon Williams (8): pathspec: provide a more descriptive die message submodule: add die_in_unpopulated_submodule function pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag pathspec: rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP pathspec: convert strip_submodule_slash to take an index pathspec: convert find_pathspecs_matching_against_index to take an index pathspec: convert init_pathspec_item to take an index pathspec: convert parse_pathspec to take an index archive.c| 4 +- builtin/add.c| 14 +++--- builtin/blame.c | 2 +- builtin/check-ignore.c | 7 +-- builtin/checkout.c | 2 +- builtin/clean.c | 2 +- builtin/commit.c | 4 +- builtin/grep.c | 2 +- builtin/ls-files.c | 6 +-- builtin/ls-tree.c| 5 ++- builtin/rerere.c | 2 +- builtin/reset.c | 4 +- builtin/rm.c | 4 +- builtin/submodule--helper.c | 4 +- builtin/update-index.c | 2 +- line-log.c | 2 +- pathspec.c | 92 +--- pathspec.h | 19 + revision.c | 5 ++- submodule.c | 32 +- submodule.h | 2 + t/t6134-pathspec-in-submodule.sh | 6 +-- tree-diff.c | 2 +- 23 files changed, 122 insertions(+), 102 deletions(-) -- 2.13.0.rc2.291.g57267f2277-goog
[PATCH 4/8] pathspec: rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP
Now that there is only a single flag which strips a submodule slash, rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to PATHSPEC_STRIP_SUBMODULE_SLASH. Signed-off-by: Brandon Williams --- builtin/add.c | 2 +- builtin/check-ignore.c | 2 +- builtin/ls-files.c | 2 +- builtin/reset.c | 2 +- builtin/rm.c| 2 +- builtin/submodule--helper.c | 2 +- pathspec.c | 6 +++--- pathspec.h | 2 +- 8 files changed, 10 insertions(+), 10 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index 2aa9aeab9..51d7a5506 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -389,7 +389,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_FULL | PATHSPEC_SYMLINK_LEADING_PATH | - PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP | + PATHSPEC_STRIP_SUBMODULE_SLASH | PATHSPEC_SUBMODULE_LEADING_PATH, prefix, argv); diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c index 73237b2b1..694e4c61b 100644 --- a/builtin/check-ignore.c +++ b/builtin/check-ignore.c @@ -88,7 +88,7 @@ static int check_ignore(struct dir_struct *dir, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP, PATHSPEC_SYMLINK_LEADING_PATH | PATHSPEC_SUBMODULE_LEADING_PATH | - PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP | + PATHSPEC_STRIP_SUBMODULE_SLASH | PATHSPEC_KEEP_ORDER, prefix, argv); diff --git a/builtin/ls-files.c b/builtin/ls-files.c index d449e46db..e9dee2e41 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -618,7 +618,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD | - PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP, + PATHSPEC_STRIP_SUBMODULE_SLASH, prefix, argv); /* diff --git a/builtin/reset.c b/builtin/reset.c index fc3b906c4..529f2f9be 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -236,7 +236,7 @@ static void parse_args(struct pathspec *pathspec, parse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL | - PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP | + PATHSPEC_STRIP_SUBMODULE_SLASH | (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0), prefix, argv); } diff --git a/builtin/rm.c b/builtin/rm.c index fb79dcab1..8fe12d0a7 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -272,7 +272,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix) parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD | - PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP, + PATHSPEC_STRIP_SUBMODULE_SLASH, prefix, argv); refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL); diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 85aafe46a..69149b557 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -234,7 +234,7 @@ static int module_list_compute(int argc, const char **argv, char *ps_matched = NULL; parse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL | - PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP, + PATHSPEC_STRIP_SUBMODULE_SLASH, prefix, argv); if (pathspec->nr) diff --git a/pathspec.c b/pathspec.c index f37d5769d..a1297ca02 100644 --- a/pathspec.c +++ b/pathspec.c @@ -386,7 +386,7 @@ static const char *parse_element_magic(unsigned *magic, int *prefix_len, return parse_short_magic(magic, elem); } -static void strip_submodule_slash_cheap(struct pathspec_item *item) +static void strip_submodule_slash(struct pathspec_item *item) { if (item->len >= 1 && item->match[item->len - 1] == '/') { int i = cache_name_pos(item->match, item->len - 1); @@ -496,8 +496,8 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags, item->original = xstrdup(elt); } - if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) - strip_submodule_slash_cheap(item); + if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH) + strip_submodule_slash(item); if (magic & PATHSPEC_LITERAL) { item->nowildcard_len = item->len; diff --git a/pathspec.h b/pathspec.h index 93a819cbf..dd6456d5d 100644 --- a/pathspec.h +++ b/pathspec.h @@ -59,7 +59,7 @@ struct pathspec { #define PATHSPEC_PREFER_FULL (1<<1) /* No args means match everything */ #define PATHSPEC_MAXDEPTH_VALID (1<<2) /* max_depth field is valid */ /* strip the t
[PATCH] fetch-pack: always allow fetching of literal SHA1s
fetch-pack, when fetching a literal SHA-1 from a server that is not configured with uploadpack.allowtipsha1inwant (or similar), always returns an error message of the form "Server does not allow request for unadvertised object %s". However, it is sometimes the case that such object is advertised. Teach fetch-pack to also check the SHA-1s of the refs in the received ref advertisement if a literal SHA-1 was given by the user. Signed-off-by: Jonathan Tan --- fetch-pack.c | 30 -- t/t5500-fetch-pack.sh | 6 ++ 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index afb8b0502..180405dff 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -592,6 +592,14 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args, } } +static int is_literal_sha1(const struct ref *ref) +{ + struct object_id oid; + return !get_oid_hex(ref->name, &oid) && + !ref->name[40] && + !oidcmp(&oid, &ref->old_oid); +} + static void filter_refs(struct fetch_pack_args *args, struct ref **refs, struct ref **sought, int nr_sought) @@ -601,7 +609,6 @@ static void filter_refs(struct fetch_pack_args *args, struct ref *ref, *next; int i; - i = 0; for (ref = *refs; ref; ref = next) { int keep = 0; next = ref->next; @@ -610,15 +617,14 @@ static void filter_refs(struct fetch_pack_args *args, check_refname_format(ref->name, 0)) ; /* trash */ else { - while (i < nr_sought) { - int cmp = strcmp(ref->name, sought[i]->name); - if (cmp < 0) - break; /* definitely do not have it */ - else if (cmp == 0) { - keep = 1; /* definitely have it */ - sought[i]->match_status = REF_MATCHED; + for (i = 0; i < nr_sought; i++) { + struct ref *s = sought[i]; + if (!strcmp(ref->name, s->name) || + (is_literal_sha1(s) && +!oidcmp(&ref->old_oid, &s->old_oid))) { + keep = 1; + s->match_status = REF_MATCHED; } - i++; } } @@ -637,14 +643,10 @@ static void filter_refs(struct fetch_pack_args *args, /* Append unmatched requests to the list */ for (i = 0; i < nr_sought; i++) { - unsigned char sha1[20]; - ref = sought[i]; if (ref->match_status != REF_NOT_MATCHED) continue; - if (get_sha1_hex(ref->name, sha1) || - ref->name[40] != '\0' || - hashcmp(sha1, ref->old_oid.hash)) + if (!is_literal_sha1(ref)) continue; if ((allow_unadvertised_object_request & diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index b5865b385..d87983bc2 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -547,6 +547,12 @@ test_expect_success 'fetch-pack can fetch a raw sha1' ' git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one) ' +test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a ref' ' + git init server && + test_commit -C server 4 && + git fetch-pack server $(git -C server rev-parse refs/heads/master) +' + check_prot_path () { cat >expected <<-EOF && Diag: url=$1 -- 2.13.0.rc2.291.g57267f2277-goog
Re: [PATCH v2] travis-ci: retry if Git for Windows CI returns HTTP error 502 or 503
> On 09 May 2017, at 07:31, Junio C Hamano wrote: > > Lars Schneider writes: > >> The Git for Windows CI web app sometimes returns HTTP errors of >> "502 bad gateway" or "503 service unavailable" [1]. We also need to >> check the HTTP content because the GfW web app seems to pass through >> (error) results from other Azure calls with HTTP code 200. >> Wait a little and retry the request if this happens. >> >> [1] >> https://docs.microsoft.com/en-in/azure/app-service-web/app-service-web-troubleshoot-http-502-http-503 >> >> Signed-off-by: Lars Schneider >> --- >> >> Hi Junio, >> >> I can't really test this as my TravisCI account does not have the >> extended timeout and I am unable to reproduce the error. >> >> It would be great if we could test this is a little bit in pu. > > This has been in 'pu' for a while. > > As the patch simply discards 502 (and others), it is unclear if the > failing test on 'next' is now gone, or the attempt to run 'pu' > happened to be lucky not to get one, from the output we can see in > https://travis-ci.org/git/git/jobs/229867212 > > Are you comfortable enough to move this forward? Yes, please move it forward. I haven't seen a "502 - Web server received an invalid response" on pu for a while. That means the patch should work as expected. Unrelated to this patch I have, however, seen two kinds of timeouts: (1) Timeout in the "notStarted" state. This job eventually finished with a failure but it did start only *after* 3h: https://travis-ci.org/git/git/jobs/230225611 (2) Timeout in the "in progress" state. This job eventually finished successfully but it took longer than 3h: https://travis-ci.org/git/git/jobs/229867248 Right now the timeout generates potential false negative results. I would like to change that and respond with a successful build *before* we approach the 3h timeout. This means we could generate false positives. Although this is not ideal, I think that is the better compromise as a failing Windows build would usually fail quickly (e.g. in the compile step). What do you guys think? Would you be OK with that reasoning? If the Git for Windows builds get more stable over time then we could reevaluate this compromise. - Lars
Re: [PATCH] fixup! use perl instead of sed
On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan wrote: > Signed-off-by: Jonathan Tan > --- > > Thanks - I didn't realize the existence of the test lint. Here's a > fixup. Let me know if you prefer a full reroll. > > I had to use perl because "push" is a binary file (the first line > contains a NUL). > > > t/t5534-push-signed.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh > index 0ef6f66b5..3ee58dafb 100755 > --- a/t/t5534-push-signed.sh > +++ b/t/t5534-push-signed.sh > @@ -152,11 +152,11 @@ test_expect_success GPG 'inconsistent push options in > signed push not allowed' ' > # Tweak the push output to make the push option outside the cert > # different, then replay it on a fresh dst, checking that ff is not > # deleted. > - sed -i "s/\\([^ ]\\)bar/\\1baz/" push && > + perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak && > prepare_dst && > git -C dst config receive.certnonceseed sekrit && > git -C dst config receive.advertisepushoptions 1 && > - git receive-pack dst out && > + git receive-pack dst out && The test should have a PERL prerequisite now, that's missing. Also using \1 will likely be deprecated in future versions of perl at some point: $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/" \1 better written as $1 at -e line 1. hifoo Finally, you can just use -i like you did with sed, no need for the tempfile: $ echo hibar >push $ perl -pi -e 's/([^ ])bar/$1baz/' push $ cat push hibaz > git -C dst rev-parse ff && > grep "inconsistent push options" out > ' > -- > 2.13.0.rc2.291.g57267f2277-goog >
[PATCH] fixup! use perl instead of sed
Signed-off-by: Jonathan Tan --- Thanks - I didn't realize the existence of the test lint. Here's a fixup. Let me know if you prefer a full reroll. I had to use perl because "push" is a binary file (the first line contains a NUL). t/t5534-push-signed.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh index 0ef6f66b5..3ee58dafb 100755 --- a/t/t5534-push-signed.sh +++ b/t/t5534-push-signed.sh @@ -152,11 +152,11 @@ test_expect_success GPG 'inconsistent push options in signed push not allowed' ' # Tweak the push output to make the push option outside the cert # different, then replay it on a fresh dst, checking that ff is not # deleted. - sed -i "s/\\([^ ]\\)bar/\\1baz/" push && + perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak && prepare_dst && git -C dst config receive.certnonceseed sekrit && git -C dst config receive.advertisepushoptions 1 && - git receive-pack dst out && + git receive-pack dst out && git -C dst rev-parse ff && grep "inconsistent push options" out ' -- 2.13.0.rc2.291.g57267f2277-goog
Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
On Tue, May 9, 2017 at 4:22 PM, demerphq wrote: > On 9 May 2017 at 13:12, Ævar Arnfjörð Bjarmason wrote: >> On Tue, May 9, 2017 at 2:37 AM, brian m. carlson >> wrote: >>> On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote: >> * gitweb is vulnerable to CPU DoS now in its default configuration. >> It's easy to provide an ERE that ends up slurping up 100% CPU for >> several seconds on any non-trivial sized repo, do that in parallel & >> you have a DoS vector. > > Does one need an ERE? Can't one do that now to many parts of git just > with a glob? in practice I don't think so because: 1) I'm now aware of any place where we expose globbing over the wire. 2) AFAICT for the issue detailed in [1] to trigger you also need a pathological filename in the repo, e.g. I can't get git-ls-files to go quadratic on either git.git or linux.git, whereas it's pretty easy to come up with a really expensive regex since there's more content to choose from when matching file content than filenames. 1. https://public-inbox.org/git/20170424211249.28553-1-ava...@gmail.com/
Re: [GIT PULL] l10n updates for 2.13.0 round 2.1
Jiang Xin writes: > Merged another l10n contribution, please pull the new tag > l10n-2.13.0-rnd2.1 (old tag is deleted): Yeah, I see our mails crossed. Will pull. Thanks!
Re: [GIT PULL] l10n updates for 2.13.0 round 2
Jiang Xin writes: > Hi Junio, > > I can not send email outside at work, but now I am back home. Here is > the pull request: > > The following changes since commit 4fa66c85f11bc5a541462ca5ae3246aa0ce02e74: > > Git 2.13-rc2 (2017-05-04 16:27:19 +0900) > > are available in the git repository at: > > git://github.com/git-l10n/git-po tags/l10n-2.13.0-rnd2 > > for you to fetch changes up to 60638e9816d0aae40d4234d1a068f94fabc2fd4d: > > l10n: zh_CN: for git v2.13.0 l10n round 2 (2017-05-09 21:55:38 +0800) Thanks. I see another merge for sv and the tag is now l10n-2.13.0-rnd2.1 (i.e. I couldn't find -rnd2 tag), which I assume is what you meant me to fetch?
Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
On 9 May 2017 at 13:12, Ævar Arnfjörð Bjarmason wrote: > On Tue, May 9, 2017 at 2:37 AM, brian m. carlson > wrote: >> On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote: > * gitweb is vulnerable to CPU DoS now in its default configuration. > It's easy to provide an ERE that ends up slurping up 100% CPU for > several seconds on any non-trivial sized repo, do that in parallel & > you have a DoS vector. Does one need an ERE? Can't one do that now to many parts of git just with a glob? Yves
Re: [GIT PULL] l10n updates for 2.13.0 round 2.1
Hi Junio, Merged another l10n contribution, please pull the new tag l10n-2.13.0-rnd2.1 (old tag is deleted): The following changes since commit 4fa66c85f11bc5a541462ca5ae3246aa0ce02e74: Git 2.13-rc2 (2017-05-04 16:27:19 +0900) are available in the git repository at: git://github.com/git-l10n/git-po tags/l10n-2.13.0-rnd2.1 for you to fetch changes up to 961f9c8b1b0f6bff09cb876f95e9ca33702763ac: Merge branch 'master' of git://github.com/nafmo/git-l10n-sv (2017-05-09 22:12:34 +0800) l10n for Git 2.13.0 round 2.1 Alexander Shopov (2): l10n: bg.po: Updated Bulgarian translation (3201t) l10n: bg.po: Updated Bulgarian translation (3195t) Jean-Noel Avila (2): l10n: fr.po v2.13 round 1 l10n: fr.po v2.13 rnd 2 Jiang Xin (10): l10n: git.pot: v2.13.0 round 1 (96 new, 37 removed) Merge branch 'master' of https://github.com/vnwildman/git Merge branch 'fr_l10n_v2.13_rnd1' of git://github.com/jnavila/git l10n: zh_CN: for git v2.13.0 l10n round 1 Merge branch 'master' of git://github.com/git-l10n/git-po l10n: git.pot: v2.13.0 round 2 (4 new, 7 removed) Merge branch 'fr_l10n_v2.13_rnd2' of git://github.com/jnavila/git Merge branch 'master' of https://github.com/vnwildman/git l10n: zh_CN: for git v2.13.0 l10n round 2 Merge branch 'master' of git://github.com/nafmo/git-l10n-sv Jordi Mas (1): l10n: Update Catalan translation Michael J Gruber (1): l10n: de.po: lower case after semi-colon Peter Krefting (2): l10n: sv.po: Update Swedish translation (3199t0f0u) l10n: sv.po: Update Swedish translation (3195t0f0u) Ralf Thielow (2): l10n: de.po: update German translation l10n: de.po: translate 4 new messages Ray Chen (1): l10n: zh_CN: review for git v2.13.0 l10n round 1 Trần Ngọc Quân (2): l10n: vi.po(3198t): Updated Vietnamese translation for v2.13.0-rc0 l10n: vi.po(3195t): Update translation for v2.13.0 round 2 Vasco Almeida (1): l10n: pt_PT: update Portuguese translation po/bg.po| 6511 +-- po/ca.po| 4617 +++--- po/de.po| 4691 ++ po/fr.po| 4548 ++--- po/git.pot | 4387 +--- po/pt_PT.po | 4528 ++--- po/sv.po| 4533 ++--- po/vi.po| 4548 ++--- po/zh_CN.po | 4569 ++--- 9 files changed, 23745 insertions(+), 19187 deletions(-) 2017-05-09 22:08 GMT+08:00 Jiang Xin : > Hi Junio, > > I can not send email outside at work, but now I am back home. Here is > the pull request: > > The following changes since commit 4fa66c85f11bc5a541462ca5ae3246aa0ce02e74: > > Git 2.13-rc2 (2017-05-04 16:27:19 +0900) > > are available in the git repository at: > > git://github.com/git-l10n/git-po tags/l10n-2.13.0-rnd2 > > for you to fetch changes up to 60638e9816d0aae40d4234d1a068f94fabc2fd4d: > > l10n: zh_CN: for git v2.13.0 l10n round 2 (2017-05-09 21:55:38 +0800) > > > l10n for Git 2.13.0 round 2 > > > Alexander Shopov (2): > l10n: bg.po: Updated Bulgarian translation (3201t) > l10n: bg.po: Updated Bulgarian translation (3195t) > > Jean-Noel Avila (2): > l10n: fr.po v2.13 round 1 > l10n: fr.po v2.13 rnd 2 > > Jiang Xin (9): > l10n: git.pot: v2.13.0 round 1 (96 new, 37 removed) > Merge branch 'master' of https://github.com/vnwildman/git > Merge branch 'fr_l10n_v2.13_rnd1' of git://github.com/jnavila/git > l10n: zh_CN: for git v2.13.0 l10n round 1 > Merge branch 'master' of git://github.com/git-l10n/git-po > l10n: git.pot: v2.13.0 round 2 (4 new, 7 removed) > Merge branch 'fr_l10n_v2.13_rnd2' of git://github.com/jnavila/git > Merge branch 'master' of https://github.com/vnwildman/git > l10n: zh_CN: for git v2.13.0 l10n round 2 > > Jordi Mas (1): > l10n: Update Catalan translation > > Michael J Gruber (1): > l10n: de.po: lower case after semi-colon > > Peter Krefting (1): > l10n: sv.po: Update Swedish translation (3199t0f0u) > > Ralf Thielow (2): > l10n: de.po: update German translation > l10n: de.po: translate 4 new messages > > Ray Chen (1): > l10n: zh_CN: review for git v2.13.0 l10n round 1 > > Trần Ngọc Quân (2): > l10n: vi.po(3198t): Updated Vietnamese translation for v2.13.0-rc0 > l10n: vi.po(3195t): Update translation for v2.13.0 round 2 > > Vasco Almeida (1): > l10n: pt_PT: update Portuguese translation > > po/bg.po| 65
[GIT PULL] l10n updates for 2.13.0 round 2
Hi Junio, I can not send email outside at work, but now I am back home. Here is the pull request: The following changes since commit 4fa66c85f11bc5a541462ca5ae3246aa0ce02e74: Git 2.13-rc2 (2017-05-04 16:27:19 +0900) are available in the git repository at: git://github.com/git-l10n/git-po tags/l10n-2.13.0-rnd2 for you to fetch changes up to 60638e9816d0aae40d4234d1a068f94fabc2fd4d: l10n: zh_CN: for git v2.13.0 l10n round 2 (2017-05-09 21:55:38 +0800) l10n for Git 2.13.0 round 2 Alexander Shopov (2): l10n: bg.po: Updated Bulgarian translation (3201t) l10n: bg.po: Updated Bulgarian translation (3195t) Jean-Noel Avila (2): l10n: fr.po v2.13 round 1 l10n: fr.po v2.13 rnd 2 Jiang Xin (9): l10n: git.pot: v2.13.0 round 1 (96 new, 37 removed) Merge branch 'master' of https://github.com/vnwildman/git Merge branch 'fr_l10n_v2.13_rnd1' of git://github.com/jnavila/git l10n: zh_CN: for git v2.13.0 l10n round 1 Merge branch 'master' of git://github.com/git-l10n/git-po l10n: git.pot: v2.13.0 round 2 (4 new, 7 removed) Merge branch 'fr_l10n_v2.13_rnd2' of git://github.com/jnavila/git Merge branch 'master' of https://github.com/vnwildman/git l10n: zh_CN: for git v2.13.0 l10n round 2 Jordi Mas (1): l10n: Update Catalan translation Michael J Gruber (1): l10n: de.po: lower case after semi-colon Peter Krefting (1): l10n: sv.po: Update Swedish translation (3199t0f0u) Ralf Thielow (2): l10n: de.po: update German translation l10n: de.po: translate 4 new messages Ray Chen (1): l10n: zh_CN: review for git v2.13.0 l10n round 1 Trần Ngọc Quân (2): l10n: vi.po(3198t): Updated Vietnamese translation for v2.13.0-rc0 l10n: vi.po(3195t): Update translation for v2.13.0 round 2 Vasco Almeida (1): l10n: pt_PT: update Portuguese translation po/bg.po| 6511 +-- po/ca.po| 4617 +++--- po/de.po| 4691 ++ po/fr.po| 4548 ++--- po/git.pot | 4387 +--- po/pt_PT.po | 4528 ++--- po/sv.po| 4232 -- po/vi.po| 4548 ++--- po/zh_CN.po | 4569 ++--- 9 files changed, 23592 insertions(+), 19039 deletions(-) 2017-05-09 12:57 GMT+08:00 Junio C Hamano : > Jiang Xin writes: > >> Git v2.13.0-rc2 introduced 4 new messages, let's start round 2 for Git >> 2.13.0 l10n. >> >> You can get it from the usual place: >> >> https://github.com/git-l10n/git-po/ >> >> As how to update your XX.po and help to translate Git, please see >> "Updating a XX.po file" and other sections in “po/README" file. > > What's the doneness of l10n for the upcoming release? It would be > preferrable if I can get a "done -- pull from me now" within 16 > hours or so, as I'd like to do the release at around May 10th 00:00 > UTC. > > Thanks everybody, as always, for your translations and general > support of the project.
Re: [PATCH v4 11/25] checkout: fix memory leak
Hi Junio & René, On Mon, 8 May 2017, Junio C Hamano wrote: > René Scharfe writes: > > >>/* > >> * NEEDSWORK: > >> * There is absolutely no reason to write this as a blob object > >> - * and create a phony cache entry just to leak. This hack is > >> - * primarily to get to the write_entry() machinery that massages > >> - * the contents to work-tree format and writes out which only > >> - * allows it for a cache entry. The code in write_entry() needs > >> - * to be refactored to allow us to feed a > >> - * instead of a cache entry. Such a refactoring would help > >> - * merge_recursive as well (it also writes the merge result to the > >> - * object database even when it may contain conflicts). > >> + * and create a phony cache entry. This hack is primarily to get > >> + * to the write_entry() machinery that massages the contents to > >> + * work-tree format and writes out which only allows it for a > >> + * cache entry. The code in write_entry() needs to be refactored > >> + * to allow us to feed a instead of a cache > >> + * entry. Such a refactoring would help merge_recursive as well > >> + * (it also writes the merge result to the object database even > >> + * when it may contain conflicts). > >> */ > >>if (write_sha1_file(result_buf.ptr, result_buf.size, > >>blob_type, oid.hash)) > > > > Random observation: Using pretend_sha1_file here would at least avoid > > writing the blob. > > Yup, you should have told that to me back in Aug 2008 ;-) when I did > 0cf8581e ("checkout -m: recreate merge when checking out of unmerged > index", 2008-08-30); pretend_sha1_file() was available since early > 2007, and there is no excuse that this codepath did not use it. I hope y'all agree that this is outside the scope of my patch series... > >> @@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct > >> checkout *state) > >>if (!ce) > >>die(_("make_cache_entry failed for path '%s'"), path); > >>status = checkout_entry(ce, state, NULL); > >> + free(ce); > >>return status; > >> } > > > > I wonder if that's safe. Why document a leak when it could have been > > plugged this easily instead? > > > > A leak is better than a use after free, so > > let's be extra careful here. Would it leave the index inconsistent? Or > > perhaps freeing it has become safe in the meantime? > > > > @Junio: Do you remember the reason for the leaks in 0cf8581e330 > > (checkout -m: recreate merge when checking out of unmerged index). > > Yes. > > In the very old days it was not allowed to free(3) contents of > active_cache[] and this was an old brain fart that came out of > inertia. We are manufacturing a brand new ce, only to feed it to > checkout_entry() without even registering it to the active_cache[] > array, and the ancient rule doesn't even apply to such a case. > > So I think it was safe to free(3) even back then. So this patch is good, then? > > And result_buf is still leaked here, right? > > Good spotting. It typically would make a larger leak than a single > ce, I would suppose ;-) I saw you added this as a fixup! commit. If you don't mind, and if no other changes are requested, would you mind rebase'ing this yourself? Thanks, Dscho
Re: Would this tool be useful - encoding repository data into single URL
On Tue, May 9, 2017 at 2:48 PM, Sebastian Gniazdowski wrote: > Hello > I wonder about usability of following tool. Quick-start: > > giturl https://github.com/zdharma/giturl -r devel -p > lib/coding_functions.cpp > > Protocol: https > Site: github.com > Repo: zdharma/giturl > Revision: devel > File: lib/coding_functions.cpp > > gitu://ŬṽǚǫoŒẗ6ẏȅcЭÑẩőn4ầŘїệαЃȣϟṈӛŀї > > It does Huffman encoding and base-1024 encoding to pack given data into > single URL. The Unicode characters selected for base-1024 encoding are > letters, not symbols, so double-clicking in e.g. web browser selects the > whole code, making it easy to grab a repository data. > > Decoding: > > giturl -qd ŬṽǚǫoŒẗ6ẏȅcЭÑẩőn4ầŘїệαЃȣϟṈӛŀї > https://github.com/zdharma/giturl / rev:devel / > file:lib/coding_functions.cpp > > I can also encode commits relative to given revision, e.g. bits 10011 are > commits 1, 4, 5. Easy to add to the g-code. Selecting 10th commit is only 1 > character in base-1024. > > However I wonder if this has any uses. Could be patches sent this way? Having > refs/patches/, encoding in URL, sending it instead of > inlining/attaching a diff, selecting e.g. 3 commits via the bit-mask > mentioned. That said, it's more about easy-grab of repository data and > storage in well-defined, consistent format, not in language "the branch is > ..., commit a7a35cb". Does this make sense? > > There are 2 implementations, in Zsh (uses Zshell like e.g. Ruby, not > interactively) and C++11 (mostly because of std::regex): > > https://github.com/zdharma/giturl > https://github.com/zdharma/cgiturl This looks really cool. Yeah patches can be sent this way, see Documentation/SubmittingPatches in the git.git repository, online version: https://github.com/git/git/blob/master/Documentation/SubmittingPatches
Re: [PATCH v3 12/25] split_commit_in_progress(): fix memory leak
Hi René, On Sat, 6 May 2017, René Scharfe wrote: > Am 04.05.2017 um 12:59 schrieb Johannes Schindelin: > > > diff --git a/wt-status.c b/wt-status.c > > index 1f3f6bcb980..117ac8cfb01 100644 > > --- a/wt-status.c > > +++ b/wt-status.c > > @@ -1082,34 +1082,29 @@ static char *read_line_from_git_path(const char > > *filename) > > static int split_commit_in_progress(struct wt_status *s) > > { > > int split_in_progress = 0; > > - char *head = read_line_from_git_path("HEAD"); > > - char *orig_head = read_line_from_git_path("ORIG_HEAD"); > > - char *rebase_amend = read_line_from_git_path("rebase-merge/amend"); > > - char *rebase_orig_head = > > read_line_from_git_path("rebase-merge/orig-head"); > > - > > - if (!head || !orig_head || !rebase_amend || !rebase_orig_head || > > - !s->branch || strcmp(s->branch, "HEAD")) { > > - free(head); > > - free(orig_head); > > - free(rebase_amend); > > - free(rebase_orig_head); > > - return split_in_progress; > > - } > > - > > - if (!strcmp(rebase_amend, rebase_orig_head)) { > > - if (strcmp(head, rebase_amend)) > > - split_in_progress = 1; > > - } else if (strcmp(orig_head, rebase_orig_head)) { > > - split_in_progress = 1; > > - } > > + char *head, *orig_head, *rebase_amend, *rebase_orig_head; > > + > > + if ((!s->amend && !s->nowarn && !s->workdir_dirty) || > > + !s->branch || strcmp(s->branch, "HEAD")) > > + return 0; > > - if (!s->amend && !s->nowarn && !s->workdir_dirty) > > - split_in_progress = 0; > > + head = read_line_from_git_path("HEAD"); > > + orig_head = read_line_from_git_path("ORIG_HEAD"); > > + rebase_amend = read_line_from_git_path("rebase-merge/amend"); > > + rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head"); > > Further improvement idea (for a later series): Use rebase_path_amend() > and rebase_path_orig_head() somehow, to build each path only once. > > Accessing the files HEAD and ORIG_HEAD directly seems odd. > > The second part of the function should probably be moved to sequencer.c. Sure. On all four accounts. > > + > > + if (!head || !orig_head || !rebase_amend || !rebase_orig_head) > > + ; /* fall through, no split in progress */ > > You could set split_in_progress to zero here. Would save a comment > without losing readability. But that would confuse e.g. myself, 6 months down the road: why assign that variable when it already has been assigned? (And we have to assign it because there is still a code path entering none of these if/else if's arms). > > + else if (!strcmp(rebase_amend, rebase_orig_head)) > > + split_in_progress = !!strcmp(head, rebase_amend); > > + else if (strcmp(orig_head, rebase_orig_head)) > > + split_in_progress = 1; > > > >free(head); > >free(orig_head); > >free(rebase_amend); > >free(rebase_orig_head); > > + > > Isn't the patch big enough already without rearranging the else blocks > and adding that blank line? :) The else blocks are not really rearranged; apart from the early return, the rest of the logic has been painstakingly kept in the same order. Ciao, Dscho
[noob] is this normal behavior
Setup: Running openindiana (hipster) a solaris 11 open source branch git 2.12.2 I've recently moved from mercurial to git mainly because git appears to receive the most development and the heaviest use. I'm using git with hold over behavior developed from my time using cvs long ago. I keep a hierarchy of directories and files that mirrors the structure on the OS where git is installed [just where ever tracked files are located on OS] not the entire structure. I keep a git repo on the same OS that is created by rsyncing the BUFFERED hierarchy into a git repo Many of the BUFFERED files are symlinked to there places in the OS Some are not because certain files being symlinked seemed to cause permissions type problems on the OS. In those cases they are periodically copied to the BUFFER hierarchy So, not to get too tangled up in explaining my setup... tracked files change in the buffer area or new files are created there. Periodically I rsync the files in BUFFER over the files in git repo And record the changes thru normal git operations git status git add (when needed) git commit , | Aside: | While I would welcome comments or suggestions about using a setup like | this, that is not the subject of this post. ` This is my most recent attempt at learning and using git but I have done a few tries before. I'm noticing behavior I do not recall seeing in earlier attempts. rsyncing the buffer over the git repo is now producing a result where `git status' shows any changed or added files as modified or new but all need to be added before a commit can take place, not just the new ones. Shouldn't files that changed but are already being tracked just show up as modified and not need adding? That is, if I have the file ~/.bashrc, which is really a symlink to BUFFER/OS/export/home/reader/.bashrc And is tracked in git repo as REPO/OS/export/home/reader/.bashrc When; BUFFER/OS/export/home/reader/.bashrc is edited. Then rsynced like below: BUFFER /OS/export/home/reader/.bashrc over GITREPO /OS/export/home/reader.bashrc Thereby changing the repo file in place Since that file is already being tracked; shouldn't `git status' show that file as modified but ready to be committed instead of a file that is modified but needs to be added before a commit can happen? Another side of this is that a `git diff FILE' only works before an `git add .' operation is done. That is, if I run `git diff FILE' AFTER `git add' .. no diff is reported, even though it is not committed yet. So, for example: if I'm committing and in the vi buffer of the commit and want to see a diff of FILE to aid my log notes. git diff FILE will report nothing whatever. Is that expected behavior?
RE: Add an option to automatically submodule update on checkout
On May 8, 2017 10:58 PM, Junio C Hamano wrote: >"Randall S. Becker" writes: >> I have to admit that I just assumed it would have to work that way >> this would not be particularly useful. However, in thinking about it, >> we might want to limit the depth of how far -b takes effect. If >> the super module brings in submodules entirely within control of the >> development group, having -b apply down to leaf submodules >> makes sense (in some policies). However, if some submodules span out >> to, say, gnulib, that might not make particular sense. >I do not see a strong reason to avoid your own branches in "other people's project" like this. >The submodule's upstream may be a project you have no control over, but the repository you have locally is under your total control and you can use >any branch names to suit the need of your project as the whole (i.e. the superproject and submodules bound to it). >The fact that local branch names are under your control and for your own use is true even when you are not using submodules, by the way. I agree with the technical aspects of this, but doing a checkout -b into something like gnulib will pin the code you are using in that submodule to whatever commit was referenced when you did the checkout. Example: In a situation like that, I would want gnulib to stay on 'master'. It is my opinion, FWIW, that this is a matter of policy or standards within the organization using git that we should not be imposing one way or another. In the current state of affairs (2.12.x), when I checkout, I make sure that people are aware of which branch each submodule is on because git won't go into the submodules - I'm fine with imposing that as a policy at present because it takes positive action by the developers and I keep the master branch on my own repositories locked down and it's obvious when they are accidentally on it. But we're talking changing this so that checkout branches can apply recursively. This changes the policy requirements so that people have to further act to undo what git will do by default on recursion. The policy will be at a high level the same (i.e., always make sure you know what branch you are on in submodules), but the implementation of it will need to be different (i.e., after you checkout recursive, go into each submodule and undo what git just did by checking out the default branch on some submodules ___ ___ ___, which depends on which super repository they are using, is onerous for me to manage, and for my developers to remember to do). With Respect, Randall
Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()
Hi Duy, On Tue, 9 May 2017, Duy Nguyen wrote: > Sorry for super late reply. I'm slowly catching up. > > On Wed, May 3, 2017 at 10:22 PM, Johannes Schindelin > wrote: > > Hi Duy, > > > > > > On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote: > > > >> There's plenty of error() in this code to safely assume --quiet is not a > >> concern. > >> > >> t5512 needs update because if we check the path 'refs*master' (i.e. the > >> asterisk is part of the path) then we'll get an EINVAL error. > > > > So the first change in this patch unmasks a bug that is fixed by the > > second patch? > > The change in read_branches_file() in this patch causes the failure. > See the original report [1], > > [1] http://public-inbox.org/git/3a2686c2-6766-1235-001f-1b5283b5f...@kdbg.org/ I disagree. I do not think that the first part of the patch causes the failure. I think the failure was always there, we just did not bother to report (and catch) it properly. Ciao, Dscho
[PATCH v3 4/6] t3901: move supporting files into t/t3901/
The current convention is to either generate files on the fly in tests, or to use supporting files taken from a t/t/ directory (where matches the test's number, or the number of the test from which we borrow supporting files). The test t3901-i18n-patch.sh was obviously introduced before that convention was in full swing, hence its supporting files still lived in t/t3901-8859-1.txt and t/t3901-utf8.txt, respectively. Let's adjust to the current convention. Signed-off-by: Johannes Schindelin --- t/t0203-gettext-setlocale-sanity.sh | 4 ++-- t/t3901-i18n-patch.sh| 38 t/{t3901-8859-1.txt => t3901/8859-1.txt} | 0 t/{t3901-utf8.txt => t3901/utf8.txt} | 0 t/t9350-fast-export.sh | 2 +- t/t9500-gitweb-standalone-no-errors.sh | 4 ++-- 6 files changed, 24 insertions(+), 24 deletions(-) rename t/{t3901-8859-1.txt => t3901/8859-1.txt} (100%) rename t/{t3901-utf8.txt => t3901/utf8.txt} (100%) diff --git a/t/t0203-gettext-setlocale-sanity.sh b/t/t0203-gettext-setlocale-sanity.sh index a2124600811..71b0d74b4dd 100755 --- a/t/t0203-gettext-setlocale-sanity.sh +++ b/t/t0203-gettext-setlocale-sanity.sh @@ -8,7 +8,7 @@ test_description="The Git C functions aren't broken by setlocale(3)" . ./lib-gettext.sh test_expect_success 'git show a ISO-8859-1 commit under C locale' ' - . "$TEST_DIRECTORY"/t3901-8859-1.txt && + . "$TEST_DIRECTORY"/t3901/8859-1.txt && test_commit "iso-c-commit" iso-under-c && git show >out 2>err && ! test -s err && @@ -16,7 +16,7 @@ test_expect_success 'git show a ISO-8859-1 commit under C locale' ' ' test_expect_success GETTEXT_LOCALE 'git show a ISO-8859-1 commit under a UTF-8 locale' ' - . "$TEST_DIRECTORY"/t3901-8859-1.txt && + . "$TEST_DIRECTORY"/t3901/8859-1.txt && test_commit "iso-utf8-commit" iso-under-utf8 && LANGUAGE=is LC_ALL="$is_IS_locale" git show >out 2>err && ! test -s err && diff --git a/t/t3901-i18n-patch.sh b/t/t3901-i18n-patch.sh index f663d567c8a..923eb01f0ea 100755 --- a/t/t3901-i18n-patch.sh +++ b/t/t3901-i18n-patch.sh @@ -31,7 +31,7 @@ test_expect_success setup ' # use UTF-8 in author and committer name to match the # i18n.commitencoding settings - . "$TEST_DIRECTORY"/t3901-utf8.txt && + . "$TEST_DIRECTORY"/t3901/utf8.txt && test_tick && echo "$GIT_AUTHOR_NAME" >mine && @@ -55,7 +55,7 @@ test_expect_success setup ' # the second one on the side branch is ISO-8859-1 git config i18n.commitencoding ISO8859-1 && # use author and committer name in ISO-8859-1 to match it. - . "$TEST_DIRECTORY"/t3901-8859-1.txt + . "$TEST_DIRECTORY"/t3901/8859-1.txt fi && test_tick && echo Yet another >theirs && @@ -100,7 +100,7 @@ test_expect_success 'rebase (U/U)' ' # The result will be committed by GIT_COMMITTER_NAME -- # we want UTF-8 encoded name. - . "$TEST_DIRECTORY"/t3901-utf8.txt && + . "$TEST_DIRECTORY"/t3901/utf8.txt && git checkout -b test && git rebase master && @@ -110,7 +110,7 @@ test_expect_success 'rebase (U/U)' ' test_expect_success 'rebase (U/L)' ' git config i18n.commitencoding UTF-8 && git config i18n.logoutputencoding ISO8859-1 && - . "$TEST_DIRECTORY"/t3901-utf8.txt && + . "$TEST_DIRECTORY"/t3901/utf8.txt && git reset --hard side && git rebase master && @@ -122,7 +122,7 @@ test_expect_success !MINGW 'rebase (L/L)' ' # In this test we want ISO-8859-1 encoded commits as the result git config i18n.commitencoding ISO8859-1 && git config i18n.logoutputencoding ISO8859-1 && - . "$TEST_DIRECTORY"/t3901-8859-1.txt && + . "$TEST_DIRECTORY"/t3901/8859-1.txt && git reset --hard side && git rebase master && @@ -135,7 +135,7 @@ test_expect_success !MINGW 'rebase (L/U)' ' # to get ISO-8859-1 results. git config i18n.commitencoding ISO8859-1 && git config i18n.logoutputencoding UTF-8 && - . "$TEST_DIRECTORY"/t3901-8859-1.txt && + . "$TEST_DIRECTORY"/t3901/8859-1.txt && git reset --hard side && git rebase master && @@ -148,7 +148,7 @@ test_expect_success 'cherry-pick(U/U)' ' git config i18n.commitencoding UTF-8 && git config i18n.logoutputencoding UTF-8 && - . "$TEST_DIRECTORY"/t3901-utf8.txt && + . "$TEST_DIRECTORY"/t3901/utf8.txt && git reset --hard master && git cherry-pick side^ && @@ -163,7 +163,7 @@ test_expect_success !MINGW 'cherry-pick(L/L)' ' git config i18n.commitencoding ISO8859-1 && git config i18n.logoutputencoding ISO8859-1 && - . "$TEST_DIRECTORY"/t3901-8859-1.txt && + . "$TEST_DIRECTORY"/t3901/8859-1.txt && git reset --hard master && git cherry-pick side^
[PATCH v3 3/6] completion: mark bash script as LF-only
Without this change, the completion script does not work, as Bash expects its scripts to have line feeds as end-of-line markers (this is particularly prominent in quoted multi-line strings, where carriage returns would slip into the strings as verbatim characters otherwise). This change is required to let t9902-completion pass when Git's source code is checked out with `core.autocrlf = true`. Signed-off-by: Johannes Schindelin Reviewed-by: Jonathan Nieder --- contrib/completion/.gitattributes | 1 + 1 file changed, 1 insertion(+) create mode 100644 contrib/completion/.gitattributes diff --git a/contrib/completion/.gitattributes b/contrib/completion/.gitattributes new file mode 100644 index 000..19116944c15 --- /dev/null +++ b/contrib/completion/.gitattributes @@ -0,0 +1 @@ +*.bash eol=lf -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 5/6] Fix the remaining tests that failed with core.autocrlf=true
The test suite is mainly developed on Linux and MacOSX, which is the reason that nobody thought to mark files as LF-only as needed. The symptom is a test suite that fails left and right when being checked out using Git for Windows (which defaults to core.autocrlf=true). Mostly, the problems stem from Git's (LF-only) output being compared to hard-coded files that are checked out with line endings according to core.autocrlf (which is of course incorrect). This includes the two test files in t/diff-lib/, README and COPYING. This patch can be validated even on Linux by using this cadence: git config core.autocrlf true rm .git/index && git stash make -j15 DEVELOPER=1 test Signed-off-by: Johannes Schindelin Reviewed-by: Jonathan Nieder --- t/.gitattributes | 21 - 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/t/.gitattributes b/t/.gitattributes index 2d44088f56e..11e5fe37281 100644 --- a/t/.gitattributes +++ b/t/.gitattributes @@ -1,2 +1,21 @@ t[0-9][0-9][0-9][0-9]/* -whitespace -t0110/url-* binary +/diff-lib/* eol=lf +/t0110/url-* binary +/t3900/*.txt eol=lf +/t3901/*.txt eol=lf +/t4034/*/* eol=lf +/t4013/* eol=lf +/t4018/* eol=lf +/t4100/* eol=lf +/t4101/* eol=lf +/t4109/* eol=lf +/t4110/* eol=lf +/t4135/* eol=lf +/t4211/* eol=lf +/t4252/* eol=lf +/t5100/* eol=lf +/t5515/* eol=lf +/t556x_common eol=lf +/t7500/* eol=lf +/t8005/*.txt eol=lf +/t9*/*.dump eol=lf -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 6/6] t4051: mark supporting files as requiring LF-only line endings
The test t4051-diff-function-context.sh passes on Linux when core.autocrlf=true even without marking its support files as LF-only, but they fail when core.autocrlf=true in Git for Windows' SDK. The reason is that `grep ... >file.c.new` will keep CR/LF line endings on Linux (obviously treating CRs as if they were regular characters), but will be converted to LF-only line endings with MSYS2's grep that is used in Git for Windows. As we do not want to validate the way the available `grep` works, let's just mark the input as LF-only and move on. Signed-off-by: Johannes Schindelin Reviewed-by: Jonathan Nieder --- t/.gitattributes | 1 + 1 file changed, 1 insertion(+) diff --git a/t/.gitattributes b/t/.gitattributes index 11e5fe37281..3bd959ae523 100644 --- a/t/.gitattributes +++ b/t/.gitattributes @@ -6,6 +6,7 @@ t[0-9][0-9][0-9][0-9]/* -whitespace /t4034/*/* eol=lf /t4013/* eol=lf /t4018/* eol=lf +/t4051/* eol=lf /t4100/* eol=lf /t4101/* eol=lf /t4109/* eol=lf -- 2.12.2.windows.2.800.gede8f145e06
Would this tool be useful - encoding repository data into single URL
Hello I wonder about usability of following tool. Quick-start: giturl https://github.com/zdharma/giturl -r devel -p lib/coding_functions.cpp Protocol: https Site: github.com Repo: zdharma/giturl Revision: devel File: lib/coding_functions.cpp gitu://ŬṽǚǫoŒẗ6ẏȅcЭÑẩőn4ầŘїệαЃȣϟṈӛŀї It does Huffman encoding and base-1024 encoding to pack given data into single URL. The Unicode characters selected for base-1024 encoding are letters, not symbols, so double-clicking in e.g. web browser selects the whole code, making it easy to grab a repository data. Decoding: giturl -qd ŬṽǚǫoŒẗ6ẏȅcЭÑẩőn4ầŘїệαЃȣϟṈӛŀї https://github.com/zdharma/giturl / rev:devel / file:lib/coding_functions.cpp I can also encode commits relative to given revision, e.g. bits 10011 are commits 1, 4, 5. Easy to add to the g-code. Selecting 10th commit is only 1 character in base-1024. However I wonder if this has any uses. Could be patches sent this way? Having refs/patches/, encoding in URL, sending it instead of inlining/attaching a diff, selecting e.g. 3 commits via the bit-mask mentioned. That said, it's more about easy-grab of repository data and storage in well-defined, consistent format, not in language "the branch is ..., commit a7a35cb". Does this make sense? There are 2 implementations, in Zsh (uses Zshell like e.g. Ruby, not interactively) and C++11 (mostly because of std::regex): https://github.com/zdharma/giturl https://github.com/zdharma/cgiturl -- Sebastian Gniazdowski psprint /at/ zdharma.org
[PATCH v3 1/6] Fix build with core.autocrlf=true
On Windows, the default line endings are denoted by a Carriage Return byte followed by a Line Feed byte, while Linux and MacOSX use a single Line Feed byte to denote a line ending. To help with this situation, Git introduced several mechanisms over the last decade, most prominently the `core.autocrlf` setting. Sometimes, however, a single setting is incorrect, e.g. when certain files in the source code are to be consumed by software that can handle only LF line endings, while other files can use whatever is appropriate for the current platform. To allow for that, Git added the `eol` option to its .gitattributes handling, expecting every user of Git to mark their source code appropriately. Bash assumes that line-endings of scripts are denoted by a single Line Feed byte. Therefore, shell scripts in Git's source code are one example where that `eol=lf` option is *required*. When generating common-cmds.h, the Unix tools we use generally operate on the assumption that input and output deliminate their lines using LF-only line endings. Consequently, they would happily copy the CR byte verbatim into the strings in common-cmds.h, which in turn makes the C preprocessor barf (that interprets them as MacOS-style line endings). Therefore, we have to mark the input files as LF-only: command-list.txt and Documentation/git-*.txt. Quite a bit belatedly, this patch brings Git's own source code in line with those expectations by setting those attributes to allow for a correct build even when core.autocrlf=true. This patch can be validated even on Linux, by using this cadence: git config core.autocrlf true rm .git/index && git stash make -j15 DEVELOPER=1 Signed-off-by: Johannes Schindelin Reviewed-by: Jonathan Nieder --- .gitattributes | 8 +++- git-gui/.gitattributes | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/.gitattributes b/.gitattributes index 320e33c327c..8ce9c6b 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,3 +1,9 @@ * whitespace=!indent,trail,space *.[ch] whitespace=indent,trail,space diff=cpp -*.sh whitespace=indent,trail,space +*.sh whitespace=indent,trail,space eol=lf +*.perl eol=lf +*.pm eol=lf +/Documentation/git-*.txt eol=lf +/command-list.txt eol=lf +/GIT-VERSION-GEN eol=lf +/mergetools/* eol=lf diff --git a/git-gui/.gitattributes b/git-gui/.gitattributes index 33d07c06bd9..59cd41dbff7 100644 --- a/git-gui/.gitattributes +++ b/git-gui/.gitattributes @@ -2,3 +2,4 @@ * encoding=US-ASCII git-gui.sh encoding=UTF-8 /po/*.poencoding=UTF-8 +/GIT-VERSION-GEN eol=lf -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 2/6] git-new-workdir: mark script as LF-only
Bash does not handle scripts with CR/LF line endings correctly, therefore they *have* to be forced to LF-only line endings. Funnily enough, this fixes t3000-ls-files-others and t1021-rerere-in-workdir when git.git was checked out with core.autocrlf=true, as these test still use git-new-workdir (once `git worktree` is no longer marked as experimental, both scripts probably want to be ported to using that command instead). Signed-off-by: Johannes Schindelin Reviewed-by: Jonathan Nieder --- contrib/workdir/.gitattributes | 1 + 1 file changed, 1 insertion(+) create mode 100644 contrib/workdir/.gitattributes diff --git a/contrib/workdir/.gitattributes b/contrib/workdir/.gitattributes new file mode 100644 index 000..1f78c5d1bd3 --- /dev/null +++ b/contrib/workdir/.gitattributes @@ -0,0 +1 @@ +/git-new-workdir eol=lf -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v3 0/6] Abide by our own rules regarding line endings
Over the past decade, there have been a couple of attempts to remedy the situation regarding line endings (Windows/DOS line endings are traditionally CR/LF even if many current applications can handle LF, too, Linux/MacOSX/*BSD/Unix uses LF-only line handlings, and old MacOS software used to use CR-only line endings). The current idea seems to be that the default line endings differ depending on the platform, and for files that should be exempt from that default, we strongly recommend using .gitattributes to define what the source code requires. It is time to heed our own recommendation and mark the files that require LF-only line endings in our very own .gitattributes. For starters, those files include shell scripts: the most prevalent shell interpreter in use (and certainly used in Git for Windows) is Bash, and Bash does not handle CR/LF line endings gracefully. There are even two shell scripts that are used in the test suite even if they are not technically supposed to be part of core Git, as indicated by their habitat inside contrib/: git-new-workdir and git-completion.bash. Related to shell scripts: when generating common-cmds.h, we use tools that generally operate on the assumption that input and output deliminate their lines using LF-only line endings. Consequently, they would happily copy the CR byte verbatim into the strings in common-cmds.h, which in turn makes the C preprocessor barf (that interprets them as MacOS-style line endings). Further, the most obvious required fixes concern tests' support files committed verbatim, to be compared to Git's output, which is always LF-only. This includes the two files in t/diff-lib/ (which does not, in fact contain library functions as suggested by its name, but a copy of the README and the COPYING files, specifically for use in the tests). There are a few SVN dump files, too, supporting the Subversion-related tests, requiring LF-only line endings. Without these fixes, Git will fail to build and pass the test suite, as can be verified even on Linux using this cadence: git config core.autocrlf true rm .git/index && git stash make DEVELOPER=1 -j15 test Note: I separated out the change marking t/t4051/* as LF-only into an individual commit for one reason: it would appear that some grep builds on Windows automagically convert CR/LF input into LF-only output. I went the easy route to work around this issue, as I do not want to bother changing MSYS2 grep's behavior. Changes since v2: - marked t/diff-lib/* as LF-only, dropping the ugly `tr -d "\015"` patch. Johannes Schindelin (6): Fix build with core.autocrlf=true git-new-workdir: mark script as LF-only completion: mark bash script as LF-only t3901: move supporting files into t/t3901/ Fix the remaining tests that failed with core.autocrlf=true t4051: mark supporting files as requiring LF-only line endings .gitattributes | 8 ++- contrib/completion/.gitattributes| 1 + contrib/workdir/.gitattributes | 1 + git-gui/.gitattributes | 1 + t/.gitattributes | 22 +- t/t0203-gettext-setlocale-sanity.sh | 4 ++-- t/t3901-i18n-patch.sh| 38 t/{t3901-8859-1.txt => t3901/8859-1.txt} | 0 t/{t3901-utf8.txt => t3901/utf8.txt} | 0 t/t9350-fast-export.sh | 2 +- t/t9500-gitweb-standalone-no-errors.sh | 4 ++-- 11 files changed, 55 insertions(+), 26 deletions(-) create mode 100644 contrib/completion/.gitattributes create mode 100644 contrib/workdir/.gitattributes rename t/{t3901-8859-1.txt => t3901/8859-1.txt} (100%) rename t/{t3901-utf8.txt => t3901/utf8.txt} (100%) base-commit: 9b669787fc6ebc527df9ad058c4bcaf46bacc267 Published-As: https://github.com/dscho/git/releases/tag/lf-attrs-v3 Fetch-It-Via: git fetch https://github.com/dscho/git lf-attrs-v3 Interdiff vs v2: diff --git a/t/.gitattributes b/t/.gitattributes index bdd82cf31f7..3bd959ae523 100644 --- a/t/.gitattributes +++ b/t/.gitattributes @@ -1,4 +1,5 @@ t[0-9][0-9][0-9][0-9]/* -whitespace +/diff-lib/* eol=lf /t0110/url-* binary /t3900/*.txt eol=lf /t3901/*.txt eol=lf diff --git a/t/t4003-diff-rename-1.sh b/t/t4003-diff-rename-1.sh index c3e0a3c3fc9..df2accb6555 100755 --- a/t/t4003-diff-rename-1.sh +++ b/t/t4003-diff-rename-1.sh @@ -11,7 +11,7 @@ test_description='More rename detection test_expect_success \ 'prepare reference tree' \ -'tr -d "\015" <"$TEST_DIRECTORY"/diff-lib/COPYING >COPYING && +'cat "$TEST_DIRECTORY"/diff-lib/COPYING >COPYING && echo frotz >rezrov && git update-index --add COPYING rezrov && tree=$(git write-tree) && @@ -99,7 +99,7 @@ test_expect_success \ test_expect_success \ 'prepare work tree once again' \ -'tr -d "\015" <"$TEST_DIRECTORY"/diff-lib/COPYING >COPYING && +'cat "$TEST_DIRECTORY"/diff-lib/COPYING >COPYI
Re: [PATCH 0/1] Preserve the untracked cache across checkout, reset --hard, etc
On 5/9/2017 1:02 AM, Junio C Hamano wrote: David Turner writes: Can you actually keep the email address as my Twopensource one? I want to make sure that Twitter, my employer at the time, gets credit for this work (just as I want to make sure that my current employer, Two Sigma, gets credit for my current work). Please feel free to add Signed-off-by: David Turner in case that makes tracking easier. Thanks. WRT the actual patch, I want to note that past me did not do a great job here. The tests do not correctly check that the post-checkout untracked cache is still valid after a checkout. For example, let's say that previously, the directory foo was entirely untracked (but it contained a file bar), but after the checkout, there is a file foo/baz. Does the untracked cache need to get updated? Unfortunately, the untracked cache is very unlikely to make it to the top of my priority list any time soon, so I won't be able to correct this test (and, if necessary, correct the code). But I would strongly suggest that the test be improved before this code is merged. Thanks for CCing me. I will try to find time to tweak what was sent to the list here to reflect your affiliations better, but marked with DONTMERGE waiting for the necessary updates you mentioned above, so that this change is not forgotten. It may turn out to be that copying from src to dst like the patch does is all that is needed, or the cache may need further invalidation when the copying happens, and I haven't got a good feeling that anybody who are familiar with the codepath vetted the correctness from seeing the discussion from sidelines (yet). Thanks. I've been looking into similar issues with the cache associated with using a file system monitor (aka Watchman) (https://github.com/git-for-windows/git/compare/master...benpeart:fsmonitor) to speed updating the index to reflect changes in the working directory. I can take a look and see if this patch results in valid results and reply to the thread or resubmit as needed. Ben
Re: Script to rebase branches
Hi Peff, On Tue, 9 May 2017, Jeff King wrote: > On Tue, May 09, 2017 at 12:50:22PM +0200, Johannes Schindelin wrote: > > > > This is what I use: > > > > > > https://github.com/peff/git/blob/meta/rebase > > > > > > There's no documentation in the script, but the commit message in its > > > history should give a good sense of what each part does. > > > > That requires Meta/ to be checked out and up-to-date. I'd bet there are > > exactly two people who fall into that category. > > Actually, it is not Junio's Meta that needs checked out, but rather the > "meta" branch where you will find that "rebase" script. Oh, okay, I misunderstood that part, indeed. > If other people find them useful, the set of scripts could perhaps be > transitioned to a namespace that is appropriate to go into people's > $PATH. > > I didn't really expect anybody to use it verbatim, though. I was > providing it more for inspiration. I deem it part of Git's mission is to avoid forcing everybody to write scripts so specific to their own needs that they cannot be shared easily. There are lots of parts in the interactive rebase, for example, that I do not want to use myself. But enough users do to make it worthwhile to support in Git proper. Likewise, I assume that most developers work on one topic branch at a time, until it is submitted as a Pull Request, and then move to the next topic branch after the previous has been merged. But there are enough users, I'd wager, including both you and me, who need to work on multiple topics in parallel (for a plethora of reasons). Certainly there will be more developers with the need to rebase multiple branches at once than there are developers needing to send patches via mail (and we support the latter operation with several top-level Git commands). And that makes me believe that there is enough need for multi-branch operations that we should consider supporting them out of the box. > > Also, I see that you do not use worktrees. Otherwise your script would > > fail. > > Yes, the script predates the invention of worktrees by several years. I > have occasionally played with worktrees, but don't use them extensively > (I'd usually use them for a one-off change, and then remove the > worktree). I assumed as much, and I did not mean to criticise you for it. I was just pointing out, implicitly, that the script has room for improvement to make it applicable to a broader audience. Personally, I am a heavy user of worktrees, and once the vexing bug that corrupts them by gc'ing in-use objects away. I kind of use them not only for developing the topic branches (as you know, my patch series frequently see many iterations and many weeks going by before they hit home), but also as kind of a Pensive, where I keep things I want to work on but do not have time right now. Some of my worktrees are both: they are topic branches in flight, and also remind me that a certain topic was not yet accepted. > > When I still hoped to be able to get the rebase--helper related topic > > branches in by August last year, I had grandiose plans to teach the > > sequencer not only to perform the Git garden shears' trick (i.e. > > recreate merges), but also to optionally update local branches > > corresponding to the merge commits, including updates to the worktrees > > that checked them out (if any). > > I don't think I need anything that fancy. But simply checking "is this > checked out in a worktree" for each branch and then doing "cd > /path/to/worktree && git rebase" instead of just "git rebase $branch" > would be enough, I think. > > (I'm assuming the problem you see is just that the directory running > Meta/rebase cannot check out a branch that is checked out in another > worktree). I understand that you do not need anything that fancy. And you probably understand that I was not talking about your needs only, nor mine. I was talking about a broader audience of power users with the need to work on multiple branches in parallel, to automate things in order to make work more fun. Kinda like Git, you know? Ciao, Dscho
Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
On Tue, May 9, 2017 at 12:40 PM, Johannes Schindelin wrote: > Hi, > > On Tue, 9 May 2017, brian m. carlson wrote: > >> On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote: >> > On Tue, May 9, 2017 at 1:32 AM, brian m. carlson >> > wrote: >> > > PCRE and PCRE2 also tend to have a lot of security updates, so I >> > > would prefer if we didn't import them into the tree. It is far >> > > better for users to use their distro's packages for PCRE, as it >> > > means they get automatic security updates even if they're using an >> > > old Git. >> > > >> > > We shouldn't consider shipping anything with a remotely frequent >> > > history of security updates in our tree, since people very >> > > frequently run old or ancient versions of Git. >> > >> > I'm aware of its security record[1], but I wonder what threat model >> > you have in mind here. I'm not aware of any parts of git (except maybe >> > gitweb?) where we take regexes from untrusted sources. >> > >> > I.e. yes there have been DoS's & even some overflow bugs leading code >> > execution in PCRE, but in the context of powering git-grep & git-log >> > with PCRE this falls into the "stop hitting yourself" category. >> >> Just because you don't drive Git with untrusted regexes doesn't mean >> other people don't. > > Or other applications. > >> It's not a good idea to require a stronger security model than we >> absolutely have to, since people can and will violate it. Think how >> devastating Shellshock was even though technically nobody should provide >> insecure environment variables to the shell. >> >> And, yes, gitweb does in fact call git grep. That means that git grep >> must in fact be secure against untrusted regexes, or you have a remote >> code execution vulnerability. > > And not only grep is affected. Think HEAD^{/}. There are plenty of > sites where you are allowed to specify revs in a freer form than SHA-1s. That will still use reg(comp|exec) for the foreseeable future. We have plenty of manual use of that all over the place: $ git grep 'reg(comp|exec)\(' *.[ch] builtin/*.[ch] And the ^{/rx} feature is powered by the one in sha1_name.c > Having said that, I do like the prospect of a faster git grep. > > Hopefully there will be a way to make use of PCRE that can be switched > off? Like, a compile-time replacement of the regex API backed by PCRE v2 > *iff* PCRE v2 is used for building? Yup, see my just-sent . It'll be optional for now, as it's been for a while. Aside from that I do think given these numbers it's worth considering making PCRE a default dependency, and possibly getting rid of stuff like kwset because a) it reduces the many codepaths we have now of either doing fixed/basic/extended/pcre into one b) since the numbers suggest pcre can support all of that faster that seems like a sensible thing to do. But anything like that will be a few patch series's down the road, for now I'm just making it all optional.
Re: [PATCH v2 7/7] t4051: mark supporting files as requiring LF-only line endings
Hi Junio, On Mon, 8 May 2017, Junio C Hamano wrote: > Johannes Schindelin writes: > > > The test t4051-diff-function-context.sh passes on Linux when > > core.autocrlf=true even without marking its support files as LF-only, > > but they fail when core.autocrlf=true in Git for Windows' SDK. > > > > The reason is that `grep ... >file.c.new` will keep CR/LF line endings > > on Linux (obviously treating CRs as if they were regular characters), > > but will be converted to LF-only line endings with MSYS2's grep that > > is used in Git for Windows. > > Ahem. > > I thought that according to your claim a UNIX tool like "grep" would > alway use LF endings? ;-) The maintainers of the MSYS2 grep package apparently disagree with me on that point. You should be familiar with that reaction. > > As we do not want to validate the way the available `grep` works, > > let's just mark the input as LF-only and move on. > > I agree with this conclusion; just like we do not want to worry about > how `grep` works when given CRLF files in this patch, we do not want to > worry about how other commands like `sh` works when given CRLF files. > And that is consistent with the overall theme of this series that marked > *.sh, *.perl and other files with eol=lf attribute. Good. That agreement is something on which you and I can build. > The only question I still have with this series is about the > README/COPYING thing. I _think_ it was my ancient mistake to use the > toplevel README and COPYING as test files, and that mistake was > corrected by somebody else earlier by having a frozen copy in > t/diff-lib/ and making tests use these files from that directory. If we > broke our tests to again use these files from outside t/diff-lib/, then > we would need to fix the tests not to do so. And if we are only looking > at t/diff-lib/ copy, then I think it is more consistent with the rest of > this series to mark them with eol=lf rather than passing them through > "tr -d '\015'". Thank you for pointing out that the README and COPYING files were reproduced in t/diff-lib/ specifically to serve as files for use in the tests. It had not really occurred to me, as I mistook this to be an extra anal licensing clarification for the diff-lib ;-) I will "re-roll" the patch series, dropping the ugly tr calls and marking t/diff-lib/* as LF-only, as you suggested. Ciao, Dscho
Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
On Tue, May 9, 2017 at 2:37 AM, brian m. carlson wrote: > On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote: >> On Tue, May 9, 2017 at 1:32 AM, brian m. carlson >> wrote: >> > PCRE and PCRE2 also tend to have a lot of security updates, so I would >> > prefer if we didn't import them into the tree. It is far better for >> > users to use their distro's packages for PCRE, as it means they get >> > automatic security updates even if they're using an old Git. >> > >> > We shouldn't consider shipping anything with a remotely frequent history >> > of security updates in our tree, since people very frequently run old or >> > ancient versions of Git. >> >> I'm aware of its security record[1], but I wonder what threat model >> you have in mind here. I'm not aware of any parts of git (except maybe >> gitweb?) where we take regexes from untrusted sources. >> >> I.e. yes there have been DoS's & even some overflow bugs leading code >> execution in PCRE, but in the context of powering git-grep & git-log >> with PCRE this falls into the "stop hitting yourself" category. > > Just because you don't drive Git with untrusted regexes doesn't mean > other people don't. It's not a good idea to require a stronger security > model than we absolutely have to, since people can and will violate it. > Think how devastating Shellshock was even though technically nobody > should provide insecure environment variables to the shell. Yes this is definitely something we should keep in mind. I see my comment could be read as dismissing your concerns, I didn't mean that. This is definitely something to be concerned about. I was trying to solicit feedback on whether there were any other parts of stock git that could take external input as regexes than gitweb, I'm not aware of any. I thought that gitweb wouldn't take regexes by default, but I see now that the 'search' feature is on by default, and that allows you to grep / pickaxe with regexes. Either -E or -F for grep, or --pickaxe-regex (which implies -E) for log. > And, yes, gitweb does in fact call git grep. That means that git grep > must in fact be secure against untrusted regexes, or you have a remote > code execution vulnerability. Indeed, but it's not as clear-cut as turning on on PCRE making you vulnerable. * gitweb is vulnerable to CPU DoS now in its default configuration. It's easy to provide an ERE that ends up slurping up 100% CPU for several seconds on any non-trivial sized repo, do that in parallel & you have a DoS vector. * Obviously something that's 2x as fast or more (which my WIP code is) makes this better. * PCRE tends to be worse at pathological patterns, but this is because it has really large limits by default and will try rather insanely hard to match your pattern. -DHEAP_LIMIT=2000 \ -DMATCH_LIMIT=1000 \ -DMATCH_LIMIT_DEPTH=1000 \ -DMAX_NAME_COUNT=1 \ -DMAX_NAME_SIZE=32 \ -DPARENS_NEST_LIMIT=250 \ This can be reduced dynamically via the API (and the patterns can't change it, except to reduce it). For example on my system 2.11.0 (before any of my changes) on linux.git: $ time git grep -E -- '-?-?-?-?-?-?-?-?-?-?-?---$' |wc -l 12434 real0m0.444s $ time git grep -P -- '-?-?-?-?-?-?-?-?-?-?-?---$' | wc -l 12434 real1m20.849s With my JIT changes: $ time ~/g/git/git --exec-path=/home/avar/g/git grep -P -- '-?-?-?-?-?-?-?-?-?-?-?---$' | wc -l 12434 real0m5.334s However for user-supplied patterns we can just turn on really conservative settings: $ time ~/g/git/git --exec-path=/home/avar/g/git grep -P -- '(*LIMIT_RECURSION=1000)(*LIMIT_MATCH=1000)-?-?-?-?-?-?-?-?-?-?-?---$' | wc -l fatal: pcre2_jit_match failed with error code -47: match limit exceeded 0 real0m0.021s When I locally compile git with something like this: - -DMATCH_LIMIT=1000 \ - -DMATCH_LIMIT_DEPTH=1000 \ - -DMATCH_LIMIT_RECURSION=1000 \ - -DMAX_NAME_COUNT=1 \ + -DMATCH_LIMIT=1000 \ + -DMATCH_LIMIT_DEPTH=1000 \ + -DMATCH_LIMIT_RECURSION=1000 \ + -DMAX_NAME_COUNT=100 \ -DMAX_NAME_SIZE=32 \ - -DPARENS_NEST_LIMIT=250 \ + -DPARENS_NEST_LIMIT=10 \ All tests still pass, and from my own testing I can't find any non-pathological patterns this would break. So we might actually consider turning this down for git by default. * Much of the rest of the patterns PCRE has CVEs for can similarly be mitigated by simply turning various features off. > Furthermore, at work we distribute Git with all releases of our product. > We normally only do non-security updates to the last couple of releases, > but we provide security updates to all supported versions. I'm not > comfortable shipping the entirety of PCRE
Re: Script to rebase branches
On Tue, May 09, 2017 at 12:50:22PM +0200, Johannes Schindelin wrote: > > This is what I use: > > > > https://github.com/peff/git/blob/meta/rebase > > > > There's no documentation in the script, but the commit message in its > > history should give a good sense of what each part does. > > That requires Meta/ to be checked out and up-to-date. I'd bet there are > exactly two people who fall into that category. Actually, it is not Junio's Meta that needs checked out, but rather the "meta" branch where you will find that "rebase" script. If other people find them useful, the set of scripts could perhaps be transitioned to a namespace that is appropriate to go into people's $PATH. I didn't really expect anybody to use it verbatim, though. I was providing it more for inspiration. > Also, I see that you do not use worktrees. Otherwise your script would > fail. Yes, the script predates the invention of worktrees by several years. I have occasionally played with worktrees, but don't use them extensively (I'd usually use them for a one-off change, and then remove the worktree). > When I still hoped to be able to get the rebase--helper related topic > branches in by August last year, I had grandiose plans to teach the > sequencer not only to perform the Git garden shears' trick (i.e. recreate > merges), but also to optionally update local branches corresponding to the > merge commits, including updates to the worktrees that checked them out > (if any). I don't think I need anything that fancy. But simply checking "is this checked out in a worktree" for each branch and then doing "cd /path/to/worktree && git rebase" instead of just "git rebase $branch" would be enough, I think. (I'm assuming the problem you see is just that the directory running Meta/rebase cannot check out a branch that is checked out in another worktree). -Peff
Re: Script to rebase branches
Hi Peff, On Tue, 9 May 2017, Jeff King wrote: > On Sat, May 06, 2017 at 12:23:32PM +0200, Lars Schneider wrote: > > > I am about to write a bash/sh script that helps me to rebase a bunch of > > branches (e.g. select branches based on prefix, conflict resolution/ > > rerere support, ...). > > > > I wonder if anyone has such a script already and is willing to share it. > > This is what I use: > > https://github.com/peff/git/blob/meta/rebase > > There's no documentation in the script, but the commit message in its > history should give a good sense of what each part does. That requires Meta/ to be checked out and up-to-date. I'd bet there are exactly two people who fall into that category. Also, I see that you do not use worktrees. Otherwise your script would fail. When I still hoped to be able to get the rebase--helper related topic branches in by August last year, I had grandiose plans to teach the sequencer not only to perform the Git garden shears' trick (i.e. recreate merges), but also to optionally update local branches corresponding to the merge commits, including updates to the worktrees that checked them out (if any). Maybe I'll still get around to do this, some time this year. And it'll probably hit git.git by mid next year ;-) Ciao, Dscho
Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
Hi, On Tue, 9 May 2017, brian m. carlson wrote: > On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote: > > On Tue, May 9, 2017 at 1:32 AM, brian m. carlson > > wrote: > > > PCRE and PCRE2 also tend to have a lot of security updates, so I > > > would prefer if we didn't import them into the tree. It is far > > > better for users to use their distro's packages for PCRE, as it > > > means they get automatic security updates even if they're using an > > > old Git. > > > > > > We shouldn't consider shipping anything with a remotely frequent > > > history of security updates in our tree, since people very > > > frequently run old or ancient versions of Git. > > > > I'm aware of its security record[1], but I wonder what threat model > > you have in mind here. I'm not aware of any parts of git (except maybe > > gitweb?) where we take regexes from untrusted sources. > > > > I.e. yes there have been DoS's & even some overflow bugs leading code > > execution in PCRE, but in the context of powering git-grep & git-log > > with PCRE this falls into the "stop hitting yourself" category. > > Just because you don't drive Git with untrusted regexes doesn't mean > other people don't. Or other applications. > It's not a good idea to require a stronger security model than we > absolutely have to, since people can and will violate it. Think how > devastating Shellshock was even though technically nobody should provide > insecure environment variables to the shell. > > And, yes, gitweb does in fact call git grep. That means that git grep > must in fact be secure against untrusted regexes, or you have a remote > code execution vulnerability. And not only grep is affected. Think HEAD^{/}. There are plenty of sites where you are allowed to specify revs in a freer form than SHA-1s. Having said that, I do like the prospect of a faster git grep. Hopefully there will be a way to make use of PCRE that can be switched off? Like, a compile-time replacement of the regex API backed by PCRE v2 *iff* PCRE v2 is used for building? Ciao, Dscho
Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning
Hi Ramsay, On Mon, 8 May 2017, Ramsay Jones wrote: > Commit dddbad728c ("timestamp_t: a new data type for timestamps", > 26-04-2017) introduced a new typedef 'timestamp_t', as a synonym for an > unsigned long, which was used at the time to represent timestamps in > git. A later commit 28f4aee3fb ("use uintmax_t for timestamps", > 26-04-2017) changed the typedef to use an 'uintmax_t' for the timestamp > representation type. > > When building on a 32-bit Linux system, sparse complains that a constant > (USTAR_MAX_MTIME) used to detect a 'far-future mtime' timestamp, is too > large; 'warning: constant 0777UL is so big it is unsigned long > long' on lines 335 and 338 of archive-tar.c. Note that both gcc and > clang only issue a warning if this constant is used in a context that > requires an 'unsigned long' (rather than an uintmax_t). (Since TIME_MAX > is no longer equal to 0x, even on a 32-bit system, the macro > USTAR_MAX_MTIME is set to 0777UL, which cannot be represented as > an 'unsigned long' constant). > > In order to suppress the warning, change the definition of the macro > constant USTAR_MAX_MTIME to use an 'ULL' type suffix. > > In a similar vein, on systems which use a 64-bit representation of the > 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with > the value 0777ULL. Although this does not cause any warning > messages to be issued, it would be more appropriate for this constant > to use an 'UL' type suffix rather than 'ULL'. The reason for the current situation is that an earlier fix for the USTAR_MAX_MTIME constant was applied to the USTAR_MAX_SIZE constant by mistake. > Signed-off-by: Ramsay Jones With that addition to the commit message: ACK Ciao, Dscho
Re: [PATCH v2 00/21]
On Sun, May 7, 2017 at 11:20 AM, Junio C Hamano wrote: > On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano wrote: >> >> Nguyễn Thái Ngọc Duy writes: >> >> > Changes since v1: >> > >> > - fopen_or_warn() and warn_on_fopen_errors() are introduced. The >> >latter's name is probably not great... >> > - A new patch (first one) to convert a bunch to using xfopen() >> > - Fix test failure on Windows, found by Johannes Sixt >> > - Fix the memory leak in log.c, found by Jeff >> > >> > There are still a couple of fopen() remained, but I'm getting slow >> > again so let's get these in first and worry about the rest when >> > somebody has time. > > Hmm, is this topic responsible for recent breakage Travis claims on MacOS X? > > https://travis-ci.org/git/git/jobs/229585098#L3091 > > seems to expect an error when test-config is fed a-directory but we are > not getting the expected warning and error? Sounds about right. Let me see if defining FREAD_READS_DIRECTORIES on MacOS X makes travis happy. -- Duy
Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()
Sorry for super late reply. I'm slowly catching up. On Wed, May 3, 2017 at 10:22 PM, Johannes Schindelin wrote: > Hi Duy, > > > On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote: > >> There's plenty of error() in this code to safely assume --quiet is not a >> concern. >> >> t5512 needs update because if we check the path 'refs*master' (i.e. the >> asterisk is part of the path) then we'll get an EINVAL error. > > So the first change in this patch unmasks a bug that is fixed by the > second patch? The change in read_branches_file() in this patch causes the failure. See the original report [1], [1] http://public-inbox.org/git/3a2686c2-6766-1235-001f-1b5283b5f...@kdbg.org/ -- Duy
Re: vger not relaying some of Junio's messages today?
Hi Brandon, On Mon, 8 May 2017, Brandon Williams wrote: > On 05/08, Johannes Schindelin wrote: > > > On Sat, 6 May 2017, Ævar Arnfjörð Bjarmason wrote: > > > > > I have one [script] to git am a patch from a msgid, thought I should > > > write something to handle a series in some DWIM fashion (e.g. apply > > > the latest continuous sequence of patches matching --author) but > > > figured that someone's probably wrote this already & I don't need to > > > hack it up myself... > > > > You probably missed my previous mails mentioning > > > > https://github.com/git-for-windows/build-extra/blob/master/apply-from-public-inbox.sh > > > > You can use this script to apply single patches (identified by their > > Message-ID), and patch series (identified by their cover letter's > > Message-ID). > > > > As I mentioned at the Contributors' Summit at GitMerge 2017: I would > > *love* to collaborate on tools that make any part of the > > contribution/review process less cumbersome than it is right now. > > Yeah its not the most streamlined process. I'm sure everyone writes > their own scripts (like I did) tailored to their workflow. I am sure you are right. What a waste of time, for everybody to come up with essentially the same sort of scripts, just to be able to participate. > For example I just tag a bunch of mails in mutt and then have a scripts > which 'git am's them on a branch/base of my choosing. But its specific > to my workflow so idk how useful it would be to others :( Hmm. So it looks more and more as if you *have* to use mutt in order to be rewarded with the option for an efficient workflow. I'm just so used to my good ole' Alpine. And others may be so used to their Thunderbird, Outlook, GMail, whatevs. But hey, maybe the vger woes will eventually become so bad that even mutt and NNTP users will be affected negatively. At that point, we may look into alternatives. Ciao, Dscho
Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required
On Tue, May 9, 2017 at 10:18 AM, Jean-Noël AVILA wrote: > Le jeudi 4 mai 2017, 10:14:58 CEST Kerry, Richard a écrit : >> >> My point was to ensure that where English is used on-screen it should make >> sense, which in this particular case it didn't (a French idiom which, on >> using an automatic translator, didn't make sense in English). The same of >> course applies to other languages used on-screen. >> >> I agree about ensuring that the application doesn't elicit a response that >> it won't, or can't, actually handle. A rhetorical question is fine, so long >> as it is clear that the program won't accept any further input. >> >> Though I don't agree about the issue of the length of words, as presented to >> a non-native speaker. Sometimes a longer word can be very specific in its >> meaning, and can be looked up in a dictionary if the reader is not familiar >> with it. Sometimes using shorter words can result in a less clear meaning, >> or perhaps be an idiomatic usage, which might be missed by a non-native >> speaker. >> > > Thanks. So what's the status of this patch series? I don't buy the idea of > rhetorical HMI. That's a sure way to confuse non-native speakers. Please note > that I kept the questions when there is a following text. Only questions > addressing the user at the end of output have been rephrased. > > For the "do you mean" questions, the proposition would then simply be: "the > most similar command is:" or "the most similar commands are:". > > and then what about the other patches? When you submit patches you can monitor the next "What's cooking" mail for the status. See "ja/do-not..." here: https://public-inbox.org/git/xmqqlgq77pse@gitster.mtv.corp.google.com/ It got picked up for the "pu" branch. You can fetch git.git and see it there. My feedback on the 3: * 1/3: Mostly covered above. I did notice after my last comment that every time gcc wants to suggest you should do something different (e.g. misspelled variable or macro) it'll say "did you mean?" similar to what git does now. While I think this is a rather tragic story of *nix usability ("user gets asked a question, types yes, gets a few GB/s of y as output") the main UX problem is surely that the user in question didn't understand from the terminal output when the program had exited & wasn't interactive anymore. But overall this seems like optimizing for a really obscure edge case at the expense of making the wording more clever. I don't think "did you mean?" will confuse non-native speakers, as the bug report shows the user in question has a reasonable command of English, they're fundimentally confused about how the shell interface works. * 2/3: Looks great, surprised it took so long for someone to remove that cutsey but bad message. * 3/3: I think this partly makes things slightly worse. I.e. now you get a specific error message about refs being missing, after it shows you the entire usage info, so you don't know if you e.g. misspelled a command-line flag or what. I couldn't find any pattern in the existing shell scripts for "print usage with custom message" thoug. >> Regards, >> Richard. >> >> >> >> >> Richard Kerry >> BNCS Engineer, SI SOL Telco & Media Vertical Practice >> T: +44 (0)20 3618 2669 >> M: +44 (0)7812 325518 >> 4 Triton Square, Regent’s Place, London NW1 3HG >> richard.ke...@atos.net >> >> >> This e-mail and the documents attached are confidential and intended solely >> for the addressee; it may also be privileged. If you receive this e-mail in >> error, please notify the sender immediately and destroy it. As its integrity >> cannot be secured on the Internet, the Atos group liability cannot be >> triggered for the message content. Although the sender endeavours to >> maintain a computer virus-free network, the sender does not warrant that >> this transmission is virus-free and will not be liable for any damages >> resulting from any virus transmitted. >> >> >> From: Ævar Arnfjörð Bjarmason [ava...@gmail.com] >> Sent: 04 May 2017 10:09 >> To: Kerry, Richard >> Cc: git@vger.kernel.org >> Subject: Re: [PATCH v2 1/3] usability: don't ask questions if no reply is >> required >> >> On Thu, May 4, 2017 at 10:52 AM, Kerry, Richard >> wrote: >> > >> > May I suggest that " The most approaching commands" doesn't make much >> > sense as English (I don't think a command can "approach"). >> > Perhaps it should be " The most appropriate commands". >> >> I had the same concern, saying "appropriate" is IMO also confusing. >> The point of this UI is not to point out what you should be running, >> which "appropriate" implies, but just "we couldn't find what you >> meant, did you mean one of these?". >> >> I think nothing needs to change here. The whole premise here is that a >> program should never ask a question when you can't give an answer, I >> think that's nonsense. There's such a thing as a rhetorical question, >> and sometimes using that form
Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required
Le jeudi 4 mai 2017, 10:14:58 CEST Kerry, Richard a écrit : > > My point was to ensure that where English is used on-screen it should make > sense, which in this particular case it didn't (a French idiom which, on > using an automatic translator, didn't make sense in English). The same of > course applies to other languages used on-screen. > > I agree about ensuring that the application doesn't elicit a response that it > won't, or can't, actually handle. A rhetorical question is fine, so long as > it is clear that the program won't accept any further input. > > Though I don't agree about the issue of the length of words, as presented to > a non-native speaker. Sometimes a longer word can be very specific in its > meaning, and can be looked up in a dictionary if the reader is not familiar > with it. Sometimes using shorter words can result in a less clear meaning, > or perhaps be an idiomatic usage, which might be missed by a non-native > speaker. > Thanks. So what's the status of this patch series? I don't buy the idea of rhetorical HMI. That's a sure way to confuse non-native speakers. Please note that I kept the questions when there is a following text. Only questions addressing the user at the end of output have been rephrased. For the "do you mean" questions, the proposition would then simply be: "the most similar command is:" or "the most similar commands are:". and then what about the other patches? Thanks > Regards, > Richard. > > > > > Richard Kerry > BNCS Engineer, SI SOL Telco & Media Vertical Practice > T: +44 (0)20 3618 2669 > M: +44 (0)7812 325518 > 4 Triton Square, Regent’s Place, London NW1 3HG > richard.ke...@atos.net > > > This e-mail and the documents attached are confidential and intended solely > for the addressee; it may also be privileged. If you receive this e-mail in > error, please notify the sender immediately and destroy it. As its integrity > cannot be secured on the Internet, the Atos group liability cannot be > triggered for the message content. Although the sender endeavours to maintain > a computer virus-free network, the sender does not warrant that this > transmission is virus-free and will not be liable for any damages resulting > from any virus transmitted. > > > From: Ævar Arnfjörð Bjarmason [ava...@gmail.com] > Sent: 04 May 2017 10:09 > To: Kerry, Richard > Cc: git@vger.kernel.org > Subject: Re: [PATCH v2 1/3] usability: don't ask questions if no reply is > required > > On Thu, May 4, 2017 at 10:52 AM, Kerry, Richard > wrote: > > > > May I suggest that " The most approaching commands" doesn't make much sense > > as English (I don't think a command can "approach"). > > Perhaps it should be " The most appropriate commands". > > I had the same concern, saying "appropriate" is IMO also confusing. > The point of this UI is not to point out what you should be running, > which "appropriate" implies, but just "we couldn't find what you > meant, did you mean one of these?". > > I think nothing needs to change here. The whole premise here is that a > program should never ask a question when you can't give an answer, I > think that's nonsense. There's such a thing as a rhetorical question, > and sometimes using that form is the most obvious & succinct way to > put things. > > Which is not to say that phrasing these things as a non-question can't > be better, but the suggestions so far just seem more complex. > > Also keep in mind that a huge part of the user base for git using the > English UI consists of non-native speakers, and when in doubt we > should definitely be picking simpler English like "did you mean?" v.s. > alternatives with >10 character more obscure words. > > > Richard Kerry > > BNCS Engineer, SI SOL Telco & Media Vertical Practice > > > > T: +44 (0)20 3618 2669 > > M: +44 (0)7812 325518 > > Lync: +44 (0) 20 3618 0778 > > Room G300, Stadium House, Wood Lane, London, W12 7TA > > richard.ke...@atos.net > > > > > > > > > > -Original Message- > > From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On > > Behalf Of Jean-Noel Avila > > Sent: Wednesday, May 03, 2017 10:07 PM > > To: git@vger.kernel.org > > Cc: rashmipa...@gmail.com; Jean-Noel Avila > > Subject: [PATCH v2 1/3] usability: don't ask questions if no reply is > > required > > > > There has been a bug report by a corporate user that stated that "spelling > > mistake of stash followed by a yes prints character 'y' > > infinite times." > > > > This analysis was false. When the spelling of a command contains errors, > > the git program tries to help the user by providing candidates which are > > close to the unexisting command. E.g Git prints the > > following: > > > > git: 'stahs' is not a git command. See 'git --help'. > > Did you mean this? > > > > stash > > > > and then exits. > > > > The problem with this hint is that it is not formally indicated as an hint > >
Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.
On Tue, May 09, 2017 at 09:58:28AM +0200, Ævar Arnfjörð Bjarmason wrote: > > Out of curiosity, how do you generate the patch-ids? Is it with > > something like diff-tree piped to patch-id? > > This: > > my $cmd = qq[git --git-dir="$repository_path" log --since="$since" > --until="$until" --all --pretty=format:%H --binary | git patch-id]; > open my $patch_id_fh, " $cmd |"; Ah, OK. I was specifically curious whether the decision to respect the config switch in plumbing would have any impact for your script. But it wouldn't, as it was already using log (though I suspect the real protection for your script is that it is used from a vanilla environment, not by random users). -Peff
Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.
On Tue, May 9, 2017 at 5:16 AM, Jeff King wrote: > On Mon, May 01, 2017 at 12:34:38PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > I don't know if we would want to be extra paranoid about patch-ids. >> > There is no helping: >> > >> > git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable >> > >> > because diff-tree doesn't know that it's trying for "--stable" output. >> > But the diffs we compute internally for patch-id could disable the >> > heuristics. I'm not sure if those matter, though. AFAIK those are used >> > only for internal comparisons within a single program. I.e., we never >> > compare them against input from the user, nor do we output them to the >> > user. So they'll change, but I don't think anybody would care. >> >> I have a few-million row table with commit_id as one column & patch_id >> as another. I.e. a commit -> patch_id mapping. > > Thanks for this data point. It's always interesting to hear about > unforeseen uses of the tools. > > Out of curiosity, how do you generate the patch-ids? Is it with > something like diff-tree piped to patch-id? This: my $cmd = qq[git --git-dir="$repository_path" log --since="$since" --until="$until" --all --pretty=format:%H --binary | git patch-id]; open my $patch_id_fh, " $cmd |"; Which is part of a loop that generates since/until for continuous pull/insertion. Also, a few lines later there's a workaround for the git.git bug of patch-id being ^0+$ (fixed in 2485eab55c ("git-patch-id: do not trip over "no newline" markers", 2011-02-17)), which gives you a sense of how long it's been since anyone's touched this. > I do feel a bit sad about breaking this case (or at the very least > forcing you to set an option to retain cross-version compatibility). But > my gut says that we don't want to lock ourselves into never changing the > diff algorithm (and I'm sure we've done it inadvertently a few times > over the years; even the recent switch to turning on renames would have > had that impact). As noted I think it's completely fine to change the patch-ids by changing the diff algorithm. I'm about to give some more detail on this in the other thread, but I find that on our repos the indent heuristic changes the patch-id for around 2% of patches, which seems fairly typical for non-changelog-y code. You *then* need to be using topic branches you didn't delete as well as having authored such a patch for this change to kick in, so the impact is really minimal. Even if it somehow changed 100% of the ids that would be fine too. It would auto-heal as the same git version started reading & inserting the ids, which are only relevant in a moving window.