* Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
From: Junio C Hamano @ 2013-02-11 17:18 UTC (permalink / raw)
To: Jeff King
Cc: Jonathan Nieder, Sitaram Chamarty, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <20130211160057.GA16402@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sun, Feb 10, 2013 at 11:17:24PM -0800, Junio C Hamano wrote:
>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>> > Isn't that a criticism of the git-shell-commands facility in general?
>> > If it is common to have a lot of users with distinct home directories
>> > but all with git-shell as their login shell, then the
>> > git-shell-commands should not go in their home directory to begin
>> > with, no?
>>
>> You can give one set of commands to some users while restricting
>> others, no?
>
> But that seems to me to argue against /etc/git/shell-disabled or
> similar, which would apply to every user. Or are you proposing that the
> check be:
>
> if -d ~/git-shell-commands; then
> : ok, interactive
> elif -x /etc/git/shell-disabled; then
> exec /etc/git/shell-disabled
> else
> echo >&2 'go away'
> exit 1
> fi
That "shell-disabled" thing was to allow customizing the existing
die() that triggers here:
} else if (argc == 1) {
/* Allow the user to run an interactive shell */
cd_to_homedir();
if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
die("Interactive git shell is not enabled.\n"
"hint: ~/" COMMAND_DIR " should exist "
"and have read and execute access.");
}
run_shell();
exit(0);
so it is more like
if ! test -d $HOME/git-shell-commands
then
if test -x /etc/git/shell-disabled
then
exec /etc/git/shell-disabled
else
die Interactive is not enabled
fi
fi
... do whatever in run_shell() ...
> That at least means you can apply _whether_ to disable the shell
> selectively for each user (by providing or not a git-shell-commands
> directory), but you cannot individually select the script that runs for
> that user. But it's probably still flexible enough;...
Such a flexibility is not a goal of /etc/git/shell-disabled. The
sole goal is to make the life easier for those site owners that do
not want any interactive shell access to give more friendly and
customized error message.
Those who want further flexibility can exit with non-zero from the
"help" (which is still a misnomer for a hook to disable interactive
for the user).
My primary objection is that implementing only that "more flexible
but requires more configuration work" solution without giving
simpler solution (i.e. just one thing to configure) to the majory of
site owners who only have simpler problem to solve (i.e. just want
to customize "no interactive here"), and saying that the latter can
be done on top. It is backwards mentality.
^ permalink raw reply
* Re: [PATH/RFC] parse-options: report invalid UTF-8 switches
From: Jeff King @ 2013-02-11 17:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Erik Faye-Lund, git
In-Reply-To: <7v7gmeok2e.fsf@alter.siamese.dyndns.org>
On Mon, Feb 11, 2013 at 09:07:53AM -0800, Junio C Hamano wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
> > However, since git only looks at one byte at the time for
> > short-options, it ends up reporting a partial UTF-8 sequence
> > in such cases, leading to corruption of the output.
>
> Isn't it a workable, easier and more robust alternative to punt and
> use the entire ctx.argv[0] as unrecognized?
Yes, but it regresses the usability:
[before]
$ git foobar -qrxs
unknown switch: x
[after]
$ git foobar -qrxs
unknown switch: -qrxs
One is much more informative than the other, and you are punishing the
common ascii case for the extremely uncommon case of utf-8. Maybe:
if (isascii(*ctx.opt))
error("unknown option `%c'", *ctx.opt);
else
error("unknown multi-byte short option in string: `%s'", ctx.argv[0]);
which only kicks in in the uncommon case (and extends the error message
to make it more clear why we are showing the whole string).
-Peff
^ permalink raw reply
* Re: git bisect result off by 1 commit
From: Tim Chen @ 2013-02-11 17:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ak
In-Reply-To: <7v1ucqxmyd.fsf@alter.siamese.dyndns.org>
>
> Looks perfectly normal to me. The message above:
>
> > HEAD is now at a0d271c... Linux 3.6
> > Bisecting: 0 revisions left to test after this (roughly 0 steps)
> > [d54b1a9e0eaca92cde678d19bd82b9594ed00450] perf script: Remove use of die/exit
>
> is asking you to test the commit at d54b1a9e and tell it the result
> of the test. The message says "0 left to test *after* this";
> doesn't it mean you still need to do *this*?
>
> A bisecct session ends when it tells you
>
> XXXXXX is the first bad commit
>
> which I do not see in the above. You seem to have stopped before
> that happens.
Yes, I should do one more step. Thanks for catching.
Tim
^ permalink raw reply
* Re: [PATH/RFC] parse-options: report invalid UTF-8 switches
From: Erik Faye-Lund @ 2013-02-11 17:21 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20130211171957.GG16402@sigill.intra.peff.net>
On Mon, Feb 11, 2013 at 6:19 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 11, 2013 at 09:07:53AM -0800, Junio C Hamano wrote:
>
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>> > However, since git only looks at one byte at the time for
>> > short-options, it ends up reporting a partial UTF-8 sequence
>> > in such cases, leading to corruption of the output.
>>
>> Isn't it a workable, easier and more robust alternative to punt and
>> use the entire ctx.argv[0] as unrecognized?
>
> Yes, but it regresses the usability:
>
> [before]
> $ git foobar -qrxs
> unknown switch: x
>
> [after]
> $ git foobar -qrxs
> unknown switch: -qrxs
>
> One is much more informative than the other, and you are punishing the
> common ascii case for the extremely uncommon case of utf-8. Maybe:
>
> if (isascii(*ctx.opt))
> error("unknown option `%c'", *ctx.opt);
> else
> error("unknown multi-byte short option in string: `%s'", ctx.argv[0]);
>
> which only kicks in in the uncommon case (and extends the error message
> to make it more clear why we are showing the whole string).
Yes. This is IMO a much better approach, and it doesn't involve trying
to figure out what encoding the string is. Thanks!
^ permalink raw reply
* Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
From: Jeff King @ 2013-02-11 17:27 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Sitaram Chamarty, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <7v38x2ojl1.fsf@alter.siamese.dyndns.org>
On Mon, Feb 11, 2013 at 09:18:18AM -0800, Junio C Hamano wrote:
> That "shell-disabled" thing was to allow customizing the existing
> die() that triggers here:
> [...]
> so it is more like
>
> if ! test -d $HOME/git-shell-commands
> then
> if test -x /etc/git/shell-disabled
> then
> exec /etc/git/shell-disabled
> else
> die Interactive is not enabled
> fi
> fi
> ... do whatever in run_shell() ...
OK, that is equivalent to what I said (or at least what I was trying to
say :) ).
> > That at least means you can apply _whether_ to disable the shell
> > selectively for each user (by providing or not a git-shell-commands
> > directory), but you cannot individually select the script that runs for
> > that user. But it's probably still flexible enough;...
>
> Such a flexibility is not a goal of /etc/git/shell-disabled. The
> sole goal is to make the life easier for those site owners that do
> not want any interactive shell access to give more friendly and
> customized error message.
>
> Those who want further flexibility can exit with non-zero from the
> "help" (which is still a misnomer for a hook to disable interactive
> for the user).
Ah, I thought you were proposing shell-disabled _instead_ of Jonathan's
patch, not in addition to.
> My primary objection is that implementing only that "more flexible
> but requires more configuration work" solution without giving
> simpler solution (i.e. just one thing to configure) to the majory of
> site owners who only have simpler problem to solve (i.e. just want
> to customize "no interactive here"), and saying that the latter can
> be done on top. It is backwards mentality.
Oh, absolutely. The easy case should be easy, and the hard case
possible. But another way of doing that (which would also make life
easier for admins who want to share config besides shell-disabled) would
be:
1. Give Jonathan's magic meaning to ~/git-shell-commands/help's exit
code.
2. Make /etc/git/shell-commands a fallback if ~/git-shell-commands
does not exist.
That turns your /etc/git/shell-disabled into /etc/git/shell-commands/help.
It is just as simple to do a site-wide change, still allows per-user
overrides, and additionally gives people who _do_ want the interactive
commands the ability to configure them site-wide instead of symlinking a
directory into everybody's homedir.
The only downside is that it has the confusing "create this directory to
turn on interactivity, then create a file in it to turn it back off"
feature.
I admit I don't care too much, though. I have never actually used
git-shell, as my systems are all either too small (i.e., users are
trusted and have shell access) or too big (grown well beyond a single
server that connects users straight to git-shell). In fact, there seems
to be a lot of guessing in this thread about what people would want, as
it seems none of us actually uses the feature. Maybe that is a sign it
is being over-engineered. :)
-Peff
^ permalink raw reply
* Re: [PATCHv3 5/5] git-send-email: use git credential to obtain password
From: Jeff King @ 2013-02-11 17:31 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Junio C Hamano, Matthieu Moy, git
In-Reply-To: <xa1tmwvag47s.fsf@mina86.com>
On Mon, Feb 11, 2013 at 06:17:27PM +0100, Michal Nazarewicz wrote:
> > I am happy to put it off until it becomes a problem, but I wonder if the
> > Git::credential() interface is sufficient to express what we would want.
> > It only allows two return values: true for approve, false for reject.
> > But we would want a tri-state: approve, reject, indeterminate.
>
> Being it tri-state is not a problem. The last can be easily represented
> by undef.
Yeah, I think undef makes sense there.
> > Reading the Net::SMTP code, it doesn't look like the information is even
> > available to us (it really just passes out success or failure), so I
> > don't think we can even make it work now. But it may be better to
> > prepare the public Git::credential interface for it now, so we do not
> > have to deal with breaking compatibility later.
>
> I guess. I left it as is since git-send-email won't make use of the
> indeterminate values, but I can add it in this patchset as well.
Yeah, I am more worried about third-party uses outside of the Git tree,
which we may then break if we change the meaning of "undef" later.
Thanks.
-Peff
^ permalink raw reply
* Re: [PATCHv3 4/5] Git.pm: add interface for git credential command
From: Jeff King @ 2013-02-11 17:36 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Junio C Hamano, Matthieu Moy, git
In-Reply-To: <xa1tr4kmg4cv.fsf@mina86.com>
On Mon, Feb 11, 2013 at 06:14:24PM +0100, Michal Nazarewicz wrote:
> > Should this return a hash reference? It seems like that is how we end up
> > using and passing it elsewhere (since we have to anyway when passing it
> > as a parameter).
>
> Admittedly I mostly just copied what git-remote-mediawiki did here and
> don't really have any preference either way, even though with this
> function returning a reference the call site would have to become:
>
> %$credential = %{ credential_read $reader };
Oh, right, because Git::credential takes the credential as an in-out
parameter rather than just returning it. Which is a bit unusual in perl,
but keeps the interface reasonably simple. The alternative would be:
$cred = Git::credential $cred, sub {
...
}
which is a little less nice.
> Another alternative would be for it to take a reference as an argument,
> possibly an optional one:
I think that is making things more ugly.
> I'd avoid modifying the hash while reading though since I think it's
> best if it's left intact in case of an error.
Agreed.
> And of course, if we want to get even more crazy, credential_write could
> accept either reference or a hash, like so:
>
> +sub credential_write {
> + my ($self, $writer, @rest) = _maybe_self(@_);
> + my $credential = @rest == 1 ? $rest[0] : { @rest };
> + my ($key, $value);
> + # ...
> +}
Ugh.
> Bottom line is, anything can be coded, but a question is whether it
> makes sense to do so. ;)
Yes, it is probably OK to leave it as-is, then. It is largely a matter
of taste, and I will defer to your judgement on that. :)
-Peff
^ permalink raw reply
* Re: [PATCHv3 0/5] Add git-credential support to git-send-email
From: Jeff King @ 2013-02-11 17:48 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Junio C Hamano, Matthieu Moy, git
In-Reply-To: <xa1tk3qeg46r.fsf@mina86.com>
On Mon, Feb 11, 2013 at 06:18:04PM +0100, Michal Nazarewicz wrote:
> On Mon, Feb 11 2013, Jeff King wrote:
> > I have two minor comments, which I'll reply inline with. But even with
> > those comments, I think this would be OK to merge.
>
> I'll send a new patchset tomorrow with.
Based on our discussion, I think it would just need the patch below
squashed into your 4/5 (this handles the "undef" thing, and I also fixed
a few typos in the API documentation):
---
diff --git a/perl/Git.pm b/perl/Git.pm
index 0e6fcf9..35893e6 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1038,7 +1038,7 @@ sub credential_read {
return %credential;
}
-=item credential_read( FILE_HANDLE, CREDENTIAL_HASH )
+=item credential_write( FILE_HANDLE, CREDENTIAL_HASH )
Writes credential key-value pairs from hash referenced by C<CREDENTIAL_HASH>
to C<FILE_HANDLE>. Keys and values cannot contain new-line or NUL byte
@@ -1102,7 +1102,7 @@ sub _credential_run {
=item credential( CREDENTIAL_HASH, CODE )
Executes C<git credential> for a given set of credentials and
-specified operation. In both form C<CREDENTIAL_HASH> needs to be
+specified operation. In both forms C<CREDENTIAL_HASH> needs to be
a reference to a hash which stores credentials. Under certain
conditions the hash can change.
@@ -1126,11 +1126,14 @@ sub _credential_run {
}
In the second form, C<CODE> needs to be a reference to a subroutine.
-The function will execute C<git credential fill> to fill provided
-credential hash, than call C<CODE> with C<CREDENTIAL> as the sole
-argument, and finally depending on C<CODE>'s return value execute
-C<git credential approve> (if return value yields true) or C<git
-credential reject> (otherwise). The return value is the same as what
+The function will execute C<git credential fill> to fill the provided
+credential hash, then call C<CODE> with C<CREDENTIAL> as the sole
+argument. If C<CODE>'s return value is defined, the function will
+execute C<git credential approve> (if return value yields true) or
+C<git credential reject> (if return value is false). If the return
+value is undef, nothing at all is executed; this is useful, for
+example, if the credential could neither be verified nor rejected due
+to an unrelated network error. The return value is the same as what
C<CODE> returned. With this form, the usage might look as follows:
if (Git::credential {
@@ -1152,7 +1155,9 @@ sub credential {
if ('CODE' eq ref $op_or_code) {
_credential_run $credential, 'fill';
my $ret = $op_or_code->($credential);
- _credential_run $credential, $ret ? 'approve' : 'reject';
+ if (defined $ret) {
+ _credential_run $credential, $ret ? 'approve' : 'reject';
+ }
return $ret;
} else {
_credential_run $credential, $op_or_code;
^ permalink raw reply related
* Re: [PATH/RFC] parse-options: report invalid UTF-8 switches
From: Junio C Hamano @ 2013-02-11 17:54 UTC (permalink / raw)
To: Jeff King; +Cc: Erik Faye-Lund, git
In-Reply-To: <20130211171957.GG16402@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Feb 11, 2013 at 09:07:53AM -0800, Junio C Hamano wrote:
>
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>> > However, since git only looks at one byte at the time for
>> > short-options, it ends up reporting a partial UTF-8 sequence
>> > in such cases, leading to corruption of the output.
>>
>> Isn't it a workable, easier and more robust alternative to punt and
>> use the entire ctx.argv[0] as unrecognized?
>
> Yes, but it regresses the usability:
>
> [before]
> $ git foobar -qrxs
> unknown switch: x
>
> [after]
> $ git foobar -qrxs
> unknown switch: -qrxs
>
> One is much more informative than the other, and you are punishing the
> common ascii case for the extremely uncommon case of utf-8. Maybe:
>
> if (isascii(*ctx.opt))
> error("unknown option `%c'", *ctx.opt);
> else
> error("unknown multi-byte short option in string: `%s'", ctx.argv[0]);
>
> which only kicks in in the uncommon case (and extends the error message
> to make it more clear why we are showing the whole string).
Yup, that is what I meant.
^ permalink raw reply
* Re: Pushing a git repository to a new server
From: Ethan Reesor @ 2013-02-11 18:17 UTC (permalink / raw)
To: Jeff King; +Cc: Konstantin Khomoutov, git
In-Reply-To: <20130211162714.GB16402@sigill.intra.peff.net>
On Mon, Feb 11, 2013 at 11:27 AM, Jeff King <peff@peff.net> wrote:
[...]
> We talked about this a long time ago. One problem is that it's
> inherently unportable, as the procedure to make a repo is potentially
> different on every server (and certainly that is the case between a
> regular user running stock git and something like GitHub or Google Code;
> I imagine even gitolite has some special procedures for creating repos,
> too).
I was more interested in creating something for my self rather than
making any changes to the mainstream git.
^ permalink raw reply
* Re: Pushing a git repository to a new server
From: Ethan Reesor @ 2013-02-11 18:18 UTC (permalink / raw)
To: Konstantin Khomoutov; +Cc: git
In-Reply-To: <20130211164518.ad0a21aff672ad0e4f03a6bb@domain007.com>
On Mon, Feb 11, 2013 at 7:45 AM, Konstantin Khomoutov
<kostix+git@007spb.ru> wrote:
[...]
> OK, here's the sketch.
> On the server, in the home directory of your "git" user, you create a
> wrapper around git-receive-pack, like this:
>
> # mkdir ~git/git-shell-commands
> # cat >~git/git-shell-commands/git-receive-new-repo
> #!/bin/sh
>
> set -e -u
>
> if [ $# -ne 1 ]; then
> echo 'Missing required argument: <directory>' >&2
> exit 1
> fi
>
> mkdir "$1" && git init --quiet --bare "$1" && git-receive-pack "$1"
> ^D
> # chmod +x $_
>
> Then, on the client side, to push a new repo, you just do
>
> $ git push --receive-pack=git-receive-new-repo --all git@server:repo.git
>
> This will make `git push` to spawn not just `git receive-pack <dir>` as
> it usually does but your wrapper, which would first create and
> initialize a bare repository and then spawn `git receive-pack` on it
> which would then communicate with the client side and receive
> everything from it.
>
> You could then create a client-side wrapper script or a Git alias for
> such "creative pushing", like this:
>
> $ git config --add --global alias.push-new-repo \
> 'push --receive-pack=git-receive-new-repo --all'
>
> So the whole client call is now reduced to
>
> $ git push-new-repo git@server:repo.git
Thanks, that's what I was going for.
^ permalink raw reply
* Re: [PATCH 1/2] shell doc: emphasize purpose and security model
From: Junio C Hamano @ 2013-02-11 18:32 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, Sitaram Chamarty, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <20130211055752.GF15329@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
> index 9b925060..4fe93203 100644
> --- a/Documentation/git-shell.txt
> +++ b/Documentation/git-shell.txt
> @@ -9,25 +9,61 @@ git-shell - Restricted login shell for Git-only SSH access
> SYNOPSIS
> --------
> [verse]
> -'git shell' [-c <command> <argument>]
> +'chsh' -s $(which git-shell) git
> +'git clone' `git@localhost:/path/to/repo.git`
> +'ssh' `git@localhost`
I am wondering if we want to do the following instead of/in addition
to fixing the $(which git-shell). It is not like we only allow a
single user and its name has to be 'git'.
diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 4fe9320..6829ea9 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -9,9 +9,9 @@ git-shell - Restricted login shell for Git-only SSH access
SYNOPSIS
--------
[verse]
-'chsh' -s $(which git-shell) git
-'git clone' `git@localhost:/path/to/repo.git`
-'ssh' `git@localhost`
+'chsh' -s /path/to/git-shell <user>
+'git clone' `<user>@localhost:/path/to/repo.git`
+'ssh' `<user>@localhost`
DESCRIPTION
-----------
^ permalink raw reply related
* Re: [PATCHv3 0/5] Add git-credential support to git-send-email
From: Michal Nazarewicz @ 2013-02-11 18:40 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Matthieu Moy, git
In-Reply-To: <20130211174811.GK16402@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]
On Mon, Feb 11 2013, Jeff King wrote:
> Based on our discussion, I think it would just need the patch below
> squashed into your 4/5 (this handles the "undef" thing, and I also fixed
> a few typos in the API documentation):
> @@ -1152,7 +1155,9 @@ sub credential {
> if ('CODE' eq ref $op_or_code) {
> _credential_run $credential, 'fill';
> my $ret = $op_or_code->($credential);
> - _credential_run $credential, $ret ? 'approve' : 'reject';
> + if (defined $ret) {
> + _credential_run $credential, $ret ? 'approve' : 'reject';
> + }
> return $ret;
> } else {
> _credential_run $credential, $op_or_code;
Yep, that's what I did as well. Thanks for spotting the typos,
I actually changed some other wording as well (most notably
CREDENTIAL_HASH -> CREDENTIAL_HASHREF), and also added some unrelated
patch in the middle: <https://github.com/mina86/git/commits/master>.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--
[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]
[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply
* Re: [PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer
From: Brandon Casey @ 2013-02-11 18:49 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <20130211090009.GR15329@elie.Belkin>
On Mon, Feb 11, 2013 at 1:00 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> By the way, regarding what the right "--signoff" behavior is for
> commit, cherry-pick, am, and format-patch:
>
> I think the best behavior would be to check if the last signed-off-by
> line (ignoring acked-by, bug, change-id, and so on lines that follow
> it) matches the one to be added, and if it doesn't, add a new
> sign-off.
This makes a lot of sense.
-Brandon
^ permalink raw reply
* [PATCH] sha1_file.c: Fix a sparse warning
From: Ramsay Jones @ 2013-02-11 19:02 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, GIT Mailing-list
Sparse issues an "... was not declared. Should it be static?" warning
against the 'report_pack_garbage' symbol. In order to suppress the
warning, since this symbol requires more than file scope, we add an
extern declaration to the cache.h header file (and remove the now
redundant extern declaration in builtin/count-objects.c).
[As an alternative solution, make the variable a (static) file scope
variable and add an extern function set_report_pack_garbage(), which
would take a function pointer, to set the local variable ... etc.]
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi Nguyen,
If you need to re-roll your 'nd/count-garbage' patches, could you
please squash this (or something like it) into commit c2071d7a
("count-objects: report garbage files in pack directory too",
08-02-2013).
Thanks!
[BTW, I have an unsettling feeling that the above salutation should
read "Hi Duy," and I'm being (unintentionally) offensive. If so,
please let me know and accept my apologies. ;-) ]
ATB,
Ramsay Jones
builtin/count-objects.c | 1 -
cache.h | 3 +++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 118b2ae..605c215 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -11,7 +11,6 @@
static unsigned long garbage;
-extern void (*report_pack_garbage)(const char *path, int len, const char *name);
static void real_report_pack_garbage(const char *path, int len, const char *name)
{
if (len && name)
diff --git a/cache.h b/cache.h
index 6818d87..0d687b4 100644
--- a/cache.h
+++ b/cache.h
@@ -779,6 +779,9 @@ extern int has_pack_index(const unsigned char *sha1);
extern void assert_sha1_type(const unsigned char *sha1, enum object_type expect);
+/* A hook for count-objects to report invalid files in pack directory */
+extern void (*report_pack_garbage)(const char *path, int len, const char *name);
+
extern const signed char hexval_table[256];
static inline unsigned int hexval(unsigned char c)
{
--
1.8.1
^ permalink raw reply related
* Re: [PATCH] fixup! graph: output padding for merge subsequent parents
From: John Keeping @ 2013-02-11 19:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Matthieu Moy, Michał Kiedrowicz
In-Reply-To: <7vehgmol8y.fsf@alter.siamese.dyndns.org>
On Mon, Feb 11, 2013 at 08:42:21AM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > Perhaps it's best to leave the patch as it originally was to guarantee
> > that we can't get stuck in graph_show_commit(), even when it's called at
> > an unexpected time, but I see you've already squashed this change in.
> >
> > Would you prefer me to resend the original patch or send an update with
> > this change and the above reasoning in the commit message?
>
> Yes, please. Let's have the original (I think I have it in my
> reflog so no need to resend it) and this update on top as a separate
> patch with an updated log message.
I was suggesting dropping the change to remove the
graph_is_commit_finished() check in the loop. I'm not sure it buys us
much and there are still situations that could result in the state
changing to PADDING during the loop if the graph API is used in an
unexpected way.
Are others convinced that this change is always safe?
John
^ permalink raw reply
* Re: [PATCH v3] branch: show rebase/bisect info when possible instead of "(no branch)"
From: Junio C Hamano @ 2013-02-11 19:13 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Matthieu Moy, Jonathan Niedier
In-Reply-To: <1360318171-17614-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> +static char *get_head_description()
> +{
> + struct stat st;
> + struct strbuf sb = STRBUF_INIT;
> + struct strbuf result = STRBUF_INIT;
> + int bisect = 0;
> + int ret;
> + if (!stat(git_path("rebase-merge"), &st) && S_ISDIR(st.st_mode))
> + ret = strbuf_read_file(&sb, git_path("rebase-merge/head-name"), 0);
Hrmph. Why isn't this checking if the file exists and then read it,
i.e.
if (access(git_path("rebase-merge/head-name"), F_OK))
ret = strbuf_read_file(&sb, git_path("rebase-merge/head-name"), 0);
It is not like you are creating this file and making sure leading
directories exist, so the sequence looks a bit strange.
> + else if (!access(git_path("rebase-apply/rebasing"), F_OK))
> + ret = strbuf_read_file(&sb, git_path("rebase-apply/head-name"), 0);
> + else if (!access(git_path("BISECT_LOG"), F_OK)) {
> + ret = strbuf_read_file(&sb, git_path("BISECT_START"), 0);
> + bisect = 1;
And if the answer to the above question is "because if rebase-merge/
exists, with or without head-name, we know we are not bisecting",
then that may suggest that the structure of if/elseif cascade is
misdesigned. Shouldn't the "bisect" boolean be an enum "what are we
doing" that is initialized to "I do not know" and each of these
if/elseif cascade set the state to it when they know what we are
doing, in order for this function to be longer-term maintainable?
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Jeff King @ 2013-02-11 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <7va9rexqii.fsf@alter.siamese.dyndns.org>
On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:
> > Yeah, that actually is a good point. We should be using logmsg_reencode
> > so that we look for strings in the user's encoding.
>
> Perhaps like this. Just like the previous one (which should be
> discarded), this makes the function always use the temporary strbuf,
> so doing this upfront actually loses more code than it adds ;-)
I didn't see this in What's Cooking or pu. We should probably pick an
approach and go with it.
I think using logmsg_reencode makes sense. I'd be in favor of avoiding
the extra copy in the common case, so something like the patch below. If
you feel strongly about the code cleanup at the minor run-time expense,
I won't argue too much, though.
---
diff --git a/revision.c b/revision.c
index d7562ee..a08d0a5 100644
--- a/revision.c
+++ b/revision.c
@@ -2268,7 +2268,10 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
static int commit_match(struct commit *commit, struct rev_info *opt)
{
int retval;
+ const char *encoding;
+ const char *message;
struct strbuf buf = STRBUF_INIT;
+
if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
return 1;
@@ -2279,13 +2282,23 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
strbuf_addch(&buf, '\n');
}
+ /*
+ * We grep in the user's output encoding, under the assumption that it
+ * is the encoding they are most likely to write their grep pattern
+ * for. In addition, it means we will match the "notes" encoding below,
+ * so we will not end up with a buffer that has two different encodings
+ * in it.
+ */
+ encoding = get_log_output_encoding();
+ message = logmsg_reencode(commit, encoding);
+
/* Copy the commit to temporary if we are using "fake" headers */
if (buf.len)
- strbuf_addstr(&buf, commit->buffer);
+ strbuf_addstr(&buf, message);
if (opt->grep_filter.header_list && opt->mailmap) {
if (!buf.len)
- strbuf_addstr(&buf, commit->buffer);
+ strbuf_addstr(&buf, message);
commit_rewrite_person(&buf, "\nauthor ", opt->mailmap);
commit_rewrite_person(&buf, "\ncommitter ", opt->mailmap);
@@ -2294,18 +2307,18 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
/* Append "fake" message parts as needed */
if (opt->show_notes) {
if (!buf.len)
- strbuf_addstr(&buf, commit->buffer);
- format_display_notes(commit->object.sha1, &buf,
- get_log_output_encoding(), 1);
+ strbuf_addstr(&buf, message);
+ format_display_notes(commit->object.sha1, &buf, encoding, 1);
}
- /* Find either in the commit object, or in the temporary */
+ /* Find either in the original commit message, or in the temporary */
if (buf.len)
retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
else
retval = grep_buffer(&opt->grep_filter,
- commit->buffer, strlen(commit->buffer));
+ message, strlen(message));
strbuf_release(&buf);
+ logmsg_free(message, commit);
return retval;
}
^ permalink raw reply related
* [PATCH] rebase -i: respect core.commentchar
From: John Keeping @ 2013-02-11 19:21 UTC (permalink / raw)
To: git; +Cc: Ralf Thielow, Junio C Hamano
Commit eff80a9 (Allow custom "comment char") introduced a custom comment
character for commit messages but did not teach git-rebase--interactive
to use it.
Change git-rebase--interactive to read core.commentchar and use its
value when generating commit messages and for the todo list.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git-rebase--interactive.sh | 85 ++++++++++++++++++++++---------------------
t/t3404-rebase-interactive.sh | 16 ++++++++
2 files changed, 60 insertions(+), 41 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 8ed7fcc..8a37bc1 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -80,6 +80,9 @@ rewritten_pending="$state_dir"/rewritten-pending
GIT_CHERRY_PICK_HELP="$resolvemsg"
export GIT_CHERRY_PICK_HELP
+comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
+: ${comment_char:=#}
+
warn () {
printf '%s\n' "$*" >&2
}
@@ -105,8 +108,8 @@ mark_action_done () {
sed -e 1q < "$todo" >> "$done"
sed -e 1d < "$todo" >> "$todo".new
mv -f "$todo".new "$todo"
- new_count=$(sane_grep -c '^[^#]' < "$done")
- total=$(($new_count+$(sane_grep -c '^[^#]' < "$todo")))
+ new_count=$(sane_grep -c "^[^${comment_char}]" < "$done")
+ total=$(($new_count+$(sane_grep -c "^[^${comment_char}]" < "$todo")))
if test "$last_count" != "$new_count"
then
last_count=$new_count
@@ -116,19 +119,19 @@ mark_action_done () {
}
append_todo_help () {
- cat >> "$todo" << EOF
-#
-# Commands:
-# p, pick = use commit
-# r, reword = use commit, but edit the commit message
-# e, edit = use commit, but stop for amending
-# s, squash = use commit, but meld into previous commit
-# f, fixup = like "squash", but discard this commit's log message
-# x, exec = run command (the rest of the line) using shell
-#
-# These lines can be re-ordered; they are executed from top to bottom.
-#
-# If you remove a line here THAT COMMIT WILL BE LOST.
+ sed -e "s/^/$comment_char /" >>"$todo" <<EOF
+
+Commands:
+ p, pick = use commit
+ r, reword = use commit, but edit the commit message
+ e, edit = use commit, but stop for amending
+ s, squash = use commit, but meld into previous commit
+ f, fixup = like "squash", but discard this commit's log message
+ x, exec = run command (the rest of the line) using shell
+
+These lines can be re-ordered; they are executed from top to bottom.
+
+If you remove a line here THAT COMMIT WILL BE LOST.
EOF
}
@@ -179,7 +182,7 @@ die_abort () {
}
has_action () {
- sane_grep '^[^#]' "$1" >/dev/null
+ sane_grep "^[^${comment_char}]" "$1" >/dev/null
}
is_empty_commit() {
@@ -363,10 +366,10 @@ update_squash_messages () {
if test -f "$squash_msg"; then
mv "$squash_msg" "$squash_msg".bak || exit
count=$(($(sed -n \
- -e "1s/^# This is a combination of \(.*\) commits\./\1/p" \
+ -e "1s/^. This is a combination of \(.*\) commits\./\1/p" \
-e "q" < "$squash_msg".bak)+1))
{
- echo "# This is a combination of $count commits."
+ echo "$comment_char This is a combination of $count commits."
sed -e 1d -e '2,/^./{
/^$/d
}' <"$squash_msg".bak
@@ -375,8 +378,8 @@ update_squash_messages () {
commit_message HEAD > "$fixup_msg" || die "Cannot write $fixup_msg"
count=2
{
- echo "# This is a combination of 2 commits."
- echo "# The first commit's message is:"
+ echo "$comment_char This is a combination of 2 commits."
+ echo "$comment_char The first commit's message is:"
echo
cat "$fixup_msg"
} >"$squash_msg"
@@ -385,21 +388,21 @@ update_squash_messages () {
squash)
rm -f "$fixup_msg"
echo
- echo "# This is the $(nth_string $count) commit message:"
+ echo "$comment_char This is the $(nth_string $count) commit message:"
echo
commit_message $2
;;
fixup)
echo
- echo "# The $(nth_string $count) commit message will be skipped:"
+ echo "$comment_char The $(nth_string $count) commit message will be skipped:"
echo
- commit_message $2 | sed -e 's/^/# /'
+ commit_message $2 | sed -e "s/^/$comment_char /"
;;
esac >>"$squash_msg"
}
peek_next_command () {
- sed -n -e "/^#/d" -e '/^$/d' -e "s/ .*//p" -e "q" < "$todo"
+ sed -n -e "/^$comment_char/d" -e '/^$/d' -e "s/ .*//p" -e "q" < "$todo"
}
# A squash/fixup has failed. Prepare the long version of the squash
@@ -464,7 +467,7 @@ do_next () {
rm -f "$msg" "$author_script" "$amend" || exit
read -r command sha1 rest < "$todo"
case "$command" in
- '#'*|''|noop)
+ $comment_char*|''|noop)
mark_action_done
;;
pick|p)
@@ -803,15 +806,15 @@ skip)
do_rest
;;
edit-todo)
- sed -e '/^#/d' < "$todo" > "$todo".new
+ sed -e "/^$comment_char/d" < "$todo" > "$todo".new
mv -f "$todo".new "$todo"
append_todo_help
- cat >> "$todo" << EOF
-#
-# You are editing the todo file of an ongoing interactive rebase.
-# To continue rebase after editing, run:
-# git rebase --continue
-#
+ sed -e "s/^/$comment_char /" >>"$todo" <<EOF
+
+You are editing the todo file of an ongoing interactive rebase.
+To continue rebase after editing, run:
+ git rebase --continue
+
EOF
git_sequence_editor "$todo" ||
@@ -881,7 +884,7 @@ do
if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
then
- comment_out="# "
+ comment_out="$comment_char "
else
comment_out=
fi
@@ -942,20 +945,20 @@ test -s "$todo" || echo noop >> "$todo"
test -n "$autosquash" && rearrange_squash "$todo"
test -n "$cmd" && add_exec_commands "$todo"
-cat >> "$todo" << EOF
-
-# Rebase $shortrevisions onto $shortonto
+echo >>"$todo"
+sed -e "s/^/$comment_char /" >> "$todo" << EOF
+Rebase $shortrevisions onto $shortonto
EOF
append_todo_help
-cat >> "$todo" << EOF
-#
-# However, if you remove everything, the rebase will be aborted.
-#
+sed -e "s/^/$comment_char /" >> "$todo" << EOF
+
+However, if you remove everything, the rebase will be aborted.
+
EOF
if test -z "$keep_empty"
then
- echo "# Note that empty commits are commented out" >>"$todo"
+ echo "$comment_char Note that empty commits are commented out" >>"$todo"
fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8462be1..1043cdc 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -934,4 +934,20 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' '
test L = $(git cat-file commit HEAD | sed -ne \$p)
'
+test_expect_success 'rebase -i respects core.commentchar' '
+ git reset --hard &&
+ git checkout E^0 &&
+ git config core.commentchar \; &&
+ test_when_finished "git config --unset core.commentchar" &&
+ cat >comment-lines.sh <<EOF &&
+#!$SHELL_PATH
+sed -e "2,\$ s/^/;/" "\$1" >"\$1".tmp
+mv "\$1".tmp "\$1"
+EOF
+ chmod a+x comment-lines.sh &&
+ test_set_editor "$(pwd)/comment-lines.sh" &&
+ git rebase -i B &&
+ test B = $(git cat-file commit HEAD^ | sed -ne \$p)
+'
+
test_done
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH] fixup! graph: output padding for merge subsequent parents
From: Junio C Hamano @ 2013-02-11 19:58 UTC (permalink / raw)
To: John Keeping; +Cc: git, Matthieu Moy, Michał Kiedrowicz
In-Reply-To: <20130211190629.GC2270@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> On Mon, Feb 11, 2013 at 08:42:21AM -0800, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>>
>> > Perhaps it's best to leave the patch as it originally was to guarantee
>> > that we can't get stuck in graph_show_commit(), even when it's called at
>> > an unexpected time, but I see you've already squashed this change in.
>> >
>> > Would you prefer me to resend the original patch or send an update with
>> > this change and the above reasoning in the commit message?
>>
>> Yes, please. Let's have the original (I think I have it in my
>> reflog so no need to resend it) and this update on top as a separate
>> patch with an updated log message.
>
> I was suggesting dropping the change to remove the
> graph_is_commit_finished() check in the loop. I'm not sure it buys us
> much and there are still situations that could result in the state
> changing to PADDING during the loop if the graph API is used in an
> unexpected way.
OK, so the fixup! was not done with enough thought. I am fine with
dropping it.
Thanks.
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Junio C Hamano @ 2013-02-11 20:01 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Haller, Git List
In-Reply-To: <20130211191607.GA21269@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:
>
>> > Yeah, that actually is a good point. We should be using logmsg_reencode
>> > so that we look for strings in the user's encoding.
>>
>> Perhaps like this. Just like the previous one (which should be
>> discarded), this makes the function always use the temporary strbuf,
>> so doing this upfront actually loses more code than it adds ;-)
>
> I didn't see this in What's Cooking or pu. We should probably pick an
> approach and go with it.
>
> I think using logmsg_reencode makes sense. I'd be in favor of avoiding
> the extra copy in the common case, so something like the patch below. If
> you feel strongly about the code cleanup at the minor run-time expense,
> I won't argue too much, though.
Sounds good to me. Care to do the log message while at it?
> ---
> diff --git a/revision.c b/revision.c
> index d7562ee..a08d0a5 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2268,7 +2268,10 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
> static int commit_match(struct commit *commit, struct rev_info *opt)
> {
> int retval;
> + const char *encoding;
> + const char *message;
> struct strbuf buf = STRBUF_INIT;
> +
> if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
> return 1;
>
> @@ -2279,13 +2282,23 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
> strbuf_addch(&buf, '\n');
> }
>
> + /*
> + * We grep in the user's output encoding, under the assumption that it
> + * is the encoding they are most likely to write their grep pattern
> + * for. In addition, it means we will match the "notes" encoding below,
> + * so we will not end up with a buffer that has two different encodings
> + * in it.
> + */
> + encoding = get_log_output_encoding();
> + message = logmsg_reencode(commit, encoding);
> +
> /* Copy the commit to temporary if we are using "fake" headers */
> if (buf.len)
> - strbuf_addstr(&buf, commit->buffer);
> + strbuf_addstr(&buf, message);
>
> if (opt->grep_filter.header_list && opt->mailmap) {
> if (!buf.len)
> - strbuf_addstr(&buf, commit->buffer);
> + strbuf_addstr(&buf, message);
>
> commit_rewrite_person(&buf, "\nauthor ", opt->mailmap);
> commit_rewrite_person(&buf, "\ncommitter ", opt->mailmap);
> @@ -2294,18 +2307,18 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
> /* Append "fake" message parts as needed */
> if (opt->show_notes) {
> if (!buf.len)
> - strbuf_addstr(&buf, commit->buffer);
> - format_display_notes(commit->object.sha1, &buf,
> - get_log_output_encoding(), 1);
> + strbuf_addstr(&buf, message);
> + format_display_notes(commit->object.sha1, &buf, encoding, 1);
> }
>
> - /* Find either in the commit object, or in the temporary */
> + /* Find either in the original commit message, or in the temporary */
> if (buf.len)
> retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
> else
> retval = grep_buffer(&opt->grep_filter,
> - commit->buffer, strlen(commit->buffer));
> + message, strlen(message));
> strbuf_release(&buf);
> + logmsg_free(message, commit);
> return retval;
> }
>
^ permalink raw reply
* Re: [PATCH] rebase -i: respect core.commentchar
From: Junio C Hamano @ 2013-02-11 20:17 UTC (permalink / raw)
To: John Keeping; +Cc: git, Ralf Thielow
In-Reply-To: <aa1deab1de2e0f998b9ac0bc8c2d76557429a46b.1360610368.git.john@keeping.me.uk>
John Keeping <john@keeping.me.uk> writes:
> Commit eff80a9 (Allow custom "comment char") introduced a custom comment
> character for commit messages but did not teach git-rebase--interactive
> to use it.
>
> Change git-rebase--interactive to read core.commentchar and use its
> value when generating commit messages and for the todo list.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
It always is better late than never, but I would have appreciated
this was caught while the topic was still in 'next'. That is the
whole point of cooking in-flight topics in 'next'.
Yikes.... and thanks.
> git-rebase--interactive.sh | 85 ++++++++++++++++++++++---------------------
> t/t3404-rebase-interactive.sh | 16 ++++++++
> 2 files changed, 60 insertions(+), 41 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 8ed7fcc..8a37bc1 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -80,6 +80,9 @@ rewritten_pending="$state_dir"/rewritten-pending
> GIT_CHERRY_PICK_HELP="$resolvemsg"
> export GIT_CHERRY_PICK_HELP
>
> +comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> +: ${comment_char:=#}
Hmm, somewhat ugly. I wonder if we can do this without pipe and cut.
> @@ -105,8 +108,8 @@ mark_action_done () {
> sed -e 1q < "$todo" >> "$done"
> sed -e 1d < "$todo" >> "$todo".new
> mv -f "$todo".new "$todo"
> - new_count=$(sane_grep -c '^[^#]' < "$done")
> - total=$(($new_count+$(sane_grep -c '^[^#]' < "$todo")))
> + new_count=$(sane_grep -c "^[^${comment_char}]" < "$done")
> + total=$(($new_count+$(sane_grep -c "^[^${comment_char}]" < "$todo")))
I do not think we would want to worry about comment_char being a
funny character that can possibly interfere with regexp. Can't we
do this with "git stripspace" piped to "wc -l" or something?
> @@ -116,19 +119,19 @@ mark_action_done () {
> }
>
> append_todo_help () {
> - cat >> "$todo" << EOF
> -#
> -# Commands:
> -# p, pick = use commit
> -# r, reword = use commit, but edit the commit message
> -# e, edit = use commit, but stop for amending
> -# s, squash = use commit, but meld into previous commit
> -# f, fixup = like "squash", but discard this commit's log message
> -# x, exec = run command (the rest of the line) using shell
> -#
> -# These lines can be re-ordered; they are executed from top to bottom.
> -#
> -# If you remove a line here THAT COMMIT WILL BE LOST.
> + sed -e "s/^/$comment_char /" >>"$todo" <<EOF
When $comment_char is slash or backslash this will break.
Perhaps "stripspace --comment-lines" can be used here.
> +
> +Commands:
> + p, pick = use commit
> + r, reword = use commit, but edit the commit message
> + e, edit = use commit, but stop for amending
> + s, squash = use commit, but meld into previous commit
> + f, fixup = like "squash", but discard this commit's log message
> + x, exec = run command (the rest of the line) using shell
> +
> +These lines can be re-ordered; they are executed from top to bottom.
> +
> +If you remove a line here THAT COMMIT WILL BE LOST.
> EOF
> }
>
> @@ -179,7 +182,7 @@ die_abort () {
> }
>
> has_action () {
> - sane_grep '^[^#]' "$1" >/dev/null
> + sane_grep "^[^${comment_char}]" "$1" >/dev/null
Likewise.
> @@ -363,10 +366,10 @@ update_squash_messages () {
> if test -f "$squash_msg"; then
> mv "$squash_msg" "$squash_msg".bak || exit
> count=$(($(sed -n \
> - -e "1s/^# This is a combination of \(.*\) commits\./\1/p" \
> + -e "1s/^. This is a combination of \(.*\) commits\./\1/p" \
This one is safe.
> -e "q" < "$squash_msg".bak)+1))
> {
> - echo "# This is a combination of $count commits."
> + echo "$comment_char This is a combination of $count commits."
But you need to do "printf" to be safe here, I think, for comment_char='\'.
> @@ -375,8 +378,8 @@ update_squash_messages () {
> commit_message HEAD > "$fixup_msg" || die "Cannot write $fixup_msg"
> count=2
> {
> - echo "# This is a combination of 2 commits."
> - echo "# The first commit's message is:"
> + echo "$comment_char This is a combination of 2 commits."
> + echo "$comment_char The first commit's message is:"
Likewise.
> @@ -385,21 +388,21 @@ update_squash_messages () {
> squash)
> rm -f "$fixup_msg"
> echo
> - echo "# This is the $(nth_string $count) commit message:"
> + echo "$comment_char This is the $(nth_string $count) commit message:"
Likewise.
> echo
> commit_message $2
> ;;
> fixup)
> echo
> - echo "# The $(nth_string $count) commit message will be skipped:"
> + echo "$comment_char The $(nth_string $count) commit message will be skipped:"
> echo
> - commit_message $2 | sed -e 's/^/# /'
> + commit_message $2 | sed -e "s/^/$comment_char /"
Likewise.
> peek_next_command () {
> - sed -n -e "/^#/d" -e '/^$/d' -e "s/ .*//p" -e "q" < "$todo"
> + sed -n -e "/^$comment_char/d" -e '/^$/d' -e "s/ .*//p" -e "q" < "$todo"
> }
Likewise.
> @@ -464,7 +467,7 @@ do_next () {
> rm -f "$msg" "$author_script" "$amend" || exit
> read -r command sha1 rest < "$todo"
> case "$command" in
> - '#'*|''|noop)
> + $comment_char*|''|noop)
This is OK.
> @@ -803,15 +806,15 @@ skip)
> do_rest
> ;;
> edit-todo)
> - sed -e '/^#/d' < "$todo" > "$todo".new
> + sed -e "/^$comment_char/d" < "$todo" > "$todo".new
Unsafe.
> mv -f "$todo".new "$todo"
> append_todo_help
> - cat >> "$todo" << EOF
> -#
> -# You are editing the todo file of an ongoing interactive rebase.
> -# To continue rebase after editing, run:
> -# git rebase --continue
> -#
> + sed -e "s/^/$comment_char /" >>"$todo" <<EOF
Unsafe.
> +
> +You are editing the todo file of an ongoing interactive rebase.
> +To continue rebase after editing, run:
> + git rebase --continue
> +
> EOF
>
> git_sequence_editor "$todo" ||
> @@ -881,7 +884,7 @@ do
>
> if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
> then
> - comment_out="# "
> + comment_out="$comment_char "
OK.
> else
> comment_out=
> fi
> @@ -942,20 +945,20 @@ test -s "$todo" || echo noop >> "$todo"
> test -n "$autosquash" && rearrange_squash "$todo"
> test -n "$cmd" && add_exec_commands "$todo"
>
> -cat >> "$todo" << EOF
> -
> -# Rebase $shortrevisions onto $shortonto
> +echo >>"$todo"
> +sed -e "s/^/$comment_char /" >> "$todo" << EOF
Unsafe.
> +Rebase $shortrevisions onto $shortonto
> EOF
> append_todo_help
> -cat >> "$todo" << EOF
> -#
> -# However, if you remove everything, the rebase will be aborted.
> -#
> +sed -e "s/^/$comment_char /" >> "$todo" << EOF
Unsafe.
> +
> +However, if you remove everything, the rebase will be aborted.
> +
> EOF
>
> if test -z "$keep_empty"
> then
> - echo "# Note that empty commits are commented out" >>"$todo"
> + echo "$comment_char Note that empty commits are commented out" >>"$todo"
> fi
>
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8462be1..1043cdc 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -934,4 +934,20 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' '
> test L = $(git cat-file commit HEAD | sed -ne \$p)
> '
>
> +test_expect_success 'rebase -i respects core.commentchar' '
> + git reset --hard &&
> + git checkout E^0 &&
> + git config core.commentchar \; &&
Try setting it to '\' or '/' or '-'; they may catch some more breakages.
> + test_when_finished "git config --unset core.commentchar" &&
> + cat >comment-lines.sh <<EOF &&
> +#!$SHELL_PATH
> +sed -e "2,\$ s/^/;/" "\$1" >"\$1".tmp
> +mv "\$1".tmp "\$1"
> +EOF
> + chmod a+x comment-lines.sh &&
> + test_set_editor "$(pwd)/comment-lines.sh" &&
> + git rebase -i B &&
> + test B = $(git cat-file commit HEAD^ | sed -ne \$p)
> +'
> +
> test_done
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Junio C Hamano @ 2013-02-11 20:36 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Haller, Git List
In-Reply-To: <7v621ymxfv.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:
>>
>>> > Yeah, that actually is a good point. We should be using logmsg_reencode
>>> > so that we look for strings in the user's encoding.
>>>
>>> Perhaps like this. Just like the previous one (which should be
>>> discarded), this makes the function always use the temporary strbuf,
>>> so doing this upfront actually loses more code than it adds ;-)
>>
>> I didn't see this in What's Cooking or pu. We should probably pick an
>> approach and go with it.
>>
>> I think using logmsg_reencode makes sense. I'd be in favor of avoiding
>> the extra copy in the common case, so something like the patch below. If
>> you feel strongly about the code cleanup at the minor run-time expense,
>> I won't argue too much, though.
>
> Sounds good to me. Care to do the log message while at it?
Heh, how about this? I still need a sign-off from you.
Thanks.
commit 6a76814cd08cac15c1faff5bd97c9e94ac8b6a01
Author: Jeff King <peff@peff.net>
Date: Mon Feb 11 14:16:07 2013 -0500
log --grep: look for the given string in log output encoding
We used to grep in the raw commit buffer contents, possibly pieces
of notes encoded in log output encoding appended to it, which was
insane.
Convert the contents of the commit message also to log output
encoding before looking for the string. This incidentally fixes a
possible NULL dereference that can happen when commit->buffer has
already been freed, which can happen with
git commit -m 'text1' --allow-empty
git commit -m 'text2' --allow-empty
git log --graph --no-walk --grep 'text2'
which arguably does not make any sense (--graph inherently wants a
connected history, and by --no-walk the command line is telling us
to show discrete points in history without connectivity), and we
probably should forbid the combination, but that is a separate
issue.
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Jeff King @ 2013-02-11 20:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <7vvc9ylh97.fsf@alter.siamese.dyndns.org>
On Mon, Feb 11, 2013 at 12:36:52PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Jeff King <peff@peff.net> writes:
> >
> >> On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:
> >>
> >>> > Yeah, that actually is a good point. We should be using logmsg_reencode
> >>> > so that we look for strings in the user's encoding.
> >>>
> >>> Perhaps like this. Just like the previous one (which should be
> >>> discarded), this makes the function always use the temporary strbuf,
> >>> so doing this upfront actually loses more code than it adds ;-)
> >>
> >> I didn't see this in What's Cooking or pu. We should probably pick an
> >> approach and go with it.
> >>
> >> I think using logmsg_reencode makes sense. I'd be in favor of avoiding
> >> the extra copy in the common case, so something like the patch below. If
> >> you feel strongly about the code cleanup at the minor run-time expense,
> >> I won't argue too much, though.
> >
> > Sounds good to me. Care to do the log message while at it?
>
> Heh, how about this? I still need a sign-off from you.
I'm working on the log message and tests right now. There's also a minor
code fixup needed to compile with -Wall.
> log --grep: look for the given string in log output encoding
>
> We used to grep in the raw commit buffer contents, possibly pieces
> of notes encoded in log output encoding appended to it, which was
> insane.
>
> Convert the contents of the commit message also to log output
> encoding before looking for the string. This incidentally fixes a
> possible NULL dereference that can happen when commit->buffer has
> already been freed, which can happen with
>
> git commit -m 'text1' --allow-empty
> git commit -m 'text2' --allow-empty
> git log --graph --no-walk --grep 'text2'
>
> which arguably does not make any sense (--graph inherently wants a
> connected history, and by --no-walk the command line is telling us
> to show discrete points in history without connectivity), and we
> probably should forbid the combination, but that is a separate
> issue.
I'll use bits of that. I had sort of punted on the "how to reproduce the
segfault" issue entirely because you had noted that it was not a sane
thing to do. Still, I think it makes sense to mention it with the caveat
you give here.
-Peff
^ permalink raw reply
* Re: segfault for git log --graph --no-walk --grep a
From: Junio C Hamano @ 2013-02-11 20:55 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Haller, Git List
In-Reply-To: <20130211204142.GA28248@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Feb 11, 2013 at 12:36:52PM -0800, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > Jeff King <peff@peff.net> writes:
>> >
>> >> On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:
>> >>
>> >>> > Yeah, that actually is a good point. We should be using logmsg_reencode
>> >>> > so that we look for strings in the user's encoding.
>> >>>
>> >>> Perhaps like this. Just like the previous one (which should be
>> >>> discarded), this makes the function always use the temporary strbuf,
>> >>> so doing this upfront actually loses more code than it adds ;-)
>> >>
>> >> I didn't see this in What's Cooking or pu. We should probably pick an
>> >> approach and go with it.
>> >>
>> >> I think using logmsg_reencode makes sense. I'd be in favor of avoiding
>> >> the extra copy in the common case, so something like the patch below. If
>> >> you feel strongly about the code cleanup at the minor run-time expense,
>> >> I won't argue too much, though.
>> >
>> > Sounds good to me. Care to do the log message while at it?
>>
>> Heh, how about this? I still need a sign-off from you.
>
> I'm working on the log message and tests right now. There's also a minor
> code fixup needed to compile with -Wall.
Thanks; I noticed the constness issue around the message variable as well.
>
>> log --grep: look for the given string in log output encoding
>>
>> We used to grep in the raw commit buffer contents, possibly pieces
>> of notes encoded in log output encoding appended to it, which was
>> insane.
>>
>> Convert the contents of the commit message also to log output
>> encoding before looking for the string. This incidentally fixes a
>> possible NULL dereference that can happen when commit->buffer has
>> already been freed, which can happen with
>>
>> git commit -m 'text1' --allow-empty
>> git commit -m 'text2' --allow-empty
>> git log --graph --no-walk --grep 'text2'
>>
>> which arguably does not make any sense (--graph inherently wants a
>> connected history, and by --no-walk the command line is telling us
>> to show discrete points in history without connectivity), and we
>> probably should forbid the combination, but that is a separate
>> issue.
>
> I'll use bits of that. I had sort of punted on the "how to reproduce the
> segfault" issue entirely because you had noted that it was not a sane
> thing to do. Still, I think it makes sense to mention it with the caveat
> you give here.
>
> -Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox