[PATCH v2] utf8.c: fix strbuf_utf8_replace() consuming data beyond input string
The main loop in strbuf_utf8_replace() could summed up as:
while ('src' is still valid) {
1) advance 'src' to copy ANSI escape sequences
2) advance 'src' to copy/replace visible characters
}
The problem is after #1, 'src' may have reached the end of the string
(so 'src' points to NUL) and #2 will continue to copy that NUL as if
it's a normal character. Because the output is stored in a strbuf,
this NULL accounted in the 'len' field as well. Check after #1 and
break the loop if necessary.
The test does not look obvious, but the combination of %>>() should
make a call trace like this
show_log()
pretty_print_commit()
format_commit_message()
strbuf_expand()
format_commit_item()
format_and_pad_commit()
strbuf_utf8_replace()
where %C(auto)%d would insert a color reset escape sequence in the end
of the string given to strbuf_utf8_replace() and show_log() uses
fwrite() to send everything to stdout (including the incorrect NUL
inserted by strbuf_utf8_replace)
Signed-off-by: Nguyễn Thái Ngọc Duy
---
t/t4205-log-pretty-formats.sh | 7 +++
utf8.c| 3 +++
2 files changed, 10 insertions(+)
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 349c531..de0cc4a 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -431,6 +431,13 @@ EOF
test_cmp expected actual
'
+test_expect_success 'strbuf_utf8_replace() not producing NUL' '
+ git log --color
--pretty="tformat:%<(10,trunc)%s%>>(10,ltrunc)%C(auto)%d" |
+ test_decode_color |
+ nul_to_q >actual &&
+ ! grep Q actual
+'
+
# get new digests (with no abbreviations)
head1=$(git rev-parse --verify HEAD~0) &&
head2=$(git rev-parse --verify HEAD~1) &&
diff --git a/utf8.c b/utf8.c
index b30790d..401a6a5 100644
--- a/utf8.c
+++ b/utf8.c
@@ -382,6 +382,9 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos,
int width,
dst += n;
}
+ if (src >= end)
+ break;
+
old = src;
n = utf8_width((const char**)&src, NULL);
if (!src) /* broken utf-8, do nothing */
--
2.1.0.rc0.78.gc0d8480
--
To unsubscribe from this list: send the line "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] git-gui: make gc warning threshold match 'git gc --auto'
Karsten Blees writes:
>The number of loose objects at which git-gui shows a gc warning has
>historically been hardcoded to ~2000, or ~200 on Windows. The warning can
>only be disabled completely via gui.gcwarning=false.
>
>Especially on Windows, the hardcoded threshold is so ridiculously low that
>git-gui often complains even immediately after gc (due to loose objects
>only referenced by the reflog).
>
>'git gc --auto' uses a much bigger threshold to check if gc is necessary.
>Additionally, the value can be configured via gc.auto (default 6700).
>There's no special case for Windows.
>
>Change git-gui so that it only warns if 'git gc --auto' would also do an
>automatic gc, i.e.:
> - calculate the threshold from the gc.auto setting (default 6700,
> disabled if <= 0)
> - check directory .git/objects/17
>
>We still check four directories (14-17) if gc.auto is very small, to get a
>better estimate.
>
>Signed-off-by: Karsten Blees
>---
> git-gui/lib/database.tcl | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
>diff --git a/git-gui/lib/database.tcl b/git-gui/lib/database.tcl
>index 1f187ed..212b195 100644
>--- a/git-gui/lib/database.tcl
>+++ b/git-gui/lib/database.tcl
>@@ -89,19 +89,26 @@ proc do_fsck_objects {} {
> }
>
> proc hint_gc {} {
>+ global repo_config
>+ set auto_gc $repo_config(gc.auto)
>+ if {$auto_gc eq {}} {
>+ set auto_gc 6700
>+ } elseif {$auto_gc <= 0} {
>+ return
>+ }
>+
> set ndirs 1
>- set limit 8
>- if {[is_Windows]} {
>+ set limit [expr {($auto_gc + 255) / 256}]
>+ if {$limit < 4} {
> set ndirs 4
>- set limit 1
> }
>
> set count [llength [glob \
> -nocomplain \
> -- \
>- [gitdir objects 4\[0-[expr {$ndirs-1}]\]/*]]]
>+ [gitdir objects 1\[[expr {8-$ndirs}]-7\]/*]]]
>
>- if {$count >= $limit * $ndirs} {
>+ if {$count > $limit * $ndirs} {
> set objects_current [expr {$count * 256/$ndirs}]
> if {[ask_popup \
> [mc "This repository currently has approximately %i
> loose objects.
Applying this to git-gui I get an error raised
can't read "repo_config(gc.auto)": no such element in array
which occurs because I've never set this config variable and it is not
present in the default_config array which is used to initialize repo_config.
The following should solve this by moving where we ser the default:
>From bdb136cbcb2a3fc0e3441f852e4bf4525ee4cf99 Mon Sep 17 00:00:00 2001
From: Pat Thoyts
Date: Sun, 10 Aug 2014 11:36:47 +0100
Subject: [PATCH] git-gui: initialize the default value for gc.auto in case it
is unset.
Signed-off-by: Pat Thoyts
---
git-gui.sh | 1 +
lib/database.tcl | 4 +---
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/git-gui.sh b/git-gui.sh
index b186329..ee9c47b 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -880,6 +880,7 @@ proc apply_config {} {
}
set default_config(branch.autosetupmerge) true
+set default_config(gc.auto) 6700
set default_config(merge.tool) {}
set default_config(mergetool.keepbackup) true
set default_config(merge.diffstat) true
diff --git a/lib/database.tcl b/lib/database.tcl
index 212b195..d9ba323 100644
--- a/lib/database.tcl
+++ b/lib/database.tcl
@@ -91,9 +91,7 @@ proc do_fsck_objects {} {
proc hint_gc {} {
global repo_config
set auto_gc $repo_config(gc.auto)
- if {$auto_gc eq {}} {
- set auto_gc 6700
- } elseif {$auto_gc <= 0} {
+ if {$auto_gc <= 0} {
return
}
--
Pat Thoytshttp://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD
--
To unsubscribe from this list: send the line "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] git-gui: Make git-gui lib dir configurable at runtime
David Turner writes: >On Mon, 2014-07-21 at 14:06 -0700, Junio C Hamano wrote: >> David Turner writes: >> >> > Introduce the GIT_GUI_LIB_DIR environment variable, to tell git-gui >> > where to look for TCL libs. This allows a git-gui which has been >> > built with a prefix of /foo to be run out of directory /bar. This is >> > the equivalent of GIT_EXEC_PATH or GITPERLLIB but for git-gui's TCL >> > libraries. >> > >> > Signed-off-by: David Turner >> > --- >> > git-gui/Makefile | 3 ++- >> > git-gui/git-gui.sh | 6 +- >> > 2 files changed, 7 insertions(+), 2 deletions(-) >> >> Would a similar change to gitk necessary/beneficial to platforms >> that would benefit from this change? > >Apparently not; it seems to work fine for me from an alternate location. >Convenient! > >> git-gui directory in my tree comes from its upstream repository >> git://repo.or.cz/git-gui.git/, and it is maintained by Pat Thoyts >> (Cc'ed). > >> Note that these two upstream projects do not have leading >> directories git-gui and gitk-git themselves (they are merged to my >> tree while their paths being renamed). A patch that is appliable to >> them would touch paths without them (e.g. Makefile and git-gui.sh >> for an equivalent of the patch I am responding to). > >Pat, do you want patches via the git mailing list, personal mail, or >some other way? > The standard method is both: personal to ensure I see it and mailing list to allow everyone to comment. I've applied this patch to git-gui master. -- Pat Thoytshttp://www.patthoyts.tk/ PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] clone.c: don't leak memory in cmd_clone
Free the refspec. Found by scan.coverity.com (Id: 1127806) Signed-off-by: Stefan Beller --- builtin/clone.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/clone.c b/builtin/clone.c index bbd169c..dd4092b 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1004,5 +1004,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) strbuf_release(&key); strbuf_release(&value); junk_mode = JUNK_LEAVE_ALL; + + free(refspec); return err; } -- 2.1.0.rc2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] remote.c: don't leak the base branch name in format_tracking_info
Found by scan.coverity.com (Id: 1127809)
Signed-off-by: Stefan Beller
---
remote.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/remote.c b/remote.c
index 3d6c86a..2c1458f 100644
--- a/remote.c
+++ b/remote.c
@@ -1983,6 +1983,7 @@ int format_tracking_info(struct branch *branch, struct
strbuf *sb)
strbuf_addf(sb,
_(" (use \"git pull\" to merge the remote
branch into yours)\n"));
}
+ free(base);
return 1;
}
--
2.1.0.rc2
--
To unsubscribe from this list: send the line "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] Fixing unclear messages
Signed-off-by: Alexander Shopov
---
builtin/clean.c | 2 +-
builtin/commit.c | 2 +-
builtin/index-pack.c | 4 ++--
builtin/log.c| 4 ++--
builtin/merge.c | 5 +++--
builtin/remote.c | 2 +-
builtin/tag.c| 2 +-
git-bisect.sh| 2 +-
merge-recursive.c| 2 +-
9 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 1032563..9f38068 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -514,7 +514,7 @@ static int parse_choice(struct menu_stuff *menu_stuff,
if (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom >
top ||
(is_single && bottom != top)) {
clean_print_color(CLEAN_COLOR_ERROR);
- printf_ln(_("Huh (%s)?"), (*ptr)->buf);
+ printf_ln(_("Wrong choice (%s). Choose again."),
(*ptr)->buf);
clean_print_color(CLEAN_COLOR_RESET);
continue;
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 5ed6036..cdc8a4e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1198,7 +1198,7 @@ static int parse_and_validate_options(int argc, const
char *argv[],
if (argc == 0 && (also || (only && !amend)))
die(_("No paths with --include/--only does not make sense."));
if (argc == 0 && only && amend)
- only_include_assumed = _("Clever... amending the last one with
dirty index.");
+ only_include_assumed = _("You are amending the last commit
only. There are additional changes in the index.");
if (argc > 0 && !also && !only)
only_include_assumed = _("Explicit paths specified without -i
or -o; assuming --only paths...");
if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 5568a5b..d9c5911 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1064,7 +1064,7 @@ static void parse_pack_objects(unsigned char *sha1)
nr_delays--;
}
if (nr_delays)
- die(_("confusion beyond insanity in parse_pack_objects()"));
+ die(_("fatal error in function \"parse_pack_objects\". This is
a bug in Git. Please report it to the developers with an e-mail to
git@vger.kernel.org."));
}
/*
@@ -1139,7 +1139,7 @@ static void conclude_pack(int fix_thin_pack, const char
*curr_pack, unsigned cha
int nr_unresolved = nr_deltas - nr_resolved_deltas;
int nr_objects_initial = nr_objects;
if (nr_unresolved <= 0)
- die(_("confusion beyond insanity"));
+ die(_("fatal error in function \"conclude_pack\". This
is a bug in Git. Please report it to the developers with an e-mail to
git@vger.kernel.org."));
objects = xrealloc(objects,
(nr_objects + nr_unresolved + 1)
* sizeof(*objects));
diff --git a/builtin/log.c b/builtin/log.c
index 4389722..d614a20 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -992,7 +992,7 @@ static const char *clean_message_id(const char *msg_id)
m++;
}
if (!z)
- die(_("insane in-reply-to: %s"), msg_id);
+ die(_("wrong format for the \"in-reply-to\" header: %s"),
msg_id);
if (++z == m)
return a;
return xmemdupz(a, z - a);
@@ -1065,7 +1065,7 @@ static int output_directory_callback(const struct option
*opt, const char *arg,
{
const char **dir = (const char **)opt->value;
if (*dir)
- die(_("Two output directories?"));
+ die(_("Maximum one output directory is allowed."));
*dir = arg;
return 0;
}
diff --git a/builtin/merge.c b/builtin/merge.c
index ce82eb2..e92a74a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -842,7 +842,8 @@ static int merge_trivial(struct commit *head, struct
commit_list *remoteheads)
struct commit_list *parents, **pptr = &parents;
write_tree_trivial(result_tree);
- printf(_("Wonderful.\n"));
+ printf(_("The first part of the trivial merge finished successfully
+.\n"));
pptr = commit_list_append(head, pptr);
pptr = commit_list_append(remoteheads->item, pptr);
prepare_to_commit(remoteheads);
@@ -1400,7 +1401,7 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
ret = merge_trivial(head_commit, remoteheads);
goto done;
}
- printf(_("Nope.\n"));
+ printf(_("Merge was not successful.\n"));
}
} else {
/*
diff --git a/builtin/remote.c b/builtin/remote.c
index 9a4640d..3f480b1 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -951,7 +951,7 @@ static int sh
Fixing WTF porcelain messages in Git
Hello everyone, I have just finished the Bulgarian translation of git. Doing this allowed me to identify about 90 places where the original messages can be improved - for consistency, spaces/tabs, abbreviations etc. 12 of these places however are really WTF messages - they do not explain properly what is wrong or right about the current state i.e. the user does not know what the problem is wrong and is left clueless how to fix things. I will send a patch with my suggestion for improving these 12 messages. Fixing these will provide most value for the final user of git. The rest of improvements can wait. I will add the maintainer of i18n, every active translator of git to CC so they can react accordingly or just add suggestions. I think fixing these in the 2.1 timeframe is not possible as the state of the translations will suffer plus there needs to be at least some minimal discussion as I can have botched the messages. I propose fixing them in early 2.1 maintenance releases or 2.2 latest. Just FYI, the set of WTFs is: #: merge-recursive.c:1861 msgid "Unprocessed path??? %s" #: builtin/clean.c:517 msgid "Huh (%s)?" #: builtin/commit.c:1201 msgid "Clever... amending the last one with dirty index." #: builtin/index-pack.c:1067 msgid "confusion beyond insanity in parse_pack_objects()" #: builtin/index-pack.c:1142 msgid "confusion beyond insanity" #: builtin/log.c:995 msgid "insane in-reply-to: %s" #: builtin/log.c:1068 msgid "Two output directories?" #: builtin/merge.c:845 msgid "Wonderful.\n" #: builtin/merge.c:1403 msgid "Nope.\n" #: builtin/remote.c:954 msgid " ???" #: builtin/tag.c:504 msgid "no tag message?" #: git-bisect.sh:424 msgid "?? what are you talking about?" Kind regards: al_shopov -- To unsubscribe from this list: send the line "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] Fixning WTF porcelain messages
Some user-facing porcelain messages in Git are hard to understand and hard to translate. They confuse users winthout providing proper information and course of action. Here is the list of these messages (starting with "-") and my suggestions (starting with "+"). The patch follows in the next letter. -Huh (%s)? +Wrong choice (%s). Choose again. -Clever... amending the last one with dirty index. +You are amending the last commit only. There are additional changes in the index. -confusion beyond insanity in parse_pack_objects() +fatal error in function "parse_pack_objects". This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org. -confusion beyond insanity +fatal error in function "conclude_pack". This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org. -insane in-reply-to: %s +wrong format for the "in-reply-to" header: %s -Two output directories? +Maximum one output directory is allowed. -Wonderful.\n +The first part of the trivial merge finished successfully.\n -Nope.\n +Merge was not successful.\n - ??? + unknown state -no tag message? +missing tag message -?? what are you talking about? +unknown command. Only "start", "good", "bad" and "skip" are possible. -Unprocessed path??? %s +The path "%s" was not processed but it had to be. This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org. Alexander Shopov (1): Fixing unclear messages builtin/clean.c | 2 +- builtin/commit.c | 2 +- builtin/index-pack.c | 4 ++-- builtin/log.c| 4 ++-- builtin/merge.c | 5 +++-- builtin/remote.c | 2 +- builtin/tag.c| 2 +- git-bisect.sh| 2 +- merge-recursive.c| 2 +- 9 files changed, 13 insertions(+), 12 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] remote.c: don't leak the base branch name in format_tracking_info
On 10.08.2014 15:57, Stefan Beller wrote:
> Found by scan.coverity.com (Id: 1127809)
>
> Signed-off-by: Stefan Beller
> ---
> remote.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/remote.c b/remote.c
> index 3d6c86a..2c1458f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1983,6 +1983,7 @@ int format_tracking_info(struct branch *branch, struct
> strbuf *sb)
> strbuf_addf(sb,
> _(" (use \"git pull\" to merge the remote
> branch into yours)\n"));
> }
> + free(base);
> return 1;
> }
>
>
Upon testing this one again, I get a warning
remote.c: In function ‘format_tracking_info’:
remote.c:1986:2: warning: passing argument 1 of ‘free’ discards ‘const’
qualifier from pointer target type [enabled by default]
free(base);
^
In file included from git-compat-util.h:103:0,
from cache.h:4,
from remote.c:1:
/usr/include/stdlib.h:483:13: note: expected ‘void *’ but argument is of type
‘const char *’
extern void free (void *__ptr) __THROW;
^
Please ignore 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: Unify subcommand structure; introduce double dashes for all subcommands?
On 23.07.2014 19:52, Junio C Hamano wrote: > Stefan Beller writes: > >> A git command is generally setup as: >> git [] [] ... >> >> The subcommands vary wildly by the nature of the command. However all >> subcommands >> could at least follow one style. The commands bundle, notes, stash and >> submodule >> have subcommands without two leading dashes (i.e. git stash list) as >> opposed to all >> other commands (i.e. git tag --list). > > Sounds familiar. E.g. here is a similar thread about a year ago. > > http://thread.gmane.org/gmane.comp.version-control.git/231376/focus=231478 > > Further discussions to make the plan more concrete is very much > welcomed. > > Thanks. > So I'd want to add have the subcommands without double dashes ideally. It's just less typing and more english like, not cryptic with dashes. This may come handy for newcomers/beginners using git. It also benefits the powerusers of git as you spare the typing of 2 dashes on the subcommand. I'm currently trying to understand the functions to parse options and I wonder if the following is possible: If there is an option set by OPT_CMDMODE (such as git tag --delete / --verify) we want to add an internal option, (such as PARSE_OPT_NODASH ?), that you can deliver this CMD_MODE option without leading 2 dashes. The current behavior with leading 2 dashes is still supported, but maybe a warning is printed about deprecating the option with 2 dashes. Having this change in place will switch over revert, replace, tag and merge-base to having sane subcommands without dashes possible. Once this change is in, we can rewrite the other commands, which as of now don't require the dashes. Coincidentally these commands heavily rely on option parsing themselves, such as git-notes having this code in place: if (argc < 1 || !strcmp(argv[0], "list")) result = list(argc, argv, prefix); else if (!strcmp(argv[0], "add")) result = add(argc, argv, prefix); else if (!strcmp(argv[0], "copy")) result = copy(argc, argv, prefix); ... Does that make sense? Stefan -- To unsubscribe from this list: send the line "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 v8 0/8] Rewrite `git_config()` using config-set API
Ramsay Jones writes: > On 08/08/14 15:07, Tanay Abhra wrote: > ... >> (cc to Ramsay) >> >> The discussion in both threads (v8 and v9), boils down to this, >> is the `key_value_info` struct really required to be declared public or >> should be >> just an implementation detail. I will give you the context, > > No, this is not the issue for me. The patch which introduces the > struct in cache.h does not make use of that struct in any interface. > It *is* an implementation detail of some code in config.c only. > > I do not know how that structure will be used in future patches. ;-) It is debatable if it is a good API that tells the users "In the data I return to you, there is a structure hanging there with these two fields. Feel free to peek into it if you need what is recorded in them". But the contract between the the endgame "API" and its callers can include such a direct access, and then you can say that it is a part of the API, not just an implementation detail. I think you and Tanay are both right (and I am wrong ;-). You are right in that the "API" is giving more than the callers converted to it at this point in the series. Tanay is right in that the way the struct will be used, which is illustrated by the example in the message you are responding to, should be in the part of this series that gives the implementation of the API before presenting the converted users, as the series deems it part of the API to let the users peek into the struct. It may turn out that we have to abstract it further when we need a more elaborate data structure kept in the kv-info in the future. At that point it will become undesirable to keep giving the callers direct access to it, because direct struct access means that the particular aspect of the implementaiton detail will be cast in stone and would not allow to be replaced with some other representation that is more efficient for the implementation. But as I said, what we see in this series can do for now. The future can come later ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unify subcommand structure; introduce double dashes for all subcommands?
Stefan Beller writes: > On 23.07.2014 19:52, Junio C Hamano wrote: > >> Sounds familiar. E.g. here is a similar thread about a year ago. >> >> http://thread.gmane.org/gmane.comp.version-control.git/231376/focus=231478 >> >> Further discussions to make the plan more concrete is very much >> welcomed. >> >> Thanks. >> > > So I'd want to add have the subcommands without double dashes ideally. That is not ideal at all, I am afraid. A command that started only with its "primary operating mode", e.g. "git tag [-s|-a] tagname [object]", may have to gain "I do not want to create, I just want to list" and the way to signal that has to be an option that cannot be mistaken as its valid first argument (to avoid "git tag list" that cannot create a tag called "list", we use "git tag --list"). You could add an entirely new command "git foo" that always takes the command-mode word, i.e. "git foo mode$n args", but you will be typing the operating mode name all the time only to save --mode$n for 2<=$n, which may not be a good economy in the end. Please do not go there. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] remote.c: don't leak the base branch name in format_tracking_info
On Sun, Aug 10, 2014 at 05:14:33PM +0200, Stefan Beller wrote:
> On 10.08.2014 15:57, Stefan Beller wrote:
> > Found by scan.coverity.com (Id: 1127809)
> >
> > Signed-off-by: Stefan Beller
> > ---
> > remote.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/remote.c b/remote.c
> > index 3d6c86a..2c1458f 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -1983,6 +1983,7 @@ int format_tracking_info(struct branch *branch,
> > struct strbuf *sb)
> > strbuf_addf(sb,
> > _(" (use \"git pull\" to merge the remote
> > branch into yours)\n"));
> > }
> > + free(base);
> > return 1;
> > }
> >
> >
>
> Upon testing this one again, I get a warning
> remote.c: In function ‘format_tracking_info’:
> remote.c:1986:2: warning: passing argument 1 of ‘free’ discards ‘const’
> qualifier from pointer target type [enabled by default]
> free(base);
> ^
> In file included from git-compat-util.h:103:0,
> from cache.h:4,
> from remote.c:1:
> /usr/include/stdlib.h:483:13: note: expected ‘void *’ but argument is of type
> ‘const char *’
> extern void free (void *__ptr) __THROW;
> ^
>
> Please ignore this patch.
I think your patch is definitely fixing a leak; it's just that the
existing code is a little sloppy. It does:
const char *base;
...
base = branch->merge[0]->dst;
base = shorten_unambiguous_ref(base, 0);
In the first assignment, "base" should be const, as we are pointing to
somebody else's memory. But in the second, we use the same pointer to
store newly allocated memory from shorten_unambiguous_ref.
In the general case, you need two pointers to do this right. However, we
don't actually look at "base" between the two assignments, so I think
you could just do it as:
char *base;
...
base = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fixing unclear messages
On Sun, Aug 10, 2014 at 06:13:27PM +0300, Alexander Shopov wrote:
> Signed-off-by: Alexander Shopov
It would probably make sense to put the discussion from your cover
letter into the commit message.
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 5568a5b..d9c5911 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1064,7 +1064,7 @@ static void parse_pack_objects(unsigned char *sha1)
> nr_delays--;
> }
> if (nr_delays)
> - die(_("confusion beyond insanity in parse_pack_objects()"));
> + die(_("fatal error in function \"parse_pack_objects\". This is
> a bug in Git. Please report it to the developers with an e-mail to
> git@vger.kernel.org."));
> }
We usually just say:
die("BUG: ...");
here (and hopefully the "..." actually describes the situation a bit
better). I have wondered if we should actually introduce a
BUG("...");
function. Then it would make it simple to be more verbose (e.g.,
pointing the user to the mailing list as you do here) without having to
repeat the text in each place.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] remote.c: don't leak the base branch name in format_tracking_info
Found by scan.coverity.com (Id: 1127809)
Signed-off-by: Stefan Beller
---
remote.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/remote.c b/remote.c
index 3d6c86a..894db09 100644
--- a/remote.c
+++ b/remote.c
@@ -1920,7 +1920,7 @@ int stat_tracking_info(struct branch *branch, int
*num_ours, int *num_theirs)
int format_tracking_info(struct branch *branch, struct strbuf *sb)
{
int ours, theirs;
- const char *base;
+ char *base;
int upstream_is_gone = 0;
switch (stat_tracking_info(branch, &ours, &theirs)) {
@@ -1936,8 +1936,7 @@ int format_tracking_info(struct branch *branch, struct
strbuf *sb)
break;
}
- base = branch->merge[0]->dst;
- base = shorten_unambiguous_ref(base, 0);
+ base = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
if (upstream_is_gone) {
strbuf_addf(sb,
_("Your branch is based on '%s', but the upstream is
gone.\n"),
@@ -1983,6 +1982,7 @@ int format_tracking_info(struct branch *branch, struct
strbuf *sb)
strbuf_addf(sb,
_(" (use \"git pull\" to merge the remote
branch into yours)\n"));
}
+ free(base);
return 1;
}
--
2.1.0.rc2
--
To unsubscribe from this list: send the line "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] Update hard-coded header dependencies
On Fri, Aug 08, 2014 at 02:58:26PM -0700, Jonathan Nieder wrote: > Maybe it's worth switching to plain > > LIB_H += $(wildcard *.h) > > ? People using ancient compilers that never change headers wouldn't > be hurt, people using modern compilers that do change headers also > wouldn't be hurt, and we could stop pretending to maintain an > up-to-date list. Yeah, I think that makes sense. I'd imagine most of the developers are on a modern platform and don't use the static list at all, so we don't notice when it breaks (and even when you do use it, it's quite hard to notice anyway). We'd have to do a multi-directory wildcard, though, to catch the header files stuck in compat/* and elsewhere. We could list the containing directories manually, but that's yet another thing to go wrong. For people using the git repo, it would probably be fine to do: LIB_H += $(shell git ls-files -- '*.h') That wouldn't count new files a developer adds until they "git add" some version of the file, but that is not so bad (right now they have to add it to the Makefile, and anyway, I think most devs are using the computed dependencies). But that doesn't work for distributed tarballs, which would have to convert that to a static list somehow. Maybe LIB_H += $(shell find . -name '*.h' -print) would work? -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unify subcommand structure; introduce double dashes for all subcommands?
On 10.08.2014 20:13, Junio C Hamano wrote: > Stefan Beller writes: > >> On 23.07.2014 19:52, Junio C Hamano wrote: >> >>> Sounds familiar. E.g. here is a similar thread about a year ago. >>> >>> http://thread.gmane.org/gmane.comp.version-control.git/231376/focus=231478 >>> >>> Further discussions to make the plan more concrete is very much >>> welcomed. >>> >>> Thanks. >>> >> >> So I'd want to add have the subcommands without double dashes ideally. > > That is not ideal at all, I am afraid. A command that started only > with its "primary operating mode", e.g. "git tag [-s|-a] tagname > [object]", may have to gain "I do not want to create, I just want to > list" and the way to signal that has to be an option that cannot be > mistaken as its valid first argument (to avoid "git tag list" that > cannot create a tag called "list", we use "git tag --list"). You > could add an entirely new command "git foo" that always takes the > command-mode word, i.e. "git foo mode$n args", but you will be > typing the operating mode name all the time only to save --mode$n > for 2<=$n, which may not be a good economy in the end. > > Please do not go there. > I see your point. However how often do you really want to create a tag called list? As of now it's easy: git tag list and for listing all tags you'd need to type: git tag --list and if you want to create a tag called --list, I'd assume you'd go git tag -- --list # However: fatal: '--list' is not a valid tag name. So even as of now certain tag names cannot be done easily. Also you have to type two more dashes for an action you'd probably want to perform more often (as opposed to creating a tag 'list') In my (ideal) world we'd rather have this behavior: git tag list # behaves the same as git tag Now creating a tag called 'list' is not as easy, because 'list' is a primary operating mode name, so we need to tell git we're actually meaning the name as opposed to the operating mode: git tag create -- list # or even git tag create -- --list Anyways despite my arguing, it seems you rather want to rather have the leading double dashes everywhere for the operating modes? So the plan is to not touch the parsing, but to adjust notes and stash ? Thanks, Stefan -- To unsubscribe from this list: send the line "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] checkpatch: Add test for commit id formatting style in commit log
Hi Joe,
On Thu, Jul 3, 2014 at 12:00 AM, Joe Perches wrote:
> Commit logs have various forms of commit id references.
>
> Try to standardize on a 12 character long lower case
> commit id along with a description of parentheses and
> the quoted subject line
>
> ie: commit 0123456789ab ("commit description")
Now this is in mainline, checkpatch starts complaining about my "too long"
(40 chars) commit IDs in commit messages :-(
40 chars may be too long (but it's quick to copy-and-paste, as "git show"
shows that by default), but 12 sounds a bit short, as that's only 48 bits.
According to the Birthday Paradox (en.wikipedia.org/wiki/Birthday_problem),
there's a probability of 50% of a collision if you use 48 bits IDs in a
repository with ca. 16 milion (2^24) objects. A Linux kernel repository
counts ca. 4 million objects, so we're getting close...
So soon we'll get "error: short SHA1 is ambiguous".
BTW, is there actually an easy way to make "git show" show all options for
an ambiguous SHA1?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "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] checkpatch: Add test for commit id formatting style in commit log
On Sun, 10 Aug 2014 14:28:01 -0700 Joe Perches wrote:
> > On Thu, Jul 3, 2014 at 12:00 AM, Joe Perches wrote:
> > > Commit logs have various forms of commit id references.
> > >
> > > Try to standardize on a 12 character long lower case
> > > commit id along with a description of parentheses and
> > > the quoted subject line
> > >
> > > ie: commit 0123456789ab ("commit description")
> >
> > Now this is in mainline, checkpatch starts complaining about my "too long"
> > (40 chars) commit IDs in commit messages :-(
> >
> > 40 chars may be too long (but it's quick to copy-and-paste, as "git show"
> > shows that by default), but 12 sounds a bit short, as that's only 48 bits.
>
> Right now, this test allows 12 to 16 byte length commit ids
> without emitting a warning.
>
> Andrew wanted this test, I don't care how long the commit id
> is in the commit log.
Well, I mainly wanted to stop having to add "commit description" when
people forget it. The length check was perhaps a bit anal. How about
we make it "12 or more"?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] blame.c: Add translation to warning about failed revision walk
blame belonging to the group of
ancillaryinterrogators and not to plumbinginterrogators
should have localized error messages?
Signed-off-by: Stefan Beller
---
builtin/blame.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 17d30d0..ca4ba6f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2700,7 +2700,7 @@ parse_done:
* uninteresting.
*/
if (prepare_revision_walk(&revs))
- die("revision walk setup failed");
+ die(_("revision walk setup failed"));
if (is_null_sha1(sb.final->object.sha1)) {
o = sb.final->util;
--
2.1.0.rc2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] prepare_revision_walk: Check for return value in all places
Even the documentation tells us:
You should check if it
returns any error (non-zero return code) and if it does not, you can
start using get_revision() to do the iteration.
In preparation for this commit, I grepped all occurrences of
prepare_revision_walk and added error messages, when there were none.
Signed-off-by: Stefan Beller
---
builtin/branch.c | 4 +++-
builtin/commit.c | 3 ++-
remote.c | 3 ++-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 0591b22..ced422b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -653,7 +653,9 @@ static int print_ref_list(int kinds, int detached, int
verbose, int abbrev, stru
add_pending_object(&ref_list.revs,
(struct object *) filter, "");
ref_list.revs.limited = 1;
- prepare_revision_walk(&ref_list.revs);
+
+ if (prepare_revision_walk(&ref_list.revs))
+ die(_("revision walk setup failed"));
if (verbose)
ref_list.maxwidth = calc_maxwidth(&ref_list);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 7867768..bb84e1d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1055,7 +1055,8 @@ static const char *find_author_by_nickname(const char
*name)
revs.mailmap = &mailmap;
read_mailmap(revs.mailmap, NULL);
- prepare_revision_walk(&revs);
+ if (prepare_revision_walk(&revs))
+ die("revision walk setup failed");
commit = get_revision(&revs);
if (commit) {
struct pretty_print_context ctx = {0};
diff --git a/remote.c b/remote.c
index 894db09..112e4d5 100644
--- a/remote.c
+++ b/remote.c
@@ -1893,7 +1893,8 @@ int stat_tracking_info(struct branch *branch, int
*num_ours, int *num_theirs)
init_revisions(&revs, NULL);
setup_revisions(rev_argc, rev_argv, &revs, NULL);
- prepare_revision_walk(&revs);
+ if (prepare_revision_walk(&revs))
+ die("revision walk setup failed");
/* ... and count the commits on each side. */
*num_ours = 0;
--
2.1.0.rc2
--
To unsubscribe from this list: send the line "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] checkpatch: Add test for commit id formatting style in commit log
On Sun, 2014-08-10 at 23:08 +0200, Geert Uytterhoeven wrote:
> Hi Joe,
Hi Geert.
> On Thu, Jul 3, 2014 at 12:00 AM, Joe Perches wrote:
> > Commit logs have various forms of commit id references.
> >
> > Try to standardize on a 12 character long lower case
> > commit id along with a description of parentheses and
> > the quoted subject line
> >
> > ie: commit 0123456789ab ("commit description")
>
> Now this is in mainline, checkpatch starts complaining about my "too long"
> (40 chars) commit IDs in commit messages :-(
>
> 40 chars may be too long (but it's quick to copy-and-paste, as "git show"
> shows that by default), but 12 sounds a bit short, as that's only 48 bits.
Right now, this test allows 12 to 16 byte length commit ids
without emitting a warning.
Andrew wanted this test, I don't care how long the commit id
is in the commit log.
> According to the Birthday Paradox (en.wikiipedia.org/wiki/Birthday_problem),
> there's a probability of 50% of a collision if you use 48 bits IDs in a
> repository with ca. 16 milion (2^24) objects. A Linux kernel repository
> counts ca. 4 million objects, so we're getting close...
>
> So soon we'll get "error: short SHA1 is ambiguous".
>
> BTW, is there actually an easy way to make "git show" show all options for
> an ambiguous SHA1?
Not so far as I know, but I'm nothing like a git expert.
The script I used before adding this to checkpatch was:
$ cat format_commit.sh
#!/bin/bash
regex1="^error: short SHA1 $1 is ambiguous\."
regex2="fatal: ambiguous argument '$1': unknown revision or path not in the
working tree\."
tmp=$(mktemp --tmpdir format_commit.X)
git log --format='%H ("%s")' -1 $1 > $tmp 2>&1
read line < $tmp
rm -f $tmp
if [[ $line =~ $regex1 ]] ; then
echo "checking commits $1..."
git rev-list --remotes | grep -i "^$1" |
while read line ; do
git log --format='%H ("%s")' -1 $line |
echo "commit $(cut -c 1-12,41-)"
done
elif [[ $line =~ $regex2 ]] ; then
echo "No matching commit"
exit 1
else
echo "commit $(echo $line | cut -c1-12,41-)"
fi
exit 0
$
so that using "$ format_commit.sh 1234" looks
at _all_ the commit references by using git rev-list
then greps that output for the matches, but it is
darn slow...
$ time ./format_commit.sh 1234
checking commits 1234...
commit 1234351cba95 ("xfs: introduce xlog_copy_iovec")
commit 1234471e2d11 ("perf header: Fix numa topology printing")
commit 1234f4bada54 ("hwrng: Kconfig: remove dependency for atmel-rng driver")
commit 12340313cf94 ("MAINTAINERS: add new cgroup list to CC notice")
commit 12346037a718 ("UBIFS: dump more in the lprops debugging check")
commit 12342c475f5d ("iwlwifi: proper monitor support")
commit 1234010684bb ("Add notation that the Asus W5F laptop has a short cable
instead of 80-wire.")
commit 123411f2d0da ("[CPUFREQ] dprintf format fixes in
cpufreq/speedstep-centrino.c")
real0m24.535s
user0m21.668s
sys 0m5.332s
--
To unsubscribe from this list: send the line "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] checkpatch: Add test for commit id formatting style in commit log
On Sun, 2014-08-10 at 14:35 -0700, Andrew Morton wrote:
> On Sun, 10 Aug 2014 14:28:01 -0700 Joe Perches wrote:
> > > On Thu, Jul 3, 2014 at 12:00 AM, Joe Perches wrote:
> > > > Commit logs have various forms of commit id references.
> > > >
> > > > Try to standardize on a 12 character long lower case
> > > > commit id along with a description of parentheses and
> > > > the quoted subject line
> > > >
> > > > ie: commit 0123456789ab ("commit description")
> > >
> > > Now this is in mainline, checkpatch starts complaining about my "too long"
> > > (40 chars) commit IDs in commit messages :-(
> > >
> > > 40 chars may be too long (but it's quick to copy-and-paste, as "git show"
> > > shows that by default), but 12 sounds a bit short, as that's only 48 bits.
> >
> > Right now, this test allows 12 to 16 byte length commit ids
> > without emitting a warning.
> >
> > Andrew wanted this test, I don't care how long the commit id
> > is in the commit log.
>
> Well, I mainly wanted to stop having to add "commit description" when
> people forget it. The length check was perhaps a bit anal. How about
> we make it "12 or more"?
Fine by me, just change the 16 to 40
---
scripts/checkpatch.pl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 31a731e..b385bcb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2133,7 +2133,7 @@ sub process {
# Check for improperly formed commit descriptions
if ($in_commit_log &&
$line =~ /\bcommit\s+[0-9a-f]{5,}/i &&
- $line !~ /\b[Cc]ommit [0-9a-f]{12,16} \("/) {
+ $line !~ /\b[Cc]ommit [0-9a-f]{12,40} \("/) {
$line =~ /\b(c)ommit\s+([0-9a-f]{5,})/i;
my $init_char = $1;
my $orig_commit = lc($2);
@@ -2141,7 +2141,7 @@ sub process {
my $desc = 'commit description';
($id, $desc) = git_commit_info($orig_commit, $id,
$desc);
ERROR("GIT_COMMIT_ID",
- "Please use 12 to 16 chars for the git commit ID
like: '${init_char}ommit $id (\"$desc\")'\n" . $herecurr);
+ "Please use 12 or more chars for the git commit
ID like: '${init_char}ommit $id (\"$desc\")'\n" . $herecurr);
}
# Check for added, moved or deleted files
--
To unsubscribe from this list: send the line "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: Tackling Git Limitations with Singular Large Line-seperated Plaintext files
On 30 June 2014 14:56, Jakub Narębski wrote:
> Linus Torvalds wrote:
> > .. even there, there's another issue. With enough memory, the diff
> > itself should be fairly reasonable to do, but we do not have any
> > sane *format* for diffing those kinds of things.
> >
> > The regular textual diff is line-based, and is not amenable to
> > comparing two long lines. You'll just get a diff that says "the two
> > really long lines are different".
> >
> > The binary diff option should work, but it is a horrible output
> > format, and not very helpful. It contains all the relevant data
> > ("copy this chunk from here to here"), but it's then shown in a
> > binary encoding that isn't really all that useful if you want to say
> > "what are the differences between these two chromosomes".
>
> There is also --word-diff[=] word-based textual diff, and I
> think one can abuse --word-diff-regex= for character-based
> diff... or maybe not, as specifies word characters, not words
> or word separators.
Yes, I have this alias defined:
dww = diff --word-diff --word-diff-regex=.
It creates nice diffs on a character level. Sometimes specifying
--patience to this helps.
-- Øyvind
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] remote.c: don't leak the base branch name in format_tracking_info
Stefan Beller writes:
> On 10.08.2014 15:57, Stefan Beller wrote:
>> Found by scan.coverity.com (Id: 1127809)
>>
>> Signed-off-by: Stefan Beller
>> ---
>> remote.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/remote.c b/remote.c
>> index 3d6c86a..2c1458f 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1983,6 +1983,7 @@ int format_tracking_info(struct branch *branch, struct
>> strbuf *sb)
>> strbuf_addf(sb,
>> _(" (use \"git pull\" to merge the remote
>> branch into yours)\n"));
>> }
>> +free(base);
>> return 1;
>> }
>>
>>
>
> Upon testing this one again, I get a warning
> remote.c: In function ‘format_tracking_info’:
> remote.c:1986:2: warning: passing argument 1 of ‘free’ discards ‘const’
> qualifier from pointer target type [enabled by default]
> free(base);
> ^
> ...
> Please ignore this patch.
It is perfectly fine to cast it to (char *) in this case, I think.
The real culprit is that the functionà reuses the same "base" (which
is a pointer into a constant region of memory) to receive the new
copy allocated by shorten_unambiguous_ref(); the piece of memory
returned by the callee belongs to this function, and it is perfectly
fine if this function modifies the contents of it (which it doesn't
happen to do). Storing the returned value to a variable of type
"const char *" does not absolve it from the responsibility to free
it (hence your 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] Update hard-coded header dependencies
Jonathan Nieder writes: > The fall-back rules used when compilers don't support the -MMD switch > to generate makefile rules based on #includes have been out of date > since v1.7.12.1~22^2~8 (move git_version_string into version.c, > 2012-06-02). > > Checked with 'make CHECK_HEADER_DEPENDENCIES=yes'. > > Signed-off-by: Jonathan Nieder > --- > Maybe it's worth switching to plain > > LIB_H += $(wildcard *.h) > > ? People using ancient compilers that never change headers wouldn't > be hurt, people using modern compilers that do change headers also > wouldn't be hurt, and we could stop pretending to maintain an > up-to-date list. I agree that it is very tempting to declare that we do not manually "maintain" the dependency list and force people without -MMD to recompile whenever any unrelated header changes. Especially that this patch only works on the 'master' branch and upwards, and does not even work on 'maint', let alone 1.9 or 1.8.5 maintenance tracks. Let's consider the merit of that approach after 2.1 is out. 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] Update hard-coded header dependencies
Junio C Hamano writes: > Jonathan Nieder writes: > >> ? People using ancient compilers that never change headers wouldn't >> be hurt, people using modern compilers that do change headers also >> wouldn't be hurt, and we could stop pretending to maintain an >> up-to-date list. > > I agree that it is very tempting to declare that we do not manually > "maintain" the dependency list and force people without -MMD to > recompile whenever any unrelated header changes. Especially that > this patch only works on the 'master' branch and upwards, and does > not even work on 'maint', let alone 1.9 or 1.8.5 maintenance tracks. > > Let's consider the merit of that approach after 2.1 is out. Thanks. Actually "upwards" is not even true; the 'next' branch already wants e.g. trace.h to build credential-store.o, which is not needed for the 'master' branch. -- To unsubscribe from this list: send the line "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] Fixing unclear messages
Alexander Shopov writes:
> diff --git a/builtin/clean.c b/builtin/clean.c
> index 1032563..9f38068 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -514,7 +514,7 @@ static int parse_choice(struct menu_stuff *menu_stuff,
> if (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom >
> top ||
> (is_single && bottom != top)) {
> clean_print_color(CLEAN_COLOR_ERROR);
> - printf_ln(_("Huh (%s)?"), (*ptr)->buf);
> + printf_ln(_("Wrong choice (%s). Choose again."),
> (*ptr)->buf);
Why is this an improvement?
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 5ed6036..cdc8a4e 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1198,7 +1198,7 @@ static int parse_and_validate_options(int argc, const
> char *argv[],
> if (argc == 0 && (also || (only && !amend)))
> die(_("No paths with --include/--only does not make sense."));
> if (argc == 0 && only && amend)
> - only_include_assumed = _("Clever... amending the last one with
> dirty index.");
> + only_include_assumed = _("You are amending the last commit
> only. There are additional changes in the index.");
Why is this an improvement?
It would be very good to give a way for people to discover that
"commit --amend -o" (no other arguments) is a good way to amend only
the commit log message without changing the tree even when the user
has already added changes to the index. But this message only
rewards those who already knew that trick and exercised it---when
they see it, they already know what they are doing. In other words,
this is really a rare "expert-only" message. I am not sure if
rewording would add much value to it, especially because it is very
unlikely for anybody to run "commit --amend -o" (no other arguments)
by mistake, expecting something else to happen, which warrant a
warning.
Besides, "amending the last commit only." implies as if there is a
way to amend more than that (there isn't), and "additional changes
in the index" does not convey any extra information as to what would
happen to them---would they be recorded in the resulting tree, or
would they be left out?
> if (argc > 0 && !also && !only)
> only_include_assumed = _("Explicit paths specified without -i
> or -o; assuming --only paths...");
It may be time to remove these messages, by the way. This one, and
the previous one, were remnants from the days we transitioned the
default behaviour of "git commit " from "Record changes that
have already been added to the index plus the changes in the given
path" (aka "include/also") to "Record only changes in the given
path, temporarily disregarding the changes already added to the
index" (aka "only"). Giving this warning had some value to remind
people who were used to the then-old default, as the result would be
different with the then-new world order. But these days, I do not
think people even remember that "include" used to be the default.
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 5568a5b..d9c5911 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1064,7 +1064,7 @@ static void parse_pack_objects(unsigned char *sha1)
> nr_delays--;
> }
> if (nr_delays)
> - die(_("confusion beyond insanity in parse_pack_objects()"));
> + die(_("fatal error in function \"parse_pack_objects\". This is
> a bug in Git. Please report it to the developers with an e-mail to
> git@vger.kernel.org."));
It probably should be spelled die("BUG: ...").
> @@ -1139,7 +1139,7 @@ static void conclude_pack(int fix_thin_pack, const char
> *curr_pack, unsigned cha
> int nr_unresolved = nr_deltas - nr_resolved_deltas;
> int nr_objects_initial = nr_objects;
> if (nr_unresolved <= 0)
> - die(_("confusion beyond insanity"));
> + die(_("fatal error in function \"conclude_pack\". This
> is a bug in Git. Please report it to the developers with an e-mail to
> git@vger.kernel.org."));
Likewise.
> diff --git a/builtin/log.c b/builtin/log.c
> index 4389722..d614a20 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -992,7 +992,7 @@ static const char *clean_message_id(const char *msg_id)
> m++;
> }
> if (!z)
> - die(_("insane in-reply-to: %s"), msg_id);
> + die(_("wrong format for the \"in-reply-to\" header: %s"),
> msg_id);
Why is s/insane/wrong format/ an improvement?
> @@ -1065,7 +1065,7 @@ static int output_directory_callback(const struct
> option *opt, const char *arg,
> {
> const char **dir = (const char **)opt->value;
> if (*dir)
> - die(_("Two output directories?"));
> + die(_("Maximum one output directory is allowed."));
Why is it an improvement?
> diff --git a/builtin/merge.c b/builtin/merge.c
> ind
Sharing merge conflict resolution between multiple developers
Hi List, At $dayjob we maintain internal forks of the a number of upstream repositories. Unsurprisingly updating these forks can be extremely problematic, especially when it's only one person doing the merge. Fortunately most of us are in the same physical location so it is possible to drag in someone who knows more about the code than the person merging but I can't see that scaling with remote developers. Is there any way where we could share the conflict resolution around but still end up with a single merge commit. I'm thinking of something like the following workflow developer A: git merge $upstream git mergetool ... git commit -m "WIP: Merge upstream" --something-like--all-but-not developer B: git pull developer_A git reset HEAD^ git mergetool ... git commit -m "WIP: Merge upstream" --something-like--all-but-not developer A: git pull developer_B git reset HEAD^ git mergetool git commit git push Any thoughts on if something like this is currently possible? Is this something other git users would find useful? Thanks, Chris -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
