git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] hash: Allow building with the external sha1dc library

From
Takashi Iwai <tiwai@suse.de>
Date
Aug 1, 2017, 16:12 UTC
Message-ID
<s5hmv7jw9lx.wl-tiwai@suse.de>
In-Reply-To
<xmqqbmnzgu3z.fsf@gitster.mtv.corp.google.com>

On Tue, 01 Aug 2017 17:56:00 +0200, Junio C Hamano wrote:

Show 38 quoted lines
> 
> Takashi Iwai <tiwai@suse.de> writes:
> 
> > On Fri, 28 Jul 2017 17:58:14 +0200,
> > Ævar Arnfjörð Bjarmason wrote:
> >>  ...
> >> * We now have much of the same header code copy/pasted between
> >> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always
> >> including the former but making what it's doing conditional on
> >> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory
> >> glance, but again your commit message doesn't list that among options
> >> considered & discarded.
> >
> > I don't mind either way, there is no perfect solution in this case.
> > As you know, many people think the ifdef ugly no matter how.
> >
> > I leave the decision to maintainer.  Just let me know which option is
> > preferred.
> 
> Yeah, I also found it somewhat confusing to have these two headers
> that look quite similar to each other at the top-level of the tree.
> 
> What's the "conditional" part between the two headers?  Is it just
> whether the header for underlying library is included?  I wonder if
> it's just the matter of adjusting "hash.h" to read like this
> 
>     ...
>     #if defined(DC_SHA1_EXTERNAL)
>    -#include "sha1dc_git_ext.h"
>    +#include <sha1dc/sha1.h>
>    +#include "sha1dc_git.h"
>     #elif  defined(DC_SHA1_SUBMODULE)
>     ...
> 
> or are there heavier tweaks needed that won't be solved by continuing
> along the same line?  As _ext.h variant is included only at this place,
> if we can do with minimum tweaks around here without introducing it,
> it may be ideal, I would think.

Well, a tricky part is that currently sha1dc_git.h is included from sha1dc/sha1.h implicitly by SHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_H definition. IMO, we should stop this, and use the standard inclusion instead, i.e. in hash.h,

#if defined(DC_SHA1_EXTERNAL) #include <sha1dc/sha1.h> #elif defined(DC_SHA1_SUBMODULE) #include "sha1collisiondetection/lib/sha1.h" #else #include "sha1dc/sha1.h" #endif #include "sha1dc_git.h"

In sha1dc_git.h, we'd need another ifdef for DC_SHA1_EXTERNAL to define the own git_SHA1DCInit(), but it's trivial.

Takashi
Previous: Junio C HamanoNext: Ævar Arnfjörð Bjarmason
Message 11 of 13 in “hash: Allow building with the external sha1dc library”
  1. hash: Allow building with the external sha1dc libraryTakashi Iwai, Jul 25, 2017
  2. Junio C HamanoJul 25, 2017
  3. Ævar Arnfjörð BjarmasonJul 28, 2017
  4. Ævar Arnfjörð BjarmasonJul 28, 2017
  5. Junio C HamanoJul 31, 2017
  6. Takashi IwaiAug 1, 2017
  7. Junio C HamanoAug 12, 2017
  8. Takashi IwaiAug 12, 2017
  9. Takashi IwaiAug 1, 2017
  10. Junio C HamanoAug 1, 2017
  11. Takashi IwaiAug 1, 2017
  12. Ævar Arnfjörð BjarmasonAug 1, 2017
  13. Takashi IwaiAug 1, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.