* Re: Query on git commit amend
From: Junio C Hamano @ 2011-12-08 17:52 UTC (permalink / raw)
To: Viresh Kumar
Cc: Björn Steinbrink, Vijay Lakshminarayanan,
git@vger.kernel.org, Shiraz HASHIM, Vipin KUMAR
In-Reply-To: <4EDEFD66.4020404@st.com>
Viresh Kumar <viresh.kumar@st.com> writes:
> GIT_EDITOR=cat git commit --amend
>
> over
>
> git commit --amend -C HEAD
>
> ?
>
> ...
> But for single commit probably second one looks easier. Isn't it?
I saw "--amend -C HEAD" mentioned in some newbie-guide webpages, and I
think you just copy/learned from one of them, so it is not entirely your
fault, but the combination of --amend and "-C HEAD" is an idiotic thing
that happens to work, if you think about what exactly you are telling to
the command.
The point of --amend is twofold. One is to let you tweak the contents of
what is committed, which is not the topic of this thread, and the other is
to allow the user to reuse the log message from the commit being amended,
instead of typing the message from scratch.
The "-c <commit>" and its cousin "-C <commit>" options are about telling
Git that the user does _not_ want the other usual logic to come up with
the initial commit template (e.g. when committing anew, it may read the
log template file, when committing a merge, it may read the MERGE_MSG
prepared by fmt-merge-msg, and most importantly in this context, when
amending, the one that is prepared by the --amend logic is used) kick in
at all, and instead wants to start from the log message of the named
commit.
So by saying "--amend -C HEAD" you are saying "I want to reuse the log
message of the commit I am amending,... eh, scratch that, I instead want
to use the log message of the HEAD commit", as if the commit you are
amending and HEAD are two different things. That is idiotic.
And you say that only for the side effect that capital "-C" stops the
editor.
Compared to that idiotic statement, "EDITOR=: git commit --amend" is a lot
saner way to say the same thing in a more direct and straightforward
way. "I want to reuse the log message of the commit I am amending, and the
editor to use it while running that commit is the command 'true', i.e. the
one that does not really touch any line in the text file and successfully
exits, because I am not going to change anything".
Of course, if "git commit --amend" honoured "--no-edit", that is even more
direct, straightforward and intuitive way to say so ;-)
^ permalink raw reply
* Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Jonathan Nieder @ 2011-12-08 17:59 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <CALkWK0m89D02aku8J0OXbpyrovHCOpsYS4Qpx2jH-pFG4rOG2A@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Having to type "--" or prefix with "./" to escape ordinary filenames
>> that do not start with "-" would be completely weird.
>
> Hm, do you have any suggestions to work around this? Can we use
> something like a parse-stopper after the the first subcommand is
> encountered, and treat the next argument as a non-subcommand (filename
> or whatever else)?
What's the desired behavior? Then we can talk about how to implement it.
If the goal is "use parse-options for a command that has subcommands",
see builtin/notes.c.
^ permalink raw reply
* Disabling Delta Compression on a fetch
From: Todd Rinaldo @ 2011-12-08 17:34 UTC (permalink / raw)
To: git
We run an internal gitorious server with many repos on it. Due to legacy issues, some of the repos are rather bloated with large binaries. When several changes happen and are pushed to the gitorious repo and then are later pulled back (fetch and pull) to other clones, we get a broken connection while the gitorious side is generating Delta compression.
All of the git communication happens on 1 subnet all connected by a single gigabit switch. As I see it, the Delta Compression is actually a performance degradation in our environment.
The solution I've come up with is to set pack.window=0 in /etc/gitconfig on the gitorious server.
I realize our need is uncommon. I realize by doing this, I am trading increased bandwidth in exchange for starting the send sooner. When I read the documentation for pack.window, it seems more a side effect than the original intent that this variable disables Delta Compression when set to 0.
My question is: Are there are any unintended consequences of this approach anyone can think of?
Thanks,
Todd Rinaldo
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)
From: Junio C Hamano @ 2011-12-08 18:19 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: git, Jonathan Nieder
In-Reply-To: <CALkWK0nNtvrLHxQv17jfrFQ=BcwLfgh7eE9X-nDCCYY0nsOskg@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> * On "revert: make commit subjects in insn sheet optional"
>>
>> After finding the verb and advancing the pointer "bol" at the beginning of
>> the object name, end_of_object_name variable points at the first SP or LF
>> using strcspn(bol, " \n"), but I wonder why we are not grabbing a run of
>> hexdigits instead, i.e. strspn(bol, "0123456789abcdef"). Is this so that
>> we can allow something like "pick rr/revert-cherry-pick~3"?
>
> Yes, it is exactly for that reason :)
That is not explained in the commit message, which in itself is a problem
that needs to be addressed, but ...
>> I also wonder if this should be (sorry for pcre) "(pick|revert)\s+(\S+)\s"
>> instead, i.e. allow users with fat fingers to use one or more SP or even HT
>> to separate the verb and the operand.
>
> Hm, I'm not too enthusiastic about this change, because we don't
> advertise hand-editing the instruction sheet yet:...
... is inconsistent with the above.
If you plan to later allow editing and if you know what kind of editing
you are going to allow, it would be better to prepare the parser to accept
input with a reasonable slack, especially because you are already touching
it in this series anyway.
On the other hand, if you do not want to think about (or do not want us to
get distracted by thinking about) allowing editing in this series, which
is also a sensible stance to take, the parser should be made to accept
only what the mechanism in the series would produce and error out
otherwise, leaving the entire "the user now can edit and here are the rules"
part for a separate series that comes after this series is perfected.
>> * On "revert: allow mixed pick and revert instructions"
>>
>> Reporting what we did not understand from parse_insn_line() is a good
>> idea, but I think the line number should be reported at the beginning
>> of the same line.
>
> Makes sense. Do you like this?
Not particularly.
> - return error(_("Unrecognized action: %.*s"), (int)len, bol);
> + return error(_("Line %d: Unrecognized action: %.*s"),
It may be just me, but personally I prefer to see which file of what line
had the problem, i.e. "%s:%d: Unrecognized action:%s".
But if there is not much point in reporting the filename because it always
is the same, "Unrecognized action (line %d): %s" would also be fine.
^ permalink raw reply
* Re: [bug?] checkout -m doesn't work without a base version
From: Junio C Hamano @ 2011-12-08 18:27 UTC (permalink / raw)
To: Pete Harlan; +Cc: git
In-Reply-To: <4EDF1631.5090906@pcharlan.com>
Pete Harlan <pgit@pcharlan.com> writes:
> to "ours" and "theirs". (The same thing happens in the 3-way merge
> case.)
That is entirely expected. "checkout -m" does not know or care _how_ the
index came to the conflicted state and reproduces the three-way conflicted
state in the file in the working tree solely from the contents of the
index which records only what the common thing looked like (stage #1),
what one side looked like (stage #2) and what the other side looked like
(stage #3) before the mergy operation began, so there is no way for it to
say "the other side came from foo/blah branch". There are even cases where
the conflict originally came not from any branch (think "am -3").
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Junio C Hamano @ 2011-12-08 18:41 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Ramkumar Ramachandra, Git List
In-Reply-To: <20111207081734.GG11737@elie.hsd1.il.comcast.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Ramkumar Ramachandra wrote:
>
>> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
>> ($gname/186365) implemented.
>
> Thanks for this. I am worried that some of my comments in previous
> reviews (especially about change descriptions) were not addressed,
> which could mean one of a few things:
>
> - you didn't notice them
> - they were wrong
> - you noticed them and they were describing real problems, but it
> wasn't worth the time to fix them
>
> Fine. But I would like to know which case they fell into, so I can
> learn in order to do a better job reviewing the future and know my
> time is well spent.
Another thing I often notice after raising an issue during a review (this
is not limited to Ram) is that I do so in the form of a question "why is
this function done this way?" expecting either "ah, doing it that way
instead is simpler and easier to read" or "because of such and such, which
you may have missed. after all this is a tricky codepath with this and
that constraints" as a response. The former should and often does result
in an update of the code in the re-rolled series, but the latter often
results in a frustrating nothing in a re-roll, when the fact that a
reviewer needed to ask the question was a sure sign that the code needs
explanation either in the log message or in-code comment.
> The series makes various changes, all essentially good, which leaves
> me all the more motivated to figure out how to get this sort of thing
> in smoothly without making life difficult for people in the future
> tracking down regressions.
I looked the series over with your comments and tend to agree with many of
them. Perhaps I should wait for another round before picking it up again
(I've already dropped the old series I kept before 1.7.8).
^ permalink raw reply
* Re: [PATCH] git-send-email: Add auto-cc to all body signatures
From: Junio C Hamano @ 2011-12-08 19:37 UTC (permalink / raw)
To: Joe Perches; +Cc: git, jeffrey.t.kirsher
In-Reply-To: <1323313119.1762.58.camel@joe2Laptop>
Joe Perches <joe@perches.com> writes:
> Many types of signatures are used by various projects.
>
> The most common type is formatted:
> "[some_signature_type]-by: First Last <email@domain.tld>"
> e.g:
> "Reported-by: First Last <email@domain.tld>" (no quotes are used)
This is just a phrasing issue, but I am a bit reluctant about the name
"signature". "Acked-by:", "Tested-by:" and "Reviewed-by:" are originally
written in the message sent to the author by the person who is giving an
Ack, a successful test report, and a review comment, and the author (at
least in spirit) copies & pastes them to the final text used in the commit
log message, so it would not be incorrect to call them "signatures". But
other "Random-by:" would not fall into that pattern.
"Reported-by:" certainly does not. It is almost always added by the
author of the patch that is different from the reporter, and the reporter
wouldn't have written "Reported-by: me" in the original bug report that
triggered the discussion and resulted in the commit to fix the bug. Such a
line is certainly not a signature of/by the reporter. Same can be said for
"Helped-by:" for the author to share credits.
Also I've seen these "Random-by:", especially the ones that the author
adds on his own initiative like "Reported-by:", followed by just a name
but not an addresses [*1*].
Does your change do the right thing on such an address-less entry? The
answer to this question must start with the definition of "the right thing
to do is X", of course.
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 327233c..17ea825 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -246,8 +246,9 @@ Automating
> patch body (commit message) except for self (use 'self' for that).
> - 'sob' will avoid including anyone mentioned in Signed-off-by lines except
> for self (use 'self' for that).
> +- 'signatures' will avoid including anyone mentioned in any "<foo>-by:" lines.
> - 'cccmd' will avoid running the --cc-cmd.
> -- 'body' is equivalent to 'sob' + 'bodycc'
> +- 'body' is equivalent to 'sob' + 'bodycc + signatures'
The quotes do not match quite well.
[Footnote]
*1* This seems to be done deliberately so; I understand that this is to
avoid running afoul of EU privacy legislation or something.
^ permalink raw reply
* Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Ramkumar Ramachandra @ 2011-12-08 19:39 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <20111208175913.GK2394@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> What's the desired behavior? Then we can talk about how to implement it.
>
> If the goal is "use parse-options for a command that has subcommands",
> see builtin/notes.c.
Uses strcmp() to match argv[0]. And you can't specify the options for
a certain subcommand before the subcommand itself on the command-line,
although I don't consider this a serious limitation. I was going for
something prettier with subcommand-specific help text, albeit a
serious limitation. I'll try working towards this for a few more
hours to see if anything useful comes out of it -- otherwise, I'll
just drop this patch and focus on eliminating the ugliness in
builtin/revert.c around '--continue', '--quit' parsing.
That being said, do you see value in lifting the restriction on
opts->long_name and PARSE_OPTS_NODASH not allowed together? The
restriction seems quite arbitrary, but I can't justify lifting it
unless I can show some valid usecase.
Thanks.
-- Ram
^ permalink raw reply
* [PATCH] mingw: give waitpid the correct signature
From: Erik Faye-Lund @ 2011-12-08 19:39 UTC (permalink / raw)
To: git; +Cc: msysgit
POSIX says that last parameter to waitpid should be 'int',
so let's make it so.
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
Just a small clean-up I stumbled over...
compat/mingw.c | 2 +-
compat/mingw.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index efdc703..a0ac487 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1712,7 +1712,7 @@ char *getpass(const char *prompt)
return strbuf_detach(&buf, NULL);
}
-pid_t waitpid(pid_t pid, int *status, unsigned options)
+pid_t waitpid(pid_t pid, int *status, int options)
{
HANDLE h = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION,
FALSE, pid);
diff --git a/compat/mingw.h b/compat/mingw.h
index fecf0d0..0ff1e04 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -120,7 +120,7 @@ static inline int mingw_mkdir(const char *path, int mode)
#define mkdir mingw_mkdir
#define WNOHANG 1
-pid_t waitpid(pid_t pid, int *status, unsigned options);
+pid_t waitpid(pid_t pid, int *status, int options);
#define kill mingw_kill
int mingw_kill(pid_t pid, int sig);
--
1.7.7.1.msysgit.0.272.g9e47e
^ permalink raw reply related
* Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)
From: Erik Faye-Lund @ 2011-12-08 19:44 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git Mailing List, Johannes Sixt, Stephan Beyer
In-Reply-To: <20111206185218.GB9492@sigill.intra.peff.net>
On Tue, Dec 6, 2011 at 7:52 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Dec 06, 2011 at 12:22:06PM +0100, Erik Faye-Lund wrote:
>
>> >> * jk/upload-archive-use-start-command (2011-11-21) 1 commit
>> >> - upload-archive: use start_command instead of fork
>> >>
>> >> What's the status of this one?
>> >
>> > I think what you have in pu is good, but of course I didn't actually
>> > test it on Windows. Erik?
>> >
>>
>> I tried to check out ee27ca4 and build, and got hit by a wall of warnings:
>
> I think you are on the wrong topic. ee27ca4 is the maint topic for
> "don't let remote clients get unreachable commits", and is based on the
> ancient 1.6.2. Which is why you are getting all of those warnings.
>
You are indeed right. Thanks for spotting :)
> The topic in question is jk/upload-archive-use-start-command, which is
> at 1bc01ef, and should be based on recent-ish master.
t5000-tar-tree pass for that one as well. No warnings this time :P
^ permalink raw reply
* Re: [PATCH 0/2] Parsing a subcommand using parse-options
From: Junio C Hamano @ 2011-12-08 19:51 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <1323346028-9201-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> git stash show
> ^^ -- The subcommand "show" to the git-stash builtin
Contrasting this with "git tag --list", one uses subcommand verb while the
other uses operating mode option and one could say they are inconsistent.
But it does not bother me that much, and for a good reason other than
inertia.
When a command whose primary purpose is very clear (e.g. "git tag" and
"git branch" are primarily to create these things, just like "git commit"
is to create a commit), it is more natural to give the primary mode the
main interface so that you do not have to say "git tag create v1.0"; hence
operating mode option makes sense than subcommand.
On the other hand, when a command does not have a clear primary mode
(e.g. if you save a stash you must be able to use (i.e. apply or pop) the
saved one and both are equally important feature), but its primary purpose
is to dispatch various different operation, it makes more sense to name
them as subcommands. You _could_ make all of them into operating mode
options, but that only requires more typing, i.e. "git stash --save"/"git
stash --pop", without adding much value to the command. In addition, it
invites unnecessary confusion "what is the default mode of operation, and
is it really that important to be the default?", because not requiring an
explicit "subcommand" but merely allowing "operation mode option" implies
that you can say "git cmd" without anything, i.e. there must be some
default.
For "stash", "save" has been the default merely by historical accident,
but that has been rectified (it now requires you do not have any message
for the quick stash "git satsh<ENTER>" to work). There really isn't any
"default" operating mode to the command, and the command is a dispatcher
to its subcommands.
^ permalink raw reply
* Re: [PATCH v2 0/6] Fix '&&' chaining in tests
From: Junio C Hamano @ 2011-12-08 19:55 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Jonathan Nieder, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> This follows-up $gmane/186481.
I take that you meant "replaces". It was confusing especially because you
seem to have included a few unrelated patches in the thread.
^ permalink raw reply
* Re: Disabling Delta Compression on a fetch
From: Junio C Hamano @ 2011-12-08 20:10 UTC (permalink / raw)
To: Todd Rinaldo; +Cc: git
In-Reply-To: <070681D4-F87B-435E-8A3B-144E59DE722B@cpanel.net>
Todd Rinaldo <toddr@cpanel.net> writes:
> The solution I've come up with is to set pack.window=0 in /etc/gitconfig on the gitorious server.
Wouldn't setting core.bigfilethreshold be a more appropriate solution?
^ permalink raw reply
* Re: Disabling Delta Compression on a fetch
From: Jeff King @ 2011-12-08 20:14 UTC (permalink / raw)
To: Todd Rinaldo; +Cc: git
In-Reply-To: <070681D4-F87B-435E-8A3B-144E59DE722B@cpanel.net>
On Thu, Dec 08, 2011 at 11:34:26AM -0600, Todd Rinaldo wrote:
> All of the git communication happens on 1 subnet all connected by a
> single gigabit switch. As I see it, the Delta Compression is actually
> a performance degradation in our environment.
>
> The solution I've come up with is to set pack.window=0 in
> /etc/gitconfig on the gitorious server.
An alternative is to mark the binary files as "-delta" with
gitattributes on the server. Then you will get the benefits of delta
compression for other files without bothering to try the binary files.
Note that git won't read the gitattributes file out of the tree in a
bare repository, so these attributes should go either in
$REPO/info/attributes (if they are repo-specific), or in
/etc/gitattributes (if they are in many repos).
I.e., something like:
echo '*.bin -delta' >/etc/gitattributes
Also, before any of that, make sure that the upstream repos are fully
packed. Git will not try to delta two objects coming from the same pack
(since it will already have tried when they were entering the pack).
That by itself might be enough to solve your problem without any other
configuration.
> My question is: Are there are any unintended consequences of this
> approach anyone can think of?
Other than trading bandwidth, you are also trading space on the client
side, since each client will store the resulting pack.
-Peff
^ permalink raw reply
* Re: [PATCH] git-send-email: Add auto-cc to all body signatures
From: Joe Perches @ 2011-12-08 20:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jeffrey.t.kirsher
In-Reply-To: <7v8vmmj1ng.fsf@alter.siamese.dyndns.org>
On Thu, 2011-12-08 at 11:37 -0800, Junio C Hamano wrote:
> Joe Perches <joe@perches.com> writes:
> > Many types of signatures are used by various projects.
> > The most common type is formatted:
> > "[some_signature_type]-by: First Last <email@domain.tld>"
> > e.g:
> > "Reported-by: First Last <email@domain.tld>" (no quotes are used)
> This is just a phrasing issue, but I am a bit reluctant about the name
> "signature".
I've called all these markings signatures.
Maybe email-address-tags or another name could be used.
I'm not bothered one way or another by any chosen name.
> Does your change do the right thing on such an address-less entry? The
> answer to this question must start with the definition of "the right thing
> to do is X", of course.
All addresses go through "extract_valid_address".
Invalid addresses are not used.
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)
From: Junio C Hamano @ 2011-12-08 21:25 UTC (permalink / raw)
To: kusmabite; +Cc: Jeff King, Git Mailing List, Johannes Sixt, Stephan Beyer
In-Reply-To: <CABPQNSYQ=nt9LYzXpQgfwV00e9AxOV3LKj6VCCO8xkMAXb-Lfg@mail.gmail.com>
Erik Faye-Lund <kusmabite@gmail.com> writes:
>> The topic in question is jk/upload-archive-use-start-command, which is
>> at 1bc01ef, and should be based on recent-ish master.
>
> t5000-tar-tree pass for that one as well. No warnings this time :P
Thanks; let's move it forward, then.
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Junio C Hamano @ 2011-12-08 21:34 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111207064231.GA499@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Because the pattern takes 0 or more lines and no terminator, we can't
> distinguish between empty or truncated input and the empty pattern.
I agree that such a positive "Ok here is the end of specification" marker
is a good idea, even if we do not worry about "an empty set".
When the requestor wants to specify the credentials with host and user,
but the wire is cut after host is communicated but before user is, we do
want to notice the communication error, instead of silently erasing all
the credentials on the host regardless of the user.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Frans Klaver @ 2011-12-08 21:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vpqg1e3au.fsf@alter.siamese.dyndns.org>
On Tue, 06 Dec 2011 23:35:53 +0100, Junio C Hamano <gitster@pobox.com>
wrote:
> Frans Klaver <fransklaver@gmail.com> writes:
>
>> +#ifndef WIN32
>> +static int is_in_group(gid_t gid)
>> ...
>> +static int have_read_execute_permissions(const char *path)
>> +{
>> + struct stat s;
>> + trace_printf("checking '%s'\n", path);
>> +
>> + if (stat(path, &s) < 0) {
>> + ...
>> + /* check world permissions */
>> + if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
>> + return 1;
>
> Hmm, do you need to do this with stat(2)?
>
> Wouldn't access(2) with R_OK|X_OK give you exactly what you want without
> this much trouble?
I just had a good look through the man page of access(2), and I think it
depends. access works for the real uid, which is what I attempted to
implement in the above check as well. However, do we actually need to use
the real uid or do we need the set uid (geteuid(2))? Would it be safe to
assume we don't setuid?
> I also think that your permission check is incorrectly implemented.
>
> $ cd /var/tmp && date >j && chmod 044 j && ls -l j
> ----r--r-- 1 junio junio 29 Dec 6 14:32 j
> $ cat j
> cat: j: Permission denied
> $ su pogo
> Password:
> $ cat j
> Tue Dec 6 14:32:23 PST 2011
> That's a world-readable but unreadable-only-to-me file.
Will fix if we can't use access(2) due to what I mentioned above.
>> + warn("file '%s' exists and permissions "
>> + "seem OK.\nIf this is a script, see if you "
>> + "have sufficient privileges to run the "
>> + "interpreter", sb.buf);
>
> Does "warn()" do the right thing for multi-line strings like this?
Looking back on it, I think I actually wanted to use warning() from
usage.c. I'll still have to check if that does the multi-line thing as I
expect it to.
^ permalink raw reply
* Re: [PATCH 5/7] add generic terminal prompt function
From: Jakub Narebski @ 2011-12-08 21:48 UTC (permalink / raw)
To: Jeff King; +Cc: git, Erik Faye-Lund, Junio C Hamano
In-Reply-To: <20111208083323.GE26409@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> diff --git a/Makefile b/Makefile
> index b024e2f..2a9bb3d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -227,6 +227,9 @@ all::
> +# Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the
> +# user.
> @@ -833,6 +838,7 @@ ifeq ($(uname_S),Linux)
> NO_STRLCPY = YesPlease
> NO_MKSTEMPS = YesPlease
> HAVE_PATHS_H = YesPlease
> + HAVE_DEV_TTY = YesPlease
> endif
Here you use HAVE_DEV_TTY (by the way, I wonder if it could be
automatically detected by ./configure script)...
> diff --git a/compat/terminal.c b/compat/terminal.c
> new file mode 100644
> index 0000000..49f16ca
> --- /dev/null
> +++ b/compat/terminal.c
> @@ -0,0 +1,81 @@
> +#include "git-compat-util.h"
> +#include "compat/terminal.h"
> +#include "sigchain.h"
> +#include "strbuf.h"
> +
> +#ifndef NO_DEV_TTY
> +#endif
+#endif /* NO_DEV_TTY */
...and here you have NO_DEV_TTY
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 5/7] add generic terminal prompt function
From: Jeff King @ 2011-12-08 21:52 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Erik Faye-Lund, Junio C Hamano
In-Reply-To: <m38vmmivkc.fsf@localhost.localdomain>
On Thu, Dec 08, 2011 at 01:48:33PM -0800, Jakub Narebski wrote:
> > @@ -833,6 +838,7 @@ ifeq ($(uname_S),Linux)
> > NO_STRLCPY = YesPlease
> > NO_MKSTEMPS = YesPlease
> > HAVE_PATHS_H = YesPlease
> > + HAVE_DEV_TTY = YesPlease
> > endif
>
> Here you use HAVE_DEV_TTY (by the way, I wonder if it could be
> automatically detected by ./configure script)...
> [...]
> ...and here you have NO_DEV_TTY
Whoops. Thanks for catching. I converted it to NO_DEV_TTY, which would
turn this code on by default (because I think _most_ platforms we use
are going to want this), but then I decided to go the conservative route
and let platforms opt into it.
And of course since my platform is the one that enables it, I didn't
notice during my testing.
I'll fix it for the next re-roll.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Junio C Hamano @ 2011-12-08 23:48 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Jonathan Nieder, Matthieu Moy, Git List
In-Reply-To: <CALkWK0nCuFgS8PKkQLMzqpBsOWouSs5y=CEKS1r0x0=LkhqC9A@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> That being said, do you see value in lifting the restriction on
> opts->long_name and PARSE_OPTS_NODASH not allowed together? The
> restriction seems quite arbitrary, but I can't justify lifting it
> unless I can show some valid usecase.
True and true.
As to the first "true", it is because the nodash was introduced only to
parse "find ... \( ... \)" style parentheses as if they are part of option
sequence, and that use case only needed a single letter.
As to the second "true", it is because so far we didn't need anything
longer.
I do not think the name of a subcommand is not a good use case example for
it, by the way. Unlike parentheses on the command line of "find" that can
come anywhere and there can be more than one, the subcommand must be the
first thing on the command line and only one subcommand is given at one
command invocation.
^ permalink raw reply
* Re: [PATCH 5/6] t1510 (worktree): fix '&&' chaining
From: Junio C Hamano @ 2011-12-09 0:00 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Ramkumar Ramachandra, Matthieu Moy, Git List
In-Reply-To: <20111208171213.GF2394@elie.hsd1.il.comcast.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> This is also not "Additionally", as in "As a separate change that
> maybe should have been another patch but I am too lazy". Rather, it
> is a necessary change that is part of the same task. So I would
> write:
>
> Breaks in a test assertion's && chain can potentially hide failures
> from earlier commands in the chain. Fix these breaks.
>
> 'unset' returns non-zero status when the variable passed was already
> unset on some shells, so now that the status is tested we need to
> change these instances to 'safe_unset'.
>
> Erm, sane_unset, not safe_unset. Did you even test this?
Thanks for careful reading and many constructive review comments.
^ permalink raw reply
* Re: Debugging git-commit slowness on a large repo
From: Joshua Redstone @ 2011-12-09 0:09 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Carlos Martín Nieto, Tomas Carnecky, Junio C Hamano,
git@vger.kernel.org
In-Reply-To: <CACsJy8DiWWr7eo86gzb-XcqfDv4_ENkqWxswTNb-k84xO18c=A@mail.gmail.com>
On 12/7/11 5:39 PM, "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> wrote:
>On Thu, Dec 8, 2011 at 5:48 AM, Joshua Redstone <joshua.redstone@fb.com>
>wrote:
>> Hi Duy,
>> Thanks for the documentation link.
>>
>> git ls-files shows 100k files, which matches # of files in the working
>> tree ('find . -type f -print | wc -l').
>
>Any chance you can split it into smaller repositories, or remove files
>from working directory (e.g. if you store logs, you don't have to keep
>logs from all time in working directory, they can be retrieved from
>history).
It's not really feasible to split it into smaller repositories. In fact,
we're expecting it to grow between 3x and 5x in number of files and number
of commits.
>
>> I added a 'git read-tree HEAD' before the git-add, and a 'git
>>write-tree'
>> after the add. With that, the commit time slowed down to 8 seconds per
>> commit, plus 4 more seconds for the read-tree/add/write-tree ops. The
>> read-tree/add/write-tree each took about a second.
>
>read-tree destroys stat info in index, refreshing 100k entries in
>index in this case may take some time. Try this to see if commit time
>reduces and how much time update-index takes
>
>read-tree HEAD
>update-index --refresh
>add ....
>write-tree
>commit -q
I added the "update-index --refresh" and the time for commit became more
like 0.6 seconds.
In this setup: read-tree takes ~2 seconds, update-index takes ~8 seconds,
git-add takes 1 to 4 seconds, and write-tree takes less than 1 second.
>
>> As an experiment, I also tried removing the 'git read-tree' and just
>> having the git-write-tree. That sped up commits to 0.6 seconds, but the
>> overall time for add/write-tree/commit was still 3 to 6 seconds.
>
>overall time is not really important because we duplicate work here
>(write-tree is done as part of commit again). What I'm trying to do is
>to determine how much time each operation in commit may take.
>--
>Duy
^ permalink raw reply
* Re: Debugging git-commit slowness on a large repo
From: Joshua Redstone @ 2011-12-09 0:17 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Carlos Martín Nieto, Tomas Carnecky, Junio C Hamano,
git@vger.kernel.org
In-Reply-To: <CB069000.2C9C6%joshua.redstone@fb.com>
Btw, I also tried doing some very poor-man's profiling on "git commit"
without any of the readtree/writetree/updateindex commands.
Around 50% of the time was in (bottom few frames may have varied)
#1 0x00000000004c467e in find_pack_entry (sha1=0x1475a44 ,
e=0x7fff2621f070) at sha1_file.c:2027
#2 0x00000000004c57b0 in has_sha1_file (sha1=0x7fe2cd9c7900 "00") at
sha1_file.c:2567
#3 0x000000000046e4af in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:333
#4 0x000000000046e278 in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:285
#5 0x000000000046e278 in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:285
#6 0x000000000046e278 in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:285
#7 0x000000000046e278 in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:285
#8 0x000000000046e278 in update_one (it=<value optimized out>,
cache=<value optimized out>, entries=<value optimized out>, base=<value
optimized out>, baselen=<value optimized out>, missing_ok=<value optimized
out>, dryrun=0) at cache-\
tree.c:285
#9 0x000000000046e869 in cache_tree_update (it=<value optimized out>,
cache=<value optimized out>, entries=dwarf2_read_address: Corrupted DWARF
expression.
) at cache-tree.c:379
#10 0x000000000041cade in prepare_to_commit (index_file=0x781740
".git/index", prefix=<value optimized out>, current_head=<value optimized
out>, s=0x7fff26220d00, author_ident=<value optimized out>) at
builtin/commit.c:866
#11 0x000000000041d891 in cmd_commit (argc=0, argv=0x7fff262213a0,
prefix=0x0) at builtin/commit.c:1407
#12 0x0000000000404bf7 in handle_internal_command (argc=4,
argv=0x7fff262213a0) at git.c:308
#13 0x0000000000404e2f in main (argc=4, argv=0x7fff262213a0) at git.c:512
And 30% of the time was in:
#0 0x00000034af2c34a5 in _lxstat () from /lib64/libc.so.6
#1 0x00000000004abe0f in refresh_cache_ent (istate=0x780940,
ce=0x7f8462a34e40, options=0, err=0x7fff6dd9f588) at
/usr/include/sys/stat.h:443
#2 0x00000000004ac1a0 in refresh_index (istate=0x780940, flags=<value
optimized out>, pathspec=<value optimized out>, seen=<value optimized
out>, header_msg=0x0) at read-cache.c:1133
#3 0x000000000041b60a in refresh_cache_or_die (refresh_flags=<value
optimized out>) at builtin/commit.c:331
#4 0x000000000041bc39 in prepare_index (argc=0, argv=0x7fff6dda0310,
prefix=0x0, current_head=<value optimized out>, is_status=<value optimized
out>) at builtin/commit.c:414
#5 0x000000000041d878 in cmd_commit (argc=0, argv=0x7fff6dda0310,
prefix=0x0) at builtin/commit.c:1403
Josh
On 12/8/11 4:09 PM, "Joshua Redstone" <joshua.redstone@fb.com> wrote:
>On 12/7/11 5:39 PM, "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> wrote:
>
>>On Thu, Dec 8, 2011 at 5:48 AM, Joshua Redstone <joshua.redstone@fb.com>
>>wrote:
>>> Hi Duy,
>>> Thanks for the documentation link.
>>>
>>> git ls-files shows 100k files, which matches # of files in the working
>>> tree ('find . -type f -print | wc -l').
>>
>>Any chance you can split it into smaller repositories, or remove files
>>from working directory (e.g. if you store logs, you don't have to keep
>>logs from all time in working directory, they can be retrieved from
>>history).
>
>It's not really feasible to split it into smaller repositories. In fact,
>we're expecting it to grow between 3x and 5x in number of files and number
>of commits.
>
>>
>>> I added a 'git read-tree HEAD' before the git-add, and a 'git
>>>write-tree'
>>> after the add. With that, the commit time slowed down to 8 seconds per
>>> commit, plus 4 more seconds for the read-tree/add/write-tree ops. The
>>> read-tree/add/write-tree each took about a second.
>>
>>read-tree destroys stat info in index, refreshing 100k entries in
>>index in this case may take some time. Try this to see if commit time
>>reduces and how much time update-index takes
>>
>>read-tree HEAD
>>update-index --refresh
>>add ....
>>write-tree
>>commit -q
>
>I added the "update-index --refresh" and the time for commit became more
>like 0.6 seconds.
>In this setup: read-tree takes ~2 seconds, update-index takes ~8 seconds,
>git-add takes 1 to 4 seconds, and write-tree takes less than 1 second.
>
>>
>>> As an experiment, I also tried removing the 'git read-tree' and just
>>> having the git-write-tree. That sped up commits to 0.6 seconds, but
>>>the
>>> overall time for add/write-tree/commit was still 3 to 6 seconds.
>>
>>overall time is not really important because we duplicate work here
>>(write-tree is done as part of commit again). What I'm trying to do is
>>to determine how much time each operation in commit may take.
>>--
>>Duy
>
^ permalink raw reply
* Question about commit message wrapping
From: Sidney San Martín @ 2011-12-09 1:59 UTC (permalink / raw)
To: git
Hey, I want to ask about the practice of wrapping commit messages to 70-something charaters.
The webpage most cited about it, which I otherwise really like, is
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
*Nothing else* in my everyday life works this way anymore. Line wrapping gets done on the display end in my email client, my web browser, my ebook reader entirely automatically, and it adapts to the size of the window.
That article gives two reasons why commits should be wrapped to 72 columns. First:
> git log doesn’t do any special special wrapping of the commit messages. With the default pager of less -S, this means your paragraphs flow far off the edge of the screen, making them difficult to read. On an 80 column terminal, if we subtract 4 columns for the indent on the left and 4 more for symmetry on the right, we’re left with 72 columns.
Here, I put a patch at the bottom of this email that wraps commit messages to, right now, 80 columns when they're displayed. (It’s a quick one, probably needs configurability. Also, beware, I don’t program in C too much.)
Second:
> git format-patch --stdout converts a series of commits to a series of emails, using the messages for the message body. Good email netiquette dictates we wrap our plain text emails such that there’s room for a few levels of nested reply indicators without overflow in an 80 column terminal. (The current rails.git workflow doesn’t include email, but who knows what the future will bring.)
There's been a standard for flowed plain text emails (which don't have to wrap at 80 columns) for well over ten years, RFC-2646 and is widely supported. Besides, code in diffs is often longer than 7x characters, and wrapping, like `git log`, could be done inside git. FWIW, there are a bunch of merge commits with lines longer than 80 characters in the git repo itself.
Finally, people read commits these days in many other places than `git log` (and make commits in many other places than a text editor configured to wrap). Most every GUI and already word wraps commit messages just fine. As a result, there are commits in popular repos much longer than the 72-column standard and no one notices. Instead, properly-formatted commit messages end up looking cramped when you see them in anywhere wider than 80 columns.
Am I crazy? If this makes sense to anyone else, I'd be happy to help massage this into something git-worthy, with some help (never worked on Git before).
- - -
From a93b390d1506652d4ad41d1cbd987ba98a8deca0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= <s@sidneysm.com>
Date: Thu, 8 Dec 2011 20:26:23 -0500
Subject: [PATCH] Wrap commit messages on display
- Wrap to 80 characters minus the indent
- Use a hanging indent for lines which begin with "- "
- Do not wrap lines which begin with whitespace
---
pretty.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/pretty.c b/pretty.c
index 230fe1c..15804ce 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1243,8 +1243,14 @@ void pp_remainder(const struct pretty_print_context *pp,
memset(sb->buf + sb->len, ' ', indent);
strbuf_setlen(sb, sb->len + indent);
}
- strbuf_add(sb, line, linelen);
- strbuf_addch(sb, '\n');
+ if (line[0] == ' ' || line[0] == '\t') {
+ strbuf_add(sb, line, linelen);
+ } else {
+ struct strbuf wrapped = STRBUF_INIT;
+ strbuf_add(&wrapped, line, linelen);
+ strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + (line[0] == '-' && line[1] == ' ' ? 2 : 0), 80 - indent);
+ strbuf_addch(sb, '\n');
+ }
}
}
--
1.7.8
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox