Git clonebundles

2017-01-30 Thread Stefan Saasen 
is feasible.
* Uniform approach for the supported transports: Our proof-of-concept
only supports HTTP as
  a transport. Ideally the clonebundle capability could be supported by all
  available transports (of which at least ssh would be highly desirable).
* Bundle manifest and bundle download: It is unclear whose responsibility it is
  to generate the bundle manifest with the bundle download URLs. Most likely the
  bundle files will be served using a webserver or CDN, so download
URL generation
  should not be a core git responsibility. For hosting purpose we envision that
  the bundle manifest might contain dynamic download URLs with personalised
  access tokens with expiry.
* Bundle generation: Similar to the above it is unclear how bundle
generation is handled.
  For hosting purposes, the operator would likely want to influence
when and how bundles are generated.



Prior art
~

Our proof-of-concept is built on top of ideas that have been
circulating for a while. We are aware of a number of proposed changes
in this space:


* Jeff King's work on network bundles:
https://github.com/peff/git/commit/17e2409df37edd0c49ef7d35f47a7695f9608900
* Nguyễn Thái Ngọc Duy's work on "[PATCH 0/8] Resumable clone
revisited, proof of concept":
https://www.spinics.net/lists/git/msg267260.html
* Resumable clone work by Kevin Wern:
https://public-inbox.org/git/1473984742-12516-1-git-send-email-kevin.m.w...@gmail.com/


Whilst the above mentioned proposals/proposed changes are in a similar
space, I would be interest to understand whether there is any
consensus on the general idea of supporting static bundle files as a
mechanism to seed a repository?
I would also appreciate any pointers to other discussions in this area.


Best regards,
Stefan Saasen & Erik van Zijst; Atlassian Bitbucket

Re: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects

2015-04-21 Thread Stefan Saasen 
> Perhaps companies like Atlassian that rely on the stability of the
> open source Git can spare some resources and join forces with like
> minded folks on LTS of older maintenance tracks, if they are truly
> interested in.

We certainly can and would like to. I'm not entirely sure what that
would entail though?

>From reading through $gmane/264365 I've identified the following
responsibilities/opportunities to help:

>- Monitor "git log --first-parent maint-lts..master" and find
>  the tip of topic branches that need to be down-merged;
>
>- Down-merge such topics to maint-lts; this might involve
>  cherry-picking instead of merge, as the bugfix topics may
>  originally be done on the codebase newer than maint-lts;

and more importantly testing the maint-lts version to ensure
backported changes don't introduce regressions and the maint-lts
branch is stable.

This suggests specific, spaced LTS versions but in the same thread you
mention maint-2.1or maint-2.2.
So a different model could be maintaining old versions in a sliding
window fashion (e.g. critical issues would be backported to the last 6
months worth of git releases).

Maybe I'm getting ahead of myself here :)

Anyway, long story short. We're interested to help but I'm not
entirely sure what that would look like at the moment. Are there
formed ideas floating around or would you be looking for some form of
proposal instead?

Cheers,
Stefan
--
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: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects

2015-04-21 Thread Stefan Saasen 
>> I've noticed Peff's patches on pu which suggest they will be available
>> in git 2.5?
>
> Being on 'pu' (or 'next' for that matter) is not a suggestion for a
> change to appear in any future version at all, even though it often
> means that it would soon be merged to 'master' and will be in the
> upcoming release to be on 'next' in early part of a development
> cycle.  Some larger topics would stay on 'next' for a few cycles.
>
>> Do you Junio, have plans to merge them to maint (2.3.x) and/or next (2.4)?
>
> The topic will hopefully be merged to 'master' after 2.4 final is
> released end of this month, down to 'maint' early May and will ship
> with 2.4.1, unless there is unforeseen issues discovered in the
> change while people try it out while it is in 'next' (which will
> happen today, hopefully).

Thanks for the clarification Junio.
--
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: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects

2015-04-20 Thread Stefan Saasen 
>> If it is critical to some people, they can downmerge to their custom
>> old installations of Git they maintain with ease, of course, and
>> that "with ease" part is the reason why I try to apply fixes to tip
>> of the original topic branch even though they were merged to the
>> mainline eons ago ;-).
>
> I think it is a bigger deal for folks who do not ship a custom
> installation, but expect to ship a third-party system that interacts
> with whatever version of git their customers happen to have (in which
> case they can only recommend their customers to upgrade).

Yes, this is the situation we are facing. We allow our customers to
use the git version that is supported/available on their OS (within a
certain range of supported versions) so our customers usually don't
compile from source.

> Either way, though, I do not think it is the upstream Git project's
> problem.

That's fair enough, I was mostly enquiring about the official git
versions this will land in so that we can advise customers what git
version to use (or not to use).

I've noticed Peff's patches on pu which suggest they will be available
in git 2.5?
Do you Junio, have plans to merge them to maint (2.3.x) and/or next (2.4)?

While I certainly agree that this is specific to Git on NFS and not a
more widespread git performance problem, I'd love to be able to
message something other than "skip all the git version between and
including git 2.2 - 2.4".

I appreciate your consideration and thanks again for the swift response on this.

Cheers,
Stefan
--
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 1/2] sha1_file: freshen pack objects before loose

2015-04-20 Thread Stefan Saasen 
I didn't expect anything else (as the patch is the same as the
previous one) but I verified that applying this patch has the desired
effect (https://bitbucket.org/snippets/ssaasen/9AXg).

Thanks for the fix Jeff.

On 21 April 2015 at 05:54, Jeff King  wrote:
> When writing out an object file, we first check whether it
> already exists and if so optimize out the write. Prior to
> 33d4221, we did this by calling has_sha1_file(), which will
> check for packed objects followed by loose. Since that
> commit, we check loose objects first.
>
> For the common case of a repository whose objects are mostly
> packed, this means we will make a lot of extra access()
> system calls checking for loose objects. We should follow
> the same packed-then-loose order that all of our other
> lookups use.
>
> Reported-by: Stefan Saasen 
> Signed-off-by: Jeff King 
> ---
>  sha1_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 88f06ba..822aaef 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -3014,7 +3014,7 @@ int write_sha1_file(const void *buf, unsigned long len, 
> const char *type, unsign
> write_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);
> if (returnsha1)
> hashcpy(returnsha1, sha1);
> -   if (freshen_loose_object(sha1) || freshen_packed_object(sha1))
> +   if (freshen_packed_object(sha1) || freshen_loose_object(sha1))
> return 0;
> return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
>  }
> --
> 2.4.0.rc2.384.g7297a4a
>
--
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] sha1_file: only freshen packs once per run

2015-04-20 Thread Stefan Saasen 
I can confirm that this patch is equivalent to the previous one.

https://bitbucket.org/snippets/ssaasen/9AXg shows both the timing and
the NFS stats showing the effect of applying this patch.

Thanks for the fix Jeff!

Cheers,
Stefan

On 21 April 2015 at 05:55, Jeff King  wrote:
> Since 33d4221 (write_sha1_file: freshen existing objects,
> 2014-10-15), we update the mtime of existing objects that we
> would have written out (had they not existed). For the
> common case in which many objects are packed, we may update
> the mtime on a single packfile repeatedly. This can result
> in a noticeable performance problem if calling utime() is
> expensive (e.g., because your storage is on NFS).
>
> We can fix this by keeping a per-pack flag that lets us
> freshen only once per program invocation.
>
> An alternative would be to keep the packed_git.mtime flag up
> to date as we freshen, and freshen only once every N
> seconds. In practice, it's not worth the complexity. We are
> racing against prune expiration times here, which inherently
> must be set to accomodate reasonable program running times
> (because they really care about the time between an object
> being written and it becoming referenced, and the latter is
> typically the last step a program takes).
>
> Signed-off-by: Jeff King 
> ---
> Hopefully I didn't botch the flag logic again. :) I tested with "strace
> -c" myself this time, so I think it is good.
>
>  cache.h | 1 +
>  sha1_file.c | 9 -
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/cache.h b/cache.h
> index 3d3244b..72c6888 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1174,6 +1174,7 @@ extern struct packed_git {
> int pack_fd;
> unsigned pack_local:1,
>  pack_keep:1,
> +freshened:1,
>  do_not_close:1;
> unsigned char sha1[20];
> /* something like ".git/objects/pack/x.pack" */
> diff --git a/sha1_file.c b/sha1_file.c
> index 822aaef..26b9b2b 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2999,7 +2999,14 @@ static int freshen_loose_object(const unsigned char 
> *sha1)
>  static int freshen_packed_object(const unsigned char *sha1)
>  {
> struct pack_entry e;
> -   return find_pack_entry(sha1, &e) && freshen_file(e.p->pack_name);
> +   if (!find_pack_entry(sha1, &e))
> +   return 0;
> +   if (e.p->freshened)
> +   return 1;
> +   if (!freshen_file(e.p->pack_name))
> +   return 0;
> +   e.p->freshened = 1;
> +   return 1;
>  }
>
>  int write_sha1_file(const void *buf, unsigned long len, const char *type, 
> unsigned char *returnsha1)
> --
> 2.4.0.rc2.384.g7297a4a
--
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: [BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects

2015-04-17 Thread Stefan Saasen 
> If it's not a problem, I'd love to see timings for your case with just
> the first patch, and then with both.

Thanks for the swift response, much appreciated Jeff!

Here are the timings for the two patches:

Patch 1 on top of 33d4221c79

   Elapsed   System  User
 Min.   :6.110   Min.   :0.6700   Min.   :0.3600
 1st Qu.:6.580   1st Qu.:0.6900   1st Qu.:0.3900
 Median :7.260   Median :0.7100   Median :0.4100
 Mean   :7.347   Mean   :0.7248   Mean   :0.4214
 3rd Qu.:8.000   3rd Qu.:0.7400   3rd Qu.:0.4600
 Max.   :8.860   Max.   :0.8700   Max.   :0.5100

I've had to slightly tweak your second patch (`freshened` was never
set) but applying the modified patch yielded even better results
compared to patch 1:

   Elapsed   System  User
 Min.   :0.38   Min.   :0.03000   Min.   :0.2900
 1st Qu.:0.38   1st Qu.:0.04000   1st Qu.:0.3100
 Median :0.39   Median :0.06000   Median :0.3200
 Mean   :0.43   Mean   :0.05667   Mean   :0.3519
 3rd Qu.:0.42   3rd Qu.:0.07000   3rd Qu.:0.3600
 Max.   :0.68   Max.   :0.08000   Max.   :0.5700

This is pretty much back to the "before" state.
The graph really tells the whole story:
https://bytebucket.org/snippets/ssaasen/GeRE/raw/7367353a58c50ccd7c493af40ffb6ba1533e1490/git-merge-timing-patched.png
(After is the change in #33d4221, Before the parent of #33d4221 and so on)
The graph and the NFS stats can be found here:
https://bitbucket.org/snippets/ssaasen/GeRE

My tweaked version of your second patch is:

diff --git a/cache.h b/cache.h
index 51ee856..8982055 100644
--- a/cache.h
+++ b/cache.h
@@ -1168,6 +1168,7 @@ extern struct packed_git {
int pack_fd;
unsigned pack_local:1,
 pack_keep:1,
+   freshened:1,
 do_not_close:1;
unsigned char sha1[20];
/* something like ".git/objects/pack/x.pack" */
diff --git a/sha1_file.c b/sha1_file.c
index bc6322e..c0ccd4b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2999,7 +2999,11 @@ static int freshen_loose_object(const unsigned
char *sha1)
 static int freshen_packed_object(const unsigned char *sha1)
 {
struct pack_entry e;
-   return find_pack_entry(sha1, &e) && freshen_file(e.p->pack_name);
+   if (!find_pack_entry(sha1, &e))
+  return 0;
+   if (e.p->freshened)
+  return 1;
+   return e.p->freshened = freshen_file(e.p->pack_name);
 }

 int write_sha1_file(const void *buf, unsigned long len, const char
*type, unsigned char *returnsha1)



The only change is that I assign the result of `freshen_file` to the
`freshened` flag.

I've only ran this with the test case I was using before but it looks
like this is pretty much fixing the merge time changes we observed.

Thanks again for the swift response. I've got my test setup sitting
here, happy to rerun the tests if that'd be useful.

Is there a chance to backport those changes to the 2.2+ branches?

> You may also be interested in:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/266370
>
> which addresses another performance problem related to the
> freshen/recent code in v2.2.

Thanks for the pointer, I'll have a look at that as well.

Cheers,
Stefan
--
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

[BUG] Performance regression due to #33d4221: write_sha1_file: freshen existing objects

2015-04-17 Thread Stefan Saasen 
We became aware of slow merge times with the following setup:

The merge is created in a temporary location that uses alternates. The
temporary repository is on a local disk, the alternate object database
on an NFS mount.

After some investigation we believe that #33d4221 (present in git
2.2.0, absent in 2.1.4) is causing this regression in merge time.

The following are merge times (in seconds) with git@33d4221~
(2.1.2-393-gabcb865) (before the change)

  Elapsed SystemUser
 Min.   :0.3700   Min.   :0.04000   Min.   :0.3000
 1st Qu.:0.3800   1st Qu.:0.05000   1st Qu.:0.3100
 Median :0.4000   Median :0.06000   Median :0.3300
 Mean   :0.4295   Mean   :0.05905   Mean   :0.3519
 3rd Qu.:0.4600   3rd Qu.:0.07000   3rd Qu.:0.3600
 Max.   :0.5900   Max.   :0.09000   Max.   :0.4900


The following are merge times with git@33d4221 (2.1.2-394-g33d4221):

  Elapsed SystemUser
 Min.   : 8.58   Min.   :1.46   Min.   :0.4400
 1st Qu.: 9.63   1st Qu.:1.60   1st Qu.:0.4400
 Median :10.64   Median :1.66   Median :0.4800
 Mean   :10.50   Mean   :1.69   Mean   :0.4986
 3rd Qu.:11.13   3rd Qu.:1.81   3rd Qu.:0.5000
 Max.   :13.96   Max.   :2.05   Max.   :0.6700


As you can see the merge times are an order of magnitude slower after
the change.

The effect of  #33d4221 can be seen when strace'ing the merge:

Running strace on git@33d4221 yields
% time seconds  usecs/call callserrors syscall
-- --- --- - - 
 70.790.714852 178  4018   utime
 14.730.148789   3 50141 50123 lstat
 13.880.140198  17  8074  8067 access
  0.240.002455 614 4   rename
  0.150.001493   3   577   write
  0.060.000618  1065   close
  0.040.000453   3   152   brk
  0.040.000433  2716   mkdir
  0.030.000310   841   fstat


Running strace on git@33d4221~ yields

% time seconds  usecs/call callserrors syscall
-- --- --- - - 
 98.370.138516   3 50141 50123 lstat
  0.920.001292   2   577   write
  0.370.000520  143831 access
  0.180.000252  36 7   getcwd
  0.170.000237   73620 stat
  0.000.00   040   read
  0.000.00   08730 open


My current hypothesis is that the additional `access`, but more
importantly the additional `utime` calls are responsible in the
increased merge times that we see.
NFS stats on the server for the tests seem to confirm this (see
nfsstat-{after,before}-change.txt on
https://bitbucket.org/snippets/ssaasen/oend).
This is certainly due to the fact that this will all happen over NFS
but in 2.1.4 this worked fine and starting with 2.2 this has become
very slow.

Looking at the detailed strace shows that utime will be called
repeatedly in same cases (e.g.
https://bitbucket.org/snippets/ssaasen/oend shows an example where the
same packfile will be updated more than 4000 times in a single merge).

http://www.spinics.net/lists/git/msg240106.html discusses a potential
improvement for this case. Would that be an acceptable avenue to
improve this situation?

Best regards,
Stefan Saasen
--
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] gc: support temporarily preserving garbage

2014-11-17 Thread Stefan Saasen 
On 18 November 2014 08:34, Jeff King  wrote:

>>
>> I am not sure if this much of code churn is warranted to work around
>> issues that only happen on repositories on NFS servers that do not
>> keep open-but-deleted files available.  Is it an option to instead
>> have a copy of repository locally off NFS?
>
> I think it is also not sufficient. This patch seems to cover only
> objects. But we assume that we can atomically rename() new versions of
> files into place whenever we like without disrupting existing readers.
> This is the case for ref updates (and packed-refs), as well as the index
> file.  The destination end of the rename is an unlink() in disguise, and
> would be susceptible to the same problems.

I’m going out on a limb here as my NFS knowledge is rather shallow but a
rename should be atomic even on NFS.

"The RENAME operation must be atomic to the client.”
(https://www.ietf.org/rfc/rfc1813.txt: 3.3.14)

Does git do something differently here for that not to be the case?
--
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] gc: support temporarily preserving garbage

2014-11-14 Thread Stefan Saasen 
On 15 November 2014 10:01, Junio C Hamano  wrote:

>>  23 files changed, 375 insertions(+), 7 deletions(-)
>
> I am not sure if this much of code churn is warranted to work around
> issues that only happen on repositories on NFS servers that do not
> keep open-but-deleted files available.  Is it an option to instead
> have a copy of repository locally off NFS?

Unfortunately not providing delete-on-last-close semantics is true for
most (all?) NFS servers that are accessed by multiple clients. NFS v3
is stateless so the server has no way of tracking open files accross
clients and the silly rename work around only works within a single
client. NFS v4 could support delete-on-last-close semantics but I’m
not sure if there are actual implementations providing that.
(This is based on my admittedly limited understanding of NFS, I’d love
to learn that I’ve got this wrong).

We are susceptible to the same problem accessing shared repositories
from multiple clients so we certainly appreciate the use case.
Unfortunately copying repositories to the local nodes is not something
that’d be feasible.

Is there another/better approach solving this problem?
--
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: Add a bugzilla website

2014-01-12 Thread Stefan Saasen 
> In any case, adding value to the existing process is hard (because it
> works quite well!) and probably requires significantly more work to
> even understand what that value might look like. This, I think, is the
> key reason it is hard to truly get started with any bug tracking
> solution; the solution is not obvious, and the current (very
> customised) workflow is not supported directly by any tool.
>
> Regards,
>
> Andrew Ardill
>
> [1] https://git-scm.atlassian.net


I think you summarised the challenges very well and I don’t think there is an
obvious answer to that.

I’d just like to offer any help that I or we (Atlassian) could give you. Given
your experience with JIRA I’m sure that you’ve got everything covered, but if
you need anything, please ping me.

Re-reading the old discussions there was a concern that the issue data was not
available, I just wanted to chime in and mention that if you are using a JIRA
OnDemand (e.g. the https://git-scm.atlassian.net) instance you can get the full
backup of your data (including any attachments, data available as XML) and
there is a JIRA REST API as well.

I know that this is just a very tangential concern given the challenges of
making an issue tracker work with an email based workflow that has proven quite
successful but I at least wanted to address that and offer any help you might
need.


Cheers,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] mergetools/diffmerge: support DiffMerge as a git mergetool

2013-10-12 Thread Stefan Saasen 
DiffMerge is a non-free (but gratis) tool that supports OS X, Windows and Linux.

See http://www.sourcegear.com/diffmerge/

DiffMerge includes a script `/usr/bin/diffmerge` that can be used to launch the
graphical compare tool.

This change adds mergetool support for DiffMerge and adds 'diffmerge' as an
option to the mergetool help.

Signed-off-by: Stefan Saasen 
Acked-by: David Aguilar 
---
 contrib/completion/git-completion.bash |  2 +-
 git-mergetool--lib.sh  |  3 ++-
 mergetools/diffmerge   | 15 +++
 3 files changed, 18 insertions(+), 2 deletions(-)
 create mode 100644 mergetools/diffmerge

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index e1b7313..07b0ba5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1188,7 +1188,7 @@ _git_diff ()
__git_complete_revlist_file
 }
 
-__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff
+__git_mergetools_common="diffuse diffmerge ecmerge emerge kdiff3 meld opendiff
tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3 
codecompare
 "
 
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index feee6a4..858bc37 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -250,7 +250,8 @@ list_merge_tool_candidates () {
else
tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
fi
-   tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3 
codecompare"
+   tools="$tools gvimdiff diffuse diffmerge ecmerge"
+   tools="$tools p4merge araxis bc3 codecompare"
fi
case "${VISUAL:-$EDITOR}" in
*vim*)
diff --git a/mergetools/diffmerge b/mergetools/diffmerge
new file mode 100644
index 000..85ac720
--- /dev/null
+++ b/mergetools/diffmerge
@@ -0,0 +1,15 @@
+diff_cmd () {
+   "$merge_tool_path" "$LOCAL" "$REMOTE" >/dev/null 2>&1
+}
+
+merge_cmd () {
+   if $base_present
+   then
+   "$merge_tool_path" --merge --result="$MERGED" \
+   "$LOCAL" "$BASE" "$REMOTE"
+   else
+   "$merge_tool_path" --merge \
+   --result="$MERGED" "$LOCAL" "$REMOTE"
+   fi
+   status=$?
+}
-- 
1.8.2.3

--
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] mergetools/diffmerge: support DiffMerge as a git mergetool

2013-10-12 Thread Stefan Saasen 
Not a problem. I'll change it to:

tools="$tools gvimdiff diffuse diffmerge ecmerge"
tools="$tools p4merge araxis bc3 code compare"

and send a v3. Thanks for the review David.


On 13 October 2013 06:55, David Aguilar  wrote:
> Thanks for the re-roll.  We're very close; see below.
>
> On Fri, Oct 11, 2013 at 10:01 PM, Stefan Saasen  wrote:
>> DiffMerge is a non-free (but gratis) tool that supports OS X, Windows and 
>> Linux.
>>
>> See http://www.sourcegear.com/diffmerge/
>>
>> DiffMerge includes a script `/usr/bin/diffmerge` that can be used to launch 
>> the
>> graphical compare tool.
>>
>> This change adds mergetool support for DiffMerge and adds 'diffmerge' as an
>> option to the mergetool help.
>>
>> Signed-off-by: Stefan Saasen 
>> Acked-by: David Aguilar 
>> ---
>>  contrib/completion/git-completion.bash |  2 +-
>>  git-mergetool--lib.sh  |  3 ++-
>>  mergetools/diffmerge   | 15 +++
>>  3 files changed, 18 insertions(+), 2 deletions(-)
>>  create mode 100644 mergetools/diffmerge
>>
>> diff --git a/contrib/completion/git-completion.bash 
>> b/contrib/completion/git-completion.bash
>> index e1b7313..07b0ba5 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -1188,7 +1188,7 @@ _git_diff ()
>> __git_complete_revlist_file
>>  }
>>
>> -__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff
>> +__git_mergetools_common="diffuse diffmerge ecmerge emerge kdiff3 meld 
>> opendiff
>> tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3 
>> codecompare
>>  "
>>
>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>> index feee6a4..0fcb253 100644
>> --- a/git-mergetool--lib.sh
>> +++ b/git-mergetool--lib.sh
>> @@ -250,7 +250,8 @@ list_merge_tool_candidates () {
>> else
>> tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
>> fi
>> -   tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3 
>> codecompare"
>> +   tools="$tools gvimdiff diffuse diffmerge ecmerge "
>> +   tools+="p4merge araxis bc3 codecompare"
>
> I don't believe "+=" is portable across all POSIX shells.
>
> I tried this on "dash" (which is the default /bin/sh on Debian) and it
> was not understood there.
>
> $ f="1 2 3"
> $ f+=" 4"
> /bin/dash: 2: f+= 4: not found
>
> I think we should stick to the tools="$tools ." style of concatenation.
>
> Everything else looks good to me.
>
> Thanks,
> --
> David
--
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 v2] mergetools/diffmerge: support DiffMerge as a git mergetool

2013-10-11 Thread Stefan Saasen 
DiffMerge is a non-free (but gratis) tool that supports OS X, Windows and Linux.

See http://www.sourcegear.com/diffmerge/

DiffMerge includes a script `/usr/bin/diffmerge` that can be used to launch the
graphical compare tool.

This change adds mergetool support for DiffMerge and adds 'diffmerge' as an
option to the mergetool help.

Signed-off-by: Stefan Saasen 
Acked-by: David Aguilar 
---
 contrib/completion/git-completion.bash |  2 +-
 git-mergetool--lib.sh  |  3 ++-
 mergetools/diffmerge   | 15 +++
 3 files changed, 18 insertions(+), 2 deletions(-)
 create mode 100644 mergetools/diffmerge

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index e1b7313..07b0ba5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1188,7 +1188,7 @@ _git_diff ()
__git_complete_revlist_file
 }
 
-__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff
+__git_mergetools_common="diffuse diffmerge ecmerge emerge kdiff3 meld opendiff
tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3 
codecompare
 "
 
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index feee6a4..0fcb253 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -250,7 +250,8 @@ list_merge_tool_candidates () {
else
tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
fi
-   tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3 
codecompare"
+   tools="$tools gvimdiff diffuse diffmerge ecmerge "
+   tools+="p4merge araxis bc3 codecompare"
fi
case "${VISUAL:-$EDITOR}" in
*vim*)
diff --git a/mergetools/diffmerge b/mergetools/diffmerge
new file mode 100644
index 000..85ac720
--- /dev/null
+++ b/mergetools/diffmerge
@@ -0,0 +1,15 @@
+diff_cmd () {
+   "$merge_tool_path" "$LOCAL" "$REMOTE" >/dev/null 2>&1
+}
+
+merge_cmd () {
+   if $base_present
+   then
+   "$merge_tool_path" --merge --result="$MERGED" \
+   "$LOCAL" "$BASE" "$REMOTE"
+   else
+   "$merge_tool_path" --merge \
+   --result="$MERGED" "$LOCAL" "$REMOTE"
+   fi
+   status=$?
+}
-- 
1.8.2.3

--
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] mergetools/diffmerge: support DiffMerge as a git mergetool

2013-10-08 Thread Stefan Saasen 
Thanks for the review David, much appreciated.

> I think this line was already too long in its current form.  Would you mind
> splitting up this long line?

I've updated the patch and had a look at how to avoid repeating the list of
available merge/difftools.

> ... follow it up with a change that generalizes the "list
> of tools" thing so that it can be reused here, possibly.  The
> show_tool_help() function, as used by "git difftool --tool-help" and
> "git mergetool --tool-help", might be a good place to look for
> inspiration.

> We were able to eliminate duplication in the docs (see the handling
> for $(mergetools_txt) in Documentation/Makefile) so it'd be nice if we
> could do the same for git-completion.bash, somehow.

I can think of a number of approaches and I would like to get some feedback.

Firstly I think a similar solution to how the duplication is avoided in the
documentation can't be easily applied to the completion script. Looking at the
script itself (and/or usage docs like
http://git-scm.com/book/en/Git-Basics-Tips-and-Tricks) the recommended way of
using it is by copying the script as-is. That means there won't be a build step
we could rely on unless I've overlooked something?

That leaves a different approach (run- vs. build time) where I can think of two
possible solutions.
The first would be similar to what is being done at the moment by looking at
the MERGE_TOOLS_DIR and in addition considering any custom merge tools
configured. I'm working with the premise that it is a reasonable assumption
that users of the git completion script have a git installation available even
though they may have gotten the script by other means.
For users to still be able to install the script by simply copying it
to any location
on the filesystem the list generation function(s) would either have to
be sourced
from the git installation or duplicated. I suppose the former would need to
take into account that the completion script doesn't necessarily matches the
installed version of git with some potential brittleness around
relying on external
files and directories. The latter doesn't buy us anything as it duplicates even
more code than the current list of available mergetools.

The second approach would be to do something similar to resolving the merge
strategies (in __git_list_merge_strategies) by parsing the output of the `git
merge tool --tools-help` option with a very similar disadvantage that it relies
on the textual output of the help command and doesn't work outside of a git
repository.


I'm currently leaning towards the last approach as it seems less reliant on
implementation details but it doesn't look ideal either and I may be missing
another approach that would be better suited.

> It might be worth leaving the git-completion.bash bits alone in this
> first patch and follow it up with a change that generalizes the "list
> of tools" thing so that it can be reused here, possibly.

To decouple this and adding the diffmerge merge tool option, I'd rather keep the
git-completion change part of the patch. That way the patch is self contained
and covers the change including the completion using the current approach and
doesn't rely on the duplication change. Any concerns around that, otherwise I'll
resend the patch with only the long line fixed?

Cheers,
Stefan


On 6 October 2013 14:21, David Aguilar  wrote:
>
> On Sat, Oct 5, 2013 at 1:29 AM, Stefan Saasen  wrote:
> > DiffMerge is a non-free (but gratis) tool that supports OS X, Windows and 
> > Linux.
> >
> > See http://www.sourcegear.com/diffmerge/
> >
> > DiffMerge includes a script `/usr/bin/diffmerge` that can be used to launch 
> > the
> > graphical compare tool.
> >
> > This change adds mergetool support for DiffMerge and adds 'diffmerge' as an
> > option to the mergetool help.
> >
> > Signed-off-by: Stefan Saasen 
> > ---
> >  contrib/completion/git-completion.bash |  2 +-
> >  git-mergetool--lib.sh  |  2 +-
> >  mergetools/diffmerge   | 15 +++
> >  3 files changed, 17 insertions(+), 2 deletions(-)
> >  create mode 100644 mergetools/diffmerge
> >
> > diff --git a/contrib/completion/git-completion.bash 
> > b/contrib/completion/git-completion.bash
> > index e1b7313..07b0ba5 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -1188,7 +1188,7 @@ _git_diff ()
> > __git_complete_revlist_file
> >  }
> >
> > -__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff
> > +__git_mergetools_common="diffuse diffmerge ecmerge emerge kdiff3 meld 
> > open

[PATCH] mergetools/diffmerge: support DiffMerge as a git mergetool

2013-10-05 Thread Stefan Saasen 
DiffMerge is a non-free (but gratis) tool that supports OS X, Windows and Linux.

See http://www.sourcegear.com/diffmerge/

DiffMerge includes a script `/usr/bin/diffmerge` that can be used to launch the
graphical compare tool.

This change adds mergetool support for DiffMerge and adds 'diffmerge' as an
option to the mergetool help.

Signed-off-by: Stefan Saasen 
---
 contrib/completion/git-completion.bash |  2 +-
 git-mergetool--lib.sh  |  2 +-
 mergetools/diffmerge   | 15 +++
 3 files changed, 17 insertions(+), 2 deletions(-)
 create mode 100644 mergetools/diffmerge

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index e1b7313..07b0ba5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1188,7 +1188,7 @@ _git_diff ()
__git_complete_revlist_file
 }
 
-__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff
+__git_mergetools_common="diffuse diffmerge ecmerge emerge kdiff3 meld opendiff
tkdiff vimdiff gvimdiff xxdiff araxis p4merge bc3 
codecompare
 "
 
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index feee6a4..6d0fa3b 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -250,7 +250,7 @@ list_merge_tool_candidates () {
else
tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
fi
-   tools="$tools gvimdiff diffuse ecmerge p4merge araxis bc3 
codecompare"
+   tools="$tools gvimdiff diffuse diffmerge ecmerge p4merge araxis 
bc3 codecompare"
fi
case "${VISUAL:-$EDITOR}" in
*vim*)
diff --git a/mergetools/diffmerge b/mergetools/diffmerge
new file mode 100644
index 000..85ac720
--- /dev/null
+++ b/mergetools/diffmerge
@@ -0,0 +1,15 @@
+diff_cmd () {
+   "$merge_tool_path" "$LOCAL" "$REMOTE" >/dev/null 2>&1
+}
+
+merge_cmd () {
+   if $base_present
+   then
+   "$merge_tool_path" --merge --result="$MERGED" \
+   "$LOCAL" "$BASE" "$REMOTE"
+   else
+   "$merge_tool_path" --merge \
+   --result="$MERGED" "$LOCAL" "$REMOTE"
+   fi
+   status=$?
+}
-- 
1.8.4.475.g3a5bb13

--
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] mergetool--lib: Fix typo in the merge/difftool help

2013-10-04 Thread Stefan Saasen 
The help text for the `tool` flag should mention:

--tool=

instead of:

--tool-

Signed-off-by: Stefan Saasen 
---
 git-mergetool--lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index feee6a4..e1c7eb1 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -263,7 +263,7 @@ list_merge_tool_candidates () {
 }
 
 show_tool_help () {
-   tool_opt="'git ${TOOL_MODE}tool --tool-'"
+   tool_opt="'git ${TOOL_MODE}tool --tool='"
 
tab='   '
LF='
-- 
1.8.4.474.g128a96c.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] Documentation: distinguish between ref and offset deltas in pack-format

2013-04-11 Thread Stefan Saasen 
eb32d236 introduced the OBJ_OFS_DELTA object that uses a relative offset to
identify the base object instead of the 20-byte SHA1 reference. The pack file
documentation only mentions the SHA1 based reference in its description of the
deltified object entry.

Update the pack format documentation to clarify that the deltified object
representation refers to its base using either a relative negative offset or
the absolute SHA1 identifier.

Signed-off-by: Stefan Saasen 
---
 Documentation/technical/pack-format.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/technical/pack-format.txt 
b/Documentation/technical/pack-format.txt
index 0e37ec9..61445c2 100644
--- a/Documentation/technical/pack-format.txt
+++ b/Documentation/technical/pack-format.txt
@@ -26,7 +26,9 @@ Git pack format
 
  (deltified representation)
  n-byte type and length (3-bit type, (n-1)*7+4-bit length)
- 20-byte base object name
+ 20-byte base object name if OBJ_REF_DELTA or a negative relative
+ offset from the delta object's position in the pack if this
+ is an OBJ_OFS_DELTA object
  compressed delta data
 
  Observation: length of each object is encoded in a variable
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html