Re: [PATCH] git.txt: update description of the configuration mechanism
Junio C Hamano venit, vidit, dixit 14.02.2013 19:03: Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@pobox.com writes: But the exact location of per-user and per-repository configuration files does not matter in this context and is best left to the git-config documentation. I'm OK with your version. I already queued your original with one s/not/now/; perhaps I will redo it then. Yes, I think the new version improves upon Matthieu's which was a good start to begin with :) Michael -- 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/5] gpg-interface: check good signature in a reliable way
Junio C Hamano venit, vidit, dixit 14.02.2013 18:22:
Michael J Gruber g...@drmicha.warpmail.net writes:
Currently, verify_signed_buffer() only checks the return code of gpg,
and some callers implement additional unreliable checks for Good
signature in the gpg output meant for the user.
Use the status output instead and parse for a line beinning with
[GNUPG:] GOODSIG . This is the only reliable way of checking for a
good gpg signature.
If needed we can change this easily to [GNUPG:] VALIDSIG if we want
to take into account the trust model.
Thanks. I didn't look beyond man gpg nor bother looking at
DETAILS file in its source, which the manpage refers to.
I think GOODSIG is a good starting point. Depending on the context
(e.g. %G?) we may also want to consider EXPSIG (but not EXPKEYSIG
or REVKEYSIG) acceptable, while reading log --show-signature on
ancient part of the history, no?
Yes, we could certainly return a more detailed status to the callers.
Currently, 0 is OK (GOODSIG) and everything else is a fail. We would
need to change the callers to allow more details on the fail as well
as the OK so that they can decide what is good enough, say:
-1: fail for technical reasons (no sig, can't run gpg etc.)
0: sig present bad (cryptographically) BAD
1: REVKEYSIG
2: EXPKEYSIG
3: EXPSIG
4: GOODSIG
5: VALIDSIG
I'd have to recheck whether a bitmask or ordered values make more sense.
Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net
---
gpg-interface.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 4559033..c582b2e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -96,15 +96,17 @@ int sign_buffer(struct strbuf *buffer, struct strbuf
*signature, const char *sig
/*
* Run gpg to see if the payload matches the detached signature.
* gpg_output, when set, receives the diagnostic output from GPG.
+ * gpg_status, when set, receives the status output from GPG.
*/
int verify_signed_buffer(const char *payload, size_t payload_size,
const char *signature, size_t signature_size,
struct strbuf *gpg_output)
{
struct child_process gpg;
-const char *args_gpg[] = {NULL, --verify, FILE, -, NULL};
+const char *args_gpg[] = {NULL, --status-fd=1, --verify, FILE,
-, NULL};
char path[PATH_MAX];
int fd, ret;
+struct strbuf buf = STRBUF_INIT;
args_gpg[0] = gpg_program;
fd = git_mkstemp(path, PATH_MAX, .git_vtag_tmpXX);
@@ -119,9 +121,10 @@ int verify_signed_buffer(const char *payload, size_t
payload_size,
memset(gpg, 0, sizeof(gpg));
gpg.argv = args_gpg;
gpg.in = -1;
+gpg.out = -1;
if (gpg_output)
gpg.err = -1;
-args_gpg[2] = path;
+args_gpg[3] = path;
if (start_command(gpg)) {
unlink(path);
return error(_(could not run gpg.));
@@ -134,9 +137,15 @@ int verify_signed_buffer(const char *payload, size_t
payload_size,
strbuf_read(gpg_output, gpg.err, 0);
close(gpg.err);
}
+strbuf_read(buf, gpg.out, 0);
+close(gpg.out);
+
ret = finish_command(gpg);
unlink_or_warn(path);
+ret |= !strstr(buf.buf, \n[GNUPG:] GOODSIG );
+strbuf_release(buf);
+
return ret;
}
--
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] add: warn when -u or -A is used without filepattern
Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com writes: warning: The behavior of 'git add --update (or -u)' with no path argument from a subdirectory of the tree will change in Git 2.0 and should not be used anymore. There is a logic gap between will change and should not be used that is not filled like the text in the manual page does. I guess it is not so bad after all, if you read the entire message, not just the first two lines. Also, the warning is meant to be read by a user who just typed git add -u, so it is expected that the user knows what it does in current (or past) versions of Git. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4+ 3/4] count-objects: report garbage files in pack directory too
prepare_packed_git_one() is modified to allow count-objects to hook a
report function to so we don't need to duplicate the pack searching
logic in count-objects.c. When report_pack_garbage is NULL, the
overhead is insignificant.
The garbage is reported with warning() instead of error() in packed
garbage case because it's not an error to have garbage. Loose garbage
is still reported as errors and will be converted to warnings later.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/git-count-objects.txt | 4 +-
builtin/count-objects.c | 12 +-
cache.h | 3 ++
sha1_file.c | 83 -
t/t5304-prune.sh| 26
5 files changed, 124 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-count-objects.txt
b/Documentation/git-count-objects.txt
index e816823..1611d7c 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -33,8 +33,8 @@ size-pack: disk space consumed by the packs, in KiB
prune-packable: the number of loose objects that are also present in
the packs. These objects could be pruned using `git prune-packed`.
+
-garbage: the number of files in loose object database that are not
-valid loose objects
+garbage: the number of files in object database that are not valid
+loose objects nor valid packs
GIT
---
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 9afaa88..1706c8b 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -9,6 +9,14 @@
#include builtin.h
#include parse-options.h
+static unsigned long garbage;
+
+static void real_report_garbage(const char *desc, const char *path)
+{
+ warning(%s: %s, desc, path);
+ garbage++;
+}
+
static void count_objects(DIR *d, char *path, int len, int verbose,
unsigned long *loose,
off_t *loose_size,
@@ -76,7 +84,7 @@ int cmd_count_objects(int argc, const char **argv, const char
*prefix)
const char *objdir = get_object_directory();
int len = strlen(objdir);
char *path = xmalloc(len + 50);
- unsigned long loose = 0, packed = 0, packed_loose = 0, garbage = 0;
+ unsigned long loose = 0, packed = 0, packed_loose = 0;
off_t loose_size = 0;
struct option opts[] = {
OPT__VERBOSE(verbose, N_(be verbose)),
@@ -87,6 +95,8 @@ int cmd_count_objects(int argc, const char **argv, const char
*prefix)
/* we do not take arguments other than flags for now */
if (argc)
usage_with_options(count_objects_usage, opts);
+ if (verbose)
+ report_garbage = real_report_garbage;
memcpy(path, objdir, len);
if (len objdir[len-1] != '/')
path[len++] = '/';
diff --git a/cache.h b/cache.h
index 7339f21..73de68c 100644
--- a/cache.h
+++ b/cache.h
@@ -1051,6 +1051,9 @@ extern const char *parse_feature_value(const char
*feature_list, const char *fea
extern struct packed_git *parse_pack_index(unsigned char *sha1, const char
*idx_path);
+/* A hook for count-objects to report invalid files in pack directory */
+extern void (*report_garbage)(const char *desc, const char *path);
+
extern void prepare_packed_git(void);
extern void reprepare_packed_git(void);
extern void install_packed_git(struct packed_git *pack);
diff --git a/sha1_file.c b/sha1_file.c
index 239bee7..16967d3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -21,6 +21,7 @@
#include sha1-lookup.h
#include bulk-checkin.h
#include streaming.h
+#include dir.h
#ifndef O_NOATIME
#if defined(__linux__) (defined(__i386__) || defined(__PPC__))
@@ -1000,6 +1001,63 @@ void install_packed_git(struct packed_git *pack)
packed_git = pack;
}
+void (*report_garbage)(const char *desc, const char *path);
+
+static void report_helper(const struct string_list *list,
+ int seen_bits, int first, int last)
+{
+ const char *msg;
+ switch (seen_bits) {
+ case 0:
+ msg = no corresponding .idx nor .pack;
+ break;
+ case 1:
+ msg = no corresponding .idx;
+ break;
+ case 2:
+ msg = no corresponding .pack;
+ break;
+ default:
+ return;
+ }
+ for (; first last; first++)
+ report_garbage(msg, list-items[first].string);
+}
+
+static void report_pack_garbage(struct string_list *list)
+{
+ int i, baselen = -1, first = 0, seen_bits = 0;
+
+ if (!report_garbage)
+ return;
+
+ sort_string_list(list);
+
+ for (i = 0; i list-nr; i++) {
+ const char *path = list-items[i].string;
+ if (baselen != -1
+ strncmp(path, list-items[first].string, baselen)) {
+ report_helper(list, seen_bits, first, i);
+
Re: [BUG] Veryfing signatures in git log fails when language is not english
2013/2/14 Junio C Hamano gits...@pobox.com: - The right one you mention for %GS is easier than you might think. If you just verify against the accompanying tagger identity, that should be sufficient. It of course cannot be generally solved, as you could tag as person A while signing with key for person B, but a simple social convention would help us out there: if you tag as Mariusz Gronczewski, your signature should also say so. unless there is someone else with same name, which happens more often (so far i've seen it happen twice) than same GPG IDs. It's all fine if you just have one keyring that you can use to validate against all repos but when there are multiple projects each with different persons responsible for deploying it can get messy ;]. my use-case is basically allow only commits signed by person X Y or Z to be deployed on production and allow only persons A, B, C, X, Y, Z to commit, while latter case can be solved by software like gitolite, credential validation is messy at best as you have to validate: - ssh key - if ssh key owner matches commiter name - if commiter name =! author name, if a given person can do that (project architect or some other person accepting patches) or can't and I'm trying to implement GPG signing so if someone does something malicious i can say OK that commit was signed by your key ID, why you did it? -- Mariusz Gronczewski (XANi) xani...@gmail.com GnuPG: 0xEA8ACE64 -- 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] read_directory: avoid invoking exclude machinery on tracked files
read_directory() (and its friendly wrapper fill_directory) collects
untracked/ignored files by traversing through the whole worktree (*),
feeding every entry to treat_one_path(), where each entry is checked
against .gitignore patterns.
One may see that tracked files can't be excluded and we do not need to
run them through exclude machinery. On repos where there are many
.gitignore patterns and/or a lot of tracked files, this unnecessary
processing can become expensive.
This patch avoids it mostly for normal cases. Directories are still
processed as before. DIR_SHOW_IGNORED and DIR_COLLECT_IGNORED are not
normally used unless some options are given (e.g. checkout
--overwrite-ignore, add -f...) so people still need to pay penalty
in some cases, just not as often as before.
git status | webkit linux-2.6 libreoffice-core gentoo-x86
-+--
before | 1.159s0.226s 0.415s 0.597s
after| 0.778s0.176s 0.266s 0.556s
nr. patterns |89 376 19 0
nr. tracked | 182k 40k 63k 101k
(*) Not completely true. read_directory may skip recursing into a
directory if it's entirely excluded and DIR_SHOW_OTHER_DIRECTORIES
is not set.
Tracked-down-by: Karsten Blees karsten.bl...@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
For reference:
http://thread.gmane.org/gmane.comp.version-control.git/215820/focus=216195
dir.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/dir.c b/dir.c
index 57394e4..bdff256 100644
--- a/dir.c
+++ b/dir.c
@@ -1244,7 +1244,19 @@ static enum path_treatment treat_one_path(struct
dir_struct *dir,
const struct path_simplify *simplify,
int dtype, struct dirent *de)
{
- int exclude = is_excluded(dir, path-buf, dtype);
+ int exclude;
+
+ if (dtype == DT_UNKNOWN)
+ dtype = get_dtype(de, path-buf, path-len);
+
+ if (!(dir-flags DIR_SHOW_IGNORED)
+ !(dir-flags DIR_COLLECT_IGNORED)
+ dtype != DT_DIR
+ cache_name_exists(path-buf, path-len, ignore_case))
+ return path_ignored;
+
+ exclude = is_excluded(dir, path-buf, dtype);
+
if (exclude (dir-flags DIR_COLLECT_IGNORED)
exclude_matches_pathspec(path-buf, path-len, simplify))
dir_add_ignored(dir, path-buf, path-len);
@@ -1256,9 +1268,6 @@ static enum path_treatment treat_one_path(struct
dir_struct *dir,
if (exclude !(dir-flags DIR_SHOW_IGNORED))
return path_ignored;
- if (dtype == DT_UNKNOWN)
- dtype = get_dtype(de, path-buf, path-len);
-
switch (dtype) {
default:
return path_ignored;
--
1.8.1.2.536.gf441e6d
--
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: [BUG] Veryfing signatures in git log fails when language is not english
Mariusz Gronczewski xani...@gmail.com writes: 2013/2/14 Junio C Hamano gits...@pobox.com: - The right one you mention for %GS is easier than you might think. If you just verify against the accompanying tagger identity, that should be sufficient. It of course cannot be generally solved, as you could tag as person A while signing with key for person B, but a simple social convention would help us out there: if you tag as Mariusz Gronczewski, your signature should also say so. unless there is someone else with same name, which happens more often (so far i've seen it happen twice) than same GPG IDs. Oh, I didn't mean to say ignore email part, which of course will make the result more likely to be ambiguous. I thought you meant by have to show right one the following scenario: The tag v1.8.1 has a GPG signature. The key 96AFE6CB was used to sign it. The key is associated with more than one identities. One of them is Junio C Hamano gits...@pobox.com, but that is not the only one. I also have combinations of other e-mail addresses and names spelled differently (e.g. Junio Hamano jchx...@gmail.com) that are _not_ associated with that key. GPG may say good signature from A aka B aka C; which one of A, B, or C should we choose? I was suggesting that among the identities associated with the key used to sign the tag, we should show the one that matches the identity on the tagger field. object 5d417842efeafb6e109db7574196901c4e95d273 type commit tag v1.8.1 tagger Junio C Hamano gits...@pobox.com 1356992771 -0800 Git 1.8.1 -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) iQIc... =v706 -END PGP SIGNATURE- Because it is clear from the context where the signature appears that that identity is what matters for me as a signer in the project the tag appears in. I may have other e-mail addresses that are not associated with that key, but it would be insane to put that on the tagger field of the tag, while GPG-signing with that key. -- 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] read_directory: avoid invoking exclude machinery on tracked files
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
read_directory() (and its friendly wrapper fill_directory) collects
untracked/ignored files by traversing through the whole worktree (*),
feeding every entry to treat_one_path(), where each entry is checked
against .gitignore patterns.
One may see that tracked files can't be excluded and we do not need to
run them through exclude machinery. On repos where there are many
.gitignore patterns and/or a lot of tracked files, this unnecessary
processing can become expensive.
This patch avoids it mostly for normal cases. Directories are still
processed as before. DIR_SHOW_IGNORED and DIR_COLLECT_IGNORED are not
normally used unless some options are given (e.g. checkout
--overwrite-ignore, add -f...) so people still need to pay penalty
in some cases, just not as often as before.
git status | webkit linux-2.6 libreoffice-core gentoo-x86
-+--
before | 1.159s0.226s 0.415s 0.597s
after| 0.778s0.176s 0.266s 0.556s
nr. patterns |89 376 19 0
nr. tracked | 182k 40k 63k 101k
(*) Not completely true. read_directory may skip recursing into a
directory if it's entirely excluded and DIR_SHOW_OTHER_DIRECTORIES
is not set.
Tracked-down-by: Karsten Blees karsten.bl...@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
For reference:
http://thread.gmane.org/gmane.comp.version-control.git/215820/focus=216195
dir.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/dir.c b/dir.c
index 57394e4..bdff256 100644
--- a/dir.c
+++ b/dir.c
@@ -1244,7 +1244,19 @@ static enum path_treatment treat_one_path(struct
dir_struct *dir,
const struct path_simplify *simplify,
int dtype, struct dirent *de)
{
- int exclude = is_excluded(dir, path-buf, dtype);
+ int exclude;
+
+ if (dtype == DT_UNKNOWN)
+ dtype = get_dtype(de, path-buf, path-len);
+
+ if (!(dir-flags DIR_SHOW_IGNORED)
+ !(dir-flags DIR_COLLECT_IGNORED)
+ dtype != DT_DIR
+ cache_name_exists(path-buf, path-len, ignore_case))
+ return path_ignored;
+
+ exclude = is_excluded(dir, path-buf, dtype);
+
if (exclude (dir-flags DIR_COLLECT_IGNORED)
exclude_matches_pathspec(path-buf, path-len, simplify))
dir_add_ignored(dir, path-buf, path-len);
Interesting.
In the current code, we always check if a path is excluded, and when
dealing with DT_REG/DT_LNK, we call treat_file():
* When such a path is excluded, treat_file() returns true when we
are not showing ignored directories. This causes treat_one_path()
to return path_ignored, so for excluded DT_REG/DT_LNK paths when
no DIR_*_IGNORED is in effect, this change is a correct
optimization.
* When such a path is not excluded, on the ther hand, and when we
are not showing ignored directories, treat_file() just returns
the value of exclude_file, which is initialized to false and is
not changed in the function. This causes treat_one_path() to
return path_handled. However, the new code returns path_ignored
in this case.
What guarantees that this change is regression free? I do not seem
to be able to find anything that checks if the path is already known
to the index in the original code for the case you special cased
(i.e. DIR_*_IGNORED is not set and dtype is not DT_DIR). Do all the
callers that reach this function in their callgraph, when they get
path_ignored for a path in the index, behave as if the difference
between path_ignored and path_handled does not matter?
@@ -1256,9 +1268,6 @@ static enum path_treatment treat_one_path(struct
dir_struct *dir,
if (exclude !(dir-flags DIR_SHOW_IGNORED))
return path_ignored;
- if (dtype == DT_UNKNOWN)
- dtype = get_dtype(de, path-buf, path-len);
-
switch (dtype) {
default:
return path_ignored;
--
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] read_directory: avoid invoking exclude machinery on tracked files
On Fri, Feb 15, 2013 at 11:52 PM, Junio C Hamano gits...@pobox.com wrote: In the current code, we always check if a path is excluded, and when dealing with DT_REG/DT_LNK, we call treat_file(): * When such a path is excluded, treat_file() returns true when we are not showing ignored directories. This causes treat_one_path() to return path_ignored, so for excluded DT_REG/DT_LNK paths when no DIR_*_IGNORED is in effect, this change is a correct optimization. * When such a path is not excluded, on the ther hand, and when we are not showing ignored directories, treat_file() just returns the value of exclude_file, which is initialized to false and is not changed in the function. This causes treat_one_path() to return path_handled. However, the new code returns path_ignored in this case. What guarantees that this change is regression free? If you consider read_directory_recursive alone, there is a regression. The return value of r_d_r depends on path_handled/path_ignored. With this patch, the return value will be different. The return value is only used by treat_directory() in two cases: - when DIR_SHOW_IGNORED is set, which disables the optimization so no regression - when DIR_HIDE_EMPTY_DIRECTORIES is _not_ set (and neither is DIR_SHOW_IGNORED), the optimization is still on and different r_d_r's return value would lead to different behavior. However I don't think it can happen. treat_directory checks if the given directory can be found in index. In that case the neither r_d_r calls in treat_directory is reachable. If the given directory cannot be found in the index, the second r_d_r is reachable. But then the cache_name_exists() in the patch should always be false (parent not in index, children cannot), so the optimization is off and r_d_r returns correctly. It's a bit tricky. I'm not sure if I miss anything else. I do not seem to be able to find anything that checks if the path is already known to the index in the original code for the case you special cased (i.e. DIR_*_IGNORED is not set and dtype is not DT_DIR). Do all the callers that reach this function in their callgraph, when they get path_ignored for a path in the index, behave as if the difference between path_ignored and path_handled does not matter? -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline
On Thu, Feb 14, 2013 at 9:58 AM, John Keeping j...@keeping.me.uk wrote:
On Tue, Feb 12, 2013 at 02:33:42AM -0800, Brandon Casey wrote:
Teach append_signoff to detect whether a blank line exists at the position
that the signed-off-by line will be added, and refrain from adding an
additional one if one already exists. Or, add an additional line if one
is needed to make sure the new footer is separated from the message body
by a blank line.
Signed-off-by: Brandon Casey bca...@nvidia.com
---
As Jonathan Nieder wondered before [1], this changes the behaviour when
the commit message is empty. Before this commit, there is an empty line
followed by the S-O-B line; now the S-O-B is on the first line of the
commit.
The previous behaviour seems better to me since the empty line is
hinting that the user should fill something in. It looks particularly
strange if your editor has syntax highlighting for commit messages such
that the first line is in a different colour.
[1] http://article.gmane.org/gmane.comp.version-control.git/214796
diff --git a/sequencer.c b/sequencer.c
index 3364faa..084573b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1127,8 +1127,19 @@ void append_signoff(struct strbuf *msgbuf, int
ignore_footer, unsigned flag)
else
has_footer = has_conforming_footer(msgbuf, sob,
ignore_footer);
- if (!has_footer)
- strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0, \n, 1);
+ if (!has_footer) {
+ const char *append_newlines = NULL;
+ size_t len = msgbuf-len - ignore_footer;
+
+ if (len msgbuf-buf[len - 1] != '\n')
+ append_newlines = \n\n;
+ else if (len 1 msgbuf-buf[len - 2] != '\n')
+ append_newlines = \n;
To restore the old behaviour this needs something like this:
else if (!len)
append_newlines = \n;
+ if (append_newlines)
+ strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0,
+ append_newlines, strlen(append_newlines));
+ }
if (has_footer != 3 (!no_dup_sob || has_footer != 2))
strbuf_splice(msgbuf, msgbuf-len - ignore_footer, 0,
Are you talking about the output produced by format-patch? Or are you
talking about what happens when you do 'commit --amend -s' for a
commit with an empty commit message. (The email that you referenced
was about the behavior of format-patch).
I'm thinking you must be talking about the 'commit --amend -s'
behavior since you mentioned your editor. Is there another case that
is affected by this? Normally, any extra blank lines that precede or
follow a commit message are removed before the commit object is
created. So, I guess it wouldn't hurt to insert a newline (or maybe
it should be two?) before the signoff in this case. Would this
provide an improvement or change for any other commands than 'commit
--amend -s'?
If we want to do this, then I'd probably do it like this:
- if (len msgbuf-buf[len - 1] != '\n')
+ if (!len || msgbuf-buf[len - 1] != '\n')
append_newlines = \n\n;
- else if (len 1 msgbuf-buf[len - 2] != '\n')
+ else if (len == 1 || msgbuf-buf[len - 2] != '\n')
append_newlines = \n;
This would ensure there were two newlines preceding the sob. The
editor would place its cursor on the top line where the user should
begin typing in a commit message. If an editor was not opened up
(e.g. if 'git cherry-pick -s --allow-empty-message ...' was used) then
the normal mechanism that removes extra blank lines would trigger to
remove the extra blank lines.
I think that's reasonable.
It seems 'git cherry-pick -s --edit' follows a different code path,
and the commit message is stripped of newlines by 'git commit' before
it is passed to the editor. 'cherry-pick -s --edit' and 'commit
--amend -s' should probably have the same behavior and present the
same buffer to the user for editing when they encounter a commit with
an empty message.
Maybe something like this is enough(?):
diff --git a/builtin/commit.c b/builtin/commit.c
index 7b9e2ac..0796412 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -124,8 +124,10 @@ static int opt_parse_m(const struct option *opt, const char
if (unset)
strbuf_setlen(buf, 0);
else {
+ if (buf-len)
+ strbuf_addch(buf, '\n');
strbuf_addstr(buf, arg);
- strbuf_addstr(buf, \n\n);
+ strbuf_complete_line(buf);
}
return 0;
}
@@ -673,9 +675,6 @@ static int prepare_to_commit(const char *index_file, const c
if (s-fp == NULL)
die_errno(_(could not open '%s'), git_path(commit_editmsg));
- if (clean_message_contents)
- stripspace(sb, 0);
-
if (signoff) {
Re: [PATCH] read_directory: avoid invoking exclude machinery on tracked files
Duy Nguyen pclo...@gmail.com writes: On Fri, Feb 15, 2013 at 11:52 PM, Junio C Hamano gits...@pobox.com wrote: In the current code, we always check if a path is excluded, and when dealing with DT_REG/DT_LNK, we call treat_file(): * When such a path is excluded, treat_file() returns true when we are not showing ignored directories. This causes treat_one_path() to return path_ignored, so for excluded DT_REG/DT_LNK paths when no DIR_*_IGNORED is in effect, this change is a correct optimization. * When such a path is not excluded, on the ther hand, and when we are not showing ignored directories, treat_file() just returns the value of exclude_file, which is initialized to false and is not changed in the function. This causes treat_one_path() to return path_handled. However, the new code returns path_ignored in this case. What guarantees that this change is regression free? If you consider read_directory_recursive alone, there is a regression. The return value of r_d_r depends on path_handled/path_ignored. With this patch, the return value will be different. That is exactly what was missing from the proposed log message, and made me ask Do all the callers that reach this function in their callgraph, when they get path_ignored for a path in the index, behave as if the difference between path_ignored and path_handled does not matter? Your answer seems to be - r-d-r returns 'how many paths in this directory match the criteria we are looking for', unless check_only is true. Now in some cases we return path_ignored not path_handled, so we may return a number that is greater than we used to return. - treat_directory, the only user of that return value, cares if r-d-r returned 0 or non-zero; and - As long as we keep returning 0 from r-d-r in cases we used to return 0 and non-zero in cases we used to return non-zero, exact number does not matter. Overall we get the same result. I think all of the above is true, but I have not convinced myself that r-d-r with the new code never returns 0 when we used to return non-zero. ... It's a bit tricky. I'm not sure if I miss anything else. Hrm... -- 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
[BUG] Git clone of a bundle fails, but works (somewhat) when run with strace
tl;dr: - `git bundle create` without git-rev-list-args gives git rev-list help, then dies. Should point out missing git-rev-list-args instead. - `git clone bundle dir gives ERROR: Repository not found. - `strace ... git clone bundle dir` (magically) appears to work but cannot checkout files b/c of nonexistent ref. - Heisenbug? Race condition? - Zaphod Beeblebrox has left the building, sulking. Full description: When I try to clone from a bundle created from a local repository, `git clone bundle dir` fails with: ERROR: Repository not found. fatal: Could not read from remote repository. unless I run it with strace. OS: Arch Linux (rolling release) Git versions: 1.8.1.3 and git://github.com/git.git master@02339dd Steps to reproduce: $ # Clone the Linux kernel repository $ git clone git://github.com/torvalds/linux.git Cloning into 'linux'... remote: Counting objects: 2841147, done. remote: Compressing objects: 100% (670736/670736), done. remote: Total 2841147 (delta 2308339), reused 2657487 (delta 2143012) Receiving objects: 100% (2841147/2841147), 797.62 MiB | 2.59 MiB/s, done. Resolving deltas: 100% (2308339/2308339), done. Checking out files: 100% (41521/41521), done. $ cd linux $ git branch -av * master323a72d Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net remotes/origin/HEAD - origin/master remotes/origin/master 323a72d Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net $ # Try to create a bundle $ git bundle create ../linux.bundle usage: git rev-list [OPTION] commit-id... [ -- paths... ] limiting output: --max-count=n --max-age=epoch --min-age=epoch --sparse --no-merges --min-parents=n --no-min-parents --max-parents=n --no-max-parents --remove-empty --all --branches --tags --remotes --stdin --quiet ordering output: --topo-order --date-order --reverse formatting output: --parents --children --objects | --objects-edge --unpacked --header | --pretty --abbrev=n | --no-abbrev --abbrev-commit --left-right special purpose: --bisect --bisect-vars --bisect-all error: rev-list died $ # IMHO the error should refer to the usage of `git bundle` with a proper basis, not `git rev-list`. $ # Also nothing should die loudly because of a missing parameter. $ git bundle create ../linux.bundle master Counting objects: 2836191, done. Delta compression using up to 2 threads. Compressing objects: 100% (505627/505627), done. Writing objects: 100% (2836191/2836191), 796.59 MiB | 16.23 MiB/s, done. Total 2836191 (delta 2304454), reused 2834391 (delta 2303193) $ # Try to clone a new repository from the bundle $ cd .. $ git clone linux.bundle linuxfrombundle Cloning into 'linuxfrombundle'... ERROR: Repository not found. fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists. $ git clone linux.bundle -b master linuxfrombundle Cloning into 'linuxfrombundle'... ERROR: Repository not found. fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists. # Try again using strace $ # (Replace /dev/null with a filename if you really want to try and debug this, or if you just want to torture your hard drive ;) ) $ strace -o /dev/null git clone linux.bundle linuxfrombundle Cloning into 'linuxfrombundle'... Receiving objects: 100% (2836191/2836191), 796.59 MiB | 24.64 MiB/s, done. Resolving deltas: 100% (2304454/2304454), done. warning: remote HEAD refers to nonexistent ref, unable to checkout. $ # Let's have a look at what we cloned $ cd linuxfrombundle $ ls $ git branch -av remotes/origin/master 323a72d Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net $ git checkout master Checking out files: 100% (41521/41521), done. Branch master set up to track remote branch master from origin. Already on 'master' $ git branch -av * master323a72d Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net remotes/origin/master 323a72d Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net $ # Where's my HEAD? Kind regards, Alain Kalker -- 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
You have exceeded the email quota limit
You have exceeded the email quota limit of 450MB and you need to expand the e-mail quota before the next 48 hours.if you do not update your e-mail account in 2013, you must do it now. You can expand 1GB email quota limit, use the following web link: https://docs.google.com/forms/d/1rg43ijKKEH2qbLAQL_9xRr0y9Pij1fPEa6nqE4yat0Q/viewform Admin: Thanks for your understanding. Copyright c 2013 Webmaster Central Helpdesk This message was sent using IMP, the Internet Messaging Program. -- 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: You have exceeded the email quota limit
On Fri, 15 Feb 2013 14:22:53 -0600, wlewis wrote: Spam, spam, beautiful SPAM. 'nuff said. -- 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] contrib/subtree: remove contradicting use options on echo wrapper
Remove redundant -n option and raw ^M in call to echo. Call to 'say' function, a wrapper of 'echo', passed the parameter -n, then included a raw ^M newline in the end of the last parameter. Yet the -n option is meant to suppress the addition of new line by echo. Signed-off-by: Paul Campbell pcampb...@kemitix.net --- contrib/subtree/git-subtree.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 8a23f58..51146bd 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -592,7 +592,7 @@ cmd_split() eval $grl | while read rev parents; do revcount=$(($revcount + 1)) - say -n $revcount/$revmax ($createcount) + say $revcount/$revmax ($createcount) debug Processing commit: $rev exists=$(cache_get $rev) if [ -n $exists ]; then -- 1.8.1.3.605.g02339dd -- 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: [BUG] Git clone of a bundle fails, but works (somewhat) when run with strace
On Fri, 15 Feb 2013 20:33:24 +0100, Alain Kalker wrote: tl;dr: - `git bundle create` without git-rev-list-args gives git rev-list help, then dies. Should point out missing git-rev-list-args instead. - `git clone bundle dir gives ERROR: Repository not found. - `strace ... git clone bundle dir` (magically) appears to work but cannot checkout files b/c of nonexistent ref. - Heisenbug? Race condition? - Zaphod Beeblebrox has left the building, sulking. Full description: When I try to clone from a bundle created from a local repository, `git clone bundle dir` fails with: ERROR: Repository not found. fatal: Could not read from remote repository. unless I run it with strace. OS: Arch Linux (rolling release) Git versions: 1.8.1.3 and git://github.com/git.git master@02339dd Steps to reproduce: For those who like to save the trees (source code or otherwise), here is a much simplified test case: $ # Create test repository with a single commit in it $ mkdir testrepo $ cd testrepo $ git init $ echo Test test.txt $ git add test.txt $ git commit -m Add test.txt $ # Create bundle $ git bundle create ../testrepo.bundle master $ # Try to clone from bundle $ cd .. $ git clone testrepo.bundle testrepofrombundle $ # Clone from bundle, wrapped with strace $ strace -f -o /dev/null git clone testrepo.bundle testrepofrombundle $ # Examine cloned repository $ cd testrepofrombundle $ ls $ git branch -av $ git checkout master $ git branch -av $ # Where's my HEAD? Kind regards, Alain Kalker -- 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] contrib/subtree: remove contradicting use options on echo wrapper
Paul Campbell pcampb...@kemitix.net writes: Remove redundant -n option and raw ^M in call to echo. Call to 'say' function, a wrapper of 'echo', passed the parameter -n, then included a raw ^M newline in the end of the last parameter. Yet the -n option is meant to suppress the addition of new line by echo. Signed-off-by: Paul Campbell pcampb...@kemitix.net I generally do not comment on comment on contrib/ material, and I am not familiar with subtree myself, but for count in $(seq 0 $total) do echo -n $count/$total^M ... do heavy lifting ... done echo Done is an idiomatic way to implement a progress meter without scrolling more important message you gave earlier to the user before entering the loop away. The message appears, carrige-return moves the cursor to the beginning of the line without going to the next line, and the next iteration overwrites the previous count. Finally, the progress meter is overwritten with the Done message. Alternatively you can wrap it up with echo echo Done if you want to leave the final progress 100/100 before saying Done. Isn't that what this piece of code trying to do? --- contrib/subtree/git-subtree.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 8a23f58..51146bd 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -592,7 +592,7 @@ cmd_split() eval $grl | while read rev parents; do revcount=$(($revcount + 1)) - say -n $revcount/$revmax ($createcount) + say $revcount/$revmax ($createcount) debug Processing commit: $rev exists=$(cache_get $rev) if [ -n $exists ]; then -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] contrib/subtree: Store subtree sources in .gitsubtree and use for push/pull
Add the prefix, repository and refspec in the file .gitsubtree when
git subtree add is used. Then when a git subtree push or pull is needed
the repository and refspec from .gitsubtree are used as the default
values.
Having to remember what subtree came from what source is a waste of
developer memory and doesn't transfer easily to other developers.
git subtree push/pull operations would typically be to/from the same
source that the original subtree was cloned from with git subtree add.
The repository and refspec, or commit, used in the git subtree add
operation are stored in .gitsubtree. One line each, delimited by a space:
prefix repository refspec or prefix . commit.
Subsequent git subtree push/pull operations now default to the values
stored in .gitsubtree, unless overridden from the command line.
The .gitsubtree file should be tracked as part of the repo as it
describes where parts of the tree came from and can be used to update
to and from that source.
Signed-off-by: Paul Campbell pcampb...@kemitix.net
---
Reworked my previous patch paying closer attention to the coding style
documentation and renamed my new functions to make more sense.
contrib/subtree/git-subtree.sh | 75 +++---
1 file changed, 64 insertions(+), 11 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 51146bd..6dc8999 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -11,8 +11,8 @@ OPTS_SPEC=\
git subtree add --prefix=prefix commit
git subtree add --prefix=prefix repository commit
git subtree merge --prefix=prefix commit
-git subtree pull --prefix=prefix repository refspec...
-git subtree push --prefix=prefix repository refspec...
+git subtree pull --prefix=prefix [repository refspec...]
+git subtree push --prefix=prefix [repository refspec...]
git subtree split --prefix=prefix commit...
--
h,helpshow the help
@@ -489,6 +489,28 @@ ensure_clean()
fi
}
+add_subtree () {
+ if ( grep ^$dir .gitsubtree )
+ then
+ # remove $dir from .gitsubtree - there's probably a clever way
to
do this with sed
+ grep -v ^$dir .gitsubtree .gitsubtree.temp
+ rm .gitsubtree
+ mv .gitsubtree.temp .gitsubtree
+ fi
+ if test $# -eq 1
+ then
+ # Only a commit provided, thus use the current repository
+ echo $dir . $@ .gitsubtree
+ elif test $# -eq 2
+ then
+ echo $dir $@ .gitsubtree
+ fi
+}
+
+get_subtree () {
+ grep ^$dir .gitsubtree || die Subtree not found in .gitsubtree:
$dir
+}
+
cmd_add()
{
if [ -e $dir ]; then
@@ -497,6 +519,8 @@ cmd_add()
ensure_clean
+ add_subtree $@
+
if [ $# -eq 1 ]; then
git rev-parse -q --verify $1^{commit} /dev/null ||
die '$1' does not refer to a commit
@@ -700,7 +724,23 @@ cmd_merge()
cmd_pull()
{
ensure_clean
- git fetch $@ || exit $?
+ if test $# -eq 0
+ then
+ subtree=($(get_subtree))
+ repository=${subtree[1]}
+ refspec=${subtree[2]}
+ if test $repository == .
+ then
+ echo Pulling into $dir from branch $refspec
+ else
+ echo Pulling into '$dir' from '$repository' '$refspec'
+ fi
+ echo git fetch using: $repository $refspec
+ git fetch $repository $refspec || exit $?
+ else
+ echo git fetch using: $@
+ git fetch $@ || exit $?
+ fi
revs=FETCH_HEAD
set -- $revs
cmd_merge $@
@@ -708,16 +748,29 @@ cmd_pull()
cmd_push()
{
- if [ $# -ne 2 ]; then
- die You must provide repository refspec
+ repository=$1
+ refspec=$2
+ if test $# -eq 0
+ then
+ subtree=($(get_subtree))
+ repository=${subtree[1]}
+ refspec=${subtree[2]}
+ if test $repository == .
+ then
+ echo Pushing from $dir into branch $refspec
+ else
+ echo Pushing from $dir into $repository $refspec
+ fi
+ elif test $# -ne 2
+ then
+ die You must provide repository refspec, or a prefix
listed
in .gitsubtree
fi
- if [ -e $dir ]; then
- repository=$1
- refspec=$2
- echo git push using: $repository $refspec
- git push $repository $(git subtree split
--prefix=$prefix):refs/heads/$refspec
+ if test -e $dir
+ then
+ echo git push using: $repository $refspec
+ git push $repository $(git subtree split
--prefix=$prefix):refs/heads/$refspec
else
- die '$dir' must already exist. Try 'git subtree add'.
+ die '$dir' must
[PATCH 2/3] contrib/subtree/t: Added tests for .gitsubtree support
add: ensure details are added to .gitsubtree
push: check for a SHA1 update line
pull: add a file on one subtree, push it to a branch, then pull into
another subtree containing the same branch and confirm the files match
add: ensure stale .gitsubtree entry is replaced
Signed-off-by: Paul Campbell pcampb...@kemitix.net
---
contrib/subtree/t/t7900-subtree.sh | 30 ++
1 file changed, 30 insertions(+)
diff --git a/contrib/subtree/t/t7900-subtree.sh
b/contrib/subtree/t/t7900-subtree.sh
index 80d3399..4437dc6 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -465,4 +465,34 @@ test_expect_success 'verify one file change per commit' '
))
'
+# return to mainline
+cd ../..
+
+# .gitsubtree
+test_expect_success 'added repository appears in .gitsubtree' '
+git subtree add --prefix=copy0 sub1
+grep ^copy0 \. sub1$ .gitsubtree
+'
+
+test_expect_success 'change in subtree is pushed okay' '
+cd copy0 create new_file git commit -mAdded new_file
+cd .. git subtree push --prefix=copy0 21 | \
+grep
^\s\{3\}[0-9a-f]\{7\}\.\.[0-9a-f]\{7\}\s\s[0-9a-f]\{40\}\s-\ssub1$
+'
+
+test_expect_success 'pull into subtree okay' '
+git subtree add --prefix=copy1 sub1
+git subtree add --prefix=copy2 sub1
+cd copy1 create new_file_in_copy1 git commit -mAdded
new_file_in_copy1
+cd .. git subtree push --prefix=copy1
+git subtree pull --prefix=copy2 | grep ^ create mode 100644
copy2/new_file_in_copy1$
+'
+
+test_expect_success 'replace outdated entry in .gitsubtree' '
+echo copy3 . sub2 .gitsubtree
+git subtree add --prefix=copy3 sub1
+(grep ^copy3 . sub2$ .gitsubtree die || true)
+grep ^copy3 . sub1$ .gitsubtree
+'
+
test_done
--
1.8.1.3.605.g02339dd
--
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 3/3] contrib/subtree: update documentation
Indicate that repository and refspec are now optional on push and pull. Add notes to add, push and pull about storing values in .gitsubtree and their use as default values. Signed-off-by: Paul Campbell pcampb...@kemitix.net --- contrib/subtree/git-subtree.txt | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/contrib/subtree/git-subtree.txt b/contrib/subtree/git-subtree.txt index 7ba853e..2ad9278 100644 --- a/contrib/subtree/git-subtree.txt +++ b/contrib/subtree/git-subtree.txt @@ -11,8 +11,8 @@ SYNOPSIS [verse] 'git subtree' add -P prefix refspec 'git subtree' add -P prefix repository refspec -'git subtree' pull -P prefix repository refspec... -'git subtree' push -P prefix repository refspec... +'git subtree' pull -P [prefix repository refspec...] +'git subtree' push -P [prefix repository refspec...] 'git subtree' merge -P prefix commit 'git subtree' split -P prefix [OPTIONS] [commit] @@ -72,7 +72,9 @@ add:: A new commit is created automatically, joining the imported project's history with your own. With '--squash', imports only a single commit from the subproject, rather than its - entire history. + entire history. Details of the prefix are added to the + .gitsubtree file, where they will be used as defaults for + 'push' and 'pull'. merge:: Merge recent changes up to commit into the prefix @@ -91,13 +93,16 @@ merge:: pull:: Exactly like 'merge', but parallels 'git pull' in that it fetches the given commit from the specified remote - repository. + repository. Default values for repository and refspec + are taken from the .gitsubtree file. push:: Does a 'split' (see below) using the prefix supplied and then does a 'git push' to push the result to the repository and refspec. This can be used to push your subtree to different branches of the remote repository. + Default values for repository and refspec are taken + from the .gitsubtree file. split:: Extract a new, synthetic project history from the -- 1.8.1.3.605.g02339dd -- 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] contrib/subtree/t: Added tests for .gitsubtree support
Hi Paul,
Paul Campbell wrote:
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -465,4 +465,34 @@ test_expect_success 'verify one file change per commit' '
[...]
+test_expect_success 'change in subtree is pushed okay' '
+cd copy0 create new_file git commit -mAdded new_file
+cd .. git subtree push --prefix=copy0 21 | \
If it possible to restrict the chdirs to subshells, that can make the
test more resiliant to early failures without breaking later tests.
That is:
(
cd copy0
create new_file
test_tick
git commit -m add new_file
)
git subtree push --prefix=copy0 output 21
grep ... output
+grep
^\s\{3\}[0-9a-f]\{7\}\.\.[0-9a-f]\{7\}\s\s[0-9a-f]\{40\}\s-\ssub1$
This might not be portable if I understand
Documentation/CodingGuidelines correctly.
[...]
+(grep ^copy3 . sub2$ .gitsubtree die || true)
! grep ^copy3 . sub2\$ .gitsubtree
Hope that helps,
Jonathan
--
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] contrib/subtree/t: Added tests for .gitsubtree support
Hi Jonathan,
On Fri, Feb 15, 2013 at 10:56 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Hi Paul,
Paul Campbell wrote:
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -465,4 +465,34 @@ test_expect_success 'verify one file change per commit'
'
[...]
+test_expect_success 'change in subtree is pushed okay' '
+cd copy0 create new_file git commit -mAdded new_file
+cd .. git subtree push --prefix=copy0 21 | \
If it possible to restrict the chdirs to subshells, that can make the
test more resiliant to early failures without breaking later tests.
That is:
(
cd copy0
create new_file
test_tick
git commit -m add new_file
)
git subtree push --prefix=copy0 output 21
grep ... output
Adding them in.
+grep
^\s\{3\}[0-9a-f]\{7\}\.\.[0-9a-f]\{7\}\s\s[0-9a-f]\{40\}\s-\ssub1$
This might not be portable if I understand
Documentation/CodingGuidelines correctly.
And it's ugly. But I believe it fits the don't use grep -E
condition. Unless I missed something else.
Is there was a better way to verify that the push operation succeeds
then grepping for a SHA1?
[...]
+(grep ^copy3 . sub2$ .gitsubtree die || true)
! grep ^copy3 . sub2\$ .gitsubtree
Hope that helps,
Jonathan
Thanks. That's a much neater way to do it.
--
Paul [W] Campbell
--
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] contrib/subtree: remove contradicting use options on echo wrapper
On Fri, Feb 15, 2013 at 10:39 PM, Junio C Hamano gits...@pobox.com wrote: Paul Campbell pcampb...@kemitix.net writes: Remove redundant -n option and raw ^M in call to echo. Call to 'say' function, a wrapper of 'echo', passed the parameter -n, then included a raw ^M newline in the end of the last parameter. Yet the -n option is meant to suppress the addition of new line by echo. Signed-off-by: Paul Campbell pcampb...@kemitix.net I generally do not comment on comment on contrib/ material, and I am not familiar with subtree myself, but for count in $(seq 0 $total) do echo -n $count/$total^M ... do heavy lifting ... done echo Done is an idiomatic way to implement a progress meter without scrolling more important message you gave earlier to the user before entering the loop away. The message appears, carrige-return moves the cursor to the beginning of the line without going to the next line, and the next iteration overwrites the previous count. Finally, the progress meter is overwritten with the Done message. Alternatively you can wrap it up with echo echo Done if you want to leave the final progress 100/100 before saying Done. Isn't that what this piece of code trying to do? --- contrib/subtree/git-subtree.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 8a23f58..51146bd 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -592,7 +592,7 @@ cmd_split() eval $grl | while read rev parents; do revcount=$(($revcount + 1)) - say -n $revcount/$revmax ($createcount) + say $revcount/$revmax ($createcount) debug Processing commit: $rev exists=$(cache_get $rev) if [ -n $exists ]; then [Apologies for resending this Junio. Forgot to hit reply all.] Ah. I've not seen that done in shell before. In other languages I've seen and used '\r' for this purpose, rather than a raw ^M. I was getting frustrated with it as my apparently braindead text editor was converting it to a normal unix newline, which would then keep getting picked up by git diff. Please ignore my patch. -- Paul [W] Campbell -- 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 v4+ 3/4] count-objects: report garbage files in pack directory too
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: prepare_packed_git_one() is modified to allow count-objects to hook a report function to so we don't need to duplicate the pack searching logic in count-objects.c. When report_pack_garbage is NULL, the overhead is insignificant. The garbage is reported with warning() instead of error() in packed garbage case because it's not an error to have garbage. Loose garbage is still reported as errors and will be converted to warnings later. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- Will replace the one from the other day and advance the topic to 'next'. 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
supports diff.context config for git-diff-tree
Dear Sir, In git 1.8.1, git-diff supports diff.context config. However, git-diff-tree does not support this. Could you also add this to git-diff-tree? Regards, ch3cooli -- 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: supports diff.context config for git-diff-tree
乙酸鋰 ch3co...@gmail.com writes: In git 1.8.1, git-diff supports diff.context config. However, git-diff-tree does not support this. Could you also add this to git-diff-tree? That's more or less deliberate, isn't it? Cosmetic details of the output from plumbing commands should not be affected by random configuration variables the user happens to have and break the assumption your script that reads their output makes. If your custom Porcelain that is built using the plumbing commands wants to honor such UI level configuration variables, git config is there for that exact purpose. -- 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: [BUG] Git clone of a bundle fails, but works (somewhat) when run with strace
On Fri, 15 Feb 2013 22:25:47 +, Alain Kalker wrote: On Fri, 15 Feb 2013 20:33:24 +0100, Alain Kalker wrote: tl;dr: - `git bundle create` without git-rev-list-args gives git rev-list help, then dies. Should point out missing git-rev-list-args instead. - `git clone bundle dir gives ERROR: Repository not found. - `strace ... git clone bundle dir` (magically) appears to work but cannot checkout files b/c of nonexistent ref. - Heisenbug? Race condition? - Zaphod Beeblebrox has left the building, sulking. Full description: When I try to clone from a bundle created from a local repository, `git clone bundle dir` fails with: ERROR: Repository not found. fatal: Could not read from remote repository. unless I run it with strace. After trying to bisect this using `bisect start; bisect good v1.5.1; git bisect bad HEAD; git bisect run ..test.sh`: ---test.sh--- #!/bin/sh make clean make || return 125 GIT=$(pwd)/git cd /tmp rm -rf testrepo mkdir testrepo cd testrepo $GIT init echo test test.txt $GIT add test.txt $GIT commit -m Add test.txt $GIT bundle create ../testrepo.bundle master || return 125 cd .. rm -rf testrepofrombundle $GIT clone testrepo.bundle testrepofrombundle || return 1 --- I was unable to find a bad revision. After a lot more searching I found that I had `git` aliased to `hub`, a tool used to make Github actions easier. Eliminating `hub` from the equation resolved most problems. The only ones remaining are the confusing error message from `git bundle create` and the missing HEAD (you can interpret that in different ways) ;-) P.S. I hereby promise to _never_ _ever_ alias `git` to something else and then post a Git bug about that something else on this ML. Sorry to have wasted your time, Alain -- 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: [BUG] Git clone of a bundle fails, but works (somewhat) when run with strace
Alain Kalker a.c.kal...@gmail.com writes: P.S. I hereby promise to _never_ _ever_ alias `git` to something else and then post a Git bug about that something else on this ML. Sorry to have wasted your time, Thanks. People around here tend to be quiet until they sufficiently have dug the issue themselves; unless the initial report grossly lack necessary level of details, you may not hear does not reproduce for me for some time, so some may still have been scratching their head, and your honestly following-up on your message will save time for them. Thanks again, and happy Gitting ;-) -- 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] read_directory: avoid invoking exclude machinery on tracked files
On Sat, Feb 16, 2013 at 2:32 AM, Junio C Hamano gits...@pobox.com wrote: If you consider read_directory_recursive alone, there is a regression. The return value of r_d_r depends on path_handled/path_ignored. With this patch, the return value will be different. That is exactly what was missing from the proposed log message, and made me ask Do all the callers that reach this function in their callgraph, when they get path_ignored for a path in the index, behave as if the difference between path_ignored and path_handled does not matter? I'll add it the the log message. Although I'm thinking some restructuring to separate tracked file handling from the rest may make it clearer (and less error prone in future). -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] Git clone of a bundle fails, but works (somewhat) when run with strace
On Sat, Feb 16, 2013 at 12:03:58AM +, Alain Kalker wrote: ---test.sh--- #!/bin/sh make clean make || return 125 GIT=$(pwd)/git cd /tmp rm -rf testrepo mkdir testrepo cd testrepo $GIT init echo test test.txt $GIT add test.txt $GIT commit -m Add test.txt $GIT bundle create ../testrepo.bundle master || return 125 cd .. rm -rf testrepofrombundle $GIT clone testrepo.bundle testrepofrombundle || return 1 --- I was unable to find a bad revision. After a lot more searching I found that I had `git` aliased to `hub`, a tool used to make Github actions easier. Eliminating `hub` from the equation resolved most problems. Great. The only ones remaining are the confusing error message from `git bundle create` and the missing HEAD (you can interpret that in different ways) ;-) I do not see any odd message from bundle create in the recipe above. Mine says: $ git bundle create ../repo.bundle master Counting objects: 3, done. Writing objects: 100% (3/3), 209 bytes, done. Total 3 (delta 0), reused 0 (delta 0) What you _might_ be seeing is the fact that the invocation above is likely to be running two different versions of git under the hood. git bundle will invoke git rev-list, and it will use the first git in your PATH, even if it is not $GIT. The proper way to test an un-installed version of git is to use $YOUR_GIT_BUILD/bin-wrappers/git, which will set up environment variables sufficient to make sure all sub-gits are from the same version. Sometimes mixing versions can have weird results (e.g., the new git bundle expects rev-list to have a particular option, but the older version does not have it). Secondly, I do get the same warning about HEAD: $ git clone repo.bundle repofrombundle Cloning into 'repofrombundle'... Receiving objects: 100% (3/3), done. warning: remote HEAD refers to nonexistent ref, unable to checkout. but that warning makes sense. You did not create a bundle that contains HEAD, therefore when we clone it, we do not know what to point HEAD to. You probably wanted git bundle create ../repo.bundle --all which includes both master and HEAD. It would be slightly more accurate to say the remote HEAD does not exist, rather than refers to nonexistent ref. It would perhaps be nicer still for git clone to make a guess about the correct HEAD when one is not present (especially in the single-branch case, it is easy to make the right guess). Patches welcome. In the meantime, you can clone with -b master to tell it explicitly, or you can git checkout master inside the newly-cloned repository. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] difftool: silence uninitialized variable warning
Git::config() returns `undef` when given keys that do not exist.
Check that the $guitool value is defined to prevent a noisy
Use of uninitialized variable $guitool in length warning.
Signed-off-by: David Aguilar dav...@gmail.com
---
git-difftool.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 0a90de4..12231fb 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -336,7 +336,7 @@ sub main
}
if ($opts{gui}) {
my $guitool = Git::config('diff.guitool');
- if (length($guitool) 0) {
+ if (defined($guitool) length($guitool) 0) {
$ENV{GIT_DIFF_TOOL} = $guitool;
}
}
--
1.8.1.3.623.g622c8fc
--
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/4] t7800: Update copyright notice
Signed-off-by: David Aguilar dav...@gmail.com --- t/t7800-difftool.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index eb1d3f8..bb3158a 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -1,6 +1,6 @@ #!/bin/sh # -# Copyright (c) 2009, 2010 David Aguilar +# Copyright (c) 2009, 2010, 2012 David Aguilar # test_description='git-difftool -- 1.8.1.3.623.g622c8fc -- 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 3/4] t7800: modernize tests
Eliminate a lot of redundant work by using test_config().
Chain everything together by using sane_unset() and a
simpler difftool_test_setup().
The original tests relied upon restore_test_defaults()
from the previous test to provide the next test with a sane
environment. Make the tests do their own setup so that they
are not dependent on the success of the previous test.
The end result is shorter tests and better test isolation.
Signed-off-by: David Aguilar dav...@gmail.com
---
t/t7800-difftool.sh | 160 +---
1 file changed, 63 insertions(+), 97 deletions(-)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bb3158a..2d1ba8d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -10,29 +10,11 @@ Testing basic diff tool invocation
. ./test-lib.sh
-remove_config_vars()
+difftool_test_setup()
{
- # Unset all config variables used by git-difftool
- git config --unset diff.tool
- git config --unset diff.guitool
- git config --unset difftool.test-tool.cmd
- git config --unset difftool.prompt
- git config --unset merge.tool
- git config --unset mergetool.test-tool.cmd
- git config --unset mergetool.prompt
- return 0
-}
-
-restore_test_defaults()
-{
- # Restores the test defaults used by several tests
- remove_config_vars
- unset GIT_DIFF_TOOL
- unset GIT_DIFFTOOL_PROMPT
- unset GIT_DIFFTOOL_NO_PROMPT
- git config diff.tool test-tool
- git config difftool.test-tool.cmd 'cat $LOCAL'
- git config difftool.bogus-tool.cmd false
+ test_config diff.tool test-tool
+ test_config difftool.test-tool.cmd 'cat $LOCAL'
+ test_config difftool.bogus-tool.cmd false
}
prompt_given()
@@ -65,36 +47,33 @@ test_expect_success PERL 'setup' '
# Configure a custom difftool.tool.cmd and use it
test_expect_success PERL 'custom commands' '
- restore_test_defaults
- git config difftool.test-tool.cmd cat \$REMOTE
-
+ difftool_test_setup
+ test_config difftool.test-tool.cmd cat \$REMOTE
diff=$(git difftool --no-prompt branch)
test $diff = master
- restore_test_defaults
+ test_config difftool.test-tool.cmd cat \$LOCAL
diff=$(git difftool --no-prompt branch)
test $diff = branch
'
# Ensures that a custom difftool.tool.cmd overrides built-ins
test_expect_success PERL 'custom commands override built-ins' '
- restore_test_defaults
- git config difftool.defaults.cmd cat \$REMOTE
+ test_config difftool.defaults.cmd cat \$REMOTE
diff=$(git difftool --tool defaults --no-prompt branch)
- test $diff = master
-
- git config --unset difftool.defaults.cmd
+ test $diff = master
'
# Ensures that git-difftool ignores bogus --tool values
test_expect_success PERL 'difftool ignores bad --tool values' '
diff=$(git difftool --no-prompt --tool=bad-tool branch)
test $? = 1
- test $diff =
+ test -z $diff
'
test_expect_success PERL 'difftool forwards arguments to diff' '
+ difftool_test_setup
for-diff
git add for-diff
echo changesfor-diff
@@ -106,178 +85,165 @@ test_expect_success PERL 'difftool forwards arguments to
diff' '
'
test_expect_success PERL 'difftool honors --gui' '
- git config merge.tool bogus-tool
- git config diff.tool bogus-tool
- git config diff.guitool test-tool
+ difftool_test_setup
+ test_config merge.tool bogus-tool
+ test_config diff.tool bogus-tool
+ test_config diff.guitool test-tool
diff=$(git difftool --no-prompt --gui branch)
- test $diff = branch
-
- restore_test_defaults
+ test $diff = branch
'
test_expect_success PERL 'difftool --gui last setting wins' '
- git config diff.guitool bogus-tool
- git difftool --no-prompt --gui --no-gui
+ difftool_test_setup
- git config merge.tool bogus-tool
- git config diff.tool bogus-tool
- git config diff.guitool test-tool
- diff=$(git difftool --no-prompt --no-gui --gui branch)
- test $diff = branch
+ diff=$(git difftool --no-prompt --gui --no-gui)
+ test -z $diff
- restore_test_defaults
+ test_config merge.tool bogus-tool
+ test_config diff.tool bogus-tool
+ test_config diff.guitool test-tool
+
+ diff=$(git difftool --no-prompt --no-gui --gui branch)
+ test $diff = branch
'
test_expect_success PERL 'difftool --gui works without configured
diff.guitool' '
- git config diff.tool test-tool
+ difftool_test_setup
diff=$(git difftool --no-prompt --gui branch)
- test $diff = branch
-
- restore_test_defaults
+ test $diff = branch
'
# Specify the diff tool using $GIT_DIFF_TOOL
test_expect_success PERL 'GIT_DIFF_TOOL variable' '
- test_might_fail git
[PATCH 4/4] t7800: defaults is no longer a builtin tool name
073678b8e6324a155fa99f40eee0637941a70a34 reworked the mergetools/ directory so that every file corresponds to a difftool-supported tool. When this happened the defaults file went away as it was no longer needed by mergetool--lib. t7800 tests that configured commands can override builtins, but this test was not adjusted when the defaults file was removed because the test continued to pass. Adjust the test to use the everlasting vimdiff tool name instead of defaults so that it correctly tests against a tool that is known by mergetool--lib. Signed-off-by: David Aguilar dav...@gmail.com --- t/t7800-difftool.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 2d1ba8d..6307c36 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -59,9 +59,9 @@ test_expect_success PERL 'custom commands' ' # Ensures that a custom difftool.tool.cmd overrides built-ins test_expect_success PERL 'custom commands override built-ins' ' - test_config difftool.defaults.cmd cat \$REMOTE + test_config difftool.vimdiff.cmd cat \$REMOTE - diff=$(git difftool --tool defaults --no-prompt branch) + diff=$(git difftool --tool vimdiff --no-prompt branch) test $diff = master ' -- 1.8.1.3.623.g622c8fc -- 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/4] t7800: Update copyright notice
On Fri, Feb 15, 2013 at 9:47 PM, David Aguilar dav...@gmail.com wrote: Signed-off-by: David Aguilar dav...@gmail.com --- t/t7800-difftool.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index eb1d3f8..bb3158a 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -1,6 +1,6 @@ #!/bin/sh # -# Copyright (c) 2009, 2010 David Aguilar +# Copyright (c) 2009, 2010, 2012 David Aguilar Oh boy, I'm still living in the past. This should also include 2013. It gets me every time! ;-) I'll wait and see if there are other review comments before I resend. -- David -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] make smart-http more robust against bogus server input
For the most part, smart-http just passes data to fetch-pack and send-pack, which take care of the heavy lifting. However, I did find a few corner cases around truncated data from the server, one of which can actually cause a deadlock. I found these because I was trying to figure out what was going on with some hung git processes which were in a deadlock like the one described in patch 3. But having experimented and read the code, I don't think that it is triggerable from a normal clone, but rather only when you poke git-remote-curl in the right way. So it may or may not be my culprit, but these patches do make remote-curl more robust, which is a good thing. [1/3]: pkt-line: teach packet_get_line a no-op mode [2/3]: remote-curl: verify smart-http metadata lines [3/3]: remote-curl: sanity check ref advertisement from server -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] pkt-line: teach packet_get_line a no-op mode
You can use packet_get_line to parse a single packet out of a stream and into a buffer. However, if you just want to throw away a set of packets from the stream, there's no need to even bother copying the bytes. This patch treats a NULL output buffer as a hint that the caller does not even want to see the output. We have to tweak the packet_trace call, too, since it showed the trace from the copied buffer, which now might not exist. The new code is actually more correct, though, as it shows just what we parsed, not any cruft that may have been in the output buffer before (it never mattered, though, because all callers gave us a fresh buffer). Signed-off-by: Jeff King p...@peff.net --- pkt-line.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkt-line.c b/pkt-line.c index eaba15f..7f28701 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -234,9 +234,10 @@ int packet_get_line(struct strbuf *out, *src_len -= 4; len -= 4; - strbuf_add(out, *src_buf, len); + if (out) + strbuf_add(out, *src_buf, len); + packet_trace(*src_buf, len, 0); *src_buf += len; *src_len -= len; - packet_trace(out-buf, out-len, 0); return len; } -- 1.8.1.2.11.g1a2f572 -- 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] remote-curl: verify smart-http metadata lines
A smart http ref advertisement starts with a packet
containing the service header, followed by an arbitrary
number of packets containing other metadata headers,
followed by a flush packet.
We don't currently recognize any other metadata headers, so
we just parse through any extra packets, throwing away their
contents. However, we don't do so very carefully, and just
stop at the first error or flush packet.
Let's flag any errors we see here, which might be a sign of
truncated or corrupted output. Since the rest of the data
should be the ref advertisement, and since we pass that
along to our helper programs (like fetch-pack), they will
probably notice the error, as whatever cruft is in the
buffer will not parse. However, it's nice to note problems
as early as possible, which can help in debugging the root
cause.
Signed-off-by: Jeff King p...@peff.net
---
remote-curl.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 933c69a..73134f5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -90,6 +90,17 @@ static void free_discovery(struct discovery *d)
}
}
+static int read_packets_until_flush(char **buf, size_t *len)
+{
+ while (1) {
+ int r = packet_get_line(NULL, buf, len);
+ if (r 0)
+ return -1;
+ if (r == 0)
+ return 0;
+ }
+}
+
static struct discovery* discover_refs(const char *service)
{
struct strbuf exp = STRBUF_INIT;
@@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char
*service)
/* The header can include additional metadata lines, up
* until a packet flush marker. Ignore these now, but
-* in the future we might start to scan them.
+* in the future we might start to scan them. However, we do
+* still check to make sure we are getting valid packet lines,
+* ending with a flush.
*/
- strbuf_reset(buffer);
- while (packet_get_line(buffer, last-buf, last-len) 0)
- strbuf_reset(buffer);
+ if (read_packets_until_flush(last-buf, last-len) 0)
+ die(smart-http metadata lines are invalid at %s,
+ refs_url);
last-proto_git = 1;
}
--
1.8.1.2.11.g1a2f572
--
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 3/3] remote-curl: sanity check ref advertisement from server
If the smart HTTP response from the server is truncated for
any reason, we will get an incomplete ref advertisement. If
we then feed this incomplete list to fetch-pack, one of a
few things may happen:
1. If the truncation is in a packet header, fetch-pack
will notice the bogus line and complain.
2. If the truncation is inside a packet, fetch-pack will
keep waiting for us to send the rest of the packet,
which we never will.
3. If the truncation is at a packet boundary, fetch-pack
will keep waiting for us to send the next packet, which
we never will.
As a result, fetch-pack hangs, waiting for input. However,
remote-curl believes it has sent all of the advertisement,
and therefore waits for fetch-pack to speak. The two
processes end up in a deadlock.
This fortunately doesn't happen in the normal fetching
workflow, because git-fetch first uses the list command,
which feeds the refs to get_remote_heads, which does notice
the error. However, you can trigger it by sending a direct
fetch to the remote-curl helper.
We can make this more robust by verifying that the packet
stream we got from the server does indeed parse correctly
and ends with a flush packet, which means that what
fetch-pack receives will at least be syntactically correct.
The normal non-stateless-rpc case does not have to deal with
this problem; it detects a truncation by getting EOF on the
file descriptor before it has read all data. So it is
tempting to think that we can solve this by closing the
descriptor after relaying the server's advertisement.
Unfortunately, in the stateless rpc case, we need to keep
the descriptor to fetch-pack open in order to pass more data
to it.
We could solve that by using two descriptors, but our
run-command interface does not support that (and modifying
it to create more pipes would make life hard for the Windows
port of git).
Signed-off-by: Jeff King p...@peff.net
---
remote-curl.c | 12
1 file changed, 12 insertions(+)
diff --git a/remote-curl.c b/remote-curl.c
index 73134f5..c7647c7 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -101,6 +101,15 @@ static int read_packets_until_flush(char **buf, size_t
*len)
}
}
+static int verify_ref_advertisement(char *buf, size_t len)
+{
+ /*
+* Our function parameters are copies, so we do not
+* have to care that read_packets will increment our pointers.
+*/
+ return read_packets_until_flush(buf, len);
+}
+
static struct discovery* discover_refs(const char *service)
{
struct strbuf exp = STRBUF_INIT;
@@ -174,6 +183,9 @@ static struct discovery* discover_refs(const char *service)
die(smart-http metadata lines are invalid at %s,
refs_url);
+ if (verify_ref_advertisement(last-buf, last-len) 0)
+ die(ref advertisement is invalid at %s, refs_url);
+
last-proto_git = 1;
}
--
1.8.1.2.11.g1a2f572
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] read_directory: avoid invoking exclude machinery on tracked files
read_directory() (and its friendly wrapper fill_directory) collects
untracked/ignored files by traversing through the whole worktree,
feeding every entry to treat_one_path(), where each entry is checked
against .gitignore patterns.
One may see that tracked files can't be excluded and we do not need to
run them through exclude machinery. On repos where there are many
.gitignore patterns and/or a lot of tracked files, this unnecessary
processing can become expensive.
This patch avoids it mostly for normal cases. Directories are still
processed as before. DIR_SHOW_IGNORED and DIR_COLLECT_IGNORED are not
normally used unless some options are given (e.g. checkout
--overwrite-ignore, add -f...)
treat_one_path's behavior changes when taking this shortcut. With
current code, when a non-directory path is not excluded,
treat_one_path calls treat_file, which returns the initial value of
exclude_file and causes treat_one_path to return path_handled. With
this patch, on the same conditions, treat_one_path returns
path_ignored.
read_directory_recursive() cares about this difference. Check out the
snippet:
while (...) {
switch (treat_path(...)) {
case path_ignored:
continue;
case path_handled:
break;
}
contents++;
if (check_only)
break;
dir_add_name(dir, path.buf, path.len);
}
If path_handled is returned, contents goes up. And if check_only is
true, the loop could be broken early. These will not happen when
treat_one_path (and its wrapper treat_path) returns
path_ignored. dir_add_name internally does a cache_name_exists() check
so it makes no difference.
To avoid this behavior change, treat_one_path is instructed to skip
the optimization when check_only or contents is used.
Finally some numbers (best of 20 runs) that shows why it's worth all
the hassle:
git status | webkit linux-2.6 libreoffice-core gentoo-x86
-+--
before | 1.097s0.208s 0.399s 0.539s
after| 0.736s0.159s 0.248s 0.501s
nr. patterns |89 376 19 0
nr. tracked | 182k 40k 63k 101k
Tracked-down-by: Karsten Blees karsten.bl...@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Instead of relying on the surrounding code happening to not trigger
the behavior change in treat_one_path, this round ensures such
triggers will disable the optimization and fall back to normal code
path.
There are no big differences in measured numbers, which indicate
incorrect triggers do not happen, at least in my tests.
dir.c | 79 ---
1 file changed, 52 insertions(+), 27 deletions(-)
diff --git a/dir.c b/dir.c
index 57394e4..a5fe0a0 100644
--- a/dir.c
+++ b/dir.c
@@ -17,8 +17,11 @@ struct path_simplify {
const char *path;
};
-static int read_directory_recursive(struct dir_struct *dir, const char *path,
int len,
- int check_only, const struct path_simplify *simplify);
+static void read_directory_recursive(struct dir_struct *dir,
+const char *path, int len,
+int check_only,
+const struct path_simplify *simplify,
+int *contents);
static int get_dtype(struct dirent *de, const char *path, int len);
/* helper string functions with support for the ignore_case flag */
@@ -1034,6 +1037,7 @@ static enum directory_treatment treat_directory(struct
dir_struct *dir,
const char *dirname, int len, int exclude,
const struct path_simplify *simplify)
{
+ int contents = 0;
/* The len-1 is to strip the final '/' */
switch (directory_exists_in_index(dirname, len-1)) {
case index_directory:
@@ -1065,19 +1069,19 @@ static enum directory_treatment treat_directory(struct
dir_struct *dir,
* check if it contains only ignored files
*/
if ((dir-flags DIR_SHOW_IGNORED) !exclude) {
- int ignored;
dir-flags = ~DIR_SHOW_IGNORED;
dir-flags |= DIR_HIDE_EMPTY_DIRECTORIES;
- ignored = read_directory_recursive(dir, dirname, len, 1,
simplify);
+ read_directory_recursive(dir, dirname, len, 1, simplify,
contents);
dir-flags = ~DIR_HIDE_EMPTY_DIRECTORIES;
dir-flags |= DIR_SHOW_IGNORED;
- return ignored ? ignore_directory : show_directory;
+ return contents ? ignore_directory : show_directory;
}
if (!(dir-flags DIR_SHOW_IGNORED)
!(dir-flags DIR_HIDE_EMPTY_DIRECTORIES))
return show_directory;
- if (!read_directory_recursive(dir, dirname, len,
