* Re: [PATCH v2 2/4] update-index: use the same structure for chmod as add
From: Junio C Hamano @ 2016-09-11 22:28 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Johannes Schindelin, Jeff King, Jan Keromnes,
Ingo Brückl, Edward Thomson
In-Reply-To: <20160911103028.5492-3-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> @@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
> PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> (parse_opt_cb *) cacheinfo_callback},
> - {OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
> - N_("override the executable bit of the listed files"),
> - PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> - chmod_callback},
> + OPT_STRING( 0, "chmod", &chmod_arg, N_("(+/-)x"),
> + N_("override the executable bit of the listed files")),
> {OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
> N_("mark files as \"not changing\""),
> PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
> @@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage_with_options(update_index_usage, options);
>
> + if (!chmod_arg)
> + force_mode = 0;
> + else if (!strcmp(chmod_arg, "-x"))
> + force_mode = 0666;
> + else if (!strcmp(chmod_arg, "+x"))
> + force_mode = 0777;
> + else
> + die(_("option 'chmod' expects \"+x\" or \"-x\""));
> +
I am afraid that this changes the behaviour drastically.
"git update-index" is an oddball command that takes options and then
processes them immediately, exactly because it was designed to take
git update-index --chmod=-x A --chmod=+x B --add C
and say things like "A and B are not in the index and you are
attempting to add them before giving me --add option".
git update-index --add --chmod=-x A --chmod=+x B C
is expected to add A as non-executable, and B and C as executable.
Many exotic parse-options callback mechanisms used in this command
were invented exactly to support its quirky way of not doing "get a
list of options and use the last one". And this patch breaks it for
only one option without changing the others.
If we were willing to take such a big backward compatiblity hit in
the upcoming release (which I personally won't be affected, but old
scripts by others need to be audited and adjusted, which I won't
volunteer to do myself), we should make such a change consistently,
e.g. "git update-index A --add --remove B" should no longer error
out when it sees A and it is not yet in the index because "--add"
hasn't been given yet, or A is in the index but is missing from the
working tree because "--remove" hasn't been given yet. Then it may
be more justifiable if "update-index --chmod=-x A --chmod=+x B"
added A as an executable. With the current form of this patch, it
is not.
Can we do this "fix" without this change?
^ permalink raw reply
* Re: [RFC/PATCH] ls-files: adding support for submodules
From: Junio C Hamano @ 2016-09-11 22:10 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <CAGZ79kaWNykEAZcjZ2Y19j2_3TF+qcz+vb8zSi1U+me3k+06LA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> The plan is to hook the ls-files machinery into
> git-grep as the way of obtaining files to grep for a pattern.
That does not make much sense to me for exactly the same reason why
the "grab the list of paths and run 'git add' on them" example in
the message you are responding to does not make sense. The use of
the thread-pool would still need to honor the submodule boundary so
that one thread may be assigned files in the top-level superproject
while another may be assigned files in lib/ submodule repository,
and the latter would be doing a rough equivalent of "git -C lib
grep" perhaps with a new option "--output-path-prefix=lib/" that
makes any and all paths that are reported from the command prefixed
with the specified string, so the result of its grepping in Makefile
may be reported as findings in lib/Makefile.
For that, it is not sufficient for the enumeration of paths done in
the top-level to just list lib/Makefile and lib/hello.py along with
Makefile and main.py, is it? You would somehow need to have a way
to tell that 'lib/' and everything in there is inside a separate
repository. Without knowing that "lib/" is its own repository, you
would not even know which files under "lib/" hierarchy in the
filesystem are actually tracked files, which you would learn only by
reading lib/.git/index, or what textconv filtering needs to be done
on them, which you would learn only by reading lib/.gitattributes
and/or lib/.git/config.
So a "ls-files" that is done internally in the end-user facing "git
grep --recurse-submodules" needs to be run _without_ recursing
itself at least once to learn "lib/" is a submodule. A flat "here
are everything we have" does not sound like a good building block.
^ permalink raw reply
* Re: git commit -p with file arguments
From: Jacob Keller @ 2016-09-11 22:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jakub Narębski, Christian Neukirchen, Git mailing list
In-Reply-To: <xmqqvay26r8u.fsf@gitster.mtv.corp.google.com>
On Sun, Sep 11, 2016 at 2:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narębski <jnareb@gmail.com> writes:
>
>> I wonder, if git-commit is to acquire such feature, what would be the
>> best interface. "git commit :0:./<path>"? "git commit -o -p <path>"
>> (that is, "git commit --only --patch <pathspec>")?
>
> Just do "git reset && git commit -p <pathspec>", I would say.
> Anything more elaborate would just confuse the end users.
>
Yes, I'm actually confused by "git commit <files>" *not* usinng what's
in the index already, so I think that isn't intuitive as is.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] git-gui: respect commit.gpgsign again
From: Junio C Hamano @ 2016-09-11 21:52 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Pat Thoyts
In-Reply-To: <alpine.DEB.2.20.1609100903010.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Thanks. There are a couple more git-gui patches waiting for quite a long
> time. So you prefer them still as patches to git-gui.git?
I prefer not to have to worry about them myself ;-) That means that
even if Pat steps down, the next maintainer for git-gui project
would not be me, so I wouldn't be making a unilateral decision to
re-root git-gui.git project one-level down.
> Also, I just noticed poor wording. Would you mind fixing it up by
>
> s/committing/& with GPG signature/
Ouch, I didn't notice. It's not in 'next' yet, so let me see if I
can futz with the history.
Thanks.
^ permalink raw reply
* Re: git commit -p with file arguments
From: Junio C Hamano @ 2016-09-11 21:50 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Christian Neukirchen, git, Jacob Keller
In-Reply-To: <b9d63103-011a-9486-2fa1-dcf3a82cbe64@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
> I wonder, if git-commit is to acquire such feature, what would be the
> best interface. "git commit :0:./<path>"? "git commit -o -p <path>"
> (that is, "git commit --only --patch <pathspec>")?
Just do "git reset && git commit -p <pathspec>", I would say.
Anything more elaborate would just confuse the end users.
^ permalink raw reply
* Re: [PATCH v3 2/4] cat-file: introduce the --filters option
From: Junio C Hamano @ 2016-09-11 21:44 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Torsten Bögershausen, Jeff King
In-Reply-To: <alpine.DEB.2.20.1609100957040.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> In other words, instead of trying to be consistent by erroring out
>> in non-regular blob case, I think the attached change on top would
>> make more sense, by consistently passing the object contents as-is
>> for all "not filtered" cases, whether it is run from the batch mode
>> or from the command line.
>> ...
>> + if ((type == OBJ_BLOB) && S_ISREG(mode)) {
>> struct strbuf strbuf = STRBUF_INIT;
>> if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
>> free(*buf);
>
> Yes, that makes most sense to me, too.
Alright; will squash it in then before merging it down to 'next'.
^ permalink raw reply
* Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams
From: Junio C Hamano @ 2016-09-11 21:42 UTC (permalink / raw)
To: Stefan Beller
Cc: Lars Schneider, git@vger.kernel.org, Jeff King,
Johannes Schindelin, Jakub Narębski, Martin-Louis Bright,
Torsten Bögershausen, Jacob Keller
In-Reply-To: <CAGZ79kYHX6-hhKW_6JEd1hBNi8R+sBY-RjZARfHDMnyLKFZnsQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> So as a caller the strbuf is in a different state in case of error
> depending whether
> the strbuf already had some data in it. I think it would be better if
> we only did
> `strbuf_setlen(sb_out, oldlen);` here, such that the caller can
> strbuf_release it
> unconditionally.
If the caller _knows_ that the strbuf is no longer needed, it can
unconditionally call strbuf_release() on it, whether its buffer was
already released or only its length was set to 0, no?
The callee is merely trying to be nice by resetting the strbuf to a
state close to the original in the error return codepath, I would
think. It may be debatable if such a niceness is needed, but it is
a different matter that does not relate to the burden imposed on the
caller.
^ permalink raw reply
* [PATCH] Do not record unstaged deleted file upon recursive merge if file was moved outside of working tree with enabled sparse-checkout.
From: Mikhail Filippov @ 2016-09-11 20:25 UTC (permalink / raw)
To: git; +Cc: Mikhail Filippov
---
merge-recursive.c | 9 +++++---
t/t6042-merge-rename-corner-cases.sh | 42 ++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+), 3 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index e349126..25dc701 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1724,9 +1724,12 @@ static int merge_content(struct merge_options *o,
*/
path_renamed_outside_HEAD = !path2 || !strcmp(path, path2);
if (!path_renamed_outside_HEAD) {
- add_cacheinfo(o, mfi.mode, &mfi.oid, path,
- 0, (!o->call_depth), 0);
- return mfi.clean;
+ struct stat st;
+ if (lstat(path, &st) == 0) {
+ add_cacheinfo(o, mfi.mode, &mfi.oid, path,
+ 0, (!o->call_depth), 0);
+ return mfi.clean;
+ }
}
} else
output(o, 2, _("Auto-merging %s"), path);
diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
index 411550d..2073e49 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -575,4 +575,46 @@ test_expect_success 'rename/rename/add-dest merge still knows about conflicting
test ! -f c
'
+test_expect_success 'move file/sparse-checkout/merge should not delete moved file' '
+ git rm -rf . &&
+ git clean -fdqx &&
+ rm -rf .git &&
+ git init &&
+
+ echo output >.gitignore &&
+ echo .gitignore >>.gitignore &&
+
+ echo b1 >b1 &&
+ git add b1 &&
+ git commit -m b1 &&
+
+ mkdir excluded &&
+ echo problem >excluded/to-be-moved.txt &&
+ git add excluded/to-be-moved.txt &&
+ git commit -m to-be-moved &&
+ git tag split_point &&
+
+ echo b2 >b2 &&
+ git add b2 &&
+ git commit -m b2 &&
+ git tag b2 &&
+
+ git reset --hard split_point &&
+
+ git mv excluded/to-be-moved.txt excluded/moved.txt &&
+ git commit -m move &&
+ git tag b1 &&
+
+ git config core.sparsecheckout true &&
+ echo "/*" >.git/info/sparse-checkout &&
+ echo "!excluded/" >>.git/info/sparse-checkout &&
+ git read-tree -mu HEAD &&
+
+ git merge -m merge b2 &&
+
+ git status >output &&
+ cp output /tmp/a &&
+ test_i18ngrep "nothing to commit" output
+'
+
test_done
--
2.7.4 (Apple Git-66)
^ permalink raw reply related
* Re: Bug: git-add .* errors out
From: Thomas Gummerer @ 2016-09-11 19:39 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPPSiy1dxZgxWg1saPcw2QWgHtZKx26oSN3g1mCv25=0AA@mail.gmail.com>
Hi,
On 09/12, Pranit Bauva wrote:
> Hey everyone,
>
> One of my friend was trying to add files using the command `git add
> .*` and got an error that "fatal: ..: '..' is outside repository"
> which did seem a little obvious to me. But then I tried to reproduce
> it in my machine with `git add ".*"` and it didn't error out. I am
> currently using git 2.9.3 on Ubuntu 15.04 while he is using git 1.9.1
> on Ubuntu 16.04. What might have gone wrong?
The difference seems to be that you quoted the .*, which leaves the .*
in place for gits internal pathspec machinery, which then only
considers paths inside of the repository.
The non quoted version your friend used meanwhile is expanded by the
shell itself, which seems to be expanding it to ., the current
directory, and .., the parent directory. This behaviour also depends
on the shell used, for me .* in bash includes the current as well as
the parent directory, while .* in zsh doesn't include either of these.
> Regards,
> Pranit Bauva
Hope this helps,
Thomas
^ permalink raw reply
* Re: Bug: git-add .* errors out
From: Andreas Schwab @ 2016-09-11 19:34 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPPSiy1dxZgxWg1saPcw2QWgHtZKx26oSN3g1mCv25=0AA@mail.gmail.com>
On Sep 12 2016, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> One of my friend was trying to add files using the command `git add
> .*` and got an error that "fatal: ..: '..' is outside repository"
> which did seem a little obvious to me. But then I tried to reproduce
> it in my machine with `git add ".*"` and it didn't error out.
Probably you were running it in a subdirectory.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Bug: git-add .* errors out
From: Pranit Bauva @ 2016-09-11 19:10 UTC (permalink / raw)
To: Git List
Hey everyone,
One of my friend was trying to add files using the command `git add
.*` and got an error that "fatal: ..: '..' is outside repository"
which did seem a little obvious to me. But then I tried to reproduce
it in my machine with `git add ".*"` and it didn't error out. I am
currently using git 2.9.3 on Ubuntu 15.04 while he is using git 1.9.1
on Ubuntu 16.04. What might have gone wrong?
Regards,
Pranit Bauva
^ permalink raw reply
* Re: Missing RPM spec file in tarball
From: Stefan Beller @ 2016-09-11 16:05 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Sergio Martín Turiel, git@vger.kernel.org
In-Reply-To: <alpine.DEB.2.20.1609101014530.129229@virtualbox>
On Sat, Sep 10, 2016 at 1:15 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Stefan,
>
> On Fri, 9 Sep 2016, Stefan Beller wrote:
>
>> On Fri, Sep 9, 2016 at 9:19 AM, Sergio Martín Turiel
>> <sergio.martin@accelya.com> wrote:
>>
>> > Can you tell me what I'm doing wrong?
>>
>> Not crying out loud when that commit was discussed on the
>> mailing list. ;)
>
> Umm, I think it would be more: "Not stepping up to maintain the RPM
> specs"...
>
> ;-)
You are right of course.
Thanks,
Stefan
>
> Ciao,
> Dscho
^ permalink raw reply
* Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams
From: Stefan Beller @ 2016-09-11 16:03 UTC (permalink / raw)
To: Lars Schneider
Cc: git@vger.kernel.org, Jeff King, Junio C Hamano,
Johannes Schindelin, Jakub Narębski, Martin-Louis Bright,
Torsten Bögershausen, Jacob Keller
In-Reply-To: <6CCFB3A0-2C3B-4142-8E62-47C3DD6AE7A3@gmail.com>
On Sun, Sep 11, 2016 at 5:33 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
> Does this convince you to keep the proposed error handling? If yes, then
> I would add a comment to the function to document that behavior explicitly!
oops. I should read the docs more carefully.
Thanks for pointing out.
Then I'd be happy with the patch as is.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v7 03/10] pkt-line: add packet_write_fmt_gently()
From: Stefan Beller @ 2016-09-11 16:01 UTC (permalink / raw)
To: Lars Schneider
Cc: git@vger.kernel.org, Jeff King, Junio C Hamano,
Johannes Schindelin, Jakub Narębski, Martin-Louis Bright,
Torsten Bögershausen, Jacob Keller
In-Reply-To: <714C1346-6A7D-4A8B-AAA2-BA936EB6B739@gmail.com>
On Sun, Sep 11, 2016 at 4:36 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>>
>> call check_pipe from write_or_die here instead of
>> reproducing that function?
>
> Yes, might be better. I wasn't sure because the check_pipe is
> not public.
>
> Where would you declare check_pipe? In cache.h?
IIRC, once upon a time the community decided to not
clutter cache.h any more as it is like a dirty kitchen sink,
piling up all unrelated things, but on the other hand that
would be handy.
> Maybe it would be more suitable to move check_pipe to
> run-command.h/c?
That's certainly possible.
I don't have a strong opinion, where the code actually
resides, but I do have a strong-ish opinion on code
duplication. ;)
^ permalink raw reply
* [GIT PULL] l10n updates for 2.10.0 maint branch
From: Jiang Xin @ 2016-09-11 14:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ray, Ray Chen, Vasco Almeida, Git List
Hi Junio,
There are some l10n updates for Git 2.10.0, please merge them to the
maint branch.
The following changes since commit e8e349249c86550d3505c4abfac28caf3d13df46:
Merge branch 'master' of https://github.com/vnwildman/git
(2016-09-02 21:29:48 +0800)
are available in the git repository at:
git://github.com/git-l10n/git-po l10n-2.10.0-rnd2.3
for you to fetch changes up to 9a4b694c539fead26833c2104c1a93d3a2b4c50a:
l10n: zh_CN: review for git v2.10.0 l10n (2016-09-11 21:34:23 +0800)
----------------------------------------------------------------
l10n-2.10.0-rnd2.3
----------------------------------------------------------------
Jiang Xin (1):
l10n: zh_CN: fixed some typos for git 2.10.0
Ray Chen (1):
l10n: zh_CN: review for git v2.10.0 l10n
Vasco Almeida (2):
l10n: pt_PT: update Portuguese translation
l10n: pt_PT: update Portuguese repository info
po/TEAMS | 5 +-
po/pt_PT.po | 739 ++++++++++++++++++++++++++++--------------------------------
po/zh_CN.po | 104 ++++-----
3 files changed, 402 insertions(+), 446 deletions(-)
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH v7 08/10] convert: modernize tests
From: Lars Schneider @ 2016-09-11 12:34 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Jeff King, Junio C Hamano,
Johannes Schindelin, Jakub Narębski, Martin-Louis Bright,
Torsten Bögershausen, Jacob Keller
In-Reply-To: <CAGZ79kbG3Zza0e_kKhz5sBZcippwjewEBasYCioJF9drEov_Aw@mail.gmail.com>
> On 09 Sep 2016, at 00:05, Stefan Beller <sbeller@google.com> wrote:
>
> On Thu, Sep 8, 2016 at 11:21 AM, <larsxschneider@gmail.com> wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> Use `test_config` to set the config, check that files are empty with
>> `test_must_be_empty`, compare files with `test_cmp`, and remove spaces
>> after ">" and "<".
>>
>> Please note that the "rot13" filter configured in "setup" keeps using
>> `git config` instead of `test_config` because subsequent tests might
>> depend on it.
>>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>
> Makes sense & Reviewed-by "Stefan Beller <sbeller@google.com>"
Thank you,
Lars
^ permalink raw reply
* Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams
From: Lars Schneider @ 2016-09-11 12:33 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Jeff King, Junio C Hamano,
Johannes Schindelin, Jakub Narębski, Martin-Louis Bright,
Torsten Bögershausen, Jacob Keller
In-Reply-To: <CAGZ79kYHX6-hhKW_6JEd1hBNi8R+sBY-RjZARfHDMnyLKFZnsQ@mail.gmail.com>
> On 08 Sep 2016, at 23:49, Stefan Beller <sbeller@google.com> wrote:
>
> On Thu, Sep 8, 2016 at 11:21 AM, <larsxschneider@gmail.com> wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> write_packetized_from_fd() and write_packetized_from_buf() write a
>> stream of packets. All content packets use the maximal packet size
>> except for the last one. After the last content packet a `flush` control
>> packet is written.
>
> I presume we need both write_* things in a later patch; can you clarify why
> we need both of them?
Since 9035d7 Git streams from fd to required filters and from buf to
non-required filters. The Git filter protocol v2 makes use of all that,
too.
https://github.com/git/git/commit/9035d75a2be9d80d82676504d69553245017f6d4
>> + if (paket_len < 0) {
>> + if (oldalloc == 0)
>> + strbuf_release(sb_out);
>
> So if old alloc is 0, we release it, which is documented as
> /**
> * Release a string buffer and the memory it used. You should not use the
> * string buffer after using this function, unless you initialize it again.
> */
>
>> + else
>> + strbuf_setlen(sb_out, oldlen);
>
> Otherwise we just set the length back, such that it looks like before.
>
> So as a caller the strbuf is in a different state in case of error
> depending whether
> the strbuf already had some data in it. I think it would be better if
> we only did
> `strbuf_setlen(sb_out, oldlen);` here, such that the caller can
> strbuf_release it
> unconditionally.
I tried to mimic the behavior of strbuf_read() [1]. The error handling
was introduced in 2fc647 [2] to ease error handling:
"This allows for easier error handling, as callers only need to call
strbuf_release() if A) the command succeeded or B) if they would have had
to do so anyway because they added something to the strbuf themselves."
Does this convince you to keep the proposed error handling? If yes, then
I would add a comment to the function to document that behavior explicitly!
[1] https://github.com/git/git/blob/cda1bbd474805e653dda8a71d4ea3790e2a66cbb/strbuf.c#L377-L383
[2] https://github.com/git/git/commit/2fc647004ac7016128372a85db8245581e493812
> Or to make things more confusing, you could use strbuf_reset in case of 0,
> as that is a strbuf_setlen internally. ;)
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH v7 05/10] pkt-line: add packet_write_gently()
From: Lars Schneider @ 2016-09-11 11:44 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Jeff King, Junio C Hamano,
Johannes Schindelin, Jakub Narębski, Martin-Louis Bright,
Torsten Bögershausen, Jacob Keller
In-Reply-To: <CAGZ79kZdroDdD5SHP+-9svSTYbJfn2vsFXAwC4aen3hMVEOOPA@mail.gmail.com>
> On 08 Sep 2016, at 23:24, Stefan Beller <sbeller@google.com> wrote:
>
> On Thu, Sep 8, 2016 at 11:21 AM, <larsxschneider@gmail.com> wrote:
>
>>
>> Add packet_write_gently() which writes arbitrary data and returns `0`
>> for success and `-1` for an error.
>
> I think documenting the return code is better done in either the header file
> or in a commend preceding the implementation instead of the commit message?
>
> Maybe just a generic comment for *_gently is good enough, maybe even no
> comment. So the commit is fine, too. I dunno.
I agree that this is too verbose as this function follows the standard
Git return value conventions (AFAIK). I'll remove this from all commit
messages.
>> This function is used by other
>> pkt-line functions in a subsequent patch.
>
> That's what I figured. Do we also need to mention that in the preceding patch
> for packet_flush_gently ?
I'll add this note to all commit messages that introduce new functions which
are used later.
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH v7 03/10] pkt-line: add packet_write_fmt_gently()
From: Lars Schneider @ 2016-09-11 11:36 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Jeff King, Junio C Hamano,
Johannes Schindelin, Jakub Narębski, Martin-Louis Bright,
Torsten Bögershausen, Jacob Keller
In-Reply-To: <CAGZ79kY0GaWuuh_MzKL6FZ7KWF2Kwhfh9qnEYd-qX8VDQWNmCQ@mail.gmail.com>
> On 08 Sep 2016, at 23:18, Stefan Beller <sbeller@google.com> wrote:
>
> On Thu, Sep 8, 2016 at 11:21 AM, <larsxschneider@gmail.com> wrote:
>
>> +static int packet_write_fmt_1(int fd, int gently,
>> + const char *fmt, va_list args)
>> +{
>> + struct strbuf buf = STRBUF_INIT;
>> + size_t count;
>> +
>> + format_packet(&buf, fmt, args);
>> + count = write_in_full(fd, buf.buf, buf.len);
>> + if (count == buf.len)
>> + return 0;
>> +
>> + if (!gently) {
>
> call check_pipe from write_or_die here instead of
> reproducing that function?
Yes, might be better. I wasn't sure because the check_pipe is
not public.
Where would you declare check_pipe? In cache.h?
Maybe it would be more suitable to move check_pipe to
run-command.h/c?
>> + if (errno == EPIPE) {
>> + if (in_async())
>> + async_exit(141);
>> +
>> + signal(SIGPIPE, SIG_DFL);
>> + raise(SIGPIPE);
>> + /* Should never happen, but just in case... */
>> + exit(141);
>> + }
>> + die_errno("packet write error");
>> + }
>> + error("packet write failed");
>> + return -1;
>
> I think the more idiomatic way is to
>
> return error(...);
>
> as error always return -1.
Of course!!
Thank you,
Lars
^ permalink raw reply
* [PATCH v2 25/25] sequencer: remove bogus hint for translators
From: Johannes Schindelin @ 2016-09-11 10:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt
In-Reply-To: <cover.1473590966.git.johannes.schindelin@gmx.de>
When translating error messages, we need to be careful *not* to translate
the todo commands such as "pick", "reword", etc because they are commands,
and Git would not understand translated versions of those commands.
Therefore, translating the commands in the error messages would simply be
misleading.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 465e018..cdff0f1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -697,8 +697,6 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
return fast_forward_to(commit->object.oid.hash, head, unborn, opts);
if (parent && parse_commit(parent) < 0)
- /* TRANSLATORS: The first %s will be "revert" or
- "cherry-pick", the second %s a SHA1 */
return error(_("%s: cannot parse parent commit %s"),
command_to_string(command),
oid_to_hex(&parent->object.oid));
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v2 24/25] sequencer: quote filenames in error messages
From: Johannes Schindelin @ 2016-09-11 10:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt
In-Reply-To: <cover.1473590966.git.johannes.schindelin@gmx.de>
[-- Attachment #1: Type: text/plain, Size: 3681 bytes --]
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>
---
sequencer.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 1e7f29e..465e018 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -255,7 +255,7 @@ static int write_with_lock_file(const char *filename,
if (append_eol && write(msg_fd, "\n", 1) < 0)
return error_errno(_("Could not write eol to '%s"), filename);
if (commit_lock_file(&msg_file) < 0)
- return error(_("Error wrapping up %s."), filename);
+ return error(_("Error wrapping up '%s'."), filename);
return 0;
}
@@ -955,16 +955,16 @@ static int read_populate_todo(struct todo_list *todo_list,
strbuf_reset(&todo_list->buf);
fd = open(todo_file, O_RDONLY);
if (fd < 0)
- return error_errno(_("Could not open %s"), todo_file);
+ return error_errno(_("Could not open '%s'"), todo_file);
if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
close(fd);
- return error(_("Could not read %s."), todo_file);
+ return error(_("Could not read '%s'."), todo_file);
}
close(fd);
res = parse_insn_buffer(todo_list->buf.buf, todo_list);
if (res)
- return error(_("Unusable instruction sheet: %s"), todo_file);
+ return error(_("Unusable instruction sheet: '%s'"), todo_file);
if (!is_rebase_i(opts)) {
enum todo_command valid =
@@ -1050,7 +1050,7 @@ static int read_populate_opts(struct replay_opts *opts)
* are pretty certain that it is syntactically correct.
*/
if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) < 0)
- return error(_("Malformed options sheet: %s"),
+ return error(_("Malformed options sheet: '%s'"),
git_path_opts_file());
return 0;
}
@@ -1093,7 +1093,7 @@ static int create_seq_dir(void)
return -1;
}
else if (mkdir(git_path_seq_dir(), 0777) < 0)
- return error_errno(_("Could not create sequencer directory %s"),
+ return error_errno(_("Could not create sequencer directory '%s'"),
git_path_seq_dir());
return 0;
}
@@ -1112,12 +1112,12 @@ static int save_head(const char *head)
strbuf_addf(&buf, "%s\n", head);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
rollback_lock_file(&head_lock);
- return error_errno(_("Could not write to %s"),
+ return error_errno(_("Could not write to '%s'"),
git_path_head_file());
}
if (commit_lock_file(&head_lock) < 0) {
rollback_lock_file(&head_lock);
- return error(_("Error wrapping up %s."), git_path_head_file());
+ return error(_("Error wrapping up '%s'."), git_path_head_file());
}
return 0;
}
@@ -1162,9 +1162,9 @@ int sequencer_rollback(struct replay_opts *opts)
return rollback_single_pick();
}
if (!f)
- return error_errno(_("cannot open %s"), git_path_head_file());
+ return error_errno(_("cannot open '%s'"), git_path_head_file());
if (strbuf_getline_lf(&buf, f)) {
- error(_("cannot read %s: %s"), git_path_head_file(),
+ error(_("cannot read '%s': %s"), git_path_head_file(),
ferror(f) ? strerror(errno) : _("unexpected end of file"));
fclose(f);
goto fail;
@@ -1203,7 +1203,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
todo_list->buf.len - offset) < 0)
return error_errno(_("Could not write to '%s'"), todo_path);
if (commit_lock_file(&todo_lock) < 0)
- return error(_("Error wrapping up %s."), todo_path);
+ return error(_("Error wrapping up '%s'."), todo_path);
return 0;
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v2 23/25] sequencer: mark action_name() for translation
From: Johannes Schindelin @ 2016-09-11 10:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt
In-Reply-To: <cover.1473590966.git.johannes.schindelin@gmx.de>
The definition of this function goes back all the way to 043a449
(sequencer: factor code out of revert builtin, 2012-01-11), long before a
serious effort was made to translate all the error messages.
It is slightly out of the context of the current patch series (whose
purpose it is to re-implement the performance critical parts of the
interactive rebase in C) to make the error messages in the sequencer
translatable, but what the heck. We'll just do it while we're looking at
this part of the code.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 5144245..1e7f29e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -176,7 +176,7 @@ int sequencer_remove_state(struct replay_opts *opts)
static const char *action_name(const struct replay_opts *opts)
{
- return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
+ return opts->action == REPLAY_REVERT ? N_("revert") : N_("cherry-pick");
}
struct commit_message {
@@ -312,10 +312,10 @@ static struct tree *empty_tree(void)
static int error_dirty_index(struct replay_opts *opts)
{
if (read_cache_unmerged())
- return error_resolve_conflict(action_name(opts));
+ return error_resolve_conflict(_(action_name(opts)));
error(_("Your local changes would be overwritten by %s."),
- action_name(opts));
+ _(action_name(opts)));
if (advice_commit_before_merge)
advise(_("Commit your changes or stash them to proceed."));
@@ -333,7 +333,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
if (checkout_fast_forward(from, to, 1))
return -1; /* the callee should have complained already */
- strbuf_addf(&sb, _("%s: fast-forward"), action_name(opts));
+ strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts)));
transaction = ref_transaction_begin(&err);
if (!transaction ||
@@ -409,7 +409,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
return error(_("%s: Unable to write new index file"),
- action_name(opts));
+ _(action_name(opts)));
rollback_lock_file(&index_lock);
if (opts->signoff)
@@ -840,14 +840,14 @@ static int read_and_refresh_cache(struct replay_opts *opts)
if (read_index_preload(&the_index, NULL) < 0) {
rollback_lock_file(&index_lock);
return error(_("git %s: failed to read the index"),
- action_name(opts));
+ _(action_name(opts)));
}
refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
if (the_index.cache_changed && index_fd >= 0) {
if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {
rollback_lock_file(&index_lock);
return error(_("git %s: failed to refresh the index"),
- action_name(opts));
+ _(action_name(opts)));
}
}
rollback_lock_file(&index_lock);
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v2 22/25] sequencer: remove overzealous assumption in rebase -i mode
From: Johannes Schindelin @ 2016-09-11 10:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt
In-Reply-To: <cover.1473590966.git.johannes.schindelin@gmx.de>
The sequencer was introduced to make the cherry-pick and revert
functionality available as library function, with the original idea
being to extend the sequencer to also implement the rebase -i
functionality.
The test to ensure that all of the commands in the script are identical
to the overall operation does not mesh well with that.
Therefore let's disable the test in rebase -i mode.
While at it, error out early if the "instruction sheet" (i.e. the todo
script) could not be parsed.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index aa949d4..5144245 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -963,7 +963,10 @@ static int read_populate_todo(struct todo_list *todo_list,
close(fd);
res = parse_insn_buffer(todo_list->buf.buf, todo_list);
- if (!res) {
+ if (res)
+ return error(_("Unusable instruction sheet: %s"), todo_file);
+
+ if (!is_rebase_i(opts)) {
enum todo_command valid =
opts->action == REPLAY_PICK ? TODO_PICK : TODO_REVERT;
int i;
@@ -977,8 +980,6 @@ static int read_populate_todo(struct todo_list *todo_list,
return error(_("Cannot revert during a cherry-pick."));
}
- if (res)
- return error(_("Unusable instruction sheet: %s"), todo_file);
return 0;
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v2 21/25] sequencer: refactor write_message()
From: Johannes Schindelin @ 2016-09-11 10:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt
In-Reply-To: <cover.1473590966.git.johannes.schindelin@gmx.de>
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 makes it easy to create new convenience function
write_file_gently(); as some of the upcoming callers of this new
function would want to append a newline character, add a flag for it in
write_file_gently(), and thus in write_with_lock_file().
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 5e5d113..aa949d4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -242,22 +242,37 @@ static void print_advice(int show_hint, struct replay_opts *opts)
}
}
-static int write_message(struct strbuf *msgbuf, const char *filename)
+static int write_with_lock_file(const char *filename,
+ const void *buf, size_t len, int append_eol)
{
static struct lock_file msg_file;
int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
if (msg_fd < 0)
return error_errno(_("Could not lock '%s'"), filename);
- if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
- return error_errno(_("Could not write to %s"), filename);
- strbuf_release(msgbuf);
+ if (write_in_full(msg_fd, buf, len) < 0)
+ return error_errno(_("Could not write to '%s'"), filename);
+ if (append_eol && write(msg_fd, "\n", 1) < 0)
+ return error_errno(_("Could not write eol to '%s"), filename);
if (commit_lock_file(&msg_file) < 0)
return error(_("Error wrapping up %s."), filename);
return 0;
}
+static int write_message(struct strbuf *msgbuf, const char *filename)
+{
+ int res = write_with_lock_file(filename, msgbuf->buf, msgbuf->len, 0);
+ strbuf_release(msgbuf);
+ return res;
+}
+
+static int write_file_gently(const char *filename,
+ const char *text, int append_eol)
+{
+ return write_with_lock_file(filename, text, strlen(text), append_eol);
+}
+
/*
* Reads a file that was presumably written by a shell script, i.e.
* with an end-of-line marker that needs to be stripped.
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v2 20/25] sequencer: left-trim lines read from the script
From: Johannes Schindelin @ 2016-09-11 10:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt
In-Reply-To: <cover.1473590966.git.johannes.schindelin@gmx.de>
Interactive rebase's scripts may be indented; we need to handle this
case, too, now that we prepare the sequencer to process interactive
rebases.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 7953a05..5e5d113 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -875,6 +875,9 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
char *end_of_object_name;
int i, saved, status, padding;
+ /* left-trim */
+ bol += strspn(bol, " \t");
+
for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
if (skip_prefix(bol, todo_command_strings[i], &bol)) {
item->command = i;
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
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;
as well as URLs for NNTP newsgroup(s).