Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects
Duy Nguyen pclo...@gmail.com writes: On Fri, Mar 7, 2014 at 1:37 AM, Junio C Hamano gits...@pobox.com wrote: I like what I see in this patch, but I wonder if we can essentially revert that temporary shallow file patch and replace it with the same (or a similar) mechanism uniformly? Using --shallow-file is uniform. The only downside is temporary file. Without it you'll need to think of a way (probably different way for each command) to feed shallow info to. Yes, that is what I meant to say by the we need a way to tell hooks in some cases below; we are in agreement. On the receive-pack side, the comment at the bottom of preprare_shallow_update() makes it clear that, if we wanted to use hooks, we cannot avoid having the proposed new shallow-file in a temporary file, which is unfortunate. Do we have a similar issue on hooks on the upload-pack side? No. I don't think we have hooks on upload-pack. The question was not only about do we happen to work OK with the current code? but about are we closing the door for the future? If we ever need to add hooks to upload-pack, with the updated approach, its operation will not be affected by the temporary shallow file tailored for this specific customer. Which I think is a good thing in general. But at the same time, it means that its operation cannot be customized for the specific customer, taking into account what they lack (which can be gleaned by looking at the temporary shallow information). I do think that it is not a big downside, but that is merely my knee-jerk reaction. Nothing for Documentation/ anywhere? Heh git-pack-objects.txt never described stdin format. At least I searched for --not in it and found none. So I gladly accepted the situation and skipped doc update :D To pile new technical debt on top of existing ones is to make things worse, which we would rather not to see. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects
Duy Nguyen pclo...@gmail.com writes: On Thu, Mar 6, 2014 at 3:49 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index 3ae9092..a980574 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -173,4 +173,17 @@ EOF ) ' +test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' ' + cp -R .git read-only.git + find read-only.git -print | xargs chmod -w + test_when_finished find read-only.git -type d -print | xargs chmod +w + git clone --no-local --depth=2 read-only.git from-read-only + git --git-dir=from-read-only/.git log --format=%s actual + cat expect EOF +add-1-back +4 +EOF + test_cmp expect actual +' + test_done It's a separate issue, but maybe we should add a similar test case for non-shallow clone from a read-only repo too. Are there any other operations that should work well on read-only repos? Any read-only operation ;-)? On the object-transfer front, that would mean fetching from, archive-r from, and perhaps creating bundle from. Locally, log, grep, blame, gitk (but only the browsing part), etc. If a read-write borrower borrows from a read-only location via the objects/info/alternates mechanism, anying operation to the borrower should also work without modifying the borrowee. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Microproject idea: new OPT_* macros for PARSE_OPT_NONEG
Duy Nguyen pclo...@gmail.com writes: I don't know the scope of these microprojects, but yes I think it'll take a few hours for this. By the way, a bit more thought on the idea: instead of making OPT_BOOL_NONEG() that sets NONEG, we could make OPT_BOOL_FLAGS(..., NONEG), which is more flexible. What does a boolean that can never be set to false achieve, by the way? If you have [alias] foo = bar --frotz then you may want to be able to say git foo --no-frotz to countermand the frotz option, and by marking the boolean frotz option to be a NONEG, you can forbid such a usage. That is the only use case I can think of, and that particular use case does not sound like a valid one. 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? I have a suspition that most users of other OPT_SET_TYPE 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. So I am not sure if doubling the number of OPT_type macros as your micro suggestion proposes is the right solution to the problem. -- To unsubscribe from this list: send the line 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 merge --date --author
Andreas Schwab sch...@linux-m68k.org writes: Yann Droneaud ydrone...@opteya.com writes: But I'd like to know if there's a specific reason for git merge to not support --date and --author ? It's rather unusual that a merge is performed on behalf of a different author. Yes. Michael's Nobody bothered to implement it is also correct but the reason why nobody bothered to most likely is due to why would you want to lie?. If the use case is to rebuild history, you would need to be able to also lie about the committer, so git merge \ --date 2013-12-31 23:59:59 + \ --author Happy New Year happy.new-year@gregorian.calendar \ current-year in such a history-rebuild script would not be sufficient. The script can set necessary environment variables to lie about both author and commiter, though, of course. -- To unsubscribe from this list: send the line 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] tag: grok --with as synonym to --contains
Just like git branch can be told to list the branches that has the
named commit by git branch --with commit, teach the same
short-hand to git tag, so that git tag --with commit shows the
releases with the named commit.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
* After umpteenth time I got an optparse error I finally decided
that this may be worth consideration.
Just like git branch, the synonym is not advertised in the
documentation nor git cmd -h output. We _might_ want to expose
both at the same time, but that is not in the scope of this patch.
builtin/tag.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/builtin/tag.c b/builtin/tag.c
index af3af3f..cb7bb2b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -471,6 +471,12 @@ int cmd_tag(int argc, const char **argv, const char
*prefix)
parse_opt_with_commit, (intptr_t)HEAD,
},
{
+ OPTION_CALLBACK, 0, with, with_commit, N_(commit),
+ N_(print only tags that contain the commit),
+ PARSE_OPT_HIDDEN | PARSE_OPT_LASTARG_DEFAULT,
+ parse_opt_with_commit, (intptr_t)HEAD,
+ },
+ {
OPTION_CALLBACK, 0, points-at, NULL, N_(object),
N_(print only tags of the object), 0,
parse_opt_points_at
},
--
To unsubscribe from this list: send the line 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 v7 03/11] trailer: read and process config information
Christian Couder chrisc...@tuxfamily.org writes:
This patch implements reading the configuration
to get trailer information, and then processing
it and storing it in a doubly linked list.
Read and process the ..., perhaps?
The config information is stored in the list
whose first item is pointed to by:
static struct trailer_item *first_conf_item;
If feels somewhat strange if a doubly linked list has only the first
pointer without the last pointer, unless the previous field of the
first one by convention points at the last one, forming a cycle
(which I think is a reasonable thing to do, as a desire for a quick
access to the top and the bottom is often a reason to use doubly
linked list---I didn't check if you implement it that way, though).
+static int set_where(struct conf_info *item, const char *value)
+{
+ if (!strcasecmp(after, value))
+ item-where = WHERE_AFTER;
+ else if (!strcasecmp(before, value))
+ item-where = WHERE_BEFORE;
+ else
+ return 1;
Please follow the usual convention of returning a negative value
upon error, unless there is a compelling reason not to do so.
Do we really want to do strcasecmp here? Are some values case
sensitive and we only allow after, AfTer, AFTER, etc. as case
insensitive keywords? If all values are expected to be case
insensitive, wouldn't it be better to downcase in the caller (just
like we do in the config parser) and use strcmp() against lower case
constants?
All of the comments applies equally to other functions.
+enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,
+ TRAILER_IF_EXISTS, TRAILER_IF_MISSING };
+
+static int set_name_and_type(const char *conf_key, const char *suffix,
+ enum trailer_info_type type,
+ char **pname, enum trailer_info_type *ptype)
+{
+ int ret = ends_with(conf_key, suffix);
+ if (ret) {
+ *pname = xstrndup(conf_key, strlen(conf_key) - strlen(suffix));
+ *ptype = type;
+ }
+ return ret;
+}
This implementation, together with the caller that makes many calls
to it, looks overly inefficient (both runtime- and reviewtime-wise),
doesn't it?
How about having the caller upfront find the last dot before
checking the name-and-type to avoid calling ends_with() so many
times unnecessarily?
Also, wouldn't it be better to make the caller more table-driven,
i.e.
static struct {
const char *name;
enum trailer_kinfo_type type;
} trailer_config_items[] = {
{ key, TRAILER_KEY },
...
};
in the caller's scope, and then
const char *trailer_item, *variable_name;
trailer_item = skip_prefix(conf_key, trailer.);
if (!trailer_item)
return 0;
variable_name = strrchr(trailer_item, '.');
if (!variable_name) {
... trailer_item is a two-level variable name.
... Handle it in whatever way.
return 0;
}
variable_name++;
for (i = 0; i ARRAY_SIZE(trailer_config_items); i++) {
if (strcmp(trailer_config_items[i].name, variable_name))
continue;
name = xstrdup(...);
type = trailer_config_items[i].type;
goto found;
}
/* Unknown trailer.item.variable_name */
return 0;
found:
... do whatever depending on the type ...
or something?
+static struct trailer_item *get_conf_item(const char *name)
+{
+ struct trailer_item *item;
+ struct trailer_item *previous;
+
+ /* Look up item with same name */
+ for (previous = NULL, item = first_conf_item;
+ item;
+ previous = item, item = item-next) {
+ if (!strcasecmp(item-conf.name, name))
+ return item;
+ }
+
+ /* Item does not already exists, create it */
+ item = xcalloc(sizeof(struct trailer_item), 1);
+ item-conf.name = xstrdup(name);
+
+ if (!previous)
+ first_conf_item = item;
+ else {
+ previous-next = item;
+ item-previous = previous;
+ }
+
+ return item;
+}
+
+static int git_trailer_config(const char *conf_key, const char *value, void
*cb)
+{
+ if (starts_with(conf_key, trailer.)) {
+ const char *orig_conf_key = conf_key;
+ struct trailer_item *item;
+ struct conf_info *conf;
+ char *name;
+ enum trailer_info_type type;
+
+ conf_key += 8;
+ if (!set_name_and_type(conf_key, .key, TRAILER_KEY, name,
type)
+ !set_name_and_type(conf_key, .command, TRAILER_COMMAND,
name, type)
+ !set_name_and_type(conf_key, .where, TRAILER_WHERE,
name, type)
+
Re: [PATCH v7 00/11] Add interpret-trailers builtin
Christian Couder chrisc...@tuxfamily.org writes: * many style fixes This round is readable ;-) Thanks. * clearer and nicer setup tests Those long lines that use printf with many embedded \n were harder to read and also looked harder to maintain if we ever wanted to change them. Splicing a string with \n in the middle of a long single line is far harder than adding an independent line, I would think. For example: ... printf Fixes: \nAcked-by= \nReviewed-by: \nSigned-off-by: \n expected ... is easier to read and maintain if written like so (with using HT properly---our MUAs may damage it and turn the indentation into spaces): ... sed -e s/ Z$/ / expect -\EOF Fixes: Z Acked-by= Z Reviewed-by: Z Signed-off-by: Z EOF ... -- To unsubscribe from this list: send the line 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 v7 03/11] trailer: read and process config information
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: Christian Couder chrisc...@tuxfamily.org writes: The config information is stored in the list whose first item is pointed to by: static struct trailer_item *first_conf_item; If feels somewhat strange ... Can't say I agree here. Basically all my doubly-linked lists are not for traversing data forwards and backwards ... Having a last pointer is an orthogonal concept ... Yeah, that is where somewhat strange, not wrong, comes from ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Documentation: Say that submodule clones use a separate gitdirs.
Andrew Keller and...@kellerfarm.com writes: diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index 21cb59a..ea837fd 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt ... Also, this file contains mostly high-level documentation, and this addition feels technical in nature. Is there a location for more technical documentation? Or, perhaps it can be reworded to sound less technical? I tend to agree that the new paragraph looked somewhat out of place and goes into a too low-level detail of the implementation. The repository-layout documentation may be a better place for readers to learn what lives where inside $GIT_DIR. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Documentation: Say that submodule clones use a separate gitdirs.
Henri GEIST geist.he...@laposte.net writes: This information is technical in nature but has some importance for general users. As this kind of clone have a separate gitdir, you will have a surprise if you copy past the worktree as the gitdir will not come together. I am not sure if I understand exactly what you are trying to say. Are you saying that you had a submodule at sub/dir in your working tree, and then mkdir ../another cp -R sub/dir ../another did not result in a usable Git working tree in ../another directory? It is almost like complaining that mkdir ../newone cp -R * ../newone/ did not result in a usable git repository in ../newone directory and honestly speaking, that sounds borderline insane, I'd have to say. Yes, if a user knows what she is doing, she should be able to make something like that work, without running git clone (which is probably the way most users would do it). And yes, it would be good to let the user learn from the documentation enough so that she knows what she is doing. But no, I do not think end-user facing documentation for git-submodule subcommand is the way to do that. That is why I suggested repository-layout as potentially a better alternative location. But perhaps I am mis-reading your rationale. -- To unsubscribe from this list: send the line 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: Trust issues with hooks and config files
Julian Brost jul...@0x4a42.net writes: On 07.03.2014 22:04, Jeff King wrote: If you want to work on it, I think it's an interesting area. But any development would need to think about the transition plan for existing sites that will be broken. I can understand the problem with backward compatibility but in my opinion the default behavior should definitely be to ignore untrusted config files and hooks as it would otherwise only protect users that are already aware of the issue anyways and manually enable this option. Are there any plans for some major release in the future that would allow introducing backward incompatible changes? Git 2.0 has been in the planning for quite some months, and I am inclined to merge these topics prepared for that release to 'master' during this cycle. Anything new like this one is way too late for it, but that does not mean we can never do 3.0 in the future. Perhaps going this way might be possible: * Introduce a configuration that is read only from $HOME/.gitconfig (or its xdg equivalent) to enable or disable the unsafe behaviour. By default (i.e. when the above configuration is not set), allow unsafe read; when instructed by the above configuration to forbid unsafe read, ignore configuration files that are not owned by the owner of the process. People can toggle the unsafe read to experiment with the above (~gitdaemon/.gitconfig can perhaps be used to restrict the daemon access) Keep it that way for a few releases. * After a few releases, start warning people who do not have the unsafe option in their $HOME/.gitconfig about a future default change, to force them to explicitly set it. Keep it that way for a few releases. * Flip the default, perhaps still keeping the warning on the flipped default to help people who have not been following along. Keep it that way for a few releases. * Then finally remove the warning. A release cycle usually last 10-12 weeks on average. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects
Duy Nguyen pclo...@gmail.com writes: On Sat, Mar 8, 2014 at 1:27 AM, Junio C Hamano gits...@pobox.com wrote: On the receive-pack side, the comment at the bottom of preprare_shallow_update() makes it clear that, if we wanted to use hooks, we cannot avoid having the proposed new shallow-file in a temporary file, which is unfortunate. Do we have a similar issue on hooks on the upload-pack side? No. I don't think we have hooks on upload-pack. The question was not only about do we happen to work OK with the current code? but about are we closing the door for the future? If we ever need to add hooks to upload-pack, with the updated approach, its operation will not be affected by the temporary shallow file tailored for this specific customer. Which I think is a good thing in general. But at the same time, it means that its operation cannot be customized for the specific customer, taking into account what they lack (which can be gleaned by looking at the temporary shallow information). I do think that it is not a big downside, but that is merely my knee-jerk reaction. When upload-pack learns about hooks, I think we'll need to go back with --shallow-file, perhaps we a secure temporary place to write in. I don't see another way out. Not really sure why upload-pack needs customization though. The only case I can think of is to prevent most users from fetching certain objects, but that does not sound realistic.. I was more thinking along the lines of logging. But you are right that we can easily revisit --shallow-file interface later. Of course.. So what should we do with this? Go with this no temp file approach and deal with hooks when they come, or prepare now and introduce a secure temp. area? I was rather hoping that we did not have to worry about temporary files. This patch solves the issue for the service people would expect to be read-only the most, and it is a good step in the right direction. So let's follow through with the approach and not worry about secure temporary for now. -- To unsubscribe from this list: send the line 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 v7 00/11] Add interpret-trailers builtin
Øystein Walle oys...@gmail.com writes: Junio C Hamano gitster at pobox.com writes: ... is easier to read and maintain if written like so (with using HT properly---our MUAs may damage it and turn the indentation into spaces): ... sed -e s/ Z$/ / expect -\EOF Fixes: Z Acked-by= Z Reviewed-by: Z Signed-off-by: Z EOF ... How about: printf '%s: \n' Fixes Acked-by Reviewed-by Signed-off-by expect Not really. This solution scores high marks in both readability and maintainability in my mind. I actually considered that approach while I was writing the message you responded to, but discarded it because it forces us to commit to the view that we forsee no need to test an output that does not end with a trailing whitespace. And I do not think that is a limitation we want to place on this test. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Documentation: Say that submodule clones use a separate gitdirs.
Andrew Keller and...@kellerfarm.com writes: On Mar 7, 2014, at 7:50 PM, Henri GEIST wrote: ... To give one of my project to someone else I have copied it on a USB key. By a simple drag and drop with the mouse. And I am quite sure I am not alone doing this way. I have done those kind of things lot of time without any problem. But that day 'the_project' happened to be a submodule cloned by 'git submodule update' then on the USB key the $GIT_DIR of 'the_project' was missing. If 'man git-submodule' have made me aware of the particularities of submodules clone I had write in a terminal: git clone the_project /media/usb/the_project Or at least I had understand what happened quicker. I have nothing against also adding something in repository-layout but I am pretty sure normal users never read repository-layout as it is not a command they use. And it is not mentioned in most tutorials. How about something like this: The git directory of a submodule lives inside the git directory of the parent repository instead of within the working directory. I'm not sure where to put it, though. This is not limited to submodules. There are multiple lower-level mechanisms for a $path/.git to borrow the repository data from elsewhere outside of $path and a cloned submodule uses only one of them. For any such $path, cp -R $path $otherplace will result in an $otherplace that does not work as a Git repository in exactly the same way, whether it happens to be a submodule checkout or not. That is why I suggested to enhance description on a more general part of the documentation that covers what a Git repository is. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GSoC][PATCH v2] use strchrnul() in place of strchr() and strlen()
Rohit Mani rohit.m...@outlook.com writes: Avoid scanning strings twice, once with strchr() and then with strlen(), by using strchrnul(). Thanks. The patch looks good. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP] Pluggable reference backends
Jeff King p...@peff.net writes: On Mon, Mar 10, 2014 at 07:30:45AM -0700, Shawn Pearce wrote: * Store references in a SQLite database, to get correct transaction handling. No to SQLLite in git-core. Using it from JGit requires building SQLLite and a JNI wrapper, which makes JGit significantly less portable. I know SQLLite is pretty amazing, but implementing compatibility with it from JGit will be a big nightmare for us. That seems like a poor reason not to implement a pluggable feature for git-core. If we implement it, then a site using only git-core can take advantage of it. Sites with JGit cannot, and would use a different pluggable storage mechanism that's supported by both. But if we don't implement, it hurts people using only git-core, and it does not help sites using JGit at all. We would need to eventually have at least one backend that we know will play well with different Git implementations that matter (namely, git-core, Jgit and libgit2) before the feature can be widely adopted. The first backend that is used while the plugging-interface is in development can be anything and does not have to be one that eventual ubiquitous one, however; as long as it is something that we do not mind carrying it forever, along with that final reference backend. I take the objection from Shawn only as against making the sqlite that final one. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Documentation: Say that submodule clones use a separate gitdirs.
Henri GEIST geist.he...@laposte.net writes: Le lundi 10 mars 2014 à 08:31 -0700, Junio C Hamano a écrit : ... This is not limited to submodules. There are multiple lower-level mechanisms for a $path/.git to borrow the repository data from elsewhere outside of $path and a cloned submodule uses only one of them. For any such $path, cp -R $path $otherplace will result in an $otherplace that does not work as a Git repository in exactly the same way, whether it happens to be a submodule checkout or not. That is why I suggested to enhance description on a more general part of the documentation that covers what a Git repository is. ... If there is some other situation where this can occur as a side effect of a git command it can be good to have the user aware of the list or at least inform them in general case a git repository cannot be copied in a place every body will see. Or place a note in the manpage of every git command which can have this side effect. I think we should do two things: - In the repository format document, state that there are two lower-level mechanisms and a half that lets a repository borrow from somewhere else, and cp -R of the former will not result in a complete, usable repository, and who employs these mechanisms. * Redirecting the entire .git via the textual gitfile mechanism, which is used by clone --separate-git-dir and submodule; * Borrowing .git/objects read-only from elsewhere, overlaying our own, via .git/objects/info/alternates, which is used by clone --reference; * Redirecting some paths in .git to another, via git workdir; soon to be replaced with .git/commondir mechansim. - In each of the documentation page on an end-user facing command that will be mentioned above, add See also reference to the above description in the repository format document. We could elaborate the See also as something like If you use this feature, do not think you can cp -R the repository to elsewhere and expect the copy to function; see also ..., if we wanted to. -- To unsubscribe from this list: send the line 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] rebase: new option to post edit a squashed or fixed up commit
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
After squashing or fixing up, you may want to have a final look at the
commit, edit some more if needed or even do some testing. --postedit
enables that. This is (to me) a paranoid mode so either I enable it
for all squashes and fixups, or none. Hence a new option, not new todo
commands that give finer selection.
If we were to adopt Michael's (?) idea of allowing flags to each
insn in the insn sheet, would this restriction be easily lifted?
That is, instead of saying squash, you say squash --stop or
something.
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a1adae8..42061fc 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -571,6 +571,11 @@ do_next () {
;;
esac
record_in_rewritten $sha1
+ if test -n $postedit
+ then
+ warn Stopped at $sha1... $rest
+ exit_with_patch $sha1 0
+ fi
;;
I would have expected that any new code would stop only at the last
squash (or fixup) in a series of squashes, but this appears to stop
even at an intermediate squashed result, which will not appear in
the final history. Am I misreading the patch (or misunderstanding
the intent of the 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] rev-parse --parseopt: option argument name hints
Ilya Bobyr ilya.bo...@gmail.com writes: On 3/4/2014 11:22 AM, Junio C Hamano wrote: Ilya Bobyr ilya.bo...@gmail.com writes: @@ -333,6 +339,7 @@ h,helpshow the help foo some nifty option --foo bar= some cool option --bar with an argument +baz=arg another cool option --baz with an argument named arg It probably is better not to have named arg at the end here, as that gives an apparent-but-false contradiction with the Angle brackets are added *automatically* and confuse readers. At least, it confused _this_ reader. I am not sure I understand what is confusing here. But I removed the named arg part. After reading Angle brackets are automatically given, seeing that the argument description has manually spelled arg gave me Huh?. Without named arg there is no such confusion. If there would be an example, I think, it is easy to understand how it works. Of course. That is why I suggested to do without named arg part---I didn't mean to suggest not to add the example. I also think that you can demonstrate something other than '=' (whose usage is already shown with bar= above) here as well, but I think we can go either way. After the eval in the existing example to parse the $@ argument list in this part of the documentation, it may be a good idea to say something like: The above command, when $@ is --help, produces the following help output: ... sample output here ... to show the actual output. That way, we can illustrate how input baz?arg description of baz is turned into --baz[=arg] output clearly (yes, I am suggesting to use '?' in the new example, not '=' whose usage is already shown in the existing example). 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? At the same time the target structure that holds the option description calls this string argh. OK, that is fine, then (I'd prefer a field name not to sound like arrrgh, but that is an entirely different topic). I've renamed it to end. It is used to remember possible end of the argument name in just one paragraph of code. Sounds good. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #01; Tue, 4)
Johannes Sixt j.s...@viscovery.net writes: Am 3/5/2014 1:10, schrieb Junio C Hamano: * nd/gitignore-trailing-whitespace (2014-02-10) 2 commits - dir: ignore trailing spaces in exclude patterns - dir: warn about trailing spaces in exclude patterns Warn and then ignore trailing whitespaces in .gitignore files, unless they are quoted for fnmatch(3), e.g. path\ . Will merge to 'next'. The new test does not pass on Windows. Thanks, will mark not to merge to 'master' yet. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] test patch hunk editing with commit -p -m
Eric Sunshine sunsh...@sunshineco.com writes: On Mon, Mar 10, 2014 at 2:49 PM, Benoit Pierre benoit.pie...@gmail.com wrote: Add (failing) test: with commit changing the environment to let hooks now that no editor will be used (by setting GIT_EDITOR to :), the edit hunk functionality does not work (no editor is launched and the whole hunk is committed). Signed-off-by: Benoit Pierre benoit.pie...@gmail.com --- t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++ 1 file changed, 34 insertions(+) create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh Is it possible to give this file a name less unusual and more consistent with other test scripts? Perhaps choose a more generic name which may allow other similar tests to be added to the file in the future (if needed)? Surely. There are reset-patch and checkout-patch tests, and if we were to add something like this, I'd imagine commit-patch would be a logical name for the new test. diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh new file mode 100755 index 000..994939a --- /dev/null +++ b/t/t7513-commit_-p_-m_hunk_edit.sh @@ -0,0 +1,34 @@ +#!/bin/sh + +test_description='hunk edit with commit -p -m' +. ./test-lib.sh + +if ! test_have_prereq PERL +then + skip_all=skipping '$test_description' tests, perl not available + test_done +fi + +test_expect_success 'setup (initial)' ' + echo line1 file + git add file + git commit -m commit1 + echo line3 file + cat expect -\EOF + diff --git a/file b/file + index a29bdeb..c0d0fb4 100644 + --- a/file + +++ b/file + @@ -1 +1,2 @@ +line1 + +line2 + EOF In the previous review, the suggestion was that creation of 'expect' should be moved to the test below where it is actually used rather than into the 'setup' phase above. +' + +test_expect_failure 'edit hunk commit -p -m message' ' + echo e | env GIT_EDITOR=sed s/+line3\$/+line2/ -i git commit -p -m commit2 file + git diff HEAD^ HEAD actual + test_cmp expect actual +' + +test_done -- 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] rev-parse --parseopt: option argument name hints
Junio C Hamano gits...@pobox.com 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] fix status_printf_ln calls zero-length format warnings
Jeff King p...@peff.net writes: Most of us who compile with -Wall decided a while ago to use -Wno-format-zero-length, because it really is a silly complaint (it assumes there are no side effects of the function besides printing the format string, which is obviously not true in this case). It would be nice to compile out of the box with -Wall -Werror, and I think your solution looks relatively clean. But I am slightly concerned about the assumption that it is OK to pass a NULL fmt parameter to a function marked with __attribute__((format)). It certainly seems to be the case now, and I do not know of any plans for that to change, but it seems like a potentially obvious thing for the compiler to check. Yes, exactly. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GSoC idea: allow git rebase --interactive todo lines to take options
Jeff King p...@peff.net writes: I'd say keep it at this point. I think there _are_ some good ideas here, and part of a project is figuring out what is good. And part of the role of the mentor is applying some taste. Amen to that. I hope we have enough mentor-candidates with good taste, though ;-) There are probably students who would be a good fit, and students who would not. That is true for just about every project, of course, but I think this one is just a little trickier than some. Perhaps. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/8] Hiding refs
Jeff King p...@peff.net writes: I think the main flag of interest is giving an fnmatch pattern to limit the advertised refs. There could potentially be others, but I do not know of any offhand. One thing that comes to mind is where symrefs point at, which we failed to add the last time around because we ran out of the hidden-space behind NUL. -- To unsubscribe from this list: send the line 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 memory leak?] Minor memory leak fix
Fredrik Gustafsson iv...@iveqy.com writes: On Tue, Mar 11, 2014 at 06:58:11PM +0700, Duy Nguyen wrote: On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson iv...@iveqy.com wrote: Strbuf needs to be released even if it's locally declared. path is declared static. So yes it's a leak but the leak is minimum. Your patch would make more sense if static is gone and it's leaked after every write_archive_entry call. That's one of the reasons of the RFC. I know Junio thinks that minor things shouldn't be fixed by themselfes because it takes up review bandwidth, so it's better to fix them once you touch that part of the code anyway. (At least that's how I've understood him). Yes, but I at the same time think this static struct strbuf is a clear statement by the original author that this is not a leak per-se. The trade-off, if I am reading the code right, is between keeping a piece of memory that is large enough to hold the longest pathname until exit() vs saving repeated allocations and frees for each of the thousands of paths in the resulting archive. I tend to think the original strikes a better balance between the two. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] install_branch_config: simplify verbose diagnostic logic
Paweł Wawruch pa...@aleg.pl writes:
Replace the chain of if statements with table of strings.
Signed-off-by: Paweł Wawruch pa...@aleg.pl
---
I changed the commit message. Logic of table has changed. To make it more
clear I added three dimensions of the table.
I am not sure if the message is diagnostic; it looks more like
reminder text to me.
diff --git a/branch.c b/branch.c
index 723a36b..741551a 100644
--- a/branch.c
+++ b/branch.c
@@ -53,6 +53,21 @@ 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.)
+ }}};
I almost agree with the above use of a strange brace opening/closing
convention in order to reduce the indentation levels [*1*]
but then perhaps the above can be dedented even further?
+ 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_is_branch ? remote : shortname;
+ int message_number;
Do you still need this variable after making it a multi-dimentional
array?
[Footnote]
*1* i.e. otherwise we would need something like
message[][][] = {
{
{
...,
...
},
{
...,
...
},
},
...
};
--
To unsubscribe from this list: send the line 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
Sandy Carter sandy.car...@savoirfairelinux.com writes: diff --git a/wt-status.c b/wt-status.c index a452407..69e0dfc 100644 --- a/wt-status.c +++ b/wt-status.c @@ -264,7 +264,7 @@ static void wt_status_print_unmerged_data(struct wt_status *s, case 6: how = _(both added:); break; case 7: how = _(both modified:); break; } - status_printf_more(s, c, %-20s%s\n, how, one); + status_printf_more(s, c, %-19s %s\n, how, one); strbuf_release(onebuf); } Thanks; I have to wonder if we would want to do something similar to what 3651e45c (wt-status: take the alignment burden off translators, 2013-11-05) to the other parts of the output, though. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/26] t1400: Pass a legitimate newvalue to update command
Michael Haggerty mhag...@alum.mit.edu writes: It seems to me that -z input will nearly always be machine-generated, so there is not much reason to accept the empty string as shorthand for zeros. So I think that my version of the rules, being simpler to explain, is a slight improvement. But your version is already out in the wild, so backwards-compatibility is also a consideration, even though it is rather a fine point in a rather unlikely usage (why use update rather than delete to delete a reference?). I don't know. I'm willing to rewrite the code to go back to your rules, or rewrite the documentation to describe my rules. Neutral bystanders *cough*Junio*cough*, what do you prefer? I may be misremembering things, but your first sentence quoted above was exactly my reaction while reviewing the original change, and I might have even raised that as an issue myself, saying something like consistency across values is more important than type-saving in a machine format. Since nobody else were raising the issue back then, however, we are stuck with the interface. I am not against deprecating and removing the support for it in the longer term, though. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/8] Hiding refs
Jeff King p...@peff.net writes: On Tue, Mar 11, 2014 at 12:32:37PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: I think the main flag of interest is giving an fnmatch pattern to limit the advertised refs. There could potentially be others, but I do not know of any offhand. One thing that comes to mind is where symrefs point at, which we failed to add the last time around because we ran out of the hidden-space behind NUL. Yeah, good idea. I might be misremembering some complications, but we can probably do it with: 1. Teach the client to send an advertise-symrefs flag before the ref advertisement. 2. Teach the server to include symrefs in the ref advertisement; we can invent a new syntax because we know the client has asked for it. I was thinking more about the underlying protocol, not advertisement in particular, and I think we came to the same conclusion. The capability advertisement deserves to have its own separate packet message type, when both sides say that they understand it, so that we do not have to be limited by the pkt-line length limit. We could do one message per capability, and at the same time can lift the traditional capability hidden after the NUL is purged every time, so we need to repeat them if we want to later change it, because that is how older clients and servers use that information insanity, for example. -- To unsubscribe from this list: send the line 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
Sandy Carter sandy.car...@savoirfairelinux.com writes: Le 2014-03-11 15:59, Junio C Hamano a écrit : Sandy Carter sandy.car...@savoirfairelinux.com writes: diff --git a/wt-status.c b/wt-status.c index a452407..69e0dfc 100644 --- a/wt-status.c +++ b/wt-status.c @@ -264,7 +264,7 @@ static void wt_status_print_unmerged_data(struct wt_status *s, case 6: how = _(both added:); break; case 7: how = _(both modified:); break; } - status_printf_more(s, c, %-20s%s\n, how, one); + status_printf_more(s, c, %-19s %s\n, how, one); strbuf_release(onebuf); } Thanks; I have to wonder if we would want to do something similar to what 3651e45c (wt-status: take the alignment burden off translators, 2013-11-05) to the other parts of the output, though. I could, do this. It would be cleaner, but there's just the issue of the colon (:) which requires a space before it in the french language[1]. As you can see in po/fr.po, the french translators have done a good job at including it [2]. 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. + if (status == DIFF_STATUS_COPIED || status == DIFF_STATUS_RENAMED) + status_printf_more(s, c, %s:%.*s%s - %s, + what, len, padding, one, two); + else + status_printf_more(s, c, %s:%.*s%s, + what, len, padding, one); [1] https://en.wikipedia.org/wiki/Colon_%28punctuation%29#Spacing [2] https://github.com/git/git/blob/master/po/fr.po#L585 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] configure.ac: link with -liconv for locale_charset()
Dmitry Marakasov amd...@amdmi3.ru writes: On e.g. FreeBSD 10.x, the following situation is common: - there's iconv implementation in libc, which has no locale_charset() function - there's GNU libiconv installed from Ports Collection Git build process - detects that iconv is in libc and thus -liconv is not needed for it - detects locale_charset in -liconv, but for some reason doesn't add it to CHARSET_LIB (as it would do with -lcharset if locale_charset() was found there instead of -liconv) - git doesn't build due to unresolved external locale_charset() Fix this by adding -liconv to CHARSET_LIB if locale_charset() is detected in this library. Signed-off-by: Dmitry Marakasov amd...@amdmi3.ru --- Looks sensible; Dilyan, any comments? configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git configure.ac configure.ac index 2f43393..3f5c644 100644 --- configure.ac +++ configure.ac @@ -890,7 +890,7 @@ GIT_CONF_SUBST([HAVE_STRINGS_H]) # and libcharset does CHARSET_LIB= AC_CHECK_LIB([iconv], [locale_charset], - [], + [CHARSET_LIB=-liconv], [AC_CHECK_LIB([charset], [locale_charset], [CHARSET_LIB=-lcharset])]) GIT_CONF_SUBST([CHARSET_LIB]) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/7] commit: fix patch hunk editing with commit -p -m
Benoit Pierre benoit.pie...@gmail.com writes:
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..da423b2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -53,10 +53,10 @@ struct checkout_opts {
static int post_checkout_hook(struct commit *old, struct commit *new,
int changed)
{
- return run_hook(NULL, post-checkout,
- sha1_to_hex(old ? old-object.sha1 : null_sha1),
- sha1_to_hex(new ? new-object.sha1 : null_sha1),
- changed ? 1 : 0, NULL);
+return run_hook_le(NULL, post-checkout,
+sha1_to_hex(old ? old-object.sha1 : null_sha1),
+sha1_to_hex(new ? new-object.sha1 : null_sha1),
+changed ? 1 : 0, NULL);
Funny indentation.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] test patch hunk editing with commit -p -m
Benoit Pierre benoit.pie...@gmail.com writes: Add (failing) test: with commit changing the environment to let hooks now that no editor will be used (by setting GIT_EDITOR to :), the edit hunk functionality does not work (no editor is launched and the whole hunk is committed). Signed-off-by: Benoit Pierre benoit.pie...@gmail.com --- t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++ 1 file changed, 34 insertions(+) create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh I'll move this to t/t7514-commit-patch.sh for now while queuing. -- To unsubscribe from this list: send the line 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.
brian m. carlson sand...@crustytoothpaste.net writes: We shrink the source and destination arrays, but not the modes or submodule_gitfile arrays, resulting in potentially mismatched data. Shrink all the arrays at the same time to prevent this. Signed-off-by: brian m. carlson sand...@crustytoothpaste.net --- builtin/mv.c | 5 + 1 file changed, 5 insertions(+) diff --git a/builtin/mv.c b/builtin/mv.c index f99c91e..b20cd95 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix) memmove(destination + i, destination + i + 1, (argc - i) * sizeof(char *)); + memmove(modes + i, modes + i + 1, + (argc - i) * sizeof(char *)); + memmove(submodule_gitfile + i, + submodule_gitfile + i + 1, + (argc - i) * sizeof(char *)); i--; } } else Thanks. Neither this nor John's seems to describe the user-visible way to trigger the symptom. Can we have tests for them? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git $Id$ smudge filter
shawn wilson ag4ve...@gmail.com writes: Currently, I've got a perl script that modifies the Id line in a smudge filter: [filter ident-line] smudge = /usr/local/bin/githook_ident-filter.pl %f The problem I've noticed with smudge filters is that it leaves the repo dirty. How do I fix this? I am basically trying to replicate the behavior of CVS or SVN $Id$ line here. It somewhat smells fishy to have only smudge filter defined without a corresponding clean filter, doesn't it? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Mar 2014, #02; Tue, 11)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. Topics that have been cooking in 'next' for 2.0 have been merged to 'master', which means we are committed to make the next one a big release. Kind of scary, isn't it? You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to master] * cc/starts-n-ends-with-endgame (2013-12-05) 1 commit (merged to 'next' on 2014-02-25 at 473e143) + strbuf: remove prefixcmp() and suffixcmp() Originally merged to 'next' on 2014-01-07 Endgame for the cc/starts-n-ends-with topic; this needs to be evil-merged with other topics that introduce new uses of prefix/suffix-cmp functions. * gj/push-more-verbose-advice (2013-11-13) 1 commit (merged to 'next' on 2014-02-25 at 1cd10b0) + push: switch default from matching to simple Originally merged to 'next' on 2013-11-21 Explain 'simple' and 'matching' in git push advice message; the topmost patch is a rebase of jc/push-2.0-default-to-simple on top of it. * jc/add-2.0-ignore-removal (2013-04-22) 1 commit (merged to 'next' on 2014-02-25 at a0d018a) + git add pathspec... defaults to -A Originally merged to 'next' on 2013-12-06 Updated endgame for git add pathspec that defaults to --all aka --no-ignore-removal. * jc/core-checkstat-2.0 (2013-05-06) 1 commit (merged to 'next' on 2014-02-25 at 62f6aeb) + core.statinfo: remove as promised in Git 2.0 Originally merged to 'next' on 2013-12-06 * jc/hold-diff-remove-q-synonym-for-no-deletion (2013-07-19) 1 commit (merged to 'next' on 2014-02-25 at ccfff88) + diff: remove diff-files -q in a version of Git in a distant future Originally merged to 'next' on 2013-12-06 Remove deprecated -q option git diff-files. * jc/push-2.0-default-to-simple (2013-06-18) 1 commit (merged to 'next' on 2014-02-25 at 1f0e178) + push: switch default from matching to simple Originally merged to 'next' on 2013-12-06 * jk/run-network-tests-by-default (2014-02-14) 1 commit (merged to 'next' on 2014-02-25 at 62a8ad0) + tests: turn on network daemon tests by default Originally merged to 'next' on 2014-02-20 Teach make test to run networking tests when possible by default. * jn/add-2.0-u-A-sans-pathspec (2013-04-26) 1 commit (merged to 'next' on 2014-02-25 at 9e5c0d2) + git add: -u/-A now affects the entire working tree Originally merged to 'next' on 2013-12-06 * ks/combine-diff (2014-02-24) 6 commits (merged to 'next' on 2014-02-25 at 69e5a87) + tests: add checking that combine-diff emits only correct paths + combine-diff: simplify intersect_paths() further + combine-diff: combine_diff_path.len is not needed anymore + combine-diff: optimize combine_diff_path sets intersection + diff test: add tests for combine-diff with orderfile + diffcore-order: export generic ordering interface (this branch is used by ks/tree-diff-nway.) Originally merged to 'next' on 2014-02-20 Teach combine-diff to honour the path-output-order imposed by diffcore-order, and optimize how matching paths are found in the N-way diffs made with parents. * nd/daemonize-gc (2014-02-10) 2 commits (merged to 'next' on 2014-02-25 at f592335) + gc: config option for running --auto in background + daemon: move daemonize() to libgit.a Originally merged to 'next' on 2014-02-20 Allow running gc --auto in the background. -- [New Topics] * jk/detect-push-typo-early (2014-03-05) 3 commits - push: detect local refspec errors early - match_explicit_lhs: allow a verify only mode - match_explicit: hoist refspec lhs checks into their own function Catch git push $there no-such-branch early. Will merge to 'next'. * jk/diff-funcname-cpp-regex (2014-03-05) 1 commit - diff: simplify cpp funcname regex Has the discussion settled on this? * jk/doc-deprecate-grafts (2014-03-05) 1 commit - docs: mark info/grafts as outdated Will merge to 'next'. * rm/strchrnul-not-strlen (2014-03-10) 1 commit - use strchrnul() in place of strchr() and strlen() Will merge to 'next'. * sh/use-hashcpy (2014-03-06) 1 commit - Use hashcpy() when copying object names Will merge to 'next'. * jc/no-need-for-env-in-sh-scripts (2014-03-06) 1 commit - *.sh: drop useless use of env Will merge to 'next'. * jc/tag-contains-with (2014-03-07) 1 commit - tag: grok --with as synonym to --contains Will merge to 'next'. * bp/commit-p-editor (2014-03-11) 8 commits - run-command: mark run_hook_with_custom_index as deprecated - merge hook tests: fix and update tests - merge: fix GIT_EDITOR override for commit hook - commit: fix patch hunk editing with commit -p -m - SQUASH??? - test patch hunk editing with commit -p -m - merge hook tests: use 'test_must_fail'
Re: [PATCH] rev-parse --parseopt: option argument name hints
Ilya Bobyr ilya.bo...@gmail.com 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: Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote
Junio C Hamano gits...@pobox.com 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 start, url and end means in the context of the overall purpose of the command. Describe the extended syntax of end 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 gits...@pobox.com --- 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 +`start` and the output asks it to integrate the changes you made +since that commit, up to the commit named by `end`, by visiting +the repository named by `url`. + OPTIONS --- -p:: - Show patch text + Include patch text in the output. start:: - Commit to start at. + Commit to start at. This names a commit that is already in + the upstream history. url:: - URL to include in the summary. + The repository URL to be pulled from. end:: - 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 `url` has the commit at a tip of a +ref that is different from the ref you have it locally, you can use +the `local:remote` 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][GSoC] parse-options: Add OPT_SET_INT_NONEG.
Duy Nguyen pclo...@gmail.com 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_type 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
Re: An idea for git bisect and a GSoC enquiry
Jacopo Notarstefano jacopo.notarstef...@gmail.com writes: On Mon, Mar 3, 2014 at 7:34 PM, Junio C Hamano gits...@pobox.com wrote: I think you fundamentally cannot use two labels that are merely distinct and bisect correctly. You need to know which ones (i.e. good) are to be excluded and the other (i.e. bad) are to be included when computing the remaining to be tested set of commits. Good point. Yes, this isn't viable. But if you make them into --no-longer-X vs --still-X, then it will be viable without us knowing what X means. -- To unsubscribe from this list: send the line 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 pclo...@gmail.com writes: On Wed, Mar 12, 2014 at 5:12 AM, Junio C Hamano gits...@pobox.com 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: [PATCH] status merge: guarentee space between msg and path
Duy Nguyen pclo...@gmail.com writes: On Wed, Mar 12, 2014 at 3:26 AM, Junio C Hamano gits...@pobox.com wrote: Sandy Carter sandy.car...@savoirfairelinux.com 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: git: problematic git status output with some translations (such as fr_FR)
Jonathan Nieder jrnie...@gmail.com 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
Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.
Yuxuan Shui yshu...@gmail.com writes: On Thu, Mar 13, 2014 at 2:30 AM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com 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_type 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: 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
[PATCH] wt-status: i18n of section labels
From: Jonathan Nieder jrnie...@gmail.com
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 jrnie...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
* 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) {
+ label_width = maxwidth
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Jeff King p...@peff.net 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: [PATCH v2 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()
Yuxuan Shui yshu...@gmail.com 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 yshu...@gmail.com
---
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: What's cooking in git.git (Mar 2014, #02; Tue, 11)
Jeff King p...@peff.net 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()
Jeff King p...@peff.net 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] wt-status: i18n of section labels
Sandy Carter sandy.car...@savoirfairelinux.com 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] submodule: add verbose mode for add/update
Orgad Shaneh org...@gmail.com 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 branch] [-f|--force] [--name name] [--reference
repository] [--] repository [path]
+USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference
repository] [-v|--verbose] [--] repository [path]
or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
or: $dashless [--quiet] init [--] [path...]
or: $dashless [--quiet] deinit [-f|--force] [--] path...
- or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge] [--recursive]
[--] [path...]
+ or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference repository] [--merge] [--recursive]
[-v|--verbose] [--] [path...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit n]
[commit] [--] [path...]
or: $dashless [--quiet] foreach [--recursive] command
or: $dashless [--quiet] sync [--recursive] [--] [path...]
@@ -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 v2] status merge: guarentee space between msg and path
Sandy Carter sandy.car...@savoirfairelinux.com 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
[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
[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 gits...@pobox.com
---
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 3/4] wt-status: i18n of section labels
From: Jonathan Nieder jrnie...@gmail.com
From: Jonathan Nieder jrnie...@gmail.com
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 jrnie...@gmail.com
Helped-by: Sandy Carter
Signed-off-by: Junio C Hamano gits...@pobox.com
---
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 jrnie...@gmail.com
From: Jonathan Nieder jrnie...@gmail.com
Date: Thu, 19 Dec 2013 11:43:19 -0800
Signed-off-by: Junio C Hamano gits...@pobox.com
---
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 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 gits...@pobox.com
---
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 file... 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 file... 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 file... 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 file... 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 file... 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 file... to unstage)
(use git add file... 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 file... to unstage)
(use git add file... 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 file... 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 file... to unstage)
(use git add file... 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
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Jeff King p...@peff.net 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()
Jeff King p...@peff.net 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()
Quint Guvernator quintus.pub...@gmail.com writes: The result after the conversion, however, still have the same magic numbers, but one less of them each. Doesn't it make it harder to later spot the patterns to come up with a better abstraction that does not rely on the magic number? It is _not_ my goal to make the code harder to maintain down the road. Good. So, at this point, which hunks (if any) are worth patching? Will, I am not going through all the mechanical hits to memcmp() and judge each and every one if it is a good idea to convert. Anybody who does so in order to tell you which hunks are worth patching would end up being the one doing the real work, and at that point there is nothing left to be credited as your work anymore ;-) But as Peff said, there are good bits, like these ones just for a few examples. diff --git a/builtin/apply.c b/builtin/apply.c index a7e72d5..16c20af 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -1631,7 +1631,7 @@ static int parse_fragment(const char *line, unsigned long size, * l10n of \ No newline... is at least that long. */ case '\\': - if (len 12 || memcmp(line, \\ , 2)) + if (len 12 || !starts_with(line, \\ )) return -1; break; } @@ -1646,7 +1646,7 @@ static int parse_fragment(const char *line, unsigned long size, * it in the above loop because we hit oldlines == newlines == 0 * before seeing it. */ - if (12 size !memcmp(line, \\ , 2)) + if (12 size starts_with(line, \\ )) offset += linelen(line, size); patch-lines_added += added; These two are about An incomplete line marker begins with a backslash and a SP and there is no other significance in the constant 2 (like, after we recognise the match, we start scanning the remainder of the line starting at the offset 2). It is a tangent but I notice that these two parts (both in the original and in the version after patch) contradict what the incomplete last line marker should look like in a minor detail. On the other hand, I think this one from nearby is iffy. @@ -846,8 +846,8 @@ static int has_epoch_timestamp(const char *nameline) * -MM-DD hh:mm:ss must be from either 1969-12-31 * (west of GMT) or 1970-01-01 (east of GMT) */ - if ((zoneoffset 0 memcmp(timestamp, 1969-12-31, 10)) || - (0 = zoneoffset memcmp(timestamp, 1970-01-01, 10))) + if ((zoneoffset 0 !starts_with(timestamp, 1969-12-31)) || + (0 = zoneoffset !starts_with(timestamp, 1970-01-01))) return 0; hourminute = (strtol(timestamp + 11, NULL, 10) * 60 + If one looks at the post-context of the hunk, one would realize that this codepath very intimately knows how the timestamp should look like at the byte-offset level, not just -MM-DD ought to be 10-byte long, but there should be two-digit hour part after skipping one byte after that -MM-DD part, followed by two-digit minute part after further skipping one byte, knowing that these details are guaranteed by the stamp_regexp[] pattern it earlier made sure the given string would match. I do not think it is a good idea to reduce this kind of precise format knowledge from this function in the first place (after all, this is parsing a header line in a traditional diff whose format is known to us), and even if it were our eventual goal to reduce the precise format knowledge, it would not help very much to get rid of constant 10 only from these two memcmp() calls, and that is why I think this hunk is iffy. Hope the above shows what kind of thinking is needed to judge each change from memcmp() to !starts_with(). Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: Taking two random examples from an early and a late parts of the patch: --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name) enum object_type type; unsigned long size; char *buffer = read_sha1_file(sha1, type, size); -if (memcmp(buffer, object , 7) || +if (!starts_with(buffer, object ) || [...] The original hunks show that the code knows and relies on magic numbers 7 and 8 very clearly and there are rooms for improvement. Like: what if the file is empty? Yes. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] connect.c: SP after }, not TAB
Thanks ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] install_branch_config: simplify verbose messages logic
Eric Sunshine sunsh...@sunshineco.com writes:
Shouldn't this logic [to decide what the printf arguments should
be] also be encoded in the table?
...
The same argument also applies to computation of the 'name' variable
above. It too can be pushed into the the table.
Because the printf argument logic does not have to be in the
table, the same argument does not apply to the 'name' thing.
After looking at the v5 patch, I do not think an extra two-element
array to switch between remote vs shortname is making it any easier
to read. I would have to say that personally I find that
const char *name[] = {remote, shortname};
... long swath of code ...
printf_ln(... name[!remote_is_branch] ...);
is a lot harder to read than:
printf_ln(... remote_is_branch ? shortname : branch ...);
HTH, and thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: An idea for git bisect and a GSoC enquiry
Michael Haggerty mhag...@alum.mit.edu writes: It seems to me that we can infer which mark is which from the normal bisect user interaction. At the startup phase of a bisect, there are only three cases: 1. There are fewer than two different types of marks on tested commits. For example, maybe one commit has been marked bad. Or two commits have both been marked slow. In this case we wait for the user to choose another commit manually, so we don't have to know the meaning of the mark. 2. There are two different types of marks, but no commits with differing marks are ancestors of each other. In this case, we pick the merge base of two commits with differing marks and present it to the user for testing. But we can do that without knowing which mark is before the change and which mark means after the change. So just defer the inference. 3. There are two different types of marks, and a commit with one mark is an ancestor of a commit with the other mark. In this case, it is clear from the ancestry which mark means before the change and which mark means after the change. So record the orientation of the marks and continue like in the old days. Of course, there are still details to be worked out, like how to tag the commits before we know which mark means what. But that is just a clerical problem, not a fundamental one. Yup, with an extra state kept somewhere in $GIT_DIR, we should in principle be able to defer the value judgement (aka which one should be treated as a bottom of the range). The first change that is needed for this scheme to be workable is to decide how we mark such an unknown state at the beginning, though. We assume that we need to keep track of a single top one (bad, aka no-longer-good) while we have to keep track of multiple bottom ones (good). There also is a safety valve in the current logic for transitioning from case #2 to case #3; when a common ancestor is marked as bad (aka no-longer-good), we notice that the original bisection is screwy in the sense that the user is seeing not just a single state flip that made something that used to be good into bad. I am afraid that we may instead _silently_ decide that the user is trying to locate a state flip that made something that used to be bad (at the common ancestor) into good with the logic proposed above. From the point of view of the user who wanted to find a regression by marking one as bad and the other good, running bisection whose semantics suddenly and silently changed into an opposite where was it fixed hunt would be an unpleasant and confusing experience. I do not know, without knowing the meaning of slow and fast (which implicitly tells us which way the user intends to bisect), how well we can keep that safety valve. Other than that, I like the idea. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Microproject idea: new OPT_* macros for PARSE_OPT_NONEG
Duy Nguyen pclo...@gmail.com writes: On Sat, Mar 8, 2014 at 2:20 AM, Junio C Hamano gits...@pobox.com wrote: Looking at git grep -B3 OPT_NONEG output, it seems that NONEG is associated mostly with OPTION_CALLBACK and OPTION_SET_INT in the existing code. Perhaps OPT_SET_INT should default to not just OPT_NOARG but also OPT_NONEG? There are OPT_SET_INT() that should not have NONEG in current code. So there are two sets of SET_INT anyway. Either we convert them all to a new macro that takes an extra flag, or we add OPT_SET_INT_NONEG() that covers one set and leave the other set alone. Are you forgetting the third alternative, of swapping the default, if the ones that do not want NONEG are in the minority, to reduce the number of spelled-out instances? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument
Yuxuan Shui yshu...@gmail.com writes:
Since fsck_ident doesn't change the content of **ident, the type of
ident could be const char **.
This change is required to rewrite fsck_commit() to use skip_prefix().
Signed-off-by: Yuxuan Shui yshu...@gmail.com
---
It may not be a bad idea to read and understand reviews other people
are receiving for their microprojects, e.g. $gmane/243852.
Change the type is not technically incorrect per-se, but when
viewed in git shortlog output, it wastes more bytes than it
conveys information about this change if stated differently. Any
patch that touch existing code is a change by definition.
Perhaps
fsck.c:fsck_ident(): ident argument points at a const string
or something?
I see that the body of the patch follows the review by Peff on the
previous round of this series, so I'll forge a Helped-by: or
something into the log message when I queue this patch.
Thanks.
fsck.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fsck.c b/fsck.c
index 99c0497..7776660 100644
--- a/fsck.c
+++ b/fsck.c
@@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict,
fsck_error error_func)
return retval;
}
-static int fsck_ident(char **ident, struct object *obj, fsck_error
error_func)
+static int fsck_ident(const char **ident, struct object *obj, fsck_error
error_func)
{
if (**ident == '')
return error_func(obj, FSCK_ERROR, invalid author/committer
line - missing space before email);
@@ -281,7 +281,7 @@ static int fsck_ident(char **ident, struct object *obj,
fsck_error error_func)
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- char *buffer = commit-buffer;
+ const char *buffer = commit-buffer;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()
Yuxuan Shui yshu...@gmail.com writes:
Currently we use memcmp() in fsck_commit() to check if buffer start
with a certain prefix, and skip the prefix if it does. This is exactly
what skip_prefix() does. And since skip_prefix() has a self-explaintory
name, this could make the code more readable.
Signed-off-by: Yuxuan Shui yshu...@gmail.com
---
fsck.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/fsck.c b/fsck.c
index 7776660..7e6b829 100644
--- a/fsck.c
+++ b/fsck.c
@@ -281,7 +281,7 @@ static int fsck_ident(const char **ident, struct object
*obj, fsck_error error_f
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- const char *buffer = commit-buffer;
+ const char *buffer = commit-buffer, *tmp;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
@@ -290,15 +290,17 @@ static int fsck_commit(struct commit *commit,
fsck_error error_func)
if (commit-date == ULONG_MAX)
return error_func(commit-object, FSCK_ERROR, invalid
author/committer line);
- if (memcmp(buffer, tree , 5))
+ buffer = skip_prefix(buffer, tree );
+ if (buffer == NULL)
We encourage people to write this as:
if (!buffer)
The same comment applies to other new lines in this patch.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()
Junio C Hamano gits...@pobox.com writes:
-if (memcmp(buffer, tree , 5))
+buffer = skip_prefix(buffer, tree );
+if (buffer == NULL)
We encourage people to write this as:
if (!buffer)
The same comment applies to other new lines in this patch.
I also see a lot of repetitions in the code, before or after the
patch. I wonder if a further refactoring along this line on top of
these two patches might be worth considering.
No, I am not proud of sneaking tree_sha1[] array pointers as a
seemingly boolean-looking must-match-header parameter into the
helper, but this is merely a how about going in this direction
weather-balloon patch, so
fsck.c | 83 --
1 file changed, 61 insertions(+), 22 deletions(-)
diff --git a/fsck.c b/fsck.c
index 6aab23b..a3eea7f 100644
--- a/fsck.c
+++ b/fsck.c
@@ -279,10 +279,55 @@ static int fsck_ident(const char **ident, struct object
*obj, fsck_error error_f
return 0;
}
+static int fsck_object_line(struct commit *commit, fsck_error error_func,
+ const char **buffer, const char *header,
+ unsigned char must_match_header[])
+{
+ unsigned char sha1_buf[20];
+ unsigned char *sha1 = must_match_header ? must_match_header : sha1_buf;
+ const char *buf;
+
+ buf = skip_prefix(*buffer, header);
+ if (!buf) {
+ if (must_match_header)
+ return error_func(commit-object, FSCK_ERROR,
+ invalid format - expected '%.*s'
line,
+ (int) strlen(header) - 1,
+ header);
+ return 1;
+ }
+ if (get_sha1_hex(buf, sha1) || buf[40] != '\n')
+ return error_func(commit-object, FSCK_ERROR,
+ invalid '%.*s' line format - bad sha1,
+ (int) strlen(header) - 1,
+ header);
+ *buffer = buf + 41;
+ return 0;
+}
+
+static int fsck_ident_line(struct commit *commit, fsck_error error_func,
+ const char **buffer, const char *header)
+{
+ const char *buf;
+ int err;
+
+ buf = skip_prefix(*buffer, header);
+ if (!buf)
+ return error_func(commit-object, FSCK_ERROR,
+ invalid format - expected '%.*s' line,
+ (int) strlen(header) - 1,
+ header);
+ err = fsck_ident(buf, commit-object, error_func);
+ if (err)
+ return err;
+ *buffer = buf;
+ return 0;
+}
+
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- const char *buffer = commit-buffer, *tmp;
- unsigned char tree_sha1[20], sha1[20];
+ const char *buffer = commit-buffer;
+ unsigned char tree_sha1[20];
struct commit_graft *graft;
int parents = 0;
int err;
@@ -290,18 +335,17 @@ static int fsck_commit(struct commit *commit, fsck_error
error_func)
if (commit-date == ULONG_MAX)
return error_func(commit-object, FSCK_ERROR, invalid
author/committer line);
- buffer = skip_prefix(buffer, tree );
- if (!buffer)
- return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'tree' line);
- if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
- return error_func(commit-object, FSCK_ERROR, invalid 'tree'
line format - bad sha1);
- buffer += 41;
- while ((tmp = skip_prefix(buffer, parent ))) {
- buffer = tmp;
- if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n')
- return error_func(commit-object, FSCK_ERROR, invalid
'parent' line format - bad sha1);
- buffer += 41;
- parents++;
+ err = fsck_object_line(commit, error_func, buffer, tree , tree_sha1);
+ if (err)
+ return err;
+ while (1) {
+ err = fsck_object_line(commit, error_func, buffer, parent ,
NULL);
+ if (err 0)
+ return err;
+ else if (!err)
+ parents++;
+ else
+ break;
}
graft = lookup_commit_graft(commit-object.sha1);
if (graft) {
@@ -324,16 +368,11 @@ static int fsck_commit(struct commit *commit, fsck_error
error_func)
if (p || parents)
return error_func(commit-object, FSCK_ERROR, parent
objects missing);
}
- buffer = skip_prefix(buffer, author );
- if (!buffer)
- return error_func(commit-object, FSCK_ERROR, invalid format
- expected 'author' line);
- err = fsck_ident(buffer, commit-object, error_func);
+
+ err = fsck_ident_line(commit
Re: Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote
Eric Sunshine sunsh...@sunshineco.com writes: +Prepare a request to your upstream project to pull your changes to +their tree to the standard output, by summarizing your changes and +showing where your changes can be pulled from. Perhaps splitting this into two sentence (and using fewer to's) would make it a bit easier to grok? Something like: Generate a request asking your upstream project to pull changes into their tree. The request, printed to standard output, summarizes the changes and indicates from where they can be pulled. Thanks. +When the repository named by `url` has the commit at a tip of a +ref that is different from the ref you have it locally, you can use Did you want to drop it from this sentence? Or did you mean to say the ref as you have it locally? Thanks for your careful reading. Will drop it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: No progress from push when using bitmaps
Jeff King p...@peff.net writes: There are a few ways around this: 1. Add a new phase Writing packs which counts from 0 to 1. Even though it's more accurate, moving from 0 to 1 really isn't that useful (the throughput is, but the 0/1 just looks like noise). 2. Add a new phase Writing reused objects that counts from 0 bytes up to N bytes. This looks stupid, though, because we are repeating the current byte count both here and in the throughput. 3. Use the regular Writing objects progress, but fake the object count. We know we are writing M bytes with N objects. Bump the counter by 1 for every M/N bytes we write. The first two require some non-trivial surgery to the progress code. I am leaning towards the third. Not just because it's easy, but because I think it actually shows the most intuitive display. Yes, it's fudging the object numbers, but those are largely meaningless anyway (in fact, it makes them _better_ because now they're even, instead of getting 95% done and then hitting some blob that is as big as the rest of the repo combined). I think the above argument, especially the fudging but largely meaningless anyway part, makes perfect sense. Thanks for looking into this. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Proposal: Write git subtree info to .git/config
John Butterfield johnb...@gmail.com writes: Has there been any talk about adding a stub for git subtrees in .git/config? I do not think so, and that is probably for a good reason. A subtree biding can change over time, but .git/config is about recording information that do not change depending on what tree you are looking at, so there is an impedance mismatch---storing that information in .git/config is probably a wrong way to go about it. It might help to keep track of In this tree, the tip of that other history is bound as a subtree at this path, which means that information more naturally belongs to each tree, I would think. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] GSoC Change multiple if-else statements to be table-driven
Eric Sunshine sunsh...@sunshineco.com writes:
Thanks for the resubmission. Comments below.
Thanks, Eric, for helping so many micro exercises.
On Thu, Mar 13, 2014 at 4:20 PM, Yao Zhao zhaox...@umn.edu wrote:
Subject: [PATCH] GSoC Change multiple if-else statements to be table-driven
It's a good idea to let reviewers know that this is attempt 2. Do so
by saying [PATCH v2]. Your next one will be [PATCH v3]. The -v option
for git format-email can help.
Yao, I think Eric meant git format-patch.
When your patch is applied via git am, text inside [...] gets
stripped automatically. The GSoC tells email readers what this
submission is about, but isn't relevant to the actual commit message.
It should be placed inside [...]. For instance: [PATCH/GSoC v2].
So in short,
Subject: [PATCH/GSoC v2] branch.c: turn nested if-else logic to
table-driven
or something.
+ typedef struct PRINT_LIST {
...
+ int b_origin;
+ } PRINT_LIST;
We do not do ALL_CAPS names and tend not to introduce one-off
typedefs for struct. Instead we would just use struct print_list
throughout (if we were to indeed use such a new struct, that is).
+ PRINT_LIST print_list[] = {
+ {.print_str = _(Branch %s set up to track remote branch %s
from %s by rebasing.),
+ .arg2 = shortname, .arg3 = origin,
+.b_rebasing = 1,
.b_remote_is_branch = 1, .b_origin = 1},
I am confused here: I use struct initializer and I am not sure if it's ok
because it is only supported by ANSI
...
Indeed, you want to avoid named field initializers in this project and
instead use positional initializers.
Correct.
Translatable strings in an initializer should be wrapped with N_()
instead of _(). You will still need to use _() later on when you
reference the string from the table. See section 4.7 [2] of the GNU
gettext manual for details.
Correct.
An alternate approach might be to use a multi-dimensional array,
where the boolean values of rebasing, remote_is_branch, and origin
are keys into the array. This would allow you to pick out the
correct PRINT_LIST entry directly (no looping), thus eliminating
the need for those b_rebasing, b_remote_is_branch, and b_origin
members.
Correct.
After seeing so many table driven submissions, I however tend to
agree with your earlier comment on another thread on this same
micro, where you said an nested if-else cascade that was rewritten
in a clearer way (sorry, I do not remember whose submission it was
offhand) may be the best answer to the Would it make sense to make
the code table-driven? question, even though I tentatively queued
d7ea7894 (install_branch_config(): simplify verbose messages logic,
2014-03-13) from Paweł on 'pu'.
Thanks for a review.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Quint Guvernator quintus.pub...@gmail.com writes: I'll be re-reading this thread and working on this patch over the weekend to try to identify the more straightforward hunks I could submit in a patch. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] reset: Print a warning when user uses git reset during a merge
Andrew Wong andrew.k...@gmail.com writes: On Fri, Mar 14, 2014 at 10:33 AM, Marc Branchaud marcn...@xiplink.com wrote: I know this approach was suggested earlier, but given these dangers it seems silly to give this big warning on a plain git reset but still go ahead and do the things the warning talks about. Is there any issue with changing git reset to error-out now but letting git reset --mixed proceed? Something like (note the reworded warning message): Yeah, I would have preferred to have git reset error out right now, because the messed up work tree can be quite a pain to clean up. The main argument for issuing the warning is about maintaining compatibility. For the users that really did mean --merge, the warning is silly. It's basically saying We know that you're about to mess up your work tree, but we let you mess up anyway. Learn the correct way so that you don't mess up next time. I suspect that you meant --mixed instead of --merge here. I do not agree with the We know that you are about to mess up above. Whoever is issuing that warning message may not know better than the user who is running reset. As you wrote most likely not what the user wants in your proposed log message, the only thing we know is that it _often_ is a newbie mistake. I recently needed to manually cherry-pick only one half of a patch, and the way I did so was git show $that_commit P.diff git apply -3 P.diff ... conflicts are expected; that is why I used -3 in the ... first place git reset git diff HEAD edit ... edit away the other half I did not want to cherry-pick ... while fixing the conflicted parts that happened to be ... in the part I did want to cherry-pick git cherry-pick --no-commit $that_commit could have been used as a replacement for the first two steps but being able to run the reset without erroring out was an essential part to make this workflow usable. So I am OK with eventually error out by default, but not OK with we know better than the user and will not allow it at all. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC GSoC idea: new git config features
Jeff King p...@peff.net writes: On Sat, Mar 01, 2014 at 12:01:44PM +0100, Matthieu Moy wrote: Jeff King p...@peff.net writes: If we had the keys in-memory, we could reverse this: config code asks for keys it cares about, and we can do an optimized lookup (binary search, hash, etc). I'm actually dreaming of a system where a configuration variable could be declared in Git's source code, with associated type (list/single value, boolean/string/path/...), default value and documentation (and then Documentation/config.txt could become a generated file). One could imagine a lot of possibilities like Yes, I think something like that would be very nice. ... ... Migrating the whole code to such system would take time, but creating the system and applying it to a few examples might be feasible as a GSoC project. Agreed, as long as we have enough examples to feel confident that the infrastructure is sufficient. I agree that it would give us a lot of enhancement opportunities if we had a central catalog of what the supported configuration variables are and what semantics (e.g. type, multi-value-ness, etc.) they have. One thing we need to be careful about is that we still must support random configuration items that git-core does not care about at all but scripts (and future versions of git-core) read off of, though. -- To unsubscribe from this list: send the line 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] t5541: don't call start_httpd after sourcing lib-terminal.sh
Jeff King p...@peff.net writes:
One option would be to _always_ define test_terminal
That looks like the right direction to go.
Something like the patch below (looks like we should be using $PERL_PATH
instead of perl, too).
;-) Also a SP between test_terminal and (), perhaps.
diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index 9a2dca5..55b708f 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -1,35 +1,36 @@
# Helpers for terminal output tests.
-test_expect_success PERL 'set up terminal for tests' '
+# Catch tests which should depend on TTY but forgot to. There's no need
+# to check that TTY is set here. If the test declared it and we are running
+# it, then it is set.
+test_terminal() {
+ if ! test_declared_prereq TTY
+ then
+ echo 4 test_terminal: need to declare TTY prerequisite
+ return 127
+ fi
+ perl $TEST_DIRECTORY/test-terminal.perl $@
+}
+
+test_lazy_prereq TTY '
+ test_have_prereq PERL
+
# Reading from the pty master seems to get stuck _sometimes_
# on Mac OS X 10.5.0, using Perl 5.10.0 or 5.8.9.
#
# Reproduction recipe: run
#
# i=0
# while ./test-terminal.perl echo hi $i
# do
# : $((i = $i + 1))
# done
#
# After 2000 iterations or so it hangs.
# https://rt.cpan.org/Ticket/Display.html?id=65692
#
- if test $(uname -s) = Darwin
- then
- :
- elif
- perl $TEST_DIRECTORY/test-terminal.perl \
- sh -c test -t 1 test -t 2
- then
- test_set_prereq TTY
- test_terminal () {
- if ! test_declared_prereq TTY
- then
- echo 4 test_terminal: need to declare TTY
prerequisite
- return 127
- fi
- perl $TEST_DIRECTORY/test-terminal.perl $@
- }
- fi
+ test $(uname -s) != Darwin
+
+ perl $TEST_DIRECTORY/test-terminal.perl \
+ sh -c test -t 1 test -t 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] t/lib-terminal: make TTY a lazy prerequisite
Jeff King p...@peff.net writes:
On Fri, Mar 14, 2014 at 02:47:14PM -0700, Junio C Hamano wrote:
Something like the patch below (looks like we should be using $PERL_PATH
instead of perl, too).
Actually, we don't need to do this, as of 94221d2 (t: use perl instead
of $PERL_PATH where applicable, 2013-10-28). If only the author of
that commit were here to correct me...
Yuck. I forgot all about that, too.
I wonder if that commit (actually the one before it) invites subtle
bugs by tempting us to say
sane_unset VAR
VAR=VAL perl -e 0
test ${VAR+isset} != isset
-- 8 --
Subject: t/lib-terminal: make TTY a lazy prerequisite
When lib-terminal.sh is sourced by a test script, we
immediately set up the TTY prerequisite. We do so inside a
test_expect_success, because that nicely isolates any
generated output.
However, this early test can interfere with a script that
later wants to skip all tests (e.g., t5541 then goes on to
set up the httpd server, and wants to skip_all if that
fails). TAP output doesn't let us skip everything after we
have already run at least one test.
We could fix this by reordering the inclusion of
lib-terminal.sh in t5541 to go after the httpd setup. That
solves this case, but we might eventually hit a case with
circular dependencies, where either lib-*.sh include might
want to skip_all after the other has run a test. So
instead, let's just remove the ordering constraint entirely
by doing the setup inside a test_lazy_prereq construct,
rather than in a regular test. We never cared about the
test outcome anyway (it was written to always succeed).
Note that in addition to setting up the prerequisite, the
current test also defines test_terminal. Since we can't
affect the environment from a lazy_prereq, we have to hoist
that out. We previously depended on it _not_ being defined
when the TTY prereq isn't set as a way to ensure that tests
properly declare their dependency on TTY. However, we still
cover the case (see the in-code comment for details).
Reported-by: Jens Lehmann jens.lehm...@web.de
Signed-off-by: Jeff King p...@peff.net
---
Thanks.
t/lib-terminal.sh | 37 +++--
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index 9a2dca5..5184549 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -1,6 +1,20 @@
# Helpers for terminal output tests.
-test_expect_success PERL 'set up terminal for tests' '
+# Catch tests which should depend on TTY but forgot to. There's no need
+# to aditionally check that the TTY prereq is set here. If the test declared
+# it and we are running the test, then it must have been set.
+test_terminal () {
+ if ! test_declared_prereq TTY
+ then
+ echo 4 test_terminal: need to declare TTY prerequisite
+ return 127
+ fi
+ perl $TEST_DIRECTORY/test-terminal.perl $@
+}
+
+test_lazy_prereq TTY '
+ test_have_prereq PERL
+
# Reading from the pty master seems to get stuck _sometimes_
# on Mac OS X 10.5.0, using Perl 5.10.0 or 5.8.9.
#
@@ -15,21 +29,8 @@ test_expect_success PERL 'set up terminal for tests' '
# After 2000 iterations or so it hangs.
# https://rt.cpan.org/Ticket/Display.html?id=65692
#
- if test $(uname -s) = Darwin
- then
- :
- elif
- perl $TEST_DIRECTORY/test-terminal.perl \
- sh -c test -t 1 test -t 2
- then
- test_set_prereq TTY
- test_terminal () {
- if ! test_declared_prereq TTY
- then
- echo 4 test_terminal: need to declare TTY
prerequisite
- return 127
- fi
- perl $TEST_DIRECTORY/test-terminal.perl $@
- }
- fi
+ test $(uname -s) != Darwin
+
+ perl $TEST_DIRECTORY/test-terminal.perl \
+ sh -c test -t 1 test -t 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
What's cooking in git.git (Mar 2014, #03; Fri, 14)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. More topics merged to 'master', some of which have been cooking before the v1.9.0 final release. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to master] * ak/gitweb-fit-image (2014-02-20) 1 commit (merged to 'next' on 2014-03-06 at ba8cb50) + gitweb: Avoid overflowing page body frame with large images Instead of allowing an img to be shown in whatever size, force scaling it to fit on the page with max-height/max-width css style attributes. * da/difftool-git-files (2014-03-05) 2 commits (merged to 'next' on 2014-03-06 at a563ec1) + t7800: add a difftool test for .git-files + difftool: support repositories with .git-files git difftool misbehaved when the repository is bound to the working tree with the .git file mechanism, where a textual file .git tells us where it is. * jc/check-attr-honor-working-tree (2014-02-06) 2 commits (merged to 'next' on 2014-03-06 at 960d679) + check-attr: move to the top of working tree when in non-bare repository + t0003: do not chdir the whole test process git check-attr when (trying to) work on a repository with a working tree did not work well when the working tree was specified via --work-tree (and obviously with --git-dir). The command also works in a bare repository but it reads from the (possibly stale, irrelevant and/or nonexistent) index, which may need to be fixed to read from HEAD, but that is a completely separate issue. As a related tangent to this separate issue, we may want to also fix check-ignore, which refuses to work in a bare repository, to also operate in a bare one. * jh/note-trees-record-blobs (2014-02-20) 1 commit (merged to 'next' on 2014-03-06 at f46852d) + notes: disallow reusing non-blob as a note object git notes -C blob should not take an object that is not a blob. * jk/commit-dates-parsing-fix (2014-03-07) 6 commits (merged to 'next' on 2014-03-07 at 01e9d92) + show_ident_date: fix tz range check (merged to 'next' on 2014-03-06 at dd641e2) + log: do not segfault on gmtime errors + log: handle integer overflow in timestamps + date: check date overflow against time_t + fsck: report integer overflow in author timestamps + t4212: test bogus timestamps with git-log Codepaths that parse timestamps in commit objects have been tightened. * jk/doc-coding-guideline (2014-02-28) 1 commit (merged to 'next' on 2014-03-06 at c33101d) + CodingGuidelines: mention C whitespace rules Elaborate on a style niggle that has been part of mimic existing code. * jk/http-no-curl-easy (2014-02-18) 1 commit (merged to 'next' on 2014-03-06 at 56d3f6f) + http: never use curl_easy_perform Uses of curl's multi interface and easy interface do not mix well when we attempt to reuse outgoing connections. Teach the RPC over http code, used in the smart HTTP transport, not to use the easy interface. * jk/janitorial-fixes (2014-02-18) 5 commits (merged to 'next' on 2014-03-06 at dac2de6) + open_istream(): do not dereference NULL in the error case + builtin/mv: don't use memory after free + utf8: use correct type for values in interval table + utf8: fix iconv error detection + notes-utils: handle boolean notes.rewritemode correctly * jk/remote-pushremote-config-reading (2014-02-24) 1 commit (merged to 'next' on 2014-03-06 at 9e71ecb) + remote: handle pushremote config in any order git push did not pay attention to branch.*.pushremote if it is defined earlier than remote.pushdefault; the order of these two variables in the configuration file should not matter, but it did by mistake. * jl/doc-submodule-update-checkout (2014-02-28) 1 commit (merged to 'next' on 2014-03-06 at 8cdf5cb) + submodule update: consistently document the '--checkout' option Add missing documentation for submodule update --checkout. * jm/stash-doc-k-for-keep (2014-02-24) 1 commit (merged to 'next' on 2014-03-06 at ddd8e48) + stash doc: mention short form -k in save description * jn/am-doc-hooks (2014-02-24) 1 commit (merged to 'next' on 2014-03-06 at 5c1c372) + am doc: add a pointer to relevant hooks * jn/bisect-coding-style (2014-03-03) 1 commit (merged to 'next' on 2014-03-06 at e1de2a5) + git-bisect.sh: fix a few style issues * ks/config-file-stdin (2014-02-18) 4 commits (merged to 'next' on 2014-03-06 at 3e77313) + config: teach git config --file - to read from the standard input + config: change git_config_with_options() interface + builtin/config.c: rename check_blob_write() - check_write() + config: disallow relative include paths from blobs git config learned to read from the standard input when - is given as the value to its --file parameter
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
Jeff King p...@peff.net writes: On Sat, Mar 15, 2014 at 05:05:29PM +0100, Thomas Rast wrote: diff --git a/builtin/mv.c b/builtin/mv.c index f99c91e..b20cd95 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix) memmove(destination + i, destination + i + 1, (argc - i) * sizeof(char *)); + memmove(modes + i, modes + i + 1, + (argc - i) * sizeof(char *)); This isn't right -- you are computing the size of things to be moved based on a type of char*, but 'modes' is an enum. (Valgrind spotted this.) Maybe using sizeof(*destination) and sizeof(*modes) would make this less error-prone? -Peff Would it make sense to go one step further to introduce two macros to make this kind of screw-up less likely? 1. array is an array that holds nr elements. Move count elements starting at index at down to remove them. #define MOVE_DOWN(array, nr, at, count) The implementation should take advantage of sizeof(*array) to come up with the number of bytes to move. 2. array is an array that holds nr elements. Move count elements starting at index at up to make room to copy new elements in. #define MOVE_UP(array, nr, at, count) The implementation should take advantage of sizeof(*array) to come up with the number of bytes to move. Optionally, to make 2. even safer, these macros could take alloc to say that array has memory allocated to hold alloc elements, and the implementation may check nr + count does not overflow alloc. This would make 1. and 2. asymmetric (move-down can do no validation using alloc, but move-up would be helped), so I am not sure it is a good idea. After letting my eyes coast over hits from git grep memmove, there do seem to be some places that these would help readability, but not very many. -- To unsubscribe from this list: send the line 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.
Junio C Hamano gits...@pobox.com writes:
Would it make sense to go one step further to introduce two macros
to make this kind of screw-up less likely?
...
After letting my eyes coast over hits from git grep memmove, there
do seem to be some places that these would help readability, but not
very many.
I see quite a many hits that follow this pattern
memmove(array + pos, array + pos + 1, sizeof(*array) * (nr - pos))
to make a single slot in a middle of array available, which would be
good candidates to use MOVE_DOWN(). Just to show a few:
builtin/mv.c:226: memmove(source + i, source + i + 1,
builtin/mv.c-227- (argc - i) * sizeof(char *));
builtin/mv.c:228: memmove(destination + i,
builtin/mv.c-229- destination + i + 1,
builtin/mv.c-230- (argc - i) * sizeof(char *));
cache-tree.c:92:memmove(it-down + pos + 1,
cache-tree.c-93-it-down + pos,
cache-tree.c-94-sizeof(down) * (it-subtree_nr - pos - 1));
Perhaps something like this patch to start off; I am not sure
MOVE_DOWN_BOUNDED is needed, though.
cache.h | 33 +
1 file changed, 33 insertions(+)
diff --git a/cache.h b/cache.h
index b66cb49..b2615ab 100644
--- a/cache.h
+++ b/cache.h
@@ -455,6 +455,39 @@ extern int daemonize(void);
} \
} while (0)
+/*
+ * With an array array that currently holds nr elements, move
+ * elements at at and later down by count elements to make room to
+ * add in new elements. The caller is responsible for making sure
+ * that the array has enough room to hold nr + count slots.
+ */
+#define MOVE_DOWN(array, nr, at, count)\
+ memmove((array) + (at) + (count), \
+ (array) + (at), \
+ sizeof((array)[0]) * ((nr) - (at)))
+
+/*
+ * With an array array that has enough memory to hold alloc
+ * elements allocated and currently holds nr elements, move elements
+ * at at and later down by count elements to make room to add in
+ * new elements.
+ */
+#define MOVE_DOWN_BOUNDED(array, nr, at, count, alloc) \
+ do { \
+ if ((alloc) = (nr) + (count)) \
+ BUG(MOVE_DOWN beyond the end of an array); \
+ MOVE_DOWN((array), (nr), (at), (count)); \
+ } while (0)
+
+/*
+ * With an array array that curently holds nr elements, move elements
+ * at at + count and later down by count elements, removing the
+ * elements between at and at + count.
+ */
+#define MOVE_UP(array, nr, at, count) \
+ memmove((array) + (at), (array) + (at) + (count), \
+ sizeof((array)[0]) * ((nr) - ((at) + (count
+
/* Initialize and use the cache information */
extern int read_index(struct index_state *);
extern int read_index_preload(struct index_state *, const struct pathspec
*pathspec);
--
To unsubscribe from this list: send the line 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: Using - for previous branch failing with rebase
Tim Chase g...@tim.thechases.com writes: Is this just an interface inconsistency or is there a some technical reason this doesn't work (or, has it been addressed/fixed, and just not pulled into Debian Stable's 1.7.10.4 version of git)? It is merely that nobody thought rebase would benefit from such a short-hand, I think. Teach more commands that operate on branch names about - shorthand for the branch we were previously on, like we did for git merge - sometime after we introduced git checkout - has been sitting in my leftover bits list at http://git-blame.blogspot.com/p/leftover-bits.html for quite some time. Hint, hint... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation/git-am: Document supported --patch-format options
Chris Packham judge.pack...@gmail.com writes: Ping? Hasn't it been already cooking in 'next' for a few days? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] GSoC Change multiple if-else statements to be table-driven
Eric Sunshine sunsh...@sunshineco.com writes: Perhaps it is time to mark this microproject as taken on the GSoC page [2], along a fews others for which we have received multiple submissions. [2]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md I actually have been of multiple minds on this. * After seeing that many candidates tried to solve the same micro, apparently without checking answers by other people, and seeing how they responded to the reviews given to them, I found that we had as equally good interactions with them to judge their skills, both techincal and social, as we would have had if each of them solved different micros. * Many reviewers may have gotten tired of seeing many novice attempts on the same micro over and over again, giving gentle suggestions for improvements. Because the _sole_ purpose of these micros were to help candidates get their toes wet in the process, duplicated efforts on the candidates' side are not wasted---they each hopefully learned how to interact with this community. But it is true that, if we were wishing to also get some trivial clean-ups in the codebase as a side effect of these micros, we have wasted reviewer bandwidth by not having enough micros, and reviewers may have had some feeling that their efforts did not fully contribute to improving our codebase, which may have been discouraging. Big thanks go to all reviewers who participated despite this. If we had more micros, the apparent wastage of the reviewer efforts might have been avoided. * Many candidates did not even bother to check if others are working on the same micro, however, which would be a bad sign by itself. Concentrating on one's own topic, without paying any attention to what others are working on the same software, is never a good discipline. Some may argue that it would be unfair to blame the candidates on this---they may have picked up micros that haven't been solved if we had more---but after seeing that many candidates' submissions apparently did not take into account the reviews given to others' submissions on the same micro and/or had many exactly the same issues like log message styles as submissions on other micros that have already been reviewed, I personally do not think they are blameless. So in short, yes it would have been nicer if we had more micros than candidates, but I do not think it was detrimental for the purpose of these micro exercises that multiple candidates ended up attempting the same micro. Again, Big Thanks to Michael for the excellent micro idea, and all reviewers who participated. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #03; Fri, 14)
Torsten Bögershausen tbo...@web.de writes: On 2014-03-14 23.09, Junio C Hamano wrote: * ap/remote-hg-skip-null-bookmarks (2014-01-02) 1 commit - remote-hg: do not fail on invalid bookmarks Reported to break tests ($gmane/240005) Expecting a reroll. I wonder what should happen here. The change breaks all the tests in test-hg-hg-git.sh (And the breakage may prevent us from detecting other breakages) The ideal situation would be to have an extra test case for the problem which we try to fix with this patch. Antoine, is there any way to make your problem reproducable ? And based on that, to make a patch which passes all test cases ? After re-reading the thread briefly (there're just five messages) http://thread.gmane.org/gmane.comp.version-control.git/239797/focus=240069 I think the breakage the patch tries to fix seems to be of dubious nature in the first place (I don't know how I ended-up with such a bookmark, Antoine says in $gmane/239800), and it has been in Expecting a reroll state in response to I will try to come-up with an improved version in $gmane/240069 but nothing has happened for a few months. At this point I think it would be OK for me to discard the topic (without prejudice); if the root cause of the issue (if there is one) and a proper fix is discovered in the future, the topic can come back with a fresh patch, but it appears to me that keeping the above patch in my tree would not help anybody. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #03; Fri, 14)
Philip Oakley philipoak...@iee.org writes: * po/git-help-user-manual (2014-02-18) 1 commit - Provide a 'git help user-manual' route to the docbook I am not sure if this is even needed. My rhetorical question would be what should 'git help user-manual' do? for the beginner, ... Why would any _beginner_ even be expected to ask git help everything, including user-manual, in the first place? Wouldn't things like /usr/share/doc/git-doc/ be the place to help them in a consistent manner across different programs instead? ... do we have a sort of policy on ensuring that the majority of user documentation should be available (or at least referenced) via the 'git help' mechanism? I doubt that there should be such a policy. git help is primarily to show the manual pages, and some technical details docs that are referenced from manpages may need to be reachable from it. The user manual, on the other hand, may reference individual manpages but because it is primarily a document that shows the overall flow to employ different commands, individual manpages referring to the user manual feels entirely the other way around. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #03; Fri, 14)
Ramkumar Ramachandra artag...@gmail.com writes: ... I'd first fix the main issue: stale content. I'm not sure who uses git show-branch or mailx anymore, for instance. Unfortunately, I haven't seen a representation better than what show-branch gives me when assessing what needs to happen during rebases of multiple topics some of which depend on other topics. git log --oneline --graph is *not* it, with too much clutter. I do not think stale is the issue. Common-ness may be an issue, as the usage of Git surely does not have to involve show-branch for a simple workflow, e.g. a beginning standalone developer's. The show-branch (and mailx) example are headed by My typical Git day in the Integrator section (emphasis on My---it was not meant to be You ought to do like I do because I know this is the best current practice back when it was written, as none of us had enough experience to declare what the BCP was). You may argue the command set shown there may be specific to My usage, and it is atypical for the Integrator workflow. We could try to come up with a different/better workflows for each classes of developers to replace that examples sections, and that will be the first step to update the listed set of commands for each classes, I would think. You need to realize that the workflow described in the examples section is a real, battle tested one, not something that came out of thin air, though. The way forward would be to think about the following things, in the order listed here: (1) Review the classes of developers. Is the classification we have in the document still good? Do we need to add new classes of developers? Do we need to collapse some into one? (2) For each class of developers, review the workflow illustrated in the Examples: . Do the steps illustrate a typical flow of activities for the class of developers? Are there steps that typically happens during a developer's day that are missing in the flow? Are some of the steps in the example unnecessary? . Have we made improvements to various Porcelain commands since the document was written? Do we have better ways to achieve some steps illustrated there? (3) For each class of developers, review the commands listed before the Examples section and adjust to the Examples updated in the second step. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #03; Fri, 14)
Duy Nguyen pclo...@gmail.com writes: There are two minor fixes [1] [2] on top of v5, but I'm not going to send v6 again unless I see more substantial changes. Just give me a signal or something before you merge to next so I have a chance to fix them if v6 never comes. [1] http://article.gmane.org/gmane.comp.version-control.git/243693 [2] http://article.gmane.org/gmane.comp.version-control.git/243692 Thanks. I'd first need to give you a signal when I replace what was queued with v5, as I have been way behind on this topic and another large one from Michael, partly due to the micro storm for the past few weeks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/28] Convert git_snpath() to strbuf_git_path()
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
In the previous patch, git_snpath() is modified to allocate a new
strbuf buffer because vsnpath() needs that. But that makes it awkward
because git_snpath() receives a pre-allocated buffer from outside and
has to copy data back. Rename it to strbuf_git_path() and make it
receive strbuf directly.
The conversion from git_snpath() to git_path() in
update_refs_for_switch() is safe because that function does not keep
any pointer to the round-robin buffer pool allocated by
get_pathname().
s/that function does not/that function and all of its callers do not/
is the guarantee we need to say it is safe. And I think that holds
true.
Thanks.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/checkout.c | 25 +
cache.h| 4 +--
path.c | 11 ++--
refs.c | 78
+-
refs.h | 2 +-
5 files changed, 65 insertions(+), 55 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 64c2aca..efb5e2f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -585,18 +585,21 @@ static void update_refs_for_switch(const struct
checkout_opts *opts,
if (opts-new_orphan_branch) {
if (opts-new_branch_log !log_all_ref_updates) {
int temp;
- char log_file[PATH_MAX];
- const char *ref_name = mkpath(refs/heads/%s,
opts-new_orphan_branch);
+ struct strbuf log_file = STRBUF_INIT;
+ int ret;
+ const char *ref_name;
+ ref_name = mkpath(refs/heads/%s,
opts-new_orphan_branch);
temp = log_all_ref_updates;
log_all_ref_updates = 1;
- if (log_ref_setup(ref_name, log_file,
sizeof(log_file))) {
+ ret = log_ref_setup(ref_name, log_file);
+ log_all_ref_updates = temp;
+ strbuf_release(log_file);
+ if (ret) {
fprintf(stderr, _(Can not do reflog
for '%s'\n),
opts-new_orphan_branch);
- log_all_ref_updates = temp;
return;
}
- log_all_ref_updates = temp;
}
}
else
@@ -651,14 +654,10 @@ static void update_refs_for_switch(const struct
checkout_opts *opts,
new-name);
}
}
- if (old-path old-name) {
- char log_file[PATH_MAX], ref_file[PATH_MAX];
-
- git_snpath(log_file, sizeof(log_file), logs/%s,
old-path);
- git_snpath(ref_file, sizeof(ref_file), %s, old-path);
- if (!file_exists(ref_file) file_exists(log_file))
- remove_path(log_file);
- }
+ if (old-path old-name
+ !file_exists(git_path(%s, old-path))
+ file_exists(git_path(logs/%s, old-path)))
+ remove_path(git_path(logs/%s, old-path));
}
remove_branch_state();
strbuf_release(msg);
diff --git a/cache.h b/cache.h
index a344a5f..0fae730 100644
--- a/cache.h
+++ b/cache.h
@@ -646,8 +646,8 @@ extern int check_repository_format(void);
extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)
__attribute__((format (printf, 3, 4)));
-extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
- __attribute__((format (printf, 3, 4)));
+extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
+ __attribute__((format (printf, 2, 3)));
extern char *git_pathdup(const char *fmt, ...)
__attribute__((format (printf, 1, 2)));
extern char *mkpathdup(const char *fmt, ...)
diff --git a/path.c b/path.c
index 36d461e..417df76 100644
--- a/path.c
+++ b/path.c
@@ -70,19 +70,12 @@ static void vsnpath(struct strbuf *buf, const char *fmt,
va_list args)
strbuf_cleanup_path(buf);
}
-char *git_snpath(char *buf, size_t n, const char *fmt, ...)
+void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
{
- struct strbuf sb = STRBUF_INIT;
va_list args;
va_start(args, fmt);
- vsnpath(sb, fmt, args);
+ vsnpath(sb, fmt, args);
va_end(args);
- if (sb.len = n)
- strlcpy(buf, bad_path, n);
- else
- memcpy(buf, sb.buf, sb.len + 1);
- strbuf_release(sb);
- return buf;
}
char *git_pathdup(const char *fmt, ...)
Re: [PATCH 3/7] test patch hunk editing with commit -p -m
Benoit Pierre benoit.pie...@gmail.com writes: Add (failing) tests: with commit changing the environment to let hooks know that no editor will be used (by setting GIT_EDITOR to :), the edit hunk functionality does not work (no editor is launched and the whole hunk is committed). Signed-off-by: Benoit Pierre benoit.pie...@gmail.com --- t/t7513-commit-patch.sh | 32 1 file changed, 32 insertions(+) create mode 100755 t/t7513-commit-patch.sh diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh Again, as I said, I'll rename this to t7514-commit.patch.sh while I queue this. Thanks. new file mode 100755 index 000..9311b0c --- /dev/null +++ b/t/t7513-commit-patch.sh @@ -0,0 +1,32 @@ +#!/bin/sh + +test_description='hunk edit with commit -p -m' +. ./test-lib.sh + +if ! test_have_prereq PERL +then + skip_all=skipping '$test_description' tests, perl not available + test_done +fi + +test_expect_success 'setup (initial)' ' + echo line1 file + git add file + git commit -m commit1 +' + +test_expect_failure 'edit hunk commit -p -m message' ' + test_when_finished rm -f editor_was_started + echo more file + echo e | env GIT_EDITOR=touch editor_was_started git commit -p -m commit2 file + test -r editor_was_started +' + +test_expect_failure 'edit hunk commit --dry-run -p -m message' ' + test_when_finished rm -f editor_was_started + echo more file + echo e | env GIT_EDITOR=touch editor_was_started git commit -p -m commit3 file + test -r editor_was_started +' + +test_done -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] test patch hunk editing with commit -p -m
Benoit Pierre benoit.pie...@gmail.com writes: +test_expect_failure 'edit hunk commit -p -m message' ' + test_when_finished rm -f editor_was_started Not just when finished, run rm -f here to make sure that the file does not exist. Later other people may add new tests before this test piece and affect the state of your throw-away working tree, and you would want to protect yourself from their changes. + echo more file + echo e | env GIT_EDITOR=touch editor_was_started git commit -p -m commit2 file Avoid touch unless you are interested in the timestamp to be updated. Use : editor_was_started or something like that when you are only interested in it was not there, now it is. The same comment applies to the next one. Thanks. + test -r editor_was_started +' + +test_expect_failure 'edit hunk commit --dry-run -p -m message' ' + test_when_finished rm -f editor_was_started + echo more file + echo e | env GIT_EDITOR=touch editor_was_started git commit -p -m commit3 file + test -r editor_was_started +' + +test_done -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] GSoC Change multiple if-else statements to be table-driven
Michael Haggerty mhag...@alum.mit.edu writes: On 03/17/2014 08:31 AM, Junio C Hamano wrote: So in short, yes it would have been nicer if we had more micros than candidates, but I do not think it was detrimental for the purpose of these micro exercises that multiple candidates ended up attempting the same micro. I wish I had had time to invent more microprojects. I think we were lucky: since students didn't seem to learn from each other's attempts, the fact that many people tried the same microprojects wasn't much of a problem. But if a student had arrived with a perfect solution to a well-trodden microproject on his/her first attempt, I would hate to have to be suspicious that the student plagiarized from somebody else's answer plus all of the review comments about the earlier answer, especially since a perfect solution would itself reduce the amount of interaction between the cheating student and the mailing list compared to a student who required several iterations. It may probably be not a huge problem. If anything, a cheater would have also learned how the mailing interaction goes when trying to plagiarise. And not really having interacted us, a cheater would have left us no impression on his or her communication skills, so it would probably have been detrimental for his or her own chance to be chosen as a student, compared to the ones who started from their own solution and polished it by responding to reviews. I also hope that students didn't feel unwelcomed during the times when the list of untaken microprojects was nearly empty. Yeah, that is why I said I was of multiple minds. I wasn't enthused by seeing the ones that somebody is attempting marked as taken in the first place. Solving one that others attempted in his or her own way and interacting with reviewers seemed to have just as much information on the candidate, at least to me. If we really wanted to make this system cheat-proof, there would have to be not only enough microprojects to go around, but also some mechanism to ensure that student B didn't start work on a microproject that student A was just about to submit a solution to. Nah, I do not think there is any need to go there. It is over for this year anyway, but I do not think such a provision is necessary for future years, if Git project will participate in future GSoCs; see above. Maybe in the future our microprojects should have two parts: a. Fix ... b. Invent a microproject for the next student. (Just kidding.) ;-) -- To unsubscribe from this list: send the line 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] rebase -i: replace an echo command by printf
Uwe Storbeck u...@ibr.ch writes: to avoid shell dependent behavior. Please do not start the body of the log message half-sentence. The title ought to be a freestanding title, not just a beginning half of a sentence that needs to be read with the rest to be understood. Something like this, perhaps. Subject: [PATCH] rebase -i: do not echo random user-supplied strings In some places we echo a string that come from a commit log message, which may have a backslash sequence that is interpreted by shells (POSIX.1 allows this), most notably dash. A commit message which contains the string '\n' (or ends with the string '\c') may result in a garbage line in the todo list of an interactive rebase which causes the rebase to fail. Will tentatively queue with the above rewrite, but if you feel strongly, please send an replacement. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
Jonathan Nieder jrnie...@gmail.com writes:
Uwe Storbeck wrote:
Backslash sequences are interpreted as control characters
by the echo command of some shells (e.g. dash).
This has bothered me for a while but never enough to do anything about
it. Thanks for fixing it.
Signed-off-by: Uwe Storbeck u...@ibr.ch
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
(patch left unsnipped for reference)
---
t/test-lib.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..8209204 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -277,7 +277,7 @@ error Test script did not set test_description.
if test $help = t
then
-echo $test_description
+printf '%s\n' $test_description
exit 0
fi
@@ -328,7 +328,7 @@ test_failure_ () {
test_failure=$(($test_failure + 1))
say_color error not ok $test_count - $1
shift
-echo $@ | sed -e 's/^/# /'
+printf '%s\n' $@ | sed -e 's/^/# /'
This is wrong, isn't it? Why do we want one line per item here?
test $immediate = || { GIT_EXIT_OK=t; exit 1; }
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] log: add --nonlinear-barrier to help see non-linear history
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
v2 renames the option name to --nonlinear-barrier and fixes using it
with --dense. Best used with --no-merges to see patch series.
I think that the earlier name show linear-break is more easily
understood than the new name, but maybe that is just me. It's not
like you are blocking something from going forward with a barrier,
and internally it is called a break-bar.
Documentation/rev-list-options.txt | 7 ++
log-tree.c | 4 +++
revision.c | 51
+++---
revision.h | 6 +
4 files changed, 65 insertions(+), 3 deletions(-)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 03533af..435aa2d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -750,6 +750,13 @@ This enables parent rewriting, see 'History
Simplification' below.
This implies the `--topo-order` option by default, but the
`--date-order` option may also be specified.
+--nonlinear-barrier[=barrier]::
+ When --graph is not used, all history branches are flatten and
s/flatten and/flattened and it/, perhaps?
+ could be hard to see that the two consecutive commits do not
+ belong to a linear branch. This option puts a barrier in
+ between them in that case. If `barrier` is specified, it
+ is the string that will be shown instead of the default one.
+
ifdef::git-rev-list[]
--count::
Print a number stating how many commits would have been
diff --git a/log-tree.c b/log-tree.c
index 08970bf..17862f6 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -805,12 +805,16 @@ int log_tree_commit(struct rev_info *opt, struct commit
*commit)
if (opt-line_level_traverse)
return line_log_print(opt, commit);
+ if (opt-track_linear !opt-linear !opt-reverse_output_stage)
+ printf(\n%s\n, opt-break_bar);
shown = log_tree_diff(opt, commit, log);
if (!shown opt-loginfo opt-always_show_header) {
log.parent = NULL;
show_log(opt);
shown = 1;
}
+ if (opt-track_linear !opt-linear opt-reverse_output_stage)
+ printf(\n%s\n, opt-break_bar);
Each time get_revision() returns a commit, it is inspected and
opt-linear is updated, this function is called and we show the
break in the single-strand-of-pearls if this is not a parent of the
commit we showed immediately before. If we are showing the commit
in reverse, we have to go the other way around, showing the commit
and then the break.
OK. It makes sense to me.
opt-loginfo = NULL;
maybe_flush_or_die(stdout, stdout);
return shown;
Does this new feature interact with -z format output in any way?
Should it, and if so how?
diff --git a/revision.c b/revision.c
index a68fde6..0a4849e 100644
--- a/revision.c
+++ b/revision.c
@@ -1832,6 +1832,12 @@ static int handle_revision_opt(struct rev_info *revs,
int argc, const char **arg
revs-notes_opt.use_default_notes = 1;
} else if (!strcmp(arg, --show-signature)) {
revs-show_signature = 1;
+ } else if (!strcmp(arg, --nonlinear-barrier)) {
+ revs-track_linear = 1;
+ revs-break_bar = ..;
+ } else if (starts_with(arg, --nonlinear-barrier=)) {
+ revs-track_linear = 1;
+ revs-break_bar = xstrdup(arg + 20);
} else if (starts_with(arg, --show-notes=) ||
starts_with(arg, --notes=)) {
struct strbuf buf = STRBUF_INIT;
@@ -2897,6 +2903,32 @@ enum commit_action simplify_commit(struct rev_info
*revs, struct commit *commit)
return action;
}
+define_commit_slab(saved_linear, int);
+
+static void track_linear(struct rev_info *revs, struct commit *commit)
+{
+ struct commit_list *p = revs-previous_parents;
+
+ if (p) {
+ int got_parent = 0;
+ for (; p !got_parent; p = p-next)
+ got_parent = !hashcmp(p-item-object.sha1,
+ commit-object.sha1);
+ revs-linear = got_parent;
+ free_commit_list(revs-previous_parents);
+ } else
+ revs-linear = 1;
+ if (revs-reverse) {
+ if (!revs-saved_linear_slab) {
+ revs-saved_linear_slab = xmalloc(sizeof(struct
saved_linear));
+ init_saved_linear(revs-saved_linear_slab);
+ }
+
+ *saved_linear_at(revs-saved_linear_slab, commit) =
revs-linear;
+ }
+ revs-previous_parents = copy_commit_list(commit-parents);
We are showing commit, and the parents (after history
simplification) of the commit we showed before this commit is kept
in
Re: [BUG] contrib/subtree: t/t7900-subtree.sh: test 21 fails when environment variable 'prefix' is set
Gilles Filippini gilles.filipp...@free.fr writes:
Test 21 from contrib/subtree/t/t7900-subtree.sh fails when an
environment variable 'prefix' is set. For instance here is what happens
when prefix=/usr:
expecting success:
echo You must provide the --prefix option. expected
test_must_fail git subtree split actual 21
test_debug printf 'expected: '
test_debug cat expected
test_debug printf 'actual: '
test_debug cat actual
test_cmp expected actual
rm -f expected actual
--- expected 2014-03-17 10:39:34.907594853 +
+++ actual2014-03-17 10:39:34.979595322 +
@@ -1 +1,9 @@
-You must provide the --prefix option.
fatal: /usr: '/usr' is outside repository
fatal: /usr: '/usr' is outside repository
fatal: /usr: '/usr' is outside repository
fatal: /usr: '/usr' is outside repository
fatal: /usr: '/usr' is outside repository
fatal: /usr: '/usr' is outside repository
fatal: /usr: '/usr' is outside repository
fatal: /usr: '/usr' is outside repository
+No new revisions were found
not ok 21 - Check that prefix argument is required for split
Thanks. Although I do not use nor touch git-subtree, I am
reasonably confident that this patch should fix it.
-- 8 --
Subject: subtree: initialise all variables to known state
Parsing the command line options may set prefix to the given value
with --prefix=value or an empty string with --no-prefix, but the
script forgets to protect against a stray environment variable.
Make sure all the variables that can be assigned in the command line
parsing are initialized to empty.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
contrib/subtree/git-subtree.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7d7af03..90e0b7e 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -46,6 +46,7 @@ ignore_joins=
annotate=
squash=
message=
+prefix=
debug()
{
--
To unsubscribe from this list: send the line 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] tests: set temp variables using 'env' in test function instead of subshell
David Tran unsignedz...@gmail.com writes: Fixed the broken -chain and the tests run correctly. The double env is fixed to be a single env. The useless subshells are removed. ... Hmph. test_expect_success 'need valid notes ref' ' - (MSG=1 GIT_NOTES_REF=/ export MSG GIT_NOTES_REF - test_must_fail git notes add) - (MSG=2 GIT_NOTES_REF=/ export MSG GIT_NOTES_REF - test_must_fail git notes show) + test_must_fail env MSG=1 env GIT_NOTES_REF=/ git notes show + test_must_fail env MSG=2 env GIT_NOTES_REF=/ git notes show ' Oh, really ;-)? test_expect_success 'refusing to add notes in refs/heads/' ' - (MSG=1 GIT_NOTES_REF=refs/heads/bogus - export MSG GIT_NOTES_REF - test_must_fail git notes add) + test_must_fail env MSG=1 GIT_NOTES_REF=refs/heads/bogus git notes add ' This one is good. @@ -529,9 +510,7 @@ test_expect_success 'aborted --continue does not squash commits after edit' ' echo edited again file7 git add file7 ( - FAKE_COMMIT_MESSAGE= - export FAKE_COMMIT_MESSAGE - test_must_fail git rebase --continue + test_must_fail env FAKE_COMMIT_MESSAGE= git rebase --continue ) Do we do anything to cause us misbehave if the above is done outside the subshell? Thanks. Getting closer, I think. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] test patch hunk editing with commit -p -m
Benoit Pierre benoit.pie...@gmail.com writes: On Mon, Mar 17, 2014 at 7:49 PM, Junio C Hamano gits...@pobox.com wrote: Benoit Pierre benoit.pie...@gmail.com writes: Add (failing) tests: with commit changing the environment to let hooks know that no editor will be used (by setting GIT_EDITOR to :), the edit hunk functionality does not work (no editor is launched and the whole hunk is committed). Signed-off-by: Benoit Pierre benoit.pie...@gmail.com --- t/t7513-commit-patch.sh | 32 1 file changed, 32 insertions(+) create mode 100755 t/t7513-commit-patch.sh diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh Again, as I said, I'll rename this to t7514-commit.patch.sh while I queue this. I assumed the 14 was a typo, will rename, but to t7514-commit-patch.sh right? (with 2 '-'). Thanks, yes. That is how I queued the previous one on 'pu', I think. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] test patch hunk editing with commit -p -m
Benoit Pierre benoit.pie...@gmail.com writes: Isn't the point of using when finished to have each test leave the tree clean after execution, to avoid bleeding onto the next test(s)? But you cannot anticipate what other people will do in the future before you have a chance to run this piece. Your using when-finished is courtesy for others. Your explicitly making sure what you do not want to be stray behind is protecting yourself from others. They serve different purposes, and you need both. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
