Re: [GSoC 2014] Replacing object loading/writing layer by libgit2
Hi all, I withdraw this proposal because I realize that I won't be eligible to work in July and August as an F-1 student. Good luck to other applicants! Guanglin 2014-03-20 23:37 GMT+08:00 Guanglin Xu : > Hello, > > My name is Guanglin Xu. I am a second-year master student at Sun > Yat-sen University in China, majoring in Software Engineering. I am > also a perspective PhD student matriculated in the US. I'm planning on > doing summer projects which I can work remotely. The GSoC 2014 program > of Git project is a great choice. > > I am kind of a "skillful" Git user with 4 years' experience in 3 > projects. For example, I am a Top 5 contributor in LibVMI project > (https://github.com/bdpayne/libvmi); I host a team-made mobile app in > Github (https://itunes.apple.com/us/app/ying-yue/id689566688?ls=1&mt=8). > For more of my projects see here > (http://www.andrew.cmu.edu/user/guanglin) > > To get familiar with the flow of Git development, I worked on > microproject [2] and had corresponding conversations with Eric > Sunshine, Jacopo Notarstefano and Sun He in threads. Thank you for > their comments on my work. > > Now I've submitted my proposal to google-melange. In brief, I propose > to replace object loading/writing layer by libgit2, which comes from > the GSoC 2014 ideas page of Git project. Particularly, since I didn't > realize where the hardest part is when I looked into the initial aim > of this idea, I added a performance requirement that the new > implementation should at least not run slower than previous one. Maybe > I underestimated specific challenge in working with Git, suggestions > or warnings for this topic are all welcomed. > > Thanks for your consideration for GSoC 2014. > > Guanglin -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2014, #01; Fri, 4)
On Tue, Apr 8, 2014 at 1:35 AM, Johannes Sixt wrote: > Am 05.04.2014 11:19, schrieb Johannes Sixt: >> Am 04.04.2014 22:58, schrieb Junio C Hamano: >>> * sz/mingw-index-pack-threaded (2014-03-19) 1 commit >>> - Enable index-pack threading in msysgit. >>> >>> What is the status of this topic? A failure report exists >>> ($gmane/245170), and I am aware of Duy's $gmane/245034 but that was >>> where the discussion stalled. Is everybody waiting for everybody >>> else to get the discussion unstuck? >> >> I still have to cross-check Duy's patch. I'll hopefully get to it in the >> next days and report back. > > The test suite passes with Duy's patch ($gmane/245034), but t5302 fails > with this patch with a MinGW build. Is "this patch" the one on 'pu', or mine? > The patches touch the Cygwin configuration, but I cannot test a Cygwin build. If no one can test the Cygwin changes, I'm happy to revert it back for safety. > I have, however, lost track of what the objective of these patches is. > Is the threaded version significantly faster, and these patches are > worth it? It depends on the repo, how much deltas it has, how deep. According to b8a2486 (index-pack: support multithreaded delta resolving - 2012-05-06), going to two cores saves ~20% time. But some actual numbers on Windows would be nice. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/2] commit: add --ignore-submodules[=] parameter
Git commit honors the 'ignore' setting from .gitmodules or .git/config,
but didn't allow to override it from command line, like other commands do.
Useful values for commit are 'all' (default) or 'none'. The others
('dirty' and 'untracked') have same effect as 'none', as commit is only
interested in whether the submodule's HEAD differs from what is commited
in the superproject.
This patch depends on Jens Lehmann's patch "commit -m: commit staged
submodules regardless of ignore config". Without it,
"commit -m --ignore-submodules" would not work and tests introduced
here would fail.
Signed-off-by: Ronald Weiss
---
Documentation/git-commit.txt| 6 ++
builtin/commit.c| 8 ++-
t/t7513-commit-ignore-submodules.sh | 42 +
3 files changed, 55 insertions(+), 1 deletion(-)
create mode 100644 t/t7513-commit-ignore-submodules.sh
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 1a7616c..8d3b2db 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -13,6 +13,7 @@ SYNOPSIS
[-F | -m ] [--reset-author] [--allow-empty]
[--allow-empty-message] [--no-verify] [-e] [--author=]
[--date=] [--cleanup=] [--[no-]status]
+ [--ignore-submodules[=]]
[-i | -o] [-S[]] [--] [...]
DESCRIPTION
@@ -271,6 +272,11 @@ The possible options are:
The default can be changed using the status.showUntrackedFiles
configuration variable documented in linkgit:git-config[1].
+--ignore-submodules[=]::
+ Can be used to override any settings of the 'submodule.*.ignore'
+ option in linkgit:git-config[1] or linkgit:gitmodules[5].
+can be either "none" or "all", which is the default.
+
-v::
--verbose::
Show unified diff between the HEAD commit and what
diff --git a/builtin/commit.c b/builtin/commit.c
index 0db215b..121c185 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -360,7 +360,7 @@ static char *prepare_index(int argc, const char **argv,
const char *prefix,
*/
if (all || (also && pathspec.nr)) {
fd = hold_locked_index(&index_lock, 1);
- add_files_to_cache(also ? prefix : NULL, &pathspec, 0, NULL);
+ add_files_to_cache(also ? prefix : NULL, &pathspec, 0,
ignore_submodule_arg);
refresh_cache_or_die(refresh_flags);
update_main_cache_tree(WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
@@ -1492,6 +1492,9 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass
post-rewrite hook")),
{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg,
N_("mode"), N_("show untracked files, optional modes: all, normal, no.
(Default: all)"), PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
+ { OPTION_STRING, 0, "ignore-submodules", &ignore_submodule_arg,
N_("when"),
+ N_("ignore changes to submodules, optional when: all, none.
(Default: all)"),
+ PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
/* end commit contents options */
OPT_HIDDEN_BOOL(0, "allow-empty", &allow_empty,
@@ -1531,6 +1534,9 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
argc = parse_and_validate_options(argc, argv, builtin_commit_options,
builtin_commit_usage,
prefix, current_head, &s);
+
+ s.ignore_submodule_arg = ignore_submodule_arg;
+
if (dry_run)
return dry_run_commit(argc, argv, prefix, current_head, &s);
index_file = prepare_index(argc, argv, prefix, current_head, 0);
diff --git a/t/t7513-commit-ignore-submodules.sh
b/t/t7513-commit-ignore-submodules.sh
new file mode 100644
index 000..83ce04c
--- /dev/null
+++ b/t/t7513-commit-ignore-submodules.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+#
+# Copyright (c) 2014 Ronald Weiss
+#
+
+test_description='Test of git commit --ignore-submodules'
+
+. ./test-lib.sh
+
+test_expect_success 'create submodule' '
+ test_create_repo sm && (
+ cd sm &&
+ >foo &&
+ git add foo &&
+ git commit -m "Add foo"
+ ) &&
+ git submodule add ./sm &&
+ git commit -m "Add sm"
+'
+
+update_sm () {
+ (cd sm &&
+ echo bar >> foo &&
+ git add foo &&
+ git commit -m "Updated foo"
+ )
+}
+
+test_expect_success 'commit -a --ignore-submodules=all ignores dirty
submodule' '
+ update_sm &&
+ test_must_fail git commit -a --ignore-submodules=all -m "Update sm"
+'
+
+test_expect_success 'commit -a --ignore-submodules=none overrides ignore=all
setting' '
+ update_sm &&
+ git config su
[PATCH v3 1/2] add: add --ignore-submodules[=] parameter
Allow overriding the ignore setting from config, using the command line
parameter like diff and status have. Git add currently doesn't honor
ignore from .gitmodules, but it does honor it from .git/config. And
support for .gitmodules will come in another patch.
Useful values are 'none' and 'all' (the default), the other values
('dirty' and 'untracked') being equivalent to 'none' for add's purposes.
Also add ignore_submodules_arg parameter to function add_files_to_cache,
which will allow implementing --ignore-submodules also for other commands
using this function (for commit command, in particular, coming in
subsequent commit). This requires compilo fixes in checkout.c and commit.c
Signed-off-by: Ronald Weiss
---
I have changed order of commits, from what Jens proposed, to avoid the patch
for commit (coming right after this one) having to mess too much with add's
source code.
If anyone disagrees, let me know, and I will redo it as needed.
I'll look in to the related "add [-f] vs .gitmodules:ignore=all" problem
soon.
Documentation/git-add.txt| 7 ++-
builtin/add.c| 21 +++
builtin/checkout.c | 2 +-
builtin/commit.c | 2 +-
cache.h | 2 +-
t/t3704-add-ignore-submodules.sh | 45
6 files changed, 71 insertions(+), 8 deletions(-)
create mode 100644 t/t3704-add-ignore-submodules.sh
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 48754cb..be2e7b5 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -11,7 +11,7 @@ SYNOPSIS
'git add' [-n] [-v] [--force | -f] [--interactive | -i] [--patch | -p]
[--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
[--intent-to-add | -N] [--refresh] [--ignore-errors]
[--ignore-missing]
- [--] [...]
+ [--ignore-submodules[=]] [--] [...]
DESCRIPTION
---
@@ -160,6 +160,11 @@ today's "git add ...", ignoring removed files.
be ignored, no matter if they are already present in the work
tree or not.
+--ignore-submodules[=]::
+ Can be used to override any settings of the 'submodule.*.ignore'
+ option in linkgit:git-config[1] or linkgit:gitmodules[5].
+can be either "none" or "all", which is the default.
+
\--::
This option can be used to separate command-line options from
the list of files, (useful when filenames might be mistaken
diff --git a/builtin/add.c b/builtin/add.c
index 672adc0..72ef792 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -168,7 +168,8 @@ static void update_callback(struct diff_queue_struct *q,
static void update_files_in_cache(const char *prefix,
const struct pathspec *pathspec,
- struct update_callback_data *data)
+ struct update_callback_data *data,
+ const char *ignore_submodules_arg)
{
struct rev_info rev;
@@ -180,17 +181,24 @@ static void update_files_in_cache(const char *prefix,
rev.diffopt.format_callback = update_callback;
rev.diffopt.format_callback_data = data;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
+
+ if (ignore_submodules_arg) {
+ DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+ handle_ignore_submodules_arg(&rev.diffopt,
ignore_submodules_arg);
+ }
+
run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
}
int add_files_to_cache(const char *prefix,
- const struct pathspec *pathspec, int flags)
+ const struct pathspec *pathspec, int flags,
+ const char *ignore_submodules_arg)
{
struct update_callback_data data;
memset(&data, 0, sizeof(data));
data.flags = flags;
- update_files_in_cache(prefix, pathspec, &data);
+ update_files_in_cache(prefix, pathspec, &data, ignore_submodules_arg);
return !!data.add_errors;
}
@@ -342,6 +350,8 @@ static int ignore_add_errors, intent_to_add, ignore_missing;
static int addremove = ADDREMOVE_DEFAULT;
static int addremove_explicit = -1; /* unspecified */
+static char *ignore_submodule_arg;
+
static int ignore_removal_cb(const struct option *opt, const char *arg, int
unset)
{
/* if we are told to ignore, we are not adding removals */
@@ -367,6 +377,9 @@ static struct option builtin_add_options[] = {
OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the
index")),
OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files
which cannot be added because of errors")),
OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even
missing - files are ignored in dry run")),
+ { OPTION_STRING, 0, "ignore-submodules", &ignore_submodule_arg,
N_("when"),
+ N_("ignore changes
Re: [PATCH] git-p4: explicitly specify that HEAD is a revision
Pete Wyckoff writes:
> vdog...@ixiacom.com wrote on Mon, 07 Apr 2014 16:19 +0300:
>> 'git p4 rebase' fails with the following message if there is a file
>> named HEAD in the current directory:
>>
>> fatal: ambiguous argument 'HEAD': both revision and filename
>> Use '--' to separate paths from revisions, like this:
>> 'git [...] -- [...]'
>>
>> Take the suggestion above and explicitly state that HEAD should be
>> treated as a revision.
>>
>> Signed-off-by: Vlad Dogaru
>
> This looks obviously good to me, thanks!
>
> Junio, could you carry it into the next release? As a trivial
> fixup.
>
> Acked-by: Pete Wyckoff
Thanks; will apply directly on 'master'.
>
>> ---
>> git-p4.py | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index cdfa2df..8d11b25 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -3086,7 +3086,7 @@ class P4Rebase(Command):
>> print "Rebasing the current branch onto %s" % upstream
>> oldHead = read_pipe("git rev-parse HEAD").strip()
>> system("git rebase %s" % upstream)
>> -system("git diff-tree --stat --summary -M %s HEAD" % oldHead)
>> +system("git diff-tree --stat --summary -M %s HEAD --" % oldHead)
>> return True
>>
>> class P4Clone(P4Sync):
>> --
>> 1.8.5.2
>>
>>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-p4: explicitly specify that HEAD is a revision
vdog...@ixiacom.com wrote on Mon, 07 Apr 2014 16:19 +0300:
> 'git p4 rebase' fails with the following message if there is a file
> named HEAD in the current directory:
>
> fatal: ambiguous argument 'HEAD': both revision and filename
> Use '--' to separate paths from revisions, like this:
> 'git [...] -- [...]'
>
> Take the suggestion above and explicitly state that HEAD should be
> treated as a revision.
>
> Signed-off-by: Vlad Dogaru
This looks obviously good to me, thanks!
Junio, could you carry it into the next release? As a trivial
fixup.
Acked-by: Pete Wyckoff
> ---
> git-p4.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index cdfa2df..8d11b25 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -3086,7 +3086,7 @@ class P4Rebase(Command):
> print "Rebasing the current branch onto %s" % upstream
> oldHead = read_pipe("git rev-parse HEAD").strip()
> system("git rebase %s" % upstream)
> -system("git diff-tree --stat --summary -M %s HEAD" % oldHead)
> +system("git diff-tree --stat --summary -M %s HEAD --" % oldHead)
> return True
>
> class P4Clone(P4Sync):
> --
> 1.8.5.2
>
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Apr 2014, #02; Mon, 7)
Here are the topics that have been cooking. Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.
You can find the changes described here in the integration branches
of the repositories listed at
http://git-blame.blogspot.com/p/git-public-repositories.html
--
[New Topics]
* jl/status-added-submodule-is-never-ignored (2014-04-07) 2 commits
- commit -m: commit staged submodules regardless of ignore config
- status/commit: show staged submodules regardless of ignore config
* mh/multimail (2014-04-07) 1 commit
- git-multimail: update to version 1.0.0
--
[Stalled]
* tr/merge-recursive-index-only (2014-02-05) 3 commits
- merge-recursive: -Xindex-only to leave worktree unchanged
- merge-recursive: internal flag to avoid touching the worktree
- merge-recursive: remove dead conditional in update_stages()
(this branch is used by tr/remerge-diff.)
Will hold.
* tr/remerge-diff (2014-02-26) 5 commits
. log --remerge-diff: show what the conflict resolution changed
. name-hash: allow dir hashing even when !ignore_case
. merge-recursive: allow storing conflict hunks in index
. revision: fold all merge diff variants into an enum merge_diff_mode
. combine-diff: do not pass revs->dense_combined_merges redundantly
(this branch uses tr/merge-recursive-index-only.)
"log -p" output learns a new way to let users inspect a merge
commit by showing the differences between the automerged result
with conflicts the person who recorded the merge would have seen
and the final conflict resolution that was recorded in the merge.
Needs to be rebased, now kb/fast-hashmap topic is in.
* sz/mingw-index-pack-threaded (2014-03-19) 1 commit
- Enable index-pack threading in msysgit.
What is the status of this topic? A failure report exists
($gmane/245170), and I am aware of Duy's $gmane/245034 but that was
where the discussion stalled. Is everybody waiting for everybody
else to get the discussion unstuck?
* bc/blame-crlf-test (2014-02-18) 1 commit
- blame: add a failing test for a CRLF issue.
I have a feeling that a fix for this should be fairly isolated and
trivial (it should be just the matter of paying attention to the
crlf settings when synthesizing the fake commit)---perhaps somebody
can squash in a fix to this?
* jk/makefile (2014-02-05) 16 commits
- FIXUP
- move LESS/LV pager environment to Makefile
- Makefile: teach scripts to include make variables
- FIXUP
- Makefile: auto-build C strings from make variables
- Makefile: drop *_SQ variables
- FIXUP
- Makefile: add c-quote helper function
- Makefile: introduce sq function for shell-quoting
- Makefile: always create files via make-var
- Makefile: store GIT-* sentinel files in MAKE/
- Makefile: prefer printf to echo for GIT-*
- Makefile: use tempfile/mv strategy for GIT-*
- Makefile: introduce make-var helper function
- Makefile: fix git-instaweb dependency on gitweb
- Makefile: drop USE_GETTEXT_SCHEME from GIT-CFLAGS
Simplify the Makefile rules and macros that exist primarily for
quoting purposes, and make it easier to robustly express the
dependency rules.
Expecting a reroll.
* po/everyday-doc (2014-01-27) 1 commit
- Make 'git help everyday' work
This may make the said command to emit something, but the source is
not meant to be formatted into a manual pages to begin with, and
also its contents are a bit stale. It may be a good first step in
the right direction, but needs more work to at least get the
mark-up right before public consumption.
Will hold.
* jk/branch-at-publish-rebased (2014-01-17) 5 commits
- t1507 (rev-parse-upstream): fix typo in test title
- implement @{publish} shorthand
- branch_get: provide per-branch pushremote pointers
- branch_get: return early on error
- sha1_name: refactor upstream_mark
Give an easier access to the tracking branches from "other" side in
a triangular workflow by introducing B@{publish} that works in a
similar way to how B@{upstream} does.
Meant to be used as a basis for whatever Ram wants to build on.
Will hold.
* rb/merge-prepare-commit-msg-hook (2014-01-10) 4 commits
- merge: drop unused arg from abort_commit method signature
- merge: make prepare_to_commit responsible for write_merge_state
- t7505: ensure cleanup after hook blocks merge
- t7505: add missing &&
Expose more merge states (e.g. $GIT_DIR/MERGE_MODE) to hooks that
run during "git merge". The log message stresses too much on one
hook, prepare-commit-msg, but it would equally apply to other hooks
like post-merge, I think.
Waiting for a reroll.
* jl/submodule-recursive-checkout (2013-12-26) 5 commits
- Teach checkout to recursively checkout submodules
- submodule: teach unpack_trees() to update submodules
- submodule: teach unpack_trees() to repopulate submodules
- submodule: teach unpack_trees() to remove submodule
Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'
Christian Couder writes: > From: Junio C Hamano >> >> A different way to sell a colon, e.g. >> >> Consider the instruction sed takes on its command line. >> (e.g. "sed 's/frotz/nitfol/' > form, you would always give it as the value of an '-e' option >> (e.g. "sed -e 's/frotz/nitfol' > be loose in limited occassions. "Key:value" is like that, and >> in the most general form, it actually needs to be spelled as >> "-e 'Key:value'". >> >> is possible, but I do not think it is a particularly good analogy, >> because what you have as the alternative is "Key=value", and not >> "-e 'Key:value'", or "--Key=value" (the last would probably be the >> most natural way to express this). > > The analogy that I would use is rather that Perl lets people use > 's:foo:bar:' as well as 's=foo=bar=' instead of 's/foo/bar/' if they > prefer. I could *almost* buy that, but that does not hold as you are not allowing (and I do not think you need to in this case) the user to pick any termination character like "s|foo|bar|". The only thing you are doing is forbidding both ":" and "=" from the set of allowed characters for labels. However. I think we could buy the syntax if the "Key:value" form were the *only* form, *without* accepting "Key=value". The latter is a poor attempt to pretend as if it is a normal command line option, but because that form does not even take double-dashes at the beginning, it even fails to mimic as a command line option. It would be one way to reduce the unnecessary cognitive load from the users when learning the Git command line argument convention to reject the "key=value" form and only stick to "key: value" form. After all, because of the shape of the footer we add to the log message (i.e. a keyword label followed by a colon followed by a SP followed by the value), it is clear that we can use ":" as the separator without inconveniencing the users who want to use some unusual characters in the label part, but there is no strong reason to reject an equal sign in the label. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: notes.rewriteRef doesn't apply to rebases that skip the commit
On Apr 7, 2014, at 2:33 PM, Junio C Hamano wrote: > Kevin Ballard writes: > >> I’ve started using notes recently, and I have notes.rewriteRef set so that >> when I rebase, my notes will be kept. Unfortunately, it turns out that if a >> rebase deletes my local commit because it already exists in upstream, it >> doesn’t copy the note to the upstream commit. It seems perfectly reasonable >> to >> me to expect the note to be copied to the upstream commit, as it represents >> the same change. > > That would cut both ways, depending on the use case. I suspect that > those who use notes as remainder of what are still to be sent out > would appreciate the current behavior. It depends on how things are sent out. I know Git operates by sending all patches to the ML, which are then reapplied, so they end up with a different commit hash. But in most of the projects I've worked in, the main workflow ends up with commits getting merged into master without getting rewritten. The reason why I'm requesting this behavior is that committing without rewriting isn't necessarily a strict rule. For example, in the project that I'm using notes for, every commit needs to go through Gerrit for code review. Normally it gets reviewed and merged into origin/master without a rewrite, and my note is preserved. But sometimes someone else will sneak a commit in first and I'll need to rebase. If I rebase locally, that works, but Gerrit also offers to rebase my commit for me. And if I let Gerrit do it, I still want my note to be preserved. In the end, there should be no practical difference between me rebasing and Gerrit rebasing. In general, Git doesn't know what the user is using notes for. If the user has requested that notes persist through rewrite operations, it seems reasonable that Git should recognize rewrites that happened remotely too, not just locally. As for your particular example of tracking what still needs to be sent out, I'm not sure I understand that example. If I `git push` or use format-patch and send an email, isn't that sending it out? Therefore I need to update/delete my note explicitly. And if I want to track what hasn't made it into origin/master yet, well, the origin/master ref already does that for me. >> Another potential issues is if the commit exists upstream, but the >> surrounding >> context has changed enough that it contains a different patch-id. In this >> case, I would want Git to take the extra effort to correlate the upstream >> commit with my local one (it has the same message, modulo any Signed-Off-By >> lines, the same authorship info, and all the - and + lines in the diff are >> identical). > > That would be an orthogonal improvement, I would think. Such a > smarter "patch-id may mistake it, but it is a moral equivalent" > detection would not only be useful for copying notes, but also for > skipping the commit from getting replayed in the first place, no? Perhaps, but replaying an empty commit already does nothing. Although I suppose `git rebase` does have the `--keep-empty` flag, so it might be useful there. >> On a semi-related note, I don't see why Git should be warning about >> notes.displayRef evaluating to a reference that doesn't exist. It doesn't >> exist because I haven't created any notes for that ref in this repository >> yet. >> But that doesn't mean I won't be creating them eventually, and when I do I >> want them to be displayed. > > That also cuts both ways. I think a warning is primarily to let > those who mistyped the refname take notice. I get that, but I don't think that's particularly important. There's no practical difference between typoing the ref in notes.displayRef and forgtting to set up notes.displayRef in the first place. Git certainly can't warn about the latter. And the warning about the former is quite annoying if I did not in fact typo, but rather just haven’t created any notes in that ref yet. -Kevin Ballard-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2.1] commit: add --ignore-submodules[=] parameter
On 6. 4. 2014 18:28, Jens Lehmann wrote: > Am 02.04.2014 21:56, schrieb Ronald Weiss: >> On 2. 4. 2014 20:53, Jens Lehmann wrote: >>> Am 01.04.2014 23:59, schrieb Ronald Weiss: On 1. 4. 2014 22:23, Jens Lehmann wrote: > Am 01.04.2014 01:35, schrieb Ronald Weiss: >> On 1. 4. 2014 0:50, Ronald Weiss wrote: >>> On 31. 3. 2014 23:47, Ronald Weiss wrote: On Mon, Mar 31, 2014 at 8:58 PM, Jens Lehmann wrote: > As Junio mentioned it would be great if you could teach the add > command also honor the --ignore-submodule command line option in > a companion patch. In the course of doing so you'll easily see if > I was right or not, then please just order them in the most logical > way. Well, if You (or Junio) really don't want my patch without another one for git add, I may try to do it. However, git add does not even honor the submodules' ignore setting from .gitmodules (just tested with git 1.9.1: "git add -u" doesn't honor it, while "git commit -a" does). So teaching git add the --ignore-submodules switch in current state doesn't seem right to me. You might propose to add also support for the ignore setting, to make "add -u" and "commit -a" more consistent. That seems like a good idea, but the effort needed is getting bigger, >>> >>> Well, now I actually looked at it, and it was pretty easy after all. >>> The changes below seem to enable support for both ignore setting in >>> .gitmodules, and also --ignore-submodules switch, for git add, on top >>> of my patch for commit. >> >> There is a catch. With the changes below, submodules are ignored by add >> even if explitely named on command line (eg. "git add x" does nothing >> if x is submodule with new commits, but with ignore=all in .gitmodules). >> That doesn't seem right. >> >> Any ideas, what to do about that? When exactly should such submodule be >> actually ignored? > > Me thinks git add should require the '-f' option to add an ignored > submodule (just like it does for files) unless the user uses the > '--ignore-submodules=none' option. And if neither of these are given > it should "fail with a list of ignored files" as the documentation > states. It's still not clear, at least not to me. Should '-f' suppress the ignore setting of all involved submodules? That would make it a synonyme (or a superset) of --ignore-submodules=none. Or only if the submodule is explicitly named on command line? That seems fuzzy to me, and also more tricky to implement. >>> >>> Maybe my impression that doing "add" together with "commit" would be >>> easy wasn't correct after all. I won't object if you try to tackle >>> commit first (but I have the slight suspicion that similar questions >>> will arise concerning the "add"ish functionality in commit too. So >>> maybe after resolving those things might look clearer ;-) >> >> There is one big distinction. My patch for commit doesn't add any new >> problems. It just adds the --ignore-submodules argument, which is easy >> to implement and no unclear behavior decisions are needed. >> >> You are right that when specifying ignored submodules on commit's >> command line, there is the same problem as with git add. However, it's >> already there anyway. I don't feel in position to solve it, I'd just >> like to have "git commit --ignore-submodules=none". >> >> With git add however, changing it to honor settings from .gitmodules >> would change behavior people might be used to, so I would be afraid to >> do that. Btw add also has the problem already, but only if somebody >> configures the submodule's ignore setting in .git/config, rather than >> .gitmodules. I don't know how much real use case that is. >> >> As I see it, there are now these rather easy possibilities (sorted >> from the easiest): >> >> 1) Just teach commit the --ignore-submodules argument, as I proposed. > > 1a) Teach commit to honor ignore from .git/config. But commit already honors that. It honors submodule..ignore from both .git/config and .gitmodules. It's just add which doesn't honor it from .gitmodules, because cmd_add() function lacks a gitmodules_config() call. Or do I miss something? >> 2) Teach both add and commit to --ignore-submodules, but dont add that >> problematic gitmodules_config() in add.c. > > Why is that problematic after add learned --ignore-submodules=none? First, because it changes current behaviour. Which is obviously inconsistent currently, however I didn't find it easy to tell what's the right thing to do. And second, because the "-f implies --ignore-submodules=none" proposal, which seems to be the easy cure for those accustomed to the current behavior, seems non-trivial. Below You wrote that --ignore-submodules=none should be implied by -f only for files specified on the command line. OK.
Re: [PATCH v10 12/12] trailer: add blank line before the trailers if needed
Christian Couder writes: > Signed-off-by: Christian Couder > --- Hmph, this is more fixing a mistake made earlier in the series at the end than adding a new feature or something. Can you start from a version that does not have the mistake from the beginning? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: notes.rewriteRef doesn't apply to rebases that skip the commit
Kevin Ballard writes: > I’ve started using notes recently, and I have notes.rewriteRef set so that > when I rebase, my notes will be kept. Unfortunately, it turns out that if a > rebase deletes my local commit because it already exists in upstream, it > doesn’t copy the note to the upstream commit. It seems perfectly reasonable to > me to expect the note to be copied to the upstream commit, as it represents > the same change. That would cut both ways, depending on the use case. I suspect that those who use notes as remainder of what are still to be sent out would appreciate the current behaviour. > One complication I can see is when my local commit is deleted not because it > exists upstream, but because it ends up being an empty commit due to the > changes existing across multiple upstream commits. In this case I see no > alternative but to have the note disappear. But I think that's acceptable. Oh, no question about that. > Another potential issues is if the commit exists upstream, but the surrounding > context has changed enough that it contains a different patch-id. In this > case, I would want Git to take the extra effort to correlate the upstream > commit with my local one (it has the same message, modulo any Signed-Off-By > lines, the same authorship info, and all the - and + lines in the diff are > identical). That would be an orthogonal improvement, I would think. Such a smarter "patch-id may mistake it, but it is a moral equivalent" detection would not only be useful for copying notes, but also for skipping the commit from getting replayed in the first place, no? > On a semi-related note, I don't see why Git should be warning about > notes.displayRef evaluating to a reference that doesn't exist. It doesn't > exist because I haven't created any notes for that ref in this repository yet. > But that doesn't mean I won't be creating them eventually, and when I do I > want them to be displayed. That also cuts both ways. I think a warning is primarily to let those who mistyped the refname take notice. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
notes.rewriteRef doesn't apply to rebases that skip the commit
I’ve started using notes recently, and I have notes.rewriteRef set so that when I rebase, my notes will be kept. Unfortunately, it turns out that if a rebase deletes my local commit because it already exists in upstream, it doesn’t copy the note to the upstream commit. It seems perfectly reasonable to me to expect the note to be copied to the upstream commit, as it represents the same change. One complication I can see is when my local commit is deleted not because it exists upstream, but because it ends up being an empty commit due to the changes existing across multiple upstream commits. In this case I see no alternative but to have the note disappear. But I think that's acceptable. Another potential issues is if the commit exists upstream, but the surrounding context has changed enough that it contains a different patch-id. In this case, I would want Git to take the extra effort to correlate the upstream commit with my local one (it has the same message, modulo any Signed-Off-By lines, the same authorship info, and all the - and + lines in the diff are identical). That said, I'd still understand if it didn't do that and lost my note. It would be unfortunate, but it would match today's behavior. I'm ok with copying over my notes when necessary, I just want Git to handle it when it's obviously correct (e.g. when the patch-id matches). --- On a semi-related note, I don't see why Git should be warning about notes.displayRef evaluating to a reference that doesn't exist. It doesn't exist because I haven't created any notes for that ref in this repository yet. But that doesn't mean I won't be creating them eventually, and when I do I want them to be displayed. For reference, I've been using git v1.9.0. The v1.9.1 release notes don't mention anything notes-related so I assume these issues still exist. -Kevin Ballard -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 18/19] tree-diff: rework diff_tree() to generate diffs for multiparent cases as well
On Mon, Apr 07, 2014 at 10:29:46AM -0700, Junio C Hamano wrote:
> Kirill Smelkov writes:
>
> > The following
> > ...
> > maybe looks a bit simpler, but calls tree_entry_pathcmp twice more times.
> >
> > Besides for important nparent=1 case we were not calling
> > tree_entry_pathcmp at all and here we'll call it once, which would slow
> > execution down a bit, as base_name_compare shows measurable enough in
> > profile.
> > To avoid that we'll need to add 'if (i==imin) continue' and this won't
> > be so simple then. And for general nparent case, as I've said, we'll be
> > calling tree_entry_pathcmp twice more times...
> >
> > Because of all that I'd suggest to go with my original version.
>
> OK.
Thanks.
> > ... After some break on the topic,
> > with a fresh eye I see a lot of confusion goes from the notation I've
> > chosen initially (because of how I was reasoning about it on paper, when
> > it was in flux) - i.e. xi for x[imin] and also using i as looping
> > variable. And also because xi was already used for x[imin] I've used
> > another letter 'k' denoting all other x'es, which leads to confusion...
> >
> >
> > I propose we do the following renaming to clarify things:
> >
> > A/a -> T/t (to match resulting tree t name in the code)
> > X/x -> P/p (to match parents trees tp in the code)
> > i -> imin(so that i would be free for other tasks)
> >
> > then the above (with a prologue) would look like
> >
> > 8<
> > * T P1 Pn
> > * - --
> > * |t| |p1| |pn|
> > * |-| |--| ... |--| imin = argmin(p1...pn)
> > * | | | | | |
> > * |-| |--| |--|
> > * |.| |. | |. |
> > * . ..
> > * . ..
> > *
> > * at any time there could be 3 cases:
> > *
> > * 1) t < p[imin];
> > * 2) t > p[imin];
> > * 3) t = p[imin].
> > *
> > * Schematic deduction of what every case means, and what to do, follows:
> > *
> > * 1) t < p[imin] -> ∀j t ∉ Pj -> "+t" ∈ D(T,Pj) -> D += "+t"; t↓
> > *
> > * 2) t > p[imin]
> > *
> > * 2.1) ∃j: pj > p[imin] -> "-p[imin]" ∉ D(T,Pj) -> D += ø; ∀
> > pi=p[imin] pi↓
> > * 2.2) ∀i pi = p[imin] -> pi ∉ T -> "-pi" ∈ D(T,Pi) -> D +=
> > "-p[imin]"; ∀i pi↓
> > *
> > * 3) t = p[imin]
> > *
> > * 3.1) ∃j: pj > p[imin] -> "+t" ∈ D(T,Pj) -> only pi=p[imin]
> > remains to investigate
> > * 3.2) pi = p[imin] -> investigate δ(t,pi)
> > * |
> > * |
> > * v
> > *
> > * 3.1+3.2) looking at δ(t,pi) ∀i: pi=p[imin] - if all != ø ->
> > *
> > * ⎧δ(t,pi) - if pi=p[imin]
> > * -> D += ⎨
> > * ⎩"+t" - if pi>p[imin]
> > *
> > *
> > * in any case t↓ ∀ pi=p[imin] pi↓
> > ...
> > now xk is gone and i matches p[i] (= pi) etc so variable names correlate
> > to algorithm description better.
> >
> > Does that maybe clarify things?
>
> That sounds more consistent (modulo perhaps s/argmin/min/ at the
> beginning?).
Thanks. argmin is there on purpose - min(p1...pn) is the minimal p, and
argmin(p1...pn) is imin such that p[imin] is minimal. As we are finding
the index of the minimal element we should use argmin.
> > P.S. Sorry for maybe some crept-in mistakes - I've tried to verify it
> > thoroughly, but am too sleepy to be completely sure. On the other hand I
> > think and hope the patch should be ok.
>
> Thanks and do not be sorry for "mistakes"---we have the review
> process exactly for catching them.
Thanks, I appreciate that.
On Mon, Apr 07, 2014 at 11:07:47AM -0700, Junio C Hamano wrote:
> Kirill Smelkov writes:
>
> >> > +if (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER)) {
> >> > +for (i = 0; i < nparent; ++i)
> >> > +if (tp[i].entry.mode &
> >> > S_IFXMIN_NEQ)
> >> > +goto skip_emit_tp;
> >> > +}
> >> > +
> >> > +p = emit_path(p, base, opt, nparent,
> >> > +/*t=*/NULL, tp, imin);
> >> > +
> >> > +skip_emit_tp:
> >> > +/* ∀ xk=ximin xk↓ */
> >> > +update_tp_entries(tp, nparent);
> >>
> >> There are parents whose path sort earlier than what is in 't'
> >> (i.e. they were lost in the result---we would want to show
> >> removal). What makes us jump to the skip label?
> >>
> >> We are looking at path in 't', and some parents have paths that
> >> sort earlier than that path. We will not go to skip label if
> >> any one of the parent's entry sorts after some other parent (or
> >> the parent in question has ran out its entries), which means we
> >> show the entry from the parents only when all the parents have
> >> that same path,
Re: [PATCH] Unicode: update of combining code points
Hi, Torsten Bögershausen wrote: > Unicode 6.3 defines the following code as combining or accents, > git_wcwidth() should return 0. > > Earlier unicode standards had defined these code point as "reserved": Thanks for the update. Could the commit message also explain how this was noticed and what the user-visible effect is? For example: "Unicode just announced that <...>. That means we should mark the relevant code points as combining characters so git knows they are zero-width and doesn't screw up the alignment when presenting branch names in columns with 'git branch --column'" or something like that. [...] > 358 COMBINING DOT ABOVE RIGHT > 359 COMBINING ASTERISK BELOW I'm not sure this list is needed --- the code + the reference to the Unicode 6.3 standard seems like enough (but if you think otherwise, I don't really mind). > This commit touches only the range 300-6FF, there may be more to be updated. The "there may be more" here sounds ominous. Does that mean Unicode 6.3 also added some zero-width characters in other ranges that should be dealt with in the future? How many such ranges? How do we know when we're done? Just biting off the most important characters first and putting off the rest for later sounds fine to me --- my complaint is that the above comment doesn't make clear what the to-do list is for finishing the update later. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2014, #01; Fri, 4)
On 07/04/14 19:35, Johannes Sixt wrote: > Am 05.04.2014 11:19, schrieb Johannes Sixt: >> Am 04.04.2014 22:58, schrieb Junio C Hamano: >>> * sz/mingw-index-pack-threaded (2014-03-19) 1 commit >>> - Enable index-pack threading in msysgit. >>> >>> What is the status of this topic? A failure report exists >>> ($gmane/245170), and I am aware of Duy's $gmane/245034 but that was >>> where the discussion stalled. Is everybody waiting for everybody >>> else to get the discussion unstuck? >> >> I still have to cross-check Duy's patch. I'll hopefully get to it in the >> next days and report back. > > The test suite passes with Duy's patch ($gmane/245034), but t5302 fails > with this patch with a MinGW build. The patches touch the Cygwin > configuration, but I cannot test a Cygwin build. I haven't tested these on cygwin yet. However, only the old version of cygwin is affected (newer cygwin has a thread-safe pread) and, since I no longer have an old installation, I _can't_ test it anyway. :( (I updated cygwin earlier today and received a brand new cygwin dll with today's date!). > I have, however, lost track of what the objective of these patches is. > Is the threaded version significantly faster, and these patches are > worth it? Indeed. I haven't seen any numbers. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/25] Lockfile correctness and refactoring
On Mon, Apr 07, 2014 at 01:33:42AM +0200, Michael Haggerty wrote: > This is a second attempt at renovating the lock file code. Thanks to > Peff, Junio, Torsten, and Eric for their helpful reviews of v1. > > v1 of this patch series [1] did some refactoring and then added a new > feature to the lock_file API: the ability to activate a new version of > a locked file while retaining the lock. > > But the review of v1 turned up even more correctness issues in the > existing implementation of lock files. So this v2 dials back the > scope of the changes (it omits the new feature) but does more work to > fix problems with the current lock file implementation. > > The main theme of this patch series is to better define the state > diagram for lock_file objects and to fix code that left them in > incorrect, indeterminate, or unexpected states. There are also a few > patches that convert several functions to use strbufs instead of > limiting pathnames to a maximum length. Looks OK to me, modulo the few comments I sent. I still think resolve_symref should probably not be "best-effort", but that can come later on top. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Unicode: update of combining code points
Unicode 6.3 defines the following code as combining or accents,
git_wcwidth() should return 0.
Earlier unicode standards had defined these code point as "reserved":
358 COMBINING DOT ABOVE RIGHT
359 COMBINING ASTERISK BELOW
35A COMBINING DOUBLE RING BELOW
35B COMBINING ZIGZAG ABOVE
35C COMBINING DOUBLE BREVE BELOW
487 COMBINING CYRILLIC POKRYTIE
5A2 HEBREW ACCENT ATNAH HAFUKH,
5BA HEBREW POINT HOLAM HASER FOR VAV
5C5 HEBREW MARK LOWER DOT
5C7 HEBREW POINT QAMATS QATAN
604 ARABIC SIGN SAMVAT
616 ARABIC SMALL HIGH LIGATURE ALEF WITH LAM WITH YEH
617 ARABIC SMALL HIGH ZAIN
618 ARABIC SMALL FATHA
619 ARABIC SMALL DAMMA
61A ARABIC SMALL KASRA
659 ARABIC ZWARAKAY
65A ARABIC VOWEL SIGN SMALL V ABOVE
65B ARABIC VOWEL SIGN INVERTED SMALL V ABOVE
65C ARABIC VOWEL SIGN DOT BELOW
65D ARABIC REVERSED DAMMA
65E ARABIC FATHA WITH TWO DOTS
65F ARABIC WAVY HAMZA BELOW
This commit touches only the range 300-6FF, there may be more to be updated.
Signed-off-by: Torsten Bögershausen
---
utf8.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/utf8.c b/utf8.c
index a831d50..77c28d4 100644
--- a/utf8.c
+++ b/utf8.c
@@ -84,11 +84,10 @@ static int git_wcwidth(ucs_char_t ch)
* "uniset +cat=Me +cat=Mn +cat=Cf -00AD +1160-11FF +200B c".
*/
static const struct interval combining[] = {
- { 0x0300, 0x0357 }, { 0x035D, 0x036F }, { 0x0483, 0x0486 },
- { 0x0488, 0x0489 }, { 0x0591, 0x05A1 }, { 0x05A3, 0x05B9 },
- { 0x05BB, 0x05BD }, { 0x05BF, 0x05BF }, { 0x05C1, 0x05C2 },
- { 0x05C4, 0x05C4 }, { 0x0600, 0x0603 }, { 0x0610, 0x0615 },
- { 0x064B, 0x0658 }, { 0x0670, 0x0670 }, { 0x06D6, 0x06E4 },
+ { 0x0300, 0x036F }, { 0x0483, 0x0489 }, { 0x0591, 0x05BD },
+ { 0x05BF, 0x05BF }, { 0x05C1, 0x05C2 }, { 0x05C4, 0x05C5 },
+ { 0x05C7, 0x05C7 }, { 0x0600, 0x0604 }, { 0x0610, 0x061A },
+ { 0x064B, 0x065F }, { 0x0670, 0x0670 }, { 0x06D6, 0x06E4 },
{ 0x06E7, 0x06E8 }, { 0x06EA, 0x06ED }, { 0x070F, 0x070F },
{ 0x0711, 0x0711 }, { 0x0730, 0x074A }, { 0x07A6, 0x07B0 },
{ 0x0901, 0x0902 }, { 0x093C, 0x093C }, { 0x0941, 0x0948 },
--
1.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Unicode: update of combining code points
Unicode 6.3 defines the following code as combining or accents,
git_wcwidth() should return 0.
Earlier unicode standards had defined these code point as "reserved":
358 COMBINING DOT ABOVE RIGHT
359 COMBINING ASTERISK BELOW
35A COMBINING DOUBLE RING BELOW
35B COMBINING ZIGZAG ABOVE
35C COMBINING DOUBLE BREVE BELOW
487 COMBINING CYRILLIC POKRYTIE
5A2 HEBREW ACCENT ATNAH HAFUKH,
5BA HEBREW POINT HOLAM HASER FOR VAV
5C5 HEBREW MARK LOWER DOT
5C7 HEBREW POINT QAMATS QATAN
604 ARABIC SIGN SAMVAT
616 ARABIC SMALL HIGH LIGATURE ALEF WITH LAM WITH YEH
617 ARABIC SMALL HIGH ZAIN
618 ARABIC SMALL FATHA
619 ARABIC SMALL DAMMA
61A ARABIC SMALL KASRA
659 ARABIC ZWARAKAY
65A ARABIC VOWEL SIGN SMALL V ABOVE
65B ARABIC VOWEL SIGN INVERTED SMALL V ABOVE
65C ARABIC VOWEL SIGN DOT BELOW
65D ARABIC REVERSED DAMMA
65E ARABIC FATHA WITH TWO DOTS
65F ARABIC WAVY HAMZA BELOW
This commit touches only the range 300-6FF, there may be more to be updated.
Signed-off-by: Torsten Bögershausen
---
utf8.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/utf8.c b/utf8.c
index a831d50..77c28d4 100644
--- a/utf8.c
+++ b/utf8.c
@@ -84,11 +84,10 @@ static int git_wcwidth(ucs_char_t ch)
* "uniset +cat=Me +cat=Mn +cat=Cf -00AD +1160-11FF +200B c".
*/
static const struct interval combining[] = {
- { 0x0300, 0x0357 }, { 0x035D, 0x036F }, { 0x0483, 0x0486 },
- { 0x0488, 0x0489 }, { 0x0591, 0x05A1 }, { 0x05A3, 0x05B9 },
- { 0x05BB, 0x05BD }, { 0x05BF, 0x05BF }, { 0x05C1, 0x05C2 },
- { 0x05C4, 0x05C4 }, { 0x0600, 0x0603 }, { 0x0610, 0x0615 },
- { 0x064B, 0x0658 }, { 0x0670, 0x0670 }, { 0x06D6, 0x06E4 },
+ { 0x0300, 0x036F }, { 0x0483, 0x0489 }, { 0x0591, 0x05BD },
+ { 0x05BF, 0x05BF }, { 0x05C1, 0x05C2 }, { 0x05C4, 0x05C5 },
+ { 0x05C7, 0x05C7 }, { 0x0600, 0x0604 }, { 0x0610, 0x061A },
+ { 0x064B, 0x065F }, { 0x0670, 0x0670 }, { 0x06D6, 0x06E4 },
{ 0x06E7, 0x06E8 }, { 0x06EA, 0x06ED }, { 0x070F, 0x070F },
{ 0x0711, 0x0711 }, { 0x0730, 0x074A }, { 0x07A6, 0x07B0 },
{ 0x0901, 0x0902 }, { 0x093C, 0x093C }, { 0x0941, 0x0948 },
--
1.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Unicode: update of combining code points
Unicode 6.3 defines the following code as combining or accents,
git_wcwidth() should return 0.
Earlier unicode standards had defined these code point as "reserved":
358 COMBINING DOT ABOVE RIGHT
359 COMBINING ASTERISK BELOW
35A COMBINING DOUBLE RING BELOW
35B COMBINING ZIGZAG ABOVE
35C COMBINING DOUBLE BREVE BELOW
487 COMBINING CYRILLIC POKRYTIE
5A2 HEBREW ACCENT ATNAH HAFUKH,
5BA HEBREW POINT HOLAM HASER FOR VAV
5C5 HEBREW MARK LOWER DOT
5C7 HEBREW POINT QAMATS QATAN
604 ARABIC SIGN SAMVAT
616 ARABIC SMALL HIGH LIGATURE ALEF WITH LAM WITH YEH
617 ARABIC SMALL HIGH ZAIN
618 ARABIC SMALL FATHA
619 ARABIC SMALL DAMMA
61A ARABIC SMALL KASRA
659 ARABIC ZWARAKAY
65A ARABIC VOWEL SIGN SMALL V ABOVE
65B ARABIC VOWEL SIGN INVERTED SMALL V ABOVE
65C ARABIC VOWEL SIGN DOT BELOW
65D ARABIC REVERSED DAMMA
65E ARABIC FATHA WITH TWO DOTS
65F ARABIC WAVY HAMZA BELOW
This commit touches only the range 300-6FF, there may be more to be updated.
Signed-off-by: Torsten Bögershausen
---
utf8.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/utf8.c b/utf8.c
index a831d50..77c28d4 100644
--- a/utf8.c
+++ b/utf8.c
@@ -84,11 +84,10 @@ static int git_wcwidth(ucs_char_t ch)
* "uniset +cat=Me +cat=Mn +cat=Cf -00AD +1160-11FF +200B c".
*/
static const struct interval combining[] = {
- { 0x0300, 0x0357 }, { 0x035D, 0x036F }, { 0x0483, 0x0486 },
- { 0x0488, 0x0489 }, { 0x0591, 0x05A1 }, { 0x05A3, 0x05B9 },
- { 0x05BB, 0x05BD }, { 0x05BF, 0x05BF }, { 0x05C1, 0x05C2 },
- { 0x05C4, 0x05C4 }, { 0x0600, 0x0603 }, { 0x0610, 0x0615 },
- { 0x064B, 0x0658 }, { 0x0670, 0x0670 }, { 0x06D6, 0x06E4 },
+ { 0x0300, 0x036F }, { 0x0483, 0x0489 }, { 0x0591, 0x05BD },
+ { 0x05BF, 0x05BF }, { 0x05C1, 0x05C2 }, { 0x05C4, 0x05C5 },
+ { 0x05C7, 0x05C7 }, { 0x0600, 0x0604 }, { 0x0610, 0x061A },
+ { 0x064B, 0x065F }, { 0x0670, 0x0670 }, { 0x06D6, 0x06E4 },
{ 0x06E7, 0x06E8 }, { 0x06EA, 0x06ED }, { 0x070F, 0x070F },
{ 0x0711, 0x0711 }, { 0x0730, 0x074A }, { 0x07A6, 0x07B0 },
{ 0x0901, 0x0902 }, { 0x093C, 0x093C }, { 0x0941, 0x0948 },
--
1.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Unicode: update of combining code points
Unicode 6.3 defines the following code as combining or accents,
git_wcwidth() should return 0.
Earlier unicode standards had defined these code point as "reserved":
358 COMBINING DOT ABOVE RIGHT
359 COMBINING ASTERISK BELOW
35A COMBINING DOUBLE RING BELOW
35B COMBINING ZIGZAG ABOVE
35C COMBINING DOUBLE BREVE BELOW
487 COMBINING CYRILLIC POKRYTIE
5A2 HEBREW ACCENT ATNAH HAFUKH,
5BA HEBREW POINT HOLAM HASER FOR VAV
5C5 HEBREW MARK LOWER DOT
5C7 HEBREW POINT QAMATS QATAN
604 ARABIC SIGN SAMVAT
616 ARABIC SMALL HIGH LIGATURE ALEF WITH LAM WITH YEH
617 ARABIC SMALL HIGH ZAIN
618 ARABIC SMALL FATHA
619 ARABIC SMALL DAMMA
61A ARABIC SMALL KASRA
659 ARABIC ZWARAKAY
65A ARABIC VOWEL SIGN SMALL V ABOVE
65B ARABIC VOWEL SIGN INVERTED SMALL V ABOVE
65C ARABIC VOWEL SIGN DOT BELOW
65D ARABIC REVERSED DAMMA
65E ARABIC FATHA WITH TWO DOTS
65F ARABIC WAVY HAMZA BELOW
This commit touches only the range 300-6FF, there may be more to be updated.
Signed-off-by: Torsten Bögershausen
---
utf8.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/utf8.c b/utf8.c
index a831d50..77c28d4 100644
--- a/utf8.c
+++ b/utf8.c
@@ -84,11 +84,10 @@ static int git_wcwidth(ucs_char_t ch)
* "uniset +cat=Me +cat=Mn +cat=Cf -00AD +1160-11FF +200B c".
*/
static const struct interval combining[] = {
- { 0x0300, 0x0357 }, { 0x035D, 0x036F }, { 0x0483, 0x0486 },
- { 0x0488, 0x0489 }, { 0x0591, 0x05A1 }, { 0x05A3, 0x05B9 },
- { 0x05BB, 0x05BD }, { 0x05BF, 0x05BF }, { 0x05C1, 0x05C2 },
- { 0x05C4, 0x05C4 }, { 0x0600, 0x0603 }, { 0x0610, 0x0615 },
- { 0x064B, 0x0658 }, { 0x0670, 0x0670 }, { 0x06D6, 0x06E4 },
+ { 0x0300, 0x036F }, { 0x0483, 0x0489 }, { 0x0591, 0x05BD },
+ { 0x05BF, 0x05BF }, { 0x05C1, 0x05C2 }, { 0x05C4, 0x05C5 },
+ { 0x05C7, 0x05C7 }, { 0x0600, 0x0604 }, { 0x0610, 0x061A },
+ { 0x064B, 0x065F }, { 0x0670, 0x0670 }, { 0x06D6, 0x06E4 },
{ 0x06E7, 0x06E8 }, { 0x06EA, 0x06ED }, { 0x070F, 0x070F },
{ 0x0711, 0x0711 }, { 0x0730, 0x074A }, { 0x07A6, 0x07B0 },
{ 0x0901, 0x0902 }, { 0x093C, 0x093C }, { 0x0941, 0x0948 },
--
1.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 18/25] lockfile: avoid transitory invalid states
On Mon, Apr 07, 2014 at 03:12:49PM +0200, Michael Haggerty wrote: > > How far *do* you want to go? I'm certainly not opposed to field-test your > > current changeset (plus and adjustment to use sig_atomic_t) -- overall it > > is an improvement. And then we will see how it works. > > For now I think I'd just like to get the biggest problems fixed without > making anything worse. Given that there might be a GSoC student working > in this neighborhood, he/she might be able to take up the baton. > > I changed the patch series to use a new "volatile sig_atomic_t active" > field rather than a bit in a "flags" field. That seems like a good place to stop for now. If any code touches the fields, it can unset the "active" flag (even temporarily), and restore it when the structure is in a known state. I'm not sure if we can ever reach full safety. If you have an event loop, the sane thing to do is set an atomic flag in your signal handler and then handle it on the next iteration of the loop. But all of our signal handlers are jumped to from arbitrary code, and are just going to die(). There's nothing to return to, so any useful work we do has to be done from the handler. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'
Christian Couder writes: >> I do not see these two as valid arguments to make the command line >> more complex to the end users > > I don't think that it makes the command more complex to the end users. > >> ---who now need to know that only this >> command treats its command line in a funny way, accepting a colon in >> place of an equal sign. I meant that it makes learning the "command line syntax of Git" more complex to new users. If one is too narrowly focused on this single command, it may not seem that "the command" is not made more complex, but I am more interested in making sure that the entire Git command line experience does not get unnecessarily harder to learn. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 17/25] commit_lock_file(): make committing an unlocked lockfile a NOP
On Mon, Apr 07, 2014 at 01:33:59AM +0200, Michael Haggerty wrote:
> It was previously a bug to call commit_lock_file() with a lock_file
> object that was not active (an illegal access would happen within the
> function). It was presumably never done, but this would be an easy
> programming error to overlook. So guard the file-renaming code with
> an if statement to change committing an unlocked file into a NOP.
>
> Signed-off-by: Michael Haggerty
> ---
> Alternatively, we could make it a fatal error (e.g., an assert() or
> if...die()) to call commit_lock_file() on an unlocked file, or omit a
> warning in this case. But since it is so hard to test code related to
> locking failures, I have the feeling that such an error is most likely
> to occur in some error-handling path, maybe when some other lockfile
> acquisition has failed, and it would be better to let the code
> continue its attempted cleanup instead of dying. But it would be easy
> to persuade me to change my opinion.
Yeah, I would have expected a die("BUG") here.
I think it is worth making it a fatal mistake and catching it. Rolling
back an uninitialized lockfile is probably OK; we are canceling an
operation that never started. But committing a lockfile that we didn't
actually fill out could be a sign of a serious error, and we may be
propagating a bogus success code. E.g., imagine that receive-pack claims
to have written your ref, but actually commit_lock_file was a silent
NOP. I'd much rather have it die loudly so we can track down the case.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 19/27] refs: add a concept of a reference transaction
Michael Haggerty writes:
> +void ref_transaction_create(struct ref_transaction *transaction,
> + const char *refname,
> + unsigned char *new_sha1,
> + int flags)
> +{
> + struct ref_update *update = add_update(transaction, refname);
> +
> + assert(!is_null_sha1(new_sha1));
> + hashcpy(update->new_sha1, new_sha1);
> + hashclr(update->old_sha1);
> + update->flags = flags;
> + update->have_old = 1;
> +}
> +
> +void ref_transaction_delete(struct ref_transaction *transaction,
> + const char *refname,
> + unsigned char *old_sha1,
> + int flags, int have_old)
> +{
> + struct ref_update *update = add_update(transaction, refname);
> +
> + update->flags = flags;
> + update->have_old = have_old;
> + if (have_old) {
> + assert(!is_null_sha1(old_sha1));
> + hashcpy(update->old_sha1, old_sha1);
> + }
> +}
These assert()s will often turn into no-op in production builds. If
it is a bug in the code (i.e. the callers are responsible for
catching these conditions and issuing errors, and there are actually
such checks implemented in the callers), it is fine to have these as
assert()s, but otherwise these should be 'if (...) die("BUG:")', I
think.
Other than that, I did not spot anything questionable in this round.
Thanks; will replace the series (but on the same base as I needed to
apply the series there to compare what got changed with the old
version of corresponding change for each patches).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] git submodule split
Am 06.04.2014 23:18, schrieb Michal Sojka: > On Sun, Apr 06 2014, Jens Lehmann wrote: >> Am 02.04.2014 23:52, schrieb Michal Sojka: >>> Hello, >>> >>> I needed to convert a subdirectory of a repo to a submodule and have the >>> histories of both repos linked together. I found that this was discussed >>> few years back [1], but the code seemed quite complicated and was not >>> merged. >>> >>> [1]: >>> http://git.661346.n2.nabble.com/RFC-What-s-the-best-UI-for-git-submodule-split-tp2318127.html >>> >>> Now, the situation is better, because git subtree can already do most of >>> the work. Below is a script that I used to split a submodule from my >>> repo. It basically consist of a call to 'git subtree split' followed by >>> 'git filter-branch' to link the histories together. >>> >>> I'd like to get some initial feedback on it before attempting to >>> integrate it with git sources (i.e. writing tests and doc). What do you >>> think? >> >> Why do want to rewrite the whole history of the superproject, >> wouldn't it suffice to turn a directory into a submodule with >> the same content in a simple commit? > > I wanted to publish a project including its history but a part of that > project could not be made public due to legal reasons. Putting that part > into submodule seemed like best idea. Yep, that makes lots of sense. I'm not sure yet if this functionality is needed often enough to put the script under contrib, but I won't object as long as you'd be willing to maintain it (and help people on this list when they report any issues). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'
From: Junio C Hamano > > Christian Couder writes: > >> First accepting both ':' and '=' means one can see the "git >> interpret-trailers" as acting on trailers only. Not just on trailers >> from the intput message and option parameters from the command line. > > Sorry, you lost me. What does "acting on trailers only" really > mean? It means that the command can seen as processing only trailers, (from stdin and from its arguments), sorry if I used the wrong verb. > Do you mean the command should/can be run without any command > line options, pick up the existing "Signed-off-by:" and friends in > its input and emit its output, somehow taking these existing ones as > its instruction regarding how to transform the input to its output? > >> And second there is also a practical advantage, as the user can >> copy-paste trailers directly from other messages into the command line >> to pass them as arguments to "git interpret-trailers" without the need >> to replace the ':' with '='. Even if this command is not often used >> directly by users, it might simplify scripts using it. >> >> Third there is a technical advantage which is that the code that >> parses arguments from the command line can be the same as the code >> that parses trailers from the input message. > > I do not see these two as valid arguments to make the command line > more complex to the end users I don't think that it makes the command more complex to the end users. > ---who now need to know that only this > command treats its command line in a funny way, accepting a colon in > place of an equal sign. It accepts both. So if they think that it is like a regular command, which uses '=' for (key, value) pairs, it will work, and if they think it works on trailers, which use ':' for (key, value) pairs, it will also work. > A different way to sell a colon, e.g. > > Consider the instruction sed takes on its command line. > (e.g. "sed 's/frotz/nitfol/' form, you would always give it as the value of an '-e' option > (e.g. "sed -e 's/frotz/nitfol' be loose in limited occassions. "Key:value" is like that, and > in the most general form, it actually needs to be spelled as > "-e 'Key:value'". > > is possible, but I do not think it is a particularly good analogy, > because what you have as the alternative is "Key=value", and not > "-e 'Key:value'", or "--Key=value" (the last would probably be the > most natural way to express this). The analogy that I would use is rather that Perl lets people use 's:foo:bar:' as well as 's=foo=bar=' instead of 's/foo/bar/' if they prefer. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-multimail: update to version 1.0.0
Michael Haggerty writes: > ... > Contributions-by: Raphaël Hertzog > Contributions-by: Eric Berberich > Contributions-by: Michiel Holtkamp > Contributions-by: Malte Swart > Signed-off-by: Michael Haggerty > --- > Junio, how would you like other people's contributions to be recorded > within the Git project? I have listed them above as > "Contributions-by". All of these people have signed off on their > contributions (recorded in my GitHub repo). So should I also/instead > add "Signed-off-by" for those people? Either is fine, as long as somewhere in that directory: - we make it clear that the copy we have in contrib/ is merely for "batteries included" convenience; - we refer to the canonical source that is your repository; - we tell readers to go there to get the authoritative and up to date copy, as what we have in contrib/ is possibly stale. In the longer term, I have a feeling that we may be better off to make the "git core" tree not be the "batteris included" convenience tree, though. In the early days, Linus's rationale for including "gitk" held true: having tools that are not quite core is a good way to get people (especially those without C background) involved in the still-small project in its infancy to help nurture the developer community. The same reasoning stood behind the merging of "gitweb". We already are beyond that stage, and good tools like iMerge and multimail that can stand on its own may be better off flourishing outside "git core" tree, still within the same developer community. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/25] unable_to_lock_die(): rename function from unable_to_lock_index_die()
On Mon, Apr 07, 2014 at 01:33:44AM +0200, Michael Haggerty wrote: > This function is used for other things besides the index, so rename it > accordingly. Oh, and here it is. I should really have just read ahead before responding to patch 1. We can either re-order these first two, or just not worry about it. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/25] api-lockfile: expand the documentation
On Mon, Apr 07, 2014 at 01:33:43AM +0200, Michael Haggerty wrote: > +unable_to_lock_error:: > + > + Emit an error describing that there was an error locking the > + specified path. The err parameter should be the errno of the > + problem that caused the failure. > + > +unable_to_lock_die:: > + > + Like `unable_to_lock_error()`, but also `die()`. The die() function is still called unable_to_lock_index_die() at this point in the series. Presumably you change it later. I don't know if it is worth caring about the order or not; it's a doc change, so it's not like it breaks bisectability. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: The fetch command should "always" honor remote..fetch
On Mon, Apr 7, 2014 at 2:23 PM, Junio C Hamano wrote: > Lewis Diamond writes: > >> 'git fetch foo develop' would result in: >> fatal: Couldn't find remote ref test2 //Not OK, (case 1) > > I have no idea where the "test2" comes from, as it does not appear > anywhere in the above write-up, and it could be a bug. > Sorry, "test2" was a local test to copy the error message. It should read "foo". >> 'git fetch foo master' would result in (FETCH_HEAD omitted): >> [new ref] refs/heads/master -> foo/master //OK, but missing another >> ref! (case 2) >> //It should also fetch refs/users/bob/heads/master -> foo/bob/master > > This is an incorrect expectation. Indeed this is the "correct" behaviour since it works as designed. However, this behaviour is inconsistent with the push command. An expectation is never "incorrect" as we are talking about intuitive vs non-intuitive. > > The user who gave the command line said only "master", and did not > want to grab "users/bob/heads/master". If the user wanted to get it > as well, the command line would have said so, e.g. > > git fetch there master users/bob/heads/master > Actually, the user specifically configured the remote to fetch refs/users/bob/heads/*, meaning "those are the branches I'm interested in". >> If you remove this configuration line: fetch = >> +refs/heads/*:refs/remotes/foo/* >> Then you run 'git fetch foo master', this would result in: >> * branch master -> FETCH_HEAD //Debatable whether this is OK or not, >> but it's definitely missing bob's master! (case 3) > > Likewise. > > The 'master' short-hand is designed not to match refs/users/any/thing. > Sure, this shorthand is designed to match refs listed in the rev parse rules list. However, a quick survey showed me that most people would expect their configuration to be honoured when using the shorthand for fetching (like it is for push). This thread is about improving the fetch command so that the short-hand works in such cases (and make it consistent with what push does). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 07/12] trailer: add interpret-trailers command
This patch adds the "git interpret-trailers" command.
This command uses the previously added process_trailers()
function in trailer.c.
Signed-off-by: Christian Couder
Signed-off-by: Junio C Hamano
---
.gitignore | 1 +
Makefile | 1 +
builtin.h| 1 +
builtin/interpret-trailers.c | 33 +
git.c| 1 +
5 files changed, 37 insertions(+)
create mode 100644 builtin/interpret-trailers.c
diff --git a/.gitignore b/.gitignore
index dc600f9..c2a0b19 100644
--- a/.gitignore
+++ b/.gitignore
@@ -74,6 +74,7 @@
/git-index-pack
/git-init
/git-init-db
+/git-interpret-trailers
/git-instaweb
/git-log
/git-ls-files
diff --git a/Makefile b/Makefile
index 179be0a..499ca30 100644
--- a/Makefile
+++ b/Makefile
@@ -944,6 +944,7 @@ BUILTIN_OBJS += builtin/hash-object.o
BUILTIN_OBJS += builtin/help.o
BUILTIN_OBJS += builtin/index-pack.o
BUILTIN_OBJS += builtin/init-db.o
+BUILTIN_OBJS += builtin/interpret-trailers.o
BUILTIN_OBJS += builtin/log.o
BUILTIN_OBJS += builtin/ls-files.o
BUILTIN_OBJS += builtin/ls-remote.o
diff --git a/builtin.h b/builtin.h
index c47c110..8ca0065 100644
--- a/builtin.h
+++ b/builtin.h
@@ -73,6 +73,7 @@ extern int cmd_hash_object(int argc, const char **argv, const
char *prefix);
extern int cmd_help(int argc, const char **argv, const char *prefix);
extern int cmd_index_pack(int argc, const char **argv, const char *prefix);
extern int cmd_init_db(int argc, const char **argv, const char *prefix);
+extern int cmd_interpret_trailers(int argc, const char **argv, const char
*prefix);
extern int cmd_log(int argc, const char **argv, const char *prefix);
extern int cmd_log_reflog(int argc, const char **argv, const char *prefix);
extern int cmd_ls_files(int argc, const char **argv, const char *prefix);
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
new file mode 100644
index 000..0c8ca72
--- /dev/null
+++ b/builtin/interpret-trailers.c
@@ -0,0 +1,33 @@
+/*
+ * Builtin "git interpret-trailers"
+ *
+ * Copyright (c) 2013, 2014 Christian Couder
+ *
+ */
+
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+#include "trailer.h"
+
+static const char * const git_interpret_trailers_usage[] = {
+ N_("git interpret-trailers [--trim-empty]
[([(=|:)])...]"),
+ NULL
+};
+
+int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
+{
+ int trim_empty = 0;
+
+ struct option options[] = {
+ OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty
trailers")),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_interpret_trailers_usage, 0);
+
+ process_trailers(trim_empty, argc, argv);
+
+ return 0;
+}
diff --git a/git.c b/git.c
index 9efd1a3..d432f11 100644
--- a/git.c
+++ b/git.c
@@ -380,6 +380,7 @@ static struct cmd_struct commands[] = {
{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
{ "init", cmd_init_db },
{ "init-db", cmd_init_db },
+ { "interpret-trailers", cmd_interpret_trailers, RUN_SETUP },
{ "log", cmd_log, RUN_SETUP },
{ "ls-files", cmd_ls_files, RUN_SETUP },
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
--
1.9.0.163.g8ca203c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 02/12] trailer: process trailers from stdin and arguments
Implement the logic to process trailers from stdin and arguments.
At the beginning trailers from stdin are in their own in_tok
doubly linked list, and trailers from arguments are in their own
arg_tok doubly linked list.
The lists are traversed and when an arg_tok should be "applied",
it is removed from its list and inserted into the in_tok list.
Signed-off-by: Christian Couder
Signed-off-by: Junio C Hamano
---
trailer.c | 198 ++
1 file changed, 198 insertions(+)
diff --git a/trailer.c b/trailer.c
index db93a63..52108c2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -47,3 +47,201 @@ static size_t alnum_len(const char *buf, size_t len)
len--;
return len;
}
+
+static void free_trailer_item(struct trailer_item *item)
+{
+ free(item->conf.name);
+ free(item->conf.key);
+ free(item->conf.command);
+ free((char *)item->token);
+ free((char *)item->value);
+ free(item);
+}
+
+static void add_arg_to_input_list(struct trailer_item *in_tok,
+ struct trailer_item *arg_tok)
+{
+ if (arg_tok->conf.where == WHERE_AFTER) {
+ arg_tok->next = in_tok->next;
+ in_tok->next = arg_tok;
+ arg_tok->previous = in_tok;
+ if (arg_tok->next)
+ arg_tok->next->previous = arg_tok;
+ } else {
+ arg_tok->previous = in_tok->previous;
+ in_tok->previous = arg_tok;
+ arg_tok->next = in_tok;
+ if (arg_tok->previous)
+ arg_tok->previous->next = arg_tok;
+ }
+}
+
+static int check_if_different(struct trailer_item *in_tok,
+ struct trailer_item *arg_tok,
+ int alnum_len, int check_all)
+{
+ enum action_where where = arg_tok->conf.where;
+ do {
+ if (!in_tok)
+ return 1;
+ if (same_trailer(in_tok, arg_tok, alnum_len))
+ return 0;
+ /*
+* if we want to add a trailer after another one,
+* we have to check those before this one
+*/
+ in_tok = (where == WHERE_AFTER) ? in_tok->previous :
in_tok->next;
+ } while (check_all);
+ return 1;
+}
+
+static void apply_arg_if_exists(struct trailer_item *in_tok,
+ struct trailer_item *arg_tok,
+ int alnum_len)
+{
+ switch (arg_tok->conf.if_exists) {
+ case EXISTS_DO_NOTHING:
+ free_trailer_item(arg_tok);
+ break;
+ case EXISTS_OVERWRITE:
+ free((char *)in_tok->value);
+ in_tok->value = xstrdup(arg_tok->value);
+ free_trailer_item(arg_tok);
+ break;
+ case EXISTS_ADD:
+ add_arg_to_input_list(in_tok, arg_tok);
+ break;
+ case EXISTS_ADD_IF_DIFFERENT:
+ if (check_if_different(in_tok, arg_tok, alnum_len, 1))
+ add_arg_to_input_list(in_tok, arg_tok);
+ else
+ free_trailer_item(arg_tok);
+ break;
+ case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
+ if (check_if_different(in_tok, arg_tok, alnum_len, 0))
+ add_arg_to_input_list(in_tok, arg_tok);
+ else
+ free_trailer_item(arg_tok);
+ break;
+ }
+}
+
+static void remove_from_list(struct trailer_item *item,
+struct trailer_item **first)
+{
+ if (item->next)
+ item->next->previous = item->previous;
+ if (item->previous)
+ item->previous->next = item->next;
+ else
+ *first = item->next;
+}
+
+static struct trailer_item *remove_first(struct trailer_item **first)
+{
+ struct trailer_item *item = *first;
+ *first = item->next;
+ if (item->next) {
+ item->next->previous = NULL;
+ item->next = NULL;
+ }
+ return item;
+}
+
+static void process_input_token(struct trailer_item *in_tok,
+ struct trailer_item **arg_tok_first,
+ enum action_where where)
+{
+ struct trailer_item *arg_tok;
+ struct trailer_item *next_arg;
+
+ int after = where == WHERE_AFTER;
+ int tok_alnum_len = alnum_len(in_tok->token, strlen(in_tok->token));
+
+ for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+ next_arg = arg_tok->next;
+ if (!same_token(in_tok, arg_tok, tok_alnum_len))
+ continue;
+ if (arg_tok->conf.where != where)
+ continue;
+ remove_from_list(arg_tok, arg_tok_first);
+ apply_arg_if_exists(in_tok, arg_tok, tok_alnum_len)
[PATCH v10 01/12] trailer: add data structures and basic functions
We will use a doubly linked list to store all information
about trailers and their configuration.
This way we can easily remove or add trailers to or from
trailer lists while traversing the lists in either direction.
Signed-off-by: Christian Couder
Signed-off-by: Junio C Hamano
---
Makefile | 1 +
trailer.c | 49 +
2 files changed, 50 insertions(+)
create mode 100644 trailer.c
diff --git a/Makefile b/Makefile
index c5316a3..179be0a 100644
--- a/Makefile
+++ b/Makefile
@@ -879,6 +879,7 @@ LIB_OBJS += submodule.o
LIB_OBJS += symlinks.o
LIB_OBJS += tag.o
LIB_OBJS += trace.o
+LIB_OBJS += trailer.o
LIB_OBJS += transport.o
LIB_OBJS += transport-helper.o
LIB_OBJS += tree-diff.o
diff --git a/trailer.c b/trailer.c
new file mode 100644
index 000..db93a63
--- /dev/null
+++ b/trailer.c
@@ -0,0 +1,49 @@
+#include "cache.h"
+/*
+ * Copyright (c) 2013, 2014 Christian Couder
+ */
+
+enum action_where { WHERE_AFTER, WHERE_BEFORE };
+enum action_if_exists { EXISTS_ADD_IF_DIFFERENT,
EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,
+ EXISTS_ADD, EXISTS_OVERWRITE, EXISTS_DO_NOTHING };
+enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
+
+struct conf_info {
+ char *name;
+ char *key;
+ char *command;
+ enum action_where where;
+ enum action_if_exists if_exists;
+ enum action_if_missing if_missing;
+};
+
+struct trailer_item {
+ struct trailer_item *previous;
+ struct trailer_item *next;
+ const char *token;
+ const char *value;
+ struct conf_info conf;
+};
+
+static int same_token(struct trailer_item *a, struct trailer_item *b, int
alnum_len)
+{
+ return !strncasecmp(a->token, b->token, alnum_len);
+}
+
+static int same_value(struct trailer_item *a, struct trailer_item *b)
+{
+ return !strcasecmp(a->value, b->value);
+}
+
+static int same_trailer(struct trailer_item *a, struct trailer_item *b, int
alnum_len)
+{
+ return same_token(a, b, alnum_len) && same_value(a, b);
+}
+
+/* Get the length of buf from its beginning until its last alphanumeric
character */
+static size_t alnum_len(const char *buf, size_t len)
+{
+ while (len > 0 && !isalnum(buf[len - 1]))
+ len--;
+ return len;
+}
--
1.9.0.163.g8ca203c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 08/12] trailer: add tests for "git interpret-trailers"
Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- t/t7513-interpret-trailers.sh | 351 ++ 1 file changed, 351 insertions(+) create mode 100755 t/t7513-interpret-trailers.sh diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh new file mode 100755 index 000..0e5d57f --- /dev/null +++ b/t/t7513-interpret-trailers.sh @@ -0,0 +1,351 @@ +#!/bin/sh +# +# Copyright (c) 2013 Christian Couder +# + +test_description='git interpret-trailers' + +. ./test-lib.sh + +# When we want one trailing space at the end of each line, let's use sed +# to make sure that these spaces are not removed by any automatic tool. + +test_expect_success 'setup' ' + cat >basic_message <<-\EOF && + subject + + body + EOF + cat >complex_message_body <<-\EOF && + my subject + + my body which is long + and contains some special + chars like : = ? ! + + EOF + sed -e "s/ Z\$/ /" >complex_message_trailers <<-\EOF + Fixes: Z + Acked-by: Z + Reviewed-by: Z + Signed-off-by: Z + EOF +' + +test_expect_success 'without config' ' + sed -e "s/ Z\$/ /" >expected <<-\EOF && + ack: Peff + Reviewed-by: Z + Acked-by: Johan + EOF + git interpret-trailers "ack = Peff" "Reviewed-by" "Acked-by: Johan" >actual && + test_cmp expected actual +' + +test_expect_success '--trim-empty without config' ' + cat >expected <<-\EOF && + ack: Peff + Acked-by: Johan + EOF + git interpret-trailers --trim-empty "ack = Peff" \ + "Reviewed-by" "Acked-by: Johan" "sob:" >actual && + test_cmp expected actual +' + +test_expect_success 'with config setup' ' + git config trailer.ack.key "Acked-by: " && + cat >expected <<-\EOF && + Acked-by: Peff + EOF + git interpret-trailers --trim-empty "ack = Peff" >actual && + test_cmp expected actual && + git interpret-trailers --trim-empty "Acked-by = Peff" >actual && + test_cmp expected actual && + git interpret-trailers --trim-empty "Acked-by :Peff" >actual && + test_cmp expected actual +' + +test_expect_success 'with config setup and = sign' ' + git config trailer.ack.key "Acked-by= " && + cat >expected <<-\EOF && + Acked-by= Peff + EOF + git interpret-trailers --trim-empty "ack = Peff" >actual && + test_cmp expected actual && + git interpret-trailers --trim-empty "Acked-by= Peff" >actual && + test_cmp expected actual && + git interpret-trailers --trim-empty "Acked-by : Peff" >actual && + test_cmp expected actual +' + +test_expect_success 'with config setup and # sign' ' + git config trailer.bug.key "Bug #" && + cat >expected <<-\EOF && + Bug #42 + EOF + git interpret-trailers --trim-empty "bug = 42" >actual && + test_cmp expected actual +' + +test_expect_success 'with commit basic message' ' + git interpret-trailers actual && + test_cmp basic_message actual +' + +test_expect_success 'with commit complex message' ' + cat complex_message_body complex_message_trailers >complex_message && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Fixes: Z + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + EOF + git interpret-trailers actual && + test_cmp expected actual +' + +test_expect_success 'with commit complex message and args' ' + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Fixes: Z + Acked-by= Z + Acked-by= Peff + Reviewed-by: Z + Signed-off-by: Z + Bug #42 + EOF + git interpret-trailers "ack: Peff" "bug: 42" actual && + test_cmp expected actual +' + +test_expect_success 'with commit complex message, args and --trim-empty' ' + cat complex_message_body >expected && + cat >>expected <<-\EOF && + Acked-by= Peff + Bug #42 + EOF + git interpret-trailers --trim-empty "ack: Peff" "bug: 42" \ + actual && + test_cmp expected actual +' + +test_expect_success 'using "where = before"' ' + git config trailer.bug.where "before" && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Bug #42 + Fixes: Z + Acked-by= Z + Acked-by= Peff + Reviewed-by: Z + Signed-off-by: Z + EOF + git interpret-trailers "ack: Peff" "bug: 42" actual && + test_cmp expected actual +' + +test_expect_success 'using "where
[PATCH v10 04/12] trailer: process command line trailer arguments
Parse the trailer command line arguments and put
the result into an arg_tok doubly linked list.
Signed-off-by: Christian Couder
Signed-off-by: Junio C Hamano
---
trailer.c | 117 ++
1 file changed, 117 insertions(+)
diff --git a/trailer.c b/trailer.c
index c7c0f54..89ebff1 100644
--- a/trailer.c
+++ b/trailer.c
@@ -391,3 +391,120 @@ static int git_trailer_config(const char *conf_key, const
char *value, void *cb)
}
return 0;
}
+
+static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char
*trailer)
+{
+ size_t len = strcspn(trailer, "=:");
+ if (len == 0)
+ return error(_("empty trailer token in trailer '%s'"), trailer);
+ if (len < strlen(trailer)) {
+ strbuf_add(tok, trailer, len);
+ strbuf_trim(tok);
+ strbuf_addstr(val, trailer + len + 1);
+ strbuf_trim(val);
+ } else {
+ strbuf_addstr(tok, trailer);
+ strbuf_trim(tok);
+ }
+ return 0;
+}
+
+
+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);
+}
+
+static const char *token_from_item(struct trailer_item *item)
+{
+ if (item->conf.key)
+ return item->conf.key;
+
+ return item->conf.name;
+}
+
+static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
+char *tok, char *val)
+{
+ struct trailer_item *new = xcalloc(sizeof(*new), 1);
+ new->value = val;
+
+ if (conf_item) {
+ duplicate_conf(&new->conf, &conf_item->conf);
+ new->token = xstrdup(token_from_item(conf_item));
+ free(tok);
+ } else
+ new->token = tok;
+
+ return new;
+}
+
+static int token_matches_item(const char *tok, struct trailer_item *item, int
alnum_len)
+{
+ if (!strncasecmp(tok, item->conf.name, alnum_len))
+ return 1;
+ return item->conf.key ? !strncasecmp(tok, item->conf.key, alnum_len) :
0;
+}
+
+static struct trailer_item *create_trailer_item(const char *string)
+{
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ struct trailer_item *item;
+ int tok_alnum_len;
+
+ if (parse_trailer(&tok, &val, string))
+ return NULL;
+
+ tok_alnum_len = alnum_len(tok.buf, tok.len);
+
+ /* Lookup if the token matches something in the config */
+ for (item = first_conf_item; item; item = item->next) {
+ if (token_matches_item(tok.buf, item, tok_alnum_len)) {
+ strbuf_release(&tok);
+ return new_trailer_item(item,
+ NULL,
+ strbuf_detach(&val, NULL));
+ }
+ }
+
+ return new_trailer_item(NULL,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL));
+}
+
+static void add_trailer_item(struct trailer_item **first,
+struct trailer_item **last,
+struct trailer_item *new)
+{
+ if (!new)
+ return;
+ if (!*last) {
+ *first = new;
+ *last = new;
+ } else {
+ (*last)->next = new;
+ new->previous = *last;
+ *last = new;
+ }
+}
+
+static struct trailer_item *process_command_line_args(int argc, const char
**argv)
+{
+ int i;
+ struct trailer_item *arg_tok_first = NULL;
+ struct trailer_item *arg_tok_last = NULL;
+
+ for (i = 0; i < argc; i++) {
+ struct trailer_item *new = create_trailer_item(argv[i]);
+ add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+ }
+
+ return arg_tok_first;
+}
--
1.9.0.163.g8ca203c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 10/12] trailer: add tests for commands in config file
And add a few other tests for some special cases. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- t/t7513-interpret-trailers.sh | 116 ++ 1 file changed, 116 insertions(+) diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh index 0e5d57f..262f7bf 100755 --- a/t/t7513-interpret-trailers.sh +++ b/t/t7513-interpret-trailers.sh @@ -348,4 +348,120 @@ test_expect_success 'using "ifMissing = doNothing"' ' test_cmp expected actual ' +test_expect_success 'with simple command' ' + git config trailer.sign.key "Signed-off-by: " && + git config trailer.sign.where "after" && + git config trailer.sign.ifExists "addIfDifferentNeighbor" && + git config trailer.sign.command "echo \"A U Thor \"" && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Fixes: Z + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + Signed-off-by: A U Thor + EOF + git interpret-trailers "review:" "fix=22" actual && + test_cmp expected actual +' + +test_expect_success 'with command using commiter information' ' + git config trailer.sign.ifExists "addIfDifferent" && + git config trailer.sign.command "echo \"\$GIT_COMMITTER_NAME <\$GIT_COMMITTER_EMAIL>\"" && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Fixes: Z + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + Signed-off-by: C O Mitter + EOF + git interpret-trailers "review:" "fix=22" actual && + test_cmp expected actual +' + +test_expect_success 'with command using author information' ' + git config trailer.sign.key "Signed-off-by: " && + git config trailer.sign.where "after" && + git config trailer.sign.ifExists "addIfDifferentNeighbor" && + git config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Fixes: Z + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + Signed-off-by: A U Thor + EOF + git interpret-trailers "review:" "fix=22" actual && + test_cmp expected actual +' + +test_expect_success 'setup a commit' ' + echo "Content of the first commit." > a.txt && + git add a.txt && + git commit -m "Add file a.txt" +' + +test_expect_success 'with command using $ARG' ' + git config trailer.fix.ifExists "overwrite" && + git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG" && + FIXED=$(git log -1 --oneline --format="%h (%s)" --abbrev-commit --abbrev=14 HEAD) && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-EOF && + Fixes: $FIXED + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + Signed-off-by: A U Thor + EOF + git interpret-trailers "review:" "fix=HEAD" actual && + test_cmp expected actual +' + +test_expect_success 'with failing command using $ARG' ' + git config trailer.fix.ifExists "overwrite" && + git config trailer.fix.command "false \$ARG" && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-EOF && + Fixes: Z + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + Signed-off-by: A U Thor + EOF + git interpret-trailers "review:" "fix=HEAD" actual && + test_cmp expected actual +' + +test_expect_success 'with empty tokens' ' + cat >expected <<-EOF && + Signed-off-by: A U Thor + EOF + git interpret-trailers ":" ":test" >actual <<-EOF && + EOF + test_cmp expected actual +' + +test_expect_success 'with command but no key' ' + git config --unset trailer.sign.key && + cat >expected <<-EOF && + sign: A U Thor + EOF + git interpret-trailers >actual <<-EOF && + EOF + test_cmp expected actual +' + +test_expect_success 'with no command and no key' ' + git config --unset trailer.review.key && + cat >expected <<-EOF && + review: Junio + sign: A U Thor + EOF + git interpret-trailers "review:Junio" >actual <<-EOF && + EOF + test_cmp expected actual +' + test_done -- 1.9.0.163.g8ca203c -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 09/12] trailer: execute command from 'trailer..command'
Let the user specify a command that will give on its standard output
the value to use for the specified trailer.
Signed-off-by: Christian Couder
Signed-off-by: Junio C Hamano
---
trailer.c | 64 +++
1 file changed, 64 insertions(+)
diff --git a/trailer.c b/trailer.c
index 16465e5..09db2c2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1,4 +1,5 @@
#include "cache.h"
+#include "run-command.h"
#include "trailer.h"
/*
* Copyright (c) 2013, 2014 Christian Couder
@@ -13,11 +14,14 @@ struct conf_info {
char *name;
char *key;
char *command;
+ unsigned command_uses_arg : 1;
enum action_where where;
enum action_if_exists if_exists;
enum action_if_missing if_missing;
};
+#define TRAILER_ARG_STRING "$ARG"
+
struct trailer_item {
struct trailer_item *previous;
struct trailer_item *next;
@@ -59,6 +63,13 @@ static inline int contains_only_spaces(const char *str)
return !*s;
}
+static inline void strbuf_replace(struct strbuf *sb, const char *a, const char
*b)
+{
+ const char *ptr = strstr(sb->buf, a);
+ if (ptr)
+ strbuf_splice(sb, ptr - sb->buf, strlen(a), b, strlen(b));
+}
+
static void free_trailer_item(struct trailer_item *item)
{
free(item->conf.name);
@@ -402,6 +413,7 @@ static int git_trailer_config(const char *conf_key, const
char *value, void *cb)
if (conf->command)
warning(_("more than one %s"), conf_key);
conf->command = xstrdup(value);
+ conf->command_uses_arg = !!strstr(conf->command,
TRAILER_ARG_STRING);
break;
case TRAILER_WHERE:
if (set_where(conf, value))
@@ -438,6 +450,45 @@ static int parse_trailer(struct strbuf *tok, struct strbuf
*val, const char *tra
return 0;
}
+static int read_from_command(struct child_process *cp, struct strbuf *buf)
+{
+ if (run_command(cp))
+ return error("running trailer command '%s' failed",
cp->argv[0]);
+ if (strbuf_read(buf, cp->out, 1024) < 1)
+ return error("reading from trailer command '%s' failed",
cp->argv[0]);
+ strbuf_trim(buf);
+ return 0;
+}
+
+static const char *apply_command(const char *command, const char *arg)
+{
+ struct strbuf cmd = STRBUF_INIT;
+ struct strbuf buf = STRBUF_INIT;
+ struct child_process cp;
+ const char *argv[] = {NULL, NULL};
+ const char *result;
+
+ strbuf_addstr(&cmd, command);
+ if (arg)
+ strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
+
+ argv[0] = cmd.buf;
+ memset(&cp, 0, sizeof(cp));
+ cp.argv = argv;
+ cp.env = local_repo_env;
+ cp.no_stdin = 1;
+ cp.out = -1;
+ cp.use_shell = 1;
+
+ if (read_from_command(&cp, &buf)) {
+ strbuf_release(&buf);
+ result = xstrdup("");
+ } else
+ result = strbuf_detach(&buf, NULL);
+
+ strbuf_release(&cmd);
+ return result;
+}
static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
{
@@ -468,6 +519,10 @@ static struct trailer_item *new_trailer_item(struct
trailer_item *conf_item,
duplicate_conf(&new->conf, &conf_item->conf);
new->token = xstrdup(token_from_item(conf_item));
free(tok);
+ if (conf_item->conf.command_uses_arg || !val) {
+ new->value = apply_command(conf_item->conf.command,
val);
+ free(val);
+ }
} else
new->token = tok;
@@ -529,12 +584,21 @@ static struct trailer_item *process_command_line_args(int
argc, const char **arg
int i;
struct trailer_item *arg_tok_first = NULL;
struct trailer_item *arg_tok_last = NULL;
+ struct trailer_item *item;
for (i = 0; i < argc; i++) {
struct trailer_item *new = create_trailer_item(argv[i]);
add_trailer_item(&arg_tok_first, &arg_tok_last, new);
}
+ /* Add conf commands that don't use $ARG */
+ for (item = first_conf_item; item; item = item->next) {
+ if (item->conf.command && !item->conf.command_uses_arg) {
+ struct trailer_item *new = new_trailer_item(item, NULL,
NULL);
+ add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+ }
+ }
+
return arg_tok_first;
}
--
1.9.0.163.g8ca203c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 12/12] trailer: add blank line before the trailers if needed
Signed-off-by: Christian Couder
---
t/t7513-interpret-trailers.sh | 12 +++-
trailer.c | 26 ++
2 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 262f7bf..44a7131 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -34,6 +34,7 @@ test_expect_success 'setup' '
test_expect_success 'without config' '
sed -e "s/ Z\$/ /" >expected <<-\EOF &&
+
ack: Peff
Reviewed-by: Z
Acked-by: Johan
@@ -44,6 +45,7 @@ test_expect_success 'without config' '
test_expect_success '--trim-empty without config' '
cat >expected <<-\EOF &&
+
ack: Peff
Acked-by: Johan
EOF
@@ -55,6 +57,7 @@ test_expect_success '--trim-empty without config' '
test_expect_success 'with config setup' '
git config trailer.ack.key "Acked-by: " &&
cat >expected <<-\EOF &&
+
Acked-by: Peff
EOF
git interpret-trailers --trim-empty "ack = Peff" >actual &&
@@ -68,6 +71,7 @@ test_expect_success 'with config setup' '
test_expect_success 'with config setup and = sign' '
git config trailer.ack.key "Acked-by= " &&
cat >expected <<-\EOF &&
+
Acked-by= Peff
EOF
git interpret-trailers --trim-empty "ack = Peff" >actual &&
@@ -81,6 +85,7 @@ test_expect_success 'with config setup and = sign' '
test_expect_success 'with config setup and # sign' '
git config trailer.bug.key "Bug #" &&
cat >expected <<-\EOF &&
+
Bug #42
EOF
git interpret-trailers --trim-empty "bug = 42" >actual &&
@@ -88,8 +93,10 @@ test_expect_success 'with config setup and # sign' '
'
test_expect_success 'with commit basic message' '
+ cat basic_message >expected &&
+ echo >>expected &&
git interpret-trailers actual &&
- test_cmp basic_message actual
+ test_cmp expected actual
'
test_expect_success 'with commit complex message' '
@@ -436,6 +443,7 @@ test_expect_success 'with failing command using $ARG' '
test_expect_success 'with empty tokens' '
cat >expected <<-EOF &&
+
Signed-off-by: A U Thor
EOF
git interpret-trailers ":" ":test" >actual <<-EOF &&
@@ -446,6 +454,7 @@ test_expect_success 'with empty tokens' '
test_expect_success 'with command but no key' '
git config --unset trailer.sign.key &&
cat >expected <<-EOF &&
+
sign: A U Thor
EOF
git interpret-trailers >actual <<-EOF &&
@@ -456,6 +465,7 @@ test_expect_success 'with command but no key' '
test_expect_success 'with no command and no key' '
git config --unset trailer.review.key &&
cat >expected <<-EOF &&
+
review: Junio
sign: A U Thor
EOF
diff --git a/trailer.c b/trailer.c
index 09db2c2..639f657 100644
--- a/trailer.c
+++ b/trailer.c
@@ -618,12 +618,14 @@ static struct strbuf **read_stdin(void)
}
/*
- * Return the the (0 based) index of the first trailer line
+ * Return the (0 based) index of the first trailer line
* or the line count if there are no trailers.
+ * The has_blank_line parameter tells if there is a blank
+ * line before the trailers.
*/
-static int find_trailer_start(struct strbuf **lines)
+static int find_trailer_start(struct strbuf **lines, int *has_blank_line)
{
- int start, empty = 1, count = 0;
+ int start, only_spaces = 1, count = 0;
/* Get the line count */
while (lines[count])
@@ -635,32 +637,40 @@ static int find_trailer_start(struct strbuf **lines)
*/
for (start = count - 1; start >= 0; start--) {
if (contains_only_spaces(lines[start]->buf)) {
- if (empty)
+ if (only_spaces)
continue;
+ *has_blank_line = 1;
return start + 1;
}
if (strchr(lines[start]->buf, ':')) {
- if (empty)
- empty = 0;
+ if (only_spaces)
+ only_spaces = 0;
continue;
}
+ *has_blank_line = start == count - 1 ?
+ 0 : contains_only_spaces(lines[start + 1]->buf);
return count;
}
- return empty ? count : start + 1;
+ *has_blank_line = only_spaces ? count > 0 : 0;
+ return only_spaces ? count : start + 1;
}
static void process_stdin(struct trailer_item **in_tok_first,
struct trailer_item **in_tok_last)
{
struct strbuf **lines = read_stdin();
- int start = find_trailer_start(lines);
+ int has_blank_line;
+ int start = find_trailer_start(lines, &has_blank_line);
int
[PATCH v10 05/12] trailer: parse trailers from stdin
Read trailers from stdin, parse them and put the result into a doubly linked
list.
Signed-off-by: Christian Couder
Signed-off-by: Junio C Hamano
---
trailer.c | 76 +++
1 file changed, 76 insertions(+)
diff --git a/trailer.c b/trailer.c
index 89ebff1..6d2da32 100644
--- a/trailer.c
+++ b/trailer.c
@@ -50,6 +50,14 @@ static size_t alnum_len(const char *buf, size_t len)
return len;
}
+static inline int contains_only_spaces(const char *str)
+{
+ const char *s = str;
+ while (*s && isspace(*s))
+ s++;
+ return !*s;
+}
+
static void free_trailer_item(struct trailer_item *item)
{
free(item->conf.name);
@@ -508,3 +516,71 @@ static struct trailer_item *process_command_line_args(int
argc, const char **arg
return arg_tok_first;
}
+
+static struct strbuf **read_stdin(void)
+{
+ struct strbuf **lines;
+ struct strbuf sb = STRBUF_INIT;
+
+ if (strbuf_read(&sb, fileno(stdin), 0) < 0)
+ die_errno(_("could not read from stdin"));
+
+ lines = strbuf_split(&sb, '\n');
+
+ strbuf_release(&sb);
+
+ return lines;
+}
+
+/*
+ * Return the the (0 based) index of the first trailer line
+ * or the line count if there are no trailers.
+ */
+static int find_trailer_start(struct strbuf **lines)
+{
+ int start, empty = 1, count = 0;
+
+ /* Get the line count */
+ while (lines[count])
+ count++;
+
+ /*
+* Get the start of the trailers by looking starting from the end
+* for a line with only spaces before lines with one ':'.
+*/
+ for (start = count - 1; start >= 0; start--) {
+ if (contains_only_spaces(lines[start]->buf)) {
+ if (empty)
+ continue;
+ return start + 1;
+ }
+ if (strchr(lines[start]->buf, ':')) {
+ if (empty)
+ empty = 0;
+ continue;
+ }
+ return count;
+ }
+
+ return empty ? count : start + 1;
+}
+
+static void process_stdin(struct trailer_item **in_tok_first,
+ struct trailer_item **in_tok_last)
+{
+ struct strbuf **lines = read_stdin();
+ int start = find_trailer_start(lines);
+ int i;
+
+ /* Print non trailer lines as is */
+ for (i = 0; lines[i] && i < start; i++)
+ printf("%s", lines[i]->buf);
+
+ /* Parse trailer lines */
+ for (i = start; lines[i]; i++) {
+ struct trailer_item *new = create_trailer_item(lines[i]->buf);
+ add_trailer_item(in_tok_first, in_tok_last, new);
+ }
+
+ strbuf_list_free(lines);
+}
--
1.9.0.163.g8ca203c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 03/12] trailer: read and process config information
Read the configuration to get trailer information, and then process
it and storing it in a doubly linked list.
The config information is stored in the list whose first item is
pointed to by:
static struct trailer_item *first_conf_item;
Signed-off-by: Christian Couder
Signed-off-by: Junio C Hamano
---
trailer.c | 146 ++
1 file changed, 146 insertions(+)
diff --git a/trailer.c b/trailer.c
index 52108c2..c7c0f54 100644
--- a/trailer.c
+++ b/trailer.c
@@ -25,6 +25,8 @@ struct trailer_item {
struct conf_info conf;
};
+static struct trailer_item *first_conf_item;
+
static int same_token(struct trailer_item *a, struct trailer_item *b, int
alnum_len)
{
return !strncasecmp(a->token, b->token, alnum_len);
@@ -245,3 +247,147 @@ static void process_trailers_lists(struct trailer_item
**in_tok_first,
apply_arg_if_missing(in_tok_first, in_tok_last, arg_tok);
}
}
+
+static int set_where(struct conf_info *item, const char *value)
+{
+ if (!strcmp("after", value))
+ item->where = WHERE_AFTER;
+ else if (!strcmp("before", value))
+ item->where = WHERE_BEFORE;
+ else
+ return -1;
+ return 0;
+}
+
+static int set_if_exists(struct conf_info *item, const char *value)
+{
+ if (!strcmp("addIfDifferent", value))
+ item->if_exists = EXISTS_ADD_IF_DIFFERENT;
+ else if (!strcmp("addIfDifferentNeighbor", value))
+ item->if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+ else if (!strcmp("add", value))
+ item->if_exists = EXISTS_ADD;
+ else if (!strcmp("overwrite", value))
+ item->if_exists = EXISTS_OVERWRITE;
+ else if (!strcmp("doNothing", value))
+ item->if_exists = EXISTS_DO_NOTHING;
+ else
+ return -1;
+ return 0;
+}
+
+static int set_if_missing(struct conf_info *item, const char *value)
+{
+ if (!strcmp("doNothing", value))
+ item->if_missing = MISSING_DO_NOTHING;
+ else if (!strcmp("add", value))
+ item->if_missing = MISSING_ADD;
+ else
+ return -1;
+ return 0;
+}
+
+static struct trailer_item *get_conf_item(const char *name)
+{
+ struct trailer_item *item;
+ struct trailer_item *previous;
+
+ /* Look up item with same name */
+ for (previous = NULL, item = first_conf_item;
+item;
+previous = item, item = item->next) {
+ if (!strcasecmp(item->conf.name, name))
+ return item;
+ }
+
+ /* Item does not already exists, create it */
+ item = xcalloc(sizeof(struct trailer_item), 1);
+ item->conf.name = xstrdup(name);
+
+ if (!previous)
+ first_conf_item = item;
+ else {
+ previous->next = item;
+ item->previous = previous;
+ }
+
+ return item;
+}
+
+enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,
+TRAILER_IF_EXISTS, TRAILER_IF_MISSING };
+
+static struct {
+ const char *name;
+ enum trailer_info_type type;
+} trailer_config_items[] = {
+ { "key", TRAILER_KEY },
+ { "command", TRAILER_COMMAND },
+ { "where", TRAILER_WHERE },
+ { "ifexists", TRAILER_IF_EXISTS },
+ { "ifmissing", TRAILER_IF_MISSING }
+};
+
+static int git_trailer_config(const char *conf_key, const char *value, void
*cb)
+{
+ const char *trailer_item, *variable_name;
+ struct trailer_item *item;
+ struct conf_info *conf;
+ char *name = NULL;
+ enum trailer_info_type type;
+ int i;
+
+ trailer_item = skip_prefix(conf_key, "trailer.");
+ if (!trailer_item)
+ return 0;
+
+ variable_name = strrchr(trailer_item, '.');
+ if (!variable_name) {
+ warning(_("two level trailer config variable %s"), conf_key);
+ return 0;
+ }
+
+ variable_name++;
+ for (i = 0; i < ARRAY_SIZE(trailer_config_items); i++) {
+ if (strcmp(trailer_config_items[i].name, variable_name))
+ continue;
+ name = xstrndup(trailer_item, variable_name - trailer_item -
1);
+ type = trailer_config_items[i].type;
+ break;
+ }
+
+ if (!name)
+ return 0;
+
+ item = get_conf_item(name);
+ conf = &item->conf;
+ free(name);
+
+ switch (type) {
+ case TRAILER_KEY:
+ if (conf->key)
+ warning(_("more than one %s"), conf_key);
+ conf->key = xstrdup(value);
+ break;
+ case TRAILER_COMMAND:
+ if (conf->command)
+ warning(_("more than one %s"), conf_key);
+ conf->command = xstrdup(value);
+ break;
+ case TRAILER_WHERE:
+
[PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'
Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- Documentation/git-interpret-trailers.txt | 123 +++ 1 file changed, 123 insertions(+) create mode 100644 Documentation/git-interpret-trailers.txt diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt new file mode 100644 index 000..75ae386 --- /dev/null +++ b/Documentation/git-interpret-trailers.txt @@ -0,0 +1,123 @@ +git-interpret-trailers(1) += + +NAME + +git-interpret-trailers - help add stuctured information into commit messages + +SYNOPSIS + +[verse] +'git interpret-trailers' [--trim-empty] [([(=|:)])...] + +DESCRIPTION +--- +Help add RFC 822-like headers, called 'trailers', at the end of the +otherwise free-form part of a commit message. + +This command is a filter. It reads the standard input for a commit +message and applies the `token` arguments, if any, to this +message. The resulting message is emited on the standard output. + +Some configuration variables control the way the `token` arguments are +applied to the message and the way any existing trailer in the message +is changed. They also make it possible to automatically add some +trailers. + +By default, a 'token=value' or 'token:value' argument will be added +only if no trailer with the same (token, value) pair is already in the +message. The 'token' and 'value' parts will be trimmed to remove +starting and trailing whitespace, and the resulting trimmed 'token' +and 'value' will appear in the message like this: + + +token: value + + +By default, if there are already trailers with the same 'token', the +new trailer will appear just after the last trailer with the same +'token'. Otherwise it will appear at the end of the message. + +Note that 'trailers' do not follow and are not intended to follow many +rules that are in RFC 822. For example they do not follow the line +breaking rules, the encoding rules and probably many other rules. + +OPTIONS +--- +--trim-empty:: + If the 'value' part of any trailer contains only whitespace, + the whole trailer will be removed from the resulting message. + +CONFIGURATION VARIABLES +--- + +trailer..key:: + This 'key' will be used instead of 'token' in the + trailer. After some alphanumeric characters, it can contain + some non alphanumeric characters like ':', '=' or '#' that will + be used instead of ':' to separate the token from the value in + the trailer, though the default ':' is more standard. + +trailer..where:: + This can be either `after`, which is the default, or + `before`. If it is `before`, then a trailer with the specified + token, will appear before, instead of after, other trailers + with the same token, or otherwise at the beginning, instead of + at the end, of all the trailers. + +trailer..ifexist:: + This option makes it possible to choose what action will be + performed when there is already at least one trailer with the + same token in the message. ++ +The valid values for this option are: `addIfDifferent` (this is the +default), `addIfDifferentNeighbor`, `add`, `overwrite` or `doNothing`. ++ +With `addIfDifferent`, a new trailer will be added only if no trailer +with the same (token, value) pair is already in the message. ++ +With `addIfDifferentNeighbor`, a new trailer will be added only if no +trailer with the same (token, value) pair is above or below the line +where the new trailer will be added. ++ +With `add`, a new trailer will be added, even if some trailers with +the same (token, value) pair are already in the message. ++ +With `overwrite`, the new trailer will overwrite an existing trailer +with the same token. ++ +With `doNothing`, nothing will be done, that is no new trailer will be +added if there is already one with the same token in the message. + +trailer..ifmissing:: + This option makes it possible to choose what action will be + performed when there is not yet any trailer with the same + token in the message. ++ +The valid values for this option are: `add` (this is the default) and +`doNothing`. ++ +With `add`, a new trailer will be added. ++ +With `doNothing`, nothing will be done. + +trailer..command:: + This option can be used to specify a shell command that will + be used to automatically add or modify a trailer with the + specified 'token'. ++ +When this option is specified, it is like if a special 'token=value' +argument is added at the end of the command line, where 'value' will +be given by the standard output of the specified command. ++ +If the command contains the `$ARG` string, this string will be +replaced with the 'value' part of an existing trailer with the same +token, if any, before the command is launched. + +SEE ALSO +--
[PATCH v10 06/12] trailer: put all the processing together and print
This patch adds the process_trailers() function that
calls all the previously added processing functions
and then prints the results on the standard output.
Signed-off-by: Christian Couder
Signed-off-by: Junio C Hamano
---
trailer.c | 49 +
trailer.h | 6 ++
2 files changed, 55 insertions(+)
create mode 100644 trailer.h
diff --git a/trailer.c b/trailer.c
index 6d2da32..16465e5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1,4 +1,5 @@
#include "cache.h"
+#include "trailer.h"
/*
* Copyright (c) 2013, 2014 Christian Couder
*/
@@ -68,6 +69,26 @@ static void free_trailer_item(struct trailer_item *item)
free(item);
}
+static void print_tok_val(const char *tok, const char *val)
+{
+ char c = tok[strlen(tok) - 1];
+ if (isalnum(c))
+ printf("%s: %s\n", tok, val);
+ else if (isspace(c) || c == '#')
+ printf("%s%s\n", tok, val);
+ else
+ printf("%s %s\n", tok, val);
+}
+
+static void print_all(struct trailer_item *first, int trim_empty)
+{
+ struct trailer_item *item;
+ for (item = first; item; item = item->next) {
+ if (!trim_empty || strlen(item->value) > 0)
+ print_tok_val(item->token, item->value);
+ }
+}
+
static void add_arg_to_input_list(struct trailer_item *in_tok,
struct trailer_item *arg_tok)
{
@@ -584,3 +605,31 @@ static void process_stdin(struct trailer_item
**in_tok_first,
strbuf_list_free(lines);
}
+
+static void free_all(struct trailer_item **first)
+{
+ while (*first) {
+ struct trailer_item *item = remove_first(first);
+ free_trailer_item(item);
+ }
+}
+
+void process_trailers(int trim_empty, int argc, const char **argv)
+{
+ struct trailer_item *in_tok_first = NULL;
+ struct trailer_item *in_tok_last = NULL;
+ struct trailer_item *arg_tok_first;
+
+ git_config(git_trailer_config, NULL);
+
+ /* Print the non trailer part of stdin */
+ process_stdin(&in_tok_first, &in_tok_last);
+
+ arg_tok_first = process_command_line_args(argc, argv);
+
+ process_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);
+
+ print_all(in_tok_first, trim_empty);
+
+ free_all(&in_tok_first);
+}
diff --git a/trailer.h b/trailer.h
new file mode 100644
index 000..9323b1e
--- /dev/null
+++ b/trailer.h
@@ -0,0 +1,6 @@
+#ifndef TRAILER_H
+#define TRAILER_H
+
+void process_trailers(int trim_empty, int argc, const char **argv);
+
+#endif /* TRAILER_H */
--
1.9.0.163.g8ca203c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 00/12] Add interpret-trailers builtin
This patch series implements a new command: git interpret-trailers and an infrastructure to process trailers that can be reused, for example in "commit.c". 1) Rationale: This command should help with RFC 822 style headers, called "trailers", that are found at the end of commit messages. (Note that these headers do not follow and are not intended to follow many rules that are in RFC 822. For example they do not follow the line breaking rules, the encoding rules and probably many other rules.) For a long time, these trailers have become a de facto standard way to add helpful information into commit messages. Until now git commit has only supported the well known "Signed-off-by: " trailer, that is used by many projects like the Linux kernel and Git. It is better to keep builtin/commit.c uncontaminated by any more hard-wired logic, like what we have for the signed-off-by line. Any new things can and should be doable in hooks, and this filter would help writing these hooks. And that is why the design goal of the filter is to make it at least as powerful as the built-in logic we have for signed-off-by lines; that would allow us to later eject the hard-wired logic for signed-off-by line from the main codepath, if/when we wanted to. Alternatively, we could build a library-ish API around this filter code and replace the hard-wired logic for signed-off-by line with a call into that API, if/when we wanted to, but that requires (in addition to the "at least as powerful as the built-in logic") that the implementation of this stand-alone filter can be cleanly made into a reusable library, so that is a bit higher bar to cross than "everything can be doable with hooks" alternative. 2) Current state: Currently the usage string of this command is: git interpret-trailers [--trim-empty] [([(=|:)])...] The following features are implemented: - the result is printed on stdout - the [[=]>] arguments are interpreted - a commit message read from stdin is interpreted - the "trailer..key" options in the config are interpreted - the "trailer..where" options are interpreted - the "trailer..ifExist" options are interpreted - the "trailer..ifMissing" options are interpreted - the "trailer..command" config works - $ARG can be used in commands - there are 31 tests (4 more than in version 9) - there is some documentation The following features are planned but not yet implemented: - add examples in documentation Possible improvements: - integration with "git commit" - support GIT_COMMIT_PROTO env variable in commands 3) Changes since version 9, thanks to Jonathan and Junio: * added 1 test with empty trailers in patch 10/12 * fixed bugs when there was no 'key' in the config in patch 4/12 and added 2 related tests in patch 10/12 * fixed bug when command failed in patch 9/12 and added 1 related test in patch 10/12 * added patch 12/12 which add one blank line before the trailers if there is not one already This means code changes only in patches 4/12, 9/12, 10/12 and 12/12. Christian Couder (12): trailer: add data structures and basic functions trailer: process trailers from stdin and arguments trailer: read and process config information trailer: process command line trailer arguments trailer: parse trailers from stdin trailer: put all the processing together and print trailer: add interpret-trailers command trailer: add tests for "git interpret-trailers" trailer: execute command from 'trailer..command' trailer: add tests for commands in config file Documentation: add documentation for 'git interpret-trailers' trailer: add blank line before the trailers if needed .gitignore | 1 + Documentation/git-interpret-trailers.txt | 123 ++ Makefile | 2 + builtin.h| 1 + builtin/interpret-trailers.c | 33 ++ git.c| 1 + t/t7513-interpret-trailers.sh| 477 + trailer.c| 709 +++ trailer.h| 6 + 9 files changed, 1353 insertions(+) create mode 100644 Documentation/git-interpret-trailers.txt create mode 100644 builtin/interpret-trailers.c create mode 100755 t/t7513-interpret-trailers.sh create mode 100644 trailer.c create mode 100644 trailer.h -- 1.9.0.163.g8ca203c -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2014, #01; Fri, 4)
Am 05.04.2014 11:19, schrieb Johannes Sixt: > Am 04.04.2014 22:58, schrieb Junio C Hamano: >> * sz/mingw-index-pack-threaded (2014-03-19) 1 commit >> - Enable index-pack threading in msysgit. >> >> What is the status of this topic? A failure report exists >> ($gmane/245170), and I am aware of Duy's $gmane/245034 but that was >> where the discussion stalled. Is everybody waiting for everybody >> else to get the discussion unstuck? > > I still have to cross-check Duy's patch. I'll hopefully get to it in the > next days and report back. The test suite passes with Duy's patch ($gmane/245034), but t5302 fails with this patch with a MinGW build. The patches touch the Cygwin configuration, but I cannot test a Cygwin build. I have, however, lost track of what the objective of these patches is. Is the threaded version significantly faster, and these patches are worth it? -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: The fetch command should "always" honor remote..fetch
Lewis Diamond writes: > 'git fetch foo develop' would result in: > fatal: Couldn't find remote ref test2 //Not OK, (case 1) I have no idea where the "test2" comes from, as it does not appear anywhere in the above write-up, and it could be a bug. > 'git fetch foo master' would result in (FETCH_HEAD omitted): > [new ref] refs/heads/master -> foo/master //OK, but missing another > ref! (case 2) > //It should also fetch refs/users/bob/heads/master -> foo/bob/master This is an incorrect expectation. The user who gave the command line said only "master", and did not want to grab "users/bob/heads/master". If the user wanted to get it as well, the command line would have said so, e.g. git fetch there master users/bob/heads/master > If you remove this configuration line: fetch = > +refs/heads/*:refs/remotes/foo/* > Then you run 'git fetch foo master', this would result in: > * branch master -> FETCH_HEAD //Debatable whether this is OK or not, > but it's definitely missing bob's master! (case 3) Likewise. The 'master' short-hand is designed not to match refs/users/any/thing. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 18/19] tree-diff: rework diff_tree() to generate diffs for multiparent cases as well
Kirill Smelkov writes:
>> > + if (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER)) {
>> > + for (i = 0; i < nparent; ++i)
>> > + if (tp[i].entry.mode & S_IFXMIN_NEQ)
>> > + goto skip_emit_tp;
>> > + }
>> > +
>> > + p = emit_path(p, base, opt, nparent,
>> > + /*t=*/NULL, tp, imin);
>> > +
>> > + skip_emit_tp:
>> > + /* ∀ xk=ximin xk↓ */
>> > + update_tp_entries(tp, nparent);
>>
>> There are parents whose path sort earlier than what is in 't'
>> (i.e. they were lost in the result---we would want to show
>> removal). What makes us jump to the skip label?
>>
>> We are looking at path in 't', and some parents have paths that
>> sort earlier than that path. We will not go to skip label if
>> any one of the parent's entry sorts after some other parent (or
>> the parent in question has ran out its entries), which means we
>> show the entry from the parents only when all the parents have
>> that same path, which is missing from 't'.
>>
>> I am not sure if I am reading this correctly, though.
>>
>> For the two-way diff, the above degenerates to "show all parent
>> entries that come before the first entry in 't'", which is correct.
>> For the combined diff, the current intersect_paths() makes sure that
>> each path appears in all the pair-wise diff between t and tp[],
>> which again means that the above logic match the current behaviour.
>
> Yes, correct (modulo we *will* go to skip label if any one of the
> parent's entry sorts after some other parent). By definition of combined
> diff we show a path only if it shows in every diff D(T,Pi), and if
>
> 2.1) ∃j: pj > p[imin] -> "-p[imin]" ∉ D(T,Pj) -> D += ø; ∀
> pi=p[imin] pi↓
>
> some pj sorts after p[imin] that would mean that Pj does not have
> p[imin] and since t > p[imin] (which means T does not have p[imin]
> either) diff D(T,Pj) does not have p[imin]. And because of that we know
> the whole combined-diff will not have p[imin] as, by definition,
> combined diff is sets intersection and one of the sets does not have
> that path.
>
> ( In usual words p[imin] is not changed between Pj..T - it was
> e.g. removed in Pj~, so merging parents to T does not bring any new
> information wrt path p[imin] and that is why we do not want to show
> p[imin] in combined-diff output - no new change about that path )
>
> So nothing to append to the output, and update minimum tree entries,
> preparing for the next step.
That's all in line with the current and traditional definition of
combined diff.
This is a tangent that is outside the scope of this current topic,
but I wonder if you found it disturbing that we treat the result 't'
that has a path and the result 't' that does not have a path with
respect to a parent that does not have the path in a somewhat
assymmetric way.
With a merge M between commits A and B, where they all have the same
path with different contents, we obviously show that path in the
combined diff format. A merge N that records exactly the same tree
as M that merges the same commits A and B plus another commit C that
does not have that path still shows the combined diff, with one
extra column to express "everything in the result N has been added
with respect to C which did not have the path at all".
However, a merge O between the same commits A and B, where A and B
have a path and O loses it, shows the path in the combined format.
A merge P among the same A, B and an extra parent C that does not
have that path ceases to show it (this is the assymmetry).
It is a natural extension of "Do not show the path when the result
matches one of the parent" rule, and in this case the result P takes
contents, "the path does not exist", from one parent "C", so it is
internally consistent, and I originally designed it that way on
purpose, but somehow it feels a bit strange.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 18/19] tree-diff: rework diff_tree() to generate diffs for multiparent cases as well
Kirill Smelkov writes: > The following > ... > maybe looks a bit simpler, but calls tree_entry_pathcmp twice more times. > > Besides for important nparent=1 case we were not calling > tree_entry_pathcmp at all and here we'll call it once, which would slow > execution down a bit, as base_name_compare shows measurable enough in profile. > To avoid that we'll need to add 'if (i==imin) continue' and this won't > be so simple then. And for general nparent case, as I've said, we'll be > calling tree_entry_pathcmp twice more times... > > Because of all that I'd suggest to go with my original version. OK. > ... After some break on the topic, > with a fresh eye I see a lot of confusion goes from the notation I've > chosen initially (because of how I was reasoning about it on paper, when > it was in flux) - i.e. xi for x[imin] and also using i as looping > variable. And also because xi was already used for x[imin] I've used > another letter 'k' denoting all other x'es, which leads to confusion... > > > I propose we do the following renaming to clarify things: > > A/a -> T/t (to match resulting tree t name in the code) > X/x -> P/p (to match parents trees tp in the code) > i -> imin(so that i would be free for other tasks) > > then the above (with a prologue) would look like > > 8< > * T P1 Pn > * - -- > * |t| |p1| |pn| > * |-| |--| ... |--| imin = argmin(p1...pn) > * | | | | | | > * |-| |--| |--| > * |.| |. | |. | > * . .. > * . .. > * > * at any time there could be 3 cases: > * > * 1) t < p[imin]; > * 2) t > p[imin]; > * 3) t = p[imin]. > * > * Schematic deduction of what every case means, and what to do, follows: > * > * 1) t < p[imin] -> ∀j t ∉ Pj -> "+t" ∈ D(T,Pj) -> D += "+t"; t↓ > * > * 2) t > p[imin] > * > * 2.1) ∃j: pj > p[imin] -> "-p[imin]" ∉ D(T,Pj) -> D += ø; ∀ > pi=p[imin] pi↓ > * 2.2) ∀i pi = p[imin] -> pi ∉ T -> "-pi" ∈ D(T,Pi) -> D += > "-p[imin]"; ∀i pi↓ > * > * 3) t = p[imin] > * > * 3.1) ∃j: pj > p[imin] -> "+t" ∈ D(T,Pj) -> only pi=p[imin] remains > to investigate > * 3.2) pi = p[imin] -> investigate δ(t,pi) > * | > * | > * v > * > * 3.1+3.2) looking at δ(t,pi) ∀i: pi=p[imin] - if all != ø -> > * > * ⎧δ(t,pi) - if pi=p[imin] > * -> D += ⎨ > * ⎩"+t" - if pi>p[imin] > * > * > * in any case t↓ ∀ pi=p[imin] pi↓ > ... > now xk is gone and i matches p[i] (= pi) etc so variable names correlate > to algorithm description better. > > Does that maybe clarify things? That sounds more consistent (modulo perhaps s/argmin/min/ at the beginning?). > P.S. Sorry for maybe some crept-in mistakes - I've tried to verify it > thoroughly, but am too sleepy to be completely sure. On the other hand I > think and hope the patch should be ok. Thanks and do not be sorry for "mistakes"---we have the review process exactly for catching them. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pack-objects: do not reuse packfiles without --delta-base-offset
Jeff King writes: > On Fri, Apr 04, 2014 at 03:28:48PM -0700, Junio C Hamano wrote: > ... >> OK, together with the fact that only ancient versions of fetcher >> would trigger this "do not reuse" codepath, I agree that we should >> go the simplest route this patch takes. > > By the way, we may want to revisit this if we grow more features that do > not allow straight byte-for-byte reuse. True. > I am thinking specifically if we > grow a packv4-like representation for an object, and we plan to convert > on-the-fly to existing packv2 clients. But I think the sensible steps > for that are: > > 1. If we have v4 on disk and are outputting v2, add this case to the > "can_reuse" function I just added. I.e., start out correct, and > turn off the optimization. > > 2. Experiment with on-the-fly conversion. It may be that the > conversion is so expensive that the reuse optimization gets lost in > the noise. Or maybe we can reclaim most of the advantage of the > reuse code path, and it is worth going object-by-object and > converting. But we won't know until we can measure. Yeah; I think these are sensible steps in the future direction. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ls-files: do not trust stat info if lstat() fails
Duy Nguyen writes: >>> Or even better to show an error message when the error code is >>> unexpected? The unkown tag '!' says "there are problems" but if it >>> shows up sort of permanently, '!' won't help much, I think. >> >> I am OK with that approach, but then one question remains: should we >> say it is deleted, modified, both, or neither? > > The question is moot if the user does not ignore stderr because they > should just ignore those error-reported entries. If they do > 2>/dev/null, I think we should err on the safe side and say modified. > We only say deleted if lstat() returns ENOENT or ENOTDIR like in your > patch. Doesn't the same reasoning behind "when we do not know for sure that a path is not modified, it would be safe if we said the path may be modified" also tell us that it is safer to say a path may be lost if we cannot tell? One likely case where we cannot tell if it is modified would be when we cannot read the path (perhaps the parent directory accidentally lost its x-bit). Saying "it may be modified" would be one way to have the user take notice, for an interactive user. A script that runs ls-files may be using the paths to drive "git add", "tar cf -", etc. and emitting such an unreadable path is one way to make these downstream commands signal that something fishy is going on by erroring out. So, I am not sure if we should be silent on an unexpected error when we are asked to report deletes when we would be vocal when we are asked to report modifications. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'
Christian Couder writes: > On Sat, Apr 5, 2014 at 12:42 AM, Junio C Hamano wrote: >> Junio C Hamano writes: >> >>> Christian Couder writes: >>> "The following features are planned but not yet implemented: >>> - add more tests related to commands >>> - add examples in documentation >>> - integration with "git commit"" >> >> I was planning to merge the series to 'next', but perhaps we should >> wait at least for the first two items (I do not think the third one >> is necessary to block the series). > > I will send soon a new version of the series with more tests and fixes. > It will also contains a patch that adds an empty line before the > trailers in the output if there is not already one. Ah, yes, that one was mentioned in the reviews, I remember. > After that I plan to work on adding examples to the documentation. OK, thanks. > First accepting both ':' and '=' means one can see the "git > interpret-trailers" as acting on trailers only. Not just on trailers > from the intput message and option parameters from the command line. Sorry, you lost me. What does "acting on trailers only" really mean? Do you mean the command should/can be run without any command line options, pick up the existing "Signed-off-by:" and friends in its input and emit its output, somehow taking these existing ones as its instruction regarding how to transform the input to its output? > And second there is also a practical advantage, as the user can > copy-paste trailers directly from other messages into the command line > to pass them as arguments to "git interpret-trailers" without the need > to replace the ':' with '='. Even if this command is not often used > directly by users, it might simplify scripts using it. > > Third there is a technical advantage which is that the code that > parses arguments from the command line can be the same as the code > that parses trailers from the input message. I do not see these two as valid arguments to make the command line more complex to the end users---who now need to know that only this command treats its command line in a funny way, accepting a colon in place of an equal sign. A different way to sell a colon, e.g. Consider the instruction sed takes on its command line. (e.g. "sed 's/frotz/nitfol/' http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2014, #01; Fri, 4)
Johannes Sixt writes: > Am 04.04.2014 22:58, schrieb Junio C Hamano: >> * sz/mingw-index-pack-threaded (2014-03-19) 1 commit >> - Enable index-pack threading in msysgit. >> >> What is the status of this topic? A failure report exists >> ($gmane/245170), and I am aware of Duy's $gmane/245034 but that was >> where the discussion stalled. Is everybody waiting for everybody >> else to get the discussion unstuck? > > I still have to cross-check Duy's patch. I'll hopefully get to it in the > next days and report back. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/22] rollback_lock_file(): set fd to -1
Michael Haggerty writes: > The first use of a lock_file object necessarily passes through > lock_file(). The only precondition for that function is that the > on_list field is zero, which is satisfied by a xcalloc()ed object. > > Subsequent uses of a lock_file object must *not* zero the object. > lock_file objects are added to the lock_file_list and never removed. So > zeroing a lock_file object would discard the rest of the linked list. > But subsequent uses must also pass through lock_file(), which sees that > on_list is set, and assumes that the object is in a self-consistent > state as left by commit_lock_file() or rollback_lock_file(). > > At least that's how it is supposed to work. But lock_file objects are > in fact not cleaned up correctly in all circumstances. The next version > of this patch series will work to fix that. Thanks; I see the next round already posted to the list. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-multimail: update to version 1.0.0
This commit contains the squashed changes from the upstream git-multimail repository since the last code drop. Highlights: * Fix encoding of non-ASCII email addresses in email headers. * Fix backwards-compatibility bugs for older Python 2.x versions. * Fix a backwards-compatibility bug for Git 1.7.1. * Add an option commitDiffOpts to customize logs for revisions. * Pass "-oi" to sendmail by default to prevent premature termination on a line containing only ".". * Stagger email "Date:" values in an attempt to help mail clients thread the emails in the right order. * If a mailing list setting is missing, just skip sending the corresponding email (with a warning) instead of failing. * Add a X-Git-Host header that can be used for email filtering. * Allow the sender's fully-qualified domain name to be configured. * Minor documentation improvements. * Add a CHANGES file. Contributions-by: Raphaël Hertzog Contributions-by: Eric Berberich Contributions-by: Michiel Holtkamp Contributions-by: Malte Swart Signed-off-by: Michael Haggerty --- Junio, how would you like other people's contributions to be recorded within the Git project? I have listed them above as "Contributions-by". All of these people have signed off on their contributions (recorded in my GitHub repo). So should I also/instead add "Signed-off-by" for those people? contrib/hooks/multimail/CHANGES | 33 + contrib/hooks/multimail/README | 46 --- contrib/hooks/multimail/README.Git | 4 +- contrib/hooks/multimail/git_multimail.py | 218 ++- contrib/hooks/multimail/post-receive | 4 +- 5 files changed, 249 insertions(+), 56 deletions(-) create mode 100644 contrib/hooks/multimail/CHANGES diff --git a/contrib/hooks/multimail/CHANGES b/contrib/hooks/multimail/CHANGES new file mode 100644 index 000..3603d56 --- /dev/null +++ b/contrib/hooks/multimail/CHANGES @@ -0,0 +1,33 @@ +Release 1.0.0 += + +* Fix encoding of non-ASCII email addresses in email headers. + +* Fix backwards-compatibility bugs for older Python 2.x versions. + +* Fix a backwards-compatibility bug for Git 1.7.1. + +* Add an option commitDiffOpts to customize logs for revisions. + +* Pass "-oi" to sendmail by default to prevent premature termination + on a line containing only ".". + +* Stagger email "Date:" values in an attempt to help mail clients + thread the emails in the right order. + +* If a mailing list setting is missing, just skip sending the + corresponding email (with a warning) instead of failing. + +* Add a X-Git-Host header that can be used for email filtering. + +* Allow the sender's fully-qualified domain name to be configured. + +* Minor documentation improvements. + +* Add this CHANGES file. + + +Release 0.9.0 += + +* Initial release. diff --git a/contrib/hooks/multimail/README b/contrib/hooks/multimail/README index 9904396..477d65f 100644 --- a/contrib/hooks/multimail/README +++ b/contrib/hooks/multimail/README @@ -91,9 +91,10 @@ Requirements been tested; if you do so, please report your results.) * To send emails using the default configuration, a standard sendmail - program must be located at '/usr/sbin/sendmail' and configured - correctly to send emails. If this is not the case, see the - multimailhook.mailer configuration variable below for how to + program must be located at '/usr/sbin/sendmail' or + '/usr/lib/sendmail' and must be configured correctly to send emails. + If this is not the case, set multimailhook.sendmailCommand, or see + the multimailhook.mailer configuration variable below for how to configure git-multimail to send emails via an SMTP server. @@ -169,7 +170,7 @@ multimailhook.repoName for gitolite repositories, or otherwise to derive this value from the repository path name. -multimailhook.mailinglist +multimailhook.mailingList The list of email addresses to which notification emails should be sent, as RFC 2822 email addresses separated by commas. This @@ -184,26 +185,29 @@ multimailhook.refchangeList reference changes should be sent, as RFC 2822 email addresses separated by commas. This configuration option can be multivalued. The default is the value in -multimailhook.mailinglist. Set this value to the empty string to -prevent reference change emails from being sent. +multimailhook.mailingList. Set this value to the empty string to +prevent reference change emails from being sent even if +multimailhook.mailingList is set. multimailhook.announceList The list of email addresses to which emails about new annotated tags should be sent, as RFC 2822 email addresses separated by commas. This configuration option can be multivalued. The -default is the value in multimailhook.refchangelist or -multimailhook.mailinglist. Set this value to the empty string to -prevent annotated tag announcement emails from being se
[PATCH v3 24/27] ref_transaction_commit(): simplify code using temporary variables
Use temporary variables in the for-loop blocks to simplify expressions
in the rest of the loop.
Signed-off-by: Michael Haggerty
---
refs.c | 21 +
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index 2ff195f..33c34df 100644
--- a/refs.c
+++ b/refs.c
@@ -3435,10 +3435,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Acquire all locks while verifying old values */
for (i = 0; i < n; i++) {
- locks[i] = update_ref_lock(updates[i]->refname,
- (updates[i]->have_old ?
- updates[i]->old_sha1 : NULL),
- updates[i]->flags,
+ struct ref_update *update = updates[i];
+
+ locks[i] = update_ref_lock(update->refname,
+ (update->have_old ?
+ update->old_sha1 : NULL),
+ update->flags,
&types[i], onerr);
if (!locks[i]) {
ret = 1;
@@ -3447,16 +3449,19 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
}
/* Perform updates first so live commits remain referenced */
- for (i = 0; i < n; i++)
- if (!is_null_sha1(updates[i]->new_sha1)) {
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (!is_null_sha1(update->new_sha1)) {
ret = update_ref_write(msg,
- updates[i]->refname,
- updates[i]->new_sha1,
+ update->refname,
+ update->new_sha1,
locks[i], onerr);
locks[i] = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
}
+ }
/* Perform deletes now that updates are safely completed */
for (i = 0; i < n; i++)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 10/27] update-ref --stdin: improve error messages for invalid values
If an invalid value is passed to "update-ref --stdin" as or
, include the command and the name of the reference at the
beginning of the error message. Update the tests accordingly.
Signed-off-by: Michael Haggerty
---
builtin/update-ref.c | 24 +---
t/t1400-update-ref.sh | 8
2 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 0dc2061..13a884a 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -35,20 +35,22 @@ static struct ref_update *update_alloc(void)
return update;
}
-static void update_store_new_sha1(struct ref_update *update,
+static void update_store_new_sha1(const char *command,
+ struct ref_update *update,
const char *newvalue)
{
if (*newvalue && get_sha1(newvalue, update->new_sha1))
- die("invalid new value for ref %s: %s",
- update->ref_name, newvalue);
+ die("%s %s: invalid new value: %s",
+ command, update->ref_name, newvalue);
}
-static void update_store_old_sha1(struct ref_update *update,
+static void update_store_old_sha1(const char *command,
+ struct ref_update *update,
const char *oldvalue)
{
if (*oldvalue && get_sha1(oldvalue, update->old_sha1))
- die("invalid old value for ref %s: %s",
- update->ref_name, oldvalue);
+ die("%s %s: invalid old value: %s",
+ command, update->ref_name, oldvalue);
/* We have an old value if non-empty, or if empty without -z */
update->have_old = *oldvalue || line_termination;
@@ -165,12 +167,12 @@ static const char *parse_cmd_update(struct strbuf *input,
const char *next)
die("update line missing ");
if (!parse_next_arg(input, &next, &newvalue))
- update_store_new_sha1(update, newvalue.buf);
+ update_store_new_sha1("update", update, newvalue.buf);
else
die("update %s missing ", update->ref_name);
if (!parse_next_arg(input, &next, &oldvalue)) {
- update_store_old_sha1(update, oldvalue.buf);
+ update_store_old_sha1("update", update, oldvalue.buf);
if (*next != line_termination)
die("update %s has extra input: %s", update->ref_name,
next);
} else if (!line_termination)
@@ -191,7 +193,7 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
die("create line missing ");
if (!parse_next_arg(input, &next, &newvalue))
- update_store_new_sha1(update, newvalue.buf);
+ update_store_new_sha1("create", update, newvalue.buf);
else
die("create %s missing ", update->ref_name);
@@ -216,7 +218,7 @@ static const char *parse_cmd_delete(struct strbuf *input,
const char *next)
die("delete line missing ");
if (!parse_next_arg(input, &next, &oldvalue)) {
- update_store_old_sha1(update, oldvalue.buf);
+ update_store_old_sha1("delete", update, oldvalue.buf);
if (update->have_old && is_null_sha1(update->old_sha1))
die("delete %s given zero old value", update->ref_name);
} else if (!line_termination)
@@ -240,7 +242,7 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
die("verify line missing ");
if (!parse_next_arg(input, &next, &value)) {
- update_store_old_sha1(update, value.buf);
+ update_store_old_sha1("verify", update, value.buf);
hashcpy(update->new_sha1, update->old_sha1);
} else if (!line_termination)
die("verify %s missing [] NUL", update->ref_name);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 00862bc..f6c6e96 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -518,14 +518,14 @@ test_expect_success 'stdin update ref fails with wrong
old value' '
test_expect_success 'stdin update ref fails with bad old value' '
echo "update $c $m does-not-exist" >stdin &&
test_must_fail git update-ref --stdin err &&
- grep "fatal: invalid old value for ref $c: does-not-exist" err &&
+ grep "fatal: update $c: invalid old value: does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
'
test_expect_success 'stdin create ref fails with bad new value' '
echo "create $c does-not-exist" >stdin &&
test_must_fail git update-ref --stdin err &&
- grep "fatal: invalid new value for ref $c: does-not-exist" err &&
+ grep "fatal: create $c: invalid new value: does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
'
@@ -840,14 +840,14 @@ test_expect_success 'stdin -z updat
[PATCH v3 26/27] struct ref_update: add a type field
It used to be that ref_transaction_commit() allocated a temporary
array to hold the types of references while it is working. Instead,
add a type field to ref_update that ref_transaction_commit() can use
as its scratch space.
Signed-off-by: Michael Haggerty
---
refs.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 6fe4bfe8..c058f30 100644
--- a/refs.c
+++ b/refs.c
@@ -3279,6 +3279,7 @@ struct ref_update {
int flags; /* REF_NODEREF? */
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
struct ref_lock *lock;
+ int type;
const char refname[FLEX_ARRAY];
};
@@ -3413,7 +3414,6 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
{
int ret = 0, delnum = 0, i;
struct ref_update **updates;
- int *types;
const char **delnames;
int n = transaction->nr;
@@ -3422,7 +3422,6 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Allocate work space */
updates = xmalloc(sizeof(*updates) * n);
- types = xmalloc(sizeof(*types) * n);
delnames = xmalloc(sizeof(*delnames) * n);
/* Copy, sort, and reject duplicate refs */
@@ -3440,7 +3439,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
(update->have_old ?
update->old_sha1 : NULL),
update->flags,
- &types[i], onerr);
+ &update->type, onerr);
if (!update->lock) {
ret = 1;
goto cleanup;
@@ -3468,7 +3467,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (update->lock) {
delnames[delnum++] = update->lock->ref_name;
- ret |= delete_ref_loose(update->lock, types[i]);
+ ret |= delete_ref_loose(update->lock, update->type);
}
}
@@ -3482,7 +3481,6 @@ cleanup:
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
free(updates);
- free(types);
free(delnames);
ref_transaction_free(transaction);
return ret;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 27/27] ref_transaction_commit(): work with transaction->updates in place
Now that we free the transaction when we are done, there is no need to
make a copy of transaction->updates before working with it.
Signed-off-by: Michael Haggerty
---
refs.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index c058f30..728a761 100644
--- a/refs.c
+++ b/refs.c
@@ -3413,19 +3413,17 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
const char *msg, enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
- struct ref_update **updates;
const char **delnames;
int n = transaction->nr;
+ struct ref_update **updates = transaction->updates;
if (!n)
return 0;
/* Allocate work space */
- updates = xmalloc(sizeof(*updates) * n);
delnames = xmalloc(sizeof(*delnames) * n);
/* Copy, sort, and reject duplicate refs */
- memcpy(updates, transaction->updates, sizeof(*updates) * n);
qsort(updates, n, sizeof(*updates), ref_update_compare);
ret = ref_update_reject_duplicates(updates, n, onerr);
if (ret)
@@ -3480,7 +3478,6 @@ cleanup:
for (i = 0; i < n; i++)
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
- free(updates);
free(delnames);
ref_transaction_free(transaction);
return ret;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 14/27] update-ref.c: extract a new function, parse_next_sha1()
Replace three functions, update_store_new_sha1(),
update_store_old_sha1(), and parse_next_arg(), with a single function,
parse_next_sha1(). The new function takes care of a whole argument,
including checking whether it is there, converting it to an SHA-1, and
emitting errors on EOF or for invalid values. The return value
indicates whether the argument was present or absent, which requires
a bit of intelligence because absent values are represented
differently depending on whether "-z" was used.
The new interface means that the calling functions, parse_cmd_*(),
don't have to interpret the result differently based on the
line_termination mode that is in effect. It also means that
parse_cmd_create() can distinguish unambiguously between an empty new
value and a zeros new value, which fixes a failure in t1400.
Signed-off-by: Michael Haggerty
---
builtin/update-ref.c | 160 +++---
t/t1400-update-ref.sh | 2 +-
2 files changed, 99 insertions(+), 63 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a9eb5fe..c61120f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -35,27 +35,6 @@ static struct ref_update *update_alloc(void)
return update;
}
-static void update_store_new_sha1(const char *command,
- struct ref_update *update,
- const char *newvalue)
-{
- if (*newvalue && get_sha1(newvalue, update->new_sha1))
- die("%s %s: invalid : %s",
- command, update->ref_name, newvalue);
-}
-
-static void update_store_old_sha1(const char *command,
- struct ref_update *update,
- const char *oldvalue)
-{
- if (*oldvalue && get_sha1(oldvalue, update->old_sha1))
- die("%s %s: invalid : %s",
- command, update->ref_name, oldvalue);
-
- /* We have an old value if non-empty, or if empty without -z */
- update->have_old = *oldvalue || line_termination;
-}
-
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
* and append the result to arg. Return a pointer to the terminator.
@@ -112,35 +91,94 @@ static char *parse_refname(struct strbuf *input, const
char **next)
}
/*
- * Parse a SP/NUL separator followed by the next SP- or NUL-terminated
- * argument, if any. If there is an argument, write it to arg, set
- * *next to point at the character terminating the argument, and
+ * The value being parsed is (as opposed to ; the
+ * difference affects which error messages are generated):
+ */
+#define PARSE_SHA1_OLD 0x01
+
+/*
+ * For backwards compatibility, accept an empty string for update's
+ * in binary mode to be equivalent to specifying zeros.
+ */
+#define PARSE_SHA1_ALLOW_EMPTY 0x02
+
+/*
+ * Parse an argument separator followed by the next argument, if any.
+ * If there is an argument, convert it to a SHA-1, write it to sha1,
+ * set *next to point at the character terminating the argument, and
* return 0. If there is no argument at all (not even the empty
- * string), return a non-zero result and leave *next unchanged.
+ * string), return 1 and leave *next unchanged. If the value is
+ * provided but cannot be converted to a SHA-1, die. flags can
+ * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY.
*/
-static int parse_next_arg(struct strbuf *input, const char **next,
- struct strbuf *arg)
+static int parse_next_sha1(struct strbuf *input, const char **next,
+ unsigned char *sha1,
+ const char *command, const char *refname,
+ int flags)
{
- strbuf_reset(arg);
+ struct strbuf arg = STRBUF_INIT;
+ int ret = 0;
+
+ if (*next == input->buf + input->len)
+ goto eof;
+
if (line_termination) {
/* Without -z, consume SP and use next argument */
if (!**next || **next == line_termination)
- return -1;
+ return 1;
if (**next != ' ')
- die("expected SP but got: %s", *next);
+ die("%s %s: expected SP but got: %s",
+ command, refname, *next);
(*next)++;
- *next = parse_arg(*next, arg);
+ *next = parse_arg(*next, &arg);
+ if (arg.len) {
+ if (get_sha1(arg.buf, sha1))
+ goto invalid;
+ } else {
+ /* Without -z, an empty value means all zeros: */
+ hashclr(sha1);
+ }
} else {
/* With -z, read the next NUL-terminated line */
if (**next)
- die("expected NUL but got: %s", *next);
+ die("%s %s: expected NUL but got: %s",
+
[PATCH v3 20/27] update-ref --stdin: reimplement using reference transactions
This change is mostly clerical: the parse_cmd_*() functions need to
use local variables rather than a struct ref_update to collect the
arguments needed for each update, and then call ref_transaction_*() to
queue the change rather than building up the list of changes at the
caller side.
Signed-off-by: Michael Haggerty
---
builtin/update-ref.c | 142 +++
1 file changed, 75 insertions(+), 67 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 423c5c3..405267f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -12,29 +12,11 @@ static const char * const git_update_ref_usage[] = {
NULL
};
-static int updates_alloc;
-static int updates_count;
-static struct ref_update **updates;
+static struct ref_transaction *transaction;
static char line_termination = '\n';
static int update_flags;
-static struct ref_update *update_alloc(void)
-{
- struct ref_update *update;
-
- /* Allocate and zero-init a struct ref_update */
- update = xcalloc(1, sizeof(*update));
- ALLOC_GROW(updates, updates_count + 1, updates_alloc);
- updates[updates_count++] = update;
-
- /* Store and reset accumulated options */
- update->flags = update_flags;
- update_flags = 0;
-
- return update;
-}
-
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
* and append the result to arg. Return a pointer to the terminator.
@@ -196,97 +178,119 @@ static int parse_next_sha1(struct strbuf *input, const
char **next,
static const char *parse_cmd_update(struct strbuf *input, const char *next)
{
- struct ref_update *update;
-
- update = update_alloc();
+ char *refname;
+ unsigned char new_sha1[20];
+ unsigned char old_sha1[20];
+ int have_old;
- update->ref_name = parse_refname(input, &next);
- if (!update->ref_name)
+ refname = parse_refname(input, &next);
+ if (!refname)
die("update: missing ");
- if (parse_next_sha1(input, &next, update->new_sha1,
- "update", update->ref_name,
+ if (parse_next_sha1(input, &next, new_sha1, "update", refname,
PARSE_SHA1_ALLOW_EMPTY))
- die("update %s: missing ", update->ref_name);
+ die("update %s: missing ", refname);
- update->have_old = !parse_next_sha1(input, &next, update->old_sha1,
- "update", update->ref_name,
- PARSE_SHA1_OLD);
+ have_old = !parse_next_sha1(input, &next, old_sha1, "update", refname,
+ PARSE_SHA1_OLD);
if (*next != line_termination)
- die("update %s: extra input: %s", update->ref_name, next);
+ die("update %s: extra input: %s", refname, next);
+
+ ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old);
+
+ update_flags = 0;
+ free(refname);
return next;
}
static const char *parse_cmd_create(struct strbuf *input, const char *next)
{
- struct ref_update *update;
-
- update = update_alloc();
+ char *refname;
+ unsigned char new_sha1[20];
- update->ref_name = parse_refname(input, &next);
- if (!update->ref_name)
+ refname = parse_refname(input, &next);
+ if (!refname)
die("create: missing ");
- if (parse_next_sha1(input, &next, update->new_sha1,
- "create", update->ref_name, 0))
- die("create %s: missing ", update->ref_name);
+ if (parse_next_sha1(input, &next, new_sha1, "create", refname, 0))
+ die("create %s: missing ", refname);
- if (is_null_sha1(update->new_sha1))
- die("create %s: zero ", update->ref_name);
+ if (is_null_sha1(new_sha1))
+ die("create %s: zero ", refname);
if (*next != line_termination)
- die("create %s: extra input: %s", update->ref_name, next);
+ die("create %s: extra input: %s", refname, next);
+
+ ref_transaction_create(transaction, refname, new_sha1, update_flags);
+
+ update_flags = 0;
+ free(refname);
return next;
}
static const char *parse_cmd_delete(struct strbuf *input, const char *next)
{
- struct ref_update *update;
+ char *refname;
+ unsigned char old_sha1[20];
+ int have_old;
- update = update_alloc();
-
- update->ref_name = parse_refname(input, &next);
- if (!update->ref_name)
+ refname = parse_refname(input, &next);
+ if (!refname)
die("delete: missing ");
- if (parse_next_sha1(input, &next, update->old_sha1,
- "delete", update->ref_name, PARSE_SHA1_OLD)) {
- update->have_old = 0;
+
[PATCH v3 17/27] update-ref --stdin: improve the error message for unexpected EOF
Distinguish this error from the error that an argument is missing for another reason. Update the tests accordingly. Signed-off-by: Michael Haggerty --- builtin/update-ref.c | 4 ++-- t/t1400-update-ref.sh | 12 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 6f3b909..0d5f1d0 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -178,8 +178,8 @@ static int parse_next_sha1(struct strbuf *input, const char **next, eof: die(flags & PARSE_SHA1_OLD ? - "%s %s missing " : - "%s %s missing ", + "%s %s: unexpected end of input when reading " : + "%s %s: unexpected end of input when reading ", command, refname); } diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 6b21e45..1db0689 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -709,7 +709,7 @@ test_expect_success 'stdin -z fails create with bad ref name' ' test_expect_success 'stdin -z fails create with no new value' ' printf $F "create $a" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: create $a missing " err + grep "fatal: create $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails create with too many arguments' ' @@ -727,7 +727,7 @@ test_expect_success 'stdin -z fails update with no ref' ' test_expect_success 'stdin -z fails update with too few args' ' printf $F "update $a" "$m" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: update $a missing " err + grep "fatal: update $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails update with bad ref name' ' @@ -747,13 +747,13 @@ test_expect_success 'stdin -z emits warning with empty new value' ' test_expect_success 'stdin -z fails update with no new value' ' printf $F "update $a" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: update $a missing " err + grep "fatal: update $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails update with no old value' ' printf $F "update $a" "$m" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: update $a missing " err + grep "fatal: update $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails update with too many arguments' ' @@ -777,7 +777,7 @@ test_expect_success 'stdin -z fails delete with bad ref name' ' test_expect_success 'stdin -z fails delete with no old value' ' printf $F "delete $a" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: delete $a missing " err + grep "fatal: delete $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails delete with too many arguments' ' @@ -795,7 +795,7 @@ test_expect_success 'stdin -z fails verify with too many arguments' ' test_expect_success 'stdin -z fails verify with no old value' ' printf $F "verify $a" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: verify $a missing " err + grep "fatal: verify $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails option with unknown name' ' -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 12/27] update-ref --stdin: simplify error messages for missing oldvalues
Instead of, for example,
fatal: update refs/heads/master missing [] NUL
emit
fatal: update refs/heads/master missing
Update the tests accordingly.
Signed-off-by: Michael Haggerty
---
builtin/update-ref.c | 6 +++---
t/t1400-update-ref.sh | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index e4c0854..a9eb5fe 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -176,7 +176,7 @@ static const char *parse_cmd_update(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("update %s has extra input: %s", update->ref_name,
next);
} else if (!line_termination)
- die("update %s missing [] NUL", update->ref_name);
+ die("update %s missing ", update->ref_name);
return next;
}
@@ -222,7 +222,7 @@ static const char *parse_cmd_delete(struct strbuf *input,
const char *next)
if (update->have_old && is_null_sha1(update->old_sha1))
die("delete %s given zero ",
update->ref_name);
} else if (!line_termination)
- die("delete %s missing [] NUL", update->ref_name);
+ die("delete %s missing ", update->ref_name);
if (*next != line_termination)
die("delete %s has extra input: %s", update->ref_name, next);
@@ -245,7 +245,7 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
update_store_old_sha1("verify", update, value.buf);
hashcpy(update->new_sha1, update->old_sha1);
} else if (!line_termination)
- die("verify %s missing [] NUL", update->ref_name);
+ die("verify %s missing ", update->ref_name);
if (*next != line_termination)
die("verify %s has extra input: %s", update->ref_name, next);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index ef61fe3..a2015d0 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -739,7 +739,7 @@ test_expect_success 'stdin -z fails update with no new
value' '
test_expect_success 'stdin -z fails update with no old value' '
printf $F "update $a" "$m" >stdin &&
test_must_fail git update-ref -z --stdin err &&
- grep "fatal: update $a missing \\[\\] NUL" err
+ grep "fatal: update $a missing " err
'
test_expect_success 'stdin -z fails update with too many arguments' '
@@ -763,7 +763,7 @@ test_expect_success 'stdin -z fails delete with bad ref
name' '
test_expect_success 'stdin -z fails delete with no old value' '
printf $F "delete $a" >stdin &&
test_must_fail git update-ref -z --stdin err &&
- grep "fatal: delete $a missing \\[\\] NUL" err
+ grep "fatal: delete $a missing " err
'
test_expect_success 'stdin -z fails delete with too many arguments' '
@@ -781,7 +781,7 @@ test_expect_success 'stdin -z fails verify with too many
arguments' '
test_expect_success 'stdin -z fails verify with no old value' '
printf $F "verify $a" >stdin &&
test_must_fail git update-ref -z --stdin err &&
- grep "fatal: verify $a missing \\[\\] NUL" err
+ grep "fatal: verify $a missing " err
'
test_expect_success 'stdin -z fails option with unknown name' '
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 19/27] refs: add a concept of a reference transaction
Build out the API for dealing with a bunch of reference checks and
changes within a transaction. Define an opaque ref_transaction type
that is managed entirely within refs.c. Introduce functions for
beginning a transaction, adding updates to a transaction, and
committing/rolling back a transaction.
This API will soon replace update_refs().
Signed-off-by: Michael Haggerty
---
refs.c | 99 ++
refs.h | 65 +++
2 files changed, 164 insertions(+)
diff --git a/refs.c b/refs.c
index 1305eb1..f0b5764 100644
--- a/refs.c
+++ b/refs.c
@@ -3267,6 +3267,96 @@ static int update_ref_write(const char *action, const
char *refname,
return 0;
}
+/*
+ * Data structure for holding a reference transaction, which can
+ * consist of checks and updates to multiple references, carried out
+ * as atomically as possible. This structure is opaque to callers.
+ */
+struct ref_transaction {
+ struct ref_update **updates;
+ size_t alloc;
+ size_t nr;
+};
+
+struct ref_transaction *ref_transaction_begin(void)
+{
+ return xcalloc(1, sizeof(struct ref_transaction));
+}
+
+static void ref_transaction_free(struct ref_transaction *transaction)
+{
+ int i;
+
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *update = transaction->updates[i];
+
+ free((char *)update->ref_name);
+ free(update);
+ }
+
+ free(transaction->updates);
+ free(transaction);
+}
+
+void ref_transaction_rollback(struct ref_transaction *transaction)
+{
+ ref_transaction_free(transaction);
+}
+
+static struct ref_update *add_update(struct ref_transaction *transaction,
+const char *refname)
+{
+ struct ref_update *update = xcalloc(1, sizeof(*update));
+
+ update->ref_name = xstrdup(refname);
+ ALLOC_GROW(transaction->updates, transaction->nr + 1,
transaction->alloc);
+ transaction->updates[transaction->nr++] = update;
+ return update;
+}
+
+void ref_transaction_update(struct ref_transaction *transaction,
+ const char *refname,
+ unsigned char *new_sha1, unsigned char *old_sha1,
+ int flags, int have_old)
+{
+ struct ref_update *update = add_update(transaction, refname);
+
+ hashcpy(update->new_sha1, new_sha1);
+ update->flags = flags;
+ update->have_old = have_old;
+ if (have_old)
+ hashcpy(update->old_sha1, old_sha1);
+}
+
+void ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ unsigned char *new_sha1,
+ int flags)
+{
+ struct ref_update *update = add_update(transaction, refname);
+
+ assert(!is_null_sha1(new_sha1));
+ hashcpy(update->new_sha1, new_sha1);
+ hashclr(update->old_sha1);
+ update->flags = flags;
+ update->have_old = 1;
+}
+
+void ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ unsigned char *old_sha1,
+ int flags, int have_old)
+{
+ struct ref_update *update = add_update(transaction, refname);
+
+ update->flags = flags;
+ update->have_old = have_old;
+ if (have_old) {
+ assert(!is_null_sha1(old_sha1));
+ hashcpy(update->old_sha1, old_sha1);
+ }
+}
+
int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
int flags, enum action_on_err onerr)
@@ -3378,6 +3468,15 @@ cleanup:
return ret;
}
+int ref_transaction_commit(struct ref_transaction *transaction,
+ const char *msg, enum action_on_err onerr)
+{
+ int ret = update_refs(msg, transaction->updates, transaction->nr,
+ onerr);
+ ref_transaction_free(transaction);
+ return ret;
+}
+
char *shorten_unambiguous_ref(const char *refname, int strict)
{
int i;
diff --git a/refs.h b/refs.h
index 08e60ac..0518dd5 100644
--- a/refs.h
+++ b/refs.h
@@ -24,6 +24,8 @@ struct ref_update {
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
};
+struct ref_transaction;
+
/*
* Bit values set in the flags argument passed to each_ref_fn():
*/
@@ -220,6 +222,69 @@ enum action_on_err {
UPDATE_REFS_QUIET_ON_ERR
};
+/*
+ * Begin a reference transaction. The reference transaction must
+ * eventually be commited using ref_transaction_commit() or rolled
+ * back using ref_transaction_rollback().
+ */
+struct ref_transaction *ref_transaction_begin(void);
+
+/*
+ * Roll back a ref_transaction and free all associated data.
+ */
+void ref_transaction_rollback(struct ref_transaction *transaction);
+
+
+/*
[PATCH v3 13/27] t1400: test that stdin -z update treats empty as zeros
This is the (slightly inconsistent) status quo; make sure it doesn't change by accident. Signed-off-by: Michael Haggerty --- t/t1400-update-ref.sh | 7 +++ 1 file changed, 7 insertions(+) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index a2015d0..208f56e 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -730,6 +730,13 @@ test_expect_success 'stdin -z fails update with bad ref name' ' grep "fatal: invalid ref format: ~a" err ' +test_expect_success 'stdin -z treats empty new value as zeros' ' + git update-ref $a $m && + printf $F "update $a" "" "" >stdin && + git update-ref -z --stdin stdin && test_must_fail git update-ref -z --stdin err && -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 22/27] struct ref_update: rename field "ref_name" to "refname"
This is consistent with the usual nomenclature.
Signed-off-by: Michael Haggerty
---
refs.c | 18 +-
refs.h | 2 +-
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 6984ff0..b6778aa 100644
--- a/refs.c
+++ b/refs.c
@@ -3274,7 +3274,7 @@ static int update_ref_write(const char *action, const
char *refname,
* value or to zero to ensure the ref does not exist before update.
*/
struct ref_update {
- const char *ref_name;
+ const char *refname;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
int flags; /* REF_NODEREF? */
@@ -3304,7 +3304,7 @@ static void ref_transaction_free(struct ref_transaction
*transaction)
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
- free((char *)update->ref_name);
+ free((char *)update->refname);
free(update);
}
@@ -3322,7 +3322,7 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
{
struct ref_update *update = xcalloc(1, sizeof(*update));
- update->ref_name = xstrdup(refname);
+ update->refname = xstrdup(refname);
ALLOC_GROW(transaction->updates, transaction->nr + 1,
transaction->alloc);
transaction->updates[transaction->nr++] = update;
return update;
@@ -3386,7 +3386,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
{
const struct ref_update * const *u1 = r1;
const struct ref_update * const *u2 = r2;
- return strcmp((*u1)->ref_name, (*u2)->ref_name);
+ return strcmp((*u1)->refname, (*u2)->refname);
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
@@ -3394,14 +3394,14 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
{
int i;
for (i = 1; i < n; i++)
- if (!strcmp(updates[i - 1]->ref_name, updates[i]->ref_name)) {
+ if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
const char *str =
"Multiple updates for ref '%s' not allowed.";
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
- error(str, updates[i]->ref_name); break;
+ error(str, updates[i]->refname); break;
case UPDATE_REFS_DIE_ON_ERR:
- die(str, updates[i]->ref_name); break;
+ die(str, updates[i]->refname); break;
case UPDATE_REFS_QUIET_ON_ERR:
break;
}
@@ -3438,7 +3438,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Acquire all locks while verifying old values */
for (i = 0; i < n; i++) {
- locks[i] = update_ref_lock(updates[i]->ref_name,
+ locks[i] = update_ref_lock(updates[i]->refname,
(updates[i]->have_old ?
updates[i]->old_sha1 : NULL),
updates[i]->flags,
@@ -3453,7 +3453,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++)
if (!is_null_sha1(updates[i]->new_sha1)) {
ret = update_ref_write(msg,
- updates[i]->ref_name,
+ updates[i]->refname,
updates[i]->new_sha1,
locks[i], onerr);
locks[i] = NULL; /* freed by update_ref_write */
diff --git a/refs.h b/refs.h
index cb799a3..0f08def 100644
--- a/refs.h
+++ b/refs.h
@@ -154,7 +154,7 @@ extern void unlock_ref(struct ref_lock *lock);
extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *msg);
/** Setup reflog before using. **/
-int log_ref_setup(const char *ref_name, char *logfile, int bufsize);
+int log_ref_setup(const char *refname, char *logfile, int bufsize);
/** Reads log for the value of ref during at_time. **/
extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 21/27] refs: remove API function update_refs()
It has been superseded by reference transactions. This also means
that struct ref_update can become private.
Signed-off-by: Michael Haggerty
---
refs.c | 33 -
refs.h | 20
2 files changed, 20 insertions(+), 33 deletions(-)
diff --git a/refs.c b/refs.c
index f0b5764..6984ff0 100644
--- a/refs.c
+++ b/refs.c
@@ -3267,6 +3267,20 @@ static int update_ref_write(const char *action, const
char *refname,
return 0;
}
+/**
+ * Information needed for a single ref update. Set new_sha1 to the
+ * new value or to zero to delete the ref. To check the old value
+ * while locking the ref, set have_old to 1 and set old_sha1 to the
+ * value or to zero to ensure the ref does not exist before update.
+ */
+struct ref_update {
+ const char *ref_name;
+ unsigned char new_sha1[20];
+ unsigned char old_sha1[20];
+ int flags; /* REF_NODEREF? */
+ int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
+};
+
/*
* Data structure for holding a reference transaction, which can
* consist of checks and updates to multiple references, carried out
@@ -3396,16 +3410,17 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
return 0;
}
-int update_refs(const char *action, struct ref_update * const *updates_orig,
- int n, enum action_on_err onerr)
+int ref_transaction_commit(struct ref_transaction *transaction,
+ const char *msg, enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
struct ref_update **updates;
int *types;
struct ref_lock **locks;
const char **delnames;
+ int n = transaction->nr;
- if (!updates_orig || !n)
+ if (!n)
return 0;
/* Allocate work space */
@@ -3415,7 +3430,7 @@ int update_refs(const char *action, struct ref_update *
const *updates_orig,
delnames = xmalloc(sizeof(*delnames) * n);
/* Copy, sort, and reject duplicate refs */
- memcpy(updates, updates_orig, sizeof(*updates) * n);
+ memcpy(updates, transaction->updates, sizeof(*updates) * n);
qsort(updates, n, sizeof(*updates), ref_update_compare);
ret = ref_update_reject_duplicates(updates, n, onerr);
if (ret)
@@ -3437,7 +3452,7 @@ int update_refs(const char *action, struct ref_update *
const *updates_orig,
/* Perform updates first so live commits remain referenced */
for (i = 0; i < n; i++)
if (!is_null_sha1(updates[i]->new_sha1)) {
- ret = update_ref_write(action,
+ ret = update_ref_write(msg,
updates[i]->ref_name,
updates[i]->new_sha1,
locks[i], onerr);
@@ -3465,14 +3480,6 @@ cleanup:
free(types);
free(locks);
free(delnames);
- return ret;
-}
-
-int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr)
-{
- int ret = update_refs(msg, transaction->updates, transaction->nr,
- onerr);
ref_transaction_free(transaction);
return ret;
}
diff --git a/refs.h b/refs.h
index 0518dd5..cb799a3 100644
--- a/refs.h
+++ b/refs.h
@@ -10,20 +10,6 @@ struct ref_lock {
int force_write;
};
-/**
- * Information needed for a single ref update. Set new_sha1 to the
- * new value or to zero to delete the ref. To check the old value
- * while locking the ref, set have_old to 1 and set old_sha1 to the
- * value or to zero to ensure the ref does not exist before update.
- */
-struct ref_update {
- const char *ref_name;
- unsigned char new_sha1[20];
- unsigned char old_sha1[20];
- int flags; /* REF_NODEREF? */
- int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
-};
-
struct ref_transaction;
/*
@@ -290,12 +276,6 @@ int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
int flags, enum action_on_err onerr);
-/**
- * Lock all refs and then perform all modifications.
- */
-int update_refs(const char *action, struct ref_update * const *updates,
- int n, enum action_on_err onerr);
-
extern int parse_hide_refs_config(const char *var, const char *value, const
char *);
extern int ref_is_hidden(const char *);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 16/27] t1400: test one mistake at a time
This case wants to test passing a bad refname to the "update" command. But it also passes too few arguments to "update", which muddles the situation: which error should be diagnosed? So split this test into two: * One that passes too few arguments to update * One that passes all three arguments to "update", but with a bad refname. Signed-off-by: Michael Haggerty --- t/t1400-update-ref.sh | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 2d61cce..6b21e45 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -724,8 +724,14 @@ test_expect_success 'stdin -z fails update with no ref' ' grep "fatal: update line missing " err ' +test_expect_success 'stdin -z fails update with too few args' ' + printf $F "update $a" "$m" >stdin && + test_must_fail git update-ref -z --stdin err && + grep "fatal: update $a missing " err +' + test_expect_success 'stdin -z fails update with bad ref name' ' - printf $F "update ~a" "$m" >stdin && + printf $F "update ~a" "$m" "" >stdin && test_must_fail git update-ref -z --stdin err && grep "fatal: invalid ref format: ~a" err ' -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 11/27] update-ref --stdin: make error messages more consistent
The old error messages emitted for invalid input sometimes said
""/"" and sometimes said "old value"/"new value".
Convert them all to the former. Update the tests accordingly.
Signed-off-by: Michael Haggerty
---
builtin/update-ref.c | 8
t/t1400-update-ref.sh | 14 +++---
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 13a884a..e4c0854 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -40,7 +40,7 @@ static void update_store_new_sha1(const char *command,
const char *newvalue)
{
if (*newvalue && get_sha1(newvalue, update->new_sha1))
- die("%s %s: invalid new value: %s",
+ die("%s %s: invalid : %s",
command, update->ref_name, newvalue);
}
@@ -49,7 +49,7 @@ static void update_store_old_sha1(const char *command,
const char *oldvalue)
{
if (*oldvalue && get_sha1(oldvalue, update->old_sha1))
- die("%s %s: invalid old value: %s",
+ die("%s %s: invalid : %s",
command, update->ref_name, oldvalue);
/* We have an old value if non-empty, or if empty without -z */
@@ -198,7 +198,7 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
die("create %s missing ", update->ref_name);
if (is_null_sha1(update->new_sha1))
- die("create %s given zero new value", update->ref_name);
+ die("create %s given zero ", update->ref_name);
if (*next != line_termination)
die("create %s has extra input: %s", update->ref_name, next);
@@ -220,7 +220,7 @@ static const char *parse_cmd_delete(struct strbuf *input,
const char *next)
if (!parse_next_arg(input, &next, &oldvalue)) {
update_store_old_sha1("delete", update, oldvalue.buf);
if (update->have_old && is_null_sha1(update->old_sha1))
- die("delete %s given zero old value", update->ref_name);
+ die("delete %s given zero ",
update->ref_name);
} else if (!line_termination)
die("delete %s missing [] NUL", update->ref_name);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index f6c6e96..ef61fe3 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -518,21 +518,21 @@ test_expect_success 'stdin update ref fails with wrong
old value' '
test_expect_success 'stdin update ref fails with bad old value' '
echo "update $c $m does-not-exist" >stdin &&
test_must_fail git update-ref --stdin err &&
- grep "fatal: update $c: invalid old value: does-not-exist" err &&
+ grep "fatal: update $c: invalid : does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
'
test_expect_success 'stdin create ref fails with bad new value' '
echo "create $c does-not-exist" >stdin &&
test_must_fail git update-ref --stdin err &&
- grep "fatal: create $c: invalid new value: does-not-exist" err &&
+ grep "fatal: create $c: invalid : does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
'
test_expect_success 'stdin create ref fails with zero new value' '
echo "create $c " >stdin &&
test_must_fail git update-ref --stdin err &&
- grep "fatal: create $c given zero new value" err &&
+ grep "fatal: create $c given zero " err &&
test_must_fail git rev-parse --verify -q $c
'
@@ -556,7 +556,7 @@ test_expect_success 'stdin delete ref fails with wrong old
value' '
test_expect_success 'stdin delete ref fails with zero old value' '
echo "delete $a " >stdin &&
test_must_fail git update-ref --stdin err &&
- grep "fatal: delete $a given zero old value" err &&
+ grep "fatal: delete $a given zero " err &&
git rev-parse $m >expect &&
git rev-parse $a >actual &&
test_cmp expect actual
@@ -840,14 +840,14 @@ test_expect_success 'stdin -z update ref fails with wrong
old value' '
test_expect_success 'stdin -z update ref fails with bad old value' '
printf $F "update $c" "$m" "does-not-exist" >stdin &&
test_must_fail git update-ref -z --stdin err &&
- grep "fatal: update $c: invalid old value: does-not-exist" err &&
+ grep "fatal: update $c: invalid : does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
'
test_expect_success 'stdin -z create ref fails with bad new value' '
printf $F "create $c" "does-not-exist" >stdin &&
test_must_fail git update-ref -z --stdin err &&
- grep "fatal: create $c: invalid new value: does-not-exist" err &&
+ grep "fatal: create $c: invalid : does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
'
@@ -878,7 +878,7 @@ test_expect_success 'stdin -z delete ref fails with wrong
old value' '
test_expect_
[PATCH v3 23/27] struct ref_update: store refname as a FLEX_ARRAY
Signed-off-by: Michael Haggerty
---
refs.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index b6778aa..2ff195f 100644
--- a/refs.c
+++ b/refs.c
@@ -3274,11 +3274,11 @@ static int update_ref_write(const char *action, const
char *refname,
* value or to zero to ensure the ref does not exist before update.
*/
struct ref_update {
- const char *refname;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
int flags; /* REF_NODEREF? */
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
+ const char refname[FLEX_ARRAY];
};
/*
@@ -3301,12 +3301,8 @@ static void ref_transaction_free(struct ref_transaction
*transaction)
{
int i;
- for (i = 0; i < transaction->nr; i++) {
- struct ref_update *update = transaction->updates[i];
-
- free((char *)update->refname);
- free(update);
- }
+ for (i = 0; i < transaction->nr; i++)
+ free(transaction->updates[i]);
free(transaction->updates);
free(transaction);
@@ -3320,9 +3316,10 @@ void ref_transaction_rollback(struct ref_transaction
*transaction)
static struct ref_update *add_update(struct ref_transaction *transaction,
const char *refname)
{
- struct ref_update *update = xcalloc(1, sizeof(*update));
+ size_t len = strlen(refname);
+ struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
- update->refname = xstrdup(refname);
+ strcpy((char *)update->refname, refname);
ALLOC_GROW(transaction->updates, transaction->nr + 1,
transaction->alloc);
transaction->updates[transaction->nr++] = update;
return update;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 18/27] update-ref --stdin: harmonize error messages
Make (most of) the error messages for invalid input have the same
format [1]:
$COMMAND [SP $REFNAME]: $MESSAGE
Update the tests accordingly.
[1] A few error messages are left with their old form, because
$COMMAND and $REFNAME aren't passed all the way down the call
stack. Maybe those sites should be changed some day, too.
Signed-off-by: Michael Haggerty
---
builtin/update-ref.c | 24
t/t1400-update-ref.sh | 32
2 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 0d5f1d0..423c5c3 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -202,19 +202,19 @@ static const char *parse_cmd_update(struct strbuf *input,
const char *next)
update->ref_name = parse_refname(input, &next);
if (!update->ref_name)
- die("update line missing ");
+ die("update: missing ");
if (parse_next_sha1(input, &next, update->new_sha1,
"update", update->ref_name,
PARSE_SHA1_ALLOW_EMPTY))
- die("update %s missing ", update->ref_name);
+ die("update %s: missing ", update->ref_name);
update->have_old = !parse_next_sha1(input, &next, update->old_sha1,
"update", update->ref_name,
PARSE_SHA1_OLD);
if (*next != line_termination)
- die("update %s has extra input: %s", update->ref_name, next);
+ die("update %s: extra input: %s", update->ref_name, next);
return next;
}
@@ -227,17 +227,17 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
update->ref_name = parse_refname(input, &next);
if (!update->ref_name)
- die("create line missing ");
+ die("create: missing ");
if (parse_next_sha1(input, &next, update->new_sha1,
"create", update->ref_name, 0))
- die("create %s missing ", update->ref_name);
+ die("create %s: missing ", update->ref_name);
if (is_null_sha1(update->new_sha1))
- die("create %s given zero ", update->ref_name);
+ die("create %s: zero ", update->ref_name);
if (*next != line_termination)
- die("create %s has extra input: %s", update->ref_name, next);
+ die("create %s: extra input: %s", update->ref_name, next);
return next;
}
@@ -250,19 +250,19 @@ static const char *parse_cmd_delete(struct strbuf *input,
const char *next)
update->ref_name = parse_refname(input, &next);
if (!update->ref_name)
- die("delete line missing ");
+ die("delete: missing ");
if (parse_next_sha1(input, &next, update->old_sha1,
"delete", update->ref_name, PARSE_SHA1_OLD)) {
update->have_old = 0;
} else {
if (is_null_sha1(update->old_sha1))
- die("delete %s given zero ",
update->ref_name);
+ die("delete %s: zero ", update->ref_name);
update->have_old = 1;
}
if (*next != line_termination)
- die("delete %s has extra input: %s", update->ref_name, next);
+ die("delete %s: extra input: %s", update->ref_name, next);
return next;
}
@@ -275,7 +275,7 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
update->ref_name = parse_refname(input, &next);
if (!update->ref_name)
- die("verify line missing ");
+ die("verify: missing ");
if (parse_next_sha1(input, &next, update->old_sha1,
"verify", update->ref_name, PARSE_SHA1_OLD)) {
@@ -286,7 +286,7 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
}
if (*next != line_termination)
- die("verify %s has extra input: %s", update->ref_name, next);
+ die("verify %s: extra input: %s", update->ref_name, next);
return next;
}
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 1db0689..48ccc4d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -371,7 +371,7 @@ test_expect_success 'stdin fails on junk after quoted
argument' '
test_expect_success 'stdin fails create with no ref' '
echo "create " >stdin &&
test_must_fail git update-ref --stdin err &&
- grep "fatal: create line missing " err
+ grep "fatal: create: missing " err
'
test_expect_success 'stdin fails create with bad ref name' '
@@ -383,19 +383,19 @@ test_expect_success 'stdin fails create with bad ref
name' '
test_expect_success 'stdin fails create with no new value' '
echo "create $a" >stdin &&
test_must_fail git updat
[PATCH v3 15/27] update-ref --stdin -z: deprecate interpreting the empty string as zeros
In the original version of this command, for the single case of the
"update" command's , the empty string was interpreted as
being equivalent to 40 "0"s. This shorthand is unnecessary (binary
input will usually be generated programmatically anyway), and it
complicates the parser and the documentation.
So gently deprecate this usage: remove its description from the
documentation and emit a warning if it is found. But for reasons of
backwards compatibility, continue to accept it.
Helped-by: Brad King
Signed-off-by: Michael Haggerty
---
Documentation/git-update-ref.txt | 18 --
builtin/update-ref.c | 2 ++
t/t1400-update-ref.sh| 5 +++--
3 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 0a0a551..c8f5ae5 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -68,7 +68,12 @@ performs all modifications together. Specify commands of
the form:
option SP LF
Quote fields containing whitespace as if they were strings in C source
-code. Alternatively, use `-z` to specify commands without quoting:
+code; i.e., surrounded by double-quotes and with backslash escapes.
+Use 40 "0" characters or the empty string to specify a zero value. To
+specify a missing value, omit the value and its preceding SP entirely.
+
+Alternatively, use `-z` to specify in NUL-terminated format, without
+quoting:
update SP NUL NUL [] NUL
create SP NUL NUL
@@ -76,8 +81,12 @@ code. Alternatively, use `-z` to specify commands without
quoting:
verify SP NUL [] NUL
option SP NUL
-Lines of any other format or a repeated produce an error.
-Command meanings are:
+In this format, use 40 "0" to specify a zero value, and use the empty
+string to specify a missing value.
+
+In either format, values can be specified in any form that Git
+recognizes as an object name. Commands in any other format or a
+repeated produce an error. Command meanings are:
update::
Set to after verifying , if given.
@@ -102,9 +111,6 @@ option::
The only valid option is `no-deref` to avoid dereferencing
a symbolic ref.
-Use 40 "0" or the empty string to specify a zero value, except that
-with `-z` an empty is considered missing.
-
If all s can be locked with matching s
simultaneously, all modifications are performed. Otherwise, no
modifications are performed. Note that while each individual
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index c61120f..6f3b909 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -154,6 +154,8 @@ static int parse_next_sha1(struct strbuf *input, const char
**next,
goto invalid;
} else if (flags & PARSE_SHA1_ALLOW_EMPTY) {
/* With -z, treat an empty value as all zeros: */
+ warning("%s %s: missing , treating as zero",
+ command, refname);
hashclr(sha1);
} else {
/*
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 15f5bfd..2d61cce 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -730,10 +730,11 @@ test_expect_success 'stdin -z fails update with bad ref
name' '
grep "fatal: invalid ref format: ~a" err
'
-test_expect_success 'stdin -z treats empty new value as zeros' '
+test_expect_success 'stdin -z emits warning with empty new value' '
git update-ref $a $m &&
printf $F "update $a" "" "" >stdin &&
- git update-ref -z --stdin err &&
+ grep "warning: update $a: missing , treating as zero" err &&
test_must_fail git rev-parse --verify -q $a
'
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 25/27] struct ref_update: add a lock field
Now that we manage ref_update objects internally, we can use them to
hold some of the scratch space we need when actually carrying out the
updates. Store the (struct ref_lock *) there.
Signed-off-by: Michael Haggerty
---
refs.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/refs.c b/refs.c
index 33c34df..6fe4bfe8 100644
--- a/refs.c
+++ b/refs.c
@@ -3278,6 +3278,7 @@ struct ref_update {
unsigned char old_sha1[20];
int flags; /* REF_NODEREF? */
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
+ struct ref_lock *lock;
const char refname[FLEX_ARRAY];
};
@@ -3413,7 +3414,6 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int ret = 0, delnum = 0, i;
struct ref_update **updates;
int *types;
- struct ref_lock **locks;
const char **delnames;
int n = transaction->nr;
@@ -3423,7 +3423,6 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Allocate work space */
updates = xmalloc(sizeof(*updates) * n);
types = xmalloc(sizeof(*types) * n);
- locks = xcalloc(n, sizeof(*locks));
delnames = xmalloc(sizeof(*delnames) * n);
/* Copy, sort, and reject duplicate refs */
@@ -3437,12 +3436,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- locks[i] = update_ref_lock(update->refname,
- (update->have_old ?
- update->old_sha1 : NULL),
- update->flags,
- &types[i], onerr);
- if (!locks[i]) {
+ update->lock = update_ref_lock(update->refname,
+ (update->have_old ?
+ update->old_sha1 : NULL),
+ update->flags,
+ &types[i], onerr);
+ if (!update->lock) {
ret = 1;
goto cleanup;
}
@@ -3456,19 +3455,23 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update->refname,
update->new_sha1,
- locks[i], onerr);
- locks[i] = NULL; /* freed by update_ref_write */
+ update->lock, onerr);
+ update->lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
}
}
/* Perform deletes now that updates are safely completed */
- for (i = 0; i < n; i++)
- if (locks[i]) {
- delnames[delnum++] = locks[i]->ref_name;
- ret |= delete_ref_loose(locks[i], types[i]);
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update->lock) {
+ delnames[delnum++] = update->lock->ref_name;
+ ret |= delete_ref_loose(update->lock, types[i]);
}
+ }
+
ret |= repack_without_refs(delnames, delnum);
for (i = 0; i < delnum; i++)
unlink_or_warn(git_path("logs/%s", delnames[i]));
@@ -3476,11 +3479,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
cleanup:
for (i = 0; i < n; i++)
- if (locks[i])
- unlock_ref(locks[i]);
+ if (updates[i]->lock)
+ unlock_ref(updates[i]->lock);
free(updates);
free(types);
- free(locks);
free(delnames);
ref_transaction_free(transaction);
return ret;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 09/27] update-ref.c: extract a new function, parse_refname()
There is no reason to obscure the fact that parse_first_arg() always
parses refnames. Form the new function by combining parse_first_arg()
and update_store_ref_name().
Signed-off-by: Michael Haggerty
---
builtin/update-ref.c | 90
1 file changed, 41 insertions(+), 49 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 51adf2d..0dc2061 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -35,14 +35,6 @@ static struct ref_update *update_alloc(void)
return update;
}
-static void update_store_ref_name(struct ref_update *update,
- const char *ref_name)
-{
- if (check_refname_format(ref_name, REFNAME_ALLOW_ONELEVEL))
- die("invalid ref format: %s", ref_name);
- update->ref_name = xstrdup(ref_name);
-}
-
static void update_store_new_sha1(struct ref_update *update,
const char *newvalue)
{
@@ -86,23 +78,35 @@ static const char *parse_arg(const char *next, struct
strbuf *arg)
}
/*
- * Parse the argument immediately after "command SP". If not -z, then
- * handle C-quoting. Write the argument to arg. Set *next to point
- * at the character that terminates the argument. Die if C-quoting is
- * malformed.
+ * Parse the reference name immediately after "command SP". If not
+ * -z, then handle C-quoting. Return a pointer to a newly allocated
+ * string containing the name of the reference, or NULL if there was
+ * an error. Update *next to point at the character that terminates
+ * the argument. Die if C-quoting is malformed or the reference name
+ * is invalid.
*/
-static void parse_first_arg(struct strbuf *input, const char **next,
- struct strbuf *arg)
+static char *parse_refname(struct strbuf *input, const char **next)
{
- strbuf_reset(arg);
+ struct strbuf ref = STRBUF_INIT;
+
if (line_termination) {
/* Without -z, use the next argument */
- *next = parse_arg(*next, arg);
+ *next = parse_arg(*next, &ref);
} else {
/* With -z, use everything up to the next NUL */
- strbuf_addstr(arg, *next);
- *next += arg->len;
+ strbuf_addstr(&ref, *next);
+ *next += ref.len;
+ }
+
+ if (!ref.len) {
+ strbuf_release(&ref);
+ return NULL;
}
+
+ if (check_refname_format(ref.buf, REFNAME_ALLOW_ONELEVEL))
+ die("invalid ref format: %s", ref.buf);
+
+ return strbuf_detach(&ref, NULL);
}
/*
@@ -150,111 +154,99 @@ static int parse_next_arg(struct strbuf *input, const
char **next,
static const char *parse_cmd_update(struct strbuf *input, const char *next)
{
- struct strbuf ref = STRBUF_INIT;
struct strbuf newvalue = STRBUF_INIT;
struct strbuf oldvalue = STRBUF_INIT;
struct ref_update *update;
update = update_alloc();
- parse_first_arg(input, &next, &ref);
- if (ref.buf[0])
- update_store_ref_name(update, ref.buf);
- else
+ update->ref_name = parse_refname(input, &next);
+ if (!update->ref_name)
die("update line missing ");
if (!parse_next_arg(input, &next, &newvalue))
update_store_new_sha1(update, newvalue.buf);
else
- die("update %s missing ", ref.buf);
+ die("update %s missing ", update->ref_name);
if (!parse_next_arg(input, &next, &oldvalue)) {
update_store_old_sha1(update, oldvalue.buf);
if (*next != line_termination)
- die("update %s has extra input: %s", ref.buf, next);
+ die("update %s has extra input: %s", update->ref_name,
next);
} else if (!line_termination)
- die("update %s missing [] NUL", ref.buf);
+ die("update %s missing [] NUL", update->ref_name);
return next;
}
static const char *parse_cmd_create(struct strbuf *input, const char *next)
{
- struct strbuf ref = STRBUF_INIT;
struct strbuf newvalue = STRBUF_INIT;
struct ref_update *update;
update = update_alloc();
- parse_first_arg(input, &next, &ref);
- if (ref.buf[0])
- update_store_ref_name(update, ref.buf);
- else
+ update->ref_name = parse_refname(input, &next);
+ if (!update->ref_name)
die("create line missing ");
if (!parse_next_arg(input, &next, &newvalue))
update_store_new_sha1(update, newvalue.buf);
else
- die("create %s missing ", ref.buf);
+ die("create %s missing ", update->ref_name);
if (is_null_sha1(update->new_sha1))
- die("create %s given zero new value", ref.buf);
+ die("create %s given zero new value", update->ref_
[PATCH v3 05/27] refs.h: rename the action_on_err constants
Given that these constants are only being used when updating
references, it is inappropriate to give them such generic names as
"DIE_ON_ERR". So prefix their names with "UPDATE_REFS_".
Signed-off-by: Michael Haggerty
---
builtin/checkout.c | 2 +-
builtin/clone.c| 9 +
builtin/merge.c| 6 +++---
builtin/notes.c| 6 +++---
builtin/reset.c| 6 --
builtin/update-ref.c | 5 +++--
contrib/examples/builtin-fetch--tool.c | 3 ++-
notes-cache.c | 2 +-
notes-utils.c | 3 ++-
refs.c | 18 +-
refs.h | 9 +++--
11 files changed, 40 insertions(+), 29 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 1b86d9c..6bf2318 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -624,7 +624,7 @@ static void update_refs_for_switch(const struct
checkout_opts *opts,
/* Nothing to do. */
} else if (opts->force_detach || !new->path) { /* No longer on any
branch. */
update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
- REF_NODEREF, DIE_ON_ERR);
+ REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
if (!opts->quiet) {
if (old->path && advice_detached_head)
detach_advice(new->name);
diff --git a/builtin/clone.c b/builtin/clone.c
index 9b3c04d..b12989d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -521,7 +521,7 @@ static void write_followtags(const struct ref *refs, const
char *msg)
if (!has_sha1_file(ref->old_sha1))
continue;
update_ref(msg, ref->name, ref->old_sha1,
- NULL, 0, DIE_ON_ERR);
+ NULL, 0, UPDATE_REFS_DIE_ON_ERR);
}
}
@@ -589,14 +589,15 @@ static void update_head(const struct ref *our, const
struct ref *remote,
create_symref("HEAD", our->name, NULL);
if (!option_bare) {
const char *head = skip_prefix(our->name,
"refs/heads/");
- update_ref(msg, "HEAD", our->old_sha1, NULL, 0,
DIE_ON_ERR);
+ update_ref(msg, "HEAD", our->old_sha1, NULL, 0,
+ UPDATE_REFS_DIE_ON_ERR);
install_branch_config(0, head, option_origin,
our->name);
}
} else if (our) {
struct commit *c = lookup_commit_reference(our->old_sha1);
/* --branch specifies a non-branch (i.e. tags), detach HEAD */
update_ref(msg, "HEAD", c->object.sha1,
- NULL, REF_NODEREF, DIE_ON_ERR);
+ NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
} else if (remote) {
/*
* We know remote HEAD points to a non-branch, or
@@ -604,7 +605,7 @@ static void update_head(const struct ref *our, const struct
ref *remote,
* Detach HEAD in all these cases.
*/
update_ref(msg, "HEAD", remote->old_sha1,
- NULL, REF_NODEREF, DIE_ON_ERR);
+ NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
}
}
diff --git a/builtin/merge.c b/builtin/merge.c
index e15d0e1..7d1d83e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -398,7 +398,7 @@ static void finish(struct commit *head_commit,
const char *argv_gc_auto[] = { "gc", "--auto", NULL };
update_ref(reflog_message.buf, "HEAD",
new_head, head, 0,
- DIE_ON_ERR);
+ UPDATE_REFS_DIE_ON_ERR);
/*
* We ignore errors in 'gc --auto', since the
* user should see them.
@@ -1222,7 +1222,7 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
die(_("%s - not something we can merge"), argv[0]);
read_empty(remote_head->object.sha1, 0);
update_ref("initial pull", "HEAD", remote_head->object.sha1,
- NULL, 0, DIE_ON_ERR);
+ NULL, 0, UPDATE_REFS_DIE_ON_ERR);
goto done;
} else {
struct strbuf merge_names = STRBUF_INIT;
@@ -1339,7 +1339,7 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
}
update_ref("updating ORIG_HEAD", "ORIG_HEAD", head_commit->object.sha1,
- NULL, 0, DIE_ON_ERR);
+ NULL, 0, UPDATE_REFS_DIE_ON_ERR);
if (remoteheads && !common)
; /* No common ancestors found. We need a real merge. */
diff --git
[PATCH v3 01/27] t1400: fix name and expected result of one test
The test stdin -z create ref fails with zero new value actually passes an empty new value, not a zero new value. So rename the test s/zero/empty/, and change the expected error from fatal: create $c given zero new value to fatal: create $c missing Of course, this makes the test fail now, because although "git update-ref" tries to distinguish between these two errors, it does not succeed in this situation. Fixing it is more than a one-liner, so mark the test test_expect_failure for now. The failure will be fixed later in this patch series. Signed-off-by: Michael Haggerty --- t/t1400-update-ref.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 6ffd82f..fa927d2 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -827,10 +827,10 @@ test_expect_success 'stdin -z create ref fails with bad new value' ' test_must_fail git rev-parse --verify -q $c ' -test_expect_success 'stdin -z create ref fails with zero new value' ' +test_expect_failure 'stdin -z create ref fails with empty new value' ' printf $F "create $c" "" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: create $c given zero new value" err && + grep "fatal: create $c missing " err && test_must_fail git rev-parse --verify -q $c ' -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 02/27] t1400: provide more usual input to the command
The old version was passing (among other things)
update SP refs/heads/c NUL NUL 0{40} NUL
to "git update-ref -z --stdin" to test whether the old-value check for
c is working. But the is empty, which is a bit off the
beaten track.
So, to be sure that we are testing what we want to test, provide an
actual on the "update" line.
Signed-off-by: Michael Haggerty
---
t/t1400-update-ref.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index fa927d2..29391c6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -912,7 +912,7 @@ test_expect_success 'stdin -z update refs works with
identity updates' '
test_expect_success 'stdin -z update refs fails with wrong old value' '
git update-ref $c $m &&
- printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" ""
"$Z" >stdin &&
+ printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "$m"
"$Z" >stdin &&
test_must_fail git update-ref -z --stdin err &&
grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
git rev-parse $m >expect &&
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 04/27] t1400: add some more tests involving quoted arguments
Previously there were no good tests of C-quoted arguments. Signed-off-by: Michael Haggerty --- t/t1400-update-ref.sh | 26 +- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 774f8c5..00862bc 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -350,12 +350,18 @@ test_expect_success 'stdin fails on unknown command' ' grep "fatal: unknown command: unknown $a" err ' -test_expect_success 'stdin fails on badly quoted input' ' +test_expect_success 'stdin fails on unbalanced quotes' ' echo "create $a \"master" >stdin && test_must_fail git update-ref --stdin err && grep "fatal: badly quoted argument: \\\"master" err ' +test_expect_success 'stdin fails on invalid escape' ' + echo "create $a \"ma\zter\"" >stdin && + test_must_fail git update-ref --stdin err && + grep "fatal: badly quoted argument: \\\"mazter\\\"" err +' + test_expect_success 'stdin fails on junk after quoted argument' ' echo "create \"$a\"master" >stdin && test_must_fail git update-ref --stdin err && @@ -458,6 +464,24 @@ test_expect_success 'stdin create ref works' ' test_cmp expect actual ' +test_expect_success 'stdin succeeds with quoted argument' ' + git update-ref -d $a && + echo "create $a \"$m\"" >stdin && + git update-ref --stdin expect && + git rev-parse $a >actual && + test_cmp expect actual +' + +test_expect_success 'stdin succeeds with escaped character' ' + git update-ref -d $a && + echo "create $a \"ma\\163ter\"" >stdin && + git update-ref --stdin expect && + git rev-parse $a >actual && + test_cmp expect actual +' + test_expect_success 'stdin update ref creates with zero old value' ' echo "update $b $m $Z" >stdin && git update-ref --stdin http://vger.kernel.org/majordomo-info.html
[PATCH v3 08/27] parse_cmd_verify(): copy old_sha1 instead of evaluating twice
Aside from avoiding a tiny bit of work, this makes it transparently
obvious that old_sha1 and new_sha1 are identical. It is arguably a
bit silly to have to set new_sha1 in order to verify old_sha1, but
that is a problem for another day.
Signed-off-by: Michael Haggerty
---
builtin/update-ref.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 5f197fe..51adf2d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -249,7 +249,7 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
if (!parse_next_arg(input, &next, &value)) {
update_store_old_sha1(update, value.buf);
- update_store_new_sha1(update, value.buf);
+ hashcpy(update->new_sha1, update->old_sha1);
} else if (!line_termination)
die("verify %s missing [] NUL", ref.buf);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 07/27] update-ref --stdin: read the whole input at once
Read the whole input into a strbuf at once, and then parse it from
there. This might also be a tad faster, but that is not the point.
The point is to decouple the parsing code from the input source (the
old parsing code had to read new data even in the middle of commands).
Add docstrings for the parsing functions.
Signed-off-by: Michael Haggerty
---
builtin/update-ref.c | 170 ---
1 file changed, 108 insertions(+), 62 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a8a68e8..5f197fe 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -85,44 +85,70 @@ static const char *parse_arg(const char *next, struct
strbuf *arg)
return next;
}
-static const char *parse_first_arg(const char *next, struct strbuf *arg)
+/*
+ * Parse the argument immediately after "command SP". If not -z, then
+ * handle C-quoting. Write the argument to arg. Set *next to point
+ * at the character that terminates the argument. Die if C-quoting is
+ * malformed.
+ */
+static void parse_first_arg(struct strbuf *input, const char **next,
+ struct strbuf *arg)
{
- /* Parse argument immediately after "command SP" */
strbuf_reset(arg);
if (line_termination) {
/* Without -z, use the next argument */
- next = parse_arg(next, arg);
+ *next = parse_arg(*next, arg);
} else {
- /* With -z, use rest of first NUL-terminated line */
- strbuf_addstr(arg, next);
- next = next + arg->len;
+ /* With -z, use everything up to the next NUL */
+ strbuf_addstr(arg, *next);
+ *next += arg->len;
}
- return next;
}
-static const char *parse_next_arg(const char *next, struct strbuf *arg)
+/*
+ * Parse a SP/NUL separator followed by the next SP- or NUL-terminated
+ * argument, if any. If there is an argument, write it to arg, set
+ * *next to point at the character terminating the argument, and
+ * return 0. If there is no argument at all (not even the empty
+ * string), return a non-zero result and leave *next unchanged.
+ */
+static int parse_next_arg(struct strbuf *input, const char **next,
+ struct strbuf *arg)
{
- /* Parse next SP-terminated or NUL-terminated argument, if any */
strbuf_reset(arg);
if (line_termination) {
/* Without -z, consume SP and use next argument */
- if (!*next)
- return NULL;
- if (*next != ' ')
- die("expected SP but got: %s", next);
- next = parse_arg(next + 1, arg);
+ if (!**next || **next == line_termination)
+ return -1;
+ if (**next != ' ')
+ die("expected SP but got: %s", *next);
+ (*next)++;
+ *next = parse_arg(*next, arg);
} else {
/* With -z, read the next NUL-terminated line */
- if (*next)
- die("expected NUL but got: %s", next);
- if (strbuf_getline(arg, stdin, '\0') == EOF)
- return NULL;
- next = arg->buf + arg->len;
+ if (**next)
+ die("expected NUL but got: %s", *next);
+ (*next)++;
+ if (*next == input->buf + input->len)
+ return -1;
+ strbuf_addstr(arg, *next);
+ *next += arg->len;
}
- return next;
+ return 0;
}
-static void parse_cmd_update(const char *next)
+
+/*
+ * The following five parse_cmd_*() functions parse the corresponding
+ * command. In each case, next points at the character following the
+ * command name and the following space. They each return a pointer
+ * to the character terminating the command, and die with an
+ * explanatory message if there are any parsing problems. All of
+ * these functions handle either text or binary format input,
+ * depending on how line_termination is set.
+ */
+
+static const char *parse_cmd_update(struct strbuf *input, const char *next)
{
struct strbuf ref = STRBUF_INIT;
struct strbuf newvalue = STRBUF_INIT;
@@ -131,26 +157,28 @@ static void parse_cmd_update(const char *next)
update = update_alloc();
- if ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0])
+ parse_first_arg(input, &next, &ref);
+ if (ref.buf[0])
update_store_ref_name(update, ref.buf);
else
die("update line missing ");
- if ((next = parse_next_arg(next, &newvalue)) != NULL)
+ if (!parse_next_arg(input, &next, &newvalue))
update_store_new_sha1(update, newvalue.buf);
else
die("update %s missing ", ref.buf);
- if ((next = parse_next_arg(next, &oldvalue)) != N
[PATCH v3 03/27] parse_arg(): really test that argument is properly terminated
The old parse_arg(), when fed an argument
"refs/heads/a"master
parsed 'refs/heads/a' off of the front of the argument and considered
itself successful. It was only when parse_next_arg() tried to parse
the *next* argument that a problem was noticed. But in fact, the
definition of the input format requires arguments to be terminated by
SP or NUL, so *this* argument is already erroneous and parse_arg()
should diagnose the problem.
So teach parse_arg() to verify that C-quoted arguments are terminated
correctly. If not, emit a more specific error message.
There is no corresponding error case of a non-C-quoted argument that
is not terminated correctly, because the end of a non-quoted argument
is *by definition* a space or NUL, so there is no way to insert other
junk between the "end" of the argument and the argument terminator.
Adjust the tests to expect the new error message. Add a docstring to
the function, incorporating the comments that were formerly within the
function plus some added information.
Signed-off-by: Michael Haggerty
---
builtin/update-ref.c | 20 +++-
t/t1400-update-ref.sh | 4 ++--
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1292cfe..02b5f95 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update *update,
update->have_old = *oldvalue || line_termination;
}
+/*
+ * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
+ * and append the result to arg. Return a pointer to the terminator.
+ * Die if there is an error in how the argument is C-quoted. This
+ * function is only used if not -z.
+ */
static const char *parse_arg(const char *next, struct strbuf *arg)
{
- /* Parse SP-terminated, possibly C-quoted argument */
- if (*next != '"')
+ if (*next == '"') {
+ const char *orig = next;
+
+ if (unquote_c_style(arg, next, &next))
+ die("badly quoted argument: %s", orig);
+ if (*next && !isspace(*next))
+ die("unexpected character after quoted argument: %s",
orig);
+ } else {
while (*next && !isspace(*next))
strbuf_addch(arg, *next++);
- else if (unquote_c_style(arg, next, &next))
- die("badly quoted argument: %s", next);
+ }
- /* Return position after the argument */
return next;
}
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 29391c6..774f8c5 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input' '
grep "fatal: badly quoted argument: \\\"master" err
'
-test_expect_success 'stdin fails on arguments not separated by space' '
+test_expect_success 'stdin fails on junk after quoted argument' '
echo "create \"$a\"master" >stdin &&
test_must_fail git update-ref --stdin err &&
- grep "fatal: expected SP but got: master" err
+ grep "fatal: unexpected character after quoted argument:
\\\"$a\\\"master" err
'
test_expect_success 'stdin fails create with no ref' '
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 00/27] Clean up update-refs --stdin and implement ref_transaction
Here is v3. It is also available on GitHub [1]. Thanks to Junio and Brad for their comments about v2. I think I have addressed all of your comments (except for Junio's regrets that the series didn't include a transactional receive-pack). See the mailing list threads about v1 [2] and v2 [3] and the longer-term goals of this campaign [4]. Changes since v2: * Rebased to current master (there were no conflicts) * Don't allow ref_transation_create() with new_sha1 set to zeros. * Don't allow ref_transation_delete() with old_sha1 set to zeros. * Fixed subject lines to use lower-case after the colon. * Expanded a few commit messages, fixed a comment, and removed some "squash" detritus in a commit message. Cheers, Michael [1] https://github.com/mhagger/git, branch ref-transactions [2] http://thread.gmane.org/gmane.comp.version-control.git/243731 [3] http://thread.gmane.org/gmane.comp.version-control.git/244857 [4] http://article.gmane.org/gmane.comp.version-control.git/243726 Michael Haggerty (27): t1400: fix name and expected result of one test t1400: provide more usual input to the command parse_arg(): really test that argument is properly terminated t1400: add some more tests involving quoted arguments refs.h: rename the action_on_err constants update_refs(): fix constness update-ref --stdin: read the whole input at once parse_cmd_verify(): copy old_sha1 instead of evaluating twice update-ref.c: extract a new function, parse_refname() update-ref --stdin: improve error messages for invalid values update-ref --stdin: make error messages more consistent update-ref --stdin: simplify error messages for missing oldvalues t1400: test that stdin -z update treats empty as zeros update-ref.c: extract a new function, parse_next_sha1() update-ref --stdin -z: deprecate interpreting the empty string as zeros t1400: test one mistake at a time update-ref --stdin: improve the error message for unexpected EOF update-ref --stdin: harmonize error messages refs: add a concept of a reference transaction update-ref --stdin: reimplement using reference transactions refs: remove API function update_refs() struct ref_update: rename field "ref_name" to "refname" struct ref_update: store refname as a FLEX_ARRAY ref_transaction_commit(): simplify code using temporary variables struct ref_update: add a lock field struct ref_update: add a type field ref_transaction_commit(): work with transaction->updates in place Documentation/git-update-ref.txt | 18 +- builtin/checkout.c | 2 +- builtin/clone.c| 9 +- builtin/merge.c| 6 +- builtin/notes.c| 6 +- builtin/reset.c| 6 +- builtin/update-ref.c | 425 - contrib/examples/builtin-fetch--tool.c | 3 +- notes-cache.c | 2 +- notes-utils.c | 3 +- refs.c | 195 +++ refs.h | 94 ++-- t/t1400-update-ref.sh | 100 +--- 13 files changed, 585 insertions(+), 284 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 06/27] update_refs(): fix constness
The old signature of update_refs() required a
(const struct ref_update **) for its updates_orig argument. The
"const" is presumably there to promise that the function will not
modify the contents of the structures.
But this declaration does not permit the function to be called with a
(struct ref_update **), which is perfectly legitimate. C's type
system is not powerful enough to express what we'd like. So remove
the first "const" from the declaration.
On the other hand, the function *can* promise not to modify the
pointers within the array that is passed to it without inconveniencing
its callers. So add a "const" that has that effect, making the final
declaration
(struct ref_update * const *).
Signed-off-by: Michael Haggerty
---
builtin/update-ref.c | 2 +-
refs.c | 2 +-
refs.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index f6345e5..a8a68e8 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -14,7 +14,7 @@ static const char * const git_update_ref_usage[] = {
static int updates_alloc;
static int updates_count;
-static const struct ref_update **updates;
+static struct ref_update **updates;
static char line_termination = '\n';
static int update_flags;
diff --git a/refs.c b/refs.c
index 196984e..1305eb1 100644
--- a/refs.c
+++ b/refs.c
@@ -3306,7 +3306,7 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
return 0;
}
-int update_refs(const char *action, const struct ref_update **updates_orig,
+int update_refs(const char *action, struct ref_update * const *updates_orig,
int n, enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
diff --git a/refs.h b/refs.h
index a713b34..08e60ac 100644
--- a/refs.h
+++ b/refs.h
@@ -228,7 +228,7 @@ int update_ref(const char *action, const char *refname,
/**
* Lock all refs and then perform all modifications.
*/
-int update_refs(const char *action, const struct ref_update **updates,
+int update_refs(const char *action, struct ref_update * const *updates,
int n, enum action_on_err onerr);
extern int parse_hide_refs_config(const char *var, const char *value, const
char *);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-p4: explicitly specify that HEAD is a revision
'git p4 rebase' fails with the following message if there is a file
named HEAD in the current directory:
fatal: ambiguous argument 'HEAD': both revision and filename
Use '--' to separate paths from revisions, like this:
'git [...] -- [...]'
Take the suggestion above and explicitly state that HEAD should be
treated as a revision.
Signed-off-by: Vlad Dogaru
---
git-p4.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-p4.py b/git-p4.py
index cdfa2df..8d11b25 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3086,7 +3086,7 @@ class P4Rebase(Command):
print "Rebasing the current branch onto %s" % upstream
oldHead = read_pipe("git rev-parse HEAD").strip()
system("git rebase %s" % upstream)
-system("git diff-tree --stat --summary -M %s HEAD" % oldHead)
+system("git diff-tree --stat --summary -M %s HEAD --" % oldHead)
return True
class P4Clone(P4Sync):
--
1.8.5.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 18/25] lockfile: avoid transitory invalid states
On 04/07/2014 02:12 PM, Johannes Sixt wrote: > Am 4/7/2014 13:13, schrieb Michael Haggerty: >> On 04/07/2014 08:16 AM, Johannes Sixt wrote: >>> Am 4/7/2014 1:34, schrieb Michael Haggerty: So, instead of encoding part of the lock_file state in the filename field, add a new bit "LOCK_FLAGS_LOCKFILE_ACTIVE" to flags, and use this bit to distinguish between a lock_file object that is active vs. one that is inactive. Be careful to set this bit only when filename really contains the name of a file that should be deleted on cleanup. >>> >>> Since this flag is primarily for communication between the main code and a >>> signal handler, the only safe way is to define the flag as volatile >>> sig_atomic_t, not to make it a bit of a larger type! >> >> Thanks for the feedback. You are obviously right, and I will fix it. >> >> But I have a feeling that this line of thought is going to lead to the >> signal handler's not being able to do anything. How far can we afford >> to pursue strict correctness? ... >> >> The signal handler currently reads >> >> lock_file_list >> lock_file::next >> lock_file::fd >> lock_file::owner >> lock_file::filename >> *lock_file::filename >> >> and writes lock_file_list. Among other things it calls close(), >> unlink(), vsnprintf(), and fprintf() (the last two via warning()). >> >> But most of these actions are undefined under the C99 standard: > > Good point. But not all is lost because some of the functions are > well-defined under POSIX, particularly close() and unlink(). (*printf are > not, though.) > >> I don't have time to rewrite *all* of Git right now, so how can we get >> reasonable safety and portability within a feasible amount of work? > > It shouldn't be *that* bad. We can make all members volatile, except > filename (because we wouldn't be able to strcpy(lk->filename, ...) without > a type cast). > > How far *do* you want to go? I'm certainly not opposed to field-test your > current changeset (plus and adjustment to use sig_atomic_t) -- overall it > is an improvement. And then we will see how it works. For now I think I'd just like to get the biggest problems fixed without making anything worse. Given that there might be a GSoC student working in this neighborhood, he/she might be able to take up the baton. I changed the patch series to use a new "volatile sig_atomic_t active" field rather than a bit in a "flags" field. I'll wait a short time to see if there is more feedback before pushing it to the list; meanwhile you can find it here if you have time to look at it and/or test it: http://github.com/mhagger/git, branch "lock-correctness" Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 18/25] lockfile: avoid transitory invalid states
Am 4/7/2014 13:13, schrieb Michael Haggerty: > On 04/07/2014 08:16 AM, Johannes Sixt wrote: >> Am 4/7/2014 1:34, schrieb Michael Haggerty: >>> So, instead of encoding part of the lock_file state in the filename >>> field, add a new bit "LOCK_FLAGS_LOCKFILE_ACTIVE" to flags, and use >>> this bit to distinguish between a lock_file object that is active >>> vs. one that is inactive. Be careful to set this bit only when >>> filename really contains the name of a file that should be deleted on >>> cleanup. >> >> Since this flag is primarily for communication between the main code and a >> signal handler, the only safe way is to define the flag as volatile >> sig_atomic_t, not to make it a bit of a larger type! > > Thanks for the feedback. You are obviously right, and I will fix it. > > But I have a feeling that this line of thought is going to lead to the > signal handler's not being able to do anything. How far can we afford > to pursue strict correctness? ... > > The signal handler currently reads > > lock_file_list > lock_file::next > lock_file::fd > lock_file::owner > lock_file::filename > *lock_file::filename > > and writes lock_file_list. Among other things it calls close(), > unlink(), vsnprintf(), and fprintf() (the last two via warning()). > > But most of these actions are undefined under the C99 standard: Good point. But not all is lost because some of the functions are well-defined under POSIX, particularly close() and unlink(). (*printf are not, though.) > I don't have time to rewrite *all* of Git right now, so how can we get > reasonable safety and portability within a feasible amount of work? It shouldn't be *that* bad. We can make all members volatile, except filename (because we wouldn't be able to strcpy(lk->filename, ...) without a type cast). How far *do* you want to go? I'm certainly not opposed to field-test your current changeset (plus and adjustment to use sig_atomic_t) -- overall it is an improvement. And then we will see how it works. Just as food for thought: A compiler barrier should be sufficient to inhibit that the compiler reorders code across accesses of the volatile flag. Like in the main code: strcpy(lk->filename, ...); BARRIER(); lk->is_active = 1; /* volatile sig_atomic_t */ and in the signal handler: if (!lk->is_active) return; BARRIER(); unlink(lk->filename); with some suitable definition of BARRIER(). I don't think that we need an explicit memory barrier (in practice) because that should be implied by the context switch leading to the signal handler. -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 18/25] lockfile: avoid transitory invalid states
On 04/07/2014 08:16 AM, Johannes Sixt wrote:
> Am 4/7/2014 1:34, schrieb Michael Haggerty:
>> Because remove_lock_file() can be called any time by the signal
>> handler, it is important that any lock_file objects that are in the
>> lock_file_list are always in a valid state. And since lock_file
>> objects are often reused (but are never removed from lock_file_list),
>> that means we have to be careful whenever mutating a lock_file object
>> to always keep it in a well-defined state.
>> ...
>> So, instead of encoding part of the lock_file state in the filename
>> field, add a new bit "LOCK_FLAGS_LOCKFILE_ACTIVE" to flags, and use
>> this bit to distinguish between a lock_file object that is active
>> vs. one that is inactive. Be careful to set this bit only when
>> filename really contains the name of a file that should be deleted on
>> cleanup.
>
> Since this flag is primarily for communication between the main code and a
> signal handler, the only safe way is to define the flag as volatile
> sig_atomic_t, not to make it a bit of a larger type!
Thanks for the feedback. You are obviously right, and I will fix it.
But I have a feeling that this line of thought is going to lead to the
signal handler's not being able to do anything. How far can we afford
to pursue strict correctness? We have
struct lock_file {
struct lock_file *next;
int fd;
pid_t owner;
char on_list;
char filename[PATH_MAX];
};
static struct lock_file *lock_file_list;
The signal handler currently reads
lock_file_list
lock_file::next
lock_file::fd
lock_file::owner
lock_file::filename
*lock_file::filename
and writes lock_file_list. Among other things it calls close(),
unlink(), vsnprintf(), and fprintf() (the last two via warning()).
But most of these actions are undefined under the C99 standard:
> If the signal occurs other than as the result of calling the abort
> or raise function, the behavior is undefined if the signal handler
> refers to any object with static storage duration other than by
> assigning a value to an object declared as volatile sig_atomic_t, or
> the signal handler calls any function in the standard library other
> than the abort function, the _Exit function, or the signal function
> with the first argument equal to the signal number corresponding to
> the signal that caused the invocation of the handler.
I don't have time to rewrite *all* of Git right now, so how can we get
reasonable safety and portability within a feasible amount of work?
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'
On Sat, Apr 5, 2014 at 12:42 AM, Junio C Hamano wrote: > Junio C Hamano writes: > >> Christian Couder writes: >> "The following features are planned but not yet implemented: >> - add more tests related to commands >> - add examples in documentation >> - integration with "git commit"" > > I was planning to merge the series to 'next', but perhaps we should > wait at least for the first two items (I do not think the third one > is necessary to block the series). I will send soon a new version of the series with more tests and fixes. It will also contains a patch that adds an empty line before the trailers in the output if there is not already one. After that I plan to work on adding examples to the documentation. >>> Why support both '=' and ':'? Using just one would make it easier to >>> grep through scripts to see who is adding signoffs. >> >> That was already discussed previously. > > I do recall it was discussed previously, but given that a new reader > posed the same question, I am not sure if the end result in this > patch under discussion sufficiently answers the question in a > satisfactory way. > >> The reason is that people are used to "token=value" for command line >> arguments, but trailers appears in the result as "token: value", so it >> is better for the user if we support both. > > While I do understand the part before ", so" on the second line, I > do not see why that leads to the conclusion at all. > > Yes, because it is a well-established convention to separate option > name with its parameter with '=', accepting "--option=parameter" > makes sense. That may result in a string "Option: parameter" added > to the output from the command. I am not sure why that output has > anything to do with how the command line should be specified. First accepting both ':' and '=' means one can see the "git interpret-trailers" as acting on trailers only. Not just on trailers from the intput message and option parameters from the command line. But from trailers both from the input message and being passed as arguments. In my opinion it is good if it can be seen this way, as it may simplifies the user's mental model of how the command works. And second there is also a practical advantage, as the user can copy-paste trailers directly from other messages into the command line to pass them as arguments to "git interpret-trailers" without the need to replace the ':' with '='. Even if this command is not often used directly by users, it might simplify scripts using it. Third there is a technical advantage which is that the code that parses arguments from the command line can be the same as the code that parses trailers from the input message. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list
On Sun, Apr 06, 2014 at 11:54:59PM +0200, Michael Haggerty wrote: > > I didn't reproduce it experimentally, though. We should be able to just > > > > lk->owner = 0; > > > > before the initial strcpy to fix it, I would think. > > I think that using the owner field to avoid this problem is a bit > indirect, so I will soon submit a fix that involves adding a flag to > lock_file objects indicating whether the filename field currently > contains the name of a file that needs to be deleted. Yeah, I agree that the current code is a bit subtle. I was planning to address this during the tempfile cleanup project (either in GSoC, if it gets a slot, or just doing it myself). But I don't mind if you want to do something in the interim. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
