* Re: [PATCH] Put sha1dc on a diet
From: Johannes Schindelin @ 2017-03-02 14:39 UTC (permalink / raw)
To: Jeff King
Cc: Linus Torvalds, Junio C Hamano, Marc Stevens, Dan Shumow,
Git Mailing List
In-Reply-To: <20170301231921.2puf7o7jkrujscwn@sigill.intra.peff.net>
Hi Peff,
On Wed, 1 Mar 2017, Jeff King wrote:
> I do think that could argue for turning on the collision detection only
> during object-write operations, which is where it matters. It would be
> really trivial to flip the "check collisions" bit on sha1dc. But I
> suspect you could go faster still by compiling against two separate
> implementations: the fast-as-possible one (which could be openssl or
> blk-sha1), and the slower-but-careful sha1dc.
Given the speed difference between OpenSSL and sha1dc, it would be a wise
thing indeed to do sha1dc only where objects enter from possibly untrusted
sources, and use OpenSSL for all other hashing.
Ciao,
Johannes
^ permalink raw reply
* Re: BUG Report: v12.0.0 "git difftool -d" fails with "fatal: cannot create directory at '': No such file or directory"
From: Johannes Schindelin @ 2017-03-02 14:26 UTC (permalink / raw)
To: Mark Phillips; +Cc: git@vger.kernel.org
In-Reply-To: <f2a8495f22854a749e615fb642d5655d@CAMSVWEXC02.scsc.local>
Hi Mark,
On Thu, 2 Mar 2017, Mark Phillips wrote:
> I am building git from source using gcc 4.8.5 on 64 bit linux.
>
> I am sorry the log info is not very helpful, please tell me how to get
> more information about what is going wrong and I will collect the info
> for you!
Well, you could provide a Minimal, Complete and Verifiable Example
(http://stackoverflow.com/help/mcve): most likely, the error depends on
particular features of your current HEAD, or environment variables.
Because i do *not* get that error if I run `git difftool -d HEAD~`.
So it is not so much about collecting info as about distilling your use
case into an example that allows other developers to reproduce the
problem, and subsequently fix it.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] README: create HTTP/HTTPS links from URLs in Markdown
From: Jeff King @ 2017-03-02 7:18 UTC (permalink / raw)
To: Eric Wong; +Cc: Junio C Hamano, git
In-Reply-To: <20170301222204.GA5413@whir>
On Wed, Mar 01, 2017 at 10:22:04PM +0000, Eric Wong wrote:
> Markdown supports automatic links by surrounding URLs with
> angle brackets, as documented in
> <https://daringfireball.net/projects/markdown/syntax#autolink>
One of the joys of markdown is that there are so many variants. A lot of
them (including GitHub-flavored markdown) will linkify URLs even when
they're not inside angle brackets.
So I don't mind this patch, but I'm curious what's rendering the
markdown you're seeing. I'd think online that one would either come
across the raw text, or the GFM from https://github.com/git/git.
-Peff
^ permalink raw reply
* Re: [PATCH v5 07/24] files-backend: add and use files_refname_path()
From: Duy Nguyen @ 2017-03-02 12:46 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <beef528b-66cd-e104-4c02-b07ddfa7e6d7@alum.mit.edu>
On Wed, Mar 1, 2017 at 12:41 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
>> Keep repo-related path handling in one place. This will make it easier
>> to add submodule/multiworktree support later.
>>
>> This automatically adds the "if submodule then use the submodule version
>> of git_path" to other call sites too. But it does not mean those
>> operations are sumodule-ready. Not yet.
>
> s/sumodule/submodule/
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> refs/files-backend.c | 45 +++++++++++++++++++++++++--------------------
>> 1 file changed, 25 insertions(+), 20 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 7b4ea4c56..72f4e1746 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -1180,6 +1180,18 @@ static void files_reflog_path(struct files_ref_store *refs,
>> strbuf_git_path(sb, "logs/%s", refname);
>> }
>>
>> +static void files_refname_path(struct files_ref_store *refs,
>> + struct strbuf *sb,
>> + const char *refname)
>> +{
>> + if (refs->submodule) {
>> + strbuf_git_path_submodule(sb, refs->submodule, "%s", refname);
>> + return;
>> + }
>> +
>> + strbuf_git_path(sb, "%s", refname);
>> +}
>
> Maybe it's just me, but I find it odd to exit early here when the first
> exit isn't due to an error. For me, structuring this like `if ()
> call1(); else call2();` would make it clearer that the two code paths
> are equally-valid alternatives, and either one or the other will be
> executed.
Its original version probably looked better. This is another case of
future patches influencing back the past ones: I structure the patch
so that in future we mostly add lines, or delete whole (in this case,
I believe), not modify a lot of lines. I think the readability does
not degrade too much though, so it's probably ok.
--
Duy
^ permalink raw reply
* Re: [PATCH v5 05/24] files-backend: move "logs/" out of TMP_RENAMED_LOG
From: Duy Nguyen @ 2017-03-02 13:07 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <cb86192d-8644-bed3-1e73-821931589e79@alum.mit.edu>
On Wed, Mar 1, 2017 at 12:19 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> @@ -2513,7 +2513,7 @@ static int files_delete_refs(struct ref_store *ref_store,
>> * IOW, to avoid cross device rename errors, the temporary renamed log must
>> * live into logs/refs.
>> */
>> -#define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
>> +#define TMP_RENAMED_LOG "refs/.tmp-renamed-log"
>
> The constant name feels a little bit misleading now that it is not the
> name of a logfile but rather a reference name. OTOH "tmp-renamed-log" is
> *in* the reference name so I guess it's not really wrong.
Heh.. I had a similar internal debate and almost renamed it to
tmp_renamed_refname. But then it's technically not a valid ref name
either (starting with a leading dot). My lazy side came in and
declared that doing nothing was always the right way.
>> struct rename_cb {
>> const char *tmp_renamed_log;
>> @@ -2549,7 +2549,7 @@ static int rename_tmp_log(const char *newrefname)
>> int ret;
>>
>> strbuf_git_path(&path, "logs/%s", newrefname);
>> - strbuf_git_path(&tmp, TMP_RENAMED_LOG);
>> + strbuf_git_path(&tmp, "logs/%s", TMP_RENAMED_LOG);
>> cb.tmp_renamed_log = tmp.buf;
>> ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb);
>> if (ret) {
>> @@ -2626,12 +2626,12 @@ static int files_rename_ref(struct ref_store *ref_store,
>> return 1;
>>
>> strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
>> - strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
>> + strbuf_git_path(&tmp_renamed_log, "logs/%s", TMP_RENAMED_LOG);
>> ret = log && rename(sb_oldref.buf, tmp_renamed_log.buf);
>> strbuf_release(&sb_oldref);
>> strbuf_release(&tmp_renamed_log);
>> if (ret)
>> - return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
>> + return error("unable to move logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
>> oldrefname, strerror(errno));
>
> It seems like it would be preferable to use `sb_oldref.buf` and
> `tmp.buf` when building the error message. But I guess that `tmp.buf`
> might include some path preceding "logs/" that is unwanted in the error
> message? But it's a shame to hardcode the file naming scheme here again.
>
> Maybe we *do* want the path in the error message?
It's an error, every piece of details matters. So yeah I'm inclined we
should print full path.
> It just occurred to me: this temporary logfile lives in the main
> repository, right? What if a worktree reference is being renamed? Part
> of the advertised use of worktrees is that the worktree might live far
> from the main directory, or even on removable media. But it's not
> possible to rename files across partitions. Maybe this will come out in
> the wash once worktrees are ref_stores themselves.
The actual working directory may be separated, but all the things that
belong to .git (even of a linked worktree) stay in the main worktree's
.git directory. And I don't think we ever support having a .git
directory on multiple partitions. You can rename refs freely even when
the worktree is on a detached removable drive.
> For that matter, what if a user tries to rename a worktree ref into a
> common ref or vice versa?
Interesting. It should work, it's just a
rename(".git/worktrees/blah/refs/bisect/good",
".git/refs/heads/saved") after the path translation done by
git_path().
--
Duy
^ permalink raw reply
* Re: [PATCH v5 08/24] files-backend: remove the use of git_path()
From: Duy Nguyen @ 2017-03-02 12:43 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <5c4a292e-ab86-ffe0-ba8d-c0fe1b527ec1@alum.mit.edu>
On Wed, Mar 1, 2017 at 12:50 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> @@ -995,7 +998,11 @@ static struct ref_store *files_ref_store_create(const char *submodule)
>> return ref_store;
>> }
>>
>> - refs->packed_refs_path = git_pathdup("packed-refs");
>> + refs->gitdir = xstrdup(gitdir);
>> + get_common_dir_noenv(&sb, gitdir);
>> + refs->gitcommondir = strbuf_detach(&sb, NULL);
>> + strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
>> + refs->packed_refs_path = strbuf_detach(&sb, NULL);
>
> `git_path()` and friends avoid adding an extra `/` if `git_dir()`
> already ends in a slash or if it is the empty string. Here you don't
> have that functionality. Is that intentional?
Kind of. I noticed that behavior but the thinking was, this $GIT_DIR
thing is going to get replaced soon anyway, and because the the uses
of these paths are very clear (in three groups) that avoiding
redundant slashes does not buy us anything extra, it's just more code.
--
Duy
^ permalink raw reply
* Re: [PATCH v5 24/24] t1406: new tests for submodule ref store
From: Duy Nguyen @ 2017-03-02 12:38 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <51f2ba5a-38c9-17eb-e845-3a84b65c7029@alum.mit.edu>
On Thu, Mar 2, 2017 at 3:16 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> The for-each-ref iteration is defined to be sorted by refname, but it's
> true that the for-each-reflog order is undefined.
Thanks. I'm going to add a few lines about this near for_each_ref and
for_each_reflog() then, assuming that this is the desired behavior,
not a bug. (Another patch, because I'm not happy until it gets 30+)
--
Duy
^ permalink raw reply
* Re: [PATCH v5 04/24] files-backend: convert git_path() to strbuf_git_path()
From: Duy Nguyen @ 2017-03-02 12:52 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <9074fcd9-2b80-8927-19d2-1659eb0e327c@alum.mit.edu>
On Wed, Mar 1, 2017 at 12:06 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> @@ -2681,13 +2709,19 @@ static int files_rename_ref(struct ref_store *ref_store,
>> log_all_ref_updates = flag;
>>
>> rollbacklog:
>> - if (logmoved && rename(git_path("logs/%s", newrefname), git_path("logs/%s", oldrefname)))
>> + strbuf_git_path(&sb_newref, "logs/%s", newrefname);
>> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
>> + if (logmoved && rename(sb_newref.buf, sb_oldref.buf))
>> error("unable to restore logfile %s from %s: %s",
>> oldrefname, newrefname, strerror(errno));
>> + strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
>> if (!logmoved && log &&
>> - rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldrefname)))
>> + rename(tmp_renamed_log.buf, sb_oldref.buf))
>> error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
>> oldrefname, strerror(errno));
>
> It feels like you're writing, releasing, re-writing these strbufs more
> than necessary. Maybe it would be clearer to set them once, and on
> errors set `ret = error()` then jump to a label here...
>
>> + strbuf_release(&sb_newref);
>> + strbuf_release(&sb_oldref);
>> + strbuf_release(&tmp_renamed_log);
>>
>
> ...and change this to `return ret`?
If that's ok with you, will do. I kept the patch dumb so the reader
does not have to keep track of many things when they check if there
are any behavior changes.
>> static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
>> {
>> + struct strbuf sb = STRBUF_INIT;
>> +
>> /* Check validity (but we don't need the result): */
>> files_downcast(ref_store, 0, "init_db");
>>
>> /*
>> * Create .git/refs/{heads,tags}
>> */
>> - safe_create_dir(git_path("refs/heads"), 1);
>> - safe_create_dir(git_path("refs/tags"), 1);
>> + strbuf_git_path(&sb, "refs/heads");
>> + safe_create_dir(sb.buf, 1);
>> + strbuf_reset(&sb);
>> + strbuf_git_path(&sb, "refs/tags");
>> + safe_create_dir(sb.buf, 1);
>> + strbuf_reset(&sb);
>> if (get_shared_repository()) {
>> - adjust_shared_perm(git_path("refs/heads"));
>> - adjust_shared_perm(git_path("refs/tags"));
>> + strbuf_git_path(&sb, "refs/heads");
>> + adjust_shared_perm(sb.buf);
>> + strbuf_reset(&sb);
>> + strbuf_git_path(&sb, "refs/tags");
>> + adjust_shared_perm(sb.buf);
>> }
>> + strbuf_release(&sb);
>> return 0;
>> }
>
> It looks to me like `safe_create_dir()` already has the ability to
> `adjust_shared_perm()`, or am I missing something? (I realize that this
> is preexisting code.)
Yeah you're right. I guess this code was written when
safe_create_dir() didn't do that. That certainly shortens this patch.
--
Duy
^ permalink raw reply
* Re: [PATCH v1] Travis: also test on 32-bit Linux
From: Johannes Schindelin @ 2017-03-02 11:24 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster
In-Reply-To: <20170302105157.59791-1-larsxschneider@gmail.com>
Hi Lars,
On Thu, 2 Mar 2017, Lars Schneider wrote:
> The patch looks good to me in general but I want to propose the following
> changes:
I know you are using your script to generate this mail, but I would have
liked to see v2 in the subject ;-)
> (1) Move all the docker magic into a dedicated file
> "ci/run-linux-32-build.sh" This way people should be able to run this
> build on their local machines without TravisCI. However, I haven't
> tested this.
I considered this, but there is serious overlap: the `docker pull` call
and the `docker run` call *have* to refer to the same image. It's very
easy for them to get out of sync if you have that information in two
files. Maybe make that an option of the script, defaulting to
daald/ubuntu32:xenial?
BTW speaking of Docker: it would be nicer if there was a Docker image that
already had the build-essentials installed, to save on startup time. But I
did not find any that was reasonably up-to-date.
> (2) The docker build command inherits the Git test environment
> variables. This way we use the same environment variables as in all
> other TravisCI builds (plus it would use *your* variables if you run it
> locally).
Good!
> (3) Silence the apt update/git output as is it clutters the log. I did
> not silence stderr output!
Also good!
> (4) Remove (to my knowledge) superfluous "compiler: clang" in the
> Linux32 job.
I copied that from one of your experimental .travis.yml patches ;-)
> I added my sign-off. I hope this is the right thing to do in this "I
> took your patch and changed it to suggest an improvement" situation.
Absolutely. Thank you for taking it from here.
> One thing that still bugs me: In the Linux32 environment prove adds the
> CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00
> csys ... Has anyone an idea why that happens and how we can disable it?
I have no idea.
> diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
> new file mode 100755
> index 0000000000..b892fbdc9e
> --- /dev/null
> +++ b/ci/run-linux32-build.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +#
> +# Build and test Git in a docker container running a 32-bit Ubuntu Linux
> +#
> +
> +set -e
Is this really necessary? I really like to avoid `set -e`, in particular
when we do pretty much everything in && chains anyway.
> +APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
> +"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
> +
> +TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
> +"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
> +"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
> +"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
> +
> +TEST_GIT_CMD="linux32 --32bit i386 sh -c '"\
> +"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
> +
> +sudo docker run \
> + --interactive --volume "${PWD}:/usr/src/git" \
> + daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
Hmm. Since it is a script now, it would be more readable this way, I
think:
sudo docker run --volume "${PWD}:/usr/src/git" "${1:-daald/ubuntu32:xenial}" \
linux32 --32bit i386 sh -c '
: update packages first &&
apt update >/dev/null &&
apt install -y build-essential libcurl4-openssl-dev libssl-dev \
libexpat-dev gettext python >/dev/null &&
: now build and test &&
cd /usr/src/git &&
DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
make -j2 test
'
This is completely untested (pun intended ;-))...
Ciao,
Dscho
^ permalink raw reply
* Re: git status --> Out of memory, realloc failed
From: Duy Nguyen @ 2017-03-02 9:31 UTC (permalink / raw)
To: Carsten Fuchs; +Cc: René Scharfe, Git Mailing List
In-Reply-To: <cbd281fc-3a4b-b4dc-5dff-145c97cd68d6@cafu.de>
On Thu, Mar 2, 2017 at 3:12 AM, Carsten Fuchs <carsten.fuchs@cafu.de> wrote:
>>> The repository is tracking about 19000 files which together take 260 MB.
>>> The git server version is 2.7.4.1.g5468f9e (Bitbucket)
>>
>>
>> Is your repository publicly accessible?
>
>
> Unfortunately, no. There are no big secrets in there, but just a couple of
> database details so that I cannot make it universally available. I can
> gladly give you access though. (E.g. by adding your public SSH key?)
You could also try "git fast-export --anonymize" (read the doc first).
I'm not sure if it keeps blob size though, and delta chain might also
matter so an anonymized repo may or may not share the same problem,
I'm not sure.
--
Duy
^ permalink raw reply
* BUG Report: v12.0.0 "git difftool -d" fails with "fatal: cannot create directory at '': No such file or directory"
From: Mark Phillips @ 2017-03-02 10:55 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <CGME20170302105511eucas1p2c40bbbb719c8fb15cc7a6cb9424a92e4@eucas1p2.samsung.com>
I am building git from source using gcc 4.8.5 on 64 bit linux.
I am sorry the log info is not very helpful, please tell me how to get more information about what is going wrong and I will collect the info for you!
V2.12.0 Broken log
==============
Export GIT_TRACE=1
git difftool -d HEAD~
09:11:27.797674 git.c:371 trace: built-in: git 'difftool' '-d' 'HEAD~'
09:11:27.798255 run-command.c:369 trace: run_command: 'diff' '--raw' '--no-abbrev' '-z' 'HEAD~'
09:11:27.798484 exec_cmd.c:118 trace: exec: 'git' 'diff' '--raw' '--no-abbrev' '-z' 'HEAD~'
09:11:27.800156 git.c:371 trace: built-in: git 'diff' '--raw' '--no-abbrev' '-z' 'HEAD~'
fatal: cannot create directory at '': No such file or directory
Running make check gives
fixed 0
success 14475
failed 0
broken 184
total 14804
Also broken in v2.12.0-rc0, v2.12.0-rc1 and v2.12.0-rc2
-----------------------------------------------------------------
V 2.11.1 works
===========
^ permalink raw reply
* [PATCH v1] Travis: also test on 32-bit Linux
From: Lars Schneider @ 2017-03-02 10:51 UTC (permalink / raw)
To: johannes.schindelin; +Cc: git, gitster
In-Reply-To: <c76a133a57514a332828099d342c9763fd946bfa.1488309430.git.johannes.schindelin@gmx.de>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When Git v2.9.1 was released, it had a bug that showed only on Windows
and on 32-bit systems: our assumption that `unsigned long` can hold
64-bit values turned out to be wrong.
This could have been caught earlier if we had a Continuous Testing
set up that includes a build and test run on 32-bit Linux.
Let's do this (and take care of the Windows build later). This patch
asks Travis CI to install a Docker image with 32-bit libraries and then
goes on to build and test Git using this 32-bit setup.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
Thanks for the patch Dscho!
The patch looks good to me in general but I want to propose the following
changes:
(1) Move all the docker magic into a dedicated file "ci/run-linux-32-build.sh"
This way people should be able to run this build on their local machines
without TravisCI. However, I haven't tested this.
(2) The docker build command inherits the Git test environment variables.
This way we use the same environment variables as in all other TravisCI
builds (plus it would use *your* variables if you run it locally).
(3) Silence the apt update/git output as is it clutters the log.
I did not silence stderr output!
(4) Remove (to my knowledge) superfluous "compiler: clang" in the Linux32 job.
I added my sign-off. I hope this is the right thing to do in this "I took your
patch and changed it to suggest an improvement" situation.
You can see a successful run here:
https://travis-ci.org/larsxschneider/git/jobs/206945950
One thing that still bugs me: In the Linux32 environment prove adds the
CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00 csys ...
Has anyone an idea why that happens and how we can disable it?
Cheers,
Lars
Notes:
Base Ref:
Web-Diff: https://github.com/larsxschneider/git/commit/82995ed59c
Checkout: git fetch https://github.com/larsxschneider/git travisci/linux32-v1 && git checkout 82995ed59c
.travis.yml | 9 +++++++++
ci/run-linux32-build.sh | 21 +++++++++++++++++++++
2 files changed, 30 insertions(+)
create mode 100755 ci/run-linux32-build.sh
diff --git a/.travis.yml b/.travis.yml
index 9c63c8c3f6..c8c789c437 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -39,6 +39,15 @@ env:
matrix:
include:
+ - env: Linux32
+ os: linux
+ sudo: required
+ services:
+ - docker
+ before_install:
+ - docker pull daald/ubuntu32:xenial
+ before_script:
+ script: ci/run-linux32-build.sh
- env: Documentation
os: linux
compiler: clang
diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
new file mode 100755
index 0000000000..b892fbdc9e
--- /dev/null
+++ b/ci/run-linux32-build.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+#
+# Build and test Git in a docker container running a 32-bit Ubuntu Linux
+#
+
+set -e
+
+APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
+"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
+
+TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
+"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
+"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
+"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
+
+TEST_GIT_CMD="linux32 --32bit i386 sh -c "\
+"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
+
+sudo docker run \
+ --interactive --volume "${PWD}:/usr/src/git" \
+ daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
base-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858
--
2.11.1
^ permalink raw reply related
* [PATCH] add--interactive: fix missing file prompt for patch mode with "-i"
From: Jeff King @ 2017-03-02 9:48 UTC (permalink / raw)
To: git; +Cc: Henrik Grubbström, Vasco Almeida
When invoked as "git add -i", each menu interactive menu
option prompts the user to select a list of files. This
includes the "patch" option, which gets the list before
starting the hunk-selection loop.
As "git add -p", it behaves differently, and jumps straight
to the hunk selection loop.
Since 0539d5e6d (i18n: add--interactive: mark patch prompt
for translation, 2016-12-14), the "add -i" case mistakenly
jumps to straight to the hunk-selection loop. Prior to that
commit the distinction between the two cases was managed by
the $patch_mode variable. That commit used $patch_mode for
something else, and moved the old meaning to the "$cmd"
variable. But it forgot to update the $patch_mode check
inside patch_update_cmd() which controls the file-list
behavior.
The simplest fix would be to change that line to check $cmd.
But while we're here, let's use a less obscure name for this
flag: $patch_mode_only, a boolean which tells whether we are
in full-interactive mode or only in patch-mode.
Reported-by: Henrik Grubbström <grubba@grubba.org>
Signed-off-by: Jeff King <peff@peff.net>
---
git-add--interactive.perl | 8 ++++----
t/t3701-add-interactive.sh | 18 ++++++++++++++++++
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 982593c89..f5c816e27 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -92,7 +92,7 @@ sub colored {
}
# command line options
-my $cmd;
+my $patch_mode_only;
my $patch_mode;
my $patch_mode_revision;
@@ -1299,7 +1299,7 @@ sub patch_update_cmd {
}
return 0;
}
- if ($patch_mode) {
+ if ($patch_mode_only) {
@them = @mods;
}
else {
@@ -1721,7 +1721,7 @@ sub process_args {
die sprintf(__("invalid argument %s, expecting --"),
$arg) unless $arg eq "--";
%patch_mode_flavour = %{$patch_modes{$patch_mode}};
- $cmd = 1;
+ $patch_mode_only = 1;
}
elsif ($arg ne "--") {
die sprintf(__("invalid argument %s, expecting --"), $arg);
@@ -1758,7 +1758,7 @@ sub main_loop {
process_args();
refresh();
-if ($cmd) {
+if ($patch_mode_only) {
patch_update_cmd();
}
else {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5ffe78e92..aaa258daa 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -394,4 +394,22 @@ test_expect_success 'diffs can be colorized' '
grep "$(printf "\\033")" output
'
+test_expect_success 'patch-mode via -i prompts for files' '
+ git reset --hard &&
+
+ echo one >file &&
+ echo two >test &&
+ git add -i <<-\EOF &&
+ patch
+ test
+
+ y
+ quit
+ EOF
+
+ echo test >expect &&
+ git diff --cached --name-only >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.12.0.367.gb23790f66
^ permalink raw reply related
* [PATCH v2 8/8] checkout: restrict @-expansions when finding branch
From: Jeff King @ 2017-03-02 8:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20170302082100.edaretznmlralswa@sigill.intra.peff.net>
When we parse "git checkout $NAME", we try to interpret
$NAME as a local branch-name. If it is, then we point HEAD
to that branch. Otherwise, we detach the HEAD at whatever
commit $NAME points to.
We do the interpretation by calling strbuf_branchname(), and
then blindly sticking "refs/heads/" on the front. This leads
to nonsense results when expansions like "@{upstream}" or
"@" point to something besides a local branch. We end up
with a local branch name like "refs/heads/origin/master" or
"refs/heads/HEAD".
Normally this has no user-visible effect because those
branches don't exist, and so we fallback to feeding the
result to get_sha1(), which resolves them correctly.
But as the new test in t3204 shows, there are corner cases
where the effect is observable, and we check out the wrong
local branch rather than detaching to the correct one.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/checkout.c | 2 +-
t/t3204-branch-name-interpretation.sh | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 05eefd994..81f07c3ef 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -452,7 +452,7 @@ static void setup_branch_path(struct branch_info *branch)
{
struct strbuf buf = STRBUF_INIT;
- strbuf_branchname(&buf, branch->name, 0);
+ strbuf_branchname(&buf, branch->name, INTERPRET_BRANCH_LOCAL);
if (strcmp(buf.buf, branch->name))
branch->name = xstrdup(buf.buf);
strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 05e88f92d..698d9cc4f 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -120,4 +120,14 @@ test_expect_success 'delete branch named "@"' '
expect_deleted refs/heads/@
'
+test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
+ git update-ref refs/remotes/origin/checkout one &&
+ git branch --set-upstream-to=origin/checkout &&
+ git update-ref refs/heads/origin/checkout two &&
+ git update-ref refs/heads/remotes/origin/checkout two &&
+
+ git checkout @{upstream} &&
+ expect_branch HEAD one
+'
+
test_done
--
2.12.0.367.gb23790f66
^ permalink raw reply related
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Jeff King @ 2017-03-02 8:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: tboegi, git, mac
In-Reply-To: <xmqqa894fyst.fsf@gitster.mtv.corp.google.com>
On Wed, Mar 01, 2017 at 01:54:26PM -0800, Junio C Hamano wrote:
> -- >8 --
> Subject: [PATCH] diff: do not short-cut CHECK_SIZE_ONLY check in diff_populate_filespec()
Thanks, this is well-explained, and the new comments in the code really
help.
I wondered if we should be checking would_convert_to_git() in
reuse_worktree_file(), but we already do. It's just that we may still
end up in this code-path when we're _actually_ diffing the working tree
file, not just trying to optimize.
> diff --git a/diff.c b/diff.c
> index 8c78fce49d..dc51dceb44 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2792,8 +2792,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
> s->should_free = 1;
> return 0;
> }
> - if (size_only)
> +
> + /*
> + * Even if the caller would be happy with getting
> + * only the size, we cannot return early at this
> + * point if the path requires us to run the content
> + * conversion.
> + */
> + if (!would_convert_to_git(s->path) && size_only)
> return 0;
The would_convert_to_git() function is a little expensive (it may have
to do an attribute lookup). It may be worth swapping the two halves of
the conditional here to get the short-circuit.
It may not matter much in practice, though, because in the !size_only
case we'd make the same query lower a few lines later (and in theory
expensive bits of the attr lookup are cached).
> +
> + /*
> + * Note: this check uses xsize_t(st.st_size) that may
> + * not be the true size of the blob after it goes
> + * through convert_to_git(). This may not strictly be
> + * correct, but the whole point of big_file_threashold
s/threashold/threshold/
> + * and is_binary check being that we want to avoid
> + * opening the file and inspecting the contents, this
> + * is probably fine.
> + */
> if ((flags & CHECK_BINARY) &&
> s->size > big_file_threshold && s->is_binary == -1) {
> s->is_binary = 1;
I'm trying to think how this "not strictly correct" could bite us. For
line-ending conversion, I'd say that the before/after are going to be
approximately the same size. But what about something like LFS? If I
have a 600MB file that convert_to_git() filters into a short LFS
pointer, I think this changes the behavior. Before, we would diff the
pointer file, but now we'll get "binary file changed".
I wonder if we should take the opposite approach, and ignore
big_file_threshold for converted files. One assumes that such gigantic
files are binary, and therefore do not have line endings to convert. And
any filtering has a reasonable chance of condensing them to something
much smaller.
I dunno. I'm sure somebody has some horrific 500MB-filtering example
that can prove me wrong.
-Peff
^ permalink raw reply
* Re: [PATCH v2] fixing corner-cases with interpret_branch_name()
From: Jacob Keller @ 2017-03-02 8:47 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, Junio C Hamano
In-Reply-To: <20170302082100.edaretznmlralswa@sigill.intra.peff.net>
On Thu, Mar 2, 2017 at 12:21 AM, Jeff King <peff@peff.net> wrote:
> This is a re-roll of the series from:
>
> http://public-inbox.org/git/20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net/
>
> Thanks Junio and Jake for reviewing the original. This is mostly the
> same, but:
>
> - it fixes the case where "branch -r @{-1}" mistakes a local branch
> for a remote (and adds a test)
>
> - as a result of the above fix, the series needs to be applied on top
> of jk/auto-namelen-in-interpret-branch-name.
>
> - I clarified the history in the commit message of patch 4
>
> - the commit message for patch 4 now explicitly mentions which
> callers can be left alone (so anybody blaming the history won't
> think they were simply forgotten).
>
> With the exception of patch 6 flipping the failure/success bit on the
> new test, the rest of the patches should be identical.
>
I didn't find any comments, it was quite pleasant and well explained.
Thanks,
Jake
> [1/8]: interpret_branch_name: move docstring to header file
> [2/8]: strbuf_branchname: drop return value
> [3/8]: strbuf_branchname: add docstring
> [4/8]: interpret_branch_name: allow callers to restrict expansions
> [5/8]: t3204: test git-branch @-expansion corner cases
> [6/8]: branch: restrict @-expansions when deleting
> [7/8]: strbuf_check_ref_format(): expand only local branches
> [8/8]: checkout: restrict @-expansions when finding branch
>
> builtin/branch.c | 5 +-
> builtin/checkout.c | 2 +-
> builtin/merge.c | 2 +-
> cache.h | 32 +++++++-
> refs.c | 2 +-
> revision.c | 2 +-
> sha1_name.c | 92 ++++++++++++-----------
> strbuf.h | 21 +++++-
> t/t3204-branch-name-interpretation.sh | 133 ++++++++++++++++++++++++++++++++++
> 9 files changed, 240 insertions(+), 51 deletions(-)
> create mode 100755 t/t3204-branch-name-interpretation.sh
>
> -Peff
^ permalink raw reply
* Re: [PATCH] README: create HTTP/HTTPS links from URLs in Markdown
From: Eric Wong @ 2017-03-02 7:57 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20170302073708.dxm4p24pt6zlqjuz@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> On Thu, Mar 02, 2017 at 07:34:21AM +0000, Eric Wong wrote:
> > Jeff King <peff@peff.net> wrote:
> > > On Wed, Mar 01, 2017 at 10:22:04PM +0000, Eric Wong wrote:
> > >
> > > > Markdown supports automatic links by surrounding URLs with
> > > > angle brackets, as documented in
> > > > <https://daringfireball.net/projects/markdown/syntax#autolink>
> > >
> > > One of the joys of markdown is that there are so many variants. A lot of
> > > them (including GitHub-flavored markdown) will linkify URLs even when
> > > they're not inside angle brackets.
> > >
> > > So I don't mind this patch, but I'm curious what's rendering the
> > > markdown you're seeing. I'd think online that one would either come
> > > across the raw text, or the GFM from https://github.com/git/git.
> >
> > I was using Gruber's reference implementation from Debian stable
> > (1.0.1-7).
>
> OK. I guess my question more was "why are you doing that?". I'd expect
> people to find the GFM rendering on GitHub, or just look at the text via
> "less".
Actually, I was initially seeing the lack of trailing slash
causing unnecessary 301 redirects on public-inbox.org.
So, I added the trailing slash and ran "markdown <README.md"
to check my work.
Then, I realized links weren't generated at all without angle
brackets. So down the rabbit hole I went to read Gruber's syntax
document and linkifying the rest for compatibility with Gruber's
implementation.
> But it's not really my business why you would want to do it. :) It's
> reasonable for us to cater to the common subset of renderers.
:)
I figure somebody unfamiliar with Markdown editing README.md is
likely to run the original implementation locally to check their
work, as I did.
(hmm... and vger is unusually slow this week)
^ permalink raw reply
* [PATCH v2 7/8] strbuf_check_ref_format(): expand only local branches
From: Jeff King @ 2017-03-02 8:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20170302082100.edaretznmlralswa@sigill.intra.peff.net>
This function asks strbuf_branchname() to expand any @-marks
in the branchname, and then we blindly stick refs/heads/ in
front of the result. This is obviously nonsense if the
expansion is "HEAD" or a ref in refs/remotes/.
The most obvious end-user effect is that creating or
renaming a branch with an expansion may have confusing
results (e.g., creating refs/heads/origin/master from
"@{upstream}" when the operation should be disallowed).
We can fix this by telling strbuf_branchname() that we are
only interested in local expansions. Any unexpanded bits are
then fed to check_ref_format(), which either disallows them
(in the case of "@{upstream}") or lets them through
("refs/heads/@" is technically valid, if a bit silly).
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 2 +-
t/t3204-branch-name-interpretation.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 7f754b60c..26ceec1d7 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1319,7 +1319,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
{
- strbuf_branchname(sb, name, 0);
+ strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
if (name[0] == '-')
return -1;
strbuf_splice(sb, 0, 0, "refs/heads/", 11);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 4f4af1fb4..05e88f92d 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -42,7 +42,7 @@ test_expect_success 'update branch via local @{upstream}' '
expect_branch local two
'
-test_expect_failure 'disallow updating branch via remote @{upstream}' '
+test_expect_success 'disallow updating branch via remote @{upstream}' '
git update-ref refs/remotes/origin/remote one &&
git branch --set-upstream-to=origin/remote &&
@@ -109,7 +109,7 @@ test_expect_success 'disallow deleting remote branch via @{-1}' '
# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a
# sane thing, but it _is_ technically allowed for now. If we disallow it, these
# can be switched to test_must_fail.
-test_expect_failure 'create branch named "@"' '
+test_expect_success 'create branch named "@"' '
git branch -f @ one &&
expect_branch refs/heads/@ one
'
--
2.12.0.367.gb23790f66
^ permalink raw reply related
* [PATCH v2 6/8] branch: restrict @-expansions when deleting
From: Jeff King @ 2017-03-02 8:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20170302082100.edaretznmlralswa@sigill.intra.peff.net>
We use strbuf_branchname() to expand the branch name from
the command line, so you can delete the branch given by
@{-1}, for example. However, we allow other nonsense like
"@", and we do not respect our "-r" flag (so we may end up
deleting an oddly-named local ref instead of a remote one).
We can fix this by passing the appropriate "allowed" flag to
strbuf_branchname().
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/branch.c | 5 ++++-
t/t3204-branch-name-interpretation.sh | 6 +++---
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index cf0ece55d..291fe90de 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -191,17 +191,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
int ret = 0;
int remote_branch = 0;
struct strbuf bname = STRBUF_INIT;
+ unsigned allowed_interpret;
switch (kinds) {
case FILTER_REFS_REMOTES:
fmt = "refs/remotes/%s";
/* For subsequent UI messages */
remote_branch = 1;
+ allowed_interpret = INTERPRET_BRANCH_REMOTE;
force = 1;
break;
case FILTER_REFS_BRANCHES:
fmt = "refs/heads/%s";
+ allowed_interpret = INTERPRET_BRANCH_LOCAL;
break;
default:
die(_("cannot use -a with -d"));
@@ -216,7 +219,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
char *target = NULL;
int flags = 0;
- strbuf_branchname(&bname, argv[i], 0);
+ strbuf_branchname(&bname, argv[i], allowed_interpret);
free(name);
name = mkpathdup(fmt, bname.buf);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index e671a7a64..4f4af1fb4 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -83,7 +83,7 @@ test_expect_success 'delete branch via remote @{upstream}' '
# Note that we create two oddly named local branches here. We want to make
# sure that we do not accidentally delete either of them, even if
# shorten_unambiguous_ref() tweaks the name to avoid ambiguity.
-test_expect_failure 'delete @{upstream} expansion matches -r option' '
+test_expect_success 'delete @{upstream} expansion matches -r option' '
git update-ref refs/remotes/origin/remote-del two &&
git branch --set-upstream-to=origin/remote-del &&
git update-ref refs/heads/origin/remote-del two &&
@@ -94,7 +94,7 @@ test_expect_failure 'delete @{upstream} expansion matches -r option' '
expect_branch refs/heads/remotes/origin/remote-del two
'
-test_expect_failure 'disallow deleting remote branch via @{-1}' '
+test_expect_success 'disallow deleting remote branch via @{-1}' '
git update-ref refs/remotes/origin/previous one &&
git checkout -b origin/previous two &&
@@ -114,7 +114,7 @@ test_expect_failure 'create branch named "@"' '
expect_branch refs/heads/@ one
'
-test_expect_failure 'delete branch named "@"' '
+test_expect_success 'delete branch named "@"' '
git update-ref refs/heads/@ two &&
git branch -D @ &&
expect_deleted refs/heads/@
--
2.12.0.367.gb23790f66
^ permalink raw reply related
* [PATCH v2 5/8] t3204: test git-branch @-expansion corner cases
From: Jeff King @ 2017-03-02 8:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20170302082100.edaretznmlralswa@sigill.intra.peff.net>
git-branch feeds the branch names from the command line to
strbuf_branchname(), but we do not yet tell that function
which kinds of expansions should be allowed. Let's create a
set of tests that cover both the allowed and disallowed
cases.
That shows off some breakages where we currently create or
delete the wrong ref (and will make sure that we do not
break any cases that _should_ be working when we do add more
restrictions).
Note that we check branch creation and deletion, but do not
bother with renames. Those follow the same code path as
creation.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t3204-branch-name-interpretation.sh | 123 ++++++++++++++++++++++++++++++++++
1 file changed, 123 insertions(+)
create mode 100755 t/t3204-branch-name-interpretation.sh
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
new file mode 100755
index 000000000..e671a7a64
--- /dev/null
+++ b/t/t3204-branch-name-interpretation.sh
@@ -0,0 +1,123 @@
+#!/bin/sh
+
+test_description='interpreting exotic branch name arguments
+
+Branch name arguments are usually names which are taken to be inside of
+refs/heads/, but we interpret some magic syntax like @{-1}, @{upstream}, etc.
+This script aims to check the behavior of those corner cases.
+'
+. ./test-lib.sh
+
+expect_branch() {
+ git log -1 --format=%s "$1" >actual &&
+ echo "$2" >expect &&
+ test_cmp expect actual
+}
+
+expect_deleted() {
+ test_must_fail git rev-parse --verify "$1"
+}
+
+test_expect_success 'set up repo' '
+ test_commit one &&
+ test_commit two &&
+ git remote add origin foo.git
+'
+
+test_expect_success 'update branch via @{-1}' '
+ git branch previous one &&
+
+ git checkout previous &&
+ git checkout master &&
+
+ git branch -f @{-1} two &&
+ expect_branch previous two
+'
+
+test_expect_success 'update branch via local @{upstream}' '
+ git branch local one &&
+ git branch --set-upstream-to=local &&
+
+ git branch -f @{upstream} two &&
+ expect_branch local two
+'
+
+test_expect_failure 'disallow updating branch via remote @{upstream}' '
+ git update-ref refs/remotes/origin/remote one &&
+ git branch --set-upstream-to=origin/remote &&
+
+ test_must_fail git branch -f @{upstream} two
+'
+
+test_expect_success 'create branch with pseudo-qualified name' '
+ git branch refs/heads/qualified two &&
+ expect_branch refs/heads/refs/heads/qualified two
+'
+
+test_expect_success 'delete branch via @{-1}' '
+ git branch previous-del &&
+
+ git checkout previous-del &&
+ git checkout master &&
+
+ git branch -D @{-1} &&
+ expect_deleted previous-del
+'
+
+test_expect_success 'delete branch via local @{upstream}' '
+ git branch local-del &&
+ git branch --set-upstream-to=local-del &&
+
+ git branch -D @{upstream} &&
+ expect_deleted local-del
+'
+
+test_expect_success 'delete branch via remote @{upstream}' '
+ git update-ref refs/remotes/origin/remote-del two &&
+ git branch --set-upstream-to=origin/remote-del &&
+
+ git branch -r -D @{upstream} &&
+ expect_deleted origin/remote-del
+'
+
+# Note that we create two oddly named local branches here. We want to make
+# sure that we do not accidentally delete either of them, even if
+# shorten_unambiguous_ref() tweaks the name to avoid ambiguity.
+test_expect_failure 'delete @{upstream} expansion matches -r option' '
+ git update-ref refs/remotes/origin/remote-del two &&
+ git branch --set-upstream-to=origin/remote-del &&
+ git update-ref refs/heads/origin/remote-del two &&
+ git update-ref refs/heads/remotes/origin/remote-del two &&
+
+ test_must_fail git branch -D @{upstream} &&
+ expect_branch refs/heads/origin/remote-del two &&
+ expect_branch refs/heads/remotes/origin/remote-del two
+'
+
+test_expect_failure 'disallow deleting remote branch via @{-1}' '
+ git update-ref refs/remotes/origin/previous one &&
+
+ git checkout -b origin/previous two &&
+ git checkout master &&
+
+ test_must_fail git branch -r -D @{-1} &&
+ expect_branch refs/remotes/origin/previous one &&
+ expect_branch refs/heads/origin/previous two
+'
+
+# The thing we are testing here is that "@" is the real branch refs/heads/@,
+# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a
+# sane thing, but it _is_ technically allowed for now. If we disallow it, these
+# can be switched to test_must_fail.
+test_expect_failure 'create branch named "@"' '
+ git branch -f @ one &&
+ expect_branch refs/heads/@ one
+'
+
+test_expect_failure 'delete branch named "@"' '
+ git update-ref refs/heads/@ two &&
+ git branch -D @ &&
+ expect_deleted refs/heads/@
+'
+
+test_done
--
2.12.0.367.gb23790f66
^ permalink raw reply related
* [PATCH v2 4/8] interpret_branch_name: allow callers to restrict expansions
From: Jeff King @ 2017-03-02 8:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20170302082100.edaretznmlralswa@sigill.intra.peff.net>
The interpret_branch_name() function converts names like
@{-1} and @{upstream} into branch names. The expanded ref
names are not fully qualified, and may be outside of the
refs/heads/ namespace (e.g., "@" expands to "HEAD", and
"@{upstream}" is likely to be in "refs/remotes/").
This is OK for callers like dwim_ref() which are primarily
interested in resolving the resulting name, no matter where
it is. But callers like "git branch" treat the result as a
branch name in refs/heads/. When we expand to a ref outside
that namespace, the results are very confusing (e.g., "git
branch @" tries to create refs/heads/HEAD, which is
nonsense).
Callers can't know from the returned string how the
expansion happened (e.g., did the user really ask for a
branch named "HEAD", or did we do a bogus expansion?). One
fix would be to return some out-parameters describing the
types of expansion that occurred. This has the benefit that
the caller can generate precise error messages ("I
understood @{upstream} to mean origin/master, but that is a
remote tracking branch, so you cannot create it as a local
name").
However, out-parameters make the function interface somewhat
cumbersome. Instead, let's do the opposite: let the caller
tell us which elements to expand. That's easier to pass in,
and none of the callers give more precise error messages
than "@{upstream} isn't a valid branch name" anyway (which
should be sufficient).
The strbuf_branchname() function needs a similar parameter,
as most of the callers access interpret_branch_name()
through it.
We can break the callers down into two groups:
1. Callers that are happy with any kind of ref in the
result. We pass "0" here, so they continue to work
without restrictions. This includes merge_name(),
the reflog handling in add_pending_object_with_path(),
and substitute_branch_name(). This last is what powers
dwim_ref().
2. Callers that have funny corner cases (mostly in
git-branch and git-checkout). These need to make use of
the new parameter, but I've left them as "0" in this
patch, and will address them individually in follow-on
patches.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/branch.c | 2 +-
builtin/checkout.c | 2 +-
builtin/merge.c | 2 +-
cache.h | 13 +++++++++--
refs.c | 2 +-
revision.c | 2 +-
sha1_name.c | 68 ++++++++++++++++++++++++++++++++++++++----------------
strbuf.h | 6 ++++-
8 files changed, 69 insertions(+), 28 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 94f7de7fa..cf0ece55d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -216,7 +216,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
char *target = NULL;
int flags = 0;
- strbuf_branchname(&bname, argv[i]);
+ strbuf_branchname(&bname, argv[i], 0);
free(name);
name = mkpathdup(fmt, bname.buf);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f174f5030..05eefd994 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -452,7 +452,7 @@ static void setup_branch_path(struct branch_info *branch)
{
struct strbuf buf = STRBUF_INIT;
- strbuf_branchname(&buf, branch->name);
+ strbuf_branchname(&buf, branch->name, 0);
if (strcmp(buf.buf, branch->name))
branch->name = xstrdup(buf.buf);
strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
diff --git a/builtin/merge.c b/builtin/merge.c
index a96d4fb50..848a29855 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -438,7 +438,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
char *found_ref;
int len, early;
- strbuf_branchname(&bname, remote);
+ strbuf_branchname(&bname, remote, 0);
remote = bname.buf;
memset(branch_head, 0, sizeof(branch_head));
diff --git a/cache.h b/cache.h
index c67995caa..a8816c914 100644
--- a/cache.h
+++ b/cache.h
@@ -1383,8 +1383,17 @@ extern char *oid_to_hex(const struct object_id *oid); /* same static buffer as s
*
* If the input was ok but there are not N branch switches in the
* reflog, it returns 0.
- */
-extern int interpret_branch_name(const char *str, int len, struct strbuf *);
+ *
+ * If "allowed" is non-zero, it is a treated as a bitfield of allowable
+ * expansions: local branches ("refs/heads/"), remote branches
+ * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
+ * allowed, even ones to refs outside of those namespaces.
+ */
+#define INTERPRET_BRANCH_LOCAL (1<<0)
+#define INTERPRET_BRANCH_REMOTE (1<<1)
+#define INTERPRET_BRANCH_HEAD (1<<2)
+extern int interpret_branch_name(const char *str, int len, struct strbuf *,
+ unsigned allowed);
extern int get_oid_mb(const char *str, struct object_id *oid);
extern int validate_headref(const char *ref);
diff --git a/refs.c b/refs.c
index 6d0961921..da62119c2 100644
--- a/refs.c
+++ b/refs.c
@@ -405,7 +405,7 @@ int refname_match(const char *abbrev_name, const char *full_name)
static char *substitute_branch_name(const char **string, int *len)
{
struct strbuf buf = STRBUF_INIT;
- int ret = interpret_branch_name(*string, *len, &buf);
+ int ret = interpret_branch_name(*string, *len, &buf, 0);
if (ret == *len) {
size_t size;
diff --git a/revision.c b/revision.c
index b37dbec37..771d079f6 100644
--- a/revision.c
+++ b/revision.c
@@ -147,7 +147,7 @@ static void add_pending_object_with_path(struct rev_info *revs,
revs->no_walk = 0;
if (revs->reflog_info && obj->type == OBJ_COMMIT) {
struct strbuf buf = STRBUF_INIT;
- int len = interpret_branch_name(name, 0, &buf);
+ int len = interpret_branch_name(name, 0, &buf, 0);
int st;
if (0 < len && name[len] && buf.len)
diff --git a/sha1_name.c b/sha1_name.c
index 4c1e91184..7f754b60c 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1176,7 +1176,8 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str
return 1;
}
-static int reinterpret(const char *name, int namelen, int len, struct strbuf *buf)
+static int reinterpret(const char *name, int namelen, int len,
+ struct strbuf *buf, unsigned allowed)
{
/* we have extra data, which might need further processing */
struct strbuf tmp = STRBUF_INIT;
@@ -1184,7 +1185,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu
int ret;
strbuf_add(buf, name + len, namelen - len);
- ret = interpret_branch_name(buf->buf, buf->len, &tmp);
+ ret = interpret_branch_name(buf->buf, buf->len, &tmp, allowed);
/* that data was not interpreted, remove our cruft */
if (ret < 0) {
strbuf_setlen(buf, used);
@@ -1205,11 +1206,27 @@ static void set_shortened_ref(struct strbuf *buf, const char *ref)
free(s);
}
+static int branch_interpret_allowed(const char *refname, unsigned allowed)
+{
+ if (!allowed)
+ return 1;
+
+ if ((allowed & INTERPRET_BRANCH_LOCAL) &&
+ starts_with(refname, "refs/heads/"))
+ return 1;
+ if ((allowed & INTERPRET_BRANCH_REMOTE) &&
+ starts_with(refname, "refs/remotes/"))
+ return 1;
+
+ return 0;
+}
+
static int interpret_branch_mark(const char *name, int namelen,
int at, struct strbuf *buf,
int (*get_mark)(const char *, int),
const char *(*get_data)(struct branch *,
- struct strbuf *))
+ struct strbuf *),
+ unsigned allowed)
{
int len;
struct branch *branch;
@@ -1234,11 +1251,15 @@ static int interpret_branch_mark(const char *name, int namelen,
if (!value)
die("%s", err.buf);
+ if (!branch_interpret_allowed(value, allowed))
+ return -1;
+
set_shortened_ref(buf, value);
return len + at;
}
-int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
+int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,
+ unsigned allowed)
{
char *at;
const char *start;
@@ -1247,31 +1268,38 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
if (!namelen)
namelen = strlen(name);
- len = interpret_nth_prior_checkout(name, namelen, buf);
- if (!len) {
- return len; /* syntax Ok, not enough switches */
- } else if (len > 0) {
- if (len == namelen)
- return len; /* consumed all */
- else
- return reinterpret(name, namelen, len, buf);
+ if (!allowed || (allowed & INTERPRET_BRANCH_LOCAL)) {
+ len = interpret_nth_prior_checkout(name, namelen, buf);
+ if (!len) {
+ return len; /* syntax Ok, not enough switches */
+ } else if (len > 0) {
+ if (len == namelen)
+ return len; /* consumed all */
+ else
+ return reinterpret(name, namelen, len, buf, allowed);
+ }
}
for (start = name;
(at = memchr(start, '@', namelen - (start - name)));
start = at + 1) {
- len = interpret_empty_at(name, namelen, at - name, buf);
- if (len > 0)
- return reinterpret(name, namelen, len, buf);
+ if (!allowed || (allowed & INTERPRET_BRANCH_HEAD)) {
+ len = interpret_empty_at(name, namelen, at - name, buf);
+ if (len > 0)
+ return reinterpret(name, namelen, len, buf,
+ allowed);
+ }
len = interpret_branch_mark(name, namelen, at - name, buf,
- upstream_mark, branch_get_upstream);
+ upstream_mark, branch_get_upstream,
+ allowed);
if (len > 0)
return len;
len = interpret_branch_mark(name, namelen, at - name, buf,
- push_mark, branch_get_push);
+ push_mark, branch_get_push,
+ allowed);
if (len > 0)
return len;
}
@@ -1279,10 +1307,10 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
return -1;
}
-void strbuf_branchname(struct strbuf *sb, const char *name)
+void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
{
int len = strlen(name);
- int used = interpret_branch_name(name, len, sb);
+ int used = interpret_branch_name(name, len, sb, allowed);
if (used < 0)
used = 0;
@@ -1291,7 +1319,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name)
int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
{
- strbuf_branchname(sb, name);
+ strbuf_branchname(sb, name, 0);
if (name[0] == '-')
return -1;
strbuf_splice(sb, 0, 0, "refs/heads/", 11);
diff --git a/strbuf.h b/strbuf.h
index 6b51b2604..17e5f29a5 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -567,8 +567,12 @@ static inline void strbuf_complete_line(struct strbuf *sb)
* "refs/remotes/origin/master").
*
* Note that the resulting name may not be a syntactically valid refname.
+ *
+ * If "allowed" is non-zero, restrict the set of allowed expansions. See
+ * interpret_branch_name() for details.
*/
-extern void strbuf_branchname(struct strbuf *sb, const char *name);
+extern void strbuf_branchname(struct strbuf *sb, const char *name,
+ unsigned allowed);
/*
* Like strbuf_branchname() above, but confirm that the result is
--
2.12.0.367.gb23790f66
^ permalink raw reply related
* [PATCH v2 2/8] strbuf_branchname: drop return value
From: Jeff King @ 2017-03-02 8:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20170302082100.edaretznmlralswa@sigill.intra.peff.net>
The return value from strbuf_branchname() is confusing and
useless: it's 0 if the whole name was consumed by an @-mark,
but otherwise is the length of the original name we fed.
No callers actually look at the return value, so let's just
get rid of it.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 5 +----
strbuf.h | 2 +-
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 28865b3a1..4c1e91184 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1279,17 +1279,14 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
return -1;
}
-int strbuf_branchname(struct strbuf *sb, const char *name)
+void strbuf_branchname(struct strbuf *sb, const char *name)
{
int len = strlen(name);
int used = interpret_branch_name(name, len, sb);
- if (used == len)
- return 0;
if (used < 0)
used = 0;
strbuf_add(sb, name + used, len - used);
- return len;
}
int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
diff --git a/strbuf.h b/strbuf.h
index cf1b5409e..47df0500d 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -560,7 +560,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
strbuf_complete(sb, '\n');
}
-extern int strbuf_branchname(struct strbuf *sb, const char *name);
+extern void strbuf_branchname(struct strbuf *sb, const char *name);
extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
--
2.12.0.367.gb23790f66
^ permalink raw reply related
* [PATCH v2 3/8] strbuf_branchname: add docstring
From: Jeff King @ 2017-03-02 8:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20170302082100.edaretznmlralswa@sigill.intra.peff.net>
This function and its companion, strbuf_check_branch_ref(),
did not have their purpose or semantics explained. Let's do
so.
Signed-off-by: Jeff King <peff@peff.net>
---
strbuf.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/strbuf.h b/strbuf.h
index 47df0500d..6b51b2604 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -560,7 +560,22 @@ static inline void strbuf_complete_line(struct strbuf *sb)
strbuf_complete(sb, '\n');
}
+/*
+ * Copy "name" to "sb", expanding any special @-marks as handled by
+ * interpret_branch_name(). The result is a non-qualified branch name
+ * (so "foo" or "origin/master" instead of "refs/heads/foo" or
+ * "refs/remotes/origin/master").
+ *
+ * Note that the resulting name may not be a syntactically valid refname.
+ */
extern void strbuf_branchname(struct strbuf *sb, const char *name);
+
+/*
+ * Like strbuf_branchname() above, but confirm that the result is
+ * syntactically valid to be used as a local branch name in refs/heads/.
+ *
+ * The return value is "0" if the result is valid, and "-1" otherwise.
+ */
extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
--
2.12.0.367.gb23790f66
^ permalink raw reply related
* [PATCH v2 1/8] interpret_branch_name: move docstring to header file
From: Jeff King @ 2017-03-02 8:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20170302082100.edaretznmlralswa@sigill.intra.peff.net>
We generally put docstrings with function declarations,
because it's the callers who need to know how the function
works. Let's do so for interpret_branch_name().
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 21 +++++++++++++++++++++
sha1_name.c | 21 ---------------------
2 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/cache.h b/cache.h
index 80b6372cf..c67995caa 100644
--- a/cache.h
+++ b/cache.h
@@ -1363,6 +1363,27 @@ extern char *oid_to_hex_r(char *out, const struct object_id *oid);
extern char *sha1_to_hex(const unsigned char *sha1); /* static buffer result! */
extern char *oid_to_hex(const struct object_id *oid); /* same static buffer as sha1_to_hex */
+/*
+ * This reads short-hand syntax that not only evaluates to a commit
+ * object name, but also can act as if the end user spelled the name
+ * of the branch from the command line.
+ *
+ * - "@{-N}" finds the name of the Nth previous branch we were on, and
+ * places the name of the branch in the given buf and returns the
+ * number of characters parsed if successful.
+ *
+ * - "<branch>@{upstream}" finds the name of the other ref that
+ * <branch> is configured to merge with (missing <branch> defaults
+ * to the current branch), and places the name of the branch in the
+ * given buf and returns the number of characters parsed if
+ * successful.
+ *
+ * If the input is not of the accepted format, it returns a negative
+ * number to signal an error.
+ *
+ * If the input was ok but there are not N branch switches in the
+ * reflog, it returns 0.
+ */
extern int interpret_branch_name(const char *str, int len, struct strbuf *);
extern int get_oid_mb(const char *str, struct object_id *oid);
diff --git a/sha1_name.c b/sha1_name.c
index 9b5d14b4b..28865b3a1 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1238,27 +1238,6 @@ static int interpret_branch_mark(const char *name, int namelen,
return len + at;
}
-/*
- * This reads short-hand syntax that not only evaluates to a commit
- * object name, but also can act as if the end user spelled the name
- * of the branch from the command line.
- *
- * - "@{-N}" finds the name of the Nth previous branch we were on, and
- * places the name of the branch in the given buf and returns the
- * number of characters parsed if successful.
- *
- * - "<branch>@{upstream}" finds the name of the other ref that
- * <branch> is configured to merge with (missing <branch> defaults
- * to the current branch), and places the name of the branch in the
- * given buf and returns the number of characters parsed if
- * successful.
- *
- * If the input is not of the accepted format, it returns a negative
- * number to signal an error.
- *
- * If the input was ok but there are not N branch switches in the
- * reflog, it returns 0.
- */
int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
{
char *at;
--
2.12.0.367.gb23790f66
^ permalink raw reply related
* [PATCH v2] fixing corner-cases with interpret_branch_name()
From: Jeff King @ 2017-03-02 8:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
This is a re-roll of the series from:
http://public-inbox.org/git/20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net/
Thanks Junio and Jake for reviewing the original. This is mostly the
same, but:
- it fixes the case where "branch -r @{-1}" mistakes a local branch
for a remote (and adds a test)
- as a result of the above fix, the series needs to be applied on top
of jk/auto-namelen-in-interpret-branch-name.
- I clarified the history in the commit message of patch 4
- the commit message for patch 4 now explicitly mentions which
callers can be left alone (so anybody blaming the history won't
think they were simply forgotten).
With the exception of patch 6 flipping the failure/success bit on the
new test, the rest of the patches should be identical.
[1/8]: interpret_branch_name: move docstring to header file
[2/8]: strbuf_branchname: drop return value
[3/8]: strbuf_branchname: add docstring
[4/8]: interpret_branch_name: allow callers to restrict expansions
[5/8]: t3204: test git-branch @-expansion corner cases
[6/8]: branch: restrict @-expansions when deleting
[7/8]: strbuf_check_ref_format(): expand only local branches
[8/8]: checkout: restrict @-expansions when finding branch
builtin/branch.c | 5 +-
builtin/checkout.c | 2 +-
builtin/merge.c | 2 +-
cache.h | 32 +++++++-
refs.c | 2 +-
revision.c | 2 +-
sha1_name.c | 92 ++++++++++++-----------
strbuf.h | 21 +++++-
t/t3204-branch-name-interpretation.sh | 133 ++++++++++++++++++++++++++++++++++
9 files changed, 240 insertions(+), 51 deletions(-)
create mode 100755 t/t3204-branch-name-interpretation.sh
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox