* [PATCH v4 2/3] connect: tighten check for unexpected early hang up
From: Jonathan Tan @ 2016-09-09 17:36 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, spearce, sbeller, gitster, peff
In-Reply-To: <cover.1473441620.git.jonathantanmy@google.com>
From: Jonathan Nieder <jrnieder@gmail.com>
A server hanging up immediately to mark access being denied does not
send any .have refs, shallow lines, or anything else before hanging
up. If the server has sent anything, then the hangup is unexpected.
That is, if the server hangs up after a shallow line but before sending
any refs, then git should tell me so:
fatal: The remote end hung up upon initial contact
instead of suggesting an access control problem:
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
Noticed while examining this code. This case isn't likely to come up
in practice but tightening the check makes the code easier to read and
manipulate.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
connect.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/connect.c b/connect.c
index 722dc3f..0c01a49 100644
--- a/connect.c
+++ b/connect.c
@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
return check_ref(ref->name, flags);
}
-static void die_initial_contact(int got_at_least_one_head)
+static void die_initial_contact(int unexpected)
{
- if (got_at_least_one_head)
+ if (unexpected)
die("The remote end hung up upon initial contact");
else
die("Could not read from remote repository.\n\n"
@@ -115,10 +115,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
struct sha1_array *shallow_points)
{
struct ref **orig_list = list;
- int got_at_least_one_head = 0;
+
+ /*
+ * A hang-up after seeing some response from the other end
+ * means that it is unexpected, as we know the other end is
+ * willing to talk to us. A hang-up before seeing any
+ * response does not necessarily mean an ACL problem, though.
+ */
+ int saw_response;
*list = NULL;
- for (;;) {
+ for (saw_response = 0; ; saw_response = 1) {
struct ref *ref;
struct object_id old_oid;
char *name;
@@ -131,7 +138,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
PACKET_READ_GENTLE_ON_EOF |
PACKET_READ_CHOMP_NEWLINE);
if (len < 0)
- die_initial_contact(got_at_least_one_head);
+ die_initial_contact(saw_response);
if (!len)
break;
@@ -171,7 +178,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
oidcpy(&ref->old_oid, &old_oid);
*list = ref;
list = &ref->next;
- got_at_least_one_head = 1;
}
annotate_refs_with_symref_info(*orig_list);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v4 3/3] connect: advertized capability is not a ref
From: Jonathan Tan @ 2016-09-09 17:36 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, jrnieder, spearce, sbeller, gitster, peff
In-Reply-To: <cover.1473441620.git.jonathantanmy@google.com>
When cloning an empty repository served by standard git, "git clone" produces
the following reassuring message:
$ git clone git://localhost/tmp/empty
Cloning into 'empty'...
warning: You appear to have cloned an empty repository.
Checking connectivity... done.
Meanwhile when cloning an empty repository served by JGit, the output is more
haphazard:
$ git clone git://localhost/tmp/empty
Cloning into 'empty'...
Checking connectivity... done.
warning: remote HEAD refers to nonexistent ref, unable to checkout.
This is a common command to run immediately after creating a remote repository
as preparation for adding content to populate it and pushing. The warning is
confusing and needlessly worrying.
The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
with no refs in upload service., 2013-08-08), JGit's ref advertisement includes
a ref named capabilities^{} to advertise its capabilities on, while git's ref
advertisement is empty in this case. This allows the client to learn about the
server's capabilities and is needed, for example, for fetch-by-sha1 to work
when no refs are advertised.
This also affects "ls-remote". For example, against an empty repository served
by JGit:
$ git ls-remote git://localhost/tmp/empty
0000000000000000000000000000000000000000 capabilities^{}
Git advertises the same capabilities^{} ref in its ref advertisement for push
but since it never did so for fetch, the client didn't need to handle this
case. Handle it.
In this aspect, JGit is compliant with the specification in pack-protocol.txt.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
connect.c | 17 +++++++++++++++++
t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+)
diff --git a/connect.c b/connect.c
index 0c01a49..cb3cd97 100644
--- a/connect.c
+++ b/connect.c
@@ -123,6 +123,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
* response does not necessarily mean an ACL problem, though.
*/
int saw_response;
+ int got_dummy_ref_with_capabilities_declaration = 0;
*list = NULL;
for (saw_response = 0; ; saw_response = 1) {
@@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
continue;
}
+ if (!strcmp(name, "capabilities^{}")) {
+ if (saw_response)
+ warning("protocol error: unexpected capabilities^{}, "
+ "continuing anyway");
+ if (got_dummy_ref_with_capabilities_declaration)
+ warning("protocol error: multiple capabilities^{}, "
+ "continuing anyway");
+ got_dummy_ref_with_capabilities_declaration = 1;
+ continue;
+ }
+
if (!check_ref(name, flags))
continue;
+
+ if (got_dummy_ref_with_capabilities_declaration)
+ warning("protocol error: unexpected ref after capabilities^{}, "
+ "using this ref and continuing anyway");
+
ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
oidcpy(&ref->old_oid, &old_oid);
*list = ref;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 819b9dd..befdfee 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -207,5 +207,45 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
test_cmp expect actual
'
+test_lazy_prereq GIT_DAEMON '
+ test_tristate GIT_TEST_GIT_DAEMON &&
+ test "$GIT_TEST_GIT_DAEMON" != false
+'
+
+# This test spawns a daemon, so run it only if the user would be OK with
+# testing with git-daemon.
+test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
+ JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&
+ JGIT_DAEMON_PID= &&
+ git init --bare empty.git &&
+ >empty.git/git-daemon-export-ok &&
+ mkfifo jgit_daemon_output &&
+ {
+ jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
+ JGIT_DAEMON_PID=$!
+ } &&
+ test_when_finished kill "$JGIT_DAEMON_PID" &&
+ {
+ read line &&
+ case $line in
+ Exporting*)
+ ;;
+ *)
+ echo "Expected: Exporting" &&
+ false;;
+ esac &&
+ read line &&
+ case $line in
+ "Listening on"*)
+ ;;
+ *)
+ echo "Expected: Listening on" &&
+ false;;
+ esac
+ } <jgit_daemon_output &&
+ # --exit-code asks the command to exit with 2 when no
+ # matching refs are found.
+ test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
+'
test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v4 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh
From: Jonathan Tan @ 2016-09-09 17:36 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, jrnieder, spearce, sbeller, gitster, peff
In-Reply-To: <cover.1473441620.git.jonathantanmy@google.com>
This enables JGIT to be used as a prereq in invocations of
test_expect_success (and other functions) in other test scripts.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
t/t5310-pack-bitmaps.sh | 4 ----
t/test-lib.sh | 4 ++++
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..1e376ea 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '
git pack-objects --stdout --revs <revs >/dev/null
'
-test_lazy_prereq JGIT '
- type jgit
-'
-
test_expect_success JGIT 'we can read jgit bitmaps' '
git clone . compat-jgit &&
(
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..c9c1037 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
test "$uid" != 0
'
+test_lazy_prereq JGIT '
+ type jgit
+'
+
# SANITY is about "can you correctly predict what the filesystem would
# do by only looking at the permission bits of the files and
# directories?" A typical example of !SANITY is running the test
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v4 0/3] handle empty spec-compliant remote repos correctly
From: Jonathan Tan @ 2016-09-09 17:36 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, jrnieder, spearce, sbeller, gitster, peff
In-Reply-To: <cover.1472836026.git.jonathantanmy@google.com>
Updates:
o Included tighten-check patch from Jonathan Nieder and Junio C Hamano
in this patch set
o Updated commit message following Jonathan Nieder's suggestion
o Updated warning messages to mention capabilities^{}
As for warning vs die, I would prefer the "liberal" approach of continuing on
when facing a recoverable error (that is, "warning"). But I agree that there
are good points in favor of using fatal errors ("die") and I can switch to that
if there is consensus.
Jonathan Nieder (1):
connect: tighten check for unexpected early hang up
Jonathan Tan (2):
tests: move test_lazy_prereq JGIT to test-lib.sh
connect: advertized capability is not a ref
connect.c | 35 +++++++++++++++++++++++++++++------
t/t5310-pack-bitmaps.sh | 4 ----
t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++
t/test-lib.sh | 4 ++++
4 files changed, 73 insertions(+), 10 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* Re: [PATCH v3 2/4] cat-file: introduce the --filters option
From: Junio C Hamano @ 2016-09-09 17:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Torsten Bögershausen, Jeff King
In-Reply-To: <xmqqvay5c7t3.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> So I would not mind if we define the semantics of "--filters" as
> such (as long as it is clearly documented, of course). AFAICS, the
> batch interface does not call filter_object() for non-blobs, and by
> returning successfully without doing anything special for a symbolic
> link from filter_object() automatically gives us the "by default
> return as-is, but give processed output only for regular file blobs"
> semantics to the batch mode.
>
> But for a non-batch mode, it feels somewhat funny to be giving the
> as-is output without saying anything to symbolic links; we can argue
> that it is being consistent with what we do in the batch mode,
> though.
In other words, instead of trying to be consistent by erroring out
in non-regular blob case, I think the attached change on top would
make more sense, by consistently passing the object contents as-is
for all "not filtered" cases, whether it is run from the batch mode
or from the command line.
builtin/cat-file.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f8a3a08..99cb525 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -33,12 +33,7 @@ static int filter_object(const char *path, unsigned mode,
if (!*buf)
return error(_("cannot read object %s '%s'"),
sha1_to_hex(sha1), path);
- if (type != OBJ_BLOB) {
- free(*buf);
- return error(_("blob expected for %s '%s'"),
- sha1_to_hex(sha1), path);
- }
- if (S_ISREG(mode)) {
+ if ((type == OBJ_BLOB) && S_ISREG(mode)) {
struct strbuf strbuf = STRBUF_INIT;
if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
free(*buf);
^ permalink raw reply related
* Re: [PATCH v3 2/4] cat-file: introduce the --filters option
From: Junio C Hamano @ 2016-09-09 17:16 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Torsten Bögershausen, Jeff King
In-Reply-To: <xmqqbmzxdpjp.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> > + if (type != OBJ_BLOB) {
>>> > + free(*buf);
>>> > + return error(_("blob expected for %s '%s'"),
>>> > + sha1_to_hex(sha1), path);
>>> > + }
>>> > + if (S_ISREG(mode)) {
>>> > + struct strbuf strbuf = STRBUF_INIT;
>>> > + if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
>>> > + free(*buf);
>>> > + *size = strbuf.len;
>>> > + *buf = strbuf_detach(&strbuf, NULL);
>>> > + }
>>> > + }
>>>
>>> This needs to error out when mode is not ISREG just like it errors
>>> out when type is not BLOB.
>>
>> Are you sure that this is desirable in batch mode?
>
> I do not quite see a reason why we should not diagnose a bad input
> that does not produce a filtered result. In batch mode or not, it
> diagnoses when the user feeds it a non-blob, and I think it should
> do so for non-regular, too. Both are "you asked me to filter, but
> you shouldn't have".
Stepping back a bit, I can also see a use case that would be helped
if this filter_object() function by default gives the contents of
the requested object as-is, unless the object is a regular blob with
a path for which filtering is defined. Driving such a mechanism via
the batch interface will allow you to first ask about the top-level
tree object (given back to you as-is), and you can iterate over its
entries recursively and get the blobs to be placed in a new working
directory (i.e. "git archive" piped to "tar xf" but regular files
are passed thru convert_to_working_tree()). In such an application,
after you learn the mode from the containing tree object and know
that RelNotes is a symbolic link blob, you still would want the
contents out of the pipe going to the same batch interface process
that is not filtered.
So I would not mind if we define the semantics of "--filters" as
such (as long as it is clearly documented, of course). AFAICS, the
batch interface does not call filter_object() for non-blobs, and by
returning successfully without doing anything special for a symbolic
link from filter_object() automatically gives us the "by default
return as-is, but give processed output only for regular file blobs"
semantics to the batch mode.
But for a non-batch mode, it feels somewhat funny to be giving the
as-is output without saying anything to symbolic links; we can argue
that it is being consistent with what we do in the batch mode,
though.
Thanks.
^ permalink raw reply
* Re: git commit -p with file arguments
From: Christian Neukirchen @ 2016-09-09 17:05 UTC (permalink / raw)
To: git
In-Reply-To: <CA+P7+xoN+q_Kst=qXG_HRznxbN7cbyi5uZe15zq1c16EifeK1Q@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> It wants to commit bar too because you already added bar before. It works like:
>
> "git add bar && git add -p foo && git commit" does it not?
>
> I fail to see why "git commit -p <path>" would unstage the bar you
> already added? Or am I missing some assumption here?
Yet the commit message comment says:
# Explicit paths specified without -i or -o; assuming --only paths...
But files are committed which were not given on the command line.
My confusion is that I use "git commit" with explicit files, yet other
files are committed. AFAICS, this only happens with -p.
--
Christian Neukirchen <chneukirchen@gmail.com> http://chneukirchen.org
^ permalink raw reply
* Re: git commit -p with file arguments
From: Jacob Keller @ 2016-09-09 16:57 UTC (permalink / raw)
To: Christian Neukirchen; +Cc: Git mailing list
In-Reply-To: <87zinmhx68.fsf@juno.home.vuxu.org>
On Mon, Sep 5, 2016 at 2:08 PM, Christian Neukirchen
<chneukirchen@gmail.com> wrote:
> Hi,
>
> I noticed the following suprising behavior:
>
> % git --version
> git version 2.10.0
>
> % git add bar
> % git status -s
> A bar
> M foo
>
> % git commit -p foo
> [stage a hunk]
> ...
> # Explicit paths specified without -i or -o; assuming --only paths...
> # On branch master
> # Changes to be committed:
> # new file: bar
> # modified: foo
> #
>
> So why does it want to commit bar too, when I explicitly wanted to
> commit foo only?
It wants to commit bar too because you already added bar before. It works like:
"git add bar && git add -p foo && git commit" does it not?
I fail to see why "git commit -p <path>" would unstage the bar you
already added? Or am I missing some assumption here?
Thanks,
Jake
>
> This is not how "git commit files..." works, and the man page says
>
> 3.by listing files as arguments to the commit command, in which
> case the commit will ignore changes staged in the index, and
> instead record the current content of the listed files (which must
> already be known to Git);
>
> I'd expect "git commit -p files..." to work like
> "git add -p files... && git commit files...".
>
I guess the part about "git commit files" is different from "git
commit -p files", which is confusing.
> Thanks,
> --
> Christian Neukirchen <chneukirchen@gmail.com> http://chneukirchen.org
>
^ permalink raw reply
* Re: Missing RPM spec file in tarball
From: Stefan Beller @ 2016-09-09 16:42 UTC (permalink / raw)
To: Sergio Martín Turiel; +Cc: git@vger.kernel.org
In-Reply-To: <4c42a1f4-4f03-0fdf-8bd2-8a7f1f978073@accelya.com>
On Fri, Sep 9, 2016 at 9:19 AM, Sergio Martín Turiel
<sergio.martin@accelya.com> wrote:
> Hello,
>
>
> I am trying to build RPM packages from tarball (release 2.9.3 and 2.10.0),
> and i do not find git.spec file, in previous releases i can found it (e.g.
> 2.8.3).
>
> O.S.: CentOS 7.2
> Command: rpmbuild -ta git-2.9.3.tar.gz
> Response: error: Failed to read spec file from git-2.9.3.tar.gz
>
We deleted the rpm target as it was breaking all the time and not reported
in a timely manner, i.e. the impression was it was always broken.
See https://kernel.googlesource.com/pub/scm/git/git/+/ab214331cf8c73f8f77540aa996eb8b4938237f2
> Can you tell me what I'm doing wrong?
Not crying out loud when that commit was discussed on the
mailing list. ;)
>
>
> Thank you very much and best regards, Sergio Martín.
Thanks,
Stefan
^ permalink raw reply
* Re: Missing RPM spec file in tarball
From: Junio C Hamano @ 2016-09-09 16:42 UTC (permalink / raw)
To: Sergio Martín Turiel; +Cc: git
In-Reply-To: <4c42a1f4-4f03-0fdf-8bd2-8a7f1f978073@accelya.com>
Sergio Martín Turiel <sergio.martin@accelya.com> writes:
> I am trying to build RPM packages from tarball (release 2.9.3 and
> 2.10.0), and i do not find git.spec file, in previous releases i can
> found it (e.g. 2.8.3).
>
> O.S.: CentOS 7.2
> Command: rpmbuild -ta git-2.9.3.tar.gz
> Response: error: Failed to read spec file from git-2.9.3.tar.gz
Since ab214331 ("Makefile: stop pretending to support rpmbuild",
2016-04-04) that is used in Git 2.9 and later, i.e.
Makefile: stop pretending to support rpmbuild
Nobody in the active development community seems to watch breakages
in the rpmbuild target. As most major RPM based distros use their
own specfile when packaging us, they aren't looking after us as
their pristine upstream tree, either. At this point, it is turning
to be a disservice to the users to pretend that our tree natively
supports "make rpmbuild" target when we do not properly maintain it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
we no longer ship an outdated git.spec and git.spec.in files. This
was done after finding out that nobody noticed that git.spec has
been left broken since around Git 2.8.0 (i.e. we stopped shipping
README without adjusting the reference to it from the git.spec
file).
^ permalink raw reply
* Missing RPM spec file in tarball
From: Sergio Martín Turiel @ 2016-09-09 16:19 UTC (permalink / raw)
To: git
Hello,
I am trying to build RPM packages from tarball (release 2.9.3 and
2.10.0), and i do not find git.spec file, in previous releases i can
found it (e.g. 2.8.3).
O.S.: CentOS 7.2
Command: rpmbuild -ta git-2.9.3.tar.gz
Response: error: Failed to read spec file from git-2.9.3.tar.gz
Can you tell me what I'm doing wrong?
Thank you very much and best regards, Sergio Martín.
^ permalink raw reply
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"
From: Junio C Hamano @ 2016-09-09 16:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, Dakota Hawkins, git
In-Reply-To: <alpine.DEB.2.20.1609091731540.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Thu, 11 Aug 2016, Junio C Hamano wrote:
>>
>> Do you have a preference either way to help us decide if we want to
>> take this change or not?
>
> I have no strong preference. I guess that it does not hurt to go with the
> patch, and it would probably help in a few cases.
OK. Let me dig the change back and how well it still fits ;-)
Thanks.
^ permalink raw reply
* Re: [PATCH v3 2/4] cat-file: introduce the --filters option
From: Junio C Hamano @ 2016-09-09 16:08 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Torsten Bögershausen, Jeff King
In-Reply-To: <alpine.DEB.2.20.1609091800020.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > + if (type != OBJ_BLOB) {
>> > + free(*buf);
>> > + return error(_("blob expected for %s '%s'"),
>> > + sha1_to_hex(sha1), path);
>> > + }
>> > + if (S_ISREG(mode)) {
>> > + struct strbuf strbuf = STRBUF_INIT;
>> > + if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
>> > + free(*buf);
>> > + *size = strbuf.len;
>> > + *buf = strbuf_detach(&strbuf, NULL);
>> > + }
>> > + }
>>
>> This needs to error out when mode is not ISREG just like it errors
>> out when type is not BLOB.
>
> Are you sure that this is desirable in batch mode?
I do not quite see a reason why we should not diagnose a bad input
that does not produce a filtered result. In batch mode or not, it
diagnoses when the user feeds it a non-blob, and I think it should
do so for non-regular, too. Both are "you asked me to filter, but
you shouldn't have".
^ permalink raw reply
* Re: [PATCH v3 2/4] cat-file: introduce the --filters option
From: Johannes Schindelin @ 2016-09-09 16:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Torsten Bögershausen, Jeff King
In-Reply-To: <xmqqfup9ds9p.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 9 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > +static int filter_object(const char *path, unsigned mode,
> > + const unsigned char *sha1,
> > + char **buf, unsigned long *size)
> > +{
> > + enum object_type type;
> > +
> > + *buf = read_sha1_file(sha1, &type, size);
> > + if (!*buf)
> > + return error(_("cannot read object %s '%s'"),
> > + sha1_to_hex(sha1), path);
> > + if (type != OBJ_BLOB) {
> > + free(*buf);
> > + return error(_("blob expected for %s '%s'"),
> > + sha1_to_hex(sha1), path);
> > + }
> > + if (S_ISREG(mode)) {
> > + struct strbuf strbuf = STRBUF_INIT;
> > + if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
> > + free(*buf);
> > + *size = strbuf.len;
> > + *buf = strbuf_detach(&strbuf, NULL);
> > + }
> > + }
>
> This needs to error out when mode is not ISREG just like it errors
> out when type is not BLOB.
Are you sure that this is desirable in batch mode?
Ciao,
Dscho
^ permalink raw reply
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"
From: Johannes Schindelin @ 2016-09-09 15:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Dakota Hawkins, git
In-Reply-To: <xmqqvaz7x6vv.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 11 Aug 2016, Junio C Hamano wrote:
> Earlier, Peff sent this patch (slightly buried in a discussion) on
> "rebase -i" in <20160729223134.GA22591@sigill.intra.peff.net>.
>
> > Subject: rebase-interactive: drop early check for valid ident
> >
> > Since the very inception of interactive-rebase in 1b1dce4
> > (Teach rebase an interactive mode, 2007-06-25), there has
> > been a preemptive check, before looking at any commits, to
> > see whether the user has a valid name/email combination.
> >
> > This is convenient, because it means that we abort the
> > operation before even beginning (rather than just
> > complaining that we are unable to pick a particular commit).
> >
> > However, it does the wrong thing when the rebase does not
> > actually need to generate any new commits (e.g., a
> > fast-forward with no commits to pick, or one where the base
> > stays the same, and we just pick the same commits without
> > rewriting anything). In this case it may complain about the
> > lack of ident, even though one would not be needed to
> > complete the operation.
> >
> > This may seem like mere nit-picking, but because interactive
> > rebase underlies the "preserve-merges" rebase, somebody who
> > has set "pull.rebase" to "preserve" cannot make even a
> > fast-forward pull without a valid ident, as we bail before
> > even realizing the fast-forward nature.
> >
> > This commit drops the extra ident check entirely. This means
> > we rely on individual commands that generate commit objects
> > to complain. So we will continue to notice and prevent cases
> > that actually do create commits, but with one important
> > difference: we fail while actually executing the "pick"
> > operations, and leave the rebase in a conflicted, half-done
> > state.
> >
> > In some ways this is less convenient, but in some ways it is
> > more so; the user can then manually commit or even "git
> > rebase --continue" after setting up their ident (or
> > providing it as a one-off on the command line).
> >
> > Reported-by: Dakota Hawkins <dakotahawkins@gmail.com>
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
>
> To which, I responded (referring to the last paragraph):
>
> Yup, that is the controvercial bit, and I suspect Dscho's original
> was siding for the "set up ident first, as you will need it anyway
> eventually", so I'll let others with viewpoints different from us to
> chime in first before picking it up.
>
> Do you have a preference either way to help us decide if we want to
> take this change or not?
I have no strong preference. I guess that it does not hurt to go with the
patch, and it would probably help in a few cases.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file
From: Johannes Schindelin @ 2016-09-09 15:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narębski, git
In-Reply-To: <xmqqeg53wj7a.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 1 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> though). The "one sequencer to rule them all" may even have to say
> >> "now give name ':1' to the result of the previous operation" in one
> >> step and in another later step have an instruction "merge ':1'".
> >> When that happens, you cannot even pre-populate the commit object
> >> when the sequencer reads the file, as the commit has not yet been
> >> created at that point.
> >
> > These considerations are pretty hypothetical. I would even place a bet
> > that we will *never* have ":1" as names, not if I have anything to say...
> > ;-)
>
> If you can always work with pre-existing commit, then you can
> validate all object references that appear in the instructions
> upfront.
Or if *some* of the commands work with pre-existing commits, *those*
commands can be validated up-front.
Which is exactly what my code does.
> I was sort of expecting that, when you do the preserve-merges mode
> of "rebase -i", you would need to jump around, doing "we have
> reconstructed the side branch on a new 'onto', let's give the result
> this temporary name ':1', and then switch to the trunk (which would
> call for 'reset <commit>' instruction) and merge that thing (which
> would be 'merge :1' or perhaps called 'pick :1')", and at that point
> you no longer validate the object references upfront.
Except that is not how --preserve-merges works: it *still* uses the SHA-1s
as identifiers, even when the SHA-1 may have changed in the meantime.
That is part of why it was a bad design.
> If you do not have to have such a "mark this point" and a "refer to
> that point we previously marked", then I agree that you should be
> able to pre-validate and keep the result in the structure.
Even then, those markers should *still* be validated. They, too, need to
be created and later used, usage before creation would be an error.
But...
1) this is not yet a problem, so why are we discussing it here? Do we not
have actual problems with these patches to discuss anymore?
2) the SHA-1s that *can* be validated *should* be validated, so I find the
objection a little bogus.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 2/4] cat-file: introduce the --filters option
From: Junio C Hamano @ 2016-09-09 15:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Torsten Bögershausen, Jeff King
In-Reply-To: <084088ba86c0af3636d960276c0bfdf7f5d2cfde.1473415827.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> +static int filter_object(const char *path, unsigned mode,
> + const unsigned char *sha1,
> + char **buf, unsigned long *size)
> +{
> + enum object_type type;
> +
> + *buf = read_sha1_file(sha1, &type, size);
> + if (!*buf)
> + return error(_("cannot read object %s '%s'"),
> + sha1_to_hex(sha1), path);
> + if (type != OBJ_BLOB) {
> + free(*buf);
> + return error(_("blob expected for %s '%s'"),
> + sha1_to_hex(sha1), path);
> + }
> + if (S_ISREG(mode)) {
> + struct strbuf strbuf = STRBUF_INIT;
> + if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
> + free(*buf);
> + *size = strbuf.len;
> + *buf = strbuf_detach(&strbuf, NULL);
> + }
> + }
This needs to error out when mode is not ISREG just like it errors
out when type is not BLOB.
Other than that, I think these four patches are good to go.
Thanks.
^ permalink raw reply
* Re: [PATCH 21/22] sequencer: left-trim the lines read from the script
From: Johannes Schindelin @ 2016-09-09 15:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narębski, git
In-Reply-To: <xmqqvayfwlgu.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 1 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> > Interactive rebase's scripts may be indented; We need to handle this
> >> > case, too, now that we prepare the sequencer to process interactive
> >> > rebases.
> >>
> >> s/; We need/; we need/
> >
> > Hrmpf. From http://grammar.ccc.commnet.edu/grammar/marks/colon.htm:
> >
> > There is some disagreement among writing reference manuals about
> > when you should capitalize an independent clause following a
> > colon. Most of the manuals advise that when you have more than one
> > sentence in your explanation or when your sentence(s) is a formal
> > quotation, a capital is a good idea. The NYPL Writer's Guide urges
> > consistency within a document; the Chicago Manual of Style says
> > you may begin an independent clause with a lowercase letter unless it's
> > one of those two things (a quotation or more than one sentence).
> > The APA Publication Manual is the most extreme: it advises us to
> > always capitalize an independent clause following a colon. The advice
> > given above is consistent with the Gregg Reference Manual.
> >
> > Based on that, I think that a capital is the correct case here.
>
> Does that manual have anything to say about semicolons, which is a
> different thing?
You're correct, I overlooked that.
Fixed,
Dscho
^ permalink raw reply
* Re: [PATCH 22/22] sequencer: refactor write_message()
From: Johannes Schindelin @ 2016-09-09 14:40 UTC (permalink / raw)
To: Jakub Narębski; +Cc: git, Junio C Hamano
In-Reply-To: <dbc1b08c-a151-29ab-a5a2-45343ca556d6@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 950 bytes --]
Hi Kuba,
On Fri, 2 Sep 2016, Jakub Narębski wrote:
> W dniu 01.09.2016 o 16:20, Johannes Schindelin pisze:
> > On Thu, 1 Sep 2016, Jakub Narębski wrote:
> >> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
>
> >>> if (commit_lock_file(&msg_file) < 0)
> >>> return error(_("Error wrapping up %s."), filename);
> >>
> >> Another "while at it"... though the one that can be safely postponed
> >> (well, the make message easier to understand part, not the quote
> >> filename part):
> >>
> >> return error(_("Error wrapping up writing to '%s'."), filename);
> >
> > As I inherited this message, I'll keep it.
>
> Well, please then add quotes while at it, at least, for consistency
>
> return error(_("Error wrapping up '%s'."), filename);
I may do that as a final patch, once all the other concerns are addressed.
I really do not want to change the error message during the conversion.
Ciao,
Dscho
^ permalink raw reply
* [PATCH v3 17/17] sequencer: ensure to release the lock when we could not read the index
From: Johannes Schindelin @ 2016-09-09 14:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
A future caller of read_and_refresh_cache() may want to do more than just
print some helpful advice in case of failure.
Suggested by Junio Hamano.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index d92a632..eec8a60 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -644,14 +644,18 @@ static int read_and_refresh_cache(struct replay_opts *opts)
{
static struct lock_file index_lock;
int index_fd = hold_locked_index(&index_lock, 0);
- if (read_index_preload(&the_index, NULL) < 0)
+ if (read_index_preload(&the_index, NULL) < 0) {
+ rollback_lock_file(&index_lock);
return error(_("git %s: failed to read the index"),
action_name(opts));
+ }
refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
if (the_index.cache_changed && index_fd >= 0) {
- if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
+ if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {
+ rollback_lock_file(&index_lock);
return error(_("git %s: failed to refresh the index"),
action_name(opts));
+ }
}
rollback_lock_file(&index_lock);
return 0;
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 07/17] sequencer: lib'ify prepare_revs()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).
The only caller of prepare_revs(), walk_revs_populate_todo() was just
taught to return errors, after verifying that its callers are prepared
to handle error returns, and with this step, we make it notice an
error return from this function.
So this is a safe conversion to make prepare_revs() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index ab599e0..7fd0f99 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -623,7 +623,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
return res;
}
-static void prepare_revs(struct replay_opts *opts)
+static int prepare_revs(struct replay_opts *opts)
{
/*
* picking (but not reverting) ranges (but not individual revisions)
@@ -633,10 +633,11 @@ static void prepare_revs(struct replay_opts *opts)
opts->revs->reverse ^= 1;
if (prepare_revision_walk(opts->revs))
- die(_("revision walk setup failed"));
+ return error(_("revision walk setup failed"));
if (!opts->revs->commits)
- die(_("empty commit set passed"));
+ return error(_("empty commit set passed"));
+ return 0;
}
static void read_and_refresh_cache(struct replay_opts *opts)
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 15/17] sequencer: lib'ify fast_forward_to()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).
The only caller of fast_forward_to(), do_pick_commit() already checks
the return value and passes it on to its callers, so its caller must
be already prepared to handle error returns, and with this step, we
make it notice an error return from this function.
So this is a safe conversion to make fast_forward_to() callable from
new callers that want it not to die, without changing the external
behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 021ddf3..d92a632 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -226,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
read_cache();
if (checkout_fast_forward(from, to, 1))
- exit(128); /* the callee should have complained already */
+ return -1; /* the callee should have complained already */
strbuf_addf(&sb, _("%s: fast-forward"), action_name(opts));
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 13/17] sequencer: lib'ify save_todo()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).
The only caller of save_todo(), pick_commits() can already return
errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.
So this is a safe conversion to make save_todo() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 7a1561e..32c53bb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -943,24 +943,31 @@ static int sequencer_rollback(struct replay_opts *opts)
return -1;
}
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
{
static struct lock_file todo_lock;
struct strbuf buf = STRBUF_INIT;
int fd;
- fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);
- if (format_todo(&buf, todo_list, opts) < 0)
- die(_("Could not format %s."), git_path_todo_file());
+ fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);
+ if (fd < 0)
+ return error_errno(_("Could not lock '%s'"),
+ git_path_todo_file());
+ if (format_todo(&buf, todo_list, opts) < 0) {
+ strbuf_release(&buf);
+ return error(_("Could not format %s."), git_path_todo_file());
+ }
if (write_in_full(fd, buf.buf, buf.len) < 0) {
strbuf_release(&buf);
- die_errno(_("Could not write to %s"), git_path_todo_file());
+ return error_errno(_("Could not write to %s"),
+ git_path_todo_file());
}
if (commit_lock_file(&todo_lock) < 0) {
strbuf_release(&buf);
- die(_("Error wrapping up %s."), git_path_todo_file());
+ return error(_("Error wrapping up %s."), git_path_todo_file());
}
strbuf_release(&buf);
+ return 0;
}
static void save_opts(struct replay_opts *opts)
@@ -1009,7 +1016,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
return -1;
for (cur = todo_list; cur; cur = cur->next) {
- save_todo(cur, opts);
+ if (save_todo(cur, opts))
+ return -1;
res = do_pick_commit(cur->item, opts);
if (res)
return res;
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 16/17] lib'ify checkout_fast_forward_to()
From: Johannes Schindelin @ 2016-09-09 14:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).
The only callers of checkout_fast_forward_to(), cmd_merge(),
pull_into_void(), cmd_pull() and sequencer's fast_forward_to(),
already check the return value and handle it appropriately. With this
step, we make it notice an error return from this function.
So this is a safe conversion to make checkout_fast_forward_to()
callable from new callers that want it not to die, without changing
the external behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
merge.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/merge.c b/merge.c
index 5db7d56..23866c9 100644
--- a/merge.c
+++ b/merge.c
@@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head,
refresh_cache(REFRESH_QUIET);
- hold_locked_index(lock_file, 1);
+ if (hold_locked_index(lock_file, 0) < 0)
+ return -1;
memset(&trees, 0, sizeof(trees));
memset(&opts, 0, sizeof(opts));
@@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head,
}
if (unpack_trees(nr_trees, t, &opts))
return -1;
- if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
- die(_("unable to write new index file"));
+ if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {
+ rollback_lock_file(lock_file);
+ return error(_("unable to write new index file"));
+ }
return 0;
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 14/17] sequencer: lib'ify save_opts()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).
The only caller of save_opts(), sequencer_pick_revisions() can already
return errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.
So this is a safe conversion to make save_opts() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 32c53bb..021ddf3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -970,37 +970,39 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
return 0;
}
-static void save_opts(struct replay_opts *opts)
+static int save_opts(struct replay_opts *opts)
{
const char *opts_file = git_path_opts_file();
+ int res = 0;
if (opts->no_commit)
- git_config_set_in_file(opts_file, "options.no-commit", "true");
+ res |= git_config_set_in_file_gently(opts_file, "options.no-commit", "true");
if (opts->edit)
- git_config_set_in_file(opts_file, "options.edit", "true");
+ res |= git_config_set_in_file_gently(opts_file, "options.edit", "true");
if (opts->signoff)
- git_config_set_in_file(opts_file, "options.signoff", "true");
+ res |= git_config_set_in_file_gently(opts_file, "options.signoff", "true");
if (opts->record_origin)
- git_config_set_in_file(opts_file, "options.record-origin", "true");
+ res |= git_config_set_in_file_gently(opts_file, "options.record-origin", "true");
if (opts->allow_ff)
- git_config_set_in_file(opts_file, "options.allow-ff", "true");
+ res |= git_config_set_in_file_gently(opts_file, "options.allow-ff", "true");
if (opts->mainline) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "%d", opts->mainline);
- git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+ res |= git_config_set_in_file_gently(opts_file, "options.mainline", buf.buf);
strbuf_release(&buf);
}
if (opts->strategy)
- git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+ res |= git_config_set_in_file_gently(opts_file, "options.strategy", opts->strategy);
if (opts->gpg_sign)
- git_config_set_in_file(opts_file, "options.gpg-sign", opts->gpg_sign);
+ res |= git_config_set_in_file_gently(opts_file, "options.gpg-sign", opts->gpg_sign);
if (opts->xopts) {
int i;
for (i = 0; i < opts->xopts_nr; i++)
- git_config_set_multivar_in_file(opts_file,
+ res |= git_config_set_multivar_in_file_gently(opts_file,
"options.strategy-option",
opts->xopts[i], "^$", 0);
}
+ return res;
}
static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
@@ -1147,7 +1149,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return error(_("Can't revert as initial commit"));
if (save_head(sha1_to_hex(sha1)))
return -1;
- save_opts(opts);
+ if (save_opts(opts))
+ return -1;
return pick_commits(todo_list, opts);
}
--
2.10.0.windows.1.10.g803177d
^ 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