Re: [PATCH 1/3] wt-status: Make status messages more consistent with others
Andrew Wong andrew.k...@gmail.com writes:
This is mainly changing messages that say:
run git foo --bar
to
use git foo --bar to baz
git foo --bar is fine, but to baz was hard to read without first
realizing that 'baz' stands for some/any verb. I think rephrasing
it to
use git foo --bar to do baz
would reduce confusion.
diff --git a/wt-status.c b/wt-status.c
index a452407..9f2358a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -899,13 +899,13 @@ static void show_merge_in_progress(struct wt_status *s,
status_printf_ln(s, color, _(You have unmerged paths.));
if (s-hints)
status_printf_ln(s, color,
- _( (fix conflicts and run \git commit\)));
+ _( (fix conflicts and use \git commit\ to
conclude the merge)));
} else {
status_printf_ln(s, color,
_(All conflicts fixed but you are still merging.));
if (s-hints)
status_printf_ln(s, color,
- _( (use \git commit\ to conclude merge)));
+ _( (use \git commit\ to conclude the
merge)));
}
wt_status_print_trailer(s);
}
The above hunk makes sense.
At first glance, I felt that none of the remainder made much sense.
My reaction was: git foo --continue to continue? What else could
the --continue option even mean?
The real value I see in these conversions is by saying use this to
continue instead of an unconditional run this, it implies *IF*
you wanted to continue, you can do this, meaning that user also has
the option of *not* continuing. But the proposed update falls short
of realizing the full potential, if that is the value we are trying
to add. I'd say
fix conflicts and then use git am --continue if you want
to continue.
or an even more explicit
fix conflicts and then use git am --continue if you want
to continue; or you can git am --abort to discontinue.
would be an improvement, but
fix conflicts and then use git am --continue to continue
is probably not quite.
@@ -922,7 +922,7 @@ static void show_am_in_progress(struct wt_status *s,
if (s-hints) {
if (!state-am_empty_patch)
status_printf_ln(s, color,
- _( (fix conflicts and then run \git am
--continue\)));
+ _( (fix conflicts and then use \git am
--continue\ to continue)));
status_printf_ln(s, color,
_( (use \git am --skip\ to skip this patch)));
status_printf_ln(s, color,
--
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 3/3] reset: Print a warning when user uses git reset during a merge
Andrew Wong andrew.k...@gmail.com writes:
During a merge, --mixed is most likely not what the user wants. Using
--mixed during a merge would leave the merged changes and new files
mixed in with the local changes. The user would have to manually clean
up the work tree, which is non-trivial. In future releases, we want to
make git reset error out when used in the middle of a merge. For now,
we simply print out a warning to the user.
Signed-off-by: Andrew Wong andrew.k...@gmail.com
---
builtin/reset.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/builtin/reset.c b/builtin/reset.c
index 4fd1c6c..04e8103 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -331,8 +331,29 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
_(reset_type_names[reset_type]));
}
if (reset_type == NONE)
+ {
reset_type = MIXED; /* by default */
+ /* During a merge, --mixed is most likely not what the user
Two style niggles here.
+ * wants. Using --mixed during a merge would leave the merged
+ * changes and new files mixed in with the local changes. The
+ * user would have to manually clean up the work tree, which is
+ * non-trivial. In future releases, we want to make git reset
we want? Has any of us decided on that?
+ * error out when used in the middle of a merge. For now, we
+ * simply print out a warning to the user. */
+ if (is_merge())
+ warning(_(You have used 'git reset' in the middle of a
merge. 'git reset' defaults to\n
+ 'git reset --mixed', which means git will
not clean up any merged changes and\n
+ new files that were created in the work
tree. It also becomes impossible for\n
+ git to automatically clean up the work tree
later, so you would have to clean\n
+ up the work tree manually. To avoid this
next time, you may want to use 'git\n
+ reset --merge', or equivalently 'git merge
--abort'.\n
+ \n
+ In future releases, using 'git reset' in the
middle of a merge will result in\n
+ an error.
+ ));
+ }
+
if (reset_type != SOFT reset_type != MIXED)
setup_work_tree();
--
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 2/3] merge: Advise user to use git merge --abort to abort merges
Andrew Wong andrew.k...@gmail.com writes: Print message during git merge and git status. Add a new mergeHints advice to silence these messages. This sounds sensible. Don't we want to have this one take effect on the places where advice.resolveConflict is used in git-pull? I.e. something like: do_we_advise=no if advice.resolveConflict is not set: if advice.mergeHints is set to false: do_we_advise=no else: do_we_advise=yes else: do_we_advise=yes if do_we_advise == 'yes': give advice in die_conflict and die_merge -- 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] index-pack: do not segfault when keep_name is NULL
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
keep_name is used to print error messages a couple lines down. Reset
it to the real path returned by odb_pack_keep() if it's set to NULL by
caller.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
One of these moments I will make git log and friends optionally recognize
Diff-Options: line in commit message. Adding -U14 in this case
should help the reviewer to see how those error messages are printed.
builtin/index-pack.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a6b1c17..d95c3dc 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1283,9 +1283,10 @@ static void final(const char *final_pack_name, const
char *curr_pack_name,
if (keep_msg) {
int keep_fd, keep_msg_len = strlen(keep_msg);
- if (!keep_name)
+ if (!keep_name) {
keep_fd = odb_pack_keep(name, sizeof(name), sha1);
- else
+ keep_name = name;
+ } else
keep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600);
I think this fixes the right problem in a wrong way that hurts
longer-term maintainability. Why not do
keep_name ? keep_name : name
at the place where the name is used? Otherwise you will have to
worry about affecting later codepaths that may want to try to use
!keep_name to switch between two codepaths, no?
if (keep_fd 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 4/4] gc --aggressive: three phase repacking
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: As explained in the previous commit,... [PATCH 3/4] becomes a commit with an empty log message for some reason. Have you tried running am -s on it? -- 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] Documentation/gitk: Document new config file location
Astril Hayato astrilhay...@gmail.com writes: User configuration file is now stored at $XDG_CONFIG_HOME/git/gitk Signed-off-by: Astril Hayato astrilhay...@gmail.com --- Documentation/gitk.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt index 1e9e38a..417a707 100644 --- a/Documentation/gitk.txt +++ b/Documentation/gitk.txt @@ -166,8 +166,8 @@ gitk --max-count=100 --all \-- Makefile:: Files - -Gitk creates the .gitk file in your $HOME directory to store preferences -such as display options, font, and colors. +User configuration and preferences are stored at $XDG_CONFIG_HOME/git/gitk or +$HOME/.config/git/gitk if $XDG_CONFIG_HOME is not set. It is a bit more complex than that, isn't it? - $XDG_CONFIG_HOME/git/gitk is used if $XDG_CONFIG_HOME is set; otherwise - $HOME/.gitk is used, if you already have one; otherwise - $HOME/.config/git/gitk is used. -- 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] Add grep.fullName config variable
Andreas Schwab sch...@linux-m68k.org writes:
This configuration variable sets the default for the --full-name option.
Signed-off-by: Andreas Schwab sch...@linux-m68k.org
---
Would this change break Porcelains (e.g. Emacs modes) and force them
to be updated to explicitly pass --no-full-name to unbreak them?
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
--
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 1/3][GSOC] diff: rename read_directory() to get_directory_list()
Hiroyuki Sano sh19910...@gmail.com writes:
Including dir.h in diff-no-index.c, it causes a compile error, because
the same name function read_directory() is declared globally in dir.h.
This change is to avoid conflicts as above.
Signed-off-by: Hiroyuki Sano sh19910...@gmail.com
---
diff-no-index.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 8e10bff..1ed5c9d 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -16,7 +16,7 @@
#include builtin.h
#include string-list.h
-static int read_directory(const char *path, struct string_list *list)
+static int get_directory_list(const char *path, struct string_list *list)
Renaming is a good idea but the new name sounds like you are
grabbing the names of directories, ignoring all the files, no?
{
DIR *dir;
struct dirent *e;
@@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
int i1, i2, ret = 0;
size_t len1 = 0, len2 = 0;
- if (name1 read_directory(name1, p1))
+ if (name1 get_directory_list(name1, p1))
return -1;
- if (name2 read_directory(name2, p2)) {
+ if (name2 get_directory_list(name2, p2)) {
string_list_clear(p1, 0);
return -1;
}
--
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 3/3][GSOC] fsck: use is_dot_or_dotdot() instead of strcmp()
Hiroyuki Sano sh19910...@gmail.com writes:
The is_dot_or_dotdot() is used to check if the string is either . or ...
Signed-off-by: Hiroyuki Sano sh19910...@gmail.com
---
fsck.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/fsck.c b/fsck.c
index b3022ad..c9d7784 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
#include commit.h
#include tag.h
#include fsck.h
+#include dir.h
static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
{
@@ -171,10 +172,12 @@ static int fsck_tree(struct tree *item, int strict,
fsck_error error_func)
has_full_path = 1;
if (!*name)
has_empty_name = 1;
- if (!strcmp(name, .))
- has_dot = 1;
- if (!strcmp(name, ..))
- has_dotdot = 1;
+ if (is_dot_or_dotdot(name)) {
+ if (!name[1])
+ has_dot = 1;
+ else
+ has_dotdot = 1;
+ }
In what way is this an improvement?
This looks like because I was told to, not because the resulting
code is better to me.
The other patch on diff-no-index looked sensible, though.
if (!strcmp(name, .git))
has_dotgit = 1;
has_zero_pad |= *(char *)desc.buffer == '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 1/1] general style: replaces memcmp() with starts_with()
Quint Guvernator quintus.pub...@gmail.com writes:
memcmp() is replaced with negated starts_with() when comparing strings
from the beginning and when it is logical to do so. 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
---
Thanks. This probably needs retitled, though (hint: replaces?
who does so?) and the message rewritten (see numerous reviews on
other GSoC micros from Eric).
diff --git a/builtin/apply.c b/builtin/apply.c
index 0189523..de84dce 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -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;
The above two looks sensible.
I sense that there is a bonus point for an independent follow-up
patch to unify the conflicting definitions of what an incomplete
line should look like. Hint, hint...
@@ -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, @@ -)) {
If there were a variant of starts_with() that works on a counted
string, and rewriting this using it to
while (starts_with_counted(line, size, @@ -)) {
would make perfect sense, but as written above, I do not think it is
an improvement.
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 3e1d5c3..4135980 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 (!memcmp(used_atom[at], color:, 6))
+ if (starts_with(used_atom[at], color:))
need_color_reset_at_eol = !!strcmp(used_atom[at],
color_reset);
}
return 0;
Good.
diff --git a/builtin/mktag.c b/builtin/mktag.c
index 640ab64..640c28f 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -57,7 +57,7 @@ static int verify_tag(char *buffer, unsigned long size)
/* Verify type line */
type_line = object + 48;
- if (memcmp(type_line - 1, \ntype , 6))
+ if (!starts_with(type_line - 1, \ntype ))
return error(char%d: could not find \\\ntype \, 47);
/* Verify tag-line */
Good.
@@ -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));
Not quite. I suspect that what actually makes this strange and
tricky is that this no tag found check is misplaced. It found the
type line, expects that the next line is a tag line, and instead of
validating the remainder of type line, it does this thing, and then
verifies the actual type string, and for that, it needs tag_line
variable to stay where it is.
If we flipped the order of things around the codepath a bit, then we
should be able to first validate the type line, and then use
skip-prefix to skip the tag part (while validating that that line
actually begins with tag ) and check the tag name is a non-empty
string that consists of a good character. All of that is a topic
for a separate patch.
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3cfe02d..23ef424 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -81,14 +81,14 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
}
/* Ignore commit comments */
- if (!patchlen memcmp(line, diff , 5))
+ if (!patchlen !starts_with(line, diff ))
continue;
/* Parsing diff header? */
if (before == -1) {
- if (!memcmp(line, index , 6))
+ if (starts_with(line, index ))
Re: [PATCH 0/3] Make git more user-friendly during a merge conflict
Andrew Wong andrew.k...@gmail.com writes: 2/3: I've added advice.mergeHints to silent the messages that suggests git merge--abort. 3/3: I've added a warning message when users used git reset during a merge. This warning will be printed if the user is in the middle of a merge. In future releases, we'll change this into an error to prevent work tree from becoming a mess. Andrew Wong (3): wt-status: Make status messages more consistent with others merge: Advise user to use git merge --abort to abort merges reset: Print a warning when user uses git reset during a merge Documentation/config.txt | 3 +++ advice.c | 2 ++ advice.h | 1 + builtin/merge.c | 6 ++ builtin/reset.c | 21 + wt-status.c | 23 +-- 6 files changed, 46 insertions(+), 10 deletions(-) Has this series been tested with existing test suite? I tentatively queued it to 'pu' but then had to revert because many tests started failing, causing me to redo the today's integration cycle for 'pu' once again. -- 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 1/1] general style: replaces memcmp() with starts_with()
Jeff King p...@peff.net writes: On Mon, Mar 17, 2014 at 03:52:51PM -0700, Junio C Hamano wrote: diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 3e1d5c3..4135980 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 (!memcmp(used_atom[at], color:, 6)) + if (starts_with(used_atom[at], color:)) need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset); } return 0; Good. Actually, I found this one confusing. We are looking for color:, but if we find it, we _don't_ skip past and look at what comes after. Instead, we compare the whole string. Which works because color_reset actually contains color:reset, and we end up just re-comparing the first bit of the string. So the memcmp here is redundant, and this can simply become: need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset); Or am I missing something? What if used_atom[at] is not related to color at all? We do not want to touch the variable. -- 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 4/4] gc --aggressive: three phase repacking
Duy Nguyen pclo...@gmail.com writes: On Tue, Mar 18, 2014 at 5:12 AM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: As explained in the previous commit,... [PATCH 3/4] becomes a commit with an empty log message for some reason. Have you tried running am -s on it? am -s worked fine. am -s --scissors did not (because of my use of scissors in the commit message). Not sure what happened on your side.. Yeah, I meant am -c. -- 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] Add grep.fullName config variable
Andreas Schwab sch...@linux-m68k.org writes: Yes, that would be required. On the other hand, currently it is impossible to cut-n-paste a file name without --full-name, since the pager is always started in top-level. Perhaps it is better to fix the latter? So far we never cared where the pager runs, but as a principle, I think it would be nice if we run it in the original subdirectory, not at the top of the working tree (unless we have to bend backwards to make the codepath involved too ugly, that is). Don't we have the exact same issue for the editor, by the way? Shouldn't we be running it in the original subdirectory as well? -- 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 v3] test-lib.sh: do not echo externally supplied strings
Uwe Storbeck u...@ibr.ch writes:
In some places we echo a string that is supplied by the calling
test script and may contain backslash sequences. The echo command
of some shells, most notably dash, interprets these backslash
sequences (POSIX.1 allows this) which may scramble the test
output.
Signed-off-by: Uwe Storbeck u...@ibr.ch
---
Commit message rewritten to avoid title continuation in the body.
Thanks Junio C Hamano for the hint.
Here is what I queued yesterday. I was wrong to say control
characters; a backslash sequence is not necessarily a control
character (e.g. \c at the end that suppresses the final LF), so I'll
replace it with your version.
The test titles are not externally supplied but under our control,
so it is less problematic than the rebase -i case where we do get
bitten by user supplied commit title string. Still it is a good
idea to apply this change to protect ourselves.
Thanks.
commit 215cd588caebe86fe77115bdda97930b4659aecd
Author: Uwe Storbeck u...@ibr.ch
Date: Sat Mar 15 00:57:36 2014 +0100
test-lib.sh: do not echo test titles
The test title could be a string with backslash in it, and can be
interpreted as control characters by the echo command of some shells
(e.g. dash).
Signed-off-by: Uwe Storbeck u...@ibr.ch
Signed-off-by: Junio C Hamano gits...@pobox.com
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..3c7cb1d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -277,7 +277,7 @@ error Test script did not set test_description.
if test $help = t
then
- echo $test_description
+ printf '%s\n' $test_description
exit 0
fi
@@ -328,7 +328,7 @@ test_failure_ () {
test_failure=$(($test_failure + 1))
say_color error not ok $test_count - $1
shift
- echo $@ | sed -e 's/^/# /'
+ printf '%s\n' $* | sed -e 's/^/# /'
test $immediate = || { GIT_EXIT_OK=t; exit 1; }
}
--
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 1/1] general style: replaces memcmp() with starts_with()
Eric Sunshine sunsh...@sunshineco.com writes: A patch of this nature doesn't require much more description than stating what it does (replace memcmp() with starts_with()) and why (improve code clarity). The following rewrite might be sufficient: Subject: replace memcmp() with starts_with() starts_with() indicates the intention of the check more clearly than memcmp(). This is more concise; thank you. I will adapt this as the message for the next version of this patch. Would it be wise to mention magic numbers, as the discussion surrounding the rationale of this patch, especially with Junio and Michael, has centered around that? I was thinking of mentioning magic numbers in the example, but decided that their removal was a natural and obvious consequence of the improvement to code clarity, so it wasn't strictly necessary to talk about it. On the other hand, it is a good secondary justification, thus it should be perfectly acceptable to mention it. If I was writing the commit message, I might start by saying As an additional benefit... and then talk a bit about magic number retirement. I think your subject is good (as you said, it makes it clear that it is a project-wide clean-up by not mentioning any specific area), but indicates the intention of the check more clearly would not tell new readers who are unfamiliar with the helper API what intention it is talking about very much, so perhaps Subject: use starts_with() instead of !memcmp() When checking if a string begins with a constant string, using starts_with() is less error prone than calling !memcmp() with an explicit byte count. or something? -- 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] Is there any interest in adding a port of checkpatch.pl to contrib/?
Jacopo Notarstefano jacopo.notarstef...@gmail.com writes: It seems to me that the topic of adding the checkpatch.pl script to Git's source tree has cropped up several times in the past, as recently as a couple of days ago: $gmane/243607. It should be noted that its usage for its sake has been discouraged by Junio Hamano in $gmane/205998. I've never said any such thing. I only said I am not enthused against a proposal to add a build target that runs checkpatch or equivalent over *all* existing code, which will invite needless churn (read again the part of the message you quoted before I say I am not enthused). It is a totally separate issue for submitters to make sure they do not introduce *new* problems, and use of checkpatch --no-tree could be one way to do 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] git-rebase: Teach rebase - shorthand.
Brian Gesiak modoca...@gmail.com writes:
Teach rebase the same shorthand as checkout and merge; that is, that -
means the branch we were previously on.
Reported-by: Tim Chase g...@tim.thechases.com
Signed-off-by: Brian Gesiak modoca...@gmail.com
---
git-rebase.sh | 4
t/t3400-rebase.sh | 6 ++
2 files changed, 10 insertions(+)
diff --git a/git-rebase.sh b/git-rebase.sh
index 5f6732b..2c75e9f 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -453,6 +453,10 @@ then
test $fork_point = auto fork_point=t
;;
*) upstream_name=$1
+ if test $upstream_name = -
+ then
+ upstream_name=@{-1}
+ fi
shift
;;
esac
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6d94b1f..00aba9f 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -88,6 +88,12 @@ test_expect_success 'rebase from ambiguous branch name' '
git rebase master
'
+test_expect_success 'rebase using shorthand' '
+ git checkout master
+ git checkout -b shorthand HEAD^
+ GIT_TRACE=1 git rebase -
I'd rather not to see that TRACE there. We would also want to make
sure the result is what we expect to see, not only the command does
not error out, no?
+'
+
test_expect_success 'rebase a single mode change' '
git checkout master
git branch -D topic
--
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][GSOC2014] add: Rewrite run_add_interactive to use struct argv_array
Movchan Pavel movchan...@gmail.com writes:
Origin code are code with own realisation argv array editing.
It was changed, and code modified for using unified argv-array
realisation from argv-array.h.
Commit for Google Summer of Code 2014
Signed-off-by: Movchan Pavel movchan...@gmail.com
---
Thanks. Commit for ... is not something we would want to see in
git log output, though.
builtin/add.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 4b045ba..258b491 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -15,6 +15,7 @@
#include diffcore.h
#include revision.h
#include bulk-checkin.h
+#include argv-array.h
static const char * const builtin_add_usage[] = {
N_(git add [options] [--] pathspec...),
@@ -141,23 +142,21 @@ static void refresh(int verbose, const struct pathspec
*pathspec)
int run_add_interactive(const char *revision, const char *patch_mode,
const struct pathspec *pathspec)
{
- int status, ac, i;
- const char **args;
+ int status, i;
+ struct argv_array *argv = ARGV_ARRAY_INIT;
Where does that pointer point at and who allocated the piece of
memory used by it?
See http://thread.gmane.org/gmane.comp.version-control.git/244151
for an example solution.
- args = xcalloc(sizeof(const char *), (pathspec-nr + 6));
- ac = 0;
- args[ac++] = add--interactive;
+ argv_array_push(argv, add--interactive);
if (patch_mode)
- args[ac++] = patch_mode;
+ argv_array_push(argv, patch_mode);
if (revision)
- args[ac++] = revision;
- args[ac++] = --;
+ argv_array_push(argv, revision);
+ argv_array_push(argv, --);
for (i = 0; i pathspec-nr; i++)
/* pass original pathspec, to be re-parsed */
- args[ac++] = pathspec-items[i].original;
+ argv_array_push(argv, pathspec-items[i].original);
- status = run_command_v_opt(args, RUN_GIT_CMD);
- free(args);
+ status = run_command_v_opt(argv-argv, RUN_GIT_CMD);
+ argv_array_clear(argv);
return status;
}
--
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 won Linux Magazine's Linux New Media Award in the category Outstanding Contribution to Open Source/Linux/Free Software
Richard Hartmann richih.mailingl...@gmail.com writes: Dear all, Git won an award in the main category of the English German Linux Magazine at CeBIT, this year. Jens Lehmann, Heiko Voigt, and myself were present to accept the award on behalf of the Git community as a whole. You can find a short blurb on my blog[1], including a picture of the physical prize. It seems the video of the award ceremony is not up yet, but I have been told it will come soon(tm). Best regards, Richard [1] http://richardhartmann.de/blog/posts/2014/03/14-Git_prize-Outstanding_Contribution_to_Open_Source__Linux__Free_Software/ Thanks, all. -- 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] Add grep.fullName config variable
Andreas Schwab sch...@linux-m68k.org writes: Junio C Hamano gits...@pobox.com writes: Don't we have the exact same issue for the editor, by the way? Shouldn't we be running it in the original subdirectory as well? It's called with an absolute name, so it shouldn't care. But we should not have to call with absolute paths when a short and sweet pathname relative to the user's current directory. That is the primary point of my comment. -- 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] tests: set temp variables using 'env' in test function instead of subshell
David Tran unsignedz...@gmail.com writes: Originally, the code used subshells instead of FOO=BAR command because the variable would otherwise leak into the surrounding context of the POSIX shell when 'command' is a shell function. The subshell was used to hold the context for the test. Using 'env' in the test function sets the temp variables without leaking, removing the need of a subshell. These are not temp variables ;-). You are improving the way how commands are run under a different settings to environment variables. Hmm, let's try to see if I can do better: Subject: tests: use env to run commands with temporary env-var settings Ordinarily, we would say VAR=VAL command to execute a tested command with environment variable(s) set only for that command. This however does not work if 'command' is a shell function (most notably 'test_must_fail'); the result of the assignment is retained and affects later commands. To avoid this, we used to assign and export environment variables and run such a test in a subshell, ( VAR=VAL export VAR test_must_fail git command to be tested ) but with env utility, we should be able to say test_must_fail env VAR=VAL git command to be tested which is much shorter and easier to read. Let's see if I replied correctly with send-email. Retrying this again. How do I 'reply' to a thread using send-email? Look for --in-reply-to option in man git-send-email. Signed-off-by: David Tran unsignedz...@gmail.com --- t/t1300-repo-config.sh| 17 ++ t/t1510-repo-setup.sh |4 +-- t/t3200-branch.sh | 12 +-- t/t3301-notes.sh | 22 -- t/t3404-rebase-interactive.sh | 65 t/t3413-rebase-hook.sh|6 +--- t/t4014-format-patch.sh | 14 ++--- t/t5305-include-tag.sh|4 +-- t/t5602-clone-remote-exec.sh | 13 ++-- t/t5801-remote-helpers.sh |6 +-- t/t6006-rev-list-format.sh|9 ++ t/t7006-pager.sh | 18 ++- 12 files changed, 42 insertions(+), 148 deletions(-) Thanks. The numbers look very good ;-) We love code reduction. diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index c9c426c..3e3f77b 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -974,24 +974,15 @@ test_expect_success SYMLINKS 'symlinked configuration' ' ' test_expect_success 'nonexistent configuration' ' - ( - GIT_CONFIG=doesnotexist - export GIT_CONFIG - test_must_fail git config --list - test_must_fail git config test.xyzzy - ) + test_must_fail env GIT_CONFIG=doesnotexist git config --list + test_must_fail env GIT_CONFIG=doesnotexist git config test.xyzzy ' test_expect_success SYMLINKS 'symlink to nonexistent configuration' ' ln -s doesnotexist linktonada ln -s linktonada linktolinktonada - ( - GIT_CONFIG=linktonada - export GIT_CONFIG - test_must_fail git config --list - GIT_CONFIG=linktolinktonada - test_must_fail git config --list - ) + test_must_fail env GIT_CONFIG=linktonada git config --list + test_must_fail env GIT_CONFIG=linktolinktonada git config --list ' test_expect_success 'check split_cmdline return' diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh index cf2ee78..e1b2a99 100755 --- a/t/t1510-repo-setup.sh +++ b/t/t1510-repo-setup.sh @@ -777,9 +777,7 @@ test_expect_success '#30: core.worktree and core.bare conflict (gitfile version) setup_repo 30 $here/30 gitfile true ( cd 30 - GIT_DIR=.git - export GIT_DIR - test_must_fail git symbolic-ref HEAD 2result + test_must_fail env GIT_DIR=.git git symbolic-ref HEAD 2result ) grep core.bare and core.worktree 30/result ' diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index fcdb867..d45e95c 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -849,11 +849,7 @@ test_expect_success 'detect typo in branch name when using --edit-description' ' write_script editor -\EOF echo New contents $1 EOF - ( - EDITOR=./editor - export EDITOR - test_must_fail git branch --edit-description no-such-branch - ) + test_must_fail env EDITOR=./editor git branch --edit-description no-such-branch ' test_expect_success 'refuse --edit-description on unborn branch for now' ' @@ -861,11 +857,7 @@ test_expect_success 'refuse --edit-description on unborn branch for now' ' echo New contents $1 EOF git checkout --orphan
[ANNOUNCE] Git v1.9.1
The latest maintenance release Git v1.9.1 is now available at the usual places. The release tarballs are found at: https://www.kernel.org/pub/software/scm/git/ The following public repositories all have a copy of the v1.9.1 tag and the maint branch that the tag points at: url = https://kernel.googlesource.com/pub/scm/git/git url = git://repo.or.cz/alt-git.git url = https://code.google.com/p/git-core/ url = git://git.sourceforge.jp/gitroot/git-core/git.git url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core url = https://github.com/gitster/git Git v1.9.1 Release Notes Fixes since v1.9.0 -- * git clean -d pathspec did not use the given pathspec correctly and ended up cleaning too much. * git difftool misbehaved when the repository is bound to the working tree with the .git file mechanism, where a textual file .git tells us where it is. * git push did not pay attention to branch.*.pushremote if it is defined earlier than remote.pushdefault; the order of these two variables in the configuration file should not matter, but it did by mistake. * Codepaths that parse timestamps in commit objects have been tightened. * git diff --external-diff incorrectly fed the submodule directory in the working tree to the external diff driver when it knew it is the same as one of the versions being compared. * git reset needs to refresh the index when working in a working tree (it can also be used to match the index to the HEAD in an otherwise bare repository), but it failed to set up the working tree properly, causing GIT_WORK_TREE to be ignored. * git check-attr when working on a repository with a working tree did not work well when the working tree was specified via the --work-tree (and obviously with --git-dir) option. * merge-recursive was broken in 1.7.7 era and stopped working in an empty (temporary) working tree, when there are renames involved. This has been corrected. * git rev-parse was loose in rejecting command line arguments that do not make sense, e.g. --default without the required value for that option. * include.path variable (or any variable that expects a path that can use ~username expansion) in the configuration file is not a boolean, but the code failed to check it. * git diff --quiet -- pathspec1 pathspec2 sometimes did not return correct status value. * Attempting to deepen a shallow repository by fetching over smart HTTP transport failed in the protocol exchange, when no-done extension was used. The fetching side waited for the list of shallow boundary commits after the sending end stopped talking to it. * Allow git cmd path/, when the 'path' is where a submodule is bound to the top-level working tree, to match 'path', despite the extra and unnecessary trailing slash (such a slash is often given by command line completion). Changes since v1.9.0 are as follows: Brad King (4): t3030-merge-recursive: test known breakage with empty work tree read-cache.c: refactor --ignore-missing implementation read-cache.c: extend make_cache_entry refresh flag with options merge-recursive.c: tolerate missing files while refreshing index David Aguilar (1): difftool: support repositories with .git-files David Sharp (1): rev-parse: check i before using argv[i] against argc Jeff King (12): expand_user_path: do not look at NULL path handle_path_include: don't look at NULL value tests: auto-set LIB_HTTPD_PORT from test name t4212: test bogus timestamps with git-log fsck: report integer overflow in author timestamps date: check date overflow against time_t log: handle integer overflow in timestamps log: do not segfault on gmtime errors remote: handle pushremote config in any order show_ident_date: fix tz range check clean: respect pathspecs with -d clean: simplify dir/not-dir logic Junio C Hamano (4): t0003: do not chdir the whole test process check-attr: move to the top of working tree when in non-bare repository t7800: add a difftool test for .git-files Git 1.9.1 Nguyễn Thái Ngọc Duy (17): test: rename http fetch and push test files pack-protocol.txt: clarify 'obj-id' in the last ACK after 'done' protocol-capabilities.txt: refer multi_ack_detailed back to pack-protocol.txt protocol-capabilities.txt: document no-done fetch-pack: fix deepen shallow over smart http with no-done cap t5537: move http tests out to t5539 reset: optionally setup worktree and refresh index on --mixed pathspec: convert some match_pathspec_depth() to ce_path_match() pathspec: convert some match_pathspec_depth() to dir_path_match() pathspec: rename match_pathspec_depth() to match_pathspec() dir.c
Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell
Jeff King p...@peff.net writes: On Tue, Mar 18, 2014 at 01:37:39PM -0700, Junio C Hamano wrote: diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index c9c426c..3e3f77b 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -974,24 +974,15 @@ test_expect_success SYMLINKS 'symlinked configuration' ' ' test_expect_success 'nonexistent configuration' ' - ( - GIT_CONFIG=doesnotexist - export GIT_CONFIG - test_must_fail git config --list - test_must_fail git config test.xyzzy - ) + test_must_fail env GIT_CONFIG=doesnotexist git config --list + test_must_fail env GIT_CONFIG=doesnotexist git config test.xyzzy ' Isn't GIT_CONFIG here another way of saying: test_must_fail git config -f doesnotexist --list Perhaps that is shorter and more readable still (and there are a few similar cases in this patch. Surely, but are we assuming that git config correctly honors the equivalence between GIT_CONFIG=file and -f file, or is that also something we are testing in these tests? -- 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.
Michael Haggerty mhag...@alum.mit.edu writes:
I had recently been thinking along the same lines. In many of the
potential callers that I noticed, ALLOC_GROW() was used immediately
before making space in the array for a new element. So I suggest
something more like
+#define MOVE_DOWN(array, nr, at, count) \
+ memmove((array) + (at) + (count), \
+ (array) + (at), \
+ sizeof((array)[0]) * ((nr) - (at)))
+#define ALLOC_INSERT_GAP(array, nr, at, count, alloc) \
+ do { \
+ ALLOC_GROW((array), (nr) + (count), (alloc));\
+ MOVE_DOWN((array), (nr), (at), (count)); \
+ } while (0)
Also, count==1 is so frequent that this special case might deserve its
own macro pair.
Yeah, probably.
I'm not inspired by these macro names, though.
Me neither, about ups and downs.
Peff's suggestion to name these around the concept of gap sounded
sensible.
--
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 4/7] commit: fix patch hunk editing with commit -p -m
Torsten Bögershausen tbo...@web.de writes:
+int run_hook_with_custom_index(const char *index_file, const char *name,
...)
+{
+const char *hook_env[3] = { NULL };
+char index[PATH_MAX];
Sorry being late with the review.
Recently some effort has been put to replace the
PATH_MAX/snprintf() combo with code using strbuf.
So I think for new developed code it could make sense to avoid
PATH_MAX from the start.
Yes but because this is a compatibility wrapper for an existing
function that does the string[PATH_MAX] thing already, it would be
clearer to have such a conversion as a separate step.
--
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] test-lib.sh: use printf instead of echo
Jonathan Nieder jrnie...@gmail.com writes:
Junio C Hamano wrote:
Uwe Storbeck wrote:
+ printf '%s\n' $@ | sed -e 's/^/# /'
This is wrong, isn't it? Why do we want one line per item here?
Yes, Hannes caught the same, too. Sorry for the sloppiness.
We currently use echo all over the place (e.g., 'echo $path' in
git-sh-setup), and every time we fix it there is a chance of making
mistakes. I wonder if it would make sense to add a helper to make the
echo calls easier to replace:
I agree that we would benefit from having a helper to print a single
line, which we very often do, without having to worry about the
boilerplate '%s\n' of printf or the portability gotcha of echo.
I am a bit reluctant to name the helper sane_echo to declare echo
that interprets backslashes in the string is insane, though. For
these print a single line uses, we are only interested in using a
subset of the features offered by 'echo', but that does not mean the
other features we do not want to trigger in our use is of no use to
any sane person. It very different from sane_unset that works
around unset on an unset variable that can trigger an error when
nobody sane is interested in that error condition. If somebody is
interested if a variable is not yet set and behave differently,
there are more direct ways to see the set-ness of a variable, and
asking unset for that information is insane, hence I think the
name sane_unset is justified. I do not feel the same way for
sane_echo.
I would have called it say if the name weren't taken.
-- 8 --
Subject: git-sh-setup: introduce sane_echo helper
Using 'echo' with arguments that might contain backslashes or -e or
-n can produce confusing results that vary from platform to platform
(e.g., dash always interprets \ escape sequences and echoes -e
verbatim, whereas bash does not interpret \ escapes unless -e is
passed as an argument to echo and suppresses the -e from its
output).
Instead, we should use printf, which is more predictable:
printf '%s\n' $foo; # Just prints $foo, on all platforms.
Blindly replacing echo with printf '%s\n' would not be good enough
because that printf prints each argument on its own line. Provide a
sane_echo helper that prints its arguments, space-delimited, on a
single line, to make this easier to remember, and tweak 'say'
and 'die_with_status' to illustrate how it is used.
No functional change intended.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
git-sh-setup.sh | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git i/git-sh-setup.sh w/git-sh-setup.sh
index 256c89a..f35b5b9 100644
--- i/git-sh-setup.sh
+++ w/git-sh-setup.sh
@@ -43,6 +43,10 @@ git_broken_path_fix () {
# @@BROKEN_PATH_FIX@@
+sane_echo () {
+ printf '%s\n' $*
+}
+
die () {
die_with_status 1 $@
}
@@ -50,7 +54,7 @@ die () {
die_with_status () {
status=$1
shift
- printf 2 '%s\n' $*
+ sane_echo 2 $*
exit $status
}
@@ -59,7 +63,7 @@ GIT_QUIET=
say () {
if test -z $GIT_QUIET
then
- printf '%s\n' $*
+ sane_echo $*
fi
}
--
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 1/3][GSOC] diff: rename read_directory() to get_directory_list()
Junio C Hamano gits...@pobox.com writes: Hiroyuki Sano sh19910...@gmail.com writes: Including dir.h in diff-no-index.c, it causes a compile error, because the same name function read_directory() is declared globally in dir.h. This change is to avoid conflicts as above. Signed-off-by: Hiroyuki Sano sh19910...@gmail.com --- diff-no-index.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/diff-no-index.c b/diff-no-index.c index 8e10bff..1ed5c9d 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -16,7 +16,7 @@ #include builtin.h #include string-list.h -static int read_directory(const char *path, struct string_list *list) +static int get_directory_list(const char *path, struct string_list *list) Renaming is a good idea but the new name sounds like you are grabbing the names of directories, ignoring all the files, no? I am tempted to suggest, because this is an internal implementation detail only visible to this narrow corner of the system, calling this just static int ls(const char *path, struct string_list *result) ;-) -- 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] tests: set temp variables using 'env' in test function instead of subshell
Jeff King p...@peff.net writes: On Tue, Mar 18, 2014 at 03:16:27PM -0700, Junio C Hamano wrote: Isn't GIT_CONFIG here another way of saying: test_must_fail git config -f doesnotexist --list Perhaps that is shorter and more readable still (and there are a few similar cases in this patch. Surely, but are we assuming that git config correctly honors the equivalence between GIT_CONFIG=file and -f file, or is that also something we are testing in these tests? I think we can assume that they are equivalent, and it is not worth testing (and they are equivalent in code since 270a344 (config: stop using config_exclusive_filename, 2012-02-16). My recollection is that GIT_CONFIG mostly exists as a historical footnote. Recall that at one time it affected all commands, but that had many problems and was done away with in dc87183 (Only use GIT_CONFIG in git config, not other programs, 2008-06-30). I think we left it in place for git-config mostly for backward compatibility,... Thanks. Then I think it makes sense to do such a conversion but it probably should be done on top of this patch (we could do it before this patch), not as a part of this 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
Re: [PATCH v2] git-rebase: Teach rebase - shorthand.
Brian Gesiak modoca...@gmail.com writes:
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6d94b1f..6176754 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -88,6 +88,17 @@ test_expect_success 'rebase from ambiguous branch name' '
git rebase master
'
+test_expect_success 'rebase using shorthand' '
+ git checkout master
+ git checkout -b shorthand HEAD^
+ git rebase - 1shorthand.stdout
+ git checkout master
+ git branch -D shorthand
+ git checkout -b shorthand HEAD^
+ git rebase @{-1} 1without_shorthand.stdout
+ test_i18ncmp without_shorthand.stdout shorthand.stdout
+'
A handful of issues here:
* 1target looks unconventional and wastes readers' time, forcing
them to wonder if there is anything special going on, only to
realize there isn't anything noteworthy. Saying target like
everybody else does avoids attracting unnecessary attention.
* rebase using shorthand is somewhat a myopic title; it assumes
that the only short-hand relevant to rebase will be that a -
stands for @{-1} to specify the branch we rebase the current
branch off of.
* The usual filename for the output from the command being tested
is 'actual', and the usual filename for the expected output is
'expect'. In this case, you are verifying that the output from
rebase - is the same as the output from rebase @{-1}, so it
is more conventional to call the former 'actual' and the latter
'expect'.
* Is the eye-candy output to the standard output what is the most
interesting during the execution of a rebase? Wouldn't we be
more interested to make sure that we did transplant the history
on the same commit between two cases?
rebase - with your change still says something like this:
First, rewinding head to replay your work on top of it...
Fast-forwarded HEAD to @{-1}.
instead of Fast-forwarded HEAD to -. Somebody may later want
to fix this, making these two eye-candy output to be different
from each other, and what your test expects will no longer hold
(not that I think it is better to say - instead of @{-1}
there).
I'll tentatively queue it with a minor tweak (see below).
Thanks.
-- 8 --
From: Brian Gesiak modoca...@gmail.com
Date: Wed, 19 Mar 2014 20:02:15 +0900
Subject: [PATCH] rebase: allow - short-hand for the previous branch
Teach rebase the same shorthand as checkout and merge to name the
branch to rebase the current branch on; that is, that - means the
branch we were previously on.
Requested-by: Tim Chase g...@tim.thechases.com
Signed-off-by: Brian Gesiak modoca...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
git-rebase.sh | 4
t/t3400-rebase.sh | 17 +
2 files changed, 21 insertions(+)
diff --git a/git-rebase.sh b/git-rebase.sh
index 8a3efa2..658c003 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -449,6 +449,10 @@ then
test $fork_point = auto fork_point=t
;;
*) upstream_name=$1
+ if test $upstream_name = -
+ then
+ upstream_name=@{-1}
+ fi
shift
;;
esac
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6d94b1f..80e0a95 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -88,6 +88,23 @@ test_expect_success 'rebase from ambiguous branch name' '
git rebase master
'
+test_expect_success 'rebase off of the previous branch using -' '
+ git checkout master
+ git checkout HEAD^
+ git rebase @{-1} expect.messages
+ git merge-base master HEAD expect.forkpoint
+
+ git checkout master
+ git checkout HEAD^
+ git rebase - actual.messages
+ git merge-base master HEAD actual.forkpoint
+
+ test_cmp expect.forkpoint actual.forkpoint
+ # the next one is dubious---we may want to say -,
+ # instead of @{-1}, in the message
+ test_i18ncmp expect.messages actual.messages
+'
+
test_expect_success 'rebase a single mode change' '
git checkout master
git branch -D topic
--
1.9.1-423-g4596e3a
--
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 v3 1/2][GSoC] diff-no-index: rename read_directory()
Brian Bourn ba.bo...@gmail.com writes:
It would be desirable to replace manual checking of . or ..
in diff-no-index.c with is_dot_or_dotdot(), which is defined
in dir.h, however, dir.h declares a read_directory() which conflicts
with a (different) static read_directory() defined
in diff-no-index.c. As a preparatory step, rename the local
read_directory() to avoid the collision.
Signed-off-by: Brian Bourn ba.bo...@gmail.com
---
Part 1 of my submission for GSoC
diff-no-index.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Looks good to me. Doesn't Eric deserve a Helped-by: above?
Thanks.
diff --git a/diff-no-index.c b/diff-no-index.c
index 8e10bff..ec51106 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -16,7 +16,7 @@
#include builtin.h
#include string-list.h
-static int read_directory(const char *path, struct string_list *list)
+static int read_directory_contents(const char *path, struct string_list
*list)
{
DIR *dir;
struct dirent *e;
@@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
int i1, i2, ret = 0;
size_t len1 = 0, len2 = 0;
- if (name1 read_directory(name1, p1))
+ if (name1 read_directory_contents(name1, p1))
return -1;
- if (name2 read_directory(name2, p2)) {
+ if (name2 read_directory_contents(name2, p2)) {
string_list_clear(p1, 0);
return -1;
}
--
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 3/3][GSOC] fsck: replace if-statements to logical expressions
Hiroyuki Sano sh19910...@gmail.com writes:
There were two different ways to check flag values, one way is
using if-statement, and the other way is using logical expression.
To make sensible, replace if-statements to logical expressions in
fsck_tree().
The change described by these two paragraphs makes sense to me, but
the to make sensible phrasing made me hiccup while reading it.
fsck_tree() uses two different ways to set flag values, many
with a simple if () condition that guards an assignment, and
one with an bitwise-or assignment operator.
Unify them to the latter, as it is shorter and easier to
read when the condition is short and to the point, which all
of them are.
or something?
When checking has_dot and has_dotdot, use is_dot_or_dotdot()
instead of strcmp() to avoid hard coding.
I am not sure how this change is an improvement. Besides being
seemingly inefficient by checking name[1] twice (which is not a huge
objection, as a sensible compiler would notice and optimize), the
caller that checks name[1] already hardcodes its knowledge on what
is_dot_or_dotdot() does, e.g. when it returns true, name[0] is never
NUL, and name[1] is NUL only when it saw a dot and not a dotdot, so
the to avoid hard coding does not really justify this change.
I further wonder if
...
if (!name[0]) {
has_empty_name = 1;
} else if (name[0] == '.') {
has_dot |= !name[1];
has_dotdot |= name[1] == '.' !name[2];
has_dotgit |= !strcmp(name + 1, git);
}
...
may be an improvement (this is not a suggestion--when I say I
wonder, I usually do not know the answer). It defeats the unify
the two styles theme of this change, so...
The is_dot_or_dotdot() is used to check if the string is
either . or ...
Include the dir.h header file to use is_dot_or_dotdot().
Signed-off-by: Hiroyuki Sano sh19910...@gmail.com
---
fsck.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/fsck.c b/fsck.c
index b3022ad..08f613d 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
#include commit.h
#include tag.h
#include fsck.h
+#include dir.h
static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
{
@@ -165,18 +166,12 @@ static int fsck_tree(struct tree *item, int strict,
fsck_error error_func)
sha1 = tree_entry_extract(desc, name, mode);
- if (is_null_sha1(sha1))
- has_null_sha1 = 1;
- if (strchr(name, '/'))
- has_full_path = 1;
- if (!*name)
- has_empty_name = 1;
- if (!strcmp(name, .))
- has_dot = 1;
- if (!strcmp(name, ..))
- has_dotdot = 1;
- if (!strcmp(name, .git))
- has_dotgit = 1;
+ has_null_sha1 |= is_null_sha1(sha1);
+ has_full_path |= !!strchr(name, '/');
+ has_empty_name |= !*name;
+ has_dot |= is_dot_or_dotdot(name) !name[1];
+ has_dotdot |= is_dot_or_dotdot(name) name[1];
+ has_dotgit |= !strcmp(name, .git);
has_zero_pad |= *(char *)desc.buffer == '0';
update_tree_entry(desc);
--
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 1/3][GSOC] diff: rename read_directory() to get_directory_list()
Junio C Hamano gits...@pobox.com writes:
-static int read_directory(const char *path, struct string_list *list)
+static int get_directory_list(const char *path, struct string_list *list)
Renaming is a good idea but the new name sounds like you are
grabbing the names of directories, ignoring all the files, no?
I am tempted to suggest, because this is an internal implementation
detail only visible to this narrow corner of the system, calling
this just
static int ls(const char *path, struct string_list *result)
;-)
Scratch that one.
I'll take read_directory_contents() from Brian Bourn, which
essentially is the same patch.
Thanks.
-- 8 --
From: Brian Bourn ba.bo...@gmail.com
Date: Wed, 19 Mar 2014 11:58:21 -0400
Subject: [PATCH] diff-no-index: rename read_directory()
In the next patch, we will replace a manual checking of . or ..
with a call to is_dot_or_dotdot() defined in dir.h. The private
function read_directory() defined in this file will conflict with
the global function declared there when we do so.
As a preparatory step, rename the private read_directory() to avoid
the name collision.
Signed-off-by: Brian Bourn ba.bo...@gmail.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
diff-no-index.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 33e5982..3e4f47e 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -16,7 +16,7 @@
#include builtin.h
#include string-list.h
-static int read_directory(const char *path, struct string_list *list)
+static int read_directory_contents(const char *path, struct string_list *list)
{
DIR *dir;
struct dirent *e;
@@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
int i1, i2, ret = 0;
size_t len1 = 0, len2 = 0;
- if (name1 read_directory(name1, p1))
+ if (name1 read_directory_contents(name1, p1))
return -1;
- if (name2 read_directory(name2, p2)) {
+ if (name2 read_directory_contents(name2, p2)) {
string_list_clear(p1, 0);
return -1;
}
--
1.9.1-423-g4596e3a
--
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 can not find this particular patch in the latest What's cooking email. Is there something I can do? IIRC, I think I was waiting for the version with a new Usage text section to the documentation you alluded to in this exchange ($gmane/243924): Ilya Bobyr ilya.bo...@gmail.com writes: On 3/11/2014 12:10 PM, Junio C Hamano wrote: Documentation on the whole argument parsing is quite short, so,... ... 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. It does not seems like there is a lot of interest, so I am not sure there will be a lot of discussion. It is a minor fix and considering the number of the emails on the list, I do not unexpected this kind of stuff to be very popular. But it seems like a valid improvement to me. Maybe I am missing something? You did the right thing by sending a reminder message with a pointer to help others locate the original (like the one I am responding to), as nobody can keep up with a busy list traffic. Same questions about this one: [PATCH] gitk: replace SHA1 entry field on keyboard paste http://www.mail-archive.com/git@vger.kernel.org/msg45040.html I think they are more or less similar, except that the second one is just trivial. I do not remember if I forwarded the patch to the area maintainer Paul Mackerras pau...@samba.org, but if I didn't please do so yourself. The changes to gitk and git-gui come to me via their own project repositories. -- 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] git-rebase: Teach rebase - shorthand.
John Keeping j...@keeping.me.uk writes:
On Wed, Mar 19, 2014 at 10:53:01AM -0700, Junio C Hamano wrote:
rebase - with your change still says something like this:
First, rewinding head to replay your work on top of it...
Fast-forwarded HEAD to @{-1}.
instead of Fast-forwarded HEAD to -. Somebody may later want
to fix this, making these two eye-candy output to be different
from each other, and what your test expects will no longer hold
(not that I think it is better to say - instead of @{-1}
there).
I don't think either of these is correct. When using - with the
commands that already support it, I have occasionally found that -
isn't what I thought it was.
Can we use `git name-rev` to put the actual name here, so that people
who have not done what they intended can hopefully notice sooner?
That sounds like a right thing to do. It however is totally
orthogonal to the change we are discussing, and should be done as a
separate patch.
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] test-lib.sh: use printf instead of echo
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: Jonathan Nieder jrnie...@gmail.com writes: Junio C Hamano wrote: Uwe Storbeck wrote: +printf '%s\n' $@ | sed -e 's/^/# /' This is wrong, isn't it? Why do we want one line per item here? Yes, Hannes caught the same, too. Sorry for the sloppiness. We currently use echo all over the place (e.g., 'echo $path' in git-sh-setup), and every time we fix it there is a chance of making mistakes. I wonder if it would make sense to add a helper to make the echo calls easier to replace: I agree that we would benefit from having a helper to print a single line, which we very often do, without having to worry about the boilerplate '%s\n' of printf or the portability gotcha of echo. I am a bit reluctant to name the helper sane_echo to declare echo that interprets backslashes in the string is insane, though. raw_echo Yeah, but the thing is, this is not even raw if you view it from the direction of knowing what echo does. That is why I repeated helper to print a single line, which is a viewpoint from the user side. We do not care how it is implemented, we just want a single line printed is what we want to express, which say is perfectly in line with. We use a subset semantics of 'echo' to implement it is of secondary concern. -- 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] git-rebase: Teach rebase - shorthand.
John Keeping j...@keeping.me.uk writes:
On Wed, Mar 19, 2014 at 12:02:01PM -0700, Junio C Hamano wrote:
John Keeping j...@keeping.me.uk writes:
On Wed, Mar 19, 2014 at 10:53:01AM -0700, Junio C Hamano wrote:
rebase - with your change still says something like this:
First, rewinding head to replay your work on top of it...
Fast-forwarded HEAD to @{-1}.
instead of Fast-forwarded HEAD to -. Somebody may later want
to fix this, making these two eye-candy output to be different
from each other, and what your test expects will no longer hold
(not that I think it is better to say - instead of @{-1}
there).
I don't think either of these is correct. When using - with the
commands that already support it, I have occasionally found that -
isn't what I thought it was.
Can we use `git name-rev` to put the actual name here, so that people
who have not done what they intended can hopefully notice sooner?
That sounds like a right thing to do. It however is totally
orthogonal to the change we are discussing, and should be done as a
separate patch.
Is it not part of adding support for -?
I thought your suggestion was:
'rebase @{-1}' says 'Fast-fowarded HEAD to @{-1}'. It should say
'Fast-forwarded HEAD to 4f407407 (rebase: allow - short-hand
for the previous branch, 2014-03-19)' instead.
Or it could be:
'rebase @{-1}' says 'Fast-fowarded HEAD to @{-1}'. It should say
'Fast-forwarded HEAD to master' instead.
In either case, it does not look like such a change is about
teaching - as a synonym to @{-1}.
--
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 v3] tests: use env to run commands with temporary env-var settings
David Tran unsignedz...@gmail.com writes: Originally, we would use VAR=VAL command to execute a test command with environment variable(s) only for that command. This does not work for commands that are shell functions (most notably test functions like test_must_fail); the result of the assignment is retained and affects later commands. To avoid this, we assigned and exported the environment variables and run the test(s) in a subshell like this, ( VAR=VAL export VAR test_must_fail git command to be tested ) Using the env utility, we should be able to say test_must_fail git command to be tested which is much short and easier to read. Looks familiar ;-) but it seems the changes from the original you took it from all look worsening, not improvements, to me. Isn't GIT_CONFIG here another way of saying: test_must_fail git config -f doesnotexist --list Perhaps that is shorter and more readable still (and there are a few similar cases in this patch. I'll ignore this for now. If needed I can make another patch to resolve this. Yes, I think that is sensible. And it does not have to be done by you. Hopefully this should be all of it. Seems to be well done. 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: What's cooking in git.git (Mar 2014, #03; Fri, 14)
Max Horn m...@quendi.de writes: On 17.03.2014, at 18:01, Junio C Hamano gits...@pobox.com wrote: Torsten Bögershausen tbo...@web.de writes: On 2014-03-14 23.09, Junio C Hamano wrote: * ap/remote-hg-skip-null-bookmarks (2014-01-02) 1 commit - remote-hg: do not fail on invalid bookmarks Reported to break tests ($gmane/240005) Expecting a reroll. I wonder what should happen here. The change breaks all the tests in test-hg-hg-git.sh (And the breakage may prevent us from detecting other breakages) The ideal situation would be to have an extra test case for the problem which we try to fix with this patch. Antoine, is there any way to make your problem reproducable ? And based on that, to make a patch which passes all test cases ? After re-reading the thread briefly (there're just five messages) http://thread.gmane.org/gmane.comp.version-control.git/239797/focus=240069 For some reason, that link does not contain all messages from that conversation (unfortunately, I have seen GMane do that on multiple occasions. I hence try not to rely on it for reviewing email history -- I just don't trust it). In particular, it misses this crucial post: [jc: please avoid overlong lines; I re-flowed above] http://thread.gmane.org/gmane.comp.version-control.git/239830 Interesting. The (or at least a) root cause has actually been discovered. Would a patch that adds an xfail test case for it be acceptable? Do you mean a patch that only adds a new test that expects a failure to the current code, without touching the current code that has the bug it exposes? That would be a good place to start. ... As a matter of fact, I a know a few more bugs in remote-hg for which I could produce xfail test cases. Of course I'd prefer to put them in together with a fix, but I don't know when I can get to that, if ever. So, would such changes be welcome? Surely. That is to keep tabs on bugs in an actionable form; it is a better way of bug tracking than having a bug-tracker that is not actively maintained, I would think. -- 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] Enable index-pack threading in msysgit.
sza...@chromium.org writes:
This adds a Windows implementation of pread. Note that it is NOT
safe to intersperse calls to read() and pread() on a file
descriptor. According to the ReadFile spec, using the 'overlapped'
argument should not affect the implicit position pointer of the
descriptor. Experiments have shown that this is, in fact, a lie.
To accomodate that fact, this change also incorporates:
http://article.gmane.org/gmane.comp.version-control.git/196042
... which gives each index-pack thread its own file descriptor.
---
Sign-off?
The new per-thread file descriptors to the same thing in a generic
codepath is a bit of eyesore. For index-pack, keeping as many file
descritors open to the current pack as the worker threads are should
not be too bad, but could we have some comment next to the field
definition please (e.g. Windows emulation of pread() needs separate
fd per thread; see $URL for details or something)?
builtin/index-pack.c | 21 -
compat/mingw.c | 31 ++-
compat/mingw.h | 3 +++
config.mak.uname | 1 -
4 files changed, 49 insertions(+), 7 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2f37a38..c02dd4c 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -51,6 +51,7 @@ struct thread_local {
#endif
struct base_data *base_cache;
size_t base_cache_used;
+ int pack_fd;
};
/*
@@ -91,7 +92,8 @@ static off_t consumed_bytes;
static unsigned deepest_delta;
static git_SHA_CTX input_ctx;
static uint32_t input_crc32;
-static int input_fd, output_fd, pack_fd;
+static const char *curr_pack;
+static int input_fd, output_fd;
#ifndef NO_PTHREADS
@@ -134,6 +136,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)
*/
static void init_thread(void)
{
+ int i;
init_recursive_mutex(read_mutex);
pthread_mutex_init(counter_mutex, NULL);
pthread_mutex_init(work_mutex, NULL);
@@ -141,11 +144,17 @@ static void init_thread(void)
pthread_mutex_init(deepest_delta_mutex, NULL);
pthread_key_create(key, NULL);
thread_data = xcalloc(nr_threads, sizeof(*thread_data));
+ for (i = 0; i nr_threads; i++) {
+ thread_data[i].pack_fd = open(curr_pack, O_RDONLY);
+ if (thread_data[i].pack_fd == -1)
+ die_errno(unable to open %s, curr_pack);
+ }
threads_active = 1;
}
static void cleanup_thread(void)
{
+ int i;
if (!threads_active)
return;
threads_active = 0;
@@ -155,6 +164,8 @@ static void cleanup_thread(void)
if (show_stat)
pthread_mutex_destroy(deepest_delta_mutex);
pthread_key_delete(key);
+ for (i = 0; i nr_threads; i++)
+ close(thread_data[i].pack_fd);
free(thread_data);
}
@@ -288,13 +299,13 @@ static const char *open_pack_file(const char *pack_name)
output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR,
0600);
if (output_fd 0)
die_errno(_(unable to create '%s'), pack_name);
- pack_fd = output_fd;
+ nothread_data.pack_fd = output_fd;
} else {
input_fd = open(pack_name, O_RDONLY);
if (input_fd 0)
die_errno(_(cannot open packfile '%s'), pack_name);
output_fd = -1;
- pack_fd = input_fd;
+ nothread_data.pack_fd = input_fd;
}
git_SHA1_Init(input_ctx);
return pack_name;
@@ -542,7 +553,7 @@ static void *unpack_data(struct object_entry *obj,
do {
ssize_t n = (len 64*1024) ? len : 64*1024;
- n = pread(pack_fd, inbuf, n, from);
+ n = pread(get_thread_data()-pack_fd, inbuf, n, from);
if (n 0)
die_errno(_(cannot pread pack file));
if (!n)
@@ -1490,7 +1501,7 @@ static void show_pack_info(int stat_only)
int cmd_index_pack(int argc, const char **argv, const char *prefix)
{
int i, fix_thin_pack = 0, verify = 0, stat_only = 0;
- const char *curr_pack, *curr_index;
+ const char *curr_index;
const char *index_name = NULL, *pack_name = NULL;
const char *keep_name = NULL, *keep_msg = NULL;
char *index_name_buf = NULL, *keep_name_buf = NULL;
diff --git a/compat/mingw.c b/compat/mingw.c
index 383cafe..6cc85d6 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -329,7 +329,36 @@ int mingw_mkdir(const char *path, int mode)
return ret;
}
-int mingw_open (const char *filename, int oflags, ...)
+
+ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)
+{
+ HANDLE hand = (HANDLE)_get_osfhandle(fd);
+ if (hand == INVALID_HANDLE_VALUE) {
+ errno = EBADF;
+ return -1;
+ }
+
+ LARGE_INTEGER offset_value;
+
Re: [PATCH v2] git-rebase: Teach rebase - shorthand.
John Keeping j...@keeping.me.uk writes:
I thought your suggestion was:
'rebase @{-1}' says 'Fast-fowarded HEAD to @{-1}'. It should say
'Fast-forwarded HEAD to 4f407407 (rebase: allow - short-hand
for the previous branch, 2014-03-19)' instead.
Or it could be:
'rebase @{-1}' says 'Fast-fowarded HEAD to @{-1}'. It should say
'Fast-forwarded HEAD to master' instead.
In either case, it does not look like such a change is about
teaching - as a synonym to @{-1}.
My suggestion was specifically:
'rebase -' says 'Fast-forwarded HEAD to -'. It should say
'Fast-forwarded HEAD to master' instead.
OK, it was closer to the latter.
But why is it OK to leave @{-1}, which is just as hmm, I do not
remember what the previous branch was myself when the user says
@{-1} in the output while it not OK to leave - in the output?
I do not think of any sane reason, and that is why I think this
improvement is not part of teaching rebase that '-' can be used in
place of @{-1} topic.
--
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] t5510: Do not use $(pwd) when fetching / pushing / pulling via rsync
Johannes Schindelin johannes.schinde...@gmx.de writes: Hi Sebastian, On Wed, 19 Mar 2014, Sebastian Schuberth wrote: On MINGW, pwd is defined as pwd -W in test-lib.sh. This usually is the right thing, but the absolute Windows path with a colon confuses rsync. We could use $PWD in this case to work around the issue, but in fact there is no need to use an absolute path in the first place, so get rid of it. This was discovered in the context of the mingwGitDevEnv project and only did not surface before with msysgit because the latter does not ship rsync. ACK Ciao, Dscho 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 v2] Bump core.deltaBaseCacheLimit to 128MiB
David Kastrup d...@gnu.org writes: The default of 16MiB causes serious thrashing for large delta chains combined with large files. Signed-off-by: David Kastrup d...@gnu.org --- Is that a good argument? Wouldn't the default of 128MiB burden smaller machines with bloated processes? Forgot the signoff. For the rationale of this patch and the 128MiB choice, see the original patch. See the original patch, especially written after three-dash lines without a reference, will not help future readers of git log who later bisects to find that this change hurt their usage and want to see why it was done unconditionally (as opposed to encouraging those who benefit from this change to configure their Git to use larger value for them, without hurting others). While I can personally afford 128MiB, I do *not* think 16MiB was chosen more scientifically than the choice of 128MiB this change proposes to make, and my gut feeling is that this would not have too big a negative impact to anybody, I would prefer to have a reason better than gut feeling before accepting a default change. Thanks. Documentation/config.txt | 2 +- environment.c| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 73c8973..1b6950a 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -484,7 +484,7 @@ core.deltaBaseCacheLimit:: to avoid unpacking and decompressing frequently used base objects multiple times. + -Default is 16 MiB on all platforms. This should be reasonable +Default is 128 MiB on all platforms. This should be reasonable for all users/operating systems, except on the largest projects. You probably do not need to adjust this value. + diff --git a/environment.c b/environment.c index c3c8606..73ed670 100644 --- a/environment.c +++ b/environment.c @@ -37,7 +37,7 @@ int core_compression_seen; int fsync_object_files; size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; -size_t delta_base_cache_limit = 16 * 1024 * 1024; +size_t delta_base_cache_limit = 128 * 1024 * 1024; unsigned long big_file_threshold = 512 * 1024 * 1024; const char *pager_program; int pager_use_color = 1; -- 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][GSoC] Calling for comments regarding rough draft of proposal
tanay abhra tanay...@gmail.com writes: 2.Other things I should add to the proposal that I have left off?I am getting confused what extra details I should add to the proposal. I will add the informal parts(my background, schedule for summer etc) of the proposal later. I would not label the schedule and success criteria informal; without them how would one judge if the proposal has merits? Other things like your background and previous achievements would become relevant, after it is decided that the proposed project has merits, to see if you are a good fit to work on that project, so I agree with your message that it is sensible to defer them before the other parts of the proposal is ironed out. #Proposed Improvements * Fix git config --unset to clean up detritus from sections that are left empty. * Read the configuration from files once and cache the results in an appropriate data structure in memory. * Change `git_config()` to iterate through the pre-read values in memory rather than re-reading the configuration files. * Add new API calls that allow the cache to be inquired easily and efficiently. Rewrite other functions like `git_config_int()` to be cache-aware. I think we already had a discussion to point out git_config_int() is not a good example for this bullet point (check the list archive). The approach seciton seems to use a more sensible example (point 2). * Rewrite callers to use the new API wherever possible. * How to invalidate the cache correctly in the case that the configuration is changed while `git` is executing. I wouldn't list this as an item of list of improvements. It is merely a point you have to be careful about because you are doing other improvements based on read all into memory first and do not re-read files approach, no? In the current code, when somebody does git_config_set() and then later uses git_config() to grab the value of the variable set with the first call, we will read the value written to the file with the first call. With the proposed change, if you parse from the file upfront, callers to git_config_set() will need to somehow invalidate that stale copy in memory, either updating only the changed part (harder) or just discarding the cache (easy). ##Changing the git_config api to retrieve values from memory Approach:- We parse the config file once, storing the raw values to records in memory. After the whole config has been read, iterate through the records, feeding the surviving values into the callback in the order they were originally read (minus deletions). Path to follow for the api conversion, 1. Convert the parser to read into an in-memory representation, but leave git_config() as a wrapper which iterates over it. 2. Add query functions like config_string_get() which will inquire cache for values efficiently. 3. Convert callbacks to query functions one by one. I propose two approaches for the format of the internal cache, 1.Using a hashmap to map keys to their values.This would bring as an advantage, constant time lookups for the values.The implementation will be similar to dict data structure in python, for example, section.subsection --mapped-to-- multi_value_string I have no idea what you wanted to illustrate with that example at all. This approach loses the relative order of different config keys. As long as it keeps the order of multi-value elements, it should not be a problem. 2.Another approach would be to actually represent the syntax tree of the config file in memory. That would make lookups of individual keys more expensive, but would enable other manipulation. E.g., if the syntax tree included nodes for comments and other non-semantic constructs, then we can use it for a complete rewrite. for a complete rewrite of what? And git config becomes: 1. Read the tree. 2. Perform operations on the tree (add nodes, delete nodes, etc). 3. Write out the tree. and things like remove the section header when the last item in the section is removed become trivial during step 2. Are you saying you will try both approaches during the summer? You should be able to look-up quickly *and* to preserve order at the same time within one approach, by either annotating the tree with a hash, or the other way around to annotate the hash with each node remembering where in the original file it came from (which you will need to keep in order to report errors anyway). -- ##Tidy configuration files When a configuration file is repeatedly modified, often garbage is left behind. For example, after git config pull.rebase true git config --unset pull.rebase git config pull.rebase true git config --unset pull.rebase the bottom of the configuration file is left with the useless lines [pull] [pull] Also,setting a config value, appends the key-value pair at the end of
Re: [PATCH v2] remote-hg: do not fail on invalid bookmarks
Max Horn m...@quendi.de writes: Mercurial can have bookmarks pointing to nullid (the empty root revision), while Git can not have references to it. When cloning or fetching from a Mercurial repository that has such a bookmark, the import will fail because git-remote-hg will not be able to create the corresponding reference. Warn the user about the invalid reference, and continue the import, instead of stopping right away. The text above is identical to what Antoine wrote in e8d48743 (remote-hg: do not fail on invalid bookmarks, 2013-12-29); I'd assume that this is to replace it. But the code seems to do more, and I think that is related to the detailed analysis you dug up from the archive earlier and then summarised in your $gmane/20. Can you say a bit more about these fake-bmark and bmark checking like you did in that original 3-patch series? Thanks. Also add some test cases for this issue. Reported-by: Antoine Pelisse apeli...@gmail.com Signed-off-by: Max Horn m...@quendi.de --- contrib/remote-helpers/git-remote-hg | 6 + contrib/remote-helpers/test-hg.sh| 48 2 files changed, 54 insertions(+) diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg index eb89ef6..49b2c2e 100755 --- a/contrib/remote-helpers/git-remote-hg +++ b/contrib/remote-helpers/git-remote-hg @@ -625,6 +625,12 @@ def list_head(repo, cur): def do_list(parser): repo = parser.repo for bmark, node in bookmarks.listbookmarks(repo).iteritems(): +if node == '': +if fake_bmark == 'default' and bmark == 'master': +pass +else: +warn(Ignoring invalid bookmark '%s', bmark) +continue bmarks[bmark] = repo[node] cur = repo.dirstate.branch() diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh index a933b1e..8d01b32 100755 --- a/contrib/remote-helpers/test-hg.sh +++ b/contrib/remote-helpers/test-hg.sh @@ -772,4 +772,52 @@ test_expect_success 'remote double failed push' ' ) ' +test_expect_success 'clone remote with master null bookmark' ' + test_when_finished rm -rf gitrepo* hgrepo* + + ( + hg init hgrepo + cd hgrepo + echo a a + hg add a + hg commit -m a + hg bookmark -r null master + ) + + git clone hg::hgrepo gitrepo + check gitrepo HEAD a +' + +test_expect_success 'clone remote with default null bookmark' ' + test_when_finished rm -rf gitrepo* hgrepo* + + ( + hg init hgrepo + cd hgrepo + echo a a + hg add a + hg commit -m a + hg bookmark -r null -f default + ) + + git clone hg::hgrepo gitrepo + check gitrepo HEAD a +' + +test_expect_success 'clone remote with generic null bookmark' ' + test_when_finished rm -rf gitrepo* hgrepo* + + ( + hg init hgrepo + cd hgrepo + echo a a + hg add a + hg commit -m a + hg bookmark -r null bmark + ) + + git clone hg::hgrepo gitrepo + check gitrepo HEAD a +' + test_done -- 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] Bump core.deltaBaseCacheLimit to 128MiB
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: David Kastrup d...@gnu.org writes: The default of 16MiB causes serious thrashing for large delta chains combined with large files. Signed-off-by: David Kastrup d...@gnu.org --- Is that a good argument? Wouldn't the default of 128MiB burden smaller machines with bloated processes? The default file size before Git forgets about delta compression is 512MiB. Unpacking 500MiB files with 16MiB of delta storage is going to be uglier. ... Documentation/config.txt states: core.deltaBaseCacheLimit:: Maximum number of bytes to reserve for caching base objects that may be referenced by multiple deltified objects. By storing the entire decompressed base objects in a cache Git is able to avoid unpacking and decompressing frequently used base objects multiple times. + Default is 16 MiB on all platforms. This should be reasonable for all users/operating systems, except on the largest projects. You probably do not need to adjust this value. I've seen this seriously screwing performance in several projects of mine that don't really count as largest projects. So the description in combination with the current setting is clearly wrong. That is a good material for proposed log message, and I think you are onto something here. I know that the 512MiB default for the bitFileThreashold (aka forget about delta compression) came out of thin air. It was just 1GB is always too huge for anybody, so let's cut it in half and declare that value the initial version of a sane threashold, nothing more. So it might be that the problem is 512MiB is still too big, relative to the 16MiB of delta base cache, and the former may be what needs to be tweaked. If a blob close to but below 512MiB is a problem for 16MiB delta base cache, it would still be too big to cause the same problem for 128MiB delta base cache---it would evict all the other objects and then end up not being able to fit in the limit itself, busting the limit immediately, no? I would understand if the change were to update the definition of deltaBaseCacheLimit and link it to the value of bigFileThreashold, for example. With the presented discussion, I am still not sure if we can say that bumping deltaBaseCacheLimit is the right solution to the description with the current setting is clearly wrong (which is a real issue). -- 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] Enable index-pack threading in msysgit.
sza...@chromium.org (Stefan Zager) writes:
This adds a Windows implementation of pread. Note that it is NOT
safe to intersperse calls to read() and pread() on a file
descriptor. According to the ReadFile spec, using the 'overlapped'
argument should not affect the implicit position pointer of the
descriptor. Experiments have shown that this is, in fact, a lie.
To accomodate that fact, this change also incorporates:
http://article.gmane.org/gmane.comp.version-control.git/196042
... which gives each index-pack thread its own file descriptor.
Signed-off-by: Stefan Zager sza...@chromium.org
---
I'll queue it on 'pu' until I hear from Windows folks.
There were a few things I tweaked while queuing, tho.
- the indentation of the new comment inside struct thread_local
declaration looked strange;
- there was one new if () statement whose block was opened on the
next line, not on the same line as if () itself.
Thanks.
builtin/index-pack.c | 30 --
compat/mingw.c | 37 -
compat/mingw.h | 3 +++
config.mak.uname | 1 -
4 files changed, 59 insertions(+), 12 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2f37a38..63b8b0e 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -40,17 +40,17 @@ struct base_data {
int ofs_first, ofs_last;
};
-#if !defined(NO_PTHREADS) defined(NO_THREAD_SAFE_PREAD)
-/* pread() emulation is not thread-safe. Disable threading. */
-#define NO_PTHREADS
-#endif
-
struct thread_local {
#ifndef NO_PTHREADS
pthread_t thread;
#endif
struct base_data *base_cache;
size_t base_cache_used;
+/*
+ * To accomodate platforms that have pthreads, but don't have a
+ * thread-safe pread, give each thread its own file descriptor.
+ */
+ int pack_fd;
};
/*
@@ -91,7 +91,8 @@ static off_t consumed_bytes;
static unsigned deepest_delta;
static git_SHA_CTX input_ctx;
static uint32_t input_crc32;
-static int input_fd, output_fd, pack_fd;
+static const char *curr_pack;
+static int input_fd, output_fd;
#ifndef NO_PTHREADS
@@ -134,6 +135,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)
*/
static void init_thread(void)
{
+ int i;
init_recursive_mutex(read_mutex);
pthread_mutex_init(counter_mutex, NULL);
pthread_mutex_init(work_mutex, NULL);
@@ -141,11 +143,17 @@ static void init_thread(void)
pthread_mutex_init(deepest_delta_mutex, NULL);
pthread_key_create(key, NULL);
thread_data = xcalloc(nr_threads, sizeof(*thread_data));
+ for (i = 0; i nr_threads; i++) {
+ thread_data[i].pack_fd = open(curr_pack, O_RDONLY);
+ if (thread_data[i].pack_fd == -1)
+ die_errno(unable to open %s, curr_pack);
+ }
threads_active = 1;
}
static void cleanup_thread(void)
{
+ int i;
if (!threads_active)
return;
threads_active = 0;
@@ -155,6 +163,8 @@ static void cleanup_thread(void)
if (show_stat)
pthread_mutex_destroy(deepest_delta_mutex);
pthread_key_delete(key);
+ for (i = 0; i nr_threads; i++)
+ close(thread_data[i].pack_fd);
free(thread_data);
}
@@ -288,13 +298,13 @@ static const char *open_pack_file(const char *pack_name)
output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR,
0600);
if (output_fd 0)
die_errno(_(unable to create '%s'), pack_name);
- pack_fd = output_fd;
+ nothread_data.pack_fd = output_fd;
} else {
input_fd = open(pack_name, O_RDONLY);
if (input_fd 0)
die_errno(_(cannot open packfile '%s'), pack_name);
output_fd = -1;
- pack_fd = input_fd;
+ nothread_data.pack_fd = input_fd;
}
git_SHA1_Init(input_ctx);
return pack_name;
@@ -542,7 +552,7 @@ static void *unpack_data(struct object_entry *obj,
do {
ssize_t n = (len 64*1024) ? len : 64*1024;
- n = pread(pack_fd, inbuf, n, from);
+ n = pread(get_thread_data()-pack_fd, inbuf, n, from);
if (n 0)
die_errno(_(cannot pread pack file));
if (!n)
@@ -1490,7 +1500,7 @@ static void show_pack_info(int stat_only)
int cmd_index_pack(int argc, const char **argv, const char *prefix)
{
int i, fix_thin_pack = 0, verify = 0, stat_only = 0;
- const char *curr_pack, *curr_index;
+ const char *curr_index;
const char *index_name = NULL, *pack_name = NULL;
const char *keep_name = NULL, *keep_msg = NULL;
char *index_name_buf = NULL, *keep_name_buf = NULL;
diff --git a/compat/mingw.c b/compat/mingw.c
index 383cafe..0efc570 100644
--- a/compat/mingw.c
+++
Re: [PATCH 0/3] Make git more user-friendly during a merge conflict
Andrew Wong andrew.k...@gmail.com writes: On Mon, Mar 17, 2014 at 7:04 PM, Junio C Hamano gits...@pobox.com wrote: Has this series been tested with existing test suite? ... I tested it during RFC, but missed it when I sent it as patch. ... I'll fix the problem. Sorry about that. Thanks. Will hold onto the topic branch lest I forget, but will keep it out of 'pu' in the meantime. -- 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] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: On Fri, Mar 07, 2014 at 08:08:37AM +0100, Christian Couder wrote: Be it graft or replace, I do not think we want to invite people to use these mechansims too lightly to locally rewrite their history willy-nilly without fixing their mistakes at the object layer with commit --amend, rebase, bfg, etc. in the longer term. So in that sense, adding a command to make it easy is not something I am enthusiastic about. On the other hand, if the user does need to use graft or replace (perhaps to prepare for casting the fixed history in stone with filter-branch), it would be good to help them avoid making mistakes while doing so and tool support may be a way to do so. So, ... I am of two minds. ... I do not think the features we are talking about are significantly more dangerous than git replace is in the first place. If we want to make people aware of the dangers, perhaps git-replace.1 is the right place to do it. Sure. So should we take the four-patch series for git replace --edit? -- 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 1.9.1 tarball
Jason St. John jstj...@purdue.edu writes: On Wed, Mar 19, 2014 at 8:09 PM, 乙酸鋰 ch3co...@gmail.com wrote: Hi, Where to find git 1.9.1 tarball? It is not uploaded to google code. -- You can download a tarball for 1.9.1 from GitHub: https://github.com/git/git/archive/v1.9.1.tar.gz Jason The announcement message starts like this: The latest maintenance release Git v1.9.1 is now available at the usual places. The release tarballs are found at: https://www.kernel.org/pub/software/scm/git/ It is somewhat strange that nobody seems to read the announcement that has that exact information before asking. -- 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] test-lib.sh: use printf instead of echo
Jonathan Nieder jrnie...@gmail.com writes: Junio C Hamano wrote: ... I am a bit reluctant to name the helper sane_echo to declare echo that interprets backslashes in the string is insane, though. For these print a single line uses, we are only interested in using a subset of the features offered by 'echo', but that does not mean the other features we do not want to trigger in our use is of no use to any sane person. In a portable script, uncareful use of 'echo' is always insane. I agree that makes sense and I actually think that it is a bit stronger than that. If a script is meant to be portable, there is no way to use echo on a string whose contents is unknown sanely. There is no careful use is OK. In a script tailored to an environment where echo behaves consistently it is perfectly reasonable to use 'echo', but that's a different story. In the context of git, saying Here is the thing you should always use instead of echo is a good thing, in my opinion. That is true in my opinion, but that thing is also what you should always use instead of printf '%s\n'. A guideline more useful for the users is Here is the thing you should always use when literally emitting a single line., isn't it? -- 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: Configuring a third-party git hook
Chris Angelico ros...@gmail.com writes: file. It doesn't really care about the full history, and wants to be reasonably fast (as the user is waiting for it). It's just a convenience, so correctness isn't a huge issue. The easiest way to keep it moving through quickly is to limit the search: $ git log ...other options... HEAD~100 some-file.pike The problem with this is that it doesn't work if HEAD doesn't have 100 great-great-...-grandparents Did you really mean that you are *not* interested in what happened to the most recent 100 commits? Or is it a typo of HEAD~100..? git log -100 should traverse from the HEAD and stop after showing at most 100 items, even if you only had 20 in the history. -- 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] Bump core.deltaBaseCacheLimit to 128MiB
Duy Nguyen pclo...@gmail.com writes: On Thu, Mar 20, 2014 at 5:11 AM, Junio C Hamano gits...@pobox.com wrote: ... I know that the 512MiB default for the bitFileThreashold (aka forget about delta compression) came out of thin air. It was just 1GB is always too huge for anybody, so let's cut it in half and declare that value the initial version of a sane threashold, nothing more. So it might be that the problem is 512MiB is still too big, relative to the 16MiB of delta base cache, and the former may be what needs to be tweaked. If a blob close to but below 512MiB is a problem for 16MiB delta base cache, it would still be too big to cause the same problem for 128MiB delta base cache---it would evict all the other objects and then end up not being able to fit in the limit itself, busting the limit immediately, no? I would understand if the change were to update the definition of deltaBaseCacheLimit and link it to the value of bigFileThreashold, for example. With the presented discussion, I am still not sure if we can say that bumping deltaBaseCacheLimit is the right solution to the description with the current setting is clearly wrong (which is a real issue). I vote make big_file_threshold smaller. 512MB is already unfriendly for many smaller machines. I'm thinking somewhere around 32MB-64MB (and maybe increase delta cache base limit to match). These numbers match my gut feeling (e.g. 4k*4k*32-bit uncompressed would be 64MB); delta cash base that is sized to the same as (or perhaps twice as big as) that limit may be a good default. The only downside I see is large blobs will be packed undeltified, which could increase pack size if you have lots of them. I think that is something that can be tweaked, unless the user tells us otherwise via command line override, when running the improved gc --aggressive ;-) -- 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] diff: optimise parse_dirstat_params() to only compare strings when necessary
Dragos Foianu dragos.foi...@gmail.com writes:
parse_dirstat_params() goes through a chain of if statements using
strcmp to parse parameters. When the parameter is a digit, the
value must go through all comparisons before the function realises
it is a digit. Optimise this logic by only going through the chain
of string compares when the parameter is not a digit.
This change could be an optimization only if parse_dirstat_params()
is called with a param that begins with a digit a lot more often
than with other forms of params, but that is a mere assumption.
Unless that assumption is substantiated, this change can be a
pessimization.
Even if the assumption were true (which I doubt), a simpler solution
to optimize such a call pattern would be to simply tweak of the
order if/else cascade to check if the param begins with a digit
first before checking other keywords, wouldn't it? I am not sure
why you even need to change the structure into a nested if
statement.
Signed-off-by: Dragos Foianu dragos.foi...@gmail.com
---
diff.c | 37 +++--
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/diff.c b/diff.c
index e343191..733764e 100644
--- a/diff.c
+++ b/diff.c
@@ -84,20 +84,25 @@ static int parse_dirstat_params(struct diff_options
*options, const char *params
string_list_split_in_place(params, params_copy, ',', -1);
for (i = 0; i params.nr; i++) {
const char *p = params.items[i].string;
- if (!strcmp(p, changes)) {
- DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
- DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
- } else if (!strcmp(p, lines)) {
- DIFF_OPT_SET(options, DIRSTAT_BY_LINE);
- DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
- } else if (!strcmp(p, files)) {
- DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
- DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
- } else if (!strcmp(p, noncumulative)) {
- DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
- } else if (!strcmp(p, cumulative)) {
- DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
- } else if (isdigit(*p)) {
+ if (!isdigit(*p)) {
+ if (!strcmp(p, changes)) {
+ DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
+ DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
+ } else if (!strcmp(p, lines)) {
+ DIFF_OPT_SET(options, DIRSTAT_BY_LINE);
+ DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
+ } else if (!strcmp(p, files)) {
+ DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
+ DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
+ } else if (!strcmp(p, noncumulative)) {
+ DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
+ } else if (!strcmp(p, cumulative)) {
+ DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
+ } else {
+ strbuf_addf(errmsg, _( Unknown dirstat
parameter '%s'\n), p);
+ ret++;
+ }
+ } else {
char *end;
int permille = strtoul(p, end, 10) * 10;
if (*end == '.' isdigit(*++end)) {
@@ -114,11 +119,7 @@ static int parse_dirstat_params(struct diff_options
*options, const char *params
p);
ret++;
}
- } else {
- strbuf_addf(errmsg, _( Unknown dirstat parameter
'%s'\n), p);
- ret++;
}
-
}
string_list_clear(params, 0);
free(params_copy);
--
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: [GSOC 2014]idea:Git Configuration API Improvement
Matthieu Moy matthieu@grenoble-inp.fr writes: Why? (In general, explaining why you chose something is more important than explaining what you chose) Good educational comment. Thanks. A tree (AST, Abstract syntax tree) can be interesting if you have some source-to-source transformations to do on the configuration files (i.e. edit the config files themselves). For read-only accesses, I would find it more natural to have a data-structure that reflects the configuration variables themselves, not the way they appear in the config file. ... and one important thing that was left unsaid is that the read-only accesses happen far more often than updates, so the data structure must be optimized for the read-only look-up case. -- 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 3/8] ls_colors.c: enable coloring on u+x files
Duy Nguyen pclo...@gmail.com writes: On Thu, Mar 20, 2014 at 6:46 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: git-compat-util.h does not seem to carry S_IXUGO. Anyway as far as Git is concerned, we only care one executable bit. Hard code it. Why not use S_IXUSR instead of a hardcoded value? (already used in path.c, so shouldn't be a problem wrt portability) Hmm..maybe cache.h does something to that macro. Will drop this patch and include cache.h. Why even include cache.h for S_IXUSR? In the context of the patch I see S_ISGID mentioned and other S_* st_mode things are already in use in this function before this step, and presumably you are using them without problems, no? -- 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: disable translation when --porcelain is used
Matthieu Moy matthieu@imag.fr writes:
git status --branch --porcelain displays the status of the branch
(ahead, behind, gone), and used gettext to translate the string.
Use hardcoded strings when --porcelain is used, but keep the gettext
translation for git status --short which is essentially the same, but
meant to be read by a human.
Reported-by: Anarky ghostana...@gmail.com
Signed-off-by: Matthieu Moy matthieu@imag.fr
---
The porcelain format of git status is described as not based on user
configuration.
But with --branch, behind/ahead are translated following the user's locale.
Is it normal that scripts need to take care of that?
Indeed, I'd call that a bug. Here's a fix.
Good thing to fix. Thanks.
wt-status.c | 15 ++-
wt-status.h | 1 +
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index a452407..e55e5b9 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1509,19 +1509,23 @@ static void wt_shortstatus_print_tracking(struct
wt_status *s)
return;
}
+ const char *gone = s-no_gettext ? gone : _(gone);
+ const char *behind = s-no_gettext ? behind : _(behind );
+ const char *ahead = s-no_gettext ? ahead : _(ahead );
Having to repeat the same string constant twice (and a half for the
variable name) each is an eyesore. I wonder if we can do better,
perhaps with:
#define LABEL(string) (s-no_gettext ? (string) : _(string))
and then
color_fprintf(s-fp, header_color, LABEL(N_(gone)));
or something along those lines?
color_fprintf(s-fp, header_color, [);
if (upstream_is_gone) {
- color_fprintf(s-fp, header_color, _(gone));
+ color_fprintf(s-fp, header_color, gone);
} else if (!num_ours) {
- color_fprintf(s-fp, header_color, _(behind ));
+ color_fprintf(s-fp, header_color, behind);
color_fprintf(s-fp, branch_color_remote, %d, num_theirs);
} else if (!num_theirs) {
- color_fprintf(s-fp, header_color, _(ahead ));
+ color_fprintf(s-fp, header_color, ahead);
color_fprintf(s-fp, branch_color_local, %d, num_ours);
} else {
- color_fprintf(s-fp, header_color, _(ahead ));
+ color_fprintf(s-fp, header_color, ahead);
color_fprintf(s-fp, branch_color_local, %d, num_ours);
- color_fprintf(s-fp, header_color, _(, behind ));
+ color_fprintf(s-fp, header_color, , %s, behind);
color_fprintf(s-fp, branch_color_remote, %d, num_theirs);
}
@@ -1566,5 +1570,6 @@ void wt_porcelain_print(struct wt_status *s)
s-use_color = 0;
s-relative_paths = 0;
s-prefix = NULL;
+ s-no_gettext = 1;
wt_shortstatus_print(s);
}
diff --git a/wt-status.h b/wt-status.h
index 30a4812..82f6ce6 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -50,6 +50,7 @@ struct wt_status {
enum commit_whence whence;
int nowarn;
int use_color;
+ int no_gettext;
int display_comment_prefix;
int relative_paths;
int submodule_summary;
--
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 v3] Documentation/gitk: Document new config file location
Astril Hayato astrilhay...@gmail.com writes: User config file location now complies with the XDG base directory specification Signed-off-by: Astril Hayato astrilhay...@gmail.com --- Documentation/gitk.txt | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt index 1e9e38a..7e03fcc 100644 --- a/Documentation/gitk.txt +++ b/Documentation/gitk.txt @@ -166,8 +166,14 @@ gitk --max-count=100 --all \-- Makefile:: Files - -Gitk creates the .gitk file in your $HOME directory to store preferences -such as display options, font, and colors. +User configuration and preferences are stored at: + +* '$XDG_CONFIG_HOME/git/gitk' if it exists, otherwise +* '$HOME/.gitk' if it exists + +If neither of the above exist then '$XDG_CONFIG_HOME/git/gitk' is created and +used by default. If '$XDG_CONFIG_HOME' is not set it defaults to +'$HOME/.config' in all cases. Thanks; looks good. -- 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 v3][GSOC] fsck: use bitwise-or assignment operator to set flag
Hiroyuki Sano sh19910...@gmail.com writes:
fsck_tree() has two different ways to set a flag,
which are the followings:
1. Using a if-statement that guards assignment.
2. Using a bitwise-or assignment operator.
Currently, many with the former way,
and one with the latter way.
In this patch, unify them to the latter way,
because it makes the code shorter and easier to read,
and it is brief and to the point.
Two issues:
* In this patch, is redundant.
* it is brief and to the point are equally applicable to both
styles, so that is not a *reason* to choose one over the other.
If a condition were *not* brief and to the point, then a rewrite to
the latter style will make the resulting code worse:
if (a very complex condition
that potentially have to consume a
lot of brain-cycles to understand) {
has_that_condition = 1;
}
is a lot easier to extend than
has_that_condition = (a very complex condition
that potentially have to consume a
lot of brain-cycles to understand);
because it is a lot more likely that we would need to later extend
such a complex condition is more likely than a simple singleton
condition, and we could end up with
if (a very complex condition
that potentially have to consume a
lot of brain-cycles to understand) {
futher computation to check if
the condition really holds
will be added here later
if (does that condition really hold true?)
has_that_condition = 1;
}
which may be harder to express in the latter form.
In other words, it is brief and to the point merely _allows_ these
statements to be expressed in the latter form; it does not say
anything about which is better between the former and the latter.
Signed-off-by: Hiroyuki Sano sh19910...@gmail.com
---
fsck.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/fsck.c b/fsck.c
index b3022ad..abed62b 100644
--- a/fsck.c
+++ b/fsck.c
@@ -165,18 +165,12 @@ static int fsck_tree(struct tree *item, int strict,
fsck_error error_func)
sha1 = tree_entry_extract(desc, name, mode);
- if (is_null_sha1(sha1))
- has_null_sha1 = 1;
- if (strchr(name, '/'))
- has_full_path = 1;
- if (!*name)
- has_empty_name = 1;
- if (!strcmp(name, .))
- has_dot = 1;
- if (!strcmp(name, ..))
- has_dotdot = 1;
- if (!strcmp(name, .git))
- has_dotgit = 1;
+ has_null_sha1 |= is_null_sha1(sha1);
+ has_full_path |= !!strchr(name, '/');
+ has_empty_name |= !*name;
+ has_dot |= !strcmp(name, .);
+ has_dotdot |= !strcmp(name, ..);
+ has_dotgit |= !strcmp(name, .git);
has_zero_pad |= *(char *)desc.buffer == '0';
update_tree_entry(desc);
--
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 v3] rev-parse --parseopt: option argument name hints
Ilya Bobyr ilya.bo...@gmail.com writes:
Built-in commands can specify names for option arguments when usage text
is generated for a command. sh based commands should be able to do the
same.
Option argument name hint is any text that comes after [*=?!] after the
argument name up to the first whitespace. Underscores are replaced with
whitespace. It is unlikely that an underscore would be useful in the
hint text.
Signed-off-by: Ilya Bobyr ilya.bo...@gmail.com
---
Changed according to the last comments. Added Usage text paragraph in the
documentation and updated variable names.
Documentation/git-rev-parse.txt | 34 --
builtin/rev-parse.c | 17 -
t/t1502-rev-parse-parseopt.sh | 20
3 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 0d2cdcd..b8aabc9 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -284,13 +284,13 @@ Input Format
'git rev-parse --parseopt' input format is fully text based. It has two
parts,
separated by a line that contains only `--`. The lines before the separator
-(should be more than one) are used for the usage.
+(should be one or more) are used for the usage.
The lines after the separator describe the options.
Each line of options has this format:
-opt_specflags* SP+ help LF
+opt_specflags*arg_hint? SP+ help LF
`opt_spec`::
@@ -313,6 +313,12 @@ Each line of options has this format:
* Use `!` to not make the corresponding negated long option available.
+`arg_hint`::
+ `arg_hing`, if specified, is used as a name of the argument in the
+ help output, for options that take arguments. `arg_hint` is
+ terminated by the first whitespace. When output the name is shown in
+ angle braces. Underscore symbols are replaced with spaces.
The last part is troubling (and sounds not very sane). Do we do
such a munging anywhere else, or is it just here? If the latter I'd
prefer not to see such a hack.
@@ -333,6 +339,8 @@ h,helpshow the help
foo some nifty option --foo
bar= some cool option --bar with an argument
+baz=arg another cool option --baz with a named argument
+qux?path qux may take a path argument but has meaning by itself
An option group Header
C?option C with an optional argument
@@ -340,6 +348,28 @@ C?option C with an optional argument
eval $(echo $OPTS_SPEC | git rev-parse --parseopt -- $@ || echo exit
$?)
+
+Usage text
+~~
+
+When $@ is -h or --help the above example would produce the following
+usage text:
Sounds like a good idea to add this; all the above arguments inside
double quotes should be typeset `as-typed`, though.
@@ -419,6 +420,20 @@ static int cmd_parseopt(int argc, const char **argv,
const char *prefix)
o-value = parsed;
o-flags = PARSE_OPT_NOARG;
o-callback = parseopt_dump;
+
+ /* Possible argument name hint */
+ end = s;
+ while (s sb.buf strchr(*=?!, s[-1]) == NULL)
+ --s;
+ if (s != sb.buf s != end) {
+ char *a;
+ o-argh = a = xmemdupz(s, end - s);
+ while (a = strchr(a, '_'))
+ *a = ' ';
... and without the underscore munging, we do not have to allocate
a new piece of memory, either.
--
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 v3][GSOC] fsck: use bitwise-or assignment operator to set flag
Junio C Hamano gits...@pobox.com writes: In other words, it is brief and to the point merely _allows_ these statements to be expressed in the latter form; it does not say anything about which is better between the former and the latter. In any case, that is a minor point. I'll queue the patch on 'pu', with a tweaked log message. 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 v3 2/2] log: add --show-linear-break to help see non-linear history
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: Option explanation is in rev-list-options.txt. The interaction with -z is left undecided. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- Thanks. * Revert back to the old option name --show-linear-break * Get rid of saved_linear, use another flag in struct object instead I cannot offhand say if I like this change or not. A flag bit is a scarce and limited resource; commit slabs felt more suited for implementation of corner case eye-candies. * Fix not showing the break bar after a root commit if the dag graph has multiple roots I definitely do not like the way a commit-list data structure is abused to hold a phoney element that points at a NULL with its item pointer. Allocate a single bit in revs that says I haven't done anything yet if you want to catch the first-ness without breaking what commit_list_insert() and friends are expecting to see---they never expect to see a NULL asked to be on the list, AFAIK. * Make it work with --graph (although I don't really see the point of using both at the same time) I do not see the point, either. I vaguely recall that the previous iteration refused the combination at the option parser level, which I think would be the right thing to do. * Let the next contributor deal with -z That is fine. -- 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] Make XDF_NEED_MINIMAL default in blame.
Michael Andreen h...@ruin.nu writes: There hasn't been any arguments against this patch. Just updated the message with a note about --no-minimal. There hasn't been any argument for this patch, either. It is not like we are still in year 2007; timing result in a small project like Git itself is not a good enough argument to change a well established default at this late in the game, especially when there are ways like command line options for users to specify their preferred settings. -- 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] configure.ac: link with -liconv for locale_charset()
Дилян Палаузов dilyan.palau...@aegee.org writes: diff --git a/Makefile b/Makefile index dddaf4f..dce4694 100644 --- a/Makefile +++ b/Makefile @@ -59,9 +59,9 @@ all:: # FreeBSD can use either, but MinGW and some others need to use # libcharset.h's locale_charset() instead. # -# Define CHARSET_LIB to you need to link with library other than -liconv to +# Define CHARSET_LIB to the library you need to link with in order to # use locale_charset() function. On some platforms this needs to set to -# -lcharset +# -lcharset, on others to -liconv . # # Define LIBC_CONTAINS_LIBINTL if your gettext implementation doesn't # need -lintl when linking. This was unfortunately lost in the noise and I missed it. I think the updated text is much more clear than the original. 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: What's cooking in git.git (Mar 2014, #04; Thu, 20)
Junio C Hamano gits...@pobox.com writes:
Quite a few topics are still outside 'pu' and I suspect some of the
larger ones deserve deeper reviews to help moving them to 'next'.
In principle, I'd prefer to keep any large topic that touch core
part of the system cooking in 'next' for at least a full cycle, and
the sooner they get merged to 'next', the better. Help is greatly
appreciated.
...
* ks/tree-diff-nway (2014-03-04) 19 commits
- combine-diff: speed it up, by using multiparent diff tree-walker directly
- tree-diff: rework diff_tree() to generate diffs for multiparent cases as
well
- Portable alloca for Git
- tree-diff: reuse base str(buf) memory on sub-tree recursion
- tree-diff: no need to call full diff_tree_sha1 from show_path()
- tree-diff: rework diff_tree interface to be sha1 based
- tree-diff: diff_tree() should now be static
- tree-diff: remove special-case diff-emitting code for empty-tree cases
- tree-diff: simplify tree_entry_pathcmp
- tree-diff: show_path prototype is not needed anymore
- tree-diff: rename compare_tree_entry - tree_entry_pathcmp
- tree-diff: move all action-taking code out of compare_tree_entry()
- tree-diff: don't assume compare_tree_entry() returns -1,0,1
- tree-diff: consolidate code for emitting diffs and recursion in one place
- tree-diff: show_tree() is not needed
- tree-diff: no need to pass match to skip_uninteresting()
- tree-diff: no need to manually verify that there is no mode change for a
path
- combine-diff: move changed-paths scanning logic into its own function
- combine-diff: move show_log_first logic/action out of paths scanning
Instead of running N pair-wise diff-trees when inspecting a
N-parent merge, find the set of paths that were touched by walking
N+1 trees in parallel. These set of paths can then be turned into
N pair-wise diff-tree results to be processed through rename
detections and such. And N=2 case nicely degenerates to the usual
2-way diff-tree, which is very nice.
So I started re-reading this series, and decided that it might be
easier to advance the topic piece-by-piece. Will be merging up to
consolidate code for emitting diffs to 'next', after tweaking that
last commit a bit (see below).
Kirill Smelkov k...@mns.spb.ru writes:
Currently both compare_tree_entry() and show_path() invoke opt diff
s/show_path/show_entry/;
callbacks (opt-add_remove() and opt-change()), and also they both have
code which decides whether to recurse into sub-tree, and whether to emit
a tree as separate entry if DIFF_OPT_TREE_IN_RECURSIVE is set.
I.e. we have code duplication and logic scattered on two places.
Let's consolidate it...
...
+/* convert path, t1/t2 - opt-diff_*() callbacks */
+static void emit_diff(struct diff_options *opt, struct strbuf *path,
+ struct tree_desc *t1, struct tree_desc *t2)
+{
+ unsigned int mode1 = t1 ? t1-entry.mode : 0;
+ unsigned int mode2 = t2 ? t2-entry.mode : 0;
+
+ if (mode1 mode2) {
+ opt-change(opt, mode1, mode2, t1-entry.sha1, t2-entry.sha1,
+ 1, 1, path-buf, 0, 0);
This is not too bad per-se, but it would have been even better if
this part was done as:
if (t1 t2) {
opt-change(opt, t1-entry.mode1, t1-entry.mode2,
t1-entry.sha1, t2-entry.sha1,
1, 1, path-buf, 0, 0);
}
Then we do not have to rely on an extra convention, 'mode == 0'
means the entry is missing, in addition to what we already have,
i.e. t == NULL means the entry is missing.
This is minor, so I will not squash such a change in while merging
to 'next', but we may want to revisit and fix it up as a follow up
patch once the series is settled.
+ }
+ else {
Style; I've merged these two lines into one, i.e.
} else {
There was another instance of it in show_path(), which I also
tweaked.
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 v2] Bump core.deltaBaseCacheLimit to 128MiB
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: Duy Nguyen pclo...@gmail.com writes: The only downside I see is large blobs will be packed undeltified, which could increase pack size if you have lots of them. I think that is something that can be tweaked, unless the user tells us otherwise via command line override, when running the improved gc --aggressive ;-) deltaBaseCacheLimit is used for unpacking, not for packing. Hmm, doesn't packing need to read existing data? -- 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, #04; Thu, 20)
Torsten Bögershausen tbo...@web.de writes: On 03/20/2014 10:09 PM, Junio C Hamano wrote: * ap/remote-hg-skip-null-bookmarks (2014-03-19) 1 commit - remote-hg: do not fail on invalid bookmarks Will merge to 'next'. Hmm, am I the only one who has 11 failures in test-hg-hg-git.sh, Still? I thought I pushed out an updated version. like this: (Tested under Debian 7, commit 04570b241ddb53ad2f355977bae541bd7ff32af5) master cde5672] clear executable bit Author: A U Thor aut...@example.com 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100755 = 100644 alpha WARNING: Ignoring invalid bookmark 'master' To hg::../hgrepo-git ! [remote rejected] master - master error: failed to push some refs to 'hg::../hgrepo-git' not ok 1 - executable bit [] # failed 11 among 11 test(s) 1..11 -- 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: [PATCHv2] branch.c: simplify chain of if statements
Dragos Foianu dragos.foi...@gmail.com writes: I'm not sure it's worth pursuing the table approach further, especially since a solution has already been accepted and merged into the codebase. Yes. I would further say that you already qualify as having finished a microproject, if I were a part of the candidate selection panel. The important thing is for potential candidates to learn the process, not to have their change merged somewhere my tree, and you and many others who did a microproject and tasted the process of proposing a change, getting reviewed and learning what are expected of their patch submissions have finished that part already. -- 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 v5] use starts_with() instead of !memcmp()
Eric Sunshine sunsh...@sunshineco.com writes: On Tue, Mar 18, 2014 at 9:18 PM, Quint Guvernator quintus.pub...@gmail.com wrote: Another version, this time very in line with the review and commentary of Junio, Eric, and Michael. This version boasts a revamped commit message and fewer but surer hunks changed. Explaining what changed in this version is indeed a courtesy to reviewers. Thanks. So, is that a reviewed-by: Eric? -- 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 v3] rev-parse --parseopt: option argument name hints
Ilya Bobyr ilya.bo...@gmail.com writes: + `arg_hing`, if specified, is used as a name of the argument in the + help output, for options that take arguments. `arg_hint` is + terminated by the first whitespace. When output the name is shown in + angle braces. Underscore symbols are replaced with spaces. The last part is troubling (and sounds not very sane). Do we do such a munging anywhere else, or is it just here? If the latter I'd prefer not to see such a hack. The following commands have spaces in argument names in the -h output for one or two arguments: * clone * commit * merge A number of commands use dashes to separate words in arguments names. That was not what I asked. I was asking if there is a precedent to use you cannot have underscores in hint; they will be turned into spaces quoting convention. I do not think of any (we either do a backslash-quote, c-quote inside dq-pair, or %20, depending on the context). Personally, because these hints are not even hints (they are more like placeholders for value that makes it easier to refer to in the description of an option [*1*]), I wouldn't shed tears if scripted Porcelains cannot use a space in the argh. In fact, it probably makes the result harder to read and format more funnily if you had a space in the argh string, be it in a subcommand implemented in C or in a scripted Porcelain. An optional argh is terminated by a whitespace is perfectly fine, and by doing so we do not have to worry about having to introduce a new quoting convention like you did, which is a big plus. [Footnote] *1* Perhaps like this: --gpg-sign[=key-id] Sign (with the key specified with key-id) -- 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] Selection of the verbose message is replaced with generated message in install_branch_config()
Eric Sunshine sunsh...@sunshineco.com writes: Sorry, you're right about message[0] case not being a crasher (though the assert() still seems overkill). Assert() often becomes no-op in production build. I think this may be an indication that table-driven may not be as good an approach as many candidates thought. The microproject suggestion asks them to think _if_ that makes sense, and it is perfectly fine for them if they answer no, it introduces more problems than it solves. -- 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: Configuring a third-party git hook
Chris Angelico ros...@gmail.com writes: On Fri, Mar 21, 2014 at 2:43 PM, Jeff King p...@peff.net wrote: Thanks, the new text looks good to me. Please follow SubmittingPatches (notably, you need to sign-off your work, and please send patches inline rather than as attachments). Ah, didn't see that file. It appears that we might need to be more explicit in that file, though. From 6e1fc126ece37c6201d0c16b76c6c87781f7b02b Mon Sep 17 00:00:00 2001 Never paste the above line to your e-mail message. It is only used to separate individual messages/patches in the format-patch output. From: Chris Angelico ros...@gmail.com Date: Fri, 21 Mar 2014 10:45:08 +1100 Subject: [PATCH] Explain that third-party tools may create 'git config' variables You _may_ paste these in-body pseudo-header lines at the beginning of your e-mail but (1) then these must be the first lines of your message, not after doing random discussions at the beginning of the message (you may separate that with scissors marker -- 8 --, though), and (2) do so only they are used to correct what appears in the real header lines in your e-mail message. * From: is useful only when you are forwarding a patch written by somebody else; otherwise your authorship can be taken from the e-mail From: header. * Date: is the same way; Date : header in your e-mail is closer to the time wider world saw the change for the first time than when you made the commit, so it is usually not desired to see in-body pseudo-header. * Subject: is used a lot more often than the above two, especially when you send a patch to an on-going discussion thread as a how about doing it this way? patch and do not want to change the e-mail Subject: (which may break the discussion thread). Also I'd title the commit with the area it touches, i.e. starting it with Explain blah is suboptimal. Will queue with a minor tweak, with retitling the change and rephrasing the ideally part, which invites people to say well it may be so in the ideal world but the rule does not apply to me. Thanks. -- 8 -- From: Chris Angelico ros...@gmail.com Date: Fri, 21 Mar 2014 15:07:08 +1100 Subject: [PATCH] config.txt: third-party tools may and do use their own variables Signed-off-by: Chris Angelico ros...@gmail.com Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/config.txt | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index ab26963..a1ea605 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -131,8 +131,13 @@ Variables Note that this list is non-comprehensive and not necessarily complete. For command-specific variables, you will find a more detailed description -in the appropriate manual page. You will find a description of non-core -porcelain configuration variables in the respective porcelain documentation. +in the appropriate manual page. + +Other git-related tools may and do use their own variables. When +inventing new variables for use in your own tool, make sure their +names do not conflict with what are used by Git itself and other +popular tools, and describe them in your documentation. + advice.*:: These variables control various optional help messages designed to -- 1.9.1-443-g8f4a3d9 -- 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: disable translation when --porcelain is used
Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@pobox.com writes: diff --git a/wt-status.c b/wt-status.c index a452407..e55e5b9 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1509,19 +1509,23 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) return; } + const char *gone = s-no_gettext ? gone : _(gone); + const char *behind = s-no_gettext ? behind : _(behind ); + const char *ahead = s-no_gettext ? ahead : _(ahead ); Having to repeat the same string constant twice (and a half for the variable name) each is an eyesore. I wonder if we can do better, perhaps with: #define LABEL(string) (s-no_gettext ? (string) : _(string)) and then color_fprintf(s-fp, header_color, LABEL(N_(gone))); or something along those lines? I first thought about trying something clever with the preprocessor, but since it's only for 3 strings, I went the KISS way. I tend to prefer my version for simplicity, but no strong opinion here. Then I'll squash 61bf9709 (SQUASH??? fix decl-after-stmt and simplify, 2014-03-20) in before merging the patch to 'next'. -- 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] [GSoC] Draft of Proposal for GSoC
Brian Bourn ba.bo...@gmail.com writes: Something like this? Sample api calls Add_Opt_Group() Parse_with_contains() Parse_with_merged() Parse_with_no_merged() Parse_with_formatting() (each of the 4 calls above may have internal calls within the library in order to parse the option for each of the different function which may call these functions) This list is a bit too sketchy to be called sample api calls, at least to me. Can you elaborate a bit more? What do they do, what does the caller expect to see (do they get something as return values? do they expect some side effects?)? -- 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 3/4] Fix misuses of nor in comments
Justin Lebar jle...@google.com writes: Thanks for the quick reply. When I send a new patch, should I fold these changes into the original commit, or should I send them as a separate commit? diff --git a/builtin/apply.c b/builtin/apply.c index b0d0986..6013e19 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -4061,7 +4061,7 @@ static int write_out_one_reject(struct patch *patch) return error(_(cannot open %s: %s), namebuf, strerror(errno)); /* Normal git tools never deal with .rej, so do not pretend -* this is a git patch by saying --git nor give extended +* this is a git patch by saying --git or giving extended * headers. While at it, maybe please kompare that wants * the trailing TAB and some garbage at the end of line ;-). */ I don't think the change from give to giving here is grammatically correct. Is it? I might be misunderstanding the sentence, then. I parse the new sentence as... The new sentence should say what the original wanted to say, which I think was: - Do not pretend this is a git patch by saying --git - Do not show extended headers. I however think that extended headers is one attribute of a patch being a git patch, so I would say that the break down of your new version: Do not pretend this is a git patch by - saying --git, or - giving extended headers. makes sense. -- 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 3/4] Fix misuses of nor in comments
Justin Lebar jle...@google.com writes: Thanks for the quick reply. When I send a new patch, should I fold these changes into the original commit, or should I send them as a separate commit? While a patch is still in an early discussion stage, consider their earlier incarnation rejected and send them afresh with [PATCH v2] (or v3, v4,...) when rerolling. When you do this kind of tree-wide clean-up, please make sure that your patch applies cleanly to 'maint', 'master', 'next' and 'pu' branches, to check if you are touching some area that are undergoing other changes. If you find conflicts, please remove overlapping parts from your main patch, make the removed parts into separate patches that can be applied on top once these other topics that are in flight are ready to be merged. 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] doc: status, remove leftover statement about '#' prefix
Dirk Wallenstein hals...@t-online.de writes: This hasn't been true since 2556b9962e7c0353d562b7bf70eed11d8f29d0b0 Signed-off-by: Dirk Wallenstein hals...@t-online.de --- Good eyes. Thanks. Documentation/git-status.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index a4acaa0..def635f 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -97,7 +97,7 @@ configuration variable documented in linkgit:git-config[1]. OUTPUT -- The output from this command is designed to be used as a commit -template comment, and all the output lines are prefixed with '#'. +template comment. The default, long format, is designed to be human readable, verbose and descriptive. Its contents and format are subject to change at any time. -- 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: File extension conflict when working with git and latex
Matthias Beyer m...@beyermatthias.de writes: I know, I can fix this by fixing the clean task in my Makefile. But maybe someone somewhere on this world doesn't know the git internals as good as me (and, of course, my coworker). Is there _any chance at all_ that this gets mentioned somewhere, so others don't fall into this pit? Surely, we are here to please ;-) All of us want to make sure newbies do not shoot themselves in the foot. But the problem is what exactly should be mentioned. With a fresh wound with your LaTeX project still in your mind, you may be tempted to special case .idx, but other newbies may inflict the same kind of hurt on themselves with different find patterns, e.g. $ find . -name '[0-9a-f]*[0-9a-f]' -type f -print | xargs rm -f when they know their project creates hexadecimal-numbered temoprary files, or whatever other pattern that match the files they do not care about, that also happens to match whatever is in $GIT_DIR. The only common caution that helps us to make sure others do not fall into this pit is Files and directories in $GIT_DIR are used to record your work; do not muck with them unless you know what you are doing e.g. manually repairing a corrupt repository, but that is a bit lame, isn't it? It is tempting to suggest git clean '*.idx', but that is a good fit in the Makefile only when you know everybody involved in the project works in a checkout from Git, not from a tarball extract, and does not apply to projects in general. -- 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] Rewrite fsck.c:fsck_commit() replace memcmp() with starts_with()
blacksimit cengoguzhanu...@gmail.com writes:
From: Oguzhan Unlu cengoguzhanu...@gmail.com
My solution to make lines containing buffer += a_number; clearer to anyone is
following; I defined a new int, magic_num, then assigned lengths of used
strings to magic_num and then changed assignment lines through using
magic_num so that where the number which is added to buffer is known although
I eliminated 3rd parameter of memcmp() when using starts_with().
Eric told you in $gmane/244637:
Wrap messages to 65-70 characters.
I do not think replacing 5 (or 7) with a variable makes anything
clearer; in fact, by forcing people to check what the last value
assigned to the variable every time they see +magic_num, the
resulting code is much harder to read, I would think.
Among the various submissions, I found this one one of the cleaner
solutions:
http://thread.gmane.org/gmane.comp.version-control.git/244019/focus=244020
and it has been queued as 2d820a61 (fsck.c:fsck_commit(): use
skip_prefix() to verify and skip constant, 2014-03-13) on 'pu'.
Signed-off-by: Oguzhan Unlu cengoguzhanu...@gmail.com
---
I didn't use skip_prefix() in this microproject and I plan to apply for GSOC
2014. I expect your feedbacks!
fsck.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/fsck.c b/fsck.c
index d43be98..4e5ca30 100644
--- a/fsck.c
+++ b/fsck.c
@@ -289,14 +289,17 @@ static int fsck_commit(struct commit *commit,
fsck_error error_func)
struct commit_graft *graft;
int parents = 0;
int err;
-
+int magic_num;
+
+magic_num = strlen(tree ); /* magic_num is 5 */
if (!starts_with(buffer, tree ))
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+magic_num, tree_sha1) || buffer[45] != '\n')
return error_func(commit-object, FSCK_ERROR, invalid 'tree'
line format - bad sha1);
buffer += 46;
+magic_num = strlen(parent ); /* magic_num is 7 */
while (starts_with(buffer, parent )) {
- if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
+ if (get_sha1_hex(buffer+magic_num, sha1) || buffer[47] != '\n')
return error_func(commit-object, FSCK_ERROR, invalid
'parent' line format - bad sha1);
buffer += 48;
parents++;
@@ -322,15 +325,17 @@ static int fsck_commit(struct commit *commit,
fsck_error error_func)
if (p || parents)
return error_func(commit-object, FSCK_ERROR, parent
objects missing);
}
+magic_num = strlen(author ); /* magic_num is 7 */
if (!starts_with(buffer, author ))
return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'author' line);
- buffer += 7;
+ buffer += magic_num;
err = fsck_ident(buffer, commit-object, error_func);
if (err)
return err;
+magic_num = strlen(committer); /* magic_num is 7 */
if (!starts_with(buffer, committer ))
return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'committer' line);
- buffer += strlen(committer );
+ buffer += magic_num;
err = fsck_ident(buffer, commit-object, error_func);
if (err)
return err;
--
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: Configuring a third-party git hook
Jeff King p...@peff.net writes: On Fri, Mar 21, 2014 at 10:31:59AM -0700, Junio C Hamano wrote: -- 8 -- From: Chris Angelico ros...@gmail.com Date: Fri, 21 Mar 2014 15:07:08 +1100 Subject: [PATCH] config.txt: third-party tools may and do use their own variables [...] +Other git-related tools may and do use their own variables. When +inventing new variables for use in your own tool, make sure their +names do not conflict with what are used by Git itself and other +popular tools, and describe them in your documentation. I think this third line should be with what _is_ used to match the verb and noun pluralness[1]. Or to keep better parallel structure with the first clause, something like ...their names do not conflict with those that are used by Git Thanks. I'll amend to do the those that are. -- 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: Configuring a third-party git hook
Jeff King p...@peff.net writes: [1] Is there a word to mean the pluralness of a noun or verb (similar to tense for a verb). I've seen plural vs singular often mentioned in the context of subject and verb agreement. en.wiktionary.org/wiki/concord talks about agreement in gender, number, person, or case, so number may be the word you are looking for. http://en.wikipedia.org/wiki/Grammatical_number Surely there is, but I could not think of it. I wanted to say here that the pluralness of what and are does not match (it seems like what is a mass noun, which usually matches a singular verb). -- 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, #04; Thu, 20)
Torsten Bögershausen tbo...@web.de writes: On 03/20/2014 10:09 PM, Junio C Hamano wrote: * ap/remote-hg-skip-null-bookmarks (2014-03-19) 1 commit - remote-hg: do not fail on invalid bookmarks Will merge to 'next'. Hmm, am I the only one who has 11 failures in test-hg-hg-git.sh, like this: (Tested under Debian 7, commit 04570b241ddb53ad2f355977bae541bd7ff32af5) master cde5672] clear executable bit Author: A U Thor aut...@example.com 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100755 = 100644 alpha WARNING: Ignoring invalid bookmark 'master' To hg::../hgrepo-git ! [remote rejected] master - master error: failed to push some refs to 'hg::../hgrepo-git' not ok 1 - executable bit [] # failed 11 among 11 test(s) 1..11 This has again been replaced; I'll queue it as d06f17f8 (remote-hg: do not fail on invalid bookmarks, 2014-03-21) on 'pu'. Please holler if this still breaks for you. 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 03/12] t: drop useless sane_unset GIT_* calls
Jeff King p...@peff.net writes:
Several test scripts manually unset GIT_CONFIG and other
GIT_* variables. These are generally taken care of for us by
test-lib.sh already.
Unsetting these is not only useless, but can be confusing to
a reader, who may wonder why some tests in a script unset
them and others do not (t0001 is particularly guilty of this
inconsistency, probably because many of its tests predate
the test-lib.sh environment-cleansing).
Note that we cannot always get rid of such unsetting. For
example, t9130 can drop the GIT_CONFIG unset, but not the
GIT_DIR one, because lib-git-svn.sh sets the latter. And in
t1000, we unset GIT_TEMPLATE_DIR, which is explicitly
initialized by test-lib.sh.
Signed-off-by: Jeff King p...@peff.net
---
I suppose one could make an argument that test-lib.sh may later change
the set of variables it clears, and these unsets are documenting an
explicit need of each test. I'd find that more compelling if it were
actually applied consistently.
Hmph. I am looking at git show HEAD^:t/t0001-init.sh after
applying this patch, and it does look consistently done with
GIT_CONFIG and GIT_DIR (I am not sure about GIT_WORK_TREE but from a
cursory read it is done consistently for tests on non-bare
repositories).
So I would actually agree with your alternative interpretation
Unsetting these is useless, but it does serve documentation
purpose---without having to see what the state of the environment
when the subprocess is started, the reader can understand what is
being tested, rather than the one in the log message.
Having said that, I am perfectly OK with the change to t0001 in this
patch, if we added at the very beginning of the test sequence a
comment that says:
Below, creation and use of repositories are tested with various
combinations of environment settings and command line flags.
They are done inside subshells to avoid leaking temporary
environment settings to later tests *and* assumes that the
initial environment does not have have GIT_DIR, GIT_CONFIG, and
GIT_WORK_TREE defined.
or something.
t/t0001-init.sh | 15 ---
t/t9130-git-svn-authors-file.sh | 1 -
t/t9400-git-cvsserver-server.sh | 1 -
3 files changed, 17 deletions(-)
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 9fb582b..ddc8160 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -25,7 +25,6 @@ check_config () {
test_expect_success 'plain' '
(
- sane_unset GIT_DIR GIT_WORK_TREE
mkdir plain
cd plain
git init
@@ -35,7 +34,6 @@ test_expect_success 'plain' '
test_expect_success 'plain nested in bare' '
(
- sane_unset GIT_DIR GIT_WORK_TREE
git init --bare bare-ancestor.git
cd bare-ancestor.git
mkdir plain-nested
@@ -47,7 +45,6 @@ test_expect_success 'plain nested in bare' '
test_expect_success 'plain through aliased command, outside any git repo' '
(
- sane_unset GIT_DIR GIT_WORK_TREE
HOME=$(pwd)/alias-config
export HOME
mkdir alias-config
@@ -65,7 +62,6 @@ test_expect_success 'plain through aliased command, outside
any git repo' '
test_expect_failure 'plain nested through aliased command' '
(
- sane_unset GIT_DIR GIT_WORK_TREE
git init plain-ancestor-aliased
cd plain-ancestor-aliased
echo [alias] aliasedinit = init .git/config
@@ -78,7 +74,6 @@ test_expect_failure 'plain nested through aliased command' '
test_expect_failure 'plain nested in bare through aliased command' '
(
- sane_unset GIT_DIR GIT_WORK_TREE
git init --bare bare-ancestor-aliased.git
cd bare-ancestor-aliased.git
echo [alias] aliasedinit = init config
@@ -91,7 +86,6 @@ test_expect_failure 'plain nested in bare through aliased
command' '
test_expect_success 'plain with GIT_WORK_TREE' '
if (
- sane_unset GIT_DIR
mkdir plain-wt
cd plain-wt
GIT_WORK_TREE=$(pwd) git init
@@ -104,7 +98,6 @@ test_expect_success 'plain with GIT_WORK_TREE' '
test_expect_success 'plain bare' '
(
- sane_unset GIT_DIR GIT_WORK_TREE GIT_CONFIG
mkdir plain-bare-1
cd plain-bare-1
git --bare init
@@ -114,7 +107,6 @@ test_expect_success 'plain bare' '
test_expect_success 'plain bare with GIT_WORK_TREE' '
if (
- sane_unset GIT_DIR GIT_CONFIG
mkdir plain-bare-2
cd plain-bare-2
GIT_WORK_TREE=$(pwd) git --bare init
@@ -128,7 +120,6 @@ test_expect_success 'plain bare with GIT_WORK_TREE' '
test_expect_success 'GIT_DIR bare' '
(
- sane_unset GIT_CONFIG
mkdir
Re: [PATCH 04/12] t: stop using GIT_CONFIG to cross repo boundaries
Jeff King p...@peff.net writes:
Some tests want to check or set config in another
repository. E.g., t1000 creates repositories and makes sure
that their core.bare and core.worktree settings are what we
expect. We can do this with:
GIT_CONFIG=$repo/.git/config git config ...
but it better shows the intent to just enter the repository
and let git config do the normal lookups:
(cd $repo git config ...)
In theory, this would cause us to use an extra subshell, but
in all such cases, we are actually already in a subshell.
Sure; alternatively we could use git -C $there, but this rewrite
is fine by me.
Thanks.
Signed-off-by: Jeff King p...@peff.net
---
t/t0001-init.sh| 4 ++--
t/t5701-clone-local.sh | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index ddc8160..9b05fdf 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -12,8 +12,8 @@ check_config () {
echo expected a directory $1, a file $1/config and $1/refs
return 1
fi
- bare=$(GIT_CONFIG=$1/config git config --bool core.bare)
- worktree=$(GIT_CONFIG=$1/config git config core.worktree) ||
+ bare=$(cd $1 git config --bool core.bare)
+ worktree=$(cd $1 git config core.worktree) ||
worktree=unset
test $bare = $2 test $worktree = $3 || {
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index c490368..3c087e9 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -12,8 +12,8 @@ test_expect_success 'preparing origin repository' '
: file git add . git commit -m1
git clone --bare . a.git
git clone --bare . x
- test $(GIT_CONFIG=a.git/config git config --bool core.bare) = true
- test $(GIT_CONFIG=x/config git config --bool core.bare) = true
+ test $(cd a.git git config --bool core.bare) = true
+ test $(cd x git config --bool core.bare) = true
git bundle create b1.bundle --all
git bundle create b2.bundle master
mkdir dir
@@ -24,7 +24,7 @@ test_expect_success 'preparing origin repository' '
test_expect_success 'local clone without .git suffix' '
git clone -l -s a b
(cd b
- test $(GIT_CONFIG=.git/config git config --bool core.bare) = false
+ test $(git config --bool core.bare) = false
git fetch)
'
--
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, #04; Thu, 20)
Junio C Hamano gits...@pobox.com writes: Torsten Bögershausen tbo...@web.de writes: On 03/20/2014 10:09 PM, Junio C Hamano wrote: * ap/remote-hg-skip-null-bookmarks (2014-03-19) 1 commit - remote-hg: do not fail on invalid bookmarks Will merge to 'next'. Hmm, am I the only one who has 11 failures in test-hg-hg-git.sh, like this: (Tested under Debian 7, commit 04570b241ddb53ad2f355977bae541bd7ff32af5) master cde5672] clear executable bit Author: A U Thor aut...@example.com 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100755 = 100644 alpha WARNING: Ignoring invalid bookmark 'master' To hg::../hgrepo-git ! [remote rejected] master - master error: failed to push some refs to 'hg::../hgrepo-git' not ok 1 - executable bit [] # failed 11 among 11 test(s) 1..11 This has again been replaced; I'll queue it as d06f17f8 (remote-hg: do not fail on invalid bookmarks, 2014-03-21) on 'pu'. Please holler if this still breaks for you. Thanks. Ah, you seem to have sent a good review on that patch while I was looking at other topics ;-) And they all make sense, I think. 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 03/10] t4018: an infrastructure to test hunk headers
Johannes Sixt j...@kdbg.org writes:
Add an infrastructure that simplifies adding new tests of the hunk
header regular expressions.
To add new tests, a file with the syntax to test can be dropped in the
directory t4018. The README file explains how a test file must contain;
s/how/what/, or how a test file must be written you mean?
the README itself tests the default behavior.
Thanks. Looks like a reasonable way to mark what must be found.
Signed-off-by: Johannes Sixt j...@kdbg.org
---
t/t4018-diff-funcname.sh | 60
+++-
t/t4018/README | 18 +++
2 files changed, 72 insertions(+), 6 deletions(-)
create mode 100644 t/t4018/README
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 38a092a..b467d9e 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -100,7 +100,25 @@ test_expect_funcname () {
grep ^@@.*@@ $1 diff
}
-for p in ada bibtex cpp csharp fortran html java matlab objc pascal perl php
python ruby tex
+diffpatterns=
+ ada
+ bibtex
+ cpp
+ csharp
+ fortran
+ html
+ java
+ matlab
+ objc
+ pascal
+ perl
+ php
+ python
+ ruby
+ tex
+
+
+for p in $diffpatterns
do
test_expect_success builtin $p pattern compiles '
echo *.java diff=$p .gitattributes
I always found this Let's apply rules for language $p to these
*.java files strange. I have wonder if it makes sense to further
change the framework to read the name of the rule to be applied from
the file in t/t4018/ directory, instead of using filename that is
the same as the name of the rule? That way, you can list the files
in t/t4018/ directory to come up with the above list, without having
to maintain the list of rules separately like the above.
@@ -118,11 +136,6 @@ do
'
done
-test_expect_success 'default behaviour' '
- rm -f .gitattributes
- test_expect_funcname public class Beer\$
-'
-
test_expect_success 'set up .gitattributes declaring drivers to test' '
cat .gitattributes -\EOF
*.java diff=java
@@ -182,4 +195,39 @@ test_expect_success 'alternation in pattern' '
test_expect_funcname public static void main(
'
+test_expect_success 'setup hunk header tests' '
+ for i in $diffpatterns
+ do
+ echo $i-* diff=$i
+ done .gitattributes
I like that you can have more than one test for each language/rule
this way, allowing you to test one kind of breakage without getting
affected by lines prepared for other tests in the same file.
+ # add all test files to the index
+ (
+ cd $TEST_DIRECTORY/t4018
+ git --git-dir=$TRASH_DIRECTORY/.git add .
+ )
+
+ # place modified files in the worktree
+ for i in $(git ls-files)
+ do
+ sed -e s/ChangeMe/IWasChanged/ $TEST_DIRECTORY/t4018/$i
$i || return 1
+ done
+'
+
+# check each individual file
+for i in $(git ls-files)
+do
+ if grep broken $i /dev/null 21
+ then
+ result=failure
+ else
+ result=success
+ fi
+ test_expect_$result hunk header: $i
+ test_when_finished 'cat actual' # for debugging only
+ git diff -U1 $i actual
+ grep '@@ .* @@.*RIGHT' actual
+
+done
+
test_done
diff --git a/t/t4018/README b/t/t4018/README
new file mode 100644
index 000..283e01cc
--- /dev/null
+++ b/t/t4018/README
@@ -0,0 +1,18 @@
+How to write RIGHT test cases
+=
+
+Insert the word ChangeMe (exactly this form) at a distance of
+at least two lines from the line that must appear in the hunk header.
+
+The text that must appear in the hunk header must contain the word
+right, but in all upper-case, like in the title above.
+
+To mark a test case that highlights a malfunction, insert the word
+BROKEN in all lower-case somewhere in the file.
+
+This text is a bit twisted and out of order, but it is itself a
+test case for the default hunk header pattern. Know what you are doing
+if you change it.
+
+BTW, this tests that the head line goes to the hunk header, not the line
+of equal signs.
--
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 00/10] userdiff: cpp pattern simplification and test framework
Thanks; will replace jk/diff-funcname-cpp-regex with this series. -- 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 v3] remote-hg: do not fail on invalid bookmarks
Max Horn m...@quendi.de writes: Hi Torsten, On 21.03.2014, at 21:47, Torsten Bögershausen tbo...@web.de wrote: On 2014-03-21 12.36, Max Horn wrote: All tests passed :-), Excellent. thanks from my side. comments inline, some are debatable Thanks for having a close look and for the constructive feedback! Unfortunately, I won't have time to look into this for the next 7 days or so. I wouldn't mind if the patch gets queued with the changes you suggest; but of course that might be a tad too much to ask for, so I'll also be happy to do a proper re-roll, but then it has to wait a bit. In the meantime, I'll pile this on top as SQUASH???. I am not sure how the original, which went into a subdirectory gitrepo that is to be cleaned with test_when_finished, was working. Perhaps it didn't clean and dug the trash directory hierarchy deeper and deeper, or something? contrib/remote-helpers/test-hg.sh | 80 +-- 1 file changed, 43 insertions(+), 37 deletions(-) diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh index 6925ca3..8834482 100755 --- a/contrib/remote-helpers/test-hg.sh +++ b/contrib/remote-helpers/test-hg.sh @@ -694,68 +694,74 @@ test_expect_success 'remote double failed push' ' test_expect_success 'clone remote with master null bookmark, then push to the bookmark' ' test_when_finished rm -rf gitrepo* hgrepo* - ( hg init hgrepo - cd hgrepo - echo a a - hg add a - hg commit -m a - hg bookmark -r null master + ( + cd hgrepo + echo a a + hg add a + hg commit -m a + hg bookmark -r null master ) git clone hg::hgrepo gitrepo check gitrepo HEAD a - cd gitrepo - git checkout --quiet -b master - echo b b - git add b - git commit -m b - git push origin master + ( + cd gitrepo + git checkout --quiet -b master + echo b b + git add b + git commit -m b + git push origin master + ) ' test_expect_success 'clone remote with default null bookmark, then push to the bookmark' ' test_when_finished rm -rf gitrepo* hgrepo* - ( hg init hgrepo - cd hgrepo - echo a a - hg add a - hg commit -m a - hg bookmark -r null -f default + ( + cd hgrepo + echo a a + hg add a + hg commit -m a + hg bookmark -r null -f default ) git clone hg::hgrepo gitrepo check gitrepo HEAD a - cd gitrepo - git checkout --quiet -b default - echo b b - git add b - git commit -m b - git push origin default + ( + cd gitrepo + git checkout --quiet -b default + echo b b + git add b + git commit -m b + git push origin default + ) ' test_expect_success 'clone remote with generic null bookmark, then push to the bookmark' ' test_when_finished rm -rf gitrepo* hgrepo* - ( hg init hgrepo - cd hgrepo - echo a a - hg add a - hg commit -m a - hg bookmark -r null bmark + ( + cd hgrepo + echo a a + hg add a + hg commit -m a + hg bookmark -r null bmark ) git clone hg::hgrepo gitrepo check gitrepo HEAD a - cd gitrepo - git checkout --quiet -b bmark - git remote -v - echo b b - git add b - git commit -m b - git push origin bmark + ( + cd gitrepo + git checkout --quiet -b bmark + git remote -v + echo b b + git add b + git commit -m b + git push origin bmark + ) ' test_done -- 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 03/10] t4018: an infrastructure to test hunk headers
Johannes Sixt j...@kdbg.org writes: I see what you mean. Notice, however, that we also test whether the regular expressions can be compiled successfully, and for this it is desirable to have the complete list of userdiff drivers. Until we have at least one test-case for each driver, we wouldn't get the complete list. I missed the fact that we lack here is a pair of test files in this lang $p; go find diff between them and make sure we have the right hunk header for many $p. So the kind of improvements I wondered about are allowed to come later, but shouldn't be done in this series, especiallly not at this step in the series. 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] Clarify pre-push hook documentation
David Cowden dco...@gmail.com writes: The documentation as-is does not mention that the pre-push hook is executed even when there is nothing to push. This can lead a new reader to beilieve there will always be lines fed to the script's standerd input and cause minor confusion as to what is happening when there are no lines provided to the pre-push script. Signed-off-by: David Cowden dco...@gmail.com --- The everything is up to date; no need to have any data sent back to the other end is one case two readers of the documentation may guess what should happen, one thinking we know nothing is pushed---there is no need to even call pre-push, the other thinking we should always call pre-push, and tell it what will be pushed, in this particular case, nothing. It is a good change to clarify that ambiguous expectation with the new paragraph. Aren't there other cases that can invite ambuguous expectations in a similar way? For example, when there are differences between what they have and what we are updating it with but the update does not fast-forward? +The hook is executed regardless of whether there are changes to push or not. +In the event that there are no changes, no data will be provided on the +script's standard input. What I am tryihng to get at is if whether there are changes to push or not is covering only too narrow a subset of cases where the readers may suffer from their expectations. 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
[PATCH 2/3] update-index: teach --cacheinfo a new syntax mode,sha1,path
The --cacheinfo option is unusual in that it takes three option
parameters. An option with an optional parameter is bad enough. An
option with multiple parameters is simply insane.
Introduce a new syntax that takes these three things concatenated
together with a comma, which makes the command line syntax more
uniform across subcommands, while retaining the traditional syntax
for backward compatiblity.
If we were designing the update-index subcommand from scratch
today, it may probably have made sense to make this option (and
possibly others) a command mode option that does not take any option
parameter (hence no need for arg-help). But we do not live in such
an ideal world, and as far as I can tell, the command still supports
(and must support) mixed command modes in a single invocation, e.g.
$ git update-index path1 --add path2 \
--cacheinfo 100644 $(git hash-object --stdin -w path3) path3 \
path4
must make sure path1 is already in the index and update all of these
four paths. So this is probably as far as we can go to fix this issue
without risking to break people's existing scripts.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
Documentation/git-update-index.txt | 8 ++--
builtin/update-index.c | 34 +++---
t/t2107-update-index-basic.sh | 13 +
3 files changed, 50 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-update-index.txt
b/Documentation/git-update-index.txt
index e0a8702..d6de4a0 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -12,7 +12,7 @@ SYNOPSIS
'git update-index'
[--add] [--remove | --force-remove] [--replace]
[--refresh] [-q] [--unmerged] [--ignore-missing]
-[(--cacheinfo mode object file)...]
+[(--cacheinfo mode,object,file)...]
[--chmod=(+|-)x]
[--[no-]assume-unchanged]
[--[no-]skip-worktree]
@@ -68,8 +68,12 @@ OPTIONS
--ignore-missing::
Ignores missing files during a --refresh
+--cacheinfo mode,object,path::
--cacheinfo mode object path::
- Directly insert the specified info into the index.
+ Directly insert the specified info into the index. For
+ backward compatibility, you can also give these three
+ arguments as three separate parameters, but new users are
+ encouraged to use a single-parameter form.
--index-info::
Read index information from stdin.
diff --git a/builtin/update-index.c b/builtin/update-index.c
index d12ad95..ba54e19 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -629,14 +629,42 @@ static int resolve_undo_clear_callback(const struct
option *opt,
return 0;
}
+static int parse_new_style_cacheinfo(const char *arg,
+unsigned int *mode,
+unsigned char sha1[],
+const char **path)
+{
+ unsigned long ul;
+ char *endp;
+
+ errno = 0;
+ ul = strtoul(arg, endp, 8);
+ if (errno || endp == arg || *endp != ',' || (unsigned int) ul != ul)
+ return -1; /* not a new-style cacheinfo */
+ *mode = ul;
+ endp++;
+ if (get_sha1_hex(endp, sha1) || endp[40] != ',')
+ return -1;
+ *path = endp + 41;
+ return 0;
+}
+
static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,
const struct option *opt, int unset)
{
unsigned char sha1[20];
unsigned int mode;
+ const char *path;
+ if (!parse_new_style_cacheinfo(ctx-argv[1], mode, sha1, path)) {
+ if (add_cacheinfo(mode, sha1, path, 0))
+ die(git update-index: --cacheinfo cannot add %s,
path);
+ ctx-argv++;
+ ctx-argc--;
+ return 0;
+ }
if (ctx-argc = 3)
- return error(option 'cacheinfo' expects three arguments);
+ return error(option 'cacheinfo' expects mode,sha1,path);
if (strtoul_ui(*++ctx-argv, 8, mode) ||
get_sha1_hex(*++ctx-argv, sha1) ||
add_cacheinfo(mode, sha1, *++ctx-argv, 0))
@@ -740,9 +768,9 @@ int cmd_update_index(int argc, const char **argv, const
char *prefix)
PARSE_OPT_NOARG | PARSE_OPT_NONEG,
really_refresh_callback},
{OPTION_LOWLEVEL_CALLBACK, 0, cacheinfo, NULL,
- N_(mode object path),
+ N_(mode,object,path),
N_(add the specified entry to the index),
- PARSE_OPT_NOARG | /* disallow --cacheinfo=mode
form */
+ PARSE_OPT_NOARG | /* disallow --cacheinfo=mode form */
PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
(parse_opt_cb *) cacheinfo_callback
[PATCH 3/3] parse-options: make sure argh string does not have SP or _
We encourage to spell an argument hint that consists of multiple words as a single-token separated with dashes. In order to help catching violations added by new callers of parse-options, make sure argh does not contain SP or _ when the code validates the option definitions. Signed-off-by: Junio C Hamano gits...@pobox.com --- parse-options.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/parse-options.c b/parse-options.c index a5fa0b8..c81d3a0 100644 --- a/parse-options.c +++ b/parse-options.c @@ -375,6 +375,9 @@ static void parse_options_check(const struct option *opts) default: ; /* ok. (usually accepts an argument) */ } + if (opts-argh + strcspn(opts-argh, _) != strlen(opts-argh)) + err |= optbug(opts, multi-word argh should use dash to separate words); } if (err) exit(128); -- 1.9.1-471-gcccbd8b -- 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 04/10] t4209: use helper functions to test --grep
René Scharfe l@web.de writes: -test_expect_success 'log --grep -i' ' - git log -i --grep=InItial --format=%H actual - test_cmp expect_initial actual -' +test_log expect_initial --grep initial +test_log expect_nomatch --grep InItial This, and the next --author one, assumes that we will never break --grep=foo without breaking --grep foo. That should be OK, but we might want to add separate tests e.g. test_log expect_initial --grep=initial perhaps? I dunno. Queued without any tweaks for now. 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 03/10] t4209: factor out helper function test_log_icase()
René Scharfe l@web.de writes:
Reduce code duplication by introducing test_log_icase() that runs the
same test with both --regexp-ignore-case and -i. The specification of
the four basic test scenarios (matching/nomatching combined with case
sensitive/insensitive) becomes easier to read and write.
Signed-off-by: Rene Scharfe l@web.de
---
t/t4209-log-pickaxe.sh | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index 9f3bb40..dd911c2 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -25,6 +25,11 @@ test_log() {
}
+test_log_icase() {
+ test_log $@ --regexp-ignore-case
+ test_log $@ -i
-cascade broken? Will squash in an obvious fix.
+}
--
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 00/10] pickaxe: honor -i when used with -S and --pickaxe-regex; cleanups
René Scharfe l@web.de writes: This series allows the options -i/--regexp-ignore-case, --pickaxe-regex, and -S to be used together and work as expected to perform a pickaxe search using case-insensitive regular expression matching. Its first half refactors the test script and extends test coverage a bit while we're at it. The actual change is in the sixth patch. It enables the two following cleanups. The last two patches are independent simple cleanups. Very nice. 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 2/2] fetch: handle overlaping refspecs on --prune
Carlos Martín Nieto c...@elego.de writes:
From: Carlos Martín Nieto c...@dwim.me
We need to consider that a remote-tracking branch may match more than
one rhs of a fetch refspec. In such a case, it is not enough to stop at
the first match but look at all of the matches in order to determine
whether a head is stale.
To this goal, introduce a variant of query_refspecs which returns all of
the matching refspecs and loop over those answers to check for
staleness.
Signed-off-by: Carlos Martín Nieto c...@elego.de
---
There is an unfortunate duplication of code here, as
query_refspecs_multiple is mostly query_refspecs but we only care
about the other side of matching refspecs and disregard the 'force'
information which query_refspecs does want.
I thought about putting both together via callbacks and having
query_refspecs stop at the first one, but I'm not sure that it would
make it easier to read or manage.
Sorry for a belated review.
I agree with your analysis of the root-cause of the symptom exposed
by new tests in [1/2] and the proposed solution.
@@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname,
const unsigned char *sha1, int flags, void *cb_data)
{
struct stale_heads_info *info = cb_data;
+ struct string_list matches = STRING_LIST_INIT_DUP;
struct refspec query;
+ int i, stale = 1;
memset(query, 0, sizeof(struct refspec));
query.dst = (char *)refname;
- if (query_refspecs(info-refs, info-ref_count, query))
+ query_refspecs_multiple(info-refs, info-ref_count, query, matches);
+ if (matches.nr == 0)
return 0; /* No matches */
/*
* If we did find a suitable refspec and it's not a symref and
* it's not in the list of refs that currently exist in that
- * remote we consider it to be stale.
+ * remote we consider it to be stale. In order to deal with
+ * overlapping refspecs, we need to go over all of the
+ * matching refs.
*/
- if (!((flags REF_ISSYMREF) ||
- string_list_has_string(info-ref_names, query.src))) {
+ if (flags REF_ISSYMREF)
+ return 0;
Who frees matches? At this point matches.nr != 0 so there must be
something we need to free, no?
+ for (i = 0; i matches.nr; i++) {
+ if (string_list_has_string(info-ref_names,
matches.items[i].string)) {
+ stale = 0;
+ break;
+ }
+ }
+
+ string_list_clear(matches, 0);
+
+ if (stale) {
struct ref *ref = make_linked_ref(refname,
info-stale_refs_tail);
hashcpy(ref-new_sha1, sha1);
}
- free(query.src);
In the new code, query_refspecs_multiple() uses the result allocated
by match_name_with_pattern() to the results list, taking it out of
query.src without copying, so losing this free() is the right thing
to do---matches must be cleared.
And string_list matches is initialized as INIT_DUP, so we can rely
on string_list_clear() to free these strings.
return 0;
}
Regarding the seemingly duplicated logic in the new function, I
wonder if the callers of non-duplicated variant may benefit if they
notice there are multiple hits, even if they cannot use more than
one in their context. That is, what would happen if we changed
these callers to instead of calling query-refspecs call the multi
variant, and if that call finds multiple matches, do something about
it (e.g. warn if they use the first hit because they are not
acting on later hits, possibly losing information)?
Here is a minor clean-ups, both to fix style and plug leaks, that
can be squashed to this patch. How does it look?
Thanks.
remote.c | 20
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/remote.c b/remote.c
index 26140c7..fde7b52 100644
--- a/remote.c
+++ b/remote.c
@@ -839,9 +839,8 @@ static void query_refspecs_multiple(struct refspec *refs,
int ref_count, struct
if (!refspec-dst)
continue;
if (refspec-pattern) {
- if (match_name_with_pattern(key, needle, value,
result)) {
+ if (match_name_with_pattern(key, needle, value, result))
string_list_append_nodup(results, *result);
- }
} else if (!strcmp(needle, key)) {
string_list_append(results, value);
}
@@ -1989,32 +1988,29 @@ static int get_stale_heads_cb(const char *refname,
query_refspecs_multiple(info-refs, info-ref_count, query, matches);
if (matches.nr == 0)
- return 0; /* No matches */
+ goto clean_exit; /* No matches */
/*
* If we did find a suitable refspec and it's not a symref and
* it's not in the list of refs that currently exist in that
-* remote we
What's cooking in git.git (Mar 2014, #05; Mon, 24)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. More topics merged to 'master', some of which have been cooking before the v1.9.0 final release, many of them fallouts from GSoC microprojects. Many topics that have been marked to be discarded are finally discarded. There seems to be a crasher somewhere in the new pack bitmap codepath that was introduced recently. I am hoping that the root cause is found and fixed soonish. Other than that, things look more or less calm on the 'next' and up. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to master] * dk/skip-prefix-scan-only-once (2014-03-03) 1 commit (merged to 'next' on 2014-03-14 at ff375fc) + skip_prefix(): scan prefix only once Update implementation of skip_prefix() to scan only once; given that most prefix arguments to the inline function are constant strings whose strlen() can be determined at the compile time, this might actually make things worse with a compiler with sufficient intelligence. * es/sh-i18n-envsubst (2014-03-12) 1 commit (merged to 'next' on 2014-03-14 at e4d5603) + sh-i18n--envsubst: retire unused string_list_member() * jc/stash-pop-not-popped (2014-02-26) 1 commit (merged to 'next' on 2014-03-14 at 9ba1de8) + stash pop: mention we did not drop the stash upon failing to apply stash pop, upon failing to apply the stash, refrains from discarding the stash to avoid information loss. Be more explicit in the error message. The wording may want to get a bit more bikeshedding. * jk/shallow-update-fix (2014-03-17) 3 commits (merged to 'next' on 2014-03-17 at 011942e) + shallow: verify shallow file after taking lock (merged to 'next' on 2014-03-12 at ce5abbf) + shallow: automatically clean up shallow tempfiles + shallow: use stat_validity to check for up-to-date file Serving objects from a shallow repository needs to write a new file to hold the temporary shallow boundaries but it was not cleaned when we exit due to die() or a signal. * jn/wt-status (2014-03-12) 4 commits (merged to 'next' on 2014-03-14 at 8ac862c) + wt-status: lift the artificual at least 20 columns floor + wt-status: i18n of section labels + wt-status: extract the code to compute width for labels + wt-status: make full label string to be subject to l10n Unify the codepaths that format new/modified/changed sections and conflicted paths in the git status output and make it possible to properly internationalize their output. * lt/request-pull (2014-03-13) 6 commits (merged to 'next' on 2014-03-17 at 21a598d) + request-pull: documentation updates + request-pull: resurrect pretty refname feature + request-pull: test updates + request-pull: pick up tag message as before + request-pull: allow local:remote to specify names on both ends + request-pull: more strictly match local/remote branches Discard the accumulated heuristics to guess from which branch the result wants to be pulled from and make sure what the end user specified is not second-guessed by git request-pull, to avoid mistakes. * nd/tag-version-sort (2014-02-27) 1 commit (merged to 'next' on 2014-03-14 at 4e7f714) + tag: support --sort=spec Allow v1.9.0 sorted before v1.10.0 in git tag --list output. * nd/upload-pack-shallow (2014-03-11) 1 commit (merged to 'next' on 2014-03-14 at d40b8c3) + upload-pack: send shallow info over stdin to pack-objects Serving objects from a shallow repository needs to write a temporary file to be used, but the serving upload-pack may not have write access to the repository which is meant to be read-only. Instead feed these temporary shallow bounds from the standard input of pack-objects so that we do not have to use a temporary file. * tc/commit-dry-run-exit-status-tests (2014-02-24) 1 commit (merged to 'next' on 2014-03-12 at b839886) + demonstrate git-commit --dry-run exit code behaviour -- [New Topics] * ca/doc-config-third-party (2014-03-21) 1 commit - config.txt: third-party tools may and do use their own variables Will merge to 'next'. * dw/doc-status-no-longer-shows-pound-prefix (2014-03-21) 1 commit - doc: status, remove leftover statement about '#' prefix Will merge to 'next'. * js/userdiff-cc (2014-03-21) 10 commits - userdiff: have 'cpp' hunk header pattern catch more C++ anchor points - t4018: test cases showing that the cpp pattern misses many anchor points - t4018: test cases for the built-in cpp pattern - t4018: reduce test files for pattern compilation tests - t4018: convert custom pattern test to the new infrastructure - t4018: convert java pattern test to the new infrastructure - t4018: convert perl pattern tests to the new
Re: with reuse-delta patches, fetching with bitmaps segfaults due to possibly incomplete bitmap traverse
Jeff King p...@peff.net writes: On Fri, Mar 21, 2014 at 07:58:55PM -0700, Siddharth Agarwal wrote: At Facebook we've found that fetch speed is a bottleneck for our Git repos, so we've been looking to deploy bitmaps to speed up fetches. We've been trying out git-next with the top two patches from https://github.com/peff/git/commits/jk/bitmap-reuse-delta, but the following is reproducible with tip of that branch, currently 81cdec2. Is it also reproducible just with the tip of next? Note that the patches in jk/bitmap-reuse-delta have not been widely deployed (in particular, we are not yet using them at GitHub, and we track segfaults on our servers closely and have not seen any related to this). Nice to hear. I was worried for a short while if I merged what was not cooked well, before I realized that Siddharth is on a codebase that is more bleeding edge than I use myself ;-) -- 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 12/19] tree-diff: remove special-case diff-emitting code for empty-tree cases
Kirill Smelkov k...@mns.spb.ru writes:
via teaching tree_entry_pathcmp() how to compare empty tree descriptors:
Drop this line, as you explain the pretend empty compares bigger
than anything else idea later anyway? This early part of the
proposed log message made me hiccup while reading it.
While walking trees, we iterate their entries from lowest to highest in
sort order, so empty tree means all entries were already went over.
If we artificially assign +infinity value to such tree entry, it will
go after all usual entries, and through the usual driver loop we will be
taking the same actions, which were hand-coded for special cases, i.e.
t1 empty, t2 non-empty
pathcmp(+∞, t2) - +1
show_path(/*t1=*/NULL, t2); /* = t1 t2 case in main loop */
t1 non-empty, t2-empty
pathcmp(t1, +∞) - -1
show_path(t1, /*t2=*/NULL); /* = t1 t2 case in main loop */
Sounds good. I would have phrased a bit differently, though:
When we have T1 and T2, we return a sign that tells the caller
to indicate the earlier one to be emitted, and by returning
the sign that causes the non-empty side to be emitted, we will
automatically cause the entries from the remaining side to be
emitted, without attempting to touch the empty side at all. We
can teach tree_entry_pathcmp() to pretend that an empty tree has
an element that sorts after anything else to achieve this.
without saying infinity.
Right now we never go to when compared tree descriptors are infinity,...
Sorry, but I cannot parse this.
as
this condition is checked in the loop beginning as finishing criteria,
What condition and which loop? The loop that immediately surrounds
the callsite of tree_entry_pathcmp() is the infinite for (;;) { loop,
and after it prepares t1 and t2 by skipping paths outside pathspec,
we check if both are empty (i.e. we ran out). Is that the condition
you are referring to?
but will do in the future, when there will be several parents iterated
simultaneously, and some pair of them would run to the end.
Signed-off-by: Kirill Smelkov k...@mns.spb.ru
Signed-off-by: Junio C Hamano gits...@pobox.com
---
( re-posting without change )
tree-diff.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/tree-diff.c b/tree-diff.c
index cf96ad7..2fd6d0e 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -12,12 +12,19 @@
*
* NOTE files and directories *always* compare differently, even when having
* the same name - thanks to base_name_compare().
+ *
+ * NOTE empty (=invalid) descriptor(s) take part in comparison as +infty.
The basic idea is very sane. It is a nice (and obvious---once you
are told about the trick) and clean restructuring of the code.
*/
static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)
{
struct name_entry *e1, *e2;
int cmp;
+ if (!t1-size)
+ return t2-size ? +1 /* +∞ c */ : 0 /* +∞ = +∞ */;
+ else if (!t2-size)
+ return -1; /* c +∞ */
Where do these c come from? I somehow feel that these comments
are making it harder to understand what is going on.
e1 = t1-entry;
e2 = t2-entry;
cmp = base_name_compare(e1-path, tree_entry_len(e1), e1-mode,
@@ -151,18 +158,8 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
skip_uninteresting(t1, base, opt);
skip_uninteresting(t2, base, opt);
}
- if (!t1-size) {
- if (!t2-size)
- break;
- show_path(base, opt, /*t1=*/NULL, t2);
- update_tree_entry(t2);
- continue;
- }
- if (!t2-size) {
- show_path(base, opt, t1, /*t2=*/NULL);
- update_tree_entry(t1);
- continue;
- }
+ if (!t1-size !t2-size)
+ break;
cmp = tree_entry_pathcmp(t1, t2);
--
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 11/19] tree-diff: simplify tree_entry_pathcmp
Kirill Smelkov k...@mns.spb.ru writes:
Since an earlier Finally switch over tree descriptors to contain a
pre-parsed entry, we can safely access all tree_desc-entry fields
directly instead of first extracting them through
tree_entry_extract.
Use it. The code generated stays the same - only it now visually looks
cleaner.
Signed-off-by: Kirill Smelkov k...@mns.spb.ru
Signed-off-by: Junio C Hamano gits...@pobox.com
---
( re-posting without change )
Thanks.
Hopefully I'll be merging the series up to this point to 'next'
soonish.
tree-diff.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/tree-diff.c b/tree-diff.c
index 20a4fda..cf96ad7 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -15,18 +15,13 @@
*/
static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)
{
- unsigned mode1, mode2;
- const char *path1, *path2;
- const unsigned char *sha1, *sha2;
- int cmp, pathlen1, pathlen2;
+ struct name_entry *e1, *e2;
+ int cmp;
- sha1 = tree_entry_extract(t1, path1, mode1);
- sha2 = tree_entry_extract(t2, path2, mode2);
-
- pathlen1 = tree_entry_len(t1-entry);
- pathlen2 = tree_entry_len(t2-entry);
-
- cmp = base_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2);
+ e1 = t1-entry;
+ e2 = t2-entry;
+ cmp = base_name_compare(e1-path, tree_entry_len(e1), e1-mode,
+ e2-path, tree_entry_len(e2), e2-mode);
return cmp;
}
--
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
