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