Re: [PATCH v4 1/2] cat-file: teach cat-file a '--literally' option
On Tue, Mar 17, 2015 at 1:16 AM, Karthik Nayak wrote:
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index df99df4..1625246 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -323,8 +338,8 @@ static int batch_objects(struct batch_options *opt)
> }
I don't presently have time for a proper read-through, however, this
popped out while quickly running my eyes over the changes.
> static const char * const cat_file_usage[] = {
> - N_("git cat-file (-t | -s | -e | -p | | --textconv) "),
> - N_("git cat-file (--batch | --batch-check) < "),
> + N_("git cat-file (-t [--literally]|-s
> [--literally]|-e|-p||--textconv) "),
> + N_("git cat-file (--batch | --batch-check) "),
The '<' in the second line before is intentional. It
means that is provided via stdin. Therefore, it is
incorrect to remove it.
> NULL
> };
>
--
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] Add revision range support on "-" and "@{-1}"
Junio C Hamano writes:
>> @@ -1499,6 +1500,13 @@ int handle_revision_arg(const char *arg_, struct
>> rev_info *revs, int flags, unsi
>> next = head_by_default;
>> if (dotdot == arg)
>> this = head_by_default;
>> +/* Allows -.. and ..- */
>> +if (!strcmp(this, "-")) {
>> +this = prev_rev;
>> +}
>> +if (!strcmp(next, "-")) {
>> +next = prev_rev;
>> +}
>
> The above two hunks are disappointing. "this" and "next" are passed
> to get_sha1_committish() and the point of the [1/2] patch was to
> allow "-" to be just as usable as "@{-1}" anywhere a branch name can
> be used.
>
>> if (this == head_by_default && next == head_by_default &&
>> !symmetric) {
>> /*
>> @@ -2198,7 +2206,7 @@ int setup_revisions(int argc, const char **argv,
>> struct rev_info *revs, struct s
>> read_from_stdin = 0;
>> for (left = i = 1; i < argc; i++) {
>> const char *arg = argv[i];
>> -if (arg[0] == '-' && arg[1]) {
>> +if (arg[0] == '-' && !strstr(arg, "..")) {
>
> Isn't this way too loose? "--some-opt=I.wish..." would have ".."
> in it, and we would want to leave room to add new options that may
> take arbitrary string as an argument.
>
> I would have expected it would be more like
>
> if (arg[0] == '-' && arg[1] && !starts_with(arg + 1, "..")) {
>
> That is, "anything that begins with '-', if it is to be taken as an
> option, must not begin with '-..'", which I think should be strict
> enough.
I have an updated version to handle the simplest forms of range
notations on 'pu' as d40f108d ("-" and "@{-1}" on various programs,
2015-03-16). I do not think either your !strstr(arg, "..") or my
!starts_with(arg + 1, "..") is correct, if we really wanted to make
"-" a true stand-in for @{-1}, as we would need to stop ourselves
fall into this "This begins with a dash, so it has to be a dashed
option" block when things like these are given:
"-^"
"-~10"
"-^{/^### match next}"
I have a suspicion that it might be a matter of swapping the if
clauses, that is, instead of the current way
if (starts with '-') {
do the option thing;
continue;
}
if (try to see if it is a revision or revision range) {
/* if failed, args must be pathspecs from here on */
check the '--' disambiguation;
add pathspec to prune-data;
} else {
got_rev_arg = 1;
}
which tries "the option thing" first, do something like this:
if (try to see if it is a revision or regvision range) {
/* if failed ... */
if (starts with '-') {
do the option thing;
continue;
}
/* args must be pathspecs from here on */
check the '--' disambiguation;
add pathspec to prune-data;
} else {
got_rev_arg = 1;
}
but I didn't trace the logic myself to see if that would work.
--
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: Promoting Git developers
On Mon, 16 Mar 2015, Junio C Hamano wrote: David Lang writes: On Sun, 15 Mar 2015, Junio C Hamano wrote: Christian Couder writes: I wrote something about a potential Git Rev News news letter: I read it. Sounds promising. Just one suggestion on the name and half a comment. How would "Git Review" (or "Git Monthly Review", or replace your favourite "how-often-per-period-ly" in its name) sound? I meant it to sound similar to academic journals that summarize and review contemporary works in the field and keeps your original "pun" about our culture around "patch reviews". ... I'll bet that LWN would publish, or at least link to, such articles on a regular basis, and if you end up doing an in-depth writeup on a particularly discussed topic, they would probably give it pretty good visibility. I hope you are right, but my observation of our coverage by lwn.net is somewhat pessimistic. In our early days, our progress often used to appear on the "Kernel Development" page, which I presume is the most important page of the weekly for the kernel developers, but in several months, the mention of us has moved two pages back to "Development" and listed together with folks like OCaml Weekly, PostgreSQL Weekly, etc. I would not count that as "pretty good visibility" particularly. I am taking it as a positive change, though. Once we got stable enough not to be a roadblock for the kernel folks and proven ourselves not to regress, our progress probably ceased to be newsworthy to them ;-) It ceased to be about kernel development, and fell into the normal development bucket :-) Routine release notes (like your notes from the maintainer) do end up just getting links to them as you have seen. But if someone is summarizing the discussions on the mailing list, those will be a bit more interesting, and if there is a particularly hot topic, the summary of that discussion can be a full fledged article on it's own. David Lang -- 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] refs.c: get_ref_cache: use a bucket hash
On Mon, Mar 16, 2015 at 10:35:18PM -0700, Junio C Hamano wrote: > > It looks like we don't even really care about the value of HEAD. We just > > want to know "is it a git directory?". I think in other places (like > > "git add"), we just do an existence check for "$dir/.git". That would > > not catch a bare repository, but I do not think the current check does > > either (it is looking for submodules, which always have a .git). > > If we wanted to be consistent, perhaps we should be reusing the "is > this a git repository?" check used by the auto-discovery codepath > (setup.c:is_git_directory(), perhaps?), but the idea looks simple > enough and sounds sensible. Yeah, I almost suggested that, but I'm concerned that would make us inconsistent with how we report untracked files. I thought that dir.c used ".git" as a magic token there. But it seems I'm wrong. We do ignore ".git" directly in treat_path(), but treat_directory actually checks resolve_gitlink_ref. I think this will suffer the same problem as Andreas's original issue (e.g., if you run "git ls-files -o"). Likewise, I think dir.c:remove_dir_recurse is in a similar boat. Grepping for resolve_gitlink_ref, it looks like there may be others, too. All of these should be using the same test, I think. Doing that with is_git_directory() is probably OK. It is a little more expensive than we might want for mass-use (it actually opens and parses the HEAD file in each directory), but it quits early when we _don't_ see a git directory, which would be the common case here. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] refs.c: get_ref_cache: use a bucket hash
Jeff King writes:
> The get_ref_cache code was designed to scale to the actual number of
> submodules. I do not mind seeing it become a hash if people really do
> have a large number of submodules, but that is not what is happening
> here.
> ...
> So git-clean speculatively asks "what is HEAD in this maybe-submodule?". The
> right solution is probably one of:
>
> 1. In remove_dirs, find out if we have an actual submodule before calling
> resolve_gitlink_ref.
>
> 2. Teach get_ref_cache a "read-only" mode that will not auto-vivify the
> cache
> if it does not already exist.
>
> Of the two, I think (1) is probably cleaner (I think the way the ref
> code is structured, we have to create the submodule ref_cache in order
> to start looking things up in it).
Thanks for a great analysis. I too wondered if we should be growing
the per-submodule ref-cache when we are only probing.
> It looks like we don't even really care about the value of HEAD. We just
> want to know "is it a git directory?". I think in other places (like
> "git add"), we just do an existence check for "$dir/.git". That would
> not catch a bare repository, but I do not think the current check does
> either (it is looking for submodules, which always have a .git).
If we wanted to be consistent, perhaps we should be reusing the "is
this a git repository?" check used by the auto-discovery codepath
(setup.c:is_git_directory(), perhaps?), but the idea looks simple
enough and sounds sensible.
> Maybe something like (largely untested):
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index 98c103f..e2cc47b 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -148,6 +148,32 @@ static int exclude_cb(const struct option *opt, const
> char *arg, int unset)
> return 0;
> }
>
> +static int dir_is_repo(struct strbuf *path)
> +{
> + size_t orig = path->len;
> + int ret;
> +
> + strbuf_addstr(path, "/.git");
> + if (!access(path->buf, F_OK))
> + ret = 1; /* definitely */
> + else if (errno == ENOENT)
> + ret = 0; /* definitely not */
> + else {
> + /*
> + * We couldn't tell. It would probably be safer to err
> + * on the side of saying "yes" here, because we are
> + * deciding what to delete, and are more likely to keep
> + * a sub-repo. But it would probably also create annoying
> + * false positives, where a directory we do not have
> + * permission to read would say something misleading
> + * like "not deleting sub-repo foo..."
> + */
> + ret = 0;
> + }
> + strbuf_setlen(path, orig);
> + return ret;
> +}
> +
> static int remove_dirs(struct strbuf *path, const char *prefix, int
> force_flag,
> int dry_run, int quiet, int *dir_gone)
> {
> @@ -155,13 +181,11 @@ static int remove_dirs(struct strbuf *path, const char
> *prefix, int force_flag,
> struct strbuf quoted = STRBUF_INIT;
> struct dirent *e;
> int res = 0, ret = 0, gone = 1, original_len = path->len, len;
> - unsigned char submodule_head[20];
> struct string_list dels = STRING_LIST_INIT_DUP;
>
> *dir_gone = 1;
>
> - if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
> - !resolve_gitlink_ref(path->buf, "HEAD",
> submodule_head)) {
> + if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) && dir_is_repo(path)) {
> if (!quiet) {
> quote_path_relative(path->buf, prefix, "ed);
> printf(dry_run ? _(msg_would_skip_git_dir) :
> _(msg_skip_git_dir),
--
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: Promoting Git developers
David Lang writes: > On Sun, 15 Mar 2015, Junio C Hamano wrote: > >> Christian Couder writes: >> >>> I wrote something about a potential Git Rev News news letter: >> >> I read it. Sounds promising. >> >> Just one suggestion on the name and half a comment. >> >> How would "Git Review" (or "Git Monthly Review", or replace your >> favourite "how-often-per-period-ly" in its name) sound? I meant it >> to sound similar to academic journals that summarize and review >> contemporary works in the field and keeps your original "pun" about >> our culture around "patch reviews". > ... > I'll bet that LWN would publish, or at least link to, such articles on > a regular basis, and if you end up doing an in-depth writeup on a > particularly discussed topic, they would probably give it pretty good > visibility. I hope you are right, but my observation of our coverage by lwn.net is somewhat pessimistic. In our early days, our progress often used to appear on the "Kernel Development" page, which I presume is the most important page of the weekly for the kernel developers, but in several months, the mention of us has moved two pages back to "Development" and listed together with folks like OCaml Weekly, PostgreSQL Weekly, etc. I would not count that as "pretty good visibility" particularly. I am taking it as a positive change, though. Once we got stable enough not to be a roadblock for the kernel folks and proven ourselves not to regress, our progress probably ceased to be newsworthy to them ;-) -- 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 2/2] sha1_file: refactor sha1_file.c to support 'cat-file --literally'
Modify sha1_loose_object_info() to support 'cat-file --literally'
by accepting flags and also make changes to copy the type to
object_info::typename.
Add parse_sha1_header_extended() which acts as a wrapper around
parse_sha1_header() allowing for more information to be obtained
based on the given flags.
Add unpack_sha1_header_literally() to unpack sha1 headers of
unknown/corrupt objects which have a unknown sha1 header size.
This was written by Junio C Hamano but tested by me.
Helped-by: Junio C Hamano
Helped-by: Eric Sunshine
Signed-off-by: Karthik Nayak
---
sha1_file.c | 121
1 file changed, 97 insertions(+), 24 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 69a60ec..e31e9e2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1564,6 +1564,36 @@ int unpack_sha1_header(git_zstream *stream, unsigned
char *map, unsigned long ma
return git_inflate(stream, 0);
}
+static int unpack_sha1_header_literally(git_zstream *stream, unsigned char
*map,
+ unsigned long mapsize,
+ struct strbuf *header)
+{
+ unsigned char buffer[32], *cp;
+ unsigned long bufsiz = sizeof(buffer);
+ int status;
+
+ /* Get the data stream */
+ memset(stream, 0, sizeof(*stream));
+ stream->next_in = map;
+ stream->avail_in = mapsize;
+ stream->next_out = buffer;
+ stream->avail_out = bufsiz;
+
+ git_inflate_init(stream);
+
+ do {
+ status = git_inflate(stream, 0);
+ strbuf_add(header, buffer, stream->next_out - buffer);
+ for (cp = buffer; cp < stream->next_out; cp++)
+ if (!*cp)
+ /* Found the NUL at the end of the header */
+ return 0;
+ stream->next_out = buffer;
+ stream->avail_out = bufsiz;
+ } while (status == Z_OK);
+ return -1;
+}
+
static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long
size, const unsigned char *sha1)
{
int bytes = strlen(buffer) + 1;
@@ -1609,32 +1639,24 @@ static void *unpack_sha1_rest(git_zstream *stream, void
*buffer, unsigned long s
return NULL;
}
-/*
- * We used to just use "sscanf()", but that's actually way
- * too permissive for what we want to check. So do an anal
- * object header parse by hand.
- */
-int parse_sha1_header(const char *hdr, unsigned long *sizep)
+int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
+ int flags)
{
- char type[10];
- int i;
+ struct strbuf typename = STRBUF_INIT;
unsigned long size;
+ int type;
/*
* The type can be at most ten bytes (including the
* terminating '\0' that we add), and is followed by
* a space.
*/
- i = 0;
for (;;) {
char c = *hdr++;
if (c == ' ')
break;
- type[i++] = c;
- if (i >= sizeof(type))
- return -1;
+ strbuf_addch(&typename, c);
}
- type[i] = 0;
/*
* The length must follow immediately, and be in canonical
@@ -1652,12 +1674,45 @@ int parse_sha1_header(const char *hdr, unsigned long
*sizep)
size = size * 10 + c;
}
}
- *sizep = size;
+
+ type = type_from_string_gently(typename.buf, -1, 1);
+ if (oi->sizep)
+ *oi->sizep = size;
+ if (oi->typename)
+ strbuf_addstr(oi->typename, typename.buf);
+ if (oi->typep)
+ *oi->typep = type;
+ strbuf_release(&typename);
+
+ /*
+* Set type to 0 if its an unknown object and
+* we're obtaining the type using '--literally'
+* option.
+*/
+ if ((flags & LOOKUP_LITERALLY) && (type == -1))
+ type = 0;
+ else if (type == -1)
+ die("invalid object type");
/*
* The length must be followed by a zero byte
*/
- return *hdr ? -1 : type_from_string(type);
+ return *hdr ? -1 : type;
+}
+
+/*
+ * We used to just use "sscanf()", but that's actually way
+ * too permissive for what we want to check. So do an anal
+ * object header parse by hand. Calls the extended function.
+ */
+int parse_sha1_header(const char *hdr, unsigned long *sizep)
+{
+ struct object_info oi;
+
+ oi.sizep = sizep;
+ oi.typename = NULL;
+ oi.typep = NULL;
+ return parse_sha1_header_extended(hdr, &oi, LOOKUP_REPLACE_OBJECT);
}
static void *unpack_sha1_file(void *map, unsigned long mapsize, enum
object_type *type, unsigned long *size, const unsigned char *sha1)
@@ -2524,13 +2579,15 @@ struct packed_git *find_sha1_pack(const unsigned char
*sha1,
}
static int sha1_loos
[PATCH v4 1/2] cat-file: teach cat-file a '--literally' option
Currently 'git cat-file' throws an error while trying to
print the type or size of a broken/corrupt object which is
created using 'git hash-object --literally'. This is
because these objects are usually of unknown types.
Teach git cat-file a '--literally' option where it prints
the type or size of a broken/corrupt object without throwing
an error.
Modify '-t' and '-s' options to call sha1_object_info_extended()
directly to support the '--literally' option.
Add a 'LOOKUP_LITERALLY' flag to notify sha1_object_info_extended()
whenever the '--literally' is being used.
Add object_info::typename to support 'cat-file --literally' and
store the typename of objects.
Helped-by: Junio C Hamano
Helped-by: Eric Sunshine
Signed-off-by: Karthik Nayak
---
builtin/cat-file.c | 43 +--
cache.h| 2 ++
2 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index df99df4..1625246 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -9,13 +9,20 @@
#include "userdiff.h"
#include "streaming.h"
-static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
+static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
+ int literally)
{
unsigned char sha1[20];
enum object_type type;
char *buf;
unsigned long size;
struct object_context obj_context;
+ struct object_info oi = {NULL};
+ struct strbuf sb = STRBUF_INIT;
+ unsigned flags = LOOKUP_REPLACE_OBJECT;
+
+ if (literally)
+ flags |= LOOKUP_LITERALLY;
if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
die("Not a valid object name %s", obj_name);
@@ -23,16 +30,24 @@ static int cat_one_file(int opt, const char *exp_type,
const char *obj_name)
buf = NULL;
switch (opt) {
case 't':
- type = sha1_object_info(sha1, NULL);
- if (type > 0) {
- printf("%s\n", typename(type));
+ oi.typep = &type;
+ oi.typename = &sb;
+ if (sha1_object_info_extended(sha1, &oi, flags) < 0)
+ die("git cat-file: could not get object info");
+ if (sb.len) {
+ printf("%s\n", sb.buf);
+ strbuf_release(&sb);
return 0;
}
break;
case 's':
- type = sha1_object_info(sha1, &size);
- if (type > 0) {
+ oi.typep = &type;
+ oi.typename = &sb;
+ oi.sizep = &size;
+ if (sha1_object_info_extended(sha1, &oi, flags) < 0)
+ die("git cat-file: could not get object info");
+ if (sb.len) {
printf("%lu\n", size);
return 0;
}
@@ -323,8 +338,8 @@ static int batch_objects(struct batch_options *opt)
}
static const char * const cat_file_usage[] = {
- N_("git cat-file (-t | -s | -e | -p | | --textconv) "),
- N_("git cat-file (--batch | --batch-check) < "),
+ N_("git cat-file (-t [--literally]|-s
[--literally]|-e|-p||--textconv) "),
+ N_("git cat-file (--batch | --batch-check) "),
NULL
};
@@ -359,6 +374,7 @@ int cmd_cat_file(int argc, const char **argv, const char
*prefix)
int opt = 0;
const char *exp_type = NULL, *obj_name = NULL;
struct batch_options batch = {0};
+ int literally = 0;
const struct option options[] = {
OPT_GROUP(N_(" can be one of: blob, tree, commit, tag")),
@@ -369,6 +385,8 @@ int cmd_cat_file(int argc, const char **argv, const char
*prefix)
OPT_SET_INT('p', NULL, &opt, N_("pretty-print object's
content"), 'p'),
OPT_SET_INT(0, "textconv", &opt,
N_("for blob objects, run textconv on object's
content"), 'c'),
+ OPT_BOOL( 0, "literally", &literally,
+ N_("get information about corrupt objects for
debugging Git")),
{ OPTION_CALLBACK, 0, "batch", &batch, "format",
N_("show info and content of objects fed from the
standard input"),
PARSE_OPT_OPTARG, batch_option_callback },
@@ -380,7 +398,7 @@ int cmd_cat_file(int argc, const char **argv, const char
*prefix)
git_config(git_cat_file_config, NULL);
- if (argc != 3 && argc != 2)
+ if (argc < 2 || argc > 4)
usage_with_options(cat_file_usage, options);
argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);
@@ -405,5 +423,10 @@ int cmd_cat_file(int argc, const char **argv, const char
*prefix)
if (batch.enabled)
return batch_objects(&batch);
- return cat_one_file(opt, exp_type, obj_name);
+ if (liter
[PATCH v4 0/2] cat-file: add a '--literally' option
Based on Junios and Erics suggestion I have made various changes over the previous iteration of the patch[1]. Changes in this version : * Add a object_info::typename to hold all the typenames. * Add a wrapper around parse_sha1_header() to get type and size of broken/corrupt objects without throwing an error whenever the type is unknown. * Also add an option for 'cat-file -s --literally'. Thanks to Junio and Eric for their suggestions and guidance. [1]http://article.gmane.org/gmane.comp.version-control.git/264853 -- 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] refs.c: get_ref_cache: use a bucket hash
[+cc Michael for get_ref_cache wisdom]
On Mon, Mar 16, 2015 at 07:40:40PM +0100, Andreas Krey wrote:
> >I am guessing that the repository has tons
> > of submodules?
>
> Not a single one. Thats's thie interesting thing that
> makes me think I'm not actually solving the right problem.
>
> This repo has about 100k subdirectories that are ignored
> (I don't know whether directly or within ignored dirs),
> and strace said that git looks for '.git/HEAD' and one
> other file in each of these. Apparently it trieds to
> find out if any of these dirs happen to be a git repo
> which git clean treats specially, but it seems it also
> calls get_ref_cache for each of these dires even though
> the turn out not to be a sub-repo.
>
> In other words: I suspect that get_ref_cache shouldn't
> be called that often, or that the cache entries should
> be removed once a directory is found not to be a sub repo.
> Then the linear list wouldn't really hurt.
Yeah, I'd agree.
The get_ref_cache code was designed to scale to the actual number of
submodules. I do not mind seeing it become a hash if people really do
have a large number of submodules, but that is not what is happening
here.
Bisecting, it looks like things got slow for your case starting in
f538a91 (git-clean: Display more accurate delete messages, 2013-01-11).
I reproduced with basically:
git init
for i in $(seq 3); do mkdir $i; done
time git clean -nd >/dev/null
It jumps in that commit from ~50ms to ~3000ms.
A backtrace from get_ref_cache shows:
#0 get_ref_cache (submodule=0xa6a4f0 "1") at refs.c:1070
#1 0x00516469 in resolve_gitlink_ref (path=0xa6a4d0 "1/",
refname=0x584822 "HEAD",
sha1=0x7fffde90 "\002") at refs.c:1429
#2 0x00423584 in remove_dirs (path=0x7fffe2f0, prefix=0x0,
force_flag=2, dry_run=1, quiet=0,
dir_gone=0x7fffe314) at builtin/clean.c:164
#3 0x004255a9 in cmd_clean (argc=0, argv=0x7fffe5e0, prefix=0x0)
at builtin/clean.c:981
#4 0x00405554 in run_builtin (p=0x7f7b18 , argc=2,
argv=0x7fffe5e0) at git.c:348
#5 0x00405761 in handle_builtin (argc=2, argv=0x7fffe5e0) at
git.c:530
#6 0x0040587d in run_argv (argcp=0x7fffe4cc,
argv=0x7fffe4d8) at git.c:576
#7 0x00405a6e in main (argc=2, av=0x7fffe5d8) at git.c:685
So git-clean speculatively asks "what is HEAD in this maybe-submodule?". The
right solution is probably one of:
1. In remove_dirs, find out if we have an actual submodule before calling
resolve_gitlink_ref.
2. Teach get_ref_cache a "read-only" mode that will not auto-vivify the cache
if it does not already exist.
Of the two, I think (1) is probably cleaner (I think the way the ref
code is structured, we have to create the submodule ref_cache in order
to start looking things up in it).
It looks like we don't even really care about the value of HEAD. We just
want to know "is it a git directory?". I think in other places (like
"git add"), we just do an existence check for "$dir/.git". That would
not catch a bare repository, but I do not think the current check does
either (it is looking for submodules, which always have a .git).
Maybe something like (largely untested):
diff --git a/builtin/clean.c b/builtin/clean.c
index 98c103f..e2cc47b 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -148,6 +148,32 @@ static int exclude_cb(const struct option *opt, const char
*arg, int unset)
return 0;
}
+static int dir_is_repo(struct strbuf *path)
+{
+ size_t orig = path->len;
+ int ret;
+
+ strbuf_addstr(path, "/.git");
+ if (!access(path->buf, F_OK))
+ ret = 1; /* definitely */
+ else if (errno == ENOENT)
+ ret = 0; /* definitely not */
+ else {
+ /*
+* We couldn't tell. It would probably be safer to err
+* on the side of saying "yes" here, because we are
+* deciding what to delete, and are more likely to keep
+* a sub-repo. But it would probably also create annoying
+* false positives, where a directory we do not have
+* permission to read would say something misleading
+* like "not deleting sub-repo foo..."
+*/
+ ret = 0;
+ }
+ strbuf_setlen(path, orig);
+ return ret;
+}
+
static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
int dry_run, int quiet, int *dir_gone)
{
@@ -155,13 +181,11 @@ static int remove_dirs(struct strbuf *path, const char
*prefix, int force_flag,
struct strbuf quoted = STRBUF_INIT;
struct dirent *e;
int res = 0, ret = 0, gone = 1, original_len = path->len, len;
- unsigned char submodule_head[20];
struct string_list dels = STRING_LIST_INIT_DUP;
*dir_gone = 1;
- if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
-
Re: [GSoC] Applying for conversion scripts to builtins
On Tue, Mar 17, 2015 at 7:22 AM, Paul Tan wrote: > Hi, > > On Tue, Mar 17, 2015 at 12:49 AM, Yurii Shevtsov wrote: >> I'm going to write for this idea. As I know good proposal should >> contain timeline and Todo estimations. What should I write in my >> proposal, since there is no clear plan for converting scripts to >> builtins. Thanks in advance! > > I'm actually writing a proposal for the same topic because I somehow > ended up with a working prototype of git-pull.c while exploring the > internal git API ;). It's not ready as a patch yet though as there are > some problems with git's internal API which causes e.g. double free > errors and too much code complexity due to required functionality not > being exposed by builtins, which will have to be addressed. > > Generally, it would be easy to convert any shell script to C by just > using the run_command* functions (and in less lines of code), but that > would not be taking advantage of the potential benefits in porting > shell scripts to C. To summarize the (ideal) requirements: While run_command() is not ideal, it would be a good intermediate state where you can verify with the test suite that the C skeleton after rewrite is working ok. Then you can start killing run_command() in subsequent patches. That would be much easier to review code too. -- 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: [GSoC] Applying for conversion scripts to builtins
Hi, On Tue, Mar 17, 2015 at 12:49 AM, Yurii Shevtsov wrote: > I'm going to write for this idea. As I know good proposal should > contain timeline and Todo estimations. What should I write in my > proposal, since there is no clear plan for converting scripts to > builtins. Thanks in advance! I'm actually writing a proposal for the same topic because I somehow ended up with a working prototype of git-pull.c while exploring the internal git API ;). It's not ready as a patch yet though as there are some problems with git's internal API which causes e.g. double free errors and too much code complexity due to required functionality not being exposed by builtins, which will have to be addressed. Generally, it would be easy to convert any shell script to C by just using the run_command* functions (and in less lines of code), but that would not be taking advantage of the potential benefits in porting shell scripts to C. To summarize the (ideal) requirements: * zero spawning of processes so that the internal object/config/index cache can be taken advantage of. (and to avoid the process spawning overhead which is relative large in e.g. Windows) * avoid needless parsing since we have direct access to the C data structures. * use the internal API as much as possible: share code between the builtins (e.g. fmt-merge-msg.c, exposed in fmt-merge-msg.h) in order to reduce code complexity. The biggest wins would definitely be portability, but there may be performance improvements, though they are theoretical at this point. I'm not exactly sure if the above requirements are sane, which is why I'm also CC-ing Dscho who knows the problems of git on Windows more than I do. Regards, Paul -- 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] show-branch: show all local heads when only giving one rev along --topics
On Mon, Mar 16, 2015 at 05:38:06PM +0900, Mike Hommey wrote: > "git show-branch --topics ..." displays ancestry graph, only > considering commits that are in all given revs, except the first one. > > "git show-branch" displays ancestry graph for all local branches. > > Unfortunately, "git show-branch --topics " only prints out the rev > info for the given rev, and nothing else, e.g.: > > $ git show-branch --topics origin/master > [origin/master] Sync with 2.3.3 > > While there is an option to add all remote-tracking branches (-r), and > another to add all local+remote branches (-a), there is no option to add > only local branches. Adding such an option could be considered, but a > user would likely already expect that the above command line considers > the lack of rev other than for --topics as meaning all local branches, > like when there is no argument at all. > > Moreover, when using -r and -a along with --topics, the first local or > remote-tracking branch, depending on alphabetic order is used instead of > the one given after --topics (any rev given on the command line is > actually simply ignored when either -r or -a is given). And if no rev is > given at all, the fact that the first alphetical branch is the base of > topics is probably not expected by users (Maybe --topics should always > require one rev on the command line?) > > This change makes > "show-branch --topics $rev" > act as > "show-branch --topics $rev $(git for-each-ref refs/heads >--format='%(refname:short)')" > > "show-branch -r --topics $rev ..." > act as > "show-branch --topics $rev ... $(git for-each-ref refs/remotes >--format='%(refname:short)')" > instead of > "show-branch --topics $(git for-each-ref refs/remotes > --format='%(refname:short)')" > > and > "show-branch -a --topics $rev ..." > act as > "show-branch --topics $rev ... $(git for-each-ref refs/heads refs/remotes >--format='%(refname:short)')" > instead of > "show-branch --topics $(git for-each-ref refs/heads refs/remotes > --format='%(refname:short)')" Sorry, this was missing: Signed-off-by: Mike Hommey Mike -- 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: Promoting Git developers
On Sun, 15 Mar 2015, Junio C Hamano wrote: Christian Couder writes: I wrote something about a potential Git Rev News news letter: I read it. Sounds promising. Just one suggestion on the name and half a comment. How would "Git Review" (or "Git Monthly Review", or replace your favourite "how-often-per-period-ly" in its name) sound? I meant it to sound similar to academic journals that summarize and review contemporary works in the field and keeps your original "pun" about our culture around "patch reviews". I obviously do not know how the actual contents would look like at this point, but depending on the quality of the publication I might be able to steal some descriptions when keeping the notes on topics in flight that appear in my "What's cooking" report. And it can go the other way around, too. The publication may want to peek my "What's cooking" report for hints on how to characterize each topic and assess its impact to the evolution of Git. I'll bet that LWN would publish, or at least link to, such articles on a regular basis, and if you end up doing an in-depth writeup on a particularly discussed topic, they would probably give it pretty good visibility. David Lang -- 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: Slow git pushes: sitting 1 minute in pack-objects
On Mon, Mar 09, 2015 at 09:37:25PM -0400, Stephen Morton wrote:
> 3. Not sure how long this part takes. It takes 1/3 - 1/2 of the time
> when straced, but I think it's much less, as little as 10s when not
> straced.
> It then reads a bunch of what look like objects from filehandle 0
> (presumably stdin, read from the remote end?)
> It then tries to lstat() these filenames in .git/
> ./git/refs/, .git/heads/, etc. It always fails ENOENT.
> It fails some 120,000 times. This could be a problem. Though I haven't
> checked to see if this happens on a fast push on another machine.
Hmm. The "push" process must feed the set of object boundaries to
"pack-objects" so it knows what to pack (i.e., what we want to send, and
what the other side has).
120,000 is an awfully large number of objects to be pass there, though.
Does the repo you are pushing to by any chance have an extremely large
number of refs (e.g., on the order of 120K tags)?
Can you try building git with this patch which would tell us more about
where those objects are coming from?
diff --git a/send-pack.c b/send-pack.c
index 9d2b0c5..17ace1f 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -74,10 +74,19 @@ static int pack_objects(int fd, struct ref *refs, struct
sha1_array *extra, stru
* We feed the pack-objects we just spawned with revision
* parameters by writing to the pipe.
*/
+ warning("feeding %d .have objects", extra->nr);
for (i = 0; i < extra->nr; i++)
if (!feed_object(extra->sha1[i], po.in, 1))
break;
+ {
+ struct ref *p;
+ int count = 0;
+ for (p = refs; p; p = p->next)
+ count++;
+ warning("feeding %d refs", count);
+ }
+
while (refs) {
if (!is_null_sha1(refs->old_sha1) &&
!feed_object(refs->old_sha1, po.in, 1))
> 4. Then it just sits there for almost 1 minute. It uses up almost 100%
> of a single core during this period. It spends a lot of time running
> lots of brk() (memory allocation) commands and then getting (polled
> by?) a SIGALRM every 1s.
> About 5.5+ GB (about the repo size) of VIRT memory is allocated. Only
> about 400M is ever RES.
The SIGALRM is normal. That's the progress code checking in every 2
seconds to see if there's anything to print. My guess is that the
allocation is probably coming as part of the "preferred base" code. This
is a fancy term for "stuff we are not going to send, but which we could
use as delta bases for objects we are sending".
Does the patch below speed things up (it is not a good thing to do in
general, but would let us know if that is the problematic area):
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d816587..c90a352 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2307,7 +2307,6 @@ static void show_object(struct object *obj,
static void show_edge(struct commit *commit)
{
- add_preferred_base(commit->object.sha1);
}
struct in_pack_object {
If not, then the slowdown may come before we even get there, and
possibly this patch would help:
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d816587..473c0a3 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2531,6 +2531,8 @@ static void get_object_list(int ac, const char **av)
}
die("not a rev '%s'", line);
}
+ if (*line == '^')
+ continue;
if (handle_revision_arg(line, &revs, flags,
REVARG_CANNOT_BE_FILENAME))
die("bad revision '%s'", line);
}
Those are all rather blunt debugging methods, but hopefully it can get
us a sense of where the time is going.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] Add a new config. option for skipping merges in git-log
Koosha Khajehmoogahi writes:
> On 03/16/2015 09:50 PM, Junio C Hamano wrote:
>> The command line overrides the config, no? If you set up what the
>> command line defaults to from the config, let the command line
>> parser do whatever it wants to do, and do nothing else after the
>> command line parser returns, wouldn't that be sufficient?
>>
>> Puzzled...
>>
>
> Yes, the command line overrides the config. But, the config and command
> line parameters are parsed in two different files. The question is how
> you would read the config in revision.c while parsing the command line
> when there is no function in revision.c for that?
What I had in mind was along the line of the attached diff.
If I were doing this new feature, it would be in two-patch series,
whose first patch [1/2] would be just the revision.[ch] to give
these three "canned" selection options (we obviously need to update
the documentation and also add tests to make sure the new option
behaves sensibly when used alone, and in conjunction with existing
"--merges" and "--no-merges" options). The second patch [2/2] would
teach git_log_config() to read the new configuration value and keep
it in a file-scope static variable in builtin/log.c, and then call
parse_merges_opt() with that value in the codepath somewhere after
init_revisions() is called on &rev and before setup_revisions() is
called.
Note that the addition I made to cmd_log() below is for illustration
purposes only; I have no strong opinion on whether it is at the
right place in the codepath (it is one place that is "between
init_revisions() and setup_revisions()", but it may not be the best
such place). The call may want to be made a lot later in the
codepath, possibly inside cmd_log_init() instead of cmd_log(), for
example. The choice depends on how widely you would want this new
configuration be honored, which I didn't think too deeply. The
questions you would need to answer before deciding where is the best
place to make the call include:
- Should "git whatchanged" also pay attention to it?
- What about "git show"?
etc.
builtin/log.c | 5 +
revision.c| 20
revision.h| 2 ++
3 files changed, 27 insertions(+)
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..11a191d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static int decoration_given;
static int use_mailmap_config;
static const char *fmt_patch_subject_prefix = "PATCH";
static const char *fmt_pretty;
+static const char *log_merges;
static const char * const builtin_log_usage[] = {
N_("git log [] [] [[--] ...]"),
@@ -397,6 +398,8 @@ static int git_log_config(const char *var, const char
*value, void *cb)
return 0;
}
+ if (!strcmp(var, "log.merges"))
+ return git_config_string(&log_merges, var, value);
if (grep_config(var, value, cb) < 0)
return -1;
if (git_gpg_config(var, value, cb) < 0)
@@ -628,6 +631,8 @@ int cmd_log(int argc, const char **argv, const char *prefix)
init_revisions(&rev, prefix);
rev.always_show_header = 1;
+ if (log_merges && parse_merges_opt(&rev, log_merges))
+ die("unknown config value for log.merges: %s", log_merges);
memset(&opt, 0, sizeof(opt));
opt.def = "HEAD";
opt.revarg_opt = REVARG_COMMITTISH;
diff --git a/revision.c b/revision.c
index dbee26b..2fa37b0 100644
--- a/revision.c
+++ b/revision.c
@@ -1678,6 +1678,23 @@ static void add_message_grep(struct rev_info *revs,
const char *pattern)
add_grep(revs, pattern, GREP_PATTERN_BODY);
}
+int parse_merges_opt(struct rev_info *revs, const char *param)
+{
+ if (!strcmp(param, "both")) {
+ revs->min_parents = 0;
+ revs->max_parents = -1;
+ } else if (!strcmp(param, "only")) {
+ revs->min_parents = 2;
+ revs->max_parents = -1;
+ } else if (!strcmp(param, "skip")) {
+ revs->min_parents = 0;
+ revs->max_parents = 1;
+ } else {
+ return -1;
+ }
+ return 0;
+}
+
static int handle_revision_opt(struct rev_info *revs, int argc, const char
**argv,
int *unkc, const char **unkv)
{
@@ -1812,6 +1829,9 @@ static int handle_revision_opt(struct rev_info *revs, int
argc, const char **arg
revs->max_parents = atoi(arg+14);
} else if (starts_with(arg, "--no-max-parents")) {
revs->max_parents = -1;
+ } else if (starts_with(arg, "--merges=")) {
+ if (parse_merges_opt(revs, arg + 9))
+ die("unknown option: %s", arg);
} else if (!strcmp(arg, "--boundary")) {
revs->boundary = 1;
} else if (!strcmp(arg, "--left-right")) {
diff --git a/revision.h b/revision.h
index 0ea8b4e..640589c 100644
--- a/revision.h
+++ b/revision.h
@@ -240,6 +240,8 @@ extern int setup_revisions(int
Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect
"Philip Oakley" writes: > From: "Junio C Hamano" > >> Hence, if you have a history that looks like this: >> >> >> G...1---2---3---4---6---8---B >>\ >> 5---7---B >> >> it follows that 4 must also be "bad". It used to be good long time >> ago somewhere before 1, and somewhere along way on the history, >> there was a single breakage event that we are hunting for. That >> single event cannot be 5, 6, 7 or 8 because breakage at say 5 would >> not explain why the tip of the upper branch is broken---its breakage >> has no way to propagate there. The breakage must have happened at 4 >> or before that commit. > > Is it not worth at least confirming the assertion that 4 is bad before > proceding, or at least an option to confirm that in complex scenarios > where the fault may be devious. That raises a somewhat interesting tangent. Christian seems to be forever interested in bisect, so I'll add him to the Cc list ;-) There is no way to give multiple "bad" from the command line. You can say "git bisect start rev rev rev..." but that gives only one bad and everything else is good. And once you specify one of the above two bad ones (say, the child of 8), then we will not even offer the other one (i.e. the child of 7) as a candidate to be tested. So in that sense, "confirm that 4 is bad before proceeding" is a moot point. However, you can say "git bisect bad " (and "git bisect good " for that matter) on a rev that is unrelated to what the current bisection state is. E.g. after you mark the child of 8 as "bad", the bisected graph would become G...1---2---3---4---6---8---B and you would be offered to test somewhere in the middle, say, 4. But it is perfectly OK for you to respond with "git bisect bad 7", if you know 7 is bad. I _think_ the current code blindly overwrites the "bad" pointer, making the bisection state into this graph if you do so. G...1---2---3---4 \ 5---B This is very suboptimal. The side branch 4-to-7 could be much longer than the original trunk 4-to-the-tip, in which case we would have made the suspect space _larger_, not smaller. We certainly should be able to take advantage of the fact that the current "bad" commit (i.e. the child of 8) and the newly given "bad" commit (i.e. 7) are both known to be bad and mark 4 as "bad" instead when that happens, instead of doing the suboptimal thing the code currently does. -- 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] [RFC] Add a new config. option for skipping merges in git-log
On 03/16/2015 09:50 PM, Junio C Hamano wrote: > The command line overrides the config, no? If you set up what the > command line defaults to from the config, let the command line > parser do whatever it wants to do, and do nothing else after the > command line parser returns, wouldn't that be sufficient? > > Puzzled... > Yes, the command line overrides the config. But, the config and command line parameters are parsed in two different files. The question is how you would read the config in revision.c while parsing the command line when there is no function in revision.c for that? Sorry if I look baffled. -- Koosha -- 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] [RFC] Add a new config. option for skipping merges in git-log
Koosha Khajehmoogahi writes: > Thanks for your suggestions. The "extra bit" in rev_info is used when > we need to compare user's command-line input to his configuration. Since > command-line input is processed in revision.c but config. options are read > in builtin/log.c, we need a way to pass the value to builtin/log.c. How > would you do that without this extra bit? The command line overrides the config, no? If you set up what the command line defaults to from the config, let the command line parser do whatever it wants to do, and do nothing else after the command line parser returns, wouldn't that be sufficient? Puzzled... -- 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] rev-list: refuse --first-parent combined with --bisect
From: "Junio C Hamano" Kevin Daudt writes: So this ref changes to the bad commit. For refs/bisect/good-*, I could only find an example snippet: GOOD=$(git for-each-ref "--format=%(objectname)" refs/bisect/good-*) But it's not really clear what * might be expanded to, nor what they mean. I guess this could use some clarrification in the documentation. Because the history is not linear in Git, bisection works by shrinking a subgraph of the history DAG that contains "yet to be tested, suspected to have introduced a badness" commits. The subgraph is defined as anything reachable from _the_ "bad" commit (initially, the one you give to the command when you start) that are not reachable from any of the "good" commits. Suppose you started from this graph. Time flows left to right as usual. ---0---2---4---6---8---9 \ / 1---3---5---7 Then mark the initial good and bad commits as G and B. ---G---2---4---6---8---B \ / 1---3---5---7 And imagine that you are asked to check 4, which turns out to be good. We do not _move_ G to 4; we mark 4 as good, while keeping 0 also as good. ---G---2---G---6---8---B \ / 1---3---5---7 And if you are next asked to check 5, and mark it as good, the graph will become like this: ---G---2---G---6---8---B \ / 1---3---G---7 Of course, at this point, the subgraph of suspects are 6, 7, 8 and 9, and the subgraph no longer is affected by the fact that 0 is good. But it is crucial to keep 0 marked as good in the step before this one, before you tested 5, as that is what allows us not having to test any ancestors of 0 at all. Now, one may wonder why we need multiple "good" commits but we do not need multiple "bad" commits. This comes from the nature of "bisection", which is a tool to find a _single_ breakage [*1*], and a fundamental assumption is that a breakage does not fix itself. Hence, if you have a history that looks like this: G...1---2---3---4---6---8---B \ 5---7---B it follows that 4 must also be "bad". It used to be good long time ago somewhere before 1, and somewhere along way on the history, there was a single breakage event that we are hunting for. That single event cannot be 5, 6, 7 or 8 because breakage at say 5 would not explain why the tip of the upper branch is broken---its breakage has no way to propagate there. The breakage must have happened at 4 or before that commit. Is it not worth at least confirming the assertion that 4 is bad before proceding, or at least an option to confirm that in complex scenarios where the fault may be devious. [the explicit explanation has been useful for me...] Which means that if you marked the child of 8 (the tip of the upper branch) as bad, there is no reason for us to even look at the lower branch. As soon as you mark the tip of the upper branch "bad", the bisection can become G...1---2---3---4---6---8---B and without looking at the lower branch, it can find the single breakage. [Footnote] *1* You may be hunting for a single _fix_, and flipping the meaning of "good" and "bad", say "It used to be broken but somewhere we seem to have fixed that bug. Where did we do that?", marking the ones that still has the bug "good" and the ones that no longer has the bug "bad". In that context, you would be looking for a single fix. A more neutral term might be - we look for a single event that changes some state. - old state before that single event is spelled G O O D, but it is pronounced "not yet". - new state before that single event is spelled B A D, but it is pronounced "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: [RFC] git submodule purge
(+cc: Jens and Heiko, submodule experts) Hi, Patrick Steinhardt wrote: > This proposal is just for discussion. If there is any interest I > will implement the feature and send some patches. > > Currently it is hard to properly remove submodules. That is when > a submodule is deinitialized and removed from a repository the > directory '.git/modules/' will still be present and > there is no way to remove it despite manually calling `rm` on it. > I think there should be a command that is able to remove those > dangling repositories if the following conditions are met: > > - the submodule should not be initialized > > - the submodule should not have an entry in .gitmodules in the > currently checked out revision > > - the submodule should not contain any commits that are not > upstream > > - the submodule should not contain other submodules that do not > meet those conditions > > This would ensure that it is hard to loose any commits that may > be of interest. In the case that the user knows what he is doing > we may provide a '--force' switch to override those checks. Those conditions look simultaneously too strong and too weak. ;-) In principle, it should be safe to remove .git/modules/ as long as (1) it (and its submodules, sub-sub-modules, etc) doesn't have any un-pushed local commits. (2) it is not being referred to by a .git file in the work tree of the parent repository. Condition (1) can be relaxed if the user knows what they are losing and is okay with that. Condition (2) can be avoided by removing (de-initing) the copy of that submodule in the worktree at the same time. The functionality sounds like a useful thing to have, whether as an option to 'git submodule deinit' or as a new subcommand. In the long term I would like it to be possible to do everything 'git submodule' can do using normal git commands instead of that specialized interface. What command do you think this would eventually belong in? (An option to "git gc", maybe?) Thanks, 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] [RFC] Add a new config. option for skipping merges in git-log
On 03/16/2015 06:53 PM, Junio C Hamano wrote:
> Koosha Khajehmoogahi writes:
>
>> This patch adds a 'showmerges' config. option for git-log.
>> This option determines whether the log should contain merge
>> commits or not. In essence, if this option is set to false,
>> git-log will be run as 'git-log --no-merges'.
>>
>> To force git-log to show merges even if 'log.showmerges' is
>> set, we use --include-merges command line option.
>
> Yuck.
>
> I agree that there is currently no way to revert the setting that is
> touched by --merges and --no-merges back to the default, but I think
> the right way to fix that is by streamlining these two options,
> instead of piling yet another kludge --include-merges on top.
>
> When we think about possible "canned" selection modes:
>
> (do we show merge commits?) * (do we show non-merge commits?)
>
> we have four combinations. Answering the above two questions with
> No/No would end up showing nothing, which is meaningless, so that
> leaves us three choices (of course, the user could choose to futz
> directly with min/max-parents to select only Octopus merges, but
> that is a more advanced/exotic usage).
>
> Wouldn't it make more sense to spell which selection mode the user
> wants with:
>
> git log --merges=
>
> by naming the three meaningful selection modes with short and sweet
> names? Perhaps
>
> default? show? both? -- show both merge commits and non-merge commits
> only -- show only merge commits
> skip -- show only non-merge commits
>
> or something?
>
> Now, as I always say, I am not great at naming things, so do not
> take these names as the final suggestion, but I think you got the
> idea.
>
> Of course, then the traditional "--merges" option can be kept as a
> short-hand for "--merges=only", and "--no-merges" would become a
> short-hand for "--merges=skip".
>
> Once we have done that streamlining of the command line options, it
> will naturally follow that "log.merges = show | only | skip" would
> be a useful configuration option.
>
> I doubt we need an extra bit in rev_info to implement such a syntax
> sugar.
>
>> diff --git a/revision.h b/revision.h
>> index 0ea8b4e..f496472 100644
>> --- a/revision.h
>> +++ b/revision.h
>> @@ -145,6 +145,7 @@ struct rev_info {
>> unsigned inttrack_linear:1,
>> track_first_time:1,
>> linear:1;
>> +unsigned int force_show_merges:1;
>>
>> enum date_mode date_mode;
Thanks for your suggestions. The "extra bit" in rev_info is used when
we need to compare user's command-line input to his configuration. Since
command-line input is processed in revision.c but config. options are read
in builtin/log.c, we need a way to pass the value to builtin/log.c. How
would you do that without this extra bit?
--
Koosha
--
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/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.
Yurii Shevtsov writes: > Yes, I have red what you wrote several times and tried your example. > I'm really sorry if I sound like I just ignored it. I just got a > little bit lost about which procedure needs patching. You're > absolutely right, queue_diff() is wrong place for it. So do you agree > that "append the name of the file at the end of the directory" logic > should be put to diff_no_index() which will also include calling > get_mode() for each path[] member? Sorry again for asking so much > questions Questions are to be asked; no need to apologize. I think that the "if asked to compare D and F, pretend as if asked to compare D/F and F" and friends (meaning, "if asked to compare F and D, compare F and D/F" needs to be handled the same way, and also it needs to handle the case where "D/F" does *not* exist) logic can be added to diff_no_index() just before it calls queue_diff(). "If one is directory but not the other, return an error" code may need to be fixed but I think that would be a larger change than a few hours work (which I understand is the size GSoC Micro aims for). -- 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] log: forbid log --graph --no-walk
On 03/16/2015 09:08 PM, Junio C Hamano wrote: > Perhaps 13b25381 (revision: forbid combining --graph and --no-walk, > 2015-03-11) that is queued on 'pu' would be a good answer to this > question? Didn't notice a patch was queued on 'pu', 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] log: forbid log --graph --no-walk
Manos Pitsidianakis writes: > On 03/15/2015 03:39 AM, Dongcan Jiang wrote: >> Because "revs->no_walk" gets set when it comes to "git show". > > So basically rewriting t4052-stat-output.sh to exclude git show --graph > cases (or similar) is not enough. If rewriting git-show code is what is > needed, is that in the scope of a microproject? Perhaps 13b25381 (revision: forbid combining --graph and --no-walk, 2015-03-11) that is queued on 'pu' would be a good answer to this question? -- 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] log: forbid log --graph --no-walk
On 03/15/2015 03:39 AM, Dongcan Jiang wrote: > Because "revs->no_walk" gets set when it comes to "git show". So basically rewriting t4052-stat-output.sh to exclude git show --graph cases (or similar) is not enough. If rewriting git-show code is what is needed, is that in the scope of a microproject? -- 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] rev-list: refuse --first-parent combined with --bisect
Kevin Daudt writes: > So this ref changes to the bad commit. > > For refs/bisect/good-*, I could only find an example snippet: > >> GOOD=$(git for-each-ref "--format=%(objectname)" refs/bisect/good-*) > > But it's not really clear what * might be expanded to, nor what they > mean. I guess this could use some clarrification in the documentation. Because the history is not linear in Git, bisection works by shrinking a subgraph of the history DAG that contains "yet to be tested, suspected to have introduced a badness" commits. The subgraph is defined as anything reachable from _the_ "bad" commit (initially, the one you give to the command when you start) that are not reachable from any of the "good" commits. Suppose you started from this graph. Time flows left to right as usual. ---0---2---4---6---8---9 \ / 1---3---5---7 Then mark the initial good and bad commits as G and B. ---G---2---4---6---8---B \ / 1---3---5---7 And imagine that you are asked to check 4, which turns out to be good. We do not _move_ G to 4; we mark 4 as good, while keeping 0 also as good. ---G---2---G---6---8---B \ / 1---3---5---7 And if you are next asked to check 5, and mark it as good, the graph will become like this: ---G---2---G---6---8---B \ / 1---3---G---7 Of course, at this point, the subgraph of suspects are 6, 7, 8 and 9, and the subgraph no longer is affected by the fact that 0 is good. But it is crucial to keep 0 marked as good in the step before this one, before you tested 5, as that is what allows us not having to test any ancestors of 0 at all. Now, one may wonder why we need multiple "good" commits but we do not need multiple "bad" commits. This comes from the nature of "bisection", which is a tool to find a _single_ breakage [*1*], and a fundamental assumption is that a breakage does not fix itself. Hence, if you have a history that looks like this: G...1---2---3---4---6---8---B \ 5---7---B it follows that 4 must also be "bad". It used to be good long time ago somewhere before 1, and somewhere along way on the history, there was a single breakage event that we are hunting for. That single event cannot be 5, 6, 7 or 8 because breakage at say 5 would not explain why the tip of the upper branch is broken---its breakage has no way to propagate there. The breakage must have happened at 4 or before that commit. Which means that if you marked the child of 8 (the tip of the upper branch) as bad, there is no reason for us to even look at the lower branch. As soon as you mark the tip of the upper branch "bad", the bisection can become G...1---2---3---4---6---8---B and without looking at the lower branch, it can find the single breakage. [Footnote] *1* You may be hunting for a single _fix_, and flipping the meaning of "good" and "bad", say "It used to be broken but somewhere we seem to have fixed that bug. Where did we do that?", marking the ones that still has the bug "good" and the ones that no longer has the bug "bad". In that context, you would be looking for a single fix. A more neutral term might be - we look for a single event that changes some state. - old state before that single event is spelled G O O D, but it is pronounced "not yet". - new state before that single event is spelled B A D, but it is pronounced "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] refs.c: get_ref_cache: use a bucket hash
On Mon, 16 Mar 2015 10:23:05 +, Junio C Hamano wrote: > Andreas Krey writes: > ... > say "a lot of ignored directories", but do you mean directories in > the working tree (which I suppose do not have much to do with the > submodule_ref_caches[])? Apparently, they do. >I am guessing that the repository has tons > of submodules? Not a single one. Thats's thie interesting thing that makes me think I'm not actually solving the right problem. This repo has about 100k subdirectories that are ignored (I don't know whether directly or within ignored dirs), and strace said that git looks for '.git/HEAD' and one other file in each of these. Apparently it trieds to find out if any of these dirs happen to be a git repo which git clean treats specially, but it seems it also calls get_ref_cache for each of these dires even though the turn out not to be a sub-repo. In other words: I suspect that get_ref_cache shouldn't be called that often, or that the cache entries should be removed once a directory is found not to be a sub repo. Then the linear list wouldn't really hurt. I'll look into that tomorrow, and also into the hashmap API. Andreas -- "Totally trivial. Famous last words." From: Linus Torvalds Date: Fri, 22 Jan 2010 07:29:21 -0800 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Add revision range support on "-" and "@{-1}"
Kenny Lee Sin Cheong writes:
> diff --git a/revision.c b/revision.c
> index 7778bbd..a79b443 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1490,6 +1490,7 @@ int handle_revision_arg(const char *arg_, struct
> rev_info *revs, int flags, unsi
> int symmetric = *next == '.';
> unsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);
> static const char head_by_default[] = "HEAD";
> + static const char prev_rev[] = "@{-1}";
> unsigned int a_flags;
>
> *dotdot = 0;
> @@ -1499,6 +1500,13 @@ int handle_revision_arg(const char *arg_, struct
> rev_info *revs, int flags, unsi
> next = head_by_default;
> if (dotdot == arg)
> this = head_by_default;
> + /* Allows -.. and ..- */
> + if (!strcmp(this, "-")) {
> + this = prev_rev;
> + }
> + if (!strcmp(next, "-")) {
> + next = prev_rev;
> + }
The above two hunks are disappointing. "this" and "next" are passed
to get_sha1_committish() and the point of the [1/2] patch was to
allow "-" to be just as usable as "@{-1}" anywhere a branch name can
be used.
> if (this == head_by_default && next == head_by_default &&
> !symmetric) {
> /*
> @@ -2198,7 +2206,7 @@ int setup_revisions(int argc, const char **argv, struct
> rev_info *revs, struct s
> read_from_stdin = 0;
> for (left = i = 1; i < argc; i++) {
> const char *arg = argv[i];
> - if (arg[0] == '-' && arg[1]) {
> + if (arg[0] == '-' && !strstr(arg, "..")) {
Isn't this way too loose? "--some-opt=I.wish..." would have ".."
in it, and we would want to leave room to add new options that may
take arbitrary string as an argument.
I would have expected it would be more like
if (arg[0] == '-' && arg[1] && !starts_with(arg + 1, "..")) {
That is, "anything that begins with '-', if it is to be taken as an
option, must not begin with '-..'", which I think should be strict
enough.
> @@ -2220,6 +2228,7 @@ int setup_revisions(int argc, const char **argv, struct
> rev_info *revs, struct s
> continue;
> }
>
> +
> opts = handle_revision_opt(revs, argc - i, argv + i,
> &left, argv);
> if (opts > 0) {
> i += opts - 1;
Noise.
> @@ -2229,7 +2238,10 @@ int setup_revisions(int argc, const char **argv,
> struct rev_info *revs, struct s
> exit(128);
> continue;
> }
> -
> + if (strstr(arg, "..")) {
> + handle_revision_arg(arg, revs, flags, revarg_opt);
> + continue;
> + }
What is this for? We will call handle_revision_arg() whether arg
has ".." or not immediately after this one.
> if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
> int j;
--
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] Applying for conversion scripts to builtins
Yurii Shevtsov writes: > I'm going to write for this idea. As I know good proposal should > contain timeline and Todo estimations. What should I write in my > proposal, since there is no clear plan for converting scripts to > builtins. Thanks in advance! The fact that there is no clear plan is part of the plan. The idea is that how much can be converted depends highly on how the GSoC goes. You already saw with your microproject that something apparently easy can be harder than it seems. See this thread for a discussion on the topic: http://thread.gmane.org/gmane.comp.version-control.git/264050/focus=21366 Now, it's up to you to make a good proposal, i.e. both convince people that you can do a good job, and OTOH be realistic about what can be done in a GSoC. -- 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
Re: [PATCH] [RFC] Add a new config. option for skipping merges in git-log
Koosha Khajehmoogahi writes:
> This patch adds a 'showmerges' config. option for git-log.
> This option determines whether the log should contain merge
> commits or not. In essence, if this option is set to false,
> git-log will be run as 'git-log --no-merges'.
>
> To force git-log to show merges even if 'log.showmerges' is
> set, we use --include-merges command line option.
Yuck.
I agree that there is currently no way to revert the setting that is
touched by --merges and --no-merges back to the default, but I think
the right way to fix that is by streamlining these two options,
instead of piling yet another kludge --include-merges on top.
When we think about possible "canned" selection modes:
(do we show merge commits?) * (do we show non-merge commits?)
we have four combinations. Answering the above two questions with
No/No would end up showing nothing, which is meaningless, so that
leaves us three choices (of course, the user could choose to futz
directly with min/max-parents to select only Octopus merges, but
that is a more advanced/exotic usage).
Wouldn't it make more sense to spell which selection mode the user
wants with:
git log --merges=
by naming the three meaningful selection modes with short and sweet
names? Perhaps
default? show? both? -- show both merge commits and non-merge commits
only -- show only merge commits
skip -- show only non-merge commits
or something?
Now, as I always say, I am not great at naming things, so do not
take these names as the final suggestion, but I think you got the
idea.
Of course, then the traditional "--merges" option can be kept as a
short-hand for "--merges=only", and "--no-merges" would become a
short-hand for "--merges=skip".
Once we have done that streamlining of the command line options, it
will naturally follow that "log.merges = show | only | skip" would
be a useful configuration option.
I doubt we need an extra bit in rev_info to implement such a syntax
sugar.
> diff --git a/revision.h b/revision.h
> index 0ea8b4e..f496472 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -145,6 +145,7 @@ struct rev_info {
> unsigned inttrack_linear:1,
> track_first_time:1,
> linear:1;
> + unsigned int force_show_merges:1;
>
> enum date_mode date_mode;
--
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/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.
Yes, I have red what you wrote several times and tried your example.
I'm really sorry if I sound like I just ignored it. I just got a
little bit lost about which procedure needs patching. You're
absolutely right, queue_diff() is wrong place for it. So do you agree
that "append the name of the file at the end of the directory" logic
should be put to diff_no_index() which will also include calling
get_mode() for each path[] member? Sorry again for asking so much
questions
2015-03-16 19:14 GMT+02:00 Junio C Hamano :
> Yurii Shevtsov writes:
>
>>> ... As it stands now, even before we think about dwimming
>>> "diff D/ F" into "diff D/F F", a simple formulation like this will
>>> error out.
>>>
>>> $ mkdir -p a/sub b
>>> $ touch a/file b/file b/sub a/sub/file
>>> $ git diff --no-index a b
>>> error: file/directory conflict: a/sub, b/sub
>>>
>>> Admittedly, that is how ordinary "diff -r" works, but I am not sure
>>> if we want to emulate that aspect of GNU diff. If the old tree a
>>> has a directory 'sub' with 'file' under it (i.e. a/sub/file) where
>>> the new tree has a file at 'sub', then the recursive diff can show
>>> the removal of sub/file and creation of sub, no? That is what we
>>> show for normal "git diff".
>>>
>>> But I _think_ fixing that is way outside the scope of GSoC Micro.
>>
>> So you want me to convert args ("diff D/ F" into "diff D/F F") before
>> calling queue_diff()? But why?
>
> Because it is wrong to do this inside queue_diff()?
>
> Have you actually read what I wrote, tried the above sample
> scenario, and thought what is happening in the codepath?
>
> When the user asks to compare directory a/ and b/, the top-level
> diff_no_index() would have paths[0]=="a" and paths[1]=="b", and
> queue_diff() is called with these in name1 and name2. Once it
> learns that both of these are directories, it _recurses_ into itself
> by appending the paths in these directories after these two names.
> It finds that both of these directories have "sub" underneath, so it
> makes a recursive call to itself to compare "a/sub" and "b/sub".
>
> That call would notice that one is a directory and the other is
> not. That is where you are getting the "f/d conflict" error.
>
> At that point, do you think it is sensible to rewrite that recursed
> part of the diff into a comparison between "a/sub/sub" (which does
> not exist, and which the user did not mean to compare with b/sub)
> and "b/sub" (which is a file)? I hope not.
>
>> queue_diff() already check args' types and decides which
>> comparison to do.
>
> Yes, and I already hinted that that is an independent issue we may
> want to fix, which I suspect is larger than GSoC Micro. Also the
> fix would be different. Right now, it checks the types of paths and
> then refuses to compare a directory and a file. If we wanted to
> change it to closer to what the rest of Git does, we would want it
> to report that the directory and everything under it are removed and
> then a new file is created (if the directory is on the left hand
> side of the comparision and the file is on the right hand side) or
> the other way around. That will not involve "append the name of the
> file at the end of the directory".
>
> In short, "append the name of the file at the end of the directory"
> logic has no place to live inside queue_diff() which handles the
> recursion part of the operation.
--
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 svn's --localtime seems to corrupt time zones
Hi. I was converting some very old svn repos of mine into git using git svn, and since I didn't fully trust the conversion process I wrote a small tool which goes through all commits/revisions (there were no branches or non-linear stuff involved in these svn repos) and compares the author names, dates, commit messages and file tree for each revision/commit. I used something like: git svn clone --trunk=/ --no-metadata --localtime --preserve-empty-dirs --authors-file=~/authors.txt file:///... and used --localtime since I wanted the times/dates with the original time zones (just as it seems to happen with a normal git repo as well). The svn repo contained commits with different time zones (mostly because of daylight saving times). With the diff tool I've noticed the following behaviour: Apparently, whenever --localtime is given, git svn takes SVN's time as is, completely ignoring it's time zone, storing *everything* in the current(!) local time zone. This has IMHO two bugs: 1) it doesn't do what one expects and what the manpage promises: > --localtime > Store Git commit times in the local time zone instead of UTC. > This makes git log (even without --date=local) show the same > times that svn log would in the local time zone. => This isn't true, cause while svn log shows the differing time zones (in my case +0100 and +0200 when the DST changes), git log shows everything in +0100. 2) but even worse, as said above, it seems to ignore the time zones from SVN, so when I'm currently in +0100, all the svn times from +0100 will be correct, but all the ones that were stored in +0200 (or anything else) will have exactly the same time value just the zone changed to +0100, thereby corrupting the time. Example svn log output: $ svn log | grep ^r | head -n 4 r781 | calestyo | 2008-08-12 23:26:12 +0200 (Tue, 12 Aug 2008) | 2 lines r780 | calestyo | 2008-01-11 01:16:59 +0100 (Fri, 11 Jan 2008) | 2 lines r779 | calestyo | 2008-01-06 19:43:08 +0100 (Sun, 06 Jan 2008) | 2 lines r778 | calestyo | 2008-01-06 18:51:37 +0100 (Sun, 06 Jan 2008) | 2 lines And from the corresponding "converted" git repo: $ git log --date=iso8601 | grep ^Date: | head -n 4 Date: 2008-08-12 23:26:12 +0100 Date: 2008-01-11 01:16:59 +0100 Date: 2008-01-06 19:43:08 +0100 Date: 2008-01-06 18:51:37 +0100 All packages from Debian sid, i.e. git 2.1.4 and subversion 1.8.10. Any ideas? Cheers, Chris. smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH] refs.c: get_ref_cache: use a bucket hash
Andreas Krey writes:
> get_ref_cache used a linear list, which obviously is O(n^2).
> Use a fixed bucket hash which just takes a factor of 10
> (~ 317^2) out of the n^2 - which is enough.
>
> Signed-off-by: Andreas Krey
> ---
>
> This brings 'git clean -ndx' times down from 17 minutes
> to 11 seconds on one of our workspaces (which accumulated
> a lot of ignored directories).
Nice.
These impressive numbers should go to the commit log message,
together with a bit more numbers to characterise the shape of the
repository that exhibits the problem with the original code. You
say "a lot of ignored directories", but do you mean directories in
the working tree (which I suppose do not have much to do with the
submodule_ref_caches[])? I am guessing that the repository has tons
of submodules? How many is "tons" to make the pain noticeable?
> Actuallly using adaptive
> hashing or other structures seems overkill.
Perhaps _implementing_ these structures only for this codepath may
be overkill, but would it be an overkill to _use_ existing hashmap.c
implementation? After all, those who wrote the original would have
thought that anything more complex than a linear list would be
overkill, and nobody disagreed until you found that your repository
disagreed with that assumption ;-)
> refs.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index e23542b..8198d9e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -982,6 +982,8 @@ struct packed_ref_cache {
> struct stat_validity validity;
> };
>
> +#define REF_CACHE_HASH 317
> +
> /*
> * Future: need to be in "struct repository"
> * when doing a full libification.
> @@ -996,7 +998,7 @@ static struct ref_cache {
>* is initialized correctly.
>*/
> char name[1];
> -} ref_cache, *submodule_ref_caches;
> +} ref_cache, *submodule_ref_caches[REF_CACHE_HASH];
>
> /* Lock used for the main packed-refs file: */
> static struct lock_file packlock;
> @@ -1065,18 +1067,19 @@ static struct ref_cache *create_ref_cache(const char
> *submodule)
> */
> static struct ref_cache *get_ref_cache(const char *submodule)
> {
> - struct ref_cache *refs;
> + struct ref_cache *refs, **bucketp;
> + bucketp = submodule_ref_caches + strhash(submodule) % REF_CACHE_HASH;
>
> if (!submodule || !*submodule)
> return &ref_cache;
>
> - for (refs = submodule_ref_caches; refs; refs = refs->next)
> + for (refs = *bucketp; refs; refs = refs->next)
> if (!strcmp(submodule, refs->name))
> return refs;
>
> refs = create_ref_cache(submodule);
> - refs->next = submodule_ref_caches;
> - submodule_ref_caches = refs;
> + refs->next = *bucketp;
> + *bucketp = refs;
> return refs;
> }
--
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] refs.c: get_ref_cache: use a bucket hash
Hi,
On 03/16, Andreas Krey wrote:
> get_ref_cache used a linear list, which obviously is O(n^2).
> Use a fixed bucket hash which just takes a factor of 10
> (~ 317^2) out of the n^2 - which is enough.
>
> Signed-off-by: Andreas Krey
> ---
>
> This brings 'git clean -ndx' times down from 17 minutes
> to 11 seconds on one of our workspaces (which accumulated
> a lot of ignored directories). Actuallly using adaptive
> hashing or other structures seems overkill.
>
> refs.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index e23542b..8198d9e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -982,6 +982,8 @@ struct packed_ref_cache {
> struct stat_validity validity;
> };
>
> +#define REF_CACHE_HASH 317
> +
> /*
> * Future: need to be in "struct repository"
> * when doing a full libification.
> @@ -996,7 +998,7 @@ static struct ref_cache {
>* is initialized correctly.
>*/
> char name[1];
> -} ref_cache, *submodule_ref_caches;
> +} ref_cache, *submodule_ref_caches[REF_CACHE_HASH];
>
> /* Lock used for the main packed-refs file: */
> static struct lock_file packlock;
> @@ -1065,18 +1067,19 @@ static struct ref_cache *create_ref_cache(const char
> *submodule)
> */
> static struct ref_cache *get_ref_cache(const char *submodule)
> {
> - struct ref_cache *refs;
> + struct ref_cache *refs, **bucketp;
> + bucketp = submodule_ref_caches + strhash(submodule) % REF_CACHE_HASH;
>
This breaks the test-suite for me, in the cases where submodule is
NULL. How about something like this on top?
diff --git a/refs.c b/refs.c
index 8198d9e..311faf2 100644
--- a/refs.c
+++ b/refs.c
@@ -1068,7 +1068,9 @@ static struct ref_cache *create_ref_cache(const char
*submodule)
static struct ref_cache *get_ref_cache(const char *submodule)
{
struct ref_cache *refs, **bucketp;
- bucketp = submodule_ref_caches + strhash(submodule) % REF_CACHE_HASH;
+ bucketp = submodule_ref_caches;
+ if (submodule)
+ bucketp += strhash(submodule) % REF_CACHE_HASH;
if (!submodule || !*submodule)
return &ref_cache;
> if (!submodule || !*submodule)
> return &ref_cache;
>
> - for (refs = submodule_ref_caches; refs; refs = refs->next)
> + for (refs = *bucketp; refs; refs = refs->next)
> if (!strcmp(submodule, refs->name))
> return refs;
>
> refs = create_ref_cache(submodule);
> - refs->next = submodule_ref_caches;
> - submodule_ref_caches = refs;
> + refs->next = *bucketp;
> + *bucketp = refs;
> return refs;
> }
>
> --
> 2.3.2.223.g7a9409c
> --
> 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
--
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/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.
Yurii Shevtsov writes:
>> ... As it stands now, even before we think about dwimming
>> "diff D/ F" into "diff D/F F", a simple formulation like this will
>> error out.
>>
>> $ mkdir -p a/sub b
>> $ touch a/file b/file b/sub a/sub/file
>> $ git diff --no-index a b
>> error: file/directory conflict: a/sub, b/sub
>>
>> Admittedly, that is how ordinary "diff -r" works, but I am not sure
>> if we want to emulate that aspect of GNU diff. If the old tree a
>> has a directory 'sub' with 'file' under it (i.e. a/sub/file) where
>> the new tree has a file at 'sub', then the recursive diff can show
>> the removal of sub/file and creation of sub, no? That is what we
>> show for normal "git diff".
>>
>> But I _think_ fixing that is way outside the scope of GSoC Micro.
>
> So you want me to convert args ("diff D/ F" into "diff D/F F") before
> calling queue_diff()? But why?
Because it is wrong to do this inside queue_diff()?
Have you actually read what I wrote, tried the above sample
scenario, and thought what is happening in the codepath?
When the user asks to compare directory a/ and b/, the top-level
diff_no_index() would have paths[0]=="a" and paths[1]=="b", and
queue_diff() is called with these in name1 and name2. Once it
learns that both of these are directories, it _recurses_ into itself
by appending the paths in these directories after these two names.
It finds that both of these directories have "sub" underneath, so it
makes a recursive call to itself to compare "a/sub" and "b/sub".
That call would notice that one is a directory and the other is
not. That is where you are getting the "f/d conflict" error.
At that point, do you think it is sensible to rewrite that recursed
part of the diff into a comparison between "a/sub/sub" (which does
not exist, and which the user did not mean to compare with b/sub)
and "b/sub" (which is a file)? I hope not.
> queue_diff() already check args' types and decides which
> comparison to do.
Yes, and I already hinted that that is an independent issue we may
want to fix, which I suspect is larger than GSoC Micro. Also the
fix would be different. Right now, it checks the types of paths and
then refuses to compare a directory and a file. If we wanted to
change it to closer to what the rest of Git does, we would want it
to report that the directory and everything under it are removed and
then a new file is created (if the directory is on the left hand
side of the comparision and the file is on the right hand side) or
the other way around. That will not involve "append the name of the
file at the end of the directory".
In short, "append the name of the file at the end of the directory"
logic has no place to live inside queue_diff() which handles the
recursion part of the operation.
--
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: Promoting Git developers
On Mon, Mar 16, 2015 at 2:20 AM, David Kastrup wrote: > > "Git Annotate"? > "Git Praise" as opposed to blame? "Git Who" as a pun on the subcommand structure which doesn't always follows grammar? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GSoC] Applying for conversion scripts to builtins
I'm going to write for this idea. As I know good proposal should contain timeline and Todo estimations. What should I write in my proposal, since there is no clear plan for converting scripts to builtins. Thanks in advance! -- 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] rev-list: refuse --first-parent combined with --bisect
On Wed, Mar 11, 2015 at 01:13:48PM -0700, Junio C Hamano wrote: > Kevin Daudt writes: > > > On Tue, Mar 10, 2015 at 04:12:18PM -0700, Junio C Hamano wrote: > > > > Step back and think why "git bisect --first-parent" is sometimes > desired in the first place. > > It is because in the regular bisection, you will almost always end > up on a commit that is _not_ on the first-parent chain and asked to > check that commit at a random place on a side branch in the first > place. And you mark such a commit as "bad". > > The thing is, traversing from that "bad" commit that is almost > always is on a side branch, following the first-parent chain, will > not be a useful history that "leaves out any merged in branches". > > When "git bisect --first-parent" feature gets implemented, "do not > use --first-parent with --bisect" limitation has to be lifted > anyway, but until then, not allowing "--first-parent --bisect" for > "rev-list" but allowing it for "log" does not buy our users much. > The output does not give us a nice "show me which merges on the > trunk may have caused the breakage to be examined with the remainder > of this bisect session". > > So, yes, there is a use case for "log --bisect --first-parent", once > there is a working "bisect --first-parent", but not until then, the > command is not useful, I would think. Thank you for you explanation. My confusion came from incorrectly assuming refs/bisect/bad and refs/bisect/good-* were pointing to the initially specified good and bad commits, in which case the combination does make sense. I was looking in the manpages for the meaning of the bisect refs, but could only find something about refs/bisect/bad: git-bisect(1): > Eventually there will be no more revisions left to bisect, and you > will have been left with the first bad kernel revision in > "refs/bisect/bad So this ref changes to the bad commit. For refs/bisect/good-*, I could only find an example snippet: > GOOD=$(git for-each-ref "--format=%(objectname)" refs/bisect/good-*) But it's not really clear what * might be expanded to, nor what they mean. I guess this could use some clarrification in the documentation. Knowing this, I agree that the combination log --bisect --first-parent doesn't make sense either. I will send in a new patch. Kevin -- 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/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.
> Matthieu Moy writes:
>
>>> --- a/diff-no-index.c
>>> +++ b/diff-no-index.c
>>> @@ -97,8 +97,25 @@ static int queue_diff(struct diff_options *o,
>>> if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
>>> return -1;
>>>
>>> - if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))
>>> - return error("file/directory conflict: %s, %s", name1,
>>> name2);
>>
>> I'm surprised to see this error message totally go away. The idea of the
>> microproject was to DWIM (do what I mean) better, but the dwim should
>> apply only when $directory/$file actually exists. Otherwise, the error
>> message should actually be raised.
>
> I actually think this check, when we really fixed "diff --no-index"
> to work sensibly, should go away and be replaced with something
> sensible. As it stands now, even before we think about dwimming
> "diff D/ F" into "diff D/F F", a simple formulation like this will
> error out.
>
> $ mkdir -p a/sub b
> $ touch a/file b/file b/sub a/sub/file
> $ git diff --no-index a b
> error: file/directory conflict: a/sub, b/sub
>
> Admittedly, that is how ordinary "diff -r" works, but I am not sure
> if we want to emulate that aspect of GNU diff. If the old tree a
> has a directory 'sub' with 'file' under it (i.e. a/sub/file) where
> the new tree has a file at 'sub', then the recursive diff can show
> the removal of sub/file and creation of sub, no? That is what we
> show for normal "git diff".
>
> But I _think_ fixing that is way outside the scope of GSoC Micro.
>
> And patching this function, because it is recursively called from
> within it, to "dwim" is simply wrong. When we see a/sub that is a
> directory and b/sub that is a file, we do *NOT* want to rewrite the
> comparison to comparison between a/sub/sub and b/sub.
>
> What needs to be fixed for the Micro is the top-level call to
> queue_diff() that is made blindly between paths[0] and paths[1]
> without checking what kind of things these are.
So you want me to convert args ("diff D/ F" into "diff D/F F") before
calling queue_diff()? But why? queue_diff() already check args' types
and decides which comparison to do. If I understood you right, it
would require calling get_mode() in diff_no_index(), while get_mode()
will be called again in queue_diff().
--
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/RFC] fetch: git fetch --deepen
Hi Junio,
Sorry for my late response.
> As this operation is not about moving _any_ refs, whether local
> branches or remote-tracking branches, any ref that used to point at
> commit B before you executed "fetch --deepen" would point at the
> same commit after the command finishes.
Thanks for clarifying the definition of "you". In this patch, it
actually would update the remote-tracking branches to the newest history.
I will keep working on it to figure out the reason.
It looks that it would be more efficient if the new history is not fetched,
as it transports less objects. Is this your main consideration on not
updating any refs?
>> - if (starts_with(line, "deepen ")) {
>> + if (starts_with(line, "depth ")) {
>> char *end;
>> - depth = strtol(line + 7, &end, 0);
>> - if (end == line + 7 || depth <= 0)
>> - die("Invalid deepen: %s", line);
>> + depth = strtol(line + 6, &end, 0);
>> + if (end == line + 6 || depth <= 0)
>> + die("Invalid depth: %s", line);
>> continue;
>> }
>> if (!starts_with(line, "want ") ||
>
> I do not quite understand this hunk. What happens when this program
> is talking to an existing fetch-pack that requested "deepen"?
In this hunk, I actually just changed the one command name in upload-pack
service from "deepen" to "depth". I made this change because the name
"deepen" here is quite misleading, as it just means "depth". Of course,
I've changed the caller's sent pack from "deepen" to "depth" too (See below).
> @@ -317,7 +318,7 @@ static int find_common(struct fetch_pack_args *args,
> if (is_repository_shallow())
> write_shallow_commits(&req_buf, 1, NULL);
> if (args->depth > 0)
> - packet_buf_write(&req_buf, "deepen %d", args->depth);
> + packet_buf_write(&req_buf, "depth %d", args->depth);
> packet_buf_flush(&req_buf);
> state_len = req_buf.len;
If the user wants "deepen", he should send a "depth_deepen" signal as well as
"depth N". When the upload-pack service in this patch receive "depth_deepen"
and "depth N", it collects N more commits ahead of the shallow commit and send
back to the caller.
Thanks,
Dongcan
2015-03-14 13:35 GMT+08:00 Junio C Hamano :
> Dongcan Jiang writes:
>
>> This patch is just for discusstion. An option --deepen is added to
>> 'git fetch'. When it comes to '--deepen', git should fetch N more
>> commits ahead the local shallow commit, where N is indicated by
>> '--depth=N'. [1]
>>
>> e.g.
>>
>>> (upstream)
>>> ---o---o---o---A---B
>>>
>>> (you)
>>> A---B
>>
>> After excuting "git fetch --depth=1 --deepen", (you) get one more
>> tip and it becomes
>>
>>> (you)
>>> o---A---B
>>
>> '--deepen' is designed to be a boolean option in this patch, which
>> is a little different from [1]. It's designed in this way, because
>> it can reuse '--depth' in the program, and just costs one more bit
>> in some data structure, such as fetch_pack_args,
>> git_transport_options.
>>
>> Of course, as a patch for discussion, it remains a long way to go
>> before being complete.
>>
>> 1) Documents should be completed.
>> 2) More test cases, expecially corner cases, should be added.
>> 3) No need to get remote refs when it comes to '--deepen' option.
>> 4) Validity on options combination should be checked.
>> 5) smart-http protocol remains to be supported. [2]
>>
>> Besides, I still have one question:
>> What does (you) exactly mean in [1]? The local branch or the local
>> remote ref?
>
> As this operation is not about moving _any_ refs, whether local
> branches or remote-tracking branches, any ref that used to point at
> commit B before you executed "fetch --deepen" would point at the
> same commit after the command finishes.
>
> The "you" does not explicitly depict any ref, because the picture is
> meant to illustrate _everything_ the repository at the receiving end
> of "fetch" has. It used to have two commits, A and B (and the tree
> and blob objects necessary to complete these two commits). After
> deepening the history by one commit, it then will have commit A^ and
> its trees and blobs.
>
>> diff --git a/upload-pack.c b/upload-pack.c
>> index b531a32..8004f2f 100644
>> --- a/upload-pack.c
>> +++ b/upload-pack.c
>> @@ -31,6 +31,7 @@ static const char upload_pack_usage[] = "git upload-pack
>> [--strict] [--timeout=<
>>
>> static unsigned long oldest_have;
>>
>> +static int depth_deepen;
>> static int multi_ack;
>> static int no_done;
>> static int use_thin_pack, use_ofs_delta, use_include_tag;
>> @@ -563,11 +564,11 @@ static void receive_needs(void)
>> }
>> continue;
>> }
>> - if (starts_with(line, "deepen ")) {
>> +
Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen
Hi Duy,
Sorry for my late response.
> we need to make sure that upload-pack barf if some client sends both "deepen"
> and
> "depth".
Actually, in my current design, when client just wants "depth", it
sends "depth N";
when it want "deepen", it sends "depth N" as well as "depth_deepen".
For the latter
case, it tells upload-pack to collect objects for "deepen N".
Thus, I'm not so sure whether upload-pack needs to check their exclusion.
> I was about to suggest you use "deepen" for both --depth and
> --deepen: --depth sends "deepen NUM" while --deepen sends "deepen
> +NUM". The protocol as described in pack-protocol.txt may allow this
> extension.
This suggestion looks neat! However, I'm afraid it may involves quite
a bit changes
as you've mentioned, which would be better to take in next stage.
Thanks,
Dongcan
2015-03-14 18:56 GMT+08:00 Duy Nguyen :
> On Fri, Mar 13, 2015 at 8:04 PM, Dongcan Jiang
> wrote:
>> @@ -317,7 +318,7 @@ static int find_common(struct fetch_pack_args *args,
>> if (is_repository_shallow())
>> write_shallow_commits(&req_buf, 1, NULL);
>> if (args->depth > 0)
>> - packet_buf_write(&req_buf, "deepen %d", args->depth);
>> + packet_buf_write(&req_buf, "depth %d", args->depth);
>> packet_buf_flush(&req_buf);
>> state_len = req_buf.len;
>
> Junio already questioned about your replacing starts_with("deepen "..)
> with starts_with("depth "..), this is part of that question. If you
> run "make test" you should find some breakages, or we need to improve
> our test suite.
>
> I think you just need to keep the old "if (args->depth > 0) send
> "depth" unchanged and add two new statements that check
> args->depth_deepen and sends "depth ". BTW, I think "deepen-more" or
> "deepen-relative" would be better than "depth" (*), which is a noun.
> But that's a minor point at this stage.
>
> And because --depth and --deepen are mutually exclusive, you need to
> make sure that the user must not specify that. The "user" includes the
> "client" from the server perspective, we need to make sure that
> upload-pack barf if some client sends both "deepen" and "depth".
>
> (*) I was about to suggest you use "deepen" for both --depth and
> --deepen: --depth sends "deepen NUM" while --deepen sends "deepen
> +NUM". The protocol as described in pack-protocol.txt may allow this
> extension. Unfortunately the current implementation is too relaxed. We
> use strtol() to parse "NUM" and it would accept the leading "+"
> instead of barfing out. So old clients would mistakes --deepen as
> --depth, not good. And it even accepts base 8 and 16!! We should fix
> this.
> --
> Duy
--
江东灿（Dongcan Jiang）
Team of Search Engine & Web Mining
School of Electronic Engineering & Computer Science
Peking University, Beijing, 100871, P.R.China
--
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] [RFC] Add a new config. option for skipping merges in git-log
This patch adds a 'showmerges' config. option for git-log.
This option determines whether the log should contain merge
commits or not. In essence, if this option is set to false,
git-log will be run as 'git-log --no-merges'.
To force git-log to show merges even if 'log.showmerges' is
set, we use --include-merges command line option.
Signed-off-by: Koosha Khajehmoogahi
---
Documentation/config.txt | 3 +++
builtin/log.c| 9 +
revision.c | 2 ++
revision.h | 1 +
4 files changed, 15 insertions(+)
This is the third time I send this patch as it seems it didn't
get delivered even after one hour!
Please help me with this patch. It seems that my --include-merges
command-line option does not have have any effect on the behavior
of git-log. I don't know why the value of force_show_merges is
always 0!
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1530255..7775b8c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1735,6 +1735,9 @@ log.showroot::
Tools like linkgit:git-log[1] or linkgit:git-whatchanged[1], which
normally hide the root commit will now show it. True by default.
+log.showmerges::
+ If true, merges will be shown in the log list. True by default.
+
log.mailmap::
If true, makes linkgit:git-log[1], linkgit:git-show[1], and
linkgit:git-whatchanged[1] assume `--use-mailmap`.
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..867bcf2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -31,6 +31,7 @@ static const char *default_date_mode = NULL;
static int default_abbrev_commit;
static int default_show_root = 1;
+static int default_max_parents = -1;
static int decoration_style;
static int decoration_given;
static int use_mailmap_config;
@@ -108,6 +109,8 @@ static void cmd_log_init_defaults(struct rev_info *rev)
rev->diffopt.stat_graph_width = -1; /* respect statGraphWidth config */
rev->abbrev_commit = default_abbrev_commit;
rev->show_root_diff = default_show_root;
+ if (rev->force_show_merges == 0)
+ rev->max_parents = default_max_parents;
rev->subject_prefix = fmt_patch_subject_prefix;
DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);
@@ -390,6 +393,12 @@ static int git_log_config(const char *var, const char
*value, void *cb)
default_show_root = git_config_bool(var, value);
return 0;
}
+
+ if (!strcmp(var, "log.showmerges")) {
+ default_max_parents = git_config_bool(var, value) ? -1 : 1;
+ return 0;
+ }
+
if (skip_prefix(var, "color.decorate.", &slot_name))
return parse_decorate_color_config(var, slot_name, value);
if (!strcmp(var, "log.mailmap")) {
diff --git a/revision.c b/revision.c
index 66520c6..e7073b8 100644
--- a/revision.c
+++ b/revision.c
@@ -1804,6 +1804,8 @@ static int handle_revision_opt(struct rev_info *revs, int
argc, const char **arg
revs->min_parents = 2;
} else if (!strcmp(arg, "--no-merges")) {
revs->max_parents = 1;
+ } else if (!strcmp(arg, "--include-merges")) {
+ revs->force_show_merges = 1;
} else if (starts_with(arg, "--min-parents=")) {
revs->min_parents = atoi(arg+14);
} else if (starts_with(arg, "--no-min-parents")) {
diff --git a/revision.h b/revision.h
index 0ea8b4e..f496472 100644
--- a/revision.h
+++ b/revision.h
@@ -145,6 +145,7 @@ struct rev_info {
unsigned inttrack_linear:1,
track_first_time:1,
linear:1;
+ unsigned int force_show_merges:1;
enum date_mode date_mode;
--
1.9.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] diff-lib.c: adjust position of i-t-a entries in diff
Michael J Gruber writes: > Nguyễn Thái Ngọc Duy venit, vidit, dixit 16.03.2015 14:56: > >> The test "cache-tree invalidates i-t-a paths" is marked failure >> because I don't think removing "--cached" from "git diff" is the right >> fix. This test relies on "diff --cached" behavior but the behavior now >> has changed. We may need to revisit this test at some point in future. > > I can't quite follow that reasoning. If the test describes expected > behavior which the patch breaks, then we should not apply the patch. If > the patch changes behavior in an expected way, then we should change the > test to match. The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a paths after generating trees, 2012-12-16), which was a fix to an earlier bug where a cache-tree written out of an index with i-t-a entries had incorrect information and still claimed it is fully valid after write-tree rebuilt it. The test probably should add another path without i-t-a bit, run the same "diff --cached" with updated expectation before write-tre, and run the "diff --cached" again to make sure it produces a result that match the updated expectation. -- 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/RFC] fetch: git fetch --deepen
Hi Eric,
Sorry for my late response. Thank you for your suggestions! I will try
to use them in my next patch version.
Best Regards,
Dongcan
2015-03-14 3:42 GMT+08:00 Eric Sunshine :
> On Fri, Mar 13, 2015 at 9:04 AM, Dongcan Jiang
> wrote:
>> This patch is just for discusstion. An option --deepen is added to
>> 'git fetch'. When it comes to '--deepen', git should fetch N more
>> commits ahead the local shallow commit, where N is indicated by
>> '--depth=N'. [1]
>> [...]
>> Of course, as a patch for discussion, it remains a long way to go
>> before being complete.
>> [...]
>> Signed-off-by: Dongcan Jiang
>> ---
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index 655ee64..6f4adca 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -295,6 +295,7 @@ static int find_common(struct fetch_pack_args *args,
>> if (no_done)strbuf_addstr(&c, "
>> no-done");
>> if (use_sideband == 2) strbuf_addstr(&c, "
>> side-band-64k");
>> if (use_sideband == 1) strbuf_addstr(&c, "
>> side-band");
>> + if (args->depth_deepen) strbuf_addstr(&c, "
>> depth_deepen");
>
> For consistency, should this be "depth-deepen" rather than "depth_deepen"?
>
>> if (args->use_thin_pack) strbuf_addstr(&c, "
>> thin-pack");
>> if (args->no_progress) strbuf_addstr(&c, "
>> no-progress");
>> if (args->include_tag) strbuf_addstr(&c, "
>> include-tag");
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index d78f320..6738006 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -708,4 +708,15 @@ test_expect_success 'fetching a one-level ref works' '
>> )
>> '
>>
>> +test_expect_success 'fetching deepen' '
>> + git clone . deepen --depth=1 &&
>> + cd deepen &&
>
> When this tests ends, the working directory will still be 'deepen',
> which will likely break tests added after this one. If you wrap the
> 'cd' and following statements in a subshell via '(' and ')', then the
> 'cd' will affect the subshell but leave the parent shell's working
> directory alone, and thus not negatively impact subsequent tests.
>
>> + git fetch .. foo --depth=1
>> + git show foo
>> + test_must_fail git show foo~
>> + git fetch .. foo --depth=1 --deepen
>> + git show foo~
>> + test_must_fail git show foo~2
>
> Broken &&-chain throughout.
>
>> +'
>> +
>> test_done
--
江东灿（Dongcan Jiang）
Team of Search Engine & Web Mining
School of Electronic Engineering & Computer Science
Peking University, Beijing, 100871, P.R.China
--
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] git submodule purge
Patrick Steinhardt writes: > I think there should be a command that is able to remove those > dangling repositories if the following conditions are met: > > - the submodule should not be initialized > > - the submodule should not have an entry in .gitmodules in the > currently checked out revision > > - the submodule should not contain any commits that are not > upstream > > - the submodule should not contain other submodules that do not > meet those conditions I do not have a strong opinion on whether it is a good idea to make it possible to remove the .git/modules/*, but should it be a separate subcommand, or should it be an option to the 'deinit' subcommand? Also, how would you apply the safety to a repository without "upstream", i.e. the authoritative 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
[PATCH] [RFC] Add a new config. option for skipping merges in git-log
This patch adds a 'showmerges' config. option for git-log.
This option determines whether the log should contain merge
commits or not. In essence, if this option is set to false,
git-log will be run as 'git-log --no-merges'.
To force git-log to show merges even if 'log.showmerges' is
set, we use --include-merges command line option.
Signed-off-by: Koosha Khajehmoogahi
---
Documentation/config.txt | 3 +++
builtin/log.c| 9 +
revision.c | 2 ++
revision.h | 1 +
4 files changed, 15 insertions(+)
This is the second time I send this patch as it seems it didn't
get delivered even after one hour!
Please help me with this patch. It seems that my --include-merges
command-line option does not have have any effect on the behavior
of git-log. I don't know why the value of force_show_merges is
always 0!
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1530255..7775b8c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1735,6 +1735,9 @@ log.showroot::
Tools like linkgit:git-log[1] or linkgit:git-whatchanged[1], which
normally hide the root commit will now show it. True by default.
+log.showmerges::
+ If true, merges will be shown in the log list. True by default.
+
log.mailmap::
If true, makes linkgit:git-log[1], linkgit:git-show[1], and
linkgit:git-whatchanged[1] assume `--use-mailmap`.
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..867bcf2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -31,6 +31,7 @@ static const char *default_date_mode = NULL;
static int default_abbrev_commit;
static int default_show_root = 1;
+static int default_max_parents = -1;
static int decoration_style;
static int decoration_given;
static int use_mailmap_config;
@@ -108,6 +109,8 @@ static void cmd_log_init_defaults(struct rev_info *rev)
rev->diffopt.stat_graph_width = -1; /* respect statGraphWidth config */
rev->abbrev_commit = default_abbrev_commit;
rev->show_root_diff = default_show_root;
+ if (rev->force_show_merges == 0)
+ rev->max_parents = default_max_parents;
rev->subject_prefix = fmt_patch_subject_prefix;
DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);
@@ -390,6 +393,12 @@ static int git_log_config(const char *var, const char
*value, void *cb)
default_show_root = git_config_bool(var, value);
return 0;
}
+
+ if (!strcmp(var, "log.showmerges")) {
+ default_max_parents = git_config_bool(var, value) ? -1 : 1;
+ return 0;
+ }
+
if (skip_prefix(var, "color.decorate.", &slot_name))
return parse_decorate_color_config(var, slot_name, value);
if (!strcmp(var, "log.mailmap")) {
diff --git a/revision.c b/revision.c
index 66520c6..e7073b8 100644
--- a/revision.c
+++ b/revision.c
@@ -1804,6 +1804,8 @@ static int handle_revision_opt(struct rev_info *revs, int
argc, const char **arg
revs->min_parents = 2;
} else if (!strcmp(arg, "--no-merges")) {
revs->max_parents = 1;
+ } else if (!strcmp(arg, "--include-merges")) {
+ revs->force_show_merges = 1;
} else if (starts_with(arg, "--min-parents=")) {
revs->min_parents = atoi(arg+14);
} else if (starts_with(arg, "--no-min-parents")) {
diff --git a/revision.h b/revision.h
index 0ea8b4e..f496472 100644
--- a/revision.h
+++ b/revision.h
@@ -145,6 +145,7 @@ struct rev_info {
unsigned inttrack_linear:1,
track_first_time:1,
linear:1;
+ unsigned int force_show_merges:1;
enum date_mode date_mode;
--
1.9.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] diff-lib.c: adjust position of i-t-a entries in diff
Nguyễn Thái Ngọc Duy venit, vidit, dixit 16.03.2015 14:56:
> Entries added by "git add -N" are reminder for the user so that they
> don't forget to add them before committing. These entries appear in
> the index even though they are not real. Their presence in the index
> leads to a confusing "git status" like this:
>
> On branch master
> Changes to be committed:
> new file: foo
>
> Changes not staged for commit:
> modified: foo
>
> If you do a "git commit", "foo" will not be included even though
> "status" reports it as "to be committed". This patch changes the
> output to become
>
> On branch master
> Changes not staged for commit:
> new file: foo
>
> no changes added to commit
>
> The two hunks in diff-lib.c adjust "diff-index" and "diff-files" so
> that i-t-a entries appear as new files in diff-files and nothing in
> diff-index.
>
> Due to this change, diff-files may start to report "new files" for the
> first time. "add -u" needs to be told about this or it will die in
> denial, screaming "new files can't exist! Reality is wrong." Luckily,
> it's the only one among run_diff_files() callers that needs fixing.
>
> The test "cache-tree invalidates i-t-a paths" is marked failure
> because I don't think removing "--cached" from "git diff" is the right
> fix. This test relies on "diff --cached" behavior but the behavior now
> has changed. We may need to revisit this test at some point in future.
I can't quite follow that reasoning. If the test describes expected
behavior which the patch breaks, then we should not apply the patch. If
the patch changes behavior in an expected way, then we should change the
test to match.
> Helped-by: Michael J Gruber
> Helped-by: Junio C Hamano
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> builtin/add.c | 1 +
> diff-lib.c | 12
> t/t2203-add-intent.sh | 21 ++---
> t/t4011-diff-symlink.sh | 10 ++
> 4 files changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 3390933..ee370b0 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -63,6 +63,7 @@ static void update_callback(struct diff_queue_struct *q,
> switch (fix_unmerged_status(p, data)) {
> default:
> die(_("unexpected diff status %c"), p->status);
> + case DIFF_STATUS_ADDED:
> case DIFF_STATUS_MODIFIED:
> case DIFF_STATUS_TYPE_CHANGED:
> if (add_file_to_index(&the_index, path, data->flags)) {
> diff --git a/diff-lib.c b/diff-lib.c
> index a85c497..714501a 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -212,6 +212,11 @@ int run_diff_files(struct rev_info *revs, unsigned int
> option)
> ce->sha1,
> !is_null_sha1(ce->sha1),
> ce->name, 0);
> continue;
> + } else if (ce->ce_flags & CE_INTENT_TO_ADD) {
> + diff_addremove(&revs->diffopt, '+', ce->ce_mode,
> +EMPTY_BLOB_SHA1_BIN, 0,
> +ce->name, 0);
> + continue;
> }
>
> changed = match_stat_with_submodule(&revs->diffopt, ce,
> &st,
> @@ -376,6 +381,13 @@ static void do_oneway_diff(struct unpack_trees_options
> *o,
> struct rev_info *revs = o->unpack_data;
> int match_missing, cached;
>
> + /* i-t-a entries do not actually exist in the index */
> + if (idx && (idx->ce_flags & CE_INTENT_TO_ADD)) {
> + idx = NULL;
> + if (!tree)
> + return; /* nothing to diff.. */
> + }
> +
> /* if the entry is not checked out, don't examine work tree */
> cached = o->index_only ||
> (idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 2a4a749..42827b8 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -5,10 +5,24 @@ test_description='Intent to add'
> . ./test-lib.sh
>
> test_expect_success 'intent to add' '
> + test_commit 1 &&
> + git rm 1.t &&
> + echo hello >1.t &&
> echo hello >file &&
> echo hello >elif &&
> git add -N file &&
> - git add elif
> + git add elif &&
> + git add -N 1.t
> +'
> +
> +test_expect_success 'git status' '
> + git status --porcelain | grep -v actual >actual &&
> + cat >expect <<-\EOF &&
> + DA 1.t
> + A elif
> + A file
> + EOF
> + test_cmp expect actual
> '
>
> test_expect_success 'check result of "add -N"' '
> @@ -43,7 +57,8 @@ test_expect_success 'i-t-a entry is simply ignored' '
> git add -N nitfol &&
> git commit -m second &&
> test $(git ls-tree HEA
[PATCH 2/2] Add revision range support on "-" and "@{-1}"
Currently it is not be possible to do something like "git checkout
master && git checkout next && git log -.." to see what master has on
top of master.
Allows use of the revision range such as ..- or -.. to see
what HEAD has on top of or vice versa, respectively.
Also allows use of symmetric differences such as ...- and -...
This is written on top of Junio's "Just For Fun" patch ($Gmane/265260).
Signed-off-by: Kenny Lee Sin Cheong
---
revision.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index 7778bbd..a79b443 100644
--- a/revision.c
+++ b/revision.c
@@ -1490,6 +1490,7 @@ int handle_revision_arg(const char *arg_, struct rev_info
*revs, int flags, unsi
int symmetric = *next == '.';
unsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);
static const char head_by_default[] = "HEAD";
+ static const char prev_rev[] = "@{-1}";
unsigned int a_flags;
*dotdot = 0;
@@ -1499,6 +1500,13 @@ int handle_revision_arg(const char *arg_, struct
rev_info *revs, int flags, unsi
next = head_by_default;
if (dotdot == arg)
this = head_by_default;
+ /* Allows -.. and ..- */
+ if (!strcmp(this, "-")) {
+ this = prev_rev;
+ }
+ if (!strcmp(next, "-")) {
+ next = prev_rev;
+ }
if (this == head_by_default && next == head_by_default &&
!symmetric) {
/*
@@ -2198,7 +2206,7 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
read_from_stdin = 0;
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
- if (arg[0] == '-' && arg[1]) {
+ if (arg[0] == '-' && !strstr(arg, "..")) {
int opts;
opts = handle_revision_pseudo_opt(submodule,
@@ -2220,6 +2228,7 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
continue;
}
+
opts = handle_revision_opt(revs, argc - i, argv + i,
&left, argv);
if (opts > 0) {
i += opts - 1;
@@ -2229,7 +2238,10 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
exit(128);
continue;
}
-
+ if (strstr(arg, "..")) {
+ handle_revision_arg(arg, revs, flags, revarg_opt);
+ continue;
+ }
if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
int j;
--
2.3.2.225.gebdc58a
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] "-" and "@{-1}" on various programs
From: Junio C Hamano
JFF stands for just for fun.
This is not meant to give out a model answer and is known to be
incomplete, but I was wondering if it would be a better direction to
allow "-" as a stand-in for "@{-1}" everywhere we allow a branch
name, losing workarounds at the surface level we have for checkout,
merge and revert.
The first three paths are to remove the surface workarounds that
become unnecessary. The one in sha1_name.c is the central change.
The change in revision.c is to allow a single "-" to be recognized
as a potential revision name (without this change, what begins with
"-" is either an option or an unknown option).
So you could do things like "git reset - $path" but also things like
"git log -" after switching out of a branch.
What does not work are what needs further tweaking in revision.c
parser. "git checkout master && git checkout next && git log -.."
should show what next has on top of master but I didn't touch the
range notation so it does not work, for example.
builtin/checkout.c | 3 ---
builtin/merge.c| 3 +--
builtin/revert.c | 2 --
revision.c | 2 +-
sha1_name.c| 57 +-
5 files changed, 37 insertions(+), 30 deletions(-)
Signed-off-by: Kenny Lee Sin Cheong
---
builtin/checkout.c | 3 ---
builtin/merge.c| 3 +--
builtin/revert.c | 2 --
revision.c | 2 +-
sha1_name.c| 57 +-
5 files changed, 37 insertions(+), 30 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3e141fc..f86bad7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -951,9 +951,6 @@ static int parse_branchname_arg(int argc, const char **argv,
else if (dash_dash_pos >= 2)
die(_("only one reference expected, %d given."), dash_dash_pos);
- if (!strcmp(arg, "-"))
- arg = "@{-1}";
-
if (get_sha1_mb(arg, rev)) {
/*
* Either case (3) or (4), with not being
diff --git a/builtin/merge.c b/builtin/merge.c
index 3b0f8f9..03b260f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1164,8 +1164,7 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
argc = setup_with_upstream(&argv);
else
die(_("No commit specified and
merge.defaultToUpstream not set."));
- } else if (argc == 1 && !strcmp(argv[0], "-"))
- argv[0] = "@{-1}";
+ }
}
if (!argc)
usage_with_options(builtin_merge_usage,
diff --git a/builtin/revert.c b/builtin/revert.c
index 56a2c36..dc98b4e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -170,8 +170,6 @@ static void parse_args(int argc, const char **argv, struct
replay_opts *opts)
opts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
if (argc < 2)
usage_with_options(usage_str, options);
- if (!strcmp(argv[1], "-"))
- argv[1] = "@{-1}";
memset(&s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.assume_dashdash = 1;
argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
diff --git a/revision.c b/revision.c
index 66520c6..7778bbd 100644
--- a/revision.c
+++ b/revision.c
@@ -2198,7 +2198,7 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
read_from_stdin = 0;
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
- if (*arg == '-') {
+ if (arg[0] == '-' && arg[1]) {
int opts;
opts = handle_revision_pseudo_opt(submodule,
diff --git a/sha1_name.c b/sha1_name.c
index 95f9f8f..7a621ba 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -483,6 +483,8 @@ static int get_sha1_basic(const char *str, int len,
unsigned char *sha1,
break;
}
}
+ } else if (len == 1 && str[0] == '-') {
+ nth_prior = 1;
}
/* Accept only unambiguous ref paths. */
@@ -491,13 +493,16 @@ static int get_sha1_basic(const char *str, int len,
unsigned char *sha1,
if (nth_prior) {
struct strbuf buf = STRBUF_INIT;
- int detached;
+ int status;
if (interpret_nth_prior_checkout(str, len, &buf) > 0) {
- detached = (buf.len == 40 && !get_sha1_hex(buf.buf,
sha1));
+ if (get_sha1(buf.buf, sha1))
+ /* bad---the previous branch no longer exists?
*/
+ status = -1;
+ else
+ status = 0; /* detached */
strbuf_release(&buf);
- if (detached)
-
[PATCH/RFC 0/2][GSoC] revision.c: Allow "-" as stand-in for "@{-1}" everywhere a branch is allowed
This is an attempt at a microproject for GSoC
An attempt to add revision range support to Junio's JFF patch sent a few days
ago. The first patch is the a copy of the one he posted.
I was wondering if it was a good idea to add support for commands like
"..-". Files that starts with "-" requires "--" or a "./" format but what
if we have a file named "next..-" and call "git log next..-" ?
Junio C Hamano (1):
"-" and "@{-1}" on various programs
Kenny Lee Sin Cheong (1):
Add revision range support on "-" and "@{-1}"
builtin/checkout.c | 3 ---
builtin/merge.c| 3 +--
builtin/revert.c | 2 --
revision.c | 16 +--
sha1_name.c| 57 +-
5 files changed, 50 insertions(+), 31 deletions(-)
--
2.3.2.225.gebdc58a
--
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] [RFC] Add a new config. option for skipping merges in git-log
This patch adds a 'showmerges' config. option for git-log.
This option determines whether the log should contain merge
commits or not. In essence, if this option is set to false,
git-log will be run as 'git-log --no-merges'.
To force git-log to show merges even if 'log.showmerges' is
set, we use --include-merges command line option.
Signed-off-by: Koosha Khajehmoogahi
---
Documentation/config.txt | 3 +++
builtin/log.c| 9 +
revision.c | 2 ++
revision.h | 1 +
4 files changed, 15 insertions(+)
Please help me with this patch. It seems that my --include-merges
command-line option does not have have any effect on the behavior
of git-log. I don't know why the value of force_show_merges is
always 0!
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1530255..7775b8c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1735,6 +1735,9 @@ log.showroot::
Tools like linkgit:git-log[1] or linkgit:git-whatchanged[1], which
normally hide the root commit will now show it. True by default.
+log.showmerges::
+ If true, merges will be shown in the log list. True by default.
+
log.mailmap::
If true, makes linkgit:git-log[1], linkgit:git-show[1], and
linkgit:git-whatchanged[1] assume `--use-mailmap`.
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..867bcf2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -31,6 +31,7 @@ static const char *default_date_mode = NULL;
static int default_abbrev_commit;
static int default_show_root = 1;
+static int default_max_parents = -1;
static int decoration_style;
static int decoration_given;
static int use_mailmap_config;
@@ -108,6 +109,8 @@ static void cmd_log_init_defaults(struct rev_info *rev)
rev->diffopt.stat_graph_width = -1; /* respect statGraphWidth config */
rev->abbrev_commit = default_abbrev_commit;
rev->show_root_diff = default_show_root;
+ if (rev->force_show_merges == 0)
+ rev->max_parents = default_max_parents;
rev->subject_prefix = fmt_patch_subject_prefix;
DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);
@@ -390,6 +393,12 @@ static int git_log_config(const char *var, const char
*value, void *cb)
default_show_root = git_config_bool(var, value);
return 0;
}
+
+ if (!strcmp(var, "log.showmerges")) {
+ default_max_parents = git_config_bool(var, value) ? -1 : 1;
+ return 0;
+ }
+
if (skip_prefix(var, "color.decorate.", &slot_name))
return parse_decorate_color_config(var, slot_name, value);
if (!strcmp(var, "log.mailmap")) {
diff --git a/revision.c b/revision.c
index 66520c6..e7073b8 100644
--- a/revision.c
+++ b/revision.c
@@ -1804,6 +1804,8 @@ static int handle_revision_opt(struct rev_info *revs, int
argc, const char **arg
revs->min_parents = 2;
} else if (!strcmp(arg, "--no-merges")) {
revs->max_parents = 1;
+ } else if (!strcmp(arg, "--include-merges")) {
+ revs->force_show_merges = 1;
} else if (starts_with(arg, "--min-parents=")) {
revs->min_parents = atoi(arg+14);
} else if (starts_with(arg, "--no-min-parents")) {
diff --git a/revision.h b/revision.h
index 0ea8b4e..f496472 100644
--- a/revision.h
+++ b/revision.h
@@ -145,6 +145,7 @@ struct rev_info {
unsigned inttrack_linear:1,
track_first_time:1,
linear:1;
+ unsigned int force_show_merges:1;
enum date_mode date_mode;
--
1.9.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
[PATCH] refs.c: get_ref_cache: use a bucket hash
get_ref_cache used a linear list, which obviously is O(n^2).
Use a fixed bucket hash which just takes a factor of 10
(~ 317^2) out of the n^2 - which is enough.
Signed-off-by: Andreas Krey
---
This brings 'git clean -ndx' times down from 17 minutes
to 11 seconds on one of our workspaces (which accumulated
a lot of ignored directories). Actuallly using adaptive
hashing or other structures seems overkill.
refs.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index e23542b..8198d9e 100644
--- a/refs.c
+++ b/refs.c
@@ -982,6 +982,8 @@ struct packed_ref_cache {
struct stat_validity validity;
};
+#define REF_CACHE_HASH 317
+
/*
* Future: need to be in "struct repository"
* when doing a full libification.
@@ -996,7 +998,7 @@ static struct ref_cache {
* is initialized correctly.
*/
char name[1];
-} ref_cache, *submodule_ref_caches;
+} ref_cache, *submodule_ref_caches[REF_CACHE_HASH];
/* Lock used for the main packed-refs file: */
static struct lock_file packlock;
@@ -1065,18 +1067,19 @@ static struct ref_cache *create_ref_cache(const char
*submodule)
*/
static struct ref_cache *get_ref_cache(const char *submodule)
{
- struct ref_cache *refs;
+ struct ref_cache *refs, **bucketp;
+ bucketp = submodule_ref_caches + strhash(submodule) % REF_CACHE_HASH;
if (!submodule || !*submodule)
return &ref_cache;
- for (refs = submodule_ref_caches; refs; refs = refs->next)
+ for (refs = *bucketp; refs; refs = refs->next)
if (!strcmp(submodule, refs->name))
return refs;
refs = create_ref_cache(submodule);
- refs->next = submodule_ref_caches;
- submodule_ref_caches = refs;
+ refs->next = *bucketp;
+ *bucketp = refs;
return refs;
}
--
2.3.2.223.g7a9409c
--
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-svn: Fetch svn branches only and have git recognize them as branches?
Just to close the loop on this, I wasn't able to get git-svn to deal with this remote repository the way I wanted, so what I did is write some perl scripts to mirror the remote subversion repository locally and then use git-svn on that local "fixed" svn mirror. The hard part was mirroring the svn repo locally, primarily because the remote subversion server was denying access to the root. Also complicating matters was the dump files weren't completely loading properly. In case anyone else is needing to do this, I've published my perl script code that overcame all these issues. Here it is: https://github.com/ucidentity/failsafe-svn-sync -- 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
[RFC] git submodule purge
Hi, This proposal is just for discussion. If there is any interest I will implement the feature and send some patches. Currently it is hard to properly remove submodules. That is when a submodule is deinitialized and removed from a repository the directory '.git/modules/' will still be present and there is no way to remove it despite manually calling `rm` on it. I think there should be a command that is able to remove those dangling repositories if the following conditions are met: - the submodule should not be initialized - the submodule should not have an entry in .gitmodules in the currently checked out revision - the submodule should not contain any commits that are not upstream - the submodule should not contain other submodules that do not meet those conditions This would ensure that it is hard to loose any commits that may be of interest. In the case that the user knows what he is doing we may provide a '--force' switch to override those checks. What is problematic, though, is when there are multiple branches under active development where one branch contains a submodule and another one does not. Given the checks listed above, though, an accidentally removed submodule repository should not prove problematic as it should be possible to easily re-clone it. It might just be cumbersome if the user accidentally removes a submodule and has to re-initialize it after switching branches. Regarding behavior of the command I thought about something like `git submodule purge (||-a)` where 'SM_NAME' would remove the given submodule, 'DIRECTORY' would remove all submodules under a certain directory and '-a' would remove all submodules that are currently inactive. It might be useful to provide a '--dry-run' switch that lists all submodules that would be removed without actually removing them. Some questions that arise here: - should those actions be recursive? E.g. if a submodule contains submodules itself, should those be deleted (after the conditions are met), as well? - should the user be asked for consent for every submodule that is about to be deleted or do we assume that he knows what he is doing? Some feedback on this would be appreciated. Regards Patrick signature.asc Description: PGP signature
[PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
Entries added by "git add -N" are reminder for the user so that they
don't forget to add them before committing. These entries appear in
the index even though they are not real. Their presence in the index
leads to a confusing "git status" like this:
On branch master
Changes to be committed:
new file: foo
Changes not staged for commit:
modified: foo
If you do a "git commit", "foo" will not be included even though
"status" reports it as "to be committed". This patch changes the
output to become
On branch master
Changes not staged for commit:
new file: foo
no changes added to commit
The two hunks in diff-lib.c adjust "diff-index" and "diff-files" so
that i-t-a entries appear as new files in diff-files and nothing in
diff-index.
Due to this change, diff-files may start to report "new files" for the
first time. "add -u" needs to be told about this or it will die in
denial, screaming "new files can't exist! Reality is wrong." Luckily,
it's the only one among run_diff_files() callers that needs fixing.
The test "cache-tree invalidates i-t-a paths" is marked failure
because I don't think removing "--cached" from "git diff" is the right
fix. This test relies on "diff --cached" behavior but the behavior now
has changed. We may need to revisit this test at some point in future.
Helped-by: Michael J Gruber
Helped-by: Junio C Hamano
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/add.c | 1 +
diff-lib.c | 12
t/t2203-add-intent.sh | 21 ++---
t/t4011-diff-symlink.sh | 10 ++
4 files changed, 37 insertions(+), 7 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 3390933..ee370b0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -63,6 +63,7 @@ static void update_callback(struct diff_queue_struct *q,
switch (fix_unmerged_status(p, data)) {
default:
die(_("unexpected diff status %c"), p->status);
+ case DIFF_STATUS_ADDED:
case DIFF_STATUS_MODIFIED:
case DIFF_STATUS_TYPE_CHANGED:
if (add_file_to_index(&the_index, path, data->flags)) {
diff --git a/diff-lib.c b/diff-lib.c
index a85c497..714501a 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -212,6 +212,11 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
ce->sha1,
!is_null_sha1(ce->sha1),
ce->name, 0);
continue;
+ } else if (ce->ce_flags & CE_INTENT_TO_ADD) {
+ diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+ EMPTY_BLOB_SHA1_BIN, 0,
+ ce->name, 0);
+ continue;
}
changed = match_stat_with_submodule(&revs->diffopt, ce,
&st,
@@ -376,6 +381,13 @@ static void do_oneway_diff(struct unpack_trees_options *o,
struct rev_info *revs = o->unpack_data;
int match_missing, cached;
+ /* i-t-a entries do not actually exist in the index */
+ if (idx && (idx->ce_flags & CE_INTENT_TO_ADD)) {
+ idx = NULL;
+ if (!tree)
+ return; /* nothing to diff.. */
+ }
+
/* if the entry is not checked out, don't examine work tree */
cached = o->index_only ||
(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2a4a749..42827b8 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,10 +5,24 @@ test_description='Intent to add'
. ./test-lib.sh
test_expect_success 'intent to add' '
+ test_commit 1 &&
+ git rm 1.t &&
+ echo hello >1.t &&
echo hello >file &&
echo hello >elif &&
git add -N file &&
- git add elif
+ git add elif &&
+ git add -N 1.t
+'
+
+test_expect_success 'git status' '
+ git status --porcelain | grep -v actual >actual &&
+ cat >expect <<-\EOF &&
+ DA 1.t
+ A elif
+A file
+ EOF
+ test_cmp expect actual
'
test_expect_success 'check result of "add -N"' '
@@ -43,7 +57,8 @@ test_expect_success 'i-t-a entry is simply ignored' '
git add -N nitfol &&
git commit -m second &&
test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
- test $(git diff --name-only HEAD -- nitfol | wc -l) = 1
+ test $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&
+ test $(git diff --name-only -- nitfol | wc -l) = 1
'
test_expect_success 'can commit with an unrelated i-t-a entry in index' '
@@ -62,7 +77,7 @@ test_expect_success 'can "commit -a" with an i-t-a entry' '
git commit -a -m all
'
-test_expect_success 'cache-tree in
Re: Bug in git-verify-pack or in its documentation?
On Mon, Mar 16, 2015 at 05:13:25PM +0700, Duy Nguyen wrote: > On Mon, Mar 16, 2015 at 3:05 PM, Mike Hommey wrote: > > Hi, > > > > git-verify-pack's man page says the following about --stat-only: > > > >Do not verify the pack contents; only show the histogram of delta > >chain length. With --verbose, list of objects is also shown. > > > > However, there is no difference of output between --verbose and > > --verbose --stat-only (and in fact, looking at the code, only one of > > them has an effect, --stat-only having precedence). > > There is. very-pack --verbose would show a lot of " > <"random" numbers>" lines before the histogram while --stat-only (with > or without --verbose) would only show the histogram. Err, I meant between --stat-only and --verbose --stat-only. Mike -- 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 1/2] reset: enable '-' short-hand for previous branch
Thank you all for your reviews and feedback. I will try and submit
this patch by taking my time to think about the various solutions and
coming up with the best one. I will also try and see if I can assist
Junio with his JFF patch.
I have learned a lot during the process of implementing this micro
project and believe that if I get selected for a gsoc under git, it
will help me become a better developer overall.
I have gone over the ideas page and I am interested in the "git bisect
--first-parent" and "git bisect fixed/unfixed" projects. I am looking
the source code at git-bisect.sh and will try and come up with a
proposal for review by the git community.
Thank you all for you time and patience.
Regards,
Sudhanshu
On Sat, Mar 14, 2015 at 2:18 AM, Eric Sunshine wrote:
> On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar
> wrote:
>> git reset '-' will reset to the previous branch. To reset a file named
>> "-" use either "git reset ./-" or "git reset -- -".
>>
>> Change error message to treat single "-" as an ambigous revision or
>> path rather than a bad flag.
>>
>> Helped-by: Junio C Hamano
>> Helped-by: Eric Sunshine
>> Helped-by: Matthieu Moy
>> Signed-off-by: Sudhanshu Shekhar
>> ---
>> I have changed the logic to ensure argv[0] isn't changed at any point.
>> Creating a modified_argv0 variable let's me do that.
>>
>> I couldn't think of any other way to achieve this, apart from changing things
>> directly in the sha1_name.c file (like Junio's changes). Please let me know
>> if some further changes are needed.
>>
>> diff --git a/builtin/reset.c b/builtin/reset.c
>> index 4c08ddc..bc50e14 100644
>> --- a/builtin/reset.c
>> +++ b/builtin/reset.c
>> @@ -192,6 +192,7 @@ static void parse_args(struct pathspec *pathspec,
>> {
>> const char *rev = "HEAD";
>> unsigned char unused[20];
>> + const char *modified_argv0 = argv[0];
>
> This variable is only needed inside the 'if (argv[0])' block below, so
> its declaration should be moved there. Also the initialization to
> argv[0] is wasted since the assignment is overwritten below.
>
> The variable name itself could be better. Unlike a name such as
> 'orig_arg0', "modified" doesn't tell us much. Modified how? Modified
> to be what? Consideration where and how the variable is used, we can
> see that it will be holding a value that _might_ be a "rev". This
> suggests a better name such as 'maybe_rev' or something similar.
>
>> /*
>> * Possible arguments are:
>> *
>> @@ -205,10 +206,17 @@ static void parse_args(struct pathspec *pathspec,
>> */
>>
>> if (argv[0]) {
>> + if (!strcmp(argv[0], "-")) {
>> + modified_argv0 = "@{-1}";
>> + }
>> + else {
>
> Style: cuddle the 'else' and braces: "} else {"
>
>> + modified_argv0 = argv[0];
>> + }
>
> The unnecessary braces make this harder to read than it could be since
> it is so spread out vertically. Dropping the braces would help. The
> ternary operator ?: might improve readability (though it might also
> make it worse).
>
>> if (!strcmp(argv[0], "--")) {
>> argv++; /* reset to HEAD, possibly with paths */
>> } else if (argv[1] && !strcmp(argv[1], "--")) {
>> - rev = argv[0];
>> + rev = modified_argv0;
>> argv += 2;
>> }
>> /*
>> @@ -216,14 +224,15 @@ static void parse_args(struct pathspec *pathspec,
>> * has to be unambiguous. If there is a single argument, it
>> * can not be a tree
>> */
>> - else if ((!argv[1] && !get_sha1_committish(argv[0], unused))
>> ||
>> -(argv[1] && !get_sha1_treeish(argv[0], unused))) {
>> + else if ((!argv[1] && !get_sha1_committish(modified_argv0,
>> unused)) ||
>> +(argv[1] && !get_sha1_treeish(modified_argv0,
>> unused))) {
>> /*
>> * Ok, argv[0] looks like a commit/tree; it should
>> not
>> * be a filename.
>> */
>> verify_non_filename(prefix, argv[0]);
>> - rev = *argv++;
>> + rev = modified_argv0;
>> + argv++;
>
> Good. This is much better than the previous rounds, and is the sort of
> solution I had hoped to see when prodding you in previous reviews to
> avoid argv[] kludges. Unlike the previous band-aid approach, this
> demonstrates that you took the time to understand the overall logic
> flow rather than merely plopping in a "quick fix".
>
>> } else {
>> /* Otherwise we treat this as a filename */
>> verify_filename(prefix, argv[0], 1);
>> diff --git a/setup.c b/setup.c
>> index 979b13
Re: Bug in git-verify-pack or in its documentation?
On Mon, Mar 16, 2015 at 3:05 PM, Mike Hommey wrote: > Hi, > > git-verify-pack's man page says the following about --stat-only: > >Do not verify the pack contents; only show the histogram of delta >chain length. With --verbose, list of objects is also shown. > > However, there is no difference of output between --verbose and > --verbose --stat-only (and in fact, looking at the code, only one of > them has an effect, --stat-only having precedence). There is. very-pack --verbose would show a lot of " <"random" numbers>" lines before the histogram while --stat-only (with or without --verbose) would only show the histogram. > The text above also implies that this should only display the stats > without doing any sha1 validation, but afaict from a quick glance at > the index-pack code, they are always performed. > > Is it an implementation problem or a documentation problem? I think code and document start to divert from 3de89c9 (verify-pack: use index-pack --verify - 2011-06-03). The conversion to using index-pack implies heavier verification anyway (all objects must be hashed, no way around it), so I'd say it's documentation problem. The other way would be revert some patches in verify-pack.c and add more maintenance burden for this command. -- 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: What is XDG_CONFIG_HOME for exactly?
> On Sun, Mar 15, 2015 at 6:37 PM, Robert Dailey
> wrote:
>> My understanding is that git reads the priority of configuration as follows:
>>
>> 1. /.git/config
>> 2. $HOME/.gitconfig
>> 3. $XDG_CONFIG_HOME/git/config
$HOME/.gitconfig is the traditional Unix location for config files. It
was the only per-user config files in early versions of Git.
$XDG_CONFIG_HOME/git/config is the location following freedesktop's XDG
standard. It has several advantages (you can version and/or share
individual $XDG_CONFIG_HOME/git/ as a whole directory, while it's much
harder to version all ~/.git* files individually for example).
You typically want to use either one or the other.
>> 4. system level git config (not sure exactly where this is; not
>> relevant to me on Windows)
This is relevant if you use a shared machine and your sysadmin wants to
have the same configuration for all users.
> 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
Robert Dailey writes:
> As a follow-up, I tested the following in my .bashrc:
>
>
> # Utilize different GIT settings based on platform
> if [[ $OSTYPE == 'msys' || $OSTYPE == 'cygwin' ]]; then
> echo 'Using WINDOWS specific git settings'
> export XDG_CONFIG_HOME=.config-windows
You need an absolute directory here:
export XDG_CONFIG_HOME=$HOME/.config-windows
Then, I'd suggest that $XDG_CONFIG_HOME/git/config contains a
[include]
path = ../../.config-shared/git/config
so that you can factor out common portions of your config file into
.config-shared/git/config and keep machine-specific portions in
~/.config-{windows,linux}.
--
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
Re: Promoting Git developers
Christian Couder writes: > On Sun, Mar 15, 2015 at 11:43 PM, Randall S. Becker > wrote: >>> On March 15, 2015 6:19 PM Christian Couder wrote: >> >>> Just one suggestion on the name and half a comment. >>> >>> How would "Git Review" (or "Git Monthly Review", or replace your favourite >>> "how-often-per-period-ly" in its name) sound? I meant it to sound similar >> to >>> academic journals that summarize and review contemporary works in the >> field >>> and keeps your original "pun" about our culture around "patch reviews". > > I would be ok for that but there is already this Gerrit related command: > > http://www.mediawiki.org/wiki/Gerrit/git-review > > Maybe I can just use "Git Rev", but it doesn't tell that it is about news? > >> If I may humbly offer the suggestion that "Git Blame" would be a far more >> appropriate pun as a name :) > > You don't want me to steal Junio's blog title: > > http://git-blame.blogspot.fr/ > > don't you? "Git Annotate"? -- David Kastrup -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Promoting Git developers
On Sun, Mar 15, 2015 at 11:43 PM, Randall S. Becker wrote: >> On March 15, 2015 6:19 PM Christian Couder wrote: > >> Just one suggestion on the name and half a comment. >> >> How would "Git Review" (or "Git Monthly Review", or replace your favourite >> "how-often-per-period-ly" in its name) sound? I meant it to sound similar > to >> academic journals that summarize and review contemporary works in the > field >> and keeps your original "pun" about our culture around "patch reviews". I would be ok for that but there is already this Gerrit related command: http://www.mediawiki.org/wiki/Gerrit/git-review Maybe I can just use "Git Rev", but it doesn't tell that it is about news? > If I may humbly offer the suggestion that "Git Blame" would be a far more > appropriate pun as a name :) You don't want me to steal Junio's blog title: http://git-blame.blogspot.fr/ don't you? -- 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] show-branch: show all local heads when only giving one rev along --topics
"git show-branch --topics ..." displays ancestry graph, only
considering commits that are in all given revs, except the first one.
"git show-branch" displays ancestry graph for all local branches.
Unfortunately, "git show-branch --topics " only prints out the rev
info for the given rev, and nothing else, e.g.:
$ git show-branch --topics origin/master
[origin/master] Sync with 2.3.3
While there is an option to add all remote-tracking branches (-r), and
another to add all local+remote branches (-a), there is no option to add
only local branches. Adding such an option could be considered, but a
user would likely already expect that the above command line considers
the lack of rev other than for --topics as meaning all local branches,
like when there is no argument at all.
Moreover, when using -r and -a along with --topics, the first local or
remote-tracking branch, depending on alphabetic order is used instead of
the one given after --topics (any rev given on the command line is
actually simply ignored when either -r or -a is given). And if no rev is
given at all, the fact that the first alphetical branch is the base of
topics is probably not expected by users (Maybe --topics should always
require one rev on the command line?)
This change makes
"show-branch --topics $rev"
act as
"show-branch --topics $rev $(git for-each-ref refs/heads
--format='%(refname:short)')"
"show-branch -r --topics $rev ..."
act as
"show-branch --topics $rev ... $(git for-each-ref refs/remotes
--format='%(refname:short)')"
instead of
"show-branch --topics $(git for-each-ref refs/remotes
--format='%(refname:short)')"
and
"show-branch -a --topics $rev ..."
act as
"show-branch --topics $rev ... $(git for-each-ref refs/heads refs/remotes
--format='%(refname:short)')"
instead of
"show-branch --topics $(git for-each-ref refs/heads refs/remotes
--format='%(refname:short)')"
---
builtin/show-branch.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 365228a..ef9e719 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -718,7 +718,7 @@ int cmd_show_branch(int ac, const char **av, const char
*prefix)
}
/* If nothing is specified, show all branches by default */
- if (ac + all_heads + all_remotes == 0)
+ if (ac <= topics && all_heads + all_remotes == 0)
all_heads = 1;
if (reflog) {
@@ -785,13 +785,13 @@ int cmd_show_branch(int ac, const char **av, const char
*prefix)
}
free(ref);
}
- else if (all_heads + all_remotes)
- snarf_refs(all_heads, all_remotes);
else {
while (0 < ac) {
append_one_rev(*av);
ac--; av++;
}
+ if (all_heads + all_remotes)
+ snarf_refs(all_heads, all_remotes);
}
head_p = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
--
2.3.3.3.g6c0eb00
--
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 in git-verify-pack or in its documentation?
Hi, git-verify-pack's man page says the following about --stat-only: Do not verify the pack contents; only show the histogram of delta chain length. With --verbose, list of objects is also shown. However, there is no difference of output between --verbose and --verbose --stat-only (and in fact, looking at the code, only one of them has an effect, --stat-only having precedence). The text above also implies that this should only display the stats without doing any sha1 validation, but afaict from a quick glance at the index-pack code, they are always performed. Is it an implementation problem or a documentation problem? Cheers, Mike -- 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 2/2] t7102: add 'reset -' tests
Hi,
On Sat, Mar 14, 2015 at 2:40 AM, Eric Sunshine wrote:
> On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar
> wrote:
>> Add following test cases:
>> 1) Confirm error message when git reset is used with no previous branch
>> 2) Confirm git reset - works like git reset @{-1}
>> 3) Confirm "-" is always treated as a commit unless the -- file option
>> is specified
>> 4) Confirm "git reset -" works normally even when a file named @{-1} is
>> present
>>
>> Helped-by: Eric Sunshine
>> Helped-by: Matthieu Moy
>> Helped-by: David Aguilar
>> Signed-off-by: Sudhanshu Shekhar
>> ---
>> Eric: Thank you for pointing out the mistake. The '&&' after the Here
>> Docs was causing the issue. I have removed the concatenation from
>> there, hope that's okay.
>
> The && needs to go on the first line, not the last line of the here-doc.
>
> However, that was not my main concern in the previous review. What
> disturbed me was that the new tests, which were supposed to be
> checking if "-" behaved as @{-1}, were succeeding even without patch
> 1/2 applied which implemented the "-" alias for @{-1}. That seems
> wrong. I don't think you particularly addressed that issue in this
> version (even though the first couple tests will now fail without 1/2
> due to the changed error message).
Actually, The issue was caused due a HERE docs error. If you run this
patch now, you will see that 7 out of the 8 test cases fail.
>
>> Regarding the @{-1} test case, I created it as a check for Junio's
>> comment on the error message generated by "git reset -" when a file
>> named @{-1} is there. Since, in this situation "git reset @{-1}" will
>> return an error (but "reset -" shouldn't).
>
> Reminder: Wrap commentary to about column 72, as you would the commit
> message. (I re-wrapped it manually to reply to it.)
>
>> I have renamed the folder to 'dash' as suggested by you, keeping the
>> old name only where it made sense.
>
> Considering that the test titles already tell us the intent of the
> tests, I don't find that the directory name "no_previous" adds much
> value to tests checking the behavior of "-" with no previous branch. A
> single, consistent name used throughout all these tests would be less
> surprising and place smaller cognitive load on the reader.
>
> More below.
>
>> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
>> index 98bcfe2..18523c1 100755
>> --- a/t/t7102-reset.sh
>> +++ b/t/t7102-reset.sh
>> @@ -568,4 +568,162 @@ test_expect_success 'reset --mixed sets up work tree' '
>> test_cmp expect actual
>> '
>>
>> +test_expect_success 'reset - with no previous branch fails' '
>> + git init no_previous &&
>> + test_when_finished rm -rf no_previous &&
>> + (
>> + cd no_previous &&
>> + test_must_fail git reset - 2>actual
>> + ) &&
>> + test_i18ngrep "ambiguous argument" no_previous/actual
>> +'
>> +
>> +test_expect_success 'reset - while having file named - and no previous
>> branch fails' '
>> + git init no_previous &&
>> + test_when_finished rm -rf no_previous &&
>> + (
>> + cd no_previous &&
>> + >- &&
>> + test_must_fail git reset - 2>actual
>> + ) &&
>> + test_i18ngrep "ambiguous argument" no_previous/actual
>> +'
>> +
>> +
>
> Style: Unnecessary extra blank line.
>
>> +test_expect_success \
>> + 'reset - in the presence of file named - with previous branch resets
>> commit' '
>> + cat >expect <<-EOF
>
> Place the && at the end of this line. Also, prefix EOF with a
> backslash to indicate that you don't intend any interpolation to occur
> within the here-doc. So:
>
> cat >expect <<-\EOF &&
>
> Ditto for the remaining tests.
>
Thank you for taking the time out to point out these style changes to
me. There is a lot I have to learn about open source contribution yet
and I believe during the course of this microproject, I did learn
something about this (By making some very silly mistakes). Thank you
to all the developers who took time out to review my code and point
out the mistakes I had done.
Regards,
Sudhanshu
--
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 RFC 1/3] add: add new --exclude option to git add
>> Isn't the problem one of "how are users to discover such magic". Yes it was main reason. -- 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
