* Re: [PATCH] add: allow users to silence Git 2.0 warnings about "add -u"
From: Matthieu Moy @ 2013-02-22 9:32 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
In-Reply-To: <CAJDDKr4dCJ3p9QBGr09kW4_0BsVJcpE7s83=eNxKE15pMznWCw@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> Please enlighten me.
> Are we really getting rid of it and replacing it with ":/"?
> That syntax looks like a meh face.. just sayin'
The current behavior is indeed replaced by "git add -u .", not ":/".
> Unlike push.default, whose warning can be silenced with configuration,
> git 1.x does not have a way to silence this warning without retraining
> existing users.
Yes, but push.default is really different: there is a config variable,
and we want the behavior to be configurable. In the case of "git add",
I don't think adding a configuration option would be the right thing.
That would mean typing "git add -u" on an account which isn't yours will
be unpredictable *forever*.
OTOH, "git add -u :/" and "git add -u ." will behave predictibly on any
version of Git that accepts them, past present or future (:/ was added
in 1.7.6, a year and a half ago).
> Another example...
>
> $ git grep 'stuff' :/
>
> would it be too much to teach it to do:
>
> $ git grep -u 'stuff'
"git grep" is out of the scope of this change. Yes, it is inconsistant
with the rest of Git, but doesn't seem to surprise users as much as "git
add -u" (for which the inconsistancy appears within the "add" command).
I don't understand what you mean by "git grep -u". "git add -u" is a
shortcut for "git add --update", and "git grep --update" wouldn't make
sense to me. Do you mean we should add a "--full-tree" to "git grep"?
That seems really overkill to me since we already have the :/ pathspec.
> but in 2.0 that -u would be a no-op because "grep" will be full tree, no?
No it won't.
> I need to read the old discussions.
> Can someone tell me the magic google search syntax they use to dig them up?
See the discussion here:
http://thread.gmane.org/gmane.comp.version-control.git/213988/focus=214106
(recursively, there's a pointer to an older discussion)
> Would a better way be a method to make "git add -u" behave like 2.0 today?
As I said, I think adding a configuration option that would remain after
2.0 would do more harm than good. But after thinking about it, I'm not
against an option like a boolean add.use2dot0Behavior that would:
* Right now, adopt the future behavior and kill the warning
* From 2.0, kill the warning without changing the bevavior
* When we stop warning, disapear.
This, or the add.silence-pathless-warnings (which BTW should be spelled
add.silencePathlessWarnings) would not harm as long as they are not
advertized in the warning. What we don't want is dumb users reading half
the message and apply the quickest receipe they find to kill the warning
without thinking about the consequences.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: -B option of git log
From: Eckhard Maass @ 2013-02-22 10:14 UTC (permalink / raw)
To: git; +Cc: Jeff King
In-Reply-To: <20130222005704.GA24287@sigill.intra.peff.net>
On 02/22/2013 01:57 AM, Jeff King wrote:
> I think the problem is that your test file is so tiny that it falls
> afoul of git's MINIMUM_BREAK_SIZE heuristic of 400 bytes (which prevents
> false positives on tiny files). Try replacing your "Lorem ipsum" echo
> with something like "seq 1 150", and I think you will find the result
> you are expecting.
Thanks. Two points, though:
With bigger files, I do get something like:
| M100 d
| R100 d e
The rename is fine. But I am a bit puzzled mit the M - I, somehow,
would have expected an A for add and not a M.
Secondly, would it be a good idea to enhance the documentation on
that point to clarify this minimum size?
SEcki
^ permalink raw reply
* Re: [PATCH v3 15/19] pkt-line: share buffer/descriptor reading implementation
From: Eric Sunshine @ 2013-02-22 11:22 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220200430.GO25647@sigill.intra.peff.net>
On Wed, Feb 20, 2013 at 3:04 PM, Jeff King <peff@peff.net> wrote:
> diff --git a/pkt-line.h b/pkt-line.h
> index fa93e32..47361f5 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -25,9 +25,16 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f
> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
>
> /*
> - * Read a packetized line from the descriptor into the buffer, which must be at
> - * least size bytes long. The return value specifies the number of bytes read
> - * into the buffer.
> + * Read a packetized line into the buffer, which must be at least size bytes
> + * long. The return value specifies the number of bytes read into the buffer.
> + *
> + * If src_buffer is not NULL (and nor is *src_buffer), it should point to a
> + * buffer containing the packet data to parse, of at least *src_len bytes.
> + * After the function returns, src_buf will be increments and src_len
s/increments/incremented/
> + * decremented by the number of bytes consumed.
^ permalink raw reply
* [PATCH v2] branch: segfault fixes and validation
From: Nguyễn Thái Ngọc Duy @ 2013-02-22 11:47 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Per Cederqvist,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7vvc9lv9rt.fsf@alter.siamese.dyndns.org>
branch_get() can return NULL (so far on detached HEAD only) but some
code paths in builtin/branch.c cannot deal with that and cause
segfaults. While at there, make sure we bail out when the user gives 2
or more arguments, but we only process the first one and silently
ignore the rest.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
On Fri, Feb 22, 2013 at 12:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> branch_get() can return NULL (so far on detached HEAD only)...
>
> Do you anticipate any other cases where the API call should validly
> return NULL?
No. But I do not see any guarantee that it may never do that in
future either. Which is why I was deliberately vague with "could not
figure out...". But you also correctly observed my laziness there. So
how about this? It makes a special case for HEAD but not insist on
detached HEAD as the only cause.
builtin/branch.c | 24 ++++++++++++++++++++++++
t/t3200-branch.sh | 21 +++++++++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/builtin/branch.c b/builtin/branch.c
index 6371bf9..82ed337 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -889,6 +889,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
} else if (new_upstream) {
struct branch *branch = branch_get(argv[0]);
+ if (argc > 1)
+ die(_("too many branches to set new upstream"));
+
+ if (!branch) {
+ if (!argc || !strcmp(argv[0], "HEAD"))
+ die(_("HEAD does not point to any branch. Is it detached?"));
+ die(_("no such branch '%s'"), argv[0]);
+ }
+
if (!ref_exists(branch->refname))
die(_("branch '%s' does not exist"), branch->name);
@@ -901,6 +910,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
struct branch *branch = branch_get(argv[0]);
struct strbuf buf = STRBUF_INIT;
+ if (argc > 1)
+ die(_("too many branches to unset upstream"));
+
+ if (!branch) {
+ if (!argc || !strcmp(argv[0], "HEAD"))
+ die(_("HEAD does not point to any branch. Is it detached?"));
+ die(_("no such branch '%s'"), argv[0]);
+ }
+
if (!branch_has_merge_config(branch)) {
die(_("Branch '%s' has no upstream information"), branch->name);
}
@@ -916,6 +934,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
int branch_existed = 0, remote_tracking = 0;
struct strbuf buf = STRBUF_INIT;
+ if (!strcmp(argv[0], "HEAD"))
+ die(_("it does not make sense to create 'HEAD' manually"));
+
+ if (!branch)
+ die(_("no such branch '%s'"), argv[0]);
+
if (kinds != REF_LOCAL_BRANCH)
die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index f3e0e4a..12f1e4a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -42,6 +42,10 @@ test_expect_success \
'git branch a/b/c should create a branch' \
'git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c'
+test_expect_success \
+ 'git branch HEAD should fail' \
+ 'test_must_fail git branch HEAD'
+
cat >expect <<EOF
$_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000 branch: Created from master
EOF
@@ -388,6 +392,14 @@ test_expect_success \
'git tag foobar &&
test_must_fail git branch --track my11 foobar'
+test_expect_success '--set-upstream-to fails on multiple branches' \
+ 'test_must_fail git branch --set-upstream-to master a b c'
+
+test_expect_success '--set-upstream-to fails on detached HEAD' \
+ 'git checkout HEAD^{} &&
+ test_must_fail git branch --set-upstream-to master &&
+ git checkout -'
+
test_expect_success 'use --set-upstream-to modify HEAD' \
'test_config branch.master.remote foo &&
test_config branch.master.merge foo &&
@@ -417,6 +429,15 @@ test_expect_success 'test --unset-upstream on HEAD' \
test_must_fail git branch --unset-upstream
'
+test_expect_success '--unset-upstream should fail on multiple branches' \
+ 'test_must_fail git branch --unset-upstream a b c'
+
+test_expect_success '--unset-upstream should fail on detached HEAD' \
+ 'git checkout HEAD^{} &&
+ test_must_fail git branch --unset-upstream &&
+ git checkout -
+'
+
test_expect_success 'test --unset-upstream on a particular branch' \
'git branch my15
git branch --set-upstream-to master my14 &&
--
1.8.1.2.536.gf441e6d
^ permalink raw reply related
* [PATCH 1/2] index-format.txt: mention of v4 is missing in some places
From: Nguyễn Thái Ngọc Duy @ 2013-02-22 12:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/technical/index-format.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/technical/index-format.txt b/Documentation/technical/index-format.txt
index 27c716b..0810251 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -12,7 +12,7 @@ Git index format
The signature is { 'D', 'I', 'R', 'C' } (stands for "dircache")
4-byte version number:
- The current supported versions are 2 and 3.
+ The current supported versions are 2, 3 and 4.
32-bit number of index entries.
@@ -93,8 +93,8 @@ Git index format
12-bit name length if the length is less than 0xFFF; otherwise 0xFFF
is stored in this field.
- (Version 3) A 16-bit field, only applicable if the "extended flag"
- above is 1, split into (high to low bits).
+ (Version 3 or later) A 16-bit field, only applicable if the
+ "extended flag" above is 1, split into (high to low bits).
1-bit reserved for future
--
1.8.1.2.536.gf441e6d
^ permalink raw reply related
* [PATCH 2/2] read-cache.c: use INDEX_FORMAT_{LB,UB} in verify_hdr()
From: Nguyễn Thái Ngọc Duy @ 2013-02-22 12:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1361534964-4232-1-git-send-email-pclouds@gmail.com>
9d22778 (read-cache.c: write prefix-compressed names in the index -
2012-04-04) defined these. Interestingly, they were not used by
read-cache.c, or anywhere in that patch. They were used in
builtin/update-index.c later for checking supported index
versions. Use them here too.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
read-cache.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/read-cache.c b/read-cache.c
index 827ae55..298161f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1260,7 +1260,8 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)
if (hdr->hdr_signature != htonl(CACHE_SIGNATURE))
return error("bad signature");
hdr_version = ntohl(hdr->hdr_version);
- if (hdr_version < 2 || 4 < hdr_version)
+ if (hdr_version < INDEX_FORMAT_LB ||
+ hdr_version > INDEX_FORMAT_UB)
return error("bad index version %d", hdr_version);
git_SHA1_Init(&c);
git_SHA1_Update(&c, hdr, size - 20);
--
1.8.1.2.536.gf441e6d
^ permalink raw reply related
* question about the ultimate source tracking
From: Blind @ 2013-02-22 14:52 UTC (permalink / raw)
To: Git List
Hi there,
Going through your plans about your meetings this year,
I noticed that there is an idea since 2005,
named Linus's ultimate source tracking something (why not "Lustra" for short?).
I see its still not completed till this day.
So I decided to give it a thought. And I have an idea.
But before to continue to analyze my idea, I want to ask you a question,
because the analyzes will take some time.
Are (approximately) 10% more objects in the store a good trade
for almost immediate answer to the question which line came from where?
Blind.
^ permalink raw reply
* Re: [PATCH] Fix in Git.pm cat_blob crashes on large files
From: Joshua Clayton @ 2013-02-22 15:11 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20130221232448.GA23736@sigill.intra.peff.net>
running git svn fetch on a remote repository (yes I know there are a
lot of possible outside variables, including network latency)
Code with 1024 reads and 64k writes:
real 75m19.906s
user 16m43.919s
sys 29m16.326s
Code with 1024 reads and 1024 writes:
real 71m21.006s
user 12m36.275s
sys 24m26.112s
...so the simpler code wins the trivial test.
I would say go with it.
Should I resubmit?
On Thu, Feb 21, 2013 at 3:24 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 21, 2013 at 03:18:40PM -0800, Joshua Clayton wrote:
>
>> > By having the read and flush size be the same, it's much simpler.
>>
>> My original bugfix did just read 1024, and write 1024. That works fine
>> and, yes, is simpler.
>> I changed it to be more similar to the original code in case there
>> were performance reasons for doing it that way.
>> That was the only reason I could think of for the design, and adding
>> the $flushSize variable means that
>> some motivated person could easily optimize it.
>>
>> So far I have been too lazy to profile the two versions....
>> I guess I'll try a trivial git svn init; git svn fetch and check back in.
>> Its running now.
>
> I doubt it will make much of a difference; we are already writing to a
> perl filehandle, so it will be buffered there (which I assume is 4K, but
> I haven't checked). And your version retains the 1024-byte read. I do
> think 1024 is quite low for this sort of descriptor-to-descriptor
> copying. I'd be tempted to just bump that 1024 to 64K.
>
>> In git svn fetch (which is how I discovered it) the file being passed
>> to cat_blob is a temporary file, which is checksummed before putting
>> it into place.
>
> Great. There may be other callers outside of our tree, of course, but I
> think it's pretty clear that the responsibility is on the caller to make
> sure the function succeeded. We are changing what gets put on the output
> stream for various error conditions, but ultimately that is an
> implementation detail that the caller should not be depending on.
>
> -Peff
^ permalink raw reply
* patch for fix build git on Haiku
From: diger @ 2013-02-22 15:13 UTC (permalink / raw)
To: git
This patch fixes build git on Haiku
----------------
$NetBSD$
--- Makefile.orig 2012-10-21 21:32:15.000000000 +0000
+++ Makefile
@@ -1211,6 +1204,16 @@ ifeq ($(uname_S),HP-UX)
endif
GIT_TEST_CMP = cmp
endif
+ifeq ($(uname_S),Haiku)
+ NO_R_TO_GCC_LINKER = YesPlease
+ NO_LIBGEN_H = YesPlease
+ NO_MEMMEM = YesPlease
+ NO_MKSTEMPS = YesPlease
+ NEEDS_LIBICONV = YesPlease
+ DEFAULT_EDITOR = nano
+ PTHREAD_LIBS = -lroot
+ NO_CROSS_DIRECTORY_HARDLINKS = YesPlease
+endif
ifeq ($(uname_S),Windows)
GIT_VERSION := $(GIT_VERSION).MSVC
pathsep = ;
--------------------
--
Jabber ID: admin@freenet59.ru
--
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: --inline-single
From: Adam Spiers @ 2013-02-22 15:32 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git list
In-Reply-To: <20130221231328.GA19808@sigill.intra.peff.net>
On Thu, Feb 21, 2013 at 06:13:28PM -0500, Jeff King wrote:
> On Thu, Feb 21, 2013 at 12:26:22PM -0800, Junio C Hamano wrote:
>
> > Some people may find it convenient to append a simple patch at the
> > bottom of a discussion e-mail separated by a "scissors" mark, ready
> > to be applied with "git am -c". Introduce "--inline-single" option
> > to format-patch to do so. A typical usage example might be to start
> > 'F'ollow-up to a discussion, write your message, conclude with "a
> > patch to do so may look like this.", and
> >
> > \C-u M-! git format-patch --inline-single -1 HEAD <ENTER>
> >
> > if you are an Emacs user. Users of other MUA's may want to consult
> > their manuals to find equivalent command to append output from an
> > external command to the message being composed.
>
> Interesting. I usually just do this by hand, but this could save a few
> keystrokes in my workflow.
Same here. This is great; thanks a lot both for working on it!
^ permalink raw reply
* Re: [PATCH] Fix in Git.pm cat_blob crashes on large files
From: Jeff King @ 2013-02-22 15:38 UTC (permalink / raw)
To: Joshua Clayton; +Cc: git
In-Reply-To: <CAMB+bf+RyerXP3VP26KJM7wtVKOup15AeRpEewwKJx3=bL9+hQ@mail.gmail.com>
On Fri, Feb 22, 2013 at 07:11:54AM -0800, Joshua Clayton wrote:
> running git svn fetch on a remote repository (yes I know there are a
> lot of possible outside variables, including network latency)
> Code with 1024 reads and 64k writes:
>
> real 75m19.906s
> user 16m43.919s
> sys 29m16.326s
>
> Code with 1024 reads and 1024 writes:
>
> real 71m21.006s
> user 12m36.275s
> sys 24m26.112s
>
> ...so the simpler code wins the trivial test.
Interesting; I'd have expected no change or a slight win for your
version, which makes me wonder if the outside variables are dominating.
I wonder what 64K/64K would look like.
> I would say go with it.
> Should I resubmit?
Yes, please.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: --inline-single
From: Junio C Hamano @ 2013-02-22 16:47 UTC (permalink / raw)
To: Jeff King; +Cc: git list, Adam Spiers
In-Reply-To: <20130221231328.GA19808@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> ... <helper function to see if the user is the author> ...
>> +}
>
> Nice, I'm glad you handled this case properly. I've wondered if we
> should have an option to do a similar test when writing out the "real"
> message format. I.e., to put the extra "From" line in the body of the
> message when !is_current_user(). Traditionally we have just said "that
> is the responsibility of the MUA you use", and let send-email handle it.
> But it means people who do not use send-email have to reimplement the
> feature themselves.
I am not sure if I follow. Do you mean that you have to remove
fewer lines if you omit Date/From when it is from you in the first
place? People who do not use send-email (like me) slurp the output
0001-have-gostak-distim-doshes.patch into their MUA editor, tell the
MUA to use the contents on the Subject: line as the subject, and
remove what is redundant, including the Subject. Because the output
cannot be used as-is anyway, I do not think it is such a big deal.
And those who have a custom mechanism to stuff our output in their
MUA's outbox, similar to what imap-send does, would already have to
have a trivial parser to read the first part of our output up to the
first blank line (i.e. parsing out the header part) and formatting
the information it finds into a form that is understood by their
MUA. Omitting From: or Date: lines would not help those people who
already have established the procedure to handle the "Oh, this one
is from me" case, or to send the output always with the Sender: and
keeping the From: intact. So,...
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2013, #05; Tue, 12)
From: Junio C Hamano @ 2013-02-22 16:58 UTC (permalink / raw)
To: Miles Bader; +Cc: Andrew Ardill, git@vger.kernel.org
In-Reply-To: <87hal4n3z1.fsf@catnip.gol.com>
Miles Bader <miles@gnu.org> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>> * Introduce "git add --ignore-removal" option in the release after
>> the current cycle (a new feature is too late for this cycle):
>
> Too late in the cycle even if the option is simply ignored ... ?
>
> [To extend the range of git versions where it's not an error]
I'd feel safer to have enough time to cook the "alleged no-op"
before merging it to 'master' and include it in a release.
Possible implementation mistakes aside, "--ignore-removal" is
probably too long to type, we haven't even discussed if it deserves
a short-and-sweet single letter option, the obvious "-i" is not
available, etc. etc. I do not think we have a concensus that the
transition plan outlined is a good way to go in the first place.
So, I do think it is a bit too late for this cycle, especially when
we still have doubts about the design. Actually it is *I* who have
doubts; I do not even know if other people share the doubts or they
support the direction wholeheartedly.
^ permalink raw reply
* [PATCH] Fix in Git.pm cat_blob crashes on large files (reviewed by Jeff King)
From: Joshua Clayton @ 2013-02-22 17:03 UTC (permalink / raw)
To: git
Read and write each 1024 byte buffer, rather than trying to buffer
the entire content of the file.
Previous code would crash on all files > 2 Gib, when the offset variable
became negative (perhaps below the level of perl), resulting in a crash.
On a 32 bit system, or a system with low memory it might crash before
reaching 2 GiB due to memory exhaustion.
Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
perl/Git.pm | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 931047c..cc91288 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -949,13 +949,16 @@ sub cat_blob {
last unless $bytesLeft;
my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;
- my $read = read($in, $blob, $bytesToRead, $bytesRead);
+ my $read = read($in, $blob, $bytesToRead);
unless (defined($read)) {
$self->_close_cat_blob();
throw Error::Simple("in pipe went bad");
}
-
$bytesRead += $read;
+ unless (print $fh $blob) {
+ $self->_close_cat_blob();
+ throw Error::Simple("couldn't write to passed in filehandle");
+ }
}
# Skip past the trailing newline.
@@ -970,11 +973,6 @@ sub cat_blob {
throw Error::Simple("didn't find newline after blob");
}
- unless (print $fh $blob) {
- $self->_close_cat_blob();
- throw Error::Simple("couldn't write to passed in filehandle");
- }
-
return $size;
}
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (reviewed by Jeff King)
From: Erik Faye-Lund @ 2013-02-22 17:10 UTC (permalink / raw)
To: Joshua Clayton; +Cc: git
In-Reply-To: <CAMB+bfLjewgKjSN6WNHGkwpRx9OaWipi_-TDa0MnJMDN+6tPZA@mail.gmail.com>
On Fri, Feb 22, 2013 at 6:03 PM, Joshua Clayton
<stillcompiling@gmail.com> wrote:
> Read and write each 1024 byte buffer, rather than trying to buffer
> the entire content of the file.
> Previous code would crash on all files > 2 Gib, when the offset variable
> became negative (perhaps below the level of perl), resulting in a crash.
> On a 32 bit system, or a system with low memory it might crash before
> reaching 2 GiB due to memory exhaustion.
>
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
We usually put "Reviewed-by: Name <email>" right below the SoB-line,
rather than in the subject. And then we CC the people who were
involved in the previous round :)
^ permalink raw reply
* Re: [PATCH] archive: let remote clients get reachable commits
From: Junio C Hamano @ 2013-02-22 17:10 UTC (permalink / raw)
To: Sergey Sergeev; +Cc: Jeff King, git@vger.kernel.org
In-Reply-To: <995301361532360@web22h.yandex.ru>
Sergey Sergeev <gurugray@yandex.ru> writes:
[jc: please do not top-post]
> You are right,
> I'll rethink this patch and write some test for this cases.
Thanks.
Note that this is harder to implement than one would naïvely think,
if one aims for a very generic solution, without walking the whole
history.
I personally think that it is OK to limit the scope to expressions
that start from the tip of ref and expressions that start with the
SHA-1 at the tip of ref, e.g.
master~12:Documentation
v2.6.11:arch/alpha
5dc01c595e6c6ec9ccda # tag v2.6.11
26791a8bcf0e6d33f43a:arch # tag v2.6.12
26791a8bcf0~12:arch # starting at 26791a8b and dig down
are OK, while forbidding the following:
c39ae07f393806ccf406 # tree of tag v2.6.11
9ee1c939d1cb936b1f98 # commit v2.6.12^0
9ee1c939d1cb936b1f98: # tree of commit v2.6.12^0
9ee1c939d1cb936b1f98:arch # subtree of commit v2.6.12^0
which will make it significantly easier to implement the necessary
validation in a robust way.
^ permalink raw reply
* Re: [PATCH] add: allow users to silence Git 2.0 warnings about "add -u"
From: Junio C Hamano @ 2013-02-22 17:18 UTC (permalink / raw)
To: David Aguilar; +Cc: git, Matthieu Moy
In-Reply-To: <CAJDDKr4dCJ3p9QBGr09kW4_0BsVJcpE7s83=eNxKE15pMznWCw@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> Please enlighten me.
As you lack the knowledge of previous discussion, I think you will
be the best person to proofread the paragraph on this issue in the
"backward compatibilty notes" section of the draft release notes to
v1.8.2 to see if that is understandable to the end users and point
out what are missing or confusing.
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: --inline-single
From: Jeff King @ 2013-02-22 17:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list, Adam Spiers
In-Reply-To: <7vmwuws3bo.fsf@alter.siamese.dyndns.org>
On Fri, Feb 22, 2013 at 08:47:39AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> ... <helper function to see if the user is the author> ...
> >> +}
> >
> > Nice, I'm glad you handled this case properly. I've wondered if we
> > should have an option to do a similar test when writing out the "real"
> > message format. I.e., to put the extra "From" line in the body of the
> > message when !is_current_user(). Traditionally we have just said "that
> > is the responsibility of the MUA you use", and let send-email handle it.
> > But it means people who do not use send-email have to reimplement the
> > feature themselves.
>
> I am not sure if I follow. Do you mean that you have to remove
> fewer lines if you omit Date/From when it is from you in the first
> place?
Sorry, I think I confused you by going off on a tangent. The rest of my
email was about dropping unnecessary lines from the inline view. But
here I was talking about another possible use of the "is user the
author" function. For the existing view, we show:
From: A U Thor <author@example.com>
Date: ...
Subject: [PATCH] whatever
body
and if committer != author, we expect the MUA to convert that to:
From: C O Mitter <committer@example.com>
Date: ...
Subject: [PATCH] whatever
From: A U Thor <author@example.com>
body
That logic happens in git-send-email right now, but given that your
patch adds the "are we the author?" function, it would be trivial to add
a "--sender-is-committer" option to format-patch to have it do it
automatically. That saves the MUA from having to worry about it.
> People who do not use send-email (like me) slurp the output
> 0001-have-gostak-distim-doshes.patch into their MUA editor, tell the
> MUA to use the contents on the Subject: line as the subject, and
> remove what is redundant, including the Subject. Because the output
> cannot be used as-is anyway, I do not think it is such a big deal.
That is one way to do it. Another way is to hand the output of
format-patch to your MUA as a template, making it a starting point for a
message we are about to send. No manual editing is necessary in that
case, unless the "From" header does not match the sender identity.
> And those who have a custom mechanism to stuff our output in their
> MUA's outbox, similar to what imap-send does, would already have to
> have a trivial parser to read the first part of our output up to the
> first blank line (i.e. parsing out the header part) and formatting
> the information it finds into a form that is understood by their
> MUA.
Not necessarily. The existing format is an rfc822 message, which mailers
understand already. It's perfectly cromulent to do:
git format-patch --stdout "$@" >mbox &&
mutt -f mbox
and use mutt's "resend-message" as a starting point for sending each
message. No editing is necessary except for adding recipients (which you
can also do on the command-line to format-patch).
> Omitting From: or Date: lines would not help those people who
> already have established the procedure to handle the "Oh, this one
> is from me" case, or to send the output always with the Sender: and
> keeping the From: intact. So,...
Right, my point was to help people who _should_ have implemented the
"oh, this one is from me" case, but were too lazy to do so (and it's
actually a little tricky to get right, because you might have to adjust
the mime headers to account for encoded author names).
-Peff
^ permalink raw reply
* Re: [PATCH] archive: let remote clients get reachable commits
From: Jeff King @ 2013-02-22 17:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sergey Sergeev, git@vger.kernel.org
In-Reply-To: <7vehg8s295.fsf@alter.siamese.dyndns.org>
On Fri, Feb 22, 2013 at 09:10:46AM -0800, Junio C Hamano wrote:
> I personally think that it is OK to limit the scope to expressions
> that start from the tip of ref and expressions that start with the
> SHA-1 at the tip of ref, e.g.
>
> master~12:Documentation
> v2.6.11:arch/alpha
> 5dc01c595e6c6ec9ccda # tag v2.6.11
> 26791a8bcf0e6d33f43a:arch # tag v2.6.12
> 26791a8bcf0~12:arch # starting at 26791a8b and dig down
>
> are OK, while forbidding the following:
>
> c39ae07f393806ccf406 # tree of tag v2.6.11
> 9ee1c939d1cb936b1f98 # commit v2.6.12^0
> 9ee1c939d1cb936b1f98: # tree of commit v2.6.12^0
> 9ee1c939d1cb936b1f98:arch # subtree of commit v2.6.12^0
>
> which will make it significantly easier to implement the necessary
> validation in a robust way.
How are you proposing to verify master~12 in that example? Because
during parsing, it starts with "master", and we remember that? Or
because you are doing a reachability traversal on all refs after we
parse?
-Peff
^ permalink raw reply
* Re: [PATCH] add: allow users to silence Git 2.0 warnings about "add -u"
From: Junio C Hamano @ 2013-02-22 17:30 UTC (permalink / raw)
To: Matthieu Moy; +Cc: David Aguilar, git
In-Reply-To: <vpqd2vssnh7.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Yes, but push.default is really different: there is a config variable,
> and we want the behavior to be configurable. In the case of "git add",
> I don't think adding a configuration option would be the right thing.
> That would mean typing "git add -u" on an account which isn't yours will
> be unpredictable *forever*.
Exactly.
>> $ git grep 'stuff' :/
>>
>> would it be too much to teach it to do:
>>
>> $ git grep -u 'stuff'
>
> "git grep" is out of the scope of this change. Yes, it is inconsistant
> with the rest of Git, but doesn't seem to surprise users as much as "git
> add -u" (for which the inconsistancy appears within the "add" command).
It is consistent with "grep", and the reason "git grep" behaves that
way is because consistency with "grep" matters more. I do not think
it is going to change.
Another is "clean", which I do not personally care too deeply about;
it being a destructive operation that is only used interactively and
occasionally), the current behaviour to limit it to the cwd is much
more sensible than making it go and touch parts of the tree that is
unrelated to cwd.
> I don't understand what you mean by "git grep -u".
I think he meant to add "git grep --full-tree" or something, and I
do not think it is going to happen when we have ":/" magic pathspec.
> As I said, I think adding a configuration option that would remain after
> 2.0 would do more harm than good. But after thinking about it, I'm not
> against an option like a boolean add.use2dot0Behavior that would:
>
> * Right now, adopt the future behavior and kill the warning
>
> * From 2.0, kill the warning without changing the bevavior
>
> * When we stop warning, disapear.
It is marginally better than David's "set once without thinking
because I read it on stackoverflow without fully understanding the
ramifications, and forget about it only to suffer when Git 2.0
happens" configuration variable, but by not much. You can easily
imagine
Q. Help, Git 1.8.2 is giving me this warning. What to do?
A. Set this configuration variable. period.
and many users setting it without realizing that they are making the
operation tree-wide at the same time. We'd want to see this answer
instead:
A. Say "git add -u ."; you want to add changed paths in the
current directory.
Another problem with use2dot0 configuration is that it would invite
people to imagine that setting it to false will keep the old
behaviour forever, which is against what you set out to do with the
patch under discussion.
^ permalink raw reply
* [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)
From: Joshua Clayton @ 2013-02-22 17:30 UTC (permalink / raw)
To: git; +Cc: Jeff King, Erik Faye-Lund
Read and write each 1024 byte buffer, rather than trying to buffer
the entire content of the file.
Previous code would crash on all files > 2 Gib, when the offset variable
became negative (perhaps below the level of perl), resulting in a crash.
On a 32 bit system, or a system with low memory it might crash before
reaching 2 GiB due to memory exhaustion.
Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
---
perl/Git.pm | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 931047c..cc91288 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -949,13 +949,16 @@ sub cat_blob {
last unless $bytesLeft;
my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;
- my $read = read($in, $blob, $bytesToRead, $bytesRead);
+ my $read = read($in, $blob, $bytesToRead);
unless (defined($read)) {
$self->_close_cat_blob();
throw Error::Simple("in pipe went bad");
}
-
$bytesRead += $read;
+ unless (print $fh $blob) {
+ $self->_close_cat_blob();
+ throw Error::Simple("couldn't write to passed
in filehandle");
+ }
}
# Skip past the trailing newline.
@@ -970,11 +973,6 @@ sub cat_blob {
throw Error::Simple("didn't find newline after blob");
}
- unless (print $fh $blob) {
- $self->_close_cat_blob();
- throw Error::Simple("couldn't write to passed in filehandle");
- }
-
return $size;
}
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH] archive: let remote clients get reachable commits
From: Junio C Hamano @ 2013-02-22 18:06 UTC (permalink / raw)
To: Jeff King; +Cc: Sergey Sergeev, git@vger.kernel.org
In-Reply-To: <20130222172710.GB17475@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> How are you proposing to verify master~12 in that example? Because
> during parsing, it starts with "master", and we remember that?
By not cheating (i.e. using get_sha1()), but making sure you can
parse "master" and the adornment on it "~12" is something sane.
That is why I said "this is harder than one would naively think, but
limiting will make it significantly easier". I didn't say that it
would become "trivial", did I?
^ permalink raw reply
* Re: [PATCH] archive: let remote clients get reachable commits
From: Jeff King @ 2013-02-22 18:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sergey Sergeev, git@vger.kernel.org
In-Reply-To: <7vfw0odxz3.fsf@alter.siamese.dyndns.org>
On Fri, Feb 22, 2013 at 10:06:56AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > How are you proposing to verify master~12 in that example? Because
> > during parsing, it starts with "master", and we remember that?
>
> By not cheating (i.e. using get_sha1()), but making sure you can
> parse "master" and the adornment on it "~12" is something sane.
So, like these patches:
http://article.gmane.org/gmane.comp.version-control.git/188386
http://article.gmane.org/gmane.comp.version-control.git/188387
? They do not allow arbitrary sha1s that happen to point to branch tips,
but I am not sure whether that is something people care about or not.
> That is why I said "this is harder than one would naively think, but
> limiting will make it significantly easier". I didn't say that it
> would become "trivial", did I?
I'm not implying it would be trivial. It was an honest question, since
you did not seem to want to do the pass-more-information-out-of-get-sha1
approach last time this came up.
Even though those patches above are from me, I've come to the conclusion
that the best thing to do is to harmonize with upload-pack. Then you
never have the "well, but I could fetch it, so why won't upload-archive
let me get it" argument. Something like:
1. split name at first colon (like we already do)
2. make sure the left-hand side is reachable according to the same
rules that upload-pack uses. Right we just say "is it a ref". It
should be:
2a. if it is a commit-ish, is it reachable from a ref?
2b. otherwise, is it pointed to directly by a ref?
3. Abort if it's not reachable. Abort if it's not a tree-ish. No
checks necessary on the right-hand side, because a path lookup in a
tree-ish is always reachable from the tree-ish. I.e., the same rule
we have now.
I did not check if upload-pack will respect a "want" line for an object
accessible only by peeling a tag. But an obvious 2c could be "is it
accessible by peeling the refs?"
That leaves the only inaccessible thing as direct-sha1s of trees and
blobs that are reachable from commits. But you also cannot ask for those
directly via upload-pack, and I do not think it's worth it to do the
much more expensive reachability check to verify those (OTOH, it is no
more expensive than the current "counting objects" for a clone, and we
could do it only as a fallback when cheaper checks do not work).
-Peff
^ permalink raw reply
* Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)
From: Jeff King @ 2013-02-22 18:34 UTC (permalink / raw)
To: Joshua Clayton; +Cc: git, Erik Faye-Lund
In-Reply-To: <CAMB+bfLvpKNLaEUyUUYsO5n2y+9tyd_QcnPVzX0s2Z2t3Fr9=g@mail.gmail.com>
On Fri, Feb 22, 2013 at 09:30:57AM -0800, Joshua Clayton wrote:
> Read and write each 1024 byte buffer, rather than trying to buffer
> the entire content of the file.
OK. Did you ever repeat your timing with a larger symmetric buffer? That
should probably be a separate patch on top, but it might be worth doing
while we are thinking about it.
> Previous code would crash on all files > 2 Gib, when the offset variable
> became negative (perhaps below the level of perl), resulting in a crash.
I'm still slightly dubious of this, just because it doesn't match my
knowledge of perl (which is admittedly imperfect). I'm curious how you
diagnosed it?
> On a 32 bit system, or a system with low memory it might crash before
> reaching 2 GiB due to memory exhaustion.
>
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> Reviewed-by: Jeff King <peff@peff.net>
The commit message is a good place to mention any side effects, and why
they are not a problem. Something like:
The previous code buffered the whole blob before writing, so any error
reading from cat-file would result in zero bytes being written to the
output stream. After this change, the output may be left in a
partially written state (or even fully written, if we fail when
parsing the final newline from cat-file). However, it's not reasonable
for callers to expect anything about the state of the output when we
return an error (after all, even with full buffering, we might fail
during the writing process). So any caller which cares about this is
broken already, and we do not have to worry about them.
> ---
> perl/Git.pm | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
The patch itself looks fine to me.
-Peff
^ permalink raw reply
* Re: [PATCH] git-commit: populate the edit buffer with 2 blank lines before s-o-b
From: Junio C Hamano @ 2013-02-22 18:35 UTC (permalink / raw)
To: Brandon Casey; +Cc: pclouds, jrnieder, john, git
In-Reply-To: <1361525158-3648-1-git-send-email-drafnel@gmail.com>
Brandon Casey <drafnel@gmail.com> writes:
> Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with
> a blank line before the Signed-off-by line. This provided a nice
> hint to the user that something should be filled in. Let's restore that
> behavior, but now let's ensure that the Signed-off-by line is preceded
> by two blank lines to hint that something should be filled in, and that
> a blank line should separate it from the Signed-off-by line.
>
> Plus, add a test for this behavior.
>
> Reported-by: John Keeping <john@keeping.me.uk>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> ---
>
> Ok. Here's a patch on top of 959a2623 bc/append-signed-off-by. It
> implements the "2 blank lines preceding sob" behavior.
>
> -Brandon
>
> sequencer.c | 5 +++--
> t/t7502-commit.sh | 12 ++++++++++++
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 53ee49a..2dac106 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1127,9 +1127,10 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> const char *append_newlines = NULL;
> size_t len = msgbuf->len - ignore_footer;
>
> - if (len && msgbuf->buf[len - 1] != '\n')
> + /* ensure a blank line precedes our signoff */
> + if (!len || msgbuf->buf[len - 1] != '\n')
> append_newlines = "\n\n";
> - else if (len > 1 && msgbuf->buf[len - 2] != '\n')
> + else if (len == 1 || msgbuf->buf[len - 2] != '\n')
> append_newlines = "\n";
Maybe I am getting slower with age, but it took me 5 minutes of
staring the above to convince me that it is doing the right thing.
The if/elseif cascade is dealing with three separate things and the
logic is a bit dense:
* Is the buffer completely empty? We need to add two LFs to give a
room for the title and body;
* Otherwise:
- Is the final line incomplete? We need to add one LF to make it a
complete line whatever we do.
- Is the final line an empty line? We need to add one more LF to
make sure we have a blank line before we add S-o-b.
I wondered if we can rewrite it to make the logic clearer (that is
where I spent most of the 5 minutes), but I did not think of a
better way; probably the above is the best we could do.
Thanks.
By the way, I think we would want to introduce a symbolic constants
for the possible return values from has_conforming_footer(). The
check that appears after this hunk
if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
sob.buf, sob.len);
is hard to grok without them.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox