git-gui resets author on amend
Hi, Amending a commit using git gui resets its author, unlike plain git commit --amend. - Orgad -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] submodule: add verbose mode for add/update
Executes checkout without -q
---
git-submodule.sh | 24 +++-
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index a33f68d..5c4e057 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,11 +5,11 @@
# Copyright (c) 2007 Lars Hjemli
dashless=$(basename $0 | sed -e 's/-/ /')
-USAGE=[--quiet] add [-b branch] [-f|--force] [--name name]
[--reference repository] [--] repository [path]
+USAGE=[--quiet] add [-b branch] [-f|--force] [--name name]
[--reference repository] [-v|--verbose] [--] repository [path]
or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
or: $dashless [--quiet] init [--] [path...]
or: $dashless [--quiet] deinit [-f|--force] [--] path...
- or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge]
[--recursive] [--] [path...]
+ or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge]
[--recursive] [-v|--verbose] [--] [path...]
or: $dashless [--quiet] summary [--cached|--files]
[--summary-limit n] [commit] [--] [path...]
or: $dashless [--quiet] foreach [--recursive] command
or: $dashless [--quiet] sync [--recursive] [--] [path...]
@@ -319,12 +319,16 @@ module_clone()
rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
(
clear_local_git_env
+ if test -z $verbose
+ then
+ subquiet=-q
+ fi
cd $sm_path
GIT_WORK_TREE=. git config core.worktree $rel/$b
# ash fails to wordsplit ${local_branch:+-B $local_branch...}
case $local_branch in
- '') git checkout -f -q ${start_point:+$start_point} ;;
- ?*) git checkout -f -q -B $local_branch ${start_point:+$start_point} ;;
+ '') git checkout -f $subquiet ${start_point:+$start_point} ;;
+ ?*) git checkout -f $subquiet -B $local_branch
${start_point:+$start_point} ;;
esac
) || die $(eval_gettext Unable to setup cloned submodule '\$sm_path')
}
@@ -380,6 +384,9 @@ cmd_add()
--depth=*)
depth=$1
;;
+ -v|--verbose)
+ verbose=1
+ ;;
--)
shift
break
@@ -786,6 +793,9 @@ cmd_update()
--depth=*)
depth=$1
;;
+ -v|--verbose)
+ verbose=1
+ ;;
--)
shift
break
@@ -913,7 +923,11 @@ Maybe you want to use 'update --init'?)
must_die_on_failure=
case $update_module in
checkout)
- command=git checkout $subforce -q
+ if test -z $verbose
+ then
+ subquiet=-q
+ fi
+ command=git checkout $subforce $subquiet
die_msg=$(eval_gettext Unable to checkout '\$sha1' in submodule
path '\$displaypath')
say_msg=$(eval_gettext Submodule path '\$displaypath': checked out
'\$sha1')
;;
--
1.9.0.msysgit.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Add grep.fullName config variable
This configuration variable sets the default for the --full-name option.
Signed-off-by: Andreas Schwab sch...@linux-m68k.org
---
Documentation/git-grep.txt | 3 +++
grep.c | 5 +
2 files changed, 8 insertions(+)
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index f837334..31811f1 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -53,6 +53,9 @@ grep.extendedRegexp::
option is ignored when the 'grep.patternType' option is set to a value
other than 'default'.
+grep.fullName::
+ If set to true, enable '--full-name' option by default.
+
OPTIONS
---
diff --git a/grep.c b/grep.c
index c668034..ece04bf 100644
--- a/grep.c
+++ b/grep.c
@@ -86,6 +86,11 @@ int grep_config(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp(var, grep.fullname)) {
+ opt-relative = !git_config_bool(var, value);
+ return 0;
+ }
+
if (!strcmp(var, color.grep))
opt-color = git_config_colorbool(var, value);
else if (!strcmp(var, color.grep.context))
--
1.9.0
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
And now for something completely different.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: option argument name hints
On 3/11/2014 12:10 PM, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: Documentation on the whole argument parsing is quite short, so, I though, adding an example just to show how usage is generated would look like I am trying to make this feature look important than it is :) You already are by saying the Angle brackets are automatic, aren't you? That is, among the things --parseopt mode does, the above stresses what happens _only_ when it emits help text for items that use this feature. `argh' is used only while help text is generated. So, there seems to be no way around it :) I was talking not about the automatic addition of angle brackets, but about the documentation on `argh' in general. The section where I've added a paragraph, is not specific to the help output, but describes --parseopt. I though that an example just to describe `argh' while useful would look a bit disproportional, compared to the amount of text on --parseopt. But now that I've added a Usage text section to looks quite in place. I just realized that the second patch I sent did not contain the changes. Sorry about - I will resend it. I was also wondering about the possible next step(s). If you like the patch will you just take it from the maillist and it would appear in the next What's cooking in git.git? Or the process is different? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
GSoC proposal: port pack bitmap support to libgit2.
Hi, I'm Yuxuan Shui, a undergraduate student from China. I'm applying for GSoC 2014, and here is my proposal: I found this idea on the ideas page, and did some research about it. The pack bitmap patchset add a new .bitmap file for every pack file which contains the reachability information of selected commits. This information is used to speed up git fetching and cloning, and produce a very convincing results. The goal of my project is to port the pack bitmap implementation in core git to libgit2, so users of libgit2 could benefit from this optimization as well. Please let me know if my proposal makes sense, thanks. P.S. I've submitted by microproject patch[1], but haven't received any response yet. [1]: http://thread.gmane.org/gmane.comp.version-control.git/243854 -- Regards Yuxuan Shui -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] install_branch_config(): switch from 'else-if' series to 'nested if-else'
From: Nishchal nishga...@gmail.com
I am Nishchal Gaba, a GSOC 2014 aspirant, submitting patch in response to
microproject(8)
Similar Execution Time, but increased readability
Alternate Solution Discarded: Table driven code using commonanilty of the
statements to be printed due to _()
Signed-off-by: Nishchal Gaba nishga...@gmail.com
---
---
branch.c | 28 +---
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/branch.c b/branch.c
index 723a36b..04e9e24 100644
--- a/branch.c
+++ b/branch.c
@@ -77,26 +77,32 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
strbuf_release(key);
if (flag BRANCH_CONFIG_VERBOSE) {
- if (remote_is_branch origin)
- printf_ln(rebasing ?
+ if (origin){
+ if(remote_is_branch)
+ printf_ln(rebasing ?
_(Branch %s set up to track remote branch %s
from %s by rebasing.) :
_(Branch %s set up to track remote branch %s
from %s.),
local, shortname, origin);
- else if (remote_is_branch !origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track local branch %s
by rebasing.) :
- _(Branch %s set up to track local branch
%s.),
- local, shortname);
- else if (!remote_is_branch origin)
- printf_ln(rebasing ?
+ else
+ printf_ln(rebasing ?
_(Branch %s set up to track remote ref %s by
rebasing.) :
_(Branch %s set up to track remote ref %s.),
local, remote);
- else if (!remote_is_branch !origin)
- printf_ln(rebasing ?
+ }
+
+ else if (!origin){
+ if(remote_is_branch)
+ printf_ln(rebasing ?
+ _(Branch %s set up to track local branch %s
by rebasing.) :
+ _(Branch %s set up to track local branch
%s.),
+ local, shortname);
+ else
+ printf_ln(rebasing ?
_(Branch %s set up to track local ref %s by
rebasing.) :
_(Branch %s set up to track local ref %s.),
local, remote);
+ }
+
else
die(BUG: impossible combination of %d and %p,
remote_is_branch, origin);
--
1.8.1.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] submodule: add verbose mode for add/update
On Mar 12, 2014, at 2:38 AM, Orgad Shaneh org...@gmail.com wrote:
Executes checkout without -q
—
Missing sign-off. See Documentation/SubmittingPatches.
Your patch is badly whitespace-damaged, as if it was pasted into your email
client. “git send-email” can avoid this problem.
As I’m not a submodule user, I won’t review the content of the patch other than
to say that such a change should be accompanied by documentation update
(Documentation/git-submodule.txt) and additional tests.
git-submodule.sh | 24 +++-
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index a33f68d..5c4e057 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,11 +5,11 @@
# Copyright (c) 2007 Lars Hjemli
dashless=$(basename $0 | sed -e 's/-/ /')
-USAGE=[--quiet] add [-b branch] [-f|--force] [--name name]
[--reference repository] [--] repository [path]
+USAGE=[--quiet] add [-b branch] [-f|--force] [--name name]
[--reference repository] [-v|--verbose] [--] repository [path]
or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
or: $dashless [--quiet] init [--] [path...]
or: $dashless [--quiet] deinit [-f|--force] [--] path...
- or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge]
[--recursive] [--] [path...]
+ or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge]
[--recursive] [-v|--verbose] [--] [path...]
or: $dashless [--quiet] summary [--cached|--files]
[--summary-limit n] [commit] [--] [path...]
or: $dashless [--quiet] foreach [--recursive] command
or: $dashless [--quiet] sync [--recursive] [--] [path...]
@@ -319,12 +319,16 @@ module_clone()
rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
(
clear_local_git_env
+ if test -z $verbose
+ then
+ subquiet=-q
+ fi
cd $sm_path
GIT_WORK_TREE=. git config core.worktree $rel/$b
# ash fails to wordsplit ${local_branch:+-B $local_branch...}
case $local_branch in
- '') git checkout -f -q ${start_point:+$start_point} ;;
- ?*) git checkout -f -q -B $local_branch ${start_point:+$start_point} ;;
+ '') git checkout -f $subquiet ${start_point:+$start_point} ;;
+ ?*) git checkout -f $subquiet -B $local_branch
${start_point:+$start_point} ;;
esac
) || die $(eval_gettext Unable to setup cloned submodule '\$sm_path')
}
@@ -380,6 +384,9 @@ cmd_add()
--depth=*)
depth=$1
;;
+ -v|--verbose)
+ verbose=1
+ ;;
--)
shift
break
@@ -786,6 +793,9 @@ cmd_update()
--depth=*)
depth=$1
;;
+ -v|--verbose)
+ verbose=1
+ ;;
--)
shift
break
@@ -913,7 +923,11 @@ Maybe you want to use 'update --init'?)
must_die_on_failure=
case $update_module in
checkout)
- command=git checkout $subforce -q
+ if test -z $verbose
+ then
+ subquiet=-q
+ fi
+ command=git checkout $subforce $subquiet
die_msg=$(eval_gettext Unable to checkout '\$sha1' in submodule
path '\$displaypath')
say_msg=$(eval_gettext Submodule path '\$displaypath': checked out
'\$sha1')
;;
--
1.9.0.msysgit.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approach
Thanks for the submission. Comments below to give you a taste of the
Git review process...
On Tue, Mar 11, 2014 at 11:47 PM, Yao Zhao zhaox...@umn.edu wrote:
Subject: SoC 2014 MicroProject No.8:change multiple if-else statement to
table-driven approach
The email subject is extracted automatically by git am as the first
line of the patch's commit message so it should contain only text
which is relevant to the commit message. In this case, everything
before changes is merely commentary for readers of the email, and
not relevant to the commit message.
It is indeed a good idea to let reviewers know that this submission is
for GSoC, and you can indicate this as such:
Subject: [PATCH GSoC] change multiple if-else statements to be table-driven
Signed-off-by: Yao Zhao zhaox...@umn.edu
---
The additional information that this is GSoC microproject #8 would go
in the commentary area right here after the --- following your
sign-off.
branch.c | 55 +--
1 file changed, 53 insertions(+), 2 deletions(-)
The patch is rife with style violations. I'll point out the first
instance of each violation, but do be sure to fix all remaining ones
when you resubmit. See Documentation/CodingGuidelines for details.
diff --git a/branch.c b/branch.c
index 723a36b..6432e27 100644
--- a/branch.c
+++ b/branch.c
@@ -53,7 +53,20 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
int remote_is_branch = starts_with(remote, refs/heads/);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
-
+ char** print_list = malloc(8 * sizeof(char*));
Style: char **print_list
Why allocate 'print_list' on the heap? An automatic variable 'char
const *print_list[]' would be more idiomatic and less likely to be
leaked.
In fact, your heap-allocated 'print_list' _is_ being leaked a few
lines down when the function returns early after warning that a branch
can not be its own upstream.
+ char* arg1=NULL;
+ char* arg2=NULL;
+ char* arg3=NULL;
Style: char *var
Style: whitespace: var = NULL
+ int index=0;
+
+ print_list[7] = _(Branch %s set up to track remote branch %s from %s
by rebasing.);
+ print_list[6] = _(Branch %s set up to track remote branch %s from
%s.);
+ print_list[5] = _(Branch %s set up to track local branch %s by
rebasing.);
+ print_list[4] = _(Branch %s set up to track local branch %s.);
+ print_list[3] = _(Branch %s set up to track remote ref %s by
rebasing.);
+ print_list[2] = _(Branch %s set up to track remote ref %s.);
+ print_list[1] = _(Branch %s set up to track local ref %s by
rebasing.);
+ print_list[0] = _(Branch %s set up to track local ref %s.);
If you make print_list[] an automatic variable, then you can declare
and populate it via a simple initializer. No need for this manual
approach.
if (remote_is_branch
!strcmp(local, shortname)
!origin) {
@@ -77,7 +90,44 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
strbuf_release(key);
if (flag BRANCH_CONFIG_VERBOSE) {
- if (remote_is_branch origin)
+ if(remote_is_branch)
Style: whitespace: if (...)
+ index += 4;
+ if(origin)
+ index += 2;
+ if(rebasing)
+ index += 1;
+
+ if(index 0 || index 7)
+ {
+ die(BUG: impossible combination of %d and %p,
+ remote_is_branch, origin);
+ }
+
+ if(index = 4) {
+ arg1 = local;
+ arg2 = remote;
+ }
+ else if(index 6) {
Style: } else if (...) {
+ arg1 = local;
+ arg2 = shortname;
+ arg3 = origin;
+ }
+ else {
+ arg1 = local;
+ arg2 = shortname;
+ }
+
+ if(!arg3) {
+ printf_ln(print_list[index],arg1,arg2);
Style: whitespace: printf_ln(x, y, z)
+ }
+ else {
+ printf_ln(print_list[index],arg1,arg2,arg3);
+ }
Unfortunately, this is quite a bit more verbose and complex than the
original code, and all the magic numbers (4, 2, 1, 0, 7, 4, 6) place a
higher cognitive load on the reader, so this change probably is a net
loss as far as clarity is concerned.
Take a step back and consider again the GSoC miniproject: It talks
about making the code table-driven. Certainly, you have moved the
strings into a table, but all the complex logic is still in the code,
and not in a table, hence it's not
Re: [PATCH] install_branch_config(): switch from 'else-if' series to 'nested if-else'
Nishhal Gaba nishga...@gmail.com writes:
From: Nishchal nishga...@gmail.com
Set user.email/user.name and sendemail.from to the same address to avoid
this inline From:.
I am Nishchal Gaba, a GSOC 2014 aspirant, submitting patch in response to
microproject(8)
This part of your message is the commit message. It should justify why
the change is good, but who you are is not very interesting here (think
of someone running git log or git blame a year from now and going
through your commit, what would he expect?). The first sentence could go
below the ---.
Please, wrap your messages (less than 80 characters per line).
Similar Execution Time, but increased readability
Why capitalize Execution Time?
+ if (origin){
Here and below: space before {
+ if(remote_is_branch)
space before (
+ printf_ln(rebasing ?
_(Branch %s set up to track remote branch %s
from %s by rebasing.) :
_(Branch %s set up to track remote branch %s
from %s.),
local, shortname, origin);
- else if (remote_is_branch !origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track local branch %s
by rebasing.) :
- _(Branch %s set up to track local branch
%s.),
- local, shortname);
- else if (!remote_is_branch origin)
- printf_ln(rebasing ?
+ else
+ printf_ln(rebasing ?
_(Branch %s set up to track remote ref %s by
rebasing.) :
_(Branch %s set up to track remote ref %s.),
At this point, it would make sense to me to factor the printf_ln call
like
const char *msg;
if (...)
msg = rebasing ? _(...) : _(...);
else
msg = rebasing ? _(...) : _(...);
printf_ln(msg, local, shortname);
(but that's very subjective)
- else if (!remote_is_branch !origin)
- printf_ln(rebasing ?
+ }
+
+ else if (!origin){
Err, isn't this the else branch of if (origin) ? If so, why repeat
!origin, and more specifically, isn't the next else branch dead
code:
+ }
+
else
die(BUG: impossible combination of %d and %p,
remote_is_branch, origin);
I mean: obviously, it has to be dead code, but it seems a bit strange to
read
if (x)
...
else if (!x)
...
else
die(...)
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
egit vs. git behaviour (was: [RFC/WIP] Pluggable reference backends)
On Mon, 10 Mar 2014 19:39:00 +, Shawn Pearce wrote: Yes, this was my real concern. Eclipse users using EGit expect EGit to be compatible with git-core at the filesystem level so they can do something in EGit then switch to a shell and bang out a command, or run a script provided by their project or co-worker. A question: Where to ask/report problems with that? We're currently running into problems that egit doesn't push to where git would when the local and remote branches aren't the same name. It seems that egit ignores the branch.*.merge settings. Or push.default? Andreas -- Totally trivial. Famous last words. From: Linus Torvalds torvalds@*.org Date: Fri, 22 Jan 2010 07:29:21 -0800 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.
By convention, no full stop in the subject line. The subject should
summarize your changes and add ..NONEG is just one part of it. The
other is convert to use ...NONEG. So I suggest parse-options:
convert to use new macro OPT_SET_INT_NONEG() or something like that.
You should also explain in the message body (before Signed-off-by:)
why this is a good thing to do. My guess is better readability and
harder to make mistakes in the future when you have to declare new
options with noneg.
On Tue, Mar 11, 2014 at 5:50 PM, Yuxuan Shui yshu...@gmail.com wrote:
Reference: http://git.github.io/SoC-2014-Microprojects.html
I think this project is actually two: one is convert current
{OPTION_SET_INT, ... _NONEG} to the new macro, which is truly a micro
project. The other is to find OPT_...(..) that should have NONEG but
does not. This one may need more time because you need to check what
those options do and if it makes sense to have --no- form.
I think we can focus on the {OPTION_..., _NONEG} conversion, which
should be enough get you familiar with git community.
diff --git a/parse-options.h b/parse-options.h
index d670cb9..7d20cf9 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -125,6 +125,10 @@ struct option {
(h), PARSE_OPT_NOARG }
#define OPT_SET_INT(s, l, v, h, i) { OPTION_SET_INT, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG, NULL, (i) }
+#define OPT_SET_INT_NONEG(s, l, v, h, i) \
+ { OPTION_SET_INT, (s), (l), (v), NULL, \
+ (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG,
\
+ NULL, (i) }
#define OPT_BOOL(s, l, v, h)OPT_SET_INT(s, l, v, h, 1)
#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG |
PARSE_OPT_HIDDEN, NULL, 1}
To avoid the proliferation of similar macros in future, I think we
should make a macro that takes any flags, e.g.
#define OPT_SET_INT_X(s, l, v, h, i, flags) { ., PARSE_OPT_NOARG
| PARSE_OPT_ ## flags, NULL, (i) }
and we can use it for NONEG like OPT_SET_INT_X(, NONEG). We
could even redefine OPT_SET_INT() to use OPT_SET_INT_X() to reduce
duplication.
While we're at NONEG, I see that builtin/grep.c has this construct {
OPTION_INTEGER...NONEG} and builtin/read-tree.c has {
OPTION_STRING..NONEG}. It would be great if you could look at them
and see if NONEG is really needed there, or simpler forms
OPT_INTEGER(...) and OPT_STRING(...) are enough.
You might need to read parse-options.c to understand these options.
Documentation/technical/api-parse-options.txt should give you a good
overview.
You could also think if we could transform { OPTION_CALLBACK }
to OPT_CALLBACK(...). But if you do and decide to do it, please make
it a separate patch (one patch deals with one thing).
That remaining of your patch looks good.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP] Pluggable reference backends
Karsten, Thanks for your feedback! On 03/11/2014 11:56 AM, Karsten Blees wrote: Am 10.03.2014 12:00, schrieb Michael Haggerty: Reference transactions -- Very cool ideas indeed. However, I'm concerned a bit that transactions are conceptual overkill. How many concurrent updates do you expect in a repository? Wouldn't a single repo-wide lock suffice (and be _much_ simpler to implement with any backend, esp. file-based)? I am mostly thinking about long-running processes, like gc and prune-refs, which need to be made race-free without blocking other processes for the whole time they are running (whereas it might be quite tolerable to have them fail or only complete part of their work in any given invocation). Also, I work at GitHub, where we have quite a few repositories, some of which are quite active :-) Remember that I'm not yet proposing anything like hard-core ACID reference transactions. I'm just clearing the way for various possible changes in reference handling. I listed the ideas only to whet people's appetites and motivate the refactoring, which will take a while before it bears any real fruit. The API you posted in [1] doesn't look very much like a transaction API either (rather like batch-updates). E.g. there's no rollback, the queue* methods cannot report failure, and there's no way to read a ref as part of the transaction. So I'm afraid that backends that support transactions out of the box (e.g. RDBMSs) will be hard to adapt to this. Gmane is down at the moment but I assume you are referring to my patch series and the ref_transaction implementation therein. No explicit rollback is necessary at this stage, because the commit function first locks all of the references that it wants to change (first verifying that they have the expected values), and then modifies them all. By the time the references are locked, the whole transaction is guaranteed to succeed [1]. If the locks can't all be acquired, then any locks that were obtained are released. If a caller wants to rollback a transaction, it only needs to free the transaction instead of committing. I should probably make that clearer by renaming free_ref_transaction() to rollback_ref_transaction(). By the time we start implementing other reference backends, that function will of course have to do more. For that matter, maybe create_ref_transaction() should be renamed to begin_ref_transaction(). Now would be a good time for concrete bikeshedding suggestions about function names or other details of the API :-) Yes, the queue_*() methods should probably later make a preliminary check of the reference's old value and return an error if the expected value is already incorrect. This would allow callers to fail fast if the transaction is doomed to failure. But that wasn't needed yet for the one existing caller, which builds up a transaction and commits it immediately, so I didn't implement it yet. And the early checks would add overhead for this caller, so maybe they should be optional anyway. Maybe these functions should already be declared to return an error status, but there should be an option passed to create_ref_transaction() that selects whether fast checks should be performed or not for that transaction. Really, all that this first patch series does is put a different API around the mechanism that was already there, in update_refs(). There will be a lot more steps before we see anything approaching real reference transactions. But I think your (implied) suggestion, to make the API more reminiscent of something like database transactions, is a good one and I will work on it. Cheers, Michael [1] Guaranteed here is of course relative. The commit could still fail due to the process being killed, disk errors, etc. But it can't fail due to lock contention with another git process. -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
New GSoC microproject ideas
Hi, I just added a few microproject suggestions to the list for newly-arriving students [1]. A couple of them are weak, but I think number 17 has enough aspects to keep a whole crew of students busy for a while. Michael [1] http://git.github.io/SoC-2014-Microprojects.html -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GSoC14][RFC] Proposal Draft: Refactor tempfile handling
Currently the linked list of lockfiles only grows, never shrinks. Once an object has been linked into the list, there is no way to remove it again even after the lock has been released. So if a lock needs to be created dynamically at a random place in the code, its memory is unavoidably leaked. Ah yes, I see. I think a good example is config.git_config_set_multivar_in_file, which even contains a comment detailing the problem: Since lockfile.c keeps a linked list of all created lock_file structures, it isn't safe to free(lock). It's better to just leave it hanging around. But I have a feeling that if we want to use a similar mechanism to handle all temporary files (of which there can be more), then it would be a good idea to lift this limitation. It will require some care, though, to make sure that record removal is done in a way that is threadsafe and safe in the event of all expected kinds of process death. It sounds like a threadsafe linked-list with an interface to manually remove elements from the list is the solution here; does that sound reasonable? Ensuring thread safety without sacrificing readability is probably more difficult than it sounds, but I don't think it's impossible. I'll add some more details on this to my proposal[1]. Thank you! - Brian Gesiak [1] https://www.google-melange.com/gsoc/proposal/review/student/google/gsoc2014/modocache/5629499534213120 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #01; Tue, 4)
On Wed, Mar 5, 2014 at 7:10 AM, Junio C Hamano gits...@pobox.com wrote: [Graduated to master] * jk/pack-bitmap (2014-02-12) 26 commits (merged to 'next' on 2014-02-25 at 5f65d26) And it's finally in! Shall we start thinking about the next on-disk format? It was put aside last time to focus on getting this series in. My concern is shallow support (surprise?) so that cloning from a 1-year-long shallow repo is not slower than a complete one. An extensible format is enough without going into details. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] submodule: add verbose mode for add/update
From: Orgad Shaneh org...@gmail.com
Executes checkout without -q
Signed-off-by: Orgad Shaneh org...@gmail.com
---
Documentation/git-submodule.txt | 7 +--
git-submodule.sh| 24 +++-
t/t7406-submodule-update.sh | 9 +
3 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 21cb59a..1867e94 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -10,13 +10,13 @@ SYNOPSIS
[verse]
'git submodule' [--quiet] add [-b branch] [-f|--force] [--name name]
- [--reference repository] [--depth depth] [--] repository
[path]
+ [--reference repository] [--depth depth] [-v|--verbose] [--]
repository [path]
'git submodule' [--quiet] status [--cached] [--recursive] [--] [path...]
'git submodule' [--quiet] init [--] [path...]
'git submodule' [--quiet] deinit [-f|--force] [--] path...
'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase|--merge|--checkout] [--reference
repository]
- [--depth depth] [--recursive] [--] [path...]
+ [--depth depth] [--recursive] [-v|--verbose] [--] [path...]
'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) n]
[commit] [--] [path...]
'git submodule' [--quiet] foreach [--recursive] command
@@ -363,6 +363,9 @@ for linkgit:git-clone[1]'s `--reference` and `--shared`
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
+--verbose::
+ This option is valid for add and update commands. Show output of
+ checkout.
path...::
Paths to submodule(s). When specified this will restrict the command
diff --git a/git-submodule.sh b/git-submodule.sh
index a33f68d..5c4e057 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,11 +5,11 @@
# Copyright (c) 2007 Lars Hjemli
dashless=$(basename $0 | sed -e 's/-/ /')
-USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference
repository] [--] repository [path]
+USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference
repository] [-v|--verbose] [--] repository [path]
or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
or: $dashless [--quiet] init [--] [path...]
or: $dashless [--quiet] deinit [-f|--force] [--] path...
- or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge] [--recursive] [--]
[path...]
+ or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge] [--recursive]
[-v|--verbose] [--] [path...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit n]
[commit] [--] [path...]
or: $dashless [--quiet] foreach [--recursive] command
or: $dashless [--quiet] sync [--recursive] [--] [path...]
@@ -319,12 +319,16 @@ module_clone()
rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
(
clear_local_git_env
+ if test -z $verbose
+ then
+ subquiet=-q
+ fi
cd $sm_path
GIT_WORK_TREE=. git config core.worktree $rel/$b
# ash fails to wordsplit ${local_branch:+-B $local_branch...}
case $local_branch in
- '') git checkout -f -q ${start_point:+$start_point} ;;
- ?*) git checkout -f -q -B $local_branch
${start_point:+$start_point} ;;
+ '') git checkout -f $subquiet ${start_point:+$start_point} ;;
+ ?*) git checkout -f $subquiet -B $local_branch
${start_point:+$start_point} ;;
esac
) || die $(eval_gettext Unable to setup cloned submodule
'\$sm_path')
}
@@ -380,6 +384,9 @@ cmd_add()
--depth=*)
depth=$1
;;
+ -v|--verbose)
+ verbose=1
+ ;;
--)
shift
break
@@ -786,6 +793,9 @@ cmd_update()
--depth=*)
depth=$1
;;
+ -v|--verbose)
+ verbose=1
+ ;;
--)
shift
break
@@ -913,7 +923,11 @@ Maybe you want to use 'update --init'?)
must_die_on_failure=
case $update_module in
checkout)
- command=git checkout $subforce -q
+ if test -z $verbose
+ then
+ subquiet=-q
+ fi
+ command=git checkout $subforce
[PATCH] general style: replaces memcmp() with starts_with()
memcmp() is replaced with starts_with() when comparing strings from the
beginning. starts_with() looks nicer and it saves the extra argument of
the length of the comparing string.
Signed-off-by: Quint Guvernator quintus.pub...@gmail.com
---
builtin/apply.c | 10 +-
builtin/cat-file.c| 2 +-
builtin/commit-tree.c | 2 +-
builtin/for-each-ref.c| 2 +-
builtin/get-tar-commit-id.c | 2 +-
builtin/mailinfo.c| 10 +-
builtin/mktag.c | 8
builtin/patch-id.c| 18 +-
commit.c | 18 +-
connect.c | 8
contrib/convert-objects/convert-objects.c | 6 +++---
convert.c | 2 +-
credential-cache.c| 2 +-
fetch-pack.c | 2 +-
fsck.c| 8
http-walker.c | 4 ++--
imap-send.c | 2 +-
pack-write.c | 2 +-
path.c| 2 +-
refs.c| 2 +-
remote.c | 2 +-
submodule.c | 2 +-
transport.c | 2 +-
xdiff-interface.c | 2 +-
24 files changed, 60 insertions(+), 60 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index a7e72d5..8f21957 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -846,8 +846,8 @@ static int has_epoch_timestamp(const char *nameline)
* -MM-DD hh:mm:ss must be from either 1969-12-31
* (west of GMT) or 1970-01-01 (east of GMT)
*/
- if ((zoneoffset 0 memcmp(timestamp, 1969-12-31, 10)) ||
- (0 = zoneoffset memcmp(timestamp, 1970-01-01, 10)))
+ if ((zoneoffset 0 starts_with(timestamp, 1969-12-31)) ||
+ (0 = zoneoffset starts_with(timestamp, 1970-01-01)))
return 0;
hourminute = (strtol(timestamp + 11, NULL, 10) * 60 +
@@ -1631,7 +1631,7 @@ static int parse_fragment(const char *line, unsigned long
size,
* l10n of \ No newline... is at least that long.
*/
case '\\':
- if (len 12 || memcmp(line, \\ , 2))
+ if (len 12 || starts_with(line, \\ ))
return -1;
break;
}
@@ -1646,7 +1646,7 @@ static int parse_fragment(const char *line, unsigned long
size,
* it in the above loop because we hit oldlines == newlines == 0
* before seeing it.
*/
- if (12 size !memcmp(line, \\ , 2))
+ if (12 size !starts_with(line, \\ ))
offset += linelen(line, size);
patch-lines_added += added;
@@ -1673,7 +1673,7 @@ static int parse_single_patch(const char *line, unsigned
long size, struct patch
unsigned long oldlines = 0, newlines = 0, context = 0;
struct fragment **fragp = patch-fragments;
- while (size 4 !memcmp(line, @@ -, 4)) {
+ while (size 4 !starts_with(line, @@ -)) {
struct fragment *fragment;
int len;
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index d5a93e0..be83345 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const
char *obj_name)
enum object_type type;
unsigned long size;
char *buffer = read_sha1_file(sha1, type,
size);
- if (memcmp(buffer, object , 7) ||
+ if (starts_with(buffer, object ) ||
get_sha1_hex(buffer + 7, blob_sha1))
die(%s not a valid tag,
sha1_to_hex(sha1));
free(buffer);
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 987a4c3..2d995a2 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -66,7 +66,7 @@ int cmd_commit_tree(int argc, const char **argv, const char
*prefix)
continue;
}
- if (!memcmp(arg, -S, 2)) {
+ if (!starts_with(arg, -S)) {
sign_commit = arg + 2;
continue;
}
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 51798b4..be14d71 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -193,7 +193,7 @@ static int verify_format(const char *format)
at = parse_atom(sp + 2, ep);
cp = ep + 1;
- if
Re: [PATCH] general style: replaces memcmp() with starts_with()
On Wed, Mar 12, 2014 at 8:44 PM, Quint Guvernator quintus.pub...@gmail.com wrote: diff --git a/builtin/apply.c b/builtin/apply.c index a7e72d5..8f21957 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -846,8 +846,8 @@ static int has_epoch_timestamp(const char *nameline) * -MM-DD hh:mm:ss must be from either 1969-12-31 * (west of GMT) or 1970-01-01 (east of GMT) */ - if ((zoneoffset 0 memcmp(timestamp, 1969-12-31, 10)) || - (0 = zoneoffset memcmp(timestamp, 1970-01-01, 10))) + if ((zoneoffset 0 starts_with(timestamp, 1969-12-31)) || + (0 = zoneoffset starts_with(timestamp, 1970-01-01))) return 0; It is not a plain search/replace. starts_with(..) == !memcmp(...). So you need to negate every replacement. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with starts_with()
2014-03-12 9:51 GMT-04:00 Duy Nguyen pclo...@gmail.com: starts_with(..) == !memcmp(...). So you need to negate every replacement. My apologies--it doesn't look like the tests caught it either. I will fix this and submit a new patch. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] general style: replaces memcmp() with proper starts_with()
memcmp() is replaced with negated starts_with() when comparing strings
from the beginning. starts_with() looks nicer and it saves the extra
argument of the length of the comparing string.
note: this commit properly handles negation in starts_with()
Signed-off-by: Quint Guvernator quintus.pub...@gmail.com
---
builtin/apply.c | 10 +-
builtin/cat-file.c| 2 +-
builtin/commit-tree.c | 2 +-
builtin/for-each-ref.c| 2 +-
builtin/get-tar-commit-id.c | 2 +-
builtin/mailinfo.c| 10 +-
builtin/mktag.c | 8
builtin/patch-id.c| 18 +-
commit.c | 18 +-
connect.c | 8
contrib/convert-objects/convert-objects.c | 6 +++---
convert.c | 2 +-
credential-cache.c| 2 +-
fetch-pack.c | 2 +-
fsck.c| 8
http-walker.c | 4 ++--
imap-send.c | 6 +++---
pack-write.c | 2 +-
path.c| 2 +-
refs.c| 2 +-
remote.c | 2 +-
submodule.c | 2 +-
transport.c | 2 +-
23 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index a7e72d5..16c20af 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -846,8 +846,8 @@ static int has_epoch_timestamp(const char *nameline)
* -MM-DD hh:mm:ss must be from either 1969-12-31
* (west of GMT) or 1970-01-01 (east of GMT)
*/
- if ((zoneoffset 0 memcmp(timestamp, 1969-12-31, 10)) ||
- (0 = zoneoffset memcmp(timestamp, 1970-01-01, 10)))
+ if ((zoneoffset 0 !starts_with(timestamp, 1969-12-31)) ||
+ (0 = zoneoffset !starts_with(timestamp, 1970-01-01)))
return 0;
hourminute = (strtol(timestamp + 11, NULL, 10) * 60 +
@@ -1631,7 +1631,7 @@ static int parse_fragment(const char *line, unsigned long
size,
* l10n of \ No newline... is at least that long.
*/
case '\\':
- if (len 12 || memcmp(line, \\ , 2))
+ if (len 12 || !starts_with(line, \\ ))
return -1;
break;
}
@@ -1646,7 +1646,7 @@ static int parse_fragment(const char *line, unsigned long
size,
* it in the above loop because we hit oldlines == newlines == 0
* before seeing it.
*/
- if (12 size !memcmp(line, \\ , 2))
+ if (12 size starts_with(line, \\ ))
offset += linelen(line, size);
patch-lines_added += added;
@@ -1673,7 +1673,7 @@ static int parse_single_patch(const char *line, unsigned
long size, struct patch
unsigned long oldlines = 0, newlines = 0, context = 0;
struct fragment **fragp = patch-fragments;
- while (size 4 !memcmp(line, @@ -, 4)) {
+ while (size 4 starts_with(line, @@ -)) {
struct fragment *fragment;
int len;
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index d5a93e0..6266bbb 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const
char *obj_name)
enum object_type type;
unsigned long size;
char *buffer = read_sha1_file(sha1, type,
size);
- if (memcmp(buffer, object , 7) ||
+ if (!starts_with(buffer, object ) ||
get_sha1_hex(buffer + 7, blob_sha1))
die(%s not a valid tag,
sha1_to_hex(sha1));
free(buffer);
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 987a4c3..2777519 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -66,7 +66,7 @@ int cmd_commit_tree(int argc, const char **argv, const char
*prefix)
continue;
}
- if (!memcmp(arg, -S, 2)) {
+ if (starts_with(arg, -S)) {
sign_commit = arg + 2;
continue;
}
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 51798b4..fe198fd 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -193,7 +193,7 @@ static int verify_format(const char *format)
at = parse_atom(sp + 2, ep);
cp = ep + 1;
-
Re: [PATCH] general style: replaces memcmp() with starts_with()
Am 12.03.2014 14:44, schrieb Quint Guvernator:
memcmp() is replaced with starts_with() when comparing strings from the
beginning. starts_with() looks nicer and it saves the extra argument of
the length of the comparing string.
Signed-off-by: Quint Guvernator quintus.pub...@gmail.com
---
...
diff --git a/submodule.c b/submodule.c
index b80ecac..1edebc1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -203,7 +203,7 @@ void gitmodules_config(void)
if (active_nr pos) { /* there is a .gitmodules */
const struct cache_entry *ce =
active_cache[pos];
if (ce_namelen(ce) == 11
- !memcmp(ce-name, .gitmodules, 11))
+ !starts_with(ce-name, .gitmodules))
gitmodules_is_unmerged = 1;
}
} else if (pos active_nr) {
I think this hunk should be dropped as the memcmp() here doesn't mean
starts with but is identical (due to the ce_namelen(ce) == 11 in
the line above).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] submodule: add verbose mode for add/update
Am 12.03.2014 14:42, schrieb Orgad Shaneh:
From: Orgad Shaneh org...@gmail.com
You don't need the line above when you are the sender ;-)
Executes checkout without -q
That's a bit terse. What about:
Add the verbose flag to add and update which displays the
progress of the actual submodule checkout when given. This
is useful for checkouts that take a long time, as the user
can then see the progress.
Signed-off-by: Orgad Shaneh org...@gmail.com
---
Documentation/git-submodule.txt | 7 +--
git-submodule.sh| 24 +++-
t/t7406-submodule-update.sh | 9 +
3 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 21cb59a..1867e94 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -10,13 +10,13 @@ SYNOPSIS
[verse]
'git submodule' [--quiet] add [-b branch] [-f|--force] [--name name]
- [--reference repository] [--depth depth] [--] repository
[path]
+ [--reference repository] [--depth depth] [-v|--verbose] [--]
repository [path]
'git submodule' [--quiet] status [--cached] [--recursive] [--] [path...]
'git submodule' [--quiet] init [--] [path...]
'git submodule' [--quiet] deinit [-f|--force] [--] path...
'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase|--merge|--checkout] [--reference
repository]
- [--depth depth] [--recursive] [--] [path...]
+ [--depth depth] [--recursive] [-v|--verbose] [--] [path...]
'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit)
n]
[commit] [--] [path...]
'git submodule' [--quiet] foreach [--recursive] command
@@ -363,6 +363,9 @@ for linkgit:git-clone[1]'s `--reference` and `--shared`
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
+--verbose::
+ This option is valid for add and update commands. Show output of
+ checkout.
The above looks whitespace-damaged, you should use TABs here to
indent (just like the other options do).
path...::
Paths to submodule(s). When specified this will restrict the command
diff --git a/git-submodule.sh b/git-submodule.sh
index a33f68d..5c4e057 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,11 +5,11 @@
# Copyright (c) 2007 Lars Hjemli
dashless=$(basename $0 | sed -e 's/-/ /')
-USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference
repository] [--] repository [path]
+USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference
repository] [-v|--verbose] [--] repository [path]
or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
or: $dashless [--quiet] init [--] [path...]
or: $dashless [--quiet] deinit [-f|--force] [--] path...
- or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge] [--recursive]
[--] [path...]
+ or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge] [--recursive]
[-v|--verbose] [--] [path...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit n]
[commit] [--] [path...]
or: $dashless [--quiet] foreach [--recursive] command
or: $dashless [--quiet] sync [--recursive] [--] [path...]
@@ -319,12 +319,16 @@ module_clone()
rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
(
clear_local_git_env
+ if test -z $verbose
+ then
+ subquiet=-q
+ fi
cd $sm_path
GIT_WORK_TREE=. git config core.worktree $rel/$b
# ash fails to wordsplit ${local_branch:+-B $local_branch...}
case $local_branch in
- '') git checkout -f -q ${start_point:+$start_point} ;;
- ?*) git checkout -f -q -B $local_branch
${start_point:+$start_point} ;;
+ '') git checkout -f $subquiet ${start_point:+$start_point} ;;
+ ?*) git checkout -f $subquiet -B $local_branch
${start_point:+$start_point} ;;
Wouldn't it be better to use the ${subquiet:+$subquiet} notation
here like the other optional arguments do? After all the subquiet
isn't always set.
esac
) || die $(eval_gettext Unable to setup cloned submodule
'\$sm_path')
}
@@ -380,6 +384,9 @@ cmd_add()
--depth=*)
depth=$1
;;
+ -v|--verbose)
+ verbose=1
+ ;;
--)
shift
break
@@ -786,6 +793,9 @@ cmd_update()
--depth=*)
depth=$1
;;
+ -v|--verbose)
+
Re: [PATCH] general style: replaces memcmp() with starts_with()
2014-03-12 11:47 GMT-04:00 Jens Lehmann jens.lehm...@web.de: I think this hunk should be dropped as the memcmp() here doesn't mean starts with but is identical (due to the ce_namelen(ce) == 11 in the line above). There is an issue with negation in this patch. I've submitted a new one [1] to the mailing list. The subject line of the new patch is [PATCH] general style: replaces memcmp() with proper starts_with(). Let me know if you still think the hunk should be dropped there. [1]: http://thread.gmane.org/gmane.comp.version-control.git/243940 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: egit vs. git behaviour (was: [RFC/WIP] Pluggable reference backends)
On Wed, Mar 12, 2014 at 3:26 AM, Andreas Krey a.k...@gmx.de wrote: On Mon, 10 Mar 2014 19:39:00 +, Shawn Pearce wrote: Yes, this was my real concern. Eclipse users using EGit expect EGit to be compatible with git-core at the filesystem level so they can do something in EGit then switch to a shell and bang out a command, or run a script provided by their project or co-worker. A question: Where to ask/report problems with that? EGit developers have a bug tracker, from: http://eclipse.org/egit/support/ We see File a bug with a link to: https://bugs.eclipse.org/bugs/enter_bug.cgi?product=EGitrep_platform=Allop_sys=All We're currently running into problems that egit doesn't push to where git would when the local and remote branches aren't the same name. It seems that egit ignores the branch.*.merge settings. Or push.default? I think this is just missing code in EGit. Its probable they already know about it, or many of them don't use these features in .git/config and thus don't realize they are missing. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: option argument name hints
Ilya Bobyr ilya.bo...@gmail.com writes: I though that an example just to describe `argh' while useful would look a bit disproportional, compared to the amount of text on --parseopt. But now that I've added a Usage text section to looks quite in place. Good thinking. I was also wondering about the possible next step(s). If you like the patch will you just take it from the maillist and it would appear in the next What's cooking in git.git? Or the process is different? It goes more like this: - A topic that is in a good enough shape to be discussed and moved forward is given its own topic branch and then merged to 'pu', so that we do not forget. The topic enters What's cooking at this stage. - Discussion on the topic continues on the list, and the topic can be replaced or built upon while it is still on 'pu' to polish it further. . We may see a grave issue with the change and may discard it from 'pu'. . We may see a period of inaction after issues are pointed out and/or improvements are suggested, which would cause the topic marked as stalled; this may cause it to be eventually discarded as abandoned if nobody cares deeply enough. - After a while, when it seems that we, collectively as the Git development circle, agree that we would eventually want that change in a released version in some future (not necessarily in the upcoming release), the topic is merged to 'next', which is the branch Git developers are expected to run in their daily lives. . We may see some updates that builds on the patches merged to 'next' so far to fix late issues discovered. . We may see a grave issue with the change and may have to revert discard it from 'next'. - After a while, when the topic proves to be solid, it is merged to 'master', in preparation for the upcoming release. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH] implement submodule config cache for lookup of submodule names
On Tue, Mar 11, 2014 at 05:58:08PM -0400, Jeff King wrote:
On Mon, Mar 10, 2014 at 10:24:12PM +0100, Heiko Voigt wrote:
I have also moved all functions into the new submodule-config-cache
module. I am not completely satisfied with the naming since it is quite
long. If someone comes up with some different naming I am open for it.
Maybe simply submodule-config (submodule_config prefix for functions)?
Since the cache is totally internal to the submodule-config code, I do
not know that you even need to have a nice submodule-config-cache API.
Can it just be a singleton?
That is bad design in a sense (it becomes harder later if you ever want
to pull submodule config from two sources simultaneously), but it
matches many other subsystems in git which cache behind the scenes.
I also suspect you could call submodule_config simply submodule, and
let it be a stand-in for the submodule (for now, only data from the
config, but potentially you could keep other data on it, too).
So with all that, the entry point into your code is just:
const struct submodule *submodule_from_path(const char *path);
and the caching magically happens behind-the-scenes.
Actually since we also need to define the revision from which we request
these submodule values that would become:
const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
const char *path);
Since the configuration for submodules for a submodule are identified by
a unique commit sha1 and its unique path (or unique name) I think it is
feasible to make it a singleton. That would also simplify using it from
the existing config parsing code.
To be future proof I can internally keep the object oriented approach
always passing on the submodule_config_cache pointer. That way it would
be easy to expose to the outside in case we later need multiple caches.
So I currently I do not see any downside of making it a singleton to the
outside and would go with that.
+/* one submodule_config_cache entry */
+struct submodule_config {
+ struct strbuf path;
+ struct strbuf name;
+ unsigned char gitmodule_sha1[20];
+};
Do these strings need changed after they are written once? If not, you
may want to just make these bare pointers (you can still use strbufs to
create them, and then strbuf_detach at the end). That may just be a matter of
taste, though.
No they do not need to be changed after parsing, since every path,
name mapping should be unique in one .gitmodule file. And I think it
actually would make the code more clear in one instance where I directly
set the buf pointer which Jonathan mentioned. There it is needed only
for the hashmap lookup.
Cheers Heiko
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with starts_with()
Am 12.03.2014 17:46, schrieb Quint Guvernator: 2014-03-12 11:47 GMT-04:00 Jens Lehmann jens.lehm...@web.de: I think this hunk should be dropped as the memcmp() here doesn't mean starts with but is identical (due to the ce_namelen(ce) == 11 in the line above). There is an issue with negation in this patch. I've submitted a new one [1] to the mailing list. The subject line of the new patch is [PATCH] general style: replaces memcmp() with proper starts_with(). Thanks, I missed that one (please use [PATCH v2] in the subject line of a second patch to make follow-ups easily distinguishable from the initial one ;-). Let me know if you still think the hunk should be dropped there. Yes, I think so. That spot uses memcmp() because ce-name may not be 0-terminated. If that assumption isn't correct, it should be replaced with a plain strcmp() instead (while also dropping the ce_namelen() comparison in the line above). But starts_with() points into the wrong direction there. [1]: http://thread.gmane.org/gmane.comp.version-control.git/243940 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.
On Wed, Mar 12, 2014 at 6:47 PM, Duy Nguyen pclo...@gmail.com wrote:
By convention, no full stop in the subject line. The subject should
summarize your changes and add ..NONEG is just one part of it. The
other is convert to use ...NONEG. So I suggest parse-options:
convert to use new macro OPT_SET_INT_NONEG() or something like that.
You should also explain in the message body (before Signed-off-by:)
why this is a good thing to do. My guess is better readability and
harder to make mistakes in the future when you have to declare new
options with noneg.
Thanks for pointing that out, I'll do as you suggested.
On Tue, Mar 11, 2014 at 5:50 PM, Yuxuan Shui yshu...@gmail.com wrote:
Reference: http://git.github.io/SoC-2014-Microprojects.html
I think this project is actually two: one is convert current
{OPTION_SET_INT, ... _NONEG} to the new macro, which is truly a micro
project. The other is to find OPT_...(..) that should have NONEG but
does not. This one may need more time because you need to check what
those options do and if it makes sense to have --no- form.
Hmm, this microproject has been striked from the ideas page, maybe I
should switch to another one...
I think we can focus on the {OPTION_..., _NONEG} conversion, which
should be enough get you familiar with git community.
diff --git a/parse-options.h b/parse-options.h
index d670cb9..7d20cf9 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -125,6 +125,10 @@ struct option {
(h), PARSE_OPT_NOARG }
#define OPT_SET_INT(s, l, v, h, i) { OPTION_SET_INT, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG, NULL, (i) }
+#define OPT_SET_INT_NONEG(s, l, v, h, i) \
+ { OPTION_SET_INT, (s), (l), (v), NULL,
\
+ (h), PARSE_OPT_NOARG |
PARSE_OPT_NONEG, \
+ NULL, (i) }
#define OPT_BOOL(s, l, v, h)OPT_SET_INT(s, l, v, h, 1)
#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG |
PARSE_OPT_HIDDEN, NULL, 1}
To avoid the proliferation of similar macros in future, I think we
should make a macro that takes any flags, e.g.
#define OPT_SET_INT_X(s, l, v, h, i, flags) { ., PARSE_OPT_NOARG
| PARSE_OPT_ ## flags, NULL, (i) }
and we can use it for NONEG like OPT_SET_INT_X(, NONEG). We
could even redefine OPT_SET_INT() to use OPT_SET_INT_X() to reduce
duplication.
I could do that, but it seems only the NONEG flag is used in the code.
While we're at NONEG, I see that builtin/grep.c has this construct {
OPTION_INTEGER...NONEG} and builtin/read-tree.c has {
OPTION_STRING..NONEG}. It would be great if you could look at them
and see if NONEG is really needed there, or simpler forms
OPT_INTEGER(...) and OPT_STRING(...) are enough.
I've grep'd through the source code, and most of the manually filled
option structures don't seems to have a pattern. And I think writing a
overly generalized macro won't help much.
You might need to read parse-options.c to understand these options.
Documentation/technical/api-parse-options.txt should give you a good
overview.
You could also think if we could transform { OPTION_CALLBACK }
to OPT_CALLBACK(...). But if you do and decide to do it, please make
it a separate patch (one patch deals with one thing).
That remaining of your patch looks good.
Thanks for reviewing my patch.
--
Duy
--
Regards
Yuxuan Shui
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
On Wed, Mar 12, 2014 at 10:43:54AM -0400, Quint Guvernator wrote:
memcmp() is replaced with negated starts_with() when comparing strings
from the beginning. starts_with() looks nicer and it saves the extra
argument of the length of the comparing string.
Thanks, I think this is a real readability improvement in most cases.
One of the things to do when reviewing a patch like this is make sure
that there aren't any silly arithmetic mistakes. Checking that can be
tedious, so I thought about how _I_ would do it programatically, and
then compared our results.
I tried:
perl -i -lpe '
s/memcmp\(([^,]+), (.*?), (\d+)\)/
length($2) == $3 ?
qq{!starts_with($1, $2)} :
$
/ge
' $(git ls-files '*.c')
That comes up with almost the same results as yours, but misses a few
cases that you caught (in addition to the need to simplify
!!starts_with()):
- memcmp(foo, bar, strlen(bar))
- strings with interpolation, like foo\n, which is actually 4
characters in length, not 5.
But there were only a few of those, and I hand-verified them. So I feel
pretty good that the changes are all correct transformations.
That leaves the question of whether they are all improvements in
readability (more on that below).
note: this commit properly handles negation in starts_with()
Signed-off-by: Quint Guvernator quintus.pub...@gmail.com
---
This note should go after the --- line so that it is not part of the
commit message (it is only interesting to people seeing v2 and wondering
what changed from v1 earlier on the list, not people reading the history
much later).
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 987a4c3..2777519 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -66,7 +66,7 @@ int cmd_commit_tree(int argc, const char **argv, const char
*prefix)
continue;
}
- if (!memcmp(arg, -S, 2)) {
+ if (starts_with(arg, -S)) {
sign_commit = arg + 2;
continue;
}
This is an improvement, but we still have the magic 2 below. Would
skip_prefix be a better match here, like:
if ((val = skip_prefix(arg, -S))) {
sign_commit = val;
continue;
}
We've also talked about refactoring skip_prefix to return a boolean, and
put the value in an out-parameter. Which would make this even more
readable:
if (skip_prefix(arg, -S, sign_commit))
continue;
Maybe the time has come to do that.
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -626,7 +626,7 @@ static int handle_boundary(void)
strbuf_addch(newline, '\n');
again:
if (line.len = (*content_top)-len + 2
- !memcmp(line.buf + (*content_top)-len, --, 2)) {
+ starts_with(line.buf + (*content_top)-len, --)) {
I'm not sure if this improves readability or not. It's certainly nice to
get rid of the magic 2, but starts_with is a bit of a misnomer, since
we are indexing deep into the buffer anyway. And we still have the magic
2 above anyway.
I'm on the fence.
@@ -727,8 +727,8 @@ static int is_scissors_line(const struct strbuf *line)
continue;
}
if (i + 1 len
- (!memcmp(buf + i, 8, 2) || !memcmp(buf + i, 8, 2) ||
- !memcmp(buf + i, %, 2) || !memcmp(buf + i, %, 2))) {
+ (starts_with(buf + i, 8) || starts_with(buf + i, 8) ||
+ starts_with(buf + i, %) || starts_with(buf + i, %)))
{
Same as above.
@@ -66,7 +66,7 @@ static int verify_tag(char *buffer, unsigned long size)
return error(char%PRIuMAX: could not find next \\\n\,
(uintmax_t) (type_line - buffer));
tag_line++;
- if (memcmp(tag_line, tag , 4) || tag_line[4] == '\n')
+ if (!starts_with(tag_line, tag ) || tag_line[4] == '\n')
return error(char%PRIuMAX: no \tag \ found,
(uintmax_t) (tag_line - buffer));
I think this is another that could benefit from an enhanced skip_prefix:
if (!skip_prefix(tag_line, tag , tag_name) || *tag_name == '\n')
...
and then we can get rid of the tag_line += 4 that is used much later
(in fact, I suspect that whole function could be improved in this
respect).
@@ -269,7 +269,7 @@ int parse_commit_buffer(struct commit *item, const void
*buffer, unsigned long s
return 0;
item-object.parsed = 1;
tail += size;
- if (tail = bufptr + 46 || memcmp(bufptr, tree , 5) || bufptr[45] !=
'\n')
+ if (tail = bufptr + 46 || !starts_with(bufptr, tree ) || bufptr[45]
!= '\n')
return error(bogus commit object %s,
sha1_to_hex(item-object.sha1));
if (get_sha1_hex(bufptr + 5, parent) 0)
Again, we just use bufptr + 5 a bit later here. So a candidate for
skip_prefix.
graft = lookup_commit_graft(item-object.sha1);
- while (bufptr + 48
Re: [PATCH] general style: replaces memcmp() with starts_with()
On Wed, Mar 12, 2014 at 06:22:24PM +0100, Jens Lehmann wrote: Let me know if you still think the hunk should be dropped there. Yes, I think so. That spot uses memcmp() because ce-name may not be 0-terminated. If that assumption isn't correct, it should be replaced with a plain strcmp() instead (while also dropping the ce_namelen() comparison in the line above). But starts_with() points into the wrong direction there. I think the length-check and memcmp is an optimization[1] here. But we should be able to encapsulate that pattern and avoid magic numbers entirely with something like mem_equals(). See my other response for more details. -Peff [1] Getting rid of the magic number entirely means we have to call strlen(.gitmodules), which seems like it is working against this optimization. But I think past experiments have shown that decent compilers will optimize strlen on a string literal to a constant, so as long as mem_equals is an inline, it should be equivalent. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote
Junio C Hamano gits...@pobox.com writes: Sorry for back-burnering this topic so long. I think the following does what you suggested in the message I am responding to. Now, hopefully the only thing we need is a documentation update and the series should be ready to go. ... and here it is, to be sanity checked before I queue the whole thing in 'next'. -- 8 -- Subject: [PATCH] request-pull: documentation updates The original description talked only about what it does. Instead, start it with the purpose of the command, i.e. what it is used for, and then mention what it does to achieve that goal. Clarify what start, url and end means in the context of the overall purpose of the command. Describe the extended syntax of end parameter that is used when the local branch name is different from the branch name at the repository the changes are published. Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/git-request-pull.txt | 55 +- 1 file changed, 49 insertions(+), 6 deletions(-) diff --git a/Documentation/git-request-pull.txt b/Documentation/git-request-pull.txt index b99681c..57bc1f5 100644 --- a/Documentation/git-request-pull.txt +++ b/Documentation/git-request-pull.txt @@ -13,22 +13,65 @@ SYNOPSIS DESCRIPTION --- -Summarizes the changes between two commits to the standard output, and includes -the given URL in the generated summary. +Prepare a request to your upstream project to pull your changes to +their tree to the standard output, by summarizing your changes and +showing where your changes can be pulled from. + +The upstream project is expected to have the commit named by +`start` and the output asks it to integrate the changes you made +since that commit, up to the commit named by `end`, by visiting +the repository named by `url`. + OPTIONS --- -p:: - Show patch text + Include patch text in the output. start:: - Commit to start at. + Commit to start at. This names a commit that is already in + the upstream history. url:: - URL to include in the summary. + The repository URL to be pulled from. end:: - Commit to end at; defaults to HEAD. + Commit to end at (defaults to HEAD). This names the commit + at the tip of the history you are asking to be pulled. ++ +When the repository named by `url` has the commit at a tip of a +ref that is different from the ref you have it locally, you can use +the `local:remote` syntax, to have its local name, a colon `:`, +and its remote name. + + +EXAMPLE +--- + +Imagine that you built your work on your `master` branch on top of +the `v1.0` release, and want it to be integrated to the project. +First you push that change to your public repository for others to +see: + + git push https://git.ko.xz/project master + +Then, you run this command: + + git request-pull v1.0 https://git.ko.xz/project master + +which will produce a request to the upstream, summarizing the +changes between the `v1.0` release and your `master`, to pull it +from your public repository. + +If you pushed your change to a branch whose name is different from +the one you have locally, e.g. + + git push https://git.ko.xz/project master:for-linus + +then you can ask that to be pulled with + + git request-pull v1.0 https://git.ko.xz/project master:for-linus + GIT --- -- 1.9.0-293-gd838d6f -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] GSoC micro project, use skip_prefix() in fsck_commit()
Yuxuan Shui (2): fsck.c: Change the type of fsck_ident()'s first argument fsck.c: Rewrite fsck_commit() to use skip_prefix() fsck.c | 26 ++ 1 file changed, 14 insertions(+), 12 deletions(-) -- 1.9.0 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] fsck.c: Change the type of fsck_ident()'s first argument
Since fsck_ident doesn't change the content of **ident, the type of
ident could be const char **.
This change is required to rewrite fsck_commit() to use skip_prefix().
---
fsck.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fsck.c b/fsck.c
index 99c0497..1789c34 100644
--- a/fsck.c
+++ b/fsck.c
@@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict,
fsck_error error_func)
return retval;
}
-static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)
+static int fsck_ident(const char **ident, struct object *obj, fsck_error
error_func)
{
if (**ident == '')
return error_func(obj, FSCK_ERROR, invalid author/committer
line - missing space before email);
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()
The purpose of skip_prefix() is much clearer than memcmp(). Also
skip_prefix() takes one less argument and its return value makes more
sense.
---
fsck.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/fsck.c b/fsck.c
index 1789c34..7e6b829 100644
--- a/fsck.c
+++ b/fsck.c
@@ -281,7 +281,7 @@ static int fsck_ident(const char **ident, struct object
*obj, fsck_error error_f
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- char *buffer = commit-buffer;
+ const char *buffer = commit-buffer, *tmp;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
@@ -290,15 +290,17 @@ static int fsck_commit(struct commit *commit, fsck_error
error_func)
if (commit-date == ULONG_MAX)
return error_func(commit-object, FSCK_ERROR, invalid
author/committer line);
- if (memcmp(buffer, tree , 5))
+ buffer = skip_prefix(buffer, tree );
+ if (buffer == NULL)
return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'tree' line);
- if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n')
+ if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
return error_func(commit-object, FSCK_ERROR, invalid 'tree'
line format - bad sha1);
- buffer += 46;
- while (!memcmp(buffer, parent , 7)) {
- if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
+ buffer += 41;
+ while ((tmp = skip_prefix(buffer, parent ))) {
+ buffer = tmp;
+ if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n')
return error_func(commit-object, FSCK_ERROR, invalid
'parent' line format - bad sha1);
- buffer += 48;
+ buffer += 41;
parents++;
}
graft = lookup_commit_graft(commit-object.sha1);
@@ -322,15 +324,15 @@ static int fsck_commit(struct commit *commit, fsck_error
error_func)
if (p || parents)
return error_func(commit-object, FSCK_ERROR, parent
objects missing);
}
- if (memcmp(buffer, author , 7))
+ buffer = skip_prefix(buffer, author );
+ if (buffer == NULL)
return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'author' line);
- buffer += 7;
err = fsck_ident(buffer, commit-object, error_func);
if (err)
return err;
- if (memcmp(buffer, committer , strlen(committer )))
+ buffer = skip_prefix(buffer, committer );
+ if (buffer == NULL)
return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'committer' line);
- buffer += strlen(committer );
err = fsck_ident(buffer, commit-object, error_func);
if (err)
return err;
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.
Duy Nguyen pclo...@gmail.com writes: By convention, no full stop in the subject line. The subject should summarize your changes and add ..NONEG is just one part of it. The other is convert to use ...NONEG. So I suggest parse-options: convert to use new macro OPT_SET_INT_NONEG() or something like that. You should also explain in the message body (before Signed-off-by:) why this is a good thing to do. My guess is better readability and harder to make mistakes in the future when you have to declare new options with noneg. As I said elsewhere, I am not sure if doubling the number of OPT_type macros as your micro suggestion proposes is the right solution to the problem. What are we trying to solve? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: An idea for git bisect and a GSoC enquiry
Jacopo Notarstefano jacopo.notarstef...@gmail.com writes: On Mon, Mar 3, 2014 at 7:34 PM, Junio C Hamano gits...@pobox.com wrote: I think you fundamentally cannot use two labels that are merely distinct and bisect correctly. You need to know which ones (i.e. good) are to be excluded and the other (i.e. bad) are to be included when computing the remaining to be tested set of commits. Good point. Yes, this isn't viable. But if you make them into --no-longer-X vs --still-X, then it will be viable without us knowing what X means. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #02; Tue, 11)
Duy Nguyen pclo...@gmail.com writes: On Wed, Mar 12, 2014 at 5:12 AM, Junio C Hamano gits...@pobox.com wrote: * nd/log-show-linear-break (2014-02-10) 1 commit - log: add --show-linear-break to help see non-linear history Attempts to show where a single-strand-of-pearls break in git log output. Will merge to 'next'. Hold this one. I haven't found time to check again if any rev-list combincation may break it. The option name is coule use some improvement too. OK. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.
Hi, On Thu, Mar 13, 2014 at 2:30 AM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: By convention, no full stop in the subject line. The subject should summarize your changes and add ..NONEG is just one part of it. The other is convert to use ...NONEG. So I suggest parse-options: convert to use new macro OPT_SET_INT_NONEG() or something like that. You should also explain in the message body (before Signed-off-by:) why this is a good thing to do. My guess is better readability and harder to make mistakes in the future when you have to declare new options with noneg. As I said elsewhere, I am not sure if doubling the number of OPT_type macros as your micro suggestion proposes is the right solution to the problem. What are we trying to solve? OK, I'll switch to another micro project. -- Regards Yuxuan Shui -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] status merge: guarentee space between msg and path
Duy Nguyen pclo...@gmail.com writes: On Wed, Mar 12, 2014 at 3:26 AM, Junio C Hamano gits...@pobox.com wrote: Sandy Carter sandy.car...@savoirfairelinux.com writes: 3651e45c takes the colon out of the control of the translators. That is a separate bug we would need to address, then. Duy Cc'ed. We went through this before http://thread.gmane.org/gmane.linux.debian.devel.bugs.general/1109239/focus=239560 If the colon needs language specific treatment then it should be part of the translatable strings. OK. So we should resurrect $gmane/239537 and adjust the codepath that was touched by 3651e45c to move the colon into translatable string? What other places do we assume that colons are to immediately follow whatever human-readable text used as a label/heading, I wonder... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/2] GSoC micro project, use skip_prefix() in fsck_commit()
I'm sorry for resending these patches, but the previous ones miss the sign-offs. Yuxuan Shui (2): fsck.c: Change the type of fsck_ident()'s first argument fsck.c: Rewrite fsck_commit() to use skip_prefix() fsck.c | 26 ++ 1 file changed, 14 insertions(+), 12 deletions(-) -- 1.9.0 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()
The purpose of skip_prefix() is much clearer than memcmp(). Also
skip_prefix() takes one less argument and its return value makes more
sense.
Signed-off-by: Yuxuan Shui yshu...@gmail.com
---
fsck.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/fsck.c b/fsck.c
index 1789c34..7e6b829 100644
--- a/fsck.c
+++ b/fsck.c
@@ -281,7 +281,7 @@ static int fsck_ident(const char **ident, struct object
*obj, fsck_error error_f
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- char *buffer = commit-buffer;
+ const char *buffer = commit-buffer, *tmp;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
@@ -290,15 +290,17 @@ static int fsck_commit(struct commit *commit, fsck_error
error_func)
if (commit-date == ULONG_MAX)
return error_func(commit-object, FSCK_ERROR, invalid
author/committer line);
- if (memcmp(buffer, tree , 5))
+ buffer = skip_prefix(buffer, tree );
+ if (buffer == NULL)
return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'tree' line);
- if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n')
+ if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
return error_func(commit-object, FSCK_ERROR, invalid 'tree'
line format - bad sha1);
- buffer += 46;
- while (!memcmp(buffer, parent , 7)) {
- if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
+ buffer += 41;
+ while ((tmp = skip_prefix(buffer, parent ))) {
+ buffer = tmp;
+ if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n')
return error_func(commit-object, FSCK_ERROR, invalid
'parent' line format - bad sha1);
- buffer += 48;
+ buffer += 41;
parents++;
}
graft = lookup_commit_graft(commit-object.sha1);
@@ -322,15 +324,15 @@ static int fsck_commit(struct commit *commit, fsck_error
error_func)
if (p || parents)
return error_func(commit-object, FSCK_ERROR, parent
objects missing);
}
- if (memcmp(buffer, author , 7))
+ buffer = skip_prefix(buffer, author );
+ if (buffer == NULL)
return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'author' line);
- buffer += 7;
err = fsck_ident(buffer, commit-object, error_func);
if (err)
return err;
- if (memcmp(buffer, committer , strlen(committer )))
+ buffer = skip_prefix(buffer, committer );
+ if (buffer == NULL)
return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'committer' line);
- buffer += strlen(committer );
err = fsck_ident(buffer, commit-object, error_func);
if (err)
return err;
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] fsck.c: Change the type of fsck_ident()'s first argument
Since fsck_ident doesn't change the content of **ident, the type of
ident could be const char **.
This change is required to rewrite fsck_commit() to use skip_prefix().
Signed-off-by: Yuxuan Shui yshu...@gmail.com
---
fsck.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fsck.c b/fsck.c
index 99c0497..1789c34 100644
--- a/fsck.c
+++ b/fsck.c
@@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict,
fsck_error error_func)
return retval;
}
-static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)
+static int fsck_ident(const char **ident, struct object *obj, fsck_error
error_func)
{
if (**ident == '')
return error_func(obj, FSCK_ERROR, invalid author/committer
line - missing space before email);
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git: problematic git status output with some translations (such as fr_FR)
Jonathan Nieder jrnie...@gmail.com writes:
@@ -292,6 +291,48 @@ static const char *wt_status_diff_status_string(int
status)
}
}
+static int maxwidth(const char *(*label)(int), int minval, int maxval)
+{
+ const char *s;
+ int result = 0, i;
+
+ for (i = minval; i = maxval; i++) {
+ const char *s = label(i);
+ int len = s ? strlen(s) : 0;
Shouldn't this be a utf8_strwidth(), as the value is to count number
of display columns to be used by the leading label part?
+ if (len result)
+ result = len;
+ }
+ return result;
+}
+
+static void wt_status_print_unmerged_data(struct wt_status *s,
+ struct string_list_item *it)
+{
+ const char *c = color(WT_STATUS_UNMERGED, s);
+ struct wt_status_change_data *d = it-util;
+ struct strbuf onebuf = STRBUF_INIT;
+ static char *padding;
+ const char *one, *how;
+ int len;
+
+ if (!padding) {
+ int width = maxwidth(wt_status_unmerged_status_string, 1, 7);
+ width += strlen( );
+ padding = xmallocz(width);
+ memset(padding, ' ', width);
+ }
+
+ one = quote_path(it-string, s-prefix, onebuf);
+ status_printf(s, color(WT_STATUS_HEADER, s), \t);
+
+ how = wt_status_unmerged_status_string(d-stagemask);
+ if (!how)
+ how = _(bug);
I'd rather see the callee do this _(bug) thing, not this
particular caller.
+ len = strlen(padding) - utf8_strwidth(how);
+ status_printf_more(s, c, %s%.*s%s\n, how, len, padding, one);
+ strbuf_release(onebuf);
+}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.
Yuxuan Shui yshu...@gmail.com writes: On Thu, Mar 13, 2014 at 2:30 AM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: By convention, no full stop in the subject line. The subject should summarize your changes and add ..NONEG is just one part of it. The other is convert to use ...NONEG. So I suggest parse-options: convert to use new macro OPT_SET_INT_NONEG() or something like that. You should also explain in the message body (before Signed-off-by:) why this is a good thing to do. My guess is better readability and harder to make mistakes in the future when you have to declare new options with noneg. As I said elsewhere, I am not sure if doubling the number of OPT_type macros as your micro suggestion proposes is the right solution to the problem. What are we trying to solve? OK, I'll switch to another micro project. That is fine, but note that it was not an objection but was a pure question. I was asking you to explain why this is a good change. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New GSoC microproject ideas
Here is another, as I seem to have managed to kill another one ;-) -- 8 -- VAR=VAL command is sufficient to run 'command' with environment variable VAR set to value VAL without affecting the environment of the shell itself, but we cannot do the same with a shell function (most notably, test_must_fail); we have subshell invocations with multiple lines like this: ... ( VAR=VAL export VAR test_must_fail git command ) ... but that could be expressed as ... test_must_fail env VAR=VAL git comand ... Find and shorten such constructs in existing test scripts. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New GSoC microproject ideas
Junio C Hamano gits...@pobox.com writes:
Here is another, as I seem to have managed to kill another one ;-)
-- 8 --
VAR=VAL command is sufficient to run 'command' with environment
variable VAR set to value VAL without affecting the environment of
the shell itself, but we cannot do the same with a shell function
(most notably, test_must_fail);
No? bash:
dak@lola:/usr/local/tmp/lilypond$ zippo()
{
echo $XXX
echo $XXX
}
dak@lola:/usr/local/tmp/lilypond$ XXX=8 zippo
8
8
dak@lola:/usr/local/tmp/lilypond$ /bin/dash
$ zippo()
{
echo $XXX
echo $XXX
}
$ XXX=8 zippo
8
8
$
dak@lola:/usr/local/tmp/lilypond$ /bin/ash
$ zippo()
{
echo $XXX
echo $XXX
}
$ XXX=8 zippo
8
8
$
Seems to work just fine with a set of typical shells.
--
David Kastrup
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New GSoC microproject ideas
On Wed, Mar 12, 2014 at 08:16:53PM +0100, David Kastrup wrote:
Junio C Hamano gits...@pobox.com writes:
Here is another, as I seem to have managed to kill another one ;-)
-- 8 --
VAR=VAL command is sufficient to run 'command' with environment
variable VAR set to value VAL without affecting the environment of
the shell itself, but we cannot do the same with a shell function
(most notably, test_must_fail);
No? bash:
dak@lola:/usr/local/tmp/lilypond$ zippo()
{
echo $XXX
echo $XXX
}
dak@lola:/usr/local/tmp/lilypond$ XXX=8 zippo
8
8
Try:
zippo() {
echo $XXX
}
XXX=8 zippo
zippo
XXX remains set after the first call under dash (but not bash). I
believe ash has the same behavior.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] wt-status: i18n of section labels
From: Jonathan Nieder jrnie...@gmail.com
Date: Thu, 19 Dec 11:43:19 2013 -0800
The original code assumes that:
(1) the number of bytes written is the width of a string, so they
can line up;
(2) the how string is always = 19 bytes.
Also a recent change to a similar codepath by 3651e45c (wt-status:
take the alignment burden off translators, 2013-11-05) adds this
assumption:
(3) the colon at the end of the label string does not have to be
subject to l10n.
None of which we should assume.
Refactor the code to compute the label width for both codepaths into
a helper function, make sure label strings have the trailing colon
that can be localized, and use it when showing the section labels in
the output.
cf. http://bugs.debian.org/725777
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
* Differences relative to Jonathan's original in $gmane/239537 are:
- labels made by wt_status_diff_status_string() have trailing
colon; the caller has been adjusted (please double check)
- a static label_width avoids repeated strlen(padding);
- unmerged status label has at least 20 columns wide
reinstated, at least for now, to keep the existing tests from
breaking. We may want to drop it in a separate follow-up
patch.
wt-status.c | 121 +++-
1 file changed, 78 insertions(+), 43 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 4e55810..a00861c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -245,51 +245,92 @@ static void wt_status_print_trailer(struct wt_status *s)
#define quote_path quote_path_relative
-static void wt_status_print_unmerged_data(struct wt_status *s,
- struct string_list_item *it)
+static const char *wt_status_unmerged_status_string(int stagemask)
{
- const char *c = color(WT_STATUS_UNMERGED, s);
- struct wt_status_change_data *d = it-util;
- struct strbuf onebuf = STRBUF_INIT;
- const char *one, *how = _(bug);
-
- one = quote_path(it-string, s-prefix, onebuf);
- status_printf(s, color(WT_STATUS_HEADER, s), \t);
- switch (d-stagemask) {
- case 1: how = _(both deleted:); break;
- case 2: how = _(added by us:); break;
- case 3: how = _(deleted by them:); break;
- case 4: how = _(added by them:); break;
- case 5: how = _(deleted by us:); break;
- case 6: how = _(both added:); break;
- case 7: how = _(both modified:); break;
+ switch (stagemask) {
+ case 1:
+ return _(both deleted:);
+ case 2:
+ return _(added by us:);
+ case 3:
+ return _(deleted by them:);
+ case 4:
+ return _(added by them:);
+ case 5:
+ return _(deleted by us:);
+ case 6:
+ return _(both added:);
+ case 7:
+ return _(both modified:);
+ default:
+ return _(bug);
}
- status_printf_more(s, c, %-20s%s\n, how, one);
- strbuf_release(onebuf);
}
static const char *wt_status_diff_status_string(int status)
{
switch (status) {
case DIFF_STATUS_ADDED:
- return _(new file);
+ return _(new file:);
case DIFF_STATUS_COPIED:
- return _(copied);
+ return _(copied:);
case DIFF_STATUS_DELETED:
- return _(deleted);
+ return _(deleted:);
case DIFF_STATUS_MODIFIED:
- return _(modified);
+ return _(modified:);
case DIFF_STATUS_RENAMED:
- return _(renamed);
+ return _(renamed:);
case DIFF_STATUS_TYPE_CHANGED:
- return _(typechange);
+ return _(typechange:);
case DIFF_STATUS_UNKNOWN:
- return _(unknown);
+ return _(unknown:);
case DIFF_STATUS_UNMERGED:
- return _(unmerged);
+ return _(unmerged:);
default:
- return NULL;
+ return _(bug);
+ }
+}
+
+static int maxwidth(const char *(*label)(int), int minval, int maxval)
+{
+ int result = 0, i;
+
+ for (i = minval; i = maxval; i++) {
+ const char *s = label(i);
+ int len = s ? utf8_strwidth(s) : 0;
+ if (len result)
+ result = len;
+ }
+ return result;
+}
+
+static void wt_status_print_unmerged_data(struct wt_status *s,
+ struct string_list_item *it)
+{
+ const char *c = color(WT_STATUS_UNMERGED, s);
+ struct wt_status_change_data *d = it-util;
+ struct strbuf onebuf = STRBUF_INIT;
+ static char *padding;
+ static int label_width;
+ const char *one, *how;
+ int len;
+
+ if (!padding) {
+ label_width =
Re: What's cooking in git.git (Mar 2014, #02; Tue, 11)
On Tue, Mar 11, 2014 at 03:12:11PM -0700, Junio C Hamano wrote: * jk/warn-on-object-refname-ambiguity (2014-01-09) 6 commits - get_sha1: drop object/refname ambiguity flag - get_sha1: speed up ambiguous 40-hex test - FIXUP: teach DO_FOR_EACH_NO_RECURSE to prime_ref_dir() - refs: teach for_each_ref a flag to avoid recursion - cat-file: fix a minor memory leak in batch_objects - cat-file: refactor error handling of batch_objects Expecting a reroll. I finally got a chance to return to this one. Michael had some good comments on the refactoring that was going on in the middle patches. He ended with: Yes. Still, the code is really piling up for this one warning for the contrived eventuality that somebody wants to pass SHA-1s and branch names together in a single cat-file invocation *and* wants to pass lots of inputs at once and so is worried about performance *and* has reference names that look like SHA-1s. Otherwise we could just leave the warning disabled in this case, as now. Or we could add a new --hashes-only option that tells cat-file to treat all of its arguments/inputs as SHA-1s; such an option would permit an even faster code path for bulk callers. Having looked at it again, I really think it is not worth pursuing. The end goal is not that interesting, there is a lot of code introduced, and a reasonable chance of accidentally introducing regressions and/or making the code less maintainable. Keeping the existing code (which just disables the check for cat-file) is probably the sanest course of action. We can do a similar thing for rev-list --stdin if we want, or we can wait until somebody complains. The bottom two patches are reasonable cleanups we should keep, though (and the rest can just be discarded). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Jeff King p...@peff.net writes:
static inline int standard_header_field(const char *field, size_t len)
{
-return ((len == 4 !memcmp(field, tree , 5)) ||
-(len == 6 !memcmp(field, parent , 7)) ||
-(len == 6 !memcmp(field, author , 7)) ||
-(len == 9 !memcmp(field, committer , 10)) ||
-(len == 8 !memcmp(field, encoding , 9)));
+return ((len == 4 starts_with(field, tree )) ||
+(len == 6 starts_with(field, parent )) ||
+(len == 6 starts_with(field, author )) ||
+(len == 9 starts_with(field, committer )) ||
+(len == 8 starts_with(field, encoding )));
These extra len checks are interesting. They look like an attempt to
optimize lookup, since the caller will already have scanned forward to
the space.
If one really wants to remove the magic constants from this, then
one must take advantage of the pattern
len == strlen(S) - 1 !memcmp(field, S, strlen(S))
that appears here, and come up with a simple abstraction to express
that we are only using the string S (e.g. tree ), length len and
location field of the counted string.
Blindly replacing starts_with() with !memcmp() in the above part is
a readability regression otherwise.
... I
think with a few more helpers we could really further clean up some of
these callsites.
Yes.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()
Yuxuan Shui yshu...@gmail.com writes:
The purpose of skip_prefix() is much clearer than memcmp(). Also
skip_prefix() takes one less argument and its return value makes
more sense.
Instead of justifying the change with a subjective-sounding and
vague much clearer and makes more sense, perhaps you can state
what the purpose is and why it makes more sense, no? We are using
memcmp() to see if the buffer starts with a known constant prefix
string and skip that prefix if it does so, skip_prefix() is a better
match or something?
Signed-off-by: Yuxuan Shui yshu...@gmail.com
---
fsck.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/fsck.c b/fsck.c
index 1789c34..7e6b829 100644
--- a/fsck.c
+++ b/fsck.c
@@ -281,7 +281,7 @@ static int fsck_ident(const char **ident, struct object
*obj, fsck_error error_f
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- char *buffer = commit-buffer;
+ const char *buffer = commit-buffer, *tmp;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
@@ -290,15 +290,17 @@ static int fsck_commit(struct commit *commit,
fsck_error error_func)
if (commit-date == ULONG_MAX)
return error_func(commit-object, FSCK_ERROR, invalid
author/committer line);
- if (memcmp(buffer, tree , 5))
+ buffer = skip_prefix(buffer, tree );
+ if (buffer == NULL)
if (!buffer)
return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'tree' line);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
On Wed, Mar 12, 2014 at 12:39:01PM -0700, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
static inline int standard_header_field(const char *field, size_t len)
{
- return ((len == 4 !memcmp(field, tree , 5)) ||
- (len == 6 !memcmp(field, parent , 7)) ||
- (len == 6 !memcmp(field, author , 7)) ||
- (len == 9 !memcmp(field, committer , 10)) ||
- (len == 8 !memcmp(field, encoding , 9)));
+ return ((len == 4 starts_with(field, tree )) ||
+ (len == 6 starts_with(field, parent )) ||
+ (len == 6 starts_with(field, author )) ||
+ (len == 9 starts_with(field, committer )) ||
+ (len == 8 starts_with(field, encoding )));
These extra len checks are interesting. They look like an attempt to
optimize lookup, since the caller will already have scanned forward to
the space.
If one really wants to remove the magic constants from this, then
one must take advantage of the pattern
len == strlen(S) - 1 !memcmp(field, S, strlen(S))
that appears here, and come up with a simple abstraction to express
that we are only using the string S (e.g. tree ), length len and
location field of the counted string.
Blindly replacing starts_with() with !memcmp() in the above part is
a readability regression otherwise.
I actually think the right solution is:
static inline int standard_header_field(const char *field, size_t len)
{
return mem_equals(field, len, tree ) ||
mem_equals(field, len, parent ) ||
...;
}
and the caller should tell us it's OK to look at field[len]:
standard_header_field(line, eof - line + 1)
We could also omit the space from the standard_header_field. The caller
just ran strchr() looking for the space, so we know that either it is
there, or we are at the end of the line/buffer. Arguably a string like
parent\n should be a parent header with no data (but right now it is
not matched by this function). I'm not aware of an implementation that
writes such a thing, but it seems to fall in the be liberal in what you
accept category.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #02; Tue, 11)
Jeff King p...@peff.net writes: On Tue, Mar 11, 2014 at 03:12:11PM -0700, Junio C Hamano wrote: * jk/warn-on-object-refname-ambiguity (2014-01-09) 6 commits - get_sha1: drop object/refname ambiguity flag - get_sha1: speed up ambiguous 40-hex test - FIXUP: teach DO_FOR_EACH_NO_RECURSE to prime_ref_dir() - refs: teach for_each_ref a flag to avoid recursion - cat-file: fix a minor memory leak in batch_objects - cat-file: refactor error handling of batch_objects Expecting a reroll. I finally got a chance to return to this one. Michael had some good comments on the refactoring that was going on in the middle patches. He ended with: Yes. Still, the code is really piling up for this one warning for the contrived eventuality that somebody wants to pass SHA-1s and branch names together in a single cat-file invocation *and* wants to pass lots of inputs at once and so is worried about performance *and* has reference names that look like SHA-1s. Otherwise we could just leave the warning disabled in this case, as now. Or we could add a new --hashes-only option that tells cat-file to treat all of its arguments/inputs as SHA-1s; such an option would permit an even faster code path for bulk callers. Having looked at it again, I really think it is not worth pursuing. The end goal is not that interesting, there is a lot of code introduced, and a reasonable chance of accidentally introducing regressions and/or making the code less maintainable. Keeping the existing code (which just disables the check for cat-file) is probably the sanest course of action. We can do a similar thing for rev-list --stdin if we want, or we can wait until somebody complains. The bottom two patches are reasonable cleanups we should keep, though (and the rest can just be discarded). Fine, let's do that. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] submodule: add verbose mode for add/update
On Wed, Mar 12, 2014 at 6:15 PM, Jens Lehmann jens.lehm...@web.de wrote:
Am 12.03.2014 14:42, schrieb Orgad Shaneh:
From: Orgad Shaneh org...@gmail.com
You don't need the line above when you are the sender ;-)
Executes checkout without -q
That's a bit terse. What about:
Add the verbose flag to add and update which displays the
progress of the actual submodule checkout when given. This
is useful for checkouts that take a long time, as the user
can then see the progress.
Signed-off-by: Orgad Shaneh org...@gmail.com
---
Documentation/git-submodule.txt | 7 +--
git-submodule.sh| 24 +++-
t/t7406-submodule-update.sh | 9 +
3 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-submodule.txt
b/Documentation/git-submodule.txt
index 21cb59a..1867e94 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -10,13 +10,13 @@ SYNOPSIS
[verse]
'git submodule' [--quiet] add [-b branch] [-f|--force] [--name name]
- [--reference repository] [--depth depth] [--] repository
[path]
+ [--reference repository] [--depth depth] [-v|--verbose]
[--] repository [path]
'git submodule' [--quiet] status [--cached] [--recursive] [--] [path...]
'git submodule' [--quiet] init [--] [path...]
'git submodule' [--quiet] deinit [-f|--force] [--] path...
'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase|--merge|--checkout] [--reference
repository]
- [--depth depth] [--recursive] [--] [path...]
+ [--depth depth] [--recursive] [-v|--verbose] [--] [path...]
'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit)
n]
[commit] [--] [path...]
'git submodule' [--quiet] foreach [--recursive] command
@@ -363,6 +363,9 @@ for linkgit:git-clone[1]'s `--reference` and `--shared`
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
+--verbose::
+ This option is valid for add and update commands. Show output of
+ checkout.
The above looks whitespace-damaged, you should use TABs here to
indent (just like the other options do).
path...::
Paths to submodule(s). When specified this will restrict the command
diff --git a/git-submodule.sh b/git-submodule.sh
index a33f68d..5c4e057 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,11 +5,11 @@
# Copyright (c) 2007 Lars Hjemli
dashless=$(basename $0 | sed -e 's/-/ /')
-USAGE=[--quiet] add [-b branch] [-f|--force] [--name name]
[--reference repository] [--] repository [path]
+USAGE=[--quiet] add [-b branch] [-f|--force] [--name name]
[--reference repository] [-v|--verbose] [--] repository [path]
or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
or: $dashless [--quiet] init [--] [path...]
or: $dashless [--quiet] deinit [-f|--force] [--] path...
- or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge] [--recursive]
[--] [path...]
+ or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge] [--recursive]
[-v|--verbose] [--] [path...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit
n] [commit] [--] [path...]
or: $dashless [--quiet] foreach [--recursive] command
or: $dashless [--quiet] sync [--recursive] [--] [path...]
@@ -319,12 +319,16 @@ module_clone()
rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
(
clear_local_git_env
+ if test -z $verbose
+ then
+ subquiet=-q
+ fi
cd $sm_path
GIT_WORK_TREE=. git config core.worktree $rel/$b
# ash fails to wordsplit ${local_branch:+-B
$local_branch...}
case $local_branch in
- '') git checkout -f -q ${start_point:+$start_point} ;;
- ?*) git checkout -f -q -B $local_branch
${start_point:+$start_point} ;;
+ '') git checkout -f $subquiet ${start_point:+$start_point}
;;
+ ?*) git checkout -f $subquiet -B $local_branch
${start_point:+$start_point} ;;
Wouldn't it be better to use the ${subquiet:+$subquiet} notation
here like the other optional arguments do? After all the subquiet
isn't always set.
esac
) || die $(eval_gettext Unable to setup cloned submodule
'\$sm_path')
}
@@ -380,6 +384,9 @@ cmd_add()
--depth=*)
depth=$1
;;
+ -v|--verbose)
+ verbose=1
+ ;;
--)
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Jeff King p...@peff.net writes:
Blindly replacing starts_with() with !memcmp() in the above part is
a readability regression otherwise.
I actually think the right solution is:
static inline int standard_header_field(const char *field, size_t len)
{
return mem_equals(field, len, tree ) ||
mem_equals(field, len, parent ) ||
...;
}
and the caller should tell us it's OK to look at field[len]:
standard_header_field(line, eof - line + 1)
We could also omit the space from the standard_header_field.
Yes, that was what I had in mind. The only reason why the callee
(over-)optimizes the SP must follow these know keywords part by
using the extra len parameter is because the caller has to do a
single strchr() to skip an arbitrary field name anyway so computing
len is essentially free.
The caller
just ran strchr() looking for the space, so we know that either it is
there, or we are at the end of the line/buffer. Arguably a string like
parent\n should be a parent header with no data (but right now it is
not matched by this function). I'm not aware of an implementation that
writes such a thing, but it seems to fall in the be liberal in what you
accept category.
It is _not_ a standard header field, so it will be read by the logic
in the caller as a non-standard header field without getting lost.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] wt-status: i18n of section labels
Sandy Carter sandy.car...@savoirfairelinux.com writes:
Le 2014-03-12 15:22, Junio C Hamano a écrit :
static const char *wt_status_diff_status_string(int status)
{
switch (status) {
case DIFF_STATUS_ADDED:
-return _(new file);
+return _(new file:);
case DIFF_STATUS_COPIED:
-return _(copied);
+return _(copied:);
case DIFF_STATUS_DELETED:
-return _(deleted);
+return _(deleted:);
case DIFF_STATUS_MODIFIED:
-return _(modified);
+return _(modified:);
case DIFF_STATUS_RENAMED:
-return _(renamed);
+return _(renamed:);
case DIFF_STATUS_TYPE_CHANGED:
-return _(typechange);
+return _(typechange:);
case DIFF_STATUS_UNKNOWN:
-return _(unknown);
+return _(unknown:);
case DIFF_STATUS_UNMERGED:
-return _(unmerged);
+return _(unmerged:);
default:
-return NULL;
+return _(bug);
+}
+}
I don't see why _(bug) is returned when, later down,
When there is a bug in the caller.
@@ -305,21 +346,16 @@ static void wt_status_print_change_data(struct
wt_status *s,
struct strbuf onebuf = STRBUF_INIT, twobuf = STRBUF_INIT;
struct strbuf extra = STRBUF_INIT;
static char *padding;
+static int label_width;
const char *what;
int len;
if (!padding) {
-int width = 0;
-/* If DIFF_STATUS_* uses outside this range, we're in trouble */
-for (status = 'A'; status = 'Z'; status++) {
-what = wt_status_diff_status_string(status);
-len = what ? strlen(what) : 0;
checks for NULL.
That extra NULL-ness check can go, I think. Thanks for
double-checking.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] wt-status: i18n of section labels
Le 2014-03-12 16:12, Junio C Hamano a écrit :
Sandy Carter sandy.car...@savoirfairelinux.com writes:
Le 2014-03-12 15:22, Junio C Hamano a écrit :
static const char *wt_status_diff_status_string(int status)
{
switch (status) {
case DIFF_STATUS_ADDED:
- return _(new file);
+ return _(new file:);
case DIFF_STATUS_COPIED:
- return _(copied);
+ return _(copied:);
case DIFF_STATUS_DELETED:
- return _(deleted);
+ return _(deleted:);
case DIFF_STATUS_MODIFIED:
- return _(modified);
+ return _(modified:);
case DIFF_STATUS_RENAMED:
- return _(renamed);
+ return _(renamed:);
case DIFF_STATUS_TYPE_CHANGED:
- return _(typechange);
+ return _(typechange:);
case DIFF_STATUS_UNKNOWN:
- return _(unknown);
+ return _(unknown:);
case DIFF_STATUS_UNMERGED:
- return _(unmerged);
+ return _(unmerged:);
default:
- return NULL;
+ return _(bug);
+ }
+}
I don't see why _(bug) is returned when, later down,
When there is a bug in the caller.
@@ -305,21 +346,16 @@ static void wt_status_print_change_data(struct wt_status
*s,
struct strbuf onebuf = STRBUF_INIT, twobuf = STRBUF_INIT;
struct strbuf extra = STRBUF_INIT;
static char *padding;
+ static int label_width;
const char *what;
int len;
if (!padding) {
- int width = 0;
- /* If DIFF_STATUS_* uses outside this range, we're in trouble */
- for (status = 'A'; status = 'Z'; status++) {
- what = wt_status_diff_status_string(status);
- len = what ? strlen(what) : 0;
checks for NULL.
That extra NULL-ness check can go, I think. Thanks for
double-checking.
I refered to the wrong lines, the ones I was refering to were:
+static int maxwidth(const char *(*label)(int), int minval, int maxval)
+{
+ int result = 0, i;
+
+ for (i = minval; i = maxval; i++) {
+ const char *s = label(i);
+ int len = s ? utf8_strwidth(s) : 0;
Sorry about that
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] submodule: add verbose mode for add/update
Orgad Shaneh org...@gmail.com writes:
+--verbose::
+ This option is valid for add and update commands. Display the progress
+ of the actual submodule checkout.
Hmm, is the valid for add and update part we want to keep? I do
not think it is a crime if some other subcommand accepted --verbose
option but its output under verbose mode and normal mode happened to
be the same.
I doubt it would take a lot of imagination to see that people would
want to see git submodule status --verbose to get richer output,
and at that point, progress of checkout as part of the description
of the --verbose option does not make any sense. Perhaps the
second part that is specific to add and update subcommands
should move to the description of these two subcommands?
I dunno.
diff --git a/git-submodule.sh b/git-submodule.sh
index a33f68d..e1df2c8 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,11 +5,11 @@
# Copyright (c) 2007 Lars Hjemli
dashless=$(basename $0 | sed -e 's/-/ /')
-USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference
repository] [--] repository [path]
+USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference
repository] [-v|--verbose] [--] repository [path]
or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
or: $dashless [--quiet] init [--] [path...]
or: $dashless [--quiet] deinit [-f|--force] [--] path...
- or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge] [--recursive]
[--] [path...]
+ or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge] [--recursive]
[-v|--verbose] [--] [path...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit n]
[commit] [--] [path...]
or: $dashless [--quiet] foreach [--recursive] command
or: $dashless [--quiet] sync [--recursive] [--] [path...]
@@ -319,12 +319,16 @@ module_clone()
rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
(
clear_local_git_env
+ if test -z $verbose
+ then
+ subquiet=-q
+ fi
cd $sm_path
GIT_WORK_TREE=. git config core.worktree $rel/$b
# ash fails to wordsplit ${local_branch:+-B $local_branch...}
case $local_branch in
- '') git checkout -f -q ${start_point:+$start_point} ;;
- ?*) git checkout -f -q -B $local_branch
${start_point:+$start_point} ;;
+ '') git checkout -f ${subquiet:+$subquiet}
${start_point:+$start_point} ;;
+ ?*) git checkout -f ${subquiet:+$subquiet} -B $local_branch
${start_point:+$start_point} ;;
esac
) || die $(eval_gettext Unable to setup cloned submodule
'\$sm_path')
}
@@ -380,6 +384,9 @@ cmd_add()
--depth=*)
depth=$1
;;
+ -v|--verbose)
+ verbose=1
+ ;;
Compare $depth and $verbose, and think what would happen if the end
user had an environment variable whose name happened to be $depth or
$verbose. Does this script misbehave under such a stray $verbose?
What does the existing script do to prevent it from misbehaving when
a stray $depth exists in the environment?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] fsck.c: Change the type of fsck_ident()'s first argument
On Thu, Mar 13, 2014 at 02:51:29AM +0800, Yuxuan Shui wrote:
Since fsck_ident doesn't change the content of **ident, the type of
ident could be const char **.
Unfortunately, const double-pointers in C are a bit tricky, and a
pointer to char * cannot automatically be passed as a pointer to
const char *.
I think you want this on top:
diff --git a/fsck.c b/fsck.c
index 1789c34..7776660 100644
--- a/fsck.c
+++ b/fsck.c
@@ -281,7 +281,7 @@ static int fsck_ident(const char **ident, struct object
*obj, fsck_error error_f
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- char *buffer = commit-buffer;
+ const char *buffer = commit-buffer;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
Otherwise, gcc will complain about incompatible pointer types.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New GSoC microproject ideas
Jeff King p...@peff.net writes:
On Wed, Mar 12, 2014 at 08:16:53PM +0100, David Kastrup wrote:
Junio C Hamano gits...@pobox.com writes:
Here is another, as I seem to have managed to kill another one ;-)
-- 8 --
VAR=VAL command is sufficient to run 'command' with environment
variable VAR set to value VAL without affecting the environment of
the shell itself, but we cannot do the same with a shell function
(most notably, test_must_fail);
No? bash:
dak@lola:/usr/local/tmp/lilypond$ zippo()
{
echo $XXX
echo $XXX
}
dak@lola:/usr/local/tmp/lilypond$ XXX=8 zippo
8
8
Try:
zippo() {
echo $XXX
}
XXX=8 zippo
zippo
XXX remains set after the first call under dash (but not bash). I
believe ash has the same behavior.
Yes. I would lean towards considering this a bug. But I agree that it
does not help.
--
David Kastrup
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] status merge: guarentee space between msg and path
Sandy Carter sandy.car...@savoirfairelinux.com writes: Seems fine except for the bit about returning _(bug), which I brought up. Seems to do the same thing as my proposal without changing the alignment of paths in of regular status output. No changes to tests necessary, less noisy. It works for me. Thanks. I'll work on a better split, then, and resend them later. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New GSoC microproject ideas
On Wed, Mar 12, 2014 at 09:37:41PM +0100, David Kastrup wrote:
Try:
zippo() {
echo $XXX
}
XXX=8 zippo
zippo
XXX remains set after the first call under dash (but not bash). I
believe ash has the same behavior.
Yes. I would lean towards considering this a bug. But I agree that it
does not help.
Dash's behavior is POSIX (and bash --posix behaves the same way).
http://article.gmane.org/gmane.comp.version-control.git/137095
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Junio C Hamano gits...@pobox.com writes:
Jeff King p...@peff.net writes:
static inline int standard_header_field(const char *field, size_t len)
{
- return ((len == 4 !memcmp(field, tree , 5)) ||
- (len == 6 !memcmp(field, parent , 7)) ||
- (len == 6 !memcmp(field, author , 7)) ||
- (len == 9 !memcmp(field, committer , 10)) ||
- (len == 8 !memcmp(field, encoding , 9)));
+ return ((len == 4 starts_with(field, tree )) ||
+ (len == 6 starts_with(field, parent )) ||
+ (len == 6 starts_with(field, author )) ||
+ (len == 9 starts_with(field, committer )) ||
+ (len == 8 starts_with(field, encoding )));
These extra len checks are interesting. They look like an attempt to
optimize lookup, since the caller will already have scanned forward to
the space.
If one really wants to remove the magic constants from this, then
one must take advantage of the pattern
len == strlen(S) - 1 !memcmp(field, S, strlen(S))
If the caller has already scanned forward to the space, then there is no
actual need to let the comparison compare the space again after checking
len, is there? That makes for a more consistent look in the memcmp
case.
One could do this in a switch on len instead. Not that it seems
terribly more efficient.
that appears here, and come up with a simple abstraction to express
that we are only using the string S (e.g. tree ), length len and
location field of the counted string.
Blindly replacing starts_with() with !memcmp() in the above part is
a readability regression otherwise.
Don't really think so: if the len at the front and the back is the same
and the space is not explicitly compared any more, both look pretty much
the same to me.
... I think with a few more helpers we could really further clean up
some of these callsites.
Yes.
Possibly. But it does seem like overkill.
--
David Kastrup
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Am 12.03.2014 20:39, schrieb Junio C Hamano:
Jeff King p...@peff.net writes:
static inline int standard_header_field(const char *field, size_t len)
{
- return ((len == 4 !memcmp(field, tree , 5)) ||
- (len == 6 !memcmp(field, parent , 7)) ||
- (len == 6 !memcmp(field, author , 7)) ||
- (len == 9 !memcmp(field, committer , 10)) ||
- (len == 8 !memcmp(field, encoding , 9)));
+ return ((len == 4 starts_with(field, tree )) ||
+ (len == 6 starts_with(field, parent )) ||
+ (len == 6 starts_with(field, author )) ||
+ (len == 9 starts_with(field, committer )) ||
+ (len == 8 starts_with(field, encoding )));
These extra len checks are interesting. They look like an attempt to
optimize lookup, since the caller will already have scanned forward to
the space.
I wonder what the performance impact might be. The length checks are
also needed for correctness, however, to avoid running over the end of
the buffer.
If one really wants to remove the magic constants from this, then
one must take advantage of the pattern
len == strlen(S) - 1 !memcmp(field, S, strlen(S))
that appears here, and come up with a simple abstraction to express
that we are only using the string S (e.g. tree ), length len and
location field of the counted string.
This might be a job for kwset.
René
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New GSoC microproject ideas
Jeff King p...@peff.net writes:
On Wed, Mar 12, 2014 at 09:37:41PM +0100, David Kastrup wrote:
Try:
zippo() {
echo $XXX
}
XXX=8 zippo
zippo
XXX remains set after the first call under dash (but not bash). I
believe ash has the same behavior.
Yes. I would lean towards considering this a bug. But I agree that it
does not help.
Dash's behavior is POSIX (and bash --posix behaves the same way).
http://article.gmane.org/gmane.comp.version-control.git/137095
In that case I consider it a standard-compliant bug (namely being a
serious problem regarding the usefulness of shell functions). Which
makes it unlikely to go away. It makes it easier to interpret, say
zippo() {
XXX=$XXX
}
XXX=8 zippo
echo $XXX
as shell functions presumably should be able to assign to shell
variables like built-ins do. But that's not really much of an
advantage.
The behavior does not make sense to me also with regard to special
built-ins. Bash does
dak@lola:/usr/local/tmp/git$ XXX=8 :
dak@lola:/usr/local/tmp/git$ echo $XXX
dak@lola:/usr/local/tmp/git$
And that makes sense to me. Whatever, does not help.
--
David Kastrup
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
On Wed, Mar 12, 2014 at 01:08:03PM -0700, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
Blindly replacing starts_with() with !memcmp() in the above part is
a readability regression otherwise.
I actually think the right solution is:
static inline int standard_header_field(const char *field, size_t len)
{
return mem_equals(field, len, tree ) ||
mem_equals(field, len, parent ) ||
...;
}
and the caller should tell us it's OK to look at field[len]:
standard_header_field(line, eof - line + 1)
We could also omit the space from the standard_header_field.
Yes, that was what I had in mind. The only reason why the callee
(over-)optimizes the SP must follow these know keywords part by
using the extra len parameter is because the caller has to do a
single strchr() to skip an arbitrary field name anyway so computing
len is essentially free.
One thing that bugs me about the current code is that the sub-function
looks one past the end of the length given to it by the caller.
Switching it to pass eof - line + 1 resolves that, but is it right?
The character pointed at by eof is either:
1. space, if our strchr(line, ' ') found something
2. the first character of the next line, if our
memchr(line, '\n', eob - line) found something
3. Whatever is at eob (end-of-buffer)
There are two questionable things here. In (1), we use strchr on a
sized buffer. And in (3), we look one past the size that was passed in.
In both cases, we are saved by the fact that the buffer is actually NUL
terminated at the end of size (because it comes from read_sha1_file).
But we may find a space much further than the line ending which is
supposed to be our boundary, and end up having to do a comparison to
cover this case.
So I think the current code is correct, but we could make it more
obvious by:
1. Restricting our search for the field separator to the current line.
2. Explicitly avoid looking for headers when we did not find a space,
since we cannot match anything anyway.
Like:
diff --git a/commit.c b/commit.c
index 6bf4fe0..9383cc1 100644
--- a/commit.c
+++ b/commit.c
@@ -1325,14 +1325,14 @@ static struct commit_extra_header
*read_commit_extra_header_lines(
strbuf_reset(buf);
it = NULL;
- eof = strchr(line, ' ');
- if (next = eof)
+ eof = memchr(line, ' ', next - line);
+ if (eof) {
+ if (standard_header_field(line, eof - line + 1) ||
+ excluded_header_field(line, eof - line, exclude))
+ continue;
+ } else
eof = next;
- if (standard_header_field(line, eof - line) ||
- excluded_header_field(line, eof - line, exclude))
- continue;
-
it = xcalloc(1, sizeof(*it));
it-key = xmemdupz(line, eof-line);
*tail = it;
I also think that eof = next (which I retained here) is off-by-one.
next here is not the newline, but the start of the next line. And I'm
guessing the code actually wanted the newline (otherwise it-key ends
up with the newline in it). But we cannot just subtract one, because if
we hit eob, it really is in the right spot.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/4] status: allow label strings to be translated
So here is my attempt to clean-up what Jonathan posted in $gmane/239537 as how about this? patch. The first one (full label string) fixes up 3651e45c (wt-status: take the alignment burden off translators, 2013-11-05) to include colon back to translatable string again, while retaining its label alignment logic. The second (extract the code) is taken from Jonathan's $gmane/239537 as a separate patch. The third is essentially the remainder of Jonathan's $gmane/239537, with one small fix s/strlen/utf8_width/; to teach the code that shows unmerged paths the same label alignment logic Duy added in 3651e45c for the tracked paths, while retaining the at least 20 columns floor to avoid the churn to the tests. And the last lifts the at least 20 columns floor. Jonathan Nieder (2): wt-status: extract the code to compute width for labels wt-status: i18n of section labels Junio C Hamano (2): wt-status: make full label string to be subject to l10n wt-status: lift the artificual at least 20 columns floor t/t7060-wtstatus.sh| 14 +++--- t/t7512-status-help.sh | 12 ++--- wt-status.c| 117 +++-- 3 files changed, 88 insertions(+), 55 deletions(-) -- 1.9.0-293-gd838d6f -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/4] wt-status: make full label string to be subject to l10n
Earlier in 3651e45c (wt-status: take the alignment burden off
translators, 2013-11-05), we assumed that it is OK to make the
string before the colon in a label string we give as the section
header of various kinds of changes (e.g. new file:) translatable.
This assumption apparently does not hold for some languages,
e.g. ones that want to have spaces around the colon.
Also introduce a static label_width to avoid having to run
strlen(padding) over and over.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
wt-status.c | 35 +--
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 4e55810..9cf7028 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -272,21 +272,21 @@ static const char *wt_status_diff_status_string(int
status)
{
switch (status) {
case DIFF_STATUS_ADDED:
- return _(new file);
+ return _(new file:);
case DIFF_STATUS_COPIED:
- return _(copied);
+ return _(copied:);
case DIFF_STATUS_DELETED:
- return _(deleted);
+ return _(deleted:);
case DIFF_STATUS_MODIFIED:
- return _(modified);
+ return _(modified:);
case DIFF_STATUS_RENAMED:
- return _(renamed);
+ return _(renamed:);
case DIFF_STATUS_TYPE_CHANGED:
- return _(typechange);
+ return _(typechange:);
case DIFF_STATUS_UNKNOWN:
- return _(unknown);
+ return _(unknown:);
case DIFF_STATUS_UNMERGED:
- return _(unmerged);
+ return _(unmerged:);
default:
return NULL;
}
@@ -305,21 +305,21 @@ static void wt_status_print_change_data(struct wt_status
*s,
struct strbuf onebuf = STRBUF_INIT, twobuf = STRBUF_INIT;
struct strbuf extra = STRBUF_INIT;
static char *padding;
+ static int label_width;
const char *what;
int len;
if (!padding) {
- int width = 0;
/* If DIFF_STATUS_* uses outside this range, we're in trouble */
for (status = 'A'; status = 'Z'; status++) {
what = wt_status_diff_status_string(status);
len = what ? strlen(what) : 0;
- if (len width)
- width = len;
+ if (len label_width)
+ label_width = len;
}
- width += 2; /* colon and a space */
- padding = xmallocz(width);
- memset(padding, ' ', width);
+ label_width += strlen( );
+ padding = xmallocz(label_width);
+ memset(padding, ' ', label_width);
}
one_name = two_name = it-string;
@@ -355,14 +355,13 @@ static void wt_status_print_change_data(struct wt_status
*s,
what = wt_status_diff_status_string(status);
if (!what)
die(_(bug: unhandled diff status %c), status);
- /* 1 for colon, which is not part of what */
- len = strlen(padding) - (utf8_strwidth(what) + 1);
+ len = label_width - utf8_strwidth(what);
assert(len = 0);
if (status == DIFF_STATUS_COPIED || status == DIFF_STATUS_RENAMED)
- status_printf_more(s, c, %s:%.*s%s - %s,
+ status_printf_more(s, c, %s%.*s%s - %s,
what, len, padding, one, two);
else
- status_printf_more(s, c, %s:%.*s%s,
+ status_printf_more(s, c, %s%.*s%s,
what, len, padding, one);
if (extra.len) {
status_printf_more(s, color(WT_STATUS_HEADER, s), %s,
extra.buf);
--
1.9.0-293-gd838d6f
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/4] wt-status: i18n of section labels
From: Jonathan Nieder jrnie...@gmail.com
From: Jonathan Nieder jrnie...@gmail.com
Date: Thu, 19 Dec 2013 11:43:19 -0800
The original code assumes that:
(1) the number of bytes written is the width of a string, so they
can line up;
(2) the how string is always = 19 bytes.
Neither of which we should assume.
Using the same approach as the earlier 3651e45c (wt-status: take the
alignment burden off translators, 2013-11-05), compute the necessary
column width to hold the longest label and use that for alignment.
cf. http://bugs.debian.org/725777
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Helped-by: Sandy Carter
Signed-off-by: Junio C Hamano gits...@pobox.com
---
wt-status.c | 66 +++--
1 file changed, 47 insertions(+), 19 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index db98c52..b1b018e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -245,27 +245,26 @@ static void wt_status_print_trailer(struct wt_status *s)
#define quote_path quote_path_relative
-static void wt_status_print_unmerged_data(struct wt_status *s,
- struct string_list_item *it)
+static const char *wt_status_unmerged_status_string(int stagemask)
{
- const char *c = color(WT_STATUS_UNMERGED, s);
- struct wt_status_change_data *d = it-util;
- struct strbuf onebuf = STRBUF_INIT;
- const char *one, *how = _(bug);
-
- one = quote_path(it-string, s-prefix, onebuf);
- status_printf(s, color(WT_STATUS_HEADER, s), \t);
- switch (d-stagemask) {
- case 1: how = _(both deleted:); break;
- case 2: how = _(added by us:); break;
- case 3: how = _(deleted by them:); break;
- case 4: how = _(added by them:); break;
- case 5: how = _(deleted by us:); break;
- case 6: how = _(both added:); break;
- case 7: how = _(both modified:); break;
+ switch (stagemask) {
+ case 1:
+ return _(both deleted:);
+ case 2:
+ return _(added by us:);
+ case 3:
+ return _(deleted by them:);
+ case 4:
+ return _(added by them:);
+ case 5:
+ return _(deleted by us:);
+ case 6:
+ return _(both added:);
+ case 7:
+ return _(both modified:);
+ default:
+ die(_(bug: unhandled unmerged status %x), stagemask);
}
- status_printf_more(s, c, %-20s%s\n, how, one);
- strbuf_release(onebuf);
}
static const char *wt_status_diff_status_string(int status)
@@ -305,6 +304,35 @@ static int maxwidth(const char *(*label)(int), int minval,
int maxval)
return result;
}
+static void wt_status_print_unmerged_data(struct wt_status *s,
+ struct string_list_item *it)
+{
+ const char *c = color(WT_STATUS_UNMERGED, s);
+ struct wt_status_change_data *d = it-util;
+ struct strbuf onebuf = STRBUF_INIT;
+ static char *padding;
+ static int label_width;
+ const char *one, *how;
+ int len;
+
+ if (!padding) {
+ label_width = maxwidth(wt_status_unmerged_status_string, 1, 7);
+ label_width += strlen( );
+ if (label_width 20)
+ label_width = 20;
+ padding = xmallocz(label_width);
+ memset(padding, ' ', label_width);
+ }
+
+ one = quote_path(it-string, s-prefix, onebuf);
+ status_printf(s, color(WT_STATUS_HEADER, s), \t);
+
+ how = wt_status_unmerged_status_string(d-stagemask);
+ len = label_width - utf8_strwidth(how);
+ status_printf_more(s, c, %s%.*s%s\n, how, len, padding, one);
+ strbuf_release(onebuf);
+}
+
static void wt_status_print_change_data(struct wt_status *s,
int change_type,
struct string_list_item *it)
--
1.9.0-293-gd838d6f
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/4] wt-status: extract the code to compute width for labels
From: Jonathan Nieder jrnie...@gmail.com
From: Jonathan Nieder jrnie...@gmail.com
Date: Thu, 19 Dec 2013 11:43:19 -0800
Signed-off-by: Junio C Hamano gits...@pobox.com
---
wt-status.c | 22 +++---
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 9cf7028..db98c52 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -292,6 +292,19 @@ static const char *wt_status_diff_status_string(int status)
}
}
+static int maxwidth(const char *(*label)(int), int minval, int maxval)
+{
+ int result = 0, i;
+
+ for (i = minval; i = maxval; i++) {
+ const char *s = label(i);
+ int len = s ? utf8_strwidth(s) : 0;
+ if (len result)
+ result = len;
+ }
+ return result;
+}
+
static void wt_status_print_change_data(struct wt_status *s,
int change_type,
struct string_list_item *it)
@@ -310,13 +323,8 @@ static void wt_status_print_change_data(struct wt_status
*s,
int len;
if (!padding) {
- /* If DIFF_STATUS_* uses outside this range, we're in trouble */
- for (status = 'A'; status = 'Z'; status++) {
- what = wt_status_diff_status_string(status);
- len = what ? strlen(what) : 0;
- if (len label_width)
- label_width = len;
- }
+ /* If DIFF_STATUS_* uses outside the range [A..Z], we're in
trouble */
+ label_width = maxwidth(wt_status_diff_status_string, 'A', 'Z');
label_width += strlen( );
padding = xmallocz(label_width);
memset(padding, ' ', label_width);
--
1.9.0-293-gd838d6f
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/4] wt-status: lift the artificual at least 20 columns floor
When we show unmerged paths, we had an artificial 20 columns floor
for the width of labels (e.g. both deleted:) shown next to the
pathnames. Depending on the locale, this may result in a label that
is too wide when all the label strings are way shorter than 20
columns, or no-op when a label string is longer than 20 columns.
Just drop the artificial floor. The screen real estate is better
utilized this way when all the strings are shorter.
Adjust the tests to this change.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
t/t7060-wtstatus.sh| 14 +++---
t/t7512-status-help.sh | 12 ++--
wt-status.c| 2 --
3 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index 7d467c0..741ec08 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -38,7 +38,7 @@ You have unmerged paths.
Unmerged paths:
(use git add/rm file... as appropriate to mark resolution)
- deleted by us: foo
+ deleted by us: foo
no changes added to commit (use git add and/or git commit -a)
EOF
@@ -142,8 +142,8 @@ You have unmerged paths.
Unmerged paths:
(use git add/rm file... as appropriate to mark resolution)
- both added: conflict.txt
- deleted by them:main.txt
+ both added: conflict.txt
+ deleted by them: main.txt
no changes added to commit (use git add and/or git commit -a)
EOF
@@ -175,9 +175,9 @@ You have unmerged paths.
Unmerged paths:
(use git add/rm file... as appropriate to mark resolution)
- both deleted: main.txt
- added by them: sub_master.txt
- added by us:sub_second.txt
+ both deleted:main.txt
+ added by them: sub_master.txt
+ added by us: sub_second.txt
no changes added to commit (use git add and/or git commit -a)
EOF
@@ -203,7 +203,7 @@ Changes to be committed:
Unmerged paths:
(use git rm file... to mark resolution)
- both deleted: main.txt
+ both deleted:main.txt
Untracked files not listed (use -u option to show untracked files)
EOF
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 3cec57a..68ad2d7 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -33,7 +33,7 @@ You have unmerged paths.
Unmerged paths:
(use git add file... to mark resolution)
- both modified: main.txt
+ both modified: main.txt
no changes added to commit (use git add and/or git commit -a)
EOF
@@ -87,7 +87,7 @@ Unmerged paths:
(use git reset HEAD file... to unstage)
(use git add file... to mark resolution)
- both modified: main.txt
+ both modified: main.txt
no changes added to commit (use git add and/or git commit -a)
EOF
@@ -146,7 +146,7 @@ Unmerged paths:
(use git reset HEAD file... to unstage)
(use git add file... to mark resolution)
- both modified: main.txt
+ both modified: main.txt
no changes added to commit (use git add and/or git commit -a)
EOF
@@ -602,7 +602,7 @@ rebase in progress; onto $ONTO
You are currently rebasing branch '\''statushints_disabled'\'' on
'\''$ONTO'\''.
Unmerged paths:
- both modified: main.txt
+ both modified: main.txt
no changes added to commit
EOF
@@ -636,7 +636,7 @@ You are currently cherry-picking commit $TO_CHERRY_PICK.
Unmerged paths:
(use git add file... to mark resolution)
- both modified: main.txt
+ both modified: main.txt
no changes added to commit (use git add and/or git commit -a)
EOF
@@ -707,7 +707,7 @@ Unmerged paths:
(use git reset HEAD file... to unstage)
(use git add file... to mark resolution)
- both modified: to-revert.txt
+ both modified: to-revert.txt
no changes added to commit (use git add and/or git commit -a)
EOF
diff --git a/wt-status.c b/wt-status.c
index b1b018e..6f3ed67 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -318,8 +318,6 @@ static void wt_status_print_unmerged_data(struct wt_status
*s,
if (!padding) {
label_width = maxwidth(wt_status_unmerged_status_string, 1, 7);
label_width += strlen( );
- if (label_width 20)
- label_width = 20;
padding = xmallocz(label_width);
memset(padding, ' ', label_width);
}
--
1.9.0-293-gd838d6f
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
On Wed, Mar 12, 2014 at 05:14:15PM -0400, Jeff King wrote: I also think that eof = next (which I retained here) is off-by-one. next here is not the newline, but the start of the next line. And I'm guessing the code actually wanted the newline (otherwise it-key ends up with the newline in it). But we cannot just subtract one, because if we hit eob, it really is in the right spot. I started on a patch for this, but found another interesting corner case. If we do not find a newline and hit end-of-buffer (which _shouldn't_ happen, as that is a malformed commit object), we set next to eob. But then we copy the whole string, including *next into the value of the header. So we intend to capture the trailing newline in the value, and do in the normal case. But in the end-of-buffer case, we capture an extra NUL. I think that's OK, because the eventual fate of the buffer is to become a C-string. But it does mean that the result sometimes has a newline-terminator and sometimes doesn't, and the calling code needs to handle this when printing, or risk lines running together. Should this function append a newline if there is not already one? If it's a mergetag header, we feed the result to gpg, etc, and expect to get the data out verbatim. We would not want to mess that up. OTOH, the commit object is bogus (and possibly truncated) in the first place, so it probably doesn't really matter. And the GPG signature on tag objects has its own delimiters, so a stray newline present or not at the end should not matter. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
On Wed, Mar 12, 2014 at 05:14:15PM -0400, Jeff King wrote: One thing that bugs me about the current code is that the sub-function looks one past the end of the length given to it by the caller. Switching it to pass eof - line + 1 resolves that, but is it right? The character pointed at by eof is either: 1. space, if our strchr(line, ' ') found something 2. the first character of the next line, if our memchr(line, '\n', eob - line) found something 3. Whatever is at eob (end-of-buffer) This misses a case. We might find no space at all, and eof is NULL. We never dereference it, so we don't segfault, but it does cause a bogus giant allocation. I'm out of time for the day, but here is a test I started on that demonstrates the failure: diff --git a/t/t7513-commit-bogus-extra-headers.sh b/t/t7513-commit-bogus-extra-headers.sh index e69de29..834db08 100755 --- a/t/t7513-commit-bogus-extra-headers.sh +++ b/t/t7513-commit-bogus-extra-headers.sh @@ -0,0 +1,26 @@ +#!/bin/sh + +test_description='check parsing of badly formed commit objects' +. ./test-lib.sh + +EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904 + +test_expect_success 'newline right after mergetag header' ' + test_tick + cat commit -EOF + tree $EMPTY_TREE + author $GIT_AUTHOR_NAME $GIT_AUTHOR_EMAIL $GIT_AUTHOR_DATE + committer $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL $GIT_COMMITTER_DATE + mergetag + + foo + EOF + commit=$(git hash-object -w -t commit commit) + cat expect -EOF + todo... + EOF + git --no-pager show --show-signature $commit actual + test_cmp expect actual +' + +test_done The git show fails with: fatal: Out of memory, malloc failed (tried to allocate 18446744073699764927 bytes) So I think the whole function could use some refactoring to handle corner cases better. I'll try to take a look tomorrow, but please feel free if somebody else wants to take a crack at it. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Jeff King p...@peff.net writes:
Thanks, I think this is a real readability improvement in most cases.
...
I tried:
perl -i -lpe '
s/memcmp\(([^,]+), (.*?), (\d+)\)/
length($2) == $3 ?
qq{!starts_with($1, $2)} :
$
/ge
' $(git ls-files '*.c')
That comes up with almost the same results as yours, but misses a few
cases that you caught (in addition to the need to simplify
!!starts_with()):
- memcmp(foo, bar, strlen(bar))
- strings with interpolation, like foo\n, which is actually 4
characters in length, not 5.
But there were only a few of those, and I hand-verified them. So I feel
pretty good that the changes are all correct transformations.
That leaves the question of whether they are all improvements in
readability (more on that below).
After reading the patch and the result of your Perl replacement, I'd
agree with the correctness but I am not as convinced as you seem
to be about the real readability improvement in most cases. some
cases, perhaps, though.
Taking two random examples from an early and a late parts of the
patch:
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const
char *obj_name)
enum object_type type;
unsigned long size;
char *buffer = read_sha1_file(sha1, type,
size);
- if (memcmp(buffer, object , 7) ||
+ if (!starts_with(buffer, object ) ||
get_sha1_hex(buffer + 7, blob_sha1))
die(%s not a valid tag,
sha1_to_hex(sha1));
free(buffer);
diff --git a/transport.c b/transport.c
index ca7bb44..a267822 100644
--- a/transport.c
+++ b/transport.c
@@ -1364,7 +1364,7 @@ static int refs_from_alternate_cb(struct
alternate_object_database *e,
while (other[len-1] == '/')
other[--len] = '\0';
- if (len 8 || memcmp(other + len - 8, /objects, 8))
+ if (len 8 || !starts_with(other + len - 8, /objects))
return 0;
/* Is this a git repository with refs? */
memcpy(other + len - 8, /refs, 6);
The original hunks show that the code knows and relies on magic
numbers 7 and 8 very clearly and there are rooms for improvement.
The result after the conversion, however, still have the same magic
numbers, but one less of them each. Doesn't it make it harder to
later spot the patterns to come up with a better abstraction that
does not rely on the magic number? Especially in the first hunk, we
can spot the repeated 7s in the original that make it glaringly
clear that we might want a better abstraction there, but in the text
after the patch, there is less clue that encourages us to take a
look at that buffer + 7 further to make sure we do not feed a
wrong string to get_sha1_hex() by mistake when we update the
surrounding code or the data format this codepath parses.
I think grepping for memcmp() that compares the same number of
bytes as a constant string, like you showed in that Perl scriptlet,
is a very good first step to locate where we might want to look for
improvements. I however think that a mechanical replacement of all
such memcmp() with !start_with() is of a dubious value.
After finding the hunk in the cat-file.c shown above, and after
seeing many other similar patterns, one may realize that it is a
good use case for a variant of skip_prefix() that returns boolean,
which we discussed earlier, perhaps like so:
if (!skip_over(buffer, object , object_name) ||
get_sha1_hex(object_name, blob_sha1))
die(...);
and such a solution would be what truly eradicates the reliance of
magic constants that might break by miscounting bytes in string
constants. I do not think mechanical replacement to !start_with()
is going in the right direction and reaching a halfway to that
goal. I honestly think that it instead is taking us into a wrong
direction, without really solving the use of brittle magic constants
and making remaining reliance of them even harder to spot.
So
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Jeff King p...@peff.net writes: So I think the whole function could use some refactoring to handle corner cases better. I'll try to take a look tomorrow, but please feel free if somebody else wants to take a crack at it. Yup, thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote
On Wed, Mar 12, 2014 at 2:04 PM, Junio C Hamano gits...@pobox.com wrote: Subject: [PATCH] request-pull: documentation updates The original description talked only about what it does. Instead, start it with the purpose of the command, i.e. what it is used for, and then mention what it does to achieve that goal. Clarify what start, url and end means in the context of the overall purpose of the command. Describe the extended syntax of end parameter that is used when the local branch name is different from the branch name at the repository the changes are published. Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/git-request-pull.txt | 55 +- 1 file changed, 49 insertions(+), 6 deletions(-) diff --git a/Documentation/git-request-pull.txt b/Documentation/git-request-pull.txt index b99681c..57bc1f5 100644 --- a/Documentation/git-request-pull.txt +++ b/Documentation/git-request-pull.txt @@ -13,22 +13,65 @@ SYNOPSIS DESCRIPTION --- -Summarizes the changes between two commits to the standard output, and includes -the given URL in the generated summary. +Prepare a request to your upstream project to pull your changes to +their tree to the standard output, by summarizing your changes and +showing where your changes can be pulled from. Perhaps splitting this into two sentence (and using fewer to's) would make it a bit easier to grok? Something like: Generate a request asking your upstream project to pull changes into their tree. The request, printed to standard output, summarizes the changes and indicates from where they can be pulled. +The upstream project is expected to have the commit named by +`start` and the output asks it to integrate the changes you made +since that commit, up to the commit named by `end`, by visiting +the repository named by `url`. + OPTIONS --- -p:: - Show patch text + Include patch text in the output. start:: - Commit to start at. + Commit to start at. This names a commit that is already in + the upstream history. url:: - URL to include in the summary. + The repository URL to be pulled from. end:: - Commit to end at; defaults to HEAD. + Commit to end at (defaults to HEAD). This names the commit + at the tip of the history you are asking to be pulled. ++ +When the repository named by `url` has the commit at a tip of a +ref that is different from the ref you have it locally, you can use Did you want to drop it from this sentence? Or did you mean to say the ref as you have it locally? +the `local:remote` syntax, to have its local name, a colon `:`, +and its remote name. + + +EXAMPLE +--- + +Imagine that you built your work on your `master` branch on top of +the `v1.0` release, and want it to be integrated to the project. +First you push that change to your public repository for others to +see: + + git push https://git.ko.xz/project master + +Then, you run this command: + + git request-pull v1.0 https://git.ko.xz/project master + +which will produce a request to the upstream, summarizing the +changes between the `v1.0` release and your `master`, to pull it +from your public repository. + +If you pushed your change to a branch whose name is different from +the one you have locally, e.g. + + git push https://git.ko.xz/project master:for-linus + +then you can ask that to be pulled with + + git request-pull v1.0 https://git.ko.xz/project master:for-linus + GIT --- -- 1.9.0-293-gd838d6f -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Microproject idea: new OPT_* macros for PARSE_OPT_NONEG
On Sat, Mar 8, 2014 at 2:20 AM, Junio C Hamano gits...@pobox.com wrote: Looking at git grep -B3 OPT_NONEG output, it seems that NONEG is associated mostly with OPTION_CALLBACK and OPTION_SET_INT in the existing code. Perhaps OPT_SET_INT should default to not just OPT_NOARG but also OPT_NONEG? There are OPT_SET_INT() that should not have NONEG in current code. So there are two sets of SET_INT anyway. Either we convert them all to a new macro that takes an extra flag, or we add OPT_SET_INT_NONEG() that covers one set and leave the other set alone. I have a suspition that most users of other OPT_SET_TYPE short-hands may also want them to default to NONEG (and the rare ones that want to allow --no-value-of-this-type=Heh for some reason to use the fully spelled form). IIRC NONEG is relatively a new addition, and many existing OPT_STRING() may predate it. I haven't checked how NONEG affects other types. But that's something I should probably look into. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
On Tue, Mar 11, 2014 at 02:45:59PM -0700, Junio C Hamano wrote: Thanks. Neither this nor John's seems to describe the user-visible way to trigger the symptom. Can we have tests for them? I'll try to get to writing some test today or tomorrow. I just noticed the bugginess by looking at the code, so I'll need to actually spend time reproducing the problem. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
[PATCH v5] install_branch_config: simplify verbose messages logic
Replace the chain of if statements with table of strings.
Signed-off-by: Paweł Wawruch pa...@aleg.pl
---
Thanks to Eric Sunshine and Junio C Hamano.
Simplified printing logic. The name moved to a table.
v4: http://thread.gmane.org/gmane.comp.version-control.git/243914
v3: http://thread.gmane.org/gmane.comp.version-control.git/243865
v2: http://thread.gmane.org/gmane.comp.version-control.git/243849
v1: http://thread.gmane.org/gmane.comp.version-control.git/243502
branch.c | 42 +-
1 file changed, 17 insertions(+), 25 deletions(-)
diff --git a/branch.c b/branch.c
index 723a36b..c17817c 100644
--- a/branch.c
+++ b/branch.c
@@ -53,6 +53,20 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
int remote_is_branch = starts_with(remote, refs/heads/);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
+ const char *message[][2][2] = {{{
+ N_(Branch %s set up to track remote branch %s from %s by
rebasing.),
+ N_(Branch %s set up to track remote branch %s from %s.),
+ },{
+ N_(Branch %s set up to track local branch %s by rebasing.),
+ N_(Branch %s set up to track local branch %s.),
+ }},{{
+ N_(Branch %s set up to track remote ref %s by rebasing.),
+ N_(Branch %s set up to track remote ref %s.),
+ },{
+ N_(Branch %s set up to track local ref %s by rebasing.),
+ N_(Branch %s set up to track local ref %s.)
+ }}};
+ const char *name[] = {remote, shortname};
if (remote_is_branch
!strcmp(local, shortname)
@@ -76,31 +90,9 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
}
strbuf_release(key);
- if (flag BRANCH_CONFIG_VERBOSE) {
- if (remote_is_branch origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track remote branch %s
from %s by rebasing.) :
- _(Branch %s set up to track remote branch %s
from %s.),
- local, shortname, origin);
- else if (remote_is_branch !origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track local branch %s
by rebasing.) :
- _(Branch %s set up to track local branch
%s.),
- local, shortname);
- else if (!remote_is_branch origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track remote ref %s by
rebasing.) :
- _(Branch %s set up to track remote ref %s.),
- local, remote);
- else if (!remote_is_branch !origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track local ref %s by
rebasing.) :
- _(Branch %s set up to track local ref %s.),
- local, remote);
- else
- die(BUG: impossible combination of %d and %p,
- remote_is_branch, origin);
- }
+ if (flag BRANCH_CONFIG_VERBOSE)
+ printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
+ local, name[!remote_is_branch], origin);
}
/*
--
1.8.3.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with starts_with()
On Thu, Mar 13, 2014 at 12:22 AM, Jens Lehmann jens.lehm...@web.de wrote: That spot uses memcmp() because ce-name may not be 0-terminated. ce-name is 0-terminated (at least if it's created the normal way, I haven't checked where this ce in submodule.c comes from). ce_namelen() is just an optimization because we happen to store name's length if it's shorter than 4096, so there's no need to strlen(ce-name) again. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH] implement submodule config cache for lookup of submodule names
Hi,
On Tue, Mar 11, 2014 at 07:28:52PM -0700, Jonathan Nieder wrote:
Heiko Voigt wrote:
This submodule configuration cache allows us to lazily read .gitmodules
configurations by commit into a runtime cache which can then be used to
easily lookup values from it. Currently only the values for path or name
are stored but it can be extended for any value needed.
Makes sense.
[...]
Next I am planning to extend this so configuration cache so it will
return the local configuration (with the usual .gitmodules,
/etc/gitconfig, .git/config, ... overlay of variables) when you pass it
null_sha1 for gitmodule or commit sha1. That way we can give this
configuration cache some use and implement its usage for the current
configuration variables in submodule.c.
Yay!
I wonder whether local configuration should also override information
from commits at times.
Yeah but for that I plan a different code path that solves conflicts
and/or extend missing values. I think its best to keep the submodule
configurations by commit as simple as possible. But we will see once I
get to the point where we need this.
[...]
--- /dev/null
+++ b/Documentation/technical/api-submodule-config-cache.txt
@@ -0,0 +1,39 @@
+submodule config cache API
+==
Most API documents in Documentation/technical have a section explaining
general usage --- e.g. (from api-revision-walking),
Calling sequence
The walking API has a given calling sequence: first you need to
initialize a rev_info structure, then add revisions to control what kind
of revision list do you want to get, finally you can iterate over the
revision list.
Or (from api-string-list):
The caller:
. Allocates and clears a `struct string_list` variable.
. Initializes the members. You might want to set the flag
`strdup_strings`
if the strings should be strdup()ed. For example, this is necessary
[...]
. Can remove items not matching a criterion from a sorted or unsorted
list using `filter_string_list`, or remove empty strings using
`string_list_remove_empty_items`.
. Finally it should free the list using `string_list_clear`.
This makes it easier to get started by looking at the documentation alone
without having to mimic an example.
True, will extend that in the next iteration.
Another thought: it's tempting to call this the 'submodule config API' and
treat the (optional?) memoization as an implementation detail instead of
reminding the caller of it too much. But I haven't thought that through
completely.
Thats the same point Jeff mentioned and I think that will simplify many
things. As I mentioned in the answer to Jeff I will keep passing around
the cache pointer internally but expose only functions without it to the
outside to be future proof but also easy to use for the current use
case.
[...]
+`struct submodule_config *get_submodule_config_from_path(
+ struct submodule_config_cache *cache,
+ const unsigned char *commit_sha1,
+ const char *path)::
+
+ Lookup a submodules configuration by its commit_sha1 and path.
The cache just always grows until it's freed, right? Would it make
sense to allow cached configurations to be explicitly evicted when the
caller is done with them some day? (Just curious, not a complaint.
Might be worth mentioning in the overview how the cache is expected to
be used, though.)
Yes it only grows at the moment. Not sure whether we need to remove
configurations. Currently the only use case that comes to my mind would
be if it grows to big to be kept in memory, but at the moment that seems
far away for me, so I would leave that for a future extension. It will
be hard to know whether someone is done with a certain .gitmodule file
since we cache it by its sha1 expecting it to be the same over many
revisions.
[...]
--- /dev/null
+++ b/submodule-config-cache.h
@@ -0,0 +1,26 @@
+#ifndef SUBMODULE_CONFIG_CACHE_H
+#define SUBMODULE_CONFIG_CACHE_H
+
+#include hashmap.h
+#include strbuf.h
+
+struct submodule_config_cache {
+ struct hashmap for_path;
+ struct hashmap for_name;
+};
Could use comments (e.g., saying what keys each maps to what values).
Would callers ever look at the hashmaps directly or are they only
supposed to be accessed using helper functions that take a whole
struct?
Users should only use the helper functions, but I will hide this in the
submodule-config module. Will add some comment as well.
[...]
+
+/* one submodule_config_cache entry */
+struct submodule_config {
+ struct strbuf path;
+ struct strbuf name;
+ unsigned char gitmodule_sha1[20];
Could also use comments. What does the gitmodule_sha1 field represent?
Thats the sha1 of the parsed .gitmodule file blob.
[...]
--- /dev/null
+++
No progress from push when using bitmaps
Today I tried pushing a copy of linux.git from a client that had bitmaps into a JGit server. The client stalled for a long time with no progress, because it reused the existing pack. No progress appeared while it was sending the existing file on the wire: $ git push git://localhost/linux.git master Reusing existing pack: 2938117, done. Total 2938117 (delta 0), reused 0 (delta 0) remote: Resolving deltas: 66% (1637269/2455727) This is not the best user experience. :-( -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[no subject]
Hello Dear, This is a personal email directed to you. My wife and I won the Euro Millions Jackpot Lottery of £148 million (Pounds) on August 10, 2012. We just commenced our Charity Donation and we will be giving out a cash donation of £7,000.000.00 GBP(Seven Million great Britain pounds) to five(5)lucky individuals and ten(10)charity organisations from any part of the world. Your email address was submitted to my wife and I by the Google Management Team, you have received this email because we have listed you as one of the lucky millionaires. Kindly get back to us so that we can direct our Bank to effect a valid Bank Draft in your name to your operational bank account in your country. you can also go through this link for more information. http://www.bbc.co.uk/news/uk-england-19254228 http://news.sky.com/story/972395/148-6m-euromillions-jackpot-winners-named Mr. Adrian bayford E-mail: adriangbayf...@bigpond.com Tel: +447035965675 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
From what I can gather, there seems to be opposition to specific
pieces of this patch.
The following area is clearly the most controversial:
static inline int standard_header_field(const char *field, size_t len)
{
-return ((len == 4 !memcmp(field, tree , 5)) ||
-(len == 6 !memcmp(field, parent , 7)) ||
-(len == 6 !memcmp(field, author , 7)) ||
-(len == 9 !memcmp(field, committer , 10)) ||
-(len == 8 !memcmp(field, encoding , 9)));
+return ((len == 4 starts_with(field, tree )) ||
+(len == 6 starts_with(field, parent )) ||
+(len == 6 starts_with(field, author )) ||
+(len == 9 starts_with(field, committer )) ||
+(len == 8 starts_with(field, encoding )));
I am happy to submit a version of this patch excluding this section
(and/or others), but it seems I've stumbled into a more fundamental
conversation about the place for helper functions in general (and
about refactoring skip_prefix()). I am working on this particular
change as a microproject, #14 on the list [1], and am not as familiar
with the conventions of the Git codebase as many of you on this list
are.
Junio said:
The result after the conversion, however, still have the same magic
numbers, but one less of them each. Doesn't it make it harder to
later spot the patterns to come up with a better abstraction that
does not rely on the magic number?
It is _not_ my goal to make the code harder to maintain down the road.
So, at this point, which hunks (if any) are worth patching?
Quint
[1]: http://git.github.io/SoC-2014-Microprojects.html
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] fsck.c: Change the type of fsck_ident()'s first argument
Hi,
On Thu, Mar 13, 2014 at 4:22 AM, Jeff King p...@peff.net wrote:
On Thu, Mar 13, 2014 at 02:51:29AM +0800, Yuxuan Shui wrote:
Since fsck_ident doesn't change the content of **ident, the type of
ident could be const char **.
Unfortunately, const double-pointers in C are a bit tricky, and a
pointer to char * cannot automatically be passed as a pointer to
const char *.
Thanks for pointing this out, I split the changes in a wrong way. I'll
fix this in next version of this patch.
I think you want this on top:
diff --git a/fsck.c b/fsck.c
index 1789c34..7776660 100644
--- a/fsck.c
+++ b/fsck.c
@@ -281,7 +281,7 @@ static int fsck_ident(const char **ident, struct object
*obj, fsck_error error_f
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- char *buffer = commit-buffer;
+ const char *buffer = commit-buffer;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
Otherwise, gcc will complain about incompatible pointer types.
-Peff
--
Regards
Yuxuan Shui
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/2] GSoC micro project, rewrite fsck_commit() to use skip_prefix()
Improved commit message, and added a missing hunk to the second commit. Yuxuan Shui (2): fsck.c: Change the type of fsck_ident()'s first argument fsck.c: Rewrite fsck_commit() to use skip_prefix() fsck.c | 26 ++ 1 file changed, 14 insertions(+), 12 deletions(-) -- 1.9.0 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument
Since fsck_ident doesn't change the content of **ident, the type of
ident could be const char **.
This change is required to rewrite fsck_commit() to use skip_prefix().
Signed-off-by: Yuxuan Shui yshu...@gmail.com
---
fsck.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fsck.c b/fsck.c
index 99c0497..7776660 100644
--- a/fsck.c
+++ b/fsck.c
@@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict,
fsck_error error_func)
return retval;
}
-static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)
+static int fsck_ident(const char **ident, struct object *obj, fsck_error
error_func)
{
if (**ident == '')
return error_func(obj, FSCK_ERROR, invalid author/committer
line - missing space before email);
@@ -281,7 +281,7 @@ static int fsck_ident(char **ident, struct object *obj,
fsck_error error_func)
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- char *buffer = commit-buffer;
+ const char *buffer = commit-buffer;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
