Re: git merge branch --no-commit does commit fast forward merges
On 18 April 2016 at 16:26, Johannes Schindelin wrote: > > > The command only works as expected when also adding the --no-ff flag. > > Then you need to fix your expectations ;-) I *think* the core of this problem is that the intent of the end-user does not align with the command options available. In this use case (as far as I can tell), the user wants to see what the result of a merge from somewhere else will look like, without changing their HEAD. While you are correct in saying a fast-forward does not create any new commits, for the user it certainly looks like a whole slew of new commits have been added. Moreover, the nature of the option means that the user has to investigate if the merge is a fast-forward in order to know what the outcome of the command will be. If the merge is a fast-forward, --no-commit has no effect on the outcome. If the merge is not a fast-forward, --no-commit has a huge effect on the outcome. If I see a --no-commit option, as an inexperienced user, I would be quite surprised to find my HEAD changed after using it. It would be far more intuitive, for that user, for --no-commit to imply --no-ff however I suspect that such a change may well cause more problems then it fixes. What I wonder is, in what situation is the current behaviour is desirable? While I agree that the option works as designed, I think its behaviour is more surprising to the end user then it should be. Regards, Andrew Ardill -- 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] Extend runtime prefix computation
Hi Junio, On Fri, 15 Apr 2016, Junio C Hamano wrote: > Michael Weiser writes: > > > Make git fully relocatable at runtime extending the runtime prefix > > calculation. Handle absolute and relative paths in argv0. Handle no path > > at all in argv0 in a system-specific manner. Replace assertions with > > initialised variables and checks that lead to fallback to the static > > prefix. > > That's a dense description of "what" without saying much about > "why". Hint: start by describing what case(s) the current code > fails to find the correct runtime prefix. That would give readers a > better understanding of what problem you are trying to solve. I have to admit that I am really, *really* skeptical. To me, it looks like this patch opens the door very wide to unintended consequences. > > #ifdef RUNTIME_PREFIX > > - assert(argv0_path); > > - assert(is_absolute_path(argv0_path)); > > Aren't these protecting against future and careless change that > forgets to call extract-argv0-path or make that function return > something that is not an absolute path? This (first) assert() indeed saved me a couple of times from hunting for bugs in the wrong place. Let's keep it. Ciao, Dscho -- 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: git merge branch --no-commit does commit fast forward merges
My expectations from what should happen came mainly from the description of the --no-commit flag in the help: With --no-commit perform the merge but pretend the merge failed and do not autocommit, to give the user a chance to inspect and further tweak the merge result before committing. So in the case of a fast-forward the flag does not pretend that the merge failed. Regards, Christoph Andrew Ardill writes: On 18 April 2016 at 16:26, Johannes Schindelin wrote: > The command only works as expected when also adding the > --no-ff flag. Then you need to fix your expectations ;-) I *think* the core of this problem is that the intent of the end-user does not align with the command options available. In this use case (as far as I can tell), the user wants to see what the result of a merge from somewhere else will look like, without changing their HEAD. While you are correct in saying a fast-forward does not create any new commits, for the user it certainly looks like a whole slew of new commits have been added. Moreover, the nature of the option means that the user has to investigate if the merge is a fast-forward in order to know what the outcome of the command will be. If the merge is a fast-forward, --no-commit has no effect on the outcome. If the merge is not a fast-forward, --no-commit has a huge effect on the outcome. If I see a --no-commit option, as an inexperienced user, I would be quite surprised to find my HEAD changed after using it. It would be far more intuitive, for that user, for --no-commit to imply --no-ff however I suspect that such a change may well cause more problems then it fixes. What I wonder is, in what situation is the current behaviour is desirable? While I agree that the option works as designed, I think its behaviour is more surprising to the end user then it should be. Regards, Andrew Ardill -- --- Christoph Paulik Twitter, Github: @cpaulik PGP: 8CFC D7DF 2867 B2DC 749B 1B0A 6E3B A262 5186 A0AC signature.asc Description: PGP signature
Re: git merge branch --no-commit does commit fast forward merges
On 18 April 2016 at 17:23, Christoph Paulik wrote: > My expectations from what should happen came mainly from the description of > the --no-commit flag in the help: > > With --no-commit perform the merge but pretend the merge failed and do not > autocommit, to give the user a chance to inspect and further tweak the merge > result before committing. > So in the case of a fast-forward the flag does not pretend that the merge > failed. Yes, I think the mis-alignment in expectations comes from a technicality in the description you quote. The fast forward is in some ways not really counted as a true merge, and no new commits are created. Thus, the merge progresses up to the point where a merge resolution would have to take place, realises that there is no merge resolution to do (it's just a fast forward!) and so exits out. Unfortunately, a side effect of this is that the fast-forward has already happened and so you are left with something different from what was expected. I do think that the --no-commit option should imply --no-ff (as this would make the behaviour consistent for end-users). I don't know if this is something that would break scripts etc, but if so you could make it implied only if we detect a terminal or something like is done in other places. Regards, Andrew Ardill -- 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
Git for Windows Portable
Hi, I have question regarding Git for Windows Portable in version 2.8.1 32bit that can be downloaded from https://git-scm.com/download/win . What is the minimum version of .NET and OS that is necessary to successfully run it? Thank you for your answer. Best Regards Lukáš Rumpala-- 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
IMPORTANT MAIL TO YOU
I am Capt. Lawrence Tyman, an officer in US Army,and also a West Point Graduate, serving in the Military with the 82nd Air Borne Division Peace keeping force deployed from Afganistan to Syria. We were moved to Syria from Iraq as the last batch just left,and i really need your help in assisting me with the safe keeping of 1 military trunk box contain funds amount of $10.2M which i secured on a raiding we carried out in January in one of the chief Syrian IsIs base which i headed the squard as the Captain. With every possible arrangement to lift this box out, is intended to arrive Belgium from there a diplomat will deliver it to your designated location I hope you can be trusted? You will be rewarded handsomely if you could help me secure the funds until I conclude my service here in 3 month to meet you while we can plan head to head on a good and profitable business or company i can invest my funds in your country. If you can be trusted and willing to support me in securing this safely kindly indicate by Letting me know this (1) Your name (2) Your address (3) Age (4) Occupation and i will explain further when i get a response from you kindly contact me in this my private email address below: lawrencety...@gmx.com Regards, Capt. Lawrence Tyman -- 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
Hi git
Greetings git http://yozgatnakliyat.net/standard.php?theres=y1p39sb1ca7safkew madhan...@yahoo.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
Fwd: BUG: absolute paths on windows
Hello, I've discovered bug in git. I'm used to add files by copying absolute path from my IDE. Now on Windows 10 I discovered bug. Git says: "is outside repository" when I use "git add C:\repo\file.txt". For "git add c:\repo\file.txt" (drive letter is lower-case) it works like it worked before on previous windows. I think it's bug because windows file paths are case insensitive and both of them should work. My git version is "git version 2.8.0.windows.1". Milan Davídek -- 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: Where can I find the MD5 or SHA1 of git preview client.
From: "Johannes Schindelin" Hi Wu, On Sun, 17 Apr 2016, bin wu wrote: There is still a question.Why not just post the the MD5 and SHA1 on the download page? We do: https://github.com/git-for-windows/git/releases Thanks, I couldn't see it for looking (I was sure there was one somewhere). I've corrected the FAQ and linked wiki release-hashes pages. -- Philip -- 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
Binary grep t7008 known breakage vanished on Cygwin
t7008.12 is marked as an expected failure, but building Git on Cygwin including a `make configure && ./configure` step has the test unexpectedly passing. Building without the configure step has the test failing as expected. This appears to be behaviour specific to Cygwin; at least I get that test failing on my CentOS box regardless of whether I perform the configure step. The "problem" here is `git grep` is matching a null byte with a "."; the test implies that ought to work in theory but hasn't worked in practice since the test was added in v1.7.1-101-gf96e567. The commit message there asserts "NUL characters themselves are not matched in any way, though", which is evidently not the case on Cygwin, provided the `configure` script is run. I'm not sufficiently familiar with the standards and library interfaces here to have any idea what the "correct" regex behaviour in this circumstance is. I'm not sure what the correct thing to do in the face of such an unexpected test pass; it looks as though Cygwin Git's `grep` is going to behave in a subtly different way to Git on other platforms as a result of this, which is probably not ideal, but I don't know if there's anything that "ought" to be done to either ensure consistent behaviour across platforms, or to stop marking the test as an expected failure on platforms where it passes. Adam -- 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: git merge branch --no-commit does commit fast forward merges
Andrew Ardill writes:
> Yes, I think the mis-alignment in expectations comes from a
> technicality in the description you quote. The fast forward is in some
> ways not really counted as a true merge, and no new commits are
> created.
Looking at 123ee3ca (Add --no-commit to git-merge/git-pull.,
2005-11-01) and $gmane/10998 [*1*], it is clear that "--no-commit"
was never meant as a "preview of what would happen". The original
documentation update at 37465016 (Documentation: -merge and -pull:
describe merge strategies., 2005-11-04) was not great, but was
clarified at d8ae1d10 (Document the --no-commit flag better,
2005-11-04), and that version of text survives to this day.
The real reason why "--no-commit" was added was because back then
"git commit --amend" did not even exist; it appeared only at
b4019f04 (git-commit --amend, 2006-03-02).
What is (and was back then) the recommended way to see what changes
merging the other branch brings in to your branch, then?
There are at least three ways, all of which are better suited than
"--no-commit".
When you want to study and understand what changes other branch
made since it forked from what you are working on, then
$ git diff ...other_branch
would give you the change as a single ball of wax [*2*].
If you want to see individual changes explained by their authors,
you can also do
$ git log -p ..other_branch
Finally, if you want to see what the merge result would look like,
you just do the merge. Advancing the HEAD by one commit and then
going back once you are done is a cheap operation. If you want to
avoid updating your branch for real, these days you can even do so
on a detached HEAD, unlike old days back when there was not even
"commit --amend".
$ git checkout HEAD^0
$ git merge other_branch
$ git diff ORIG_HEAD ;# what changed overall?
$ git log -p ORIG_HEAD.. ;# inspect individual changes
$ git checkout - ;# come back to the original branch
> I do think that the --no-commit option should imply --no-ff (as this
> would make the behaviour consistent for end-users). I don't know if
> this is something that would break scripts etc, but if so you could
> make it implied only if we detect a terminal or something like is done
> in other places.
If we were living in an ideal world where "git commit --amend" were
already there in November 2005, we wouldn't have "merge --no-commit"
or "pull --no-commit" in our system today, and in such a world, I
would agree that "try to populate the working tree and the index
with result of a hypothetical merge and stop without updating HEAD
nor creating MERGE_HEAD, only to show what would happen if I merged"
option could be a useful addition to these two commands. And we may
choose to call such an option "--no-commit". I agree that such an
option should probably imply "--no-ff".
But we are not living in that world. Making "--no-commit" (which is
not that "try to populate and show" command) imply "--no-ff" will
break existing scripts. And unlike cosmetic things like "do we show
in color", changing the behaviour of a command in a fundamental way
based on term and istty() is a sure way to make commands harder to
understand ("it works this way from the terminal, but it works
differently in my script. what is going on?" is not a question we
are better off not seeing on this list).
Thanks.
[Notes and References]
*1* http://thread.gmane.org/gmane.comp.version-control.git/10998
*2* Notice the three dots; it is a short-hand for
$ git diff ^$(git merge-base HEAD other_branch) other_branch
--
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: git merge branch --no-commit does commit fast forward merges
Junio C Hamano writes: > Andrew Ardill writes: > >> I do think that the --no-commit option should imply --no-ff (as this >> would make the behaviour consistent for end-users). I don't know if >> this is something that would break scripts etc, but if so you could >> make it implied only if we detect a terminal or something like is done >> in other places. > > But we are not living in that world. Making "--no-commit" (which is > not that "try to populate and show" command) imply "--no-ff" will > break existing scripts Having said all that, there is one change we might want to consider, with a plan to transition to cope with backward incompatibility. A user who uses "--no-commit" does so with the intention to record a resulting merge after amending the merge result in the working tree. But there is nothing to amend and record, if the same "git merge" without "--no-commit" wouldn't have created a merge commit (there are two cases: (1) the other branch is a descendant of the current branch, (2) the other branch is an ancestor of the current branch). And the user would want to know that before doing further damange to his history, so we may want to start warn when "--no-commit" fast-forwarded or succeeded with "already up-to-date", with deprecation notice, and eventually want to make it an error. Those who want to do a scripted git merge --no-commit "$1" && autoedit "$1" && git commit (where the script takes a branch name $1 and uses auto-edit to further record the fact that a branch $1 was merged to somewhere in the contents) would already be buggy if it wants to force no-ff, and will get a warning (and later an error), with such a change. And fixing the script to add "--no-ff" next to "--no-commit" will also stop the new warning/error. -- 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] mv: allow moving nested submodules
When directories are moved using `git mv` all files in the directory
have been just moved, but no further action was taken on them. This
was done by assigning the mode = WORKING_DIRECTORY to the files
inside a moved directory.
submodules however need to update their link to the git directory as
well as updates to the .gitmodules file. By removing the condition of
`mode != INDEX` (the remaining modes are BOTH and WORKING_DIRECTORY) for
the required submodule actions, we perform these for submodules in a
moved directory.
Signed-off-by: Stefan Beller
---
builtin/mv.c | 7 +--
t/t7001-mv.sh | 16
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index aeae855..65fff11 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -252,9 +252,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
int pos;
if (show_only || verbose)
printf(_("Renaming %s to %s\n"), src, dst);
- if (!show_only && mode != INDEX) {
- if (rename(src, dst) < 0 && !ignore_errors)
+ if (!show_only) {
+ if (mode != INDEX &&
+ rename(src, dst) < 0 &&
+ !ignore_errors)
die_errno(_("renaming '%s' failed"), src);
+
if (submodule_gitfile[i]) {
if (submodule_gitfile[i] !=
SUBMODULE_WITH_GITDIR)
connect_work_tree_and_git_dir(dst,
submodule_gitfile[i]);
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 4008fae..4a2570e 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -292,6 +292,9 @@ test_expect_success 'setup submodule' '
echo content >file &&
git add file &&
git commit -m "added sub and file" &&
+ mkdir -p deep/directory/hierachy &&
+ git submodule add ./. deep/directory/hierachy/sub &&
+ git commit -m "added another submodule" &&
git branch submodule
'
@@ -475,4 +478,17 @@ test_expect_success 'mv -k does not accidentally destroy
submodules' '
git checkout .
'
+test_expect_success 'moving a submodule in nested directories' '
+ (
+ cd deep &&
+ git mv directory ../ &&
+ # git status would fail if the update of linking git dir to
+ # work dir of the submodule failed.
+ git status &&
+ git config -f ../.gitmodules
submodule.deep/directory/hierachy/sub.path >../actual &&
+ echo "directory/hierachy/sub" >../expect
+ ) &&
+ test_cmp actual expect
+'
+
test_done
--
2.8.0.26.gba39a1b.dirty
--
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: Binary grep t7008 known breakage vanished on Cygwin
On 18/04/16 16:21, Adam Dinwoodie wrote: > t7008.12 is marked as an expected failure, but building Git on Cygwin > including a `make configure && ./configure` step has the test > unexpectedly passing. Building without the configure step has the test > failing as expected. > > This appears to be behaviour specific to Cygwin; at least I get that > test failing on my CentOS box regardless of whether I perform the > configure step. Yes, the configure sets NO_REGEX= whereas the config.mak.uname sets NO_REGEX=UnfortunatelyYes. [Note that the regex bug (see t0070-fundamental.sh test #5) now seems to pass with the 'native' regex library] 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 v6 3/6] verify-tag: change variable name for readability
On Sun, Apr 17, 2016 at 6:26 PM, wrote:
> The run_gpg_verify() function has two variables, size and len.
>
> This may come off as confusing when reading the code. Clarify which
> one pertains to the length of the tag headers by renaming len to
> payload_length.
The commit message talks about 'payload_length', but the code names it
'payload_size'.
> Signed-off-by: Santiago Torres
> ---
> builtin/verify-tag.c | 11 ++-
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 77f070a..010353c 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -21,20 +21,21 @@ static const char * const verify_tag_usage[] = {
> static int run_gpg_verify(const char *buf, unsigned long size, unsigned
> flags)
> {
> struct signature_check sigc;
> - int len;
> + int payload_size;
> int ret;
>
> memset(&sigc, 0, sizeof(sigc));
>
> - len = parse_signature(buf, size);
> + payload_size = parse_signature(buf, size);
>
> - if (size == len) {
> + if (size == payload_size) {
> if (flags & GPG_VERIFY_VERBOSE)
> - write_in_full(1, buf, len);
> + write_in_full(1, buf, payload_size);
> return error("no signature found");
> }
>
> - ret = check_signature(buf, len, buf + len, size - len, &sigc);
> + ret = check_signature(buf, payload_size, buf + payload_size,
> + size - payload_size, &sigc);
> print_signature_buffer(&sigc, flags);
>
> signature_check_clear(&sigc);
> --
--
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 0/7] fix checking out a being-rebased branch
Nguyễn Thái Ngọc Duy writes: > First cut. I need opinions on 05/07, which converts > wt_status_get_state() to support selecting any worktree. I'm not super > happy with leaving "TODO: not supported yet" comments, even though > strictly speaking this series does not need it. It is a reasonable idea to hook this to wt_status_get_state(), I would say. > Another option is leave wt_status_get_state() alone, factor out the > rebase-detection code and use that for worktree/checkout. We save a > few syscalls this way too. > > Comments? > > [01/07] path.c: add git_common_path() and strbuf_git_common_path() > [02/07] worktree.c: store "id" instead of "git_dir" > [03/07] path.c: refactor and add worktree_git_path() > [04/07] worktree.c: add worktree_git_path_..._head() I actually wondered how ky/branch-[dm]-worktree topics to avoid "branch -d" and "branch -m" from removing or renaming a branch that is checked out in other worktrees from screwing them up. There may be a missed opportunity to clean them up with using these? > [05/07] wt-status.c: make wt_status_get_state() support worktree > [06/07] worktree.c: avoid referencing to worktrees[i] multiple times > [07/07] checkout: prevent checking out a branch being rebased in another > worktree > > Total 6 files changed, 167 insertions(+), 38 deletions(-) -- 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 v6 4/6] verify-tag: add sha1 argument to verify_tag()
On Sun, Apr 17, 2016 at 6:26 PM, wrote:
> verify-tag: add sha1 argument to verify_tag()
Mentioned previously[1]: This subject is talking about low level
details of the change rather than giving a high-level overview. A
suggested replacement[1] would be:
verify-tag: prepare verify_tag() for libification
> The current interface of verify_tag() resolves reference names to SHA1,
> which might be redundant as future callers may resolve the refname to
> SHA1 beforehand.
There is no mention here that the plan is to libify verify_tag() and
"might be redundant" is a somewhat weak way to argue in favor of this
change. The commit messages proposed in the previous review[1] was
more explicit:
verify_tag() accepts a tag name which it resolves to a SHA1
before verification, however, the plan is to make this
functionality public and it is possible that future callers will
already have a SHA1 in hand. Since it would be wasteful for them
to supply a tag name only to have it resolved again, change
verify_tag() to accept a tag SHA1 rather than a name.
> Add a SHA1 parameter to use instead of the name parameter. We also
> replace the name argument to report_name and use it for error reporting
> only.
The patch itself looks okay, though see a few nits below (which may
not be worth a re-roll).
[1]: http://article.gmane.org/gmane.comp.version-control.git/290829
> Signed-off-by: Santiago Torres
> ---
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> @@ -80,6 +79,8 @@ int cmd_verify_tag(int argc, const char **argv, const char
> *prefix)
> {
> int i = 1, verbose = 0, had_error = 0;
> unsigned flags = 0;
> + unsigned char sha1[20];
> + const char *name;
Nit: These could have been declared in the scope of the while-loop
(below) since you've added braces to it.
> @@ -96,8 +97,14 @@ int cmd_verify_tag(int argc, const char **argv, const char
> *prefix)
> - while (i < argc)
> - if (verify_tag(argv[i++], flags))
> + while (i < argc) {
> + name = argv[i++];
> + if (get_sha1(name, sha1)) {
> + error("tag '%s' not found.", name);
> had_error = 1;
These lines could be combined:
had_error = !!error("tag '%s' not found.", name);
which would allow you to drop the braces.
> + }
> + else if (verify_tag(sha1, name, flags))
> + had_error = 1;
Style: cuddle '}' and else:
} else
> + }
> return had_error;
> }
--
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 v6 6/6] tag -v: verfy directly rather than exec-ing verify-tag
On Sun, Apr 17, 2016 at 6:27 PM, wrote:
> tag -v forks into verify-tag, which only calls gpg_verify_tag().
"forks into" sounds odd.
> Instead of forking to verify-tag, call gpg_verify_tag directly().
s/ directly()/() directly/
I found the commit message of your previous version[1] more
descriptive and easier to understand (minus the grammo):
Instead of running the verify-tag plumbing command, use the
gpg_verify_tag() function to avoid doing an additional fork call.
The patch itself looks fine.
[1]: http://article.gmane.org/gmane.comp.version-control.git/290831
> Signed-off-by: Santiago Torres
> ---
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..7b2918e 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -104,13 +104,7 @@ static int delete_tag(const char *name, const char *ref,
> static int verify_tag(const char *name, const char *ref,
> const unsigned char *sha1)
> {
> - const char *argv_verify_tag[] = {"verify-tag",
> - "-v", "SHA1_HEX", NULL};
> - argv_verify_tag[2] = sha1_to_hex(sha1);
> -
> - if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
> - return error(_("could not verify the tag '%s'"), name);
> - return 0;
> + return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
> }
>
> static int do_sign(struct strbuf *buffer)
> --
--
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 v6 0/6] Move PGP verification out of verify-tag
On Sun, Apr 17, 2016 at 6:26 PM, wrote: > This is a follow up of [1], [2], [3], [4], and [5]. patches 1/6, 2/6 and 3/6 > are the same as the corresponding commits in pu. > > v6: > * As Junio suggested, updated 4/6, to include the name argument and the >ternary operator to provide more descriptive error messages. I propagated >these changes to 5/6 and 6/6 as well. I'm unsure about the 80-column >on 4/6, the ternary operator is rather long. > * Updated and reviewed the commit messages based on Eric and Junio's >feedback Thanks. See my responses to individual patches for a few very minor issues, not necessarily even deserving a re-roll. With or without the code and commit message nits addressed, this version is: Reviewed-by: Eric Sunshine -- 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/RFC 1/6] http-backend: use argv_array functions
David Turner writes:
> Signed-off-by: David Turner
> ---
OK (it might be easier to read if you used the pushl form for the
"fixed initial segment" like these calls, though).
> http-backend.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/http-backend.c b/http-backend.c
> index 8870a26..a4f0066 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -450,9 +450,7 @@ static void get_info_refs(char *arg)
> hdr_nocache();
>
> if (service_name) {
> - const char *argv[] = {NULL /* service name */,
> - "--stateless-rpc", "--advertise-refs",
> - ".", NULL};
> + struct argv_array argv = ARGV_ARRAY_INIT;
> struct rpc_service *svc = select_service(service_name);
>
> strbuf_addf(&buf, "application/x-git-%s-advertisement",
> @@ -463,9 +461,13 @@ static void get_info_refs(char *arg)
> packet_write(1, "# service=git-%s\n", svc->name);
> packet_flush(1);
>
> - argv[0] = svc->name;
> - run_service(argv, 0);
> + argv_array_push(&argv, svc->name);
> + argv_array_push(&argv, "--stateless-rpc");
> + argv_array_push(&argv, "--advertise-refs");
>
> + argv_array_push(&argv, ".");
> + run_service(argv.argv, 0);
> + argv_array_clear(&argv);
> } else {
> select_getanyfile();
> for_each_namespaced_ref(show_text_ref, &buf);
--
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/RFC 2/6] remote-curl.c: fix variable shadowing
David Turner writes:
> The local variable 'options' was shadowing a global of the same name.
>
> Signed-off-by: David Turner
> ---
OK. In general, giving a longer and more descriptive name to the
global would be a direction to lead to more readable code, though.
> remote-curl.c | 18 +-
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 15e48e2..b9b6a90 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -254,7 +254,7 @@ static struct discovery *discover_refs(const char
> *service, int for_push)
> struct strbuf effective_url = STRBUF_INIT;
> struct discovery *last = last_discovery;
> int http_ret, maybe_smart = 0;
> - struct http_get_options options;
> + struct http_get_options get_options;
>
> if (last && !strcmp(service, last->service))
> return last;
> @@ -271,15 +271,15 @@ static struct discovery *discover_refs(const char
> *service, int for_push)
> strbuf_addf(&refs_url, "service=%s", service);
> }
>
> - memset(&options, 0, sizeof(options));
> - options.content_type = &type;
> - options.charset = &charset;
> - options.effective_url = &effective_url;
> - options.base_url = &url;
> - options.no_cache = 1;
> - options.keep_error = 1;
> + memset(&get_options, 0, sizeof(get_options));
> + get_options.content_type = &type;
> + get_options.charset = &charset;
> + get_options.effective_url = &effective_url;
> + get_options.base_url = &url;
> + get_options.no_cache = 1;
> + get_options.keep_error = 1;
>
> - http_ret = http_get_strbuf(refs_url.buf, &buffer, &options);
> + http_ret = http_get_strbuf(refs_url.buf, &buffer, &get_options);
> switch (http_ret) {
> case HTTP_OK:
> break;
--
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/RFC 4/6] transport: add refspec list parameters to functions
David Turner writes: > Add parameters for a list of refspecs to transport_get_remote_refs and > get_refs_list. These parameters are presently unused -- soon, we will > use them to implement fetches which only learn about a subset of refs. > > Signed-off-by: David Turner > --- What the code tries to do I am more than halfway happy. It is unfortunate that we cannot do this natively without upgrading the protocol in a fundamental way, but this is a nice way to work it around only for Git-over-HTTP transport without having to break the protocol. As a POC it is OK, but I am moderately unhappy with the use of "refspec" here. At the transport layer, we shouldn't care what the receiving end intends to do with the objects that sits at the tip of the refs at the other end, so sending "refspecs" down feels somewhat wrong for this feature. At one layer up in the next patch, you do use "interesting refs" which makes it clear that only the left-hand-side of a refspec, i.e. what they call it, matters, and I think that is a much better phrasing of the concept (and the passed data should only be the left-hand-side of refspecs). 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 2/2] xdiff: implement empty line chunk heuristic
Jacob Keller writes:
> I think we're going to make use of xdl_blankline instead of this or
> our own "is_emptyline"
OK, so perhaps either of you two can do a final version people can
start having fun with?
By the way, I really do not want to see something this low-level to
be end-user tweakable with "one bit enable/disable"; the end users
shouldn't have to bother [1]. I left it in but renamed after "what"
it enables/disables, not "how" the enabled thing works, to clarify
that we have this only as a developers' aid.
*1* I am fine with --compaction-heuristic=(shortest|blank|...) that
allows a choice among many as a developers' aid, but I do not think
this topic is there yet.
Documentation/diff-config.txt | 9 -
Documentation/diff-options.txt | 10 +-
diff.c | 18 +-
xdiff/xdiff.h | 2 +-
xdiff/xdiffi.c | 22 ++
5 files changed, 29 insertions(+), 32 deletions(-)
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index c62745b..9bf3e92 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -166,11 +166,10 @@ diff.tool::
include::mergetools-diff.txt[]
-diff.shortestLineHeuristic::
- Set this option to true to enable the shortest line chunk heuristic when
- producing diff output. This heuristic will attempt to shift hunks such
- that the last shortest common line occurs below the hunk with the rest
of
- the context above it.
+diff.compactionHeuristic::
+ Set this option to enable an experimental heuristic that
+ shifts the hunk boundary in an attempt to make the resulting
+ patch easier to read.
diff.algorithm::
Choose a diff algorithm. The variants are as follows:
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 238f39c..b513023 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -63,11 +63,11 @@ ifndef::git-format-patch[]
Synonym for `-p --raw`.
endif::git-format-patch[]
---shortest-line-heuristic::
---no-shortest-line-heuristic::
- When possible, shift common shortest line in diff hunks below the hunk
- such that the last common shortest line for each hunk is below, with the
- rest of the context above the hunk.
+--compaction-heuristic::
+--no-compaction-heuristic::
+ These are to help debugging and tuning an experimental
+ heuristic that shifts the hunk boundary in an attempt to
+ make the resulting patch easier to read.
--minimal::
Spend extra time to make sure the smallest possible
diff --git a/diff.c b/diff.c
index 276174c..02c75c3 100644
--- a/diff.c
+++ b/diff.c
@@ -25,7 +25,7 @@
#endif
static int diff_detect_rename_default;
-static int diff_shortest_line_heuristic = 0;
+static int diff_compaction_heuristic = 1;
static int diff_rename_limit_default = 400;
static int diff_suppress_blank_empty;
static int diff_use_color_default = -1;
@@ -184,8 +184,8 @@ int git_diff_ui_config(const char *var, const char *value,
void *cb)
diff_detect_rename_default = git_config_rename(var, value);
return 0;
}
- if (!strcmp(var, "diff.shortestlineheuristic")) {
- diff_shortest_line_heuristic = git_config_bool(var, value);
+ if (!strcmp(var, "diff.compactionheuristic")) {
+ diff_compaction_heuristic = git_config_bool(var, value);
return 0;
}
if (!strcmp(var, "diff.autorefreshindex")) {
@@ -3240,8 +3240,8 @@ void diff_setup(struct diff_options *options)
options->use_color = diff_use_color_default;
options->detect_rename = diff_detect_rename_default;
options->xdl_opts |= diff_algorithm;
- if (diff_shortest_line_heuristic)
- DIFF_XDL_SET(options, SHORTEST_LINE_HEURISTIC);
+ if (diff_compaction_heuristic)
+ DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
options->orderfile = diff_order_file_cfg;
@@ -3719,10 +3719,10 @@ int diff_opt_parse(struct diff_options *options, const
char **av, int ac)
DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
else if (!strcmp(arg, "--ignore-blank-lines"))
DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
- else if (!strcmp(arg, "--shortest-line-heuristic"))
- DIFF_XDL_SET(options, SHORTEST_LINE_HEURISTIC);
- else if (!strcmp(arg, "--no-shortest-line-heuristic"))
- DIFF_XDL_CLR(options, SHORTEST_LINE_HEURISTIC);
+ else if (!strcmp(arg, "--compaction-heuristic"))
+ DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
+ else if (!strcmp(arg, "--no-compaction-heuristic"))
+ DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
else if (!strcmp(arg, "--patience"))
options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
else if (!strcmp
Re: [PATCH 0/2] Another reroll of sb/submodule-init
Stefan Beller writes:
> * squashed the fixes from Johannes Sixt to unbreak Windows tests.
> (I had encoding issues; so I manually integrated the changes)
> * fixed memleaks
> * the base to apply did not change (ee30f17805f51d37 Merge branch
> 'sb/submodule-path-misc-bugs' into sb/submodule-init)
>
> Thanks,
> Stefan
Queued. I read it twice and I think this is ready for 'next'.
Comments from others?
>
> diff to current origin/sb/submodule-init:
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ad3cba6..b6d4f27 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -309,8 +309,7 @@ static void init_submodule(const char *path, const char
> *prefix, int quiet)
> {
> const struct submodule *sub;
> struct strbuf sb = STRBUF_INIT;
> - const char *upd = NULL;
> - char *url = NULL, *displaypath;
> + char *upd = NULL, *url = NULL, *displaypath;
>
> /* Only loads from .gitmodules, no overlay with .git/config */
> gitmodules_config();
> @@ -340,7 +339,7 @@ static void init_submodule(const char *path, const char
> *prefix, int quiet)
> /* Possibly a url relative to parent */
> if (starts_with_dot_dot_slash(url) ||
> starts_with_dot_slash(url)) {
> - char *remoteurl;
> + char *remoteurl, *relurl;
> char *remote = get_default_remote();
> struct strbuf remotesb = STRBUF_INIT;
> strbuf_addf(&remotesb, "remote.%s.url", remote);
> @@ -352,9 +351,11 @@ static void init_submodule(const char *path, const char
> *prefix, int quiet)
>* authoritative upstream
>*/
> remoteurl = xgetcwd();
> - url = relative_url(remoteurl, url, NULL);
> + relurl = relative_url(remoteurl, url, NULL);
> strbuf_release(&remotesb);
> free(remoteurl);
> + free(url);
> + url = relurl;
> }
>
> if (git_config_set_gently(sb.buf, url))
> @@ -368,14 +369,14 @@ static void init_submodule(const char *path, const char
> *prefix, int quiet)
> /* Copy "update" setting when it is not set yet */
> strbuf_reset(&sb);
> strbuf_addf(&sb, "submodule.%s.update", sub->name);
> - if (git_config_get_string_const(sb.buf, &upd) &&
> + if (git_config_get_string(sb.buf, &upd) &&
> sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
> if (sub->update_strategy.type == SM_UPDATE_COMMAND) {
> fprintf(stderr, _("warning: command update mode
> suggested for submodule '%s'\n"),
> sub->name);
> - upd = "none";
> + upd = xstrdup("none");
> } else
> - upd =
> submodule_strategy_to_string(&sub->update_strategy);
> + upd =
> xstrdup(submodule_strategy_to_string(&sub->update_strategy));
>
> if (git_config_set_gently(sb.buf, upd))
> die(_("Failed to register update mode for submodule
> path '%s'"), displaypath);
> @@ -383,6 +384,7 @@ static void init_submodule(const char *path, const char
> *prefix, int quiet)
> strbuf_release(&sb);
> free(displaypath);
> free(url);
> + free(upd);
> }
>
> static int module_init(int argc, const char **argv, const char *prefix)
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index 2e62548..bf2deee 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -305,13 +305,16 @@ test_git_path GIT_COMMON_DIR=bar config
> bar/config
> test_git_path GIT_COMMON_DIR=bar packed-refs bar/packed-refs
> test_git_path GIT_COMMON_DIR=bar shallow bar/shallow
>
> +# In the tests below, the distinction between $PWD and $(pwd) is important:
> +# on Windows, $PWD is POSIX style (/c/foo), $(pwd) has drive letter (c:/foo).
> +
> test_submodule_relative_url "../" "../foo" "../submodule" "../../submodule"
> test_submodule_relative_url "../" "../foo/bar" "../submodule"
> "../../foo/submodule"
> test_submodule_relative_url "../" "../foo/submodule" "../submodule"
> "../../foo/submodule"
> test_submodule_relative_url "../" "./foo" "../submodule" "../submodule"
> test_submodule_relative_url "../" "./foo/bar" "../submodule"
> "../foo/submodule"
> test_submodule_relative_url "../../../" "../foo/bar" "../sub/a/b/c"
> "../../../../foo/sub/a/b/c"
> -test_submodule_relative_url "../" "$PWD/addtest" "../repo" "$PWD/repo"
> +test_submodule_relative_url "../" "$PWD/addtest" "../repo" "$(pwd)/repo"
> test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
> test_submodule_relative_url "../" "foo" "../submodule" "../submodu
Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic
On Mon, Apr 18, 2016 at 12:22 PM, Junio C Hamano wrote: > Jacob Keller writes: > >> I think we're going to make use of xdl_blankline instead of this or >> our own "is_emptyline" > > OK, so perhaps either of you two can do a final version people can > start having fun with? Junios proposal seems to be on top of my latest series sent out, I'll squash it in and send it out as a final version if you don't mind (though I'll do it later today; currently diving into Gerrits Java) > > By the way, I really do not want to see something this low-level to > be end-user tweakable with "one bit enable/disable"; the end users > shouldn't have to bother [1]. Ok. Thanks for fixing that mistake. > I left it in but renamed after "what" > it enables/disables, not "how" the enabled thing works, to clarify > that we have this only as a developers' aid. > > *1* I am fine with --compaction-heuristic=(shortest|blank|...) that > allows a choice among many as a developers' aid, but I do not think > this topic is there yet. This doesn't bode well with > +--compaction-heuristic:: > +--no-compaction-heuristic:: in the future? I'd rather have +--compaction-heuristic=none +--compaction-heuristic=lastEmptyLine such that we don't have to worry about further experiments (or matured heuristics) later? -- 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 v6 0/6] Move PGP verification out of verify-tag
santi...@nyu.edu writes:
>I'm unsure about the 80-column
>on 4/6, the ternary operator is rather long.
You can do something like this:
type = sha1_object_info(sha1, NULL);
if (type != OBJ_TAG)
return error("%s: cannot verify a non-tag object of type %s.",
report_name ?
report_name :
find_unique_abbrev(sha1, DEFAULT_ABBREV),
typename(type));
buf = read_sha1_file(sha1, &type, &size);
if (!buf)
return error("%s: unable to read file.",
report_name ?
report_name :
find_unique_abbrev(sha1, DEFAULT_ABBREV));
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 v6 4/6] verify-tag: add sha1 argument to verify_tag()
Eric Sunshine writes:
> On Sun, Apr 17, 2016 at 6:26 PM, wrote:
>> verify-tag: add sha1 argument to verify_tag()
>
> Mentioned previously[1]: This subject is talking about low level
> details of the change rather than giving a high-level overview. A
> suggested replacement[1] would be:
>
> verify-tag: prepare verify_tag() for libification
>
>> The current interface of verify_tag() resolves reference names to SHA1,
>> which might be redundant as future callers may resolve the refname to
>> SHA1 beforehand.
>
> There is no mention here that the plan is to libify verify_tag() and
> "might be redundant" is a somewhat weak way to argue in favor of this
> change. The commit messages proposed in the previous review[1] was
> more explicit:
>
> verify_tag() accepts a tag name which it resolves to a SHA1
> before verification, however, the plan is to make this
> functionality public and it is possible that future callers will
> already have a SHA1 in hand. Since it would be wasteful for them
> to supply a tag name only to have it resolved again, change
> verify_tag() to accept a tag SHA1 rather than a name.
Phrased that way, it is not "might be redundant" that this change is
fixing, and "It is possible ... will already have" is still weak. I
think the real reason for this change is that some future callers
may only have object name without the end-user supplied textual
input, and the current interface that takes strings is cumbersome
for them to use--they have to use sha1_to_hex() only so that the
callee can do get_sha1() on it.
I guess with 's/already/only/', your version is good.
By the way, it can also be read in a negative way: "Some current
callers may let this function resolve tagname, but they no longer
can rely on it, as we force them to resolve before we allow them to
call this function."
>> Add a SHA1 parameter to use instead of the name parameter. We also
>> replace the name argument to report_name and use it for error reporting
>> only.
I know I am to blame, but perhaps "reported_name" or
"name_to_report"? "report_name" sounds as if it is a boolean that
tells the function to report the name of the tag (as opposed to stay
silent).
I agree all the clean-up points in the code part of your review.
Thanks.
> The patch itself looks okay, though see a few nits below (which may
> not be worth a re-roll).
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/290829
>
>> Signed-off-by: Santiago Torres
>> ---
>> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
>> @@ -80,6 +79,8 @@ int cmd_verify_tag(int argc, const char **argv, const char
>> *prefix)
>> {
>> int i = 1, verbose = 0, had_error = 0;
>> unsigned flags = 0;
>> + unsigned char sha1[20];
>> + const char *name;
>
> Nit: These could have been declared in the scope of the while-loop
> (below) since you've added braces to it.
>
>> @@ -96,8 +97,14 @@ int cmd_verify_tag(int argc, const char **argv, const
>> char *prefix)
>> - while (i < argc)
>> - if (verify_tag(argv[i++], flags))
>> + while (i < argc) {
>> + name = argv[i++];
>> + if (get_sha1(name, sha1)) {
>> + error("tag '%s' not found.", name);
>> had_error = 1;
>
> These lines could be combined:
>
> had_error = !!error("tag '%s' not found.", name);
>
> which would allow you to drop the braces.
>
>> + }
>> + else if (verify_tag(sha1, name, flags))
>> + had_error = 1;
>
> Style: cuddle '}' and else:
>
> } else
>
>> + }
>> return had_error;
>> }
--
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] mv: allow moving nested submodules
Stefan Beller writes:
> if (show_only || verbose)
> printf(_("Renaming %s to %s\n"), src, dst);
> - if (!show_only && mode != INDEX) {
> - if (rename(src, dst) < 0 && !ignore_errors)
> + if (!show_only) {
> + if (mode != INDEX &&
> + rename(src, dst) < 0 &&
> + !ignore_errors)
> die_errno(_("renaming '%s' failed"), src);
> +
If ignore-errors is set and rename fails, this would fall through
and try to touch this codepath...
> if (submodule_gitfile[i]) {
> if (submodule_gitfile[i] !=
> SUBMODULE_WITH_GITDIR)
> connect_work_tree_and_git_dir(dst,
> submodule_gitfile[i]);
... but I am not sure if this thing is prepared to cope with such a
case? src should have been moved to dst but if rename() failed we
wouldn't see what we expect at dst, or would we?
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 4008fae..4a2570e 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -292,6 +292,9 @@ test_expect_success 'setup submodule' '
> echo content >file &&
> git add file &&
> git commit -m "added sub and file" &&
> + mkdir -p deep/directory/hierachy &&
> + git submodule add ./. deep/directory/hierachy/sub &&
> + git commit -m "added another submodule" &&
> git branch submodule
> '
>
> @@ -475,4 +478,17 @@ test_expect_success 'mv -k does not accidentally destroy
> submodules' '
> git checkout .
> '
>
> +test_expect_success 'moving a submodule in nested directories' '
> + (
> + cd deep &&
> + git mv directory ../ &&
> + # git status would fail if the update of linking git dir to
> + # work dir of the submodule failed.
> + git status &&
> + git config -f ../.gitmodules
> submodule.deep/directory/hierachy/sub.path >../actual &&
> + echo "directory/hierachy/sub" >../expect
> + ) &&
> + test_cmp actual expect
> +'
> +
> test_done
--
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 2/2] xdiff: implement empty line chunk heuristic
In order to produce the smallest possible diff and combine several diff
hunks together, we implement a heuristic from GNU Diff which moves diff
hunks forward as far as possible when we find common context above and
below a diff hunk. This sometimes produces less readable diffs when
writing C, Shell, or other programming languages, ie:
...
/*
+ *
+ *
+ */
+
+/*
...
instead of the more readable equivalent of
...
+/*
+ *
+ *
+ */
+
/*
...
Implement the following heuristic to (optionally) produce the desired
output.
If there are diff chunks which can be shifted around, shift each hunk
such that the last common empty line is below the chunk with the rest
of the context above.
This heuristic appears to resolve the above example and several other
common issues without producing significantly weird results. However, as
with any heuristic it is not really known whether this will always be
more optimal. Thus, it can be disabled via diff.compactionHeuristic.
Signed-off-by: Stefan Beller
Signed-off-by: Jacob Keller
Signed-off-by: Stefan Beller
---
Documentation/diff-config.txt | 5 +
Documentation/diff-options.txt | 6 ++
diff.c | 11 +++
xdiff/xdiff.h | 2 ++
xdiff/xdiffi.c | 26 ++
5 files changed, 50 insertions(+)
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index edba565..a9f4b57 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -170,6 +170,11 @@ diff.tool::
include::mergetools-diff.txt[]
+diff.compactionHeuristic::
+ Set this option to enable an experimental heuristic that
+ shifts the hunk boundary in an attempt to make the resulting
+ patch easier to read.
+
diff.algorithm::
Choose a diff algorithm. The variants are as follows:
+
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 4b0318e..0993742 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -63,6 +63,12 @@ ifndef::git-format-patch[]
Synonym for `-p --raw`.
endif::git-format-patch[]
+--compaction-heuristic::
+--no-compaction-heuristic::
+ These are to help debugging and tuning an experimental
+ heuristic that shifts the hunk boundary in an attempt to
+ make the resulting patch easier to read.
+
--minimal::
Spend extra time to make sure the smallest possible
diff is produced.
diff --git a/diff.c b/diff.c
index 4dfe660..d3734d3 100644
--- a/diff.c
+++ b/diff.c
@@ -26,6 +26,7 @@
#endif
static int diff_detect_rename_default;
+static int diff_compaction_heuristic = 1;
static int diff_rename_limit_default = 400;
static int diff_suppress_blank_empty;
static int diff_use_color_default = -1;
@@ -189,6 +190,10 @@ int git_diff_ui_config(const char *var, const char *value,
void *cb)
diff_detect_rename_default = git_config_rename(var, value);
return 0;
}
+ if (!strcmp(var, "diff.compactionheuristic")) {
+ diff_compaction_heuristic = git_config_bool(var, value);
+ return 0;
+ }
if (!strcmp(var, "diff.autorefreshindex")) {
diff_auto_refresh_index = git_config_bool(var, value);
return 0;
@@ -3278,6 +3283,8 @@ void diff_setup(struct diff_options *options)
options->use_color = diff_use_color_default;
options->detect_rename = diff_detect_rename_default;
options->xdl_opts |= diff_algorithm;
+ if (diff_compaction_heuristic)
+ DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
options->orderfile = diff_order_file_cfg;
@@ -3798,6 +3805,10 @@ int diff_opt_parse(struct diff_options *options,
DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
else if (!strcmp(arg, "--ignore-blank-lines"))
DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
+ else if (!strcmp(arg, "--compaction-heuristic"))
+ DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
+ else if (!strcmp(arg, "--no-compaction-heuristic"))
+ DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
else if (!strcmp(arg, "--patience"))
options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
else if (!strcmp(arg, "--histogram"))
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 4fb7e79..7423f77 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -41,6 +41,8 @@ extern "C" {
#define XDF_IGNORE_BLANK_LINES (1 << 7)
+#define XDF_COMPACTION_HEURISTIC (1 << 8)
+
#define XDL_EMIT_FUNCNAMES (1 << 0)
#define XDL_EMIT_FUNCCONTEXT (1 << 2)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 748eeb9..5a02b15 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -400,6 +400,11 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long
i1, long i2, long chg1,
}
+static int is_blank_line(xrecord_t **recs, long ix, long flags)
+{
+ return xdl_blankl
[PATCH 0/2 v4] xdiff: implement empty line chunk heuristic
> OK, so perhaps either of you two can do a final version people can
> start having fun with?
Here we go. I squashed in your patch, although with a minor change:
- if ((flags & XDF_SHORTEST_LINE_HEURISTIC)) {
+ if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
We did not need that in the "shortest line" heuristic as we know
a line with the shortest line length must exist. We do not know about
empty lines though.
Thanks,
Stefan
Jacob Keller (1):
xdiff: add recs_match helper function
Stefan Beller (1):
xdiff: implement empty line chunk heuristic
Documentation/diff-config.txt | 5 +
Documentation/diff-options.txt | 6 ++
diff.c | 11 +++
xdiff/xdiff.h | 2 ++
xdiff/xdiffi.c | 40
5 files changed, 60 insertions(+), 4 deletions(-)
--
2.8.0.26.gba39a1b.dirty
--
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 1/2] xdiff: add recs_match helper function
From: Jacob Keller
It is a common pattern in xdl_change_compact to check that hashes and
strings match. The resulting code to perform this change causes very
long lines and makes it hard to follow the intention. Introduce a helper
function recs_match which performs both checks to increase
code readability.
Signed-off-by: Jacob Keller
Signed-off-by: Stefan Beller
---
xdiff/xdiffi.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 2358a2d..748eeb9 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -400,6 +400,14 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long
i1, long i2, long chg1,
}
+static int recs_match(xrecord_t **recs, long ixs, long ix, long flags)
+{
+ return (recs[ixs]->ha == recs[ix]->ha &&
+ xdl_recmatch(recs[ixs]->ptr, recs[ixs]->size,
+recs[ix]->ptr, recs[ix]->size,
+flags));
+}
+
int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
long ix, ixo, ixs, ixref, grpsiz, nrec = xdf->nrec;
char *rchg = xdf->rchg, *rchgo = xdfo->rchg;
@@ -442,8 +450,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long
flags) {
* the last line of the current change group, shift
backward
* the group.
*/
- while (ixs > 0 && recs[ixs - 1]->ha == recs[ix - 1]->ha
&&
- xdl_recmatch(recs[ixs - 1]->ptr, recs[ixs -
1]->size, recs[ix - 1]->ptr, recs[ix - 1]->size, flags)) {
+ while (ixs > 0 && recs_match(recs, ixs - 1, ix - 1,
flags)) {
rchg[--ixs] = 1;
rchg[--ix] = 0;
@@ -470,8 +477,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long
flags) {
* the line next of the current change group, shift
forward
* the group.
*/
- while (ix < nrec && recs[ixs]->ha == recs[ix]->ha &&
- xdl_recmatch(recs[ixs]->ptr, recs[ixs]->size,
recs[ix]->ptr, recs[ix]->size, flags)) {
+ while (ix < nrec && recs_match(recs, ixs, ix, flags)) {
rchg[ixs++] = 0;
rchg[ix++] = 1;
--
2.8.0.26.gba39a1b.dirty
--
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] mv: allow moving nested submodules
Junio C Hamano writes:
> If ignore-errors is set and rename fails, this would fall through
> and try to touch this codepath...
>
>> if (submodule_gitfile[i]) {
>> if (submodule_gitfile[i] !=
>> SUBMODULE_WITH_GITDIR)
>> connect_work_tree_and_git_dir(dst,
>> submodule_gitfile[i]);
>
> ... but I am not sure if this thing is prepared to cope with such a
> case? src should have been moved to dst but if rename() failed we
> wouldn't see what we expect at dst, or would we?
In other words, I was wondering if this part should read more like
this:
builtin/mv.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index aeae855..37ed0fc 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -252,9 +252,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
int pos;
if (show_only || verbose)
printf(_("Renaming %s to %s\n"), src, dst);
- if (!show_only && mode != INDEX) {
- if (rename(src, dst) < 0 && !ignore_errors)
+ if (show_only)
+ ;
+ else {
+ if (mode != INDEX && rename(src, dst) < 0) {
+ if (ignore_errors)
+ continue;
die_errno(_("renaming '%s' failed"), src);
+ }
if (submodule_gitfile[i]) {
if (submodule_gitfile[i] !=
SUBMODULE_WITH_GITDIR)
connect_work_tree_and_git_dir(dst,
submodule_gitfile[i]);
--
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 0/2 v4] xdiff: implement empty line chunk heuristic
Stefan Beller writes:
>> OK, so perhaps either of you two can do a final version people can
>> start having fun with?
>
> Here we go. I squashed in your patch, although with a minor change:
>
> - if ((flags & XDF_SHORTEST_LINE_HEURISTIC)) {
> + if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
>
> We did not need that in the "shortest line" heuristic as we know
> a line with the shortest line length must exist. We do not know about
> empty lines though.
Makes sense. The last hunk of
$ git show 9614b8dcf -- update-cache.c
gives an unexpected result without "&& blank_lines" above. Lack of
"&& blank_lines" happens to make the result slightly easier to read,
but at the cost of having an extra line in the hunk.
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] mv: allow moving nested submodules
On Mon, Apr 18, 2016 at 2:13 PM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>> If ignore-errors is set and rename fails, this would fall through
>> and try to touch this codepath...
>>
>>> if (submodule_gitfile[i]) {
>>> if (submodule_gitfile[i] !=
>>> SUBMODULE_WITH_GITDIR)
>>> connect_work_tree_and_git_dir(dst,
>>> submodule_gitfile[i]);
>>
>> ... but I am not sure if this thing is prepared to cope with such a
>> case? src should have been moved to dst but if rename() failed we
>> wouldn't see what we expect at dst, or would we?
>
> In other words, I was wondering if this part should read more like
> this:
>
> builtin/mv.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index aeae855..37ed0fc 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -252,9 +252,14 @@ int cmd_mv(int argc, const char **argv, const char
> *prefix)
> int pos;
> if (show_only || verbose)
> printf(_("Renaming %s to %s\n"), src, dst);
> - if (!show_only && mode != INDEX) {
> - if (rename(src, dst) < 0 && !ignore_errors)
> + if (show_only)
> + ;
> + else {
> + if (mode != INDEX && rename(src, dst) < 0) {
I agree until here.
> + if (ignore_errors)
> + continue;
> die_errno(_("renaming '%s' failed"), src);
This I thought would be better as:
if (!ignore_errors)
die_errno(...);
and not continue, but continuing is the right thing I would expect.
Speaking of which, connect_work_tree_and_git_dir as well as
update_path_in_gitmodules need to learn about the ignore_errors
flag, too. You would expect them to not die at the faintest problem,
but rather honor the promise of "Skip move or rename actions which
would lead to an error condition."
Thanks for a starting pointer for a new patch!
Stefan
> + }
> if (submodule_gitfile[i]) {
> if (submodule_gitfile[i] !=
> SUBMODULE_WITH_GITDIR)
> connect_work_tree_and_git_dir(dst,
> submodule_gitfile[i]);
--
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 2/2] xdiff: implement empty line chunk heuristic
On Mon, Apr 18, 2016 at 2:12 PM, Stefan Beller wrote: > In order to produce the smallest possible diff and combine several diff > hunks together, we implement a heuristic from GNU Diff which moves diff > hunks forward as far as possible when we find common context above and > below a diff hunk. This sometimes produces less readable diffs when > writing C, Shell, or other programming languages, ie: > > ... > /* > + * > + * > + */ > + > +/* > ... > > instead of the more readable equivalent of > > ... > +/* > + * > + * > + */ > + > /* > ... > > Implement the following heuristic to (optionally) produce the desired > output. > > If there are diff chunks which can be shifted around, shift each hunk > such that the last common empty line is below the chunk with the rest > of the context above. > > This heuristic appears to resolve the above example and several other > common issues without producing significantly weird results. However, as > with any heuristic it is not really known whether this will always be > more optimal. Thus, it can be disabled via diff.compactionHeuristic. > > Signed-off-by: Stefan Beller > Signed-off-by: Jacob Keller > Signed-off-by: Stefan Beller > --- Thanks Stephan and Junio, this looks pretty good. I think before it's merged we'd probably want to implement some sort of attributes which allows per-path configuration, incase it needs to be configured at all. I've got it applied to my local git, and I'm going to try to run a diff between enabled vs disabled on a large section of the Linux kernel history and a few other projects to see if I spot anything odd. Thanks, Jake -- 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 2/2] xdiff: implement empty line chunk heuristic
Jacob Keller writes: > Thanks Stephan and Junio, this looks pretty good. I think before it's > merged we'd probably want to implement some sort of attributes which > allows per-path configuration, incase it needs to be configured at > all. My take on it is that we'd want to make sure that the shift with blank line heuristics is "good enough", i.e. there is no need for end-user configuration or attributes, and then remove the tentative option, configuration and its documentation, before this is merged. If we really want to add knobs to handle different kind of payloads in vastly different way, the right place to do so is to add a set of bits "use this and that heuristics" to userdiff driver, I would say, but in the compaction codepath it does not seem to be enough room to have that many knobs to be tweaked in the first place to me. > I've got it applied to my local git, and I'm going to try to run a > diff between enabled vs disabled on a large section of the Linux > kernel history and a few other projects to see if I spot anything odd. 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
What's cooking in git.git (Apr 2016, #05; Mon, 18)
Here are the topics that have been cooking. Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'. The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.
The 'master' branch now has the fifth batch of topics of this cycle.
There are a handful of topics that are stuck; they are marked as
"Needs review", "Needs an Ack", etc. in the following list.
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
--
[Graduated to "master"]
* ak/use-hashmap-iter-first-in-submodule-config (2016-03-23) 1 commit
(merged to 'next' on 2016-04-06 at 2aab890)
+ submodule-config: use hashmap_iter_first()
Minor code cleanup.
* cc/apply (2016-04-01) 4 commits
(merged to 'next' on 2016-04-06 at 2e23c44)
+ builtin/apply: free patch when parse_chunk() fails
+ builtin/apply: handle parse_binary() failure
+ apply: remove unused call to free() in gitdiff_{old,new}name()
+ builtin/apply: get rid of useless 'name' variable
Minor code clean-up.
* ep/trace-doc-sample-fix (2016-04-05) 1 commit
(merged to 'next' on 2016-04-06 at 0df7357)
+ api-trace.txt: fix typo
Fix a typo in an example in the trace API documentation.
* es/format-patch-doc-hide-no-patch (2016-04-04) 1 commit
(merged to 'next' on 2016-04-06 at 25d79bb)
+ git-format-patch.txt: don't show -s as shorthand for multiple options
"git format-patch --help" showed `-s` and `--no-patch` as if these
are valid options to the command. We already hide `--patch` option
from the documentation, because format-patch is about showing the
diff, and the documentation now hides these options as well.
* jc/makefile-redirection-stderr (2016-04-05) 1 commit
(merged to 'next' on 2016-04-06 at e3f2ded)
+ Makefile: fix misdirected redirections
A minor fix in the Makefile.
* jk/branch-shortening-funny-symrefs (2016-04-04) 1 commit
(merged to 'next' on 2016-04-06 at 1a3f8be)
+ branch: fix shortening of non-remote symrefs
A change back in version 2.7 to "git branch" broke display of a
symbolic ref in a non-standard place in the refs/ hierarchy (we
expect symbolic refs to appear in refs/remotes/*/HEAD to point at
the primary branch the remote has, and as .git/HEAD to point at the
branch we locally checked out).
Will merge down to maint-2.7.
* jk/check-repository-format (2016-03-11) 10 commits
(merged to 'next' on 2016-04-06 at a0dada0)
+ verify_repository_format: mark messages for translation
+ setup: drop repository_format_version global
+ setup: unify repository version callbacks
+ init: use setup.c's repo version verification
+ setup: refactor repo format reading and verification
+ config: drop git_config_early
+ check_repository_format_gently: stop using git_config_early
+ lazily load core.sharedrepository
+ wrap shared_repository global in get/set accessors
+ setup: document check_repository_format()
(this branch is used by dt/pre-refs-backend.)
The repository set-up sequence has been streamlined (the biggest
change is that there is no longer git_config_early()), so that we
do not attempt to look into refs/* when we know we do not have a
Git repository.
* jn/mergetools-examdiff (2016-04-04) 2 commits
(merged to 'next' on 2016-04-06 at 819e858)
+ mergetools: add support for ExamDiff
+ mergetools: create mergetool_find_win32_cmd() helper function for winmerge
"git mergetools" learned to drive ExamDiff.
* js/mingw-tests-2.8 (2016-04-04) 1 commit
(merged to 'next' on 2016-04-06 at f85a013)
+ Windows: shorten code by re-using convert_slashes()
Code clean-up.
* kn/for-each-tag-branch (2016-03-30) 1 commit
(merged to 'next' on 2016-04-06 at 4595ad3)
+ for-each-ref: fix description of '--contains' in manpage
A minor documentation update.
* ky/branch-d-worktree (2016-03-29) 1 commit
(merged to 'next' on 2016-04-06 at 00f9bff)
+ branch -d: refuse deleting a branch which is currently checked out
When "git worktree" feature is in use, "git branch -d" allowed
deletion of a branch that is checked out in another worktree
* ky/branch-m-worktree (2016-04-08) 3 commits
(merged to 'next' on 2016-04-08 at b673b5e)
+ set_worktree_head_symref(): fix error message
(merged to 'next' on 2016-04-06 at e7b285c)
+ branch -m: update all per-worktree HEADs
+ refs: add a new function set_worktree_head_symref
When "git worktree" feature is in use, "git branch -m" renamed a
branch that is checked out in another worktree without adjusting
the HEAD symbolic ref for the worktree.
* lt/pretty-expand-tabs (2016-04-04) 4 commits
(merged to 'next' on 2016-04-06 at 186ac2a)
+ pretty: test --expand-tabs
+ pretty: allow tweaking tabwidth in --expand-tabs
+ pretty: enable --expand-tabs by default for selected pretty formats
+ pretty: expand tabs in indented
[PATCH 0/2] WAS: [PATCH] mv: allow moving nested submodules
A single patch evolves into a series. The second patch is essentially a resend with Junios suggestion squashed[1]. That patch alone doesn't quite fix it yet, as we need to make sure the submodule code respects the ignore_errors flag as instructed by the user. Patch 1 libifies the used functions from submodule.c and handles the errors appropriately. [1] http://thread.gmane.org/gmane.comp.version-control.git/291810/focus=291829 Thanks, Stefan Stefan Beller (2): mv submodule: respect ignore_errors for errors in submodule code mv: allow moving nested submodules builtin/mv.c | 32 +++- submodule.c | 33 - submodule.h | 6 -- t/t7001-mv.sh | 16 4 files changed, 67 insertions(+), 20 deletions(-) -- 2.8.1.211.g24879d1 -- 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 1/2] mv submodule: respect ignore_errors for errors in submodule code
The error handling via passing around a strbuf is well exercised in the
refs code, so apply that pattern here, too.
Signed-off-by: Stefan Beller
---
builtin/mv.c | 15 ---
submodule.c | 33 -
submodule.h | 6 --
3 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index aeae855..74516f4 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -247,6 +247,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
}
for (i = 0; i < argc; i++) {
+ struct strbuf err = STRBUF_INIT;
const char *src = source[i], *dst = destination[i];
enum update_mode mode = modes[i];
int pos;
@@ -256,9 +257,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
if (rename(src, dst) < 0 && !ignore_errors)
die_errno(_("renaming '%s' failed"), src);
if (submodule_gitfile[i]) {
- if (submodule_gitfile[i] !=
SUBMODULE_WITH_GITDIR)
- connect_work_tree_and_git_dir(dst,
submodule_gitfile[i]);
- if (!update_path_in_gitmodules(src, dst))
+ if ((submodule_gitfile[i] !=
SUBMODULE_WITH_GITDIR &&
+ connect_work_tree_and_git_dir(dst,
submodule_gitfile[i], &err)) ||
+ update_path_in_gitmodules(src, dst, &err)) {
+ if (err.len) {
+ if (ignore_errors) {
+ warning("%s", err.buf);
+ continue;
+ } else
+ die("%s", err.buf);
+ }
+ } else
gitmodules_modified = 1;
}
}
diff --git a/submodule.c b/submodule.c
index 90825e1..ed18d34 100644
--- a/submodule.c
+++ b/submodule.c
@@ -51,7 +51,8 @@ int is_staging_gitmodules_ok(void)
* .gitmodules file. Return 0 only if a .gitmodules file was found, a section
* with the correct path= setting was found and we could update it.
*/
-int update_path_in_gitmodules(const char *oldpath, const char *newpath)
+int update_path_in_gitmodules(const char *oldpath, const char *newpath,
+ struct strbuf *err)
{
struct strbuf entry = STRBUF_INIT;
const struct submodule *submodule;
@@ -59,8 +60,10 @@ int update_path_in_gitmodules(const char *oldpath, const
char *newpath)
if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
return -1;
- if (gitmodules_is_unmerged)
- die(_("Cannot change unmerged .gitmodules, resolve merge
conflicts first"));
+ if (gitmodules_is_unmerged) {
+ strbuf_addf(err, _("Cannot change unmerged .gitmodules, resolve
merge conflicts first"));
+ return -1;
+ }
submodule = submodule_from_path(null_sha1, oldpath);
if (!submodule || !submodule->name) {
@@ -1102,27 +1105,39 @@ int merge_submodule(unsigned char result[20], const
char *path,
}
/* Update gitfile and core.worktree setting to connect work tree and git dir */
-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
+int connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir,
+ struct strbuf *err)
{
+ int ret = 0;
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
const char *real_work_tree = xstrdup(real_path(work_tree));
/* Update gitfile */
strbuf_addf(&file_name, "%s/.git", work_tree);
- write_file(file_name.buf, "gitdir: %s",
- relative_path(git_dir, real_work_tree, &rel_path));
+ if (write_file_gently(file_name.buf, "gitdir: %s",
+ relative_path(git_dir, real_work_tree, &rel_path))) {
+ strbuf_addf(err, _("could not write .git file (%s): %s"),
+ file_name.buf, strerror(errno));
+ ret = -1;
+ goto out;
+ }
/* Update core.worktree setting */
strbuf_reset(&file_name);
strbuf_addf(&file_name, "%s/config", git_dir);
- git_config_set_in_file(file_name.buf, "core.worktree",
- relative_path(real_work_tree, git_dir,
-&rel_path));
+ if (git_config_set_in_file_gently(file_name.buf, "core.worktree",
+ relative_path(real_work_tree, git_dir, &rel_path))) {
+ strbuf_addf(err, _("could not set core.worktree in %s"
[PATCH 2/2] mv: allow moving nested submodules
When directories are moved using `git mv` all files in the directory
have been just moved, but no further action was taken on them. This
was done by assigning the mode = WORKING_DIRECTORY to the files
inside a moved directory.
submodules however need to update their link to the git directory as
well as updates to the .gitmodules file. By removing the condition of
`mode != INDEX` (the remaining modes are BOTH and WORKING_DIRECTORY) for
the required submodule actions, we perform these for submodules in a
moved directory.
Signed-off-by: Stefan Beller
---
builtin/mv.c | 39 ++-
t/t7001-mv.sh | 16
2 files changed, 38 insertions(+), 17 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index 74516f4..2deb95b 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -253,23 +253,28 @@ int cmd_mv(int argc, const char **argv, const char
*prefix)
int pos;
if (show_only || verbose)
printf(_("Renaming %s to %s\n"), src, dst);
- if (!show_only && mode != INDEX) {
- if (rename(src, dst) < 0 && !ignore_errors)
- die_errno(_("renaming '%s' failed"), src);
- if (submodule_gitfile[i]) {
- if ((submodule_gitfile[i] !=
SUBMODULE_WITH_GITDIR &&
- connect_work_tree_and_git_dir(dst,
submodule_gitfile[i], &err)) ||
- update_path_in_gitmodules(src, dst, &err)) {
- if (err.len) {
- if (ignore_errors) {
- warning("%s", err.buf);
- continue;
- } else
- die("%s", err.buf);
- }
- } else
- gitmodules_modified = 1;
- }
+ if (show_only)
+ continue;
+ if (mode != INDEX &&
+ rename(src, dst) < 0) {
+ if (ignore_errors)
+ continue;
+ die_errno(_("renaming '%s' failed"), src);
+ }
+
+ if (submodule_gitfile[i]) {
+ if ((submodule_gitfile[i] != SUBMODULE_WITH_GITDIR &&
+ connect_work_tree_and_git_dir(dst,
submodule_gitfile[i], &err)) ||
+ update_path_in_gitmodules(src, dst, &err)) {
+ if (err.len) {
+ if (ignore_errors) {
+ warning("%s", err.buf);
+ continue;
+ } else
+ die("%s", err.buf);
+ }
+ } else
+ gitmodules_modified = 1;
}
if (mode == WORKING_DIRECTORY)
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 4008fae..4a2570e 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -292,6 +292,9 @@ test_expect_success 'setup submodule' '
echo content >file &&
git add file &&
git commit -m "added sub and file" &&
+ mkdir -p deep/directory/hierachy &&
+ git submodule add ./. deep/directory/hierachy/sub &&
+ git commit -m "added another submodule" &&
git branch submodule
'
@@ -475,4 +478,17 @@ test_expect_success 'mv -k does not accidentally destroy
submodules' '
git checkout .
'
+test_expect_success 'moving a submodule in nested directories' '
+ (
+ cd deep &&
+ git mv directory ../ &&
+ # git status would fail if the update of linking git dir to
+ # work dir of the submodule failed.
+ git status &&
+ git config -f ../.gitmodules
submodule.deep/directory/hierachy/sub.path >../actual &&
+ echo "directory/hierachy/sub" >../expect
+ ) &&
+ test_cmp actual expect
+'
+
test_done
--
2.8.1.211.g24879d1
--
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 0/2 v4] xdiff: implement empty line chunk heuristic
On Mon, Apr 18, 2016 at 2:22 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>>> OK, so perhaps either of you two can do a final version people can
>>> start having fun with?
>>
>> Here we go. I squashed in your patch, although with a minor change:
>>
>> - if ((flags & XDF_SHORTEST_LINE_HEURISTIC)) {
>> + if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
>>
>> We did not need that in the "shortest line" heuristic as we know
>> a line with the shortest line length must exist. We do not know about
>> empty lines though.
>
> Makes sense. The last hunk of
>
> $ git show 9614b8dcf -- update-cache.c
>
> gives an unexpected result without "&& blank_lines" above. Lack of
> "&& blank_lines" happens to make the result slightly easier to read,
> but at the cost of having an extra line in the hunk.
So without the blank_lines check you get (A):
@@ -271,15 +279,14 @@ int main(int argc, char **argv)
if (!verify_path(path)) {
fprintf(stderr, "Ignoring path %s\n", argv[i]);
continue;
-}
-if (add_file_to_cache(path)) {
-fprintf(stderr, "Unable to add %s to
database\n", path);
-goto out;
}
+if (add_file_to_cache(path))
+usage("Unable to add %s to database", path);
}
...
and with the heuristic you get (B):
@@ -272,14 +280,13 @@ int main(int argc, char **argv)
@@ -272,14 +280,13 @@ int main(int argc, char **argv)
fprintf(stderr, "Ignoring path %s\n", argv[i]);
continue;
}
-if (add_file_to_cache(path)) {
-fprintf(stderr, "Unable to add %s to
database\n", path);
-goto out;
-}
+if (add_file_to_cache(path))
+usage("Unable to add %s to database", path);
}
...
In case of (A) the compaction heuristic tries to shift the hunk upwards,
stopping at the first empty line or when lines miss match.
As there is no blank line, it goes until the miss match.
Personally I'd find it less readable, because the intent was not to remove
-}
-if (add_file_to_cache(path)) {
-fprintf(stderr, "Unable to add %s to
database\n", path);
-goto out;
but rather remove
-if (add_file_to_cache(path)) {
-fprintf(stderr, "Unable to add %s to
database\n", path);
-goto out;
-}
as that is the logic unit I'd think.
Although you find this instance easier to read the behavior without the
blank_lines check would result in
Shift hunk upward as much as possible, stop at the first empty line.
For hunks without empty line this just becomes
Shift hunk upward as much as possible.
which is 50:50 for looking good, so we kept the old behavior as
that is just as good.
Thanks,
Stefan
>
> 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 0/2] WAS: [PATCH] mv: allow moving nested submodules
Stefan Beller writes: > A single patch evolves into a series. Power of code inspection to see bugs that are not reported, perhaps ;-)? I wonder if we can come up with test cases to cover these potential issues that are addressed in [1/2]? 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 0/7] fix checking out a being-rebased branch
On Tue, Apr 19, 2016 at 12:42 AM, Junio C Hamano wrote: >> Another option is leave wt_status_get_state() alone, factor out the >> rebase-detection code and use that for worktree/checkout. We save a >> few syscalls this way too. >> >> Comments? >> >> [01/07] path.c: add git_common_path() and strbuf_git_common_path() >> [02/07] worktree.c: store "id" instead of "git_dir" >> [03/07] path.c: refactor and add worktree_git_path() >> [04/07] worktree.c: add worktree_git_path_..._head() > > I actually wondered how ky/branch-[dm]-worktree topics to avoid > "branch -d" and "branch -m" from removing or renaming a branch that > is checked out in other worktrees from screwing them up. There may > be a missed opportunity to clean them up with using these? branch-m-worktree uses info populated at get_worktrees() phase. branch-d-worktree uses find_shared_symref() which is modified in this series in order to detect rebase branch. So both topics have the same problem when a branch is being rebased and if I do it right, I'll fix them both without extra code. Actually I may need to check branch-m-worktree again, renaming a branch under rebase might cause problems, I need to have a closer look at git-rebase*.sh. -- 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
Re: [PATCH v2] git-p4: add P4 jobs to git commit message
Jan Durovec writes:
> When migrating from Perforce to git the information about P4 jobs
> associated with P4 changelists is lost.
>
> Having these jobs listed on messages of related git commits enables smooth
> migration for projects that take advantage of e.g. JIRA integration
> (which uses jobs on Perforce side and parses commit messages on git side).
>
> The jobs are added to the message in the same format as is expected when
> migrating in the reverse direction.
>
> Signed-off-by: Jan Durovec
> ---
Thanks for describing the change more throughly than the previous
round.
Luke, how does this one look?
> git-p4.py | 12 ++
> t/lib-git-p4.sh| 10 +
> t/t9829-git-p4-jobs.sh | 99
> ++
> 3 files changed, 121 insertions(+)
> create mode 100755 t/t9829-git-p4-jobs.sh
>
> diff --git a/git-p4.py b/git-p4.py
> index 527d44b..8f869d7 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2320,6 +2320,15 @@ def extractFilesFromCommit(self, commit):
> fnum = fnum + 1
> return files
>
> +def extractJobsFromCommit(self, commit):
> +jobs = []
> +jnum = 0
> +while commit.has_key("job%s" % jnum):
> +job = commit["job%s" % jnum]
> +jobs.append(job)
> +jnum = jnum + 1
I am not familiar with "Perforce jobs", but I assume that they are
always named as "job" + small non-negative integer in a dense way
and it is OK for this loop to always begin at 0 and immediately stop
when job + num does not exist (i.e. if job7 is missing, it is
guaranteed that we will not see job8).
Shouldn't the formatting be "job%d" % jnum, though, as you are using
jnum as a number that begins at 0 and increments by 1?
> +return jobs
> +
> def stripRepoPath(self, path, prefixes):
> """When streaming files, this is called to map a p4 depot path
> to where it should go in git. The prefixes are either
> @@ -2665,6 +2674,7 @@ def hasBranchPrefix(self, path):
> def commit(self, details, files, branch, parent = ""):
> epoch = details["time"]
> author = details["user"]
> +jobs = self.extractJobsFromCommit(details)
>
> if self.verbose:
> print('commit into {0}'.format(branch))
> @@ -2692,6 +2702,8 @@ def commit(self, details, files, branch, parent = ""):
>
> self.gitStream.write("data < self.gitStream.write(details["desc"])
> +if len(jobs) > 0:
> +self.gitStream.write("\nJobs: %s" % (' '.join(jobs)))
> self.gitStream.write("\n[git-p4: depot-paths = \"%s\": change = %s" %
> (','.join(self.branchPrefixes),
> details["change"]))
> if len(details['options']) > 0:
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index f9ae1d7..3907560 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -160,6 +160,16 @@ p4_add_user() {
> EOF
> }
>
> +p4_add_job() {
Not a new problem in this script, but we'd prefer to spell this as
p4_add_job () {
i.e. a space on both sides of ().
> + name=$1 &&
> + p4 job -f -i <<-EOF
> + Job: $name
> + Status: open
> + User: dummy
> + Description:
> + EOF
> +}
It may be better without $name?
> +test_expect_success 'check log message of changelist with no jobs' '
> + client_view "//depot/... //client/..." &&
> + test_when_finished cleanup_git &&
> + (
> + cd "$git" &&
> + git init . &&
> + git p4 clone --use-client-spec --destination="$git" //depot@all
> &&
> + cat >expect <<-\EOF &&
> +Add file 1
> +[git-p4: depot-paths = "//depot/": change = 1]
> +
> + EOF
As you are using <<- to begin the here document, it is easier on the
eyes if you indented the text with HT, i.e.
cat >expect <<-\EOF &&
Add file 1
[git-p4: depot-paths = "//depot/": change = 1]
EOF
I won't repeat the same for other instances below.
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 2/2] xdiff: implement empty line chunk heuristic
On Mon, Apr 18, 2016 at 02:12:30PM -0700, Stefan Beller wrote:
> +
> + /*
> + * If a group can be moved back and forth, see if there is an
> + * blank line in the moving space. If there is a blank line,
> + * make sure the last blank line is the end of the group.
s/an/a/ on the first line
> + * As we shifted the group forward as far as possible, we only
> + * need to shift it back if at all.
Maybe because I'm reading it as a diff that only contains this hunk and
not the whole rest of the function, but the "we" here confused me. You
mean the earlier, existing loop in xdl_change_compact, right?
Maybe something like:
As we already shifted the group forward as far as possible in the
earlier loop...
would help.
> + if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
> + while (ixs > 0 &&
> +!is_blank_line(recs, ix - 1, flags) &&
> +recs_match(recs, ixs - 1, ix - 1, flags)) {
> + rchg[--ixs] = 1;
> + rchg[--ix] = 0;
> + }
> + }
This turned out to be delightfully simple (especially compared to the
perl monstrosity).
I tried comparing the output to the perl one, but it's not quite the
same. In that one we had to work with the existing hunks and context
lines, so any hunk that got shifted ended up with extra context on one
side, and too little on the other. But here, we can actually bump the
context lines to give the correct amount on both sides, which is good.
I guess this will invalidate old patch-ids, but there's not much to be
done about that.
-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 v6 3/6] verify-tag: change variable name for readability
On Sun, Apr 17, 2016 at 06:26:58PM -0400, santi...@nyu.edu wrote:
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 77f070a..010353c 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -21,20 +21,21 @@ static const char * const verify_tag_usage[] = {
> static int run_gpg_verify(const char *buf, unsigned long size, unsigned
> flags)
> {
> struct signature_check sigc;
> - int len;
> + int payload_size;
> int ret;
>
> memset(&sigc, 0, sizeof(sigc));
>
> - len = parse_signature(buf, size);
> + payload_size = parse_signature(buf, size);
While we're here, can we make payload_size a "size_t"? That is the
return type of parse_signature.
> - if (size == len) {
> + if (size == payload_size) {
That would make this a comparison between "unsigned long" and "size_t",
but I doubt it would be the first such place in git. And it works out in
practice, because "unsigned long" is generally at least as large as
"size_t", and if our buffer is larger than "size_t" can store, we'd have
failed long before, when trying to allocate the buffer.
We could make "size" also a "size_t", but I suspect then we'd just be
bumping the mismatch out to its caller.
-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 v6 0/6] Move PGP verification out of verify-tag
On Sun, Apr 17, 2016 at 06:26:55PM -0400, santi...@nyu.edu wrote: > From: Santiago Torres > > This is a follow up of [1], [2], [3], [4], and [5]. patches 1/6, 2/6 and 3/6 > are the same as the corresponding commits in pu. Aside from the minor point I mentioned, and the ones from Eric, this looks good to me. None of them is that big a deal, but there are enough that it's probably worth one more re-roll to clean them all up. Thanks for sticking with 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 2/2] xdiff: implement empty line chunk heuristic
On Mon, Apr 18, 2016 at 10:03 PM, Jeff King wrote:
> On Mon, Apr 18, 2016 at 02:12:30PM -0700, Stefan Beller wrote:
>
>> +
>> + /*
>> + * If a group can be moved back and forth, see if there is an
>> + * blank line in the moving space. If there is a blank line,
>> + * make sure the last blank line is the end of the group.
>
> s/an/a/ on the first line
So it looks like I'll be resending another version for this series tomorrow.
Thanks for pointing this out!
>
>> + * As we shifted the group forward as far as possible, we only
>> + * need to shift it back if at all.
>
> Maybe because I'm reading it as a diff that only contains this hunk and
> not the whole rest of the function, but the "we" here confused me. You
> mean the earlier, existing loop in xdl_change_compact, right?
>
> Maybe something like:
>
> As we already shifted the group forward as far as possible in the
> earlier loop...
>
> would help.
I'll see to get rid of the 'we', otherwise I'll stick with your suggestion.
>
>> + if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
>> + while (ixs > 0 &&
>> +!is_blank_line(recs, ix - 1, flags) &&
>> +recs_match(recs, ixs - 1, ix - 1, flags)) {
>> + rchg[--ixs] = 1;
>> + rchg[--ix] = 0;
>> + }
>> + }
>
> This turned out to be delightfully simple (especially compared to the
> perl monstrosity).
>
> I tried comparing the output to the perl one, but it's not quite the
> same. In that one we had to work with the existing hunks and context
> lines, so any hunk that got shifted ended up with extra context on one
> side, and too little on the other. But here, we can actually bump the
> context lines to give the correct amount on both sides, which is good.
>
> I guess this will invalidate old patch-ids, but there's not much to be
> done about that.
For the record:
I thought about "optimal hunk separation" for a while, specially during my
bike commute. And while this heuristic seems to be a good fit for most of
the cases inspected, we can do better (in the future).
I am convinced the better way to do it is like this:
Calculate the entropy for each line and take the last line with the
lowest entropy as the last line of the hunk.
That heuristic requires more compute though as it will be hard to compute
the entropy for the line. To do that I would imagine, we'd need to loop over
the whole file and count the occurrences for each char (byte) and then
take the negative log of (#number of that byte / #number of bytes in file) [1].
This would model our actual goal a bit more closely to split at parts, where
there is low information density (the definition of entropy).
One example Jacob pointed out was a thing like
/**
* Comment here. Over
* more lines.
*
+ * Add line here with a blank line
+ *
+ * in between and a trailing blank after.
+ *
*/
I think we had cases like this in the kernel tree and else where,
and for a human it is clear to break after the last "empty line"
(which for comments starts with " * "). To detect those we can use
the entropy as it doesn't convey lots of information.
(git show e1f7037167323461c0415447676262dcb)
It also keeps the false positives out, Jacob pointed at
85ed2f32064b82e541fc7dcf2b0049a05 IIRC, which was bad with
the shortest lines only, but I'd imagine the entropy based
heuristic will do better there.
[1] https://en.wikipedia.org/wiki/Entropy_(information_theory)
Thanks for the review,
Stefan
>
> -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
