Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Junio C Hamano gits...@pobox.com writes: Taking two random examples from an early and a late parts of the patch: --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name) enum object_type type; unsigned long size; char *buffer = read_sha1_file(sha1, type, size); - if (memcmp(buffer, object , 7) || + if (!starts_with(buffer, object ) || [...] The original hunks show that the code knows and relies on magic numbers 7 and 8 very clearly and there are rooms for improvement. Like: what if the file is empty? -- 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
[PATCH 1/2] checkout_entry(): use the strbuf throughout the function
There is no need to break out the buf and len members into
separate temporary variables. Rename path_buf to path and use
path.buf and path.len directly. This makes it easier to reason about
the data flow in the function.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
entry.c | 32
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/entry.c b/entry.c
index 7b7aa81..9adddb6 100644
--- a/entry.c
+++ b/entry.c
@@ -245,27 +245,25 @@ static int check_path(const char *path, int len, struct
stat *st, int skiplen)
int checkout_entry(struct cache_entry *ce,
const struct checkout *state, char *topath)
{
- static struct strbuf path_buf = STRBUF_INIT;
- char *path;
+ static struct strbuf path = STRBUF_INIT;
struct stat st;
- int len;
if (topath)
return write_entry(ce, topath, state, 1);
- strbuf_reset(path_buf);
- strbuf_add(path_buf, state-base_dir, state-base_dir_len);
- strbuf_add(path_buf, ce-name, ce_namelen(ce));
- path = path_buf.buf;
- len = path_buf.len;
+ strbuf_reset(path);
+ strbuf_add(path, state-base_dir, state-base_dir_len);
+ strbuf_add(path, ce-name, ce_namelen(ce));
- if (!check_path(path, len, st, state-base_dir_len)) {
+ if (!check_path(path.buf, path.len, st, state-base_dir_len)) {
unsigned changed = ce_match_stat(ce, st,
CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
if (!changed)
return 0;
if (!state-force) {
if (!state-quiet)
- fprintf(stderr, %s already exists, no
checkout\n, path);
+ fprintf(stderr,
+ %s already exists, no checkout\n,
+ path.buf);
return -1;
}
@@ -280,12 +278,14 @@ int checkout_entry(struct cache_entry *ce,
if (S_ISGITLINK(ce-ce_mode))
return 0;
if (!state-force)
- return error(%s is a directory, path);
- remove_subtree(path);
- } else if (unlink(path))
- return error(unable to unlink old '%s' (%s), path,
strerror(errno));
+ return error(%s is a directory, path.buf);
+ remove_subtree(path.buf);
+ } else if (unlink(path.buf))
+ return error(unable to unlink old '%s' (%s),
+path.buf, strerror(errno));
} else if (state-not_new)
return 0;
- create_directories(path, len, state);
- return write_entry(ce, path, state, 0);
+
+ create_directories(path.buf, path.len, state);
+ return write_entry(ce, path.buf, state, 0);
}
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] entry.c: fix possible buffer overflow in remove_subtree()
remove_subtree() manipulated path in a fixed-size buffer even though
the length of the input, let alone the length of entries within the
directory, were not known in advance. Change the function to take a
strbuf argument and use that object as its scratch space.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
In
fd356f6aa8 entry.c: convert checkout_entry to use strbuf
from last October, checkout_entry() was changed to use strbuf, but
this callee must have been overlooked.
entry.c | 34 +-
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/entry.c b/entry.c
index 9adddb6..77c6882 100644
--- a/entry.c
+++ b/entry.c
@@ -44,33 +44,33 @@ static void create_directories(const char *path, int
path_len,
free(buf);
}
-static void remove_subtree(const char *path)
+static void remove_subtree(struct strbuf *path)
{
- DIR *dir = opendir(path);
+ DIR *dir = opendir(path-buf);
struct dirent *de;
- char pathbuf[PATH_MAX];
- char *name;
+ int origlen = path-len;
if (!dir)
- die_errno(cannot opendir '%s', path);
- strcpy(pathbuf, path);
- name = pathbuf + strlen(path);
- *name++ = '/';
+ die_errno(cannot opendir '%s', path-buf);
while ((de = readdir(dir)) != NULL) {
struct stat st;
+
if (is_dot_or_dotdot(de-d_name))
continue;
- strcpy(name, de-d_name);
- if (lstat(pathbuf, st))
- die_errno(cannot lstat '%s', pathbuf);
+
+ strbuf_addch(path, '/');
+ strbuf_addstr(path, de-d_name);
+ if (lstat(path-buf, st))
+ die_errno(cannot lstat '%s', path-buf);
if (S_ISDIR(st.st_mode))
- remove_subtree(pathbuf);
- else if (unlink(pathbuf))
- die_errno(cannot unlink '%s', pathbuf);
+ remove_subtree(path);
+ else if (unlink(path-buf))
+ die_errno(cannot unlink '%s', path-buf);
+ strbuf_setlen(path, origlen);
}
closedir(dir);
- if (rmdir(path))
- die_errno(cannot rmdir '%s', path);
+ if (rmdir(path-buf))
+ die_errno(cannot rmdir '%s', path-buf);
}
static int create_file(const char *path, unsigned int mode)
@@ -279,7 +279,7 @@ int checkout_entry(struct cache_entry *ce,
return 0;
if (!state-force)
return error(%s is a directory, path.buf);
- remove_subtree(path.buf);
+ remove_subtree(path);
} else if (unlink(path.buf))
return error(unable to unlink old '%s' (%s),
path.buf, strerror(errno));
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Fix possible buffer overflow in remove_subtree()
These patches are proposed for maint (but also apply cleanly to master). I presume that this is exploitable via Git commands, though I haven't verified it explicitly [1]. I *think* that the rest of the file is OK. open_output_fd() initially looks suspicious, because it strcpy()s a string onto the end of its path argument. But that is only done when to_tempfile is set, which in turn is handled consistently up the callstack up to the point where it is initially set in checkout_entry() if topath is not NULL. So as long as the caller obeys checkout_entry()'s docstring and passes a long enough buffer for topath, I think everything is OK. In any case, the string appended in open_output_fd() is not under the control of the user, so even if there were a bug in this code path it shouldn't be exploitable. [1] For example, it is conceivable that there are some checks when writing a tree that prevent files with such long names from being written by Git. But even if so, it is clearly a bug that could be hit locally on any filesystem where PATH_MAX is not a hard limit. Michael Haggerty (2): checkout_entry(): use the strbuf throughout the function entry.c: fix possible buffer overflow in remove_subtree() entry.c | 64 1 file changed, 32 insertions(+), 32 deletions(-) -- 1.9.0 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Committing a change from one branch another branch
Hi, I have two branches. - master-b1 - master-b2 Suppose I'm in master-b1 then did a change on master-b1 $ git add test/init.c $ git commit -s -m init.c Changed! $ git log Author: Jagan Teki jagannadh.t...@gmail.com Date: Thu Mar 13 00:48:44 2014 -0700 init.c Changed! $ git checkout master-b2 $ git log Author: Jagan Teki jagannadh.t...@gmail.com Date: Thu Mar 13 00:48:44 2014 -0700 init.c Changed! How can we do this, any idea? -- Jagan. -- To unsubscribe from this list: send the line 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: Committing a change from one branch another branch
Jagan Teki jagannadh.t...@gmail.com writes: How can we do this, any idea? git cherry-pick Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] checkout_entry(): use the strbuf throughout the function
On Thu, Mar 13, 2014 at 4:19 PM, Michael Haggerty mhag...@alum.mit.edu wrote: There is no need to break out the buf and len members into separate temporary variables. Rename path_buf to path and use path.buf and path.len directly. This makes it easier to reason about the data flow in the function. I wanted to keep changes to minimum when I did that. But I guess it has its downside as you found out. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] entry.c: fix possible buffer overflow in remove_subtree()
On Thu, Mar 13, 2014 at 4:19 PM, Michael Haggerty mhag...@alum.mit.edu wrote: remove_subtree() manipulated path in a fixed-size buffer even though the length of the input, let alone the length of entries within the directory, were not known in advance. Change the function to take a strbuf argument and use that object as its scratch space. Converting more PATH_MAX to strbuf could be a micro project. Not sure if we still any more of them though. The patches look fine btw. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] connect.c: SP after }, not TAB
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
I missed the else anchor because it's indented far to the right and
misread the code for a second there. Might as well fix it while I'm
at it.
connect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/connect.c b/connect.c
index 4cb1c7c..4be84e8 100644
--- a/connect.c
+++ b/connect.c
@@ -727,7 +727,7 @@ struct child_process *git_connect(int fd[2], const char
*url,
*arg++ = port;
}
*arg++ = ssh_host;
- } else {
+ } else {
/* remove repo-local variables from the environment */
conn-env = local_repo_env;
conn-use_shell = 1;
--
1.9.0.40.gaa8c3ea
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Your Trust and Partnership
-- Your Trust and Partnership Firstly, I apologize for sending you this sensitive information via e-mail instead of a Certified mail/Post-mail. This is due to the urgency and importance of the information. This project is based on trust, confidentiality and sincerity of purpose in order to have an acceptable meeting of the minds.. My name is Mr Armel Yacouba, Regional Manager, (African Development Bank (ADB) Ouagadougou Burkina-Faso, West Africa). I am sending this brief letter to solicit your partnership to transfer the sum of us$2.4 million US Dollars that belongs to my late customer, who passed on heart-related condition after years of struggling with the disease. On your confirmation of this message and indicating your interest, I will furnish you with more details and procedures. kindest regards, Mr.Armel Yacouba. -- To unsubscribe from this list: send the line 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 0/2] Fix possible buffer overflow in remove_subtree()
On Thu, Mar 13, 2014 at 10:19:06AM +0100, Michael Haggerty wrote: These patches are proposed for maint (but also apply cleanly to master). I presume that this is exploitable via Git commands, though I haven't verified it explicitly [1]. It's possible to overflow this buffer, like: git init repo cd repo blob=$(git hash-object -w /dev/null) big=$(perl -e 'print a x 250') (for i in $(seq 1 17); do mkdir $big cd $big; done) printf 100644 blob $blob\t$big\n tree tree=$(git mktree tree) git read-tree $tree git checkout-index -f --all but I'm not sure if it is easily exploitable for two reasons: 1. We never actually return from the function with the smashed stack. Immediately after overflowing the buffer, we call lstat(), which will fail with ENAMETOOLONG (at least on Linux), causing us to call into die_errno and exit. 2. The overflow relies on us trying to move a very deep hierarchy out of the way, but I could not convince git to _create_ such a hierarchy in the first place. Here I do it using relative paths and recursion, but git generally tries to create paths from the top of the repo using full pathnames. So it gets ENAMETOOLONG trying to create the paths in the first place. Of course somebody may be more clever than I am, or perhaps some systems have a PATH_MAX that is not enforced by the kernel. It's still a suspicious bit of code, and I think your patches are a strict improvement. Besides fixing this potential problem, I notice that we currently put 4096 bytes on the stack for a recursive function. Removing a/a/a... can put up to 8M on the stack, which might be too much on some systems (besides just being silly and wasteful). -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: Committing a change from one branch another branch
Jagan:
On Thu, Mar 13, 2014 at 4:56 AM, Jagan Teki jagannadh.t...@gmail.com wrote:
Hi,
Hello,
I have two branches.
- master-b1
- master-b2
Suppose I'm in master-b1 then did a change
on master-b1
$ git add test/init.c
$ git commit -s -m init.c Changed!
$ git log
Author: Jagan Teki jagannadh.t...@gmail.com
Date: Thu Mar 13 00:48:44 2014 -0700
init.c Changed!
$ git checkout master-b2
$ git log
Author: Jagan Teki jagannadh.t...@gmail.com
Date: Thu Mar 13 00:48:44 2014 -0700
init.c Changed!
How can we do this, any idea?
What you're asking is ambiguous and vague. The example output that you
give doesn't even really make sense. You need to give more details
about what you have and what you want to do to get proper help.
Or join #git on irc.freenode.net for real-time help if you aren't sure
how to explain it.
Regards,
--
Brandon McCaig bamcc...@gmail.com bamcc...@castopulence.org
Castopulence Software https://www.castopulence.org/
Blog http://www.bamccaig.com/
perl -E '$_=q{V zrna gur orfg jvgu jung V fnl. }.
q{Vg qbrfa'\''g nyjnlf fbhaq gung jnl.};
tr/A-Ma-mN-Zn-z/N-Zn-zA-Ma-m/;say'
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with starts_with()
On 03/12/2014 03:06 PM, Quint Guvernator wrote: 2014-03-12 9:51 GMT-04:00 Duy Nguyen pclo...@gmail.com: starts_with(..) == !memcmp(...). So you need to negate every replacement. My apologies--it doesn't look like the tests caught it either. I will fix this and submit a new patch. It is very, very unlikely that you inverted the sense of dozens of tests throughout the Git code base and the tests ran correctly. I rather think that you made a mistake when testing. You should double- and triple-check that you really ran the tests and ran them correctly. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New GSoC microproject ideas
On 03/12/2014 08:04 PM, Junio C Hamano wrote: Here is another, as I seem to have managed to kill another one ;-) -- 8 -- VAR=VAL command is sufficient to run 'command' with environment variable VAR set to value VAL without affecting the environment of the shell itself, but we cannot do the same with a shell function (most notably, test_must_fail); we have subshell invocations with multiple lines like this: ... ( VAR=VAL export VAR test_must_fail git command ) ... but that could be expressed as ... test_must_fail env VAR=VAL git comand ... Find and shorten such constructs in existing test scripts. Thanks; I just added it. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: An idea for git bisect and a GSoC enquiry
On 03/12/2014 07:31 PM, Junio C Hamano wrote: Jacopo Notarstefano jacopo.notarstef...@gmail.com writes: On Mon, Mar 3, 2014 at 7:34 PM, Junio C Hamano gits...@pobox.com wrote: I think you fundamentally cannot use two labels that are merely distinct and bisect correctly. You need to know which ones (i.e. good) are to be excluded and the other (i.e. bad) are to be included when computing the remaining to be tested set of commits. Good point. Yes, this isn't viable. But if you make them into --no-longer-X vs --still-X, then it will be viable without us knowing what X means. Yes, but who wants to type such long and inelegant option names? It seems to me that we can infer which mark is which from the normal bisect user interaction. At the startup phase of a bisect, there are only three cases: 1. There are fewer than two different types of marks on tested commits. For example, maybe one commit has been marked bad. Or two commits have both been marked slow. In this case we wait for the user to choose another commit manually, so we don't have to know the meaning of the mark. 2. There are two different types of marks, but no commits with differing marks are ancestors of each other. In this case, we pick the merge base of two commits with differing marks and present it to the user for testing. But we can do that without knowing which mark is before the change and which mark means after the change. So just defer the inference. 3. There are two different types of marks, and a commit with one mark is an ancestor of a commit with the other mark. In this case, it is clear from the ancestry which mark means before the change and which mark means after the change. So record the orientation of the marks and continue like in the old days. Of course, there are still details to be worked out, like how to tag the commits before we know which mark means what. But that is just a clerical problem, not a fundamental one. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GSoC proposal: port pack bitmap support to libgit2.
Hi, On Wed, Mar 12, 2014 at 4:19 PM, Yuxuan Shui yshu...@gmail.com wrote: Hi, I'm Yuxuan Shui, a undergraduate student from China. I'm applying for GSoC 2014, and here is my proposal: I found this idea on the ideas page, and did some research about it. The pack bitmap patchset add a new .bitmap file for every pack file which contains the reachability information of selected commits. This information is used to speed up git fetching and cloning, and produce a very convincing results. The goal of my project is to port the pack bitmap implementation in core git to libgit2, so users of libgit2 could benefit from this optimization as well. Please let me know if my proposal makes sense, thanks. P.S. I've submitted by microproject patch[1], but haven't received any response yet. [1]: http://thread.gmane.org/gmane.comp.version-control.git/243854 -- Regards Yuxuan Shui Could anyone please review my proposal a little bit? Is this project helpful and worth doing? Did I get anything wrong in my proposal? Thanks. -- Regards Yuxuan Shui -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Quint Guvernator quintus.pub...@gmail.com writes: The result after the conversion, however, still have the same magic numbers, but one less of them each. Doesn't it make it harder to later spot the patterns to come up with a better abstraction that does not rely on the magic number? It is _not_ my goal to make the code harder to maintain down the road. Good. So, at this point, which hunks (if any) are worth patching? Will, I am not going through all the mechanical hits to memcmp() and judge each and every one if it is a good idea to convert. Anybody who does so in order to tell you which hunks are worth patching would end up being the one doing the real work, and at that point there is nothing left to be credited as your work anymore ;-) But as Peff said, there are good bits, like these ones just for a few examples. diff --git a/builtin/apply.c b/builtin/apply.c index a7e72d5..16c20af 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -1631,7 +1631,7 @@ static int parse_fragment(const char *line, unsigned long size, * l10n of \ No newline... is at least that long. */ case '\\': - if (len 12 || memcmp(line, \\ , 2)) + if (len 12 || !starts_with(line, \\ )) return -1; break; } @@ -1646,7 +1646,7 @@ static int parse_fragment(const char *line, unsigned long size, * it in the above loop because we hit oldlines == newlines == 0 * before seeing it. */ - if (12 size !memcmp(line, \\ , 2)) + if (12 size starts_with(line, \\ )) offset += linelen(line, size); patch-lines_added += added; These two are about An incomplete line marker begins with a backslash and a SP and there is no other significance in the constant 2 (like, after we recognise the match, we start scanning the remainder of the line starting at the offset 2). It is a tangent but I notice that these two parts (both in the original and in the version after patch) contradict what the incomplete last line marker should look like in a minor detail. On the other hand, I think this one from nearby is iffy. @@ -846,8 +846,8 @@ static int has_epoch_timestamp(const char *nameline) * -MM-DD hh:mm:ss must be from either 1969-12-31 * (west of GMT) or 1970-01-01 (east of GMT) */ - if ((zoneoffset 0 memcmp(timestamp, 1969-12-31, 10)) || - (0 = zoneoffset memcmp(timestamp, 1970-01-01, 10))) + if ((zoneoffset 0 !starts_with(timestamp, 1969-12-31)) || + (0 = zoneoffset !starts_with(timestamp, 1970-01-01))) return 0; hourminute = (strtol(timestamp + 11, NULL, 10) * 60 + If one looks at the post-context of the hunk, one would realize that this codepath very intimately knows how the timestamp should look like at the byte-offset level, not just -MM-DD ought to be 10-byte long, but there should be two-digit hour part after skipping one byte after that -MM-DD part, followed by two-digit minute part after further skipping one byte, knowing that these details are guaranteed by the stamp_regexp[] pattern it earlier made sure the given string would match. I do not think it is a good idea to reduce this kind of precise format knowledge from this function in the first place (after all, this is parsing a header line in a traditional diff whose format is known to us), and even if it were our eventual goal to reduce the precise format knowledge, it would not help very much to get rid of constant 10 only from these two memcmp() calls, and that is why I think this hunk is iffy. Hope the above shows what kind of thinking is needed to judge each change from memcmp() to !starts_with(). 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] general style: replaces memcmp() with proper starts_with()
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: Taking two random examples from an early and a late parts of the patch: --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name) enum object_type type; unsigned long size; char *buffer = read_sha1_file(sha1, type, size); -if (memcmp(buffer, object , 7) || +if (!starts_with(buffer, object ) || [...] The original hunks show that the code knows and relies on magic numbers 7 and 8 very clearly and there are rooms for improvement. Like: what if the file is empty? Yes. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] connect.c: SP after }, not TAB
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] general style: replaces memcmp() with proper starts_with()
On Thu, Mar 13, 2014 at 10:47:28AM -0700, Junio C Hamano wrote: --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name) enum object_type type; unsigned long size; char *buffer = read_sha1_file(sha1, type, size); - if (memcmp(buffer, object , 7) || + if (!starts_with(buffer, object ) || [...] The original hunks show that the code knows and relies on magic numbers 7 and 8 very clearly and there are rooms for improvement. Like: what if the file is empty? Yes. I think this one is actually OK. The result of read_sha1_file is NUL-terminated, and we know that starts_with reads byte-by-byte (the prior memcmp is wrong, but only if you care about accessing bytes past the end of the NUL). That whole piece of code seems silly, though, IMHO. It should be using parse_tag or peel_to_type. -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 v4] install_branch_config: simplify verbose messages logic
Eric Sunshine sunsh...@sunshineco.com writes:
Shouldn't this logic [to decide what the printf arguments should
be] also be encoded in the table?
...
The same argument also applies to computation of the 'name' variable
above. It too can be pushed into the the table.
Because the printf argument logic does not have to be in the
table, the same argument does not apply to the 'name' thing.
After looking at the v5 patch, I do not think an extra two-element
array to switch between remote vs shortname is making it any easier
to read. I would have to say that personally I find that
const char *name[] = {remote, shortname};
... long swath of code ...
printf_ln(... name[!remote_is_branch] ...);
is a lot harder to read than:
printf_ln(... remote_is_branch ? shortname : branch ...);
HTH, and 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: An idea for git bisect and a GSoC enquiry
Michael Haggerty mhag...@alum.mit.edu writes: It seems to me that we can infer which mark is which from the normal bisect user interaction. At the startup phase of a bisect, there are only three cases: 1. There are fewer than two different types of marks on tested commits. For example, maybe one commit has been marked bad. Or two commits have both been marked slow. In this case we wait for the user to choose another commit manually, so we don't have to know the meaning of the mark. 2. There are two different types of marks, but no commits with differing marks are ancestors of each other. In this case, we pick the merge base of two commits with differing marks and present it to the user for testing. But we can do that without knowing which mark is before the change and which mark means after the change. So just defer the inference. 3. There are two different types of marks, and a commit with one mark is an ancestor of a commit with the other mark. In this case, it is clear from the ancestry which mark means before the change and which mark means after the change. So record the orientation of the marks and continue like in the old days. Of course, there are still details to be worked out, like how to tag the commits before we know which mark means what. But that is just a clerical problem, not a fundamental one. Yup, with an extra state kept somewhere in $GIT_DIR, we should in principle be able to defer the value judgement (aka which one should be treated as a bottom of the range). The first change that is needed for this scheme to be workable is to decide how we mark such an unknown state at the beginning, though. We assume that we need to keep track of a single top one (bad, aka no-longer-good) while we have to keep track of multiple bottom ones (good). There also is a safety valve in the current logic for transitioning from case #2 to case #3; when a common ancestor is marked as bad (aka no-longer-good), we notice that the original bisection is screwy in the sense that the user is seeing not just a single state flip that made something that used to be good into bad. I am afraid that we may instead _silently_ decide that the user is trying to locate a state flip that made something that used to be bad (at the common ancestor) into good with the logic proposed above. From the point of view of the user who wanted to find a regression by marking one as bad and the other good, running bisection whose semantics suddenly and silently changed into an opposite where was it fixed hunt would be an unpleasant and confusing experience. I do not know, without knowing the meaning of slow and fast (which implicitly tells us which way the user intends to bisect), how well we can keep that safety valve. Other than that, I like the idea. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Microproject idea: new OPT_* macros for PARSE_OPT_NONEG
Duy Nguyen pclo...@gmail.com writes: On Sat, Mar 8, 2014 at 2:20 AM, Junio C Hamano gits...@pobox.com wrote: Looking at git grep -B3 OPT_NONEG output, it seems that NONEG is associated mostly with OPTION_CALLBACK and OPTION_SET_INT in the existing code. Perhaps OPT_SET_INT should default to not just OPT_NOARG but also OPT_NONEG? There are OPT_SET_INT() that should not have NONEG in current code. So there are two sets of SET_INT anyway. Either we convert them all to a new macro that takes an extra flag, or we add OPT_SET_INT_NONEG() that covers one set and leave the other set alone. Are you forgetting the third alternative, of swapping the default, if the ones that do not want NONEG are in the minority, to reduce the number of spelled-out instances? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument
Yuxuan Shui yshu...@gmail.com writes:
Since fsck_ident doesn't change the content of **ident, the type of
ident could be const char **.
This change is required to rewrite fsck_commit() to use skip_prefix().
Signed-off-by: Yuxuan Shui yshu...@gmail.com
---
It may not be a bad idea to read and understand reviews other people
are receiving for their microprojects, e.g. $gmane/243852.
Change the type is not technically incorrect per-se, but when
viewed in git shortlog output, it wastes more bytes than it
conveys information about this change if stated differently. Any
patch that touch existing code is a change by definition.
Perhaps
fsck.c:fsck_ident(): ident argument points at a const string
or something?
I see that the body of the patch follows the review by Peff on the
previous round of this series, so I'll forge a Helped-by: or
something into the log message when I queue this patch.
Thanks.
fsck.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fsck.c b/fsck.c
index 99c0497..7776660 100644
--- a/fsck.c
+++ b/fsck.c
@@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict,
fsck_error error_func)
return retval;
}
-static int fsck_ident(char **ident, struct object *obj, fsck_error
error_func)
+static int fsck_ident(const char **ident, struct object *obj, fsck_error
error_func)
{
if (**ident == '')
return error_func(obj, FSCK_ERROR, invalid author/committer
line - missing space before email);
@@ -281,7 +281,7 @@ static int fsck_ident(char **ident, struct object *obj,
fsck_error error_func)
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- char *buffer = commit-buffer;
+ const char *buffer = commit-buffer;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()
Yuxuan Shui yshu...@gmail.com writes:
Currently we use memcmp() in fsck_commit() to check if buffer start
with a certain prefix, and skip the prefix if it does. This is exactly
what skip_prefix() does. And since skip_prefix() has a self-explaintory
name, this could make the code more readable.
Signed-off-by: Yuxuan Shui yshu...@gmail.com
---
fsck.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/fsck.c b/fsck.c
index 7776660..7e6b829 100644
--- a/fsck.c
+++ b/fsck.c
@@ -281,7 +281,7 @@ static int fsck_ident(const char **ident, struct object
*obj, fsck_error error_f
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- const char *buffer = commit-buffer;
+ const char *buffer = commit-buffer, *tmp;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
@@ -290,15 +290,17 @@ static int fsck_commit(struct commit *commit,
fsck_error error_func)
if (commit-date == ULONG_MAX)
return error_func(commit-object, FSCK_ERROR, invalid
author/committer line);
- if (memcmp(buffer, tree , 5))
+ buffer = skip_prefix(buffer, tree );
+ if (buffer == NULL)
We encourage people to write this as:
if (!buffer)
The same comment applies to other new lines in this patch.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()
Junio C Hamano gits...@pobox.com writes:
-if (memcmp(buffer, tree , 5))
+buffer = skip_prefix(buffer, tree );
+if (buffer == NULL)
We encourage people to write this as:
if (!buffer)
The same comment applies to other new lines in this patch.
I also see a lot of repetitions in the code, before or after the
patch. I wonder if a further refactoring along this line on top of
these two patches might be worth considering.
No, I am not proud of sneaking tree_sha1[] array pointers as a
seemingly boolean-looking must-match-header parameter into the
helper, but this is merely a how about going in this direction
weather-balloon patch, so
fsck.c | 83 --
1 file changed, 61 insertions(+), 22 deletions(-)
diff --git a/fsck.c b/fsck.c
index 6aab23b..a3eea7f 100644
--- a/fsck.c
+++ b/fsck.c
@@ -279,10 +279,55 @@ static int fsck_ident(const char **ident, struct object
*obj, fsck_error error_f
return 0;
}
+static int fsck_object_line(struct commit *commit, fsck_error error_func,
+ const char **buffer, const char *header,
+ unsigned char must_match_header[])
+{
+ unsigned char sha1_buf[20];
+ unsigned char *sha1 = must_match_header ? must_match_header : sha1_buf;
+ const char *buf;
+
+ buf = skip_prefix(*buffer, header);
+ if (!buf) {
+ if (must_match_header)
+ return error_func(commit-object, FSCK_ERROR,
+ invalid format - expected '%.*s'
line,
+ (int) strlen(header) - 1,
+ header);
+ return 1;
+ }
+ if (get_sha1_hex(buf, sha1) || buf[40] != '\n')
+ return error_func(commit-object, FSCK_ERROR,
+ invalid '%.*s' line format - bad sha1,
+ (int) strlen(header) - 1,
+ header);
+ *buffer = buf + 41;
+ return 0;
+}
+
+static int fsck_ident_line(struct commit *commit, fsck_error error_func,
+ const char **buffer, const char *header)
+{
+ const char *buf;
+ int err;
+
+ buf = skip_prefix(*buffer, header);
+ if (!buf)
+ return error_func(commit-object, FSCK_ERROR,
+ invalid format - expected '%.*s' line,
+ (int) strlen(header) - 1,
+ header);
+ err = fsck_ident(buf, commit-object, error_func);
+ if (err)
+ return err;
+ *buffer = buf;
+ return 0;
+}
+
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- const char *buffer = commit-buffer, *tmp;
- unsigned char tree_sha1[20], sha1[20];
+ const char *buffer = commit-buffer;
+ unsigned char tree_sha1[20];
struct commit_graft *graft;
int parents = 0;
int err;
@@ -290,18 +335,17 @@ static int fsck_commit(struct commit *commit, fsck_error
error_func)
if (commit-date == ULONG_MAX)
return error_func(commit-object, FSCK_ERROR, invalid
author/committer line);
- buffer = skip_prefix(buffer, tree );
- if (!buffer)
- return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'tree' line);
- if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
- return error_func(commit-object, FSCK_ERROR, invalid 'tree'
line format - bad sha1);
- buffer += 41;
- while ((tmp = skip_prefix(buffer, parent ))) {
- buffer = tmp;
- if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n')
- return error_func(commit-object, FSCK_ERROR, invalid
'parent' line format - bad sha1);
- buffer += 41;
- parents++;
+ err = fsck_object_line(commit, error_func, buffer, tree , tree_sha1);
+ if (err)
+ return err;
+ while (1) {
+ err = fsck_object_line(commit, error_func, buffer, parent ,
NULL);
+ if (err 0)
+ return err;
+ else if (!err)
+ parents++;
+ else
+ break;
}
graft = lookup_commit_graft(commit-object.sha1);
if (graft) {
@@ -324,16 +368,11 @@ static int fsck_commit(struct commit *commit, fsck_error
error_func)
if (p || parents)
return error_func(commit-object, FSCK_ERROR, parent
objects missing);
}
- buffer = skip_prefix(buffer, author );
- if (!buffer)
- return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'author' line);
- err = fsck_ident(buffer, commit-object, error_func);
+
+ err = fsck_ident_line(commit,
[PATCH] GSoC Change multiple if-else statements to be table-driven
Signed-off-by: Yao Zhao zhaox...@umn.edu
---
GSoC_MicroProject_#8
Hello Eric,
Thanks for reviewing my code. I implemented table-driven method this time and
correct the style
problems indicated in review.
Thank you,
Yao
branch.c | 72
1 file changed, 50 insertions(+), 22 deletions(-)
diff --git a/branch.c b/branch.c
index 723a36b..6451c99 100644
--- a/branch.c
+++ b/branch.c
@@ -49,10 +49,43 @@ static int should_setup_rebase(const char *origin)
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
+
const char *shortname = remote + 11;
int remote_is_branch = starts_with(remote, refs/heads/);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
+ int size=8, i;
+
+ typedef struct PRINT_LIST {
+ const char *print_str;
+ const char *arg2;
+ //arg1 is always local, so I only add arg2 and arg3 in struct
+ const char *arg3;
+ int b_rebasing;
+ int b_remote_is_branch;
+ int b_origin;
+ } PRINT_LIST;
+
+ PRINT_LIST print_list[] = {
+ {.print_str = _(Branch %s set up to track remote branch %s
from %s by rebasing.),
+ .arg2 = shortname, .arg3 = origin,
+.b_rebasing = 1, .b_remote_is_branch =
1, .b_origin = 1},
+ {.print_str = _(Branch %s set up to track remote branch %s from
%s.),
+ .arg2 = shortname, .arg3 = origin,
+.b_rebasing = 0, .b_remote_is_branch =
1, .b_origin = 1},
+{.print_str = _(Branch %s set up to track local branch %s by rebasing.),
+ .arg2 = shortname, .b_rebasing = 1,
.b_remote_is_branch = 1, .b_origin = 0},
+ {.print_str = _(Branch %s set up to track local branch %s.),
+ .arg2 = shortname, .b_rebasing = 0,
.b_remote_is_branch = 1, .b_origin = 0},
+ {.print_str = _(Branch %s set up to track remote ref %s by
rebasing.),
+ .arg2 = remote, .b_rebasing = 1,
.b_remote_is_branch = 0, .b_origin = 1},
+ {.print_str = _(Branch %s set up to track remote ref %s.),
+ .arg2 = remote, .b_rebasing = 0,
.b_remote_is_branch = 0, .b_origin = 1},
+ {.print_str = _(Branch %s set up to track local ref %s by
rebasing.),
+ .arg2 = remote, .b_rebasing = 1,
.b_remote_is_branch = 0, .b_origin = 0},
+ {.print_str = _(Branch %s set up to track local ref %s.),
+ .arg2 = remote, .b_rebasing = 0,
.b_remote_is_branch = 0, .b_origin = 0},
+};
I am confused here: I use struct initializer and I am not sure if it's ok
because it is only supported by ANSI
if (remote_is_branch
!strcmp(local, shortname)
@@ -75,31 +108,26 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
git_config_set(key.buf, true);
}
strbuf_release(key);
-
if (flag BRANCH_CONFIG_VERBOSE) {
- if (remote_is_branch origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track remote branch %s
from %s by rebasing.) :
- _(Branch %s set up to track remote branch %s
from %s.),
- local, shortname, origin);
- else if (remote_is_branch !origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track local branch %s
by rebasing.) :
- _(Branch %s set up to track local branch
%s.),
- local, shortname);
- else if (!remote_is_branch origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track remote ref %s by
rebasing.) :
- _(Branch %s set up to track remote ref %s.),
- local, remote);
- else if (!remote_is_branch !origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track local ref %s by
rebasing.) :
- _(Branch %s set up to track local ref %s.),
- local, remote);
- else
+ for (i=0;isize;i++)
+ {
+ if (print_list[i].b_rebasing == (rebasing? 1 : 0)
+
print_list[i].b_remote_is_branch == (remote_is_branch? 1 : 0)
+
print_list[i].b_origin == (origin? 1 : 0))
Re: [PATCH v4] install_branch_config: simplify verbose messages logic
On Thu, Mar 13, 2014 at 2:34 PM, Junio C Hamano gits...@pobox.com wrote:
Eric Sunshine sunsh...@sunshineco.com writes:
Shouldn't this logic [to decide what the printf arguments should
be] also be encoded in the table?
...
The same argument also applies to computation of the 'name' variable
above. It too can be pushed into the the table.
Because the printf argument logic does not have to be in the
table, the same argument does not apply to the 'name' thing.
After looking at the v5 patch, I do not think an extra two-element
array to switch between remote vs shortname is making it any easier
to read. I would have to say that personally I find that
const char *name[] = {remote, shortname};
... long swath of code ...
printf_ln(... name[!remote_is_branch] ...);
is a lot harder to read than:
printf_ln(... remote_is_branch ? shortname : branch ...);
Indeed, that's a step backward, and is not what was asked. Merely
pushing data into tables does not make the logic table-driven
(emphasis on *driven*). The GSoC microproject did not demand a
table-driven approach, but instead asked students if such an approach
would make sense. A more table-driven approach might look something
like this:
struct M { const char *s; const char **a1; const char **a2; }
message[][2][2] = {{{
{ Branch %s set ... %s ... %s, shortname, origin },
...
}},{{
{ Branch %s set ... %s, remote, NULL },
...
}}};
const struct M *m = message[!remote_is_branch][!origin][!rebasing];
printf_ln(m-s, local, *m-a1, *m-a2);
Whether this approach is more clear than the original code is a matter
for debate [1], however, it's obvious from the table which arguments
belong with each message, and the printf_ln() invocation does not
require any logic. When moving only messages into a table, they become
disconnected from their arguments which makes reasoning about them a
bit more difficult. The original code does not have this problem, nor
does a table-driven approach.
[1]: While ungainly, the original code may not be sufficiently bad to
warrant the extra complications of a table. A simple refactoring, such
as [2], can make the code a bit easier to read without adding
complexity.
[2]: http://thread.gmane.org/gmane.comp.version-control.git/243704
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote
Eric Sunshine sunsh...@sunshineco.com writes: +Prepare a request to your upstream project to pull your changes to +their tree to the standard output, by summarizing your changes and +showing where your changes can be pulled from. Perhaps splitting this into two sentence (and using fewer to's) would make it a bit easier to grok? Something like: Generate a request asking your upstream project to pull changes into their tree. The request, printed to standard output, summarizes the changes and indicates from where they can be pulled. Thanks. +When the repository named by `url` has the commit at a tip of a +ref that is different from the ref you have it locally, you can use Did you want to drop it from this sentence? Or did you mean to say the ref as you have it locally? Thanks for your careful reading. Will drop it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: No progress from push when using bitmaps
On Wed, Mar 12, 2014 at 05:21:21PM -0700, Shawn Pearce wrote: Today I tried pushing a copy of linux.git from a client that had bitmaps into a JGit server. The client stalled for a long time with no progress, because it reused the existing pack. No progress appeared while it was sending the existing file on the wire: $ git push git://localhost/linux.git master Reusing existing pack: 2938117, done. Total 2938117 (delta 0), reused 0 (delta 0) remote: Resolving deltas: 66% (1637269/2455727) This is not the best user experience. :-( Yeah, I agree that sucks. I hadn't noticed it, as I don't typically have my client repos bitmapped (and on fetch, the interesting progress is coming from the local index-pack). It would definitely be good to have throughput measurements while writing out the pack. However, I'm not sure we have anything useful to count. We know the total number of objects we're reusing, but we're not actually parsing the data; we're just blitting it out as a stream. I think the progress code will need some refactoring to handle a throughput-only case. -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: No progress from push when using bitmaps
On Thu, Mar 13, 2014 at 2:26 PM, Jeff King p...@peff.net wrote: On Wed, Mar 12, 2014 at 05:21:21PM -0700, Shawn Pearce wrote: Today I tried pushing a copy of linux.git from a client that had bitmaps into a JGit server. The client stalled for a long time with no progress, because it reused the existing pack. No progress appeared while it was sending the existing file on the wire: $ git push git://localhost/linux.git master Reusing existing pack: 2938117, done. Total 2938117 (delta 0), reused 0 (delta 0) remote: Resolving deltas: 66% (1637269/2455727) This is not the best user experience. :-( Yeah, I agree that sucks. I hadn't noticed it, as I don't typically have my client repos bitmapped (and on fetch, the interesting progress is coming from the local index-pack). It would definitely be good to have throughput measurements while writing out the pack. However, I'm not sure we have anything useful to count. We know the total number of objects we're reusing, but we're not actually parsing the data; we're just blitting it out as a stream. I think the progress code will need some refactoring to handle a throughput-only case. Yes. I think JGit suffers from this same bug, and again we never noticed it because usually only the servers are bitmapped, not the clients. pack-objects writes a throughput meter when its writing objects. Really just the bytes out/second would be enough to let the user know the client is working. Unfortunately I think that is still tied to the overall progress system having some other counter? -- To unsubscribe from this list: send the line 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: No progress from push when using bitmaps
On Thu, Mar 13, 2014 at 03:01:09PM -0700, Shawn Pearce wrote: It would definitely be good to have throughput measurements while writing out the pack. However, I'm not sure we have anything useful to count. We know the total number of objects we're reusing, but we're not actually parsing the data; we're just blitting it out as a stream. I think the progress code will need some refactoring to handle a throughput-only case. Yes. I think JGit suffers from this same bug, and again we never noticed it because usually only the servers are bitmapped, not the clients. pack-objects writes a throughput meter when its writing objects. Really just the bytes out/second would be enough to let the user know the client is working. Unfortunately I think that is still tied to the overall progress system having some other counter? Yes, I'm looking at it right now. The throughput meter is actually connected to the sha1fd output. So really we just need to call display_progress periodically as we loop through the data. It's a one-liner fix. _But_ it still looks ugly, because, as you mention, it's tied to the progress meter, which is counting up to N objects. So we basically sit there at 0, pumping data, and then after the pack is done, we can say we sent N. :) There are a few ways around this: 1. Add a new phase Writing packs which counts from 0 to 1. Even though it's more accurate, moving from 0 to 1 really isn't that useful (the throughput is, but the 0/1 just looks like noise). 2. Add a new phase Writing reused objects that counts from 0 bytes up to N bytes. This looks stupid, though, because we are repeating the current byte count both here and in the throughput. 3. Use the regular Writing objects progress, but fake the object count. We know we are writing M bytes with N objects. Bump the counter by 1 for every M/N bytes we write. The first two require some non-trivial surgery to the progress code. I am leaning towards the third. Not just because it's easy, but because I think it actually shows the most intuitive display. Yes, it's fudging the object numbers, but those are largely meaningless anyway (in fact, it makes them _better_ because now they're even, instead of getting 95% done and then hitting some blob that is as big as the rest of the repo combined). -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][GSOC2014] install_branch_config: change logical chain to lookup table
On Wed, Mar 12, 2014 at 5:24 PM, TamerTas tamer...@outlook.com wrote:
Signed-off-by: TamerTas tamer...@outlook.com
--
There should be three hyphens --- here but somehow you have only two
--. Since --- is detected automatically when the patch is applied,
this deviation can be problematic.
Thanks for the feedback. Comments below.
Thanks for the resubmission. Comments below. :-)
I've made the suggested changes [1] to patch [2]
Better. This is a more well-crafted submission.
but, since there are different number of format
specifiers, an if-else clause is necessary.
Removing the if-else chain completely doesn't seem to be possible.
So making the format table-driven seems to be like an optional change.
Explaining why you chose one approach over another is indeed good
etiquette, and can forestall questions which reviewers may otherwise
ask.
Note, however, that it is possible to move this logic into a table.
Clue: it is not an error to supply more arguments than there are %s's
in the format string. This is not saying that you must make it
table-driven, but perhaps it may alter the reasons you gave above for
rejecting it (assuming you still do).
[1]:
http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605444.html
[2]:
http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605407.html
--
branch.c | 44 +---
1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/branch.c b/branch.c
index 723a36b..1ccf30f 100644
--- a/branch.c
+++ b/branch.c
@@ -50,10 +50,25 @@ static int should_setup_rebase(const char *origin)
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
const char *shortname = remote + 11;
+ const char *setup_message[] = {
+ N_(Branch %s set up to track local ref %s.),
+ N_(Branch %s set up to track local branch %s.),
+ N_(Branch %s set up to track remote ref %s.),
+ N_(Branch %s set up to track remote branch %s from %s.),
+ N_(Branch %s set up to track local ref %s by rebasing.)
+ N_(Branch %s set up to track local branch %s by rebasing.),
+ N_(Branch %s set up to track remote ref %s by rebasing.),
+ N_(Branch %s set up to track remote branch %s from %s by
rebasing.),
Some compilers will warn about the trailing comma, so you might want to drop it.
+ };
+
int remote_is_branch = starts_with(remote, refs/heads/);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
+ int msg_index = (!!remote_is_branch 0) +
+ (!!origin 1) +
+ (!!rebasing 2);
Better than the last (buggy) attempt. Nevertheless, it's a fairly
magical incantation requiring more thought than some other approaches.
Have you considered instead using a multi-dimensional array for the
messages and then indexing into it with these variables as direct keys
(after using ! or !! to constrain them to 0 or 1)? Would that be
better or worse?
if (remote_is_branch
!strcmp(local, shortname)
!origin) {
@@ -77,29 +92,12 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
strbuf_release(key);
if (flag BRANCH_CONFIG_VERBOSE) {
- if (remote_is_branch origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track remote branch
%s from %s by rebasing.) :
- _(Branch %s set up to track remote branch
%s from %s.),
- local, shortname, origin);
- else if (remote_is_branch !origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track local branch
%s by rebasing.) :
- _(Branch %s set up to track local branch
%s.),
- local, shortname);
- else if (!remote_is_branch origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track remote ref %s
by rebasing.) :
- _(Branch %s set up to track remote ref
%s.),
- local, remote);
- else if (!remote_is_branch !origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track local ref %s
by rebasing.) :
- _(Branch %s set up to track local ref
%s.),
- local, remote);
- else
- die(BUG: impossible combination of %d and %p,
- remote_is_branch,
Re: No progress from push when using bitmaps
Jeff King p...@peff.net writes: There are a few ways around this: 1. Add a new phase Writing packs which counts from 0 to 1. Even though it's more accurate, moving from 0 to 1 really isn't that useful (the throughput is, but the 0/1 just looks like noise). 2. Add a new phase Writing reused objects that counts from 0 bytes up to N bytes. This looks stupid, though, because we are repeating the current byte count both here and in the throughput. 3. Use the regular Writing objects progress, but fake the object count. We know we are writing M bytes with N objects. Bump the counter by 1 for every M/N bytes we write. The first two require some non-trivial surgery to the progress code. I am leaning towards the third. Not just because it's easy, but because I think it actually shows the most intuitive display. Yes, it's fudging the object numbers, but those are largely meaningless anyway (in fact, it makes them _better_ because now they're even, instead of getting 95% done and then hitting some blob that is as big as the rest of the repo combined). I think the above argument, especially the fudging but largely meaningless anyway part, makes perfect sense. Thanks for looking into this. -- To unsubscribe from this list: send the line 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: No progress from push when using bitmaps
On Thu, Mar 13, 2014 at 06:07:54PM -0400, Jeff King wrote:
3. Use the regular Writing objects progress, but fake the object
count. We know we are writing M bytes with N objects. Bump the
counter by 1 for every M/N bytes we write.
Here is that strategy. I think it looks pretty nice, and it seamlessly
handles the case where you have extra objects to send on top of the
reused pack (we just keep the same progress meter counting up).
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 831dd05..f187859 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -709,7 +709,7 @@ static struct object_entry **compute_write_order(void)
static off_t write_reused_pack(struct sha1file *f)
{
unsigned char buffer[8192];
- off_t to_write;
+ off_t to_write, total;
int fd;
if (!is_pack_valid(reuse_packfile))
@@ -726,7 +726,7 @@ static off_t write_reused_pack(struct sha1file *f)
if (reuse_packfile_offset 0)
reuse_packfile_offset = reuse_packfile-pack_size - 20;
- to_write = reuse_packfile_offset - sizeof(struct pack_header);
+ total = to_write = reuse_packfile_offset - sizeof(struct pack_header);
while (to_write) {
int read_pack = xread(fd, buffer, sizeof(buffer));
@@ -739,10 +739,23 @@ static off_t write_reused_pack(struct sha1file *f)
sha1write(f, buffer, read_pack);
to_write -= read_pack;
+
+ /*
+* We don't know the actual number of objects written,
+* only how many bytes written, how many bytes total, and
+* how many objects total. So we can fake it by pretending all
+* objects we are writing are the same size. This gives us a
+* smooth progress meter, and at the end it matches the true
+* answer.
+*/
+ written = reuse_packfile_objects *
+ (((double)(total - to_write)) / total);
+ display_progress(progress_state, written);
}
close(fd);
- written += reuse_packfile_objects;
+ written = reuse_packfile_objects;
+ display_progress(progress_state, written);
return reuse_packfile_offset - sizeof(struct pack_header);
}
--
To unsubscribe from this list: send the line 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] install_branch_config: simplify verbose messages logic
On Wed, Mar 12, 2014 at 7:47 PM, Paweł Wawruch pa...@aleg.pl wrote:
Replace the chain of if statements with table of strings.
Signed-off-by: Paweł Wawruch pa...@aleg.pl
---
Thanks to Eric Sunshine and Junio C Hamano.
Simplified printing logic. The name moved to a table.
v4: http://thread.gmane.org/gmane.comp.version-control.git/243914
v3: http://thread.gmane.org/gmane.comp.version-control.git/243865
v2: http://thread.gmane.org/gmane.comp.version-control.git/243849
v1: http://thread.gmane.org/gmane.comp.version-control.git/243502
Thanks. Using the vN notation makes these links self-explanatory.
branch.c | 42 +-
1 file changed, 17 insertions(+), 25 deletions(-)
diff --git a/branch.c b/branch.c
index 723a36b..c17817c 100644
--- a/branch.c
+++ b/branch.c
@@ -53,6 +53,20 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
int remote_is_branch = starts_with(remote, refs/heads/);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
+ const char *message[][2][2] = {{{
+ N_(Branch %s set up to track remote branch %s from %s by
rebasing.),
+ N_(Branch %s set up to track remote branch %s from %s.),
Some compilers may complain about the trailing comma. Ditto for the ones below.
+ },{
+ N_(Branch %s set up to track local branch %s by rebasing.),
+ N_(Branch %s set up to track local branch %s.),
+ }},{{
+ N_(Branch %s set up to track remote ref %s by rebasing.),
+ N_(Branch %s set up to track remote ref %s.),
+ },{
+ N_(Branch %s set up to track local ref %s by rebasing.),
+ N_(Branch %s set up to track local ref %s.)
+ }}};
+ const char *name[] = {remote, shortname};
See [1] and the messages following it for commentary about this change.
if (remote_is_branch
!strcmp(local, shortname)
@@ -76,31 +90,9 @@ void install_branch_config(int flag, const char *local,
const char *origin, cons
}
strbuf_release(key);
- if (flag BRANCH_CONFIG_VERBOSE) {
- if (remote_is_branch origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track remote branch
%s from %s by rebasing.) :
- _(Branch %s set up to track remote branch
%s from %s.),
- local, shortname, origin);
- else if (remote_is_branch !origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track local branch
%s by rebasing.) :
- _(Branch %s set up to track local branch
%s.),
- local, shortname);
- else if (!remote_is_branch origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track remote ref %s
by rebasing.) :
- _(Branch %s set up to track remote ref
%s.),
- local, remote);
- else if (!remote_is_branch !origin)
- printf_ln(rebasing ?
- _(Branch %s set up to track local ref %s
by rebasing.) :
- _(Branch %s set up to track local ref
%s.),
- local, remote);
- else
- die(BUG: impossible combination of %d and %p,
- remote_is_branch, origin);
- }
+ if (flag BRANCH_CONFIG_VERBOSE)
+ printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
+ local, name[!remote_is_branch], origin);
Much simpler than the previous versions (sans [1] commentary)
[1]: http://thread.gmane.org/gmane.comp.version-control.git/243914/focus=244043
}
/*
--
1.8.3.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] install_branch_config: simplify verbose messages logic
On Thu, Mar 13, 2014 at 4:35 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
A more table-driven approach might look something
like this:
struct M { const char *s; const char **a1; const char **a2; }
message[][2][2] = {{{
{ Branch %s set ... %s ... %s, shortname, origin },
...
}},{{
{ Branch %s set ... %s, remote, NULL },
...
}}};
const struct M *m = message[!remote_is_branch][!origin][!rebasing];
printf_ln(m-s, local, *m-a1, *m-a2);
Of course, using NULL in the table like that would crash when
dereferenced: *m-a2. It was just an quick example typed on-the-fly.
Real code would want to be more careful.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Proposal: Write git subtree info to .git/config
Has there been any talk about adding a stub for git subtrees in .git/config? The primary benefits would be: 1. Determine what sub directories of the project were at one time pulled from another repo (where from and which commit id), without having to attempt to infer this by scanning the log. 2. Simplify command syntax by providing a predictable default (ie. last pulled from, last pushed to), and not requiring the repo argument optional. 3. Improvement for default commit id to start split operations over using --rejoin which creates blank log entries just so the log scan can find it (afaict). It's a default either way, so it can still always be explicitly specified. If this information were available in the config, I think additional features could be added as well: - The command 'git subtree pull' for instance could be made to pull *all* subtrees, similar to the way 'git submodule update' works. - An option -i (interactive), or -p (prompt), etc. could be added that confirms the defaults read from the config before actually executing the command with implicit arguments, and the ability to modify the arguments before the command actually executes. - If the current working directory from which the command is run happens to be a subtree specified in the config, the --prefix could even be implied. None of these ideas would break the way the command currently works since it can still always take explicit arguments. There's a comment in the documentation about the command that says: Unlike submodules, subtrees do not need any special constructions (like .gitmodule files or gitlinks) be present in your repository It would still be true that subtrees do not *need* any special config settings, but that doesn't mean they are bad, and by having them the command could be improved and made easier to use. I'm happy to contribute the changes myself if this proposal is acceptable. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Microproject idea: new OPT_* macros for PARSE_OPT_NONEG
On Fri, Mar 14, 2014 at 2:00 AM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: On Sat, Mar 8, 2014 at 2:20 AM, Junio C Hamano gits...@pobox.com wrote: Looking at git grep -B3 OPT_NONEG output, it seems that NONEG is associated mostly with OPTION_CALLBACK and OPTION_SET_INT in the existing code. Perhaps OPT_SET_INT should default to not just OPT_NOARG but also OPT_NONEG? There are OPT_SET_INT() that should not have NONEG in current code. So there are two sets of SET_INT anyway. Either we convert them all to a new macro that takes an extra flag, or we add OPT_SET_INT_NONEG() that covers one set and leave the other set alone. Are you forgetting the third alternative, of swapping the default, if the ones that do not want NONEG are in the minority, to reduce the number of spelled-out instances? There are 12 SET_INT with NONEG and 81 without (though I suspect some of them should have NONEG). So NONEG is not exactly the majority. And swapping does not go well with git development style, some in-flight topics may introduce new OPT_SET_INT() that uses the old behavior. -- 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: Proposal: Write git subtree info to .git/config
John Butterfield johnb...@gmail.com writes: Has there been any talk about adding a stub for git subtrees in .git/config? I do not think so, and that is probably for a good reason. A subtree biding can change over time, but .git/config is about recording information that do not change depending on what tree you are looking at, so there is an impedance mismatch---storing that information in .git/config is probably a wrong way to go about it. It might help to keep track of In this tree, the tip of that other history is bound as a subtree at this path, which means that information more naturally belongs to each tree, I would think. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Corner case bug caused by shell dependent behavior
Hi
When your system shell (/bin/sh) is a dash control sequences in
strings get interpreted by the echo command. A commit message
which ends with the string '\n' may result in a garbage line in
the todo list of an interactive rebase which causes the rebase
to fail.
To reproduce the behavior (with dash as /bin/sh):
mkdir test cd test git init
echo 1 foo git add foo
git commit -mthis commit message ends with '\n'
echo 2 foo git commit -a --fixup HEAD
git rebase -i --autosquash --root
Now the editor opens with garbage in line 3 which has to be
removed or the rebase fails.
The attached one-line patch fixes the bug. Be free to edit the
commit message when it's too long.
Maybe there are more places where it would be more robust to use
printf instead of echo.
Uwe
From 53262bc8a7a3ec9d9a6b0e8ecaaea598257b87fe Mon Sep 17 00:00:00 2001
From: Uwe Storbeck u...@ibr.ch
Date: Fri, 14 Mar 2014 00:28:33 +0100
Subject: [PATCH] git-rebase--interactive: replace echo by printf
to avoid shell dependent behavior.
When your system shell (/bin/sh) is a dash control sequences in
strings get interpreted by the echo command. A commit message
which ends with the string '\n' may result in a garbage line in
the todo list of an interactive rebase which causes the rebase
to fail.
To reproduce the behavior (with dash as /bin/sh):
mkdir test cd test git init
echo 1 foo git add foo
git commit -mthis commit message ends with '\n'
echo 2 foo git commit -a --fixup HEAD
git rebase -i --autosquash --root
Now the editor opens with garbage in line 3 which has to be
removed or the rebase fails.
---
git-rebase--interactive.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a1adae8..3ffe14c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -749,7 +749,7 @@ rearrange_squash () {
;;
esac
done
- echo $sha1 $action $prefix $rest
+ printf %s %s %s %s\n $sha1 $action $prefix $rest
# if it's a single word, try to resolve to a full sha1 and
# emit a second copy. This allows us to match on both message
# and on sha1 prefix
--
1.9.0
Re: Corner case bug caused by shell dependent behavior
Hi, Uwe Storbeck wrote: To reproduce the behavior (with dash as /bin/sh): mkdir test cd test git init echo 1 foo git add foo git commit -mthis commit message ends with '\n' echo 2 foo git commit -a --fixup HEAD git rebase -i --autosquash --root Now the editor opens with garbage in line 3 which has to be removed or the rebase fails. Would it make sense to add this as a test to e.g. t/t3404-rebase-interactive.sh? The attached one-line patch fixes the bug. May we have your sign-off? (See Documentation/SubmittingPatches section Sign your work for what this means. Looks obviously correct, so for what it's worth, Reviewed-by: Jonathan Nieder jrnie...@gmail.com 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: Proposal: Write git subtree info to .git/config
A subtree biding can change over time, but .git/config is about recording information that do not change depending on what tree you are looking at, so there is an impedance mismatch---storing that information in .git/config is probably a wrong way to go about it. I see. How about a .gitsubtrees config file in the root of a project? It might help to keep track of In this tree, the tip of that other history is bound as a subtree at this path, which means that information more naturally belongs to each tree, I would think. Anything in the subdirectory must be part of the contents of the subtree repo. It should not know how it is linked to it's parent project; parents should know how their children are fetched. Therefore it cannot live in the subtree. Subtrees could be nested. So, should the config be in the root of the parent subtree? This makes sense to me. Example: / A/ B/# a subtree of (blah) X/ Y/ # a subtree of (yada-yada) Z/ So, lets say B has many updates remotely, including pushing and pulling changes to Y. When pulling the changes from B, it would be convenient for it to come with the meta data, (subtree repo and commit info) for Y. So how does that sound; Could we store subtree repo and commit id references per folder in a .gitsubtrees file in the root of every project? (Project B is technically it's own project so it would pull it's own .gitsubtrees in /B/.gitsubtrees) `John On Thu, Mar 13, 2014 at 4:36 PM, Junio C Hamano gits...@pobox.com wrote: John Butterfield johnb...@gmail.com writes: Has there been any talk about adding a stub for git subtrees in .git/config? I do not think so, and that is probably for a good reason. A subtree biding can change over time, but .git/config is about recording information that do not change depending on what tree you are looking at, so there is an impedance mismatch---storing that information in .git/config is probably a wrong way to go about it. It might help to keep track of In this tree, the tip of that other history is bound as a subtree at this path, which means that information more naturally belongs to each tree, I would think. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Proposal: Write git subtree info to .git/config
by per folder I meant, for each subtree On Thu, Mar 13, 2014 at 5:43 PM, John Butterfield johnb...@gmail.com wrote: A subtree biding can change over time, but .git/config is about recording information that do not change depending on what tree you are looking at, so there is an impedance mismatch---storing that information in .git/config is probably a wrong way to go about it. I see. How about a .gitsubtrees config file in the root of a project? It might help to keep track of In this tree, the tip of that other history is bound as a subtree at this path, which means that information more naturally belongs to each tree, I would think. Anything in the subdirectory must be part of the contents of the subtree repo. It should not know how it is linked to it's parent project; parents should know how their children are fetched. Therefore it cannot live in the subtree. Subtrees could be nested. So, should the config be in the root of the parent subtree? This makes sense to me. Example: / A/ B/# a subtree of (blah) X/ Y/ # a subtree of (yada-yada) Z/ So, lets say B has many updates remotely, including pushing and pulling changes to Y. When pulling the changes from B, it would be convenient for it to come with the meta data, (subtree repo and commit info) for Y. So how does that sound; Could we store subtree repo and commit id references per folder in a .gitsubtrees file in the root of every project? (Project B is technically it's own project so it would pull it's own .gitsubtrees in /B/.gitsubtrees) `John On Thu, Mar 13, 2014 at 4:36 PM, Junio C Hamano gits...@pobox.com wrote: John Butterfield johnb...@gmail.com writes: Has there been any talk about adding a stub for git subtrees in .git/config? I do not think so, and that is probably for a good reason. A subtree biding can change over time, but .git/config is about recording information that do not change depending on what tree you are looking at, so there is an impedance mismatch---storing that information in .git/config is probably a wrong way to go about it. It might help to keep track of In this tree, the tip of that other history is bound as a subtree at this path, which means that information more naturally belongs to each tree, I would think. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] simplifying branch.c:install_branch_config() if()
Signed-off-by: Nemina Amarasinghe nemi...@gmail.com --- branch.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/branch.c b/branch.c index 723a36b..fd93603 100644 --- a/branch.c +++ b/branch.c @@ -87,16 +87,11 @@ void install_branch_config(int flag, const char *local, const char *origin, cons _(Branch %s set up to track local branch %s by rebasing.) : _(Branch %s set up to track local branch %s.), local, shortname); - else if (!remote_is_branch origin) + else if (!remote_is_branch) printf_ln(rebasing ? _(Branch %s set up to track remote ref %s by rebasing.) : _(Branch %s set up to track remote ref %s.), local, remote); - else if (!remote_is_branch !origin) - printf_ln(rebasing ? - _(Branch %s set up to track local ref %s by rebasing.) : - _(Branch %s set up to track local ref %s.), - local, remote); else die(BUG: impossible combination of %d and %p, remote_is_branch, origin); -- 1.9.0.152.g6ab4ae2 -- To unsubscribe from this list: send the line 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] simplifying branch.c:install_branch_config() if()
Hi, Nemina Amarasinghe wrote: Signed-off-by: Nemina Amarasinghe nemi...@gmail.com The above is a place to explain why this is a good change. For example: When it prints a message indicating what it has done, install_branch_config() treats the (!remote_is_branch origin) and (!remote_is_branch !origin) cases separately. But they are the same, and it uses the same message for both. Simplify by just checking for !remote_is_branch. Noticed using the magic-identical-branch-checker tool. Signed-off-by: ... (That's just a first random hypothetical guess --- of course please do not use it but put your own rationale for the change there.) [...] --- a/branch.c +++ b/branch.c @@ -87,16 +87,11 @@ void install_branch_config(int flag, const char *local, const char *origin, cons _(Branch %s set up to track local branch %s by rebasing.) : _(Branch %s set up to track local branch %s.), local, shortname); - else if (!remote_is_branch origin) + else if (!remote_is_branch) printf_ln(rebasing ? _(Branch %s set up to track remote ref %s by rebasing.) : _(Branch %s set up to track remote ref %s.), local, remote); - else if (!remote_is_branch !origin) - printf_ln(rebasing ? - _(Branch %s set up to track local ref %s by rebasing.) : - _(Branch %s set up to track local ref %s.), - local, remote); Is this safe? Today branch.c::create_branch checks that the argument to e.g. --set-upstream-to is either in refs/heads/* or the image of some remote, but what is making sure that remains true tomorrow? So if changing this, I would be happier if the if condition were still (!remote_is_branch origin) so the impossible case could emit a BUG. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] GSoC Change multiple if-else statements to be table-driven
Thanks for the resubmission. Comments below.
On Thu, Mar 13, 2014 at 4:20 PM, Yao Zhao zhaox...@umn.edu wrote:
Subject: [PATCH] GSoC Change multiple if-else statements to be table-driven
It's a good idea to let reviewers know that this is attempt 2. Do so
by saying [PATCH v2]. Your next one will be [PATCH v3]. The -v option
for git format-email can help.
When your patch is applied via git am, text inside [...] gets
stripped automatically. The GSoC tells email readers what this
submission is about, but isn't relevant to the actual commit message.
It should be placed inside [...]. For instance: [PATCH/GSoC v2].
Signed-off-by: Yao Zhao zhaox...@umn.edu
---
GSoC_MicroProject_#8
Hello Eric,
Thanks for reviewing my code. I implemented table-driven method this time and
correct the style
problems indicated in review.
Explaining what you changed since the last version is indeed good
etiquette. Thanks. For bonus points, also provide a link to the
previous version, like this [1].
[1]: http://thread.gmane.org/gmane.comp.version-control.git/243919
branch.c | 72
1 file changed, 50 insertions(+), 22 deletions(-)
diff --git a/branch.c b/branch.c
index 723a36b..6451c99 100644
--- a/branch.c
+++ b/branch.c
@@ -49,10 +49,43 @@ static int should_setup_rebase(const char *origin)
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
+
Unnecessary insertion of blank line.
const char *shortname = remote + 11;
int remote_is_branch = starts_with(remote, refs/heads/);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
+ int size=8, i;
Style: whitespace: size = 8
You can use ARRAY_SIZE(print_list) to avoid hardcoding 8 (and then you
don't need the variable 'size').
+ typedef struct PRINT_LIST {
+ const char *print_str;
+ const char *arg2;
+ //arg1 is always local, so I only add arg2 and arg3 in struct
This commentary should be placed below the --- under your sign-off
(or dropped altogether since it's pretty obvious).
Also, in this project avoid //-style comments.
+ const char *arg3;
+ int b_rebasing;
+ int b_remote_is_branch;
Strange indentation. Use tabs for indentation, and set your editor so
tabs have width 8.
+ int b_origin;
+ } PRINT_LIST;
Read below for some commentary about b_rebasing, b_remote_is_branch, b_origin.
+ PRINT_LIST print_list[] = {
+ {.print_str = _(Branch %s set up to track remote branch %s
from %s by rebasing.),
+ .arg2 = shortname, .arg3 = origin,
+.b_rebasing = 1, .b_remote_is_branch
= 1, .b_origin = 1},
+ {.print_str = _(Branch %s set up to track remote branch %s from
%s.),
+ .arg2 = shortname, .arg3 = origin,
+.b_rebasing = 0, .b_remote_is_branch
= 1, .b_origin = 1},
+{.print_str = _(Branch %s set up to track local branch %s by
rebasing.),
+ .arg2 = shortname, .b_rebasing = 1,
.b_remote_is_branch = 1, .b_origin = 0},
+ {.print_str = _(Branch %s set up to track local branch %s.),
+ .arg2 = shortname, .b_rebasing = 0,
.b_remote_is_branch = 1, .b_origin = 0},
+ {.print_str = _(Branch %s set up to track remote ref %s by
rebasing.),
+ .arg2 = remote, .b_rebasing = 1,
.b_remote_is_branch = 0, .b_origin = 1},
+ {.print_str = _(Branch %s set up to track remote ref %s.),
+ .arg2 = remote, .b_rebasing = 0,
.b_remote_is_branch = 0, .b_origin = 1},
+ {.print_str = _(Branch %s set up to track local ref %s by
rebasing.),
+ .arg2 = remote, .b_rebasing = 1,
.b_remote_is_branch = 0, .b_origin = 0},
+ {.print_str = _(Branch %s set up to track local ref %s.),
+ .arg2 = remote, .b_rebasing = 0,
.b_remote_is_branch = 0, .b_origin = 0},
+};
I am confused here: I use struct initializer and I am not sure if it's ok
because it is only supported by ANSI
This commentary should be placed below --- after your sign-off.
Indeed, you want to avoid named field initializers in this project and
instead use positional initializers.
Translatable strings in an initializer should be wrapped with N_()
instead of _(). You will still need to use _() later on when you
reference the string from the table. See section 4.7 [2] of the GNU
gettext manual for details.
[2]: http://www.gnu.org/software/gettext/manual/gettext.html#Special-cases
if (remote_is_branch
!strcmp(local, shortname)
@@ -75,31 +108,26 @@ void
Re: Corner case bug caused by shell dependent behavior
On Fri, Mar 14, 2014 at 01:02:13AM +0100, Uwe Storbeck wrote: When your system shell (/bin/sh) is a dash control sequences in strings get interpreted by the echo command. A commit message which ends with the string '\n' may result in a garbage line in the todo list of an interactive rebase which causes the rebase to fail. To reproduce the behavior (with dash as /bin/sh): mkdir test cd test git init echo 1 foo git add foo git commit -mthis commit message ends with '\n' echo 2 foo git commit -a --fixup HEAD git rebase -i --autosquash --root Hmph. We ran into this before and fixed all of the sites (e.g., d1c3b10 and 938791c). This one appears to have been added a few months later (by 68d5d03). Maybe there are more places where it would be more robust to use printf instead of echo. FWIW, I just looked through the other uses of echo in git-rebase*.sh, and I think this is the only problematic case. - echo $sha1 $action $prefix $rest + printf %s %s %s %s\n $sha1 $action $prefix $rest Looks obviously correct. The echo just below here does not need the same treatment, as $rest is the problematic bit ($prefix is always fixup or squash). -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: [RFC/PATCH] diff: simplify cpp funcname regex
On Fri, Mar 07, 2014 at 08:23:05AM +0100, Johannes Sixt wrote:
No, I meant lines like
static double var;
-static int old;
+static int new;
The motivation is to show hints where in a file the change is located:
Anything that could be of significance for the author should be picked up.
I see. Coupled with what you said below:
As I said, my motivation is not so much to find a container, but rather
a clue to help locate a change while reading the patch text. I can speak
for myself, but I have no idea what is more important for the majority.
your proposal makes a lot more sense to me, and I think that is really
at the center of our discussion. I do not have a gut feeling for which
is more right (i.e., container versus context).
But given that most of the cases we are discussing are ones where the
diff -p default regex gets it right (or at least better than) compared
to the C regex, I am tempted to say that we should be erring in the
direction of simplicity, and just finding interesting lines without
worrying about containers (i.e., it argues for your patch).
Makes sense. I noticed your fix is to look for end-of-line or comments
afterwards. Would it be simpler to just check for a non-colon, like:
!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:($|[^:])
I want to match [[:space:]] after the label's colon, because I have lot's
of C++ files with CRLF, and I need to match the CR. Your more liberal
pattern would fit as well, although it would pick up a bit field as in
struct foo {
unsigned
flag: 1;
-old
+new
Thanks, I was having trouble thinking of another good use of a colon,
but bitfields are what I was missing. Your pattern is probably better
here.
So I am leaning towards your patch, but I'm not sure if I understand all
of the implications for git grep. Can you give some concrete examples?
-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] simplifying branch.c:install_branch_config() if()
Is this safe? Today branch.c::create_branch checks that the argument to e.g. --set-upstream-to is either in refs/heads/* or the image of some remote, but what is making sure that remains true tomorrow? So if changing this, I would be happier if the if condition were still (!remote_is_branch origin) so the impossible case could emit a BUG. Hope that helps, Jonathan Thanks for the coments Jonathan, Yes you are correct... I was just thought about how to simplify this chain of if() statement. Not the big picture. Now I understand when I change or implenet something I have to think not only about the current matter but abot the future also. Nemina -- To unsubscribe from this list: send the line 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] Git Configuration API improvements
On Tue, Mar 11, 2014 at 09:49:33PM +0530, karthik nayak wrote: On Tue, Mar 11, 2014 at 8:21 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: karthik nayak karthik@gmail.com writes: Currently we have multiple invocation of git_config() in an individual invocation of git() which is not efficient. Also, it is hard to implement new features, I think efficiency is not a real concern here. Config files are small and easy to parse, so parsing them multiple times isn't really noticeable from the performance point of view. OTOH, the extensibility is a real concern, and that should be the main motivation for the project. Thanks. I understand what you mean. extensibility is the main motivation of the project, i think that by implementing the cache system we can fix the small problems (reappearing of headers while setting and unsetting configs) and also implement new features like to unset a config easily. I think the most interesting part of the config idea is turning the fetching of config variables inside out. That is, right now we turn control over to the config code, which invokes our callbacks. So we see all variables in sequence, and pick out the ones that are interesting. We implement precedence with a last one wins technique where we keep overwriting a variable with subsequent config options. This can lead to difficult ordering situations, such as when a config option _might_ be respected based on another factor (e.g., the presence of a command line option, as in the status.branch option). It also means that it's impossible to tell after the fact whether a value was set explicitly on the command line, by config, or if we are using a system default. Most of the time this doesn't matter, but there are a few cases where we care, and we end up having to manually manage a separate flag in each case. By the phrase inside out above, I mean that we could read all config, and then fetch individual values as we need them. We do need to care about efficiency here, but only insofar as we don't cause any regressions (that is, the current system is fine speed-wise, we just need to make sure that we do not create new problems by calling a slow lookup in a tight loop or anything like that). -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: GSoC proposal: port pack bitmap support to libgit2.
On Wed, Mar 12, 2014 at 04:19:23PM +0800, Yuxuan Shui wrote: I'm Yuxuan Shui, a undergraduate student from China. I'm applying for GSoC 2014, and here is my proposal: I found this idea on the ideas page, and did some research about it. The pack bitmap patchset add a new .bitmap file for every pack file which contains the reachability information of selected commits. This information is used to speed up git fetching and cloning, and produce a very convincing results. The goal of my project is to port the pack bitmap implementation in core git to libgit2, so users of libgit2 could benefit from this optimization as well. Please let me know if my proposal makes sense, thanks. You'd want to flesh it out a bit more to show how you're thinking about tackling the problem: - What are the areas of libgit2 that you will need to touch? Be specific. What's the current state of the packing code? What files and functions will you need to touch? - What are the challenges you expect to encounter in porting the code? - Can you give a detailed schedule of the summer's work? What will you work on in each week? What milestones do you expect to hit, and when? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] wt-status: Make status messages more consistent with others
This is mainly changing messages that say:
run git foo --bar
to
use git foo --bar to baz
Although the commands and flags are usually self-explanatory, this is
more consistent with other status messages, and gives some sort of
explanation to the user.
Signed-off-by: Andrew Wong andrew.k...@gmail.com
---
wt-status.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index a452407..9f2358a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -899,13 +899,13 @@ static void show_merge_in_progress(struct wt_status *s,
status_printf_ln(s, color, _(You have unmerged paths.));
if (s-hints)
status_printf_ln(s, color,
- _( (fix conflicts and run \git commit\)));
+ _( (fix conflicts and use \git commit\ to
conclude the merge)));
} else {
status_printf_ln(s, color,
_(All conflicts fixed but you are still merging.));
if (s-hints)
status_printf_ln(s, color,
- _( (use \git commit\ to conclude merge)));
+ _( (use \git commit\ to conclude the
merge)));
}
wt_status_print_trailer(s);
}
@@ -922,7 +922,7 @@ static void show_am_in_progress(struct wt_status *s,
if (s-hints) {
if (!state-am_empty_patch)
status_printf_ln(s, color,
- _( (fix conflicts and then run \git am
--continue\)));
+ _( (fix conflicts and then use \git am
--continue\ to continue)));
status_printf_ln(s, color,
_( (use \git am --skip\ to skip this patch)));
status_printf_ln(s, color,
@@ -994,7 +994,7 @@ static void show_rebase_in_progress(struct wt_status *s,
_(You are currently rebasing.));
if (s-hints) {
status_printf_ln(s, color,
- _( (fix conflicts and then run \git rebase
--continue\)));
+ _( (fix conflicts and then use \git rebase
--continue\ to continue)));
status_printf_ln(s, color,
_( (use \git rebase --skip\ to skip this
patch)));
status_printf_ln(s, color,
@@ -1011,7 +1011,7 @@ static void show_rebase_in_progress(struct wt_status *s,
_(You are currently rebasing.));
if (s-hints)
status_printf_ln(s, color,
- _( (all conflicts fixed: run \git rebase
--continue\)));
+ _( (all conflicts fixed: use \git rebase
--continue\ to continue)));
} else if (split_commit_in_progress(s)) {
if (state-branch)
status_printf_ln(s, color,
@@ -1023,7 +1023,7 @@ static void show_rebase_in_progress(struct wt_status *s,
_(You are currently splitting a
commit during a rebase.));
if (s-hints)
status_printf_ln(s, color,
- _( (Once your working directory is clean, run
\git rebase --continue\)));
+ _( (Once your working directory is clean, use
\git rebase --continue\ to continue)));
} else {
if (state-branch)
status_printf_ln(s, color,
@@ -1052,10 +1052,10 @@ static void show_cherry_pick_in_progress(struct
wt_status *s,
if (s-hints) {
if (has_unmerged(s))
status_printf_ln(s, color,
- _( (fix conflicts and run \git cherry-pick
--continue\)));
+ _( (fix conflicts and use \git cherry-pick
--continue\ to continue)));
else
status_printf_ln(s, color,
- _( (all conflicts fixed: run \git
cherry-pick --continue\)));
+ _( (all conflicts fixed: use \git
cherry-pick --continue\ to continue)));
status_printf_ln(s, color,
_( (use \git cherry-pick --abort\ to cancel the
cherry-pick operation)));
}
@@ -1071,10 +1071,10 @@ static void show_revert_in_progress(struct wt_status *s,
if (s-hints) {
if (has_unmerged(s))
status_printf_ln(s, color,
- _( (fix conflicts and run \git revert
--continue\)));
+ _( (fix conflicts and use \git revert
--continue\ to continue)));
else
status_printf_ln(s, color,
- _( (all conflicts fixed: run \git revert
[PATCH 2/3] merge: Advise user to use git merge --abort to abort merges
Print message during git merge and git status.
Add a new mergeHints advice to silence these messages.
Signed-off-by: Andrew Wong andrew.k...@gmail.com
---
Documentation/config.txt | 3 +++
advice.c | 2 ++
advice.h | 1 +
builtin/merge.c | 6 ++
wt-status.c | 3 +++
5 files changed, 15 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 73904bc..936a20b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -196,6 +196,9 @@ advice.*::
rmHints::
In case of failure in the output of linkgit:git-rm[1],
show directions on how to proceed from the current state.
+ mergeHints::
+Show directions on how to proceed from the current state in the
+output of linkgit:git-merge[1].
--
core.fileMode::
diff --git a/advice.c b/advice.c
index 486f823..e910734 100644
--- a/advice.c
+++ b/advice.c
@@ -15,6 +15,7 @@ int advice_detached_head = 1;
int advice_set_upstream_failure = 1;
int advice_object_name_warning = 1;
int advice_rm_hints = 1;
+int advice_merge_hints = 1;
static struct {
const char *name;
@@ -35,6 +36,7 @@ static struct {
{ setupstreamfailure, advice_set_upstream_failure },
{ objectnamewarning, advice_object_name_warning },
{ rmhints, advice_rm_hints },
+ { mergehints, advice_merge_hints },
/* make this an alias for backward compatibility */
{ pushnonfastforward, advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index 5ecc6c1..d337f1c 100644
--- a/advice.h
+++ b/advice.h
@@ -18,6 +18,7 @@ extern int advice_detached_head;
extern int advice_set_upstream_failure;
extern int advice_object_name_warning;
extern int advice_rm_hints;
+extern int advice_merge_hints;
int git_default_advice_config(const char *var, const char *value);
__attribute__((format (printf, 1, 2)))
diff --git a/builtin/merge.c b/builtin/merge.c
index f0cf120..c55ac03 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -805,6 +805,8 @@ static void abort_commit(struct commit_list *remoteheads,
const char *err_msg)
error(%s, err_msg);
fprintf(stderr,
_(Not committing merge; use 'git commit' to complete the
merge.\n));
+ if (advice_merge_hints)
+ printf(_( (use \git merge --abort\ to abort the merge)\n));
write_merge_state(remoteheads);
exit(1);
}
@@ -913,6 +915,8 @@ static int suggest_conflicts(int renormalizing)
rerere(allow_rerere_auto);
printf(_(Automatic merge failed;
fix conflicts and then commit the result.\n));
+ if (advice_merge_hints)
+ printf(_( (use \git merge --abort\ to abort the merge)\n));
return 1;
}
@@ -1559,6 +1563,8 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
if (merge_was_ok)
fprintf(stderr, _(Automatic merge went well;
stopped before committing as requested\n));
+ if (advice_merge_hints)
+ printf(_( (use \git merge --abort\ to abort the merge)\n));
else
ret = suggest_conflicts(option_renormalize);
diff --git a/wt-status.c b/wt-status.c
index 9f2358a..3b30bf9 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -907,6 +907,9 @@ static void show_merge_in_progress(struct wt_status *s,
status_printf_ln(s, color,
_( (use \git commit\ to conclude the
merge)));
}
+ if (s-hints)
+ status_printf_ln(s, color,
+ _( (use \git merge --abort\ to abort the merge)));
wt_status_print_trailer(s);
}
--
1.9.0.174.g6f75b8f
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] reset: Print a warning when user uses git reset during a merge
During a merge, --mixed is most likely not what the user wants. Using
--mixed during a merge would leave the merged changes and new files
mixed in with the local changes. The user would have to manually clean
up the work tree, which is non-trivial. In future releases, we want to
make git reset error out when used in the middle of a merge. For now,
we simply print out a warning to the user.
Signed-off-by: Andrew Wong andrew.k...@gmail.com
---
builtin/reset.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/builtin/reset.c b/builtin/reset.c
index 4fd1c6c..04e8103 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -331,8 +331,29 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
_(reset_type_names[reset_type]));
}
if (reset_type == NONE)
+ {
reset_type = MIXED; /* by default */
+ /* During a merge, --mixed is most likely not what the user
+* wants. Using --mixed during a merge would leave the merged
+* changes and new files mixed in with the local changes. The
+* user would have to manually clean up the work tree, which is
+* non-trivial. In future releases, we want to make git reset
+* error out when used in the middle of a merge. For now, we
+* simply print out a warning to the user. */
+ if (is_merge())
+ warning(_(You have used 'git reset' in the middle of a
merge. 'git reset' defaults to\n
+ 'git reset --mixed', which means git will
not clean up any merged changes and\n
+ new files that were created in the work
tree. It also becomes impossible for\n
+ git to automatically clean up the work tree
later, so you would have to clean\n
+ up the work tree manually. To avoid this
next time, you may want to use 'git\n
+ reset --merge', or equivalently 'git merge
--abort'.\n
+ \n
+ In future releases, using 'git reset' in the
middle of a merge will result in\n
+ an error.
+));
+ }
+
if (reset_type != SOFT reset_type != MIXED)
setup_work_tree();
--
1.9.0.174.g6f75b8f
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] Make git more user-friendly during a merge conflict
2/3: I've added advice.mergeHints to silent the messages that suggests git merge--abort. 3/3: I've added a warning message when users used git reset during a merge. This warning will be printed if the user is in the middle of a merge. In future releases, we'll change this into an error to prevent work tree from becoming a mess. Andrew Wong (3): wt-status: Make status messages more consistent with others merge: Advise user to use git merge --abort to abort merges reset: Print a warning when user uses git reset during a merge Documentation/config.txt | 3 +++ advice.c | 2 ++ advice.h | 1 + builtin/merge.c | 6 ++ builtin/reset.c | 21 + wt-status.c | 23 +-- 6 files changed, 46 insertions(+), 10 deletions(-) -- 1.9.0.174.g6f75b8f -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC GSoC idea: new git config features
On Sat, Mar 01, 2014 at 12:01:44PM +0100, Matthieu Moy wrote:
Jeff King p...@peff.net writes:
If we had the keys in-memory, we could reverse this: config code asks
for keys it cares about, and we can do an optimized lookup (binary
search, hash, etc).
I'm actually dreaming of a system where a configuration variable could
be declared in Git's source code, with associated type (list/single
value, boolean/string/path/...), default value and documentation (and
then Documentation/config.txt could become a generated file). One could
imagine a lot of possibilities like
Yes, I think something like that would be very nice. I am not a big
fan of code generation, but if we had config queries like
config_get_bool, then I think it would be reasonably pleasant to take
a spec like:
Key: help.browser
Type: string
Description: Specify the browser for help...
and turn it into:
const char *config_get_help_browser(void)
{
return config_get_string(help.browser);
}
So technically code generation, but all the heavy lifting is done behind
the scenes. We're not saving lines in the result so much as avoiding
repeating ourselves (that is, the generated code is only mapping the
config-type from the spec into a C type and function name that gives us
extra compile-time safety).
However, I skimmed through config.txt looking for a key to use in my
example above, and there are a surprising number of one-off semantics
(e.g., things that are mostly bool, but can be auto or take some other
special value). We may find that the Type field has a surprising
number of variants that makes a technique like this annoying. But I'd
reserve judgement until somebody actually tries encoding a significant
chunk of the config keys and we see what it looks like.
Migrating the whole code to such system would take time, but creating
the system and applying it to a few examples might be feasible as a GSoC
project.
Agreed, as long as we have enough examples to feel confident that the
infrastructure is sufficient.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
On Wed, Mar 12, 2014 at 11:33:50PM -0400, Quint Guvernator wrote: It is _not_ my goal to make the code harder to maintain down the road. So, at this point, which hunks (if any) are worth patching? This discussion ended up encompassing a lot of other related cleanups. I hope we didn't scare you away. :) My understanding is that you were approaching this as a micro-project for GSoC. I'd love it if you want to pick up and run with some of the ideas discussed here. But as far as a microproject goes, I think it would make sense to identify one or two no-brainer improvement spots by hand, and submit a patch with just those (and I think Junio gave some good guidelines in his reply). -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
