* Re: Bug report: $program_name in error message
From: Junio C Hamano @ 2016-12-20 17:41 UTC (permalink / raw)
To: Vasco Almeida; +Cc: Stefan Beller, Josh Bleecher Snyder, git@vger.kernel.org
In-Reply-To: <1482243418.2029.10.camel@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> Thanks for the report and letting me know.
> Yes, these were mistakes and lack of attention mine. It was supposed to
> call 'eval_gettext' rather than 'gettext' when \$variable interpolation
> is needed.
Thanks.
As both of the offending commits (d323c6b641 & f2d17068fd) were part
of the topic that was merged at 2703572b3a ("Merge branch
'va/i18n-even-more'", 2016-07-13), I'll queue this single patch on
top of 2703572b3a^2 (i.e. the tip of the topic).
-- >8 --
Subject: [PATCH] i18n: fix misconversion in shell scripts
An earlier series that was merged at 2703572b3a ("Merge branch
'va/i18n-even-more'", 2016-07-13) failed to use $(eval_gettext
"string with \$variable interpolation") and instead used gettext in
a few places, and ended up showing the variable names in the
message, e.g.
$ git submodule
fatal: $program_name cannot be used without a working tree.
Catch these mistakes with
$ git grep -n '[^_]gettext .*\\\$'
and fix them all to use eval_gettext instead.
Reported-by: Josh Bleecher Snyder
Acked-by: Vasco Almeida <vascomalmeida@sapo.pt>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-rebase--interactive.sh | 3 ++-
git-sh-setup.sh | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a545d92c26..c5806859f0 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -467,7 +467,8 @@ update_squash_messages () {
}' <"$squash_msg".bak
} >"$squash_msg"
else
- commit_message HEAD > "$fixup_msg" || die "$(gettext "Cannot write \$fixup_msg")"
+ commit_message HEAD >"$fixup_msg" ||
+ die "$(eval_gettext "Cannot write \$fixup_msg")"
count=2
{
printf '%s\n' "$comment_char $(gettext "This is a combination of 2 commits.")"
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 2eda134800..c7b2a95463 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -194,14 +194,14 @@ require_work_tree_exists () {
if test "z$(git rev-parse --is-bare-repository)" != zfalse
then
program_name=$0
- die "$(gettext "fatal: \$program_name cannot be used without a working tree.")"
+ die "$(eval_gettext "fatal: \$program_name cannot be used without a working tree.")"
fi
}
require_work_tree () {
test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true || {
program_name=$0
- die "$(gettext "fatal: \$program_name cannot be used without a working tree.")"
+ die "$(eval_gettext "fatal: \$program_name cannot be used without a working tree.")"
}
}
--
2.11.0-416-g1351c11cce
^ permalink raw reply related
* Re: Allow "git shortlog" to group by committer information
From: Johannes Sixt @ 2016-12-20 18:12 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Junio C Hamano, Git Mailing List
In-Reply-To: <20161216135141.yhas67pzfm7bxxum@sigill.intra.peff.net>
Am 16.12.2016 um 14:51 schrieb Jeff King:
> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
> index ae08b57712..6c7c637481 100755
> --- a/t/t4201-shortlog.sh
> +++ b/t/t4201-shortlog.sh
> @@ -190,4 +190,17 @@ test_expect_success 'shortlog with --output=<file>' '
> test_line_count = 3 shortlog
> '
>
> +test_expect_success 'shortlog --committer (internal)' '
> + cat >expect <<-\EOF &&
> + 3 C O Mitter
> + EOF
> + git shortlog -nsc HEAD >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'shortlog --committer (external)' '
> + git log --format=full | git shortlog -nsc >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
>
May I kindly ask you to make this work on Windows, too? Just
sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh
on your Linux box and make it pass the tests.
Thank you so much in advance.
-- Hannes
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Junio C Hamano @ 2016-12-20 18:19 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Linus Torvalds, Git Mailing List
In-Reply-To: <16b115e0-3a7e-a5c2-1526-44bbcfc97db8@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 16.12.2016 um 14:51 schrieb Jeff King:
>> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
>> index ae08b57712..6c7c637481 100755
>> --- a/t/t4201-shortlog.sh
>> +++ b/t/t4201-shortlog.sh
>> @@ -190,4 +190,17 @@ test_expect_success 'shortlog with --output=<file>' '
>> test_line_count = 3 shortlog
>> '
>>
>> +test_expect_success 'shortlog --committer (internal)' '
>> + cat >expect <<-\EOF &&
>> + 3 C O Mitter
>> + EOF
>> + git shortlog -nsc HEAD >actual &&
>> + test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'shortlog --committer (external)' '
>> + git log --format=full | git shortlog -nsc >actual &&
>> + test_cmp expect actual
>> +'
>> +
>> test_done
>>
>
> May I kindly ask you to make this work on Windows, too? Just
>
> sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh
HAVENOT???
>
> on your Linux box and make it pass the tests.
>
> Thank you so much in advance.
>
> -- Hannes
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Junio C Hamano @ 2016-12-20 18:24 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Linus Torvalds, Git Mailing List
In-Reply-To: <xmqq60melazp.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> May I kindly ask you to make this work on Windows, too? Just
>>
>> sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh
>
> HAVENOT???
>>
>> on your Linux box and make it pass the tests.
>>
>> Thank you so much in advance.
Ah, I think I am slower than my usual today.
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Johannes Sixt @ 2016-12-20 18:33 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, bmwill, sandals, David.Turner
In-Reply-To: <20161219232828.5075-4-sbeller@google.com>
Am 20.12.2016 um 00:28 schrieb Stefan Beller:
> +static void report_and_die(struct child_process *cmd, const char *action)
> +{
> + int i;
> + struct strbuf err = STRBUF_INIT;
> + if (cmd->git_cmd)
> + strbuf_addstr(&err, "git ");
> + for (i = 0; cmd->argv[i]; )
> + strbuf_addf(&err, "'%s'", cmd->argv[i]);
Take note that cmd is accessed here.
> + die(_("could not %s %s"), action, err.buf);
Should lego sentences not be avoided? They are not exactly translator
friendly.
Given that a lot of effort is spent elsewhere to actually *avoid* dying
in library code, this new die() is not very welcome, I must say.
Granted, you just add convenience functions here, and callers have
alternatives that do not die, but still...
> +}
> +
> int start_command(struct child_process *cmd)
> {
> int need_in, need_out, need_err;
> @@ -546,6 +557,12 @@ int start_command(struct child_process *cmd)
> return 0;
> }
>
> +void start_command_or_die(struct child_process *cmd)
> +{
> + if (start_command(cmd))
> + report_and_die(cmd, "start");
But cmd has been cleaned up at this point of call of report_and_die. The
access noted above goes to freed memory, I think.
> +}
> ...
> +void finish_command_or_die(struct child_process *cmd)
> +{
> + if (finish_command(cmd))
> + report_and_die(cmd, "finish");
Same here.
> +}
> ...
> +void run_command_or_die(struct child_process *cmd)
> +{
> + if (finish_command(cmd))
> + report_and_die(cmd, "run");
And here as well.
> +}
-- Hannes
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Junio C Hamano @ 2016-12-20 18:35 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Linus Torvalds, Git Mailing List
In-Reply-To: <xmqq1sx2lara.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> May I kindly ask you to make this work on Windows, too? Just
>>>
>>> sed -i -e s/MINGW/MINGW,HAVENOT/ t4201-shortlog.sh
>>
>> HAVENOT???
>>>
>>> on your Linux box and make it pass the tests.
>>>
>>> Thank you so much in advance.
>
> Ah, I think I am slower than my usual today.
-- >8 --
Subject: SQUASH???
Make sure the test does not depend on the result of the previous
tests; with MINGW prerequisite satisfied, a "reset to original and
rebuild" in an earlier test was skipped, resulting in different
history being tested with this and the next tests.
---
t/t4201-shortlog.sh | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 6c7c637481..9df054bf05 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -191,8 +191,14 @@ test_expect_success 'shortlog with --output=<file>' '
'
test_expect_success 'shortlog --committer (internal)' '
+ git checkout --orphan side &&
+ git commit --allow-empty -m one &&
+ git commit --allow-empty -m two &&
+ GIT_COMMITTER_NAME="Sin Nombre" git commit --allow-empty -m three &&
+
cat >expect <<-\EOF &&
- 3 C O Mitter
+ 2 C O Mitter
+ 1 Sin Nombre
EOF
git shortlog -nsc HEAD >actual &&
test_cmp expect actual
^ permalink raw reply related
* Re: Allow "git shortlog" to group by committer information
From: Johannes Sixt @ 2016-12-20 18:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Linus Torvalds, Git Mailing List
In-Reply-To: <xmqqvauejvnr.fsf@gitster.mtv.corp.google.com>
Am 20.12.2016 um 19:35 schrieb Junio C Hamano:
> test_expect_success 'shortlog --committer (internal)' '
> + git checkout --orphan side &&
> + git commit --allow-empty -m one &&
> + git commit --allow-empty -m two &&
> + GIT_COMMITTER_NAME="Sin Nombre" git commit --allow-empty -m three &&
Clever! Thank you. Will test in 12 hours.
> +
> cat >expect <<-\EOF &&
> - 3 C O Mitter
> + 2 C O Mitter
> + 1 Sin Nombre
> EOF
> git shortlog -nsc HEAD >actual &&
> test_cmp expect actual
>
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Johannes Sixt @ 2016-12-20 18:54 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, bmwill, sandals, David.Turner
In-Reply-To: <aad0af97-7588-632d-a113-5d8372b8b7a8@kdbg.org>
Am 20.12.2016 um 19:33 schrieb Johannes Sixt:
> Am 20.12.2016 um 00:28 schrieb Stefan Beller:
>> +void run_command_or_die(struct child_process *cmd)
>> +{
>> + if (finish_command(cmd))
>> + report_and_die(cmd, "run");
>
> And here as well.
Oh, and BTW, this one wraps only finish_command, not run_command.
-- Hannes
^ permalink raw reply
* [GIT GUI/PATCH/RFC] git gui: get current theme in 8.5-backwards-compatible way
From: Pete Harlan @ 2016-12-20 19:20 UTC (permalink / raw)
To: Pat Thoyts; +Cc: git
Later Tcl 8.5 versions learned to return the current theme by omitting
the final argument to "[ttk::style theme use]", but this throws an
error on earlier 8.5 versions (e.g., 8.5.7).
InitTheme works around this by catching the error and reading
::ttk::currentTheme instead.
A call to "[ttk::style theme use]" was added to ttext in 30508bc
("Amend tab ordering and text widget border and highlighting.",
2016-10-02). Break out InitTheme's workaround into its own
get_current_theme proc and use it in both places.
Signed-off-by: Pete Harlan <pgit@tento.net>
---
Note: Applies to the upstream git-gui repo, http://repo.or.cz/git-gui .
To apply to Git itself, apply in the git-gui directory.
Issue entered Git in v2.10.1-537-g3eae30870.
lib/themed.tcl | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/lib/themed.tcl b/lib/themed.tcl
index 351a712..85c157b 100644
--- a/lib/themed.tcl
+++ b/lib/themed.tcl
@@ -28,10 +28,7 @@ proc InitTheme {} {
}
}
- # Handle either current Tk or older versions of 8.5
- if {[catch {set theme [ttk::style theme use]}]} {
- set theme $::ttk::currentTheme
- }
+ set theme [get_current_theme]
if {[lsearch -exact {default alt classic clam} $theme] != -1} {
# Simple override of standard ttk::entry to change the field
@@ -248,7 +245,7 @@ proc tspinbox {w args} {
proc ttext {w args} {
global use_ttk
if {$use_ttk} {
- switch -- [ttk::style theme use] {
+ switch -- [get_current_theme] {
"vista" - "xpnative" {
lappend args -highlightthickness 0
-borderwidth 0
}
@@ -343,6 +340,16 @@ proc on_choosefont {familyvar sizevar font} {
set size [dict get $font -size]
}
+# Get current theme in a backwards-compatible way.
+proc get_current_theme {} {
+ # Handle either current Tk or older versions of 8.5
+ if {[catch {set theme [ttk::style theme use]}]} {
+ set theme $::ttk::currentTheme
+ }
+
+ return $theme
+}
+
# Local variables:
# mode: tcl
# indent-tabs-mode: t
^ permalink raw reply related
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Stefan Beller @ 2016-12-20 19:23 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams,
brian m. carlson, David Turner
In-Reply-To: <aad0af97-7588-632d-a113-5d8372b8b7a8@kdbg.org>
On Tue, Dec 20, 2016 at 10:33 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 20.12.2016 um 00:28 schrieb Stefan Beller:
>>
>> +static void report_and_die(struct child_process *cmd, const char *action)
>> +{
>> + int i;
>> + struct strbuf err = STRBUF_INIT;
>> + if (cmd->git_cmd)
>> + strbuf_addstr(&err, "git ");
>> + for (i = 0; cmd->argv[i]; )
>> + strbuf_addf(&err, "'%s'", cmd->argv[i]);
>
>
> Take note that cmd is accessed here.
>
>> + die(_("could not %s %s"), action, err.buf);
>
>
> Should lego sentences not be avoided? They are not exactly translator
> friendly.
>
> Given that a lot of effort is spent elsewhere to actually *avoid* dying in
> library code, this new die() is not very welcome, I must say.
I agree on the sentiment. In a reroll I'll drop this patch and instead introduce
a function `char *get_child_command_line(struct child_process*);`, which
a caller can call before calling finish_command and then use the
resulting string
to assemble an error message without lego.
Thanks for the thorough review!
Stefan
^ permalink raw reply
* Re: [PATCH 0/3] push only submodules
From: Junio C Hamano @ 2016-12-20 19:25 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller
In-Reply-To: <1482171933-180601-1-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> This series teaches 'git push' to be able to only push submodules
> while leaving a superproject unpushed.
It somehow feels a bit strange to single out the top-level as
special like this one (iow, shouldn't it be equally as easy to push
out the superproject and two submodules leaving one submodule alone,
as to push out these three submodules leaving the superproject
alone?), but I will queue these for now with tweaks to address a few
minor points.
* What is done with 1/3 I do not think is "refactor".
* "perhaps a code review tool" -> "perhaps a tool like Gerrit code
review" as suggested by Stefan.
* Swap the order expected_pub and expected_submodule are generated
to match the order they are used for final verification in the
test.
^ permalink raw reply
* Re: [PATCH] fast-import: properly fanout notes when tree is imported
From: Junio C Hamano @ 2016-12-20 19:34 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, johan
In-Reply-To: <20161219021212.15978-1-mh@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> In typical uses of fast-import, trees are inherited from a parent
> commit. In that case, the tree_entry for the branch looks like:
> ...
> +# Create another notes tree from the one above
> +cat >>input <<INPUT_END
> +...
> +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)
There is a trailing SP that cannot be seen by anybody.
Don't do this. It makes it very easy to miss what is going on and
wastes reviewers' time.
Protect it by doing something like:
sed -e 's/Z$//' >>input <<INPUT_END
...
M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] config.c: handle lock file in error case in git_config_rename_...
From: Junio C Hamano @ 2016-12-20 20:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, josharian
In-Reply-To: <20161220094836.4131-2-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> We could rely on atexit() to clean up everything, but let's be
> explicit when we can. And it's good anyway because the function is
> called the second time in the same process, we're in trouble.
>
> This function should not affect the successful case because after
> commit_lock_file() is called, rollback_lock_file() becomes no-op.
Not really. At the point of the first "goto out" in this function,
lock is merely an uninitialized pointer.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> config.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/config.c b/config.c
> index 505e0d0..e02def4 100644
> --- a/config.c
> +++ b/config.c
> @@ -2483,6 +2483,7 @@ int git_config_rename_section_in_file(const char *config_filename,
> ret = error_errno("could not write config file %s",
> config_filename);
> out:
> + rollback_lock_file(lock);
> free(filename_buf);
> return ret;
> }
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Johannes Sixt @ 2016-12-20 20:12 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams,
brian m. carlson, David Turner
In-Reply-To: <CAGZ79kYNKWfnEXWJfyRUutFyaQiRD9qW--LkK4Nbwdf7FtdPQA@mail.gmail.com>
Am 20.12.2016 um 20:23 schrieb Stefan Beller:
> In a reroll I'll drop this patch and instead introduce
> a function `char *get_child_command_line(struct child_process*);`, which
> a caller can call before calling finish_command and then use the
> resulting string
> to assemble an error message without lego.
That sounds a lot better, thank you. Note though, that the function
would have to be called before start_command(), because when it fails,
it would be too late.
I have to ask, though: Is it so much necessary to report the exact
command line in an error message? If someone is interested in which
command failed, it would be "sufficient" to run the command under
GIT_TRACE=2.
-- Hannes
^ permalink raw reply
* Re: [PATCH] fast-import: properly fanout notes when tree is imported
From: Mike Hommey @ 2016-12-20 20:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johan
In-Reply-To: <xmqqk2aujsyb.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 20, 2016 at 11:34:04AM -0800, Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> writes:
>
> > In typical uses of fast-import, trees are inherited from a parent
> > commit. In that case, the tree_entry for the branch looks like:
> > ...
> > +# Create another notes tree from the one above
> > +cat >>input <<INPUT_END
> > +...
> > +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)
>
> There is a trailing SP that cannot be seen by anybody.
>
> Don't do this. It makes it very easy to miss what is going on and
> wastes reviewers' time.
>
> Protect it by doing something like:
>
> sed -e 's/Z$//' >>input <<INPUT_END
> ...
> M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z
How about
EMPTY=
...
M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) $EMPTY
?
Mike
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Stefan Beller @ 2016-12-20 20:49 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams,
brian m. carlson, David Turner
In-Reply-To: <f14ee492-8297-c8ec-f80f-f8f24caf91e1@kdbg.org>
On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 20.12.2016 um 20:23 schrieb Stefan Beller:
>>
>> In a reroll I'll drop this patch and instead introduce
>> a function `char *get_child_command_line(struct child_process*);`, which
>> a caller can call before calling finish_command and then use the
>> resulting string
>> to assemble an error message without lego.
>
>
> That sounds a lot better, thank you. Note though, that the function would
> have to be called before start_command(), because when it fails, it would be
> too late.
Yes, the pattern to use it would be
// first assemble the child process struct with conditions
char *cmdline = get_child_command_line(&child)
if (start_command(&child))
// use cmdline here for error reporting.
>
> I have to ask, though: Is it so much necessary to report the exact command
> line in an error message?
Have a look at https://github.com/git/git/blob/master/submodule.c#L1122
die("Could not run 'git status --porcelain -uall \
--ignore-submodules=none' in submodule %s", path);
There are 2 things:
1) The error message is not very informative, as your question hints at.
I consider changing it to add more meaning. I think the end user
would also be interested in "Why did we run this command?".
2) We really want to report the correct command line here.
Currently that is the case, but if you look at the prior patch that extends
the arguments conditionally (and then also uses the same condition
to format the error message... that hints at hard to maintain error
messages.)
So the new proposed function really only addresses 2) here.
> If someone is interested in which command failed,
> it would be "sufficient" to run the command under GIT_TRACE=2.
>
Yes, while at it, I may just move up the error reporting to the builtin
command giving a higher level message.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH] fast-import: properly fanout notes when tree is imported
From: Junio C Hamano @ 2016-12-20 20:56 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, johan
In-Reply-To: <20161220204841.awvabgwsxudxfzca@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> On Tue, Dec 20, 2016 at 11:34:04AM -0800, Junio C Hamano wrote:
>> Mike Hommey <mh@glandium.org> writes:
>>
>> > In typical uses of fast-import, trees are inherited from a parent
>> > commit. In that case, the tree_entry for the branch looks like:
>> > ...
>> > +# Create another notes tree from the one above
>> > +cat >>input <<INPUT_END
>> > +...
>> > +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)
>>
>> There is a trailing SP that cannot be seen by anybody.
>>
>> Don't do this. It makes it very easy to miss what is going on and
>> wastes reviewers' time.
>>
>> Protect it by doing something like:
>>
>> sed -e 's/Z$//' >>input <<INPUT_END
>> ...
>> M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z
>
> How about
> EMPTY=
> ...
> M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) $EMPTY
>
> ?
Notice I said "something like" ;-)
I think you are bringing that up to avoid sed, but if you want to go
that route, the long string $EMPTY is distracting, and makes readers
wonder why something that is loud but expands to nothing has to be
there. It hides the true intention, which is that the SP that comes
before it is the most important thing on that line.
I would think a lot more understandable variant would be to do this
instead:
SP=" "
...
M a lot of garbage $(and command)$SP
^ permalink raw reply
* [PATCH] fast-import: properly fanout notes when tree is imported
From: Mike Hommey @ 2016-12-20 21:04 UTC (permalink / raw)
To: gitster; +Cc: git, johan
In-Reply-To: <xmqqbmw6jp59.fsf@gitster.mtv.corp.google.com>
In typical uses of fast-import, trees are inherited from a parent
commit. In that case, the tree_entry for the branch looks like:
.versions[1].sha1 = $some_sha1
.tree = <tree structure loaded from $some_sha1>
However, when trees are imported, rather than inherited, that is not the
case. One can import a tree with a filemodify command, replacing the
root tree object.
e.g.
"M 040000 $some_sha1 \n"
In this case, the tree_entry for the branch looks like:
.versions[1].sha1 = $some_sha1
.tree = NULL
When adding new notes with the notemodify command, do_change_note_fanout
is called to get a notes count, and to do so, it loops over the
tree_entry->tree, but doesn't do anything when the tree is NULL.
In the latter case above, it means do_change_note_fanout thinks the tree
contains no notes, and new notes are added with no fanout.
Interestingly, do_change_note_fanout does check whether subdirectories
have a NULL .tree, in which case it uses load_tree(). Which means the
right behaviour happens when using the filemodify command to import
subdirectories.
This change makes do_change_note_fanount call load_tree() whenever the
tree_entry it is given has no tree loaded, making all cases handled
equally.
Signed-off-by: Mike Hommey <mh@glandium.org>
---
fast-import.c | 8 +++++---
t/t9301-fast-import-notes.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index cb545d7df5..5e528b1999 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2220,13 +2220,17 @@ static uintmax_t do_change_note_fanout(
char *fullpath, unsigned int fullpath_len,
unsigned char fanout)
{
- struct tree_content *t = root->tree;
+ struct tree_content *t;
struct tree_entry *e, leaf;
unsigned int i, tmp_hex_sha1_len, tmp_fullpath_len;
uintmax_t num_notes = 0;
unsigned char sha1[20];
char realpath[60];
+ if (!root->tree)
+ load_tree(root);
+ t = root->tree;
+
for (i = 0; t && i < t->entry_count; i++) {
e = t->entries[i];
tmp_hex_sha1_len = hex_sha1_len + e->name->str_len;
@@ -2278,8 +2282,6 @@ static uintmax_t do_change_note_fanout(
leaf.tree);
} else if (S_ISDIR(e->versions[1].mode)) {
/* This is a subdir that may contain note entries */
- if (!e->tree)
- load_tree(e);
num_notes += do_change_note_fanout(orig_root, e,
hex_sha1, tmp_hex_sha1_len,
fullpath, tmp_fullpath_len, fanout);
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 83acf68bc3..dadc70b7d5 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -483,6 +483,48 @@ test_expect_success 'verify that lots of notes trigger a fanout scheme' '
'
+# Create another notes tree from the one above
+SP=" "
+cat >>input <<INPUT_END
+commit refs/heads/other_commits
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+commit #$(($num_commit + 1))
+COMMIT
+
+from refs/heads/many_commits
+M 644 inline file
+data <<EOF
+file contents in commit #$(($num_commit + 1))
+EOF
+
+commit refs/notes/other_notes
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+committing one more note on a tree imported from a previous notes tree
+COMMIT
+
+M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)$SP
+N inline :$(($num_commit + 1))
+data <<EOF
+note for commit #$(($num_commit + 1))
+EOF
+INPUT_END
+
+test_expect_success 'verify that importing a notes tree respects the fanout scheme' '
+ git fast-import <input &&
+
+ # None of the entries in the top-level notes tree should be a full SHA1
+ git ls-tree --name-only refs/notes/other_notes |
+ while read path
+ do
+ if test $(expr length "$path") -ge 40
+ then
+ return 1
+ fi
+ done
+'
+
cat >>expect_non-note1 << EOF
This is not a note, but rather a regular file residing in a notes tree
EOF
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] fast-import: properly fanout notes when tree is imported
From: Mike Hommey @ 2016-12-20 21:06 UTC (permalink / raw)
To: gitster; +Cc: git, johan
In-Reply-To: <20161220210448.32213-1-mh@glandium.org>
Sorry, I forgot the v2 in the subject.
Mike
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Johannes Sixt @ 2016-12-20 21:47 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams,
brian m. carlson, David Turner
In-Reply-To: <CAGZ79kYG-veuWNFh6G1g5MiQHBGk2i1qbH_NBtnMS6jFcoWocg@mail.gmail.com>
Am 20.12.2016 um 21:49 schrieb Stefan Beller:
> On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> I have to ask, though: Is it so much necessary to report the exact command
>> line in an error message?
>
> Have a look at https://github.com/git/git/blob/master/submodule.c#L1122
>
> die("Could not run 'git status --porcelain -uall \
> --ignore-submodules=none' in submodule %s", path);
>
> There are 2 things:
> 1) The error message is not very informative, as your question hints at.
> I consider changing it to add more meaning. I think the end user
> would also be interested in "Why did we run this command?".
You don't have to. start_command() and finish_command() report any
low-level errors (exec failed, signal received...). If the exit code of
the program is non-zero, finish_command() reports nothing because the
command *itself* will have written some error message ("working
directory dirty", "object database corrupt", "xyz: no such file or
directory"...). At this level, it only makes sense to leave a trace in
which submodule an error occured. So I think it would be sufficient to just
die("could not run 'git status' in submodule %s", path);
or
die("'git status' in submodule %s failed", path);
> 2) We really want to report the correct command line here.
Why? We do not do this anywhere else.
> Currently that is the case, but if you look at the prior patch that extends
> the arguments conditionally (and then also uses the same condition
> to format the error message... that hints at hard to maintain error
> messages.)
This does not explain why the *complete and detailed* invocation must be
reported. I haven't followed this topic at all, so I may be missing some
cruical detail. (If you say "it must happen" one more time, then I will
believe you, because for me that's simpler than to plough through a
flock of submodule topics. ;-)
-- Hannes
^ permalink raw reply
* Re: [PATCH] fast-import: properly fanout notes when tree is imported
From: Junio C Hamano @ 2016-12-20 21:50 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, johan
In-Reply-To: <20161220210448.32213-1-mh@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> In typical uses of fast-import, trees are inherited from a parent
> commit. In that case, the tree_entry for the branch looks like:
> ...
> This change makes do_change_note_fanount call load_tree() whenever the
> tree_entry it is given has no tree loaded, making all cases handled
> equally.
Thanks.
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Stefan Beller @ 2016-12-20 22:07 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams,
brian m. carlson, David Turner
In-Reply-To: <48970168-ba4b-e125-6624-e4c37b8a7180@kdbg.org>
On Tue, Dec 20, 2016 at 1:47 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> This does not explain why the *complete and detailed* invocation must be
> reported.
eh. I tried to say that a report that looks like a *complete and
detailed* invocation
should be such, and not be misleading (and e.g. miss one out of 5
arguments printed).
I haven't followed this topic at all, so I may be missing some
> cruical detail. (If you say "it must happen" one more time, then I will
> believe you, because for me that's simpler than to plough through a flock of
> submodule topics. ;-)
It doesn't have to happen; I am trying to come up with a better message.
>
> -- Hannes
>
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Paul Mackerras @ 2016-12-20 22:17 UTC (permalink / raw)
To: Marc Branchaud; +Cc: Michael Haggerty, git
In-Reply-To: <97d97bc6-54f1-2ef2-fe04-7e7f144d7e51@xiplink.com>
On Tue, Dec 20, 2016 at 10:01:15AM -0500, Marc Branchaud wrote:
> On 2016-12-19 11:44 AM, Michael Haggerty wrote:
> >This patch series changes a bunch of details about how remote-tracking
> >references are rendered in the commit list of gitk:
>
> Thanks for this! I like the new, compact look very much!
>
> That said, I remember when I was a new git user and I leaned heavily on gitk
> to understand how references worked. It was particularly illuminating to
> see the remote references distinctly labeled, and the fact that they were
> "remotes/origin/foo" gave me an Aha! moment where I came to understand that
> the refs hierarchy is more flexible than just the conventions coded into git
> itself. I eventually felt free to create my own, private ref hierarchies.
>
> I am in no way opposed to this series. I just wanted to point out that
> there was some utility in those labels. It makes me think that it might be
> worthwhile for gitk to have a "raw-refs" mode, that shows the full
> "refs/foo/bar/baz" paths of all the heads, tags, and whatever else. It
> could be a useful teaching tool for git.
Do you think we should have a checkbox in the preferences dialog to
select whether to display the long form or the short form?
Paul.
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Marc Branchaud @ 2016-12-20 22:53 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Michael Haggerty, git
In-Reply-To: <20161220221719.GB22566@fergus.ozlabs.ibm.com>
On 2016-12-20 05:17 PM, Paul Mackerras wrote:
> On Tue, Dec 20, 2016 at 10:01:15AM -0500, Marc Branchaud wrote:
>> On 2016-12-19 11:44 AM, Michael Haggerty wrote:
>>> This patch series changes a bunch of details about how remote-tracking
>>> references are rendered in the commit list of gitk:
>>
>> Thanks for this! I like the new, compact look very much!
>>
>> That said, I remember when I was a new git user and I leaned heavily on gitk
>> to understand how references worked. It was particularly illuminating to
>> see the remote references distinctly labeled, and the fact that they were
>> "remotes/origin/foo" gave me an Aha! moment where I came to understand that
>> the refs hierarchy is more flexible than just the conventions coded into git
>> itself. I eventually felt free to create my own, private ref hierarchies.
>>
>> I am in no way opposed to this series. I just wanted to point out that
>> there was some utility in those labels. It makes me think that it might be
>> worthwhile for gitk to have a "raw-refs" mode, that shows the full
>> "refs/foo/bar/baz" paths of all the heads, tags, and whatever else. It
>> could be a useful teaching tool for git.
>
> Do you think we should have a checkbox in the preferences dialog to
> select whether to display the long form or the short form?
Mmmm, more knobs!
No, I don't think that's necessary. Such a setting would probably just
confuse people. Plus it's not something anyone is likely to want to
change anyway. Maybe if there were an "Advanced" tab in the settings
dialog, but even then it seems like overkill.
I suspect there are better ways to teach people about the refs hierarchy
than cluttering up gitk. These may even already exist -- I've been a
git user for 8 years now, so I'm sure my perspective of the learning
curve is skewed.
M.
^ permalink raw reply
* [PATCHv4 0/4] git-rm absorbs submodule git directory before deletion
From: Stefan Beller @ 2016-12-20 23:12 UTC (permalink / raw)
To: gitster
Cc: git, Stefan Beller, Johannes Sixt, Brandon Williams,
brian m. carlson, David Turner
v4:
* removed the patch for adding {run,start,finish}_or_die
* added one more flag to the function ok_to_remove_submodule
(if die on error is ok)
* renamed ok_to_remove_submodule to bad_to_remove_submodule to signal
the error case better.
v3:
* removed the patch to enhance ok_to_remove_submodule to absorb the submodule
if needed
* Removed all the error reporting from git-rm that was related to submodule
git directories not absorbed.
* instead just absorb the git repositories or let the absorb function die
with an appropriate error message.
v2:
* new base where to apply the patch:
sb/submodule-embed-gitdir merged with sb/t3600-cleanup.
I got merge conflicts and resolved them this way:
#@@@ -709,9 -687,10 +687,9 @@@ test_expect_success 'checking out a com
# git commit -m "submodule removal" submod &&
# git checkout HEAD^ &&
# git submodule update &&
#- git checkout -q HEAD^ 2>actual &&
#+ git checkout -q HEAD^ &&
# git checkout -q master 2>actual &&
# - echo "warning: unable to rmdir submod: Directory not empty" >expected &&
# - test_i18ncmp expected actual &&
# + test_i18ngrep "^warning: unable to rmdir submod:" actual &&
# git status -s submod >actual &&
# echo "?? submod/" >expected &&
# test_cmp expected actual &&
#
* improved commit message in "ok_to_remove_submodule: absorb the submodule git dir"
(David Turner offered me some advice on how to write better English off list)
* simplified code in last patch:
-> dropped wrong comment for fallthrough
-> moved redundant code out of both bodies of an if-clause.
* Fixed last patchs commit message to have "or_die" instead of or_dir.
v1:
The "checkout --recurse-submodules" series got too large to comfortably send
it out for review, so I had to break it up into smaller series'; this is the
first subseries, but it makes sense on its own.
This series teaches git-rm to absorb the git directory of a submodule instead
of failing and complaining about the git directory preventing deletion.
It applies on origin/sb/submodule-embed-gitdir.
Any feedback welcome!
Thanks,
Stefan
CC: Johannes Sixt <j6t@kdbg.org>
CC: Brandon Williams <bmwill@google.com>
CC: "brian m. carlson" <sandals@crustytoothpaste.net>
CC: David Turner <David.Turner@twosigma.com>
Stefan Beller (4):
submodule.h: add extern keyword to functions
submodule: modernize ok_to_remove_submodule to use argv_array
submodule: rename and add flags to ok_to_remove_submodule
rm: absorb a submodules git dir before deletion
builtin/rm.c | 83 +++++++++++++++--------------------------------------------
submodule.c | 57 ++++++++++++++++++++++++++--------------
submodule.h | 59 ++++++++++++++++++++++++------------------
t/t3600-rm.sh | 39 +++++++++++-----------------
4 files changed, 108 insertions(+), 130 deletions(-)
--
2.11.0.rc2.53.gb7b3fba.dirty
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox