* Re: git push - 401 unauthorized
From: Jeff King @ 2017-03-03 8:07 UTC (permalink / raw)
To: Alessio Rocchi; +Cc: git
In-Reply-To: <017901d28183$6fb72c60$4f258520$@gmail.com>
On Tue, Feb 07, 2017 at 09:47:45PM +0100, Alessio Rocchi wrote:
> I try to push my commit on a private repository (which has been working
> since about five years).
It wasn't clear to me from your email, but did this work with a
previous version of Git and is now broken?
> me@superstar:/var/www/scorte$ git push --verbose
> Pushing to http://isisenscorte:mypassword@mymachine/scorte_git
> Getting pack list
> Fetching remote heads...
> refs/
> refs/tags/
> refs/heads/
> updating 'refs/heads/master'
> from d9fd2e49cb0c32a6d8fddcff2954f04b4104d176
> to 23d8edfb7fa70bce44c21a7f93064c08a7288e23
> sending 6 objects
> MOVE 33fcba80fdec82f43f995e5c693255da075358be failed, aborting (52/0)
> MOVE 60e1a097d50fe62319413ed20129580cf175d1ca failed, aborting (52/0)
> MOVE cfea41ef02f9aef5cecfbf7eac5a9e55975113f4 failed, aborting (52/0)
> MOVE 3d87ab6ff7652f2b30e48612b70c8335d4625699 failed, aborting (52/0)
> MOVE 4adb1b39e0446e0bfc3182258ff1cd7077871f7f failed, aborting (52/0)
> Updating remote server info
> fatal: git-http-push failed
OK, that looks like the old dumb-http protocol. And you said here:
> Permissions on the unauthorized object folders are 777 everywhere. My git
> version is 1.7.0.4 on both client and server. Do you have any clue of this
> strange behaviour?
...that you're using v1.7.0.4. There were tons of auth-related corner
cases that have been fixed since then. Can you try a more recent version
of Git on the client side (preferably v2.12.0)?
The version on the server shouldn't matter; the 401 is generated by
Apache, and as you are using the dumb-http protocol, git is not involved
on the server side of the request at all.
I'd also suggest that you move to the smart-http protocol on the server
if possible. It's much more efficient, and the dumb-http code paths are
not nearly as well tested, especially around things like authentication.
Running "git help http-backend" gives some sample Apache config.
-Peff
^ permalink raw reply
* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
From: Jeff King @ 2017-03-03 7:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Devin J. Pohly, Johannes Schindelin, Charles Bailey, git
In-Reply-To: <xmqqshmvbhe5.fsf@gitster.mtv.corp.google.com>
On Thu, Mar 02, 2017 at 11:36:18AM -0800, Junio C Hamano wrote:
> "Devin J. Pohly" <djpohly@gmail.com> writes:
>
> > I think your point is interesting too, though. If a commit is also
> > TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
> > reasonable that someone might want to leave it in the filtered branch as
> > an empty commit while pruning empt*ied* commits. I would imagine that
> > as another option (--prune-newly-empty?).
>
> I was hoping to hear from others who may care about filter-branch to
> comment on this topic to help me decide, but I haven't heard
> anything, so here is my tentative thinking.
>
> I am leaning to:
>
> * Take your series as-is, which would mean --prune-empty will
> change the behaviour to unconditionally lose the empty root.
>
> * Then, people who care deeply about it can add a new option that
> prunes commits that become empty while keeping the originally
> empty ones.
>
> Thoughts?
Sorry, this was on my to-review list but the sha1 stuff has been much
more exciting. :)
The behavior that Devin proposes is what I would have expected to
happen. Between "prune-existing-empty" and "prune-became-empty", I've
never had a use for the distinction. But as I think this is similar to
the way cherry-pick distinguished between "redundant" and "empty", I
guess some people have.
I agree that it should be a new, separate option, as it's orthogonal to
dealing with the root commit (i.e., somebody is equally likely to want
to preserve an already-empty commit from the middle of history).
The change to filter-branch itself looks obviously correct. The only
objectionable thing I noticed in the test additions is that the early
ones should be marked test_expect_failure until the fix from 3/4 flips
them to "success". Otherwise it breaks bisectability.
-Peff
^ permalink raw reply
* Re: [PATCH v2 4/9] Export the discover_git_directory() function
From: Jeff King @ 2017-03-03 4:45 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <0ca4cce042cc0b0f13f87363b70a3689057ae9b0.1488506615.git.johannes.schindelin@gmx.de>
On Fri, Mar 03, 2017 at 03:04:15AM +0100, Johannes Schindelin wrote:
> The function we introduced earlier needs to return both the path to the
> .git/ directory as well as the "cd-up" path to allow
> setup_git_directory() to retain its previous behavior as if it changed
> the current working directory on its quest for the .git/ directory.
>
> Let's rename it and export a function with an easier signature that
> *just* discovers the .git/ directory.
>
> We will use it in a subsequent patch to support early config reading
> better.
>
Seems like an obviously good next step.
> diff --git a/cache.h b/cache.h
> index 80b6372cf76..a104b76c02e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -518,6 +518,7 @@ extern void set_git_work_tree(const char *tree);
> #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
>
> extern void setup_work_tree(void);
> +extern const char *discover_git_directory(struct strbuf *gitdir);
Perhaps it's worth adding a short docstring describing the function. I
know that would make it unlike all of its neighbors, but it is not
immediately obvious to me what the return value is (or whether gitdir is
an input or output parameter).
> +const char *discover_git_directory(struct strbuf *gitdir)
> +{
> + struct strbuf dir = STRBUF_INIT;
> + int len;
Nit: please use size_t for storing strbuf lengths.
> + if (strbuf_getcwd(&dir))
> + return NULL;
> +
> + len = dir.len;
> + if (discover_git_directory_1(&dir, gitdir) < 0) {
> + strbuf_release(&dir);
> + return NULL;
> + }
> +
> + if (dir.len < len && !is_absolute_path(gitdir->buf)) {
> + strbuf_addch(&dir, '/');
> + strbuf_insert(gitdir, 0, dir.buf, dir.len);
> + }
> + strbuf_release(&dir);
I was confused by two things here.
One is that because I was wondering whether "gitdir" was supposed to be
passed empty or not, it wasn't clear that this insert is doing the right
thing. If there _is_ something in it, then discover_git_directory_1()
would just append to it, which makes sense. But then this insert blindly
sticks the absolute-path bit at the front of the string, leaving
whatever was originally there spliced into the middle of the path.
Doing:
size_t base = gitdir->len;
...
strbuf_insert(gitdir, base, dir.buf, dir.len);
would solve that. It's probably not that likely for somebody to do:
strbuf_addstr(&buf, "my git dir is ");
discover_git_directory(&buf);
but since it's not much effort, it might be worth making it work.
The second is that I don't quite understand why we only make the result
absolute when we walked upwards. In git.git, the result of the function
in various directories is:
- ".git", when we're at the root of the worktree
- "/home/peff/git/.git, when we're in "t/"
- "." when we're already in ".git"
- "/home/peff/git/.git/." when we're in ".git/objects"
I'm not sure if some of those cases are not behaving as intended, or
there's some reason for the differences that I don't quite understand.
-Peff
^ permalink raw reply
* [PATCH] t/perf: add fallback for pre-bin-wrappers versions of git
From: Jeff King @ 2017-03-03 7:36 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170303071403.2k3vbcdstllq2j3y@sigill.intra.peff.net>
On Fri, Mar 03, 2017 at 02:14:03AM -0500, Jeff King wrote:
> With this patch I was able to run p0001 against v1.7.0. I don't think we
> can go further back than that because the perf library depends on the
> presence of bin-wrappers. That's probably enough. Unlike the t/interop
> library I proposed recently it's not that interesting to go really far
> back in time (and I did hack around the bin-wrappers thing in t/interop;
> you really can test against v1.0.0 there).
This is easy to fix (see below). I doubt anybody cares, but it's
probably worth fixing just because the failure mode (quietly running
whatever git is in your PATH) is so confusing. It would also be an
improvement to just detect the situation and die(), but this is
literally not any more effort.
-- >8 --
Subject: [PATCH] t/perf: add fallback for pre-bin-wrappers versions of git
It's tempting to say:
./run v1.0.0 HEAD
to see how we've sped up Git over the years. Unfortunately,
this doesn't quite work because versions of Git prior to
v1.7.0 lack bin-wrappers, so our "run" script doesn't
correctly put them in the PATH.
Worse, it means we silently find whatever other "git" is in
the PATH, and produce test results that have no bearing on
what we asked for.
Let's fallback to the main git directory when bin-wrappers
isn't present. Many modern perf scripts won't run with such
an antique version of Git, of course, but at least those
failures are detected and reported (and you're free to write
a limited perf script that works across many versions).
Signed-off-by: Jeff King <peff@peff.net>
---
t/perf/run | 3 +++
1 file changed, 3 insertions(+)
diff --git a/t/perf/run b/t/perf/run
index e8adedadf..c788d713a 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -63,6 +63,9 @@ run_dirs_helper () {
unset GIT_TEST_INSTALLED
else
GIT_TEST_INSTALLED="$mydir/bin-wrappers"
+ # Older versions of git lacked bin-wrappers; fallback to the
+ # files in the root.
+ test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$mydir
export GIT_TEST_INSTALLED
fi
run_one_dir "$@"
--
2.12.0.385.gdf4947bc7
^ permalink raw reply related
* Re: [PATCH v2 5/9] Make read_early_config() reusable
From: Jeff King @ 2017-03-03 4:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <64135c6df520c1ca6a6e667855ac03c14d2dd153.1488506615.git.johannes.schindelin@gmx.de>
On Fri, Mar 03, 2017 at 03:04:20AM +0100, Johannes Schindelin wrote:
> The pager configuration needs to be read early, possibly before
> discovering any .git/ directory.
>
> Let's not hide this function in pager.c, but make it available to other
> callers.
> [...]
> + * Note that this is a really dirty hack that does the wrong thing in
> + * many cases. The crux of the problem is that we cannot run
Makes sense. I'll assume the words "dirty hack" disappear from this
now-public function as you fix it up in a future patch. :)
-Peff
^ permalink raw reply
* Re: [PATCH 4/4] p7000: add test for filter-branch with --prune-empty
From: Jeff King @ 2017-03-03 7:56 UTC (permalink / raw)
To: Devin J. Pohly; +Cc: git
In-Reply-To: <20170223082736.31283-4-djpohly@gmail.com>
On Thu, Feb 23, 2017 at 02:27:36AM -0600, Devin J. Pohly wrote:
> +test_perf 'noop prune-empty' '
> + git checkout --detach tip &&
> + git filter-branch -f --prune-empty base..HEAD
> +'
I don't mind adding this, but of curiosity, does it show anything
interesting?
-Peff
^ permalink raw reply
* [PATCH 2/2] attr.c: check if .gitattributes is a file before parsing it
From: Nguyễn Thái Ngọc Duy @ 2017-03-03 9:42 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170303094252.11706-1-pclouds@gmail.com>
Similar to the previous patch, this is about better error messages
when .gitattributes happens to be a directory.
FWIW .gitignore code is also checked. There open() is used instead and
open("dir") does not fail on Linux. But the next read should fail with
EISDIR, which is a pretty good clue already. No idea how open() on
Windows behaves.
---
attr.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 5493bff224..34b6a6b9a8 100644
--- a/attr.c
+++ b/attr.c
@@ -703,11 +703,17 @@ void git_attr_set_direction(enum git_attr_direction new_direction,
static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
{
- FILE *fp = fopen(path, "r");
+ FILE *fp;
struct attr_stack *res;
char buf[2048];
int lineno = 0;
+ if (is_not_file(path)) {
+ warning(_("'%s' is not a file"), path);
+ return NULL;
+ }
+
+ fp = fopen(path, "r");
if (!fp) {
if (errno != ENOENT && errno != ENOTDIR)
warn_on_inaccessible(path);
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH 1/2] config: check if config path is a file before parsing it
From: Nguyễn Thái Ngọc Duy @ 2017-03-03 9:42 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170303094252.11706-1-pclouds@gmail.com>
If a directory is given as a config file by accident, we keep open it
as a file. The behavior of fopen() in this case seems to be
undefined.
On Linux, we open a directory as a file ok, then get error (which we
consider eof) on the first read. So the config parser sees this "file"
as empty (i.e. valid config). All is well and we don't complain
anything (but we should).
The situation is slighly different on Windows. I think fopen() returns
NULL. And we get a very unhelpful message:
$ cat >abc <<EOF
[include]
path = /tmp/foo
EOF
$ mkdir /tmp/foo
$ git config --includes --file=abc core.bare
fatal: bad config line 3 in file abc
Opening a directory is wrong in the first place. Avoid it. If caught,
print something better. With this patch, we have
$ git config --includes --file=abc core.bare
error: '/tmp/foo' is not a file
fatal: bad config line 3 in file abc
It's not perfect (line should be 2 instead of 3). But it's definitely
improving.
The new test is only relevant on linux where we blindly open the
directory and consider it an empty file. On Windows, the test should
pass even without this patch.
---
abspath.c | 7 +++++++
cache.h | 1 +
config.c | 9 +++++++++
t/t1300-repo-config.sh | 5 +++++
4 files changed, 22 insertions(+)
diff --git a/abspath.c b/abspath.c
index 2f0c26e0e2..373cc3abb2 100644
--- a/abspath.c
+++ b/abspath.c
@@ -11,6 +11,13 @@ int is_directory(const char *path)
return (!stat(path, &st) && S_ISDIR(st.st_mode));
}
+int is_not_file(const char *path)
+{
+ struct stat st;
+
+ return !stat(path, &st) && !S_ISREG(st.st_mode);
+}
+
/* removes the last path component from 'path' except if 'path' is root */
static void strip_last_component(struct strbuf *path)
{
diff --git a/cache.h b/cache.h
index 80b6372cf7..bdd1402ab9 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,6 +1149,7 @@ static inline int is_absolute_path(const char *path)
return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
}
int is_directory(const char *);
+int is_not_file(const char *);
char *strbuf_realpath(struct strbuf *resolved, const char *path,
int die_on_error);
const char *real_path(const char *path);
diff --git a/config.c b/config.c
index c6b874a7bf..c21c0ce433 100644
--- a/config.c
+++ b/config.c
@@ -13,6 +13,7 @@
#include "hashmap.h"
#include "string-list.h"
#include "utf8.h"
+#include "dir.h"
struct config_source {
struct config_source *prev;
@@ -1212,6 +1213,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
int ret = -1;
FILE *f;
+ if (is_not_file(filename))
+ return error(_("'%s' is not a file"), filename);
+
f = fopen(filename, "r");
if (f) {
flockfile(f);
@@ -2451,6 +2455,11 @@ int git_config_rename_section_in_file(const char *config_filename,
goto out;
}
+ if (!S_ISREG(st.st_mode)) {
+ ret = error(_("'%s' is not a file"), config_filename);
+ goto out;
+ }
+
if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {
ret = error_errno("chmod on %s failed",
get_lock_file_path(lock));
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 052f120216..3683fbb84e 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1477,4 +1477,9 @@ test_expect_success !MINGW '--show-origin blob ref' '
test_cmp expect output
'
+test_expect_success 'a directory is given as a config file' '
+ mkdir config-dir &&
+ test_must_fail git config --file=config-dir --list
+'
+
test_done
--
2.11.0.157.gd943d85
^ permalink raw reply related
* Re: [PATCH v7 0/3] Conditional config include
From: Duy Nguyen @ 2017-03-03 9:52 UTC (permalink / raw)
To: Jeff King, Junio C Hamano
Cc: git, sschuberth, Matthieu Moy, Philip Oakley, Ramsay Jones
In-Reply-To: <20170303063329.ji6do6eqjbpuwmxz@sigill.intra.peff.net>
On Fri, Mar 03, 2017 at 01:33:29AM -0500, Jeff King wrote:
> For those following on the mailing list, there is some discussion at:
>
> https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33
>
> I think that is mostly focused around another failing in the
> error-handling of the config code, and that does not need to be
> addressed by this series (though of course I'd welcome any fixes).
>
> But there's a test failure that probably does need to be dealt with
> before this graduates to 'next'.
The patch to fix it is this
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index f0cd2056ba..e833939320 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -102,7 +102,7 @@ test_expect_success 'config modification does not affect includes' '
test_expect_success 'missing include files are ignored' '
cat >.gitconfig <<-\EOF &&
- [include]path = foo
+ [include]path = non-existent
[test]value = yes
EOF
echo yes >expect &&
Junio could you squash this in?
A lot more explanation is in another series [1] I just sent. Not sure
if we want to repeat the same in this commit's message.
And this series without that squash, when combined with [1], will fail
this test even on Linux. Good thing imo.
[1] http://public-inbox.org/git/20170303094252.11706-1-pclouds@gmail.com/
--
Duy
^ permalink raw reply related
* Re: [PATCH 1/2] config: check if config path is a file before parsing it
From: Duy Nguyen @ 2017-03-03 10:06 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin
In-Reply-To: <20170303095351.rgifjpfuvafx33jy@sigill.intra.peff.net>
On Fri, Mar 3, 2017 at 4:53 PM, Jeff King <peff@peff.net> wrote:
> I'm mildly negative on this approach for two reasons:
>
> 1. It requires doing an _extra_ check anywhere we want to care about
> this. So if we care about file/directory confusion, we're going to
> sprinkle these is_not_file() checks all over the code base.
>
> I think we're much better to just do the thing we want to do (like
> open the file), and deal with the error results. I'm on the fence
> on whether we want to care about the fopen behavior on Linux here
> (where reading a directory essentially behaves like an empty file,
> because the first read() gives an error and we don't distinguish
> between error and EOF).
I can't fix problems of my series on Windows because I don't use
Windows (because I will not be able to verify it). So I'm definitely
on the side that makes behavior consistent across platforms. Then I
can at least verify some (assuming that the consistent behavior is the
right one).
I didn't go with yours because I would have to handle two separate
code paths (fopen returns NULL and read returns EISDIR). But yeah it
should be that way even if it takes more time and effort. At least
we're now back on the mailing list and I didn't have to hurry to get
something out, to get off github.
> But if we do, I think we'd either want to:
>
> a. actually check ferror() after getting EOF and report the read
> error. That catches EISDIR, along with any other unexpected
> errors.
>
> b. use an fopen wrapper that checks fstat(fileno(fh)) after the
> open, and turns fopen(some_dir) into an error.
If you don't like extra check, I guess you're negative on b as well
since it is an extra check on Windows. That leaves us with option a.
> 2. It doesn't address the root problem for git_config_from_file(),
> which is that it is quiet when fopen fails, even if the reason is
> something interesting besides ENOENT. The caller can't check errno
> because it doesn't know if fopen() failed, or if the config
> callback returned an error.
>
> There's an attempt to protect the call to git_config_from_file() by
> checking access(), but that breaks down when access() and fopen()
> have two different results (which is exactly what happens on
> Windows in this case).
>
> -Peff
--
Duy
^ permalink raw reply
* Re: [PATCH 1/2] config: check if config path is a file before parsing it
From: Jeff King @ 2017-03-03 10:15 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin
In-Reply-To: <CACsJy8DU7-o06mfuw1L02CFFR2wmoNa0MQJ8KqsV79ULzjRaRQ@mail.gmail.com>
On Fri, Mar 03, 2017 at 05:06:57PM +0700, Duy Nguyen wrote:
> > But if we do, I think we'd either want to:
> >
> > a. actually check ferror() after getting EOF and report the read
> > error. That catches EISDIR, along with any other unexpected
> > errors.
> >
> > b. use an fopen wrapper that checks fstat(fileno(fh)) after the
> > open, and turns fopen(some_dir) into an error.
>
> If you don't like extra check, I guess you're negative on b as well
> since it is an extra check on Windows. That leaves us with option a.
I don't mind _doing_ the extra check that much. I don't think we fopen
so many files that an extra fstat on each would kill us. I mostly just
don't like having to sprinkle the explicit call to it everywhere. I'd be
OK with:
FILE *xfopen(const char *path, const char *mode)
{
FILE *ret = fopen(path, mode);
#ifdef FOPEN_OPENS_DIRECTORIES
if (ret) {
struct stat st;
if (!fstat(fileno(ret), &st) && S_ISDIR(st.st_mode)) {
fclose(ret);
ret = NULL;
}
}
#endif
return ret;
}
But I do think option (a) is cleaner. The only trick is that for errno
to be valid, we need to make sure we check ferror() soon after seeing
the EOF return value. I suspect it would work OK in practice for the
git_config_from_file() case.
-Peff
^ permalink raw reply
* [PATCH 0/2] Improve error messages when opening a directory as file
From: Nguyễn Thái Ngọc Duy @ 2017-03-03 9:42 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy
The topic nd/conditional-config-include hit a problem on Windows [1].
The test basically does this (much simplified)
echo '[include]path=foo' >~/.gitconfig
cd ~ && git init foo
At some point in 'git init' after 'foo' directory has been created, we
request to include ~/foo as an extra config file. But it's a directory
and we get some error like this
fatal: bad config line 2 in file ~/.gitconfig
The message gives no clue that 'foo' is a directory (and probably
wasted a good chunk of time of Johannes). This series tells the user
about that.
The other half of the problem is, the same test runs without error on
Linux because it looks like fopen(dir) returns NULL on Windows, but
non-NULL on Linux and only subsequent read() returns EISDIR.
Unfortunately the config parser conflates errors with eof, I think.
And it simply sees <dir> as an empty config file, ie. a valid config
file. So no "bad config line..."
I'm making sure even Linux now reports loud and clear that config
files should be _files_. The same treatment is done for .gitattributes.
I'm not so sure about .gitignore because it uses open(), not fopen()
and I don't know if open() behaves differently on Windows.
I briefly considered fopen() and open() wrappers that always rejects
directories (if you need to open a directory, do it without wrappers),
but discarded it because it adds an extra stat() call everywhere.
[1] https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33#commitcomment-21121210
Nguyễn Thái Ngọc Duy (2):
config: check if config path is a file before parsing it
attr.c: check if .gitattributes is a file before parsing it
abspath.c | 7 +++++++
attr.c | 8 +++++++-
cache.h | 1 +
config.c | 9 +++++++++
t/t1300-repo-config.sh | 5 +++++
5 files changed, 29 insertions(+), 1 deletion(-)
--
2.11.0.157.gd943d85
^ permalink raw reply
* Re: [PATCH 1/2] config: check if config path is a file before parsing it
From: Jeff King @ 2017-03-03 10:31 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin
In-Reply-To: <20170303101503.lf2ub2c7i6w7kg3t@sigill.intra.peff.net>
On Fri, Mar 03, 2017 at 05:15:03AM -0500, Jeff King wrote:
> But I do think option (a) is cleaner. The only trick is that for errno
> to be valid, we need to make sure we check ferror() soon after seeing
> the EOF return value. I suspect it would work OK in practice for the
> git_config_from_file() case.
Something like this is a big improvement, I think:
diff --git a/config.c b/config.c
index c6b874a7b..27b410dfe 100644
--- a/config.c
+++ b/config.c
@@ -156,15 +156,14 @@ static int handle_path_include(const char *path, struct config_include_data *inc
path = buf.buf;
}
- if (!access_or_die(path, R_OK, 0)) {
- if (++inc->depth > MAX_INCLUDE_DEPTH)
- die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
- !cf ? "<unknown>" :
- cf->name ? cf->name :
- "the command line");
- ret = git_config_from_file(git_config_include, path, inc);
- inc->depth--;
- }
+ if (++inc->depth > MAX_INCLUDE_DEPTH)
+ die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
+ !cf ? "<unknown>" :
+ cf->name ? cf->name :
+ "the command line");
+ ret = git_config_from_file(git_config_include, path, inc);
+ inc->depth--;
+
strbuf_release(&buf);
free(expanded);
return ret;
@@ -1213,10 +1212,18 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
FILE *f;
f = fopen(filename, "r");
- if (f) {
+ if (!f) {
+ /* a missing file is silently treated as an empty one */
+ if (errno == ENOENT || errno == EISDIR)
+ ret = 0;
+ else
+ ret = error_errno("unable to open %s", filename);
+ } else {
flockfile(f);
ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);
funlockfile(f);
+ if (!ret && ferror(f))
+ ret = error_errno("unable to read from %s", filename);
fclose(f);
}
return ret;
Then if you do:
cd repo.git
git config include.path this-is-broken
you get useful errors for a variety of situations:
$ mkdir this-is-broken
$ git rev-parse
error: unable to read from this-is-broken: Is a directory
fatal: bad config line 7 in file config
$ rmdir this-is-broken
$ ln -s this-is-broken this-is-broken
$ git rev-parse
error: unable to open this-is-broken: Too many levels of symbolic links
fatal: bad config line 7 in file config
and so on. The two caveats are:
1. A few of the callers treat EACCES specially, so we'd potentially
want a flag for that (or alternatively, everybody should just fopen
the file themselves and pass in the handle).
2. The call in read_repository_format() does not check the return
value at all. It measures errors only as "did the parser find a
core.repositoryformatversion field I can look at", though arguably
it should check for other errors, too (if we read "version=2", but
then got a read error before we were able to read the extensions,
that would be wrong and bad).
But either way I suspect it probably prefers the current "quiet"
behavior, since it is used to speculatively look for repositories.
So probably git_config_from_file() needs a flags parameter, and both
"quiet" and EACCES handling can go in there.
-Peff
^ permalink raw reply related
* Re: [PATCH 1/2] config: check if config path is a file before parsing it
From: Jeff King @ 2017-03-03 10:33 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin
In-Reply-To: <CACsJy8CDOBUe7S3sYz=_tAJy2ajM5h2v+ZypD6Cr1uJo7XAnGQ@mail.gmail.com>
On Fri, Mar 03, 2017 at 05:29:47PM +0700, Duy Nguyen wrote:
> On Fri, Mar 3, 2017 at 5:15 PM, Jeff King <peff@peff.net> wrote:
> > But I do think option (a) is cleaner. The only trick is that for errno
> > to be valid, we need to make sure we check ferror() soon after seeing
> > the EOF return value. I suspect it would work OK in practice for the
> > git_config_from_file() case.
>
> stdio error handling is a pain. Maybe we're better of with open() and
> mmap() (or even read_in_full)? I/O error handling would be at the
> beginning, not buried deep in the parser. Hmm.. since we already have
> "fgetc' version for config blobs, this could kill some code...
Yeah, I don't mind a read_in_full() version. Config isn't _supposed_ to
be big (and if it is you're in trouble anyway, because I'm pretty sure
we still parse it several times per command invocation).
I don't think that removes the issues I've mentioned with
git_config_from_file() being too quiet. But it solves the ferror()
question (though I think we pretty much return immediately from the
parser on EOF, so it's _probably_ OK to use it like in the diff I just
sent).
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] config: check if config path is a file before parsing it
From: Duy Nguyen @ 2017-03-03 10:29 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin
In-Reply-To: <20170303101503.lf2ub2c7i6w7kg3t@sigill.intra.peff.net>
On Fri, Mar 3, 2017 at 5:15 PM, Jeff King <peff@peff.net> wrote:
> But I do think option (a) is cleaner. The only trick is that for errno
> to be valid, we need to make sure we check ferror() soon after seeing
> the EOF return value. I suspect it would work OK in practice for the
> git_config_from_file() case.
stdio error handling is a pain. Maybe we're better of with open() and
mmap() (or even read_in_full)? I/O error handling would be at the
beginning, not buried deep in the parser. Hmm.. since we already have
"fgetc' version for config blobs, this could kill some code...
--
Duy
^ permalink raw reply
* [PATCH] Do not require Python for the git-remote-{bzr,hg} placeholder scripts
From: Sebastian Schuberth @ 2017-03-03 10:57 UTC (permalink / raw)
To: git
It does not make sense for these placeholder scripts to depend on Python
just because the real scripts do. At the example of Git for Windows, we
would not even be able to see those warnings as it does not ship with
Python. So just use plain shell scripts instead.
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
contrib/remote-helpers/git-remote-bzr | 16 +++++++---------
contrib/remote-helpers/git-remote-hg | 16 +++++++---------
2 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index 712a137..ccc4aea 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -1,13 +1,11 @@
-#!/usr/bin/env python
+#!/bin/sh
-import sys
-
-sys.stderr.write('WARNING: git-remote-bzr is now maintained independently.\n')
-sys.stderr.write('WARNING: For more information visit https://github.com/felipec/git-remote-bzr\n')
-
-sys.stderr.write('''WARNING:
+cat <<'EOT'
+WARNING: git-remote-bzr is now maintained independently.
+WARNING: For more information visit https://github.com/felipec/git-remote-bzr
+WARNING:
WARNING: You can pick a directory on your $PATH and download it, e.g.:
-WARNING: $ wget -O $HOME/bin/git-remote-bzr \\
+WARNING: $ wget -O $HOME/bin/git-remote-bzr \
WARNING: https://raw.github.com/felipec/git-remote-bzr/master/git-remote-bzr
WARNING: $ chmod +x $HOME/bin/git-remote-bzr
-''')
+EOT
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 4255ad6..dfda44f 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -1,13 +1,11 @@
-#!/usr/bin/env python
+#!/bin/sh
-import sys
-
-sys.stderr.write('WARNING: git-remote-hg is now maintained independently.\n')
-sys.stderr.write('WARNING: For more information visit https://github.com/felipec/git-remote-hg\n')
-
-sys.stderr.write('''WARNING:
+cat <<'EOT'
+WARNING: git-remote-hg is now maintained independently.
+WARNING: For more information visit https://github.com/felipec/git-remote-hg
+WARNING:
WARNING: You can pick a directory on your $PATH and download it, e.g.:
-WARNING: $ wget -O $HOME/bin/git-remote-hg \\
+WARNING: $ wget -O $HOME/bin/git-remote-hg \
WARNING: https://raw.github.com/felipec/git-remote-hg/master/git-remote-hg
WARNING: $ chmod +x $HOME/bin/git-remote-hg
-''')
+EOT
--
https://github.com/git/git/pull/333
^ permalink raw reply related
* [PATCH v2] Travis: also test on 32-bit Linux
From: Lars Schneider @ 2017-03-03 11:01 UTC (permalink / raw)
To: git
Cc: Johannes.Schindelin, gitster, ramsay, christian.couder,
Johannes Schindelin
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>
---
Hi,
changes based on reviews since v2:
- removed "set -e"
- pass docker image name to run-linux32-build script
- use Dscho's docker run formatting
other changes:
- We run "make" with "-j2" and "make test" without parallelization because
prove already parallelizes the tests (see GIT_PROVE_OPTS in .travis.yml).
- If the tests fail then I make all output files readable to everyone.
This is necessary because the files are created with a root account
inside the docker container and I want to allow the "after_failure" step
outside the container to read the files without root permissions.
The job passes on the current master:
https://travis-ci.org/larsxschneider/git/jobs/207168867 (JS)
https://api.travis-ci.org/jobs/207168867/log.txt?deansi=true (non-JS)
The job fails on v2.9.1:
https://travis-ci.org/larsxschneider/git/jobs/207306002 (JS)
https://api.travis-ci.org/jobs/207306002/log.txt?deansi=true (non-JS)
Cheers,
Lars
Notes:
Base Ref: master
Web-Diff: https://github.com/larsxschneider/git/commit/c5d84e8785
Checkout: git fetch https://github.com/larsxschneider/git travisci/linux32-v2 && git checkout c5d84e8785
Interdiff (v1..v2):
diff --git a/.travis.yml b/.travis.yml
index c8c789c437..fd60fd8328 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -47,7 +47,7 @@ matrix:
before_install:
- docker pull daald/ubuntu32:xenial
before_script:
- script: ci/run-linux32-build.sh
+ script: ci/run-linux32-build.sh daald/ubuntu32:xenial
- env: Documentation
os: linux
compiler: clang
diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
index b892fbdc9e..13c184d41c 100755
--- a/ci/run-linux32-build.sh
+++ b/ci/run-linux32-build.sh
@@ -2,20 +2,30 @@
#
# Build and test Git in a docker container running a 32-bit Ubuntu Linux
#
+# Usage:
+# run-linux32-build.sh [container-image]
+#
-set -e
-
-APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
-"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
+CONTAINER="${1:-daald/ubuntu32:xenial}"
-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"
+sudo docker run --interactive --volume "${PWD}:/usr/src/git" "$CONTAINER" \
+ /bin/bash -c 'linux32 --32bit i386 sh -c "
+ : update packages &&
+ apt update >/dev/null &&
+ apt install -y build-essential libcurl4-openssl-dev libssl-dev \
+ libexpat-dev gettext python >/dev/null &&
-TEST_GIT_CMD="linux32 --32bit i386 sh -c "\
-"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
+ : build and test &&
+ cd /usr/src/git &&
+ export DEFAULT_TEST_TARGET='$DEFAULT_TEST_TARGET' &&
+ export GIT_PROVE_OPTS=\"'"$GIT_PROVE_OPTS"'\" &&
+ export GIT_TEST_OPTS=\"'"$GIT_TEST_OPTS"'\" &&
+ export GIT_TEST_CLONE_2GB='$GIT_TEST_CLONE_2GB' &&
+ make --jobs=2 &&
+ make --quiet test || (
-sudo docker run \
- --interactive --volume "${PWD}:/usr/src/git" \
- daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
+ : make test-results readable to non-root user on TravisCI &&
+ test '$TRAVIS' &&
+ find t/test-results/ -type f -exec chmod o+r {} \; &&
+ false )
+"'
\0
.travis.yml | 9 +++++++++
ci/run-linux32-build.sh | 31 +++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+)
create mode 100755 ci/run-linux32-build.sh
diff --git a/.travis.yml b/.travis.yml
index 9c63c8c3f6..fd60fd8328 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 daald/ubuntu32:xenial
- 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..13c184d41c
--- /dev/null
+++ b/ci/run-linux32-build.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+#
+# Build and test Git in a docker container running a 32-bit Ubuntu Linux
+#
+# Usage:
+# run-linux32-build.sh [container-image]
+#
+
+CONTAINER="${1:-daald/ubuntu32:xenial}"
+
+sudo docker run --interactive --volume "${PWD}:/usr/src/git" "$CONTAINER" \
+ /bin/bash -c 'linux32 --32bit i386 sh -c "
+ : update packages &&
+ apt update >/dev/null &&
+ apt install -y build-essential libcurl4-openssl-dev libssl-dev \
+ libexpat-dev gettext python >/dev/null &&
+
+ : build and test &&
+ cd /usr/src/git &&
+ export DEFAULT_TEST_TARGET='$DEFAULT_TEST_TARGET' &&
+ export GIT_PROVE_OPTS=\"'"$GIT_PROVE_OPTS"'\" &&
+ export GIT_TEST_OPTS=\"'"$GIT_TEST_OPTS"'\" &&
+ export GIT_TEST_CLONE_2GB='$GIT_TEST_CLONE_2GB' &&
+ make --jobs=2 &&
+ make --quiet test || (
+
+ : make test-results readable to non-root user on TravisCI &&
+ test '$TRAVIS' &&
+ find t/test-results/ -type f -exec chmod o+r {} \; &&
+ false )
+"'
base-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858
--
2.11.1
^ permalink raw reply related
* Re: [PATCH v2 2/9] setup_git_directory(): use is_dir_sep() helper
From: Johannes Schindelin @ 2017-03-03 11:16 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <20170303033714.5h7qrq57hdqwoi3y@sigill.intra.peff.net>
Hi Peff,
On Thu, 2 Mar 2017, Jeff King wrote:
> On Fri, Mar 03, 2017 at 03:04:07AM +0100, Johannes Schindelin wrote:
>
> > It is okay in practice to test for forward slashes in the output of
> > getcwd(), because we go out of our way to convert backslashes to
> > forward slashes in getcwd()'s output on Windows.
> >
> > Still, the correct way to test for a dir separator is by using the
> > helper function we introduced for that very purpose. It also serves as
> > a good documentation what the code tries to do (not "how").
>
> Makes sense, but...
>
> > @@ -910,7 +910,8 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
> > return setup_bare_git_dir(&cwd, offset, nongit_ok);
> >
> > offset_parent = offset;
> > - while (--offset_parent > ceil_offset && cwd.buf[offset_parent] != '/');
> > + while (--offset_parent > ceil_offset &&
> > + !is_dir_sep(dir->buf[offset_parent]));
>
> What is "dir"? I'm guessing this patch got reordered and it should stay
> as cwd.buf?
Oh drats. Usually I do a final `git rebase -x "make test" upstream/master`
run before submitting, but I was really, really tired by the end of that
stretch.
Thanks for being thorough (and I fixed it, of course),
Dscho
^ permalink raw reply
* Re: [PATCH 1/2] config: check if config path is a file before parsing it
From: Jeff King @ 2017-03-03 9:53 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Johannes Schindelin
In-Reply-To: <20170303094252.11706-2-pclouds@gmail.com>
On Fri, Mar 03, 2017 at 04:42:51PM +0700, Nguyễn Thái Ngọc Duy wrote:
> If a directory is given as a config file by accident, we keep open it
> as a file. The behavior of fopen() in this case seems to be
> undefined.
>
> On Linux, we open a directory as a file ok, then get error (which we
> consider eof) on the first read. So the config parser sees this "file"
> as empty (i.e. valid config). All is well and we don't complain
> anything (but we should).
>
> The situation is slighly different on Windows. I think fopen() returns
> NULL. And we get a very unhelpful message:
>
> $ cat >abc <<EOF
> [include]
> path = /tmp/foo
> EOF
> $ mkdir /tmp/foo
> $ git config --includes --file=abc core.bare
> fatal: bad config line 3 in file abc
>
> Opening a directory is wrong in the first place. Avoid it. If caught,
> print something better. With this patch, we have
>
> $ git config --includes --file=abc core.bare
> error: '/tmp/foo' is not a file
> fatal: bad config line 3 in file abc
>
> It's not perfect (line should be 2 instead of 3). But it's definitely
> improving.
>
> The new test is only relevant on linux where we blindly open the
> directory and consider it an empty file. On Windows, the test should
> pass even without this patch.
I'm mildly negative on this approach for two reasons:
1. It requires doing an _extra_ check anywhere we want to care about
this. So if we care about file/directory confusion, we're going to
sprinkle these is_not_file() checks all over the code base.
I think we're much better to just do the thing we want to do (like
open the file), and deal with the error results. I'm on the fence
on whether we want to care about the fopen behavior on Linux here
(where reading a directory essentially behaves like an empty file,
because the first read() gives an error and we don't distinguish
between error and EOF). But if we do, I think we'd either want to:
a. actually check ferror() after getting EOF and report the read
error. That catches EISDIR, along with any other unexpected
errors.
b. use an fopen wrapper that checks fstat(fileno(fh)) after the
open, and turns fopen(some_dir) into an error.
2. It doesn't address the root problem for git_config_from_file(),
which is that it is quiet when fopen fails, even if the reason is
something interesting besides ENOENT. The caller can't check errno
because it doesn't know if fopen() failed, or if the config
callback returned an error.
There's an attempt to protect the call to git_config_from_file() by
checking access(), but that breaks down when access() and fopen()
have two different results (which is exactly what happens on
Windows in this case).
-Peff
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-03-03 11:13 UTC (permalink / raw)
To: Brandon Williams
Cc: Jason Cooper, Junio C Hamano, Linus Torvalds, Ian Jackson,
Joey Hess, Git Mailing List
In-Reply-To: <20170302214610.GA86054@google.com>
On Thu, Mar 02, 2017 at 01:46:10PM -0800, Brandon Williams wrote:
> There were a few of us discussing this sort of approach internally. We
> also figured that, given some performance hit, you could maintain your
> repo in sha256 and do some translation to sha1 if you need to push or
> fetch to a server which has the the repo in a sha1 format. This way you
> can convert your repo independently of the rest of the world.
Yeah, you definitely _can_ convert between the two. It's just expensive
to do so on the fly. We'd potentially want to be able to during the
transition period just to help people get all the work converted over.
But I had assumed that the conversion would be a mix of:
1. Unpublished work (or work which is otherwise OK to be rewritten)
could be converted to the new hash.
2. Old history could be grafted with a parent pointer that mentions
the tip of the old history by its new hash, but the pointed-to
parent contains sha1s.
> As for storing the translation table, you should really only need to
> maintain the table until old clients are phased out and all of the repos
> of a project have experienced flag day and have been converted to
> sha256.
I think you've read more into my "conversion" than I intended. The old
history won't get rewritten. It will just be grafted onto the bottom of
the commit history you've got, and the new trees will all be written
with the new hash.
So you still have those old objects hanging around that refer to things
by their sha1 (not to mention bug trackers, commit messages, etc, which
all use commit ids). And you want to be able to quickly resolve those
references.
What _does_ get rewritten is what's in your ref files, your pack .idx,
etc. Those are all sha256 (or whatever), and work as sha1's do now.
Looking up a sha1 reference from an old object just goes through the
extra level of indirection.
-Peff
^ permalink raw reply
* [PATCH 1/2] t6042: Add failing test for rename/rename/delete/delete.
From: Nicolas Cavallari @ 2017-03-03 11:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
Each side of a rename 2to1 could also be deleted by the other side.
The code does not expect this. While it luckily works for files,
it fails for symlinks as follow:
CONFLICT (rename/rename): Rename a->c in HEAD. Rename b->c in C^0
Renaming a to c~HEAD and b to c~C^0 instead
error: cannot read object 0000000000000000000000000000000000000000 'c~C^0'
Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
---
t/t6042-merge-rename-corner-cases.sh | 38 ++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
index 411550d2b..ea4e14cbd 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -575,4 +575,42 @@ test_expect_success 'rename/rename/add-dest merge still knows about conflicting
test ! -f c
'
+# Testcase setup for rename/rename/delete/delete (2in1, deleted):
+# Commit A: new symlinks: a->alice b->bob
+# Commit B: rename a->c, delete b
+# Commit C: rename b->c, delete a
+#
+# We should not fail completely.
+
+test_expect_success 'setup rename/rename/delete/delete conflict' '
+ git rm -rf . &&
+ git clean -fdqx &&
+ rm -rf .git &&
+ git init &&
+
+ ln -s alice a &&
+ ln -s bob b &&
+ git add a b &&
+ git commit -m A &&
+ git tag A &&
+
+ git checkout -b B A &&
+ git mv a c &&
+ git rm b &&
+ git commit -m B &&
+
+ git checkout -b C A &&
+ git mv b c &&
+ git rm a &&
+ git commit -m C
+'
+
+test_expect_failure 'rename/rename/delete/delete leaves at least one file' '
+ git checkout B^0 &&
+ test_must_fail git merge -s recursive C^0 &&
+
+ test -L "c~HEAD" &&
+ test -L "c~C^0"
+'
+
test_done
--
2.11.0
^ permalink raw reply related
* [PATCH/RFC 2/2] merge-recursive: Handle rename/rename/delete/delete conflicts.
From: Nicolas Cavallari @ 2017-03-03 11:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20170303113317.18575-1-nicolas.cavallari@green-communications.fr>
On a rename 2to1 conflict, where both a and b are renamed to c,
the other branch may have deleted a or b. This currently luckily
works for files but fails for symlinks.
Fix conflict_rename_rename_2to1 to not merge_file_special_markers()
with diff_filespecs where mode == 0. The alternative would be to make
merge_file_1() handle it.
Mark the corresponding testcase in t6042 as fixed.
Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
---
merge-recursive.c | 28 ++++++++++++++++++++++------
t/t6042-merge-rename-corner-cases.sh | 2 +-
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index b7ff1ada3..0e5a3e3ed 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1305,13 +1305,29 @@ static int conflict_rename_rename_2to1(struct merge_options *o,
remove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path));
remove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path));
- if (merge_file_special_markers(o, a, c1, &ci->ren1_other,
- o->branch1, c1->path,
- o->branch2, ci->ren1_other.path, &mfi_c1) ||
- merge_file_special_markers(o, b, &ci->ren2_other, c2,
- o->branch1, ci->ren2_other.path,
- o->branch2, c2->path, &mfi_c2))
+ // The other branch may have deleted a, as indicated by mode == 0.
+ if (ci->ren1_other.mode == 0) {
+ mfi_c1.clean = 0;
+ mfi_c1.merge = 0;
+ mfi_c1.mode = c1->mode;
+ oidcpy(&mfi_c1.oid, &c1->oid);
+ } else if (merge_file_special_markers(o, a, c1, &ci->ren1_other,
+ o->branch1, c1->path,
+ o->branch2, ci->ren1_other.path,
+ &mfi_c1)) {
return -1;
+ }
+
+ if (ci->ren2_other.mode == 0) {
+ mfi_c2.clean = 0;
+ mfi_c2.merge = 0;
+ mfi_c2.mode = c2->mode;
+ oidcpy(&mfi_c2.oid, &c2->oid);
+ } else if (merge_file_special_markers(o, b, &ci->ren2_other, c2,
+ o->branch1, ci->ren2_other.path,
+ o->branch2, c2->path, &mfi_c2)) {
+ return -1;
+ }
if (o->call_depth) {
/*
diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
index ea4e14cbd..34b16d5d7 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -605,7 +605,7 @@ test_expect_success 'setup rename/rename/delete/delete conflict' '
git commit -m C
'
-test_expect_failure 'rename/rename/delete/delete leaves at least one file' '
+test_expect_success 'rename/rename/delete/delete leaves at least one file' '
git checkout B^0 &&
test_must_fail git merge -s recursive C^0 &&
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v2 2/9] setup_git_directory(): use is_dir_sep() helper
From: Jeff King @ 2017-03-03 11:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Michael Haggerty, git, Junio C Hamano, Duy Nguyen
In-Reply-To: <alpine.DEB.2.20.1703031212280.3767@virtualbox>
On Fri, Mar 03, 2017 at 12:16:31PM +0100, Johannes Schindelin wrote:
> > What is "dir"? I'm guessing this patch got reordered and it should stay
> > as cwd.buf?
>
> Oh drats. Usually I do a final `git rebase -x "make test" upstream/master`
> run before submitting, but I was really, really tired by the end of that
> stretch.
I usually do the same, and have done the "too tired" thing, too, only to
have it bite me. That's why I so readily recognized the problem. :)
I've recently switched to using Michael's "git test" program[1], which
caches the test results for each tree in a git-note. That makes the
final "rebase -x" a lot less painful if you've left the early commits
alone.
The python dependency might be a blocker for you, but I suspect the
caching parts would be easy to hack together with shell.
-Peff
[1] https://github.com/mhagger/git-test
^ permalink raw reply
* [PATCH 1/3] Add --gui option to mergetool
From: Denton Liu @ 2017-03-03 11:57 UTC (permalink / raw)
To: git; +Cc: davvid, Johannes.Schindelin
This fixes the discrepancy between difftool and mergetool where the
former has the --gui flag and the latter does not by adding the
functionality to mergetool.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-mergetool.txt | 8 +++++++-
contrib/completion/git-completion.bash | 3 ++-
git-mergetool.sh | 5 ++++-
t/t7610-mergetool.sh | 28 +++++++++++++++++++++++++++-
4 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 3622d6648..2ab56efcf 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -8,7 +8,7 @@ git-mergetool - Run merge conflict resolution tools to resolve merge conflicts
SYNOPSIS
--------
[verse]
-'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [<file>...]
+'git mergetool' [--tool=<tool>] [-g|--gui] [-y | --[no-]prompt] [<file>...]
DESCRIPTION
-----------
@@ -64,6 +64,12 @@ variable `mergetool.<tool>.trustExitCode` can be set to `true`.
Otherwise, 'git mergetool' will prompt the user to indicate the
success of the resolution after the custom tool has exited.
+-g::
+--gui::
+ When 'git-mergetool' is invoked with the `-g` or `--gui` option
+ the default diff tool will be read from the configured
+ `merge.guitool` variable instead of `merge.tool`.
+
--tool-help::
Print a list of merge tools that may be used with `--tool`.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 41ee52991..d5f3b9aeb 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1584,7 +1584,7 @@ _git_mergetool ()
return
;;
--*)
- __gitcomp "--tool= --prompt --no-prompt"
+ __gitcomp "--tool= --prompt --no-prompt --gui"
return
;;
esac
@@ -2308,6 +2308,7 @@ _git_config ()
merge.renormalize
merge.stat
merge.tool
+ merge.guitool
merge.verbosity
mergetool.
mergetool.keepBackup
diff --git a/git-mergetool.sh b/git-mergetool.sh
index c062e3de3..f3fce528b 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,7 @@
# at the discretion of Junio C Hamano.
#
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O<orderfile>] [file to merge] ...'
+USAGE='[--tool=tool] [-g|--gui] [--tool-help] [-y|--no-prompt|--prompt] [-O<orderfile>] [file to merge] ...'
SUBDIRECTORY_OK=Yes
NONGIT_OK=Yes
OPTIONS_SPEC=
@@ -414,6 +414,9 @@ main () {
shift ;;
esac
;;
+ -g|--gui)
+ merge_tool=$(git config merge.guitool)
+ ;;
-y|--no-prompt)
prompt=false
;;
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 381b7df45..5683907ab 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -123,7 +123,9 @@ test_expect_success 'setup' '
git config mergetool.mytool.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
git config mergetool.mytool.trustExitCode true &&
git config mergetool.mybase.cmd "cat \"\$BASE\" >\"\$MERGED\"" &&
- git config mergetool.mybase.trustExitCode true
+ git config mergetool.mybase.trustExitCode true &&
+ git config mergetool.badtool.cmd false &&
+ git config mergetool.badtool.trustExitCode true
'
test_expect_success 'custom mergetool' '
@@ -145,6 +147,30 @@ test_expect_success 'custom mergetool' '
git commit -m "branch1 resolved with mergetool"
'
+test_expect_success 'gui mergetool' '
+ test_when_finished "git reset --hard" &&
+ test_when_finished "git config merge.tool mytool" &&
+ test_when_finished "git config --unset merge.guitool" &&
+ git config merge.tool badtool &&
+ git config merge.guitool mytool &&
+ git checkout -b test$test_count branch1 &&
+ git submodule update -N &&
+ test_must_fail git merge master >/dev/null 2>&1 &&
+ ( yes "" | git mergetool -g both >/dev/null 2>&1 ) &&
+ ( yes "" | git mergetool -g file1 file1 ) &&
+ ( yes "" | git mergetool --gui file2 "spaced name" >/dev/null 2>&1 ) &&
+ ( yes "" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool -g file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool --gui file12 >/dev/null 2>&1 ) &&
+ ( yes "l" | git mergetool --gui submod >/dev/null 2>&1 ) &&
+ cat file1 &&
+ test "$(cat file1)" = "master updated" &&
+ test "$(cat file2)" = "master new" &&
+ test "$(cat subdir/file3)" = "master new sub" &&
+ test "$(cat submod/bar)" = "branch1 submodule" &&
+ git commit -m "branch1 resolved with gui mergetool"
+'
+
test_expect_success 'mergetool crlf' '
test_when_finished "git reset --hard" &&
# This test_config line must go after the above reset line so that
--
2.12.0.1.g5415fdfc5.dirty
^ permalink raw reply related
* [PATCH 3/3] Remove outdated info in difftool manpage
From: Denton Liu @ 2017-03-03 11:57 UTC (permalink / raw)
To: git; +Cc: davvid, Johannes.Schindelin
When difftool was rewritten in C, it removed the capability to read
fallback configs from mergetool. This changes the documentation to
reflect this.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-difftool.txt | 3 ---
1 file changed, 3 deletions(-)
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 96c26e6aa..a00cb033e 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -105,9 +105,6 @@ See linkgit:git-diff[1] for the full list of supported options.
CONFIG VARIABLES
----------------
-'git difftool' falls back to 'git mergetool' config variables when the
-difftool equivalents have not been defined.
-
diff.tool::
The default diff tool to use.
--
2.12.0.1.g5415fdfc5.dirty
^ permalink raw reply related
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