* [PATCH v2] rev-list: use hdr_termination instead of a always using a newline
From: Jacob Keller @ 2016-10-20 20:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dennis Kaarsemaker, Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
When adding support for prefixing output of log and other commands using
--line-prefix, commit 660e113ce118 ("graph: add support for
--line-prefix on all graph-aware output", 2016-08-31) accidentally
broke rev-list --header output.
In order to make the output appear with a line-prefix, the flow was
changed to always use the graph subsystem for display. Unfortunately
the graph flow in rev-list did not use info->hdr_termination as it was
assumed that graph output would never need to putput NULs.
Since we now always use the graph code in order to handle the case of
line-prefix, simply replace putchar('\n') with
putchar(info->hdr_termination) which will correct this issue.
Add a test for the --header case to make sure we don't break it in the
future. Implement a helper function test_ends_with_nul() to make it more
obvious what sort of check we are looking for.
Reported-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Changes in v2
* Squash Junio's suggested (better) test
* Add Junio's signed-off-by since he wrote the new test
builtin/rev-list.c | 2 +-
t/t6000-rev-list-misc.sh | 14 ++++++++++++++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git c/builtin/rev-list.c c/builtin/rev-list.c
index 8479f6ed28aa..c43decda7011 100644
--- c/builtin/rev-list.c
+++ c/builtin/rev-list.c
@@ -145,7 +145,7 @@ static void show_commit(struct commit *commit, void *data)
*/
if (buf.len && buf.buf[buf.len - 1] == '\n')
graph_show_padding(revs->graph);
- putchar('\n');
+ putchar(info->hdr_termination);
} else {
/*
* If the message buffer is empty, just show
diff --git c/t/t6000-rev-list-misc.sh c/t/t6000-rev-list-misc.sh
index 3e752ce03280..969e4e9e5261 100755
--- c/t/t6000-rev-list-misc.sh
+++ c/t/t6000-rev-list-misc.sh
@@ -100,4 +100,18 @@ test_expect_success '--bisect and --first-parent can not be combined' '
test_must_fail git rev-list --bisect --first-parent HEAD
'
+test_expect_success '--header shows a NUL after each commit' '
+ # We know that there is no Q in the true payload; names and
+ # addresses of the authors and the committers do not have
+ # any, and object names or header names do not, either.
+ git rev-list --header --max-count=2 HEAD |
+ nul_to_q |
+ grep "^Q" >actual &&
+ cat >expect <<-EOF &&
+ Q$(git rev-parse HEAD~1)
+ Q
+ EOF
+ test_cmp expect actual
+'
+
test_done
base-commit: 659889482ac63411daea38b2c3d127842ea04e4d
--
git-series 0.8.10
^ permalink raw reply related
* Re: Drastic jump in the time required for the test suite
From: Johannes Sixt @ 2016-10-20 20:38 UTC (permalink / raw)
To: Jeff King, Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <20161020123111.qnbsainul2g54z4z@sigill.intra.peff.net>
Am 20.10.2016 um 14:31 schrieb Jeff King:
> Close to 1/3 of those processes are just invoking the bin-wrapper
> script to set up the EXEC_PATH, etc. I imagine it would not be too hard
> to just do that in the test script. In fact, it looks like:
>
> make prefix=/wherever install
> GIT_TEST_INSTALLED=/wherever/bin make test
>
> might give you an immediate speedup by skipping bin-wrappers entirely.
Running the tests with --with-dashes should give you the same effect, no?
-- Hannes
^ permalink raw reply
* Re: [PATCH v4 23/25] sequencer: quote filenames in error messages
From: Junio C Hamano @ 2016-10-20 20:28 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqq8ttig3zn.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> This makes the code consistent by fixing quite a couple of error messages.
>>
>> Suggested by Jakub Narębski.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>
> These finishing touches in 21-23 look all sensible to me.
Make that 21-25. I finished reading to the end and it was mostly a
pleasnt read, except for a few things I noticed and sent reviews
separately.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 23/25] sequencer: quote filenames in error messages
From: Junio C Hamano @ 2016-10-20 20:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <930ea31d6f10973807276c7a9f35f0d692d1ada6.1476450940.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> This makes the code consistent by fixing quite a couple of error messages.
>
> Suggested by Jakub Narębski.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
These finishing touches in 21-23 look all sensible to me.
^ permalink raw reply
* Re: [PATCH v4 20/25] sequencer: refactor write_message()
From: Junio C Hamano @ 2016-10-20 20:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqqh986g49i.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> If I were doing this, I would make this into three separate steps:
>
> - move the strbuf_release(msgbuf) to the caller in
> do_pick_commit();
>
> - add the missing rollback_lock_file(), which is a bugfix; and
> then finally
>
> - allow the helper to take not a strbuf but <buf, len> pair as
> parameters.
>
> The end result of this patch achieves two thirds of the above, but
> especially given that write_message() only has two call sites in a
> single function, I think it is OK and preferrable even to do all
> three.
Ah, make that four steps. The final one is:
- add append_eol parameter that nobody uses at this step in the
series.
This is a new feature to the helper. While it is OK to have it as a
preparatory step in this series, it is easier to understand if it
kept as a separate step. It is even more preferrable if it is made
as a preparatory step in a series that adds a caller that passes
true to append_eol to this helper, or if that real change is small
enough, part of that patch that adds such a caller, not as a
separate step.
^ permalink raw reply
* Re: [PATCH v4 20/25] sequencer: refactor write_message()
From: Junio C Hamano @ 2016-10-20 20:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <9a66d53ebd86d35136597e534be03605bb1346bc.1476450940.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The write_message() function safely writes an strbuf to a file.
> Sometimes it is inconvenient to require an strbuf, though: the text to
> be written may not be stored in a strbuf, or the strbuf should not be
> released after writing.
>
> Let's refactor "safely writing string to a file" into
> write_with_lock_file(), and make write_message() use it.
>
> The new function will make it easy to create a new convenience function
> write_file_gently() that does not die(); as some of the upcoming callers
> of this new function will want to append a newline character, we already
> add that flag as parameter to write_with_lock_file().
>
> While at it, roll back the locked files in case of failure, as pointed
> out by Hannes Sixt.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
Once a helper function starts taking <buf, len> pair, not a strbuf,
it becomes obvious that it does not make much sense to calling
strbuf_release() from the helper. It is caller's job to do the
memory management of the strbuf it uses to pass information to the
helper, and the current api into write_message() is klunky.
If I were doing this, I would make this into three separate steps:
- move the strbuf_release(msgbuf) to the caller in
do_pick_commit();
- add the missing rollback_lock_file(), which is a bugfix; and
then finally
- allow the helper to take not a strbuf but <buf, len> pair as
parameters.
The end result of this patch achieves two thirds of the above, but
especially given that write_message() only has two call sites in a
single function, I think it is OK and preferrable even to do all
three.
^ permalink raw reply
* Re: [PATCH v4 18/25] sequencer: do not try to commit when there were merge conflicts
From: Junio C Hamano @ 2016-10-20 20:11 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <38d5f853444f80c90713f7a6e4ef1f2382549b29.1476450940.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The return value of do_recursive_merge() may be positive (indicating merge
> conflicts), or 0 (indicating success). It also may be negative, indicating
> a fatal error that requires us to abort.
>
> Now, if the return value indicates that there are merge conflicts, we
> should not try to commit those changes, of course.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index cbc3742..9ffc090 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -787,7 +787,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> res = allow;
> goto leave;
> }
> - if (!opts->no_commit)
> + if (!res && !opts->no_commit)
> res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
> opts, allow, opts->edit, 0, 0);
This by itself looks more like a bugfix than preparation for later
steps. The only reason why it is not a bugfix is because there is
nothing in this function that makes res a non-zero value and reach
this if statement at this step. We would have been caught by an
"if (res) { ... rerere(); goto leave; }" or
"if (allow < 0) { res = allow; goto leave; }"
that appear before this part of the code.
So while it is not wrong per-se, I think this should be part of an
actual change that makes it possible for the control flow to reach
here with non-zero res.
^ permalink raw reply
* Re: [PATCH v4 17/25] sequencer: support cleaning up commit messages
From: Junio C Hamano @ 2016-10-20 20:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <4173eb7716e8f4fbd083961a05dc4329686a9912.1476450940.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The run_git_commit() function already knows how to amend commits, and
> with this new option, it can also clean up commit messages (i.e. strip
> out commented lines). This is needed to implement rebase -i's 'fixup'
> and 'squash' commands as sequencer commands.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index fa77c82..cbc3742 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -481,7 +481,8 @@ static char **read_author_script(void)
> * author metadata.
> */
> static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> - int allow_empty, int edit, int amend)
> + int allow_empty, int edit, int amend,
> + int cleanup_commit_message)
> {
> char **env = NULL;
> struct argv_array array;
Looks OK to me.
This starts to look like calling for a single flag word even more,
but it is a file-local helper so we can clean it up if it becomes
necessary without affecting too many things later.
> @@ -518,9 +519,12 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> argv_array_push(&array, "-s");
> if (defmsg)
> argv_array_pushl(&array, "-F", defmsg, NULL);
> + if (cleanup_commit_message)
> + argv_array_push(&array, "--cleanup=strip");
> if (edit)
> argv_array_push(&array, "-e");
> - else if (!opts->signoff && !opts->record_origin &&
> + else if (!cleanup_commit_message &&
> + !opts->signoff && !opts->record_origin &&
> git_config_get_value("commit.cleanup", &value))
> argv_array_push(&array, "--cleanup=verbatim");
>
> @@ -785,7 +789,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> }
> if (!opts->no_commit)
> res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
> - opts, allow, opts->edit, 0);
> + opts, allow, opts->edit, 0, 0);
>
> leave:
> free_message(commit, &msg);
^ permalink raw reply
* Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline
From: Jacob Keller @ 2016-10-20 20:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list, Dennis Kaarsemaker
In-Reply-To: <xmqq4m46hmvq.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 11:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> The main part of the patch looks good. For "passing NUL to sed",
> I'd probably work it around like so:
>
Yep. I wasn't sure on the test as it was, because of the portability concern.
> t/t6000-rev-list-misc.sh | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
> index e8c6979baf..737026c34f 100755
> --- a/t/t6000-rev-list-misc.sh
> +++ b/t/t6000-rev-list-misc.sh
> @@ -4,12 +4,6 @@ test_description='miscellaneous rev-list tests'
>
> . ./test-lib.sh
>
> -test_ends_with_nul() {
> - printf "\0" >nul
> - sed '$!d' "$@" >contents
> - test_cmp_bin nul contents
> -}
> -
> test_expect_success setup '
> echo content1 >wanted_file &&
> echo content2 >unwanted_file &&
> @@ -107,8 +101,17 @@ test_expect_success '--bisect and --first-parent can not be combined' '
> '
>
> test_expect_success '--header shows a NUL after each commit' '
> - git rev-list --header --max-count=1 HEAD | sed \$!d >actual &&
> - test_ends_with_nul actual
> + # We know there is no Q in the true payload; names and
> + # addresses of the authors and the committers do not have
> + # any, and object names or header names do not, either.
> + git rev-list --header --max-count=2 HEAD |
> + nul_to_q |
> + grep "^Q" >actual &&
> + cat >expect <<-EOF &&
> + Q$(git rev-parse HEAD~1)
> + Q
> + EOF
> + test_cmp expect actual
> '
>
> test_done
I will squash this in and re-send.
Thanks,
Jake
^ permalink raw reply
* Re: tools for easily "uncommitting" parts of a patch I just commited?
From: Jacob Keller @ 2016-10-20 20:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Git mailing list
In-Reply-To: <xmqqfunqhnhf.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 11:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>> I am not sure if that is OK. I think it is less not-OK than the use
>>> case I mentioned in my earlier message, in that this is not a case
>>> that "please don't do it" breaks. It however is an inconvenience
>>> that the user has to say "git add file" before the "git commit" (or
>>> "git commit file") to conclude the sequence.
>>>
>>> So I dunno.
>>
>> Hmmm.. Ya ok I don't think we can actually distinguish between these
>> two work flows.
>
> What we might want to have in "git commit <paths>" is a new mode
> that is totally different from -i/-o that says roughly "Start from
> the tree of HEAD, pretend as if you removed all the paths that match
> the given pathspec from the tree, and then added all the entries in
> the index that match that pathspec. Write that tree and commit.
> Take nothing from the working tree". I have a feeling that when
> people do
>
> $ git add -p file1 file2 file3
> $ git commit file2
>
> and ends up including _all_ changes made to file2, not just the ones
> they picked in the earlier part of the workflow, they are expecting
> such a behaviour.
>
Right now I think people who use it intentionally do expect it to work
that way. I just happen to not have wanted to add <file> but did so
anyways without considering, and thus I ended up including changes
that were for the next commit.
As long as there is a way to change "git commit" default from that
mode then we could make the default work and then let people configure
it to what makes sense.
I'll take a look at going this route.
Thanks,
Jake
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Stefan Beller @ 2016-10-20 19:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <alpine.DEB.2.20.1610201218060.3264@virtualbox>
On Thu, Oct 20, 2016 at 3:50 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi peff,
>
> On Wed, 19 Oct 2016, Jeff King wrote:
>
>> On Wed, Oct 19, 2016 at 10:32:12AM -0700, Junio C Hamano wrote:
>>
>> > > Maybe we should start optimizing the tests...
>> >
Maybe we should stop introducing un-optimized tests.
For other reasons I just stumbled upon t7064
(porcelain V2 output for git status)
This is how an arbitrary test looks like:
test_expect_success 'staged changes in added submodule (AM S.M.)' '
( cd super_repo &&
## stage the changes in the submodule.
( cd sub1 &&
git add file_in_sub
) &&
HMOD=$(git hash-object -t blob -- .gitmodules) &&
HSUP=$(git rev-parse HEAD) &&
HSUB=$HSUP &&
cat >expect <<-EOF &&
# branch.oid $HSUP
# branch.head master
# branch.upstream origin/master
# branch.ab +0 -0
1 A. N... 000000 100644 100644 $_z40 $HMOD .gitmodules
1 AM S.M. 000000 160000 160000 $_z40 $HSUB sub1
EOF
git status --porcelain=v2 --branch --untracked-files=all >actual &&
test_cmp expect actual
)
'
Following "modern" Git tests I would have expected:
* heavy use of the "git -C <dir>" pattern. When applying that
thouroughly we'd save spanning the subshells.
* no `cd` on the same line as the opening paren.
(This is style and would derail the performance discussion)
test_expect_success 'staged changes in added submodule (AM S.M.)' '
git -C super_repo/sub1 add file_in_sub &&
HMOD=$(git -C super_repo hash-object -t blob -- .gitmodules) &&
HSUP=$(git -C super_repo rev-parse HEAD) &&
# as a comment: HSUB is equal to HSUP, because ...
cat >expect <<-EOF &&
# branch.oid $HSUP
# branch.head master
# branch.upstream origin/master
# branch.ab +0 -0
1 A. N... 000000 100644 100644 $_z40 $HMOD .gitmodules
1 AM S.M. 000000 160000 160000 $_z40 $HSUP sub1
EOF
git -C super_repo status --porcelain=v2 --branch
--untracked-files=all >../actual &&
test_cmp expect actual
'
That said I really like the idea of having a helper that would eliminate the cat
for you, e.g. :
git_test_helper_equal_stdin_or_diff_and_die -C super_repo status
--porcelain=v2 --branch --untracked-files=all <<-EOF
1 A. N... 000000 100644 100644 $_z40 $HMOD .gitmodules
1 AM S.M. 000000 160000 160000 $_z40 $HSUP sub1
EOF
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-20 19:53 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kZyCXKnt9kTbnD8KrnsjJVb_7juW2epmdP9XqcRGfg=GA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> My thought was to fix it nevertheless, such that the url recorded as
> remote.origin.url is always the first case (no l or /. at the end).
>
> If we were to add this fix to clone, then it may be easier to debug
> submodule url schemes for users as the submodule url would then
> be a concatenation of remote.origin.url and the relative part.
>
> That seems easier to understand than ${remote.origin.url%%/.} +
> relative path, maybe? (Because then the user doesn't need to guess
> or remember historical behavior that is wrong on how this)
Are you declaring that trailing / or /. will now be illegal? If you
are declaring that, then I agree that new codepaths no longer have
to worry about "strip / or /. before concatenating" and will
simplify things for them. But otherwise, such a "fix" also would
have an effect of hiding bugs from codepaths. They still need to be
prepared to see any of the three variants and cope with them
correctly, right?
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-20 19:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <xmqqzilyg6tk.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 12:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Do we actually want to fix git-clone as well?
>
> If I understand correctly, the point of this fix is to make it not
> to matter whether the original URL the end user gives or recorded as
> the remote by "git clone" in the repository is any one of:
>
> $any_leading_part/path/to/dir
> $any_leading_part/path/to/dir/
> $any_leading_part/path/to/dir/.
>
> So I do not think there is anything to "fix", as long as "git clone"
Yes "git clone" works with any of the three above.
My thought was to fix it nevertheless, such that the url recorded as
remote.origin.url is always the first case (no l or /. at the end).
If we were to add this fix to clone, then it may be easier to debug
submodule url schemes for users as the submodule url would then
be a concatenation of remote.origin.url and the relative part.
That seems easier to understand than ${remote.origin.url%%/.} +
relative path, maybe? (Because then the user doesn't need to guess
or remember historical behavior that is wrong on how this)
> that is given any one of the above three records any one of the
> above three as the result. It _may_ be desirable if the result is
> identical what was given as input, but I do not offhand think that
> is required.
>
>> I tried and then I see breakage in 5603-clone-dirname
>> as ssh://host seems to be an invalid url; it has to end with a slash?
>
> That is a separate issue, isn't it? We shouldn't be touching the
> leading "<scheme>://<host>/" part, I would think.
I agree, So I'll first fix the submodule parts only.
>
> For example, a URL "../another" relative to "ssh://host/path" may be
> "ssh://host/another", but shouldn't it be an error to take
> "../../outside" relative to "ssh://host/path"?
That is correct. I'll stop looking at clone code.
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-20 19:26 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kY4u-k8v3g5Jmod4acwJ81AnJYeYMQZ2tarx6UMGj8Xew@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> Do we actually want to fix git-clone as well?
If I understand correctly, the point of this fix is to make it not
to matter whether the original URL the end user gives or recorded as
the remote by "git clone" in the repository is any one of:
$any_leading_part/path/to/dir
$any_leading_part/path/to/dir/
$any_leading_part/path/to/dir/.
So I do not think there is anything to "fix", as long as "git clone"
that is given any one of the above three records any one of the
above three as the result. It _may_ be desirable if the result is
identical what was given as input, but I do not offhand think that
is required.
> I tried and then I see breakage in 5603-clone-dirname
> as ssh://host seems to be an invalid url; it has to end with a slash?
That is a separate issue, isn't it? We shouldn't be touching the
leading "<scheme>://<host>/" part, I would think.
For example, a URL "../another" relative to "ssh://host/path" may be
"ssh://host/another", but shouldn't it be an error to take
"../../outside" relative to "ssh://host/path"?
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-20 19:15 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <xmqqwph5m6th.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 18, 2016 at 7:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> I am not sure. Certainly we would want to make sure that the normal
>>> case (i.e. no funny trailing junk) to work correctly, but we do want
>>> to protect the fix from future breakage as well, no?
>>
>> Exactly. So not intermediate "root" that we clone from, but adapting the
>> relative URLs. Maybe half the broken tests can switch to 'root' and the others
>> go with the current behavior of cloning . to super.
>>>
>>> Perhaps we can do a preliminary step to update tests to primarily
>>> check the cases that do not involve URL with trailing "/." by either
>>> doing that double clone, or more preferrably, clone from "$(pwd)"
>>> and adjust the incorrect submodule reference that have been relying
>>> on the buggy behaviour. With that "root" trick, the test would pass
>>> with or without the fix under discussion, right?
>>
>> I assume so, (not tested).
>
> OK. Thanks for sticking with it.
Do we actually want to fix git-clone as well?
I tried and then I see breakage in 5603-clone-dirname
as ssh://host seems to be an invalid url; it has to end with a slash?
If we were to fix clone as well, then we'd still have a lot of possible stale
data (ending in /.) out there, so maybe we want to not fix clone for
now and only
fix it when computing the submodule url.
So I'll first fix up the test suite to not rely on buggy behavior and
then send this patch
with no change in tests? That sounds strange to me as it hides away the cause.
^ permalink raw reply
* Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline
From: Junio C Hamano @ 2016-10-20 18:54 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Dennis Kaarsemaker, Jacob Keller
In-Reply-To: <20161020181930.21084-1-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> writes:
> diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
> index 3e752ce03280..e8c6979baf59 100755
> --- a/t/t6000-rev-list-misc.sh
> +++ b/t/t6000-rev-list-misc.sh
> @@ -4,6 +4,12 @@ test_description='miscellaneous rev-list tests'
>
> . ./test-lib.sh
>
> +test_ends_with_nul() {
> + printf "\0" >nul
> + sed '$!d' "$@" >contents
> + test_cmp_bin nul contents
> +}
> +
> test_expect_success setup '
> echo content1 >wanted_file &&
> echo content2 >unwanted_file &&
> @@ -100,4 +106,9 @@ test_expect_success '--bisect and --first-parent can not be combined' '
> test_must_fail git rev-list --bisect --first-parent HEAD
> '
>
> +test_expect_success '--header shows a NUL after each commit' '
> + git rev-list --header --max-count=1 HEAD | sed \$!d >actual &&
> + test_ends_with_nul actual
> +'
> +
> test_done
Thanks.
The main part of the patch looks good. For "passing NUL to sed",
I'd probably work it around like so:
t/t6000-rev-list-misc.sh | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index e8c6979baf..737026c34f 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -4,12 +4,6 @@ test_description='miscellaneous rev-list tests'
. ./test-lib.sh
-test_ends_with_nul() {
- printf "\0" >nul
- sed '$!d' "$@" >contents
- test_cmp_bin nul contents
-}
-
test_expect_success setup '
echo content1 >wanted_file &&
echo content2 >unwanted_file &&
@@ -107,8 +101,17 @@ test_expect_success '--bisect and --first-parent can not be combined' '
'
test_expect_success '--header shows a NUL after each commit' '
- git rev-list --header --max-count=1 HEAD | sed \$!d >actual &&
- test_ends_with_nul actual
+ # We know there is no Q in the true payload; names and
+ # addresses of the authors and the committers do not have
+ # any, and object names or header names do not, either.
+ git rev-list --header --max-count=2 HEAD |
+ nul_to_q |
+ grep "^Q" >actual &&
+ cat >expect <<-EOF &&
+ Q$(git rev-parse HEAD~1)
+ Q
+ EOF
+ test_cmp expect actual
'
test_done
^ permalink raw reply related
* Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline
From: Dennis Kaarsemaker @ 2016-10-20 18:49 UTC (permalink / raw)
To: Jacob Keller, git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20161020181930.21084-1-jacob.e.keller@intel.com>
On Thu, 2016-10-20 at 11:19 -0700, Jacob Keller wrote:
> Here's my solution, with an updated test using a helper function based
> on using sed (which I think is more portable than tail -n1 ?). The
> change actually is very simple. I ran the test suite and it appears to
> be not breaking anyone else since the normal case is
> hdr_termination="\n" except in the cases where it needs to be NUL.
>
> Thanks for the bug report!
I like both improvements, and both 'make test' and gitweb are happy
with this version. Thanks for the quick fix.
D.
^ permalink raw reply
* Re: [PATCH] rev-list: restore the NUL commit separator in --header mode
From: Junio C Hamano @ 2016-10-20 18:46 UTC (permalink / raw)
To: Jacob Keller
Cc: Dennis Kaarsemaker, Git mailing list, Jacob Keller, Stefan Beller,
Jeff King, Johannes Sixt
In-Reply-To: <CA+P7+xq5bo-Fwa95j3aynjMP0Qw+PiuMt=hc4ngvTDpeG8nhPw@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> I did some searching, and we do use sed so I replaced it with sed \$!d
> which appears to work. I think we should probably implement a
> test_ends_with_nul or something.
As it is "a stream editor that shall read one or more text files", I
do not think "sed" is any better (or any worse) than "tail -n" from
the portability point of view. They both may happen to work on GNU
systems.
^ permalink raw reply
* Re: tools for easily "uncommitting" parts of a patch I just commited?
From: Junio C Hamano @ 2016-10-20 18:41 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jeff King, Git mailing list
In-Reply-To: <CA+P7+xqbqfCTOHJZJ9ZGXv3oeSvKUdJByc8mdzoAoXRd7UZ6HA@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
>> I am not sure if that is OK. I think it is less not-OK than the use
>> case I mentioned in my earlier message, in that this is not a case
>> that "please don't do it" breaks. It however is an inconvenience
>> that the user has to say "git add file" before the "git commit" (or
>> "git commit file") to conclude the sequence.
>>
>> So I dunno.
>
> Hmmm.. Ya ok I don't think we can actually distinguish between these
> two work flows.
What we might want to have in "git commit <paths>" is a new mode
that is totally different from -i/-o that says roughly "Start from
the tree of HEAD, pretend as if you removed all the paths that match
the given pathspec from the tree, and then added all the entries in
the index that match that pathspec. Write that tree and commit.
Take nothing from the working tree". I have a feeling that when
people do
$ git add -p file1 file2 file3
$ git commit file2
and ends up including _all_ changes made to file2, not just the ones
they picked in the earlier part of the workflow, they are expecting
such a behaviour.
^ permalink raw reply
* [PATCH] rev-list: use hdr_termination instead of a always using a newline
From: Jacob Keller @ 2016-10-20 18:19 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dennis Kaarsemaker, Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
When adding support for prefixing output of log and other commands using
--line-prefix, commit 660e113ce118 ("graph: add support for
--line-prefix on all graph-aware output", 2016-08-31) accidentally
broke rev-list --header output.
In order to make the output appear with a line-prefix, the flow was
changed to always use the graph subsystem for display. Unfortunately
the graph flow in rev-list did not use info->hdr_termination as it was
assumed that graph output would never need to putput NULs.
Since we now always use the graph code in order to handle the case of
line-prefix, simply replace putchar('\n') with
putchar(info->hdr_termination) which will correct this issue.
Add a test for the --header case to make sure we don't break it in the
future. Implement a helper function test_ends_with_nul() to make it more
obvious what sort of check we are looking for.
Reported-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Here's my solution, with an updated test using a helper function based
on using sed (which I think is more portable than tail -n1 ?). The
change actually is very simple. I ran the test suite and it appears to
be not breaking anyone else since the normal case is
hdr_termination="\n" except in the cases where it needs to be NUL.
Thanks for the bug report!
builtin/rev-list.c | 2 +-
t/t6000-rev-list-misc.sh | 11 +++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 8479f6ed28aa..c43decda7011 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -145,7 +145,7 @@ static void show_commit(struct commit *commit, void *data)
*/
if (buf.len && buf.buf[buf.len - 1] == '\n')
graph_show_padding(revs->graph);
- putchar('\n');
+ putchar(info->hdr_termination);
} else {
/*
* If the message buffer is empty, just show
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 3e752ce03280..e8c6979baf59 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -4,6 +4,12 @@ test_description='miscellaneous rev-list tests'
. ./test-lib.sh
+test_ends_with_nul() {
+ printf "\0" >nul
+ sed '$!d' "$@" >contents
+ test_cmp_bin nul contents
+}
+
test_expect_success setup '
echo content1 >wanted_file &&
echo content2 >unwanted_file &&
@@ -100,4 +106,9 @@ test_expect_success '--bisect and --first-parent can not be combined' '
test_must_fail git rev-list --bisect --first-parent HEAD
'
+test_expect_success '--header shows a NUL after each commit' '
+ git rev-list --header --max-count=1 HEAD | sed \$!d >actual &&
+ test_ends_with_nul actual
+'
+
test_done
--
2.10.0.560.g867c144
^ permalink raw reply related
* Re: tools for easily "uncommitting" parts of a patch I just commited?
From: Jacob Keller @ 2016-10-20 18:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Git mailing list
In-Reply-To: <xmqqwph2hqdg.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 10:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> I still think we're misunderstanding. I want git commit to complain
>> *only* under the following circumstance:
>>
>> I run "git add -p" and put a partial change into the index in <file>.
>> There are still other parts which were not added to the index yet.
>> Thus, the index version of the file and the actual file differ.
>>
>> Then, I (accidentally) run "git commit <file>"
>
> I agree that this case is different.
>
> Again, users are different, and I also often do
>
> $ edit file; think; decide it is a good enough first cut
> $ git add file
> $ edit file; think; decide it is getting better
> $ git add file
> $ edit file; think; decide it is now perfect
> $ git commit file
>
> Because I do not think you can differentiate the above workflow from
> the case where "git add -p" was used earlier, I think your updated
> "git commit" needs to complain at this point.
>
> I am not sure if that is OK. I think it is less not-OK than the use
> case I mentioned in my earlier message, in that this is not a case
> that "please don't do it" breaks. It however is an inconvenience
> that the user has to say "git add file" before the "git commit" (or
> "git commit file") to conclude the sequence.
>
> So I dunno.
Hmmm.. Ya ok I don't think we can actually distinguish between these
two work flows.
Given that I now know how to fix my mistake easily (git reset -p) I
think I will just go ahead and not bother with this as it's much less
of a pain now.
Thanks,
Jake
^ permalink raw reply
* Re: Fwd: New Defects reported by Coverity Scan for git
From: Stefan Beller @ 2016-10-20 18:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Jeff King
In-Reply-To: <xmqqoa2ehp5u.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 11:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Good to know that you have been managing it; I was mostly worried
> about not having anybody managing it (i.e. imagining Coverity
> nominated/volunteered me as manager with everybody else as viewers)
> and the new viewer requests get totally ignored by the project as
> the whole.
No, I have been paying attention, but in case I suddenly stop contributing
to the git project I thought it's better to have a couple of people there.
>
>> I see that some of these emails may be inconvenient to you, I can
>> change your role to defect viewer/contributor if you prefer.
>
> It is not a huge inconvenience to me, because any piece of e-mail
> that is addressed directly to gitster@ without CC'ing to git@vger
> and is not a follow-up to any earlier message goes to a separate
> lowest-priority mailbox that I rarely look at. But if it is easy
> to recategorize me, please do so.
done.
^ permalink raw reply
* Re: [PATCH] rev-list: restore the NUL commit separator in --header mode
From: Jacob Keller @ 2016-10-20 18:12 UTC (permalink / raw)
To: Dennis Kaarsemaker
Cc: Junio C Hamano, Git mailing list, Jacob Keller, Stefan Beller,
Jeff King, Johannes Sixt
In-Reply-To: <1476986671.28685.5.camel@kaarsemaker.net>
On Thu, Oct 20, 2016 at 11:04 AM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> On Wed, 2016-10-19 at 15:41 -0700, Junio C Hamano wrote:
>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>>
>> > + touch expect &&
>> > + printf "\0" > expect &&
>>
>>
>> What's the point of that "touch", especially if you are going to
>> overwrite it immediately after?
>
> Leftover debugging crud. I tried various ways of generating an
> actual/expect to compare.
>
>> > + git rev-list --header --max-count=1 HEAD | tail -n1 >actual &&
>>
>>
>> As "tail" is a tool for text files, it is likely unportable to use
>> "tail -n1" to grab the "last incomplete line that happens to contain
>> a single NUL".
>>
>> > + test_cmp_bin expect actual
>> > +'
>
> Yeah, I was fearing that. I didn't find anything in the testsuite that
> helps answering the question "does this file end with a NUL" and would
> appreciate a hint :)
>
> D.
I did some searching, and we do use sed so I replaced it with sed \$!d
which appears to work. I think we should probably implement a
test_ends_with_nul or something.
Thanks,
Jake
^ permalink raw reply
* [PATCH] commit parsing: replace unchecked parse_commit by parse_commit_or_die
From: Stefan Beller @ 2016-10-20 18:06 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <xmqqoa2ehp5u.fsf@gitster.mtv.corp.google.com>
The reason parse_commit() would fail at these points would be because
the repository is corrupt.
This was noticed by coverity.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
developed on pu as that's where coverity spotted it.
I have no overview if these areas are being worked on. (It may clash
with at least jc/merge-base-fp-only)
Thanks,
Stefan
builtin/blame.c | 2 +-
builtin/describe.c | 4 ++--
builtin/name-rev.c | 2 +-
builtin/show-branch.c | 4 ++--
commit.c | 2 +-
fetch-pack.c | 2 +-
6 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 992a79c..3b8564c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1801,7 +1801,7 @@ static void assign_blame(struct scoreboard *sb, int opt)
* so hold onto it in the meantime.
*/
origin_incref(suspect);
- parse_commit(commit);
+ parse_commit_or_die(commit);
if (reverse ||
(!(commit->object.flags & UNINTERESTING) &&
!(revs->max_age != -1 && commit->date < revs->max_age)))
diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a1..8299b16 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -199,7 +199,7 @@ static unsigned long finish_depth_computation(
best->depth++;
while (parents) {
struct commit *p = parents->item;
- parse_commit(p);
+ parse_commit_or_die(p);
if (!(p->object.flags & SEEN))
commit_list_insert_by_date(p, list);
p->object.flags |= c->object.flags;
@@ -322,7 +322,7 @@ static void describe(const char *arg, int last_one)
}
while (parents) {
struct commit *p = parents->item;
- parse_commit(p);
+ parse_commit_or_die(p);
if (!(p->object.flags & SEEN))
commit_list_insert_by_date(p, &list);
p->object.flags |= c->object.flags;
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48..92c3316 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -29,7 +29,7 @@ static void name_rev(struct commit *commit,
struct commit_list *parents;
int parent_number = 1;
- parse_commit(commit);
+ parse_commit_or_die(commit);
if (commit->date < cutoff)
return;
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 974f340..fd911b5 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -218,7 +218,7 @@ static void join_revs(struct commit_list **list_p,
parents = parents->next;
if ((this_flag & flags) == flags)
continue;
- parse_commit(p);
+ parse_commit_or_die(p);
if (mark_seen(p, seen_p) && !still_interesting)
extra--;
p->object.flags |= flags;
@@ -835,7 +835,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
if (!commit)
die(_("cannot find commit %s (%s)"),
ref_name[num_rev], oid_to_hex(&revkey));
- parse_commit(commit);
+ parse_commit_or_die(commit);
mark_seen(commit, &seen);
/* rev#0 uses bit REV_SHIFT, rev#1 uses bit REV_SHIFT+1,
diff --git a/commit.c b/commit.c
index b9c0c81..5b23eaf 100644
--- a/commit.c
+++ b/commit.c
@@ -910,7 +910,7 @@ static void mark_redundant(struct commit **array, int cnt)
ALLOC_ARRAY(filled_index, cnt - 1);
for (i = 0; i < cnt; i++)
- parse_commit(array[i]);
+ parse_commit_or_die(array[i]);
for (i = 0; i < cnt; i++) {
struct commit_list *common;
diff --git a/fetch-pack.c b/fetch-pack.c
index cb45c34..8b4ab47 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -159,7 +159,7 @@ static const unsigned char *get_rev(void)
return NULL;
commit = prio_queue_get(&rev_list);
- parse_commit(commit);
+ parse_commit_or_die(commit);
parents = commit->parents;
commit->object.flags |= POPPED;
--
2.10.1.448.g862ec83.dirty
^ permalink raw reply related
* Re: Fwd: New Defects reported by Coverity Scan for git
From: Junio C Hamano @ 2016-10-20 18:05 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Jeff King
In-Reply-To: <CAGZ79kYTwsWy+oxnPKghWKGLEaO9qWZbrQOOejeEBOe0c-o0Pg@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> I do it most of the time, but I did not start managing it.
> And I have been pretty lax/liberal about handing out rights to do stuff,
> because I did not want to tip on anyone's toes giving to few rights
> and thereby annoying them.
Good to know that you have been managing it; I was mostly worried
about not having anybody managing it (i.e. imagining Coverity
nominated/volunteered me as manager with everybody else as viewers)
and the new viewer requests get totally ignored by the project as
the whole.
> I see that some of these emails may be inconvenient to you, I can
> change your role to defect viewer/contributor if you prefer.
It is not a huge inconvenience to me, because any piece of e-mail
that is addressed directly to gitster@ without CC'ing to git@vger
and is not a follow-up to any earlier message goes to a separate
lowest-priority mailbox that I rarely look at. But if it is easy
to recategorize me, please do so.
Thanks.
^ 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