* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 16:20 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kZrNSmPAQ6SmBzFDJtSmdCbqKcgQu4KDLfoYVkSXvo-og@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> I am not sure if the updates to the callers fulfill that purpose.
>> For example, look at this hunk.
>>
>>> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
>>> struct archiver_args *args = c->args;
>>> write_archive_entry_fn_t write_entry = c->write_entry;
>>> static struct git_attr_check *check;
>>> + static struct git_attr_result result;
>>
>> As we discussed, this caller, even when threaded, will always want
>> to ask for a fixed two attributes, so "check" being static and
>> shared across threads is perfectly fine. But we do not want to see
>> "result" shared, do we?
>
> Well all of the hunks in the patch are not threaded, so they
> don't follow a threading pattern, but the static pattern to not be
> more expensive than needed.
Is it too invasive a change to make them as if they are thread-ready
users of API that happen to know their callers are not threading?
It would be ideal if we can prepare them so that the way they
interact with the attr subsystem will not have to change after this
step.
>> In other words, ideally, I think this part of the patch should
>> rather read like this:
>>
>> static struct git_attr_check *check;
>> struct git_attr_result result[2];
>>
>> ...
>> git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
>> if (!git_check_attr(path_without_prefix, check, result)) {
>> ... use result[0] and result[1] ...
>>
>> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
>> and extra parameter to git_check_attr() so that the function can
>> make sure it matches (or exceeds) check->nr.
>
> That seems tempting from a callers perspective; I'll look into that.
For callers that prepare "check" and "result" before asking
check-attr about the attributes in "check" for many paths, it is OK
to use your "allocate with attr_result_init()" pattern. The "result"
still needs to be made non-static, though.
But many callers do not follow that; rather they do
loop to iterate over paths {
call a helper func to learn attr X for path
use the value of attr X
}
using a callchain that embeds a helper function deep inside, and
"check" is kept in the helper, check-attr function is called from
there, and "result" is not passed from the caller to the helper
(obviously, because it does not exist in the current API). See the
callchain that leads down to convert.c::convert_attrs() for a
typical example. When converted to the new API, it needs to have a
new "result" structure every time it is called, and cannot reuse the
one that was used in its previous call.
^ permalink raw reply
* Re: interactive rebase should better highlight the not-applying commit
From: Johannes Schindelin @ 2016-10-12 16:24 UTC (permalink / raw)
To: Joshua N Pritikin; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <20161012132740.dvyofl36qtualxgk@droplet>
Hi Joshua,
On Wed, 12 Oct 2016, Joshua N Pritikin wrote:
> On Tue, Oct 11, 2016 at 01:55:22PM -0700, Stefan Beller wrote:
> > On Tue, Oct 11, 2016 at 12:07 PM, Joshua N Pritikin <jpritikin@pobox.com> wrote:
> > > I assume somebody familiar with GIT's code base could make this
> > > change in about 10 minutes.
> >
> > Can you elaborate how you come to that estimate?
>
> Hm, a false belief in the general awesomeness of GIT developers?
No, a false belief in your own shortcomings, as you thought it would be
easier to address your wishes for somebody else than you.
> On Tue, Oct 11, 2016 at 02:25:19PM -0700, Stefan Beller wrote:
> > On Tue, Oct 11, 2016 at 12:07 PM, Joshua N Pritikin <jpritikin@pobox.com> wrote:
> > > As of GIT 2.8.1, if you do an interactive rebase and get some conflict
> > > in the stack of patches then the commit with the conflict is buried in
> > > 4-5 lines of output. It is visually difficult to immediately pick out
> > > which commit did not apply cleanly. I suggest highlighting the 1 line
> > > commit summary in red or green or some color to help it stand out from
> > > all the other output.
> > >
> > > I decided to suggest this change after I realized that I probably
> > > skipped a commit during an interactive rebase instead of resolving the
> > > conflict. I knew I had to skip some commit so I assumed that I just need
> > > to skip without reading the commit summary carefully. Now it is 7-15
> > > days after I did the erroneous rebase. I had to spend a few hours today
> > > with GIT's archaeology tools to find the lost code.
> >
> > Looking at the actual code, this is not as easy as one might assume,
> > because rebase is written in shell. (One of the last remaining large
> > commands in shell), and there is no color support in the die(..)
> > function.
>
> I'm sorry to hear that.
>
> > However IIUC currently rebase is completely rewritten/ported to C
> > where it is easier to add color support as we do have some color
> > support in there already.
>
> Sounds great. Is there a beta release that I can try out?
There is no release as such, unless you count Git for Windows v2.10.0.
But you can try the `interactive-rebase` branch of
https://github.com/dscho/git; please note, though, that my main aim was to
be as faithful as possible in the conversion (modulo speed, of course).
> Also, I have another wishlist item for (interactive) rebase.
Hmm. You know, I cannot say that I am a fan of wishlists for Git, unless
the originator of said wishlist takes on their responsibility as an Open
Source user to make their wishes come true.
But maybe I read it all wrong and you do want to make this happen
yourself, and you simply want a little advice how to go about it?
> Sometimes I do a rebase to fix some tiny thing 10-15 commits from HEAD.
> Maybe only 1 file is affected and there are no merge conflicts, but when
> rebase reapplies all the commits, the timestamps of lots of unmodified
> files change even though they are unmodified compared to before the
> rebase.
Well, they *were* modified, right?
A workaround would be to create a new worktree using the awesome `git
worktree` command, perform the rebase there (on an unnamed branch -- AKA
"detached HEAD", no relation to Helloween), and then come back to the
original worktree and reset --hard to the new revision. That reset would
detect that there are actually no changes required to said files.
> Since the modification times are used by 'make' to compute dependencies,
> this creates a lot of useless recompilation that slows things down. It
> would be great if rebase only changed the timestamps of files that were
> actually modified.
Rebase will always have to change those timestamps. Because it really
changes those files. So the mtimes *need* to be updated. As far as rebase
is concerned, it does not matter that the final contents are identical to
*some* previous version...
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v3 12/25] sequencer: remember the onelines when parsing the todo file
From: Junio C Hamano @ 2016-10-12 16:24 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610121347361.3492@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > + const char *arg;
>> > + int arg_len;
>> > size_t offset_in_buf;
>>
>> micronit: you can make it to size_t and lose the cast below, no?
>
> No. The primary users of arg_len call a printf() style function with %.*s,
> expecting an int. So your suggestion would lose one cast, but introduce at
> least four casts in return.
Actually my point was not the number of casts required, but more
about using the correct type to store things. Granted, I do not
expect each of the lines would ever get too long to exceed "int"
(but fit in "size_t") in practice, and from that point of view, one
may be able to argue that "int" and "size_t" are both correct types,
but that argument applies equally to offset_in_buf, so...
^ permalink raw reply
* Re: Bug with git merge-base and a packed ref
From: Jeff King @ 2016-10-12 16:32 UTC (permalink / raw)
To: Stepan Kasal; +Cc: John Keeping, git
In-Reply-To: <20161012103716.GA31533@ucw.cz>
On Wed, Oct 12, 2016 at 12:37:16PM +0200, Stepan Kasal wrote:
> A reproducer would look like that
>
> # in repo1:
> git checkout tmp
> cd ..
> git clone repo1 repo2
> cd repo1
> git rebase elsewhere tmp
> cd ../repo2
> # edit
> git commit -a -m 'Another commit'
> git pull -r
>
> The last command performs something like
> git rebase new-origin/tmp
> instead of
> git rebase --onto new-origin/tmp old-origin/tmp
>
> I'm using git version 2.10.1.windows.1
>
>
> I tried to debug the issue:
> I found that the bug happens only at the very first pull after clone.
> I was able to reproduce it with git-pull.sh
>
> The problem seems to be that command
> git merge-base --fork-point refs/remotes/origin/tmp refs/heads/tmp
> returns nothing, because the refs are packed.
The --fork-point option looks in the reflog to notice that the upstream
branch has been rebased. I don't think clone actually writes reflog
entries, though, which would explain why it happens only on the first
pull after clone.
I suspect the necessary information _is_ there, though. When we update
the tracking branch, the new reflog entry will show it going from sha1
X to sha1 Y. So my guess is that --fork-point is looking for the entry
where it became "X" (which doesn't exist, because clone did not write
it), but it _could_ find that we came from "X" in the very first reflog
entry.
That's all without looking at the code, though. I don't have time to
examine it now, but maybe that can point somebody in the right
direction.
> Could you please fix merge-base so that it understands packed refs?
I think the packed-refs thing is probably a red herring. If merge-base
didn't understand packed refs, a huge chunk of git would be horribly
broken.
-Peff
^ permalink raw reply
* Re: [PATCH] t1512: become resilient to GETTEXT_POISON build
From: Jeff King @ 2016-10-12 16:32 UTC (permalink / raw)
To: Vasco Almeida; +Cc: git
In-Reply-To: <20161012122533.32223-1-vascomalmeida@sapo.pt>
On Wed, Oct 12, 2016 at 12:25:33PM +0000, Vasco Almeida wrote:
> The concerned message was marked for translation by 0c99171
> ("get_short_sha1: mark ambiguity error for translation", 2016-09-26).
Thanks, this looks obviously correct.
-Peff
^ permalink raw reply
* [PATCH] worktree: allow the main brach of a bare repository to be checked out
From: Dennis Kaarsemaker @ 2016-10-12 16:41 UTC (permalink / raw)
To: git; +Cc: pclouds, mtutty, rappazzo
In-Reply-To: <CACsJy8CmgmGLEi0xQUY9Eo-4FkA4eDNk9WJ2LtEDVFQBjbFdCA@mail.gmail.com>
In bare repositories, get_worktrees() still returns the main repository,
so git worktree list can show it. ignore it in find_shared_symref so we
can still check out the main branch.
Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
t/t2025-worktree-add.sh | 8 ++++++++
worktree.c | 2 ++
2 files changed, 10 insertions(+)
On Sun, 2016-10-09 at 17:52 +0700, Duy Nguyen wrote:
> On Sun, Oct 9, 2016 at 2:51 PM, Dennis Kaarsemaker > <dennis@kaarsemaker.net> wrote:
> > On Sat, 2016-10-08 at 19:30 -0500, Michael Tutty wrote:
> > >
> > > The only exception seems to be merging to master. When I do git
> > > worktree add /tmp/path/to/worktree master I get an error:
> > >
> > > [fatal: 'master' is already checked out at '/path/to/bare/repo']
> > >
> >
> > The worktree code treats the base repo as a worktree, even if it's
> > bare. For the purpose of being able to do a checkout of the main branch
> > of a bare repo, this patch should do:
> >
> --snip--
>
> You're fast :) I'm still studying 8d9fdd7 (worktree.c: check whether
> branch is rebased in another worktree - 2016-04-22). But yeah that
> should fix it.
OK, so here it is as a proper patch.
D.
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 4bcc335..2996c38 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -138,6 +138,14 @@ test_expect_success 'checkout from a bare repo without "add"' '
)
'
++test_expect_success '"add" default branch of a bare repo' '
+ (
+ git clone --bare . bare2 &&
+ cd bare2 &&
+ git worktree add ../there3 master
+ )
+'
+
test_expect_success 'checkout with grafts' '
test_when_finished rm .git/info/grafts &&
test_commit abc &&
diff --git a/worktree.c b/worktree.c
index 5acfe4c..35e95b7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -345,6 +345,8 @@ const struct worktree *find_shared_symref(const char *symref,
for (i = 0; worktrees[i]; i++) {
struct worktree *wt = worktrees[i];
+ if(wt->is_bare)
+ continue;
if (wt->is_detached && !strcmp(symref, "HEAD")) {
if (is_worktree_being_rebased(wt, target)) {
--
2.10.1-356-g947a599
--
Dennis Kaarsemaker <dennis@kaarsemaker.net>
http://twitter.com/seveas
^ permalink raw reply related
* Re: [PATCH v3 13/25] sequencer: prepare for rebase -i's commit functionality
From: Junio C Hamano @ 2016-10-12 16:55 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Jakub Narębski, Johannes Sixt, Michael Haggerty
In-Reply-To: <alpine.DEB.2.20.1610121356510.3492@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Hmph, didn't we recently add parse_key_value_squoted() to build
>> read_author_script() in builtin/am.c on top of it, so that this
>> piece of code can also take advantage of and share the parser?
>
> I already pointed out that the author-script file may *not* be quoted.
I think my puzzlement comes from here. What makes it OK for "am" to
expect the contents of author-script file to be quoted but it is not
OK to expect the same here? What makes it not quoted for _this_
reader, in other words?
I am not sure what you meant by "nominally related", but the purpose
of the author-script in these two codepaths is the same, isn't it?
Somebody leaves the author information from the source (either from
an e-mailed patch or an existing commit), so that a later step can
use that pieces of information left in the file when (re)creating a
commit to record the tree made by using pieces of information from
the source.
Are our use in the author-script in these two codepaths _already_
inconsistent? IOW, "am" never writes malformed unquoted values,
while the sequencer writes out in a way that is randomly quoted or
not quoted, iow, if you fed such an author-file to "am", it wouldn't
understand it?
I fully support your position to use different codepaths, if the
file that has the same name and that is used for the same purpose
uses different format in these two separate codepaths and the users
already expect them to be different. We obviously need to have two
separate parsers.
But if that is not the case, IOW, if "am"'s author-script shares the
same issue (i.e. "'am' initially writes the file properly quoted,
but this or that can happen to change its quoting and we need to
read from such a file"), then perhaps sharing needs to happen the
other way around? This patch may prepare "rebase -i" side for the
"this or that" (I still do not know what they are) to allow the
resulting file read correctly, but the same "this or that" can break
what "am" has used and is in use there if that is the case, no?
What makes it OK for "am" to expect the contents of author-script
file to be quoted but it is not OK to expect the same here? What
makes it not quoted for _this_ reader, and doesn't "am" share the
same issue?
>> > +/*
>>
>> Offtopic: this line and the beginning of the new comment block that
>> begins with "Read the author-script" above show a suboptimal marking
>> of what is added and what is left. I wonder "diff-indent-heuristic"
>> topic by Michael can help to make it look better.
>
> Maybe. I'll try to look into that once the more serious questions about
> this patch series have been addressed.
You do not have to; the remark was meant for Michael (newly cc'ed in
the message you are responding to).
^ permalink raw reply
* Re: interactive rebase should better highlight the not-applying commit
From: Joshua N Pritikin @ 2016-10-12 17:02 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git@vger.kernel.org
In-Reply-To: <alpine.DEB.2.20.1610121815160.197091@virtualbox>
On Wed, Oct 12, 2016 at 06:24:37PM +0200, Johannes Schindelin wrote:
> No, a false belief in your own shortcomings, as you thought it would be
> easier to address your wishes for somebody else than you.
Ah, shucks, I guess I could jump in.
> But maybe I read it all wrong and you do want to make this happen
> yourself, and you simply want a little advice how to go about it?
Ugh, if you insist. You really know how to hold someone's feet to the
fire, eh?
> > On Tue, Oct 11, 2016 at 02:25:19PM -0700, Stefan Beller wrote:
> > > On Tue, Oct 11, 2016 at 12:07 PM, Joshua N Pritikin <jpritikin@pobox.com> wrote:
> > > However IIUC currently rebase is completely rewritten/ported to C
> > > where it is easier to add color support as we do have some color
> > > support in there already.
> >
> > Sounds great. Is there a beta release that I can try out?
>
> There is no release as such, unless you count Git for Windows v2.10.0.
Nope, that doesn't count. ;-)
> But you can try the `interactive-rebase` branch of
> https://github.com/dscho/git; please note, though, that my main aim was to
> be as faithful as possible in the conversion (modulo speed, of course).
Hm OK
> > Sometimes I do a rebase to fix some tiny thing 10-15 commits from HEAD.
> > Maybe only 1 file is affected and there are no merge conflicts, but when
> > rebase reapplies all the commits, the timestamps of lots of unmodified
> > files change even though they are unmodified compared to before the
> > rebase.
>
> Well, they *were* modified, right?
Were they? Isn't that just an artefact of the implementation?
> A workaround would be to create a new worktree using the awesome `git
> worktree` command, perform the rebase there (on an unnamed branch -- AKA
> "detached HEAD", no relation to Helloween), and then come back to the
> original worktree and reset --hard to the new revision. That reset would
> detect that there are actually no changes required to said files.
What would be the problem with doing this by default? Or could it be a
configuration option that can be enabled?
--
Joshua N. Pritikin, Ph.D.
Virginia Institute for Psychiatric and Behavioral Genetics
Virginia Commonwealth University
PO Box 980126
800 E Leigh St, Biotech One, Suite 1-133
Richmond, VA 23219
http://people.virginia.edu/~jnp3bc
^ permalink raw reply
* Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL
From: Stefan Beller @ 2016-10-12 17:06 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, git@vger.kernel.org, Karl A., Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1610121501390.3492@virtualbox>
On Wed, Oct 12, 2016 at 6:30 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Stefan,
>
> On Mon, 10 Oct 2016, Stefan Beller wrote:
>
>> Before 63e95beb0 (2016-04-15, submodule: port resolve_relative_url from
>> shell to C), it did not matter if the superprojects URL had a trailing
>> slash or not. It was just chopped off as one of the first steps
>> (The "remoteurl=${remoteurl%/}" near the beginning of
>> resolve_relative_url(), which was removed in said commit).
>>
>> When porting this to the C version, an off-by-one error was introduced
>> and we did not check the actual last character to be a slash, but the
>> NULL delimiter.
>
> It is a NUL delimiter, not a NULL delimiter.
null character ('\0', called NUL in ASCII)
I see. Thanks for pointing out.
>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 444ec06..a7841a5 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -95,6 +95,8 @@ static int chop_last_dir(char **remoteurl, int is_relative)
>> * NEEDSWORK: This works incorrectly on the domain and protocol part.
>> * remote_url url outcome expectation
>> * http://a.com/b ../c http://a.com/c as is
>> + * http://a.com/b/ ../c http://a.com/c same as previous line, but
>> + * ignore trailing slash in url
>> * http://a.com/b ../../c http://c error out
>> * http://a.com/b ../../../c http:/c error out
>> * http://a.com/b ../../../../c http:c error out
>> @@ -113,8 +115,8 @@ static char *relative_url(const char *remote_url,
>> struct strbuf sb = STRBUF_INIT;
>> size_t len = strlen(remoteurl);
>>
>> - if (is_dir_sep(remoteurl[len]))
>> - remoteurl[len] = '\0';
>> + if (is_dir_sep(remoteurl[len-1]))
>> + remoteurl[len-1] = '\0';
>>
>> if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
>> is_relative = 0;
>> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
>> index bf2deee..82b98f8 100755
>> --- a/t/t0060-path-utils.sh
>> +++ b/t/t0060-path-utils.sh
>> @@ -319,6 +319,7 @@ test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
>> test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
>>
>> test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" "../foo/sub/a/b/c"
>> +test_submodule_relative_url "(null)" "../foo/bar/" "../sub/a/b/c" "../foo/sub/a/b/c"
>> test_submodule_relative_url "(null)" "../foo/bar" "../submodule" "../foo/submodule"
>> test_submodule_relative_url "(null)" "../foo/submodule" "../submodule" "../foo/submodule"
>> test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
>
> I see that this already made it to `next`. I saw that because it breaks
> the build of Git for Windows (this was not noticed earlier because other
> compile failures prevented the tests from running), as now the test cases
> 173 and 177 of t0060 fail (*not* the newly introduced 163).
>
> Here is the output with -v -x:
>
> -- snip --
> [...]
> expecting success:
> actual=$(git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' '../.') &&
> test "$actual" = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
>
> +++ git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' ../.
> ++ actual=C:/git-sdk-64/usr/src/git/wip/t/.
> ++ test C:/git-sdk-64/usr/src/git/wip/t/. = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
So this wipes away one dir too much in a test that doesn't end with a
dir separator
(In Windows that is '/' and '\' only, no dots?)
> error: last command exited with $?=1
> not ok 172 - test_submodule_relative_url: (null) /usr/src/git/wip/t/trash directory.t0060-path-utils/. ../. => C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.
> #
> # actual=$(git submodule--helper
> # resolve-relative-url-test '(null)'
> # '/usr/src/git/wip/t/trash
> # directory.t0060-path-utils/.' '../.') &&
> # test "$actual" =
> # 'C:/git-sdk-64/usr/src/git/wip/t/trash
> # directory.t0060-path-utils/.'
> #
> [...]
> expecting success:
> actual=$(git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' '../submodule') &&
> test "$actual" = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/submodule'
>
> +++ git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' ../submodule
> ++ actual=C:/git-sdk-64/usr/src/git/wip/t/submodule
> ++ test C:/git-sdk-64/usr/src/git/wip/t/submodule = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/submodule'
> error: last command exited with $?=1
> not ok 176 - test_submodule_relative_url: (null) /usr/src/git/wip/t/trash directory.t0060-path-utils/. ../submodule => C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/submodule
> #
> # actual=$(git submodule--helper
> # resolve-relative-url-test '(null)'
> # '/usr/src/git/wip/t/trash
> # directory.t0060-path-utils/.' '../submodule') &&
> # test "$actual" =
> # 'C:/git-sdk-64/usr/src/git/wip/t/trash
> # directory.t0060-path-utils/submodule'
> #
> [...]
> -- snap --
>
> For comparison, this is how it succeeds in an Ubuntu VM:
>
> -- snap --
> expecting success:
> actual=$(git submodule--helper resolve-relative-url-test '(null)' '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.' '../.') &&
> test "$actual" = '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.'
>
> +++ git submodule--helper resolve-relative-url-test '(null)' '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.' ../.
> ++ actual='/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.'
> ++ test '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.' = '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.'
> + test_eval_ret_=0
> + want_trace
> + test t = t
> + test t = t
> + set +x
> ok 173 - test_submodule_relative_url: (null) /home/virtualbox/git/wip/t/trash directory.t0060-path-utils/. ../. => /home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.
> -- snap --
>
> The reason that this fails on Windows is that the POSIX->Windows path
> mangling of the MSYS2 shell strips the trailing . from "/some/directory/."
> when converting it to "C:/git-sdk-64/some/directory", and for a good
> reason: most Windows programs do not handle the trailing "." very well.
>
> One very, very ugly workaround for this newly-introduced breakage would be
> this:
>
> -- snip --
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index 82b98f8..abd82e9 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -328,11 +328,11 @@ test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
> test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
> test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
> test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
> -test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
> +test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."
> test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)/."
> test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
> test_submodule_relative_url "(null)" "$PWD" "./ " "$(pwd)/ "
> -test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$(pwd)/submodule"
> +test_submodule_relative_url "(null)" "$(pwd)/." "../submodule" "$(pwd)/submodule"
> test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$(pwd)/submodule"
> test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
> test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
> -- snap --
>
> The reasons this is ugly: we specifically test for *Unixy* paths when we
> use $PWD, as opposed to *Windowsy* paths when using $(pwd). We do this to
> ensure a certain level of confidence that running things such as
>
> git clone --recurse-submodules /z/project/.
>
> work. And now that does not work anymore.
After a while of thinking how I could fix it, it occurs to me, I could
claim the removal of the dot as a defect in the Windows path handling. ;)
But that doesn't help users.
Would it be possible to mark the last dir separator special once the
trailing dot is removed? (i.e. put a \ there, and in this patch we
only check for /)
Sounds hacky to me, though.
>
> So where to go from here?
So IIUC this patch fixed a bug in Git and introduced a very similar bug
in Git for Windows?
I have no expertise on how to deal with these path issues, but it sounds like
this dot-stripping is done too early, i.e. you'd want to first let the
Git part handle
the URL concatenation and stuff and only at the end when it comes to using
the path it should get the Windows treatment?
Thanks,
Stefan
>
> Ciao,
> Dscho
^ permalink raw reply
* Re: [PATCH v2 1/3] serialize collection of changed submodules
From: Junio C Hamano @ 2016-10-12 17:18 UTC (permalink / raw)
To: Heiko Voigt
Cc: Stefan Beller, Jeff King, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <20161012130034.GA84247@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> Which seems quite extensively long for a static function so how about
> we shorten it a bit and add a comment:
>
> /* lookup or create commit object list for submodule */
> get_commit_objects_for_submodule_path(...
Or you can even lose "get_" and "path", I guess. You are not even
"getting" commits but the array that holds them, so the caller can
use it to "get" one of them or it can even use it to "put" a new
one, no? "get-commit-objects" is a misnomer in that sense. Either
one of
get_submodule_commits_array()
submodule_commits()
perhaps? I dunno.
^ permalink raw reply
* Re: [PATCH 1/5] trailer: use singly-linked list, not doubly
From: Jeff King @ 2016-10-12 17:26 UTC (permalink / raw)
To: Christian Couder; +Cc: Jonathan Tan, git, Junio C Hamano
In-Reply-To: <CAP8UFD03sOgVVb5-VEgHxHaXBBUPJ9UTB0=Y=th8YnUCk+Aa4Q@mail.gmail.com>
On Wed, Oct 12, 2016 at 05:38:14PM +0200, Christian Couder wrote:
> On Wed, Oct 12, 2016 at 3:23 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
> > Use singly-linked lists (instead of doubly-linked lists) in trailer to
> > keep track of arguments (whether implicit from configuration or explicit
> > from the command line) and trailer items.
> >
> > This change significantly reduces the code length and simplifies the code.
>
> It's true that the code can be simplified a lot by using a
> singly-linked list, but if we already had and used some generic
> functions or macros to handle doubly-linked list, I doubt there would
> be a significant simplification (as the generic code could not be
> deleted in this case).
We didn't have such generic macros when you wrote the trailer code
originally, but we do now, in list.h (they come from the kernel's
doubly-linked list implementation). I used them recently in a series and
found them pretty pleasant and complete.
Maybe it's worth trying the conversion here to see if it simplifies the
code.
> > There are now fewer pointers to be manipulated, but most trailer
> > manipulations now require seeking from beginning to end, so there might
> > be a slight net decrease in performance; however the number of trailers
> > is usually small (10 to 15 at the most) so this should not cause a big
> > impact.
>
> By default we append new trailers at the end of the trailer list, so a
> singly-linked list is theoretically not well suited at all for
> handling trailer lists.
>
> You say that at most there is a small number of trailers, and that may
> be true indeed for the Linux kernel and most projects these days, but
> I am not sure it is a good assumption to make in general.
I agree. As somebody who has fixed quite a number of accidentally
quadratic cases in the number of refs, width of the commit graph, etc,
over the years, these things have a way of creeping up or finding
pathological cases.
You _can_ append to a singly linked list in O(1) if you keep a tail
pointer, but I think using list.h would be even nicer.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call
From: Junio C Hamano @ 2016-10-12 17:37 UTC (permalink / raw)
To: Heiko Voigt
Cc: Jeff King, Stefan Beller, git, Jens.Lehmann, Fredrik Gustafsson,
Leandro Lucarella
In-Reply-To: <20161012133338.GD84247@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
>> If we do not even have these commits locally, then there is no point
>> attempting to push, so returning 0 (i.e. it is not "needs pushing"
>> situation) is correct but it is a but subtle. It's not "we know
>> they already have them", but it is "even if we tried to push, it
>> won't do us or the other side any good." A single-liner in-code
>> comment may help.
>
> First the naming part. How about:
>
> submodule_has_commits()
Nice.
> Returning 0 here means: "No push needed" but the correct answer would
> be: "We do not know".
Is it? Perhaps I am misreading the "submodule-has-commits"; I
thought it was "the remote may or may not need updating, but we
ourselves don't have what they may need to have commits in their
submodule that are referenced by their superproject, so it would not
help them even if we pushed our submodule to them". It indeed is
different from "No push needed" (rather, "our pushing would be
pointless").
> So how about:
>
>
> if (!submodule_has_hashes(path, hashes))
> /* NEEDSWORK: The correct answer here is "We do not
> * know" instead of "No". We currently proceed pushing
> * here as if the submodules commits are available on a
> * remote, which is not always correct. */
> return 0;
I am not sure.
What should happen in this scenario?
* We have two remotes, A and B, for our superproject.
* We are not interested in one submodule at path X. Our repository
is primarily used to work on the superproject and possibly other
submodules but not the one at path X.
* We pulled from A to update ourselves. They were actively working
on the submodule we are not interested in, and path X in the
superproject records a new commit that we do not have.
* We are now trying to push to B.
Should different things happen in these two subcases?
- We are not interested in submodule at path X, so we haven't even
done "submodule init" on it.
- We are not interested in submodule at path X, so even though we
do have a rather stale clone of it, we do not usually bother
updating what is checked out at path X and commit our changes
outside that area.
I tend to think that in these two cases the same thing should
happen. I am not sure if that same thing should be rejection
(i.e. "you do not know for sure that the commit at path X of the
superproject you are pushing exists in the submodule repository at
the receiving end, so I'd refuse to push the superproject"), as it
makes the only remedy for the situation is for you to make a full
clone of the submodule you are not interested in and you have never
touched yourself in either of these two subcases.
^ permalink raw reply
* Re: [PATCH] worktree: allow the main brach of a bare repository to be checked out
From: Michael Tutty @ 2016-10-12 17:35 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: Git Mailing List, Duy Nguyen, Michael Rappazzo
In-Reply-To: <20161012164104.zcxpcwqrp5h65qmc@hurricane>
I have tested this successfully with git 2.10.1 built from source
(failing), then patched as above (passing).
Thanks!
M.
On Wed, Oct 12, 2016 at 11:41 AM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> In bare repositories, get_worktrees() still returns the main repository,
> so git worktree list can show it. ignore it in find_shared_symref so we
> can still check out the main branch.
>
> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> ---
> t/t2025-worktree-add.sh | 8 ++++++++
> worktree.c | 2 ++
> 2 files changed, 10 insertions(+)
>
> On Sun, 2016-10-09 at 17:52 +0700, Duy Nguyen wrote:
>> On Sun, Oct 9, 2016 at 2:51 PM, Dennis Kaarsemaker > <dennis@kaarsemaker.net> wrote:
>> > On Sat, 2016-10-08 at 19:30 -0500, Michael Tutty wrote:
>> > >
>> > > The only exception seems to be merging to master. When I do git
>> > > worktree add /tmp/path/to/worktree master I get an error:
>> > >
>> > > [fatal: 'master' is already checked out at '/path/to/bare/repo']
>> > >
>> >
>> > The worktree code treats the base repo as a worktree, even if it's
>> > bare. For the purpose of being able to do a checkout of the main branch
>> > of a bare repo, this patch should do:
>> >
>> --snip--
>>
>> You're fast :) I'm still studying 8d9fdd7 (worktree.c: check whether
>> branch is rebased in another worktree - 2016-04-22). But yeah that
>> should fix it.
>
> OK, so here it is as a proper patch.
>
> D.
>
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 4bcc335..2996c38 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -138,6 +138,14 @@ test_expect_success 'checkout from a bare repo without "add"' '
> )
> '
>
> ++test_expect_success '"add" default branch of a bare repo' '
> + (
> + git clone --bare . bare2 &&
> + cd bare2 &&
> + git worktree add ../there3 master
> + )
> +'
> +
> test_expect_success 'checkout with grafts' '
> test_when_finished rm .git/info/grafts &&
> test_commit abc &&
> diff --git a/worktree.c b/worktree.c
> index 5acfe4c..35e95b7 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -345,6 +345,8 @@ const struct worktree *find_shared_symref(const char *symref,
>
> for (i = 0; worktrees[i]; i++) {
> struct worktree *wt = worktrees[i];
> + if(wt->is_bare)
> + continue;
>
> if (wt->is_detached && !strcmp(symref, "HEAD")) {
> if (is_worktree_being_rebased(wt, target)) {
> --
> 2.10.1-356-g947a599
>
>
> --
> Dennis Kaarsemaker <dennis@kaarsemaker.net>
> http://twitter.com/seveas
--
Michael Tutty, CTO
e: mtutty@gforgegroup.com
t: @mtutty, @gforgegroup
v: 515-789-0772
w: http://gforgegroup.com, http://gforge.com
^ permalink raw reply
* Re: [PATCH v2 2/2] Feature Request: user defined suffix for temp files created by git-mergetool
From: Junio C Hamano @ 2016-10-12 17:59 UTC (permalink / raw)
To: David Aguilar; +Cc: git, Josef Ridky
In-Reply-To: <1550673688.5271111.1476260677732.JavaMail.zimbra@redhat.com>
Josef Ridky <jridky@redhat.com> writes:
> This is update of the second variant for request to add option to change
> suffix of name of temporary files generated by git mergetool. This
> change is requested for cases, when is git mergetool used for local
> comparison between two version of same package during package rebase.
>
> Signed-off-by: Josef Ridky <jridky@redhat.com>
> ---
David, what do you think?
I don't think you were ever CC'ed on any of the messages in
this thread, and I don't think you've commented on the topic. The
thread begins here:
https://public-inbox.org/git/1329039097.128066.1475476591437.JavaMail.zimbra@redhat.com/
In any case, I suggest update to the log message to something like
this:
Subject: mergetool: allow custom naming for temporary files
A front-end program that is spawned by "git mergetool" is given
three temporary files (e.g. it may get "x_LOCAL.txt",
"x_REMOTE.txt", and "x_BASE.txt" while merging "x.txt").
Custom wrappers to "git mergetool" benefits if they are allowed
to rename these hardcoded suffixes to match the workflow they
implement. For example, they may be used to compare and merge
two versions that is available locally, and OLD/NEW may be more
appropriate than LOCAL/REMOTE in such a context.
primarily because "the second variant" is meaningless thing to say
in our long term history, when the first variant never was recorded
there. Josef may want to elaborate more on the latter paragraph.
> Documentation/git-mergetool.txt | 22 ++++++++++++++-
> git-mergetool.sh | 60 +++++++++++++++++++++++++++++++++++++----
> 2 files changed, 76 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
> index e846c2e..a0466ac 100644
> --- a/Documentation/git-mergetool.txt
> +++ b/Documentation/git-mergetool.txt
> @@ -8,7 +8,7 @@ git-mergetool - Run merge conflict resolution tools to resolve merge conflicts
> SYNOPSIS
> --------
> [verse]
> -'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [<file>...]
> +'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [--local=<name>] [--remote=<name>] [--backup=<name>] [--base=<name>] [<file>...]
>
> DESCRIPTION
> -----------
> @@ -79,6 +79,26 @@ success of the resolution after the custom tool has exited.
> Prompt before each invocation of the merge resolution program
> to give the user a chance to skip the path.
>
> +--local=<name>::
> + Use string from <name> as part of suffix of name of temporary
> + file (local) for merging. If not set, default value is used.
> + Default suffix is LOCAL.
> +
> +--remote=<name>::
> + Use string from <name> as part of suffix of name of temporary
> + file (remote) for merging. If not set, default value is used.
> + Default suffix is REMOTE.
> +
> +--backup=<name>::
> + Use string from <name> as part of suffix of name of temporary
> + file (backup) for merging. If not set, default value is used.
> + Default suffix is BACKUP.
> +
> +--base=<name>::
> + Use string from <name> as part of suffix of name of temporary
> + file (base) for merging. If not set, default value is used.
> + Default suffix is BASE.
> +
> TEMPORARY FILES
> ---------------
> `git mergetool` creates `*.orig` backup files while resolving merges.
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index bf86270..ed9ba82 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -8,7 +8,7 @@
> # at the discretion of Junio C Hamano.
> #
>
> -USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'
> +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [--local=name] [--remote=name] [--backup=name] [--base=name] [file to merge] ...'
> SUBDIRECTORY_OK=Yes
> NONGIT_OK=Yes
> OPTIONS_SPEC=
> @@ -16,6 +16,13 @@ TOOL_MODE=merge
> . git-sh-setup
> . git-mergetool--lib
>
> +# Can be changed by user
> +LOCAL_NAME='LOCAL'
> +BASE_NAME='BASE'
> +BACKUP_NAME='BACKUP'
> +REMOTE_NAME='REMOTE'
> +
> +
> # Returns true if the mode reflects a symlink
> is_symlink () {
> test "$1" = 120000
> @@ -271,10 +278,10 @@ merge_file () {
> BASE=${BASE##*/}
> fi
>
> - BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
> - LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
> - REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
> - BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
> + BACKUP="$MERGETOOL_TMPDIR/${BASE}_${BACKUP_NAME}_$$$ext"
> + LOCAL="$MERGETOOL_TMPDIR/${BASE}_${LOCAL_NAME}_$$$ext"
> + REMOTE="$MERGETOOL_TMPDIR/${BASE}_${REMOTE_NAME}_$$$ext"
> + BASE="$MERGETOOL_TMPDIR/${BASE}_${BASE_NAME}_$$$ext"
>
> base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}')
> local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}')
> @@ -396,6 +403,18 @@ do
> --prompt)
> prompt=true
> ;;
> + --local=*)
> + LOCAL_NAME=${1#--local=}
> + ;;
> + --remote=*)
> + REMOTE_NAME=${1#--remote=}
> + ;;
> + --base=*)
> + BASE_NAME=${1#--base=}
> + ;;
> + --backup=*)
> + BACKUP_NAME=${1#--backup=}
> + ;;
> --)
> shift
> break
> @@ -410,6 +429,37 @@ do
> shift
> done
>
> +# sanity check after parsing command line
> +case "" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/remote/base/backup to empty."
> + ;;
> +esac
> +
> +case "$LOCAL_NAME" in
> +"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --remote/base/backup to same as --local."
> + ;;
> +esac
> +
> +case "$REMOTE_NAME" in
> +"$LOCAL_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/base/backup to same as --remote."
> + ;;
> +esac
> +
> +case "$BASE_NAME" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/remote/backup to same as --base."
> + ;;
> +esac
> +
> +case "$BACKUP_NAME" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME")
> + die "You cannot set any of --local/remote/base to same as --backup."
> + ;;
> +esac
> +
> prompt_after_failed_merge () {
> while true
> do
^ permalink raw reply
* Re* [PATCH v3 05/25] sequencer: eventually release memory allocated for the option values
From: Junio C Hamano @ 2016-10-12 18:20 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Jakub Narębski, Johannes Sixt, René Scharfe
In-Reply-To: <alpine.DEB.2.20.1610121405410.3492@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Tue, 11 Oct 2016, Junio C Hamano wrote:
>
>> The only reason why the OPT_STRDUP appeared convenient was because
>> options[] element happened to use a field in the structure directly.
>> The patch under discussion does an equivalent of
>>
>> app.x_field = xstrdup_or_null(opt_x);
>
> Oh, that xstrdup_or_null() function slipped by me. My local patches use it
> now.
It has slipped many people ;-)
-- >8 --
Subject: cocci: refactor common patterns to use xstrdup_or_null()
d64ea0f83b ("git-compat-util: add xstrdup_or_null helper",
2015-01-12) added a handy wrapper that allows us to get a duplicate
of a string or NULL if the original is NULL, but a handful of
codepath predate its introduction or just weren't aware of it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
contrib/coccinelle/xstrdup_or_null.cocci | 7 +++++++
git.c | 3 +--
imap-send.c | 6 ++----
mailmap.c | 6 ++----
refs.c | 3 +--
send-pack.c | 3 +--
trailer.c | 9 +++------
7 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/contrib/coccinelle/xstrdup_or_null.cocci b/contrib/coccinelle/xstrdup_or_null.cocci
new file mode 100644
index 0000000000..3fceef132b
--- /dev/null
+++ b/contrib/coccinelle/xstrdup_or_null.cocci
@@ -0,0 +1,7 @@
+@@
+expression E;
+expression V;
+@@
+- if (E)
+- V = xstrdup(E);
++ V = xstrdup_or_null(E);
diff --git a/git.c b/git.c
index 0f1937fd0c..f914490e14 100644
--- a/git.c
+++ b/git.c
@@ -35,8 +35,7 @@ static void save_env_before_alias(void)
orig_cwd = xgetcwd();
for (i = 0; i < ARRAY_SIZE(env_names); i++) {
orig_env[i] = getenv(env_names[i]);
- if (orig_env[i])
- orig_env[i] = xstrdup(orig_env[i]);
+ orig_env[i] = xstrdup_or_null(orig_env[i]);
}
}
diff --git a/imap-send.c b/imap-send.c
index 0f5f4760e9..9514ddc565 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1082,10 +1082,8 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f
cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
cred.host = xstrdup(srvc->host);
- if (srvc->user)
- cred.username = xstrdup(srvc->user);
- if (srvc->pass)
- cred.password = xstrdup(srvc->pass);
+ cred.username = xstrdup_or_null(srvc->user);
+ cred.password = xstrdup_or_null(srvc->pass);
credential_fill(&cred);
diff --git a/mailmap.c b/mailmap.c
index b5c521fdea..c1a79c100c 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -103,10 +103,8 @@ static void add_mapping(struct string_list *map,
} else {
struct mailmap_info *mi = xcalloc(1, sizeof(struct mailmap_info));
debug_mm("mailmap: adding (complex) entry for '%s'\n", old_email);
- if (new_name)
- mi->name = xstrdup(new_name);
- if (new_email)
- mi->email = xstrdup(new_email);
+ mi->name = xstrdup_or_null(new_name);
+ mi->email = xstrdup_or_null(new_email);
string_list_insert(&me->namemap, old_name)->util = mi;
}
diff --git a/refs.c b/refs.c
index b4e7cac7b2..62055ab091 100644
--- a/refs.c
+++ b/refs.c
@@ -791,8 +791,7 @@ struct ref_update *ref_transaction_add_update(
hashcpy(update->new_sha1, new_sha1);
if (flags & REF_HAVE_OLD)
hashcpy(update->old_sha1, old_sha1);
- if (msg)
- update->msg = xstrdup(msg);
+ update->msg = xstrdup_or_null(msg);
return update;
}
diff --git a/send-pack.c b/send-pack.c
index 90f2ac51a7..6195b43e9a 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -181,8 +181,7 @@ static int receive_status(int in, struct ref *refs)
hint->status = REF_STATUS_REMOTE_REJECT;
ret = -1;
}
- if (msg)
- hint->remote_status = xstrdup(msg);
+ hint->remote_status = xstrdup_or_null(msg);
/* start our next search from the next ref */
hint = hint->next;
}
diff --git a/trailer.c b/trailer.c
index c6ea9ac64d..aecaf9232a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -428,12 +428,9 @@ static int set_if_missing(struct conf_info *item, const char *value)
static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
{
*dst = *src;
- if (src->name)
- dst->name = xstrdup(src->name);
- if (src->key)
- dst->key = xstrdup(src->key);
- if (src->command)
- dst->command = xstrdup(src->command);
+ dst->name = xstrdup_or_null(src->name);
+ dst->key = xstrdup_or_null(src->key);
+ dst->command = xstrdup_or_null(src->command);
}
static struct trailer_item *get_conf_item(const char *name)
^ permalink raw reply related
* Re: [PATCH] worktree: allow the main brach of a bare repository to be checked out
From: Junio C Hamano @ 2016-10-12 18:37 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git, pclouds, mtutty, rappazzo
In-Reply-To: <20161012164104.zcxpcwqrp5h65qmc@hurricane>
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> OK, so here it is as a proper patch.
>
> D.
>
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 4bcc335..2996c38 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -138,6 +138,14 @@ test_expect_success 'checkout from a bare repo without "add"' '
> )
> '
>
> ++test_expect_success '"add" default branch of a bare repo' '
Huh?
> + (
> + git clone --bare . bare2 &&
> + cd bare2 &&
> + git worktree add ../there3 master
> + )
> +'
> +
> test_expect_success 'checkout with grafts' '
> test_when_finished rm .git/info/grafts &&
> test_commit abc &&
> diff --git a/worktree.c b/worktree.c
> index 5acfe4c..35e95b7 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -345,6 +345,8 @@ const struct worktree *find_shared_symref(const char *symref,
>
> for (i = 0; worktrees[i]; i++) {
> struct worktree *wt = worktrees[i];
> + if(wt->is_bare)
> + continue;
>
> if (wt->is_detached && !strcmp(symref, "HEAD")) {
> if (is_worktree_being_rebased(wt, target)) {
> --
> 2.10.1-356-g947a599
^ permalink raw reply
* Re: [PATCH] worktree: allow the main brach of a bare repository to be checked out
From: Junio C Hamano @ 2016-10-12 18:50 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git, pclouds, mtutty, rappazzo
In-Reply-To: <xmqqfuo14dnr.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>
>> OK, so here it is as a proper patch.
Here is what I queued. Duy, what do you think? It seems OK to me.
Thanks.
-- >8 --
From: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Date: Wed, 12 Oct 2016 18:41:07 +0200
Subject: [PATCH] worktree: allow the main brach of a bare repository to be
checked out
In bare repositories, get_worktrees() still returns the main repository,
so git worktree list can show it. ignore it in find_shared_symref so we
can still check out the main branch.
Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t2025-worktree-add.sh | 8 ++++++++
worktree.c | 2 ++
2 files changed, 10 insertions(+)
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 4bcc335a19..b618d6be21 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -138,6 +138,14 @@ test_expect_success 'checkout from a bare repo without "add"' '
)
'
+test_expect_success '"add" default branch of a bare repo' '
+ (
+ git clone --bare . bare2 &&
+ cd bare2 &&
+ git worktree add ../there3 master
+ )
+'
+
test_expect_success 'checkout with grafts' '
test_when_finished rm .git/info/grafts &&
test_commit abc &&
diff --git a/worktree.c b/worktree.c
index 5acfe4cd64..f7869f8d60 100644
--- a/worktree.c
+++ b/worktree.c
@@ -345,6 +345,8 @@ const struct worktree *find_shared_symref(const char *symref,
for (i = 0; worktrees[i]; i++) {
struct worktree *wt = worktrees[i];
+ if (wt->is_bare)
+ continue;
if (wt->is_detached && !strcmp(symref, "HEAD")) {
if (is_worktree_being_rebased(wt, target)) {
--
2.10.1-591-g271c03b70f
^ permalink raw reply related
* Re: Bug with git merge-base and a packed ref
From: Stepan Kasal @ 2016-10-12 19:33 UTC (permalink / raw)
To: Jeff King; +Cc: John Keeping, git
In-Reply-To: <20161012163209.oadmm7xsmm7oeumr@sigill.intra.peff.net>
Hello,
On Wed, Oct 12, 2016 at 12:32:09PM -0400, Jeff King wrote:
> The --fork-point option looks in the reflog [...]
> On Wed, Oct 12, 2016 at 12:37:16PM +0200, Stepan Kasal wrote:
> > Could you please fix merge-base so that it understands packed refs?
I bet you nailed it; nothing with packed refs.
Thanks for correcting me.
Stepan
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-12 20:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <xmqqfuo15yjm.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 12, 2016 at 9:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> I am not sure if the updates to the callers fulfill that purpose.
>>> For example, look at this hunk.
>>>
>>>> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
>>>> struct archiver_args *args = c->args;
>>>> write_archive_entry_fn_t write_entry = c->write_entry;
>>>> static struct git_attr_check *check;
>>>> + static struct git_attr_result result;
>>>
>>> As we discussed, this caller, even when threaded, will always want
>>> to ask for a fixed two attributes, so "check" being static and
>>> shared across threads is perfectly fine. But we do not want to see
>>> "result" shared, do we?
>>
>> Well all of the hunks in the patch are not threaded, so they
>> don't follow a threading pattern, but the static pattern to not be
>> more expensive than needed.
>
> Is it too invasive a change to make them as if they are thread-ready
> users of API that happen to know their callers are not threading?
> It would be ideal if we can prepare them so that the way they
> interact with the attr subsystem will not have to change after this
> step.
As far as I see the future, we do not need to change those in the future,
unless we add the threading to the current callers, which is usually a very
invasive thing?
>
>>> In other words, ideally, I think this part of the patch should
>>> rather read like this:
>>>
>>> static struct git_attr_check *check;
>>> struct git_attr_result result[2];
>>>
>>> ...
>>> git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
>>> if (!git_check_attr(path_without_prefix, check, result)) {
>>> ... use result[0] and result[1] ...
>>>
>>> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
>>> and extra parameter to git_check_attr() so that the function can
>>> make sure it matches (or exceeds) check->nr.
>>
>> That seems tempting from a callers perspective; I'll look into that.
>
> For callers that prepare "check" and "result" before asking
> check-attr about the attributes in "check" for many paths, it is OK
> to use your "allocate with attr_result_init()" pattern. The "result"
> still needs to be made non-static, though.
I do not see why we would want to have a non static result for
non threaded callers.
>
> But many callers do not follow that; rather they do
>
> loop to iterate over paths {
> call a helper func to learn attr X for path
> use the value of attr X
> }
>
> using a callchain that embeds a helper function deep inside, and
> "check" is kept in the helper, check-attr function is called from
> there, and "result" is not passed from the caller to the helper
> (obviously, because it does not exist in the current API). See the
> callchain that leads down to convert.c::convert_attrs() for a
> typical example. When converted to the new API, it needs to have a
> new "result" structure every time it is called, and cannot reuse the
> one that was used in its previous call.
Why would that be? i.e. I do not understand the reasoning/motivation
as well as what you propose to change here. Do you think of the result
being held at the top of the call chain (and there it may be just allocated
on the stack) and passed down all the way to convert_attrs, that writes
into that result?
Currently the static result in convert_attrs just works fine (as all tests pass)
and that is not the place where we'd add threading to?
Threading would be added in dir.c, a new place that will use attrs.
Maybe instead of discussing the meaning of this patch further, I should
rebase the "pathspecs can ask for attributes" patches to either experience
the problems myself or at least have a series that has a meaningful endgoal.
I thought this preparatory series may be better than sending an even
larger series.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Johannes Sixt @ 2016-10-12 20:07 UTC (permalink / raw)
To: Stefan Beller, gitster; +Cc: git, bmwill
In-Reply-To: <20161011235951.8358-1-sbeller@google.com>
Am 12.10.2016 um 01:59 schrieb Stefan Beller:
> +void git_attr_check_initl(struct git_attr_check **check_,
> + const char *one, ...)
> {
> - struct git_attr_check *check;
> int cnt;
> va_list params;
> const char *param;
> + struct git_attr_check *check;
> +
> + if (*check_)
> + return;
> +
> + attr_lock();
> + if (*check_) {
> + attr_unlock();
> + return;
> + }
> ...
> check = xcalloc(1,
> - sizeof(*check) + cnt * sizeof(*(check->check)));
> + sizeof(*check) + cnt * sizeof(*(check->attr)));
> ...
> + *check_ = check;
> + attr_unlock();
Sigh. DCLP, the Double Checked Locking Pattern. These days, it should be
common knowledge among professionals that this naïve version
_does_not_work_ [1]!
I suggest you go without it, then measure, and only *then* optimize if
it is a bottleneck. Did I read "we do not expect much contention" somewhere?
[1] http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf C++
centric, but applies to C just as well
-- Hannes
^ permalink raw reply
* [PATCH] merge-base: handle --fork-point without reflog
From: Jeff King @ 2016-10-12 20:10 UTC (permalink / raw)
To: Stepan Kasal; +Cc: John Keeping, git
In-Reply-To: <20161012163209.oadmm7xsmm7oeumr@sigill.intra.peff.net>
On Wed, Oct 12, 2016 at 12:32:09PM -0400, Jeff King wrote:
> > The problem seems to be that command
> > git merge-base --fork-point refs/remotes/origin/tmp refs/heads/tmp
> > returns nothing, because the refs are packed.
>
> The --fork-point option looks in the reflog to notice that the upstream
> branch has been rebased. I don't think clone actually writes reflog
> entries, though, which would explain why it happens only on the first
> pull after clone.
>
> I suspect the necessary information _is_ there, though. When we update
> the tracking branch, the new reflog entry will show it going from sha1
> X to sha1 Y. So my guess is that --fork-point is looking for the entry
> where it became "X" (which doesn't exist, because clone did not write
> it), but it _could_ find that we came from "X" in the very first reflog
> entry.
Actually, --fork-point gets this case right; it will put the "old" sha1
for the initial reflog entry into the list of base tips. But this
merge-base actually runs before we fetch, so there literally is no
reflog when it runs. And it doesn't get that case right.
Here's a fix. The test I added checks things more directly, but I
confirmed manually that it also fixes the rebase case that you reported.
-- >8 --
Subject: merge-base: handle --fork-point without reflog
The --fork-point option looks in the reflog to try to find
where a derived branch forked from a base branch. However,
if the reflog for the base branch is totally empty (as it
commonly is right after cloning, which does not write a
reflog entry), then our for_each_reflog call will not find
any entries, and we will come up with no merge base, even
though there may be one with the current tip of the base.
We can fix this by just adding the current tip to
our list of collected entries.
Signed-off-by: Jeff King <peff@peff.net>
---
It would actually be correct to just unconditionally add the ref tip, as
add_one_commit already drops duplicates. But it would only be necessary
in other cases if you have a broken reflog which is missing the entry
that moved us to the current tip.
builtin/merge-base.c | 3 +++
t/t6010-merge-base.sh | 6 ++++++
2 files changed, 9 insertions(+)
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index c0d1822..b572a37 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -173,6 +173,9 @@ static int handle_fork_point(int argc, const char **argv)
revs.initial = 1;
for_each_reflog_ent(refname, collect_one_reflog_ent, &revs);
+ if (!revs.nr && !get_sha1(refname, sha1))
+ add_one_commit(sha1, &revs);
+
for (i = 0; i < revs.nr; i++)
revs.commit[i]->object.flags &= ~TMP_MARK;
diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
index e0c5f44..31db7b5 100755
--- a/t/t6010-merge-base.sh
+++ b/t/t6010-merge-base.sh
@@ -260,6 +260,12 @@ test_expect_success 'using reflog to find the fork point' '
test_cmp expect3 actual
'
+test_expect_success '--fork-point works with empty reflog' '
+ git -c core.logallrefupdates=false branch no-reflog base &&
+ git merge-base --fork-point no-reflog derived &&
+ test_cmp expect3 actual
+'
+
test_expect_success 'merge-base --octopus --all for complex tree' '
# Best common ancestor for JE, JAA and JDD is JC
# JE
--
2.10.1.587.g4098016
^ permalink raw reply related
* Re: [PATCH v3 25/25] sequencer: mark all error messages for translation
From: Johannes Sixt @ 2016-10-12 20:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Jakub Narębski
In-Reply-To: <e38bf77ff57d06ca3250bc3ec71a34088b49abd0.1476120229.git.johannes.schindelin@gmx.de>
Am 10.10.2016 um 19:26 schrieb Johannes Schindelin:
> There was actually only one error message that was not yet marked for
> translation.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 676f16c..86d86ce 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -515,16 +515,19 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> if (!env) {
> const char *gpg_opt = gpg_sign_opt_quoted(opts);
>
> - return error("you have staged changes in your working "
> - "tree. If these changes are meant to be\n"
> - "squashed into the previous commit, run:\n\n"
> - " git commit --amend %s\n\n"
> - "If they are meant to go into a new commit, "
> - "run:\n\n"
> - " git commit %s\n\n"
> - "In both cases, once you're done, continue "
> - "with:\n\n"
> - " git rebase --continue\n", gpg_opt, gpg_opt);
> + return error(_("you have staged changes in your "
> + "working tree. If these changes are "
> + "meant to be\n"
> + "squashed into the previous commit, "
> + "run:\n\n"
> + " git commit --amend %s\n\n"
> + "If they are meant to go into a new "
> + "commit, run:\n\n"
> + " git commit %s\n\n"
> + "In both cases, once you're done, "
> + "continue with:\n\n"
> + " git rebase --continue\n"),
> + gpg_opt, gpg_opt);
> }
> }
>
>
Can we please have the following change instead? I think it makes sense
to deviate from the usual conventions in a case like this.
Note that this is an error() text, hence, there should not be a
fullstop on the first line. That's now a good excuse to start the next
sentence on a new line; hence, this is not a faithful conversion to _()
anymore (a will happily take authorship and all blame if you don't
want to for this reason). Also note that _( is not moved to the
beginning of the line because it would be picked up as hunk header by
git diff.
---- 8< ----
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH] sequencer: mark all error messages for translation
There was actually only one error message that was not yet marked for
translation.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
sequencer.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 95a382e..79f7aa4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -515,16 +515,20 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
if (!env) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
- return error("you have staged changes in your working "
- "tree. If these changes are meant to be\n"
- "squashed into the previous commit, run:\n\n"
- " git commit --amend %s\n\n"
- "If they are meant to go into a new commit, "
- "run:\n\n"
- " git commit %s\n\n"
- "In both cases, once you're done, continue "
- "with:\n\n"
- " git rebase --continue\n", gpg_opt, gpg_opt);
+ return error(_(
+"you have staged changes in your working tree\n"
+"If these changes are meant to be squashed into the previous commit, run:\n"
+"\n"
+" git commit --amend %s\n"
+"\n"
+"If they are meant to go into a new commit, run:\n"
+"\n"
+" git commit %s\n"
+"\n"
+"In both cases, once you're done, continue with:\n"
+"\n"
+" git rebase --continue\n"),
+ gpg_opt, gpg_opt);
}
}
--
2.10.0.343.g37bc62b
^ permalink raw reply related
* Re: Formatting problem send_mail in version 2.10.0
From: Junio C Hamano @ 2016-10-12 20:53 UTC (permalink / raw)
To: Matthieu Moy
Cc: Larry Finger, Jeff King, Mathieu Lienard--Mayor, Remi Lespinet,
git
In-Reply-To: <vpq8tttr2ps.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>> If it's not in the body of the message, then where is it?
>>
>> This point is clarified in the thread
>> http://marc.info/?l=linux-wireless&m=147625930203434&w=2, which is
>> with my upstream maintainer.
>
> Which explicitly states that the syntax is not [$number], but # $number,
> right?
But I do not think that works, either. Let's step back.
People write things like these
Cc: Stable <stable@vger.kernel.org> # 4.8
Cc: Stable <stable@vger.kernel.org> [4.8+]
in the trailer part in the body of the message. Are these lines
meant to be usable if they appear as Cc: headers of an outgoing
piece of e-mail as-is?
That question has two subissues.
* Are these RFC compliant?
* Do real-world servers accept them?
I did a quick test with my e-mail provider, by sending a test
message with the above (with <stable@vger.kernel.org> replaced with
my address, of course) on Cc: and the answer to the second question
appears to be no.
When I send the former, I get this:
550 5.1.1 <4.8>: Recipient address rejected: User unknown...'
errormsg='recipient address 4.8 not accepted by the server'
The latter gives me this:
550 5.1.1 <[4.8+]>: Recipient address rejected: User unknown...'
errormsg='recipient address 4.8 not accepted by the server'
So even if these were accepted by some servers, it is not sane
to assume that these lines are meant to be copied to Cc: headers
when sending out. "send-email --cc" needs to salvage these
malformed lines.
For me, it seems that I get correct envelope rcpt-to addresses for
both of them (i.e. "stable@vger.kernel.org" and nothing else appears
on the command line of sendmail invocation), even though I seem to
get a strange Cc: that are visible to humans:
Cc: "Stable # 4 . 8" <stable@vger.kernel.org>,
"Stable [4.8+]" <stable@vger.kernel.org>
I seem to have libmailtools-perl 2.12 on my system.
Just like Peff who tried to disable Mail::Address, it seems that
I get complaint on both of them.
"error: unable to extract a valid address from:" is followed by
Stable <stable@vger.kernel.org#4.8>
Stable <stable@vger.kernel.org[4.8+]>
which is not ideal.
If I were to issue a decree, I would say that people should stop
suggesting to put RFC-bogus things on the Cc: line. As you
mentioned, things like:
Cc: Stable (4.8+) <stable@vger.kernel.org>
Cc: "Stable 4.8+" <stable@vger.kernel.org>
are perfectly readable alternative that are still RFC-kosher.
Things may have appeared to work by accident, and things may still
work by accident, depending on the vintage and availability of
Mail::Address package (which seems to be the case), but it is not
worth risking random breakages that depends on what other people
use in the first place, no?
That is, even though people seem to expect "send-email --cc" to
somehow be able to judge that " # 4.8" and " [4.8+]" are cruft not
meant as part of a valid address, I do not think it is a realistic
expectation. How would it know "Cc: Stable <add@re.ss> 4.8, 4.9"
has garbage " 4.8, 4.9" that needs to be stripped, while "Cc: Stable
<add@re.ss> 4.8, torvalds@linux-foundation.org" has two valid
addresses that need to be CC'ed and " 4.8" is the only thing that is
unwanted?
I'd welcome to be proven wrong with a working patch that makes
"send-email --cc" to be hyper intelligent ;-), though.
^ permalink raw reply
* Re: problem with git worktree and git svn
From: Stefan Beller @ 2016-10-12 21:05 UTC (permalink / raw)
To: Mathieu Arnold, Duy Nguyen, Eric Wong; +Cc: git@vger.kernel.org
In-Reply-To: <6c83c905-b10a-7f54-873f-54186faacfc8@FreeBSD.org>
+cc Duy who knows about worktree,
+cc Eric Wong, who knows about git-svn
On Wed, Oct 12, 2016 at 7:45 AM, Mathieu Arnold <mat@freebsd.org> wrote:
> Hi,
>
> I'm a FreeBSD ports developer, and while our main repository is in
> Subversion, some of us use git-svn a lot for development.
>
> I discovered git worktree earlier this week, and I found it a great
> asset to be able to have more than one branch of my worktree accessible
> at the same time...
>
> Anyway, back to my problem, the way git-svn works, is that it looks for
> a directory named "svn" in its gitdir and if it is not present, decide
> the repository is using git-svn version 1 (whatever that is) and goes to
> parse all the revisions to recreate the svn directory.
> So I can only use git svn commands in my main worktree, the one with the
> real gitdir.
> To fix that, all I had to do is to add a symlink named svn in each
> worktree's gitdir and pointing to ../../svn.
For some definition of fix. ;)
Sure it fixes your local setup now, but would we want to use that as well here?
My gut reaction:
* not all platforms know symlinks
* IIRC there is some worktree magic that tells you the "main" dir,
so if that was used in git-svn instead it should "just work".
>
> I think all that needs to happen is that when adding a new worktree, if
> the main git directory has a "svn" directory, add a symlink to it in the
> worktree's gitdir.
>
> --
> Mathieu Arnold
>
>
^ permalink raw reply
* Re: [PATCH v3 25/25] sequencer: mark all error messages for translation
From: Junio C Hamano @ 2016-10-12 21:24 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Jakub Narębski
In-Reply-To: <d24a3823-1ed0-ad97-f02d-febab7a97590@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Can we please have the following change instead? I think it makes sense
> to deviate from the usual conventions in a case like this.
You have at least two independent changes relative to Dscho's
version.
(1) Show line breaks more prominently by avoiding "\n\n" and
breaking the string at "\n"; this matches the way how the
result would be displayed more closely to how the source looks
like.
(2) Ignore the usual indentation rule and have messages start at
the left end of the source.
Which one are you saying "makes sense" to? Both?
I guess both can be grouped together into one theme: match the way
the final output and the source code look like.
If that is the motivation behind "makes sense", I'd prefer to see
the change explained explicitly with that rationale in the log
message.
Thanks. I personally agree with that motivation (if the one I
guessed above is your motivation, that is).
> Note that this is an error() text, hence, there should not be a
> fullstop on the first line. That's now a good excuse to start the next
> sentence on a new line; hence, this is not a faithful conversion to _()
> anymore (a will happily take authorship and all blame if you don't
> want to for this reason). Also note that _( is not moved to the
> beginning of the line because it would be picked up as hunk header by
> git diff.
>
> ---- 8< ----
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Subject: [PATCH] sequencer: mark all error messages for translation
>
> There was actually only one error message that was not yet marked for
> translation.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> sequencer.c | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 95a382e..79f7aa4 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -515,16 +515,20 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> if (!env) {
> const char *gpg_opt = gpg_sign_opt_quoted(opts);
>
> - return error("you have staged changes in your working "
> - "tree. If these changes are meant to be\n"
> - "squashed into the previous commit, run:\n\n"
> - " git commit --amend %s\n\n"
> - "If they are meant to go into a new commit, "
> - "run:\n\n"
> - " git commit %s\n\n"
> - "In both cases, once you're done, continue "
> - "with:\n\n"
> - " git rebase --continue\n", gpg_opt, gpg_opt);
> + return error(_(
> +"you have staged changes in your working tree\n"
> +"If these changes are meant to be squashed into the previous commit, run:\n"
> +"\n"
> +" git commit --amend %s\n"
> +"\n"
> +"If they are meant to go into a new commit, run:\n"
> +"\n"
> +" git commit %s\n"
> +"\n"
> +"In both cases, once you're done, continue with:\n"
> +"\n"
> +" git rebase --continue\n"),
> + gpg_opt, gpg_opt);
> }
> }
^ 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